[PATCH 0/7] drm/msm/dpu: drop DPU_INTF_TE and DPU_PINGPONG_TE

[PATCH 0/7] drm/msm/dpu: drop DPU_INTF_TE and DPU_PINGPONG_TE

[Dmitry Baryshkov]

Drop two feature flags, DPU_INTF_TE and DPU_PINGPONG_TE, in favour of
performing the MDSS revision checks instead.

Dependencies: [1], [2]

[1] https://patchwork.freedesktop.org/series/118088/
[2] https://patchwork.freedesktop.org/series/118836/

Dmitry Baryshkov (7):
  drm/msm/dpu: enable PINGPONG TE operations only when supported by HW
  drm/msm/dpu: drop the DPU_PINGPONG_TE flag
  drm/msm/dpu: inline _setup_intf_ops()
  drm/msm/dpu: enable INTF TE operations only when supported by HW
  drm/msm/dpu: drop DPU_INTF_TE feature flag
  drm/msm/dpu: drop useless check from
    dpu_encoder_phys_cmd_te_rd_ptr_irq()
  drm/msm/dpu: move INTF tearing checks to dpu_encoder_phys_cmd_init

 .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  | 49 +++++++++----------
 .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c    |  3 +-
 .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h    |  6 +--
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c   | 47 ++++++++----------
 .../gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c   |  2 +-
 5 files changed, 47 insertions(+), 60 deletions(-)


base-commit: be4dacf4eee1c4e2e91586e75e95b2852274dc58
prerequisite-patch-id: be3f3e5df89f9f2cc6b6289a83422d65e29d4900
prerequisite-patch-id: 59cd61ccd3cde7218fe3db6a8c672faafb7167f5
prerequisite-patch-id: d493b9bd85d518c15ca94f22174cb5ab2e2443d9
prerequisite-patch-id: 6a5bf3083c3da70ca110c17d54027e96c0335636
prerequisite-patch-id: 670883101f3b5dca122501f2828d8e45a6843b38
prerequisite-patch-id: 5dacdaf8ba4b369b966ca341db6691b208476fa1
prerequisite-patch-id: 5d8ff96e0fbea3358931d9d1fcb1bf114ae172df
prerequisite-patch-id: 9d7a4964337aee22c325fa04424f1e20946e1bb4
prerequisite-patch-id: 7310e0a9f3aa611cb080930a4c8247ced69ed5d5
prerequisite-patch-id: ec7e1b84ef2780c43cf59e2c2bf638d7eff188fd
-- 
2.39.2

[PATCH 1/7] drm/msm/dpu: enable PINGPONG TE operations only when supported by HW

[Dmitry Baryshkov]

