diff mbox series

[v2,12/24] drm/msm: dpu: Move crtc runtime resume to encoder

Message ID 20181116184238.170034-13-sean@poorly.run (mailing list archive)
State New, archived
Headers show
Series drm/msm: Various dpu locking and legacy cleanups | expand

Commit Message

Sean Paul Nov. 16, 2018, 6:42 p.m. UTC 
From: Sean Paul <seanpaul@chromium.org>

The crtc runtime resume doesn't actually operate on the crtc, but rather
its encoders. The problem with this is that we need to inspect the crtc
state to get the currently connected encoders. Since runtime resume
isn't guaranteed to be called while holding the modeset locks (although
it sometimes is), this presents a race condition.

Now that we have ->enabled on the virtual encoders, and a lock to
protect it, just call resume on each encoder and only restore the ones
that are enabled.

Changes in v2:
- None

Cc: Jeykumar Sankaran <jsanka@codeaurora.org>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 24 ---------------------
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  6 ------
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 24 +++++++++------------
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  4 ++--
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     |  6 +++---
 5 files changed, 15 insertions(+), 49 deletions(-)

Comments

Jeykumar Sankaran Dec. 3, 2018, 7:52 p.m. UTC | #1 
On 2018-11-16 10:42, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
> 
> The crtc runtime resume doesn't actually operate on the crtc, but 
> rather
> its encoders. The problem with this is that we need to inspect the crtc
> state to get the currently connected encoders. Since runtime resume
> isn't guaranteed to be called while holding the modeset locks (although
> it sometimes is), this presents a race condition.
> 
> Now that we have ->enabled on the virtual encoders, and a lock to
> protect it, just call resume on each encoder and only restore the ones
> that are enabled.
> 
> Changes in v2:
> - None
> 
> Cc: Jeykumar Sankaran <jsanka@codeaurora.org>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---

Reviewed-by: Jeykumar Sankaran <jsanka@codeaurora.org>

>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 24 ---------------------
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  6 ------
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 24 +++++++++------------
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  4 ++--
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     |  6 +++---
>  5 files changed, 15 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index d8f58caf2772..9be24907f8c1 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -841,30 +841,6 @@ static struct drm_crtc_state
> *dpu_crtc_duplicate_state(struct drm_crtc *crtc)
>  	return &cstate->base;
>  }
> 
> -void dpu_crtc_runtime_resume(struct drm_crtc *crtc)
> -{
> -	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
> -	struct drm_encoder *encoder;
> -
> -	mutex_lock(&dpu_crtc->crtc_lock);
> -
> -	if (!dpu_crtc->enabled)
> -		goto end;
> -
> -	trace_dpu_crtc_runtime_resume(DRMID(crtc));
> -
> -	/* restore encoder; crtc will be programmed during commit */
> -	drm_for_each_encoder(encoder, crtc->dev) {
> -		if (encoder->crtc != crtc)
> -			continue;
> -
> -		dpu_encoder_virt_restore(encoder);
> -	}
> -
> -end:
> -	mutex_unlock(&dpu_crtc->crtc_lock);
> -}
> -
>  static void dpu_crtc_disable(struct drm_crtc *crtc)
>  {
>  	struct dpu_crtc *dpu_crtc;
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> index 1dca91d1210f..93d21a61a040 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> @@ -329,10 +329,4 @@ static inline bool dpu_crtc_is_enabled(struct
> drm_crtc *crtc)
>  	return crtc ? crtc->enabled : false;
>  }
> 
> -/**
> - * dpu_crtc_runtime_resume - called by the top-level on 
> pm_runtime_resume
> - * @crtc: CRTC to resume
> - */
> -void dpu_crtc_runtime_resume(struct drm_crtc *crtc);
> -
>  #endif /* _DPU_CRTC_H_ */
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 3daa86220d47..d89ac520f7e6 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -1089,28 +1089,24 @@ static void 
> _dpu_encoder_virt_enable_helper(struct
> drm_encoder *drm_enc)
>  	_dpu_encoder_update_vsync_source(dpu_enc, &dpu_enc->disp_info);
>  }
> 
> -void dpu_encoder_virt_restore(struct drm_encoder *drm_enc)
> +void dpu_encoder_virt_runtime_resume(struct drm_encoder *drm_enc)
>  {
> -	struct dpu_encoder_virt *dpu_enc = NULL;
> -	int i;
> -
> -	if (!drm_enc) {
> -		DPU_ERROR("invalid encoder\n");
> -		return;
> -	}
> -	dpu_enc = to_dpu_encoder_virt(drm_enc);
> +	struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
> 
> -	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> -		struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
> +	mutex_lock(&dpu_enc->enc_lock);
> 
> -		if (phys && (phys != dpu_enc->cur_master) &&
> phys->ops.restore)
> -			phys->ops.restore(phys);
> -	}
> +	if (!dpu_enc->enabled)
> +		goto out;
> 
> +	if (dpu_enc->cur_slave && dpu_enc->cur_slave->ops.restore)
> +		dpu_enc->cur_slave->ops.restore(dpu_enc->cur_slave);
>  	if (dpu_enc->cur_master && dpu_enc->cur_master->ops.restore)
>  		dpu_enc->cur_master->ops.restore(dpu_enc->cur_master);
> 
>  	_dpu_encoder_virt_enable_helper(drm_enc);
> +
> +out:
> +	mutex_unlock(&dpu_enc->enc_lock);
>  }
> 
>  static void dpu_encoder_virt_enable(struct drm_encoder *drm_enc)
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> index 9dbf38f446d9..aa4f135218fa 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> @@ -126,10 +126,10 @@ int dpu_encoder_wait_for_event(struct drm_encoder
> *drm_encoder,
>  enum dpu_intf_mode dpu_encoder_get_intf_mode(struct drm_encoder
> *encoder);
> 
>  /**
> - * dpu_encoder_virt_restore - restore the encoder configs
> + * dpu_encoder_virt_runtime_resume - pm runtime resume the encoder
> configs
>   * @encoder:	encoder pointer
>   */
> -void dpu_encoder_virt_restore(struct drm_encoder *encoder);
> +void dpu_encoder_virt_runtime_resume(struct drm_encoder *encoder);
> 
>  /**
>   * dpu_encoder_init - initialize virtual encoder object
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 9f7da56bb453..1bec4540f3e1 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -1129,7 +1129,7 @@ static int __maybe_unused 
> dpu_runtime_resume(struct
> device *dev)
>  	int rc = -1;
>  	struct platform_device *pdev = to_platform_device(dev);
>  	struct dpu_kms *dpu_kms = platform_get_drvdata(pdev);
> -	struct drm_crtc *crtc;
> +	struct drm_encoder *encoder;
>  	struct drm_device *ddev;
>  	struct dss_module_power *mp = &dpu_kms->mp;
> 
> @@ -1147,8 +1147,8 @@ static int __maybe_unused 
> dpu_runtime_resume(struct
> device *dev)
> 
>  	dpu_vbif_init_memtypes(dpu_kms);
> 
> -	drm_for_each_crtc(crtc, ddev)
> -		dpu_crtc_runtime_resume(crtc);
> +	drm_for_each_encoder(encoder, ddev)
> +		dpu_encoder_virt_runtime_resume(encoder);
> 
>  	return rc;
>  }
diff mbox series

Patch

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index d8f58caf2772..9be24907f8c1 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -841,30 +841,6 @@  static struct drm_crtc_state *dpu_crtc_duplicate_state(struct drm_crtc *crtc)
 	return &cstate->base;
 }
 
-void dpu_crtc_runtime_resume(struct drm_crtc *crtc)
-{
-	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
-	struct drm_encoder *encoder;
-
-	mutex_lock(&dpu_crtc->crtc_lock);
-
-	if (!dpu_crtc->enabled)
-		goto end;
-
-	trace_dpu_crtc_runtime_resume(DRMID(crtc));
-
-	/* restore encoder; crtc will be programmed during commit */
-	drm_for_each_encoder(encoder, crtc->dev) {
-		if (encoder->crtc != crtc)
-			continue;
-
-		dpu_encoder_virt_restore(encoder);
-	}
-
-end:
-	mutex_unlock(&dpu_crtc->crtc_lock);
-}
-
 static void dpu_crtc_disable(struct drm_crtc *crtc)
 {
 	struct dpu_crtc *dpu_crtc;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
index 1dca91d1210f..93d21a61a040 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
@@ -329,10 +329,4 @@  static inline bool dpu_crtc_is_enabled(struct drm_crtc *crtc)
 	return crtc ? crtc->enabled : false;
 }
 
-/**
- * dpu_crtc_runtime_resume - called by the top-level on pm_runtime_resume
- * @crtc: CRTC to resume
- */
-void dpu_crtc_runtime_resume(struct drm_crtc *crtc);
-
 #endif /* _DPU_CRTC_H_ */
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 3daa86220d47..d89ac520f7e6 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1089,28 +1089,24 @@  static void _dpu_encoder_virt_enable_helper(struct drm_encoder *drm_enc)
 	_dpu_encoder_update_vsync_source(dpu_enc, &dpu_enc->disp_info);
 }
 
-void dpu_encoder_virt_restore(struct drm_encoder *drm_enc)
+void dpu_encoder_virt_runtime_resume(struct drm_encoder *drm_enc)
 {
-	struct dpu_encoder_virt *dpu_enc = NULL;
-	int i;
-
-	if (!drm_enc) {
-		DPU_ERROR("invalid encoder\n");
-		return;
-	}
-	dpu_enc = to_dpu_encoder_virt(drm_enc);
+	struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
 
-	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
-		struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
+	mutex_lock(&dpu_enc->enc_lock);
 
-		if (phys && (phys != dpu_enc->cur_master) && phys->ops.restore)
-			phys->ops.restore(phys);
-	}
+	if (!dpu_enc->enabled)
+		goto out;
 
+	if (dpu_enc->cur_slave && dpu_enc->cur_slave->ops.restore)
+		dpu_enc->cur_slave->ops.restore(dpu_enc->cur_slave);
 	if (dpu_enc->cur_master && dpu_enc->cur_master->ops.restore)
 		dpu_enc->cur_master->ops.restore(dpu_enc->cur_master);
 
 	_dpu_encoder_virt_enable_helper(drm_enc);
+
+out:
+	mutex_unlock(&dpu_enc->enc_lock);
 }
 
 static void dpu_encoder_virt_enable(struct drm_encoder *drm_enc)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
index 9dbf38f446d9..aa4f135218fa 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
@@ -126,10 +126,10 @@  int dpu_encoder_wait_for_event(struct drm_encoder *drm_encoder,
 enum dpu_intf_mode dpu_encoder_get_intf_mode(struct drm_encoder *encoder);
 
 /**
- * dpu_encoder_virt_restore - restore the encoder configs
+ * dpu_encoder_virt_runtime_resume - pm runtime resume the encoder configs
  * @encoder:	encoder pointer
  */
-void dpu_encoder_virt_restore(struct drm_encoder *encoder);
+void dpu_encoder_virt_runtime_resume(struct drm_encoder *encoder);
 
 /**
  * dpu_encoder_init - initialize virtual encoder object
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 9f7da56bb453..1bec4540f3e1 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -1129,7 +1129,7 @@  static int __maybe_unused dpu_runtime_resume(struct device *dev)
 	int rc = -1;
 	struct platform_device *pdev = to_platform_device(dev);
 	struct dpu_kms *dpu_kms = platform_get_drvdata(pdev);
-	struct drm_crtc *crtc;
+	struct drm_encoder *encoder;
 	struct drm_device *ddev;
 	struct dss_module_power *mp = &dpu_kms->mp;
 
@@ -1147,8 +1147,8 @@  static int __maybe_unused dpu_runtime_resume(struct device *dev)
 
 	dpu_vbif_init_memtypes(dpu_kms);
 
-	drm_for_each_crtc(crtc, ddev)
-		dpu_crtc_runtime_resume(crtc);
+	drm_for_each_encoder(encoder, ddev)
+		dpu_encoder_virt_runtime_resume(encoder);
 
 	return rc;
 }