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

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

On 14/04/2022 19:39, Doug Anderson wrote:
> Hi,
> 
> On Thu, Apr 14, 2022 at 5:20 AM Sankeerth Billakanti
> <quic_sbillaka@quicinc.com> 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@chromium.org

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