* [PATCH v4 0/2] drm/panfrost: drm_gem_map_offset() helper @ 2019-06-27 15:53 Steven Price 2019-06-27 15:53 ` [PATCH v4 1/2] drm/gem: Rename drm_gem_dumb_map_offset() to drm_gem_map_offset() Steven Price 2019-06-27 15:53 ` [PATCH v4 2/2] drm/panfrost: Use drm_gem_map_offset() Steven Price 0 siblings, 2 replies; 6+ messages in thread From: Steven Price @ 2019-06-27 15:53 UTC (permalink / raw) To: Daniel Vetter, Rob Herring, Tomeu Vizoso Cc: Steven Price, Alyssa Rosenzweig, Chris Wilson, David Airlie, Inki Dae, Joonyoung Shim, Krzysztof Kozlowski, Kukjin Kim, Kyungmin Park, Maarten Lankhorst, Maxime Ripard, Sean Paul, Seung-Woo Kim, dri-devel, linux-kernel Panfrost has a re-implementation of drm_gem_dumb_map_offset() with an extra bug regarding the handling of imported buffers. However we don't really want Panfrost calling _dumb functions because it's not a KMS driver. This series renames drm_gem_dumb_map_offset() to drop the '_dumb' and updates Panfrost to use it rather than it's own implementation. Previous versions: v3: https://lore.kernel.org/lkml/20190520092306.27633-1-steven.price@arm.com Changes since v3: * Add a comment to drm_gem_map_offset() explaining that it can be used with shmem clients as well as dumb clients. v2: https://lore.kernel.org/lkml/20190516141447.46839-1-steven.price@arm.com/ Changes since v2: * Drop the shmem helper v1: https://lore.kernel.org/lkml/20190513143244.16478-1-steven.price@arm.com/ Changes since v1: * Rename drm_gem_dumb_map_offset to drop _dumb * Add a shmem helper Steven Price (2): drm/gem: Rename drm_gem_dumb_map_offset() to drm_gem_map_offset() drm/panfrost: Use drm_gem_map_offset() drivers/gpu/drm/drm_dumb_buffers.c | 4 ++-- drivers/gpu/drm/drm_gem.c | 9 ++++++--- drivers/gpu/drm/exynos/exynos_drm_gem.c | 3 +-- drivers/gpu/drm/panfrost/panfrost_drv.c | 16 ++-------------- include/drm/drm_gem.h | 4 ++-- 5 files changed, 13 insertions(+), 23 deletions(-) -- 2.20.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v4 1/2] drm/gem: Rename drm_gem_dumb_map_offset() to drm_gem_map_offset() 2019-06-27 15:53 [PATCH v4 0/2] drm/panfrost: drm_gem_map_offset() helper Steven Price @ 2019-06-27 15:53 ` Steven Price 2019-06-27 17:57 ` Rob Herring 2019-06-27 15:53 ` [PATCH v4 2/2] drm/panfrost: Use drm_gem_map_offset() Steven Price 1 sibling, 1 reply; 6+ messages in thread From: Steven Price @ 2019-06-27 15:53 UTC (permalink / raw) To: Daniel Vetter, Rob Herring, Tomeu Vizoso Cc: Steven Price, Alyssa Rosenzweig, Chris Wilson, David Airlie, Inki Dae, Joonyoung Shim, Krzysztof Kozlowski, Kukjin Kim, Kyungmin Park, Maarten Lankhorst, Maxime Ripard, Sean Paul, Seung-Woo Kim, dri-devel, linux-kernel drm_gem_dumb_map_offset() is a useful helper for non-dumb clients, so rename it to remove the _dumb and add a comment that it can be used by shmem clients. Signed-off-by: Steven Price <steven.price@arm.com> --- drivers/gpu/drm/drm_dumb_buffers.c | 4 ++-- drivers/gpu/drm/drm_gem.c | 9 ++++++--- drivers/gpu/drm/exynos/exynos_drm_gem.c | 3 +-- include/drm/drm_gem.h | 4 ++-- 4 files changed, 11 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/drm_dumb_buffers.c b/drivers/gpu/drm/drm_dumb_buffers.c index d18a740fe0f1..b55cfc9e8772 100644 --- a/drivers/gpu/drm/drm_dumb_buffers.c +++ b/drivers/gpu/drm/drm_dumb_buffers.c @@ -48,7 +48,7 @@ * To support dumb objects drivers must implement the &drm_driver.dumb_create * operation. &drm_driver.dumb_destroy defaults to drm_gem_dumb_destroy() if * not set and &drm_driver.dumb_map_offset defaults to - * drm_gem_dumb_map_offset(). See the callbacks for further details. + * drm_gem_map_offset(). See the callbacks for further details. * * Note that dumb objects may not be used for gpu acceleration, as has been * attempted on some ARM embedded platforms. Such drivers really must have @@ -127,7 +127,7 @@ int drm_mode_mmap_dumb_ioctl(struct drm_device *dev, args->handle, &args->offset); else - return drm_gem_dumb_map_offset(file_priv, dev, args->handle, + return drm_gem_map_offset(file_priv, dev, args->handle, &args->offset); } diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index a8c4468f03d9..62842b7701bb 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -298,7 +298,7 @@ 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 + * drm_gem_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 @@ -307,10 +307,13 @@ EXPORT_SYMBOL(drm_gem_handle_delete); * This implements the &drm_driver.dumb_map_offset kms driver callback for * drivers which use gem to manage their backing storage. * + * It can also be used by drivers using the shmem backend as they have the + * same restriction that imported objects cannot be mapped. + * * 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, +int drm_gem_map_offset(struct drm_file *file, struct drm_device *dev, u32 handle, u64 *offset) { struct drm_gem_object *obj; @@ -336,7 +339,7 @@ int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev, return ret; } -EXPORT_SYMBOL_GPL(drm_gem_dumb_map_offset); +EXPORT_SYMBOL_GPL(drm_gem_map_offset); /** * drm_gem_dumb_destroy - dumb fb callback helper for gem based drivers diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c index d8f1fe9b68d8..3b8cffc7a8e0 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c @@ -272,8 +272,7 @@ int exynos_drm_gem_map_ioctl(struct drm_device *dev, void *data, { struct drm_exynos_gem_map *args = data; - return drm_gem_dumb_map_offset(file_priv, dev, args->handle, - &args->offset); + return drm_gem_map_offset(file_priv, dev, args->handle, &args->offset); } struct exynos_drm_gem *exynos_drm_gem_get(struct drm_file *filp, diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h index a9121fe66ea2..65b884930583 100644 --- a/include/drm/drm_gem.h +++ b/include/drm/drm_gem.h @@ -395,8 +395,8 @@ int drm_gem_fence_array_add(struct xarray *fence_array, int drm_gem_fence_array_add_implicit(struct xarray *fence_array, struct drm_gem_object *obj, bool write); -int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev, - u32 handle, u64 *offset); +int drm_gem_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.20.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v4 1/2] drm/gem: Rename drm_gem_dumb_map_offset() to drm_gem_map_offset() 2019-06-27 15:53 ` [PATCH v4 1/2] drm/gem: Rename drm_gem_dumb_map_offset() to drm_gem_map_offset() Steven Price @ 2019-06-27 17:57 ` Rob Herring 2019-06-27 21:35 ` Daniel Vetter 0 siblings, 1 reply; 6+ messages in thread From: Rob Herring @ 2019-06-27 17:57 UTC (permalink / raw) To: Steven Price Cc: Tomeu Vizoso, David Airlie, Seung-Woo Kim, linux-kernel@vger.kernel.org, Krzysztof Kozlowski, Maxime Ripard, Kyungmin Park, Kukjin Kim, dri-devel, Sean Paul, Alyssa Rosenzweig On Thu, Jun 27, 2019 at 9:53 AM Steven Price <steven.price@arm.com> wrote: > > drm_gem_dumb_map_offset() is a useful helper for non-dumb clients, so > rename it to remove the _dumb and add a comment that it can be used by > shmem clients. > > Signed-off-by: Steven Price <steven.price@arm.com> > --- > drivers/gpu/drm/drm_dumb_buffers.c | 4 ++-- > drivers/gpu/drm/drm_gem.c | 9 ++++++--- > drivers/gpu/drm/exynos/exynos_drm_gem.c | 3 +-- > include/drm/drm_gem.h | 4 ++-- > 4 files changed, 11 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/drm_dumb_buffers.c b/drivers/gpu/drm/drm_dumb_buffers.c > index d18a740fe0f1..b55cfc9e8772 100644 > --- a/drivers/gpu/drm/drm_dumb_buffers.c > +++ b/drivers/gpu/drm/drm_dumb_buffers.c > @@ -48,7 +48,7 @@ > * To support dumb objects drivers must implement the &drm_driver.dumb_create > * operation. &drm_driver.dumb_destroy defaults to drm_gem_dumb_destroy() if > * not set and &drm_driver.dumb_map_offset defaults to > - * drm_gem_dumb_map_offset(). See the callbacks for further details. > + * drm_gem_map_offset(). See the callbacks for further details. > * > * Note that dumb objects may not be used for gpu acceleration, as has been > * attempted on some ARM embedded platforms. Such drivers really must have > @@ -127,7 +127,7 @@ int drm_mode_mmap_dumb_ioctl(struct drm_device *dev, > args->handle, > &args->offset); > else > - return drm_gem_dumb_map_offset(file_priv, dev, args->handle, > + return drm_gem_map_offset(file_priv, dev, args->handle, > &args->offset); > } > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > index a8c4468f03d9..62842b7701bb 100644 > --- a/drivers/gpu/drm/drm_gem.c > +++ b/drivers/gpu/drm/drm_gem.c > @@ -298,7 +298,7 @@ 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 > + * drm_gem_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 > @@ -307,10 +307,13 @@ EXPORT_SYMBOL(drm_gem_handle_delete); > * This implements the &drm_driver.dumb_map_offset kms driver callback for > * drivers which use gem to manage their backing storage. > * > + * It can also be used by drivers using the shmem backend as they have the > + * same restriction that imported objects cannot be mapped. Maybe better not to say 'shmem' explicitly or just mention it as an example so when we have a 2nd case we don't have to update the comment. ...drivers with GEM BO implementations which have the same... I can fix up and apply. Some other acks would be nice first. Rob _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4 1/2] drm/gem: Rename drm_gem_dumb_map_offset() to drm_gem_map_offset() 2019-06-27 17:57 ` Rob Herring @ 2019-06-27 21:35 ` Daniel Vetter 0 siblings, 0 replies; 6+ messages in thread From: Daniel Vetter @ 2019-06-27 21:35 UTC (permalink / raw) To: Rob Herring Cc: Steven Price, Daniel Vetter, Tomeu Vizoso, Alyssa Rosenzweig, Chris Wilson, David Airlie, Inki Dae, Joonyoung Shim, Krzysztof Kozlowski, Kukjin Kim, Kyungmin Park, Maarten Lankhorst, Maxime Ripard, Sean Paul, Seung-Woo Kim, dri-devel, linux-kernel@vger.kernel.org On Thu, Jun 27, 2019 at 11:57:34AM -0600, Rob Herring wrote: > On Thu, Jun 27, 2019 at 9:53 AM Steven Price <steven.price@arm.com> wrote: > > > > drm_gem_dumb_map_offset() is a useful helper for non-dumb clients, so > > rename it to remove the _dumb and add a comment that it can be used by > > shmem clients. > > > > Signed-off-by: Steven Price <steven.price@arm.com> > > --- > > drivers/gpu/drm/drm_dumb_buffers.c | 4 ++-- > > drivers/gpu/drm/drm_gem.c | 9 ++++++--- > > drivers/gpu/drm/exynos/exynos_drm_gem.c | 3 +-- > > include/drm/drm_gem.h | 4 ++-- > > 4 files changed, 11 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_dumb_buffers.c b/drivers/gpu/drm/drm_dumb_buffers.c > > index d18a740fe0f1..b55cfc9e8772 100644 > > --- a/drivers/gpu/drm/drm_dumb_buffers.c > > +++ b/drivers/gpu/drm/drm_dumb_buffers.c > > @@ -48,7 +48,7 @@ > > * To support dumb objects drivers must implement the &drm_driver.dumb_create > > * operation. &drm_driver.dumb_destroy defaults to drm_gem_dumb_destroy() if > > * not set and &drm_driver.dumb_map_offset defaults to > > - * drm_gem_dumb_map_offset(). See the callbacks for further details. > > + * drm_gem_map_offset(). See the callbacks for further details. > > * > > * Note that dumb objects may not be used for gpu acceleration, as has been > > * attempted on some ARM embedded platforms. Such drivers really must have > > @@ -127,7 +127,7 @@ int drm_mode_mmap_dumb_ioctl(struct drm_device *dev, > > args->handle, > > &args->offset); > > else > > - return drm_gem_dumb_map_offset(file_priv, dev, args->handle, > > + return drm_gem_map_offset(file_priv, dev, args->handle, > > &args->offset); > > } > > > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > > index a8c4468f03d9..62842b7701bb 100644 > > --- a/drivers/gpu/drm/drm_gem.c > > +++ b/drivers/gpu/drm/drm_gem.c > > @@ -298,7 +298,7 @@ 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 > > + * drm_gem_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 > > @@ -307,10 +307,13 @@ EXPORT_SYMBOL(drm_gem_handle_delete); > > * This implements the &drm_driver.dumb_map_offset kms driver callback for > > * drivers which use gem to manage their backing storage. > > * > > + * It can also be used by drivers using the shmem backend as they have the > > + * same restriction that imported objects cannot be mapped. > > Maybe better not to say 'shmem' explicitly or just mention it as an > example so when we have a 2nd case we don't have to update the > comment. > > ...drivers with GEM BO implementations which have the same... > > I can fix up and apply. Some other acks would be nice first. Yeah kerneldoc is a bit suboptimal, with that polished, a-b: me. Yours should be good enough though. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v4 2/2] drm/panfrost: Use drm_gem_map_offset() 2019-06-27 15:53 [PATCH v4 0/2] drm/panfrost: drm_gem_map_offset() helper Steven Price 2019-06-27 15:53 ` [PATCH v4 1/2] drm/gem: Rename drm_gem_dumb_map_offset() to drm_gem_map_offset() Steven Price @ 2019-06-27 15:53 ` Steven Price 2019-07-03 2:32 ` Rob Herring 1 sibling, 1 reply; 6+ messages in thread From: Steven Price @ 2019-06-27 15:53 UTC (permalink / raw) To: Daniel Vetter, Rob Herring, Tomeu Vizoso Cc: Steven Price, Alyssa Rosenzweig, Chris Wilson, David Airlie, Inki Dae, Joonyoung Shim, Krzysztof Kozlowski, Kukjin Kim, Kyungmin Park, Maarten Lankhorst, Maxime Ripard, Sean Paul, Seung-Woo Kim, dri-devel, linux-kernel panfrost_ioctl_mmap_bo() contains a reimplementation of drm_gem_map_offset() but with a bug - it allows mapping imported objects (without going through the exporter). Fix this by switching to use the newly renamed drm_gem_map_offset() function instead which has the bonus of simplifying the code. Signed-off-by: Steven Price <steven.price@arm.com> Reviewed-by: Alyssa Rosenzweig <alyssa@rosenzweig.io> --- drivers/gpu/drm/panfrost/panfrost_drv.c | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c index e34e86a7378a..1498b9b07a8f 100644 --- a/drivers/gpu/drm/panfrost/panfrost_drv.c +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c @@ -259,26 +259,14 @@ static int panfrost_ioctl_mmap_bo(struct drm_device *dev, void *data, struct drm_file *file_priv) { struct drm_panfrost_mmap_bo *args = data; - struct drm_gem_object *gem_obj; - int ret; if (args->flags != 0) { DRM_INFO("unknown mmap_bo flags: %d\n", args->flags); return -EINVAL; } - gem_obj = drm_gem_object_lookup(file_priv, args->handle); - if (!gem_obj) { - DRM_DEBUG("Failed to look up GEM BO %d\n", args->handle); - return -ENOENT; - } - - ret = drm_gem_create_mmap_offset(gem_obj); - if (ret == 0) - args->offset = drm_vma_node_offset_addr(&gem_obj->vma_node); - drm_gem_object_put_unlocked(gem_obj); - - return ret; + return drm_gem_map_offset(file_priv, dev, args->handle, + &args->offset); } static int panfrost_ioctl_get_bo_offset(struct drm_device *dev, void *data, -- 2.20.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v4 2/2] drm/panfrost: Use drm_gem_map_offset() 2019-06-27 15:53 ` [PATCH v4 2/2] drm/panfrost: Use drm_gem_map_offset() Steven Price @ 2019-07-03 2:32 ` Rob Herring 0 siblings, 0 replies; 6+ messages in thread From: Rob Herring @ 2019-07-03 2:32 UTC (permalink / raw) To: Steven Price, Tomeu Vizoso, Alyssa Rosenzweig Cc: Daniel Vetter, Chris Wilson, David Airlie, Inki Dae, Joonyoung Shim, Krzysztof Kozlowski, Kukjin Kim, Kyungmin Park, Maarten Lankhorst, Maxime Ripard, Sean Paul, Seung-Woo Kim, dri-devel, linux-kernel@vger.kernel.org On Thu, Jun 27, 2019 at 9:53 AM Steven Price <steven.price@arm.com> wrote: > > panfrost_ioctl_mmap_bo() contains a reimplementation of > drm_gem_map_offset() but with a bug - it allows mapping imported > objects (without going through the exporter). Fix this by switching to > use the newly renamed drm_gem_map_offset() function instead which has > the bonus of simplifying the code. While it may have been a bug, it worked (by some definition of worked). Now mesa breaks on importing buffers which always get mmapped. So we need to revert this, get import mmaps to work, or drop mmapping of imports and backport that to 19.1. I don't think there should be any need to mmap imports. Rob ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-07-03 2:32 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-06-27 15:53 [PATCH v4 0/2] drm/panfrost: drm_gem_map_offset() helper Steven Price 2019-06-27 15:53 ` [PATCH v4 1/2] drm/gem: Rename drm_gem_dumb_map_offset() to drm_gem_map_offset() Steven Price 2019-06-27 17:57 ` Rob Herring 2019-06-27 21:35 ` Daniel Vetter 2019-06-27 15:53 ` [PATCH v4 2/2] drm/panfrost: Use drm_gem_map_offset() Steven Price 2019-07-03 2:32 ` Rob Herring
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox