Re: [RFC PATCH hwc] drm_hwcomposer: set CRTC background color when available

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Feb 22, 2018 at 04:54:08AM +0100, Stefan Schake wrote:
> Android assumes an implicit black background layer is always present
> behind all layers it specifies for composition. drm_hwcomposer currently
> punts responsibility for this to the kernel/DRM platform and puts layers
> with per-pixel alpha content on the primary plane when requested.
> 
> On some platforms (e.g. VC4) a background color fill has a cycle cost for
> the hardware composer and is not enabled by default. Instead, userland can
> request a background color through a CRTC property. Use this property to
> specify the implicit black background Android expects.
> 
> Signed-off-by: Stefan Schake <stschake@gmail.com>
> ---
> Kernel changes for this (background_color) are available here:
> 
> https://github.com/stschake/linux/commits/background-upstream
> 
> Sending as RFC because I'm not entirely clear on whose responsibility
> this should be, on most DRM drivers it seems to be implicit. I think
> a case could also be made that VC4 should not accept alpha formats on
> the lowest layer or enable background color fill when given one anyway.
> On the other hand, userland control over background color seems desirable
> regardless and is a feature of multiple hardware composers (i915, vc4, omap).

I see you've already typed vc4 patches to address, just wanted to clarify
this here: The assumption is that all drivers already have a background
color of black/0. The background color property would be to set a
different background color than black.

If the driver has undefined behaviour for instead of an assumed background
color of black, then I think that's a driver bug.

See https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#plane-composition-properties

"(e.g. for the background color, which currently is not exposed and
assumed to be black)"

I'm still hoping for some artist to create a nice svg that explains the
kms composition model with a nice graphic ...

Cheers, Daniel

> 
>  drmcrtc.cpp              | 11 +++++++++++
>  drmcrtc.h                |  2 ++
>  drmdisplaycompositor.cpp | 15 +++++++++++++++
>  3 files changed, 28 insertions(+)
> 
> diff --git a/drmcrtc.cpp b/drmcrtc.cpp
> index 1b354fe..d7bcd7a 100644
> --- a/drmcrtc.cpp
> +++ b/drmcrtc.cpp
> @@ -52,6 +52,13 @@ int DrmCrtc::Init() {
>      ALOGE("Failed to get OUT_FENCE_PTR property");
>      return ret;
>    }
> +
> +  ret = drm_->GetCrtcProperty(*this, "background_color",
> +                              &background_color_property_);
> +  if (ret) {
> +    ALOGI("Failed to get background_color property");
> +    // This is an optional property
> +  }
>    return 0;
>  }
>  
> @@ -86,4 +93,8 @@ const DrmProperty &DrmCrtc::mode_property() const {
>  const DrmProperty &DrmCrtc::out_fence_ptr_property() const {
>    return out_fence_ptr_property_;
>  }
> +
> +const DrmProperty &DrmCrtc::background_color_property() const {
> +  return background_color_property_;
> +}
>  }
> diff --git a/drmcrtc.h b/drmcrtc.h
> index c5a5599..704573a 100644
> --- a/drmcrtc.h
> +++ b/drmcrtc.h
> @@ -46,6 +46,7 @@ class DrmCrtc {
>    const DrmProperty &active_property() const;
>    const DrmProperty &mode_property() const;
>    const DrmProperty &out_fence_ptr_property() const;
> +  const DrmProperty &background_color_property() const;
>  
>   private:
>    DrmResources *drm_;
> @@ -59,6 +60,7 @@ class DrmCrtc {
>    DrmProperty active_property_;
>    DrmProperty mode_property_;
>    DrmProperty out_fence_ptr_property_;
> +  DrmProperty background_color_property_;
>  };
>  }
>  
> diff --git a/drmdisplaycompositor.cpp b/drmdisplaycompositor.cpp
> index e556e86..69c52dd 100644
> --- a/drmdisplaycompositor.cpp
> +++ b/drmdisplaycompositor.cpp
> @@ -527,6 +527,21 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
>        return ret;
>      }
>  
> +    if (crtc->background_color_property().id() != 0) {
> +      // Background color is specified as a 16 bit per channel RGBA value.
> +      // Set a fully opaque black background as Android HWC expects.
> +      const uint64_t background_color = 0xFFFF;
> +
> +      ret = drmModeAtomicAddProperty(pset, crtc->id(),
> +                                     crtc->background_color_property().id(),
> +                                     background_color) < 0;
> +      if (ret) {
> +        ALOGE("Failed to add CRTC background_color to pset: %d", ret);
> +        drmModeAtomicFree(pset);
> +        return ret;
> +      }
> +    }
> +
>      ret = drmModeAtomicAddProperty(pset, crtc->id(), crtc->mode_property().id(),
>                                     mode_.blob_id) < 0 ||
>            drmModeAtomicAddProperty(pset, connector->id(),
> -- 
> 2.7.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel