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

[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 ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch