From: Rob Clark <robdclark(a)chromium.org>
virtgpu "vram" GEM objects do not implement obj->get_sg_table(). But
they also don't use drm_gem_map_dma_buf(). In fact they may not even
have guest visible pages. But it is perfectly fine to export and share
with other virtual devices.
Reported-by: Dominik Behr <dbehr(a)chromium.org>
Fixes: 207395da5a97 ("drm/prime: reject DMA-BUF attach when get_sg_table is missing")
Signed-off-by: Rob Clark <robdclark(a)chromium.org>
---
drivers/gpu/drm/drm_prime.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 7352bde299d5..64dd6276e828 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -582,7 +582,12 @@ int drm_gem_map_attach(struct dma_buf *dma_buf,
{
struct drm_gem_object *obj = dma_buf->priv;
- if (!obj->funcs->get_sg_table)
+ /*
+ * drm_gem_map_dma_buf() requires obj->get_sg_table(), but drivers
+ * that implement their own ->map_dma_buf() do not.
+ */
+ if ((dma_buf->ops->map_dma_buf == drm_gem_map_dma_buf) &&
+ !obj->funcs->get_sg_table)
return -ENOSYS;
return drm_gem_pin(obj);
--
2.44.0
This is actually a bit concerning.. importing a host page backed
buffer without guest mapping into a passthru device probably doesn't
work and should be rejected earlier.
I do think we should relax the restriction (either taking my patch or
reverting the commit it fixes) until we work this out properly
(because the original patch is a regression), but importing a buffer
without guest pages into a passthru device can't possibly work
properly. Maybe it works by chance if the host buffer is mapped to
the guest, but that is not guaranteed.
BR,
-R
On Mon, Mar 25, 2024 at 3:35 PM Dominik Behr <dbehr(a)chromium.org> wrote:
>
> It also fixes importing virtgpu blobs into real hardware, for instance amdgpu for DRI_PRIME rendering.
>
> On Fri, Mar 22, 2024 at 2:48 PM Rob Clark <robdclark(a)gmail.com> wrote:
>>
>> From: Rob Clark <robdclark(a)chromium.org>
>>
>> virtgpu "vram" GEM objects do not implement obj->get_sg_table(). But
>> they also don't use drm_gem_map_dma_buf(). In fact they may not even
>> have guest visible pages. But it is perfectly fine to export and share
>> with other virtual devices.
>>
>> Reported-by: Dominik Behr <dbehr(a)chromium.org>
>> Fixes: 207395da5a97 ("drm/prime: reject DMA-BUF attach when get_sg_table is missing")
>> Signed-off-by: Rob Clark <robdclark(a)chromium.org>
>> ---
>> drivers/gpu/drm/drm_prime.c | 7 ++++++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
>> index 7352bde299d5..64dd6276e828 100644
>> --- a/drivers/gpu/drm/drm_prime.c
>> +++ b/drivers/gpu/drm/drm_prime.c
>> @@ -582,7 +582,12 @@ int drm_gem_map_attach(struct dma_buf *dma_buf,
>> {
>> struct drm_gem_object *obj = dma_buf->priv;
>>
>> - if (!obj->funcs->get_sg_table)
>> + /*
>> + * drm_gem_map_dma_buf() requires obj->get_sg_table(), but drivers
>> + * that implement their own ->map_dma_buf() do not.
>> + */
>> + if ((dma_buf->ops->map_dma_buf == drm_gem_map_dma_buf) &&
>> + !obj->funcs->get_sg_table)
>> return -ENOSYS;
>>
>> return drm_gem_pin(obj);
>> --
>> 2.44.0
>>
Il 20/03/24 03:42, Shawn Sung ha scritto:
> From: Hsiao Chien Sung <shawn.sung(a)mediatek.corp-partner.google.com>
>
> Rename files mtk_drm_ddp_comp.c to mtk_ddp_comp.c and
> modify the Makefile accordingly.
>
> Reviewed-by: CK Hu <ck.hu(a)mediatek.com>
> Signed-off-by: Hsiao Chien Sung <shawn.sung(a)mediatek.corp-partner.google.com>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno(a)collabora.com>
Il 20/03/24 03:42, Shawn Sung ha scritto:
> From: Hsiao Chien Sung <shawn.sung(a)mediatek.corp-partner.google.com>
>
> Rename files mtk_drm_crtc.c to mtk_crtc.c and
> modify the Makefile accordingly.
>
> Reviewed-by: CK Hu <ck.hu(a)mediatek.com>
> Signed-off-by: Hsiao Chien Sung <shawn.sung(a)mediatek.corp-partner.google.com>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno(a)collabora.com>
If due to a memory allocation failure mock_chain() returns NULL, it is
passed to dma_fence_enable_sw_signaling() resulting in NULL pointer
dereference there.
Call dma_fence_enable_sw_signaling() only if mock_chain() succeeds.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Fixes: d62c43a953ce ("dma-buf: Enable signaling on fence for selftests")
Signed-off-by: Pavel Sakharov <p.sakharov(a)ispras.ru>
---
drivers/dma-buf/st-dma-fence-chain.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/dma-buf/st-dma-fence-chain.c b/drivers/dma-buf/st-dma-fence-chain.c
index 9c2a0c082a76..ed4b323886e4 100644
--- a/drivers/dma-buf/st-dma-fence-chain.c
+++ b/drivers/dma-buf/st-dma-fence-chain.c
@@ -84,11 +84,11 @@ static int sanitycheck(void *arg)
return -ENOMEM;
chain = mock_chain(NULL, f, 1);
- if (!chain)
+ if (chain)
+ dma_fence_enable_sw_signaling(chain);
+ else
err = -ENOMEM;
- dma_fence_enable_sw_signaling(chain);
-
dma_fence_signal(f);
dma_fence_put(f);
--
2.44.0
Il 19/03/24 08:02, Shawn Sung ha scritto:
> From: Hsiao Chien Sung <shawn.sung(a)mediatek.corp-partner.google.com>
>
> Rename all "mtk_drm_ddp_comp" to "mtk_ddp_comp":
> - To align the naming rule
> - To reduce the code size
>
> Reviewed-by: AngeloGiaocchino Del Regno <angelogioacchino.delregno(a)collabora.com>
Shawn, I don't know if I typoed my own name (which is actually possible, since
I write the tags by hand), or what actually happened to my Reviewed-by tags on
the entire series.
Can you please fix the typo in the tag?
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno(a)collabora.com>
Use this one, please.
Thanks,
Angelo
> Reviewed-by: CK Hu <ck.hu(a)mediatek.com>
> Signed-off-by: Hsiao Chien Sung <shawn.sung(a)mediatek.corp-partner.google.com>
On Fri, Mar 15, 2024 at 06:36:19PM +0100, Alexandre Mergnat wrote:
> On 15/03/2024 16:15, Mark Brown wrote:
> > On Fri, Mar 15, 2024 at 04:05:21PM +0100, Alexandre Mergnat wrote:
> > > > In the register. You only need to reset the gain to -40dB at the start
> > > > of the ramp.
> > > Sorry but I don't understand your logic, I'm not able to implement it...
> > > If I'm at -10dB and doing a ramp to reach -40dB, next time I will read the
> > > register the value will be -40dB.
> > After we've done the ramp and turned the amplifier off we can just
> > restore the desired value? The hardware is not going to care what the
> > volume is while it's not enabled.
> If you do that, HP will be enabled at the saved gain, and after that you
> will do the ramp. To avoid pop, the driver should be rewrite to:
So reset the volume to -40dB prior to turning the amplifier on...
> Read gain in the reg and save it locally
> Set -40dB in the reg
> Enable HP
> Do ramp
...as you yourself suggest?
> > > AFAII from the comment in the code, it's done to avoid the "pop noises".
> > Yes, that's the usual reason to ramp gains. Though if you've just
> > copied the code without checking that it's needed it's possible that
> > this is something that's been fixed in current hardware.
> I did the test at 24dB with and without the "pop filter". Isn't big but I
> ear the pop at the start of the record without the "pop filter".
OK, it probably is still doing something then.
> To be clear, the algo/behavior of this code is an implementation based on
> the 6k+ lines downstream code for this specific audio codec. But the
> shape/style is based on upstreamed drivers like mt6358.c.
The Mediatek code has a bunch of issues, I wouldn't read too much into
something being present in the code.
On Fri, Mar 15, 2024 at 04:05:21PM +0100, Alexandre Mergnat wrote:
> On 15/03/2024 15:30, Mark Brown wrote:
> > > Let me know, when you change de gain to do a ramp down (start from user gain
> > > to gain=-40db), next time for the ramp up, how/where do you find the user
> > > gain ?
> > In the register. You only need to reset the gain to -40dB at the start
> > of the ramp.
> Sorry but I don't understand your logic, I'm not able to implement it...
> If I'm at -10dB and doing a ramp to reach -40dB, next time I will read the
> register the value will be -40dB.
After we've done the ramp and turned the amplifier off we can just
restore the desired value? The hardware is not going to care what the
volume is while it's not enabled.
> This implementation is also done in other MTK audio codec drivers.
Perhaps they should be updated too?
> > > When microphone isn't capturing, the gain read back from the register is
> > > 0dB. I've put some logs in my code and do capture to show how it works:
> > Is this a property of the hardware or a property of your driver?
> At the end of the capture, the gain is set to 0dB by the driver.
> At the start of the capture, the gain is set to the setup gain.
So that's a property of the driver then?
> AFAII from the comment in the code, it's done to avoid the "pop noises".
Yes, that's the usual reason to ramp gains. Though if you've just
copied the code without checking that it's needed it's possible that
this is something that's been fixed in current hardware.