Re: [Freedreno] [RFC PATCH v2 1/5] drm/msm/dp: fix panel bridge attachment

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, 25 Feb 2022 at 20:11, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
>
>
>
> On 2/25/2022 1:04 AM, Dmitry Baryshkov wrote:
> > On Fri, 25 Feb 2022 at 07:45, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
> >>
> >>
> >>
> >> On 2/24/2022 8:22 PM, Dmitry Baryshkov wrote:
> >>> On Fri, 25 Feb 2022 at 05:01, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 2/24/2022 12:41 PM, Dmitry Baryshkov wrote:
> >>>>> On Thu, 24 Feb 2022 at 21:25, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> On 2/18/2022 6:26 PM, Dmitry Baryshkov wrote:
> >>>>>>> On 19/02/2022 02:56, Stephen Boyd wrote:
> >>>>>>>> Quoting Dmitry Baryshkov (2022-02-11 14:40:02)
> >>>>>>>>> In commit 8a3b4c17f863 ("drm/msm/dp: employ bridge mechanism for display
> >>>>>>>>> enable and disable") the DP driver received a drm_bridge instance, which
> >>>>>>>>> is always attached to the encoder as a root bridge. However it conflicts
> >>>>>>>>> with the panel_bridge support for eDP panels. The panel bridge attaches
> >>>>>>>>> to the encoder before the "dp" bridge has a chace to do so. Change
> >>>>>>>>
> >>>>>>>> s/chace/chance/
> >>>>>>>>
> >>>>>>>>> panel_bridge attachment to come after dp_bridge attachment.
> >>>>>>>>
> >>>>>>>> s/panel_bridge/panel bridge/ possibly? And maybe clarify that dp_bridge
> >>>>>>>> is the "DP driver's drm_bridge instance created in
> >>>>>>>> msm_dp_bridge_init()"?
> >>>>>>>>
> >>>>>>>> My understanding is that we want to pass the bridge created in
> >>>>>>>> msm_dp_bridge_init() as the 'previous' bridge so that it attaches the
> >>>>>>>> panel bridge to the output of the DP bridge that's attached to the
> >>>>>>>> encoder.
> >>>>>>>
> >>>>>>> Thanks! I'll update the commit log when pushing the patches.
> >>>>>>
> >>>>>> Please correct if i am missing something here.
> >>>>>>
> >>>>>> You are right that the eDP panel's panel bridge attaches to the encoder
> >>>>>> in dp_drm_connector_init() which happens before msm_dp_bridge_init() and
> >>>>>> hence it will attach directly to the encoder.
> >>>>>>
> >>>>>> But we are talking about different encoders here. DP's dp_display has a
> >>>>>> different encoder compared to the EDP's dp_display.
> >>>>>
> >>>>> The encoders are different. However both encoders use the same
> >>>>> codepath, which includes dp_bridge. It controls dp_display by calling
> >>>>> msm_dp_display_foo() functions.
> >>>>>
> >>>
> >>>>>> So DP's bridge will still be attached to its encoder's root.
> >>>>>
> >>>>> There is one dp_bridge per each encoder. Consider sc8180x which has 3
> >>>>> DP controllers (and thus 3 dp_bridges).
> >>>>>
> >>>>
> >>>> Sorry, but I still didnt follow this.
> >>>>
> >>>> So for eDP, dp_drm_connector_init() will attach the panel_bridge
> >>>> and then msm_dp_bridge_init() will add a drm_bridge.
> >>>>
> >>>> And yes in that case, the drm_bridge will be after the panel_bridge
> >>>>
> >>>> But since panel_bridge is at the root for eDP it should be okay.
> >>>
> >>> No. It is not.
> >>> For both DP and eDP the chain should be:
> >>> dpu_encoder -> dp_bridge -> external (panel) bridge, optional for DP
> >>> -> [other bridges] -> connector
> >>>
> >>> Otherwise drm_bridge_chain_foo() functions would be called in the
> >>> incorrect order.
> >>
> >> Agreed. For drm_bridge_chain_foo() ordering to be correct, for eDP chain
> >> the order should be what you mentioned and panel_bridge should be at the
> >> end ( should be the last one ).
> >>
> >> For the above reason,
> >>
> >> Reviewed-by: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>
> >>
> >> I still didnt understand what gets affected by this for the
> >> msm_dp_display_foo() functions you mentioned earlier and wanted to get
> >> some clarity on that.
> >
> > They should be called at the correct time.
> >
> >>>
> >>> Thus the dp_bridge should be attached directly to the encoder
> >>> (drm_encoder) and panel_bridge should use dp_bridge as the 'previous'
> >>> bridge.
> >>>
> >>
> >> Agreed.
> >>
> >>> For example, for the DP port one can use a display-connector (which
> >>> actually implements drm_bridge) as an external bridge to provide hpd
> >>> or dp power GPIOs.
> >>>
> >>> Note, that the current dp_connector breaks layering. It makes calls
> >>> directly into dp_display, not allowing external bridge (and other
> >>> bridges) to override get_modes/mode_valid and other callbacks.
> >>> Thus one of the next patches in series (the one that Kuogee had issues
> >>> with) tries to replace the chain with the following one:
> >>> dpu_encoder -> dp_bridge -> external (panel) bridge -> [other bridges]
> >>> -> drm_bridge_connector.
> >>>
> >>>>
> >>
> >> So originally the plan was always that the DP connector layer intercepts
> >> the call because panel-eDP file did not support reading of the EDID ( we
> >> have not provided the aux bus ). So it was intended that we did not want
> >> to goto the eDP panel to get the modes. Not an error but something which
> >> we wanted to cleanup later when we moved to panel-eDP completely.
> >
> > panel_edp_get_modes() correctly handles this case and returns modes
> > specified in the panel description. So the code should work even with
> > panel-eDP and w/o the AUX bus.
> >
>
> If hard-coded modes are not present, it will fail in below case:
>
>      /*
>       * Add hard-coded panel modes. Don't call this if there are no timings
>       * and no modes (the generic edp-panel case) because it will clobber
>       * the display_info that was already set by drm_add_edid_modes().
>       */
>      if (p->desc->num_timings || p->desc->num_modes)
>          num += panel_edp_get_non_edid_modes(p, connector);
>      else if (!num)
>          dev_warn(p->base.dev, "No display modes\n");
>
> Thats exactly the error he was seeing.

Ah, so he had neither timings nor modes. The rest of the panels does.

>
> >>
> >> Till then we wanted the dp_connector to read the EDID and get the modes.
> >>
> >> So this was actually intended to happen till the point where we moved to
> >> panel-eDP to get the modes.
> >>
> >> Hence what you have mentioned in your cover letter is right that the
> >> chain was broken but there was no loss of functionality due to that today.
> >>
> >> Now that these changes are made, we can still goto panel-eDP file for
> >> the modes because of the below change from Sankeerth where the mode is
> >> hard-coded:
> >>
> >> https://patchwork.freedesktop.org/patch/473431/
> >>
> >> With this change cherry-picked it should work for kuogee. We will
> >> re-test that with this change.
> >
> > I suppose he had both of the changes in the tree. Otherwise the
> > panel_edp_get_modes() wouldn't be called.
>
> No he did not.
>
> That brings up another point.
>
> Even Bjorn was relying on the DP connector's get_modes() for his eDP
> panel if I am not mistaken.
>
> Unless he makes an equivalent change for his panel OR supports reading
> EDID in panel-edp, then he will hit the same error.
>
> Given that your changes were only compile tested, lets wait for both
> Kuogee and Bjorn to validate their resp setups.

Sure, anyway I think it's too late to bring in this patch during this cycle.

>
> >
> >>>> Your commit text was mentioning about DP.
> >>>>
> >>>> For DP, for each controller in the catalog, we will call modeset_init()
> >>>> which should skip this part for DP
> >>>>
> >>>>      if (dp_display->panel_bridge) {
> >>>>            ret = drm_bridge_attach(dp_display->encoder,
> >>>>                        dp_display->panel_bridge, NULL,
> >>>
> >>> as you see, NULL is incorrect. It should be a pointer to dp_bridge instead
> >>>
> >>>>                        DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> >>>>            if (ret < 0) {
> >>>>                DRM_ERROR("failed to attach panel bridge: %d\n", ret);
> >>>>                return ERR_PTR(ret);
> >>>>            }
> >>>>        }
> >>>>
> >>>> Followed by calling msm_dp_bridge_init() which will actually attach the
> >>>> bridge:
> >>>>
> >>>> drm_bridge_attach(encoder, bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> >>>
> >>> And this bridge should be attached before the external bridge.
> >>>
> >>>>
> >>>> Now, even for 3 DP controllers, this shall be true as there will be 3
> >>>> separate encoders and 3 dp_displays and hence 3 drm_bridges.
> >>>>
> >>>> What am i missing here?
> >>>>
> >>>>>>
> >>>>>> So what are we achieving with this change?
> >>>>>>
> >>>>>>>
> >>>>>>>>
> >>>>>>>>>
> >>>>>>>>> Fixes: 8a3b4c17f863 ("drm/msm/dp: employ bridge mechanism for display
> >>>>>>>>> enable and disable")
> >>>>>>>>> Cc: Kuogee Hsieh <quic_khsieh@xxxxxxxxxxx>
> >>>>>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> >>>>>>>>> ---
> >>>>>>>>
> >>>>>>>> Reviewed-by: Stephen Boyd <swboyd@xxxxxxxxxxxx>
> >>>>>>>>
> >>>>>>>>>      drivers/gpu/drm/msm/dp/dp_drm.c | 21 +++++++++++----------
> >>>>>>>>>      1 file changed, 11 insertions(+), 10 deletions(-)
> >>>>>>>>>
> >>>>>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c
> >>>>>>>>> b/drivers/gpu/drm/msm/dp/dp_drm.c
> >>>>>>>>> index d4d360d19eba..26ef41a4c1b6 100644
> >>>>>>>>> --- a/drivers/gpu/drm/msm/dp/dp_drm.c
> >>>>>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_drm.c
> >>>>>>>>> @@ -169,16 +169,6 @@ struct drm_connector
> >>>>>>>>> *dp_drm_connector_init(struct msm_dp *dp_display)
> >>>>>>>>>
> >>>>>>>>>             drm_connector_attach_encoder(connector, dp_display->encoder);
> >>>>>>>>>
> >>>>>>>>> -       if (dp_display->panel_bridge) {
> >>>>>>>>> -               ret = drm_bridge_attach(dp_display->encoder,
> >>>>>>>>> -                                       dp_display->panel_bridge, NULL,
> >>>>>>>>> -                                       DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> >>>>>>>>> -               if (ret < 0) {
> >>>>>>>>> -                       DRM_ERROR("failed to attach panel bridge:
> >>>>>>>>> %d\n", ret);
> >>>>>>>>> -                       return ERR_PTR(ret);
> >>>>>>>>> -               }
> >>>>>>>>> -       }
> >>>>>>>>> -
> >>>>>>>>>             return connector;
> >>>>>>>>>      }
> >>>>>>>>>
> >>>>>>>>> @@ -246,5 +236,16 @@ struct drm_bridge *msm_dp_bridge_init(struct
> >>>>>>>>> msm_dp *dp_display, struct drm_devi
> >>>>>>>>>                     return ERR_PTR(rc);
> >>>>>>>>>             }
> >>>>>>>>>
> >>>>>>>>> +       if (dp_display->panel_bridge) {
> >>>>>>>>> +               rc = drm_bridge_attach(dp_display->encoder,
> >>>>>>>>> +                                       dp_display->panel_bridge,
> >>>>>>>>> bridge,
> >>>>>>>>> +                                       DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> >>>>>>>>> +               if (rc < 0) {
> >>>>>>>>> +                       DRM_ERROR("failed to attach panel bridge:
> >>>>>>>>> %d\n", rc);
> >>>>>>>>> +                       drm_bridge_remove(bridge);
> >>>>>>>>> +                       return ERR_PTR(rc);
> >>>>>>>>> +               }
> >>>>>>>>> +       }
> >>>>>>>>> +
> >>>>>>>>>             return bridge;
> >>>>>>>>
> >>>>>>>> Not a problem with this patch, but what is this pointer used for? I see
> >>>>>>>> it's assigned to priv->bridges and num_bridges is incremented but nobody
> >>>>>>>> seems to look at that.
> >>>>>>>
> >>>>>>>
> >>>>>>> That's on my todo list. to remove connectors array and to destroy
> >>>>>>> created bridges during drm device destruction.
> >>>>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>
> >>>
> >>>
> >
> >
> >



-- 
With best wishes
Dmitry



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux