Re: [PATCH 2/2] drm/msm/dpu: Remove all dpu pm_suspend/resume code

[Sean Paul <sean-p7yTbzM4H96eqtR555YLDQ@public.gmane.org>]

On Tue, Oct 02, 2018 at 11:05:30AM -0400, Bruce Wang wrote:
> Removes the now uncalled pm_suspend/resume code. Also removes
> dpu_kms_is_suspend_blocked which is never called, and changes
> dpu_kms_is_suspend_state to work with the new msm_pm_suspend/resume
> implementations.
> 
> Signed-off-by: Bruce Wang <bzwang@chromium.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 121 ------------------------
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h |  23 +----
>  2 files changed, 2 insertions(+), 142 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index e0142912d676..ff06b50dfc87 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -709,127 +709,6 @@ static void dpu_kms_destroy(struct msm_kms *kms)
>  	_dpu_kms_hw_destroy(dpu_kms);
>  }
>  
> -static int dpu_kms_pm_suspend(struct device *dev)
> -{
> -	struct drm_device *ddev;
> -	struct drm_modeset_acquire_ctx ctx;
> -	struct drm_atomic_state *state;
> -	struct dpu_kms *dpu_kms;
> -	int ret = 0, num_crtcs = 0;
> -
> -	if (!dev)
> -		return -EINVAL;
> -
> -	ddev = dev_get_drvdata(dev);
> -	if (!ddev || !ddev_to_msm_kms(ddev))
> -		return -EINVAL;
> -
> -	dpu_kms = to_dpu_kms(ddev_to_msm_kms(ddev));
> -
> -	/* disable hot-plug polling */
> -	drm_kms_helper_poll_disable(ddev);
> -
> -	/* acquire modeset lock(s) */
> -	drm_modeset_acquire_init(&ctx, 0);
> -
> -retry:
> -	DPU_ATRACE_BEGIN("kms_pm_suspend");
> -
> -	ret = drm_modeset_lock_all_ctx(ddev, &ctx);
> -	if (ret)
> -		goto unlock;
> -
> -	/* save current state for resume */
> -	if (dpu_kms->suspend_state)
> -		drm_atomic_state_put(dpu_kms->suspend_state);
> -	dpu_kms->suspend_state = drm_atomic_helper_duplicate_state(ddev, &ctx);
> -	if (IS_ERR_OR_NULL(dpu_kms->suspend_state)) {
> -		DRM_ERROR("failed to back up suspend state\n");
> -		dpu_kms->suspend_state = NULL;
> -		goto unlock;
> -	}
> -
> -	/* create atomic state to disable all CRTCs */
> -	state = drm_atomic_state_alloc(ddev);
> -	if (IS_ERR_OR_NULL(state)) {
> -		DRM_ERROR("failed to allocate crtc disable state\n");
> -		goto unlock;
> -	}
> -
> -	state->acquire_ctx = &ctx;
> -
> -	/* check for nothing to do */
> -	if (num_crtcs == 0) {
> -		DRM_DEBUG("all crtcs are already in the off state\n");
> -		drm_atomic_state_put(state);
> -		goto suspended;
> -	}
> -
> -	/* commit the "disable all" state */
> -	ret = drm_atomic_commit(state);
> -	if (ret < 0) {
> -		DRM_ERROR("failed to disable crtcs, %d\n", ret);
> -		drm_atomic_state_put(state);
> -		goto unlock;
> -	}
> -
> -suspended:
> -	dpu_kms->suspend_block = true;
> -
> -unlock:
> -	if (ret == -EDEADLK) {
> -		drm_modeset_backoff(&ctx);
> -		goto retry;
> -	}
> -	drm_modeset_drop_locks(&ctx);
> -	drm_modeset_acquire_fini(&ctx);
> -
> -	DPU_ATRACE_END("kms_pm_suspend");
> -	return 0;
> -}
> -
> -static int dpu_kms_pm_resume(struct device *dev)
> -{
> -	struct drm_device *ddev;
> -	struct dpu_kms *dpu_kms;
> -	int ret;
> -
> -	if (!dev)
> -		return -EINVAL;
> -
> -	ddev = dev_get_drvdata(dev);
> -	if (!ddev || !ddev_to_msm_kms(ddev))
> -		return -EINVAL;
> -
> -	dpu_kms = to_dpu_kms(ddev_to_msm_kms(ddev));
> -
> -	DPU_ATRACE_BEGIN("kms_pm_resume");
> -
> -	drm_mode_config_reset(ddev);
> -
> -	drm_modeset_lock_all(ddev);
> -
> -	dpu_kms->suspend_block = false;
> -
> -	if (dpu_kms->suspend_state) {
> -		dpu_kms->suspend_state->acquire_ctx =
> -			ddev->mode_config.acquire_ctx;
> -		ret = drm_atomic_commit(dpu_kms->suspend_state);
> -		if (ret < 0) {
> -			DRM_ERROR("failed to restore state, %d\n", ret);
> -			drm_atomic_state_put(dpu_kms->suspend_state);
> -		}
> -		dpu_kms->suspend_state = NULL;
> -	}
> -	drm_modeset_unlock_all(ddev);
> -
> -	/* enable hot-plug polling */
> -	drm_kms_helper_poll_enable(ddev);
> -
> -	DPU_ATRACE_END("kms_pm_resume");
> -	return 0;
> -}
> -
>  static void _dpu_kms_set_encoder_mode(struct msm_kms *kms,
>  				 struct drm_encoder *encoder,
>  				 bool cmd_mode)
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> index 66d466628e2b..b04aa222e150 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> @@ -135,10 +135,6 @@ struct dpu_kms {
>  
>  	struct dpu_core_perf perf;
>  
> -	/* saved atomic state during system suspend */
> -	struct drm_atomic_state *suspend_state;
> -	bool suspend_block;
> -
>  	struct dpu_rm rm;
>  	bool rm_init;
>  
> @@ -170,24 +166,9 @@ struct vsync_info {
>   */
>  static inline bool dpu_kms_is_suspend_state(struct drm_device *dev)

I have a feeling we should be getting rid of this. If not, it probably means
some thread is not being terminated on suspend and we're trying to change hw
after the core has suspended. Can you please audit the callsites and either
remove this, add appropriate thread joins/flushs/locking, or add a comment here
about why this is safe?

Thanks!


>  {
> -	if (!ddev_to_msm_kms(dev))
> -		return false;
> -
> -	return to_dpu_kms(ddev_to_msm_kms(dev))->suspend_state != NULL;
> -}
> -
> -/**
> - * dpu_kms_is_suspend_blocked - whether or not commits are blocked due to pm
> - *				suspend status
> - * @dev: Pointer to drm device
> - * Return: True if commits should be rejected due to pm suspend
> - */
> -static inline bool dpu_kms_is_suspend_blocked(struct drm_device *dev)
> -{
> -	if (!dpu_kms_is_suspend_state(dev))
> -		return false;
> +	struct msm_drm_private *priv = dev->dev_private;
>  
> -	return to_dpu_kms(ddev_to_msm_kms(dev))->suspend_block;
> +	return !IS_ERR_OR_NULL(priv->pm_state);
>  }
>  
>  /**
> -- 
> 2.19.0.605.g01d371f741-goog
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno