Re: [PATCH v3 1/2] drm/gem: drm_gem_dumb_map_offset(): reject dma-buf

[Eric Anholt <eric@anholt.net>]

[-- Attachment #1.1: Type: text/plain, Size: 2975 bytes --]

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>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel