| Message ID | 20210312133430.1478156-1-gwan-gyeong.mun@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/display/psr: Add sink not reliable check to intel_psr_work() | expand |
On Fri, 2021-03-12 at 15:34 +0200, Gwan-gyeong Mun wrote: > If the sink state is not reliable, it does not need to wait for > PSR "IDLE state" for re-enabling PSR. And it should not try to re-enable > PSR. > > Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> > --- > drivers/gpu/drm/i915/display/intel_psr.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c > index cd434285e3b7..7f555407de06 100644 > --- a/drivers/gpu/drm/i915/display/intel_psr.c > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > @@ -1686,6 +1686,9 @@ static void intel_psr_work(struct work_struct *work) > if (READ_ONCE(intel_dp->psr.irq_aux_error)) > intel_psr_handle_irq(intel_dp); > > > > > + if (intel_dp->psr.sink_not_reliable) > + goto unlock; I can't think any scenario that this will hit. Before set sink_not_reliable PSR will be disabled so it will be caught in the first check of intel_psr_work(). > + > /* > * We have to make sure PSR is ready for re-enable > * otherwise it keeps disabled until next full enable/disable cycle.
On Fri, 2021-03-12 at 05:37 -0800, Souza, Jose wrote: > On Fri, 2021-03-12 at 15:34 +0200, Gwan-gyeong Mun wrote: > > If the sink state is not reliable, it does not need to wait for > > PSR "IDLE state" for re-enabling PSR. And it should not try to re- > > enable > > PSR. > > > > Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_psr.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c > > b/drivers/gpu/drm/i915/display/intel_psr.c > > index cd434285e3b7..7f555407de06 100644 > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > @@ -1686,6 +1686,9 @@ static void intel_psr_work(struct work_struct > > *work) > > if (READ_ONCE(intel_dp->psr.irq_aux_error)) > > intel_psr_handle_irq(intel_dp); > > > > > > > > 1. In intel_psr_irq_handler() when the psr error happens, intel_dp->psr.irq_aux_error = true; schedule_work(&intel_dp->psr.work); 2. In intel_psr_work() ... if (READ_ONCE(intel_dp->psr.irq_aux_error)) intel_psr_handle_irq(intel_dp); -> intel_psr_disable_locked(intel_dp); psr->sink_not_reliable = true; ... IMO, when this scenario happens, the below code seems to be needed. > > + if (intel_dp->psr.sink_not_reliable) > > + goto unlock; > > I can't think any scenario that this will hit. > Before set sink_not_reliable PSR will be disabled so it will be caught > in the first check of intel_psr_work(). > > > + > > /* > > * We have to make sure PSR is ready for re-enable > > * otherwise it keeps disabled until next full enable/disable > > cycle. >
On Sat, 2021-03-13 at 20:01 +0000, Mun, Gwan-gyeong wrote: > On Fri, 2021-03-12 at 05:37 -0800, Souza, Jose wrote: > > On Fri, 2021-03-12 at 15:34 +0200, Gwan-gyeong Mun wrote: > > > If the sink state is not reliable, it does not need to wait for > > > PSR "IDLE state" for re-enabling PSR. And it should not try to re- > > > enable > > > PSR. > > > > > > Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_psr.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c > > > b/drivers/gpu/drm/i915/display/intel_psr.c > > > index cd434285e3b7..7f555407de06 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > > @@ -1686,6 +1686,9 @@ static void intel_psr_work(struct work_struct > > > *work) > > > if (READ_ONCE(intel_dp->psr.irq_aux_error)) > > > intel_psr_handle_irq(intel_dp); > > > > > > > > > > > > > 1. In intel_psr_irq_handler() > when the psr error happens, > intel_dp->psr.irq_aux_error = true; > schedule_work(&intel_dp->psr.work); > > > 2. In intel_psr_work() > ... > if (READ_ONCE(intel_dp->psr.irq_aux_error)) > intel_psr_handle_irq(intel_dp); > -> intel_psr_disable_locked(intel_dp); > psr->sink_not_reliable = true; > ... > > > IMO, when this scenario happens, the below code seems to be needed. Information like this should be in the commit message. Do not add another check, just do a goto close to the function that handles irq_aux_error. > > > > + if (intel_dp->psr.sink_not_reliable) > > > + goto unlock; > > > > I can't think any scenario that this will hit. > > Before set sink_not_reliable PSR will be disabled so it will be caught > > in the first check of intel_psr_work(). > > > > > + > > > /* > > > * We have to make sure PSR is ready for re-enable > > > * otherwise it keeps disabled until next full enable/disable > > > cycle. > > >
diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c index cd434285e3b7..7f555407de06 100644 --- a/drivers/gpu/drm/i915/display/intel_psr.c +++ b/drivers/gpu/drm/i915/display/intel_psr.c @@ -1686,6 +1686,9 @@ static void intel_psr_work(struct work_struct *work) if (READ_ONCE(intel_dp->psr.irq_aux_error)) intel_psr_handle_irq(intel_dp); + if (intel_dp->psr.sink_not_reliable) + goto unlock; + /* * We have to make sure PSR is ready for re-enable * otherwise it keeps disabled until next full enable/disable cycle.
If the sink state is not reliable, it does not need to wait for PSR "IDLE state" for re-enabling PSR. And it should not try to re-enable PSR. Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> --- drivers/gpu/drm/i915/display/intel_psr.c | 3 +++ 1 file changed, 3 insertions(+)