dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [RFC][PATCH 1/2] drm_hwcomposer: Cleanup gl precompositor init and provide uses_GL flag
@ 2018-04-24  0:06 John Stultz
  2018-04-24  0:06 ` [RFC][PATCH 2/2] drm_hwcomposer: Fall back to client compositon if the gl precompostior fails John Stultz
  2018-04-24  1:30 ` [RFC][PATCH 1/2] drm_hwcomposer: Cleanup gl precompositor init and provide uses_GL flag Rob Herring
  0 siblings, 2 replies; 9+ messages in thread
From: John Stultz @ 2018-04-24  0:06 UTC (permalink / raw)
  To: dri-devel
  Cc: Rob Herring, Matt Szczesiak, Dmitry Shmidt, Sean Paul,
	Robert Foss, Alexandru-Cosmin Gheorghe, Liviu Dudau,
	Alistair Strachan, Marissa Wall, David Hanna

The drm_hwcomposer has its own GL pre-compositor which is used
to squish layers when there are more layers then planes on the
display hardware. In many ways this duplicates the client-side
GL compositing that is done in SurfaceFlinger, but in theory can
be more highly optimized for the hardware.

Unfortunately, due to these optimizations, the drm_hwcomposer's
pre-compositor becomes somewhat hardware specific (originally
targeting nvidia hardware, I believe).

So on some hardware, the gl precompositor may not actually
initialize due to hardware missing features, or the hardware
supporting different shader APIs.

Rather then try to rework the drm_hwcomposers precompositor
to be more generic, I instead suggest that when the
precompositor fails to initialize, we simply fall back to the
already more widely compatible client compositor in
SurfaceFlinger.

Thus, this patch cleans up some of the precompositor
initialization, which didn't handle failures well.

Feedback or alternative ideas would be greatly appreciated!

Cc: Marissa Wall <marissaw@google.com>
Cc: Sean Paul <seanpaul@google.com>
Cc: Dmitry Shmidt <dimitrysh@google.com>
Cc: Robert Foss <robert.foss@collabora.com>
Cc: Matt Szczesiak <matt.szczesiak@arm.com>
Cc: Liviu Dudau <Liviu.Dudau@arm.com>
Cc: David Hanna <david.hanna11@gmail.com>
Cc: Rob Herring <rob.herring@linaro.org>
Cc: Alexandru-Cosmin Gheorghe <Alexandru-Cosmin.Gheorghe@arm.com>
Cc: Alistair Strachan <astrachan@google.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drmdisplaycompositor.cpp | 40 +++++++++++++++++++++-------------------
 drmdisplaycompositor.h   |  3 +++
 2 files changed, 24 insertions(+), 19 deletions(-)

diff --git a/drmdisplaycompositor.cpp b/drmdisplaycompositor.cpp
index e556e86..5c6bf9b 100644
--- a/drmdisplaycompositor.cpp
+++ b/drmdisplaycompositor.cpp
@@ -222,6 +222,13 @@ int DrmDisplayCompositor::Init(DrmResources *drm, int display) {
     return ret;
   }
 
+  pre_compositor_.reset(new GLWorkerCompositor());
+  ret = pre_compositor_->Init();
+  if (ret) {
+    ALOGE("Failed to initialize OpenGL compositor %d", ret);
+    pre_compositor_.reset();
+  }
+
   initialized_ = true;
   return 0;
 }
@@ -294,14 +301,16 @@ int DrmDisplayCompositor::ApplySquash(DrmDisplayComposition *display_comp) {
   }
 
   std::vector<DrmCompositionRegion> &regions = display_comp->squash_regions();
-  ret = pre_compositor_->Composite(display_comp->layers().data(),
+  if (pre_compositor_) {
+    ret = pre_compositor_->Composite(display_comp->layers().data(),
                                    regions.data(), regions.size(), fb.buffer(),
                                    display_comp->importer());
-  pre_compositor_->Finish();
+    pre_compositor_->Finish();
 
-  if (ret) {
-    ALOGE("Failed to squash layers");
-    return ret;
+    if (ret) {
+      ALOGE("Failed to squash layers");
+      return ret;
+    }
   }
 
   ret = display_comp->CreateNextTimelineFence();
@@ -328,14 +337,16 @@ int DrmDisplayCompositor::ApplyPreComposite(
   }
 
   std::vector<DrmCompositionRegion> &regions = display_comp->pre_comp_regions();
-  ret = pre_compositor_->Composite(display_comp->layers().data(),
+  if (pre_compositor_) {
+    ret = pre_compositor_->Composite(display_comp->layers().data(),
                                    regions.data(), regions.size(), fb.buffer(),
                                    display_comp->importer());
-  pre_compositor_->Finish();
+    pre_compositor_->Finish();
 
-  if (ret) {
-    ALOGE("Failed to pre-composite layers");
-    return ret;
+    if (ret) {
+      ALOGE("Failed to pre-composite layers");
+      return ret;
+    }
   }
 
   ret = display_comp->CreateNextTimelineFence();
@@ -395,15 +406,6 @@ int DrmDisplayCompositor::PrepareFrame(DrmDisplayComposition *display_comp) {
   std::vector<DrmCompositionRegion> &pre_comp_regions =
       display_comp->pre_comp_regions();
 
-  if (!pre_compositor_) {
-    pre_compositor_.reset(new GLWorkerCompositor());
-    int ret = pre_compositor_->Init();
-    if (ret) {
-      ALOGE("Failed to initialize OpenGL compositor %d", ret);
-      return ret;
-    }
-  }
-
   int squash_layer_index = -1;
   if (squash_regions.size() > 0) {
     squash_framebuffer_index_ = (squash_framebuffer_index_ + 1) % 2;
diff --git a/drmdisplaycompositor.h b/drmdisplaycompositor.h
index f1965fb..ed6c5f9 100644
--- a/drmdisplaycompositor.h
+++ b/drmdisplaycompositor.h
@@ -98,6 +98,9 @@ class DrmDisplayCompositor {
     return &squash_state_;
   }
 
+  bool uses_GL() {
+    return !!pre_compositor_;
+  }
  private:
   struct ModeState {
     bool needs_modeset = false;
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [RFC][PATCH 2/2] drm_hwcomposer: Fall back to client compositon if the gl precompostior fails
  2018-04-24  0:06 [RFC][PATCH 1/2] drm_hwcomposer: Cleanup gl precompositor init and provide uses_GL flag John Stultz
@ 2018-04-24  0:06 ` John Stultz
  2018-04-24  0:09   ` John Stultz
  2018-04-24  8:09   ` Alexandru-Cosmin Gheorghe
  2018-04-24  1:30 ` [RFC][PATCH 1/2] drm_hwcomposer: Cleanup gl precompositor init and provide uses_GL flag Rob Herring
  1 sibling, 2 replies; 9+ messages in thread
From: John Stultz @ 2018-04-24  0:06 UTC (permalink / raw)
  To: dri-devel
  Cc: Rob Herring, Matt Szczesiak, Dmitry Shmidt, Sean Paul,
	Robert Foss, Alexandru-Cosmin Gheorghe, Liviu Dudau,
	Alistair Strachan, Marissa Wall, David Hanna

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 <marissaw@google.com>
Cc: Sean Paul <seanpaul@google.com>
Cc: Dmitry Shmidt <dimitrysh@google.com>
Cc: Robert Foss <robert.foss@collabora.com>
Cc: Matt Szczesiak <matt.szczesiak@arm.com>
Cc: Liviu Dudau <Liviu.Dudau@arm.com>
Cc: David Hanna <david.hanna11@gmail.com>
Cc: Rob Herring <rob.herring@linaro.org>
Cc: Alexandru-Cosmin Gheorghe <Alexandru-Cosmin.Gheorghe@arm.com>
Cc: Alistair Strachan <astrachan@google.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 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) {
+          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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [RFC][PATCH 2/2] drm_hwcomposer: Fall back to client compositon if the gl precompostior fails
  2018-04-24  0:06 ` [RFC][PATCH 2/2] drm_hwcomposer: Fall back to client compositon if the gl precompostior fails John Stultz
@ 2018-04-24  0:09   ` John Stultz
  2018-04-24  8:09   ` Alexandru-Cosmin Gheorghe
  1 sibling, 0 replies; 9+ messages in thread
From: John Stultz @ 2018-04-24  0:09 UTC (permalink / raw)
  To: dri-devel
  Cc: Rob Herring, Matt Szczesiak, Dmitry Shmidt, Sean Paul,
	Robert Foss, Alexandru-Cosmin Gheorghe, Liviu Dudau,
	Alistair Strachan, Marissa Wall, David Hanna

On Mon, Apr 23, 2018 at 5:06 PM, John Stultz <john.stultz@linaro.org> 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 <marissaw@google.com>
> Cc: Sean Paul <seanpaul@google.com>
> Cc: Dmitry Shmidt <dimitrysh@google.com>
> Cc: Robert Foss <robert.foss@collabora.com>
> Cc: Matt Szczesiak <matt.szczesiak@arm.com>
> Cc: Liviu Dudau <Liviu.Dudau@arm.com>
> Cc: David Hanna <david.hanna11@gmail.com>
> Cc: Rob Herring <rob.herring@linaro.org>
> Cc: Alexandru-Cosmin Gheorghe <Alexandru-Cosmin.Gheorghe@arm.com>
> Cc: Alistair Strachan <astrachan@google.com>
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> Signed-off-by: John Stultz <john.stultz@linaro.org>

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC][PATCH 1/2] drm_hwcomposer: Cleanup gl precompositor init and provide uses_GL flag
  2018-04-24  0:06 [RFC][PATCH 1/2] drm_hwcomposer: Cleanup gl precompositor init and provide uses_GL flag John Stultz
  2018-04-24  0:06 ` [RFC][PATCH 2/2] drm_hwcomposer: Fall back to client compositon if the gl precompostior fails John Stultz
@ 2018-04-24  1:30 ` Rob Herring
  1 sibling, 0 replies; 9+ messages in thread
From: Rob Herring @ 2018-04-24  1:30 UTC (permalink / raw)
  To: John Stultz
  Cc: Matt Szczesiak, Dmitry Shmidt, Sean Paul, Robert Foss,
	Alexandru-Cosmin Gheorghe, Liviu Dudau, Alistair Strachan,
	dri-devel, Marissa Wall, David Hanna

Please use "PATCH hwc" for drm_hwcomposer patches.

On Mon, Apr 23, 2018 at 7:06 PM, John Stultz <john.stultz@linaro.org> wrote:
> The drm_hwcomposer has its own GL pre-compositor which is used
> to squish layers when there are more layers then planes on the
> display hardware. In many ways this duplicates the client-side
> GL compositing that is done in SurfaceFlinger, but in theory can
> be more highly optimized for the hardware.
>
> Unfortunately, due to these optimizations, the drm_hwcomposer's
> pre-compositor becomes somewhat hardware specific (originally
> targeting nvidia hardware, I believe).
>
> So on some hardware, the gl precompositor may not actually
> initialize due to hardware missing features, or the hardware
> supporting different shader APIs.
>
> Rather then try to rework the drm_hwcomposers precompositor
> to be more generic, I instead suggest that when the
> precompositor fails to initialize, we simply fall back to the
> already more widely compatible client compositor in
> SurfaceFlinger.
>
> Thus, this patch cleans up some of the precompositor
> initialization, which didn't handle failures well.
>
> Feedback or alternative ideas would be greatly appreciated!
>
> Cc: Marissa Wall <marissaw@google.com>
> Cc: Sean Paul <seanpaul@google.com>
> Cc: Dmitry Shmidt <dimitrysh@google.com>
> Cc: Robert Foss <robert.foss@collabora.com>
> Cc: Matt Szczesiak <matt.szczesiak@arm.com>
> Cc: Liviu Dudau <Liviu.Dudau@arm.com>
> Cc: David Hanna <david.hanna11@gmail.com>
> Cc: Rob Herring <rob.herring@linaro.org>
> Cc: Alexandru-Cosmin Gheorghe <Alexandru-Cosmin.Gheorghe@arm.com>
> Cc: Alistair Strachan <astrachan@google.com>
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>  drmdisplaycompositor.cpp | 40 +++++++++++++++++++++-------------------
>  drmdisplaycompositor.h   |  3 +++
>  2 files changed, 24 insertions(+), 19 deletions(-)

Both patches look good to me.

Reviewed-by: Rob Herring <robh@kernel.org>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC][PATCH 2/2] drm_hwcomposer: Fall back to client compositon if the gl precompostior fails
  2018-04-24  0:06 ` [RFC][PATCH 2/2] drm_hwcomposer: Fall back to client compositon if the gl precompostior fails John Stultz
  2018-04-24  0:09   ` John Stultz
@ 2018-04-24  8:09   ` Alexandru-Cosmin Gheorghe
  2018-04-24 10:34     ` Stefan Schake
  1 sibling, 1 reply; 9+ messages in thread
From: Alexandru-Cosmin Gheorghe @ 2018-04-24  8:09 UTC (permalink / raw)
  To: John Stultz
  Cc: Rob Herring, Matt Szczesiak, Dmitry Shmidt, Sean Paul,
	Robert Foss, Liviu Dudau, Alistair Strachan, dri-devel,
	Marissa Wall, David Hanna, nd

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 <marissaw@google.com>
> Cc: Sean Paul <seanpaul@google.com>
> Cc: Dmitry Shmidt <dimitrysh@google.com>
> Cc: Robert Foss <robert.foss@collabora.com>
> Cc: Matt Szczesiak <matt.szczesiak@arm.com>
> Cc: Liviu Dudau <Liviu.Dudau@arm.com>
> Cc: David Hanna <david.hanna11@gmail.com>
> Cc: Rob Herring <rob.herring@linaro.org>
> Cc: Alexandru-Cosmin Gheorghe <Alexandru-Cosmin.Gheorghe@arm.com>
> Cc: Alistair Strachan <astrachan@google.com>
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>  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

-- 
Cheers,
Alex G
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC][PATCH 2/2] drm_hwcomposer: Fall back to client compositon if the gl precompostior fails
  2018-04-24  8:09   ` Alexandru-Cosmin Gheorghe
@ 2018-04-24 10:34     ` Stefan Schake
  2018-04-24 18:38       ` John Stultz
  0 siblings, 1 reply; 9+ messages in thread
From: Stefan Schake @ 2018-04-24 10:34 UTC (permalink / raw)
  To: Alexandru-Cosmin Gheorghe
  Cc: Rob Herring, Matt Szczesiak, Dmitry Shmidt, Sean Paul,
	Robert Foss, Liviu Dudau, Alistair Strachan, dri-devel,
	Marissa Wall, David Hanna, nd

On Tue, Apr 24, 2018 at 10:09 AM, Alexandru-Cosmin Gheorghe
<Alexandru-Cosmin.Gheorghe@arm.com> 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 <marissaw@google.com>
>> Cc: Sean Paul <seanpaul@google.com>
>> Cc: Dmitry Shmidt <dimitrysh@google.com>
>> Cc: Robert Foss <robert.foss@collabora.com>
>> Cc: Matt Szczesiak <matt.szczesiak@arm.com>
>> Cc: Liviu Dudau <Liviu.Dudau@arm.com>
>> Cc: David Hanna <david.hanna11@gmail.com>
>> Cc: Rob Herring <rob.herring@linaro.org>
>> Cc: Alexandru-Cosmin Gheorghe <Alexandru-Cosmin.Gheorghe@arm.com>
>> Cc: Alistair Strachan <astrachan@google.com>
>> Signed-off-by: John Stultz <john.stultz@linaro.org>
>> Signed-off-by: John Stultz <john.stultz@linaro.org>
>> ---
>>  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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC][PATCH 2/2] drm_hwcomposer: Fall back to client compositon if the gl precompostior fails
  2018-04-24 10:34     ` Stefan Schake
@ 2018-04-24 18:38       ` John Stultz
  2018-04-24 19:40         ` Sean Paul
  2018-04-25 18:15         ` Alistair Strachan
  0 siblings, 2 replies; 9+ messages in thread
From: John Stultz @ 2018-04-24 18:38 UTC (permalink / raw)
  To: Stefan Schake
  Cc: Rob Herring, Matt Szczesiak, Dmitry Shmidt, Sean Paul,
	Robert Foss, Alexandru-Cosmin Gheorghe, Liviu Dudau,
	Alistair Strachan, dri-devel, Marissa Wall, David Hanna, nd

On Tue, Apr 24, 2018 at 3:34 AM, Stefan Schake <stschake@gmail.com> wrote:
> On Tue, Apr 24, 2018 at 10:09 AM, Alexandru-Cosmin Gheorghe
> <Alexandru-Cosmin.Gheorghe@arm.com> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC][PATCH 2/2] drm_hwcomposer: Fall back to client compositon if the gl precompostior fails
  2018-04-24 18:38       ` John Stultz
@ 2018-04-24 19:40         ` Sean Paul
  2018-04-25 18:15         ` Alistair Strachan
  1 sibling, 0 replies; 9+ messages in thread
From: Sean Paul @ 2018-04-24 19:40 UTC (permalink / raw)
  To: John Stultz
  Cc: Rob Herring, Matt Szczesiak, Dmitry Shmidt, Robert Foss,
	Alexandru-Cosmin.Gheorghe, Liviu Dudau, astrachan, dri-devel,
	stschake, Marissa Wall, David Hanna, nd

On Tue, Apr 24, 2018 at 2:38 PM John Stultz <john.stultz@linaro.org> wrote:

> On Tue, Apr 24, 2018 at 3:34 AM, Stefan Schake <stschake@gmail.com> wrote:
> > On Tue, Apr 24, 2018 at 10:09 AM, Alexandru-Cosmin Gheorghe
> > <Alexandru-Cosmin.Gheorghe@arm.com> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC][PATCH 2/2] drm_hwcomposer: Fall back to client compositon if the gl precompostior fails
  2018-04-24 18:38       ` John Stultz
  2018-04-24 19:40         ` Sean Paul
@ 2018-04-25 18:15         ` Alistair Strachan
  1 sibling, 0 replies; 9+ messages in thread
From: Alistair Strachan @ 2018-04-25 18:15 UTC (permalink / raw)
  To: John Stultz
  Cc: rob.herring, matt.szczesiak, Dmitry Shmidt, seanpaul,
	robert.foss, Alexandru-Cosmin.Gheorghe, Liviu.Dudau, dri-devel,
	stschake, Marissa Wall, david.hanna11, nd


[-- Attachment #1.1: Type: text/plain, Size: 3398 bytes --]

Hi John,

On Tue, Apr 24, 2018 at 11:38 AM John Stultz <john.stultz@linaro.org> wrote:

> On Tue, Apr 24, 2018 at 3:34 AM, Stefan Schake <stschake@gmail.com> wrote:
> > On Tue, Apr 24, 2018 at 10:09 AM, Alexandru-Cosmin Gheorghe
> > <Alexandru-Cosmin.Gheorghe@arm.com> 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
>

[-- Attachment #1.2: Type: text/html, Size: 6362 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2018-04-25 18:16 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-24  0:06 [RFC][PATCH 1/2] drm_hwcomposer: Cleanup gl precompositor init and provide uses_GL flag John Stultz
2018-04-24  0:06 ` [RFC][PATCH 2/2] drm_hwcomposer: Fall back to client compositon if the gl precompostior fails John Stultz
2018-04-24  0:09   ` John Stultz
2018-04-24  8:09   ` Alexandru-Cosmin Gheorghe
2018-04-24 10:34     ` Stefan Schake
2018-04-24 18:38       ` John Stultz
2018-04-24 19:40         ` Sean Paul
2018-04-25 18:15         ` Alistair Strachan
2018-04-24  1:30 ` [RFC][PATCH 1/2] drm_hwcomposer: Cleanup gl precompositor init and provide uses_GL flag Rob Herring

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).