| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <bb98ca29-8752-6864-ddbd-19547fb6f73b@linaro.org>
Date: Sat, 18 Jun 2022 02:07:58 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Stephen Boyd <swboyd@...omium.org>,
Rob Clark <robdclark@...il.com>,
Abhinav Kumar <quic_abhinavk@...cinc.com>
Cc: linux-kernel@...r.kernel.org, patches@...ts.linux.dev,
Sean Paul <sean@...rly.run>, dri-devel@...ts.freedesktop.org,
freedreno@...ts.freedesktop.org,
Kuogee Hsieh <quic_khsieh@...cinc.com>
Subject: Re: [PATCH 2/3] drm/msm/dp: Remove pixel_rate from struct dp_ctrl
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@...cinc.com>
> Signed-off-by: Stephen Boyd <swboyd@...omium.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
Powered by blists - more mailing lists