On Tue, 2021-05-18 at 14:06 +0300, Ville Syrjälä wrote:
> 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.
>
Hi Ville and Stan,
Thanks, Ville, for explaining.
intel_psr_pause() and intel_psr_resume() are an api added to use when
reactivating (disable and enable) the psr functionality without
intel_crtc_state and drm_connector_state, as described in the commit
log.
And in order to deactivate and activate psr normally, we must
deactivate the psr functionality of the sink as well, and at this time,
sink psr deactivate using dpcd.
And in the part explaining disabling psr in cdclk setting in bspec, the
following procedure is explained for disabling psr.
1. Temporarily disable PSR1, PSR2, and GTC.
2. Wait for disabling status from those functions.
3. Wait for any pending Aux transactions to complete, and do not start
any new Aux transaction.
...
So, in my opinion, when the cdclk setting is called from
intel_atomic_commit_tail() with functions such as
intel_set_cdclk_pre_plane_update() /
intel_set_cdclk_post_plane_update(),
if psr deactivation/activation is necessary, it seems that
intel_set_cdclk_pre_plane_update() /
intel_set_cdclk_post_plane_update() should be called with
intel_psr_enable() / intel_psr_disable() functions together. What do
you think?
Br,
G.G.
> >
> > 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__ */
> > >
> >
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
