Re: [PATCH 2/3] drm/msm/dp: Remove pixel_rate from struct dp_ctrl

From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
To: Stephen Boyd <swboyd@chromium.org>,
	Abhinav Kumar <quic_abhinavk@quicinc.com>,
	Rob Clark <robdclark@gmail.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: Wed, 22 Jun 2022 10:24:54 +0300	[thread overview]
Message-ID: <48d83380-edb1-ad61-3878-5fa3ac3e5169@linaro.org> (raw)
In-Reply-To: <CAE-0n51_zysbkktVEfhvXtGqpADTWcaPBAX7A7rD1FV+vcK3Uw@mail.gmail.com>

On 22/06/2022 05:59, Stephen Boyd wrote:
> 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@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().
> 
> 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.

Good question. I'll let Kuogee or somebody else from Qualcomm to comment 
on test code vs widebus vs pixel rate, as I don't know these details.

I'm not sure if we should halve the pixel clock in 
dp_ctrl_on_stream_phy_test_report() or not if the widebus is supported.
 From the current code I'd assume that we have to do this. Let's raise 
this question in the corresponding patch discussion.

> 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?

Yes, do it please.

> 
>>> @@ -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.

Yes, this sounds good. Then we can drop the drm_dbg_dp from it (as it 
nearly duplicates the data that was just printed.

-- 
With best wishes
Dmitry