[PATCH 1/2] drm/msm/dpu: remove dpu_encoder_virt_ops

[PATCH 1/2] drm/msm/dpu: remove dpu_encoder_virt_ops

[Dmitry Baryshkov]

Struct dpu_encoder_virt_ops is used to provide several callbacks to the
phys_enc backends. However these ops are static and are not supposed to
change in the foreseeble future. Drop the indirection and call
corresponding functions directly.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   | 17 ++-----
 .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  | 47 ++++++++++---------
 .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  | 17 ++-----
 .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  | 11 ++---
 .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c   | 13 ++---
 5 files changed, 40 insertions(+), 65 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 9c6817b5a194..84f8c8a1b049 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -340,9 +340,7 @@ void dpu_encoder_helper_report_irq_timeout(struct dpu_encoder_phys *phys_enc,
 			phys_enc->intf_idx - INTF_0, phys_enc->wb_idx - WB_0,
 			phys_enc->hw_pp->idx - PINGPONG_0, intr_idx);
 
-	if (phys_enc->parent_ops->handle_frame_done)
-		phys_enc->parent_ops->handle_frame_done(
-				phys_enc->parent, phys_enc,
+	dpu_encoder_frame_done_callback(phys_enc->parent, phys_enc,
 				DPU_ENCODER_FRAME_EVENT_ERROR);
 }
 
@@ -1284,7 +1282,7 @@ static enum dpu_wb dpu_encoder_get_wb(const struct dpu_mdss_cfg *catalog,
 	return WB_MAX;
 }
 
-static void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc,
+void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc,
 		struct dpu_encoder_phys *phy_enc)
 {
 	struct dpu_encoder_virt *dpu_enc = NULL;
@@ -1306,7 +1304,7 @@ static void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc,
 	DPU_ATRACE_END("encoder_vblank_callback");
 }
 
-static void dpu_encoder_underrun_callback(struct drm_encoder *drm_enc,
+void dpu_encoder_underrun_callback(struct drm_encoder *drm_enc,
 		struct dpu_encoder_phys *phy_enc)
 {
 	if (!phy_enc)
@@ -1382,7 +1380,7 @@ void dpu_encoder_register_frame_event_callback(struct drm_encoder *drm_enc,
 	spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags);
 }
 
-static void dpu_encoder_frame_done_callback(
+void dpu_encoder_frame_done_callback(
 		struct drm_encoder *drm_enc,
 		struct dpu_encoder_phys *ready_phys, u32 event)
 {
@@ -2233,12 +2231,6 @@ static int dpu_encoder_virt_add_phys_encs(
 	return 0;
 }
 
-static const struct dpu_encoder_virt_ops dpu_encoder_parent_ops = {
-	.handle_vblank_virt = dpu_encoder_vblank_callback,
-	.handle_underrun_virt = dpu_encoder_underrun_callback,
-	.handle_frame_done = dpu_encoder_frame_done_callback,
-};
-
 static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc,
 				 struct dpu_kms *dpu_kms,
 				 struct msm_display_info *disp_info)
@@ -2258,7 +2250,6 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc,
 	memset(&phys_params, 0, sizeof(phys_params));
 	phys_params.dpu_kms = dpu_kms;
 	phys_params.parent = &dpu_enc->base;
-	phys_params.parent_ops = &dpu_encoder_parent_ops;
 	phys_params.enc_spinlock = &dpu_enc->enc_spinlock;
 
 	switch (disp_info->intf_type) {
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
index f2af07d87f56..1d434b22180d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
@@ -60,25 +60,6 @@ enum dpu_enc_enable_state {
 
 struct dpu_encoder_phys;
 
-/**
- * struct dpu_encoder_virt_ops - Interface the containing virtual encoder
- *	provides for the physical encoders to use to callback.
- * @handle_vblank_virt:	Notify virtual encoder of vblank IRQ reception
- *			Note: This is called from IRQ handler context.
- * @handle_underrun_virt: Notify virtual encoder of underrun IRQ reception
- *			Note: This is called from IRQ handler context.
- * @handle_frame_done:	Notify virtual encoder that this phys encoder
- *			completes last request frame.
- */
-struct dpu_encoder_virt_ops {
-	void (*handle_vblank_virt)(struct drm_encoder *,
-			struct dpu_encoder_phys *phys);
-	void (*handle_underrun_virt)(struct drm_encoder *,
-			struct dpu_encoder_phys *phys);
-	void (*handle_frame_done)(struct drm_encoder *,
-			struct dpu_encoder_phys *phys, u32 event);
-};
-
 /**
  * struct dpu_encoder_phys_ops - Interface the physical encoders provide to
  *	the containing virtual encoder.
@@ -199,7 +180,6 @@ enum dpu_intr_idx {
 struct dpu_encoder_phys {
 	struct drm_encoder *parent;
 	struct dpu_encoder_phys_ops ops;
-	const struct dpu_encoder_virt_ops *parent_ops;
 	struct dpu_hw_mdp *hw_mdptop;
 	struct dpu_hw_ctl *hw_ctl;
 	struct dpu_hw_pingpong *hw_pp;
@@ -283,7 +263,6 @@ struct dpu_encoder_phys_cmd {
 struct dpu_enc_phys_init_params {
 	struct dpu_kms *dpu_kms;
 	struct drm_encoder *parent;
-	const struct dpu_encoder_virt_ops *parent_ops;
 	enum dpu_enc_split_role split_role;
 	enum dpu_intf intf_idx;
 	enum dpu_wb wb_idx;
@@ -400,4 +379,30 @@ int dpu_encoder_helper_wait_for_irq(struct dpu_encoder_phys *phys_enc,
  */
 void dpu_encoder_helper_phys_cleanup(struct dpu_encoder_phys *phys_enc);
 
+/**
+ * dpu_encoder_vblank_callback - Notify virtual encoder of vblank IRQ reception
+ * @drm_enc:    Pointer to drm encoder structure
+ * @phys_enc:	Pointer to physical encoder
+ * Note: This is called from IRQ handler context.
+ */
+void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc,
+				 struct dpu_encoder_phys *phy_enc);
+
+/** dpu_encoder_underrun_callback - Notify virtual encoder of underrun IRQ reception
+ * @drm_enc:    Pointer to drm encoder structure
+ * @phys_enc:	Pointer to physical encoder
+ * Note: This is called from IRQ handler context.
+ */
+void dpu_encoder_underrun_callback(struct drm_encoder *drm_enc,
+				   struct dpu_encoder_phys *phy_enc);
+
+/** dpu_encoder_frame_done_callback -- Notify virtual encoder that this phys encoder completes last request frame
+ * @drm_enc:    Pointer to drm encoder structure
+ * @phys_enc:	Pointer to physical encoder
+ * @event:	Event to process
+ */
+void dpu_encoder_frame_done_callback(
+		struct drm_encoder *drm_enc,
+		struct dpu_encoder_phys *ready_phys, u32 event);
+
 #endif /* __dpu_encoder_phys_H__ */
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
index ae28b2b93e69..41bd7dd2b482 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
@@ -83,9 +83,7 @@ static void dpu_encoder_phys_cmd_pp_tx_done_irq(void *arg, int irq_idx)
 
 	DPU_ATRACE_BEGIN("pp_done_irq");
 	/* notify all synchronous clients first, then asynchronous clients */
-	if (phys_enc->parent_ops->handle_frame_done)
-		phys_enc->parent_ops->handle_frame_done(phys_enc->parent,
-				phys_enc, event);
+	dpu_encoder_frame_done_callback(phys_enc->parent, phys_enc, event);
 
 	spin_lock_irqsave(phys_enc->enc_spinlock, lock_flags);
 	new_cnt = atomic_add_unless(&phys_enc->pending_kickoff_cnt, -1, 0);
@@ -111,9 +109,7 @@ static void dpu_encoder_phys_cmd_pp_rd_ptr_irq(void *arg, int irq_idx)
 	DPU_ATRACE_BEGIN("rd_ptr_irq");
 	cmd_enc = to_dpu_encoder_phys_cmd(phys_enc);
 
-	if (phys_enc->parent_ops->handle_vblank_virt)
-		phys_enc->parent_ops->handle_vblank_virt(phys_enc->parent,
-			phys_enc);
+	dpu_encoder_vblank_callback(phys_enc->parent, phys_enc);
 
 	atomic_add_unless(&cmd_enc->pending_vblank_cnt, -1, 0);
 	wake_up_all(&cmd_enc->pending_vblank_wq);
@@ -137,9 +133,7 @@ static void dpu_encoder_phys_cmd_underrun_irq(void *arg, int irq_idx)
 {
 	struct dpu_encoder_phys *phys_enc = arg;
 
-	if (phys_enc->parent_ops->handle_underrun_virt)
-		phys_enc->parent_ops->handle_underrun_virt(phys_enc->parent,
-			phys_enc);
+	dpu_encoder_underrun_callback(phys_enc->parent, phys_enc);
 }
 
 static void dpu_encoder_phys_cmd_atomic_mode_set(
@@ -202,9 +196,7 @@ static int _dpu_encoder_phys_cmd_handle_ppdone_timeout(
 	/* request a ctl reset before the next kickoff */
 	phys_enc->enable_state = DPU_ENC_ERR_NEEDS_HW_RESET;
 
-	if (phys_enc->parent_ops->handle_frame_done)
-		phys_enc->parent_ops->handle_frame_done(
-				drm_enc, phys_enc, frame_event);
+	dpu_encoder_frame_done_callback(phys_enc->parent, phys_enc, frame_event);
 
 	return -ETIMEDOUT;
 }
@@ -780,7 +772,6 @@ struct dpu_encoder_phys *dpu_encoder_phys_cmd_init(
 
 	dpu_encoder_phys_cmd_init_ops(&phys_enc->ops);
 	phys_enc->parent = p->parent;
-	phys_enc->parent_ops = p->parent_ops;
 	phys_enc->dpu_kms = p->dpu_kms;
 	phys_enc->split_role = p->split_role;
 	phys_enc->intf_mode = INTF_MODE_CMD;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
index 0f71e8fe7be7..39ca1b305114 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
@@ -308,9 +308,7 @@ static void dpu_encoder_phys_vid_vblank_irq(void *arg, int irq_idx)
 
 	DPU_ATRACE_BEGIN("vblank_irq");
 
-	if (phys_enc->parent_ops->handle_vblank_virt)
-		phys_enc->parent_ops->handle_vblank_virt(phys_enc->parent,
-				phys_enc);
+	dpu_encoder_vblank_callback(phys_enc->parent, phys_enc);
 
 	atomic_read(&phys_enc->pending_kickoff_cnt);
 
@@ -330,7 +328,7 @@ static void dpu_encoder_phys_vid_vblank_irq(void *arg, int irq_idx)
 	/* Signal any waiting atomic commit thread */
 	wake_up_all(&phys_enc->pending_kickoff_wq);
 
-	phys_enc->parent_ops->handle_frame_done(phys_enc->parent, phys_enc,
+	dpu_encoder_frame_done_callback(phys_enc->parent, phys_enc,
 			DPU_ENCODER_FRAME_EVENT_DONE);
 
 	DPU_ATRACE_END("vblank_irq");
@@ -340,9 +338,7 @@ static void dpu_encoder_phys_vid_underrun_irq(void *arg, int irq_idx)
 {
 	struct dpu_encoder_phys *phys_enc = arg;
 
-	if (phys_enc->parent_ops->handle_underrun_virt)
-		phys_enc->parent_ops->handle_underrun_virt(phys_enc->parent,
-			phys_enc);
+	dpu_encoder_underrun_callback(phys_enc->parent, phys_enc);
 }
 
 static bool dpu_encoder_phys_vid_needs_single_flush(
@@ -700,7 +696,6 @@ struct dpu_encoder_phys *dpu_encoder_phys_vid_init(
 
 	dpu_encoder_phys_vid_init_ops(&phys_enc->ops);
 	phys_enc->parent = p->parent;
-	phys_enc->parent_ops = p->parent_ops;
 	phys_enc->dpu_kms = p->dpu_kms;
 	phys_enc->split_role = p->split_role;
 	phys_enc->intf_mode = INTF_MODE_VIDEO;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
index 7cbcef6efe17..c5146b6477d6 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
@@ -365,13 +365,9 @@ static void _dpu_encoder_phys_wb_frame_done_helper(void *arg)
 
 	DPU_DEBUG("[wb:%d]\n", hw_wb->idx - WB_0);
 
-	if (phys_enc->parent_ops->handle_frame_done)
-		phys_enc->parent_ops->handle_frame_done(phys_enc->parent,
-				phys_enc, event);
+	dpu_encoder_frame_done_callback(phys_enc->parent, phys_enc, event);
 
-	if (phys_enc->parent_ops->handle_vblank_virt)
-		phys_enc->parent_ops->handle_vblank_virt(phys_enc->parent,
-				phys_enc);
+	dpu_encoder_vblank_callback(phys_enc->parent, phys_enc);
 
 	spin_lock_irqsave(phys_enc->enc_spinlock, lock_flags);
 	atomic_add_unless(&phys_enc->pending_kickoff_cnt, -1, 0);
@@ -441,9 +437,7 @@ static void _dpu_encoder_phys_wb_handle_wbdone_timeout(
 	if (wb_enc->wb_conn)
 		drm_writeback_signal_completion(wb_enc->wb_conn, 0);
 
-	if (phys_enc->parent_ops->handle_frame_done)
-		phys_enc->parent_ops->handle_frame_done(
-				phys_enc->parent, phys_enc, frame_event);
+	dpu_encoder_frame_done_callback(phys_enc->parent, phys_enc, frame_event);
 }
 
 /**
@@ -723,7 +717,6 @@ struct dpu_encoder_phys *dpu_encoder_phys_wb_init(
 
 	dpu_encoder_phys_wb_init_ops(&phys_enc->ops);
 	phys_enc->parent = p->parent;
-	phys_enc->parent_ops = p->parent_ops;
 	phys_enc->dpu_kms = p->dpu_kms;
 	phys_enc->split_role = p->split_role;
 	phys_enc->intf_mode = INTF_MODE_WB_LINE;
-- 
2.39.0

[PATCH 2/2] drm/msm/dpu: remove CRTC frame event callback registration

[Dmitry Baryshkov]

The frame event callback is always set to dpu_crtc_frame_event_cb() (or
to NULL) and the data is always either the CRTC itself or NULL
(correpondingly). Thus drop the event callback registration, call the
dpu_crtc_frame_event_cb() directly and gate on the dpu_enc->crtc
assigned using dpu_encoder_assign_crtc().

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 17 +--------
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    | 14 +++++++
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 41 +++------------------
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 10 -----
 drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h   |  4 --
 5 files changed, 21 insertions(+), 65 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 13ce321283ff..64cd2ec8a0c4 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -640,18 +640,8 @@ static void dpu_crtc_frame_event_work(struct kthread_work *work)
 	DPU_ATRACE_END("crtc_frame_event");
 }
 
-/*
- * dpu_crtc_frame_event_cb - crtc frame event callback API. CRTC module
- * registers this API to encoder for all frame event callbacks like
- * frame_error, frame_done, idle_timeout, etc. Encoder may call different events
- * from different context - IRQ, user thread, commit_thread, etc. Each event
- * should be carefully reviewed and should be processed in proper task context
- * to avoid schedulin delay or properly manage the irq context's bottom half
- * processing.
- */
-static void dpu_crtc_frame_event_cb(void *data, u32 event)
+void dpu_crtc_frame_event_cb(struct drm_crtc *crtc, u32 event)
 {
-	struct drm_crtc *crtc = (struct drm_crtc *)data;
 	struct dpu_crtc *dpu_crtc;
 	struct msm_drm_private *priv;
 	struct dpu_crtc_frame_event *fevent;
@@ -1051,9 +1041,6 @@ static void dpu_crtc_disable(struct drm_crtc *crtc,
 
 	dpu_core_perf_crtc_update(crtc, 0, true);
 
-	drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask)
-		dpu_encoder_register_frame_event_callback(encoder, NULL, NULL);
-
 	memset(cstate->mixers, 0, sizeof(cstate->mixers));
 	cstate->num_mixers = 0;
 
@@ -1089,8 +1076,6 @@ static void dpu_crtc_enable(struct drm_crtc *crtc,
 		 */
 		if (dpu_encoder_get_intf_mode(encoder) == INTF_MODE_VIDEO)
 			request_bandwidth = true;
-		dpu_encoder_register_frame_event_callback(encoder,
-				dpu_crtc_frame_event_cb, (void *)crtc);
 	}
 
 	if (request_bandwidth)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
index 539b68b1626a..3aa536d95721 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
@@ -300,4 +300,18 @@ static inline enum dpu_crtc_client_type dpu_crtc_get_client_type(
 	return crtc && crtc->state ? RT_CLIENT : NRT_CLIENT;
 }
 
+/**
+ * dpu_crtc_frame_event_cb - crtc frame event callback API
+ * @crtc: Pointer to crtc
+ * @event: Event to process
+ *
+ * CRTC module registers this API to encoder for all frame event callbacks like
+ * frame_error, frame_done, idle_timeout, etc. Encoder may call different events
+ * from different context - IRQ, user thread, commit_thread, etc. Each event
+ * should be carefully reviewed and should be processed in proper task context
+ * to avoid schedulin delay or properly manage the irq context's bottom half
+ * processing.
+ */
+void dpu_crtc_frame_event_cb(struct drm_crtc *crtc, u32 event);
+
 #endif /* _DPU_CRTC_H_ */
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 84f8c8a1b049..bf0af5af4306 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -147,8 +147,6 @@ enum dpu_enc_rc_states {
  * @frame_busy_mask:		Bitmask tracking which phys_enc we are still
  *				busy processing current command.
  *				Bit0 = phys_encs[0] etc.
- * @crtc_frame_event_cb:	callback handler for frame event
- * @crtc_frame_event_cb_data:	callback handler private data
  * @frame_done_timeout_ms:	frame done timeout in ms
  * @frame_done_timer:		watchdog timer for frame done event
  * @vsync_event_timer:		vsync timer
@@ -187,8 +185,6 @@ struct dpu_encoder_virt {
 	struct dentry *debugfs_root;
 	struct mutex enc_lock;
 	DECLARE_BITMAP(frame_busy_mask, MAX_PHYS_ENCODERS_PER_VIRTUAL);
-	void (*crtc_frame_event_cb)(void *, u32 event);
-	void *crtc_frame_event_cb_data;
 
 	atomic_t frame_done_timeout_ms;
 	struct timer_list frame_done_timer;
@@ -1358,28 +1354,6 @@ void dpu_encoder_toggle_vblank_for_crtc(struct drm_encoder *drm_enc,
 	}
 }
 
-void dpu_encoder_register_frame_event_callback(struct drm_encoder *drm_enc,
-		void (*frame_event_cb)(void *, u32 event),
-		void *frame_event_cb_data)
-{
-	struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
-	unsigned long lock_flags;
-	bool enable;
-
-	enable = frame_event_cb ? true : false;
-
-	if (!drm_enc) {
-		DPU_ERROR("invalid encoder\n");
-		return;
-	}
-	trace_dpu_enc_frame_event_cb(DRMID(drm_enc), enable);
-
-	spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags);
-	dpu_enc->crtc_frame_event_cb = frame_event_cb;
-	dpu_enc->crtc_frame_event_cb_data = frame_event_cb_data;
-	spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags);
-}
-
 void dpu_encoder_frame_done_callback(
 		struct drm_encoder *drm_enc,
 		struct dpu_encoder_phys *ready_phys, u32 event)
@@ -1418,15 +1392,12 @@ void dpu_encoder_frame_done_callback(
 			dpu_encoder_resource_control(drm_enc,
 					DPU_ENC_RC_EVENT_FRAME_DONE);
 
-			if (dpu_enc->crtc_frame_event_cb)
-				dpu_enc->crtc_frame_event_cb(
-					dpu_enc->crtc_frame_event_cb_data,
-					event);
+			if (dpu_enc->crtc)
+				dpu_crtc_frame_event_cb(dpu_enc->crtc, event);
 		}
 	} else {
-		if (dpu_enc->crtc_frame_event_cb)
-			dpu_enc->crtc_frame_event_cb(
-				dpu_enc->crtc_frame_event_cb_data, event);
+		if (dpu_enc->crtc)
+			dpu_crtc_frame_event_cb(dpu_enc->crtc, event);
 	}
 }
 
@@ -2367,7 +2338,7 @@ static void dpu_encoder_frame_done_timeout(struct timer_list *t)
 		return;
 	}
 
-	if (!dpu_enc->frame_busy_mask[0] || !dpu_enc->crtc_frame_event_cb) {
+	if (!dpu_enc->frame_busy_mask[0] || !dpu_enc->crtc) {
 		DRM_DEBUG_KMS("id:%u invalid timeout frame_busy_mask=%lu\n",
 			      DRMID(drm_enc), dpu_enc->frame_busy_mask[0]);
 		return;
@@ -2380,7 +2351,7 @@ static void dpu_encoder_frame_done_timeout(struct timer_list *t)
 
 	event = DPU_ENCODER_FRAME_EVENT_ERROR;
 	trace_dpu_enc_frame_done_timeout(DRMID(drm_enc), event);
-	dpu_enc->crtc_frame_event_cb(dpu_enc->crtc_frame_event_cb_data, event);
+	dpu_crtc_frame_event_cb(dpu_enc->crtc, event);
 }
 
 static const struct drm_encoder_helper_funcs dpu_encoder_helper_funcs = {
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
index 9e7236ef34e6..dd9cff4f1606 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
@@ -57,16 +57,6 @@ void dpu_encoder_assign_crtc(struct drm_encoder *encoder,
 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.
- * @encoder:	encoder pointer
- * @cb:		callback pointer, provide NULL to deregister
- * @data:	user data provided to callback
- */
-void dpu_encoder_register_frame_event_callback(struct drm_encoder *encoder,
-		void (*cb)(void *, u32), void *data);
-
 /**
  * dpu_encoder_prepare_for_kickoff - schedule double buffer flip of the ctl
  *	path (i.e. ctl flush and start) at next appropriate time.
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
index 76169f406505..c75b4363bfc6 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
@@ -346,10 +346,6 @@ DEFINE_EVENT(dpu_enc_id_enable_template, dpu_enc_vblank_cb,
 	TP_PROTO(uint32_t drm_id, bool enable),
 	TP_ARGS(drm_id, enable)
 );
-DEFINE_EVENT(dpu_enc_id_enable_template, dpu_enc_frame_event_cb,
-	TP_PROTO(uint32_t drm_id, bool enable),
-	TP_ARGS(drm_id, enable)
-);
 DEFINE_EVENT(dpu_enc_id_enable_template, dpu_enc_phys_cmd_connect_te,
 	TP_PROTO(uint32_t drm_id, bool enable),
 	TP_ARGS(drm_id, enable)
-- 
2.39.0

Re: [PATCH 1/2] drm/msm/dpu: remove dpu_encoder_virt_ops

[Abhinav Kumar]

On 1/2/2023 7:47 AM, Dmitry Baryshkov wrote:
> Struct dpu_encoder_virt_ops is used to provide several callbacks to the
> phys_enc backends. However these ops are static and are not supposed to
> change in the foreseeble future. Drop the indirection and call
> corresponding functions directly.
> 

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

With a few minor comments below.

Also, I did a quick check if this would conflict with PSR but this 
change wont, probably the next one will so this one should be okay.

> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   | 17 ++-----
>   .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  | 47 ++++++++++---------
>   .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  | 17 ++-----
>   .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  | 11 ++---
>   .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c   | 13 ++---
>   5 files changed, 40 insertions(+), 65 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 9c6817b5a194..84f8c8a1b049 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -340,9 +340,7 @@ void dpu_encoder_helper_report_irq_timeout(struct dpu_encoder_phys *phys_enc,
>   			phys_enc->intf_idx - INTF_0, phys_enc->wb_idx - WB_0,
>   			phys_enc->hw_pp->idx - PINGPONG_0, intr_idx);
>   
> -	if (phys_enc->parent_ops->handle_frame_done)
> -		phys_enc->parent_ops->handle_frame_done(
> -				phys_enc->parent, phys_enc,
> +	dpu_encoder_frame_done_callback(phys_enc->parent, phys_enc,
>   				DPU_ENCODER_FRAME_EVENT_ERROR);
>   }
>   
> @@ -1284,7 +1282,7 @@ static enum dpu_wb dpu_encoder_get_wb(const struct dpu_mdss_cfg *catalog,
>   	return WB_MAX;
>   }
>   
> -static void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc,
> +void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc,
>   		struct dpu_encoder_phys *phy_enc)
>   {
>   	struct dpu_encoder_virt *dpu_enc = NULL;
> @@ -1306,7 +1304,7 @@ static void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc,
>   	DPU_ATRACE_END("encoder_vblank_callback");
>   }
>   
> -static void dpu_encoder_underrun_callback(struct drm_encoder *drm_enc,
> +void dpu_encoder_underrun_callback(struct drm_encoder *drm_enc,
>   		struct dpu_encoder_phys *phy_enc)
>   {
>   	if (!phy_enc)
> @@ -1382,7 +1380,7 @@ void dpu_encoder_register_frame_event_callback(struct drm_encoder *drm_enc,
>   	spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags);
>   }
>   
> -static void dpu_encoder_frame_done_callback(
> +void dpu_encoder_frame_done_callback(
>   		struct drm_encoder *drm_enc,
>   		struct dpu_encoder_phys *ready_phys, u32 event)
>   {
> @@ -2233,12 +2231,6 @@ static int dpu_encoder_virt_add_phys_encs(
>   	return 0;
>   }
>   
> -static const struct dpu_encoder_virt_ops dpu_encoder_parent_ops = {
> -	.handle_vblank_virt = dpu_encoder_vblank_callback,
> -	.handle_underrun_virt = dpu_encoder_underrun_callback,
> -	.handle_frame_done = dpu_encoder_frame_done_callback,
> -};
> -
>   static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc,
>   				 struct dpu_kms *dpu_kms,
>   				 struct msm_display_info *disp_info)
> @@ -2258,7 +2250,6 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc,
>   	memset(&phys_params, 0, sizeof(phys_params));
>   	phys_params.dpu_kms = dpu_kms;
>   	phys_params.parent = &dpu_enc->base;
> -	phys_params.parent_ops = &dpu_encoder_parent_ops;
>   	phys_params.enc_spinlock = &dpu_enc->enc_spinlock;
>   
>   	switch (disp_info->intf_type) {
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> index f2af07d87f56..1d434b22180d 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> @@ -60,25 +60,6 @@ enum dpu_enc_enable_state {
>   
>   struct dpu_encoder_phys;
>   
> -/**
> - * struct dpu_encoder_virt_ops - Interface the containing virtual encoder
> - *	provides for the physical encoders to use to callback.
> - * @handle_vblank_virt:	Notify virtual encoder of vblank IRQ reception
> - *			Note: This is called from IRQ handler context.
> - * @handle_underrun_virt: Notify virtual encoder of underrun IRQ reception
> - *			Note: This is called from IRQ handler context.
> - * @handle_frame_done:	Notify virtual encoder that this phys encoder
> - *			completes last request frame.
> - */
> -struct dpu_encoder_virt_ops {
> -	void (*handle_vblank_virt)(struct drm_encoder *,
> -			struct dpu_encoder_phys *phys);
> -	void (*handle_underrun_virt)(struct drm_encoder *,
> -			struct dpu_encoder_phys *phys);
> -	void (*handle_frame_done)(struct drm_encoder *,
> -			struct dpu_encoder_phys *phys, u32 event);
> -};
> -
>   /**
>    * struct dpu_encoder_phys_ops - Interface the physical encoders provide to
>    *	the containing virtual encoder.
> @@ -199,7 +180,6 @@ enum dpu_intr_idx {
>   struct dpu_encoder_phys {
>   	struct drm_encoder *parent;
>   	struct dpu_encoder_phys_ops ops;
> -	const struct dpu_encoder_virt_ops *parent_ops;
>   	struct dpu_hw_mdp *hw_mdptop;
>   	struct dpu_hw_ctl *hw_ctl;
>   	struct dpu_hw_pingpong *hw_pp;
> @@ -283,7 +263,6 @@ struct dpu_encoder_phys_cmd {
>   struct dpu_enc_phys_init_params {
>   	struct dpu_kms *dpu_kms;
>   	struct drm_encoder *parent;
> -	const struct dpu_encoder_virt_ops *parent_ops;
>   	enum dpu_enc_split_role split_role;
>   	enum dpu_intf intf_idx;
>   	enum dpu_wb wb_idx;
> @@ -400,4 +379,30 @@ int dpu_encoder_helper_wait_for_irq(struct dpu_encoder_phys *phys_enc,
>    */
>   void dpu_encoder_helper_phys_cleanup(struct dpu_encoder_phys *phys_enc);
>   
> +/**
> + * dpu_encoder_vblank_callback - Notify virtual encoder of vblank IRQ reception
> + * @drm_enc:    Pointer to drm encoder structure
> + * @phys_enc:	Pointer to physical encoder
> + * Note: This is called from IRQ handler context.
> + */
> +void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc,
> +				 struct dpu_encoder_phys *phy_enc);
> +
> +/** dpu_encoder_underrun_callback - Notify virtual encoder of underrun IRQ reception
> + * @drm_enc:    Pointer to drm encoder structure
> + * @phys_enc:	Pointer to physical encoder
> + * Note: This is called from IRQ handler context.
> + */
> +void dpu_encoder_underrun_callback(struct drm_encoder *drm_enc,
> +				   struct dpu_encoder_phys *phy_enc);
> +
> +/** dpu_encoder_frame_done_callback -- Notify virtual encoder that this phys encoder completes last request frame
extra - before notify
Notify virtual encoder that this phys encoder has completed the last frame
> + * @drm_enc:    Pointer to drm encoder structure
> + * @phys_enc:	Pointer to physical encoder
> + * @event:	Event to process
> + */
> +void dpu_encoder_frame_done_callback(
> +		struct drm_encoder *drm_enc,
> +		struct dpu_encoder_phys *ready_phys, u32 event);
> +
>   #endif /* __dpu_encoder_phys_H__ */
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> index ae28b2b93e69..41bd7dd2b482 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> @@ -83,9 +83,7 @@ static void dpu_encoder_phys_cmd_pp_tx_done_irq(void *arg, int irq_idx)
>   
>   	DPU_ATRACE_BEGIN("pp_done_irq");
>   	/* notify all synchronous clients first, then asynchronous clients */
> -	if (phys_enc->parent_ops->handle_frame_done)
> -		phys_enc->parent_ops->handle_frame_done(phys_enc->parent,
> -				phys_enc, event);
> +	dpu_encoder_frame_done_callback(phys_enc->parent, phys_enc, event);
>   
>   	spin_lock_irqsave(phys_enc->enc_spinlock, lock_flags);
>   	new_cnt = atomic_add_unless(&phys_enc->pending_kickoff_cnt, -1, 0);
> @@ -111,9 +109,7 @@ static void dpu_encoder_phys_cmd_pp_rd_ptr_irq(void *arg, int irq_idx)
>   	DPU_ATRACE_BEGIN("rd_ptr_irq");
>   	cmd_enc = to_dpu_encoder_phys_cmd(phys_enc);
>   
> -	if (phys_enc->parent_ops->handle_vblank_virt)
> -		phys_enc->parent_ops->handle_vblank_virt(phys_enc->parent,
> -			phys_enc);
> +	dpu_encoder_vblank_callback(phys_enc->parent, phys_enc);
>   
>   	atomic_add_unless(&cmd_enc->pending_vblank_cnt, -1, 0);
>   	wake_up_all(&cmd_enc->pending_vblank_wq);
> @@ -137,9 +133,7 @@ static void dpu_encoder_phys_cmd_underrun_irq(void *arg, int irq_idx)
>   {
>   	struct dpu_encoder_phys *phys_enc = arg;
>   
> -	if (phys_enc->parent_ops->handle_underrun_virt)
> -		phys_enc->parent_ops->handle_underrun_virt(phys_enc->parent,
> -			phys_enc);
> +	dpu_encoder_underrun_callback(phys_enc->parent, phys_enc);
>   }
>   
>   static void dpu_encoder_phys_cmd_atomic_mode_set(
> @@ -202,9 +196,7 @@ static int _dpu_encoder_phys_cmd_handle_ppdone_timeout(
>   	/* request a ctl reset before the next kickoff */
>   	phys_enc->enable_state = DPU_ENC_ERR_NEEDS_HW_RESET;
>   
> -	if (phys_enc->parent_ops->handle_frame_done)
> -		phys_enc->parent_ops->handle_frame_done(
> -				drm_enc, phys_enc, frame_event);
> +	dpu_encoder_frame_done_callback(phys_enc->parent, phys_enc, frame_event);
>   
>   	return -ETIMEDOUT;
>   }
> @@ -780,7 +772,6 @@ struct dpu_encoder_phys *dpu_encoder_phys_cmd_init(
>   
>   	dpu_encoder_phys_cmd_init_ops(&phys_enc->ops);
>   	phys_enc->parent = p->parent;
> -	phys_enc->parent_ops = p->parent_ops;
>   	phys_enc->dpu_kms = p->dpu_kms;
>   	phys_enc->split_role = p->split_role;
>   	phys_enc->intf_mode = INTF_MODE_CMD;
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> index 0f71e8fe7be7..39ca1b305114 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> @@ -308,9 +308,7 @@ static void dpu_encoder_phys_vid_vblank_irq(void *arg, int irq_idx)
>   
>   	DPU_ATRACE_BEGIN("vblank_irq");
>   
> -	if (phys_enc->parent_ops->handle_vblank_virt)
> -		phys_enc->parent_ops->handle_vblank_virt(phys_enc->parent,
> -				phys_enc);
> +	dpu_encoder_vblank_callback(phys_enc->parent, phys_enc);
>   
>   	atomic_read(&phys_enc->pending_kickoff_cnt);
>   
> @@ -330,7 +328,7 @@ static void dpu_encoder_phys_vid_vblank_irq(void *arg, int irq_idx)
>   	/* Signal any waiting atomic commit thread */
>   	wake_up_all(&phys_enc->pending_kickoff_wq);
>   
> -	phys_enc->parent_ops->handle_frame_done(phys_enc->parent, phys_enc,
> +	dpu_encoder_frame_done_callback(phys_enc->parent, phys_enc,
>   			DPU_ENCODER_FRAME_EVENT_DONE);
>   
>   	DPU_ATRACE_END("vblank_irq");
> @@ -340,9 +338,7 @@ static void dpu_encoder_phys_vid_underrun_irq(void *arg, int irq_idx)
>   {
>   	struct dpu_encoder_phys *phys_enc = arg;
>   
> -	if (phys_enc->parent_ops->handle_underrun_virt)
> -		phys_enc->parent_ops->handle_underrun_virt(phys_enc->parent,
> -			phys_enc);
> +	dpu_encoder_underrun_callback(phys_enc->parent, phys_enc);
>   }
>   
>   static bool dpu_encoder_phys_vid_needs_single_flush(
> @@ -700,7 +696,6 @@ struct dpu_encoder_phys *dpu_encoder_phys_vid_init(
>   
>   	dpu_encoder_phys_vid_init_ops(&phys_enc->ops);
>   	phys_enc->parent = p->parent;
> -	phys_enc->parent_ops = p->parent_ops;
>   	phys_enc->dpu_kms = p->dpu_kms;
>   	phys_enc->split_role = p->split_role;
>   	phys_enc->intf_mode = INTF_MODE_VIDEO;
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> index 7cbcef6efe17..c5146b6477d6 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> @@ -365,13 +365,9 @@ static void _dpu_encoder_phys_wb_frame_done_helper(void *arg)
>   
>   	DPU_DEBUG("[wb:%d]\n", hw_wb->idx - WB_0);
>   
> -	if (phys_enc->parent_ops->handle_frame_done)
> -		phys_enc->parent_ops->handle_frame_done(phys_enc->parent,
> -				phys_enc, event);
> +	dpu_encoder_frame_done_callback(phys_enc->parent, phys_enc, event);
>   
> -	if (phys_enc->parent_ops->handle_vblank_virt)
> -		phys_enc->parent_ops->handle_vblank_virt(phys_enc->parent,
> -				phys_enc);
> +	dpu_encoder_vblank_callback(phys_enc->parent, phys_enc);
>   
>   	spin_lock_irqsave(phys_enc->enc_spinlock, lock_flags);
>   	atomic_add_unless(&phys_enc->pending_kickoff_cnt, -1, 0);
> @@ -441,9 +437,7 @@ static void _dpu_encoder_phys_wb_handle_wbdone_timeout(
>   	if (wb_enc->wb_conn)
>   		drm_writeback_signal_completion(wb_enc->wb_conn, 0);
>   
> -	if (phys_enc->parent_ops->handle_frame_done)
> -		phys_enc->parent_ops->handle_frame_done(
> -				phys_enc->parent, phys_enc, frame_event);
> +	dpu_encoder_frame_done_callback(phys_enc->parent, phys_enc, frame_event);
>   }
>   
>   /**
> @@ -723,7 +717,6 @@ struct dpu_encoder_phys *dpu_encoder_phys_wb_init(
>   
>   	dpu_encoder_phys_wb_init_ops(&phys_enc->ops);
>   	phys_enc->parent = p->parent;
> -	phys_enc->parent_ops = p->parent_ops;
>   	phys_enc->dpu_kms = p->dpu_kms;
>   	phys_enc->split_role = p->split_role;
>   	phys_enc->intf_mode = INTF_MODE_WB_LINE;

Re: [PATCH 2/2] drm/msm/dpu: remove CRTC frame event callback registration

[Abhinav Kumar]

On 1/2/2023 7:47 AM, Dmitry Baryshkov wrote:
> The frame event callback is always set to dpu_crtc_frame_event_cb() (or
> to NULL) and the data is always either the CRTC itself or NULL
> (correpondingly). Thus drop the event callback registration, call the
> dpu_crtc_frame_event_cb() directly and gate on the dpu_enc->crtc
> assigned using dpu_encoder_assign_crtc().

I suggest you wait till we sort out the PSR series for this, especially 
this patch https://patchwork.freedesktop.org/patch/515787/

There is going to be some change in this code when PSR is pushed again 
sometime early next week because PSR will touch the crtc assignment code 
(dpu_enc->crtc),

Based on how we all like that patch, we can get back to this one as this 
one is a minor cleanup.

> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 17 +--------
>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    | 14 +++++++
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 41 +++------------------
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 10 -----
>   drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h   |  4 --
>   5 files changed, 21 insertions(+), 65 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index 13ce321283ff..64cd2ec8a0c4 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -640,18 +640,8 @@ static void dpu_crtc_frame_event_work(struct kthread_work *work)
>   	DPU_ATRACE_END("crtc_frame_event");
>   }
>   
> -/*
> - * dpu_crtc_frame_event_cb - crtc frame event callback API. CRTC module
> - * registers this API to encoder for all frame event callbacks like
> - * frame_error, frame_done, idle_timeout, etc. Encoder may call different events
> - * from different context - IRQ, user thread, commit_thread, etc. Each event
> - * should be carefully reviewed and should be processed in proper task context
> - * to avoid schedulin delay or properly manage the irq context's bottom half
> - * processing.
> - */
> -static void dpu_crtc_frame_event_cb(void *data, u32 event)
> +void dpu_crtc_frame_event_cb(struct drm_crtc *crtc, u32 event)
>   {
> -	struct drm_crtc *crtc = (struct drm_crtc *)data;
>   	struct dpu_crtc *dpu_crtc;
>   	struct msm_drm_private *priv;
>   	struct dpu_crtc_frame_event *fevent;
> @@ -1051,9 +1041,6 @@ static void dpu_crtc_disable(struct drm_crtc *crtc,
>   
>   	dpu_core_perf_crtc_update(crtc, 0, true);
>   
> -	drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask)
> -		dpu_encoder_register_frame_event_callback(encoder, NULL, NULL);
> -
>   	memset(cstate->mixers, 0, sizeof(cstate->mixers));
>   	cstate->num_mixers = 0;
>   
> @@ -1089,8 +1076,6 @@ static void dpu_crtc_enable(struct drm_crtc *crtc,
>   		 */
>   		if (dpu_encoder_get_intf_mode(encoder) == INTF_MODE_VIDEO)
>   			request_bandwidth = true;
> -		dpu_encoder_register_frame_event_callback(encoder,
> -				dpu_crtc_frame_event_cb, (void *)crtc);
>   	}
>   
>   	if (request_bandwidth)
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> index 539b68b1626a..3aa536d95721 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> @@ -300,4 +300,18 @@ static inline enum dpu_crtc_client_type dpu_crtc_get_client_type(
>   	return crtc && crtc->state ? RT_CLIENT : NRT_CLIENT;
>   }
>   
> +/**
> + * dpu_crtc_frame_event_cb - crtc frame event callback API
> + * @crtc: Pointer to crtc
> + * @event: Event to process
> + *
> + * CRTC module registers this API to encoder for all frame event callbacks like
> + * frame_error, frame_done, idle_timeout, etc. Encoder may call different events
> + * from different context - IRQ, user thread, commit_thread, etc. Each event
> + * should be carefully reviewed and should be processed in proper task context
> + * to avoid schedulin delay or properly manage the irq context's bottom half
> + * processing.
> + */
> +void dpu_crtc_frame_event_cb(struct drm_crtc *crtc, u32 event);
> +
>   #endif /* _DPU_CRTC_H_ */
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 84f8c8a1b049..bf0af5af4306 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -147,8 +147,6 @@ enum dpu_enc_rc_states {
>    * @frame_busy_mask:		Bitmask tracking which phys_enc we are still
>    *				busy processing current command.
>    *				Bit0 = phys_encs[0] etc.
> - * @crtc_frame_event_cb:	callback handler for frame event
> - * @crtc_frame_event_cb_data:	callback handler private data
>    * @frame_done_timeout_ms:	frame done timeout in ms
>    * @frame_done_timer:		watchdog timer for frame done event
>    * @vsync_event_timer:		vsync timer
> @@ -187,8 +185,6 @@ struct dpu_encoder_virt {
>   	struct dentry *debugfs_root;
>   	struct mutex enc_lock;
>   	DECLARE_BITMAP(frame_busy_mask, MAX_PHYS_ENCODERS_PER_VIRTUAL);
> -	void (*crtc_frame_event_cb)(void *, u32 event);
> -	void *crtc_frame_event_cb_data;
>   
>   	atomic_t frame_done_timeout_ms;
>   	struct timer_list frame_done_timer;
> @@ -1358,28 +1354,6 @@ void dpu_encoder_toggle_vblank_for_crtc(struct drm_encoder *drm_enc,
>   	}
>   }
>   
> -void dpu_encoder_register_frame_event_callback(struct drm_encoder *drm_enc,
> -		void (*frame_event_cb)(void *, u32 event),
> -		void *frame_event_cb_data)
> -{
> -	struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
> -	unsigned long lock_flags;
> -	bool enable;
> -
> -	enable = frame_event_cb ? true : false;
> -
> -	if (!drm_enc) {
> -		DPU_ERROR("invalid encoder\n");
> -		return;
> -	}
> -	trace_dpu_enc_frame_event_cb(DRMID(drm_enc), enable);
> -
> -	spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags);
> -	dpu_enc->crtc_frame_event_cb = frame_event_cb;
> -	dpu_enc->crtc_frame_event_cb_data = frame_event_cb_data;
> -	spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags);
> -}
> -
>   void dpu_encoder_frame_done_callback(
>   		struct drm_encoder *drm_enc,
>   		struct dpu_encoder_phys *ready_phys, u32 event)
> @@ -1418,15 +1392,12 @@ void dpu_encoder_frame_done_callback(
>   			dpu_encoder_resource_control(drm_enc,
>   					DPU_ENC_RC_EVENT_FRAME_DONE);
>   
> -			if (dpu_enc->crtc_frame_event_cb)
> -				dpu_enc->crtc_frame_event_cb(
> -					dpu_enc->crtc_frame_event_cb_data,
> -					event);
> +			if (dpu_enc->crtc)
> +				dpu_crtc_frame_event_cb(dpu_enc->crtc, event);
>   		}
>   	} else {
> -		if (dpu_enc->crtc_frame_event_cb)
> -			dpu_enc->crtc_frame_event_cb(
> -				dpu_enc->crtc_frame_event_cb_data, event);
> +		if (dpu_enc->crtc)
> +			dpu_crtc_frame_event_cb(dpu_enc->crtc, event);
>   	}
>   }
>   
> @@ -2367,7 +2338,7 @@ static void dpu_encoder_frame_done_timeout(struct timer_list *t)
>   		return;
>   	}
>   
> -	if (!dpu_enc->frame_busy_mask[0] || !dpu_enc->crtc_frame_event_cb) {
> +	if (!dpu_enc->frame_busy_mask[0] || !dpu_enc->crtc) {
>   		DRM_DEBUG_KMS("id:%u invalid timeout frame_busy_mask=%lu\n",
>   			      DRMID(drm_enc), dpu_enc->frame_busy_mask[0]);
>   		return;
> @@ -2380,7 +2351,7 @@ static void dpu_encoder_frame_done_timeout(struct timer_list *t)
>   
>   	event = DPU_ENCODER_FRAME_EVENT_ERROR;
>   	trace_dpu_enc_frame_done_timeout(DRMID(drm_enc), event);
> -	dpu_enc->crtc_frame_event_cb(dpu_enc->crtc_frame_event_cb_data, event);
> +	dpu_crtc_frame_event_cb(dpu_enc->crtc, event);
>   }
>   
>   static const struct drm_encoder_helper_funcs dpu_encoder_helper_funcs = {
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> index 9e7236ef34e6..dd9cff4f1606 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> @@ -57,16 +57,6 @@ void dpu_encoder_assign_crtc(struct drm_encoder *encoder,
>   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.
> - * @encoder:	encoder pointer
> - * @cb:		callback pointer, provide NULL to deregister
> - * @data:	user data provided to callback
> - */
> -void dpu_encoder_register_frame_event_callback(struct drm_encoder *encoder,
> -		void (*cb)(void *, u32), void *data);
> -
>   /**
>    * dpu_encoder_prepare_for_kickoff - schedule double buffer flip of the ctl
>    *	path (i.e. ctl flush and start) at next appropriate time.
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
> index 76169f406505..c75b4363bfc6 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
> @@ -346,10 +346,6 @@ DEFINE_EVENT(dpu_enc_id_enable_template, dpu_enc_vblank_cb,
>   	TP_PROTO(uint32_t drm_id, bool enable),
>   	TP_ARGS(drm_id, enable)
>   );
> -DEFINE_EVENT(dpu_enc_id_enable_template, dpu_enc_frame_event_cb,
> -	TP_PROTO(uint32_t drm_id, bool enable),
> -	TP_ARGS(drm_id, enable)
> -);
>   DEFINE_EVENT(dpu_enc_id_enable_template, dpu_enc_phys_cmd_connect_te,
>   	TP_PROTO(uint32_t drm_id, bool enable),
>   	TP_ARGS(drm_id, enable)

Re: [PATCH 1/2] drm/msm/dpu: remove dpu_encoder_virt_ops

[Dmitry Baryshkov]

On Mon, 02 Jan 2023 17:47:47 +0200, Dmitry Baryshkov wrote:
> Struct dpu_encoder_virt_ops is used to provide several callbacks to the
> phys_enc backends. However these ops are static and are not supposed to
> change in the foreseeble future. Drop the indirection and call
> corresponding functions directly.
> 
> 

Applied, thanks!

[1/2] drm/msm/dpu: remove dpu_encoder_virt_ops
      https://gitlab.freedesktop.org/lumag/msm/-/commit/59f0182a291c

Best regards,
-- 
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Re: [PATCH 2/2] drm/msm/dpu: remove CRTC frame event callback registration

[Dmitry Baryshkov]

On 14/01/2023 02:49, Abhinav Kumar wrote:
> 
> 
> On 1/2/2023 7:47 AM, Dmitry Baryshkov wrote:
>> The frame event callback is always set to dpu_crtc_frame_event_cb() (or
>> to NULL) and the data is always either the CRTC itself or NULL
>> (correpondingly). Thus drop the event callback registration, call the
>> dpu_crtc_frame_event_cb() directly and gate on the dpu_enc->crtc
>> assigned using dpu_encoder_assign_crtc().
> 
> I suggest you wait till we sort out the PSR series for this, especially 
> this patch https://patchwork.freedesktop.org/patch/515787/
> 
> There is going to be some change in this code when PSR is pushed again 
> sometime early next week because PSR will touch the crtc assignment code 
> (dpu_enc->crtc),
> 
> Based on how we all like that patch, we can get back to this one as this 
> one is a minor cleanup.

As the PSR series have landed, I'd like to point to this patch again.

> 
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 17 +--------
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    | 14 +++++++
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 41 +++------------------
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 10 -----
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h   |  4 --
>>   5 files changed, 21 insertions(+), 65 deletions(-)


-- 
With best wishes
Dmitry