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

[Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>]

On 5/6/2022 1:49 PM, Dmitry Baryshkov wrote:
> On 25/04/2022 14:44, Sankeerth Billakanti wrote:
> > This patch adds support for generic eDP sink through aux_bus. The eDP/DP
> > controller driver should support aux transactions originating from the
> > panel-edp driver and hence should be initialized and ready.
> >
> > The panel bridge supporting the panel should be ready before the bridge
> > connector is initialized. The generic panel probe needs the controller
> > resources to be enabled to support the aux transactions originating from
> > the panel probe.
> >
> > Signed-off-by: Sankeerth Billakanti <quic_sbillaka@xxxxxxxxxxx>
> > Reviewed-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
> > Reviewed-by: Stephen Boyd <swboyd@xxxxxxxxxxxx>
>
> An additional side effect from this patch. Previously missing panel 
> would have caused the bind error. Now it is the dp_modeset_init error, 
> which translates to kms_hw_init returning -517. I kind ask to move the 
> next_bridge acquisition back to the dp_bind in one of the followup patches.

This is true. But the end result would be same isnt it?

When dp_display_bind() failed earlier, it would cause master bind 
failure too due to component model.

Even now, it causes the same result?

> > ---
> > Changes in v10:
> >    - modify the error handling condition
> >    - modify the kernel doc
> >
> > Changes in v9:
> >    - add comments for panel probe
> >    - modify the error handling checks
> >
> > Changes in v8:
> >    - handle corner cases
> >    - add comment for the bridge ops
> >
> > Changes in v7:
> >    - aux_bus is mandatory for eDP
> >    - connector type check modified to just check for eDP
> >
> > Changes in v6:
> >    - Remove initialization
> >    - Fix aux_bus node leak
> >    - Split the patches
> >
> >   drivers/gpu/drm/msm/dp/dp_display.c | 72 
> > ++++++++++++++++++++++++++++++++++---
> >   drivers/gpu/drm/msm/dp/dp_display.h |  1 +
> >   drivers/gpu/drm/msm/dp/dp_drm.c     | 21 ++++++++---
> >   drivers/gpu/drm/msm/dp/dp_parser.c  | 23 ++----------
> >   drivers/gpu/drm/msm/dp/dp_parser.h  | 14 +++++++-
> >   5 files changed, 101 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
> > b/drivers/gpu/drm/msm/dp/dp_display.c
> > index d7a19d6..f772d84 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > @@ -10,6 +10,7 @@
> >   #include <linux/component.h>
> >   #include <linux/of_irq.h>
> >   #include <linux/delay.h>
> > +#include <drm/dp/drm_dp_aux_bus.h>
> >   #include "msm_drv.h"
> >   #include "msm_kms.h"
> > @@ -259,14 +260,12 @@ static int dp_display_bind(struct device *dev, 
> > struct device *master,
> >       dp->dp_display.drm_dev = drm;
> >       priv->dp[dp->id] = &dp->dp_display;
> > -    rc = dp->parser->parse(dp->parser, dp->dp_display.connector_type);
> > +    rc = dp->parser->parse(dp->parser);
> >       if (rc) {
> >           DRM_ERROR("device tree parsing failed\n");
> >           goto end;
> >       }
> > -    dp->dp_display.next_bridge = dp->parser->next_bridge;
> > -
> >       dp->aux->drm_dev = drm;
> >       rc = dp_aux_register(dp->aux);
> >       if (rc) {
> > @@ -1319,6 +1318,8 @@ static int dp_display_probe(struct 
> > platform_device *pdev)
> >       dp->pdev = pdev;
> >       dp->name = "drm_dp";
> >       dp->dp_display.connector_type = desc->connector_type;
> > +    dp->dp_display.is_edp =
> > +        (dp->dp_display.connector_type == DRM_MODE_CONNECTOR_eDP);
> >       rc = dp_init_sub_modules(dp);
> >       if (rc) {
> > @@ -1508,7 +1509,8 @@ void msm_dp_irq_postinstall(struct msm_dp 
> > *dp_display)
> >       dp_hpd_event_setup(dp);
> > -    dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
> > +    if (!dp_display->is_edp)
> > +        dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
> >   }
> >   void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor 
> > *minor)
> > @@ -1530,6 +1532,64 @@ 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);
> > +
> > +        /*
> > +         * The code below assumes that the panel will finish probing
> > +         * by the time devm_of_dp_aux_populate_ep_devices() returns.
> > +         * This isn't a great assumption since it will fail if the
> > +         * panel driver is probed asynchronously but is the best we
> > +         * can do without a bigger driver reorganization.
> > +         */
> > +        rc = devm_of_dp_aux_populate_ep_devices(dp_priv->aux);
> > +        of_node_put(aux_bus);
> > +        if (rc)
> > +            goto error;
> > +    } 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);
> > +    if (!dp->is_edp && rc == -ENODEV)
> > +        return 0;
> > +
> > +    if (!rc) {
> > +        dp->next_bridge = dp_priv->parser->next_bridge;
> > +        return 0;
> > +    }
> > +
> > +error:
> > +    if (dp->is_edp) {
> > +        disable_irq(dp_priv->irq);
> > +        dp_display_host_phy_exit(dp_priv);
> > +        dp_display_host_deinit(dp_priv);
> > +    }
> > +    return rc;
> > +}
> > +
> >   int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device 
> > *dev,
> >               struct drm_encoder *encoder)
> >   {
> > @@ -1553,6 +1613,10 @@ int msm_dp_modeset_init(struct msm_dp 
> > *dp_display, struct drm_device *dev,
> >       dp_display->encoder = encoder;
> > +    ret = dp_display_get_next_bridge(dp_display);
> > +    if (ret)
> > +        return ret;
> > +
> >       dp_display->bridge = dp_bridge_init(dp_display, dev, encoder);
> >       if (IS_ERR(dp_display->bridge)) {
> >           ret = PTR_ERR(dp_display->bridge);
> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.h 
> > b/drivers/gpu/drm/msm/dp/dp_display.h
> > index 49a1d89..1377cc3 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_display.h
> > +++ b/drivers/gpu/drm/msm/dp/dp_display.h
> > @@ -21,6 +21,7 @@ struct msm_dp {
> >       bool audio_enabled;
> >       bool power_on;
> >       unsigned int connector_type;
> > +    bool is_edp;
> >       hdmi_codec_plugged_cb plugged_cb;
> > diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c 
> > b/drivers/gpu/drm/msm/dp/dp_drm.c
> > index 7ce1aca..8a75c55 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_drm.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_drm.c
> > @@ -114,10 +114,23 @@ 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;
> > +    /*
> > +     * 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.
> > +     */
> > +    if (!dp_display->is_edp) {
> > +        bridge->ops =
> > +            DRM_BRIDGE_OP_DETECT |
> > +            DRM_BRIDGE_OP_HPD |
> > +            DRM_BRIDGE_OP_MODES;
> > +    }
> >       rc = drm_bridge_attach(encoder, bridge, NULL, 
> > DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> >       if (rc) {
> > diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c 
> > b/drivers/gpu/drm/msm/dp/dp_parser.c
> > index 1056b8d..4bdbf91 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_parser.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_parser.c
> > @@ -265,7 +265,7 @@ static int dp_parser_clock(struct dp_parser *parser)
> >       return 0;
> >   }
> > -static int dp_parser_find_next_bridge(struct dp_parser *parser)
> > +int dp_parser_find_next_bridge(struct dp_parser *parser)
> >   {
> >       struct device *dev = &parser->pdev->dev;
> >       struct drm_bridge *bridge;
> > @@ -279,7 +279,7 @@ static int dp_parser_find_next_bridge(struct 
> > dp_parser *parser)
> >       return 0;
> >   }
> > -static int dp_parser_parse(struct dp_parser *parser, int connector_type)
> > +static int dp_parser_parse(struct dp_parser *parser)
> >   {
> >       int rc = 0;
> > @@ -300,25 +300,6 @@ static int dp_parser_parse(struct dp_parser 
> > *parser, int connector_type)
> >       if (rc)
> >           return rc;
> > -    /*
> > -     * 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(parser);
> > -    if (rc == -ENODEV) {
> > -        if (connector_type == DRM_MODE_CONNECTOR_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;
> > -    }
> > -
> >       /* Map the corresponding regulator information according to
> >        * version. Currently, since we only have one supported platform,
> >        * mapping the regulator directly.
> > diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h 
> > b/drivers/gpu/drm/msm/dp/dp_parser.h
> > index d371bae..3a4d797 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_parser.h
> > +++ b/drivers/gpu/drm/msm/dp/dp_parser.h
> > @@ -125,7 +125,7 @@ struct dp_parser {
> >       u32 max_dp_lanes;
> >       struct drm_bridge *next_bridge;
> > -    int (*parse)(struct dp_parser *parser, int connector_type);
> > +    int (*parse)(struct dp_parser *parser);
> >   };
> >   /**
> > @@ -141,4 +141,16 @@ struct dp_parser {
> >    */
> >   struct dp_parser *dp_parser_get(struct platform_device *pdev);
> > +/**
> > + * dp_parser_find_next_bridge() - find an additional bridge to DP
> > + *
> > + * @parser: dp_parser data from client
> > + *
> > + * This function is used to find any additional bridge attached to
> > + * the DP controller. The eDP interface requires a panel bridge.
> > + *
> > + * Return: 0 if able to get the bridge, otherwise negative errno for 
> > failure.
> > + */
> > +int dp_parser_find_next_bridge(struct dp_parser *parser);
> > +
> >   #endif