[Intel-gfx] [PATCH v2] drm/i915/hsw: keep gamma and CSC enabled for primary plane disable

[Ville Syrjälä]

On Tue, Oct 20, 2015 at 08:48:36AM -0700, Bob Paauwe wrote:
> On Mon, 19 Oct 2015 10:13:58 -0700
> Kevin Strasser <kevin.strasser at linux.intel.com> wrote:
> 
> > On Mon, Oct 19, 2015 at 12:15:41PM +0200, Daniel Vetter wrote:
> > > On Fri, Oct 16, 2015 at 03:53:22PM -0700, Bob Paauwe wrote:
> > > > On Thu, 15 Oct 2015 15:41:30 +0300
> > > > Ville Syrjälä <ville.syrjala at linux.intel.com> wrote:
> > > > 
> > > > > On Thu, Oct 15, 2015 at 02:31:09PM +0200, Daniel Vetter wrote:
> > > > > > 
> > > > > > On Thu, Oct 15, 2015 at 12:51 AM, Kevin Strasser <kevin.strasser at 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 gamma correction
> > > > > > > and color space conversion are causing the black primary plane case to result in
> > > > > > > a brighter color than the disabled primary plane case.
> > > > > > >
> > > > > > > Keep gamma and CSC bits enabled for plane disable path on HSW.
> > > > > > >
> > > > > > > v2: Avoid use of RMW
> > > > > > >     Keep path unchanged for non-HSW users
> > > > > > >
> > > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89331
> > > > > > > Testcase: igt/kms_universal_plane/universal-plane-pipe-A-functional
> > > > > > > Signed-off-by: Kevin Strasser <kevin.strasser at linux.intel.com>
> > > > > > 
> > > > > > With big discussions please add everyone who participated (Ville, Chris,
> > > > > > Jani, me) to the cc list of the sob section of the patch when doing a new
> > > > > > revisions.
> > > > > > 
> > > > > > Bob did something eerily similarly a while ago to fix crc failures:
> > > > > > 
> > > > > > http://lists.freedesktop.org/archives/intel-gfx/2015-August/074657.html
> > > > > > http://lists.freedesktop.org/archives/intel-gfx/2015-August/074828.html
> > > > > > 
> > > > > > Unfortunately those patches did go nowhere :( Related?
> > > > > 
> > > > > Those seem to be for active plane cases. I'll go review them...
> > > > > 
> > > > 
> > > > I've been looking at the same test case failure on BXT. The bxt/skl
> > > > update primary plane function is similar to HSW's in that it was
> > > > clearing the gamma and csc bits when the plane was disabled. So I first
> > > > tried the same fix but it didn't work.  Matt and I were discussing this
> > > > and thought it might be the background (PIPE_BOTTOM_COLOR) that was
> > > > causing the CRC failures.  And yes, it is.  The PIPE_BOTTOM_COLOR also
> > > > has gamma and csc enable bits which default to off.  If those are
> > > > flipped on, then the CRC's match between a disabled plane and a black
> > > > plane. 
> > > > 
> > > > So it seems to make CRC's match we need to make sure that all planes
> > > > (primary/sprite/cursor/bottom) have the same gamma/csc settings.
> > > > 
> > > > Chandra had a patch that adds a property for bottom color a while back
> > > > but it seems to have stalled. 
> > > 
> > > We don't need the bottom color to make sure the bottom color is the right
> > > kind of black. So for now we can just make sure this is the case in the
> > > crtc_enable hook.
> > > 
> > > And yes the gamma table/color correction stuff needs to make sure we stay
> > > uniform (i.e. all off or all on), another igt for them to write I think.
> > 
> > Are there any changes I need to make to this patch? Or are we moving in a
> > different direction?
> > 
> > Thanks,
> > Kevin
> 
> It looks good to me so I'll add my RB, but I'm not able to test it
> since I don't have either of those platforms.  I was hoping that the
> same fix would work for SKL/BXT, but it doesn't, those need the bottom
> color gamma/csc enabled to fix the test case.  I have a patch to add
> that and will submit it shortly.

Feels like we're moving forward blindly. I didn't see anyone actually
explain why the test fails? Normally I would assume palette entry 0 to
be 0, and black input should go through palette entry 0, so why is it
actually not producing black output? 

-- 
Ville Syrjälä
Intel OTC