| Message ID | 1500837417-40580-2-git-send-email-noralf@tronnes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, 2017-07-23 at 21:16 +0200, Noralf Trønnes wrote: > Add a common drm_driver.dumb_map_offset function for GEM backed drivers. > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> regards Philipp
Hi Noralf, Thank you for the patch. On Sunday 23 Jul 2017 21:16:17 Noralf Trønnes wrote: > Add a common drm_driver.dumb_map_offset function for GEM backed drivers. > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > 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 5df028a..a8d396b 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);
Hi Noralf, On 23 July 2017 at 20:16, Noralf Trønnes <noralf@tronnes.org> wrote: > 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 5df028a..a8d396b 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); With later patches one goes to reuse this helper instead of drm_gem_cma_dumb_map_offset(). At the same time, the latter does not have the drm_gem_create_mmap_offset() call we see here. Meanwhile some drivers have their own offset function which is virtually identical to the original drm_gem_cma_dumb_map_offset. Yet those are left unchanged. For example in cirrus [1]: There the only code difference seems to be an "upcast" followed immediately by a "downcast". Which should effectively be a noop. That said, I could be loosing my marbles. HTH Emil [1] https://cgit.freedesktop.org/drm/drm-misc/tree/drivers/gpu/drm/cirrus/cirrus_main.c#n292
On Sun, Jul 23, 2017 at 09:16:17PM +0200, Noralf Trønnes wrote: > Add a common drm_driver.dumb_map_offset function for GEM backed drivers. > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org> Reviewed-by: Sean Paul <seanpaul@chromium.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 5df028a..a8d396b 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); > -- > 2.7.4 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Den 26.07.2017 14.05, skrev Emil Velikov: > Hi Noralf, > > > On 23 July 2017 at 20:16, Noralf Trønnes <noralf@tronnes.org> wrote: >> 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 5df028a..a8d396b 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); > With later patches one goes to reuse this helper instead of > drm_gem_cma_dumb_map_offset(). > At the same time, the latter does not have the > drm_gem_create_mmap_offset() call we see here. You're rigth, the cma library and a couple of other drivers if I recall correctly, call drm_gem_create_mmap_offset() during object creation. Daniel was of the opinion that this should happen when mmap is called. drm_gem_create_mmap_offset() is idempotent as the docs call it. Calling it a second time is ok, since it does nothing (needs to take a lock though). I haven't removed drm_gem_create_mmap_offset() from the drivers that do it during object creation, trying to keep this from doing to much. The cma library will be changed though when I add the generic fb/gem helper. > Meanwhile some drivers have their own offset function which is > virtually identical to the original drm_gem_cma_dumb_map_offset. > Yet those are left unchanged. > > For example in cirrus [1]: > There the only code difference seems to be an "upcast" followed > immediately by a "downcast". Which should effectively be a noop. The main reason for that is to try and keep this simple so I'm able to add my shmem/gem library in reasonable time :-) I didn't want to do changes that wasn't straight forward. But yes, the cirrus drivers looks quite straight forward and the functions isn't used by other parts of the driver. I looked at a driver (omap?) that had similar functions, and those where called from other parts of the driver, so I expected the same here I guess. You say "some drivers", can you name them? Some drivers takes locks or do other stuff that made me skip them. Noralf. > That said, I could be loosing my marbles. > > HTH > Emil > > [1] https://cgit.freedesktop.org/drm/drm-misc/tree/drivers/gpu/drm/cirrus/cirrus_main.c#n292 >
On 26 July 2017 at 19:41, Noralf Trønnes <noralf@tronnes.org> wrote: > > Den 26.07.2017 14.05, skrev Emil Velikov: >> >> Hi Noralf, >> >> >> On 23 July 2017 at 20:16, Noralf Trønnes <noralf@tronnes.org> wrote: >>> >>> 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 5df028a..a8d396b 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); >> >> With later patches one goes to reuse this helper instead of >> drm_gem_cma_dumb_map_offset(). >> At the same time, the latter does not have the >> drm_gem_create_mmap_offset() call we see here. > > > You're rigth, the cma library and a couple of other drivers if I recall > correctly, call drm_gem_create_mmap_offset() during object creation. > Daniel was of the opinion that this should happen when mmap is called. > drm_gem_create_mmap_offset() is idempotent as the docs call it. Calling > it a second time is ok, since it does nothing (needs to take a lock > though). > > I haven't removed drm_gem_create_mmap_offset() from the drivers that do > it during object creation, trying to keep this from doing to much. > The cma library will be changed though when I add the generic fb/gem > helper. > I was not concerned about calling the function twice, but that it's not called at all ... Which doesn't seems to be the case - virtually all the .dumb_create callbacks effectively end up in __drm_gem_cma_create() which itself calls drm_gem_create_mmap_offset(). I should have looked deeper into the rabbit hole :-) One thing that I forgot to mention: Depending on the tree you're working on, please grep through staging as well - the vbox drm driver got merged somewhat recently. >> Meanwhile some drivers have their own offset function which is >> virtually identical to the original drm_gem_cma_dumb_map_offset. >> Yet those are left unchanged. >> >> For example in cirrus [1]: >> There the only code difference seems to be an "upcast" followed >> immediately by a "downcast". Which should effectively be a noop. > > > The main reason for that is to try and keep this simple so I'm able to > add my shmem/gem library in reasonable time :-) I didn't want to do > changes that wasn't straight forward. But yes, the cirrus drivers looks > quite straight forward and the functions isn't used by other parts of > the driver. I looked at a driver (omap?) that had similar functions, > and those where called from other parts of the driver, so I expected > the same here I guess. > > You say "some drivers", can you name them? > Some drivers takes locks or do other stuff that made me skip them. > Agreed - keeping that cleanup separate might be the wise thing to do. Pardon if I came too impatient. About a list - initial grep showed cirrus, bochs, qxl. Now, after having a more extensive look - cirrus, qxl, ast, nouveau, exynos, hibmc, mgag200, bochs, virtio. On the "maybe" list - msm and omap have extra locking (is it still needed?) while armada has an import_attach restriction (where nobody else seems to bother?). The remaining drivers seem a bit more complicated. HTH Emil
Hi Noralf, On Wednesday 26 Jul 2017 20:41:53 Noralf Trønnes wrote: > Den 26.07.2017 14.05, skrev Emil Velikov: > > On 23 July 2017 at 20:16, Noralf Trønnes <noralf@tronnes.org> wrote: > >> 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 5df028a..a8d396b 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); > > > > With later patches one goes to reuse this helper instead of > > drm_gem_cma_dumb_map_offset(). > > At the same time, the latter does not have the > > drm_gem_create_mmap_offset() call we see here. > > You're rigth, the cma library and a couple of other drivers if I recall > correctly, call drm_gem_create_mmap_offset() during object creation. > Daniel was of the opinion that this should happen when mmap is called. > drm_gem_create_mmap_offset() is idempotent as the docs call it. Calling > it a second time is ok, since it does nothing (needs to take a lock > though). I came to the exact same conclusion. It might be a slightly more efficient if we coudl call drm_gem_create_mmap_offset() at the GEM object creation time, but as that's not possible for all drivers, and as the .dumb_map_offset() operation is not in any hot path anyway, I stopped investigating how we could optimize the implementation. > I haven't removed drm_gem_create_mmap_offset() from the drivers that do > it during object creation, trying to keep this from doing to much. > The cma library will be changed though when I add the generic fb/gem > helper. > > > Meanwhile some drivers have their own offset function which is > > virtually identical to the original drm_gem_cma_dumb_map_offset. > > Yet those are left unchanged. > > > > For example in cirrus [1]: > > There the only code difference seems to be an "upcast" followed > > immediately by a "downcast". Which should effectively be a noop. > > The main reason for that is to try and keep this simple so I'm able to > add my shmem/gem library in reasonable time :-) I didn't want to do > changes that wasn't straight forward. But yes, the cirrus drivers looks > quite straight forward and the functions isn't used by other parts of > the driver. I looked at a driver (omap?) that had similar functions, > and those where called from other parts of the driver, so I expected > the same here I guess. For omapdrm I believe we could drop the custom .dumb_map_offset() operation, as the only use for customs offsets in the driver is for tiled buffers, which are created by an omapdrm-specific IOCTL. However, I can't tell at the moment whether userspace code that create tiled buffers with the OMAP_GEM_NEW ioctl get the mmap offset with OMAP_GEM_INFO as they should, or if DRM_IOCTL_MODE_MAP_DUMB gets sometimes abused for non-dumb buffers. > You say "some drivers", can you name them? > Some drivers takes locks or do other stuff that made me skip them. > > Noralf. > > > That said, I could be loosing my marbles. > > > > HTH > > Emil > > > > [1] > > https://cgit.freedesktop.org/drm/drm-misc/tree/drivers/gpu/drm/cirrus/cir > > rus_main.c#n292
Den 27.07.2017 02.13, skrev Emil Velikov: > On 26 July 2017 at 19:41, Noralf Trønnes <noralf@tronnes.org> wrote: >> Den 26.07.2017 14.05, skrev Emil Velikov: >>> Hi Noralf, >>> >>> >>> On 23 July 2017 at 20:16, Noralf Trønnes <noralf@tronnes.org> wrote: >>>> 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 5df028a..a8d396b 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); >>> With later patches one goes to reuse this helper instead of >>> drm_gem_cma_dumb_map_offset(). >>> At the same time, the latter does not have the >>> drm_gem_create_mmap_offset() call we see here. >> >> You're rigth, the cma library and a couple of other drivers if I recall >> correctly, call drm_gem_create_mmap_offset() during object creation. >> Daniel was of the opinion that this should happen when mmap is called. >> drm_gem_create_mmap_offset() is idempotent as the docs call it. Calling >> it a second time is ok, since it does nothing (needs to take a lock >> though). >> >> I haven't removed drm_gem_create_mmap_offset() from the drivers that do >> it during object creation, trying to keep this from doing to much. >> The cma library will be changed though when I add the generic fb/gem >> helper. >> > I was not concerned about calling the function twice, but that it's > not called at all ... > Which doesn't seems to be the case - virtually all the .dumb_create > callbacks effectively end up in __drm_gem_cma_create() which itself > calls drm_gem_create_mmap_offset(). > > I should have looked deeper into the rabbit hole :-) > > One thing that I forgot to mention: > Depending on the tree you're working on, please grep through staging > as well - the vbox drm driver got merged somewhat recently. > >>> Meanwhile some drivers have their own offset function which is >>> virtually identical to the original drm_gem_cma_dumb_map_offset. >>> Yet those are left unchanged. >>> >>> For example in cirrus [1]: >>> There the only code difference seems to be an "upcast" followed >>> immediately by a "downcast". Which should effectively be a noop. >> >> The main reason for that is to try and keep this simple so I'm able to >> add my shmem/gem library in reasonable time :-) I didn't want to do >> changes that wasn't straight forward. But yes, the cirrus drivers looks >> quite straight forward and the functions isn't used by other parts of >> the driver. I looked at a driver (omap?) that had similar functions, >> and those where called from other parts of the driver, so I expected >> the same here I guess. >> >> You say "some drivers", can you name them? >> Some drivers takes locks or do other stuff that made me skip them. >> > Agreed - keeping that cleanup separate might be the wise thing to do. > Pardon if I came too impatient. > > About a list - initial grep showed cirrus, bochs, qxl. > Now, after having a more extensive look - cirrus, qxl, ast, nouveau, > exynos, hibmc, mgag200, bochs, virtio. All of those except exynos gets their offset from ttm_buffer_object->vma_node and not drm_gem_object.vma_node. Is it the same vma_node? I'll add exynos in the second round, I missed that one. Thanks for the scrutiny, it's appreciated. Noralf. > On the "maybe" list - msm and omap have extra locking (is it still > needed?) while armada has an import_attach restriction (where nobody > else seems to bother?). > > The remaining drivers seem a bit more complicated. > > HTH > Emil >
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 5df028a..a8d396b 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(+)