| Message ID | 1444771481-5625-1-git-send-email-kevin.strasser@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/13/2015 02:24 PM, Kevin Strasser wrote: > On HSW the crc differs between black and disabled primary planes, causing an > assert to fail in the kms_universal_plane test. It seems that things like gamma > correction are causing the black primary plane case to result in a brighter > color than the disabled primary plane case. > > Only toggle the enable bit instead of clearing the control register, making the > disable path more similar to that of the sprite plane. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89331 > Testcase: igt/kms_universal_plane > Signed-off-by: Kevin Strasser <kevin.strasser@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index cddb0c6..b6164d8e 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -2829,7 +2829,7 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc, > int pixel_size; > > if (!visible || !fb) { > - I915_WRITE(reg, 0); > + I915_WRITE(reg, I915_READ(reg) & ~DISPLAY_PLANE_ENABLE); > I915_WRITE(DSPSURF(plane), 0); > POSTING_READ(reg); > return; For some reason this rings a bell. Paulo did you work on something similar awhile back? Anyway, hooray for fixing bugs! Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org> Thanks, Jesse
On Wed, 14 Oct 2015, Kevin Strasser <kevin.strasser@linux.intel.com> wrote: > On HSW the crc differs between black and disabled primary planes, causing an > assert to fail in the kms_universal_plane test. It seems that things like gamma > correction are causing the black primary plane case to result in a brighter > color than the disabled primary plane case. > > Only toggle the enable bit instead of clearing the control register, making the > disable path more similar to that of the sprite plane. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89331 > Testcase: igt/kms_universal_plane > Signed-off-by: Kevin Strasser <kevin.strasser@linux.intel.com> Cc: stable@vger.kernel.org # v3.18 Fixes: fdd508a64192 ("drm/i915: Call .update_primary_plane in intel_{enable, disable}_primary_hw_plane()") How about i9xx_update_primary_plane, modified in the same commit above, and skylake_update_primary_plane, added in commit 6156a45602f990cdb140025a3ced96e6695980cf Author: Chandra Konduru <chandra.konduru@intel.com> Date: Mon Apr 27 13:48:39 2015 -0700 drm/i915: skylake primary plane scaling using shared scalers BR, Jani. > --- > drivers/gpu/drm/i915/intel_display.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index cddb0c6..b6164d8e 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -2829,7 +2829,7 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc, > int pixel_size; > > if (!visible || !fb) { > - I915_WRITE(reg, 0); > + I915_WRITE(reg, I915_READ(reg) & ~DISPLAY_PLANE_ENABLE); > I915_WRITE(DSPSURF(plane), 0); > POSTING_READ(reg); > return; > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, 14 Oct 2015, Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Wed, 14 Oct 2015, Kevin Strasser <kevin.strasser@linux.intel.com> wrote: >> On HSW the crc differs between black and disabled primary planes, causing an >> assert to fail in the kms_universal_plane test. It seems that things like gamma >> correction are causing the black primary plane case to result in a brighter >> color than the disabled primary plane case. >> >> Only toggle the enable bit instead of clearing the control register, making the >> disable path more similar to that of the sprite plane. >> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89331 >> Testcase: igt/kms_universal_plane >> Signed-off-by: Kevin Strasser <kevin.strasser@linux.intel.com> > > Cc: stable@vger.kernel.org # v3.18 > Fixes: fdd508a64192 ("drm/i915: Call .update_primary_plane in intel_{enable, disable}_primary_hw_plane()") > > How about i9xx_update_primary_plane, modified in the same commit above, > and skylake_update_primary_plane, added in > > commit 6156a45602f990cdb140025a3ced96e6695980cf > Author: Chandra Konduru <chandra.konduru@intel.com> > Date: Mon Apr 27 13:48:39 2015 -0700 > > drm/i915: skylake primary plane scaling using shared scalers PS. If these need fixing too, please keep i9xx/ironlake fix in one commit, and skylake in another, as they're destined to be backported to different kernels. BR, Jani. > > BR, > Jani. > >> --- >> drivers/gpu/drm/i915/intel_display.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index cddb0c6..b6164d8e 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -2829,7 +2829,7 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc, >> int pixel_size; >> >> if (!visible || !fb) { >> - I915_WRITE(reg, 0); >> + I915_WRITE(reg, I915_READ(reg) & ~DISPLAY_PLANE_ENABLE); >> I915_WRITE(DSPSURF(plane), 0); >> POSTING_READ(reg); >> return; >> -- >> 1.9.1 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Jani Nikula, Intel Open Source Technology Center
On Tue, Oct 13, 2015 at 02:24:41PM -0700, Kevin Strasser wrote: > On HSW the crc differs between black and disabled primary planes, causing an > assert to fail in the kms_universal_plane test. It seems that things like gamma > correction are causing the black primary plane case to result in a brighter > color than the disabled primary plane case. > > Only toggle the enable bit instead of clearing the control register, making the > disable path more similar to that of the sprite plane. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89331 > Testcase: igt/kms_universal_plane > Signed-off-by: Kevin Strasser <kevin.strasser@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index cddb0c6..b6164d8e 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -2829,7 +2829,7 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc, > int pixel_size; > > if (!visible || !fb) { > - I915_WRITE(reg, 0); > + I915_WRITE(reg, I915_READ(reg) & ~DISPLAY_PLANE_ENABLE); Eh, what now? We've been trying to eliminate these nasty RMWs. Are you saying that if we disabled the plane, but leave the "pass plane data through gamma" it still affects the output for any pixel "covered" by the disabled plane? > I915_WRITE(DSPSURF(plane), 0); > POSTING_READ(reg); > return; > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Oct 14, 2015 at 03:07:41PM +0300, Ville Syrjälä wrote: > On Tue, Oct 13, 2015 at 02:24:41PM -0700, Kevin Strasser wrote: > > On HSW the crc differs between black and disabled primary planes, causing an > > assert to fail in the kms_universal_plane test. It seems that things like gamma > > correction are causing the black primary plane case to result in a brighter > > color than the disabled primary plane case. > > > > Only toggle the enable bit instead of clearing the control register, making the > > disable path more similar to that of the sprite plane. > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89331 > > Testcase: igt/kms_universal_plane > > Signed-off-by: Kevin Strasser <kevin.strasser@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index cddb0c6..b6164d8e 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -2829,7 +2829,7 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc, > > int pixel_size; > > > > if (!visible || !fb) { > > - I915_WRITE(reg, 0); > > + I915_WRITE(reg, I915_READ(reg) & ~DISPLAY_PLANE_ENABLE); > > Eh, what now? We've been trying to eliminate these nasty RMWs. > > Are you saying that if we disabled the plane, but leave the "pass plane > data through gamma" it still affects the output for any pixel "covered" > by the disabled plane? What I thought was being said was that if a plane is set to black (but with gamma enabled on the pipe) then a different CRC is produced compared to when the pipe is completely disabled (no plane at all). It sounded to me like a test case failure. -Chris
On Wed, Oct 14, 2015 at 01:12:27PM +0100, Chris Wilson wrote: > On Wed, Oct 14, 2015 at 03:07:41PM +0300, Ville Syrjälä wrote: > > On Tue, Oct 13, 2015 at 02:24:41PM -0700, Kevin Strasser wrote: > > > On HSW the crc differs between black and disabled primary planes, causing an > > > assert to fail in the kms_universal_plane test. It seems that things like gamma > > > correction are causing the black primary plane case to result in a brighter > > > color than the disabled primary plane case. > > > > > > Only toggle the enable bit instead of clearing the control register, making the > > > disable path more similar to that of the sprite plane. > > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89331 > > > Testcase: igt/kms_universal_plane > > > Signed-off-by: Kevin Strasser <kevin.strasser@linux.intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_display.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > index cddb0c6..b6164d8e 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -2829,7 +2829,7 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc, > > > int pixel_size; > > > > > > if (!visible || !fb) { > > > - I915_WRITE(reg, 0); > > > + I915_WRITE(reg, I915_READ(reg) & ~DISPLAY_PLANE_ENABLE); > > > > Eh, what now? We've been trying to eliminate these nasty RMWs. > > > > Are you saying that if we disabled the plane, but leave the "pass plane > > data through gamma" it still affects the output for any pixel "covered" > > by the disabled plane? > > What I thought was being said was that if a plane is set to black (but > with gamma enabled on the pipe) then a different CRC is produced > compared to when the pipe is completely disabled (no plane at all). It > sounded to me like a test case failure. In that case I don't understand how the patch is supposed to help. But yeah, tests like these should really set up an identity gamma and pipe csc matrix. Also we should grow some properties to control whether the plane data passes through the gamma/csc or not. Those could then be used to achieeve the same effect.
On Wed, Oct 14, 2015 at 03:07:41PM +0300, Ville Syrjälä wrote: > On Tue, Oct 13, 2015 at 02:24:41PM -0700, Kevin Strasser wrote: > > On HSW the crc differs between black and disabled primary planes, causing an > > assert to fail in the kms_universal_plane test. It seems that things like gamma > > correction are causing the black primary plane case to result in a brighter > > color than the disabled primary plane case. > > > > Only toggle the enable bit instead of clearing the control register, making the > > disable path more similar to that of the sprite plane. > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89331 > > Testcase: igt/kms_universal_plane > > Signed-off-by: Kevin Strasser <kevin.strasser@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index cddb0c6..b6164d8e 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -2829,7 +2829,7 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc, > > int pixel_size; > > > > if (!visible || !fb) { > > - I915_WRITE(reg, 0); > > + I915_WRITE(reg, I915_READ(reg) & ~DISPLAY_PLANE_ENABLE); > > Eh, what now? We've been trying to eliminate these nasty RMWs. > > Are you saying that if we disabled the plane, but leave the "pass plane > data through gamma" it still affects the output for any pixel "covered" > by the disabled plane? Yeah if we need to preserve the gamma bits then we should write that instead of keeping everything. RMWs are indeed evil, since they hide bugs. Like this one here. Problem is that your bugfix is incomplete - do a dpms on/off in between with runtime pm and the plane state will be _completely_ gone. Hence _never_ do an RMW anywhere in modeset code. Ever. -Daniel > > > I915_WRITE(DSPSURF(plane), 0); > > POSTING_READ(reg); > > return; > > -- > > 1.9.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Oct 14, 2015 at 03:04:05PM +0200, Daniel Vetter wrote: > On Wed, Oct 14, 2015 at 03:07:41PM +0300, Ville Syrjälä wrote: > > On Tue, Oct 13, 2015 at 02:24:41PM -0700, Kevin Strasser wrote: > > > On HSW the crc differs between black and disabled primary planes, causing an > > > assert to fail in the kms_universal_plane test. It seems that things like gamma > > > correction are causing the black primary plane case to result in a brighter > > > color than the disabled primary plane case. > > > > > > Only toggle the enable bit instead of clearing the control register, making the > > > disable path more similar to that of the sprite plane. > > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89331 > > > Testcase: igt/kms_universal_plane > > > Signed-off-by: Kevin Strasser <kevin.strasser@linux.intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_display.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > index cddb0c6..b6164d8e 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -2829,7 +2829,7 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc, > > > int pixel_size; > > > > > > if (!visible || !fb) { > > > - I915_WRITE(reg, 0); > > > + I915_WRITE(reg, I915_READ(reg) & ~DISPLAY_PLANE_ENABLE); > > > > Eh, what now? We've been trying to eliminate these nasty RMWs. > > > > Are you saying that if we disabled the plane, but leave the "pass plane > > data through gamma" it still affects the output for any pixel "covered" > > by the disabled plane? > > Yeah if we need to preserve the gamma bits then we should write that > instead of keeping everything. I'd say we never ever want to leave the gamma bit enabled if this is the case because that would produce a discolored rectangle on the screen whenever the plane is disabled and not fullscreen.
On Wed, Oct 14, 2015 at 10:58:29AM +0300, Jani Nikula wrote: > On Wed, 14 Oct 2015, Kevin Strasser <kevin.strasser@linux.intel.com> wrote: > > On HSW the crc differs between black and disabled primary planes, causing an > > assert to fail in the kms_universal_plane test. It seems that things like gamma > > correction are causing the black primary plane case to result in a brighter > > color than the disabled primary plane case. > > > > Only toggle the enable bit instead of clearing the control register, making the > > disable path more similar to that of the sprite plane. > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89331 > > Testcase: igt/kms_universal_plane > > Signed-off-by: Kevin Strasser <kevin.strasser@linux.intel.com> > > Cc: stable@vger.kernel.org # v3.18 > Fixes: fdd508a64192 ("drm/i915: Call .update_primary_plane in intel_{enable, disable}_primary_hw_plane()") > > How about i9xx_update_primary_plane, modified in the same commit above, > and skylake_update_primary_plane, added in > > commit 6156a45602f990cdb140025a3ced96e6695980cf > Author: Chandra Konduru <chandra.konduru@intel.com> > Date: Mon Apr 27 13:48:39 2015 -0700 > > drm/i915: skylake primary plane scaling using shared scalers Afaict HSW is the only platform that behaves in this way. I will follow up with a separate patch if needed. Thanks, Kevin
On Wed, Oct 14, 2015 at 03:22:23PM +0300, Ville Syrjälä wrote: > On Wed, Oct 14, 2015 at 01:12:27PM +0100, Chris Wilson wrote: > > On Wed, Oct 14, 2015 at 03:07:41PM +0300, Ville Syrjälä wrote: > > > On Tue, Oct 13, 2015 at 02:24:41PM -0700, Kevin Strasser wrote: [...] > > > > - I915_WRITE(reg, 0); > > > > + I915_WRITE(reg, I915_READ(reg) & ~DISPLAY_PLANE_ENABLE); > > > > > > Eh, what now? We've been trying to eliminate these nasty RMWs. > > > > > > Are you saying that if we disabled the plane, but leave the "pass plane > > > data through gamma" it still affects the output for any pixel "covered" > > > by the disabled plane? > > > > What I thought was being said was that if a plane is set to black (but > > with gamma enabled on the pipe) then a different CRC is produced > > compared to when the pipe is completely disabled (no plane at all). It > > sounded to me like a test case failure. > > In that case I don't understand how the patch is supposed to help. > > But yeah, tests like these should really set up an identity gamma > and pipe csc matrix. > > Also we should grow some properties to control whether the plane > data passes through the gamma/csc or not. Those could then be used > to achieeve the same effect. Just to level set, these cases will produce different CRCs on HSW: 1. Primary plane disabled, gamma correction disabled 2. Primary plane disabled, gamma correction enabled Case 2 is visibly brighter than case 1 and looks more like the enabled black primary plane case. The purpose of this patch is to get the behavior of a disabled primary plane to match that of an enabled black plane, just as it does on non-HSW platforms. Understood, RMWs are inappropriate here. I'll rework the patch to explicitly enable the bits that are needed. Thanks, Kevin
On Wed, Oct 14, 2015 at 8:44 PM, Kevin Strasser <kevin.strasser@linux.intel.com> wrote: > On Wed, Oct 14, 2015 at 10:58:29AM +0300, Jani Nikula wrote: >> On Wed, 14 Oct 2015, Kevin Strasser <kevin.strasser@linux.intel.com> wrote: >> > On HSW the crc differs between black and disabled primary planes, causing an >> > assert to fail in the kms_universal_plane test. It seems that things like gamma >> > correction are causing the black primary plane case to result in a brighter >> > color than the disabled primary plane case. >> > >> > Only toggle the enable bit instead of clearing the control register, making the >> > disable path more similar to that of the sprite plane. >> > >> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89331 >> > Testcase: igt/kms_universal_plane >> > Signed-off-by: Kevin Strasser <kevin.strasser@linux.intel.com> >> >> Cc: stable@vger.kernel.org # v3.18 >> Fixes: fdd508a64192 ("drm/i915: Call .update_primary_plane in intel_{enable, disable}_primary_hw_plane()") >> >> How about i9xx_update_primary_plane, modified in the same commit above, >> and skylake_update_primary_plane, added in >> >> commit 6156a45602f990cdb140025a3ced96e6695980cf >> Author: Chandra Konduru <chandra.konduru@intel.com> >> Date: Mon Apr 27 13:48:39 2015 -0700 >> >> drm/i915: skylake primary plane scaling using shared scalers > > Afaict HSW is the only platform that behaves in this way. I will follow up with > a separate patch if needed. Are you sure this is specific to hsw and not an artifact of e.g. the hdmi color compression we do? That would apply the same on bdw, but you need the same screen plugged in. -Daniel
On Wed, Oct 14, 2015 at 11:59:59AM -0700, Kevin Strasser wrote: > On Wed, Oct 14, 2015 at 03:22:23PM +0300, Ville Syrjälä wrote: > > On Wed, Oct 14, 2015 at 01:12:27PM +0100, Chris Wilson wrote: > > > On Wed, Oct 14, 2015 at 03:07:41PM +0300, Ville Syrjälä wrote: > > > > On Tue, Oct 13, 2015 at 02:24:41PM -0700, Kevin Strasser wrote: > [...] > > > > > - I915_WRITE(reg, 0); > > > > > + I915_WRITE(reg, I915_READ(reg) & ~DISPLAY_PLANE_ENABLE); > > > > > > > > Eh, what now? We've been trying to eliminate these nasty RMWs. > > > > > > > > Are you saying that if we disabled the plane, but leave the "pass plane > > > > data through gamma" it still affects the output for any pixel "covered" > > > > by the disabled plane? > > > > > > What I thought was being said was that if a plane is set to black (but > > > with gamma enabled on the pipe) then a different CRC is produced > > > compared to when the pipe is completely disabled (no plane at all). It > > > sounded to me like a test case failure. > > > > In that case I don't understand how the patch is supposed to help. > > > > But yeah, tests like these should really set up an identity gamma > > and pipe csc matrix. > > > > Also we should grow some properties to control whether the plane > > data passes through the gamma/csc or not. Those could then be used > > to achieeve the same effect. > > Just to level set, these cases will produce different CRCs on HSW: > 1. Primary plane disabled, gamma correction disabled > 2. Primary plane disabled, gamma correction enabled > > Case 2 is visibly brighter than case 1 and looks more like the enabled black > primary plane case. Ugh. That's weird. I thought data not going through any plane would bypass the gamma too. > The purpose of this patch is to get the behavior of a > disabled primary plane to match that of an enabled black plane, just as it does > on non-HSW platforms. Does it? I just tried it on IVB, and behaves just like you said. So not sure how far back this goes. And now I'm really wondering about platforms where the primary plane need not be fullscreen (gen2/3 and chv). I tried this on SKL too, but that confused me even more. The data not going through any plane seems to be gamma corrected regardless of any plane control bits, so that's good. However the legacy palette seems all fubar. Black input apparently doesn't map to palette entry 0. I wonder if you're seeing this on HSW too, or is your palette entry 0 supposed to be non-black? Looks like quite a bit more testing is needed to get to the bottom of this.
On Wed, Oct 14, 2015 at 10:48:52PM +0300, Ville Syrjälä wrote: > On Wed, Oct 14, 2015 at 11:59:59AM -0700, Kevin Strasser wrote: [...] > > Just to level set, these cases will produce different CRCs on HSW: > > 1. Primary plane disabled, gamma correction disabled > > 2. Primary plane disabled, gamma correction enabled > > > > Case 2 is visibly brighter than case 1 and looks more like the enabled black > > primary plane case. > > Ugh. That's weird. I thought data not going through any plane would > bypass the gamma too. > > > The purpose of this patch is to get the behavior of a > > disabled primary plane to match that of an enabled black plane, just as it does > > on non-HSW platforms. > > Does it? I just tried it on IVB, and behaves just like you said. So not > sure how far back this goes. Ah, so this test case is failing on IVB too? Are there any reporting tools we can look at to find out what tests are passing for each platform? > And now I'm really wondering about platforms where the primary > plane need not be fullscreen (gen2/3 and chv). > > I tried this on SKL too, but that confused me even more. The data not > going through any plane seems to be gamma corrected regardless of any > plane control bits, so that's good. However the legacy palette seems > all fubar. Black input apparently doesn't map to palette entry 0. > I wonder if you're seeing this on HSW too, or is your palette entry 0 > supposed to be non-black? I also tried on BDW and it is passing the test with and without my patch applied. I'm not really sure what 'palette entry 0' you are looking for. Could you point me to where in the code I can find out? > Looks like quite a bit more testing is needed to get to the bottom of > this. Agreed, this does seem to extend beyond HSW. For now do you think my patch is the right approach at least for HSW alone? Thanks, Kevin
On Wed, Oct 14, 2015 at 01:33:57PM -0700, Kevin Strasser wrote: > On Wed, Oct 14, 2015 at 10:48:52PM +0300, Ville Syrjälä wrote: > > On Wed, Oct 14, 2015 at 11:59:59AM -0700, Kevin Strasser wrote: > [...] > > > Just to level set, these cases will produce different CRCs on HSW: > > > 1. Primary plane disabled, gamma correction disabled > > > 2. Primary plane disabled, gamma correction enabled > > > > > > Case 2 is visibly brighter than case 1 and looks more like the enabled black > > > primary plane case. > > > > Ugh. That's weird. I thought data not going through any plane would > > bypass the gamma too. > > > > > The purpose of this patch is to get the behavior of a > > > disabled primary plane to match that of an enabled black plane, just as it does > > > on non-HSW platforms. > > > > Does it? I just tried it on IVB, and behaves just like you said. So not > > sure how far back this goes. > > Ah, so this test case is failing on IVB too? I just poked at the registers. I don't think we have specific test cases for this, so any currently failing test case is just bad luck. It would be good to write a small tool that just frobs the registers in specific ways, and tells us if the test machine suffers from this issue. Would be easy to run on any machine then. What I did manually was: intel_reg write 0x4a000 0x400000 intel_reg write 0x70180 0x0 <whatever> = intel_reg read 0x7019c intel_reg write 0x7019c <whatever> and as a second test I tried: intel_reg write 0x70180 <original value & ~(1<<31)) <whatever> = intel_reg read 0x7019c intel_reg write 0x7019c <whatever> And the result was black in the first test, dark red in the second. On SKL I additionally tried to resize the plane to be smaller, and then tried the same thing. But as stated the palette seems to misbehave somehow, so there was no change in the output from changing entry 0 even though most of the screen was supposed to be black. > Are there any reporting tools we > can look at to find out what tests are passing for each platform? > > > And now I'm really wondering about platforms where the primary > > plane need not be fullscreen (gen2/3 and chv). > > > > I tried this on SKL too, but that confused me even more. The data not > > going through any plane seems to be gamma corrected regardless of any > > plane control bits, so that's good. However the legacy palette seems > > all fubar. Black input apparently doesn't map to palette entry 0. > > I wonder if you're seeing this on HSW too, or is your palette entry 0 > > supposed to be non-black? > > I also tried on BDW and it is passing the test with and without my patch > applied. > > I'm not really sure what 'palette entry 0' you are looking for. Could you point > me to where in the code I can find out? Register 0x4a000, 0x4a800, 0x4b000, depending on which pipe you're using. > > > Looks like quite a bit more testing is needed to get to the bottom of > > this. > > Agreed, this does seem to extend beyond HSW. For now do you think my patch is > the right approach at least for HSW alone? Yeah, looks that way. But we do need to figure out which bits behave this way. Ie. is it just gamma, or pipe csc too, or perhaps even some other bits?
On Thu, Oct 15, 2015 at 11:20:59AM +0300, Ville Syrjälä wrote: > On Wed, Oct 14, 2015 at 01:33:57PM -0700, Kevin Strasser wrote: > > On Wed, Oct 14, 2015 at 10:48:52PM +0300, Ville Syrjälä wrote: > > > Does it? I just tried it on IVB, and behaves just like you said. So not > > > sure how far back this goes. > > > > Ah, so this test case is failing on IVB too? > > I just poked at the registers. I don't think we have specific test cases > for this, so any currently failing test case is just bad luck. > > It would be good to write a small tool that just frobs the registers in > specific ways, and tells us if the test machine suffers from this issue. > Would be easy to run on any machine then. Agreed, it is somewhat painful getting the whole test suite built and running in some environments. > What I did manually was: > intel_reg write 0x4a000 0x400000 > intel_reg write 0x70180 0x0 > <whatever> = intel_reg read 0x7019c > intel_reg write 0x7019c <whatever> > > and as a second test I tried: > intel_reg write 0x70180 <original value & ~(1<<31)) > <whatever> = intel_reg read 0x7019c > intel_reg write 0x7019c <whatever> > > And the result was black in the first test, dark red in > the second. > > On SKL I additionally tried to resize the plane to be smaller, and then > tried the same thing. But as stated the palette seems to misbehave > somehow, so there was no change in the output from changing entry 0 even > though most of the screen was supposed to be black. > > > Are there any reporting tools we > > can look at to find out what tests are passing for each platform? > > > > > And now I'm really wondering about platforms where the primary > > > plane need not be fullscreen (gen2/3 and chv). > > > > > > I tried this on SKL too, but that confused me even more. The data not > > > going through any plane seems to be gamma corrected regardless of any > > > plane control bits, so that's good. However the legacy palette seems > > > all fubar. Black input apparently doesn't map to palette entry 0. > > > I wonder if you're seeing this on HSW too, or is your palette entry 0 > > > supposed to be non-black? > > > > I also tried on BDW and it is passing the test with and without my patch > > applied. > > > > I'm not really sure what 'palette entry 0' you are looking for. Could you point > > me to where in the code I can find out? > > Register 0x4a000, 0x4a800, 0x4b000, depending on which pipe you're > using. I'm using pipe A, here is the output of 'intel_reg read 0x4a000': (0x0004a000): 0x00000000 I suppose this means palette entry 0 is black for me. > > > Looks like quite a bit more testing is needed to get to the bottom of > > > this. > > > > Agreed, this does seem to extend beyond HSW. For now do you think my patch is > > the right approach at least for HSW alone? > > Yeah, looks that way. But we do need to figure out which bits behave > this way. Ie. is it just gamma, or pipe csc too, or perhaps even some > other bits? I did some testing and it seems that gamma and pipe csc are both needed to pass the test on HSW. v2 of the patch sets them explicitly. Thanks, Kevin
On Thu, Oct 15, 2015 at 04:00:48PM -0700, Kevin Strasser wrote: > On Thu, Oct 15, 2015 at 11:20:59AM +0300, Ville Syrjälä wrote: > > On Wed, Oct 14, 2015 at 01:33:57PM -0700, Kevin Strasser wrote: > > > On Wed, Oct 14, 2015 at 10:48:52PM +0300, Ville Syrjälä wrote: > > > > Does it? I just tried it on IVB, and behaves just like you said. So not > > > > sure how far back this goes. > > > > > > Ah, so this test case is failing on IVB too? > > > > I just poked at the registers. I don't think we have specific test cases > > for this, so any currently failing test case is just bad luck. > > > > It would be good to write a small tool that just frobs the registers in > > specific ways, and tells us if the test machine suffers from this issue. > > Would be easy to run on any machine then. > > Agreed, it is somewhat painful getting the whole test suite built and running in > some environments. > > > What I did manually was: > > intel_reg write 0x4a000 0x400000 > > intel_reg write 0x70180 0x0 > > <whatever> = intel_reg read 0x7019c > > intel_reg write 0x7019c <whatever> > > > > and as a second test I tried: > > intel_reg write 0x70180 <original value & ~(1<<31)) > > <whatever> = intel_reg read 0x7019c > > intel_reg write 0x7019c <whatever> > > > > And the result was black in the first test, dark red in > > the second. > > > > On SKL I additionally tried to resize the plane to be smaller, and then > > tried the same thing. But as stated the palette seems to misbehave > > somehow, so there was no change in the output from changing entry 0 even > > though most of the screen was supposed to be black. > > > > > Are there any reporting tools we > > > can look at to find out what tests are passing for each platform? > > > > > > > And now I'm really wondering about platforms where the primary > > > > plane need not be fullscreen (gen2/3 and chv). > > > > > > > > I tried this on SKL too, but that confused me even more. The data not > > > > going through any plane seems to be gamma corrected regardless of any > > > > plane control bits, so that's good. However the legacy palette seems > > > > all fubar. Black input apparently doesn't map to palette entry 0. > > > > I wonder if you're seeing this on HSW too, or is your palette entry 0 > > > > supposed to be non-black? > > > > > > I also tried on BDW and it is passing the test with and without my patch > > > applied. > > > > > > I'm not really sure what 'palette entry 0' you are looking for. Could you point > > > me to where in the code I can find out? > > > > Register 0x4a000, 0x4a800, 0x4b000, depending on which pipe you're > > using. > > I'm using pipe A, here is the output of 'intel_reg read 0x4a000': > (0x0004a000): 0x00000000 > > I suppose this means palette entry 0 is black for me. It means it's supposed to be black. But if you saw a difference between black when it went through the gamma and when it didn't, then I guess black is not hitting entry 0 on hsw either. So something seems fishy. > > > > > Looks like quite a bit more testing is needed to get to the bottom of > > > > this. > > > > > > Agreed, this does seem to extend beyond HSW. For now do you think my patch is > > > the right approach at least for HSW alone? > > > > Yeah, looks that way. But we do need to figure out which bits behave > > this way. Ie. is it just gamma, or pipe csc too, or perhaps even some > > other bits? > > I did some testing and it seems that gamma and pipe csc are both needed to pass > the test on HSW. v2 of the patch sets them explicitly. > > Thanks, > Kevin
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index cddb0c6..b6164d8e 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2829,7 +2829,7 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc, int pixel_size; if (!visible || !fb) { - I915_WRITE(reg, 0); + I915_WRITE(reg, I915_READ(reg) & ~DISPLAY_PLANE_ENABLE); I915_WRITE(DSPSURF(plane), 0); POSTING_READ(reg); return;
On HSW the crc differs between black and disabled primary planes, causing an assert to fail in the kms_universal_plane test. It seems that things like gamma correction are causing the black primary plane case to result in a brighter color than the disabled primary plane case. Only toggle the enable bit instead of clearing the control register, making the disable path more similar to that of the sprite plane. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89331 Testcase: igt/kms_universal_plane Signed-off-by: Kevin Strasser <kevin.strasser@linux.intel.com> --- drivers/gpu/drm/i915/intel_display.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)