[PATCH 0/3] drm/msm/dpu: remove dpu_encoder_phys_ops::atomic_mode_set callback

[PATCH 0/3] drm/msm/dpu: remove dpu_encoder_phys_ops::atomic_mode_set callback

[Dmitry Baryshkov]

The dpu_encoder_phys_ops::atomic_mode_set() callback is mostly
redundant. Implementations only set the IRQ indices there. Move
statically allocated IRQs to dpu_encoder_phys_*_init() and set
dynamically allocated IRQs in the irq_enable() callback.

Dmitry Baryshkov (3):
  drm/msm/dpu: split irq_control into irq_enable and _disable
  drm/msm/dpu: split _dpu_encoder_resource_control_helper()
  drm/msm/dpu: drop dpu_encoder_phys_ops.atomic_mode_set

 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   | 79 +++++++++++-----
 .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  | 11 +--
 .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  | 93 ++++++++++---------
 .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  | 61 ++++++------
 .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c   | 34 +++----
 5 files changed, 152 insertions(+), 126 deletions(-)

-- 
2.39.2

[PATCH 1/3] drm/msm/dpu: split irq_control into irq_enable and _disable

[Dmitry Baryshkov]

The single helper for both enable and disable cases is too complicated,
especially if we start adding more code to these helpers. Split it into
irq_enable and irq_disable cases.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   | 36 ++++++++---
 .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |  6 +-
 .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  | 63 ++++++++++---------
 .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  | 48 +++++++-------
 .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c   | 29 ++++++---
 5 files changed, 112 insertions(+), 70 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 2e1873d29c4b..7c131c5cbe71 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -717,7 +717,7 @@ static void _dpu_encoder_update_vsync_source(struct dpu_encoder_virt *dpu_enc,
 	}
 }
 
-static void _dpu_encoder_irq_control(struct drm_encoder *drm_enc, bool enable)
+static void _dpu_encoder_irq_enable(struct drm_encoder *drm_enc)
 {
 	struct dpu_encoder_virt *dpu_enc;
 	int i;
@@ -729,14 +729,32 @@ static void _dpu_encoder_irq_control(struct drm_encoder *drm_enc, bool enable)
 
 	dpu_enc = to_dpu_encoder_virt(drm_enc);
 
-	DPU_DEBUG_ENC(dpu_enc, "enable:%d\n", enable);
+	DPU_DEBUG_ENC(dpu_enc, "\n");
 	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
 		struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
 
-		if (phys->ops.irq_control)
-			phys->ops.irq_control(phys, enable);
+		phys->ops.irq_enable(phys);
+	}
+}
+
+static void _dpu_encoder_irq_disable(struct drm_encoder *drm_enc)
+{
+	struct dpu_encoder_virt *dpu_enc;
+	int i;
+
+	if (!drm_enc) {
+		DPU_ERROR("invalid encoder\n");
+		return;
 	}
 
+	dpu_enc = to_dpu_encoder_virt(drm_enc);
+
+	DPU_DEBUG_ENC(dpu_enc, "\n");
+	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
+		struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
+
+		phys->ops.irq_disable(phys);
+	}
 }
 
 static void _dpu_encoder_resource_control_helper(struct drm_encoder *drm_enc,
@@ -762,11 +780,11 @@ static void _dpu_encoder_resource_control_helper(struct drm_encoder *drm_enc,
 		pm_runtime_get_sync(&dpu_kms->pdev->dev);
 
 		/* enable all the irq */
-		_dpu_encoder_irq_control(drm_enc, true);
+		_dpu_encoder_irq_enable(drm_enc);
 
 	} else {
 		/* disable all the irq */
-		_dpu_encoder_irq_control(drm_enc, false);
+		_dpu_encoder_irq_disable(drm_enc);
 
 		/* disable DPU core clks */
 		pm_runtime_put_sync(&dpu_kms->pdev->dev);
@@ -827,7 +845,7 @@ static int dpu_encoder_resource_control(struct drm_encoder *drm_enc,
 		}
 
 		if (is_vid_mode && dpu_enc->rc_state == DPU_ENC_RC_STATE_IDLE)
-			_dpu_encoder_irq_control(drm_enc, true);
+			_dpu_encoder_irq_enable(drm_enc);
 		else
 			_dpu_encoder_resource_control_helper(drm_enc, true);
 
@@ -882,7 +900,7 @@ static int dpu_encoder_resource_control(struct drm_encoder *drm_enc,
 
 		if (is_vid_mode &&
 			  dpu_enc->rc_state == DPU_ENC_RC_STATE_IDLE) {
-			_dpu_encoder_irq_control(drm_enc, true);
+			_dpu_encoder_irq_enable(drm_enc);
 		}
 		/* skip if is already OFF or IDLE, resources are off already */
 		else if (dpu_enc->rc_state == DPU_ENC_RC_STATE_OFF ||
@@ -957,7 +975,7 @@ static int dpu_encoder_resource_control(struct drm_encoder *drm_enc,
 		}
 
 		if (is_vid_mode)
-			_dpu_encoder_irq_control(drm_enc, false);
+			_dpu_encoder_irq_disable(drm_enc);
 		else
 			_dpu_encoder_resource_control_helper(drm_enc, false);
 
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 d48558ede488..faf033cd086e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
@@ -84,7 +84,8 @@ struct dpu_encoder_phys;
  * @handle_post_kickoff:	Do any work necessary post-kickoff work
  * @trigger_start:		Process start event on physical encoder
  * @needs_single_flush:		Whether encoder slaves need to be flushed
- * @irq_control:		Handler to enable/disable all the encoder IRQs
+ * @irq_enable:			Handler to enable all the encoder IRQs
+ * @irq_disable:		Handler to disable all the encoder IRQs
  * @prepare_idle_pc:		phys encoder can update the vsync_enable status
  *                              on idle power collapse prepare
  * @restore:			Restore all the encoder configs.
@@ -111,7 +112,8 @@ struct dpu_encoder_phys_ops {
 	void (*handle_post_kickoff)(struct dpu_encoder_phys *phys_enc);
 	void (*trigger_start)(struct dpu_encoder_phys *phys_enc);
 	bool (*needs_single_flush)(struct dpu_encoder_phys *phys_enc);
-	void (*irq_control)(struct dpu_encoder_phys *phys, bool enable);
+	void (*irq_enable)(struct dpu_encoder_phys *phys);
+	void (*irq_disable)(struct dpu_encoder_phys *phys);
 	void (*prepare_idle_pc)(struct dpu_encoder_phys *phys_enc);
 	void (*restore)(struct dpu_encoder_phys *phys);
 	int (*get_line_count)(struct dpu_encoder_phys *phys);
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 4f8c9187f76d..3422b49f23c2 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
@@ -280,40 +280,44 @@ static int dpu_encoder_phys_cmd_control_vblank_irq(
 	return ret;
 }
 
-static void dpu_encoder_phys_cmd_irq_control(struct dpu_encoder_phys *phys_enc,
-		bool enable)
+static void dpu_encoder_phys_cmd_irq_enable(struct dpu_encoder_phys *phys_enc)
 {
 	trace_dpu_enc_phys_cmd_irq_ctrl(DRMID(phys_enc->parent),
-			phys_enc->hw_pp->idx - PINGPONG_0,
-			enable, atomic_read(&phys_enc->vblank_refcount));
-
-	if (enable) {
+					phys_enc->hw_pp->idx - PINGPONG_0,
+					true,
+					atomic_read(&phys_enc->vblank_refcount));
+
+	dpu_core_irq_register_callback(phys_enc->dpu_kms,
+				       phys_enc->irq[INTR_IDX_PINGPONG],
+				       dpu_encoder_phys_cmd_pp_tx_done_irq,
+				       phys_enc);
+	dpu_core_irq_register_callback(phys_enc->dpu_kms,
+				       phys_enc->irq[INTR_IDX_UNDERRUN],
+				       dpu_encoder_phys_cmd_underrun_irq,
+				       phys_enc);
+	dpu_encoder_phys_cmd_control_vblank_irq(phys_enc, true);
+
+	if (dpu_encoder_phys_cmd_is_master(phys_enc))
 		dpu_core_irq_register_callback(phys_enc->dpu_kms,
-				phys_enc->irq[INTR_IDX_PINGPONG],
-				dpu_encoder_phys_cmd_pp_tx_done_irq,
-				phys_enc);
-		dpu_core_irq_register_callback(phys_enc->dpu_kms,
-				phys_enc->irq[INTR_IDX_UNDERRUN],
-				dpu_encoder_phys_cmd_underrun_irq,
-				phys_enc);
-		dpu_encoder_phys_cmd_control_vblank_irq(phys_enc, true);
+					       phys_enc->irq[INTR_IDX_CTL_START],
+					       dpu_encoder_phys_cmd_ctl_start_irq,
+					       phys_enc);
+}
 
-		if (dpu_encoder_phys_cmd_is_master(phys_enc))
-			dpu_core_irq_register_callback(phys_enc->dpu_kms,
-					phys_enc->irq[INTR_IDX_CTL_START],
-					dpu_encoder_phys_cmd_ctl_start_irq,
-					phys_enc);
-	} else {
-		if (dpu_encoder_phys_cmd_is_master(phys_enc))
-			dpu_core_irq_unregister_callback(phys_enc->dpu_kms,
-					phys_enc->irq[INTR_IDX_CTL_START]);
+static void dpu_encoder_phys_cmd_irq_disable(struct dpu_encoder_phys *phys_enc)
+{
+	trace_dpu_enc_phys_cmd_irq_ctrl(DRMID(phys_enc->parent),
+			phys_enc->hw_pp->idx - PINGPONG_0,
+			false,
+			atomic_read(&phys_enc->vblank_refcount));
 
+	if (dpu_encoder_phys_cmd_is_master(phys_enc))
 		dpu_core_irq_unregister_callback(phys_enc->dpu_kms,
-				phys_enc->irq[INTR_IDX_UNDERRUN]);
-		dpu_encoder_phys_cmd_control_vblank_irq(phys_enc, false);
-		dpu_core_irq_unregister_callback(phys_enc->dpu_kms,
-				phys_enc->irq[INTR_IDX_PINGPONG]);
-	}
+				phys_enc->irq[INTR_IDX_CTL_START]);
+
+	dpu_core_irq_unregister_callback(phys_enc->dpu_kms, phys_enc->irq[INTR_IDX_UNDERRUN]);
+	dpu_encoder_phys_cmd_control_vblank_irq(phys_enc, false);
+	dpu_core_irq_unregister_callback(phys_enc->dpu_kms, phys_enc->irq[INTR_IDX_PINGPONG]);
 }
 
 static void dpu_encoder_phys_cmd_tearcheck_config(
@@ -744,7 +748,8 @@ static void dpu_encoder_phys_cmd_init_ops(
 	ops->wait_for_vblank = dpu_encoder_phys_cmd_wait_for_vblank;
 	ops->trigger_start = dpu_encoder_phys_cmd_trigger_start;
 	ops->needs_single_flush = dpu_encoder_phys_cmd_needs_single_flush;
-	ops->irq_control = dpu_encoder_phys_cmd_irq_control;
+	ops->irq_enable = dpu_encoder_phys_cmd_irq_enable;
+	ops->irq_disable = dpu_encoder_phys_cmd_irq_disable;
 	ops->restore = dpu_encoder_phys_cmd_enable_helper;
 	ops->prepare_idle_pc = dpu_encoder_phys_cmd_prepare_idle_pc;
 	ops->handle_post_kickoff = dpu_encoder_phys_cmd_handle_post_kickoff;
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 e26629e9e303..a550b290246c 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
@@ -611,30 +611,35 @@ static void dpu_encoder_phys_vid_handle_post_kickoff(
 	}
 }
 
-static void dpu_encoder_phys_vid_irq_control(struct dpu_encoder_phys *phys_enc,
-		bool enable)
+static void dpu_encoder_phys_vid_irq_enable(struct dpu_encoder_phys *phys_enc)
 {
 	int ret;
 
 	trace_dpu_enc_phys_vid_irq_ctrl(DRMID(phys_enc->parent),
-			    phys_enc->hw_intf->idx - INTF_0,
-			    enable,
-			    atomic_read(&phys_enc->vblank_refcount));
-
-	if (enable) {
-		ret = dpu_encoder_phys_vid_control_vblank_irq(phys_enc, true);
-		if (WARN_ON(ret))
-			return;
-
-		dpu_core_irq_register_callback(phys_enc->dpu_kms,
-				phys_enc->irq[INTR_IDX_UNDERRUN],
-				dpu_encoder_phys_vid_underrun_irq,
-				phys_enc);
-	} else {
-		dpu_encoder_phys_vid_control_vblank_irq(phys_enc, false);
-		dpu_core_irq_unregister_callback(phys_enc->dpu_kms,
-				phys_enc->irq[INTR_IDX_UNDERRUN]);
-	}
+					phys_enc->hw_intf->idx - INTF_0,
+					true,
+					atomic_read(&phys_enc->vblank_refcount));
+
+	ret = dpu_encoder_phys_vid_control_vblank_irq(phys_enc, true);
+	if (WARN_ON(ret))
+		return;
+
+	dpu_core_irq_register_callback(phys_enc->dpu_kms,
+				       phys_enc->irq[INTR_IDX_UNDERRUN],
+				       dpu_encoder_phys_vid_underrun_irq,
+				       phys_enc);
+}
+
+static void dpu_encoder_phys_vid_irq_disable(struct dpu_encoder_phys *phys_enc)
+{
+	trace_dpu_enc_phys_vid_irq_ctrl(DRMID(phys_enc->parent),
+					phys_enc->hw_intf->idx - INTF_0,
+					false,
+					atomic_read(&phys_enc->vblank_refcount));
+
+	dpu_encoder_phys_vid_control_vblank_irq(phys_enc, false);
+	dpu_core_irq_unregister_callback(phys_enc->dpu_kms,
+					 phys_enc->irq[INTR_IDX_UNDERRUN]);
 }
 
 static int dpu_encoder_phys_vid_get_line_count(
@@ -687,7 +692,8 @@ static void dpu_encoder_phys_vid_init_ops(struct dpu_encoder_phys_ops *ops)
 	ops->wait_for_commit_done = dpu_encoder_phys_vid_wait_for_commit_done;
 	ops->wait_for_vblank = dpu_encoder_phys_vid_wait_for_vblank;
 	ops->wait_for_tx_complete = dpu_encoder_phys_vid_wait_for_vblank;
-	ops->irq_control = dpu_encoder_phys_vid_irq_control;
+	ops->irq_enable = dpu_encoder_phys_vid_irq_enable;
+	ops->irq_disable = dpu_encoder_phys_vid_irq_disable;
 	ops->prepare_for_kickoff = dpu_encoder_phys_vid_prepare_for_kickoff;
 	ops->handle_post_kickoff = dpu_encoder_phys_vid_handle_post_kickoff;
 	ops->needs_single_flush = dpu_encoder_phys_vid_needs_single_flush;
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 e9325cafb1a8..858fe6656c9b 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
@@ -382,21 +382,31 @@ static void dpu_encoder_phys_wb_done_irq(void *arg, int irq_idx)
 }
 
 /**
- * dpu_encoder_phys_wb_irq_ctrl - irq control of WB
+ * dpu_encoder_phys_wb_irq_enable - irq control of WB
  * @phys:	Pointer to physical encoder
- * @enable:	indicates enable or disable interrupts
  */
-static void dpu_encoder_phys_wb_irq_ctrl(
-		struct dpu_encoder_phys *phys, bool enable)
+static void dpu_encoder_phys_wb_irq_enable(struct dpu_encoder_phys *phys)
 {
 
 	struct dpu_encoder_phys_wb *wb_enc = to_dpu_encoder_phys_wb(phys);
 
-	if (enable && atomic_inc_return(&wb_enc->wbirq_refcount) == 1)
+	if (atomic_inc_return(&wb_enc->wbirq_refcount) == 1)
 		dpu_core_irq_register_callback(phys->dpu_kms,
-				phys->irq[INTR_IDX_WB_DONE], dpu_encoder_phys_wb_done_irq, phys);
-	else if (!enable &&
-			atomic_dec_return(&wb_enc->wbirq_refcount) == 0)
+					       phys->irq[INTR_IDX_WB_DONE],
+					       dpu_encoder_phys_wb_done_irq,
+					       phys);
+}
+
+/**
+ * dpu_encoder_phys_wb_irq_disable - irq control of WB
+ * @phys:	Pointer to physical encoder
+ */
+static void dpu_encoder_phys_wb_irq_disable(struct dpu_encoder_phys *phys)
+{
+
+	struct dpu_encoder_phys_wb *wb_enc = to_dpu_encoder_phys_wb(phys);
+
+	if (atomic_dec_return(&wb_enc->wbirq_refcount) == 0)
 		dpu_core_irq_unregister_callback(phys->dpu_kms, phys->irq[INTR_IDX_WB_DONE]);
 }
 
@@ -670,7 +680,8 @@ static void dpu_encoder_phys_wb_init_ops(struct dpu_encoder_phys_ops *ops)
 	ops->trigger_start = dpu_encoder_helper_trigger_start;
 	ops->prepare_wb_job = dpu_encoder_phys_wb_prepare_wb_job;
 	ops->cleanup_wb_job = dpu_encoder_phys_wb_cleanup_wb_job;
-	ops->irq_control = dpu_encoder_phys_wb_irq_ctrl;
+	ops->irq_enable = dpu_encoder_phys_wb_irq_enable;
+	ops->irq_disable = dpu_encoder_phys_wb_irq_disable;
 	ops->is_valid_for_commit = dpu_encoder_phys_wb_is_valid_for_commit;
 
 }
