Re: [PATCH v7 1/4] drm/msm/dp: Add eDP support via aux_bus

[Dmitry Baryshkov]

On 14/04/2022 19:39, Doug Anderson wrote:
> Hi,
>
> On Thu, Apr 14, 2022 at 5:20 AM Sankeerth Billakanti
> <quic_sbillaka@xxxxxxxxxxx> wrote:
> > @@ -1530,6 +1532,60 @@ void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor)
> >          }
> >   }
> >
> > +static int dp_display_get_next_bridge(struct msm_dp *dp)
> > +{
> > +       int rc;
> > +       struct dp_display_private *dp_priv;
> > +       struct device_node *aux_bus;
> > +       struct device *dev;
> > +
> > +       dp_priv = container_of(dp, struct dp_display_private, dp_display);
> > +       dev = &dp_priv->pdev->dev;
> > +       aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
> > +
> > +       if (aux_bus && dp->is_edp) {
> > +               dp_display_host_init(dp_priv);
> > +               dp_catalog_ctrl_hpd_config(dp_priv->catalog);
> > +               dp_display_host_phy_init(dp_priv);
> > +               enable_irq(dp_priv->irq);
> > +
> > +               rc = devm_of_dp_aux_populate_ep_devices(dp_priv->aux);
> > +               of_node_put(aux_bus);
> > +               if (rc) {
> > +                       disable_irq(dp_priv->irq);
> > +                       dp_display_host_phy_exit(dp_priv);
> > +                       dp_display_host_deinit(dp_priv);
> > +                       return rc;
> > +               }
> > +       } else if (dp->is_edp) {
> > +               DRM_ERROR("eDP aux_bus not found\n");
> > +               return -ENODEV;
> > +       }
> > +
> > +       /*
> > +        * External bridges are mandatory for eDP interfaces: one has to
> > +        * provide at least an eDP panel (which gets wrapped into panel-bridge).
> > +        *
> > +        * For DisplayPort interfaces external bridges are optional, so
> > +        * silently ignore an error if one is not present (-ENODEV).
> > +        */
> > +       rc = dp_parser_find_next_bridge(dp_priv->parser);
>
> This gets into the same problem that Dmitry pointed out that ps8640
> has that's addressed by my recent series [1].  Namely it's not
> guaranteed that the panel will have finished probing by the time
> devm_of_dp_aux_populate_ep_devices() finishes probing. I don't think
> it's going to be really solvable without the bigger rewrite that we've
> been discussing, though. ...it's probably OK to land something like
> what you have here, but it might at least deserve a comment in the
> code?
>
> [1] https://lore.kernel.org/r/20220409023628.2104952-1-dianders@xxxxxxxxxxxx

We agreed that rework would follow up in a timely manner if these 
patches are applied. However a comment would be still a good thing.

> > +       if (rc == -ENODEV) {
> > +               if (dp->is_edp) {
> > +                       DRM_ERROR("eDP: next bridge is not present\n");
> > +                       return rc;
> > +               }
> > +       } else if (rc) {
> > +               if (rc != -EPROBE_DEFER)
> > +                       DRM_ERROR("DP: error parsing next bridge: %d\n", rc);
> > +               return rc;
>
> In both of your two error returns here isn't it a problem that you don't do:
>
>    disable_irq(dp_priv->irq);
>    dp_display_host_phy_exit(dp_priv);
>    dp_display_host_deinit(dp_priv);
>
> Should probably at least fix that clear error before landing, unless
> I'm misunderstanding and there's some reason not to do that?
>
>
> As discussed previously, I'm not convinced that we've covered every
> corner case for properly doing and undoing the above things. I'm
> hoping that once we do the cleanup and move to pm_runtime() management
> that it will be cleaned up?
>
>
> > @@ -114,10 +114,12 @@ struct drm_bridge *dp_bridge_init(struct msm_dp *dp_display, struct drm_device *
> >          bridge->funcs = &dp_bridge_ops;
> >          bridge->type = dp_display->connector_type;
> >
> > -       bridge->ops =
> > -               DRM_BRIDGE_OP_DETECT |
> > -               DRM_BRIDGE_OP_HPD |
> > -               DRM_BRIDGE_OP_MODES;
> > +       if (!dp_display->is_edp) {
> > +               bridge->ops =
> > +                       DRM_BRIDGE_OP_DETECT |
> > +                       DRM_BRIDGE_OP_HPD |
> > +                       DRM_BRIDGE_OP_MODES;
>
> Given that Dmitry had questions about why eDP has different ops in his
> previous review of this code, the above probably deserves an inline
> code comment. If you want to use my wording, you could paste this into
> your code:
>
>    /*
>     * Many ops only make sense for DP. Why?
>     * - Detect/HPD are used by DRM to know if a display is _physically_
>     *   there, not whether the display is powered on / finished initting.
>     *   On eDP we assume the display is always there because you can't
>     *   know until power is applied. If we don't implement the ops DRM will
>     *   assume our display is always there.
>     * - Currently eDP mode reading is driven by the panel driver. This
>     *   allows the panel driver to properly power itself on to read the
>     *   modes.
>     */

I think it's too verbose and a bit incorrect.
This is a bit saner:
/*
 * These ops do not make sense for eDP, since they are provided
 * by the panel-bridge corresponding to the attached eDP panel.
 */

My question was whether we really need to disable them for eDP since for 
eDP the detect and and get_modes will be overridden anyway.

> Overall: as discussed, I think that the current implementation is a
> bit fragile and might have some wrong corner cases since it's hard for
> me to reason about exactly when we init/de-init things. Even if it
> works great, the fact that it's hard to reason about isn't wonderful.
> That being said, I honestly believe that would benefit upstream to get
> this landed and iterate on it. I don't think this should be causing
> any existing behavior to be _worse_ and getting it landed upstream
> will keep more people focused on the same codebase.
>
>
> -Doug


--
With best wishes
Dmitry