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

[Eric Anholt]

Rob Clark  writes:

> On Tue, Mar 20, 2018 at 11:16 AM, 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 +, 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.

This is also how VC4's writeback would work.


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Liviu Dudau]

On Wed, Mar 21, 2018 at 10:01:07AM -0400, Sean Paul wrote:
> 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
> >  wrote:
> > > On Tue, Mar 20, 2018 at 10:00:17AM -0400, Sean Paul wrote:
> > >> On Tue, Mar 13, 2018 at 04:21:20PM +, 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.

When I've inquired on #dri-devel about possible usecase that makes use
of the writeback functionality in the kernel, the flattening of layers
was the one suggested by Sean and this is what Alex has implemented
here. While we could re-focus on virtual displays I feel like this
should be better tackled in a different patchset.

Rob, how is any userspace supposed to drive your implementation? The
framework patches pose no restriction in terms of how the connector gets
attached to a CRTC. Are you saying that for msm/mdp5 we need to have an
atomic commit where only the writeback connector is attached to one of
the 

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

[Alexandru-Cosmin Gheorghe]

Hi Sean & Rob,

On Wed, Mar 21, 2018 at 10:01:07AM -0400, Sean Paul wrote:
> 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
> >  wrote:
> > > On Tue, Mar 20, 2018 at 10:00:17AM -0400, Sean Paul wrote:
> > >> On Tue, Mar 13, 2018 at 04:21:20PM +, 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.

Which case? With the same CRTC or different CRTCs, I think both could
prove useful.

> 
> 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

For Mali DP-650 this is a HW requirement, we don't have any way of
individually controlling just the writeback functionality without
affecting what's sent to the display.

> 
> > 
> > (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)
> > 

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

[Sean Paul]

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
>  wrote:
> > On Tue, Mar 20, 2018 at 10:00:17AM -0400, Sean Paul wrote:
> >> On Tue, Mar 13, 2018 at 04:21:20PM +, 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 

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

[Rob Clark]

On Tue, Mar 20, 2018 at 11:16 AM, 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 +, 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.

(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 

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

[Sean Paul]

On Tue, Mar 20, 2018 at 03:16:08PM +, 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 +, 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.

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

[Alexandru-Cosmin Gheorghe]

On Tue, Mar 20, 2018 at 10:00:17AM -0400, Sean Paul wrote:
> On Tue, Mar 13, 2018 at 04:21:20PM +, 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.

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 

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

[Sean Paul]

On Tue, Mar 13, 2018 at 04:21:20PM +, 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.

> 
> 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?


> 
> 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


-- 
Sean Paul, Software Engineer, Google / Chromium OS
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Alexandru-Cosmin Gheorghe]

