On 2018-11-14 07:13, Sean Paul wrote:
On Tue, Nov 13, 2018 at 04:47:22PM -0800, Jeykumar Sankaran wrote:
On 2018-11-13 12:52, Sean Paul wrote:
> From: Sean Paul <seanpaul@xxxxxxxxxxxx>
>
> The indirection of registering a callback and opaque pointer isn't
real
> 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.
>
> Signed-off-by: Sean Paul <seanpaul@xxxxxxxxxxxx>
> ---
> 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 adda0aa0cbaa..38119b4d4a80 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -291,9 +291,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);
Since we are calling into a locally stored CRTC and not tracking
disable.
Should
we check for crtc->enable before processing the callback?
I see vblank_on/off cant be called when CRTC is disabled with the rest
of the patches in the series. But this callback is triggered by IRQ's
context.
Hmm, yeah, I had assumed that we wouldn't have any interrupts after irq
disable.
However it doesn't seem like that's the case.
That said, I don't think checking crtc->enable is quite what we want.
In
the
case of disable, the crtc is cleared from the encoder, so checking for
crtc != NULL would be better here. SGTY?
That would still cause a race as crtc assignment will be protected by
modeset
locks and crtc != NULL check isn't.
Now that I've dug into the vblank irq handling a bit in the encoder, it
seems
like that could be moved to the crtc and a bunch of the
encoder->crtc->encoder
bouncing around could be avoided. Off the top of your head, is there
any
reason
we couldn't move the vblank irq handling into crtc from encoder?
vblank irq handlers are only needed for video mode. As with all the
other
IRQ's, the handlers are implemented in phys_encoder level and the events
are forwarded as and when needed.
BTW, the vblank irq flows from phys_enc->virtual_enc->crtc.
Thanks,
Jeykumar S.
Sean
>
> /* keep statistics on vblank callback - with auto reset via
> debugfs */
> @@ -779,8 +778,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) {
> @@ -791,7 +789,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
--
Jeykumar S
