>-----Original Message----- >From: Vivi, Rodrigo >Sent: Friday, November 13, 2015 3:08 AM >To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; R, Durgadoss >Subject: Re: [PATCH 1/4] drm/i915: Delay first PSR activation. > >On Thu, 2015-11-12 at 13:50 +0000, R, Durgadoss wrote: >> Hi Rodrigo, >> >> > -----Original Message----- >> > From: Intel-gfx [mailto:intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx] On >> > Behalf Of Rodrigo Vivi >> > Sent: Thursday, November 12, 2015 1:07 AM >> > To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx >> > Cc: Vivi, Rodrigo >> > Subject: [PATCH 1/4] drm/i915: Delay first PSR >> > activation. >> > >> > When debuging the frozen screen caused by HW tracking with low >> > power state I noticed that if we keep moving the mouse non stop >> > you will miss the screen updates for a while. At least >> > until we stop moving the mouse for a small time and move again. >> > >> > The actual enabling should happen immediately after >> > Display Port enabling sequence finished with links trained and >> > everything enabled. However we face many issues when enabling PSR >> > right after a modeset. >> > >> > On VLV/CHV we face blank screens on this scenario and on HSW+ >> > we face a recoverable frozen screen, at least until next >> > exit-activate sequence. >> > >> > Another workaround for the same issue here would be to increase >> > re-enable idle time from 100 to 500 as we did for VLV/CHV. >> > However this patch workaround this issue in a better >> > way since it doesn't reduce PSR residency and also >> > allow us to reduce the delay time between re-enables at least >> > on VLV/CHV. >> > >> > This is also important to make the sysfs toggle working properly. >> > >> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> >> > --- >> > drivers/gpu/drm/i915/intel_psr.c | 18 ++++++++++++++++-- >> > 1 file changed, 16 insertions(+), 2 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/intel_psr.c >> > b/drivers/gpu/drm/i915/intel_psr.c >> > index 213581c..6b24c24 100644 >> > --- a/drivers/gpu/drm/i915/intel_psr.c >> > +++ b/drivers/gpu/drm/i915/intel_psr.c >> > @@ -427,6 +427,19 @@ void intel_psr_enable(struct intel_dp >> > *intel_dp) >> > vlv_psr_enable_source(intel_dp); >> > } >> > >> > + /* >> > + * FIXME: Activation should happen immediately since this >> > function >> > + * is just called after pipe is fully trained and enabled. >> > + * However on every platform we face issues when first >> > activation >> > + * follows a modeset so quickly. >> > + * - On VLV/CHV we get bank screen on first activation >> > + * - On HSW/BDW we get a recoverable frozen screen >> > until next >> > + * exit-activate sequence. >> > + */ >> > + if (INTEL_INFO(dev)->gen < 9) >> > + schedule_delayed_work(&dev_priv->psr.work, >> > + msecs_to_jiffies(intel_dp >> > ->panel_power_cycle_delay * 5)); >> > + >> > dev_priv->psr.enabled = intel_dp; Should we set this before scheduling the delayed work ? >> > unlock: >> > mutex_unlock(&dev_priv->psr.lock); >> > @@ -735,8 +748,9 @@ void intel_psr_flush(struct drm_device *dev, >> > } >> > >> > if (!dev_priv->psr.active && !dev_priv >> > ->psr.busy_frontbuffer_bits) >> > - schedule_delayed_work(&dev_priv->psr.work, >> > - msecs_to_jiffies(delay_ms)); >> > + if (!work_busy(&dev_priv->psr.work.work)) >> > + schedule_delayed_work(&dev_priv->psr.work, >> > + >> > msecs_to_jiffies(delay_ms)); >> >> Agree with the theory of the patch as such.. But, Is there any >> specific reason for >> the !work_busy() check here ? >> >> I believe when the later work runs, it will anyway bail out in >> _activate >> function, if it sees PSR_ENABLE bit set already. So, is this check >> just to >> prevent scheduling one more work item when there is one pending >> already ? (or it helps in something else also ?) > >The !work_busy is to prevent that eventual _activate call reduce the >first activation time. Yes, this is what I understood from the code. Just wanted to confirm whether you meant the same. The other thing I am thinking is: Inside intel_psr_enable() we call _activate() for DDI platforms & gen>=9. For others, we schedule a work. May be we should have only the worker thread do _activate() for every Platform .. I believe this would simplify things a lot. Not sure whether this Will impact gen>=9 platforms in any way.. Anyway, we can have that as a separate change if required and valid. So, for this patch: Reviewed-by: Durgadoss R <durgadoss.r@xxxxxxxxx> Thanks, Durga > >for instance: > >0s - we enable and schedule first activation to 2.5s >1s - we got a page flip that flushed fb tracking and called >psr_activation to 0.1s >1.1s - psr is activated > >while we want > >0s - we enable and schedule first activation to 2.5s >1s - we got a page >flip that flushed fb tracking and called psr_activation to 0.1s # just >ignore and move ahead since we are going to activate it soon. >2.5s - psr >is activated > >I'm open to hear ideas to make it better or more clear. > > >> >> Thanks, >> Durga > >Thank you very much for all the reviews! > >> >> > mutex_unlock(&dev_priv->psr.lock); >> > } >> > >> > -- >> > 2.4.3 >> > >> > _______________________________________________ >> > Intel-gfx mailing list >> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx >> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
