[v15,3/4] drm: Add colorspace info to AVI Infoframe

Submitted by Shankar, Uma on Feb. 8, 2019, 6:24 p.m.

Details

Message ID 1549650296-28648-4-git-send-email-uma.shankar@intel.com
State New
Headers show
Series "Add Colorspace connector property interface" ( rev: 14 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Shankar, Uma Feb. 8, 2019, 6:24 p.m.
This adds colorspace information to HDMI AVI infoframe.
A helper function is added to program the same.

v2: Moved this to drm core instead of i915 driver.

v3: Exported the helper function.

v4: Added separate HDMI specific macro as per CTA spec.
This is separate from user exposed enum values. This is
as per Ville's suggestion.

v5: Appended BT709 and SMPTE 170M with YCC information as per Ville's
review comment to be clear and not to be confused with RGB.

Signed-off-by: Uma Shankar <uma.shankar@intel.com>
---
 drivers/gpu/drm/drm_edid.c  | 57 +++++++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_connector.h | 20 ++++++++++++++++
 include/drm/drm_edid.h      |  6 +++++
 3 files changed, 83 insertions(+)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 990b190..5202fea 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -4925,6 +4925,63 @@  static bool is_hdmi2_sink(struct drm_connector *connector)
 EXPORT_SYMBOL(drm_hdmi_avi_infoframe_from_display_mode);
 
 /**
+ * drm_hdmi_avi_infoframe_colorspace() - fill the HDMI AVI infoframe
+ *                                       colorspace information
+ * @frame: HDMI AVI infoframe
+ * @conn_state: connector state
+ */
+void
+drm_hdmi_avi_infoframe_colorspace(struct hdmi_avi_infoframe *frame,
+				  const struct drm_connector_state *conn_state)
+{
+	u32 colorimetry_val = conn_state->colorspace &
+				FULL_COLORIMETRY_MASK;
+
+	switch (colorimetry_val) {
+	case DRM_MODE_COLORIMETRY_NO_DATA:
+		colorimetry_val = HDMI_COLORIMETRY_NO_DATA;
+		break;
+	case HDMI_COLORIMETRY_SMPTE_170M_YCC:
+		colorimetry_val = HDMI_COLORIMETRY_SMPTE_170M_YCC;
+		break;
+	case DRM_MODE_COLORIMETRY_BT709_YCC:
+		colorimetry_val = HDMI_COLORIMETRY_BT709_YCC;
+		break;
+	case DRM_MODE_COLORIMETRY_XVYCC_601:
+		colorimetry_val = HDMI_COLORIMETRY_XVYCC_601;
+		break;
+	case DRM_MODE_COLORIMETRY_XVYCC_709:
+		colorimetry_val = HDMI_COLORIMETRY_XVYCC_709;
+		break;
+	case DRM_MODE_COLORIMETRY_SYCC_601:
+		colorimetry_val = HDMI_COLORIMETRY_SYCC_601;
+		break;
+	case DRM_MODE_COLORIMETRY_OPYCC_601:
+		colorimetry_val = HDMI_COLORIMETRY_OPYCC_601;
+		break;
+	case DRM_MODE_COLORIMETRY_OPRGB:
+		colorimetry_val = HDMI_COLORIMETRY_OPRGB;
+		break;
+	case DRM_MODE_COLORIMETRY_BT2020_RGB:
+	case DRM_MODE_COLORIMETRY_BT2020_YCC:
+		colorimetry_val = HDMI_COLORIMETRY_BT2020_RGB;
+		break;
+	case DRM_MODE_COLORIMETRY_BT2020_CYCC:
+		colorimetry_val = HDMI_COLORIMETRY_BT2020_CYCC;
+		break;
+	default:
+		/* ToDo: DCI-P3 will be handled as part of ACE enabling */
+		colorimetry_val = HDMI_COLORIMETRY_NO_DATA;
+		break;
+	}
+
+	frame->colorimetry = colorimetry_val & NORMAL_COLORIMETRY_MASK;
+	frame->extended_colorimetry = (colorimetry_val >> 2) &
+					EXTENDED_COLORIMETRY_MASK;
+}
+EXPORT_SYMBOL(drm_hdmi_avi_infoframe_colorspace);
+
+/**
  * drm_hdmi_avi_infoframe_quant_range() - fill the HDMI AVI infoframe
  *                                        quantization range information
  * @frame: HDMI AVI infoframe
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 4d9aa2f..efacf29 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -288,6 +288,26 @@  enum drm_panel_orientation {
 #define DRM_MODE_DP_COLORIMETRY_RGB_WIDE_GAMUT		16
 #define DRM_MODE_DP_COLORIMETRY_SCRGB			17
 
+/* HDMI Colorspace Spec Definitions */
+#define FULL_COLORIMETRY_MASK				0x1FF
+#define NORMAL_COLORIMETRY_MASK				0x3
+#define EXTENDED_COLORIMETRY_MASK			0x7
+#define EXTENDED_ACE_COLORIMETRY_MASK			0xF
+
+#define HDMI_COLORIMETRY_NO_DATA			0x0
+#define HDMI_COLORIMETRY_SMPTE_170M_YCC			0x1
+#define HDMI_COLORIMETRY_BT709_YCC			0x2
+#define HDMI_COLORIMETRY_XVYCC_601			0x3
+#define HDMI_COLORIMETRY_XVYCC_709			0x7
+#define HDMI_COLORIMETRY_SYCC_601			0xB
+#define HDMI_COLORIMETRY_OPYCC_601			0xF
+#define HDMI_COLORIMETRY_OPRGB				0x13
+#define HDMI_COLORIMETRY_BT2020_CYCC			0x17
+#define HDMI_COLORIMETRY_BT2020_RGB			0x1C
+#define HDMI_COLORIMETRY_BT2020_YCC			0x1C
+#define HDMI_COLORIMETRY_DCI_P3_RGB_D65			0x1F
+#define HDMI_COLORIMETRY_DCI_P3_RGB_THEATER		0x3F
+
 /**
  * struct drm_display_info - runtime data about the connected sink
  *
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 8dc1a08..9d3b5b9 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -331,6 +331,7 @@  struct cea_sad {
 
 struct drm_encoder;
 struct drm_connector;
+struct drm_connector_state;
 struct drm_display_mode;
 
 int drm_edid_to_sad(struct edid *edid, struct cea_sad **sads);
@@ -358,6 +359,11 @@  int drm_av_sync_delay(struct drm_connector *connector,
 drm_hdmi_vendor_infoframe_from_display_mode(struct hdmi_vendor_infoframe *frame,
 					    struct drm_connector *connector,
 					    const struct drm_display_mode *mode);
+
+void
+drm_hdmi_avi_infoframe_colorspace(struct hdmi_avi_infoframe *frame,
+				  const struct drm_connector_state *conn_state);
+
 void
 drm_hdmi_avi_infoframe_quant_range(struct hdmi_avi_infoframe *frame,
 				   struct drm_connector *connector,

Comments

On Fri, Feb 08, 2019 at 11:54:55PM +0530, Uma Shankar wrote:
> This adds colorspace information to HDMI AVI infoframe.
> A helper function is added to program the same.
> 
> v2: Moved this to drm core instead of i915 driver.
> 
> v3: Exported the helper function.
> 
> v4: Added separate HDMI specific macro as per CTA spec.
> This is separate from user exposed enum values. This is
> as per Ville's suggestion.
> 
> v5: Appended BT709 and SMPTE 170M with YCC information as per Ville's
> review comment to be clear and not to be confused with RGB.
> 
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c  | 57 +++++++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_connector.h | 20 ++++++++++++++++
>  include/drm/drm_edid.h      |  6 +++++
>  3 files changed, 83 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 990b190..5202fea 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -4925,6 +4925,63 @@ static bool is_hdmi2_sink(struct drm_connector *connector)
>  EXPORT_SYMBOL(drm_hdmi_avi_infoframe_from_display_mode);
>  
>  /**
> + * drm_hdmi_avi_infoframe_colorspace() - fill the HDMI AVI infoframe
> + *                                       colorspace information
> + * @frame: HDMI AVI infoframe
> + * @conn_state: connector state
> + */
> +void
> +drm_hdmi_avi_infoframe_colorspace(struct hdmi_avi_infoframe *frame,
> +				  const struct drm_connector_state *conn_state)
> +{
> +	u32 colorimetry_val = conn_state->colorspace &
> +				FULL_COLORIMETRY_MASK;
> +
> +	switch (colorimetry_val) {
> +	case DRM_MODE_COLORIMETRY_NO_DATA:
> +		colorimetry_val = HDMI_COLORIMETRY_NO_DATA;
> +		break;
> +	case HDMI_COLORIMETRY_SMPTE_170M_YCC:
> +		colorimetry_val = HDMI_COLORIMETRY_SMPTE_170M_YCC;
> +		break;
> +	case DRM_MODE_COLORIMETRY_BT709_YCC:
> +		colorimetry_val = HDMI_COLORIMETRY_BT709_YCC;
> +		break;
> +	case DRM_MODE_COLORIMETRY_XVYCC_601:
> +		colorimetry_val = HDMI_COLORIMETRY_XVYCC_601;
> +		break;
> +	case DRM_MODE_COLORIMETRY_XVYCC_709:
> +		colorimetry_val = HDMI_COLORIMETRY_XVYCC_709;
> +		break;
> +	case DRM_MODE_COLORIMETRY_SYCC_601:
> +		colorimetry_val = HDMI_COLORIMETRY_SYCC_601;
> +		break;
> +	case DRM_MODE_COLORIMETRY_OPYCC_601:
> +		colorimetry_val = HDMI_COLORIMETRY_OPYCC_601;
> +		break;
> +	case DRM_MODE_COLORIMETRY_OPRGB:
> +		colorimetry_val = HDMI_COLORIMETRY_OPRGB;
> +		break;
> +	case DRM_MODE_COLORIMETRY_BT2020_RGB:
> +	case DRM_MODE_COLORIMETRY_BT2020_YCC:
> +		colorimetry_val = HDMI_COLORIMETRY_BT2020_RGB;
> +		break;
> +	case DRM_MODE_COLORIMETRY_BT2020_CYCC:
> +		colorimetry_val = HDMI_COLORIMETRY_BT2020_CYCC;
> +		break;
> +	default:
> +		/* ToDo: DCI-P3 will be handled as part of ACE enabling */
> +		colorimetry_val = HDMI_COLORIMETRY_NO_DATA;
> +		break;
> +	}

A table would probably be prettier.

> +
> +	frame->colorimetry = colorimetry_val & NORMAL_COLORIMETRY_MASK;
> +	frame->extended_colorimetry = (colorimetry_val >> 2) &
> +					EXTENDED_COLORIMETRY_MASK;
> +}
> +EXPORT_SYMBOL(drm_hdmi_avi_infoframe_colorspace);
> +
> +/**
>   * drm_hdmi_avi_infoframe_quant_range() - fill the HDMI AVI infoframe
>   *                                        quantization range information
>   * @frame: HDMI AVI infoframe
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 4d9aa2f..efacf29 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -288,6 +288,26 @@ enum drm_panel_orientation {
>  #define DRM_MODE_DP_COLORIMETRY_RGB_WIDE_GAMUT		16
>  #define DRM_MODE_DP_COLORIMETRY_SCRGB			17
>  
> +/* HDMI Colorspace Spec Definitions */
> +#define FULL_COLORIMETRY_MASK				0x1FF
> +#define NORMAL_COLORIMETRY_MASK				0x3
> +#define EXTENDED_COLORIMETRY_MASK			0x7
> +#define EXTENDED_ACE_COLORIMETRY_MASK			0xF
> +
> +#define HDMI_COLORIMETRY_NO_DATA			0x0
> +#define HDMI_COLORIMETRY_SMPTE_170M_YCC			0x1
> +#define HDMI_COLORIMETRY_BT709_YCC			0x2
> +#define HDMI_COLORIMETRY_XVYCC_601			0x3
> +#define HDMI_COLORIMETRY_XVYCC_709			0x7
> +#define HDMI_COLORIMETRY_SYCC_601			0xB
> +#define HDMI_COLORIMETRY_OPYCC_601			0xF
> +#define HDMI_COLORIMETRY_OPRGB				0x13
> +#define HDMI_COLORIMETRY_BT2020_CYCC			0x17
> +#define HDMI_COLORIMETRY_BT2020_RGB			0x1C
> +#define HDMI_COLORIMETRY_BT2020_YCC			0x1C

Those should be 0x1b

I would have suggested something like:
#define C0(x) ((x) << 0)
#define C1(x) ((x) << 1)
#define EC0(x) ((x) << 2)
#define EC1(x) ((x) << 3)
etc.

to make it easier to cross check against the spec. Or if a define for
each bit is too much maybe just something like:

#define C(x) ((x) << 0)
#define EC(x) ((x) << 2)
#define ACE(x) ((x) << 5)

One problem is namespace pollution though. But since these aren't used
elsewhere we could move them into .c file instead. Or maybe we should
stuff them into linux/hdmi.h and avoid the namespace pollution by
C/EC/ACE by using an enum and undeffing the helper macros after the
enum has been defined. The use of an enum would be in line with other
stuff in hdmi.h.

> +#define HDMI_COLORIMETRY_DCI_P3_RGB_D65			0x1F
> +#define HDMI_COLORIMETRY_DCI_P3_RGB_THEATER		0x3F
> +
>  /**
>   * struct drm_display_info - runtime data about the connected sink
>   *
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 8dc1a08..9d3b5b9 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -331,6 +331,7 @@ struct cea_sad {
>  
>  struct drm_encoder;
>  struct drm_connector;
> +struct drm_connector_state;
>  struct drm_display_mode;
>  
>  int drm_edid_to_sad(struct edid *edid, struct cea_sad **sads);
> @@ -358,6 +359,11 @@ int drm_av_sync_delay(struct drm_connector *connector,
>  drm_hdmi_vendor_infoframe_from_display_mode(struct hdmi_vendor_infoframe *frame,
>  					    struct drm_connector *connector,
>  					    const struct drm_display_mode *mode);
> +
> +void
> +drm_hdmi_avi_infoframe_colorspace(struct hdmi_avi_infoframe *frame,
> +				  const struct drm_connector_state *conn_state);
> +
>  void
>  drm_hdmi_avi_infoframe_quant_range(struct hdmi_avi_infoframe *frame,
>  				   struct drm_connector *connector,
> -- 
> 1.9.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>Sent: Tuesday, February 12, 2019 10:35 PM
>To: Shankar, Uma <uma.shankar@intel.com>
>Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Syrjala, Ville
><ville.syrjala@intel.com>; emil.l.velikov@gmail.com; Lankhorst, Maarten
><maarten.lankhorst@intel.com>
>Subject: Re: [v15 3/4] drm: Add colorspace info to AVI Infoframe
>
>On Fri, Feb 08, 2019 at 11:54:55PM +0530, Uma Shankar wrote:
>> This adds colorspace information to HDMI AVI infoframe.
>> A helper function is added to program the same.
>>
>> v2: Moved this to drm core instead of i915 driver.
>>
>> v3: Exported the helper function.
>>
>> v4: Added separate HDMI specific macro as per CTA spec.
>> This is separate from user exposed enum values. This is as per Ville's
>> suggestion.
>>
>> v5: Appended BT709 and SMPTE 170M with YCC information as per Ville's
>> review comment to be clear and not to be confused with RGB.
>>
>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>> ---
>>  drivers/gpu/drm/drm_edid.c  | 57
>> +++++++++++++++++++++++++++++++++++++++++++++
>>  include/drm/drm_connector.h | 20 ++++++++++++++++
>>  include/drm/drm_edid.h      |  6 +++++
>>  3 files changed, 83 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index 990b190..5202fea 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -4925,6 +4925,63 @@ static bool is_hdmi2_sink(struct drm_connector
>> *connector)  EXPORT_SYMBOL(drm_hdmi_avi_infoframe_from_display_mode);
>>
>>  /**
>> + * drm_hdmi_avi_infoframe_colorspace() - fill the HDMI AVI infoframe
>> + *                                       colorspace information
>> + * @frame: HDMI AVI infoframe
>> + * @conn_state: connector state
>> + */
>> +void
>> +drm_hdmi_avi_infoframe_colorspace(struct hdmi_avi_infoframe *frame,
>> +				  const struct drm_connector_state *conn_state) {
>> +	u32 colorimetry_val = conn_state->colorspace &
>> +				FULL_COLORIMETRY_MASK;
>> +
>> +	switch (colorimetry_val) {
>> +	case DRM_MODE_COLORIMETRY_NO_DATA:
>> +		colorimetry_val = HDMI_COLORIMETRY_NO_DATA;
>> +		break;
>> +	case HDMI_COLORIMETRY_SMPTE_170M_YCC:
>> +		colorimetry_val = HDMI_COLORIMETRY_SMPTE_170M_YCC;
>> +		break;
>> +	case DRM_MODE_COLORIMETRY_BT709_YCC:
>> +		colorimetry_val = HDMI_COLORIMETRY_BT709_YCC;
>> +		break;
>> +	case DRM_MODE_COLORIMETRY_XVYCC_601:
>> +		colorimetry_val = HDMI_COLORIMETRY_XVYCC_601;
>> +		break;
>> +	case DRM_MODE_COLORIMETRY_XVYCC_709:
>> +		colorimetry_val = HDMI_COLORIMETRY_XVYCC_709;
>> +		break;
>> +	case DRM_MODE_COLORIMETRY_SYCC_601:
>> +		colorimetry_val = HDMI_COLORIMETRY_SYCC_601;
>> +		break;
>> +	case DRM_MODE_COLORIMETRY_OPYCC_601:
>> +		colorimetry_val = HDMI_COLORIMETRY_OPYCC_601;
>> +		break;
>> +	case DRM_MODE_COLORIMETRY_OPRGB:
>> +		colorimetry_val = HDMI_COLORIMETRY_OPRGB;
>> +		break;
>> +	case DRM_MODE_COLORIMETRY_BT2020_RGB:
>> +	case DRM_MODE_COLORIMETRY_BT2020_YCC:
>> +		colorimetry_val = HDMI_COLORIMETRY_BT2020_RGB;
>> +		break;
>> +	case DRM_MODE_COLORIMETRY_BT2020_CYCC:
>> +		colorimetry_val = HDMI_COLORIMETRY_BT2020_CYCC;
>> +		break;
>> +	default:
>> +		/* ToDo: DCI-P3 will be handled as part of ACE enabling */
>> +		colorimetry_val = HDMI_COLORIMETRY_NO_DATA;
>> +		break;
>> +	}
>
>A table would probably be prettier.

Ok, I will add that as table.

>> +
>> +	frame->colorimetry = colorimetry_val & NORMAL_COLORIMETRY_MASK;
>> +	frame->extended_colorimetry = (colorimetry_val >> 2) &
>> +					EXTENDED_COLORIMETRY_MASK;
>> +}
>> +EXPORT_SYMBOL(drm_hdmi_avi_infoframe_colorspace);
>> +
>> +/**
>>   * drm_hdmi_avi_infoframe_quant_range() - fill the HDMI AVI infoframe
>>   *                                        quantization range information
>>   * @frame: HDMI AVI infoframe
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index 4d9aa2f..efacf29 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -288,6 +288,26 @@ enum drm_panel_orientation {
>>  #define DRM_MODE_DP_COLORIMETRY_RGB_WIDE_GAMUT		16
>>  #define DRM_MODE_DP_COLORIMETRY_SCRGB			17
>>
>> +/* HDMI Colorspace Spec Definitions */
>> +#define FULL_COLORIMETRY_MASK				0x1FF
>> +#define NORMAL_COLORIMETRY_MASK				0x3
>> +#define EXTENDED_COLORIMETRY_MASK			0x7
>> +#define EXTENDED_ACE_COLORIMETRY_MASK			0xF
>> +
>> +#define HDMI_COLORIMETRY_NO_DATA			0x0
>> +#define HDMI_COLORIMETRY_SMPTE_170M_YCC			0x1
>> +#define HDMI_COLORIMETRY_BT709_YCC			0x2
>> +#define HDMI_COLORIMETRY_XVYCC_601			0x3
>> +#define HDMI_COLORIMETRY_XVYCC_709			0x7
>> +#define HDMI_COLORIMETRY_SYCC_601			0xB
>> +#define HDMI_COLORIMETRY_OPYCC_601			0xF
>> +#define HDMI_COLORIMETRY_OPRGB				0x13
>> +#define HDMI_COLORIMETRY_BT2020_CYCC			0x17
>> +#define HDMI_COLORIMETRY_BT2020_RGB			0x1C
>> +#define HDMI_COLORIMETRY_BT2020_YCC			0x1C
>
>Those should be 0x1b
>
>I would have suggested something like:
>#define C0(x) ((x) << 0)
>#define C1(x) ((x) << 1)
>#define EC0(x) ((x) << 2)
>#define EC1(x) ((x) << 3)
>etc.
>
>to make it easier to cross check against the spec. Or if a define for each bit is too
>much maybe just something like:
>
>#define C(x) ((x) << 0)
>#define EC(x) ((x) << 2)
>#define ACE(x) ((x) << 5)

This looks good, will modify as per this.

>One problem is namespace pollution though. But since these aren't used elsewhere
>we could move them into .c file instead. Or maybe we should stuff them into
>linux/hdmi.h and avoid the namespace pollution by C/EC/ACE by using an enum and
>undeffing the helper macros after the enum has been defined. The use of an enum
>would be in line with other stuff in hdmi.h.

Since no one else is supposed to be using this, should I just move it to the C file where we are
using them to avoid any namespace issues as such. If that is fine, will send out the next version
addressing all the comments.

Thanks Ville for the review and valuable inputs.

Regards,
Uma Shankar

>
>> +#define HDMI_COLORIMETRY_DCI_P3_RGB_D65			0x1F
>> +#define HDMI_COLORIMETRY_DCI_P3_RGB_THEATER		0x3F
>> +
>>  /**
>>   * struct drm_display_info - runtime data about the connected sink
>>   *
>> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h index
>> 8dc1a08..9d3b5b9 100644
>> --- a/include/drm/drm_edid.h
>> +++ b/include/drm/drm_edid.h
>> @@ -331,6 +331,7 @@ struct cea_sad {
>>
>>  struct drm_encoder;
>>  struct drm_connector;
>> +struct drm_connector_state;
>>  struct drm_display_mode;
>>
>>  int drm_edid_to_sad(struct edid *edid, struct cea_sad **sads); @@
>> -358,6 +359,11 @@ int drm_av_sync_delay(struct drm_connector
>> *connector,  drm_hdmi_vendor_infoframe_from_display_mode(struct
>hdmi_vendor_infoframe *frame,
>>  					    struct drm_connector *connector,
>>  					    const struct drm_display_mode *mode);
>> +
>> +void
>> +drm_hdmi_avi_infoframe_colorspace(struct hdmi_avi_infoframe *frame,
>> +				  const struct drm_connector_state *conn_state);
>> +
>>  void
>>  drm_hdmi_avi_infoframe_quant_range(struct hdmi_avi_infoframe *frame,
>>  				   struct drm_connector *connector,
>> --
>> 1.9.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>--
>Ville Syrjälä
>Intel
On Tue, Feb 12, 2019 at 09:30:50PM +0000, Shankar, Uma wrote:
> 
> 
> >-----Original Message-----
> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> >Sent: Tuesday, February 12, 2019 10:35 PM
> >To: Shankar, Uma <uma.shankar@intel.com>
> >Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Syrjala, Ville
> ><ville.syrjala@intel.com>; emil.l.velikov@gmail.com; Lankhorst, Maarten
> ><maarten.lankhorst@intel.com>
> >Subject: Re: [v15 3/4] drm: Add colorspace info to AVI Infoframe
> >
> >On Fri, Feb 08, 2019 at 11:54:55PM +0530, Uma Shankar wrote:
> >> This adds colorspace information to HDMI AVI infoframe.
> >> A helper function is added to program the same.
> >>
> >> v2: Moved this to drm core instead of i915 driver.
> >>
> >> v3: Exported the helper function.
> >>
> >> v4: Added separate HDMI specific macro as per CTA spec.
> >> This is separate from user exposed enum values. This is as per Ville's
> >> suggestion.
> >>
> >> v5: Appended BT709 and SMPTE 170M with YCC information as per Ville's
> >> review comment to be clear and not to be confused with RGB.
> >>
> >> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> >> ---
> >>  drivers/gpu/drm/drm_edid.c  | 57
> >> +++++++++++++++++++++++++++++++++++++++++++++
> >>  include/drm/drm_connector.h | 20 ++++++++++++++++
> >>  include/drm/drm_edid.h      |  6 +++++
> >>  3 files changed, 83 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> >> index 990b190..5202fea 100644
> >> --- a/drivers/gpu/drm/drm_edid.c
> >> +++ b/drivers/gpu/drm/drm_edid.c
> >> @@ -4925,6 +4925,63 @@ static bool is_hdmi2_sink(struct drm_connector
> >> *connector)  EXPORT_SYMBOL(drm_hdmi_avi_infoframe_from_display_mode);
> >>
> >>  /**
> >> + * drm_hdmi_avi_infoframe_colorspace() - fill the HDMI AVI infoframe
> >> + *                                       colorspace information
> >> + * @frame: HDMI AVI infoframe
> >> + * @conn_state: connector state
> >> + */
> >> +void
> >> +drm_hdmi_avi_infoframe_colorspace(struct hdmi_avi_infoframe *frame,
> >> +				  const struct drm_connector_state *conn_state) {
> >> +	u32 colorimetry_val = conn_state->colorspace &
> >> +				FULL_COLORIMETRY_MASK;
> >> +
> >> +	switch (colorimetry_val) {
> >> +	case DRM_MODE_COLORIMETRY_NO_DATA:
> >> +		colorimetry_val = HDMI_COLORIMETRY_NO_DATA;
> >> +		break;
> >> +	case HDMI_COLORIMETRY_SMPTE_170M_YCC:
> >> +		colorimetry_val = HDMI_COLORIMETRY_SMPTE_170M_YCC;
> >> +		break;
> >> +	case DRM_MODE_COLORIMETRY_BT709_YCC:
> >> +		colorimetry_val = HDMI_COLORIMETRY_BT709_YCC;
> >> +		break;
> >> +	case DRM_MODE_COLORIMETRY_XVYCC_601:
> >> +		colorimetry_val = HDMI_COLORIMETRY_XVYCC_601;
> >> +		break;
> >> +	case DRM_MODE_COLORIMETRY_XVYCC_709:
> >> +		colorimetry_val = HDMI_COLORIMETRY_XVYCC_709;
> >> +		break;
> >> +	case DRM_MODE_COLORIMETRY_SYCC_601:
> >> +		colorimetry_val = HDMI_COLORIMETRY_SYCC_601;
> >> +		break;
> >> +	case DRM_MODE_COLORIMETRY_OPYCC_601:
> >> +		colorimetry_val = HDMI_COLORIMETRY_OPYCC_601;
> >> +		break;
> >> +	case DRM_MODE_COLORIMETRY_OPRGB:
> >> +		colorimetry_val = HDMI_COLORIMETRY_OPRGB;
> >> +		break;
> >> +	case DRM_MODE_COLORIMETRY_BT2020_RGB:
> >> +	case DRM_MODE_COLORIMETRY_BT2020_YCC:
> >> +		colorimetry_val = HDMI_COLORIMETRY_BT2020_RGB;
> >> +		break;
> >> +	case DRM_MODE_COLORIMETRY_BT2020_CYCC:
> >> +		colorimetry_val = HDMI_COLORIMETRY_BT2020_CYCC;
> >> +		break;
> >> +	default:
> >> +		/* ToDo: DCI-P3 will be handled as part of ACE enabling */
> >> +		colorimetry_val = HDMI_COLORIMETRY_NO_DATA;
> >> +		break;
> >> +	}
> >
> >A table would probably be prettier.
> 
> Ok, I will add that as table.
> 
> >> +
> >> +	frame->colorimetry = colorimetry_val & NORMAL_COLORIMETRY_MASK;
> >> +	frame->extended_colorimetry = (colorimetry_val >> 2) &
> >> +					EXTENDED_COLORIMETRY_MASK;
> >> +}
> >> +EXPORT_SYMBOL(drm_hdmi_avi_infoframe_colorspace);
> >> +
> >> +/**
> >>   * drm_hdmi_avi_infoframe_quant_range() - fill the HDMI AVI infoframe
> >>   *                                        quantization range information
> >>   * @frame: HDMI AVI infoframe
> >> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> >> index 4d9aa2f..efacf29 100644
> >> --- a/include/drm/drm_connector.h
> >> +++ b/include/drm/drm_connector.h
> >> @@ -288,6 +288,26 @@ enum drm_panel_orientation {
> >>  #define DRM_MODE_DP_COLORIMETRY_RGB_WIDE_GAMUT		16
> >>  #define DRM_MODE_DP_COLORIMETRY_SCRGB			17
> >>
> >> +/* HDMI Colorspace Spec Definitions */
> >> +#define FULL_COLORIMETRY_MASK				0x1FF
> >> +#define NORMAL_COLORIMETRY_MASK				0x3
> >> +#define EXTENDED_COLORIMETRY_MASK			0x7
> >> +#define EXTENDED_ACE_COLORIMETRY_MASK			0xF
> >> +
> >> +#define HDMI_COLORIMETRY_NO_DATA			0x0
> >> +#define HDMI_COLORIMETRY_SMPTE_170M_YCC			0x1
> >> +#define HDMI_COLORIMETRY_BT709_YCC			0x2
> >> +#define HDMI_COLORIMETRY_XVYCC_601			0x3
> >> +#define HDMI_COLORIMETRY_XVYCC_709			0x7
> >> +#define HDMI_COLORIMETRY_SYCC_601			0xB
> >> +#define HDMI_COLORIMETRY_OPYCC_601			0xF
> >> +#define HDMI_COLORIMETRY_OPRGB				0x13
> >> +#define HDMI_COLORIMETRY_BT2020_CYCC			0x17
> >> +#define HDMI_COLORIMETRY_BT2020_RGB			0x1C
> >> +#define HDMI_COLORIMETRY_BT2020_YCC			0x1C
> >
> >Those should be 0x1b
> >
> >I would have suggested something like:
> >#define C0(x) ((x) << 0)
> >#define C1(x) ((x) << 1)
> >#define EC0(x) ((x) << 2)
> >#define EC1(x) ((x) << 3)
> >etc.
> >
> >to make it easier to cross check against the spec. Or if a define for each bit is too
> >much maybe just something like:
> >
> >#define C(x) ((x) << 0)
> >#define EC(x) ((x) << 2)
> >#define ACE(x) ((x) << 5)
> 
> This looks good, will modify as per this.
> 
> >One problem is namespace pollution though. But since these aren't used elsewhere
> >we could move them into .c file instead. Or maybe we should stuff them into
> >linux/hdmi.h and avoid the namespace pollution by C/EC/ACE by using an enum and
> >undeffing the helper macros after the enum has been defined. The use of an enum
> >would be in line with other stuff in hdmi.h.
> 
> Since no one else is supposed to be using this, should I just move it to the C file where we are
> using them to avoid any namespace issues as such. If that is fine, will send out the next version
> addressing all the comments.

Yeah, I guess having it in the c file is fine. Someone can move it if
they need it elsewhere.

> 
> Thanks Ville for the review and valuable inputs.
> 
> Regards,
> Uma Shankar
> 
> >
> >> +#define HDMI_COLORIMETRY_DCI_P3_RGB_D65			0x1F
> >> +#define HDMI_COLORIMETRY_DCI_P3_RGB_THEATER		0x3F
> >> +
> >>  /**
> >>   * struct drm_display_info - runtime data about the connected sink
> >>   *
> >> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h index
> >> 8dc1a08..9d3b5b9 100644
> >> --- a/include/drm/drm_edid.h
> >> +++ b/include/drm/drm_edid.h
> >> @@ -331,6 +331,7 @@ struct cea_sad {
> >>
> >>  struct drm_encoder;
> >>  struct drm_connector;
> >> +struct drm_connector_state;
> >>  struct drm_display_mode;
> >>
> >>  int drm_edid_to_sad(struct edid *edid, struct cea_sad **sads); @@
> >> -358,6 +359,11 @@ int drm_av_sync_delay(struct drm_connector
> >> *connector,  drm_hdmi_vendor_infoframe_from_display_mode(struct
> >hdmi_vendor_infoframe *frame,
> >>  					    struct drm_connector *connector,
> >>  					    const struct drm_display_mode *mode);
> >> +
> >> +void
> >> +drm_hdmi_avi_infoframe_colorspace(struct hdmi_avi_infoframe *frame,
> >> +				  const struct drm_connector_state *conn_state);
> >> +
> >>  void
> >>  drm_hdmi_avi_infoframe_quant_range(struct hdmi_avi_infoframe *frame,
> >>  				   struct drm_connector *connector,
> >> --
> >> 1.9.1
> >>
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> >--
> >Ville Syrjälä
> >Intel
>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>Sent: Wednesday, February 13, 2019 4:34 PM
>To: Shankar, Uma <uma.shankar@intel.com>
>Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Syrjala, Ville
><ville.syrjala@intel.com>; emil.l.velikov@gmail.com; Lankhorst, Maarten
><maarten.lankhorst@intel.com>
>Subject: Re: [v15 3/4] drm: Add colorspace info to AVI Infoframe
>
>On Tue, Feb 12, 2019 at 09:30:50PM +0000, Shankar, Uma wrote:
>>
>>
>> >-----Original Message-----
>> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>> >Sent: Tuesday, February 12, 2019 10:35 PM
>> >To: Shankar, Uma <uma.shankar@intel.com>
>> >Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org;
>> >Syrjala, Ville <ville.syrjala@intel.com>; emil.l.velikov@gmail.com;
>> >Lankhorst, Maarten <maarten.lankhorst@intel.com>
>> >Subject: Re: [v15 3/4] drm: Add colorspace info to AVI Infoframe
>> >
>> >On Fri, Feb 08, 2019 at 11:54:55PM +0530, Uma Shankar wrote:
>> >> This adds colorspace information to HDMI AVI infoframe.
>> >> A helper function is added to program the same.
>> >>
>> >> v2: Moved this to drm core instead of i915 driver.
>> >>
>> >> v3: Exported the helper function.
>> >>
>> >> v4: Added separate HDMI specific macro as per CTA spec.
>> >> This is separate from user exposed enum values. This is as per
>> >> Ville's suggestion.
>> >>
>> >> v5: Appended BT709 and SMPTE 170M with YCC information as per
>> >> Ville's review comment to be clear and not to be confused with RGB.
>> >>
>> >> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>> >> ---
>> >>  drivers/gpu/drm/drm_edid.c  | 57
>> >> +++++++++++++++++++++++++++++++++++++++++++++
>> >>  include/drm/drm_connector.h | 20 ++++++++++++++++
>> >>  include/drm/drm_edid.h      |  6 +++++
>> >>  3 files changed, 83 insertions(+)
>> >>
>> >> diff --git a/drivers/gpu/drm/drm_edid.c
>> >> b/drivers/gpu/drm/drm_edid.c index 990b190..5202fea 100644
>> >> --- a/drivers/gpu/drm/drm_edid.c
>> >> +++ b/drivers/gpu/drm/drm_edid.c
>> >> @@ -4925,6 +4925,63 @@ static bool is_hdmi2_sink(struct
>> >> drm_connector
>> >> *connector)
>> >> EXPORT_SYMBOL(drm_hdmi_avi_infoframe_from_display_mode);
>> >>
>> >>  /**
>> >> + * drm_hdmi_avi_infoframe_colorspace() - fill the HDMI AVI infoframe
>> >> + *                                       colorspace information
>> >> + * @frame: HDMI AVI infoframe
>> >> + * @conn_state: connector state
>> >> + */
>> >> +void
>> >> +drm_hdmi_avi_infoframe_colorspace(struct hdmi_avi_infoframe *frame,
>> >> +				  const struct drm_connector_state *conn_state) {
>> >> +	u32 colorimetry_val = conn_state->colorspace &
>> >> +				FULL_COLORIMETRY_MASK;
>> >> +
>> >> +	switch (colorimetry_val) {
>> >> +	case DRM_MODE_COLORIMETRY_NO_DATA:
>> >> +		colorimetry_val = HDMI_COLORIMETRY_NO_DATA;
>> >> +		break;
>> >> +	case HDMI_COLORIMETRY_SMPTE_170M_YCC:
>> >> +		colorimetry_val = HDMI_COLORIMETRY_SMPTE_170M_YCC;
>> >> +		break;
>> >> +	case DRM_MODE_COLORIMETRY_BT709_YCC:
>> >> +		colorimetry_val = HDMI_COLORIMETRY_BT709_YCC;
>> >> +		break;
>> >> +	case DRM_MODE_COLORIMETRY_XVYCC_601:
>> >> +		colorimetry_val = HDMI_COLORIMETRY_XVYCC_601;
>> >> +		break;
>> >> +	case DRM_MODE_COLORIMETRY_XVYCC_709:
>> >> +		colorimetry_val = HDMI_COLORIMETRY_XVYCC_709;
>> >> +		break;
>> >> +	case DRM_MODE_COLORIMETRY_SYCC_601:
>> >> +		colorimetry_val = HDMI_COLORIMETRY_SYCC_601;
>> >> +		break;
>> >> +	case DRM_MODE_COLORIMETRY_OPYCC_601:
>> >> +		colorimetry_val = HDMI_COLORIMETRY_OPYCC_601;
>> >> +		break;
>> >> +	case DRM_MODE_COLORIMETRY_OPRGB:
>> >> +		colorimetry_val = HDMI_COLORIMETRY_OPRGB;
>> >> +		break;
>> >> +	case DRM_MODE_COLORIMETRY_BT2020_RGB:
>> >> +	case DRM_MODE_COLORIMETRY_BT2020_YCC:
>> >> +		colorimetry_val = HDMI_COLORIMETRY_BT2020_RGB;
>> >> +		break;
>> >> +	case DRM_MODE_COLORIMETRY_BT2020_CYCC:
>> >> +		colorimetry_val = HDMI_COLORIMETRY_BT2020_CYCC;
>> >> +		break;
>> >> +	default:
>> >> +		/* ToDo: DCI-P3 will be handled as part of ACE enabling */
>> >> +		colorimetry_val = HDMI_COLORIMETRY_NO_DATA;
>> >> +		break;
>> >> +	}
>> >
>> >A table would probably be prettier.
>>
>> Ok, I will add that as table.
>>
>> >> +
>> >> +	frame->colorimetry = colorimetry_val & NORMAL_COLORIMETRY_MASK;
>> >> +	frame->extended_colorimetry = (colorimetry_val >> 2) &
>> >> +					EXTENDED_COLORIMETRY_MASK;
>> >> +}
>> >> +EXPORT_SYMBOL(drm_hdmi_avi_infoframe_colorspace);
>> >> +
>> >> +/**
>> >>   * drm_hdmi_avi_infoframe_quant_range() - fill the HDMI AVI infoframe
>> >>   *                                        quantization range information
>> >>   * @frame: HDMI AVI infoframe
>> >> diff --git a/include/drm/drm_connector.h
>> >> b/include/drm/drm_connector.h index 4d9aa2f..efacf29 100644
>> >> --- a/include/drm/drm_connector.h
>> >> +++ b/include/drm/drm_connector.h
>> >> @@ -288,6 +288,26 @@ enum drm_panel_orientation {
>> >>  #define DRM_MODE_DP_COLORIMETRY_RGB_WIDE_GAMUT		16
>> >>  #define DRM_MODE_DP_COLORIMETRY_SCRGB			17
>> >>
>> >> +/* HDMI Colorspace Spec Definitions */
>> >> +#define FULL_COLORIMETRY_MASK				0x1FF
>> >> +#define NORMAL_COLORIMETRY_MASK				0x3
>> >> +#define EXTENDED_COLORIMETRY_MASK			0x7
>> >> +#define EXTENDED_ACE_COLORIMETRY_MASK			0xF
>> >> +
>> >> +#define HDMI_COLORIMETRY_NO_DATA			0x0
>> >> +#define HDMI_COLORIMETRY_SMPTE_170M_YCC			0x1
>> >> +#define HDMI_COLORIMETRY_BT709_YCC			0x2
>> >> +#define HDMI_COLORIMETRY_XVYCC_601			0x3
>> >> +#define HDMI_COLORIMETRY_XVYCC_709			0x7
>> >> +#define HDMI_COLORIMETRY_SYCC_601			0xB
>> >> +#define HDMI_COLORIMETRY_OPYCC_601			0xF
>> >> +#define HDMI_COLORIMETRY_OPRGB				0x13
>> >> +#define HDMI_COLORIMETRY_BT2020_CYCC			0x17
>> >> +#define HDMI_COLORIMETRY_BT2020_RGB			0x1C
>> >> +#define HDMI_COLORIMETRY_BT2020_YCC			0x1C
>> >
>> >Those should be 0x1b
>> >
>> >I would have suggested something like:
>> >#define C0(x) ((x) << 0)
>> >#define C1(x) ((x) << 1)
>> >#define EC0(x) ((x) << 2)
>> >#define EC1(x) ((x) << 3)
>> >etc.
>> >
>> >to make it easier to cross check against the spec. Or if a define for
>> >each bit is too much maybe just something like:
>> >
>> >#define C(x) ((x) << 0)
>> >#define EC(x) ((x) << 2)
>> >#define ACE(x) ((x) << 5)
>>
>> This looks good, will modify as per this.
>>
>> >One problem is namespace pollution though. But since these aren't
>> >used elsewhere we could move them into .c file instead. Or maybe we
>> >should stuff them into linux/hdmi.h and avoid the namespace pollution
>> >by C/EC/ACE by using an enum and undeffing the helper macros after
>> >the enum has been defined. The use of an enum would be in line with other stuff in
>hdmi.h.
>>
>> Since no one else is supposed to be using this, should I just move it
>> to the C file where we are using them to avoid any namespace issues as
>> such. If that is fine, will send out the next version addressing all the comments.
>
>Yeah, I guess having it in the c file is fine. Someone can move it if they need it
>elsewhere.

Hi Ville,
Thanks, I have floated the next version with all the comments addressed. Please check and approve
if all looks ok.

Regards,
Uma Shankar
>>
>> Thanks Ville for the review and valuable inputs.
>>
>> Regards,
>> Uma Shankar
>>
>> >
>> >> +#define HDMI_COLORIMETRY_DCI_P3_RGB_D65			0x1F
>> >> +#define HDMI_COLORIMETRY_DCI_P3_RGB_THEATER		0x3F
>> >> +
>> >>  /**
>> >>   * struct drm_display_info - runtime data about the connected sink
>> >>   *
>> >> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h index
>> >> 8dc1a08..9d3b5b9 100644
>> >> --- a/include/drm/drm_edid.h
>> >> +++ b/include/drm/drm_edid.h
>> >> @@ -331,6 +331,7 @@ struct cea_sad {
>> >>
>> >>  struct drm_encoder;
>> >>  struct drm_connector;
>> >> +struct drm_connector_state;
>> >>  struct drm_display_mode;
>> >>
>> >>  int drm_edid_to_sad(struct edid *edid, struct cea_sad **sads); @@
>> >> -358,6 +359,11 @@ int drm_av_sync_delay(struct drm_connector
>> >> *connector,  drm_hdmi_vendor_infoframe_from_display_mode(struct
>> >hdmi_vendor_infoframe *frame,
>> >>  					    struct drm_connector *connector,
>> >>  					    const struct drm_display_mode *mode);
>> >> +
>> >> +void
>> >> +drm_hdmi_avi_infoframe_colorspace(struct hdmi_avi_infoframe *frame,
>> >> +				  const struct drm_connector_state *conn_state);
>> >> +
>> >>  void
>> >>  drm_hdmi_avi_infoframe_quant_range(struct hdmi_avi_infoframe *frame,
>> >>  				   struct drm_connector *connector,
>> >> --
>> >> 1.9.1
>> >>
>> >> _______________________________________________
>> >> dri-devel mailing list
>> >> dri-devel@lists.freedesktop.org
>> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>> >
>> >--
>> >Ville Syrjälä
>> >Intel
>
>--
>Ville Syrjälä
>Intel