911391 - HwcComposer implementation for JB.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Sushil]

Attachment: Add HwcComposer implementation to integrate with display HAL and to enable MDP Composition.

I have uploaded these 2 patches. Please review them.
1. Add-HW-Composer-wrapper-to-integrate-with-display-HAL: It has HwcComposer implementation for JB, required to add support for H/W Composition (OVERLAY layers). It talks to display HAL via GonkDisplayJB.
2. Add-support-for-MDP-Composition-feature: It makes changes in GonkDisplayJB to enable MDP Composition of HWC layers.

If frame has number of layers less than or equal to number of MDP pipes on the device and each layer qualifies for H/W composition then the complete frame can be composed with MDP only. MDP Composition has better power savings and performance as compared to GPU Composition. We are using mHwc->prepare call to check if the current frame can be fully supported with H/W Composition. If Yes, display hal programs corresponding MDP pipes to compose HWC layers of current frame. If No, composition falls back to GPU and only RGB pipe gets programmed to display FrameBuffer layer once it gets composed by GPU.

The patches work fine but it needs buffer synchronization in Gecko as I see tearing in couple of internal Benchmark apps. There is no buffer synchronization for HWC layers in gecko. We need to add support for setting “releaseFenceFd” (which is returned from driver)
of an OVERLAY layer so that framework does not use it until the Fence object gets signaled, when buffer is no longer being read by driver. It has been implemented for FB Surface but not for a Gecko layer (Image, Thebes, Canvas, etc.). So I tried to experiment in display HAL by waiting for (T-1) releaseFenceFd until it gets signalled after the display commit (set) call of T draw-cycle . It reduces tearing a lot but it does not fix it completely. This experiment assumes that b2g apps are double buffered and if rendering phase and composition phase happens in different threads then rendering thread must not over-write (T-1) buffer until we close corresponding releaseFenceFd in T cycle.

Comment 1

[Sushil]

Attachment: Add support for MDP Composition feature.

Comment 2

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Hi Sushil,
I tried those patches and some layers are missing on my device. In the homescreen, I only have app icons and page indicator on my scrren. Background, top status bar and soft home button are missing.

Comment 3

[Sushil]

Hi Morris,

It seems partial MDP Composition is kicking-in on Home screen and we have not enabled it yet in Gecko. So please make following changes in system.prop and re-compile:

debug.composition.type=gpu
-persist.hwc.mdpcomp.enable=false
+persist.hwc.mdpcomp.enable=true
+debug.mdpcomp.mixedmode.disable=1
+debug.mdpcomp.idletime=-1

Please make above 3 changes in system.prop and test. The purpose is to:
1. Enable MDP Composition.
2. Disable partial MDP Composition (I will add Gecko support in my next patch after full MDP Comp.)
3. Disable Idle Invalidator (There is no support in Gecko, it is just an optimization in display HAL).

Please let me know and share ADB/kernel logs, if you still see any issue. We are handling R/B swap issue in display HAL. We are working on landing the patch to handle R/B swap in display HAL (in CAF).

Comment 4

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: set-planeAlpha

Hi Sushil,
I found the attribute planeAlpha of hwLayer did not set to property value and layer is missing when this value is zero. I added a line to set 0xFF to planeAlpha in HwcComposerJB::PrepareHwcLayer and it works now.

Comment 5

[Sushil]

Thanks Morris. I will update my patch with your change. For now, we can set it to 0xFF but not sure why I didn't observe this issue during my testing. I will enable planeAlpha support (layer opacity < 1.0) in Gecko once we have the plane alpha support in display hal and driver. I also worked on sync fence for OVERLAY layers. I will update that too in the patch tomorrow.

Comment 6

[Sushil]

Attachment: Add HWComposer for JB to integrate with display HAL.

Comment 7

[Sushil]

Attachment: Add support for MDP Composition feature.

For OVERLAY layers, in current draw cycle, wait for previous retire Fence to signal which denotes that the contents on display have been replaced (driver is not reading them). Framework should not over-write previous buffers until we close the previous releaseFenceFds for buffer synchronization.

Comment 8

[Sushil]

Hi Morris, I have updated and tested both the patches. Please review them.

Comment 9

[Michael Wu [:mwu]]

Can you minimize your changes to GonkDisplayJB and keep it to HwcComposerJB? The use of HWC layers there is required to do early rendering, but I don't think any particularly sophisticated use of HWC layers belong there.

Also, isn't MDP a term specific to particular chips? If so, I recommend using more generic terms in the comments.

Comment 10

[Michael Wu [:mwu]]

