Message-ID: <20170316140725.GF31595@intel.com>
Date:   Thu, 16 Mar 2017 16:07:25 +0200
From:   Ville Syrjälä <ville.syrjala@...ux.intel.com>
To:     Brian Starkey <brian.starkey@....com>
Cc:     dri-devel@...ts.freedesktop.org, linux-media@...r.kernel.org,
        linux-kernel@...r.kernel.org, mihail.atanassov@....com,
        liviu.dudau@....com, Shashank Sharma <shashank.sharma@...el.com>
Subject: Re: DRM Atomic property for color-space conversion

On Tue, Jan 31, 2017 at 03:55:41PM +0000, Brian Starkey wrote:
> On Tue, Jan 31, 2017 at 05:15:46PM +0200, Ville Syrjälä wrote:
> >On Tue, Jan 31, 2017 at 12:33:29PM +0000, Brian Starkey wrote:
> >> Hi,
> >>
> >> On Mon, Jan 30, 2017 at 03:35:13PM +0200, Ville Syrjälä wrote:
> >> >On Fri, Jan 27, 2017 at 05:23:24PM +0000, Brian Starkey wrote:
> >> >> Hi,
> >> >>
> >> >> We're looking to enable the per-plane color management hardware in
> >> >> Mali-DP with atomic properties, which has sparked some conversation
> >> >> around how to handle YCbCr formats.
> >> >>
> >> >> As it stands today, it's assumed that a driver will implicitly "do the
> >> >> right thing" to display a YCbCr buffer.
> >> >>
> >> >> YCbCr data often uses different gamma curves and signal ranges (e.g.
> >> >> BT.609, BT.701, BT.2020, studio range, full-range), so its desirable
> >> >> to be able to explicitly control the YCbCr to RGB conversion process
> >> >> from userspace.
> >> >>
> >> >> We're proposing adding a "CSC" (color-space conversion) property to
> >> >> control this - primarily per-plane for framebuffer->pipeline CSC, but
> >> >> perhaps one per CRTC too for devices which have an RGB pipeline and
> >> >> want to output in YUV to the display:
> >> >>
> >> >> Name: "CSC"
> >> >> Type: ENUM | ATOMIC;
> >> >> Enum values (representative):
> >> >> "default":
> >> >> 	Same behaviour as now. "Some kind" of YCbCr->RGB conversion
> >> >> 	for YCbCr buffers, bypass for RGB buffers
> >> >> "disable":
> >> >> 	Explicitly disable all colorspace conversion (i.e. use an
> >> >> 	identity matrix).
> >> >> "YCbCr to RGB: BT.709":
> >> >> 	Only valid for YCbCr formats. CSC in accordance with BT.709
> >> >> 	using [16..235] for (8-bit) luma values, and [16..240] for
> >> >> 	8-bit chroma values. For 10-bit formats, the range limits are
> >> >> 	multiplied by 4.
> >> >> "YCbCr to RGB: BT.709 full-swing":
> >> >> 	Only valid for YCbCr formats. CSC in accordance with BT.709,
> >> >> 	but using the full range of each channel.
> >> >> "YCbCr to RGB: Use CTM":*
> >> >> 	Only valid for YCbCr formats. Use the matrix applied via the
> >> >> 	plane's CTM property
> >> >> "RGB to RGB: Use CTM":*
> >> >> 	Only valid for RGB formats. Use the matrix applied via the
> >> >> 	plane's CTM property
> >> >> "Use CTM":*
> >> >> 	Valid for any format. Use the matrix applied via the plane's
> >> >> 	CTM property
> >> >> ... any other values for BT.601, BT.2020, RGB to YCbCr etc. etc. as
> >> >> they are required.
> >> >
> >> >Having some RGB2RGB and YCBCR2RGB things in the same property seems
> >> >weird. I would just go with something very simple like:
> >> >
> >> >YCBCR_TO_RGB_CSC:
> >> >* BT.601
> >> >* BT.709
> >> >* custom matrix
> >> >
> >>
> >> I think we've agreed in #dri-devel that this CSC property
> >> can't/shouldn't be mapped on-to the existing (hardware implementing
> >> the) CTM property - even in the case of per-plane color management -
> >> because CSC needs to be done before DEGAMMA.
> >>
> >> So, I'm in favour of going with what you suggested in the first place:
> >>
> >> A new YCBCR_TO_RGB_CSC property, enum type, with a list of fixed
> >> conversions. I'd drop the custom matrix for now, as we'd need to add
> >> another property to attach the custom matrix blob too.
> >>
> >> I still think we need a way to specify whether the source data range
> >> is broadcast/full-range, so perhaps the enum list should be expanded
> >> to all combinations of BT.601/BT.709 + broadcast/full-range.
> >
> >Sounds reasonable. Not that much full range YCbCr stuff out there
> >perhaps. Well, apart from jpegs I suppose. But no harm in being able
> >to deal with it.
> >
> >>
> >> (I'm not sure what the canonical naming for broadcast/full-range is,
> >> we call them narrow and wide)
> >
> >We tend to call them full vs. limited range. That's how our
> >"Broadcast RGB" property is defined as well.
> >
> 
> OK, using the same ones sounds sensible.
> 
> >>
> >> >And trying to use the same thing for the crtc stuff is probably not
> >> >going to end well. Like Daniel said we already have the
> >> >'Broadcast RGB' property muddying the waters there, and that stuff
> >> >also ties in with what colorspace we signal to the sink via
> >> >infoframes/whatever the DP thing was called. So my gut feeling is
> >> >that trying to use the same property everywhere will just end up
> >> >messy.
> >>
> >> Yeah, agreed. If/when someone wants to add CSC on the output of a CRTC
> >> (after GAMMA), we can add a new property.
> >>
> >> That makes me wonder about calling this one SOURCE_YCBCR_TO_RGB_CSC to
> >> be explicit that it describes the source data. Then we can later add
> >> SINK_RGB_TO_YCBCR_CSC, and it will be reasonably obvious that its
> >> value describes the output data rather than the input data.
> >
> >Source and sink have a slight connotation in my mind wrt. the box that
> >produces the display signal and the box that eats the signal. So trying
> >to use the same terms to describe the internals of the pipeline inside
> >the "source box" migth lead to some confusion. But we do probably need
> >some decent names for these to make the layout of the pipeline clear.
> >Input/output are the other names that popped to my mind but those aren't
> >necessarily any better. But in the end I think I could live with whatever
> >names we happen to pick, as long as we document the pipeline clearly.
> >
> >Long ago I did wonder if we should just start indexing these things
> >somehow, and then just looking at the index should tell you the order
> >of the operations. But we already have the ctm/gamma w/o any indexes so
> >that idea probably isn't so great anymore.
> >
> >>
> >> I want to avoid confusion caused by ending up with two
> >> {CS}_TO_{CS}_CSC properties, where one is describing the data to the
> >> left of it, and the other describing the data to the right of it, with
> >> no real way of telling which way around it is.
> >
> >Not really sure what you mean. It should always be
> ><left>_to_<right>_csc.
> 
> Agreed, left-to-right. But for instance on a CSC property representing
> a CRTC output CSC (just before hitting the connector), which happens
> to be converting RGB to YCbCr:
> 
> CRTC -> GAMMA -> RGB_TO_YCBCR_CSC
> 
> ...the enum value "BT.601 Limited" means that the data on the *right*
> of RGB_TO_YCBCR_CSC is "BT.601 Limited"
> 
> On the other hand for a CSC on the input of a plane, which happens to
> be converting YCbCr to RGB:
> 
> RAM -> YCBCR_TO_RGB_CSC -> DEGAMMA
> 
> ...the enum value "BT.601 Limited" means that the data on the *left*
> of YCBCR_TO_RGB_CSC is "BT.601 Limited".
> 
> Indicating in the property name whether its value is describing the
> data on the left or the right is needed (and I don't think inferring
> that "it's always the YCBCR one" is the correct approach).
> 
> In my example above, "SOURCE_xxx" would mean the enum value is
> describing the "source" data (i.e. the data on the left) and
> "SINK_xxx" would mean the enum value is describing the "sink" data
> (i.e. the data on the right). This doesn't necessarily need to infer a
> particular point in the pipeline.

Right, so I guess you want the values to be named "<a> to <b>" as well?
Yes, I think we'll be wanting that as well.

So what we might need is something like:
enum YCBCR_TO_RGB_CSC
 * YCbCr BT.601 limited to RGB BT.709 full
 * YCbCr BT.709 limited to RGB BT.709 full <this would be the likely default value IMO>
 * YCbCr BT.601 limited to RGB BT.2020 full
 * YCbCr BT.709 limited to RGB BT.2020 full
 * YCbCr BT.2020 limited to RGB BT.2020 full

And thanks to BT.2020 we'll need a RGB->RGB CSC property as well. Eg:
enum RGB_TO_RGB_CSC
 * bypass (or separate 709->709, 2020->2020?) <this would be the default>
 * RGB BT.709 full to RGB BT.2020 full

Alternatives would involve two properties to define the input and output
from the CSC separately, but then you lose the capability to see which
combinations are actually supoorted.

We may want to add the "curstom matrix" enum value + the blob property
for the actual matrix for hw capable of doing that.

Adding Shashank to cc since he's the one who has been
looking at this colorspacey stuff on our side.

-- 
Ville Syrjälä
Intel OTC

Powered by blists - more mailing lists