| Message ID | 20210515031035.2561658-19-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Alder Lake-P Support | expand |
On Fri, 2021-05-14 at 20:10 -0700, Matt Roper wrote: > From: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> > > 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@intel.com> > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > --- > .../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__ */
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()? 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? 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@intel.com> > > > > 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@intel.com> > > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > > --- > > .../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__ */ >
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@intel.com> > > > > > > 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@intel.com> > > > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> > > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > > > --- > > > .../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__ */ > > >
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@intel.com> > > > > > > > > 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@intel.com> > > > > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> > > > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > > > > --- > > > > .../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__ */ > > > > > >
On Fri, 2021-05-21 at 11:58 +0100, Mun, Gwan-gyeong wrote: > 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. > ... I don't think we need to disable, psr_exit() + wait until PSR is idle is enough, all other stuff can be left as is. > > 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@intel.com> > > > > > > > > > > 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@intel.com> > > > > > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> > > > > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > > > > > --- > > > > > .../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__ */ > > > > > > > > > >
The v2 patch which addressed Jose's comments was floated to https://patchwork.freedesktop.org/series/90819/ On Fri, 2021-05-21 at 14:52 -0700, Souza, Jose wrote: > On Fri, 2021-05-21 at 11:58 +0100, Mun, Gwan-gyeong wrote: > > 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. > > ... > > I don't think we need to disable, psr_exit() + wait until PSR is idle > is enough, all other stuff can be left as is. > > > > > 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@intel.com> > > > > > > > > > > > > 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@intel.com> > > > > > > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> > > > > > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > > > > > > --- > > > > > > .../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__ */ > > > > > > > > > > > > > > >
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); + +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__ */