The DPU_PINGPONG_TE bit is set for all PINGPONG blocks on DPU < 5.0.
Rather than checking for the flag, check for the presense of the
corresponding interrupt line.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
index 9298c166b213..912a3bdf8ad4 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
@@ -296,7 +296,7 @@ struct dpu_hw_pingpong *dpu_hw_pingpong_init(const struct dpu_pingpong_cfg *cfg,
 	c->idx = cfg->id;
 	c->caps = cfg;
 
-	if (test_bit(DPU_PINGPONG_TE, &cfg->features)) {
+	if (cfg->intr_rdptr) {
 		c->ops.enable_tearcheck = dpu_hw_pp_enable_te;
 		c->ops.disable_tearcheck = dpu_hw_pp_disable_te;
 		c->ops.connect_external_te = dpu_hw_pp_connect_external_te;
-- 
2.39.2

[PATCH 2/7] drm/msm/dpu: drop the DPU_PINGPONG_TE flag

[Dmitry Baryshkov]

The DPU_PINGPONG_TE flag became unused, we can drop it now.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 2 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 4 +---
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
index 3ff07d7cbf4b..c19dc70d4456 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -79,7 +79,7 @@
 	(BIT(DPU_DIM_LAYER) | BIT(DPU_MIXER_COMBINED_ALPHA))
 
 #define PINGPONG_SDM845_MASK \
-	(BIT(DPU_PINGPONG_DITHER) | BIT(DPU_PINGPONG_TE) | BIT(DPU_PINGPONG_DSC))
+	(BIT(DPU_PINGPONG_DITHER) | BIT(DPU_PINGPONG_DSC))
 
 #define PINGPONG_SDM845_TE2_MASK \
 	(PINGPONG_SDM845_MASK | BIT(DPU_PINGPONG_TE2))
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
index 945b88c5ab58..a6f8eee58b92 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
@@ -136,7 +136,6 @@ enum {
 
 /**
  * PINGPONG sub-blocks
- * @DPU_PINGPONG_TE         Tear check block
  * @DPU_PINGPONG_TE2        Additional tear check block for split pipes
  * @DPU_PINGPONG_SPLIT      PP block supports split fifo
  * @DPU_PINGPONG_SLAVE      PP block is a suitable slave for split fifo
@@ -145,8 +144,7 @@ enum {
  * @DPU_PINGPONG_MAX
  */
 enum {
-	DPU_PINGPONG_TE = 0x1,
-	DPU_PINGPONG_TE2,
+	DPU_PINGPONG_TE2 = 0x1,
 	DPU_PINGPONG_SPLIT,
 	DPU_PINGPONG_SLAVE,
 	DPU_PINGPONG_DITHER,
-- 
2.39.2

[PATCH 3/7] drm/msm/dpu: inline _setup_intf_ops()

[Dmitry Baryshkov]

Inline the _setup_intf_ops() function, it makes it easier to handle
different conditions involving INTF configuration.

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

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
index 8ec6505d9e78..7ca772791a73 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
@@ -524,31 +524,6 @@ static void dpu_hw_intf_program_intf_cmd_cfg(struct dpu_hw_intf *ctx,
 	DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2);
 }
 
-static void _setup_intf_ops(struct dpu_hw_intf_ops *ops,
-		unsigned long cap, const struct dpu_mdss_version *mdss_rev)
-{
-	ops->setup_timing_gen = dpu_hw_intf_setup_timing_engine;
-	ops->setup_prg_fetch  = dpu_hw_intf_setup_prg_fetch;
-	ops->get_status = dpu_hw_intf_get_status;
-	ops->enable_timing = dpu_hw_intf_enable_timing_engine;
-	ops->get_line_count = dpu_hw_intf_get_line_count;
-	if (cap & BIT(DPU_INTF_INPUT_CTRL))
-		ops->bind_pingpong_blk = dpu_hw_intf_bind_pingpong_blk;
-	ops->setup_misr = dpu_hw_intf_setup_misr;
-	ops->collect_misr = dpu_hw_intf_collect_misr;
-
-	if (cap & BIT(DPU_INTF_TE)) {
-		ops->enable_tearcheck = dpu_hw_intf_enable_te;
-		ops->disable_tearcheck = dpu_hw_intf_disable_te;
-		ops->connect_external_te = dpu_hw_intf_connect_external_te;
-		ops->vsync_sel = dpu_hw_intf_vsync_sel;
-		ops->disable_autorefresh = dpu_hw_intf_disable_autorefresh;
-	}
-
-	if (mdss_rev->core_major_ver >= 7)
-		ops->program_intf_cmd_cfg = dpu_hw_intf_program_intf_cmd_cfg;
-}
-
 struct dpu_hw_intf *dpu_hw_intf_init(const struct dpu_intf_cfg *cfg,
 		void __iomem *addr, const struct dpu_mdss_version *mdss_rev)
 {
@@ -571,7 +546,27 @@ struct dpu_hw_intf *dpu_hw_intf_init(const struct dpu_intf_cfg *cfg,
 	 */
 	c->idx = cfg->id;
 	c->cap = cfg;
-	_setup_intf_ops(&c->ops, c->cap->features, mdss_rev);
+
+	c->ops.setup_timing_gen = dpu_hw_intf_setup_timing_engine;
+	c->ops.setup_prg_fetch  = dpu_hw_intf_setup_prg_fetch;
+	c->ops.get_status = dpu_hw_intf_get_status;
+	c->ops.enable_timing = dpu_hw_intf_enable_timing_engine;
+	c->ops.get_line_count = dpu_hw_intf_get_line_count;
+	if (cfg->features & BIT(DPU_INTF_INPUT_CTRL))
+		c->ops.bind_pingpong_blk = dpu_hw_intf_bind_pingpong_blk;
+	c->ops.setup_misr = dpu_hw_intf_setup_misr;
+	c->ops.collect_misr = dpu_hw_intf_collect_misr;
+
+	if (cfg->features & BIT(DPU_INTF_TE)) {
+		c->ops.enable_tearcheck = dpu_hw_intf_enable_te;
+		c->ops.disable_tearcheck = dpu_hw_intf_disable_te;
+		c->ops.connect_external_te = dpu_hw_intf_connect_external_te;
+		c->ops.vsync_sel = dpu_hw_intf_vsync_sel;
+		c->ops.disable_autorefresh = dpu_hw_intf_disable_autorefresh;
+	}
+
+	if (mdss_rev->core_major_ver >= 7)
+		c->ops.program_intf_cmd_cfg = dpu_hw_intf_program_intf_cmd_cfg;
 
 	return c;
 }
-- 
2.39.2

[PATCH 4/7] drm/msm/dpu: enable INTF TE operations only when supported by HW

[Dmitry Baryshkov]

The DPU_INTF_TE bit is set for all INTF blocks on DPU >= 5.0, however
only INTF_1 and INTF_2 actually support tearing control. Rather than
trying to fix the DPU_INTF_TE, check for the presense of the
corresponding interrupt line.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
index 7ca772791a73..8abdf9553f3b 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
@@ -557,7 +557,7 @@ struct dpu_hw_intf *dpu_hw_intf_init(const struct dpu_intf_cfg *cfg,
 	c->ops.setup_misr = dpu_hw_intf_setup_misr;
 	c->ops.collect_misr = dpu_hw_intf_collect_misr;
 
-	if (cfg->features & BIT(DPU_INTF_TE)) {
+	if (cfg->intr_tear_rd_ptr) {
 		c->ops.enable_tearcheck = dpu_hw_intf_enable_te;
 		c->ops.disable_tearcheck = dpu_hw_intf_disable_te;
 		c->ops.connect_external_te = dpu_hw_intf_connect_external_te;
-- 
2.39.2

[PATCH 5/7] drm/msm/dpu: drop DPU_INTF_TE feature flag

[Dmitry Baryshkov]

Replace the only user of the DPU_INTF_TE feature flag with the direct
DPU version comparison.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 4 ++--
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c       | 1 -
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h       | 2 --
 3 files changed, 2 insertions(+), 5 deletions(-)

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 9589fe719452..60d4dd88725e 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
@@ -776,8 +776,8 @@ struct dpu_encoder_phys *dpu_encoder_phys_cmd_init(
 	phys_enc->intf_mode = INTF_MODE_CMD;
 	cmd_enc->stream_sel = 0;
 
-	phys_enc->has_intf_te = test_bit(DPU_INTF_TE,
-					 &phys_enc->hw_intf->cap->features);
+	if (phys_enc->dpu_kms->catalog->mdss_ver->core_major_ver >= 5)
+		phys_enc->has_intf_te = true;
 
 	atomic_set(&cmd_enc->pending_vblank_cnt, 0);
 	init_waitqueue_head(&cmd_enc->pending_vblank_wq);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
index c19dc70d4456..17426f0f981e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -100,7 +100,6 @@
 
 #define INTF_SC7180_MASK \
 	(BIT(DPU_INTF_INPUT_CTRL) | \
-	 BIT(DPU_INTF_TE) | \
 	 BIT(DPU_INTF_STATUS_SUPPORTED) | \
 	 BIT(DPU_DATA_HCTL_EN))
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
index a6f8eee58b92..69c9099cf5a6 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
@@ -175,7 +175,6 @@ enum {
  * INTF sub-blocks
  * @DPU_INTF_INPUT_CTRL             Supports the setting of pp block from which
  *                                  pixel data arrives to this INTF
- * @DPU_INTF_TE                     INTF block has TE configuration support
  * @DPU_DATA_HCTL_EN                Allows data to be transferred at different rate
  *                                  than video timing
  * @DPU_INTF_STATUS_SUPPORTED       INTF block has INTF_STATUS register
@@ -183,7 +182,6 @@ enum {
  */
 enum {
 	DPU_INTF_INPUT_CTRL = 0x1,
-	DPU_INTF_TE,
 	DPU_DATA_HCTL_EN,
 	DPU_INTF_STATUS_SUPPORTED,
 	DPU_INTF_MAX
-- 
2.39.2

[PATCH 6/7] drm/msm/dpu: drop useless check from dpu_encoder_phys_cmd_te_rd_ptr_irq()

[Dmitry Baryshkov]

The dpu_encoder_phys_cmd_te_rd_ptr_irq() function uses neither hw_intf
nor hw_pp data, so we can drop the corresponding check.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 8 --------
 1 file changed, 8 deletions(-)

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 60d4dd88725e..04a1106101a7 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
@@ -108,14 +108,6 @@ static void dpu_encoder_phys_cmd_te_rd_ptr_irq(void *arg)
 	struct dpu_encoder_phys *phys_enc = arg;
 	struct dpu_encoder_phys_cmd *cmd_enc;
 
-	if (phys_enc->has_intf_te) {
-		if (!phys_enc->hw_intf)
-			return;
-	} else {
-		if (!phys_enc->hw_pp)
-			return;
-	}
-
 	DPU_ATRACE_BEGIN("rd_ptr_irq");
 	cmd_enc = to_dpu_encoder_phys_cmd(phys_enc);
 
-- 
2.39.2

[PATCH 7/7] drm/msm/dpu: move INTF tearing checks to dpu_encoder_phys_cmd_init

[Dmitry Baryshkov]

As the INTF is fixed at the encoder creation time, we can move the
check whether INTF supports tearchck to dpu_encoder_phys_cmd_init().
This function can return an error if INTF doesn't have required feature.
Performing this check in dpu_encoder_phys_cmd_tearcheck_config() is less
useful, as this function returns void.

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

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 04a1106101a7..e1dd0e1b4793 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
@@ -325,24 +325,17 @@ static void dpu_encoder_phys_cmd_tearcheck_config(
 	unsigned long vsync_hz;
 	struct dpu_kms *dpu_kms;
 
-	if (phys_enc->has_intf_te) {
-		if (!phys_enc->hw_intf ||
-		    !phys_enc->hw_intf->ops.enable_tearcheck) {
-			DPU_DEBUG_CMDENC(cmd_enc, "tearcheck not supported\n");
-			return;
-		}
-
-		DPU_DEBUG_CMDENC(cmd_enc, "");
-	} else {
-		if (!phys_enc->hw_pp ||
-		    !phys_enc->hw_pp->ops.enable_tearcheck) {
-			DPU_DEBUG_CMDENC(cmd_enc, "tearcheck not supported\n");
-			return;
-		}
-
-		DPU_DEBUG_CMDENC(cmd_enc, "pp %d\n", phys_enc->hw_pp->idx - PINGPONG_0);
+	if (!phys_enc->has_intf_te &&
+	    (!phys_enc->hw_pp ||
+	     !phys_enc->hw_pp->ops.enable_tearcheck)) {
+		DPU_DEBUG_CMDENC(cmd_enc, "tearcheck not supported\n");
+		return;
 	}
 
+	DPU_DEBUG_CMDENC(cmd_enc, "intf %d pp %d\n",
+			 phys_enc->hw_intf->idx - INTF_0,
+			 phys_enc->hw_pp->idx - PINGPONG_0);
+
 	mode = &phys_enc->cached_mode;
 
 	dpu_kms = phys_enc->dpu_kms;
@@ -768,9 +761,21 @@ struct dpu_encoder_phys *dpu_encoder_phys_cmd_init(
 	phys_enc->intf_mode = INTF_MODE_CMD;
 	cmd_enc->stream_sel = 0;
 
+	if (!phys_enc->hw_intf) {
+		DPU_ERROR_CMDENC(cmd_enc, "no INTF provided\n");
+
+		return ERR_PTR(-EINVAL);
+	}
+
 	if (phys_enc->dpu_kms->catalog->mdss_ver->core_major_ver >= 5)
 		phys_enc->has_intf_te = true;
 
+	if (phys_enc->has_intf_te && !phys_enc->hw_intf->ops.enable_tearcheck) {
+		DPU_ERROR_CMDENC(cmd_enc, "tearcheck not supported\n");
+
+		return ERR_PTR(-EINVAL);
+	}
+
 	atomic_set(&cmd_enc->pending_vblank_cnt, 0);
 	init_waitqueue_head(&cmd_enc->pending_vblank_wq);
 
-- 
2.39.2

Re: [PATCH 1/7] drm/msm/dpu: enable PINGPONG TE operations only when supported by HW

[Marijn Suijten]

On 2023-07-27 19:20:58, Dmitry Baryshkov wrote:
> The DPU_PINGPONG_TE bit is set for all PINGPONG blocks on DPU < 5.0.
> Rather than checking for the flag, check for the presense of the
> corresponding interrupt line.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

That's a smart use of the interrupt field.  I both like it, and I do
not.  While we didn't do any validation for consistency previously, this
means we now have multiple ways of controlling available "features":

- Feature flags on hardware blocks;
- Presence of certain IRQs;
- DPU core revision.

Maybe that is more confusing to follow?  Regardless of that I'm
convinced that this patch does what it's supposed to and gets rid of
some ambiguity.  Maybe a comment above the IF explaining the "PP TE"
feature could alleviate the above concerns thoo.  Hence:

Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>

> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
> index 9298c166b213..912a3bdf8ad4 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
> @@ -296,7 +296,7 @@ struct dpu_hw_pingpong *dpu_hw_pingpong_init(const struct dpu_pingpong_cfg *cfg,
>  	c->idx = cfg->id;
>  	c->caps = cfg;
>  
> -	if (test_bit(DPU_PINGPONG_TE, &cfg->features)) {
> +	if (cfg->intr_rdptr) {
>  		c->ops.enable_tearcheck = dpu_hw_pp_enable_te;
>  		c->ops.disable_tearcheck = dpu_hw_pp_disable_te;
>  		c->ops.connect_external_te = dpu_hw_pp_connect_external_te;
> -- 
> 2.39.2
> 

Re: [PATCH 1/7] drm/msm/dpu: enable PINGPONG TE operations only when supported by HW

[Marijn Suijten]

On 2023-07-27 19:20:58, Dmitry Baryshkov wrote:
> The DPU_PINGPONG_TE bit is set for all PINGPONG blocks on DPU < 5.0.
> Rather than checking for the flag, check for the presense of the
> corresponding interrupt line.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
> index 9298c166b213..912a3bdf8ad4 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
> @@ -296,7 +296,7 @@ struct dpu_hw_pingpong *dpu_hw_pingpong_init(const struct dpu_pingpong_cfg *cfg,
>  	c->idx = cfg->id;
>  	c->caps = cfg;

In hindsight, maybe there's one patch missing from this series.  You
inlined _setup_intf_ops() later, but there's no patch inlining
_setup_pingpong_ops() which looks to be required for applying this
patch.

- Marijn

>  
> -	if (test_bit(DPU_PINGPONG_TE, &cfg->features)) {
> +	if (cfg->intr_rdptr) {
>  		c->ops.enable_tearcheck = dpu_hw_pp_enable_te;
>  		c->ops.disable_tearcheck = dpu_hw_pp_disable_te;
>  		c->ops.connect_external_te = dpu_hw_pp_connect_external_te;
> -- 
> 2.39.2
> 

Re: [PATCH 2/7] drm/msm/dpu: drop the DPU_PINGPONG_TE flag

[Marijn Suijten]

On 2023-07-27 19:20:59, Dmitry Baryshkov wrote:
> The DPU_PINGPONG_TE flag became unused, we can drop it now.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>

> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 2 +-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 4 +---
>  2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> index 3ff07d7cbf4b..c19dc70d4456 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> @@ -79,7 +79,7 @@
>  	(BIT(DPU_DIM_LAYER) | BIT(DPU_MIXER_COMBINED_ALPHA))
>  
>  #define PINGPONG_SDM845_MASK \
> -	(BIT(DPU_PINGPONG_DITHER) | BIT(DPU_PINGPONG_TE) | BIT(DPU_PINGPONG_DSC))
> +	(BIT(DPU_PINGPONG_DITHER) | BIT(DPU_PINGPONG_DSC))
>  
>  #define PINGPONG_SDM845_TE2_MASK \
>  	(PINGPONG_SDM845_MASK | BIT(DPU_PINGPONG_TE2))
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> index 945b88c5ab58..a6f8eee58b92 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> @@ -136,7 +136,6 @@ enum {
>  
>  /**
>   * PINGPONG sub-blocks
> - * @DPU_PINGPONG_TE         Tear check block
>   * @DPU_PINGPONG_TE2        Additional tear check block for split pipes
>   * @DPU_PINGPONG_SPLIT      PP block supports split fifo
>   * @DPU_PINGPONG_SLAVE      PP block is a suitable slave for split fifo
> @@ -145,8 +144,7 @@ enum {
>   * @DPU_PINGPONG_MAX
>   */
>  enum {
> -	DPU_PINGPONG_TE = 0x1,
> -	DPU_PINGPONG_TE2,
> +	DPU_PINGPONG_TE2 = 0x1,
>  	DPU_PINGPONG_SPLIT,
>  	DPU_PINGPONG_SLAVE,
>  	DPU_PINGPONG_DITHER,
> -- 
> 2.39.2
> 

Re: [PATCH 3/7] drm/msm/dpu: inline _setup_intf_ops()

[Marijn Suijten]

On 2023-07-27 19:21:00, Dmitry Baryshkov wrote:
> Inline the _setup_intf_ops() function, it makes it easier to handle
> different conditions involving INTF configuration.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>

> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 47 +++++++++------------
>  1 file changed, 21 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> index 8ec6505d9e78..7ca772791a73 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> @@ -524,31 +524,6 @@ static void dpu_hw_intf_program_intf_cmd_cfg(struct dpu_hw_intf *ctx,
>  	DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2);
>  }
>  
> -static void _setup_intf_ops(struct dpu_hw_intf_ops *ops,
> -		unsigned long cap, const struct dpu_mdss_version *mdss_rev)
> -{
> -	ops->setup_timing_gen = dpu_hw_intf_setup_timing_engine;
> -	ops->setup_prg_fetch  = dpu_hw_intf_setup_prg_fetch;
> -	ops->get_status = dpu_hw_intf_get_status;
> -	ops->enable_timing = dpu_hw_intf_enable_timing_engine;
> -	ops->get_line_count = dpu_hw_intf_get_line_count;
> -	if (cap & BIT(DPU_INTF_INPUT_CTRL))
> -		ops->bind_pingpong_blk = dpu_hw_intf_bind_pingpong_blk;
> -	ops->setup_misr = dpu_hw_intf_setup_misr;
> -	ops->collect_misr = dpu_hw_intf_collect_misr;
> -
> -	if (cap & BIT(DPU_INTF_TE)) {
> -		ops->enable_tearcheck = dpu_hw_intf_enable_te;
> -		ops->disable_tearcheck = dpu_hw_intf_disable_te;
> -		ops->connect_external_te = dpu_hw_intf_connect_external_te;
> -		ops->vsync_sel = dpu_hw_intf_vsync_sel;
> -		ops->disable_autorefresh = dpu_hw_intf_disable_autorefresh;
> -	}
> -
> -	if (mdss_rev->core_major_ver >= 7)
> -		ops->program_intf_cmd_cfg = dpu_hw_intf_program_intf_cmd_cfg;
> -}
> -
>  struct dpu_hw_intf *dpu_hw_intf_init(const struct dpu_intf_cfg *cfg,
>  		void __iomem *addr, const struct dpu_mdss_version *mdss_rev)
>  {
> @@ -571,7 +546,27 @@ struct dpu_hw_intf *dpu_hw_intf_init(const struct dpu_intf_cfg *cfg,
>  	 */
>  	c->idx = cfg->id;
>  	c->cap = cfg;
> -	_setup_intf_ops(&c->ops, c->cap->features, mdss_rev);
> +
> +	c->ops.setup_timing_gen = dpu_hw_intf_setup_timing_engine;
> +	c->ops.setup_prg_fetch  = dpu_hw_intf_setup_prg_fetch;
> +	c->ops.get_status = dpu_hw_intf_get_status;
> +	c->ops.enable_timing = dpu_hw_intf_enable_timing_engine;
> +	c->ops.get_line_count = dpu_hw_intf_get_line_count;
> +	if (cfg->features & BIT(DPU_INTF_INPUT_CTRL))
> +		c->ops.bind_pingpong_blk = dpu_hw_intf_bind_pingpong_blk;

While at it, could we sort these down with the other conditional
callbacks?

> +	c->ops.setup_misr = dpu_hw_intf_setup_misr;
> +	c->ops.collect_misr = dpu_hw_intf_collect_misr;
> +
> +	if (cfg->features & BIT(DPU_INTF_TE)) {

Any clue why we're not using test_bit()?  Feels a bit inconsistent.

> +		c->ops.enable_tearcheck = dpu_hw_intf_enable_te;
> +		c->ops.disable_tearcheck = dpu_hw_intf_disable_te;
> +		c->ops.connect_external_te = dpu_hw_intf_connect_external_te;
> +		c->ops.vsync_sel = dpu_hw_intf_vsync_sel;
> +		c->ops.disable_autorefresh = dpu_hw_intf_disable_autorefresh;
> +	}
> +
> +	if (mdss_rev->core_major_ver >= 7)
> +		c->ops.program_intf_cmd_cfg = dpu_hw_intf_program_intf_cmd_cfg;
>  
>  	return c;
>  }
> -- 
> 2.39.2
> 

Re: [PATCH 4/7] drm/msm/dpu: enable INTF TE operations only when supported by HW

[Marijn Suijten]

On 2023-07-27 19:21:01, Dmitry Baryshkov wrote:
> The DPU_INTF_TE bit is set for all INTF blocks on DPU >= 5.0, however
> only INTF_1 and INTF_2 actually support tearing control. Rather than
> trying to fix the DPU_INTF_TE, check for the presense of the

I would more exactly expand "fix" to "Rather than specifying that
feature bit on DSI INTF_1 and INTF_2 exclusively..."

> corresponding interrupt line.

... which the catalog will only provide on DPU >= 5.0.

> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>

> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> index 7ca772791a73..8abdf9553f3b 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> @@ -557,7 +557,7 @@ struct dpu_hw_intf *dpu_hw_intf_init(const struct dpu_intf_cfg *cfg,
>  	c->ops.setup_misr = dpu_hw_intf_setup_misr;
>  	c->ops.collect_misr = dpu_hw_intf_collect_misr;
>  
> -	if (cfg->features & BIT(DPU_INTF_TE)) {
> +	if (cfg->intr_tear_rd_ptr) {
>  		c->ops.enable_tearcheck = dpu_hw_intf_enable_te;
>  		c->ops.disable_tearcheck = dpu_hw_intf_disable_te;
>  		c->ops.connect_external_te = dpu_hw_intf_connect_external_te;
> -- 
> 2.39.2
> 

Re: [PATCH 5/7] drm/msm/dpu: drop DPU_INTF_TE feature flag

[Marijn Suijten]

On 2023-07-27 19:21:02, Dmitry Baryshkov wrote:
> Replace the only user of the DPU_INTF_TE feature flag with the direct
> DPU version comparison.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>

> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 4 ++--
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c       | 1 -
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h       | 2 --
>  3 files changed, 2 insertions(+), 5 deletions(-)
> 
> 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 9589fe719452..60d4dd88725e 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
> @@ -776,8 +776,8 @@ struct dpu_encoder_phys *dpu_encoder_phys_cmd_init(
>  	phys_enc->intf_mode = INTF_MODE_CMD;
>  	cmd_enc->stream_sel = 0;
>  
> -	phys_enc->has_intf_te = test_bit(DPU_INTF_TE,
> -					 &phys_enc->hw_intf->cap->features);
> +	if (phys_enc->dpu_kms->catalog->mdss_ver->core_major_ver >= 5)
> +		phys_enc->has_intf_te = true;

We could also check if the INTF block has the callbacks (which it based
on the presence of the interrupt line in the catalog instead), but then
I think we might loose some extra validation which you tidied up in a
later patch in this series?

>  
>  	atomic_set(&cmd_enc->pending_vblank_cnt, 0);
>  	init_waitqueue_head(&cmd_enc->pending_vblank_wq);
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> index c19dc70d4456..17426f0f981e 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> @@ -100,7 +100,6 @@
>  
>  #define INTF_SC7180_MASK \
>  	(BIT(DPU_INTF_INPUT_CTRL) | \
> -	 BIT(DPU_INTF_TE) | \
>  	 BIT(DPU_INTF_STATUS_SUPPORTED) | \
>  	 BIT(DPU_DATA_HCTL_EN))
>  
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> index a6f8eee58b92..69c9099cf5a6 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> @@ -175,7 +175,6 @@ enum {
>   * INTF sub-blocks
>   * @DPU_INTF_INPUT_CTRL             Supports the setting of pp block from which
>   *                                  pixel data arrives to this INTF
> - * @DPU_INTF_TE                     INTF block has TE configuration support
>   * @DPU_DATA_HCTL_EN                Allows data to be transferred at different rate
>   *                                  than video timing
>   * @DPU_INTF_STATUS_SUPPORTED       INTF block has INTF_STATUS register
> @@ -183,7 +182,6 @@ enum {
>   */
>  enum {
>  	DPU_INTF_INPUT_CTRL = 0x1,
> -	DPU_INTF_TE,
>  	DPU_DATA_HCTL_EN,
>  	DPU_INTF_STATUS_SUPPORTED,
>  	DPU_INTF_MAX
> -- 
> 2.39.2
> 

Re: [PATCH 5/7] drm/msm/dpu: drop DPU_INTF_TE feature flag

[Dmitry Baryshkov]

On 27/07/2023 23:14, Marijn Suijten wrote:
> On 2023-07-27 19:21:02, Dmitry Baryshkov wrote:
>> Replace the only user of the DPU_INTF_TE feature flag with the direct
>> DPU version comparison.
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> 
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 4 ++--
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c       | 1 -
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h       | 2 --
>>   3 files changed, 2 insertions(+), 5 deletions(-)
>>
>> 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 9589fe719452..60d4dd88725e 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
>> @@ -776,8 +776,8 @@ struct dpu_encoder_phys *dpu_encoder_phys_cmd_init(
>>   	phys_enc->intf_mode = INTF_MODE_CMD;
>>   	cmd_enc->stream_sel = 0;
>>   
>> -	phys_enc->has_intf_te = test_bit(DPU_INTF_TE,
>> -					 &phys_enc->hw_intf->cap->features);
>> +	if (phys_enc->dpu_kms->catalog->mdss_ver->core_major_ver >= 5)
>> +		phys_enc->has_intf_te = true;
> 
> We could also check if the INTF block has the callbacks (which it based
> on the presence of the interrupt line in the catalog instead), but then
> I think we might loose some extra validation which you tidied up in a
> later patch in this series?

Almost. The logic was the following: we should be using INTF for DPU >= 
5.0. And if we have DPU >= 5.0 and no callbacks, it's an error.

> 
>>   
>>   	atomic_set(&cmd_enc->pending_vblank_cnt, 0);
>>   	init_waitqueue_head(&cmd_enc->pending_vblank_wq);
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>> index c19dc70d4456..17426f0f981e 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>> @@ -100,7 +100,6 @@
>>   
>>   #define INTF_SC7180_MASK \
>>   	(BIT(DPU_INTF_INPUT_CTRL) | \
>> -	 BIT(DPU_INTF_TE) | \
>>   	 BIT(DPU_INTF_STATUS_SUPPORTED) | \
>>   	 BIT(DPU_DATA_HCTL_EN))
>>   
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> index a6f8eee58b92..69c9099cf5a6 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> @@ -175,7 +175,6 @@ enum {
>>    * INTF sub-blocks
>>    * @DPU_INTF_INPUT_CTRL             Supports the setting of pp block from which
>>    *                                  pixel data arrives to this INTF
>> - * @DPU_INTF_TE                     INTF block has TE configuration support
>>    * @DPU_DATA_HCTL_EN                Allows data to be transferred at different rate
>>    *                                  than video timing
>>    * @DPU_INTF_STATUS_SUPPORTED       INTF block has INTF_STATUS register
>> @@ -183,7 +182,6 @@ enum {
>>    */
>>   enum {
>>   	DPU_INTF_INPUT_CTRL = 0x1,
>> -	DPU_INTF_TE,
>>   	DPU_DATA_HCTL_EN,
>>   	DPU_INTF_STATUS_SUPPORTED,
>>   	DPU_INTF_MAX
>> -- 
>> 2.39.2
>>

-- 
With best wishes
Dmitry

Re: [PATCH 6/7] drm/msm/dpu: drop useless check from dpu_encoder_phys_cmd_te_rd_ptr_irq()

[Marijn Suijten]

On 2023-07-27 19:21:03, Dmitry Baryshkov wrote:
> The dpu_encoder_phys_cmd_te_rd_ptr_irq() function uses neither hw_intf
> nor hw_pp data, so we can drop the corresponding check.

Maybe because it would catch "bogus" interrupts, or these blocks are
accessed somewhere down the line in the vblank callback chain?  I have
no clue :)

> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 8 --------
>  1 file changed, 8 deletions(-)
> 
> 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 60d4dd88725e..04a1106101a7 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
> @@ -108,14 +108,6 @@ static void dpu_encoder_phys_cmd_te_rd_ptr_irq(void *arg)
>  	struct dpu_encoder_phys *phys_enc = arg;
>  	struct dpu_encoder_phys_cmd *cmd_enc;
>  
> -	if (phys_enc->has_intf_te) {
> -		if (!phys_enc->hw_intf)
> -			return;
> -	} else {
> -		if (!phys_enc->hw_pp)
> -			return;
> -	}
> -
>  	DPU_ATRACE_BEGIN("rd_ptr_irq");
>  	cmd_enc = to_dpu_encoder_phys_cmd(phys_enc);
>  
> -- 
> 2.39.2
> 

Re: [PATCH 7/7] drm/msm/dpu: move INTF tearing checks to dpu_encoder_phys_cmd_init

[Marijn Suijten]

On 2023-07-27 19:21:04, Dmitry Baryshkov wrote:
> As the INTF is fixed at the encoder creation time, we can move the
> check whether INTF supports tearchck to dpu_encoder_phys_cmd_init().
> This function can return an error if INTF doesn't have required feature.
> Performing this check in dpu_encoder_phys_cmd_tearcheck_config() is less
> useful, as this function returns void.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  | 37 +++++++++++--------
>  1 file changed, 21 insertions(+), 16 deletions(-)
> 
> 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 04a1106101a7..e1dd0e1b4793 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
> @@ -325,24 +325,17 @@ static void dpu_encoder_phys_cmd_tearcheck_config(
>  	unsigned long vsync_hz;
>  	struct dpu_kms *dpu_kms;
>  
> -	if (phys_enc->has_intf_te) {
> -		if (!phys_enc->hw_intf ||
> -		    !phys_enc->hw_intf->ops.enable_tearcheck) {
> -			DPU_DEBUG_CMDENC(cmd_enc, "tearcheck not supported\n");
> -			return;
> -		}
> -
> -		DPU_DEBUG_CMDENC(cmd_enc, "");
> -	} else {
> -		if (!phys_enc->hw_pp ||
> -		    !phys_enc->hw_pp->ops.enable_tearcheck) {
> -			DPU_DEBUG_CMDENC(cmd_enc, "tearcheck not supported\n");
> -			return;
> -		}
> -
> -		DPU_DEBUG_CMDENC(cmd_enc, "pp %d\n", phys_enc->hw_pp->idx - PINGPONG_0);
> +	if (!phys_enc->has_intf_te &&
> +	    (!phys_enc->hw_pp ||
> +	     !phys_enc->hw_pp->ops.enable_tearcheck)) {

when is hw_pp assigned?  Can't we also check that somewhere in an init
phase?

Also, you won't go over 100 chars (not even 80) by having the (!... ||
!...) on a single line.

> +		DPU_DEBUG_CMDENC(cmd_enc, "tearcheck not supported\n");
> +		return;
>  	}
>  
> +	DPU_DEBUG_CMDENC(cmd_enc, "intf %d pp %d\n",
> +			 phys_enc->hw_intf->idx - INTF_0,
> +			 phys_enc->hw_pp->idx - PINGPONG_0);
> +
>  	mode = &phys_enc->cached_mode;
>  
>  	dpu_kms = phys_enc->dpu_kms;
> @@ -768,9 +761,21 @@ struct dpu_encoder_phys *dpu_encoder_phys_cmd_init(
>  	phys_enc->intf_mode = INTF_MODE_CMD;
>  	cmd_enc->stream_sel = 0;
>  
> +	if (!phys_enc->hw_intf) {
> +		DPU_ERROR_CMDENC(cmd_enc, "no INTF provided\n");
> +
> +		return ERR_PTR(-EINVAL);
> +	}
> +
>  	if (phys_enc->dpu_kms->catalog->mdss_ver->core_major_ver >= 5)
>  		phys_enc->has_intf_te = true;
>  
> +	if (phys_enc->has_intf_te && !phys_enc->hw_intf->ops.enable_tearcheck) {

Any other callbacks we could check here, and remove the checks
elsewhere?

As with enable_tearcheck() though, it does make the code less consistent
with its PP counterpart, which is checked ad-hoc everywhere (but maybe
that is fixable too).

- Marijn

> +		DPU_ERROR_CMDENC(cmd_enc, "tearcheck not supported\n");
> +
> +		return ERR_PTR(-EINVAL);
> +	}
> +
>  	atomic_set(&cmd_enc->pending_vblank_cnt, 0);
>  	init_waitqueue_head(&cmd_enc->pending_vblank_wq);
>  
> -- 
> 2.39.2
> 

Re: [PATCH 5/7] drm/msm/dpu: drop DPU_INTF_TE feature flag

[Marijn Suijten]

On 2023-07-27 23:16:22, Dmitry Baryshkov wrote:
> On 27/07/2023 23:14, Marijn Suijten wrote:
> > On 2023-07-27 19:21:02, Dmitry Baryshkov wrote:
> >> Replace the only user of the DPU_INTF_TE feature flag with the direct
> >> DPU version comparison.
> >>
> >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > 
> > Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> > 
> >> ---
> >>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 4 ++--
> >>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c       | 1 -
> >>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h       | 2 --
> >>   3 files changed, 2 insertions(+), 5 deletions(-)
> >>
> >> 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 9589fe719452..60d4dd88725e 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
> >> @@ -776,8 +776,8 @@ struct dpu_encoder_phys *dpu_encoder_phys_cmd_init(
> >>   	phys_enc->intf_mode = INTF_MODE_CMD;
> >>   	cmd_enc->stream_sel = 0;
> >>   
> >> -	phys_enc->has_intf_te = test_bit(DPU_INTF_TE,
> >> -					 &phys_enc->hw_intf->cap->features);
> >> +	if (phys_enc->dpu_kms->catalog->mdss_ver->core_major_ver >= 5)
> >> +		phys_enc->has_intf_te = true;
> > 
> > We could also check if the INTF block has the callbacks (which it based
> > on the presence of the interrupt line in the catalog instead), but then
> > I think we might loose some extra validation which you tidied up in a
> > later patch in this series?
> 
> Almost. The logic was the following: we should be using INTF for DPU >= 
> 5.0. And if we have DPU >= 5.0 and no callbacks, it's an error.

Indeed.  Let's keep that validation just in case.

- Marijn

Re: [PATCH 7/7] drm/msm/dpu: move INTF tearing checks to dpu_encoder_phys_cmd_init

[Marijn Suijten]

On 2023-07-27 22:22:20, Marijn Suijten wrote:
> On 2023-07-27 19:21:04, Dmitry Baryshkov wrote:
> > As the INTF is fixed at the encoder creation time, we can move the
> > check whether INTF supports tearchck to dpu_encoder_phys_cmd_init().
> > This function can return an error if INTF doesn't have required feature.
> > Performing this check in dpu_encoder_phys_cmd_tearcheck_config() is less
> > useful, as this function returns void.
> > 
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >  .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  | 37 +++++++++++--------
> >  1 file changed, 21 insertions(+), 16 deletions(-)
> > 
> > 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 04a1106101a7..e1dd0e1b4793 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
> > @@ -325,24 +325,17 @@ static void dpu_encoder_phys_cmd_tearcheck_config(
> >  	unsigned long vsync_hz;
> >  	struct dpu_kms *dpu_kms;
> >  
> > -	if (phys_enc->has_intf_te) {
> > -		if (!phys_enc->hw_intf ||
> > -		    !phys_enc->hw_intf->ops.enable_tearcheck) {
> > -			DPU_DEBUG_CMDENC(cmd_enc, "tearcheck not supported\n");
> > -			return;
> > -		}
> > -
> > -		DPU_DEBUG_CMDENC(cmd_enc, "");
> > -	} else {
> > -		if (!phys_enc->hw_pp ||
> > -		    !phys_enc->hw_pp->ops.enable_tearcheck) {
> > -			DPU_DEBUG_CMDENC(cmd_enc, "tearcheck not supported\n");
> > -			return;
> > -		}
> > -
> > -		DPU_DEBUG_CMDENC(cmd_enc, "pp %d\n", phys_enc->hw_pp->idx - PINGPONG_0);
> > +	if (!phys_enc->has_intf_te &&
> > +	    (!phys_enc->hw_pp ||
> > +	     !phys_enc->hw_pp->ops.enable_tearcheck)) {
> 
> when is hw_pp assigned?  Can't we also check that somewhere in an init
> phase?

It would happen right before dpu_encoder_phys_cmd_atomic_mode_set()
where we already happen to check has_intf_te to switch on PP
intr_readptr vs INTF intr_tear_rd_ptr.  Might be the perfect place for
the pingpong callback checks?

- Marijn

> 
> Also, you won't go over 100 chars (not even 80) by having the (!... ||
> !...) on a single line.
> 
> > +		DPU_DEBUG_CMDENC(cmd_enc, "tearcheck not supported\n");
> > +		return;
> >  	}
> >  
> > +	DPU_DEBUG_CMDENC(cmd_enc, "intf %d pp %d\n",
> > +			 phys_enc->hw_intf->idx - INTF_0,
> > +			 phys_enc->hw_pp->idx - PINGPONG_0);
> > +
> >  	mode = &phys_enc->cached_mode;
> >  
> >  	dpu_kms = phys_enc->dpu_kms;
> > @@ -768,9 +761,21 @@ struct dpu_encoder_phys *dpu_encoder_phys_cmd_init(
> >  	phys_enc->intf_mode = INTF_MODE_CMD;
> >  	cmd_enc->stream_sel = 0;
> >  
> > +	if (!phys_enc->hw_intf) {
> > +		DPU_ERROR_CMDENC(cmd_enc, "no INTF provided\n");
> > +
> > +		return ERR_PTR(-EINVAL);
> > +	}
> > +
> >  	if (phys_enc->dpu_kms->catalog->mdss_ver->core_major_ver >= 5)
> >  		phys_enc->has_intf_te = true;
> >  
> > +	if (phys_enc->has_intf_te && !phys_enc->hw_intf->ops.enable_tearcheck) {
> 
> Any other callbacks we could check here, and remove the checks
> elsewhere?
> 
> As with enable_tearcheck() though, it does make the code less consistent
> with its PP counterpart, which is checked ad-hoc everywhere (but maybe
> that is fixable too).
> 
> - Marijn
> 
> > +		DPU_ERROR_CMDENC(cmd_enc, "tearcheck not supported\n");
> > +
> > +		return ERR_PTR(-EINVAL);
> > +	}
> > +
> >  	atomic_set(&cmd_enc->pending_vblank_cnt, 0);
> >  	init_waitqueue_head(&cmd_enc->pending_vblank_wq);
> >  
> > -- 
> > 2.39.2
> > 

Re: [PATCH 3/7] drm/msm/dpu: inline _setup_intf_ops()

[Dmitry Baryshkov]

On 27/07/2023 23:10, Marijn Suijten wrote:
> On 2023-07-27 19:21:00, Dmitry Baryshkov wrote:
>> Inline the _setup_intf_ops() function, it makes it easier to handle
>> different conditions involving INTF configuration.
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> 
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 47 +++++++++------------
>>   1 file changed, 21 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>> index 8ec6505d9e78..7ca772791a73 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>> @@ -524,31 +524,6 @@ static void dpu_hw_intf_program_intf_cmd_cfg(struct dpu_hw_intf *ctx,
>>   	DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2);
>>   }
>>   
>> -static void _setup_intf_ops(struct dpu_hw_intf_ops *ops,
>> -		unsigned long cap, const struct dpu_mdss_version *mdss_rev)
>> -{
>> -	ops->setup_timing_gen = dpu_hw_intf_setup_timing_engine;
>> -	ops->setup_prg_fetch  = dpu_hw_intf_setup_prg_fetch;
>> -	ops->get_status = dpu_hw_intf_get_status;
>> -	ops->enable_timing = dpu_hw_intf_enable_timing_engine;
>> -	ops->get_line_count = dpu_hw_intf_get_line_count;
>> -	if (cap & BIT(DPU_INTF_INPUT_CTRL))
>> -		ops->bind_pingpong_blk = dpu_hw_intf_bind_pingpong_blk;
>> -	ops->setup_misr = dpu_hw_intf_setup_misr;
>> -	ops->collect_misr = dpu_hw_intf_collect_misr;
>> -
>> -	if (cap & BIT(DPU_INTF_TE)) {
>> -		ops->enable_tearcheck = dpu_hw_intf_enable_te;
>> -		ops->disable_tearcheck = dpu_hw_intf_disable_te;
>> -		ops->connect_external_te = dpu_hw_intf_connect_external_te;
>> -		ops->vsync_sel = dpu_hw_intf_vsync_sel;
>> -		ops->disable_autorefresh = dpu_hw_intf_disable_autorefresh;
>> -	}
>> -
>> -	if (mdss_rev->core_major_ver >= 7)
>> -		ops->program_intf_cmd_cfg = dpu_hw_intf_program_intf_cmd_cfg;
>> -}
>> -
>>   struct dpu_hw_intf *dpu_hw_intf_init(const struct dpu_intf_cfg *cfg,
>>   		void __iomem *addr, const struct dpu_mdss_version *mdss_rev)
>>   {
>> @@ -571,7 +546,27 @@ struct dpu_hw_intf *dpu_hw_intf_init(const struct dpu_intf_cfg *cfg,
>>   	 */
>>   	c->idx = cfg->id;
>>   	c->cap = cfg;
>> -	_setup_intf_ops(&c->ops, c->cap->features, mdss_rev);
>> +
>> +	c->ops.setup_timing_gen = dpu_hw_intf_setup_timing_engine;
>> +	c->ops.setup_prg_fetch  = dpu_hw_intf_setup_prg_fetch;
>> +	c->ops.get_status = dpu_hw_intf_get_status;
>> +	c->ops.enable_timing = dpu_hw_intf_enable_timing_engine;
>> +	c->ops.get_line_count = dpu_hw_intf_get_line_count;
>> +	if (cfg->features & BIT(DPU_INTF_INPUT_CTRL))
>> +		c->ops.bind_pingpong_blk = dpu_hw_intf_bind_pingpong_blk;
> 
> While at it, could we sort these down with the other conditional
> callbacks?

What kind of sorting do you have in mind?

>> +	c->ops.setup_misr = dpu_hw_intf_setup_misr;
>> +	c->ops.collect_misr = dpu_hw_intf_collect_misr;
>> +
>> +	if (cfg->features & BIT(DPU_INTF_TE)) {
> 
> Any clue why we're not using test_bit()?  Feels a bit inconsistent.

Yes, some files use test_bit(), others just check the bit directly. 
Maybe after moving some/most of conditionals to core_major_ver we can 
clean that too.

> 
>> +		c->ops.enable_tearcheck = dpu_hw_intf_enable_te;
>> +		c->ops.disable_tearcheck = dpu_hw_intf_disable_te;
>> +		c->ops.connect_external_te = dpu_hw_intf_connect_external_te;
>> +		c->ops.vsync_sel = dpu_hw_intf_vsync_sel;
>> +		c->ops.disable_autorefresh = dpu_hw_intf_disable_autorefresh;
>> +	}
>> +
>> +	if (mdss_rev->core_major_ver >= 7)
>> +		c->ops.program_intf_cmd_cfg = dpu_hw_intf_program_intf_cmd_cfg;
>>   
>>   	return c;
>>   }
>> -- 
>> 2.39.2
>>

-- 
With best wishes
Dmitry

Re: [PATCH 1/7] drm/msm/dpu: enable PINGPONG TE operations only when supported by HW

[Dmitry Baryshkov]

On 27/07/2023 23:05, Marijn Suijten wrote:
> On 2023-07-27 19:20:58, Dmitry Baryshkov wrote:
>> The DPU_PINGPONG_TE bit is set for all PINGPONG blocks on DPU < 5.0.
>> Rather than checking for the flag, check for the presense of the
>> corresponding interrupt line.
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
>> index 9298c166b213..912a3bdf8ad4 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
>> @@ -296,7 +296,7 @@ struct dpu_hw_pingpong *dpu_hw_pingpong_init(const struct dpu_pingpong_cfg *cfg,
>>   	c->idx = cfg->id;
>>   	c->caps = cfg;
> 
> In hindsight, maybe there's one patch missing from this series.  You
> inlined _setup_intf_ops() later, but there's no patch inlining
> _setup_pingpong_ops() which looks to be required for applying this
> patch.

Yes, I missed it somehow.

> 
> - Marijn
> 
>>   
>> -	if (test_bit(DPU_PINGPONG_TE, &cfg->features)) {
>> +	if (cfg->intr_rdptr) {
>>   		c->ops.enable_tearcheck = dpu_hw_pp_enable_te;
>>   		c->ops.disable_tearcheck = dpu_hw_pp_disable_te;
>>   		c->ops.connect_external_te = dpu_hw_pp_connect_external_te;
>> -- 
>> 2.39.2
>>

-- 
With best wishes
Dmitry

Re: [PATCH 1/7] drm/msm/dpu: enable PINGPONG TE operations only when supported by HW

[Dmitry Baryshkov]

On 27/07/2023 23:03, Marijn Suijten wrote:
> On 2023-07-27 19:20:58, Dmitry Baryshkov wrote:
>> The DPU_PINGPONG_TE bit is set for all PINGPONG blocks on DPU < 5.0.
>> Rather than checking for the flag, check for the presense of the
>> corresponding interrupt line.
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
> That's a smart use of the interrupt field.  I both like it, and I do
> not.  While we didn't do any validation for consistency previously, this
> means we now have multiple ways of controlling available "features":
> 
> - Feature flags on hardware blocks;
> - Presence of certain IRQs;
> - DPU core revision.

I hesitated here too. For INTF it is clear that there is no other good 
way to check for the TE feature. For PP we can just check for the DPU 
revision.

> 
> Maybe that is more confusing to follow?  Regardless of that I'm
> convinced that this patch does what it's supposed to and gets rid of
> some ambiguity.  Maybe a comment above the IF explaining the "PP TE"
> feature could alleviate the above concerns thoo.  Hence:
> 
> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> 
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
>> index 9298c166b213..912a3bdf8ad4 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
>> @@ -296,7 +296,7 @@ struct dpu_hw_pingpong *dpu_hw_pingpong_init(const struct dpu_pingpong_cfg *cfg,
>>   	c->idx = cfg->id;
>>   	c->caps = cfg;
>>   
>> -	if (test_bit(DPU_PINGPONG_TE, &cfg->features)) {
>> +	if (cfg->intr_rdptr) {
>>   		c->ops.enable_tearcheck = dpu_hw_pp_enable_te;
>>   		c->ops.disable_tearcheck = dpu_hw_pp_disable_te;
>>   		c->ops.connect_external_te = dpu_hw_pp_connect_external_te;
>> -- 
>> 2.39.2
>>

-- 
With best wishes
Dmitry

Re: [PATCH 3/7] drm/msm/dpu: inline _setup_intf_ops()

[Marijn Suijten]

On 2023-07-29 02:45:43, Dmitry Baryshkov wrote:
> On 27/07/2023 23:10, Marijn Suijten wrote:
> > On 2023-07-27 19:21:00, Dmitry Baryshkov wrote:
> >> Inline the _setup_intf_ops() function, it makes it easier to handle
> >> different conditions involving INTF configuration.
> >>
> >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > 
> > Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> > 
> >> ---
> >>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 47 +++++++++------------
> >>   1 file changed, 21 insertions(+), 26 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> >> index 8ec6505d9e78..7ca772791a73 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> >> @@ -524,31 +524,6 @@ static void dpu_hw_intf_program_intf_cmd_cfg(struct dpu_hw_intf *ctx,
> >>   	DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2);
> >>   }
> >>   
> >> -static void _setup_intf_ops(struct dpu_hw_intf_ops *ops,
> >> -		unsigned long cap, const struct dpu_mdss_version *mdss_rev)
> >> -{
> >> -	ops->setup_timing_gen = dpu_hw_intf_setup_timing_engine;
> >> -	ops->setup_prg_fetch  = dpu_hw_intf_setup_prg_fetch;
> >> -	ops->get_status = dpu_hw_intf_get_status;
> >> -	ops->enable_timing = dpu_hw_intf_enable_timing_engine;
> >> -	ops->get_line_count = dpu_hw_intf_get_line_count;
> >> -	if (cap & BIT(DPU_INTF_INPUT_CTRL))
> >> -		ops->bind_pingpong_blk = dpu_hw_intf_bind_pingpong_blk;
> >> -	ops->setup_misr = dpu_hw_intf_setup_misr;
> >> -	ops->collect_misr = dpu_hw_intf_collect_misr;
> >> -
> >> -	if (cap & BIT(DPU_INTF_TE)) {
> >> -		ops->enable_tearcheck = dpu_hw_intf_enable_te;
> >> -		ops->disable_tearcheck = dpu_hw_intf_disable_te;
> >> -		ops->connect_external_te = dpu_hw_intf_connect_external_te;
> >> -		ops->vsync_sel = dpu_hw_intf_vsync_sel;
> >> -		ops->disable_autorefresh = dpu_hw_intf_disable_autorefresh;
> >> -	}
> >> -
> >> -	if (mdss_rev->core_major_ver >= 7)
> >> -		ops->program_intf_cmd_cfg = dpu_hw_intf_program_intf_cmd_cfg;
> >> -}
> >> -
> >>   struct dpu_hw_intf *dpu_hw_intf_init(const struct dpu_intf_cfg *cfg,
> >>   		void __iomem *addr, const struct dpu_mdss_version *mdss_rev)
> >>   {
> >> @@ -571,7 +546,27 @@ struct dpu_hw_intf *dpu_hw_intf_init(const struct dpu_intf_cfg *cfg,
> >>   	 */
> >>   	c->idx = cfg->id;
> >>   	c->cap = cfg;
> >> -	_setup_intf_ops(&c->ops, c->cap->features, mdss_rev);
> >> +
> >> +	c->ops.setup_timing_gen = dpu_hw_intf_setup_timing_engine;
> >> +	c->ops.setup_prg_fetch  = dpu_hw_intf_setup_prg_fetch;
> >> +	c->ops.get_status = dpu_hw_intf_get_status;
> >> +	c->ops.enable_timing = dpu_hw_intf_enable_timing_engine;
> >> +	c->ops.get_line_count = dpu_hw_intf_get_line_count;
> >> +	if (cfg->features & BIT(DPU_INTF_INPUT_CTRL))
> >> +		c->ops.bind_pingpong_blk = dpu_hw_intf_bind_pingpong_blk;
> > 
> > While at it, could we sort these down with the other conditional
> > callbacks?
> 
> What kind of sorting do you have in mind?

Moving this conditional ( if (...) ) down with the other conditional
assignment below, instead of being right in the middle of get_line_count
and setup_misr, both which are not conditional and make it harder to
read, especially considering the lack of newlines and/or curly braces.

> >> +	c->ops.setup_misr = dpu_hw_intf_setup_misr;
> >> +	c->ops.collect_misr = dpu_hw_intf_collect_misr;
> >> +
> >> +	if (cfg->features & BIT(DPU_INTF_TE)) {
> > 
> > Any clue why we're not using test_bit()?  Feels a bit inconsistent.
> 
> Yes, some files use test_bit(), others just check the bit directly. 
> Maybe after moving some/most of conditionals to core_major_ver we can 
> clean that too.

Sounds good.

- Marijn

> >> +		c->ops.enable_tearcheck = dpu_hw_intf_enable_te;
> >> +		c->ops.disable_tearcheck = dpu_hw_intf_disable_te;
> >> +		c->ops.connect_external_te = dpu_hw_intf_connect_external_te;
> >> +		c->ops.vsync_sel = dpu_hw_intf_vsync_sel;
> >> +		c->ops.disable_autorefresh = dpu_hw_intf_disable_autorefresh;
> >> +	}
> >> +
> >> +	if (mdss_rev->core_major_ver >= 7)
> >> +		c->ops.program_intf_cmd_cfg = dpu_hw_intf_program_intf_cmd_cfg;
> >>   
> >>   	return c;
> >>   }
> >> -- 
> >> 2.39.2
> >>
> 
> -- 
> With best wishes
> Dmitry
> 

Re: [PATCH 1/7] drm/msm/dpu: enable PINGPONG TE operations only when supported by HW

[Marijn Suijten]

On 2023-07-29 02:59:32, Dmitry Baryshkov wrote:
> On 27/07/2023 23:03, Marijn Suijten wrote:
> > On 2023-07-27 19:20:58, Dmitry Baryshkov wrote:
> >> The DPU_PINGPONG_TE bit is set for all PINGPONG blocks on DPU < 5.0.
> >> Rather than checking for the flag, check for the presense of the
> >> corresponding interrupt line.
> >>
> >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > 
> > That's a smart use of the interrupt field.  I both like it, and I do
> > not.  While we didn't do any validation for consistency previously, this
> > means we now have multiple ways of controlling available "features":
> > 
> > - Feature flags on hardware blocks;
> > - Presence of certain IRQs;
> > - DPU core revision.
> 
> I hesitated here too. For INTF it is clear that there is no other good 
> way to check for the TE feature. For PP we can just check for the DPU 
> revision.

For both we could additionally check the DPU revision, and for INTF we
could check for TYPE_DSI.  Both would aid in extra validation, if we
require the IRQ to be present or absent under these conditions.

It might also help to document this, either here and on the respective
struct fields so that catalog implementers know when they should supply
or leave out an IRQ?

- Marijn

> > Maybe that is more confusing to follow?  Regardless of that I'm
> > convinced that this patch does what it's supposed to and gets rid of
> > some ambiguity.  Maybe a comment above the IF explaining the "PP TE"
> > feature could alleviate the above concerns thoo.  Hence:
> > 
> > Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> > 
> >> ---
> >>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
> >> index 9298c166b213..912a3bdf8ad4 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
> >> @@ -296,7 +296,7 @@ struct dpu_hw_pingpong *dpu_hw_pingpong_init(const struct dpu_pingpong_cfg *cfg,
> >>   	c->idx = cfg->id;
> >>   	c->caps = cfg;
> >>   
> >> -	if (test_bit(DPU_PINGPONG_TE, &cfg->features)) {
> >> +	if (cfg->intr_rdptr) {
> >>   		c->ops.enable_tearcheck = dpu_hw_pp_enable_te;
> >>   		c->ops.disable_tearcheck = dpu_hw_pp_disable_te;
> >>   		c->ops.connect_external_te = dpu_hw_pp_connect_external_te;
> >> -- 
> >> 2.39.2
> >>
> 
> -- 
> With best wishes
> Dmitry
> 

Re: [PATCH 3/7] drm/msm/dpu: inline _setup_intf_ops()

[Dmitry Baryshkov]

On Sat, 29 Jul 2023 at 21:28, Marijn Suijten
<marijn.suijten@somainline.org> wrote:
>
> On 2023-07-29 02:45:43, Dmitry Baryshkov wrote:
> > On 27/07/2023 23:10, Marijn Suijten wrote:
> > > On 2023-07-27 19:21:00, Dmitry Baryshkov wrote:
> > >> Inline the _setup_intf_ops() function, it makes it easier to handle
> > >> different conditions involving INTF configuration.
> > >>
> > >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > >
> > > Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> > >
> > >> ---
> > >>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 47 +++++++++------------
> > >>   1 file changed, 21 insertions(+), 26 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> > >> index 8ec6505d9e78..7ca772791a73 100644
> > >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> > >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> > >> @@ -524,31 +524,6 @@ static void dpu_hw_intf_program_intf_cmd_cfg(struct dpu_hw_intf *ctx,
> > >>    DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2);
> > >>   }
> > >>
> > >> -static void _setup_intf_ops(struct dpu_hw_intf_ops *ops,
> > >> -          unsigned long cap, const struct dpu_mdss_version *mdss_rev)
> > >> -{
> > >> -  ops->setup_timing_gen = dpu_hw_intf_setup_timing_engine;
> > >> -  ops->setup_prg_fetch  = dpu_hw_intf_setup_prg_fetch;
> > >> -  ops->get_status = dpu_hw_intf_get_status;
> > >> -  ops->enable_timing = dpu_hw_intf_enable_timing_engine;
> > >> -  ops->get_line_count = dpu_hw_intf_get_line_count;
> > >> -  if (cap & BIT(DPU_INTF_INPUT_CTRL))
> > >> -          ops->bind_pingpong_blk = dpu_hw_intf_bind_pingpong_blk;
> > >> -  ops->setup_misr = dpu_hw_intf_setup_misr;
> > >> -  ops->collect_misr = dpu_hw_intf_collect_misr;
> > >> -
> > >> -  if (cap & BIT(DPU_INTF_TE)) {
> > >> -          ops->enable_tearcheck = dpu_hw_intf_enable_te;
> > >> -          ops->disable_tearcheck = dpu_hw_intf_disable_te;
> > >> -          ops->connect_external_te = dpu_hw_intf_connect_external_te;
> > >> -          ops->vsync_sel = dpu_hw_intf_vsync_sel;
> > >> -          ops->disable_autorefresh = dpu_hw_intf_disable_autorefresh;
> > >> -  }
> > >> -
> > >> -  if (mdss_rev->core_major_ver >= 7)
> > >> -          ops->program_intf_cmd_cfg = dpu_hw_intf_program_intf_cmd_cfg;
> > >> -}
> > >> -
> > >>   struct dpu_hw_intf *dpu_hw_intf_init(const struct dpu_intf_cfg *cfg,
> > >>            void __iomem *addr, const struct dpu_mdss_version *mdss_rev)
> > >>   {
> > >> @@ -571,7 +546,27 @@ struct dpu_hw_intf *dpu_hw_intf_init(const struct dpu_intf_cfg *cfg,
> > >>     */
> > >>    c->idx = cfg->id;
> > >>    c->cap = cfg;
> > >> -  _setup_intf_ops(&c->ops, c->cap->features, mdss_rev);
> > >> +
> > >> +  c->ops.setup_timing_gen = dpu_hw_intf_setup_timing_engine;
> > >> +  c->ops.setup_prg_fetch  = dpu_hw_intf_setup_prg_fetch;
> > >> +  c->ops.get_status = dpu_hw_intf_get_status;
> > >> +  c->ops.enable_timing = dpu_hw_intf_enable_timing_engine;
> > >> +  c->ops.get_line_count = dpu_hw_intf_get_line_count;
> > >> +  if (cfg->features & BIT(DPU_INTF_INPUT_CTRL))
> > >> +          c->ops.bind_pingpong_blk = dpu_hw_intf_bind_pingpong_blk;
> > >
> > > While at it, could we sort these down with the other conditional
> > > callbacks?
> >
> > What kind of sorting do you have in mind?
>
> Moving this conditional ( if (...) ) down with the other conditional
> assignment below, instead of being right in the middle of get_line_count
> and setup_misr, both which are not conditional and make it harder to
> read, especially considering the lack of newlines and/or curly braces.

Ack, sounds good.

>
> > >> +  c->ops.setup_misr = dpu_hw_intf_setup_misr;
> > >> +  c->ops.collect_misr = dpu_hw_intf_collect_misr;
> > >> +
> > >> +  if (cfg->features & BIT(DPU_INTF_TE)) {
> > >
> > > Any clue why we're not using test_bit()?  Feels a bit inconsistent.
> >
> > Yes, some files use test_bit(), others just check the bit directly.
> > Maybe after moving some/most of conditionals to core_major_ver we can
> > clean that too.
>
> Sounds good.
>
> - Marijn
>
> > >> +          c->ops.enable_tearcheck = dpu_hw_intf_enable_te;
> > >> +          c->ops.disable_tearcheck = dpu_hw_intf_disable_te;
> > >> +          c->ops.connect_external_te = dpu_hw_intf_connect_external_te;
> > >> +          c->ops.vsync_sel = dpu_hw_intf_vsync_sel;
> > >> +          c->ops.disable_autorefresh = dpu_hw_intf_disable_autorefresh;
> > >> +  }
> > >> +
> > >> +  if (mdss_rev->core_major_ver >= 7)
> > >> +          c->ops.program_intf_cmd_cfg = dpu_hw_intf_program_intf_cmd_cfg;
> > >>
> > >>    return c;
> > >>   }
> > >> --
> > >> 2.39.2
> > >>
> >
> > --
> > With best wishes
> > Dmitry
> >



-- 
With best wishes
Dmitry

Re: [PATCH 1/7] drm/msm/dpu: enable PINGPONG TE operations only when supported by HW

[Dmitry Baryshkov]

On 29/07/2023 21:31, Marijn Suijten wrote:
> On 2023-07-29 02:59:32, Dmitry Baryshkov wrote:
>> On 27/07/2023 23:03, Marijn Suijten wrote:
>>> On 2023-07-27 19:20:58, Dmitry Baryshkov wrote:
>>>> The DPU_PINGPONG_TE bit is set for all PINGPONG blocks on DPU < 5.0.
>>>> Rather than checking for the flag, check for the presense of the
>>>> corresponding interrupt line.
>>>>
>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>
>>> That's a smart use of the interrupt field.  I both like it, and I do
>>> not.  While we didn't do any validation for consistency previously, this
>>> means we now have multiple ways of controlling available "features":
>>>
>>> - Feature flags on hardware blocks;
>>> - Presence of certain IRQs;
>>> - DPU core revision.
>>
>> I hesitated here too. For INTF it is clear that there is no other good
>> way to check for the TE feature. For PP we can just check for the DPU
>> revision.
> 
> For both we could additionally check the DPU revision, and for INTF we
> could check for TYPE_DSI.  Both would aid in extra validation, if we
> require the IRQ to be present or absent under these conditions.

Yep, maybe that's better.

> 
> It might also help to document this, either here and on the respective
> struct fields so that catalog implementers know when they should supply
> or leave out an IRQ?

Probably a WARN_ON would be enough.

> 
> - Marijn
> 
>>> Maybe that is more confusing to follow?  Regardless of that I'm
>>> convinced that this patch does what it's supposed to and gets rid of
>>> some ambiguity.  Maybe a comment above the IF explaining the "PP TE"
>>> feature could alleviate the above concerns thoo.  Hence:
>>>
>>> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
>>>
>>>> ---
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
>>>> index 9298c166b213..912a3bdf8ad4 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
>>>> @@ -296,7 +296,7 @@ struct dpu_hw_pingpong *dpu_hw_pingpong_init(const struct dpu_pingpong_cfg *cfg,
>>>>    	c->idx = cfg->id;
>>>>    	c->caps = cfg;
>>>>    
>>>> -	if (test_bit(DPU_PINGPONG_TE, &cfg->features)) {
>>>> +	if (cfg->intr_rdptr) {
>>>>    		c->ops.enable_tearcheck = dpu_hw_pp_enable_te;
>>>>    		c->ops.disable_tearcheck = dpu_hw_pp_disable_te;
>>>>    		c->ops.connect_external_te = dpu_hw_pp_connect_external_te;
>>>> -- 
>>>> 2.39.2
>>>>
>>
>> -- 
>> With best wishes
>> Dmitry
>>

-- 
With best wishes
Dmitry

Re: [PATCH 7/7] drm/msm/dpu: move INTF tearing checks to dpu_encoder_phys_cmd_init

[Dmitry Baryshkov]

On 27/07/2023 23:25, Marijn Suijten wrote:
> On 2023-07-27 22:22:20, Marijn Suijten wrote:
>> On 2023-07-27 19:21:04, Dmitry Baryshkov wrote:
>>> As the INTF is fixed at the encoder creation time, we can move the
>>> check whether INTF supports tearchck to dpu_encoder_phys_cmd_init().
>>> This function can return an error if INTF doesn't have required feature.
>>> Performing this check in dpu_encoder_phys_cmd_tearcheck_config() is less
>>> useful, as this function returns void.
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>   .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  | 37 +++++++++++--------
>>>   1 file changed, 21 insertions(+), 16 deletions(-)
>>>
>>> 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 04a1106101a7..e1dd0e1b4793 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
>>> @@ -325,24 +325,17 @@ static void dpu_encoder_phys_cmd_tearcheck_config(
>>>   	unsigned long vsync_hz;
>>>   	struct dpu_kms *dpu_kms;
>>>   
>>> -	if (phys_enc->has_intf_te) {
>>> -		if (!phys_enc->hw_intf ||
>>> -		    !phys_enc->hw_intf->ops.enable_tearcheck) {
>>> -			DPU_DEBUG_CMDENC(cmd_enc, "tearcheck not supported\n");
>>> -			return;
>>> -		}
>>> -
>>> -		DPU_DEBUG_CMDENC(cmd_enc, "");
>>> -	} else {
>>> -		if (!phys_enc->hw_pp ||
>>> -		    !phys_enc->hw_pp->ops.enable_tearcheck) {
>>> -			DPU_DEBUG_CMDENC(cmd_enc, "tearcheck not supported\n");
>>> -			return;
>>> -		}
>>> -
>>> -		DPU_DEBUG_CMDENC(cmd_enc, "pp %d\n", phys_enc->hw_pp->idx - PINGPONG_0);
>>> +	if (!phys_enc->has_intf_te &&
>>> +	    (!phys_enc->hw_pp ||
>>> +	     !phys_enc->hw_pp->ops.enable_tearcheck)) {
>>
>> when is hw_pp assigned?  Can't we also check that somewhere in an init
>> phase?
> 
> It would happen right before dpu_encoder_phys_cmd_atomic_mode_set()
> where we already happen to check has_intf_te to switch on PP
> intr_readptr vs INTF intr_tear_rd_ptr.  Might be the perfect place for
> the pingpong callback checks?

The problem is that mode_set doesn't return an error (by design). I'd 
put a TODO here, so that if we ever move/change resource allocation, 
this check can be done next to it (atomic_check isn't a good place, 
since phys_enc.atomic_check happens before resource reallocation).

> 
> - Marijn
> 
>>
>> Also, you won't go over 100 chars (not even 80) by having the (!... ||
>> !...) on a single line.
>>
>>> +		DPU_DEBUG_CMDENC(cmd_enc, "tearcheck not supported\n");
>>> +		return;
>>>   	}
>>>   
>>> +	DPU_DEBUG_CMDENC(cmd_enc, "intf %d pp %d\n",
>>> +			 phys_enc->hw_intf->idx - INTF_0,
>>> +			 phys_enc->hw_pp->idx - PINGPONG_0);
>>> +
>>>   	mode = &phys_enc->cached_mode;
>>>   
>>>   	dpu_kms = phys_enc->dpu_kms;
>>> @@ -768,9 +761,21 @@ struct dpu_encoder_phys *dpu_encoder_phys_cmd_init(
>>>   	phys_enc->intf_mode = INTF_MODE_CMD;
>>>   	cmd_enc->stream_sel = 0;
>>>   
>>> +	if (!phys_enc->hw_intf) {
>>> +		DPU_ERROR_CMDENC(cmd_enc, "no INTF provided\n");
>>> +
>>> +		return ERR_PTR(-EINVAL);
>>> +	}
>>> +
>>>   	if (phys_enc->dpu_kms->catalog->mdss_ver->core_major_ver >= 5)
>>>   		phys_enc->has_intf_te = true;
>>>   
>>> +	if (phys_enc->has_intf_te && !phys_enc->hw_intf->ops.enable_tearcheck) {
>>
>> Any other callbacks we could check here, and remove the checks
>> elsewhere?
>>
>> As with enable_tearcheck() though, it does make the code less consistent
>> with its PP counterpart, which is checked ad-hoc everywhere (but maybe
>> that is fixable too).
>>
>> - Marijn
>>
>>> +		DPU_ERROR_CMDENC(cmd_enc, "tearcheck not supported\n");
>>> +
>>> +		return ERR_PTR(-EINVAL);
>>> +	}
>>> +
>>>   	atomic_set(&cmd_enc->pending_vblank_cnt, 0);
>>>   	init_waitqueue_head(&cmd_enc->pending_vblank_wq);
>>>   
>>> -- 
>>> 2.39.2
>>>

-- 
With best wishes
Dmitry

Re: [PATCH 4/7] drm/msm/dpu: enable INTF TE operations only when supported by HW

[Dmitry Baryshkov]

On 27/07/2023 23:12, Marijn Suijten wrote:
> On 2023-07-27 19:21:01, Dmitry Baryshkov wrote:
>> The DPU_INTF_TE bit is set for all INTF blocks on DPU >= 5.0, however
>> only INTF_1 and INTF_2 actually support tearing control. Rather than
>> trying to fix the DPU_INTF_TE, check for the presense of the
> 
> I would more exactly expand "fix" to "Rather than specifying that
> feature bit on DSI INTF_1 and INTF_2 exclusively..."
> 
>> corresponding interrupt line.
> 
> ... which the catalog will only provide on DPU >= 5.0.

I'm going to rephrase this in a slightly different way to follow the irq 
presence -> major & type change.

> 
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> 
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>> index 7ca772791a73..8abdf9553f3b 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>> @@ -557,7 +557,7 @@ struct dpu_hw_intf *dpu_hw_intf_init(const struct dpu_intf_cfg *cfg,
>>   	c->ops.setup_misr = dpu_hw_intf_setup_misr;
>>   	c->ops.collect_misr = dpu_hw_intf_collect_misr;
>>   
>> -	if (cfg->features & BIT(DPU_INTF_TE)) {
>> +	if (cfg->intr_tear_rd_ptr) {
>>   		c->ops.enable_tearcheck = dpu_hw_intf_enable_te;
>>   		c->ops.disable_tearcheck = dpu_hw_intf_disable_te;
>>   		c->ops.connect_external_te = dpu_hw_intf_connect_external_te;
>> -- 
>> 2.39.2
>>

-- 
With best wishes
Dmitry

Re: [PATCH 1/7] drm/msm/dpu: enable PINGPONG TE operations only when supported by HW

[Marijn Suijten]

On 2023-07-30 02:18:10, Dmitry Baryshkov wrote:
> On 29/07/2023 21:31, Marijn Suijten wrote:
> > On 2023-07-29 02:59:32, Dmitry Baryshkov wrote:
> >> On 27/07/2023 23:03, Marijn Suijten wrote:
> >>> On 2023-07-27 19:20:58, Dmitry Baryshkov wrote:
> >>>> The DPU_PINGPONG_TE bit is set for all PINGPONG blocks on DPU < 5.0.
> >>>> Rather than checking for the flag, check for the presense of the
> >>>> corresponding interrupt line.
> >>>>
> >>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >>>
> >>> That's a smart use of the interrupt field.  I both like it, and I do
> >>> not.  While we didn't do any validation for consistency previously, this
> >>> means we now have multiple ways of controlling available "features":
> >>>
> >>> - Feature flags on hardware blocks;
> >>> - Presence of certain IRQs;
> >>> - DPU core revision.
> >>
> >> I hesitated here too. For INTF it is clear that there is no other good
> >> way to check for the TE feature. For PP we can just check for the DPU
> >> revision.
> > 
> > For both we could additionally check the DPU revision, and for INTF we
> > could check for TYPE_DSI.  Both would aid in extra validation, if we
> > require the IRQ to be present or absent under these conditions.
> 
> Yep, maybe that's better.
> 
> > 
> > It might also help to document this, either here and on the respective
> > struct fields so that catalog implementers know when they should supply
> > or leave out an IRQ?
> 
> Probably a WARN_ON would be enough.

SGTM, it is after all only for bring-up as after that (additions have
been validated, reviewed and merged) we "trust the kernel" including our
catalog, so errors like this should pretty much be unreachable.

- Marijn

Re: [PATCH 7/7] drm/msm/dpu: move INTF tearing checks to dpu_encoder_phys_cmd_init

[Marijn Suijten]

On 2023-07-30 03:16:59, Dmitry Baryshkov wrote:
<snip>
> >>> +	if (!phys_enc->has_intf_te &&
> >>> +	    (!phys_enc->hw_pp ||
> >>> +	     !phys_enc->hw_pp->ops.enable_tearcheck)) {
> >>
> >> when is hw_pp assigned?  Can't we also check that somewhere in an init
> >> phase?
> > 
> > It would happen right before dpu_encoder_phys_cmd_atomic_mode_set()
> > where we already happen to check has_intf_te to switch on PP
> > intr_readptr vs INTF intr_tear_rd_ptr.  Might be the perfect place for
> > the pingpong callback checks?
> 
> The problem is that mode_set doesn't return an error (by design). I'd 
> put a TODO here, so that if we ever move/change resource allocation, 
> this check can be done next to it (atomic_check isn't a good place, 
> since phys_enc.atomic_check happens before resource reallocation).

As thought of in another patch, perhaps it could just be a WARN_ON() as
these almost always relate directly to constant information provided by
the catalog, which we trust to be sound (and these error cases to hardly
be reachable) after it has been validated, reviewed and merged.

- Marijn

Re: [PATCH 4/7] drm/msm/dpu: enable INTF TE operations only when supported by HW

[Marijn Suijten]

On 2023-07-30 03:22:46, Dmitry Baryshkov wrote:
> On 27/07/2023 23:12, Marijn Suijten wrote:
> > On 2023-07-27 19:21:01, Dmitry Baryshkov wrote:
> >> The DPU_INTF_TE bit is set for all INTF blocks on DPU >= 5.0, however
> >> only INTF_1 and INTF_2 actually support tearing control. Rather than
> >> trying to fix the DPU_INTF_TE, check for the presense of the
> > 
> > I would more exactly expand "fix" to "Rather than specifying that
> > feature bit on DSI INTF_1 and INTF_2 exclusively..."
> > 
> >> corresponding interrupt line.
> > 
> > ... which the catalog will only provide on DPU >= 5.0.
> 
> I'm going to rephrase this in a slightly different way to follow the irq 
> presence -> major & type change.

Sure, looks good, including the validation (that doesn't really need to
be mentioned in the commit message anymore).

- Marijn