Re: [PATCH v3 1/3] drm: Introduce DRM_MODE_DUMB_KERNEL_MAP flag

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 27.03.26 um 10:38 schrieb Chen-Yu Tsai:
> On Fri, Mar 27, 2026 at 3:21 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Hi
>>
>> Am 26.03.26 um 11:01 schrieb Chen-Yu Tsai:
>>> From: Rob Herring <robh@kernel.org>
>>>
>>> Introduce a new flag, DRM_MODE_DUMB_KERNEL_MAP, for struct
>>> drm_mode_create_dumb. This flag is for internal kernel use to indicate
>>> if dumb buffer allocation needs a kernel mapping. This is needed only for
>>> GEM DMA where creating a kernel mapping or not has to be decided at
>>> allocation time because creating a mapping on demand (with vmap()) is not
>>> guaranteed to work.
>> I still don't understand what you're trying to achieve. As I pointed
>> out, ever driver's memory manager potentially requires a vmap. Passing
>> around flags will not solve that problem. If vmap is not possible, the
>> driver should not provide the vmap callbacks in the first place.
> I'm trying to convert the Rockchip and Exynos drivers to the GEM DMA

Ok.

> helpers. Both have their reasons for not using the helpers, one of them
> being that the DRM device is not the DMA device. This is already fixed
> by my other series. Both drivers also set DMA_ATTR_NO_KERNEL_MAPPING
> for buffer allocations other than the one used by fb_helper.
>
> Exynos's GEM implementation also sets extra DMA attributes at the
> allocation phase for their custom GEM create ioctl. But otherwise
> exynos is just reimplementing all the GEM DMA helpers in almost
> identical fashion. And to your point, exynos does not provide the
> vmap callback.
>
> Rockchip's GEM implementation splits the code path between DMA and
> IOMMU usage as it needs to attach and detach the DMA device from the
> IOMMU domain on the fly. This predates the DMA IOMMU framework. This
> GEM implementation provides the vmap callback, which will fail for
> direct DMA allocations without the kernel mapping.
>
> There's also something fishy about Rockchip's DRM driver. It's using
> fbdev-dma, even though it's not based on the GEM DMA helpers or even
> drm_gem_dma_object.

Oh!


>
> As you are against changing the existing *_dumb_create() interface,
> I will look into reusing drm_gem_dma_object with driver-specific
> functions. I believe I will need to keep patch 2 adding dma_attrs
> though. Would that be acceptable?

I think so. Sharing code is good, but I don't like these flags that 
change a function's overall behavior. I think that's the case here. 
Flags are OK when they extent or modify a little detail, but extensively 
used the affected code becomes unmaintainable. It's better to have a 
dedicated function for such special cases. What you could do is to 
provide a separate helper that allocates an object with the specific 
vmap behavior and use it for these drivers. The allocator function 
should also set dedicated gem-object funcs w/o vmap/vunmap where 
possible.  Drivers can then pick the helpers.

If you 're looking for an example, take a look at commit 660cd44659a0 
("drm/shmem-helper: Import dmabuf without mapping its sg_table")  and 
it's patch series.  With gem-shmem, many drivers don't work correctly 
with the sg-table imported.  The commit provides them with a separate 
import helper that takes care of this.

Best regards
Thomas


