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@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:
Add drm_gem_dumb_map_offset() and implement defaults as discussed above.
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
Add drm_fb_helper_simple_init() and drm_fb_helper_simple_fini() Use them in drivers where applicable.
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