[RFC 1/7] drm/gem: Add drm_gem_dumb_map_offset()

[Noralf Trønnes]

Den 24.07.2017 10.28, skrev Daniel Vetter:
> On Fri, Jul 21, 2017 at 08:41:50PM +0200, Noralf Trønnes wrote:
>> Den 20.07.2017 10.00, skrev Daniel Vetter:
>>> On Thu, Jul 20, 2017 at 12:13:07AM +0200, Noralf Trønnes wrote:
>>>> Den 19.07.2017 23.01, skrev Eric Anholt:
>>>>> Noralf Trønnes <noralf at tronnes.org> writes:
>>>>>
>>>>>> Add a common drm_driver.dumb_map_offset function for GEM backed drivers.
>>>>>>
>>>>>> Signed-off-by: Noralf Trønnes <noralf at tronnes.org>
>>>>> Instead of just introducing the new code, could we replace CMA's code
>>>>> with calls to this at the same time?
>>>> I have gone through all the drm_driver->dumb_map_offset
>>>> implementations and found 23 drivers that could use it:
>>>> - 18 drivers use drm_gem_cma_dumb_map_offset()
>>>> - exynos_drm_gem_dumb_map_offset()
>>>> - mtk_drm_gem_dumb_map_offset()
>>>> - psb_gem_dumb_map_gtt()
>>>> - rockchip_gem_dumb_map_offset()
>>>> - tegra_bo_dumb_map_offset()
>>>>
>>>> vgem_gem_dumb_map() can't use it because of this check:
>>>>       if (!obj->filp) {
>>>>           ret = -EINVAL;
>>>>           goto unref;
>>>>       }
>>>>
>>>> armada_gem_dumb_map_offset() can't use it because of this check:
>>>>       /* Don't allow imported objects to be mapped */
>>>>       if (obj->obj.import_attach) {
>>>>           ret = -EINVAL;
>>>>           goto err_unref;
>>>>       }
>>>>
>>>> I see that drivers must implement all 3 .dumb_* callbacks:
>>>>
>>>>    * To support dumb objects drivers must implement the
>>>> &drm_driver.dumb_create,
>>>>    * &drm_driver.dumb_destroy and &drm_driver.dumb_map_offset operations. See
>>>>    * there for further details.
>>>>
>>>> I'm a fan of defaults, is there any reason we can't do this:
>>> So in general we try not to set defaults for the main driver entry hooks,
>>> to avoid the helper functions leaking into core and becoming mandatory.
>>> So e.g. ->atomic_commit should never have a default of
>>> drm_atomic_helper_commit.
>>>
>>> Same thought applied here for the dumb buffers - the idea is that drivers
>>> using any kind of buffer manager scheme could have dumb buffers, including
>>> maybe not having a buffer manager at all (and you get some cookie to
>>> direct vram allocations or whatever). But everyone ended up with gem, just
>>> with different kinds of backing storage implementations (cma, shmem or
>>> ttm).
>>>
>>> I think it makes sense to make these the defaults and rip out the default
>>> assignment from all drivers.
>>>>    int drm_mode_mmap_dumb_ioctl(struct drm_device *dev,
>>>>                     void *data, struct drm_file *file_priv)
>>>>    {
>>>>        struct drm_mode_map_dumb *args = data;
>>>>
>>>>        /* call driver ioctl to get mmap offset */
>>>> -     if (!dev->driver->dumb_map_offset)
>>>> +    if (!dev->driver->dumb_create)
>>>>           return -ENOSYS;
>>>>
>>>> -     return dev->driver->dumb_map_offset(file_priv, dev, args->handle,
>>>> &args->offset);
>>>> +     if (dev->driver->dumb_map_offset)
>>>> +        return dev->driver->dumb_map_offset(file_priv, dev, args->handle,
>>>> &args->offset);
>>>> +    else
>>>> +        return drm_gem_dumb_map_offset(file_priv, dev, args->handle,
>>>> &args->offset);
>>>>    }
>>>>
>>>>    int drm_mode_destroy_dumb_ioctl(struct drm_device *dev,
>>>>                    void *data, struct drm_file *file_priv)
>>>>    {
>>>>        struct drm_mode_destroy_dumb *args = data;
>>>>
>>>> -     if (!dev->driver->dumb_destroy)
>>>> +    if (!dev->driver->dumb_create)
>>>>            return -ENOSYS;
>>>>
>>>> -    return dev->driver->dumb_destroy(file_priv, dev, args->handle);
>>>> +     if (dev->driver->dumb_destroy)
>>>> +        return dev->driver->dumb_destroy(file_priv, dev, args->handle);
>>>> +    else
>>>> +        return drm_gem_dumb_destroy(file_priv, dev, args->handle);
>>>>    }
>>>>
>>>> There are 36 drivers that use drm_gem_dumb_destroy() directly.
>>>> vgem violates the docs and doesn't set .dumb_destroy.
>>> Interesting, suprising it doesn't leak like mad.
>>>>> I suspect that if we had CMA subclass from drm_fb_gem (or we move the
>>>>> gem objects to the base class), we could remove a lot of its code that
>>>>> you're copying in patch 2, as well.
>>>> Yes, that was the intention.
>>> I guess we'd need to see more of the grand plan ...
>> Currently it looks like 4 patchsets:
>>
>> 1. Add drm_gem_dumb_map_offset() and implement defaults as discussed above.
>>
>> 2. Add drm_gem_object pointers to drm_framebuffer and create matching
>>     helpers for:
>>       drm_framebuffer_funcs->create_handle
>>       drm_framebuffer_funcs->destroy
>>       drm_mode_config_funcs->fb_create
>>       Should I put the functions in drm_framebuffer_helper.[h,c] ?
> I'd call them drm_gem_framebuffer_helper.[hc], just to highlight the
> gem<->kms connection a bit more.
>
>>     Use these helpers in the cma library
>>
>> 3. Add drm_fb_helper_simple_init() and drm_fb_helper_simple_fini()
>>     Use them in drivers where applicable.
>>
>> 4. Add shmem library
>>     Convert tinydrm to shmem.
> Sounds like a good plan. I'll try to scrape away some review bandwidth to
> look at your patches.

Thanks, this fanned out a bit more than first expected.

Noralf.