Re: [PATCH 6/8] drm/msm: dpu: Separate crtc assignment from vblank enable

[Sean Paul <sean-p7yTbzM4H96eqtR555YLDQ@public.gmane.org>]

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

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno