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

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