[PATCH v6 0/8] Add support for the eDP panel over aux_bus

[PATCH v6 0/8] Add support for the eDP panel over aux_bus

[Sankeerth Billakanti]

This series adds support for generic eDP panel over aux_bus.

These changes are dependent on the following series in order:
https://patchwork.kernel.org/project/linux-arm-msm/list/?series=620127&state=*
https://patchwork.kernel.org/project/linux-arm-msm/list/?series=616587&state=*
https://patchwork.kernel.org/project/linux-arm-msm/list/?series=613654&state=*


Sankeerth Billakanti (8):
  drm/msm/dp: Add eDP support via aux_bus
  drm/msm/dp: wait for hpd high before aux transaction
  drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP
  drm/msm/dp: avoid handling masked interrupts
  drm/msm/dp: prevent multiple votes for dp resources
  drm/msm/dp: remove unnecessary delay during boot
  drm/msm/dp: Support edp/dp without hpd
  drm/msm/dp: Handle eDP mode_valid differently from dp

 drivers/gpu/drm/msm/dp/dp_aux.c     | 13 ++++-
 drivers/gpu/drm/msm/dp/dp_aux.h     |  3 +-
 drivers/gpu/drm/msm/dp/dp_catalog.c | 35 ++++++++++---
 drivers/gpu/drm/msm/dp/dp_catalog.h |  1 +
 drivers/gpu/drm/msm/dp/dp_display.c | 99 ++++++++++++++++++++++++++++++++++---
 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 +
 8 files changed, 143 insertions(+), 40 deletions(-)

-- 
2.7.4

[PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus

[Sankeerth Billakanti]

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

Changes in v6:
  - Remove initialization
  - Fix aux_bus node leak
  - Split the patches

 drivers/gpu/drm/msm/dp/dp_display.c | 54 +++++++++++++++++++++++++++++++++++--
 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, 60 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 382b3aa..e082d02 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"
@@ -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) {
@@ -1524,6 +1523,53 @@ 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_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);
+		of_node_put(aux_bus);
+	}
+
+	/*
+	 * 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 +1593,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_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) {
+		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..6317dce 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;
@@ -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..091ff41 100644
--- a/drivers/gpu/drm/msm/dp/dp_parser.h
+++ b/drivers/gpu/drm/msm/dp/dp_parser.h
@@ -140,5 +140,6 @@ struct dp_parser {
  * can be parsed using this module.
  */
 struct dp_parser *dp_parser_get(struct platform_device *pdev);
+int dp_parser_find_next_bridge(struct dp_parser *parser);
 
 #endif
-- 
2.7.4

Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus

[Dmitry Baryshkov]

On 30/03/2022 19:02, 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>
> ---
> 
> Changes in v6:
>    - Remove initialization
>    - Fix aux_bus node leak
>    - Split the patches
> 
>   drivers/gpu/drm/msm/dp/dp_display.c | 54 +++++++++++++++++++++++++++++++++++--
>   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, 60 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 382b3aa..e082d02 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"
> @@ -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) {
> @@ -1524,6 +1523,53 @@ 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_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);
> +		of_node_put(aux_bus);
> +	}
> +
> +	/*
> +	 * 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) {

The more I think about these conditions, the closer I dislike them (yes, 
I added this one in one of the patches). I'd suggest to change 
dp->connector_type to boolean 'is_edp' field use it in all conditions 
instead.

> +			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 +1593,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_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) {

And in this case we can also check dp_display->connector_type (or the 
suggested dp_display->is_edp) for the uniformity of the code.

> +		bridge->ops =
> +			DRM_BRIDGE_OP_DETECT |
> +			DRM_BRIDGE_OP_HPD |
> +			DRM_BRIDGE_OP_MODES;

I think OP_MODES should be used for eDP, shouldn't it?

> +	}
>   
>   	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..6317dce 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;
> @@ -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..091ff41 100644
> --- a/drivers/gpu/drm/msm/dp/dp_parser.h
> +++ b/drivers/gpu/drm/msm/dp/dp_parser.h
> @@ -140,5 +140,6 @@ struct dp_parser {
>    * can be parsed using this module.
>    */
>   struct dp_parser *dp_parser_get(struct platform_device *pdev);
> +int dp_parser_find_next_bridge(struct dp_parser *parser);
>   
>   #endif


-- 
With best wishes
Dmitry

Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus

[Doug Anderson]

Hi,

On Wed, Mar 30, 2022 at 4:19 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
> > +             bridge->ops =
> > +                     DRM_BRIDGE_OP_DETECT |
> > +                     DRM_BRIDGE_OP_HPD |
> > +                     DRM_BRIDGE_OP_MODES;
>
> I think OP_MODES should be used for eDP, shouldn't it?

No. It's confusing, but basically to get the power sequencing correct
we end up driving the EDID read from the panel driver in the eDP case.

-Doug

Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus

[Doug Anderson]

Hi,

On Wed, Mar 30, 2022 at 9:03 AM Sankeerth Billakanti
<quic_sbillaka@quicinc.com> wrote:
>
> @@ -1547,6 +1593,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;

It feels weird to me that this is in a function called "modeset_init",
though I certainly don't know the structure of the MSM display code
well enough to fully comment. My expectation would have been that
devm_of_dp_aux_populate_ep_devices() would have been called from your
probe routine and then you would have returned -EPROBE_DEFER from your
probe if you were unable to find the panel afterwards.

Huh, but I guess you _are_ getting called (indirectly) from
dpu_kms_hw_init() and I can't imagine AUX transfers working before
that function is called, so maybe I should just accept that it's
complicated and let those who understand this driver better confirm
that it's OK. ;-)


> @@ -140,5 +140,6 @@ struct dp_parser {
>   * can be parsed using this module.
>   */
>  struct dp_parser *dp_parser_get(struct platform_device *pdev);
> +int dp_parser_find_next_bridge(struct dp_parser *parser);

Everything else in this file is described w/ kerneldoc. Shouldn't your
function also have a kerneldoc comment?

Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus

[Dmitry Baryshkov]

On 01/04/2022 02:22, Doug Anderson wrote:
> Hi,
> 
> On Wed, Mar 30, 2022 at 9:03 AM Sankeerth Billakanti
> <quic_sbillaka@quicinc.com> wrote:
>>
>> @@ -1547,6 +1593,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;
> 
> It feels weird to me that this is in a function called "modeset_init",
> though I certainly don't know the structure of the MSM display code
> well enough to fully comment.

It's called modeset_init() as it initializes KMS objects used by DP 
driver. We have similar functions for dsi and hdmi

> My expectation would have been that
> devm_of_dp_aux_populate_ep_devices() would have been called from your
> probe routine and then you would have returned -EPROBE_DEFER from your
> probe if you were unable to find the panel afterwards.

I don't think it's possible to call it from probe() since 
drm_dp_aux_register() is called only from dp_display_bind().
The PHY also isn't initialized at that moment, so we can not probe AUX 
devices.

The overall semantics of the AUX bus is not clear to me.
Typically the bus is populated (and probed) when devices are accessible. 
But for the display related buses this might not be the case.
For example for the DSI bus we clearly define that DSI transfer are not 
possible before the corresponding bridge's (or panel's) enable call.

Maybe the same approach should be adopted for the AUX bus. This would 
allow us to populate the AUX bus before hardware access is actually 
possible, thus creating all the DRM bridges before the hardware is 
actually up and running.

> Huh, but I guess you _are_ getting called (indirectly) from
> dpu_kms_hw_init() and I can't imagine AUX transfers working before
> that function is called, so maybe I should just accept that it's
> complicated and let those who understand this driver better confirm
> that it's OK. ;-)
> 
> 
>> @@ -140,5 +140,6 @@ struct dp_parser {
>>    * can be parsed using this module.
>>    */
>>   struct dp_parser *dp_parser_get(struct platform_device *pdev);
>> +int dp_parser_find_next_bridge(struct dp_parser *parser);
> 
> Everything else in this file is described w/ kerneldoc. Shouldn't your
> function also have a kerneldoc comment?

-- 
With best wishes
Dmitry

Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus

[Doug Anderson]

Hi,

On Sat, Apr 2, 2022 at 3:37 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On 01/04/2022 02:22, Doug Anderson wrote:
> > Hi,
> >
> > On Wed, Mar 30, 2022 at 9:03 AM Sankeerth Billakanti
> > <quic_sbillaka@quicinc.com> wrote:
> >>
> >> @@ -1547,6 +1593,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;
> >
> > It feels weird to me that this is in a function called "modeset_init",
> > though I certainly don't know the structure of the MSM display code
> > well enough to fully comment.
>
> It's called modeset_init() as it initializes KMS objects used by DP
> driver. We have similar functions for dsi and hdmi

Sorry, I wasn't meaning to imply that modeset_init() was a bad name or
anything. Mostly saying that I wasn't sure that modeset init was the
proper time to populate the aux bus. ...but then again, perhaps it is
given the current structure of this driver?


> > My expectation would have been that
> > devm_of_dp_aux_populate_ep_devices() would have been called from your
> > probe routine and then you would have returned -EPROBE_DEFER from your
> > probe if you were unable to find the panel afterwards.
>
> I don't think it's possible to call it from probe() since
> drm_dp_aux_register() is called only from dp_display_bind().
> The PHY also isn't initialized at that moment, so we can not probe AUX
> devices.
>
> The overall semantics of the AUX bus is not clear to me.
> Typically the bus is populated (and probed) when devices are accessible.
> But for the display related buses this might not be the case.

In general the AUX bus is modeled much like the i2c bus. You probe the
sub-device when you're able to transfer. Then you can confirm that the
device is actually there and init the device.


> For example for the DSI bus we clearly define that DSI transfer are not
> possible before the corresponding bridge's (or panel's) enable call.
>
> Maybe the same approach should be adopted for the AUX bus. This would
> allow us to populate the AUX bus before hardware access is actually
> possible, thus creating all the DRM bridges before the hardware is
> actually up and running.

So I guess what you're proposing is that you could probe the devices
under the AUX bus and they could acquire resources (and possibly
return EPROBE_DEFER) at a point in time _before_ it's actually
possible to transfer. Then I guess you'd later do the transfer?

I guess conceivably one could re-design the DRM subsystem like that,
but I don't think it's trivial. Why? I believe that we need to know
things about the panel at probe time. For instance, we need to be able
to populate the panel's modes. To get this information we need the
EDID which means we need to be able to do a transfer. If we're using
an AUX backlight we also need to add info about the backlight at probe
time and that also needs the transfer to work.


So I guess the net result is maybe we should just keep it where it is.
Long term I'd be interested in knowing if there's a reason why we
can't structure the driver so that AUX transfers can happen with less
intertwining with the rest of the code, but that can happen later. I
would expect that you'd basically just need clocks and regulators on
and maybe your PHY on. Ideally with some pm_runtime fun we should be
able to do that independently with anything else the driver needs to
do?

-Doug

Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus

[Dmitry Baryshkov]

On Sat, 2 Apr 2022 at 20:06, Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Sat, Apr 2, 2022 at 3:37 AM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > On 01/04/2022 02:22, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Wed, Mar 30, 2022 at 9:03 AM Sankeerth Billakanti
> > > <quic_sbillaka@quicinc.com> wrote:
> > >>
> > >> @@ -1547,6 +1593,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;
> > >
> > > It feels weird to me that this is in a function called "modeset_init",
> > > though I certainly don't know the structure of the MSM display code
> > > well enough to fully comment.
> >
> > It's called modeset_init() as it initializes KMS objects used by DP
> > driver. We have similar functions for dsi and hdmi
>
> Sorry, I wasn't meaning to imply that modeset_init() was a bad name or
> anything. Mostly saying that I wasn't sure that modeset init was the
> proper time to populate the aux bus. ...but then again, perhaps it is
> given the current structure of this driver?
>
>
> > > My expectation would have been that
> > > devm_of_dp_aux_populate_ep_devices() would have been called from your
> > > probe routine and then you would have returned -EPROBE_DEFER from your
> > > probe if you were unable to find the panel afterwards.
> >
> > I don't think it's possible to call it from probe() since
> > drm_dp_aux_register() is called only from dp_display_bind().
> > The PHY also isn't initialized at that moment, so we can not probe AUX
> > devices.
> >
> > The overall semantics of the AUX bus is not clear to me.
> > Typically the bus is populated (and probed) when devices are accessible.
> > But for the display related buses this might not be the case.
>
> In general the AUX bus is modeled much like the i2c bus. You probe the
> sub-device when you're able to transfer. Then you can confirm that the
> device is actually there and init the device.
>
>
> > For example for the DSI bus we clearly define that DSI transfer are not
> > possible before the corresponding bridge's (or panel's) enable call.
> >
> > Maybe the same approach should be adopted for the AUX bus. This would
> > allow us to populate the AUX bus before hardware access is actually
> > possible, thus creating all the DRM bridges before the hardware is
> > actually up and running.
>
> So I guess what you're proposing is that you could probe the devices
> under the AUX bus and they could acquire resources (and possibly
> return EPROBE_DEFER) at a point in time _before_ it's actually
> possible to transfer. Then I guess you'd later do the transfer?

Exactly.

>
> I guess conceivably one could re-design the DRM subsystem like that,
> but I don't think it's trivial.

The problem is that the DRM subsystem is already designed like that.
All the bridge chains are static. They are created during the device
probe. And the modes are populated later (via the get_modes()
callback), after the HPD signal is delivered.
For the encoder/bridge chains it is explicitly stated that the display
pipe (clocks and timing signals) are not running before bridge's
enable() callback or after the disable() callback being called.

> Why? I believe that we need to know
> things about the panel at probe time. For instance, we need to be able
> to populate the panel's modes.

As I said, panel modes are not needed at the probe time. The fact that
most (if not all) of the panel drivers provide them in the platform
data (and thus modes are typically populated at the probe time) comes
from the fact that the panel is usually a known static piece of
hardware. With the generic edp-panel this is no longer the case. A
single device handles a (probed) variety of panels.

Compare it with the generic monitor:
We have a known bridge (display-connector.c), so the driver can build
the display chain. However a set of modes is not known till the actual
monitor is plugged into the device.

> To get this information we need the
> EDID which means we need to be able to do a transfer. If we're using
> an AUX backlight we also need to add info about the backlight at probe
> time and that also needs the transfer to work.

Yes, the backlight is the problem in the suggested design. I'm not
sure when panel->backlight has to  be populated for things to work.
If we can set it after the probe but before calling into
mode_set/drm_bridge_chain_enable(), then it should be fine.

> So I guess the net result is maybe we should just keep it where it is.
> Long term I'd be interested in knowing if there's a reason why we
> can't structure the driver so that AUX transfers can happen with less
> intertwining with the rest of the code, but that can happen later. I
> would expect that you'd basically just need clocks and regulators on
> and maybe your PHY on. Ideally with some pm_runtime fun we should be
> able to do that independently with anything else the driver needs to
> do?

Not really. The driver is shared between the DP and eDP. And the DP
(well, combo DP+USB-C) part has quite logical expectations that e.g.
AUX channel is not up until all negotiations about the USB-C altmodes
are done and the HPD event is delivered. This is the source for my
suggestion regarding AUX bus rework/redesign. For non-eDP cases the
connected device becomes known much later than the dp_parser code runs
(and much later than all the bridges are to be instantiated).

Another option would be to keep common drm/msm/dp core code and split
the actual driver code into two distinct code paths: one supporting
DP, another one supporting eDP. I think, up to now we were trying hard
to stay away from such a split.

-- 
With best wishes
Dmitry

Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus

[Doug Anderson]

Hi,

On Sat, Apr 2, 2022 at 1:26 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Sat, 2 Apr 2022 at 20:06, Doug Anderson <dianders@chromium.org> wrote:
> >
> > Hi,
> >
> > On Sat, Apr 2, 2022 at 3:37 AM Dmitry Baryshkov
> > <dmitry.baryshkov@linaro.org> wrote:
> > >
> > > On 01/04/2022 02:22, Doug Anderson wrote:
> > > > Hi,
> > > >
> > > > On Wed, Mar 30, 2022 at 9:03 AM Sankeerth Billakanti
> > > > <quic_sbillaka@quicinc.com> wrote:
> > > >>
> > > >> @@ -1547,6 +1593,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;
> > > >
> > > > It feels weird to me that this is in a function called "modeset_init",
> > > > though I certainly don't know the structure of the MSM display code
> > > > well enough to fully comment.
> > >
> > > It's called modeset_init() as it initializes KMS objects used by DP
> > > driver. We have similar functions for dsi and hdmi
> >
> > Sorry, I wasn't meaning to imply that modeset_init() was a bad name or
> > anything. Mostly saying that I wasn't sure that modeset init was the
> > proper time to populate the aux bus. ...but then again, perhaps it is
> > given the current structure of this driver?
> >
> >
> > > > My expectation would have been that
> > > > devm_of_dp_aux_populate_ep_devices() would have been called from your
> > > > probe routine and then you would have returned -EPROBE_DEFER from your
> > > > probe if you were unable to find the panel afterwards.
> > >
> > > I don't think it's possible to call it from probe() since
> > > drm_dp_aux_register() is called only from dp_display_bind().
> > > The PHY also isn't initialized at that moment, so we can not probe AUX
> > > devices.
> > >
> > > The overall semantics of the AUX bus is not clear to me.
> > > Typically the bus is populated (and probed) when devices are accessible.
> > > But for the display related buses this might not be the case.
> >
> > In general the AUX bus is modeled much like the i2c bus. You probe the
> > sub-device when you're able to transfer. Then you can confirm that the
> > device is actually there and init the device.
> >
> >
> > > For example for the DSI bus we clearly define that DSI transfer are not
> > > possible before the corresponding bridge's (or panel's) enable call.
> > >
> > > Maybe the same approach should be adopted for the AUX bus. This would
> > > allow us to populate the AUX bus before hardware access is actually
> > > possible, thus creating all the DRM bridges before the hardware is
> > > actually up and running.
> >
> > So I guess what you're proposing is that you could probe the devices
> > under the AUX bus and they could acquire resources (and possibly
> > return EPROBE_DEFER) at a point in time _before_ it's actually
> > possible to transfer. Then I guess you'd later do the transfer?
>
> Exactly.
>
> >
> > I guess conceivably one could re-design the DRM subsystem like that,
> > but I don't think it's trivial.
>
> The problem is that the DRM subsystem is already designed like that.
> All the bridge chains are static. They are created during the device
> probe. And the modes are populated later (via the get_modes()
> callback), after the HPD signal is delivered.
> For the encoder/bridge chains it is explicitly stated that the display
> pipe (clocks and timing signals) are not running before bridge's
> enable() callback or after the disable() callback being called.
>
> > Why? I believe that we need to know
> > things about the panel at probe time. For instance, we need to be able
> > to populate the panel's modes.
>
> As I said, panel modes are not needed at the probe time. The fact that
> most (if not all) of the panel drivers provide them in the platform
> data (and thus modes are typically populated at the probe time) comes
> from the fact that the panel is usually a known static piece of
> hardware. With the generic edp-panel this is no longer the case. A
> single device handles a (probed) variety of panels.

OK, so I misspoke when I said that the modes are populated during
probe time for edp-panel. They're not and I guess I managed to confuse
myself when I wrote my previous email. Instead they (and everything
else that comes from the EDID) isn't needed until the panel's
get_modes() is called, as you said. So, ignoring the backlight problem
talked about below, we could conceivably delay the reading of the EDID
until get_modes.

...but I think something still isn't quite right with your description
of how it works. Specifically:

1. I'm 99% certain that get_modes() is called _before_ enable, even
for the DP case. I have no idea how that works for you for DP if the
clocks / timing signals don't work until enable happens. Aside from my
previous observations of this, it also logically makes sense that
get_modes() needs to be called before enable(), doesn't it? When
enable() is called then don't you already know what mode userspace has
picked for you? How can userspace pick a mode to give to enable if you
can't query the modes until after enable?

2. As soon as you have told userspace that a display is present then,
I believe, it's legal for userspace to ask for the set of available
modes.

3. For DP and eDP HPD means something a little different. Essentially
there are two concepts: a) is a display physically connected and b) is
the display powered up and ready. For DP, the two are really tied
together. From the kernel's point of view you never "power down" a DP
display and you can't detect that it's physically connected until it's
ready. Said another way, on you tie "is a display there" to the HPD
line and the moment a display is there it's ready for you to do AUX
transfers. For eDP, in the lowest power state of a display it _won't_
assert its "HPD" signal. However, it's still physically present. For
eDP you simply have to _assume_ it's present without any actual proof
since you can't get proof until you power it up. Thus for eDP, you
report that the display is there as soon as we're asked. We can't
_talk_ to the display yet, though. So in get_modes() we need to be
able to power the display on enough to talk over the AUX channel to
it. As part of this, we wait for the signal named "HPD" which really
means "panel finished powering on" in this context.

NOTE: for aux transfer, we don't have the _display_ pipe and clocks
running. We only have enough stuff running to do the AUX transfer.
We're not clocking out pixels. We haven't fully powered on the
display. The AUX transfer is designed to be something that can be done
early _before_ you turn on the display.


OK, so basically that was a longwinded way of saying: yes, we could
avoid the AUX transfer in probe, but we can't wait all the way to
enable. We have to be able to transfer in get_modes(). If you think
that's helpful I think it'd be a pretty easy patch to write even if it
would look a tad bit awkward IMO. Let me know if you want me to post
it up.


> Compare it with the generic monitor:
> We have a known bridge (display-connector.c), so the driver can build
> the display chain. However a set of modes is not known till the actual
> monitor is plugged into the device.
>
> > To get this information we need the
> > EDID which means we need to be able to do a transfer. If we're using
> > an AUX backlight we also need to add info about the backlight at probe
> > time and that also needs the transfer to work.
>
> Yes, the backlight is the problem in the suggested design. I'm not
> sure when panel->backlight has to  be populated for things to work.
> If we can set it after the probe but before calling into
> mode_set/drm_bridge_chain_enable(), then it should be fine.

Actually we _probably_ can do this. Right now it only affects a small
subset of panels (those that use AUX backlight) and I can give it a
shot if this is the last blocker.

...this is even more awkward than the above, though, because the first
first call to get_modes() will actually _cause_ the backlight device
to show up. That's not super elegant but it might work OK?


> > So I guess the net result is maybe we should just keep it where it is.
> > Long term I'd be interested in knowing if there's a reason why we
> > can't structure the driver so that AUX transfers can happen with less
> > intertwining with the rest of the code, but that can happen later. I
> > would expect that you'd basically just need clocks and regulators on
> > and maybe your PHY on. Ideally with some pm_runtime fun we should be
> > able to do that independently with anything else the driver needs to
> > do?
>
> Not really. The driver is shared between the DP and eDP. And the DP
> (well, combo DP+USB-C) part has quite logical expectations that e.g.
> AUX channel is not up until all negotiations about the USB-C altmodes
> are done and the HPD event is delivered. This is the source for my
> suggestion regarding AUX bus rework/redesign. For non-eDP cases the
> connected device becomes known much later than the dp_parser code runs
> (and much later than all the bridges are to be instantiated).
>
> Another option would be to keep common drm/msm/dp core code and split
> the actual driver code into two distinct code paths: one supporting
> DP, another one supporting eDP. I think, up to now we were trying hard
> to stay away from such a split.

Even for eDP the AUX transfer shouldn't happen until after HPD is
asserted. That's why the AUX transfer function has to wait for HPD.
However, for eDP it's a requirement to register/create the AUX bus
before HPD is asserted. We went through lots of design discussions and
the end result of it all was that we pass the AUX bus to the panel at
the panel's probe time. This is something I don't think we want to
revisit.

Logically there are simply some things that will have to be different
for eDP and DP. It really stems from the case that in the lowest power
state of eDP that we truly can't tell if the panel is there and thus
we have to assume it's there. It also comes from the fact that, in
eDP, the panel driver is in charge of doing power sequencing across
several regulators / GPIOs including getting the timing right.


-Doug

Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus

[Dmitry Baryshkov]

On Mon, 4 Apr 2022 at 23:53, Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Sat, Apr 2, 2022 at 1:26 PM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > On Sat, 2 Apr 2022 at 20:06, Doug Anderson <dianders@chromium.org> wrote:
> > >
> > > Hi,
> > >
> > > On Sat, Apr 2, 2022 at 3:37 AM Dmitry Baryshkov
> > > <dmitry.baryshkov@linaro.org> wrote:
> > > >
> > > > On 01/04/2022 02:22, Doug Anderson wrote:
> > > > > Hi,
> > > > >
> > > > > On Wed, Mar 30, 2022 at 9:03 AM Sankeerth Billakanti
> > > > > <quic_sbillaka@quicinc.com> wrote:
> > > > >>
> > > > >> @@ -1547,6 +1593,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;
> > > > >
> > > > > It feels weird to me that this is in a function called "modeset_init",
> > > > > though I certainly don't know the structure of the MSM display code
> > > > > well enough to fully comment.
> > > >
> > > > It's called modeset_init() as it initializes KMS objects used by DP
> > > > driver. We have similar functions for dsi and hdmi
> > >
> > > Sorry, I wasn't meaning to imply that modeset_init() was a bad name or
> > > anything. Mostly saying that I wasn't sure that modeset init was the
> > > proper time to populate the aux bus. ...but then again, perhaps it is
> > > given the current structure of this driver?
> > >
> > >
> > > > > My expectation would have been that
> > > > > devm_of_dp_aux_populate_ep_devices() would have been called from your
> > > > > probe routine and then you would have returned -EPROBE_DEFER from your
> > > > > probe if you were unable to find the panel afterwards.
> > > >
> > > > I don't think it's possible to call it from probe() since
> > > > drm_dp_aux_register() is called only from dp_display_bind().
> > > > The PHY also isn't initialized at that moment, so we can not probe AUX
> > > > devices.
> > > >
> > > > The overall semantics of the AUX bus is not clear to me.
> > > > Typically the bus is populated (and probed) when devices are accessible.
> > > > But for the display related buses this might not be the case.
> > >
> > > In general the AUX bus is modeled much like the i2c bus. You probe the
> > > sub-device when you're able to transfer. Then you can confirm that the
> > > device is actually there and init the device.
> > >
> > >
> > > > For example for the DSI bus we clearly define that DSI transfer are not
> > > > possible before the corresponding bridge's (or panel's) enable call.
> > > >
> > > > Maybe the same approach should be adopted for the AUX bus. This would
> > > > allow us to populate the AUX bus before hardware access is actually
> > > > possible, thus creating all the DRM bridges before the hardware is
> > > > actually up and running.
> > >
> > > So I guess what you're proposing is that you could probe the devices
> > > under the AUX bus and they could acquire resources (and possibly
> > > return EPROBE_DEFER) at a point in time _before_ it's actually
> > > possible to transfer. Then I guess you'd later do the transfer?
> >
> > Exactly.
> >
> > >
> > > I guess conceivably one could re-design the DRM subsystem like that,
> > > but I don't think it's trivial.
> >
> > The problem is that the DRM subsystem is already designed like that.
> > All the bridge chains are static. They are created during the device
> > probe. And the modes are populated later (via the get_modes()
> > callback), after the HPD signal is delivered.
> > For the encoder/bridge chains it is explicitly stated that the display
> > pipe (clocks and timing signals) are not running before bridge's
> > enable() callback or after the disable() callback being called.
> >
> > > Why? I believe that we need to know
> > > things about the panel at probe time. For instance, we need to be able
> > > to populate the panel's modes.
> >
> > As I said, panel modes are not needed at the probe time. The fact that
> > most (if not all) of the panel drivers provide them in the platform
> > data (and thus modes are typically populated at the probe time) comes
> > from the fact that the panel is usually a known static piece of
> > hardware. With the generic edp-panel this is no longer the case. A
> > single device handles a (probed) variety of panels.
>
> OK, so I misspoke when I said that the modes are populated during
> probe time for edp-panel. They're not and I guess I managed to confuse
> myself when I wrote my previous email. Instead they (and everything
> else that comes from the EDID) isn't needed until the panel's
> get_modes() is called, as you said. So, ignoring the backlight problem
> talked about below, we could conceivably delay the reading of the EDID
> until get_modes.

Yes.

>
> ...but I think something still isn't quite right with your description
> of how it works. Specifically:
>
> 1. I'm 99% certain that get_modes() is called _before_ enable, even
> for the DP case. I have no idea how that works for you for DP if the
> clocks / timing signals don't work until enable happens. Aside from my
> previous observations of this, it also logically makes sense that
> get_modes() needs to be called before enable(), doesn't it?

Yes, you are correct here. I also wasn't clear enough about the
display clocks/aux clocks being enabled.
Of course, it's the display (video) mode clocks not being provided
before the enable() call. (well, before the moment between
prepare()/pre_enable() and enable()).

The side channel buses are a separate story.

> When
> enable() is called then don't you already know what mode userspace has
> picked for you? How can userspace pick a mode to give to enable if you
> can't query the modes until after enable?

Yes. You are correct here.

>
> 2. As soon as you have told userspace that a display is present then,
> I believe, it's legal for userspace to ask for the set of available
> modes.

Yes.

>
> 3. For DP and eDP HPD means something a little different. Essentially
> there are two concepts: a) is a display physically connected and b) is
> the display powered up and ready. For DP, the two are really tied
> together. From the kernel's point of view you never "power down" a DP
> display and you can't detect that it's physically connected until it's
> ready. Said another way, on you tie "is a display there" to the HPD
> line and the moment a display is there it's ready for you to do AUX
> transfers. For eDP, in the lowest power state of a display it _won't_
> assert its "HPD" signal. However, it's still physically present. For
> eDP you simply have to _assume_ it's present without any actual proof
> since you can't get proof until you power it up. Thus for eDP, you
> report that the display is there as soon as we're asked. We can't
> _talk_ to the display yet, though. So in get_modes() we need to be
> able to power the display on enough to talk over the AUX channel to
> it. As part of this, we wait for the signal named "HPD" which really
> means "panel finished powering on" in this context.
>
> NOTE: for aux transfer, we don't have the _display_ pipe and clocks
> running. We only have enough stuff running to do the AUX transfer.
> We're not clocking out pixels. We haven't fully powered on the
> display. The AUX transfer is designed to be something that can be done
> early _before_ you turn on the display.
>
>
> OK, so basically that was a longwinded way of saying: yes, we could
> avoid the AUX transfer in probe, but we can't wait all the way to
> enable. We have to be able to transfer in get_modes(). If you think
> that's helpful I think it'd be a pretty easy patch to write even if it
> would look a tad bit awkward IMO. Let me know if you want me to post
> it up.

I think it would be a good idea. At least it will allow us to judge,
which is the more correct way.
And I also think it might help the ti,sn65dsi86 driver, as it won't
have to ensure that gpio is available during the AUX bus probe.

BTW, another random idea, before you start coding.

We have the bridge's hpd_notify call. Currently it is called only by
the means of drm_bridge_connector's HPD mechanism, tied to the bridge
registering as DRM_BRIDGE_OP_HPD.
It looks to me like it might be a perfect fit for the first aux-bus
related reads.

We'd need to trigger it manually once and tie it to the new
drm_panel_funcs callback, which in turn would probe the aux bus,
create backlight, etc.

Regarding the Sankeerth's patch. I have been comparing it with the
hpd_event_thread()'s calls.
It looks to me like we should reuse dp_display_config_hpd()
/EV_HPD_INIT_SETUP and maybe others.

What I'm trying to say is that if we split AUX probing and first AUX
transfers, it would be possible to reuse a significant part of MSM DP
HPD machine rather than hacking around it and replicating it manually.

>
>
> > Compare it with the generic monitor:
> > We have a known bridge (display-connector.c), so the driver can build
> > the display chain. However a set of modes is not known till the actual
> > monitor is plugged into the device.
> >
> > > To get this information we need the
> > > EDID which means we need to be able to do a transfer. If we're using
> > > an AUX backlight we also need to add info about the backlight at probe
> > > time and that also needs the transfer to work.
> >
> > Yes, the backlight is the problem in the suggested design. I'm not
> > sure when panel->backlight has to  be populated for things to work.
> > If we can set it after the probe but before calling into
> > mode_set/drm_bridge_chain_enable(), then it should be fine.
>
> Actually we _probably_ can do this. Right now it only affects a small
> subset of panels (those that use AUX backlight) and I can give it a
> shot if this is the last blocker.
>
> ...this is even more awkward than the above, though, because the first
> first call to get_modes() will actually _cause_ the backlight device
> to show up. That's not super elegant but it might work OK?

This is really an interesting question. See the hpd_notify suggestion above.


>
>
> > > So I guess the net result is maybe we should just keep it where it is.
> > > Long term I'd be interested in knowing if there's a reason why we
> > > can't structure the driver so that AUX transfers can happen with less
> > > intertwining with the rest of the code, but that can happen later. I
> > > would expect that you'd basically just need clocks and regulators on
> > > and maybe your PHY on. Ideally with some pm_runtime fun we should be
> > > able to do that independently with anything else the driver needs to
> > > do?
> >
> > Not really. The driver is shared between the DP and eDP. And the DP
> > (well, combo DP+USB-C) part has quite logical expectations that e.g.
> > AUX channel is not up until all negotiations about the USB-C altmodes
> > are done and the HPD event is delivered. This is the source for my
> > suggestion regarding AUX bus rework/redesign. For non-eDP cases the
> > connected device becomes known much later than the dp_parser code runs
> > (and much later than all the bridges are to be instantiated).
> >
> > Another option would be to keep common drm/msm/dp core code and split
> > the actual driver code into two distinct code paths: one supporting
> > DP, another one supporting eDP. I think, up to now we were trying hard
> > to stay away from such a split.
>
> Even for eDP the AUX transfer shouldn't happen until after HPD is
> asserted. That's why the AUX transfer function has to wait for HPD.
> However, for eDP it's a requirement to register/create the AUX bus
> before HPD is asserted. We went through lots of design discussions and
> the end result of it all was that we pass the AUX bus to the panel at
> the panel's probe time. This is something I don't think we want to
> revisit.
>
> Logically there are simply some things that will have to be different
> for eDP and DP. It really stems from the case that in the lowest power
> state of eDP that we truly can't tell if the panel is there and thus
> we have to assume it's there. It also comes from the fact that, in
> eDP, the panel driver is in charge of doing power sequencing across
> several regulators / GPIOs including getting the timing right.
>
>
> -Doug



--
With best wishes
Dmitry

Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus

[Doug Anderson]

Hi,

On Tue, Apr 5, 2022 at 5:54 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
> > 3. For DP and eDP HPD means something a little different. Essentially
> > there are two concepts: a) is a display physically connected and b) is
> > the display powered up and ready. For DP, the two are really tied
> > together. From the kernel's point of view you never "power down" a DP
> > display and you can't detect that it's physically connected until it's
> > ready. Said another way, on you tie "is a display there" to the HPD
> > line and the moment a display is there it's ready for you to do AUX
> > transfers. For eDP, in the lowest power state of a display it _won't_
> > assert its "HPD" signal. However, it's still physically present. For
> > eDP you simply have to _assume_ it's present without any actual proof
> > since you can't get proof until you power it up. Thus for eDP, you
> > report that the display is there as soon as we're asked. We can't
> > _talk_ to the display yet, though. So in get_modes() we need to be
> > able to power the display on enough to talk over the AUX channel to
> > it. As part of this, we wait for the signal named "HPD" which really
> > means "panel finished powering on" in this context.
> >
> > NOTE: for aux transfer, we don't have the _display_ pipe and clocks
> > running. We only have enough stuff running to do the AUX transfer.
> > We're not clocking out pixels. We haven't fully powered on the
> > display. The AUX transfer is designed to be something that can be done
> > early _before_ you turn on the display.
> >
> >
> > OK, so basically that was a longwinded way of saying: yes, we could
> > avoid the AUX transfer in probe, but we can't wait all the way to
> > enable. We have to be able to transfer in get_modes(). If you think
> > that's helpful I think it'd be a pretty easy patch to write even if it
> > would look a tad bit awkward IMO. Let me know if you want me to post
> > it up.
>
> I think it would be a good idea. At least it will allow us to judge,
> which is the more correct way.

I'm still happy to prototype this, but the more I think about it the
more it feels like a workaround for the Qualcomm driver. The eDP panel
driver is actually given a pointer to the AUX bus at probe time. It's
really weird to say that we can't do a transfer on it yet... As you
said, this is a little sideband bus. It should be able to be used
without all the full blown infra of the rest of the driver.


> And I also think it might help the ti,sn65dsi86 driver, as it won't
> have to ensure that gpio is available during the AUX bus probe.

The ti,sn65dsi86 GPIO issue has been solved for a while, though so not
sure why we need to do something there? I'm also unclear how it would
have helped. In this discussion, we've agreed that the panel driver
would still acquire resources during its probe time and the only thing
that would be delayed would be the first AUX transfer. The GPIO is a
resource here and it's ideal to acquire it at probe time so we could
EPROBE_DEFER if needed.


> BTW, another random idea, before you start coding.
>
> We have the bridge's hpd_notify call. Currently it is called only by
> the means of drm_bridge_connector's HPD mechanism, tied to the bridge
> registering as DRM_BRIDGE_OP_HPD.
> It looks to me like it might be a perfect fit for the first aux-bus
> related reads.
>
> We'd need to trigger it manually once and tie it to the new
> drm_panel_funcs callback, which in turn would probe the aux bus,
> create backlight, etc.
>
> Regarding the Sankeerth's patch. I have been comparing it with the
> hpd_event_thread()'s calls.
> It looks to me like we should reuse dp_display_config_hpd()
> /EV_HPD_INIT_SETUP and maybe others.
>
> What I'm trying to say is that if we split AUX probing and first AUX
> transfers, it would be possible to reuse a significant part of MSM DP
> HPD machine rather than hacking around it and replicating it manually.

I'm not sure I completely understand, but I'm pretty wary here. It's
my assertion that all of the current "HPD" infrastructure in DRM all
relates to the physical presence of the panel. If you start
implementing these functions for eDP I think you're going to confuse
the heck out of everything. The kernel will think that this is a
display that's sometimes not there. Whenever the display is powered
off then HPD will be low and it will look like there's no display.
Nothing will ever try to power it on because it looks like there's no
display.

I think your idea is to "trigger once" at bootup and then it all
magically works, right? ...but what about after bootup? If you turn
the display off for whatever reason (modeset or you simply close the
lid of your laptop because you're using an external display) and then
you want to use the eDP display again, how do you kickstart the
process another time? You can't reboot, and when the display is off
the HPD line is low.

I can't say it enough times, HPD on eDP _does not mean hot plug
detect_. The panel is always there. HPD is really a "panel ready /
panel notify" signal for eDP. That's fully what its function is.

-Doug

Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus

[Dmitry Baryshkov]

On 05/04/2022 20:02, Doug Anderson wrote:
> Hi,
> 
> On Tue, Apr 5, 2022 at 5:54 AM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
>>> 3. For DP and eDP HPD means something a little different. Essentially
>>> there are two concepts: a) is a display physically connected and b) is
>>> the display powered up and ready. For DP, the two are really tied
>>> together. From the kernel's point of view you never "power down" a DP
>>> display and you can't detect that it's physically connected until it's
>>> ready. Said another way, on you tie "is a display there" to the HPD
>>> line and the moment a display is there it's ready for you to do AUX
>>> transfers. For eDP, in the lowest power state of a display it _won't_
>>> assert its "HPD" signal. However, it's still physically present. For
>>> eDP you simply have to _assume_ it's present without any actual proof
>>> since you can't get proof until you power it up. Thus for eDP, you
>>> report that the display is there as soon as we're asked. We can't
>>> _talk_ to the display yet, though. So in get_modes() we need to be
>>> able to power the display on enough to talk over the AUX channel to
>>> it. As part of this, we wait for the signal named "HPD" which really
>>> means "panel finished powering on" in this context.
>>>
>>> NOTE: for aux transfer, we don't have the _display_ pipe and clocks
>>> running. We only have enough stuff running to do the AUX transfer.
>>> We're not clocking out pixels. We haven't fully powered on the
>>> display. The AUX transfer is designed to be something that can be done
>>> early _before_ you turn on the display.
>>>
>>>
>>> OK, so basically that was a longwinded way of saying: yes, we could
>>> avoid the AUX transfer in probe, but we can't wait all the way to
>>> enable. We have to be able to transfer in get_modes(). If you think
>>> that's helpful I think it'd be a pretty easy patch to write even if it
>>> would look a tad bit awkward IMO. Let me know if you want me to post
>>> it up.
>>
>> I think it would be a good idea. At least it will allow us to judge,
>> which is the more correct way.
> 
> I'm still happy to prototype this, but the more I think about it the
> more it feels like a workaround for the Qualcomm driver. The eDP panel
> driver is actually given a pointer to the AUX bus at probe time. It's
> really weird to say that we can't do a transfer on it yet... As you
> said, this is a little sideband bus. It should be able to be used
> without all the full blown infra of the rest of the driver.

Yes, I have that feeling too. However I also have a feeling that just 
powering up the PHY before the bus probe is ... a hack. There are no 
obvious stopgaps for the driver not to power it down later.

A cleaner design might be to split all hotplug event handling from the 
dp_display, provide a lightweight state machine for the eDP and select 
which state machine to use depending on the hardware connector type. The 
dp_display_bind/unbind would probably also be duplicated and receive 
correct code flows for calling dp_parser_get_next_bridge, etc.
Basically that means that depending on the device data we'd use either 
dp_display_comp_ops or (new) edp_comp_ops.

WDYT?

>> And I also think it might help the ti,sn65dsi86 driver, as it won't
>> have to ensure that gpio is available during the AUX bus probe.
> 
> The ti,sn65dsi86 GPIO issue has been solved for a while, though so not
> sure why we need to do something there? I'm also unclear how it would
> have helped. In this discussion, we've agreed that the panel driver
> would still acquire resources during its probe time and the only thing
> that would be delayed would be the first AUX transfer. The GPIO is a
> resource here and it's ideal to acquire it at probe time so we could
> EPROBE_DEFER if needed.

Ack. I agree here. The world at 5 a.m. looks differently :D

> 
> 
>> BTW, another random idea, before you start coding.
>>
>> We have the bridge's hpd_notify call. Currently it is called only by
>> the means of drm_bridge_connector's HPD mechanism, tied to the bridge
>> registering as DRM_BRIDGE_OP_HPD.
>> It looks to me like it might be a perfect fit for the first aux-bus
>> related reads.
>>
>> We'd need to trigger it manually once and tie it to the new
>> drm_panel_funcs callback, which in turn would probe the aux bus,
>> create backlight, etc.
>>
>> Regarding the Sankeerth's patch. I have been comparing it with the
>> hpd_event_thread()'s calls.
>> It looks to me like we should reuse dp_display_config_hpd()
>> /EV_HPD_INIT_SETUP and maybe others.
>>
>> What I'm trying to say is that if we split AUX probing and first AUX
>> transfers, it would be possible to reuse a significant part of MSM DP
>> HPD machine rather than hacking around it and replicating it manually.
> 
> I'm not sure I completely understand, but I'm pretty wary here. It's
> my assertion that all of the current "HPD" infrastructure in DRM all
> relates to the physical presence of the panel. If you start
> implementing these functions for eDP I think you're going to confuse
> the heck out of everything. The kernel will think that this is a
> display that's sometimes not there. Whenever the display is powered
> off then HPD will be low and it will look like there's no display.
> Nothing will ever try to power it on because it looks like there's no
> display.
> 
> I think your idea is to "trigger once" at bootup and then it all
> magically works, right? ...but what about after bootup? If you turn
> the display off for whatever reason (modeset or you simply close the
> lid of your laptop because you're using an external display) and then
> you want to use the eDP display again, how do you kickstart the
> process another time? You can't reboot, and when the display is off
> the HPD line is low.
> 
> I can't say it enough times, HPD on eDP _does not mean hot plug
> detect_. The panel is always there. HPD is really a "panel ready /
> panel notify" signal for eDP. That's fully what its function is.

Too many things being called HPD. I meant to trigger drm's hpd handling, 
which is not tied to the HPD pin for panel-edp. HPD pin is checked 
during panel runtime resume. However... let's probably go a simpler way.

-- 
With best wishes
Dmitry

Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus

[Doug Anderson]

Hi,

On Tue, Apr 5, 2022 at 10:36 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On 05/04/2022 20:02, Doug Anderson wrote:
> > Hi,
> >
> > On Tue, Apr 5, 2022 at 5:54 AM Dmitry Baryshkov
> > <dmitry.baryshkov@linaro.org> wrote:
> >>> 3. For DP and eDP HPD means something a little different. Essentially
> >>> there are two concepts: a) is a display physically connected and b) is
> >>> the display powered up and ready. For DP, the two are really tied
> >>> together. From the kernel's point of view you never "power down" a DP
> >>> display and you can't detect that it's physically connected until it's
> >>> ready. Said another way, on you tie "is a display there" to the HPD
> >>> line and the moment a display is there it's ready for you to do AUX
> >>> transfers. For eDP, in the lowest power state of a display it _won't_
> >>> assert its "HPD" signal. However, it's still physically present. For
> >>> eDP you simply have to _assume_ it's present without any actual proof
> >>> since you can't get proof until you power it up. Thus for eDP, you
> >>> report that the display is there as soon as we're asked. We can't
> >>> _talk_ to the display yet, though. So in get_modes() we need to be
> >>> able to power the display on enough to talk over the AUX channel to
> >>> it. As part of this, we wait for the signal named "HPD" which really
> >>> means "panel finished powering on" in this context.
> >>>
> >>> NOTE: for aux transfer, we don't have the _display_ pipe and clocks
> >>> running. We only have enough stuff running to do the AUX transfer.
> >>> We're not clocking out pixels. We haven't fully powered on the
> >>> display. The AUX transfer is designed to be something that can be done
> >>> early _before_ you turn on the display.
> >>>
> >>>
> >>> OK, so basically that was a longwinded way of saying: yes, we could
> >>> avoid the AUX transfer in probe, but we can't wait all the way to
> >>> enable. We have to be able to transfer in get_modes(). If you think
> >>> that's helpful I think it'd be a pretty easy patch to write even if it
> >>> would look a tad bit awkward IMO. Let me know if you want me to post
> >>> it up.
> >>
> >> I think it would be a good idea. At least it will allow us to judge,
> >> which is the more correct way.
> >
> > I'm still happy to prototype this, but the more I think about it the
> > more it feels like a workaround for the Qualcomm driver. The eDP panel
> > driver is actually given a pointer to the AUX bus at probe time. It's
> > really weird to say that we can't do a transfer on it yet... As you
> > said, this is a little sideband bus. It should be able to be used
> > without all the full blown infra of the rest of the driver.
>
> Yes, I have that feeling too. However I also have a feeling that just
> powering up the PHY before the bus probe is ... a hack. There are no
> obvious stopgaps for the driver not to power it down later.

This is why I think we need to move to Runtime PM to manage this. Basically:

1. When an AUX transfer happens, you grab a PM runtime reference that
_that_ powers up the PHY.

2. At the end of the AUX transfer function, you do a "put_autosuspend".

Then it becomes not a hack, right?


> A cleaner design might be to split all hotplug event handling from the
> dp_display, provide a lightweight state machine for the eDP and select
> which state machine to use depending on the hardware connector type. The
> dp_display_bind/unbind would probably also be duplicated and receive
> correct code flows for calling dp_parser_get_next_bridge, etc.
> Basically that means that depending on the device data we'd use either
> dp_display_comp_ops or (new) edp_comp_ops.
>
> WDYT?

I don't think I know the structure of the MSM DP code to make a
definitive answer here. I think I'd have to see a patch. However I'd
agree in general terms that we need some different flows for the two.
;-) We definitely want to limit the differences but some of them will
be unavoidable...


-Doug

RE: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus

[Sankeerth Billakanti (QUIC)]

Hi Dmitry and Doug,

> Hi,
> 
> On Tue, Apr 5, 2022 at 10:36 AM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > On 05/04/2022 20:02, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Tue, Apr 5, 2022 at 5:54 AM Dmitry Baryshkov
> > > <dmitry.baryshkov@linaro.org> wrote:
> > >>> 3. For DP and eDP HPD means something a little different.
> > >>> Essentially there are two concepts: a) is a display physically
> > >>> connected and b) is the display powered up and ready. For DP, the
> > >>> two are really tied together. From the kernel's point of view you
> > >>> never "power down" a DP display and you can't detect that it's
> > >>> physically connected until it's ready. Said another way, on you
> > >>> tie "is a display there" to the HPD line and the moment a display
> > >>> is there it's ready for you to do AUX transfers. For eDP, in the
> > >>> lowest power state of a display it _won't_ assert its "HPD"
> > >>> signal. However, it's still physically present. For eDP you simply
> > >>> have to _assume_ it's present without any actual proof since you
> > >>> can't get proof until you power it up. Thus for eDP, you report
> > >>> that the display is there as soon as we're asked. We can't _talk_
> > >>> to the display yet, though. So in get_modes() we need to be able
> > >>> to power the display on enough to talk over the AUX channel to it.
> > >>> As part of this, we wait for the signal named "HPD" which really means
> "panel finished powering on" in this context.
> > >>>
> > >>> NOTE: for aux transfer, we don't have the _display_ pipe and
> > >>> clocks running. We only have enough stuff running to do the AUX
> transfer.
> > >>> We're not clocking out pixels. We haven't fully powered on the
> > >>> display. The AUX transfer is designed to be something that can be
> > >>> done early _before_ you turn on the display.
> > >>>
> > >>>
> > >>> OK, so basically that was a longwinded way of saying: yes, we
> > >>> could avoid the AUX transfer in probe, but we can't wait all the
> > >>> way to enable. We have to be able to transfer in get_modes(). If
> > >>> you think that's helpful I think it'd be a pretty easy patch to
> > >>> write even if it would look a tad bit awkward IMO. Let me know if
> > >>> you want me to post it up.
> > >>
> > >> I think it would be a good idea. At least it will allow us to
> > >> judge, which is the more correct way.
> > >
> > > I'm still happy to prototype this, but the more I think about it the
> > > more it feels like a workaround for the Qualcomm driver. The eDP
> > > panel driver is actually given a pointer to the AUX bus at probe
> > > time. It's really weird to say that we can't do a transfer on it
> > > yet... As you said, this is a little sideband bus. It should be able
> > > to be used without all the full blown infra of the rest of the driver.
> >
> > Yes, I have that feeling too. However I also have a feeling that just
> > powering up the PHY before the bus probe is ... a hack. There are no
> > obvious stopgaps for the driver not to power it down later.
> 
> This is why I think we need to move to Runtime PM to manage this. Basically:
> 
> 1. When an AUX transfer happens, you grab a PM runtime reference that
> _that_ powers up the PHY.
> 
> 2. At the end of the AUX transfer function, you do a "put_autosuspend".
> 
> Then it becomes not a hack, right?
> 
> 

pm runtime ops needs to be implemented for both eDP and DP. This change
take good amount of planning and code changes as it affects DP also.

Because this patch series consist of basic eDP changes for SC7280 bootup,
shall we take this pm_runtime implementation in subsequent patch series?

> > A cleaner design might be to split all hotplug event handling from the
> > dp_display, provide a lightweight state machine for the eDP and select
> > which state machine to use depending on the hardware connector type.
> > The dp_display_bind/unbind would probably also be duplicated and
> > receive correct code flows for calling dp_parser_get_next_bridge, etc.
> > Basically that means that depending on the device data we'd use either
> > dp_display_comp_ops or (new) edp_comp_ops.
> >
> > WDYT?
> 
> I don't think I know the structure of the MSM DP code to make a definitive
> answer here. I think I'd have to see a patch. However I'd agree in general
> terms that we need some different flows for the two.
> ;-) We definitely want to limit the differences but some of them will be
> unavoidable...
> 
> 
> -Doug

Thank you,
Sankeerth

Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus

[Doug Anderson]

Hi,

On Thu, Apr 7, 2022 at 7:19 AM Sankeerth Billakanti (QUIC)
<quic_sbillaka@quicinc.com> wrote:
>
> Hi Dmitry and Doug,
>
> > Hi,
> >
> > On Tue, Apr 5, 2022 at 10:36 AM Dmitry Baryshkov
> > <dmitry.baryshkov@linaro.org> wrote:
> > >
> > > On 05/04/2022 20:02, Doug Anderson wrote:
> > > > Hi,
> > > >
> > > > On Tue, Apr 5, 2022 at 5:54 AM Dmitry Baryshkov
> > > > <dmitry.baryshkov@linaro.org> wrote:
> > > >>> 3. For DP and eDP HPD means something a little different.
> > > >>> Essentially there are two concepts: a) is a display physically
> > > >>> connected and b) is the display powered up and ready. For DP, the
> > > >>> two are really tied together. From the kernel's point of view you
> > > >>> never "power down" a DP display and you can't detect that it's
> > > >>> physically connected until it's ready. Said another way, on you
> > > >>> tie "is a display there" to the HPD line and the moment a display
> > > >>> is there it's ready for you to do AUX transfers. For eDP, in the
> > > >>> lowest power state of a display it _won't_ assert its "HPD"
> > > >>> signal. However, it's still physically present. For eDP you simply
> > > >>> have to _assume_ it's present without any actual proof since you
> > > >>> can't get proof until you power it up. Thus for eDP, you report
> > > >>> that the display is there as soon as we're asked. We can't _talk_
> > > >>> to the display yet, though. So in get_modes() we need to be able
> > > >>> to power the display on enough to talk over the AUX channel to it.
> > > >>> As part of this, we wait for the signal named "HPD" which really means
> > "panel finished powering on" in this context.
> > > >>>
> > > >>> NOTE: for aux transfer, we don't have the _display_ pipe and
> > > >>> clocks running. We only have enough stuff running to do the AUX
> > transfer.
> > > >>> We're not clocking out pixels. We haven't fully powered on the
> > > >>> display. The AUX transfer is designed to be something that can be
> > > >>> done early _before_ you turn on the display.
> > > >>>
> > > >>>
> > > >>> OK, so basically that was a longwinded way of saying: yes, we
> > > >>> could avoid the AUX transfer in probe, but we can't wait all the
> > > >>> way to enable. We have to be able to transfer in get_modes(). If
> > > >>> you think that's helpful I think it'd be a pretty easy patch to
> > > >>> write even if it would look a tad bit awkward IMO. Let me know if
> > > >>> you want me to post it up.
> > > >>
> > > >> I think it would be a good idea. At least it will allow us to
> > > >> judge, which is the more correct way.
> > > >
> > > > I'm still happy to prototype this, but the more I think about it the
> > > > more it feels like a workaround for the Qualcomm driver. The eDP
> > > > panel driver is actually given a pointer to the AUX bus at probe
> > > > time. It's really weird to say that we can't do a transfer on it
> > > > yet... As you said, this is a little sideband bus. It should be able
> > > > to be used without all the full blown infra of the rest of the driver.
> > >
> > > Yes, I have that feeling too. However I also have a feeling that just
> > > powering up the PHY before the bus probe is ... a hack. There are no
> > > obvious stopgaps for the driver not to power it down later.
> >
> > This is why I think we need to move to Runtime PM to manage this. Basically:
> >
> > 1. When an AUX transfer happens, you grab a PM runtime reference that
> > _that_ powers up the PHY.
> >
> > 2. At the end of the AUX transfer function, you do a "put_autosuspend".
> >
> > Then it becomes not a hack, right?
> >
> >
>
> pm runtime ops needs to be implemented for both eDP and DP. This change
> take good amount of planning and code changes as it affects DP also.
>
> Because this patch series consist of basic eDP changes for SC7280 bootup,
> shall we take this pm_runtime implementation in subsequent patch series?

Dmitry is the real decision maker here, but in my opinion it would be
OK to get something landed first that worked OK and wasn't taking us
too far in the wrong direction and then we could get a follow up patch
to move to pm_runtime.

Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus

[Abhinav Kumar]

Hi Doug and Dmitry

Sorry, but I caught up on this email just now.

Some comments below.

Thanks

Abhinav
On 4/7/2022 10:07 AM, Doug Anderson wrote:
> Hi,
> 
> On Thu, Apr 7, 2022 at 7:19 AM Sankeerth Billakanti (QUIC)
> <quic_sbillaka@quicinc.com> wrote:
>>
>> Hi Dmitry and Doug,
>>
>>> Hi,
>>>
>>> On Tue, Apr 5, 2022 at 10:36 AM Dmitry Baryshkov
>>> <dmitry.baryshkov@linaro.org> wrote:
>>>>
>>>> On 05/04/2022 20:02, Doug Anderson wrote:
>>>>> Hi,
>>>>>
>>>>> On Tue, Apr 5, 2022 at 5:54 AM Dmitry Baryshkov
>>>>> <dmitry.baryshkov@linaro.org> wrote:
>>>>>>> 3. For DP and eDP HPD means something a little different.
>>>>>>> Essentially there are two concepts: a) is a display physically
>>>>>>> connected and b) is the display powered up and ready. For DP, the
>>>>>>> two are really tied together. From the kernel's point of view you
>>>>>>> never "power down" a DP display and you can't detect that it's
>>>>>>> physically connected until it's ready. Said another way, on you
>>>>>>> tie "is a display there" to the HPD line and the moment a display
>>>>>>> is there it's ready for you to do AUX transfers. For eDP, in the
>>>>>>> lowest power state of a display it _won't_ assert its "HPD"
>>>>>>> signal. However, it's still physically present. For eDP you simply
>>>>>>> have to _assume_ it's present without any actual proof since you
>>>>>>> can't get proof until you power it up. Thus for eDP, you report
>>>>>>> that the display is there as soon as we're asked. We can't _talk_
>>>>>>> to the display yet, though. So in get_modes() we need to be able
>>>>>>> to power the display on enough to talk over the AUX channel to it.
>>>>>>> As part of this, we wait for the signal named "HPD" which really means
>>> "panel finished powering on" in this context.
>>>>>>>
>>>>>>> NOTE: for aux transfer, we don't have the _display_ pipe and
>>>>>>> clocks running. We only have enough stuff running to do the AUX
>>> transfer.
>>>>>>> We're not clocking out pixels. We haven't fully powered on the
>>>>>>> display. The AUX transfer is designed to be something that can be
>>>>>>> done early _before_ you turn on the display.
>>>>>>>
>>>>>>>
>>>>>>> OK, so basically that was a longwinded way of saying: yes, we
>>>>>>> could avoid the AUX transfer in probe, but we can't wait all the
>>>>>>> way to enable. We have to be able to transfer in get_modes(). If
>>>>>>> you think that's helpful I think it'd be a pretty easy patch to
>>>>>>> write even if it would look a tad bit awkward IMO. Let me know if
>>>>>>> you want me to post it up.
>>>>>>
>>>>>> I think it would be a good idea. At least it will allow us to
>>>>>> judge, which is the more correct way.
>>>>>
>>>>> I'm still happy to prototype this, but the more I think about it the
>>>>> more it feels like a workaround for the Qualcomm driver. The eDP
>>>>> panel driver is actually given a pointer to the AUX bus at probe
>>>>> time. It's really weird to say that we can't do a transfer on it
>>>>> yet... As you said, this is a little sideband bus. It should be able
>>>>> to be used without all the full blown infra of the rest of the driver.
>>>>
>>>> Yes, I have that feeling too. However I also have a feeling that just
>>>> powering up the PHY before the bus probe is ... a hack. There are no
>>>> obvious stopgaps for the driver not to power it down later.
>>>

Lets go back to why we need to power up the PHY before the bus probe.

We need to power up PHY before bus probe because panel-eDP tries to read 
the EDID in probe() for the panel_id. Not get_modes().

So doug, I didnt follow your comment that panel-eDP only does EDID read 
in get_modes()

	panel_id = drm_edid_get_panel_id(panel->ddc);
	if (!panel_id) {
		dev_err(dev, "Couldn't identify panel via EDID\n");
		ret = -EIO;
		goto exit;
	}

If we do not need this part, we really dont need to power up the PHY 
before the probe(). The hack which dmitry was referring to.

So this is boiling down to why or how panel-eDP was originally designed.

>>> This is why I think we need to move to Runtime PM to manage this. Basically:
>>>
>>> 1. When an AUX transfer happens, you grab a PM runtime reference that
>>> _that_ powers up the PHY.

This will not be trivial and needs to be scoped out as sankeerth said 
but if the above is the only concern, why do we need to do this? There 
seems to be an explanation why we are doing this and its not a hack.

How would Dmitry's rework address this? We need some RFC to conclude on 
that first.

>>>
>>> 2. At the end of the AUX transfer function, you do a "put_autosuspend".
>>>
>>> Then it becomes not a hack, right?
>>>
>>>
>>
>> pm runtime ops needs to be implemented for both eDP and DP. This change
>> take good amount of planning and code changes as it affects DP also.
>>
>> Because this patch series consist of basic eDP changes for SC7280 bootup,
>> shall we take this pm_runtime implementation in subsequent patch series?
> 
> Dmitry is the real decision maker here, but in my opinion it would be
> OK to get something landed first that worked OK and wasn't taking us
> too far in the wrong direction and then we could get a follow up patch
> to move to pm_runtime.

I would say the discussion changed into a direction of implementing 
pm-runtime because the current patch series does what it takes to adhere 
to panel-eDP's design along with aux bus requirements of PHY needing to 
be on.

So doug, to answer your questions here:

"So I guess the net result is maybe we should just keep it where it is.
Long term I'd be interested in knowing if there's a reason why we
can't structure the driver so that AUX transfers can happen with less
intertwining with the rest of the code, but that can happen later. I
would expect that you'd basically just need clocks and regulators on
and maybe your PHY on."

Yes PHY needs to be absolutely on and configured before aux transfers.

If we want to change that up to stop reading the panel_id in the panel 
probe() and do it later, perhaps some of the changes done here are not 
needed.

It only seems reasonable that we first prototype that in a separate 
patch even a RFC perhaps and take this further as these set of changes 
are needed for basic display functionality on sc7280 chromebooks.

Let us know what are the concerns with doing it in a follow up change.

Thanks

Abhinav

Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus

[Doug Anderson]

Hi,

On Thu, Apr 7, 2022 at 1:11 PM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
> Hi Doug and Dmitry
>
> Sorry, but I caught up on this email just now.
>
> Some comments below.
>
> Thanks
>
> Abhinav
> On 4/7/2022 10:07 AM, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, Apr 7, 2022 at 7:19 AM Sankeerth Billakanti (QUIC)
> > <quic_sbillaka@quicinc.com> wrote:
> >>
> >> Hi Dmitry and Doug,
> >>
> >>> Hi,
> >>>
> >>> On Tue, Apr 5, 2022 at 10:36 AM Dmitry Baryshkov
> >>> <dmitry.baryshkov@linaro.org> wrote:
> >>>>
> >>>> On 05/04/2022 20:02, Doug Anderson wrote:
> >>>>> Hi,
> >>>>>
> >>>>> On Tue, Apr 5, 2022 at 5:54 AM Dmitry Baryshkov
> >>>>> <dmitry.baryshkov@linaro.org> wrote:
> >>>>>>> 3. For DP and eDP HPD means something a little different.
> >>>>>>> Essentially there are two concepts: a) is a display physically
> >>>>>>> connected and b) is the display powered up and ready. For DP, the
> >>>>>>> two are really tied together. From the kernel's point of view you
> >>>>>>> never "power down" a DP display and you can't detect that it's
> >>>>>>> physically connected until it's ready. Said another way, on you
> >>>>>>> tie "is a display there" to the HPD line and the moment a display
> >>>>>>> is there it's ready for you to do AUX transfers. For eDP, in the
> >>>>>>> lowest power state of a display it _won't_ assert its "HPD"
> >>>>>>> signal. However, it's still physically present. For eDP you simply
> >>>>>>> have to _assume_ it's present without any actual proof since you
> >>>>>>> can't get proof until you power it up. Thus for eDP, you report
> >>>>>>> that the display is there as soon as we're asked. We can't _talk_
> >>>>>>> to the display yet, though. So in get_modes() we need to be able
> >>>>>>> to power the display on enough to talk over the AUX channel to it.
> >>>>>>> As part of this, we wait for the signal named "HPD" which really means
> >>> "panel finished powering on" in this context.
> >>>>>>>
> >>>>>>> NOTE: for aux transfer, we don't have the _display_ pipe and
> >>>>>>> clocks running. We only have enough stuff running to do the AUX
> >>> transfer.
> >>>>>>> We're not clocking out pixels. We haven't fully powered on the
> >>>>>>> display. The AUX transfer is designed to be something that can be
> >>>>>>> done early _before_ you turn on the display.
> >>>>>>>
> >>>>>>>
> >>>>>>> OK, so basically that was a longwinded way of saying: yes, we
> >>>>>>> could avoid the AUX transfer in probe, but we can't wait all the
> >>>>>>> way to enable. We have to be able to transfer in get_modes(). If
> >>>>>>> you think that's helpful I think it'd be a pretty easy patch to
> >>>>>>> write even if it would look a tad bit awkward IMO. Let me know if
> >>>>>>> you want me to post it up.
> >>>>>>
> >>>>>> I think it would be a good idea. At least it will allow us to
> >>>>>> judge, which is the more correct way.
> >>>>>
> >>>>> I'm still happy to prototype this, but the more I think about it the
> >>>>> more it feels like a workaround for the Qualcomm driver. The eDP
> >>>>> panel driver is actually given a pointer to the AUX bus at probe
> >>>>> time. It's really weird to say that we can't do a transfer on it
> >>>>> yet... As you said, this is a little sideband bus. It should be able
> >>>>> to be used without all the full blown infra of the rest of the driver.
> >>>>
> >>>> Yes, I have that feeling too. However I also have a feeling that just
> >>>> powering up the PHY before the bus probe is ... a hack. There are no
> >>>> obvious stopgaps for the driver not to power it down later.
> >>>
>
> Lets go back to why we need to power up the PHY before the bus probe.
>
> We need to power up PHY before bus probe because panel-eDP tries to read
> the EDID in probe() for the panel_id. Not get_modes().
>
> So doug, I didnt follow your comment that panel-eDP only does EDID read
> in get_modes()
>
>         panel_id = drm_edid_get_panel_id(panel->ddc);
>         if (!panel_id) {
>                 dev_err(dev, "Couldn't identify panel via EDID\n");
>                 ret = -EIO;
>                 goto exit;
>         }
>
> If we do not need this part, we really dont need to power up the PHY
> before the probe(). The hack which dmitry was referring to.

Right. ...so we _could_ remove the above from the panel-edp probe and
defer it to get_modes() and it wouldn't be that hard. ...but:

1. It feels like a hack to work around the Qualcomm driver. The way
the AUX bus is designed is that a pointer to the AUX bus is passed to
the panel-edp probe. It seems kinda strange to say that the panel
isn't allowed to do transfers with the pointer that's passed in.

2. There's a second place where we might do an AUX transfer at probe
time which is when we're using the DP AUX backlight. There we call
drm_panel_dp_aux_backlight(). Conceivably this too could be deferred
until the get_modes(), but now it feels even more like a hack. We're
going to be registering the backlight in the first call to
get_modes()? That's, ummm, unexpected. We could look at perhaps
breaking the "DP AUX backlight" in two parts also, but that gets
involved. I think we're supposed to know the number of backlight
levels at device init time for backlight devices and we need an AUX
transfer to that.


So the answer is that we could probably make it work, but it seems
like an uglier solution than just making the Qualcomm driver able to
do AUX transfers when it should be able to.

> So this is boiling down to why or how panel-eDP was originally designed.
>
> >>> This is why I think we need to move to Runtime PM to manage this. Basically:
> >>>
> >>> 1. When an AUX transfer happens, you grab a PM runtime reference that
> >>> _that_ powers up the PHY.
>
> This will not be trivial and needs to be scoped out as sankeerth said
> but if the above is the only concern, why do we need to do this? There
> seems to be an explanation why we are doing this and its not a hack.
>
> How would Dmitry's rework address this? We need some RFC to conclude on
> that first.
>
> >>>
> >>> 2. At the end of the AUX transfer function, you do a "put_autosuspend".
> >>>
> >>> Then it becomes not a hack, right?
> >>>
> >>>
> >>
> >> pm runtime ops needs to be implemented for both eDP and DP. This change
> >> take good amount of planning and code changes as it affects DP also.
> >>
> >> Because this patch series consist of basic eDP changes for SC7280 bootup,
> >> shall we take this pm_runtime implementation in subsequent patch series?
> >
> > Dmitry is the real decision maker here, but in my opinion it would be
> > OK to get something landed first that worked OK and wasn't taking us
> > too far in the wrong direction and then we could get a follow up patch
> > to move to pm_runtime.
>
> I would say the discussion changed into a direction of implementing
> pm-runtime because the current patch series does what it takes to adhere
> to panel-eDP's design along with aux bus requirements of PHY needing to
> be on.
>
> So doug, to answer your questions here:
>
> "So I guess the net result is maybe we should just keep it where it is.
> Long term I'd be interested in knowing if there's a reason why we
> can't structure the driver so that AUX transfers can happen with less
> intertwining with the rest of the code, but that can happen later. I
> would expect that you'd basically just need clocks and regulators on
> and maybe your PHY on."
>
> Yes PHY needs to be absolutely on and configured before aux transfers.
>
> If we want to change that up to stop reading the panel_id in the panel
> probe() and do it later, perhaps some of the changes done here are not
> needed.
>
> It only seems reasonable that we first prototype that in a separate
> patch even a RFC perhaps and take this further as these set of changes
> are needed for basic display functionality on sc7280 chromebooks.
>
> Let us know what are the concerns with doing it in a follow up change.

As per above, I'm not objecting to it being a follow-up change, but I
do believe it's the right design and will lead to an overall cleaner
solution. I think I even mentioned in my reviews that the current
patch series seems to "scattershot" enable resources and that's how we
end up with patches like patch #5 in this series ("drm/msm/dp: prevent
multiple votes for dp resources"). IMO there should be be a 1-to-1
mapping between "turn on resources" and "turn off resources" and it
should be reference counted. So if your codepath turned on resources
then it's up to your codepath to turn resources off when done. If a
seconde code path might be running at the same time then it should
also turn on/off resources itself. ...and it should all be managed by
pm_runtime which is _exactly designed_ for this specific use case.

-Doug

Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus

[Abhinav Kumar]

Hi Doug

Thanks for the response, some comments below.

Abhinav
On 4/7/2022 1:47 PM, Doug Anderson wrote:
> Hi,
> 
> On Thu, Apr 7, 2022 at 1:11 PM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>> Hi Doug and Dmitry
>>
>> Sorry, but I caught up on this email just now.
>>
>> Some comments below.
>>
>> Thanks
>>
>> Abhinav
>> On 4/7/2022 10:07 AM, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Thu, Apr 7, 2022 at 7:19 AM Sankeerth Billakanti (QUIC)
>>> <quic_sbillaka@quicinc.com> wrote:
>>>>
>>>> Hi Dmitry and Doug,
>>>>
>>>>> Hi,
>>>>>
>>>>> On Tue, Apr 5, 2022 at 10:36 AM Dmitry Baryshkov
>>>>> <dmitry.baryshkov@linaro.org> wrote:
>>>>>>
>>>>>> On 05/04/2022 20:02, Doug Anderson wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On Tue, Apr 5, 2022 at 5:54 AM Dmitry Baryshkov
>>>>>>> <dmitry.baryshkov@linaro.org> wrote:
>>>>>>>>> 3. For DP and eDP HPD means something a little different.
>>>>>>>>> Essentially there are two concepts: a) is a display physically
>>>>>>>>> connected and b) is the display powered up and ready. For DP, the
>>>>>>>>> two are really tied together. From the kernel's point of view you
>>>>>>>>> never "power down" a DP display and you can't detect that it's
>>>>>>>>> physically connected until it's ready. Said another way, on you
>>>>>>>>> tie "is a display there" to the HPD line and the moment a display
>>>>>>>>> is there it's ready for you to do AUX transfers. For eDP, in the
>>>>>>>>> lowest power state of a display it _won't_ assert its "HPD"
>>>>>>>>> signal. However, it's still physically present. For eDP you simply
>>>>>>>>> have to _assume_ it's present without any actual proof since you
>>>>>>>>> can't get proof until you power it up. Thus for eDP, you report
>>>>>>>>> that the display is there as soon as we're asked. We can't _talk_
>>>>>>>>> to the display yet, though. So in get_modes() we need to be able
>>>>>>>>> to power the display on enough to talk over the AUX channel to it.
>>>>>>>>> As part of this, we wait for the signal named "HPD" which really means
>>>>> "panel finished powering on" in this context.
>>>>>>>>>
>>>>>>>>> NOTE: for aux transfer, we don't have the _display_ pipe and
>>>>>>>>> clocks running. We only have enough stuff running to do the AUX
>>>>> transfer.
>>>>>>>>> We're not clocking out pixels. We haven't fully powered on the
>>>>>>>>> display. The AUX transfer is designed to be something that can be
>>>>>>>>> done early _before_ you turn on the display.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> OK, so basically that was a longwinded way of saying: yes, we
>>>>>>>>> could avoid the AUX transfer in probe, but we can't wait all the
>>>>>>>>> way to enable. We have to be able to transfer in get_modes(). If
>>>>>>>>> you think that's helpful I think it'd be a pretty easy patch to
>>>>>>>>> write even if it would look a tad bit awkward IMO. Let me know if
>>>>>>>>> you want me to post it up.
>>>>>>>>
>>>>>>>> I think it would be a good idea. At least it will allow us to
>>>>>>>> judge, which is the more correct way.
>>>>>>>
>>>>>>> I'm still happy to prototype this, but the more I think about it the
>>>>>>> more it feels like a workaround for the Qualcomm driver. The eDP
>>>>>>> panel driver is actually given a pointer to the AUX bus at probe
>>>>>>> time. It's really weird to say that we can't do a transfer on it
>>>>>>> yet... As you said, this is a little sideband bus. It should be able
>>>>>>> to be used without all the full blown infra of the rest of the driver.
>>>>>>
>>>>>> Yes, I have that feeling too. However I also have a feeling that just
>>>>>> powering up the PHY before the bus probe is ... a hack. There are no
>>>>>> obvious stopgaps for the driver not to power it down later.
>>>>>
>>
>> Lets go back to why we need to power up the PHY before the bus probe.
>>
>> We need to power up PHY before bus probe because panel-eDP tries to read
>> the EDID in probe() for the panel_id. Not get_modes().
>>
>> So doug, I didnt follow your comment that panel-eDP only does EDID read
>> in get_modes()
>>
>>          panel_id = drm_edid_get_panel_id(panel->ddc);
>>          if (!panel_id) {
>>                  dev_err(dev, "Couldn't identify panel via EDID\n");
>>                  ret = -EIO;
>>                  goto exit;
>>          }
>>
>> If we do not need this part, we really dont need to power up the PHY
>> before the probe(). The hack which dmitry was referring to.
> 
> Right. ...so we _could_ remove the above from the panel-edp probe and
> defer it to get_modes() and it wouldn't be that hard. ...but:
> 
> 1. It feels like a hack to work around the Qualcomm driver. The way
> the AUX bus is designed is that a pointer to the AUX bus is passed to
> the panel-edp probe. It seems kinda strange to say that the panel
> isn't allowed to do transfers with the pointer that's passed in.
> 

And thats why to satisfy the requirements of passing an initialized AUX, 
sankeerth is delaying devm_of_dp_aux_populate_ep_devices() till PHY is 
initialized which seems reasonable to satisfy the probe() time requirements.

Even if we move to pm_runtime(), yes I agree it will club all the 
resources needed to control AUX in one place but you will still have to 
initialize PHY before probe() under the hood of pm_runtime().

So how will it help this cause?

We just have to accept that initializing PHY is a requirement to use AUX 
and before calling panel-eDP's probe(), we have to have an initialized AUX.

So we are not working around the driver but just satisfying the hardware 
requirements to be able to satisfy panel-edp's and 
drm_panel_dp_aux_backlight()'s aux bus requirements.

> 2. There's a second place where we might do an AUX transfer at probe
> time which is when we're using the DP AUX backlight. There we call
> drm_panel_dp_aux_backlight(). Conceivably this too could be deferred
> until the get_modes(), but now it feels even more like a hack. We're
> going to be registering the backlight in the first call to
> get_modes()? That's, ummm, unexpected. We could look at perhaps
> breaking the "DP AUX backlight" in two parts also, but that gets
> involved. I think we're supposed to know the number of backlight
> levels at device init time for backlight devices and we need an AUX
> transfer to that.
> 


> 
> So the answer is that we could probably make it work, but it seems
> like an uglier solution than just making the Qualcomm driver able to
> do AUX transfers when it should be able to.

Correct and by delaying the panel-edp's probe(), we are doing exactly that?

> 
>> So this is boiling down to why or how panel-eDP was originally designed.
>>
>>>>> This is why I think we need to move to Runtime PM to manage this. Basically:
>>>>>
>>>>> 1. When an AUX transfer happens, you grab a PM runtime reference that
>>>>> _that_ powers up the PHY.
>>
>> This will not be trivial and needs to be scoped out as sankeerth said
>> but if the above is the only concern, why do we need to do this? There
>> seems to be an explanation why we are doing this and its not a hack.
>>
>> How would Dmitry's rework address this? We need some RFC to conclude on
>> that first.
>>
>>>>>
>>>>> 2. At the end of the AUX transfer function, you do a "put_autosuspend".
>>>>>
>>>>> Then it becomes not a hack, right?
>>>>>
>>>>>
>>>>
>>>> pm runtime ops needs to be implemented for both eDP and DP. This change
>>>> take good amount of planning and code changes as it affects DP also.
>>>>
>>>> Because this patch series consist of basic eDP changes for SC7280 bootup,
>>>> shall we take this pm_runtime implementation in subsequent patch series?
>>>
>>> Dmitry is the real decision maker here, but in my opinion it would be
>>> OK to get something landed first that worked OK and wasn't taking us
>>> too far in the wrong direction and then we could get a follow up patch
>>> to move to pm_runtime.
>>
>> I would say the discussion changed into a direction of implementing
>> pm-runtime because the current patch series does what it takes to adhere
>> to panel-eDP's design along with aux bus requirements of PHY needing to
>> be on.
>>
>> So doug, to answer your questions here:
>>
>> "So I guess the net result is maybe we should just keep it where it is.
>> Long term I'd be interested in knowing if there's a reason why we
>> can't structure the driver so that AUX transfers can happen with less
>> intertwining with the rest of the code, but that can happen later. I
>> would expect that you'd basically just need clocks and regulators on
>> and maybe your PHY on."
>>
>> Yes PHY needs to be absolutely on and configured before aux transfers.
>>
>> If we want to change that up to stop reading the panel_id in the panel
>> probe() and do it later, perhaps some of the changes done here are not
>> needed.
>>
>> It only seems reasonable that we first prototype that in a separate
>> patch even a RFC perhaps and take this further as these set of changes
>> are needed for basic display functionality on sc7280 chromebooks.
>>
>> Let us know what are the concerns with doing it in a follow up change.
> 
> As per above, I'm not objecting to it being a follow-up change, but I
> do believe it's the right design and will lead to an overall cleaner
> solution. I think I even mentioned in my reviews that the current
> patch series seems to "scattershot" enable resources and that's how we
> end up with patches like patch #5 in this series ("drm/msm/dp: prevent
> multiple votes for dp resources"). IMO there should be be a 1-to-1
> mapping between "turn on resources" and "turn off resources" and it
> should be reference counted. So if your codepath turned on resources
> then it's up to your codepath to turn resources off when done. If a
> seconde code path might be running at the same time then it should
> also turn on/off resources itself. ...and it should all be managed by
> pm_runtime which is _exactly designed_ for this specific use case.
> 
Agreed on this topic, moving to pm_runtime() will club all resources in 
one place and make things cleaner that way.

Complexity of it obviously needs to be evaluated to check the effort Vs 
rewards.

But it will still not address the original concern of this thread that
powering up the PHY before the probe() is a hack.

"Yes, I have that feeling too. However I also have a feeling that just
 > powering up the PHY before the bus probe is ... a hack. There are no
 > obvious stopgaps for the driver not to power it down later."

We would still end up doing that under the hood of pm_runtime.

And thats why its an improvement and not a necessity qualifying it for a 
separate change.

> -Doug

Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus

[Doug Anderson]

Hi,

On Thu, Apr 7, 2022 at 3:03 PM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
> Hi Doug
>
> Thanks for the response, some comments below.
>
> Abhinav
> On 4/7/2022 1:47 PM, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, Apr 7, 2022 at 1:11 PM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>
> >> Hi Doug and Dmitry
> >>
> >> Sorry, but I caught up on this email just now.
> >>
> >> Some comments below.
> >>
> >> Thanks
> >>
> >> Abhinav
> >> On 4/7/2022 10:07 AM, Doug Anderson wrote:
> >>> Hi,
> >>>
> >>> On Thu, Apr 7, 2022 at 7:19 AM Sankeerth Billakanti (QUIC)
> >>> <quic_sbillaka@quicinc.com> wrote:
> >>>>
> >>>> Hi Dmitry and Doug,
> >>>>
> >>>>> Hi,
> >>>>>
> >>>>> On Tue, Apr 5, 2022 at 10:36 AM Dmitry Baryshkov
> >>>>> <dmitry.baryshkov@linaro.org> wrote:
> >>>>>>
> >>>>>> On 05/04/2022 20:02, Doug Anderson wrote:
> >>>>>>> Hi,
> >>>>>>>
> >>>>>>> On Tue, Apr 5, 2022 at 5:54 AM Dmitry Baryshkov
> >>>>>>> <dmitry.baryshkov@linaro.org> wrote:
> >>>>>>>>> 3. For DP and eDP HPD means something a little different.
> >>>>>>>>> Essentially there are two concepts: a) is a display physically
> >>>>>>>>> connected and b) is the display powered up and ready. For DP, the
> >>>>>>>>> two are really tied together. From the kernel's point of view you
> >>>>>>>>> never "power down" a DP display and you can't detect that it's
> >>>>>>>>> physically connected until it's ready. Said another way, on you
> >>>>>>>>> tie "is a display there" to the HPD line and the moment a display
> >>>>>>>>> is there it's ready for you to do AUX transfers. For eDP, in the
> >>>>>>>>> lowest power state of a display it _won't_ assert its "HPD"
> >>>>>>>>> signal. However, it's still physically present. For eDP you simply
> >>>>>>>>> have to _assume_ it's present without any actual proof since you
> >>>>>>>>> can't get proof until you power it up. Thus for eDP, you report
> >>>>>>>>> that the display is there as soon as we're asked. We can't _talk_
> >>>>>>>>> to the display yet, though. So in get_modes() we need to be able
> >>>>>>>>> to power the display on enough to talk over the AUX channel to it.
> >>>>>>>>> As part of this, we wait for the signal named "HPD" which really means
> >>>>> "panel finished powering on" in this context.
> >>>>>>>>>
> >>>>>>>>> NOTE: for aux transfer, we don't have the _display_ pipe and
> >>>>>>>>> clocks running. We only have enough stuff running to do the AUX
> >>>>> transfer.
> >>>>>>>>> We're not clocking out pixels. We haven't fully powered on the
> >>>>>>>>> display. The AUX transfer is designed to be something that can be
> >>>>>>>>> done early _before_ you turn on the display.
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> OK, so basically that was a longwinded way of saying: yes, we
> >>>>>>>>> could avoid the AUX transfer in probe, but we can't wait all the
> >>>>>>>>> way to enable. We have to be able to transfer in get_modes(). If
> >>>>>>>>> you think that's helpful I think it'd be a pretty easy patch to
> >>>>>>>>> write even if it would look a tad bit awkward IMO. Let me know if
> >>>>>>>>> you want me to post it up.
> >>>>>>>>
> >>>>>>>> I think it would be a good idea. At least it will allow us to
> >>>>>>>> judge, which is the more correct way.
> >>>>>>>
> >>>>>>> I'm still happy to prototype this, but the more I think about it the
> >>>>>>> more it feels like a workaround for the Qualcomm driver. The eDP
> >>>>>>> panel driver is actually given a pointer to the AUX bus at probe
> >>>>>>> time. It's really weird to say that we can't do a transfer on it
> >>>>>>> yet... As you said, this is a little sideband bus. It should be able
> >>>>>>> to be used without all the full blown infra of the rest of the driver.
> >>>>>>
> >>>>>> Yes, I have that feeling too. However I also have a feeling that just
> >>>>>> powering up the PHY before the bus probe is ... a hack. There are no
> >>>>>> obvious stopgaps for the driver not to power it down later.
> >>>>>
> >>
> >> Lets go back to why we need to power up the PHY before the bus probe.
> >>
> >> We need to power up PHY before bus probe because panel-eDP tries to read
> >> the EDID in probe() for the panel_id. Not get_modes().
> >>
> >> So doug, I didnt follow your comment that panel-eDP only does EDID read
> >> in get_modes()
> >>
> >>          panel_id = drm_edid_get_panel_id(panel->ddc);
> >>          if (!panel_id) {
> >>                  dev_err(dev, "Couldn't identify panel via EDID\n");
> >>                  ret = -EIO;
> >>                  goto exit;
> >>          }
> >>
> >> If we do not need this part, we really dont need to power up the PHY
> >> before the probe(). The hack which dmitry was referring to.
> >
> > Right. ...so we _could_ remove the above from the panel-edp probe and
> > defer it to get_modes() and it wouldn't be that hard. ...but:
> >
> > 1. It feels like a hack to work around the Qualcomm driver. The way
> > the AUX bus is designed is that a pointer to the AUX bus is passed to
> > the panel-edp probe. It seems kinda strange to say that the panel
> > isn't allowed to do transfers with the pointer that's passed in.
> >
>
> And thats why to satisfy the requirements of passing an initialized AUX,
> sankeerth is delaying devm_of_dp_aux_populate_ep_devices() till PHY is
> initialized which seems reasonable to satisfy the probe() time requirements.
>
> Even if we move to pm_runtime(), yes I agree it will club all the
> resources needed to control AUX in one place but you will still have to
> initialize PHY before probe() under the hood of pm_runtime().
>
> So how will it help this cause?
>
> We just have to accept that initializing PHY is a requirement to use AUX
> and before calling panel-eDP's probe(), we have to have an initialized AUX.
>
> So we are not working around the driver but just satisfying the hardware
> requirements to be able to satisfy panel-edp's and
> drm_panel_dp_aux_backlight()'s aux bus requirements.

The way I'm arguing it should work is that:

1. A whole bunch of the DP init code should move to the DP driver's
probe function. This includes parsing the DT, acquiring clocks,
getting a handle to our PHY, and IO mapping registers. As far as I
know, there's no reason to wait on all the components being probed in
order to do this stuff.

2. Once we have done the above things, it should be possible to do AUX
transfers, correct? ...and then we can populate the AUX bus from the
probe function too.

3. Any other init (setting up pixel clocks) can continue to happen
where it is today.


> > 2. There's a second place where we might do an AUX transfer at probe
> > time which is when we're using the DP AUX backlight. There we call
> > drm_panel_dp_aux_backlight(). Conceivably this too could be deferred
> > until the get_modes(), but now it feels even more like a hack. We're
> > going to be registering the backlight in the first call to
> > get_modes()? That's, ummm, unexpected. We could look at perhaps
> > breaking the "DP AUX backlight" in two parts also, but that gets
> > involved. I think we're supposed to know the number of backlight
> > levels at device init time for backlight devices and we need an AUX
> > transfer to that.
> >
>
>
> >
> > So the answer is that we could probably make it work, but it seems
> > like an uglier solution than just making the Qualcomm driver able to
> > do AUX transfers when it should be able to.
>
> Correct and by delaying the panel-edp's probe(), we are doing exactly that?

Right. Where you put the probe now makes it work OK from an AUX
transfer point of view and it's probably OK for the short term, but
I'm not 100% convinced it would handle the -EPROBE_DEFER case, though
I haven't actually tested it.

Imagine this case:

1. 100% of your code is built-in to the kernel except for your PWM
driver, which is a module.

2. You start booting up. All the DRM components for MSM are finished
and eventually modeset_init() gets called.

3. We try to probe the panel. When the panel tries to acquire the PWM
backlight, it finds that the PWM driver hasn't been loaded yet. It
gets back -EPROBE_DEFER which prevents the panel driver from probing.

The question is: does modeset_init() handle that -EPROBE_DEFER
elegantly? Normally that's something that would only be returned by
probe functions. Maybe this is all handled, though? I definitely
haven't followed enough of the code and haven't tested it myself.

The above is probably not a giant deal, but I think long term it would
be better to be acquiring resources earlier.

Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus

[Dmitry Baryshkov]

On Fri, 8 Apr 2022 at 02:35, Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Thu, Apr 7, 2022 at 3:03 PM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >
> > Hi Doug
> >
> > Thanks for the response, some comments below.
> >
> > Abhinav
> > On 4/7/2022 1:47 PM, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Thu, Apr 7, 2022 at 1:11 PM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> > >>
> > >> Hi Doug and Dmitry
> > >>
> > >> Sorry, but I caught up on this email just now.
> > >>
> > >> Some comments below.
> > >>
> > >> Thanks
> > >>
> > >> Abhinav
> > >> On 4/7/2022 10:07 AM, Doug Anderson wrote:
> > >>> Hi,
> > >>>
> > >>> On Thu, Apr 7, 2022 at 7:19 AM Sankeerth Billakanti (QUIC)
> > >>> <quic_sbillaka@quicinc.com> wrote:
> > >>>>
> > >>>> Hi Dmitry and Doug,
> > >>>>
> > >>>>> Hi,
> > >>>>>
> > >>>>> On Tue, Apr 5, 2022 at 10:36 AM Dmitry Baryshkov
> > >>>>> <dmitry.baryshkov@linaro.org> wrote:
> > >>>>>>
> > >>>>>> On 05/04/2022 20:02, Doug Anderson wrote:
> > >>>>>>> Hi,
> > >>>>>>>
> > >>>>>>> On Tue, Apr 5, 2022 at 5:54 AM Dmitry Baryshkov
> > >>>>>>> <dmitry.baryshkov@linaro.org> wrote:
> > >>>>>>>>> 3. For DP and eDP HPD means something a little different.
> > >>>>>>>>> Essentially there are two concepts: a) is a display physically
> > >>>>>>>>> connected and b) is the display powered up and ready. For DP, the
> > >>>>>>>>> two are really tied together. From the kernel's point of view you
> > >>>>>>>>> never "power down" a DP display and you can't detect that it's
> > >>>>>>>>> physically connected until it's ready. Said another way, on you
> > >>>>>>>>> tie "is a display there" to the HPD line and the moment a display
> > >>>>>>>>> is there it's ready for you to do AUX transfers. For eDP, in the
> > >>>>>>>>> lowest power state of a display it _won't_ assert its "HPD"
> > >>>>>>>>> signal. However, it's still physically present. For eDP you simply
> > >>>>>>>>> have to _assume_ it's present without any actual proof since you
> > >>>>>>>>> can't get proof until you power it up. Thus for eDP, you report
> > >>>>>>>>> that the display is there as soon as we're asked. We can't _talk_
> > >>>>>>>>> to the display yet, though. So in get_modes() we need to be able
> > >>>>>>>>> to power the display on enough to talk over the AUX channel to it.
> > >>>>>>>>> As part of this, we wait for the signal named "HPD" which really means
> > >>>>> "panel finished powering on" in this context.
> > >>>>>>>>>
> > >>>>>>>>> NOTE: for aux transfer, we don't have the _display_ pipe and
> > >>>>>>>>> clocks running. We only have enough stuff running to do the AUX
> > >>>>> transfer.
> > >>>>>>>>> We're not clocking out pixels. We haven't fully powered on the
> > >>>>>>>>> display. The AUX transfer is designed to be something that can be
> > >>>>>>>>> done early _before_ you turn on the display.
> > >>>>>>>>>
> > >>>>>>>>>
> > >>>>>>>>> OK, so basically that was a longwinded way of saying: yes, we
> > >>>>>>>>> could avoid the AUX transfer in probe, but we can't wait all the
> > >>>>>>>>> way to enable. We have to be able to transfer in get_modes(). If
> > >>>>>>>>> you think that's helpful I think it'd be a pretty easy patch to
> > >>>>>>>>> write even if it would look a tad bit awkward IMO. Let me know if
> > >>>>>>>>> you want me to post it up.
> > >>>>>>>>
> > >>>>>>>> I think it would be a good idea. At least it will allow us to
> > >>>>>>>> judge, which is the more correct way.
> > >>>>>>>
> > >>>>>>> I'm still happy to prototype this, but the more I think about it the
> > >>>>>>> more it feels like a workaround for the Qualcomm driver. The eDP
> > >>>>>>> panel driver is actually given a pointer to the AUX bus at probe
> > >>>>>>> time. It's really weird to say that we can't do a transfer on it
> > >>>>>>> yet... As you said, this is a little sideband bus. It should be able
> > >>>>>>> to be used without all the full blown infra of the rest of the driver.
> > >>>>>>
> > >>>>>> Yes, I have that feeling too. However I also have a feeling that just
> > >>>>>> powering up the PHY before the bus probe is ... a hack. There are no
> > >>>>>> obvious stopgaps for the driver not to power it down later.
> > >>>>>
> > >>
> > >> Lets go back to why we need to power up the PHY before the bus probe.
> > >>
> > >> We need to power up PHY before bus probe because panel-eDP tries to read
> > >> the EDID in probe() for the panel_id. Not get_modes().
> > >>
> > >> So doug, I didnt follow your comment that panel-eDP only does EDID read
> > >> in get_modes()
> > >>
> > >>          panel_id = drm_edid_get_panel_id(panel->ddc);
> > >>          if (!panel_id) {
> > >>                  dev_err(dev, "Couldn't identify panel via EDID\n");
> > >>                  ret = -EIO;
> > >>                  goto exit;
> > >>          }
> > >>
> > >> If we do not need this part, we really dont need to power up the PHY
> > >> before the probe(). The hack which dmitry was referring to.
> > >
> > > Right. ...so we _could_ remove the above from the panel-edp probe and
> > > defer it to get_modes() and it wouldn't be that hard. ...but:
> > >
> > > 1. It feels like a hack to work around the Qualcomm driver. The way
> > > the AUX bus is designed is that a pointer to the AUX bus is passed to
> > > the panel-edp probe. It seems kinda strange to say that the panel
> > > isn't allowed to do transfers with the pointer that's passed in.
> > >
> >
> > And thats why to satisfy the requirements of passing an initialized AUX,
> > sankeerth is delaying devm_of_dp_aux_populate_ep_devices() till PHY is
> > initialized which seems reasonable to satisfy the probe() time requirements.
> >
> > Even if we move to pm_runtime(), yes I agree it will club all the
> > resources needed to control AUX in one place but you will still have to
> > initialize PHY before probe() under the hood of pm_runtime().
> >
> > So how will it help this cause?
> >
> > We just have to accept that initializing PHY is a requirement to use AUX
> > and before calling panel-eDP's probe(), we have to have an initialized AUX.
> >
> > So we are not working around the driver but just satisfying the hardware
> > requirements to be able to satisfy panel-edp's and
> > drm_panel_dp_aux_backlight()'s aux bus requirements.
>
> The way I'm arguing it should work is that:
>
> 1. A whole bunch of the DP init code should move to the DP driver's
> probe function. This includes parsing the DT, acquiring clocks,
> getting a handle to our PHY, and IO mapping registers. As far as I
> know, there's no reason to wait on all the components being probed in
> order to do this stuff.

Yes. And that's one of the reasons I tried to stay away from the DP
driver. Each time I open the source code, my hands itch to start
refactoring the code.

>
> 2. Once we have done the above things, it should be possible to do AUX
> transfers, correct? ...and then we can populate the AUX bus from the
> probe function too.

No. In the DP case the AUX bus is inaccessible until the dongle is
plugged (see all the HPD handling, phy_init()/phy_power_on() is hidden
somewhere in that path)

eDP needs to be a special case in the probe() function.

>
> 3. Any other init (setting up pixel clocks) can continue to happen
> where it is today.

Yes.

>
>
> > > 2. There's a second place where we might do an AUX transfer at probe
> > > time which is when we're using the DP AUX backlight. There we call
> > > drm_panel_dp_aux_backlight(). Conceivably this too could be deferred
> > > until the get_modes(), but now it feels even more like a hack. We're
> > > going to be registering the backlight in the first call to
> > > get_modes()? That's, ummm, unexpected. We could look at perhaps
> > > breaking the "DP AUX backlight" in two parts also, but that gets
> > > involved. I think we're supposed to know the number of backlight
> > > levels at device init time for backlight devices and we need an AUX
> > > transfer to that.
> > >
> >
> >
> > >
> > > So the answer is that we could probably make it work, but it seems
> > > like an uglier solution than just making the Qualcomm driver able to
> > > do AUX transfers when it should be able to.
> >
> > Correct and by delaying the panel-edp's probe(), we are doing exactly that?
>
> Right. Where you put the probe now makes it work OK from an AUX
> transfer point of view and it's probably OK for the short term, but
> I'm not 100% convinced it would handle the -EPROBE_DEFER case, though
> I haven't actually tested it.
>
> Imagine this case:
>
> 1. 100% of your code is built-in to the kernel except for your PWM
> driver, which is a module.
>
> 2. You start booting up. All the DRM components for MSM are finished
> and eventually modeset_init() gets called.
>
> 3. We try to probe the panel. When the panel tries to acquire the PWM
> backlight, it finds that the PWM driver hasn't been loaded yet. It
> gets back -EPROBE_DEFER which prevents the panel driver from probing.
>
> The question is: does modeset_init() handle that -EPROBE_DEFER
> elegantly? Normally that's something that would only be returned by
> probe functions. Maybe this is all handled, though? I definitely
> haven't followed enough of the code and haven't tested it myself.

It would be handled up to some point. The error would propagate to the
msm_drm_init() = msm_drm_bind(), failing the mdss probe() (and putting
it to the defer list).
However in the dp's error path the driver would destroy the EP device.
The kernel would notice this and retry devices from the defer list. We
have just sorted this out for the DSI (thank you Maxime, Rob and
Angelo for doing this).

>
> The above is probably not a giant deal, but I think long term it would
> be better to be acquiring resources earlier.


-- 
With best wishes
Dmitry

Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus

[Doug Anderson]

Hi,

On Thu, Apr 7, 2022 at 4:46 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> > The way I'm arguing it should work is that:
> >
> > 1. A whole bunch of the DP init code should move to the DP driver's
> > probe function. This includes parsing the DT, acquiring clocks,
> > getting a handle to our PHY, and IO mapping registers. As far as I
> > know, there's no reason to wait on all the components being probed in
> > order to do this stuff.
>
> Yes. And that's one of the reasons I tried to stay away from the DP
> driver. Each time I open the source code, my hands itch to start
> refactoring the code.
>
> >
> > 2. Once we have done the above things, it should be possible to do AUX
> > transfers, correct? ...and then we can populate the AUX bus from the
> > probe function too.
>
> No. In the DP case the AUX bus is inaccessible until the dongle is
> plugged (see all the HPD handling, phy_init()/phy_power_on() is hidden
> somewhere in that path)

I guess my thought was that in DP you could still create the AUX bus
at probe time. Then for DP you just return an instant "transfer
failed" from the AUX bus if HPD isn't asserted. For eDP (as discussed
elsewhere) when we try to do an AUX transfer then we delay until HPD
is there.

So we can still acquire resources (clocks, PHY, io maps, etc) at probe
time for DP and create the AUX bus, right? It will just return
"-ENODEV" if HPD isn't asserted and you're DP?

-Doug

Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus

[Dmitry Baryshkov]

On Fri, 8 Apr 2022 at 03:26, Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Thu, Apr 7, 2022 at 4:46 PM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > > The way I'm arguing it should work is that:
> > >
> > > 1. A whole bunch of the DP init code should move to the DP driver's
> > > probe function. This includes parsing the DT, acquiring clocks,
> > > getting a handle to our PHY, and IO mapping registers. As far as I
> > > know, there's no reason to wait on all the components being probed in
> > > order to do this stuff.
> >
> > Yes. And that's one of the reasons I tried to stay away from the DP
> > driver. Each time I open the source code, my hands itch to start
> > refactoring the code.
> >
> > >
> > > 2. Once we have done the above things, it should be possible to do AUX
> > > transfers, correct? ...and then we can populate the AUX bus from the
> > > probe function too.
> >
> > No. In the DP case the AUX bus is inaccessible until the dongle is
> > plugged (see all the HPD handling, phy_init()/phy_power_on() is hidden
> > somewhere in that path)
>
> I guess my thought was that in DP you could still create the AUX bus
> at probe time. Then for DP you just return an instant "transfer
> failed" from the AUX bus if HPD isn't asserted. For eDP (as discussed
> elsewhere) when we try to do an AUX transfer then we delay until HPD
> is there.

I think panel-edp would already handle the delay, so we do not need to
have this logic in the DP driver.

> So we can still acquire resources (clocks, PHY, io maps, etc) at probe
> time for DP and create the AUX bus, right? It will just return
> "-ENODEV" if HPD isn't asserted and you're DP?

Yes, please. I still suppose that we'd need a separate case to
power_on eDP's PHY during the probe time. Maybe I'm mistaken here.

-- 
With best wishes
Dmitry

Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus

[Doug Anderson]

Hi,

On Fri, Apr 8, 2022 at 5:20 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> > I guess my thought was that in DP you could still create the AUX bus
> > at probe time. Then for DP you just return an instant "transfer
> > failed" from the AUX bus if HPD isn't asserted. For eDP (as discussed
> > elsewhere) when we try to do an AUX transfer then we delay until HPD
> > is there.
>
> I think panel-edp would already handle the delay, so we do not need to
> have this logic in the DP driver.

There's a whole discussion about this between Stephen and me in patch
#5 ("drm/msm/dp: wait for hpd high before any sink interaction").
Basically:

* If panel HPD is hooked up to the dedicated HPD pin on the eDP
controller then the panel driver doesn't have a way to read it.

* We can't leverage the existing "HPD" query functions in DRM because
those indicate whether a panel is _physically_ connected. For eDP, it
always is.

For now the rule is that the AUX transfer function is in charge of
waiting for HPD for eDP if the dedicated HPD pin is used. If we want
to re-invent this we could, but that system works, isn't _too_ ugly,
and we're already making big enough changes in this series.


> > So we can still acquire resources (clocks, PHY, io maps, etc) at probe
> > time for DP and create the AUX bus, right? It will just return
> > "-ENODEV" if HPD isn't asserted and you're DP?
>
> Yes, please. I still suppose that we'd need a separate case to
> power_on eDP's PHY during the probe time. Maybe I'm mistaken here.

I think the ideal way is to do it like Kieran's proposal for sn65dsi86:

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

* When enabling HPD (physical hot plug detect) in the hpd_enable()
callback you do a pm_runtime_get(). You do the
pm_runtime_put_autosuspend() when disabling. This is only used for DP
since we only provide DRM_BRIDGE_OP_HPD for DP, not for eDP.

* We do a pm_runtime_get() / pm_runtime_put_autosuspend() in the AUX
transfer routine. While holding the pm_runtime reference we check HPD.
For DP we return immediately if HPD isn't asserted. For eDP, we delay.

* We do the pm_runtime_get() in pre_enable and the pm_runtime_put() in
post_disable. For DP this will add a 2nd refcount (since we probably
were holding the reference for HPD). For eDP this will cause us to
power on.

* If there's any other time we need to read HW registers, and we
aren't guaranteed to already have a pm_runtime reference (like during
probe), we can do a temporary pm_runtime_get() /
pm_runtime_put_autosuspend().

Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus

[Dmitry Baryshkov]

On Fri, 8 Apr 2022 at 16:43, Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Fri, Apr 8, 2022 at 5:20 AM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > > I guess my thought was that in DP you could still create the AUX bus
> > > at probe time. Then for DP you just return an instant "transfer
> > > failed" from the AUX bus if HPD isn't asserted. For eDP (as discussed
> > > elsewhere) when we try to do an AUX transfer then we delay until HPD
> > > is there.
> >
> > I think panel-edp would already handle the delay, so we do not need to
> > have this logic in the DP driver.
>
> There's a whole discussion about this between Stephen and me in patch
> #5 ("drm/msm/dp: wait for hpd high before any sink interaction").
> Basically:
>
> * If panel HPD is hooked up to the dedicated HPD pin on the eDP
> controller then the panel driver doesn't have a way to read it.

I refreshed that dialog. I must admit, I have missed the fact that the
HPD pin might not be visible as the GPIO pin.

> * We can't leverage the existing "HPD" query functions in DRM because
> those indicate whether a panel is _physically_ connected. For eDP, it
> always is.

Yes, I was thinking about (mis)using the
drm_bridge_connector_hpd_notify() for generic HPD-related
notifications (to tell eDP that it should check the current state). I
have abandoned that idea.

> For now the rule is that the AUX transfer function is in charge of
> waiting for HPD for eDP if the dedicated HPD pin is used. If we want
> to re-invent this we could, but that system works, isn't _too_ ugly,
> and we're already making big enough changes in this series.

The is_hpd_asserted() looks like a good callback for the aux bus.
It will allow the panel driver to check if the panel is powered up (in
the absence of the GPIO pin).

> > > So we can still acquire resources (clocks, PHY, io maps, etc) at probe
> > > time for DP and create the AUX bus, right? It will just return
> > > "-ENODEV" if HPD isn't asserted and you're DP?
> >
> > Yes, please. I still suppose that we'd need a separate case to
> > power_on eDP's PHY during the probe time. Maybe I'm mistaken here.
>
> I think the ideal way is to do it like Kieran's proposal for sn65dsi86:
>
> https://lore.kernel.org/r/20220317131250.1481275-4-kieran.bingham+renesas@ideasonboard.com/
>
> * When enabling HPD (physical hot plug detect) in the hpd_enable()
> callback you do a pm_runtime_get(). You do the
> pm_runtime_put_autosuspend() when disabling. This is only used for DP
> since we only provide DRM_BRIDGE_OP_HPD for DP, not for eDP.
>
> * We do a pm_runtime_get() / pm_runtime_put_autosuspend() in the AUX
> transfer routine. While holding the pm_runtime reference we check HPD.
> For DP we return immediately if HPD isn't asserted. For eDP, we delay.
>
> * We do the pm_runtime_get() in pre_enable and the pm_runtime_put() in
> post_disable. For DP this will add a 2nd refcount (since we probably
> were holding the reference for HPD). For eDP this will cause us to
> power on.
>
> * If there's any other time we need to read HW registers, and we
> aren't guaranteed to already have a pm_runtime reference (like during
> probe), we can do a temporary pm_runtime_get() /
> pm_runtime_put_autosuspend().

This looks good. I'd be more than welcome to review such series.

Note: I think this would require using
drm_bridge_connector_enable_hpd() in the DP code.
Hopefully at some point we would be able to move all
drm_bridge_connector calls to the core msm layer.
--
With best wishes
Dmitry

Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus

[Abhinav Kumar]

Hi Doug and Dmitry

On 4/8/2022 7:58 AM, Dmitry Baryshkov wrote:
> On Fri, 8 Apr 2022 at 16:43, Doug Anderson <dianders@chromium.org> wrote:
>>
>> Hi,
>>
>> On Fri, Apr 8, 2022 at 5:20 AM Dmitry Baryshkov
>> <dmitry.baryshkov@linaro.org> wrote:
>>>
>>>> I guess my thought was that in DP you could still create the AUX bus
>>>> at probe time. Then for DP you just return an instant "transfer
>>>> failed" from the AUX bus if HPD isn't asserted. For eDP (as discussed
>>>> elsewhere) when we try to do an AUX transfer then we delay until HPD
>>>> is there.
>>>
>>> I think panel-edp would already handle the delay, so we do not need to
>>> have this logic in the DP driver.
>>
>> There's a whole discussion about this between Stephen and me in patch
>> #5 ("drm/msm/dp: wait for hpd high before any sink interaction").
>> Basically:
>>
>> * If panel HPD is hooked up to the dedicated HPD pin on the eDP
>> controller then the panel driver doesn't have a way to read it.
> 
> I refreshed that dialog. I must admit, I have missed the fact that the
> HPD pin might not be visible as the GPIO pin.
> 
>> * We can't leverage the existing "HPD" query functions in DRM because
>> those indicate whether a panel is _physically_ connected. For eDP, it
>> always is.
> 
> Yes, I was thinking about (mis)using the
> drm_bridge_connector_hpd_notify() for generic HPD-related
> notifications (to tell eDP that it should check the current state). I
> have abandoned that idea.
> 
>> For now the rule is that the AUX transfer function is in charge of
>> waiting for HPD for eDP if the dedicated HPD pin is used. If we want
>> to re-invent this we could, but that system works, isn't _too_ ugly,
>> and we're already making big enough changes in this series.
> 
> The is_hpd_asserted() looks like a good callback for the aux bus.
> It will allow the panel driver to check if the panel is powered up (in
> the absence of the GPIO pin).
> 
>>>> So we can still acquire resources (clocks, PHY, io maps, etc) at probe
>>>> time for DP and create the AUX bus, right? It will just return
>>>> "-ENODEV" if HPD isn't asserted and you're DP?
>>>
>>> Yes, please. I still suppose that we'd need a separate case to
>>> power_on eDP's PHY during the probe time. Maybe I'm mistaken here.
>>
>> I think the ideal way is to do it like Kieran's proposal for sn65dsi86:
>>
>> https://lore.kernel.org/r/20220317131250.1481275-4-kieran.bingham+renesas@ideasonboard.com/
>>
>> * When enabling HPD (physical hot plug detect) in the hpd_enable()
>> callback you do a pm_runtime_get(). You do the
>> pm_runtime_put_autosuspend() when disabling. This is only used for DP
>> since we only provide DRM_BRIDGE_OP_HPD for DP, not for eDP.
>>
>> * We do a pm_runtime_get() / pm_runtime_put_autosuspend() in the AUX
>> transfer routine. While holding the pm_runtime reference we check HPD.
>> For DP we return immediately if HPD isn't asserted. For eDP, we delay.
>>
>> * We do the pm_runtime_get() in pre_enable and the pm_runtime_put() in
>> post_disable. For DP this will add a 2nd refcount (since we probably
>> were holding the reference for HPD). For eDP this will cause us to
>> power on.
>>
>> * If there's any other time we need to read HW registers, and we
>> aren't guaranteed to already have a pm_runtime reference (like during
>> probe), we can do a temporary pm_runtime_get() /
>> pm_runtime_put_autosuspend().
> 
> This looks good. I'd be more than welcome to review such series.
> 
> Note: I think this would require using
> drm_bridge_connector_enable_hpd() in the DP code.
> Hopefully at some point we would be able to move all
> drm_bridge_connector calls to the core msm layer.
> --
> With best wishes
> Dmitry


Thanks for the proposals.

In general I would break up this task as follows:

1) Re-factoring dp/edp parsing code to move it to probe ( its currently
done in bind ). So not sure what dependencies we will uncover there.
Nonetheless, lets assume for now it can be done.

2) Then bind all the power resources needed for AUX in pm_runtime_ops

3) Handle EPROBE_DEFER cases of the panel-eDP aux device

4) Probably the biggest from our point of view --- makes sure none of 
this breaks DP/eDP

Since QC will be taking ownership of all of this, I would still suggest 
land this series first so that basic display functionality on sc7280
chromebooks works, unblocks more developers and this program and we can
internally evaluate all of this and post the changes as-and-when ready
for review.

So, I suggest/request acking this current one after
fixing the other comments (unrelated to this re-factor) which have been 
given so far ofcourse, as we all agree this is not breaking and seems 
pretty reasonable short term.

Doug, you can track this re-factor with a different bug so that all this 
discussion remains intact.

Thanks

Abhinav

Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus

[Dmitry Baryshkov]

On Thu, 7 Apr 2022 at 23:11, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
> Hi Doug and Dmitry
>
> Sorry, but I caught up on this email just now.
>
> Some comments below.
>
> Thanks
>
> Abhinav
> On 4/7/2022 10:07 AM, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, Apr 7, 2022 at 7:19 AM Sankeerth Billakanti (QUIC)
> > <quic_sbillaka@quicinc.com> wrote:
> >>
> >> Hi Dmitry and Doug,
> >>
> >>> Hi,
> >>>
> >>> On Tue, Apr 5, 2022 at 10:36 AM Dmitry Baryshkov
> >>> <dmitry.baryshkov@linaro.org> wrote:
> >>>>
> >>>> On 05/04/2022 20:02, Doug Anderson wrote:
> >>>>> Hi,
> >>>>>
> >>>>> On Tue, Apr 5, 2022 at 5:54 AM Dmitry Baryshkov
> >>>>> <dmitry.baryshkov@linaro.org> wrote:
> >>>>>>> 3. For DP and eDP HPD means something a little different.
> >>>>>>> Essentially there are two concepts: a) is a display physically
> >>>>>>> connected and b) is the display powered up and ready. For DP, the
> >>>>>>> two are really tied together. From the kernel's point of view you
> >>>>>>> never "power down" a DP display and you can't detect that it's
> >>>>>>> physically connected until it's ready. Said another way, on you
> >>>>>>> tie "is a display there" to the HPD line and the moment a display
> >>>>>>> is there it's ready for you to do AUX transfers. For eDP, in the
> >>>>>>> lowest power state of a display it _won't_ assert its "HPD"
> >>>>>>> signal. However, it's still physically present. For eDP you simply
> >>>>>>> have to _assume_ it's present without any actual proof since you
> >>>>>>> can't get proof until you power it up. Thus for eDP, you report
> >>>>>>> that the display is there as soon as we're asked. We can't _talk_
> >>>>>>> to the display yet, though. So in get_modes() we need to be able
> >>>>>>> to power the display on enough to talk over the AUX channel to it.
> >>>>>>> As part of this, we wait for the signal named "HPD" which really means
> >>> "panel finished powering on" in this context.
> >>>>>>>
> >>>>>>> NOTE: for aux transfer, we don't have the _display_ pipe and
> >>>>>>> clocks running. We only have enough stuff running to do the AUX
> >>> transfer.
> >>>>>>> We're not clocking out pixels. We haven't fully powered on the
> >>>>>>> display. The AUX transfer is designed to be something that can be
> >>>>>>> done early _before_ you turn on the display.
> >>>>>>>
> >>>>>>>
> >>>>>>> OK, so basically that was a longwinded way of saying: yes, we
> >>>>>>> could avoid the AUX transfer in probe, but we can't wait all the
> >>>>>>> way to enable. We have to be able to transfer in get_modes(). If
> >>>>>>> you think that's helpful I think it'd be a pretty easy patch to
> >>>>>>> write even if it would look a tad bit awkward IMO. Let me know if
> >>>>>>> you want me to post it up.
> >>>>>>
> >>>>>> I think it would be a good idea. At least it will allow us to
> >>>>>> judge, which is the more correct way.
> >>>>>
> >>>>> I'm still happy to prototype this, but the more I think about it the
> >>>>> more it feels like a workaround for the Qualcomm driver. The eDP
> >>>>> panel driver is actually given a pointer to the AUX bus at probe
> >>>>> time. It's really weird to say that we can't do a transfer on it
> >>>>> yet... As you said, this is a little sideband bus. It should be able
> >>>>> to be used without all the full blown infra of the rest of the driver.
> >>>>
> >>>> Yes, I have that feeling too. However I also have a feeling that just
> >>>> powering up the PHY before the bus probe is ... a hack. There are no
> >>>> obvious stopgaps for the driver not to power it down later.
> >>>
>
> Lets go back to why we need to power up the PHY before the bus probe.
>
> We need to power up PHY before bus probe because panel-eDP tries to read
> the EDID in probe() for the panel_id. Not get_modes().
>
> So doug, I didnt follow your comment that panel-eDP only does EDID read
> in get_modes()
>
>         panel_id = drm_edid_get_panel_id(panel->ddc);
>         if (!panel_id) {
>                 dev_err(dev, "Couldn't identify panel via EDID\n");
>                 ret = -EIO;
>                 goto exit;
>         }
>
> If we do not need this part, we really dont need to power up the PHY
> before the probe(). The hack which dmitry was referring to.
>
> So this is boiling down to why or how panel-eDP was originally designed.

Well, it's not just panel-edp. It boils down to the DP-AUX bus design
vs DRM design. However in Doug's defense I should admit that I can not
come up with a significantly cleaner solution.

Just to emphasise (or to repeat): for all other display
panels/bridges, we either do not use a sidechannel bus or the
sidechannel bus (i2c, spi, platform) is managed outside of the DRM
framework. Thus it's possible to create the source in the drm's driver
probe path and then in the component's bind() path check (and return
an error) if the sink device wasn't yet probed successfully. The
source can then either communicate with the sink using the sidechannel
bus provided elsewhere or (e.g. in case of purely DSI panel), defer
communication till the moment the display path is fully available
(after encoder enablement).

For the DP/eDP the sidechannel (DP AUX) bus is closer to the display
path. It can not be used separately. For DP we can defer all
communication till the moment we know (through the DP/USB-C/etc
hotplug mechanism) that the sink is really available and running.

The eDP is being caught in between these two approaches:

For example sn65dsi86 and tegra have separate dpaux and separate
bridge/output devices. Thus dpaux'es probe() methos populates DP AUX
bus, then a separate device checks whether the panel/bridge has become
available in its own probe() method.

The ps8640 driver looks 'working by coincidence'. It calls
dp_aux_populate, then immediately after the function returns it checks
for the panel. If panel-edp is built as a module, the probe might fail
easily.
The anx7625 driver has the same kind of issue. The DP AUX bus is
populated from the probe() and after some additional work the panel is
being checked.
This design is fragile and from my quick glance it can break (or be
broken) too easy. It reminds me of our drm msm 'probe' loops
preventing the device to boot completely if the dsi bridge/panel could
not be probed in time.

If we talk about DP AUX bus EP drivers, both panel-edp and
panel-samsung-atna33xc20 (the only EP drivers present in Linux master)
expect that they can access DPCD from the probe method.

Now back to Qualcomm DP driver. We do not have a separate dpaux
entity. If leave aside the idea of adding a separate 'bus available'
callback, I see two possible solutions:

- Implement separate lightweight eDP driver sharing source with the DP
driver. Make it skip all the DP HPD craziness, init PHY and call
devm_of_dp_aux_ep_populate_ep_devices() from the probe method, check
in the bind method that the next bridge is available. However this can
potentially break ports which can be used either in the DP or in eDP
mode.

or

- Modify existing Qualcomm DP driver to return -EPROBE_DEFER from
dp_aux_transfer() if the AUX bus is not available. Make the driver
init PHY from probe() if it is running in the eDP mode. Populate DP
AUX bus from probe(). Check for the next bridge in dp_bind().

There might be potentially other possibilities, but I think you have
my main idea. Create the bus in probe(), check for the bridge in
bind().

or

- Create a bus at some point in bind. Forbid (and document that) AUX
access from EP probe(). Access it only from get_modes().


>
> >>> This is why I think we need to move to Runtime PM to manage this. Basically:
> >>>
> >>> 1. When an AUX transfer happens, you grab a PM runtime reference that
> >>> _that_ powers up the PHY.
>
> This will not be trivial and needs to be scoped out as sankeerth said
> but if the above is the only concern, why do we need to do this? There
> seems to be an explanation why we are doing this and its not a hack.
>
> How would Dmitry's rework address this? We need some RFC to conclude on
> that first.

Just to put things clear: I do not have plans to work on either of my
suggestions at least in the next few months. I do not have eDP hardware at hand.




>
> >>>
> >>> 2. At the end of the AUX transfer function, you do a "put_autosuspend".
> >>>
> >>> Then it becomes not a hack, right?
> >>>
> >>>
> >>
> >> pm runtime ops needs to be implemented for both eDP and DP. This change
> >> take good amount of planning and code changes as it affects DP also.
> >>
> >> Because this patch series consist of basic eDP changes for SC7280 bootup,
> >> shall we take this pm_runtime implementation in subsequent patch series?
> >
> > Dmitry is the real decision maker here, but in my opinion it would be
> > OK to get something landed first that worked OK and wasn't taking us
> > too far in the wrong direction and then we could get a follow up patch
> > to move to pm_runtime.
>
> I would say the discussion changed into a direction of implementing
> pm-runtime because the current patch series does what it takes to adhere
> to panel-eDP's design along with aux bus requirements of PHY needing to
> be on.
>
> So doug, to answer your questions here:
>
> "So I guess the net result is maybe we should just keep it where it is.
> Long term I'd be interested in knowing if there's a reason why we
> can't structure the driver so that AUX transfers can happen with less
> intertwining with the rest of the code, but that can happen later. I
> would expect that you'd basically just need clocks and regulators on
> and maybe your PHY on."
>
> Yes PHY needs to be absolutely on and configured before aux transfers.
>
> If we want to change that up to stop reading the panel_id in the panel
> probe() and do it later, perhaps some of the changes done here are not
> needed.
>
> It only seems reasonable that we first prototype that in a separate
> patch even a RFC perhaps and take this further as these set of changes
> are needed for basic display functionality on sc7280 chromebooks.
>
> Let us know what are the concerns with doing it in a follow up change.
>
> Thanks
>
> Abhinav



--
With best wishes
Dmitry

Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus

[Doug Anderson]

Hi,

On Thu, Apr 7, 2022 at 4:36 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> The ps8640 driver looks 'working by coincidence'. It calls
> dp_aux_populate, then immediately after the function returns it checks
> for the panel. If panel-edp is built as a module, the probe might fail
> easily.
> The anx7625 driver has the same kind of issue. The DP AUX bus is
> populated from the probe() and after some additional work the panel is
> being checked.
> This design is fragile and from my quick glance it can break (or be
> broken) too easy. It reminds me of our drm msm 'probe' loops
> preventing the device to boot completely if the dsi bridge/panel could
> not be probed in time.

I did spend some time thinking about this, at least for ps8640. I
believe that as long as the panel's probe isn't asynchronous.
Basically if the panel isn't ready then ps8640 should return and we'll
retry later. I do remember the probe loops that we used to have with
msm and I don't _think_ this would trigger it.

That being said, if we need to separate out the AUX bus into a
sub-device like we did in sn65dsi86 we certainly could.

-Doug

Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus

[Dmitry Baryshkov]

On Fri, 8 Apr 2022 at 03:28, Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Thu, Apr 7, 2022 at 4:36 PM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > The ps8640 driver looks 'working by coincidence'. It calls
> > dp_aux_populate, then immediately after the function returns it checks
> > for the panel. If panel-edp is built as a module, the probe might fail
> > easily.
> > The anx7625 driver has the same kind of issue. The DP AUX bus is
> > populated from the probe() and after some additional work the panel is
> > being checked.
> > This design is fragile and from my quick glance it can break (or be
> > broken) too easy. It reminds me of our drm msm 'probe' loops
> > preventing the device to boot completely if the dsi bridge/panel could
> > not be probed in time.
>
> I did spend some time thinking about this, at least for ps8640. I
> believe that as long as the panel's probe isn't asynchronous.

By panel probe (as a probe of any device) is potentially asynchronous.
As in your example, the PWM might not be present, the regulator probe
might have been delayed, the panel-edp might be built as a module,
which is not present for some reason.

> Basically if the panel isn't ready then ps8640 should return and we'll
> retry later. I do remember the probe loops that we used to have with
> msm and I don't _think_ this would trigger it.

I don't have proof here. But as I wrote, this thing might break at some point.

> That being said, if we need to separate out the AUX bus into a
> sub-device like we did in sn65dsi86 we certainly could.

Ideally we should separate the "bridge" subdevice, like it was done in
sn65dsi86.
So that the aux host probes, registers the EP device, then the bridge
device can not be probed (and thus the drm_bridge is not created)
until the next  bridge becomes available.

-- 
With best wishes
Dmitry

Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus

[Doug Anderson]

Hi,

On Fri, Apr 8, 2022 at 5:13 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Fri, 8 Apr 2022 at 03:28, Doug Anderson <dianders@chromium.org> wrote:
> >
> > Hi,
> >
> > On Thu, Apr 7, 2022 at 4:36 PM Dmitry Baryshkov
> > <dmitry.baryshkov@linaro.org> wrote:
> > >
> > > The ps8640 driver looks 'working by coincidence'. It calls
> > > dp_aux_populate, then immediately after the function returns it checks
> > > for the panel. If panel-edp is built as a module, the probe might fail
> > > easily.
> > > The anx7625 driver has the same kind of issue. The DP AUX bus is
> > > populated from the probe() and after some additional work the panel is
> > > being checked.
> > > This design is fragile and from my quick glance it can break (or be
> > > broken) too easy. It reminds me of our drm msm 'probe' loops
> > > preventing the device to boot completely if the dsi bridge/panel could
> > > not be probed in time.
> >
> > I did spend some time thinking about this, at least for ps8640. I
> > believe that as long as the panel's probe isn't asynchronous.
>
> By panel probe (as a probe of any device) is potentially asynchronous.
> As in your example, the PWM might not be present, the regulator probe
> might have been delayed, the panel-edp might be built as a module,
> which is not present for some reason.

Well, in those cases it's not exactly asynchronous, or at least not in
the way I was thinking about. Either it will work now, or we should
try again later when more drivers have probed. The case I was worried
about was:

1. We call devm_of_dp_aux_populate_ep_devices()

2. devm_of_dp_aux_populate_ep_devices() kicks off a probe to the panel
in the background

3. devm_of_dp_aux_populate_ep_devices() returns to us without waiting
for the panel's probe to finish.

I think that's possible if the panel driver sets PROBE_PREFER_ASYNCHRONOUS.

I was less worried about the resources missing since I thought that
would be handled by the normal probe deferral case. IIRC the "infinite
loop" that we used to end up with MSM's probe was because something
about the way the MSM DRM driver worked would cause the deferral
mechanisms to retry instantly each time we deferred. I don't remember
exactly what triggered it, but I don't _think_ that's true for ps8640?


> > Basically if the panel isn't ready then ps8640 should return and we'll
> > retry later. I do remember the probe loops that we used to have with
> > msm and I don't _think_ this would trigger it.
>
> I don't have proof here. But as I wrote, this thing might break at some point.
>
> > That being said, if we need to separate out the AUX bus into a
> > sub-device like we did in sn65dsi86 we certainly could.
>
> Ideally we should separate the "bridge" subdevice, like it was done in
> sn65dsi86.
> So that the aux host probes, registers the EP device, then the bridge
> device can not be probed (and thus the drm_bridge is not created)
> until the next  bridge becomes available.

You're definitely right, that's best. I was hesitant to force the
issue during review of the ps8640 because it adds a bunch of
complexity and didn't _seem_ to be needed. Maybe it makes sense to
just code it up, though...

-Doug

Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus

[Dmitry Baryshkov]

On Fri, 8 Apr 2022 at 16:56, Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Fri, Apr 8, 2022 at 5:13 AM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > On Fri, 8 Apr 2022 at 03:28, Doug Anderson <dianders@chromium.org> wrote:
> > >
> > > Hi,
> > >
> > > On Thu, Apr 7, 2022 at 4:36 PM Dmitry Baryshkov
> > > <dmitry.baryshkov@linaro.org> wrote:
> > > >
> > > > The ps8640 driver looks 'working by coincidence'. It calls
> > > > dp_aux_populate, then immediately after the function returns it checks
> > > > for the panel. If panel-edp is built as a module, the probe might fail
> > > > easily.
> > > > The anx7625 driver has the same kind of issue. The DP AUX bus is
> > > > populated from the probe() and after some additional work the panel is
> > > > being checked.
> > > > This design is fragile and from my quick glance it can break (or be
> > > > broken) too easy. It reminds me of our drm msm 'probe' loops
> > > > preventing the device to boot completely if the dsi bridge/panel could
> > > > not be probed in time.
> > >
> > > I did spend some time thinking about this, at least for ps8640. I
> > > believe that as long as the panel's probe isn't asynchronous.
> >
> > By panel probe (as a probe of any device) is potentially asynchronous.
> > As in your example, the PWM might not be present, the regulator probe
> > might have been delayed, the panel-edp might be built as a module,
> > which is not present for some reason.
>
> Well, in those cases it's not exactly asynchronous, or at least not in
> the way I was thinking about. Either it will work now, or we should
> try again later when more drivers have probed. The case I was worried
> about was:
>
> 1. We call devm_of_dp_aux_populate_ep_devices()
>
> 2. devm_of_dp_aux_populate_ep_devices() kicks off a probe to the panel
> in the background
>
> 3. devm_of_dp_aux_populate_ep_devices() returns to us without waiting
> for the panel's probe to finish.
>
> I think that's possible if the panel driver sets PROBE_PREFER_ASYNCHRONOUS.

That would be a separate story, yes. However the general case is still
valid: one can not guarantee that the panel device is available
immediately after aux bus population.
So ps8640 works at this moment and in the particular configuration.

>
> I was less worried about the resources missing since I thought that
> would be handled by the normal probe deferral case. IIRC the "infinite
> loop" that we used to end up with MSM's probe was because something
> about the way the MSM DRM driver worked would cause the deferral
> mechanisms to retry instantly each time we deferred. I don't remember
> exactly what triggered it, but I don't _think_ that's true for ps8640?

I'm not sure either. If you have a system with that bridge, it can be
easily tried by returning -EPROBE_DEFER from the panel driver's
probe().

For the msm driver it was the following sequence of events:
- mdss probes
- mdss creates subdevices including dsi host
- subdevices are probed
- mdss drivers tries to perform component binding
- dsi host determines that the next item is not available
- it returns -EPROBE_DEFER to the component bind
- mdss reverts registration of subdevices, returning probe defer.

However as there were devices added to the device list, the deferral
list was retried immediately. Thus we faced the probe loop.

I think this looks close to the ps8640, but I wouldn't bet on that.

> > > Basically if the panel isn't ready then ps8640 should return and we'll
> > > retry later. I do remember the probe loops that we used to have with
> > > msm and I don't _think_ this would trigger it.
> >
> > I don't have proof here. But as I wrote, this thing might break at some point.
> >
> > > That being said, if we need to separate out the AUX bus into a
> > > sub-device like we did in sn65dsi86 we certainly could.
> >
> > Ideally we should separate the "bridge" subdevice, like it was done in
> > sn65dsi86.
> > So that the aux host probes, registers the EP device, then the bridge
> > device can not be probed (and thus the drm_bridge is not created)
> > until the next  bridge becomes available.
>
> You're definitely right, that's best. I was hesitant to force the
> issue during review of the ps8640 because it adds a bunch of
> complexity and didn't _seem_ to be needed. Maybe it makes sense to
> just code it up, though...

As I wrote, the test is easy. If the system boots fine, there is no
need to fix that immediately.
However I think in general we should stop depending on the panel being
available right after populating the aux bus.

--
With best wishes
Dmitry

[PATCH v6 2/8] drm/msm/dp: wait for hpd high before aux transaction

[Sankeerth Billakanti]

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 controller
driver needs to wait for the hpd line to be asserted by the sink before
performing 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>
---

Changes in v6:
  - Wait for hpd high only for eDP
  - Split into smaller patches

 drivers/gpu/drm/msm/dp/dp_aux.c     | 13 ++++++++++++-
 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 |  3 ++-
 5 files changed, 30 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..a217c80 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;
 
 	struct drm_dp_aux dp_aux;
 };
@@ -337,6 +338,14 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
 		goto exit;
 	}
 
+	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 +500,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)
 {
 	struct dp_aux_private *aux;
 
@@ -506,6 +516,7 @@ struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog)
 
 	init_completion(&aux->comp);
 	aux->cmd_busy = false;
+	aux->is_edp = is_edp;
 	mutex_init(&aux->mutex);
 
 	aux->dev = dev;
diff --git a/drivers/gpu/drm/msm/dp/dp_aux.h b/drivers/gpu/drm/msm/dp/dp_aux.h
index 82afc8d..c99aeec 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.h
+++ b/drivers/gpu/drm/msm/dp/dp_aux.h
@@ -16,7 +16,8 @@ void dp_aux_init(struct drm_dp_aux *dp_aux);
 void dp_aux_deinit(struct drm_dp_aux *dp_aux);
 void dp_aux_reconfig(struct drm_dp_aux *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);
 void dp_aux_put(struct drm_dp_aux *aux);
 
 #endif /*__DP_AUX_H_*/
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
index fac815f..b6add4e 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -242,6 +242,19 @@ 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;
+	struct dp_catalog_private *catalog = container_of(dp_catalog,
+				struct dp_catalog_private, dp_catalog);
+
+	/* poll for hpd connected status every 2ms and timeout after 500ms */
+	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, 500000);
+}
+
 static void dump_regs(void __iomem *base, int len)
 {
 	int i;
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h
index 7dea101..45140a3 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.h
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
@@ -84,6 +84,7 @@ int dp_catalog_aux_clear_hw_interrupts(struct dp_catalog *dp_catalog);
 void dp_catalog_aux_reset(struct dp_catalog *dp_catalog);
 void dp_catalog_aux_enable(struct dp_catalog *dp_catalog, bool enable);
 void dp_catalog_aux_update_cfg(struct dp_catalog *dp_catalog);
+int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog);
 u32 dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog);
 
 /* DP Controller APIs */
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index e082d02..274bbcf 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -755,6 +755,7 @@ static void dp_display_deinit_sub_modules(struct dp_display_private *dp)
 static int dp_init_sub_modules(struct dp_display_private *dp)
 {
 	int rc = 0;
+	bool is_edp = (dp->dp_display.connector_type == DRM_MODE_CONNECTOR_eDP);
 	struct device *dev = &dp->pdev->dev;
 	struct dp_usbpd_cb *cb = &dp->usbpd_cb;
 	struct dp_panel_in panel_in = {
@@ -798,7 +799,7 @@ static int dp_init_sub_modules(struct dp_display_private *dp)
 		goto error;
 	}
 
-	dp->aux = dp_aux_get(dev, dp->catalog);
+	dp->aux = dp_aux_get(dev, dp->catalog, is_edp);
 	if (IS_ERR(dp->aux)) {
 		rc = PTR_ERR(dp->aux);
 		DRM_ERROR("failed to initialize aux, rc = %d\n", rc);
-- 
2.7.4

Re: [PATCH v6 2/8] drm/msm/dp: wait for hpd high before aux transaction

[Doug Anderson]

Hi,

On Wed, Mar 30, 2022 at 9:03 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 performing any aux transactions. The sink

s/performing/perform

> 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
> performing 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>
> ---
>
> Changes in v6:
>   - Wait for hpd high only for eDP
>   - Split into smaller patches
>
>  drivers/gpu/drm/msm/dp/dp_aux.c     | 13 ++++++++++++-
>  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 |  3 ++-
>  5 files changed, 30 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..a217c80 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;
>
>         struct drm_dp_aux dp_aux;
>  };
> @@ -337,6 +338,14 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
>                 goto exit;
>         }
>
> +       if (aux->is_edp) {

Adding a comment about _why_ you're doing this just for eDP would
probably be a good idea. Like maybe:

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


> @@ -491,7 +500,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 your indentation of the 2nd line isn't quite right.


> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.h b/drivers/gpu/drm/msm/dp/dp_aux.h
> index 82afc8d..c99aeec 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.h
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.h
> @@ -16,7 +16,8 @@ void dp_aux_init(struct drm_dp_aux *dp_aux);
>  void dp_aux_deinit(struct drm_dp_aux *dp_aux);
>  void dp_aux_reconfig(struct drm_dp_aux *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 your indentation of the 2nd line isn't quite right.


Things are pretty much nits, so FWIW:

Reviewed-by: Douglas Anderson <dianders@chromium.org>

RE: [PATCH v6 2/8] drm/msm/dp: wait for hpd high before aux transaction

[Sankeerth Billakanti (QUIC)]

Hi Doug,

> On Wed, Mar 30, 2022 at 9:03 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 performing any aux transactions. The sink
> 
> s/performing/perform
> 
> > 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 performing 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>
> > ---
> >
> > Changes in v6:
> >   - Wait for hpd high only for eDP
> >   - Split into smaller patches
> >
> >  drivers/gpu/drm/msm/dp/dp_aux.c     | 13 ++++++++++++-
> >  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 |  3 ++-
> >  5 files changed, 30 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..a217c80 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;
> >
> >         struct drm_dp_aux dp_aux;
> >  };
> > @@ -337,6 +338,14 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux
> *dp_aux,
> >                 goto exit;
> >         }
> >
> > +       if (aux->is_edp) {
> 
> Adding a comment about _why_ you're doing this just for eDP would
> probably be a good idea. Like maybe:
> 
> /*
>  * 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.
>  */
> 
> 

Okay. Will add it

> > @@ -491,7 +500,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 your indentation of the 2nd line isn't quite right.
> 
>

I moved bool is_edp into the next line. In vim , it was sowing fine. I'll check
 
> > diff --git a/drivers/gpu/drm/msm/dp/dp_aux.h
> > b/drivers/gpu/drm/msm/dp/dp_aux.h index 82afc8d..c99aeec 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_aux.h
> > +++ b/drivers/gpu/drm/msm/dp/dp_aux.h
> > @@ -16,7 +16,8 @@ void dp_aux_init(struct drm_dp_aux *dp_aux);  void
> > dp_aux_deinit(struct drm_dp_aux *dp_aux);  void dp_aux_reconfig(struct
> > drm_dp_aux *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 your indentation of the 2nd line isn't quite right.
> 

I'll check

> 
> Things are pretty much nits, so FWIW:
> 
> Reviewed-by: Douglas Anderson <dianders@chromium.org>

Thank you,
Sankeerth

[PATCH v6 3/8] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP

[Sankeerth Billakanti]

The panel-edp enables the eDP panel power during probe, get_modes
and 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>
---
 drivers/gpu/drm/msm/dp/dp_catalog.c |  4 ----
 drivers/gpu/drm/msm/dp/dp_display.c | 24 ++++++++++++++++++++++--
 2 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
index b6add4e..3c16f95 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -582,10 +582,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);
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 274bbcf..888ff03 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -677,7 +677,8 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
 	dp_display_handle_plugged_change(&dp->dp_display, false);
 
 	/* enable HDP plug interrupt to prepare for next plugin */
-	dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, true);
+	if (dp->dp_display.connector_type == DRM_MODE_CONNECTOR_DisplayPort)
+		dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, true);
 
 	DRM_DEBUG_DP("After, type=%d hpd_state=%d\n",
 			dp->dp_display.connector_type, state);
@@ -1087,10 +1088,17 @@ void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp)
 
 static void dp_display_config_hpd(struct dp_display_private *dp)
 {
-
 	dp_display_host_init(dp);
+
 	dp_catalog_ctrl_hpd_config(dp->catalog);
 
+	/* Enable plug and unplug interrupts only for external DisplayPort */
+	if (dp->dp_display.connector_type == DRM_MODE_CONNECTOR_DisplayPort)
+		dp_catalog_hpd_config_intr(dp->catalog,
+				DP_DP_HPD_PLUG_INT_MASK |
+				DP_DP_HPD_UNPLUG_INT_MASK,
+				true);
+
 	/* Enable interrupt first time
 	 * we are leaving dp clocks on during disconnect
 	 * and never disable interrupt
@@ -1374,6 +1382,12 @@ static int dp_pm_resume(struct device *dev)
 	dp_catalog_ctrl_hpd_config(dp->catalog);
 
 
+	if (dp->dp_display.connector_type == DRM_MODE_CONNECTOR_DisplayPort)
+		dp_catalog_hpd_config_intr(dp->catalog,
+				DP_DP_HPD_PLUG_INT_MASK |
+				DP_DP_HPD_UNPLUG_INT_MASK,
+				true);
+
 	if (dp_catalog_link_is_connected(dp->catalog)) {
 		/*
 		 * set sink to normal operation mode -- D0
@@ -1639,6 +1653,9 @@ void dp_bridge_enable(struct drm_bridge *drm_bridge)
 		return;
 	}
 
+	if (dp->connector_type == DRM_MODE_CONNECTOR_eDP)
+		dp_hpd_plug_handle(dp_display, 0);
+
 	mutex_lock(&dp_display->event_mutex);
 
 	/* stop sentinel checking */
@@ -1703,6 +1720,9 @@ void dp_bridge_post_disable(struct drm_bridge *drm_bridge)
 
 	dp_display = container_of(dp, struct dp_display_private, dp_display);
 
+	if (dp->connector_type == DRM_MODE_CONNECTOR_eDP)
+		dp_hpd_unplug_handle(dp_display, 0);
+
 	mutex_lock(&dp_display->event_mutex);
 
 	/* stop sentinel checking */
-- 
2.7.4

Re: [PATCH v6 3/8] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP

[Doug Anderson]

Hi,

On Wed, Mar 30, 2022 at 9:03 AM Sankeerth Billakanti
<quic_sbillaka@quicinc.com> wrote:
>
> @@ -1374,6 +1382,12 @@ static int dp_pm_resume(struct device *dev)
>         dp_catalog_ctrl_hpd_config(dp->catalog);
>
>
> +       if (dp->dp_display.connector_type == DRM_MODE_CONNECTOR_DisplayPort)
> +               dp_catalog_hpd_config_intr(dp->catalog,
> +                               DP_DP_HPD_PLUG_INT_MASK |
> +                               DP_DP_HPD_UNPLUG_INT_MASK,
> +                               true);
> +

nit: why are there two blank lines above?


> @@ -1639,6 +1653,9 @@ void dp_bridge_enable(struct drm_bridge *drm_bridge)
>                 return;
>         }
>
> +       if (dp->connector_type == DRM_MODE_CONNECTOR_eDP)
> +               dp_hpd_plug_handle(dp_display, 0);
> +

Should you add a "pre_enable" and do it there? That would make it more
symmetric with the fact that you have the countertpart in
"post_disable".


Overall: I'm probably not familiar enough with this code to give it a
full review. I'm hoping that Dmitry knows it well enough... ;-)


-Doug

RE: [PATCH v6 3/8] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP

[Sankeerth Billakanti (QUIC)]

Hi Doug,

> On Wed, Mar 30, 2022 at 9:03 AM Sankeerth Billakanti
> <quic_sbillaka@quicinc.com> wrote:
> >
> > @@ -1374,6 +1382,12 @@ static int dp_pm_resume(struct device *dev)
> >         dp_catalog_ctrl_hpd_config(dp->catalog);
> >
> >
> > +       if (dp->dp_display.connector_type ==
> DRM_MODE_CONNECTOR_DisplayPort)
> > +               dp_catalog_hpd_config_intr(dp->catalog,
> > +                               DP_DP_HPD_PLUG_INT_MASK |
> > +                               DP_DP_HPD_UNPLUG_INT_MASK,
> > +                               true);
> > +
> 
> nit: why are there two blank lines above?
> 
>

Will remove a blank line.
 
> > @@ -1639,6 +1653,9 @@ void dp_bridge_enable(struct drm_bridge
> *drm_bridge)
> >                 return;
> >         }
> >
> > +       if (dp->connector_type == DRM_MODE_CONNECTOR_eDP)
> > +               dp_hpd_plug_handle(dp_display, 0);
> > +
> 
> Should you add a "pre_enable" and do it there? That would make it more
> symmetric with the fact that you have the countertpart in "post_disable".
>

We want to handle the screen off or eDP disable like a cable unplug so that it can be integrated into the dp code.
So, we can move plug_handle into pre_enable and the unplug_handle to pre_disable.

If we do unplug in post_disable, we need to handle the state changes also. It will complicate the driver code.

> 
> Overall: I'm probably not familiar enough with this code to give it a full
> review. I'm hoping that Dmitry knows it well enough... ;-)
> 
> 
> -Doug

Thank you,
Sankeerth

[PATCH v6 4/8] drm/msm/dp: avoid handling masked interrupts

[Sankeerth Billakanti]

The interrupt register will still reflect the connect and disconnect
interrupt status without generating an actual HW interrupt.
The controller driver should not handle those masked interrupts.

Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
---
 drivers/gpu/drm/msm/dp/dp_catalog.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
index 3c16f95..1809ce2 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -608,13 +608,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 & (DP_DP_HPD_STATE_STATUS_MASK | mask);
 }
 
 int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog)
-- 
2.7.4

Re: [PATCH v6 4/8] drm/msm/dp: avoid handling masked interrupts

[Dmitry Baryshkov]

On Wed, 30 Mar 2022 at 19:03, Sankeerth Billakanti
<quic_sbillaka@quicinc.com> wrote:
>
> The interrupt register will still reflect the connect and disconnect
> interrupt status without generating an actual HW interrupt.
> The controller driver should not handle those masked interrupts.
>
> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
> ---
>  drivers/gpu/drm/msm/dp/dp_catalog.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
> index 3c16f95..1809ce2 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> @@ -608,13 +608,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 & (DP_DP_HPD_STATE_STATUS_MASK | mask);

I suspect that the logic is inverted here. Shouldn't it be:

return isr & DP_DP_HPD_STATE_STATUS_MASK & mask;

?

>  }
>
>  int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog)
> --
> 2.7.4
>


-- 
With best wishes
Dmitry

RE: [PATCH v6 4/8] drm/msm/dp: avoid handling masked interrupts

[Sankeerth Billakanti (QUIC)]

Hi Dmitry,

> On Wed, 30 Mar 2022 at 19:03, Sankeerth Billakanti
> <quic_sbillaka@quicinc.com> wrote:
> >
> > The interrupt register will still reflect the connect and disconnect
> > interrupt status without generating an actual HW interrupt.
> > The controller driver should not handle those masked interrupts.
> >
> > Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
> > ---
> >  drivers/gpu/drm/msm/dp/dp_catalog.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
> > b/drivers/gpu/drm/msm/dp/dp_catalog.c
> > index 3c16f95..1809ce2 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> > @@ -608,13 +608,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 & (DP_DP_HPD_STATE_STATUS_MASK | mask);
> 
> I suspect that the logic is inverted here. Shouldn't it be:
> 
> return isr & DP_DP_HPD_STATE_STATUS_MASK & mask;
> 
> ?
> 
 
The value of DP_DP_HPD_STATE_STATUS_MASK is 0xE0000000 and the value of the read
interrupt mask variable could be is 0xF.

The mask value is indicated via the register, REG_DP_DP_HPD_INT_MASK, bits 3:0.
The HPD status is indicated via a different read-only register REG_DP_DP_HPD_INT_STATUS, bits 31:29. 

isr & DP_DP_HPD_STATE_STATUS_MASK & mask, will return 0 always.

> >  }
> >
> >  int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog)
> > --
> > 2.7.4
> >
> 
> 
> --
> With best wishes
> Dmitry

Thank you,
Sankeerth

Re: [PATCH v6 4/8] drm/msm/dp: avoid handling masked interrupts

[Dmitry Baryshkov]

On 31/03/2022 08:53, Sankeerth Billakanti (QUIC) wrote:
> Hi Dmitry,
> 
>> On Wed, 30 Mar 2022 at 19:03, Sankeerth Billakanti
>> <quic_sbillaka@quicinc.com> wrote:
>>>
>>> The interrupt register will still reflect the connect and disconnect
>>> interrupt status without generating an actual HW interrupt.
>>> The controller driver should not handle those masked interrupts.
>>>
>>> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
>>> ---
>>>   drivers/gpu/drm/msm/dp/dp_catalog.c | 5 +++--
>>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
>>> b/drivers/gpu/drm/msm/dp/dp_catalog.c
>>> index 3c16f95..1809ce2 100644
>>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
>>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
>>> @@ -608,13 +608,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 & (DP_DP_HPD_STATE_STATUS_MASK | mask);
>>
>> I suspect that the logic is inverted here. Shouldn't it be:
>>
>> return isr & DP_DP_HPD_STATE_STATUS_MASK & mask;
>>
>> ?
>>
>   
> The value of DP_DP_HPD_STATE_STATUS_MASK is 0xE0000000 and the value of the read
> interrupt mask variable could be is 0xF.
> 
> The mask value is indicated via the register, REG_DP_DP_HPD_INT_MASK, bits 3:0.
> The HPD status is indicated via a different read-only register REG_DP_DP_HPD_INT_STATUS, bits 31:29.

I see. Maybe the following expression would be better?

return isr & (mask & ~DP_DP_HPD_INT_MASK);

> 
> isr & DP_DP_HPD_STATE_STATUS_MASK & mask, will return 0 always.
> 
>>>   }
>>>
>>>   int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog)
>>> --
>>> 2.7.4
>>>
>>
>>
>> --
>> With best wishes
>> Dmitry
> 
> Thank you,
> Sankeerth


-- 
With best wishes
Dmitry

RE: [PATCH v6 4/8] drm/msm/dp: avoid handling masked interrupts

[Sankeerth Billakanti]

Hi Dmitry,

> On 31/03/2022 08:53, Sankeerth Billakanti (QUIC) wrote:
> > Hi Dmitry,
> >
> >> On Wed, 30 Mar 2022 at 19:03, Sankeerth Billakanti
> >> <quic_sbillaka@quicinc.com> wrote:
> >>>
> >>> The interrupt register will still reflect the connect and disconnect
> >>> interrupt status without generating an actual HW interrupt.
> >>> The controller driver should not handle those masked interrupts.
> >>>
> >>> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
> >>> ---
> >>>   drivers/gpu/drm/msm/dp/dp_catalog.c | 5 +++--
> >>>   1 file changed, 3 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
> >>> b/drivers/gpu/drm/msm/dp/dp_catalog.c
> >>> index 3c16f95..1809ce2 100644
> >>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> >>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> >>> @@ -608,13 +608,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 & (DP_DP_HPD_STATE_STATUS_MASK | mask);
> >>
> >> I suspect that the logic is inverted here. Shouldn't it be:
> >>
> >> return isr & DP_DP_HPD_STATE_STATUS_MASK & mask;
> >>
> >> ?
> >>
> >
> > The value of DP_DP_HPD_STATE_STATUS_MASK is 0xE0000000 and the
> value
> > of the read interrupt mask variable could be is 0xF.
> >
> > The mask value is indicated via the register, REG_DP_DP_HPD_INT_MASK,
> bits 3:0.
> > The HPD status is indicated via a different read-only register
> REG_DP_DP_HPD_INT_STATUS, bits 31:29.
> 
> I see. Maybe the following expression would be better?
> 
> return isr & (mask & ~DP_DP_HPD_INT_MASK);
> 

I believe the confusion occurred because the DP_DP_HPD_STATE_STATUS_CONNECTED and others were defined under the same register definition as REG_DP_DP_HPD_INT_MASK
I will rearrange the definitions below.

#define REG_DP_DP_HPD_INT_MASK		(0x0000000C)
#define DP_DP_HPD_PLUG_INT_MASK		(0x00000001)
#define DP_DP_IRQ_HPD_INT_MASK		(0x00000002)
#define DP_DP_HPD_REPLUG_INT_MASK	(0x00000004)
#define DP_DP_HPD_UNPLUG_INT_MASK	(0x00000008)
#define DP_DP_HPD_INT_MASK		(DP_DP_HPD_PLUG_INT_MASK | \
						DP_DP_IRQ_HPD_INT_MASK | \
						DP_DP_HPD_REPLUG_INT_MASK | \
						DP_DP_HPD_UNPLUG_INT_MASK)

Below are status bits from register REG_DP_DP_HPD_INT_STATUS

#define DP_DP_HPD_STATE_STATUS_CONNECTED	(0x40000000)
#define DP_DP_HPD_STATE_STATUS_PENDING		(0x20000000)
#define DP_DP_HPD_STATE_STATUS_DISCONNECTED	(0x00000000)
#define DP_DP_HPD_STATE_STATUS_MASK		(0xE0000000)

DP_DP_HPD_INT_MASK is 0xF and scope of mask variable is also 0xF (bits 3:0), mask & ~DP_DP_HPD_INT_MASK is 0 always.

For DP, we want to enable all interrupts.
So the programmed mask value is 0xF. We want to return 0x40000001 for connect and 8 for disconnect

For eDP, we want to disable the connect and disconnect interrupts. So, the mask will be 0x6 (i.e. DP_DP_IRQ_HPD_INT_MASK | DP_DP_HPD_REPLUG_INT_MASK)
We want to return 0x40000000 (or 0x20000000 based on hpd line status) and 0 for eDP connect and disconnect respectively.

> >
> > isr & DP_DP_HPD_STATE_STATUS_MASK & mask, will return 0 always.
> >
> >>>   }
> >>>
> >>>   int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog)
> >>> --
> >>> 2.7.4
> >>>
> >>
> >>
> >> --
> >> With best wishes
> >> Dmitry
> >
> > Thank you,
> > Sankeerth
> 
> 
> --
> With best wishes
> Dmitry

Thank you,
Sankeerth

Re: [PATCH v6 4/8] drm/msm/dp: avoid handling masked interrupts

[Dmitry Baryshkov]

On Thu, 31 Mar 2022 at 14:05, Sankeerth Billakanti
<sbillaka@qti.qualcomm.com> wrote:
>
> Hi Dmitry,
>
> > On 31/03/2022 08:53, Sankeerth Billakanti (QUIC) wrote:
> > > Hi Dmitry,
> > >
> > >> On Wed, 30 Mar 2022 at 19:03, Sankeerth Billakanti
> > >> <quic_sbillaka@quicinc.com> wrote:
> > >>>
> > >>> The interrupt register will still reflect the connect and disconnect
> > >>> interrupt status without generating an actual HW interrupt.
> > >>> The controller driver should not handle those masked interrupts.
> > >>>
> > >>> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
> > >>> ---
> > >>>   drivers/gpu/drm/msm/dp/dp_catalog.c | 5 +++--
> > >>>   1 file changed, 3 insertions(+), 2 deletions(-)
> > >>>
> > >>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
> > >>> b/drivers/gpu/drm/msm/dp/dp_catalog.c
> > >>> index 3c16f95..1809ce2 100644
> > >>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> > >>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> > >>> @@ -608,13 +608,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 & (DP_DP_HPD_STATE_STATUS_MASK | mask);
> > >>
> > >> I suspect that the logic is inverted here. Shouldn't it be:
> > >>
> > >> return isr & DP_DP_HPD_STATE_STATUS_MASK & mask;
> > >>
> > >> ?
> > >>
> > >
> > > The value of DP_DP_HPD_STATE_STATUS_MASK is 0xE0000000 and the
> > value
> > > of the read interrupt mask variable could be is 0xF.
> > >
> > > The mask value is indicated via the register, REG_DP_DP_HPD_INT_MASK,
> > bits 3:0.
> > > The HPD status is indicated via a different read-only register
> > REG_DP_DP_HPD_INT_STATUS, bits 31:29.
> >
> > I see. Maybe the following expression would be better?
> >
> > return isr & (mask & ~DP_DP_HPD_INT_MASK);

Ugh, excuse me please. I meant:

return isr & (mask | ~DP_DP_HPD_INT_MASK);

> >
>
> I believe the confusion occurred because the DP_DP_HPD_STATE_STATUS_CONNECTED and others were defined under the same register definition as REG_DP_DP_HPD_INT_MASK
> I will rearrange the definitions below.
>
> #define REG_DP_DP_HPD_INT_MASK          (0x0000000C)
> #define DP_DP_HPD_PLUG_INT_MASK         (0x00000001)
> #define DP_DP_IRQ_HPD_INT_MASK          (0x00000002)
> #define DP_DP_HPD_REPLUG_INT_MASK       (0x00000004)
> #define DP_DP_HPD_UNPLUG_INT_MASK       (0x00000008)
> #define DP_DP_HPD_INT_MASK              (DP_DP_HPD_PLUG_INT_MASK | \
>                                                 DP_DP_IRQ_HPD_INT_MASK | \
>                                                 DP_DP_HPD_REPLUG_INT_MASK | \
>                                                 DP_DP_HPD_UNPLUG_INT_MASK)
>
> Below are status bits from register REG_DP_DP_HPD_INT_STATUS
>
> #define DP_DP_HPD_STATE_STATUS_CONNECTED        (0x40000000)
> #define DP_DP_HPD_STATE_STATUS_PENDING          (0x20000000)
> #define DP_DP_HPD_STATE_STATUS_DISCONNECTED     (0x00000000)
> #define DP_DP_HPD_STATE_STATUS_MASK             (0xE0000000)
>
> DP_DP_HPD_INT_MASK is 0xF and scope of mask variable is also 0xF (bits 3:0), mask & ~DP_DP_HPD_INT_MASK is 0 always.
>
> For DP, we want to enable all interrupts.
> So the programmed mask value is 0xF. We want to return 0x40000001 for connect and 8 for disconnect
>
> For eDP, we want to disable the connect and disconnect interrupts. So, the mask will be 0x6 (i.e. DP_DP_IRQ_HPD_INT_MASK | DP_DP_HPD_REPLUG_INT_MASK)
> We want to return 0x40000000 (or 0x20000000 based on hpd line status) and 0 for eDP connect and disconnect respectively.
>
> > >
> > > isr & DP_DP_HPD_STATE_STATUS_MASK & mask, will return 0 always.
> > >
> > >>>   }
> > >>>
> > >>>   int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog)
> > >>> --
> > >>> 2.7.4
> > >>>
> > >>
> > >>
> > >> --
> > >> With best wishes
> > >> Dmitry
> > >
> > > Thank you,
> > > Sankeerth
> >
> >
> > --
> > With best wishes
> > Dmitry
>
> Thank you,
> Sankeerth



-- 
With best wishes
Dmitry

RE: [PATCH v6 4/8] drm/msm/dp: avoid handling masked interrupts

[Sankeerth Billakanti (QUIC)]

> On Thu, 31 Mar 2022 at 14:05, Sankeerth Billakanti
> <sbillaka@qti.qualcomm.com> wrote:
> >
> > Hi Dmitry,
> >
> > > On 31/03/2022 08:53, Sankeerth Billakanti (QUIC) wrote:
> > > > Hi Dmitry,
> > > >
> > > >> On Wed, 30 Mar 2022 at 19:03, Sankeerth Billakanti
> > > >> <quic_sbillaka@quicinc.com> wrote:
> > > >>>
> > > >>> The interrupt register will still reflect the connect and
> > > >>> disconnect interrupt status without generating an actual HW
> interrupt.
> > > >>> The controller driver should not handle those masked interrupts.
> > > >>>
> > > >>> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
> > > >>> ---
> > > >>>   drivers/gpu/drm/msm/dp/dp_catalog.c | 5 +++--
> > > >>>   1 file changed, 3 insertions(+), 2 deletions(-)
> > > >>>
> > > >>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
> > > >>> b/drivers/gpu/drm/msm/dp/dp_catalog.c
> > > >>> index 3c16f95..1809ce2 100644
> > > >>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> > > >>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> > > >>> @@ -608,13 +608,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 & (DP_DP_HPD_STATE_STATUS_MASK | mask);
> > > >>
> > > >> I suspect that the logic is inverted here. Shouldn't it be:
> > > >>
> > > >> return isr & DP_DP_HPD_STATE_STATUS_MASK & mask;
> > > >>
> > > >> ?
> > > >>
> > > >
> > > > The value of DP_DP_HPD_STATE_STATUS_MASK is 0xE0000000 and the
> > > value
> > > > of the read interrupt mask variable could be is 0xF.
> > > >
> > > > The mask value is indicated via the register,
> > > > REG_DP_DP_HPD_INT_MASK,
> > > bits 3:0.
> > > > The HPD status is indicated via a different read-only register
> > > REG_DP_DP_HPD_INT_STATUS, bits 31:29.
> > >
> > > I see. Maybe the following expression would be better?
> > >
> > > return isr & (mask & ~DP_DP_HPD_INT_MASK);
> 
> Ugh, excuse me please. I meant:
> 
> return isr & (mask | ~DP_DP_HPD_INT_MASK);
> 

Okay. I will change it.

> > >
> >
> > I believe the confusion occurred because the
> > DP_DP_HPD_STATE_STATUS_CONNECTED and others were defined under
> the same register definition as REG_DP_DP_HPD_INT_MASK I will rearrange
> the definitions below.
> >
> > #define REG_DP_DP_HPD_INT_MASK          (0x0000000C)
> > #define DP_DP_HPD_PLUG_INT_MASK         (0x00000001)
> > #define DP_DP_IRQ_HPD_INT_MASK          (0x00000002)
> > #define DP_DP_HPD_REPLUG_INT_MASK       (0x00000004)
> > #define DP_DP_HPD_UNPLUG_INT_MASK       (0x00000008)
> > #define DP_DP_HPD_INT_MASK              (DP_DP_HPD_PLUG_INT_MASK | \
> >                                                 DP_DP_IRQ_HPD_INT_MASK | \
> >                                                 DP_DP_HPD_REPLUG_INT_MASK | \
> >
> > DP_DP_HPD_UNPLUG_INT_MASK)
> >
> > Below are status bits from register REG_DP_DP_HPD_INT_STATUS
> >
> > #define DP_DP_HPD_STATE_STATUS_CONNECTED        (0x40000000)
> > #define DP_DP_HPD_STATE_STATUS_PENDING          (0x20000000)
> > #define DP_DP_HPD_STATE_STATUS_DISCONNECTED     (0x00000000)
> > #define DP_DP_HPD_STATE_STATUS_MASK             (0xE0000000)
> >
> > DP_DP_HPD_INT_MASK is 0xF and scope of mask variable is also 0xF (bits
> 3:0), mask & ~DP_DP_HPD_INT_MASK is 0 always.
> >
> > For DP, we want to enable all interrupts.
> > So the programmed mask value is 0xF. We want to return 0x40000001 for
> > connect and 8 for disconnect
> >
> > For eDP, we want to disable the connect and disconnect interrupts. So,
> > the mask will be 0x6 (i.e. DP_DP_IRQ_HPD_INT_MASK |
> DP_DP_HPD_REPLUG_INT_MASK) We want to return 0x40000000 (or
> 0x20000000 based on hpd line status) and 0 for eDP connect and disconnect
> respectively.
> >
> > > >
> > > > isr & DP_DP_HPD_STATE_STATUS_MASK & mask, will return 0 always.
> > > >
> > > >>>   }
> > > >>>
> > > >>>   int dp_catalog_ctrl_get_interrupt(struct dp_catalog
> > > >>> *dp_catalog)
> > > >>> --
> > > >>> 2.7.4
> > > >>>
> > > >>
> > > >>
> > > >> --
> > > >> With best wishes
> > > >> Dmitry
> > > >
> > > > Thank you,
> > > > Sankeerth
> > >
> > >
> > > --
> > > With best wishes
> > > Dmitry
> >
> > Thank you,
> > Sankeerth
> 
> 
> 
> --
> With best wishes
> Dmitry

[PATCH v6 5/8] drm/msm/dp: prevent multiple votes for dp resources

[Sankeerth Billakanti]

The aux_bus support with the dp_display driver will enable the dp
resources during msm_dp_modeset_init. The host_init has to return early
if the core is already initialized to prevent putting an additional vote
for the dp controller resources.

Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 888ff03..798b30b 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -420,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);
@@ -432,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);
-- 
2.7.4

Re: [PATCH v6 5/8] drm/msm/dp: prevent multiple votes for dp resources

[Doug Anderson]

Hi,

On Wed, Mar 30, 2022 at 9:04 AM Sankeerth Billakanti
<quic_sbillaka@quicinc.com> wrote:
>
> The aux_bus support with the dp_display driver will enable the dp
> resources during msm_dp_modeset_init. The host_init has to return early
> if the core is already initialized to prevent putting an additional vote
> for the dp controller resources.
>
> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
> ---
>  drivers/gpu/drm/msm/dp/dp_display.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)

I'm not a huge fan of this but I'll leave it up to Dmitry. In general
it feels like there should be _a_ place that enables these resources.
Checks like this make it feel like we just scattershot enabling
resources in a bunch of random places instead of coming up with the
design for enabling them in the right place.

In any case, if we do end up landing this patch, it sure feels like it
needs to move earlier in the patch series, right? This patch shouldn't
hurt even without the other patches in the series but if you apply the
earlier patches in the series without this one then you'll have a bug,
right? That means this needs to come earlier.

-Doug

Re: [PATCH v6 5/8] drm/msm/dp: prevent multiple votes for dp resources

[Dmitry Baryshkov]

On 01/04/2022 02:23, Doug Anderson wrote:
> Hi,
> 
> On Wed, Mar 30, 2022 at 9:04 AM Sankeerth Billakanti
> <quic_sbillaka@quicinc.com> wrote:
>>
>> The aux_bus support with the dp_display driver will enable the dp
>> resources during msm_dp_modeset_init. The host_init has to return early
>> if the core is already initialized to prevent putting an additional vote
>> for the dp controller resources.
>>
>> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/dp/dp_display.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
> 
> I'm not a huge fan of this but I'll leave it up to Dmitry. In general
> it feels like there should be _a_ place that enables these resources.
> Checks like this make it feel like we just scattershot enabling
> resources in a bunch of random places instead of coming up with the
> design for enabling them in the right place.

I'd prefer to see a check for eDP in dp_display_config_hpd(). Or even 
better to see that this function isn't called for eDP at all.

> 
> In any case, if we do end up landing this patch, it sure feels like it
> needs to move earlier in the patch series, right? This patch shouldn't
> hurt even without the other patches in the series but if you apply the
> earlier patches in the series without this one then you'll have a bug,
> right? That means this needs to come earlier.
> 
> -Doug


-- 
With best wishes
Dmitry

RE: [PATCH v6 5/8] drm/msm/dp: prevent multiple votes for dp resources

[Sankeerth Billakanti]

> > On Wed, Mar 30, 2022 at 9:04 AM Sankeerth Billakanti
> > <quic_sbillaka@quicinc.com> wrote:
> >>
> >> The aux_bus support with the dp_display driver will enable the dp
> >> resources during msm_dp_modeset_init. The host_init has to return
> >> early if the core is already initialized to prevent putting an
> >> additional vote for the dp controller resources.
> >>
> >> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
> >> ---
> >>   drivers/gpu/drm/msm/dp/dp_display.c | 10 ++++++++++
> >>   1 file changed, 10 insertions(+)
> >
> > I'm not a huge fan of this but I'll leave it up to Dmitry. In general
> > it feels like there should be _a_ place that enables these resources.
> > Checks like this make it feel like we just scattershot enabling
> > resources in a bunch of random places instead of coming up with the
> > design for enabling them in the right place.
> 
> I'd prefer to see a check for eDP in dp_display_config_hpd(). Or even better
> to see that this function isn't called for eDP at all.
>

This needs to be called when eDP is not using the aux_bus path. If the eDP panel is
given as a separate panel driver, then the resources need to be enabled here.

If we don't want to support eDP without aux_bus, then we can skip this function.
 
> >
> > In any case, if we do end up landing this patch, it sure feels like it
> > needs to move earlier in the patch series, right? This patch shouldn't
> > hurt even without the other patches in the series but if you apply the
> > earlier patches in the series without this one then you'll have a bug,
> > right? That means this needs to come earlier.
> >
> > -Doug
> 
> 
> --
> With best wishes
> Dmitry

Thank you,
Sankeerth

Re: [PATCH v6 5/8] drm/msm/dp: prevent multiple votes for dp resources

[Dmitry Baryshkov]

On Fri, 8 Apr 2022 at 20:12, Sankeerth Billakanti
<sbillaka@qti.qualcomm.com> wrote:
>
> > > On Wed, Mar 30, 2022 at 9:04 AM Sankeerth Billakanti
> > > <quic_sbillaka@quicinc.com> wrote:
> > >>
> > >> The aux_bus support with the dp_display driver will enable the dp
> > >> resources during msm_dp_modeset_init. The host_init has to return
> > >> early if the core is already initialized to prevent putting an
> > >> additional vote for the dp controller resources.
> > >>
> > >> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
> > >> ---
> > >>   drivers/gpu/drm/msm/dp/dp_display.c | 10 ++++++++++
> > >>   1 file changed, 10 insertions(+)
> > >
> > > I'm not a huge fan of this but I'll leave it up to Dmitry. In general
> > > it feels like there should be _a_ place that enables these resources.
> > > Checks like this make it feel like we just scattershot enabling
> > > resources in a bunch of random places instead of coming up with the
> > > design for enabling them in the right place.
> >
> > I'd prefer to see a check for eDP in dp_display_config_hpd(). Or even better
> > to see that this function isn't called for eDP at all.
> >
>
> This needs to be called when eDP is not using the aux_bus path. If the eDP panel is
> given as a separate panel driver, then the resources need to be enabled here.
>
> If we don't want to support eDP without aux_bus, then we can skip this function.

I think it's up to you to decide, if it's necessary or not.
But if it is, please change it accordingly.

> > >
> > > In any case, if we do end up landing this patch, it sure feels like it
> > > needs to move earlier in the patch series, right? This patch shouldn't
> > > hurt even without the other patches in the series but if you apply the
> > > earlier patches in the series without this one then you'll have a bug,
> > > right? That means this needs to come earlier.
> > >
> > > -Doug
> >
> >
> > --
> > With best wishes
> > Dmitry
>
> Thank you,
> Sankeerth



-- 
With best wishes
Dmitry

[PATCH v6 6/8] drm/msm/dp: remove unnecessary delay during boot

[Sankeerth Billakanti]

Remove the unnecessary delay in executing the EV_HPD_INIT_SETUP event.

Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 798b30b..8bafdd0 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1526,7 +1526,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)
-- 
2.7.4

Re: [PATCH v6 6/8] drm/msm/dp: remove unnecessary delay during boot

[Doug Anderson]

Hi,

On Wed, Mar 30, 2022 at 9:04 AM Sankeerth Billakanti
<quic_sbillaka@quicinc.com> wrote:
>
> Remove the unnecessary delay in executing the EV_HPD_INIT_SETUP event.

Tell me more and put it in the commit message! Why did it used to be
necessary and why is it no longer necessary? Inquiring minds want to
know.

-Doug

RE: [PATCH v6 6/8] drm/msm/dp: remove unnecessary delay during boot

[Sankeerth Billakanti (QUIC)]

Hi Doug,

> On Wed, Mar 30, 2022 at 9:04 AM Sankeerth Billakanti
> <quic_sbillaka@quicinc.com> wrote:
> >
> > Remove the unnecessary delay in executing the EV_HPD_INIT_SETUP
> event.
> 
> Tell me more and put it in the commit message! Why did it used to be
> necessary and why is it no longer necessary? Inquiring minds want to know.
>

Okay. I will add proper description. The DP phy is shared with usb and executing the
dp phy_init before the usb phy_init was causing usb devices to not function.

Earlier, enabling phy_init was done when the EV_HPD_INIT_SETUP event was executed.
So, we had scheduled it to execute after 10 second to let the usb driver complete the phy_init first.

Kuogee made the below change to move the DP phy_init to execute after the DP is connected
https://patchwork.kernel.org/project/linux-arm-msm/patch/1642531648-8448-2-git-send-email-quic_khsieh@quicinc.com/

So, there is no need for the DP driver to wait 10 seconds for the phy initialization anymore.

eDP PHY is not shared with usb. So, it can be programmed anytime, hence not needing any delay.

> -Doug

Thank you,
Sankeerth

Re: [PATCH v6 6/8] drm/msm/dp: remove unnecessary delay during boot

[Dmitry Baryshkov]

On Mon, 4 Apr 2022 at 16:53, Sankeerth Billakanti (QUIC)
<quic_sbillaka@quicinc.com> wrote:
>
> Hi Doug,
>
> > On Wed, Mar 30, 2022 at 9:04 AM Sankeerth Billakanti
> > <quic_sbillaka@quicinc.com> wrote:
> > >
> > > Remove the unnecessary delay in executing the EV_HPD_INIT_SETUP
> > event.
> >
> > Tell me more and put it in the commit message! Why did it used to be
> > necessary and why is it no longer necessary? Inquiring minds want to know.
> >
>
> Okay. I will add proper description. The DP phy is shared with usb and executing the
> dp phy_init before the usb phy_init was causing usb devices to not function.

I always wondered, how does this work for the 4-lane DP dongles, where
there is no USB mode/lanes?

> Earlier, enabling phy_init was done when the EV_HPD_INIT_SETUP event was executed.
> So, we had scheduled it to execute after 10 second to let the usb driver complete the phy_init first.
>
> Kuogee made the below change to move the DP phy_init to execute after the DP is connected
> https://patchwork.kernel.org/project/linux-arm-msm/patch/1642531648-8448-2-git-send-email-quic_khsieh@quicinc.com/
>
> So, there is no need for the DP driver to wait 10 seconds for the phy initialization anymore.
>
> eDP PHY is not shared with usb. So, it can be programmed anytime, hence not needing any delay.



-- 
With best wishes
Dmitry

RE: [PATCH v6 6/8] drm/msm/dp: remove unnecessary delay during boot

[Sankeerth Billakanti (QUIC)]

Hi Dmitry,

> > > On Wed, Mar 30, 2022 at 9:04 AM Sankeerth Billakanti
> > > <quic_sbillaka@quicinc.com> wrote:
> > > >
> > > > Remove the unnecessary delay in executing the EV_HPD_INIT_SETUP
> > > event.
> > >
> > > Tell me more and put it in the commit message! Why did it used to be
> > > necessary and why is it no longer necessary? Inquiring minds want to
> know.
> > >
> >
> > Okay. I will add proper description. The DP phy is shared with usb and
> > executing the dp phy_init before the usb phy_init was causing usb devices
> to not function.
> 
> I always wondered, how does this work for the 4-lane DP dongles, where
> there is no USB mode/lanes?
> 

For 4 lane DP, the DP driver overrides the phy programming. For 2 lanes
DP, the usb phy programming is leveraged.

I believe the usb controller needs to enable both usb3 (qmp phy) and usb2
(hs phy) before DP phy_init. If one of the usb phy init fails, the usb controller
will not enumerate and will cause usb2 devices (like keyboard or mouse) to
not work. The usb controller probe was failing because DP was turning on
(voting) a clock which was supposed to be initialized by the usb driver.

We did not see any issue with DP because the programming anyway gets
overwritten but due to usb controller failure, usb devices did not work.

> > Earlier, enabling phy_init was done when the EV_HPD_INIT_SETUP event
> was executed.
> > So, we had scheduled it to execute after 10 second to let the usb driver
> complete the phy_init first.
> >
> > Kuogee made the below change to move the DP phy_init to execute after
> > the DP is connected
> > https://patchwork.kernel.org/project/linux-arm-msm/patch/1642531648-
> 84
> > 48-2-git-send-email-quic_khsieh@quicinc.com/
> >
> > So, there is no need for the DP driver to wait 10 seconds for the phy
> initialization anymore.
> >
> > eDP PHY is not shared with usb. So, it can be programmed anytime, hence
> not needing any delay.
> 
> 
> 
> --
> With best wishes
> Dmitry

[PATCH v6 7/8] drm/msm/dp: Support edp/dp without hpd

[Sankeerth Billakanti]

Some eDP sinks or platform boards will not support hpd.
This patch adds support for those cases.

Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
---
 drivers/gpu/drm/msm/dp/dp_catalog.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
index 1809ce2..8f1fc71 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -244,10 +244,17 @@ void dp_catalog_aux_update_cfg(struct dp_catalog *dp_catalog)
 
 int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog)
 {
-	u32 state;
+	u32 state, hpd_en;
 	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);
+	hpd_en &= DP_DP_HPD_CTRL_HPD_EN;
+
+	/* no-hpd case */
+	if (!hpd_en)
+		return 0;
+
 	/* poll for hpd connected status every 2ms and timeout after 500ms */
 	return readl_poll_timeout(catalog->io->dp_controller.aux.base +
 				REG_DP_DP_HPD_INT_STATUS,
@@ -586,8 +593,10 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog *dp_catalog)
 	reftimer |= DP_DP_HPD_REFTIMER_ENABLE;
 	dp_write_aux(catalog, REG_DP_DP_HPD_REFTIMER, reftimer);
 
-	/* Enable HPD */
-	dp_write_aux(catalog, REG_DP_DP_HPD_CTRL, DP_DP_HPD_CTRL_HPD_EN);
+	/* Enable HPD if supported*/
+	if (!of_property_read_bool(catalog->dev->of_node, "no-hpd"))
+		dp_write_aux(catalog, REG_DP_DP_HPD_CTRL,
+				DP_DP_HPD_CTRL_HPD_EN);
 }
 
 u32 dp_catalog_link_is_connected(struct dp_catalog *dp_catalog)
-- 
2.7.4

Re: [PATCH v6 7/8] drm/msm/dp: Support edp/dp without hpd

[Doug Anderson]

Hi,

On Wed, Mar 30, 2022 at 9:04 AM Sankeerth Billakanti
<quic_sbillaka@quicinc.com> wrote:
>
> Some eDP sinks or platform boards will not support hpd.
> This patch adds support for those cases.

You could say more, like:

If we're not using HPD then _both_ the panel node and the eDP
controller node will have "no-hpd". This tells the eDP panel code to
hardcode the maximum possible delay for a panel to power up and tells
the eDP driver that it should continue to do transfers even if HPD
isn't asserted.


> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
> ---
>  drivers/gpu/drm/msm/dp/dp_catalog.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
> index 1809ce2..8f1fc71 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> @@ -244,10 +244,17 @@ void dp_catalog_aux_update_cfg(struct dp_catalog *dp_catalog)
>
>  int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog)
>  {
> -       u32 state;
> +       u32 state, hpd_en;
>         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);
> +       hpd_en &= DP_DP_HPD_CTRL_HPD_EN;
> +
> +       /* no-hpd case */
> +       if (!hpd_en)
> +               return 0;
> +
>         /* poll for hpd connected status every 2ms and timeout after 500ms */
>         return readl_poll_timeout(catalog->io->dp_controller.aux.base +
>                                 REG_DP_DP_HPD_INT_STATUS,
> @@ -586,8 +593,10 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog *dp_catalog)
>         reftimer |= DP_DP_HPD_REFTIMER_ENABLE;
>         dp_write_aux(catalog, REG_DP_DP_HPD_REFTIMER, reftimer);
>
> -       /* Enable HPD */
> -       dp_write_aux(catalog, REG_DP_DP_HPD_CTRL, DP_DP_HPD_CTRL_HPD_EN);
> +       /* Enable HPD if supported*/
> +       if (!of_property_read_bool(catalog->dev->of_node, "no-hpd"))

I don't think this is a particularly lightweight operation. It's
literally iterating through all of our device tree properties and
doing string compares on them. ...but this function is called somewhat
often, isn't it? It feels like the kind of thing that should happen at
probe time and be stored in a boolean.

...and then you can use that same boolean in
dp_catalog_aux_wait_for_hpd_connect_state() rather than reading the
register value, right?


-Doug

RE: [PATCH v6 7/8] drm/msm/dp: Support edp/dp without hpd

[Sankeerth Billakanti (QUIC)]

Hi Doug,

> On Wed, Mar 30, 2022 at 9:04 AM Sankeerth Billakanti
> <quic_sbillaka@quicinc.com> wrote:
> >
> > Some eDP sinks or platform boards will not support hpd.
> > This patch adds support for those cases.
> 
> You could say more, like:
> 
> If we're not using HPD then _both_ the panel node and the eDP controller
> node will have "no-hpd". This tells the eDP panel code to hardcode the
> maximum possible delay for a panel to power up and tells the eDP driver that
> it should continue to do transfers even if HPD isn't asserted.
> 

Okay. I will add it

> 
> > Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
> > ---
> >  drivers/gpu/drm/msm/dp/dp_catalog.c | 15 ++++++++++++---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
> > b/drivers/gpu/drm/msm/dp/dp_catalog.c
> > index 1809ce2..8f1fc71 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> > @@ -244,10 +244,17 @@ void dp_catalog_aux_update_cfg(struct
> dp_catalog
> > *dp_catalog)
> >
> >  int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog
> > *dp_catalog)  {
> > -       u32 state;
> > +       u32 state, hpd_en;
> >         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);
> > +       hpd_en &= DP_DP_HPD_CTRL_HPD_EN;
> > +
> > +       /* no-hpd case */
> > +       if (!hpd_en)
> > +               return 0;
> > +
> >         /* poll for hpd connected status every 2ms and timeout after 500ms */
> >         return readl_poll_timeout(catalog->io->dp_controller.aux.base +
> >                                 REG_DP_DP_HPD_INT_STATUS, @@ -586,8
> > +593,10 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog
> *dp_catalog)
> >         reftimer |= DP_DP_HPD_REFTIMER_ENABLE;
> >         dp_write_aux(catalog, REG_DP_DP_HPD_REFTIMER, reftimer);
> >
> > -       /* Enable HPD */
> > -       dp_write_aux(catalog, REG_DP_DP_HPD_CTRL,
> DP_DP_HPD_CTRL_HPD_EN);
> > +       /* Enable HPD if supported*/
> > +       if (!of_property_read_bool(catalog->dev->of_node, "no-hpd"))
> 
> I don't think this is a particularly lightweight operation. It's literally iterating
> through all of our device tree properties and doing string compares on them.
> ...but this function is called somewhat often, isn't it? It feels like the kind of
> thing that should happen at probe time and be stored in a boolean.
> 
> ...and then you can use that same boolean in
> dp_catalog_aux_wait_for_hpd_connect_state() rather than reading the
> register value, right?
> 
It is called twice for DP. Once while booting through a thread scheduled from kms_obj_init
and when resuming from PM suspend.

With aux_bus addition, this function will be called thrice for eDP. Once during bootup with
aux_bus, then from scheduled event from kms_obj_init and pm resume like DP.

I will check if we can use a no-hpd Boolean flag instead.

> 
> -Doug

Thank you,
Sankeerth

Re: [PATCH v6 7/8] drm/msm/dp: Support edp/dp without hpd

[Dmitry Baryshkov]

On Mon, 4 Apr 2022 at 21:32, Sankeerth Billakanti (QUIC)
<quic_sbillaka@quicinc.com> wrote:
>
> Hi Doug,
>
> > On Wed, Mar 30, 2022 at 9:04 AM Sankeerth Billakanti
> > <quic_sbillaka@quicinc.com> wrote:
> > >
> > > Some eDP sinks or platform boards will not support hpd.
> > > This patch adds support for those cases.
> >
> > You could say more, like:
> >
> > If we're not using HPD then _both_ the panel node and the eDP controller
> > node will have "no-hpd". This tells the eDP panel code to hardcode the
> > maximum possible delay for a panel to power up and tells the eDP driver that
> > it should continue to do transfers even if HPD isn't asserted.
> >
>
> Okay. I will add it
>
> >
> > > Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
> > > ---
> > >  drivers/gpu/drm/msm/dp/dp_catalog.c | 15 ++++++++++++---
> > >  1 file changed, 12 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
> > > b/drivers/gpu/drm/msm/dp/dp_catalog.c
> > > index 1809ce2..8f1fc71 100644
> > > --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> > > +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> > > @@ -244,10 +244,17 @@ void dp_catalog_aux_update_cfg(struct
> > dp_catalog
> > > *dp_catalog)
> > >
> > >  int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog
> > > *dp_catalog)  {
> > > -       u32 state;
> > > +       u32 state, hpd_en;
> > >         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);
> > > +       hpd_en &= DP_DP_HPD_CTRL_HPD_EN;
> > > +
> > > +       /* no-hpd case */
> > > +       if (!hpd_en)
> > > +               return 0;
> > > +
> > >         /* poll for hpd connected status every 2ms and timeout after 500ms */
> > >         return readl_poll_timeout(catalog->io->dp_controller.aux.base +
> > >                                 REG_DP_DP_HPD_INT_STATUS, @@ -586,8
> > > +593,10 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog
> > *dp_catalog)
> > >         reftimer |= DP_DP_HPD_REFTIMER_ENABLE;
> > >         dp_write_aux(catalog, REG_DP_DP_HPD_REFTIMER, reftimer);
> > >
> > > -       /* Enable HPD */
> > > -       dp_write_aux(catalog, REG_DP_DP_HPD_CTRL,
> > DP_DP_HPD_CTRL_HPD_EN);
> > > +       /* Enable HPD if supported*/
> > > +       if (!of_property_read_bool(catalog->dev->of_node, "no-hpd"))
> >
> > I don't think this is a particularly lightweight operation. It's literally iterating
> > through all of our device tree properties and doing string compares on them.
> > ...but this function is called somewhat often, isn't it? It feels like the kind of
> > thing that should happen at probe time and be stored in a boolean.
> >
> > ...and then you can use that same boolean in
> > dp_catalog_aux_wait_for_hpd_connect_state() rather than reading the
> > register value, right?
> >
> It is called twice for DP. Once while booting through a thread scheduled from kms_obj_init
> and when resuming from PM suspend.
>
> With aux_bus addition, this function will be called thrice for eDP. Once during bootup with
> aux_bus, then from scheduled event from kms_obj_init and pm resume like DP.
>
> I will check if we can use a no-hpd Boolean flag instead.

As the driver has a separate dp_parser code, it might be a good fit to
parse the DT once and then to use boolean flag afterwards.

-- 
With best wishes
Dmitry

RE: [PATCH v6 7/8] drm/msm/dp: Support edp/dp without hpd

[Sankeerth Billakanti (QUIC)]

> > > On Wed, Mar 30, 2022 at 9:04 AM Sankeerth Billakanti
> > > <quic_sbillaka@quicinc.com> wrote:
> > > >
> > > > Some eDP sinks or platform boards will not support hpd.
> > > > This patch adds support for those cases.
> > >
> > > You could say more, like:
> > >
> > > If we're not using HPD then _both_ the panel node and the eDP
> > > controller node will have "no-hpd". This tells the eDP panel code to
> > > hardcode the maximum possible delay for a panel to power up and
> > > tells the eDP driver that it should continue to do transfers even if HPD
> isn't asserted.
> > >
> >
> > Okay. I will add it
> >
> > >
> > > > Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
> > > > ---
> > > >  drivers/gpu/drm/msm/dp/dp_catalog.c | 15 ++++++++++++---
> > > >  1 file changed, 12 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
> > > > b/drivers/gpu/drm/msm/dp/dp_catalog.c
> > > > index 1809ce2..8f1fc71 100644
> > > > --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> > > > +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> > > > @@ -244,10 +244,17 @@ void dp_catalog_aux_update_cfg(struct
> > > dp_catalog
> > > > *dp_catalog)
> > > >
> > > >  int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog
> > > > *dp_catalog)  {
> > > > -       u32 state;
> > > > +       u32 state, hpd_en;
> > > >         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);
> > > > +       hpd_en &= DP_DP_HPD_CTRL_HPD_EN;
> > > > +
> > > > +       /* no-hpd case */
> > > > +       if (!hpd_en)
> > > > +               return 0;
> > > > +
> > > >         /* poll for hpd connected status every 2ms and timeout after
> 500ms */
> > > >         return readl_poll_timeout(catalog->io->dp_controller.aux.base +
> > > >                                 REG_DP_DP_HPD_INT_STATUS, @@
> > > > -586,8
> > > > +593,10 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog
> > > *dp_catalog)
> > > >         reftimer |= DP_DP_HPD_REFTIMER_ENABLE;
> > > >         dp_write_aux(catalog, REG_DP_DP_HPD_REFTIMER, reftimer);
> > > >
> > > > -       /* Enable HPD */
> > > > -       dp_write_aux(catalog, REG_DP_DP_HPD_CTRL,
> > > DP_DP_HPD_CTRL_HPD_EN);
> > > > +       /* Enable HPD if supported*/
> > > > +       if (!of_property_read_bool(catalog->dev->of_node,
> > > > + "no-hpd"))
> > >
> > > I don't think this is a particularly lightweight operation. It's
> > > literally iterating through all of our device tree properties and doing string
> compares on them.
> > > ...but this function is called somewhat often, isn't it? It feels
> > > like the kind of thing that should happen at probe time and be stored in a
> boolean.
> > >
> > > ...and then you can use that same boolean in
> > > dp_catalog_aux_wait_for_hpd_connect_state() rather than reading the
> > > register value, right?
> > >
> > It is called twice for DP. Once while booting through a thread
> > scheduled from kms_obj_init and when resuming from PM suspend.
> >
> > With aux_bus addition, this function will be called thrice for eDP.
> > Once during bootup with aux_bus, then from scheduled event from
> kms_obj_init and pm resume like DP.
> >
> > I will check if we can use a no-hpd Boolean flag instead.
> 
> As the driver has a separate dp_parser code, it might be a good fit to parse
> the DT once and then to use boolean flag afterwards.
> 

Okay. Will add it.

> --
> With best wishes
> Dmitry

[PATCH v6 8/8] drm/msm/dp: Handle eDP mode_valid differently from dp

[Sankeerth Billakanti]

The panel-edp driver modes needs to be validated differently from DP
because the link capabilities are not available for EDP by that time.

Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 8bafdd0..f9c7d9a 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1003,6 +1003,12 @@ enum drm_mode_status dp_bridge_mode_valid(struct drm_bridge *bridge,
 		return -EINVAL;
 	}
 
+	if (dp->connector_type == DRM_MODE_CONNECTOR_eDP) {
+		if (mode_pclk_khz > DP_MAX_PIXEL_CLK_KHZ)
+			return MODE_CLOCK_HIGH;
+		return MODE_OK;
+	}
+
 	if ((dp->max_pclk_khz <= 0) ||
 			(dp->max_pclk_khz > DP_MAX_PIXEL_CLK_KHZ) ||
 			(mode->clock > dp->max_pclk_khz))
-- 
2.7.4

Re: [PATCH v6 8/8] drm/msm/dp: Handle eDP mode_valid differently from dp

[Dmitry Baryshkov]

On Wed, 30 Mar 2022 at 19:04, Sankeerth Billakanti
<quic_sbillaka@quicinc.com> wrote:
>
> The panel-edp driver modes needs to be validated differently from DP
> because the link capabilities are not available for EDP by that time.
>
> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>

This should not be necessary after
https://patchwork.freedesktop.org/patch/479261/?series=101682&rev=1.
Could you please check?

> ---
>  drivers/gpu/drm/msm/dp/dp_display.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 8bafdd0..f9c7d9a 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1003,6 +1003,12 @@ enum drm_mode_status dp_bridge_mode_valid(struct drm_bridge *bridge,
>                 return -EINVAL;
>         }
>
> +       if (dp->connector_type == DRM_MODE_CONNECTOR_eDP) {
> +               if (mode_pclk_khz > DP_MAX_PIXEL_CLK_KHZ)
> +                       return MODE_CLOCK_HIGH;
> +               return MODE_OK;
> +       }
> +
>         if ((dp->max_pclk_khz <= 0) ||
>                         (dp->max_pclk_khz > DP_MAX_PIXEL_CLK_KHZ) ||
>                         (mode->clock > dp->max_pclk_khz))
> --
> 2.7.4
>


-- 
With best wishes
Dmitry

RE: [PATCH v6 8/8] drm/msm/dp: Handle eDP mode_valid differently from dp

[Sankeerth Billakanti (QUIC)]

Hi Dmitry,

> On Wed, 30 Mar 2022 at 19:04, Sankeerth Billakanti
> <quic_sbillaka@quicinc.com> wrote:
> >
> > The panel-edp driver modes needs to be validated differently from DP
> > because the link capabilities are not available for EDP by that time.
> >
> > Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
> 
> This should not be necessary after
> https://patchwork.freedesktop.org/patch/479261/?series=101682&rev=1.
> Could you please check?
> 

The check for DP_MAX_PIXEL_CLK_KHZ is not necessary anymore but we need
to return early for eDP because unlike DP, eDP context will not have the information
about the number of lanes and link clock.

So, I will modify the patch to return after the DP_MAX_PIXEL_CLK_KHZ check if is_eDP is set.

> > ---
> >  drivers/gpu/drm/msm/dp/dp_display.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> > b/drivers/gpu/drm/msm/dp/dp_display.c
> > index 8bafdd0..f9c7d9a 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > @@ -1003,6 +1003,12 @@ enum drm_mode_status
> dp_bridge_mode_valid(struct drm_bridge *bridge,
> >                 return -EINVAL;
> >         }
> >
> > +       if (dp->connector_type == DRM_MODE_CONNECTOR_eDP) {
> > +               if (mode_pclk_khz > DP_MAX_PIXEL_CLK_KHZ)
> > +                       return MODE_CLOCK_HIGH;
> > +               return MODE_OK;
> > +       }
> > +
> >         if ((dp->max_pclk_khz <= 0) ||
> >                         (dp->max_pclk_khz > DP_MAX_PIXEL_CLK_KHZ) ||
> >                         (mode->clock > dp->max_pclk_khz))
> > --
> > 2.7.4
> >
> 
> 
> --
> With best wishes
> Dmitry

Re: [PATCH v6 8/8] drm/msm/dp: Handle eDP mode_valid differently from dp

[Doug Anderson]

Hi,

On Wed, Mar 30, 2022 at 11:02 PM Sankeerth Billakanti (QUIC)
<quic_sbillaka@quicinc.com> wrote:
>
> Hi Dmitry,
>
> > On Wed, 30 Mar 2022 at 19:04, Sankeerth Billakanti
> > <quic_sbillaka@quicinc.com> wrote:
> > >
> > > The panel-edp driver modes needs to be validated differently from DP
> > > because the link capabilities are not available for EDP by that time.
> > >
> > > Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
> >
> > This should not be necessary after
> > https://patchwork.freedesktop.org/patch/479261/?series=101682&rev=1.
> > Could you please check?
> >
>
> The check for DP_MAX_PIXEL_CLK_KHZ is not necessary anymore but we need
> to return early for eDP because unlike DP, eDP context will not have the information
> about the number of lanes and link clock.
>
> So, I will modify the patch to return after the DP_MAX_PIXEL_CLK_KHZ check if is_eDP is set.

I haven't walked through all the relevant code but something you said
above sounds strange. You say that for eDP we don't have info about
the number of lanes? We _should_.

It's certainly possible to have a panel that supports _either_ 1 or 2
lanes but then only physically connect 1 lane to it. ...or you could
have a panel that supports 2 or 4 lanes and you only connect 1 lane.
See, for instance, ti_sn_bridge_parse_lanes. There we assume 4 lanes
but if a "data-lanes" property is present then we can use that to know
that fewer lanes are physically connected.

It's also possible to connect more lanes to a panel than it supports.
You could connect 2 lanes to it but then it only supports 1. This case
needs to be handled as well...


-Doug

RE: [PATCH v6 8/8] drm/msm/dp: Handle eDP mode_valid differently from dp

[Sankeerth Billakanti (QUIC)]

Hi Doug,

> On Wed, Mar 30, 2022 at 11:02 PM Sankeerth Billakanti (QUIC)
> <quic_sbillaka@quicinc.com> wrote:
> >
> > Hi Dmitry,
> >
> > > On Wed, 30 Mar 2022 at 19:04, Sankeerth Billakanti
> > > <quic_sbillaka@quicinc.com> wrote:
> > > >
> > > > The panel-edp driver modes needs to be validated differently from
> > > > DP because the link capabilities are not available for EDP by that time.
> > > >
> > > > Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
> > >
> > > This should not be necessary after
> > >
> https://patchwork.freedesktop.org/patch/479261/?series=101682&rev=1.
> > > Could you please check?
> > >
> >
> > The check for DP_MAX_PIXEL_CLK_KHZ is not necessary anymore but we
> > need to return early for eDP because unlike DP, eDP context will not
> > have the information about the number of lanes and link clock.
> >
> > So, I will modify the patch to return after the DP_MAX_PIXEL_CLK_KHZ
> check if is_eDP is set.
> 
> I haven't walked through all the relevant code but something you said above
> sounds strange. You say that for eDP we don't have info about the number
> of lanes? We _should_.
> 
> It's certainly possible to have a panel that supports _either_ 1 or 2 lanes but
> then only physically connect 1 lane to it. ...or you could have a panel that
> supports 2 or 4 lanes and you only connect 1 lane.
> See, for instance, ti_sn_bridge_parse_lanes. There we assume 4 lanes but if
> a "data-lanes" property is present then we can use that to know that fewer
> lanes are physically connected.
> 
> It's also possible to connect more lanes to a panel than it supports.
> You could connect 2 lanes to it but then it only supports 1. This case needs to
> be handled as well...
>

I was referring to the checks we do for DP in dp_bridge_mode_valid. We check if the
Link bandwidth can support the pixel bandwidth. For an external DP connection, the
Initial DPCD/EDID read after cable connection will return the sink capabilities like link
rate, lane count and bpp information that are used to we filter out the unsupported
modes from the list of modes from EDID.

For eDP case, the dp driver performs the first dpcd read during bridge_enable. The
dp_bridge_mode_valid function is executed before bridge_enable and hence does
not have the full link or the sink capabilities information like external DP connection,
by then.

So, we need to proceed with the reported mode for eDP.

> 
> -Doug

Re: [PATCH v6 8/8] drm/msm/dp: Handle eDP mode_valid differently from dp

[Dmitry Baryshkov]

On Mon, 4 Apr 2022 at 21:21, Sankeerth Billakanti (QUIC)
<quic_sbillaka@quicinc.com> wrote:
>
> Hi Doug,
>
> > On Wed, Mar 30, 2022 at 11:02 PM Sankeerth Billakanti (QUIC)
> > <quic_sbillaka@quicinc.com> wrote:
> > >
> > > Hi Dmitry,
> > >
> > > > On Wed, 30 Mar 2022 at 19:04, Sankeerth Billakanti
> > > > <quic_sbillaka@quicinc.com> wrote:
> > > > >
> > > > > The panel-edp driver modes needs to be validated differently from
> > > > > DP because the link capabilities are not available for EDP by that time.
> > > > >
> > > > > Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
> > > >
> > > > This should not be necessary after
> > > >
> > https://patchwork.freedesktop.org/patch/479261/?series=101682&rev=1.
> > > > Could you please check?
> > > >
> > >
> > > The check for DP_MAX_PIXEL_CLK_KHZ is not necessary anymore but we
> > > need to return early for eDP because unlike DP, eDP context will not
> > > have the information about the number of lanes and link clock.
> > >
> > > So, I will modify the patch to return after the DP_MAX_PIXEL_CLK_KHZ
> > check if is_eDP is set.
> >
> > I haven't walked through all the relevant code but something you said above
> > sounds strange. You say that for eDP we don't have info about the number
> > of lanes? We _should_.
> >
> > It's certainly possible to have a panel that supports _either_ 1 or 2 lanes but
> > then only physically connect 1 lane to it. ...or you could have a panel that
> > supports 2 or 4 lanes and you only connect 1 lane.
> > See, for instance, ti_sn_bridge_parse_lanes. There we assume 4 lanes but if
> > a "data-lanes" property is present then we can use that to know that fewer
> > lanes are physically connected.
> >
> > It's also possible to connect more lanes to a panel than it supports.
> > You could connect 2 lanes to it but then it only supports 1. This case needs to
> > be handled as well...
> >
>
> I was referring to the checks we do for DP in dp_bridge_mode_valid. We check if the
> Link bandwidth can support the pixel bandwidth. For an external DP connection, the
> Initial DPCD/EDID read after cable connection will return the sink capabilities like link
> rate, lane count and bpp information that are used to we filter out the unsupported
> modes from the list of modes from EDID.
>
> For eDP case, the dp driver performs the first dpcd read during bridge_enable. The
> dp_bridge_mode_valid function is executed before bridge_enable and hence does
> not have the full link or the sink capabilities information like external DP connection,
> by then.

It sounds to me like we should emulate the HPD event for eDP to be
handled earlier than the get_modes()/prepare() calls are attempted.
However this might open another can of worms.

> So, we need to proceed with the reported mode for eDP.

Well... Even if during the first call to get_modes() the DPCD is not
read, during subsequent calls the driver has necessary information, so
it can proceed with all the checks, can't it?

-- 
With best wishes
Dmitry

RE: [PATCH v6 8/8] drm/msm/dp: Handle eDP mode_valid differently from dp

[Sankeerth Billakanti (QUIC)]

Hi Dmitry,

> > > > > On Wed, 30 Mar 2022 at 19:04, Sankeerth Billakanti
> > > > > <quic_sbillaka@quicinc.com> wrote:
> > > > > >
> > > > > > The panel-edp driver modes needs to be validated differently
> > > > > > from DP because the link capabilities are not available for EDP by
> that time.
> > > > > >
> > > > > > Signed-off-by: Sankeerth Billakanti
> > > > > > <quic_sbillaka@quicinc.com>
> > > > >
> > > > > This should not be necessary after
> > > > >
> > >
> https://patchwork.freedesktop.org/patch/479261/?series=101682&rev=1.
> > > > > Could you please check?
> > > > >
> > > >
> > > > The check for DP_MAX_PIXEL_CLK_KHZ is not necessary anymore but
> we
> > > > need to return early for eDP because unlike DP, eDP context will
> > > > not have the information about the number of lanes and link clock.
> > > >
> > > > So, I will modify the patch to return after the
> > > > DP_MAX_PIXEL_CLK_KHZ
> > > check if is_eDP is set.
> > >
> > > I haven't walked through all the relevant code but something you
> > > said above sounds strange. You say that for eDP we don't have info
> > > about the number of lanes? We _should_.
> > >
> > > It's certainly possible to have a panel that supports _either_ 1 or
> > > 2 lanes but then only physically connect 1 lane to it. ...or you
> > > could have a panel that supports 2 or 4 lanes and you only connect 1 lane.
> > > See, for instance, ti_sn_bridge_parse_lanes. There we assume 4 lanes
> > > but if a "data-lanes" property is present then we can use that to
> > > know that fewer lanes are physically connected.
> > >
> > > It's also possible to connect more lanes to a panel than it supports.
> > > You could connect 2 lanes to it but then it only supports 1. This
> > > case needs to be handled as well...
> > >
> >
> > I was referring to the checks we do for DP in dp_bridge_mode_valid. We
> > check if the Link bandwidth can support the pixel bandwidth. For an
> > external DP connection, the Initial DPCD/EDID read after cable
> > connection will return the sink capabilities like link rate, lane
> > count and bpp information that are used to we filter out the unsupported
> modes from the list of modes from EDID.
> >
> > For eDP case, the dp driver performs the first dpcd read during
> > bridge_enable. The dp_bridge_mode_valid function is executed before
> > bridge_enable and hence does not have the full link or the sink
> > capabilities information like external DP connection, by then.
> 
> It sounds to me like we should emulate the HPD event for eDP to be handled
> earlier than the get_modes()/prepare() calls are attempted.
> However this might open another can of worms.
> 

For DP, the HPD handler mainly initiates link training and gets the EDID.

Before adding support for a separate eDP panel, we had discussed about
this internally and decided to emulate eDP HPD during enable(). Main reason
being, eDP power is guaranteed to be on only after bridge_enable().
So, eDP link training can happen and sustain only after bridge_enable().

Emulating HPD before/during get_modes will not have any effect because:

1. get_modes() will go to panel's get_modes() function to power on read EDID

2. panel power will be turned off after get_modes(). Panel power off will
reset every write transaction in DPCD. anyway invalidating link training

3. mode_valid will land in dp driver but panel will not be powered on at that
time and we cannot do aux transfers or DPCD read writes.

> > So, we need to proceed with the reported mode for eDP.
> 
> Well... Even if during the first call to get_modes() the DPCD is not read,
> during subsequent calls the driver has necessary information, so it can
> proceed with all the checks, can't it?
> 

get_modes() currently does not land in DP driver. It gets executed in panel
bridge. But the mode_valid() comes to DP driver to check the controller
compatibility.

> --
> With best wishes
> Dmitry

Thank you,
Sankeerth

Re: [PATCH v6 8/8] drm/msm/dp: Handle eDP mode_valid differently from dp

[Dmitry Baryshkov]

On Thu, 7 Apr 2022 at 17:05, Sankeerth Billakanti (QUIC)
<quic_sbillaka@quicinc.com> wrote:
>
> Hi Dmitry,
>
> > > > > > On Wed, 30 Mar 2022 at 19:04, Sankeerth Billakanti
> > > > > > <quic_sbillaka@quicinc.com> wrote:
> > > > > > >
> > > > > > > The panel-edp driver modes needs to be validated differently
> > > > > > > from DP because the link capabilities are not available for EDP by
> > that time.
> > > > > > >
> > > > > > > Signed-off-by: Sankeerth Billakanti
> > > > > > > <quic_sbillaka@quicinc.com>
> > > > > >
> > > > > > This should not be necessary after
> > > > > >
> > > >
> > https://patchwork.freedesktop.org/patch/479261/?series=101682&rev=1.
> > > > > > Could you please check?
> > > > > >
> > > > >
> > > > > The check for DP_MAX_PIXEL_CLK_KHZ is not necessary anymore but
> > we
> > > > > need to return early for eDP because unlike DP, eDP context will
> > > > > not have the information about the number of lanes and link clock.
> > > > >
> > > > > So, I will modify the patch to return after the
> > > > > DP_MAX_PIXEL_CLK_KHZ
> > > > check if is_eDP is set.
> > > >
> > > > I haven't walked through all the relevant code but something you
> > > > said above sounds strange. You say that for eDP we don't have info
> > > > about the number of lanes? We _should_.
> > > >
> > > > It's certainly possible to have a panel that supports _either_ 1 or
> > > > 2 lanes but then only physically connect 1 lane to it. ...or you
> > > > could have a panel that supports 2 or 4 lanes and you only connect 1 lane.
> > > > See, for instance, ti_sn_bridge_parse_lanes. There we assume 4 lanes
> > > > but if a "data-lanes" property is present then we can use that to
> > > > know that fewer lanes are physically connected.
> > > >
> > > > It's also possible to connect more lanes to a panel than it supports.
> > > > You could connect 2 lanes to it but then it only supports 1. This
> > > > case needs to be handled as well...
> > > >
> > >
> > > I was referring to the checks we do for DP in dp_bridge_mode_valid. We
> > > check if the Link bandwidth can support the pixel bandwidth. For an
> > > external DP connection, the Initial DPCD/EDID read after cable
> > > connection will return the sink capabilities like link rate, lane
> > > count and bpp information that are used to we filter out the unsupported
> > modes from the list of modes from EDID.
> > >
> > > For eDP case, the dp driver performs the first dpcd read during
> > > bridge_enable. The dp_bridge_mode_valid function is executed before
> > > bridge_enable and hence does not have the full link or the sink
> > > capabilities information like external DP connection, by then.
> >
> > It sounds to me like we should emulate the HPD event for eDP to be handled
> > earlier than the get_modes()/prepare() calls are attempted.
> > However this might open another can of worms.
> >
>
> For DP, the HPD handler mainly initiates link training and gets the EDID.
>
> Before adding support for a separate eDP panel, we had discussed about
> this internally and decided to emulate eDP HPD during enable(). Main reason
> being, eDP power is guaranteed to be on only after bridge_enable().
> So, eDP link training can happen and sustain only after bridge_enable().
>
> Emulating HPD before/during get_modes will not have any effect because:

As we have seen, the term HPD is significantly overloaded. What do you
want to emulate?

>
> 1. get_modes() will go to panel's get_modes() function to power on read EDID
>
> 2. panel power will be turned off after get_modes(). Panel power off will
> reset every write transaction in DPCD. anyway invalidating link training

I tend to agree with Doug here. eDP link power status should be
handled by the pm_runtime_autosuspend with grace period being high
enough to cover the timeslot between get_mode() and enable().

panel-edp already does most of required work.

>
> 3. mode_valid will land in dp driver but panel will not be powered on at that
> time and we cannot do aux transfers or DPCD read writes.

Why do we need to perform AUX writes in mode_valid?

>
> > > So, we need to proceed with the reported mode for eDP.
> >
> > Well... Even if during the first call to get_modes() the DPCD is not read,
> > during subsequent calls the driver has necessary information, so it can
> > proceed with all the checks, can't it?
> >
>
> get_modes() currently does not land in DP driver. It gets executed in panel
> bridge. But the mode_valid() comes to DP driver to check the controller
> compatibility.

Yes, this is correct. the DP's mode_valid() knows the hardware
limitations (max clock speed, amount of lanes, etc) and thus it can
decide whether the mode is supported by the whole chain or not.
We should not skip such checks for the eDP.


-- 
With best wishes
Dmitry

RE: [PATCH v6 8/8] drm/msm/dp: Handle eDP mode_valid differently from dp

[Sankeerth Billakanti]

Hi Dmitry,

> > > > > > > On Wed, 30 Mar 2022 at 19:04, Sankeerth Billakanti
> > > > > > > <quic_sbillaka@quicinc.com> wrote:
> > > > > > > >
> > > > > > > > The panel-edp driver modes needs to be validated
> > > > > > > > differently from DP because the link capabilities are not
> > > > > > > > available for EDP by
> > > that time.
> > > > > > > >
> > > > > > > > Signed-off-by: Sankeerth Billakanti
> > > > > > > > <quic_sbillaka@quicinc.com>
> > > > > > >
> > > > > > > This should not be necessary after
> > > > > > >
> > > > >
> > >
> https://patchwork.freedesktop.org/patch/479261/?series=101682&rev=1.
> > > > > > > Could you please check?
> > > > > > >
> > > > > >
> > > > > > The check for DP_MAX_PIXEL_CLK_KHZ is not necessary anymore
> > > > > > but
> > > we
> > > > > > need to return early for eDP because unlike DP, eDP context
> > > > > > will not have the information about the number of lanes and link
> clock.
> > > > > >
> > > > > > So, I will modify the patch to return after the
> > > > > > DP_MAX_PIXEL_CLK_KHZ
> > > > > check if is_eDP is set.
> > > > >
> > > > > I haven't walked through all the relevant code but something you
> > > > > said above sounds strange. You say that for eDP we don't have
> > > > > info about the number of lanes? We _should_.
> > > > >
> > > > > It's certainly possible to have a panel that supports _either_ 1
> > > > > or
> > > > > 2 lanes but then only physically connect 1 lane to it. ...or you
> > > > > could have a panel that supports 2 or 4 lanes and you only connect 1
> lane.
> > > > > See, for instance, ti_sn_bridge_parse_lanes. There we assume 4
> > > > > lanes but if a "data-lanes" property is present then we can use
> > > > > that to know that fewer lanes are physically connected.
> > > > >
> > > > > It's also possible to connect more lanes to a panel than it supports.
> > > > > You could connect 2 lanes to it but then it only supports 1.
> > > > > This case needs to be handled as well...
> > > > >
> > > >
> > > > I was referring to the checks we do for DP in
> > > > dp_bridge_mode_valid. We check if the Link bandwidth can support
> > > > the pixel bandwidth. For an external DP connection, the Initial
> > > > DPCD/EDID read after cable connection will return the sink
> > > > capabilities like link rate, lane count and bpp information that
> > > > are used to we filter out the unsupported
> > > modes from the list of modes from EDID.
> > > >
> > > > For eDP case, the dp driver performs the first dpcd read during
> > > > bridge_enable. The dp_bridge_mode_valid function is executed
> > > > before bridge_enable and hence does not have the full link or the
> > > > sink capabilities information like external DP connection, by then.
> > >
> > > It sounds to me like we should emulate the HPD event for eDP to be
> > > handled earlier than the get_modes()/prepare() calls are attempted.
> > > However this might open another can of worms.
> > >
> >
> > For DP, the HPD handler mainly initiates link training and gets the EDID.
> >
> > Before adding support for a separate eDP panel, we had discussed about
> > this internally and decided to emulate eDP HPD during enable(). Main
> > reason being, eDP power is guaranteed to be on only after
> bridge_enable().
> > So, eDP link training can happen and sustain only after bridge_enable().
> >
> > Emulating HPD before/during get_modes will not have any effect because:
> 
> As we have seen, the term HPD is significantly overloaded. What do you
> want to emulate?
> 

On DP, we use HPD event for link training and EDID read.

I understood that you wanted me to emulate HPD event before get_modes()
but because the panel power is controlled by panel-edp, whatever programming
we do on the sink side will be reset when panel power will be turned off by
the pm_runtime_put_autosuspend() of the panel-edp in panel_edp_get_modes().

> >
> > 1. get_modes() will go to panel's get_modes() function to power on
> > read EDID
> >
> > 2. panel power will be turned off after get_modes(). Panel power off
> > will reset every write transaction in DPCD. anyway invalidating link
> > training
> 
> I tend to agree with Doug here. eDP link power status should be handled by
> the pm_runtime_autosuspend with grace period being high enough to cover
> the timeslot between get_mode() and enable().
> 
> panel-edp already does most of required work.
> 

The eDP controller resources are enabled through the host_init() and the link
resources need to be powered on for doing link training, which needs to happen
in the enable() with generic panel-edp.

> >
> > 3. mode_valid will land in dp driver but panel will not be powered on
> > at that time and we cannot do aux transfers or DPCD read writes.
> 
> Why do we need to perform AUX writes in mode_valid?
> 

I am trying to justify why we cannot have mode_valid() implementation similar to DP for eDP.
The detect() and get_modes() are called from panel bridge and panel-edp.c respectively.
The first eDP specific call which will land in the dp_driver is mode_valid(), in which the
controller cannot perform aux access because the panel will not be powered-on.

As the panel-power and backlight are panel resources, we are not enabling/voting for them
from the DP/eDP controller driver.

> >
> > > > So, we need to proceed with the reported mode for eDP.
> > >
> > > Well... Even if during the first call to get_modes() the DPCD is not
> > > read, during subsequent calls the driver has necessary information,
> > > so it can proceed with all the checks, can't it?
> > >
> >
> > get_modes() currently does not land in DP driver. It gets executed in
> > panel bridge. But the mode_valid() comes to DP driver to check the
> > controller compatibility.
> 
> Yes, this is correct. the DP's mode_valid() knows the hardware limitations
> (max clock speed, amount of lanes, etc) and thus it can decide whether the
> mode is supported by the whole chain or not.
> We should not skip such checks for the eDP.
> 
> 

For eDP, we have no information about the number of lanes or the link rate supported
We only know the max lanes from the data-lanes DT property.

> --
> With best wishes
> Dmitry

Re: [PATCH v6 8/8] drm/msm/dp: Handle eDP mode_valid differently from dp

[Dmitry Baryshkov]

On Fri, 8 Apr 2022 at 18:50, Sankeerth Billakanti
<sbillaka@qti.qualcomm.com> wrote:
>
> Hi Dmitry,
>
> > > > > > > > On Wed, 30 Mar 2022 at 19:04, Sankeerth Billakanti
> > > > > > > > <quic_sbillaka@quicinc.com> wrote:
> > > > > > > > >
> > > > > > > > > The panel-edp driver modes needs to be validated
> > > > > > > > > differently from DP because the link capabilities are not
> > > > > > > > > available for EDP by
> > > > that time.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Sankeerth Billakanti
> > > > > > > > > <quic_sbillaka@quicinc.com>
> > > > > > > >
> > > > > > > > This should not be necessary after
> > > > > > > >
> > > > > >
> > > >
> > https://patchwork.freedesktop.org/patch/479261/?series=101682&rev=1.
> > > > > > > > Could you please check?
> > > > > > > >
> > > > > > >
> > > > > > > The check for DP_MAX_PIXEL_CLK_KHZ is not necessary anymore
> > > > > > > but
> > > > we
> > > > > > > need to return early for eDP because unlike DP, eDP context
> > > > > > > will not have the information about the number of lanes and link
> > clock.
> > > > > > >
> > > > > > > So, I will modify the patch to return after the
> > > > > > > DP_MAX_PIXEL_CLK_KHZ
> > > > > > check if is_eDP is set.
> > > > > >
> > > > > > I haven't walked through all the relevant code but something you
> > > > > > said above sounds strange. You say that for eDP we don't have
> > > > > > info about the number of lanes? We _should_.
> > > > > >
> > > > > > It's certainly possible to have a panel that supports _either_ 1
> > > > > > or
> > > > > > 2 lanes but then only physically connect 1 lane to it. ...or you
> > > > > > could have a panel that supports 2 or 4 lanes and you only connect 1
> > lane.
> > > > > > See, for instance, ti_sn_bridge_parse_lanes. There we assume 4
> > > > > > lanes but if a "data-lanes" property is present then we can use
> > > > > > that to know that fewer lanes are physically connected.
> > > > > >
> > > > > > It's also possible to connect more lanes to a panel than it supports.
> > > > > > You could connect 2 lanes to it but then it only supports 1.
> > > > > > This case needs to be handled as well...
> > > > > >
> > > > >
> > > > > I was referring to the checks we do for DP in
> > > > > dp_bridge_mode_valid. We check if the Link bandwidth can support
> > > > > the pixel bandwidth. For an external DP connection, the Initial
> > > > > DPCD/EDID read after cable connection will return the sink
> > > > > capabilities like link rate, lane count and bpp information that
> > > > > are used to we filter out the unsupported
> > > > modes from the list of modes from EDID.
> > > > >
> > > > > For eDP case, the dp driver performs the first dpcd read during
> > > > > bridge_enable. The dp_bridge_mode_valid function is executed
> > > > > before bridge_enable and hence does not have the full link or the
> > > > > sink capabilities information like external DP connection, by then.
> > > >
> > > > It sounds to me like we should emulate the HPD event for eDP to be
> > > > handled earlier than the get_modes()/prepare() calls are attempted.
> > > > However this might open another can of worms.
> > > >
> > >
> > > For DP, the HPD handler mainly initiates link training and gets the EDID.
> > >
> > > Before adding support for a separate eDP panel, we had discussed about
> > > this internally and decided to emulate eDP HPD during enable(). Main
> > > reason being, eDP power is guaranteed to be on only after
> > bridge_enable().
> > > So, eDP link training can happen and sustain only after bridge_enable().
> > >
> > > Emulating HPD before/during get_modes will not have any effect because:
> >
> > As we have seen, the term HPD is significantly overloaded. What do you
> > want to emulate?
> >
>
> On DP, we use HPD event for link training and EDID read.
>
> I understood that you wanted me to emulate HPD event before get_modes()
> but because the panel power is controlled by panel-edp, whatever programming
> we do on the sink side will be reset when panel power will be turned off by
> the pm_runtime_put_autosuspend() of the panel-edp in panel_edp_get_modes().

The pm_runtime_put_autosuspend() wouldn't suspend the device
immediately. It will be suspended after the grace period finished, if
nobody resumes the devices again. This is how it works in the
sn65dsi86 driver. It sets the timeout delay long enough, so that
get_modes and pre_enable would typically work together without
suspending the host.

>
> > >
> > > 1. get_modes() will go to panel's get_modes() function to power on
> > > read EDID
> > >
> > > 2. panel power will be turned off after get_modes(). Panel power off
> > > will reset every write transaction in DPCD. anyway invalidating link
> > > training
> >
> > I tend to agree with Doug here. eDP link power status should be handled by
> > the pm_runtime_autosuspend with grace period being high enough to cover
> > the timeslot between get_mode() and enable().
> >
> > panel-edp already does most of required work.
> >
>
> The eDP controller resources are enabled through the host_init() and the link
> resources need to be powered on for doing link training, which needs to happen
> in the enable() with generic panel-edp.

nothing wrong with that up to now

>
> > >
> > > 3. mode_valid will land in dp driver but panel will not be powered on
> > > at that time and we cannot do aux transfers or DPCD read writes.
> >
> > Why do we need to perform AUX writes in mode_valid?
> >
>
> I am trying to justify why we cannot have mode_valid() implementation similar to DP for eDP.
> The detect() and get_modes() are called from panel bridge and panel-edp.c respectively.
> The first eDP specific call which will land in the dp_driver is mode_valid(), in which the
> controller cannot perform aux access because the panel will not be powered-on.

I fail to understand why you'd like to perform aux access from
mode_valid at all.

> As the panel-power and backlight are panel resources, we are not enabling/voting for them
> from the DP/eDP controller driver.

correct

>
> > >
> > > > > So, we need to proceed with the reported mode for eDP.
> > > >
> > > > Well... Even if during the first call to get_modes() the DPCD is not
> > > > read, during subsequent calls the driver has necessary information,
> > > > so it can proceed with all the checks, can't it?
> > > >
> > >
> > > get_modes() currently does not land in DP driver. It gets executed in
> > > panel bridge. But the mode_valid() comes to DP driver to check the
> > > controller compatibility.
> >
> > Yes, this is correct. the DP's mode_valid() knows the hardware limitations
> > (max clock speed, amount of lanes, etc) and thus it can decide whether the
> > mode is supported by the whole chain or not.
> > We should not skip such checks for the eDP.
> >
> >
>
> For eDP, we have no information about the number of lanes or the link rate supported
> We only know the max lanes from the data-lanes DT property.

If the device connects just a single line to the eDP panel, the DT
will be changed to list that single lane.
It looks like we have to call dp_panel_read_sink_caps() somewhere for
the eDP case. For the DP case the HPD callbacks do this work.

No, mode_valid doesn't look like a proper place. We already have read
modes, so the AUX bus has been powered for some time. We could do it
earlier.

-- 
With best wishes
Dmitry

RE: [PATCH v6 8/8] drm/msm/dp: Handle eDP mode_valid differently from dp

[Sankeerth Billakanti]

> > > > > > > > > On Wed, 30 Mar 2022 at 19:04, Sankeerth Billakanti
> > > > > > > > > <quic_sbillaka@quicinc.com> wrote:
> > > > > > > > > >
> > > > > > > > > > The panel-edp driver modes needs to be validated
> > > > > > > > > > differently from DP because the link capabilities are
> > > > > > > > > > not available for EDP by
> > > > > that time.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Sankeerth Billakanti
> > > > > > > > > > <quic_sbillaka@quicinc.com>
> > > > > > > > >
> > > > > > > > > This should not be necessary after
> > > > > > > > >
> > > > > > >
> > > > >
> > >
> https://patchwork.freedesktop.org/patch/479261/?series=101682&rev=1.
> > > > > > > > > Could you please check?
> > > > > > > > >
> > > > > > > >
> > > > > > > > The check for DP_MAX_PIXEL_CLK_KHZ is not necessary
> > > > > > > > anymore but
> > > > > we
> > > > > > > > need to return early for eDP because unlike DP, eDP
> > > > > > > > context will not have the information about the number of
> > > > > > > > lanes and link
> > > clock.
> > > > > > > >
> > > > > > > > So, I will modify the patch to return after the
> > > > > > > > DP_MAX_PIXEL_CLK_KHZ
> > > > > > > check if is_eDP is set.
> > > > > > >
> > > > > > > I haven't walked through all the relevant code but something
> > > > > > > you said above sounds strange. You say that for eDP we don't
> > > > > > > have info about the number of lanes? We _should_.
> > > > > > >
> > > > > > > It's certainly possible to have a panel that supports
> > > > > > > _either_ 1 or
> > > > > > > 2 lanes but then only physically connect 1 lane to it. ...or
> > > > > > > you could have a panel that supports 2 or 4 lanes and you
> > > > > > > only connect 1
> > > lane.
> > > > > > > See, for instance, ti_sn_bridge_parse_lanes. There we assume
> > > > > > > 4 lanes but if a "data-lanes" property is present then we
> > > > > > > can use that to know that fewer lanes are physically connected.
> > > > > > >
> > > > > > > It's also possible to connect more lanes to a panel than it supports.
> > > > > > > You could connect 2 lanes to it but then it only supports 1.
> > > > > > > This case needs to be handled as well...
> > > > > > >
> > > > > >
> > > > > > I was referring to the checks we do for DP in
> > > > > > dp_bridge_mode_valid. We check if the Link bandwidth can
> > > > > > support the pixel bandwidth. For an external DP connection,
> > > > > > the Initial DPCD/EDID read after cable connection will return
> > > > > > the sink capabilities like link rate, lane count and bpp
> > > > > > information that are used to we filter out the unsupported
> > > > > modes from the list of modes from EDID.
> > > > > >
> > > > > > For eDP case, the dp driver performs the first dpcd read
> > > > > > during bridge_enable. The dp_bridge_mode_valid function is
> > > > > > executed before bridge_enable and hence does not have the full
> > > > > > link or the sink capabilities information like external DP connection,
> by then.
> > > > >
> > > > > It sounds to me like we should emulate the HPD event for eDP to
> > > > > be handled earlier than the get_modes()/prepare() calls are
> attempted.
> > > > > However this might open another can of worms.
> > > > >
> > > >
> > > > For DP, the HPD handler mainly initiates link training and gets the EDID.
> > > >
> > > > Before adding support for a separate eDP panel, we had discussed
> > > > about this internally and decided to emulate eDP HPD during
> > > > enable(). Main reason being, eDP power is guaranteed to be on only
> > > > after
> > > bridge_enable().
> > > > So, eDP link training can happen and sustain only after bridge_enable().
> > > >
> > > > Emulating HPD before/during get_modes will not have any effect
> because:
> > >
> > > As we have seen, the term HPD is significantly overloaded. What do
> > > you want to emulate?
> > >
> >
> > On DP, we use HPD event for link training and EDID read.
> >
> > I understood that you wanted me to emulate HPD event before
> > get_modes() but because the panel power is controlled by panel-edp,
> > whatever programming we do on the sink side will be reset when panel
> > power will be turned off by the pm_runtime_put_autosuspend() of the
> panel-edp in panel_edp_get_modes().
> 
> The pm_runtime_put_autosuspend() wouldn't suspend the device
> immediately. It will be suspended after the grace period finished, if nobody
> resumes the devices again. This is how it works in the
> sn65dsi86 driver. It sets the timeout delay long enough, so that get_modes
> and pre_enable would typically work together without suspending the host.
> 

Okay. We are not implementing a bridge pre_enable currently

> >
> > > >
> > > > 1. get_modes() will go to panel's get_modes() function to power on
> > > > read EDID
> > > >
> > > > 2. panel power will be turned off after get_modes(). Panel power
> > > > off will reset every write transaction in DPCD. anyway
> > > > invalidating link training
> > >
> > > I tend to agree with Doug here. eDP link power status should be
> > > handled by the pm_runtime_autosuspend with grace period being high
> > > enough to cover the timeslot between get_mode() and enable().
> > >
> > > panel-edp already does most of required work.
> > >
> >
> > The eDP controller resources are enabled through the host_init() and
> > the link resources need to be powered on for doing link training,
> > which needs to happen in the enable() with generic panel-edp.
> 
> nothing wrong with that up to now
> 
> >
> > > >
> > > > 3. mode_valid will land in dp driver but panel will not be powered
> > > > on at that time and we cannot do aux transfers or DPCD read writes.
> > >
> > > Why do we need to perform AUX writes in mode_valid?
> > >
> >
> > I am trying to justify why we cannot have mode_valid() implementation
> similar to DP for eDP.
> > The detect() and get_modes() are called from panel bridge and panel-
> edp.c respectively.
> > The first eDP specific call which will land in the dp_driver is
> > mode_valid(), in which the controller cannot perform aux access because
> the panel will not be powered-on.
> 
> I fail to understand why you'd like to perform aux access from mode_valid at
> all.

I don't want to perform it in mode_valid. I am just saying that, apart from mode_valid(),
there is no other eDP call (other than aux_transfer) which will land in the DP driver before the mode_set().
So, currently there is no function in which we can get the eDP sink capabilities before enable().
So, we assume the mode will be supported if the pixel clock is less than the max clock of 675MHz.

> 
> > As the panel-power and backlight are panel resources, we are not
> > enabling/voting for them from the DP/eDP controller driver.
> 
> correct
> 
> >
> > > >
> > > > > > So, we need to proceed with the reported mode for eDP.
> > > > >
> > > > > Well... Even if during the first call to get_modes() the DPCD is
> > > > > not read, during subsequent calls the driver has necessary
> > > > > information, so it can proceed with all the checks, can't it?
> > > > >
> > > >
> > > > get_modes() currently does not land in DP driver. It gets executed
> > > > in panel bridge. But the mode_valid() comes to DP driver to check
> > > > the controller compatibility.
> > >
> > > Yes, this is correct. the DP's mode_valid() knows the hardware
> > > limitations (max clock speed, amount of lanes, etc) and thus it can
> > > decide whether the mode is supported by the whole chain or not.
> > > We should not skip such checks for the eDP.
> > >
> > >
> >
> > For eDP, we have no information about the number of lanes or the link
> > rate supported We only know the max lanes from the data-lanes DT
> property.
> 
> If the device connects just a single line to the eDP panel, the DT will be
> changed to list that single lane.
> It looks like we have to call dp_panel_read_sink_caps() somewhere for the
> eDP case. For the DP case the HPD callbacks do this work.
> 
> No, mode_valid doesn't look like a proper place. We already have read
> modes, so the AUX bus has been powered for some time. We could do it
> earlier.

Correct, we have to do it earlier. But is there some function in which we can get
the dp_panel_read_sink_caps() before get_modes?

A way could be to implement the mode_valid also in panel-eDP along with the
get_modes. We can read the sink capabilities in get_modes in panel-edp.c and
check in the mode_valid() in panel-edp.c.

I also feel the mode_valid() needs to check if a controller can support it rather
than the panel. So, I cannot find a way where to get the sink caps now before
the mode_set() or enable()

> 
> --
> With best wishes
> Dmitry

Re: [PATCH v6 8/8] drm/msm/dp: Handle eDP mode_valid differently from dp

[Dmitry Baryshkov]

On Fri, 8 Apr 2022 at 20:38, Sankeerth Billakanti
<sbillaka@qti.qualcomm.com> wrote:
>
> > > > > > > > > > On Wed, 30 Mar 2022 at 19:04, Sankeerth Billakanti
> > > > > > > > > > <quic_sbillaka@quicinc.com> wrote:
> > > > > > > > > > >
> > > > > > > > > > > The panel-edp driver modes needs to be validated
> > > > > > > > > > > differently from DP because the link capabilities are
> > > > > > > > > > > not available for EDP by
> > > > > > that time.
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Sankeerth Billakanti
> > > > > > > > > > > <quic_sbillaka@quicinc.com>
> > > > > > > > > >
> > > > > > > > > > This should not be necessary after
> > > > > > > > > >
> > > > > > > >
> > > > > >
> > > >
> > https://patchwork.freedesktop.org/patch/479261/?series=101682&rev=1.
> > > > > > > > > > Could you please check?
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > The check for DP_MAX_PIXEL_CLK_KHZ is not necessary
> > > > > > > > > anymore but
> > > > > > we
> > > > > > > > > need to return early for eDP because unlike DP, eDP
> > > > > > > > > context will not have the information about the number of
> > > > > > > > > lanes and link
> > > > clock.
> > > > > > > > >
> > > > > > > > > So, I will modify the patch to return after the
> > > > > > > > > DP_MAX_PIXEL_CLK_KHZ
> > > > > > > > check if is_eDP is set.
> > > > > > > >
> > > > > > > > I haven't walked through all the relevant code but something
> > > > > > > > you said above sounds strange. You say that for eDP we don't
> > > > > > > > have info about the number of lanes? We _should_.
> > > > > > > >
> > > > > > > > It's certainly possible to have a panel that supports
> > > > > > > > _either_ 1 or
> > > > > > > > 2 lanes but then only physically connect 1 lane to it. ...or
> > > > > > > > you could have a panel that supports 2 or 4 lanes and you
> > > > > > > > only connect 1
> > > > lane.
> > > > > > > > See, for instance, ti_sn_bridge_parse_lanes. There we assume
> > > > > > > > 4 lanes but if a "data-lanes" property is present then we
> > > > > > > > can use that to know that fewer lanes are physically connected.
> > > > > > > >
> > > > > > > > It's also possible to connect more lanes to a panel than it supports.
> > > > > > > > You could connect 2 lanes to it but then it only supports 1.
> > > > > > > > This case needs to be handled as well...
> > > > > > > >
> > > > > > >
> > > > > > > I was referring to the checks we do for DP in
> > > > > > > dp_bridge_mode_valid. We check if the Link bandwidth can
> > > > > > > support the pixel bandwidth. For an external DP connection,
> > > > > > > the Initial DPCD/EDID read after cable connection will return
> > > > > > > the sink capabilities like link rate, lane count and bpp
> > > > > > > information that are used to we filter out the unsupported
> > > > > > modes from the list of modes from EDID.
> > > > > > >
> > > > > > > For eDP case, the dp driver performs the first dpcd read
> > > > > > > during bridge_enable. The dp_bridge_mode_valid function is
> > > > > > > executed before bridge_enable and hence does not have the full
> > > > > > > link or the sink capabilities information like external DP connection,
> > by then.
> > > > > >
> > > > > > It sounds to me like we should emulate the HPD event for eDP to
> > > > > > be handled earlier than the get_modes()/prepare() calls are
> > attempted.
> > > > > > However this might open another can of worms.
> > > > > >
> > > > >
> > > > > For DP, the HPD handler mainly initiates link training and gets the EDID.
> > > > >
> > > > > Before adding support for a separate eDP panel, we had discussed
> > > > > about this internally and decided to emulate eDP HPD during
> > > > > enable(). Main reason being, eDP power is guaranteed to be on only
> > > > > after
> > > > bridge_enable().
> > > > > So, eDP link training can happen and sustain only after bridge_enable().
> > > > >
> > > > > Emulating HPD before/during get_modes will not have any effect
> > because:
> > > >
> > > > As we have seen, the term HPD is significantly overloaded. What do
> > > > you want to emulate?
> > > >
> > >
> > > On DP, we use HPD event for link training and EDID read.
> > >
> > > I understood that you wanted me to emulate HPD event before
> > > get_modes() but because the panel power is controlled by panel-edp,
> > > whatever programming we do on the sink side will be reset when panel
> > > power will be turned off by the pm_runtime_put_autosuspend() of the
> > panel-edp in panel_edp_get_modes().
> >
> > The pm_runtime_put_autosuspend() wouldn't suspend the device
> > immediately. It will be suspended after the grace period finished, if nobody
> > resumes the devices again. This is how it works in the
> > sn65dsi86 driver. It sets the timeout delay long enough, so that get_modes
> > and pre_enable would typically work together without suspending the host.
> >
>
> Okay. We are not implementing a bridge pre_enable currently
>
> > >
> > > > >
> > > > > 1. get_modes() will go to panel's get_modes() function to power on
> > > > > read EDID
> > > > >
> > > > > 2. panel power will be turned off after get_modes(). Panel power
> > > > > off will reset every write transaction in DPCD. anyway
> > > > > invalidating link training
> > > >
> > > > I tend to agree with Doug here. eDP link power status should be
> > > > handled by the pm_runtime_autosuspend with grace period being high
> > > > enough to cover the timeslot between get_mode() and enable().
> > > >
> > > > panel-edp already does most of required work.
> > > >
> > >
> > > The eDP controller resources are enabled through the host_init() and
> > > the link resources need to be powered on for doing link training,
> > > which needs to happen in the enable() with generic panel-edp.
> >
> > nothing wrong with that up to now
> >
> > >
> > > > >
> > > > > 3. mode_valid will land in dp driver but panel will not be powered
> > > > > on at that time and we cannot do aux transfers or DPCD read writes.
> > > >
> > > > Why do we need to perform AUX writes in mode_valid?
> > > >
> > >
> > > I am trying to justify why we cannot have mode_valid() implementation
> > similar to DP for eDP.
> > > The detect() and get_modes() are called from panel bridge and panel-
> > edp.c respectively.
> > > The first eDP specific call which will land in the dp_driver is
> > > mode_valid(), in which the controller cannot perform aux access because
> > the panel will not be powered-on.
> >
> > I fail to understand why you'd like to perform aux access from mode_valid at
> > all.
>
> I don't want to perform it in mode_valid. I am just saying that, apart from mode_valid(),
> there is no other eDP call (other than aux_transfer) which will land in the DP driver before the mode_set().
> So, currently there is no function in which we can get the eDP sink capabilities before enable().
> So, we assume the mode will be supported if the pixel clock is less than the max clock of 675MHz.
>
> >
> > > As the panel-power and backlight are panel resources, we are not
> > > enabling/voting for them from the DP/eDP controller driver.
> >
> > correct
> >
> > >
> > > > >
> > > > > > > So, we need to proceed with the reported mode for eDP.
> > > > > >
> > > > > > Well... Even if during the first call to get_modes() the DPCD is
> > > > > > not read, during subsequent calls the driver has necessary
> > > > > > information, so it can proceed with all the checks, can't it?
> > > > > >
> > > > >
> > > > > get_modes() currently does not land in DP driver. It gets executed
> > > > > in panel bridge. But the mode_valid() comes to DP driver to check
> > > > > the controller compatibility.
> > > >
> > > > Yes, this is correct. the DP's mode_valid() knows the hardware
> > > > limitations (max clock speed, amount of lanes, etc) and thus it can
> > > > decide whether the mode is supported by the whole chain or not.
> > > > We should not skip such checks for the eDP.
> > > >
> > > >
> > >
> > > For eDP, we have no information about the number of lanes or the link
> > > rate supported We only know the max lanes from the data-lanes DT
> > property.
> >
> > If the device connects just a single line to the eDP panel, the DT will be
> > changed to list that single lane.
> > It looks like we have to call dp_panel_read_sink_caps() somewhere for the
> > eDP case. For the DP case the HPD callbacks do this work.
> >
> > No, mode_valid doesn't look like a proper place. We already have read
> > modes, so the AUX bus has been powered for some time. We could do it
> > earlier.
>
> Correct, we have to do it earlier. But is there some function in which we can get
> the dp_panel_read_sink_caps() before get_modes?
>
> A way could be to implement the mode_valid also in panel-eDP along with the
> get_modes. We can read the sink capabilities in get_modes in panel-edp.c and
> check in the mode_valid() in panel-edp.c.
>
> I also feel the mode_valid() needs to check if a controller can support it rather
> than the panel. So, I cannot find a way where to get the sink caps now before
> the mode_set() or enable()

Anywhere after you have the reference to the next_bridge, you can be
sure that the panel is present. So you can power up the AUX bus, read
the caps, and (auto-)suspend it again.


-- 
With best wishes
Dmitry