Re: [Intel-gfx] [v16 3/4] drm: Add colorspace info to AVI Infoframe

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

On Tue, Feb 19, 2019 at 03:09:00PM +0000, Shankar, Uma wrote:
> 
> 
> >-----Original Message-----
> >From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf Of Ville
> >Syrjälä
> >Sent: Tuesday, February 19, 2019 1:37 AM
> >To: Shankar, Uma <uma.shankar@intel.com>
> >Cc: intel-gfx@lists.freedesktop.org; Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst,
> >Maarten <maarten.lankhorst@intel.com>; dri-devel@lists.freedesktop.org
> >Subject: Re: [Intel-gfx] [v16 3/4] drm: Add colorspace info to AVI Infoframe
> >
> >On Wed, Feb 13, 2019 at 12:35:10PM +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.
> >>
> >> v6: Added bit wise macro for various fields of colorimetry for easier
> >> understanding and review as per Ville's comments. Moved the same out
> >> of header file to avoid any namespace issues.
> >>
> >> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> >> ---
> >>  drivers/gpu/drm/drm_edid.c | 66
> >++++++++++++++++++++++++++++++++++++++++++++++
> >>  include/drm/drm_edid.h     |  6 +++++
> >>  2 files changed, 72 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> >> index 990b190..64816c0 100644
> >> --- a/drivers/gpu/drm/drm_edid.c
> >> +++ b/drivers/gpu/drm/drm_edid.c
> >> @@ -4924,6 +4924,72 @@ static bool is_hdmi2_sink(struct drm_connector
> >> *connector)  }
> >> EXPORT_SYMBOL(drm_hdmi_avi_infoframe_from_display_mode);
> >>
> >> +/* 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 C(x) ((x) << 0)
> >> +#define EC(x) ((x) << 2)
> >> +#define ACE(x) ((x) << 5)
> >> +
> >> +#define HDMI_COLORIMETRY_NO_DATA		0x0
> >> +#define HDMI_COLORIMETRY_SMPTE_170M_YCC		(C(1) | EC(0) |
> >ACE(0))
> >> +#define HDMI_COLORIMETRY_BT709_YCC		(C(2) | EC(0) | ACE(0))
> >> +#define HDMI_COLORIMETRY_XVYCC_601		(C(3) | EC(0) | ACE(0))
> >> +#define HDMI_COLORIMETRY_XVYCC_709		(C(3) | EC(1) | ACE(0))
> >> +#define HDMI_COLORIMETRY_SYCC_601		(C(3) | EC(2) | ACE(0))
> >> +#define HDMI_COLORIMETRY_OPYCC_601		(C(3) | EC(3) | ACE(0))
> >> +#define HDMI_COLORIMETRY_OPRGB			(C(3) | EC(4) |
> >ACE(0))
> >> +#define HDMI_COLORIMETRY_BT2020_CYCC		(C(3) | EC(5) | ACE(0))
> >> +#define HDMI_COLORIMETRY_BT2020_RGB		(C(3) | EC(6) | ACE(0))
> >> +#define HDMI_COLORIMETRY_BT2020_YCC		(C(3) | EC(6) | ACE(0))
> >> +#define HDMI_COLORIMETRY_DCI_P3_RGB_D65		(C(3) | EC(7) |
> >ACE(0))
> >> +#define HDMI_COLORIMETRY_DCI_P3_RGB_THEATER	(C(3) | EC(7) |
> >ACE(1))
> >> +
> >> +static const u32 hdmi_colorimetry_val[] = {
> >> +	[DRM_MODE_COLORIMETRY_NO_DATA] =
> >HDMI_COLORIMETRY_NO_DATA,
> >> +	[DRM_MODE_COLORIMETRY_SMPTE_170M_YCC] =
> >HDMI_COLORIMETRY_SMPTE_170M_YCC,
> >> +	[DRM_MODE_COLORIMETRY_BT709_YCC] =
> >HDMI_COLORIMETRY_BT709_YCC,
> >> +	[DRM_MODE_COLORIMETRY_XVYCC_601] =
> >HDMI_COLORIMETRY_XVYCC_601,
> >> +	[DRM_MODE_COLORIMETRY_XVYCC_709] =
> >HDMI_COLORIMETRY_XVYCC_709,
> >> +	[DRM_MODE_COLORIMETRY_SYCC_601] =
> >HDMI_COLORIMETRY_SYCC_601,
> >> +	[DRM_MODE_COLORIMETRY_OPYCC_601] =
> >HDMI_COLORIMETRY_OPYCC_601,
> >> +	[DRM_MODE_COLORIMETRY_OPRGB] = HDMI_COLORIMETRY_OPRGB,
> >> +	[DRM_MODE_COLORIMETRY_BT2020_CYCC] =
> >HDMI_COLORIMETRY_BT2020_CYCC,
> >> +	[DRM_MODE_COLORIMETRY_BT2020_RGB] =
> >HDMI_COLORIMETRY_BT2020_RGB,
> >> +	[DRM_MODE_COLORIMETRY_BT2020_YCC] =
> >HDMI_COLORIMETRY_BT2020_YCC, };
> >
> >Probably best to
> >
> >#undef C
> >#undef EC
> >#undef ACE
> >
> >here to avoid accidents.
> 
> Sure will add that. So with this fixed and DP stuff dropped, can I mark your RB on all the remaining 2 patches
> (DP patch will get dropped). If you confirm will resend with your RB for merge.

Yeah, I think it should be good to go in.

> 
> Thanks & Regards,
> Uma Shankar
> 
> >With that
> >Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> 
> >> +
> >> +/**
> >> + * 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;
> >> +	u32 colorimetry_index = conn_state->colorspace &
> >> +FULL_COLORIMETRY_MASK;
> >> +
> >> +	if (colorimetry_index >= ARRAY_SIZE(hdmi_colorimetry_val))
> >> +		colorimetry_val = HDMI_COLORIMETRY_NO_DATA;
> >> +	else
> >> +		colorimetry_val = hdmi_colorimetry_val[colorimetry_index];
> >> +
> >> +	frame->colorimetry = colorimetry_val & NORMAL_COLORIMETRY_MASK;
> >> +	/*
> >> +	 * ToDo: Extend it for ACE formats as well. Modify the infoframe
> >> +	 * structure and extend it in drivers/video/hdmi
> >> +	 */
> >> +	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
> >> 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
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> >--
> >Ville Syrjälä
> >Intel
> >_______________________________________________
> >dri-devel mailing list
> >dri-devel@lists.freedesktop.org
> >https://lists.freedesktop.org/mailman/listinfo/dri-devel

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