Closed Bug 1144012 Opened 5 years ago Closed 5 years ago

Wrap HwcDevice from HwcComposer2D

Categories

(Core Graveyard :: Widget: Gonk, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(firefox41 fixed)

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: boris, Assigned: boris)

References

Details

Attachments

(4 files, 85 obsolete files)

16.62 KB, patch
boris
: review+
Details | Diff | Splinter Review
19.92 KB, patch
boris
: review+
Details | Diff | Splinter Review
12.43 KB, patch
boris
: review+
Details | Diff | Splinter Review
2.87 KB, patch
boris
: review+
Details | Diff | Splinter Review
Use another class, ex. HwcHAL, to wrap HAL specific code, so we can make the code more concise in HwcComposer2D.
A simple idea is: 
1. ICS has a specific class which is inherited from HwcHAL.
2. JB/KK/L/others also has a class which is inherited from HwcHAL.
We put the common implementation into HwcHAL, and the specific implementation for each version into the corresponding class.
Blocks: RefactorHwc
Use HwcHAL as a base class which has two subclasses, HwcICS and HwcJB.
1. HwcICS for Android ICS
2. HwcJB for Android JB/KK/L.
Attachment #8579174 - Attachment is obsolete: true
Attachment #8579262 - Flags: feedback?(pchang)
Attachment #8579262 - Flags: feedback?(hshih)
Comment on attachment 8579262 [details] [diff] [review]
[WIP] Wrap HwcHAL from HwcComposer2D (v0.3)

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

::: widget/gonk/HwcComposer2D.cpp
@@ +136,5 @@
>  HwcComposer2D::Init(hwc_display_t dpy, hwc_surface_t sur, gl::GLContext* aGLContext)
>  {
>      MOZ_ASSERT(!Initialized());
>  
> +    mHal = HwcHAL::GetHwcHAL();

I think we can allocate different HwcHAL here and keep mHal inside HwcComposer2D

@@ +761,5 @@
>      return true;
>  }
> +#else
> +bool
> +HwcComposer2D::TryHwComposition()

Please refactor TryHwComposition and Render too.

::: widget/gonk/HwcComposer2D.h
@@ +55,5 @@
>  #else
>  typedef hwc_composer_device_t HwcDevice;
>  typedef hwc_layer_list_t HwcList;
>  typedef hwc_layer_t HwcLayer;
> +#endif*/

Remove this code.

::: widget/gonk/hwchal/HwcHAL.cpp
@@ +32,5 @@
> +
> +bool
> +HwcHAL::Init(HwcDevice* aHwc)
> +{
> +    mHwc = aHwc;

What happen if mHwc is nullptr?
Please consider the error handling for mHwc

@@ +37,5 @@
> +    return mHwc;
> +}
> +
> +bool
> +HwcHAL::Query(QueryType aType)

Since you don't have implementation for Query/Set/Prepare/Commit, suggest to declare as pure virtual functions.

::: widget/gonk/hwchal/HwcICS.cpp
@@ +21,5 @@
> +#include "cutils/properties.h"
> +
> +namespace mozilla {
> +
> +static HwcICS* sHwcHAL = nullptr;

It's not clear for me to maintain sHwcHAL inside hal. We could keep it in HwcComposer2D

::: widget/gonk/hwchal/HwcJB.cpp
@@ +22,5 @@
> +#include "mozilla/layers/TextureHostOGL.h"
> +
> +namespace mozilla {
> +
> +static HwcJB* sHwcHAL = nullptr;

Same as the question I mentioned for HwcICS

@@ +61,5 @@
> +}
> +
> +int
> +HwcJB::Set(HwcList* aList, hwc_display_t aDpy, hwc_surface_t aSur)
> +{

Please consider the error handling for empty mHwc. I think you can check it before allocation the HwcHAL.

@@ +62,5 @@
> +
> +int
> +HwcJB::Set(HwcList* aList, hwc_display_t aDpy, hwc_surface_t aSur)
> +{
> +    hwc_display_contents_1_t *displays[HWC_NUM_DISPLAY_TYPES] = { nullptr };

s/HwcList/hwc_display_contents_1_t

@@ +75,5 @@
> +               int aFence)
> +{
> +    int idx = aList->numHwLayers - 1;
> +    const hwc_rect_t r = {0, 0, aScreenRect.width, aScreenRect.height};
> +    hwc_display_contents_1_t *displays[HWC_NUM_DISPLAY_TYPES] = { nullptr };

s/HwcList/hwc_display_contents_1_t

@@ +81,5 @@
> +
> +    aList->outbufAcquireFenceFd = -1;
> +    aList->outbuf = nullptr;
> +    aList->retireFenceFd = -1;
> +

Please move below 'hard code' configuration of hwLayers into InitHwcLayer.

@@ +88,5 @@
> +    aList->hwLayers[idx].transform = 0;
> +    aList->hwLayers[idx].handle = aFbHandle;
> +    aList->hwLayers[idx].blending = HWC_BLENDING_PREMULT;
> +    aList->hwLayers[idx].compositionType = HWC_FRAMEBUFFER_TARGET;
> +    SetCrop(aList->hwLayers[idx], r);

We might not need a separate function for SetCrop.

@@ +91,5 @@
> +    aList->hwLayers[idx].compositionType = HWC_FRAMEBUFFER_TARGET;
> +    SetCrop(aList->hwLayers[idx], r);
> +    aList->hwLayers[idx].displayFrame = r;
> +    aList->hwLayers[idx].visibleRegionScreen.numRects = 1;
> +    aList->hwLayers[idx].visibleRegionScreen.rects = &aList->hwLayers[idx].displayFrame;

set aList->hwLayers[idx].visibleRegionScreen.rects = r

@@ +104,5 @@
> +int
> +HwcJB::Commit(HwcList* aList,
> +              LayerMap& aMap)
> +{
> +    hwc_display_contents_1_t *displays[HWC_NUM_DISPLAY_TYPES] = { nullptr };

s/HwcList/hwc_display_contents_1_t

@@ +113,5 @@
> +        if (aMap.IsEmpty() ||
> +            (aList->hwLayers[j].compositionType == HWC_FRAMEBUFFER)) {
> +            continue;
> +        }
> +        layers::LayerRenderState state = aMap[j]->GetLayer()->GetRenderState();

Do we need to put these code inside HwcHAL? It looks like more gecko's logic.

@@ +158,5 @@
> +    return err;
> +}
> +
> +void
> +HwcJB::FillHwcLayer(HwcLayer& aLayer,

s/FillHwcLayer/InitHwcLayer
Attachment #8579262 - Flags: feedback?(pchang)
Comment on attachment 8579262 [details] [diff] [review]
[WIP] Wrap HwcHAL from HwcComposer2D (v0.3)

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

::: widget/gonk/HwcComposer2D.cpp
@@ +136,5 @@
>  HwcComposer2D::Init(hwc_display_t dpy, hwc_surface_t sur, gl::GLContext* aGLContext)
>  {
>      MOZ_ASSERT(!Initialized());
>  
> +    mHal = HwcHAL::GetHwcHAL();

I'm not sure the comment Peter said, but I think we don't need to call "GetGonkDisplay()->GetHWCDevice()" in this file. Move to HwcHAL.

::: widget/gonk/hwchal/HwcJB.cpp
@@ +91,5 @@
> +    aList->hwLayers[idx].compositionType = HWC_FRAMEBUFFER_TARGET;
> +    SetCrop(aList->hwLayers[idx], r);
> +    aList->hwLayers[idx].displayFrame = r;
> +    aList->hwLayers[idx].visibleRegionScreen.numRects = 1;
> +    aList->hwLayers[idx].visibleRegionScreen.rects = &aList->hwLayers[idx].displayFrame;

I'm not sure it's safe that we pass a local variable's address into list. It's fine for hwc->prepare(), but how about the hwc->set() function? Maybe hwc needs to read the rect in set function.

@@ +113,5 @@
> +        if (aMap.IsEmpty() ||
> +            (aList->hwLayers[j].compositionType == HWC_FRAMEBUFFER)) {
> +            continue;
> +        }
> +        layers::LayerRenderState state = aMap[j]->GetLayer()->GetRenderState();

ICS doesn't have fence. If these go back to HwcComposer2D, that means we still have android version checking for these codes. Peter, is it still worth to do?
Attachment #8579262 - Flags: feedback?(hshih)
Use HwcHAL as a base class which has two subclasses, HwcICS and HwcJB.
1. HwcICS for Android ICS
2. HwcJB for Android JB/KK/L.
Attachment #8586640 - Attachment is obsolete: true
Attachment #8586695 - Attachment is obsolete: true
Attachment #8588951 - Flags: feedback?(pchang)
Attachment #8588951 - Flags: feedback?(hshih)
Comment on attachment 8588951 [details] [diff] [review]
[WIP] Wrap HwcHAL from HwcComposer2D (v0.6)

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

Can you draw the UML for HWComposer2D and HAL? And then we have more clear picture about HWC.

::: widget/gonk/HwcComposer2D.cpp
@@ +32,2 @@
>  #include "mozilla/StaticPtr.h"
> +#include "nsThreadUtils.h"

why need this header

@@ +98,5 @@
>      , mLock("mozilla.HwcComposer2D.mLock")
>  {
> +    MOZ_ASSERT(mHal->Initialized());
> +    if (!mHal->Initialized()) {
> +        LOGE("Failed to initialize hwc");

Looks like HwcComposer2D::Init already calls the mHal->Initialized()

@@ +553,3 @@
>  bool
>  HwcComposer2D::TryHwComposition()
>  {

How about the flow of ICS in this function?
It looks like your patch do more than current implementation which only calls set function.

@@ +565,5 @@
>              return false;
>          }
>      }
>  
> +    if (!Prepare(false)) {

It's hard to read the code when pass input as bool.

@@ +635,5 @@
>  
>      // BLIT or full OVERLAY Composition
>      Commit();
>  
> +    mHal->ResetFence(mList->hwLayers[mList->numHwLayers - 1], HwcHAL::REL, true);

Move to commit?

@@ +677,3 @@
>  bool
>  HwcComposer2D::Commit()
>  {

If we put mList inside mHal, then we could define android version for fence stuff.

HwcComposer2D::Commit()

#if defined(MOZ_WIDGET_GONK) && ANDROID_VERSION >= 17
for each i in mHwcLayerMap
{
  if(state && texture)
  {
    fence = texture->GetAndResetAcquireFence();
    //dup fence
    mHal.SetAcquireFence(i, fence);
  }
}
#endif
mHwc->Set()

#if defined(MOZ_WIDGET_GONK) && ANDROID_VERSION >= 17
for each i in mHwcLayerMap
{
  mHal.getreleaseFence(i, fence);
  if(state && texture)
  {   
     texture->SetReleaseFence(fence);
  }
}
#endif

@@ +695,3 @@
>      }
>  
> +    int err = mHal->Set(mList, false);

It's hard to read the code when pass input as bool.
With above pseduo code, I suppose the ICS one should call HwcComposer2D::Commit to invoke mHal->Set().

::: widget/gonk/HwcComposer2D.h
@@ -115,1 @@
>      HwcList*                mList;

Please consider to move mList into mHal

::: widget/gonk/hwchal/HwcHAL.h
@@ +41,5 @@
> +
> +namespace layers {
> +    class LayerComposite;
> +    class TextureHostOGL;
> +}

I would expect these Layer/Texture stuff should not be included inside HAL.

@@ +95,5 @@
> +    virtual int Prepare(HwcList *aList,
> +                        bool aFbFence = false) = 0;
> +
> +    virtual bool CheckSemiTransParency(uint8_t aOpacity) const = 0;
> +

s/CheckSemiTransparency/IsOpaque

@@ +100,5 @@
> +    virtual uint32_t GeometryChangeFlag(bool aGeometryChanged) const;
> +
> +    virtual bool ForceGonkSwapBuffer(const HwcList *aList) const;
> +
> +    virtual bool ForceSet() const;

Please add comment to explain ForceSet/ForceGonkSwapBuffer
Attachment #8588951 - Flags: feedback?(pchang) → feedback-
Comment on attachment 8588951 [details] [diff] [review]
[WIP] Wrap HwcHAL from HwcComposer2D (v0.6)

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

::: widget/gonk/HwcComposer2D.cpp
@@ +32,2 @@
>  #include "mozilla/StaticPtr.h"
> +#include "nsThreadUtils.h"

I move this header from line 39. I think vsync will use NS_IsMainThread(), which is included in nsThreadUtils.h.

@@ +98,5 @@
>      , mLock("mozilla.HwcComposer2D.mLock")
>  {
> +    MOZ_ASSERT(mHal->Initialized());
> +    if (!mHal->Initialized()) {
> +        LOGE("Failed to initialize hwc");

Yes. But Vsync will create HwcComposer2D object and do something before HwcComposer2D::Init(). I have to print a warning if the initialization of mHal is failed.

@@ +553,3 @@
>  bool
>  HwcComposer2D::TryHwComposition()
>  {

On ICS devices, mHal->ForceSet() is true, so it just goes into next code segment:
"return !mHal->Set(mList, mDpy, mSur);"

@@ +635,5 @@
>  
>      // BLIT or full OVERLAY Composition
>      Commit();
>  
> +    mHal->ResetFence(mList->hwLayers[mList->numHwLayers - 1], HwcHAL::REL, true);

Sure. I can move this.

::: widget/gonk/HwcComposer2D.h
@@ -115,1 @@
>      HwcList*                mList;

OK.

::: widget/gonk/hwchal/HwcHAL.h
@@ +95,5 @@
> +    virtual int Prepare(HwcList *aList,
> +                        bool aFbFence = false) = 0;
> +
> +    virtual bool CheckSemiTransParency(uint8_t aOpacity) const = 0;
> +

OK.

@@ +100,5 @@
> +    virtual uint32_t GeometryChangeFlag(bool aGeometryChanged) const;
> +
> +    virtual bool ForceGonkSwapBuffer(const HwcList *aList) const;
> +
> +    virtual bool ForceSet() const;

Sure.
Depends on: 1152135
Assignee: nobody → boris.chiou
Use HwcHAL as a base class which has two subclasses, HwcICS and HwcJB.
1. HwcICS for Android ICS
2. HwcJB for Android JB/KK/L or later.
3. mList, mHwc are wrappd into HwcHAL.
4. I tried my best to remove the "#if ANDROID_VERSION" from
   HwcComposer2D, but there is still one in Commit() function.
Attachment #8595262 - Attachment is obsolete: true
Attachment #8595266 - Flags: feedback?(pchang)
Attachment #8595266 - Flags: feedback?(hshih)
Comment on attachment 8595266 [details] [diff] [review]
Wrap HwcHAL from HwcComposer2D (v1)

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

::: widget/gonk/HwcComposer2D.cpp
@@ +202,5 @@
>          LOGD("%s Layer has zero opacity; skipping", aLayer->Name());
>          return true;
>      }
> +
> +    if (!mHal->IsOpaque(opacity)) {

I think this should be like the following logic, we don't need to pass the gecko layer logic inside mHal.

(opacity < 0xFF && !mHal->SupportTransapent())

@@ +361,2 @@
>      buffer_handle_t handle = fillColor ? nullptr : state.mSurface->getNativeBuffer()->handle;
> +    mHal->InitHwcLayer(hwcLayer, sourceCrop, displayFrame, handle, isOpaque, opacity);

From line 353 to line 362, I think we can put these into HAL. HAL will re-allocate list if need and append new hwcLayer based on the attributes from gecko layer tree.

mHal->AppendHwcLayer(sourceCrop, displayFrame...)

About the following code inside PrepareLayerLit, we could also put into HAL because it only modifies the HwcLayer.

I strong recommend you to split your patch into several parts to reduce the review and modification effort.

@@ +564,5 @@
> +                overlayComposite = false;
> +                break;
> +            }
> +        }
> +

I would like to see the code from line 545 to line 568 move into HAL, and we just call the following function. Please create a split patch to hide this composition logic inside HAL.

bool overlayComposite = mHal->IsFullOverlaySupported();

@@ +614,5 @@
>  HwcComposer2D::Render()
>  {
> +    // If HWC module does not exist or mList is not created yet,
> +    // or on ICS, we call Gonk SwapBuffers directly.
> +    if (!mHal->HwcCanRender()) {

I think the logic here should be like the following and the SupportFBDev in ICS is false.


if (!mHal->SupportFBDev() || !mHal->GetHwcList()) {
  // call swapbuffers
}

@@ +720,5 @@
>  {
>      LOGD("hwcomposer is already prepared, reset with null set");
> +    // Only ICS need mDpy and mSur
> +    // true means we will reset it by nullptr
> +    mHal->Set(mDpy, mSur, true);

Can we have diagram for ICS and JB flows for these API calls? And then we don't need to mention ICS or KK in comment.

@@ +733,5 @@
>          return false;
>      }
>  
> +    if (mHal->HwcListInited()) {
> +        HwcList *list = mHal->GetHwcList();

Make this simple to check GetHwcList() is null or not, the initialization of HwcList in your patch does nothing.

::: widget/gonk/hwchal/HwcHAL.h
@@ +88,5 @@
> +public:
> +    HwcHAL();
> +
> +    virtual ~HwcHAL();
> +

Please add description for each public function, because other modules are using them.
Comment on attachment 8595266 [details] [diff] [review]
Wrap HwcHAL from HwcComposer2D (v1)

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

::: widget/gonk/HwcComposer2D.cpp
@@ +202,5 @@
>          LOGD("%s Layer has zero opacity; skipping", aLayer->Name());
>          return true;
>      }
> +
> +    if (!mHal->IsOpaque(opacity)) {

OK. I agree.

@@ +361,2 @@
>      buffer_handle_t handle = fillColor ? nullptr : state.mSurface->getNativeBuffer()->handle;
> +    mHal->InitHwcLayer(hwcLayer, sourceCrop, displayFrame, handle, isOpaque, opacity);

Using AppendHwcLayer is a Good idea, and I will try my best to split my patch.

@@ +564,5 @@
> +                overlayComposite = false;
> +                break;
> +            }
> +        }
> +

OK.

@@ +614,5 @@
>  HwcComposer2D::Render()
>  {
> +    // If HWC module does not exist or mList is not created yet,
> +    // or on ICS, we call Gonk SwapBuffers directly.
> +    if (!mHal->HwcCanRender()) {

OK

@@ +720,5 @@
>  {
>      LOGD("hwcomposer is already prepared, reset with null set");
> +    // Only ICS need mDpy and mSur
> +    // true means we will reset it by nullptr
> +    mHal->Set(mDpy, mSur, true);

We can discuss this later.

::: widget/gonk/hwchal/HwcHAL.h
@@ +88,5 @@
> +public:
> +    HwcHAL();
> +
> +    virtual ~HwcHAL();
> +

Sure.
Use HwcHAL as a base class which has two subclasses, HwcICS and HwcJB.
1. HwcICS for Android ICS
2. HwcJB for Android JB/KK/L or later.
3. mHwc is wrappd into HwcHAL.
Attachment #8597081 - Attachment is obsolete: true
Remove #if definitions from these two function,
and just use flags to control the flow.
Attachment #8597082 - Attachment is obsolete: true
Attached patch Part 3: Wrap mList (v5) (obsolete) — Splinter Review
Wrap HwcList code into HwcHAL
Attachment #8597083 - Attachment is obsolete: true
Attached patch Part 5: Wrap vsync code (v2) (obsolete) — Splinter Review
Attached patch Part 4: Wrap fence code (v4) (obsolete) — Splinter Review
Wrap fence related code in Commit().
Attachment #8597135 - Attachment is obsolete: true
Also, use renge-based for loop in HwcComposer2D
Use comments in this version. If possible, I can draw some ASCII diagrams in
the comments.
Attachment #8597600 - Flags: feedback?(pchang)
Attachment #8597127 - Flags: feedback?(hshih)
Attachment #8597194 - Flags: feedback?(pchang)
Attachment #8597094 - Flags: feedback?(pchang)
Attachment #8597097 - Flags: feedback?(pchang)
Attachment #8597101 - Flags: feedback?(pchang)
Attachment #8597101 - Flags: feedback?(hshih)
Attachment #8597140 - Flags: feedback?(pchang)
Attachment #8597140 - Flags: feedback?(etlin)
Attachment #8597140 - Flags: feedback?(etlin)
Comment on attachment 8597094 [details] [diff] [review]
Part 1: Wrap mHwc from HwcComposer2D (v4)

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

Peter, I am unsure if I should split this patch further. Could you please check this patch first? Thanks.
Comment on attachment 8597094 [details] [diff] [review]
Part 1: Wrap mHwc from HwcComposer2D (v4)

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

::: widget/gonk/HwcComposer2D.cpp
@@ +28,5 @@
>  #include "mozilla/layers/CompositorParent.h"
>  #include "mozilla/layers/LayerManagerComposite.h"
>  #include "mozilla/layers/PLayerTransaction.h"
>  #include "mozilla/layers/ShadowLayerUtilsGralloc.h"
> +#include "mozilla/layers/TextureHostOGL.h"

no need to remove the comment

@@ +99,5 @@
>  
>  static StaticRefPtr<HwcComposer2D> sInstance;
>  
>  HwcComposer2D::HwcComposer2D()
> +    : mHal(HwcHAL::CreateHwcHAL())

Suggest to allocate mHal in line 122

@@ +590,5 @@
>                  return true;
>              }
>              region.numRects = visibleRects->size();
>              region.rects = &((*visibleRects)[0]);
> +        }else {

Undo here

@@ +708,5 @@
>  
> +    buffer_handle_t handle = nullptr;
> +    int fd = -1;
> +    if (!mHal->GetFbData(handle, fd)) {
> +        LOGE("Get framebuffer data failed");

Is it necessary to put FramebufferSurface inside HwcHAL?

@@ +822,5 @@
>  {
>      LOGD("hwcomposer is already prepared, reset with null set");
> +    // Only ICS need mDpy and mSur
> +    // true means we will reset it by nullptr
> +    mHal->Set(mList, mDpy, mSur, true);

Please don't add the reset action inside set call and think a better way implement this logic.
Maybe you can add 'resetpreparedata' in hal and you can remove the HwcComposer2D::Reset function.

::: widget/gonk/hwchal/HwcHAL.h
@@ +79,5 @@
> +    // HwcDevice set
> +    virtual int Set(HwcList *aList,
> +                    hwc_display_t aDpy,
> +                    hwc_surface_t aSur,
> +                    bool aReset = false) = 0;

Why we need aReset here?

@@ +90,5 @@
> +    // Get framebuffer data
> +    virtual bool GetFbData(buffer_handle_t &aHandle,
> +                           int &aFenceFd) const = 0;
> +
> +    // Check Transpapency support

Please keep lower case for all comments except the first word and you should describe the return value for each function. Please refer others' code as reference.

@@ +94,5 @@
> +    // Check Transpapency support
> +    virtual bool SupportTransapent() const = 0;
> +
> +    // Get a Geometry Change Flag
> +    virtual uint32_t GeometryChangeFlag(bool aGeometryChanged) const = 0;

// Get the geometrychanged flag
s/GeometryChangeFlag/GetGeometryChangedFlag

::: widget/gonk/hwchal/HwcICS.cpp
@@ +44,5 @@
> +
> +int
> +HwcICS::Set(HwcList *aList
> +            hwc_display_t aDpy,
> +            hwc_surface_t aSur,

Please adjust the parameter sequence as same as mHwc::set call.

@@ +85,5 @@
> +}
> +
> +UniquePtr<HwcHAL>
> +HwcHAL::CreateHwcHAL()
> +{

Why do we need to put HwcHAL function in HwcICS?

For the CreateHwcHAL, why not allocate HwcJB or HwcICS in HwcComposer2D?

::: widget/gonk/hwchal/HwcJB.cpp
@@ +55,5 @@
> +int
> +HwcJB::Set(HwcList *aList,
> +           hwc_display_t aDpy,
> +           hwc_surface_t aSur,
> +           bool aReset)

Why we need the aReset flag here?
Attachment #8597094 - Flags: feedback?(pchang) → feedback-
Comment on attachment 8597094 [details] [diff] [review]
Part 1: Wrap mHwc from HwcComposer2D (v4)

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

::: widget/gonk/HwcComposer2D.cpp
@@ +28,5 @@
>  #include "mozilla/layers/CompositorParent.h"
>  #include "mozilla/layers/LayerManagerComposite.h"
>  #include "mozilla/layers/PLayerTransaction.h"
>  #include "mozilla/layers/ShadowLayerUtilsGralloc.h"
> +#include "mozilla/layers/TextureHostOGL.h"

OK. Sorry.

@@ +99,5 @@
>  
>  static StaticRefPtr<HwcComposer2D> sInstance;
>  
>  HwcComposer2D::HwcComposer2D()
> +    : mHal(HwcHAL::CreateHwcHAL())

OK.

@@ +590,5 @@
>                  return true;
>              }
>              region.numRects = visibleRects->size();
>              region.rects = &((*visibleRects)[0]);
> +        }else {

OK.

@@ +708,5 @@
>  
> +    buffer_handle_t handle = nullptr;
> +    int fd = -1;
> +    if (!mHal->GetFbData(handle, fd)) {
> +        LOGE("Get framebuffer data failed");

Yes. FramebufferSurface is only defined in JB or later, not in ICS. I have to wrap this into HwcHAL, so we don't need to understand the implementation of it.

@@ +822,5 @@
>  {
>      LOGD("hwcomposer is already prepared, reset with null set");
> +    // Only ICS need mDpy and mSur
> +    // true means we will reset it by nullptr
> +    mHal->Set(mList, mDpy, mSur, true);

I will add a new function to handle this. Thanks for your suggestion.

::: widget/gonk/hwchal/HwcHAL.h
@@ +79,5 @@
> +    // HwcDevice set
> +    virtual int Set(HwcList *aList,
> +                    hwc_display_t aDpy,
> +                    hwc_surface_t aSur,
> +                    bool aReset = false) = 0;

As your suggestion, I will add a new function to handle the reset case.

@@ +90,5 @@
> +    // Get framebuffer data
> +    virtual bool GetFbData(buffer_handle_t &aHandle,
> +                           int &aFenceFd) const = 0;
> +
> +    // Check Transpapency support

OK.

@@ +94,5 @@
> +    // Check Transpapency support
> +    virtual bool SupportTransapent() const = 0;
> +
> +    // Get a Geometry Change Flag
> +    virtual uint32_t GeometryChangeFlag(bool aGeometryChanged) const = 0;

Good. Thanks.

::: widget/gonk/hwchal/HwcICS.cpp
@@ +44,5 @@
> +
> +int
> +HwcICS::Set(HwcList *aList
> +            hwc_display_t aDpy,
> +            hwc_surface_t aSur,

Sure

@@ +85,5 @@
> +}
> +
> +UniquePtr<HwcHAL>
> +HwcHAL::CreateHwcHAL()
> +{

I put this function into HwcICS and HwcJB, so we don't need to add a #if to check the version in HwcComposer2D.
Comment on attachment 8597097 [details] [diff] [review]
Part 2: Merge TryHwComposition and Render (v2)

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

::: widget/gonk/HwcComposer2D.cpp
@@ +706,5 @@
>  HwcComposer2D::Render()
>  {
>      // If HWC module does not exist or mList is not created yet,
> +    if (!mHal->SupportFBDev() || !mList) {
> +        // Only GonkDisplayICS uses arguments.

Please move the comment into HwcICS.

::: widget/gonk/hwchal/HwcJB.h
@@ +47,5 @@
>  
>      virtual uint32_t GeometryChangeFlag(bool aGeometryChanged) const override;
>  
> +    virtual bool SupportFBDev() const override;
> +

Please add comment for these two functions.
Attachment #8597097 - Flags: feedback?(pchang) → feedback+
Comment on attachment 8597101 [details] [diff] [review]
Part 3: Wrap mList (v5)

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

::: widget/gonk/HwcComposer2D.cpp
@@ +542,5 @@
>                                                 visibleRects)) {
>                  return true;
>              }
> +            hwcLayer.visibleRegionScreen.numRects = visibleRects->size();
> +            hwcLayer.visibleRegionScreen.rects = &((*visibleRects)[0]);

Why the logic for visibleRegion.GetNumRects()==1 is changed?

@@ +555,5 @@
>          hwcLayer.transform = colorLayer->GetColor().Packed();
>      }
>  
>      mHwcLayerMap.AppendElement(static_cast<LayerComposite*>(aLayer->ImplData()));
> +    mHal->AppendHwcLayerEnd();

The reason for AppendHwcLayerEnd is because there are some early return in PrepareLayerList call. Could you try to fix them? And you don't need the AppendHwcLayerEnd call.

@@ +575,5 @@
>              return false;
>          }
>  
>          // Add FB layer
> +        if (!mHal->AppendHwcLayerFb()) {

Please rename the function and update the comment as 'Append a hwclayer for FB'

::: widget/gonk/hwchal/HwcHAL.cpp
@@ +63,5 @@
> +        mHwcList.mList->numHwLayers + 1 > mHwcList.mMaxCount) {
> +        // need alloc more space
> +        if (!ReallocHwcList() ||
> +            mHwcList.mList->numHwLayers + 1 > mHwcList.mMaxCount) {
> +            // Can not rellaoc memory or aMinCap is too large

what is aMinCap?

@@ +83,5 @@
> +    }
> +}
> +
> +bool
> +HwcHAL::AppendHwcLayerFb()

This is not related to FB, it just appends one layer in the end.

::: widget/gonk/hwchal/HwcHAL.h
@@ +150,3 @@
>  protected:
>      HwcDevice *mHwc;
> +    HwcListData_t mHwcList;

s/mHwcList/mHwcListData
Attachment #8597101 - Flags: feedback?(pchang) → feedback-
Comment on attachment 8597094 [details] [diff] [review]
Part 1: Wrap mHwc from HwcComposer2D (v4)

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

::: widget/gonk/HwcComposer2D.cpp
@@ +708,5 @@
>  
> +    buffer_handle_t handle = nullptr;
> +    int fd = -1;
> +    if (!mHal->GetFbData(handle, fd)) {
> +        LOGE("Get framebuffer data failed");

I think we can add #if for FrameBufferSurface because we just wrap stuffs related to HWC into HwcHAL.

::: widget/gonk/hwchal/HwcICS.cpp
@@ +85,5 @@
> +}
> +
> +UniquePtr<HwcHAL>
> +HwcHAL::CreateHwcHAL()
> +{

In my opinion, I think it is strange to put HwcHAL in HwcICS and HwcJB. Add version checking with ifdef in HwcComposer2D is fine for me.
Comment on attachment 8597097 [details] [diff] [review]
Part 2: Merge TryHwComposition and Render (v2)

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

::: widget/gonk/HwcComposer2D.cpp
@@ +706,5 @@
>  HwcComposer2D::Render()
>  {
>      // If HWC module does not exist or mList is not created yet,
> +    if (!mHal->SupportFBDev() || !mList) {
> +        // Only GonkDisplayICS uses arguments.

OK. I should remove this comment.

::: widget/gonk/hwchal/HwcJB.h
@@ +47,5 @@
>  
>      virtual uint32_t GeometryChangeFlag(bool aGeometryChanged) const override;
>  
> +    virtual bool SupportFBDev() const override;
> +

OK. I will add comments in HwcICS.cpp, HwcJB.cpp, and HwcHAL.h
Attachment #8597127 - Flags: feedback?(pchang)
As we discussed offline yesterday, it is better to split HWC refactor codes into several bugs. Please re-clarify the scope you want to refactor for this bug.
Comment on attachment 8597094 [details] [diff] [review]
Part 1: Wrap mHwc from HwcComposer2D (v4)

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

::: widget/gonk/hwchal/HwcJB.h
@@ +22,5 @@
> +#include "HwcHAL.h"
> +
> +namespace mozilla {
> +
> +class HwcJB : public HwcHAL {

BTW, you are going to maintain the android versions after JB in this class.
I would suggest to rename the base class to HwcHALBase and the derived class to HwcHAL.
And this new HwcHAL contains the several latest android versions support.

We can split the old android support from HwcHAL and rename with related android version if need.
Summary: Wrap HAL code from HwcComposer2D → Wrap HwcDevice from HwcComposer2D
Status: NEW → ASSIGNED
Blocks: 1159592
Blocks: 1159597
Depends on: 1155498
Use HwcHALBase as a base class which has two subclasses, HwcICS and HwcHAL.
1. HwcICS for Android ICS
2. HwcHAL for Android JB/KK/L or later.
3. mHwc is wrappd into HwcHALBase.
4. Move fence reset into commit()
Attachment #8599694 - Attachment is obsolete: true
Use two flags to check the composition flow, so we
can reduce the usage of #ifdef. BTW, revise Commit(),
so we can use Commit() on different platforms.
Attachment #8599714 - Attachment is obsolete: true
Attached patch Part 3: Wrap vsync code (v1) (obsolete) — Splinter Review
All the patches are based on Bug 1155498.
Attachment #8600812 - Flags: feedback?(pchang)
Attachment #8600813 - Flags: feedback?(pchang)
Attachment #8600814 - Flags: feedback?(pchang)
Attachment #8600814 - Flags: feedback?(hshih)
Comment on attachment 8600812 [details] [diff] [review]
Part 1: Wrap HwcDevice from HwcComposer2D (v7)

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

::: widget/gonk/HwcComposer2D.cpp
@@ +751,4 @@
>      if (mPrepared) {
>          LOGE("Multiple hwc prepare calls!");
>      }
> +    int err = mHal->Prepare(mList, aDispHandle, aFence);

This is not clear for me, it looks like you keep the ICS hwc::prepare API format for android JB or later.
But you need to change the interface again when HWC supports multiple displays

@@ +779,5 @@
>          }
>      }
>  
> +    // Only ICS need mDpy and mSur
> +    int err = mHal->Set(mDpy, mSur, mList);

This is not clear for me, it looks like you keep the ICS hwc::set API format for android JB or later.
But you need to change the interface again when HWC supports multiple displays

@@ +804,5 @@
>  
>      if (mList->retireFenceFd >= 0) {
>          mPrevRetireFence = FenceHandle(mList->retireFenceFd);
>      }
>  

The comment should be correct to "Set the release fence for the fb layer"

::: widget/gonk/hwchal/HwcHAL.cpp
@@ +134,5 @@
> +HwcHAL::SetCrop(HwcLayer &aLayer,
> +                const hwc_rect_t &aSrcCrop) const
> +{
> +#if ANDROID_VERSION >= 19
> +    if (mHwc && mHwc->common.version >= HWC_DEVICE_API_VERSION_1_3) {

You can move ANDROID_VERSION inside if and call HwcHALBase::SetCrop for else case.
For example, 
if (mHwc...) {
#if android_version
...
#endif
} else {
  HwcHALBase::SetCrop()
}
Attachment #8600812 - Flags: feedback?(pchang)
Comment on attachment 8600813 [details] [diff] [review]
Part 2: Refine TryHWComposition and Render (v2)

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

::: widget/gonk/HwcComposer2D.cpp
@@ +717,5 @@
>  
> +/* 1. ICS: Call SwapBuffers() directly
> + * 2. JB/KK/L:
> + *   a. Check fb device and HwcList
> + *   b. Prepare() (if needed) and then Commit() */

It is hard to understand your comment, please describe more detail when and why the Render will be called.
About the behavior of different android versions, you can say explain by using SupportDispDevice() and then we don't need to know ICS or JB...

@@ +722,5 @@
>  bool
>  HwcComposer2D::Render()
>  {
> +    // If Display device is not supported or mList is not created yet,
> +    if (!mHal->SupportDispDevice() || !mList) {

Please consider a better name for SupportDispDevice(), for me, it is better to call SupportFBDevice.

@@ +783,5 @@
> + * 2. JB/KK/L:
> + *   a. Reset layers acquire fences
> + *   b. Call set()
> + *   c. Reset layers release fences
> + *   d. Reset fb layer fence */

Same problem in line 721.
Attachment #8600813 - Flags: feedback?(pchang)
Attachment #8600814 - Flags: feedback?(pchang) → feedback+
Comment on attachment 8600814 [details] [diff] [review]
Part 3: Wrap vsync code (v1)

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

F+ if the following comment is addressed.

::: widget/gonk/HwcComposer2D.cpp
@@ +152,5 @@
> +        &HookInvalidate, // 1st: void (*invalidate)(...)
> +        &HookVsync,      // 2nd: void (*vsync)(...)
> +        &HookHotplug     // 3rd: void (*hotplug)(...)
> +    };
> +    mHasHWVsync = mHal->RegisterHwcEventCallback(cHWCProcs);

This should be the result for callbacks registration not vsync is available or not.
And please keep the gfxPrefs in HwcComposer2D.cpp.
Comment on attachment 8600812 [details] [diff] [review]
Part 1: Wrap HwcDevice from HwcComposer2D (v7)

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

::: widget/gonk/HwcComposer2D.cpp
@@ +779,5 @@
>          }
>      }
>  
> +    // Only ICS need mDpy and mSur
> +    int err = mHal->Set(mDpy, mSur, mList);

OK. Let me fix the API in next patch.

@@ +804,5 @@
>  
>      if (mList->retireFenceFd >= 0) {
>          mPrevRetireFence = FenceHandle(mList->retireFenceFd);
>      }
>  

OK

::: widget/gonk/hwchal/HwcHAL.cpp
@@ +134,5 @@
> +HwcHAL::SetCrop(HwcLayer &aLayer,
> +                const hwc_rect_t &aSrcCrop) const
> +{
> +#if ANDROID_VERSION >= 19
> +    if (mHwc && mHwc->common.version >= HWC_DEVICE_API_VERSION_1_3) {

JB doesn't define HWC_DEVICE_API_VERSION_1_3, so I think we should keep this definition in the #ifdef.
Comment on attachment 8600813 [details] [diff] [review]
Part 2: Refine TryHWComposition and Render (v2)

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

::: widget/gonk/HwcComposer2D.cpp
@@ +717,5 @@
>  
> +/* 1. ICS: Call SwapBuffers() directly
> + * 2. JB/KK/L:
> + *   a. Check fb device and HwcList
> + *   b. Prepare() (if needed) and then Commit() */

I will add more explanation. Thanks.

@@ +722,5 @@
>  bool
>  HwcComposer2D::Render()
>  {
> +    // If Display device is not supported or mList is not created yet,
> +    if (!mHal->SupportDispDevice() || !mList) {

OK. I can revise the name. Thanks for your suggestion
Comment on attachment 8600814 [details] [diff] [review]
Part 3: Wrap vsync code (v1)

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

::: widget/gonk/HwcComposer2D.cpp
@@ +152,5 @@
> +        &HookInvalidate, // 1st: void (*invalidate)(...)
> +        &HookVsync,      // 2nd: void (*vsync)(...)
> +        &HookHotplug     // 3rd: void (*hotplug)(...)
> +    };
> +    mHasHWVsync = mHal->RegisterHwcEventCallback(cHWCProcs);

OK. Thanks.
Use HwcHALBase as a base class which has two subclasses, HwcICS and HwcHAL.
1. HwcICS for Android ICS
2. HwcHAL for Android JB/KK/L or later.
3. mHwc is wrappd into HwcHALBase.
4. Move fence set for the fb layer into commit()
Attachment #8601402 - Attachment is obsolete: true
Use two flags to check the composition flow, so we
can reduce the usage of #ifdef. BTW, revise Commit(),
so we can use Commit() on different platforms.
Attachment #8600813 - Attachment is obsolete: true
Attached patch Part 3: Wrap vsync code (v2) (obsolete) — Splinter Review
Attachment #8600814 - Attachment is obsolete: true
Attachment #8600814 - Flags: feedback?(hshih)
Attachment #8601938 - Flags: review?(sotaro.ikeda.g)
Attachment #8601939 - Flags: review?(sotaro.ikeda.g)
Attachment #8601940 - Flags: review?(sotaro.ikeda.g)
(In reply to Boris Chiou [:boris] from comment #65)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc503ba85a56

Looks like I should rebase the code for fence changes from Bug 1155498 after current reviews.
Comment on attachment 8601938 [details] [diff] [review]
Part 1: Wrap HwcDevice from HwcComposer2D (v9)

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

I prefer to use GonkDisplay to encapsulate hwc's gonk version dependent differences than adding a new HwcHAL class. GonkDisplay actually/already load hwc hal and doing some tasks related to hwc hal. And GonkDisplay will becomes more like android's HWComposer. From this point of view, GonkDisplay and HwcHAL conflict about the hwc hal handling. Bug 1138287 Comment 44 and  Bug 1138287 Comment 47 might help to understand the context.

About how to use GonkDisplay, it seems better to get feedback from mwu.
Attachment #8601938 - Flags: review?(sotaro.ikeda.g)
(In reply to Sotaro Ikeda [:sotaro] from comment #68)
>
> Bug 1138287 Comment 44 and 
> Bug 1138287 Comment 47 might help to understand the context.

From this point of view, HwcComposer2D's position might be similar to SurfaceFlinger::setUpHWComposer().
  http://androidxref.com/5.1.0_r1/xref/frameworks/native/services/surfaceflinger/SurfaceFlinger.cpp#1016
Even on current gecko, it is not clear that who is responsible to accessing to hwc hal. HwcComposer2D? or GonkDisplay?
(In reply to Sotaro Ikeda [:sotaro] from comment #70)
> Even on current gecko, it is not clear that who is responsible to accessing
> to hwc hal. HwcComposer2D? or GonkDisplay?

It seems better to address this problem at first, I think.
(In reply to Sotaro Ikeda [:sotaro] from comment #71)
> (In reply to Sotaro Ikeda [:sotaro] from comment #70)
> > Even on current gecko, it is not clear that who is responsible to accessing
> > to hwc hal. HwcComposer2D? or GonkDisplay?
> 
> It seems better to address this problem at first, I think.

It makes sense. The responsibility of accessing hwc hal now is in both HwcComposer2D and GonkDisplay. The reason I create this new class, HwcHAL, is that I want to make the code more modular. I can merge HwcHAL class into GonkDisplay, so only GonkDisplay can handle/access HwcDevice.

Hi mwu,
Do you have any suggestion or concern about moving the access to HwcDevice into GonkDisplay? Just merge the HwcHAL into GonkDisplay, and GonkDisplay provides the APIs so HwcComposer2D can query, prepare, set the HwcDevice.
Flags: needinfo?(mwu)
Provide Prepare, set, and query APIs in GonkDisplay.
(In reply to Boris Chiou [:boris] from comment #72)
> (In reply to Sotaro Ikeda [:sotaro] from comment #71)
> > (In reply to Sotaro Ikeda [:sotaro] from comment #70)
> > > Even on current gecko, it is not clear that who is responsible to accessing
> > > to hwc hal. HwcComposer2D? or GonkDisplay?
> > 
> > It seems better to address this problem at first, I think.
> 
> It makes sense. The responsibility of accessing hwc hal now is in both
> HwcComposer2D and GonkDisplay. The reason I create this new class, HwcHAL,
> is that I want to make the code more modular. I can merge HwcHAL class into
> GonkDisplay, so only GonkDisplay can handle/access HwcDevice.
> 
> Hi mwu,
> Do you have any suggestion or concern about moving the access to HwcDevice
> into GonkDisplay? Just merge the HwcHAL into GonkDisplay, and GonkDisplay
> provides the APIs so HwcComposer2D can query, prepare, set the HwcDevice.

Hi mwu,
I tried to merge these APIs into GonkDisplay, so you can check attachment 8602641 [details] [diff] [review] and attachment 8602643 [details] [diff] [review] for your reference.
Comment on attachment 8602643 [details] [diff] [review]
[WIP] Part 2: Wrap mHwc by GonkDisplay APIs

Set flag based on comment 75.
Attachment #8602643 - Flags: feedback?(mwu)
Attachment #8602641 - Flags: feedback?(mwu)
Attachment #8602641 - Flags: feedback?(mwu)
Comment on attachment 8602643 [details] [diff] [review]
[WIP] Part 2: Wrap mHwc by GonkDisplay APIs

Sorry, there is already ni.
Attachment #8602643 - Flags: feedback?(mwu)
(In reply to Sotaro Ikeda [:sotaro] from comment #71)
> (In reply to Sotaro Ikeda [:sotaro] from comment #70)
> > Even on current gecko, it is not clear that who is responsible to accessing
> > to hwc hal. HwcComposer2D? or GonkDisplay?
> 
> It seems better to address this problem at first, I think.

Hi Sotaro,
Moving HwcDevice into GonkDisplay is reasonable, but if I also want to add some flags dependent on different versions (ICS or JB later), which class can I use? I think GonkDisplay is not a good choice to handle these things. For example, Attachment 8601939 [details] [diff] want to add two flags to control the working flow which is dependent on different Android versions. Is there any other class or manager I can use the wrap these HAL related code? Therefore, we can remove more #ifdefs from HwcComposer2D.
Flags: needinfo?(sotaro.ikeda.g)
(In reply to Sotaro Ikeda [:sotaro] from comment #70)
> Even on current gecko, it is not clear that who is responsible to accessing
> to hwc hal. HwcComposer2D? or GonkDisplay?

HwcComposer2D. GonkDisplay *has* to know about the hwc hal in order to render the boot animation, but it lets HwcComposer2D take over once it's ready.
Flags: needinfo?(mwu)
If the usage of hwc hal is take over to HwcComposer2D. It seems better to create a class to wrap HWC hal.
Flags: needinfo?(sotaro.ikeda.g)
boris, comment 79 seems policy of GonkDisplay. If it is positioned like that, it is OK for me to create a new class to wrap HWC hal. The new HWC wrap class might simplify both GonkDisplay and HwcComposer2D.
(In reply to Sotaro Ikeda [:sotaro] from comment #81)
> boris, comment 79 seems policy of GonkDisplay. If it is positioned like
> that, it is OK for me to create a new class to wrap HWC hal. The new HWC
> wrap class might simplify both GonkDisplay and HwcComposer2D.

Got it. Thanks for your feedback. I will create the new class to wrap HWC hal.
(In reply to Boris Chiou [:boris] from comment #82)
> (In reply to Sotaro Ikeda [:sotaro] from comment #81)
> > boris, comment 79 seems policy of GonkDisplay. If it is positioned like
> > that, it is OK for me to create a new class to wrap HWC hal. The new HWC
> > wrap class might simplify both GonkDisplay and HwcComposer2D.
> 
> Got it. Thanks for your feedback. I will create the new class to wrap HWC
> hal.

BTW, I should redesign HwcHAL class, so GonkDisplay can also use it.
Attachment #8601939 - Flags: review?(sotaro.ikeda.g)
Attachment #8601940 - Flags: review?(sotaro.ikeda.g)
(In reply to Sotaro Ikeda [:sotaro] from comment #81)
> boris, comment 79 seems policy of GonkDisplay. If it is positioned like
> that, it is OK for me to create a new class to wrap HWC hal. The new HWC
> wrap class might simplify both GonkDisplay and HwcComposer2D.

Hi Sotaro,

According to comment #79, so I have to create the HwcHAL object by GonkDisplay to handle Boot animation, and then transfer the ownership to HwcComposer2D? Or make the HwcHAL object as a global static object and GonkDisplay/HwcComposer2D just get it by a special get function?
I am thinking where should I put the HwcHAL because GonkDisplay also need this. Right now I would put it in libdisplay, so both HwcComposer2D and GonkDisplay can access this class, ex. widget/gonk/libdisplay/hwchal/HwcHAL.cpp. Do you have any other suggestion?
Flags: needinfo?(sotaro.ikeda.g)
(In reply to Boris Chiou [:boris] from comment #84)
> (In reply to Sotaro Ikeda [:sotaro] from comment #81)
> > boris, comment 79 seems policy of GonkDisplay. If it is positioned like
> > that, it is OK for me to create a new class to wrap HWC hal. The new HWC
> > wrap class might simplify both GonkDisplay and HwcComposer2D.
> 
> Hi Sotaro,
> 
> According to comment #79, so I have to create the HwcHAL object by
> GonkDisplay to handle Boot animation, and then transfer the ownership to
> HwcComposer2D? Or make the HwcHAL object as a global static object and
> GonkDisplay/HwcComposer2D just get it by a special get function?
> I am thinking where should I put the HwcHAL because GonkDisplay also need
> this. Right now I would put it in libdisplay, so both HwcComposer2D and
> GonkDisplay can access this class, ex.
> widget/gonk/libdisplay/hwchal/HwcHAL.cpp. Do you have any other suggestion?

GonkDisplay cannot access HwcComposer2D right now, and GonkDisplay may use HwcHAL after BootAnimation (HwcCompsoer2D::Render() -> GonkDisplay::SwapBuffer() -> GonkDisplay::Post()). Is it possible to make sure we will not use HwcHAL in GonkDisplay after BootAnimation?
(In reply to Boris Chiou [:boris] from comment #85)
> 
> GonkDisplay cannot access HwcComposer2D right now, and GonkDisplay may use
> HwcHAL after BootAnimation (HwcCompsoer2D::Render() ->
> GonkDisplay::SwapBuffer() -> GonkDisplay::Post()). Is it possible to make
> sure we will not use HwcHAL in GonkDisplay after BootAnimation?

Cant's we move "GonkDisplay::SwapBuffer() -> GonkDisplay::Post()" part to hal?
Flags: needinfo?(sotaro.ikeda.g)
(In reply to Boris Chiou [:boris] from comment #84)
> Hi Sotaro,
> 
> According to comment #79, so I have to create the HwcHAL object by
> GonkDisplay to handle Boot animation, and then transfer the ownership to
> HwcComposer2D? Or make the HwcHAL object as a global static object and
> GonkDisplay/HwcComposer2D just get it by a special get function?

It might bet better to implement it like incremental way. If you create too much at first, you could face a risk of too much architecture compared to actual necessity. In current GonkDisplay and HwcComposer2D, such ownership transfer is not explicit. Therefore, similar way of implementation is enough for the time being.
(In reply to Sotaro Ikeda [:sotaro] from comment #86)
> (In reply to Boris Chiou [:boris] from comment #85)
> > 
> > GonkDisplay cannot access HwcComposer2D right now, and GonkDisplay may use
> > HwcHAL after BootAnimation (HwcCompsoer2D::Render() ->
> > GonkDisplay::SwapBuffer() -> GonkDisplay::Post()). Is it possible to make
> > sure we will not use HwcHAL in GonkDisplay after BootAnimation?
> 
> Cant's we move "GonkDisplay::SwapBuffer() -> GonkDisplay::Post()" part to
> hal?

Anyway, it seems better to remove this dependency.
(In reply to Sotaro Ikeda [:sotaro] from comment #88)
> (In reply to Sotaro Ikeda [:sotaro] from comment #86)
> > (In reply to Boris Chiou [:boris] from comment #85)
> > > 
> > > GonkDisplay cannot access HwcComposer2D right now, and GonkDisplay may use
> > > HwcHAL after BootAnimation (HwcCompsoer2D::Render() ->
> > > GonkDisplay::SwapBuffer() -> GonkDisplay::Post()). Is it possible to make
> > > sure we will not use HwcHAL in GonkDisplay after BootAnimation?
> > 
> > Cant's we move "GonkDisplay::SwapBuffer() -> GonkDisplay::Post()" part to
> > hal?
> 
> Anyway, it seems better to remove this dependency.

Yes. I can try to move this part into hal, so HwcComposer2D won't need to call GonkDisplay::SwapBuffer() any more. Thanks.
(In reply to Sotaro Ikeda [:sotaro] from comment #87)
> (In reply to Boris Chiou [:boris] from comment #84)
> > Hi Sotaro,
> > 
> > According to comment #79, so I have to create the HwcHAL object by
> > GonkDisplay to handle Boot animation, and then transfer the ownership to
> > HwcComposer2D? Or make the HwcHAL object as a global static object and
> > GonkDisplay/HwcComposer2D just get it by a special get function?
> 
> It might bet better to implement it like incremental way. If you create too
> much at first, you could face a risk of too much architecture compared to
> actual necessity. In current GonkDisplay and HwcComposer2D, such ownership
> transfer is not explicit. Therefore, similar way of implementation is enough
> for the time being.

Sorry, I am not sure the incremental way. Could you give me a example? Thanks.
BTW, the current usage of mHwc is almost the same between HwcComposer2D and GonkDisplayJB, so maybe I can try this idea after removing the dependency you mentioned: 
1. After we boot, GonkDisplay needs hal, so it creates a hal object and use it, then free this object after Boot Animation?
2. After HwcCompoer2D is created, we create hal again, and then HwcComposer2D holds this object until shutdown.
So we don't need to transfer the ownership.
> I am thinking where should I put the HwcHAL because GonkDisplay also need
> this. Right now I would put it in libdisplay, so both HwcComposer2D and
> GonkDisplay can access this class, ex.
> widget/gonk/libdisplay/hwchal/HwcHAL.cpp. Do you have any other suggestion?

It might better to get feedback from mwu.
(In reply to Sotaro Ikeda [:sotaro] from comment #91)
> > I am thinking where should I put the HwcHAL because GonkDisplay also need
> > this. Right now I would put it in libdisplay, so both HwcComposer2D and
> > GonkDisplay can access this class, ex.
> > widget/gonk/libdisplay/hwchal/HwcHAL.cpp. Do you have any other suggestion?
> 
> It might better to get feedback from mwu.

Sure,
hi mwu, 
At first I though we should put hwchal in /widget/gonk (libxul), but GonkDisplay (libdisplay) cannot access it, right? so I want to move it into /widget/gonk/libdisplay, so both GonkDisplay and HwcComposer2D can access hwchal files. Do you have any concern if I put hwchal files into /widget/gonk/libdisplay/hwchal? Thanks.
Flags: needinfo?(mwu)
(In reply to Boris Chiou [:boris] from comment #90)
> Sorry, I am not sure the incremental way. Could you give me a example?

Sorry, my comment was not clear. If there is a thing that can be put off, it seems better to keep same way of implementation until it becomes necessary. The followings are some examples.
- Do we need to explicitly transfer ownership?
- HwcComposer2D and GonkDisplay need to be totally independent?

There could be several choices for implementations, each person have different preferences. Therefore, to make a consensus about the change, small step is easier to get a consensus.

From my point of view, GonkDisplay transfer rendering role to HwcComposer2D. But it does not transfer all hwc control roles. Because GonkDisplay has a role to handle additional HWC displays(Bug 1138287). Therefore, GonkDisplay need to keep the pointer of the wrapper after transferring the rendering role.

About hwc control, it is still not clear yet, which should have a responsibility. "vsync on/off" is controlled by HwcComposer2D, but display add/remove should be handled by GonkDisplay. Therefore we could put off about this point.

I do not think HwcComposer2D and GonkDisplay need to be totally independent now. At least, HwcComposer2D needs to get the wrapper from GonkDisplay. It makes loading order simpler. If the wrapper is reference counted, the lifetime of the wrapper could be controlled by the reference count.

But GonkDisplay transfers rendering role to HwcComposer2D when animation boot end. Therefore it seems better to remove dependency about the rendering.

As a first step of incremental implementation. The following is reasonable from my point of view.
- Create hal wrapper, decide wrapper's role at this point.
  It might better to move some code like GonkDisplayJB::Post(). It is just my preference.
  Remove rendering dependency between HwcComposer2D and GonkDisplay.
  We put off hwc control role problem.
  Keep same way of rendering role transfer as current gecko.
  GonkDisplayJB uses the wrapper instead of direct hwc hal module.

From the above change, we could make clear if the hwc wrapper is necessary. If we convinced the wrapper is necessary, the could proceed to another HwcComposer2D's fix.
One of my concern is that in this bug, a necessity of hwc wrapper is not clear enough.
(In reply to Sotaro Ikeda [:sotaro] from comment #94)
> One of my concern is that in this bug, a necessity of hwc wrapper is not
> clear enough.

My understanding is that the wrapper encapsulate the hwc's version difference.
If a hardware do not use hwc for rendering, how is the case handled?
Depends on: 1163681
By the way, even related to rendering, HwcComposer2D still seems to need to reference GonkDisplay, because GonkDisplay owns FrameBufferSurface.
(In reply to Sotaro Ikeda [:sotaro] from comment #93)
> The followings are some examples.
> - Do we need to explicitly transfer ownership?
> - HwcComposer2D and GonkDisplay need to be totally independent?
> 
> From my point of view, GonkDisplay transfer rendering role to HwcComposer2D.
> But it does not transfer all hwc control roles. Because GonkDisplay has a
> role to handle additional HWC displays(Bug 1138287). Therefore, GonkDisplay
> need to keep the pointer of the wrapper after transferring the rendering
> role.
> 
> About hwc control, it is still not clear yet, which should have a
> responsibility. "vsync on/off" is controlled by HwcComposer2D, but display
> add/remove should be handled by GonkDisplay. Therefore we could put off
> about this point.
> 
> I do not think HwcComposer2D and GonkDisplay need to be totally independent
> now. At least, HwcComposer2D needs to get the wrapper from GonkDisplay. It
> makes loading order simpler. If the wrapper is reference counted, the
> lifetime of the wrapper could be controlled by the reference count.
> 
> But GonkDisplay transfers rendering role to HwcComposer2D when animation
> boot end. Therefore it seems better to remove dependency about the rendering.
> 
> As a first step of incremental implementation. The following is reasonable
> from my point of view.
> - Create hal wrapper, decide wrapper's role at this point.
>   It might better to move some code like GonkDisplayJB::Post(). It is just
> my preference.
>   Remove rendering dependency between HwcComposer2D and GonkDisplay.
>   We put off hwc control role problem.
>   Keep same way of rendering role transfer as current gecko.
>   GonkDisplayJB uses the wrapper instead of direct hwc hal module.
> 
> From the above change, we could make clear if the hwc wrapper is necessary.
> If we convinced the wrapper is necessary, the could proceed to another
> HwcComposer2D's fix.

Thanks for your detailed explanation.
I agree with you. HwcComposer2D needs GonkDisplay, so we don't have to make them to be totally independent. I think we can just reduce the dependency on rendering part, and try to make the rendering path clearer.
The primary purpose of creating the wrapper is not only to encapsulate the hwc's version difference but also to split the gecko logic and hal logic in HwcComposer2D. Also, there are some duplicate code between GonkDisplay and HwcComposer2D (ex. GonkDisplay::Post(), as you mentioned), so we can use this wrapper to make the code more concise, and make sure who has the responsibility of accessing mHwc in each phase.

Therefore, I think the role of this wrapper is to access mHwc (and encapsulate the duplicate code), and tell HwcComposer2D which feature the current hal supports (based on the current Android version or driver vender).

BTW, I hope GonkDisplay would not need to access the wrapper after boot animation, but using a refer-counted wrapper is a good choice for current implementation, and I will try this first. Thank you.
(In reply to Sotaro Ikeda [:sotaro] from comment #95)
> (In reply to Sotaro Ikeda [:sotaro] from comment #94)
> > One of my concern is that in this bug, a necessity of hwc wrapper is not
> > clear enough.
> 
> My understanding is that the wrapper encapsulate the hwc's version
> difference.
> If a hardware do not use hwc for rendering, how is the case handled?

I didn't think this matter over before, but I guess HwcHAL should tell HwcComposer2D/GonkDisplay this case, so HwcComposer2D should use another renderer. Maybe we could handle this later.
Attachment #8610036 - Flags: feedback?(pchang)
Attachment #8610036 - Flags: feedback?(hshih)
Flags: needinfo?(mwu)
(In reply to Boris Chiou [:boris] from comment #114)
> Comment on attachment 8610036 [details] [diff] [review]
> [WIP] Part 5: Remove dependecy between SwapBuffer and Post (v2)
> 
> Review of attachment 8610036 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/gonk/HwcComposer2D.cpp
> @@ +690,5 @@
> > +    if (!mList) {
> > +        // If mList == nullptr, mPrepared is false, so we only need
> > +        // to alloc enough memory (at least 2 HwcLayers) for mList
> > +        ReallocLayerList();
> > +        printf_stderr("[Boris] mList is nullptr in HwcComposer2D");
> 
> Please ignore all the printfs. I will remove them in my next patch. Thanks.

Please have a summary about the changes you made.
(In reply to peter chang[:pchang][:peter] from comment #115)
> (In reply to Boris Chiou [:boris] from comment #114)
> > Comment on attachment 8610036 [details] [diff] [review]
> > [WIP] Part 5: Remove dependecy between SwapBuffer and Post (v2)
> > 
> > Review of attachment 8610036 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: widget/gonk/HwcComposer2D.cpp
> > @@ +690,5 @@
> > > +    if (!mList) {
> > > +        // If mList == nullptr, mPrepared is false, so we only need
> > > +        // to alloc enough memory (at least 2 HwcLayers) for mList
> > > +        ReallocLayerList();
> > > +        printf_stderr("[Boris] mList is nullptr in HwcComposer2D");
> > 
> > Please ignore all the printfs. I will remove them in my next patch. Thanks.
> 
> Please have a summary about the changes you made.

Sure.

As #comment 93 mentioned, this patch is trying to make sure that GonkDisplay transfers rendering role to HwcComposer2D when boot animation ends, so I remove rendering dependency between HwcComposer2D and GonkDisplay. In other words, we will not see any hal access in GonkDisplay::SwapBuffers(). Actually, I want to remove GonkDisplay::SwapBuffers() in HwcComposer2D, but we still have to consider this case: if we don't have hwc and only have fb device, we still need GonkDisplay. There are three possible ways to handle this:
1. Still call GonkDisplay::Swapbuffer() in HwcComposer2D, as my current implementation
2. Move hwc check and call GonkDisplay::Swapbuffer() into LayerManagerComposite,
   but I think this is gonk related code, so I didn't do this in this patch.
3. Add a new API in GonkDisplay, ex. GonkDisplay()::GetFBDevice(), and access fbdevice in HwcComposer2D,
   so we don't have to call GonkDisplay()::SwapBuffers().

If you have any good suggestion, please let me know. Thanks.
Attachment #8610033 - Flags: feedback?(pchang)
(In reply to Boris Chiou [:boris] from comment #116)
> (In reply to peter chang[:pchang][:peter] from comment #115)
> > (In reply to Boris Chiou [:boris] from comment #114)
> > > Comment on attachment 8610036 [details] [diff] [review]
> > > [WIP] Part 5: Remove dependecy between SwapBuffer and Post (v2)
> > > 
> > > Review of attachment 8610036 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > ::: widget/gonk/HwcComposer2D.cpp
> > > @@ +690,5 @@
> > > > +    if (!mList) {
> > > > +        // If mList == nullptr, mPrepared is false, so we only need
> > > > +        // to alloc enough memory (at least 2 HwcLayers) for mList
> > > > +        ReallocLayerList();
> > > > +        printf_stderr("[Boris] mList is nullptr in HwcComposer2D");
> > > 
> > > Please ignore all the printfs. I will remove them in my next patch. Thanks.
> > 
> > Please have a summary about the changes you made.
> 
> Sure.
> 
> As #comment 93 mentioned, this patch is trying to make sure that GonkDisplay
> transfers rendering role to HwcComposer2D when boot animation ends, so I
> remove rendering dependency between HwcComposer2D and GonkDisplay. In other
> words, we will not see any hal access in GonkDisplay::SwapBuffers().
> Actually, I want to remove GonkDisplay::SwapBuffers() in HwcComposer2D, but
> we still have to consider this case: if we don't have hwc and only have fb
> device, we still need GonkDisplay. There are three possible ways to handle
> this:
> 1. Still call GonkDisplay::Swapbuffer() in HwcComposer2D, as my current
> implementation
> 2. Move hwc check and call GonkDisplay::Swapbuffer() into
> LayerManagerComposite,
>    but I think this is gonk related code, so I didn't do this in this patch.
> 3. Add a new API in GonkDisplay, ex. GonkDisplay()::GetFBDevice(), and
> access fbdevice in HwcComposer2D,
>    so we don't have to call GonkDisplay()::SwapBuffers().
> 
> If you have any good suggestion, please let me know. Thanks.

OK, according to our discussion this morning, we don't have a good/easy way to remove the rendering dependency (ex. check fb device, handle fb surface) between GonkDisplay and HwcComposer2D, so I will obsolete Part 5.
Attachment #8610036 - Flags: feedback?(pchang)
Attachment #8610036 - Flags: feedback?(hshih)
Attachment #8610032 - Flags: review?(sotaro.ikeda.g)
Attachment #8610032 - Flags: review?(mwu)
Attachment #8610033 - Flags: feedback?(pchang) → review?(mwu)
Attachment #8610381 - Flags: review?(sotaro.ikeda.g)
Attachment #8610383 - Flags: review?(sotaro.ikeda.g)
Attachment #8610384 - Flags: review?(sotaro.ikeda.g)
Attachment #8610384 - Flags: review?(mwu)
(In reply to Sotaro Ikeda [:sotaro] from comment #96)
> By the way, even related to rendering, HwcComposer2D still seems to need to
> reference GonkDisplay, because GonkDisplay owns FrameBufferSurface.

Yes, the rendering path in HwcComposer2D still need to reference GonkDisplay to get FrameBufferSurface and fb device (included in SwapBuffers), so as you mentioned, HwcComposer2D and GonkDisplay don't need to be totally independent right now. If we can make sure HwcComposer2D is initialized before GonkDisplay, the problem can be resolved, but it's another problem.

In my current patches, 
1. Create hal wrapper, the role of this wrapper is to access HwcDevice,
   so others don't need to know the hal version/vendor and just use its public APIs to control hal.
   Therefore, we can reduce some duplicate code.
2. I didn't move GonkDisplayJB::Post() totally into hal yet because it still needs FrameBufferSurface.
   BTW, I can move the mList allocation, mHwc->Prepare(), and mHwc->Set() into HwcHAL if it is necessary.
3. Patch (part 5) removes dependency between HwcComposer2D and GonkDisplayICS.
   On JB or later, if we don't have mHwc, GonkDisplay::SwapBuffers will call Post() -> fbDevice::post().
   I only revise the case: when mList is not initialized, we can allocate memory for mList and handle it
   in HwcCompsoer2D, so it won't call GonkDisplay::SwapBuffers() in this case.
4. GonkDisplayJB also uses the wrapper.
Attachment #8611051 - Flags: review?(mwu)
(In reply to Sotaro Ikeda [:sotaro] from comment #93)
> About hwc control, it is still not clear yet, which should have a
> responsibility. "vsync on/off" is controlled by HwcComposer2D, but display
> add/remove should be handled by GonkDisplay. Therefore we could put off
> about this point.

"Vsync on/off" should be handled followed by enabling/disabling screen (mHwc->setPowerMode). However, we control "screen enable/disable" in GonkDisplay and "vsync on/off" is controlled by HwcComposer2D. Android uses android::HWComposer to handle these, but we split the APIs into GonkDisplay and HwcComposer2D right now. Using HwcHAL might be a compromising way to collect all the manipulations of hwc into one object.
Comment on attachment 8610032 [details] [diff] [review]
Part 1: Create HwcDevice wrapper (v5)

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

Overall seems good. I have one question to the patch. About the place of "./hwchal/", it seems better to wait mwu's response.

::: widget/gonk/libdisplay/hwchal/HwcHALBase.h
@@ +78,5 @@
> +
> +    // Create a new HwcHAL/HwcICS object
> +    // The definition is in HwcHAL.h/HwcICS.h to avoid #ifdef
> +    MOZ_EXPORT __attribute__ ((weak))
> +    static already_AddRefed<HwcHALBase> CreateHwcHAL();

Is it necessary to expose CreateHwcHAL()? Since HwcHALBase's instance is a singleton, just expose GetInstance() seems to make the code simpler like GeckoTouchDispatcher::GetInstance().
https://dxr.mozilla.org/mozilla-central/source/widget/gonk/GeckoTouchDispatcher.cpp#52
Comment on attachment 8610381 [details] [diff] [review]
Part 3: Use wrapper in HwcComposer2D (v4)

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

Overall look good. Some comments.

::: widget/gonk/HwcComposer2D.cpp
@@ +117,5 @@
>  
> +    mHal = (HwcHALBase*)GetGonkDisplay()->GetHAL();
> +    if (!mHal) {
> +        LOGD("no hal support from GonkDisplay, we should create a new one");
> +        mHal = HwcHALBase::CreateHwcHAL();

Isn't it better just to get HAL instance instead of explicitly create it, is it? This comment is related to my comment to the previous patch.

@@ +786,5 @@
>              mList->hwLayers[j].acquireFenceFd = fdObj->GetAndResetFd();
>          }
>      }
>  
> +    bool success = mHal->Set(mList, HwcHALBase::DisplayType::PRIMARY);

Is there a reason to revert? mHal->Set() seems to return 0, when success.

@@ +824,5 @@
>  #else
>  bool
>  HwcComposer2D::TryHwComposition()
>  {
> +    return mHal->Set(mList, HwcHALBase::DisplayType::PRIMARY);

Is there a reason to revert? mHal->Set() seems to return 0, when success.
Attachment #8610383 - Flags: review?(sotaro.ikeda.g) → review+
Comment on attachment 8610384 [details] [diff] [review]
Part 5: Remove mHal from GonkDisplayICS (v1)

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

Looks good. One comment to function's return type.

::: widget/gonk/libdisplay/GonkDisplayICS.h
@@ +34,5 @@
>      virtual void SetEnabled(bool enabled);
>  
>      virtual void OnEnabled(OnEnabledCallbackType callback);
>  
> +    virtual void* GetHAL()

Is there a reason to keep this pointer to void*? It might be better to use specific type.
Attachment #8610384 - Flags: review?(sotaro.ikeda.g) → review+
Comment on attachment 8610384 [details] [diff] [review]
Part 5: Remove mHal from GonkDisplayICS (v1)

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

::: widget/gonk/libdisplay/GonkDisplayICS.h
@@ +34,5 @@
>      virtual void SetEnabled(bool enabled);
>  
>      virtual void OnEnabled(OnEnabledCallbackType callback);
>  
> +    virtual void* GetHAL()

Sure, I can revise this.
Comment on attachment 8610032 [details] [diff] [review]
Part 1: Create HwcDevice wrapper (v5)

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

::: widget/gonk/libdisplay/hwchal/HwcHALBase.h
@@ +78,5 @@
> +
> +    // Create a new HwcHAL/HwcICS object
> +    // The definition is in HwcHAL.h/HwcICS.h to avoid #ifdef
> +    MOZ_EXPORT __attribute__ ((weak))
> +    static already_AddRefed<HwcHALBase> CreateHwcHAL();

Thanks! I agree with you and will revise this in my next patch.
Comment on attachment 8610381 [details] [diff] [review]
Part 3: Use wrapper in HwcComposer2D (v4)

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

::: widget/gonk/HwcComposer2D.cpp
@@ +117,5 @@
>  
> +    mHal = (HwcHALBase*)GetGonkDisplay()->GetHAL();
> +    if (!mHal) {
> +        LOGD("no hal support from GonkDisplay, we should create a new one");
> +        mHal = HwcHALBase::CreateHwcHAL();

OK. I will revise this. Thanks!

@@ +786,5 @@
>              mList->hwLayers[j].acquireFenceFd = fdObj->GetAndResetFd();
>          }
>      }
>  
> +    bool success = mHal->Set(mList, HwcHALBase::DisplayType::PRIMARY);

HwcHAL::Set() will return true when success because I hide the error number in the API. In other words, HwcHAL::Set() will return "!mHwc->set()". If you don't like this conversion, I can let HwcHAL:Set() return the error number. Therefore, I didn't revert the value. Sorry for unclear APIs.
Use HwcHALBase as a base class which has two subclasses, HwcICS and HwcHAL.
1. HwcICS for Android ICS
2. HwcHAL for Android JB/KK/L or later.
3. mHwc is wrappd into HwcHAL/HwcICS.
4. Use ref-counted pointer to hold this wrapper
Attachment #8610032 - Attachment is obsolete: true
Attachment #8610032 - Flags: review?(sotaro.ikeda.g)
Attachment #8610032 - Flags: review?(mwu)
Use nsRefPtr to handle HwcHAL in GonkDisplay
Attachment #8611051 - Attachment is obsolete: true
Attachment #8611051 - Flags: review?(mwu)
Use nsRefPtr to handle HwcHAL Object.
Attachment #8610381 - Attachment is obsolete: true
Attachment #8610381 - Flags: review?(sotaro.ikeda.g)
Rebased.
Attachment #8610383 - Attachment is obsolete: true
Attachment #8612167 - Flags: review+
Attachment #8610384 - Attachment is obsolete: true
Attachment #8610384 - Flags: review?(mwu)
Attachment #8612169 - Flags: review+
Attachment #8612161 - Flags: review?(sotaro.ikeda.g)
Attachment #8612161 - Flags: review?(mwu)
Attachment #8612163 - Flags: review?(mwu)
Attachment #8612165 - Flags: review?(sotaro.ikeda.g)
> >  
> > +    bool success = mHal->Set(mList, HwcHALBase::DisplayType::PRIMARY);
> 
> HwcHAL::Set() will return true when success because I hide the error number
> in the API. In other words, HwcHAL::Set() will return "!mHwc->set()". If you
> don't like this conversion, I can let HwcHAL:Set() return the error number.
> Therefore, I didn't revert the value. Sorry for unclear APIs.

Oh, I see.
Comment on attachment 8612161 [details] [diff] [review]
Part 1: Create HwcDevice wrapper (v6)

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

Looks good.

::: widget/gonk/libdisplay/hwchal/HwcHAL.cpp
@@ +29,5 @@
> +HwcHALBase*
> +HwcHALBase::GetInstance()
> +{
> +    if (!sHwcHAL) {
> +        sHwcHAL = new HwcHAL();

It seems better to add "ClearOnShutdown(&sHwcHAL)".

@@ +100,5 @@
> +int
> +HwcHAL::Prepare(HwcList *aList,
> +                DisplayType aDisp,
> +                buffer_handle_t aHandle,
> +                int aFenceFd)

Can't we use layers::FenceHandle here? It could prevent a risk or fd leak. It means to remove raw fd handlings like GetPrevDispAcquireFd(). It could be done in another bug.
Attachment #8612161 - Flags: review?(sotaro.ikeda.g) → review+
Comment on attachment 8612165 [details] [diff] [review]
Part 3: Use wrapper in HwcComposer2D (v5)

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

Looks good.

::: widget/gonk/HwcComposer2D.cpp
@@ +115,5 @@
>      RegisterHwcEventCallback();
>  #endif
>  
> +    mHal = HwcHALBase::GetInstance();
> +    if (!mHal || !mHal->HasHwc()) {

HwcHALBase::GetInstance() always return valid instance. So, " if (!mHal->HasHwc()) {" seems enough here.

@@ +799,5 @@
>      if (mList->retireFenceFd >= 0) {
>          mPrevRetireFence = FenceHandle(new FenceHandle::FdObj(mList->retireFenceFd));
>      }
>  
> +    // Set fb layer fence

It seems better to replace "fb layer fence" by "DisplaySurface layer fence", because in future, VirtualDisplaySurface could also be used.
Attachment #8612165 - Flags: review?(sotaro.ikeda.g) → review+
Comment on attachment 8612161 [details] [diff] [review]
Part 1: Create HwcDevice wrapper (v6)

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

::: widget/gonk/libdisplay/hwchal/HwcHAL.cpp
@@ +29,5 @@
> +HwcHALBase*
> +HwcHALBase::GetInstance()
> +{
> +    if (!sHwcHAL) {
> +        sHwcHAL = new HwcHAL();

OK. Thanks for your reminder.

@@ +100,5 @@
> +int
> +HwcHAL::Prepare(HwcList *aList,
> +                DisplayType aDisp,
> +                buffer_handle_t aHandle,
> +                int aFenceFd)

You are right. FenceHandle is much safer than integer fd. I will revise this API in next patch.
Comment on attachment 8612165 [details] [diff] [review]
Part 3: Use wrapper in HwcComposer2D (v5)

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

::: widget/gonk/HwcComposer2D.cpp
@@ +115,5 @@
>      RegisterHwcEventCallback();
>  #endif
>  
> +    mHal = HwcHALBase::GetInstance();
> +    if (!mHal || !mHal->HasHwc()) {

OK.

@@ +799,5 @@
>      if (mList->retireFenceFd >= 0) {
>          mPrevRetireFence = FenceHandle(new FenceHandle::FdObj(mList->retireFenceFd));
>      }
>  
> +    // Set fb layer fence

Sure!
Comment on attachment 8612161 [details] [diff] [review]
Part 1: Create HwcDevice wrapper (v6)

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

::: widget/gonk/libdisplay/hwchal/HwcHAL.cpp
@@ +29,5 @@
> +HwcHALBase*
> +HwcHALBase::GetInstance()
> +{
> +    if (!sHwcHAL) {
> +        sHwcHAL = new HwcHAL();

Oops, I cannot include "ClearOnShutdown.h" in libdisplay. I got this link error:
../../../dist/include/mozilla/LinkedList.h:287: error: undefined reference to 'mozilla::ClearOnShutdown_Internal::sShutdownObservers'

Do you have other suggestion? Or I will keep the current implementation in this bug.

@@ +100,5 @@
> +int
> +HwcHAL::Prepare(HwcList *aList,
> +                DisplayType aDisp,
> +                buffer_handle_t aHandle,
> +                int aFenceFd)

I cannot include FenceUtils.h in libdisplay because FenceUtils.h also includes other ipc headers which result in many compilation errors. I think we can create another bug to make FenceUtils.h class simpler, so libdisplay can use this class.
Use nsRefPtr to handle HwcHAL Object.
Attachment #8612165 - Attachment is obsolete: true
Attachment #8612759 - Flags: review+
Fix typo.
Attachment #8612169 - Attachment is obsolete: true
Attachment #8612761 - Flags: review+
(In reply to Boris Chiou [:boris] from comment #141)
> Comment on attachment 8612161 [details] [diff] [review]
> Part 1: Create HwcDevice wrapper (v6)
> 
> Review of attachment 8612161 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/gonk/libdisplay/hwchal/HwcHAL.cpp
> @@ +29,5 @@
> > +HwcHALBase*
> > +HwcHALBase::GetInstance()
> > +{
> > +    if (!sHwcHAL) {
> > +        sHwcHAL = new HwcHAL();
> 
> Oops, I cannot include "ClearOnShutdown.h" in libdisplay. I got this link
> error:
> ../../../dist/include/mozilla/LinkedList.h:287: error: undefined reference
> to 'mozilla::ClearOnShutdown_Internal::sShutdownObservers'
> 
> Do you have other suggestion? Or I will keep the current implementation in
> this bug.

HwcComposer2D also do not do it now. Therefore it seems ok for the time being.

One easier possible fix seems like the following. 
I do not know why HwcComposer2D do not call ClearOnShutdown. We could add it to HwcComposer2D and from HwcComposer2D's destructor. We could manually clear it.
> @@ +100,5 @@
> > +int
> > +HwcHAL::Prepare(HwcList *aList,
> > +                DisplayType aDisp,
> > +                buffer_handle_t aHandle,
> > +                int aFenceFd)
> 
> I cannot include FenceUtils.h in libdisplay because FenceUtils.h also
> includes other ipc headers which result in many compilation errors. I think
> we can create another bug to make FenceUtils.h class simpler, so libdisplay
> can use this class.

We do not need to add FenceUtils here, if we face such problem.
Comment on attachment 8612161 [details] [diff] [review]
Part 1: Create HwcDevice wrapper (v6)

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

Looks good.

::: widget/gonk/libdisplay/hwchal/HwcHAL.cpp
@@ +29,5 @@
> +HwcHALBase*
> +HwcHALBase::GetInstance()
> +{
> +    if (!sHwcHAL) {
> +        sHwcHAL = new HwcHAL();

It seems better to add "ClearOnShutdown(&sHwcHAL)".

@@ +92,5 @@
> +}
> +
> +int
> +HwcHAL::ResetHwc()
> +{

Yesterday, I checked hwc's set() internal implementation. I found that when HwcList is nullptr, recent qcom's hwc since JB do nothing. Therefore, it does not work as reset... ICS gonk's hwc hal seems have a meaning, though.
  http://androidxref.com/5.1.0_r1/xref/hardware/qcom/display/msm8974/libhwcomposer/hwc.cpp#402

@@ +100,5 @@
> +int
> +HwcHAL::Prepare(HwcList *aList,
> +                DisplayType aDisp,
> +                buffer_handle_t aHandle,
> +                int aFenceFd)

Can't we use layers::FenceHandle here? It could prevent a risk or fd leak. It means to remove raw fd handlings like GetPrevDispAcquireFd(). It could be done in another bug.
Comment on attachment 8612161 [details] [diff] [review]
Part 1: Create HwcDevice wrapper (v6)

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

::: widget/gonk/libdisplay/hwchal/HwcHAL.cpp
@@ +92,5 @@
> +}
> +
> +int
> +HwcHAL::ResetHwc()
> +{

Oops, I just copied the original implementation from HwcComposer2D. Should I keep this function for other venders because we only know qcom doesn't implement the reset?

@@ +100,5 @@
> +int
> +HwcHAL::Prepare(HwcList *aList,
> +                DisplayType aDisp,
> +                buffer_handle_t aHandle,
> +                int aFenceFd)

After merging this bug, I will create another bug to fix the FenceHandle problem.
(In reply to Sotaro Ikeda [:sotaro] from comment #146)
> (In reply to Boris Chiou [:boris] from comment #141)
> > Comment on attachment 8612161 [details] [diff] [review]
> > Part 1: Create HwcDevice wrapper (v6)
> > 
> > Review of attachment 8612161 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: widget/gonk/libdisplay/hwchal/HwcHAL.cpp
> > @@ +29,5 @@
> > > +HwcHALBase*
> > > +HwcHALBase::GetInstance()
> > > +{
> > > +    if (!sHwcHAL) {
> > > +        sHwcHAL = new HwcHAL();
> > 
> > Oops, I cannot include "ClearOnShutdown.h" in libdisplay. I got this link
> > error:
> > ../../../dist/include/mozilla/LinkedList.h:287: error: undefined reference
> > to 'mozilla::ClearOnShutdown_Internal::sShutdownObservers'
> > 
> > Do you have other suggestion? Or I will keep the current implementation in
> > this bug.
> 
> HwcComposer2D also do not do it now. Therefore it seems ok for the time
> being.
> 
> One easier possible fix seems like the following. 
> I do not know why HwcComposer2D do not call ClearOnShutdown. We could add it
> to HwcComposer2D and from HwcComposer2D's destructor. We could manually
> clear it.

Ok, this seems a possible solution. We can handle this problem in another bug.
Blocks: 1170061
Hi mwu,
Could you please help us review part 1 and part 2? Thanks.
Comment on attachment 8612161 [details] [diff] [review]
Part 1: Create HwcDevice wrapper (v6)

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

This seems like indirection for indirection's sake. GonkDisplay's use of HWC should be straightforward and we should not make any attempt to abstract it. GonkDisplay's only purpose is to bring up enough display functionality early on to display a boot animation. Anything else is the responsibility of HwcComposer2D.
Attachment #8612161 - Flags: review?(mwu) → review-
Comment on attachment 8612163 [details] [diff] [review]
Part 2: Use wrapper in GonkDisplay (v5)

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

r- for reasons stated in part 1.
Attachment #8612163 - Flags: review?(mwu) → review-
(In reply to Michael Wu [:mwu] from comment #152)
> Comment on attachment 8612161 [details] [diff] [review]
> Part 1: Create HwcDevice wrapper (v6)
> 
> Review of attachment 8612161 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This seems like indirection for indirection's sake. GonkDisplay's use of HWC
> should be straightforward and we should not make any attempt to abstract it.
> GonkDisplay's only purpose is to bring up enough display functionality early
> on to display a boot animation. Anything else is the responsibility of
> HwcComposer2D.

OK, I will remove GonkDisplay part and move HwcHAL out of libdisplay. Thanks for your review.
Use HwcHALBase as a base class which has two subclasses, HwcICS and HwcHAL.
1. HwcICS for Android ICS
2. HwcHAL for Android JB/KK/L or later.
3. mHwc is wrappd into HwcHAL/HwcICS.
Attachment #8613959 - Attachment is obsolete: true
Use UniquePtr to access HwcHAL Object becasue only HwcComposer2D
takes the responsibility of HwcHAL.
Attachment #8613961 - Attachment is obsolete: true
Attachment #8613963 - Attachment is obsolete: true
Attachment #8613973 - Flags: review+
Move the usage of mHwc into HwcComposer2D, so we don't need
mHwc in GonkDisplayICS any more.
Attachment #8613964 - Attachment is obsolete: true
Attachment #8613971 - Flags: review?(sotaro.ikeda.g)
Attachment #8613972 - Flags: review?(sotaro.ikeda.g)
Attachment #8613975 - Flags: review?(sotaro.ikeda.g)
Hi Sotaro,
According to Comment #154, only HwcComposer2D take responsibility of HwcHAL, so I use an unique pointer to hold this object. The usage of hwcomposer in GonkDisplay is a special case(for BootAnimation) so I remove that part. Could you please review again, especially for the object creation part? Thanks.
Comment on attachment 8613971 [details] [diff] [review]
Part 1: Create HwcDevice wrapper (v8)

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

This is a update to fix muw's comment, then forward the review to mwu.
Attachment #8613971 - Flags: review?(sotaro.ikeda.g) → review?(mwu)
Comment on attachment 8613972 [details] [diff] [review]
Part 2: Use wrapper in HwcComposer2D (v8)

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

This is update to mwu's comment. Forward a review to mwu.
Attachment #8613972 - Flags: review?(sotaro.ikeda.g) → review?(mwu)
Attachment #8613975 - Flags: review?(sotaro.ikeda.g) → review+
(In reply to Michael Wu [:mwu] from comment #152)
> GonkDisplay's only purpose is to bring up enough display functionality early
> on to display a boot animation. Anything else is the responsibility of
> HwcComposer2D.

If it is correct, from my point of view, it seems better to move almost everything to HwcComposer2D and HwcHAL. Then GonkDisplay has only boot animation control code and rename to like BootAnimationManager.

Current GonkDisplay has some controls code even after boot. It is like GonkDisplay::SetEnabled(). They make the architecture confusing.
Current implementation does not care about thread context of accessing hwc hal. It is accessed from main thread and Compositor thread. It seems better to be fixed at some point. On android, hwc hal is accessed only from SurfaceFlinger thread.
(In reply to Sotaro Ikeda [:sotaro] from comment #167)
> (In reply to Michael Wu [:mwu] from comment #152)
> > GonkDisplay's only purpose is to bring up enough display functionality early
> > on to display a boot animation. Anything else is the responsibility of
> > HwcComposer2D.
> 
> If it is correct, from my point of view, it seems better to move almost
> everything to HwcComposer2D and HwcHAL. Then GonkDisplay has only boot
> animation control code 

If it is done, architectural position of GonkDisplay becomes similar to android's boot animation.
  http://androidxref.com/5.1.0_r1/xref/frameworks/base/cmds/bootanimation/
(In reply to Sotaro Ikeda [:sotaro] from comment #168)
> Current implementation does not care about thread context of accessing hwc
> hal. It is accessed from main thread and Compositor thread. It seems better
> to be fixed at some point. On android, hwc hal is accessed only from
> SurfaceFlinger thread.

Yes, I agree. For eexample, when implementing vsync eventControl, we have to make sure only one specific thread to handle this in HwcComposer2D. I remember Android uses an eventControl thread to make sure only one thread to enable/disable vsync, just like that we make sure only main thread can enable/disable vsync. This could be fixed after we move mHwc->SetPowerMode() from GonkDisplay::SetEnable into HwcHAL. Maybe we have to create a new bug to fix these things.
Comment on attachment 8613971 [details] [diff] [review]
Part 1: Create HwcDevice wrapper (v8)

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

Hi, mwu,

Could you please take a look at  part 1 and part 2 again? Thank you.

::: widget/gonk/moz.build
@@ +51,5 @@
> +        'hwchal/HwcHAL.cpp',
> +    ]
> +elif CONFIG['ANDROID_VERSION'] == '15':
> +    SOURCES += [
> +        'hwchal.HwcICS.cpp',

typo, I will fix this after reviewing.
FYI: attachment 8616206 [details] in Bug 1171671 shows an architectual relationship around GonkDisplayJB after bug 1138287 and Bug 1171671 are fixed. It might help to understand around GonkDisplayJB.
attachment 8613971 [details] [diff] [review] and attachment 8613972 [details] [diff] [review] looks a bit wired for me.

If we want to keep the following,
 - GonkDisplay loads hwc hal
 - GonkDisplayJB handle the hwc hal same way as current gecko.

the following role seems reasonable for me.
- hwc hal is always loaded by GonkDisplay. Other module do not load the hwc hal
- hwc hal's life time is controlled by GonkDisplay
- HwcHAL / HwcComposer2D uses the hwc hal. But they do not control its life time.
- HwcHAL is wrapper utility of the hwc hal.
- The HwcHAL is not a owner of the hwc hal from the above assumption.
(In reply to Sotaro Ikeda [:sotaro] from comment #173)
> attachment 8613971 [details] [diff] [review] and attachment 8613972 [details] [diff] [review]
> [details] [diff] [review] looks a bit wired for me.
> 
> If we want to keep the following,
>  - GonkDisplay loads hwc hal
>  - GonkDisplayJB handle the hwc hal same way as current gecko.
> 
> the following role seems reasonable for me.
> - hwc hal is always loaded by GonkDisplay. Other module do not load the hwc
> hal
> - hwc hal's life time is controlled by GonkDisplay
> - HwcHAL / HwcComposer2D uses the hwc hal. But they do not control its life
> time.
> - HwcHAL is wrapper utility of the hwc hal.
> - The HwcHAL is not a owner of the hwc hal from the above assumption.

In my original design, hwc hal's life time is controlled by HwcHAL, and HwcComposer2D & GonkDisplayJB only access the wrapper, HwcHAL. However, according to comment 152, it's better to access hwc hal directly in GonkDisplayJB, so the current implementation may be a bit wired because GonkDisplayICS doesn't need to access/use hwc hal in part 4 (attachment 8613975 [details] [diff] [review]).

"Who should control the life time of hwc hal" is a big problem in this bug. It's better to open hwc hal once (open twice may have problems) in one specific class. GonkDisplayJB needs hwchal after booting (in the very early phase), so your opinion is reasonable. I can let GonkDisplayJB/ICS control hwc hal's life time and the wrapper (HwcHAL) only uses it from GonkDisplay. I will upload new patches soon to let you feedback. Thank you very much.
Use HwcHALBase as a base class which has two subclasses, HwcICS and HwcHAL.
1. HwcICS for Android ICS
2. HwcHAL for Android JB/KK/L or later.
3. mHwc is wrappd into HwcHAL/HwcICS.
4. Get mHwc from GonkDisplay
Attachment #8613971 - Attachment is obsolete: true
Attachment #8613971 - Flags: review?(mwu)
Attachment #8616584 - Flags: feedback?(sotaro.ikeda.g)
Attachment #8613972 - Flags: review?(mwu) → feedback?(sotaro.ikeda.g)
Attachment #8616587 - Flags: feedback?(sotaro.ikeda.g)
Comment on attachment 8616584 [details] [diff] [review]
Part 1: Create HwcDevice wrapper (v9)

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

::: widget/gonk/hwchal/HwcHAL.cpp
@@ +26,5 @@
> +    : HwcHALBase()
> +{
> +    // Some HALs don't want to open hwc twice.
> +    // If GetDisplay already load hwc module, we don't need to load again
> +    mHwc = (HwcDevice*)GetGonkDisplay()->GetHWCDevice();

Get hwc hal from GonkDisplay directly because GonkDisplay opened hwc hal already.
Attachment #8616587 - Attachment is obsolete: true
Attachment #8616587 - Flags: feedback?(sotaro.ikeda.g)
Attachment #8616592 - Flags: feedback?(sotaro.ikeda.g)
Attachment #8616592 - Flags: feedback?(sotaro.ikeda.g) → feedback+
Attachment #8613972 - Flags: feedback?(sotaro.ikeda.g) → feedback+
Attachment #8616584 - Flags: feedback?(sotaro.ikeda.g) → feedback+
Attachment #8616584 - Flags: review?(mwu)
Attachment #8613972 - Flags: review?(mwu)
No longer blocks: 1170061
Comment on attachment 8613972 [details] [diff] [review]
Part 2: Use wrapper in HwcComposer2D (v8)

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

Sotaro is more qualified to review this. Forwarding to Sotaro per email discussion.
Attachment #8613972 - Flags: review?(mwu) → review?(sotaro.ikeda.g)
Attachment #8616584 - Flags: review?(mwu) → review?(sotaro.ikeda.g)
Comment on attachment 8613972 [details] [diff] [review]
Part 2: Use wrapper in HwcComposer2D (v8)

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

Review+ as a short term fix. mwu's comment is also valid. mwu's concern was double abstraction between hwc hal wrapper and Composer2D. Both abstract hwc hal's capability. I think that Composer2D should focus to layer transform for hwc. Others(like enabling vsync) should be moved out from HwcComposer2D.

For now, the following roles are reasonable for the following classes.
- GonkDisplay : hwc display's DisplaySurfaces holder
                After boot up, this role is mostly provided by nsScreenManagerGonk.
                But nsScreenManagerGonk could exist only after xpcom init.
                Bootanimation needs to be started before xpcom init. Therefore it exists.
- Composer2D : Transform gfx layer list to hwc layer list (high level logic)
- hwc hal wrapper : wrap hwc hal and misc.

From this point of view, HwcComposer2D's some implementations that just abstract hwc hal should be moved out from the class. In a follow-up bugs, the architectual problem needs to be addressed.
Attachment #8613972 - Flags: review?(sotaro.ikeda.g) → review+
Comment on attachment 8616584 [details] [diff] [review]
Part 1: Create HwcDevice wrapper (v9)

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

Review+ as a short term fix.
Attachment #8616584 - Flags: review?(sotaro.ikeda.g) → review+
(In reply to Sotaro Ikeda [:sotaro] from comment #180)
> Comment on attachment 8613972 [details] [diff] [review]
> Part 2: Use wrapper in HwcComposer2D (v8)
> 
> Review of attachment 8613972 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Review+ as a short term fix. mwu's comment is also valid. mwu's concern was
> double abstraction between hwc hal wrapper and Composer2D. Both abstract hwc
> hal's capability. I think that Composer2D should focus to layer transform
> for hwc. Others(like enabling vsync) should be moved out from HwcComposer2D.
> 
> For now, the following roles are reasonable for the following classes.
> - GonkDisplay : hwc display's DisplaySurfaces holder
>                 After boot up, this role is mostly provided by
> nsScreenManagerGonk.
>                 But nsScreenManagerGonk could exist only after xpcom init.
>                 Bootanimation needs to be started before xpcom init.
> Therefore it exists.
> - Composer2D : Transform gfx layer list to hwc layer list (high level logic)
> - hwc hal wrapper : wrap hwc hal and misc.
> 
> From this point of view, HwcComposer2D's some implementations that just
> abstract hwc hal should be moved out from the class. In a follow-up bugs,
> the architectual problem needs to be addressed.

Yes. I agree. HwcComposer2D should focus on high level gfx layers implementation, so we should keep refactoring it. Thanks for your clarification and explanation.
Use HwcHALBase as a base class which has two subclasses, HwcICS and HwcHAL.
1. HwcICS for Android ICS
2. HwcHAL for Android JB/KK/L or later.
3. mHwc is wrappd into HwcHAL/HwcICS.

Rebased.
Attachment #8616584 - Attachment is obsolete: true
Attachment #8624062 - Flags: review+
Attachment #8624065 - Flags: review+
Use UniquePtr to access HwcHAL Object becasue only HwcComposer2D
takes the responsibility of HwcHAL.

Fix build errors
Attachment #8624063 - Attachment is obsolete: true
Attachment #8624067 - Flags: review+
Fix ICS build errors
Attachment #8624065 - Attachment is obsolete: true
Attachment #8624156 - Flags: review+
Keywords: checkin-needed
Hi, part 4 failed to apply:

renamed 1144012 -> Bug-1144012---Part-4-Remove-the-usage-of-mHwc-from.patch
applying Bug-1144012---Part-4-Remove-the-usage-of-mHwc-from.patch
patching file widget/gonk/HwcComposer2D.cpp
Hunk #1 FAILED at 710
1 out of 2 hunks FAILED -- saving rejects to file widget/gonk/HwcComposer2D.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh Bug-1144012---Part-4-Remove-the-usage-of-mHwc-from.patch
Flags: needinfo?(boris.chiou)
Keywords: checkin-needed
(In reply to Carsten Book [:Tomcat] from comment #192)
> Hi, part 4 failed to apply:
> 
> renamed 1144012 -> Bug-1144012---Part-4-Remove-the-usage-of-mHwc-from.patch
> applying Bug-1144012---Part-4-Remove-the-usage-of-mHwc-from.patch
> patching file widget/gonk/HwcComposer2D.cpp
> Hunk #1 FAILED at 710
> 1 out of 2 hunks FAILED -- saving rejects to file
> widget/gonk/HwcComposer2D.cpp.rej
> patch failed, unable to continue (try -v)
> patch failed, rejects left in working directory
> errors during apply, please fix and refresh
> Bug-1144012---Part-4-Remove-the-usage-of-mHwc-from.patch

Got it! I will rebase it soon. Thanks.
Flags: needinfo?(boris.chiou)
Use HwcHALBase as a base class which has two subclasses, HwcICS and HwcHAL.
1. HwcICS for Android ICS
2. HwcHAL for Android JB/KK/L or later.
3. mHwc is wrappd into HwcHAL/HwcICS.

Rebased.
Attachment #8624062 - Attachment is obsolete: true
Attachment #8625544 - Flags: review+
Use UniquePtr to access HwcHAL Object becasue only HwcComposer2D
takes the responsibility of HwcHAL.

Rebased.
Attachment #8624067 - Attachment is obsolete: true
Attachment #8625545 - Flags: review+
Rebased.
Attachment #8624156 - Attachment is obsolete: true
Attachment #8625546 - Flags: review+
Rebased.
Attachment #8624066 - Attachment is obsolete: true
Attachment #8625547 - Flags: review+
Keywords: checkin-needed
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.