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

[Thomas Zimmermann]

Hi

Am 11.03.26 um 05:55 schrieb Chen-Yu Tsai:
> On Tue, Mar 10, 2026 at 4:21 PM Thomas Zimmermann <tzimmermann@xxxxxxx> 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@xxxxxxxxxxxx>
> > The changes look correct, so I'm here's the
> >
> > Reviewed-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
> >
> > 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)