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@xxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxx>
> ---
> 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
