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

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

Details

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

Not browsing as part of any series.

Commit Message

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

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 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

Comments

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