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

Jeykumar Sankaran
2018-12-03T19:52:42Z

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;
> }

--
Jeykumar S