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

Submitted by Sean Paul on Nov. 13, 2018, 8:52 p.m.

Details

Message ID 20181113205257.170707-6-sean@poorly.run
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Freedreno

Not browsing as part of any series.

Commit Message

Sean Paul Nov. 13, 2018, 8:52 p.m.
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_
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
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(-)

Patch hide | download patch | download mbox

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.

Comments

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