[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 at pengutronix.de>
> >>>>>> Cc: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >>>>>> Cc: Sean Paul <seanpaul at chromium.org>
> >>>>>> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> >>>>>> Signed-off-by: Noralf Trønnes <noralf at 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