From: Stephen Boyd <swboyd@chromium.org> To: Sankeerth Billakanti <quic_sbillaka@quicinc.com>, devicetree@vger.kernel.org, dri-devel@lists.freedesktop.org, freedreno@lists.freedesktop.org, linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org Cc: robdclark@gmail.com, seanpaul@chromium.org, quic_kalyant@quicinc.com, quic_abhinavk@quicinc.com, dianders@chromium.org, quic_khsieh@quicinc.com, agross@kernel.org, bjorn.andersson@linaro.org, robh+dt@kernel.org, krzk+dt@kernel.org, sean@poorly.run, airlied@linux.ie, daniel@ffwll.ch, thierry.reding@gmail.com, sam@ravnborg.org, dmitry.baryshkov@linaro.org, quic_vproddut@quicinc.com Subject: Re: [PATCH v5 5/9] drm/msm/dp: Add eDP support via aux_bus Date: Thu, 17 Mar 2022 17:37:48 -0400 [thread overview] Message-ID: <CAE-0n50dmA0ETgNvaBGs+XmGu+r=6RbfbmnHqXAFqUBGjVGDvg@mail.gmail.com> (raw) In-Reply-To: <1647452154-16361-6-git-send-email-quic_sbillaka@quicinc.com> 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@quicinc.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) {
WARNING: multiple messages have this Message-ID (diff)
From: Stephen Boyd <swboyd@chromium.org> To: Sankeerth Billakanti <quic_sbillaka@quicinc.com>, devicetree@vger.kernel.org, dri-devel@lists.freedesktop.org, freedreno@lists.freedesktop.org, linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org Cc: quic_kalyant@quicinc.com, dianders@chromium.org, quic_vproddut@quicinc.com, airlied@linux.ie, sam@ravnborg.org, quic_abhinavk@quicinc.com, robh+dt@kernel.org, quic_khsieh@quicinc.com, agross@kernel.org, seanpaul@chromium.org, dmitry.baryshkov@linaro.org, thierry.reding@gmail.com, krzk+dt@kernel.org, bjorn.andersson@linaro.org, sean@poorly.run Subject: Re: [PATCH v5 5/9] drm/msm/dp: Add eDP support via aux_bus Date: Thu, 17 Mar 2022 17:37:48 -0400 [thread overview] Message-ID: <CAE-0n50dmA0ETgNvaBGs+XmGu+r=6RbfbmnHqXAFqUBGjVGDvg@mail.gmail.com> (raw) In-Reply-To: <1647452154-16361-6-git-send-email-quic_sbillaka@quicinc.com> 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@quicinc.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) {
next prev parent reply other threads:[~2022-03-17 21:37 UTC|newest] Thread overview: 68+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-03-16 17:35 [PATCH v5 0/9] Add support for the eDP panel on sc7280 CRD Sankeerth Billakanti 2022-03-16 17:35 ` Sankeerth Billakanti 2022-03-16 17:35 ` [PATCH v5 1/9] arm64: dts: qcom: sc7280: rename edp_out label to mdss_edp_out Sankeerth Billakanti 2022-03-16 17:35 ` Sankeerth Billakanti 2022-03-17 21:11 ` Stephen Boyd 2022-03-17 21:11 ` Stephen Boyd 2022-03-18 0:13 ` Doug Anderson 2022-03-18 0:13 ` Doug Anderson 2022-03-16 17:35 ` [PATCH v5 2/9] arm64: dts: qcom: sc7280: Add support for eDP panel on CRD Sankeerth Billakanti 2022-03-16 17:35 ` Sankeerth Billakanti 2022-03-17 21:23 ` Stephen Boyd 2022-03-17 21:23 ` Stephen Boyd 2022-03-25 13:30 ` Sankeerth Billakanti (QUIC) 2022-03-25 13:30 ` Sankeerth Billakanti (QUIC) 2022-03-25 19:51 ` Stephen Boyd 2022-03-25 19:51 ` Stephen Boyd 2022-03-18 17:20 ` Doug Anderson 2022-03-18 17:20 ` Doug Anderson 2022-03-25 13:41 ` Sankeerth Billakanti (QUIC) 2022-03-25 13:41 ` Sankeerth Billakanti (QUIC) 2022-03-25 13:44 ` Doug Anderson 2022-03-25 13:44 ` Doug Anderson 2022-03-16 17:35 ` [PATCH v5 3/9] arm64: dts: qcom: sc7280: Enable backlight for eDP panel Sankeerth Billakanti 2022-03-16 17:35 ` Sankeerth Billakanti 2022-03-17 21:28 ` Stephen Boyd 2022-03-17 21:28 ` Stephen Boyd 2022-03-25 13:34 ` Sankeerth Billakanti (QUIC) 2022-03-25 13:34 ` Sankeerth Billakanti (QUIC) 2022-03-16 17:35 ` [PATCH v5 4/9] drm/panel-edp: add LQ140M1JW46 edp panel entry Sankeerth Billakanti 2022-03-16 17:35 ` Sankeerth Billakanti 2022-03-17 21:28 ` Stephen Boyd 2022-03-17 21:28 ` Stephen Boyd 2022-03-18 0:04 ` Doug Anderson 2022-03-18 0:04 ` Doug Anderson 2022-03-16 17:35 ` [PATCH v5 5/9] drm/msm/dp: Add eDP support via aux_bus Sankeerth Billakanti 2022-03-16 17:35 ` Sankeerth Billakanti 2022-03-17 21:37 ` Stephen Boyd [this message] 2022-03-17 21:37 ` Stephen Boyd 2022-03-25 14:11 ` Sankeerth Billakanti (QUIC) 2022-03-25 14:11 ` Sankeerth Billakanti (QUIC) 2022-03-25 16:45 ` Doug Anderson 2022-03-25 16:45 ` Doug Anderson 2022-03-16 17:35 ` [PATCH v5 6/9] drm/msm/dp: wait for hpd high before any sink interaction Sankeerth Billakanti 2022-03-16 17:35 ` Sankeerth Billakanti 2022-03-18 1:19 ` Stephen Boyd 2022-03-18 1:19 ` Stephen Boyd 2022-03-18 16:24 ` Doug Anderson 2022-03-18 16:24 ` Doug Anderson 2022-03-18 20:16 ` Stephen Boyd 2022-03-18 20:16 ` Stephen Boyd 2022-03-18 21:58 ` Doug Anderson 2022-03-18 21:58 ` Doug Anderson 2022-03-18 23:27 ` Stephen Boyd 2022-03-18 23:27 ` Stephen Boyd 2022-03-18 23:56 ` Doug Anderson 2022-03-18 23:56 ` Doug Anderson 2022-03-25 15:54 ` Sankeerth Billakanti (QUIC) 2022-03-25 15:54 ` Sankeerth Billakanti (QUIC) 2022-03-25 16:05 ` Doug Anderson 2022-03-25 16:05 ` Doug Anderson 2022-03-25 16:44 ` Sankeerth Billakanti 2022-03-25 16:44 ` Sankeerth Billakanti 2022-03-16 17:35 ` [PATCH v5 7/9] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP Sankeerth Billakanti 2022-03-16 17:35 ` Sankeerth Billakanti 2022-03-16 17:35 ` [PATCH v5 8/9] drm/msm/dp: Handle eDP mode_valid case Sankeerth Billakanti 2022-03-16 17:35 ` Sankeerth Billakanti 2022-03-16 17:35 ` [PATCH v5 9/9] drm/msm/dp: Support edp/dp without hpd Sankeerth Billakanti 2022-03-16 17:35 ` Sankeerth Billakanti
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAE-0n50dmA0ETgNvaBGs+XmGu+r=6RbfbmnHqXAFqUBGjVGDvg@mail.gmail.com' \ --to=swboyd@chromium.org \ --cc=agross@kernel.org \ --cc=airlied@linux.ie \ --cc=bjorn.andersson@linaro.org \ --cc=daniel@ffwll.ch \ --cc=devicetree@vger.kernel.org \ --cc=dianders@chromium.org \ --cc=dmitry.baryshkov@linaro.org \ --cc=dri-devel@lists.freedesktop.org \ --cc=freedreno@lists.freedesktop.org \ --cc=krzk+dt@kernel.org \ --cc=linux-arm-msm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=quic_abhinavk@quicinc.com \ --cc=quic_kalyant@quicinc.com \ --cc=quic_khsieh@quicinc.com \ --cc=quic_sbillaka@quicinc.com \ --cc=quic_vproddut@quicinc.com \ --cc=robdclark@gmail.com \ --cc=robh+dt@kernel.org \ --cc=sam@ravnborg.org \ --cc=sean@poorly.run \ --cc=seanpaul@chromium.org \ --cc=thierry.reding@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.