| Message ID | 1440708391-18358-2-git-send-email-bob.j.paauwe@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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 '*'
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; }
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(-)