Re: [PATCH 3/3] [VPG HSW-A] drm/i915: Populate all fields of AVI infoframe

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

On Thu, Aug 15, 2013 at 10:29:03AM +0530, vandana.kannan@intel.com wrote:
> From: vkannan <vandana.kannan@intel.com>
> 
> Populate bar information, colorimetry, IT content, quantization range fields
> of AVI infoframe based on CEA 861-D spec.
> 
> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_drv.h  |    4 ++++
>  drivers/gpu/drm/i915/intel_hdmi.c |   39 +++++++++++++++++++++++++++++++++++--
>  2 files changed, 41 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 97df85d..e02c442 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -412,6 +412,10 @@ struct cxsr_latency {
>  #define DIP_AVI_RGB_QUANT_RANGE_DEFAULT	(0 << 2)
>  #define DIP_AVI_RGB_QUANT_RANGE_LIMITED	(1 << 2)
>  #define DIP_AVI_RGB_QUANT_RANGE_FULL	(2 << 2)
> +#define DIP_AVI_IT_CONTENT     (1 << 7)
> +#define DIP_AVI_BAR_BOTH       (3 << 2)
> +#define DIP_AVI_COLOR_ITU601   (1 << 6)
> +#define DIP_AVI_COLOR_ITU709   (2 << 6)
>  
>  #define DIP_TYPE_SPD	0x83
>  #define DIP_VERSION_SPD	0x1
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 7cf6fc5..dbbc02b 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -340,9 +340,22 @@ static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
>  		.type = DIP_TYPE_AVI,
>  		.ver = DIP_VERSION_AVI,
>  		.len = DIP_LEN_AVI,
> +		.body.avi.Y_A_B_S = 0,
>  		.body.avi.C_M_R = 8,
> +		.body.avi.ITC_EC_Q_SC = 0,
> +		.body.avi.VIC = 0,
> +		.body.avi.YQ_CN_PR = 0,
> +		.body.avi.top_bar_end = 0,
> +		.body.avi.bottom_bar_start = 0,
> +		.body.avi.left_bar_end = 0,
> +		.body.avi.right_bar_start = 0,

This is not needed. All the members not explicitly initialized are
initialized to 0 implicitly.

>  	};
>  
> +	/* Bar information */
> +	avi_if.body.avi.Y_A_B_S |= DIP_AVI_BAR_BOTH;

Now you're sending bars left/right/top/bottom=0 bars. AFAICS that
would indicate that the entire picture is made of of the bottom and
right bars, ie. no actual content in the picture.

> +
> +	avi_if.body.avi.VIC = drm_match_cea_mode(adjusted_mode);
> +
>  	if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK)
>  		avi_if.body.avi.YQ_CN_PR |= DIP_AVI_PR_2;
>  
> @@ -351,10 +364,17 @@ static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
>  			avi_if.body.avi.ITC_EC_Q_SC |= DIP_AVI_RGB_QUANT_RANGE_LIMITED;
>  		else
>  			avi_if.body.avi.ITC_EC_Q_SC |= DIP_AVI_RGB_QUANT_RANGE_FULL;
> +	} else {
> +		/* Set full range quantization for non-CEA modes
> +		and 640x480 */
> +		if ((avi_if.body.avi.VIC == 0) || (avi_if.body.avi.VIC == 1))
> +			avi_if.body.avi.ITC_EC_Q_SC |=
> +				DIP_AVI_RGB_QUANT_RANGE_FULL;
> +		else
> +			avi_if.body.avi.ITC_EC_Q_SC |=
> +				DIP_AVI_RGB_QUANT_RANGE_LIMITED;
>  	}

The spec says we shouldn't send the quantization range unless the QS bit
in EDID CEA block is 1. Now you're sending it anyway. What's the deal?

>  
> -	avi_if.body.avi.VIC = drm_match_cea_mode(adjusted_mode);
> -
>  	/*If picture aspect ratio (PAR) is set to custom value, then use that,
>  	else if VIC > 1, then get PAR from CEA mode list, else, calculate
>  	PAR based on resolution */
> @@ -377,6 +397,21 @@ static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
>  			avi_if.body.avi.C_M_R |= HDMI_PICTURE_ASPECT_16_9 << 4;
>  	}
>  
> +	if (avi_if.body.avi.VIC) {
> +		/* colorimetry: Sections 5.1 and 5.2 of CEA 861-D spec */
> +		if ((adjusted_mode->vdisplay == 480) ||
> +			(adjusted_mode->vdisplay == 576) ||
> +			(adjusted_mode->vdisplay == 240) ||
> +			(adjusted_mode->vdisplay == 288)) {
> +			avi_if.body.avi.C_M_R |= DIP_AVI_COLOR_ITU601;
> +		} else if ((adjusted_mode->vdisplay == 720) ||
> +			(adjusted_mode->vdisplay == 1080)) {
> +			avi_if.body.avi.C_M_R |= DIP_AVI_COLOR_ITU709;
> +		}
> +	}

We're outputting RGB data only, so we should not set the colorimetry bits.
When we decide to implement better colorspace support, we're again going
to need some new properties to let the user set the colorimetry
appropriately.

> +
> +	avi_if.body.avi.ITC_EC_Q_SC |= DIP_AVI_IT_CONTENT;
> +
>  	intel_set_infoframe(encoder, &avi_if);
>  }
>  
> -- 
> 1.7.9.5
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC