[Intel-gfx] [PATCH] drm/i915: respect previous reg values on primary plane disable

[Daniel Vetter]

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 at 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 at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch