Messages in this thread | | | Date | Sat, 7 May 2022 00:05:01 +0300 | Subject | Re: [PATCH v10 1/4] drm/msm/dp: Add eDP support via aux_bus | From | Dmitry Baryshkov <> |
| |
On 07/05/2022 00:03, Abhinav Kumar wrote: > > > 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@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. >> > > 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?
Yes, it helped us to uncover several error. But I'd still prefer to have EPROBE_DEFER being returned earlier rather than later.
> >>> --- >>> 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
| |