[v4,3/7] drm: Add DisplayPort colorspace property

Commit Message

Gwan-gyeong Mun Sept. 3, 2019, 9:12 a.m. UTC

In order to use colorspace property to Display Port connectors, it extends
DRM_MODE_CONNECTOR_DisplayPort connector_type on
drm_mode_create_colorspace_property function.

v3: Addressed review comments from Ville
    - Add new colorimetry options for DP 1.4a spec.
    - Separate set of colorimetry enum values for DP.
v4: Add additional comments to struct drm_prop_enum_list.
    Polishing an enum string of struct drm_prop_enum_list
Signed-off-by: Gwan-gyeong Mun <[e-mail]>
Reviewed-by: Uma Shankar <[e-mail]>
---
 drivers/gpu/drm/drm_connector.c | 46 +++++++++++++++++++++++++++++++++
 include/drm/drm_connector.h     |  8 ++++++
 2 files changed, 54 insertions(+)

Comments

Ilia Mirkin Sept. 3, 2019, 12:42 p.m. UTC | #1

So how would this work with a DP++ connector? Should it list the HDMI
or DP properties? Or do we need a custom property checker which is
aware of what is currently plugged in to validate the values?

On Tue, Sep 3, 2019 at 5:12 AM Gwan-gyeong Mun
<[e-mail]> wrote:
>
> In order to use colorspace property to Display Port connectors, it extends
> DRM_MODE_CONNECTOR_DisplayPort connector_type on
> drm_mode_create_colorspace_property function.
>
> v3: Addressed review comments from Ville
>     - Add new colorimetry options for DP 1.4a spec.
>     - Separate set of colorimetry enum values for DP.
> v4: Add additional comments to struct drm_prop_enum_list.
>     Polishing an enum string of struct drm_prop_enum_list
> Signed-off-by: Gwan-gyeong Mun <[e-mail]>
> Reviewed-by: Uma Shankar <[e-mail]>
> ---
>  drivers/gpu/drm/drm_connector.c | 46 +++++++++++++++++++++++++++++++++
>  include/drm/drm_connector.h     |  8 ++++++
>  2 files changed, 54 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 4c766624b20d..5834e6d330a0 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -882,6 +882,44 @@ static const struct drm_prop_enum_list hdmi_colorspaces[] = {
>         { DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER, "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" },
> +       /* Colorimetry based on IEC 61966-2-1 */
> +       { DRM_MODE_COLORIMETRY_SRGB, "sRGB" },
> +       { DRM_MODE_COLORIMETRY_WIDE_GAMUT_FIXED_POINT_RGB, "wide_gamut_fixed_point_RGB" },
> +       /* Colorimetry based on IEC 61966-2-2, wide gamut floating point RGB */
> +       { DRM_MODE_COLORIMETRY_SCRGB, "scRGB" },
> +       { DRM_MODE_COLORIMETRY_ADOBE_RGB, "Adobe_RGB" },
> +       /* Colorimetry based on SMPTE RP 431-2 */
> +       { DRM_MODE_COLORIMETRY_DCP_P3_RGB, "DCI-P3_RGB" },
> +       /* 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" },
> +       /*
> +        * Colorumetry based on Digital Imaging and Communications in Medicine
> +        * (DICOM) Part 14: Grayscale Standard Display Function
> +        */
> +       { DRM_MODE_COLORIMETRY_DICOM_PART_14_GRAYSCALE, "DICOM_Part_14_Grayscale" },
> +};
> +
>  /**
>   * DOC: standard connector properties
>   *
> @@ -1710,6 +1748,14 @@ int drm_mode_create_colorspace_property(struct drm_connector *connector)
>                                                 ARRAY_SIZE(hdmi_colorspaces));
>                 if (!prop)
>                         return -ENOMEM;
> +       } else if (connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort ||
> +                  connector->connector_type == DRM_MODE_CONNECTOR_eDP) {
> +               prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM,
> +                                               "Colorspace",
> +                                               dp_colorspaces,
> +                                               ARRAY_SIZE(dp_colorspaces));
> +               if (!prop)
> +                       return -ENOMEM;
>         } else {
>                 DRM_DEBUG_KMS("Colorspace property not supported\n");
>                 return 0;
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 681cb590f952..8848e5d6b0c4 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -281,6 +281,14 @@ enum drm_panel_orientation {
>  /* Additional Colorimetry extension added as part of CTA 861.G */
>  #define DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65            11
>  #define DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER                12
> +/* Additional Colorimetry Options added for DP 1.4a VSC Colorimetry Format */
> +#define DRM_MODE_COLORIMETRY_SRGB                      13
> +#define DRM_MODE_COLORIMETRY_WIDE_GAMUT_FIXED_POINT_RGB        14
> +#define DRM_MODE_COLORIMETRY_SCRGB                     15
> +#define DRM_MODE_COLORIMETRY_ADOBE_RGB                 16
> +#define DRM_MODE_COLORIMETRY_DCP_P3_RGB                        17
> +#define DRM_MODE_COLORIMETRY_BT601_YCC                 18
> +#define DRM_MODE_COLORIMETRY_DICOM_PART_14_GRAYSCALE   19
>
>  /**
>   * enum drm_bus_flags - bus_flags info for &drm_display_info
> --
> 2.23.0
>
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Uma Shankar Sept. 3, 2019, 2:11 p.m. UTC | #2

>-----Original Message-----
>From: dri-devel <[e-mail]> On Behalf Of Gwan-
>gyeong Mun
>Sent: Tuesday, September 3, 2019 2:43 PM
>To: [e-mail]
>Cc: Shankar, Uma <[e-mail]>; [e-mail]
>Subject: [PATCH v4 3/7] drm: Add DisplayPort colorspace property
>
>In order to use colorspace property to Display Port connectors, it extends
>DRM_MODE_CONNECTOR_DisplayPort connector_type on
>drm_mode_create_colorspace_property function.
>
>v3: Addressed review comments from Ville
>    - Add new colorimetry options for DP 1.4a spec.
>    - Separate set of colorimetry enum values for DP.

Thanks Ville for spotting this and Gwan-gyeong Mun for fixing it.
Somehow missed this in my first scan.

>v4: Add additional comments to struct drm_prop_enum_list.
>    Polishing an enum string of struct drm_prop_enum_list
>Signed-off-by: Gwan-gyeong Mun <[e-mail]>
>Reviewed-by: Uma Shankar <[e-mail]>
>---
> drivers/gpu/drm/drm_connector.c | 46 +++++++++++++++++++++++++++++++++
> include/drm/drm_connector.h     |  8 ++++++
> 2 files changed, 54 insertions(+)
>
>diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>index 4c766624b20d..5834e6d330a0 100644
>--- a/drivers/gpu/drm/drm_connector.c
>+++ b/drivers/gpu/drm/drm_connector.c
>@@ -882,6 +882,44 @@ static const struct drm_prop_enum_list hdmi_colorspaces[]
>= {
> 	{ DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER, "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" },
>+	/* Colorimetry based on IEC 61966-2-1 */
>+	{ DRM_MODE_COLORIMETRY_SRGB, "sRGB" },
>+	{ DRM_MODE_COLORIMETRY_WIDE_GAMUT_FIXED_POINT_RGB,
>"wide_gamut_fixed_point_RGB" },
>+	/* Colorimetry based on IEC 61966-2-2, wide gamut floating point RGB */
>+	{ DRM_MODE_COLORIMETRY_SCRGB, "scRGB" },
>+	{ DRM_MODE_COLORIMETRY_ADOBE_RGB, "Adobe_RGB" },
>+	/* Colorimetry based on SMPTE RP 431-2 */
>+	{ DRM_MODE_COLORIMETRY_DCP_P3_RGB, "DCI-P3_RGB" },
>+	/* 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" },
>+	/*
>+	 * Colorumetry based on Digital Imaging and Communications in Medicine

Typo here.

>+	 * (DICOM) Part 14: Grayscale Standard Display Function
>+	 */
>+	{ DRM_MODE_COLORIMETRY_DICOM_PART_14_GRAYSCALE,
>+"DICOM_Part_14_Grayscale" }, };
>+
> /**
>  * DOC: standard connector properties
>  *
>@@ -1710,6 +1748,14 @@ int drm_mode_create_colorspace_property(struct
>drm_connector *connector)
> 						ARRAY_SIZE(hdmi_colorspaces));
> 		if (!prop)
> 			return -ENOMEM;
>+	} else if (connector->connector_type ==
>DRM_MODE_CONNECTOR_DisplayPort ||
>+		   connector->connector_type == DRM_MODE_CONNECTOR_eDP) {
>+		prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM,
>+						"Colorspace",
>+						dp_colorspaces,
>+						ARRAY_SIZE(dp_colorspaces));
>+		if (!prop)
>+			return -ENOMEM;
> 	} else {
> 		DRM_DEBUG_KMS("Colorspace property not supported\n");
> 		return 0;
>diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index
>681cb590f952..8848e5d6b0c4 100644
>--- a/include/drm/drm_connector.h
>+++ b/include/drm/drm_connector.h
>@@ -281,6 +281,14 @@ enum drm_panel_orientation {
> /* Additional Colorimetry extension added as part of CTA 861.G */
> #define DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65		11
> #define DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER		12
>+/* Additional Colorimetry Options added for DP 1.4a VSC Colorimetry Format */
>+#define DRM_MODE_COLORIMETRY_SRGB			13
>+#define DRM_MODE_COLORIMETRY_WIDE_GAMUT_FIXED_POINT_RGB	14
>+#define DRM_MODE_COLORIMETRY_SCRGB			15
>+#define DRM_MODE_COLORIMETRY_ADOBE_RGB			16

I feel we can re-use the OPRGB version of HDMI to define the ADOBE_RGB.  My
understanding is they should be the same. Someone please correct me if that’s not
the case.

>+#define DRM_MODE_COLORIMETRY_DCP_P3_RGB			17

Same for DCI_P3 we can re-use the DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65.

>+#define DRM_MODE_COLORIMETRY_BT601_YCC			18
>+#define DRM_MODE_COLORIMETRY_DICOM_PART_14_GRAYSCALE	19
>
> /**
>  * enum drm_bus_flags - bus_flags info for &drm_display_info
>--
>2.23.0
>
>_______________________________________________
>dri-devel mailing list
>[e-mail]
>https://lists.freedesktop.org/mailman/listinfo/dri-devel

Uma Shankar Sept. 6, 2019, 11:31 a.m. UTC | #3

>-----Original Message-----
>From: Ilia Mirkin <[e-mail]>
>Sent: Tuesday, September 3, 2019 6:12 PM
>To: Mun, Gwan-gyeong <[e-mail]>
>Cc: Intel Graphics Development <[e-mail]>; Shankar, Uma
><[e-mail]>; dri-devel <[e-mail]>
>Subject: Re: [PATCH v4 3/7] drm: Add DisplayPort colorspace property
>
>So how would this work with a DP++ connector? Should it list the HDMI or DP
>properties? Or do we need a custom property checker which is aware of what is
>currently plugged in to validate the values?

AFAIU For DP++ cases, we detect what kind of sink its driving DP or HDMI (with a passive dongle).
Based on the type of sink detected, we should expose DP or HDMI colorspaces to userspace.

>On Tue, Sep 3, 2019 at 5:12 AM Gwan-gyeong Mun <[e-mail]>
>wrote:
>>
>> In order to use colorspace property to Display Port connectors, it
>> extends DRM_MODE_CONNECTOR_DisplayPort connector_type on
>> drm_mode_create_colorspace_property function.
>>
>> v3: Addressed review comments from Ville
>>     - Add new colorimetry options for DP 1.4a spec.
>>     - Separate set of colorimetry enum values for DP.
>> v4: Add additional comments to struct drm_prop_enum_list.
>>     Polishing an enum string of struct drm_prop_enum_list
>> Signed-off-by: Gwan-gyeong Mun <[e-mail]>
>> Reviewed-by: Uma Shankar <[e-mail]>
>> ---
>>  drivers/gpu/drm/drm_connector.c | 46 +++++++++++++++++++++++++++++++++
>>  include/drm/drm_connector.h     |  8 ++++++
>>  2 files changed, 54 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_connector.c
>> b/drivers/gpu/drm/drm_connector.c index 4c766624b20d..5834e6d330a0
>> 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -882,6 +882,44 @@ static const struct drm_prop_enum_list
>hdmi_colorspaces[] = {
>>         { DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER,
>> "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" },
>> +       /* Colorimetry based on IEC 61966-2-1 */
>> +       { DRM_MODE_COLORIMETRY_SRGB, "sRGB" },
>> +       { DRM_MODE_COLORIMETRY_WIDE_GAMUT_FIXED_POINT_RGB,
>"wide_gamut_fixed_point_RGB" },
>> +       /* Colorimetry based on IEC 61966-2-2, wide gamut floating point RGB */
>> +       { DRM_MODE_COLORIMETRY_SCRGB, "scRGB" },
>> +       { DRM_MODE_COLORIMETRY_ADOBE_RGB, "Adobe_RGB" },
>> +       /* Colorimetry based on SMPTE RP 431-2 */
>> +       { DRM_MODE_COLORIMETRY_DCP_P3_RGB, "DCI-P3_RGB" },
>> +       /* 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" },
>> +       /*
>> +        * Colorumetry based on Digital Imaging and Communications in Medicine
>> +        * (DICOM) Part 14: Grayscale Standard Display Function
>> +        */
>> +       { DRM_MODE_COLORIMETRY_DICOM_PART_14_GRAYSCALE,
>> +"DICOM_Part_14_Grayscale" }, };
>> +
>>  /**
>>   * DOC: standard connector properties
>>   *
>> @@ -1710,6 +1748,14 @@ int drm_mode_create_colorspace_property(struct
>drm_connector *connector)
>>                                                 ARRAY_SIZE(hdmi_colorspaces));
>>                 if (!prop)
>>                         return -ENOMEM;
>> +       } else if (connector->connector_type ==
>DRM_MODE_CONNECTOR_DisplayPort ||
>> +                  connector->connector_type == DRM_MODE_CONNECTOR_eDP) {
>> +               prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM,
>> +                                               "Colorspace",
>> +                                               dp_colorspaces,
>> +                                               ARRAY_SIZE(dp_colorspaces));
>> +               if (!prop)
>> +                       return -ENOMEM;
>>         } else {
>>                 DRM_DEBUG_KMS("Colorspace property not supported\n");
>>                 return 0;
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index 681cb590f952..8848e5d6b0c4 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -281,6 +281,14 @@ enum drm_panel_orientation {
>>  /* Additional Colorimetry extension added as part of CTA 861.G */
>>  #define DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65            11
>>  #define DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER                12
>> +/* Additional Colorimetry Options added for DP 1.4a VSC Colorimetry Format */
>> +#define DRM_MODE_COLORIMETRY_SRGB                      13
>> +#define DRM_MODE_COLORIMETRY_WIDE_GAMUT_FIXED_POINT_RGB        14
>> +#define DRM_MODE_COLORIMETRY_SCRGB                     15
>> +#define DRM_MODE_COLORIMETRY_ADOBE_RGB                 16
>> +#define DRM_MODE_COLORIMETRY_DCP_P3_RGB                        17
>> +#define DRM_MODE_COLORIMETRY_BT601_YCC                 18
>> +#define DRM_MODE_COLORIMETRY_DICOM_PART_14_GRAYSCALE   19
>>
>>  /**
>>   * enum drm_bus_flags - bus_flags info for &drm_display_info
>> --
>> 2.23.0
>>
>> _______________________________________________
>> dri-devel mailing list
>> [e-mail]
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Ville Syrjälä Sept. 6, 2019, 11:42 a.m. UTC | #4

On Fri, Sep 06, 2019 at 11:31:55AM +0000, Shankar, Uma wrote:
> 
> 
> >-----Original Message-----
> >From: Ilia Mirkin <[e-mail]>
> >Sent: Tuesday, September 3, 2019 6:12 PM
> >To: Mun, Gwan-gyeong <[e-mail]>
> >Cc: Intel Graphics Development <[e-mail]>; Shankar, Uma
> ><[e-mail]>; dri-devel <[e-mail]>
> >Subject: Re: [PATCH v4 3/7] drm: Add DisplayPort colorspace property
> >
> >So how would this work with a DP++ connector? Should it list the HDMI or DP
> >properties? Or do we need a custom property checker which is aware of what is
> >currently plugged in to validate the values?
> 
> AFAIU For DP++ cases, we detect what kind of sink its driving DP or HDMI (with a passive dongle).
> Based on the type of sink detected, we should expose DP or HDMI colorspaces to userspace.

For i915 DP connector always drives DP mode, HDMI connector always drives
HDMI mode, even when the physical connector is DP++.

> 
> >On Tue, Sep 3, 2019 at 5:12 AM Gwan-gyeong Mun <[e-mail]>
> >wrote:
> >>
> >> In order to use colorspace property to Display Port connectors, it
> >> extends DRM_MODE_CONNECTOR_DisplayPort connector_type on
> >> drm_mode_create_colorspace_property function.
> >>
> >> v3: Addressed review comments from Ville
> >>     - Add new colorimetry options for DP 1.4a spec.
> >>     - Separate set of colorimetry enum values for DP.
> >> v4: Add additional comments to struct drm_prop_enum_list.
> >>     Polishing an enum string of struct drm_prop_enum_list
> >> Signed-off-by: Gwan-gyeong Mun <[e-mail]>
> >> Reviewed-by: Uma Shankar <[e-mail]>
> >> ---
> >>  drivers/gpu/drm/drm_connector.c | 46 +++++++++++++++++++++++++++++++++
> >>  include/drm/drm_connector.h     |  8 ++++++
> >>  2 files changed, 54 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_connector.c
> >> b/drivers/gpu/drm/drm_connector.c index 4c766624b20d..5834e6d330a0
> >> 100644
> >> --- a/drivers/gpu/drm/drm_connector.c
> >> +++ b/drivers/gpu/drm/drm_connector.c
> >> @@ -882,6 +882,44 @@ static const struct drm_prop_enum_list
> >hdmi_colorspaces[] = {
> >>         { DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER,
> >> "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" },
> >> +       /* Colorimetry based on IEC 61966-2-1 */
> >> +       { DRM_MODE_COLORIMETRY_SRGB, "sRGB" },
> >> +       { DRM_MODE_COLORIMETRY_WIDE_GAMUT_FIXED_POINT_RGB,
> >"wide_gamut_fixed_point_RGB" },
> >> +       /* Colorimetry based on IEC 61966-2-2, wide gamut floating point RGB */
> >> +       { DRM_MODE_COLORIMETRY_SCRGB, "scRGB" },
> >> +       { DRM_MODE_COLORIMETRY_ADOBE_RGB, "Adobe_RGB" },
> >> +       /* Colorimetry based on SMPTE RP 431-2 */
> >> +       { DRM_MODE_COLORIMETRY_DCP_P3_RGB, "DCI-P3_RGB" },
> >> +       /* 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" },
> >> +       /*
> >> +        * Colorumetry based on Digital Imaging and Communications in Medicine
> >> +        * (DICOM) Part 14: Grayscale Standard Display Function
> >> +        */
> >> +       { DRM_MODE_COLORIMETRY_DICOM_PART_14_GRAYSCALE,
> >> +"DICOM_Part_14_Grayscale" }, };
> >> +
> >>  /**
> >>   * DOC: standard connector properties
> >>   *
> >> @@ -1710,6 +1748,14 @@ int drm_mode_create_colorspace_property(struct
> >drm_connector *connector)
> >>                                                 ARRAY_SIZE(hdmi_colorspaces));
> >>                 if (!prop)
> >>                         return -ENOMEM;
> >> +       } else if (connector->connector_type ==
> >DRM_MODE_CONNECTOR_DisplayPort ||
> >> +                  connector->connector_type == DRM_MODE_CONNECTOR_eDP) {
> >> +               prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM,
> >> +                                               "Colorspace",
> >> +                                               dp_colorspaces,
> >> +                                               ARRAY_SIZE(dp_colorspaces));
> >> +               if (!prop)
> >> +                       return -ENOMEM;
> >>         } else {
> >>                 DRM_DEBUG_KMS("Colorspace property not supported\n");
> >>                 return 0;
> >> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> >> index 681cb590f952..8848e5d6b0c4 100644
> >> --- a/include/drm/drm_connector.h
> >> +++ b/include/drm/drm_connector.h
> >> @@ -281,6 +281,14 @@ enum drm_panel_orientation {
> >>  /* Additional Colorimetry extension added as part of CTA 861.G */
> >>  #define DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65            11
> >>  #define DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER                12
> >> +/* Additional Colorimetry Options added for DP 1.4a VSC Colorimetry Format */
> >> +#define DRM_MODE_COLORIMETRY_SRGB                      13
> >> +#define DRM_MODE_COLORIMETRY_WIDE_GAMUT_FIXED_POINT_RGB        14
> >> +#define DRM_MODE_COLORIMETRY_SCRGB                     15
> >> +#define DRM_MODE_COLORIMETRY_ADOBE_RGB                 16
> >> +#define DRM_MODE_COLORIMETRY_DCP_P3_RGB                        17
> >> +#define DRM_MODE_COLORIMETRY_BT601_YCC                 18
> >> +#define DRM_MODE_COLORIMETRY_DICOM_PART_14_GRAYSCALE   19
> >>
> >>  /**
> >>   * enum drm_bus_flags - bus_flags info for &drm_display_info
> >> --
> >> 2.23.0
> >>
> >> _______________________________________________
> >> dri-devel mailing list
> >> [e-mail]
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Ilia Mirkin Sept. 6, 2019, 1:24 p.m. UTC | #5

On Fri, Sep 6, 2019 at 7:43 AM Ville Syrjälä
<[e-mail]> wrote:
>
> On Fri, Sep 06, 2019 at 11:31:55AM +0000, Shankar, Uma wrote:
> >
> >
> > >-----Original Message-----
> > >From: Ilia Mirkin <[e-mail]>
> > >Sent: Tuesday, September 3, 2019 6:12 PM
> > >To: Mun, Gwan-gyeong <[e-mail]>
> > >Cc: Intel Graphics Development <[e-mail]>; Shankar, Uma
> > ><[e-mail]>; dri-devel <[e-mail]>
> > >Subject: Re: [PATCH v4 3/7] drm: Add DisplayPort colorspace property
> > >
> > >So how would this work with a DP++ connector? Should it list the HDMI or DP
> > >properties? Or do we need a custom property checker which is aware of what is
> > >currently plugged in to validate the values?
> >
> > AFAIU For DP++ cases, we detect what kind of sink its driving DP or HDMI (with a passive dongle).
> > Based on the type of sink detected, we should expose DP or HDMI colorspaces to userspace.
>
> For i915 DP connector always drives DP mode, HDMI connector always drives
> HDMI mode, even when the physical connector is DP++.

Right, i915 creates 2 connectors, while nouveau, radeon, and amdgpu
create 1 connector (not sure about other drivers) for a single
physical DP++ socket. Since we supply the list of valid values at the
time of creating the connector, we can't know at that point whether in
the future a HDMI or DP will be plugged into it.

  -ilia

Gwan-gyeong Mun Sept. 7, 2019, 11:19 p.m. UTC | #6

On Fri, 2019-09-06 at 09:24 -0400, Ilia Mirkin wrote:
> On Fri, Sep 6, 2019 at 7:43 AM Ville Syrjälä
> <[e-mail]> wrote:
> > On Fri, Sep 06, 2019 at 11:31:55AM +0000, Shankar, Uma wrote:
> > > 
> > > > -----Original Message-----
> > > > From: Ilia Mirkin <[e-mail]>
> > > > Sent: Tuesday, September 3, 2019 6:12 PM
> > > > To: Mun, Gwan-gyeong <[e-mail]>
> > > > Cc: Intel Graphics Development <[e-mail]
> > > > >; Shankar, Uma
> > > > <[e-mail]>; dri-devel <
> > > > [e-mail]>
> > > > Subject: Re: [PATCH v4 3/7] drm: Add DisplayPort colorspace
> > > > property
> > > > 
> > > > So how would this work with a DP++ connector? Should it list
> > > > the HDMI or DP
> > > > properties? Or do we need a custom property checker which is
> > > > aware of what is
> > > > currently plugged in to validate the values?
> > > 
> > > AFAIU For DP++ cases, we detect what kind of sink its driving DP
> > > or HDMI (with a passive dongle).
> > > Based on the type of sink detected, we should expose DP or HDMI
> > > colorspaces to userspace.
> > 
> > For i915 DP connector always drives DP mode, HDMI connector always
> > drives
> > HDMI mode, even when the physical connector is DP++.
> 
> Right, i915 creates 2 connectors, while nouveau, radeon, and amdgpu
> create 1 connector (not sure about other drivers) for a single
> physical DP++ socket. Since we supply the list of valid values at the
> time of creating the connector, we can't know at that point whether
> in
> the future a HDMI or DP will be plugged into it.
> 
>   -ilia
Ilia, does it mean that the drm_connector type is
DRM_MODE_CONNECTOR_DisplayPort and protocol is DP++ mode?

And Ville and Uma,  when we are useing dp active dongle (DP to HDMI
dongle and DP branch device is HDMI) should we expose HDMI colorspace?

-G.G.

Ilia Mirkin Sept. 8, 2019, 1:43 a.m. UTC | #7

On Sat, Sep 7, 2019 at 7:20 PM Mun, Gwan-gyeong
<[e-mail]> wrote:
>
> On Fri, 2019-09-06 at 09:24 -0400, Ilia Mirkin wrote:
> > On Fri, Sep 6, 2019 at 7:43 AM Ville Syrjälä
> > <[e-mail]> wrote:
> > > On Fri, Sep 06, 2019 at 11:31:55AM +0000, Shankar, Uma wrote:
> > > >
> > > > > -----Original Message-----
> > > > > From: Ilia Mirkin <[e-mail]>
> > > > > Sent: Tuesday, September 3, 2019 6:12 PM
> > > > > To: Mun, Gwan-gyeong <[e-mail]>
> > > > > Cc: Intel Graphics Development <[e-mail]
> > > > > >; Shankar, Uma
> > > > > <[e-mail]>; dri-devel <
> > > > > [e-mail]>
> > > > > Subject: Re: [PATCH v4 3/7] drm: Add DisplayPort colorspace
> > > > > property
> > > > >
> > > > > So how would this work with a DP++ connector? Should it list
> > > > > the HDMI or DP
> > > > > properties? Or do we need a custom property checker which is
> > > > > aware of what is
> > > > > currently plugged in to validate the values?
> > > >
> > > > AFAIU For DP++ cases, we detect what kind of sink its driving DP
> > > > or HDMI (with a passive dongle).
> > > > Based on the type of sink detected, we should expose DP or HDMI
> > > > colorspaces to userspace.
> > >
> > > For i915 DP connector always drives DP mode, HDMI connector always
> > > drives
> > > HDMI mode, even when the physical connector is DP++.
> >
> > Right, i915 creates 2 connectors, while nouveau, radeon, and amdgpu
> > create 1 connector (not sure about other drivers) for a single
> > physical DP++ socket. Since we supply the list of valid values at the
> > time of creating the connector, we can't know at that point whether
> > in
> > the future a HDMI or DP will be plugged into it.
> >
> >   -ilia
> Ilia, does it mean that the drm_connector type is
> DRM_MODE_CONNECTOR_DisplayPort and protocol is DP++ mode?

That is correct. The connector type is "DisplayPort" in such a case.

Cheers,

  -ilia

Ville Syrjälä Sept. 9, 2019, 10:25 a.m. UTC | #8

On Sat, Sep 07, 2019 at 11:19:55PM +0000, Mun, Gwan-gyeong wrote:
> On Fri, 2019-09-06 at 09:24 -0400, Ilia Mirkin wrote:
> > On Fri, Sep 6, 2019 at 7:43 AM Ville Syrjälä
> > <[e-mail]> wrote:
> > > On Fri, Sep 06, 2019 at 11:31:55AM +0000, Shankar, Uma wrote:
> > > > 
> > > > > -----Original Message-----
> > > > > From: Ilia Mirkin <[e-mail]>
> > > > > Sent: Tuesday, September 3, 2019 6:12 PM
> > > > > To: Mun, Gwan-gyeong <[e-mail]>
> > > > > Cc: Intel Graphics Development <[e-mail]
> > > > > >; Shankar, Uma
> > > > > <[e-mail]>; dri-devel <
> > > > > [e-mail]>
> > > > > Subject: Re: [PATCH v4 3/7] drm: Add DisplayPort colorspace
> > > > > property
> > > > > 
> > > > > So how would this work with a DP++ connector? Should it list
> > > > > the HDMI or DP
> > > > > properties? Or do we need a custom property checker which is
> > > > > aware of what is
> > > > > currently plugged in to validate the values?
> > > > 
> > > > AFAIU For DP++ cases, we detect what kind of sink its driving DP
> > > > or HDMI (with a passive dongle).
> > > > Based on the type of sink detected, we should expose DP or HDMI
> > > > colorspaces to userspace.
> > > 
> > > For i915 DP connector always drives DP mode, HDMI connector always
> > > drives
> > > HDMI mode, even when the physical connector is DP++.
> > 
> > Right, i915 creates 2 connectors, while nouveau, radeon, and amdgpu
> > create 1 connector (not sure about other drivers) for a single
> > physical DP++ socket. Since we supply the list of valid values at the
> > time of creating the connector, we can't know at that point whether
> > in
> > the future a HDMI or DP will be plugged into it.
> > 
> >   -ilia
> Ilia, does it mean that the drm_connector type is
> DRM_MODE_CONNECTOR_DisplayPort and protocol is DP++ mode?
> 
> And Ville and Uma,  when we are useing dp active dongle (DP to HDMI
> dongle and DP branch device is HDMI) should we expose HDMI colorspace?

We still set it up via DP MSA/VSC no? In that case it should follow the
DP spec I think. LSPCON is probably different because we manually generate
the AVI infoframe for it. But I'm not sure how we're going to reconcile
that with the DP stuff we also set up for it.

Gwan-gyeong Mun Sept. 10, 2019, 7:21 a.m. UTC | #9

On Mon, 2019-09-09 at 13:25 +0300, Ville Syrjälä wrote:
> On Sat, Sep 07, 2019 at 11:19:55PM +0000, Mun, Gwan-gyeong wrote:
> > On Fri, 2019-09-06 at 09:24 -0400, Ilia Mirkin wrote:
> > > On Fri, Sep 6, 2019 at 7:43 AM Ville Syrjälä
> > > <[e-mail]> wrote:
> > > > On Fri, Sep 06, 2019 at 11:31:55AM +0000, Shankar, Uma wrote:
> > > > > > -----Original Message-----
> > > > > > From: Ilia Mirkin <[e-mail]>
> > > > > > Sent: Tuesday, September 3, 2019 6:12 PM
> > > > > > To: Mun, Gwan-gyeong <[e-mail]>
> > > > > > Cc: Intel Graphics Development <
> > > > > > [e-mail]
> > > > > > > ; Shankar, Uma
> > > > > > <[e-mail]>; dri-devel <
> > > > > > [e-mail]>
> > > > > > Subject: Re: [PATCH v4 3/7] drm: Add DisplayPort colorspace
> > > > > > property
> > > > > > 
> > > > > > So how would this work with a DP++ connector? Should it
> > > > > > list
> > > > > > the HDMI or DP
> > > > > > properties? Or do we need a custom property checker which
> > > > > > is
> > > > > > aware of what is
> > > > > > currently plugged in to validate the values?
> > > > > 
> > > > > AFAIU For DP++ cases, we detect what kind of sink its driving
> > > > > DP
> > > > > or HDMI (with a passive dongle).
> > > > > Based on the type of sink detected, we should expose DP or
> > > > > HDMI
> > > > > colorspaces to userspace.
> > > > 
> > > > For i915 DP connector always drives DP mode, HDMI connector
> > > > always
> > > > drives
> > > > HDMI mode, even when the physical connector is DP++.
> > > 
> > > Right, i915 creates 2 connectors, while nouveau, radeon, and
> > > amdgpu
> > > create 1 connector (not sure about other drivers) for a single
> > > physical DP++ socket. Since we supply the list of valid values at
> > > the
> > > time of creating the connector, we can't know at that point
> > > whether
> > > in
> > > the future a HDMI or DP will be plugged into it.
> > > 
> > >   -ilia
> > Ilia, does it mean that the drm_connector type is
> > DRM_MODE_CONNECTOR_DisplayPort and protocol is DP++ mode?
> > 
> > And Ville and Uma,  when we are useing dp active dongle (DP to HDMI
> > dongle and DP branch device is HDMI) should we expose HDMI
> > colorspace?
> 
> We still set it up via DP MSA/VSC no? In that case it should follow
> the
> DP spec I think. LSPCON is probably different because we manually 
Yes, I agree too. 

- G.G.
> generate
> the AVI infoframe for it. But I'm not sure how we're going to
> reconcile
> that with the DP stuff we also set up for it.
>

Gwan-gyeong Mun Sept. 10, 2019, 7:34 a.m. UTC | #10

On Sat, 2019-09-07 at 21:43 -0400, Ilia Mirkin wrote:
> On Sat, Sep 7, 2019 at 7:20 PM Mun, Gwan-gyeong
> <[e-mail]> wrote:
> > On Fri, 2019-09-06 at 09:24 -0400, Ilia Mirkin wrote:
> > > On Fri, Sep 6, 2019 at 7:43 AM Ville Syrjälä
> > > <[e-mail]> wrote:
> > > > On Fri, Sep 06, 2019 at 11:31:55AM +0000, Shankar, Uma wrote:
> > > > > > -----Original Message-----
> > > > > > From: Ilia Mirkin <[e-mail]>
> > > > > > Sent: Tuesday, September 3, 2019 6:12 PM
> > > > > > To: Mun, Gwan-gyeong <[e-mail]>
> > > > > > Cc: Intel Graphics Development <
> > > > > > [e-mail]
> > > > > > > ; Shankar, Uma
> > > > > > <[e-mail]>; dri-devel <
> > > > > > [e-mail]>
> > > > > > Subject: Re: [PATCH v4 3/7] drm: Add DisplayPort colorspace
> > > > > > property
> > > > > > 
> > > > > > So how would this work with a DP++ connector? Should it
> > > > > > list
> > > > > > the HDMI or DP
> > > > > > properties? Or do we need a custom property checker which
> > > > > > is
> > > > > > aware of what is
> > > > > > currently plugged in to validate the values?
> > > > > 
> > > > > AFAIU For DP++ cases, we detect what kind of sink its driving
> > > > > DP
> > > > > or HDMI (with a passive dongle).
> > > > > Based on the type of sink detected, we should expose DP or
> > > > > HDMI
> > > > > colorspaces to userspace.
> > > > 
> > > > For i915 DP connector always drives DP mode, HDMI connector
> > > > always
> > > > drives
> > > > HDMI mode, even when the physical connector is DP++.
> > > 
> > > Right, i915 creates 2 connectors, while nouveau, radeon, and
> > > amdgpu
> > > create 1 connector (not sure about other drivers) for a single
> > > physical DP++ socket. Since we supply the list of valid values at
> > > the
> > > time of creating the connector, we can't know at that point
> > > whether
> > > in
> > > the future a HDMI or DP will be plugged into it.
> > > 
> > >   -ilia
> > Ilia, does it mean that the drm_connector type is
> > DRM_MODE_CONNECTOR_DisplayPort and protocol is DP++ mode?
> 
> That is correct. The connector type is "DisplayPort" in such a case.
> 
> Cheers,
> 
>   -ilia

For now drm_mode_create_colorspace_property() is only used for i915.
IMHO, when other drivers ( nouveau, radeon, and amdgpu ) are ready for
using of drm_mode_create_colorspace_property(),
what about do we add a variable which can identify DP++ and DP to
drm_connector?
And when the drivers (nouveau, radeon, and amdgpu) detect the current
protocol, the drivers will set the variable.

Br,
- G.G.

Ville Syrjälä Sept. 10, 2019, 7:42 a.m. UTC | #11

On Tue, Sep 10, 2019 at 07:34:31AM +0000, Mun, Gwan-gyeong wrote:
> On Sat, 2019-09-07 at 21:43 -0400, Ilia Mirkin wrote:
> > On Sat, Sep 7, 2019 at 7:20 PM Mun, Gwan-gyeong
> > <[e-mail]> wrote:
> > > On Fri, 2019-09-06 at 09:24 -0400, Ilia Mirkin wrote:
> > > > On Fri, Sep 6, 2019 at 7:43 AM Ville Syrjälä
> > > > <[e-mail]> wrote:
> > > > > On Fri, Sep 06, 2019 at 11:31:55AM +0000, Shankar, Uma wrote:
> > > > > > > -----Original Message-----
> > > > > > > From: Ilia Mirkin <[e-mail]>
> > > > > > > Sent: Tuesday, September 3, 2019 6:12 PM
> > > > > > > To: Mun, Gwan-gyeong <[e-mail]>
> > > > > > > Cc: Intel Graphics Development <
> > > > > > > [e-mail]
> > > > > > > > ; Shankar, Uma
> > > > > > > <[e-mail]>; dri-devel <
> > > > > > > [e-mail]>
> > > > > > > Subject: Re: [PATCH v4 3/7] drm: Add DisplayPort colorspace
> > > > > > > property
> > > > > > > 
> > > > > > > So how would this work with a DP++ connector? Should it
> > > > > > > list
> > > > > > > the HDMI or DP
> > > > > > > properties? Or do we need a custom property checker which
> > > > > > > is
> > > > > > > aware of what is
> > > > > > > currently plugged in to validate the values?
> > > > > > 
> > > > > > AFAIU For DP++ cases, we detect what kind of sink its driving
> > > > > > DP
> > > > > > or HDMI (with a passive dongle).
> > > > > > Based on the type of sink detected, we should expose DP or
> > > > > > HDMI
> > > > > > colorspaces to userspace.
> > > > > 
> > > > > For i915 DP connector always drives DP mode, HDMI connector
> > > > > always
> > > > > drives
> > > > > HDMI mode, even when the physical connector is DP++.
> > > > 
> > > > Right, i915 creates 2 connectors, while nouveau, radeon, and
> > > > amdgpu
> > > > create 1 connector (not sure about other drivers) for a single
> > > > physical DP++ socket. Since we supply the list of valid values at
> > > > the
> > > > time of creating the connector, we can't know at that point
> > > > whether
> > > > in
> > > > the future a HDMI or DP will be plugged into it.
> > > > 
> > > >   -ilia
> > > Ilia, does it mean that the drm_connector type is
> > > DRM_MODE_CONNECTOR_DisplayPort and protocol is DP++ mode?
> > 
> > That is correct. The connector type is "DisplayPort" in such a case.
> > 
> > Cheers,
> > 
> >   -ilia
> 
> For now drm_mode_create_colorspace_property() is only used for i915.
> IMHO, when other drivers ( nouveau, radeon, and amdgpu ) are ready for
> using of drm_mode_create_colorspace_property(),
> what about do we add a variable which can identify DP++ and DP to
> drm_connector?
> And when the drivers (nouveau, radeon, and amdgpu) detect the current
> protocol, the drivers will set the variable.

IMO better to just have two functions in that case: one for DP, another
for HDMI.

Ilia Mirkin Sept. 10, 2019, 1:21 p.m. UTC | #12

On Tue, Sep 10, 2019 at 3:34 AM Mun, Gwan-gyeong
<[e-mail]> wrote:
>
> On Sat, 2019-09-07 at 21:43 -0400, Ilia Mirkin wrote:
> > On Sat, Sep 7, 2019 at 7:20 PM Mun, Gwan-gyeong
> > <[e-mail]> wrote:
> > > On Fri, 2019-09-06 at 09:24 -0400, Ilia Mirkin wrote:
> > > > On Fri, Sep 6, 2019 at 7:43 AM Ville Syrjälä
> > > > <[e-mail]> wrote:
> > > > > On Fri, Sep 06, 2019 at 11:31:55AM +0000, Shankar, Uma wrote:
> > > > > > > -----Original Message-----
> > > > > > > From: Ilia Mirkin <[e-mail]>
> > > > > > > Sent: Tuesday, September 3, 2019 6:12 PM
> > > > > > > To: Mun, Gwan-gyeong <[e-mail]>
> > > > > > > Cc: Intel Graphics Development <
> > > > > > > [e-mail]
> > > > > > > > ; Shankar, Uma
> > > > > > > <[e-mail]>; dri-devel <
> > > > > > > [e-mail]>
> > > > > > > Subject: Re: [PATCH v4 3/7] drm: Add DisplayPort colorspace
> > > > > > > property
> > > > > > >
> > > > > > > So how would this work with a DP++ connector? Should it
> > > > > > > list
> > > > > > > the HDMI or DP
> > > > > > > properties? Or do we need a custom property checker which
> > > > > > > is
> > > > > > > aware of what is
> > > > > > > currently plugged in to validate the values?
> > > > > >
> > > > > > AFAIU For DP++ cases, we detect what kind of sink its driving
> > > > > > DP
> > > > > > or HDMI (with a passive dongle).
> > > > > > Based on the type of sink detected, we should expose DP or
> > > > > > HDMI
> > > > > > colorspaces to userspace.
> > > > >
> > > > > For i915 DP connector always drives DP mode, HDMI connector
> > > > > always
> > > > > drives
> > > > > HDMI mode, even when the physical connector is DP++.
> > > >
> > > > Right, i915 creates 2 connectors, while nouveau, radeon, and
> > > > amdgpu
> > > > create 1 connector (not sure about other drivers) for a single
> > > > physical DP++ socket. Since we supply the list of valid values at
> > > > the
> > > > time of creating the connector, we can't know at that point
> > > > whether
> > > > in
> > > > the future a HDMI or DP will be plugged into it.
> > > >
> > > >   -ilia
> > > Ilia, does it mean that the drm_connector type is
> > > DRM_MODE_CONNECTOR_DisplayPort and protocol is DP++ mode?
> >
> > That is correct. The connector type is "DisplayPort" in such a case.
> >
> > Cheers,
> >
> >   -ilia
>
> For now drm_mode_create_colorspace_property() is only used for i915.
> IMHO, when other drivers ( nouveau, radeon, and amdgpu ) are ready for
> using of drm_mode_create_colorspace_property(),
> what about do we add a variable which can identify DP++ and DP to
> drm_connector?
> And when the drivers (nouveau, radeon, and amdgpu) detect the current
> protocol, the drivers will set the variable.

I've been working on adding this to nouveau.

Can/should such properties be added/removed at "runtime", rather than
at connector creation time? Either way, the function
drm_mode_create_colorspace_property as proposed would not be reusable,
since it looks at the connector type, which will always be
"DisplayPort" in such cases.

  -ilia

Ilia Mirkin Sept. 10, 2019, 4:14 p.m. UTC | #13

On Tue, Sep 10, 2019 at 9:21 AM Ilia Mirkin <[e-mail]> wrote:
>
> On Tue, Sep 10, 2019 at 3:34 AM Mun, Gwan-gyeong
> <[e-mail]> wrote:
> >
> > On Sat, 2019-09-07 at 21:43 -0400, Ilia Mirkin wrote:
> > > On Sat, Sep 7, 2019 at 7:20 PM Mun, Gwan-gyeong
> > > <[e-mail]> wrote:
> > > > On Fri, 2019-09-06 at 09:24 -0400, Ilia Mirkin wrote:
> > > > > On Fri, Sep 6, 2019 at 7:43 AM Ville Syrjälä
> > > > > <[e-mail]> wrote:
> > > > > > On Fri, Sep 06, 2019 at 11:31:55AM +0000, Shankar, Uma wrote:
> > > > > > > > -----Original Message-----
> > > > > > > > From: Ilia Mirkin <[e-mail]>
> > > > > > > > Sent: Tuesday, September 3, 2019 6:12 PM
> > > > > > > > To: Mun, Gwan-gyeong <[e-mail]>
> > > > > > > > Cc: Intel Graphics Development <
> > > > > > > > [e-mail]
> > > > > > > > > ; Shankar, Uma
> > > > > > > > <[e-mail]>; dri-devel <
> > > > > > > > [e-mail]>
> > > > > > > > Subject: Re: [PATCH v4 3/7] drm: Add DisplayPort colorspace
> > > > > > > > property
> > > > > > > >
> > > > > > > > So how would this work with a DP++ connector? Should it
> > > > > > > > list
> > > > > > > > the HDMI or DP
> > > > > > > > properties? Or do we need a custom property checker which
> > > > > > > > is
> > > > > > > > aware of what is
> > > > > > > > currently plugged in to validate the values?
> > > > > > >
> > > > > > > AFAIU For DP++ cases, we detect what kind of sink its driving
> > > > > > > DP
> > > > > > > or HDMI (with a passive dongle).
> > > > > > > Based on the type of sink detected, we should expose DP or
> > > > > > > HDMI
> > > > > > > colorspaces to userspace.
> > > > > >
> > > > > > For i915 DP connector always drives DP mode, HDMI connector
> > > > > > always
> > > > > > drives
> > > > > > HDMI mode, even when the physical connector is DP++.
> > > > >
> > > > > Right, i915 creates 2 connectors, while nouveau, radeon, and
> > > > > amdgpu
> > > > > create 1 connector (not sure about other drivers) for a single
> > > > > physical DP++ socket. Since we supply the list of valid values at
> > > > > the
> > > > > time of creating the connector, we can't know at that point
> > > > > whether
> > > > > in
> > > > > the future a HDMI or DP will be plugged into it.
> > > > >
> > > > >   -ilia
> > > > Ilia, does it mean that the drm_connector type is
> > > > DRM_MODE_CONNECTOR_DisplayPort and protocol is DP++ mode?
> > >
> > > That is correct. The connector type is "DisplayPort" in such a case.
> > >
> > > Cheers,
> > >
> > >   -ilia
> >
> > For now drm_mode_create_colorspace_property() is only used for i915.
> > IMHO, when other drivers ( nouveau, radeon, and amdgpu ) are ready for
> > using of drm_mode_create_colorspace_property(),
> > what about do we add a variable which can identify DP++ and DP to
> > drm_connector?
> > And when the drivers (nouveau, radeon, and amdgpu) detect the current
> > protocol, the drivers will set the variable.
>
> I've been working on adding this to nouveau.
>
> Can/should such properties be added/removed at "runtime", rather than
> at connector creation time? Either way, the function
> drm_mode_create_colorspace_property as proposed would not be reusable,
> since it looks at the connector type, which will always be
> "DisplayPort" in such cases.

Summary of conversation Ville and I had on IRC:

 - DP++ connectors to provide a single combined list of options for colorspace
 - set_property hook will check against currently plugged in thing,
and reject incorrect values
 - in the case where someone sets e.g. an HDMI value for an
HDMI-plugged-in thing, and then unplugs, and then plugs in a DP
screen, the modeset should continue to succeed but use a default
colorspace value.

I think there was a bit of contention on that last point. Open to
opinions, but we should try to avoid putting undue burden on esp
non-atomic userspace.

Cheers,

  -ilia

Patch

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 4c766624b20d..5834e6d330a0 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -882,6 +882,44 @@  static const struct drm_prop_enum_list hdmi_colorspaces[] = {
 	{ DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER, "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" },
+	/* Colorimetry based on IEC 61966-2-1 */
+	{ DRM_MODE_COLORIMETRY_SRGB, "sRGB" },
+	{ DRM_MODE_COLORIMETRY_WIDE_GAMUT_FIXED_POINT_RGB, "wide_gamut_fixed_point_RGB" },
+	/* Colorimetry based on IEC 61966-2-2, wide gamut floating point RGB */
+	{ DRM_MODE_COLORIMETRY_SCRGB, "scRGB" },
+	{ DRM_MODE_COLORIMETRY_ADOBE_RGB, "Adobe_RGB" },
+	/* Colorimetry based on SMPTE RP 431-2 */
+	{ DRM_MODE_COLORIMETRY_DCP_P3_RGB, "DCI-P3_RGB" },
+	/* 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" },
+	/*
+	 * Colorumetry based on Digital Imaging and Communications in Medicine
+	 * (DICOM) Part 14: Grayscale Standard Display Function
+	 */
+	{ DRM_MODE_COLORIMETRY_DICOM_PART_14_GRAYSCALE, "DICOM_Part_14_Grayscale" },
+};
+
 /**
  * DOC: standard connector properties
  *
@@ -1710,6 +1748,14 @@  int drm_mode_create_colorspace_property(struct drm_connector *connector)
 						ARRAY_SIZE(hdmi_colorspaces));
 		if (!prop)
 			return -ENOMEM;
+	} else if (connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort ||
+		   connector->connector_type == DRM_MODE_CONNECTOR_eDP) {
+		prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM,
+						"Colorspace",
+						dp_colorspaces,
+						ARRAY_SIZE(dp_colorspaces));
+		if (!prop)
+			return -ENOMEM;
 	} else {
 		DRM_DEBUG_KMS("Colorspace property not supported\n");
 		return 0;
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 681cb590f952..8848e5d6b0c4 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -281,6 +281,14 @@  enum drm_panel_orientation {
 /* Additional Colorimetry extension added as part of CTA 861.G */
 #define DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65		11
 #define DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER		12
+/* Additional Colorimetry Options added for DP 1.4a VSC Colorimetry Format */
+#define DRM_MODE_COLORIMETRY_SRGB			13
+#define DRM_MODE_COLORIMETRY_WIDE_GAMUT_FIXED_POINT_RGB	14
+#define DRM_MODE_COLORIMETRY_SCRGB			15
+#define DRM_MODE_COLORIMETRY_ADOBE_RGB			16
+#define DRM_MODE_COLORIMETRY_DCP_P3_RGB			17
+#define DRM_MODE_COLORIMETRY_BT601_YCC			18
+#define DRM_MODE_COLORIMETRY_DICOM_PART_14_GRAYSCALE	19
 
 /**
  * enum drm_bus_flags - bus_flags info for &drm_display_info