Den 24.07.2017 10.28, skrev 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@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.
Thanks, this fanned out a bit more than first expected.
Noralf.
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
