From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 4/4] drm/i915: Provide the quantization range in the AVI infoframe Date: Thu, 17 Jan 2013 14:37:54 +0200 Message-ID: <20130117123754.GR3503@intel.com> References: <1358356365-23191-1-git-send-email-ville.syrjala@linux.intel.com> <1358356365-23191-5-git-send-email-ville.syrjala@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Paulo Zanoni Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org On Thu, Jan 17, 2013 at 10:16:46AM -0200, Paulo Zanoni wrote: > Hi > = > 2013/1/16 : > > From: Ville Syrj=E4l=E4 > > > > The AVI infoframe is able to inform the display whether the source is > > sending full or limited range RGB data. > > > > As per CEA-861 we must first check whether the display reports the > > quantization range as selectable, and if so we can set the approriate > > bits in the AVI inforframe. > > > > Signed-off-by: Ville Syrj=E4l=E4 > > --- > > drivers/gpu/drm/i915/intel_drv.h | 3 +++ > > drivers/gpu/drm/i915/intel_hdmi.c | 11 +++++++++++ > > drivers/gpu/drm/i915/intel_sdvo.c | 16 ++++++++++++++-- > > 3 files changed, 28 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/in= tel_drv.h > > index 1a698c6..c5251d9 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -289,6 +289,8 @@ struct cxsr_latency { > > #define DIP_LEN_AVI 13 > > #define DIP_AVI_PR_1 0 > > #define DIP_AVI_PR_2 1 > > +#define DIP_AVI_Q0 0x4 > > +#define DIP_AVI_Q1 0x8 > = > > = > I'd define more descriptive names here... How about the following? > #define DIP_AVI_QUANTIZATION_DEFAULT (0 << 2) > #define DIP_AVI_QUANTIZATION_LIMITED (1 << 2) > #define DIP_AVI_QUANTIZATION_FULL (2 << 2) Sure, that seems like sensible idea. I think I'll want the fact that these only refer to RGB to be included in the name though. So something like this probably: DIP_AVI_RGB_QUANT_RANGE_DEFAULT DIP_AVI_RGB_QUANT_RANGE_LIMITED DIP_AVI_RGB_QUANT_RANGE_FULL > > = > Everything else looks fine. > = > With or without that: > Reviewed-by: Paulo Zanoni > = > Also, these things kinda conflict with Thierry's patches, but I know > you're aware because you've reviewed some of them :) Yeah. BTW are we (as in i915 folks) mostly OK with those? > > #define DIP_TYPE_SPD 0x83 > > #define DIP_VERSION_SPD 0x1 > > @@ -347,6 +349,7 @@ struct intel_hdmi { > > bool has_hdmi_sink; > > bool has_audio; > > enum hdmi_force_audio force_audio; > > + bool rgb_quant_range_selectable; > > void (*write_infoframe)(struct drm_encoder *encoder, > > struct dip_infoframe *frame); > > void (*set_infoframes)(struct drm_encoder *encoder, > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/i= ntel_hdmi.c > > index 58b072e..270d7ee 100644 > > --- a/drivers/gpu/drm/i915/intel_hdmi.c > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > > @@ -331,6 +331,7 @@ static void intel_set_infoframe(struct drm_encoder = *encoder, > > static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder, > > struct drm_display_mode *adjus= ted_mode) > > { > > + struct intel_hdmi *intel_hdmi =3D enc_to_intel_hdmi(encoder); > > struct dip_infoframe avi_if =3D { > > .type =3D DIP_TYPE_AVI, > > .ver =3D DIP_VERSION_AVI, > > @@ -340,6 +341,13 @@ static void intel_hdmi_set_avi_infoframe(struct dr= m_encoder *encoder, > > if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK) > > avi_if.body.avi.YQ_CN_PR |=3D DIP_AVI_PR_2; > > > > + if (intel_hdmi->rgb_quant_range_selectable) { > > + if (adjusted_mode->private_flags & INTEL_MODE_LIMITED_C= OLOR_RANGE) > > + avi_if.body.avi.ITC_EC_Q_SC |=3D DIP_AVI_Q0; > > + else > > + avi_if.body.avi.ITC_EC_Q_SC |=3D DIP_AVI_Q1; > > + } > > + > > avi_if.body.avi.VIC =3D drm_mode_cea_vic(adjusted_mode); > > > > intel_set_infoframe(encoder, &avi_if); > > @@ -825,6 +833,7 @@ intel_hdmi_detect(struct drm_connector *connector, = bool force) > > > > intel_hdmi->has_hdmi_sink =3D false; > > intel_hdmi->has_audio =3D false; > > + intel_hdmi->rgb_quant_range_selectable =3D false; > > edid =3D drm_get_edid(connector, > > intel_gmbus_get_adapter(dev_priv, > > intel_hdmi->ddc_bus= )); > > @@ -836,6 +845,8 @@ intel_hdmi_detect(struct drm_connector *connector, = bool force) > > intel_hdmi->has_hdmi_sink =3D > > drm_detect_hdmi_monitor= (edid); > > intel_hdmi->has_audio =3D drm_detect_monitor_au= dio(edid); > > + intel_hdmi->rgb_quant_range_selectable =3D > > + drm_rgb_quant_range_selectable(edid); > > } > > kfree(edid); > > } > > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/i= ntel_sdvo.c > > index b422109..af93999 100644 > > --- a/drivers/gpu/drm/i915/intel_sdvo.c > > +++ b/drivers/gpu/drm/i915/intel_sdvo.c > > @@ -126,6 +126,7 @@ struct intel_sdvo { > > bool is_hdmi; > > bool has_hdmi_monitor; > > bool has_hdmi_audio; > > + bool rgb_quant_range_selectable; > > > > /** > > * This is set if we detect output of sdvo device as LVDS and > > @@ -947,7 +948,8 @@ static bool intel_sdvo_write_infoframe(struct intel= _sdvo *intel_sdvo, > > &tx_rate, 1); > > } > > > > -static bool intel_sdvo_set_avi_infoframe(struct intel_sdvo *intel_sdvo) > > +static bool intel_sdvo_set_avi_infoframe(struct intel_sdvo *intel_sdvo, > > + const struct drm_display_mode = *adjusted_mode) > > { > > struct dip_infoframe avi_if =3D { > > .type =3D DIP_TYPE_AVI, > > @@ -956,6 +958,13 @@ static bool intel_sdvo_set_avi_infoframe(struct in= tel_sdvo *intel_sdvo) > > }; > > uint8_t sdvo_data[4 + sizeof(avi_if.body.avi)]; > > > > + if (intel_sdvo->rgb_quant_range_selectable) { > > + if (adjusted_mode->private_flags & INTEL_MODE_LIMITED_C= OLOR_RANGE) > > + avi_if.body.avi.ITC_EC_Q_SC |=3D DIP_AVI_Q0; > > + else > > + avi_if.body.avi.ITC_EC_Q_SC |=3D DIP_AVI_Q1; > > + } > > + > > intel_dip_infoframe_csum(&avi_if); > > > > /* sdvo spec says that the ecc is handled by the hw, and it loo= ks like > > @@ -1134,7 +1143,7 @@ static void intel_sdvo_mode_set(struct drm_encode= r *encoder, > > intel_sdvo_set_encode(intel_sdvo, SDVO_ENCODE_HDMI); > > intel_sdvo_set_colorimetry(intel_sdvo, > > SDVO_COLORIMETRY_RGB256); > > - intel_sdvo_set_avi_infoframe(intel_sdvo); > > + intel_sdvo_set_avi_infoframe(intel_sdvo, adjusted_mode); > > } else > > intel_sdvo_set_encode(intel_sdvo, SDVO_ENCODE_DVI); > > > > @@ -1526,6 +1535,8 @@ intel_sdvo_tmds_sink_detect(struct drm_connector = *connector) > > if (intel_sdvo->is_hdmi) { > > intel_sdvo->has_hdmi_monitor =3D drm_de= tect_hdmi_monitor(edid); > > intel_sdvo->has_hdmi_audio =3D drm_dete= ct_monitor_audio(edid); > > + intel_sdvo->rgb_quant_range_selectable = =3D > > + drm_rgb_quant_range_selectable(= edid); > > } > > } else > > status =3D connector_status_disconnected; > > @@ -1577,6 +1588,7 @@ intel_sdvo_detect(struct drm_connector *connector= , bool force) > > > > intel_sdvo->has_hdmi_monitor =3D false; > > intel_sdvo->has_hdmi_audio =3D false; > > + intel_sdvo->rgb_quant_range_selectable =3D false; > > > > if ((intel_sdvo_connector->output_flag & response) =3D=3D 0) > > ret =3D connector_status_disconnected; > > -- > > 1.7.8.6 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/dri-devel > = > = > = > -- = > Paulo Zanoni -- = Ville Syrj=E4l=E4 Intel OTC