[Intel-gfx] [PATCH 1/4] drm/i915: Delay first PSR activation.

[R, Durgadoss]

>-----Original Message-----
>From: Vivi, Rodrigo
>Sent: Friday, November 13, 2015 3:08 AM
>To: intel-gfx at lists.freedesktop.org; R, Durgadoss
>Subject: Re: [Intel-gfx] [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 at lists.freedesktop.org] On
>> > Behalf Of Rodrigo Vivi
>> > Sent: Thursday, November 12, 2015 1:07 AM
>> > To: intel-gfx at lists.freedesktop.org
>> > Cc: Vivi, Rodrigo
>> > Subject: [Intel-gfx] [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 at intel.com>
>> > ---
>> > 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 at intel.com>

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 at lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx