Re: [PATCH] drm/i915: make sure we write all the DIP data bytes

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Sep 25, 2012 at 02:06:30PM -0300, Rodrigo Vivi wrote:
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> 
> On Tue, Sep 25, 2012 at 1:23 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >
> > ... even if the actual infoframe is smaller than the maximum possible
> > size.
> >
> > If we don't write all the 32 DIP data bytes the InfoFrame ECC may not
> > be correctly calculated in some cases (e.g., when changing the port),
> > and this will lead to black screens on HDMI monitors. The ECC value is
> > generated by the hardware.
> >
> > I don't see how this should break anything since we're writing 0 and
> > that should be the correct value, so this patch should be safe.
> >
> > Notice that on IVB and older we actually have 64 bytes available for
> > VIDEO_DIP_DATA, but only bytes 0-31 actually store infoframe data: the
> > others are either read-only ECC values or marked as "reserved". On HSW
> > we only have 32 bytes, and the ECC value is stored on its own separate
> > read-only register. See BSpec.
> >
> > This patch fixes bug #46761, which is marked as a regression
> > introduced by commit 4e89ee174bb2da341bf90a84321c7008a3c9210d:
> >     drm/i915: set the DIP port on ibx_write_infoframe
> >
> > Before commit 4e89 we were just failing to send AVI infoframes when we
> > needed to change the port, which can lead to black screens in some
> > cases. After commit 4e89 we started sending infoframes, but with a
> > possibly wrong ECC value. After this patch I hope we start sending
> > correct infoframes.
> >
> > Version 2:
> >   - Improve commit message
> >   - Try to make the code more clear
> >
> > Cc: stable@vger.kernel.org
> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=46761
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Applied to -fixes, although I guess I'll push this through 3.7 -
infoframes blow up too often.
-Daniel
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h   |    4 ++++
> >  drivers/gpu/drm/i915/intel_hdmi.c |   15 +++++++++++++++
> >  2 files changed, 19 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index a828e90..7637824 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -1794,6 +1794,10 @@
> >
> >  /* Video Data Island Packet control */
> >  #define VIDEO_DIP_DATA         0x61178
> > +/* Read the description of VIDEO_DIP_DATA (before Haswel) or VIDEO_DIP_ECC
> > + * (Haswell and newer) to see which VIDEO_DIP_DATA byte corresponds to each byte
> > + * of the infoframe structure specified by CEA-861. */
> > +#define   VIDEO_DIP_DATA_SIZE  32
> >  #define VIDEO_DIP_CTL          0x61170
> >  /* Pre HSW: */
> >  #define   VIDEO_DIP_ENABLE             (1 << 31)
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> > index f9fb47c..08f2b63 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -151,6 +151,9 @@ static void g4x_write_infoframe(struct drm_encoder *encoder,
> >                 I915_WRITE(VIDEO_DIP_DATA, *data);
> >                 data++;
> >         }
> > +       /* Write every possible data byte to force correct ECC calculation. */
> > +       for (; i < VIDEO_DIP_DATA_SIZE; i += 4)
> > +               I915_WRITE(VIDEO_DIP_DATA, 0);
> >         mmiowb();
> >
> >         val |= g4x_infoframe_enable(frame);
> > @@ -186,6 +189,9 @@ static void ibx_write_infoframe(struct drm_encoder *encoder,
> >                 I915_WRITE(TVIDEO_DIP_DATA(intel_crtc->pipe), *data);
> >                 data++;
> >         }
> > +       /* Write every possible data byte to force correct ECC calculation. */
> > +       for (; i < VIDEO_DIP_DATA_SIZE; i += 4)
> > +               I915_WRITE(TVIDEO_DIP_DATA(intel_crtc->pipe), 0);
> >         mmiowb();
> >
> >         val |= g4x_infoframe_enable(frame);
> > @@ -224,6 +230,9 @@ static void cpt_write_infoframe(struct drm_encoder *encoder,
> >                 I915_WRITE(TVIDEO_DIP_DATA(intel_crtc->pipe), *data);
> >                 data++;
> >         }
> > +       /* Write every possible data byte to force correct ECC calculation. */
> > +       for (; i < VIDEO_DIP_DATA_SIZE; i += 4)
> > +               I915_WRITE(TVIDEO_DIP_DATA(intel_crtc->pipe), 0);
> >         mmiowb();
> >
> >         val |= g4x_infoframe_enable(frame);
> > @@ -259,6 +268,9 @@ static void vlv_write_infoframe(struct drm_encoder *encoder,
> >                 I915_WRITE(VLV_TVIDEO_DIP_DATA(intel_crtc->pipe), *data);
> >                 data++;
> >         }
> > +       /* Write every possible data byte to force correct ECC calculation. */
> > +       for (; i < VIDEO_DIP_DATA_SIZE; i += 4)
> > +               I915_WRITE(VLV_TVIDEO_DIP_DATA(intel_crtc->pipe), 0);
> >         mmiowb();
> >
> >         val |= g4x_infoframe_enable(frame);
> > @@ -292,6 +304,9 @@ static void hsw_write_infoframe(struct drm_encoder *encoder,
> >                 I915_WRITE(data_reg + i, *data);
> >                 data++;
> >         }
> > +       /* Write every possible data byte to force correct ECC calculation. */
> > +       for (; i < VIDEO_DIP_DATA_SIZE; i += 4)
> > +               I915_WRITE(data_reg + i, 0);
> >         mmiowb();
> >
> >         val |= hsw_infoframe_enable(frame);
> > --
> > 1.7.10.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch