[PATCH v3 0/2] drm/dumb-buffers: Add defaults for .dumb_map_offset and .dumb_destroy

[PATCH v3 0/2] drm/dumb-buffers: Add defaults for .dumb_map_offset and .dumb_destroy

[Noralf Trønnes]

This is a follow up that lets armada also use the default
.dumb_map_offset.

First version:
https://lists.freedesktop.org/archives/dri-devel/2017-July/148101.html

Changes since version 2
-----------------------
- drm_gem_dumb_map_offset(): reject imported buffers (Daniel Vetter)
- armada can now also use drm_gem_dumb_map_offset()

Changes since version 1
-----------------------
- Exynos can also use drm_gem_dumb_map_offset() (Emil Velikov)
- Remove drm_gem_cma_dumb_map_offset() (Philipp Zabel)

Noralf.

Noralf Trønnes (2):
  drm/gem: drm_gem_dumb_map_offset(): reject dma-buf
  drm/armada: Use .dumb_map_offset and .dumb_destroy defaults

 drivers/gpu/drm/armada/armada_drv.c |  2 --
 drivers/gpu/drm/armada/armada_gem.c | 36 ------------------------------------
 drivers/gpu/drm/armada/armada_gem.h |  4 ----
 drivers/gpu/drm/drm_gem.c           |  6 ++++++
 4 files changed, 6 insertions(+), 42 deletions(-)

-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 1/2] drm/gem: drm_gem_dumb_map_offset(): reject dma-buf

[Noralf Trønnes]

Reject mapping an imported dma-buf since is's an invalid use-case.

Cc: Philipp Zabel <p.zabel@pengutronix.de>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/drm_gem.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index ad4e9cf..8da5801 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -333,6 +333,12 @@ int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
 	if (!obj)
 		return -ENOENT;
 
+	/* Don't allow imported objects to be mapped */
+	if (obj->import_attach) {
+		ret = -EINVAL;
+		goto out;
+	}
+
 	ret = drm_gem_create_mmap_offset(obj);
 	if (ret)
 		goto out;
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 2/2] drm/armada: Use .dumb_map_offset and .dumb_destroy defaults

[Noralf Trønnes]

This driver can use the drm_driver.dumb_destroy and
drm_driver.dumb_map_offset defaults, so no need to set them.

Cc: Russell King <linux@armlinux.org.uk>
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/armada/armada_drv.c |  2 --
 drivers/gpu/drm/armada/armada_gem.c | 36 ------------------------------------
 drivers/gpu/drm/armada/armada_gem.h |  4 ----
 3 files changed, 42 deletions(-)

diff --git a/drivers/gpu/drm/armada/armada_drv.c b/drivers/gpu/drm/armada/armada_drv.c
index 0b3227c..2fbd9d3 100644
--- a/drivers/gpu/drm/armada/armada_drv.c
+++ b/drivers/gpu/drm/armada/armada_drv.c
@@ -70,8 +70,6 @@ static struct drm_driver armada_drm_driver = {
 	.gem_prime_export	= armada_gem_prime_export,
 	.gem_prime_import	= armada_gem_prime_import,
 	.dumb_create		= armada_gem_dumb_create,
-	.dumb_map_offset	= armada_gem_dumb_map_offset,
-	.dumb_destroy		= armada_gem_dumb_destroy,
 	.gem_vm_ops		= &armada_gem_vm_ops,
 	.major			= 1,
 	.minor			= 0,
diff --git a/drivers/gpu/drm/armada/armada_gem.c b/drivers/gpu/drm/armada/armada_gem.c
index a76ca21..7983538 100644
--- a/drivers/gpu/drm/armada/armada_gem.c
+++ b/drivers/gpu/drm/armada/armada_gem.c
@@ -270,42 +270,6 @@ int armada_gem_dumb_create(struct drm_file *file, struct drm_device *dev,
 	return ret;
 }
 
-int armada_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
-	uint32_t handle, uint64_t *offset)
-{
-	struct armada_gem_object *obj;
-	int ret = 0;
-
-	obj = armada_gem_object_lookup(file, handle);
-	if (!obj) {
-		DRM_ERROR("failed to lookup gem object\n");
-		return -EINVAL;
-	}
-
-	/* Don't allow imported objects to be mapped */
-	if (obj->obj.import_attach) {
-		ret = -EINVAL;
-		goto err_unref;
-	}
-
-	ret = drm_gem_create_mmap_offset(&obj->obj);
-	if (ret == 0) {
-		*offset = drm_vma_node_offset_addr(&obj->obj.vma_node);
-		DRM_DEBUG_DRIVER("handle %#x offset %llx\n", handle, *offset);
-	}
-
- err_unref:
-	drm_gem_object_unreference_unlocked(&obj->obj);
-
-	return ret;
-}
-
-int armada_gem_dumb_destroy(struct drm_file *file, struct drm_device *dev,
-	uint32_t handle)
-{
-	return drm_gem_handle_delete(file, handle);
-}
-
 /* Private driver gem ioctls */
 int armada_gem_create_ioctl(struct drm_device *dev, void *data,
 	struct drm_file *file)
diff --git a/drivers/gpu/drm/armada/armada_gem.h b/drivers/gpu/drm/armada/armada_gem.h
index 6e524e0..1ac9079 100644
--- a/drivers/gpu/drm/armada/armada_gem.h
+++ b/drivers/gpu/drm/armada/armada_gem.h
@@ -35,10 +35,6 @@ struct armada_gem_object *armada_gem_alloc_private_object(struct drm_device *,
 	size_t);
 int armada_gem_dumb_create(struct drm_file *, struct drm_device *,
 	struct drm_mode_create_dumb *);
-int armada_gem_dumb_map_offset(struct drm_file *, struct drm_device *,
-	uint32_t, uint64_t *);
-int armada_gem_dumb_destroy(struct drm_file *, struct drm_device *,
-	uint32_t);
 struct dma_buf *armada_gem_prime_export(struct drm_device *dev,
 	struct drm_gem_object *obj, int flags);
 struct drm_gem_object *armada_gem_prime_import(struct drm_device *,
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/2] drm/gem: drm_gem_dumb_map_offset(): reject dma-buf

[Laurent Pinchart]

Hi Noralf,

Thank you for the patch.

On Thursday 17 Aug 2017 18:21:30 Noralf Trønnes wrote:
> Reject mapping an imported dma-buf since is's an invalid use-case.
> 
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>  drivers/gpu/drm/drm_gem.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index ad4e9cf..8da5801 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -333,6 +333,12 @@ int drm_gem_dumb_map_offset(struct drm_file *file,
> struct drm_device *dev, if (!obj)
>  		return -ENOENT;
> 
> +	/* Don't allow imported objects to be mapped */
> +	if (obj->import_attach) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +

Wouldn't it be better to move this check to drm_gem_create_mmap_offset_size() 
to reject mapping of all imported dmabuf, not just the ones associated with 
dumb buffers ?

>  	ret = drm_gem_create_mmap_offset(obj);
>  	if (ret)
>  		goto out;

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/2] drm/gem: drm_gem_dumb_map_offset(): reject dma-buf

[Chris Wilson]

Quoting Laurent Pinchart (2017-08-17 18:56:09)
> Hi Noralf,
> 
> Thank you for the patch.
> 
> On Thursday 17 Aug 2017 18:21:30 Noralf Trønnes wrote:
> > Reject mapping an imported dma-buf since is's an invalid use-case.
> > 
> > Cc: Philipp Zabel <p.zabel@pengutronix.de>
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: Sean Paul <seanpaul@chromium.org>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> > ---
> >  drivers/gpu/drm/drm_gem.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > index ad4e9cf..8da5801 100644
> > --- a/drivers/gpu/drm/drm_gem.c
> > +++ b/drivers/gpu/drm/drm_gem.c
> > @@ -333,6 +333,12 @@ int drm_gem_dumb_map_offset(struct drm_file *file,
> > struct drm_device *dev, if (!obj)
> >               return -ENOENT;
> > 
> > +     /* Don't allow imported objects to be mapped */
> > +     if (obj->import_attach) {
> > +             ret = -EINVAL;
> > +             goto out;
> > +     }
> > +
> 
> Wouldn't it be better to move this check to drm_gem_create_mmap_offset_size() 
> to reject mapping of all imported dmabuf, not just the ones associated with 
> dumb buffers ?

No, we use that and we don't ban mapping an imported object.
-Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/2] drm/gem: drm_gem_dumb_map_offset(): reject dma-buf

[Laurent Pinchart]

Hi Chris,

On Thursday 17 Aug 2017 19:30:44 Chris Wilson wrote:
> Quoting Laurent Pinchart (2017-08-17 18:56:09)
> > On Thursday 17 Aug 2017 18:21:30 Noralf Trønnes wrote:
> > > Reject mapping an imported dma-buf since is's an invalid use-case.
> > > 
> > > Cc: Philipp Zabel <p.zabel@pengutronix.de>
> > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Cc: Sean Paul <seanpaul@chromium.org>
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> > > ---
> > > 
> > >  drivers/gpu/drm/drm_gem.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > > index ad4e9cf..8da5801 100644
> > > --- a/drivers/gpu/drm/drm_gem.c
> > > +++ b/drivers/gpu/drm/drm_gem.c
> > > @@ -333,6 +333,12 @@ int drm_gem_dumb_map_offset(struct drm_file *file,
> > > struct drm_device *dev, if (!obj)
> > > 
> > >               return -ENOENT;
> > > 
> > > +     /* Don't allow imported objects to be mapped */
> > > +     if (obj->import_attach) {
> > > +             ret = -EINVAL;
> > > +             goto out;
> > > +     }
> > > +
> > 
> > Wouldn't it be better to move this check to
> > drm_gem_create_mmap_offset_size() to reject mapping of all imported
> > dmabuf, not just the ones associated with dumb buffers ?
> 
> No, we use that and we don't ban mapping an imported object.

Do you use that with driver-specific buffers only or with dumb buffers too ?

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/2] drm/gem: drm_gem_dumb_map_offset(): reject dma-buf

