[PATCH 1/2] drm: Add DRM_CAP_PRIME_SCANOUT.

Mon Apr 17 05:05:01 UTC 2017

On 10 April 2017 6:51:21 pm AEST, "Michel Dänzer" <michel at daenzer.net> wrote:
>On 06/04/17 04:47 PM, Christopher James Halse Rogers wrote:
>> On Wed, 5 Apr 2017 at 20:14 Lucas Stach <l.stach at pengutronix.de
>> <mailto:l.stach at pengutronix.de>> wrote:
>> 
>>     Am Mittwoch, den 05.04.2017, 11:59 +0200 schrieb Daniel Vetter:
>>     > On Wed, Apr 05, 2017 at 10:15:44AM +0200, Lucas Stach wrote:
>>     > > Am Mittwoch, den 05.04.2017, 00:20 +0000 schrieb Christopher
>>     James Halse
>>     > > Rogers:
>>     > > >
>>     > > >
>>     > > > On Tue, Apr 4, 2017 at 9:53 PM Daniel Vetter
><daniel at ffwll.ch
>>     <mailto:daniel at ffwll.ch>> wrote:
>>     > > >
>>     > > >         On Tue, Apr 4, 2017 at 12:43 PM, Lucas Stach
>>     > > >         <l.stach at pengutronix.de
>>     <mailto:l.stach at pengutronix.de>> wrote:
>>     > > >         >> If I could guarantee that I'd only ever run on
>>     > > >         4.13-or-later kernels
>>     > > >         >> (I think that's when the previous patches will
>>     land?), then
>>     > > >         this would
>>     > > >         >> indeed be mostly unnecessary. It would save me a
>>     bunch of
>>     > > >         addfb calls
>>     > > >         >> that would predictably fail, but they're cheap.
>>     > > >         >
>>     > > >         > I don't think we ever had caps for "things are
>>     working now,
>>     > > >         as they are
>>     > > >         > supposed to". i915 wasn't properly synchronizing
>on
>>     foreign
>>     > > >         fences for a
>>     > > >         > long time, yet we didn't gain a cap for "cross
>>     device sync
>>     > > >         works now".
>>     > > >         >
>>     > > >         > If your distro use-case relies on those things
>>     working it's
>>     > > >         probably
>>     > > >         > best to just backport the relevant fixes.
>>     > > >
>>     > > >         The even better solution for this is to push the
>backports
>>     > > >         through
>>     > > >         upstream -stable kernels. This stuff here is simple
>enough
>>     > > >         that we can
>>     > > >         do it. Same could have been done for the fairly
>minimal
>>     > > >         fencing fixes
>>     > > >         for i915 (at least some of them, the ones in the
>>     page-flip).
>>     > > >
>>     > > >         Otherwise we'll end up with tons IM_NOT_BUGGY and
>>     > > >         IM_SLIGHTLY_LESS_BUGGY and
>>     > > >         IM_NOT_BUGGY_EXCEPT_THIS_BOTCHED_BACKPORT
>>     > > >         flags where no one at all knows what they mean,
>usage
>>     between
>>     > > >         different drivers and different userspace is
>entirely
>>     > > >         inconsistent and
>>     > > >         they just all add to the confusion. They're just
>bugs,
>>     lets
>>     > > >         treat them
>>     > > >         like that.
>>     > > >
>>     > > >
>>     > > > It's not *quite* DRM_CAP_PRIME_SCANOUT_NOT_BUGGY - while
>the
>>     relevant
>>     > > > hardware allegedly supports it, nouveau/radeon/amdgpu don't
>do
>>     scanout
>>     > > > of GTT, so the lack of this cap indicates that there's no
>point in
>>     > > > trying to call addfb2.
>>     > > >
>>     > > >
>>     > > > But calling addfb2 and it failing is not expensive, so this
>is
>>     rather
>>     > > > niche.
>>     > > >
>>     > > >
>>     > > > In practice I can just restrict attempting to scanout of
>imported
>>     > > > buffers to i915, as that's the only driver that it'll work
>on.
>>     By the
>>     > > > time nouveau/radeon/amdgpu get patches to scanout of GTT
>the fixes
>>     > > > should be old enough that I don't need to care about
>unfixed
>>     kernels.
>>     > > >
>>     > > So given that most discreet hardware won't ever be able to
>>     scanout from
>>     > > GTT (latency and iso requirements will be hard to meet),
>can't
>>     we just
>>     > > fix the case of the broken prime sharing when migrating to
>VRAM?
>>     > >
>>     > > I'm thinking about attaching an exclusive fence to the
>dma-buf
>>     when the
>>     > > migration to VRAM happens, then when the GPU is done with the
>>     buffer we
>>     > > can either write back any changes to GTT, or just drop the
>fence
>>     in case
>>     > > the GPU didn't modify the buffer.
>>     >
>>     > We could, but someone needs to type the code for it. There's
>also the
>>     > problem that you need to migrate back, and then doing all that
>behind
>>     > userspaces back might not be the best idea.
>> 
>>     Drivers with separate VRAM and GTT are already doing a lot of
>migration
>>     behind the userspaces back. The only issue with dma-buf migration
>to
>>     VRAM is that you probably don't want to migrate the pages, but
>duplicate
>>     them in VRAM, doubling memory consumption with possible OOM. But
>then
>>     you could alloc the memory on addfb where you are able to return
>proper
>>     errors.
>> 
>> 
>> Hm. So, on a first inspection, this looks like something I could
>> actually cook up.
>> 
>> Looking at amdgpu it seems like the thing to do would be to associate
>a
>> shadow-bo in VRAM for the imported dma-buf in the addfb call, then
>> pin-and-copy-to the shadow bo in the places where the bo is currently
>> pinned.
>> 
>> Is this approach likely to be acceptable?
>
>It would break e.g. with DRI2 flips, because they replace the screen
>pixmap buffer with the buffer we're flipping to. If the app stops
>flipping while such a shadow BO is being scanned out, later draws to
>the
>screen pixmap won't become visible.

This shadow BO would only ever be used for imported dma-bufs. This would change the behaviour from “addfb fails” to “you get a shadow BO”. (And, pre-patch, from “addfb succeeds but you never see any new rendering”).

I don't think any DRI2 implementation hits this, because of it did it would already be broken.

(On paternity leave, so I'll be intermittent in following this up)