[Freedreno] [PATCH v5 3/5] drm/msm/dp: set stream_pixel rate directly

[Dmitry Baryshkov]

On Fri, 4 Mar 2022 at 01:32, Stephen Boyd <swboyd at 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 at 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
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?

> and I vaguely recall that the DP phy needs to be configured for some
> frequency so that the pixel clk can use it when determining the rate to
> set.
>
> >         else
> >                 DRM_ERROR("%s clock doesn't exit to set rate %lu\n",
> >                                 name, rate);



-- 
With best wishes
Dmitry