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.