On Fri, Oct 05, 2018 at 11:48:42AM -0400, Bruce Wang wrote:
> dpu_crtc_enable/disable/reset are only being called upon
> initialization/destruction of the device and as part of the PM
> suspend/resume handling.
*and dpms/enable on/off
> Since core suspend and resume calls
> are guarenteed to not overlap with one another we don't need to
*guaranteed
It also might be more clear to just state "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.
> This patch removes the suspend logic from the crtc, and removes the relevant
> tracing from dpu_trace.
>
> Signed-off-by: Bruce Wang <bzwang@xxxxxxxxxxxx>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 61 ++---------------------
> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 2 -
> drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h | 15 +-----
> 3 files changed, 7 insertions(+), 71 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..31e372554e0c 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
> @@ -881,25 +852,14 @@ static struct drm_crtc_state *dpu_crtc_duplicate_state(struct drm_crtc *crtc)
> */
> static void dpu_crtc_reset(struct drm_crtc *crtc)
> {
Can you add a patch to the series to delete this function entirely and use
drm_atomic_helper_crtc_reset in dpu_crtc_funcs instead?
> - struct dpu_crtc *dpu_crtc;
> struct dpu_crtc_state *cstate;
>
> - if (!crtc) {
> - DPU_ERROR("invalid crtc\n");
> - return;
> - }
> -
Since we're going to delete this function entirely, please also rollback the
cleanups in it so the diff is a bit more easy to read.
> - /* revert suspend actions, if necessary */
> - if (dpu_kms_is_suspend_state(crtc->dev))
> - _dpu_crtc_set_suspend(crtc, false);
> -
> /* remove previous state, if present */
> if (crtc->state) {
> dpu_crtc_destroy_state(crtc, crtc->state);
> - crtc->state = 0;
> + crtc->state = NULL;
> }
>
> - dpu_crtc = to_dpu_crtc(crtc);
> cstate = kzalloc(sizeof(*cstate), GFP_KERNEL);
> if (!cstate) {
> DPU_ERROR("failed to allocate state\n");
> @@ -951,9 +911,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 +923,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 +989,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 +1244,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_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
