Re: [PATCH RESEND 3/4] drm/mediatek: Set dedicated DMA device and drop custom GEM callbacks

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 11.03.26 um 05:55 schrieb Chen-Yu Tsai:
> On Tue, Mar 10, 2026 at 4:21 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Hi
>>
>> Am 10.03.26 um 04:25 schrieb Chen-Yu Tsai:
>>> In commit 9b54a32c7c6a ("drm/mediatek: mtk_gem: Partial refactor and
>>> use drm_gem_dma_object") the MediaTek DRM driver was refactored to use
>>> drm_gem_dma_object, but custom callbacks were still needed to deal with
>>> using the first device of the pipeline as the DMA device, instead of
>>> the MMSYS device that the DRM driver binds to.
>>>
>>> Turns out there is already partial support for dedicated DMA devices in
>>> the DRM subsystem for PRIME imports. The preceding patches add support
>>> for dedicated DMA devices to the GEM DMA helpers.
>>>
>>> This allows us to just set the dedicated DMA device for the DRM device,
>>> and drop all the custom GEM callbacks. Also drop the .dma_dev field
>>> from the driver private data as it is no longer needed.
>> My impression is that this patch should be split up: first replace
>> dma_dev with the dma device; then replace the now-duplicate code with
>> shared helpers.
> I'm not sure which of the following you are thinking about:
>
> a) one patch with just the drm_dev_set_dma_dev() call,
>     one patch dropping the custom GEM callbacks
>
> In this case the first patch has no actual effect, since nothing is using
> drm_dev->dma_dev. I think this is not a good split.
>
> b) one patch adding drm_dev_set_dma_dev() and replacing priv->dma_dev
>     with drm_dev_dma_dev(),
>     one patch dropping the custom GEM callbacks
>
> In this case the first patch would modify a bunch of places that then
> get dropped in the second. The churn maybe isn't worth it.
>
> c) one patch adding drm_dev_set_dma_dev() and replacing
>     drm_gem_prime_import_dev() with drm_gem_prime_import(),
>     one patch dropping the custom GEM callbacks
>
> Functionally the second patch would depend on the first.

Not my call to make, but the driver maintainers.

What I meant was option b). Switching dma_dev for drm_dev_dma_dev() and 
replacing the mediatek code with common helpers can both introduce 
regressions.  Having two patches might make later bisecting easier.

>
>
> I still think having one single patch is better.
>
>>> There are slight differences in the mmap helper: the VM_DONTDUMP and
>>> VM_IO flags are no longer set. Both were lifted from drm_gem_mmap_obj().
>>> VM_IO probably doesn't make sense since the buffer is allocated using
>>> dma_alloc_attrs().
>> We can add VM_DONTDUMP to gem-dma's mmap helper. [1] It's most likely
>> the right thing to not dump graphics buffers and standard practice in
>> most other GEM memory managers.
> Makes sense. I can add a patch to the series to do this.

Rather send it out separately. This affects many drivers and we 
shouldn't hide such a change somewhere in an unrelated series.

Best regards
Thomas

>
>> [1]
>> https://elixir.bootlin.com/linux/v6.19.6/source/drivers/gpu/drm/drm_gem_dma_helper.c#L537
>>
>>> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
>> The changes look correct, so I'm here's the
>>
>> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
>>
>> But this patch requires a review from the driver maintainer as well.
> Sure, and I need to respin the series because I forgot to remove the
> deleted file from the Makefile.
>
>
> Thanks
> ChenYu

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)