| Message ID | 20181113205257.170707-6-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> > > Instead of assigning/clearing the crtc on vblank enable/disable, we can > just assign and clear the crtc on modeset. That allows us to just > toggle > the encoder's vblank interrupts on vblank_enable. > > So why is this important? Previously the driver was using the legacy > pointers to assign/clear the crtc. Legacy pointers are cleared _after_ Which pointers are you referring here as legacy pointers? CRTC? > disabling the hardware, so the legacy pointer was valid during > vblank_disable, but that's not something we should rely on. > > Instead of relying on the core ordering the legacy pointer assignments > just so, we'll assign the crtc in dpu_crtc enable/disable. This is the > only place that mapping can change, so we're covered there. > > We're also taking advantage of drm_crtc_vblank_on/off. By using this, > we > ensure that vblank_enable/disable can never be called while the crtc is > off (which means the assigned crtc will always be valid). As such, we What about the drm_vblank_enable/disable triggered by drm_vblank_get when crtc is disabled? What is the expected behavior there? the vblank_requested variable removed by the next patch was introduced to cache the request. > don't need to use modeset locks or the crtc_lock in the > vblank_enable/disable routine to be sure state is consistent. > > ...I think. > > Signed-off-by: Sean Paul <seanpaul@chromium.org> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 77 +++++++++------------ > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 27 +++++--- > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 10 +++ > 3 files changed, 59 insertions(+), 55 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > index 4b7f98a6ab60..59e823281fdf 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > @@ -757,43 +757,6 @@ void dpu_crtc_commit_kickoff(struct drm_crtc > *crtc) > DPU_ATRACE_END("crtc_commit"); > } > > -/** > - * _dpu_crtc_vblank_enable_no_lock - update power resource and vblank > request > - * @dpu_crtc: Pointer to dpu crtc structure > - * @enable: Whether to enable/disable vblanks > - */ > -static void _dpu_crtc_vblank_enable_no_lock( > - struct dpu_crtc *dpu_crtc, bool enable) > -{ > - struct drm_crtc *crtc = &dpu_crtc->base; > - struct drm_device *dev = crtc->dev; > - struct drm_encoder *enc; > - > - if (enable) { > - list_for_each_entry(enc, &dev->mode_config.encoder_list, > head) { > - if (enc->crtc != crtc) > - continue; > - > - > trace_dpu_crtc_vblank_enable(DRMID(&dpu_crtc->base), > - DRMID(enc), enable, > - dpu_crtc); > - > - dpu_encoder_assign_crtc(enc, crtc); > - } > - } else { > - list_for_each_entry(enc, &dev->mode_config.encoder_list, > head) { > - if (enc->crtc != crtc) > - continue; > - > - > trace_dpu_crtc_vblank_enable(DRMID(&dpu_crtc->base), > - DRMID(enc), enable, > - dpu_crtc); > - > - dpu_encoder_assign_crtc(enc, NULL); > - } > - } > -} > - > /** > * dpu_crtc_duplicate_state - state duplicate hook > * @crtc: Pointer to drm crtc structure > @@ -847,6 +810,10 @@ static void dpu_crtc_disable(struct drm_crtc > *crtc, > /* Disable/save vblank irq handling */ > drm_crtc_vblank_off(crtc); > > + drm_for_each_encoder_mask(encoder, crtc->dev, > + old_crtc_state->encoder_mask) > + dpu_encoder_assign_crtc(encoder, NULL); > + > mutex_lock(&dpu_crtc->crtc_lock); > > /* wait for frame_event_done completion */ > @@ -856,9 +823,6 @@ static void dpu_crtc_disable(struct drm_crtc *crtc, > atomic_read(&dpu_crtc->frame_pending)); > > trace_dpu_crtc_disable(DRMID(crtc), false, dpu_crtc); > - if (dpu_crtc->enabled && dpu_crtc->vblank_requested) { > - _dpu_crtc_vblank_enable_no_lock(dpu_crtc, false); > - } > dpu_crtc->enabled = false; > > if (atomic_read(&dpu_crtc->frame_pending)) { > @@ -922,13 +886,13 @@ static void dpu_crtc_enable(struct drm_crtc > *crtc, > > mutex_lock(&dpu_crtc->crtc_lock); > trace_dpu_crtc_enable(DRMID(crtc), true, dpu_crtc); > - if (!dpu_crtc->enabled && dpu_crtc->vblank_requested) { > - _dpu_crtc_vblank_enable_no_lock(dpu_crtc, true); > - } > dpu_crtc->enabled = true; > > mutex_unlock(&dpu_crtc->crtc_lock); > > + drm_for_each_encoder_mask(encoder, crtc->dev, > crtc->state->encoder_mask) > + dpu_encoder_assign_crtc(encoder, crtc); > + > /* Enable/restore vblank irq handling */ > drm_crtc_vblank_on(crtc); > } > @@ -1173,10 +1137,33 @@ static int dpu_crtc_atomic_check(struct > drm_crtc > *crtc, > int dpu_crtc_vblank(struct drm_crtc *crtc, bool en) > { > struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc); > + struct drm_encoder *enc; > > - mutex_lock(&dpu_crtc->crtc_lock); > trace_dpu_crtc_vblank(DRMID(&dpu_crtc->base), en, dpu_crtc); > - _dpu_crtc_vblank_enable_no_lock(dpu_crtc, en); > + > + /* > + * Normally we would iterate through encoder_mask in crtc state to > find > + * attached encoders. In this case, we might be disabling vblank > _after_ > + * encoder_mask has been cleared. > + * > + * Instead, we "assign" a crtc to the encoder in enable and clear > it in > + * disable (which is also after encoder_mask is cleared). So > instead of > + * using encoder mask, we'll ask the encoder to toggle itself iff > it's > + * currently assigned to our crtc. > + * > + * Note also that this function cannot be called while crtc is > disabled > + * since we use drm_crtc_vblank_on/off. So we don't need to worry > + * about the assigned crtcs being inconsistent with the current > state > + * (which means no need to worry about modeset locks). > + */ > + list_for_each_entry(enc, &crtc->dev->mode_config.encoder_list, > head) { > + trace_dpu_crtc_vblank_enable(DRMID(crtc), DRMID(enc), en, > + dpu_crtc); > + > + dpu_encoder_toggle_vblank_for_crtc(enc, crtc, en); > + } > + > + mutex_lock(&dpu_crtc->crtc_lock); > dpu_crtc->vblank_requested = en; > mutex_unlock(&dpu_crtc->crtc_lock); > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > index fd6514f681ae..5914ae70572c 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > @@ -1267,22 +1267,29 @@ 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 = crtc ? true : false; > - > - if (!drm_enc) { > - DPU_ERROR("invalid encoder\n"); > - return; > - } > - trace_dpu_enc_vblank_cb(DRMID(drm_enc), enable); > > spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags); > /* 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); > +} > + > +void dpu_encoder_toggle_vblank_for_crtc(struct drm_encoder *drm_enc, > + struct drm_crtc *crtc, bool > enable) > +{ > + struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc); > + unsigned long lock_flags; > + int i; > + > + trace_dpu_enc_vblank_cb(DRMID(drm_enc), enable); > + > + spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags); > + if (dpu_enc->crtc == crtc) { > + spin_unlock_irqrestore(&dpu_enc->enc_spinlock, > lock_flags); > + return; > + } Why are you returning when the crtc's are same? Won't they be same all the time between modesets? for both enable/disable crtc will be the same and you still need to call the for loop below to enable/disable the phys encoders. > + spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags); > > for (i = 0; i < dpu_enc->num_phys_encs; i++) { > struct dpu_encoder_phys *phys = dpu_enc->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 be1d80867834..6896ea2ab854 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > @@ -62,6 +62,16 @@ void dpu_encoder_get_hw_resources(struct drm_encoder > *encoder, > void dpu_encoder_assign_crtc(struct drm_encoder *encoder, > struct drm_crtc *crtc); > > +/** > + * dpu_encoder_toggle_vblank_for_crtc - Toggles vblank interrupts on > or > off if > + * the encoder is assigned to the given crtc > + * @encoder: encoder pointer > + * @crtc: crtc pointer > + * @enable: true if vblank should be enabled > + */ > +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.
On Tue, Nov 13, 2018 at 04:48:12PM -0800, Jeykumar Sankaran wrote: > On 2018-11-13 12:52, Sean Paul wrote: > > From: Sean Paul <seanpaul@chromium.org> > > > > Instead of assigning/clearing the crtc on vblank enable/disable, we can > > just assign and clear the crtc on modeset. That allows us to just toggle > > the encoder's vblank interrupts on vblank_enable. > > > > So why is this important? Previously the driver was using the legacy > > pointers to assign/clear the crtc. Legacy pointers are cleared _after_ > Which pointers are you referring here as legacy pointers? CRTC? encoder->crtc in this case. The loop in vblank_enable_no_lock relies on enc->crtc == crtc > > disabling the hardware, so the legacy pointer was valid during > > vblank_disable, but that's not something we should rely on. > > > > Instead of relying on the core ordering the legacy pointer assignments > > just so, we'll assign the crtc in dpu_crtc enable/disable. This is the > > only place that mapping can change, so we're covered there. > > > > We're also taking advantage of drm_crtc_vblank_on/off. By using this, we > > ensure that vblank_enable/disable can never be called while the crtc is > > off (which means the assigned crtc will always be valid). As such, we > > What about the drm_vblank_enable/disable triggered by drm_vblank_get when > crtc > is disabled? What is the expected behavior there? the vblank_requested > variable removed by the next patch was introduced to cache the request. That case is covered by the modeset locks held when calling disable(). > > > don't need to use modeset locks or the crtc_lock in the > > vblank_enable/disable routine to be sure state is consistent. > > > > ...I think. > > > > Signed-off-by: Sean Paul <seanpaul@chromium.org> > > --- > > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 77 +++++++++------------ > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 27 +++++--- > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 10 +++ > > 3 files changed, 59 insertions(+), 55 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > index 4b7f98a6ab60..59e823281fdf 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > @@ -757,43 +757,6 @@ void dpu_crtc_commit_kickoff(struct drm_crtc *crtc) > > DPU_ATRACE_END("crtc_commit"); > > } > > > > -/** > > - * _dpu_crtc_vblank_enable_no_lock - update power resource and vblank > > request > > - * @dpu_crtc: Pointer to dpu crtc structure > > - * @enable: Whether to enable/disable vblanks > > - */ > > -static void _dpu_crtc_vblank_enable_no_lock( > > - struct dpu_crtc *dpu_crtc, bool enable) > > -{ > > - struct drm_crtc *crtc = &dpu_crtc->base; > > - struct drm_device *dev = crtc->dev; > > - struct drm_encoder *enc; > > - > > - if (enable) { > > - list_for_each_entry(enc, &dev->mode_config.encoder_list, > > head) { > > - if (enc->crtc != crtc) > > - continue; > > - > > - > > trace_dpu_crtc_vblank_enable(DRMID(&dpu_crtc->base), > > - DRMID(enc), enable, > > - dpu_crtc); > > - > > - dpu_encoder_assign_crtc(enc, crtc); > > - } > > - } else { > > - list_for_each_entry(enc, &dev->mode_config.encoder_list, > > head) { > > - if (enc->crtc != crtc) > > - continue; > > - > > - > > trace_dpu_crtc_vblank_enable(DRMID(&dpu_crtc->base), > > - DRMID(enc), enable, > > - dpu_crtc); > > - > > - dpu_encoder_assign_crtc(enc, NULL); > > - } > > - } > > -} > > - > > /** > > * dpu_crtc_duplicate_state - state duplicate hook > > * @crtc: Pointer to drm crtc structure > > @@ -847,6 +810,10 @@ static void dpu_crtc_disable(struct drm_crtc *crtc, > > /* Disable/save vblank irq handling */ > > drm_crtc_vblank_off(crtc); > > > > + drm_for_each_encoder_mask(encoder, crtc->dev, > > + old_crtc_state->encoder_mask) > > + dpu_encoder_assign_crtc(encoder, NULL); > > + > > mutex_lock(&dpu_crtc->crtc_lock); > > > > /* wait for frame_event_done completion */ > > @@ -856,9 +823,6 @@ static void dpu_crtc_disable(struct drm_crtc *crtc, > > atomic_read(&dpu_crtc->frame_pending)); > > > > trace_dpu_crtc_disable(DRMID(crtc), false, dpu_crtc); > > - if (dpu_crtc->enabled && dpu_crtc->vblank_requested) { > > - _dpu_crtc_vblank_enable_no_lock(dpu_crtc, false); > > - } > > dpu_crtc->enabled = false; > > > > if (atomic_read(&dpu_crtc->frame_pending)) { > > @@ -922,13 +886,13 @@ static void dpu_crtc_enable(struct drm_crtc *crtc, > > > > mutex_lock(&dpu_crtc->crtc_lock); > > trace_dpu_crtc_enable(DRMID(crtc), true, dpu_crtc); > > - if (!dpu_crtc->enabled && dpu_crtc->vblank_requested) { > > - _dpu_crtc_vblank_enable_no_lock(dpu_crtc, true); > > - } > > dpu_crtc->enabled = true; > > > > mutex_unlock(&dpu_crtc->crtc_lock); > > > > + drm_for_each_encoder_mask(encoder, crtc->dev, > > crtc->state->encoder_mask) > > + dpu_encoder_assign_crtc(encoder, crtc); > > + > > /* Enable/restore vblank irq handling */ > > drm_crtc_vblank_on(crtc); > > } > > @@ -1173,10 +1137,33 @@ static int dpu_crtc_atomic_check(struct drm_crtc > > *crtc, > > int dpu_crtc_vblank(struct drm_crtc *crtc, bool en) > > { > > struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc); > > + struct drm_encoder *enc; > > > > - mutex_lock(&dpu_crtc->crtc_lock); > > trace_dpu_crtc_vblank(DRMID(&dpu_crtc->base), en, dpu_crtc); > > - _dpu_crtc_vblank_enable_no_lock(dpu_crtc, en); > > + > > + /* > > + * Normally we would iterate through encoder_mask in crtc state to > > find > > + * attached encoders. In this case, we might be disabling vblank > > _after_ > > + * encoder_mask has been cleared. > > + * > > + * Instead, we "assign" a crtc to the encoder in enable and clear > > it in > > + * disable (which is also after encoder_mask is cleared). So > > instead of > > + * using encoder mask, we'll ask the encoder to toggle itself iff > > it's > > + * currently assigned to our crtc. > > + * > > + * Note also that this function cannot be called while crtc is > > disabled > > + * since we use drm_crtc_vblank_on/off. So we don't need to worry > > + * about the assigned crtcs being inconsistent with the current > > state > > + * (which means no need to worry about modeset locks). > > + */ > > + list_for_each_entry(enc, &crtc->dev->mode_config.encoder_list, > > head) { > > + trace_dpu_crtc_vblank_enable(DRMID(crtc), DRMID(enc), en, > > + dpu_crtc); > > + > > + dpu_encoder_toggle_vblank_for_crtc(enc, crtc, en); > > + } > > + > > + mutex_lock(&dpu_crtc->crtc_lock); > > dpu_crtc->vblank_requested = en; > > mutex_unlock(&dpu_crtc->crtc_lock); > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > index fd6514f681ae..5914ae70572c 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > @@ -1267,22 +1267,29 @@ 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 = crtc ? true : false; > > - > > - if (!drm_enc) { > > - DPU_ERROR("invalid encoder\n"); > > - return; > > - } > > - trace_dpu_enc_vblank_cb(DRMID(drm_enc), enable); > > > > spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags); > > /* 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); > > +} > > + > > +void dpu_encoder_toggle_vblank_for_crtc(struct drm_encoder *drm_enc, > > + struct drm_crtc *crtc, bool > > enable) > > +{ > > + struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc); > > + unsigned long lock_flags; > > + int i; > > + > > + trace_dpu_enc_vblank_cb(DRMID(drm_enc), enable); > > + > > + spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags); > > + if (dpu_enc->crtc == crtc) { > > + spin_unlock_irqrestore(&dpu_enc->enc_spinlock, > > lock_flags); > > + return; > > + } > Why are you returning when the crtc's are same? > Won't they be same all the time between modesets? > > for both enable/disable crtc will be the same and you still need to call the > for loop below to enable/disable the phys encoders. > > > + spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags); > > > > for (i = 0; i < dpu_enc->num_phys_encs; i++) { > > struct dpu_encoder_phys *phys = dpu_enc->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 be1d80867834..6896ea2ab854 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > > @@ -62,6 +62,16 @@ void dpu_encoder_get_hw_resources(struct drm_encoder > > *encoder, > > void dpu_encoder_assign_crtc(struct drm_encoder *encoder, > > struct drm_crtc *crtc); > > > > +/** > > + * dpu_encoder_toggle_vblank_for_crtc - Toggles vblank interrupts on or > > off if > > + * the encoder is assigned to the given crtc > > + * @encoder: encoder pointer > > + * @crtc: crtc pointer > > + * @enable: true if vblank should be enabled > > + */ > > +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. > > -- > Jeykumar S
On 2018-11-14 07:16, Sean Paul wrote: > On Tue, Nov 13, 2018 at 04:48:12PM -0800, Jeykumar Sankaran wrote: >> On 2018-11-13 12:52, Sean Paul wrote: >> > From: Sean Paul <seanpaul@chromium.org> >> > >> > Instead of assigning/clearing the crtc on vblank enable/disable, we > can >> > just assign and clear the crtc on modeset. That allows us to just > toggle >> > the encoder's vblank interrupts on vblank_enable. >> > >> > So why is this important? Previously the driver was using the legacy >> > pointers to assign/clear the crtc. Legacy pointers are cleared _after_ >> Which pointers are you referring here as legacy pointers? CRTC? > > encoder->crtc in this case. The loop in vblank_enable_no_lock relies on > enc->crtc == crtc > >> > disabling the hardware, so the legacy pointer was valid during >> > vblank_disable, but that's not something we should rely on. >> > >> > Instead of relying on the core ordering the legacy pointer assignments >> > just so, we'll assign the crtc in dpu_crtc enable/disable. This is the >> > only place that mapping can change, so we're covered there. >> > >> > We're also taking advantage of drm_crtc_vblank_on/off. By using this, > we >> > ensure that vblank_enable/disable can never be called while the crtc > is >> > off (which means the assigned crtc will always be valid). As such, we >> >> What about the drm_vblank_enable/disable triggered by drm_vblank_get > when >> crtc >> is disabled? What is the expected behavior there? the vblank_requested >> variable removed by the next patch was introduced to cache the >> request. > > That case is covered by the modeset locks held when calling disable(). > I am sure it will take care of drm_crtc_vblank_on/off triggered within crtc_disable. My question was what was the expected behaviour when DRM_IOCTL_WAIT_VBLANK is called when crtc is disabled? the ioctl will try to call drm_vblank_get and I don't see the patch checking for crtc being enabled in the path. Thanks and Regards, Jeykumar S. >> >> > don't need to use modeset locks or the crtc_lock in the >> > vblank_enable/disable routine to be sure state is consistent. >> > >> > ...I think. >> > >> > Signed-off-by: Sean Paul <seanpaul@chromium.org> >> > --- >> > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 77 > +++++++++------------ >> > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 27 +++++--- >> > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 10 +++ >> > 3 files changed, 59 insertions(+), 55 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >> > index 4b7f98a6ab60..59e823281fdf 100644 >> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >> > @@ -757,43 +757,6 @@ void dpu_crtc_commit_kickoff(struct drm_crtc > *crtc) >> > DPU_ATRACE_END("crtc_commit"); >> > } >> > >> > -/** >> > - * _dpu_crtc_vblank_enable_no_lock - update power resource and vblank >> > request >> > - * @dpu_crtc: Pointer to dpu crtc structure >> > - * @enable: Whether to enable/disable vblanks >> > - */ >> > -static void _dpu_crtc_vblank_enable_no_lock( >> > - struct dpu_crtc *dpu_crtc, bool enable) >> > -{ >> > - struct drm_crtc *crtc = &dpu_crtc->base; >> > - struct drm_device *dev = crtc->dev; >> > - struct drm_encoder *enc; >> > - >> > - if (enable) { >> > - list_for_each_entry(enc, &dev->mode_config.encoder_list, >> > head) { >> > - if (enc->crtc != crtc) >> > - continue; >> > - >> > - >> > trace_dpu_crtc_vblank_enable(DRMID(&dpu_crtc->base), >> > - DRMID(enc), enable, >> > - dpu_crtc); >> > - >> > - dpu_encoder_assign_crtc(enc, crtc); >> > - } >> > - } else { >> > - list_for_each_entry(enc, &dev->mode_config.encoder_list, >> > head) { >> > - if (enc->crtc != crtc) >> > - continue; >> > - >> > - >> > trace_dpu_crtc_vblank_enable(DRMID(&dpu_crtc->base), >> > - DRMID(enc), enable, >> > - dpu_crtc); >> > - >> > - dpu_encoder_assign_crtc(enc, NULL); >> > - } >> > - } >> > -} >> > - >> > /** >> > * dpu_crtc_duplicate_state - state duplicate hook >> > * @crtc: Pointer to drm crtc structure >> > @@ -847,6 +810,10 @@ static void dpu_crtc_disable(struct drm_crtc > *crtc, >> > /* Disable/save vblank irq handling */ >> > drm_crtc_vblank_off(crtc); >> > >> > + drm_for_each_encoder_mask(encoder, crtc->dev, >> > + old_crtc_state->encoder_mask) >> > + dpu_encoder_assign_crtc(encoder, NULL); >> > + >> > mutex_lock(&dpu_crtc->crtc_lock); >> > >> > /* wait for frame_event_done completion */ >> > @@ -856,9 +823,6 @@ static void dpu_crtc_disable(struct drm_crtc > *crtc, >> > atomic_read(&dpu_crtc->frame_pending)); >> > >> > trace_dpu_crtc_disable(DRMID(crtc), false, dpu_crtc); >> > - if (dpu_crtc->enabled && dpu_crtc->vblank_requested) { >> > - _dpu_crtc_vblank_enable_no_lock(dpu_crtc, false); >> > - } >> > dpu_crtc->enabled = false; >> > >> > if (atomic_read(&dpu_crtc->frame_pending)) { >> > @@ -922,13 +886,13 @@ static void dpu_crtc_enable(struct drm_crtc > *crtc, >> > >> > mutex_lock(&dpu_crtc->crtc_lock); >> > trace_dpu_crtc_enable(DRMID(crtc), true, dpu_crtc); >> > - if (!dpu_crtc->enabled && dpu_crtc->vblank_requested) { >> > - _dpu_crtc_vblank_enable_no_lock(dpu_crtc, true); >> > - } >> > dpu_crtc->enabled = true; >> > >> > mutex_unlock(&dpu_crtc->crtc_lock); >> > >> > + drm_for_each_encoder_mask(encoder, crtc->dev, >> > crtc->state->encoder_mask) >> > + dpu_encoder_assign_crtc(encoder, crtc); >> > + >> > /* Enable/restore vblank irq handling */ >> > drm_crtc_vblank_on(crtc); >> > } >> > @@ -1173,10 +1137,33 @@ static int dpu_crtc_atomic_check(struct > drm_crtc >> > *crtc, >> > int dpu_crtc_vblank(struct drm_crtc *crtc, bool en) >> > { >> > struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc); >> > + struct drm_encoder *enc; >> > >> > - mutex_lock(&dpu_crtc->crtc_lock); >> > trace_dpu_crtc_vblank(DRMID(&dpu_crtc->base), en, dpu_crtc); >> > - _dpu_crtc_vblank_enable_no_lock(dpu_crtc, en); >> > + >> > + /* >> > + * Normally we would iterate through encoder_mask in crtc state to >> > find >> > + * attached encoders. In this case, we might be disabling vblank >> > _after_ >> > + * encoder_mask has been cleared. >> > + * >> > + * Instead, we "assign" a crtc to the encoder in enable and clear >> > it in >> > + * disable (which is also after encoder_mask is cleared). So >> > instead of >> > + * using encoder mask, we'll ask the encoder to toggle itself iff >> > it's >> > + * currently assigned to our crtc. >> > + * >> > + * Note also that this function cannot be called while crtc is >> > disabled >> > + * since we use drm_crtc_vblank_on/off. So we don't need to worry >> > + * about the assigned crtcs being inconsistent with the current >> > state >> > + * (which means no need to worry about modeset locks). >> > + */ >> > + list_for_each_entry(enc, &crtc->dev->mode_config.encoder_list, >> > head) { >> > + trace_dpu_crtc_vblank_enable(DRMID(crtc), DRMID(enc), en, >> > + dpu_crtc); >> > + >> > + dpu_encoder_toggle_vblank_for_crtc(enc, crtc, en); >> > + } >> > + >> > + mutex_lock(&dpu_crtc->crtc_lock); >> > dpu_crtc->vblank_requested = en; >> > mutex_unlock(&dpu_crtc->crtc_lock); >> > >> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> > index fd6514f681ae..5914ae70572c 100644 >> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> > @@ -1267,22 +1267,29 @@ 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 = crtc ? true : false; >> > - >> > - if (!drm_enc) { >> > - DPU_ERROR("invalid encoder\n"); >> > - return; >> > - } >> > - trace_dpu_enc_vblank_cb(DRMID(drm_enc), enable); >> > >> > spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags); >> > /* 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); >> > +} >> > + >> > +void dpu_encoder_toggle_vblank_for_crtc(struct drm_encoder *drm_enc, >> > + struct drm_crtc *crtc, bool >> > enable) >> > +{ >> > + struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc); >> > + unsigned long lock_flags; >> > + int i; >> > + >> > + trace_dpu_enc_vblank_cb(DRMID(drm_enc), enable); >> > + >> > + spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags); >> > + if (dpu_enc->crtc == crtc) { >> > + spin_unlock_irqrestore(&dpu_enc->enc_spinlock, >> > lock_flags); >> > + return; >> > + } >> Why are you returning when the crtc's are same? >> Won't they be same all the time between modesets? >> >> for both enable/disable crtc will be the same and you still need to >> call > the >> for loop below to enable/disable the phys encoders. >> Can you go through my comments above? >> > + spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags); >> > >> > for (i = 0; i < dpu_enc->num_phys_encs; i++) { >> > struct dpu_encoder_phys *phys = dpu_enc->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 be1d80867834..6896ea2ab854 100644 >> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h >> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h >> > @@ -62,6 +62,16 @@ void dpu_encoder_get_hw_resources(struct > drm_encoder >> > *encoder, >> > void dpu_encoder_assign_crtc(struct drm_encoder *encoder, >> > struct drm_crtc *crtc); >> > >> > +/** >> > + * dpu_encoder_toggle_vblank_for_crtc - Toggles vblank interrupts on > or >> > off if >> > + * the encoder is assigned to the given crtc >> > + * @encoder: encoder pointer >> > + * @crtc: crtc pointer >> > + * @enable: true if vblank should be enabled >> > + */ >> > +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. >> >> -- >> Jeykumar S
On Wed, Nov 14, 2018 at 11:43:51AM -0800, Jeykumar Sankaran wrote: > On 2018-11-14 07:16, Sean Paul wrote: > > On Tue, Nov 13, 2018 at 04:48:12PM -0800, Jeykumar Sankaran wrote: > > > On 2018-11-13 12:52, Sean Paul wrote: > > > > From: Sean Paul <seanpaul@chromium.org> > > > > > > > > Instead of assigning/clearing the crtc on vblank enable/disable, we > > can > > > > just assign and clear the crtc on modeset. That allows us to just > > toggle > > > > the encoder's vblank interrupts on vblank_enable. > > > > > > > > So why is this important? Previously the driver was using the legacy > > > > pointers to assign/clear the crtc. Legacy pointers are cleared _after_ > > > Which pointers are you referring here as legacy pointers? CRTC? > > > > encoder->crtc in this case. The loop in vblank_enable_no_lock relies on > > enc->crtc == crtc > > > > > > disabling the hardware, so the legacy pointer was valid during > > > > vblank_disable, but that's not something we should rely on. > > > > > > > > Instead of relying on the core ordering the legacy pointer assignments > > > > just so, we'll assign the crtc in dpu_crtc enable/disable. This is the > > > > only place that mapping can change, so we're covered there. > > > > > > > > We're also taking advantage of drm_crtc_vblank_on/off. By using this, > > we > > > > ensure that vblank_enable/disable can never be called while the crtc > > is > > > > off (which means the assigned crtc will always be valid). As such, we > > > > > > What about the drm_vblank_enable/disable triggered by drm_vblank_get > > when > > > crtc > > > is disabled? What is the expected behavior there? the vblank_requested > > > variable removed by the next patch was introduced to cache the > > > request. > > > > That case is covered by the modeset locks held when calling disable(). > > > I am sure it will take care of drm_crtc_vblank_on/off triggered within > crtc_disable. > My question was what was the expected behaviour when DRM_IOCTL_WAIT_VBLANK > is > called when crtc is disabled? the ioctl will try to call drm_vblank_get and > I > don't see the patch checking for crtc being enabled in the path. It's the same as any other drm_vblank_get call when the crtc is disabled, it's gated internally by the core when vblank is turned off. > > Thanks and Regards, > Jeykumar S. > > > > > > > > > don't need to use modeset locks or the crtc_lock in the > > > > vblank_enable/disable routine to be sure state is consistent. > > > > > > > > ...I think. > > > > > > > > Signed-off-by: Sean Paul <seanpaul@chromium.org> > > > > --- > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 77 > > +++++++++------------ > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 27 +++++--- > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 10 +++ > > > > 3 files changed, 59 insertions(+), 55 deletions(-) > > > > /snip > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > > > index fd6514f681ae..5914ae70572c 100644 > > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > > > @@ -1267,22 +1267,29 @@ 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 = crtc ? true : false; > > > > - > > > > - if (!drm_enc) { > > > > - DPU_ERROR("invalid encoder\n"); > > > > - return; > > > > - } > > > > - trace_dpu_enc_vblank_cb(DRMID(drm_enc), enable); > > > > > > > > spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags); > > > > /* 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); > > > > +} > > > > + > > > > +void dpu_encoder_toggle_vblank_for_crtc(struct drm_encoder *drm_enc, > > > > + struct drm_crtc *crtc, bool > > > > enable) > > > > +{ > > > > + struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc); > > > > + unsigned long lock_flags; > > > > + int i; > > > > + > > > > + trace_dpu_enc_vblank_cb(DRMID(drm_enc), enable); > > > > + > > > > + spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags); > > > > + if (dpu_enc->crtc == crtc) { > > > > + spin_unlock_irqrestore(&dpu_enc->enc_spinlock, > > > > lock_flags); > > > > + return; > > > > + } > > > Why are you returning when the crtc's are same? > > > Won't they be same all the time between modesets? > > > Ugh, well that's embarrassing! This is supposed to be != > > > for both enable/disable crtc will be the same and you still need to > > > call > > the > > > for loop below to enable/disable the phys encoders. > > > > > Can you go through my comments above? Shoot, sorry, I didn't see these. Sean > > > > > + spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags); > > > > > > > > for (i = 0; i < dpu_enc->num_phys_encs; i++) { > > > > struct dpu_encoder_phys *phys = dpu_enc->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 be1d80867834..6896ea2ab854 100644 > > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > > > > @@ -62,6 +62,16 @@ void dpu_encoder_get_hw_resources(struct > > drm_encoder > > > > *encoder, > > > > void dpu_encoder_assign_crtc(struct drm_encoder *encoder, > > > > struct drm_crtc *crtc); > > > > > > > > +/** > > > > + * dpu_encoder_toggle_vblank_for_crtc - Toggles vblank interrupts on > > or > > > > off if > > > > + * the encoder is assigned to the given crtc > > > > + * @encoder: encoder pointer > > > > + * @crtc: crtc pointer > > > > + * @enable: true if vblank should be enabled > > > > + */ > > > > +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. > > > > > > -- > > > Jeykumar S > > -- > Jeykumar S
On Wed, Nov 14, 2018 at 11:43:51AM -0800, Jeykumar Sankaran wrote: > On 2018-11-14 07:16, Sean Paul wrote: > > On Tue, Nov 13, 2018 at 04:48:12PM -0800, Jeykumar Sankaran wrote: > >> On 2018-11-13 12:52, Sean Paul wrote: > >> > From: Sean Paul <seanpaul@chromium.org> > >> > > >> > Instead of assigning/clearing the crtc on vblank enable/disable, we > > can > >> > just assign and clear the crtc on modeset. That allows us to just > > toggle > >> > the encoder's vblank interrupts on vblank_enable. > >> > > >> > So why is this important? Previously the driver was using the legacy > >> > pointers to assign/clear the crtc. Legacy pointers are cleared _after_ > >> Which pointers are you referring here as legacy pointers? CRTC? > > > > encoder->crtc in this case. The loop in vblank_enable_no_lock relies on > > enc->crtc == crtc > > > >> > disabling the hardware, so the legacy pointer was valid during > >> > vblank_disable, but that's not something we should rely on. > >> > > >> > Instead of relying on the core ordering the legacy pointer assignments > >> > just so, we'll assign the crtc in dpu_crtc enable/disable. This is the > >> > only place that mapping can change, so we're covered there. > >> > > >> > We're also taking advantage of drm_crtc_vblank_on/off. By using this, > > we > >> > ensure that vblank_enable/disable can never be called while the crtc > > is > >> > off (which means the assigned crtc will always be valid). As such, we > >> > >> What about the drm_vblank_enable/disable triggered by drm_vblank_get > > when > >> crtc > >> is disabled? What is the expected behavior there? the vblank_requested > >> variable removed by the next patch was introduced to cache the > >> request. > > > > That case is covered by the modeset locks held when calling disable(). > > > I am sure it will take care of drm_crtc_vblank_on/off triggered within > crtc_disable. > My question was what was the expected behaviour when > DRM_IOCTL_WAIT_VBLANK is > called when crtc is disabled? the ioctl will try to call drm_vblank_get > and I > don't see the patch checking for crtc being enabled in the path. drm_vblank_off() // .enable_vblank/.disable_vblank will never be called here drm_vblank_on() So if you use drm_vblank_on/off judiciously you will never have to deal with this particular problem.
On 2018-11-14 11:57, Ville Syrjälä wrote: > On Wed, Nov 14, 2018 at 11:43:51AM -0800, Jeykumar Sankaran wrote: >> On 2018-11-14 07:16, Sean Paul wrote: >> > On Tue, Nov 13, 2018 at 04:48:12PM -0800, Jeykumar Sankaran wrote: >> >> On 2018-11-13 12:52, Sean Paul wrote: >> >> > From: Sean Paul <seanpaul@chromium.org> >> >> > >> >> > Instead of assigning/clearing the crtc on vblank enable/disable, we >> > can >> >> > just assign and clear the crtc on modeset. That allows us to just >> > toggle >> >> > the encoder's vblank interrupts on vblank_enable. >> >> > >> >> > So why is this important? Previously the driver was using the > legacy >> >> > pointers to assign/clear the crtc. Legacy pointers are cleared > _after_ >> >> Which pointers are you referring here as legacy pointers? CRTC? >> > >> > encoder->crtc in this case. The loop in vblank_enable_no_lock relies > on >> > enc->crtc == crtc >> > >> >> > disabling the hardware, so the legacy pointer was valid during >> >> > vblank_disable, but that's not something we should rely on. >> >> > >> >> > Instead of relying on the core ordering the legacy pointer > assignments >> >> > just so, we'll assign the crtc in dpu_crtc enable/disable. This is > the >> >> > only place that mapping can change, so we're covered there. >> >> > >> >> > We're also taking advantage of drm_crtc_vblank_on/off. By using > this, >> > we >> >> > ensure that vblank_enable/disable can never be called while the > crtc >> > is >> >> > off (which means the assigned crtc will always be valid). As such, > we >> >> >> >> What about the drm_vblank_enable/disable triggered by drm_vblank_get >> > when >> >> crtc >> >> is disabled? What is the expected behavior there? the > vblank_requested >> >> variable removed by the next patch was introduced to cache the >> >> request. >> > >> > That case is covered by the modeset locks held when calling disable(). >> > >> I am sure it will take care of drm_crtc_vblank_on/off triggered within >> crtc_disable. >> My question was what was the expected behaviour when >> DRM_IOCTL_WAIT_VBLANK is >> called when crtc is disabled? the ioctl will try to call >> drm_vblank_get >> and I >> don't see the patch checking for crtc being enabled in the path. > > drm_vblank_off() > // .enable_vblank/.disable_vblank will never be called here > drm_vblank_on() > > So if you use drm_vblank_on/off judiciously you will never > have to deal with this particular problem. > Thanks ville for clarifying that. We can make sure to avoid that sequence if we have control over it. In DPU, CRTC calls dpu_vblank_off in crtc_disable. After disabling, no one stopping userspace clients to call into DRM_IOCTL_WAIT_VBLANK on the crtc. This ioctl calls enable_vblank when it sees the vblank is disabled [1]. So far, the dpu_crtc_vblank was failing when it finds that the crtc is disabled. With this patch, the condition was removed, so I was wondering what is the expected behavior with this patch. [1] https://gitlab.freedesktop.org/seanpaul/dpu-staging/blob/for-next/drivers/gpu/drm/drm_vblank.c#L956 Thanks and Regards, Jeykumar S. > -- > Ville Syrjälä > Intel
On Wed, Nov 14, 2018 at 12:46:32PM -0800, Jeykumar Sankaran wrote: > On 2018-11-14 11:57, Ville Syrjälä wrote: > > On Wed, Nov 14, 2018 at 11:43:51AM -0800, Jeykumar Sankaran wrote: > > > On 2018-11-14 07:16, Sean Paul wrote: > > > > On Tue, Nov 13, 2018 at 04:48:12PM -0800, Jeykumar Sankaran wrote: > > > >> On 2018-11-13 12:52, Sean Paul wrote: > > > >> > From: Sean Paul <seanpaul@chromium.org> > > > >> > > > > >> > Instead of assigning/clearing the crtc on vblank enable/disable, we > > > > can > > > >> > just assign and clear the crtc on modeset. That allows us to just > > > > toggle > > > >> > the encoder's vblank interrupts on vblank_enable. > > > >> > > > > >> > So why is this important? Previously the driver was using the > > legacy > > > >> > pointers to assign/clear the crtc. Legacy pointers are cleared > > _after_ > > > >> Which pointers are you referring here as legacy pointers? CRTC? > > > > > > > > encoder->crtc in this case. The loop in vblank_enable_no_lock relies > > on > > > > enc->crtc == crtc > > > > > > > >> > disabling the hardware, so the legacy pointer was valid during > > > >> > vblank_disable, but that's not something we should rely on. > > > >> > > > > >> > Instead of relying on the core ordering the legacy pointer > > assignments > > > >> > just so, we'll assign the crtc in dpu_crtc enable/disable. This is > > the > > > >> > only place that mapping can change, so we're covered there. > > > >> > > > > >> > We're also taking advantage of drm_crtc_vblank_on/off. By using > > this, > > > > we > > > >> > ensure that vblank_enable/disable can never be called while the > > crtc > > > > is > > > >> > off (which means the assigned crtc will always be valid). As such, > > we > > > >> > > > >> What about the drm_vblank_enable/disable triggered by drm_vblank_get > > > > when > > > >> crtc > > > >> is disabled? What is the expected behavior there? the > > vblank_requested > > > >> variable removed by the next patch was introduced to cache the > > > >> request. > > > > > > > > That case is covered by the modeset locks held when calling disable(). > > > > > > > I am sure it will take care of drm_crtc_vblank_on/off triggered within > > > crtc_disable. > > > My question was what was the expected behaviour when > > > DRM_IOCTL_WAIT_VBLANK is > > > called when crtc is disabled? the ioctl will try to call > > > drm_vblank_get > > > and I > > > don't see the patch checking for crtc being enabled in the path. > > > > drm_vblank_off() > > // .enable_vblank/.disable_vblank will never be called here > > drm_vblank_on() > > > > So if you use drm_vblank_on/off judiciously you will never > > have to deal with this particular problem. > > > Thanks ville for clarifying that. > > We can make sure to avoid that sequence if we have control over it. In DPU, > CRTC calls > dpu_vblank_off in crtc_disable. After disabling, no one stopping > userspace clients to call into DRM_IOCTL_WAIT_VBLANK on the crtc. This ioctl > calls enable_vblank when it sees the vblank is disabled [1]. That's prevented by this bit in drm_vblank.c: /* * Prevent subsequent drm_vblank_get() from re-enabling * the vblank interrupt by bumping the refcount. */ if (!vblank->inmodeset) { atomic_inc(&vblank->refcount); vblank->inmodeset = 1; } Basically it turns off the hw and then takes a reference such that drm_vblank_get will never call drm_vblank_enable. Sean > > So far, the dpu_crtc_vblank was failing when it finds that the crtc is > disabled. > With this patch, the condition was removed, so I was wondering what is the > expected behavior with this patch. > > [1] https://gitlab.freedesktop.org/seanpaul/dpu-staging/blob/for-next/drivers/gpu/drm/drm_vblank.c#L956 > > Thanks and Regards, > Jeykumar S. > > -- > > Ville Syrjälä > > Intel > > -- > Jeykumar S
On 2018-11-14 12:52, Sean Paul wrote: > On Wed, Nov 14, 2018 at 12:46:32PM -0800, Jeykumar Sankaran wrote: >> On 2018-11-14 11:57, Ville Syrjälä wrote: >> > On Wed, Nov 14, 2018 at 11:43:51AM -0800, Jeykumar Sankaran wrote: >> > > On 2018-11-14 07:16, Sean Paul wrote: >> > > > On Tue, Nov 13, 2018 at 04:48:12PM -0800, Jeykumar Sankaran wrote: >> > > >> On 2018-11-13 12:52, Sean Paul wrote: >> > > >> > From: Sean Paul <seanpaul@chromium.org> >> > > >> > >> > > >> > Instead of assigning/clearing the crtc on vblank > enable/disable, we >> > > > can >> > > >> > just assign and clear the crtc on modeset. That allows us to > just >> > > > toggle >> > > >> > the encoder's vblank interrupts on vblank_enable. >> > > >> > >> > > >> > So why is this important? Previously the driver was using the >> > legacy >> > > >> > pointers to assign/clear the crtc. Legacy pointers are cleared >> > _after_ >> > > >> Which pointers are you referring here as legacy pointers? CRTC? >> > > > >> > > > encoder->crtc in this case. The loop in vblank_enable_no_lock > relies >> > on >> > > > enc->crtc == crtc >> > > > >> > > >> > disabling the hardware, so the legacy pointer was valid during >> > > >> > vblank_disable, but that's not something we should rely on. >> > > >> > >> > > >> > Instead of relying on the core ordering the legacy pointer >> > assignments >> > > >> > just so, we'll assign the crtc in dpu_crtc enable/disable. This > is >> > the >> > > >> > only place that mapping can change, so we're covered there. >> > > >> > >> > > >> > We're also taking advantage of drm_crtc_vblank_on/off. By using >> > this, >> > > > we >> > > >> > ensure that vblank_enable/disable can never be called while the >> > crtc >> > > > is >> > > >> > off (which means the assigned crtc will always be valid). As > such, >> > we >> > > >> >> > > >> What about the drm_vblank_enable/disable triggered by > drm_vblank_get >> > > > when >> > > >> crtc >> > > >> is disabled? What is the expected behavior there? the >> > vblank_requested >> > > >> variable removed by the next patch was introduced to cache the >> > > >> request. >> > > > >> > > > That case is covered by the modeset locks held when calling > disable(). >> > > > >> > > I am sure it will take care of drm_crtc_vblank_on/off triggered > within >> > > crtc_disable. >> > > My question was what was the expected behaviour when >> > > DRM_IOCTL_WAIT_VBLANK is >> > > called when crtc is disabled? the ioctl will try to call >> > > drm_vblank_get >> > > and I >> > > don't see the patch checking for crtc being enabled in the path. >> > >> > drm_vblank_off() >> > // .enable_vblank/.disable_vblank will never be called here >> > drm_vblank_on() >> > >> > So if you use drm_vblank_on/off judiciously you will never >> > have to deal with this particular problem. >> > >> Thanks ville for clarifying that. >> >> We can make sure to avoid that sequence if we have control over it. In > DPU, >> CRTC calls >> dpu_vblank_off in crtc_disable. After disabling, no one stopping >> userspace clients to call into DRM_IOCTL_WAIT_VBLANK on the crtc. This > ioctl >> calls enable_vblank when it sees the vblank is disabled [1]. > > > That's prevented by this bit in drm_vblank.c: > > /* > * Prevent subsequent drm_vblank_get() from re-enabling > * the vblank interrupt by bumping the refcount. > */ > if (!vblank->inmodeset) { > atomic_inc(&vblank->refcount); > vblank->inmodeset = 1; > } > > Basically it turns off the hw and then takes a reference such that > drm_vblank_get will never call drm_vblank_enable. > > Sean > That explains it! Thanks Sean! >> >> So far, the dpu_crtc_vblank was failing when it finds that the crtc is >> disabled. >> With this patch, the condition was removed, so I was wondering what is > the >> expected behavior with this patch. >> >> [1] > https://gitlab.freedesktop.org/seanpaul/dpu-staging/blob/for-next/drivers/ > gpu/drm/drm_vblank.c#L956 >> >> Thanks and Regards, >> Jeykumar S. >> > -- >> > Ville Syrjälä >> > Intel >> >> -- >> 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 4b7f98a6ab60..59e823281fdf 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -757,43 +757,6 @@ void dpu_crtc_commit_kickoff(struct drm_crtc *crtc) DPU_ATRACE_END("crtc_commit"); } -/** - * _dpu_crtc_vblank_enable_no_lock - update power resource and vblank request - * @dpu_crtc: Pointer to dpu crtc structure - * @enable: Whether to enable/disable vblanks - */ -static void _dpu_crtc_vblank_enable_no_lock( - struct dpu_crtc *dpu_crtc, bool enable) -{ - struct drm_crtc *crtc = &dpu_crtc->base; - struct drm_device *dev = crtc->dev; - struct drm_encoder *enc; - - if (enable) { - list_for_each_entry(enc, &dev->mode_config.encoder_list, head) { - if (enc->crtc != crtc) - continue; - - trace_dpu_crtc_vblank_enable(DRMID(&dpu_crtc->base), - DRMID(enc), enable, - dpu_crtc); - - dpu_encoder_assign_crtc(enc, crtc); - } - } else { - list_for_each_entry(enc, &dev->mode_config.encoder_list, head) { - if (enc->crtc != crtc) - continue; - - trace_dpu_crtc_vblank_enable(DRMID(&dpu_crtc->base), - DRMID(enc), enable, - dpu_crtc); - - dpu_encoder_assign_crtc(enc, NULL); - } - } -} - /** * dpu_crtc_duplicate_state - state duplicate hook * @crtc: Pointer to drm crtc structure @@ -847,6 +810,10 @@ static void dpu_crtc_disable(struct drm_crtc *crtc, /* Disable/save vblank irq handling */ drm_crtc_vblank_off(crtc); + drm_for_each_encoder_mask(encoder, crtc->dev, + old_crtc_state->encoder_mask) + dpu_encoder_assign_crtc(encoder, NULL); + mutex_lock(&dpu_crtc->crtc_lock); /* wait for frame_event_done completion */ @@ -856,9 +823,6 @@ static void dpu_crtc_disable(struct drm_crtc *crtc, atomic_read(&dpu_crtc->frame_pending)); trace_dpu_crtc_disable(DRMID(crtc), false, dpu_crtc); - if (dpu_crtc->enabled && dpu_crtc->vblank_requested) { - _dpu_crtc_vblank_enable_no_lock(dpu_crtc, false); - } dpu_crtc->enabled = false; if (atomic_read(&dpu_crtc->frame_pending)) { @@ -922,13 +886,13 @@ static void dpu_crtc_enable(struct drm_crtc *crtc, mutex_lock(&dpu_crtc->crtc_lock); trace_dpu_crtc_enable(DRMID(crtc), true, dpu_crtc); - if (!dpu_crtc->enabled && dpu_crtc->vblank_requested) { - _dpu_crtc_vblank_enable_no_lock(dpu_crtc, true); - } dpu_crtc->enabled = true; mutex_unlock(&dpu_crtc->crtc_lock); + drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask) + dpu_encoder_assign_crtc(encoder, crtc); + /* Enable/restore vblank irq handling */ drm_crtc_vblank_on(crtc); } @@ -1173,10 +1137,33 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, int dpu_crtc_vblank(struct drm_crtc *crtc, bool en) { struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc); + struct drm_encoder *enc; - mutex_lock(&dpu_crtc->crtc_lock); trace_dpu_crtc_vblank(DRMID(&dpu_crtc->base), en, dpu_crtc); - _dpu_crtc_vblank_enable_no_lock(dpu_crtc, en); + + /* + * Normally we would iterate through encoder_mask in crtc state to find + * attached encoders. In this case, we might be disabling vblank _after_ + * encoder_mask has been cleared. + * + * Instead, we "assign" a crtc to the encoder in enable and clear it in + * disable (which is also after encoder_mask is cleared). So instead of + * using encoder mask, we'll ask the encoder to toggle itself iff it's + * currently assigned to our crtc. + * + * Note also that this function cannot be called while crtc is disabled + * since we use drm_crtc_vblank_on/off. So we don't need to worry + * about the assigned crtcs being inconsistent with the current state + * (which means no need to worry about modeset locks). + */ + list_for_each_entry(enc, &crtc->dev->mode_config.encoder_list, head) { + trace_dpu_crtc_vblank_enable(DRMID(crtc), DRMID(enc), en, + dpu_crtc); + + dpu_encoder_toggle_vblank_for_crtc(enc, crtc, en); + } + + mutex_lock(&dpu_crtc->crtc_lock); dpu_crtc->vblank_requested = en; mutex_unlock(&dpu_crtc->crtc_lock); diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index fd6514f681ae..5914ae70572c 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -1267,22 +1267,29 @@ 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 = crtc ? true : false; - - if (!drm_enc) { - DPU_ERROR("invalid encoder\n"); - return; - } - trace_dpu_enc_vblank_cb(DRMID(drm_enc), enable); spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags); /* 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); +} + +void dpu_encoder_toggle_vblank_for_crtc(struct drm_encoder *drm_enc, + struct drm_crtc *crtc, bool enable) +{ + struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc); + unsigned long lock_flags; + int i; + + trace_dpu_enc_vblank_cb(DRMID(drm_enc), enable); + + spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags); + if (dpu_enc->crtc == crtc) { + spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags); + return; + } + spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags); for (i = 0; i < dpu_enc->num_phys_encs; i++) { struct dpu_encoder_phys *phys = dpu_enc->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 be1d80867834..6896ea2ab854 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h @@ -62,6 +62,16 @@ void dpu_encoder_get_hw_resources(struct drm_encoder *encoder, void dpu_encoder_assign_crtc(struct drm_encoder *encoder, struct drm_crtc *crtc); +/** + * dpu_encoder_toggle_vblank_for_crtc - Toggles vblank interrupts on or off if + * the encoder is assigned to the given crtc + * @encoder: encoder pointer + * @crtc: crtc pointer + * @enable: true if vblank should be enabled + */ +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.