901978 - [B2G]Query the capability of HWC about BRG format and color layer support

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Peter Chang[:pchang]]

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.

https://bugzilla.mozilla.org/show_bug.cgi?id=900827#c10

Comment 1

[Andreas Gal :gal]

When we can't use color layers, we should allocate a small (if gralloc allows it, 1x1) buffer and fill it with that color and then transform it to the size we need.

Comment 2

[Peter Chang[:pchang]]

(In reply to Andreas Gal :gal from comment #1)
> When we can't use color layers, we should allocate a small (if gralloc
> allows it, 1x1) buffer and fill it with that color and then transform it to
> the size we need.

Does it mean we pass the small gralloc of color layer to HWC?
As I knew, HWC usually supports limited layer numbers for composition, like 3 or 4 layers(copybit supports more). We will waste one HWC composition layer.

And the scale capability of HWC is limited to 4x or 8x, therefore too small gralloc is not enough.

How about combine colorlayer and thebeslayer during layout process, like clear thebeslayer with some color first? But need to make sure we have the same size thebeslayer to replace color layer.

Comment 3

[Andreas Gal :gal]

I don't think thats worth it. Painting a color with the CPU is actually pretty expensive in comparison to the shader, so it might be worth to use the GPU. If we generally disable color layers, just because HWC doesn't support them, that we might accidentally also lose a GPU optimization. Its somewhat of a catch 22.

Comment 4

[Andreas Gal :gal]

Looking at the QC implementation of HWC via copybit I think it can do arbitrary scaling. What HWC implementations are limited to 4x and 8x?

Comment 5

[Peter Chang[:pchang]]

(In reply to Andreas Gal :gal from comment #4)
> Looking at the QC implementation of HWC via copybit I think it can do
> arbitrary scaling. What HWC implementations are limited to 4x and 8x?

QCT platform could use MDP or copybit for HWC composition.
For Tegra3 or other platforms, they may only had MDP-like HWC composition module.

For MDP-like 2D modules, they have the layer numbers and up/down scale limitation.

For example, QCT MDP has the up/down scale limitation.
You can see MDP up scale limitation from below file.

#define MDP4_REV40_UP_SCALING_MAX (8)
#define MDP4_REV41_OR_LATER_UP_SCALING_MAX (20)

https://android.googlesource.com/kernel/msm.git/+/android-msm-mako-3.4-jb-mr2/drivers/video/msm/mdp4.h

Agree with you about waste time to combine colorlayer and thebeslayer in comment 3.

To handle the color layer on platform which only has MDP-like module, how about reduce the occurrence of colorlayer if we have the same size of thebeslayer and above this color layer?
If above condition couldn't meet, we will fallback to GPU composition.

[LayerTree dump of youtube]
In the following example, we can replace colorlayer with ThebesLayerComposite 0x487b4c00.

aLayer 0x487b3c00 name ContainerLayer fmt 0 vis(0 0 320 480) sf(0 0) visReg xxx offset0 (0 0) opacity 1.00
  aLayer 0x487b4400 name ThebesLayerComposite fmt 0 vis(0 0 0 0) sf(0 0) visReg xxx offset0 (0 0) opacity 1.00
  aLayer 0x487b5000 name ColorLayer fmt 0 vis(0 0 0 0) sf(0 0) visReg xxx offset0 (0 0) opacity 1.00
  aLayer 0x487b4800 name ContainerLayer fmt 0 vis(0 0 320 480) sf(0 0) visReg xxx offset0 (0 0) opacity 1.00
    aLayer 0x487b4c00 name ThebesLayerComposite fmt 4 vis(0 0 320 480) sf(320 480) visReg xxx offset1 (0 0) opacity 1.00
    aLayer 0x488c1000 name RefLayer fmt 0 vis(0 0 320 460) sf(0 0) visReg xxx offset0 (0 0) opacity 1.00
      aLayer 0x48860800 name ContainerLayer fmt 0 vis(0 0 320 460) sf(0 0) visReg xxx offset0 (0 0) opacity 1.00
        aLayer 0x488bec00 name ThebesLayerComposite fmt 1 vis(0 0 0 0) sf(320 460) visReg xxx offset1 (0 0) opacity 1.00
        aLayer 0x488c3400 name ContainerLayer fmt 0 vis(0 0 320 180) sf(0 0) visReg xxx offset0 (0 0) opacity 1.00
          aLayer 0x48bbc400 name ImageLayer fmt 266 vis(0 0 640 360) sf(640 360) visReg xxx offset0 (0 0) opacity 1.00
    aLayer 0x44a27c00 name CanvasLayer fmt 0 vis(0 0 16 16) sf(0 0) visReg xxx offset0 (0 0) opacity 1.00

Comment 6

[Andreas Gal :gal]

Peter, the API doesn't seem to offer a way to limit whats supported:

            /* area of the source to consider, the origin is the top-left corner of                                                                                             
             * the buffer */
            hwc_rect_t sourceCrop;

            /* where to composite the sourceCrop onto the display. The sourceCrop                                                                                               
             * is scaled using linear filtering to the displayFrame. The origin is the                                                                                          
             * top-left corner of the screen.                                                                                                                                   
             */
            hwc_rect_t displayFrame;

This will work and will be fast on QCT chips right? Because they can use copybit. Platforms that use an MDP-like processor to composite still have to handle the general case here somehow. What does tegra do? Do you know?

Comment 7

[Andreas Gal :gal]

Do you mean that HWC will refuse to composite the layer and set it to HWC_FRAMEBUFFER if its not 4x and 8x scaling?

Comment 8

[Peter Chang[:pchang]]

(In reply to Andreas Gal :gal from comment #7)
> Do you mean that HWC will refuse to composite the layer and set it to
> HWC_FRAMEBUFFER if its not 4x and 8x scaling?

If you are using MDP-like component, the driver will return error and fallback to GPU I think. For tegra3 platform, they only have MDP-like component which also has the scaling limitation.

Diego, let us know your comment about the MDP scaling capability.
       Do we prefer to use copybit first on all QCT platform?

Comment 9

[Diego Wilson [:diego]]

(In reply to peter chang[:pchang] from comment #8)
> (In reply to Andreas Gal :gal from comment #7)
> > Do you mean that HWC will refuse to composite the layer and set it to
> > HWC_FRAMEBUFFER if its not 4x and 8x scaling?
> 
> If you are using MDP-like component, the driver will return error and
> fallback to GPU I think. For tegra3 platform, they only have MDP-like
> component which also has the scaling limitation.
> 
> Diego, let us know your comment about the MDP scaling capability.
>        Do we prefer to use copybit first on all QCT platform?

As Andreas pointed out, this is kind of a moot point since we know HWC on QCT chips will support the HWC_FOLOR_FILL flag.

Comment 10

[Vincent Lin[:vilin]]

MDP-like composer is layer-limited(at most 4 I've ever known) and hardly to support ColorLayer.
CopyBit-like composer is just opposite.

The worse case for us is one platform doesn't embed CopyBit-like composer and GPU composition performance is poor. I'm afraid the major problem will be the limited layer number, but not dealing with ColorLayer. In general, B2G's UI layers are more than Android's UI layers.

If one platform embedded CopyBit-like composer, we just need to support our partners or the vendors to implement the extension functions for B2G.

By the way...
"Scaling" and "layer number" are platform-dependent.
As I know, they're still some platforms embedded CopyBit-like HWC.
It composes layers to a target layer-by-layer. No layer number limitation, but performance depends on total area of layers.

I know a platform even doesn't support scaling. Then mHwc->prepare() will set the layer's composition type to HWC_FRAMEBUFFER if sourceCrop is not equal to displayFrame.

I think what we can do first is to define a well-designed extension interface(additional enum to current interface) to HAL.

Comment 11

[Vincent Lin[:vilin]]

Attachment: query_hwc_ext_func.patch

I think we have to fix this bug based on patches of Bug 896765 and Bug 902831(Attachment #800008 [details] [diff]). This attachment is the extension function for query ColorLayer and R/B swap.

The rest importance is the HAL implementation. Without the support from HAL, composition is hardly handled by HWComposer even the TryRender() path is enabled. This is because software rendering always needs R/B swap here.
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp#425

Comment 12

[Peter Chang[:pchang]]

Comment on attachment 800014 [details] [diff] [review]
query_hwc_ext_func.patch

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

::: widget/gonk/HwcComposerJB.cpp
@@ +281,5 @@
> +	         hwcLayer.flags |= HwcUtils::HWC_FORMAT_RB_SWAP;
> +	     else {
> +	         LOGD("R/B swap is not supported");
> +		  return false;
> +	     }

If we don't support RBSwap or colorfill feature, why just skip HWC composition checking?
Or you still want to use HWC when no RBSwap and color layers.

Comment 13

[Diego Wilson [:diego]]

Comment on attachment 800014 [details] [diff] [review]
query_hwc_ext_func.patch

LGTM.

@Sushil We'll need to support these new queries in libhwcomposer

Comment 14

[Sushil]

Yes, we will add support for these 2 queries once HwcComposerJB lands in Gecko.

Comment 15

[Sushil]

@vlin: R/B swap support in display HAL has already landed in CAF. To test, you can set 'mRBswapSupport' to 'true' always in your patch. We will add these 2 queries in hwc.query, once HwcComposerJB lands in Gecko.

Comment 16

[Vincent Lin[:vilin]]

(In reply to peter chang[:pchang] from comment #12)
> Comment on attachment 800014 [details] [diff] [review]
> query_hwc_ext_func.patch
> 
> Review of attachment 800014 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/gonk/HwcComposerJB.cpp
> @@ +281,5 @@
> > +	         hwcLayer.flags |= HwcUtils::HWC_FORMAT_RB_SWAP;
> > +	     else {
> > +	         LOGD("R/B swap is not supported");
> > +		  return false;
> > +	     }
> 
> If we don't support RBSwap or colorfill feature, why just skip HWC
> composition checking?
> Or you still want to use HWC when no RBSwap and color layers.

I just thought not all scenarios needs RBSwap or ColorLayers, so made it run-timely switched.
But according to my investigation, RBSwap is frequently necessary.

Anyway, the patch is just a sample code and for efficient discussion.

Comment 17

[Sushil]

Attachment: Query HWC hal to check for "R/B swap" and Color layer support, at run-time.

Re-based the patch on tip. Framework can query HWC hal to check for "R/B swap"  and Color layer support, at run-time. As every other frame requires R/B swap, so if there is no R/B swap support in HAL, then it is better to use GPU Composition by default.

Comment 18

[Diego Wilson [:diego]]

Comment on attachment 807465 [details] [diff] [review]
Query HWC hal to check for "R/B swap"  and Color layer support, at run-time.

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

::: widget/gonk/HwcComposer2D.cpp
@@ +82,3 @@
>      }
>  
>      nsIntSize screenSize;

Nit: please remove this superfluous new line

@@ +97,5 @@
>      mColorFill = (atoi(propValue) == 1) ? true : false;
> +    mRBSwap = true;
> +#endif
> +
> +    if (!mRBSwap) {

Only bail if state.FormatRBSwapped() is true and rb swap is unsupported.

Comment 19

[Sushil]

Attachment: Query HWC hal to check for R/B swap and Color layer support.

Updated the patch with review comments addressed.

Comment 20

[Diego Wilson [:diego]]

Comment on attachment 809455 [details] [diff] [review]
Query HWC hal to check for R/B swap and Color layer support.

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

r=me with nits picked

::: widget/gonk/HwcComposer2D.cpp
@@ +84,3 @@
>          LOGE("Failed to initialize hwc");
>          return -1;
>      }

Nit: please undo the unnecessary line change

@@ +87,1 @@
>      nsIntSize screenSize;

Nit: please undo this unnecessary line change

Comment 21

[Sushil]

Attachment: Query HWC hal to check for RB swap and Color layer support.

Last comments addressed.

Comment 22

[Nick Cameron [:nrc]]

Comment on attachment 809455 [details] [diff] [review]
Query HWC hal to check for R/B swap and Color layer support.

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

R=me with nits addressed

::: widget/gonk/HwcComposer2D.cpp
@@ +65,5 @@
>      , mList(nullptr)
>      , mHwc(nullptr)
>      , mHwcPrepared(false)
> +    , mRBSwap(false)
> +    , mColorFill(false)

wrong order, please swap

@@ +89,5 @@
>      mScreenRect  = nsIntRect(nsIntPoint(0, 0), screenSize);
>  
> +#if ANDROID_VERSION >= 18
> +    int supported = 0;
> +    if (mHwc->query(mHwc, HwcUtils::HWC_COLOR_FILL, &supported) == NO_ERROR)

use braces

@@ +91,5 @@
> +#if ANDROID_VERSION >= 18
> +    int supported = 0;
> +    if (mHwc->query(mHwc, HwcUtils::HWC_COLOR_FILL, &supported) == NO_ERROR)
> +        mColorFill = supported ? true : false;
> +    if (mHwc->query(mHwc, HwcUtils::HWC_FORMAT_RB_SWAP, &supported) == NO_ERROR)

use braces

::: widget/gonk/HwcComposer2D.h
@@ +77,4 @@
>      nsIntRect               mScreenRect;
>      int                     mMaxLayerCount;
>      bool                    mColorFill;
> +    bool                    mRBSwap;

Please rename to mRBSwapSupported or something similar

Comment 23

[Peter Chang[:pchang]]

(In reply to Sushil from comment #21)
> Created attachment 809550 [details] [diff] [review]
> Query HWC hal to check for RB swap and Color layer support.
> 
> Last comments addressed.

You can re-upload attachment 809455 [details] [diff] [review] with updated version and carry with reviewer comment, like r=diego
No need to create another attachment for review.

Comment 24

[Sushil]

Attachment: Query HWC to check for RB swap and Color layer support.

Bug 901978 - Query HWC to check for "R/B swap" and Color layer support. r=ncameron

Comment 25

[Diego Wilson [:diego]]

Attachment: Query-HWC-to-check-for-RB-swap-and-Color-layer-support.patch

Reformat patch to HG

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/9c47f46943ff

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9c47f46943ff