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

["Noralf Trønnes" <noralf@tronnes.org>]

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@tronnes.org> writes:
>>>
>>>> Add a common drm_driver.dumb_map_offset function for GEM backed drivers.
>>>>
>>>> Signed-off-by: Noralf Trønnes <noralf@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] ?
    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.

Noralf.

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel