[RFC][PATCH] drm: kirin: Fix dsi probe/attach logic

[RFC][PATCH] drm: kirin: Fix dsi probe/attach logic

[John Stultz]

Since commit 83f35bc3a852 ("drm/bridge: adv7511: Attach to DSI
host at probe time") landed in -next the HiKey board would fail
to boot, looping:

  adv7511 2-0039: failed to find dsi host

messages over and over. Andrzej Hajda suggested this is due to a
circular dependency issue, and that the adv7511 change is
correcting the improper order used earlier.

Unfortunately this means the DSI drivers that use adv7511 need
to also need to be updated to use the proper ordering to
continue to work.

This patch tries to reorder the initialization to register the
dsi_host first, and then call component_add via dsi_host_attach,
instead of doing that at probe time.

This seems to resolve the issue with the HiKey board.

Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Matt Redfearn <matt.redfearn@thinci.com>
Cc: Xinliang Liu <z.liuxinliang@hisilicon.com>
Cc: Rongrong Zou <zourongrong@gmail.com>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Jernej Skrabec <jernej.skrabec@siol.net>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: David Airlie <airlied@linux.ie>,
Cc: Sean Paul <seanpaul@chromium.org>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>
Fixes: 83f35bc3a852 ("drm/bridge: adv7511: Attach to DSI host at probe time")
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
Note: I'm really not super familiar with the DSI code here,
and am mostly just trying to refactor the existing code in a
similar fashion to the suggested dw-mipi-dsi-rockchip.c
implementation. Careful review would be greatly appreciated!

Also there is an outstanding regression on the db410c since it
similarly uses the adv7511 and probably needs a similar rework.
---
 drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c | 111 ++++++++++---------
 1 file changed, 56 insertions(+), 55 deletions(-)

diff --git a/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c b/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c
index 5bf8138941de..696cee1a1219 100644
--- a/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c
+++ b/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c
@@ -79,6 +79,7 @@ struct dsi_hw_ctx {
 };
 
 struct dw_dsi {
+	struct device *dev;
 	struct drm_encoder encoder;
 	struct drm_bridge *bridge;
 	struct mipi_dsi_host host;
@@ -724,51 +725,6 @@ static int dw_drm_encoder_init(struct device *dev,
 	return 0;
 }
 
-static int dsi_host_attach(struct mipi_dsi_host *host,
-			   struct mipi_dsi_device *mdsi)
-{
-	struct dw_dsi *dsi = host_to_dsi(host);
-
-	if (mdsi->lanes < 1 || mdsi->lanes > 4) {
-		DRM_ERROR("dsi device params invalid\n");
-		return -EINVAL;
-	}
-
-	dsi->lanes = mdsi->lanes;
-	dsi->format = mdsi->format;
-	dsi->mode_flags = mdsi->mode_flags;
-
-	return 0;
-}
-
-static int dsi_host_detach(struct mipi_dsi_host *host,
-			   struct mipi_dsi_device *mdsi)
-{
-	/* do nothing */
-	return 0;
-}
-
-static const struct mipi_dsi_host_ops dsi_host_ops = {
-	.attach = dsi_host_attach,
-	.detach = dsi_host_detach,
-};
-
-static int dsi_host_init(struct device *dev, struct dw_dsi *dsi)
-{
-	struct mipi_dsi_host *host = &dsi->host;
-	int ret;
-
-	host->dev = dev;
-	host->ops = &dsi_host_ops;
-	ret = mipi_dsi_host_register(host);
-	if (ret) {
-		DRM_ERROR("failed to register dsi host\n");
-		return ret;
-	}
-
-	return 0;
-}
-
 static int dsi_bridge_init(struct drm_device *dev, struct dw_dsi *dsi)
 {
 	struct drm_encoder *encoder = &dsi->encoder;
@@ -796,10 +752,6 @@ static int dsi_bind(struct device *dev, struct device *master, void *data)
 	if (ret)
 		return ret;
 
-	ret = dsi_host_init(dev, dsi);
-	if (ret)
-		return ret;
-
 	ret = dsi_bridge_init(drm_dev, dsi);
 	if (ret)
 		return ret;
@@ -817,13 +769,22 @@ static const struct component_ops dsi_ops = {
 	.unbind	= dsi_unbind,
 };
 
-static int dsi_parse_dt(struct platform_device *pdev, struct dw_dsi *dsi)
+static int dsi_host_attach(struct mipi_dsi_host *host,
+			   struct mipi_dsi_device *mdsi)
 {
-	struct dsi_hw_ctx *ctx = dsi->ctx;
-	struct device_node *np = pdev->dev.of_node;
-	struct resource *res;
+	struct dw_dsi *dsi = host_to_dsi(host);
+	struct device_node *np = dsi->dev->of_node;
 	int ret;
 
+	if (mdsi->lanes < 1 || mdsi->lanes > 4) {
+		DRM_ERROR("dsi device params invalid\n");
+		return -EINVAL;
+	}
+
+	dsi->lanes = mdsi->lanes;
+	dsi->format = mdsi->format;
+	dsi->mode_flags = mdsi->mode_flags;
+
 	/*
 	 * Get the endpoint node. In our case, dsi has one output port1
 	 * to which the external HDMI bridge is connected.
@@ -832,6 +793,42 @@ static int dsi_parse_dt(struct platform_device *pdev, struct dw_dsi *dsi)
 	if (ret)
 		return ret;
 
+	return component_add(dsi->dev, &dsi_ops);
+}
+
+static int dsi_host_detach(struct mipi_dsi_host *host,
+			   struct mipi_dsi_device *mdsi)
+{
+	/* do nothing */
+	return 0;
+}
+
+static const struct mipi_dsi_host_ops dsi_host_ops = {
+	.attach = dsi_host_attach,
+	.detach = dsi_host_detach,
+};
+
+static int dsi_host_init(struct device *dev, struct dw_dsi *dsi)
+{
+	struct mipi_dsi_host *host = &dsi->host;
+	int ret;
+
+	host->dev = dev;
+	host->ops = &dsi_host_ops;
+	ret = mipi_dsi_host_register(host);
+	if (ret) {
+		DRM_ERROR("failed to register dsi host\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int dsi_parse_dt(struct platform_device *pdev, struct dw_dsi *dsi)
+{
+	struct dsi_hw_ctx *ctx = dsi->ctx;
+	struct resource *res;
+
 	ctx->pclk = devm_clk_get(&pdev->dev, "pclk");
 	if (IS_ERR(ctx->pclk)) {
 		DRM_ERROR("failed to get pclk clock\n");
@@ -862,15 +859,19 @@ static int dsi_probe(struct platform_device *pdev)
 	}
 	dsi = &data->dsi;
 	ctx = &data->ctx;
+	dsi->dev = &pdev->dev;
 	dsi->ctx = ctx;
 
 	ret = dsi_parse_dt(pdev, dsi);
 	if (ret)
 		return ret;
 
-	platform_set_drvdata(pdev, data);
+	ret = dsi_host_init(&pdev->dev, dsi);
+	if (ret)
+		return ret;
 
-	return component_add(&pdev->dev, &dsi_ops);
+	platform_set_drvdata(pdev, data);
+	return 0;
 }
 
 static int dsi_remove(struct platform_device *pdev)
-- 
2.17.1

Re: [RFC][PATCH] drm: kirin: Fix dsi probe/attach logic

[Andrzej Hajda]

On 29.08.2019 08:05, John Stultz wrote:
> Since commit 83f35bc3a852 ("drm/bridge: adv7511: Attach to DSI
> host at probe time") landed in -next the HiKey board would fail
> to boot, looping:
>
>   adv7511 2-0039: failed to find dsi host
>
> messages over and over. Andrzej Hajda suggested this is due to a
> circular dependency issue, and that the adv7511 change is
> correcting the improper order used earlier.
>
> Unfortunately this means the DSI drivers that use adv7511 need
> to also need to be updated to use the proper ordering to
> continue to work.
>
> This patch tries to reorder the initialization to register the
> dsi_host first, and then call component_add via dsi_host_attach,
> instead of doing that at probe time.
>
> This seems to resolve the issue with the HiKey board.
>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Matt Redfearn <matt.redfearn@thinci.com>
> Cc: Xinliang Liu <z.liuxinliang@hisilicon.com>
> Cc: Rongrong Zou <zourongrong@gmail.com>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Jernej Skrabec <jernej.skrabec@siol.net>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: David Airlie <airlied@linux.ie>,
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>
> Fixes: 83f35bc3a852 ("drm/bridge: adv7511: Attach to DSI host at probe time")
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
> Note: I'm really not super familiar with the DSI code here,
> and am mostly just trying to refactor the existing code in a
> similar fashion to the suggested dw-mipi-dsi-rockchip.c
> implementation. Careful review would be greatly appreciated!
>
> Also there is an outstanding regression on the db410c since it
> similarly uses the adv7511 and probably needs a similar rework.
> ---
>  drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c | 111 ++++++++++---------
>  1 file changed, 56 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c b/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c
> index 5bf8138941de..696cee1a1219 100644
> --- a/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c
> +++ b/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c
> @@ -79,6 +79,7 @@ struct dsi_hw_ctx {
>  };
>  
>  struct dw_dsi {
> +	struct device *dev;
>  	struct drm_encoder encoder;
>  	struct drm_bridge *bridge;
>  	struct mipi_dsi_host host;
> @@ -724,51 +725,6 @@ static int dw_drm_encoder_init(struct device *dev,
>  	return 0;
>  }
>  
> -static int dsi_host_attach(struct mipi_dsi_host *host,
> -			   struct mipi_dsi_device *mdsi)
> -{
> -	struct dw_dsi *dsi = host_to_dsi(host);
> -
> -	if (mdsi->lanes < 1 || mdsi->lanes > 4) {
> -		DRM_ERROR("dsi device params invalid\n");
> -		return -EINVAL;
> -	}
> -
> -	dsi->lanes = mdsi->lanes;
> -	dsi->format = mdsi->format;
> -	dsi->mode_flags = mdsi->mode_flags;
> -
> -	return 0;
> -}
> -
> -static int dsi_host_detach(struct mipi_dsi_host *host,
> -			   struct mipi_dsi_device *mdsi)
> -{
> -	/* do nothing */
> -	return 0;
> -}
> -
> -static const struct mipi_dsi_host_ops dsi_host_ops = {
> -	.attach = dsi_host_attach,
> -	.detach = dsi_host_detach,
> -};
> -
> -static int dsi_host_init(struct device *dev, struct dw_dsi *dsi)
> -{
> -	struct mipi_dsi_host *host = &dsi->host;
> -	int ret;
> -
> -	host->dev = dev;
> -	host->ops = &dsi_host_ops;
> -	ret = mipi_dsi_host_register(host);
> -	if (ret) {
> -		DRM_ERROR("failed to register dsi host\n");
> -		return ret;
> -	}
> -
> -	return 0;
> -}
> -
>  static int dsi_bridge_init(struct drm_device *dev, struct dw_dsi *dsi)
>  {
>  	struct drm_encoder *encoder = &dsi->encoder;
> @@ -796,10 +752,6 @@ static int dsi_bind(struct device *dev, struct device *master, void *data)
>  	if (ret)
>  		return ret;
>  
> -	ret = dsi_host_init(dev, dsi);
> -	if (ret)
> -		return ret;
> -
>  	ret = dsi_bridge_init(drm_dev, dsi);
>  	if (ret)
>  		return ret;
> @@ -817,13 +769,22 @@ static const struct component_ops dsi_ops = {
>  	.unbind	= dsi_unbind,
>  };
>  
> -static int dsi_parse_dt(struct platform_device *pdev, struct dw_dsi *dsi)
> +static int dsi_host_attach(struct mipi_dsi_host *host,
> +			   struct mipi_dsi_device *mdsi)
>  {
> -	struct dsi_hw_ctx *ctx = dsi->ctx;
> -	struct device_node *np = pdev->dev.of_node;
> -	struct resource *res;
> +	struct dw_dsi *dsi = host_to_dsi(host);
> +	struct device_node *np = dsi->dev->of_node;
>  	int ret;
>  
> +	if (mdsi->lanes < 1 || mdsi->lanes > 4) {
> +		DRM_ERROR("dsi device params invalid\n");
> +		return -EINVAL;
> +	}
> +
> +	dsi->lanes = mdsi->lanes;
> +	dsi->format = mdsi->format;
> +	dsi->mode_flags = mdsi->mode_flags;
> +
>  	/*
>  	 * Get the endpoint node. In our case, dsi has one output port1
>  	 * to which the external HDMI bridge is connected.
> @@ -832,6 +793,42 @@ static int dsi_parse_dt(struct platform_device *pdev, struct dw_dsi *dsi)
>  	if (ret)
>  		return ret;
>  
> +	return component_add(dsi->dev, &dsi_ops);
> +}
> +
> +static int dsi_host_detach(struct mipi_dsi_host *host,
> +			   struct mipi_dsi_device *mdsi)
> +{
> +	/* do nothing */
> +	return 0;
> +}
> +
> +static const struct mipi_dsi_host_ops dsi_host_ops = {
> +	.attach = dsi_host_attach,
> +	.detach = dsi_host_detach,
> +};
> +
> +static int dsi_host_init(struct device *dev, struct dw_dsi *dsi)
> +{
> +	struct mipi_dsi_host *host = &dsi->host;
> +	int ret;
> +
> +	host->dev = dev;
> +	host->ops = &dsi_host_ops;
> +	ret = mipi_dsi_host_register(host);
> +	if (ret) {
> +		DRM_ERROR("failed to register dsi host\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int dsi_parse_dt(struct platform_device *pdev, struct dw_dsi *dsi)
> +{
> +	struct dsi_hw_ctx *ctx = dsi->ctx;
> +	struct resource *res;
> +
>  	ctx->pclk = devm_clk_get(&pdev->dev, "pclk");
>  	if (IS_ERR(ctx->pclk)) {
>  		DRM_ERROR("failed to get pclk clock\n");
> @@ -862,15 +859,19 @@ static int dsi_probe(struct platform_device *pdev)
>  	}
>  	dsi = &data->dsi;
>  	ctx = &data->ctx;
> +	dsi->dev = &pdev->dev;
>  	dsi->ctx = ctx;
>  
>  	ret = dsi_parse_dt(pdev, dsi);
>  	if (ret)
>  		return ret;
>  
> -	platform_set_drvdata(pdev, data);
> +	ret = dsi_host_init(&pdev->dev, dsi);
> +	if (ret)
> +		return ret;
>  
> -	return component_add(&pdev->dev, &dsi_ops);
> +	platform_set_drvdata(pdev, data);


I suspect platform_set_drvdata should be before dsi_host_init.

Imagine dsi_probe calls dsi_host_init it registers dsi_host and
instantly dsi_device appears, so dsi_host_attach is called, it calls
component_add and dsi_bind is called, which requires valid drvdata.


Beside this it looks correct, but it would be good to test it.

So after fixing above you can add my R-B.


Regards

Andrzej


> +	return 0;
>  }
>  
>  static int dsi_remove(struct platform_device *pdev)

Re: [RFC][PATCH] drm: kirin: Fix dsi probe/attach logic

[Rob Clark]

On Wed, Aug 28, 2019 at 11:06 PM John Stultz <john.stultz@linaro.org> wrote:
>
> Since commit 83f35bc3a852 ("drm/bridge: adv7511: Attach to DSI
> host at probe time") landed in -next the HiKey board would fail
> to boot, looping:

No, please revert 83f35bc3a852.. that is going in the *complete* wrong
direction.  We actually should be moving panels to not require dsi
host until attach time, similar to how bridges work, not the other way
around.

The problem is that, when dealing with bootloader enabled display, we
need to be really careful not to touch the hardware until the display
driver knows the bridge/panel is present.  If the bridge/panel probes
after the display driver, we could end up killing scanout
(efifb/simplefb).. if the bridge/panel is missing some dependency and
never probes, it is rather unpleasant to be stuck trying to debug what
went wrong with no display.

Sorry I didn't notice that adv7511 patch before it landed, but the
right thing to do now is to revert it.

BR,
-R

>   adv7511 2-0039: failed to find dsi host
>
> messages over and over. Andrzej Hajda suggested this is due to a
> circular dependency issue, and that the adv7511 change is
> correcting the improper order used earlier.
>
> Unfortunately this means the DSI drivers that use adv7511 need
> to also need to be updated to use the proper ordering to
> continue to work.
>
> This patch tries to reorder the initialization to register the
> dsi_host first, and then call component_add via dsi_host_attach,
> instead of doing that at probe time.
>
> This seems to resolve the issue with the HiKey board.
>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Matt Redfearn <matt.redfearn@thinci.com>
> Cc: Xinliang Liu <z.liuxinliang@hisilicon.com>
> Cc: Rongrong Zou <zourongrong@gmail.com>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Jernej Skrabec <jernej.skrabec@siol.net>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: David Airlie <airlied@linux.ie>,
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>
> Fixes: 83f35bc3a852 ("drm/bridge: adv7511: Attach to DSI host at probe time")
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
> Note: I'm really not super familiar with the DSI code here,
> and am mostly just trying to refactor the existing code in a
> similar fashion to the suggested dw-mipi-dsi-rockchip.c
> implementation. Careful review would be greatly appreciated!
>
> Also there is an outstanding regression on the db410c since it
> similarly uses the adv7511 and probably needs a similar rework.
> ---
>  drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c | 111 ++++++++++---------
>  1 file changed, 56 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c b/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c
> index 5bf8138941de..696cee1a1219 100644
> --- a/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c
> +++ b/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c
> @@ -79,6 +79,7 @@ struct dsi_hw_ctx {
>  };
>
>  struct dw_dsi {
> +       struct device *dev;
>         struct drm_encoder encoder;
>         struct drm_bridge *bridge;
>         struct mipi_dsi_host host;
> @@ -724,51 +725,6 @@ static int dw_drm_encoder_init(struct device *dev,
>         return 0;
>  }
>
> -static int dsi_host_attach(struct mipi_dsi_host *host,
> -                          struct mipi_dsi_device *mdsi)
> -{
> -       struct dw_dsi *dsi = host_to_dsi(host);
> -
> -       if (mdsi->lanes < 1 || mdsi->lanes > 4) {
> -               DRM_ERROR("dsi device params invalid\n");
> -               return -EINVAL;
> -       }
> -
> -       dsi->lanes = mdsi->lanes;
> -       dsi->format = mdsi->format;
> -       dsi->mode_flags = mdsi->mode_flags;
> -
> -       return 0;
> -}
> -
> -static int dsi_host_detach(struct mipi_dsi_host *host,
> -                          struct mipi_dsi_device *mdsi)
> -{
> -       /* do nothing */
> -       return 0;
> -}
> -
> -static const struct mipi_dsi_host_ops dsi_host_ops = {
> -       .attach = dsi_host_attach,
> -       .detach = dsi_host_detach,
> -};
> -
> -static int dsi_host_init(struct device *dev, struct dw_dsi *dsi)
> -{
> -       struct mipi_dsi_host *host = &dsi->host;
> -       int ret;
> -
> -       host->dev = dev;
> -       host->ops = &dsi_host_ops;
> -       ret = mipi_dsi_host_register(host);
> -       if (ret) {
> -               DRM_ERROR("failed to register dsi host\n");
> -               return ret;
> -       }
> -
> -       return 0;
> -}
> -
>  static int dsi_bridge_init(struct drm_device *dev, struct dw_dsi *dsi)
>  {
>         struct drm_encoder *encoder = &dsi->encoder;
> @@ -796,10 +752,6 @@ static int dsi_bind(struct device *dev, struct device *master, void *data)
>         if (ret)
>                 return ret;
>
> -       ret = dsi_host_init(dev, dsi);
> -       if (ret)
> -               return ret;
> -
>         ret = dsi_bridge_init(drm_dev, dsi);
>         if (ret)
>                 return ret;
> @@ -817,13 +769,22 @@ static const struct component_ops dsi_ops = {
>         .unbind = dsi_unbind,
>  };
>
> -static int dsi_parse_dt(struct platform_device *pdev, struct dw_dsi *dsi)
> +static int dsi_host_attach(struct mipi_dsi_host *host,
> +                          struct mipi_dsi_device *mdsi)
>  {
> -       struct dsi_hw_ctx *ctx = dsi->ctx;
> -       struct device_node *np = pdev->dev.of_node;
> -       struct resource *res;
> +       struct dw_dsi *dsi = host_to_dsi(host);
> +       struct device_node *np = dsi->dev->of_node;
>         int ret;
>
> +       if (mdsi->lanes < 1 || mdsi->lanes > 4) {
> +               DRM_ERROR("dsi device params invalid\n");
> +               return -EINVAL;
> +       }
> +
> +       dsi->lanes = mdsi->lanes;
> +       dsi->format = mdsi->format;
> +       dsi->mode_flags = mdsi->mode_flags;
> +
>         /*
>          * Get the endpoint node. In our case, dsi has one output port1
>          * to which the external HDMI bridge is connected.
> @@ -832,6 +793,42 @@ static int dsi_parse_dt(struct platform_device *pdev, struct dw_dsi *dsi)
>         if (ret)
>                 return ret;
>
> +       return component_add(dsi->dev, &dsi_ops);
> +}
> +
> +static int dsi_host_detach(struct mipi_dsi_host *host,
> +                          struct mipi_dsi_device *mdsi)
> +{
> +       /* do nothing */
> +       return 0;
> +}
> +
> +static const struct mipi_dsi_host_ops dsi_host_ops = {
> +       .attach = dsi_host_attach,
> +       .detach = dsi_host_detach,
> +};
> +
> +static int dsi_host_init(struct device *dev, struct dw_dsi *dsi)
> +{
> +       struct mipi_dsi_host *host = &dsi->host;
> +       int ret;
> +
> +       host->dev = dev;
> +       host->ops = &dsi_host_ops;
> +       ret = mipi_dsi_host_register(host);
> +       if (ret) {
> +               DRM_ERROR("failed to register dsi host\n");
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +static int dsi_parse_dt(struct platform_device *pdev, struct dw_dsi *dsi)
> +{
> +       struct dsi_hw_ctx *ctx = dsi->ctx;
> +       struct resource *res;
> +
>         ctx->pclk = devm_clk_get(&pdev->dev, "pclk");
>         if (IS_ERR(ctx->pclk)) {
>                 DRM_ERROR("failed to get pclk clock\n");
> @@ -862,15 +859,19 @@ static int dsi_probe(struct platform_device *pdev)
>         }
>         dsi = &data->dsi;
>         ctx = &data->ctx;
> +       dsi->dev = &pdev->dev;
>         dsi->ctx = ctx;
>
>         ret = dsi_parse_dt(pdev, dsi);
>         if (ret)
>                 return ret;
>
> -       platform_set_drvdata(pdev, data);
> +       ret = dsi_host_init(&pdev->dev, dsi);
> +       if (ret)
> +               return ret;
>
> -       return component_add(&pdev->dev, &dsi_ops);
> +       platform_set_drvdata(pdev, data);
> +       return 0;
>  }
>
>  static int dsi_remove(struct platform_device *pdev)
> --
> 2.17.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC][PATCH] drm: kirin: Fix dsi probe/attach logic

[Andrzej Hajda]

On 29.08.2019 19:39, Rob Clark wrote:
> On Wed, Aug 28, 2019 at 11:06 PM John Stultz <john.stultz@linaro.org> wrote:
>> Since commit 83f35bc3a852 ("drm/bridge: adv7511: Attach to DSI
>> host at probe time") landed in -next the HiKey board would fail
>> to boot, looping:
> No, please revert 83f35bc3a852.. that is going in the *complete* wrong
> direction.  We actually should be moving panels to not require dsi
> host until attach time, similar to how bridges work, not the other way
> around.


Devices of panels and bridges controlled via DSI will not appear at all
if DSI host is not created.

So this is the only direction!!!


>
> The problem is that, when dealing with bootloader enabled display, we
> need to be really careful not to touch the hardware until the display
> driver knows the bridge/panel is present.  If the bridge/panel probes
> after the display driver, we could end up killing scanout
> (efifb/simplefb).. if the bridge/panel is missing some dependency and
> never probes, it is rather unpleasant to be stuck trying to debug what
> went wrong with no display.


It has nothing to do with touching hardware, you can always (I hope)
postpone it till all components are present.

But it is just requirement of device/driver model in Linux Kernel.


>
> Sorry I didn't notice that adv7511 patch before it landed, but the
> right thing to do now is to revert it.


The 1st version of the patch was posted at the end of April and final
version was queued 1st July, so it was quite long time for discussions
and tests.

Reverting it now seems quite late, especially if the patch does right
thing and there is already proper fix for one encoder (kirin), moreover
revert will break another platforms.

Of course it seems you have different opinion what is the right thing in
this case, so if you convince us that your approach is better one can
revert the patch.


Regards

Andrzej



>
> BR,
> -R
>
>>   adv7511 2-0039: failed to find dsi host
>>
>> messages over and over. Andrzej Hajda suggested this is due to a
>> circular dependency issue, and that the adv7511 change is
>> correcting the improper order used earlier.
>>
>> Unfortunately this means the DSI drivers that use adv7511 need
>> to also need to be updated to use the proper ordering to
>> continue to work.
>>
>> This patch tries to reorder the initialization to register the
>> dsi_host first, and then call component_add via dsi_host_attach,
>> instead of doing that at probe time.
>>
>> This seems to resolve the issue with the HiKey board.
>>
>> Cc: Andrzej Hajda <a.hajda@samsung.com>
>> Cc: Matt Redfearn <matt.redfearn@thinci.com>
>> Cc: Xinliang Liu <z.liuxinliang@hisilicon.com>
>> Cc: Rongrong Zou <zourongrong@gmail.com>
>> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
>> Cc: Neil Armstrong <narmstrong@baylibre.com>
>> Cc: Jonas Karlman <jonas@kwiboo.se>
>> Cc: Jernej Skrabec <jernej.skrabec@siol.net>
>> Cc: Thierry Reding <thierry.reding@gmail.com>
>> Cc: David Airlie <airlied@linux.ie>,
>> Cc: Sean Paul <seanpaul@chromium.org>
>> Cc: Sam Ravnborg <sam@ravnborg.org>
>> Cc: "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>
>> Fixes: 83f35bc3a852 ("drm/bridge: adv7511: Attach to DSI host at probe time")
>> Signed-off-by: John Stultz <john.stultz@linaro.org>
>> ---
>> Note: I'm really not super familiar with the DSI code here,
>> and am mostly just trying to refactor the existing code in a
>> similar fashion to the suggested dw-mipi-dsi-rockchip.c
>> implementation. Careful review would be greatly appreciated!
>>
>> Also there is an outstanding regression on the db410c since it
>> similarly uses the adv7511 and probably needs a similar rework.
>> ---
>>  drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c | 111 ++++++++++---------
>>  1 file changed, 56 insertions(+), 55 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c b/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c
>> index 5bf8138941de..696cee1a1219 100644
>> --- a/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c
>> +++ b/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c
>> @@ -79,6 +79,7 @@ struct dsi_hw_ctx {
>>  };
>>
>>  struct dw_dsi {
>> +       struct device *dev;
>>         struct drm_encoder encoder;
>>         struct drm_bridge *bridge;
>>         struct mipi_dsi_host host;
>> @@ -724,51 +725,6 @@ static int dw_drm_encoder_init(struct device *dev,
>>         return 0;
>>  }
>>
>> -static int dsi_host_attach(struct mipi_dsi_host *host,
>> -                          struct mipi_dsi_device *mdsi)
>> -{
>> -       struct dw_dsi *dsi = host_to_dsi(host);
>> -
>> -       if (mdsi->lanes < 1 || mdsi->lanes > 4) {
>> -               DRM_ERROR("dsi device params invalid\n");
>> -               return -EINVAL;
>> -       }
>> -
>> -       dsi->lanes = mdsi->lanes;
>> -       dsi->format = mdsi->format;
>> -       dsi->mode_flags = mdsi->mode_flags;
>> -
>> -       return 0;
>> -}
>> -
>> -static int dsi_host_detach(struct mipi_dsi_host *host,
>> -                          struct mipi_dsi_device *mdsi)
>> -{
>> -       /* do nothing */
>> -       return 0;
>> -}
>> -
>> -static const struct mipi_dsi_host_ops dsi_host_ops = {
>> -       .attach = dsi_host_attach,
>> -       .detach = dsi_host_detach,
>> -};
>> -
>> -static int dsi_host_init(struct device *dev, struct dw_dsi *dsi)
>> -{
>> -       struct mipi_dsi_host *host = &dsi->host;
>> -       int ret;
>> -
>> -       host->dev = dev;
>> -       host->ops = &dsi_host_ops;
>> -       ret = mipi_dsi_host_register(host);
>> -       if (ret) {
>> -               DRM_ERROR("failed to register dsi host\n");
>> -               return ret;
>> -       }
>> -
>> -       return 0;
>> -}
>> -
>>  static int dsi_bridge_init(struct drm_device *dev, struct dw_dsi *dsi)
>>  {
>>         struct drm_encoder *encoder = &dsi->encoder;
>> @@ -796,10 +752,6 @@ static int dsi_bind(struct device *dev, struct device *master, void *data)
>>         if (ret)
>>                 return ret;
>>
>> -       ret = dsi_host_init(dev, dsi);
>> -       if (ret)
>> -               return ret;
>> -
>>         ret = dsi_bridge_init(drm_dev, dsi);
>>         if (ret)
>>                 return ret;
>> @@ -817,13 +769,22 @@ static const struct component_ops dsi_ops = {
>>         .unbind = dsi_unbind,
>>  };
>>
>> -static int dsi_parse_dt(struct platform_device *pdev, struct dw_dsi *dsi)
>> +static int dsi_host_attach(struct mipi_dsi_host *host,
>> +                          struct mipi_dsi_device *mdsi)
>>  {
>> -       struct dsi_hw_ctx *ctx = dsi->ctx;
>> -       struct device_node *np = pdev->dev.of_node;
>> -       struct resource *res;
>> +       struct dw_dsi *dsi = host_to_dsi(host);
>> +       struct device_node *np = dsi->dev->of_node;
>>         int ret;
>>
>> +       if (mdsi->lanes < 1 || mdsi->lanes > 4) {
>> +               DRM_ERROR("dsi device params invalid\n");
>> +               return -EINVAL;
>> +       }
>> +
>> +       dsi->lanes = mdsi->lanes;
>> +       dsi->format = mdsi->format;
>> +       dsi->mode_flags = mdsi->mode_flags;
>> +
>>         /*
>>          * Get the endpoint node. In our case, dsi has one output port1
>>          * to which the external HDMI bridge is connected.
>> @@ -832,6 +793,42 @@ static int dsi_parse_dt(struct platform_device *pdev, struct dw_dsi *dsi)
>>         if (ret)
>>                 return ret;
>>
>> +       return component_add(dsi->dev, &dsi_ops);
>> +}
>> +
>> +static int dsi_host_detach(struct mipi_dsi_host *host,
>> +                          struct mipi_dsi_device *mdsi)
>> +{
>> +       /* do nothing */
>> +       return 0;
>> +}
>> +
>> +static const struct mipi_dsi_host_ops dsi_host_ops = {
>> +       .attach = dsi_host_attach,
>> +       .detach = dsi_host_detach,
>> +};
>> +
>> +static int dsi_host_init(struct device *dev, struct dw_dsi *dsi)
>> +{
>> +       struct mipi_dsi_host *host = &dsi->host;
>> +       int ret;
>> +
>> +       host->dev = dev;
>> +       host->ops = &dsi_host_ops;
>> +       ret = mipi_dsi_host_register(host);
>> +       if (ret) {
>> +               DRM_ERROR("failed to register dsi host\n");
>> +               return ret;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static int dsi_parse_dt(struct platform_device *pdev, struct dw_dsi *dsi)
>> +{
>> +       struct dsi_hw_ctx *ctx = dsi->ctx;
>> +       struct resource *res;
>> +
>>         ctx->pclk = devm_clk_get(&pdev->dev, "pclk");
>>         if (IS_ERR(ctx->pclk)) {
>>                 DRM_ERROR("failed to get pclk clock\n");
>> @@ -862,15 +859,19 @@ static int dsi_probe(struct platform_device *pdev)
>>         }
>>         dsi = &data->dsi;
>>         ctx = &data->ctx;
>> +       dsi->dev = &pdev->dev;
>>         dsi->ctx = ctx;
>>
>>         ret = dsi_parse_dt(pdev, dsi);
>>         if (ret)
>>                 return ret;
>>
>> -       platform_set_drvdata(pdev, data);
>> +       ret = dsi_host_init(&pdev->dev, dsi);
>> +       if (ret)
>> +               return ret;
>>
>> -       return component_add(&pdev->dev, &dsi_ops);
>> +       platform_set_drvdata(pdev, data);
>> +       return 0;
>>  }
>>
>>  static int dsi_remove(struct platform_device *pdev)
>> --
>> 2.17.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC][PATCH] drm: kirin: Fix dsi probe/attach logic

[Rob Clark]

On Thu, Aug 29, 2019 at 11:52 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
>
> On 29.08.2019 19:39, Rob Clark wrote:
> > On Wed, Aug 28, 2019 at 11:06 PM John Stultz <john.stultz@linaro.org> wrote:
> >> Since commit 83f35bc3a852 ("drm/bridge: adv7511: Attach to DSI
> >> host at probe time") landed in -next the HiKey board would fail
> >> to boot, looping:
> > No, please revert 83f35bc3a852.. that is going in the *complete* wrong
> > direction.  We actually should be moving panels to not require dsi
> > host until attach time, similar to how bridges work, not the other way
> > around.
>
>
> Devices of panels and bridges controlled via DSI will not appear at all
> if DSI host is not created.
>
> So this is the only direction!!!
>

I disagree, there is really no harm in the bridge probing if there is no dsi.

Furthermore, it seems that this change broke a few other drivers.

> >
> > The problem is that, when dealing with bootloader enabled display, we
> > need to be really careful not to touch the hardware until the display
> > driver knows the bridge/panel is present.  If the bridge/panel probes
> > after the display driver, we could end up killing scanout
> > (efifb/simplefb).. if the bridge/panel is missing some dependency and
> > never probes, it is rather unpleasant to be stuck trying to debug what
> > went wrong with no display.
>
>
> It has nothing to do with touching hardware, you can always (I hope)
> postpone it till all components are present.

Unfortunately this is harder than it sounds, since we need to read hw
revision registers for display and dsi blocks to know which hw
revision we are dealing with.

(Also, we need to avoid
drm_fb_helper_remove_conflicting_framebuffers() until we know we are
ready to go.)

We could possibly put more information in dt.  But the less we depend
on dt, the easier time we'll have adding support for ACPI boot on the
windows arm laptops.

> But it is just requirement of device/driver model in Linux Kernel.

yes and no.. the way the existing bridges work with a bridge->attach()
step seems fairly pragmatic to me.

>
> >
> > Sorry I didn't notice that adv7511 patch before it landed, but the
> > right thing to do now is to revert it.
>
>
> The 1st version of the patch was posted at the end of April and final
> version was queued 1st July, so it was quite long time for discussions
> and tests.

sorry I didn't notice the patch sooner, most of my bandwidth goes to mesa.

> Reverting it now seems quite late, especially if the patch does right
> thing and there is already proper fix for one encoder (kirin), moreover
> revert will break another platforms.

kirin isn't the only other user of adv75xx.. at least it is my
understanding that this broke db410c as well.

> Of course it seems you have different opinion what is the right thing in
> this case, so if you convince us that your approach is better one can
> revert the patch.

I guess my strongest / most immediate opinion is to not break other
existing adv75xx bridge users.

Beyond that, I found doing mipi_dsi_attach() in bridge->attach() was
quite convenient to get display handover from efifb work.  And that
was (previously) the way most of the bridges worked.

But maybe there is another way.. perhaps somehow the bridges/panels
could be added as sub-components using the component framework, to
prevent the toplevel drm device from probing until they are ready?
We'd still have the problem that the dsi component wants to be able to
read hw revision before registering dsi host.. but I suppose if CCF
exported a way that we could query whether the interface clk was
already enabled, we could have the clk enable/disable cycle that would
break efifb.

BR,
-R

Re: [RFC][PATCH] drm: kirin: Fix dsi probe/attach logic

[Andrzej Hajda]

On 30.08.2019 19:00, Rob Clark wrote:
> On Thu, Aug 29, 2019 at 11:52 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
>> On 29.08.2019 19:39, Rob Clark wrote:
>>> On Wed, Aug 28, 2019 at 11:06 PM John Stultz <john.stultz@linaro.org> wrote:
>>>> Since commit 83f35bc3a852 ("drm/bridge: adv7511: Attach to DSI
>>>> host at probe time") landed in -next the HiKey board would fail
>>>> to boot, looping:
>>> No, please revert 83f35bc3a852.. that is going in the *complete* wrong
>>> direction.  We actually should be moving panels to not require dsi
>>> host until attach time, similar to how bridges work, not the other way
>>> around.
>>
>> Devices of panels and bridges controlled via DSI will not appear at all
>> if DSI host is not created.
>>
>> So this is the only direction!!!
>>
> I disagree, there is really no harm in the bridge probing if there is no dsi.
>
> Furthermore, it seems that this change broke a few other drivers.


If the bridge/panel is controlled via dsi, it will not be probed at all
if DSI host/bus is not created, so it will not work at all. Upstream
device will wait forever for downstream drm_(panel|bridge).

So IMO we just do not have choice here.

If you know better alternative let us know, otherwise we should proceed
with "drm: kirin: Fix dsi probe/attach logic" patch.


>
>>> The problem is that, when dealing with bootloader enabled display, we
>>> need to be really careful not to touch the hardware until the display
>>> driver knows the bridge/panel is present.  If the bridge/panel probes
>>> after the display driver, we could end up killing scanout
>>> (efifb/simplefb).. if the bridge/panel is missing some dependency and
>>> never probes, it is rather unpleasant to be stuck trying to debug what
>>> went wrong with no display.
>>
>> It has nothing to do with touching hardware, you can always (I hope)
>> postpone it till all components are present.
> Unfortunately this is harder than it sounds, since we need to read hw
> revision registers for display and dsi blocks to know which hw
> revision we are dealing with.
>
> (Also, we need to avoid
> drm_fb_helper_remove_conflicting_framebuffers() until we know we are
> ready to go.)
>
> We could possibly put more information in dt.  But the less we depend
> on dt, the easier time we'll have adding support for ACPI boot on the
> windows arm laptops.
>
>> But it is just requirement of device/driver model in Linux Kernel.
> yes and no.. the way the existing bridges work with a bridge->attach()
> step seems fairly pragmatic to me.
>
>>> Sorry I didn't notice that adv7511 patch before it landed, but the
>>> right thing to do now is to revert it.
>>
>> The 1st version of the patch was posted at the end of April and final
>> version was queued 1st July, so it was quite long time for discussions
>> and tests.
> sorry I didn't notice the patch sooner, most of my bandwidth goes to mesa.
>
>> Reverting it now seems quite late, especially if the patch does right
>> thing and there is already proper fix for one encoder (kirin), moreover
>> revert will break another platforms.
> kirin isn't the only other user of adv75xx.. at least it is my
> understanding that this broke db410c as well.
>
>> Of course it seems you have different opinion what is the right thing in
>> this case, so if you convince us that your approach is better one can
>> revert the patch.
> I guess my strongest / most immediate opinion is to not break other
> existing adv75xx bridge users.


It is pity that breakage happened, and next time we should be more
strict about testing other platforms, before patch acceptance.

But reverting it now will break also platform which depend on it.

Anyway the old order was incorrect and prevented other users from adv*
driver usage, so it should be fixed anyway.


> Beyond that, I found doing mipi_dsi_attach() in bridge->attach() was
> quite convenient to get display handover from efifb work.  And that
> was (previously) the way most of the bridges worked.
>
> But maybe there is another way.. perhaps somehow the bridges/panels
> could be added as sub-components using the component framework, to
> prevent the toplevel drm device from probing until they are ready?


If you mean 'probe' as in initialization from component_master::bind
callback with this patch it still works, DSI-HOST calls component_add
after drm_bridge is found.


> We'd still have the problem that the dsi component wants to be able to
> read hw revision before registering dsi host.. but I suppose if CCF
> exported a way that we could query whether the interface clk was
> already enabled, we could have the clk enable/disable cycle that would
> break efifb.


I am not familiar with efifb, if you describe the issue you have in more
detail we can try to find some solution together.


Regards

Andrzej


>
> BR,
> -R
>
>

Re: [RFC][PATCH] drm: kirin: Fix dsi probe/attach logic

[Rob Clark]

On Mon, Sep 2, 2019 at 6:22 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
>
> On 30.08.2019 19:00, Rob Clark wrote:
> > On Thu, Aug 29, 2019 at 11:52 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
> >> On 29.08.2019 19:39, Rob Clark wrote:
> >>> On Wed, Aug 28, 2019 at 11:06 PM John Stultz <john.stultz@linaro.org> wrote:
> >>>> Since commit 83f35bc3a852 ("drm/bridge: adv7511: Attach to DSI
> >>>> host at probe time") landed in -next the HiKey board would fail
> >>>> to boot, looping:
> >>> No, please revert 83f35bc3a852.. that is going in the *complete* wrong
> >>> direction.  We actually should be moving panels to not require dsi
> >>> host until attach time, similar to how bridges work, not the other way
> >>> around.
> >>
> >> Devices of panels and bridges controlled via DSI will not appear at all
> >> if DSI host is not created.
> >>
> >> So this is the only direction!!!
> >>
> > I disagree, there is really no harm in the bridge probing if there is no dsi.
> >
> > Furthermore, it seems that this change broke a few other drivers.
>
>
> If the bridge/panel is controlled via dsi, it will not be probed at all
> if DSI host/bus is not created, so it will not work at all. Upstream
> device will wait forever for downstream drm_(panel|bridge).
>
> So IMO we just do not have choice here.
>
> If you know better alternative let us know, otherwise we should proceed
> with "drm: kirin: Fix dsi probe/attach logic" patch.
>
>
> >
> >>> The problem is that, when dealing with bootloader enabled display, we
> >>> need to be really careful not to touch the hardware until the display
> >>> driver knows the bridge/panel is present.  If the bridge/panel probes
> >>> after the display driver, we could end up killing scanout
> >>> (efifb/simplefb).. if the bridge/panel is missing some dependency and
> >>> never probes, it is rather unpleasant to be stuck trying to debug what
> >>> went wrong with no display.
> >>
> >> It has nothing to do with touching hardware, you can always (I hope)
> >> postpone it till all components are present.
> > Unfortunately this is harder than it sounds, since we need to read hw
> > revision registers for display and dsi blocks to know which hw
> > revision we are dealing with.
> >
> > (Also, we need to avoid
> > drm_fb_helper_remove_conflicting_framebuffers() until we know we are
> > ready to go.)
> >
> > We could possibly put more information in dt.  But the less we depend
> > on dt, the easier time we'll have adding support for ACPI boot on the
> > windows arm laptops.
> >
> >> But it is just requirement of device/driver model in Linux Kernel.
> > yes and no.. the way the existing bridges work with a bridge->attach()
> > step seems fairly pragmatic to me.
> >
> >>> Sorry I didn't notice that adv7511 patch before it landed, but the
> >>> right thing to do now is to revert it.
> >>
> >> The 1st version of the patch was posted at the end of April and final
> >> version was queued 1st July, so it was quite long time for discussions
> >> and tests.
> > sorry I didn't notice the patch sooner, most of my bandwidth goes to mesa.
> >
> >> Reverting it now seems quite late, especially if the patch does right
> >> thing and there is already proper fix for one encoder (kirin), moreover
> >> revert will break another platforms.
> > kirin isn't the only other user of adv75xx.. at least it is my
> > understanding that this broke db410c as well.
> >
> >> Of course it seems you have different opinion what is the right thing in
> >> this case, so if you convince us that your approach is better one can
> >> revert the patch.
> > I guess my strongest / most immediate opinion is to not break other
> > existing adv75xx bridge users.
>
>
> It is pity that breakage happened, and next time we should be more
> strict about testing other platforms, before patch acceptance.
>
> But reverting it now will break also platform which depend on it.
>
> Anyway the old order was incorrect and prevented other users from adv*
> driver usage, so it should be fixed anyway.
>
>
> > Beyond that, I found doing mipi_dsi_attach() in bridge->attach() was
> > quite convenient to get display handover from efifb work.  And that
> > was (previously) the way most of the bridges worked.
> >
> > But maybe there is another way.. perhaps somehow the bridges/panels
> > could be added as sub-components using the component framework, to
> > prevent the toplevel drm device from probing until they are ready?
>
>
> If you mean 'probe' as in initialization from component_master::bind
> callback with this patch it still works, DSI-HOST calls component_add
> after drm_bridge is found.
>
>
> > We'd still have the problem that the dsi component wants to be able to
> > read hw revision before registering dsi host.. but I suppose if CCF
> > exported a way that we could query whether the interface clk was
> > already enabled, we could have the clk enable/disable cycle that would
> > break efifb.
>
>
> I am not familiar with efifb, if you describe the issue you have in more
> detail we can try to find some solution together.
>

You don't really need to know too much about efifb.. the story would
be the same with simplefb, the only difference is that the scanout
address/size/format come from EFI GOP instead of dt.

The main thing is we really want to avoid a runpm resume/suspend cycle
until the we have all the components of the display, including
bridge/panel.  If the display is enabled by bootloader, then the
initial resume will be no-op (turning on clks and power domains that
are already on), but the following suspend will actually turn things
off.  If this happens because, for example, the bridge driver didn't
get copied to initrd, or probe-deferred because it is missing one of
it's dependencies, the result will be a black screen with no way to
debug what went wrong.

I guess if we can rely on panel/bridge being probed indirectly from
mipi_dsi_host_register() (but before it returns, so dsi host can defer
if bridge/panel is missing) maybe that works out..

BR,
-R

Re: [RFC][PATCH] drm: kirin: Fix dsi probe/attach logic

[John Stultz]

On Mon, Sep 2, 2019 at 6:22 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
> On 30.08.2019 19:00, Rob Clark wrote:
> > On Thu, Aug 29, 2019 at 11:52 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
> >> Of course it seems you have different opinion what is the right thing in
> >> this case, so if you convince us that your approach is better one can
> >> revert the patch.
> > I guess my strongest / most immediate opinion is to not break other
> > existing adv75xx bridge users.
>
>
> It is pity that breakage happened, and next time we should be more
> strict about testing other platforms, before patch acceptance.
>
> But reverting it now will break also platform which depend on it.

I'm really of no opinion of which approach is better here, but I will
say that when a patch breaks previously working boards, that's a
regression and justifying that some other board is now enabled that
would be broken by the revert (of a patch that is not yet upstream)
isn't really a strong argument.

I'm happy to work with folks to try to fixup the kirin driver if this
patch really is the right approach, but we need someone to do the same
for the db410c, and I don't think its fair to just dump that work onto
folks under the threat of the board breaking.

thanks
-john

Re: [RFC][PATCH] drm: kirin: Fix dsi probe/attach logic

[Andrzej Hajda]

On 03.09.2019 18:18, John Stultz wrote:
> On Mon, Sep 2, 2019 at 6:22 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
>> On 30.08.2019 19:00, Rob Clark wrote:
>>> On Thu, Aug 29, 2019 at 11:52 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
>>>> Of course it seems you have different opinion what is the right thing in
>>>> this case, so if you convince us that your approach is better one can
>>>> revert the patch.
>>> I guess my strongest / most immediate opinion is to not break other
>>> existing adv75xx bridge users.
>>
>> It is pity that breakage happened, and next time we should be more
>> strict about testing other platforms, before patch acceptance.
>>
>> But reverting it now will break also platform which depend on it.
> I'm really of no opinion of which approach is better here, but I will
> say that when a patch breaks previously working boards, that's a
> regression and justifying that some other board is now enabled that
> would be broken by the revert (of a patch that is not yet upstream)
> isn't really a strong argument.
>
> I'm happy to work with folks to try to fixup the kirin driver if this
> patch really is the right approach, but we need someone to do the same
> for the db410c, and I don't think its fair to just dump that work onto
> folks under the threat of the board breaking.


These drivers should be fixed anyway - assumption that
drm_bridge/drm_panel will be registered before the bus it is attached to
is just incorrect.

So instead of reverting, fixing and then re-applying the patch I have
gently proposed shorter path. If you prefer long path we can try to go
this way.

Matt, is the pure revert OK for you or is it possible to prepare some
workaround allowing cooperation with both approaches?


Regards

Andrzej


>
> thanks
> -john
>
>

Re: [RFC][PATCH] drm: kirin: Fix dsi probe/attach logic

[John Stultz]

On Wed, Sep 4, 2019 at 3:26 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
> On 03.09.2019 18:18, John Stultz wrote:
> > On Mon, Sep 2, 2019 at 6:22 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
> >> On 30.08.2019 19:00, Rob Clark wrote:
> >>> On Thu, Aug 29, 2019 at 11:52 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
> >>>> Of course it seems you have different opinion what is the right thing in
> >>>> this case, so if you convince us that your approach is better one can
> >>>> revert the patch.
> >>> I guess my strongest / most immediate opinion is to not break other
> >>> existing adv75xx bridge users.
> >>
> >> It is pity that breakage happened, and next time we should be more
> >> strict about testing other platforms, before patch acceptance.
> >>
> >> But reverting it now will break also platform which depend on it.
> > I'm really of no opinion of which approach is better here, but I will
> > say that when a patch breaks previously working boards, that's a
> > regression and justifying that some other board is now enabled that
> > would be broken by the revert (of a patch that is not yet upstream)
> > isn't really a strong argument.
> >
> > I'm happy to work with folks to try to fixup the kirin driver if this
> > patch really is the right approach, but we need someone to do the same
> > for the db410c, and I don't think its fair to just dump that work onto
> > folks under the threat of the board breaking.
>
>
> These drivers should be fixed anyway - assumption that
> drm_bridge/drm_panel will be registered before the bus it is attached to
> is just incorrect.
>
> So instead of reverting, fixing and then re-applying the patch I have
> gently proposed shorter path. If you prefer long path we can try to go
> this way.
>
> Matt, is the pure revert OK for you or is it possible to prepare some
> workaround allowing cooperation with both approaches?

Rob/Andrzej: What's the call here?

Should I resubmit the kirin fix for the adv7511 regression here?
Or do we revert the adv7511 patch? I believe db410c still needs a fix.

I'd just like to keep the HiKey board from breaking, so let me know so
I can get the fix submitted if needed.

thanks
-john

Re: [RFC][PATCH] drm: kirin: Fix dsi probe/attach logic

[Andrzej Hajda]

On 12.09.2019 04:38, John Stultz wrote:
> On Wed, Sep 4, 2019 at 3:26 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
>> On 03.09.2019 18:18, John Stultz wrote:
>>> On Mon, Sep 2, 2019 at 6:22 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
>>>> On 30.08.2019 19:00, Rob Clark wrote:
>>>>> On Thu, Aug 29, 2019 at 11:52 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
>>>>>> Of course it seems you have different opinion what is the right thing in
>>>>>> this case, so if you convince us that your approach is better one can
>>>>>> revert the patch.
>>>>> I guess my strongest / most immediate opinion is to not break other
>>>>> existing adv75xx bridge users.
>>>> It is pity that breakage happened, and next time we should be more
>>>> strict about testing other platforms, before patch acceptance.
>>>>
>>>> But reverting it now will break also platform which depend on it.
>>> I'm really of no opinion of which approach is better here, but I will
>>> say that when a patch breaks previously working boards, that's a
>>> regression and justifying that some other board is now enabled that
>>> would be broken by the revert (of a patch that is not yet upstream)
>>> isn't really a strong argument.
>>>
>>> I'm happy to work with folks to try to fixup the kirin driver if this
>>> patch really is the right approach, but we need someone to do the same
>>> for the db410c, and I don't think its fair to just dump that work onto
>>> folks under the threat of the board breaking.
>>
>> These drivers should be fixed anyway - assumption that
>> drm_bridge/drm_panel will be registered before the bus it is attached to
>> is just incorrect.
>>
>> So instead of reverting, fixing and then re-applying the patch I have
>> gently proposed shorter path. If you prefer long path we can try to go
>> this way.
>>
>> Matt, is the pure revert OK for you or is it possible to prepare some
>> workaround allowing cooperation with both approaches?
> Rob/Andrzej: What's the call here?
>
> Should I resubmit the kirin fix for the adv7511 regression here?
> Or do we revert the adv7511 patch? I believe db410c still needs a fix.
>
> I'd just like to keep the HiKey board from breaking, so let me know so
> I can get the fix submitted if needed.


Since there is no response from Matt, we can perform revert, since there
are no other ideas. I will apply it tomorrow, if there are no objections.

And for the future: I guess it is not possible to make adv work with old
and new approach, but simple workaround for adv could be added later:

if (source is msm or kirin)

    do_the_old_way

else

    do_correctly.


And remove it after fixing both dsi masters.


Regards

Andrzej


>
> thanks
> -john
>
>

Re: [RFC][PATCH] drm: kirin: Fix dsi probe/attach logic

[Matt Redfearn]

On 12/09/2019 14:21, Andrzej Hajda wrote:
> On 12.09.2019 04:38, John Stultz wrote:
>> On Wed, Sep 4, 2019 at 3:26 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
>>> On 03.09.2019 18:18, John Stultz wrote:
>>>> On Mon, Sep 2, 2019 at 6:22 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
>>>>> On 30.08.2019 19:00, Rob Clark wrote:
>>>>>> On Thu, Aug 29, 2019 at 11:52 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
>>>>>>> Of course it seems you have different opinion what is the right thing in
>>>>>>> this case, so if you convince us that your approach is better one can
>>>>>>> revert the patch.
>>>>>> I guess my strongest / most immediate opinion is to not break other
>>>>>> existing adv75xx bridge users.
>>>>> It is pity that breakage happened, and next time we should be more
>>>>> strict about testing other platforms, before patch acceptance.
>>>>>
>>>>> But reverting it now will break also platform which depend on it.
>>>> I'm really of no opinion of which approach is better here, but I will
>>>> say that when a patch breaks previously working boards, that's a
>>>> regression and justifying that some other board is now enabled that
>>>> would be broken by the revert (of a patch that is not yet upstream)
>>>> isn't really a strong argument.
>>>>
>>>> I'm happy to work with folks to try to fixup the kirin driver if this
>>>> patch really is the right approach, but we need someone to do the same
>>>> for the db410c, and I don't think its fair to just dump that work onto
>>>> folks under the threat of the board breaking.
>>>
>>> These drivers should be fixed anyway - assumption that
>>> drm_bridge/drm_panel will be registered before the bus it is attached to
>>> is just incorrect.
>>>
>>> So instead of reverting, fixing and then re-applying the patch I have
>>> gently proposed shorter path. If you prefer long path we can try to go
>>> this way.
>>>
>>> Matt, is the pure revert OK for you or is it possible to prepare some
>>> workaround allowing cooperation with both approaches?
>> Rob/Andrzej: What's the call here?
>>
>> Should I resubmit the kirin fix for the adv7511 regression here?
>> Or do we revert the adv7511 patch? I believe db410c still needs a fix.
>>
>> I'd just like to keep the HiKey board from breaking, so let me know so
>> I can get the fix submitted if needed.
> 
> 
> Since there is no response from Matt, we can perform revert, since there
> are no other ideas. I will apply it tomorrow, if there are no objections.

Hi,

Sorry - yeah I think reverting is probably best at this point to avoid 
breaking in-tree platforms.
It's a shame though that there is a built-in incompatibility within the 
tree between drivers. The way that the generic Synopsys DSI host driver 
probes is currently incompatible with the ADV7533 (hence the patch), 
other DSI host drivers are now compatible with the ADV7533 but break 
when we change it to probe in a similar way to panel drivers.

> 
> And for the future: I guess it is not possible to make adv work with old
> and new approach, but simple workaround for adv could be added later:
> 
> if (source is msm or kirin)
> 
>      do_the_old_way
> 
> else
> 
>      do_correctly.

Maybe this would work, but how do we know that the list "msm or kirin" 
is exhaustive to cope with all platforms? It seems to me the built in 
incompatibility between DSI hosts needs to be resolved really rather 
than trying to work around it in the ADV7533 driver (and any other DSI 
bus device that falls into this trap).

Anyway, my platform is out of tree so better to revert my patch and keep 
the in-tree platforms working.

Thanks everyone.
Matt

> 
> 
> And remove it after fixing both dsi masters.
> 
> 
> Regards
> 
> Andrzej
> 
> 
>>
>> thanks
>> -john
>>
>>
> 

Re: [RFC][PATCH] drm: kirin: Fix dsi probe/attach logic

[Andrzej Hajda]

On 12.09.2019 16:18, Matt Redfearn wrote:
>
> On 12/09/2019 14:21, Andrzej Hajda wrote:
>> On 12.09.2019 04:38, John Stultz wrote:
>>> On Wed, Sep 4, 2019 at 3:26 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
>>>> On 03.09.2019 18:18, John Stultz wrote:
>>>>> On Mon, Sep 2, 2019 at 6:22 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
>>>>>> On 30.08.2019 19:00, Rob Clark wrote:
>>>>>>> On Thu, Aug 29, 2019 at 11:52 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
>>>>>>>> Of course it seems you have different opinion what is the right thing in
>>>>>>>> this case, so if you convince us that your approach is better one can
>>>>>>>> revert the patch.
>>>>>>> I guess my strongest / most immediate opinion is to not break other
>>>>>>> existing adv75xx bridge users.
>>>>>> It is pity that breakage happened, and next time we should be more
>>>>>> strict about testing other platforms, before patch acceptance.
>>>>>>
>>>>>> But reverting it now will break also platform which depend on it.
>>>>> I'm really of no opinion of which approach is better here, but I will
>>>>> say that when a patch breaks previously working boards, that's a
>>>>> regression and justifying that some other board is now enabled that
>>>>> would be broken by the revert (of a patch that is not yet upstream)
>>>>> isn't really a strong argument.
>>>>>
>>>>> I'm happy to work with folks to try to fixup the kirin driver if this
>>>>> patch really is the right approach, but we need someone to do the same
>>>>> for the db410c, and I don't think its fair to just dump that work onto
>>>>> folks under the threat of the board breaking.
>>>> These drivers should be fixed anyway - assumption that
>>>> drm_bridge/drm_panel will be registered before the bus it is attached to
>>>> is just incorrect.
>>>>
>>>> So instead of reverting, fixing and then re-applying the patch I have
>>>> gently proposed shorter path. If you prefer long path we can try to go
>>>> this way.
>>>>
>>>> Matt, is the pure revert OK for you or is it possible to prepare some
>>>> workaround allowing cooperation with both approaches?
>>> Rob/Andrzej: What's the call here?
>>>
>>> Should I resubmit the kirin fix for the adv7511 regression here?
>>> Or do we revert the adv7511 patch? I believe db410c still needs a fix.
>>>
>>> I'd just like to keep the HiKey board from breaking, so let me know so
>>> I can get the fix submitted if needed.
>>
>> Since there is no response from Matt, we can perform revert, since there
>> are no other ideas. I will apply it tomorrow, if there are no objections.
> Hi,
>
> Sorry - yeah I think reverting is probably best at this point to avoid 
> breaking in-tree platforms.
> It's a shame though that there is a built-in incompatibility within the 
> tree between drivers.


Quite common in development - some issues becomes visible after some time.

> The way that the generic Synopsys DSI host driver 
> probes is currently incompatible with the ADV7533 (hence the patch), 
> other DSI host drivers are now compatible with the ADV7533 but break 
> when we change it to probe in a similar way to panel drivers.


1. The behavior should be consistent between all hosts/device drivers.

2. DSI controlled devices can expose drm objects (drm_bridge/drm_panel)
only when they can probe, ie DSI bus they sit on must be created 1st.

1 and 2 enforces policy that all host drivers should 1st create control
bus (DSI in this case) then look for higher level objects
(drm_bridge/drm_panel).

As a consequence all bridges and panels should 1st gather the resources
they depends on, and then they can expose higher level objects.


>
>> And for the future: I guess it is not possible to make adv work with old
>> and new approach, but simple workaround for adv could be added later:
>>
>> if (source is msm or kirin)
>>
>>      do_the_old_way
>>
>> else
>>
>>      do_correctly.
> Maybe this would work, but how do we know that the list "msm or kirin" 
> is exhaustive to cope with all platforms?


By checking dts/config files.


> It seems to me the built in 
> incompatibility between DSI hosts needs to be resolved really rather 
> than trying to work around it in the ADV7533 driver (and any other DSI 
> bus device that falls into this trap).


If you know how, please describe. Atm the only real solution I see is
explicit workaround to allow new adv7511, then fixing controllers,
together with workaround-removal.

OK, it could be possible to change msm, kirin and adv in one patch,
however I am not sure if this is the best solution.


Regards

Andrzej


>
> Anyway, my platform is out of tree so better to revert my patch and keep 
> the in-tree platforms working.
>
> Thanks everyone.
> Matt
>
>>
>> And remove it after fixing both dsi masters.
>>
>>
>> Regards
>>
>> Andrzej
>>
>>
>>> thanks
>>> -john
>>>
>>>

Re: [RFC][PATCH] drm: kirin: Fix dsi probe/attach logic

[John Stultz]

On Fri, Sep 13, 2019 at 1:47 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
> On 12.09.2019 16:18, Matt Redfearn wrote:
> > On 12/09/2019 14:21, Andrzej Hajda wrote:
> >> On 12.09.2019 04:38, John Stultz wrote:
> >>> Should I resubmit the kirin fix for the adv7511 regression here?
> >>> Or do we revert the adv7511 patch? I believe db410c still needs a fix.
> >>>
> >>> I'd just like to keep the HiKey board from breaking, so let me know so
> >>> I can get the fix submitted if needed.
> >>
> >> Since there is no response from Matt, we can perform revert, since there
> >> are no other ideas. I will apply it tomorrow, if there are no objections.
> > Hi,
> >
> > Sorry - yeah I think reverting is probably best at this point to avoid
> > breaking in-tree platforms.
> > It's a shame though that there is a built-in incompatibility within the
> > tree between drivers.
>
>
> Quite common in development - some issues becomes visible after some time.
>
> > The way that the generic Synopsys DSI host driver
> > probes is currently incompatible with the ADV7533 (hence the patch),
> > other DSI host drivers are now compatible with the ADV7533 but break
> > when we change it to probe in a similar way to panel drivers.
>
>
> 1. The behavior should be consistent between all hosts/device drivers.
>

Yea, I worry about this as well, as I know there is also the lt9611
bridge chip driver pending for the db845c which works against the
current msm dsi code. So changing the msm driver for the adv7533 may
break other drivers (in the case of lt9611, out of tree - so wouldn't
be a regression, but there may be others) written against the current
expectations.


> 2. DSI controlled devices can expose drm objects (drm_bridge/drm_panel)
> only when they can probe, ie DSI bus they sit on must be created 1st.
>
> 1 and 2 enforces policy that all host drivers should 1st create control
> bus (DSI in this case) then look for higher level objects
> (drm_bridge/drm_panel).
>
> As a consequence all bridges and panels should 1st gather the resources
> they depends on, and then they can expose higher level objects.
>
>
> >
> >> And for the future: I guess it is not possible to make adv work with old
> >> and new approach, but simple workaround for adv could be added later:
> >>
> >> if (source is msm or kirin)
> >>
> >>      do_the_old_way
> >>
> >> else
> >>
> >>      do_correctly.
> > Maybe this would work, but how do we know that the list "msm or kirin"
> > is exhaustive to cope with all platforms?
>
>
> By checking dts/config files.
>

A temporary clearly-marked-deprecated config option might also a
reasonable approach while we transition drivers over?

> > It seems to me the built in
> > incompatibility between DSI hosts needs to be resolved really rather
> > than trying to work around it in the ADV7533 driver (and any other DSI
> > bus device that falls into this trap).
>
>
> If you know how, please describe. Atm the only real solution I see is
> explicit workaround to allow new adv7511, then fixing controllers,
> together with workaround-removal.
>
> OK, it could be possible to change msm, kirin and adv in one patch,
> however I am not sure if this is the best solution.

Matt: I'm happy to help you test and validate things. Let me know how
I can help!

thanks
-john

Re: [RFC][PATCH] drm: kirin: Fix dsi probe/attach logic

[Rob Clark]

On Wed, Sep 11, 2019 at 7:39 PM John Stultz <john.stultz@linaro.org> wrote:
>
> On Wed, Sep 4, 2019 at 3:26 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
> > On 03.09.2019 18:18, John Stultz wrote:
> > > On Mon, Sep 2, 2019 at 6:22 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
> > >> On 30.08.2019 19:00, Rob Clark wrote:
> > >>> On Thu, Aug 29, 2019 at 11:52 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
> > >>>> Of course it seems you have different opinion what is the right thing in
> > >>>> this case, so if you convince us that your approach is better one can
> > >>>> revert the patch.
> > >>> I guess my strongest / most immediate opinion is to not break other
> > >>> existing adv75xx bridge users.
> > >>
> > >> It is pity that breakage happened, and next time we should be more
> > >> strict about testing other platforms, before patch acceptance.
> > >>
> > >> But reverting it now will break also platform which depend on it.
> > > I'm really of no opinion of which approach is better here, but I will
> > > say that when a patch breaks previously working boards, that's a
> > > regression and justifying that some other board is now enabled that
> > > would be broken by the revert (of a patch that is not yet upstream)
> > > isn't really a strong argument.
> > >
> > > I'm happy to work with folks to try to fixup the kirin driver if this
> > > patch really is the right approach, but we need someone to do the same
> > > for the db410c, and I don't think its fair to just dump that work onto
> > > folks under the threat of the board breaking.
> >
> >
> > These drivers should be fixed anyway - assumption that
> > drm_bridge/drm_panel will be registered before the bus it is attached to
> > is just incorrect.
> >
> > So instead of reverting, fixing and then re-applying the patch I have
> > gently proposed shorter path. If you prefer long path we can try to go
> > this way.
> >
> > Matt, is the pure revert OK for you or is it possible to prepare some
> > workaround allowing cooperation with both approaches?
>
> Rob/Andrzej: What's the call here?
>
> Should I resubmit the kirin fix for the adv7511 regression here?
> Or do we revert the adv7511 patch? I believe db410c still needs a fix.
>
> I'd just like to keep the HiKey board from breaking, so let me know so
> I can get the fix submitted if needed.
>

Does the kirin fix break things if the adv7511 patch is reverted?  If
not, I'd submit it anyways.

Hopefully by next weekend I'll be finished with my move and unpacked
enough to be able to setup db410c + monitor to start working on fixing
db410c.  So perhaps one option would be to wait a week, and see if I
can come up with something small enough to land as a post-merge-window
fix, with the fallback being to revert and try again next merge
window?

BR,
-R