[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 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.
> 
> 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