[PATCH 6/8] drm/msm: dpu: Separate crtc assignment from vblank enable
Jeykumar Sankaran
jsanka at codeaurora.org
Wed Nov 14 19:43:51 UTC 2018
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 at 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 at 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
--
Jeykumar S
More information about the dri-devel
mailing list