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@xxxxxxxxxxx>
> > Signed-off-by: Stephen Boyd <swboyd@xxxxxxxxxxxx>
> > ---
> >   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.