[Intel-gfx] [PATCH 5/8] drm/i915/hdmi: Port the infoframe code to the common hdmi helpers

[Damien Lespiau]

On Mon, Aug 05, 2013 at 04:12:11PM +0300, Ville Syrjälä wrote:
> >  static void intel_set_infoframe(struct drm_encoder *encoder,
> > -				struct dip_infoframe *frame)
> > +				union hdmi_infoframe *frame)
> >  {
> >  	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
> > +	uint8_t buffer[DIP_BUFFER_SIZE];
> > +	ssize_t len;
> > +
> > +	memset(buffer, 0, sizeof(buffer));
> 
> Could just zero initialize 'buffer' when declaring it. However
> hdmi_*_infoframe_pack() already contain a memset(), so we shouldn't even
> need to zero it here.

There was a good, admittedly well hidden, reason for this one. _pack()
only zeroes up to the size of the infoframe and _write_infoframe() makes
sure sure to write 0s dwords after the infoframe (rounded up to a
multiple of 4) for the checksum. Without it we're left with
4 - (infoframe_size % 4) bytes of garbage in the infoframe data.

The new series makes the hdmi _pack helpers zero the whole incoming
buffer instead, I think it makes sense.

-- 
Damien

> 
> > +
> > +	/* see comment above for the reason for this offset */
> > +	len = hdmi_infoframe_pack(frame, buffer + 1, sizeof(buffer) - 1);
> > +	if (len < 0)
> > +		return;
> >  
> > -	intel_dip_infoframe_csum(frame);
> > -	intel_hdmi->write_infoframe(encoder, frame->type, (uint8_t *)frame,
> > -				    DIP_HEADER_SIZE + frame->len);
> > +	/* Insert the 'hole' (see big comment above) at position 3 */
> > +	buffer[0] = buffer[1];
> > +	buffer[1] = buffer[2];
> > +	buffer[2] = buffer[3];
> > +	buffer[3] = 0;
> > +	len++;
> > +
> > +	intel_hdmi->write_infoframe(encoder, frame->any.type, buffer, len);
> >  }
> >  
> >  static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
> > @@ -343,40 +384,45 @@ static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
> >  {
> >  	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
> > -	struct dip_infoframe avi_if = {
> > -		.type = DIP_TYPE_AVI,
> > -		.ver = DIP_VERSION_AVI,
> > -		.len = DIP_LEN_AVI,
> > -	};
> > +	union hdmi_infoframe frame;
> > +	int ret;
> > +
> > +	ret = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi,
> > +						       adjusted_mode);
> > +	if (ret < 0) {
> > +		DRM_ERROR("couldn't fill AVI infoframe\n");
> > +		return;
> > +	}
> >  
> >  	if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK)
> > -		avi_if.body.avi.YQ_CN_PR |= DIP_AVI_PR_2;
> > +		frame.avi.pixel_repeat = 1;
> 
> Perhaps that should be part of drm_hdmi_avi_infoframe_from_display_mode()?
> 
> >  
> >  	if (intel_hdmi->rgb_quant_range_selectable) {
> >  		if (intel_crtc->config.limited_color_range)
> > -			avi_if.body.avi.ITC_EC_Q_SC |= DIP_AVI_RGB_QUANT_RANGE_LIMITED;
> > +			frame.avi.quantization_range =
> > +				HDMI_QUANTIZATION_RANGE_LIMITED;
> >  		else
> > -			avi_if.body.avi.ITC_EC_Q_SC |= DIP_AVI_RGB_QUANT_RANGE_FULL;
> > +			frame.avi.quantization_range =
> > +				HDMI_QUANTIZATION_RANGE_FULL;
> >  	}
> >  
> > -	avi_if.body.avi.VIC = drm_match_cea_mode(adjusted_mode);
> > -
> > -	intel_set_infoframe(encoder, &avi_if);
> > +	intel_set_infoframe(encoder, &frame);
> >  }
> >  
> >  static void intel_hdmi_set_spd_infoframe(struct drm_encoder *encoder)
> >  {
> > -	struct dip_infoframe spd_if;
> > +	union hdmi_infoframe frame;
> > +	int ret;
> > +
> > +	ret = hdmi_spd_infoframe_init(&frame.spd, "Intel", "Integrated gfx");
> > +	if (ret < 0) {
> > +		DRM_ERROR("couldn't fill SPD infoframe\n");
> > +		return;
> > +	}
> >  
> > -	memset(&spd_if, 0, sizeof(spd_if));
> > -	spd_if.type = DIP_TYPE_SPD;
> > -	spd_if.ver = DIP_VERSION_SPD;
> > -	spd_if.len = DIP_LEN_SPD;
> > -	strcpy(spd_if.body.spd.vn, "Intel");
> > -	strcpy(spd_if.body.spd.pd, "Integrated gfx");
> > -	spd_if.body.spd.sdi = DIP_SPD_PC;
> > +	frame.spd.sdi = HDMI_SPD_SDI_PC;
> >  
> > -	intel_set_infoframe(encoder, &spd_if);
> > +	intel_set_infoframe(encoder, &frame);
> >  }
> >  
> >  static void g4x_set_infoframes(struct drm_encoder *encoder,
> > -- 
> > 1.8.3.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC