Re: [PATCH 2/2] drm/i915: VLV/CHV PSR: Increase wait delay time before active PSR.

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Dec 12, 2014 at 07:16:34PM -0800, Rodrigo Vivi wrote:
> On Mon, Dec 8, 2014 at 1:35 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Fri, Dec 05, 2014 at 08:40:42PM -0500, Rodrigo Vivi wrote:
> >> Since active function on VLV immediately activate PSR let's give more
> >> time for idleness.
> >>
> >> v2: Rebase over intel_psr.c and fix typo.
> >> v3: Revival: Manual tests indicated that this is needed. With a short delay there is a huge
> >>     risk of getting blank screens when planes are being enabled.
> >>
> >> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >> Reviewed-by: Durgadoss R <durgadoss.r@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_psr.c | 8 +++++++-
> >>  1 file changed, 7 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> >> index afb8b8c..a6028b6 100644
> >> --- a/drivers/gpu/drm/i915/intel_psr.c
> >> +++ b/drivers/gpu/drm/i915/intel_psr.c
> >> @@ -597,6 +597,12 @@ void intel_psr_flush(struct drm_device *dev,
> >>       struct drm_i915_private *dev_priv = dev->dev_private;
> >>       struct drm_crtc *crtc;
> >>       enum pipe pipe;
> >> +     /* On HSW/BDW Hardware controls idle_frames to go to PSR entry state
> >> +      * However on VLV we go to PSR active state with psr work. So let's
> >> +      * wait more time. The main reason is to give more time when primary
> >> +      * plane is getting enabled avoiding blank screens.
> >> +      */
> >> +     int delay = msecs_to_jiffies(HAS_DDI(dev) ? 100 : 5000);
> >
> > Same question as before: Shouldn't we look at vbt perhaps like on ddi
> > platforms to compute a reasonable delay? 5s is kinda not reasonable I
> > think ;-)
> 
> I agree 5s is an eternity here... to be honest I was using 1 sec here
> but when sending the patch I reused the one I had sent before without
> noticing it was 5.
> Anyway, what kind of value at vbt do you suggest? If we use the tp1
> wake for instance that is 50 here in multiple of 100 it will end up in
> 5 sec again.

Yeah that's the one I was thinking off. So maybe 5s is indeed the right
value, but we should compute it from vbt?

> Another thing that I notice today is that I also got that sink crc
> issue on BDW. So it isn't only on VLV/CHV that we are unable to use
> sink crc for test. I still believe that is the link fully off that is
> making us to be unable to get panel crc when in psr.

Hm, maybe we need a special mode to disallow link shutdown for the psr
test? Then we could run all the functional correctness tests in that mode
(psr still works the same wrt frontbuffer tracking). Ofc that means we
need a new set of tests to exercise full psr (including link shutdown)
against e.g. suspend/resume and stuff to make sure that still works. But
those are then only doing modesets, s/r and won't check crcs. So
additional tests on top of what we have.

> But there is hope. I tested different values here and if we use 400 ms
> I could use automated kms_psr_sink_crc here... with 300 and bellow it
> fails.
> The issue is that I'm not sure if it depends on different panel so
> I'll test this value on other platforms and different panels.
> 
> Do you think 500ms reasonable to apply for all platforms in order to
> keep automated tests and everything working well?
> 
> What do you think?

Imo if vbt provides a delay, we should follow that. Even when it's
horrible, who knows what kind of broken panels we'll see ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx