HwcComposer2D is currently designed for HWC in ICS only and disabled in master so far. We have to revise it or design a new one to support HWC in JB. As attached UML, HwcComposer2D plays the role to link between LayerManagerComposite and GonkDisplay(interface to HAL). Without proper HwcComposer2D, Composition won't be done by HWComposer but fall back to GLComposer(-->GLContextEGL::SwapBuffers). This Bug is to track the necessary implementation in Gecko side.

Why is this a confidential bug?

(In reply to Michael Wu [:mwu] from comment #1) > Why is this a confidential bug? Just a wrong operation .. :) Remove the confidential check now.

Copy/paste from bug 902954: There's quite some changes on this feature since ICS. The current plan is to split off a new HwcComposerJB that would live side by side the existing ICS implementation, all while sharing as much as possible.

this patch enable hardware composer on my nexus 4 device. anyone who want to apply this patch should apply another 2 patches mentioned in bug 896765 first. as a WIP patch, some function doesn't work correctly. e.g: I didn't handle RB swap for buffer with BGR pixel format and I treated it as RGB format so the color of result looks wrong.

(In reply to Morris Tseng [:mtseng] from comment #5) > Created attachment 791085 [details] [diff] [review] > enable-hwcomposer-for-JB(WIP) > > this patch enable hardware composer on my nexus 4 device. > anyone who want to apply this patch should apply another 2 patches mentioned > in bug 896765 first. Hello Morris, Thanks for sharing! I'm currently working on this too. Though my revision of the patch is more closely tied to GonkDisplayJB. I'm hoping to have something to share soon so perhaps we can collaborate on this? At the very least I would appreciate some feedback ;) > > as a WIP patch, some function doesn't > work correctly. e.g: I didn't handle RB swap for buffer with BGR pixel > format and I treated it as RGB format so the color of result looks wrong. Yes, this is going to be interesting. We discussed is in bug 900827 and are currently leaning towards falling back to GPU when there's BGR textures on targets that don't support swapping. pchang created bug 901978 for this.

(In reply to Diego Wilson [:diego] from comment #6) > (In reply to Morris Tseng [:mtseng] from comment #5) > > Created attachment 791085 [details] [diff] [review] > > enable-hwcomposer-for-JB(WIP) > > > > this patch enable hardware composer on my nexus 4 device. > > anyone who want to apply this patch should apply another 2 patches mentioned > > in bug 896765 first. > > Hello Morris, > > Thanks for sharing! I'm currently working on this too. Though my revision of > the patch is more closely tied to GonkDisplayJB. I'm hoping to have > something to share soon so perhaps we can collaborate on this? At the very > least I would appreciate some feedback ;) > Sure, is there anything I could help? > > > > as a WIP patch, some function doesn't > > work correctly. e.g: I didn't handle RB swap for buffer with BGR pixel > > format and I treated it as RGB format so the color of result looks wrong. > > Yes, this is going to be interesting. We discussed is in bug 900827 and are > currently leaning towards falling back to GPU when there's BGR textures on > targets that don't support swapping. pchang created bug 901978 for this. Do you handle this problem in your upcoming patch? or something else I could do first? By the way, I didn't handle colorfill as well. I think colorfill fallback scenario is similar with RB swap case described above. Do you handle this as well?

(In reply to Morris Tseng [:mtseng] from comment #7) > (In reply to Diego Wilson [:diego] from comment #6) > > (In reply to Morris Tseng [:mtseng] from comment #5) > Sure, is there anything I could help? > That'd be great! Maybe you can try out the patches I'll share on Nexus. > By the way, I didn't handle colorfill as well. I think colorfill fallback > scenario is similar with RB swap case described above. Do you handle this as > well? Correct. I currently assume that it's running on a target that supports both these features. Perhaps you can help with the feature querying like bug 901978?

Hi Morris & Michael Wu, We are working on HWC implementation for JB to enable MDP Composition (OVERLAY). HwcComposer2D never queried for H/W capabilities, so in JB specific HWC wrapper in Gecko, we are also querying H/W capabilities (in mHwc->prepare) to find out if current frame can be composed fully or partially with MDP Composition. I tested, MDP Composition is working fine for UI layers (Thebes). We are also adding support in driver for MDP Composition of a Color layer. We are also taking care of R/B swap issue in HAL. For Image layers, it fails in Gecko itself with the error "no gralloc buffer". The only major issue which I am facing is tearing, it is clearly noticeable on apps like PowerSkull, GLFlingtest, etc. There is no buffer-locking mechanism for HWC (MDP) layers in Gecko. Like Android, Gecko needs to add support for setting “releaseFenceFd” (which is returned from driver) of a MDP layer so that framework does not use it until the Fence object gets signaled, when the buffer is no longer being read by MDP driver. It has been done for FB Surface but not for a Gecko layer (Image, Thebes, Canvas, etc.)

https://bugzilla.mozilla.org/show_bug.cgi?id=896765 has the patches to move the common utilities from HwcComposer2D to HwcUtils. This is in preparation for landing HwcComposerJB implementation in Gecko. The common utils will be shared by HwcComposer2D & HwcComposerJB. HwcComposerJB talks to HWC device (in HAL) via GonkDisplayJB. We want to discuss the buffer locking issue.

Hi Sushil, I'm trying some experimentation about releaseFenceFd. The experimentation is waiting for all releaseFenceFd right after hardware composer's set call. but those Fence objects never got signaled and my wait call always timeout. Is there something wrong or I shouldn't wait fence right after set all? Here is some code snippet: if (mHwc->set(mHwc, HWC_NUM_DISPLAY_TYPES, displays)) { LOGE("Hardware device failed to render"); return false; } for(uint32_t i = 0; i < mList->numHwLayers; ++i) { if(mList->hwLayers[i].releaseFenceFd != -1 && st->hwLayers[i].compositionType == HWC_USE_OVERLAY) { sp<Fence> fence = new Fence(mList->hwLayers[i].releaseFenceFd); if(fence->wait(1000) == -ETIME) { LOGE("wait releaseFenceFd %d timeout", mList->hwLayers[i].releaseFenceFd); } } }

Hi Morris, These Fence objects are not getting signaled because driver is still reading them. Driver keeps on reading the buffers of current draw cycle until the set call of next draw cycle finishes (assuming the display_commit call in HAL is synchronous). So, releaseFenceFd of current draw cycle will get signaled in the next draw cycle after set call. So you should wait for (T-1) fences in T cycle. This experiment assumes 2 things: 1. App buffers are at-least double buffered. B2g apps are double buffered, so we are good here. 2. If rendering phase and composition phase happens in different threads then rendering thread must not over-write (T-1) buffer until Fence gets signaled in T cycle. I had tried this experiment in display hal. But it does not fix tearing in PowerSkull app.

We still have buffer share problem in b2g when using WebGL. Related bug: https://bugzilla.mozilla.org/show_bug.cgi?id=901225 So, even the fence behavior is correct in hardware composer. It may still tearing. Maybe you can try other apps that doesn't use WebGL to see if tearing happened

Based on Bug 896765(I think we have to), I create this patch to enable HWComposer with reference to attachment 791085 [details] [diff] [review].

Alright. Looks like we're duplicating our efforts a little bit between this bug and bug 911391. I'll be in the Oslo next week and I believe some of you guys from the Taipei graphics team will be there too. If so, it'll give us an opportunity to work together on this.

Comment on attachment 800008 [details] [diff] [review] enable_hwcomposer.patch Review of attachment 800008 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/gonk/HwcComposerJB.cpp @@ +488,5 @@ > + if (mHwc->set(mHwc, HWC_NUM_DISPLAY_TYPES, displays)) { > + LOGE("Hardware device failed to render"); > + return false; > + } > + I think this patch will lead to fd leak. Please check 'adb shell lsof | grep "sync"' when frames are using H/W Composition. How are you planning to handle buffer synchronization for HWC_OVERLAY layers ?

Comment on attachment 800008 [details] [diff] [review] enable_hwcomposer.patch Review of attachment 800008 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/gonk/HwcComposerJB.cpp @@ +473,5 @@ > + displays[HWC_DISPLAY_PRIMARY] = mList; > + > + if(mHwc->prepare(mHwc, HWC_NUM_DISPLAY_TYPES, displays)) { > + LOGE("Hardware device failed to prepare"); > + return false; Display HAL expects same list in prepare and set. As prepare has been called here, so mList should remain same for mHwc->set (with updated FBSurface handle for current frame) which gets called from GonkDisplayJB::Post() when it falls back to GPU Composition.

(In reply to Sushil from comment #17) > Comment on attachment 800008 [details] [diff] [review] > enable_hwcomposer.patch > > Review of attachment 800008 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: widget/gonk/HwcComposerJB.cpp > @@ +473,5 @@ > > + displays[HWC_DISPLAY_PRIMARY] = mList; > > + > > + if(mHwc->prepare(mHwc, HWC_NUM_DISPLAY_TYPES, displays)) { > > + LOGE("Hardware device failed to prepare"); > > + return false; > > Display HAL expects same list in prepare and set. As prepare has been called > here, so mList should remain same for mHwc->set (with updated FBSurface > handle for current frame) which gets called from GonkDisplayJB::Post() when > it falls back to GPU Composition. Both HwcComposerJB::TryRender() and GonkDisplay::Post() have its own prepare() and set(). Are you afraid of TryRender()->prepare() returns fail and finally fallback to GonkDisplayJB::Post() ? According to AOSP's comments on prepare() "(*prepare)() can be called more than once, the last call prevails."

(In reply to Diego Wilson [:diego] from comment #15) > Alright. Looks like we're duplicating our efforts a little bit between this > bug and bug 911391. I'll be in the Oslo next week and I believe some of you > guys from the Taipei graphics team will be there too. If so, it'll give us > an opportunity to work together on this. That's fine. I'm just preparing for integrating HWComposer to another HW platform. The implementation developing now and finally committed will be critical to my future work. Unfortunately, none of us who know about HWComposer will be in Oslo next week. We can keep following up Bug 911391. I think there aren't much essential differences between Bug 911391 and this one.

(In reply to vlin from comment #18) > (In reply to Sushil from comment #17) > > Comment on attachment 800008 [details] [diff] [review] > > enable_hwcomposer.patch > > > > Review of attachment 800008 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: widget/gonk/HwcComposerJB.cpp > > @@ +473,5 @@ > > > + displays[HWC_DISPLAY_PRIMARY] = mList; > > > + > > > + if(mHwc->prepare(mHwc, HWC_NUM_DISPLAY_TYPES, displays)) { > > > + LOGE("Hardware device failed to prepare"); > > > + return false; > > > > Display HAL expects same list in prepare and set. As prepare has been called > > here, so mList should remain same for mHwc->set (with updated FBSurface > > handle for current frame) which gets called from GonkDisplayJB::Post() when > > it falls back to GPU Composition. > > Both HwcComposerJB::TryRender() and GonkDisplay::Post() have its own > prepare() and set(). > Are you afraid of TryRender()->prepare() returns fail and finally fallback > to GonkDisplayJB::Post() ? > > According to AOSP's comments on prepare() > "(*prepare)() can be called more than once, the last call prevails." I will look into it.

FYI some gralloc refactoring was just enabled in bug 907745 which according to Sotaro causes some problems in HWC video playback [1]. I wouldn't be surprised if it causes some other issues in HWC. [1]https://bugzilla.mozilla.org/show_bug.cgi?id=907745#c20