Messages in this thread | | | Date | Fri, 6 May 2022 23:49:15 +0300 | Subject | Re: [PATCH v10 1/4] drm/msm/dp: Add eDP support via aux_bus | From | Dmitry Baryshkov <> |
| |
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@quicinc.com> > Reviewed-by: Douglas Anderson <dianders@chromium.org> > Reviewed-by: Stephen Boyd <swboyd@chromium.org>
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.
> --- > 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
-- With best wishes Dmitry
| |