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

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