900827 - (HWComposer) [meta] B2G HWComposer integration issues

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Vincent Lin[:vilin]]

Attachment: HWC_Seq_diagram.gif

B2G already had HWComposer implemented by HwcComposer2D.cpp, GonkDisplayICS.cpp, GonkDisplayJB.cpp as a framework side for interfacing to HAL. The problem we encountered now is B2G can't reuse the HAL for Android OS without proprietary modifications regarding to the follows(kind of hacky).

4 FxOS-specific indications to HAL(HWC libs from chip vendor) 
hwcLayer.compositionType = HWC_USE_COPYBIT;    // indicate this layer needs HWC
hwcLayer.flags |= HWC_FORMAT_RB_SWAP;          // indicate HWC to swap R/B channel while composing
hwcLayer.flags |= HWC_COLOR_FILL;              // indicate it's a color layer
hwcLayer.transform = colorLayer->GetColor().Packed(); // For color layer, it indicates pixel color

Our certain OEM partner can't push their chip vendor conform B2G's HWComposer requirements.
I'm afraid this won't be a special case after all we may meet more and more different platforms.

This bug is to discuss if any chance to make B2G HWComposer totally compatible with Android's.
Here are my comments.
1. Can we replace ColorLayer by ThebesLayer ?
2. HWC HAL will cast buffer_handle_t to private_handle_t and get the format.
   Lots of HAL_PIXEL_FORMAT_xxxx_xxx enumerations were defined in <hardware/hardware.h>.
   Is HWC_FORMAT_RB_SWAP flag necessary ?
3. HWC_USE_OVERLAY is the Android-defined indication to do HW Composition while HWC_USE_COPYBIT is not defined.
   I would suggest to leave the platform-specific design to vendor's HAL.
4. The HWC policy implemented here is platform-specific.
http://mxr.mozilla.org/mozilla-central/source/widget/gonk/HwcComposer2D.cpp#484
   I would suggest to leave it to vendor's HAL. (by calling mHwc->prepare())

Simply speaking, B2G gonk for HWComposer can be built on top of HAL and ideally reuse the one for Android.
Attached *.gif  is the sequence diagram of Android's HWComposer referenced to AOSP.

Comment 1

[Vincent Lin[:vilin]]

1st comment is too wordy I think.

The objective can be simply described to "Make B2G reuse Android's HWComposer HAL".
The reason is to make all our partners effortless to leverage HW composition in FxOS. Or I'm afraid it will be a block for promoting FxOS, especially for OEM partners.

Comment 2

[Peter Chang[:pchang]]

