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@xxxxxxxxxxx> writes:
> > > >
> > > > > Add a common drm_driver.dumb_map_offset function for GEM backed drivers.
> > > > >
> > > > > Signed-off-by: Noralf Trønnes <noralf@xxxxxxxxxxx>
> > > > 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