[Chris Wilson]

Quoting Laurent Pinchart (2017-08-17 20:01:40)
> Hi Chris,
> 
> On Thursday 17 Aug 2017 19:30:44 Chris Wilson wrote:
> > Quoting Laurent Pinchart (2017-08-17 18:56:09)
> > > On Thursday 17 Aug 2017 18:21:30 Noralf Trønnes wrote:
> > > > Reject mapping an imported dma-buf since is's an invalid use-case.
> > > > 
> > > > Cc: Philipp Zabel <p.zabel@pengutronix.de>
> > > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > Cc: Sean Paul <seanpaul@chromium.org>
> > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> > > > ---
> > > > 
> > > >  drivers/gpu/drm/drm_gem.c | 6 ++++++
> > > >  1 file changed, 6 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > > > index ad4e9cf..8da5801 100644
> > > > --- a/drivers/gpu/drm/drm_gem.c
> > > > +++ b/drivers/gpu/drm/drm_gem.c
> > > > @@ -333,6 +333,12 @@ int drm_gem_dumb_map_offset(struct drm_file *file,
> > > > struct drm_device *dev, if (!obj)
> > > > 
> > > >               return -ENOENT;
> > > > 
> > > > +     /* Don't allow imported objects to be mapped */
> > > > +     if (obj->import_attach) {
> > > > +             ret = -EINVAL;
> > > > +             goto out;
> > > > +     }
> > > > +
> > > 
> > > Wouldn't it be better to move this check to
> > > drm_gem_create_mmap_offset_size() to reject mapping of all imported
> > > dmabuf, not just the ones associated with dumb buffers ?
> > 
> > No, we use that and we don't ban mapping an imported object.
> 
> Do you use that with driver-specific buffers only or with dumb buffers too ?

For dma-buf we import? The current presumption is that the sg_table is
backed by struct page. How would we know otherwise? I am actually not
sure what would happen if we tried to use another mmio bar from the GPU.

For dumb buffers we create, they are just ordinary buffers.
-Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/2] drm/gem: drm_gem_dumb_map_offset(): reject dma-buf

[Laurent Pinchart]

Hi Chris,

On Thursday 17 Aug 2017 20:09:20 Chris Wilson wrote:
> Quoting Laurent Pinchart (2017-08-17 20:01:40)
> > On Thursday 17 Aug 2017 19:30:44 Chris Wilson wrote:
> > > Quoting Laurent Pinchart (2017-08-17 18:56:09)
> > > 
> > > > On Thursday 17 Aug 2017 18:21:30 Noralf Trønnes wrote:
> > > > > Reject mapping an imported dma-buf since is's an invalid use-case.
> > > > > 
> > > > > Cc: Philipp Zabel <p.zabel@pengutronix.de>
> > > > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > Cc: Sean Paul <seanpaul@chromium.org>
> > > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> > > > > ---
> > > > > 
> > > > >  drivers/gpu/drm/drm_gem.c | 6 ++++++
> > > > >  1 file changed, 6 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > > > > index ad4e9cf..8da5801 100644
> > > > > --- a/drivers/gpu/drm/drm_gem.c
> > > > > +++ b/drivers/gpu/drm/drm_gem.c
> > > > > @@ -333,6 +333,12 @@ int drm_gem_dumb_map_offset(struct drm_file
> > > > > *file,
> > > > > struct drm_device *dev, if (!obj)
> > > > > 
> > > > >               return -ENOENT;
> > > > > 
> > > > > +     /* Don't allow imported objects to be mapped */
> > > > > +     if (obj->import_attach) {
> > > > > +             ret = -EINVAL;
> > > > > +             goto out;
> > > > > +     }
> > > > > +
> > > > 
> > > > Wouldn't it be better to move this check to
> > > > drm_gem_create_mmap_offset_size() to reject mapping of all imported
> > > > dmabuf, not just the ones associated with dumb buffers ?
> > > 
> > > No, we use that and we don't ban mapping an imported object.
> > 
> > Do you use that with driver-specific buffers only or with dumb buffers too
> > ?
> > 
> For dma-buf we import? The current presumption is that the sg_table is
> backed by struct page. How would we know otherwise? I am actually not
> sure what would happen if we tried to use another mmio bar from the GPU.
> 
> For dumb buffers we create, they are just ordinary buffers.

My question was whether you need to mmap the dma-buf you import through the 
dumb buffers API (which this patch prevents for drivers using the GEM dumb 
helpers), or only through your driver-specific buffer API.

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/2] drm/gem: drm_gem_dumb_map_offset(): reject dma-buf

[Chris Wilson]

Quoting Laurent Pinchart (2017-08-17 20:40:52)
> Hi Chris,
> 
> On Thursday 17 Aug 2017 20:09:20 Chris Wilson wrote:
> > Quoting Laurent Pinchart (2017-08-17 20:01:40)
> > > On Thursday 17 Aug 2017 19:30:44 Chris Wilson wrote:
> > > > Quoting Laurent Pinchart (2017-08-17 18:56:09)
> > > > 
> > > > > On Thursday 17 Aug 2017 18:21:30 Noralf Trønnes wrote:
> > > > > > Reject mapping an imported dma-buf since is's an invalid use-case.
> > > > > > 
> > > > > > Cc: Philipp Zabel <p.zabel@pengutronix.de>
> > > > > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > > Cc: Sean Paul <seanpaul@chromium.org>
> > > > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> > > > > > ---
> > > > > > 
> > > > > >  drivers/gpu/drm/drm_gem.c | 6 ++++++
> > > > > >  1 file changed, 6 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > > > > > index ad4e9cf..8da5801 100644
> > > > > > --- a/drivers/gpu/drm/drm_gem.c
> > > > > > +++ b/drivers/gpu/drm/drm_gem.c
> > > > > > @@ -333,6 +333,12 @@ int drm_gem_dumb_map_offset(struct drm_file
> > > > > > *file,
> > > > > > struct drm_device *dev, if (!obj)
> > > > > > 
> > > > > >               return -ENOENT;
> > > > > > 
> > > > > > +     /* Don't allow imported objects to be mapped */
> > > > > > +     if (obj->import_attach) {
> > > > > > +             ret = -EINVAL;
> > > > > > +             goto out;
> > > > > > +     }
> > > > > > +
> > > > > 
> > > > > Wouldn't it be better to move this check to
> > > > > drm_gem_create_mmap_offset_size() to reject mapping of all imported
> > > > > dmabuf, not just the ones associated with dumb buffers ?
> > > > 
> > > > No, we use that and we don't ban mapping an imported object.
> > > 
> > > Do you use that with driver-specific buffers only or with dumb buffers too
> > > ?
> > > 
> > For dma-buf we import? The current presumption is that the sg_table is
> > backed by struct page. How would we know otherwise? I am actually not
> > sure what would happen if we tried to use another mmio bar from the GPU.
> > 
> > For dumb buffers we create, they are just ordinary buffers.
> 
> My question was whether you need to mmap the dma-buf you import through the 
> dumb buffers API (which this patch prevents for drivers using the GEM dumb 
> helpers), or only through your driver-specific buffer API.

i915_gem_mmap_gtt_ioctl -> drm_gem_create_mmap_offset/_size
drvier->dumb_map_offset also uses the i915_gem_mmap_gtt mechanism.

I was responding to the suggestion that this check could be centralised in
drm_gem_create_mmap_offset_size, as that would prevent us from offering
the mmap interface on imported buffers.
-Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/2] drm/gem: drm_gem_dumb_map_offset(): reject dma-buf

[Laurent Pinchart]

Hi Chris,

