[PATCH v2 0/8] drm/msm/dpu: handle non-default TE source pins

[PATCH v2 0/8] drm/msm/dpu: handle non-default TE source pins

[Dmitry Baryshkov]

Command-mode DSI panels need to signal the display controlller when
vsync happens, so that the device can start sending the next frame. Some
devices (Google Pixel 3) use a non-default pin, so additional
configuration is required. Add a way to specify this information in DT
and handle it in the DSI and DPU drivers.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
Changes in v2:
- In DT bindings renamed mdp_gpioN to mdp_vsync_p/_s/_e per pins name (Abhinav)
- Extended bindings to include default: mdp_vsync_p (Rob)
- Renamed dpu_hw_setup_vsync_source() and
  dpu_hw_setup_vsync_source_and_vsync_sel() to match the implementation
  (Abhinav)
- Link to v1: https://lore.kernel.org/r/20240520-dpu-handle-te-signal-v1-0-f273b42a089c@linaro.org

---
Dmitry Baryshkov (8):
      dt-bindings: display/msm/dsi: allow specifying TE source
      drm/msm/dpu: convert vsync source defines to the enum
      drm/msm/dsi: drop unused GPIOs handling
      drm/msm/dpu: pull the is_cmd_mode out of _dpu_encoder_update_vsync_source()
      drm/msm/dpu: rework vsync_source handling
      drm/msm/dsi: parse vsync source from device tree
      drm/msm/dpu: support setting the TE source
      drm/msm/dpu: rename dpu_hw_setup_vsync_source functions

 .../bindings/display/msm/dsi-controller-main.yaml  | 17 ++++++++
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c        | 11 ++---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h        |  5 +--
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c        |  2 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h        |  2 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h        | 26 ++++++------
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c         | 14 +++----
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h         |  2 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c            | 44 ++++++++++++++++++++
 drivers/gpu/drm/msm/dsi/dsi.h                      |  1 +
 drivers/gpu/drm/msm/dsi/dsi_host.c                 | 48 +++++-----------------
 drivers/gpu/drm/msm/dsi/dsi_manager.c              |  5 +++
 drivers/gpu/drm/msm/msm_drv.h                      |  6 +++
 13 files changed, 114 insertions(+), 69 deletions(-)
---
base-commit: 03d44168cbd7fc57d5de56a3730427db758fc7f6
change-id: 20240514-dpu-handle-te-signal-82663c0211bd

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

[PATCH v2 1/8] dt-bindings: display/msm/dsi: allow specifying TE source

[Dmitry Baryshkov]

Command mode panels provide TE signal back to the DSI host to signal
that the frame display has completed and update of the image will not
cause tearing. Usually it is connected to the first GPIO with the
mdp_vsync function, which is the default. In such case the property can
be skipped.

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 .../bindings/display/msm/dsi-controller-main.yaml       | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml b/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
index 1fa28e976559..e1cb3a1fee81 100644
--- a/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
+++ b/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
@@ -162,6 +162,22 @@ properties:
                 items:
                   enum: [ 0, 1, 2, 3 ]
 
+              qcom,te-source:
+                $ref: /schemas/types.yaml#/definitions/string
+                description:
+                  Specifies the source of vsync signal from the panel used for
+                  tearing elimination.
+                default: mdp_vsync_p
+                enum:
+                  - mdp_vsync_p
+                  - mdp_vsync_s
+                  - mdp_vsync_e
+                  - timer0
+                  - timer1
+                  - timer2
+                  - timer3
+                  - timer4
+
     required:
       - port@0
       - port@1
@@ -452,6 +468,7 @@ examples:
                           dsi0_out: endpoint {
                                    remote-endpoint = <&sn65dsi86_in>;
                                    data-lanes = <0 1 2 3>;
+                                   qcom,te-source = "mdp_vsync_e";
                           };
                   };
            };

-- 
2.39.2

[PATCH v2 2/8] drm/msm/dpu: convert vsync source defines to the enum

[Dmitry Baryshkov]