-- 
2.39.2

Re: [PATCH 1/3] drm/msm/dpu: split irq_control into irq_enable and _disable

[Abhinav Kumar]

On 6/4/2023 7:45 AM, Dmitry Baryshkov wrote:
> The single helper for both enable and disable cases is too complicated,
> especially if we start adding more code to these helpers. Split it into
> irq_enable and irq_disable cases.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   | 36 ++++++++---
>   .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |  6 +-
>   .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  | 63 ++++++++++---------
>   .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  | 48 +++++++-------
>   .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c   | 29 ++++++---
>   5 files changed, 112 insertions(+), 70 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 2e1873d29c4b..7c131c5cbe71 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -717,7 +717,7 @@ static void _dpu_encoder_update_vsync_source(struct dpu_encoder_virt *dpu_enc,
>   	}
>   }
>   
> -static void _dpu_encoder_irq_control(struct drm_encoder *drm_enc, bool enable)
> +static void _dpu_encoder_irq_enable(struct drm_encoder *drm_enc)
>   {
>   	struct dpu_encoder_virt *dpu_enc;
>   	int i;
> @@ -729,14 +729,32 @@ static void _dpu_encoder_irq_control(struct drm_encoder *drm_enc, bool enable)
>   
>   	dpu_enc = to_dpu_encoder_virt(drm_enc);
>   
> -	DPU_DEBUG_ENC(dpu_enc, "enable:%d\n", enable);
> +	DPU_DEBUG_ENC(dpu_enc, "\n");
>   	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
>   		struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
>   
> -		if (phys->ops.irq_control)
> -			phys->ops.irq_control(phys, enable);
> +		phys->ops.irq_enable(phys);
> +	}
> +}
> +
> +static void _dpu_encoder_irq_disable(struct drm_encoder *drm_enc)
> +{
> +	struct dpu_encoder_virt *dpu_enc;
> +	int i;
> +
> +	if (!drm_enc) {
> +		DPU_ERROR("invalid encoder\n");
> +		return;
>   	}
>   
> +	dpu_enc = to_dpu_encoder_virt(drm_enc);
> +
> +	DPU_DEBUG_ENC(dpu_enc, "\n");
> +	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> +		struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
> +
> +		phys->ops.irq_disable(phys);
> +	}
>   }
>   
>   static void _dpu_encoder_resource_control_helper(struct drm_encoder *drm_enc,
> @@ -762,11 +780,11 @@ static void _dpu_encoder_resource_control_helper(struct drm_encoder *drm_enc,
>   		pm_runtime_get_sync(&dpu_kms->pdev->dev);
>   
>   		/* enable all the irq */
> -		_dpu_encoder_irq_control(drm_enc, true);
> +		_dpu_encoder_irq_enable(drm_enc);
>   
>   	} else {
>   		/* disable all the irq */
> -		_dpu_encoder_irq_control(drm_enc, false);
> +		_dpu_encoder_irq_disable(drm_enc);
>   
>   		/* disable DPU core clks */
>   		pm_runtime_put_sync(&dpu_kms->pdev->dev);
> @@ -827,7 +845,7 @@ static int dpu_encoder_resource_control(struct drm_encoder *drm_enc,
>   		}
>   
>   		if (is_vid_mode && dpu_enc->rc_state == DPU_ENC_RC_STATE_IDLE)
> -			_dpu_encoder_irq_control(drm_enc, true);
> +			_dpu_encoder_irq_enable(drm_enc);
>   		else
>   			_dpu_encoder_resource_control_helper(drm_enc, true);
>   
> @@ -882,7 +900,7 @@ static int dpu_encoder_resource_control(struct drm_encoder *drm_enc,
>   
>   		if (is_vid_mode &&
>   			  dpu_enc->rc_state == DPU_ENC_RC_STATE_IDLE) {
> -			_dpu_encoder_irq_control(drm_enc, true);
> +			_dpu_encoder_irq_enable(drm_enc);
>   		}
>   		/* skip if is already OFF or IDLE, resources are off already */
>   		else if (dpu_enc->rc_state == DPU_ENC_RC_STATE_OFF ||
> @@ -957,7 +975,7 @@ static int dpu_encoder_resource_control(struct drm_encoder *drm_enc,
>   		}
>   
>   		if (is_vid_mode)
> -			_dpu_encoder_irq_control(drm_enc, false);
> +			_dpu_encoder_irq_disable(drm_enc);
>   		else
>   			_dpu_encoder_resource_control_helper(drm_enc, false);
>   
> 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 d48558ede488..faf033cd086e 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> @@ -84,7 +84,8 @@ struct dpu_encoder_phys;
>    * @handle_post_kickoff:	Do any work necessary post-kickoff work
>    * @trigger_start:		Process start event on physical encoder
>    * @needs_single_flush:		Whether encoder slaves need to be flushed
> - * @irq_control:		Handler to enable/disable all the encoder IRQs
> + * @irq_enable:			Handler to enable all the encoder IRQs
> + * @irq_disable:		Handler to disable all the encoder IRQs
>    * @prepare_idle_pc:		phys encoder can update the vsync_enable status
>    *                              on idle power collapse prepare
>    * @restore:			Restore all the encoder configs.
> @@ -111,7 +112,8 @@ struct dpu_encoder_phys_ops {
>   	void (*handle_post_kickoff)(struct dpu_encoder_phys *phys_enc);
>   	void (*trigger_start)(struct dpu_encoder_phys *phys_enc);
>   	bool (*needs_single_flush)(struct dpu_encoder_phys *phys_enc);
> -	void (*irq_control)(struct dpu_encoder_phys *phys, bool enable);
> +	void (*irq_enable)(struct dpu_encoder_phys *phys);
> +	void (*irq_disable)(struct dpu_encoder_phys *phys);
>   	void (*prepare_idle_pc)(struct dpu_encoder_phys *phys_enc);
>   	void (*restore)(struct dpu_encoder_phys *phys);
>   	int (*get_line_count)(struct dpu_encoder_phys *phys);
> 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 4f8c9187f76d..3422b49f23c2 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
> @@ -280,40 +280,44 @@ static int dpu_encoder_phys_cmd_control_vblank_irq(
>   	return ret;
>   }
>   
> -static void dpu_encoder_phys_cmd_irq_control(struct dpu_encoder_phys *phys_enc,
> -		bool enable)
> +static void dpu_encoder_phys_cmd_irq_enable(struct dpu_encoder_phys *phys_enc)
>   {
>   	trace_dpu_enc_phys_cmd_irq_ctrl(DRMID(phys_enc->parent),
> -			phys_enc->hw_pp->idx - PINGPONG_0,
> -			enable, atomic_read(&phys_enc->vblank_refcount));

Was it intentional to re-use this trace and not add a new one for 
dpu_encoder_phys_cmd_irq_enable/dpu_encoder_phys_cmd_irq_disable?

Just thinking if the trace names Vs function names not matching would 
become confusing.

> -
> -	if (enable) {
> +					phys_enc->hw_pp->idx - PINGPONG_0,
> +					true,
> +					atomic_read(&phys_enc->vblank_refcount));
> +
> +	dpu_core_irq_register_callback(phys_enc->dpu_kms,
> +				       phys_enc->irq[INTR_IDX_PINGPONG],
> +				       dpu_encoder_phys_cmd_pp_tx_done_irq,
> +				       phys_enc);
> +	dpu_core_irq_register_callback(phys_enc->dpu_kms,
> +				       phys_enc->irq[INTR_IDX_UNDERRUN],
> +				       dpu_encoder_phys_cmd_underrun_irq,
> +				       phys_enc);
> +	dpu_encoder_phys_cmd_control_vblank_irq(phys_enc, true);
> +
> +	if (dpu_encoder_phys_cmd_is_master(phys_enc))
>   		dpu_core_irq_register_callback(phys_enc->dpu_kms,
> -				phys_enc->irq[INTR_IDX_PINGPONG],
> -				dpu_encoder_phys_cmd_pp_tx_done_irq,
> -				phys_enc);
> -		dpu_core_irq_register_callback(phys_enc->dpu_kms,
> -				phys_enc->irq[INTR_IDX_UNDERRUN],
> -				dpu_encoder_phys_cmd_underrun_irq,
> -				phys_enc);
> -		dpu_encoder_phys_cmd_control_vblank_irq(phys_enc, true);
> +					       phys_enc->irq[INTR_IDX_CTL_START],
> +					       dpu_encoder_phys_cmd_ctl_start_irq,
> +					       phys_enc);
> +}
>   
> -		if (dpu_encoder_phys_cmd_is_master(phys_enc))
> -			dpu_core_irq_register_callback(phys_enc->dpu_kms,
> -					phys_enc->irq[INTR_IDX_CTL_START],
> -					dpu_encoder_phys_cmd_ctl_start_irq,
> -					phys_enc);
> -	} else {
> -		if (dpu_encoder_phys_cmd_is_master(phys_enc))
> -			dpu_core_irq_unregister_callback(phys_enc->dpu_kms,
> -					phys_enc->irq[INTR_IDX_CTL_START]);
> +static void dpu_encoder_phys_cmd_irq_disable(struct dpu_encoder_phys *phys_enc)
> +{
> +	trace_dpu_enc_phys_cmd_irq_ctrl(DRMID(phys_enc->parent),
> +			phys_enc->hw_pp->idx - PINGPONG_0,
> +			false,
> +			atomic_read(&phys_enc->vblank_refcount));
>   
> +	if (dpu_encoder_phys_cmd_is_master(phys_enc))
>   		dpu_core_irq_unregister_callback(phys_enc->dpu_kms,
> -				phys_enc->irq[INTR_IDX_UNDERRUN]);
> -		dpu_encoder_phys_cmd_control_vblank_irq(phys_enc, false);
> -		dpu_core_irq_unregister_callback(phys_enc->dpu_kms,
> -				phys_enc->irq[INTR_IDX_PINGPONG]);
> -	}
> +				phys_enc->irq[INTR_IDX_CTL_START]);
> +
> +	dpu_core_irq_unregister_callback(phys_enc->dpu_kms, phys_enc->irq[INTR_IDX_UNDERRUN]);
> +	dpu_encoder_phys_cmd_control_vblank_irq(phys_enc, false);
> +	dpu_core_irq_unregister_callback(phys_enc->dpu_kms, phys_enc->irq[INTR_IDX_PINGPONG]);
>   }
>   
>   static void dpu_encoder_phys_cmd_tearcheck_config(
> @@ -744,7 +748,8 @@ static void dpu_encoder_phys_cmd_init_ops(
>   	ops->wait_for_vblank = dpu_encoder_phys_cmd_wait_for_vblank;
>   	ops->trigger_start = dpu_encoder_phys_cmd_trigger_start;
>   	ops->needs_single_flush = dpu_encoder_phys_cmd_needs_single_flush;
> -	ops->irq_control = dpu_encoder_phys_cmd_irq_control;
> +	ops->irq_enable = dpu_encoder_phys_cmd_irq_enable;
> +	ops->irq_disable = dpu_encoder_phys_cmd_irq_disable;
>   	ops->restore = dpu_encoder_phys_cmd_enable_helper;
>   	ops->prepare_idle_pc = dpu_encoder_phys_cmd_prepare_idle_pc;
>   	ops->handle_post_kickoff = dpu_encoder_phys_cmd_handle_post_kickoff;
> 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 e26629e9e303..a550b290246c 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
> @@ -611,30 +611,35 @@ static void dpu_encoder_phys_vid_handle_post_kickoff(
>   	}
>   }
>   
> -static void dpu_encoder_phys_vid_irq_control(struct dpu_encoder_phys *phys_enc,
> -		bool enable)
> +static void dpu_encoder_phys_vid_irq_enable(struct dpu_encoder_phys *phys_enc)
>   {
>   	int ret;
>   
>   	trace_dpu_enc_phys_vid_irq_ctrl(DRMID(phys_enc->parent),
> -			    phys_enc->hw_intf->idx - INTF_0,
> -			    enable,
> -			    atomic_read(&phys_enc->vblank_refcount));
> -
> -	if (enable) {
> -		ret = dpu_encoder_phys_vid_control_vblank_irq(phys_enc, true);
> -		if (WARN_ON(ret))
> -			return;
> -
> -		dpu_core_irq_register_callback(phys_enc->dpu_kms,
> -				phys_enc->irq[INTR_IDX_UNDERRUN],
> -				dpu_encoder_phys_vid_underrun_irq,
> -				phys_enc);
> -	} else {
> -		dpu_encoder_phys_vid_control_vblank_irq(phys_enc, false);
> -		dpu_core_irq_unregister_callback(phys_enc->dpu_kms,
> -				phys_enc->irq[INTR_IDX_UNDERRUN]);
> -	}
> +					phys_enc->hw_intf->idx - INTF_0,
> +					true,
> +					atomic_read(&phys_enc->vblank_refcount));
> +
> +	ret = dpu_encoder_phys_vid_control_vblank_irq(phys_enc, true);
> +	if (WARN_ON(ret))
> +		return;
> +
> +	dpu_core_irq_register_callback(phys_enc->dpu_kms,
> +				       phys_enc->irq[INTR_IDX_UNDERRUN],
> +				       dpu_encoder_phys_vid_underrun_irq,
> +				       phys_enc);
> +}
> +
> +static void dpu_encoder_phys_vid_irq_disable(struct dpu_encoder_phys *phys_enc)
> +{
> +	trace_dpu_enc_phys_vid_irq_ctrl(DRMID(phys_enc->parent),
> +					phys_enc->hw_intf->idx - INTF_0,
> +					false,
> +					atomic_read(&phys_enc->vblank_refcount));
> +
> +	dpu_encoder_phys_vid_control_vblank_irq(phys_enc, false);
> +	dpu_core_irq_unregister_callback(phys_enc->dpu_kms,
> +					 phys_enc->irq[INTR_IDX_UNDERRUN]);
>   }
>   
>   static int dpu_encoder_phys_vid_get_line_count(
> @@ -687,7 +692,8 @@ static void dpu_encoder_phys_vid_init_ops(struct dpu_encoder_phys_ops *ops)
>   	ops->wait_for_commit_done = dpu_encoder_phys_vid_wait_for_commit_done;
>   	ops->wait_for_vblank = dpu_encoder_phys_vid_wait_for_vblank;
>   	ops->wait_for_tx_complete = dpu_encoder_phys_vid_wait_for_vblank;
> -	ops->irq_control = dpu_encoder_phys_vid_irq_control;
> +	ops->irq_enable = dpu_encoder_phys_vid_irq_enable;
> +	ops->irq_disable = dpu_encoder_phys_vid_irq_disable;
>   	ops->prepare_for_kickoff = dpu_encoder_phys_vid_prepare_for_kickoff;
>   	ops->handle_post_kickoff = dpu_encoder_phys_vid_handle_post_kickoff;
>   	ops->needs_single_flush = dpu_encoder_phys_vid_needs_single_flush;
> 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 e9325cafb1a8..858fe6656c9b 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
> @@ -382,21 +382,31 @@ static void dpu_encoder_phys_wb_done_irq(void *arg, int irq_idx)
>   }
>   
>   /**
> - * dpu_encoder_phys_wb_irq_ctrl - irq control of WB
> + * dpu_encoder_phys_wb_irq_enable - irq control of WB
>    * @phys:	Pointer to physical encoder
> - * @enable:	indicates enable or disable interrupts
>    */
> -static void dpu_encoder_phys_wb_irq_ctrl(
> -		struct dpu_encoder_phys *phys, bool enable)
> +static void dpu_encoder_phys_wb_irq_enable(struct dpu_encoder_phys *phys)
>   {
>   
>   	struct dpu_encoder_phys_wb *wb_enc = to_dpu_encoder_phys_wb(phys);
>   
> -	if (enable && atomic_inc_return(&wb_enc->wbirq_refcount) == 1)
> +	if (atomic_inc_return(&wb_enc->wbirq_refcount) == 1)
>   		dpu_core_irq_register_callback(phys->dpu_kms,
> -				phys->irq[INTR_IDX_WB_DONE], dpu_encoder_phys_wb_done_irq, phys);
> -	else if (!enable &&
> -			atomic_dec_return(&wb_enc->wbirq_refcount) == 0)
> +					       phys->irq[INTR_IDX_WB_DONE],
> +					       dpu_encoder_phys_wb_done_irq,
> +					       phys);
> +}
> +
> +/**
> + * dpu_encoder_phys_wb_irq_disable - irq control of WB
> + * @phys:	Pointer to physical encoder
> + */
> +static void dpu_encoder_phys_wb_irq_disable(struct dpu_encoder_phys *phys)
> +{
> +
> +	struct dpu_encoder_phys_wb *wb_enc = to_dpu_encoder_phys_wb(phys);
> +
> +	if (atomic_dec_return(&wb_enc->wbirq_refcount) == 0)
>   		dpu_core_irq_unregister_callback(phys->dpu_kms, phys->irq[INTR_IDX_WB_DONE]);
>   }
>   
> @@ -670,7 +680,8 @@ static void dpu_encoder_phys_wb_init_ops(struct dpu_encoder_phys_ops *ops)
>   	ops->trigger_start = dpu_encoder_helper_trigger_start;
>   	ops->prepare_wb_job = dpu_encoder_phys_wb_prepare_wb_job;
>   	ops->cleanup_wb_job = dpu_encoder_phys_wb_cleanup_wb_job;
> -	ops->irq_control = dpu_encoder_phys_wb_irq_ctrl;
> +	ops->irq_enable = dpu_encoder_phys_wb_irq_enable;
> +	ops->irq_disable = dpu_encoder_phys_wb_irq_disable;
>   	ops->is_valid_for_commit = dpu_encoder_phys_wb_is_valid_for_commit;
>   
>   }

[PATCH 2/3] drm/msm/dpu: split _dpu_encoder_resource_control_helper()

[Dmitry Baryshkov]

Follow the _dpu_encoder_irq_control() change and split the
_dpu_encoder_resource_control_helper() into enable and disable parts.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 45 +++++++++++++--------
 1 file changed, 29 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 7c131c5cbe71..cc61f0cf059d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -757,8 +757,7 @@ static void _dpu_encoder_irq_disable(struct drm_encoder *drm_enc)
 	}
 }
 
-static void _dpu_encoder_resource_control_helper(struct drm_encoder *drm_enc,
-		bool enable)
+static void _dpu_encoder_resource_enable(struct drm_encoder *drm_enc)
 {
 	struct msm_drm_private *priv;
 	struct dpu_kms *dpu_kms;
@@ -768,28 +767,42 @@ static void _dpu_encoder_resource_control_helper(struct drm_encoder *drm_enc,
 	priv = drm_enc->dev->dev_private;
 	dpu_kms = to_dpu_kms(priv->kms);
 
-	trace_dpu_enc_rc_helper(DRMID(drm_enc), enable);
+	trace_dpu_enc_rc_helper(DRMID(drm_enc), true);
 
 	if (!dpu_enc->cur_master) {
 		DPU_ERROR("encoder master not set\n");
 		return;
 	}
 
-	if (enable) {
-		/* enable DPU core clks */
-		pm_runtime_get_sync(&dpu_kms->pdev->dev);
+	/* enable DPU core clks */
+	pm_runtime_get_sync(&dpu_kms->pdev->dev);
 
-		/* enable all the irq */
-		_dpu_encoder_irq_enable(drm_enc);
+	/* enable all the irq */
+	_dpu_encoder_irq_enable(drm_enc);
+}
 
-	} else {
-		/* disable all the irq */
-		_dpu_encoder_irq_disable(drm_enc);
+static void _dpu_encoder_resource_disable(struct drm_encoder *drm_enc)
+{
+	struct msm_drm_private *priv;
+	struct dpu_kms *dpu_kms;
+	struct dpu_encoder_virt *dpu_enc;
 
-		/* disable DPU core clks */
-		pm_runtime_put_sync(&dpu_kms->pdev->dev);
+	dpu_enc = to_dpu_encoder_virt(drm_enc);
+	priv = drm_enc->dev->dev_private;
+	dpu_kms = to_dpu_kms(priv->kms);
+
+	trace_dpu_enc_rc_helper(DRMID(drm_enc), false);
+
+	if (!dpu_enc->cur_master) {
+		DPU_ERROR("encoder master not set\n");
+		return;
 	}
 
+	/* disable all the irq */
+	_dpu_encoder_irq_disable(drm_enc);
+
+	/* disable DPU core clks */
+	pm_runtime_put_sync(&dpu_kms->pdev->dev);
 }
 
 static int dpu_encoder_resource_control(struct drm_encoder *drm_enc,
@@ -847,7 +860,7 @@ static int dpu_encoder_resource_control(struct drm_encoder *drm_enc,
 		if (is_vid_mode && dpu_enc->rc_state == DPU_ENC_RC_STATE_IDLE)
 			_dpu_encoder_irq_enable(drm_enc);
 		else
-			_dpu_encoder_resource_control_helper(drm_enc, true);
+			_dpu_encoder_resource_enable(drm_enc);
 
 		dpu_enc->rc_state = DPU_ENC_RC_STATE_ON;
 
@@ -942,7 +955,7 @@ static int dpu_encoder_resource_control(struct drm_encoder *drm_enc,
 		 * and in IDLE state the resources are already disabled
 		 */
 		if (dpu_enc->rc_state == DPU_ENC_RC_STATE_PRE_OFF)
-			_dpu_encoder_resource_control_helper(drm_enc, false);
+			_dpu_encoder_resource_disable(drm_enc);
 
 		dpu_enc->rc_state = DPU_ENC_RC_STATE_OFF;
 
@@ -977,7 +990,7 @@ static int dpu_encoder_resource_control(struct drm_encoder *drm_enc,
 		if (is_vid_mode)
 			_dpu_encoder_irq_disable(drm_enc);
 		else
-			_dpu_encoder_resource_control_helper(drm_enc, false);
+			_dpu_encoder_resource_disable(drm_enc);
 
 		dpu_enc->rc_state = DPU_ENC_RC_STATE_IDLE;
 
-- 
2.39.2

Re: [PATCH 2/3] drm/msm/dpu: split _dpu_encoder_resource_control_helper()

[Abhinav Kumar]

On 6/4/2023 7:45 AM, Dmitry Baryshkov wrote:
> Follow the _dpu_encoder_irq_control() change and split the
> _dpu_encoder_resource_control_helper() into enable and disable parts.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 45 +++++++++++++--------
>   1 file changed, 29 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 7c131c5cbe71..cc61f0cf059d 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -757,8 +757,7 @@ static void _dpu_encoder_irq_disable(struct drm_encoder *drm_enc)
>   	}
>   }
>   
> -static void _dpu_encoder_resource_control_helper(struct drm_encoder *drm_enc,
> -		bool enable)
> +static void _dpu_encoder_resource_enable(struct drm_encoder *drm_enc)
>   {
>   	struct msm_drm_private *priv;
>   	struct dpu_kms *dpu_kms;
> @@ -768,28 +767,42 @@ static void _dpu_encoder_resource_control_helper(struct drm_encoder *drm_enc,
>   	priv = drm_enc->dev->dev_private;
>   	dpu_kms = to_dpu_kms(priv->kms);
>   
> -	trace_dpu_enc_rc_helper(DRMID(drm_enc), enable);
> +	trace_dpu_enc_rc_helper(DRMID(drm_enc), true);

same question about trace here.

>   
>   	if (!dpu_enc->cur_master) {
>   		DPU_ERROR("encoder master not set\n");
>   		return;
>   	}
>   
> -	if (enable) {
> -		/* enable DPU core clks */
> -		pm_runtime_get_sync(&dpu_kms->pdev->dev);
> +	/* enable DPU core clks */
> +	pm_runtime_get_sync(&dpu_kms->pdev->dev);
>   
> -		/* enable all the irq */
> -		_dpu_encoder_irq_enable(drm_enc);
> +	/* enable all the irq */
> +	_dpu_encoder_irq_enable(drm_enc);
> +}
>   
> -	} else {
> -		/* disable all the irq */
> -		_dpu_encoder_irq_disable(drm_enc);
> +static void _dpu_encoder_resource_disable(struct drm_encoder *drm_enc)
> +{
> +	struct msm_drm_private *priv;
> +	struct dpu_kms *dpu_kms;
> +	struct dpu_encoder_virt *dpu_enc;
>   
> -		/* disable DPU core clks */
> -		pm_runtime_put_sync(&dpu_kms->pdev->dev);
> +	dpu_enc = to_dpu_encoder_virt(drm_enc);
> +	priv = drm_enc->dev->dev_private;
> +	dpu_kms = to_dpu_kms(priv->kms);
> +
> +	trace_dpu_enc_rc_helper(DRMID(drm_enc), false);

and here.

> +
> +	if (!dpu_enc->cur_master) {
> +		DPU_ERROR("encoder master not set\n");
> +		return;
>   	}
>   
> +	/* disable all the irq */
> +	_dpu_encoder_irq_disable(drm_enc);
> +
> +	/* disable DPU core clks */
> +	pm_runtime_put_sync(&dpu_kms->pdev->dev);
>   }
>   
>   static int dpu_encoder_resource_control(struct drm_encoder *drm_enc,
> @@ -847,7 +860,7 @@ static int dpu_encoder_resource_control(struct drm_encoder *drm_enc,
>   		if (is_vid_mode && dpu_enc->rc_state == DPU_ENC_RC_STATE_IDLE)
>   			_dpu_encoder_irq_enable(drm_enc);
>   		else
> -			_dpu_encoder_resource_control_helper(drm_enc, true);
> +			_dpu_encoder_resource_enable(drm_enc);
>   
>   		dpu_enc->rc_state = DPU_ENC_RC_STATE_ON;
>   
> @@ -942,7 +955,7 @@ static int dpu_encoder_resource_control(struct drm_encoder *drm_enc,
>   		 * and in IDLE state the resources are already disabled
>   		 */
>   		if (dpu_enc->rc_state == DPU_ENC_RC_STATE_PRE_OFF)
> -			_dpu_encoder_resource_control_helper(drm_enc, false);
> +			_dpu_encoder_resource_disable(drm_enc);
>   
>   		dpu_enc->rc_state = DPU_ENC_RC_STATE_OFF;
>   
> @@ -977,7 +990,7 @@ static int dpu_encoder_resource_control(struct drm_encoder *drm_enc,
>   		if (is_vid_mode)
>   			_dpu_encoder_irq_disable(drm_enc);
>   		else
> -			_dpu_encoder_resource_control_helper(drm_enc, false);
> +			_dpu_encoder_resource_disable(drm_enc);
>   
>   		dpu_enc->rc_state = DPU_ENC_RC_STATE_IDLE;
>   

[PATCH 3/3] drm/msm/dpu: drop dpu_encoder_phys_ops.atomic_mode_set

[Dmitry Baryshkov]

The atomic_mode_set() callback only sets the phys_enc's IRQ data. As the
INTF and WB are statically allocated to each encoder/phys_enc, drop the
atomic_mode_set callback and set the IRQs during encoder init.

For the CMD panel usecase some of IRQ indexes depend on the selected
resources. Move setting them to the irq_enable() callback.

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

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index cc61f0cf059d..6b5c80dc5967 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1148,8 +1148,6 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
 		phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]);
 
 		phys->cached_mode = crtc_state->adjusted_mode;
-		if (phys->ops.atomic_mode_set)
-			phys->ops.atomic_mode_set(phys, crtc_state, conn_state);
 	}
 }
 
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 faf033cd086e..24dbc28be4f8 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
@@ -67,8 +67,6 @@ struct dpu_encoder_phys;
  * @is_master:			Whether this phys_enc is the current master
  *				encoder. Can be switched at enable time. Based
  *				on split_role and current mode (CMD/VID).
- * @atomic_mode_set:		DRM Call. Set a DRM mode.
- *				This likely caches the mode, for use at enable.
  * @enable:			DRM Call. Enable a DRM mode.
  * @disable:			DRM Call. Disable mode.
  * @atomic_check:		DRM Call. Atomic check new DRM state.
@@ -95,9 +93,6 @@ struct dpu_encoder_phys;
 struct dpu_encoder_phys_ops {
 	void (*prepare_commit)(struct dpu_encoder_phys *encoder);
 	bool (*is_master)(struct dpu_encoder_phys *encoder);
-	void (*atomic_mode_set)(struct dpu_encoder_phys *encoder,
-			struct drm_crtc_state *crtc_state,
-			struct drm_connector_state *conn_state);
 	void (*enable)(struct dpu_encoder_phys *encoder);
 	void (*disable)(struct dpu_encoder_phys *encoder);
 	int (*atomic_check)(struct dpu_encoder_phys *encoder,
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 3422b49f23c2..a0b7d8803e94 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
@@ -140,23 +140,6 @@ static void dpu_encoder_phys_cmd_underrun_irq(void *arg, int irq_idx)
 	dpu_encoder_underrun_callback(phys_enc->parent, phys_enc);
 }
 
-static void dpu_encoder_phys_cmd_atomic_mode_set(
-		struct dpu_encoder_phys *phys_enc,
-		struct drm_crtc_state *crtc_state,
-		struct drm_connector_state *conn_state)
-{
-	phys_enc->irq[INTR_IDX_CTL_START] = phys_enc->hw_ctl->caps->intr_start;
-
-	phys_enc->irq[INTR_IDX_PINGPONG] = phys_enc->hw_pp->caps->intr_done;
-
-	if (phys_enc->has_intf_te)
-		phys_enc->irq[INTR_IDX_RDPTR] = phys_enc->hw_intf->cap->intr_tear_rd_ptr;
-	else
-		phys_enc->irq[INTR_IDX_RDPTR] = phys_enc->hw_pp->caps->intr_rdptr;
-
-	phys_enc->irq[INTR_IDX_UNDERRUN] = phys_enc->hw_intf->cap->intr_underrun;
-}
-
 static int _dpu_encoder_phys_cmd_handle_ppdone_timeout(
 		struct dpu_encoder_phys *phys_enc)
 {
@@ -287,6 +270,14 @@ static void dpu_encoder_phys_cmd_irq_enable(struct dpu_encoder_phys *phys_enc)
 					true,
 					atomic_read(&phys_enc->vblank_refcount));
 
+	phys_enc->irq[INTR_IDX_CTL_START] = phys_enc->hw_ctl->caps->intr_start;
+	phys_enc->irq[INTR_IDX_PINGPONG] = phys_enc->hw_pp->caps->intr_done;
+
+	if (phys_enc->has_intf_te)
+		phys_enc->irq[INTR_IDX_RDPTR] = phys_enc->hw_intf->cap->intr_tear_rd_ptr;
+	else
+		phys_enc->irq[INTR_IDX_RDPTR] = phys_enc->hw_pp->caps->intr_rdptr;
+
 	dpu_core_irq_register_callback(phys_enc->dpu_kms,
 				       phys_enc->irq[INTR_IDX_PINGPONG],
 				       dpu_encoder_phys_cmd_pp_tx_done_irq,
@@ -318,6 +309,10 @@ static void dpu_encoder_phys_cmd_irq_disable(struct dpu_encoder_phys *phys_enc)
 	dpu_core_irq_unregister_callback(phys_enc->dpu_kms, phys_enc->irq[INTR_IDX_UNDERRUN]);
 	dpu_encoder_phys_cmd_control_vblank_irq(phys_enc, false);
 	dpu_core_irq_unregister_callback(phys_enc->dpu_kms, phys_enc->irq[INTR_IDX_PINGPONG]);
+
+	phys_enc->irq[INTR_IDX_CTL_START] = -1;
+	phys_enc->irq[INTR_IDX_PINGPONG] = -1;
+	phys_enc->irq[INTR_IDX_RDPTR] = -1;
 }
 
 static void dpu_encoder_phys_cmd_tearcheck_config(
@@ -737,7 +732,6 @@ static void dpu_encoder_phys_cmd_init_ops(
 		struct dpu_encoder_phys_ops *ops)
 {
 	ops->is_master = dpu_encoder_phys_cmd_is_master;
-	ops->atomic_mode_set = dpu_encoder_phys_cmd_atomic_mode_set;
 	ops->enable = dpu_encoder_phys_cmd_enable;
 	ops->disable = dpu_encoder_phys_cmd_disable;
 	ops->destroy = dpu_encoder_phys_cmd_destroy;
@@ -775,6 +769,8 @@ struct dpu_encoder_phys *dpu_encoder_phys_cmd_init(
 
 	dpu_encoder_phys_cmd_init_ops(&phys_enc->ops);
 	phys_enc->intf_mode = INTF_MODE_CMD;
+	phys_enc->irq[INTR_IDX_UNDERRUN] = phys_enc->hw_intf->cap->intr_underrun;
+
 	cmd_enc->stream_sel = 0;
 
 	phys_enc->has_intf_te = test_bit(DPU_INTF_TE,
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 a550b290246c..3f2e0ebe3cfc 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
@@ -348,16 +348,6 @@ static bool dpu_encoder_phys_vid_needs_single_flush(
 	return phys_enc->split_role != ENC_ROLE_SOLO;
 }
 
-static void dpu_encoder_phys_vid_atomic_mode_set(
-		struct dpu_encoder_phys *phys_enc,
-		struct drm_crtc_state *crtc_state,
-		struct drm_connector_state *conn_state)
-{
-	phys_enc->irq[INTR_IDX_VSYNC] = phys_enc->hw_intf->cap->intr_vsync;
-
-	phys_enc->irq[INTR_IDX_UNDERRUN] = phys_enc->hw_intf->cap->intr_underrun;
-}
-
 static int dpu_encoder_phys_vid_control_vblank_irq(
 		struct dpu_encoder_phys *phys_enc,
 		bool enable)
@@ -684,7 +674,6 @@ static int dpu_encoder_phys_vid_get_frame_count(
 static void dpu_encoder_phys_vid_init_ops(struct dpu_encoder_phys_ops *ops)
 {
 	ops->is_master = dpu_encoder_phys_vid_is_master;
-	ops->atomic_mode_set = dpu_encoder_phys_vid_atomic_mode_set;
 	ops->enable = dpu_encoder_phys_vid_enable;
 	ops->disable = dpu_encoder_phys_vid_disable;
 	ops->destroy = dpu_encoder_phys_vid_destroy;
@@ -723,6 +712,8 @@ struct dpu_encoder_phys *dpu_encoder_phys_vid_init(
 
 	dpu_encoder_phys_vid_init_ops(&phys_enc->ops);
 	phys_enc->intf_mode = INTF_MODE_VIDEO;
+	phys_enc->irq[INTR_IDX_VSYNC] = phys_enc->hw_intf->cap->intr_vsync;
+	phys_enc->irq[INTR_IDX_UNDERRUN] = phys_enc->hw_intf->cap->intr_underrun;
 
 	DPU_DEBUG_VIDENC(phys_enc, "created intf idx:%d\n", p->hw_intf->idx);
 
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 858fe6656c9b..439f645e0bc9 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
@@ -410,15 +410,6 @@ static void dpu_encoder_phys_wb_irq_disable(struct dpu_encoder_phys *phys)
 		dpu_core_irq_unregister_callback(phys->dpu_kms, phys->irq[INTR_IDX_WB_DONE]);
 }
 
-static void dpu_encoder_phys_wb_atomic_mode_set(
-		struct dpu_encoder_phys *phys_enc,
-		struct drm_crtc_state *crtc_state,
-		struct drm_connector_state *conn_state)
-{
-
-	phys_enc->irq[INTR_IDX_WB_DONE] = phys_enc->hw_wb->caps->intr_wb_done;
-}
-
 static void _dpu_encoder_phys_wb_handle_wbdone_timeout(
 		struct dpu_encoder_phys *phys_enc)
 {
@@ -668,7 +659,6 @@ static bool dpu_encoder_phys_wb_is_valid_for_commit(struct dpu_encoder_phys *phy
 static void dpu_encoder_phys_wb_init_ops(struct dpu_encoder_phys_ops *ops)
 {
 	ops->is_master = dpu_encoder_phys_wb_is_master;
-	ops->atomic_mode_set = dpu_encoder_phys_wb_atomic_mode_set;
 	ops->enable = dpu_encoder_phys_wb_enable;
 	ops->disable = dpu_encoder_phys_wb_disable;
 	ops->destroy = dpu_encoder_phys_wb_destroy;
@@ -715,6 +705,7 @@ struct dpu_encoder_phys *dpu_encoder_phys_wb_init(
 
 	dpu_encoder_phys_wb_init_ops(&phys_enc->ops);
 	phys_enc->intf_mode = INTF_MODE_WB_LINE;
+	phys_enc->irq[INTR_IDX_WB_DONE] = phys_enc->hw_wb->caps->intr_wb_done;
 
 	atomic_set(&wb_enc->wbirq_refcount, 0);
 
-- 
2.39.2

Re: [PATCH 3/3] drm/msm/dpu: drop dpu_encoder_phys_ops.atomic_mode_set

[Abhinav Kumar]

On 6/4/2023 7:45 AM, Dmitry Baryshkov wrote:
> The atomic_mode_set() callback only sets the phys_enc's IRQ data. As the
> INTF and WB are statically allocated to each encoder/phys_enc, drop the
> atomic_mode_set callback and set the IRQs during encoder init.
> 
> For the CMD panel usecase some of IRQ indexes depend on the selected
> resources. Move setting them to the irq_enable() callback.
> 

The irq_enable() callback is called from the 
dpu_encoder_virt_atomic_enable() after the phys layer's enable.

Thats late.

So lets consider the case where command mode panel's clock is powered 
from bootloader (quite common).

Now, as soon as the tearcheck is configured and interface is ON from the 
phys's enable(), nothing prevents / should prevent the interrupt from 
firing.

So I feel / think mode_set is the correct location to assign these.

I can ack patches 1 and 2 but I think you did those mainly for this one, 
so I would like to get some clarity on this part.

> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   |  2 --
>   .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |  5 ---
>   .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  | 32 ++++++++-----------
>   .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  | 13 ++------
>   .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c   | 11 +------
>   5 files changed, 17 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index cc61f0cf059d..6b5c80dc5967 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -1148,8 +1148,6 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
>   		phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]);
>   
>   		phys->cached_mode = crtc_state->adjusted_mode;
> -		if (phys->ops.atomic_mode_set)
> -			phys->ops.atomic_mode_set(phys, crtc_state, conn_state);
>   	}
>   }
>   
> 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 faf033cd086e..24dbc28be4f8 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> @@ -67,8 +67,6 @@ struct dpu_encoder_phys;
>    * @is_master:			Whether this phys_enc is the current master
>    *				encoder. Can be switched at enable time. Based
>    *				on split_role and current mode (CMD/VID).
> - * @atomic_mode_set:		DRM Call. Set a DRM mode.
> - *				This likely caches the mode, for use at enable.
>    * @enable:			DRM Call. Enable a DRM mode.
>    * @disable:			DRM Call. Disable mode.
>    * @atomic_check:		DRM Call. Atomic check new DRM state.
> @@ -95,9 +93,6 @@ struct dpu_encoder_phys;
>   struct dpu_encoder_phys_ops {
>   	void (*prepare_commit)(struct dpu_encoder_phys *encoder);
>   	bool (*is_master)(struct dpu_encoder_phys *encoder);
> -	void (*atomic_mode_set)(struct dpu_encoder_phys *encoder,
> -			struct drm_crtc_state *crtc_state,
> -			struct drm_connector_state *conn_state);
>   	void (*enable)(struct dpu_encoder_phys *encoder);
>   	void (*disable)(struct dpu_encoder_phys *encoder);
>   	int (*atomic_check)(struct dpu_encoder_phys *encoder,
> 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 3422b49f23c2..a0b7d8803e94 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
> @@ -140,23 +140,6 @@ static void dpu_encoder_phys_cmd_underrun_irq(void *arg, int irq_idx)
>   	dpu_encoder_underrun_callback(phys_enc->parent, phys_enc);
>   }
>   
> -static void dpu_encoder_phys_cmd_atomic_mode_set(
> -		struct dpu_encoder_phys *phys_enc,
> -		struct drm_crtc_state *crtc_state,
> -		struct drm_connector_state *conn_state)
> -{
> -	phys_enc->irq[INTR_IDX_CTL_START] = phys_enc->hw_ctl->caps->intr_start;
> -
> -	phys_enc->irq[INTR_IDX_PINGPONG] = phys_enc->hw_pp->caps->intr_done;
> -
> -	if (phys_enc->has_intf_te)
> -		phys_enc->irq[INTR_IDX_RDPTR] = phys_enc->hw_intf->cap->intr_tear_rd_ptr;
> -	else
> -		phys_enc->irq[INTR_IDX_RDPTR] = phys_enc->hw_pp->caps->intr_rdptr;
> -
> -	phys_enc->irq[INTR_IDX_UNDERRUN] = phys_enc->hw_intf->cap->intr_underrun;
> -}
> -
>   static int _dpu_encoder_phys_cmd_handle_ppdone_timeout(
>   		struct dpu_encoder_phys *phys_enc)
>   {
> @@ -287,6 +270,14 @@ static void dpu_encoder_phys_cmd_irq_enable(struct dpu_encoder_phys *phys_enc)
>   					true,
>   					atomic_read(&phys_enc->vblank_refcount));
>   
> +	phys_enc->irq[INTR_IDX_CTL_START] = phys_enc->hw_ctl->caps->intr_start;
> +	phys_enc->irq[INTR_IDX_PINGPONG] = phys_enc->hw_pp->caps->intr_done;
> +
> +	if (phys_enc->has_intf_te)
> +		phys_enc->irq[INTR_IDX_RDPTR] = phys_enc->hw_intf->cap->intr_tear_rd_ptr;
> +	else
> +		phys_enc->irq[INTR_IDX_RDPTR] = phys_enc->hw_pp->caps->intr_rdptr;
> +
>   	dpu_core_irq_register_callback(phys_enc->dpu_kms,
>   				       phys_enc->irq[INTR_IDX_PINGPONG],
>   				       dpu_encoder_phys_cmd_pp_tx_done_irq,
> @@ -318,6 +309,10 @@ static void dpu_encoder_phys_cmd_irq_disable(struct dpu_encoder_phys *phys_enc)
>   	dpu_core_irq_unregister_callback(phys_enc->dpu_kms, phys_enc->irq[INTR_IDX_UNDERRUN]);
>   	dpu_encoder_phys_cmd_control_vblank_irq(phys_enc, false);
>   	dpu_core_irq_unregister_callback(phys_enc->dpu_kms, phys_enc->irq[INTR_IDX_PINGPONG]);
> +
> +	phys_enc->irq[INTR_IDX_CTL_START] = -1;
> +	phys_enc->irq[INTR_IDX_PINGPONG] = -1;
> +	phys_enc->irq[INTR_IDX_RDPTR] = -1;
>   }
>   
>   static void dpu_encoder_phys_cmd_tearcheck_config(
> @@ -737,7 +732,6 @@ static void dpu_encoder_phys_cmd_init_ops(
>   		struct dpu_encoder_phys_ops *ops)
>   {
>   	ops->is_master = dpu_encoder_phys_cmd_is_master;
> -	ops->atomic_mode_set = dpu_encoder_phys_cmd_atomic_mode_set;
>   	ops->enable = dpu_encoder_phys_cmd_enable;
>   	ops->disable = dpu_encoder_phys_cmd_disable;
>   	ops->destroy = dpu_encoder_phys_cmd_destroy;
> @@ -775,6 +769,8 @@ struct dpu_encoder_phys *dpu_encoder_phys_cmd_init(
>   
>   	dpu_encoder_phys_cmd_init_ops(&phys_enc->ops);
>   	phys_enc->intf_mode = INTF_MODE_CMD;
> +	phys_enc->irq[INTR_IDX_UNDERRUN] = phys_enc->hw_intf->cap->intr_underrun;
> +
>   	cmd_enc->stream_sel = 0;
>   
>   	phys_enc->has_intf_te = test_bit(DPU_INTF_TE,
> 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 a550b290246c..3f2e0ebe3cfc 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
> @@ -348,16 +348,6 @@ static bool dpu_encoder_phys_vid_needs_single_flush(
>   	return phys_enc->split_role != ENC_ROLE_SOLO;
>   }
>   
> -static void dpu_encoder_phys_vid_atomic_mode_set(
> -		struct dpu_encoder_phys *phys_enc,
> -		struct drm_crtc_state *crtc_state,
> -		struct drm_connector_state *conn_state)
> -{
> -	phys_enc->irq[INTR_IDX_VSYNC] = phys_enc->hw_intf->cap->intr_vsync;
> -
> -	phys_enc->irq[INTR_IDX_UNDERRUN] = phys_enc->hw_intf->cap->intr_underrun;
> -}
> -
>   static int dpu_encoder_phys_vid_control_vblank_irq(
>   		struct dpu_encoder_phys *phys_enc,
>   		bool enable)
> @@ -684,7 +674,6 @@ static int dpu_encoder_phys_vid_get_frame_count(
>   static void dpu_encoder_phys_vid_init_ops(struct dpu_encoder_phys_ops *ops)
>   {
>   	ops->is_master = dpu_encoder_phys_vid_is_master;
> -	ops->atomic_mode_set = dpu_encoder_phys_vid_atomic_mode_set;
>   	ops->enable = dpu_encoder_phys_vid_enable;
>   	ops->disable = dpu_encoder_phys_vid_disable;
>   	ops->destroy = dpu_encoder_phys_vid_destroy;
> @@ -723,6 +712,8 @@ struct dpu_encoder_phys *dpu_encoder_phys_vid_init(
>   
>   	dpu_encoder_phys_vid_init_ops(&phys_enc->ops);
>   	phys_enc->intf_mode = INTF_MODE_VIDEO;
> +	phys_enc->irq[INTR_IDX_VSYNC] = phys_enc->hw_intf->cap->intr_vsync;
> +	phys_enc->irq[INTR_IDX_UNDERRUN] = phys_enc->hw_intf->cap->intr_underrun;
>   
>   	DPU_DEBUG_VIDENC(phys_enc, "created intf idx:%d\n", p->hw_intf->idx);
>   
> 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 858fe6656c9b..439f645e0bc9 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
> @@ -410,15 +410,6 @@ static void dpu_encoder_phys_wb_irq_disable(struct dpu_encoder_phys *phys)
>   		dpu_core_irq_unregister_callback(phys->dpu_kms, phys->irq[INTR_IDX_WB_DONE]);
>   }
>   
> -static void dpu_encoder_phys_wb_atomic_mode_set(
> -		struct dpu_encoder_phys *phys_enc,
> -		struct drm_crtc_state *crtc_state,
> -		struct drm_connector_state *conn_state)
> -{
> -
> -	phys_enc->irq[INTR_IDX_WB_DONE] = phys_enc->hw_wb->caps->intr_wb_done;
> -}
> -
>   static void _dpu_encoder_phys_wb_handle_wbdone_timeout(
>   		struct dpu_encoder_phys *phys_enc)
>   {
> @@ -668,7 +659,6 @@ static bool dpu_encoder_phys_wb_is_valid_for_commit(struct dpu_encoder_phys *phy
>   static void dpu_encoder_phys_wb_init_ops(struct dpu_encoder_phys_ops *ops)
>   {
>   	ops->is_master = dpu_encoder_phys_wb_is_master;
> -	ops->atomic_mode_set = dpu_encoder_phys_wb_atomic_mode_set;
>   	ops->enable = dpu_encoder_phys_wb_enable;
>   	ops->disable = dpu_encoder_phys_wb_disable;
>   	ops->destroy = dpu_encoder_phys_wb_destroy;
> @@ -715,6 +705,7 @@ struct dpu_encoder_phys *dpu_encoder_phys_wb_init(
>   
>   	dpu_encoder_phys_wb_init_ops(&phys_enc->ops);
>   	phys_enc->intf_mode = INTF_MODE_WB_LINE;
> +	phys_enc->irq[INTR_IDX_WB_DONE] = phys_enc->hw_wb->caps->intr_wb_done;
>   
>   	atomic_set(&wb_enc->wbirq_refcount, 0);
>   

Re: [PATCH 3/3] drm/msm/dpu: drop dpu_encoder_phys_ops.atomic_mode_set

[Dmitry Baryshkov]

On 08/08/2023 02:49, Abhinav Kumar wrote:
> 
> 
> On 6/4/2023 7:45 AM, Dmitry Baryshkov wrote:
>> The atomic_mode_set() callback only sets the phys_enc's IRQ data. As the
>> INTF and WB are statically allocated to each encoder/phys_enc, drop the
>> atomic_mode_set callback and set the IRQs during encoder init.
>>
>> For the CMD panel usecase some of IRQ indexes depend on the selected
>> resources. Move setting them to the irq_enable() callback.
>>
> 
> The irq_enable() callback is called from the 
> dpu_encoder_virt_atomic_enable() after the phys layer's enable.
> 
> Thats late.
> 
> So lets consider the case where command mode panel's clock is powered 
> from bootloader (quite common).
> 
> Now, as soon as the tearcheck is configured and interface is ON from the 
> phys's enable(), nothing prevents / should prevent the interrupt from 
> firing.

Please note that interrupt callbacks are also registered from the 
irq_control(). There is no way the interrupt can fire beforehand (and 
the call chain is dpu_encoder_virt_atomic_enable() -> 
dpu_encoder_resource_control() -> _dpu_encoder_resource_control_helper() 
-> _dpu_encoder_irq_control() -> irq_control().

If we ever want to move irq_control() to be called before phys's 
enable() callbacks, this will be a separate patchset, involving 
refactoring of dpu_encoder_resource_control().

> 
> So I feel / think mode_set is the correct location to assign these.
> 
> I can ack patches 1 and 2 but I think you did those mainly for this one, 
> so I would like to get some clarity on this part.
> 
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   |  2 --
>>   .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |  5 ---
>>   .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  | 32 ++++++++-----------
>>   .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  | 13 ++------
>>   .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c   | 11 +------
>>   5 files changed, 17 insertions(+), 46 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index cc61f0cf059d..6b5c80dc5967 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> @@ -1148,8 +1148,6 @@ static void 
>> dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
>>           phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]);
>>           phys->cached_mode = crtc_state->adjusted_mode;
>> -        if (phys->ops.atomic_mode_set)
>> -            phys->ops.atomic_mode_set(phys, crtc_state, conn_state);
>>       }
>>   }
>> 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 faf033cd086e..24dbc28be4f8 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>> @@ -67,8 +67,6 @@ struct dpu_encoder_phys;
>>    * @is_master:            Whether this phys_enc is the current master
>>    *                encoder. Can be switched at enable time. Based
>>    *                on split_role and current mode (CMD/VID).
>> - * @atomic_mode_set:        DRM Call. Set a DRM mode.
>> - *                This likely caches the mode, for use at enable.
>>    * @enable:            DRM Call. Enable a DRM mode.
>>    * @disable:            DRM Call. Disable mode.
>>    * @atomic_check:        DRM Call. Atomic check new DRM state.
>> @@ -95,9 +93,6 @@ struct dpu_encoder_phys;
>>   struct dpu_encoder_phys_ops {
>>       void (*prepare_commit)(struct dpu_encoder_phys *encoder);
>>       bool (*is_master)(struct dpu_encoder_phys *encoder);
>> -    void (*atomic_mode_set)(struct dpu_encoder_phys *encoder,
>> -            struct drm_crtc_state *crtc_state,
>> -            struct drm_connector_state *conn_state);
>>       void (*enable)(struct dpu_encoder_phys *encoder);
>>       void (*disable)(struct dpu_encoder_phys *encoder);
>>       int (*atomic_check)(struct dpu_encoder_phys *encoder,
>> 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 3422b49f23c2..a0b7d8803e94 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
>> @@ -140,23 +140,6 @@ static void 
>> dpu_encoder_phys_cmd_underrun_irq(void *arg, int irq_idx)
>>       dpu_encoder_underrun_callback(phys_enc->parent, phys_enc);
>>   }
>> -static void dpu_encoder_phys_cmd_atomic_mode_set(
>> -        struct dpu_encoder_phys *phys_enc,
>> -        struct drm_crtc_state *crtc_state,
>> -        struct drm_connector_state *conn_state)
>> -{
>> -    phys_enc->irq[INTR_IDX_CTL_START] = 
>> phys_enc->hw_ctl->caps->intr_start;
>> -
>> -    phys_enc->irq[INTR_IDX_PINGPONG] = phys_enc->hw_pp->caps->intr_done;
>> -
>> -    if (phys_enc->has_intf_te)
>> -        phys_enc->irq[INTR_IDX_RDPTR] = 
>> phys_enc->hw_intf->cap->intr_tear_rd_ptr;
>> -    else
>> -        phys_enc->irq[INTR_IDX_RDPTR] = 
>> phys_enc->hw_pp->caps->intr_rdptr;
>> -
>> -    phys_enc->irq[INTR_IDX_UNDERRUN] = 
>> phys_enc->hw_intf->cap->intr_underrun;
>> -}
>> -
>>   static int _dpu_encoder_phys_cmd_handle_ppdone_timeout(
>>           struct dpu_encoder_phys *phys_enc)
>>   {
>> @@ -287,6 +270,14 @@ static void 
>> dpu_encoder_phys_cmd_irq_enable(struct dpu_encoder_phys *phys_enc)
>>                       true,
>>                       atomic_read(&phys_enc->vblank_refcount));
>> +    phys_enc->irq[INTR_IDX_CTL_START] = 
>> phys_enc->hw_ctl->caps->intr_start;
>> +    phys_enc->irq[INTR_IDX_PINGPONG] = phys_enc->hw_pp->caps->intr_done;
>> +
>> +    if (phys_enc->has_intf_te)
>> +        phys_enc->irq[INTR_IDX_RDPTR] = 
>> phys_enc->hw_intf->cap->intr_tear_rd_ptr;
>> +    else
>> +        phys_enc->irq[INTR_IDX_RDPTR] = 
>> phys_enc->hw_pp->caps->intr_rdptr;
>> +
>>       dpu_core_irq_register_callback(phys_enc->dpu_kms,
>>                          phys_enc->irq[INTR_IDX_PINGPONG],
>>                          dpu_encoder_phys_cmd_pp_tx_done_irq,
>> @@ -318,6 +309,10 @@ static void 
>> dpu_encoder_phys_cmd_irq_disable(struct dpu_encoder_phys *phys_enc)
>>       dpu_core_irq_unregister_callback(phys_enc->dpu_kms, 
>> phys_enc->irq[INTR_IDX_UNDERRUN]);
>>       dpu_encoder_phys_cmd_control_vblank_irq(phys_enc, false);
>>       dpu_core_irq_unregister_callback(phys_enc->dpu_kms, 
>> phys_enc->irq[INTR_IDX_PINGPONG]);
>> +
>> +    phys_enc->irq[INTR_IDX_CTL_START] = -1;
>> +    phys_enc->irq[INTR_IDX_PINGPONG] = -1;
>> +    phys_enc->irq[INTR_IDX_RDPTR] = -1;
>>   }
>>   static void dpu_encoder_phys_cmd_tearcheck_config(
>> @@ -737,7 +732,6 @@ static void dpu_encoder_phys_cmd_init_ops(
>>           struct dpu_encoder_phys_ops *ops)
>>   {
>>       ops->is_master = dpu_encoder_phys_cmd_is_master;
>> -    ops->atomic_mode_set = dpu_encoder_phys_cmd_atomic_mode_set;
>>       ops->enable = dpu_encoder_phys_cmd_enable;
>>       ops->disable = dpu_encoder_phys_cmd_disable;
>>       ops->destroy = dpu_encoder_phys_cmd_destroy;
>> @@ -775,6 +769,8 @@ struct dpu_encoder_phys *dpu_encoder_phys_cmd_init(
>>       dpu_encoder_phys_cmd_init_ops(&phys_enc->ops);
>>       phys_enc->intf_mode = INTF_MODE_CMD;
>> +    phys_enc->irq[INTR_IDX_UNDERRUN] = 
>> phys_enc->hw_intf->cap->intr_underrun;
>> +
>>       cmd_enc->stream_sel = 0;
>>       phys_enc->has_intf_te = test_bit(DPU_INTF_TE,
>> 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 a550b290246c..3f2e0ebe3cfc 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
>> @@ -348,16 +348,6 @@ static bool dpu_encoder_phys_vid_needs_single_flush(
>>       return phys_enc->split_role != ENC_ROLE_SOLO;
>>   }
>> -static void dpu_encoder_phys_vid_atomic_mode_set(
>> -        struct dpu_encoder_phys *phys_enc,
>> -        struct drm_crtc_state *crtc_state,
>> -        struct drm_connector_state *conn_state)
>> -{
>> -    phys_enc->irq[INTR_IDX_VSYNC] = phys_enc->hw_intf->cap->intr_vsync;
>> -
>> -    phys_enc->irq[INTR_IDX_UNDERRUN] = 
>> phys_enc->hw_intf->cap->intr_underrun;
>> -}
>> -
>>   static int dpu_encoder_phys_vid_control_vblank_irq(
>>           struct dpu_encoder_phys *phys_enc,
>>           bool enable)
>> @@ -684,7 +674,6 @@ static int dpu_encoder_phys_vid_get_frame_count(
>>   static void dpu_encoder_phys_vid_init_ops(struct 
>> dpu_encoder_phys_ops *ops)
>>   {
>>       ops->is_master = dpu_encoder_phys_vid_is_master;
>> -    ops->atomic_mode_set = dpu_encoder_phys_vid_atomic_mode_set;
>>       ops->enable = dpu_encoder_phys_vid_enable;
>>       ops->disable = dpu_encoder_phys_vid_disable;
>>       ops->destroy = dpu_encoder_phys_vid_destroy;
>> @@ -723,6 +712,8 @@ struct dpu_encoder_phys *dpu_encoder_phys_vid_init(
>>       dpu_encoder_phys_vid_init_ops(&phys_enc->ops);
>>       phys_enc->intf_mode = INTF_MODE_VIDEO;
>> +    phys_enc->irq[INTR_IDX_VSYNC] = phys_enc->hw_intf->cap->intr_vsync;
>> +    phys_enc->irq[INTR_IDX_UNDERRUN] = 
>> phys_enc->hw_intf->cap->intr_underrun;
>>       DPU_DEBUG_VIDENC(phys_enc, "created intf idx:%d\n", 
>> p->hw_intf->idx);
>> 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 858fe6656c9b..439f645e0bc9 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
>> @@ -410,15 +410,6 @@ static void 
>> dpu_encoder_phys_wb_irq_disable(struct dpu_encoder_phys *phys)
>>           dpu_core_irq_unregister_callback(phys->dpu_kms, 
>> phys->irq[INTR_IDX_WB_DONE]);
>>   }
>> -static void dpu_encoder_phys_wb_atomic_mode_set(
>> -        struct dpu_encoder_phys *phys_enc,
>> -        struct drm_crtc_state *crtc_state,
>> -        struct drm_connector_state *conn_state)
>> -{
>> -
>> -    phys_enc->irq[INTR_IDX_WB_DONE] = 
>> phys_enc->hw_wb->caps->intr_wb_done;
>> -}
>> -
>>   static void _dpu_encoder_phys_wb_handle_wbdone_timeout(
>>           struct dpu_encoder_phys *phys_enc)
>>   {
>> @@ -668,7 +659,6 @@ static bool 
>> dpu_encoder_phys_wb_is_valid_for_commit(struct dpu_encoder_phys *phy
>>   static void dpu_encoder_phys_wb_init_ops(struct dpu_encoder_phys_ops 
>> *ops)
>>   {
>>       ops->is_master = dpu_encoder_phys_wb_is_master;
>> -    ops->atomic_mode_set = dpu_encoder_phys_wb_atomic_mode_set;
>>       ops->enable = dpu_encoder_phys_wb_enable;
>>       ops->disable = dpu_encoder_phys_wb_disable;
>>       ops->destroy = dpu_encoder_phys_wb_destroy;
>> @@ -715,6 +705,7 @@ struct dpu_encoder_phys *dpu_encoder_phys_wb_init(
>>       dpu_encoder_phys_wb_init_ops(&phys_enc->ops);
>>       phys_enc->intf_mode = INTF_MODE_WB_LINE;
>> +    phys_enc->irq[INTR_IDX_WB_DONE] = 
>> phys_enc->hw_wb->caps->intr_wb_done;
>>       atomic_set(&wb_enc->wbirq_refcount, 0);

-- 
With best wishes
Dmitry

Re: [Freedreno] [PATCH 3/3] drm/msm/dpu: drop dpu_encoder_phys_ops.atomic_mode_set

[Abhinav Kumar]

On 8/17/2023 11:50 AM, Dmitry Baryshkov wrote:
> On 08/08/2023 02:49, Abhinav Kumar wrote:
>>
>>
>> On 6/4/2023 7:45 AM, Dmitry Baryshkov wrote:
>>> The atomic_mode_set() callback only sets the phys_enc's IRQ data. As the
>>> INTF and WB are statically allocated to each encoder/phys_enc, drop the
>>> atomic_mode_set callback and set the IRQs during encoder init.
>>>
>>> For the CMD panel usecase some of IRQ indexes depend on the selected
>>> resources. Move setting them to the irq_enable() callback.
>>>
>>
>> The irq_enable() callback is called from the 
>> dpu_encoder_virt_atomic_enable() after the phys layer's enable.
>>
>> Thats late.
>>
>> So lets consider the case where command mode panel's clock is powered 
>> from bootloader (quite common).
>>
>> Now, as soon as the tearcheck is configured and interface is ON from 
>> the phys's enable(), nothing prevents / should prevent the interrupt 
>> from firing.
> 
> Please note that interrupt callbacks are also registered from the 
> irq_control(). There is no way the interrupt can fire beforehand (and 
> the call chain is dpu_encoder_virt_atomic_enable() -> 
> dpu_encoder_resource_control() -> _dpu_encoder_resource_control_helper() 
> -> _dpu_encoder_irq_control() -> irq_control().
> 
> If we ever want to move irq_control() to be called before phys's 
> enable() callbacks, this will be a separate patchset, involving 
> refactoring of dpu_encoder_resource_control().
> 

Ack, I will rebase my patches on top of this and re-test it.

Then will give my R-b, tested-by tags by Monday.

>>
>> So I feel / think mode_set is the correct location to assign these.
>>
>> I can ack patches 1 and 2 but I think you did those mainly for this 
>> one, so I would like to get some clarity on this part.
>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   |  2 --
>>>   .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |  5 ---
>>>   .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  | 32 ++++++++-----------
>>>   .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  | 13 ++------
>>>   .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c   | 11 +------
>>>   5 files changed, 17 insertions(+), 46 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> index cc61f0cf059d..6b5c80dc5967 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> @@ -1148,8 +1148,6 @@ static void 
>>> dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
>>>           phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]);
>>>           phys->cached_mode = crtc_state->adjusted_mode;
>>> -        if (phys->ops.atomic_mode_set)
>>> -            phys->ops.atomic_mode_set(phys, crtc_state, conn_state);
>>>       }
>>>   }
>>> 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 faf033cd086e..24dbc28be4f8 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>>> @@ -67,8 +67,6 @@ struct dpu_encoder_phys;
>>>    * @is_master:            Whether this phys_enc is the current master
>>>    *                encoder. Can be switched at enable time. Based
>>>    *                on split_role and current mode (CMD/VID).
>>> - * @atomic_mode_set:        DRM Call. Set a DRM mode.
>>> - *                This likely caches the mode, for use at enable.
>>>    * @enable:            DRM Call. Enable a DRM mode.
>>>    * @disable:            DRM Call. Disable mode.
>>>    * @atomic_check:        DRM Call. Atomic check new DRM state.
>>> @@ -95,9 +93,6 @@ struct dpu_encoder_phys;
>>>   struct dpu_encoder_phys_ops {
>>>       void (*prepare_commit)(struct dpu_encoder_phys *encoder);
>>>       bool (*is_master)(struct dpu_encoder_phys *encoder);
>>> -    void (*atomic_mode_set)(struct dpu_encoder_phys *encoder,
>>> -            struct drm_crtc_state *crtc_state,
>>> -            struct drm_connector_state *conn_state);
>>>       void (*enable)(struct dpu_encoder_phys *encoder);
>>>       void (*disable)(struct dpu_encoder_phys *encoder);
>>>       int (*atomic_check)(struct dpu_encoder_phys *encoder,
>>> 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 3422b49f23c2..a0b7d8803e94 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
>>> @@ -140,23 +140,6 @@ static void 
>>> dpu_encoder_phys_cmd_underrun_irq(void *arg, int irq_idx)
>>>       dpu_encoder_underrun_callback(phys_enc->parent, phys_enc);
>>>   }
>>> -static void dpu_encoder_phys_cmd_atomic_mode_set(
>>> -        struct dpu_encoder_phys *phys_enc,
>>> -        struct drm_crtc_state *crtc_state,
>>> -        struct drm_connector_state *conn_state)
>>> -{
>>> -    phys_enc->irq[INTR_IDX_CTL_START] = 
>>> phys_enc->hw_ctl->caps->intr_start;
>>> -
>>> -    phys_enc->irq[INTR_IDX_PINGPONG] = 
>>> phys_enc->hw_pp->caps->intr_done;
>>> -
>>> -    if (phys_enc->has_intf_te)
>>> -        phys_enc->irq[INTR_IDX_RDPTR] = 
>>> phys_enc->hw_intf->cap->intr_tear_rd_ptr;
>>> -    else
>>> -        phys_enc->irq[INTR_IDX_RDPTR] = 
>>> phys_enc->hw_pp->caps->intr_rdptr;
>>> -
>>> -    phys_enc->irq[INTR_IDX_UNDERRUN] = 
>>> phys_enc->hw_intf->cap->intr_underrun;
>>> -}
>>> -
>>>   static int _dpu_encoder_phys_cmd_handle_ppdone_timeout(
>>>           struct dpu_encoder_phys *phys_enc)
>>>   {
>>> @@ -287,6 +270,14 @@ static void 
>>> dpu_encoder_phys_cmd_irq_enable(struct dpu_encoder_phys *phys_enc)
>>>                       true,
>>>                       atomic_read(&phys_enc->vblank_refcount));
>>> +    phys_enc->irq[INTR_IDX_CTL_START] = 
>>> phys_enc->hw_ctl->caps->intr_start;
>>> +    phys_enc->irq[INTR_IDX_PINGPONG] = 
>>> phys_enc->hw_pp->caps->intr_done;
>>> +
>>> +    if (phys_enc->has_intf_te)
>>> +        phys_enc->irq[INTR_IDX_RDPTR] = 
>>> phys_enc->hw_intf->cap->intr_tear_rd_ptr;
>>> +    else
>>> +        phys_enc->irq[INTR_IDX_RDPTR] = 
>>> phys_enc->hw_pp->caps->intr_rdptr;
>>> +
>>>       dpu_core_irq_register_callback(phys_enc->dpu_kms,
>>>                          phys_enc->irq[INTR_IDX_PINGPONG],
>>>                          dpu_encoder_phys_cmd_pp_tx_done_irq,
>>> @@ -318,6 +309,10 @@ static void 
>>> dpu_encoder_phys_cmd_irq_disable(struct dpu_encoder_phys *phys_enc)
>>>       dpu_core_irq_unregister_callback(phys_enc->dpu_kms, 
>>> phys_enc->irq[INTR_IDX_UNDERRUN]);
>>>       dpu_encoder_phys_cmd_control_vblank_irq(phys_enc, false);
>>>       dpu_core_irq_unregister_callback(phys_enc->dpu_kms, 
>>> phys_enc->irq[INTR_IDX_PINGPONG]);
>>> +
>>> +    phys_enc->irq[INTR_IDX_CTL_START] = -1;
>>> +    phys_enc->irq[INTR_IDX_PINGPONG] = -1;
>>> +    phys_enc->irq[INTR_IDX_RDPTR] = -1;
>>>   }
>>>   static void dpu_encoder_phys_cmd_tearcheck_config(
>>> @@ -737,7 +732,6 @@ static void dpu_encoder_phys_cmd_init_ops(
>>>           struct dpu_encoder_phys_ops *ops)
>>>   {
>>>       ops->is_master = dpu_encoder_phys_cmd_is_master;
>>> -    ops->atomic_mode_set = dpu_encoder_phys_cmd_atomic_mode_set;
>>>       ops->enable = dpu_encoder_phys_cmd_enable;
>>>       ops->disable = dpu_encoder_phys_cmd_disable;
>>>       ops->destroy = dpu_encoder_phys_cmd_destroy;
>>> @@ -775,6 +769,8 @@ struct dpu_encoder_phys *dpu_encoder_phys_cmd_init(
>>>       dpu_encoder_phys_cmd_init_ops(&phys_enc->ops);
>>>       phys_enc->intf_mode = INTF_MODE_CMD;
>>> +    phys_enc->irq[INTR_IDX_UNDERRUN] = 
>>> phys_enc->hw_intf->cap->intr_underrun;
>>> +
>>>       cmd_enc->stream_sel = 0;
>>>       phys_enc->has_intf_te = test_bit(DPU_INTF_TE,
>>> 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 a550b290246c..3f2e0ebe3cfc 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
>>> @@ -348,16 +348,6 @@ static bool 
>>> dpu_encoder_phys_vid_needs_single_flush(
>>>       return phys_enc->split_role != ENC_ROLE_SOLO;
>>>   }
>>> -static void dpu_encoder_phys_vid_atomic_mode_set(
>>> -        struct dpu_encoder_phys *phys_enc,
>>> -        struct drm_crtc_state *crtc_state,
>>> -        struct drm_connector_state *conn_state)
>>> -{
>>> -    phys_enc->irq[INTR_IDX_VSYNC] = phys_enc->hw_intf->cap->intr_vsync;
>>> -
>>> -    phys_enc->irq[INTR_IDX_UNDERRUN] = 
>>> phys_enc->hw_intf->cap->intr_underrun;
>>> -}
>>> -
>>>   static int dpu_encoder_phys_vid_control_vblank_irq(
>>>           struct dpu_encoder_phys *phys_enc,
>>>           bool enable)
>>> @@ -684,7 +674,6 @@ static int dpu_encoder_phys_vid_get_frame_count(
>>>   static void dpu_encoder_phys_vid_init_ops(struct 
>>> dpu_encoder_phys_ops *ops)
>>>   {
>>>       ops->is_master = dpu_encoder_phys_vid_is_master;
>>> -    ops->atomic_mode_set = dpu_encoder_phys_vid_atomic_mode_set;
>>>       ops->enable = dpu_encoder_phys_vid_enable;
>>>       ops->disable = dpu_encoder_phys_vid_disable;
>>>       ops->destroy = dpu_encoder_phys_vid_destroy;
>>> @@ -723,6 +712,8 @@ struct dpu_encoder_phys *dpu_encoder_phys_vid_init(
>>>       dpu_encoder_phys_vid_init_ops(&phys_enc->ops);
>>>       phys_enc->intf_mode = INTF_MODE_VIDEO;
>>> +    phys_enc->irq[INTR_IDX_VSYNC] = phys_enc->hw_intf->cap->intr_vsync;
>>> +    phys_enc->irq[INTR_IDX_UNDERRUN] = 
>>> phys_enc->hw_intf->cap->intr_underrun;
>>>       DPU_DEBUG_VIDENC(phys_enc, "created intf idx:%d\n", 
>>> p->hw_intf->idx);
>>> 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 858fe6656c9b..439f645e0bc9 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
>>> @@ -410,15 +410,6 @@ static void 
>>> dpu_encoder_phys_wb_irq_disable(struct dpu_encoder_phys *phys)
>>>           dpu_core_irq_unregister_callback(phys->dpu_kms, 
>>> phys->irq[INTR_IDX_WB_DONE]);
>>>   }
>>> -static void dpu_encoder_phys_wb_atomic_mode_set(
>>> -        struct dpu_encoder_phys *phys_enc,
>>> -        struct drm_crtc_state *crtc_state,
>>> -        struct drm_connector_state *conn_state)
>>> -{
>>> -
>>> -    phys_enc->irq[INTR_IDX_WB_DONE] = 
>>> phys_enc->hw_wb->caps->intr_wb_done;
>>> -}
>>> -
>>>   static void _dpu_encoder_phys_wb_handle_wbdone_timeout(
>>>           struct dpu_encoder_phys *phys_enc)
>>>   {
>>> @@ -668,7 +659,6 @@ static bool 
>>> dpu_encoder_phys_wb_is_valid_for_commit(struct dpu_encoder_phys *phy
>>>   static void dpu_encoder_phys_wb_init_ops(struct 
>>> dpu_encoder_phys_ops *ops)
>>>   {
>>>       ops->is_master = dpu_encoder_phys_wb_is_master;
>>> -    ops->atomic_mode_set = dpu_encoder_phys_wb_atomic_mode_set;
>>>       ops->enable = dpu_encoder_phys_wb_enable;
>>>       ops->disable = dpu_encoder_phys_wb_disable;
>>>       ops->destroy = dpu_encoder_phys_wb_destroy;
>>> @@ -715,6 +705,7 @@ struct dpu_encoder_phys *dpu_encoder_phys_wb_init(
>>>       dpu_encoder_phys_wb_init_ops(&phys_enc->ops);
>>>       phys_enc->intf_mode = INTF_MODE_WB_LINE;
>>> +    phys_enc->irq[INTR_IDX_WB_DONE] = 
>>> phys_enc->hw_wb->caps->intr_wb_done;
>>>       atomic_set(&wb_enc->wbirq_refcount, 0);
> 

Re: [Freedreno] [PATCH 3/3] drm/msm/dpu: drop dpu_encoder_phys_ops.atomic_mode_set

[Abhinav Kumar]

On 8/17/2023 12:27 PM, Abhinav Kumar wrote:
> 
> 
> On 8/17/2023 11:50 AM, Dmitry Baryshkov wrote:
>> On 08/08/2023 02:49, Abhinav Kumar wrote:
>>>
>>>
>>> On 6/4/2023 7:45 AM, Dmitry Baryshkov wrote:
>>>> The atomic_mode_set() callback only sets the phys_enc's IRQ data. As 
>>>> the
>>>> INTF and WB are statically allocated to each encoder/phys_enc, drop the
>>>> atomic_mode_set callback and set the IRQs during encoder init.
>>>>
>>>> For the CMD panel usecase some of IRQ indexes depend on the selected
>>>> resources. Move setting them to the irq_enable() callback.
>>>>
>>>
>>> The irq_enable() callback is called from the 
>>> dpu_encoder_virt_atomic_enable() after the phys layer's enable.
>>>
>>> Thats late.
>>>
>>> So lets consider the case where command mode panel's clock is powered 
>>> from bootloader (quite common).
>>>
>>> Now, as soon as the tearcheck is configured and interface is ON from 
>>> the phys's enable(), nothing prevents / should prevent the interrupt 
>>> from firing.
>>
>> Please note that interrupt callbacks are also registered from the 
>> irq_control(). There is no way the interrupt can fire beforehand (and 
>> the call chain is dpu_encoder_virt_atomic_enable() -> 
>> dpu_encoder_resource_control() -> 
>> _dpu_encoder_resource_control_helper() -> _dpu_encoder_irq_control() 
>> -> irq_control().
>>
>> If we ever want to move irq_control() to be called before phys's 
>> enable() callbacks, this will be a separate patchset, involving 
>> refactoring of dpu_encoder_resource_control().
>>
> 
> Ack, I will rebase my patches on top of this and re-test it.
> 
> Then will give my R-b, tested-by tags by Monday.
> 

Sorry for the delay on this.

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

Made sure that sc7280 boots up fine and kms_writeback works