Re: [PATCH v2] drm/msm/dpu: remove CRTC frame event callback registration

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux