Re: [PATCH v3 3/4] drm/msm/dpu: Remove suspend state tracking from crtc

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

On Fri, Oct 05, 2018 at 05:04:02PM -0400, Bruce Wang wrote:
> Since drm core's modeset locks serialize atomic commits, we don't need to
> track whether or not we're in a suspended state from inside the crtc for
> dpu_crtc_enable/disable. This patch removes the suspend logic from the crtc and
> removes the relevant tracing from dpu_trace. Since we removed all calls
> to dpu_kms_is_suspend_state, we can remove that function and the
> suspend_state field of dpu_kms as well.
> 
> v2: added patch to patchset
> v3: reworded commit body and moved deletion of dpu_kms_is_suspend_state and
> suspend_state to this patch
> 
> Signed-off-by: Bruce Wang <bzwang@chromium.org>

Reviewed-by: Sean Paul <seanpaul@chromium.org>

> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  | 48 ++---------------------
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h  |  2 -
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h   | 16 --------
>  drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h | 15 +------
>  4 files changed, 6 insertions(+), 75 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index d4530d60767b..ac604055a824 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -815,35 +815,6 @@ static void _dpu_crtc_vblank_enable_no_lock(
>  	}
>  }
>  
> -/**
> - * _dpu_crtc_set_suspend - notify crtc of suspend enable/disable
> - * @crtc: Pointer to drm crtc object
> - * @enable: true to enable suspend, false to indicate resume
> - */
> -static void _dpu_crtc_set_suspend(struct drm_crtc *crtc, bool enable)
> -{
> -	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
> -
> -	DRM_DEBUG_KMS("crtc%d suspend = %d\n", crtc->base.id, enable);
> -
> -	mutex_lock(&dpu_crtc->crtc_lock);
> -
> -	/*
> -	 * If the vblank is enabled, release a power reference on suspend
> -	 * and take it back during resume (if it is still enabled).
> -	 */
> -	trace_dpu_crtc_set_suspend(DRMID(&dpu_crtc->base), enable, dpu_crtc);
> -	if (dpu_crtc->suspend == enable)
> -		DPU_DEBUG("crtc%d suspend already set to %d, ignoring update\n",
> -				crtc->base.id, enable);
> -	else if (dpu_crtc->enabled && dpu_crtc->vblank_requested) {
> -		_dpu_crtc_vblank_enable_no_lock(dpu_crtc, !enable);
> -	}
> -
> -	dpu_crtc->suspend = enable;
> -	mutex_unlock(&dpu_crtc->crtc_lock);
> -}
> -
>  /**
>   * dpu_crtc_duplicate_state - state duplicate hook
>   * @crtc: Pointer to drm crtc structure
> @@ -951,9 +922,6 @@ static void dpu_crtc_disable(struct drm_crtc *crtc)
>  
>  	DRM_DEBUG_KMS("crtc%d\n", crtc->base.id);
>  
> -	if (dpu_kms_is_suspend_state(crtc->dev))
> -		_dpu_crtc_set_suspend(crtc, true);
> -
>  	/* Disable/save vblank irq handling */
>  	drm_crtc_vblank_off(crtc);
>  
> @@ -966,8 +934,7 @@ static void dpu_crtc_disable(struct drm_crtc *crtc)
>  				atomic_read(&dpu_crtc->frame_pending));
>  
>  	trace_dpu_crtc_disable(DRMID(crtc), false, dpu_crtc);
> -	if (dpu_crtc->enabled && !dpu_crtc->suspend &&
> -			dpu_crtc->vblank_requested) {
> +	if (dpu_crtc->enabled && dpu_crtc->vblank_requested) {
>  		_dpu_crtc_vblank_enable_no_lock(dpu_crtc, false);
>  	}
>  	dpu_crtc->enabled = false;
> @@ -1033,8 +1000,7 @@ static void dpu_crtc_enable(struct drm_crtc *crtc,
>  
>  	mutex_lock(&dpu_crtc->crtc_lock);
>  	trace_dpu_crtc_enable(DRMID(crtc), true, dpu_crtc);
> -	if (!dpu_crtc->enabled && !dpu_crtc->suspend &&
> -			dpu_crtc->vblank_requested) {
> +	if (!dpu_crtc->enabled && dpu_crtc->vblank_requested) {
>  		_dpu_crtc_vblank_enable_no_lock(dpu_crtc, true);
>  	}
>  	dpu_crtc->enabled = true;
> @@ -1289,17 +1255,11 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
>  
>  int dpu_crtc_vblank(struct drm_crtc *crtc, bool en)
>  {
> -	struct dpu_crtc *dpu_crtc;
> -
> -	if (!crtc) {
> -		DPU_ERROR("invalid crtc\n");
> -		return -EINVAL;
> -	}
> -	dpu_crtc = to_dpu_crtc(crtc);
> +	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
>  
>  	mutex_lock(&dpu_crtc->crtc_lock);
>  	trace_dpu_crtc_vblank(DRMID(&dpu_crtc->base), en, dpu_crtc);
> -	if (dpu_crtc->enabled && !dpu_crtc->suspend) {
> +	if (dpu_crtc->enabled) {
>  		_dpu_crtc_vblank_enable_no_lock(dpu_crtc, en);
>  	}
>  	dpu_crtc->vblank_requested = en;
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> index 3723b4830335..7dc181aabb4d 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> @@ -133,7 +133,6 @@ struct dpu_crtc_frame_event {
>   * @play_count    : frame count between crtc enable and disable
>   * @vblank_cb_time  : ktime at vblank count reset
>   * @vblank_requested : whether the user has requested vblank events
> - * @suspend         : whether or not a suspend operation is in progress
>   * @enabled       : whether the DPU CRTC is currently enabled. updated in the
>   *                  commit-thread, not state-swap time which is earlier, so
>   *                  safe to make decisions on during VBLANK on/off work
> @@ -169,7 +168,6 @@ struct dpu_crtc {
>  	u64 play_count;
>  	ktime_t vblank_cb_time;
>  	bool vblank_requested;
> -	bool suspend;
>  	bool enabled;
>  
>  	struct list_head feature_list;
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> index 1991cbe424a3..2a3625eef6d3 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> @@ -135,9 +135,6 @@ struct dpu_kms {
>  
>  	struct dpu_core_perf perf;
>  
> -	/* saved atomic state during system suspend */
> -	struct drm_atomic_state *suspend_state;
> -
>  	struct dpu_rm rm;
>  	bool rm_init;
>  
> @@ -162,19 +159,6 @@ struct vsync_info {
>  #define ddev_to_msm_kms(D) ((D) && (D)->dev_private ? \
>  		((struct msm_drm_private *)((D)->dev_private))->kms : NULL)
>  
> -/**
> - * dpu_kms_is_suspend_state - whether or not the system is pm suspended
> - * @dev: Pointer to drm device
> - * Return: Suspend status
> - */
> -static inline bool dpu_kms_is_suspend_state(struct drm_device *dev)
> -{
> -	if (!ddev_to_msm_kms(dev))
> -		return false;
> -
> -	return to_dpu_kms(ddev_to_msm_kms(dev))->suspend_state != NULL;
> -}
> -
>  /**
>   * Debugfs functions - extra helper functions for debugfs support
>   *
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
> index 636b31b0d311..0c122e173892 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
> @@ -749,7 +749,6 @@ TRACE_EVENT(dpu_crtc_vblank_enable,
>  		__field(	uint32_t,		enc_id	)
>  		__field(	bool,			enable	)
>  		__field(	bool,			enabled )
> -		__field(	bool,			suspend )
>  		__field(	bool,			vblank_requested )
>  	),
>  	TP_fast_assign(
> @@ -757,15 +756,12 @@ TRACE_EVENT(dpu_crtc_vblank_enable,
>  		__entry->enc_id = enc_id;
>  		__entry->enable = enable;
>  		__entry->enabled = crtc->enabled;
> -		__entry->suspend = crtc->suspend;
>  		__entry->vblank_requested = crtc->vblank_requested;
>  	),
> -	TP_printk("id:%u encoder:%u enable:%s state{enabled:%s suspend:%s "
> -		  "vblank_req:%s}",
> +	TP_printk("id:%u encoder:%u enable:%s state{enabled:%s vblank_req:%s}",
>  		  __entry->drm_id, __entry->enc_id,
>  		  __entry->enable ? "true" : "false",
>  		  __entry->enabled ? "true" : "false",
> -		  __entry->suspend ? "true" : "false",
>  		  __entry->vblank_requested ? "true" : "false")
>  );
>  
> @@ -776,26 +772,19 @@ DECLARE_EVENT_CLASS(dpu_crtc_enable_template,
>  		__field(	uint32_t,		drm_id	)
>  		__field(	bool,			enable	)
>  		__field(	bool,			enabled )
> -		__field(	bool,			suspend )
>  		__field(	bool,			vblank_requested )
>  	),
>  	TP_fast_assign(
>  		__entry->drm_id = drm_id;
>  		__entry->enable = enable;
>  		__entry->enabled = crtc->enabled;
> -		__entry->suspend = crtc->suspend;
>  		__entry->vblank_requested = crtc->vblank_requested;
>  	),
> -	TP_printk("id:%u enable:%s state{enabled:%s suspend:%s vblank_req:%s}",
> +	TP_printk("id:%u enable:%s state{enabled:%s vblank_req:%s}",
>  		  __entry->drm_id, __entry->enable ? "true" : "false",
>  		  __entry->enabled ? "true" : "false",
> -		  __entry->suspend ? "true" : "false",
>  		  __entry->vblank_requested ? "true" : "false")
>  );
> -DEFINE_EVENT(dpu_crtc_enable_template, dpu_crtc_set_suspend,
> -	TP_PROTO(uint32_t drm_id, bool enable, struct dpu_crtc *crtc),
> -	TP_ARGS(drm_id, enable, crtc)
> -);
>  DEFINE_EVENT(dpu_crtc_enable_template, dpu_crtc_enable,
>  	TP_PROTO(uint32_t drm_id, bool enable, struct dpu_crtc *crtc),
>  	TP_ARGS(drm_id, enable, crtc)
> -- 
> 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