On Thursday 17 Aug 2017 21:01:54 Chris Wilson wrote:
> Quoting Laurent Pinchart (2017-08-17 20:40:52)
> > On Thursday 17 Aug 2017 20:09:20 Chris Wilson wrote:
> >> Quoting Laurent Pinchart (2017-08-17 20:01:40)
> >>> On Thursday 17 Aug 2017 19:30:44 Chris Wilson wrote:
> >>>> Quoting Laurent Pinchart (2017-08-17 18:56:09)
> >>>>> On Thursday 17 Aug 2017 18:21:30 Noralf Trønnes wrote:
> >>>>>> Reject mapping an imported dma-buf since is's an invalid
> >>>>>> use-case.
> >>>>>> 
> >>>>>> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> >>>>>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>>>>> Cc: Sean Paul <seanpaul@chromium.org>
> >>>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>>>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> >>>>>> ---
> >>>>>> 
> >>>>>>  drivers/gpu/drm/drm_gem.c | 6 ++++++
> >>>>>>  1 file changed, 6 insertions(+)
> >>>>>> 
> >>>>>> diff --git a/drivers/gpu/drm/drm_gem.c
> >>>>>> b/drivers/gpu/drm/drm_gem.c
> >>>>>> index ad4e9cf..8da5801 100644
> >>>>>> --- a/drivers/gpu/drm/drm_gem.c
> >>>>>> +++ b/drivers/gpu/drm/drm_gem.c
> >>>>>> @@ -333,6 +333,12 @@ int drm_gem_dumb_map_offset(struct drm_file
> >>>>>> *file, struct drm_device *dev,
> >>>>>>  	if (!obj)
> >>>>>>               return -ENOENT;
> >>>>>> 
> >>>>>> +     /* Don't allow imported objects to be mapped */
> >>>>>> +     if (obj->import_attach) {
> >>>>>> +             ret = -EINVAL;
> >>>>>> +             goto out;
> >>>>>> +     }
> >>>>>> +
> >>>>> 
> >>>>> Wouldn't it be better to move this check to
> >>>>> drm_gem_create_mmap_offset_size() to reject mapping of all
> >>>>> imported dmabuf, not just the ones associated with dumb buffers ?
> >>>> 
> >>>> No, we use that and we don't ban mapping an imported object.
> >>> 
> >>> Do you use that with driver-specific buffers only or with dumb buffers
> >>> too ?
> >> 
> >> For dma-buf we import? The current presumption is that the sg_table is
> >> backed by struct page. How would we know otherwise? I am actually not
> >> sure what would happen if we tried to use another mmio bar from the GPU.
> >> 
> >> For dumb buffers we create, they are just ordinary buffers.
> > 
> > My question was whether you need to mmap the dma-buf you import through
> > the dumb buffers API (which this patch prevents for drivers using the GEM
> > dumb helpers), or only through your driver-specific buffer API.
> 
> i915_gem_mmap_gtt_ioctl -> drm_gem_create_mmap_offset/_size
> drvier->dumb_map_offset also uses the i915_gem_mmap_gtt mechanism.
> 
> I was responding to the suggestion that this check could be centralised in
> drm_gem_create_mmap_offset_size, as that would prevent us from offering
> the mmap interface on imported buffers.

Fair enough. I wonder, however, why mmap()ing an imported buffer through the 
dumb buffers API is an invalid use case, while doing the same through the 
driver-specific buffer API isn't.

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/2] drm/gem: drm_gem_dumb_map_offset(): reject dma-buf

[Daniel Vetter]

On Fri, Aug 18, 2017 at 01:35:41AM +0300, Laurent Pinchart wrote:
> Hi Chris,
> 
> On Thursday 17 Aug 2017 21:01:54 Chris Wilson wrote:
> > Quoting Laurent Pinchart (2017-08-17 20:40:52)
> > > On Thursday 17 Aug 2017 20:09:20 Chris Wilson wrote:
> > >> Quoting Laurent Pinchart (2017-08-17 20:01:40)
> > >>> On Thursday 17 Aug 2017 19:30:44 Chris Wilson wrote:
> > >>>> Quoting Laurent Pinchart (2017-08-17 18:56:09)
> > >>>>> On Thursday 17 Aug 2017 18:21:30 Noralf Trønnes wrote:
> > >>>>>> Reject mapping an imported dma-buf since is's an invalid
> > >>>>>> use-case.
> > >>>>>> 
> > >>>>>> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> > >>>>>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > >>>>>> Cc: Sean Paul <seanpaul@chromium.org>
> > >>>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > >>>>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> > >>>>>> ---
> > >>>>>> 
> > >>>>>>  drivers/gpu/drm/drm_gem.c | 6 ++++++
> > >>>>>>  1 file changed, 6 insertions(+)
> > >>>>>> 
> > >>>>>> diff --git a/drivers/gpu/drm/drm_gem.c
> > >>>>>> b/drivers/gpu/drm/drm_gem.c
> > >>>>>> index ad4e9cf..8da5801 100644
> > >>>>>> --- a/drivers/gpu/drm/drm_gem.c
> > >>>>>> +++ b/drivers/gpu/drm/drm_gem.c
> > >>>>>> @@ -333,6 +333,12 @@ int drm_gem_dumb_map_offset(struct drm_file
> > >>>>>> *file, struct drm_device *dev,
> > >>>>>>  	if (!obj)
> > >>>>>>               return -ENOENT;
> > >>>>>> 
> > >>>>>> +     /* Don't allow imported objects to be mapped */
> > >>>>>> +     if (obj->import_attach) {
> > >>>>>> +             ret = -EINVAL;
> > >>>>>> +             goto out;
> > >>>>>> +     }
> > >>>>>> +
> > >>>>> 
> > >>>>> Wouldn't it be better to move this check to
> > >>>>> drm_gem_create_mmap_offset_size() to reject mapping of all
> > >>>>> imported dmabuf, not just the ones associated with dumb buffers ?
> > >>>> 
> > >>>> No, we use that and we don't ban mapping an imported object.
> > >>> 
> > >>> Do you use that with driver-specific buffers only or with dumb buffers
> > >>> too ?
> > >> 
> > >> For dma-buf we import? The current presumption is that the sg_table is
> > >> backed by struct page. How would we know otherwise? I am actually not
> > >> sure what would happen if we tried to use another mmio bar from the GPU.
> > >> 
> > >> For dumb buffers we create, they are just ordinary buffers.
> > > 
> > > My question was whether you need to mmap the dma-buf you import through
> > > the dumb buffers API (which this patch prevents for drivers using the GEM
> > > dumb helpers), or only through your driver-specific buffer API.
> > 
> > i915_gem_mmap_gtt_ioctl -> drm_gem_create_mmap_offset/_size
> > drvier->dumb_map_offset also uses the i915_gem_mmap_gtt mechanism.
> > 
> > I was responding to the suggestion that this check could be centralised in
> > drm_gem_create_mmap_offset_size, as that would prevent us from offering
> > the mmap interface on imported buffers.
> 
> Fair enough. I wonder, however, why mmap()ing an imported buffer through the 
> dumb buffers API is an invalid use case, while doing the same through the 
> driver-specific buffer API isn't.

Because i915 is special. Our offset-based mmap goes throug the gart, and
for that all we need is to dma-map the sg table, which is the primary
use-case for dma-buf. Which means we can always do that. But yeah it's
special.

In general dma-buf mmap on imported stuff isn't really a good idea, if you
want that, use the mmap on the dma-buf itself.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/2] drm/gem: drm_gem_dumb_map_offset(): reject dma-buf

[Daniel Vetter]

On Thu, Aug 17, 2017 at 06:21:30PM +0200, Noralf Trønnes wrote:
> Reject mapping an imported dma-buf since is's an invalid use-case.
> 
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>

I think acks from someone using mali would be good too. amdgpu already has
such checks, so I think on the desktop side we're ok.

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

But I think this one here definitely needs a few more acks. I could break
uabi if we're unlucky, so let's not rush it.
-Daniel

> ---
>  drivers/gpu/drm/drm_gem.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index ad4e9cf..8da5801 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -333,6 +333,12 @@ int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
>  	if (!obj)
>  		return -ENOENT;
>  
> +	/* Don't allow imported objects to be mapped */
> +	if (obj->import_attach) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
>  	ret = drm_gem_create_mmap_offset(obj);
>  	if (ret)
>  		goto out;
> -- 
> 2.7.4
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 2/2] drm/armada: Use .dumb_map_offset and .dumb_destroy defaults

[Daniel Vetter]

On Thu, Aug 17, 2017 at 06:21:31PM +0200, Noralf Trønnes wrote:
> This driver can use the drm_driver.dumb_destroy and
> drm_driver.dumb_map_offset defaults, so no need to set them.
> 
> Cc: Russell King <linux@armlinux.org.uk>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>

