Re: [RFC v2 5/5] drm/i915: Use generic HDMI infoframe helpers

[Thierry Reding <thierry.reding@avionic-design.de>]

[-- Attachment #1.1: Type: text/plain, Size: 2583 bytes --]

On Thu, Dec 06, 2012 at 03:16:31PM +0100, Daniel Vetter wrote:
> On Wed, Dec 05, 2012 at 05:45:44PM +0100, Thierry Reding wrote:
> > Use the generic HDMI infoframe helpers to get rid of the duplicate
> > implementation in the i915 driver.
> > 
> > Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
> 
> > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> > index 4b07401..192d791 100644
> > --- a/drivers/gpu/drm/i915/intel_sdvo.c
> > +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> > @@ -29,6 +29,7 @@
> >  #include <linux/slab.h>
> >  #include <linux/delay.h>
> >  #include <linux/export.h>
> > +#include <linux/hdmi.h>
> >  #include <drm/drmP.h>
> >  #include <drm/drm_crtc.h>
> >  #include <drm/drm_edid.h>
> > @@ -935,20 +936,17 @@ static bool intel_sdvo_write_infoframe(struct intel_sdvo *intel_sdvo,
> >  
> >  static bool intel_sdvo_set_avi_infoframe(struct intel_sdvo *intel_sdvo)
> >  {
> > -	struct dip_infoframe avi_if = {
> > -		.type = DIP_TYPE_AVI,
> > -		.ver = DIP_VERSION_AVI,
> > -		.len = DIP_LEN_AVI,
> > -	};
> > -	uint8_t sdvo_data[4 + sizeof(avi_if.body.avi)];
> > +	uint8_t sdvo_data[HDMI_AVI_INFOFRAME_SIZE];
> > +	struct hdmi_avi_infoframe frame;
> > +	ssize_t len;
> >  
> > -	intel_dip_infoframe_csum(&avi_if);
> > +	len = hdmi_avi_infoframe_init(&frame);
> > +	if (len < 0)
> > +		return false;
> >  
> > -	/* sdvo spec says that the ecc is handled by the hw, and it looks like
> > -	 * we must not send the ecc field, either. */
> > -	memcpy(sdvo_data, &avi_if, 3);
> > -	sdvo_data[3] = avi_if.checksum;
> 
> This difference between how we need to send the infoframe to the hw
> between sdvo (without ecc field present) and native hdmi (ecc field
> present, but still computed by the hw) seems to have been lost. On a quick
> read from your infoframe packer it looks like the ecc field isn't part of
> the packed infoframe, so this patch would break native hdmi ports (in the
> intel_hdmi.c file).

Actually this ECC field seems to be something specific to Intel
hardware. It doesn't appear in the HDMI specification nor in CEA-861-D.
So it the binary infoframe representation doesn't contain it, therefore
it should be handled by the respective drivers. So in fact the code
should be working fine for HDMI and SDVO, since for the HDMI ports I
explicitly left the fourth byte of the header empty, while for SDVO, the
checksum is the one as computed by hdmi_avi_infoframe_pack(), which is
at byte 3 and therefore matches the previous code.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel