[PATCH v2 0/3] drm: msm+ti-sn65dsi86 support for NO_CONNECTOR

[PATCH v2 0/3] drm: msm+ti-sn65dsi86 support for NO_CONNECTOR

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

Respin of https://www.spinics.net/lists/linux-arm-msm/msg92182.html with
the remaining 3 patches that are not yet merged.

At the end of this series, but drm/msm and ti-sn65dsi86 work in both
combinations, so the two bridge patches can be merged indepdendently of
the msm/dsi patch.

The last patch has some conficts with https://www.spinics.net/lists/linux-arm-msm/msg93731.html
but I already have a rebased variant of it depending on which order
patches land.

Rob Clark (3):
  drm/msm/dsi: Support NO_CONNECTOR bridges
  drm/bridge: ti-sn65dsi86: Implement bridge->mode_valid()
  drm/bridge: ti-sn65dsi86: Add NO_CONNECTOR support

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 64 ++++++++++++++++++---------
 drivers/gpu/drm/msm/Kconfig           |  2 +
 drivers/gpu/drm/msm/dsi/dsi_manager.c | 50 +++++++++++++++------
 3 files changed, 81 insertions(+), 35 deletions(-)

-- 
2.31.1

[PATCH v2 1/3] drm/msm/dsi: Support NO_CONNECTOR bridges

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

For now, since we have a mix of bridges which support this flag, which
which do *not* support this flag, or work both ways, try it once with
NO_CONNECTOR and then fall back to the old way if that doesn't work.
Eventually we can drop the fallback path.

v2: Add missing drm_connector_attach_encoder() so display actually comes
    up when the bridge properly handles the NO_CONNECTOR flag

Signed-off-by: Rob Clark <robdclark@chromium.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/msm/Kconfig           |  2 ++
 drivers/gpu/drm/msm/dsi/dsi_manager.c | 50 ++++++++++++++++++++-------
 2 files changed, 39 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/msm/Kconfig b/drivers/gpu/drm/msm/Kconfig
index e9c6af78b1d7..36e5ba3ccc28 100644
--- a/drivers/gpu/drm/msm/Kconfig
+++ b/drivers/gpu/drm/msm/Kconfig
@@ -14,6 +14,8 @@ config DRM_MSM
 	select REGULATOR
 	select DRM_KMS_HELPER
 	select DRM_PANEL
+	select DRM_BRIDGE
+	select DRM_PANEL_BRIDGE
 	select DRM_SCHED
 	select SHMEM
 	select TMPFS
diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index c41d39f5b7cf..e25877073d31 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -3,6 +3,8 @@
  * Copyright (c) 2015, The Linux Foundation. All rights reserved.
  */
 
+#include "drm/drm_bridge_connector.h"
+
 #include "msm_kms.h"
 #include "dsi.h"
 
@@ -688,10 +690,10 @@ struct drm_connector *msm_dsi_manager_ext_bridge_init(u8 id)
 {
 	struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
 	struct drm_device *dev = msm_dsi->dev;
+	struct drm_connector *connector;
 	struct drm_encoder *encoder;
 	struct drm_bridge *int_bridge, *ext_bridge;
-	struct drm_connector *connector;
-	struct list_head *connector_list;
+	int ret;
 
 	int_bridge = msm_dsi->bridge;
 	ext_bridge = msm_dsi->external_bridge =
@@ -699,22 +701,44 @@ struct drm_connector *msm_dsi_manager_ext_bridge_init(u8 id)
 
 	encoder = msm_dsi->encoder;
 
-	/* link the internal dsi bridge to the external bridge */
-	drm_bridge_attach(encoder, ext_bridge, int_bridge, 0);
-
 	/*
-	 * we need the drm_connector created by the external bridge
-	 * driver (or someone else) to feed it to our driver's
-	 * priv->connector[] list, mainly for msm_fbdev_init()
+	 * Try first to create the bridge without it creating its own
+	 * connector.. currently some bridges support this, and others
+	 * do not (and some support both modes)
 	 */
-	connector_list = &dev->mode_config.connector_list;
+	ret = drm_bridge_attach(encoder, ext_bridge, int_bridge,
+			DRM_BRIDGE_ATTACH_NO_CONNECTOR);
+	if (ret == -EINVAL) {
+		struct drm_connector *connector;
+		struct list_head *connector_list;
+
+		/* link the internal dsi bridge to the external bridge */
+		drm_bridge_attach(encoder, ext_bridge, int_bridge, 0);
+
+		/*
+		 * we need the drm_connector created by the external bridge
+		 * driver (or someone else) to feed it to our driver's
+		 * priv->connector[] list, mainly for msm_fbdev_init()
+		 */
+		connector_list = &dev->mode_config.connector_list;
 
-	list_for_each_entry(connector, connector_list, head) {
-		if (drm_connector_has_possible_encoder(connector, encoder))
-			return connector;
+		list_for_each_entry(connector, connector_list, head) {
+			if (drm_connector_has_possible_encoder(connector, encoder))
+				return connector;
+		}
+
+		return ERR_PTR(-ENODEV);
+	}
+
+	connector = drm_bridge_connector_init(dev, encoder);
+	if (IS_ERR(connector)) {
+		DRM_ERROR("Unable to create bridge connector\n");
+		return ERR_CAST(connector);
 	}
 
-	return ERR_PTR(-ENODEV);
+	drm_connector_attach_encoder(connector, encoder);
+
+	return connector;
 }
 
 void msm_dsi_manager_bridge_destroy(struct drm_bridge *bridge)
-- 
2.31.1

[PATCH v2 2/3] drm/bridge: ti-sn65dsi86: Implement bridge->mode_valid()

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

For the brave new world of bridges not creating their own connectors, we
need to implement the max clock limitation via bridge->mode_valid()
instead of connector->mode_valid().

v2: Drop unneeded connector->mode_valid()

Signed-off-by: Rob Clark <robdclark@chromium.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 41d48a393e7f..6154bed0af5b 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -615,20 +615,8 @@ static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector)
 	return drm_bridge_get_modes(pdata->next_bridge, connector);
 }
 
-static enum drm_mode_status
-ti_sn_bridge_connector_mode_valid(struct drm_connector *connector,
-				  struct drm_display_mode *mode)
-{
-	/* maximum supported resolution is 4K at 60 fps */
-	if (mode->clock > 594000)
-		return MODE_CLOCK_HIGH;
-
-	return MODE_OK;
-}
-
 static struct drm_connector_helper_funcs ti_sn_bridge_connector_helper_funcs = {
 	.get_modes = ti_sn_bridge_connector_get_modes,
-	.mode_valid = ti_sn_bridge_connector_mode_valid,
 };
 
 static const struct drm_connector_funcs ti_sn_bridge_connector_funcs = {
@@ -766,6 +754,18 @@ static void ti_sn_bridge_detach(struct drm_bridge *bridge)
 	drm_dp_aux_unregister(&bridge_to_ti_sn65dsi86(bridge)->aux);
 }
 
+static enum drm_mode_status
+ti_sn_bridge_mode_valid(struct drm_bridge *bridge,
+			const struct drm_display_info *info,
+			const struct drm_display_mode *mode)
+{
+	/* maximum supported resolution is 4K at 60 fps */
+	if (mode->clock > 594000)
+		return MODE_CLOCK_HIGH;
+
+	return MODE_OK;
+}
+
 static void ti_sn_bridge_disable(struct drm_bridge *bridge)
 {
 	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
@@ -1127,6 +1127,7 @@ static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
 static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
 	.attach = ti_sn_bridge_attach,
 	.detach = ti_sn_bridge_detach,
+	.mode_valid = ti_sn_bridge_mode_valid,
 	.pre_enable = ti_sn_bridge_pre_enable,
 	.enable = ti_sn_bridge_enable,
 	.disable = ti_sn_bridge_disable,
-- 
2.31.1

[PATCH v2 3/3] drm/bridge: ti-sn65dsi86: Add NO_CONNECTOR support

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

Slightly awkward to fish out the display_info when we aren't creating
own connector.  But I don't see an obvious better way.

v2: Remove error return with NO_CONNECTOR flag

Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 39 ++++++++++++++++++++-------
 1 file changed, 29 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 6154bed0af5b..94c94cc8a4d8 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -667,11 +667,6 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
 						   .node = NULL,
 						 };
 
-	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
-		DRM_ERROR("Fix bridge driver to make connector optional!");
-		return -EINVAL;
-	}
-
 	pdata->aux.drm_dev = bridge->dev;
 	ret = drm_dp_aux_register(&pdata->aux);
 	if (ret < 0) {
@@ -679,9 +674,11 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
 		return ret;
 	}
 
-	ret = ti_sn_bridge_connector_init(pdata);
-	if (ret < 0)
-		goto err_conn_init;
+	if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {
+		ret = ti_sn_bridge_connector_init(pdata);
+		if (ret < 0)
+			goto err_conn_init;
+	}
 
 	/*
 	 * TODO: ideally finding host resource and dsi dev registration needs
@@ -743,7 +740,8 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
 err_dsi_attach:
 	mipi_dsi_device_unregister(dsi);
 err_dsi_host:
-	drm_connector_cleanup(&pdata->connector);
+	if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
+		drm_connector_cleanup(&pdata->connector);
 err_conn_init:
 	drm_dp_aux_unregister(&pdata->aux);
 	return ret;
@@ -792,9 +790,30 @@ static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata)
 	regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val);
 }
 
+/*
+ * Find the connector and fish out the bpc from display_info.  It would
+ * be nice if we could get this instead from drm_bridge_state, but that
+ * doesn't yet appear to be the case.
+ */
 static unsigned int ti_sn_bridge_get_bpp(struct ti_sn65dsi86 *pdata)
 {
-	if (pdata->connector.display_info.bpc <= 6)
+	struct drm_bridge *bridge = &pdata->bridge;
+	struct drm_connector_list_iter conn_iter;
+	struct drm_connector *connector;
+	unsigned bpc = 0;
+
+	drm_connector_list_iter_begin(bridge->dev, &conn_iter);
+	drm_for_each_connector_iter(connector, &conn_iter) {
+		if (drm_connector_has_possible_encoder(connector, bridge->encoder)) {
+			bpc = connector->display_info.bpc;
+			break;
+		}
+	}
+	drm_connector_list_iter_end(&conn_iter);
+
+	WARN_ON(bpc == 0);
+
+	if (bpc <= 6)
 		return 18;
 	else
 		return 24;
-- 
2.31.1

Re: [PATCH v2 3/3] drm/bridge: ti-sn65dsi86: Add NO_CONNECTOR support

[Doug Anderson]

Hi,

On Mon, Sep 20, 2021 at 3:53 PM Rob Clark <robdclark@gmail.com> wrote:
>
> From: Rob Clark <robdclark@chromium.org>
>
> Slightly awkward to fish out the display_info when we aren't creating
> own connector.  But I don't see an obvious better way.
>
> v2: Remove error return with NO_CONNECTOR flag
>
> Signed-off-by: Rob Clark <robdclark@chromium.org>
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 39 ++++++++++++++++++++-------
>  1 file changed, 29 insertions(+), 10 deletions(-)

This seems fine to me:

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

...if you would like me to apply patch #2 / #3 to drm-misc-next then
please yell.

-Doug

Re: [PATCH v2 3/3] drm/bridge: ti-sn65dsi86: Add NO_CONNECTOR support

[Doug Anderson]

Hi,

On Mon, Sep 20, 2021 at 3:53 PM Rob Clark <robdclark@gmail.com> wrote:
>
> From: Rob Clark <robdclark@chromium.org>
>
> Slightly awkward to fish out the display_info when we aren't creating
> own connector.  But I don't see an obvious better way.
>
> v2: Remove error return with NO_CONNECTOR flag
>
> Signed-off-by: Rob Clark <robdclark@chromium.org>
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 39 ++++++++++++++++++++-------
>  1 file changed, 29 insertions(+), 10 deletions(-)

This seems fine to me:

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

...if you would like me to apply patch #2 / #3 to drm-misc-next then
please yell.

-Doug

Re: [PATCH v2 3/3] drm/bridge: ti-sn65dsi86: Add NO_CONNECTOR support

[Rob Clark]

On Tue, Sep 21, 2021 at 3:20 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Mon, Sep 20, 2021 at 3:53 PM Rob Clark <robdclark@gmail.com> wrote:
> >
> > From: Rob Clark <robdclark@chromium.org>
> >
> > Slightly awkward to fish out the display_info when we aren't creating
> > own connector.  But I don't see an obvious better way.
> >
> > v2: Remove error return with NO_CONNECTOR flag
> >
> > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > ---
> >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 39 ++++++++++++++++++++-------
> >  1 file changed, 29 insertions(+), 10 deletions(-)
>
> This seems fine to me:
>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
>
> ...if you would like me to apply patch #2 / #3 to drm-misc-next then
> please yell.

Thanks.. I think we can give it a few days for Laurent to have a look.

This will conflict some with Maxime's bridge vs dsi-host ordering
series.. not sure how close that one is to the finish line, but I can
rebase either patch on top of the other depending on which order they
are applied

BR,
-R

Re: [PATCH v2 3/3] drm/bridge: ti-sn65dsi86: Add NO_CONNECTOR support

[Rob Clark]

On Tue, Sep 21, 2021 at 3:20 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Mon, Sep 20, 2021 at 3:53 PM Rob Clark <robdclark@gmail.com> wrote:
> >
> > From: Rob Clark <robdclark@chromium.org>
> >
> > Slightly awkward to fish out the display_info when we aren't creating
> > own connector.  But I don't see an obvious better way.
> >
> > v2: Remove error return with NO_CONNECTOR flag
> >
> > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > ---
> >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 39 ++++++++++++++++++++-------
> >  1 file changed, 29 insertions(+), 10 deletions(-)
>
> This seems fine to me:
>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
>
> ...if you would like me to apply patch #2 / #3 to drm-misc-next then
> please yell.

Thanks.. I think we can give it a few days for Laurent to have a look.

This will conflict some with Maxime's bridge vs dsi-host ordering
series.. not sure how close that one is to the finish line, but I can
rebase either patch on top of the other depending on which order they
are applied

BR,
-R

Re: [PATCH v2 2/3] drm/bridge: ti-sn65dsi86: Implement bridge->mode_valid()

[Laurent Pinchart]

Hi Rob,

Thank you for the patch.

On Mon, Sep 20, 2021 at 03:57:59PM -0700, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
> 
> For the brave new world of bridges not creating their own connectors, we
> need to implement the max clock limitation via bridge->mode_valid()
> instead of connector->mode_valid().
> 
> v2: Drop unneeded connector->mode_valid()
> 
> Signed-off-by: Rob Clark <robdclark@chromium.org>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 41d48a393e7f..6154bed0af5b 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -615,20 +615,8 @@ static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector)
>  	return drm_bridge_get_modes(pdata->next_bridge, connector);
>  }
>  
> -static enum drm_mode_status
> -ti_sn_bridge_connector_mode_valid(struct drm_connector *connector,
> -				  struct drm_display_mode *mode)
> -{
> -	/* maximum supported resolution is 4K at 60 fps */
> -	if (mode->clock > 594000)
> -		return MODE_CLOCK_HIGH;
> -
> -	return MODE_OK;
> -}
> -
>  static struct drm_connector_helper_funcs ti_sn_bridge_connector_helper_funcs = {
>  	.get_modes = ti_sn_bridge_connector_get_modes,
> -	.mode_valid = ti_sn_bridge_connector_mode_valid,
>  };
>  
>  static const struct drm_connector_funcs ti_sn_bridge_connector_funcs = {
> @@ -766,6 +754,18 @@ static void ti_sn_bridge_detach(struct drm_bridge *bridge)
>  	drm_dp_aux_unregister(&bridge_to_ti_sn65dsi86(bridge)->aux);
>  }
>  
> +static enum drm_mode_status
> +ti_sn_bridge_mode_valid(struct drm_bridge *bridge,
> +			const struct drm_display_info *info,
> +			const struct drm_display_mode *mode)
> +{
> +	/* maximum supported resolution is 4K at 60 fps */
> +	if (mode->clock > 594000)
> +		return MODE_CLOCK_HIGH;
> +
> +	return MODE_OK;
> +}
> +
>  static void ti_sn_bridge_disable(struct drm_bridge *bridge)
>  {
>  	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> @@ -1127,6 +1127,7 @@ static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
>  static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
>  	.attach = ti_sn_bridge_attach,
>  	.detach = ti_sn_bridge_detach,
> +	.mode_valid = ti_sn_bridge_mode_valid,
>  	.pre_enable = ti_sn_bridge_pre_enable,
>  	.enable = ti_sn_bridge_enable,
>  	.disable = ti_sn_bridge_disable,

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 3/3] drm/bridge: ti-sn65dsi86: Add NO_CONNECTOR support

[Laurent Pinchart]

Hi Rob,

Thank you for the patch.

On Mon, Sep 20, 2021 at 03:58:00PM -0700, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
> 
> Slightly awkward to fish out the display_info when we aren't creating
> own connector.  But I don't see an obvious better way.
> 
> v2: Remove error return with NO_CONNECTOR flag
> 
> Signed-off-by: Rob Clark <robdclark@chromium.org>
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 39 ++++++++++++++++++++-------
>  1 file changed, 29 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 6154bed0af5b..94c94cc8a4d8 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -667,11 +667,6 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
>  						   .node = NULL,
>  						 };
>  
> -	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
> -		DRM_ERROR("Fix bridge driver to make connector optional!");
> -		return -EINVAL;
> -	}
> -
>  	pdata->aux.drm_dev = bridge->dev;
>  	ret = drm_dp_aux_register(&pdata->aux);
>  	if (ret < 0) {
> @@ -679,9 +674,11 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
>  		return ret;
>  	}
>  
> -	ret = ti_sn_bridge_connector_init(pdata);
> -	if (ret < 0)
> -		goto err_conn_init;
> +	if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {
> +		ret = ti_sn_bridge_connector_init(pdata);
> +		if (ret < 0)
> +			goto err_conn_init;
> +	}
>  
>  	/*
>  	 * TODO: ideally finding host resource and dsi dev registration needs
> @@ -743,7 +740,8 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
>  err_dsi_attach:
>  	mipi_dsi_device_unregister(dsi);
>  err_dsi_host:
> -	drm_connector_cleanup(&pdata->connector);
> +	if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
> +		drm_connector_cleanup(&pdata->connector);

I wonder if we actually need this. The connector gets attached to the
encoder, won't it be destroyed by the DRM core in the error path ?

>  err_conn_init:
>  	drm_dp_aux_unregister(&pdata->aux);
>  	return ret;
> @@ -792,9 +790,30 @@ static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata)
>  	regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val);
>  }
>  
> +/*
> + * Find the connector and fish out the bpc from display_info.  It would
> + * be nice if we could get this instead from drm_bridge_state, but that
> + * doesn't yet appear to be the case.

You already have a bus format in the bridge state, from which you can
derive the bpp. Could you give it a try ?

> + */
>  static unsigned int ti_sn_bridge_get_bpp(struct ti_sn65dsi86 *pdata)
>  {
> -	if (pdata->connector.display_info.bpc <= 6)
> +	struct drm_bridge *bridge = &pdata->bridge;
> +	struct drm_connector_list_iter conn_iter;
> +	struct drm_connector *connector;
> +	unsigned bpc = 0;
> +
> +	drm_connector_list_iter_begin(bridge->dev, &conn_iter);
> +	drm_for_each_connector_iter(connector, &conn_iter) {
> +		if (drm_connector_has_possible_encoder(connector, bridge->encoder)) {
> +			bpc = connector->display_info.bpc;
> +			break;
> +		}
> +	}
> +	drm_connector_list_iter_end(&conn_iter);
> +
> +	WARN_ON(bpc == 0);
> +
> +	if (bpc <= 6)
>  		return 18;
>  	else
>  		return 24;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 3/3] drm/bridge: ti-sn65dsi86: Add NO_CONNECTOR support

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1388 bytes --]

Hi,

On Tue, Sep 21, 2021 at 03:42:05PM -0700, Rob Clark wrote:
> On Tue, Sep 21, 2021 at 3:20 PM Doug Anderson <dianders@chromium.org> wrote:
> >
> > Hi,
> >
> > On Mon, Sep 20, 2021 at 3:53 PM Rob Clark <robdclark@gmail.com> wrote:
> > >
> > > From: Rob Clark <robdclark@chromium.org>
> > >
> > > Slightly awkward to fish out the display_info when we aren't creating
> > > own connector.  But I don't see an obvious better way.
> > >
> > > v2: Remove error return with NO_CONNECTOR flag
> > >
> > > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > > ---
> > >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 39 ++++++++++++++++++++-------
> > >  1 file changed, 29 insertions(+), 10 deletions(-)
> >
> > This seems fine to me:
> >
> > Reviewed-by: Douglas Anderson <dianders@chromium.org>
> >
> > ...if you would like me to apply patch #2 / #3 to drm-misc-next then
> > please yell.
> 
> Thanks.. I think we can give it a few days for Laurent to have a look.
> 
> This will conflict some with Maxime's bridge vs dsi-host ordering
> series.. not sure how close that one is to the finish line, but I can
> rebase either patch on top of the other depending on which order they
> are applied

It's probably going to take a bit of time to get merged, so don't worry
about this series and just go ahead, I'll rebase it on top of yours if
needed.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 3/3] drm/bridge: ti-sn65dsi86: Add NO_CONNECTOR support

[Rob Clark]

On Wed, Sep 22, 2021 at 5:44 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Rob,
>
> Thank you for the patch.
>
> On Mon, Sep 20, 2021 at 03:58:00PM -0700, Rob Clark wrote:
> > From: Rob Clark <robdclark@chromium.org>
> >
> > Slightly awkward to fish out the display_info when we aren't creating
> > own connector.  But I don't see an obvious better way.
> >
> > v2: Remove error return with NO_CONNECTOR flag
> >
> > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > ---
> >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 39 ++++++++++++++++++++-------
> >  1 file changed, 29 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index 6154bed0af5b..94c94cc8a4d8 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > @@ -667,11 +667,6 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
> >                                                  .node = NULL,
> >                                                };
> >
> > -     if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
> > -             DRM_ERROR("Fix bridge driver to make connector optional!");
> > -             return -EINVAL;
> > -     }
> > -
> >       pdata->aux.drm_dev = bridge->dev;
> >       ret = drm_dp_aux_register(&pdata->aux);
> >       if (ret < 0) {
> > @@ -679,9 +674,11 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
> >               return ret;
> >       }
> >
> > -     ret = ti_sn_bridge_connector_init(pdata);
> > -     if (ret < 0)
> > -             goto err_conn_init;
> > +     if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {
> > +             ret = ti_sn_bridge_connector_init(pdata);
> > +             if (ret < 0)
> > +                     goto err_conn_init;
> > +     }
> >
> >       /*
> >        * TODO: ideally finding host resource and dsi dev registration needs
> > @@ -743,7 +740,8 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
> >  err_dsi_attach:
> >       mipi_dsi_device_unregister(dsi);
> >  err_dsi_host:
> > -     drm_connector_cleanup(&pdata->connector);
> > +     if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
> > +             drm_connector_cleanup(&pdata->connector);
>
> I wonder if we actually need this. The connector gets attached to the
> encoder, won't it be destroyed by the DRM core in the error path ?

This does not appear to be the case, we leak the connector if I remove
this (and add a hack to trigger the error path)

> >  err_conn_init:
> >       drm_dp_aux_unregister(&pdata->aux);
> >       return ret;
> > @@ -792,9 +790,30 @@ static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata)
> >       regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val);
> >  }
> >
> > +/*
> > + * Find the connector and fish out the bpc from display_info.  It would
> > + * be nice if we could get this instead from drm_bridge_state, but that
> > + * doesn't yet appear to be the case.
>
> You already have a bus format in the bridge state, from which you can
> derive the bpp. Could you give it a try ?

Possibly the bridge should be converted to ->atomic_enable(), etc..
I'll leave that for another time

BR,
-R

> > + */
> >  static unsigned int ti_sn_bridge_get_bpp(struct ti_sn65dsi86 *pdata)
> >  {
> > -     if (pdata->connector.display_info.bpc <= 6)
> > +     struct drm_bridge *bridge = &pdata->bridge;
> > +     struct drm_connector_list_iter conn_iter;
> > +     struct drm_connector *connector;
> > +     unsigned bpc = 0;
> > +
> > +     drm_connector_list_iter_begin(bridge->dev, &conn_iter);
> > +     drm_for_each_connector_iter(connector, &conn_iter) {
> > +             if (drm_connector_has_possible_encoder(connector, bridge->encoder)) {
> > +                     bpc = connector->display_info.bpc;
> > +                     break;
> > +             }
> > +     }
> > +     drm_connector_list_iter_end(&conn_iter);
> > +
> > +     WARN_ON(bpc == 0);
> > +
> > +     if (bpc <= 6)
> >               return 18;
> >       else
> >               return 24;
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH v2 3/3] drm/bridge: ti-sn65dsi86: Add NO_CONNECTOR support

[Rob Clark]

On Wed, Sep 22, 2021 at 5:44 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Rob,
>
> Thank you for the patch.
>
> On Mon, Sep 20, 2021 at 03:58:00PM -0700, Rob Clark wrote:
> > From: Rob Clark <robdclark@chromium.org>
> >
> > Slightly awkward to fish out the display_info when we aren't creating
> > own connector.  But I don't see an obvious better way.
> >
> > v2: Remove error return with NO_CONNECTOR flag
> >
> > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > ---
> >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 39 ++++++++++++++++++++-------
> >  1 file changed, 29 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index 6154bed0af5b..94c94cc8a4d8 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > @@ -667,11 +667,6 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
> >                                                  .node = NULL,
> >                                                };
> >
> > -     if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
> > -             DRM_ERROR("Fix bridge driver to make connector optional!");
> > -             return -EINVAL;
> > -     }
> > -
> >       pdata->aux.drm_dev = bridge->dev;
> >       ret = drm_dp_aux_register(&pdata->aux);
> >       if (ret < 0) {
> > @@ -679,9 +674,11 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
> >               return ret;
> >       }
> >
> > -     ret = ti_sn_bridge_connector_init(pdata);
> > -     if (ret < 0)
> > -             goto err_conn_init;
> > +     if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {
> > +             ret = ti_sn_bridge_connector_init(pdata);
> > +             if (ret < 0)
> > +                     goto err_conn_init;
> > +     }
> >
> >       /*
> >        * TODO: ideally finding host resource and dsi dev registration needs
> > @@ -743,7 +740,8 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
> >  err_dsi_attach:
> >       mipi_dsi_device_unregister(dsi);
> >  err_dsi_host:
> > -     drm_connector_cleanup(&pdata->connector);
> > +     if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
> > +             drm_connector_cleanup(&pdata->connector);
>
> I wonder if we actually need this. The connector gets attached to the
> encoder, won't it be destroyed by the DRM core in the error path ?

This does not appear to be the case, we leak the connector if I remove
this (and add a hack to trigger the error path)

> >  err_conn_init:
> >       drm_dp_aux_unregister(&pdata->aux);
> >       return ret;
> > @@ -792,9 +790,30 @@ static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata)
> >       regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val);
> >  }
> >
> > +/*
> > + * Find the connector and fish out the bpc from display_info.  It would
> > + * be nice if we could get this instead from drm_bridge_state, but that
> > + * doesn't yet appear to be the case.
>
> You already have a bus format in the bridge state, from which you can
> derive the bpp. Could you give it a try ?

Possibly the bridge should be converted to ->atomic_enable(), etc..
I'll leave that for another time

BR,
-R

> > + */
> >  static unsigned int ti_sn_bridge_get_bpp(struct ti_sn65dsi86 *pdata)
> >  {
> > -     if (pdata->connector.display_info.bpc <= 6)
> > +     struct drm_bridge *bridge = &pdata->bridge;
> > +     struct drm_connector_list_iter conn_iter;
> > +     struct drm_connector *connector;
> > +     unsigned bpc = 0;
> > +
> > +     drm_connector_list_iter_begin(bridge->dev, &conn_iter);
> > +     drm_for_each_connector_iter(connector, &conn_iter) {
> > +             if (drm_connector_has_possible_encoder(connector, bridge->encoder)) {
> > +                     bpc = connector->display_info.bpc;
> > +                     break;
> > +             }
> > +     }
> > +     drm_connector_list_iter_end(&conn_iter);
> > +
> > +     WARN_ON(bpc == 0);
> > +
> > +     if (bpc <= 6)
> >               return 18;
> >       else
> >               return 24;
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH v2 3/3] drm/bridge: ti-sn65dsi86: Add NO_CONNECTOR support

[Laurent Pinchart]

Hi Rob,

On Thu, Sep 23, 2021 at 10:31:52AM -0700, Rob Clark wrote:
> On Wed, Sep 22, 2021 at 5:44 PM Laurent Pinchart wrote:
> > On Mon, Sep 20, 2021 at 03:58:00PM -0700, Rob Clark wrote:
> > > From: Rob Clark <robdclark@chromium.org>
> > >
> > > Slightly awkward to fish out the display_info when we aren't creating
> > > own connector.  But I don't see an obvious better way.
> > >
> > > v2: Remove error return with NO_CONNECTOR flag
> > >
> > > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > > ---
> > >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 39 ++++++++++++++++++++-------
> > >  1 file changed, 29 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > index 6154bed0af5b..94c94cc8a4d8 100644
> > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > @@ -667,11 +667,6 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
> > >                                                  .node = NULL,
> > >                                                };
> > >
> > > -     if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
> > > -             DRM_ERROR("Fix bridge driver to make connector optional!");
> > > -             return -EINVAL;
> > > -     }
> > > -
> > >       pdata->aux.drm_dev = bridge->dev;
> > >       ret = drm_dp_aux_register(&pdata->aux);
> > >       if (ret < 0) {
> > > @@ -679,9 +674,11 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
> > >               return ret;
> > >       }
> > >
> > > -     ret = ti_sn_bridge_connector_init(pdata);
> > > -     if (ret < 0)
> > > -             goto err_conn_init;
> > > +     if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {
> > > +             ret = ti_sn_bridge_connector_init(pdata);
> > > +             if (ret < 0)
> > > +                     goto err_conn_init;
> > > +     }
> > >
> > >       /*
> > >        * TODO: ideally finding host resource and dsi dev registration needs
> > > @@ -743,7 +740,8 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
> > >  err_dsi_attach:
> > >       mipi_dsi_device_unregister(dsi);
> > >  err_dsi_host:
> > > -     drm_connector_cleanup(&pdata->connector);
> > > +     if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
> > > +             drm_connector_cleanup(&pdata->connector);
> >
> > I wonder if we actually need this. The connector gets attached to the
> > encoder, won't it be destroyed by the DRM core in the error path ?
> 
> This does not appear to be the case, we leak the connector if I remove
> this (and add a hack to trigger the error path)

OK.

> > >  err_conn_init:
> > >       drm_dp_aux_unregister(&pdata->aux);
> > >       return ret;
> > > @@ -792,9 +790,30 @@ static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata)
> > >       regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val);
> > >  }
> > >
> > > +/*
> > > + * Find the connector and fish out the bpc from display_info.  It would
> > > + * be nice if we could get this instead from drm_bridge_state, but that
> > > + * doesn't yet appear to be the case.
> >
> > You already have a bus format in the bridge state, from which you can
> > derive the bpp. Could you give it a try ?
> 
> Possibly the bridge should be converted to ->atomic_enable(), etc..
> I'll leave that for another time

It should be fairly straightforward, and would avoid the hack below.

> > > + */
> > >  static unsigned int ti_sn_bridge_get_bpp(struct ti_sn65dsi86 *pdata)
> > >  {
> > > -     if (pdata->connector.display_info.bpc <= 6)
> > > +     struct drm_bridge *bridge = &pdata->bridge;
> > > +     struct drm_connector_list_iter conn_iter;
> > > +     struct drm_connector *connector;
> > > +     unsigned bpc = 0;
> > > +
> > > +     drm_connector_list_iter_begin(bridge->dev, &conn_iter);
> > > +     drm_for_each_connector_iter(connector, &conn_iter) {
> > > +             if (drm_connector_has_possible_encoder(connector, bridge->encoder)) {
> > > +                     bpc = connector->display_info.bpc;
> > > +                     break;
> > > +             }
> > > +     }
> > > +     drm_connector_list_iter_end(&conn_iter);
> > > +
> > > +     WARN_ON(bpc == 0);
> > > +
> > > +     if (bpc <= 6)
> > >               return 18;
> > >       else
> > >               return 24;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/3] drm/msm/dsi: Support NO_CONNECTOR bridges

[Dmitry Baryshkov]

On 21/09/2021 01:57, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
> 
> For now, since we have a mix of bridges which support this flag, which
> which do *not* support this flag, or work both ways, try it once with
> NO_CONNECTOR and then fall back to the old way if that doesn't work.
> Eventually we can drop the fallback path.
> 
> v2: Add missing drm_connector_attach_encoder() so display actually comes
>      up when the bridge properly handles the NO_CONNECTOR flag
> 
> Signed-off-by: Rob Clark <robdclark@chromium.org>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

I think this patch can go through the drm/msm, while two other patches 
would need to through the drm-misc. Is it correct?

> ---
>   drivers/gpu/drm/msm/Kconfig           |  2 ++
>   drivers/gpu/drm/msm/dsi/dsi_manager.c | 50 ++++++++++++++++++++-------
>   2 files changed, 39 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/Kconfig b/drivers/gpu/drm/msm/Kconfig
> index e9c6af78b1d7..36e5ba3ccc28 100644
> --- a/drivers/gpu/drm/msm/Kconfig
> +++ b/drivers/gpu/drm/msm/Kconfig
> @@ -14,6 +14,8 @@ config DRM_MSM
>   	select REGULATOR
>   	select DRM_KMS_HELPER
>   	select DRM_PANEL
> +	select DRM_BRIDGE
> +	select DRM_PANEL_BRIDGE
>   	select DRM_SCHED
>   	select SHMEM
>   	select TMPFS
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> index c41d39f5b7cf..e25877073d31 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> @@ -3,6 +3,8 @@
>    * Copyright (c) 2015, The Linux Foundation. All rights reserved.
>    */
>   
> +#include "drm/drm_bridge_connector.h"
> +
>   #include "msm_kms.h"
>   #include "dsi.h"
>   
> @@ -688,10 +690,10 @@ struct drm_connector *msm_dsi_manager_ext_bridge_init(u8 id)
>   {
>   	struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
>   	struct drm_device *dev = msm_dsi->dev;
> +	struct drm_connector *connector;
>   	struct drm_encoder *encoder;
>   	struct drm_bridge *int_bridge, *ext_bridge;
> -	struct drm_connector *connector;
> -	struct list_head *connector_list;
> +	int ret;
>   
>   	int_bridge = msm_dsi->bridge;
>   	ext_bridge = msm_dsi->external_bridge =
> @@ -699,22 +701,44 @@ struct drm_connector *msm_dsi_manager_ext_bridge_init(u8 id)
>   
>   	encoder = msm_dsi->encoder;
>   
> -	/* link the internal dsi bridge to the external bridge */
> -	drm_bridge_attach(encoder, ext_bridge, int_bridge, 0);
> -
>   	/*
> -	 * we need the drm_connector created by the external bridge
> -	 * driver (or someone else) to feed it to our driver's
> -	 * priv->connector[] list, mainly for msm_fbdev_init()
> +	 * Try first to create the bridge without it creating its own
> +	 * connector.. currently some bridges support this, and others
> +	 * do not (and some support both modes)
>   	 */
> -	connector_list = &dev->mode_config.connector_list;
> +	ret = drm_bridge_attach(encoder, ext_bridge, int_bridge,
> +			DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> +	if (ret == -EINVAL) {
> +		struct drm_connector *connector;
> +		struct list_head *connector_list;
> +
> +		/* link the internal dsi bridge to the external bridge */
> +		drm_bridge_attach(encoder, ext_bridge, int_bridge, 0);
> +
> +		/*
> +		 * we need the drm_connector created by the external bridge
> +		 * driver (or someone else) to feed it to our driver's
> +		 * priv->connector[] list, mainly for msm_fbdev_init()
> +		 */
> +		connector_list = &dev->mode_config.connector_list;
>   
> -	list_for_each_entry(connector, connector_list, head) {
> -		if (drm_connector_has_possible_encoder(connector, encoder))
> -			return connector;
> +		list_for_each_entry(connector, connector_list, head) {
> +			if (drm_connector_has_possible_encoder(connector, encoder))
> +				return connector;
> +		}
> +
> +		return ERR_PTR(-ENODEV);
> +	}
> +
> +	connector = drm_bridge_connector_init(dev, encoder);
> +	if (IS_ERR(connector)) {
> +		DRM_ERROR("Unable to create bridge connector\n");
> +		return ERR_CAST(connector);
>   	}
>   
> -	return ERR_PTR(-ENODEV);
> +	drm_connector_attach_encoder(connector, encoder);
> +
> +	return connector;
>   }
>   
>   void msm_dsi_manager_bridge_destroy(struct drm_bridge *bridge)
> 


-- 
With best wishes
Dmitry

Re: [PATCH v2 1/3] drm/msm/dsi: Support NO_CONNECTOR bridges

[Dmitry Baryshkov]

On 21/09/2021 01:57, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
> 
> For now, since we have a mix of bridges which support this flag, which
> which do *not* support this flag, or work both ways, try it once with
> NO_CONNECTOR and then fall back to the old way if that doesn't work.
> Eventually we can drop the fallback path.
> 
> v2: Add missing drm_connector_attach_encoder() so display actually comes
>      up when the bridge properly handles the NO_CONNECTOR flag
> 
> Signed-off-by: Rob Clark <robdclark@chromium.org>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

I think this patch can go through the drm/msm, while two other patches 
would need to through the drm-misc. Is it correct?

> ---
>   drivers/gpu/drm/msm/Kconfig           |  2 ++
>   drivers/gpu/drm/msm/dsi/dsi_manager.c | 50 ++++++++++++++++++++-------
>   2 files changed, 39 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/Kconfig b/drivers/gpu/drm/msm/Kconfig
> index e9c6af78b1d7..36e5ba3ccc28 100644
> --- a/drivers/gpu/drm/msm/Kconfig
> +++ b/drivers/gpu/drm/msm/Kconfig
> @@ -14,6 +14,8 @@ config DRM_MSM
>   	select REGULATOR
>   	select DRM_KMS_HELPER
>   	select DRM_PANEL
> +	select DRM_BRIDGE
> +	select DRM_PANEL_BRIDGE
>   	select DRM_SCHED
>   	select SHMEM
>   	select TMPFS
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> index c41d39f5b7cf..e25877073d31 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> @@ -3,6 +3,8 @@
>    * Copyright (c) 2015, The Linux Foundation. All rights reserved.
>    */
>   
> +#include "drm/drm_bridge_connector.h"
> +
>   #include "msm_kms.h"
>   #include "dsi.h"
>   
> @@ -688,10 +690,10 @@ struct drm_connector *msm_dsi_manager_ext_bridge_init(u8 id)
>   {
>   	struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
>   	struct drm_device *dev = msm_dsi->dev;
> +	struct drm_connector *connector;
>   	struct drm_encoder *encoder;
>   	struct drm_bridge *int_bridge, *ext_bridge;
> -	struct drm_connector *connector;
> -	struct list_head *connector_list;
> +	int ret;
>   
>   	int_bridge = msm_dsi->bridge;
>   	ext_bridge = msm_dsi->external_bridge =
> @@ -699,22 +701,44 @@ struct drm_connector *msm_dsi_manager_ext_bridge_init(u8 id)
>   
>   	encoder = msm_dsi->encoder;
>   
> -	/* link the internal dsi bridge to the external bridge */
> -	drm_bridge_attach(encoder, ext_bridge, int_bridge, 0);
> -
>   	/*
> -	 * we need the drm_connector created by the external bridge
> -	 * driver (or someone else) to feed it to our driver's
> -	 * priv->connector[] list, mainly for msm_fbdev_init()
> +	 * Try first to create the bridge without it creating its own
> +	 * connector.. currently some bridges support this, and others
> +	 * do not (and some support both modes)
>   	 */
> -	connector_list = &dev->mode_config.connector_list;
> +	ret = drm_bridge_attach(encoder, ext_bridge, int_bridge,
> +			DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> +	if (ret == -EINVAL) {
> +		struct drm_connector *connector;
> +		struct list_head *connector_list;
> +
> +		/* link the internal dsi bridge to the external bridge */
> +		drm_bridge_attach(encoder, ext_bridge, int_bridge, 0);
> +
> +		/*
> +		 * we need the drm_connector created by the external bridge
> +		 * driver (or someone else) to feed it to our driver's
> +		 * priv->connector[] list, mainly for msm_fbdev_init()
> +		 */
> +		connector_list = &dev->mode_config.connector_list;
>   
> -	list_for_each_entry(connector, connector_list, head) {
> -		if (drm_connector_has_possible_encoder(connector, encoder))
> -			return connector;
> +		list_for_each_entry(connector, connector_list, head) {
> +			if (drm_connector_has_possible_encoder(connector, encoder))
> +				return connector;
> +		}
> +
> +		return ERR_PTR(-ENODEV);
> +	}
> +
> +	connector = drm_bridge_connector_init(dev, encoder);
> +	if (IS_ERR(connector)) {
> +		DRM_ERROR("Unable to create bridge connector\n");
> +		return ERR_CAST(connector);
>   	}
>   
> -	return ERR_PTR(-ENODEV);
> +	drm_connector_attach_encoder(connector, encoder);
> +
> +	return connector;
>   }
>   
>   void msm_dsi_manager_bridge_destroy(struct drm_bridge *bridge)
> 


-- 
With best wishes
Dmitry

Re: [PATCH v2 2/3] drm/bridge: ti-sn65dsi86: Implement bridge->mode_valid()

[Doug Anderson]

Hi,

On Wed, Sep 22, 2021 at 5:31 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Rob,
>
> Thank you for the patch.
>
> On Mon, Sep 20, 2021 at 03:57:59PM -0700, Rob Clark wrote:
> > From: Rob Clark <robdclark@chromium.org>
> >
> > For the brave new world of bridges not creating their own connectors, we
> > need to implement the max clock limitation via bridge->mode_valid()
> > instead of connector->mode_valid().
> >
> > v2: Drop unneeded connector->mode_valid()
> >
> > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > Reviewed-by: Douglas Anderson <dianders@chromium.org>
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> > ---
> >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 25 +++++++++++++------------
> >  1 file changed, 13 insertions(+), 12 deletions(-)

There's no reason to wait on this patch. Landed to drm-misc-next.

77d40e0176a5 drm/bridge: ti-sn65dsi86: Implement bridge->mode_valid()

-Doug

Re: [PATCH v2 2/3] drm/bridge: ti-sn65dsi86: Implement bridge->mode_valid()

[Doug Anderson]

Hi,

On Wed, Sep 22, 2021 at 5:31 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Rob,
>
> Thank you for the patch.
>
> On Mon, Sep 20, 2021 at 03:57:59PM -0700, Rob Clark wrote:
> > From: Rob Clark <robdclark@chromium.org>
> >
> > For the brave new world of bridges not creating their own connectors, we
> > need to implement the max clock limitation via bridge->mode_valid()
> > instead of connector->mode_valid().
> >
> > v2: Drop unneeded connector->mode_valid()
> >
> > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > Reviewed-by: Douglas Anderson <dianders@chromium.org>
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> > ---
> >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 25 +++++++++++++------------
> >  1 file changed, 13 insertions(+), 12 deletions(-)

There's no reason to wait on this patch. Landed to drm-misc-next.

77d40e0176a5 drm/bridge: ti-sn65dsi86: Implement bridge->mode_valid()

-Doug

Re: [PATCH v2 1/3] drm/msm/dsi: Support NO_CONNECTOR bridges

[Rob Clark]

On Fri, Oct 1, 2021 at 10:28 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On 21/09/2021 01:57, Rob Clark wrote:
> > From: Rob Clark <robdclark@chromium.org>
> >
> > For now, since we have a mix of bridges which support this flag, which
> > which do *not* support this flag, or work both ways, try it once with
> > NO_CONNECTOR and then fall back to the old way if that doesn't work.
> > Eventually we can drop the fallback path.
> >
> > v2: Add missing drm_connector_attach_encoder() so display actually comes
> >      up when the bridge properly handles the NO_CONNECTOR flag
> >
> > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>
> I think this patch can go through the drm/msm, while two other patches
> would need to through the drm-misc. Is it correct?

Correct, I made sure things worked in either order (ie. with msm patch
but without bridge patches, and visa versa), so they can land through
different trees

BR,
-R

>
> > ---
> >   drivers/gpu/drm/msm/Kconfig           |  2 ++
> >   drivers/gpu/drm/msm/dsi/dsi_manager.c | 50 ++++++++++++++++++++-------
> >   2 files changed, 39 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/Kconfig b/drivers/gpu/drm/msm/Kconfig
> > index e9c6af78b1d7..36e5ba3ccc28 100644
> > --- a/drivers/gpu/drm/msm/Kconfig
> > +++ b/drivers/gpu/drm/msm/Kconfig
> > @@ -14,6 +14,8 @@ config DRM_MSM
> >       select REGULATOR
> >       select DRM_KMS_HELPER
> >       select DRM_PANEL
> > +     select DRM_BRIDGE
> > +     select DRM_PANEL_BRIDGE
> >       select DRM_SCHED
> >       select SHMEM
> >       select TMPFS
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > index c41d39f5b7cf..e25877073d31 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > @@ -3,6 +3,8 @@
> >    * Copyright (c) 2015, The Linux Foundation. All rights reserved.
> >    */
> >
> > +#include "drm/drm_bridge_connector.h"
> > +
> >   #include "msm_kms.h"
> >   #include "dsi.h"
> >
> > @@ -688,10 +690,10 @@ struct drm_connector *msm_dsi_manager_ext_bridge_init(u8 id)
> >   {
> >       struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
> >       struct drm_device *dev = msm_dsi->dev;
> > +     struct drm_connector *connector;
> >       struct drm_encoder *encoder;
> >       struct drm_bridge *int_bridge, *ext_bridge;
> > -     struct drm_connector *connector;
> > -     struct list_head *connector_list;
> > +     int ret;
> >
> >       int_bridge = msm_dsi->bridge;
> >       ext_bridge = msm_dsi->external_bridge =
> > @@ -699,22 +701,44 @@ struct drm_connector *msm_dsi_manager_ext_bridge_init(u8 id)
> >
> >       encoder = msm_dsi->encoder;
> >
> > -     /* link the internal dsi bridge to the external bridge */
> > -     drm_bridge_attach(encoder, ext_bridge, int_bridge, 0);
> > -
> >       /*
> > -      * we need the drm_connector created by the external bridge
> > -      * driver (or someone else) to feed it to our driver's
> > -      * priv->connector[] list, mainly for msm_fbdev_init()
> > +      * Try first to create the bridge without it creating its own
> > +      * connector.. currently some bridges support this, and others
> > +      * do not (and some support both modes)
> >        */
> > -     connector_list = &dev->mode_config.connector_list;
> > +     ret = drm_bridge_attach(encoder, ext_bridge, int_bridge,
> > +                     DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> > +     if (ret == -EINVAL) {
> > +             struct drm_connector *connector;
> > +             struct list_head *connector_list;
> > +
> > +             /* link the internal dsi bridge to the external bridge */
> > +             drm_bridge_attach(encoder, ext_bridge, int_bridge, 0);
> > +
> > +             /*
> > +              * we need the drm_connector created by the external bridge
> > +              * driver (or someone else) to feed it to our driver's
> > +              * priv->connector[] list, mainly for msm_fbdev_init()
> > +              */
> > +             connector_list = &dev->mode_config.connector_list;
> >
> > -     list_for_each_entry(connector, connector_list, head) {
> > -             if (drm_connector_has_possible_encoder(connector, encoder))
> > -                     return connector;
> > +             list_for_each_entry(connector, connector_list, head) {
> > +                     if (drm_connector_has_possible_encoder(connector, encoder))
> > +                             return connector;
> > +             }
> > +
> > +             return ERR_PTR(-ENODEV);
> > +     }
> > +
> > +     connector = drm_bridge_connector_init(dev, encoder);
> > +     if (IS_ERR(connector)) {
> > +             DRM_ERROR("Unable to create bridge connector\n");
> > +             return ERR_CAST(connector);
> >       }
> >
> > -     return ERR_PTR(-ENODEV);
> > +     drm_connector_attach_encoder(connector, encoder);
> > +
> > +     return connector;
> >   }
> >
> >   void msm_dsi_manager_bridge_destroy(struct drm_bridge *bridge)
> >
>
>
> --
> With best wishes
> Dmitry

Re: [PATCH v2 1/3] drm/msm/dsi: Support NO_CONNECTOR bridges

[Rob Clark]

On Fri, Oct 1, 2021 at 10:28 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On 21/09/2021 01:57, Rob Clark wrote:
> > From: Rob Clark <robdclark@chromium.org>
> >
> > For now, since we have a mix of bridges which support this flag, which
> > which do *not* support this flag, or work both ways, try it once with
> > NO_CONNECTOR and then fall back to the old way if that doesn't work.
> > Eventually we can drop the fallback path.
> >
> > v2: Add missing drm_connector_attach_encoder() so display actually comes
> >      up when the bridge properly handles the NO_CONNECTOR flag
> >
> > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>
> I think this patch can go through the drm/msm, while two other patches
> would need to through the drm-misc. Is it correct?

Correct, I made sure things worked in either order (ie. with msm patch
but without bridge patches, and visa versa), so they can land through
different trees

BR,
-R

>
> > ---
> >   drivers/gpu/drm/msm/Kconfig           |  2 ++
> >   drivers/gpu/drm/msm/dsi/dsi_manager.c | 50 ++++++++++++++++++++-------
> >   2 files changed, 39 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/Kconfig b/drivers/gpu/drm/msm/Kconfig
> > index e9c6af78b1d7..36e5ba3ccc28 100644
> > --- a/drivers/gpu/drm/msm/Kconfig
> > +++ b/drivers/gpu/drm/msm/Kconfig
> > @@ -14,6 +14,8 @@ config DRM_MSM
> >       select REGULATOR
> >       select DRM_KMS_HELPER
> >       select DRM_PANEL
> > +     select DRM_BRIDGE
> > +     select DRM_PANEL_BRIDGE
> >       select DRM_SCHED
> >       select SHMEM
> >       select TMPFS
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > index c41d39f5b7cf..e25877073d31 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > @@ -3,6 +3,8 @@
> >    * Copyright (c) 2015, The Linux Foundation. All rights reserved.
> >    */
> >
> > +#include "drm/drm_bridge_connector.h"
> > +
> >   #include "msm_kms.h"
> >   #include "dsi.h"
> >
> > @@ -688,10 +690,10 @@ struct drm_connector *msm_dsi_manager_ext_bridge_init(u8 id)
> >   {
> >       struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
> >       struct drm_device *dev = msm_dsi->dev;
> > +     struct drm_connector *connector;
> >       struct drm_encoder *encoder;
> >       struct drm_bridge *int_bridge, *ext_bridge;
> > -     struct drm_connector *connector;
> > -     struct list_head *connector_list;
> > +     int ret;
> >
> >       int_bridge = msm_dsi->bridge;
> >       ext_bridge = msm_dsi->external_bridge =
> > @@ -699,22 +701,44 @@ struct drm_connector *msm_dsi_manager_ext_bridge_init(u8 id)
> >
> >       encoder = msm_dsi->encoder;
> >
> > -     /* link the internal dsi bridge to the external bridge */
> > -     drm_bridge_attach(encoder, ext_bridge, int_bridge, 0);
> > -
> >       /*
> > -      * we need the drm_connector created by the external bridge
> > -      * driver (or someone else) to feed it to our driver's
> > -      * priv->connector[] list, mainly for msm_fbdev_init()
> > +      * Try first to create the bridge without it creating its own
> > +      * connector.. currently some bridges support this, and others
> > +      * do not (and some support both modes)
> >        */
> > -     connector_list = &dev->mode_config.connector_list;
> > +     ret = drm_bridge_attach(encoder, ext_bridge, int_bridge,
> > +                     DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> > +     if (ret == -EINVAL) {
> > +             struct drm_connector *connector;
> > +             struct list_head *connector_list;
> > +
> > +             /* link the internal dsi bridge to the external bridge */
> > +             drm_bridge_attach(encoder, ext_bridge, int_bridge, 0);
> > +
> > +             /*
> > +              * we need the drm_connector created by the external bridge
> > +              * driver (or someone else) to feed it to our driver's
> > +              * priv->connector[] list, mainly for msm_fbdev_init()
> > +              */
> > +             connector_list = &dev->mode_config.connector_list;
> >
> > -     list_for_each_entry(connector, connector_list, head) {
> > -             if (drm_connector_has_possible_encoder(connector, encoder))
> > -                     return connector;
> > +             list_for_each_entry(connector, connector_list, head) {
> > +                     if (drm_connector_has_possible_encoder(connector, encoder))
> > +                             return connector;
> > +             }
> > +
> > +             return ERR_PTR(-ENODEV);
> > +     }
> > +
> > +     connector = drm_bridge_connector_init(dev, encoder);
> > +     if (IS_ERR(connector)) {
> > +             DRM_ERROR("Unable to create bridge connector\n");
> > +             return ERR_CAST(connector);
> >       }
> >
> > -     return ERR_PTR(-ENODEV);
> > +     drm_connector_attach_encoder(connector, encoder);
> > +
> > +     return connector;
> >   }
> >
> >   void msm_dsi_manager_bridge_destroy(struct drm_bridge *bridge)
> >
>
>
> --
> With best wishes
> Dmitry

Re: [PATCH v2 3/3] drm/bridge: ti-sn65dsi86: Add NO_CONNECTOR support

[Doug Anderson]

Hi,

On Thu, Sep 23, 2021 at 7:26 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> > > >  err_conn_init:
> > > >       drm_dp_aux_unregister(&pdata->aux);
> > > >       return ret;
> > > > @@ -792,9 +790,30 @@ static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata)
> > > >       regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val);
> > > >  }
> > > >
> > > > +/*
> > > > + * Find the connector and fish out the bpc from display_info.  It would
> > > > + * be nice if we could get this instead from drm_bridge_state, but that
> > > > + * doesn't yet appear to be the case.
> > >
> > > You already have a bus format in the bridge state, from which you can
> > > derive the bpp. Could you give it a try ?
> >
> > Possibly the bridge should be converted to ->atomic_enable(), etc..
> > I'll leave that for another time
>
> It should be fairly straightforward, and would avoid the hack below.

Given this point of controversy, my inclination is to wait and not
apply this patch now. I don't think there's anything urgent here,
right? Worst case eventually Laurent might pick it up in his patch
series? At least we know it will work with the MSM driver once patch
#1 lands. :-)


-Doug

Re: [PATCH v2 3/3] drm/bridge: ti-sn65dsi86: Add NO_CONNECTOR support

[Doug Anderson]

Hi,

On Thu, Sep 23, 2021 at 7:26 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> > > >  err_conn_init:
> > > >       drm_dp_aux_unregister(&pdata->aux);
> > > >       return ret;
> > > > @@ -792,9 +790,30 @@ static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata)
> > > >       regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val);
> > > >  }
> > > >
> > > > +/*
> > > > + * Find the connector and fish out the bpc from display_info.  It would
> > > > + * be nice if we could get this instead from drm_bridge_state, but that
> > > > + * doesn't yet appear to be the case.
> > >
> > > You already have a bus format in the bridge state, from which you can
> > > derive the bpp. Could you give it a try ?
> >
> > Possibly the bridge should be converted to ->atomic_enable(), etc..
> > I'll leave that for another time
>
> It should be fairly straightforward, and would avoid the hack below.

Given this point of controversy, my inclination is to wait and not
apply this patch now. I don't think there's anything urgent here,
right? Worst case eventually Laurent might pick it up in his patch
series? At least we know it will work with the MSM driver once patch
#1 lands. :-)


-Doug

Re: [PATCH v2 3/3] drm/bridge: ti-sn65dsi86: Add NO_CONNECTOR support

[Laurent Pinchart]

Hi Doug,

On Fri, Oct 01, 2021 at 11:02:54AM -0700, Doug Anderson wrote:
> On Thu, Sep 23, 2021 at 7:26 PM Laurent Pinchart wrote:
> >
> > > > >  err_conn_init:
> > > > >       drm_dp_aux_unregister(&pdata->aux);
> > > > >       return ret;
> > > > > @@ -792,9 +790,30 @@ static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata)
> > > > >       regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val);
> > > > >  }
> > > > >
> > > > > +/*
> > > > > + * Find the connector and fish out the bpc from display_info.  It would
> > > > > + * be nice if we could get this instead from drm_bridge_state, but that
> > > > > + * doesn't yet appear to be the case.
> > > >
> > > > You already have a bus format in the bridge state, from which you can
> > > > derive the bpp. Could you give it a try ?
> > >
> > > Possibly the bridge should be converted to ->atomic_enable(), etc..
> > > I'll leave that for another time
> >
> > It should be fairly straightforward, and would avoid the hack below.
> 
> Given this point of controversy, my inclination is to wait and not
> apply this patch now. I don't think there's anything urgent here,
> right? Worst case eventually Laurent might pick it up in his patch
> series? At least we know it will work with the MSM driver once patch
> #1 lands. :-)

I've recorded the task for my upcoming work on the ti-sn65dsi86 driver.

-- 
Regards,

Laurent Pinchart