Assuming patch 1 gets enough acks and lands:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/armada/armada_drv.c |  2 --
>  drivers/gpu/drm/armada/armada_gem.c | 36 ------------------------------------
>  drivers/gpu/drm/armada/armada_gem.h |  4 ----
>  3 files changed, 42 deletions(-)
> 
> diff --git a/drivers/gpu/drm/armada/armada_drv.c b/drivers/gpu/drm/armada/armada_drv.c
> index 0b3227c..2fbd9d3 100644
> --- a/drivers/gpu/drm/armada/armada_drv.c
> +++ b/drivers/gpu/drm/armada/armada_drv.c
> @@ -70,8 +70,6 @@ static struct drm_driver armada_drm_driver = {
>  	.gem_prime_export	= armada_gem_prime_export,
>  	.gem_prime_import	= armada_gem_prime_import,
>  	.dumb_create		= armada_gem_dumb_create,
> -	.dumb_map_offset	= armada_gem_dumb_map_offset,
> -	.dumb_destroy		= armada_gem_dumb_destroy,
>  	.gem_vm_ops		= &armada_gem_vm_ops,
>  	.major			= 1,
>  	.minor			= 0,
> diff --git a/drivers/gpu/drm/armada/armada_gem.c b/drivers/gpu/drm/armada/armada_gem.c
> index a76ca21..7983538 100644
> --- a/drivers/gpu/drm/armada/armada_gem.c
> +++ b/drivers/gpu/drm/armada/armada_gem.c
> @@ -270,42 +270,6 @@ int armada_gem_dumb_create(struct drm_file *file, struct drm_device *dev,
>  	return ret;
>  }
>  
> -int armada_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
> -	uint32_t handle, uint64_t *offset)
> -{
> -	struct armada_gem_object *obj;
> -	int ret = 0;
> -
> -	obj = armada_gem_object_lookup(file, handle);
> -	if (!obj) {
> -		DRM_ERROR("failed to lookup gem object\n");
> -		return -EINVAL;
> -	}
> -
> -	/* Don't allow imported objects to be mapped */
> -	if (obj->obj.import_attach) {
> -		ret = -EINVAL;
> -		goto err_unref;
> -	}
> -
> -	ret = drm_gem_create_mmap_offset(&obj->obj);
> -	if (ret == 0) {
> -		*offset = drm_vma_node_offset_addr(&obj->obj.vma_node);
> -		DRM_DEBUG_DRIVER("handle %#x offset %llx\n", handle, *offset);
> -	}
> -
> - err_unref:
> -	drm_gem_object_unreference_unlocked(&obj->obj);
> -
> -	return ret;
> -}
> -
> -int armada_gem_dumb_destroy(struct drm_file *file, struct drm_device *dev,
> -	uint32_t handle)
> -{
> -	return drm_gem_handle_delete(file, handle);
> -}
> -
>  /* Private driver gem ioctls */
>  int armada_gem_create_ioctl(struct drm_device *dev, void *data,
>  	struct drm_file *file)
> diff --git a/drivers/gpu/drm/armada/armada_gem.h b/drivers/gpu/drm/armada/armada_gem.h
> index 6e524e0..1ac9079 100644
> --- a/drivers/gpu/drm/armada/armada_gem.h
> +++ b/drivers/gpu/drm/armada/armada_gem.h
> @@ -35,10 +35,6 @@ struct armada_gem_object *armada_gem_alloc_private_object(struct drm_device *,
>  	size_t);
>  int armada_gem_dumb_create(struct drm_file *, struct drm_device *,
>  	struct drm_mode_create_dumb *);
> -int armada_gem_dumb_map_offset(struct drm_file *, struct drm_device *,
> -	uint32_t, uint64_t *);
> -int armada_gem_dumb_destroy(struct drm_file *, struct drm_device *,
> -	uint32_t);
>  struct dma_buf *armada_gem_prime_export(struct drm_device *dev,
>  	struct drm_gem_object *obj, int flags);
>  struct drm_gem_object *armada_gem_prime_import(struct drm_device *,
> -- 
> 2.7.4
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/2] drm/gem: drm_gem_dumb_map_offset(): reject dma-buf

[Laurent Pinchart]

Hi Daniel,

On Friday 18 Aug 2017 09:45:48 Daniel Vetter wrote:
> On Fri, Aug 18, 2017 at 01:35:41AM +0300, Laurent Pinchart wrote:
> > On Thursday 17 Aug 2017 21:01:54 Chris Wilson wrote:
> >> Quoting Laurent Pinchart (2017-08-17 20:40:52)
> >>> On Thursday 17 Aug 2017 20:09:20 Chris Wilson wrote:
> >>>> Quoting Laurent Pinchart (2017-08-17 20:01:40)
> >>>>> On Thursday 17 Aug 2017 19:30:44 Chris Wilson wrote:
> >>>>>> Quoting Laurent Pinchart (2017-08-17 18:56:09)
> >>>>>>> On Thursday 17 Aug 2017 18:21:30 Noralf Trønnes wrote:
> >>>>>>>> Reject mapping an imported dma-buf since is's an invalid
> >>>>>>>> use-case.
> >>>>>>>> 
> >>>>>>>> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> >>>>>>>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>>>>>>> Cc: Sean Paul <seanpaul@chromium.org>
> >>>>>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>>>>>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> >>>>>>>> ---
> >>>>>>>> 
> >>>>>>>>  drivers/gpu/drm/drm_gem.c | 6 ++++++
> >>>>>>>>  1 file changed, 6 insertions(+)
> >>>>>>>> 
> >>>>>>>> diff --git a/drivers/gpu/drm/drm_gem.c
> >>>>>>>> b/drivers/gpu/drm/drm_gem.c
> >>>>>>>> index ad4e9cf..8da5801 100644
> >>>>>>>> --- a/drivers/gpu/drm/drm_gem.c
> >>>>>>>> +++ b/drivers/gpu/drm/drm_gem.c
> >>>>>>>> @@ -333,6 +333,12 @@ int drm_gem_dumb_map_offset(struct drm_file
> >>>>>>>> *file, struct drm_device *dev,
> >>>>>>>>  	if (!obj)
> >>>>>>>>               return -ENOENT;
> >>>>>>>> 
> >>>>>>>> +     /* Don't allow imported objects to be mapped */
> >>>>>>>> +     if (obj->import_attach) {
> >>>>>>>> +             ret = -EINVAL;
> >>>>>>>> +             goto out;
> >>>>>>>> +     }
> >>>>>>>> +
> >>>>>>> 
> >>>>>>> Wouldn't it be better to move this check to
> >>>>>>> drm_gem_create_mmap_offset_size() to reject mapping of all
> >>>>>>> imported dmabuf, not just the ones associated with dumb buffers ?
> >>>>>> 
> >>>>>> No, we use that and we don't ban mapping an imported object.
> >>>>> 
> >>>>> Do you use that with driver-specific buffers only or with dumb
> >>>>> buffers too ?
> >>>> 
> >>>> For dma-buf we import? The current presumption is that the sg_table
> >>>> is backed by struct page. How would we know otherwise? I am actually
> >>>> not sure what would happen if we tried to use another mmio bar from
> >>>> the GPU.
> >>>> 
> >>>> For dumb buffers we create, they are just ordinary buffers.
> >>> 
> >>> My question was whether you need to mmap the dma-buf you import
> >>> through the dumb buffers API (which this patch prevents for drivers
> >>> using the GEM dumb helpers), or only through your driver-specific
> >>> buffer API.
> >> 
> >> i915_gem_mmap_gtt_ioctl -> drm_gem_create_mmap_offset/_size
> >> drvier->dumb_map_offset also uses the i915_gem_mmap_gtt mechanism.
> >> 
> >> I was responding to the suggestion that this check could be centralised
> >> in drm_gem_create_mmap_offset_size, as that would prevent us from
> >> offering the mmap interface on imported buffers.
> > 
> > Fair enough. I wonder, however, why mmap()ing an imported buffer through
> > the dumb buffers API is an invalid use case, while doing the same through
> > the driver-specific buffer API isn't.
> 
> Because i915 is special. Our offset-based mmap goes throug the gart, and
> for that all we need is to dma-map the sg table, which is the primary
> use-case for dma-buf. Which means we can always do that. But yeah it's
> special.

I'll take this as meaning that i915 shouldn't allow this use case, but does 
and will need to continue doing so because we can't break userspace (it would 
be nice to start moving userspace away from mmap()ing imported buffers 
though).

> In general dma-buf mmap on imported stuff isn't really a good idea, if you
> want that, use the mmap on the dma-buf itself.

Should we disallow this in all drivers that don't depend on it yet ?

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/2] drm/gem: drm_gem_dumb_map_offset(): reject dma-buf

[Chris Wilson]

Quoting Laurent Pinchart (2017-08-18 09:01:38)
> Hi Daniel,
> 
> On Friday 18 Aug 2017 09:45:48 Daniel Vetter wrote:
> > On Fri, Aug 18, 2017 at 01:35:41AM +0300, Laurent Pinchart wrote:
> > > Fair enough. I wonder, however, why mmap()ing an imported buffer through
> > > the dumb buffers API is an invalid use case, while doing the same through
> > > the driver-specific buffer API isn't.
> > 
> > Because i915 is special. Our offset-based mmap goes throug the gart, and
> > for that all we need is to dma-map the sg table, which is the primary
> > use-case for dma-buf. Which means we can always do that. But yeah it's
> > special.
> 
> I'll take this as meaning that i915 shouldn't allow this use case, but does 
> and will need to continue doing so because we can't break userspace (it would 
> be nice to start moving userspace away from mmap()ing imported buffers 
> though).

We do like pretend ingthat all GEM handles userspace has are equal. Partly
this is due to the lack of side-channels in protocols like DRI2/DRI3 where
all shared buffers must be fully capable or else either party may die in
weird ways. Keeping everything first class allows for a simple agnostic
userspace. Alas that is not the case, but the one thing we do have atm
is that we can rely on using a mmap via the GTT. If we throw that out
the window, we have teach all of our userspace about transfer buffers as
the final fallback. (Yes, history repeats.)
-Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/2] drm/gem: drm_gem_dumb_map_offset(): reject dma-buf

[Noralf Trønnes]

(cc affected parties)


Den 18.08.2017 09.46, skrev Daniel Vetter:
> On Thu, Aug 17, 2017 at 06:21:30PM +0200, Noralf Trønnes wrote:
>> Reject mapping an imported dma-buf since is's an invalid use-case.
>>
>> Cc: Philipp Zabel <p.zabel@pengutronix.de>
>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Cc: Sean Paul <seanpaul@chromium.org>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> I think acks from someone using mali would be good too. amdgpu already has
> such checks, so I think on the desktop side we're ok.
>
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> But I think this one here definitely needs a few more acks. I could break
> uabi if we're unlucky, so let's not rush it.

Ok, I've CC'ed the affected parties to increase the odds that they look
at this. These are the drivers using drm_gem_dumb_map_offset()
(hopefully I got the list right):

arc
atmel-hlcdc
cirrus
exynos
fsl-dcu
gma500
hdlcd
imx
kirin
mali-dp
mediatek
meson
mxsfb
pl111
rcar-du
rockchip
shmobile
sti
stm
sun4i
tegra
tilcd
vc4
zte


Noralf.

> -Daniel
>
>> ---
>>   drivers/gpu/drm/drm_gem.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
>> index ad4e9cf..8da5801 100644
>> --- a/drivers/gpu/drm/drm_gem.c
>> +++ b/drivers/gpu/drm/drm_gem.c
>> @@ -333,6 +333,12 @@ int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
>>   	if (!obj)
>>   		return -ENOENT;
>>   
>> +	/* Don't allow imported objects to be mapped */
>> +	if (obj->import_attach) {
>> +		ret = -EINVAL;
>> +		goto out;
>> +	}
>> +
>>   	ret = drm_gem_create_mmap_offset(obj);
>>   	if (ret)
>>   		goto out;
>> -- 
>> 2.7.4
>>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/2] drm/gem: drm_gem_dumb_map_offset(): reject dma-buf

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 1287 bytes --]

Noralf Trønnes <noralf@tronnes.org> writes:

> (cc affected parties)
>
>
> Den 18.08.2017 09.46, skrev Daniel Vetter:
>> On Thu, Aug 17, 2017 at 06:21:30PM +0200, Noralf Trønnes wrote:
>>> Reject mapping an imported dma-buf since is's an invalid use-case.
>>>
>>> Cc: Philipp Zabel <p.zabel@pengutronix.de>
>>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> Cc: Sean Paul <seanpaul@chromium.org>
>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> I think acks from someone using mali would be good too. amdgpu already has
>> such checks, so I think on the desktop side we're ok.
>>
>> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>
>> But I think this one here definitely needs a few more acks. I could break
>> uabi if we're unlucky, so let's not rush it.
>
> Ok, I've CC'ed the affected parties to increase the odds that they look
> at this. These are the drivers using drm_gem_dumb_map_offset()
> (hopefully I got the list right):

If I understand the affected path right, this would break the PL111+VC4
combination: PL111 makes (dumb) buffers for scanout, and VC4 imports
them and uses them for rendering.  A vc4 glReadPixels of the window
system buffer would map it and fail.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/2] drm/gem: drm_gem_dumb_map_offset(): reject dma-buf

[Daniel Vetter]

On Fri, Aug 18, 2017 at 10:41:21AM -0700, Eric Anholt wrote:
> Noralf Trønnes <noralf@tronnes.org> writes:
> > Den 18.08.2017 09.46, skrev Daniel Vetter:
> >> On Thu, Aug 17, 2017 at 06:21:30PM +0200, Noralf Trønnes wrote:
> >>> Reject mapping an imported dma-buf since is's an invalid use-case.
> >>>
> >>> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> >>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>> Cc: Sean Paul <seanpaul@chromium.org>
> >>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> >> I think acks from someone using mali would be good too. amdgpu already has
> >> such checks, so I think on the desktop side we're ok.
> >>
> >> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>
> >> But I think this one here definitely needs a few more acks. I could break
> >> uabi if we're unlucky, so let's not rush it.
> >
> > Ok, I've CC'ed the affected parties to increase the odds that they look
> > at this. These are the drivers using drm_gem_dumb_map_offset()
> > (hopefully I got the list right):
> 
> If I understand the affected path right, this would break the PL111+VC4
> combination: PL111 makes (dumb) buffers for scanout, and VC4 imports
> them and uses them for rendering.  A vc4 glReadPixels of the window
> system buffer would map it and fail.

It only rejects the map call on dumb buffers, and mmap on imported dma-buf
tends to not really work well, or at least break a few abstractions. Are
you sure this works currently?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/2] drm/gem: drm_gem_dumb_map_offset(): reject dma-buf

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 2100 bytes --]

Daniel Vetter <daniel@ffwll.ch> writes:

> On Fri, Aug 18, 2017 at 10:41:21AM -0700, Eric Anholt wrote:
>> Noralf Trønnes <noralf@tronnes.org> writes:
>> > Den 18.08.2017 09.46, skrev Daniel Vetter:
>> >> On Thu, Aug 17, 2017 at 06:21:30PM +0200, Noralf Trønnes wrote:
>> >>> Reject mapping an imported dma-buf since is's an invalid use-case.
>> >>>
>> >>> Cc: Philipp Zabel <p.zabel@pengutronix.de>
>> >>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> >>> Cc: Sean Paul <seanpaul@chromium.org>
>> >>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> >>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> >> I think acks from someone using mali would be good too. amdgpu already has
>> >> such checks, so I think on the desktop side we're ok.
>> >>
>> >> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> >>
>> >> But I think this one here definitely needs a few more acks. I could break
>> >> uabi if we're unlucky, so let's not rush it.
>> >
>> > Ok, I've CC'ed the affected parties to increase the odds that they look
>> > at this. These are the drivers using drm_gem_dumb_map_offset()
>> > (hopefully I got the list right):
>> 
>> If I understand the affected path right, this would break the PL111+VC4
>> combination: PL111 makes (dumb) buffers for scanout, and VC4 imports
>> them and uses them for rendering.  A vc4 glReadPixels of the window
>> system buffer would map it and fail.
>
> It only rejects the map call on dumb buffers, and mmap on imported dma-buf
> tends to not really work well, or at least break a few abstractions. Are
> you sure this works currently?

OK, that's right -- vc4 would be doing its "native" map call (the same
code), not dumb map.

Furthermore, I had it backwards (I had written things both ways at
different points, iirc).  We have VC4 making the buffers and PL111
dma-buf importing them.  I don't see X11 mapping those buffers if glamor
is enabled, so this should be OK for vc4.

GBM's dumb mapping looks safe to me.  X11 does some dumb maps, but I
don't think any of those would be on imports.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/2] drm/gem: drm_gem_dumb_map_offset(): reject dma-buf

[Daniel Vetter]

On Fri, Aug 18, 2017 at 01:34:45PM -0700, Eric Anholt wrote:
> Daniel Vetter <daniel@ffwll.ch> writes:
> 
> > On Fri, Aug 18, 2017 at 10:41:21AM -0700, Eric Anholt wrote:
> >> Noralf Trønnes <noralf@tronnes.org> writes:
> >> > Den 18.08.2017 09.46, skrev Daniel Vetter:
> >> >> On Thu, Aug 17, 2017 at 06:21:30PM +0200, Noralf Trønnes wrote:
> >> >>> Reject mapping an imported dma-buf since is's an invalid use-case.
> >> >>>
> >> >>> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> >> >>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> >>> Cc: Sean Paul <seanpaul@chromium.org>
> >> >>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> >>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> >> >> I think acks from someone using mali would be good too. amdgpu already has
> >> >> such checks, so I think on the desktop side we're ok.
> >> >>
> >> >> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> >>
> >> >> But I think this one here definitely needs a few more acks. I could break
> >> >> uabi if we're unlucky, so let's not rush it.
> >> >
> >> > Ok, I've CC'ed the affected parties to increase the odds that they look
> >> > at this. These are the drivers using drm_gem_dumb_map_offset()
> >> > (hopefully I got the list right):
> >> 
> >> If I understand the affected path right, this would break the PL111+VC4
> >> combination: PL111 makes (dumb) buffers for scanout, and VC4 imports
> >> them and uses them for rendering.  A vc4 glReadPixels of the window
> >> system buffer would map it and fail.
> >
> > It only rejects the map call on dumb buffers, and mmap on imported dma-buf
> > tends to not really work well, or at least break a few abstractions. Are
> > you sure this works currently?
> 
> OK, that's right -- vc4 would be doing its "native" map call (the same
> code), not dumb map.
> 
> Furthermore, I had it backwards (I had written things both ways at
> different points, iirc).  We have VC4 making the buffers and PL111
> dma-buf importing them.  I don't see X11 mapping those buffers if glamor
> is enabled, so this should be OK for vc4.
> 
> GBM's dumb mapping looks safe to me.  X11 does some dumb maps, but I
> don't think any of those would be on imports.

Yeah the idea is only to lock down the dumb mmap and make sure abi abuse
(which might work on a specific combo of exporters/kms drivers) is caught
for this generic interface. dumb really should only be used for
unaccelarated rendering on exactly that driver.

So ack from you?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/2] drm/gem: drm_gem_dumb_map_offset(): reject dma-buf

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 2975 bytes --]

Daniel Vetter <daniel@ffwll.ch> writes:

> On Fri, Aug 18, 2017 at 01:34:45PM -0700, Eric Anholt wrote:
>> Daniel Vetter <daniel@ffwll.ch> writes:
>> 
>> > On Fri, Aug 18, 2017 at 10:41:21AM -0700, Eric Anholt wrote:
>> >> Noralf Trønnes <noralf@tronnes.org> writes:
>> >> > Den 18.08.2017 09.46, skrev Daniel Vetter:
>> >> >> On Thu, Aug 17, 2017 at 06:21:30PM +0200, Noralf Trønnes wrote:
>> >> >>> Reject mapping an imported dma-buf since is's an invalid use-case.
>> >> >>>
>> >> >>> Cc: Philipp Zabel <p.zabel@pengutronix.de>
>> >> >>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> >> >>> Cc: Sean Paul <seanpaul@chromium.org>
>> >> >>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> >> >>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> >> >> I think acks from someone using mali would be good too. amdgpu already has
>> >> >> such checks, so I think on the desktop side we're ok.
>> >> >>
>> >> >> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> >> >>
>> >> >> But I think this one here definitely needs a few more acks. I could break
>> >> >> uabi if we're unlucky, so let's not rush it.
>> >> >
>> >> > Ok, I've CC'ed the affected parties to increase the odds that they look
>> >> > at this. These are the drivers using drm_gem_dumb_map_offset()
>> >> > (hopefully I got the list right):
>> >> 
>> >> If I understand the affected path right, this would break the PL111+VC4
>> >> combination: PL111 makes (dumb) buffers for scanout, and VC4 imports
>> >> them and uses them for rendering.  A vc4 glReadPixels of the window
>> >> system buffer would map it and fail.
>> >
>> > It only rejects the map call on dumb buffers, and mmap on imported dma-buf
>> > tends to not really work well, or at least break a few abstractions. Are
>> > you sure this works currently?
>> 
>> OK, that's right -- vc4 would be doing its "native" map call (the same
>> code), not dumb map.
>> 
>> Furthermore, I had it backwards (I had written things both ways at
>> different points, iirc).  We have VC4 making the buffers and PL111
>> dma-buf importing them.  I don't see X11 mapping those buffers if glamor
>> is enabled, so this should be OK for vc4.
>> 
>> GBM's dumb mapping looks safe to me.  X11 does some dumb maps, but I
>> don't think any of those would be on imports.
>
> Yeah the idea is only to lock down the dumb mmap and make sure abi abuse
> (which might work on a specific combo of exporters/kms drivers) is caught
> for this generic interface. dumb really should only be used for
> unaccelarated rendering on exactly that driver.
>
> So ack from you?

