On Thu, Jan 17, 2013 at 10:16:46AM -0200, Paulo Zanoni wrote:
> Hi
>
> 2013/1/16 <ville.syrjala at linux.intel.com>:
> > From: Ville Syrj?l? <ville.syrjala at linux.intel.com>
> >
> > 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?l? <ville.syrjala at linux.intel.com>
> > ---
> > 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/intel_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
>
> <bikeshedding optional="true">
>
> 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
> <bikeshedding optional="true">
>
> Everything else looks fine.
>
> With or without that:
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
>
> 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/intel_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 *adjusted_mode)
> > {
> > + struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
> > struct dip_infoframe avi_if = {
> > .type = DIP_TYPE_AVI,
> > .ver = DIP_VERSION_AVI,
> > @@ -340,6 +341,13 @@ static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
> > if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK)
> > avi_if.body.avi.YQ_CN_PR |= DIP_AVI_PR_2;
> >
> > + if (intel_hdmi->rgb_quant_range_selectable) {
> > + if (adjusted_mode->private_flags & INTEL_MODE_LIMITED_COLOR_RANGE)
> > + avi_if.body.avi.ITC_EC_Q_SC |= DIP_AVI_Q0;
> > + else
> > + avi_if.body.avi.ITC_EC_Q_SC |= DIP_AVI_Q1;
> > + }
> > +
> > avi_if.body.avi.VIC = 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 = false;
> > intel_hdmi->has_audio = false;
> > + intel_hdmi->rgb_quant_range_selectable = false;
> > edid = 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 =
> > drm_detect_hdmi_monitor(edid);
> > intel_hdmi->has_audio = drm_detect_monitor_audio(edid);
> > + intel_hdmi->rgb_quant_range_selectable =
> > + drm_rgb_quant_range_selectable(edid);
> > }
> > kfree(edid);
> > }
> > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_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 = {
> > .type = DIP_TYPE_AVI,
> > @@ -956,6 +958,13 @@ static bool intel_sdvo_set_avi_infoframe(struct intel_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_COLOR_RANGE)
> > + avi_if.body.avi.ITC_EC_Q_SC |= DIP_AVI_Q0;
> > + else
> > + avi_if.body.avi.ITC_EC_Q_SC |= DIP_AVI_Q1;
> > + }
> > +
> > intel_dip_infoframe_csum(&avi_if);
> >
> > /* sdvo spec says that the ecc is handled by the hw, and it looks like
> > @@ -1134,7 +1143,7 @@ static void intel_sdvo_mode_set(struct drm_encoder *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 = drm_detect_hdmi_monitor(edid);
> > intel_sdvo->has_hdmi_audio = drm_detect_monitor_audio(edid);
> > + intel_sdvo->rgb_quant_range_selectable =
> > + drm_rgb_quant_range_selectable(edid);
> > }
> > } else
> > status = connector_status_disconnected;
> > @@ -1577,6 +1588,7 @@ intel_sdvo_detect(struct drm_connector *connector, bool force)
> >
> > intel_sdvo->has_hdmi_monitor = false;
> > intel_sdvo->has_hdmi_audio = false;
> > + intel_sdvo->rgb_quant_range_selectable = false;
> >
> > if ((intel_sdvo_connector->output_flag & response) == 0)
> > ret = connector_status_disconnected;
> > --
> > 1.7.8.6
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
>
> --
> Paulo Zanoni
--
Ville Syrj?l?
Intel OTC