Add enum dpu_vsync_source instead of a series of defines. Use this enum
to pass vsync information.

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c |  2 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c |  2 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h |  2 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h | 26 ++++++++++++++------------
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h  |  2 +-
 5 files changed, 18 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 119f3ea50a7c..4988a1029431 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -747,7 +747,7 @@ static void _dpu_encoder_update_vsync_source(struct dpu_encoder_virt *dpu_enc,
 		if (disp_info->is_te_using_watchdog_timer)
 			vsync_cfg.vsync_source = DPU_VSYNC_SOURCE_WD_TIMER_0;
 		else
-			vsync_cfg.vsync_source = DPU_VSYNC0_SOURCE_GPIO;
+			vsync_cfg.vsync_source = DPU_VSYNC_SOURCE_GPIO_0;
 
 		hw_mdptop->ops.setup_vsync_source(hw_mdptop, &vsync_cfg);
 
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 225c1c7768ff..96f6160cf607 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
@@ -462,7 +462,7 @@ static int dpu_hw_intf_get_vsync_info(struct dpu_hw_intf *intf,
 }
 
 static void dpu_hw_intf_vsync_sel(struct dpu_hw_intf *intf,
-		u32 vsync_source)
+				  enum dpu_vsync_source vsync_source)
 {
 	struct dpu_hw_blk_reg_map *c;
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
index f9015c67a574..ac244f0b33fb 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
@@ -107,7 +107,7 @@ struct dpu_hw_intf_ops {
 
 	int (*connect_external_te)(struct dpu_hw_intf *intf, bool enable_external_te);
 
-	void (*vsync_sel)(struct dpu_hw_intf *intf, u32 vsync_source);
+	void (*vsync_sel)(struct dpu_hw_intf *intf, enum dpu_vsync_source vsync_source);
 
 	/**
 	 * Disable autorefresh if enabled
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
index 66759623fc42..a2eff36a2224 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
@@ -54,18 +54,20 @@
 #define DPU_BLEND_BG_INV_MOD_ALPHA	(1 << 12)
 #define DPU_BLEND_BG_TRANSP_EN		(1 << 13)
 
-#define DPU_VSYNC0_SOURCE_GPIO		0
-#define DPU_VSYNC1_SOURCE_GPIO		1
-#define DPU_VSYNC2_SOURCE_GPIO		2
-#define DPU_VSYNC_SOURCE_INTF_0		3
-#define DPU_VSYNC_SOURCE_INTF_1		4
-#define DPU_VSYNC_SOURCE_INTF_2		5
-#define DPU_VSYNC_SOURCE_INTF_3		6
-#define DPU_VSYNC_SOURCE_WD_TIMER_4	11
-#define DPU_VSYNC_SOURCE_WD_TIMER_3	12
-#define DPU_VSYNC_SOURCE_WD_TIMER_2	13
-#define DPU_VSYNC_SOURCE_WD_TIMER_1	14
-#define DPU_VSYNC_SOURCE_WD_TIMER_0	15
+enum dpu_vsync_source {
+	DPU_VSYNC_SOURCE_GPIO_0,
+	DPU_VSYNC_SOURCE_GPIO_1,
+	DPU_VSYNC_SOURCE_GPIO_2,
+	DPU_VSYNC_SOURCE_INTF_0 = 3,
+	DPU_VSYNC_SOURCE_INTF_1,
+	DPU_VSYNC_SOURCE_INTF_2,
+	DPU_VSYNC_SOURCE_INTF_3,
+	DPU_VSYNC_SOURCE_WD_TIMER_4 = 11,
+	DPU_VSYNC_SOURCE_WD_TIMER_3,
+	DPU_VSYNC_SOURCE_WD_TIMER_2,
+	DPU_VSYNC_SOURCE_WD_TIMER_1,
+	DPU_VSYNC_SOURCE_WD_TIMER_0,
+};
 
 enum dpu_hw_blk_type {
 	DPU_HW_BLK_TOP = 0,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h
index 6f3dc98087df..5c9a7ede991e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h
@@ -64,7 +64,7 @@ struct dpu_vsync_source_cfg {
 	u32 pp_count;
 	u32 frame_rate;
 	u32 ppnumber[PINGPONG_MAX];
-	u32 vsync_source;
+	enum dpu_vsync_source vsync_source;
 };
 
 /**

-- 
2.39.2

[PATCH v2 3/8] drm/msm/dsi: drop unused GPIOs handling

[Dmitry Baryshkov]

Neither disp-enable-gpios nor disp-te-gpios are defined in the schema.
None of the board DT files use those GPIO pins. Drop them from the
driver.

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/dsi/dsi_host.c | 37 -------------------------------------
 1 file changed, 37 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index a50f4dda5941..c4d72562c95a 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -7,7 +7,6 @@
 #include <linux/delay.h>
 #include <linux/dma-mapping.h>
 #include <linux/err.h>
-#include <linux/gpio/consumer.h>
 #include <linux/interrupt.h>
 #include <linux/mfd/syscon.h>
 #include <linux/of.h>
@@ -130,9 +129,6 @@ struct msm_dsi_host {
 
 	unsigned long src_clk_rate;
 
-	struct gpio_desc *disp_en_gpio;
-	struct gpio_desc *te_gpio;
-
 	const struct msm_dsi_cfg_handler *cfg_hnd;
 
 	struct completion dma_comp;
@@ -1613,28 +1609,6 @@ static irqreturn_t dsi_host_irq(int irq, void *ptr)
 	return IRQ_HANDLED;
 }
 
-static int dsi_host_init_panel_gpios(struct msm_dsi_host *msm_host,
-			struct device *panel_device)
-{
-	msm_host->disp_en_gpio = devm_gpiod_get_optional(panel_device,
-							 "disp-enable",
-							 GPIOD_OUT_LOW);
-	if (IS_ERR(msm_host->disp_en_gpio)) {
-		DBG("cannot get disp-enable-gpios %ld",
-				PTR_ERR(msm_host->disp_en_gpio));
-		return PTR_ERR(msm_host->disp_en_gpio);
-	}
-
-	msm_host->te_gpio = devm_gpiod_get_optional(panel_device, "disp-te",
-								GPIOD_IN);
-	if (IS_ERR(msm_host->te_gpio)) {
-		DBG("cannot get disp-te-gpios %ld", PTR_ERR(msm_host->te_gpio));
-		return PTR_ERR(msm_host->te_gpio);
-	}
-
-	return 0;
-}
-
 static int dsi_host_attach(struct mipi_dsi_host *host,
 					struct mipi_dsi_device *dsi)
 {
@@ -1651,11 +1625,6 @@ static int dsi_host_attach(struct mipi_dsi_host *host,
 	if (dsi->dsc)
 		msm_host->dsc = dsi->dsc;
 
-	/* Some gpios defined in panel DT need to be controlled by host */
-	ret = dsi_host_init_panel_gpios(msm_host, &dsi->dev);
-	if (ret)
-		return ret;
-
 	ret = dsi_dev_attach(msm_host->pdev);
 	if (ret)
 		return ret;
@@ -2422,9 +2391,6 @@ int msm_dsi_host_power_on(struct mipi_dsi_host *host,
 	dsi_sw_reset(msm_host);
 	dsi_ctrl_enable(msm_host, phy_shared_timings, phy);
 
-	if (msm_host->disp_en_gpio)
-		gpiod_set_value(msm_host->disp_en_gpio, 1);
-
 	msm_host->power_on = true;
 	mutex_unlock(&msm_host->dev_mutex);
 
@@ -2454,9 +2420,6 @@ int msm_dsi_host_power_off(struct mipi_dsi_host *host)
 
 	dsi_ctrl_disable(msm_host);
 
-	if (msm_host->disp_en_gpio)
-		gpiod_set_value(msm_host->disp_en_gpio, 0);
-
 	pinctrl_pm_select_sleep_state(&msm_host->pdev->dev);
 
 	cfg_hnd->ops->link_clk_disable(msm_host);

-- 
2.39.2

[PATCH v2 4/8] drm/msm/dpu: pull the is_cmd_mode out of _dpu_encoder_update_vsync_source()

[Dmitry Baryshkov]

Setting vsync source makes sense only for DSI CMD panels. Pull the
is_cmd_mode condition out of the function into the calling code, so that
it becomes more explicit.

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 4988a1029431..bd37a56b4d03 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -736,8 +736,7 @@ static void _dpu_encoder_update_vsync_source(struct dpu_encoder_virt *dpu_enc,
 		return;
 	}
 
-	if (hw_mdptop->ops.setup_vsync_source &&
-			disp_info->is_cmd_mode) {
+	if (hw_mdptop->ops.setup_vsync_source) {
 		for (i = 0; i < dpu_enc->num_phys_encs; i++)
 			vsync_cfg.ppnumber[i] = dpu_enc->hw_pp[i]->idx;
 
@@ -1226,7 +1225,8 @@ static void _dpu_encoder_virt_enable_helper(struct drm_encoder *drm_enc)
 		dpu_enc->cur_master->hw_mdptop->ops.intf_audio_select(
 			dpu_enc->cur_master->hw_mdptop);
 
-	_dpu_encoder_update_vsync_source(dpu_enc, &dpu_enc->disp_info);
+	if (dpu_enc->disp_info.is_cmd_mode)
+		_dpu_encoder_update_vsync_source(dpu_enc, &dpu_enc->disp_info);
 
 	if (dpu_enc->disp_info.intf_type == INTF_DSI &&
 			!WARN_ON(dpu_enc->num_phys_encs == 0)) {

-- 
2.39.2

[PATCH v2 5/8] drm/msm/dpu: rework vsync_source handling

[Dmitry Baryshkov]

The struct msm_display_info has is_te_using_watchdog_timer field which
is neither used anywhere nor is flexible enough to specify different
sources. Replace it with the field specifying the vsync source using
enum dpu_vsync_source.

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 5 +----
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 5 ++---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     | 2 ++
 3 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index bd37a56b4d03..b147f8814a18 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -743,10 +743,7 @@ static void _dpu_encoder_update_vsync_source(struct dpu_encoder_virt *dpu_enc,
 		vsync_cfg.pp_count = dpu_enc->num_phys_encs;
 		vsync_cfg.frame_rate = drm_mode_vrefresh(&dpu_enc->base.crtc->state->adjusted_mode);
 
-		if (disp_info->is_te_using_watchdog_timer)
-			vsync_cfg.vsync_source = DPU_VSYNC_SOURCE_WD_TIMER_0;
-		else
-			vsync_cfg.vsync_source = DPU_VSYNC_SOURCE_GPIO_0;
+		vsync_cfg.vsync_source = disp_info->vsync_source;
 
 		hw_mdptop->ops.setup_vsync_source(hw_mdptop, &vsync_cfg);
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
index 76be77e30954..cb59bd4436f4 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
@@ -26,15 +26,14 @@
  * @h_tile_instance:    Controller instance used per tile. Number of elements is
  *                      based on num_of_h_tiles
  * @is_cmd_mode		Boolean to indicate if the CMD mode is requested
- * @is_te_using_watchdog_timer:  Boolean to indicate watchdog TE is
- *				 used instead of panel TE in cmd mode panels
+ * @vsync_source:	Source of the TE signal for DSI CMD devices
  */
 struct msm_display_info {
 	enum dpu_intf_type intf_type;
 	uint32_t num_of_h_tiles;
 	uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY];
 	bool is_cmd_mode;
-	bool is_te_using_watchdog_timer;
+	enum dpu_vsync_source vsync_source;
 };
 
 /**
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 1955848b1b78..e9991f3756d4 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -543,6 +543,8 @@ static int _dpu_kms_initialize_dsi(struct drm_device *dev,
 
 		info.is_cmd_mode = msm_dsi_is_cmd_mode(priv->dsi[i]);
 
+		info.vsync_source = DPU_VSYNC_SOURCE_GPIO_0;
+
 		encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_DSI, &info);
 		if (IS_ERR(encoder)) {
 			DPU_ERROR("encoder init failed for dsi display\n");

-- 
2.39.2

[PATCH v2 6/8] drm/msm/dsi: parse vsync source from device tree

[Dmitry Baryshkov]

Allow board's device tree to specify the vsync source (aka TE source).
If the property is omitted, the display controller driver will use the
default setting.

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/dsi/dsi.h         |  1 +
 drivers/gpu/drm/msm/dsi/dsi_host.c    | 11 +++++++++++
 drivers/gpu/drm/msm/dsi/dsi_manager.c |  5 +++++
 drivers/gpu/drm/msm/msm_drv.h         |  6 ++++++
 4 files changed, 23 insertions(+)

diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
index afc290408ba4..87496db203d6 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.h
+++ b/drivers/gpu/drm/msm/dsi/dsi.h
@@ -37,6 +37,7 @@ struct msm_dsi {
 
 	struct mipi_dsi_host *host;
 	struct msm_dsi_phy *phy;
+	const char *te_source;
 
 	struct drm_bridge *next_bridge;
 
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index c4d72562c95a..c26ad0fed54d 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -1786,9 +1786,11 @@ static int dsi_populate_dsc_params(struct msm_dsi_host *msm_host, struct drm_dsc
 
 static int dsi_host_parse_dt(struct msm_dsi_host *msm_host)
 {
+	struct msm_dsi *msm_dsi = platform_get_drvdata(msm_host->pdev);
 	struct device *dev = &msm_host->pdev->dev;
 	struct device_node *np = dev->of_node;
 	struct device_node *endpoint;
+	const char *te_source;
 	int ret = 0;
 
 	/*
@@ -1811,6 +1813,15 @@ static int dsi_host_parse_dt(struct msm_dsi_host *msm_host)
 		goto err;
 	}
 
+	ret = of_property_read_string(endpoint, "qcom,te-source", &te_source);
+	if (ret && ret != -EINVAL) {
+		DRM_DEV_ERROR(dev, "%s: invalid TE source configuration %d\n",
+			__func__, ret);
+		goto err;
+	}
+	if (!ret)
+		msm_dsi->te_source = devm_kstrdup(dev, te_source, GFP_KERNEL);
+
 	if (of_property_read_bool(np, "syscon-sfpb")) {
 		msm_host->sfpb = syscon_regmap_lookup_by_phandle(np,
 					"syscon-sfpb");
diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index 5b3f3068fd92..a210b7c9e5ca 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -603,3 +603,8 @@ bool msm_dsi_is_master_dsi(struct msm_dsi *msm_dsi)
 {
 	return IS_MASTER_DSI_LINK(msm_dsi->id);
 }
+
+const char *msm_dsi_get_te_source(struct msm_dsi *msm_dsi)
+{
+	return msm_dsi->te_source;
+}
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 912ebaa5df84..afd98dffea99 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -330,6 +330,7 @@ bool msm_dsi_is_bonded_dsi(struct msm_dsi *msm_dsi);
 bool msm_dsi_is_master_dsi(struct msm_dsi *msm_dsi);
 bool msm_dsi_wide_bus_enabled(struct msm_dsi *msm_dsi);
 struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_dsi);
+const char *msm_dsi_get_te_source(struct msm_dsi *msm_dsi);
 #else
 static inline void __init msm_dsi_register(void)
 {
@@ -367,6 +368,11 @@ static inline struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_
 {
 	return NULL;
 }
+
+static inline const char *msm_dsi_get_te_source(struct msm_dsi *msm_dsi)
+{
+	return NULL;
+}
 #endif
 
 #ifdef CONFIG_DRM_MSM_DP

-- 
2.39.2

[PATCH v2 7/8] drm/msm/dpu: support setting the TE source

[Dmitry Baryshkov]

Make the DPU driver use the TE source specified in the DT. If none is
specified, the driver defaults to the first GPIO (mdp_vsync0).

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

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index e9991f3756d4..6fcb3cf4a382 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -505,6 +505,44 @@ static void dpu_kms_wait_flush(struct msm_kms *kms, unsigned crtc_mask)
 		dpu_kms_wait_for_commit_done(kms, crtc);
 }
 
+static const char *dpu_vsync_sources[] = {
+	[DPU_VSYNC_SOURCE_GPIO_0] = "mdp_vsync_p",
+	[DPU_VSYNC_SOURCE_GPIO_1] = "mdp_vsync_s",
+	[DPU_VSYNC_SOURCE_GPIO_2] = "mdp_vsync_e",
+	[DPU_VSYNC_SOURCE_INTF_0] = "mdp_intf0",
+	[DPU_VSYNC_SOURCE_INTF_1] = "mdp_intf1",
+	[DPU_VSYNC_SOURCE_INTF_2] = "mdp_intf2",
+	[DPU_VSYNC_SOURCE_INTF_3] = "mdp_intf3",
+	[DPU_VSYNC_SOURCE_WD_TIMER_0] = "timer0",
+	[DPU_VSYNC_SOURCE_WD_TIMER_1] = "timer1",
+	[DPU_VSYNC_SOURCE_WD_TIMER_2] = "timer2",
+	[DPU_VSYNC_SOURCE_WD_TIMER_3] = "timer3",
+	[DPU_VSYNC_SOURCE_WD_TIMER_4] = "timer4",
+};
+
+static int dpu_kms_dsi_set_te_source(struct msm_display_info *info,
+				     struct msm_dsi *dsi)
+{
+	const char *te_source = msm_dsi_get_te_source(dsi);
+	int i;
+
+	if (!te_source) {
+		info->vsync_source = DPU_VSYNC_SOURCE_GPIO_0;
+		return 0;
+	}
+
+	/* we can not use match_string since dpu_vsync_sources is a sparse array */
+	for (i = 0; i < ARRAY_SIZE(dpu_vsync_sources); i++) {
+		if (dpu_vsync_sources[i] &&
+		    !strcmp(dpu_vsync_sources[i], te_source)) {
+			info->vsync_source = i;
+			return 0;
+		}
+	}
+
+	return -EINVAL;
+}
+
 static int _dpu_kms_initialize_dsi(struct drm_device *dev,
 				    struct msm_drm_private *priv,
 				    struct dpu_kms *dpu_kms)
@@ -543,7 +581,11 @@ static int _dpu_kms_initialize_dsi(struct drm_device *dev,
 
 		info.is_cmd_mode = msm_dsi_is_cmd_mode(priv->dsi[i]);
 
-		info.vsync_source = DPU_VSYNC_SOURCE_GPIO_0;
+		rc = dpu_kms_dsi_set_te_source(&info, priv->dsi[i]);
+		if (rc) {
+			DPU_ERROR("failed to identify TE source for dsi display\n");
+			return rc;
+		}
 
 		encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_DSI, &info);
 		if (IS_ERR(encoder)) {

-- 
2.39.2

[PATCH v2 8/8] drm/msm/dpu: rename dpu_hw_setup_vsync_source functions

[Dmitry Baryshkov]

Rename dpu_hw_setup_vsync_source functions to make the names match the
implementation: on DPU 5.x the TOP only contains timer setup, while 3.x
and 4.x used MDP_VSYNC_SEL register to select TE source.

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

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c
index 05e48cf4ec1d..6e2ac50b94a4 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c
@@ -107,8 +107,8 @@ static void dpu_hw_get_danger_status(struct dpu_hw_mdp *mdp,
 	status->sspp[SSPP_CURSOR1] = (value >> 26) & 0x3;
 }
 
-static void dpu_hw_setup_vsync_source(struct dpu_hw_mdp *mdp,
-		struct dpu_vsync_source_cfg *cfg)
+static void dpu_hw_setup_wd_timer(struct dpu_hw_mdp *mdp,
+				  struct dpu_vsync_source_cfg *cfg)
 {
 	struct dpu_hw_blk_reg_map *c;
 	u32 reg, wd_load_value, wd_ctl, wd_ctl2;
@@ -163,8 +163,8 @@ static void dpu_hw_setup_vsync_source(struct dpu_hw_mdp *mdp,
 	}
 }
 
-static void dpu_hw_setup_vsync_source_and_vsync_sel(struct dpu_hw_mdp *mdp,
-		struct dpu_vsync_source_cfg *cfg)
+static void dpu_hw_setup_vsync_sel(struct dpu_hw_mdp *mdp,
+				   struct dpu_vsync_source_cfg *cfg)
 {
 	struct dpu_hw_blk_reg_map *c;
 	u32 reg, i;
@@ -187,7 +187,7 @@ static void dpu_hw_setup_vsync_source_and_vsync_sel(struct dpu_hw_mdp *mdp,
 	}
 	DPU_REG_WRITE(c, MDP_VSYNC_SEL, reg);
 
-	dpu_hw_setup_vsync_source(mdp, cfg);
+	dpu_hw_setup_wd_timer(mdp, cfg);
 }
 
 static void dpu_hw_get_safe_status(struct dpu_hw_mdp *mdp,
@@ -239,9 +239,9 @@ static void _setup_mdp_ops(struct dpu_hw_mdp_ops *ops,
 	ops->get_danger_status = dpu_hw_get_danger_status;
 
 	if (cap & BIT(DPU_MDP_VSYNC_SEL))
-		ops->setup_vsync_source = dpu_hw_setup_vsync_source_and_vsync_sel;
+		ops->setup_vsync_source = dpu_hw_setup_vsync_sel;
 	else
-		ops->setup_vsync_source = dpu_hw_setup_vsync_source;
+		ops->setup_vsync_source = dpu_hw_setup_wd_timer;
 
 	ops->get_safe_status = dpu_hw_get_safe_status;
 

-- 
2.39.2

Re: [PATCH v2 3/8] drm/msm/dsi: drop unused GPIOs handling

[Marijn Suijten]

On 2024-06-13 20:05:06, Dmitry Baryshkov wrote:
> Neither disp-enable-gpios nor disp-te-gpios are defined in the schema.
> None of the board DT files use those GPIO pins. Drop them from the
> driver.

What's worse, when people set disp-te-gpios the
devm_gpiod_get_optional("disp-te", GPIOD_IN) below resets the typical mdp_vsync
function via pinctrl to the IN function, causing vsync signals to be lost and
the MDP hardware to fall back to half the requested refresh rate since commit
da9e7b7696d8 ("drm/msm/dpu: Correctly configure vsync tearcheck for command
mode").

> 
> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

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

> ---
>  drivers/gpu/drm/msm/dsi/dsi_host.c | 37 -------------------------------------
>  1 file changed, 37 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index a50f4dda5941..c4d72562c95a 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -7,7 +7,6 @@
>  #include <linux/delay.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/err.h>
> -#include <linux/gpio/consumer.h>
>  #include <linux/interrupt.h>
>  #include <linux/mfd/syscon.h>
>  #include <linux/of.h>
> @@ -130,9 +129,6 @@ struct msm_dsi_host {
>  
>  	unsigned long src_clk_rate;
>  
> -	struct gpio_desc *disp_en_gpio;
> -	struct gpio_desc *te_gpio;
> -
>  	const struct msm_dsi_cfg_handler *cfg_hnd;
>  
>  	struct completion dma_comp;
> @@ -1613,28 +1609,6 @@ static irqreturn_t dsi_host_irq(int irq, void *ptr)
>  	return IRQ_HANDLED;
>  }
>  
> -static int dsi_host_init_panel_gpios(struct msm_dsi_host *msm_host,
> -			struct device *panel_device)
> -{
> -	msm_host->disp_en_gpio = devm_gpiod_get_optional(panel_device,
> -							 "disp-enable",
> -							 GPIOD_OUT_LOW);
> -	if (IS_ERR(msm_host->disp_en_gpio)) {
> -		DBG("cannot get disp-enable-gpios %ld",
> -				PTR_ERR(msm_host->disp_en_gpio));
> -		return PTR_ERR(msm_host->disp_en_gpio);
> -	}
> -
> -	msm_host->te_gpio = devm_gpiod_get_optional(panel_device, "disp-te",
> -								GPIOD_IN);
> -	if (IS_ERR(msm_host->te_gpio)) {
> -		DBG("cannot get disp-te-gpios %ld", PTR_ERR(msm_host->te_gpio));
> -		return PTR_ERR(msm_host->te_gpio);
> -	}
> -
> -	return 0;
> -}
> -
>  static int dsi_host_attach(struct mipi_dsi_host *host,
>  					struct mipi_dsi_device *dsi)
>  {
> @@ -1651,11 +1625,6 @@ static int dsi_host_attach(struct mipi_dsi_host *host,
>  	if (dsi->dsc)
>  		msm_host->dsc = dsi->dsc;
>  
> -	/* Some gpios defined in panel DT need to be controlled by host */
> -	ret = dsi_host_init_panel_gpios(msm_host, &dsi->dev);
> -	if (ret)
> -		return ret;
> -
>  	ret = dsi_dev_attach(msm_host->pdev);
>  	if (ret)
>  		return ret;
> @@ -2422,9 +2391,6 @@ int msm_dsi_host_power_on(struct mipi_dsi_host *host,
>  	dsi_sw_reset(msm_host);
>  	dsi_ctrl_enable(msm_host, phy_shared_timings, phy);
>  
> -	if (msm_host->disp_en_gpio)
> -		gpiod_set_value(msm_host->disp_en_gpio, 1);
> -
>  	msm_host->power_on = true;
>  	mutex_unlock(&msm_host->dev_mutex);
>  
> @@ -2454,9 +2420,6 @@ int msm_dsi_host_power_off(struct mipi_dsi_host *host)
>  
>  	dsi_ctrl_disable(msm_host);
>  
> -	if (msm_host->disp_en_gpio)
> -		gpiod_set_value(msm_host->disp_en_gpio, 0);
> -
>  	pinctrl_pm_select_sleep_state(&msm_host->pdev->dev);
>  
>  	cfg_hnd->ops->link_clk_disable(msm_host);
> 
> -- 
> 2.39.2
> 

Re: [PATCH v2 7/8] drm/msm/dpu: support setting the TE source

[Marijn Suijten]

On 2024-06-13 20:05:10, Dmitry Baryshkov wrote:
> Make the DPU driver use the TE source specified in the DT. If none is
> specified, the driver defaults to the first GPIO (mdp_vsync0).

mdp_vsync_p?

> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 44 ++++++++++++++++++++++++++++++++-
>  1 file changed, 43 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index e9991f3756d4..6fcb3cf4a382 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -505,6 +505,44 @@ static void dpu_kms_wait_flush(struct msm_kms *kms, unsigned crtc_mask)
>  		dpu_kms_wait_for_commit_done(kms, crtc);
>  }
>  
> +static const char *dpu_vsync_sources[] = {
> +	[DPU_VSYNC_SOURCE_GPIO_0] = "mdp_vsync_p",
> +	[DPU_VSYNC_SOURCE_GPIO_1] = "mdp_vsync_s",
> +	[DPU_VSYNC_SOURCE_GPIO_2] = "mdp_vsync_e",
> +	[DPU_VSYNC_SOURCE_INTF_0] = "mdp_intf0",
> +	[DPU_VSYNC_SOURCE_INTF_1] = "mdp_intf1",
> +	[DPU_VSYNC_SOURCE_INTF_2] = "mdp_intf2",
> +	[DPU_VSYNC_SOURCE_INTF_3] = "mdp_intf3",
> +	[DPU_VSYNC_SOURCE_WD_TIMER_0] = "timer0",
> +	[DPU_VSYNC_SOURCE_WD_TIMER_1] = "timer1",
> +	[DPU_VSYNC_SOURCE_WD_TIMER_2] = "timer2",
> +	[DPU_VSYNC_SOURCE_WD_TIMER_3] = "timer3",
> +	[DPU_VSYNC_SOURCE_WD_TIMER_4] = "timer4",
> +};
> +
> +static int dpu_kms_dsi_set_te_source(struct msm_display_info *info,
> +				     struct msm_dsi *dsi)
> +{
> +	const char *te_source = msm_dsi_get_te_source(dsi);

Just checking: if the TE source is different and one has dual-DSI, it must be
set on both controllers?

> +	int i;
> +
> +	if (!te_source) {
> +		info->vsync_source = DPU_VSYNC_SOURCE_GPIO_0;
> +		return 0;
> +	}
> +
> +	/* we can not use match_string since dpu_vsync_sources is a sparse array */

Instead of having gaps in the array, you could also store both the vsync_source
and name as the array elements?

> +	for (i = 0; i < ARRAY_SIZE(dpu_vsync_sources); i++) {
> +		if (dpu_vsync_sources[i] &&
> +		    !strcmp(dpu_vsync_sources[i], te_source)) {
> +			info->vsync_source = i;
> +			return 0;
> +		}
> +	}
> +
> +	return -EINVAL;
> +}
> +
>  static int _dpu_kms_initialize_dsi(struct drm_device *dev,
>  				    struct msm_drm_private *priv,
>  				    struct dpu_kms *dpu_kms)
> @@ -543,7 +581,11 @@ static int _dpu_kms_initialize_dsi(struct drm_device *dev,
>  
>  		info.is_cmd_mode = msm_dsi_is_cmd_mode(priv->dsi[i]);
>  
> -		info.vsync_source = DPU_VSYNC_SOURCE_GPIO_0;
> +		rc = dpu_kms_dsi_set_te_source(&info, priv->dsi[i]);
> +		if (rc) {
> +			DPU_ERROR("failed to identify TE source for dsi display\n");
> +			return rc;
> +		}
>  
>  		encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_DSI, &info);
>  		if (IS_ERR(encoder)) {
> 
> -- 
> 2.39.2
> 

Re: [PATCH v2 1/8] dt-bindings: display/msm/dsi: allow specifying TE source

[Marijn Suijten]

On 2024-06-13 20:05:04, Dmitry Baryshkov wrote:
> Command mode panels provide TE signal back to the DSI host to signal
> that the frame display has completed and update of the image will not
> cause tearing. Usually it is connected to the first GPIO with the
> mdp_vsync function, which is the default. In such case the property can
> be skipped.
> 
> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  .../bindings/display/msm/dsi-controller-main.yaml       | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml b/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
> index 1fa28e976559..e1cb3a1fee81 100644
> --- a/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
> +++ b/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
> @@ -162,6 +162,22 @@ properties:
>                  items:
>                    enum: [ 0, 1, 2, 3 ]
>  
> +              qcom,te-source:
> +                $ref: /schemas/types.yaml#/definitions/string
> +                description:
> +                  Specifies the source of vsync signal from the panel used for
> +                  tearing elimination.
> +                default: mdp_vsync_p
> +                enum:
> +                  - mdp_vsync_p
> +                  - mdp_vsync_s
> +                  - mdp_vsync_e

When discussing that these should be renamed, was it also documented what the
suffix means?  I can only guess something like primary/secondary/e...?

Are the mdp_intfX variants missing here that you're handling in patch 7/8?

> +                  - timer0
> +                  - timer1
> +                  - timer2
> +                  - timer3
> +                  - timer4
> +
>      required:
>        - port@0
>        - port@1
> @@ -452,6 +468,7 @@ examples:
>                            dsi0_out: endpoint {
>                                     remote-endpoint = <&sn65dsi86_in>;
>                                     data-lanes = <0 1 2 3>;
> +                                   qcom,te-source = "mdp_vsync_e";
>                            };
>                    };
>             };
> 
> -- 
> 2.39.2
> 

Re: [PATCH v2 2/8] drm/msm/dpu: convert vsync source defines to the enum

[Marijn Suijten]

On 2024-06-13 20:05:05, Dmitry Baryshkov wrote:
> Add enum dpu_vsync_source instead of a series of defines. Use this enum
> to pass vsync information.
> 
> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c |  2 +-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c |  2 +-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h |  2 +-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h | 26 ++++++++++++++------------
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h  |  2 +-
>  5 files changed, 18 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 119f3ea50a7c..4988a1029431 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -747,7 +747,7 @@ static void _dpu_encoder_update_vsync_source(struct dpu_encoder_virt *dpu_enc,
>  		if (disp_info->is_te_using_watchdog_timer)
>  			vsync_cfg.vsync_source = DPU_VSYNC_SOURCE_WD_TIMER_0;
>  		else
> -			vsync_cfg.vsync_source = DPU_VSYNC0_SOURCE_GPIO;
> +			vsync_cfg.vsync_source = DPU_VSYNC_SOURCE_GPIO_0;
>  
>  		hw_mdptop->ops.setup_vsync_source(hw_mdptop, &vsync_cfg);
>  
> 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 225c1c7768ff..96f6160cf607 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> @@ -462,7 +462,7 @@ static int dpu_hw_intf_get_vsync_info(struct dpu_hw_intf *intf,
>  }
>  
>  static void dpu_hw_intf_vsync_sel(struct dpu_hw_intf *intf,
> -		u32 vsync_source)
> +				  enum dpu_vsync_source vsync_source)
>  {
>  	struct dpu_hw_blk_reg_map *c;
>  
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> index f9015c67a574..ac244f0b33fb 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> @@ -107,7 +107,7 @@ struct dpu_hw_intf_ops {
>  
>  	int (*connect_external_te)(struct dpu_hw_intf *intf, bool enable_external_te);
>  
> -	void (*vsync_sel)(struct dpu_hw_intf *intf, u32 vsync_source);
> +	void (*vsync_sel)(struct dpu_hw_intf *intf, enum dpu_vsync_source vsync_source);
>  
>  	/**
>  	 * Disable autorefresh if enabled
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
> index 66759623fc42..a2eff36a2224 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
> @@ -54,18 +54,20 @@
>  #define DPU_BLEND_BG_INV_MOD_ALPHA	(1 << 12)
>  #define DPU_BLEND_BG_TRANSP_EN		(1 << 13)
>  
> -#define DPU_VSYNC0_SOURCE_GPIO		0
> -#define DPU_VSYNC1_SOURCE_GPIO		1
> -#define DPU_VSYNC2_SOURCE_GPIO		2
> -#define DPU_VSYNC_SOURCE_INTF_0		3
> -#define DPU_VSYNC_SOURCE_INTF_1		4
> -#define DPU_VSYNC_SOURCE_INTF_2		5
> -#define DPU_VSYNC_SOURCE_INTF_3		6
> -#define DPU_VSYNC_SOURCE_WD_TIMER_4	11
> -#define DPU_VSYNC_SOURCE_WD_TIMER_3	12
> -#define DPU_VSYNC_SOURCE_WD_TIMER_2	13
> -#define DPU_VSYNC_SOURCE_WD_TIMER_1	14
> -#define DPU_VSYNC_SOURCE_WD_TIMER_0	15
> +enum dpu_vsync_source {
> +	DPU_VSYNC_SOURCE_GPIO_0,
> +	DPU_VSYNC_SOURCE_GPIO_1,
> +	DPU_VSYNC_SOURCE_GPIO_2,

While at it, rename this to _P / _S / _E to match the other patches/code?

- Marijn

> +	DPU_VSYNC_SOURCE_INTF_0 = 3,
> +	DPU_VSYNC_SOURCE_INTF_1,
> +	DPU_VSYNC_SOURCE_INTF_2,
> +	DPU_VSYNC_SOURCE_INTF_3,
> +	DPU_VSYNC_SOURCE_WD_TIMER_4 = 11,
> +	DPU_VSYNC_SOURCE_WD_TIMER_3,
> +	DPU_VSYNC_SOURCE_WD_TIMER_2,
> +	DPU_VSYNC_SOURCE_WD_TIMER_1,
> +	DPU_VSYNC_SOURCE_WD_TIMER_0,
> +};
>  
>  enum dpu_hw_blk_type {
>  	DPU_HW_BLK_TOP = 0,
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h
> index 6f3dc98087df..5c9a7ede991e 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h
> @@ -64,7 +64,7 @@ struct dpu_vsync_source_cfg {
>  	u32 pp_count;
>  	u32 frame_rate;
>  	u32 ppnumber[PINGPONG_MAX];
> -	u32 vsync_source;
> +	enum dpu_vsync_source vsync_source;
>  };
>  
>  /**
> 
> -- 
> 2.39.2
> 

Re: [PATCH v2 4/8] drm/msm/dpu: pull the is_cmd_mode out of _dpu_encoder_update_vsync_source()

[Marijn Suijten]

On 2024-06-13 20:05:07, Dmitry Baryshkov wrote:
> Setting vsync source makes sense only for DSI CMD panels. Pull the
> is_cmd_mode condition out of the function into the calling code, so that
> it becomes more explicit.
> 
> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> 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.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 4988a1029431..bd37a56b4d03 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -736,8 +736,7 @@ static void _dpu_encoder_update_vsync_source(struct dpu_encoder_virt *dpu_enc,
>  		return;
>  	}
>  
> -	if (hw_mdptop->ops.setup_vsync_source &&
> -			disp_info->is_cmd_mode) {
> +	if (hw_mdptop->ops.setup_vsync_source) {
>  		for (i = 0; i < dpu_enc->num_phys_encs; i++)
>  			vsync_cfg.ppnumber[i] = dpu_enc->hw_pp[i]->idx;
>  
> @@ -1226,7 +1225,8 @@ static void _dpu_encoder_virt_enable_helper(struct drm_encoder *drm_enc)
>  		dpu_enc->cur_master->hw_mdptop->ops.intf_audio_select(
>  			dpu_enc->cur_master->hw_mdptop);
>  
> -	_dpu_encoder_update_vsync_source(dpu_enc, &dpu_enc->disp_info);
> +	if (dpu_enc->disp_info.is_cmd_mode)
> +		_dpu_encoder_update_vsync_source(dpu_enc, &dpu_enc->disp_info);
>  
>  	if (dpu_enc->disp_info.intf_type == INTF_DSI &&
>  			!WARN_ON(dpu_enc->num_phys_encs == 0)) {
> 
> -- 
> 2.39.2
> 

Re: [PATCH v2 5/8] drm/msm/dpu: rework vsync_source handling

[Marijn Suijten]

Maybe retitle this to something that more closely resembles "remove unset
is_te_using_watchdog_timer field"?

On 2024-06-13 20:05:08, Dmitry Baryshkov wrote:
> The struct msm_display_info has is_te_using_watchdog_timer field which
> is neither used anywhere nor is flexible enough to specify different

Well, it's "used", but not "set" (to anything other than the zero-initialized
default). s/used/set?

> sources. Replace it with the field specifying the vsync source using
> enum dpu_vsync_source.
> 
> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Patch itself is fine, just think the title could be clearer:

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

> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 5 +----
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 5 ++---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     | 2 ++
>  3 files changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index bd37a56b4d03..b147f8814a18 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -743,10 +743,7 @@ static void _dpu_encoder_update_vsync_source(struct dpu_encoder_virt *dpu_enc,
>  		vsync_cfg.pp_count = dpu_enc->num_phys_encs;
>  		vsync_cfg.frame_rate = drm_mode_vrefresh(&dpu_enc->base.crtc->state->adjusted_mode);
>  
> -		if (disp_info->is_te_using_watchdog_timer)
> -			vsync_cfg.vsync_source = DPU_VSYNC_SOURCE_WD_TIMER_0;
> -		else
> -			vsync_cfg.vsync_source = DPU_VSYNC_SOURCE_GPIO_0;
> +		vsync_cfg.vsync_source = disp_info->vsync_source;
>  
>  		hw_mdptop->ops.setup_vsync_source(hw_mdptop, &vsync_cfg);
>  
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> index 76be77e30954..cb59bd4436f4 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> @@ -26,15 +26,14 @@
>   * @h_tile_instance:    Controller instance used per tile. Number of elements is
>   *                      based on num_of_h_tiles
>   * @is_cmd_mode		Boolean to indicate if the CMD mode is requested
> - * @is_te_using_watchdog_timer:  Boolean to indicate watchdog TE is
> - *				 used instead of panel TE in cmd mode panels
> + * @vsync_source:	Source of the TE signal for DSI CMD devices
>   */
>  struct msm_display_info {
>  	enum dpu_intf_type intf_type;
>  	uint32_t num_of_h_tiles;
>  	uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY];
>  	bool is_cmd_mode;
> -	bool is_te_using_watchdog_timer;
> +	enum dpu_vsync_source vsync_source;
>  };
>  
>  /**
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 1955848b1b78..e9991f3756d4 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -543,6 +543,8 @@ static int _dpu_kms_initialize_dsi(struct drm_device *dev,
>  
>  		info.is_cmd_mode = msm_dsi_is_cmd_mode(priv->dsi[i]);
>  
> +		info.vsync_source = DPU_VSYNC_SOURCE_GPIO_0;
> +
>  		encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_DSI, &info);
>  		if (IS_ERR(encoder)) {
>  			DPU_ERROR("encoder init failed for dsi display\n");
> 
> -- 
> 2.39.2
> 

Re: [PATCH v2 6/8] drm/msm/dsi: parse vsync source from device tree

[Marijn Suijten]

On 2024-06-13 20:05:09, Dmitry Baryshkov wrote:
> Allow board's device tree to specify the vsync source (aka TE source).
> If the property is omitted, the display controller driver will use the
> default setting.

Well, that specific default handling is not really part of this patch, but
how a followup patch is going to respond when msm_dsi_get_te_source() returns
NULL. (Or how that followup patch is expected to deal with that - worth a
doc-comment?)

> 
> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

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

> ---
>  drivers/gpu/drm/msm/dsi/dsi.h         |  1 +
>  drivers/gpu/drm/msm/dsi/dsi_host.c    | 11 +++++++++++
>  drivers/gpu/drm/msm/dsi/dsi_manager.c |  5 +++++
>  drivers/gpu/drm/msm/msm_drv.h         |  6 ++++++
>  4 files changed, 23 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
> index afc290408ba4..87496db203d6 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi.h
> +++ b/drivers/gpu/drm/msm/dsi/dsi.h
> @@ -37,6 +37,7 @@ struct msm_dsi {
>  
>  	struct mipi_dsi_host *host;
>  	struct msm_dsi_phy *phy;
> +	const char *te_source;
>  
>  	struct drm_bridge *next_bridge;
>  
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index c4d72562c95a..c26ad0fed54d 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -1786,9 +1786,11 @@ static int dsi_populate_dsc_params(struct msm_dsi_host *msm_host, struct drm_dsc
>  
>  static int dsi_host_parse_dt(struct msm_dsi_host *msm_host)
>  {
> +	struct msm_dsi *msm_dsi = platform_get_drvdata(msm_host->pdev);
>  	struct device *dev = &msm_host->pdev->dev;
>  	struct device_node *np = dev->of_node;
>  	struct device_node *endpoint;
> +	const char *te_source;
>  	int ret = 0;
>  
>  	/*
> @@ -1811,6 +1813,15 @@ static int dsi_host_parse_dt(struct msm_dsi_host *msm_host)
>  		goto err;
>  	}
>  
> +	ret = of_property_read_string(endpoint, "qcom,te-source", &te_source);
> +	if (ret && ret != -EINVAL) {
> +		DRM_DEV_ERROR(dev, "%s: invalid TE source configuration %d\n",
> +			__func__, ret);
> +		goto err;
> +	}
> +	if (!ret)
> +		msm_dsi->te_source = devm_kstrdup(dev, te_source, GFP_KERNEL);
> +
>  	if (of_property_read_bool(np, "syscon-sfpb")) {
>  		msm_host->sfpb = syscon_regmap_lookup_by_phandle(np,
>  					"syscon-sfpb");
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> index 5b3f3068fd92..a210b7c9e5ca 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> @@ -603,3 +603,8 @@ bool msm_dsi_is_master_dsi(struct msm_dsi *msm_dsi)
>  {
>  	return IS_MASTER_DSI_LINK(msm_dsi->id);
>  }
> +
> +const char *msm_dsi_get_te_source(struct msm_dsi *msm_dsi)
> +{
> +	return msm_dsi->te_source;
> +}
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index 912ebaa5df84..afd98dffea99 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -330,6 +330,7 @@ bool msm_dsi_is_bonded_dsi(struct msm_dsi *msm_dsi);
>  bool msm_dsi_is_master_dsi(struct msm_dsi *msm_dsi);
>  bool msm_dsi_wide_bus_enabled(struct msm_dsi *msm_dsi);
>  struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_dsi);
> +const char *msm_dsi_get_te_source(struct msm_dsi *msm_dsi);
>  #else
>  static inline void __init msm_dsi_register(void)
>  {
> @@ -367,6 +368,11 @@ static inline struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_
>  {
>  	return NULL;
>  }
> +
> +static inline const char *msm_dsi_get_te_source(struct msm_dsi *msm_dsi)
> +{
> +	return NULL;
> +}
>  #endif
>  
>  #ifdef CONFIG_DRM_MSM_DP
> 
> -- 
> 2.39.2
> 

Re: [PATCH v2 1/8] dt-bindings: display/msm/dsi: allow specifying TE source

[Dmitry Baryshkov]

On Thu, 13 Jun 2024 at 21:16, Marijn Suijten
<marijn.suijten@somainline.org> wrote:
>
> On 2024-06-13 20:05:04, Dmitry Baryshkov wrote:
> > Command mode panels provide TE signal back to the DSI host to signal
> > that the frame display has completed and update of the image will not
> > cause tearing. Usually it is connected to the first GPIO with the
> > mdp_vsync function, which is the default. In such case the property can
> > be skipped.
> >
> > Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >  .../bindings/display/msm/dsi-controller-main.yaml       | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml b/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
> > index 1fa28e976559..e1cb3a1fee81 100644
> > --- a/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
> > +++ b/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
> > @@ -162,6 +162,22 @@ properties:
> >                  items:
> >                    enum: [ 0, 1, 2, 3 ]
> >
> > +              qcom,te-source:
> > +                $ref: /schemas/types.yaml#/definitions/string
> > +                description:
> > +                  Specifies the source of vsync signal from the panel used for
> > +                  tearing elimination.
> > +                default: mdp_vsync_p
> > +                enum:
> > +                  - mdp_vsync_p
> > +                  - mdp_vsync_s
> > +                  - mdp_vsync_e
>
> When discussing that these should be renamed, was it also documented what the
> suffix means?  I can only guess something like primary/secondary/e...?

external. Note, these names match the name in the datasheets.

>
> Are the mdp_intfX variants missing here that you're handling in patch 7/8?

I didn't test them, so I didn't document them.

>
> > +                  - timer0
> > +                  - timer1
> > +                  - timer2
> > +                  - timer3
> > +                  - timer4
> > +
> >      required:
> >        - port@0
> >        - port@1
> > @@ -452,6 +468,7 @@ examples:
> >                            dsi0_out: endpoint {
> >                                     remote-endpoint = <&sn65dsi86_in>;
> >                                     data-lanes = <0 1 2 3>;
> > +                                   qcom,te-source = "mdp_vsync_e";
> >                            };
> >                    };
> >             };
> >
> > --
> > 2.39.2
> >



-- 
With best wishes
Dmitry

Re: [PATCH v2 8/8] drm/msm/dpu: rename dpu_hw_setup_vsync_source functions

[Marijn Suijten]

On 2024-06-13 20:05:11, Dmitry Baryshkov wrote:
> Rename dpu_hw_setup_vsync_source functions to make the names match the
> implementation: on DPU 5.x the TOP only contains timer setup, while 3.x
> and 4.x used MDP_VSYNC_SEL register to select TE source.

Yeah that was never really clear anymore after I split this function in two in
commit a2ff096803b3 ("drm/msm/dpu: Disable MDP vsync source selection on DPU
5.0.0 and above").

> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c
> index 05e48cf4ec1d..6e2ac50b94a4 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c
> @@ -107,8 +107,8 @@ static void dpu_hw_get_danger_status(struct dpu_hw_mdp *mdp,
>  	status->sspp[SSPP_CURSOR1] = (value >> 26) & 0x3;
>  }
>  
> -static void dpu_hw_setup_vsync_source(struct dpu_hw_mdp *mdp,
> -		struct dpu_vsync_source_cfg *cfg)
> +static void dpu_hw_setup_wd_timer(struct dpu_hw_mdp *mdp,
> +				  struct dpu_vsync_source_cfg *cfg)
>  {
>  	struct dpu_hw_blk_reg_map *c;
>  	u32 reg, wd_load_value, wd_ctl, wd_ctl2;
> @@ -163,8 +163,8 @@ static void dpu_hw_setup_vsync_source(struct dpu_hw_mdp *mdp,
>  	}
>  }
>  
> -static void dpu_hw_setup_vsync_source_and_vsync_sel(struct dpu_hw_mdp *mdp,
> -		struct dpu_vsync_source_cfg *cfg)
> +static void dpu_hw_setup_vsync_sel(struct dpu_hw_mdp *mdp,

Maybe keep setup_wd_timer_and_vsync_sel()?  OTOH it doesn't always set wd_timer,
only when vsync_source calls for it.

> +				   struct dpu_vsync_source_cfg *cfg)
>  {
>  	struct dpu_hw_blk_reg_map *c;
>  	u32 reg, i;
> @@ -187,7 +187,7 @@ static void dpu_hw_setup_vsync_source_and_vsync_sel(struct dpu_hw_mdp *mdp,
>  	}
>  	DPU_REG_WRITE(c, MDP_VSYNC_SEL, reg);
>  
> -	dpu_hw_setup_vsync_source(mdp, cfg);
> +	dpu_hw_setup_wd_timer(mdp, cfg);
>  }
>  
>  static void dpu_hw_get_safe_status(struct dpu_hw_mdp *mdp,
> @@ -239,9 +239,9 @@ static void _setup_mdp_ops(struct dpu_hw_mdp_ops *ops,
>  	ops->get_danger_status = dpu_hw_get_danger_status;
>  
>  	if (cap & BIT(DPU_MDP_VSYNC_SEL))
> -		ops->setup_vsync_source = dpu_hw_setup_vsync_source_and_vsync_sel;
> +		ops->setup_vsync_source = dpu_hw_setup_vsync_sel;
>  	else
> -		ops->setup_vsync_source = dpu_hw_setup_vsync_source;
> +		ops->setup_vsync_source = dpu_hw_setup_wd_timer;

Should the callback also be renamed - and the docs improved?  Seems the
vsync_sel register is used to selsect a vsync_source (plus some other bits like
the pingpong block).

- Marijn

>  
>  	ops->get_safe_status = dpu_hw_get_safe_status;
>  
> 
> -- 
> 2.39.2
> 

Re: [PATCH v2 2/8] drm/msm/dpu: convert vsync source defines to the enum

[Dmitry Baryshkov]

On Thu, 13 Jun 2024 at 21:17, Marijn Suijten
<marijn.suijten@somainline.org> wrote:
>
> On 2024-06-13 20:05:05, Dmitry Baryshkov wrote:
> > Add enum dpu_vsync_source instead of a series of defines. Use this enum
> > to pass vsync information.
> >
> > Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c |  2 +-
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c |  2 +-
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h |  2 +-
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h | 26 ++++++++++++++------------
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h  |  2 +-
> >  5 files changed, 18 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 119f3ea50a7c..4988a1029431 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > @@ -747,7 +747,7 @@ static void _dpu_encoder_update_vsync_source(struct dpu_encoder_virt *dpu_enc,
> >               if (disp_info->is_te_using_watchdog_timer)
> >                       vsync_cfg.vsync_source = DPU_VSYNC_SOURCE_WD_TIMER_0;
> >               else
> > -                     vsync_cfg.vsync_source = DPU_VSYNC0_SOURCE_GPIO;
> > +                     vsync_cfg.vsync_source = DPU_VSYNC_SOURCE_GPIO_0;
> >
> >               hw_mdptop->ops.setup_vsync_source(hw_mdptop, &vsync_cfg);
> >
> > 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 225c1c7768ff..96f6160cf607 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> > @@ -462,7 +462,7 @@ static int dpu_hw_intf_get_vsync_info(struct dpu_hw_intf *intf,
> >  }
> >
> >  static void dpu_hw_intf_vsync_sel(struct dpu_hw_intf *intf,
> > -             u32 vsync_source)
> > +                               enum dpu_vsync_source vsync_source)
> >  {
> >       struct dpu_hw_blk_reg_map *c;
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> > index f9015c67a574..ac244f0b33fb 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> > @@ -107,7 +107,7 @@ struct dpu_hw_intf_ops {
> >
> >       int (*connect_external_te)(struct dpu_hw_intf *intf, bool enable_external_te);
> >
> > -     void (*vsync_sel)(struct dpu_hw_intf *intf, u32 vsync_source);
> > +     void (*vsync_sel)(struct dpu_hw_intf *intf, enum dpu_vsync_source vsync_source);
> >
> >       /**
> >        * Disable autorefresh if enabled
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
> > index 66759623fc42..a2eff36a2224 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
> > @@ -54,18 +54,20 @@
> >  #define DPU_BLEND_BG_INV_MOD_ALPHA   (1 << 12)
> >  #define DPU_BLEND_BG_TRANSP_EN               (1 << 13)
> >
> > -#define DPU_VSYNC0_SOURCE_GPIO               0
> > -#define DPU_VSYNC1_SOURCE_GPIO               1
> > -#define DPU_VSYNC2_SOURCE_GPIO               2
> > -#define DPU_VSYNC_SOURCE_INTF_0              3
> > -#define DPU_VSYNC_SOURCE_INTF_1              4
> > -#define DPU_VSYNC_SOURCE_INTF_2              5
> > -#define DPU_VSYNC_SOURCE_INTF_3              6
> > -#define DPU_VSYNC_SOURCE_WD_TIMER_4  11
> > -#define DPU_VSYNC_SOURCE_WD_TIMER_3  12
> > -#define DPU_VSYNC_SOURCE_WD_TIMER_2  13
> > -#define DPU_VSYNC_SOURCE_WD_TIMER_1  14
> > -#define DPU_VSYNC_SOURCE_WD_TIMER_0  15
> > +enum dpu_vsync_source {
> > +     DPU_VSYNC_SOURCE_GPIO_0,
> > +     DPU_VSYNC_SOURCE_GPIO_1,
> > +     DPU_VSYNC_SOURCE_GPIO_2,
>
> While at it, rename this to _P / _S / _E to match the other patches/code?

I thought about it, but Abhinav pointed out that DPU docs use this
kind of naming.

>
> - Marijn
>
> > +     DPU_VSYNC_SOURCE_INTF_0 = 3,
> > +     DPU_VSYNC_SOURCE_INTF_1,
> > +     DPU_VSYNC_SOURCE_INTF_2,
> > +     DPU_VSYNC_SOURCE_INTF_3,
> > +     DPU_VSYNC_SOURCE_WD_TIMER_4 = 11,
> > +     DPU_VSYNC_SOURCE_WD_TIMER_3,
> > +     DPU_VSYNC_SOURCE_WD_TIMER_2,
> > +     DPU_VSYNC_SOURCE_WD_TIMER_1,
> > +     DPU_VSYNC_SOURCE_WD_TIMER_0,
> > +};
> >
> >  enum dpu_hw_blk_type {
> >       DPU_HW_BLK_TOP = 0,
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h
> > index 6f3dc98087df..5c9a7ede991e 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h
> > @@ -64,7 +64,7 @@ struct dpu_vsync_source_cfg {
> >       u32 pp_count;
> >       u32 frame_rate;
> >       u32 ppnumber[PINGPONG_MAX];
> > -     u32 vsync_source;
> > +     enum dpu_vsync_source vsync_source;
> >  };
> >
> >  /**
> >
> > --
> > 2.39.2
> >



-- 
With best wishes
Dmitry

Re: [PATCH v2 1/8] dt-bindings: display/msm/dsi: allow specifying TE source

[Abhinav Kumar]

On 6/13/2024 10:05 AM, Dmitry Baryshkov wrote:
> Command mode panels provide TE signal back to the DSI host to signal
> that the frame display has completed and update of the image will not
> cause tearing. Usually it is connected to the first GPIO with the
> mdp_vsync function, which is the default. In such case the property can
> be skipped.
> 
> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   .../bindings/display/msm/dsi-controller-main.yaml       | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
> 

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

Re: [PATCH v2 7/8] drm/msm/dpu: support setting the TE source

[Abhinav Kumar]

On 6/13/2024 10:05 AM, Dmitry Baryshkov wrote:
> Make the DPU driver use the TE source specified in the DT. If none is
> specified, the driver defaults to the first GPIO (mdp_vsync0).
> 

as marijn noted,

mdp_vsync0 ---> mdp_vsyncp

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

With that addressed,

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

Re: [PATCH v2 7/8] drm/msm/dpu: support setting the TE source

[Abhinav Kumar]

On 6/13/2024 11:14 AM, Marijn Suijten wrote:
> On 2024-06-13 20:05:10, Dmitry Baryshkov wrote:
>> Make the DPU driver use the TE source specified in the DT. If none is
>> specified, the driver defaults to the first GPIO (mdp_vsync0).
> 
> mdp_vsync_p?
> 
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 44 ++++++++++++++++++++++++++++++++-
>>   1 file changed, 43 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> index e9991f3756d4..6fcb3cf4a382 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> @@ -505,6 +505,44 @@ static void dpu_kms_wait_flush(struct msm_kms *kms, unsigned crtc_mask)
>>   		dpu_kms_wait_for_commit_done(kms, crtc);
>>   }
>>   
>> +static const char *dpu_vsync_sources[] = {
>> +	[DPU_VSYNC_SOURCE_GPIO_0] = "mdp_vsync_p",
>> +	[DPU_VSYNC_SOURCE_GPIO_1] = "mdp_vsync_s",
>> +	[DPU_VSYNC_SOURCE_GPIO_2] = "mdp_vsync_e",
>> +	[DPU_VSYNC_SOURCE_INTF_0] = "mdp_intf0",
>> +	[DPU_VSYNC_SOURCE_INTF_1] = "mdp_intf1",
>> +	[DPU_VSYNC_SOURCE_INTF_2] = "mdp_intf2",
>> +	[DPU_VSYNC_SOURCE_INTF_3] = "mdp_intf3",
>> +	[DPU_VSYNC_SOURCE_WD_TIMER_0] = "timer0",
>> +	[DPU_VSYNC_SOURCE_WD_TIMER_1] = "timer1",
>> +	[DPU_VSYNC_SOURCE_WD_TIMER_2] = "timer2",
>> +	[DPU_VSYNC_SOURCE_WD_TIMER_3] = "timer3",
>> +	[DPU_VSYNC_SOURCE_WD_TIMER_4] = "timer4",
>> +};
>> +
>> +static int dpu_kms_dsi_set_te_source(struct msm_display_info *info,
>> +				     struct msm_dsi *dsi)
>> +{
>> +	const char *te_source = msm_dsi_get_te_source(dsi);
> 
> Just checking: if the TE source is different and one has dual-DSI, it must be
> set on both controllers?
> 

If we use dual-dsi (in NON-bonded mode), then yes we will have two TE 
sources - one for each controller.

>> +	int i;
>> +
>> +	if (!te_source) {
>> +		info->vsync_source = DPU_VSYNC_SOURCE_GPIO_0;
>> +		return 0;
>> +	}
>> +
>> +	/* we can not use match_string since dpu_vsync_sources is a sparse array */
> 
> Instead of having gaps in the array, you could also store both the vsync_source
> and name as the array elements?
> 

Yes, there is a gap because the DPU_VSYNC_* macros have a gap.

Can you pls explain your suggestion to remove the gap a little more?
I didnt follow this part very well.

>> +	for (i = 0; i < ARRAY_SIZE(dpu_vsync_sources); i++) {
>> +		if (dpu_vsync_sources[i] &&
>> +		    !strcmp(dpu_vsync_sources[i], te_source)) {
>> +			info->vsync_source = i;
>> +			return 0;
>> +		}
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>>   static int _dpu_kms_initialize_dsi(struct drm_device *dev,
>>   				    struct msm_drm_private *priv,
>>   				    struct dpu_kms *dpu_kms)
>> @@ -543,7 +581,11 @@ static int _dpu_kms_initialize_dsi(struct drm_device *dev,
>>   
>>   		info.is_cmd_mode = msm_dsi_is_cmd_mode(priv->dsi[i]);
>>   
>> -		info.vsync_source = DPU_VSYNC_SOURCE_GPIO_0;
>> +		rc = dpu_kms_dsi_set_te_source(&info, priv->dsi[i]);
>> +		if (rc) {
>> +			DPU_ERROR("failed to identify TE source for dsi display\n");
>> +			return rc;
>> +		}
>>   
>>   		encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_DSI, &info);
>>   		if (IS_ERR(encoder)) {
>>
>> -- 
>> 2.39.2
>>

Re: [PATCH v2 8/8] drm/msm/dpu: rename dpu_hw_setup_vsync_source functions

[Abhinav Kumar]

On 6/13/2024 11:29 AM, Marijn Suijten wrote:
> On 2024-06-13 20:05:11, Dmitry Baryshkov wrote:
>> Rename dpu_hw_setup_vsync_source functions to make the names match the
>> implementation: on DPU 5.x the TOP only contains timer setup, while 3.x
>> and 4.x used MDP_VSYNC_SEL register to select TE source.
> 
> Yeah that was never really clear anymore after I split this function in two in
> commit a2ff096803b3 ("drm/msm/dpu: Disable MDP vsync source selection on DPU
> 5.0.0 and above").
> 
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c | 14 +++++++-------
>>   1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c
>> index 05e48cf4ec1d..6e2ac50b94a4 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c
>> @@ -107,8 +107,8 @@ static void dpu_hw_get_danger_status(struct dpu_hw_mdp *mdp,
>>   	status->sspp[SSPP_CURSOR1] = (value >> 26) & 0x3;
>>   }
>>   
>> -static void dpu_hw_setup_vsync_source(struct dpu_hw_mdp *mdp,
>> -		struct dpu_vsync_source_cfg *cfg)
>> +static void dpu_hw_setup_wd_timer(struct dpu_hw_mdp *mdp,
>> +				  struct dpu_vsync_source_cfg *cfg)
>>   {
>>   	struct dpu_hw_blk_reg_map *c;
>>   	u32 reg, wd_load_value, wd_ctl, wd_ctl2;
>> @@ -163,8 +163,8 @@ static void dpu_hw_setup_vsync_source(struct dpu_hw_mdp *mdp,
>>   	}
>>   }
>>   
>> -static void dpu_hw_setup_vsync_source_and_vsync_sel(struct dpu_hw_mdp *mdp,
>> -		struct dpu_vsync_source_cfg *cfg)
>> +static void dpu_hw_setup_vsync_sel(struct dpu_hw_mdp *mdp,
> 
> Maybe keep setup_wd_timer_and_vsync_sel()?  OTOH it doesn't always set wd_timer,
> only when vsync_source calls for it.
> 

Yeah, I think this name is fine.

>> +				   struct dpu_vsync_source_cfg *cfg)
>>   {
>>   	struct dpu_hw_blk_reg_map *c;
>>   	u32 reg, i;
>> @@ -187,7 +187,7 @@ static void dpu_hw_setup_vsync_source_and_vsync_sel(struct dpu_hw_mdp *mdp,
>>   	}
>>   	DPU_REG_WRITE(c, MDP_VSYNC_SEL, reg);
>>   
>> -	dpu_hw_setup_vsync_source(mdp, cfg);
>> +	dpu_hw_setup_wd_timer(mdp, cfg);
>>   }
>>   
>>   static void dpu_hw_get_safe_status(struct dpu_hw_mdp *mdp,
>> @@ -239,9 +239,9 @@ static void _setup_mdp_ops(struct dpu_hw_mdp_ops *ops,
>>   	ops->get_danger_status = dpu_hw_get_danger_status;
>>   
>>   	if (cap & BIT(DPU_MDP_VSYNC_SEL))
>> -		ops->setup_vsync_source = dpu_hw_setup_vsync_source_and_vsync_sel;
>> +		ops->setup_vsync_source = dpu_hw_setup_vsync_sel;
>>   	else
>> -		ops->setup_vsync_source = dpu_hw_setup_vsync_source;
>> +		ops->setup_vsync_source = dpu_hw_setup_wd_timer;
> 
> Should the callback also be renamed - and the docs improved?  Seems the
> vsync_sel register is used to selsect a vsync_source (plus some other bits like
> the pingpong block).
> 
> - Marijn
> 

Its a good thought but then we will also have to change the callers of 
setup_vsync_source to check we have ops->setup_vsync_source || 
ops->setup_watchdog_timer in dpu_encoder.c

This way, the caller does not know because whether to use TE or watchdog 
as the setting the source will happen under the hood.

So I think this is okay actually, hence

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


>>   
>>   	ops->get_safe_status = dpu_hw_get_safe_status;
>>   
>>
>> -- 
>> 2.39.2
>>

Re: [PATCH v2 5/8] drm/msm/dpu: rework vsync_source handling

[Dmitry Baryshkov]

On Thu, Jun 13, 2024 at 08:21:59PM GMT, Marijn Suijten wrote:
> Maybe retitle this to something that more closely resembles "remove unset
> is_te_using_watchdog_timer field"?

Well, it really moves vsync_source selection to
_dpu_kms_initialize_dsi(), it doesn't just drop the
is_te_using_watchdog_timer.

> 
> On 2024-06-13 20:05:08, Dmitry Baryshkov wrote:
> > The struct msm_display_info has is_te_using_watchdog_timer field which
> > is neither used anywhere nor is flexible enough to specify different
> 
> Well, it's "used", but not "set" (to anything other than the zero-initialized
> default). s/used/set?

ack

> 
> > sources. Replace it with the field specifying the vsync source using
> > enum dpu_vsync_source.
> > 
> > Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
> Patch itself is fine, just think the title could be clearer:
> 
> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> 
> > ---
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 5 +----
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 5 ++---
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     | 2 ++
> >  3 files changed, 5 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > index bd37a56b4d03..b147f8814a18 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > @@ -743,10 +743,7 @@ static void _dpu_encoder_update_vsync_source(struct dpu_encoder_virt *dpu_enc,
> >  		vsync_cfg.pp_count = dpu_enc->num_phys_encs;
> >  		vsync_cfg.frame_rate = drm_mode_vrefresh(&dpu_enc->base.crtc->state->adjusted_mode);
> >  
> > -		if (disp_info->is_te_using_watchdog_timer)
> > -			vsync_cfg.vsync_source = DPU_VSYNC_SOURCE_WD_TIMER_0;
> > -		else
> > -			vsync_cfg.vsync_source = DPU_VSYNC_SOURCE_GPIO_0;
> > +		vsync_cfg.vsync_source = disp_info->vsync_source;
> >  
> >  		hw_mdptop->ops.setup_vsync_source(hw_mdptop, &vsync_cfg);
> >  
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > index 76be77e30954..cb59bd4436f4 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > @@ -26,15 +26,14 @@
> >   * @h_tile_instance:    Controller instance used per tile. Number of elements is
> >   *                      based on num_of_h_tiles
> >   * @is_cmd_mode		Boolean to indicate if the CMD mode is requested
> > - * @is_te_using_watchdog_timer:  Boolean to indicate watchdog TE is
> > - *				 used instead of panel TE in cmd mode panels
> > + * @vsync_source:	Source of the TE signal for DSI CMD devices
> >   */
> >  struct msm_display_info {
> >  	enum dpu_intf_type intf_type;
> >  	uint32_t num_of_h_tiles;
> >  	uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY];
> >  	bool is_cmd_mode;
> > -	bool is_te_using_watchdog_timer;
> > +	enum dpu_vsync_source vsync_source;
> >  };
> >  
> >  /**
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > index 1955848b1b78..e9991f3756d4 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > @@ -543,6 +543,8 @@ static int _dpu_kms_initialize_dsi(struct drm_device *dev,
> >  
> >  		info.is_cmd_mode = msm_dsi_is_cmd_mode(priv->dsi[i]);
> >  
> > +		info.vsync_source = DPU_VSYNC_SOURCE_GPIO_0;
> > +
> >  		encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_DSI, &info);
> >  		if (IS_ERR(encoder)) {
> >  			DPU_ERROR("encoder init failed for dsi display\n");
> > 
> > -- 
> > 2.39.2
> > 

-- 
With best wishes
Dmitry

Re: [PATCH v2 0/8] drm/msm/dpu: handle non-default TE source pins

[Dmitry Baryshkov]

On Thu, 13 Jun 2024 20:05:03 +0300, Dmitry Baryshkov wrote:
> Command-mode DSI panels need to signal the display controlller when
> vsync happens, so that the device can start sending the next frame. Some
> devices (Google Pixel 3) use a non-default pin, so additional
> configuration is required. Add a way to specify this information in DT
> and handle it in the DSI and DPU drivers.
> 
> 
> [...]

Applied, thanks!

[1/8] dt-bindings: display/msm/dsi: allow specifying TE source
      https://gitlab.freedesktop.org/lumag/msm/-/commit/e0bc725bdd0f
[2/8] drm/msm/dpu: convert vsync source defines to the enum
      https://gitlab.freedesktop.org/lumag/msm/-/commit/c09b19b79d27
[3/8] drm/msm/dsi: drop unused GPIOs handling
      https://gitlab.freedesktop.org/lumag/msm/-/commit/149d195638c9
[4/8] drm/msm/dpu: pull the is_cmd_mode out of _dpu_encoder_update_vsync_source()
      https://gitlab.freedesktop.org/lumag/msm/-/commit/1ed505b60480
[5/8] drm/msm/dpu: rework vsync_source handling
      https://gitlab.freedesktop.org/lumag/msm/-/commit/cd1592c3e31d
[6/8] drm/msm/dsi: parse vsync source from device tree
      https://gitlab.freedesktop.org/lumag/msm/-/commit/4404dd757c5d
[7/8] drm/msm/dpu: support setting the TE source
      https://gitlab.freedesktop.org/lumag/msm/-/commit/ecfc21292865
[8/8] drm/msm/dpu: rename dpu_hw_setup_vsync_source functions
      https://gitlab.freedesktop.org/lumag/msm/-/commit/b8caa9e8668b

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