I'm hesitant, but that's mostly because I don't see the reason we're
trying to lock it down, so it just looks like a chance of breakage from
my perspective.

However, given that some drivers are banning it already, let's make
things consistent until we find we need to relax it globally.

Acked-by: Eric Anholt <eric@anholt.net>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/2] drm/gem: drm_gem_dumb_map_offset(): reject dma-buf

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 731 bytes --]

On Thu, Aug 17, 2017 at 06:21:30PM +0200, Noralf Trønnes wrote:
> Reject mapping an imported dma-buf since is's an invalid use-case.
> 
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>  drivers/gpu/drm/drm_gem.c | 6 ++++++
>  1 file changed, 6 insertions(+)

I'm not sure we've documented exactly why this is an invalid use-case.
Perhaps that's something we should do along with this patch, or maybe
as a follow-up.

Either way, I think this is a good idea, so:

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/2] drm/gem: drm_gem_dumb_map_offset(): reject dma-buf

[Brian Starkey]

Hi,

Thanks for the CC.

On Fri, Aug 18, 2017 at 06:13:14PM +0200, Noralf Tr??nnes wrote:
>(cc affected parties)
>
>
>Den 18.08.2017 09.46, skrev Daniel Vetter:
>>On Thu, Aug 17, 2017 at 06:21:30PM +0200, Noralf Tr??nnes wrote:
>>>Reject mapping an imported dma-buf since is's an invalid use-case.
>>>
>>>Cc: Philipp Zabel <p.zabel@pengutronix.de>
>>>Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>Cc: Sean Paul <seanpaul@chromium.org>
>>>Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>Signed-off-by: Noralf Tr??nnes <noralf@tronnes.org>
>>I think acks from someone using mali would be good too. amdgpu already has
>>such checks, so I think on the desktop side we're ok.
>>

This looks like it would break anyone running the Mali-4xx series GPUs
with the Arm graphics stack (e.g. Hikey board).

I don't know where that sits in terms of policy.

Cheers,
-Brian

>>Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>
>>But I think this one here definitely needs a few more acks. I could break
>>uabi if we're unlucky, so let's not rush it.
>
>Ok, I've CC'ed the affected parties to increase the odds that they look
>at this. These are the drivers using drm_gem_dumb_map_offset()
>(hopefully I got the list right):
>
>arc
>atmel-hlcdc
>cirrus
>exynos
>fsl-dcu
>gma500
>hdlcd
>imx
>kirin
>mali-dp
>mediatek
>meson
>mxsfb
>pl111
>rcar-du
>rockchip
>shmobile
>sti
>stm
>sun4i
>tegra
>tilcd
>vc4
>zte
>
>
>Noralf.
>
>>-Daniel
>>
>>>---
>>>  drivers/gpu/drm/drm_gem.c | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>>diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
>>>index ad4e9cf..8da5801 100644
>>>--- a/drivers/gpu/drm/drm_gem.c
>>>+++ b/drivers/gpu/drm/drm_gem.c
>>>@@ -333,6 +333,12 @@ int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
>>>  	if (!obj)
>>>  		return -ENOENT;
>>>+	/* Don't allow imported objects to be mapped */
>>>+	if (obj->import_attach) {
>>>+		ret = -EINVAL;
>>>+		goto out;
>>>+	}
>>>+
>>>  	ret = drm_gem_create_mmap_offset(obj);
>>>  	if (ret)
>>>  		goto out;
>>>-- 
>>>2.7.4
>>>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/2] drm/gem: drm_gem_dumb_map_offset(): reject dma-buf

[Daniel Vetter]

On Thu, Aug 24, 2017 at 11:04:01AM +0100, Brian Starkey wrote:
> Hi,
> 
> Thanks for the CC.
> 
> On Fri, Aug 18, 2017 at 06:13:14PM +0200, Noralf Tr??nnes wrote:
> > (cc affected parties)
> > 
> > 
> > Den 18.08.2017 09.46, skrev Daniel Vetter:
> > > On Thu, Aug 17, 2017 at 06:21:30PM +0200, Noralf Tr??nnes wrote:
> > > > Reject mapping an imported dma-buf since is's an invalid use-case.
> > > > 
> > > > Cc: Philipp Zabel <p.zabel@pengutronix.de>
> > > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > Cc: Sean Paul <seanpaul@chromium.org>
> > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > Signed-off-by: Noralf Tr??nnes <noralf@tronnes.org>
> > > I think acks from someone using mali would be good too. amdgpu already has
> > > such checks, so I think on the desktop side we're ok.
> > > 
> 
> This looks like it would break anyone running the Mali-4xx series GPUs
> with the Arm graphics stack (e.g. Hikey board).
> 
> I don't know where that sits in terms of policy.

Why would this break this use-case? Is the mali blob somehow using dumb
mmap for it's own buffers it shares with display? That would be rather
backwards ...

Either way, blob -> meh, not a concern really.
-Daniel

> 
> Cheers,
> -Brian
> 
> > > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > 
> > > But I think this one here definitely needs a few more acks. I could break
> > > uabi if we're unlucky, so let's not rush it.
> > 
> > Ok, I've CC'ed the affected parties to increase the odds that they look
> > at this. These are the drivers using drm_gem_dumb_map_offset()
> > (hopefully I got the list right):
> > 
> > arc
> > atmel-hlcdc
> > cirrus
> > exynos
> > fsl-dcu
> > gma500
> > hdlcd
> > imx
> > kirin
> > mali-dp
> > mediatek
> > meson
> > mxsfb
> > pl111
> > rcar-du
> > rockchip
> > shmobile
> > sti
> > stm
> > sun4i
> > tegra
> > tilcd
> > vc4
> > zte
> > 
> > 
> > Noralf.
> > 
> > > -Daniel
> > > 
> > > > ---
> > > >  drivers/gpu/drm/drm_gem.c | 6 ++++++
> > > >  1 file changed, 6 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > > > index ad4e9cf..8da5801 100644
> > > > --- a/drivers/gpu/drm/drm_gem.c
> > > > +++ b/drivers/gpu/drm/drm_gem.c
> > > > @@ -333,6 +333,12 @@ int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
> > > >  	if (!obj)
> > > >  		return -ENOENT;
> > > > +	/* Don't allow imported objects to be mapped */
> > > > +	if (obj->import_attach) {
> > > > +		ret = -EINVAL;
> > > > +		goto out;
> > > > +	}
> > > > +
> > > >  	ret = drm_gem_create_mmap_offset(obj);
> > > >  	if (ret)
> > > >  		goto out;
> > > > -- 
> > > > 2.7.4
> > > > 
> > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/2] drm/gem: drm_gem_dumb_map_offset(): reject dma-buf

[Brian Starkey]

On Fri, Aug 25, 2017 at 03:34:33PM +0200, Daniel Vetter wrote:
>On Thu, Aug 24, 2017 at 11:04:01AM +0100, Brian Starkey wrote:
>> Hi,
>>
>> Thanks for the CC.
>>
>> On Fri, Aug 18, 2017 at 06:13:14PM +0200, Noralf Tr??nnes wrote:
>> > (cc affected parties)
>> >
>> >
>> > Den 18.08.2017 09.46, skrev Daniel Vetter:
>> > > On Thu, Aug 17, 2017 at 06:21:30PM +0200, Noralf Tr??nnes wrote:
>> > > > Reject mapping an imported dma-buf since is's an invalid use-case.
>> > > >
>> > > > Cc: Philipp Zabel <p.zabel@pengutronix.de>
>> > > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> > > > Cc: Sean Paul <seanpaul@chromium.org>
>> > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> > > > Signed-off-by: Noralf Tr??nnes <noralf@tronnes.org>
>> > > I think acks from someone using mali would be good too. amdgpu already has
>> > > such checks, so I think on the desktop side we're ok.
>> > >
>>
>> This looks like it would break anyone running the Mali-4xx series GPUs
>> with the Arm graphics stack (e.g. Hikey board).
>>
>> I don't know where that sits in terms of policy.
>
>Why would this break this use-case? Is the mali blob somehow using dumb
>mmap for it's own buffers it shares with display? That would be rather
>backwards ...

If it's running with a DRM display driver (which may or may not be
Mali-DP, they are separate IPs), then its window system code always
uses dumb mmap if it needs a pointer. That might be a native DRM
buffer that the display driver allocated, or a dma-buf which it PRIME
imported (which would now fail).

