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

[Dmitry Baryshkov]

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.

> > 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(-)



-- 
With best wishes
Dmitry