(In reply to vlin from comment #0)
> Created attachment 784802 [details]
> HWC_Seq_diagram.gif
> 
> B2G already had HWComposer implemented by HwcComposer2D.cpp,
> GonkDisplayICS.cpp, GonkDisplayJB.cpp as a framework side for interfacing to
> HAL. The problem we encountered now is B2G can't reuse the HAL for Android
> OS without proprietary modifications regarding to the follows(kind of hacky).
> 
> 4 FxOS-specific indications to HAL(HWC libs from chip vendor) 
> hwcLayer.compositionType = HWC_USE_COPYBIT;    // indicate this layer needs
> HWC
> hwcLayer.flags |= HWC_FORMAT_RB_SWAP;          // indicate HWC to swap R/B
> channel while composing
> hwcLayer.flags |= HWC_COLOR_FILL;              // indicate it's a color layer
> hwcLayer.transform = colorLayer->GetColor().Packed(); // For color layer, it
> indicates pixel color
> 
> Our certain OEM partner can't push their chip vendor conform B2G's
> HWComposer requirements.
> I'm afraid this won't be a special case after all we may meet more and more
> different platforms.
> 
Right now B2G HWC is using copybit on qualcomm platform. I think it is reasonable to support HWC with HWC_OVERLAY type.

> This bug is to discuss if any chance to make B2G HWComposer totally
> compatible with Android's.
> Here are my comments.
> 1. Can we replace ColorLayer by ThebesLayer ?

For using HWC with HWC_OVERLAY, we will have layer number limitation, like at most three layers. No sure we could do optimize ColorLayer without creating another layer with gralloc buffer.

> 2. HWC HAL will cast buffer_handle_t to private_handle_t and get the format.
>    Lots of HAL_PIXEL_FORMAT_xxxx_xxx enumerations were defined in
> <hardware/hardware.h>.
>    Is HWC_FORMAT_RB_SWAP flag necessary ?

We need the FORMAT_RB_SWAP because FxOS not only output RGBX/RGBA but also BGRX/BGRA data. But Android only supports RGB format.

> 3. HWC_USE_OVERLAY is the Android-defined indication to do HW Composition
> while HWC_USE_COPYBIT is not defined.
>    I would suggest to leave the platform-specific design to vendor's HAL.
> 4. The HWC policy implemented here is platform-specific.
> http://mxr.mozilla.org/mozilla-central/source/widget/gonk/HwcComposer2D.
> cpp#484
>    I would suggest to leave it to vendor's HAL. (by calling mHwc->prepare())
> 

I think it is better to describe which kind of HWC policy need to discuss.

> Simply speaking, B2G gonk for HWComposer can be built on top of HAL and
> ideally reuse the one for Android.
> Attached *.gif  is the sequence diagram of Android's HWComposer referenced
> to AOSP.

Loop Diego to comment support HWC with HWC_OVERLAY type.

Comment 3

[Andreas Gal :gal]

1. ColorLayer is much faster than ThebesLayer. However, we should detect that ColorLayers are not supported and then fall back to ThebesLayer. We can have both. Fast composition of QC hardware, and work everywhere. Please detect this dynamically.

2. Is HWC_FORMAT_RB_SWAP flag necessary ? Detect QC hardware and support for RB_SWAP and then use it (dynamically), otherwise swap on the CPU (fallback).

Lets lobby chipset vendors to support these extensions but we can support HWC that doesn't have them. Will that work?

Comment 4

[Vincent Lin[:vilin]]

(In reply to peter chang[:pchang] from comment #2)
> (In reply to vlin from comment #0)
> > Created attachment 784802 [details]
> > HWC_Seq_diagram.gif
> > 
> > B2G already had HWComposer implemented by HwcComposer2D.cpp,
> > GonkDisplayICS.cpp, GonkDisplayJB.cpp as a framework side for interfacing to
> > HAL. The problem we encountered now is B2G can't reuse the HAL for Android
> > OS without proprietary modifications regarding to the follows(kind of hacky).
> > 
> > 4 FxOS-specific indications to HAL(HWC libs from chip vendor) 
> > hwcLayer.compositionType = HWC_USE_COPYBIT;    // indicate this layer needs
> > HWC
> > hwcLayer.flags |= HWC_FORMAT_RB_SWAP;          // indicate HWC to swap R/B
> > channel while composing
> > hwcLayer.flags |= HWC_COLOR_FILL;              // indicate it's a color layer
> > hwcLayer.transform = colorLayer->GetColor().Packed(); // For color layer, it
> > indicates pixel color
> > 
> > Our certain OEM partner can't push their chip vendor conform B2G's
> > HWComposer requirements.
> > I'm afraid this won't be a special case after all we may meet more and more
> > different platforms.
> > 
> Right now B2G HWC is using copybit on qualcomm platform. I think it is
> reasonable to support HWC with HWC_OVERLAY type.
> 
> > This bug is to discuss if any chance to make B2G HWComposer totally
> > compatible with Android's.
> > Here are my comments.
> > 1. Can we replace ColorLayer by ThebesLayer ?
> 
> For using HWC with HWC_OVERLAY, we will have layer number limitation, like
> at most three layers. No sure we could do optimize ColorLayer without
> creating another layer with gralloc buffer.
> 

Layer number limitation depends on chip/platform. There's CopyBit device in Qcom which has  no layer number limitation in theorem. I think B2G is better to use HWC_OVERLAY always while Qcom's HAL decides to leverage CopyBit device or not depends on numHwLayers.

What if we hack ColorLayer in Hwcomposer2D ? Just creating a graphic buffer to pretend it's not ColorLayer. Then performance issue would be an open question.


> > 2. HWC HAL will cast buffer_handle_t to private_handle_t and get the format.
> >    Lots of HAL_PIXEL_FORMAT_xxxx_xxx enumerations were defined in
> > <hardware/hardware.h>.
> >    Is HWC_FORMAT_RB_SWAP flag necessary ?
> 
> We need the FORMAT_RB_SWAP because FxOS not only output RGBX/RGBA but also
> BGRX/BGRA data. But Android only supports RGB format.

Android defines PIXEL_FORMAT_RGBX_8888 while skips PIXEL_FORMAT_BGRX_8888.
So B2G defines HWC_FORMAT_RB_SWAP flag to workaround the follows ?

PIXEL_FORMAT_BGRX_8888 = PIXEL_FORMAT_RGBX_8888 plus HWC_FORMAT_RB_SWAP

> 
> > 3. HWC_USE_OVERLAY is the Android-defined indication to do HW Composition
> > while HWC_USE_COPYBIT is not defined.
> >    I would suggest to leave the platform-specific design to vendor's HAL.
> > 4. The HWC policy implemented here is platform-specific.
> > http://mxr.mozilla.org/mozilla-central/source/widget/gonk/HwcComposer2D.
> > cpp#484
> >    I would suggest to leave it to vendor's HAL. (by calling mHwc->prepare())
> > 
> 
> I think it is better to describe which kind of HWC policy need to discuss.

4-th comment is my bad.
PrepareLayerList() implementation is kind of framework side filtering.
What I'm concerned about is ...
Without calling mHwc->prepare() to determine what composition steps the HWC can handle, HAL side may have no chance to determine some states. You can leave my concern until B2G builds on more chips and something wrong really happens.

> 
> > Simply speaking, B2G gonk for HWComposer can be built on top of HAL and
> > ideally reuse the one for Android.
> > Attached *.gif  is the sequence diagram of Android's HWComposer referenced
> > to AOSP.
> 
> Loop Diego to comment support HWC with HWC_OVERLAY type.

Comment 5

[Vincent Lin[:vilin]]

(In reply to Andreas Gal :gal from comment #3)
> 1. ColorLayer is much faster than ThebesLayer. However, we should detect
> that ColorLayers are not supported and then fall back to ThebesLayer. We can
> have both. Fast composition of QC hardware, and work everywhere. Please
> detect this dynamically.
> 
> 2. Is HWC_FORMAT_RB_SWAP flag necessary ? Detect QC hardware and support for
> RB_SWAP and then use it (dynamically), otherwise swap on the CPU (fallback).
> 
> Lets lobby chipset vendors to support these extensions but we can support
> HWC that doesn't have them. Will that work?

The idea will work.
Then we may have to hack the Android-defined interface to HAL.
There're still 4 reserved function pointer in Android 4.3.

    /*
     * Reserved for future use. Must be NULL.
     */
    void* reserved_proc[4];

} hwc_composer_device_1_t;

Any concern from HAL side ?

Comment 6

[u459114]

> 4-th comment is my bad.
> PrepareLayerList() implementation is kind of framework side filtering.
> What I'm concerned about is ...
> Without calling mHwc->prepare() to determine what composition steps the HWC
> can handle, HAL side may have no chance to determine some states. You can
> leave my concern until B2G builds on more chips and something wrong really
> happens.
hwc_composer_device.set() may, or may not, call prepare() implicitly. Explicit call prepare() before set() make sense to me

Comment 7

[u459114]

> 2. Is HWC_FORMAT_RB_SWAP flag necessary ? Detect QC hardware and support for
> RB_SWAP and then use it (dynamically), otherwise swap on the CPU (fallback).
If HWComposer does not support RB_SWAP, fallback GLComposer is a better choice, compare to CPU solution. With CPU fallback, you need to convert BGR buffer into _another_ RGB buffer. Even if SIMD is cheap on performance, more memory need is what I concern.

Comment 8

[u459114]

Here is what I learn from these comment
1. Query HWC the ability of BGR convertion
   Fallback chain need. HWC -> GPU or CPU solution.
   A mechanism to query this ability from HWC
2. Query HWC the ability of single color layer 
   Fallback chain need. CPU soltuion need in HWC.
   A mechanism to query this ability from HWC
3. Enable COPYBIT inside driver instead of add a platform dependent enumeration in framework.

Comment 9

[Diego Wilson [:diego]]

(In reply to C.J. Ku[:CJKu] from comment #8)
> Here is what I learn from these comment
> 1. Query HWC the ability of BGR convertion
>    Fallback chain need. HWC -> GPU or CPU solution.
>    A mechanism to query this ability from HWC
> 2. Query HWC the ability of single color layer 
>    Fallback chain need. CPU soltuion need in HWC.
>    A mechanism to query this ability from HWC
> 3. Enable COPYBIT inside driver instead of add a platform dependent
> enumeration in framework.

I agree with (1) and (2). As a far as (3) goes, we are working on a JB implementation and I believe copybit will go away, so it may be a non-issue.

Comment 10

[Peter Chang[:pchang]]

(In reply to Diego Wilson [:diego] from comment #9)
> (In reply to C.J. Ku[:CJKu] from comment #8)
> > Here is what I learn from these comment
> > 1. Query HWC the ability of BGR convertion
> >    Fallback chain need. HWC -> GPU or CPU solution.
> >    A mechanism to query this ability from HWC
> > 2. Query HWC the ability of single color layer 
> >    Fallback chain need. CPU soltuion need in HWC.
> >    A mechanism to query this ability from HWC

For item 1 and 2, I think we could query through hwc_query API. If the query result is "not support", we will use GPU composition by default.
 
https://android.googlesource.com/platform/hardware/qcom/display/+/android-4.3_r2/libhwcomposer/hwc.cpp Line 329.

For the new color layer handling proposed by gal in comment 3, I will open another bug for it.

> > 3. Enable COPYBIT inside driver instead of add a platform dependent
> > enumeration in framework.
> 
> I agree with (1) and (2). As a far as (3) goes, we are working on a JB
> implementation and I believe copybit will go away, so it may be a non-issue.
Do we have bug for item 3?

And we also need the bug for adding hwc_prepare on b2g.

Comment 11

[Diego Wilson [:diego]]

(In reply to peter chang[:pchang] from comment #10)
> For item 1 and 2, I think we could query through hwc_query API. If the query
> result is "not support", we will use GPU composition by default.

SGTM. We'll need to coordinate to add new support flags to gecko and libhwcomposer implementations.

> Do we have bug for item 3?

I just filed bug 902954 for JB Gonk HWC

Comment 12

[Vincent Lin[:vilin]]

Thanks to all professionals.

As there're so many dependencies between HWComposer's Bugs, here I summarize them simply in order to avoid missing something or redundant work.

All Bugs is supposed to be fixed by Bug 896765(HAL side) and Bug 902954(B2G side). 
The Output will be
1. HW composition will be enabled for FxOS 1.2 with Android 4.3.
2. Compatible to Android's HWComposer interface and move FxOS proprietary part to extension.

Bug 900827 - (HWComposer) [B2G] HWComposer integration issues
    HWComposer compatibility issue discussion
Bug 896765 - Prepare HWComposer for JB Gonk implementation
    HAL of HW composition implementation
Bug 901978 - [B2G]Query the capability of HWC about BRG format and color layer support 
Bug 902831 - [B2G] HwcComposer2D is necessary to be upgraded for HWC in JB
Bug 902954 - Enable HwcComposer2D in JB Gonk (dup 902831)
    Framework(B2G side) of HW composition implementation

Legacy issues
Bug 886415 - Add format Swap flag to libcopybit
Bug 899798 - [B2G] With Skia/GL, canvases get re-composited with B and R channels swapped
    Will be covered by Bug 896765 and Bug 902954 incidentally.

Comment 13

[Michael Vines [:m1] [:evilmachines]]

(In reply to vlin from comment #12)
> 2. Compatible to Android's HWComposer interface and move FxOS proprietary
> part to extension.

What "proprietary part"?  All the CAF HWC code is open source.

Comment 14

[Vincent Lin[:vilin]]

(In reply to Michael Vines [:m1] [:evilmachines] from comment #13)
> (In reply to vlin from comment #12)
> > 2. Compatible to Android's HWComposer interface and move FxOS proprietary
> > part to extension.
> 
> What "proprietary part"?  All the CAF HWC code is open source.

"proprietary part" => "FxOS-specific part"

4 FxOS-specific indications to HAL currently
hwcLayer.compositionType = HWC_USE_COPYBIT;    // indicate this layer needs
HWC
hwcLayer.flags |= HWC_FORMAT_RB_SWAP;          // indicate HWC to swap R/B
channel while composing
hwcLayer.flags |= HWC_COLOR_FILL;              // indicate it's a color layer
hwcLayer.transform = colorLayer->GetColor().Packed(); // For color layer, it
indicates pixel color

With FxOS-defined "HWC_USE_COPYBIT, FxOS HWComposer will never work with Android-compatible HAL library.

If they're move to extension, even one Android-compatible HAL doesn't support the extension functions it's supposed to be able to compose couple layers without ColorLayer, R/B swapped layers.

Comment 15

[Diego Wilson [:diego]]

(In reply to vlin from comment #12)
> All Bugs is supposed to be fixed by Bug 896765(HAL side) and Bug 902954(B2G
> side).

Wait, bug 896765 will not solve all the issues you mentioned on the HAL side. It is only for splitting off a common base for HWC on ICS and JB.

Comment 16

[Florian Bender]

What's the status here?