[RFC,2/2] drm_hwcomposer: Fall back to client compositon if the gl precompostior fails

Commit Message

John Stultz April 24, 2018, 12:06 a.m. UTC

If the gl precompositor isn't being used, we cannot accept
every layer as a device composited layer.

Thus this patch adds some extra logic in the validate function
to try to map layers to available device planes, falling back
to client side compositing if we run-out of planes.

Credit to Rob Herring, who's work this single plane patch was
originally based on.

Feedback or alternative ideas would be greatly appreciated!

Cc: Marissa Wall <[e-mail]>
Cc: Sean Paul <[e-mail]>
Cc: Dmitry Shmidt <[e-mail]>
Cc: Robert Foss <[e-mail]>
Cc: Matt Szczesiak <[e-mail]>
Cc: Liviu Dudau <[e-mail]>
Cc: David Hanna <[e-mail]>
Cc: Rob Herring <[e-mail]>
Cc: Alexandru-Cosmin Gheorghe <[e-mail]>
Cc: Alistair Strachan <[e-mail]>
Signed-off-by: John Stultz <[e-mail]>
Signed-off-by: John Stultz <[e-mail]>
---
 drmhwctwo.cpp | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

Comments

John Stultz April 24, 2018, 12:09 a.m. UTC | #1

On Mon, Apr 23, 2018 at 5:06 PM, John Stultz <[e-mail]> wrote:
> If the gl precompositor isn't being used, we cannot accept
> every layer as a device composited layer.
>
> Thus this patch adds some extra logic in the validate function
> to try to map layers to available device planes, falling back
> to client side compositing if we run-out of planes.
>
> Credit to Rob Herring, who's work this single plane patch was
> originally based on.
>
> Feedback or alternative ideas would be greatly appreciated!
>
> Cc: Marissa Wall <[e-mail]>
> Cc: Sean Paul <[e-mail]>
> Cc: Dmitry Shmidt <[e-mail]>
> Cc: Robert Foss <[e-mail]>
> Cc: Matt Szczesiak <[e-mail]>
> Cc: Liviu Dudau <[e-mail]>
> Cc: David Hanna <[e-mail]>
> Cc: Rob Herring <[e-mail]>
> Cc: Alexandru-Cosmin Gheorghe <[e-mail]>
> Cc: Alistair Strachan <[e-mail]>
> Signed-off-by: John Stultz <[e-mail]>
> Signed-off-by: John Stultz <[e-mail]>

A patch so nice, I signed it off twice! :P

Sorry, looks like I yanked and pasted one too many lines from the cc list.

-john

Alexandru-Cosmin Gheorghe April 24, 2018, 8:09 a.m. UTC | #2

On Mon, Apr 23, 2018 at 05:06:44PM -0700, John Stultz wrote:
> If the gl precompositor isn't being used, we cannot accept
> every layer as a device composited layer.
> 
> Thus this patch adds some extra logic in the validate function
> to try to map layers to available device planes, falling back
> to client side compositing if we run-out of planes.
> 
> Credit to Rob Herring, who's work this single plane patch was
> originally based on.
> 
> Feedback or alternative ideas would be greatly appreciated!
> 
> Cc: Marissa Wall <[e-mail]>
> Cc: Sean Paul <[e-mail]>
> Cc: Dmitry Shmidt <[e-mail]>
> Cc: Robert Foss <[e-mail]>
> Cc: Matt Szczesiak <[e-mail]>
> Cc: Liviu Dudau <[e-mail]>
> Cc: David Hanna <[e-mail]>
> Cc: Rob Herring <[e-mail]>
> Cc: Alexandru-Cosmin Gheorghe <[e-mail]>
> Cc: Alistair Strachan <[e-mail]>
> Signed-off-by: John Stultz <[e-mail]>
> Signed-off-by: John Stultz <[e-mail]>
> ---
>  drmhwctwo.cpp | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drmhwctwo.cpp b/drmhwctwo.cpp
> index dfca1a6..437a439 100644
> --- a/drmhwctwo.cpp
> +++ b/drmhwctwo.cpp
> @@ -686,6 +686,15 @@ HWC2::Error DrmHwcTwo::HwcDisplay::ValidateDisplay(uint32_t *num_types,
>    supported(__func__);
>    *num_types = 0;
>    *num_requests = 0;
> +  int avail_planes = primary_planes_.size() + overlay_planes_.size();
> +
> +  /*
> +   * If more layers then planes, save one plane
> +   * for client composited layers
> +   */
> +  if (avail_planes < layers_.size())
> +	avail_planes--;
> +
>    for (std::pair<const hwc2_layer_t, DrmHwcTwo::HwcLayer> &l : layers_) {
>      DrmHwcTwo::HwcLayer &layer = l.second;
>      switch (layer.sf_type()) {
> @@ -695,6 +704,15 @@ HWC2::Error DrmHwcTwo::HwcDisplay::ValidateDisplay(uint32_t *num_types,
>          layer.set_validated_type(HWC2::Composition::Client);
>          ++*num_types;
>          break;
> +      case HWC2::Composition::Device:
> +        if (!compositor_.uses_GL() && !avail_planes) {

Something is not entirely correct here, you can't assume just because
you have planes available they can be used. 
E.g Layer has rotation, but the plane doesn't support it.
This part is handled by the planning part in presentDisplay which
falls back on GPU.

I think the easiest and safe way is to just fallback to
Composition::Client whenever the GLCompositor failed to initialize.


> +          layer.set_validated_type(HWC2::Composition::Client);
> +          ++*num_types;
> +          break;
> +        } else {
> +	   avail_planes--;
> +	}
> +	/* fall through */
>        default:
>          layer.set_validated_type(layer.sf_type());
>          break;
> -- 
> 2.7.4

Stefan Schake April 24, 2018, 10:34 a.m. UTC | #3

On Tue, Apr 24, 2018 at 10:09 AM, Alexandru-Cosmin Gheorghe
<[e-mail]> wrote:
> On Mon, Apr 23, 2018 at 05:06:44PM -0700, John Stultz wrote:
>> If the gl precompositor isn't being used, we cannot accept
>> every layer as a device composited layer.
>>
>> Thus this patch adds some extra logic in the validate function
>> to try to map layers to available device planes, falling back
>> to client side compositing if we run-out of planes.
>>
>> Credit to Rob Herring, who's work this single plane patch was
>> originally based on.
>>
>> Feedback or alternative ideas would be greatly appreciated!
>>
>> Cc: Marissa Wall <[e-mail]>
>> Cc: Sean Paul <[e-mail]>
>> Cc: Dmitry Shmidt <[e-mail]>
>> Cc: Robert Foss <[e-mail]>
>> Cc: Matt Szczesiak <[e-mail]>
>> Cc: Liviu Dudau <[e-mail]>
>> Cc: David Hanna <[e-mail]>
>> Cc: Rob Herring <[e-mail]>
>> Cc: Alexandru-Cosmin Gheorghe <[e-mail]>
>> Cc: Alistair Strachan <[e-mail]>
>> Signed-off-by: John Stultz <[e-mail]>
>> Signed-off-by: John Stultz <[e-mail]>
>> ---
>>  drmhwctwo.cpp | 18 ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>>
>> diff --git a/drmhwctwo.cpp b/drmhwctwo.cpp
>> index dfca1a6..437a439 100644
>> --- a/drmhwctwo.cpp
>> +++ b/drmhwctwo.cpp
>> @@ -686,6 +686,15 @@ HWC2::Error DrmHwcTwo::HwcDisplay::ValidateDisplay(uint32_t *num_types,
>>    supported(__func__);
>>    *num_types = 0;
>>    *num_requests = 0;
>> +  int avail_planes = primary_planes_.size() + overlay_planes_.size();
>> +
>> +  /*
>> +   * If more layers then planes, save one plane
>> +   * for client composited layers
>> +   */
>> +  if (avail_planes < layers_.size())
>> +     avail_planes--;
>> +
>>    for (std::pair<const hwc2_layer_t, DrmHwcTwo::HwcLayer> &l : layers_) {
>>      DrmHwcTwo::HwcLayer &layer = l.second;
>>      switch (layer.sf_type()) {
>> @@ -695,6 +704,15 @@ HWC2::Error DrmHwcTwo::HwcDisplay::ValidateDisplay(uint32_t *num_types,
>>          layer.set_validated_type(HWC2::Composition::Client);
>>          ++*num_types;
>>          break;
>> +      case HWC2::Composition::Device:
>> +        if (!compositor_.uses_GL() && !avail_planes) {
>
> Something is not entirely correct here, you can't assume just because
> you have planes available they can be used.
> E.g Layer has rotation, but the plane doesn't support it.
> This part is handled by the planning part in presentDisplay which
> falls back on GPU.
>
> I think the easiest and safe way is to just fallback to
> Composition::Client whenever the GLCompositor failed to initialize.
>

I agree, this would only work out for the single plane case where you end
up using client composition for everything.

In the spirit of DRM what we would probably want is to atomic test a
composition, and if that fails we can still fallback to client or GL
compositor depending on its availability. And then in a far far away
future we might even do a few iterations simplifying our composition until
it atomic tests correctly so we can still offload some "simple" parts
like the cursor.

Thanks,
Stefan

John Stultz April 24, 2018, 6:38 p.m. UTC | #4

On Tue, Apr 24, 2018 at 3:34 AM, Stefan Schake <[e-mail]> wrote:
> On Tue, Apr 24, 2018 at 10:09 AM, Alexandru-Cosmin Gheorghe
> <[e-mail]> wrote:
>> On Mon, Apr 23, 2018 at 05:06:44PM -0700, John Stultz wrote:
>>> @@ -695,6 +704,15 @@ HWC2::Error DrmHwcTwo::HwcDisplay::ValidateDisplay(uint32_t *num_types,
>>>          layer.set_validated_type(HWC2::Composition::Client);
>>>          ++*num_types;
>>>          break;
>>> +      case HWC2::Composition::Device:
>>> +        if (!compositor_.uses_GL() && !avail_planes) {
>>
>> Something is not entirely correct here, you can't assume just because
>> you have planes available they can be used.
>> E.g Layer has rotation, but the plane doesn't support it.
>> This part is handled by the planning part in presentDisplay which
>> falls back on GPU.

Ah. Thanks for the correction here!

This is part of the disconnect I have been having with the
drm_hwcomposer logic. The plan/validate step doesn't really do either,
instead it defers all the checking/planning to the set/present step.
But unfortunately it seems if we want to use the surfaceflinger gl
composer, we need to mark the layers as client rendered in the
validate function.

>> I think the easiest and safe way is to just fallback to
>> Composition::Client whenever the GLCompositor failed to initialize.
>>
>
> I agree, this would only work out for the single plane case where you end
> up using client composition for everything.

True, but maybe as a first step we simply fall back on the
glcompositor, just so we have something displaying in that case (right
now nothing gets displayed).

> In the spirit of DRM what we would probably want is to atomic test a
> composition, and if that fails we can still fallback to client or GL
> compositor depending on its availability. And then in a far far away
> future we might even do a few iterations simplifying our composition until
> it atomic tests correctly so we can still offload some "simple" parts
> like the cursor.

This sounds reasonable. I suspect it will take some heavier rework of
the drm_hwcomposer to move such logic to the validate step.

Marissa/Sean: Any objections here? Doing more planning in the validate
step sounds like it aligns closer to the HWC2 interface goals, but I
don't want to have to relearn-the-hard-way any lessons that resulted
in the current mode of accept it all and sort it in present.

In the meantime I'll drop the flawed plane/layer allocation from this
patch and resend.

thanks
-john

Sean Paul April 24, 2018, 7:40 p.m. UTC | #5

On Tue, Apr 24, 2018 at 2:38 PM John Stultz <[e-mail]> wrote:

> On Tue, Apr 24, 2018 at 3:34 AM, Stefan Schake <[e-mail]> wrote:
> > On Tue, Apr 24, 2018 at 10:09 AM, Alexandru-Cosmin Gheorghe
> > <[e-mail]> wrote:
> >> On Mon, Apr 23, 2018 at 05:06:44PM -0700, John Stultz wrote:
> >>> @@ -695,6 +704,15 @@ HWC2::Error
DrmHwcTwo::HwcDisplay::ValidateDisplay(uint32_t *num_types,
> >>>          layer.set_validated_type(HWC2::Composition::Client);
> >>>          ++*num_types;
> >>>          break;
> >>> +      case HWC2::Composition::Device:
> >>> +        if (!compositor_.uses_GL() && !avail_planes) {
> >>
> >> Something is not entirely correct here, you can't assume just because
> >> you have planes available they can be used.
> >> E.g Layer has rotation, but the plane doesn't support it.
> >> This part is handled by the planning part in presentDisplay which
> >> falls back on GPU.

> Ah. Thanks for the correction here!

> This is part of the disconnect I have been having with the
> drm_hwcomposer logic. The plan/validate step doesn't really do either,
> instead it defers all the checking/planning to the set/present step.
> But unfortunately it seems if we want to use the surfaceflinger gl
> composer, we need to mark the layers as client rendered in the
> validate function.

> >> I think the easiest and safe way is to just fallback to
> >> Composition::Client whenever the GLCompositor failed to initialize.
> >>
> >
> > I agree, this would only work out for the single plane case where you
end
> > up using client composition for everything.

> True, but maybe as a first step we simply fall back on the
> glcompositor, just so we have something displaying in that case (right
> now nothing gets displayed).

> > In the spirit of DRM what we would probably want is to atomic test a
> > composition, and if that fails we can still fallback to client or GL
> > compositor depending on its availability. And then in a far far away
> > future we might even do a few iterations simplifying our composition
until
> > it atomic tests correctly so we can still offload some "simple" parts
> > like the cursor.

> This sounds reasonable. I suspect it will take some heavier rework of
> the drm_hwcomposer to move such logic to the validate step.

> Marissa/Sean: Any objections here? Doing more planning in the validate
> step sounds like it aligns closer to the HWC2 interface goals, but I
> don't want to have to relearn-the-hard-way any lessons that resulted
> in the current mode of accept it all and sort it in present.


No objections, this needs to be done. As you mentioned, this is going to
require a good amount of code movement. I think what we're seeing here as
well, as with the writeback series, are growing pains. When this was
written, it was for HWC1 and the GLCompositor was the only alternate
compositor. Now that we're on HWC2 and discussing using writeback,
SurfaceFlinger, and possibly GLCompositor, we need something more robust.

Sean

> In the meantime I'll drop the flawed plane/layer allocation from this
> patch and resend.

> thanks
> -john

Alistair Strachan April 25, 2018, 6:15 p.m. UTC | #6

Hi John,

On Tue, Apr 24, 2018 at 11:38 AM John Stultz <[e-mail]> wrote:

> On Tue, Apr 24, 2018 at 3:34 AM, Stefan Schake <[e-mail]> wrote:
> > On Tue, Apr 24, 2018 at 10:09 AM, Alexandru-Cosmin Gheorghe
> > <[e-mail]> wrote:
> >> On Mon, Apr 23, 2018 at 05:06:44PM -0700, John Stultz wrote:
> >>> @@ -695,6 +704,15 @@ HWC2::Error
> DrmHwcTwo::HwcDisplay::ValidateDisplay(uint32_t *num_types,
> >>>          layer.set_validated_type(HWC2::Composition::Client);
> >>>          ++*num_types;
> >>>          break;
> >>> +      case HWC2::Composition::Device:
> >>> +        if (!compositor_.uses_GL() && !avail_planes) {
> >>
> >> Something is not entirely correct here, you can't assume just because
> >> you have planes available they can be used.
> >> E.g Layer has rotation, but the plane doesn't support it.
> >> This part is handled by the planning part in presentDisplay which
> >> falls back on GPU.
>
> Ah. Thanks for the correction here!
>
> This is part of the disconnect I have been having with the
> drm_hwcomposer logic. The plan/validate step doesn't really do either,
> instead it defers all the checking/planning to the set/present step.
> But unfortunately it seems if we want to use the surfaceflinger gl
> composer, we need to mark the layers as client rendered in the
> validate function.
>
> >> I think the easiest and safe way is to just fallback to
> >> Composition::Client whenever the GLCompositor failed to initialize.
> >>
> >
> > I agree, this would only work out for the single plane case where you end
> > up using client composition for everything.
>
> True, but maybe as a first step we simply fall back on the
> glcompositor, just so we have something displaying in that case (right
> now nothing gets displayed).
>
> > In the spirit of DRM what we would probably want is to atomic test a
> > composition, and if that fails we can still fallback to client or GL
> > compositor depending on its availability. And then in a far far away
> > future we might even do a few iterations simplifying our composition
> until
> > it atomic tests correctly so we can still offload some "simple" parts
> > like the cursor.
>
> This sounds reasonable. I suspect it will take some heavier rework of
> the drm_hwcomposer to move such logic to the validate step.
>
> Marissa/Sean: Any objections here? Doing more planning in the validate
> step sounds like it aligns closer to the HWC2 interface goals, but I
> don't want to have to relearn-the-hard-way any lessons that resulted
> in the current mode of accept it all and sort it in present.
>

I agree. This was the design goal behind HWC_GEOMETRY_CHANGED /
HWC2_PFN_GET_CHANGED_COMPOSITION_TYPES. The validate() function should know
that the composition can be set in HWC2_PFN_PRESENT_DISPLAY so there should
be no need for an internal GL fallback. This feedback to surfaceflinger
also allows for small state optimizations that can't be done in the
fallback.

I think validating compositions w/ atomic in kernel is a valid way to do it.

I would suggest that code is also refactored a little so that the
validation/planning could also be done in userspace, maybe by linking a
static library. This would allow drivers without a way to do it in kernel
to keep working.

In the meantime I'll drop the flawed plane/layer allocation from this
> patch and resend.
>
> thanks
> -john
>

Patch

diff --git a/drmhwctwo.cpp b/drmhwctwo.cpp
index dfca1a6..437a439 100644
--- a/drmhwctwo.cpp
+++ b/drmhwctwo.cpp
@@ -686,6 +686,15 @@  HWC2::Error DrmHwcTwo::HwcDisplay::ValidateDisplay(uint32_t *num_types,
   supported(__func__);
   *num_types = 0;
   *num_requests = 0;
+  int avail_planes = primary_planes_.size() + overlay_planes_.size();
+
+  /*
+   * If more layers then planes, save one plane
+   * for client composited layers
+   */
+  if (avail_planes < layers_.size())
+	avail_planes--;
+
   for (std::pair<const hwc2_layer_t, DrmHwcTwo::HwcLayer> &l : layers_) {
     DrmHwcTwo::HwcLayer &layer = l.second;
     switch (layer.sf_type()) {
@@ -695,6 +704,15 @@  HWC2::Error DrmHwcTwo::HwcDisplay::ValidateDisplay(uint32_t *num_types,
         layer.set_validated_type(HWC2::Composition::Client);
         ++*num_types;
         break;
+      case HWC2::Composition::Device:
+        if (!compositor_.uses_GL() && !avail_planes) {
+          layer.set_validated_type(HWC2::Composition::Client);
+          ++*num_types;
+          break;
+        } else {
+	   avail_planes--;
+	}
+	/* fall through */
       default:
         layer.set_validated_type(layer.sf_type());
         break;