From: Stephen Boyd <swboyd@chromium.org> To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> Cc: Abhinav Kumar <quic_abhinavk@quicinc.com>, Bjorn Andersson <bjorn.andersson@linaro.org>, Rob Clark <robdclark@gmail.com>, Sean Paul <sean@poorly.run>, David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>, linux-arm-msm@vger.kernel.org, dri-devel@lists.freedesktop.org, freedreno@lists.freedesktop.org Subject: Re: [PATCH v5 3/5] drm/msm/dp: set stream_pixel rate directly Date: Thu, 3 Mar 2022 20:31:23 -0800 [thread overview] Message-ID: <CAE-0n50h=REsyLsjNMaMaZtH7Dptowink7Tq0nzmBRYNas9OmQ@mail.gmail.com> (raw) In-Reply-To: <CAA8EJpq4fXHH6GEJO=m3Ckw0A2p7B_X0D3SiXi1xnJ=4VZOC=g@mail.gmail.com> Quoting Dmitry Baryshkov (2022-03-03 20:23:06) > On Fri, 4 Mar 2022 at 01:32, Stephen Boyd <swboyd@chromium.org> wrote: > > > > Quoting Dmitry Baryshkov (2022-02-16 21:55:27) > > > The only clock for which we set the rate is the "stream_pixel". Rather > > > than storing the rate and then setting it by looping over all the > > > clocks, set the clock rate directly. > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > [...] > > > diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c > > > index 07f6bf7e1acb..8e6361dedd77 100644 > > > --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c > > > +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c > > > @@ -1315,7 +1315,7 @@ static void dp_ctrl_set_clock_rate(struct dp_ctrl_private *ctrl, > > > DRM_DEBUG_DP("setting rate=%lu on clk=%s\n", rate, name); > > > > > > if (num) > > > - cfg->rate = rate; > > > + clk_set_rate(cfg->clk, rate); > > > > This looks bad. From what I can tell we set the rate of the pixel clk > > after enabling the phy and configuring it. See the order of operations > > in dp_ctrl_enable_mainlink_clocks() and note how dp_power_clk_enable() > > is the one that eventually sets a rate through dp_power_clk_set_rate() > > > > dp_ctrl_set_clock_rate(ctrl, DP_CTRL_PM, "ctrl_link", > > ctrl->link->link_params.rate * 1000); > > > > phy_configure(phy, &dp_io->phy_opts); > > phy_power_on(phy); > > > > ret = dp_power_clk_enable(ctrl->power, DP_CTRL_PM, true); > > This code has been changed in the previous patch. > > Let's get back a bit. > Currently dp_ctrl_set_clock_rate() doesn't change the clock rate. It > just stores the rate in the config so that later the sequence of > dp_power_clk_enable() -> dp_power_clk_set_rate() -> > [dp_power_clk_set_link_rate() -> dev_pm_opp_set_rate() or > msm_dss_clk_set_rate() -> clk_set_rate()] will use that. > > There are only two users of dp_ctrl_set_clock_rate(): > - dp_ctrl_enable_mainlink_clocks(), which you have quoted above. > This case is handled in the patch 1 from this series. It makes Patch 1 form this series says DP is unaffected. Huh? > dp_ctrl_enable_mainlink_clocks() call dev_pm_opp_set_rate() directly > without storing (!) the rate in the config, calling > phy_configure()/phy_power_on() and then setting the opp via the > sequence of calls specified above > > - dp_ctrl_enable_stream_clocks(), which calls dp_power_clk_enable() > immediately afterwards. This call would set the stream_pixel rate > while enabling stream clocks. As far as I can see, the stream_pixel is > the only stream clock. So this patch sets the clock rate without > storing in the interim configuration data. > > Could you please clarify, what exactly looks bad to you? > I'm concerned about the order of operations changing between the phy being powered on and the pixel clk frequency being set. From what I recall the pixel clk rate operations depend on the phy frequency being set (which is done through phy_configure?) so if we call clk_set_rate() on the pixel clk before the phy is set then the clk frequency will be calculated badly and probably be incorrect.
WARNING: multiple messages have this Message-ID (diff)
From: Stephen Boyd <swboyd@chromium.org> To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> Cc: freedreno@lists.freedesktop.org, David Airlie <airlied@linux.ie>, linux-arm-msm@vger.kernel.org, Abhinav Kumar <quic_abhinavk@quicinc.com>, dri-devel@lists.freedesktop.org, Bjorn Andersson <bjorn.andersson@linaro.org>, Sean Paul <sean@poorly.run> Subject: Re: [PATCH v5 3/5] drm/msm/dp: set stream_pixel rate directly Date: Thu, 3 Mar 2022 20:31:23 -0800 [thread overview] Message-ID: <CAE-0n50h=REsyLsjNMaMaZtH7Dptowink7Tq0nzmBRYNas9OmQ@mail.gmail.com> (raw) In-Reply-To: <CAA8EJpq4fXHH6GEJO=m3Ckw0A2p7B_X0D3SiXi1xnJ=4VZOC=g@mail.gmail.com> Quoting Dmitry Baryshkov (2022-03-03 20:23:06) > On Fri, 4 Mar 2022 at 01:32, Stephen Boyd <swboyd@chromium.org> wrote: > > > > Quoting Dmitry Baryshkov (2022-02-16 21:55:27) > > > The only clock for which we set the rate is the "stream_pixel". Rather > > > than storing the rate and then setting it by looping over all the > > > clocks, set the clock rate directly. > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > [...] > > > diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c > > > index 07f6bf7e1acb..8e6361dedd77 100644 > > > --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c > > > +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c > > > @@ -1315,7 +1315,7 @@ static void dp_ctrl_set_clock_rate(struct dp_ctrl_private *ctrl, > > > DRM_DEBUG_DP("setting rate=%lu on clk=%s\n", rate, name); > > > > > > if (num) > > > - cfg->rate = rate; > > > + clk_set_rate(cfg->clk, rate); > > > > This looks bad. From what I can tell we set the rate of the pixel clk > > after enabling the phy and configuring it. See the order of operations > > in dp_ctrl_enable_mainlink_clocks() and note how dp_power_clk_enable() > > is the one that eventually sets a rate through dp_power_clk_set_rate() > > > > dp_ctrl_set_clock_rate(ctrl, DP_CTRL_PM, "ctrl_link", > > ctrl->link->link_params.rate * 1000); > > > > phy_configure(phy, &dp_io->phy_opts); > > phy_power_on(phy); > > > > ret = dp_power_clk_enable(ctrl->power, DP_CTRL_PM, true); > > This code has been changed in the previous patch. > > Let's get back a bit. > Currently dp_ctrl_set_clock_rate() doesn't change the clock rate. It > just stores the rate in the config so that later the sequence of > dp_power_clk_enable() -> dp_power_clk_set_rate() -> > [dp_power_clk_set_link_rate() -> dev_pm_opp_set_rate() or > msm_dss_clk_set_rate() -> clk_set_rate()] will use that. > > There are only two users of dp_ctrl_set_clock_rate(): > - dp_ctrl_enable_mainlink_clocks(), which you have quoted above. > This case is handled in the patch 1 from this series. It makes Patch 1 form this series says DP is unaffected. Huh? > dp_ctrl_enable_mainlink_clocks() call dev_pm_opp_set_rate() directly > without storing (!) the rate in the config, calling > phy_configure()/phy_power_on() and then setting the opp via the > sequence of calls specified above > > - dp_ctrl_enable_stream_clocks(), which calls dp_power_clk_enable() > immediately afterwards. This call would set the stream_pixel rate > while enabling stream clocks. As far as I can see, the stream_pixel is > the only stream clock. So this patch sets the clock rate without > storing in the interim configuration data. > > Could you please clarify, what exactly looks bad to you? > I'm concerned about the order of operations changing between the phy being powered on and the pixel clk frequency being set. From what I recall the pixel clk rate operations depend on the phy frequency being set (which is done through phy_configure?) so if we call clk_set_rate() on the pixel clk before the phy is set then the clk frequency will be calculated badly and probably be incorrect.
next prev parent reply other threads:[~2022-03-04 4:31 UTC|newest] Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-02-17 5:55 [PATCH v5 0/5] drm/msm: rework clock handling Dmitry Baryshkov 2022-02-17 5:55 ` Dmitry Baryshkov 2022-02-17 5:55 ` [PATCH v5 1/5] drm/msm/dpu: simplify clocks handling Dmitry Baryshkov 2022-02-17 5:55 ` Dmitry Baryshkov 2022-03-03 22:24 ` Stephen Boyd 2022-03-03 22:24 ` Stephen Boyd 2022-02-17 5:55 ` [PATCH v5 2/5] drm/msm/dp: "inline" dp_ctrl_set_clock_rate("ctrl_link") Dmitry Baryshkov 2022-02-17 5:55 ` Dmitry Baryshkov 2022-03-03 22:25 ` Stephen Boyd 2022-03-03 22:25 ` Stephen Boyd 2022-02-17 5:55 ` [PATCH v5 3/5] drm/msm/dp: set stream_pixel rate directly Dmitry Baryshkov 2022-02-17 5:55 ` Dmitry Baryshkov 2022-03-03 22:32 ` Stephen Boyd 2022-03-03 22:32 ` Stephen Boyd 2022-03-04 4:23 ` Dmitry Baryshkov 2022-03-04 4:23 ` Dmitry Baryshkov 2022-03-04 4:31 ` Stephen Boyd [this message] 2022-03-04 4:31 ` Stephen Boyd 2022-03-04 7:58 ` Dmitry Baryshkov 2022-03-04 7:58 ` Dmitry Baryshkov 2022-03-08 20:46 ` Stephen Boyd 2022-03-08 20:46 ` Stephen Boyd 2022-04-19 16:34 ` Dmitry Baryshkov 2022-04-19 16:34 ` Dmitry Baryshkov 2022-04-28 21:49 ` Stephen Boyd 2022-04-28 21:49 ` Stephen Boyd 2022-04-28 21:51 ` Dmitry Baryshkov 2022-04-28 21:51 ` Dmitry Baryshkov 2022-04-29 1:20 ` Stephen Boyd 2022-04-29 1:20 ` Stephen Boyd 2022-04-29 9:44 ` Dmitry Baryshkov 2022-04-29 9:44 ` Dmitry Baryshkov 2022-02-17 5:55 ` [PATCH v5 4/5] drm/msm/dp: inline dp_power_clk_set_rate() Dmitry Baryshkov 2022-02-17 5:55 ` Dmitry Baryshkov 2022-03-03 22:32 ` Stephen Boyd 2022-03-03 22:32 ` Stephen Boyd 2022-02-17 5:55 ` [PATCH v5 5/5] drm/msm/dp: rewrite dss_module_power to use bulk clock functions Dmitry Baryshkov 2022-02-17 5:55 ` Dmitry Baryshkov 2022-03-03 22:33 ` Stephen Boyd 2022-03-03 22:33 ` Stephen Boyd
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='CAE-0n50h=REsyLsjNMaMaZtH7Dptowink7Tq0nzmBRYNas9OmQ@mail.gmail.com' \ --to=swboyd@chromium.org \ --cc=airlied@linux.ie \ --cc=bjorn.andersson@linaro.org \ --cc=daniel@ffwll.ch \ --cc=dmitry.baryshkov@linaro.org \ --cc=dri-devel@lists.freedesktop.org \ --cc=freedreno@lists.freedesktop.org \ --cc=linux-arm-msm@vger.kernel.org \ --cc=quic_abhinavk@quicinc.com \ --cc=robdclark@gmail.com \ --cc=sean@poorly.run \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.