Re: [Intel-gfx] [PATCH v3 1/2] drm/i915/display: Introduce new intel_psr_pause/resume function
On Mon, 2021-06-07 at 17:19 +0300, Gwan-gyeong Mun wrote:
> This introduces the following function that can exit and activate a psr
> source when intel_psr is already enabled.
>
> - intel_psr_pause(): Pause current PSR. It deactivates current psr state.
> - intel_psr_resume(): Resume paused PSR. It activates paused psr state.
>
> v2: Address Jose's review comment.
> - Remove unneeded changes around the intel_psr_enable().
> - Add intel_psr_post_exit() which processes waiting until PSR is idle
> and WA for SelectiveFetch.
> v3: Address Jose's review comment.
> - Rename intel_psr_post_exit() to intel_psr_wait_exit_locked().
> - Move WA_1408330847 to intel_psr_disable_locked()
> - If the PSR is paused by an explicit intel_psr_paused() call, make the
> intel_psr_flush() not to activate PSR.
>
> Cc: José Roberto de Souza
> Cc: Stanislav Lisovskiy
> Cc: Ville Syrjälä
> Signed-off-by: Gwan-gyeong Mun
> Signed-off-by: Matt Roper
> ---
> .../drm/i915/display/intel_display_types.h| 1 +
> drivers/gpu/drm/i915/display/intel_psr.c | 94 ---
> drivers/gpu/drm/i915/display/intel_psr.h | 2 +
> 3 files changed, 86 insertions(+), 11 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 000e1ffe8c05..f547c80ed55c 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -1113,6 +1113,7 @@ static void intel_psr_enable_locked(struct intel_dp
> *intel_dp,
> intel_psr_enable_sink(intel_dp);
> intel_psr_enable_source(intel_dp);
> intel_dp->psr.enabled = true;
> + intel_dp->psr.paused = false;
>
> intel_psr_activate(intel_dp);
> }
> @@ -1182,22 +1183,12 @@ static void intel_psr_exit(struct intel_dp *intel_dp)
> intel_dp->psr.active = false;
> }
>
> -static void intel_psr_disable_locked(struct intel_dp *intel_dp)
> +static void intel_psr_wait_exit_locked(struct intel_dp *intel_dp)
> {
> struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> i915_reg_t psr_status;
> u32 psr_status_mask;
>
> - lockdep_assert_held(_dp->psr.lock);
> -
> - if (!intel_dp->psr.enabled)
> - return;
> -
> - drm_dbg_kms(_priv->drm, "Disabling PSR%s\n",
> - intel_dp->psr.psr2_enabled ? "2" : "1");
> -
> - intel_psr_exit(intel_dp);
> -
> if (intel_dp->psr.psr2_enabled) {
> psr_status = EDP_PSR2_STATUS(intel_dp->psr.transcoder);
> psr_status_mask = EDP_PSR2_STATUS_STATE_MASK;
> @@ -1210,6 +1201,22 @@ static void intel_psr_disable_locked(struct intel_dp
> *intel_dp)
> if (intel_de_wait_for_clear(dev_priv, psr_status,
> psr_status_mask, 2000))
> drm_err(_priv->drm, "Timed out waiting PSR idle state\n");
> +}
> +
> +static void intel_psr_disable_locked(struct intel_dp *intel_dp)
> +{
> + struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> +
> + lockdep_assert_held(_dp->psr.lock);
> +
> + if (!intel_dp->psr.enabled)
> + return;
> +
> + drm_dbg_kms(_priv->drm, "Disabling PSR%s\n",
> + intel_dp->psr.psr2_enabled ? "2" : "1");
> +
> + intel_psr_exit(intel_dp);
> + intel_psr_wait_exit_locked(intel_dp);
>
> /* WA 1408330847 */
> if (intel_dp->psr.psr2_sel_fetch_enabled &&
> @@ -1254,6 +1261,61 @@ void intel_psr_disable(struct intel_dp *intel_dp,
> cancel_delayed_work_sync(_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 = _dp->psr;
> +
> + if (!CAN_PSR(intel_dp))
> + return;
> +
> + mutex_lock(>lock);
> +
> + if (!psr->active) {
Hum just one more case came to mind.
PSR is not active but there is a scheduled psr->work that will execute after
this call.
psr->active will be false, returning then a few msec later PSR will be
activated.
So can you change this to psr->enabled?
intel_psr_exit() and intel_psr_wait_exit_locked() will handle the psr->active
== false.
With that:
Reviewed-by: José Roberto de Souza
> + mutex_unlock(>lock);
> + return;
> + }
> +
> + intel_psr_exit(intel_dp);
> + intel_psr_wait_exit_locked(intel_dp);
> + psr->paused = true;
>
[Intel-gfx] [PATCH v3 1/2] drm/i915/display: Introduce new intel_psr_pause/resume function
This introduces the following function that can exit and activate a psr
source when intel_psr is already enabled.
- intel_psr_pause(): Pause current PSR. It deactivates current psr state.
- intel_psr_resume(): Resume paused PSR. It activates paused psr state.
v2: Address Jose's review comment.
- Remove unneeded changes around the intel_psr_enable().
- Add intel_psr_post_exit() which processes waiting until PSR is idle
and WA for SelectiveFetch.
v3: Address Jose's review comment.
- Rename intel_psr_post_exit() to intel_psr_wait_exit_locked().
- Move WA_1408330847 to intel_psr_disable_locked()
- If the PSR is paused by an explicit intel_psr_paused() call, make the
intel_psr_flush() not to activate PSR.
Cc: José Roberto de Souza
Cc: Stanislav Lisovskiy
Cc: Ville Syrjälä
Signed-off-by: Gwan-gyeong Mun
Signed-off-by: Matt Roper
---
.../drm/i915/display/intel_display_types.h| 1 +
drivers/gpu/drm/i915/display/intel_psr.c | 94 ---
drivers/gpu/drm/i915/display/intel_psr.h | 2 +
3 files changed, 86 insertions(+), 11 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 000e1ffe8c05..f547c80ed55c 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.c
+++ b/drivers/gpu/drm/i915/display/intel_psr.c
@@ -1113,6 +1113,7 @@ static void intel_psr_enable_locked(struct intel_dp
*intel_dp,
intel_psr_enable_sink(intel_dp);
intel_psr_enable_source(intel_dp);
intel_dp->psr.enabled = true;
+ intel_dp->psr.paused = false;
intel_psr_activate(intel_dp);
}
@@ -1182,22 +1183,12 @@ static void intel_psr_exit(struct intel_dp *intel_dp)
intel_dp->psr.active = false;
}
-static void intel_psr_disable_locked(struct intel_dp *intel_dp)
+static void intel_psr_wait_exit_locked(struct intel_dp *intel_dp)
{
struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
i915_reg_t psr_status;
u32 psr_status_mask;
- lockdep_assert_held(_dp->psr.lock);
-
- if (!intel_dp->psr.enabled)
- return;
-
- drm_dbg_kms(_priv->drm, "Disabling PSR%s\n",
- intel_dp->psr.psr2_enabled ? "2" : "1");
-
- intel_psr_exit(intel_dp);
-
if (intel_dp->psr.psr2_enabled) {
psr_status = EDP_PSR2_STATUS(intel_dp->psr.transcoder);
psr_status_mask = EDP_PSR2_STATUS_STATE_MASK;
@@ -1210,6 +1201,22 @@ static void intel_psr_disable_locked(struct intel_dp
*intel_dp)
if (intel_de_wait_for_clear(dev_priv, psr_status,
psr_status_mask, 2000))
drm_err(_priv->drm, "Timed out waiting PSR idle state\n");
+}
+
+static void intel_psr_disable_locked(struct intel_dp *intel_dp)
+{
+ struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
+
+ lockdep_assert_held(_dp->psr.lock);
+
+ if (!intel_dp->psr.enabled)
+ return;
+
+ drm_dbg_kms(_priv->drm, "Disabling PSR%s\n",
+ intel_dp->psr.psr2_enabled ? "2" : "1");
+
+ intel_psr_exit(intel_dp);
+ intel_psr_wait_exit_locked(intel_dp);
/* WA 1408330847 */
if (intel_dp->psr.psr2_sel_fetch_enabled &&
@@ -1254,6 +1261,61 @@ void intel_psr_disable(struct intel_dp *intel_dp,
cancel_delayed_work_sync(_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 = _dp->psr;
+
+ if (!CAN_PSR(intel_dp))
+ return;
+
+ mutex_lock(>lock);
+
+ if (!psr->active) {
+ mutex_unlock(>lock);
+ return;
+ }
+
+ intel_psr_exit(intel_dp);
+ intel_psr_wait_exit_locked(intel_dp);
+ psr->paused = true;
+
+ mutex_unlock(>lock);
+
+ cancel_work_sync(>work);
+ cancel_delayed_work_sync(>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 = _dp->psr;
+
+ if (!CAN_PSR(intel_dp))
+ return;
+
+ mutex_lock(>lock);
+
+ if (!psr->paused)
+ goto unlock;
+
+ psr->paused = false;
+ intel_psr_activate(intel_dp);
+
+unlock:
+ mutex_unlock(>lock);
+}
+
static
