Re: [PATCH] drm/mediatek: mtk_gem: Partial refactor and use drm_gem_dma_object

["CK Hu (胡俊光)" <ck.hu@mediatek.com>]

Hi, Angelo:

On Tue, 2025-11-11 at 09:51 +0100, AngeloGioacchino Del Regno wrote:
> External email : Please do not click links or open attachments until you have verified the sender or the content.
> 
> 
> Partially refactor mtk_gem to stop using (and remove) the unneeded
> custom mtk_gem_obj structure and migrate drivers to use the API
> defined drm_gem_dma_object structure instead, and to align all of
> the functions to be similar to the logic from drm_gem_dma_helper.
> 
> Unfortunately, for this driver it wasn't possible to directly use
> the drm_gem_dma_helper callbacks (apart from .print_info), as the
> DMA mapping here is done on specific dma devices instead of the
> main DRM device.
> 
> Also, since the mtk_gem_obj structure is no more, also migrate the
> mtk_plane.c code to grab the DMA address from a drm_gem_dma_object
> and replace the inclusion of the custom mtk_gem.h header (as it is
> now unneeded) with the DRM API provided drm_gem_dma_helper.
> 
> While at it, also set DRM_GEM_DMA_HELPER as an unconditional
> dependency (remove the `if DRM_FBDEV_EMULATION` from the select
> DRM_GEM_DMA_HELPER statement in Kconfig).
> 
> This resolves an issue pointed by UBSAN, as when using drm_fbdev_dma
> the drm_gem_object is supposed to be child of a drm_gem_dma_object
> instead of a custom mtk_gem_obj (or the mtk_gem_obj should have been
> reordered to have the same fields as drm_gem_dma_object, but that
> would have been too fragile and generally a bad idea anyway).
> 
> Fixes: 0992284b4fe4 ("drm/mediatek: Use fbdev-dma")
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---

[snip]

> -
>  int mtk_gem_dumb_create(struct drm_file *file_priv, struct drm_device *dev,
>                         struct drm_mode_create_dumb *args)
>  {
> -       struct mtk_gem_obj *mtk_gem;
> +       struct drm_gem_dma_object *dma_obj;
>         int ret;
> 
>         args->pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
> @@ -134,25 +155,24 @@ int mtk_gem_dumb_create(struct drm_file *file_priv, struct drm_device *dev,
>         args->size = args->pitch;
>         args->size *= args->height;
> 
> -       mtk_gem = mtk_gem_create(dev, args->size, false);
> -       if (IS_ERR(mtk_gem))
> -               return PTR_ERR(mtk_gem);
> +       dma_obj = mtk_gem_create(dev, args->size);
> +       if (IS_ERR(dma_obj))
> +               return PTR_ERR(dma_obj);
> 
>         /*
>          * allocate a id of idr table where the obj is registered
>          * and handle has the id what user can see.
>          */
> -       ret = drm_gem_handle_create(file_priv, &mtk_gem->base, &args->handle);
> +       ret = drm_gem_handle_create(file_priv, &dma_obj->base, &args->handle);
> +       /* drop reference from allocate - handle holds it now. */
> +       drm_gem_object_put(&dma_obj->base);

I've trce drm_gem_handle_create() and find it would call drm_gem_object_put() when error.
Please confirm this.
If so, I would modify this when I applied.

Regards,
CK

>         if (ret)
>                 goto err_handle_create;
> 
> -       /* drop reference from allocate - handle holds it now. */
> -       drm_gem_object_put(&mtk_gem->base);
> -
>         return 0;
> 
>  err_handle_create:
> -       mtk_gem_free_object(&mtk_gem->base);
> +       mtk_gem_free_object(&dma_obj->base);
>         return ret;
>  }
>