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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx