On Wed, Jul 04, 2018 at 11:24:10AM +0200, Daniel Vetter wrote:
On Wed, Jul 04, 2018 at 10:05:41AM +0100, Russell King - ARM Linux wrote:
On Wed, Jul 04, 2018 at 10:26:04AM +0200, Daniel Vetter wrote:
On Tue, Jul 03, 2018 at 05:18:57PM +0100, Russell King - ARM Linux wrote:
Can someone provide a deeper explanation about exactly what this property represents please?
Does this represent the range of YCbCr values _into_ a YCbCr-to-RGB conversion (in other words, the range of values in the framebuffer), or the expected output range from the conversion?
This matters, because a "limited", (iow, eg, BT601 compliant YCbCr) framebuffer where the Y signal is between 16..235 being displayed on a full-range RGB output would need different conversion from needing a limited-range RGB output.
If it is indeed the output, then why is this a property of the plane? Is that not a property of:
(a) whether the plane is being blended or overlaid onto a graphics plane which uses full-range RGB (b) the properties of the output(s) to which the plane is being displayed.
IOW, it seems that the output of the CSC is more to do with what's downstream of the plane than with the plane itself.
For example, take this situation:
plane 0 - graphics, full range RGB >-- CRTC --> HDMI sink only supporting plane 1 - video, limited range YUV limited range RGB
In order to display the graphics correctly in that scenario, the HDMI output needs to compress the RGB 0-255 range down to 16..236 to be compliant. If the video is limited range, and the CSC produces a limited range RGB output, then plane 1 gets its range further compressed at the HDMI output, which surely is undesirable.
It would surely be better, if it's not possible to map the range of plane 0 to limited range, to instead expand the YUV range and then recompress it at the HDMI output to match the capabilities of the attached source.
It also seems logical that describing the range of the RGB plane would also be sane - if the application is limiting graphics RGB to 16..235, then you'd want the CSC output to do the same and there'd be no need for any range expansion or compression.
I'd personally like drm_plane_create_color_properties() to allow creation of COLOR_ENCODING without COLOR_RANGE (iow, supported_ranges being zero) until COLOR_RANGE is better defined than it is at present.
Thoughts?
I'm bringing this up, because the hardware I have has a CSC that accepts BT601 and BT709 formats, controlled by a single bit. Another bit controls whether the CSC produces 0..255 output or 16..235 output. That is then blended/overlaid with the graphics plane (0..255) and sent to the output. Having a "limited range" YUV plane produce 16..235 range output makes it look low-contrast compared to the graphics, which is what would be expected - "16" is not black compared to the black of the graphics in the same way that "235" is not white compared to the graphics.
Drivers are supposed to automatically figure this out by looking at the edid. In i915 we also allow userspace to override this with the "Broadcast RGB" property on the connector. Unfortunately we haven't polished that property yet (not sure what other drivers are doing tbh), so it's only listed in the Documentation/gpu/kms-properties.csv graveyard :-/
In which case, I'd like to implement the COLOR_ENCODING property but not the COLOR_RANGE property until COLOR_RANGE is better defined. Unfortunately, at the moment the choices are to have either both properties or no properties - drm_plane_create_color_properties() doesn't support only creating the encoding property.
Implementing it without it being well defined is a recipe for having a broken UAPI. So, I propose:
Hm maybe I misunderstood, but I thought the COLOR_RANGE is on the input side.
If that's the case, I should force it to only indicate support for limited range, while programming the CSC to produce full range RGB on its output (see below).
And you need to adjust your CSC to make sure the output range is whatever makes things Just Work. Probably better to poke Ville on these details, and then clarify the docs.
It's not possible to arbitarily program the CSC - I have two bits:
- one bit selects whether BT601 or BT709 YUV encoding is used on the CSC input. - one bit selects whether limited or full range RGB is produced on the CSC output.
Given that I have to assume that the primary plane contains full range RGB values (DRM has no means to communicate from userspace anything else at present) I have to always select full range RGB at the CSC output to be blended with the primary plane.
I then need some way to know that the TDA998x is being fed by a full range RGB (for the time being, I'm going to assume that's the case) and convert to limited range in the TDA998x if the sink doesn't support full range. I'm proposing to implement your i915 "broadcast rgb" control in TDA998x to allow that to be overridden - maybe that should become a standardised control for HDMI outputs?
As an optimisation, if the primary plane is disabled, then it would be possible (in theory) to have the plane CSC produce limited range and feed that straight to the TDA998x with no conversion there - but it's probably not feasible to switch both modes without some artifacts being generated, especially as we're talking over I2C to the TDA998x. Also, as mentioned above, the TDA998x has no way to know the format from the upstream CRTC, since it can't assume what DRM driver it is being used with, and the DRM driver can't really assume that the down-stream device is a TDA998x. (I have the same problem with the bus format used between the TDA998x and CRTC, and I notice comments in DRM talking about inter-bridge bus formats also being a problem - it's all the same issue really.)