On Sat, May 12, 2012 at 08:22:00PM +0200, Daniel Vetter wrote: > At least the worst offenders: > - SDVO specifies that the encoder should compute the ecc. Testing also > shows that we must not send the ecc field, so copy the dip_infoframe > struct to a temporay place and avoid the ecc field. This way the avi > infoframe is exactly 17 bytes long, which agrees with what the spec > mandates as a minimal storage capacity (with the ecc field it would > be 18 bytes). > - Only 17 when sending the avi infoframe. The SDVO spec explicitly > says that sending more data than what the device announces results > in undefined behaviour. > - Add __attribute__((packed)) to the avi and spd infoframes, for > otherwise they're wrongly aligned. Noticed because the avi infoframe > ended up being 18 bytes large instead of 17. We haven't noticed this > yet because we don't use the uint16_t fields yet (which are the only > ones that would be wrongly aligned). > > This regression has been introduce by > > 3c17fe4b8f40a112a85758a9ab2aebf772bdd647 is the first bad commit > commit 3c17fe4b8f40a112a85758a9ab2aebf772bdd647 > Author: David H?rdeman <david at hardeman.nu> > Date: Fri Sep 24 21:44:32 2010 +0200 > > i915: enable AVI infoframe for intel_hdmi.c [v4] > > Patch tested on my g33 with a sdvo hdmi adaptor. > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=25732 > Signed-Off-by: Daniel Vetter <daniel.vetter at ffwll.ch> Forwarded from the bug entry: Tested-by: Peter Ross <pross at xvid.org> (G35 SDVO-HDMI) > --- > drivers/gpu/drm/i915/intel_drv.h | 4 ++-- > drivers/gpu/drm/i915/intel_sdvo.c | 11 +++++++++-- > 2 files changed, 11 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index d51f27f..3e09188 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -280,12 +280,12 @@ struct dip_infoframe { > uint16_t bottom_bar_start; > uint16_t left_bar_end; > uint16_t right_bar_start; > - } avi; > + } __attribute__ ((packed)) avi; > struct { > uint8_t vn[8]; > uint8_t pd[16]; > uint8_t sdi; > - } spd; > + } __attribute__ ((packed)) spd; > uint8_t payload[27]; > } __attribute__ ((packed)) body; > } __attribute__((packed)); > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c > index 7d3f238..125228e 100644 > --- a/drivers/gpu/drm/i915/intel_sdvo.c > +++ b/drivers/gpu/drm/i915/intel_sdvo.c > @@ -887,17 +887,24 @@ static bool intel_sdvo_set_avi_infoframe(struct intel_sdvo *intel_sdvo) > }; > uint8_t tx_rate = SDVO_HBUF_TX_VSYNC; > uint8_t set_buf_index[2] = { 1, 0 }; > - uint64_t *data = (uint64_t *)&avi_if; > + uint8_t sdvo_data[4 + sizeof(avi_if.body.avi)]; > + uint64_t *data = (uint64_t *)sdvo_data; > unsigned i; > > intel_dip_infoframe_csum(&avi_if); > > + /* 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; > + memcpy(&sdvo_data[4], &avi_if.body, sizeof(avi_if.body.avi)); > + > if (!intel_sdvo_set_value(intel_sdvo, > SDVO_CMD_SET_HBUF_INDEX, > set_buf_index, 2)) > return false; > > - for (i = 0; i < sizeof(avi_if); i += 8) { > + for (i = 0; i < sizeof(sdvo_data); i += 8) { > if (!intel_sdvo_set_value(intel_sdvo, > SDVO_CMD_SET_HBUF_DATA, > data, 8)) > -- > 1.7.8.3 > -- Daniel Vetter Mail: daniel at ffwll.ch Mobile: +41 (0)79 365 57 48
