From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
To: Stephen Boyd <swboyd@chromium.org>,
Rob Clark <robdclark@gmail.com>,
Abhinav Kumar <quic_abhinavk@quicinc.com>
Cc: linux-kernel@vger.kernel.org, patches@lists.linux.dev,
Sean Paul <sean@poorly.run>,
dri-devel@lists.freedesktop.org, freedreno@lists.freedesktop.org,
Kuogee Hsieh <quic_khsieh@quicinc.com>
Subject: Re: [PATCH 2/3] drm/msm/dp: Remove pixel_rate from struct dp_ctrl
Date: Sat, 18 Jun 2022 02:07:58 +0300 [thread overview]
Message-ID: <bb98ca29-8752-6864-ddbd-19547fb6f73b@linaro.org> (raw)
In-Reply-To: <20220617204750.2347797-3-swboyd@chromium.org>
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
next prev parent reply other threads:[~2022-06-17 23:08 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-17 20:47 [PATCH 0/3] drm/msm/dp: More cleanups for force link train Stephen Boyd
2022-06-17 20:47 ` [PATCH 1/3] drm/msm/dp: Reorganize code to avoid forward declaration Stephen Boyd
2022-06-17 22:50 ` Dmitry Baryshkov
2022-06-20 15:23 ` Kuogee Hsieh
2022-06-17 20:47 ` [PATCH 2/3] drm/msm/dp: Remove pixel_rate from struct dp_ctrl Stephen Boyd
2022-06-17 23:07 ` Dmitry Baryshkov [this message]
2022-06-22 2:59 ` Stephen Boyd
2022-06-22 7:24 ` Dmitry Baryshkov
2022-06-22 15:22 ` Kuogee Hsieh
2022-06-22 17:58 ` Dmitry Baryshkov
2022-06-17 20:47 ` [PATCH 3/3] drm/msm/dp: Get rid of dp_ctrl_on_stream_phy_test_report() Stephen Boyd
2022-06-20 15:23 ` Kuogee Hsieh
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=bb98ca29-8752-6864-ddbd-19547fb6f73b@linaro.org \
--to=dmitry.baryshkov@linaro.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=freedreno@lists.freedesktop.org \
--cc=linux-kernel@vger.kernel.org \
--cc=patches@lists.linux.dev \
--cc=quic_abhinavk@quicinc.com \
--cc=quic_khsieh@quicinc.com \
--cc=robdclark@gmail.com \
--cc=sean@poorly.run \
--cc=swboyd@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).