| Message ID | 1348590215-4053-1-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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> > --- > 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
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
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);