| Message ID | 1455645225-13074-1-git-send-email-ander.conselvan.de.oliveira@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 16 Feb 2016, Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> wrote: > The wait for other gens was added in commit 564ed191f5d8 ("drm/i915: > gmch: fix stuck primary plane due to memory self-refresh mode") since > that's necessary when disabling cxsr. However, cxsr disabling was later > moved to intel_pre_disable_primary() in commit 87d4300a7dbc ("drm/i915: > Move intel_(pre_disable/post_enable)_primary to intel_display.c, and use > it there.") and that function got its own vblank wait for cxsr in commit > 262cd2e154c2 ("drm/i915: CHV DDR DVFS support and another watermark > rewrite"). So remove the extra vblank wait from i9xx_crtc_distable(). > > Cc: Kalyan Kondapally <kalyan.kondapally@intel.com> > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> Cc: fixes or stable? Fixes which commit exactly of all those? BR, Jani. > --- > drivers/gpu/drm/i915/intel_display.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 568eefc..3cb9383 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -6317,10 +6317,9 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc) > /* > * On gen2 planes are double buffered but the pipe isn't, so we must > * wait for planes to fully turn off before disabling the pipe. > - * We also need to wait on all gmch platforms because of the > - * self-refresh mode constraint explained above. > */ > - intel_wait_for_vblank(dev, pipe); > + if (IS_GEN2(dev)) > + intel_wait_for_vblank(dev, pipe); > > for_each_encoder_on_crtc(dev, crtc, encoder) > encoder->disable(encoder);
On Wed, 2016-02-17 at 11:25 +0200, Jani Nikula wrote: > On Tue, 16 Feb 2016, Ander Conselvan de Oliveira < > ander.conselvan.de.oliveira@intel.com> wrote: > > The wait for other gens was added in commit 564ed191f5d8 ("drm/i915: > > gmch: fix stuck primary plane due to memory self-refresh mode") since > > that's necessary when disabling cxsr. However, cxsr disabling was later > > moved to intel_pre_disable_primary() in commit 87d4300a7dbc ("drm/i915: > > Move intel_(pre_disable/post_enable)_primary to intel_display.c, and use > > it there.") and that function got its own vblank wait for cxsr in commit > > 262cd2e154c2 ("drm/i915: CHV DDR DVFS support and another watermark > > rewrite"). So remove the extra vblank wait from i9xx_crtc_distable(). > > > > Cc: Kalyan Kondapally <kalyan.kondapally@intel.com> > > Signed-off-by: Ander Conselvan de Oliveira < > > ander.conselvan.de.oliveira@intel.com> > > Cc: fixes or stable? Fixes which commit exactly of all those? Fixes: 262cd2e154c2 ("drm/i915: CHV DDR DVFS support and another watermark rewrite") Kalyan was seeing some screen corruption with BSW when suspending. He did some experimenting with moving vblank waits around and it seemed to help. When I dug through the history of the code I noticed that we now have a double wait for vblank for cxsr, but I wasn't able to confirm this is actually the fix for that screen corruption. So this may be a candidate for fixes or even stable, but I'm not entirely sure. I attached an image of what the display corruption looks like, in case someone has any clues. Thanks, Ander > > BR, > Jani. > > > > --- > > drivers/gpu/drm/i915/intel_display.c | 5 ++--- > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c > > index 568eefc..3cb9383 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -6317,10 +6317,9 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc) > > /* > > * On gen2 planes are double buffered but the pipe isn't, so we > > must > > * wait for planes to fully turn off before disabling the pipe. > > - * We also need to wait on all gmch platforms because of the > > - * self-refresh mode constraint explained above. > > */ > > - intel_wait_for_vblank(dev, pipe); > > + if (IS_GEN2(dev)) > > + intel_wait_for_vblank(dev, pipe); > > > > for_each_encoder_on_crtc(dev, crtc, encoder) > > encoder->disable(encoder); >
On Wed, Feb 17, 2016 at 11:25:49AM +0200, Jani Nikula wrote: > On Tue, 16 Feb 2016, Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> wrote: > > The wait for other gens was added in commit 564ed191f5d8 ("drm/i915: > > gmch: fix stuck primary plane due to memory self-refresh mode") since > > that's necessary when disabling cxsr. However, cxsr disabling was later > > moved to intel_pre_disable_primary() in commit 87d4300a7dbc ("drm/i915: > > Move intel_(pre_disable/post_enable)_primary to intel_display.c, and use > > it there.") and that function got its own vblank wait for cxsr in commit > > 262cd2e154c2 ("drm/i915: CHV DDR DVFS support and another watermark > > rewrite"). So remove the extra vblank wait from i9xx_crtc_distable(). > > > > Cc: Kalyan Kondapally <kalyan.kondapally@intel.com> > > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> > > Cc: fixes or stable? Fixes which commit exactly of all those? Why do we want to put a fix to remove a harmless roughly 16ms delay to stable? Or do I miss something here? -Daniel > > BR, > Jani. > > > > --- > > drivers/gpu/drm/i915/intel_display.c | 5 ++--- > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 568eefc..3cb9383 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -6317,10 +6317,9 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc) > > /* > > * On gen2 planes are double buffered but the pipe isn't, so we must > > * wait for planes to fully turn off before disabling the pipe. > > - * We also need to wait on all gmch platforms because of the > > - * self-refresh mode constraint explained above. > > */ > > - intel_wait_for_vblank(dev, pipe); > > + if (IS_GEN2(dev)) > > + intel_wait_for_vblank(dev, pipe); > > > > for_each_encoder_on_crtc(dev, crtc, encoder) > > encoder->disable(encoder); > > -- > Jani Nikula, Intel Open Source Technology Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, 17 Feb 2016, Daniel Vetter <daniel@ffwll.ch> wrote: > On Wed, Feb 17, 2016 at 11:25:49AM +0200, Jani Nikula wrote: >> On Tue, 16 Feb 2016, Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> wrote: >> > The wait for other gens was added in commit 564ed191f5d8 ("drm/i915: >> > gmch: fix stuck primary plane due to memory self-refresh mode") since >> > that's necessary when disabling cxsr. However, cxsr disabling was later >> > moved to intel_pre_disable_primary() in commit 87d4300a7dbc ("drm/i915: >> > Move intel_(pre_disable/post_enable)_primary to intel_display.c, and use >> > it there.") and that function got its own vblank wait for cxsr in commit >> > 262cd2e154c2 ("drm/i915: CHV DDR DVFS support and another watermark >> > rewrite"). So remove the extra vblank wait from i9xx_crtc_distable(). >> > >> > Cc: Kalyan Kondapally <kalyan.kondapally@intel.com> >> > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> >> >> Cc: fixes or stable? Fixes which commit exactly of all those? > > Why do we want to put a fix to remove a harmless roughly 16ms delay to > stable? Or do I miss something here? I missed something here. Nevermind, sorry for the noise. :) BR, Jani. > -Daniel > >> >> BR, >> Jani. >> >> >> > --- >> > drivers/gpu/drm/i915/intel_display.c | 5 ++--- >> > 1 file changed, 2 insertions(+), 3 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> > index 568eefc..3cb9383 100644 >> > --- a/drivers/gpu/drm/i915/intel_display.c >> > +++ b/drivers/gpu/drm/i915/intel_display.c >> > @@ -6317,10 +6317,9 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc) >> > /* >> > * On gen2 planes are double buffered but the pipe isn't, so we must >> > * wait for planes to fully turn off before disabling the pipe. >> > - * We also need to wait on all gmch platforms because of the >> > - * self-refresh mode constraint explained above. >> > */ >> > - intel_wait_for_vblank(dev, pipe); >> > + if (IS_GEN2(dev)) >> > + intel_wait_for_vblank(dev, pipe); >> > >> > for_each_encoder_on_crtc(dev, crtc, encoder) >> > encoder->disable(encoder); >> >> -- >> Jani Nikula, Intel Open Source Technology Center >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Feb 16, 2016 at 07:53:45PM +0200, Ander Conselvan de Oliveira wrote: > The wait for other gens was added in commit 564ed191f5d8 ("drm/i915: > gmch: fix stuck primary plane due to memory self-refresh mode") since > that's necessary when disabling cxsr. However, cxsr disabling was later > moved to intel_pre_disable_primary() in commit 87d4300a7dbc ("drm/i915: > Move intel_(pre_disable/post_enable)_primary to intel_display.c, and use > it there.") and that function got its own vblank wait for cxsr in commit > 262cd2e154c2 ("drm/i915: CHV DDR DVFS support and another watermark > rewrite"). So remove the extra vblank wait from i9xx_crtc_distable(). > > Cc: Kalyan Kondapally <kalyan.kondapally@intel.com> > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/intel_display.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 568eefc..3cb9383 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -6317,10 +6317,9 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc) > /* > * On gen2 planes are double buffered but the pipe isn't, so we must > * wait for planes to fully turn off before disabling the pipe. > - * We also need to wait on all gmch platforms because of the > - * self-refresh mode constraint explained above. > */ > - intel_wait_for_vblank(dev, pipe); > + if (IS_GEN2(dev)) > + intel_wait_for_vblank(dev, pipe); > > for_each_encoder_on_crtc(dev, crtc, encoder) > encoder->disable(encoder); > -- > 2.4.3 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, 2016-02-17 at 08:27 +0000, Patchwork wrote: > == Summary == > > Series 3496v1 drm/i915: Wait for vblank in i9xx_disable_crtc() for gen 2 only > http://patchwork.freedesktop.org/api/1.0/series/3496/revisions/1/mbox/ > > Test gem_ringfill: > Subgroup basic-default-hang: > incomplete -> PASS (snb-dellxps) > Test kms_flip: > Subgroup basic-flip-vs-modeset: > pass -> DMESG-WARN (ilk-hp8440p) UNSTABLE https://bugs.freedesktop.org/show_bug.cgi?id=93787 > Test kms_pipe_crc_basic: > Subgroup suspend-read-crc-pipe-a: > dmesg-warn -> PASS (skl-i5k-2) > Subgroup suspend-read-crc-pipe-b: > pass -> INCOMPLETE (hsw-gt2) I'm pretty sure this is a fluke. The code the patch changes doesn't even run on HSW. What is the normal procedure for this kind of issues? > Subgroup suspend-read-crc-pipe-c: > pass -> DMESG-WARN (skl-i5k-2) https://bugs.freedesktop.org/show_bug.cgi?id=93294 > dmesg-warn -> PASS (bsw-nuc-2) > Test pm_rpm: > Subgroup basic-pci-d3-state: > fail -> PASS (bdw-nuci7) > Subgroup basic-rte: > dmesg-warn -> PASS (bsw-nuc-2) > > bdw-nuci7 total:162 pass:152 dwarn:0 dfail:0 fail:0 skip:10 > bdw-ultra total:165 pass:152 dwarn:0 dfail:0 fail:0 skip:13 > bsw-nuc-2 total:165 pass:136 dwarn:0 dfail:0 fail:0 skip:29 > byt-nuc total:165 pass:140 dwarn:1 dfail:0 fail:0 skip:24 > hsw-brixbox total:165 pass:151 dwarn:0 dfail:0 fail:0 skip:14 > hsw-gt2 total:127 pass:118 dwarn:0 dfail:0 fail:0 skip:8 > ilk-hp8440p total:165 pass:115 dwarn:1 dfail:0 fail:1 skip:48 > skl-i5k-2 total:165 pass:149 dwarn:1 dfail:0 fail:0 skip:15 > snb-dellxps total:165 pass:142 dwarn:0 dfail:0 fail:1 skip:22 > > Results at /archive/results/CI_IGT_test/Patchwork_1418/ > > bd0b1a9aa8b7fdb2e06a5cbf1756ef93de2fa3fd drm-intel-nightly: 2016y-02m-16d-17h > -53m-05s UTC integration manifest > 3623c71eeb82092a0774b8b959a6a367ffca46c2 drm/i915: Wait for vblank in > i9xx_disable_crtc() for gen 2 only >
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 568eefc..3cb9383 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -6317,10 +6317,9 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc) /* * On gen2 planes are double buffered but the pipe isn't, so we must * wait for planes to fully turn off before disabling the pipe. - * We also need to wait on all gmch platforms because of the - * self-refresh mode constraint explained above. */ - intel_wait_for_vblank(dev, pipe); + if (IS_GEN2(dev)) + intel_wait_for_vblank(dev, pipe); for_each_encoder_on_crtc(dev, crtc, encoder) encoder->disable(encoder);
The wait for other gens was added in commit 564ed191f5d8 ("drm/i915: gmch: fix stuck primary plane due to memory self-refresh mode") since that's necessary when disabling cxsr. However, cxsr disabling was later moved to intel_pre_disable_primary() in commit 87d4300a7dbc ("drm/i915: Move intel_(pre_disable/post_enable)_primary to intel_display.c, and use it there.") and that function got its own vblank wait for cxsr in commit 262cd2e154c2 ("drm/i915: CHV DDR DVFS support and another watermark rewrite"). So remove the extra vblank wait from i9xx_crtc_distable(). Cc: Kalyan Kondapally <kalyan.kondapally@intel.com> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)