Re: [PATCH] i915: enable AVI infoframe for intel_hdmi.c [v2]

["David Härdeman" <david@hardeman.nu>]

On Thu, Sep 09, 2010 at 11:00:01PM +0200, David Härdeman 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).

Just to clarify that last sentence a bit...

intel_sdvo.c currently defines this struct:

struct dip_infoframe {
	uint8_t type;
	uint8_t version;
	uint8_t len;
	uint8_t checksum;
	union {
		...
		uint8_t payload[28];
	] __attribute__ ((packed)) u;
} __attribute__((packed));

If the "checksum" member refers to the body checksum of the infoframe, 
then the payload size is incorrect. The checksum is the first byte of 
the body which is 28 bytes in total (HDMI spec 1.3a, table 5-15) so the 
payload array should be 27 bytes. If the payload size is corrected to 27 
bytes, then the size of the entire struct is 31 bytes and the writing 
(which is done in 8 byte chunks in intel_sdvo.c) needs to be fixed.

If, on the other hand, the checksum field refers to the header ecc 
checksum, then the payload member is correctly sized but the body 
checksum is missing.

My guess is that the sdvo hardware has the same setup as described in 
http://intellinuxgraphics.org/VOL_3_display_registers_updated.pdf, 
section 2.8.9, meaning that the correct struct would be:

struct dip_infoframe {
	uint8_t type;
	uint8_t version;
	uint8_t len;
	uint8_t ecc;
	uint8_t checksum;
	union {
		...
		uint8_t payload[27];
	] __attribute__ ((packed)) u;
} __attribute__((packed));

Which would give a 32 byte struct which can be written in 8-byte chunks 
and which has the correct body size.

However, I can't confirm that suspicion...see:
http://software.intel.com/en-us/forums/showannouncement.php?a=17

Could someone at Intel please clarify?

On a related note, the struct currently defined in intel_sdvo.c relies 
on bitfields and expect the bits to be encoded in a certain order (which 
is the reason a casual reader might get the impression that the order of 
the members of the struct is reversed when compared to the relevant 
specs). However, the order of the bits within a unit is 
implementation-defined (C99, 6.7.2.1, point 10), so it shouldn't be 
relied upon...which is why I didn't use bitfields in my patch.

-- 
David Härdeman