Re: [PATCH v2 19/24] drm/msm: dpu: Remove vblank_callback from encoder

Jeykumar Sankaran
2018-11-19T20:11:11Z

On 2018-11-16 10:42, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
>
> The indirection of registering a callback and opaque pointer isn't
> reall
> useful when there's only one callsite. So instead of having the
> vblank_cb registration, just give encoder a crtc and let it directly
> call the vblank handler.
>
> In a later patch, we'll make use of this further.
>
> Changes in v2:
> - None
>
> Cc: Jeykumar Sankaran <jsanka@codeaurora.org>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>

Reviewed-by: Jeykumar Sankaran <jsanka@codeaurora.org>

> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 8 +++----
> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 6 +++++
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 25 +++++++++++----------
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 10 ++++-----
> 4 files changed, 26 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index 141ed1b0e90a..4d7f9ff1e9f4 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -293,9 +293,8 @@ enum dpu_intf_mode dpu_crtc_get_intf_mode(struct
> drm_crtc *crtc)
> return INTF_MODE_NONE;
> }
>
> -static void dpu_crtc_vblank_cb(void *data)
> +void dpu_crtc_vblank_callback(struct drm_crtc *crtc)
> {
> - struct drm_crtc *crtc = (struct drm_crtc *)data;
> struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
>
> /* keep statistics on vblank callback - with auto reset via
> debugfs */
> @@ -771,8 +770,7 @@ static void _dpu_crtc_vblank_enable_no_lock(
> DRMID(enc), enable,
> dpu_crtc);
>
> - dpu_encoder_register_vblank_callback(enc,
> - dpu_crtc_vblank_cb, (void *)crtc);
> + dpu_encoder_assign_crtc(enc, crtc);
> }
> } else {
> list_for_each_entry(enc, &dev->mode_config.encoder_list,
> head) {
> @@ -783,7 +781,7 @@ static void _dpu_crtc_vblank_enable_no_lock(
> DRMID(enc), enable,
> dpu_crtc);
>
> - dpu_encoder_register_vblank_callback(enc, NULL,
> NULL);
> + dpu_encoder_assign_crtc(enc, NULL);
> }
> }
> }
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> index 93d21a61a040..54595cc29be5 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> @@ -270,6 +270,12 @@ static inline int dpu_crtc_frame_pending(struct
> drm_crtc *crtc)
> */
> int dpu_crtc_vblank(struct drm_crtc *crtc, bool en);
>
> +/**
> + * dpu_crtc_vblank_callback - called on vblank irq, issues completion
> events
> + * @crtc: Pointer to drm crtc object
> + */
> +void dpu_crtc_vblank_callback(struct drm_crtc *crtc);
> +
> /**
> * dpu_crtc_commit_kickoff - trigger kickoff of the commit for this
> crtc
> * @crtc: Pointer to drm crtc object
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index d89ac520f7e6..fd6514f681ae 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -142,9 +142,11 @@ enum dpu_enc_rc_states {
> * @intfs_swapped Whether or not the phys_enc interfaces have been
> swapped
> * for partial update right-only cases, such as
> pingpong
> * split where virtual pingpong does not generate
> IRQs
> - * @crtc_vblank_cb: Callback into the upper layer / CRTC for
> - * notification of the VBLANK
> - * @crtc_vblank_cb_data: Data from upper layer for VBLANK
> notification
> + * @crtc: Pointer to the currently assigned crtc. Normally
> you
> + * would use crtc->state->encoder_mask to determine
> the
> + * link between encoder/crtc. However in this case we
> need
> + * to track crtc in the disable() hook which is
> called
> + * _after_ encoder_mask is cleared.
> * @crtc_kickoff_cb: Callback into CRTC that will flush & start
> * all CTL paths
> * @crtc_kickoff_cb_data: Opaque user data given to crtc_kickoff_cb
> @@ -186,8 +188,7 @@ struct dpu_encoder_virt {
>
> bool intfs_swapped;
>
> - void (*crtc_vblank_cb)(void *);
> - void *crtc_vblank_cb_data;
> + struct drm_crtc *crtc;
>
> struct dentry *debugfs_root;
> struct mutex enc_lock;
> @@ -1241,8 +1242,8 @@ static void dpu_encoder_vblank_callback(struct
> drm_encoder *drm_enc,
> dpu_enc = to_dpu_encoder_virt(drm_enc);
>
> spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags);
> - if (dpu_enc->crtc_vblank_cb)
> - dpu_enc->crtc_vblank_cb(dpu_enc->crtc_vblank_cb_data);
> + if (dpu_enc->crtc)
> + dpu_crtc_vblank_callback(dpu_enc->crtc);
> spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags);
>
> atomic_inc(&phy_enc->vsync_cnt);
> @@ -1262,15 +1263,14 @@ static void
> dpu_encoder_underrun_callback(struct
> drm_encoder *drm_enc,
> DPU_ATRACE_END("encoder_underrun_callback");
> }
>
> -void dpu_encoder_register_vblank_callback(struct drm_encoder *drm_enc,
> - void (*vbl_cb)(void *), void *vbl_data)
> +void dpu_encoder_assign_crtc(struct drm_encoder *drm_enc, struct
> drm_crtc
> *crtc)
> {
> struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
> unsigned long lock_flags;
> bool enable;
> int i;
>
> - enable = vbl_cb ? true : false;
> + enable = crtc ? true : false;
>
> if (!drm_enc) {
> DPU_ERROR("invalid encoder\n");
> @@ -1279,8 +1279,9 @@ void dpu_encoder_register_vblank_callback(struct
> drm_encoder *drm_enc,
> trace_dpu_enc_vblank_cb(DRMID(drm_enc), enable);
>
> spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags);
> - dpu_enc->crtc_vblank_cb = vbl_cb;
> - dpu_enc->crtc_vblank_cb_data = vbl_data;
> + /* crtc should always be cleared before re-assigning */
> + WARN_ON(crtc && dpu_enc->crtc);
> + dpu_enc->crtc = crtc;
> spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags);
>
> for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> index aa4f135218fa..be1d80867834 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> @@ -55,14 +55,12 @@ void dpu_encoder_get_hw_resources(struct
> drm_encoder
> *encoder,
> struct dpu_encoder_hw_resources
> *hw_res);
>
> /**
> - * dpu_encoder_register_vblank_callback - provide callback to encoder
> that
> - * will be called on the next vblank.
> + * dpu_encoder_assign_crtc - Link the encoder to the crtc it's
> assigned
> to
> * @encoder: encoder pointer
> - * @cb: callback pointer, provide NULL to deregister and
> disable IRQs
> - * @data: user data provided to callback
> + * @crtc: crtc pointer
> */
> -void dpu_encoder_register_vblank_callback(struct drm_encoder *encoder,
> - void (*cb)(void *), void *data);
> +void dpu_encoder_assign_crtc(struct drm_encoder *encoder,
> + struct drm_crtc *crtc);
>
> /**
> * dpu_encoder_register_frame_event_callback - provide callback to
> encoder that

--
Jeykumar S