| Message ID | 20210618091116.14428-16-wse@tuxedocomputers.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | New uAPI drm properties for color management | expand |
On Fri, 18 Jun 2021 11:11:14 +0200 Werner Sembach <wse@tuxedocomputers.com> wrote: > Add "Broadcast RGB" to general drm context so that more drivers besides > i915 and gma500 can implement it without duplicating code. > > Userspace can use this property to tell the graphic driver to use full or > limited color range for a given connector, overwriting the default > behaviour/automatic detection. > > Possible options are: > - Automatic (default/current behaviour) > - Full > - Limited 16:235 > > In theory the driver should be able to automatically detect the monitors > capabilities, but because of flawed standard implementations in Monitors, > this might fail. In this case a manual overwrite is required to not have > washed out colors or lose details in very dark or bright scenes. > > Signed-off-by: Werner Sembach <wse@tuxedocomputers.com> > --- > drivers/gpu/drm/drm_atomic_helper.c | 4 +++ > drivers/gpu/drm/drm_atomic_uapi.c | 4 +++ > drivers/gpu/drm/drm_connector.c | 43 +++++++++++++++++++++++++++++ > include/drm/drm_connector.h | 16 +++++++++++ > 4 files changed, 67 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index 90d62f305257..0c89d32efbd0 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -691,6 +691,10 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, > if (old_connector_state->preferred_color_format != > new_connector_state->preferred_color_format) > new_crtc_state->connectors_changed = true; > + > + if (old_connector_state->preferred_color_range != > + new_connector_state->preferred_color_range) > + new_crtc_state->connectors_changed = true; > } > > if (funcs->atomic_check) > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c > index c536f5e22016..c589bb1a8163 100644 > --- a/drivers/gpu/drm/drm_atomic_uapi.c > +++ b/drivers/gpu/drm/drm_atomic_uapi.c > @@ -798,6 +798,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector, > state->max_requested_bpc = val; > } else if (property == connector->preferred_color_format_property) { > state->preferred_color_format = val; > + } else if (property == connector->preferred_color_range_property) { > + state->preferred_color_range = val; > } else if (connector->funcs->atomic_set_property) { > return connector->funcs->atomic_set_property(connector, > state, property, val); > @@ -877,6 +879,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector, > *val = state->max_requested_bpc; > } else if (property == connector->preferred_color_format_property) { > *val = state->preferred_color_format; > + } else if (property == connector->preferred_color_range_property) { > + *val = state->preferred_color_range; > } else if (connector->funcs->atomic_get_property) { > return connector->funcs->atomic_get_property(connector, > state, property, val); > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > index aea03dd02e33..9bc596638613 100644 > --- a/drivers/gpu/drm/drm_connector.c > +++ b/drivers/gpu/drm/drm_connector.c > @@ -905,6 +905,12 @@ static const struct drm_prop_enum_list drm_active_color_format_enum_list[] = { > { DRM_COLOR_FORMAT_YCRCB420, "ycbcr420" }, > }; > > +static const struct drm_prop_enum_list drm_preferred_color_range_enum_list[] = { > + { DRM_MODE_COLOR_RANGE_UNSET, "Automatic" }, > + { DRM_MODE_COLOR_RANGE_FULL, "Full" }, > + { DRM_MODE_COLOR_RANGE_LIMITED_16_235, "Limited 16:235" }, Hi, the same question here about these numbers as I asked on the "active color range" property. > +}; > + > static const struct drm_prop_enum_list drm_active_color_range_enum_list[] = { > { DRM_MODE_COLOR_RANGE_UNSET, "Unknown" }, > { DRM_MODE_COLOR_RANGE_FULL, "Full" }, > @@ -1243,6 +1249,13 @@ static const struct drm_prop_enum_list dp_colorspaces[] = { > * drm_connector_attach_active_color_format_property() to install this > * property. > * > + * Broadcast RGB: > + * This property is used by userspace to change the used color range. When > + * used the driver will use the selected range if valid for the current > + * color format. Drivers to use the function > + * drm_connector_attach_preferred_color_format_property() to create and > + * attach the property to the connector during initialization. An important detail to document here is: does userspace need to take care that pixel data at the connector will already match the set range, or will the driver program the hardware to produce the set range? If the former, then I'm afraid the preference/active property pair design does not work. Userspace needs to make sure the content is in the right range, so the driver cannot second-guess that afterwards. If the latter, then what does the driver assume about color range just before the automatic conversion to the final color range, and does the range conversion happen as the final step in the color pipeline? If I remember the discussion about Intel right, then the driver does the latter and assume that userspace programs KMS to always produce full range pixels. There is no provision for userspace to produce limited range pixels, IIRC. Thanks, pq > + * > * active color range: > * This read-only property tells userspace the color range actually used by > * the hardware display engine on "the cable" on a connector. The chosen > @@ -2324,6 +2337,36 @@ void drm_connector_set_active_color_format_property(struct drm_connector *connec > } > EXPORT_SYMBOL(drm_connector_set_active_color_format_property); > > +/** > + * drm_connector_attach_preferred_color_range_property - attach "Broadcast RGB" property > + * @connector: connector to attach preferred color range property on. > + * > + * This is used to add support for selecting a color range on a connector. > + * > + * Returns: > + * Zero on success, negative errno on failure. > + */ > +int drm_connector_attach_preferred_color_range_property(struct drm_connector *connector) > +{ > + struct drm_device *dev = connector->dev; > + struct drm_property *prop; > + > + if (!connector->preferred_color_range_property) { > + prop = drm_property_create_enum(dev, 0, "Broadcast RGB", > + drm_preferred_color_range_enum_list, > + ARRAY_SIZE(drm_preferred_color_range_enum_list)); > + if (!prop) > + return -ENOMEM; > + > + connector->preferred_color_range_property = prop; > + drm_object_attach_property(&connector->base, prop, DRM_MODE_COLOR_RANGE_UNSET); > + connector->state->preferred_color_range = DRM_MODE_COLOR_RANGE_UNSET; > + } > + > + return 0; > +} > +EXPORT_SYMBOL(drm_connector_attach_preferred_color_range_property); > + > /** > * drm_connector_attach_active_color_range_property - attach "active color range" property > * @connector: connector to attach active color range property on. > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > index 7b85407ba45c..b319760d4a8c 100644 > --- a/include/drm/drm_connector.h > +++ b/include/drm/drm_connector.h > @@ -809,6 +809,15 @@ struct drm_connector_state { > */ > u32 preferred_color_format; > > + /** > + * preferred_color_range: Property set by userspace via "Broadcast RGB" > + * property to tell the GPU driver which color range to use. It > + * overwrites existing automatic detection mechanisms, if set and valid > + * for the current color format. Userspace can check for (un-)successful > + * application via the "active color range" property. > + */ > + enum drm_mode_color_range preferred_color_range; > + > /** > * @hdr_output_metadata: > * DRM blob property for HDR output metadata > @@ -1426,6 +1435,12 @@ struct drm_connector { > */ > struct drm_property *active_color_format_property; > > + /** > + * @preferred_color_range_property: Default connector property for the > + * preferred color range to be driven out of the connector. > + */ > + struct drm_property *preferred_color_range_property; > + > /** > * @active_color_range_property: Default connector property for the > * active color range to be driven out of the connector. > @@ -1760,6 +1775,7 @@ int drm_connector_attach_preferred_color_format_property(struct drm_connector *c > int drm_connector_attach_active_color_format_property(struct drm_connector *connector); > void drm_connector_set_active_color_format_property(struct drm_connector *connector, > u32 active_color_format); > +int drm_connector_attach_preferred_color_range_property(struct drm_connector *connector); > int drm_connector_attach_active_color_range_property(struct drm_connector *connector); > void drm_connector_set_active_color_range_property(struct drm_connector *connector, > enum drm_mode_color_range active_color_range);
Am 22.06.21 um 09:25 schrieb Pekka Paalanen: > On Fri, 18 Jun 2021 11:11:14 +0200 > Werner Sembach <wse@tuxedocomputers.com> wrote: > >> Add "Broadcast RGB" to general drm context so that more drivers besides >> i915 and gma500 can implement it without duplicating code. >> >> Userspace can use this property to tell the graphic driver to use full or >> limited color range for a given connector, overwriting the default >> behaviour/automatic detection. >> >> Possible options are: >> - Automatic (default/current behaviour) >> - Full >> - Limited 16:235 >> >> In theory the driver should be able to automatically detect the monitors >> capabilities, but because of flawed standard implementations in Monitors, >> this might fail. In this case a manual overwrite is required to not have >> washed out colors or lose details in very dark or bright scenes. >> >> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com> >> --- >> drivers/gpu/drm/drm_atomic_helper.c | 4 +++ >> drivers/gpu/drm/drm_atomic_uapi.c | 4 +++ >> drivers/gpu/drm/drm_connector.c | 43 +++++++++++++++++++++++++++++ >> include/drm/drm_connector.h | 16 +++++++++++ >> 4 files changed, 67 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c >> index 90d62f305257..0c89d32efbd0 100644 >> --- a/drivers/gpu/drm/drm_atomic_helper.c >> +++ b/drivers/gpu/drm/drm_atomic_helper.c >> @@ -691,6 +691,10 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, >> if (old_connector_state->preferred_color_format != >> new_connector_state->preferred_color_format) >> new_crtc_state->connectors_changed = true; >> + >> + if (old_connector_state->preferred_color_range != >> + new_connector_state->preferred_color_range) >> + new_crtc_state->connectors_changed = true; >> } >> >> if (funcs->atomic_check) >> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c >> index c536f5e22016..c589bb1a8163 100644 >> --- a/drivers/gpu/drm/drm_atomic_uapi.c >> +++ b/drivers/gpu/drm/drm_atomic_uapi.c >> @@ -798,6 +798,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector, >> state->max_requested_bpc = val; >> } else if (property == connector->preferred_color_format_property) { >> state->preferred_color_format = val; >> + } else if (property == connector->preferred_color_range_property) { >> + state->preferred_color_range = val; >> } else if (connector->funcs->atomic_set_property) { >> return connector->funcs->atomic_set_property(connector, >> state, property, val); >> @@ -877,6 +879,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector, >> *val = state->max_requested_bpc; >> } else if (property == connector->preferred_color_format_property) { >> *val = state->preferred_color_format; >> + } else if (property == connector->preferred_color_range_property) { >> + *val = state->preferred_color_range; >> } else if (connector->funcs->atomic_get_property) { >> return connector->funcs->atomic_get_property(connector, >> state, property, val); >> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c >> index aea03dd02e33..9bc596638613 100644 >> --- a/drivers/gpu/drm/drm_connector.c >> +++ b/drivers/gpu/drm/drm_connector.c >> @@ -905,6 +905,12 @@ static const struct drm_prop_enum_list drm_active_color_format_enum_list[] = { >> { DRM_COLOR_FORMAT_YCRCB420, "ycbcr420" }, >> }; >> >> +static const struct drm_prop_enum_list drm_preferred_color_range_enum_list[] = { >> + { DRM_MODE_COLOR_RANGE_UNSET, "Automatic" }, >> + { DRM_MODE_COLOR_RANGE_FULL, "Full" }, >> + { DRM_MODE_COLOR_RANGE_LIMITED_16_235, "Limited 16:235" }, > Hi, > > the same question here about these numbers as I asked on the "active > color range" property. > >> +}; >> + >> static const struct drm_prop_enum_list drm_active_color_range_enum_list[] = { >> { DRM_MODE_COLOR_RANGE_UNSET, "Unknown" }, >> { DRM_MODE_COLOR_RANGE_FULL, "Full" }, >> @@ -1243,6 +1249,13 @@ static const struct drm_prop_enum_list dp_colorspaces[] = { >> * drm_connector_attach_active_color_format_property() to install this >> * property. >> * >> + * Broadcast RGB: >> + * This property is used by userspace to change the used color range. When >> + * used the driver will use the selected range if valid for the current >> + * color format. Drivers to use the function >> + * drm_connector_attach_preferred_color_format_property() to create and >> + * attach the property to the connector during initialization. > An important detail to document here is: does userspace need to > take care that pixel data at the connector will already match the set > range, or will the driver program the hardware to produce the set range? Since until now, the userspace didn't even know for sure if RGB or YCbCr and therefore which full/limited format was used I guess it's all kernel space conversion. > > If the former, then I'm afraid the preference/active property pair > design does not work. Userspace needs to make sure the content is in > the right range, so the driver cannot second-guess that afterwards. > > If the latter, then what does the driver assume about color range just > before the automatic conversion to the final color range, and does the > range conversion happen as the final step in the color pipeline? > > If I remember the discussion about Intel right, then the driver does > the latter and assume that userspace programs KMS to always produce > full range pixels. There is no provision for userspace to produce > limited range pixels, IIRC. I think I remember this too from an answer to one of the revisions of this patchset. > > > Thanks, > pq > >> + * >> * active color range: >> * This read-only property tells userspace the color range actually used by >> * the hardware display engine on "the cable" on a connector. The chosen >> @@ -2324,6 +2337,36 @@ void drm_connector_set_active_color_format_property(struct drm_connector *connec >> } >> EXPORT_SYMBOL(drm_connector_set_active_color_format_property); >> >> +/** >> + * drm_connector_attach_preferred_color_range_property - attach "Broadcast RGB" property >> + * @connector: connector to attach preferred color range property on. >> + * >> + * This is used to add support for selecting a color range on a connector. >> + * >> + * Returns: >> + * Zero on success, negative errno on failure. >> + */ >> +int drm_connector_attach_preferred_color_range_property(struct drm_connector *connector) >> +{ >> + struct drm_device *dev = connector->dev; >> + struct drm_property *prop; >> + >> + if (!connector->preferred_color_range_property) { >> + prop = drm_property_create_enum(dev, 0, "Broadcast RGB", >> + drm_preferred_color_range_enum_list, >> + ARRAY_SIZE(drm_preferred_color_range_enum_list)); >> + if (!prop) >> + return -ENOMEM; >> + >> + connector->preferred_color_range_property = prop; >> + drm_object_attach_property(&connector->base, prop, DRM_MODE_COLOR_RANGE_UNSET); >> + connector->state->preferred_color_range = DRM_MODE_COLOR_RANGE_UNSET; >> + } >> + >> + return 0; >> +} >> +EXPORT_SYMBOL(drm_connector_attach_preferred_color_range_property); >> + >> /** >> * drm_connector_attach_active_color_range_property - attach "active color range" property >> * @connector: connector to attach active color range property on. >> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h >> index 7b85407ba45c..b319760d4a8c 100644 >> --- a/include/drm/drm_connector.h >> +++ b/include/drm/drm_connector.h >> @@ -809,6 +809,15 @@ struct drm_connector_state { >> */ >> u32 preferred_color_format; >> >> + /** >> + * preferred_color_range: Property set by userspace via "Broadcast RGB" >> + * property to tell the GPU driver which color range to use. It >> + * overwrites existing automatic detection mechanisms, if set and valid >> + * for the current color format. Userspace can check for (un-)successful >> + * application via the "active color range" property. >> + */ >> + enum drm_mode_color_range preferred_color_range; >> + >> /** >> * @hdr_output_metadata: >> * DRM blob property for HDR output metadata >> @@ -1426,6 +1435,12 @@ struct drm_connector { >> */ >> struct drm_property *active_color_format_property; >> >> + /** >> + * @preferred_color_range_property: Default connector property for the >> + * preferred color range to be driven out of the connector. >> + */ >> + struct drm_property *preferred_color_range_property; >> + >> /** >> * @active_color_range_property: Default connector property for the >> * active color range to be driven out of the connector. >> @@ -1760,6 +1775,7 @@ int drm_connector_attach_preferred_color_format_property(struct drm_connector *c >> int drm_connector_attach_active_color_format_property(struct drm_connector *connector); >> void drm_connector_set_active_color_format_property(struct drm_connector *connector, >> u32 active_color_format); >> +int drm_connector_attach_preferred_color_range_property(struct drm_connector *connector); >> int drm_connector_attach_active_color_range_property(struct drm_connector *connector); >> void drm_connector_set_active_color_range_property(struct drm_connector *connector, >> enum drm_mode_color_range active_color_range); > > _______________________________________________ > amd-gfx mailing list > amd-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
On Tue, 22 Jun 2021 11:57:53 +0200 Werner Sembach <wse@tuxedocomputers.com> wrote: > Am 22.06.21 um 09:25 schrieb Pekka Paalanen: > > On Fri, 18 Jun 2021 11:11:14 +0200 > > Werner Sembach <wse@tuxedocomputers.com> wrote: > > > >> Add "Broadcast RGB" to general drm context so that more drivers besides > >> i915 and gma500 can implement it without duplicating code. > >> > >> Userspace can use this property to tell the graphic driver to use full or > >> limited color range for a given connector, overwriting the default > >> behaviour/automatic detection. > >> > >> Possible options are: > >> - Automatic (default/current behaviour) > >> - Full > >> - Limited 16:235 > >> > >> In theory the driver should be able to automatically detect the monitors > >> capabilities, but because of flawed standard implementations in Monitors, > >> this might fail. In this case a manual overwrite is required to not have > >> washed out colors or lose details in very dark or bright scenes. > >> > >> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com> > >> --- > >> drivers/gpu/drm/drm_atomic_helper.c | 4 +++ > >> drivers/gpu/drm/drm_atomic_uapi.c | 4 +++ > >> drivers/gpu/drm/drm_connector.c | 43 +++++++++++++++++++++++++++++ > >> include/drm/drm_connector.h | 16 +++++++++++ > >> 4 files changed, 67 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > >> index 90d62f305257..0c89d32efbd0 100644 > >> --- a/drivers/gpu/drm/drm_atomic_helper.c > >> +++ b/drivers/gpu/drm/drm_atomic_helper.c > >> @@ -691,6 +691,10 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, > >> if (old_connector_state->preferred_color_format != > >> new_connector_state->preferred_color_format) > >> new_crtc_state->connectors_changed = true; > >> + > >> + if (old_connector_state->preferred_color_range != > >> + new_connector_state->preferred_color_range) > >> + new_crtc_state->connectors_changed = true; > >> } > >> > >> if (funcs->atomic_check) > >> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c > >> index c536f5e22016..c589bb1a8163 100644 > >> --- a/drivers/gpu/drm/drm_atomic_uapi.c > >> +++ b/drivers/gpu/drm/drm_atomic_uapi.c > >> @@ -798,6 +798,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector, > >> state->max_requested_bpc = val; > >> } else if (property == connector->preferred_color_format_property) { > >> state->preferred_color_format = val; > >> + } else if (property == connector->preferred_color_range_property) { > >> + state->preferred_color_range = val; > >> } else if (connector->funcs->atomic_set_property) { > >> return connector->funcs->atomic_set_property(connector, > >> state, property, val); > >> @@ -877,6 +879,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector, > >> *val = state->max_requested_bpc; > >> } else if (property == connector->preferred_color_format_property) { > >> *val = state->preferred_color_format; > >> + } else if (property == connector->preferred_color_range_property) { > >> + *val = state->preferred_color_range; > >> } else if (connector->funcs->atomic_get_property) { > >> return connector->funcs->atomic_get_property(connector, > >> state, property, val); > >> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > >> index aea03dd02e33..9bc596638613 100644 > >> --- a/drivers/gpu/drm/drm_connector.c > >> +++ b/drivers/gpu/drm/drm_connector.c > >> @@ -905,6 +905,12 @@ static const struct drm_prop_enum_list drm_active_color_format_enum_list[] = { > >> { DRM_COLOR_FORMAT_YCRCB420, "ycbcr420" }, > >> }; > >> > >> +static const struct drm_prop_enum_list drm_preferred_color_range_enum_list[] = { > >> + { DRM_MODE_COLOR_RANGE_UNSET, "Automatic" }, > >> + { DRM_MODE_COLOR_RANGE_FULL, "Full" }, > >> + { DRM_MODE_COLOR_RANGE_LIMITED_16_235, "Limited 16:235" }, > > Hi, > > > > the same question here about these numbers as I asked on the "active > > color range" property. > > > >> +}; > >> + > >> static const struct drm_prop_enum_list drm_active_color_range_enum_list[] = { > >> { DRM_MODE_COLOR_RANGE_UNSET, "Unknown" }, > >> { DRM_MODE_COLOR_RANGE_FULL, "Full" }, > >> @@ -1243,6 +1249,13 @@ static const struct drm_prop_enum_list dp_colorspaces[] = { > >> * drm_connector_attach_active_color_format_property() to install this > >> * property. > >> * > >> + * Broadcast RGB: > >> + * This property is used by userspace to change the used color range. When > >> + * used the driver will use the selected range if valid for the current > >> + * color format. Drivers to use the function > >> + * drm_connector_attach_preferred_color_format_property() to create and > >> + * attach the property to the connector during initialization. > > An important detail to document here is: does userspace need to > > take care that pixel data at the connector will already match the set > > range, or will the driver program the hardware to produce the set range? > Since until now, the userspace didn't even know for sure if RGB or YCbCr and therefore which full/limited format was > used I guess it's all kernel space conversion. > > > > If the former, then I'm afraid the preference/active property pair > > design does not work. Userspace needs to make sure the content is in > > the right range, so the driver cannot second-guess that afterwards. > > > > If the latter, then what does the driver assume about color range just > > before the automatic conversion to the final color range, and does the > > range conversion happen as the final step in the color pipeline? > > > > If I remember the discussion about Intel right, then the driver does > > the latter and assume that userspace programs KMS to always produce > > full range pixels. There is no provision for userspace to produce > > limited range pixels, IIRC. > I think I remember this too from an answer to one of the revisions of this patchset. As long as you keep the old KMS property as is, just moving code so it is used by more drivers, this is fine and one can't do otherwise anyway. (The rest of this email is merely pondering the future, so not about this patch in particular.) But if we had a new, more general property for the range reported to monitors via infoframes, then it would be worth to re-visit the design. The HDR properties only set the infoframe and expect userspace to make sure that the pixels actually correspond to what the infoframes tell the monitor. One can't do HDR tone mapping automatically in the kernel, so in that sense the HDR property behaviour is obvious. But which behaviour would fit range property or others better, I'm not sure. Generally there seems to be two approaches to designing KMS properties: - Let userspace describe what data it has and what data should be sent to a monitor, and let the kernel driver magically come up with a conversion. - Only userspace understands how the pixel data is encoded, and programs the transformations (DEGAMMA/CTM/GAMMA etc.) such, that the result is what a monitor expects based on e.g. infoframes. Doing the former requires policy in the kernel. If there is a specification that uniquely defines what the conversion is, this is good. But if not or if there are artistic decisions to be made, like with HDR tone mapping, then it doesn't work. OTOH, the former approach allows the driver to use any and all hardware features it has to realize the conversion, perhaps taking advantage of even fixed-function hardware blocks. The latter approach is much harder to map to hardware features. This dilemma has been discussed in length in https://lists.freedesktop.org/archives/dri-devel/2021-June/311689.html Thanks, pq
Am 23.06.21 um 09:48 schrieb Pekka Paalanen: > On Tue, 22 Jun 2021 11:57:53 +0200 > Werner Sembach <wse@tuxedocomputers.com> wrote: > >> Am 22.06.21 um 09:25 schrieb Pekka Paalanen: >>> On Fri, 18 Jun 2021 11:11:14 +0200 >>> Werner Sembach <wse@tuxedocomputers.com> wrote: >>> >>>> Add "Broadcast RGB" to general drm context so that more drivers besides >>>> i915 and gma500 can implement it without duplicating code. >>>> >>>> Userspace can use this property to tell the graphic driver to use full or >>>> limited color range for a given connector, overwriting the default >>>> behaviour/automatic detection. >>>> >>>> Possible options are: >>>> - Automatic (default/current behaviour) >>>> - Full >>>> - Limited 16:235 >>>> >>>> In theory the driver should be able to automatically detect the monitors >>>> capabilities, but because of flawed standard implementations in Monitors, >>>> this might fail. In this case a manual overwrite is required to not have >>>> washed out colors or lose details in very dark or bright scenes. >>>> >>>> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com> >>>> --- >>>> drivers/gpu/drm/drm_atomic_helper.c | 4 +++ >>>> drivers/gpu/drm/drm_atomic_uapi.c | 4 +++ >>>> drivers/gpu/drm/drm_connector.c | 43 +++++++++++++++++++++++++++++ >>>> include/drm/drm_connector.h | 16 +++++++++++ >>>> 4 files changed, 67 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c >>>> index 90d62f305257..0c89d32efbd0 100644 >>>> --- a/drivers/gpu/drm/drm_atomic_helper.c >>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c >>>> @@ -691,6 +691,10 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, >>>> if (old_connector_state->preferred_color_format != >>>> new_connector_state->preferred_color_format) >>>> new_crtc_state->connectors_changed = true; >>>> + >>>> + if (old_connector_state->preferred_color_range != >>>> + new_connector_state->preferred_color_range) >>>> + new_crtc_state->connectors_changed = true; >>>> } >>>> >>>> if (funcs->atomic_check) >>>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c >>>> index c536f5e22016..c589bb1a8163 100644 >>>> --- a/drivers/gpu/drm/drm_atomic_uapi.c >>>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c >>>> @@ -798,6 +798,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector, >>>> state->max_requested_bpc = val; >>>> } else if (property == connector->preferred_color_format_property) { >>>> state->preferred_color_format = val; >>>> + } else if (property == connector->preferred_color_range_property) { >>>> + state->preferred_color_range = val; >>>> } else if (connector->funcs->atomic_set_property) { >>>> return connector->funcs->atomic_set_property(connector, >>>> state, property, val); >>>> @@ -877,6 +879,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector, >>>> *val = state->max_requested_bpc; >>>> } else if (property == connector->preferred_color_format_property) { >>>> *val = state->preferred_color_format; >>>> + } else if (property == connector->preferred_color_range_property) { >>>> + *val = state->preferred_color_range; >>>> } else if (connector->funcs->atomic_get_property) { >>>> return connector->funcs->atomic_get_property(connector, >>>> state, property, val); >>>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c >>>> index aea03dd02e33..9bc596638613 100644 >>>> --- a/drivers/gpu/drm/drm_connector.c >>>> +++ b/drivers/gpu/drm/drm_connector.c >>>> @@ -905,6 +905,12 @@ static const struct drm_prop_enum_list drm_active_color_format_enum_list[] = { >>>> { DRM_COLOR_FORMAT_YCRCB420, "ycbcr420" }, >>>> }; >>>> >>>> +static const struct drm_prop_enum_list drm_preferred_color_range_enum_list[] = { >>>> + { DRM_MODE_COLOR_RANGE_UNSET, "Automatic" }, >>>> + { DRM_MODE_COLOR_RANGE_FULL, "Full" }, >>>> + { DRM_MODE_COLOR_RANGE_LIMITED_16_235, "Limited 16:235" }, >>> Hi, >>> >>> the same question here about these numbers as I asked on the "active >>> color range" property. >>> >>>> +}; >>>> + >>>> static const struct drm_prop_enum_list drm_active_color_range_enum_list[] = { >>>> { DRM_MODE_COLOR_RANGE_UNSET, "Unknown" }, >>>> { DRM_MODE_COLOR_RANGE_FULL, "Full" }, >>>> @@ -1243,6 +1249,13 @@ static const struct drm_prop_enum_list dp_colorspaces[] = { >>>> * drm_connector_attach_active_color_format_property() to install this >>>> * property. >>>> * >>>> + * Broadcast RGB: >>>> + * This property is used by userspace to change the used color range. When >>>> + * used the driver will use the selected range if valid for the current >>>> + * color format. Drivers to use the function >>>> + * drm_connector_attach_preferred_color_format_property() to create and >>>> + * attach the property to the connector during initialization. >>> An important detail to document here is: does userspace need to >>> take care that pixel data at the connector will already match the set >>> range, or will the driver program the hardware to produce the set range? >> Since until now, the userspace didn't even know for sure if RGB or YCbCr and therefore which full/limited format was >> used I guess it's all kernel space conversion. >>> If the former, then I'm afraid the preference/active property pair >>> design does not work. Userspace needs to make sure the content is in >>> the right range, so the driver cannot second-guess that afterwards. >>> >>> If the latter, then what does the driver assume about color range just >>> before the automatic conversion to the final color range, and does the >>> range conversion happen as the final step in the color pipeline? >>> >>> If I remember the discussion about Intel right, then the driver does >>> the latter and assume that userspace programs KMS to always produce >>> full range pixels. There is no provision for userspace to produce >>> limited range pixels, IIRC. >> I think I remember this too from an answer to one of the revisions of this patchset. > As long as you keep the old KMS property as is, just moving code so it > is used by more drivers, this is fine and one can't do otherwise anyway. > > (The rest of this email is merely pondering the future, so not about > this patch in particular.) > > > But if we had a new, more general property for the range reported to > monitors via infoframes, then it would be worth to re-visit the design. > The HDR properties only set the infoframe and expect userspace to make > sure that the pixels actually correspond to what the infoframes tell > the monitor. One can't do HDR tone mapping automatically in the kernel, > so in that sense the HDR property behaviour is obvious. But which > behaviour would fit range property or others better, I'm not sure. > > Generally there seems to be two approaches to designing KMS properties: > > - Let userspace describe what data it has and what data should be sent > to a monitor, and let the kernel driver magically come up with a > conversion. > > - Only userspace understands how the pixel data is encoded, and > programs the transformations (DEGAMMA/CTM/GAMMA etc.) such, that the > result is what a monitor expects based on e.g. infoframes. Why not both? This patchset is thought to control what's happening "on the cable", so if the input data is in a different format, the kernel has to convert it. Maybe in the future there could be an additional set of "input-" properties. aka "input bpc", "input color format", and "input color range". With an additional constraint that if "input-" property == "active-" property the kernel is not allowed to do any conversion regarding this aspect, giving userspace full control if wanted. > > Doing the former requires policy in the kernel. If there is a > specification that uniquely defines what the conversion is, this is > good. But if not or if there are artistic decisions to be made, like > with HDR tone mapping, then it doesn't work. > > OTOH, the former approach allows the driver to use any and all hardware > features it has to realize the conversion, perhaps taking advantage of > even fixed-function hardware blocks. The latter approach is much harder > to map to hardware features. > > This dilemma has been discussed in length in > https://lists.freedesktop.org/archives/dri-devel/2021-June/311689.html > > > Thanks, > pq
On Wed, 23 Jun 2021 12:10:14 +0200 Werner Sembach <wse@tuxedocomputers.com> wrote: > Am 23.06.21 um 09:48 schrieb Pekka Paalanen: > > On Tue, 22 Jun 2021 11:57:53 +0200 > > Werner Sembach <wse@tuxedocomputers.com> wrote: > > > >> Am 22.06.21 um 09:25 schrieb Pekka Paalanen: > >>> On Fri, 18 Jun 2021 11:11:14 +0200 > >>> Werner Sembach <wse@tuxedocomputers.com> wrote: > >>> > >>>> Add "Broadcast RGB" to general drm context so that more drivers besides > >>>> i915 and gma500 can implement it without duplicating code. > >>>> > >>>> Userspace can use this property to tell the graphic driver to use full or > >>>> limited color range for a given connector, overwriting the default > >>>> behaviour/automatic detection. > >>>> > >>>> Possible options are: > >>>> - Automatic (default/current behaviour) > >>>> - Full > >>>> - Limited 16:235 > >>>> > >>>> In theory the driver should be able to automatically detect the monitors > >>>> capabilities, but because of flawed standard implementations in Monitors, > >>>> this might fail. In this case a manual overwrite is required to not have > >>>> washed out colors or lose details in very dark or bright scenes. > >>>> > >>>> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com> > >>>> --- > >>>> drivers/gpu/drm/drm_atomic_helper.c | 4 +++ > >>>> drivers/gpu/drm/drm_atomic_uapi.c | 4 +++ > >>>> drivers/gpu/drm/drm_connector.c | 43 +++++++++++++++++++++++++++++ > >>>> include/drm/drm_connector.h | 16 +++++++++++ > >>>> 4 files changed, 67 insertions(+) > >>>> > >>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > >>>> index 90d62f305257..0c89d32efbd0 100644 > >>>> --- a/drivers/gpu/drm/drm_atomic_helper.c > >>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c > >>>> @@ -691,6 +691,10 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, > >>>> if (old_connector_state->preferred_color_format != > >>>> new_connector_state->preferred_color_format) > >>>> new_crtc_state->connectors_changed = true; > >>>> + > >>>> + if (old_connector_state->preferred_color_range != > >>>> + new_connector_state->preferred_color_range) > >>>> + new_crtc_state->connectors_changed = true; > >>>> } > >>>> > >>>> if (funcs->atomic_check) > >>>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c > >>>> index c536f5e22016..c589bb1a8163 100644 > >>>> --- a/drivers/gpu/drm/drm_atomic_uapi.c > >>>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c > >>>> @@ -798,6 +798,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector, > >>>> state->max_requested_bpc = val; > >>>> } else if (property == connector->preferred_color_format_property) { > >>>> state->preferred_color_format = val; > >>>> + } else if (property == connector->preferred_color_range_property) { > >>>> + state->preferred_color_range = val; > >>>> } else if (connector->funcs->atomic_set_property) { > >>>> return connector->funcs->atomic_set_property(connector, > >>>> state, property, val); > >>>> @@ -877,6 +879,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector, > >>>> *val = state->max_requested_bpc; > >>>> } else if (property == connector->preferred_color_format_property) { > >>>> *val = state->preferred_color_format; > >>>> + } else if (property == connector->preferred_color_range_property) { > >>>> + *val = state->preferred_color_range; > >>>> } else if (connector->funcs->atomic_get_property) { > >>>> return connector->funcs->atomic_get_property(connector, > >>>> state, property, val); > >>>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > >>>> index aea03dd02e33..9bc596638613 100644 > >>>> --- a/drivers/gpu/drm/drm_connector.c > >>>> +++ b/drivers/gpu/drm/drm_connector.c > >>>> @@ -905,6 +905,12 @@ static const struct drm_prop_enum_list drm_active_color_format_enum_list[] = { > >>>> { DRM_COLOR_FORMAT_YCRCB420, "ycbcr420" }, > >>>> }; > >>>> > >>>> +static const struct drm_prop_enum_list drm_preferred_color_range_enum_list[] = { > >>>> + { DRM_MODE_COLOR_RANGE_UNSET, "Automatic" }, > >>>> + { DRM_MODE_COLOR_RANGE_FULL, "Full" }, > >>>> + { DRM_MODE_COLOR_RANGE_LIMITED_16_235, "Limited 16:235" }, > >>> Hi, > >>> > >>> the same question here about these numbers as I asked on the "active > >>> color range" property. > >>> > >>>> +}; > >>>> + > >>>> static const struct drm_prop_enum_list drm_active_color_range_enum_list[] = { > >>>> { DRM_MODE_COLOR_RANGE_UNSET, "Unknown" }, > >>>> { DRM_MODE_COLOR_RANGE_FULL, "Full" }, > >>>> @@ -1243,6 +1249,13 @@ static const struct drm_prop_enum_list dp_colorspaces[] = { > >>>> * drm_connector_attach_active_color_format_property() to install this > >>>> * property. > >>>> * > >>>> + * Broadcast RGB: > >>>> + * This property is used by userspace to change the used color range. When > >>>> + * used the driver will use the selected range if valid for the current > >>>> + * color format. Drivers to use the function > >>>> + * drm_connector_attach_preferred_color_format_property() to create and > >>>> + * attach the property to the connector during initialization. > >>> An important detail to document here is: does userspace need to > >>> take care that pixel data at the connector will already match the set > >>> range, or will the driver program the hardware to produce the set range? > >> Since until now, the userspace didn't even know for sure if RGB or YCbCr and therefore which full/limited format was > >> used I guess it's all kernel space conversion. > >>> If the former, then I'm afraid the preference/active property pair > >>> design does not work. Userspace needs to make sure the content is in > >>> the right range, so the driver cannot second-guess that afterwards. > >>> > >>> If the latter, then what does the driver assume about color range just > >>> before the automatic conversion to the final color range, and does the > >>> range conversion happen as the final step in the color pipeline? > >>> > >>> If I remember the discussion about Intel right, then the driver does > >>> the latter and assume that userspace programs KMS to always produce > >>> full range pixels. There is no provision for userspace to produce > >>> limited range pixels, IIRC. > >> I think I remember this too from an answer to one of the revisions of this patchset. > > As long as you keep the old KMS property as is, just moving code so it > > is used by more drivers, this is fine and one can't do otherwise anyway. > > > > (The rest of this email is merely pondering the future, so not about > > this patch in particular.) > > > > > > But if we had a new, more general property for the range reported to > > monitors via infoframes, then it would be worth to re-visit the design. > > The HDR properties only set the infoframe and expect userspace to make > > sure that the pixels actually correspond to what the infoframes tell > > the monitor. One can't do HDR tone mapping automatically in the kernel, > > so in that sense the HDR property behaviour is obvious. But which > > behaviour would fit range property or others better, I'm not sure. > > > > Generally there seems to be two approaches to designing KMS properties: > > > > - Let userspace describe what data it has and what data should be sent > > to a monitor, and let the kernel driver magically come up with a > > conversion. > > > > - Only userspace understands how the pixel data is encoded, and > > programs the transformations (DEGAMMA/CTM/GAMMA etc.) such, that the > > result is what a monitor expects based on e.g. infoframes. > > Why not both? Because "both" means you have overlapping sets of properties that might contradict each other. Or then you need a switch between the two models. > This patchset is thought to control what's happening "on the cable", > so if the input data is in a different format, the kernel has to > convert it. Right, if that is the desired semantics. That's not how the HDR property works. Kernel does not convert there. The HDR property only sets infoframes that the monitor interprets. > Maybe in the future there could be an additional set of "input-" > properties. aka "input bpc", "input color format", and "input color > range". With an additional constraint that if "input-" property == > "active-" property the kernel is not allowed to do any conversion > regarding this aspect, giving userspace full control if wanted. If by "input" you mean "the result from userspace provided content going through the configured KMS pixel pipeline", then yes. But it's hard to put it into words accurately. The FB could contain whatever which userspace then programs DEGAMMA and CTM to produce what would be the "input" pixels for example. This is getting closer to the "abstract KMS pipeline" idea which has been predicted to fall apart in the email thread linked below. > > Doing the former requires policy in the kernel. If there is a > > specification that uniquely defines what the conversion is, this is > > good. But if not or if there are artistic decisions to be made, like > > with HDR tone mapping, then it doesn't work. > > > > OTOH, the former approach allows the driver to use any and all hardware > > features it has to realize the conversion, perhaps taking advantage of > > even fixed-function hardware blocks. The latter approach is much harder > > to map to hardware features. > > > > This dilemma has been discussed in length in > > https://lists.freedesktop.org/archives/dri-devel/2021-June/311689.html > > Thanks, pq
Am 23.06.21 um 13:26 schrieb Pekka Paalanen: > On Wed, 23 Jun 2021 12:10:14 +0200 > Werner Sembach <wse@tuxedocomputers.com> wrote: > >> Am 23.06.21 um 09:48 schrieb Pekka Paalanen: >>> On Tue, 22 Jun 2021 11:57:53 +0200 >>> Werner Sembach <wse@tuxedocomputers.com> wrote: >>> >>>> Am 22.06.21 um 09:25 schrieb Pekka Paalanen: >>>>> On Fri, 18 Jun 2021 11:11:14 +0200 >>>>> Werner Sembach <wse@tuxedocomputers.com> wrote: >>>>> >>>>>> Add "Broadcast RGB" to general drm context so that more drivers besides >>>>>> i915 and gma500 can implement it without duplicating code. >>>>>> >>>>>> Userspace can use this property to tell the graphic driver to use full or >>>>>> limited color range for a given connector, overwriting the default >>>>>> behaviour/automatic detection. >>>>>> >>>>>> Possible options are: >>>>>> - Automatic (default/current behaviour) >>>>>> - Full >>>>>> - Limited 16:235 >>>>>> >>>>>> In theory the driver should be able to automatically detect the monitors >>>>>> capabilities, but because of flawed standard implementations in Monitors, >>>>>> this might fail. In this case a manual overwrite is required to not have >>>>>> washed out colors or lose details in very dark or bright scenes. >>>>>> >>>>>> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com> >>>>>> --- >>>>>> drivers/gpu/drm/drm_atomic_helper.c | 4 +++ >>>>>> drivers/gpu/drm/drm_atomic_uapi.c | 4 +++ >>>>>> drivers/gpu/drm/drm_connector.c | 43 +++++++++++++++++++++++++++++ >>>>>> include/drm/drm_connector.h | 16 +++++++++++ >>>>>> 4 files changed, 67 insertions(+) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c >>>>>> index 90d62f305257..0c89d32efbd0 100644 >>>>>> --- a/drivers/gpu/drm/drm_atomic_helper.c >>>>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c >>>>>> @@ -691,6 +691,10 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, >>>>>> if (old_connector_state->preferred_color_format != >>>>>> new_connector_state->preferred_color_format) >>>>>> new_crtc_state->connectors_changed = true; >>>>>> + >>>>>> + if (old_connector_state->preferred_color_range != >>>>>> + new_connector_state->preferred_color_range) >>>>>> + new_crtc_state->connectors_changed = true; >>>>>> } >>>>>> >>>>>> if (funcs->atomic_check) >>>>>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c >>>>>> index c536f5e22016..c589bb1a8163 100644 >>>>>> --- a/drivers/gpu/drm/drm_atomic_uapi.c >>>>>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c >>>>>> @@ -798,6 +798,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector, >>>>>> state->max_requested_bpc = val; >>>>>> } else if (property == connector->preferred_color_format_property) { >>>>>> state->preferred_color_format = val; >>>>>> + } else if (property == connector->preferred_color_range_property) { >>>>>> + state->preferred_color_range = val; >>>>>> } else if (connector->funcs->atomic_set_property) { >>>>>> return connector->funcs->atomic_set_property(connector, >>>>>> state, property, val); >>>>>> @@ -877,6 +879,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector, >>>>>> *val = state->max_requested_bpc; >>>>>> } else if (property == connector->preferred_color_format_property) { >>>>>> *val = state->preferred_color_format; >>>>>> + } else if (property == connector->preferred_color_range_property) { >>>>>> + *val = state->preferred_color_range; >>>>>> } else if (connector->funcs->atomic_get_property) { >>>>>> return connector->funcs->atomic_get_property(connector, >>>>>> state, property, val); >>>>>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c >>>>>> index aea03dd02e33..9bc596638613 100644 >>>>>> --- a/drivers/gpu/drm/drm_connector.c >>>>>> +++ b/drivers/gpu/drm/drm_connector.c >>>>>> @@ -905,6 +905,12 @@ static const struct drm_prop_enum_list drm_active_color_format_enum_list[] = { >>>>>> { DRM_COLOR_FORMAT_YCRCB420, "ycbcr420" }, >>>>>> }; >>>>>> >>>>>> +static const struct drm_prop_enum_list drm_preferred_color_range_enum_list[] = { >>>>>> + { DRM_MODE_COLOR_RANGE_UNSET, "Automatic" }, >>>>>> + { DRM_MODE_COLOR_RANGE_FULL, "Full" }, >>>>>> + { DRM_MODE_COLOR_RANGE_LIMITED_16_235, "Limited 16:235" }, >>>>> Hi, >>>>> >>>>> the same question here about these numbers as I asked on the "active >>>>> color range" property. >>>>> >>>>>> +}; >>>>>> + >>>>>> static const struct drm_prop_enum_list drm_active_color_range_enum_list[] = { >>>>>> { DRM_MODE_COLOR_RANGE_UNSET, "Unknown" }, >>>>>> { DRM_MODE_COLOR_RANGE_FULL, "Full" }, >>>>>> @@ -1243,6 +1249,13 @@ static const struct drm_prop_enum_list dp_colorspaces[] = { >>>>>> * drm_connector_attach_active_color_format_property() to install this >>>>>> * property. >>>>>> * >>>>>> + * Broadcast RGB: >>>>>> + * This property is used by userspace to change the used color range. When >>>>>> + * used the driver will use the selected range if valid for the current >>>>>> + * color format. Drivers to use the function >>>>>> + * drm_connector_attach_preferred_color_format_property() to create and >>>>>> + * attach the property to the connector during initialization. >>>>> An important detail to document here is: does userspace need to >>>>> take care that pixel data at the connector will already match the set >>>>> range, or will the driver program the hardware to produce the set range? >>>> Since until now, the userspace didn't even know for sure if RGB or YCbCr and therefore which full/limited format was >>>> used I guess it's all kernel space conversion. >>>>> If the former, then I'm afraid the preference/active property pair >>>>> design does not work. Userspace needs to make sure the content is in >>>>> the right range, so the driver cannot second-guess that afterwards. >>>>> >>>>> If the latter, then what does the driver assume about color range just >>>>> before the automatic conversion to the final color range, and does the >>>>> range conversion happen as the final step in the color pipeline? >>>>> >>>>> If I remember the discussion about Intel right, then the driver does >>>>> the latter and assume that userspace programs KMS to always produce >>>>> full range pixels. There is no provision for userspace to produce >>>>> limited range pixels, IIRC. >>>> I think I remember this too from an answer to one of the revisions of this patchset. >>> As long as you keep the old KMS property as is, just moving code so it >>> is used by more drivers, this is fine and one can't do otherwise anyway. >>> >>> (The rest of this email is merely pondering the future, so not about >>> this patch in particular.) >>> >>> >>> But if we had a new, more general property for the range reported to >>> monitors via infoframes, then it would be worth to re-visit the design. >>> The HDR properties only set the infoframe and expect userspace to make >>> sure that the pixels actually correspond to what the infoframes tell >>> the monitor. One can't do HDR tone mapping automatically in the kernel, >>> so in that sense the HDR property behaviour is obvious. But which >>> behaviour would fit range property or others better, I'm not sure. >>> >>> Generally there seems to be two approaches to designing KMS properties: >>> >>> - Let userspace describe what data it has and what data should be sent >>> to a monitor, and let the kernel driver magically come up with a >>> conversion. >>> >>> - Only userspace understands how the pixel data is encoded, and >>> programs the transformations (DEGAMMA/CTM/GAMMA etc.) such, that the >>> result is what a monitor expects based on e.g. infoframes. >> Why not both? > Because "both" means you have overlapping sets of properties that might > contradict each other. Or then you need a switch between the two models. > >> This patchset is thought to control what's happening "on the cable", >> so if the input data is in a different format, the kernel has to >> convert it. > Right, if that is the desired semantics. > > That's not how the HDR property works. Kernel does not convert there. > The HDR property only sets infoframes that the monitor interprets. > >> Maybe in the future there could be an additional set of "input-" >> properties. aka "input bpc", "input color format", and "input color >> range". With an additional constraint that if "input-" property == >> "active-" property the kernel is not allowed to do any conversion >> regarding this aspect, giving userspace full control if wanted. > If by "input" you mean "the result from userspace provided content > going through the configured KMS pixel pipeline", then yes. But it's > hard to put it into words accurately. > > The FB could contain whatever which userspace then programs DEGAMMA and > CTM to produce what would be the "input" pixels for example. > > This is getting closer to the "abstract KMS pipeline" idea which has > been predicted to fall apart in the email thread linked below. Ok, I'm not deep enough in the topic. For this patchset anyways I try to stick to the preexisting behavior of "max bpc" and "Broadcast RGB", which I believe is the "on the cable/kernel does the conversion" discussed. I will double check on that before I submit the next revision. > >>> Doing the former requires policy in the kernel. If there is a >>> specification that uniquely defines what the conversion is, this is >>> good. But if not or if there are artistic decisions to be made, like >>> with HDR tone mapping, then it doesn't work. >>> >>> OTOH, the former approach allows the driver to use any and all hardware >>> features it has to realize the conversion, perhaps taking advantage of >>> even fixed-function hardware blocks. The latter approach is much harder >>> to map to hardware features. >>> >>> This dilemma has been discussed in length in >>> https://lists.freedesktop.org/archives/dri-devel/2021-June/311689.html >>> > Thanks, > pq
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 90d62f305257..0c89d32efbd0 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -691,6 +691,10 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, if (old_connector_state->preferred_color_format != new_connector_state->preferred_color_format) new_crtc_state->connectors_changed = true; + + if (old_connector_state->preferred_color_range != + new_connector_state->preferred_color_range) + new_crtc_state->connectors_changed = true; } if (funcs->atomic_check) diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index c536f5e22016..c589bb1a8163 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -798,6 +798,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector, state->max_requested_bpc = val; } else if (property == connector->preferred_color_format_property) { state->preferred_color_format = val; + } else if (property == connector->preferred_color_range_property) { + state->preferred_color_range = val; } else if (connector->funcs->atomic_set_property) { return connector->funcs->atomic_set_property(connector, state, property, val); @@ -877,6 +879,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector, *val = state->max_requested_bpc; } else if (property == connector->preferred_color_format_property) { *val = state->preferred_color_format; + } else if (property == connector->preferred_color_range_property) { + *val = state->preferred_color_range; } else if (connector->funcs->atomic_get_property) { return connector->funcs->atomic_get_property(connector, state, property, val); diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index aea03dd02e33..9bc596638613 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -905,6 +905,12 @@ static const struct drm_prop_enum_list drm_active_color_format_enum_list[] = { { DRM_COLOR_FORMAT_YCRCB420, "ycbcr420" }, }; +static const struct drm_prop_enum_list drm_preferred_color_range_enum_list[] = { + { DRM_MODE_COLOR_RANGE_UNSET, "Automatic" }, + { DRM_MODE_COLOR_RANGE_FULL, "Full" }, + { DRM_MODE_COLOR_RANGE_LIMITED_16_235, "Limited 16:235" }, +}; + static const struct drm_prop_enum_list drm_active_color_range_enum_list[] = { { DRM_MODE_COLOR_RANGE_UNSET, "Unknown" }, { DRM_MODE_COLOR_RANGE_FULL, "Full" }, @@ -1243,6 +1249,13 @@ static const struct drm_prop_enum_list dp_colorspaces[] = { * drm_connector_attach_active_color_format_property() to install this * property. * + * Broadcast RGB: + * This property is used by userspace to change the used color range. When + * used the driver will use the selected range if valid for the current + * color format. Drivers to use the function + * drm_connector_attach_preferred_color_format_property() to create and + * attach the property to the connector during initialization. + * * active color range: * This read-only property tells userspace the color range actually used by * the hardware display engine on "the cable" on a connector. The chosen @@ -2324,6 +2337,36 @@ void drm_connector_set_active_color_format_property(struct drm_connector *connec } EXPORT_SYMBOL(drm_connector_set_active_color_format_property); +/** + * drm_connector_attach_preferred_color_range_property - attach "Broadcast RGB" property + * @connector: connector to attach preferred color range property on. + * + * This is used to add support for selecting a color range on a connector. + * + * Returns: + * Zero on success, negative errno on failure. + */ +int drm_connector_attach_preferred_color_range_property(struct drm_connector *connector) +{ + struct drm_device *dev = connector->dev; + struct drm_property *prop; + + if (!connector->preferred_color_range_property) { + prop = drm_property_create_enum(dev, 0, "Broadcast RGB", + drm_preferred_color_range_enum_list, + ARRAY_SIZE(drm_preferred_color_range_enum_list)); + if (!prop) + return -ENOMEM; + + connector->preferred_color_range_property = prop; + drm_object_attach_property(&connector->base, prop, DRM_MODE_COLOR_RANGE_UNSET); + connector->state->preferred_color_range = DRM_MODE_COLOR_RANGE_UNSET; + } + + return 0; +} +EXPORT_SYMBOL(drm_connector_attach_preferred_color_range_property); + /** * drm_connector_attach_active_color_range_property - attach "active color range" property * @connector: connector to attach active color range property on. diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index 7b85407ba45c..b319760d4a8c 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -809,6 +809,15 @@ struct drm_connector_state { */ u32 preferred_color_format; + /** + * preferred_color_range: Property set by userspace via "Broadcast RGB" + * property to tell the GPU driver which color range to use. It + * overwrites existing automatic detection mechanisms, if set and valid + * for the current color format. Userspace can check for (un-)successful + * application via the "active color range" property. + */ + enum drm_mode_color_range preferred_color_range; + /** * @hdr_output_metadata: * DRM blob property for HDR output metadata @@ -1426,6 +1435,12 @@ struct drm_connector { */ struct drm_property *active_color_format_property; + /** + * @preferred_color_range_property: Default connector property for the + * preferred color range to be driven out of the connector. + */ + struct drm_property *preferred_color_range_property; + /** * @active_color_range_property: Default connector property for the * active color range to be driven out of the connector. @@ -1760,6 +1775,7 @@ int drm_connector_attach_preferred_color_format_property(struct drm_connector *c int drm_connector_attach_active_color_format_property(struct drm_connector *connector); void drm_connector_set_active_color_format_property(struct drm_connector *connector, u32 active_color_format); +int drm_connector_attach_preferred_color_range_property(struct drm_connector *connector); int drm_connector_attach_active_color_range_property(struct drm_connector *connector); void drm_connector_set_active_color_range_property(struct drm_connector *connector, enum drm_mode_color_range active_color_range);
Add "Broadcast RGB" to general drm context so that more drivers besides i915 and gma500 can implement it without duplicating code. Userspace can use this property to tell the graphic driver to use full or limited color range for a given connector, overwriting the default behaviour/automatic detection. Possible options are: - Automatic (default/current behaviour) - Full - Limited 16:235 In theory the driver should be able to automatically detect the monitors capabilities, but because of flawed standard implementations in Monitors, this might fail. In this case a manual overwrite is required to not have washed out colors or lose details in very dark or bright scenes. Signed-off-by: Werner Sembach <wse@tuxedocomputers.com> --- drivers/gpu/drm/drm_atomic_helper.c | 4 +++ drivers/gpu/drm/drm_atomic_uapi.c | 4 +++ drivers/gpu/drm/drm_connector.c | 43 +++++++++++++++++++++++++++++ include/drm/drm_connector.h | 16 +++++++++++ 4 files changed, 67 insertions(+)