[PATCH 0/3] drm/msm/dp: More cleanups for force link train

[PATCH 0/3] drm/msm/dp: More cleanups for force link train

[Stephen Boyd]

These patches do a little cleanup on the v9 patch from Kuogee.

Stephen Boyd (3):
  drm/msm/dp: Reorganize code to avoid forward declaration
  drm/msm/dp: Remove pixel_rate from struct dp_ctrl
  drm/msm/dp: Get rid of dp_ctrl_on_stream_phy_test_report()

 drivers/gpu/drm/msm/dp/dp_ctrl.c | 145 ++++++++++++++-----------------
 drivers/gpu/drm/msm/dp/dp_ctrl.h |   1 -
 2 files changed, 65 insertions(+), 81 deletions(-)

[1] https://lore.kernel.org/r/1655411200-7255-1-git-send-email-quic_khsieh@quicinc.com

base-commit: 9639746eebb13068dc9de6f436151bd88da2b827
prerequisite-patch-id: 2fc33a2830ec84d922023fddb585728c48a59525
-- 
https://chromeos.dev

[PATCH 0/3] drm/msm/dp: More cleanups for force link train

[Stephen Boyd]

These patches do a little cleanup on the v9 patch from Kuogee.

Stephen Boyd (3):
  drm/msm/dp: Reorganize code to avoid forward declaration
  drm/msm/dp: Remove pixel_rate from struct dp_ctrl
  drm/msm/dp: Get rid of dp_ctrl_on_stream_phy_test_report()

 drivers/gpu/drm/msm/dp/dp_ctrl.c | 145 ++++++++++++++-----------------
 drivers/gpu/drm/msm/dp/dp_ctrl.h |   1 -
 2 files changed, 65 insertions(+), 81 deletions(-)

[1] https://lore.kernel.org/r/1655411200-7255-1-git-send-email-quic_khsieh@quicinc.com

base-commit: 9639746eebb13068dc9de6f436151bd88da2b827
prerequisite-patch-id: 2fc33a2830ec84d922023fddb585728c48a59525
-- 
https://chromeos.dev

[PATCH 1/3] drm/msm/dp: Reorganize code to avoid forward declaration

[Stephen Boyd]

Let's move these functions around to avoid having to forward declare
dp_ctrl_on_stream_phy_test_report(). Also remove
dp_ctrl_reinitialize_mainlink() forward declaration because we're doing
that sort of task.

Cc: Kuogee Hsieh <quic_khsieh@quicinc.com>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/gpu/drm/msm/dp/dp_ctrl.c | 104 +++++++++++++++----------------
 1 file changed, 50 insertions(+), 54 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index 703249384e7c..bd445e683cfc 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1238,8 +1238,6 @@ static int dp_ctrl_link_train_2(struct dp_ctrl_private *ctrl,
 	return -ETIMEDOUT;
 }
 
-static int dp_ctrl_reinitialize_mainlink(struct dp_ctrl_private *ctrl);
-
 static int dp_ctrl_link_train(struct dp_ctrl_private *ctrl,
 			int *training_step)
 {
@@ -1534,38 +1532,6 @@ static int dp_ctrl_link_maintenance(struct dp_ctrl_private *ctrl)
 	return ret;
 }
 
-static int dp_ctrl_on_stream_phy_test_report(struct dp_ctrl *dp_ctrl);
-
-static int dp_ctrl_process_phy_test_request(struct dp_ctrl_private *ctrl)
-{
-	int ret = 0;
-
-	if (!ctrl->link->phy_params.phy_test_pattern_sel) {
-		drm_dbg_dp(ctrl->drm_dev,
-			"no test pattern selected by sink\n");
-		return ret;
-	}
-
-	/*
-	 * The global reset will need DP link related clocks to be
-	 * running. Add the global reset just before disabling the
-	 * link clocks and core clocks.
-	 */
-	ret = dp_ctrl_off(&ctrl->dp_ctrl);
-	if (ret) {
-		DRM_ERROR("failed to disable DP controller\n");
-		return ret;
-	}
-
-	ret = dp_ctrl_on_link(&ctrl->dp_ctrl);
-	if (!ret)
-		ret = dp_ctrl_on_stream_phy_test_report(&ctrl->dp_ctrl);
-	else
-		DRM_ERROR("failed to enable DP link controller\n");
-
-	return ret;
-}
-
 static bool dp_ctrl_send_phy_test_pattern(struct dp_ctrl_private *ctrl)
 {
 	bool success = false;
@@ -1618,6 +1584,56 @@ static bool dp_ctrl_send_phy_test_pattern(struct dp_ctrl_private *ctrl)
 	return success;
 }
 
+static int dp_ctrl_on_stream_phy_test_report(struct dp_ctrl *dp_ctrl)
+{
+	int ret;
+	struct dp_ctrl_private *ctrl;
+
+	ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
+
+	ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
+
+	ret = dp_ctrl_enable_stream_clocks(ctrl);
+	if (ret) {
+		DRM_ERROR("Failed to start pixel clocks. ret=%d\n", ret);
+		return ret;
+	}
+
+	dp_ctrl_send_phy_test_pattern(ctrl);
+
+	return 0;
+}
+
+static int dp_ctrl_process_phy_test_request(struct dp_ctrl_private *ctrl)
+{
+	int ret = 0;
+
+	if (!ctrl->link->phy_params.phy_test_pattern_sel) {
+		drm_dbg_dp(ctrl->drm_dev,
+			"no test pattern selected by sink\n");
+		return ret;
+	}
+
+	/*
+	 * The global reset will need DP link related clocks to be
+	 * running. Add the global reset just before disabling the
+	 * link clocks and core clocks.
+	 */
+	ret = dp_ctrl_off(&ctrl->dp_ctrl);
+	if (ret) {
+		DRM_ERROR("failed to disable DP controller\n");
+		return ret;
+	}
+
+	ret = dp_ctrl_on_link(&ctrl->dp_ctrl);
+	if (!ret)
+		ret = dp_ctrl_on_stream_phy_test_report(&ctrl->dp_ctrl);
+	else
+		DRM_ERROR("failed to enable DP link controller\n");
+
+	return ret;
+}
+
 void dp_ctrl_handle_sink_request(struct dp_ctrl *dp_ctrl)
 {
 	struct dp_ctrl_private *ctrl;
@@ -1815,26 +1831,6 @@ static int dp_ctrl_link_retrain(struct dp_ctrl_private *ctrl)
 	return dp_ctrl_setup_main_link(ctrl, &training_step);
 }
 
-static int dp_ctrl_on_stream_phy_test_report(struct dp_ctrl *dp_ctrl)
-{
-	int ret;
-	struct dp_ctrl_private *ctrl;
-
-	ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
-
-	ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
-
-	ret = dp_ctrl_enable_stream_clocks(ctrl);
-	if (ret) {
-		DRM_ERROR("Failed to start pixel clocks. ret=%d\n", ret);
-		return ret;
-	}
-
-	dp_ctrl_send_phy_test_pattern(ctrl);
-
-	return 0;
-}
-
 int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl, bool force_link_train)
 {
 	int ret = 0;
-- 
https://chromeos.dev

[PATCH 1/3] drm/msm/dp: Reorganize code to avoid forward declaration

[Stephen Boyd]

Let's move these functions around to avoid having to forward declare
dp_ctrl_on_stream_phy_test_report(). Also remove
dp_ctrl_reinitialize_mainlink() forward declaration because we're doing
that sort of task.

Cc: Kuogee Hsieh <quic_khsieh@quicinc.com>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/gpu/drm/msm/dp/dp_ctrl.c | 104 +++++++++++++++----------------
 1 file changed, 50 insertions(+), 54 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index 703249384e7c..bd445e683cfc 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1238,8 +1238,6 @@ static int dp_ctrl_link_train_2(struct dp_ctrl_private *ctrl,
 	return -ETIMEDOUT;
 }
 
-static int dp_ctrl_reinitialize_mainlink(struct dp_ctrl_private *ctrl);
-
 static int dp_ctrl_link_train(struct dp_ctrl_private *ctrl,
 			int *training_step)
 {
@@ -1534,38 +1532,6 @@ static int dp_ctrl_link_maintenance(struct dp_ctrl_private *ctrl)
 	return ret;
 }
 
-static int dp_ctrl_on_stream_phy_test_report(struct dp_ctrl *dp_ctrl);
-
-static int dp_ctrl_process_phy_test_request(struct dp_ctrl_private *ctrl)
-{
-	int ret = 0;
-
-	if (!ctrl->link->phy_params.phy_test_pattern_sel) {
-		drm_dbg_dp(ctrl->drm_dev,
-			"no test pattern selected by sink\n");
-		return ret;
-	}
-
-	/*
-	 * The global reset will need DP link related clocks to be
-	 * running. Add the global reset just before disabling the
-	 * link clocks and core clocks.
-	 */
-	ret = dp_ctrl_off(&ctrl->dp_ctrl);
-	if (ret) {
-		DRM_ERROR("failed to disable DP controller\n");
-		return ret;
-	}
-
-	ret = dp_ctrl_on_link(&ctrl->dp_ctrl);
-	if (!ret)
-		ret = dp_ctrl_on_stream_phy_test_report(&ctrl->dp_ctrl);
-	else
-		DRM_ERROR("failed to enable DP link controller\n");
-
-	return ret;
-}
-
 static bool dp_ctrl_send_phy_test_pattern(struct dp_ctrl_private *ctrl)
 {
 	bool success = false;
@@ -1618,6 +1584,56 @@ static bool dp_ctrl_send_phy_test_pattern(struct dp_ctrl_private *ctrl)
 	return success;
 }
 
+static int dp_ctrl_on_stream_phy_test_report(struct dp_ctrl *dp_ctrl)
+{
+	int ret;
+	struct dp_ctrl_private *ctrl;
+
+	ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
+
+	ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
+
+	ret = dp_ctrl_enable_stream_clocks(ctrl);
+	if (ret) {
+		DRM_ERROR("Failed to start pixel clocks. ret=%d\n", ret);
+		return ret;
+	}
+
+	dp_ctrl_send_phy_test_pattern(ctrl);
+
+	return 0;
+}
+
+static int dp_ctrl_process_phy_test_request(struct dp_ctrl_private *ctrl)
+{
+	int ret = 0;
+
+	if (!ctrl->link->phy_params.phy_test_pattern_sel) {
+		drm_dbg_dp(ctrl->drm_dev,
+			"no test pattern selected by sink\n");
+		return ret;
+	}
+
+	/*
+	 * The global reset will need DP link related clocks to be
+	 * running. Add the global reset just before disabling the
+	 * link clocks and core clocks.
+	 */
+	ret = dp_ctrl_off(&ctrl->dp_ctrl);
+	if (ret) {
+		DRM_ERROR("failed to disable DP controller\n");
+		return ret;
+	}
+
+	ret = dp_ctrl_on_link(&ctrl->dp_ctrl);
+	if (!ret)
+		ret = dp_ctrl_on_stream_phy_test_report(&ctrl->dp_ctrl);
+	else
+		DRM_ERROR("failed to enable DP link controller\n");
+
+	return ret;
+}
+
 void dp_ctrl_handle_sink_request(struct dp_ctrl *dp_ctrl)
 {
 	struct dp_ctrl_private *ctrl;
@@ -1815,26 +1831,6 @@ static int dp_ctrl_link_retrain(struct dp_ctrl_private *ctrl)
 	return dp_ctrl_setup_main_link(ctrl, &training_step);
 }
 
-static int dp_ctrl_on_stream_phy_test_report(struct dp_ctrl *dp_ctrl)
-{
-	int ret;
-	struct dp_ctrl_private *ctrl;
-
-	ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
-
-	ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
-
-	ret = dp_ctrl_enable_stream_clocks(ctrl);
-	if (ret) {
-		DRM_ERROR("Failed to start pixel clocks. ret=%d\n", ret);
-		return ret;
-	}
-
-	dp_ctrl_send_phy_test_pattern(ctrl);
-
-	return 0;
-}
-
 int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl, bool force_link_train)
 {
 	int ret = 0;
-- 
https://chromeos.dev

[PATCH 2/3] drm/msm/dp: Remove pixel_rate from struct dp_ctrl

[Stephen Boyd]

This struct member is stored to in the function that calls the function
which uses it. That's possible with a function argument instead of
storing to a struct member. Pass the pixel_rate as an argument instead
to simplify the code. Note that dp_ctrl_link_maintenance() was storing
the pixel_rate but never using it so we just remove the assignment from
there.

Cc: Kuogee Hsieh <quic_khsieh@quicinc.com>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/gpu/drm/msm/dp/dp_ctrl.c | 57 ++++++++++++++++----------------
 drivers/gpu/drm/msm/dp/dp_ctrl.h |  1 -
 2 files changed, 28 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index bd445e683cfc..e114521af2e9 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1336,7 +1336,7 @@ static void dp_ctrl_set_clock_rate(struct dp_ctrl_private *ctrl,
 				name, rate);
 }
 
-static int dp_ctrl_enable_mainlink_clocks(struct dp_ctrl_private *ctrl)
+static int dp_ctrl_enable_mainlink_clocks(struct dp_ctrl_private *ctrl, unsigned long pixel_rate)
 {
 	int ret = 0;
 	struct dp_io *dp_io = &ctrl->parser->io;
@@ -1357,25 +1357,25 @@ static int dp_ctrl_enable_mainlink_clocks(struct dp_ctrl_private *ctrl)
 	if (ret)
 		DRM_ERROR("Unable to start link clocks. ret=%d\n", ret);
 
-	drm_dbg_dp(ctrl->drm_dev, "link rate=%d pixel_clk=%d\n",
-		ctrl->link->link_params.rate, ctrl->dp_ctrl.pixel_rate);
+	drm_dbg_dp(ctrl->drm_dev, "link rate=%d pixel_clk=%lu\n",
+		ctrl->link->link_params.rate, pixel_rate);
 
 	return ret;
 }
 
-static int dp_ctrl_enable_stream_clocks(struct dp_ctrl_private *ctrl)
+static int dp_ctrl_enable_stream_clocks(struct dp_ctrl_private *ctrl,
+					unsigned long pixel_rate)
 {
-	int ret = 0;
+	int ret;
 
-	dp_ctrl_set_clock_rate(ctrl, DP_STREAM_PM, "stream_pixel",
-					ctrl->dp_ctrl.pixel_rate * 1000);
+	dp_ctrl_set_clock_rate(ctrl, DP_STREAM_PM, "stream_pixel", pixel_rate * 1000);
 
 	ret = dp_power_clk_enable(ctrl->power, DP_STREAM_PM, true);
 	if (ret)
 		DRM_ERROR("Unabled to start pixel clocks. ret=%d\n", ret);
 
-	drm_dbg_dp(ctrl->drm_dev, "link rate=%d pixel_clk=%d\n",
-			ctrl->link->link_params.rate, ctrl->dp_ctrl.pixel_rate);
+	drm_dbg_dp(ctrl->drm_dev, "link rate=%d pixel_clk=%lu\n",
+			ctrl->link->link_params.rate, pixel_rate);
 
 	return ret;
 }
@@ -1445,7 +1445,7 @@ static bool dp_ctrl_use_fixed_nvid(struct dp_ctrl_private *ctrl)
 	return false;
 }
 
-static int dp_ctrl_reinitialize_mainlink(struct dp_ctrl_private *ctrl)
+static int dp_ctrl_reinitialize_mainlink(struct dp_ctrl_private *ctrl, unsigned long pixel_rate)
 {
 	int ret = 0;
 	struct dp_io *dp_io = &ctrl->parser->io;
@@ -1469,7 +1469,7 @@ static int dp_ctrl_reinitialize_mainlink(struct dp_ctrl_private *ctrl)
 	/* hw recommended delay before re-enabling clocks */
 	msleep(20);
 
-	ret = dp_ctrl_enable_mainlink_clocks(ctrl);
+	ret = dp_ctrl_enable_mainlink_clocks(ctrl, pixel_rate);
 	if (ret) {
 		DRM_ERROR("Failed to enable mainlink clks. ret=%d\n", ret);
 		return ret;
@@ -1517,8 +1517,6 @@ static int dp_ctrl_link_maintenance(struct dp_ctrl_private *ctrl)
 	ctrl->link->phy_params.p_level = 0;
 	ctrl->link->phy_params.v_level = 0;
 
-	ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
-
 	ret = dp_ctrl_setup_main_link(ctrl, &training_step);
 	if (ret)
 		goto end;
@@ -1588,12 +1586,12 @@ static int dp_ctrl_on_stream_phy_test_report(struct dp_ctrl *dp_ctrl)
 {
 	int ret;
 	struct dp_ctrl_private *ctrl;
+	unsigned long pixel_rate;
 
 	ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
 
-	ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
-
-	ret = dp_ctrl_enable_stream_clocks(ctrl);
+	pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
+	ret = dp_ctrl_enable_stream_clocks(ctrl, pixel_rate);
 	if (ret) {
 		DRM_ERROR("Failed to start pixel clocks. ret=%d\n", ret);
 		return ret;
@@ -1709,6 +1707,7 @@ int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
 	u32 const phy_cts_pixel_clk_khz = 148500;
 	u8 link_status[DP_LINK_STATUS_SIZE];
 	unsigned int training_step;
+	unsigned long pixel_rate;
 
 	if (!dp_ctrl)
 		return -EINVAL;
@@ -1723,25 +1722,25 @@ int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
 		drm_dbg_dp(ctrl->drm_dev,
 				"using phy test link parameters\n");
 		if (!ctrl->panel->dp_mode.drm_mode.clock)
-			ctrl->dp_ctrl.pixel_rate = phy_cts_pixel_clk_khz;
+			pixel_rate = phy_cts_pixel_clk_khz;
 	} else {
 		ctrl->link->link_params.rate = rate;
 		ctrl->link->link_params.num_lanes =
 			ctrl->panel->link_info.num_lanes;
-		ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
+		pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
 	}
 
-	drm_dbg_dp(ctrl->drm_dev, "rate=%d, num_lanes=%d, pixel_rate=%d\n",
+	drm_dbg_dp(ctrl->drm_dev, "rate=%d, num_lanes=%d, pixel_rate=%lu\n",
 		ctrl->link->link_params.rate, ctrl->link->link_params.num_lanes,
-		ctrl->dp_ctrl.pixel_rate);
+		pixel_rate);
 
 
-	rc = dp_ctrl_enable_mainlink_clocks(ctrl);
+	rc = dp_ctrl_enable_mainlink_clocks(ctrl, pixel_rate);
 	if (rc)
 		return rc;
 
 	while (--link_train_max_retries) {
-		rc = dp_ctrl_reinitialize_mainlink(ctrl);
+		rc = dp_ctrl_reinitialize_mainlink(ctrl, pixel_rate);
 		if (rc) {
 			DRM_ERROR("Failed to reinitialize mainlink. rc=%d\n",
 					rc);
@@ -1836,6 +1835,7 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl, bool force_link_train)
 	int ret = 0;
 	bool mainlink_ready = false;
 	struct dp_ctrl_private *ctrl;
+	unsigned long pixel_rate;
 	unsigned long pixel_rate_orig;
 
 	if (!dp_ctrl)
@@ -1843,25 +1843,24 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl, bool force_link_train)
 
 	ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
 
-	ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
+	pixel_rate = pixel_rate_orig = ctrl->panel->dp_mode.drm_mode.clock;
 
-	pixel_rate_orig = ctrl->dp_ctrl.pixel_rate;
 	if (dp_ctrl->wide_bus_en)
-		ctrl->dp_ctrl.pixel_rate >>= 1;
+		pixel_rate >>= 1;
 
-	drm_dbg_dp(ctrl->drm_dev, "rate=%d, num_lanes=%d, pixel_rate=%d\n",
+	drm_dbg_dp(ctrl->drm_dev, "rate=%d, num_lanes=%d, pixel_rate=%lu\n",
 		ctrl->link->link_params.rate,
-		ctrl->link->link_params.num_lanes, ctrl->dp_ctrl.pixel_rate);
+		ctrl->link->link_params.num_lanes, pixel_rate);
 
 	if (!dp_power_clk_status(ctrl->power, DP_CTRL_PM)) { /* link clk is off */
-		ret = dp_ctrl_enable_mainlink_clocks(ctrl);
+		ret = dp_ctrl_enable_mainlink_clocks(ctrl, pixel_rate);
 		if (ret) {
 			DRM_ERROR("Failed to start link clocks. ret=%d\n", ret);
 			goto end;
 		}
 	}
 
-	ret = dp_ctrl_enable_stream_clocks(ctrl);
+	ret = dp_ctrl_enable_stream_clocks(ctrl, pixel_rate);
 	if (ret) {
 		DRM_ERROR("Failed to start pixel clocks. ret=%d\n", ret);
 		goto end;
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h b/drivers/gpu/drm/msm/dp/dp_ctrl.h
index b563e2e3bfe5..9f29734af81c 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
@@ -16,7 +16,6 @@
 struct dp_ctrl {
 	bool orientation;
 	atomic_t aborted;
-	u32 pixel_rate;
 	bool wide_bus_en;
 };
 
-- 
https://chromeos.dev

[PATCH 2/3] drm/msm/dp: Remove pixel_rate from struct dp_ctrl

[Stephen Boyd]

This struct member is stored to in the function that calls the function
which uses it. That's possible with a function argument instead of
storing to a struct member. Pass the pixel_rate as an argument instead
to simplify the code. Note that dp_ctrl_link_maintenance() was storing
the pixel_rate but never using it so we just remove the assignment from
there.

Cc: Kuogee Hsieh <quic_khsieh@quicinc.com>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/gpu/drm/msm/dp/dp_ctrl.c | 57 ++++++++++++++++----------------
 drivers/gpu/drm/msm/dp/dp_ctrl.h |  1 -
 2 files changed, 28 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index bd445e683cfc..e114521af2e9 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1336,7 +1336,7 @@ static void dp_ctrl_set_clock_rate(struct dp_ctrl_private *ctrl,
 				name, rate);
 }
 
-static int dp_ctrl_enable_mainlink_clocks(struct dp_ctrl_private *ctrl)
+static int dp_ctrl_enable_mainlink_clocks(struct dp_ctrl_private *ctrl, unsigned long pixel_rate)
 {
 	int ret = 0;
 	struct dp_io *dp_io = &ctrl->parser->io;
@@ -1357,25 +1357,25 @@ static int dp_ctrl_enable_mainlink_clocks(struct dp_ctrl_private *ctrl)
 	if (ret)
 		DRM_ERROR("Unable to start link clocks. ret=%d\n", ret);
 
-	drm_dbg_dp(ctrl->drm_dev, "link rate=%d pixel_clk=%d\n",
-		ctrl->link->link_params.rate, ctrl->dp_ctrl.pixel_rate);
+	drm_dbg_dp(ctrl->drm_dev, "link rate=%d pixel_clk=%lu\n",
+		ctrl->link->link_params.rate, pixel_rate);
 
 	return ret;
 }
 
-static int dp_ctrl_enable_stream_clocks(struct dp_ctrl_private *ctrl)
+static int dp_ctrl_enable_stream_clocks(struct dp_ctrl_private *ctrl,
+					unsigned long pixel_rate)
 {
-	int ret = 0;
+	int ret;
 
-	dp_ctrl_set_clock_rate(ctrl, DP_STREAM_PM, "stream_pixel",
-					ctrl->dp_ctrl.pixel_rate * 1000);
+	dp_ctrl_set_clock_rate(ctrl, DP_STREAM_PM, "stream_pixel", pixel_rate * 1000);
 
 	ret = dp_power_clk_enable(ctrl->power, DP_STREAM_PM, true);
 	if (ret)
 		DRM_ERROR("Unabled to start pixel clocks. ret=%d\n", ret);
 
-	drm_dbg_dp(ctrl->drm_dev, "link rate=%d pixel_clk=%d\n",
-			ctrl->link->link_params.rate, ctrl->dp_ctrl.pixel_rate);
+	drm_dbg_dp(ctrl->drm_dev, "link rate=%d pixel_clk=%lu\n",
+			ctrl->link->link_params.rate, pixel_rate);
 
 	return ret;
 }
@@ -1445,7 +1445,7 @@ static bool dp_ctrl_use_fixed_nvid(struct dp_ctrl_private *ctrl)
 	return false;
 }
 
-static int dp_ctrl_reinitialize_mainlink(struct dp_ctrl_private *ctrl)
+static int dp_ctrl_reinitialize_mainlink(struct dp_ctrl_private *ctrl, unsigned long pixel_rate)
 {
 	int ret = 0;
 	struct dp_io *dp_io = &ctrl->parser->io;
@@ -1469,7 +1469,7 @@ static int dp_ctrl_reinitialize_mainlink(struct dp_ctrl_private *ctrl)
 	/* hw recommended delay before re-enabling clocks */
 	msleep(20);
 
-	ret = dp_ctrl_enable_mainlink_clocks(ctrl);
+	ret = dp_ctrl_enable_mainlink_clocks(ctrl, pixel_rate);
 	if (ret) {
 		DRM_ERROR("Failed to enable mainlink clks. ret=%d\n", ret);
 		return ret;
@@ -1517,8 +1517,6 @@ static int dp_ctrl_link_maintenance(struct dp_ctrl_private *ctrl)
 	ctrl->link->phy_params.p_level = 0;
 	ctrl->link->phy_params.v_level = 0;
 
-	ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
-
 	ret = dp_ctrl_setup_main_link(ctrl, &training_step);
 	if (ret)
 		goto end;
@@ -1588,12 +1586,12 @@ static int dp_ctrl_on_stream_phy_test_report(struct dp_ctrl *dp_ctrl)
 {
 	int ret;
 	struct dp_ctrl_private *ctrl;
+	unsigned long pixel_rate;
 
 	ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
 
-	ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
-
-	ret = dp_ctrl_enable_stream_clocks(ctrl);
+	pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
+	ret = dp_ctrl_enable_stream_clocks(ctrl, pixel_rate);
 	if (ret) {
 		DRM_ERROR("Failed to start pixel clocks. ret=%d\n", ret);
 		return ret;
@@ -1709,6 +1707,7 @@ int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
 	u32 const phy_cts_pixel_clk_khz = 148500;
 	u8 link_status[DP_LINK_STATUS_SIZE];
 	unsigned int training_step;
+	unsigned long pixel_rate;
 
 	if (!dp_ctrl)
 		return -EINVAL;
@@ -1723,25 +1722,25 @@ int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
 		drm_dbg_dp(ctrl->drm_dev,
 				"using phy test link parameters\n");
 		if (!ctrl->panel->dp_mode.drm_mode.clock)
-			ctrl->dp_ctrl.pixel_rate = phy_cts_pixel_clk_khz;
+			pixel_rate = phy_cts_pixel_clk_khz;
 	} else {
 		ctrl->link->link_params.rate = rate;
 		ctrl->link->link_params.num_lanes =
 			ctrl->panel->link_info.num_lanes;
-		ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
+		pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
 	}
 
-	drm_dbg_dp(ctrl->drm_dev, "rate=%d, num_lanes=%d, pixel_rate=%d\n",
+	drm_dbg_dp(ctrl->drm_dev, "rate=%d, num_lanes=%d, pixel_rate=%lu\n",
 		ctrl->link->link_params.rate, ctrl->link->link_params.num_lanes,
-		ctrl->dp_ctrl.pixel_rate);
+		pixel_rate);
 
 
-	rc = dp_ctrl_enable_mainlink_clocks(ctrl);
+	rc = dp_ctrl_enable_mainlink_clocks(ctrl, pixel_rate);
 	if (rc)
 		return rc;
 
 	while (--link_train_max_retries) {
-		rc = dp_ctrl_reinitialize_mainlink(ctrl);
+		rc = dp_ctrl_reinitialize_mainlink(ctrl, pixel_rate);
 		if (rc) {
 			DRM_ERROR("Failed to reinitialize mainlink. rc=%d\n",
 					rc);
@@ -1836,6 +1835,7 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl, bool force_link_train)
 	int ret = 0;
 	bool mainlink_ready = false;
 	struct dp_ctrl_private *ctrl;
+	unsigned long pixel_rate;
 	unsigned long pixel_rate_orig;
 
 	if (!dp_ctrl)
@@ -1843,25 +1843,24 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl, bool force_link_train)
 
 	ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
 
-	ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
+	pixel_rate = pixel_rate_orig = ctrl->panel->dp_mode.drm_mode.clock;
 
-	pixel_rate_orig = ctrl->dp_ctrl.pixel_rate;
 	if (dp_ctrl->wide_bus_en)
-		ctrl->dp_ctrl.pixel_rate >>= 1;
+		pixel_rate >>= 1;
 
-	drm_dbg_dp(ctrl->drm_dev, "rate=%d, num_lanes=%d, pixel_rate=%d\n",
+	drm_dbg_dp(ctrl->drm_dev, "rate=%d, num_lanes=%d, pixel_rate=%lu\n",
 		ctrl->link->link_params.rate,
-		ctrl->link->link_params.num_lanes, ctrl->dp_ctrl.pixel_rate);
+		ctrl->link->link_params.num_lanes, pixel_rate);
 
 	if (!dp_power_clk_status(ctrl->power, DP_CTRL_PM)) { /* link clk is off */
-		ret = dp_ctrl_enable_mainlink_clocks(ctrl);
+		ret = dp_ctrl_enable_mainlink_clocks(ctrl, pixel_rate);
 		if (ret) {
 			DRM_ERROR("Failed to start link clocks. ret=%d\n", ret);
 			goto end;
 		}
 	}
 
-	ret = dp_ctrl_enable_stream_clocks(ctrl);
+	ret = dp_ctrl_enable_stream_clocks(ctrl, pixel_rate);
 	if (ret) {
 		DRM_ERROR("Failed to start pixel clocks. ret=%d\n", ret);
 		goto end;
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h b/drivers/gpu/drm/msm/dp/dp_ctrl.h
index b563e2e3bfe5..9f29734af81c 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
@@ -16,7 +16,6 @@
 struct dp_ctrl {
 	bool orientation;
 	atomic_t aborted;
-	u32 pixel_rate;
 	bool wide_bus_en;
 };
 
-- 
https://chromeos.dev

[PATCH 3/3] drm/msm/dp: Get rid of dp_ctrl_on_stream_phy_test_report()

[Stephen Boyd]

This API isn't really more than a couple lines now that we don't store
the pixel_rate to the struct member. Inline it into the caller.

Cc: Kuogee Hsieh <quic_khsieh@quicinc.com>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/gpu/drm/msm/dp/dp_ctrl.c | 40 ++++++++++++--------------------
 1 file changed, 15 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index e114521af2e9..d04fddb29fdf 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1582,34 +1582,15 @@ static bool dp_ctrl_send_phy_test_pattern(struct dp_ctrl_private *ctrl)
 	return success;
 }
 
-static int dp_ctrl_on_stream_phy_test_report(struct dp_ctrl *dp_ctrl)
+static int dp_ctrl_process_phy_test_request(struct dp_ctrl_private *ctrl)
 {
 	int ret;
-	struct dp_ctrl_private *ctrl;
 	unsigned long pixel_rate;
 
-	ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
-
-	pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
-	ret = dp_ctrl_enable_stream_clocks(ctrl, pixel_rate);
-	if (ret) {
-		DRM_ERROR("Failed to start pixel clocks. ret=%d\n", ret);
-		return ret;
-	}
-
-	dp_ctrl_send_phy_test_pattern(ctrl);
-
-	return 0;
-}
-
-static int dp_ctrl_process_phy_test_request(struct dp_ctrl_private *ctrl)
-{
-	int ret = 0;
-
 	if (!ctrl->link->phy_params.phy_test_pattern_sel) {
 		drm_dbg_dp(ctrl->drm_dev,
 			"no test pattern selected by sink\n");
-		return ret;
+		return 0;
 	}
 
 	/*
@@ -1624,12 +1605,21 @@ static int dp_ctrl_process_phy_test_request(struct dp_ctrl_private *ctrl)
 	}
 
 	ret = dp_ctrl_on_link(&ctrl->dp_ctrl);
-	if (!ret)
-		ret = dp_ctrl_on_stream_phy_test_report(&ctrl->dp_ctrl);
-	else
+	if (ret) {
 		DRM_ERROR("failed to enable DP link controller\n");
+		return ret;
+	}
 
-	return ret;
+	pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
+	ret = dp_ctrl_enable_stream_clocks(ctrl, pixel_rate);
+	if (ret) {
+		DRM_ERROR("Failed to start pixel clocks. ret=%d\n", ret);
+		return ret;
+	}
+
+	dp_ctrl_send_phy_test_pattern(ctrl);
+
+	return 0;
 }
 
 void dp_ctrl_handle_sink_request(struct dp_ctrl *dp_ctrl)
-- 
https://chromeos.dev

[PATCH 3/3] drm/msm/dp: Get rid of dp_ctrl_on_stream_phy_test_report()

[Stephen Boyd]

This API isn't really more than a couple lines now that we don't store
the pixel_rate to the struct member. Inline it into the caller.

Cc: Kuogee Hsieh <quic_khsieh@quicinc.com>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/gpu/drm/msm/dp/dp_ctrl.c | 40 ++++++++++++--------------------
 1 file changed, 15 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index e114521af2e9..d04fddb29fdf 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1582,34 +1582,15 @@ static bool dp_ctrl_send_phy_test_pattern(struct dp_ctrl_private *ctrl)
 	return success;
 }
 
-static int dp_ctrl_on_stream_phy_test_report(struct dp_ctrl *dp_ctrl)
+static int dp_ctrl_process_phy_test_request(struct dp_ctrl_private *ctrl)
 {
 	int ret;
-	struct dp_ctrl_private *ctrl;
 	unsigned long pixel_rate;
 
-	ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
-
-	pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
-	ret = dp_ctrl_enable_stream_clocks(ctrl, pixel_rate);
-	if (ret) {
-		DRM_ERROR("Failed to start pixel clocks. ret=%d\n", ret);
-		return ret;
-	}
-
-	dp_ctrl_send_phy_test_pattern(ctrl);
-
-	return 0;
-}
-
-static int dp_ctrl_process_phy_test_request(struct dp_ctrl_private *ctrl)
-{
-	int ret = 0;
-
 	if (!ctrl->link->phy_params.phy_test_pattern_sel) {
 		drm_dbg_dp(ctrl->drm_dev,
 			"no test pattern selected by sink\n");
-		return ret;
+		return 0;
 	}
 
 	/*
@@ -1624,12 +1605,21 @@ static int dp_ctrl_process_phy_test_request(struct dp_ctrl_private *ctrl)
 	}
 
 	ret = dp_ctrl_on_link(&ctrl->dp_ctrl);
-	if (!ret)
-		ret = dp_ctrl_on_stream_phy_test_report(&ctrl->dp_ctrl);
-	else
+	if (ret) {
 		DRM_ERROR("failed to enable DP link controller\n");
+		return ret;
+	}
 
-	return ret;
+	pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
+	ret = dp_ctrl_enable_stream_clocks(ctrl, pixel_rate);
+	if (ret) {
+		DRM_ERROR("Failed to start pixel clocks. ret=%d\n", ret);
+		return ret;
+	}
+
+	dp_ctrl_send_phy_test_pattern(ctrl);
+
+	return 0;
 }
 
 void dp_ctrl_handle_sink_request(struct dp_ctrl *dp_ctrl)
-- 
https://chromeos.dev

Re: [PATCH 1/3] drm/msm/dp: Reorganize code to avoid forward declaration

[Dmitry Baryshkov]

On 17/06/2022 23:47, Stephen Boyd wrote:
> Let's move these functions around to avoid having to forward declare
> dp_ctrl_on_stream_phy_test_report(). Also remove
> dp_ctrl_reinitialize_mainlink() forward declaration because we're doing
> that sort of task.
> 
> Cc: Kuogee Hsieh <quic_khsieh@quicinc.com>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>

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

> ---
>   drivers/gpu/drm/msm/dp/dp_ctrl.c | 104 +++++++++++++++----------------
>   1 file changed, 50 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index 703249384e7c..bd445e683cfc 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -1238,8 +1238,6 @@ static int dp_ctrl_link_train_2(struct dp_ctrl_private *ctrl,
>   	return -ETIMEDOUT;
>   }
>   
> -static int dp_ctrl_reinitialize_mainlink(struct dp_ctrl_private *ctrl);
> -
>   static int dp_ctrl_link_train(struct dp_ctrl_private *ctrl,
>   			int *training_step)
>   {
> @@ -1534,38 +1532,6 @@ static int dp_ctrl_link_maintenance(struct dp_ctrl_private *ctrl)
>   	return ret;
>   }
>   
> -static int dp_ctrl_on_stream_phy_test_report(struct dp_ctrl *dp_ctrl);
> -
> -static int dp_ctrl_process_phy_test_request(struct dp_ctrl_private *ctrl)
> -{
> -	int ret = 0;
> -
> -	if (!ctrl->link->phy_params.phy_test_pattern_sel) {
> -		drm_dbg_dp(ctrl->drm_dev,
> -			"no test pattern selected by sink\n");
> -		return ret;
> -	}
> -
> -	/*
> -	 * The global reset will need DP link related clocks to be
> -	 * running. Add the global reset just before disabling the
> -	 * link clocks and core clocks.
> -	 */
> -	ret = dp_ctrl_off(&ctrl->dp_ctrl);
> -	if (ret) {
> -		DRM_ERROR("failed to disable DP controller\n");
> -		return ret;
> -	}
> -
> -	ret = dp_ctrl_on_link(&ctrl->dp_ctrl);
> -	if (!ret)
> -		ret = dp_ctrl_on_stream_phy_test_report(&ctrl->dp_ctrl);
> -	else
> -		DRM_ERROR("failed to enable DP link controller\n");
> -
> -	return ret;
> -}
> -
>   static bool dp_ctrl_send_phy_test_pattern(struct dp_ctrl_private *ctrl)
>   {
>   	bool success = false;
> @@ -1618,6 +1584,56 @@ static bool dp_ctrl_send_phy_test_pattern(struct dp_ctrl_private *ctrl)
>   	return success;
>   }
>   
> +static int dp_ctrl_on_stream_phy_test_report(struct dp_ctrl *dp_ctrl)
> +{
> +	int ret;
> +	struct dp_ctrl_private *ctrl;
> +
> +	ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
> +
> +	ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
> +
> +	ret = dp_ctrl_enable_stream_clocks(ctrl);
> +	if (ret) {
> +		DRM_ERROR("Failed to start pixel clocks. ret=%d\n", ret);
> +		return ret;
> +	}
> +
> +	dp_ctrl_send_phy_test_pattern(ctrl);
> +
> +	return 0;
> +}
> +
> +static int dp_ctrl_process_phy_test_request(struct dp_ctrl_private *ctrl)
> +{
> +	int ret = 0;
> +
> +	if (!ctrl->link->phy_params.phy_test_pattern_sel) {
> +		drm_dbg_dp(ctrl->drm_dev,
> +			"no test pattern selected by sink\n");
> +		return ret;
> +	}
> +
> +	/*
> +	 * The global reset will need DP link related clocks to be
> +	 * running. Add the global reset just before disabling the
> +	 * link clocks and core clocks.
> +	 */
> +	ret = dp_ctrl_off(&ctrl->dp_ctrl);
> +	if (ret) {
> +		DRM_ERROR("failed to disable DP controller\n");
> +		return ret;
> +	}
> +
> +	ret = dp_ctrl_on_link(&ctrl->dp_ctrl);
> +	if (!ret)
> +		ret = dp_ctrl_on_stream_phy_test_report(&ctrl->dp_ctrl);
> +	else
> +		DRM_ERROR("failed to enable DP link controller\n");
> +
> +	return ret;
> +}
> +
>   void dp_ctrl_handle_sink_request(struct dp_ctrl *dp_ctrl)
>   {
>   	struct dp_ctrl_private *ctrl;
> @@ -1815,26 +1831,6 @@ static int dp_ctrl_link_retrain(struct dp_ctrl_private *ctrl)
>   	return dp_ctrl_setup_main_link(ctrl, &training_step);
>   }
>   
> -static int dp_ctrl_on_stream_phy_test_report(struct dp_ctrl *dp_ctrl)
> -{
> -	int ret;
> -	struct dp_ctrl_private *ctrl;
> -
> -	ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
> -
> -	ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
> -
> -	ret = dp_ctrl_enable_stream_clocks(ctrl);
> -	if (ret) {
> -		DRM_ERROR("Failed to start pixel clocks. ret=%d\n", ret);
> -		return ret;
> -	}
> -
> -	dp_ctrl_send_phy_test_pattern(ctrl);
> -
> -	return 0;
> -}
> -
>   int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl, bool force_link_train)
>   {
>   	int ret = 0;


-- 
With best wishes
Dmitry

Re: [PATCH 1/3] drm/msm/dp: Reorganize code to avoid forward declaration

[Dmitry Baryshkov]

On 17/06/2022 23:47, Stephen Boyd wrote:
> Let's move these functions around to avoid having to forward declare
> dp_ctrl_on_stream_phy_test_report(). Also remove
> dp_ctrl_reinitialize_mainlink() forward declaration because we're doing
> that sort of task.
> 
> Cc: Kuogee Hsieh <quic_khsieh@quicinc.com>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>

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

> ---
>   drivers/gpu/drm/msm/dp/dp_ctrl.c | 104 +++++++++++++++----------------
>   1 file changed, 50 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index 703249384e7c..bd445e683cfc 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -1238,8 +1238,6 @@ static int dp_ctrl_link_train_2(struct dp_ctrl_private *ctrl,
>   	return -ETIMEDOUT;
>   }
>   
> -static int dp_ctrl_reinitialize_mainlink(struct dp_ctrl_private *ctrl);
> -
>   static int dp_ctrl_link_train(struct dp_ctrl_private *ctrl,
>   			int *training_step)
>   {
> @@ -1534,38 +1532,6 @@ static int dp_ctrl_link_maintenance(struct dp_ctrl_private *ctrl)
>   	return ret;
>   }
>   
> -static int dp_ctrl_on_stream_phy_test_report(struct dp_ctrl *dp_ctrl);
> -
> -static int dp_ctrl_process_phy_test_request(struct dp_ctrl_private *ctrl)
> -{
> -	int ret = 0;
> -
> -	if (!ctrl->link->phy_params.phy_test_pattern_sel) {
> -		drm_dbg_dp(ctrl->drm_dev,
> -			"no test pattern selected by sink\n");
> -		return ret;
> -	}
> -
> -	/*
> -	 * The global reset will need DP link related clocks to be
> -	 * running. Add the global reset just before disabling the
> -	 * link clocks and core clocks.
> -	 */
> -	ret = dp_ctrl_off(&ctrl->dp_ctrl);
> -	if (ret) {
> -		DRM_ERROR("failed to disable DP controller\n");
> -		return ret;
> -	}
> -
> -	ret = dp_ctrl_on_link(&ctrl->dp_ctrl);
> -	if (!ret)
> -		ret = dp_ctrl_on_stream_phy_test_report(&ctrl->dp_ctrl);
> -	else
> -		DRM_ERROR("failed to enable DP link controller\n");
> -
> -	return ret;
> -}
> -
>   static bool dp_ctrl_send_phy_test_pattern(struct dp_ctrl_private *ctrl)
>   {
>   	bool success = false;
> @@ -1618,6 +1584,56 @@ static bool dp_ctrl_send_phy_test_pattern(struct dp_ctrl_private *ctrl)
>   	return success;
>   }
>   
> +static int dp_ctrl_on_stream_phy_test_report(struct dp_ctrl *dp_ctrl)
> +{
> +	int ret;
> +	struct dp_ctrl_private *ctrl;
> +
> +	ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
> +
> +	ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
> +
> +	ret = dp_ctrl_enable_stream_clocks(ctrl);
> +	if (ret) {
> +		DRM_ERROR("Failed to start pixel clocks. ret=%d\n", ret);
> +		return ret;
> +	}
> +
> +	dp_ctrl_send_phy_test_pattern(ctrl);
> +
> +	return 0;
> +}
> +
> +static int dp_ctrl_process_phy_test_request(struct dp_ctrl_private *ctrl)
> +{
> +	int ret = 0;
> +
> +	if (!ctrl->link->phy_params.phy_test_pattern_sel) {
> +		drm_dbg_dp(ctrl->drm_dev,
> +			"no test pattern selected by sink\n");
> +		return ret;
> +	}
> +
> +	/*
> +	 * The global reset will need DP link related clocks to be
> +	 * running. Add the global reset just before disabling the
> +	 * link clocks and core clocks.
> +	 */
> +	ret = dp_ctrl_off(&ctrl->dp_ctrl);
> +	if (ret) {
> +		DRM_ERROR("failed to disable DP controller\n");
> +		return ret;
> +	}
> +
> +	ret = dp_ctrl_on_link(&ctrl->dp_ctrl);
> +	if (!ret)
> +		ret = dp_ctrl_on_stream_phy_test_report(&ctrl->dp_ctrl);
> +	else
> +		DRM_ERROR("failed to enable DP link controller\n");
> +
> +	return ret;
> +}
> +
>   void dp_ctrl_handle_sink_request(struct dp_ctrl *dp_ctrl)
>   {
>   	struct dp_ctrl_private *ctrl;
> @@ -1815,26 +1831,6 @@ static int dp_ctrl_link_retrain(struct dp_ctrl_private *ctrl)
>   	return dp_ctrl_setup_main_link(ctrl, &training_step);
>   }
>   
> -static int dp_ctrl_on_stream_phy_test_report(struct dp_ctrl *dp_ctrl)
> -{
> -	int ret;
> -	struct dp_ctrl_private *ctrl;
> -
> -	ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
> -
> -	ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
> -
> -	ret = dp_ctrl_enable_stream_clocks(ctrl);
> -	if (ret) {
> -		DRM_ERROR("Failed to start pixel clocks. ret=%d\n", ret);
> -		return ret;
> -	}
> -
> -	dp_ctrl_send_phy_test_pattern(ctrl);
> -
> -	return 0;
> -}
> -
>   int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl, bool force_link_train)
>   {
>   	int ret = 0;


-- 
With best wishes
Dmitry

Re: [PATCH 2/3] drm/msm/dp: Remove pixel_rate from struct dp_ctrl

[Dmitry Baryshkov]

On 17/06/2022 23:47, Stephen Boyd wrote:
> This struct member is stored to in the function that calls the function
> which uses it. That's possible with a function argument instead of
> storing to a struct member. Pass the pixel_rate as an argument instead
> to simplify the code. Note that dp_ctrl_link_maintenance() was storing
> the pixel_rate but never using it so we just remove the assignment from
> there.
> 
> Cc: Kuogee Hsieh <quic_khsieh@quicinc.com>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>   drivers/gpu/drm/msm/dp/dp_ctrl.c | 57 ++++++++++++++++----------------
>   drivers/gpu/drm/msm/dp/dp_ctrl.h |  1 -
>   2 files changed, 28 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index bd445e683cfc..e114521af2e9 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -1336,7 +1336,7 @@ static void dp_ctrl_set_clock_rate(struct dp_ctrl_private *ctrl,
>   				name, rate);
>   }
>   
> -static int dp_ctrl_enable_mainlink_clocks(struct dp_ctrl_private *ctrl)
> +static int dp_ctrl_enable_mainlink_clocks(struct dp_ctrl_private *ctrl, unsigned long pixel_rate)


I think we can read pixel_rate here rather than getting it as an 
argument. We'd need to move handling (DP_TEST_LINK_PHY_TEST_PATTERN && 
!ctrl->panel->dp_mode.drm_mode.clock) case here from dp_ctrl_on_link().

>   {
>   	int ret = 0;
>   	struct dp_io *dp_io = &ctrl->parser->io;
> @@ -1357,25 +1357,25 @@ static int dp_ctrl_enable_mainlink_clocks(struct dp_ctrl_private *ctrl)
>   	if (ret)
>   		DRM_ERROR("Unable to start link clocks. ret=%d\n", ret);
>   
> -	drm_dbg_dp(ctrl->drm_dev, "link rate=%d pixel_clk=%d\n",
> -		ctrl->link->link_params.rate, ctrl->dp_ctrl.pixel_rate);
> +	drm_dbg_dp(ctrl->drm_dev, "link rate=%d pixel_clk=%lu\n",
> +		ctrl->link->link_params.rate, pixel_rate);
>   
>   	return ret;
>   }
>   
> -static int dp_ctrl_enable_stream_clocks(struct dp_ctrl_private *ctrl)
> +static int dp_ctrl_enable_stream_clocks(struct dp_ctrl_private *ctrl,
> +					unsigned long pixel_rate)
>   {
> -	int ret = 0;
> +	int ret;
>   
> -	dp_ctrl_set_clock_rate(ctrl, DP_STREAM_PM, "stream_pixel",
> -					ctrl->dp_ctrl.pixel_rate * 1000);
> +	dp_ctrl_set_clock_rate(ctrl, DP_STREAM_PM, "stream_pixel", pixel_rate * 1000);

Note to myself (or to anybody doing further cleanup): store stream_pixel 
clock into dp_ctrl_private and set it directly here. Then 
dp_ctrl_set_clock_rate() can be removed.

>   
>   	ret = dp_power_clk_enable(ctrl->power, DP_STREAM_PM, true);
>   	if (ret)
>   		DRM_ERROR("Unabled to start pixel clocks. ret=%d\n", ret);
>   
> -	drm_dbg_dp(ctrl->drm_dev, "link rate=%d pixel_clk=%d\n",
> -			ctrl->link->link_params.rate, ctrl->dp_ctrl.pixel_rate);
> +	drm_dbg_dp(ctrl->drm_dev, "link rate=%d pixel_clk=%lu\n",
> +			ctrl->link->link_params.rate, pixel_rate);
>   
>   	return ret;
>   }
> @@ -1445,7 +1445,7 @@ static bool dp_ctrl_use_fixed_nvid(struct dp_ctrl_private *ctrl)
>   	return false;
>   }
>   
> -static int dp_ctrl_reinitialize_mainlink(struct dp_ctrl_private *ctrl)
> +static int dp_ctrl_reinitialize_mainlink(struct dp_ctrl_private *ctrl, unsigned long pixel_rate)
>   {
>   	int ret = 0;
>   	struct dp_io *dp_io = &ctrl->parser->io;
> @@ -1469,7 +1469,7 @@ static int dp_ctrl_reinitialize_mainlink(struct dp_ctrl_private *ctrl)
>   	/* hw recommended delay before re-enabling clocks */
>   	msleep(20);
>   
> -	ret = dp_ctrl_enable_mainlink_clocks(ctrl);
> +	ret = dp_ctrl_enable_mainlink_clocks(ctrl, pixel_rate);
>   	if (ret) {
>   		DRM_ERROR("Failed to enable mainlink clks. ret=%d\n", ret);
>   		return ret;
> @@ -1517,8 +1517,6 @@ static int dp_ctrl_link_maintenance(struct dp_ctrl_private *ctrl)
>   	ctrl->link->phy_params.p_level = 0;
>   	ctrl->link->phy_params.v_level = 0;
>   
> -	ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
> -
>   	ret = dp_ctrl_setup_main_link(ctrl, &training_step);
>   	if (ret)
>   		goto end;
> @@ -1588,12 +1586,12 @@ static int dp_ctrl_on_stream_phy_test_report(struct dp_ctrl *dp_ctrl)
>   {
>   	int ret;
>   	struct dp_ctrl_private *ctrl;
> +	unsigned long pixel_rate;
>   
>   	ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
>   
> -	ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
> -
> -	ret = dp_ctrl_enable_stream_clocks(ctrl);
> +	pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
> +	ret = dp_ctrl_enable_stream_clocks(ctrl, pixel_rate);

I think we can take another step forward here. Read the 
ctrl->panel->dp_mode.drm_mode.clock from within the 
dp_ctrl_enable_stream_clocks() function. This removes the need to pass 
pixel_rate as an argument here.

>   	if (ret) {
>   		DRM_ERROR("Failed to start pixel clocks. ret=%d\n", ret);
>   		return ret;
> @@ -1709,6 +1707,7 @@ int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
>   	u32 const phy_cts_pixel_clk_khz = 148500;
>   	u8 link_status[DP_LINK_STATUS_SIZE];
>   	unsigned int training_step;
> +	unsigned long pixel_rate;
>   
>   	if (!dp_ctrl)
>   		return -EINVAL;
> @@ -1723,25 +1722,25 @@ int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
>   		drm_dbg_dp(ctrl->drm_dev,
>   				"using phy test link parameters\n");
>   		if (!ctrl->panel->dp_mode.drm_mode.clock)
> -			ctrl->dp_ctrl.pixel_rate = phy_cts_pixel_clk_khz;
> +			pixel_rate = phy_cts_pixel_clk_khz;
>   	} else {
>   		ctrl->link->link_params.rate = rate;
>   		ctrl->link->link_params.num_lanes =
>   			ctrl->panel->link_info.num_lanes;
> -		ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
> +		pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
>   	}
>   
> -	drm_dbg_dp(ctrl->drm_dev, "rate=%d, num_lanes=%d, pixel_rate=%d\n",
> +	drm_dbg_dp(ctrl->drm_dev, "rate=%d, num_lanes=%d, pixel_rate=%lu\n",
>   		ctrl->link->link_params.rate, ctrl->link->link_params.num_lanes,
> -		ctrl->dp_ctrl.pixel_rate);
> +		pixel_rate);
>   
>   
> -	rc = dp_ctrl_enable_mainlink_clocks(ctrl);
> +	rc = dp_ctrl_enable_mainlink_clocks(ctrl, pixel_rate);
>   	if (rc)
>   		return rc;
>   
>   	while (--link_train_max_retries) {
> -		rc = dp_ctrl_reinitialize_mainlink(ctrl);
> +		rc = dp_ctrl_reinitialize_mainlink(ctrl, pixel_rate);
>   		if (rc) {
>   			DRM_ERROR("Failed to reinitialize mainlink. rc=%d\n",
>   					rc);
> @@ -1836,6 +1835,7 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl, bool force_link_train)
>   	int ret = 0;
>   	bool mainlink_ready = false;
>   	struct dp_ctrl_private *ctrl;
> +	unsigned long pixel_rate;
>   	unsigned long pixel_rate_orig;
>   
>   	if (!dp_ctrl)
> @@ -1843,25 +1843,24 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl, bool force_link_train)
>   
>   	ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
>   
> -	ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
> +	pixel_rate = pixel_rate_orig = ctrl->panel->dp_mode.drm_mode.clock;
>   
> -	pixel_rate_orig = ctrl->dp_ctrl.pixel_rate;
>   	if (dp_ctrl->wide_bus_en)
> -		ctrl->dp_ctrl.pixel_rate >>= 1;
> +		pixel_rate >>= 1;
>   
> -	drm_dbg_dp(ctrl->drm_dev, "rate=%d, num_lanes=%d, pixel_rate=%d\n",
> +	drm_dbg_dp(ctrl->drm_dev, "rate=%d, num_lanes=%d, pixel_rate=%lu\n",
>   		ctrl->link->link_params.rate,
> -		ctrl->link->link_params.num_lanes, ctrl->dp_ctrl.pixel_rate);
> +		ctrl->link->link_params.num_lanes, pixel_rate);
>   
>   	if (!dp_power_clk_status(ctrl->power, DP_CTRL_PM)) { /* link clk is off */
> -		ret = dp_ctrl_enable_mainlink_clocks(ctrl);
> +		ret = dp_ctrl_enable_mainlink_clocks(ctrl, pixel_rate);
>   		if (ret) {
>   			DRM_ERROR("Failed to start link clocks. ret=%d\n", ret);
>   			goto end;
>   		}
>   	}
>   
> -	ret = dp_ctrl_enable_stream_clocks(ctrl);
> +	ret = dp_ctrl_enable_stream_clocks(ctrl, pixel_rate);
>   	if (ret) {
>   		DRM_ERROR("Failed to start pixel clocks. ret=%d\n", ret);
>   		goto end;
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h b/drivers/gpu/drm/msm/dp/dp_ctrl.h
> index b563e2e3bfe5..9f29734af81c 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
> @@ -16,7 +16,6 @@
>   struct dp_ctrl {
>   	bool orientation;
>   	atomic_t aborted;
> -	u32 pixel_rate;
>   	bool wide_bus_en;
>   };
>   


-- 
With best wishes
Dmitry

Re: [PATCH 2/3] drm/msm/dp: Remove pixel_rate from struct dp_ctrl

[Dmitry Baryshkov]

On 17/06/2022 23:47, Stephen Boyd wrote:
> This struct member is stored to in the function that calls the function
> which uses it. That's possible with a function argument instead of
> storing to a struct member. Pass the pixel_rate as an argument instead
> to simplify the code. Note that dp_ctrl_link_maintenance() was storing
> the pixel_rate but never using it so we just remove the assignment from
> there.
> 
> Cc: Kuogee Hsieh <quic_khsieh@quicinc.com>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>   drivers/gpu/drm/msm/dp/dp_ctrl.c | 57 ++++++++++++++++----------------
>   drivers/gpu/drm/msm/dp/dp_ctrl.h |  1 -
>   2 files changed, 28 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index bd445e683cfc..e114521af2e9 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -1336,7 +1336,7 @@ static void dp_ctrl_set_clock_rate(struct dp_ctrl_private *ctrl,
>   				name, rate);
>   }
>   
> -static int dp_ctrl_enable_mainlink_clocks(struct dp_ctrl_private *ctrl)
> +static int dp_ctrl_enable_mainlink_clocks(struct dp_ctrl_private *ctrl, unsigned long pixel_rate)


I think we can read pixel_rate here rather than getting it as an 
argument. We'd need to move handling (DP_TEST_LINK_PHY_TEST_PATTERN && 
!ctrl->panel->dp_mode.drm_mode.clock) case here from dp_ctrl_on_link().

>   {
>   	int ret = 0;
>   	struct dp_io *dp_io = &ctrl->parser->io;
> @@ -1357,25 +1357,25 @@ static int dp_ctrl_enable_mainlink_clocks(struct dp_ctrl_private *ctrl)
>   	if (ret)
>   		DRM_ERROR("Unable to start link clocks. ret=%d\n", ret);
>   
> -	drm_dbg_dp(ctrl->drm_dev, "link rate=%d pixel_clk=%d\n",
> -		ctrl->link->link_params.rate, ctrl->dp_ctrl.pixel_rate);
> +	drm_dbg_dp(ctrl->drm_dev, "link rate=%d pixel_clk=%lu\n",
> +		ctrl->link->link_params.rate, pixel_rate);
>   
>   	return ret;
>   }
>   
> -static int dp_ctrl_enable_stream_clocks(struct dp_ctrl_private *ctrl)
> +static int dp_ctrl_enable_stream_clocks(struct dp_ctrl_private *ctrl,
> +					unsigned long pixel_rate)
>   {
> -	int ret = 0;
> +	int ret;
>   
> -	dp_ctrl_set_clock_rate(ctrl, DP_STREAM_PM, "stream_pixel",
> -					ctrl->dp_ctrl.pixel_rate * 1000);
> +	dp_ctrl_set_clock_rate(ctrl, DP_STREAM_PM, "stream_pixel", pixel_rate * 1000);

Note to myself (or to anybody doing further cleanup): store stream_pixel 
clock into dp_ctrl_private and set it directly here. Then 
dp_ctrl_set_clock_rate() can be removed.

>   
>   	ret = dp_power_clk_enable(ctrl->power, DP_STREAM_PM, true);
>   	if (ret)
>   		DRM_ERROR("Unabled to start pixel clocks. ret=%d\n", ret);
>   
> -	drm_dbg_dp(ctrl->drm_dev, "link rate=%d pixel_clk=%d\n",
> -			ctrl->link->link_params.rate, ctrl->dp_ctrl.pixel_rate);
> +	drm_dbg_dp(ctrl->drm_dev, "link rate=%d pixel_clk=%lu\n",
> +			ctrl->link->link_params.rate, pixel_rate);
>   
>   	return ret;
>   }
> @@ -1445,7 +1445,7 @@ static bool dp_ctrl_use_fixed_nvid(struct dp_ctrl_private *ctrl)
>   	return false;
>   }
>   
> -static int dp_ctrl_reinitialize_mainlink(struct dp_ctrl_private *ctrl)
> +static int dp_ctrl_reinitialize_mainlink(struct dp_ctrl_private *ctrl, unsigned long pixel_rate)
>   {
>   	int ret = 0;
>   	struct dp_io *dp_io = &ctrl->parser->io;
> @@ -1469,7 +1469,7 @@ static int dp_ctrl_reinitialize_mainlink(struct dp_ctrl_private *ctrl)
>   	/* hw recommended delay before re-enabling clocks */
>   	msleep(20);
>   
> -	ret = dp_ctrl_enable_mainlink_clocks(ctrl);
> +	ret = dp_ctrl_enable_mainlink_clocks(ctrl, pixel_rate);
>   	if (ret) {
>   		DRM_ERROR("Failed to enable mainlink clks. ret=%d\n", ret);
>   		return ret;
> @@ -1517,8 +1517,6 @@ static int dp_ctrl_link_maintenance(struct dp_ctrl_private *ctrl)
>   	ctrl->link->phy_params.p_level = 0;
>   	ctrl->link->phy_params.v_level = 0;
>   
> -	ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
> -
>   	ret = dp_ctrl_setup_main_link(ctrl, &training_step);
>   	if (ret)
>   		goto end;
> @@ -1588,12 +1586,12 @@ static int dp_ctrl_on_stream_phy_test_report(struct dp_ctrl *dp_ctrl)
>   {
>   	int ret;
>   	struct dp_ctrl_private *ctrl;
> +	unsigned long pixel_rate;
>   
>   	ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
>   
> -	ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
> -
> -	ret = dp_ctrl_enable_stream_clocks(ctrl);
> +	pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
> +	ret = dp_ctrl_enable_stream_clocks(ctrl, pixel_rate);

I think we can take another step forward here. Read the 
ctrl->panel->dp_mode.drm_mode.clock from within the 
dp_ctrl_enable_stream_clocks() function. This removes the need to pass 
pixel_rate as an argument here.

>   	if (ret) {
>   		DRM_ERROR("Failed to start pixel clocks. ret=%d\n", ret);
>   		return ret;
> @@ -1709,6 +1707,7 @@ int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
>   	u32 const phy_cts_pixel_clk_khz = 148500;
>   	u8 link_status[DP_LINK_STATUS_SIZE];
>   	unsigned int training_step;
> +	unsigned long pixel_rate;
>   
>   	if (!dp_ctrl)
>   		return -EINVAL;
> @@ -1723,25 +1722,25 @@ int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
>   		drm_dbg_dp(ctrl->drm_dev,
>   				"using phy test link parameters\n");
>   		if (!ctrl->panel->dp_mode.drm_mode.clock)
> -			ctrl->dp_ctrl.pixel_rate = phy_cts_pixel_clk_khz;
> +			pixel_rate = phy_cts_pixel_clk_khz;
>   	} else {
>   		ctrl->link->link_params.rate = rate;
>   		ctrl->link->link_params.num_lanes =
>   			ctrl->panel->link_info.num_lanes;
> -		ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
> +		pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
>   	}
>   
> -	drm_dbg_dp(ctrl->drm_dev, "rate=%d, num_lanes=%d, pixel_rate=%d\n",
> +	drm_dbg_dp(ctrl->drm_dev, "rate=%d, num_lanes=%d, pixel_rate=%lu\n",
>   		ctrl->link->link_params.rate, ctrl->link->link_params.num_lanes,
> -		ctrl->dp_ctrl.pixel_rate);
> +		pixel_rate);
>   
>   
> -	rc = dp_ctrl_enable_mainlink_clocks(ctrl);
> +	rc = dp_ctrl_enable_mainlink_clocks(ctrl, pixel_rate);
>   	if (rc)
>   		return rc;
>   
>   	while (--link_train_max_retries) {
> -		rc = dp_ctrl_reinitialize_mainlink(ctrl);
> +		rc = dp_ctrl_reinitialize_mainlink(ctrl, pixel_rate);
>   		if (rc) {
>   			DRM_ERROR("Failed to reinitialize mainlink. rc=%d\n",
>   					rc);
> @@ -1836,6 +1835,7 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl, bool force_link_train)
>   	int ret = 0;
>   	bool mainlink_ready = false;
>   	struct dp_ctrl_private *ctrl;
> +	unsigned long pixel_rate;
>   	unsigned long pixel_rate_orig;
>   
>   	if (!dp_ctrl)
> @@ -1843,25 +1843,24 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl, bool force_link_train)
>   
>   	ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
>   
> -	ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
> +	pixel_rate = pixel_rate_orig = ctrl->panel->dp_mode.drm_mode.clock;
>   
> -	pixel_rate_orig = ctrl->dp_ctrl.pixel_rate;
>   	if (dp_ctrl->wide_bus_en)
> -		ctrl->dp_ctrl.pixel_rate >>= 1;
> +		pixel_rate >>= 1;
>   
> -	drm_dbg_dp(ctrl->drm_dev, "rate=%d, num_lanes=%d, pixel_rate=%d\n",
> +	drm_dbg_dp(ctrl->drm_dev, "rate=%d, num_lanes=%d, pixel_rate=%lu\n",
>   		ctrl->link->link_params.rate,
> -		ctrl->link->link_params.num_lanes, ctrl->dp_ctrl.pixel_rate);
> +		ctrl->link->link_params.num_lanes, pixel_rate);
>   
>   	if (!dp_power_clk_status(ctrl->power, DP_CTRL_PM)) { /* link clk is off */
> -		ret = dp_ctrl_enable_mainlink_clocks(ctrl);
> +		ret = dp_ctrl_enable_mainlink_clocks(ctrl, pixel_rate);
>   		if (ret) {
>   			DRM_ERROR("Failed to start link clocks. ret=%d\n", ret);
>   			goto end;
>   		}
>   	}
>   
> -	ret = dp_ctrl_enable_stream_clocks(ctrl);
> +	ret = dp_ctrl_enable_stream_clocks(ctrl, pixel_rate);
>   	if (ret) {
>   		DRM_ERROR("Failed to start pixel clocks. ret=%d\n", ret);
>   		goto end;
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h b/drivers/gpu/drm/msm/dp/dp_ctrl.h
> index b563e2e3bfe5..9f29734af81c 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
> @@ -16,7 +16,6 @@
>   struct dp_ctrl {
>   	bool orientation;
>   	atomic_t aborted;
> -	u32 pixel_rate;
>   	bool wide_bus_en;
>   };
>   


-- 
With best wishes
Dmitry

Re: [PATCH 3/3] drm/msm/dp: Get rid of dp_ctrl_on_stream_phy_test_report()

[Kuogee Hsieh]

On 6/17/2022 1:47 PM, Stephen Boyd wrote:
> This API isn't really more than a couple lines now that we don't store
> the pixel_rate to the struct member. Inline it into the caller.
>
> Cc: Kuogee Hsieh <quic_khsieh@quicinc.com>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
Reviewed-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
>   drivers/gpu/drm/msm/dp/dp_ctrl.c | 40 ++++++++++++--------------------
>   1 file changed, 15 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index e114521af2e9..d04fddb29fdf 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -1582,34 +1582,15 @@ static bool dp_ctrl_send_phy_test_pattern(struct dp_ctrl_private *ctrl)
>   	return success;
>   }
>   
> -static int dp_ctrl_on_stream_phy_test_report(struct dp_ctrl *dp_ctrl)
> +static int dp_ctrl_process_phy_test_request(struct dp_ctrl_private *ctrl)
>   {
>   	int ret;
> -	struct dp_ctrl_private *ctrl;
>   	unsigned long pixel_rate;
>   
> -	ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
> -
> -	pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
> -	ret = dp_ctrl_enable_stream_clocks(ctrl, pixel_rate);
> -	if (ret) {
> -		DRM_ERROR("Failed to start pixel clocks. ret=%d\n", ret);
> -		return ret;
> -	}
> -
> -	dp_ctrl_send_phy_test_pattern(ctrl);
> -
> -	return 0;
> -}
> -
> -static int dp_ctrl_process_phy_test_request(struct dp_ctrl_private *ctrl)
> -{
> -	int ret = 0;
> -
>   	if (!ctrl->link->phy_params.phy_test_pattern_sel) {
>   		drm_dbg_dp(ctrl->drm_dev,
>   			"no test pattern selected by sink\n");
> -		return ret;
> +		return 0;
>   	}
>   
>   	/*
> @@ -1624,12 +1605,21 @@ static int dp_ctrl_process_phy_test_request(struct dp_ctrl_private *ctrl)
>   	}
>   
>   	ret = dp_ctrl_on_link(&ctrl->dp_ctrl);
> -	if (!ret)
> -		ret = dp_ctrl_on_stream_phy_test_report(&ctrl->dp_ctrl);
> -	else
> +	if (ret) {
>   		DRM_ERROR("failed to enable DP link controller\n");
> +		return ret;
> +	}
>   
> -	return ret;
> +	pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
> +	ret = dp_ctrl_enable_stream_clocks(ctrl, pixel_rate);
> +	if (ret) {
> +		DRM_ERROR("Failed to start pixel clocks. ret=%d\n", ret);
> +		return ret;
> +	}
> +
> +	dp_ctrl_send_phy_test_pattern(ctrl);
> +
> +	return 0;
>   }
>   
>   void dp_ctrl_handle_sink_request(struct dp_ctrl *dp_ctrl)

Re: [PATCH 3/3] drm/msm/dp: Get rid of dp_ctrl_on_stream_phy_test_report()

[Kuogee Hsieh]

On 6/17/2022 1:47 PM, Stephen Boyd wrote:
> This API isn't really more than a couple lines now that we don't store
> the pixel_rate to the struct member. Inline it into the caller.
>
> Cc: Kuogee Hsieh <quic_khsieh@quicinc.com>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
Reviewed-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
>   drivers/gpu/drm/msm/dp/dp_ctrl.c | 40 ++++++++++++--------------------
>   1 file changed, 15 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index e114521af2e9..d04fddb29fdf 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -1582,34 +1582,15 @@ static bool dp_ctrl_send_phy_test_pattern(struct dp_ctrl_private *ctrl)
>   	return success;
>   }
>   
> -static int dp_ctrl_on_stream_phy_test_report(struct dp_ctrl *dp_ctrl)
> +static int dp_ctrl_process_phy_test_request(struct dp_ctrl_private *ctrl)
>   {
>   	int ret;
> -	struct dp_ctrl_private *ctrl;
>   	unsigned long pixel_rate;
>   
> -	ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
> -
> -	pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
> -	ret = dp_ctrl_enable_stream_clocks(ctrl, pixel_rate);
> -	if (ret) {
> -		DRM_ERROR("Failed to start pixel clocks. ret=%d\n", ret);
> -		return ret;
> -	}
> -
> -	dp_ctrl_send_phy_test_pattern(ctrl);
> -
> -	return 0;
> -}
> -
> -static int dp_ctrl_process_phy_test_request(struct dp_ctrl_private *ctrl)
> -{
> -	int ret = 0;
> -
>   	if (!ctrl->link->phy_params.phy_test_pattern_sel) {
>   		drm_dbg_dp(ctrl->drm_dev,
>   			"no test pattern selected by sink\n");
> -		return ret;
> +		return 0;
>   	}
>   
>   	/*
> @@ -1624,12 +1605,21 @@ static int dp_ctrl_process_phy_test_request(struct dp_ctrl_private *ctrl)
>   	}
>   
>   	ret = dp_ctrl_on_link(&ctrl->dp_ctrl);
> -	if (!ret)
> -		ret = dp_ctrl_on_stream_phy_test_report(&ctrl->dp_ctrl);
> -	else
> +	if (ret) {
>   		DRM_ERROR("failed to enable DP link controller\n");
> +		return ret;
> +	}
>   
> -	return ret;
> +	pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
> +	ret = dp_ctrl_enable_stream_clocks(ctrl, pixel_rate);
> +	if (ret) {
> +		DRM_ERROR("Failed to start pixel clocks. ret=%d\n", ret);
> +		return ret;
> +	}
> +
> +	dp_ctrl_send_phy_test_pattern(ctrl);
> +
> +	return 0;
>   }
>   
>   void dp_ctrl_handle_sink_request(struct dp_ctrl *dp_ctrl)

Re: [PATCH 1/3] drm/msm/dp: Reorganize code to avoid forward declaration

[Kuogee Hsieh]

On 6/17/2022 3:50 PM, Dmitry Baryshkov wrote:
> On 17/06/2022 23:47, Stephen Boyd wrote:
>> Let's move these functions around to avoid having to forward declare
>> dp_ctrl_on_stream_phy_test_report(). Also remove
>> dp_ctrl_reinitialize_mainlink() forward declaration because we're doing
>> that sort of task.
>>
>> Cc: Kuogee Hsieh <quic_khsieh@quicinc.com>
>> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>
>> ---
>>   drivers/gpu/drm/msm/dp/dp_ctrl.c | 104 +++++++++++++++----------------
>>   1 file changed, 50 insertions(+), 54 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c 
>> b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> index 703249384e7c..bd445e683cfc 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> @@ -1238,8 +1238,6 @@ static int dp_ctrl_link_train_2(struct 
>> dp_ctrl_private *ctrl,
>>       return -ETIMEDOUT;
>>   }
>>   -static int dp_ctrl_reinitialize_mainlink(struct dp_ctrl_private 
>> *ctrl);
>> -
>>   static int dp_ctrl_link_train(struct dp_ctrl_private *ctrl,
>>               int *training_step)
>>   {
>> @@ -1534,38 +1532,6 @@ static int dp_ctrl_link_maintenance(struct 
>> dp_ctrl_private *ctrl)
>>       return ret;
>>   }
>>   -static int dp_ctrl_on_stream_phy_test_report(struct dp_ctrl 
>> *dp_ctrl);
>> -
>> -static int dp_ctrl_process_phy_test_request(struct dp_ctrl_private 
>> *ctrl)
>> -{
>> -    int ret = 0;
>> -
>> -    if (!ctrl->link->phy_params.phy_test_pattern_sel) {
>> -        drm_dbg_dp(ctrl->drm_dev,
>> -            "no test pattern selected by sink\n");
>> -        return ret;
>> -    }
>> -
>> -    /*
>> -     * The global reset will need DP link related clocks to be
>> -     * running. Add the global reset just before disabling the
>> -     * link clocks and core clocks.
>> -     */
>> -    ret = dp_ctrl_off(&ctrl->dp_ctrl);
>> -    if (ret) {
>> -        DRM_ERROR("failed to disable DP controller\n");
>> -        return ret;
>> -    }
>> -
>> -    ret = dp_ctrl_on_link(&ctrl->dp_ctrl);
>> -    if (!ret)
>> -        ret = dp_ctrl_on_stream_phy_test_report(&ctrl->dp_ctrl);
>> -    else
>> -        DRM_ERROR("failed to enable DP link controller\n");
>> -
>> -    return ret;
>> -}
>> -
>>   static bool dp_ctrl_send_phy_test_pattern(struct dp_ctrl_private 
>> *ctrl)
>>   {
>>       bool success = false;
>> @@ -1618,6 +1584,56 @@ static bool 
>> dp_ctrl_send_phy_test_pattern(struct dp_ctrl_private *ctrl)
>>       return success;
>>   }
>>   +static int dp_ctrl_on_stream_phy_test_report(struct dp_ctrl *dp_ctrl)
>> +{
>> +    int ret;
>> +    struct dp_ctrl_private *ctrl;
>> +
>> +    ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
>> +
>> +    ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
>> +
>> +    ret = dp_ctrl_enable_stream_clocks(ctrl);
>> +    if (ret) {
>> +        DRM_ERROR("Failed to start pixel clocks. ret=%d\n", ret);
>> +        return ret;
>> +    }
>> +
>> +    dp_ctrl_send_phy_test_pattern(ctrl);
>> +
>> +    return 0;
>> +}
>> +
>> +static int dp_ctrl_process_phy_test_request(struct dp_ctrl_private 
>> *ctrl)
>> +{
>> +    int ret = 0;
>> +
>> +    if (!ctrl->link->phy_params.phy_test_pattern_sel) {
>> +        drm_dbg_dp(ctrl->drm_dev,
>> +            "no test pattern selected by sink\n");
>> +        return ret;
>> +    }
>> +
>> +    /*
>> +     * The global reset will need DP link related clocks to be
>> +     * running. Add the global reset just before disabling the
>> +     * link clocks and core clocks.
>> +     */
>> +    ret = dp_ctrl_off(&ctrl->dp_ctrl);
>> +    if (ret) {
>> +        DRM_ERROR("failed to disable DP controller\n");
>> +        return ret;
>> +    }
>> +
>> +    ret = dp_ctrl_on_link(&ctrl->dp_ctrl);
>> +    if (!ret)
>> +        ret = dp_ctrl_on_stream_phy_test_report(&ctrl->dp_ctrl);
>> +    else
>> +        DRM_ERROR("failed to enable DP link controller\n");
>> +
>> +    return ret;
>> +}
>> +
>>   void dp_ctrl_handle_sink_request(struct dp_ctrl *dp_ctrl)
>>   {
>>       struct dp_ctrl_private *ctrl;
>> @@ -1815,26 +1831,6 @@ static int dp_ctrl_link_retrain(struct 
>> dp_ctrl_private *ctrl)
>>       return dp_ctrl_setup_main_link(ctrl, &training_step);
>>   }
>>   -static int dp_ctrl_on_stream_phy_test_report(struct dp_ctrl *dp_ctrl)
>> -{
>> -    int ret;
>> -    struct dp_ctrl_private *ctrl;
>> -
>> -    ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
>> -
>> -    ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
>> -
>> -    ret = dp_ctrl_enable_stream_clocks(ctrl);
>> -    if (ret) {
>> -        DRM_ERROR("Failed to start pixel clocks. ret=%d\n", ret);
>> -        return ret;
>> -    }
>> -
>> -    dp_ctrl_send_phy_test_pattern(ctrl);
>> -
>> -    return 0;
>> -}
>> -
>>   int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl, bool force_link_train)
>>   {
>>       int ret = 0;
>
>

Re: [PATCH 1/3] drm/msm/dp: Reorganize code to avoid forward declaration

[Kuogee Hsieh]

On 6/17/2022 3:50 PM, Dmitry Baryshkov wrote:
> On 17/06/2022 23:47, Stephen Boyd wrote:
>> Let's move these functions around to avoid having to forward declare
>> dp_ctrl_on_stream_phy_test_report(). Also remove
>> dp_ctrl_reinitialize_mainlink() forward declaration because we're doing
>> that sort of task.
>>
>> Cc: Kuogee Hsieh <quic_khsieh@quicinc.com>
>> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>
>> ---
>>   drivers/gpu/drm/msm/dp/dp_ctrl.c | 104 +++++++++++++++----------------
>>   1 file changed, 50 insertions(+), 54 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c 
>> b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> index 703249384e7c..bd445e683cfc 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> @@ -1238,8 +1238,6 @@ static int dp_ctrl_link_train_2(struct 
>> dp_ctrl_private *ctrl,
>>       return -ETIMEDOUT;
>>   }
>>   -static int dp_ctrl_reinitialize_mainlink(struct dp_ctrl_private 
>> *ctrl);
>> -
>>   static int dp_ctrl_link_train(struct dp_ctrl_private *ctrl,
>>               int *training_step)
>>   {
>> @@ -1534,38 +1532,6 @@ static int dp_ctrl_link_maintenance(struct 
>> dp_ctrl_private *ctrl)
>>       return ret;
>>   }
>>   -static int dp_ctrl_on_stream_phy_test_report(struct dp_ctrl 
>> *dp_ctrl);
>> -
>> -static int dp_ctrl_process_phy_test_request(struct dp_ctrl_private 
>> *ctrl)
>> -{
>> -    int ret = 0;
>> -
>> -    if (!ctrl->link->phy_params.phy_test_pattern_sel) {
>> -        drm_dbg_dp(ctrl->drm_dev,
>> -            "no test pattern selected by sink\n");
>> -        return ret;
>> -    }
>> -
>> -    /*
>> -     * The global reset will need DP link related clocks to be
>> -     * running. Add the global reset just before disabling the
>> -     * link clocks and core clocks.
>> -     */
>> -    ret = dp_ctrl_off(&ctrl->dp_ctrl);
>> -    if (ret) {
>> -        DRM_ERROR("failed to disable DP controller\n");
>> -        return ret;
>> -    }
>> -
>> -    ret = dp_ctrl_on_link(&ctrl->dp_ctrl);
>> -    if (!ret)
>> -        ret = dp_ctrl_on_stream_phy_test_report(&ctrl->dp_ctrl);
>> -    else
>> -        DRM_ERROR("failed to enable DP link controller\n");
>> -
>> -    return ret;
>> -}
>> -
>>   static bool dp_ctrl_send_phy_test_pattern(struct dp_ctrl_private 
>> *ctrl)
>>   {
>>       bool success = false;
>> @@ -1618,6 +1584,56 @@ static bool 
>> dp_ctrl_send_phy_test_pattern(struct dp_ctrl_private *ctrl)
>>       return success;
>>   }
>>   +static int dp_ctrl_on_stream_phy_test_report(struct dp_ctrl *dp_ctrl)
>> +{
>> +    int ret;
>> +    struct dp_ctrl_private *ctrl;
>> +
>> +    ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
>> +
>> +    ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
>> +
>> +    ret = dp_ctrl_enable_stream_clocks(ctrl);
>> +    if (ret) {
>> +        DRM_ERROR("Failed to start pixel clocks. ret=%d\n", ret);
>> +        return ret;
>> +    }
>> +
>> +    dp_ctrl_send_phy_test_pattern(ctrl);
>> +
>> +    return 0;
>> +}
>> +
>> +static int dp_ctrl_process_phy_test_request(struct dp_ctrl_private 
>> *ctrl)
>> +{
>> +    int ret = 0;
>> +
>> +    if (!ctrl->link->phy_params.phy_test_pattern_sel) {
>> +        drm_dbg_dp(ctrl->drm_dev,
>> +            "no test pattern selected by sink\n");
>> +        return ret;
>> +    }
>> +
>> +    /*
>> +     * The global reset will need DP link related clocks to be
>> +     * running. Add the global reset just before disabling the
>> +     * link clocks and core clocks.
>> +     */
>> +    ret = dp_ctrl_off(&ctrl->dp_ctrl);
>> +    if (ret) {
>> +        DRM_ERROR("failed to disable DP controller\n");
>> +        return ret;
>> +    }
>> +
>> +    ret = dp_ctrl_on_link(&ctrl->dp_ctrl);
>> +    if (!ret)
>> +        ret = dp_ctrl_on_stream_phy_test_report(&ctrl->dp_ctrl);
>> +    else
>> +        DRM_ERROR("failed to enable DP link controller\n");
>> +
>> +    return ret;
>> +}
>> +
>>   void dp_ctrl_handle_sink_request(struct dp_ctrl *dp_ctrl)
>>   {
>>       struct dp_ctrl_private *ctrl;
>> @@ -1815,26 +1831,6 @@ static int dp_ctrl_link_retrain(struct 
>> dp_ctrl_private *ctrl)
>>       return dp_ctrl_setup_main_link(ctrl, &training_step);
>>   }
>>   -static int dp_ctrl_on_stream_phy_test_report(struct dp_ctrl *dp_ctrl)
>> -{
>> -    int ret;
>> -    struct dp_ctrl_private *ctrl;
>> -
>> -    ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
>> -
>> -    ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
>> -
>> -    ret = dp_ctrl_enable_stream_clocks(ctrl);
>> -    if (ret) {
>> -        DRM_ERROR("Failed to start pixel clocks. ret=%d\n", ret);
>> -        return ret;
>> -    }
>> -
>> -    dp_ctrl_send_phy_test_pattern(ctrl);
>> -
>> -    return 0;
>> -}
>> -
>>   int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl, bool force_link_train)
>>   {
>>       int ret = 0;
>
>

Re: [PATCH 2/3] drm/msm/dp: Remove pixel_rate from struct dp_ctrl

[Stephen Boyd]

Quoting Dmitry Baryshkov (2022-06-17 16:07:58)
> On 17/06/2022 23:47, Stephen Boyd wrote:
> > This struct member is stored to in the function that calls the function
> > which uses it. That's possible with a function argument instead of
> > storing to a struct member. Pass the pixel_rate as an argument instead
> > to simplify the code. Note that dp_ctrl_link_maintenance() was storing
> > the pixel_rate but never using it so we just remove the assignment from
> > there.
> >
> > Cc: Kuogee Hsieh <quic_khsieh@quicinc.com>
> > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > ---
> >   drivers/gpu/drm/msm/dp/dp_ctrl.c | 57 ++++++++++++++++----------------
> >   drivers/gpu/drm/msm/dp/dp_ctrl.h |  1 -
> >   2 files changed, 28 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> > index bd445e683cfc..e114521af2e9 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> > @@ -1336,7 +1336,7 @@ static void dp_ctrl_set_clock_rate(struct dp_ctrl_private *ctrl,
> >                               name, rate);
> >   }
> >
> > -static int dp_ctrl_enable_mainlink_clocks(struct dp_ctrl_private *ctrl)
> > +static int dp_ctrl_enable_mainlink_clocks(struct dp_ctrl_private *ctrl, unsigned long pixel_rate)
>
>
> I think we can read pixel_rate here rather than getting it as an
> argument. We'd need to move handling (DP_TEST_LINK_PHY_TEST_PATTERN &&
> !ctrl->panel->dp_mode.drm_mode.clock) case here from dp_ctrl_on_link().

This is also called from dp_ctrl_on_stream() and
dp_ctrl_reinitialize_mainlink(). In the dp_ctrl_on_stream() case we may
divide the pixel_rate by 2 with widebus. We could move the
dp_ctrl_on_link() code here, but then we also need to move widebus, and
then I'm not sure which pixel rate to use.

It looks like the test code doesn't care about widebus? And similarly,
we may run the pixel clk faster until we get a modeset and then divide
it for widebus. Is that why you're suggesting to check
!ctrl->panel->dp_mode.drm_mode.clock? I hesitate because it isn't a
direct conversion, instead it checks some other stashed struct member.

I'll also note that dp_ctrl_enable_mainlink_clocks() doesn't really use
this argument except to print the value in drm_dbg_dp(). Maybe we should
simply remove it from here instead?

> > @@ -1588,12 +1586,12 @@ static int dp_ctrl_on_stream_phy_test_report(struct dp_ctrl *dp_ctrl)
> >   {
> >       int ret;
> >       struct dp_ctrl_private *ctrl;
> > +     unsigned long pixel_rate;
> >
> >       ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
> >
> > -     ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
> > -
> > -     ret = dp_ctrl_enable_stream_clocks(ctrl);
> > +     pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
> > +     ret = dp_ctrl_enable_stream_clocks(ctrl, pixel_rate);
>
> I think we can take another step forward here. Read the
> ctrl->panel->dp_mode.drm_mode.clock from within the
> dp_ctrl_enable_stream_clocks() function. This removes the need to pass
> pixel_rate as an argument here.

This is also affected by widebus and if the function is called from
dp_ctrl_on_stream() or dp_ctrl_on_stream_phy_test_report(). Maybe it
would be better to inline dp_ctrl_enable_stream_clocks() to the
callsites? That would probably simplify things because the function is
mostly a wrapper around a couple functions.

Re: [PATCH 2/3] drm/msm/dp: Remove pixel_rate from struct dp_ctrl

[Stephen Boyd]

Quoting Dmitry Baryshkov (2022-06-17 16:07:58)
> On 17/06/2022 23:47, Stephen Boyd wrote:
> > This struct member is stored to in the function that calls the function
> > which uses it. That's possible with a function argument instead of
> > storing to a struct member. Pass the pixel_rate as an argument instead
> > to simplify the code. Note that dp_ctrl_link_maintenance() was storing
> > the pixel_rate but never using it so we just remove the assignment from
> > there.
> >
> > Cc: Kuogee Hsieh <quic_khsieh@quicinc.com>
> > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > ---
> >   drivers/gpu/drm/msm/dp/dp_ctrl.c | 57 ++++++++++++++++----------------
> >   drivers/gpu/drm/msm/dp/dp_ctrl.h |  1 -
> >   2 files changed, 28 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> > index bd445e683cfc..e114521af2e9 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> > @@ -1336,7 +1336,7 @@ static void dp_ctrl_set_clock_rate(struct dp_ctrl_private *ctrl,
> >                               name, rate);
> >   }
> >
> > -static int dp_ctrl_enable_mainlink_clocks(struct dp_ctrl_private *ctrl)
> > +static int dp_ctrl_enable_mainlink_clocks(struct dp_ctrl_private *ctrl, unsigned long pixel_rate)
>
>
> I think we can read pixel_rate here rather than getting it as an
> argument. We'd need to move handling (DP_TEST_LINK_PHY_TEST_PATTERN &&
> !ctrl->panel->dp_mode.drm_mode.clock) case here from dp_ctrl_on_link().

This is also called from dp_ctrl_on_stream() and
dp_ctrl_reinitialize_mainlink(). In the dp_ctrl_on_stream() case we may
divide the pixel_rate by 2 with widebus. We could move the
dp_ctrl_on_link() code here, but then we also need to move widebus, and
then I'm not sure which pixel rate to use.

It looks like the test code doesn't care about widebus? And similarly,
we may run the pixel clk faster until we get a modeset and then divide
it for widebus. Is that why you're suggesting to check
!ctrl->panel->dp_mode.drm_mode.clock? I hesitate because it isn't a
direct conversion, instead it checks some other stashed struct member.

I'll also note that dp_ctrl_enable_mainlink_clocks() doesn't really use
this argument except to print the value in drm_dbg_dp(). Maybe we should
simply remove it from here instead?

> > @@ -1588,12 +1586,12 @@ static int dp_ctrl_on_stream_phy_test_report(struct dp_ctrl *dp_ctrl)
> >   {
> >       int ret;
> >       struct dp_ctrl_private *ctrl;
> > +     unsigned long pixel_rate;
> >
> >       ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
> >
> > -     ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
> > -
> > -     ret = dp_ctrl_enable_stream_clocks(ctrl);
> > +     pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
> > +     ret = dp_ctrl_enable_stream_clocks(ctrl, pixel_rate);
>
> I think we can take another step forward here. Read the
> ctrl->panel->dp_mode.drm_mode.clock from within the
> dp_ctrl_enable_stream_clocks() function. This removes the need to pass
> pixel_rate as an argument here.

This is also affected by widebus and if the function is called from
dp_ctrl_on_stream() or dp_ctrl_on_stream_phy_test_report(). Maybe it
would be better to inline dp_ctrl_enable_stream_clocks() to the
callsites? That would probably simplify things because the function is
mostly a wrapper around a couple functions.

Re: [PATCH 2/3] drm/msm/dp: Remove pixel_rate from struct dp_ctrl

[Dmitry Baryshkov]

On 22/06/2022 05:59, Stephen Boyd wrote:
> Quoting Dmitry Baryshkov (2022-06-17 16:07:58)
>> On 17/06/2022 23:47, Stephen Boyd wrote:
>>> This struct member is stored to in the function that calls the function
>>> which uses it. That's possible with a function argument instead of
>>> storing to a struct member. Pass the pixel_rate as an argument instead
>>> to simplify the code. Note that dp_ctrl_link_maintenance() was storing
>>> the pixel_rate but never using it so we just remove the assignment from
>>> there.
>>>
>>> Cc: Kuogee Hsieh <quic_khsieh@quicinc.com>
>>> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
>>> ---
>>>    drivers/gpu/drm/msm/dp/dp_ctrl.c | 57 ++++++++++++++++----------------
>>>    drivers/gpu/drm/msm/dp/dp_ctrl.h |  1 -
>>>    2 files changed, 28 insertions(+), 30 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>>> index bd445e683cfc..e114521af2e9 100644
>>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>>> @@ -1336,7 +1336,7 @@ static void dp_ctrl_set_clock_rate(struct dp_ctrl_private *ctrl,
>>>                                name, rate);
>>>    }
>>>
>>> -static int dp_ctrl_enable_mainlink_clocks(struct dp_ctrl_private *ctrl)
>>> +static int dp_ctrl_enable_mainlink_clocks(struct dp_ctrl_private *ctrl, unsigned long pixel_rate)
>>
>>
>> I think we can read pixel_rate here rather than getting it as an
>> argument. We'd need to move handling (DP_TEST_LINK_PHY_TEST_PATTERN &&
>> !ctrl->panel->dp_mode.drm_mode.clock) case here from dp_ctrl_on_link().
> 
> This is also called from dp_ctrl_on_stream() and
> dp_ctrl_reinitialize_mainlink(). In the dp_ctrl_on_stream() case we may
> divide the pixel_rate by 2 with widebus. We could move the
> dp_ctrl_on_link() code here, but then we also need to move widebus, and
> then I'm not sure which pixel rate to use.
> 
> It looks like the test code doesn't care about widebus? And similarly,
> we may run the pixel clk faster until we get a modeset and then divide
> it for widebus.

Good question. I'll let Kuogee or somebody else from Qualcomm to comment 
on test code vs widebus vs pixel rate, as I don't know these details.

I'm not sure if we should halve the pixel clock in 
dp_ctrl_on_stream_phy_test_report() or not if the widebus is supported.
 From the current code I'd assume that we have to do this. Let's raise 
this question in the corresponding patch discussion.

> Is that why you're suggesting to check
> !ctrl->panel->dp_mode.drm_mode.clock? I hesitate because it isn't a
> direct conversion, instead it checks some other stashed struct member.
> 
> I'll also note that dp_ctrl_enable_mainlink_clocks() doesn't really use
> this argument except to print the value in drm_dbg_dp(). Maybe we should
> simply remove it from here instead?

Yes, do it please.

> 
>>> @@ -1588,12 +1586,12 @@ static int dp_ctrl_on_stream_phy_test_report(struct dp_ctrl *dp_ctrl)
>>>    {
>>>        int ret;
>>>        struct dp_ctrl_private *ctrl;
>>> +     unsigned long pixel_rate;
>>>
>>>        ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
>>>
>>> -     ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
>>> -
>>> -     ret = dp_ctrl_enable_stream_clocks(ctrl);
>>> +     pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
>>> +     ret = dp_ctrl_enable_stream_clocks(ctrl, pixel_rate);
>>
>> I think we can take another step forward here. Read the
>> ctrl->panel->dp_mode.drm_mode.clock from within the
>> dp_ctrl_enable_stream_clocks() function. This removes the need to pass
>> pixel_rate as an argument here.
> 
> This is also affected by widebus and if the function is called from
> dp_ctrl_on_stream() or dp_ctrl_on_stream_phy_test_report(). Maybe it
> would be better to inline dp_ctrl_enable_stream_clocks() to the
> callsites? That would probably simplify things because the function is
> mostly a wrapper around a couple functions.

Yes, this sounds good. Then we can drop the drm_dbg_dp from it (as it 
nearly duplicates the data that was just printed.


-- 
With best wishes
Dmitry

Re: [PATCH 2/3] drm/msm/dp: Remove pixel_rate from struct dp_ctrl

[Dmitry Baryshkov]

On 22/06/2022 05:59, Stephen Boyd wrote:
> Quoting Dmitry Baryshkov (2022-06-17 16:07:58)
>> On 17/06/2022 23:47, Stephen Boyd wrote:
>>> This struct member is stored to in the function that calls the function
>>> which uses it. That's possible with a function argument instead of
>>> storing to a struct member. Pass the pixel_rate as an argument instead
>>> to simplify the code. Note that dp_ctrl_link_maintenance() was storing
>>> the pixel_rate but never using it so we just remove the assignment from
>>> there.
>>>
>>> Cc: Kuogee Hsieh <quic_khsieh@quicinc.com>
>>> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
>>> ---
>>>    drivers/gpu/drm/msm/dp/dp_ctrl.c | 57 ++++++++++++++++----------------
>>>    drivers/gpu/drm/msm/dp/dp_ctrl.h |  1 -
>>>    2 files changed, 28 insertions(+), 30 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>>> index bd445e683cfc..e114521af2e9 100644
>>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>>> @@ -1336,7 +1336,7 @@ static void dp_ctrl_set_clock_rate(struct dp_ctrl_private *ctrl,
>>>                                name, rate);
>>>    }
>>>
>>> -static int dp_ctrl_enable_mainlink_clocks(struct dp_ctrl_private *ctrl)
>>> +static int dp_ctrl_enable_mainlink_clocks(struct dp_ctrl_private *ctrl, unsigned long pixel_rate)
>>
>>
>> I think we can read pixel_rate here rather than getting it as an
>> argument. We'd need to move handling (DP_TEST_LINK_PHY_TEST_PATTERN &&
>> !ctrl->panel->dp_mode.drm_mode.clock) case here from dp_ctrl_on_link().
> 
> This is also called from dp_ctrl_on_stream() and
> dp_ctrl_reinitialize_mainlink(). In the dp_ctrl_on_stream() case we may
> divide the pixel_rate by 2 with widebus. We could move the
> dp_ctrl_on_link() code here, but then we also need to move widebus, and
> then I'm not sure which pixel rate to use.
> 
> It looks like the test code doesn't care about widebus? And similarly,
> we may run the pixel clk faster until we get a modeset and then divide
> it for widebus.

Good question. I'll let Kuogee or somebody else from Qualcomm to comment 
on test code vs widebus vs pixel rate, as I don't know these details.

I'm not sure if we should halve the pixel clock in 
dp_ctrl_on_stream_phy_test_report() or not if the widebus is supported.
 From the current code I'd assume that we have to do this. Let's raise 
this question in the corresponding patch discussion.

> Is that why you're suggesting to check
> !ctrl->panel->dp_mode.drm_mode.clock? I hesitate because it isn't a
> direct conversion, instead it checks some other stashed struct member.
> 
> I'll also note that dp_ctrl_enable_mainlink_clocks() doesn't really use
> this argument except to print the value in drm_dbg_dp(). Maybe we should
> simply remove it from here instead?

Yes, do it please.

> 
>>> @@ -1588,12 +1586,12 @@ static int dp_ctrl_on_stream_phy_test_report(struct dp_ctrl *dp_ctrl)
>>>    {
>>>        int ret;
>>>        struct dp_ctrl_private *ctrl;
>>> +     unsigned long pixel_rate;
>>>
>>>        ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
>>>
>>> -     ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
>>> -
>>> -     ret = dp_ctrl_enable_stream_clocks(ctrl);
>>> +     pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
>>> +     ret = dp_ctrl_enable_stream_clocks(ctrl, pixel_rate);
>>
>> I think we can take another step forward here. Read the
>> ctrl->panel->dp_mode.drm_mode.clock from within the
>> dp_ctrl_enable_stream_clocks() function. This removes the need to pass
>> pixel_rate as an argument here.
> 
> This is also affected by widebus and if the function is called from
> dp_ctrl_on_stream() or dp_ctrl_on_stream_phy_test_report(). Maybe it
> would be better to inline dp_ctrl_enable_stream_clocks() to the
> callsites? That would probably simplify things because the function is
> mostly a wrapper around a couple functions.

Yes, this sounds good. Then we can drop the drm_dbg_dp from it (as it 
nearly duplicates the data that was just printed.


-- 
With best wishes
Dmitry

Re: [PATCH 2/3] drm/msm/dp: Remove pixel_rate from struct dp_ctrl

[Kuogee Hsieh]

On 6/22/2022 12:24 AM, Dmitry Baryshkov wrote:
> On 22/06/2022 05:59, Stephen Boyd wrote:
>> Quoting Dmitry Baryshkov (2022-06-17 16:07:58)
>>> On 17/06/2022 23:47, Stephen Boyd wrote:
>>>> This struct member is stored to in the function that calls the 
>>>> function
>>>> which uses it. That's possible with a function argument instead of
>>>> storing to a struct member. Pass the pixel_rate as an argument instead
>>>> to simplify the code. Note that dp_ctrl_link_maintenance() was storing
>>>> the pixel_rate but never using it so we just remove the assignment 
>>>> from
>>>> there.
>>>>
>>>> Cc: Kuogee Hsieh <quic_khsieh@quicinc.com>
>>>> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
>>>> ---
>>>>    drivers/gpu/drm/msm/dp/dp_ctrl.c | 57 
>>>> ++++++++++++++++----------------
>>>>    drivers/gpu/drm/msm/dp/dp_ctrl.h |  1 -
>>>>    2 files changed, 28 insertions(+), 30 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c 
>>>> b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>>>> index bd445e683cfc..e114521af2e9 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>>>> @@ -1336,7 +1336,7 @@ static void dp_ctrl_set_clock_rate(struct 
>>>> dp_ctrl_private *ctrl,
>>>>                                name, rate);
>>>>    }
>>>>
>>>> -static int dp_ctrl_enable_mainlink_clocks(struct dp_ctrl_private 
>>>> *ctrl)
>>>> +static int dp_ctrl_enable_mainlink_clocks(struct dp_ctrl_private 
>>>> *ctrl, unsigned long pixel_rate)
>>>
>>>
>>> I think we can read pixel_rate here rather than getting it as an
>>> argument. We'd need to move handling (DP_TEST_LINK_PHY_TEST_PATTERN &&
>>> !ctrl->panel->dp_mode.drm_mode.clock) case here from dp_ctrl_on_link().
>>
>> This is also called from dp_ctrl_on_stream() and
>> dp_ctrl_reinitialize_mainlink(). In the dp_ctrl_on_stream() case we may
>> divide the pixel_rate by 2 with widebus. We could move the
>> dp_ctrl_on_link() code here, but then we also need to move widebus, and
>> then I'm not sure which pixel rate to use.
>>
>> It looks like the test code doesn't care about widebus? And similarly,
>> we may run the pixel clk faster until we get a modeset and then divide
>> it for widebus.
>
> Good question. I'll let Kuogee or somebody else from Qualcomm to 
> comment on test code vs widebus vs pixel rate, as I don't know these 
> details.
>
> I'm not sure if we should halve the pixel clock in 
> dp_ctrl_on_stream_phy_test_report() or not if the widebus is supported.
> From the current code I'd assume that we have to do this. Let's raise 
> this question in the corresponding patch discussion.
>
yes, phy test does not care pixel clock rate.
>> Is that why you're suggesting to check
>> !ctrl->panel->dp_mode.drm_mode.clock? I hesitate because it isn't a
>> direct conversion, instead it checks some other stashed struct member.
>>
>> I'll also note that dp_ctrl_enable_mainlink_clocks() doesn't really use
>> this argument except to print the value in drm_dbg_dp(). Maybe we should
>> simply remove it from here instead?
>
> Yes, do it please.
>
>>
>>>> @@ -1588,12 +1586,12 @@ static int 
>>>> dp_ctrl_on_stream_phy_test_report(struct dp_ctrl *dp_ctrl)
>>>>    {
>>>>        int ret;
>>>>        struct dp_ctrl_private *ctrl;
>>>> +     unsigned long pixel_rate;
>>>>
>>>>        ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
>>>>
>>>> -     ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
>>>> -
>>>> -     ret = dp_ctrl_enable_stream_clocks(ctrl);
>>>> +     pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
>>>> +     ret = dp_ctrl_enable_stream_clocks(ctrl, pixel_rate);
>>>
>>> I think we can take another step forward here. Read the
>>> ctrl->panel->dp_mode.drm_mode.clock from within the
>>> dp_ctrl_enable_stream_clocks() function. This removes the need to pass
>>> pixel_rate as an argument here.
>>
>> This is also affected by widebus and if the function is called from
>> dp_ctrl_on_stream() or dp_ctrl_on_stream_phy_test_report(). Maybe it
>> would be better to inline dp_ctrl_enable_stream_clocks() to the
>> callsites? That would probably simplify things because the function is
>> mostly a wrapper around a couple functions.
>
> Yes, this sounds good. Then we can drop the drm_dbg_dp from it (as it 
> nearly duplicates the data that was just printed.
>
>

Re: [PATCH 2/3] drm/msm/dp: Remove pixel_rate from struct dp_ctrl

[Kuogee Hsieh]

On 6/22/2022 12:24 AM, Dmitry Baryshkov wrote:
> On 22/06/2022 05:59, Stephen Boyd wrote:
>> Quoting Dmitry Baryshkov (2022-06-17 16:07:58)
>>> On 17/06/2022 23:47, Stephen Boyd wrote:
>>>> This struct member is stored to in the function that calls the 
>>>> function
>>>> which uses it. That's possible with a function argument instead of
>>>> storing to a struct member. Pass the pixel_rate as an argument instead
>>>> to simplify the code. Note that dp_ctrl_link_maintenance() was storing
>>>> the pixel_rate but never using it so we just remove the assignment 
>>>> from
>>>> there.
>>>>
>>>> Cc: Kuogee Hsieh <quic_khsieh@quicinc.com>
>>>> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
>>>> ---
>>>>    drivers/gpu/drm/msm/dp/dp_ctrl.c | 57 
>>>> ++++++++++++++++----------------
>>>>    drivers/gpu/drm/msm/dp/dp_ctrl.h |  1 -
>>>>    2 files changed, 28 insertions(+), 30 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c 
>>>> b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>>>> index bd445e683cfc..e114521af2e9 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>>>> @@ -1336,7 +1336,7 @@ static void dp_ctrl_set_clock_rate(struct 
>>>> dp_ctrl_private *ctrl,
>>>>                                name, rate);
>>>>    }
>>>>
>>>> -static int dp_ctrl_enable_mainlink_clocks(struct dp_ctrl_private 
>>>> *ctrl)
>>>> +static int dp_ctrl_enable_mainlink_clocks(struct dp_ctrl_private 
>>>> *ctrl, unsigned long pixel_rate)
>>>
>>>
>>> I think we can read pixel_rate here rather than getting it as an
>>> argument. We'd need to move handling (DP_TEST_LINK_PHY_TEST_PATTERN &&
>>> !ctrl->panel->dp_mode.drm_mode.clock) case here from dp_ctrl_on_link().
>>
>> This is also called from dp_ctrl_on_stream() and
>> dp_ctrl_reinitialize_mainlink(). In the dp_ctrl_on_stream() case we may
>> divide the pixel_rate by 2 with widebus. We could move the
>> dp_ctrl_on_link() code here, but then we also need to move widebus, and
>> then I'm not sure which pixel rate to use.
>>
>> It looks like the test code doesn't care about widebus? And similarly,
>> we may run the pixel clk faster until we get a modeset and then divide
>> it for widebus.
>
> Good question. I'll let Kuogee or somebody else from Qualcomm to 
> comment on test code vs widebus vs pixel rate, as I don't know these 
> details.
>
> I'm not sure if we should halve the pixel clock in 
> dp_ctrl_on_stream_phy_test_report() or not if the widebus is supported.
> From the current code I'd assume that we have to do this. Let's raise 
> this question in the corresponding patch discussion.
>
yes, phy test does not care pixel clock rate.
>> Is that why you're suggesting to check
>> !ctrl->panel->dp_mode.drm_mode.clock? I hesitate because it isn't a
>> direct conversion, instead it checks some other stashed struct member.
>>
>> I'll also note that dp_ctrl_enable_mainlink_clocks() doesn't really use
>> this argument except to print the value in drm_dbg_dp(). Maybe we should
>> simply remove it from here instead?
>
> Yes, do it please.
>
>>
>>>> @@ -1588,12 +1586,12 @@ static int 
>>>> dp_ctrl_on_stream_phy_test_report(struct dp_ctrl *dp_ctrl)
>>>>    {
>>>>        int ret;
>>>>        struct dp_ctrl_private *ctrl;
>>>> +     unsigned long pixel_rate;
>>>>
>>>>        ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
>>>>
>>>> -     ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
>>>> -
>>>> -     ret = dp_ctrl_enable_stream_clocks(ctrl);
>>>> +     pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
>>>> +     ret = dp_ctrl_enable_stream_clocks(ctrl, pixel_rate);
>>>
>>> I think we can take another step forward here. Read the
>>> ctrl->panel->dp_mode.drm_mode.clock from within the
>>> dp_ctrl_enable_stream_clocks() function. This removes the need to pass
>>> pixel_rate as an argument here.
>>
>> This is also affected by widebus and if the function is called from
>> dp_ctrl_on_stream() or dp_ctrl_on_stream_phy_test_report(). Maybe it
>> would be better to inline dp_ctrl_enable_stream_clocks() to the
>> callsites? That would probably simplify things because the function is
>> mostly a wrapper around a couple functions.
>
> Yes, this sounds good. Then we can drop the drm_dbg_dp from it (as it 
> nearly duplicates the data that was just printed.
>
>

Re: [PATCH 2/3] drm/msm/dp: Remove pixel_rate from struct dp_ctrl

[Dmitry Baryshkov]

On 22/06/2022 18:22, Kuogee Hsieh wrote:
> 
> On 6/22/2022 12:24 AM, Dmitry Baryshkov wrote:
>> On 22/06/2022 05:59, Stephen Boyd wrote:
>>> Quoting Dmitry Baryshkov (2022-06-17 16:07:58)
>>>> On 17/06/2022 23:47, Stephen Boyd wrote:
>>>>> This struct member is stored to in the function that calls the 
>>>>> function
>>>>> which uses it. That's possible with a function argument instead of
>>>>> storing to a struct member. Pass the pixel_rate as an argument instead
>>>>> to simplify the code. Note that dp_ctrl_link_maintenance() was storing
>>>>> the pixel_rate but never using it so we just remove the assignment 
>>>>> from
>>>>> there.
>>>>>
>>>>> Cc: Kuogee Hsieh <quic_khsieh@quicinc.com>
>>>>> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
>>>>> ---
>>>>>    drivers/gpu/drm/msm/dp/dp_ctrl.c | 57 
>>>>> ++++++++++++++++----------------
>>>>>    drivers/gpu/drm/msm/dp/dp_ctrl.h |  1 -
>>>>>    2 files changed, 28 insertions(+), 30 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c 
>>>>> b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>>>>> index bd445e683cfc..e114521af2e9 100644
>>>>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>>>>> @@ -1336,7 +1336,7 @@ static void dp_ctrl_set_clock_rate(struct 
>>>>> dp_ctrl_private *ctrl,
>>>>>                                name, rate);
>>>>>    }
>>>>>
>>>>> -static int dp_ctrl_enable_mainlink_clocks(struct dp_ctrl_private 
>>>>> *ctrl)
>>>>> +static int dp_ctrl_enable_mainlink_clocks(struct dp_ctrl_private 
>>>>> *ctrl, unsigned long pixel_rate)
>>>>
>>>>
>>>> I think we can read pixel_rate here rather than getting it as an
>>>> argument. We'd need to move handling (DP_TEST_LINK_PHY_TEST_PATTERN &&
>>>> !ctrl->panel->dp_mode.drm_mode.clock) case here from dp_ctrl_on_link().
>>>
>>> This is also called from dp_ctrl_on_stream() and
>>> dp_ctrl_reinitialize_mainlink(). In the dp_ctrl_on_stream() case we may
>>> divide the pixel_rate by 2 with widebus. We could move the
>>> dp_ctrl_on_link() code here, but then we also need to move widebus, and
>>> then I'm not sure which pixel rate to use.
>>>
>>> It looks like the test code doesn't care about widebus? And similarly,
>>> we may run the pixel clk faster until we get a modeset and then divide
>>> it for widebus.
>>
>> Good question. I'll let Kuogee or somebody else from Qualcomm to 
>> comment on test code vs widebus vs pixel rate, as I don't know these 
>> details.
>>
>> I'm not sure if we should halve the pixel clock in 
>> dp_ctrl_on_stream_phy_test_report() or not if the widebus is supported.
>> From the current code I'd assume that we have to do this. Let's raise 
>> this question in the corresponding patch discussion.
>>
> yes, phy test does not care pixel clock rate.

So, is it 'does not care' or 'set to mode clock'?

In other words, can we unify both functions by always accounting for the 
wide_bus_en value?


-- 
With best wishes
Dmitry

Re: [PATCH 2/3] drm/msm/dp: Remove pixel_rate from struct dp_ctrl

[Dmitry Baryshkov]

On 22/06/2022 18:22, Kuogee Hsieh wrote:
> 
> On 6/22/2022 12:24 AM, Dmitry Baryshkov wrote:
>> On 22/06/2022 05:59, Stephen Boyd wrote:
>>> Quoting Dmitry Baryshkov (2022-06-17 16:07:58)
>>>> On 17/06/2022 23:47, Stephen Boyd wrote:
>>>>> This struct member is stored to in the function that calls the 
>>>>> function
>>>>> which uses it. That's possible with a function argument instead of
>>>>> storing to a struct member. Pass the pixel_rate as an argument instead
>>>>> to simplify the code. Note that dp_ctrl_link_maintenance() was storing
>>>>> the pixel_rate but never using it so we just remove the assignment 
>>>>> from
>>>>> there.
>>>>>
>>>>> Cc: Kuogee Hsieh <quic_khsieh@quicinc.com>
>>>>> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
>>>>> ---
>>>>>    drivers/gpu/drm/msm/dp/dp_ctrl.c | 57 
>>>>> ++++++++++++++++----------------
>>>>>    drivers/gpu/drm/msm/dp/dp_ctrl.h |  1 -
>>>>>    2 files changed, 28 insertions(+), 30 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c 
>>>>> b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>>>>> index bd445e683cfc..e114521af2e9 100644
>>>>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>>>>> @@ -1336,7 +1336,7 @@ static void dp_ctrl_set_clock_rate(struct 
>>>>> dp_ctrl_private *ctrl,
>>>>>                                name, rate);
>>>>>    }
>>>>>
>>>>> -static int dp_ctrl_enable_mainlink_clocks(struct dp_ctrl_private 
>>>>> *ctrl)
>>>>> +static int dp_ctrl_enable_mainlink_clocks(struct dp_ctrl_private 
>>>>> *ctrl, unsigned long pixel_rate)
>>>>
>>>>
>>>> I think we can read pixel_rate here rather than getting it as an
>>>> argument. We'd need to move handling (DP_TEST_LINK_PHY_TEST_PATTERN &&
>>>> !ctrl->panel->dp_mode.drm_mode.clock) case here from dp_ctrl_on_link().
>>>
>>> This is also called from dp_ctrl_on_stream() and
>>> dp_ctrl_reinitialize_mainlink(). In the dp_ctrl_on_stream() case we may
>>> divide the pixel_rate by 2 with widebus. We could move the
>>> dp_ctrl_on_link() code here, but then we also need to move widebus, and
>>> then I'm not sure which pixel rate to use.
>>>
>>> It looks like the test code doesn't care about widebus? And similarly,
>>> we may run the pixel clk faster until we get a modeset and then divide
>>> it for widebus.
>>
>> Good question. I'll let Kuogee or somebody else from Qualcomm to 
>> comment on test code vs widebus vs pixel rate, as I don't know these 
>> details.
>>
>> I'm not sure if we should halve the pixel clock in 
>> dp_ctrl_on_stream_phy_test_report() or not if the widebus is supported.
>> From the current code I'd assume that we have to do this. Let's raise 
>> this question in the corresponding patch discussion.
>>
> yes, phy test does not care pixel clock rate.

So, is it 'does not care' or 'set to mode clock'?

In other words, can we unify both functions by always accounting for the 
wide_bus_en value?


-- 
With best wishes
Dmitry