On Tue, Mar 20, 2018 at 11:35:40PM -0400, Rob Clark wrote: > On Tue, Mar 20, 2018 at 11:16 AM, Alexandru-Cosmin Gheorghe > <Alexandru-Cosmin.Gheorghe@xxxxxxx> 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. > > > > fwiw, the msm/mdp5 writeback support I implemented was using a > different CRTC (ie. output going either to display or to wb).. (which > unfortunately implies using different planes).. not sure how much this > case is worth supporting in drm-hwc. Yeah, I wonder whether this is the method of operation we should focus on. It's unclear whether the restrictions Alex describes above are HW or SW limited, my impression was SW. Of course, this means we'll need to disable WB if we are using all crtcs (ie: external display connected), but that seems like a rare usecase of Android, and one might argue that power savings are not as important in this case. Further, if we enable compositing with a dedicated WB pipe, we can also use it for virtual displays. Sean > > (I think I can do the single CRTC and multiple encoder cases, but it > wasn't really obvious how to get that working with a video mode > display and the hw I was working on didn't have a DSI cmd mode panel) > > BR, > -R > > > 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. > > > > > >> > > >> > 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? > > > >> > >> > > >> > 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 > > _______________________________________________ > > dri-devel mailing list > > dri-devel@xxxxxxxxxxxxxxxxxxxxx > > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Sean Paul, Software Engineer, Google / Chromium OS _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
