Re: [PATCH v2 2/3] drm/msm/dsi: Set PHY usescase (and mode) before registering DSI host

[Abhinav Kumar <quic_abhinavk@quicinc.com>]

Hi Marijn

On 2/10/2025 2:17 PM, Abhinav Kumar wrote:
> 
> 
> On 2/10/2025 6:24 AM, Dmitry Baryshkov wrote:
>> On Mon, Feb 10, 2025 at 01:54:29PM +0100, Marijn Suijten wrote:
>>> On 2025-02-10 01:11:59, Dmitry Baryshkov wrote:
>>>> On Sun, Feb 09, 2025 at 10:42:53PM +0100, Marijn Suijten wrote:
>>>>> Ordering issues here cause an uninitialized (default STANDALONE)
>>>>> usecase to be programmed (which appears to be a MUX) in some cases
>>>>> when msm_dsi_host_register() is called, leading to the slave PLL in
>>>>> bonded-DSI mode to source from a clock parent (dsi1vco) that is off.
>>>>>
>>>>> This should seemingly not be a problem as the actual dispcc clocks 
>>>>> from
>>>>> DSI1 that are muxed in the clock tree of DSI0 are way further down, 
>>>>> this
>>>>> bit still seems to have an effect on them somehow and causes the right
>>>>> side of the panel controlled by DSI1 to not function.
>>>>>
>>>>> In an ideal world this code is refactored to no longer have such
>>>>> error-prone calls "across subsystems", and instead model the "PLL src"
>>>>> register field as a regular mux so that changing the clock parents
>>>>> programmatically or in DTS via `assigned-clock-parents` has the
>>>>> desired effect.
>>>>> But for the avid reader, the clocks that we *are* muxing into DSI0's
>>>>> tree are way further down, so if this bit turns out to be a simple mux
>>>>> between dsiXvco and out_div, that shouldn't have any effect as this
>>>>> whole tree is off anyway.
>>>>>
>>>>> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
>>>>> ---
>>>>>   drivers/gpu/drm/msm/dsi/dsi_manager.c | 30 ++++++++++++++++++ 
>>>>> +-----------
>>>>>   1 file changed, 19 insertions(+), 11 deletions(-)
>>>>
>>>>
>>>> Fixes: 57bf43389337 ("drm/msm/dsi: Pass down use case to PHY")
>>>
>>> I'm not exactly confident about that.  Abhinav pointed out in
>>> https://gitlab.freedesktop.org/drm/msm/-/issues/41#note_2375646 that
>>> msm_dsi_host_register() was not supposed to be enabling the PHY, which I
>>> provided a counter-stacktrace for to show that is indeed the case.
>>>
>>> Either this was always a problem that's only become visible now (and 
>>> it's an
>>> issue with that patch), or a different change causes 
>>> msm_dsi_host_register()
>>> to enable the PHY and program the usecase too early?
>>
>> As currently usecase is being programmed after the DSI host being
>> registered, there might be a race condition between panel driver probe
>> _and_ usecase programming.
>>
>>>
>>> What do you think?
>>>
>>> - Marijn
>>>
> 
> Yes I agree with Dmitry's explanation. The race condition between the 
> two can cause this. Hence I am also fine with this change.
> 
>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/ 
>>>>> drm/msm/dsi/dsi_manager.c
>>>>> index 
>>>>> a210b7c9e5ca281a46fbdb226e25832719a684ea..b93205c034e4acc73d536deeddce6ebd694b4a80 100644
>>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>>>> @@ -74,17 +74,33 @@ static int dsi_mgr_setup_components(int id)
>>>>>       int ret;
>>>>>       if (!IS_BONDED_DSI()) {
>>>>> +        /* Set the usecase before calling msm_dsi_host_register(), 
>>>>> which would
>>>>> +         * already program the PLL source mux based on a default 
>>>>> usecase.
>>>>> +         */
>>>>> +        msm_dsi_phy_set_usecase(msm_dsi->phy, 
>>>>> MSM_DSI_PHY_STANDALONE);
>>>>> +        msm_dsi_host_set_phy_mode(msm_dsi->host, msm_dsi->phy);
>>>>> +
>>>>>           ret = msm_dsi_host_register(msm_dsi->host);
>>>>>           if (ret)
>>>>>               return ret;
>>>>> -
>>>>> -        msm_dsi_phy_set_usecase(msm_dsi->phy, 
>>>>> MSM_DSI_PHY_STANDALONE);
>>>>> -        msm_dsi_host_set_phy_mode(msm_dsi->host, msm_dsi->phy);
>>>>>       } else if (other_dsi) {
>>>>>           struct msm_dsi *master_link_dsi = IS_MASTER_DSI_LINK(id) ?
>>>>>                               msm_dsi : other_dsi;
>>>>>           struct msm_dsi *slave_link_dsi = IS_MASTER_DSI_LINK(id) ?
>>>>>                               other_dsi : msm_dsi;
>>>>> +
>>>>> +        /* PLL0 is to drive both DSI link clocks in bonded DSI mode.
>>>>> +         *
>>>>> +        /* Set the usecase before calling msm_dsi_host_register(), 
>>>>> which would
>>>>> +         * already program the PLL source mux based on a default 
>>>>> usecase.
>>>>> +         */

My compiler is throwing a warning here

drivers/gpu/drm/msm/dsi/dsi_manager.c: In function 
'dsi_mgr_setup_components':
drivers/gpu/drm/msm/dsi/dsi_manager.c:94:3: warning: "/*" within comment 
[-Wcomment]
    /* Set the usecase before calling msm_dsi_host_register(), which would
    ^
drivers/gpu/drm/msm/dsi/dsi_manager.c: At top level:

Can you pls address this one so that I can pick this one up?

Thanks

Abhinav