[PATCH 3/8] drm/msm: dpu: Remove vblank_callback from encoder
Jeykumar Sankaran
jsanka at codeaurora.org
Wed Nov 14 19:33:53 UTC 2018
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 at chromium.org>
>> >
>> > 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 at chromium.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 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
More information about the dri-devel
mailing list