diff mbox series

[v2,22/24] drm/msm: dpu: Separate crtc assignment from vblank enable

Message ID 20181116184238.170034-23-sean@poorly.run (mailing list archive)
State New, archived
Headers show
Series drm/msm: Various dpu locking and legacy cleanups | expand

Commit Message

Sean Paul Nov. 16, 2018, 6:42 p.m. UTC 
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.

Changes in v2:
- Changed crtc check in toggle_vblank to != (Jeykumar)

Cc: Jeykumar Sankaran <jsanka@codeaurora.org>
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(-)

Comments

Jeykumar Sankaran Nov. 19, 2018, 8:13 p.m. UTC | #1 
On 2018-11-16 10:42, 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_
> 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.
> 
> Changes in v2:
> - Changed crtc check in toggle_vblank to != (Jeykumar)
> 
> Cc: Jeykumar Sankaran <jsanka@codeaurora.org>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>

Reviewed-by: Jeykumar Sankaran <jsanka@codeaurora.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 54bb777b2d0c..725e15178cdb 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -749,43 +749,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
> @@ -839,6 +802,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 */
> @@ -848,9 +815,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)) {
> @@ -908,13 +872,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);
>  }
> @@ -1159,10 +1123,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..90b77d98dde8 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.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 54bb777b2d0c..725e15178cdb 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -749,43 +749,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
@@ -839,6 +802,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 */
@@ -848,9 +815,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)) {
@@ -908,13 +872,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);
 }
@@ -1159,10 +1123,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..90b77d98dde8 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.