[PATCH 2/2] drm/msm/dpu: remove CRTC frame event callback registration
Abhinav Kumar
quic_abhinavk at quicinc.com
Sat Jan 14 00:49:37 UTC 2023
On 1/2/2023 7:47 AM, Dmitry Baryshkov wrote:
> The frame event callback is always set to dpu_crtc_frame_event_cb() (or
> to NULL) and the data is always either the CRTC itself or NULL
> (correpondingly). Thus drop the event callback registration, call the
> dpu_crtc_frame_event_cb() directly and gate on the dpu_enc->crtc
> assigned using dpu_encoder_assign_crtc().
I suggest you wait till we sort out the PSR series for this, especially
this patch https://patchwork.freedesktop.org/patch/515787/
There is going to be some change in this code when PSR is pushed again
sometime early next week because PSR will touch the crtc assignment code
(dpu_enc->crtc),
Based on how we all like that patch, we can get back to this one as this
one is a minor cleanup.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 17 +--------
> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 14 +++++++
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 41 +++------------------
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 10 -----
> drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h | 4 --
> 5 files changed, 21 insertions(+), 65 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index 13ce321283ff..64cd2ec8a0c4 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -640,18 +640,8 @@ static void dpu_crtc_frame_event_work(struct kthread_work *work)
> DPU_ATRACE_END("crtc_frame_event");
> }
>
> -/*
> - * dpu_crtc_frame_event_cb - crtc frame event callback API. CRTC module
> - * registers this API to encoder for all frame event callbacks like
> - * frame_error, frame_done, idle_timeout, etc. Encoder may call different events
> - * from different context - IRQ, user thread, commit_thread, etc. Each event
> - * should be carefully reviewed and should be processed in proper task context
> - * to avoid schedulin delay or properly manage the irq context's bottom half
> - * processing.
> - */
> -static void dpu_crtc_frame_event_cb(void *data, u32 event)
> +void dpu_crtc_frame_event_cb(struct drm_crtc *crtc, u32 event)
> {
> - struct drm_crtc *crtc = (struct drm_crtc *)data;
> struct dpu_crtc *dpu_crtc;
> struct msm_drm_private *priv;
> struct dpu_crtc_frame_event *fevent;
> @@ -1051,9 +1041,6 @@ static void dpu_crtc_disable(struct drm_crtc *crtc,
>
> dpu_core_perf_crtc_update(crtc, 0, true);
>
> - drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask)
> - dpu_encoder_register_frame_event_callback(encoder, NULL, NULL);
> -
> memset(cstate->mixers, 0, sizeof(cstate->mixers));
> cstate->num_mixers = 0;
>
> @@ -1089,8 +1076,6 @@ static void dpu_crtc_enable(struct drm_crtc *crtc,
> */
> if (dpu_encoder_get_intf_mode(encoder) == INTF_MODE_VIDEO)
> request_bandwidth = true;
> - dpu_encoder_register_frame_event_callback(encoder,
> - dpu_crtc_frame_event_cb, (void *)crtc);
> }
>
> if (request_bandwidth)
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> index 539b68b1626a..3aa536d95721 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> @@ -300,4 +300,18 @@ static inline enum dpu_crtc_client_type dpu_crtc_get_client_type(
> return crtc && crtc->state ? RT_CLIENT : NRT_CLIENT;
> }
>
> +/**
> + * dpu_crtc_frame_event_cb - crtc frame event callback API
> + * @crtc: Pointer to crtc
> + * @event: Event to process
> + *
> + * CRTC module registers this API to encoder for all frame event callbacks like
> + * frame_error, frame_done, idle_timeout, etc. Encoder may call different events
> + * from different context - IRQ, user thread, commit_thread, etc. Each event
> + * should be carefully reviewed and should be processed in proper task context
> + * to avoid schedulin delay or properly manage the irq context's bottom half
> + * processing.
> + */
> +void dpu_crtc_frame_event_cb(struct drm_crtc *crtc, u32 event);
> +
> #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 84f8c8a1b049..bf0af5af4306 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -147,8 +147,6 @@ enum dpu_enc_rc_states {
> * @frame_busy_mask: Bitmask tracking which phys_enc we are still
> * busy processing current command.
> * Bit0 = phys_encs[0] etc.
> - * @crtc_frame_event_cb: callback handler for frame event
> - * @crtc_frame_event_cb_data: callback handler private data
> * @frame_done_timeout_ms: frame done timeout in ms
> * @frame_done_timer: watchdog timer for frame done event
> * @vsync_event_timer: vsync timer
> @@ -187,8 +185,6 @@ struct dpu_encoder_virt {
> struct dentry *debugfs_root;
> struct mutex enc_lock;
> DECLARE_BITMAP(frame_busy_mask, MAX_PHYS_ENCODERS_PER_VIRTUAL);
> - void (*crtc_frame_event_cb)(void *, u32 event);
> - void *crtc_frame_event_cb_data;
>
> atomic_t frame_done_timeout_ms;
> struct timer_list frame_done_timer;
> @@ -1358,28 +1354,6 @@ void dpu_encoder_toggle_vblank_for_crtc(struct drm_encoder *drm_enc,
> }
> }
>
> -void dpu_encoder_register_frame_event_callback(struct drm_encoder *drm_enc,
> - void (*frame_event_cb)(void *, u32 event),
> - void *frame_event_cb_data)
> -{
> - struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
> - unsigned long lock_flags;
> - bool enable;
> -
> - enable = frame_event_cb ? true : false;
> -
> - if (!drm_enc) {
> - DPU_ERROR("invalid encoder\n");
> - return;
> - }
> - trace_dpu_enc_frame_event_cb(DRMID(drm_enc), enable);
> -
> - spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags);
> - dpu_enc->crtc_frame_event_cb = frame_event_cb;
> - dpu_enc->crtc_frame_event_cb_data = frame_event_cb_data;
> - spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags);
> -}
> -
> void dpu_encoder_frame_done_callback(
> struct drm_encoder *drm_enc,
> struct dpu_encoder_phys *ready_phys, u32 event)
> @@ -1418,15 +1392,12 @@ void dpu_encoder_frame_done_callback(
> dpu_encoder_resource_control(drm_enc,
> DPU_ENC_RC_EVENT_FRAME_DONE);
>
> - if (dpu_enc->crtc_frame_event_cb)
> - dpu_enc->crtc_frame_event_cb(
> - dpu_enc->crtc_frame_event_cb_data,
> - event);
> + if (dpu_enc->crtc)
> + dpu_crtc_frame_event_cb(dpu_enc->crtc, event);
> }
> } else {
> - if (dpu_enc->crtc_frame_event_cb)
> - dpu_enc->crtc_frame_event_cb(
> - dpu_enc->crtc_frame_event_cb_data, event);
> + if (dpu_enc->crtc)
> + dpu_crtc_frame_event_cb(dpu_enc->crtc, event);
> }
> }
>
> @@ -2367,7 +2338,7 @@ static void dpu_encoder_frame_done_timeout(struct timer_list *t)
> return;
> }
>
> - if (!dpu_enc->frame_busy_mask[0] || !dpu_enc->crtc_frame_event_cb) {
> + if (!dpu_enc->frame_busy_mask[0] || !dpu_enc->crtc) {
> DRM_DEBUG_KMS("id:%u invalid timeout frame_busy_mask=%lu\n",
> DRMID(drm_enc), dpu_enc->frame_busy_mask[0]);
> return;
> @@ -2380,7 +2351,7 @@ static void dpu_encoder_frame_done_timeout(struct timer_list *t)
>
> event = DPU_ENCODER_FRAME_EVENT_ERROR;
> trace_dpu_enc_frame_done_timeout(DRMID(drm_enc), event);
> - dpu_enc->crtc_frame_event_cb(dpu_enc->crtc_frame_event_cb_data, event);
> + dpu_crtc_frame_event_cb(dpu_enc->crtc, event);
> }
>
> static const struct drm_encoder_helper_funcs dpu_encoder_helper_funcs = {
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> index 9e7236ef34e6..dd9cff4f1606 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> @@ -57,16 +57,6 @@ void dpu_encoder_assign_crtc(struct drm_encoder *encoder,
> void dpu_encoder_toggle_vblank_for_crtc(struct drm_encoder *encoder,
> struct drm_crtc *crtc, bool enable);
>
> -/**
> - * dpu_encoder_register_frame_event_callback - provide callback to encoder that
> - * will be called after the request is complete, or other events.
> - * @encoder: encoder pointer
> - * @cb: callback pointer, provide NULL to deregister
> - * @data: user data provided to callback
> - */
> -void dpu_encoder_register_frame_event_callback(struct drm_encoder *encoder,
> - void (*cb)(void *, u32), void *data);
> -
> /**
> * dpu_encoder_prepare_for_kickoff - schedule double buffer flip of the ctl
> * path (i.e. ctl flush and start) at next appropriate time.
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
> index 76169f406505..c75b4363bfc6 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
> @@ -346,10 +346,6 @@ DEFINE_EVENT(dpu_enc_id_enable_template, dpu_enc_vblank_cb,
> TP_PROTO(uint32_t drm_id, bool enable),
> TP_ARGS(drm_id, enable)
> );
> -DEFINE_EVENT(dpu_enc_id_enable_template, dpu_enc_frame_event_cb,
> - TP_PROTO(uint32_t drm_id, bool enable),
> - TP_ARGS(drm_id, enable)
> -);
> DEFINE_EVENT(dpu_enc_id_enable_template, dpu_enc_phys_cmd_connect_te,
> TP_PROTO(uint32_t drm_id, bool enable),
> TP_ARGS(drm_id, enable)
More information about the dri-devel
mailing list