(In reply to Sushil from comment #8)
> Hi Morris, I have updated and tested both the patches. Please review them.

You can explicitly request review in the attachment's detail page. Click the dropdown next to review and set it to "?". Then enter the bugzilla email address of the reviewer. Alternately the field also takes something like :mwu or :mtseng since it searches names also.

Comment 11

[Sushil]

Comment on attachment 799785 [details] [diff] [review]
Add support for MDP Composition feature.

Review of attachment 799785 [details] [diff] [review]:
-----------------------------------------------------------------

I am working on the review comments.

Comment 12

[Vincent Lin[:vilin]]

Comment on attachment 799782 [details] [diff] [review]
Add HWComposer for JB to integrate with display HAL.

Review of attachment 799782 [details] [diff] [review]:
-----------------------------------------------------------------

Why not just reuse and modify HwcComposer2D.cpp for JB support(HwcComposerJB.cpp) ?
It seems the foundation of mapping transform to rotation and flipping is missing.

Comment 13

[Vincent Lin[:vilin]]

Comment on attachment 799785 [details] [diff] [review]
Add support for MDP Composition feature.

Review of attachment 799785 [details] [diff] [review]:
-----------------------------------------------------------------

I agree with Michael's comment about minimizing the change to GonkDisplayJB.cpp.

Comment 14

[Vincent Lin[:vilin]]

(In reply to Sushil from comment #3)
> Hi Morris,
> 
> It seems partial MDP Composition is kicking-in on Home screen and we have
> not enabled it yet in Gecko. So please make following changes in system.prop
> and re-compile:
> 
> debug.composition.type=gpu
> -persist.hwc.mdpcomp.enable=false
> +persist.hwc.mdpcomp.enable=true
> +debug.mdpcomp.mixedmode.disable=1
> +debug.mdpcomp.idletime=-1
> 
> Please make above 3 changes in system.prop and test. The purpose is to:
> 1. Enable MDP Composition.
> 2. Disable partial MDP Composition (I will add Gecko support in my next
> patch after full MDP Comp.)
> 3. Disable Idle Invalidator (There is no support in Gecko, it is just an
> optimization in display HAL).
> 
> Please let me know and share ADB/kernel logs, if you still see any issue. We
> are handling R/B swap issue in display HAL. We are working on landing the
> patch to handle R/B swap in display HAL (in CAF).

What does partial MDP Composition mean ?
Display HAL leaves some of layers for GPU composition and just composes the rest ones and the one output from GPU composition ?

Comment 15

[Sushil]

(In reply to vlin from comment #12)
> Comment on attachment 799782 [details] [diff] [review]
> Add HWComposer for JB to integrate with display HAL.
> 
> Review of attachment 799782 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why not just reuse and modify HwcComposer2D.cpp for JB
> support(HwcComposerJB.cpp) ?
> It seems the foundation of mapping transform to rotation and flipping is
> missing.

We are planning to move the whole transform logic block (which is in HwcComposer2D.cpp) to a common utility function in HwcUtils, that is why I left the old one (in JB) as it is. The block will be replaced by a function call. Diego, we need to move few more stuff to HwcUtils.

Comment 16

[Sushil]

(In reply to vlin from comment #13)
> Comment on attachment 799785 [details] [diff] [review]
> Add support for MDP Composition feature.
> 
> Review of attachment 799785 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I agree with Michael's comment about minimizing the change to
> GonkDisplayJB.cpp.

Yes, I am working on minimizing the changes to GonkDisplayJB. I will update the patches as soon as they are tested.

Comment 17

[Sushil]

(In reply to vlin from comment #14)
> (In reply to Sushil from comment #3)
> > Hi Morris,
> > 
> > It seems partial MDP Composition is kicking-in on Home screen and we have
> > not enabled it yet in Gecko. So please make following changes in system.prop
> > and re-compile:
> > 
> > debug.composition.type=gpu
> > -persist.hwc.mdpcomp.enable=false
> > +persist.hwc.mdpcomp.enable=true
> > +debug.mdpcomp.mixedmode.disable=1
> > +debug.mdpcomp.idletime=-1
> > 
> > Please make above 3 changes in system.prop and test. The purpose is to:
> > 1. Enable MDP Composition.
> > 2. Disable partial MDP Composition (I will add Gecko support in my next
> > patch after full MDP Comp.)
> > 3. Disable Idle Invalidator (There is no support in Gecko, it is just an
> > optimization in display HAL).
> > 
> > Please let me know and share ADB/kernel logs, if you still see any issue. We
> > are handling R/B swap issue in display HAL. We are working on landing the
> > patch to handle R/B swap in display HAL (in CAF).
> 
> What does partial MDP Composition mean ?
> Display HAL leaves some of layers for GPU composition and just composes the
> rest ones and the one output from GPU composition ?

Yes. I am planning to add support for partial MDP Composition after this.

Comment 18

[Diego Wilson [:diego]]

(In reply to Sushil from comment #15)
> Diego, we need to move few more stuff to HwcUtils.

Agreed. I'll work on moving the transform/rotation logic to the common HwcUtils and see what else can be moved.

Comment 19

[Diego Wilson [:diego]]

(In reply to vlin from comment #12)
> Comment on attachment 799782 [details] [diff] [review]
> Add HWComposer for JB to integrate with display HAL.
> 
> Review of attachment 799782 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why not just reuse and modify HwcComposer2D.cpp for JB
> support(HwcComposerJB.cpp) ?
> It seems the foundation of mapping transform to rotation and flipping is
> missing.

The main reason is that ICS will still be supported in v1.2. There are enough compile time and run time differences between JB HWC and ICS HWC to warrant two separate implementations. Trust me, both Sushil and I tried keeping it all in HwcComposer2D but it gets waaay to messy.

As Sushil explained, we'll try to reduce code duplication via HwcUtils

Comment 20

[Sushil]

Attachment: Add HWComposer wrapper to integrate with display HAL.

Following updates in new patch:
1. Moved TryHwComposition() and mPrevRelFd[] from GonkDisplayJB to HwcComposerJB.
2. HWC layer transform logic block is same as HwcComposer2D now and it will be moved to HwcUtils. As discussed earlier, Diego can upload a clean-up patch to move common stuff to HwcUtils.

Comment 21

[Sushil]

Attachment: Add support for MDP Composition.

Minimized code changes in GonkDisplayJB.

Comment 22

[Vincent Lin[:vilin]]

(In reply to Diego Wilson [:diego] from comment #19)
> (In reply to vlin from comment #12)
> > Comment on attachment 799782 [details] [diff] [review]
> > Add HWComposer for JB to integrate with display HAL.
> > 
> > Review of attachment 799782 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Why not just reuse and modify HwcComposer2D.cpp for JB
> > support(HwcComposerJB.cpp) ?
> > It seems the foundation of mapping transform to rotation and flipping is
> > missing.
> 
> The main reason is that ICS will still be supported in v1.2. There are
> enough compile time and run time differences between JB HWC and ICS HWC to
> warrant two separate implementations. Trust me, both Sushil and I tried
> keeping it all in HwcComposer2D but it gets waaay to messy.
> 
> As Sushil explained, we'll try to reduce code duplication via HwcUtils

The idea to create HwcComposerJB for JB is definitely fine. I just thought to create HwcComposerJB cloned from HwcComposer2D.cpp and modify HwcComposerJB for JB. Then the previous common infrastructure is migrated to HwcComposerJB.cpp. Anyway, to maintain the common infrastructure in HwcUtils is cool.

Comment 23

[Vincent Lin[:vilin]]

(In reply to Sushil from comment #0)

> The patches work fine but it needs buffer synchronization in Gecko as I see
> tearing in couple of internal Benchmark apps. There is no buffer
> synchronization for HWC layers in gecko. We need to add support for setting
> “releaseFenceFd” (which is returned from driver)
> of an OVERLAY layer so that framework does not use it until the Fence object
> gets signaled, when buffer is no longer being read by driver. It has been

Don't we have to add acquireFenceFd support for Gecko layers ?
For GL-rendering case, I think the mHwc->set() also have to wait for all acquireFenceFds signaled.

> completely. This experiment assumes that b2g apps are double buffered and if
> rendering phase and composition phase happens in different threads then
> rendering thread must not over-write (T-1) buffer until we close
> corresponding releaseFenceFd in T cycle.

Is the assumption(or limitation) now because only retireFenceFd is implemented for buffer synchronization at present ?

Comment 24

[Sushil]

(In reply to vlin from comment #23)
> (In reply to Sushil from comment #0)
> 
> Don't we have to add acquireFenceFd support for Gecko layers ?
> For GL-rendering case, I think the mHwc->set() also have to wait for all
> acquireFenceFds signaled.
> 

I have set acquireFenceFd as -1 because I believe, at the start of Composition phase, the content has already been drawn on Gecko layers. Is this not true for GL-rendering case? mHwc->set() can wait for an acquireFenceFd which is != -1

Comment 25

[Sushil]

(In reply to vlin from comment #23)
> (In reply to Sushil from comment #0)
> 
> Is the assumption(or limitation) now because only retireFenceFd is
> implemented for buffer synchronization at present ?

As per hwc header, "retireFenceFd" getting signaled signifies the contents on display have been replaced. So I believe waiting on retire Fence is sufficient in HwcComposerJB. Even, waiting on each releaseFenceFd in HwcComposerJB, cannot control the rendering thread.To achieve buffer synchronization, rendering thread must not over-write (T-1)th cycle buffers until we close corresponding releaseFenceFd in the Tth cycle.

Comment 26

[Sushil]

Comment on attachment 801041 [details] [diff] [review]
Add HWComposer wrapper to integrate with display HAL.

Review of attachment 801041 [details] [diff] [review]:
-----------------------------------------------------------------

Please review the latest uploaded patches.

Comment 27

[Sushil]

Comment on attachment 801045 [details] [diff] [review]
Add support for MDP Composition.

Review of attachment 801045 [details] [diff] [review]:
-----------------------------------------------------------------

Please review.

Comment 28

[Peter Chang[:pchang]]

Comment on attachment 801045 [details] [diff] [review]
Add support for MDP Composition.

Review of attachment 801045 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/gonk/libdisplay/GonkDisplayJB.cpp
@@ +191,4 @@
>      }
>  
>      hwc_display_contents_1_t *displays[HWC_NUM_DISPLAY_TYPES] = {NULL};
> +    bool hwcPrepared = (mList->numHwLayers != 0) ? true : false;

I think the numHwLayers indicate that HWC is using or not. (using HWC when numHwLayers > 0)
So using hwcPrepared flag didn't link to "HWC is using" for me.

@@ +198,4 @@
>      mList->flags = HWC_GEOMETRY_CHANGED;
> +
> +    if (!hwcPrepared) {
> +        mList->numHwLayers = 2;

Use hwcPrepared here, but it doesn't explain why we need to set numHwLayer as 2 if hwcPrepared is false.

@@ +272,5 @@
> +    return mFBSurface->lastHandle;
> +}
> +
> +bool
> +GonkDisplayJB::ReallocLayerList()

Do we need to put HwLayer logic into GonkDisplay class? Why not handle it under HwcComposer module?
Because the list is more generic data structure and is shared by ICS/JB.

How about create a basic class, HwcComposer, and put the common code shared by HwcComposerJB and HwcComposerICS?

Or you move hwList to GonkDisplayJB because GPU composition needs to call hwc::set with hwList info.

@@ +294,5 @@
> +}
> +
> +hwc_layer_1_t*
> +GonkDisplayJB::GetNextLayer()
> +{

Should be put into HwcComposer class...

Comment 29

[Sushil]

Comment on attachment 801045 [details] [diff] [review]
Add support for MDP Composition.

Review of attachment 801045 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/gonk/libdisplay/GonkDisplayJB.cpp
@@ +191,4 @@
>      }
>  
>      hwc_display_contents_1_t *displays[HWC_NUM_DISPLAY_TYPES] = {NULL};
> +    bool hwcPrepared = (mList->numHwLayers != 0) ? true : false;

No, "hwcPrepared" signifies if mHwc->prepare() has been called or not, for "current draw cycle". So, if we fail in PrepareHwcLayer() call from HwcComposerJB::TryRender() or last draw cycle was successful with MDP Composition, then hwcPrepared is false because mHwc->prepare has not been called yet. But if we fall back to GPU Composition after mHwc->prepare call in TryHwComposition, hwcPrepared should be true because hwc (in display hal) has been prepared for this cycle and MDP pipe has been configured for FB layer.

@@ +198,4 @@
>      mList->flags = HWC_GEOMETRY_CHANGED;
> +
> +    if (!hwcPrepared) {
> +        mList->numHwLayers = 2;

hwcPrepared is false, means mHwc->prepare hasn't been called yet and it is GPU Composition, so follow the default path for Post(), i.e. background + FB layer.

@@ +272,5 @@
> +    return mFBSurface->lastHandle;
> +}
> +
> +bool
> +GonkDisplayJB::ReallocLayerList()

Yes, hwcList was member of GonkDisplayJB because GPU Composition path needs it, even when HwcComposerJB module is not present or disabled.

mList cannot be shared between ICS & JB because hwc list data structure (hwc_display_contents_1_t in JB) has significantly changed from ICS.

Regarding creating a base class, Diego tried it but it looks too messed-up so we added HwcUtils to keep the common code shared by ICS and JB.

@@ +294,5 @@
> +}
> +
> +hwc_layer_1_t*
> +GonkDisplayJB::GetNextLayer()
> +{

I will try, if I can move GetNextLayer and ReallocLayerList to HwcComposerJB.

Comment 30

[Sushil]

Attachment: Add HWComposer wrapper to integrate with display HAL.

Comment 31

[Sushil]

Attachment: Add support for MDP Composition.

Comment 32

[Sushil]

Removed GetNextLayer() and ReallocLayerList() from GonkDisplayJB. Please review the latest patches.

Comment 33

[Sushil]

Comment on attachment 801944 [details] [diff] [review]
Add HWComposer wrapper to integrate with display HAL.

Review of attachment 801944 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/gonk/HwcComposerJB.cpp
@@ +64,5 @@
> +      sInstance = nullptr;
> +      return;
> +  }
> +
> +  mList = (hwc_display_contents_1_t*)display->GetHwcList();

mList will be pointing to the private member 'mList' of GonkDisplayJB. And later on, we are re-allocating it in HwcComposerJB::ReallocLayerList. It looks fine if we think of 'GonkDisplayJB' as just an interface. Otherwise, I was thinking of adding "friend class HwcComposerJB;" declaration in GonkDisplayJB class and remove GetHwcList(). Any suggestions ? These patches work fine though.

Comment 34

[Michael Wu [:mwu]]

Please remove all changes from GonkDisplayJB.cpp. The use of hwc in GonkDisplayJB is *only* because it is required, and GonkDisplay.h is the only interface that other code is permitted to use.

Comment 35

[Vincent Lin[:vilin]]

(In reply to Sushil from comment #24)
> (In reply to vlin from comment #23)
> > (In reply to Sushil from comment #0)
> > 
> > Don't we have to add acquireFenceFd support for Gecko layers ?
> > For GL-rendering case, I think the mHwc->set() also have to wait for all
> > acquireFenceFds signaled.
> > 
> 
> I have set acquireFenceFd as -1 because I believe, at the start of
> Composition phase, the content has already been drawn on Gecko layers. Is
> this not true for GL-rendering case? mHwc->set() can wait for an
> acquireFenceFd which is != -1


Bug 901225 and Bug 894847 is discussing buffer sync. issue.
I think for GL-rendering case now it's not true while for software-rendering case now it's true.

We need to make a path for SharedSurface to send acquireFenceFd to HWComposer and get releaseFenceFd from HWComposer.

Comment 36

[Vincent Lin[:vilin]]

(In reply to Sushil from comment #25)
> (In reply to vlin from comment #23)
> > (In reply to Sushil from comment #0)
> > 
> > Is the assumption(or limitation) now because only retireFenceFd is
> > implemented for buffer synchronization at present ?
> 
> As per hwc header, "retireFenceFd" getting signaled signifies the contents
> on display have been replaced. So I believe waiting on retire Fence is
> sufficient in HwcComposerJB. Even, waiting on each releaseFenceFd in

I think retireFenceFd is just to make sure display HAL safely swaps reading buffers in B2G process(composite) thread. 

> HwcComposerJB, cannot control the rendering thread.To achieve buffer
> synchronization, rendering thread must not over-write (T-1)th cycle buffers
> until we close corresponding releaseFenceFd in the Tth cycle.

releaseFenceFd is necessary for content process(rendering thread) to make sure swaps writing buffer safely(display controller is not reading).

Comment 37

[Peter Chang[:pchang]]

(In reply to Sushil from comment #29)
> Comment on attachment 801045 [details] [diff] [review]
> Add support for MDP Composition.
> 
> Review of attachment 801045 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/gonk/libdisplay/GonkDisplayJB.cpp
> @@ +191,4 @@
> >      }
> >  
> >      hwc_display_contents_1_t *displays[HWC_NUM_DISPLAY_TYPES] = {NULL};
> > +    bool hwcPrepared = (mList->numHwLayers != 0) ? true : false;
> 
> No, "hwcPrepared" signifies if mHwc->prepare() has been called or not, for
> "current draw cycle". So, if we fail in PrepareHwcLayer() call from
> HwcComposerJB::TryRender() or last draw cycle was successful with MDP
> Composition, then hwcPrepared is false because mHwc->prepare has not been
> called yet. But if we fall back to GPU Composition after mHwc->prepare call
> in TryHwComposition, hwcPrepared should be true because hwc (in display hal)
> has been prepared for this cycle and MDP pipe has been configured for FB
> layer.
> 
Please add comment for hwcPrepared flag because it is hard for others to know numHWLayer is identical to "hwcPrepare is called before or not".

Comment 38

[Sushil]

(In reply to vlin from comment #35)
> (In reply to Sushil from comment #24)
> > (In reply to vlin from comment #23)
> > > (In reply to Sushil from comment #0)
> > > 
> > > Don't we have to add acquireFenceFd support for Gecko layers ?
> > > For GL-rendering case, I think the mHwc->set() also have to wait for all
> > > acquireFenceFds signaled.
> > > 
> > 
> > I have set acquireFenceFd as -1 because I believe, at the start of
> > Composition phase, the content has already been drawn on Gecko layers. Is
> > this not true for GL-rendering case? mHwc->set() can wait for an
> > acquireFenceFd which is != -1
> 
> 
> Bug 901225 and Bug 894847 is discussing buffer sync. issue.
> I think for GL-rendering case now it's not true while for software-rendering
> case now it's true.
> 
> We need to make a path for SharedSurface to send acquireFenceFd to
> HWComposer and get releaseFenceFd from HWComposer.

I totally agree to this. HwcComposerJB needs acquireFenceFd for the layers for which content is not ready and there should be an API (like mFBSurface) so that HwcComposerJB can set releaseFenceFds (set by the driver during mHwc->set) so that rendering thread can wait for release Fence to get signaled. Who is working on this ?

Comment 39

[Sushil]

(In reply to vlin from comment #36)
> (In reply to Sushil from comment #25)
> > (In reply to vlin from comment #23)
> > > (In reply to Sushil from comment #0)
> > 
> > As per hwc header, "retireFenceFd" getting signaled signifies the contents
> > on display have been replaced. So I believe waiting on retire Fence is
> > sufficient in HwcComposerJB. Even, waiting on each releaseFenceFd in
> 
> I think retireFenceFd is just to make sure display HAL safely swaps reading
> buffers in B2G process(composite) thread. 
> 
> > HwcComposerJB, cannot control the rendering thread.To achieve buffer
> > synchronization, rendering thread must not over-write (T-1)th cycle buffers
> > until we close corresponding releaseFenceFd in the Tth cycle.
> 
> releaseFenceFd is necessary for content process(rendering thread) to make
> sure swaps writing buffer safely(display controller is not reading).

Yes, "waiting on (T-1) retireFenceFd" is temporary code to reduce tearing until we get an API to set releaseFenceFd for OVERLAY layers and pass it to content process (rendering thread) to make sure it waits until release Fence gets signaled (i.e. when driver is no longer reading them).

Comment 40

[Sushil]

(In reply to Michael Wu [:mwu] from comment #34)
> Please remove all changes from GonkDisplayJB.cpp. The use of hwc in
> GonkDisplayJB is *only* because it is required, and GonkDisplay.h is the
> only interface that other code is permitted to use.

:mwu, the only effective change in GonkDisplayJB is sharing 'mList' with HwcComposerJB. I could keep separate lists for HwcComposerJB and GonkDisplayJB, but shared 'mList' helps to:
1. Prevent two mHwc->prepare calls when we fall back from MDP Comp to GPU Comp. Because display hal takes care of configuring resource (like RGB pipe) for FB layer in the 1st prepare call. So calling mHwc->prepare 2nd time for current frame is not efficient when it falls back from MDP Comp to GPU Comp.
2. Shared 'mList' will also help in partial MDP Composition, where OVERLAY layers are composed along with FB layer.

Comment 41

[Michael Wu [:mwu]]

Isn't partial HWC composition going to be done entirely in HwcComposerJB?

If switching to GonkDisplayJB for GPU comp fallback is too slow, we can avoid GonkDisplayJB entirely.

Comment 42

[Sushil]

Yes, the logic to set "skip GPU render" flag on a Gecko layer (i.e. corresponding HWC layer has been marked as OVERLAY) will be done in HwcComposerJB after mHwc->prepare call. Once the GPU layers are composed on FB Surface, framework calls GetGonkDisplay()->SwapBuffers()->Post()->mHwc->set() which will take care of drawing OVERLAY layers and FB layer. I can share partial HWC patch in a new bug. We also need to take care of retireFenceFds of OVERLAY layers after set call.

Yes, I agree to this, GonkDisplayJB stuff can be moved to HwcComposerJB.

Comment 43

[Sushil]

@Diego, here is system.prop change to enable MDP Comp and disable partial MDP Comp in HAL:
-persist.hwc.mdpcomp.enable=false
+persist.hwc.mdpcomp.enable=true
+debug.mdpcomp.mixedmode.disable=1
+debug.mdpcomp.idletime=-1 (Disable it, just an optimization in hal, no support in Gecko)

Comment 44

[Peter Chang[:pchang]]

(In reply to Sushil from comment #42)
> Yes, the logic to set "skip GPU render" flag on a Gecko layer (i.e.
> corresponding HWC layer has been marked as OVERLAY) will be done in
> HwcComposerJB after mHwc->prepare call. Once the GPU layers are composed on
> FB Surface, framework calls
> GetGonkDisplay()->SwapBuffers()->Post()->mHwc->set() which will take care of
You are talking about partial HWC composition. Am I right?
It still needs to call GonkDisplay::Post func, how could you remove GonkDisplayJB?
Or you are talking about merging GonkDisplayJB to HwcComposerJB?

> drawing OVERLAY layers and FB layer. I can share partial HWC patch in a new
> bug. We also need to take care of retireFenceFds of OVERLAY layers after set
> call.
> 
> Yes, I agree to this, GonkDisplayJB stuff can be moved to HwcComposerJB.


1. Prevent two mHwc->prepare calls when we fall back from MDP Comp to GPU Comp. Because display hal takes care of configuring resource (like RGB pipe) for FB layer in the 1st prepare call. So calling mHwc->prepare 2nd time for current frame is not efficient when it falls back from MDP Comp to GPU Comp.

By the way, if the 2nd prepare happened, we actually just sent 2 layers(backgorund+FB layer) structure to hwc::prepare. Is the cost expensive?

Comment 45

[Sushil]

(In reply to peter chang[:pchang] from comment #44)
> (In reply to Sushil from comment #42)
> You are talking about partial HWC composition. Am I right?
> It still needs to call GonkDisplay::Post func, how could you remove
> GonkDisplayJB?
> Or you are talking about merging GonkDisplayJB to HwcComposerJB?
> 
Yes, I was pointing to merging GonkDisplayJB to HwcComposerJB.

> 
> 1. Prevent two mHwc->prepare calls when we fall back from MDP Comp to GPU
> Comp. Because display hal takes care of configuring resource (like RGB pipe)
> for FB layer in the 1st prepare call. So calling mHwc->prepare 2nd time for
> current frame is not efficient when it falls back from MDP Comp to GPU Comp.
> 
> By the way, if the 2nd prepare happened, we actually just sent 2
> layers(backgorund+FB layer) structure to hwc::prepare. Is the cost expensive?

1. Yes, 2nd prepare is cost expensive and it does not buy anything.
2. For partial HWC Comp, we need to have a common list so that OVERLAY layers can be drawn along with FB layer in mHwc->set call from Post(). I will upload partial HWC patch tomorrow. Worked on it and tested today.

Comment 47

[Diego Wilson [:diego]]

Attachment: Part 1 - Remove local hwcomposer.h copy

Comment 48

[Diego Wilson [:diego]]

Attachment: Part 2 - Add GetFBSurface to GonkDisplay

Comment 49

[Diego Wilson [:diego]]

Attachment: Part 3 - Add Jellybean support to HwcComposer2D

Comment 50

[Diego Wilson [:diego]]

After chatting with mwu I was able to reduce the size of the patch and pull it into HwcComposer2D. This also reduces the changes in GonkDisplay.

Comment 51

[Diego Wilson [:diego]]

Attachment: Part 2 - Add GetFBSurface to GonkDisplay - v2

Comment 52

[Diego Wilson [:diego]]

Attachment: Part 3 - Add Jellybean support to HwcComposer2D - v2

Comment 53

[Michael Wu [:mwu]]

Comment on attachment 803031 [details] [diff] [review]
Part 3 - Add Jellybean support to HwcComposer2D

Review of attachment 803031 [details] [diff] [review]:
-----------------------------------------------------------------

I like this approach. Much less code and keeps more logic all in one place.

::: widget/gonk/HwcComposer2D.cpp
@@ +412,5 @@
>  }
>  
> +
> +bool
> +HwcComposer2D::TryHwComposition()

This won't build on ICS, will it?

We might as well ifdef and make another TryHwComposition for ICS to make this work.

@@ +417,5 @@
> +{
> +    FramebufferSurface* fbsurface = GetGonkDisplay()->GetFBSurface();
> +
> +    if (!fbsurface) {
> +      LOGE("H/W Composition failed. FBSurface not initialized.");

Wrong indentation level

@@ +432,5 @@
> +
> +    mList->hwLayers[idx].hints = 0;
> +    mList->hwLayers[idx].flags = 0;
> +    mList->hwLayers[idx].transform = 0;
> +    mList->hwLayers[idx].handle = fbsurface->lastHandle;

Grabbing the last handle from fbsurface isn't so useful at the moment since we only do full HWC rendering, right?

@@ +458,5 @@
> +
> +    // Full MDP Composition
> +    mHwc->set(mHwc, HWC_NUM_DISPLAY_TYPES, displays);
> +
> +    for (int i=0; (i <= MAX_HWC_LAYERS); i++) {

Space before and after =

The extra parens in the middle shouldn't be necessary.

@@ +459,5 @@
> +    // Full MDP Composition
> +    mHwc->set(mHwc, HWC_NUM_DISPLAY_TYPES, displays);
> +
> +    for (int i=0; (i <= MAX_HWC_LAYERS); i++) {
> +        if (mPrevRelFd[i] > 0) {

if (mPrevRelFd[i] < 0)
  break;

This will reduce the indentation level in this loop.

@@ +465,5 @@
> +                // Wait for previous retire Fence to signal.
> +                // Denotes contents on display have been replaced.
> +                // For buffer-sync, framework should not over-write
> +                // prev buffers until we close prev releaseFenceFds
> +                sp<Fence> fence = new Fence(mPrevRelFd[i]);

Why do we need a special case for the first fence? close should also work there, right?

@@ +480,5 @@
> +
> +    mPrevRelFd[0] = mList->retireFenceFd;
> +    for (uint32_t j=0; (j < idx); j++) {
> +        if (mList->hwLayers[j].compositionType == HWC_OVERLAY) {
> +            mPrevRelFd[j+1] = mList->hwLayers[j].releaseFenceFd;

Space before and after +

::: widget/gonk/HwcComposer2D.h
@@ +68,5 @@
>      bool PrepareLayerList(layers::Layer* aContainer, const nsIntRect& aClip,
>            const gfxMatrix& aParentTransform, const gfxMatrix& aGLWorldTransform);
>  
> +    HwcDevice*  mHwc;
> +    HwcList*       mList;

These need to be aligned with the other fields.

Comment 54

[Sushil]

(In reply to Michael Wu [:mwu] from comment #53)
> Comment on attachment 803031 [details] [diff] [review]
> Part 3 - Add Jellybean support to HwcComposer2D
> 
> @@ +432,5 @@
> > +
> > +    mList->hwLayers[idx].hints = 0;
> > +    mList->hwLayers[idx].flags = 0;
> > +    mList->hwLayers[idx].transform = 0;
> > +    mList->hwLayers[idx].handle = fbsurface->lastHandle;
> 
> Grabbing the last handle from fbsurface isn't so useful at the moment since
> we only do full HWC rendering, right?
> 

Yes, it is not so useful in mHwc->prepare call but display HAL complains if FB layer handle is NULL.

> @@ +465,5 @@
> > +                // Wait for previous retire Fence to signal.
> > +                // Denotes contents on display have been replaced.
> > +                // For buffer-sync, framework should not over-write
> > +                // prev buffers until we close prev releaseFenceFds
> > +                sp<Fence> fence = new Fence(mPrevRelFd[i]);
> 
> Why do we need a special case for the first fence? close should also work
> there, right?
> 

First fence is retire Fence of (T-1)th cycle. So we just wait for retire Fence to signal (which denotes that contents on display have been replaced), then close it and releaseFenceFds of the list.

Comment 55

[Sushil]

It looks fine as the changes are less but there are separate mList for HwcComposer2D and GonkDisplayJB. So:
1. It will not support partial HWC Composition as OVERLAY layers needs to be drawn along with FB layer.
2. When we fall-back from full HWC Comp to GPU Comp, the 2nd prepare call is expensive as all the resources configured for FB layer in 1st prepare, will need to be unset (through out HAL and driver). Display HAL might support multiple prepare calls (no set call in between) but I am not sure if driver takes care of cleaning-up resources in such cases.

Comment 56

[Sushil]

Attachment: Add support for partial HWC Composition on JB - v1

@Diego, I have attached patch to support partial HWC Composition. HwcComposerJB changes can be easily picked to latest HwcComposer2D in your patch. But we need to think for GonkDisplayJB change.

The system prop change to enable partial HWC Composition in hal is:
-debug.mdpcomp.mixedmode.disable=1

Comment 57

[Peter Chang[:pchang]]

Comment on attachment 803073 [details] [diff] [review]
Part 3 - Add Jellybean support to HwcComposer2D - v2

Review of attachment 803073 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/gonk/HwcComposer2D.cpp
@@ +448,5 @@
> +    mList->hwLayers[idx].releaseFenceFd = -1;
> +    mList->hwLayers[idx].planeAlpha = 0xFF;
> +    mList->numHwLayers++;
> +
> +    LOGE("prepare() on HWC %p", mHwc);

No need for this log

@@ +454,5 @@
> +
> +    for (int j=0; (j < idx); j++) {
> +        if (mList->hwLayers[j].compositionType == HWC_FRAMEBUFFER) {
> +            // GPU or Partial MDP Composition
> +            LOGE("Layer %i is of HWC_FRAMEBUFFER composition type", j);

Should be "Layer %i is HWCxxxxxx" and do we have to set it as error?

@@ +484,5 @@
> +
> +    mPrevRelFd[0] = mList->retireFenceFd;
> +    for (uint32_t j=0; (j < idx); j++) {
> +        if (mList->hwLayers[j].compositionType == HWC_OVERLAY) {
> +            mPrevRelFd[j+1] = mList->hwLayers[j].releaseFenceFd;

If we had handled the retireFenceFd from last T-1 draw cycle, do we still need to handle the releaseFenceFD for each hwclayer here? Will it help for tearing?

Comment 58

[Vincent Lin[:vilin]]

Comment on attachment 803424 [details] [diff] [review]
Add support for partial HWC Composition on JB - v1

Review of attachment 803424 [details] [diff] [review]:
-----------------------------------------------------------------

If there's any case like that(after mHwc-prepare()) ?

the layer composition type by z-order :
FB0
OVL0
F1
OVL1

Then I think GPU can't just skip OVL0/1, for some conditions it has to clean the framebuffer at the clip region of OVL0/1.

This's also what Android SurfaceFlinger is dealing with.
https://github.com/Evervolv/android_frameworks_native/blob/jellybean/services/surfaceflinger/SurfaceFlinger.cpp#L1620

Comment 59

[Sushil]

(In reply to peter chang[:pchang] from comment #57)
> Comment on attachment 803073 [details] [diff] [review]
> Part 3 - Add Jellybean support to HwcComposer2D - v2
> 
> Review of attachment 803073 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/gonk/HwcComposer2D.cpp
> @@ +454,5 @@
> > +            LOGE("Layer %i is of HWC_FRAMEBUFFER composition type", j);
> 
> Should be "Layer %i is HWCxxxxxx" and do we have to set it as error?

No need for this log.

> @@ +484,5 @@
> > +
> > +    mPrevRelFd[0] = mList->retireFenceFd;
> > +    for (uint32_t j=0; (j < idx); j++) {
> > +        if (mList->hwLayers[j].compositionType == HWC_OVERLAY) {
> > +            mPrevRelFd[j+1] = mList->hwLayers[j].releaseFenceFd;
> 
> If we had handled the retireFenceFd from last T-1 draw cycle, do we still
> need to handle the releaseFenceFD for each hwclayer here? Will it help for
> tearing?

Yes, it will help to reduce tearing. These are releaseFenceFds of current draw cycle, so driver is still reading them. We will close them in the next draw cycle after the retire Fence of current draw cycle gets signaled (after the set call). But rendering thread needs to make sure that it does not over-write the buffers of current cycle until we close these releaseFenceFds in next cycle. So we need an API to set releaseFenceFd of OVERLAY layers and pass it to content process (rendering thread) to make sure it waits until release Fence gets signaled (i.e. when driver is no longer reading them).

Comment 60

[Diego Wilson [:diego]]

Attachment: Part 3 - Add Jellybean support to HwcComposer2D - v3

Comment 61

[Diego Wilson [:diego]]

Comment on attachment 803424 [details] [diff] [review]
Add support for partial HWC Composition on JB - v1

Moved to bug 915729

Comment 62

[Diego Wilson [:diego]]

Comment on attachment 799239 [details] [diff] [review]
set-planeAlpha

Rolled up into attachment 803730 [details] [diff] [review]

Comment 63

[Diego Wilson [:diego]]

Bug 887819 disabled gralloc back buffers which in turn disabled all HWC composition. I already chatted with the authors and they agreed to back it up.

Comment 64

[Diego Wilson [:diego]]

Attachment: Part 3 - Add Jellybean support to HwcComposer2D - v4

Rebased to tip of m-c

Comment 65

[Diego Wilson [:diego]]

All current have been successfully tested on a JB reference device and an ICS reference device. I had to manually back up the change in bug 887819 to re-enable gralloc backed layers. Hopefully that'll be backed up in m-c soon.

Comment 66

[Michael Wu [:mwu]]

Comment on attachment 804319 [details] [diff] [review]
Part 3 - Add Jellybean support to HwcComposer2D - v4

Review of attachment 804319 [details] [diff] [review]:
-----------------------------------------------------------------

I tested it on the Nexus 4, and HWC seems to have a habit of swapping R and B channels. Oh well. We can fix that later.

::: widget/gonk/HwcComposer2D.h
@@ +23,5 @@
>  #include <list>
>  
> +#include <hardware/hwcomposer.h>
> +
> +#define MAX_HWC_LAYERS 15

Any reason for this?

::: widget/gonk/Makefile.in
@@ +27,5 @@
>  
>  LOCAL_INCLUDES += \
>  	-I$(ANDROID_SOURCE)/hardware/libhardware/include \
>  	-I$(ANDROID_SOURCE)/hardware/libhardware_legacy/include \
> +        -I$(ANDROID_SOURCE)/frameworks/native/opengl/include \

This part is unnecessary - we already have it.

Comment 67

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/36ee8fab5c59
https://hg.mozilla.org/integration/b2g-inbound/rev/dcb0b06893a7
https://hg.mozilla.org/integration/b2g-inbound/rev/5dabcacc0aa1

Comment 68

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/36ee8fab5c59
https://hg.mozilla.org/mozilla-central/rev/dcb0b06893a7
https://hg.mozilla.org/mozilla-central/rev/5dabcacc0aa1