| Message ID | 1499867165-60925-2-git-send-email-noralf@tronnes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Den 12.07.2017 15.45, skrev Noralf Trønnes: > Add a common drm_driver.dumb_map_offset function for GEM backed drivers. > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org> > --- > drivers/gpu/drm/drm_gem.c | 35 +++++++++++++++++++++++++++++++++++ > include/drm/drm_gem.h | 2 ++ > 2 files changed, 37 insertions(+) > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > index 8dc1106..44ecbaa 100644 > --- a/drivers/gpu/drm/drm_gem.c > +++ b/drivers/gpu/drm/drm_gem.c > @@ -311,6 +311,41 @@ drm_gem_handle_delete(struct drm_file *filp, u32 handle) > EXPORT_SYMBOL(drm_gem_handle_delete); > > /** > + * drm_gem_dumb_map_offset - return the fake mmap offset for a gem object > + * @file: drm file-private structure containing the gem object > + * @dev: corresponding drm_device > + * @handle: gem object handle > + * @offset: return location for the fake mmap offset > + * > + * This implements the &drm_driver.dumb_map_offset kms driver callback for > + * drivers which use gem to manage their backing storage. > + * > + * Returns: > + * 0 on success or a negative error code on failure. > + */ > +int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev, > + u32 handle, u64 *offset) > +{ > + struct drm_gem_object *obj; > + int ret; > + > + obj = drm_gem_object_lookup(file, handle); > + if (!obj) > + return -ENOENT; > + > + ret = drm_gem_create_mmap_offset(obj); There are 3 drm_driver->dumb_map_offset implementations that don't call drm_gem_create_mmap_offset(): - drm_gem_cma_dumb_map_offset() - exynos_drm_gem_dumb_map_offset() - tegra_bo_dumb_map_offset() They do it during object creation. exynos have this commit: drm/exynos: create a fake mmap offset with gem creation 48cf53f4343ae12ddc1c60dbe116161ecf7a2885 I looked at the discussion but didn't understand the rationale: https://lists.freedesktop.org/archives/dri-devel/2015-August/088541.html I see that it's ok to call drm_gem_create_mmap_offset() multiple times, so it's not really a problem for this function, but it would be nice to know why for my shmem gem library which is based on the cma library. Noralf. > + if (ret) > + goto out; > + > + *offset = drm_vma_node_offset_addr(&obj->vma_node); > +out: > + drm_gem_object_put_unlocked(obj); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(drm_gem_dumb_map_offset); > + > +/** > * drm_gem_dumb_destroy - dumb fb callback helper for gem based drivers > * @file: drm file-private structure to remove the dumb handle from > * @dev: corresponding drm_device > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h > index 4a9d231..9c55c2a 100644 > --- a/include/drm/drm_gem.h > +++ b/include/drm/drm_gem.h > @@ -302,6 +302,8 @@ void drm_gem_put_pages(struct drm_gem_object *obj, struct page **pages, > bool dirty, bool accessed); > > struct drm_gem_object *drm_gem_object_lookup(struct drm_file *filp, u32 handle); > +int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev, > + u32 handle, u64 *offset); > int drm_gem_dumb_destroy(struct drm_file *file, > struct drm_device *dev, > uint32_t handle);
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 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.
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: 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. > 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. Noralf.
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 ... -Daniel
On Tue, Jul 18, 2017 at 11:06:56PM +0200, Noralf Trønnes wrote: > > Den 12.07.2017 15.45, skrev Noralf Trønnes: > > Add a common drm_driver.dumb_map_offset function for GEM backed drivers. > > > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org> > > --- > > drivers/gpu/drm/drm_gem.c | 35 +++++++++++++++++++++++++++++++++++ > > include/drm/drm_gem.h | 2 ++ > > 2 files changed, 37 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > > index 8dc1106..44ecbaa 100644 > > --- a/drivers/gpu/drm/drm_gem.c > > +++ b/drivers/gpu/drm/drm_gem.c > > @@ -311,6 +311,41 @@ drm_gem_handle_delete(struct drm_file *filp, u32 handle) > > EXPORT_SYMBOL(drm_gem_handle_delete); > > /** > > + * drm_gem_dumb_map_offset - return the fake mmap offset for a gem object > > + * @file: drm file-private structure containing the gem object > > + * @dev: corresponding drm_device > > + * @handle: gem object handle > > + * @offset: return location for the fake mmap offset > > + * > > + * This implements the &drm_driver.dumb_map_offset kms driver callback for > > + * drivers which use gem to manage their backing storage. > > + * > > + * Returns: > > + * 0 on success or a negative error code on failure. > > + */ > > +int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev, > > + u32 handle, u64 *offset) > > +{ > > + struct drm_gem_object *obj; > > + int ret; > > + > > + obj = drm_gem_object_lookup(file, handle); > > + if (!obj) > > + return -ENOENT; > > + > > + ret = drm_gem_create_mmap_offset(obj); > > There are 3 drm_driver->dumb_map_offset implementations that > don't call drm_gem_create_mmap_offset(): > - drm_gem_cma_dumb_map_offset() > - exynos_drm_gem_dumb_map_offset() > - tegra_bo_dumb_map_offset() > > They do it during object creation. > > exynos have this commit: > drm/exynos: create a fake mmap offset with gem creation > 48cf53f4343ae12ddc1c60dbe116161ecf7a2885 > > I looked at the discussion but didn't understand the rationale: > https://lists.freedesktop.org/archives/dri-devel/2015-August/088541.html This discussion indeed doesn't make sense - Inki says the patch doesn't make sense and then still goes ahead and merges it. > I see that it's ok to call drm_gem_create_mmap_offset() multiple times, > so it's not really a problem for this function, but it would be nice to > know why for my shmem gem library which is based on the cma library. I don't think there's any reasonable reason for this. Lazy creating of offsets is perfectly fine imo, we might as well bake that in as the standard. And for some drivers it's required (you'll run out of mmap space), but modern userspace should be all using mmap64. Only thing to make sure is that the driver-specific mmap also creates the offset when needed, when we remove it from gem_init functions. -Daniel > > Noralf. > > > > + if (ret) > > + goto out; > > + > > + *offset = drm_vma_node_offset_addr(&obj->vma_node); > > +out: > > + drm_gem_object_put_unlocked(obj); > > + > > + return ret; > > +} > > +EXPORT_SYMBOL_GPL(drm_gem_dumb_map_offset); > > + > > +/** > > * drm_gem_dumb_destroy - dumb fb callback helper for gem based drivers > > * @file: drm file-private structure to remove the dumb handle from > > * @dev: corresponding drm_device > > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h > > index 4a9d231..9c55c2a 100644 > > --- a/include/drm/drm_gem.h > > +++ b/include/drm/drm_gem.h > > @@ -302,6 +302,8 @@ void drm_gem_put_pages(struct drm_gem_object *obj, struct page **pages, > > bool dirty, bool accessed); > > struct drm_gem_object *drm_gem_object_lookup(struct drm_file *filp, u32 handle); > > +int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev, > > + u32 handle, u64 *offset); > > int drm_gem_dumb_destroy(struct drm_file *file, > > struct drm_device *dev, > > uint32_t handle); > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
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: 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] ? 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. Noralf.
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: > > 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
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@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: >> >> 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.
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 8dc1106..44ecbaa 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -311,6 +311,41 @@ drm_gem_handle_delete(struct drm_file *filp, u32 handle) EXPORT_SYMBOL(drm_gem_handle_delete); /** + * drm_gem_dumb_map_offset - return the fake mmap offset for a gem object + * @file: drm file-private structure containing the gem object + * @dev: corresponding drm_device + * @handle: gem object handle + * @offset: return location for the fake mmap offset + * + * This implements the &drm_driver.dumb_map_offset kms driver callback for + * drivers which use gem to manage their backing storage. + * + * Returns: + * 0 on success or a negative error code on failure. + */ +int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev, + u32 handle, u64 *offset) +{ + struct drm_gem_object *obj; + int ret; + + obj = drm_gem_object_lookup(file, handle); + if (!obj) + return -ENOENT; + + ret = drm_gem_create_mmap_offset(obj); + if (ret) + goto out; + + *offset = drm_vma_node_offset_addr(&obj->vma_node); +out: + drm_gem_object_put_unlocked(obj); + + return ret; +} +EXPORT_SYMBOL_GPL(drm_gem_dumb_map_offset); + +/** * drm_gem_dumb_destroy - dumb fb callback helper for gem based drivers * @file: drm file-private structure to remove the dumb handle from * @dev: corresponding drm_device diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h index 4a9d231..9c55c2a 100644 --- a/include/drm/drm_gem.h +++ b/include/drm/drm_gem.h @@ -302,6 +302,8 @@ void drm_gem_put_pages(struct drm_gem_object *obj, struct page **pages, bool dirty, bool accessed); struct drm_gem_object *drm_gem_object_lookup(struct drm_file *filp, u32 handle); +int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev, + u32 handle, u64 *offset); int drm_gem_dumb_destroy(struct drm_file *file, struct drm_device *dev, uint32_t handle);
Add a common drm_driver.dumb_map_offset function for GEM backed drivers. Signed-off-by: Noralf Trønnes <noralf@tronnes.org> --- drivers/gpu/drm/drm_gem.c | 35 +++++++++++++++++++++++++++++++++++ include/drm/drm_gem.h | 2 ++ 2 files changed, 37 insertions(+)