[PATCH 13/14] drm/msm/dp: move next_bridge handling to dp_display

[Kuogee Hsieh]

On 1/24/2024 11:07 AM, Dmitry Baryshkov wrote:
> On Tue, 23 Jan 2024 at 19:31, Kuogee Hsieh <quic_khsieh at quicinc.com> wrote:
>>
>> On 1/22/2024 4:23 PM, Dmitry Baryshkov wrote:
>>> On Tue, 23 Jan 2024 at 01:20, Kuogee Hsieh <quic_khsieh at quicinc.com> wrote:
>>>> On 1/22/2024 9:28 AM, Kuogee Hsieh wrote:
>>>>> On 1/19/2024 6:31 PM, Dmitry Baryshkov wrote:
>>>>>> On Fri, 19 Jan 2024 at 23:14, Kuogee Hsieh <quic_khsieh at quicinc.com>
>>>>>> wrote:
>>>>>>> Dmitry,
>>>>>>>
>>>>>>> I am testing this patch serial with msm-next branch.
>>>>>>>
>>>>>>> This patch cause system crash during booting up for me.
>>>>>>>
>>>>>>> Is this patch work for you?
>>>>>> Yes, tested on top of linux-next. However I only tested it with
>>>>>> DP-over-USBC. What is your testcase? Could you please share the crash
>>>>>> log?
>>>>> I tested it on chrome device (sc7280) which has eDP as primary and
>>>>> without external USBC DP connected.
>>>>>
>>>>> It crashes during boot.
>>>>>
>>>>> I will debug it more and collect logs for you.
>>>>>
>>>> Below  patch work for chrome with both eDP and external DP.
>>>>
>>>> We have to return failed if it is the external DP and return value of
>>>> devm_drm_of_get_bridge()  is !ENODEV since DP does not have next bridge.
>>>>
>>>> Otherwise should continues to component_add()
>>> We also should not continue if it is eDP in case of any error.
>>>
>>> So it is if (is_edp || (!is_edp && ret != -ENODEV)) which is exactly
>>> equivalent to (is_edp || ret != -ENODEV).
>> yes, you are correct.
>>
>> I just found that the real fix of crash is "+ dp->next_bridge = NULL;"
>> since dp->next_bridge will be used at dp_bridge_init() at dp_drm.c later.
>>
>> Therefore it need to be restored to NULL at failed case.
> Ack, this sounds logical.

ok, I will add tested-by and reviewed-by at next upload.

Thanks,

>
>>> Could you please post the backtrace that you have observed?
>> I do not have serial console port to collect logs.
>>
>> If you still need it, i will collect it tomorrow.
> No, I think it is fine now, thank you!
>
>>>> @@ -1210,7 +1210,9 @@ static int dp_display_probe_tail(struct device *dev)
>>>>            dp->next_bridge = devm_drm_of_get_bridge(&dp->pdev->dev,
>>>> dp->pdev->dev.of_node, 1, 0);
>>>>            if (IS_ERR(dp->next_bridge)) {
>>>>                    ret = PTR_ERR(dp->next_bridge);
>>>> -               if (dp->is_edp || ret != -ENODEV)
>>>> +               dp->next_bridge = NULL;
>>>> +
>>>> +               if (!dp->is_edp && ret != -ENODEV)
>>>>                            return ret;
>>>>            }
>>>>>>> On 12/29/2023 2:56 PM, Dmitry Baryshkov wrote:
>>>>>>>> Remove two levels of indirection and fetch next bridge directly in
>>>>>>>> dp_display_probe_tail().
>>>>>>>>
>>>>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
>>>>>>>> ---
>>>>>>>>      drivers/gpu/drm/msm/dp/dp_display.c | 42
>>>>>>>> +++++++++--------------------
>>>>>>>>      drivers/gpu/drm/msm/dp/dp_parser.c  | 14 ----------
>>>>>>>>      drivers/gpu/drm/msm/dp/dp_parser.h  | 14 ----------
>>>>>>>>      3 files changed, 13 insertions(+), 57 deletions(-)
>
>