| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAE-0n50dmA0ETgNvaBGs+XmGu+r=6RbfbmnHqXAFqUBGjVGDvg@mail.gmail.com>
Date: Thu, 17 Mar 2022 17:37:48 -0400
From: Stephen Boyd <swboyd@...omium.org>
To: Sankeerth Billakanti <quic_sbillaka@...cinc.com>,
devicetree@...r.kernel.org, dri-devel@...ts.freedesktop.org,
freedreno@...ts.freedesktop.org, linux-arm-msm@...r.kernel.org,
linux-kernel@...r.kernel.org
Cc: robdclark@...il.com, seanpaul@...omium.org,
quic_kalyant@...cinc.com, quic_abhinavk@...cinc.com,
dianders@...omium.org, quic_khsieh@...cinc.com, agross@...nel.org,
bjorn.andersson@...aro.org, robh+dt@...nel.org, krzk+dt@...nel.org,
sean@...rly.run, airlied@...ux.ie, daniel@...ll.ch,
thierry.reding@...il.com, sam@...nborg.org,
dmitry.baryshkov@...aro.org, quic_vproddut@...cinc.com
Subject: Re: [PATCH v5 5/9] drm/msm/dp: Add eDP support via aux_bus
Quoting Sankeerth Billakanti (2022-03-16 10:35:50)
> This patch adds support for generic eDP sink through aux_bus.
Please unindent commit text paragraphs. This isn't a book.
> 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 aux tractions originating
s/tractions/transactions/
> from it. So, the host_init and phy_init are moved to execute before the
> panel probe.
>
> The host_init has to return early if the core is already
> initialized so that the regulator and clock votes for the controller
> resources are balanced.
>
> EV_HPD_INIT_SETUP needs to execute immediately to enable the
> interrupts for the aux transactions from panel-edp to get the modes
> supported.
There are a lot of things going on in this patch. Can it be split up?
>
> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@...cinc.com>
> ---
> drivers/gpu/drm/msm/dp/dp_display.c | 65 +++++++++++++++++++++++++++++++++++--
> drivers/gpu/drm/msm/dp/dp_drm.c | 10 +++---
> drivers/gpu/drm/msm/dp/dp_parser.c | 21 +-----------
> drivers/gpu/drm/msm/dp/dp_parser.h | 1 +
> 4 files changed, 70 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 382b3aa..688bbed 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/drm_dp_aux_bus.h>
>
> #include "msm_drv.h"
> #include "msm_kms.h"
> @@ -265,8 +266,6 @@ static int dp_display_bind(struct device *dev, struct device *master,
> goto end;
> }
>
> - dp->dp_display.next_bridge = dp->parser->next_bridge;
> -
> dp->aux->drm_dev = drm;
> rc = dp_aux_register(dp->aux);
> if (rc) {
> @@ -421,6 +420,11 @@ static void dp_display_host_init(struct dp_display_private *dp)
> dp->dp_display.connector_type, dp->core_initialized,
> dp->phy_initialized);
>
> + if (dp->core_initialized) {
> + DRM_DEBUG_DP("DP core already initialized\n");
> + return;
> + }
> +
> dp_power_init(dp->power, false);
> dp_ctrl_reset_irq_ctrl(dp->ctrl, true);
> dp_aux_init(dp->aux);
> @@ -433,6 +437,11 @@ static void dp_display_host_deinit(struct dp_display_private *dp)
> dp->dp_display.connector_type, dp->core_initialized,
> dp->phy_initialized);
>
> + if (!dp->core_initialized) {
> + DRM_DEBUG_DP("DP core not initialized\n");
> + return;
> + }
> +
> dp_ctrl_reset_irq_ctrl(dp->ctrl, false);
> dp_aux_deinit(dp->aux);
> dp_power_deinit(dp->power);
> @@ -1502,7 +1511,7 @@ 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);
> + dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 0);
> }
>
> void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor)
> @@ -1524,6 +1533,52 @@ 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 = 0;
Drop initialization.
> + 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_display_host_init(dp_priv);
> + dp_catalog_ctrl_hpd_config(dp_priv->catalog);
> + enable_irq(dp_priv->irq);
> + dp_display_host_phy_init(dp_priv);
> +
> + devm_of_dp_aux_populate_ep_devices(dp_priv->aux);
> +
> + disable_irq(dp_priv->irq);
Why do we disable irq?
> + }
The aux_bus node leaked.
> +
> + /*
> + * 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 (rc == -ENODEV) {
> + if (dp->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;
> + }
> +
> + dp->next_bridge = dp_priv->parser->next_bridge;
> +
> + return 0;
> +}
> +
> int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
> struct drm_encoder *encoder)
> {
> @@ -1547,6 +1602,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);
Didn't we just move bridge attachment out of modeset? Why is it being
done here?
> + 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_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
> index 7ce1aca..5254bd6 100644
> --- a/drivers/gpu/drm/msm/dp/dp_drm.c
> +++ b/drivers/gpu/drm/msm/dp/dp_drm.c
> @@ -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 (bridge->type == DRM_MODE_CONNECTOR_DisplayPort) {
Why can't eDP have bridge ops that are the same?
> + 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) {
Powered by blists - more mailing lists