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(-)
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;
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;
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
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 ?
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
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.
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
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.
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
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 ?
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
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
(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
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.
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 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.
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 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
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
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
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
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
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
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
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
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 *,
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
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
dri-devel@lists.freedesktop.org
-
Brian Starkey -
Chris Wilson -
Daniel Vetter -
Eric Anholt -
Laurent Pinchart -
Noralf Trønnes -
Thierry Reding