[Intel-gfx] [PATCH] i915: enable AVI infoframe for intel_hdmi.c [v2]

[Chris Wilson]

On Thu, 09 Sep 2010 23:00:01 +0200, David Härdeman <david at hardeman.nu> wrote:
> This is the second version which merges the infoframe code from
> intel_hdmi.c and intel_sdvo.c, I hope this is something along the lines
> Chris Wilson had in mind. Note that I'm assuming that the sdvo hardware
> also stores a header ECC byte in the MSB of the first dword (haven't found
> any documentation for the sdvo).

Pretty close. Using a callback function for writing the data is overkill,
just call the common function to compute the avi csum, then pass the avi
to the sdvo or hdmi handlers. It just makes following the logic that
little bit easier.

drm-intel-next has changed slightly since you wrote the patch, so can you
respin it - just a couple of the macro names have changed to be
consistent.

> ---
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index ccd4c97..4fc323c 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -48,6 +48,79 @@ static struct intel_hdmi *enc_to_intel_hdmi(struct drm_encoder *encoder)
>  	return container_of(enc_to_intel_encoder(encoder), struct intel_hdmi, base);
>  }
>  
> +static void intel_hdmi_calc_infoframe_csum(struct dip_infoframe *avi_if)
void intel_dip_infoframe_csum(struct dip_infoframe *avi_if)

> +{
> +	uint8_t *data = (uint8_t *)avi_if;
> +	uint8_t sum = 0;
> +	unsigned i;
> +
> +	avi_if->checksum = 0;
> +	avi_if->ecc = 0;
> +	avi_if->padding = 0;
> +
> +	for (i = 0; i < sizeof(*avi_if); i++)
> +		sum += data[i];
> +
> +	avi_if->checksum = 0x100 - sum;
> +}
> +
> +static bool intel_hdmi_write_avi_infoframe(struct drm_encoder *encoder,
> +					   uint8_t *data)
> +{
> +	struct drm_device *dev = encoder->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	I915_WRITE(VIDEO_DIP_DATA, *((u32 *)data));
> +	return true;
> +}
> +
> +bool intel_hdmi_proc_infoframe(struct drm_encoder *encoder,
> +			    struct dip_infoframe *avi_if,
> +			    bool (*write_func)(struct drm_encoder *, uint8_t *))
> +{
> +	unsigned size;
> +	uint8_t *data = (uint8_t *)avi_if;
> +
> +	intel_hdmi_calc_infoframe_csum(avi_if);
> +	for (size = sizeof(*avi_if); size > 0; size -= 4) {
> +		if (!write_func(encoder, data))
> +			return false;
> +		data += 4;
> +	}
> +	return true;
> +}
This I think is unnecessary, especially as the current SDVO code handles
it in 8 byte chunks.

> +
> +static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder)
> +{
> +	struct dip_infoframe avi_if = {
> +		.type = DIP_TYPE_AVI,
> +		.ver = DIP_VERSION_AVI,
> +		.len = DIP_LEN_AVI,
> +		.Y_A_B_S = 0x02,
> +		.C_M_R = 0x28,
> +	};
> +	struct drm_device *dev = encoder->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
> +	u32 port;
> +
> +	if (!intel_hdmi->has_hdmi_sink)
> +		return;
> +
> +	if (intel_hdmi->sdvox_reg == SDVOB)
> +		port = VIDEO_DIP_PORT_B;
> +	else if (intel_hdmi->sdvox_reg == SDVOC)
> +		port = VIDEO_DIP_PORT_C;
> +	else
> +		return;
> +
> +	I915_WRITE(VIDEO_DIP_CTL, VIDEO_DIP_ENABLE | port | VIDEO_DIP_SELECT_AVI | VIDEO_DIP_FREQ_VSYNC);
Split this up over multiple lines to fit within 80-columns.

> +	POSTING_READ(VIDEO_DIP_ENABLE);
Shouldn't need to flush the write here.

> +	intel_hdmi_proc_infoframe(encoder, &avi_if, intel_hdmi_write_avi_infoframe);
I think this works better as:
	intel_dip_infofame_csum(&avi_if);
	intel_hdmi_write_dip_infoframe(encoder, &avi_if);

> +	I915_WRITE(VIDEO_DIP_CTL, VIDEO_DIP_ENABLE | port | VIDEO_DIP_ENABLE_AVI | VIDEO_DIP_SELECT_AVI | VIDEO_DIP_FREQ_VSYNC);
> +	POSTING_READ(VIDEO_DIP_ENABLE);
Same as above.


Thanks, adding a second copy of that data structure would have been too
ugly.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre