On 12/13/22 05:23, Pekka Paalanen wrote: > On Mon, 12 Dec 2022 13:21:27 -0500 > Harry Wentland <harry.wentland@xxxxxxx> wrote: > >> Drivers might not support all colorspaces defined in >> dp_colorspaces and hdmi_colorspaces. This results in >> undefined behavior when userspace is setting an >> unsupported colorspace. >> >> Allow drivers to pass the list of supported colorspaces >> when creating the colorspace property. > > Hi Harry, > > what is there for drivers to support? Isn't this just infoframe data > that shall be sent down to the sink as-is with no other effect? > You have a good point. Right now the supported colorspaces de-facto depend on driver implementations as you can see in [1] for i915 and [2] for amdgpu. The amdgpu driver will also program the MSA [3] for DP connections, and a bunch of other things which are deeper in the driver. [1] https://gitlab.freedesktop.org/hwentland/linux/-/blob/hdr-colorimetry/drivers/gpu/drm/i915/display/intel_dp.c#L1741 [2] https://gitlab.freedesktop.org/hwentland/linux/-/blob/hdr-colorimetry/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c#L5155 [3] https://gitlab.freedesktop.org/hwentland/linux/-/blob/hdr-colorimetry/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_stream_encoder.c#L368 I don't know why the DP VSC stuff needs to be in drivers. It should be common. The MSA packing would likely have to be driver specific since the mechanism of packing it is HW specific. I'll have a closer look and see if we can eliminate the "driver supported" bit. If we can't we'll probably need to describe the reasoning better. Will it be a problem if the list of supported colorspaces differs between drivers? Harry > Is the driver confusing colorimetry with color-representation (the > RGB-YCbCr conversion)? Or is this property defining both? > > I feel that the documentation of "Colorspace" KMS connector property > needs clarification, and a list of potentially available values with > explanations, more than just a reference to CTA-816-H which it does not > even do yet. > > Perhaps a table, where for each enum drm_colorspace entry has a row explaining: > > > Thanks, > pq > > >> Signed-off-by: Harry Wentland <harry.wentland@xxxxxxx> >> Cc: Pekka Paalanen <ppaalanen@xxxxxxxxx> >> Cc: Sebastian Wick <sebastian.wick@xxxxxxxxxx> >> Cc: Vitaly.Prosyak@xxxxxxx >> Cc: Uma Shankar <uma.shankar@xxxxxxxxx> >> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> >> Cc: Joshua Ashton <joshua@xxxxxxxxx> >> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx >> Cc: amd-gfx@xxxxxxxxxxxxxxxxxxxxx >> --- >> drivers/gpu/drm/drm_connector.c | 140 +++++++++--------- >> .../gpu/drm/i915/display/intel_connector.c | 4 +- >> drivers/gpu/drm/vc4/vc4_hdmi.c | 2 +- >> include/drm/drm_connector.h | 8 +- >> 4 files changed, 83 insertions(+), 71 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c >> index ddba0b9fcc17..0df5db3e4fec 100644 >> --- a/drivers/gpu/drm/drm_connector.c >> +++ b/drivers/gpu/drm/drm_connector.c >> @@ -1012,64 +1012,57 @@ static const struct drm_prop_enum_list drm_dp_subconnector_enum_list[] = { >> DRM_ENUM_NAME_FN(drm_get_dp_subconnector_name, >> drm_dp_subconnector_enum_list) >> >> -static const struct drm_prop_enum_list hdmi_colorspaces[] = { >> - /* For Default case, driver will set the colorspace */ >> - { DRM_MODE_COLORIMETRY_DEFAULT, "Default" }, >> - /* Standard Definition Colorimetry based on CEA 861 */ >> - { DRM_MODE_COLORIMETRY_SMPTE_170M_YCC, "SMPTE_170M_YCC" }, >> - { DRM_MODE_COLORIMETRY_BT709_YCC, "BT709_YCC" }, >> - /* Standard Definition Colorimetry based on IEC 61966-2-4 */ >> - { DRM_MODE_COLORIMETRY_XVYCC_601, "XVYCC_601" }, >> - /* High Definition Colorimetry based on IEC 61966-2-4 */ >> - { DRM_MODE_COLORIMETRY_XVYCC_709, "XVYCC_709" }, >> - /* Colorimetry based on IEC 61966-2-1/Amendment 1 */ >> - { DRM_MODE_COLORIMETRY_SYCC_601, "SYCC_601" }, >> - /* Colorimetry based on IEC 61966-2-5 [33] */ >> - { DRM_MODE_COLORIMETRY_OPYCC_601, "opYCC_601" }, >> - /* Colorimetry based on IEC 61966-2-5 */ >> - { DRM_MODE_COLORIMETRY_OPRGB, "opRGB" }, >> - /* Colorimetry based on ITU-R BT.2020 */ >> - { DRM_MODE_COLORIMETRY_BT2020_CYCC, "BT2020_CYCC" }, >> - /* Colorimetry based on ITU-R BT.2020 */ >> - { DRM_MODE_COLORIMETRY_BT2020_RGB, "BT2020_RGB" }, >> - /* Colorimetry based on ITU-R BT.2020 */ >> - { DRM_MODE_COLORIMETRY_BT2020_YCC, "BT2020_YCC" }, >> - /* Added as part of Additional Colorimetry Extension in 861.G */ >> - { DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65, "DCI-P3_RGB_D65" }, >> - { DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER, "DCI-P3_RGB_Theater" }, >> +static const char * const colorspace_names[] = { >> + [DRM_MODE_COLORIMETRY_DEFAULT] = "Default", >> + [DRM_MODE_COLORIMETRY_SMPTE_170M_YCC] = "SMPTE_170M_YCC", >> + [DRM_MODE_COLORIMETRY_BT709_YCC] = "BT709_YCC", >> + [DRM_MODE_COLORIMETRY_XVYCC_601] = "XVYCC_601", >> + [DRM_MODE_COLORIMETRY_XVYCC_709] = "XVYCC_709", >> + [DRM_MODE_COLORIMETRY_SYCC_601] = "SYCC_601", >> + [DRM_MODE_COLORIMETRY_OPYCC_601] = "opYCC_601", >> + [DRM_MODE_COLORIMETRY_OPRGB] = "opRGB", >> + [DRM_MODE_COLORIMETRY_BT2020_CYCC] = "BT2020_CYCC", >> + [DRM_MODE_COLORIMETRY_BT2020_RGB] = "BT2020_RGB", >> + [DRM_MODE_COLORIMETRY_BT2020_YCC] = "BT2020_YCC", >> + [DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65] = "P3_RGB_D65", >> + [DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER] = "P3_RGB_Theater", >> + [DRM_MODE_COLORIMETRY_RGB_WIDE_FIXED] = "RGB_WIDE_FIXED", >> + [DRM_MODE_COLORIMETRY_RGB_WIDE_FLOAT] = "RGB_WIDE_FLOAT", >> + [DRM_MODE_COLORIMETRY_BT601_YCC] = "BT601_YCC", >> }; >> >> +static const u32 hdmi_colorspaces = >> + BIT(DRM_MODE_COLORIMETRY_SMPTE_170M_YCC) | >> + BIT(DRM_MODE_COLORIMETRY_BT709_YCC) | >> + BIT(DRM_MODE_COLORIMETRY_XVYCC_601) | >> + BIT(DRM_MODE_COLORIMETRY_XVYCC_709) | >> + BIT(DRM_MODE_COLORIMETRY_SYCC_601) | >> + BIT(DRM_MODE_COLORIMETRY_OPYCC_601) | >> + BIT(DRM_MODE_COLORIMETRY_OPRGB) | >> + BIT(DRM_MODE_COLORIMETRY_BT2020_CYCC) | >> + BIT(DRM_MODE_COLORIMETRY_BT2020_RGB) | >> + BIT(DRM_MODE_COLORIMETRY_BT2020_YCC) | >> + BIT(DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65) | >> + BIT(DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER); >> + >> /* >> * As per DP 1.4a spec, 2.2.5.7.5 VSC SDP Payload for Pixel Encoding/Colorimetry >> * Format Table 2-120 >> */ >> -static const struct drm_prop_enum_list dp_colorspaces[] = { >> - /* For Default case, driver will set the colorspace */ >> - { DRM_MODE_COLORIMETRY_DEFAULT, "Default" }, >> - { DRM_MODE_COLORIMETRY_RGB_WIDE_FIXED, "RGB_Wide_Gamut_Fixed_Point" }, >> - /* Colorimetry based on scRGB (IEC 61966-2-2) */ >> - { DRM_MODE_COLORIMETRY_RGB_WIDE_FLOAT, "RGB_Wide_Gamut_Floating_Point" }, >> - /* Colorimetry based on IEC 61966-2-5 */ >> - { DRM_MODE_COLORIMETRY_OPRGB, "opRGB" }, >> - /* Colorimetry based on SMPTE RP 431-2 */ >> - { DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65, "DCI-P3_RGB_D65" }, >> - /* Colorimetry based on ITU-R BT.2020 */ >> - { DRM_MODE_COLORIMETRY_BT2020_RGB, "BT2020_RGB" }, >> - { DRM_MODE_COLORIMETRY_BT601_YCC, "BT601_YCC" }, >> - { DRM_MODE_COLORIMETRY_BT709_YCC, "BT709_YCC" }, >> - /* Standard Definition Colorimetry based on IEC 61966-2-4 */ >> - { DRM_MODE_COLORIMETRY_XVYCC_601, "XVYCC_601" }, >> - /* High Definition Colorimetry based on IEC 61966-2-4 */ >> - { DRM_MODE_COLORIMETRY_XVYCC_709, "XVYCC_709" }, >> - /* Colorimetry based on IEC 61966-2-1/Amendment 1 */ >> - { DRM_MODE_COLORIMETRY_SYCC_601, "SYCC_601" }, >> - /* Colorimetry based on IEC 61966-2-5 [33] */ >> - { DRM_MODE_COLORIMETRY_OPYCC_601, "opYCC_601" }, >> - /* Colorimetry based on ITU-R BT.2020 */ >> - { DRM_MODE_COLORIMETRY_BT2020_CYCC, "BT2020_CYCC" }, >> - /* Colorimetry based on ITU-R BT.2020 */ >> - { DRM_MODE_COLORIMETRY_BT2020_YCC, "BT2020_YCC" }, >> -}; >> +static const u32 dp_colorspaces = >> + BIT(DRM_MODE_COLORIMETRY_RGB_WIDE_FIXED) | >> + BIT(DRM_MODE_COLORIMETRY_RGB_WIDE_FLOAT) | >> + BIT(DRM_MODE_COLORIMETRY_OPRGB) | >> + BIT(DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65) | >> + BIT(DRM_MODE_COLORIMETRY_BT2020_RGB) | >> + BIT(DRM_MODE_COLORIMETRY_BT601_YCC) | >> + BIT(DRM_MODE_COLORIMETRY_BT709_YCC) | >> + BIT(DRM_MODE_COLORIMETRY_XVYCC_601) | >> + BIT(DRM_MODE_COLORIMETRY_XVYCC_709) | >> + BIT(DRM_MODE_COLORIMETRY_SYCC_601) | >> + BIT(DRM_MODE_COLORIMETRY_OPYCC_601) | >> + BIT(DRM_MODE_COLORIMETRY_BT2020_CYCC) | >> + BIT(DRM_MODE_COLORIMETRY_BT2020_YCC); >> >> /** >> * DOC: standard connector properties >> @@ -1972,21 +1965,34 @@ EXPORT_SYMBOL(drm_mode_create_aspect_ratio_property); >> */ >> >> static int drm_mode_create_colorspace_property(struct drm_connector *connector, >> - const struct drm_prop_enum_list *colorspaces, >> - int size) >> + u32 supported_colorspaces) >> { >> struct drm_device *dev = connector->dev; >> + u32 colorspaces = supported_colorspaces | BIT(DRM_MODE_COLORIMETRY_DEFAULT); >> + struct drm_prop_enum_list enum_list[DRM_MODE_COLORIMETRY_MAX]; >> + int i, len; >> >> if (connector->colorspace_property) >> return 0; >> >> - if (!colorspaces) >> - return 0; >> + if (WARN_ON(supported_colorspaces == 0 || >> + (supported_colorspaces & -BIT(DRM_MODE_COLORIMETRY_MAX)) != 0)) >> + return -EINVAL; >> + >> + len = 0; >> + for (i = 0; i < DRM_MODE_COLORIMETRY_MAX; i++) { >> + if ((colorspaces & BIT(i)) == 0) >> + continue; >> + >> + enum_list[len].type = i; >> + enum_list[len].name = colorspace_names[i]; >> + len++; >> + } >> >> connector->colorspace_property = >> drm_property_create_enum(dev, DRM_MODE_PROP_ENUM, "Colorspace", >> - colorspaces, >> - size); >> + enum_list, >> + len); >> >> if (!connector->colorspace_property) >> return -ENOMEM; >> @@ -2003,11 +2009,12 @@ static int drm_mode_create_colorspace_property(struct drm_connector *connector, >> * Returns: >> * Zero on success, negative errno on failure. >> */ >> -int drm_mode_create_hdmi_colorspace_property(struct drm_connector *connector) >> +int drm_mode_create_hdmi_colorspace_property(struct drm_connector *connector, >> + u32 supported_colorspaces) >> { >> - return drm_mode_create_colorspace_property(connector, >> - hdmi_colorspaces, >> - ARRAY_SIZE(hdmi_colorspaces)); >> + u32 colorspaces = supported_colorspaces & hdmi_colorspaces; >> + >> + return drm_mode_create_colorspace_property(connector, colorspaces); >> } >> EXPORT_SYMBOL(drm_mode_create_hdmi_colorspace_property); >> >> @@ -2021,11 +2028,12 @@ EXPORT_SYMBOL(drm_mode_create_hdmi_colorspace_property); >> * Returns: >> * Zero on success, negative errno on failure. >> */ >> -int drm_mode_create_dp_colorspace_property(struct drm_connector *connector) >> +int drm_mode_create_dp_colorspace_property(struct drm_connector *connector, >> + u32 supported_colorspaces) >> { >> - return drm_mode_create_colorspace_property(connector, >> - dp_colorspaces, >> - ARRAY_SIZE(dp_colorspaces)); >> + u32 colorspaces = supported_colorspaces & dp_colorspaces; >> + >> + return drm_mode_create_colorspace_property(connector, colorspaces); >> } >> EXPORT_SYMBOL(drm_mode_create_dp_colorspace_property); >> >> diff --git a/drivers/gpu/drm/i915/display/intel_connector.c b/drivers/gpu/drm/i915/display/intel_connector.c >> index 1dcc268927a2..6e7cef58a626 100644 >> --- a/drivers/gpu/drm/i915/display/intel_connector.c >> +++ b/drivers/gpu/drm/i915/display/intel_connector.c >> @@ -283,13 +283,13 @@ intel_attach_aspect_ratio_property(struct drm_connector *connector) >> void >> intel_attach_hdmi_colorspace_property(struct drm_connector *connector) >> { >> - if (!drm_mode_create_hdmi_colorspace_property(connector)) >> + if (!drm_mode_create_hdmi_colorspace_property(connector, 0xffffffff)) >> drm_connector_attach_colorspace_property(connector); >> } >> >> void >> intel_attach_dp_colorspace_property(struct drm_connector *connector) >> { >> - if (!drm_mode_create_dp_colorspace_property(connector)) >> + if (!drm_mode_create_dp_colorspace_property(connector, 0xffffffff)) >> drm_connector_attach_colorspace_property(connector); >> } >> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c >> index 6ab83296b0e4..8d08d6a36f37 100644 >> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c >> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c >> @@ -416,7 +416,7 @@ static int vc4_hdmi_connector_init(struct drm_device *dev, >> if (ret) >> return ret; >> >> - ret = drm_mode_create_hdmi_colorspace_property(connector); >> + ret = drm_mode_create_hdmi_colorspace_property(connector, 0xffffffff); >> if (ret) >> return ret; >> >> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h >> index edef65388c29..5825c6ab969b 100644 >> --- a/include/drm/drm_connector.h >> +++ b/include/drm/drm_connector.h >> @@ -30,6 +30,7 @@ >> #include <linux/notifier.h> >> #include <drm/drm_mode_object.h> >> #include <drm/drm_util.h> >> +#include <drm/drm_property.h> >> >> #include <uapi/drm/drm_mode.h> >> >> @@ -393,6 +394,7 @@ enum drm_colorspace { >> DRM_MODE_COLORIMETRY_RGB_WIDE_FIXED, >> DRM_MODE_COLORIMETRY_RGB_WIDE_FLOAT, >> DRM_MODE_COLORIMETRY_BT601_YCC, >> + DRM_MODE_COLORIMETRY_MAX >> }; >> >> /** >> @@ -1818,8 +1820,10 @@ int drm_connector_attach_hdr_output_metadata_property(struct drm_connector *conn >> bool drm_connector_atomic_hdr_metadata_equal(struct drm_connector_state *old_state, >> struct drm_connector_state *new_state); >> int drm_mode_create_aspect_ratio_property(struct drm_device *dev); >> -int drm_mode_create_hdmi_colorspace_property(struct drm_connector *connector); >> -int drm_mode_create_dp_colorspace_property(struct drm_connector *connector); >> +int drm_mode_create_hdmi_colorspace_property(struct drm_connector *connector, >> + u32 supported_colorspaces); >> +int drm_mode_create_dp_colorspace_property(struct drm_connector *connector, >> + u32 supported_colorspaces); >> int drm_mode_create_content_type_property(struct drm_device *dev); >> int drm_mode_create_suggested_offset_properties(struct drm_device *dev); >> >
