[PATCH 1/8] drm/msm: dpu: Move pm_runtime_(get|put) from vblank_enable

[PATCH 1/8] drm/msm: dpu: Move pm_runtime_(get|put) from vblank_enable

[Sean Paul]

From: Sean Paul <seanpaul@chromium.org>

There are 4 times that _dpu_crtc_vblank_enable_no_lock() is called:

1- crtc enable
2- crtc disable
3- crtc vblank enable
4- crtc vblank disable

When we enable or disable the crtc, we call drm_crtc_vblank_on and
drm_crtc_vblank_off respectively. That will gate vblank enables and
disables to only being called when the crtc is active. That means that
we can just enable/disable pm runtime in crtc enable/disable. This will
be beneficial in trying to eliminate blocking calls from the vblank call
chain.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 80de5289ada3..aa2f20c05dd7 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -777,8 +777,6 @@ static void _dpu_crtc_vblank_enable_no_lock(
 	struct drm_encoder *enc;
 
 	if (enable) {
-		pm_runtime_get_sync(dev->dev);
-
 		list_for_each_entry(enc, &dev->mode_config.encoder_list, head) {
 			if (enc->crtc != crtc)
 				continue;
@@ -801,8 +799,6 @@ static void _dpu_crtc_vblank_enable_no_lock(
 
 			dpu_encoder_register_vblank_callback(enc, NULL, NULL);
 		}
-
-		pm_runtime_put_sync(dev->dev);
 	}
 }
 
@@ -902,6 +898,8 @@ static void dpu_crtc_disable(struct drm_crtc *crtc)
 		crtc->state->event = NULL;
 		spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
 	}
+
+	pm_runtime_put_sync(crtc->dev->dev);
 }
 
 static void dpu_crtc_enable(struct drm_crtc *crtc,
@@ -917,6 +915,8 @@ static void dpu_crtc_enable(struct drm_crtc *crtc,
 	}
 	priv = crtc->dev->dev_private;
 
+	pm_runtime_get_sync(crtc->dev->dev);
+
 	DRM_DEBUG_KMS("crtc%d\n", crtc->base.id);
 	dpu_crtc = to_dpu_crtc(crtc);
 
-- 
Sean Paul, Software Engineer, Google / Chromium OS

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[PATCH 3/8] drm/msm: dpu: Remove vblank_callback from encoder

[Sean Paul]

From: Sean Paul <seanpaul@chromium.org>

The indirection of registering a callback and opaque pointer isn't real
useful when there's only one callsite. So instead of having the
vblank_cb registration, just give encoder a crtc and let it directly
call the vblank handler.

In a later patch, we'll make use of this further.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    |  8 +++----
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  6 +++++
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 25 +++++++++++----------
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 10 ++++-----
 4 files changed, 26 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index adda0aa0cbaa..38119b4d4a80 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -291,9 +291,8 @@ enum dpu_intf_mode dpu_crtc_get_intf_mode(struct drm_crtc *crtc)
 	return INTF_MODE_NONE;
 }
 
-static void dpu_crtc_vblank_cb(void *data)
+void dpu_crtc_vblank_callback(struct drm_crtc *crtc)
 {
-	struct drm_crtc *crtc = (struct drm_crtc *)data;
 	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
 
 	/* keep statistics on vblank callback - with auto reset via debugfs */
@@ -779,8 +778,7 @@ static void _dpu_crtc_vblank_enable_no_lock(
 						     DRMID(enc), enable,
 						     dpu_crtc);
 
-			dpu_encoder_register_vblank_callback(enc,
-					dpu_crtc_vblank_cb, (void *)crtc);
+			dpu_encoder_assign_crtc(enc, crtc);
 		}
 	} else {
 		list_for_each_entry(enc, &dev->mode_config.encoder_list, head) {
@@ -791,7 +789,7 @@ static void _dpu_crtc_vblank_enable_no_lock(
 						     DRMID(enc), enable,
 						     dpu_crtc);
 
-			dpu_encoder_register_vblank_callback(enc, NULL, NULL);
+			dpu_encoder_assign_crtc(enc, NULL);
 		}
 	}
 }
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
index 93d21a61a040..54595cc29be5 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
@@ -270,6 +270,12 @@ static inline int dpu_crtc_frame_pending(struct drm_crtc *crtc)
  */
 int dpu_crtc_vblank(struct drm_crtc *crtc, bool en);
 
+/**
+ * dpu_crtc_vblank_callback - called on vblank irq, issues completion events
+ * @crtc: Pointer to drm crtc object
+ */
+void dpu_crtc_vblank_callback(struct drm_crtc *crtc);
+
 /**
  * dpu_crtc_commit_kickoff - trigger kickoff of the commit for this crtc
  * @crtc: Pointer to drm crtc object
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index d89ac520f7e6..fd6514f681ae 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -142,9 +142,11 @@ enum dpu_enc_rc_states {
  * @intfs_swapped	Whether or not the phys_enc interfaces have been swapped
  *			for partial update right-only cases, such as pingpong
  *			split where virtual pingpong does not generate IRQs
- * @crtc_vblank_cb:	Callback into the upper layer / CRTC for
- *			notification of the VBLANK
- * @crtc_vblank_cb_data:	Data from upper layer for VBLANK notification
+ * @crtc:		Pointer to the currently assigned crtc. Normally you
+ *			would use crtc->state->encoder_mask to determine the
+ *			link between encoder/crtc. However in this case we need
+ *			to track crtc in the disable() hook which is called
+ *			_after_ encoder_mask is cleared.
  * @crtc_kickoff_cb:		Callback into CRTC that will flush & start
  *				all CTL paths
  * @crtc_kickoff_cb_data:	Opaque user data given to crtc_kickoff_cb
@@ -186,8 +188,7 @@ struct dpu_encoder_virt {
 
 	bool intfs_swapped;
 
-	void (*crtc_vblank_cb)(void *);
-	void *crtc_vblank_cb_data;
+	struct drm_crtc *crtc;
 
 	struct dentry *debugfs_root;
 	struct mutex enc_lock;
@@ -1241,8 +1242,8 @@ static void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc,
 	dpu_enc = to_dpu_encoder_virt(drm_enc);
 
 	spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags);
-	if (dpu_enc->crtc_vblank_cb)
-		dpu_enc->crtc_vblank_cb(dpu_enc->crtc_vblank_cb_data);
+	if (dpu_enc->crtc)
+		dpu_crtc_vblank_callback(dpu_enc->crtc);
 	spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags);
 
 	atomic_inc(&phy_enc->vsync_cnt);
@@ -1262,15 +1263,14 @@ static void dpu_encoder_underrun_callback(struct drm_encoder *drm_enc,
 	DPU_ATRACE_END("encoder_underrun_callback");
 }
 
-void dpu_encoder_register_vblank_callback(struct drm_encoder *drm_enc,
-		void (*vbl_cb)(void *), void *vbl_data)
+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 = vbl_cb ? true : false;
+	enable = crtc ? true : false;
 
 	if (!drm_enc) {
 		DPU_ERROR("invalid encoder\n");
@@ -1279,8 +1279,9 @@ void dpu_encoder_register_vblank_callback(struct drm_encoder *drm_enc,
 	trace_dpu_enc_vblank_cb(DRMID(drm_enc), enable);
 
 	spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags);
-	dpu_enc->crtc_vblank_cb = vbl_cb;
-	dpu_enc->crtc_vblank_cb_data = vbl_data;
+	/* 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);
 
 	for (i = 0; i < dpu_enc->num_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 aa4f135218fa..be1d80867834 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
@@ -55,14 +55,12 @@ void dpu_encoder_get_hw_resources(struct drm_encoder *encoder,
 				  struct dpu_encoder_hw_resources *hw_res);
 
 /**
- * dpu_encoder_register_vblank_callback - provide callback to encoder that
- *	will be called on the next vblank.
+ * dpu_encoder_assign_crtc - Link the encoder to the crtc it's assigned to
  * @encoder:	encoder pointer
- * @cb:		callback pointer, provide NULL to deregister and disable IRQs
- * @data:	user data provided to callback
+ * @crtc:	crtc pointer
  */
-void dpu_encoder_register_vblank_callback(struct drm_encoder *encoder,
-		void (*cb)(void *), void *data);
+void dpu_encoder_assign_crtc(struct drm_encoder *encoder,
+			     struct drm_crtc *crtc);
 
 /**
  * dpu_encoder_register_frame_event_callback - provide callback to encoder that
-- 
Sean Paul, Software Engineer, Google / Chromium OS

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/8] drm/msm: dpu: Remove vblank_callback from encoder

[Jeykumar Sankaran]

On 2018-11-13 12:52, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
> 
> The indirection of registering a callback and opaque pointer isn't real
> useful when there's only one callsite. So instead of having the
> vblank_cb registration, just give encoder a crtc and let it directly
> call the vblank handler.
> 
> In a later patch, we'll make use of this further.
> 
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    |  8 +++----
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  6 +++++
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 25 +++++++++++----------
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 10 ++++-----
>  4 files changed, 26 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index adda0aa0cbaa..38119b4d4a80 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -291,9 +291,8 @@ enum dpu_intf_mode dpu_crtc_get_intf_mode(struct
> drm_crtc *crtc)
>  	return INTF_MODE_NONE;
>  }
> 
> -static void dpu_crtc_vblank_cb(void *data)
> +void dpu_crtc_vblank_callback(struct drm_crtc *crtc)
>  {
> -	struct drm_crtc *crtc = (struct drm_crtc *)data;
>  	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);

Since we are calling into a locally stored CRTC and not tracking 
disable. Should
we check for crtc->enable before processing the callback?

I see vblank_on/off cant be called when CRTC is disabled with the rest
of the patches in the series. But this callback is triggered by IRQ's
context.

> 
>  	/* keep statistics on vblank callback - with auto reset via
> debugfs */
> @@ -779,8 +778,7 @@ static void _dpu_crtc_vblank_enable_no_lock(
>  						     DRMID(enc), enable,
>  						     dpu_crtc);
> 
> -			dpu_encoder_register_vblank_callback(enc,
> -					dpu_crtc_vblank_cb, (void *)crtc);
> +			dpu_encoder_assign_crtc(enc, crtc);
>  		}
>  	} else {
>  		list_for_each_entry(enc, &dev->mode_config.encoder_list,
> head) {
> @@ -791,7 +789,7 @@ static void _dpu_crtc_vblank_enable_no_lock(
>  						     DRMID(enc), enable,
>  						     dpu_crtc);
> 
> -			dpu_encoder_register_vblank_callback(enc, NULL,
> NULL);
> +			dpu_encoder_assign_crtc(enc, NULL);
>  		}
>  	}
>  }
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> index 93d21a61a040..54595cc29be5 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> @@ -270,6 +270,12 @@ static inline int dpu_crtc_frame_pending(struct
> drm_crtc *crtc)
>   */
>  int dpu_crtc_vblank(struct drm_crtc *crtc, bool en);
> 
> +/**
> + * dpu_crtc_vblank_callback - called on vblank irq, issues completion
> events
> + * @crtc: Pointer to drm crtc object
> + */
> +void dpu_crtc_vblank_callback(struct drm_crtc *crtc);
> +
>  /**
>   * dpu_crtc_commit_kickoff - trigger kickoff of the commit for this 
> crtc
>   * @crtc: Pointer to drm crtc object
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index d89ac520f7e6..fd6514f681ae 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -142,9 +142,11 @@ enum dpu_enc_rc_states {
>   * @intfs_swapped	Whether or not the phys_enc interfaces have been
> swapped
>   *			for partial update right-only cases, such as
> pingpong
>   *			split where virtual pingpong does not generate
> IRQs
> - * @crtc_vblank_cb:	Callback into the upper layer / CRTC for
> - *			notification of the VBLANK
> - * @crtc_vblank_cb_data:	Data from upper layer for VBLANK
> notification
> + * @crtc:		Pointer to the currently assigned crtc. Normally
> you
> + *			would use crtc->state->encoder_mask to determine
> the
> + *			link between encoder/crtc. However in this case we
> need
> + *			to track crtc in the disable() hook which is
> called
> + *			_after_ encoder_mask is cleared.
>   * @crtc_kickoff_cb:		Callback into CRTC that will flush & start
>   *				all CTL paths
>   * @crtc_kickoff_cb_data:	Opaque user data given to crtc_kickoff_cb
> @@ -186,8 +188,7 @@ struct dpu_encoder_virt {
> 
>  	bool intfs_swapped;
> 
> -	void (*crtc_vblank_cb)(void *);
> -	void *crtc_vblank_cb_data;
> +	struct drm_crtc *crtc;
> 
>  	struct dentry *debugfs_root;
>  	struct mutex enc_lock;
> @@ -1241,8 +1242,8 @@ static void dpu_encoder_vblank_callback(struct
> drm_encoder *drm_enc,
>  	dpu_enc = to_dpu_encoder_virt(drm_enc);
> 
>  	spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags);
> -	if (dpu_enc->crtc_vblank_cb)
> -		dpu_enc->crtc_vblank_cb(dpu_enc->crtc_vblank_cb_data);
> +	if (dpu_enc->crtc)
> +		dpu_crtc_vblank_callback(dpu_enc->crtc);
>  	spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags);
> 
>  	atomic_inc(&phy_enc->vsync_cnt);
> @@ -1262,15 +1263,14 @@ static void 
> dpu_encoder_underrun_callback(struct
> drm_encoder *drm_enc,
>  	DPU_ATRACE_END("encoder_underrun_callback");
>  }
> 
> -void dpu_encoder_register_vblank_callback(struct drm_encoder *drm_enc,
> -		void (*vbl_cb)(void *), void *vbl_data)
> +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 = vbl_cb ? true : false;
> +	enable = crtc ? true : false;
> 
>  	if (!drm_enc) {
>  		DPU_ERROR("invalid encoder\n");
> @@ -1279,8 +1279,9 @@ void dpu_encoder_register_vblank_callback(struct
> drm_encoder *drm_enc,
>  	trace_dpu_enc_vblank_cb(DRMID(drm_enc), enable);
> 
>  	spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags);
> -	dpu_enc->crtc_vblank_cb = vbl_cb;
> -	dpu_enc->crtc_vblank_cb_data = vbl_data;
> +	/* 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);
> 
>  	for (i = 0; i < dpu_enc->num_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 aa4f135218fa..be1d80867834 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> @@ -55,14 +55,12 @@ void dpu_encoder_get_hw_resources(struct 
> drm_encoder
> *encoder,
>  				  struct dpu_encoder_hw_resources
> *hw_res);
> 
>  /**
> - * dpu_encoder_register_vblank_callback - provide callback to encoder
> that
> - *	will be called on the next vblank.
> + * dpu_encoder_assign_crtc - Link the encoder to the crtc it's 
> assigned
> to
>   * @encoder:	encoder pointer
> - * @cb:		callback pointer, provide NULL to deregister and
> disable IRQs
> - * @data:	user data provided to callback
> + * @crtc:	crtc pointer
>   */
> -void dpu_encoder_register_vblank_callback(struct drm_encoder *encoder,
> -		void (*cb)(void *), void *data);
> +void dpu_encoder_assign_crtc(struct drm_encoder *encoder,
> +			     struct drm_crtc *crtc);
> 
>  /**
>   * dpu_encoder_register_frame_event_callback - provide callback to
> encoder that

-- 
Jeykumar S
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [PATCH 3/8] drm/msm: dpu: Remove vblank_callback from encoder

[Sean Paul]

On Tue, Nov 13, 2018 at 04:47:22PM -0800, Jeykumar Sankaran wrote:
> On 2018-11-13 12:52, Sean Paul wrote:
> > From: Sean Paul <seanpaul@chromium.org>
> > 
> > The indirection of registering a callback and opaque pointer isn't real
> > useful when there's only one callsite. So instead of having the
> > vblank_cb registration, just give encoder a crtc and let it directly
> > call the vblank handler.
> > 
> > In a later patch, we'll make use of this further.
> > 
> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > ---
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    |  8 +++----
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  6 +++++
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 25 +++++++++++----------
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 10 ++++-----
> >  4 files changed, 26 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > index adda0aa0cbaa..38119b4d4a80 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > @@ -291,9 +291,8 @@ enum dpu_intf_mode dpu_crtc_get_intf_mode(struct
> > drm_crtc *crtc)
> >  	return INTF_MODE_NONE;
> >  }
> > 
> > -static void dpu_crtc_vblank_cb(void *data)
> > +void dpu_crtc_vblank_callback(struct drm_crtc *crtc)
> >  {
> > -	struct drm_crtc *crtc = (struct drm_crtc *)data;
> >  	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
> 
> Since we are calling into a locally stored CRTC and not tracking disable.
> Should
> we check for crtc->enable before processing the callback?
> 
> I see vblank_on/off cant be called when CRTC is disabled with the rest
> of the patches in the series. But this callback is triggered by IRQ's
> context.

Hmm, yeah, I had assumed that we wouldn't have any interrupts after irq disable.
However it doesn't seem like that's the case.

That said, I don't think checking crtc->enable is quite what we want. In the
case of disable, the crtc is cleared from the encoder, so checking for
crtc != NULL would be better here. SGTY?

Now that I've dug into the vblank irq handling a bit in the encoder, it seems
like that could be moved to the crtc and a bunch of the encoder->crtc->encoder
bouncing around could be avoided. Off the top of your head, is there any reason
we couldn't move the vblank irq handling into crtc from encoder?

Sean

> 
> > 
> >  	/* keep statistics on vblank callback - with auto reset via
> > debugfs */
> > @@ -779,8 +778,7 @@ static void _dpu_crtc_vblank_enable_no_lock(
> >  						     DRMID(enc), enable,
> >  						     dpu_crtc);
> > 
> > -			dpu_encoder_register_vblank_callback(enc,
> > -					dpu_crtc_vblank_cb, (void *)crtc);
> > +			dpu_encoder_assign_crtc(enc, crtc);
> >  		}
> >  	} else {
> >  		list_for_each_entry(enc, &dev->mode_config.encoder_list,
> > head) {
> > @@ -791,7 +789,7 @@ static void _dpu_crtc_vblank_enable_no_lock(
> >  						     DRMID(enc), enable,
> >  						     dpu_crtc);
> > 
> > -			dpu_encoder_register_vblank_callback(enc, NULL,
> > NULL);
> > +			dpu_encoder_assign_crtc(enc, NULL);
> >  		}
> >  	}
> >  }
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> > index 93d21a61a040..54595cc29be5 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> > @@ -270,6 +270,12 @@ static inline int dpu_crtc_frame_pending(struct
> > drm_crtc *crtc)
> >   */
> >  int dpu_crtc_vblank(struct drm_crtc *crtc, bool en);
> > 
> > +/**
> > + * dpu_crtc_vblank_callback - called on vblank irq, issues completion
> > events
> > + * @crtc: Pointer to drm crtc object
> > + */
> > +void dpu_crtc_vblank_callback(struct drm_crtc *crtc);
> > +
> >  /**
> >   * dpu_crtc_commit_kickoff - trigger kickoff of the commit for this
> > crtc
> >   * @crtc: Pointer to drm crtc object
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > index d89ac520f7e6..fd6514f681ae 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > @@ -142,9 +142,11 @@ enum dpu_enc_rc_states {
> >   * @intfs_swapped	Whether or not the phys_enc interfaces have been
> > swapped
> >   *			for partial update right-only cases, such as
> > pingpong
> >   *			split where virtual pingpong does not generate
> > IRQs
> > - * @crtc_vblank_cb:	Callback into the upper layer / CRTC for
> > - *			notification of the VBLANK
> > - * @crtc_vblank_cb_data:	Data from upper layer for VBLANK
> > notification
> > + * @crtc:		Pointer to the currently assigned crtc. Normally
> > you
> > + *			would use crtc->state->encoder_mask to determine
> > the
> > + *			link between encoder/crtc. However in this case we
> > need
> > + *			to track crtc in the disable() hook which is
> > called
> > + *			_after_ encoder_mask is cleared.
> >   * @crtc_kickoff_cb:		Callback into CRTC that will flush & start
> >   *				all CTL paths
> >   * @crtc_kickoff_cb_data:	Opaque user data given to crtc_kickoff_cb
> > @@ -186,8 +188,7 @@ struct dpu_encoder_virt {
> > 
> >  	bool intfs_swapped;
> > 
> > -	void (*crtc_vblank_cb)(void *);
> > -	void *crtc_vblank_cb_data;
> > +	struct drm_crtc *crtc;
> > 
> >  	struct dentry *debugfs_root;
> >  	struct mutex enc_lock;
> > @@ -1241,8 +1242,8 @@ static void dpu_encoder_vblank_callback(struct
> > drm_encoder *drm_enc,
> >  	dpu_enc = to_dpu_encoder_virt(drm_enc);
> > 
> >  	spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags);
> > -	if (dpu_enc->crtc_vblank_cb)
> > -		dpu_enc->crtc_vblank_cb(dpu_enc->crtc_vblank_cb_data);
> > +	if (dpu_enc->crtc)
> > +		dpu_crtc_vblank_callback(dpu_enc->crtc);
> >  	spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags);
> > 
> >  	atomic_inc(&phy_enc->vsync_cnt);
> > @@ -1262,15 +1263,14 @@ static void dpu_encoder_underrun_callback(struct
> > drm_encoder *drm_enc,
> >  	DPU_ATRACE_END("encoder_underrun_callback");
> >  }
> > 
> > -void dpu_encoder_register_vblank_callback(struct drm_encoder *drm_enc,
> > -		void (*vbl_cb)(void *), void *vbl_data)
> > +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 = vbl_cb ? true : false;
> > +	enable = crtc ? true : false;
> > 
> >  	if (!drm_enc) {
> >  		DPU_ERROR("invalid encoder\n");
> > @@ -1279,8 +1279,9 @@ void dpu_encoder_register_vblank_callback(struct
> > drm_encoder *drm_enc,
> >  	trace_dpu_enc_vblank_cb(DRMID(drm_enc), enable);
> > 
> >  	spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags);
> > -	dpu_enc->crtc_vblank_cb = vbl_cb;
> > -	dpu_enc->crtc_vblank_cb_data = vbl_data;
> > +	/* 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);
> > 
> >  	for (i = 0; i < dpu_enc->num_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 aa4f135218fa..be1d80867834 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > @@ -55,14 +55,12 @@ void dpu_encoder_get_hw_resources(struct drm_encoder
> > *encoder,
> >  				  struct dpu_encoder_hw_resources
> > *hw_res);
> > 
> >  /**
> > - * dpu_encoder_register_vblank_callback - provide callback to encoder
> > that
> > - *	will be called on the next vblank.
> > + * dpu_encoder_assign_crtc - Link the encoder to the crtc it's assigned
> > to
> >   * @encoder:	encoder pointer
> > - * @cb:		callback pointer, provide NULL to deregister and
> > disable IRQs
> > - * @data:	user data provided to callback
> > + * @crtc:	crtc pointer
> >   */
> > -void dpu_encoder_register_vblank_callback(struct drm_encoder *encoder,
> > -		void (*cb)(void *), void *data);
> > +void dpu_encoder_assign_crtc(struct drm_encoder *encoder,
> > +			     struct drm_crtc *crtc);
> > 
> >  /**
> >   * dpu_encoder_register_frame_event_callback - provide callback to
> > encoder that
> 
> -- 
> Jeykumar S

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

Re: [PATCH 3/8] drm/msm: dpu: Remove vblank_callback from encoder

[Jeykumar Sankaran]

On 2018-11-14 07:13, Sean Paul wrote:
> On Tue, Nov 13, 2018 at 04:47:22PM -0800, Jeykumar Sankaran wrote:
>> On 2018-11-13 12:52, Sean Paul wrote:
>> > From: Sean Paul <seanpaul@chromium.org>
>> >
>> > The indirection of registering a callback and opaque pointer isn't
> real
>> > useful when there's only one callsite. So instead of having the
>> > vblank_cb registration, just give encoder a crtc and let it directly
>> > call the vblank handler.
>> >
>> > In a later patch, we'll make use of this further.
>> >
>> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
>> > ---
>> >  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    |  8 +++----
>> >  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  6 +++++
>> >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 25
> +++++++++++----------
>> >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 10 ++++-----
>> >  4 files changed, 26 insertions(+), 23 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> > index adda0aa0cbaa..38119b4d4a80 100644
>> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> > @@ -291,9 +291,8 @@ enum dpu_intf_mode dpu_crtc_get_intf_mode(struct
>> > drm_crtc *crtc)
>> >  	return INTF_MODE_NONE;
>> >  }
>> >
>> > -static void dpu_crtc_vblank_cb(void *data)
>> > +void dpu_crtc_vblank_callback(struct drm_crtc *crtc)
>> >  {
>> > -	struct drm_crtc *crtc = (struct drm_crtc *)data;
>> >  	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
>> 
>> Since we are calling into a locally stored CRTC and not tracking
> disable.
>> Should
>> we check for crtc->enable before processing the callback?
>> 
>> I see vblank_on/off cant be called when CRTC is disabled with the rest
>> of the patches in the series. But this callback is triggered by IRQ's
>> context.
> 
> Hmm, yeah, I had assumed that we wouldn't have any interrupts after irq
> disable.
> However it doesn't seem like that's the case.
> 
> That said, I don't think checking crtc->enable is quite what we want. 
> In
> the
> case of disable, the crtc is cleared from the encoder, so checking for
> crtc != NULL would be better here. SGTY?
That would still cause a race as crtc assignment will be protected by 
modeset
locks and crtc != NULL check isn't.
> 
> Now that I've dug into the vblank irq handling a bit in the encoder, it
> seems
> like that could be moved to the crtc and a bunch of the
> encoder->crtc->encoder
> bouncing around could be avoided. Off the top of your head, is there 
> any
> reason
> we couldn't move the vblank irq handling into crtc from encoder?
vblank irq handlers are only needed for video mode. As with all the 
other
IRQ's, the handlers are implemented in phys_encoder level and the events
are forwarded as and when needed.

BTW, the vblank irq flows from phys_enc->virtual_enc->crtc.

Thanks,
Jeykumar S.

> 
> Sean
> 
>> 
>> >
>> >  	/* keep statistics on vblank callback - with auto reset via
>> > debugfs */
>> > @@ -779,8 +778,7 @@ static void _dpu_crtc_vblank_enable_no_lock(
>> >  						     DRMID(enc), enable,
>> >  						     dpu_crtc);
>> >
>> > -			dpu_encoder_register_vblank_callback(enc,
>> > -					dpu_crtc_vblank_cb, (void *)crtc);
>> > +			dpu_encoder_assign_crtc(enc, crtc);
>> >  		}
>> >  	} else {
>> >  		list_for_each_entry(enc, &dev->mode_config.encoder_list,
>> > head) {
>> > @@ -791,7 +789,7 @@ static void _dpu_crtc_vblank_enable_no_lock(
>> >  						     DRMID(enc), enable,
>> >  						     dpu_crtc);
>> >
>> > -			dpu_encoder_register_vblank_callback(enc, NULL,
>> > NULL);
>> > +			dpu_encoder_assign_crtc(enc, NULL);
>> >  		}
>> >  	}
>> >  }
>> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> > index 93d21a61a040..54595cc29be5 100644
>> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> > @@ -270,6 +270,12 @@ static inline int dpu_crtc_frame_pending(struct
>> > drm_crtc *crtc)
>> >   */
>> >  int dpu_crtc_vblank(struct drm_crtc *crtc, bool en);
>> >
>> > +/**
>> > + * dpu_crtc_vblank_callback - called on vblank irq, issues completion
>> > events
>> > + * @crtc: Pointer to drm crtc object
>> > + */
>> > +void dpu_crtc_vblank_callback(struct drm_crtc *crtc);
>> > +
>> >  /**
>> >   * dpu_crtc_commit_kickoff - trigger kickoff of the commit for this
>> > crtc
>> >   * @crtc: Pointer to drm crtc object
>> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> > index d89ac520f7e6..fd6514f681ae 100644
>> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> > @@ -142,9 +142,11 @@ enum dpu_enc_rc_states {
>> >   * @intfs_swapped	Whether or not the phys_enc interfaces have been
>> > swapped
>> >   *			for partial update right-only cases, such as
>> > pingpong
>> >   *			split where virtual pingpong does not generate
>> > IRQs
>> > - * @crtc_vblank_cb:	Callback into the upper layer / CRTC for
>> > - *			notification of the VBLANK
>> > - * @crtc_vblank_cb_data:	Data from upper layer for VBLANK
>> > notification
>> > + * @crtc:		Pointer to the currently assigned crtc. Normally
>> > you
>> > + *			would use crtc->state->encoder_mask to determine
>> > the
>> > + *			link between encoder/crtc. However in this case we
>> > need
>> > + *			to track crtc in the disable() hook which is
>> > called
>> > + *			_after_ encoder_mask is cleared.
>> >   * @crtc_kickoff_cb:		Callback into CRTC that will flush
> & start
>> >   *				all CTL paths
>> >   * @crtc_kickoff_cb_data:	Opaque user data given to crtc_kickoff_cb
>> > @@ -186,8 +188,7 @@ struct dpu_encoder_virt {
>> >
>> >  	bool intfs_swapped;
>> >
>> > -	void (*crtc_vblank_cb)(void *);
>> > -	void *crtc_vblank_cb_data;
>> > +	struct drm_crtc *crtc;
>> >
>> >  	struct dentry *debugfs_root;
>> >  	struct mutex enc_lock;
>> > @@ -1241,8 +1242,8 @@ static void dpu_encoder_vblank_callback(struct
>> > drm_encoder *drm_enc,
>> >  	dpu_enc = to_dpu_encoder_virt(drm_enc);
>> >
>> >  	spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags);
>> > -	if (dpu_enc->crtc_vblank_cb)
>> > -		dpu_enc->crtc_vblank_cb(dpu_enc->crtc_vblank_cb_data);
>> > +	if (dpu_enc->crtc)
>> > +		dpu_crtc_vblank_callback(dpu_enc->crtc);
>> >  	spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags);
>> >
>> >  	atomic_inc(&phy_enc->vsync_cnt);
>> > @@ -1262,15 +1263,14 @@ static void
> dpu_encoder_underrun_callback(struct
>> > drm_encoder *drm_enc,
>> >  	DPU_ATRACE_END("encoder_underrun_callback");
>> >  }
>> >
>> > -void dpu_encoder_register_vblank_callback(struct drm_encoder
> *drm_enc,
>> > -		void (*vbl_cb)(void *), void *vbl_data)
>> > +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 = vbl_cb ? true : false;
>> > +	enable = crtc ? true : false;
>> >
>> >  	if (!drm_enc) {
>> >  		DPU_ERROR("invalid encoder\n");
>> > @@ -1279,8 +1279,9 @@ void dpu_encoder_register_vblank_callback(struct
>> > drm_encoder *drm_enc,
>> >  	trace_dpu_enc_vblank_cb(DRMID(drm_enc), enable);
>> >
>> >  	spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags);
>> > -	dpu_enc->crtc_vblank_cb = vbl_cb;
>> > -	dpu_enc->crtc_vblank_cb_data = vbl_data;
>> > +	/* 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);
>> >
>> >  	for (i = 0; i < dpu_enc->num_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 aa4f135218fa..be1d80867834 100644
>> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> > @@ -55,14 +55,12 @@ void dpu_encoder_get_hw_resources(struct
> drm_encoder
>> > *encoder,
>> >  				  struct dpu_encoder_hw_resources
>> > *hw_res);
>> >
>> >  /**
>> > - * dpu_encoder_register_vblank_callback - provide callback to encoder
>> > that
>> > - *	will be called on the next vblank.
>> > + * dpu_encoder_assign_crtc - Link the encoder to the crtc it's
> assigned
>> > to
>> >   * @encoder:	encoder pointer
>> > - * @cb:		callback pointer, provide NULL to deregister and
>> > disable IRQs
>> > - * @data:	user data provided to callback
>> > + * @crtc:	crtc pointer
>> >   */
>> > -void dpu_encoder_register_vblank_callback(struct drm_encoder
> *encoder,
>> > -		void (*cb)(void *), void *data);
>> > +void dpu_encoder_assign_crtc(struct drm_encoder *encoder,
>> > +			     struct drm_crtc *crtc);
>> >
>> >  /**
>> >   * dpu_encoder_register_frame_event_callback - provide callback to
>> > encoder that
>> 
>> --
>> Jeykumar S

-- 
Jeykumar S
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [PATCH 3/8] drm/msm: dpu: Remove vblank_callback from encoder

[Sean Paul]

On Wed, Nov 14, 2018 at 11:33:53AM -0800, Jeykumar Sankaran wrote:
> On 2018-11-14 07:13, Sean Paul wrote:
> > On Tue, Nov 13, 2018 at 04:47:22PM -0800, Jeykumar Sankaran wrote:
> > > On 2018-11-13 12:52, Sean Paul wrote:
> > > > From: Sean Paul <seanpaul@chromium.org>
> > > >
> > > > The indirection of registering a callback and opaque pointer isn't
> > real
> > > > useful when there's only one callsite. So instead of having the
> > > > vblank_cb registration, just give encoder a crtc and let it directly
> > > > call the vblank handler.
> > > >
> > > > In a later patch, we'll make use of this further.
> > > >
> > > > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > > > ---
> > > >  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    |  8 +++----
> > > >  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  6 +++++
> > > >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 25
> > +++++++++++----------
> > > >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 10 ++++-----
> > > >  4 files changed, 26 insertions(+), 23 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > > > index adda0aa0cbaa..38119b4d4a80 100644
> > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > > > @@ -291,9 +291,8 @@ enum dpu_intf_mode dpu_crtc_get_intf_mode(struct
> > > > drm_crtc *crtc)
> > > >  	return INTF_MODE_NONE;
> > > >  }
> > > >
> > > > -static void dpu_crtc_vblank_cb(void *data)
> > > > +void dpu_crtc_vblank_callback(struct drm_crtc *crtc)
> > > >  {
> > > > -	struct drm_crtc *crtc = (struct drm_crtc *)data;
> > > >  	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
> > > 
> > > Since we are calling into a locally stored CRTC and not tracking
> > disable.
> > > Should
> > > we check for crtc->enable before processing the callback?
> > > 
> > > I see vblank_on/off cant be called when CRTC is disabled with the rest
> > > of the patches in the series. But this callback is triggered by IRQ's
> > > context.
> > 
> > Hmm, yeah, I had assumed that we wouldn't have any interrupts after irq
> > disable.
> > However it doesn't seem like that's the case.
> > 
> > That said, I don't think checking crtc->enable is quite what we want. In
> > the
> > case of disable, the crtc is cleared from the encoder, so checking for
> > crtc != NULL would be better here. SGTY?
> That would still cause a race as crtc assignment will be protected by
> modeset
> locks and crtc != NULL check isn't.

Right, we'd need to wrap the callback in the encoder spinlock.

> > 
> > Now that I've dug into the vblank irq handling a bit in the encoder, it
> > seems
> > like that could be moved to the crtc and a bunch of the
> > encoder->crtc->encoder
> > bouncing around could be avoided. Off the top of your head, is there any
> > reason
> > we couldn't move the vblank irq handling into crtc from encoder?
> vblank irq handlers are only needed for video mode. As with all the other
> IRQ's, the handlers are implemented in phys_encoder level and the events
> are forwarded as and when needed.
> 

I took a look at how that might work, and it seems possible but probably not
worthwhile.

Sean

> BTW, the vblank irq flows from phys_enc->virtual_enc->crtc.
> 
> Thanks,
> Jeykumar S.
> 
> > 
> > Sean
> > 
> > > 
> > > >
> > > >  	/* keep statistics on vblank callback - with auto reset via
> > > > debugfs */
> > > > @@ -779,8 +778,7 @@ static void _dpu_crtc_vblank_enable_no_lock(
> > > >  						     DRMID(enc), enable,
> > > >  						     dpu_crtc);
> > > >
> > > > -			dpu_encoder_register_vblank_callback(enc,
> > > > -					dpu_crtc_vblank_cb, (void *)crtc);
> > > > +			dpu_encoder_assign_crtc(enc, crtc);
> > > >  		}
> > > >  	} else {
> > > >  		list_for_each_entry(enc, &dev->mode_config.encoder_list,
> > > > head) {
> > > > @@ -791,7 +789,7 @@ static void _dpu_crtc_vblank_enable_no_lock(
> > > >  						     DRMID(enc), enable,
> > > >  						     dpu_crtc);
> > > >
> > > > -			dpu_encoder_register_vblank_callback(enc, NULL,
> > > > NULL);
> > > > +			dpu_encoder_assign_crtc(enc, NULL);
> > > >  		}
> > > >  	}
> > > >  }
> > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> > > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> > > > index 93d21a61a040..54595cc29be5 100644
> > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> > > > @@ -270,6 +270,12 @@ static inline int dpu_crtc_frame_pending(struct
> > > > drm_crtc *crtc)
> > > >   */
> > > >  int dpu_crtc_vblank(struct drm_crtc *crtc, bool en);
> > > >
> > > > +/**
> > > > + * dpu_crtc_vblank_callback - called on vblank irq, issues completion
> > > > events
> > > > + * @crtc: Pointer to drm crtc object
> > > > + */
> > > > +void dpu_crtc_vblank_callback(struct drm_crtc *crtc);
> > > > +
> > > >  /**
> > > >   * dpu_crtc_commit_kickoff - trigger kickoff of the commit for this
> > > > crtc
> > > >   * @crtc: Pointer to drm crtc object
> > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > > index d89ac520f7e6..fd6514f681ae 100644
> > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > > @@ -142,9 +142,11 @@ enum dpu_enc_rc_states {
> > > >   * @intfs_swapped	Whether or not the phys_enc interfaces have been
> > > > swapped
> > > >   *			for partial update right-only cases, such as
> > > > pingpong
> > > >   *			split where virtual pingpong does not generate
> > > > IRQs
> > > > - * @crtc_vblank_cb:	Callback into the upper layer / CRTC for
> > > > - *			notification of the VBLANK
> > > > - * @crtc_vblank_cb_data:	Data from upper layer for VBLANK
> > > > notification
> > > > + * @crtc:		Pointer to the currently assigned crtc. Normally
> > > > you
> > > > + *			would use crtc->state->encoder_mask to determine
> > > > the
> > > > + *			link between encoder/crtc. However in this case we
> > > > need
> > > > + *			to track crtc in the disable() hook which is
> > > > called
> > > > + *			_after_ encoder_mask is cleared.
> > > >   * @crtc_kickoff_cb:		Callback into CRTC that will flush
> > & start
> > > >   *				all CTL paths
> > > >   * @crtc_kickoff_cb_data:	Opaque user data given to crtc_kickoff_cb
> > > > @@ -186,8 +188,7 @@ struct dpu_encoder_virt {
> > > >
> > > >  	bool intfs_swapped;
> > > >
> > > > -	void (*crtc_vblank_cb)(void *);
> > > > -	void *crtc_vblank_cb_data;
> > > > +	struct drm_crtc *crtc;
> > > >
> > > >  	struct dentry *debugfs_root;
> > > >  	struct mutex enc_lock;
> > > > @@ -1241,8 +1242,8 @@ static void dpu_encoder_vblank_callback(struct
> > > > drm_encoder *drm_enc,
> > > >  	dpu_enc = to_dpu_encoder_virt(drm_enc);
> > > >
> > > >  	spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags);
> > > > -	if (dpu_enc->crtc_vblank_cb)
> > > > -		dpu_enc->crtc_vblank_cb(dpu_enc->crtc_vblank_cb_data);
> > > > +	if (dpu_enc->crtc)
> > > > +		dpu_crtc_vblank_callback(dpu_enc->crtc);
> > > >  	spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags);
> > > >
> > > >  	atomic_inc(&phy_enc->vsync_cnt);
> > > > @@ -1262,15 +1263,14 @@ static void
> > dpu_encoder_underrun_callback(struct
> > > > drm_encoder *drm_enc,
> > > >  	DPU_ATRACE_END("encoder_underrun_callback");
> > > >  }
> > > >
> > > > -void dpu_encoder_register_vblank_callback(struct drm_encoder
> > *drm_enc,
> > > > -		void (*vbl_cb)(void *), void *vbl_data)
> > > > +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 = vbl_cb ? true : false;
> > > > +	enable = crtc ? true : false;
> > > >
> > > >  	if (!drm_enc) {
> > > >  		DPU_ERROR("invalid encoder\n");
> > > > @@ -1279,8 +1279,9 @@ void dpu_encoder_register_vblank_callback(struct
> > > > drm_encoder *drm_enc,
> > > >  	trace_dpu_enc_vblank_cb(DRMID(drm_enc), enable);
> > > >
> > > >  	spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags);
> > > > -	dpu_enc->crtc_vblank_cb = vbl_cb;
> > > > -	dpu_enc->crtc_vblank_cb_data = vbl_data;
> > > > +	/* 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);
> > > >
> > > >  	for (i = 0; i < dpu_enc->num_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 aa4f135218fa..be1d80867834 100644
> > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > > > @@ -55,14 +55,12 @@ void dpu_encoder_get_hw_resources(struct
> > drm_encoder
> > > > *encoder,
> > > >  				  struct dpu_encoder_hw_resources
> > > > *hw_res);
> > > >
> > > >  /**
> > > > - * dpu_encoder_register_vblank_callback - provide callback to encoder
> > > > that
> > > > - *	will be called on the next vblank.
> > > > + * dpu_encoder_assign_crtc - Link the encoder to the crtc it's
> > assigned
> > > > to
> > > >   * @encoder:	encoder pointer
> > > > - * @cb:		callback pointer, provide NULL to deregister and
> > > > disable IRQs
> > > > - * @data:	user data provided to callback
> > > > + * @crtc:	crtc pointer
> > > >   */
> > > > -void dpu_encoder_register_vblank_callback(struct drm_encoder
> > *encoder,
> > > > -		void (*cb)(void *), void *data);
> > > > +void dpu_encoder_assign_crtc(struct drm_encoder *encoder,
> > > > +			     struct drm_crtc *crtc);
> > > >
> > > >  /**
> > > >   * dpu_encoder_register_frame_event_callback - provide callback to
> > > > encoder that
> > > 
> > > --
> > > Jeykumar S
> 
> -- 
> Jeykumar S

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

Re: [PATCH 3/8] drm/msm: dpu: Remove vblank_callback from encoder

[Sean Paul]

On Wed, Nov 14, 2018 at 02:43:51PM -0500, Sean Paul wrote:
> On Wed, Nov 14, 2018 at 11:33:53AM -0800, Jeykumar Sankaran wrote:
> > On 2018-11-14 07:13, Sean Paul wrote:
> > > On Tue, Nov 13, 2018 at 04:47:22PM -0800, Jeykumar Sankaran wrote:
> > > > On 2018-11-13 12:52, Sean Paul wrote:
> > > > > From: Sean Paul <seanpaul@chromium.org>
> > > > >
> > > > > The indirection of registering a callback and opaque pointer isn't
> > > real
> > > > > useful when there's only one callsite. So instead of having the
> > > > > vblank_cb registration, just give encoder a crtc and let it directly
> > > > > call the vblank handler.
> > > > >
> > > > > In a later patch, we'll make use of this further.
> > > > >
> > > > > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > > > > ---
> > > > >  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    |  8 +++----
> > > > >  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  6 +++++
> > > > >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 25
> > > +++++++++++----------
> > > > >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 10 ++++-----
> > > > >  4 files changed, 26 insertions(+), 23 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > > > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > > > > index adda0aa0cbaa..38119b4d4a80 100644
> > > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > > > > @@ -291,9 +291,8 @@ enum dpu_intf_mode dpu_crtc_get_intf_mode(struct
> > > > > drm_crtc *crtc)
> > > > >  	return INTF_MODE_NONE;
> > > > >  }
> > > > >
> > > > > -static void dpu_crtc_vblank_cb(void *data)
> > > > > +void dpu_crtc_vblank_callback(struct drm_crtc *crtc)
> > > > >  {
> > > > > -	struct drm_crtc *crtc = (struct drm_crtc *)data;
> > > > >  	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
> > > > 
> > > > Since we are calling into a locally stored CRTC and not tracking
> > > disable.
> > > > Should
> > > > we check for crtc->enable before processing the callback?
> > > > 
> > > > I see vblank_on/off cant be called when CRTC is disabled with the rest
> > > > of the patches in the series. But this callback is triggered by IRQ's
> > > > context.
> > > 
> > > Hmm, yeah, I had assumed that we wouldn't have any interrupts after irq
> > > disable.
> > > However it doesn't seem like that's the case.
> > > 
> > > That said, I don't think checking crtc->enable is quite what we want. In
> > > the
> > > case of disable, the crtc is cleared from the encoder, so checking for
> > > crtc != NULL would be better here. SGTY?
> > That would still cause a race as crtc assignment will be protected by
> > modeset
> > locks and crtc != NULL check isn't.
> 
> Right, we'd need to wrap the callback in the encoder spinlock.

Just went to do this, and it's already wrapped in the encoder spinlock. So
AFAICT, no further changes needed.

Sean

> 
> > > 
> > > Now that I've dug into the vblank irq handling a bit in the encoder, it
> > > seems
> > > like that could be moved to the crtc and a bunch of the
> > > encoder->crtc->encoder
> > > bouncing around could be avoided. Off the top of your head, is there any
> > > reason
> > > we couldn't move the vblank irq handling into crtc from encoder?
> > vblank irq handlers are only needed for video mode. As with all the other
> > IRQ's, the handlers are implemented in phys_encoder level and the events
> > are forwarded as and when needed.
> > 
> 
> I took a look at how that might work, and it seems possible but probably not
> worthwhile.
> 
> Sean
> 
> > BTW, the vblank irq flows from phys_enc->virtual_enc->crtc.
> > 
> > Thanks,
> > Jeykumar S.
> > 
> > > 
> > > Sean
> > > 
> > > > 
> > > > >
> > > > >  	/* keep statistics on vblank callback - with auto reset via
> > > > > debugfs */
> > > > > @@ -779,8 +778,7 @@ static void _dpu_crtc_vblank_enable_no_lock(
> > > > >  						     DRMID(enc), enable,
> > > > >  						     dpu_crtc);
> > > > >
> > > > > -			dpu_encoder_register_vblank_callback(enc,
> > > > > -					dpu_crtc_vblank_cb, (void *)crtc);
> > > > > +			dpu_encoder_assign_crtc(enc, crtc);
> > > > >  		}
> > > > >  	} else {
> > > > >  		list_for_each_entry(enc, &dev->mode_config.encoder_list,
> > > > > head) {
> > > > > @@ -791,7 +789,7 @@ static void _dpu_crtc_vblank_enable_no_lock(
> > > > >  						     DRMID(enc), enable,
> > > > >  						     dpu_crtc);
> > > > >
> > > > > -			dpu_encoder_register_vblank_callback(enc, NULL,
> > > > > NULL);
> > > > > +			dpu_encoder_assign_crtc(enc, NULL);
> > > > >  		}
> > > > >  	}
> > > > >  }
> > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> > > > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> > > > > index 93d21a61a040..54595cc29be5 100644
> > > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> > > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> > > > > @@ -270,6 +270,12 @@ static inline int dpu_crtc_frame_pending(struct
> > > > > drm_crtc *crtc)
> > > > >   */
> > > > >  int dpu_crtc_vblank(struct drm_crtc *crtc, bool en);
> > > > >
> > > > > +/**
> > > > > + * dpu_crtc_vblank_callback - called on vblank irq, issues completion
> > > > > events
> > > > > + * @crtc: Pointer to drm crtc object
> > > > > + */
> > > > > +void dpu_crtc_vblank_callback(struct drm_crtc *crtc);
> > > > > +
> > > > >  /**
> > > > >   * dpu_crtc_commit_kickoff - trigger kickoff of the commit for this
> > > > > crtc
> > > > >   * @crtc: Pointer to drm crtc object
> > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > > > index d89ac520f7e6..fd6514f681ae 100644
> > > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > > > @@ -142,9 +142,11 @@ enum dpu_enc_rc_states {
> > > > >   * @intfs_swapped	Whether or not the phys_enc interfaces have been
> > > > > swapped
> > > > >   *			for partial update right-only cases, such as
> > > > > pingpong
> > > > >   *			split where virtual pingpong does not generate
> > > > > IRQs
> > > > > - * @crtc_vblank_cb:	Callback into the upper layer / CRTC for
> > > > > - *			notification of the VBLANK
> > > > > - * @crtc_vblank_cb_data:	Data from upper layer for VBLANK
> > > > > notification
> > > > > + * @crtc:		Pointer to the currently assigned crtc. Normally
> > > > > you
> > > > > + *			would use crtc->state->encoder_mask to determine
> > > > > the
> > > > > + *			link between encoder/crtc. However in this case we
> > > > > need
> > > > > + *			to track crtc in the disable() hook which is
> > > > > called
> > > > > + *			_after_ encoder_mask is cleared.
> > > > >   * @crtc_kickoff_cb:		Callback into CRTC that will flush
> > > & start
> > > > >   *				all CTL paths
> > > > >   * @crtc_kickoff_cb_data:	Opaque user data given to crtc_kickoff_cb
> > > > > @@ -186,8 +188,7 @@ struct dpu_encoder_virt {
> > > > >
> > > > >  	bool intfs_swapped;
> > > > >
> > > > > -	void (*crtc_vblank_cb)(void *);
> > > > > -	void *crtc_vblank_cb_data;
> > > > > +	struct drm_crtc *crtc;
> > > > >
> > > > >  	struct dentry *debugfs_root;
> > > > >  	struct mutex enc_lock;
> > > > > @@ -1241,8 +1242,8 @@ static void dpu_encoder_vblank_callback(struct
> > > > > drm_encoder *drm_enc,
> > > > >  	dpu_enc = to_dpu_encoder_virt(drm_enc);
> > > > >
> > > > >  	spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags);
> > > > > -	if (dpu_enc->crtc_vblank_cb)
> > > > > -		dpu_enc->crtc_vblank_cb(dpu_enc->crtc_vblank_cb_data);
> > > > > +	if (dpu_enc->crtc)
> > > > > +		dpu_crtc_vblank_callback(dpu_enc->crtc);
> > > > >  	spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags);
> > > > >
> > > > >  	atomic_inc(&phy_enc->vsync_cnt);
> > > > > @@ -1262,15 +1263,14 @@ static void
> > > dpu_encoder_underrun_callback(struct
> > > > > drm_encoder *drm_enc,
> > > > >  	DPU_ATRACE_END("encoder_underrun_callback");
> > > > >  }
> > > > >
> > > > > -void dpu_encoder_register_vblank_callback(struct drm_encoder
> > > *drm_enc,
> > > > > -		void (*vbl_cb)(void *), void *vbl_data)
> > > > > +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 = vbl_cb ? true : false;
> > > > > +	enable = crtc ? true : false;
> > > > >
> > > > >  	if (!drm_enc) {
> > > > >  		DPU_ERROR("invalid encoder\n");
> > > > > @@ -1279,8 +1279,9 @@ void dpu_encoder_register_vblank_callback(struct
> > > > > drm_encoder *drm_enc,
> > > > >  	trace_dpu_enc_vblank_cb(DRMID(drm_enc), enable);
> > > > >
> > > > >  	spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags);
> > > > > -	dpu_enc->crtc_vblank_cb = vbl_cb;
> > > > > -	dpu_enc->crtc_vblank_cb_data = vbl_data;
> > > > > +	/* 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);
> > > > >
> > > > >  	for (i = 0; i < dpu_enc->num_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 aa4f135218fa..be1d80867834 100644
> > > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > > > > @@ -55,14 +55,12 @@ void dpu_encoder_get_hw_resources(struct
> > > drm_encoder
> > > > > *encoder,
> > > > >  				  struct dpu_encoder_hw_resources
> > > > > *hw_res);
> > > > >
> > > > >  /**
> > > > > - * dpu_encoder_register_vblank_callback - provide callback to encoder
> > > > > that
> > > > > - *	will be called on the next vblank.
> > > > > + * dpu_encoder_assign_crtc - Link the encoder to the crtc it's
> > > assigned
> > > > > to
> > > > >   * @encoder:	encoder pointer
> > > > > - * @cb:		callback pointer, provide NULL to deregister and
> > > > > disable IRQs
> > > > > - * @data:	user data provided to callback
> > > > > + * @crtc:	crtc pointer
> > > > >   */
> > > > > -void dpu_encoder_register_vblank_callback(struct drm_encoder
> > > *encoder,
> > > > > -		void (*cb)(void *), void *data);
> > > > > +void dpu_encoder_assign_crtc(struct drm_encoder *encoder,
> > > > > +			     struct drm_crtc *crtc);
> > > > >
> > > > >  /**
> > > > >   * dpu_encoder_register_frame_event_callback - provide callback to
> > > > > encoder that
> > > > 
> > > > --
> > > > Jeykumar S
> > 
> > -- 
> > Jeykumar S
> 
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS

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

[PATCH 4/8] drm/msm: dpu: Use atomic_disable for dpu_crtc_disable

[Sean Paul]

From: Sean Paul <seanpaul@chromium.org>

Matches dpu_crtc_enable and we'll need the old state in a future patch

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 38119b4d4a80..a4d1a3c98318 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -823,7 +823,8 @@ static struct drm_crtc_state *dpu_crtc_duplicate_state(struct drm_crtc *crtc)
 	return &cstate->base;
 }
 
-static void dpu_crtc_disable(struct drm_crtc *crtc)
+static void dpu_crtc_disable(struct drm_crtc *crtc,
+			     struct drm_crtc_state *old_crtc_state)
 {
 	struct dpu_crtc *dpu_crtc;
 	struct dpu_crtc_state *cstate;
@@ -1421,7 +1422,7 @@ static const struct drm_crtc_funcs dpu_crtc_funcs = {
 };
 
 static const struct drm_crtc_helper_funcs dpu_crtc_helper_funcs = {
-	.disable = dpu_crtc_disable,
+	.atomic_disable = dpu_crtc_disable,
 	.atomic_enable = dpu_crtc_enable,
 	.atomic_check = dpu_crtc_atomic_check,
 	.atomic_begin = dpu_crtc_atomic_begin,
-- 
Sean Paul, Software Engineer, Google / Chromium OS

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/8] drm/msm: dpu: Remove crtc_lock from setup_mixers

[Sean Paul]

From: Sean Paul <seanpaul@chromium.org>

I think the intention here was to protect the enc->crtc access, but
that's insufficient to avoid enc->crtc changing. Fortunately we're
already holding the modeset lock when this is called (from
atomic_check), so remove the crtc_lock and add a modeset lock check.

While we're at it, use the encoder mask from crtc state instead of
legacy pointer.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index aa2f20c05dd7..adda0aa0cbaa 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -470,19 +470,13 @@ static void _dpu_crtc_setup_mixer_for_encoder(
 
 static void _dpu_crtc_setup_mixers(struct drm_crtc *crtc)
 {
-	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
 	struct drm_encoder *enc;
 
-	mutex_lock(&dpu_crtc->crtc_lock);
-	/* Check for mixers on all encoders attached to this crtc */
-	list_for_each_entry(enc, &crtc->dev->mode_config.encoder_list, head) {
-		if (enc->crtc != crtc)
-			continue;
+	WARN_ON(!drm_modeset_is_locked(&crtc->mutex));
 
+	/* Check for mixers on all encoders attached to this crtc */
+	drm_for_each_encoder_mask(enc, crtc->dev, crtc->state->encoder_mask)
 		_dpu_crtc_setup_mixer_for_encoder(crtc, enc);
-	}
-
-	mutex_unlock(&dpu_crtc->crtc_lock);
 }
 
 static void _dpu_crtc_setup_lm_bounds(struct drm_crtc *crtc,
-- 
Sean Paul, Software Engineer, Google / Chromium OS

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[PATCH 5/8] drm/msm: dpu: Don't bother checking ->enabled in dpu_crtc_vblank

[Sean Paul]

From: Sean Paul <seanpaul@chromium.org>

The drm_crtc_vblank_on/off calls in enable/disable guarantee that we
won't call this function when crtc is not enabled.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index a4d1a3c98318..4b7f98a6ab60 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -1176,9 +1176,7 @@ int dpu_crtc_vblank(struct drm_crtc *crtc, bool en)
 
 	mutex_lock(&dpu_crtc->crtc_lock);
 	trace_dpu_crtc_vblank(DRMID(&dpu_crtc->base), en, dpu_crtc);
-	if (dpu_crtc->enabled) {
-		_dpu_crtc_vblank_enable_no_lock(dpu_crtc, en);
-	}
+	_dpu_crtc_vblank_enable_no_lock(dpu_crtc, en);
 	dpu_crtc->vblank_requested = en;
 	mutex_unlock(&dpu_crtc->crtc_lock);
 
-- 
Sean Paul, Software Engineer, Google / Chromium OS

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[PATCH 8/8] drm/msm: dpu: Remove crtc_lock

[Sean Paul]

From: Sean Paul <seanpaul@chromium.org>

Each time it's called we're holding the crtc modeset lock, so it's
redundant.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 11 -----------
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h |  3 ---
 2 files changed, 14 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index ab96a2e69efa..df4ac1242706 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -69,7 +69,6 @@ static void dpu_crtc_destroy(struct drm_crtc *crtc)
 		return;
 
 	drm_crtc_cleanup(crtc);
-	mutex_destroy(&dpu_crtc->crtc_lock);
 	kfree(dpu_crtc);
 }
 
@@ -814,8 +813,6 @@ static void dpu_crtc_disable(struct drm_crtc *crtc,
 				  old_crtc_state->encoder_mask)
 		dpu_encoder_assign_crtc(encoder, NULL);
 
-	mutex_lock(&dpu_crtc->crtc_lock);
-
 	/* wait for frame_event_done completion */
 	if (_dpu_crtc_wait_for_frame_done(crtc))
 		DPU_ERROR("crtc%d wait for frame done failed;frame_pending%d\n",
@@ -847,8 +844,6 @@ static void dpu_crtc_disable(struct drm_crtc *crtc,
 	cstate->bw_control = false;
 	cstate->bw_split_vote = false;
 
-	mutex_unlock(&dpu_crtc->crtc_lock);
-
 	if (crtc->state->event && !crtc->state->active) {
 		spin_lock_irqsave(&crtc->dev->event_lock, flags);
 		drm_crtc_send_vblank_event(crtc, crtc->state->event);
@@ -884,12 +879,9 @@ static void dpu_crtc_enable(struct drm_crtc *crtc,
 				dpu_crtc_frame_event_cb, (void *)crtc);
 	}
 
-	mutex_lock(&dpu_crtc->crtc_lock);
 	trace_dpu_crtc_enable(DRMID(crtc), true, dpu_crtc);
 	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);
 
@@ -1191,7 +1183,6 @@ static int _dpu_debugfs_status_show(struct seq_file *s, void *data)
 	drm_modeset_lock_all(crtc->dev);
 	cstate = to_dpu_crtc_state(crtc->state);
 
-	mutex_lock(&dpu_crtc->crtc_lock);
 	mode = &crtc->state->adjusted_mode;
 	out_width = _dpu_crtc_get_mixer_width(cstate, mode);
 
@@ -1278,7 +1269,6 @@ static int _dpu_debugfs_status_show(struct seq_file *s, void *data)
 		dpu_crtc->vblank_cb_time = ktime_set(0, 0);
 	}
 