-Brian

>
>Either way, blob -> meh, not a concern really.
>-Daniel
>
>>
>> Cheers,
>> -Brian
>>
>> > > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> > >
>> > > But I think this one here definitely needs a few more acks. I could break
>> > > uabi if we're unlucky, so let's not rush it.
>> >
>> > Ok, I've CC'ed the affected parties to increase the odds that they look
>> > at this. These are the drivers using drm_gem_dumb_map_offset()
>> > (hopefully I got the list right):
>> >
>> > arc
>> > atmel-hlcdc
>> > cirrus
>> > exynos
>> > fsl-dcu
>> > gma500
>> > hdlcd
>> > imx
>> > kirin
>> > mali-dp
>> > mediatek
>> > meson
>> > mxsfb
>> > pl111
>> > rcar-du
>> > rockchip
>> > shmobile
>> > sti
>> > stm
>> > sun4i
>> > tegra
>> > tilcd
>> > vc4
>> > zte
>> >
>> >
>> > Noralf.
>> >
>> > > -Daniel
>> > >
>> > > > ---
>> > > >  drivers/gpu/drm/drm_gem.c | 6 ++++++
>> > > >  1 file changed, 6 insertions(+)
>> > > >
>> > > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
>> > > > index ad4e9cf..8da5801 100644
>> > > > --- a/drivers/gpu/drm/drm_gem.c
>> > > > +++ b/drivers/gpu/drm/drm_gem.c
>> > > > @@ -333,6 +333,12 @@ int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
>> > > >  	if (!obj)
>> > > >  		return -ENOENT;
>> > > > +	/* Don't allow imported objects to be mapped */
>> > > > +	if (obj->import_attach) {
>> > > > +		ret = -EINVAL;
>> > > > +		goto out;
>> > > > +	}
>> > > > +
>> > > >  	ret = drm_gem_create_mmap_offset(obj);
>> > > >  	if (ret)
>> > > >  		goto out;
>> > > > --
>> > > > 2.7.4
>> > > >
>> >
>
>-- 
>Daniel Vetter
>Software Engineer, Intel Corporation
>http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/2] drm/gem: drm_gem_dumb_map_offset(): reject dma-buf

[Daniel Vetter]

On Tue, Aug 29, 2017 at 05:03:19PM +0100, Brian Starkey wrote:
> On Fri, Aug 25, 2017 at 03:34:33PM +0200, Daniel Vetter wrote:
> > On Thu, Aug 24, 2017 at 11:04:01AM +0100, Brian Starkey wrote:
> > > Hi,
> > > 
> > > Thanks for the CC.
> > > 
> > > On Fri, Aug 18, 2017 at 06:13:14PM +0200, Noralf Tr??nnes wrote:
> > > > (cc affected parties)
> > > >
> > > >
> > > > Den 18.08.2017 09.46, skrev Daniel Vetter:
> > > > > On Thu, Aug 17, 2017 at 06:21:30PM +0200, Noralf Tr??nnes wrote:
> > > > > > Reject mapping an imported dma-buf since is's an invalid use-case.
> > > > > >
> > > > > > Cc: Philipp Zabel <p.zabel@pengutronix.de>
> > > > > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > > Cc: Sean Paul <seanpaul@chromium.org>
> > > > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > > > Signed-off-by: Noralf Tr??nnes <noralf@tronnes.org>
> > > > > I think acks from someone using mali would be good too. amdgpu already has
> > > > > such checks, so I think on the desktop side we're ok.
> > > > >
> > > 
> > > This looks like it would break anyone running the Mali-4xx series GPUs
> > > with the Arm graphics stack (e.g. Hikey board).
> > > 
> > > I don't know where that sits in terms of policy.
> > 
> > Why would this break this use-case? Is the mali blob somehow using dumb
> > mmap for it's own buffers it shares with display? That would be rather
> > backwards ...
> 
> If it's running with a DRM display driver (which may or may not be
> Mali-DP, they are separate IPs), then its window system code always
> uses dumb mmap if it needs a pointer. That might be a native DRM
> buffer that the display driver allocated, or a dma-buf which it PRIME
> imported (which would now fail).

Yeah, that's pretty much the kind of uabi abuse I want to prevent. Dumb
mmap is for dumb buffers, if you have a egl platform then you need to mmap
through that one anyway. If a egl stack uses dumb mmap, something went
wrong somewhere.
-Daniel

> 
> -Brian
> 
> > 
> > Either way, blob -> meh, not a concern really.
> > -Daniel
> > 
> > > 
> > > Cheers,
> > > -Brian
> > > 
> > > > > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > >
> > > > > But I think this one here definitely needs a few more acks. I could break
> > > > > uabi if we're unlucky, so let's not rush it.
> > > >
> > > > Ok, I've CC'ed the affected parties to increase the odds that they look
> > > > at this. These are the drivers using drm_gem_dumb_map_offset()
> > > > (hopefully I got the list right):
> > > >
> > > > arc
> > > > atmel-hlcdc
> > > > cirrus
> > > > exynos
> > > > fsl-dcu
> > > > gma500
> > > > hdlcd
> > > > imx
> > > > kirin
> > > > mali-dp
> > > > mediatek
> > > > meson
> > > > mxsfb
> > > > pl111
> > > > rcar-du
> > > > rockchip
> > > > shmobile
> > > > sti
> > > > stm
> > > > sun4i
> > > > tegra
> > > > tilcd
> > > > vc4
> > > > zte
> > > >
> > > >
> > > > Noralf.
> > > >
> > > > > -Daniel
> > > > >
> > > > > > ---
> > > > > >  drivers/gpu/drm/drm_gem.c | 6 ++++++
> > > > > >  1 file changed, 6 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > > > > > index ad4e9cf..8da5801 100644
> > > > > > --- a/drivers/gpu/drm/drm_gem.c
> > > > > > +++ b/drivers/gpu/drm/drm_gem.c
> > > > > > @@ -333,6 +333,12 @@ int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
> > > > > >  	if (!obj)
> > > > > >  		return -ENOENT;
> > > > > > +	/* Don't allow imported objects to be mapped */
> > > > > > +	if (obj->import_attach) {
> > > > > > +		ret = -EINVAL;
> > > > > > +		goto out;
> > > > > > +	}
> > > > > > +
> > > > > >  	ret = drm_gem_create_mmap_offset(obj);
> > > > > >  	if (ret)
> > > > > >  		goto out;
> > > > > > --
> > > > > > 2.7.4
> > > > > >
> > > >
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 2/2] drm/armada: Use .dumb_map_offset and .dumb_destroy defaults

[Daniel Vetter]

On Fri, Aug 18, 2017 at 09:47:25AM +0200, Daniel Vetter wrote:
> On Thu, Aug 17, 2017 at 06:21:31PM +0200, Noralf Trønnes wrote:
> > This driver can use the drm_driver.dumb_destroy and
> > drm_driver.dumb_map_offset defaults, so no need to set them.
> > 
> > Cc: Russell King <linux@armlinux.org.uk>
> > Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> 
> Assuming patch 1 gets enough acks and lands:

I think patch 1 has soaked for long enough now, feel free to push.
-Daniel

> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> > ---
> >  drivers/gpu/drm/armada/armada_drv.c |  2 --
> >  drivers/gpu/drm/armada/armada_gem.c | 36 ------------------------------------
> >  drivers/gpu/drm/armada/armada_gem.h |  4 ----
> >  3 files changed, 42 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/armada/armada_drv.c b/drivers/gpu/drm/armada/armada_drv.c
> > index 0b3227c..2fbd9d3 100644
> > --- a/drivers/gpu/drm/armada/armada_drv.c
> > +++ b/drivers/gpu/drm/armada/armada_drv.c
> > @@ -70,8 +70,6 @@ static struct drm_driver armada_drm_driver = {
> >  	.gem_prime_export	= armada_gem_prime_export,
> >  	.gem_prime_import	= armada_gem_prime_import,
> >  	.dumb_create		= armada_gem_dumb_create,
> > -	.dumb_map_offset	= armada_gem_dumb_map_offset,
> > -	.dumb_destroy		= armada_gem_dumb_destroy,
> >  	.gem_vm_ops		= &armada_gem_vm_ops,
> >  	.major			= 1,
> >  	.minor			= 0,
> > diff --git a/drivers/gpu/drm/armada/armada_gem.c b/drivers/gpu/drm/armada/armada_gem.c
> > index a76ca21..7983538 100644
> > --- a/drivers/gpu/drm/armada/armada_gem.c
> > +++ b/drivers/gpu/drm/armada/armada_gem.c
> > @@ -270,42 +270,6 @@ int armada_gem_dumb_create(struct drm_file *file, struct drm_device *dev,
> >  	return ret;
> >  }
> >  
> > -int armada_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
> > -	uint32_t handle, uint64_t *offset)
> > -{
> > -	struct armada_gem_object *obj;
> > -	int ret = 0;
> > -
> > -	obj = armada_gem_object_lookup(file, handle);
> > -	if (!obj) {
> > -		DRM_ERROR("failed to lookup gem object\n");
> > -		return -EINVAL;
> > -	}
> > -
> > -	/* Don't allow imported objects to be mapped */
> > -	if (obj->obj.import_attach) {
> > -		ret = -EINVAL;
> > -		goto err_unref;
> > -	}
> > -
> > -	ret = drm_gem_create_mmap_offset(&obj->obj);
> > -	if (ret == 0) {
> > -		*offset = drm_vma_node_offset_addr(&obj->obj.vma_node);
> > -		DRM_DEBUG_DRIVER("handle %#x offset %llx\n", handle, *offset);
> > -	}
> > -
> > - err_unref:
> > -	drm_gem_object_unreference_unlocked(&obj->obj);
> > -
> > -	return ret;
> > -}
> > -
> > -int armada_gem_dumb_destroy(struct drm_file *file, struct drm_device *dev,
> > -	uint32_t handle)
> > -{
> > -	return drm_gem_handle_delete(file, handle);
> > -}
> > -
> >  /* Private driver gem ioctls */
> >  int armada_gem_create_ioctl(struct drm_device *dev, void *data,
> >  	struct drm_file *file)
> > diff --git a/drivers/gpu/drm/armada/armada_gem.h b/drivers/gpu/drm/armada/armada_gem.h
> > index 6e524e0..1ac9079 100644
> > --- a/drivers/gpu/drm/armada/armada_gem.h
> > +++ b/drivers/gpu/drm/armada/armada_gem.h
> > @@ -35,10 +35,6 @@ struct armada_gem_object *armada_gem_alloc_private_object(struct drm_device *,
> >  	size_t);
> >  int armada_gem_dumb_create(struct drm_file *, struct drm_device *,
> >  	struct drm_mode_create_dumb *);
> > -int armada_gem_dumb_map_offset(struct drm_file *, struct drm_device *,
> > -	uint32_t, uint64_t *);
> > -int armada_gem_dumb_destroy(struct drm_file *, struct drm_device *,
> > -	uint32_t);
> >  struct dma_buf *armada_gem_prime_export(struct drm_device *dev,
> >  	struct drm_gem_object *obj, int flags);
> >  struct drm_gem_object *armada_gem_prime_import(struct drm_device *,
> > -- 
> > 2.7.4
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/2] drm/gem: drm_gem_dumb_map_offset(): reject dma-buf

