From mboxrd@z Thu Jan 1 00:00:00 1970 From: David =?iso-8859-1?Q?H=E4rdeman?= Subject: Re: [PATCH] i915: enable AVI infoframe for intel_hdmi.c [v2] Date: Sat, 11 Sep 2010 00:04:06 +0200 Message-ID: <20100910220406.GA16734@hardeman.nu> References: <20100909210001.11985.1244.stgit@localhost.localdomain> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from palpatine.hardeman.nu (1-1-12-13a.han.sth.bostream.se [82.182.30.168]) by gabe.freedesktop.org (Postfix) with ESMTP id B764F9E7BC for ; Fri, 10 Sep 2010 15:04:10 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20100909210001.11985.1244.stgit@localhost.localdomain> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: intel-gfx@lists.freedesktop.org Cc: jesse.barnes@intel.com List-Id: intel-gfx@lists.freedesktop.org On Thu, Sep 09, 2010 at 11:00:01PM +0200, David H=E4rdeman 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=3D17 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=E4rdeman