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

[Jeykumar Sankaran <jsanka-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>]

On 2018-11-12 11: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.
> 
> 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(-)
> 
> 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);
> -	}
I agree the patch provides a cleaner solution. Just for the 
understanding,
won't the race condition be addressed if we acquire the
modeset lock here and iterate through crtc->state->encoder_mask?

Thanks and Regards,
Jeykumar S.
> -
> -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 e42685a1d928..c385eab7e441 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -1132,7 +1132,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;
> 
> @@ -1150,8 +1150,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
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno