On Wed, 7 Feb 2024 at 19:52, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote: > > > > On 10/5/2023 3:06 PM, 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(). > > > > The idea behind the registration was for CRTC to register for events if > it wants to and perhaps have different callbacks for different events > through a common registration mechanism and encoder need not know each > dpu_crtc calls as most of the time we dont want encoder to go back to > crtc to look up what its APIs are. > > But, we are always registering today and have only one callback, so it > kind of makes it an additional redundant wrapper. So I guess, once again > one of those things which , seems not necessary with the current code > but nothing really wrong with it. > > Anyway, couple of comments below. > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> > > --- > > > > This patch was previously posted as a part of the [1] > > > > Changes since v1: > > - Rebased on top of linux-next > > > > [1] https://patchwork.freedesktop.org/series/112353/ > > > > --- > > 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 8ce7586e2ddf..dec5417b69d8 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > @@ -669,18 +669,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; > > @@ -1102,9 +1092,6 @@ static void dpu_crtc_disable(struct drm_crtc *crtc, > > > > dpu_core_perf_crtc_update(crtc, 0); > > > > - 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; > > > > @@ -1143,8 +1130,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. > > + */ > > This doc is no longer correct. > > CRTC module no longer registers anything. Ack. I should have fixed this c&p. > > > +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 d34e684a4178..709fffa4dfa7 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > @@ -148,8 +148,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 > > * @disp_info: local copy of msm_display_info struct > > @@ -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; > > @@ -1377,28 +1373,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) > > @@ -1438,15 +1412,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); > > } > > } > > > > @@ -2318,7 +2289,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) { > > Why do we need !dpu_enc->crtc check for just printing this error log and > returning? This was to keep function semantics: bail out either if there is no frame_busy_mask or if there is no CRTC for this encoder. > > > DRM_DEBUG_KMS("id:%u invalid timeout frame_busy_mask=%lu\n", > > DRMID(drm_enc), dpu_enc->frame_busy_mask[0]); > > return; > > @@ -2331,7 +2302,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 4c05fd5e9ed1..dfa8edeca925 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > > @@ -55,16 +55,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 c74b9be25e68..dc097e109fd2 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) -- With best wishes Dmitry