-	mutex_unlock(&dpu_crtc->crtc_lock);
 	drm_modeset_unlock_all(crtc->dev);
 
 	return 0;
@@ -1428,7 +1418,6 @@ struct drm_crtc *dpu_crtc_init(struct drm_device *dev, struct drm_plane *plane,
 	crtc = &dpu_crtc->base;
 	crtc->dev = dev;
 
-	mutex_init(&dpu_crtc->crtc_lock);
 	spin_lock_init(&dpu_crtc->spin_lock);
 	atomic_set(&dpu_crtc->frame_pending, 0);
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
index 2b358546af49..34f0c4d4d774 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
@@ -140,7 +140,6 @@ struct dpu_crtc_frame_event {
  * @dirty_list    : list of color processing features are dirty
  * @ad_dirty: list containing ad properties that are dirty
  * @ad_active: list containing ad properties that are active
- * @crtc_lock     : crtc lock around create, destroy and access.
  * @frame_pending : Whether or not an update is pending
  * @frame_events  : static allocation of in-flight frame events
  * @frame_event_list : available frame event list
@@ -173,8 +172,6 @@ struct dpu_crtc {
 	struct list_head ad_dirty;
 	struct list_head ad_active;
 
-	struct mutex crtc_lock;
-
 	atomic_t frame_pending;
 	struct dpu_crtc_frame_event frame_events[DPU_CRTC_FRAME_EVENT_SIZE];
 	struct list_head frame_event_list;
-- 
Sean Paul, Software Engineer, Google / Chromium OS

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [PATCH 1/8] drm/msm: dpu: Move pm_runtime_(get|put) from vblank_enable

[Sean Paul]

On Tue, Nov 13, 2018 at 03:52:44PM -0500, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>

I neglected to add --cover-letter to send-email, so pasting my cover here
instead:

Hi all,
So I kept digging into the locking and dependencies around encoder/crtc and this
is the latest series to pop out. I think things are a bit more clear and safe
at the end. We have now gotten rid of the crtc_lock and the pm_runtime sync
operations in vblank enable/disable, so we should be able to completely turf the
display thread/worker/etc!

Note that this series applies on top of [1].

Thanks!

Sean

[1]- https://lists.freedesktop.org/archives/dri-devel/2018-November/196170.html



> 
> There are 4 times that _dpu_crtc_vblank_enable_no_lock() is called:
> 
> 1- crtc enable
> 2- crtc disable
> 3- crtc vblank enable
> 4- crtc vblank disable
> 
> When we enable or disable the crtc, we call drm_crtc_vblank_on and
> drm_crtc_vblank_off respectively. That will gate vblank enables and
> disables to only being called when the crtc is active. That means that
> we can just enable/disable pm runtime in crtc enable/disable. This will
> be beneficial in trying to eliminate blocking calls from the vblank call
> chain.
> 
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index 80de5289ada3..aa2f20c05dd7 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -777,8 +777,6 @@ static void _dpu_crtc_vblank_enable_no_lock(
>  	struct drm_encoder *enc;
>  
>  	if (enable) {
> -		pm_runtime_get_sync(dev->dev);
> -
>  		list_for_each_entry(enc, &dev->mode_config.encoder_list, head) {
>  			if (enc->crtc != crtc)
>  				continue;
> @@ -801,8 +799,6 @@ static void _dpu_crtc_vblank_enable_no_lock(
>  
>  			dpu_encoder_register_vblank_callback(enc, NULL, NULL);
>  		}
> -
> -		pm_runtime_put_sync(dev->dev);
>  	}
>  }
>  
> @@ -902,6 +898,8 @@ static void dpu_crtc_disable(struct drm_crtc *crtc)
>  		crtc->state->event = NULL;
>  		spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
>  	}
> +
> +	pm_runtime_put_sync(crtc->dev->dev);
>  }
>  
>  static void dpu_crtc_enable(struct drm_crtc *crtc,
> @@ -917,6 +915,8 @@ static void dpu_crtc_enable(struct drm_crtc *crtc,
>  	}
>  	priv = crtc->dev->dev_private;
>  
> +	pm_runtime_get_sync(crtc->dev->dev);
> +
>  	DRM_DEBUG_KMS("crtc%d\n", crtc->base.id);
>  	dpu_crtc = to_dpu_crtc(crtc);
>  
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS
> 

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

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

[Sean Paul]

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(-)

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.
-- 
Sean Paul, Software Engineer, Google / Chromium OS

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Jeykumar Sankaran]

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.

-- 
Jeykumar S
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

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

[Sean Paul]

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

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

[Jeykumar Sankaran]

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

-- 
Jeykumar S
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

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

[Sean Paul]

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

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

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

[Ville Syrjälä]

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.

-- 
Ville Syrjälä
Intel
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

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

[Jeykumar Sankaran]

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

-- 
Jeykumar S
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

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

[Sean Paul]

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

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

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

[Jeykumar Sankaran]

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

-- 
Jeykumar S
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[PATCH 7/8] drm/msm: dpu: Remove vblank_requested flag from dpu_crtc

[Sean Paul]

From: Sean Paul <seanpaul@chromium.org>

It's just for debug output, we don't need it

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  |  6 ------
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h  |  2 --
 drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h | 14 ++++----------
 3 files changed, 4 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 59e823281fdf..ab96a2e69efa 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -1163,10 +1163,6 @@ int dpu_crtc_vblank(struct drm_crtc *crtc, bool en)
 		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);
-
 	return 0;
 }
 
