| Message ID | 20221212182137.374625-7-harry.wentland@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enable Colorspace connector property in amdgpu | expand |
On Mon, 12 Dec 2022, Harry Wentland <harry.wentland@amd.com> 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. > > Signed-off-by: Harry Wentland <harry.wentland@amd.com> > Cc: Pekka Paalanen <ppaalanen@gmail.com> > Cc: Sebastian Wick <sebastian.wick@redhat.com> > Cc: Vitaly.Prosyak@amd.com > Cc: Uma Shankar <uma.shankar@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Joshua Ashton <joshua@froggi.es> > Cc: dri-devel@lists.freedesktop.org > Cc: amd-gfx@lists.freedesktop.org > --- > 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); > } The 0xffffffff here looks rather ugly. Since 0 is a forbidden value for the mask and there's a WARN_ON() for it, could we perhaps repurpose 0 for the default "all"? If the intention is for drivers to fix the default afterwards, there could even be a drm_dbg_kms() to log this. Something like this, for drm_mode_create_hdmi_colorspace_property(): u32 colorspaces = supported_colorspaces & hdmi_colorspaces; if (!supported_colorspaces) { colorspaces = hdmi_colorspaces; drm_dbg_kms(...); } BR, Jani. > 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);
On Mon, 12 Dec 2022 13:21:27 -0500 Harry Wentland <harry.wentland@amd.com> 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? 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@amd.com> > Cc: Pekka Paalanen <ppaalanen@gmail.com> > Cc: Sebastian Wick <sebastian.wick@redhat.com> > Cc: Vitaly.Prosyak@amd.com > Cc: Uma Shankar <uma.shankar@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Joshua Ashton <joshua@froggi.es> > Cc: dri-devel@lists.freedesktop.org > Cc: amd-gfx@lists.freedesktop.org > --- > 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); >
Sorry, hand slipped on keyboard and sent out a draft of this email too early. On Mon, 12 Dec 2022 13:21:27 -0500 Harry Wentland <harry.wentland@amd.com> 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? The kerneldoc for "Colorspace" says it has no other effect. 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-861-H which it does not even do yet. Perhaps a table, where for each enum drm_colorspace entry has a row explaining the expectations of the sink: - primaries and white point - transfer characteristic - YCbCr-RGB or similar conversion to/from some RGB Each cell can be a reference to a spec like BT.709 or BT.601 (525 line). I think this belongs in the kernel doc more than in color-and-hdr. CTA-861-H does not give all the information but refers to things like xvYCC601 which you then need to figure out from Wikipedia or whatever which is annoying and raises questions about its correctness. Would be better if someone who actually has access to the authoritative specs would review the table. Thanks, pq > Signed-off-by: Harry Wentland <harry.wentland@amd.com> > Cc: Pekka Paalanen <ppaalanen@gmail.com> > Cc: Sebastian Wick <sebastian.wick@redhat.com> > Cc: Vitaly.Prosyak@amd.com > Cc: Uma Shankar <uma.shankar@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Joshua Ashton <joshua@froggi.es> > Cc: dri-devel@lists.freedesktop.org > Cc: amd-gfx@lists.freedesktop.org > --- > 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); >
On 12/13/22 05:23, Pekka Paalanen wrote: > On Mon, 12 Dec 2022 13:21:27 -0500 > Harry Wentland <harry.wentland@amd.com> 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@amd.com> >> Cc: Pekka Paalanen <ppaalanen@gmail.com> >> Cc: Sebastian Wick <sebastian.wick@redhat.com> >> Cc: Vitaly.Prosyak@amd.com >> Cc: Uma Shankar <uma.shankar@intel.com> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> Cc: Joshua Ashton <joshua@froggi.es> >> Cc: dri-devel@lists.freedesktop.org >> Cc: amd-gfx@lists.freedesktop.org >> --- >> 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); >> >
On 12/13/22 05:34, Pekka Paalanen wrote: > Sorry, hand slipped on keyboard and sent out a draft of this email too > early. > > > On Mon, 12 Dec 2022 13:21:27 -0500 > Harry Wentland <harry.wentland@amd.com> 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? > The kerneldoc for "Colorspace" says it has no other effect. > > 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-861-H which it does not > even do yet. > > Perhaps a table, where for each enum drm_colorspace entry has a row > explaining the expectations of the sink: > - primaries and white point > - transfer characteristic > - YCbCr-RGB or similar conversion to/from some RGB > > Each cell can be a reference to a spec like BT.709 or BT.601 (525 line). > > I think this belongs in the kernel doc more than in color-and-hdr. > > CTA-861-H does not give all the information but refers to things like > xvYCC601 which you then need to figure out from Wikipedia or whatever > which is annoying and raises questions about its correctness. Would be > better if someone who actually has access to the authoritative specs > would review the table. > Agreed. This was based on previous work for Colorspace support on a drm_connector and I already went deeper than I hope I had to go, but it looks I didn't go far enough. :) Documenting this should also address your comments on Patch 14. Harry > > Thanks, > pq > > >> Signed-off-by: Harry Wentland <harry.wentland@amd.com> >> Cc: Pekka Paalanen <ppaalanen@gmail.com> >> Cc: Sebastian Wick <sebastian.wick@redhat.com> >> Cc: Vitaly.Prosyak@amd.com >> Cc: Uma Shankar <uma.shankar@intel.com> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> Cc: Joshua Ashton <joshua@froggi.es> >> Cc: dri-devel@lists.freedesktop.org >> Cc: amd-gfx@lists.freedesktop.org >> --- >> 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); >> >
On Tue, 13 Dec 2022 11:32:01 -0500 Harry Wentland <harry.wentland@amd.com> wrote: > On 12/13/22 05:23, Pekka Paalanen wrote: > > On Mon, 12 Dec 2022 13:21:27 -0500 > > Harry Wentland <harry.wentland@amd.com> 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. What's MSA? I don't see it in https://www.kernel.org/doc/html/latest/gpu/amdgpu/display/dc-glossary.html or anywhere under Documentation/gpu or in CTA-861-H. > 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. That would be nice, thanks! > Will it be a problem if the list of supported colorspaces differs between > drivers? I do not think so. It's just normal KMS UAPI that one must always inspect an enumeration to see what values are possible. Userspace cannot use a header with pre-defined numerical values, they always need to be introspected first like everything else about KMS properties. I know there were some opinions about hard-coding enum numerical values in headers, but I think in the end everyone agreed to the introspection even if it didn't seem useful at the time. Besides, if a driver never supported a given value but misbehaved or refused, I don't think that counts as a kernel regression? Thanks, pq > > 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@amd.com> > >> Cc: Pekka Paalanen <ppaalanen@gmail.com> > >> Cc: Sebastian Wick <sebastian.wick@redhat.com> > >> Cc: Vitaly.Prosyak@amd.com > >> Cc: Uma Shankar <uma.shankar@intel.com> > >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> Cc: Joshua Ashton <joshua@froggi.es> > >> Cc: dri-devel@lists.freedesktop.org > >> Cc: amd-gfx@lists.freedesktop.org > >> --- > >> 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(-) > >>
On 12/14/22 03:55, Pekka Paalanen wrote: > On Tue, 13 Dec 2022 11:32:01 -0500 > Harry Wentland <harry.wentland@amd.com> wrote: > >> On 12/13/22 05:23, Pekka Paalanen wrote: >>> On Mon, 12 Dec 2022 13:21:27 -0500 >>> Harry Wentland <harry.wentland@amd.com> 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. > > What's MSA? This is from the DP spec. It stands for Main Stream Attribute. It's a packet similar to an SDP that is encoded in the DP main signal stream (as opposed to an AUX message). > > I don't see it in > https://www.kernel.org/doc/html/latest/gpu/amdgpu/display/dc-glossary.html > or anywhere under Documentation/gpu or in CTA-861-H. > >> 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. > > That would be nice, thanks! > >> Will it be a problem if the list of supported colorspaces differs between >> drivers? > > I do not think so. It's just normal KMS UAPI that one must always > inspect an enumeration to see what values are possible. Userspace > cannot use a header with pre-defined numerical values, they always need > to be introspected first like everything else about KMS properties. > > I know there were some opinions about hard-coding enum numerical values > in headers, but I think in the end everyone agreed to the introspection > even if it didn't seem useful at the time. > > Besides, if a driver never supported a given value but misbehaved or > refused, I don't think that counts as a kernel regression? > True, but I would imagine that can be confusing for developers of DRM clients when enabling support for a new feature. I would guess it's much less confusing if drivers only exposed functionality that are expected to work (and are hopefully tested regularly). Harry > > Thanks, > pq > >> >> 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@amd.com> >>>> Cc: Pekka Paalanen <ppaalanen@gmail.com> >>>> Cc: Sebastian Wick <sebastian.wick@redhat.com> >>>> Cc: Vitaly.Prosyak@amd.com >>>> Cc: Uma Shankar <uma.shankar@intel.com> >>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >>>> Cc: Joshua Ashton <joshua@froggi.es> >>>> Cc: dri-devel@lists.freedesktop.org >>>> Cc: amd-gfx@lists.freedesktop.org >>>> --- >>>> 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(-) >>>>
On Wed, Dec 14, 2022 at 3:56 AM Pekka Paalanen <ppaalanen@gmail.com> wrote: > > On Tue, 13 Dec 2022 11:32:01 -0500 > Harry Wentland <harry.wentland@amd.com> wrote: > > > On 12/13/22 05:23, Pekka Paalanen wrote: > > > On Mon, 12 Dec 2022 13:21:27 -0500 > > > Harry Wentland <harry.wentland@amd.com> 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. > > What's MSA? I think it's Main Stream Attribute data. Part of DP. See slide 31 of this document: https://www.vesa.org/wp-content/uploads/2011/01/ICCE-Presentation-on-VESA-DisplayPort.pdf Alex > > I don't see it in > https://www.kernel.org/doc/html/latest/gpu/amdgpu/display/dc-glossary.html > or anywhere under Documentation/gpu or in CTA-861-H. > > > 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. > > That would be nice, thanks! > > > Will it be a problem if the list of supported colorspaces differs between > > drivers? > > I do not think so. It's just normal KMS UAPI that one must always > inspect an enumeration to see what values are possible. Userspace > cannot use a header with pre-defined numerical values, they always need > to be introspected first like everything else about KMS properties. > > I know there were some opinions about hard-coding enum numerical values > in headers, but I think in the end everyone agreed to the introspection > even if it didn't seem useful at the time. > > Besides, if a driver never supported a given value but misbehaved or > refused, I don't think that counts as a kernel regression? > > > Thanks, > pq > > > > > 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@amd.com> > > >> Cc: Pekka Paalanen <ppaalanen@gmail.com> > > >> Cc: Sebastian Wick <sebastian.wick@redhat.com> > > >> Cc: Vitaly.Prosyak@amd.com > > >> Cc: Uma Shankar <uma.shankar@intel.com> > > >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > >> Cc: Joshua Ashton <joshua@froggi.es> > > >> Cc: dri-devel@lists.freedesktop.org > > >> Cc: amd-gfx@lists.freedesktop.org > > >> --- > > >> 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);
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. Signed-off-by: Harry Wentland <harry.wentland@amd.com> Cc: Pekka Paalanen <ppaalanen@gmail.com> Cc: Sebastian Wick <sebastian.wick@redhat.com> Cc: Vitaly.Prosyak@amd.com Cc: Uma Shankar <uma.shankar@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Joshua Ashton <joshua@froggi.es> Cc: dri-devel@lists.freedesktop.org Cc: amd-gfx@lists.freedesktop.org --- 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(-)