If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in 1.3 Sprint 2 - 10/11

Status

Firefox OS
General
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: pchang, Assigned: diego)

Tracking

(Depends on: 1 bug)

unspecified
1.3 Sprint 2 - 10/11
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [TPE_GFX])

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

4 years ago
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
(Reporter)

Updated

4 years ago
Blocks: 900827
(Reporter)

Updated

4 years ago
Blocks: 896765

Updated

4 years ago
Blocks: 902831

Updated

4 years ago
Whiteboard: [TPE_GFX]

Comment 1

4 years ago
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.
(Reporter)

Comment 2

4 years ago
(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

4 years ago
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

4 years ago
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?
(Reporter)

Comment 5

4 years ago
(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

4 years ago
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

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

Comment 8

4 years ago
(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?
Flags: needinfo?(dwilson)
(Assignee)

Updated

4 years ago
Flags: needinfo?(dwilson) → needinfo?(sushilchauhan)
(Assignee)

Comment 9

4 years ago
(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.
Flags: needinfo?(sushilchauhan)

Comment 10

4 years ago
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.
(Reporter)

Updated

4 years ago
Depends on: 912373

Comment 11

4 years ago
Created attachment 800014 [details] [diff] [review]
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
Attachment #800014 - Flags: review?(pchang)
Attachment #800014 - Flags: review?(mtseng)
Attachment #800014 - Flags: review?(dwilson)
Flags: needinfo?(dwilson)
(Reporter)

Comment 12

4 years ago
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.
(Assignee)

Comment 13

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

LGTM.

@Sushil We'll need to support these new queries in libhwcomposer
Attachment #800014 - Flags: review?(dwilson) → review?(sushilchauhan)
(Assignee)

Updated

4 years ago
Flags: needinfo?(dwilson)

Comment 14

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

Comment 15

4 years ago
@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

4 years ago
(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

4 years ago
Created attachment 807465 [details] [diff] [review]
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.
Attachment #807465 - Flags: review?(pchang)
Attachment #807465 - Flags: review?(mtseng)
Attachment #807465 - Flags: review?(dwilson)
(Assignee)

Comment 18

4 years ago
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.
Attachment #807465 - Flags: review?(dwilson)

Comment 19

4 years ago
Created attachment 809455 [details] [diff] [review]
Query HWC hal to check for R/B swap and Color layer support.

Updated the patch with review comments addressed.
Attachment #807465 - Attachment is obsolete: true
Attachment #807465 - Flags: review?(pchang)
Attachment #807465 - Flags: review?(mtseng)
Attachment #809455 - Flags: review?(pchang)
Attachment #809455 - Flags: review?(mtseng)
Attachment #809455 - Flags: review?(dwilson)
(Assignee)

Comment 20

4 years ago
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
Attachment #809455 - Flags: review?(dwilson) → review+

Comment 21

4 years ago
Created attachment 809550 [details] [diff] [review]
Query HWC hal to check for RB swap and Color layer support.

Last comments addressed.
Attachment #809550 - Flags: review?(pchang)
Attachment #809550 - Flags: review?(dwilson)

Updated

4 years ago
Depends on: 919676
(Reporter)

Updated

4 years ago
Attachment #809455 - Flags: review?(pchang) → review+
(Reporter)

Updated

4 years ago
Attachment #809455 - Flags: review?(ncameron)
Attachment #809455 - Flags: review?(mtseng)
Attachment #809455 - Flags: review-
(Reporter)

Updated

4 years ago
Attachment #809455 - Flags: review-

Comment 22

4 years ago
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
Attachment #809455 - Flags: review?(ncameron) → review+
(Reporter)

Comment 23

4 years ago
(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.
(Assignee)

Updated

4 years ago
Attachment #809550 - Flags: review?(dwilson)

Updated

4 years ago
No longer depends on: 919676

Comment 24

4 years ago
Created attachment 811317 [details] [diff] [review]
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
Attachment #809455 - Attachment is obsolete: true
Attachment #809550 - Attachment is obsolete: true
Attachment #809550 - Flags: review?(pchang)
Attachment #811317 - Flags: review+
(Assignee)

Comment 25

4 years ago
Created attachment 811355 [details] [diff] [review]
Query-HWC-to-check-for-RB-swap-and-Color-layer-support.patch

Reformat patch to HG
Attachment #800014 - Attachment is obsolete: true
Attachment #811317 - Attachment is obsolete: true
Attachment #800014 - Flags: review?(sushilchauhan)
Attachment #800014 - Flags: review?(pchang)
Attachment #800014 - Flags: review?(mtseng)
Attachment #811355 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/b2g-inbound/rev/9c47f46943ff
Assignee: nobody → dwilson
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9c47f46943ff
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.3 Sprint 2 - 10/11
You need to log in before you can comment on or make changes to this bug.