| Message ID | 20181113205257.170707-3-sean@poorly.run (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/8] drm/msm: dpu: Move pm_runtime_(get|put) from vblank_enable | expand |
On 2018-11-13 12:52, Sean Paul wrote: > From: Sean Paul <seanpaul@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@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. > > /* 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
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@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@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? 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? 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
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@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@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
On Wed, Nov 14, 2018 at 11:33:53AM -0800, Jeykumar Sankaran wrote: > 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@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@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. Right, we'd need to wrap the callback in the encoder spinlock. > > > > 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. > I took a look at how that might work, and it seems possible but probably not worthwhile. Sean > 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
On Wed, Nov 14, 2018 at 02:43:51PM -0500, Sean Paul wrote: > On Wed, Nov 14, 2018 at 11:33:53AM -0800, Jeykumar Sankaran wrote: > > 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@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@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. > > Right, we'd need to wrap the callback in the encoder spinlock. Just went to do this, and it's already wrapped in the encoder spinlock. So AFAICT, no further changes needed. Sean > > > > > > > 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. > > > > I took a look at how that might work, and it seems possible but probably not > worthwhile. > > Sean > > > 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 > > -- > Sean Paul, Software Engineer, Google / Chromium OS
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); /* 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