diff mbox

[v3,2/4] drm/i915/psr: Prevent PSR exit when a non-pipe related register is written

Message ID 20180420222758.6168-2-jose.souza@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Souza, Jose April 20, 2018, 10:27 p.m. UTC 
Any write in any display register was causing HW to exit PSR,
masking it to allow more power savings. Writes to pipe related
registers will still cause HW to exit PSR.
This is already masked for PSR2.

Bspec: 7721 and 8042

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---

New patch in this series.

 drivers/gpu/drm/i915/intel_psr.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Rodrigo Vivi April 20, 2018, 10:57 p.m. UTC | #1 
On Fri, Apr 20, 2018 at 03:27:56PM -0700, José Roberto de Souza wrote:
> Any write in any display register was causing HW to exit PSR,
> masking it to allow more power savings. Writes to pipe related
> registers will still cause HW to exit PSR.
> This is already masked for PSR2.

This seems a good idea indeed with the test case on perspective.

But it needs more tests to make sure it doesn't break
"Display WA #0884: all"

Or we might need to revert that patch before moving with this idea.

> 
> Bspec: 7721 and 8042
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
> 
> New patch in this series.
> 
>  drivers/gpu/drm/i915/intel_psr.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 0938df48107a..c907282dc82d 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -712,7 +712,8 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
>  		I915_WRITE(EDP_PSR_DEBUG,
>  			   EDP_PSR_DEBUG_MASK_MEMUP |
>  			   EDP_PSR_DEBUG_MASK_HPD |
> -			   EDP_PSR_DEBUG_MASK_LPSP);
> +			   EDP_PSR_DEBUG_MASK_LPSP |
> +			   EDP_PSR_DEBUG_MASK_DISP_REG_WRITE);
>  	}
>  }
>  
> -- 
> 2.17.0
>
Rodrigo Vivi April 20, 2018, 10:58 p.m. UTC | #2 
On Fri, Apr 20, 2018 at 03:27:56PM -0700, José Roberto de Souza wrote:
> Any write in any display register was causing HW to exit PSR,
> masking it to allow more power savings. Writes to pipe related
> registers will still cause HW to exit PSR.
> This is already masked for PSR2.
> 
> Bspec: 7721 and 8042
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
> 
> New patch in this series.
> 
>  drivers/gpu/drm/i915/intel_psr.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 0938df48107a..c907282dc82d 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -712,7 +712,8 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
>  		I915_WRITE(EDP_PSR_DEBUG,
>  			   EDP_PSR_DEBUG_MASK_MEMUP |
>  			   EDP_PSR_DEBUG_MASK_HPD |
> -			   EDP_PSR_DEBUG_MASK_LPSP);
> +			   EDP_PSR_DEBUG_MASK_LPSP |
> +			   EDP_PSR_DEBUG_MASK_DISP_REG_WRITE);

What about only setting this bit before debugfs reads
and unseting it after that?


>  	}
>  }
>  
> -- 
> 2.17.0
>
Souza, Jose April 24, 2018, 12:42 a.m. UTC | #3 
On Fri, 2018-04-20 at 15:57 -0700, Rodrigo Vivi wrote:
> On Fri, Apr 20, 2018 at 03:27:56PM -0700, José Roberto de Souza

> wrote:

> > Any write in any display register was causing HW to exit PSR,

> > masking it to allow more power savings. Writes to pipe related

> > registers will still cause HW to exit PSR.

> > This is already masked for PSR2.

> 

> This seems a good idea indeed with the test case on perspective.


what test cases are thinking? the current ones already do pages flips
that will only touch the pipe related registers.

> 

> But it needs more tests to make sure it doesn't break

> "Display WA #0884: all"


I just tested the WA #0884 and it still causes PSR to exit. I have
added a new debugfs that when read it writes to
'I915_WRITE(CURSURFLIVE(pipe), 0);' causing a PSR exit.

> 

> Or we might need to revert that patch before moving with this idea.

> 

> > 

> > Bspec: 7721 and 8042

> > 

> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>

> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>

> > ---

> > 

> > New patch in this series.

> > 

> >  drivers/gpu/drm/i915/intel_psr.c | 3 ++-

> >  1 file changed, 2 insertions(+), 1 deletion(-)

> > 

> > diff --git a/drivers/gpu/drm/i915/intel_psr.c

> > b/drivers/gpu/drm/i915/intel_psr.c

> > index 0938df48107a..c907282dc82d 100644

> > --- a/drivers/gpu/drm/i915/intel_psr.c

> > +++ b/drivers/gpu/drm/i915/intel_psr.c

