| Message ID | 20230102154748.951328-2-dmitry.baryshkov@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [1/2] drm/msm/dpu: remove dpu_encoder_virt_ops | expand |
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@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)
On 14/01/2023 02:49, Abhinav Kumar wrote: > > > 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. As the PSR series have landed, I'd like to point to this patch again. > >> >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@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)
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(). Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@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(-)