Messages in this thread | | | Date | Tue, 31 Jan 2017 15:55:41 +0000 | From | Brian Starkey <> | Subject | Re: DRM Atomic property for color-space conversion |
| |
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.
-Brian
> >-- >Ville Syrjälä >Intel OTC >-- >To unsubscribe from this list: send the line "unsubscribe linux-media" in >the body of a message to majordomo@vger.kernel.org >More majordomo info at http://vger.kernel.org/majordomo-info.html
| |