>
> Once again, thank you for your patience.
>
>
> ChenYu
>
>
>> Best regards
>> Thomas
>>
>>> Several drivers are using reimplementing the GEM DMA helpers because
>>> they distinguish between kernel and userspace allocations to create a
>>> kernel mapping or not. Adding a flag allows migrating these drivers
>>> to the helpers while preserving their existing behavior. These include
>>> exynos, rockchip, and previously mediatek.
>>>
>>> Update the callers of drm_mode_dumb_create() to set
>>> drm_mode_dumb_create.flags to appropriate defaults. Currently, flags can
>>> be set to anything by userspace, but is unused within the kernel. Let's
>>> force flags to zero (no kernel mapping) for userspace callers by default.
>>> For in kernel clients, set DRM_MODE_DUMB_KERNEL_MAP by default. Drivers
>>> can override this as needed.
>>>
>>> Signed-off-by: Rob Herring <robh@kernel.org>
>>> [wenst@chromium.org: Emit warning (once) if args->flags is not zero]
>>> [wenst@chromium.org: Moved flag def. to include/drm/drm_dumb_buffers.h]
>>> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
>>> ---
>>> Changes since v2:
>>> - Switched to drm_warn_once()
>>> - Moved flag definition from include/uapi/ to include/drm/drm_dumb_buffers.h
>>> - Reworded commit message
>>>
>>> Changes since v1:
>>> - Emit warning if args->flags is not zero
>>> ---
>>>    drivers/gpu/drm/drm_client.c       | 2 ++
>>>    drivers/gpu/drm/drm_dumb_buffers.c | 4 ++++
>>>    include/drm/drm_dumb_buffers.h     | 3 +++
>>>    3 files changed, 9 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
>>> index 46c465bce98c..3d3e61823cc1 100644
>>> --- a/drivers/gpu/drm/drm_client.c
>>> +++ b/drivers/gpu/drm/drm_client.c
>>> @@ -14,6 +14,7 @@
>>>    #include <drm/drm_client_event.h>
>>>    #include <drm/drm_device.h>
>>>    #include <drm/drm_drv.h>
>>> +#include <drm/drm_dumb_buffers.h>
>>>    #include <drm/drm_file.h>
>>>    #include <drm/drm_fourcc.h>
>>>    #include <drm/drm_framebuffer.h>
>>> @@ -404,6 +405,7 @@ drm_client_buffer_create_dumb(struct drm_client_dev *client, u32 width, u32 heig
>>>        dumb_args.width = width;
>>>        dumb_args.height = height;
>>>        dumb_args.bpp = drm_format_info_bpp(info, 0);
>>> +     dumb_args.flags = DRM_MODE_DUMB_KERNEL_MAP;
>>>        ret = drm_mode_create_dumb(dev, &dumb_args, client->file);
>>>        if (ret)
>>>                return ERR_PTR(ret);
>>> diff --git a/drivers/gpu/drm/drm_dumb_buffers.c b/drivers/gpu/drm/drm_dumb_buffers.c
>>> index e2b62e5fb891..60f4c2d08641 100644
>>> --- a/drivers/gpu/drm/drm_dumb_buffers.c
>>> +++ b/drivers/gpu/drm/drm_dumb_buffers.c
>>> @@ -233,6 +233,10 @@ int drm_mode_create_dumb_ioctl(struct drm_device *dev,
>>>        struct drm_mode_create_dumb *args = data;
>>>        int err;
>>>
>>> +     if (args->flags)
>>> +             drm_warn_once(dev, "drm_mode_create_dumb.flags is not zero.\n");
>>> +     args->flags = 0;
>>> +
>>>        err = drm_mode_create_dumb(dev, args, file_priv);
>>>        if (err) {
>>>                args->handle = 0;
>>> diff --git a/include/drm/drm_dumb_buffers.h b/include/drm/drm_dumb_buffers.h
>>> index 1f3a8236fb3d..4657e44533f4 100644
>>> --- a/include/drm/drm_dumb_buffers.h
>>> +++ b/include/drm/drm_dumb_buffers.h
>>> @@ -6,6 +6,9 @@
>>>    struct drm_device;
>>>    struct drm_mode_create_dumb;
>>>
>>> +/* drm_mode_create_dumb flags for internal use */
>>> +#define DRM_MODE_DUMB_KERNEL_MAP     (1<<0)
>>> +
>>>    int drm_mode_size_dumb(struct drm_device *dev,
>>>                       struct drm_mode_create_dumb *args,
>>>                       unsigned long hw_pitch_align,
>> --
>> --
>> 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)
>>
>>

-- 
--
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)