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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2018-11-13 12:52, Sean Paul wrote:
From: Sean Paul <seanpaul@xxxxxxxxxxxx>

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@xxxxxxxxxxxx>
---
 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux