-----Original Message----- From: Stephen Boyd swboyd@chromium.org Sent: Friday, March 18, 2022 2:53 AM To: Sankeerth Billakanti (QUIC) 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 quic_kalyant@quicinc.com; Abhinav Kumar (QUIC) quic_abhinavk@quicinc.com; dianders@chromium.org; Kuogee Hsieh (QUIC) 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 quic_vproddut@quicinc.com Subject: Re: [PATCH v5 2/9] arm64: dts: qcom: sc7280: Add support for eDP panel on CRD

Quoting Sankeerth Billakanti (2022-03-16 10:35:47)

...

diff --git a/arch/arm64/boot/dts/qcom/sc7280-crd.dts b/arch/arm64/boot/dts/qcom/sc7280-crd.dts index e2efbdd..2df654e 100644 --- a/arch/arm64/boot/dts/qcom/sc7280-crd.dts +++ b/arch/arm64/boot/dts/qcom/sc7280-crd.dts @@ -7,6 +7,7 @@

/dts-v1/;

+#include <dt-bindings/pinctrl/qcom,pmic-gpio.h> #include "sc7280-idp.dtsi" #include "sc7280-idp-ec-h1.dtsi"

@@ -21,6 +22,27 @@ chosen { stdout-path = "serial0:115200n8"; };

•   edp_3v3_regulator: edp-3v3-regulator {
  

•           compatible = "regulator-fixed";
  

•           regulator-name = "edp_3v3_regulator";
  

•           regulator-min-microvolt = <3300000>;
  

•           regulator-max-microvolt = <3300000>;
  

•           gpio = <&tlmm 80 GPIO_ACTIVE_HIGH>;
  

•           enable-active-high;
  

•           pinctrl-names = "default";
  

•           pinctrl-0 = <&edp_panel_power>;
  

•   };
  

•   vreg_edp_bp: vreg-edp-bp-regulator {
  

•           compatible = "regulator-fixed";
  

•           regulator-name = "vreg_edp_bp";
  

•           regulator-always-on;
  

•           regulator-boot-on;
  

•   };
  

};

&apps_rsc { @@ -76,6 +98,58 @@ ap_ts_pen_1v8: &i2c13 { }; };

+&mdss {

•   status = "okay";
  

+};

+&mdss_dp {

•   status = "okay";
  

•   pinctrl-names = "default";
  

•   pinctrl-0 = <&dp_hot_plug_det>;
  

•   data-lanes = <0 1>;
  

•   vdda-1p2-supply = <&vreg_l6b_1p2>;
  

•   vdda-0p9-supply = <&vreg_l1b_0p8>; };
  

+&mdss_edp {

•   status = "okay";
  

•   data-lanes = <0 1 2 3>;
  

Is this property necessary? It looks like the default.

Will remove it

...

•   vdda-1p2-supply = <&vreg_l6b_1p2>;
  

•   vdda-0p9-supply = <&vreg_l10c_0p8>;
  

•   aux-bus {
  

Can this move to sc7280.dtsi and get a phandle?

Okay, I will move this to sc7280.dtsi like below. Shall I define the required properties under &mdss_edp_panel node in the sc7280-crd3.dts?

--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi @@ -3283,6 +3283,18 @@

status = "disabled";

+ aux-bus { + mdss_edp_panel: panel { + compatible = "edp-panel"; + + port { + mdss_edp_panel_in: endpoint { + remote-endpoint = <&mdss_edp_out>; + }; + }; + }; + }; + ports { #address-cells = <1>; #size-cells = <0>; @@ -3296,7 +3308,9 @@

port@1 { reg = <1>; - mdss_edp_out: endpoint { }; + mdss_edp_out: endpoint { + remote-endpoint = <&mdss_edp_panel_in>; + }; }; };

...

•           edp_panel: edp-panel {
  

I'd prefer

edp_panel: panel {

because there's only one panel node at this level.

Okay. I will change it.

...

•                   compatible = "edp-panel";
  

•                   power-supply = <&edp_3v3_regulator>;
  

This is board specific, but I thought it was on the qcard so we should move this to sc7280-qcard.dtsi?

Qcard is used only for herobrine as of now according to the code. We defined this only for CRD. We will discuss this internally to understand the plan ahead.

...

•                   ports {
  

•                           #address-cells = <1>;
  

•                           #size-cells = <0>;
  

•                           port@0 {
  

•                                   reg = <0>;
  

•                                   edp_panel_in: endpoint {
  

This can be shortened to

	port {
		edp_panel_in: endpoint {

according to panel-edp.yaml

Okay. I will do it

...

•                                           remote-endpoint = <&mdss_edp_out>;
  

•                                   };
  

•                           };
  

•                   };
  

•           };
  

•   };
  

+};

+&mdss_edp_out {

•   remote-endpoint = <&edp_panel_in>; };
  

+&mdss_edp_phy {

•   status = "okay";
  

+};

+&mdss_mdp {

•   status = "okay";
  

+};

&nvme_3v3_regulator { gpio = <&tlmm 51 GPIO_ACTIVE_HIGH>; }; @@ -84,7 +158,26 @@ ap_ts_pen_1v8: &i2c13 { pins = "gpio51"; };

+&pm8350c_gpios {

•   edp_bl_power: edp-bl-power {
  

Is this used in a later patch?

Yes, will move it to that patch.

...

•           pins = "gpio7";
  

•           function = "normal";
  

•           qcom,drive-strength = <PMIC_GPIO_STRENGTH_LOW>;
  

•   };
  

•   edp_bl_pwm: edp-bl-pwm {
  

Is this used in a later patch? Can it be moved to the patch that uses it?

Yes, will move it to that patch. We split the patch to exclude the dependent pwm nodes so that Bjorn can merge this patch. But we will move all the related nodes to the next patch

...

•           pins = "gpio8";
  

•           function = "func1";
  

•           qcom,drive-strength = <PMIC_GPIO_STRENGTH_LOW>;
  

•   };
  

+};

&tlmm {

•   edp_panel_power: edp-panel-power {
  

•           pins = "gpio80";
  

•           function = "gpio";
  

function of gpio is unnecessary. Where is the bias and drive-strength settings?

Will add it

...

•   };
  

•   tp_int_odl: tp-int-odl {
            pins = "gpio7";
            function = "gpio";
  

-- 2.7.4

Quoting Sankeerth Billakanti (2022-03-16 10:35:48)

Enable backlight support for eDP panel on CRD platform for sc7280.

Signed-off-by: Sankeerth Billakanti quic_sbillaka@quicinc.com

Changes in v5:

• Separate out backlight nodes

arch/arm64/boot/dts/qcom/sc7280-crd.dts | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7280-crd.dts b/arch/arm64/boot/dts/qcom/sc7280-crd.dts index 2df654e..16d1a5b 100644 --- a/arch/arm64/boot/dts/qcom/sc7280-crd.dts +++ b/arch/arm64/boot/dts/qcom/sc7280-crd.dts @@ -37,6 +37,15 @@ pinctrl-0 = <&edp_panel_power>; };

•   edp_backlight: edp-backlight {
  

Does this also move to qcard.dtsi? Why can't this be combined with the previous patch?

•           compatible = "pwm-backlight";
  

•           power-supply = <&vreg_edp_bp>;
  

•           pwms = <&pm8350c_pwm 3 65535>;
  

•           enable-gpios = <&pm8350c_gpios 7 GPIO_ACTIVE_HIGH>;
  

•   };
  

•   vreg_edp_bp: vreg-edp-bp-regulator {
            compatible = "regulator-fixed";
            regulator-name = "vreg_edp_bp";
  

@@ -123,7 +132,9 @@ ap_ts_pen_1v8: &i2c13 { edp_panel: edp-panel { compatible = "edp-panel";

•                   backlight = <&edp_backlight>;
                    power-supply = <&edp_3v3_regulator>;
  

Nitpick: Remove this newline from this hunk and put it in when power-supply is introduced.

                    ports {
                            #address-cells = <1>;
                            #size-cells = <0>;

@@ -172,6 +183,13 @@ ap_ts_pen_1v8: &i2c13 { }; };

+&pm8350c_pwm {

•   status = "okay";
  

•   pinctrl-names = "default";
  

•   pinctrl-0 = <&edp_bl_pwm>;
  

I see the pinctrl is used now but it would be easier to review this patch if the pinctrl was in this patch.

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) {
  

Hi,

On Fri, Mar 25, 2022 at 7:11 AM Sankeerth Billakanti (QUIC) quic_sbillaka@quicinc.com wrote:

...

...

@@ -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?

eDP needs to be reported as always connected. Whichever bridge is setting these ops flags should provide the ops. The farthest bridge from the encoder with the ops flag set should implement the ops. drm_bridge_connector_detect reports always connected for eDP. So, we don't need DRM_BRIDGE_OP_DETECT eDP panel bridge will add DRM_BRIDGE_OP_MODES in drm_panel_bridge_add_typed and will call panel_edp_get_modes. As we are not supporting HPD for EDP, we are not setting the HPD ops flag.

Right. It's Expected that eDP and DP would have different ops. If we define "detect" and "HPD" as whether the display is _physically_ connected, not the status of the poorly-named eDP "HPD" pin, then eDP is _supposed_ to be considered always connected and thus would never support DETECT / HPD.

...and right that the panel is expected to handle the modes.

This matches how things have been progressing in Laurent's patches (taken over by Kieran) to add full DP support to sn65dsi86. For instance:

https://lore.kernel.org/r/20220317131250.1481275-3-kieran.bingham+renesas@id... https://lore.kernel.org/r/20220317131250.1481275-4-kieran.bingham+renesas@id...

-Doug

Quoting Sankeerth Billakanti (2022-03-16 10:35:51)

    The source device should ensure the sink is ready before

proceeding to read the sink capability or performing any aux transactions. The sink will indicate its readiness by asserting the HPD line.

    The eDP sink requires power from the source and its HPD line will

be asserted only after the panel is powered on. The panel power will be enabled from the panel-edp driver.

    The controller driver needs to wait for the hpd line to be asserted

by the sink.

Signed-off-by: Sankeerth Billakanti quic_sbillaka@quicinc.com

drivers/gpu/drm/msm/dp/dp_aux.c | 6 ++++++ drivers/gpu/drm/msm/dp/dp_catalog.c | 23 +++++++++++++++++++++++ drivers/gpu/drm/msm/dp/dp_catalog.h | 1 + drivers/gpu/drm/msm/dp/dp_reg.h | 7 ++++++- 4 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c index 6d36f63..2ddc303 100644 --- a/drivers/gpu/drm/msm/dp/dp_aux.c +++ b/drivers/gpu/drm/msm/dp/dp_aux.c @@ -337,6 +337,12 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux, goto exit; }

•   ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog);
  

Why are we making aux transactions when hpd isn't asserted? Can we only register the aux device once we know that state is "connected"? I'm concerned that we're going to be possibly polling the connected bit up to some amount of time (0x0003FFFF usecs?) for each aux transfer when that doesn't make any sense to keep checking. We should be able to check it once, register aux, and then when disconnect happens we can unregister aux.

•   if (ret) {
  

•           DRM_DEBUG_DP("DP sink not ready for aux transactions\n");
  

•           goto exit;
  

•   }
  

•   dp_aux_update_offset_and_segment(aux, msg);
    dp_aux_transfer_helper(aux, msg, true);
  
  

diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c index fac815f..2c3b0f7 100644 --- a/drivers/gpu/drm/msm/dp/dp_catalog.c +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c @@ -242,6 +242,29 @@ void dp_catalog_aux_update_cfg(struct dp_catalog *dp_catalog) phy_calibrate(phy); }

+int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog) +{

•   u32 state, hpd_en, timeout;
  

•   struct dp_catalog_private *catalog = container_of(dp_catalog,
  

•                           struct dp_catalog_private, dp_catalog);
  

•   hpd_en = dp_read_aux(catalog, REG_DP_DP_HPD_CTRL) &
  

•                                   DP_DP_HPD_CTRL_HPD_EN;
  

Use two lines

hpd_en = dp_read_aux(); hpd_en &= DP_DP_HPD_CTRL_HPD_EN;

•   /* no-hpd case */
  

•   if (!hpd_en)
  

•           return 0;
  

•   /* Poll for HPD connected status */
  

•   timeout = dp_read_aux(catalog, REG_DP_DP_HPD_EVENT_TIME_0) &
  

•                                   DP_HPD_CONNECT_TIME_MASK;
  

•   return readl_poll_timeout(catalog->io->dp_controller.aux.base +
  

•                           REG_DP_DP_HPD_INT_STATUS,
  

•                           state, state & DP_DP_HPD_STATE_STATUS_CONNECTED,
  

•                           2000, timeout);
  

+}

static void dump_regs(void __iomem *base, int len) { int i;

Quoting Doug Anderson (2022-03-18 09:24:17)

Hi,

On Thu, Mar 17, 2022 at 6:19 PM Stephen Boyd swboyd@chromium.org wrote:

...

Quoting Sankeerth Billakanti (2022-03-16 10:35:51)

...

    The source device should ensure the sink is ready before

proceeding to read the sink capability or performing any aux transactions. The sink will indicate its readiness by asserting the HPD line.

    The eDP sink requires power from the source and its HPD line will

be asserted only after the panel is powered on. The panel power will be enabled from the panel-edp driver.

    The controller driver needs to wait for the hpd line to be asserted

by the sink.

Signed-off-by: Sankeerth Billakanti quic_sbillaka@quicinc.com

drivers/gpu/drm/msm/dp/dp_aux.c | 6 ++++++ drivers/gpu/drm/msm/dp/dp_catalog.c | 23 +++++++++++++++++++++++ drivers/gpu/drm/msm/dp/dp_catalog.h | 1 + drivers/gpu/drm/msm/dp/dp_reg.h | 7 ++++++- 4 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c index 6d36f63..2ddc303 100644 --- a/drivers/gpu/drm/msm/dp/dp_aux.c +++ b/drivers/gpu/drm/msm/dp/dp_aux.c @@ -337,6 +337,12 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux, goto exit; }

•   ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog);
  

Why are we making aux transactions when hpd isn't asserted? Can we only register the aux device once we know that state is "connected"? I'm concerned that we're going to be possibly polling the connected bit up to some amount of time (0x0003FFFF usecs?) for each aux transfer when that doesn't make any sense to keep checking. We should be able to check it once, register aux, and then when disconnect happens we can unregister aux.

This is for eDP and, unless someone wants to redesign it again, is just how it works.

Specifically:

1. On eDP you _always_ report "connected". This is because when an eDP

panel is turned off (but still there) you actually have no way to detect it--you just have to assume it's there. And thus you _always_ register the AUX bus.

Is reporting "connected" the same as HPD being asserted in the case of eDP? I can understand wanting to report "connected", because as you say, the panel is always connected; there aren't dongles or cables involved. But the state of the HPD pin is changing at runtime, and eDP supports irq_hpd pulses from what I recall, for "link management".

I think this device requires the status bit in the hardware to say it is "connected" before aux transactions are guaranteed to work. Presumably the HPD pin could go be asserted at the SoC's pad and there could be some time still where the hardware status bit hasn't flipped over to "connected" yet and thus aux transactions are going to fail. Can qcom confirm this?

2. When we are asked to read the EDID that happens _before_ the normal

prepare/enable steps. The way that this should work is that the request travels down to the panel. The panel turns itself on (waiting for any hardcoded delays it knows about) and then initiates an AUX transaction. The AUX transaction is in charge of waiting for HPD.

Are we talking about generic_edp_panel_probe()? Why doesn't that poll hpd gpio like panel_edp_prepare_once() does? Are there any links to discussions about this I can read? Pushing hpd state checking into aux transactions looks like the wrong direction. Also, as I said up above I am concerned that even checking the GPIO won't work and we need some way to ask the bridge if HPD is asserted or not and then fallback to the GPIO method if the display phy/controller doesn't have support to check HPD internally. Something on top of DRM_BRIDGE_OP_HPD?

For the DP case this should not cause any significant overhead, right? HPD should always be asserted so this is basically just one extra IO read confirming that HPD is asserted which should be almost nothing... You're just about to do a whole bunch of IO reads/writes in order to program the AUX transaction anyway.

In the DP case the dongle/cable can be disconnected in the middle of aux transactions. If that happens we could be waiting a while in this transfer function to timeout looking for the status bit. The driver already gets an "unplug" irq when the cable is disconnected though so it would be better to figure out a way to stop the aux transactions quickly when that happens without having to read the hardware and poll the bit that we already know is doomed to timeout. I think apple dongles throw this logic for a loop though because the HDMI cable can be disconnected from the dongle and then we don't see an "unplug" irq, just the number of sinks becomes 0. Maybe there's an irq_hpd event, not sure.

...

...

•   if (ret) {
  

•           DRM_DEBUG_DP("DP sink not ready for aux transactions\n");
  

•           goto exit;
  

•   }
  

•   dp_aux_update_offset_and_segment(aux, msg);
    dp_aux_transfer_helper(aux, msg, true);
  
  

diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c index fac815f..2c3b0f7 100644 --- a/drivers/gpu/drm/msm/dp/dp_catalog.c +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c @@ -242,6 +242,29 @@ void dp_catalog_aux_update_cfg(struct dp_catalog *dp_catalog) phy_calibrate(phy); }

+int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog) +{

•   u32 state, hpd_en, timeout;
  

•   struct dp_catalog_private *catalog = container_of(dp_catalog,
  

•                           struct dp_catalog_private, dp_catalog);
  

•   hpd_en = dp_read_aux(catalog, REG_DP_DP_HPD_CTRL) &
  

•                                   DP_DP_HPD_CTRL_HPD_EN;
  

Use two lines

    hpd_en = dp_read_aux();
    hpd_en &= DP_DP_HPD_CTRL_HPD_EN;

...

•   /* no-hpd case */
  

•   if (!hpd_en)
  

•           return 0;
  

I guess reading from hardware is fine, but I would have expected the driver to simply know whether HPD is used or not. Don't need to read it from hardware, do we? It's not like it's changing from minute to minute--this is something known at probe time.

Are you saying that HPD is always asserted? That doesn't sound right. My understanding is that HPD will be asserted after the panel is powered up. Before that HPD is deasserted. Similarly, when we power down the panel, HPD will be deasserted. I guess DRM wants to assume that an eDP panel is always connected? That sounds like it might be OK as long as userspace doesn't use "connected" to know that it's OK to do things like read/write aux or push pixels to the panel when HPD is deasserted.

Quoting Doug Anderson (2022-03-18 14:58:55)

Hi,

On Fri, Mar 18, 2022 at 1:17 PM Stephen Boyd swboyd@chromium.org wrote:

...

...

...

...

•   ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog);
  

Why are we making aux transactions when hpd isn't asserted? Can we only register the aux device once we know that state is "connected"? I'm concerned that we're going to be possibly polling the connected bit up to some amount of time (0x0003FFFF usecs?) for each aux transfer when that doesn't make any sense to keep checking. We should be able to check it once, register aux, and then when disconnect happens we can unregister aux.

This is for eDP and, unless someone wants to redesign it again, is just how it works.

Specifically:

1. On eDP you _always_ report "connected". This is because when an eDP

panel is turned off (but still there) you actually have no way to detect it--you just have to assume it's there. And thus you _always_ register the AUX bus.

Is reporting "connected" the same as HPD being asserted in the case of eDP? I can understand wanting to report "connected", because as you say, the panel is always connected; there aren't dongles or cables involved.

No. What I mean by connected is that when DRM asks "hey, do you have a panel" connected then for eDP we always say "yes" regardless of any hardware state.

HPD is a _huge_ misnomer for eDP and IMO the name causes lots of confusion. It's not "hot plug detect". You don't hot plug eDP. It's really "panel ready / panel IRQ"

...

But the state of the HPD pin is changing at runtime, and eDP supports irq_hpd pulses from what I recall, for "link management".

I think this device requires the status bit in the hardware to say it is "connected" before aux transactions are guaranteed to work. Presumably the HPD pin could go be asserted at the SoC's pad and there could be some time still where the hardware status bit hasn't flipped over to "connected" yet and thus aux transactions are going to fail. Can qcom confirm this?

...

2. When we are asked to read the EDID that happens _before_ the normal

prepare/enable steps. The way that this should work is that the request travels down to the panel. The panel turns itself on (waiting for any hardcoded delays it knows about) and then initiates an AUX transaction. The AUX transaction is in charge of waiting for HPD.

Are we talking about generic_edp_panel_probe()? Why doesn't that poll hpd gpio like panel_edp_prepare_once() does?

There's no HPD GPIO in this case, right?

Right. The hardware supports HPD here, so polling the pin as a gpio is incorrect.

In the trogdor case we ended up not using the HPD that was part of the ti-sn65dsi86 controller because it was fairly useless (it debounced for far too long), so we ended up hooking it up as a GPIO and I guess gave up on getting the extra notifications from the panel. Maybe a good thing, in hindsight, that we didn't do PSR because that might have been a pain.

In any case, originally I had the GPIO being handled by the ti-sn65dsi86 controller driver and that seemed like it made sense to me (after all, the ti-sn65dsi86 driver would have to handle HPD if this went to the dedicated HPD pin) but got told "no, put it in the panel" by both you and Laurent [1].

[1] https://lore.kernel.org/r/20200415203256.GP4758@pendragon.ideasonboard.com/

...

Are there any links to discussions about this I can read?

I'm not sure if there's any more than the conversation I pointed at above where we talked about hpd-gpios. Atop that, I believe I just realized that this was the only way it could work without re-designing again.

To some extent the status quo is documented in commit a64ad9c3e4a5 ("drm/panel-edp: Fix "prepare_to_enable" if panel doesn't handle HPD"). I wrote that commit when I thought about how HPD would need to be handled if it was a dedicated pin on the controller and the panel didn't have knowledge about it.

...

Pushing hpd state checking into aux transactions looks like the wrong direction. Also, as I said up above I am concerned that even checking the GPIO won't work and we need some way to ask the bridge if HPD is asserted or not and then fallback to the GPIO method if the display phy/controller doesn't have support to check HPD internally. Something on top of DRM_BRIDGE_OP_HPD?

If we could somehow get the HPD status from the bridge in the panel driver it definitely would be convenient. It does feel like that's an improvement that could be done later, though. We've already landed a few instances of doing what's done here, like for parade-ps8640 and analogix_dp. I suspect designing a new mechanism might not be the most trivial.

What is done in the bridge drivers is to wait for a fixed timeout and assume aux is ready? Or is it something else? If there's just a fixed timeout for the eDP case it sounds OK to do that for now and we can fine tune it later to actually check HPD status register before the panel tries to read EDID.

I haven't actually tried it, but I suspect that to get something like what you're talking about we'd have to get the rest of drm to know that for eDP ports that it should assume something is connected _regardless_ of what the "detect" / "HPD" options say. Then we'd have to extend the edp-panel code to be able to be able to query the next bridge in the chain if a GPIO wasn't provided.

Can the panel interrogate the bridge chain somehow? It feels like either something in the chain should know the status of HPD (the case here where display controller in the SoC knows) or it should be a gpio to the panel (trogdor case). The bridge ops can implement DRM_BRIDGE_OP_HPD and the first bridge from the encoder that supports HPD can implement some sort of "wait for hpd asserted" function that the panel then uses once it powers up the panel during probe. If the panel has a gpio and nothing else in the chain can detect hpd then the panel polls the gpio, or it waits for the amount of time delay after powering on the panel if the panel's hpd function is called.

...

...

For the DP case this should not cause any significant overhead, right? HPD should always be asserted so this is basically just one extra IO read confirming that HPD is asserted which should be almost nothing... You're just about to do a whole bunch of IO reads/writes in order to program the AUX transaction anyway.

In the DP case the dongle/cable can be disconnected in the middle of aux transactions. If that happens we could be waiting a while in this transfer function to timeout looking for the status bit. The driver already gets an "unplug" irq when the cable is disconnected though so it would be better to figure out a way to stop the aux transactions quickly when that happens without having to read the hardware and poll the bit that we already know is doomed to timeout. I think apple dongles throw this logic for a loop though because the HDMI cable can be disconnected from the dongle and then we don't see an "unplug" irq, just the number of sinks becomes 0. Maybe there's an irq_hpd event, not sure.

Ah, interesting. Having a DP cable unplugged in the middle of an aux transaction does seem like it could be a problem. What if we just wait in the case our bridge.type is "DRM_MODE_CONNECTOR_eDP"? That should be easy, right?

Sounds like it would work. Is this supposed to fix some DP case as well though? There were some patches to speed up aux failures when the dongle was unplugged but I haven't checked after that. I guess this waiting is only important for eDP because the edp-panel code is trying to read EDID and it isn't waiting for HPD to be asserted before doing that.

...

...

...

...

•   if (ret) {
  

•           DRM_DEBUG_DP("DP sink not ready for aux transactions\n");
  

•           goto exit;
  

•   }
  

•   dp_aux_update_offset_and_segment(aux, msg);
    dp_aux_transfer_helper(aux, msg, true);
  
  

diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c index fac815f..2c3b0f7 100644 --- a/drivers/gpu/drm/msm/dp/dp_catalog.c +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c @@ -242,6 +242,29 @@ void dp_catalog_aux_update_cfg(struct dp_catalog *dp_catalog) phy_calibrate(phy); }

+int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog) +{

•   u32 state, hpd_en, timeout;
  

•   struct dp_catalog_private *catalog = container_of(dp_catalog,
  

•                           struct dp_catalog_private, dp_catalog);
  

•   hpd_en = dp_read_aux(catalog, REG_DP_DP_HPD_CTRL) &
  

•                                   DP_DP_HPD_CTRL_HPD_EN;
  

Use two lines

    hpd_en = dp_read_aux();
    hpd_en &= DP_DP_HPD_CTRL_HPD_EN;

...

•   /* no-hpd case */
  

•   if (!hpd_en)
  

•           return 0;
  

I guess reading from hardware is fine, but I would have expected the driver to simply know whether HPD is used or not. Don't need to read it from hardware, do we? It's not like it's changing from minute to minute--this is something known at probe time.

Are you saying that HPD is always asserted?

I don't think this is looking for HPD assertion, is it? This is looking for whether the HPD interrupt is enabled, isn't it? This is to support the case of eDP panels where we didn't hook the HPD line up, right? It should be known at probe time whether we've hooked HPD up or not. ...or am I misreading?

Ah right. This is basically a proxy for "is no-hpd present in DT?" per the last patch in this series.

...

That doesn't sound right. My understanding is that HPD will be asserted after the panel is powered up. Before that HPD is deasserted. Similarly, when we power down the panel, HPD will be deasserted. I guess DRM wants to assume that an eDP panel is always connected? That sounds like it might be OK as long as userspace doesn't use "connected" to know that it's OK to do things like read/write aux or push pixels to the panel when HPD is deasserted.

IMO having userspace reading / writing aux directly and expecting it to work is a terrible idea anyway. It's _maybe_ sorta OK in the DP case, but it's really not good in the eDP case. To me it's sorta like expecting things to be amazing and foolproof when you go behind the kernel's back and write to an i2c device using `i2cset -f`. Sure, it might work, but it can also confuse the heck out of things. It also turns out to be a huge problem when you get to PSR because userspace will get errors if it tries to write to the AUX channel and the panel is in PSR mode. This came up in the context of Brian's analogix dp patches [1]. The right answer, in my mind, is to treat userspace accessing the AUX channel directly as more of a debug feature, at least for eDP panels.

If it's a debug feature then it should be removed from the system. The flow of data is passing through the kernel so if the kernel is getting confused about backdoor access over aux it should snoop the transactions and block things it doesn't like. I don't know the backstory on aux being exposed to userspace, but leaving it in a broken state isn't good.

In terms of userspace pushing pixels to the panel, I don't think that's quite the same, is it? Generally userspace is in charge of whether the eDP panel is powered on or powered off, isn't it?

I'm not sure what it's the same as, but I meant drawing on the screen when the display is powering on but not visible yet is concerning. drm_helper_hpd_irq_event() is used to tell userspace that it can "start drawing now" because it calls drm_kms_helper_hotplug_event() when the connector status changes. This is my understanding of how the DP path works in this driver. I don't know how it works for eDP bridge drivers.

So generally I think that for eDP a panel is always "connected" in all senses of the word. It might not be "powered" at some given point of time, but it's always connected.

[1] https://lore.kernel.org/r/CAD=FV=VYe1rLKANQ8eom7g8x1v6_s_OYnX819Ax4m7O3UwDHm...