Closed Bug 911391 Opened 6 years ago Closed 6 years ago

HwcComposer implementation for JB.

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
1.2 FC (16sep)

People

(Reporter: sushilchauhan, Assigned: sushilchauhan)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 14 obsolete files)

12.35 KB, patch
mwu
: review+
Details | Diff | Splinter Review
4.07 KB, patch
mwu
: review+
pchang
: feedback+
Details | Diff | Splinter Review
15.48 KB, patch
mwu
: review+
Details | Diff | Splinter Review
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.
Summary: HwcComposer implemeatation for JB. → HwcComposer implementation for JB.
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.
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).
Attached patch set-planeAlpha (obsolete) — Splinter Review
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.
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.
Attachment #798072 - Attachment is obsolete: true
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.
Attachment #798073 - Attachment is obsolete: true
Hi Morris, I have updated and tested both the patches. Please review them.
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.
(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.
Assignee: nobody → sushilchauhan
Blocks: HWComposer
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #799782 - Flags: review?(vlin)
Attachment #799782 - Flags: review?(pchang)
Attachment #799782 - Flags: review?(mwu)
Attachment #799782 - Flags: review?(mtseng)
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.
Attachment #799785 - Flags: review?(vlin)
Attachment #799785 - Flags: review?(pchang)
Attachment #799785 - Flags: review?(mwu)
Attachment #799785 - Flags: review?(mtseng)
Attachment #799785 - Flags: review?(dwilson)
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 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.
(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 ?
(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.
(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.
(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.
(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.
(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
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.
Attachment #799782 - Attachment is obsolete: true
Attachment #799782 - Flags: review?(vlin)
Attachment #799782 - Flags: review?(pchang)
Attachment #799782 - Flags: review?(mwu)
Attachment #799782 - Flags: review?(mtseng)
Attachment #801041 - Flags: review?(vlin)
Attachment #801041 - Flags: review?(pchang)
Attachment #801041 - Flags: review?(mwu)
Attachment #801041 - Flags: review?(mtseng)
Attachment #801041 - Flags: review?(dwilson)
Attached patch Add support for MDP Composition. (obsolete) — Splinter Review
Minimized code changes in GonkDisplayJB.
Attachment #799785 - Attachment is obsolete: true
Attachment #799785 - Flags: review?(vlin)
Attachment #799785 - Flags: review?(pchang)
Attachment #799785 - Flags: review?(mwu)
Attachment #799785 - Flags: review?(mtseng)
Attachment #799785 - Flags: review?(dwilson)
Attachment #801045 - Flags: review?(vlin)
Attachment #801045 - Flags: review?(pchang)
Attachment #801045 - Flags: review?(mwu)
Attachment #801045 - Flags: review?(mtseng)
Attachment #801045 - Flags: review?(dwilson)
(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.
(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 ?
(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
(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 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 on attachment 801045 [details] [diff] [review]
Add support for MDP Composition.

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

Please review.
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 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.
Attachment #801041 - Attachment is obsolete: true
Attachment #801041 - Flags: review?(vlin)
Attachment #801041 - Flags: review?(pchang)
Attachment #801041 - Flags: review?(mwu)
Attachment #801041 - Flags: review?(mtseng)
Attachment #801041 - Flags: review?(dwilson)
Attachment #801944 - Flags: review?(vlin)
Attachment #801944 - Flags: review?(pchang)
Attachment #801944 - Flags: review?(mwu)
Attachment #801944 - Flags: review?(mtseng)
Attachment #801944 - Flags: review?(dwilson)
Attached patch Add support for MDP Composition. (obsolete) — Splinter Review
Attachment #801045 - Attachment is obsolete: true
Attachment #801045 - Flags: review?(vlin)
Attachment #801045 - Flags: review?(pchang)
Attachment #801045 - Flags: review?(mwu)
Attachment #801045 - Flags: review?(mtseng)
Attachment #801045 - Flags: review?(dwilson)
Attachment #801947 - Flags: review?(vlin)
Attachment #801947 - Flags: review?(pchang)
Attachment #801947 - Flags: review?(mwu)
Attachment #801947 - Flags: review?(mtseng)
Attachment #801947 - Flags: review?(dwilson)
Removed GetNextLayer() and ReallocLayerList() from GonkDisplayJB. Please review the latest patches.
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.
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.
(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.
(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).
(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".
(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 ?
(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).
(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.
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.
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.
@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)
(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?
(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.
Duplicate of this bug: 902831
Attachment #803027 - Flags: feedback?(sushilchauhan)
Attachment #803027 - Flags: feedback?(pchang)
Attachment #803027 - Flags: feedback?(mwu)
Attachment #803030 - Flags: feedback?(sushilchauhan)
Attachment #803030 - Flags: feedback?(pchang)
Attachment #803030 - Flags: feedback?(mwu)
Attachment #803031 - Flags: feedback?(sushilchauhan)
Attachment #803031 - Flags: feedback?(pchang)
Attachment #803031 - Flags: feedback?(mwu)
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.
Attachment #803027 - Attachment description: Remove local hwcomposer.h copy → Part 1 - Remove local hwcomposer.h copy
Attachment #803030 - Attachment description: Add GetFBSurface to GonkDisplay → Part 2 - Add GetFBSurface to GonkDisplay
Attachment #803031 - Attachment description: Add Jellybean support to HwcComposer2D → Part 3 - Add Jellybean support to HwcComposer2D
Attachment #803030 - Attachment is obsolete: true
Attachment #803030 - Flags: feedback?(sushilchauhan)
Attachment #803030 - Flags: feedback?(pchang)
Attachment #803030 - Flags: feedback?(mwu)
Attachment #803072 - Flags: feedback?(sushilchauhan)
Attachment #803072 - Flags: feedback?(pchang)
Attachment #803072 - Flags: feedback?(mwu)
Attachment #803031 - Attachment is obsolete: true
Attachment #803031 - Flags: feedback?(sushilchauhan)
Attachment #803031 - Flags: feedback?(pchang)
Attachment #803031 - Flags: feedback?(mwu)
Attachment #803073 - Flags: feedback?(sushilchauhan)
Attachment #803073 - Flags: feedback?(pchang)
Attachment #803073 - Flags: feedback?(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.
Attachment #803031 - Attachment is obsolete: false
Attachment #803031 - Attachment is obsolete: true
(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.
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.
@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
Attachment #803424 - Flags: review?(dwilson)
Attachment #803072 - Flags: feedback?(pchang) → feedback+
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 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
(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).
Blocks: 915545
Attachment #803073 - Attachment is obsolete: true
Attachment #803073 - Flags: feedback?(sushilchauhan)
Attachment #803073 - Flags: feedback?(pchang)
Attachment #803073 - Flags: feedback?(mwu)
Attachment #803730 - Flags: review?(mwu)
Blocks: 915729
Comment on attachment 803424 [details] [diff] [review]
Add support for partial HWC Composition on JB - v1

Moved to bug 915729
Attachment #803424 - Attachment is obsolete: true
Attachment #803424 - Flags: review?(dwilson)
Comment on attachment 799239 [details] [diff] [review]
set-planeAlpha

Rolled up into attachment 803730 [details] [diff] [review]
Attachment #799239 - Attachment is obsolete: true
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.
Rebased to tip of m-c
Attachment #803730 - Attachment is obsolete: true
Attachment #803730 - Flags: review?(mwu)
Attachment #804319 - Flags: review?(mwu)
Attachment #803072 - Flags: review?(mwu)
Attachment #803072 - Flags: feedback?(sushilchauhan)
Attachment #803072 - Flags: feedback?(mwu)
Attachment #803027 - Flags: review?(mwu)
Attachment #803027 - Flags: feedback?(sushilchauhan)
Attachment #803027 - Flags: feedback?(pchang)
Attachment #803027 - Flags: feedback?(mwu)
Attachment #801944 - Attachment is obsolete: true
Attachment #801944 - Flags: review?(vlin)
Attachment #801944 - Flags: review?(pchang)
Attachment #801944 - Flags: review?(mwu)
Attachment #801944 - Flags: review?(mtseng)
Attachment #801944 - Flags: review?(dwilson)
Attachment #801947 - Attachment is obsolete: true
Attachment #801947 - Flags: review?(vlin)
Attachment #801947 - Flags: review?(pchang)
Attachment #801947 - Flags: review?(mwu)
Attachment #801947 - Flags: review?(mtseng)
Attachment #801947 - Flags: review?(dwilson)
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.
Depends on: 887819
Attachment #803027 - Flags: review?(mwu) → review+
Attachment #803072 - Flags: review?(mwu) → review+
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.
Attachment #804319 - Flags: review?(mwu) → review+
Keywords: checkin-needed
Depends on: 973967
You need to log in before you can comment on or make changes to this bug.