On Tue, Mar 20, 2018 at 09:43:53AM -0400, Sean Paul wrote:
> On Tue, Mar 20, 2018 at 01:35:53PM +, Alexandru-Cosmin Gheorghe wrote:
> > Hi,
> > 
> > On Tue, Mar 20, 2018 at 09:28:50AM -0400, Sean Paul wrote:
> > > On Tue, Mar 20, 2018 at 11:26:39AM +, Alexandru-Cosmin Gheorghe wrote:
> > > > Hi Sean,
> > > > 
> > > > Thank you for the feedback.
> > > > I will comeback with v2, later on.
> > > > 
> > > > On Mon, Mar 19, 2018 at 02:03:54PM -0400, Sean Paul wrote:
> > > > > On Tue, Mar 13, 2018 at 04:21:20PM +, 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.
> > > > > 
> > > > > Thank you for your patch, it's looking good. Feel free to submit the 
> > > > > v2 in
> > > > > multiple patches. Also note that we've migrated to gitlab, the new 
> > > > > tree is
> > > > > located here:
> > > > > 
> > > > > https://gitlab.freedesktop.org/drm-hwcomposer/drm-hwcomposer
> > > > 
> > > > This seems to be behind
> > > > https://anongit.freedesktop.org/git/drm_hwcomposer.git.
> > > > Is this location deprecated or does it serve other purposes.
> > > > 
> > > 
> > > Ah, thanks for catching this. It's a migration glitch, I've made it whole
> > > again.
> > > 
> > > 
> > > 
> > > > > >  #include "drmdisplaycompositor.h"
> > > > > > 
> > > > > >  #include 
> > > > > > @@ -36,9 +35,24 @@
> > > > > >  #include "drmplane.h"
> > > > > >  #include "drmresources.h"
> > > > > >  #include "glworker.h"
> > > > > > +static const uint32_t kWaitWritebackFence = 100; //ms
> > > > > > 
> > > > > >  namespace android {
> > > > > > 
> > > > > > +class CompositorVsyncCallback : public VsyncCallback {
> > > > > > + public:
> > > > > > +  CompositorVsyncCallback(DrmDisplayCompositor *compositor)
> > > > > > +  :compositor_(compositor) {
> > > > > > +  }
> > > > > > +
> > > > > > +  void Callback(int display, int64_t timestamp) {
> > > > > > +compositor_->Vsync(display, timestamp);
> > > > > > +  }
> > > > > > +
> > > > > > + private:
> > > > > > +  DrmDisplayCompositor *compositor_;
> > > > > > +};
> > > > > > +
> > > > > 
> > > > > The GLCompositor already has flattening, so we should tie into that. 
> > > > > I assume
> > > > > writeback is going to be more efficient than using GPU (given that 
> > > > > the GPU is
> > > > > probably turned off in these scenarios), so you're probably safe to 
> > > > > use
> > > > > writeback when available and fall back to GL if it's not.
> > > > > 
> > > > > 
> > > > 
> > > > As far as I understood, this was done by the drmcompositorworker.cpp
> > > > by calling SquashAll but that's removed from build now. GLCompositor
> > > > seems to do compositing only when there are not enough overlays or 
> > > > atomic check fails.
> > > > 
> > > > Am I missing something ? 
> > > > Is it still doing flattening when then scene does not change ?
> > > 
> > > https://gitlab.freedesktop.org/drm-hwcomposer/drm-hwcomposer/blob/master/drmcompositorworker.cpp#L62
> > > 
> > > If the scene doesn't change, everything is squashed.
> > 
> > Yeah, but that file it's not even built, see [1].
> > So, I kind of assumed someone had a reason of deleting it. 
> > Any idea about that?
> 
> Ha, WTF! Yeah, guess that's a regression. 
> 
> /me wonders whether we should just rip out the GLCompositor since no one seems
> to care about it.

I thought about removing/disabling it as well, but it turnout to be
not as easy as I expected, since on validateDisplay we don't do
anything and we do the planning and atomic check on presentDisplay and
then we fallback on GLCompositor if needed.

> 
> I'll circle back on the review and take a look at how you're doing the
> inactivity stuff.

Thanks!.

> 
> Sean
> 
> > 
> > [1] 
> > https://gitlab.freedesktop.org/drm-hwcomposer/drm-hwcomposer/blob/master/Android.mk#L53
> > 
> > > 
> > > 
> > > 
> > > > > > diff --git a/drmresources.cpp b/drmresources.cpp
> > > > > > index 32dd376..880cef2 100644
> > > > > > --- a/drmresources.cpp
> > > > > > +++ b/drmresources.cpp
> > > > > > @@ -33,6 +33,8 @@
> > > > > >  #include 
> > > > > >  #include 
> > > > > > 
> > > > > > +#define DRM_CLIENT_CAP_WRITEBACK_CONNECTORS4
> > > > > > +
> > > > > 
> > > > > Presumably this should come from libdrm headers?
> > > > 
> > > > Yes it will, this was just a quick fix to be able to compile against
> > > > older libdrm.
> > > > 
> > > > Btw, this would make drm_hwcomposer compilable only with newer libdrm
> > > > version, but I suppose that's acceptable.
> > > 
> > > You can use preprocessor 

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

[Sean Paul]

On Tue, Mar 20, 2018 at 01:35:53PM +, Alexandru-Cosmin Gheorghe wrote:
> Hi,
> 
> On Tue, Mar 20, 2018 at 09:28:50AM -0400, Sean Paul wrote:
> > On Tue, Mar 20, 2018 at 11:26:39AM +, Alexandru-Cosmin Gheorghe wrote:
> > > Hi Sean,
> > > 
> > > Thank you for the feedback.
> > > I will comeback with v2, later on.
> > > 
> > > On Mon, Mar 19, 2018 at 02:03:54PM -0400, Sean Paul wrote:
> > > > On Tue, Mar 13, 2018 at 04:21:20PM +, 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.
> > > > 
> > > > Thank you for your patch, it's looking good. Feel free to submit the v2 
> > > > in
> > > > multiple patches. Also note that we've migrated to gitlab, the new tree 
> > > > is
> > > > located here:
> > > > 
> > > > https://gitlab.freedesktop.org/drm-hwcomposer/drm-hwcomposer
> > > 
> > > This seems to be behind
> > > https://anongit.freedesktop.org/git/drm_hwcomposer.git.
> > > Is this location deprecated or does it serve other purposes.
> > > 
> > 
> > Ah, thanks for catching this. It's a migration glitch, I've made it whole
> > again.
> > 
> > 
> > 
> > > > >  #include "drmdisplaycompositor.h"
> > > > > 
> > > > >  #include 
> > > > > @@ -36,9 +35,24 @@
> > > > >  #include "drmplane.h"
> > > > >  #include "drmresources.h"
> > > > >  #include "glworker.h"
> > > > > +static const uint32_t kWaitWritebackFence = 100; //ms
> > > > > 
> > > > >  namespace android {
> > > > > 
> > > > > +class CompositorVsyncCallback : public VsyncCallback {
> > > > > + public:
> > > > > +  CompositorVsyncCallback(DrmDisplayCompositor *compositor)
> > > > > +  :compositor_(compositor) {
> > > > > +  }
> > > > > +
> > > > > +  void Callback(int display, int64_t timestamp) {
> > > > > +compositor_->Vsync(display, timestamp);
> > > > > +  }
> > > > > +
> > > > > + private:
> > > > > +  DrmDisplayCompositor *compositor_;
> > > > > +};
> > > > > +
> > > > 
> > > > The GLCompositor already has flattening, so we should tie into that. I 
> > > > assume
> > > > writeback is going to be more efficient than using GPU (given that the 
> > > > GPU is
> > > > probably turned off in these scenarios), so you're probably safe to use
> > > > writeback when available and fall back to GL if it's not.
> > > > 
> > > > 
> > > 
> > > As far as I understood, this was done by the drmcompositorworker.cpp
> > > by calling SquashAll but that's removed from build now. GLCompositor
> > > seems to do compositing only when there are not enough overlays or 
> > > atomic check fails.
> > > 
> > > Am I missing something ? 
> > > Is it still doing flattening when then scene does not change ?
> > 
> > https://gitlab.freedesktop.org/drm-hwcomposer/drm-hwcomposer/blob/master/drmcompositorworker.cpp#L62
> > 
> > If the scene doesn't change, everything is squashed.
> 
> Yeah, but that file it's not even built, see [1].
> So, I kind of assumed someone had a reason of deleting it. 
> Any idea about that?

Ha, WTF! Yeah, guess that's a regression. 

/me wonders whether we should just rip out the GLCompositor since no one seems
to care about it.

I'll circle back on the review and take a look at how you're doing the
inactivity stuff.

Sean

> 
> [1] 
> https://gitlab.freedesktop.org/drm-hwcomposer/drm-hwcomposer/blob/master/Android.mk#L53
> 
> > 
> > 
> > 
> > > > > diff --git a/drmresources.cpp b/drmresources.cpp
> > > > > index 32dd376..880cef2 100644
> > > > > --- a/drmresources.cpp
> > > > > +++ b/drmresources.cpp
> > > > > @@ -33,6 +33,8 @@
> > > > >  #include 
> > > > >  #include 
> > > > > 
> > > > > +#define DRM_CLIENT_CAP_WRITEBACK_CONNECTORS4
> > > > > +
> > > > 
> > > > Presumably this should come from libdrm headers?
> > > 
> > > Yes it will, this was just a quick fix to be able to compile against
> > > older libdrm.
> > > 
> > > Btw, this would make drm_hwcomposer compilable only with newer libdrm
> > > version, but I suppose that's acceptable.
> > 
> > You can use preprocessor checks to determine if
> > DRM_CLIENT_CAP_WRITEBACK_CONNECTORS is defined, and disable the 
> > functionality if
> > it's not.
> > 
> > > 
> > > > 
> > > > >  namespace android {
> > > > > 
> > > > >  DrmResources::DrmResources() : event_listener_(this) {
> > > > > @@ -65,6 +67,11 @@ int DrmResources::Init() {
> > > > >  return ret;
> > > > >}
> > > > > 
> > > > > +  ret = drmSetClientCap(fd(), DRM_CLIENT_CAP_WRITEBACK_CONNECTORS, 
> > > > > 1);
> > > > > +  if (ret) {
> > > > > +ALOGI("Failed to set writeback cap %d", ret);
> > > > > +ret = 0;
> > > > > +  }
> > > > >

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

[Alexandru-Cosmin Gheorghe]

Hi,

On Tue, Mar 20, 2018 at 09:28:50AM -0400, Sean Paul wrote:
> On Tue, Mar 20, 2018 at 11:26:39AM +, Alexandru-Cosmin Gheorghe wrote:
> > Hi Sean,
> > 
> > Thank you for the feedback.
> > I will comeback with v2, later on.
> > 
> > On Mon, Mar 19, 2018 at 02:03:54PM -0400, Sean Paul wrote:
> > > On Tue, Mar 13, 2018 at 04:21:20PM +, 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.
> > > 
> > > Thank you for your patch, it's looking good. Feel free to submit the v2 in
> > > multiple patches. Also note that we've migrated to gitlab, the new tree is
> > > located here:
> > > 
> > > https://gitlab.freedesktop.org/drm-hwcomposer/drm-hwcomposer
> > 
> > This seems to be behind
> > https://anongit.freedesktop.org/git/drm_hwcomposer.git.
> > Is this location deprecated or does it serve other purposes.
> > 
> 
> Ah, thanks for catching this. It's a migration glitch, I've made it whole
> again.
> 
> 
> 
> > > >  #include "drmdisplaycompositor.h"
> > > > 
> > > >  #include 
> > > > @@ -36,9 +35,24 @@
> > > >  #include "drmplane.h"
> > > >  #include "drmresources.h"
> > > >  #include "glworker.h"
> > > > +static const uint32_t kWaitWritebackFence = 100; //ms
> > > > 
> > > >  namespace android {
> > > > 
> > > > +class CompositorVsyncCallback : public VsyncCallback {
> > > > + public:
> > > > +  CompositorVsyncCallback(DrmDisplayCompositor *compositor)
> > > > +  :compositor_(compositor) {
> > > > +  }
> > > > +
> > > > +  void Callback(int display, int64_t timestamp) {
> > > > +compositor_->Vsync(display, timestamp);
> > > > +  }
> > > > +
> > > > + private:
> > > > +  DrmDisplayCompositor *compositor_;
> > > > +};
> > > > +
> > > 
> > > The GLCompositor already has flattening, so we should tie into that. I 
> > > assume
> > > writeback is going to be more efficient than using GPU (given that the 
> > > GPU is
> > > probably turned off in these scenarios), so you're probably safe to use
> > > writeback when available and fall back to GL if it's not.
> > > 
> > > 
> > 
> > As far as I understood, this was done by the drmcompositorworker.cpp
> > by calling SquashAll but that's removed from build now. GLCompositor
> > seems to do compositing only when there are not enough overlays or 
> > atomic check fails.
> > 
> > Am I missing something ? 
> > Is it still doing flattening when then scene does not change ?
> 
> https://gitlab.freedesktop.org/drm-hwcomposer/drm-hwcomposer/blob/master/drmcompositorworker.cpp#L62
> 
> If the scene doesn't change, everything is squashed.

Yeah, but that file it's not even built, see [1].
So, I kind of assumed someone had a reason of deleting it. 
Any idea about that?

[1] 
https://gitlab.freedesktop.org/drm-hwcomposer/drm-hwcomposer/blob/master/Android.mk#L53

> 
> 
> 
> > > > diff --git a/drmresources.cpp b/drmresources.cpp
> > > > index 32dd376..880cef2 100644
> > > > --- a/drmresources.cpp
> > > > +++ b/drmresources.cpp
> > > > @@ -33,6 +33,8 @@
> > > >  #include 
> > > >  #include 
> > > > 
> > > > +#define DRM_CLIENT_CAP_WRITEBACK_CONNECTORS4
> > > > +
> > > 
> > > Presumably this should come from libdrm headers?
> > 
> > Yes it will, this was just a quick fix to be able to compile against
> > older libdrm.
> > 
> > Btw, this would make drm_hwcomposer compilable only with newer libdrm
> > version, but I suppose that's acceptable.
> 
> You can use preprocessor checks to determine if
> DRM_CLIENT_CAP_WRITEBACK_CONNECTORS is defined, and disable the functionality 
> if
> it's not.
> 
> > 
> > > 
> > > >  namespace android {
> > > > 
> > > >  DrmResources::DrmResources() : event_listener_(this) {
> > > > @@ -65,6 +67,11 @@ int DrmResources::Init() {
> > > >  return ret;
> > > >}
> > > > 
> > > > +  ret = drmSetClientCap(fd(), DRM_CLIENT_CAP_WRITEBACK_CONNECTORS, 1);
> > > > +  if (ret) {
> > > > +ALOGI("Failed to set writeback cap %d", ret);
> > > > +ret = 0;
> > > > +  }
> > > >drmModeResPtr res = drmModeGetResources(fd());
> > > >if (!res) {
> > > >  ALOGE("Failed to get DrmResources resources");
> > > > @@ -77,6 +84,7 @@ int DrmResources::Init() {
> > > >std::pair(res->max_width, res->max_height);
> > > > 
> > > >bool found_primary = false;
> > > > +  int primary_index = 0;
> > > >int display_num = 1;
> > > > 
> > > >for (int i = 0; !ret && i < res->count_crtcs; ++i) {
> > > > @@ -162,18 +170,22 @@ int DrmResources::Init() {
> > > >  if (conn->internal() && !found_primary) {
> > > >conn->set_display(0);
> > > >

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

[Sean Paul]

On Tue, Mar 20, 2018 at 11:26:39AM +, Alexandru-Cosmin Gheorghe wrote:
> Hi Sean,
> 
> Thank you for the feedback.
> I will comeback with v2, later on.
> 
> On Mon, Mar 19, 2018 at 02:03:54PM -0400, Sean Paul wrote:
> > On Tue, Mar 13, 2018 at 04:21:20PM +, 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.
> > 
> > Thank you for your patch, it's looking good. Feel free to submit the v2 in
> > multiple patches. Also note that we've migrated to gitlab, the new tree is
> > located here:
> > 
> > https://gitlab.freedesktop.org/drm-hwcomposer/drm-hwcomposer
> 
> This seems to be behind
> https://anongit.freedesktop.org/git/drm_hwcomposer.git.
> Is this location deprecated or does it serve other purposes.
> 

Ah, thanks for catching this. It's a migration glitch, I've made it whole
again.



> > >  #include "drmdisplaycompositor.h"
> > > 
> > >  #include 
> > > @@ -36,9 +35,24 @@
> > >  #include "drmplane.h"
> > >  #include "drmresources.h"
> > >  #include "glworker.h"
> > > +static const uint32_t kWaitWritebackFence = 100; //ms
> > > 
> > >  namespace android {
> > > 
> > > +class CompositorVsyncCallback : public VsyncCallback {
> > > + public:
> > > +  CompositorVsyncCallback(DrmDisplayCompositor *compositor)
> > > +  :compositor_(compositor) {
> > > +  }
> > > +
> > > +  void Callback(int display, int64_t timestamp) {
> > > +compositor_->Vsync(display, timestamp);
> > > +  }
> > > +
> > > + private:
> > > +  DrmDisplayCompositor *compositor_;
> > > +};
> > > +
> > 
> > The GLCompositor already has flattening, so we should tie into that. I 
> > assume
> > writeback is going to be more efficient than using GPU (given that the GPU 
> > is
> > probably turned off in these scenarios), so you're probably safe to use
> > writeback when available and fall back to GL if it's not.
> > 
> > 
> 
> As far as I understood, this was done by the drmcompositorworker.cpp
> by calling SquashAll but that's removed from build now. GLCompositor
> seems to do compositing only when there are not enough overlays or 
> atomic check fails.
> 
> Am I missing something ? 
> Is it still doing flattening when then scene does not change ?

https://gitlab.freedesktop.org/drm-hwcomposer/drm-hwcomposer/blob/master/drmcompositorworker.cpp#L62

If the scene doesn't change, everything is squashed.



> > > diff --git a/drmresources.cpp b/drmresources.cpp
> > > index 32dd376..880cef2 100644
> > > --- a/drmresources.cpp
> > > +++ b/drmresources.cpp
> > > @@ -33,6 +33,8 @@
> > >  #include 
> > >  #include 
> > > 
> > > +#define DRM_CLIENT_CAP_WRITEBACK_CONNECTORS4
> > > +
> > 
> > Presumably this should come from libdrm headers?
> 
> Yes it will, this was just a quick fix to be able to compile against
> older libdrm.
> 
> Btw, this would make drm_hwcomposer compilable only with newer libdrm
> version, but I suppose that's acceptable.

You can use preprocessor checks to determine if
DRM_CLIENT_CAP_WRITEBACK_CONNECTORS is defined, and disable the functionality if
it's not.

> 
> > 
> > >  namespace android {
> > > 
> > >  DrmResources::DrmResources() : event_listener_(this) {
> > > @@ -65,6 +67,11 @@ int DrmResources::Init() {
> > >  return ret;
> > >}
> > > 
> > > +  ret = drmSetClientCap(fd(), DRM_CLIENT_CAP_WRITEBACK_CONNECTORS, 1);
> > > +  if (ret) {
> > > +ALOGI("Failed to set writeback cap %d", ret);
> > > +ret = 0;
> > > +  }
> > >drmModeResPtr res = drmModeGetResources(fd());
> > >if (!res) {
> > >  ALOGE("Failed to get DrmResources resources");
> > > @@ -77,6 +84,7 @@ int DrmResources::Init() {
> > >std::pair(res->max_width, res->max_height);
> > > 
> > >bool found_primary = false;
> > > +  int primary_index = 0;
> > >int display_num = 1;
> > > 
> > >for (int i = 0; !ret && i < res->count_crtcs; ++i) {
> > > @@ -162,18 +170,22 @@ int DrmResources::Init() {
> > >  if (conn->internal() && !found_primary) {
> > >conn->set_display(0);
> > >found_primary = true;
> > > -} else {
> > > +} else if (conn->external()) {
> > > +  if (!found_primary) primary_index++;
> > 
> > This is against style guide (and same below).
> 
> I suppose that's what happens when you skip some steps from 
> "Submitting patches", clang will catch this next time.
> 

I might look into adding a hook to gitlab to automatically run this, time
permitting.

Sean


-- 
Sean Paul, Software Engineer, Google / Chromium OS
___
dri-devel mailing list
dri-devel@lists.freedesktop.org

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

[Alexandru-Cosmin Gheorghe]

Hi Sean,

Thank you for the feedback.
I will comeback with v2, later on.

On Mon, Mar 19, 2018 at 02:03:54PM -0400, Sean Paul wrote:
> On Tue, Mar 13, 2018 at 04:21:20PM +, 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.
> 
> Thank you for your patch, it's looking good. Feel free to submit the v2 in
> multiple patches. Also note that we've migrated to gitlab, the new tree is
> located here:
> 
> https://gitlab.freedesktop.org/drm-hwcomposer/drm-hwcomposer

This seems to be behind
https://anongit.freedesktop.org/git/drm_hwcomposer.git.
Is this location deprecated or does it serve other purposes.


> 
> 
> > 
> > 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.
> > 
> > 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.
> > 
> > 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?
> > 
> > Some unfinished/untested things:
> > - Make sure the DrmFrameBuffer allocates one of the formats reported
> >   in WRITEBACK_PIXEL_FORMATS.
> > - I'm using a hacked setup where, when needed it, the GL compositing is
> >   done by Surfaceflinger, so I'm not sure how well this changes are
> >   getting along with the GLCompositorWorker.
> > 
> > [1] 
> > https://people.freedesktop.org/~cbrill/dri-log/?channel=dri-devel_names==2018-02-23
> > [2] 
> > https://people.freedesktop.org/~cbrill/dri-log/?channel=dri-devel_names==2018-02-09
> > [3] 
> > https://lists.freedesktop.org/archives/dri-devel/2018-February/167703.html
> > 
> > Signed-off-by: Alexandru Gheorghe 
> > ---
> >  drmconnector.cpp |  36 ++-
> >  drmconnector.h   |   8 +++
> >  drmcrtc.cpp  |  11 +++-
> >  drmcrtc.h|   8 ++-
> >  drmdisplaycompositor.cpp | 164 
> > +--
> >  drmdisplaycompositor.h   |  16 +++--
> >  drmencoder.cpp   |  15 +
> >  drmencoder.h |   7 +-
> >  drmhwctwo.cpp|   1 +
> >  drmresources.cpp |  56 +++-
> >  drmresources.h   |   1 +
> >  11 files changed, 306 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drmconnector.cpp b/drmconnector.cpp
> > index 145518f..2ed4f23 100644
> > --- a/drmconnector.cpp
> > +++ b/drmconnector.cpp
> > @@ -52,6 +52,23 @@ int DrmConnector::Init() {
> >  ALOGE("Could not get CRTC_ID property\n");
> >  return ret;
> >}
> > +  if (writeback()) {
> > +ret = drm_->GetConnectorProperty(*this, "WRITEBACK_PIXEL_FORMATS", 
> > 

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

[Sean Paul]

On Tue, Mar 13, 2018 at 04:21:20PM +, 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.

Thank you for your patch, it's looking good. Feel free to submit the v2 in
multiple patches. Also note that we've migrated to gitlab, the new tree is
located here:

https://gitlab.freedesktop.org/drm-hwcomposer/drm-hwcomposer


> 
> 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.
> 
> 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.
> 
> 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?
> 
> Some unfinished/untested things:
> - Make sure the DrmFrameBuffer allocates one of the formats reported
>   in WRITEBACK_PIXEL_FORMATS.
> - I'm using a hacked setup where, when needed it, the GL compositing is
>   done by Surfaceflinger, so I'm not sure how well this changes are
>   getting along with the GLCompositorWorker.
> 
> [1] 
> https://people.freedesktop.org/~cbrill/dri-log/?channel=dri-devel_names==2018-02-23
> [2] 
> https://people.freedesktop.org/~cbrill/dri-log/?channel=dri-devel_names==2018-02-09
> [3] https://lists.freedesktop.org/archives/dri-devel/2018-February/167703.html
> 
> Signed-off-by: Alexandru Gheorghe 
> ---
>  drmconnector.cpp |  36 ++-
>  drmconnector.h   |   8 +++
>  drmcrtc.cpp  |  11 +++-
>  drmcrtc.h|   8 ++-
>  drmdisplaycompositor.cpp | 164 
> +--
>  drmdisplaycompositor.h   |  16 +++--
>  drmencoder.cpp   |  15 +
>  drmencoder.h |   7 +-
>  drmhwctwo.cpp|   1 +
>  drmresources.cpp |  56 +++-
>  drmresources.h   |   1 +
>  11 files changed, 306 insertions(+), 17 deletions(-)
> 
> diff --git a/drmconnector.cpp b/drmconnector.cpp
> index 145518f..2ed4f23 100644
> --- a/drmconnector.cpp
> +++ b/drmconnector.cpp
> @@ -52,6 +52,23 @@ int DrmConnector::Init() {
>  ALOGE("Could not get CRTC_ID property\n");
>  return ret;
>}
> +  if (writeback()) {
> +ret = drm_->GetConnectorProperty(*this, "WRITEBACK_PIXEL_FORMATS", 
> _pixel_formats_);
> +if (ret) {
> +  ALOGE("Could not get WRITEBACK_PIXEL_FORMATS connector_id = %d\n", 
> id_);
> +  return ret;
> +}
> +ret = drm_->GetConnectorProperty(*this, "WRITEBACK_FB_ID", 
> _fb_id_);
> +if (ret) {
> +  ALOGE("Could not get WRITEBACK_FB_ID connector_id = %d\n", id_);
> +  return ret;
> +}
> +ret = drm_->GetConnectorProperty(*this, "WRITEBACK_OUT_FENCE_PTR", 
> _out_fence_);
> +if (ret) {
> +  ALOGE("Could not get 

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

[Alexandru-Cosmin Gheorghe]

Hi Stefan,

On Tue, Mar 13, 2018 at 07:20:33PM +0100, Stefan Schake wrote:
> Hey Alexandru,
> 
> On Tue, Mar 13, 2018 at 5:21 PM, 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].
> >
> > 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.
> 
> I think when Android deems that the display is sufficiently idle, it will
> disable VSync through setVsyncEnabled. Presumably we could just trigger the
> flattening on an enabled -> disabled transition?
> 
> Thanks,
> Stefan

I will look into it thanks.

-- 
Cheers,
Alex G
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Stefan Schake]

Hey Alexandru,

On Tue, Mar 13, 2018 at 5:21 PM, 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].
>
> 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.

I think when Android deems that the display is sufficiently idle, it will
disable VSync through setVsyncEnabled. Presumably we could just trigger the
flattening on an enabled -> disabled transition?

Thanks,
Stefan
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Alexandru Gheorghe]

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.

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.

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?

Some unfinished/untested things:
- Make sure the DrmFrameBuffer allocates one of the formats reported
  in WRITEBACK_PIXEL_FORMATS.
- I'm using a hacked setup where, when needed it, the GL compositing is
  done by Surfaceflinger, so I'm not sure how well this changes are
  getting along with the GLCompositorWorker.

[1] 
https://people.freedesktop.org/~cbrill/dri-log/?channel=dri-devel_names==2018-02-23
[2] 
https://people.freedesktop.org/~cbrill/dri-log/?channel=dri-devel_names==2018-02-09
[3] https://lists.freedesktop.org/archives/dri-devel/2018-February/167703.html

Signed-off-by: Alexandru Gheorghe 
---
 drmconnector.cpp |  36 ++-
 drmconnector.h   |   8 +++
 drmcrtc.cpp  |  11 +++-
 drmcrtc.h|   8 ++-
 drmdisplaycompositor.cpp | 164 +--
 drmdisplaycompositor.h   |  16 +++--
 drmencoder.cpp   |  15 +
 drmencoder.h |   7 +-
 drmhwctwo.cpp|   1 +
 drmresources.cpp |  56 +++-
 drmresources.h   |   1 +
 11 files changed, 306 insertions(+), 17 deletions(-)

diff --git a/drmconnector.cpp b/drmconnector.cpp
index 145518f..2ed4f23 100644
--- a/drmconnector.cpp
+++ b/drmconnector.cpp
@@ -52,6 +52,23 @@ int DrmConnector::Init() {
 ALOGE("Could not get CRTC_ID property\n");
 return ret;
   }
+  if (writeback()) {
+ret = drm_->GetConnectorProperty(*this, "WRITEBACK_PIXEL_FORMATS", 
_pixel_formats_);
+if (ret) {
+  ALOGE("Could not get WRITEBACK_PIXEL_FORMATS connector_id = %d\n", id_);
+  return ret;
+}
+ret = drm_->GetConnectorProperty(*this, "WRITEBACK_FB_ID", 
_fb_id_);
+if (ret) {
+  ALOGE("Could not get WRITEBACK_FB_ID connector_id = %d\n", id_);
+  return ret;
+}
+ret = drm_->GetConnectorProperty(*this, "WRITEBACK_OUT_FENCE_PTR", 
_out_fence_);
+if (ret) {
+  ALOGE("Could not get WRITEBACK_OUT_FENCE_PTR connector_id = %d\n", id_);
+  return ret;
+}
+  }
   return 0;
 }

@@ -78,8 +95,13 @@ bool DrmConnector::external() const {
  type_ == DRM_MODE_CONNECTOR_VGA;
 }

+#define DRM_MODE_CONNECTOR_WRITEBACK   18
+bool DrmConnector::writeback() const {
+return type_ == DRM_MODE_CONNECTOR_WRITEBACK;
+}
+
 bool DrmConnector::valid_type() const {
-  return internal() || external();
+  return internal() || external() || writeback();
 }

 int DrmConnector::UpdateModes() {
@@ -130,6