[Brian Starkey]

On Wed, Aug 30, 2017 at 09:44:53AM +0200, Daniel Vetter wrote:
>On Tue, Aug 29, 2017 at 05:03:19PM +0100, Brian Starkey wrote:
>> On Fri, Aug 25, 2017 at 03:34:33PM +0200, Daniel Vetter wrote:
>> > On Thu, Aug 24, 2017 at 11:04:01AM +0100, Brian Starkey wrote:
>> > > Hi,
>> > >
>> > > Thanks for the CC.
>> > >
>> > > On Fri, Aug 18, 2017 at 06:13:14PM +0200, Noralf Tr??nnes wrote:
>> > > > (cc affected parties)
>> > > >
>> > > >
>> > > > Den 18.08.2017 09.46, skrev Daniel Vetter:
>> > > > > On Thu, Aug 17, 2017 at 06:21:30PM +0200, Noralf Tr??nnes wrote:
>> > > > > > Reject mapping an imported dma-buf since is's an invalid use-case.
>> > > > > >
>> > > > > > Cc: Philipp Zabel <p.zabel@pengutronix.de>
>> > > > > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> > > > > > Cc: Sean Paul <seanpaul@chromium.org>
>> > > > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> > > > > > Signed-off-by: Noralf Tr??nnes <noralf@tronnes.org>
>> > > > > I think acks from someone using mali would be good too. amdgpu already has
>> > > > > such checks, so I think on the desktop side we're ok.
>> > > > >
>> > >
>> > > This looks like it would break anyone running the Mali-4xx series GPUs
>> > > with the Arm graphics stack (e.g. Hikey board).
>> > >
>> > > I don't know where that sits in terms of policy.
>> >
>> > Why would this break this use-case? Is the mali blob somehow using dumb
>> > mmap for it's own buffers it shares with display? That would be rather
>> > backwards ...
>>
>> If it's running with a DRM display driver (which may or may not be
>> Mali-DP, they are separate IPs), then its window system code always
>> uses dumb mmap if it needs a pointer. That might be a native DRM
>> buffer that the display driver allocated, or a dma-buf which it PRIME
>> imported (which would now fail).
>
>Yeah, that's pretty much the kind of uabi abuse I want to prevent. Dumb
>mmap is for dumb buffers, if you have a egl platform then you need to mmap
>through that one anyway. If a egl stack uses dumb mmap, something went
>wrong somewhere.

But it exists. There's applications depending on that behaviour to
continue working, so shouldn't it continue to work?  I thought that
was rule #1

-Brian

>-Daniel
>
>>
>> -Brian
>>
>> >
>> > Either way, blob -> meh, not a concern really.
>> > -Daniel
>> >
>> > >
>> > > Cheers,
>> > > -Brian
>> > >
>> > > > > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> > > > >
>> > > > > But I think this one here definitely needs a few more acks. I could break
>> > > > > uabi if we're unlucky, so let's not rush it.
>> > > >
>> > > > Ok, I've CC'ed the affected parties to increase the odds that they look
>> > > > at this. These are the drivers using drm_gem_dumb_map_offset()
>> > > > (hopefully I got the list right):
>> > > >
>> > > > arc
>> > > > atmel-hlcdc
>> > > > cirrus
>> > > > exynos
>> > > > fsl-dcu
>> > > > gma500
>> > > > hdlcd
>> > > > imx
>> > > > kirin
>> > > > mali-dp
>> > > > mediatek
>> > > > meson
>> > > > mxsfb
>> > > > pl111
>> > > > rcar-du
>> > > > rockchip
>> > > > shmobile
>> > > > sti
>> > > > stm
>> > > > sun4i
>> > > > tegra
>> > > > tilcd
>> > > > vc4
>> > > > zte
>> > > >
>> > > >
>> > > > Noralf.
>> > > >
>> > > > > -Daniel
>> > > > >
>> > > > > > ---
>> > > > > >  drivers/gpu/drm/drm_gem.c | 6 ++++++
>> > > > > >  1 file changed, 6 insertions(+)
>> > > > > >
>> > > > > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
>> > > > > > index ad4e9cf..8da5801 100644
>> > > > > > --- a/drivers/gpu/drm/drm_gem.c
>> > > > > > +++ b/drivers/gpu/drm/drm_gem.c
>> > > > > > @@ -333,6 +333,12 @@ int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
>> > > > > >  	if (!obj)
>> > > > > >  		return -ENOENT;
>> > > > > > +	/* Don't allow imported objects to be mapped */
>> > > > > > +	if (obj->import_attach) {
>> > > > > > +		ret = -EINVAL;
>> > > > > > +		goto out;
>> > > > > > +	}
>> > > > > > +
>> > > > > >  	ret = drm_gem_create_mmap_offset(obj);
>> > > > > >  	if (ret)
>> > > > > >  		goto out;
>> > > > > > --
>> > > > > > 2.7.4
>> > > > > >
>> > > >
>> >
>> > --
>> > Daniel Vetter
>> > Software Engineer, Intel Corporation
>> > http://blog.ffwll.ch
>
>-- 
>Daniel Vetter
>Software Engineer, Intel Corporation
>http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/2] drm/gem: drm_gem_dumb_map_offset(): reject dma-buf

[Noralf Trønnes]

Den 18.08.2017 18.13, skrev Noralf Trønnes:
> (cc affected parties)
>
>
> Den 18.08.2017 09.46, skrev Daniel Vetter:
>> On Thu, Aug 17, 2017 at 06:21:30PM +0200, Noralf Trønnes wrote:
>>> Reject mapping an imported dma-buf since is's an invalid use-case.
>>>
>>> Cc: Philipp Zabel <p.zabel@pengutronix.de>
>>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> Cc: Sean Paul <seanpaul@chromium.org>
>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> I think acks from someone using mali would be good too. amdgpu 
>> already has
>> such checks, so I think on the desktop side we're ok.
>>
>> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>
>> But I think this one here definitely needs a few more acks. I could 
>> break
>> uabi if we're unlucky, so let's not rush it.
>
> Ok, I've CC'ed the affected parties to increase the odds that they look
> at this. These are the drivers using drm_gem_dumb_map_offset()
> (hopefully I got the list right):
>
> arc
> atmel-hlcdc
> cirrus
> exynos
> fsl-dcu
> gma500
> hdlcd
> imx
> kirin
> mali-dp
> mediatek
> meson
> mxsfb
> pl111
> rcar-du
> rockchip
> shmobile
> sti
> stm
> sun4i
> tegra
> tilcd
> vc4
> zte
>

Applied to drm-misc together with the armada patch.

Noralf.

>
> Noralf.
>
>> -Daniel
>>
>>> ---
>>>   drivers/gpu/drm/drm_gem.c | 6 ++++++
>>>   1 file changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
>>> index ad4e9cf..8da5801 100644
>>> --- a/drivers/gpu/drm/drm_gem.c
>>> +++ b/drivers/gpu/drm/drm_gem.c
>>> @@ -333,6 +333,12 @@ int drm_gem_dumb_map_offset(struct drm_file 
>>> *file, struct drm_device *dev,
>>>       if (!obj)
>>>           return -ENOENT;
>>>   +    /* Don't allow imported objects to be mapped */
>>> +    if (obj->import_attach) {
>>> +        ret = -EINVAL;
>>> +        goto out;
>>> +    }
>>> +
>>>       ret = drm_gem_create_mmap_offset(obj);
>>>       if (ret)
>>>           goto out;
>>> -- 
>>> 2.7.4
>>>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel