| Message ID | 1519271648-33102-1-git-send-email-stschake@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hey Stefan, On 02/22/2018 04:54 AM, 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. I wasn't aware of this assumption, but given that it is the case, the patch looks like a good fix for the problem. Unfortunately I don't have a hardware platform up and running to test the patch with at the moment. > > 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). > > 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(), >
Hey Robert, On Thu, Feb 22, 2018 at 11:04 AM, Robert Foss <robert.foss@collabora.com> wrote: > Hey Stefan, > > On 02/22/2018 04:54 AM, 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. > > > I wasn't aware of this assumption, but given that it is the case, > the patch looks like a good fix for the problem. > > Unfortunately I don't have a hardware platform up and running to test the > patch with at the moment. > HWC1 has this ominous comment: * IMPORTANT NOTE: There is an implicit layer containing opaque black * pixels behind all the layers in the list. It is the responsibility of * the hwcomposer module to make sure black pixels are output (or blended * from). It's not repeated in the HWC2 docs, but I think the assumption carried over given that Android prefers all things RGBA. Admittedly it's rare that you don't have a full-size opaque layer in the composition like a background picture, so the cases where this causes corrupted rendering on VC4 are limited to some time during boot after the animation finishes but the launcher isn't up yet. Thanks, Stefan
Stefan Schake <stschake@gmail.com> writes: > 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). Couldn't we just ignore the alpha channel on the primary plane, on the assumption that it's supposed to be all zeroes below it? Or are we not premultiplied, so we do still need to multiply the primary plane's colors by the alpha value? I couldn't find any obvious DRM documentation about whether alpha is premultiplied.
Hey Eric, On Thu, Feb 22, 2018 at 9:47 PM, Eric Anholt <eric@anholt.net> wrote: > Stefan Schake <stschake@gmail.com> writes: > >> 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). > > Couldn't we just ignore the alpha channel on the primary plane, on the > assumption that it's supposed to be all zeroes below it? Or are we not > premultiplied, so we do still need to multiply the primary plane's > colors by the alpha value? I couldn't find any obvious DRM > documentation about whether alpha is premultiplied. That would work, too (for a black background anyway). Though the easiest place to spoof this is presumably in the kernel. From what I have seen, everything in Android is already premultiplied, but technically it can specify either. Not sure if this is correctly handled in drm_hwcomposer yet. Thanks, Stefan
Hey, On 02/23/2018 03:34 PM, Stefan Schake wrote: > Hey Eric, > > On Thu, Feb 22, 2018 at 9:47 PM, Eric Anholt <eric@anholt.net> wrote: >> Stefan Schake <stschake@gmail.com> writes: >> >>> 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). >> >> Couldn't we just ignore the alpha channel on the primary plane, on the >> assumption that it's supposed to be all zeroes below it? Or are we not >> premultiplied, so we do still need to multiply the primary plane's >> colors by the alpha value? I couldn't find any obvious DRM >> documentation about whether alpha is premultiplied. > > That would work, too (for a black background anyway). Though the easiest place > to spoof this is presumably in the kernel. From what I have seen, everything in > Android is already premultiplied, but technically it can specify > either. Not sure if this > is correctly handled in drm_hwcomposer yet. > To me Erics solution sounds pragmatic, and would be easy to reason about going forward. Making sure that all of the kernel drivers behave the same way might be the hardest part. Stefan: Are you looking at an entirely kernel side fix now, or are you pushing this series forward? Rob.
Hey Rob, On Wed, Feb 28, 2018 at 11:53 AM, Robert Foss <robert.foss@collabora.com> wrote: > Hey, > > Stefan: Are you looking at an entirely kernel side fix now, or are you > pushing this series forward? I've sent out a kernel side fix for this: https://patchwork.freedesktop.org/patch/207667/ So I guess for now this can be dropped, pending review. Thanks, Stefan
Ack, thanks for the heads up! Rob. On 03/02/2018 01:41 AM, Stefan Schake wrote: > Hey Rob, > > On Wed, Feb 28, 2018 at 11:53 AM, Robert Foss <robert.foss@collabora.com> wrote: >> Hey, >> >> Stefan: Are you looking at an entirely kernel side fix now, or are you >> pushing this series forward? > > I've sent out a kernel side fix for this: > > https://patchwork.freedesktop.org/patch/207667/ > > So I guess for now this can be dropped, pending review. > > Thanks, > Stefan > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel >
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
Op 22-02-18 om 04:54 schreef Stefan Schake: > 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). Ping? Would be nice if we were moving forward. :)
Hey Maarten, On Fri, Jun 29, 2018 at 12:05 PM, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote: > Op 22-02-18 om 04:54 schreef Stefan Schake: >> 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). > Ping? Would be nice if we were moving forward. :) I was unclear if DRM specified a black background when writing this. Someone pointed me towards the excerpt in the docs that explicitly mandates black background fill and so I ended up writing a VC4 patch that automatically sets it when required instead of the optional property used here. Adding a background_color property would still be desirable, but I'm unclear on what the userspace would be at the moment. drm_hwc doesn't need any background color other than black and since that is the DRM default, it wouldn't need to use a property. Thanks, Stefan
Op 29-06-18 om 12:27 schreef Stefan Schake: > Hey Maarten, > > On Fri, Jun 29, 2018 at 12:05 PM, Maarten Lankhorst > <maarten.lankhorst@linux.intel.com> wrote: >> Op 22-02-18 om 04:54 schreef Stefan Schake: >>> 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). >> Ping? Would be nice if we were moving forward. :) > I was unclear if DRM specified a black background when writing this. > Someone pointed me towards the excerpt in the docs that explicitly > mandates black background fill and so I ended up writing a VC4 patch > that automatically sets it when required instead of the optional property > used here. > > Adding a background_color property would still be desirable, but I'm > unclear on what the userspace would be at the moment. drm_hwc doesn't > need any background color other than black and since that is the DRM > default, it wouldn't need to use a property. > > Thanks, > Stefan It would still be useful for e-paper displays where the background color would be better suited as white, or when we want to blend a different color than black. But I fear for now there's no real userspace. :( Which is unfortunate, because the series are easy to implement, but it's just a case where there's no need from userspace yet for the hw capability. ~Maarten
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(),
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). drmcrtc.cpp | 11 +++++++++++ drmcrtc.h | 2 ++ drmdisplaycompositor.cpp | 15 +++++++++++++++ 3 files changed, 28 insertions(+)