[2/2] drm/i915/skl+: Enable pipe CSC on cursor planes.
diff mbox

Message ID 1440708391-18358-2-git-send-email-bob.j.paauwe@intel.com
State New
Headers show

Commit Message

Paauwe, Bob J Aug. 27, 2015, 8:46 p.m. UTC
Extend this to SKL and BXT as it's needed for these platforms as well.

Signed-off-by: Bob Paauwe <bob.j.paauwe@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Daniel Stone Aug. 28, 2015, 2:19 p.m. UTC | #1
Hi Bob,

On 27 August 2015 at 21:46, Bob Paauwe <bob.j.paauwe@intel.com> wrote:
> Extend this to SKL and BXT as it's needed for these platforms as well.
>
> Signed-off-by: Bob Paauwe <bob.j.paauwe@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 88f9764..007bf7d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10001,7 +10001,8 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
>                 }
>                 cntl |= pipe << 28; /* Connect to correct pipe */
>
> -               if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> +               if (IS_HASWELL(dev) || IS_BROADWELL(dev) ||
> +                   IS_SKYLAKE(dev) || IS_BROXTON(dev))
>                         cntl |= CURSOR_PIPE_CSC_ENABLE;

For both this and the previous patch, cf. the corresponding patch for
HSW/BDW[0], have you ensured these values are sanitised at startup,
even if UEFI hasn't set something clever? Enabling fastboot on my
(UEFI-based) BDW caused a black screen because were enabling CSC but
with an empty table.

Cheers,
Daniel

[0]: https://www.mail-archive.com/intel-gfx@lists.freedesktop.org/msg67294.html
Paauwe, Bob J Aug. 28, 2015, 3:55 p.m. UTC | #2
On Fri, 28 Aug 2015 15:19:04 +0100
Daniel Stone <daniel@fooishbar.org> wrote:

> Hi Bob,
> 
> On 27 August 2015 at 21:46, Bob Paauwe <bob.j.paauwe@intel.com> wrote:
> > Extend this to SKL and BXT as it's needed for these platforms as well.
> >
> > Signed-off-by: Bob Paauwe <bob.j.paauwe@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 88f9764..007bf7d 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -10001,7 +10001,8 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
> >                 }
> >                 cntl |= pipe << 28; /* Connect to correct pipe */
> >
> > -               if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> > +               if (IS_HASWELL(dev) || IS_BROADWELL(dev) ||
> > +                   IS_SKYLAKE(dev) || IS_BROXTON(dev))
> >                         cntl |= CURSOR_PIPE_CSC_ENABLE;
> 
> For both this and the previous patch, cf. the corresponding patch for
> HSW/BDW[0], have you ensured these values are sanitised at startup,
> even if UEFI hasn't set something clever? Enabling fastboot on my
> (UEFI-based) BDW caused a black screen because were enabling CSC but
> with an empty table.
> 
> Cheers,
> Daniel
> 
> [0]: https://www.mail-archive.com/intel-gfx@lists.freedesktop.org/msg67294.html

Hmm, no I didn't.  I assumed that it was all set up correctly since for
SKL+ the primary plane always has PIPE_CSC enabled. My two patches are
just to ensure that both the cursor and sprite planes also have it
enabled.  If all the planes are configured the same, it causes a lot of
CRC failures in the igt tests.  

Unless I'm missing something (very possible), the pipe CSC setup/lack of
setup is a separate issue.

Looking at Maarten's patch, it looks like mine above should have been
written as 

  if (HAS_DDI(dev))

instead of all the separate conditions. 

Bob
Daniel Stone Aug. 28, 2015, 4:12 p.m. UTC | #3
Hi,

On 28 August 2015 at 16:55, Bob Paauwe <bob.j.paauwe@intel.com> wrote:
> On Fri, 28 Aug 2015 15:19:04 +0100
> Daniel Stone <daniel@fooishbar.org> wrote:
>> For both this and the previous patch, cf. the corresponding patch for
>> HSW/BDW[0], have you ensured these values are sanitised at startup,
>> even if UEFI hasn't set something clever? Enabling fastboot on my
>> (UEFI-based) BDW caused a black screen because were enabling CSC but
>> with an empty table.
>>
>> Cheers,
>> Daniel
>>
>> [0]: https://www.mail-archive.com/intel-gfx@lists.freedesktop.org/msg67294.html
>
> Hmm, no I didn't.  I assumed that it was all set up correctly since for
> SKL+ the primary plane always has PIPE_CSC enabled.

So does HSW/BDW. ;) The problem is that a CSC is only applied in the
modeset path; when using 'fastboot' to skip the original modeset (i.e.
to achieve flicker-free and quicker boot), the plane configuration
applies CSC/gamma, but the tables are only applied on modeset. So you
may end up with an invalid configuration (and blank screen), without a
similar patch.

> My two patches are
> just to ensure that both the cursor and sprite planes also have it
> enabled.  If all the planes are configured the same, it causes a lot of
> CRC failures in the igt tests.
>
> Unless I'm missing something (very possible), the pipe CSC setup/lack of
> setup is a separate issue.

Yeah, it is. But it'd be good to make sure Maarten's patch gets dragged in too.

> Looking at Maarten's patch, it looks like mine above should have been
> written as
>
>   if (HAS_DDI(dev))
>
> instead of all the separate conditions.

Yeah, indeed. I'd misread the HAS_DDI bit myself, so it seems fine.
Are you able to test Maarten's patch to always program the CSC tables
and make sure it doesn't break SKL/BXT?

Thanks, and sorry for the confusion.

Cheers,
Daniel
Paauwe, Bob J Aug. 28, 2015, 9:04 p.m. UTC | #4
On Fri, 28 Aug 2015 17:12:12 +0100
Daniel Stone <daniel@fooishbar.org> wrote:

> Hi,
> 
> On 28 August 2015 at 16:55, Bob Paauwe <bob.j.paauwe@intel.com> wrote:
> > On Fri, 28 Aug 2015 15:19:04 +0100
> > Daniel Stone <daniel@fooishbar.org> wrote:
> >> For both this and the previous patch, cf. the corresponding patch for
> >> HSW/BDW[0], have you ensured these values are sanitised at startup,
> >> even if UEFI hasn't set something clever? Enabling fastboot on my
> >> (UEFI-based) BDW caused a black screen because were enabling CSC but
> >> with an empty table.
> >>
> >> Cheers,
> >> Daniel
> >>
> >> [0]: https://www.mail-archive.com/intel-gfx@lists.freedesktop.org/msg67294.html
> >
> > Hmm, no I didn't.  I assumed that it was all set up correctly since for
> > SKL+ the primary plane always has PIPE_CSC enabled.
> 
> So does HSW/BDW. ;) The problem is that a CSC is only applied in the
> modeset path; when using 'fastboot' to skip the original modeset (i.e.
> to achieve flicker-free and quicker boot), the plane configuration
> applies CSC/gamma, but the tables are only applied on modeset. So you
> may end up with an invalid configuration (and blank screen), without a
> similar patch.
> 
> > My two patches are
> > just to ensure that both the cursor and sprite planes also have it
> > enabled.  If all the planes are configured the same, it causes a lot of
> > CRC failures in the igt tests.
> >
> > Unless I'm missing something (very possible), the pipe CSC setup/lack of
> > setup is a separate issue.
> 
> Yeah, it is. But it'd be good to make sure Maarten's patch gets dragged in too.

That's what I thought you meant, but wanted to make sure I wasn't
confused. 

> 
> > Looking at Maarten's patch, it looks like mine above should have been
> > written as
> >
> >   if (HAS_DDI(dev))
> >
> > instead of all the separate conditions.
> 
> Yeah, indeed. I'd misread the HAS_DDI bit myself, so it seems fine.
> Are you able to test Maarten's patch to always program the CSC tables
> and make sure it doesn't break SKL/BXT?

I did apply his series and it doesn't change the behavior on my SKL,
but I also don't have any issues if I enable fastboot without his
series.  Which I guess means that my UEFI is setting up the table
correctly.
> 
> Thanks, and sorry for the confusion.

You made me dig into how CSC works a bit more and that a good thing!
Thanks for bring it up.

> 
> Cheers,
> Daniel

Bob
Shuang He Sept. 1, 2015, 10:54 a.m. UTC | #5
Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 7280
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
ILK                 -1              302/302              301/302
SNB                                  315/315              315/315
IVB                                  336/336              336/336
BYT                                  283/283              283/283
HSW                                  378/378              378/378
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*ILK  igt@kms_flip@wf_vblank-vs-modeset-interruptible      PASS(1)      DMESG_WARN(1)
Note: You need to pay more attention to line start with '*'

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 88f9764..007bf7d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10001,7 +10001,8 @@  static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
 		}
 		cntl |= pipe << 28; /* Connect to correct pipe */
 
-		if (IS_HASWELL(dev) || IS_BROADWELL(dev))
+		if (IS_HASWELL(dev) || IS_BROADWELL(dev) ||
+		    IS_SKYLAKE(dev) || IS_BROXTON(dev))
 			cntl |= CURSOR_PIPE_CSC_ENABLE;
 	}