LKML Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] drm/panfrost: Use drm_gem_dump_map_offset()
@ 2019-05-13 14:32 Steven Price
  2019-05-13 14:39 ` Daniel Vetter
  0 siblings, 1 reply; 5+ messages in thread
From: Steven Price @ 2019-05-13 14:32 UTC (permalink / raw)
  To: Rob Herring, Tomeu Vizoso
  Cc: Daniel Vetter, David Airlie, dri-devel, linux-kernel,
	Steven Price, Alyssa Rosenzweig

panfrost_ioctl_mmap_bo() contains a reimplementation of
drm_gem_dump_map_offset() but with a bug - it allows mapping imported
objects (without going through the exporter). Fix this by switching to
use the generic drm_gem_dump_map_offset() function instead which has the
bonus of simplifying the code.

CC: Alyssa Rosenzweig <alyssa@rosenzweig.io>
Signed-off-by: Steven Price <steven.price@arm.com>
---
 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 94b0819ad50b..d048250ad8ab 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -254,26 +254,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_dumb_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	[flat|nested] 5+ messages in thread

* Re: [PATCH] drm/panfrost: Use drm_gem_dump_map_offset()
  2019-05-13 14:32 [PATCH] drm/panfrost: Use drm_gem_dump_map_offset() Steven Price
@ 2019-05-13 14:39 ` Daniel Vetter
  2019-05-13 14:47   ` Chris Wilson
  0 siblings, 1 reply; 5+ messages in thread
From: Daniel Vetter @ 2019-05-13 14:39 UTC (permalink / raw)
  To: Steven Price
  Cc: Rob Herring, Tomeu Vizoso, Daniel Vetter, David Airlie,
	dri-devel, linux-kernel, Alyssa Rosenzweig

On Mon, May 13, 2019 at 03:32:44PM +0100, Steven Price wrote:
> panfrost_ioctl_mmap_bo() contains a reimplementation of
> drm_gem_dump_map_offset() but with a bug - it allows mapping imported
> objects (without going through the exporter). Fix this by switching to
> use the generic drm_gem_dump_map_offset() function instead which has the
> bonus of simplifying the code.

gem_dumb stuff is for kms drivers, panfrost is a render driver. We're
generally trying to separate these two worlds somewhat cleanly.

I think it'd be good to have a non-dumb version of this in the core, and
use that. Or upgrade the dumb version to be that helper for everyone (and
drop the _dumb).
-Daniel

> CC: Alyssa Rosenzweig <alyssa@rosenzweig.io>
> Signed-off-by: Steven Price <steven.price@arm.com>
> ---
>  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 94b0819ad50b..d048250ad8ab 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -254,26 +254,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_dumb_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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] drm/panfrost: Use drm_gem_dump_map_offset()
  2019-05-13 14:39 ` Daniel Vetter
@ 2019-05-13 14:47   ` Chris Wilson
  2019-05-13 15:14     ` Steven Price
  0 siblings, 1 reply; 5+ messages in thread
From: Chris Wilson @ 2019-05-13 14:47 UTC (permalink / raw)
  To: Daniel Vetter, Steven Price
  Cc: Tomeu Vizoso, David Airlie, linux-kernel, dri-devel, Alyssa Rosenzweig

Quoting Daniel Vetter (2019-05-13 15:39:21)
> On Mon, May 13, 2019 at 03:32:44PM +0100, Steven Price wrote:
> > panfrost_ioctl_mmap_bo() contains a reimplementation of
> > drm_gem_dump_map_offset() but with a bug - it allows mapping imported
> > objects (without going through the exporter). Fix this by switching to
> > use the generic drm_gem_dump_map_offset() function instead which has the
> > bonus of simplifying the code.
> 
> gem_dumb stuff is for kms drivers, panfrost is a render driver. We're
> generally trying to separate these two worlds somewhat cleanly.
> 
> I think it'd be good to have a non-dumb version of this in the core, and
> use that. Or upgrade the dumb version to be that helper for everyone (and
> drop the _dumb).

More specifically, since panfrost is using the drm_gem_shmem helper and
vm_ops, it too can provide the wrapper as it is the drm_gem_shmem layer
that implies that trying to mmap an imported object is an issue as that
is not a universal problem.
-Chris

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] drm/panfrost: Use drm_gem_dump_map_offset()
  2019-05-13 14:47   ` Chris Wilson
@ 2019-05-13 15:14     ` Steven Price
  2019-05-13 16:46       ` Chris Wilson
  0 siblings, 1 reply; 5+ messages in thread
From: Steven Price @ 2019-05-13 15:14 UTC (permalink / raw)
  To: Chris Wilson, Daniel Vetter
  Cc: David Airlie, Alyssa Rosenzweig, dri-devel, linux-kernel, Tomeu Vizoso

On 13/05/2019 15:47, Chris Wilson wrote:
> Quoting Daniel Vetter (2019-05-13 15:39:21)
>> On Mon, May 13, 2019 at 03:32:44PM +0100, Steven Price wrote:
>>> panfrost_ioctl_mmap_bo() contains a reimplementation of
>>> drm_gem_dump_map_offset() but with a bug - it allows mapping imported
>>> objects (without going through the exporter). Fix this by switching to
>>> use the generic drm_gem_dump_map_offset() function instead which has the
>>> bonus of simplifying the code.
>>
>> gem_dumb stuff is for kms drivers, panfrost is a render driver. We're
>> generally trying to separate these two worlds somewhat cleanly.
>>
>> I think it'd be good to have a non-dumb version of this in the core, and
>> use that. Or upgrade the dumb version to be that helper for everyone (and
>> drop the _dumb).

I'm slightly confused by what you think is the best course of action here.

I can create a patch dropping the '_dumb' from drm_gem_dump_map_offset()
if that's the objection. Or do you want a helper function with two
callers (the 'dump' and the 'shmem' versions)?

> More specifically, since panfrost is using the drm_gem_shmem helper and
> vm_ops, it too can provide the wrapper as it is the drm_gem_shmem layer
> that implies that trying to mmap an imported object is an issue as that
> is not a universal problem.
mmap'ing an imported object isn't a problem as such. But rather than
going behind the exporter's back we would need to call dma_buf_mmap() to
ensure that the exporter can do whatever is necessary.

I could add a wrapper in drm_gem_shmem_helper, but I'm not sure what the
benefit of a wrapper here is?

Steve

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] drm/panfrost: Use drm_gem_dump_map_offset()
  2019-05-13 15:14     ` Steven Price
@ 2019-05-13 16:46       ` Chris Wilson
  0 siblings, 0 replies; 5+ messages in thread
From: Chris Wilson @ 2019-05-13 16:46 UTC (permalink / raw)
  To: Daniel Vetter, Steven Price
  Cc: David Airlie, Alyssa Rosenzweig, dri-devel, linux-kernel, Tomeu Vizoso

Quoting Steven Price (2019-05-13 16:14:01)
> On 13/05/2019 15:47, Chris Wilson wrote:
> > Quoting Daniel Vetter (2019-05-13 15:39:21)
> >> On Mon, May 13, 2019 at 03:32:44PM +0100, Steven Price wrote:
> >>> panfrost_ioctl_mmap_bo() contains a reimplementation of
> >>> drm_gem_dump_map_offset() but with a bug - it allows mapping imported
> >>> objects (without going through the exporter). Fix this by switching to
> >>> use the generic drm_gem_dump_map_offset() function instead which has the
> >>> bonus of simplifying the code.
> >>
> >> gem_dumb stuff is for kms drivers, panfrost is a render driver. We're
> >> generally trying to separate these two worlds somewhat cleanly.
> >>
> >> I think it'd be good to have a non-dumb version of this in the core, and
> >> use that. Or upgrade the dumb version to be that helper for everyone (and
> >> drop the _dumb).
> 
> I'm slightly confused by what you think is the best course of action here.
> 
> I can create a patch dropping the '_dumb' from drm_gem_dump_map_offset()
> if that's the objection. Or do you want a helper function with two
> callers (the 'dump' and the 'shmem' versions)?
> 
> > More specifically, since panfrost is using the drm_gem_shmem helper and
> > vm_ops, it too can provide the wrapper as it is the drm_gem_shmem layer
> > that implies that trying to mmap an imported object is an issue as that
> > is not a universal problem.
> mmap'ing an imported object isn't a problem as such. But rather than
> going behind the exporter's back we would need to call dma_buf_mmap() to
> ensure that the exporter can do whatever is necessary.
> 
> I could add a wrapper in drm_gem_shmem_helper, but I'm not sure what the
> benefit of a wrapper here is?

No, my point is that the pagefaulting is being provided by drm_gem_shmem
and that is relying on direct access to struct page, hence that is
imposing the restriction that it can only handle its own shmem objects.
The driver should not guess the helper, but ask the helper if it can
perform an mmap of this object, and since this object is not an shmem
object it will say no.

Different drivers can use their HW to fault in indirect access to dmabuf
(treating the dmabuf as a conveyance of a sglist and not struct page)
and so do not need to different/reject foreign objects from their
specialised mmap routines and are oblivious to them.
-Chris

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-13 14:32 [PATCH] drm/panfrost: Use drm_gem_dump_map_offset() Steven Price
2019-05-13 14:39 ` Daniel Vetter
2019-05-13 14:47   ` Chris Wilson
2019-05-13 15:14     ` Steven Price
2019-05-13 16:46       ` Chris Wilson

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git