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

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

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

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