> > @@ -712,7 +712,8 @@ static void hsw_psr_enable_source(struct

> > intel_dp *intel_dp,

> >  		I915_WRITE(EDP_PSR_DEBUG,

> >  			   EDP_PSR_DEBUG_MASK_MEMUP |

> >  			   EDP_PSR_DEBUG_MASK_HPD |

> > -			   EDP_PSR_DEBUG_MASK_LPSP);

> > +			   EDP_PSR_DEBUG_MASK_LPSP |

> > +			   EDP_PSR_DEBUG_MASK_DISP_REG_WRITE);

> >  	}

> >  }

> >  

> > -- 

> > 2.17.0

> >
Rodrigo Vivi April 24, 2018, 9:20 p.m. UTC | #4 
On Mon, Apr 23, 2018 at 05:42:40PM -0700, Souza, Jose wrote:
> On Fri, 2018-04-20 at 15:57 -0700, Rodrigo Vivi wrote:
> > On Fri, Apr 20, 2018 at 03:27:56PM -0700, José Roberto de Souza
> > wrote:
> > > Any write in any display register was causing HW to exit PSR,
> > > masking it to allow more power savings. Writes to pipe related
> > > registers will still cause HW to exit PSR.
> > > This is already masked for PSR2.
> > 
> > This seems a good idea indeed with the test case on perspective.
> 
> what test cases are thinking? the current ones already do pages flips
> that will only touch the pipe related registers.
> 
> > 
> > But it needs more tests to make sure it doesn't break
> > "Display WA #0884: all"
> 
> I just tested the WA #0884 and it still causes PSR to exit. I have
> added a new debugfs that when read it writes to
> 'I915_WRITE(CURSURFLIVE(pipe), 0);' causing a PSR exit.

Interesting. Thanks a lot for checking this.
I guess we are safe then. DK?!

> 
> > 
> > Or we might need to revert that patch before moving with this idea.
> > 
> > > 
> > > Bspec: 7721 and 8042
> > > 
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > > 
> > > New patch in this series.
> > > 
> > >  drivers/gpu/drm/i915/intel_psr.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index 0938df48107a..c907282dc82d 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -712,7 +712,8 @@ static void hsw_psr_enable_source(struct
> > > intel_dp *intel_dp,
> > >  		I915_WRITE(EDP_PSR_DEBUG,
> > >  			   EDP_PSR_DEBUG_MASK_MEMUP |
> > >  			   EDP_PSR_DEBUG_MASK_HPD |
> > > -			   EDP_PSR_DEBUG_MASK_LPSP);
> > > +			   EDP_PSR_DEBUG_MASK_LPSP |
> > > +			   EDP_PSR_DEBUG_MASK_DISP_REG_WRITE);
> > >  	}
> > >  }
> > >  
> > > -- 
> > > 2.17.0
> > >
Dhinakaran Pandiyan April 25, 2018, 12:16 a.m. UTC | #5 
On Tue, 2018-04-24 at 14:20 -0700, Rodrigo Vivi wrote:
> On Mon, Apr 23, 2018 at 05:42:40PM -0700, Souza, Jose wrote:
> > On Fri, 2018-04-20 at 15:57 -0700, Rodrigo Vivi wrote:
> > > On Fri, Apr 20, 2018 at 03:27:56PM -0700, José Roberto de Souza
> > > wrote:
> > > > Any write in any display register was causing HW to exit PSR,
> > > > masking it to allow more power savings. Writes to pipe related
> > > > registers will still cause HW to exit PSR.
> > > > This is already masked for PSR2.
> > > 
> > > This seems a good idea indeed with the test case on perspective.
> > 
> > what test cases are thinking? the current ones already do pages flips
> > that will only touch the pipe related registers.
> > 
> > > 
> > > But it needs more tests to make sure it doesn't break
> > > "Display WA #0884: all"
> > 
> > I just tested the WA #0884 and it still causes PSR to exit. I have
> > added a new debugfs that when read it writes to
> > 'I915_WRITE(CURSURFLIVE(pipe), 0);' causing a PSR exit.
> 
> Interesting. Thanks a lot for checking this.
> I guess we are safe then. DK?!
> 

Not sure why it works though, let's give it a try.

It might be worth adding more information about test platform and how
was this was tested in the commit message. Since this is not clearly
documented in bspec, someone is going read this in a few months and
think the code does not make any sense.

Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>


> > 
> > > 
> > > Or we might need to revert that patch before moving with this idea.
> > > 
> > > > 
> > > > Bspec: 7721 and 8042
> > > > 
> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > ---
> > > > 
> > > > New patch in this series.
> > > > 
> > > >  drivers/gpu/drm/i915/intel_psr.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > index 0938df48107a..c907282dc82d 100644
> > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > @@ -712,7 +712,8 @@ static void hsw_psr_enable_source(struct
> > > > intel_dp *intel_dp,
> > > >  		I915_WRITE(EDP_PSR_DEBUG,
> > > >  			   EDP_PSR_DEBUG_MASK_MEMUP |
> > > >  			   EDP_PSR_DEBUG_MASK_HPD |
> > > > -			   EDP_PSR_DEBUG_MASK_LPSP);
> > > > +			   EDP_PSR_DEBUG_MASK_LPSP |
> > > > +			   EDP_PSR_DEBUG_MASK_DISP_REG_WRITE);
> > > >  	}
> > > >  }
> > > >  
> > > > -- 
> > > > 2.17.0
> > > > 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Souza, Jose April 25, 2018, 8:37 p.m. UTC | #6 
On Tue, 2018-04-24 at 17:16 -0700, Dhinakaran Pandiyan wrote:
> 

> 

> On Tue, 2018-04-24 at 14:20 -0700, Rodrigo Vivi wrote:

> > On Mon, Apr 23, 2018 at 05:42:40PM -0700, Souza, Jose wrote:

> > > On Fri, 2018-04-20 at 15:57 -0700, Rodrigo Vivi wrote:

> > > > On Fri, Apr 20, 2018 at 03:27:56PM -0700, José Roberto de Souza

> > > > wrote:

> > > > > Any write in any display register was causing HW to exit PSR,

> > > > > masking it to allow more power savings. Writes to pipe

> > > > > related

> > > > > registers will still cause HW to exit PSR.

> > > > > This is already masked for PSR2.

> > > > 

> > > > This seems a good idea indeed with the test case on

> > > > perspective.

> > > 

> > > what test cases are thinking? the current ones already do pages

> > > flips

> > > that will only touch the pipe related registers.

> > > 

> > > > 

> > > > But it needs more tests to make sure it doesn't break

> > > > "Display WA #0884: all"

> > > 

> > > I just tested the WA #0884 and it still causes PSR to exit. I

> > > have

> > > added a new debugfs that when read it writes to

> > > 'I915_WRITE(CURSURFLIVE(pipe), 0);' causing a PSR exit.

> > 

> > Interesting. Thanks a lot for checking this.

> > I guess we are safe then. DK?!

> > 

> 

> Not sure why it works though, let's give it a try.

> 

> It might be worth adding more information about test platform and how

> was this was tested in the commit message. Since this is not clearly

> documented in bspec, someone is going read this in a few months and

> think the code does not make any sense.

> 

> Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>


Done

> 

> 

> > > 

> > > > 

> > > > Or we might need to revert that patch before moving with this

> > > > idea.

> > > > 

> > > > > 

> > > > > Bspec: 7721 and 8042

> > > > > 

> > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>

> > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

> > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>

> > > > > ---

> > > > > 

> > > > > New patch in this series.

> > > > > 

> > > > >  drivers/gpu/drm/i915/intel_psr.c | 3 ++-

> > > > >  1 file changed, 2 insertions(+), 1 deletion(-)

> > > > > 

> > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c

> > > > > b/drivers/gpu/drm/i915/intel_psr.c

> > > > > index 0938df48107a..c907282dc82d 100644

> > > > > --- a/drivers/gpu/drm/i915/intel_psr.c

> > > > > +++ b/drivers/gpu/drm/i915/intel_psr.c

> > > > > @@ -712,7 +712,8 @@ static void hsw_psr_enable_source(struct

> > > > > intel_dp *intel_dp,

> > > > >  		I915_WRITE(EDP_PSR_DEBUG,

> > > > >  			   EDP_PSR_DEBUG_MASK_MEMUP |

> > > > >  			   EDP_PSR_DEBUG_MASK_HPD |

> > > > > -			   EDP_PSR_DEBUG_MASK_LPSP);

> > > > > +			   EDP_PSR_DEBUG_MASK_LPSP |

> > > > > +			   EDP_PSR_DEBUG_MASK_DISP_REG_WRITE

> > > > > );

> > > > >  	}

> > > > >  }

> > > > >  

> > > > > -- 

> > > > > 2.17.0

> > > > > 

> > 

> > _______________________________________________

> > Intel-gfx mailing list

> > Intel-gfx@lists.freedesktop.org

> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

> 

>
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 0938df48107a..c907282dc82d 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -712,7 +712,8 @@  static void hsw_psr_enable_source(struct intel_dp *intel_dp,
 		I915_WRITE(EDP_PSR_DEBUG,
 			   EDP_PSR_DEBUG_MASK_MEMUP |
 			   EDP_PSR_DEBUG_MASK_HPD |
-			   EDP_PSR_DEBUG_MASK_LPSP);
+			   EDP_PSR_DEBUG_MASK_LPSP |
+			   EDP_PSR_DEBUG_MASK_DISP_REG_WRITE);
 	}
 }