Message-ID: <d77d2989-7270-d1ec-fda6-7001ea337f5b@quicinc.com>
Date:   Wed, 22 Jun 2022 08:22:55 -0700
From:   Kuogee Hsieh <quic_khsieh@...cinc.com>
To:     Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        Stephen Boyd <swboyd@...omium.org>,
        Abhinav Kumar <quic_abhinavk@...cinc.com>,
        Rob Clark <robdclark@...il.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>
Subject: Re: [PATCH 2/3] drm/msm/dp: Remove pixel_rate from struct dp_ctrl


On 6/22/2022 12:24 AM, Dmitry Baryshkov wrote:
> 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@...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().
>>
>> 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.
>
yes, phy test does not care pixel clock rate.
>> 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.
>
>

Powered by blists - more mailing lists