Re: [RFC v2 5/8] drm/fence: add in-fences support

[Ville Syrjälä]

On Tue, Apr 26, 2016 at 08:23:46PM +0200, Daniel Vetter wrote:
> On Tue, Apr 26, 2016 at 08:40:45PM +0300, Ville Syrjälä wrote:
> > On Tue, Apr 26, 2016 at 07:20:49PM +0200, Daniel Vetter wrote:
> > > On Tue, Apr 26, 2016 at 07:26:21PM +0300, Ville Syrjälä wrote:
> > > > On Tue, Apr 26, 2016 at 04:36:36PM +0200, Daniel Vetter wrote:
> > > > > On Tue, Apr 26, 2016 at 11:14:22AM -0300, Gustavo Padovan wrote:
> > > > > > 2016-04-26 Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>:
> > > > > > 
> > > > > > > On Mon, Apr 25, 2016 at 07:33:25PM -0300, Gustavo Padovan wrote:
> > > > > > > > From: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxxxx>
> > > > > > > > 
> > > > > > > > There is now a new property called FENCE_FD attached to every plane
> > > > > > > > state that receives the sync_file fd from userspace via the atomic commit
> > > > > > > > IOCTL.
> > > > > > > 
> > > > > > > I still don't like this property abuse. Also with atomic, all passed
> > > > > > > fences must be waited upon before anything is done, so attaching them
> > > > > > > to planes seems like it might just give people the wrong idea.
> > > > > > 
> > > > > > I'm actually fine with this as property, but another solutions is use
> > > > > > an array of {plane, fence_fd} and extend drm_atomic_ioctl args just like
> > > > > > we have done for out fences. However the FENCE_FD property is easier to
> > > > > > handle in userspace than the array. Any other idea?
> > > > > 
> > > > > Imo FENCE_FD is perfectly fine. But what's the concern around giving
> > > > > people the wrong idea with attaching fences to planes? For nonblocking
> > > > > commits we need to store them somewhere for the worker, drm_plane_state
> > > > > seems like an as good place as any other.
> > > > 
> > > > It gives the impression that each plane might flip as soon as its fence
> > > > signals.
> > > 
> > > That wouldn't be atomic. Not sure how someone could come up with that
> > > idea.
> > 
> > What else would it mean? It's attached to a specific plane, so why would
> > it affect other planes?
> > 
> > > I mean we could move FENCE_FD to the crtc (fence fds can be merged),
> > > but that's just a needless difference to what hwc expects. I think
> > > aligning with the only real-world users in this case here makes sense.
> > 
> > Well it doesn't belong on the crtc either. I would just stick in the
> > ioctl as a separate thing, then it's clear it's related to the whole
> > operation rather than any kms object.
> 
> We want it per-crtc I'd say, so that you could flip each crtc
> individually.

Then you could just issue multiple ioctls. For eg. those nasty 4k MST
display (or just otherwise neatly synced displayes) you want to wait for
all the fences upfront and the flip everything at once, otherwise you'll
get nasty tears at the seams.

> But really the reason for per-plane is hw composer from
> Android. I don't see any point in designing an api that's needlessly
> different from what the main user expects (even if it may be silly).

What are they doing that can't stuff the fences into an array
instead of props?

> 
> The other bit is that for implicit syncing you need one fence per fb/plane
> anyway, so this also fits nicely on the driver side I think.
> 
> > > Plus docs in case someone has funny ideas.
> > 
> > Weren't you just quoting rusty's API manifesto recently? ;)
> 
> I quote it all the time.
> 
> http://sweng.the-davies.net/Home/rustys-api-design-manifesto
> 
> I think current interface is scoring pretty high since as part of
> Gustavo's work there's no also a new atomic helper which will get the
> waiting right. There's still the problem that neither for drm_event nor
> the fence do we have anything idiot-proof. So for that the solution is
> testcases (which are also happening).
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel OTC