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

[Bob Paauwe]

On Tue, 20 Oct 2015 19:13:19 +0300
Ville Syrjälä <ville.syrjala at linux.intel.com> wrote:

> 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? 
> 

The tests fail because the CRC values are different when gamma/CSC is
enabled and when it's disabled.   Now are we properly setting up
gamma and the CSC before enabling them such that if they were
properly set up, there wouldn't be any difference?  Maybe not, but I
don't think that's what we're trying to solve or test with the various
igt tests that are failing.

My working assumption is that we don't want the pipe gamma/csc
turning on/off as we enable/disable planes. Since we started enabling
it for the primary plane, we should consistently leave it enabled
regardless of which planes are enabled and which are disabled.  


-- 
--
Bob Paauwe                  
Bob.J.Paauwe at intel.com
IOTG / PED Software Organization
Intel Corp.  Folsom, CA
(916) 356-6193