[PATCH] drm/i915: Wait for vblank in i9xx_disable_crtc() for gen 2 only

[PATCH] drm/i915: Wait for vblank in i9xx_disable_crtc() for gen 2 only

[Ander Conselvan de Oliveira]

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(-)

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

✗ Fi.CI.BAT: failure for drm/i915: Wait for vblank in i9xx_disable_crtc() for gen 2 only

[Patchwork]

== 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
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)
        Subgroup suspend-read-crc-pipe-c:
                pass       -> DMESG-WARN (skl-i5k-2)
                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

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

Re: [PATCH] drm/i915: Wait for vblank in i9xx_disable_crtc() for gen 2 only

[Jani Nikula]

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

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

Re: [PATCH] drm/i915: Wait for vblank in i9xx_disable_crtc() for gen 2 only

[Ander Conselvan De Oliveira]

[-- Attachment #1: Type: text/plain, Size: 2521 bytes --]

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

[-- Attachment #2: vlcsnap-2016-02-17-14h25m02s857.png --]
[-- Type: image/png, Size: 1436467 bytes --]

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH] drm/i915: Wait for vblank in i9xx_disable_crtc() for gen 2 only

[Daniel Vetter]

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

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

Re: [PATCH] drm/i915: Wait for vblank in i9xx_disable_crtc() for gen 2 only

[Jani Nikula]

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

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

Re: [PATCH] drm/i915: Wait for vblank in i9xx_disable_crtc() for gen 2 only

[Daniel Vetter]

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

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

Re: ✗ Fi.CI.BAT: failure for drm/i915: Wait for vblank in i9xx_disable_crtc() for gen 2 only

[Ander Conselvan De Oliveira]

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