@@ -1282,8 +1278,6 @@ static int _dpu_debugfs_status_show(struct seq_file *s, void *data)
 		dpu_crtc->vblank_cb_time = ktime_set(0, 0);
 	}
 
-	seq_printf(s, "vblank_enable:%d\n", dpu_crtc->vblank_requested);
-
 	mutex_unlock(&dpu_crtc->crtc_lock);
 	drm_modeset_unlock_all(crtc->dev);
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
index 54595cc29be5..2b358546af49 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
@@ -132,7 +132,6 @@ struct dpu_crtc_frame_event {
  * @vblank_cb_count : count of vblank callback since last reset
  * @play_count    : frame count between crtc enable and disable
  * @vblank_cb_time  : ktime at vblank count reset
- * @vblank_requested : whether the user has requested vblank events
  * @enabled       : whether the DPU CRTC is currently enabled. updated in the
  *                  commit-thread, not state-swap time which is earlier, so
  *                  safe to make decisions on during VBLANK on/off work
@@ -166,7 +165,6 @@ struct dpu_crtc {
 	u32 vblank_cb_count;
 	u64 play_count;
 	ktime_t vblank_cb_time;
-	bool vblank_requested;
 	bool enabled;
 
 	struct list_head feature_list;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
index 328df37d7580..c78b521ceda1 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
@@ -728,20 +728,17 @@ TRACE_EVENT(dpu_crtc_vblank_enable,
 		__field(	uint32_t,		enc_id	)
 		__field(	bool,			enable	)
 		__field(	bool,			enabled )
-		__field(	bool,			vblank_requested )
 	),
 	TP_fast_assign(
 		__entry->drm_id = drm_id;
 		__entry->enc_id = enc_id;
 		__entry->enable = enable;
 		__entry->enabled = crtc->enabled;
-		__entry->vblank_requested = crtc->vblank_requested;
 	),
-	TP_printk("id:%u encoder:%u enable:%s state{enabled:%s vblank_req:%s}",
+	TP_printk("id:%u encoder:%u enable:%s state{enabled:%s}",
 		  __entry->drm_id, __entry->enc_id,
 		  __entry->enable ? "true" : "false",
-		  __entry->enabled ? "true" : "false",
-		  __entry->vblank_requested ? "true" : "false")
+		  __entry->enabled ? "true" : "false")
 );
 
 DECLARE_EVENT_CLASS(dpu_crtc_enable_template,
@@ -751,18 +748,15 @@ DECLARE_EVENT_CLASS(dpu_crtc_enable_template,
 		__field(	uint32_t,		drm_id	)
 		__field(	bool,			enable	)
 		__field(	bool,			enabled )
-		__field(	bool,			vblank_requested )
 	),
 	TP_fast_assign(
 		__entry->drm_id = drm_id;
 		__entry->enable = enable;
 		__entry->enabled = crtc->enabled;
-		__entry->vblank_requested = crtc->vblank_requested;
 	),
-	TP_printk("id:%u enable:%s state{enabled:%s vblank_req:%s}",
+	TP_printk("id:%u enable:%s state{enabled:%s}",
 		  __entry->drm_id, __entry->enable ? "true" : "false",
-		  __entry->enabled ? "true" : "false",
-		  __entry->vblank_requested ? "true" : "false")
+		  __entry->enabled ? "true" : "false")
 );
 DEFINE_EVENT(dpu_crtc_enable_template, dpu_crtc_enable,
 	TP_PROTO(uint32_t drm_id, bool enable, struct dpu_crtc *crtc),
-- 
Sean Paul, Software Engineer, Google / Chromium OS

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 7/8] drm/msm: dpu: Remove vblank_requested flag from dpu_crtc

[Jeykumar Sankaran]

On 2018-11-13 12:52, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
> 
> It's just for debug output, we don't need it
> 
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  |  6 ------
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h  |  2 --
>  drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h | 14 ++++----------
>  3 files changed, 4 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index 59e823281fdf..ab96a2e69efa 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -1163,10 +1163,6 @@ int dpu_crtc_vblank(struct drm_crtc *crtc, bool 
> en)
>  		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);
> -
>  	return 0;
>  }
> 
> @@ -1282,8 +1278,6 @@ static int _dpu_debugfs_status_show(struct 
> seq_file
> *s, void *data)
>  		dpu_crtc->vblank_cb_time = ktime_set(0, 0);
>  	}
> 
> -	seq_printf(s, "vblank_enable:%d\n", dpu_crtc->vblank_requested);
> -
>  	mutex_unlock(&dpu_crtc->crtc_lock);
>  	drm_modeset_unlock_all(crtc->dev);
> 

I see few more references @ 
https://gitlab.freedesktop.org/seanpaul/dpu-staging/blob/for-next/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c#L900

Which change is removing them?

> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> index 54595cc29be5..2b358546af49 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> @@ -132,7 +132,6 @@ struct dpu_crtc_frame_event {
>   * @vblank_cb_count : count of vblank callback since last reset
>   * @play_count    : frame count between crtc enable and disable
>   * @vblank_cb_time  : ktime at vblank count reset
> - * @vblank_requested : whether the user has requested vblank events
>   * @enabled       : whether the DPU CRTC is currently enabled. updated 
> in
> the
>   *                  commit-thread, not state-swap time which is 
> earlier,
> so
>   *                  safe to make decisions on during VBLANK on/off 
> work
> @@ -166,7 +165,6 @@ struct dpu_crtc {
>  	u32 vblank_cb_count;
>  	u64 play_count;
>  	ktime_t vblank_cb_time;
> -	bool vblank_requested;
>  	bool enabled;
> 
>  	struct list_head feature_list;
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
> index 328df37d7580..c78b521ceda1 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
> @@ -728,20 +728,17 @@ TRACE_EVENT(dpu_crtc_vblank_enable,
>  		__field(	uint32_t,		enc_id	)
>  		__field(	bool,			enable	)
>  		__field(	bool,			enabled )
> -		__field(	bool,			vblank_requested )
>  	),
>  	TP_fast_assign(
>  		__entry->drm_id = drm_id;
>  		__entry->enc_id = enc_id;
>  		__entry->enable = enable;
>  		__entry->enabled = crtc->enabled;
> -		__entry->vblank_requested = crtc->vblank_requested;
>  	),
> -	TP_printk("id:%u encoder:%u enable:%s state{enabled:%s
> vblank_req:%s}",
> +	TP_printk("id:%u encoder:%u enable:%s state{enabled:%s}",
>  		  __entry->drm_id, __entry->enc_id,
>  		  __entry->enable ? "true" : "false",
> -		  __entry->enabled ? "true" : "false",
> -		  __entry->vblank_requested ? "true" : "false")
> +		  __entry->enabled ? "true" : "false")
>  );
> 
>  DECLARE_EVENT_CLASS(dpu_crtc_enable_template,
> @@ -751,18 +748,15 @@ DECLARE_EVENT_CLASS(dpu_crtc_enable_template,
>  		__field(	uint32_t,		drm_id	)
>  		__field(	bool,			enable	)
>  		__field(	bool,			enabled )
> -		__field(	bool,			vblank_requested )
>  	),
>  	TP_fast_assign(
>  		__entry->drm_id = drm_id;
>  		__entry->enable = enable;
>  		__entry->enabled = crtc->enabled;
> -		__entry->vblank_requested = crtc->vblank_requested;
>  	),
> -	TP_printk("id:%u enable:%s state{enabled:%s vblank_req:%s}",
> +	TP_printk("id:%u enable:%s state{enabled:%s}",
>  		  __entry->drm_id, __entry->enable ? "true" : "false",
> -		  __entry->enabled ? "true" : "false",
> -		  __entry->vblank_requested ? "true" : "false")
> +		  __entry->enabled ? "true" : "false")
>  );
>  DEFINE_EVENT(dpu_crtc_enable_template, dpu_crtc_enable,
>  	TP_PROTO(uint32_t drm_id, bool enable, struct dpu_crtc *crtc),

-- 
Jeykumar S
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [PATCH 7/8] drm/msm: dpu: Remove vblank_requested flag from dpu_crtc

[Sean Paul]

On Tue, Nov 13, 2018 at 04:47:34PM -0800, Jeykumar Sankaran wrote:
> On 2018-11-13 12:52, Sean Paul wrote:
> > From: Sean Paul <seanpaul@chromium.org>
> > 
> > It's just for debug output, we don't need it
> > 
> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > ---
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  |  6 ------
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h  |  2 --
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h | 14 ++++----------
> >  3 files changed, 4 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > index 59e823281fdf..ab96a2e69efa 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > @@ -1163,10 +1163,6 @@ int dpu_crtc_vblank(struct drm_crtc *crtc, bool
> > en)
> >  		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);
> > -
> >  	return 0;
> >  }
> > 
> > @@ -1282,8 +1278,6 @@ static int _dpu_debugfs_status_show(struct
> > seq_file
> > *s, void *data)
> >  		dpu_crtc->vblank_cb_time = ktime_set(0, 0);
> >  	}
> > 
> > -	seq_printf(s, "vblank_enable:%d\n", dpu_crtc->vblank_requested);
> > -
> >  	mutex_unlock(&dpu_crtc->crtc_lock);
> >  	drm_modeset_unlock_all(crtc->dev);
> > 
> 
> I see few more references @ https://gitlab.freedesktop.org/seanpaul/dpu-staging/blob/for-next/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c#L900
> 
> Which change is removing them?

Patch 6

> 
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> > index 54595cc29be5..2b358546af49 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> > @@ -132,7 +132,6 @@ struct dpu_crtc_frame_event {
> >   * @vblank_cb_count : count of vblank callback since last reset
> >   * @play_count    : frame count between crtc enable and disable
> >   * @vblank_cb_time  : ktime at vblank count reset
> > - * @vblank_requested : whether the user has requested vblank events
> >   * @enabled       : whether the DPU CRTC is currently enabled. updated
> > in
> > the
> >   *                  commit-thread, not state-swap time which is
> > earlier,
> > so
> >   *                  safe to make decisions on during VBLANK on/off work
> > @@ -166,7 +165,6 @@ struct dpu_crtc {
> >  	u32 vblank_cb_count;
> >  	u64 play_count;
> >  	ktime_t vblank_cb_time;
> > -	bool vblank_requested;
> >  	bool enabled;
> > 
> >  	struct list_head feature_list;
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
> > index 328df37d7580..c78b521ceda1 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
> > @@ -728,20 +728,17 @@ TRACE_EVENT(dpu_crtc_vblank_enable,
> >  		__field(	uint32_t,		enc_id	)
> >  		__field(	bool,			enable	)
> >  		__field(	bool,			enabled )
> > -		__field(	bool,			vblank_requested )
> >  	),
> >  	TP_fast_assign(
> >  		__entry->drm_id = drm_id;
> >  		__entry->enc_id = enc_id;
> >  		__entry->enable = enable;
> >  		__entry->enabled = crtc->enabled;
> > -		__entry->vblank_requested = crtc->vblank_requested;
> >  	),
> > -	TP_printk("id:%u encoder:%u enable:%s state{enabled:%s
> > vblank_req:%s}",
> > +	TP_printk("id:%u encoder:%u enable:%s state{enabled:%s}",
> >  		  __entry->drm_id, __entry->enc_id,
> >  		  __entry->enable ? "true" : "false",
> > -		  __entry->enabled ? "true" : "false",
> > -		  __entry->vblank_requested ? "true" : "false")
> > +		  __entry->enabled ? "true" : "false")
> >  );
> > 
> >  DECLARE_EVENT_CLASS(dpu_crtc_enable_template,
> > @@ -751,18 +748,15 @@ DECLARE_EVENT_CLASS(dpu_crtc_enable_template,
> >  		__field(	uint32_t,		drm_id	)
> >  		__field(	bool,			enable	)
> >  		__field(	bool,			enabled )
> > -		__field(	bool,			vblank_requested )
> >  	),
> >  	TP_fast_assign(
> >  		__entry->drm_id = drm_id;
> >  		__entry->enable = enable;
> >  		__entry->enabled = crtc->enabled;
> > -		__entry->vblank_requested = crtc->vblank_requested;
> >  	),
> > -	TP_printk("id:%u enable:%s state{enabled:%s vblank_req:%s}",
> > +	TP_printk("id:%u enable:%s state{enabled:%s}",
> >  		  __entry->drm_id, __entry->enable ? "true" : "false",
> > -		  __entry->enabled ? "true" : "false",
> > -		  __entry->vblank_requested ? "true" : "false")
> > +		  __entry->enabled ? "true" : "false")
> >  );
> >  DEFINE_EVENT(dpu_crtc_enable_template, dpu_crtc_enable,
> >  	TP_PROTO(uint32_t drm_id, bool enable, struct dpu_crtc *crtc),
> 
> -- 
> Jeykumar S

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