Re: [PATCH v6 1/3] drm: Add CRTC background color property (v5)

[Matt Roper <matthew.d.roper@intel.com>]

On Tue, Feb 26, 2019 at 08:26:36AM +0100, Maarten Lankhorst wrote:
> Hey,
> 
> Op 21-02-2019 om 01:28 schreef Matt Roper:
> > Some display controllers can be programmed to present non-black colors
> > for pixels not covered by any plane (or pixels covered by the
> > transparent regions of higher planes).  Compositors that want a UI with
> > a solid color background can potentially save memory bandwidth by
> > setting the CRTC background property and using smaller planes to display
> > the rest of the content.
> >
> > To avoid confusion between different ways of encoding RGB data, we
> > define a standard 64-bit format that should be used for this property's
> > value.  Helper functions and macros are provided to generate and dissect
> > values in this standard format with varying component precision values.
> >
> > v2:
> >  - Swap internal representation's blue and red bits to make it easier
> >    to read if printed out.  (Ville)
> >  - Document bgcolor property in drm_blend.c.  (Sean Paul)
> >  - s/background_color/bgcolor/ for consistency between property name and
> >    value storage field.  (Sean Paul)
> >  - Add a convenience function to attach property to a given crtc.
> >
> > v3:
> >  - Restructure ARGB component extraction macros to be easier to
> >    understand and enclose the parameters in () to avoid calculations
> >    if expressions are passed.  (Sean Paul)
> >  - s/rgba/argb/ in helper function/macro names.  Even though the idea is
> >    to not worry about the internal representation of the u64, it can
> >    still be confusing to look at code that uses 'rgba' terminology, but
> >    stores values with argb ordering.  (Ville)
> >
> > v4:
> >  - Drop the bgcolor_changed flag.  (Ville, Brian Starkey)
> >  - Clarify in kerneldoc that background color is expected to undergo the
> >    same pipe-level degamma/csc/gamma transformations that planes do.
> >    (Brian Starkey)
> >  - Update kerneldoc to indicate non-opaque colors are allowed, but are
> >    generally only useful in special cases such as when writeback
> >    connectors are used (Brian Starkey / Eric Anholt)
> >
> > v5:
> >  - Set crtc->state->bgcolor to solid black inside
> >    drm_crtc_add_bgcolor_property() in case drivers don't do this
> >    themselves.  (Ville)
> >  - Add kerneldoc to drm_crtc_add_bgcolor_property() function.
> >
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: wei.c.li@intel.com
> > Cc: harish.krupo.kps@intel.com
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Sean Paul <sean@poorly.run>
> > Cc: Brian Starkey <Brian.Starkey@arm.com>
> > Cc: Eric Anholt <eric@anholt.net>
> > Cc: Stéphane Marchesin <marcheu@chromium.org>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > Reviewed-by(v2): Sean Paul <sean@poorly.run>
> > Reviewed-by: Brian Starkey <brian.starkey@arm.com>
> 
> I like how bgcolor is a u64 now, but there is an issue with setting
> crtc->state->bgcolor in attaching the property.
> 
> We should do it in atomic core init instead, like we already do for
> connector/plane properties..
> 
> See for example https://patchwork.freedesktop.org/series/52363/
> 
> This was specificallly for the background color proposal, but we
> probalby have to split out the non-trivial conversions of
> __drm_atomic_helper_crtc_reset.

Makes sense.  What's the status of that patch?  It looks like it has a
lot of a-b's/r-b's but hasn't landed yet.  Is there anything blocking
it?  If you're still driving it forward, I can wait for that to land and
then build on top of it.


Matt

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx