[PATCH hwc v1] [RFC] drm_hwcomposer: Flatten composition using writeback connector

[Sean Paul]

On Tue, Mar 20, 2018 at 03:16:08PM +0000, Alexandru-Cosmin Gheorghe wrote:
> On Tue, Mar 20, 2018 at 10:00:17AM -0400, Sean Paul wrote:
> > On Tue, Mar 13, 2018 at 04:21:20PM +0000, Alexandru Gheorghe wrote:
> > > This patchset tries to add support for using writeback connector to
> > > flatten a scene when it doesn't change for a while. This idea had
> > > been floated around on IRC here [1] and here [2].
> > > 
> > > Developed on top of the latest writeback series, sent by Liviu here
> > > [3].
> > > 
> > > Probably the patch should/could be broken in more patches, but since I
> > > want to put this out there to get feedback, I kept them as a single
> > > patch for now.
> > > 
> > > This change could be summarize as follow:
> > > - Attach a writeback connector to the CRTC that's controlling a
> > > display.
> > > - Detect the scene did not change for a while(60 vblanks).
> > > - Re-commit scene and get the composited scene through the writeback
> > > connector.
> > > - Commit the whole scene as a single plane.
> > > 
> > > Some areas that I consider important and I could use some
> > > feedback/ideas:
> > > 
> > > 1. Building the pipeline.
> > > Currently, drm_hwcomposer allows to connect just a single connector
> > > to a crtc. For now, I decided to treat the writeback connector as a
> > > separate field inside DrmCrtc. I'm not sure if it's a good idea to try
> > > to handle this in a unified way, since the writeback connector is such
> > > a special connector. Regarding the allocation of writeback connectors,
> > > my idea was to allocate writeback connector to the primary display
> > > first and then continue allocating while respecting the display number. 0
> > > gets a writeback before 1 and so on.
> > > 
> > > 2. Heuristic for triggering the flattening.
> > > I just created a VSyncWorker which will trigger the flattening of the
> > > scene if it doesn't change for 60 consecutive vsyncs. The countdown
> > > gets reset every time ValidateDisplay is called. This is a relatively
> > > basic heuristic, so I'm open to suggestions.
> > > 
> > > 3. Locking scheme corner cases.
> > > The Vsynworker is a separate thread which will contend with
> > > SurfaceFlinger for showing things on the screen. I tried to limit the
> > > race window by resetting the countdown on ValidateDisplay and
> > > explicitely checking that we still need to use the flatten scene before
> > > commiting to get the writeback result or before applying the flattened
> > > scene.
> > 
> > What are the consequences of the race? It seems like it might be possible that
> > stale content will be shown over fresh? 
> > I think it'd be fine to serialize
> > vsyncworker and "normal" commits such that the race window is closed instead of
> > reduced. I think you could do the writeback operation asynchronously and then
> > commit the result once it's ready, that shouldn't block things up too much.
> > 
> 
> Just, to make sure we are on the same page, for Mali DP650 the pipeline
> looks like this, I didn't see how it looks for the other hardware.
> 
> CRTC ---- encoder ------------ display connector
>  |------- writeback enc ------ writeback connector.
> 
> There is no asynchronously writeback operation, the scene that you
> do writeback for will be shown on the display as well.

Ah, hmm, that's not how i was envisioning things working. I suppose that makes
sense since if you drop the display connector from the wb commit it'll be
disconnected from the crtc.

> 
> Flattening thread:                             
> 1) Commit original scene + writeback buffer
> 2) Wait for writeback fence.
> 3) Commit flattened scene.
> 
> Before 1) and 3) I'm doing a locked check where I verify if flattened
> scene is still needed and then I release the lock and commit.
> 
> Now, I can see a crazy race where immediately after I decided that we
> need the flattened scene the flattening thread doesn't get any CPU and
> the SurfaceFlinger comes ahead and commits it's new scene followed by
> a commit of the old scene.
> That's the limitted race window I'm talking about.
> 
> The alternative would be to serialize the commits 1) & 3) with
> SurfaceFlinger commits, but while 1) or 3) happens surfaceflinger
> cannot commit, therefore could potentially miss a VBlank. I suppose
> this option is more desireable than the side effect of previously
> explained race.
> 

So I think you can probably let the kernel serialize things. Take the out fence
from (1) and feed it right back into (3), remove (2) from userspace. Then you
can wrap that function in a lock, and acquire the same lock when doing a "normal"
commit.

> 
> > > 
> > > 4. Building the DrmDisplayComposition for the flattened scene.
> > > I kind of lost myself  in all types of layers/planes and compositions,
> > > so I'm not sure if I'm correctly building the DrmDisplayComposition
> > > object for the FlattenScene, it works and shows what I expect on the
> > > screen. So, any feedback here is appreciated.
> > 
> > Yeah, this code needs some love. I had envisioned some Planner-esque interface
> > where platforms could plug their 2D blocks in and they'd be used by core to
> > flatten things. This scheme would at least separate the squashing complexity
> > from compositing. Any interest in taking this on?
> > 
> 
> I could imagine how that would work with a totally independent 2D
> block, not sure if it's doable in my current setup with the writeback
> linked to same CRTC as the display, don't you think?
> 

Yeah, agreed. It doesn't seem like writeback can be used for squashing layers in
the normal commit path.

Sean

> > 
> > > 
> > > 5. I see there is a drmcompositorworker.cpp which implemented the same
> > > idea using the GPU, however that seems to not be used anymore, does
> > > anyone know the rationale behind it?
> > 
> > Let's delete it if it's not used.
> > 
> > Sean
> > 
> > <snip />
> > -- 
> > Sean Paul, Software Engineer, Google / Chromium OS
> 
> -- 
> Cheers,
> Alex G

-- 
Sean Paul, Software Engineer, Google / Chromium OS