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

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

[Kevin Strasser]

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;
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: respect previous reg values on primary plane disable

[Jesse Barnes]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: respect previous reg values on primary plane disable

[Jani Nikula]

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

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: respect previous reg values on primary plane disable

[Jani Nikula]

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

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: respect previous reg values on primary plane disable

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: respect previous reg values on primary plane disable

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: respect previous reg values on primary plane disable

[Ville Syrjälä]

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.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: respect previous reg values on primary plane disable

[Ville Syrjälä]

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.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: respect previous reg values on primary plane disable

[Kevin Strasser]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: respect previous reg values on primary plane disable

[Kevin Strasser]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: respect previous reg values on primary plane disable

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: respect previous reg values on primary plane disable

[Ville Syrjälä]

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.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: respect previous reg values on primary plane disable

[Kevin Strasser]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Kevin Strasser]

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@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d37b7a1..4a04e8b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2798,8 +2798,17 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
 	u32 reg = DSPCNTR(plane);
 	int pixel_size;
 
+	dspcntr = DISPPLANE_GAMMA_ENABLE;
+
+	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
+		dspcntr |= DISPPLANE_PIPE_CSC_ENABLE;
+
 	if (!visible || !fb) {
-		I915_WRITE(reg, 0);
+		if (IS_HASWELL(dev)) {
+			I915_WRITE(reg, dspcntr);
+		} else {
+			I915_WRITE(reg, 0);
+		}
 		I915_WRITE(DSPSURF(plane), 0);
 		POSTING_READ(reg);
 		return;
@@ -2811,13 +2820,8 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
 
 	pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
 
-	dspcntr = DISPPLANE_GAMMA_ENABLE;
-
 	dspcntr |= DISPLAY_PLANE_ENABLE;
 
-	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
-		dspcntr |= DISPPLANE_PIPE_CSC_ENABLE;
-
 	switch (fb->pixel_format) {
 	case DRM_FORMAT_C8:
 		dspcntr |= DISPPLANE_8BPP;
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: respect previous reg values on primary plane disable

[Ville Syrjälä]

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?

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

On Thu, Oct 15, 2015 at 12:51 AM, 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 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@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?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

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@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@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...

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: respect previous reg values on primary plane disable

[Kevin Strasser]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: respect previous reg values on primary plane disable

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Bob Paauwe]

On Thu, 15 Oct 2015 15:41:30 +0300
Ville Syrjälä <ville.syrjala@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@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@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. 

http://lists.freedesktop.org/archives/intel-gfx/2015-June/069836.html

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

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@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@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@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.
-Daniel

> 
> http://lists.freedesktop.org/archives/intel-gfx/2015-June/069836.html
> 
> -- 
> --
> Bob Paauwe                  
> Bob.J.Paauwe@intel.com
> IOTG / PED Software Organization
> Intel Corp.  Folsom, CA
> (916) 356-6193    
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Kevin Strasser]

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@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@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@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Bob Paauwe]

On Mon, 19 Oct 2015 10:13:58 -0700
Kevin Strasser <kevin.strasser@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@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@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@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.

Bob



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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Bob Paauwe]

On Wed, 14 Oct 2015 15:51:39 -0700
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 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@linux.intel.com>

Reviewed-by: Bob Paauwe <bob.j.paauwe@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index d37b7a1..4a04e8b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2798,8 +2798,17 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
>  	u32 reg = DSPCNTR(plane);
>  	int pixel_size;
>  
> +	dspcntr = DISPPLANE_GAMMA_ENABLE;
> +
> +	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> +		dspcntr |= DISPPLANE_PIPE_CSC_ENABLE;
> +
>  	if (!visible || !fb) {
> -		I915_WRITE(reg, 0);
> +		if (IS_HASWELL(dev)) {
> +			I915_WRITE(reg, dspcntr);
> +		} else {
> +			I915_WRITE(reg, 0);
> +		}
>  		I915_WRITE(DSPSURF(plane), 0);
>  		POSTING_READ(reg);
>  		return;
> @@ -2811,13 +2820,8 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
>  
>  	pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
>  
> -	dspcntr = DISPPLANE_GAMMA_ENABLE;
> -
>  	dspcntr |= DISPLAY_PLANE_ENABLE;
>  
> -	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> -		dspcntr |= DISPPLANE_PIPE_CSC_ENABLE;
> -
>  	switch (fb->pixel_format) {
>  	case DRM_FORMAT_C8:
>  		dspcntr |= DISPPLANE_8BPP;



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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@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@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@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@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@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@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@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@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@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@intel.com
IOTG / PED Software Organization
Intel Corp.  Folsom, CA
(916) 356-6193    

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

On Tue, Oct 20, 2015 at 10:00:40AM -0700, Bob Paauwe wrote:
> On Tue, 20 Oct 2015 19:13:19 +0300
> Ville Syrjälä <ville.syrjala@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@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@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@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@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.  

I agree with Ville, this looks like a random piece of duct-tape and seems
to be missing a solid explanation for why we need to do this. One possible
explanation is that before universal planes disabling the primary plane
didn't disable it, but instead just feeding it black. Hence why we still
need to set gamma. And with universal planes on gen9+ that background
color gamma bit move to the real background color registers.

But that's just an wild guess, I'd like confirmation from hw engineers.

Also, pretty sure this isn't just a problem on Haswell.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx