On 14/04/2022 19:39, Doug Anderson wrote:

Hi,

On Thu, Apr 14, 2022 at 5:20 AM Sankeerth Billakanti quic_sbillaka@quicinc.com wrote:

...

@@ -1530,6 +1532,60 @@ 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);
  

•           rc = devm_of_dp_aux_populate_ep_devices(dp_priv->aux);
  

•           of_node_put(aux_bus);
  

•           if (rc) {
  

•                   disable_irq(dp_priv->irq);
  

•                   dp_display_host_phy_exit(dp_priv);
  

•                   dp_display_host_deinit(dp_priv);
  

•                   return rc;
  

•           }
  

•   } 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);
  

This gets into the same problem that Dmitry pointed out that ps8640 has that's addressed by my recent series [1]. Namely it's not guaranteed that the panel will have finished probing by the time devm_of_dp_aux_populate_ep_devices() finishes probing. I don't think it's going to be really solvable without the bigger rewrite that we've been discussing, though. ...it's probably OK to land something like what you have here, but it might at least deserve a comment in the code?

[1] https://lore.kernel.org/r/20220409023628.2104952-1-dianders@chromium.org

We agreed that rework would follow up in a timely manner if these patches are applied. However a comment would be still a good thing.

...

•   if (rc == -ENODEV) {
  

•           if (dp->is_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;
  

In both of your two error returns here isn't it a problem that you don't do:

disable_irq(dp_priv->irq); dp_display_host_phy_exit(dp_priv); dp_display_host_deinit(dp_priv);

Should probably at least fix that clear error before landing, unless I'm misunderstanding and there's some reason not to do that?

As discussed previously, I'm not convinced that we've covered every corner case for properly doing and undoing the above things. I'm hoping that once we do the cleanup and move to pm_runtime() management that it will be cleaned up?

...

@@ -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 (!dp_display->is_edp) {
  

•           bridge->ops =
  

•                   DRM_BRIDGE_OP_DETECT |
  

•                   DRM_BRIDGE_OP_HPD |
  

•                   DRM_BRIDGE_OP_MODES;
  

Given that Dmitry had questions about why eDP has different ops in his previous review of this code, the above probably deserves an inline code comment. If you want to use my wording, you could paste this into your code:

/* * 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. */

I think it's too verbose and a bit incorrect. This is a bit saner: /* * These ops do not make sense for eDP, since they are provided * by the panel-bridge corresponding to the attached eDP panel. */

My question was whether we really need to disable them for eDP since for eDP the detect and and get_modes will be overridden anyway.

Overall: as discussed, I think that the current implementation is a bit fragile and might have some wrong corner cases since it's hard for me to reason about exactly when we init/de-init things. Even if it works great, the fact that it's hard to reason about isn't wonderful. That being said, I honestly believe that would benefit upstream to get this landed and iterate on it. I don't think this should be causing any existing behavior to be _worse_ and getting it landed upstream will keep more people focused on the same codebase.

-Doug

Hi,

On Thu, Apr 14, 2022 at 5:20 AM Sankeerth Billakanti quic_sbillaka@quicinc.com wrote:

The panel-edp enables the eDP panel power during probe, get_modes and enable.

Technically the panel-edp powers on the panel in pre_enable()

The eDP connect and disconnect interrupts for the eDP/DP controller are directly dependent on panel power. As eDP display can be assumed as always connected, the controller driver can skip the eDP connect and disconnect interrupts. Any disruption in the link status will be indicated via the IRQ_HPD interrupts.

So, the eDP controller driver can just enable the IRQ_HPD and replug interrupts. The DP controller driver still needs to enable all the interrupts.

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

Changes in v7:

• reordered the patch in the series
• modified the return statement for isr
• connector check modified to just check for eDP

drivers/gpu/drm/msm/dp/dp_catalog.c | 9 +++------ drivers/gpu/drm/msm/dp/dp_display.c | 22 +++++++++++++++++++++- 2 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c index fac815f..07f2389 100644 --- a/drivers/gpu/drm/msm/dp/dp_catalog.c +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c @@ -569,10 +569,6 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog *dp_catalog)

    u32 reftimer = dp_read_aux(catalog, REG_DP_DP_HPD_REFTIMER);

•   /* enable HPD plug and unplug interrupts */
  

•   dp_catalog_hpd_config_intr(dp_catalog,
  

•           DP_DP_HPD_PLUG_INT_MASK | DP_DP_HPD_UNPLUG_INT_MASK, true);
  

•   /* Configure REFTIMER and enable it */
    reftimer |= DP_DP_HPD_REFTIMER_ENABLE;
    dp_write_aux(catalog, REG_DP_DP_HPD_REFTIMER, reftimer);
  

@@ -599,13 +595,14 @@ u32 dp_catalog_hpd_get_intr_status(struct dp_catalog *dp_catalog) { struct dp_catalog_private *catalog = container_of(dp_catalog, struct dp_catalog_private, dp_catalog);

•   int isr = 0;
  

•   int isr, mask;
  
    isr = dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS);
    dp_write_aux(catalog, REG_DP_DP_HPD_INT_ACK,
                             (isr & DP_DP_HPD_INT_MASK));
  

•   mask = dp_read_aux(catalog, REG_DP_DP_HPD_INT_MASK);
  
  

•   return isr;
  

•   return isr & (mask | ~DP_DP_HPD_INT_MASK);
  

Please add a comment above this explaining what the goal of the above statement is. I guess it's something like this, though you might want to modify it to remove snark and insert the real reason unless you like being snarky:

/* * Report the raw status of all interrupts (AKA we still report the * interrupt as asserted even if it's masked) _except_ for HPD-related. * interrupts. We only report HPD-related interrupts if they're * unmasked. We do it this way because we thought it would be extra * confusing for readers of this code and we were bribed by Mordac to * confuse you. OK, maybe that's not true. We actually do it this way * because of <insert your compelling reason here>. */

Along the same lines as my comments in patch #1, I don't have a great feel for exactly when the various HPD bits are enabled / disabled and it feels like it need to be made super obvious / well documented. That being said, I'd be OK w/ that happening in the proposed cleanup.

-Doug

Hi,

On Thu, Apr 14, 2022 at 5:20 AM Sankeerth Billakanti quic_sbillaka@quicinc.com wrote:

The source device should ensure the sink is ready before proceeding to read the sink capability or perform any aux transactions. The sink will indicate its readiness by asserting the HPD line. The controller driver needs to wait for the hpd line to be asserted by the sink before it performs any aux transactions.

The eDP sink is assumed to be always connected. It needs 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 and only after that, the hpd line will be asserted.

Whereas for DP, the sink can be hotplugged and unplugged anytime. The hpd line gets asserted to indicate the sink is connected and ready. Hence there is no need to wait for the hpd line to be asserted for a DP sink.

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

It might be worth mentioning "after the cut" that we may eventually end up changing the rules if people like my proposal [1]. However, what your code is doing here for eDP is correct as things are currently intended to work and it would make sense to land it while we debate about whether we want to add the is_hpd_asserted() callback like my patch does.

[1] https://lore.kernel.org/r/20220408193536.RFC.3.Icf57bb12233a47727013c6ab69ee...

Changes in v7:

• add a comment to say why the wait si done for eDP
• correct the commit text

Changes in v6:

• Wait for hpd high only for eDP
• Split into smaller patches

drivers/gpu/drm/msm/dp/dp_aux.c | 21 ++++++++++++++++++++- drivers/gpu/drm/msm/dp/dp_aux.h | 3 ++- drivers/gpu/drm/msm/dp/dp_catalog.c | 13 +++++++++++++ drivers/gpu/drm/msm/dp/dp_catalog.h | 1 + drivers/gpu/drm/msm/dp/dp_display.c | 2 +- 5 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c index 6d36f63..cf0739f 100644 --- a/drivers/gpu/drm/msm/dp/dp_aux.c +++ b/drivers/gpu/drm/msm/dp/dp_aux.c @@ -36,6 +36,7 @@ struct dp_aux_private { bool initted; u32 offset; u32 segment;

•   bool is_edp;
  

Kinda nitty, but can you put it next to the other booleans? This will help with structure packing.

    struct drm_dp_aux dp_aux;

}; @@ -337,6 +338,22 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux, goto exit; }

•   /*
  

•    * For eDP it's important to give a reasonably long wait here for HPD
  

•    * to be asserted. This is because the panel driver may have _just_
  

•    * turned on the panel and then tried to do an AUX transfer. The panel
  

•    * driver has no way of knowing when the panel is ready, so it's up
  

•    * to us to wait. For DP we never get into this situation so let's
  

•    * avoid ever doing the extra long wait for DP.
  

•    */
  

•   if (aux->is_edp) {
  

•           ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog);
  

•           if (ret) {
  

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

•                   goto exit;
  

•           }
  

•   }
  

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

@@ -491,7 +508,8 @@ void dp_aux_unregister(struct drm_dp_aux *dp_aux) drm_dp_aux_unregister(dp_aux); }

-struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog) +struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog,

•                           bool is_edp)
  

nit: I think indentation rules for this file are that the type of the argument for the 2nd line should line up right under the 1st. Thus you should delete one tab character and insert 6 spaces before the "bool".

Similar in other places, like your header file.

Stuff above is all nits and this looks right to me. I'm happy with:

Reviewed-by: Douglas Anderson dianders@chromium.org

Hi,

On Thu, Apr 14, 2022 at 5:20 AM Sankeerth Billakanti quic_sbillaka@quicinc.com wrote:

The eDP controller does not have a reliable way keep panel powered on to read the sink capabilities. So, the controller driver cannot validate if a mode can be supported by the source. We will rely on the panel driver to populate only the supported modes for now.

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

drivers/gpu/drm/msm/dp/dp_display.c | 8 ++++++++ 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index c7277f0..0f18a16 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -998,6 +998,14 @@ enum drm_mode_status dp_bridge_mode_valid(struct drm_bridge *bridge, return -EINVAL; }

•   /*
  

•    * The eDP controller currently does not have a reliable way of
  

•    * enabling panel power to read sink capabilities. So, we rely
  

•    * on the panel driver to populate only supported modes for now.
  

•    */
  

•   if (dp->is_edp)
  

•           return MODE_OK;
  

As discussed out-of-band, I agree that this is the right thing for now and making this assumption won't break anything. In general the set of eDP panels is known ahead of time it's fairly unlikely someone would set things up so that a panel couldn't use the mode it was reporting.

Longer term we should figure out a way to solve this but it doesn't have to be today. To properly implement mode_valid() we've got to combine knowledge from the panel (mostly rates supported and number of lanes supported) with the controller (rates supported, number of lanes supported/hooked up on this board).

In any case:

Reviewed-by: Douglas Anderson dianders@chromium.org