| Message ID | 20180704090541.GR17271@n2100.armlinux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jul 04, 2018 at 01:06:42PM +0200, Daniel Vetter wrote: > On Wed, Jul 4, 2018 at 11:58 AM, Russell King - ARM Linux > <linux@armlinux.org.uk> wrote: > > 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). > > Agreed (as far as my understanding of all this goes at least). > > >> 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. > > You could attach the color range stuff also to the primary plane, but > I don't think any current userspace is prepared to use it. > > > 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? > > Yup that sounds like a very good idea, plus documenting it in > > https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#hdmi-specific-connector-properties Note that at some point we'll have to come up with some new props for specifying the full output colorimetry information and I have a feeling the broadcast rgb prop may get slightly in the way. But we'll have to cross that bridge in i915 anyway so I guess it wouldn't complicate the situation any more if other drivers have this prop as well.
diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c index b97e2de2c029..fc397fcf80ab 100644 --- a/drivers/gpu/drm/drm_color_mgmt.c +++ b/drivers/gpu/drm/drm_color_mgmt.c @@ -426,9 +426,9 @@ int drm_plane_create_color_properties(struct drm_plane *plane, (supported_encodings & BIT(default_encoding)) == 0)) return -EINVAL; - if (WARN_ON(supported_ranges == 0 || - (supported_ranges & -BIT(DRM_COLOR_RANGE_MAX)) != 0 || - (supported_ranges & BIT(default_range)) == 0)) + if (WARN_ON(supported_ranges != 0 && + ((supported_ranges & -BIT(DRM_COLOR_RANGE_MAX)) != 0 || + (supported_ranges & BIT(default_range)) == 0))) return -EINVAL; len = 0; @@ -450,6 +450,9 @@ int drm_plane_create_color_properties(struct drm_plane *plane, if (plane->state) plane->state->color_encoding = default_encoding; + if (supported_ranges == 0) + return 0; + len = 0; for (i = 0; i < DRM_COLOR_RANGE_MAX; i++) { if ((supported_ranges & BIT(i)) == 0)