On Tue, May 18, 2021 at 09:33:09AM +0000, Mun, Gwan-gyeong wrote:
> Hi Ville,
> initially, intel_psr_pause() called intel_psr_disable_locked() instead
> of intel_psr_exit().
> In intel_psr_resume(), _intel_psr_enable_locked() was called instead of
> intel_psr_activate().
> Can you share what problem the initial code caused when calling
> intel_psr_pause() / intel_psr_resume()?
It was doing illegal stuff with crtc->state/etc. That was oopsing.
The other problem was that IIRC it was going to do DPCD accesses
while the cdclk code was already holding the aux mutexes. I moved it
out from under the lock, but I think we might actually want it inside
the lock since we'll need that to prevent PSR during all AUX transfers
anyway. Putting it back inside the lock should also make it less racy
I guess.
>
> In addition, intel_psr_exit() /intel_psr_activate() function disable /
> enable only the PSR source.
> So, if disable/enable for PSR Sink Device is not called together, there
> will be a problem that the PSR state machine of sink and source is
> different.
> What do you think?
If possible I wouldn't want it touch the sink at all. It should
basically be no different to eg. enabling the vblank interrupt.
>
> On Mon, 2021-05-17 at 09:58 -0700, Souza, Jose wrote:
> > On Fri, 2021-05-14 at 20:10 -0700, Matt Roper wrote:
> > > From: Gwan-gyeong Mun <gwan-gyeong.mun@xxxxxxxxx>
> > >
> > > This introduces the following function that can enable and disable
> > > psr
> > > without intel_crtc_state/drm_connector_state when intel_psr is
> > > already
> > > enabled with current intel_crtc_state and drm_connector_state
> > > information.
> > >
> > > - intel_psr_pause(): Pause current PSR. it deactivates current psr
> > > state.
> > > - intel_psr_resume(): Resume paused PSR without intel_crtc_state and
> > > drm_connector_state. It activates paused psr
> > > state.
> > >
> > > Cc: José Roberto de Souza <jose.souza@xxxxxxxxx>
> > > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@xxxxxxxxx>
> > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@xxxxxxxxx>
> > > Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
> > > ---
> > > .../drm/i915/display/intel_display_types.h | 1 +
> > > drivers/gpu/drm/i915/display/intel_psr.c | 93 ++++++++++++++++-
> > > --
> > > drivers/gpu/drm/i915/display/intel_psr.h | 2 +
> > > 3 files changed, 82 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > index b8d1f702d808..ee7cbdd7db87 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > @@ -1482,6 +1482,7 @@ struct intel_psr {
> > > bool sink_support;
> > > bool source_support;
> > > bool enabled;
> > > + bool paused;
> > > enum pipe pipe;
> > > enum transcoder transcoder;
> > > bool active;
> > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > index 4a63d10876ce..d4df3f5e7918 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > @@ -1060,34 +1060,23 @@ static bool psr_interrupt_error_check(struct
> > > intel_dp *intel_dp)
> > > return true;
> > > }
> > >
> > > -static void intel_psr_enable_locked(struct intel_dp *intel_dp,
> > > - const struct intel_crtc_state
> > > *crtc_state,
> > > - const struct drm_connector_state
> > > *conn_state)
> > > +static void _intel_psr_enable_locked(struct intel_dp *intel_dp,
> > > + const struct intel_crtc_state
> > > *crtc_state)
> > > {
> > > struct intel_digital_port *dig_port =
> > > dp_to_dig_port(intel_dp);
> > > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > > struct intel_encoder *encoder = &dig_port->base;
> > > - u32 val;
> > >
> > > drm_WARN_ON(&dev_priv->drm, intel_dp->psr.enabled);
> > >
> > > - intel_dp->psr.psr2_enabled = crtc_state->has_psr2;
> > > intel_dp->psr.busy_frontbuffer_bits = 0;
> > > - intel_dp->psr.pipe = to_intel_crtc(crtc_state->uapi.crtc)-
> > > >pipe;
> > > - intel_dp->psr.transcoder = crtc_state->cpu_transcoder;
> > > - /* DC5/DC6 requires at least 6 idle frames */
> > > - val = usecs_to_jiffies(intel_get_frame_time_us(crtc_state) *
> > > 6);
> > > - intel_dp->psr.dc3co_exit_delay = val;
> > > - intel_dp->psr.dc3co_exitline = crtc_state->dc3co_exitline;
> > > - intel_dp->psr.psr2_sel_fetch_enabled = crtc_state-
> > > >enable_psr2_sel_fetch;
> > >
> > > if (!psr_interrupt_error_check(intel_dp))
> > > return;
> > >
> > > drm_dbg_kms(&dev_priv->drm, "Enabling PSR%s\n",
> > > intel_dp->psr.psr2_enabled ? "2" : "1");
> > > - intel_dp_compute_psr_vsc_sdp(intel_dp, crtc_state,
> > > conn_state,
> > > - &intel_dp->psr.vsc);
> > > +
> > > intel_write_dp_vsc_sdp(encoder, crtc_state, &intel_dp-
> > > >psr.vsc);
> > > intel_psr_enable_sink(intel_dp);
> > > intel_psr_enable_source(intel_dp);
> > > @@ -1096,6 +1085,28 @@ static void intel_psr_enable_locked(struct
> > > intel_dp *intel_dp,
> > > intel_psr_activate(intel_dp);
> > > }
> > >
> > > +static void intel_psr_enable_locked(struct intel_dp *intel_dp,
> > > + const struct intel_crtc_state
> > > *crtc_state,
> > > + const struct drm_connector_state
> > > *conn_state)
> > > +{
> > > + u32 val;
> > > +
> > > + intel_dp->psr.psr2_enabled = crtc_state->has_psr2;
> > > + intel_dp->psr.pipe = to_intel_crtc(crtc_state->uapi.crtc)-
> > > >pipe;
> > > + intel_dp->psr.transcoder = crtc_state->cpu_transcoder;
> > > + /* DC5/DC6 requires at least 6 idle frames */
> > > + val = usecs_to_jiffies(intel_get_frame_time_us(crtc_state) *
> > > 6);
> > > + intel_dp->psr.dc3co_exit_delay = val;
> > > + intel_dp->psr.psr2_sel_fetch_enabled = crtc_state-
> > > >enable_psr2_sel_fetch;
> > > + intel_dp->psr.dc3co_exitline = crtc_state->dc3co_exitline;
> > > + intel_dp->psr.paused = false;
> > > +
> > > + intel_dp_compute_psr_vsc_sdp(intel_dp, crtc_state,
> > > conn_state,
> > > + &intel_dp->psr.vsc);
> > > +
> > > + _intel_psr_enable_locked(intel_dp, crtc_state);
> > > +}
> > > +
> > > /**
> > > * intel_psr_enable - Enable PSR
> > > * @intel_dp: Intel DP
> > > @@ -1233,6 +1244,60 @@ void intel_psr_disable(struct intel_dp
> > > *intel_dp,
> > > cancel_delayed_work_sync(&intel_dp->psr.dc3co_work);
> > > }
> > >
> > > +/**
> > > + * intel_psr_pause - Pause PSR
> > > + * @intel_dp: Intel DP
> > > + *
> > > + * This function need to be called after enabling psr.
> > > + */
> > > +void intel_psr_pause(struct intel_dp *intel_dp)
> > > +{
> > > + struct intel_psr *psr = &intel_dp->psr;
> > > +
> > > + if (!CAN_PSR(intel_dp))
> > > + return;
> > > +
> > > + mutex_lock(&psr->lock);
> > > +
> > > + if (!psr->active) {
> > > + mutex_unlock(&psr->lock);
> > > + return;
> > > + }
> > > +
> > > + intel_psr_exit(intel_dp);
> > > + psr->paused = true;
> > > +
> > > + mutex_unlock(&psr->lock);
> > > +
> > > + cancel_work_sync(&psr->work);
> > > + cancel_delayed_work_sync(&psr->dc3co_work);
> > > +}
> > > +
> > > +/**
> > > + * intel_psr_resume - Resume PSR
> > > + * @intel_dp: Intel DP
> > > + *
> > > + * This function need to be called after pausing psr.
> > > + */
> > > +void intel_psr_resume(struct intel_dp *intel_dp)
> > > +{
> > > + struct intel_psr *psr = &intel_dp->psr;
> > > +
> > > + if (!CAN_PSR(intel_dp))
> > > + return;
> > > +
> > > + mutex_lock(&psr->lock);
> > > +
> > > + if (!psr->paused)
> > > + goto unlock;
> > > +
> > > + psr->paused = false;
> > > + intel_psr_activate(intel_dp);
> >
> > This patch is doing a bunch of changes around the intel_psr_enable but
> > then it is calling intel_psr_activate().
> >
> > > +
> > > +unlock:
> > > + mutex_unlock(&psr->lock);
> > > +}
> > > +
> > > static void psr_force_hw_tracking_exit(struct intel_dp *intel_dp)
> > > {
> > > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.h
> > > b/drivers/gpu/drm/i915/display/intel_psr.h
> > > index e3db85e97f4c..641521b101c8 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_psr.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_psr.h
> > > @@ -51,5 +51,7 @@ void intel_psr2_program_plane_sel_fetch(struct
> > > intel_plane *plane,
> > > const struct intel_crtc_state
> > > *crtc_state,
> > > const struct
> > > intel_plane_state *plane_state,
> > > int color_plane);
> > > +void intel_psr_pause(struct intel_dp *intel_dp);
> > > +void intel_psr_resume(struct intel_dp *intel_dp);
> > >
> > > #endif /* __INTEL_PSR_H__ */
> >
>
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
