[PATCH 0/5] drm/tidss: Use new connector model for tidss

[PATCH 0/5] drm/tidss: Use new connector model for tidss

[Nikhil Devshatwar]

This series moves the tidss to using new connectoe model, where the 
SoC driver (tidss) creates the connector and all the bridges are 
attached with the flag DRM_BRIDGE_ATTACH_NO_CONNECTOR

Since the bridges do not create the connector, the bus format and
bus_flag is set after the format negotiation.
Support format negotiations in the tfp410 and mhdp bridge drivers.

Nikhil Devshatwar (5):
  drm/tidss: Move to newer connector model
  drm/tidss: Set bus_format correctly from bridge/connector
  drm: bridge: Propagate the bus flags from bridge->timings
  drm/bridge: tfp410: Support format negotiation
  drm/bridge: mhdp8564: Support format negotiation

 .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 29 +++++++++++++++++
 drivers/gpu/drm/bridge/ti-tfp410.c            | 32 +++++++++++++++++++
 drivers/gpu/drm/drm_bridge.c                  |  7 ++++
 drivers/gpu/drm/tidss/tidss_drv.h             |  3 ++
 drivers/gpu/drm/tidss/tidss_encoder.c         | 16 +++++++---
 drivers/gpu/drm/tidss/tidss_kms.c             | 15 ++++++++-
 6 files changed, 96 insertions(+), 6 deletions(-)

-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/5] drm/tidss: Move to newer connector model

[Nikhil Devshatwar]

To be able to support connector operations across multiple
bridges, it is recommended that the connector should be
created by the SoC driver instead of the bridges.

Modify the tidss modesetting initialization sequence to
create the connector and attach bridges with flag
DRM_BRIDGE_ATTACH_NO_CONNECTOR

Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
---
 drivers/gpu/drm/tidss/tidss_drv.h |  3 +++
 drivers/gpu/drm/tidss/tidss_kms.c | 15 ++++++++++++++-
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tidss/tidss_drv.h b/drivers/gpu/drm/tidss/tidss_drv.h
index 7de4bba52e6f..cfbf85a4d92b 100644
--- a/drivers/gpu/drm/tidss/tidss_drv.h
+++ b/drivers/gpu/drm/tidss/tidss_drv.h
@@ -27,6 +27,9 @@ struct tidss_device {
 	unsigned int num_planes;
 	struct drm_plane *planes[TIDSS_MAX_PLANES];
 
+	unsigned int num_connectors;
+	struct drm_connector *connectors[TIDSS_MAX_PORTS];
+
 	spinlock_t wait_lock;	/* protects the irq masks */
 	dispc_irq_t irq_mask;	/* enabled irqs in addition to wait_list */
 };
diff --git a/drivers/gpu/drm/tidss/tidss_kms.c b/drivers/gpu/drm/tidss/tidss_kms.c
index 09485c7f0d6f..51c24b4a6a21 100644
--- a/drivers/gpu/drm/tidss/tidss_kms.c
+++ b/drivers/gpu/drm/tidss/tidss_kms.c
@@ -7,6 +7,7 @@
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_bridge.h>
+#include <drm/drm_bridge_connector.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_fb_cma_helper.h>
 #include <drm/drm_fb_helper.h>
@@ -192,6 +193,7 @@ static int tidss_dispc_modeset_init(struct tidss_device *tidss)
 	for (i = 0; i < num_pipes; ++i) {
 		struct tidss_plane *tplane;
 		struct tidss_crtc *tcrtc;
+		struct drm_connector *connector;
 		struct drm_encoder *enc;
 		u32 hw_plane_id = feat->vid_order[tidss->num_planes];
 		int ret;
@@ -222,11 +224,22 @@ static int tidss_dispc_modeset_init(struct tidss_device *tidss)
 			return PTR_ERR(enc);
 		}
 
-		ret = drm_bridge_attach(enc, pipes[i].bridge, NULL, 0);
+		ret = drm_bridge_attach(enc, pipes[i].bridge, NULL,
+					DRM_BRIDGE_ATTACH_NO_CONNECTOR);
 		if (ret) {
 			dev_err(tidss->dev, "bridge attach failed: %d\n", ret);
 			return ret;
 		}
+
+		connector = drm_bridge_connector_init(&tidss->ddev, enc);
+		if (IS_ERR(connector)) {
+			dev_err(tidss->dev, "bridge_connector create failed\n");
+			return PTR_ERR(connector);
+		}
+
+		tidss->connectors[tidss->num_connectors++] = connector;
+
+		drm_connector_attach_encoder(connector, enc);
 	}
 
 	/* create overlay planes of the leftover planes */
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/5] drm/tidss: Set bus_format correctly from bridge/connector

[Nikhil Devshatwar]

When there is a chain of bridges attached to the encoder,
the bus_format should be ideally set from the input format of the
first bridge in the chain.

Use the bridge state to get the negotiated bus_format.
If the bridge does not support format negotiation, error out
and fail.

Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
---
 drivers/gpu/drm/tidss/tidss_encoder.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_encoder.c b/drivers/gpu/drm/tidss/tidss_encoder.c
index e278a9c89476..ae7f134754b7 100644
--- a/drivers/gpu/drm/tidss/tidss_encoder.c
+++ b/drivers/gpu/drm/tidss/tidss_encoder.c
@@ -22,6 +22,7 @@ static int tidss_encoder_atomic_check(struct drm_encoder *encoder,
 	struct drm_device *ddev = encoder->dev;
 	struct tidss_crtc_state *tcrtc_state = to_tidss_crtc_state(crtc_state);
 	struct drm_display_info *di = &conn_state->connector->display_info;
+	struct drm_bridge_state *bstate;
 	struct drm_bridge *bridge;
 	bool bus_flags_set = false;
 
@@ -41,14 +42,19 @@ static int tidss_encoder_atomic_check(struct drm_encoder *encoder,
 		break;
 	}
 
-	if (!di->bus_formats || di->num_bus_formats == 0)  {
-		dev_err(ddev->dev, "%s: No bus_formats in connected display\n",
-			__func__);
+	/* Copy the bus_format from the input_bus_format of first bridge */
+	bridge = drm_bridge_chain_get_first_bridge(encoder);
+	bstate = drm_atomic_get_new_bridge_state(crtc_state->state, bridge);
+	if (bstate)
+		tcrtc_state->bus_format = bstate->input_bus_cfg.format;
+
+	if (tcrtc_state->bus_format == 0 ||
+	    tcrtc_state->bus_format == MEDIA_BUS_FMT_FIXED) {
+
+		dev_err(ddev->dev, "Bridge connected to the encoder did not specify media bus format\n");
 		return -EINVAL;
 	}
 
-	// XXX any cleaner way to set bus format and flags?
-	tcrtc_state->bus_format = di->bus_formats[0];
 	if (!bus_flags_set)
 		tcrtc_state->bus_flags = di->bus_flags;
 
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/5] drm: bridge: Propagate the bus flags from bridge->timings

[Nikhil Devshatwar]

When the next bridge does not specify any bus flags, use the
bridge->timings->input_bus_flags as fallback when propagating
bus flags from next bridge to current bridge.

Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
---
 drivers/gpu/drm/drm_bridge.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 64f0effb52ac..8353723323ab 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -975,6 +975,13 @@ drm_atomic_bridge_propagate_bus_flags(struct drm_bridge *bridge,
 	 * duplicate the "dummy propagation" logic.
 	 */
 	bridge_state->input_bus_cfg.flags = output_flags;
+
+	/*
+	 * Use the bridge->timings->input_bus_flags as fallback if the next bridge
+	 * does not specify the flags
+	 */
+	if (!bridge_state->input_bus_cfg.flags)
+		bridge_state->input_bus_cfg.flags = bridge->timings->input_bus_flags;
 }
 
 /**
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/5] drm/bridge: tfp410: Support format negotiation

[Nikhil Devshatwar]

With new connector model, tfp410 will not create the connector and
SoC driver will rely on format negotiation to setup the encoder format.

Support format negotiations hooks in the drm_bridge_funcs.
Use helper functions for state management.
Support one of the two RGB formats as selected from DT bindings.

Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
---
 drivers/gpu/drm/bridge/ti-tfp410.c | 32 ++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c
index ba3fa2a9b8a4..b65e48e080c7 100644
--- a/drivers/gpu/drm/bridge/ti-tfp410.c
+++ b/drivers/gpu/drm/bridge/ti-tfp410.c
@@ -204,12 +204,44 @@ static enum drm_mode_status tfp410_mode_valid(struct drm_bridge *bridge,
 	return MODE_OK;
 }
 
+static u32 *tfp410_get_input_bus_fmts(struct drm_bridge *bridge,
+				      struct drm_bridge_state *bridge_state,
+				      struct drm_crtc_state *crtc_state,
+				      struct drm_connector_state *conn_state,
+				      u32 output_fmt,
+				      unsigned int *num_input_fmts)
+{
+	struct tfp410 *dvi = drm_bridge_to_tfp410(bridge);
+	u32 *input_fmts;
+
+	*num_input_fmts = 0;
+
+	/*
+	 * This bridge does not support media_bus_format conversion
+	 * Propagate only if supported
+	 */
+	if (output_fmt != dvi->bus_format && output_fmt != MEDIA_BUS_FMT_FIXED)
+		return NULL;
+
+	input_fmts = kzalloc(sizeof(*input_fmts), GFP_KERNEL);
+	if (!input_fmts)
+		return NULL;
+
+	*num_input_fmts = 1;
+	input_fmts[0] = dvi->bus_format;
+	return input_fmts;
+}
+
 static const struct drm_bridge_funcs tfp410_bridge_funcs = {
 	.attach		= tfp410_attach,
 	.detach		= tfp410_detach,
 	.enable		= tfp410_enable,
 	.disable	= tfp410_disable,
 	.mode_valid	= tfp410_mode_valid,
+	.atomic_reset = drm_atomic_helper_bridge_reset,
+	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
+	.atomic_get_input_bus_fmts = tfp410_get_input_bus_fmts,
 };
 
 static const struct drm_bridge_timings tfp410_default_timings = {
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 5/5] drm/bridge: mhdp8564: Support format negotiation

[Nikhil Devshatwar]

With new connector model, mhdp bridge will not create the connector and
SoC driver will rely on format negotiation to setup the encoder format.

Support format negotiations hooks in the drm_bridge_funcs.
Support a single format for input.

Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
---
 .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 29 +++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
index d0c65610ebb5..230f6e28f82f 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
@@ -2078,6 +2078,34 @@ cdns_mhdp_bridge_atomic_reset(struct drm_bridge *bridge)
 	return &cdns_mhdp_state->base;
 }
 
+static u32 *cdns_mhdp_get_input_bus_fmts(struct drm_bridge *bridge,
+				      struct drm_bridge_state *bridge_state,
+				      struct drm_crtc_state *crtc_state,
+				      struct drm_connector_state *conn_state,
+				      u32 output_fmt,
+				      unsigned int *num_input_fmts)
+{
+	u32 *input_fmts;
+	u32 default_bus_format = MEDIA_BUS_FMT_RGB121212_1X36;
+
+	*num_input_fmts = 0;
+
+	/*
+	 * This bridge does not support media_bus_format conversion
+	 * Propagate only if supported
+	 */
+	if (output_fmt != default_bus_format && output_fmt != MEDIA_BUS_FMT_FIXED)
+		return NULL;
+
+	input_fmts = kzalloc(sizeof(*input_fmts), GFP_KERNEL);
+	if (!input_fmts)
+		return NULL;
+
+	*num_input_fmts = 1;
+	input_fmts[0] = default_bus_format;
+	return input_fmts;
+}
+
 static int cdns_mhdp_atomic_check(struct drm_bridge *bridge,
 				  struct drm_bridge_state *bridge_state,
 				  struct drm_crtc_state *crtc_state,
@@ -2142,6 +2170,7 @@ static const struct drm_bridge_funcs cdns_mhdp_bridge_funcs = {
 	.atomic_duplicate_state = cdns_mhdp_bridge_atomic_duplicate_state,
 	.atomic_destroy_state = cdns_mhdp_bridge_atomic_destroy_state,
 	.atomic_reset = cdns_mhdp_bridge_atomic_reset,
+	.atomic_get_input_bus_fmts = cdns_mhdp_get_input_bus_fmts,
 	.detect = cdns_mhdp_bridge_detect,
 	.get_edid = cdns_mhdp_bridge_get_edid,
 	.hpd_enable = cdns_mhdp_bridge_hpd_enable,
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/5] drm/tidss: Use new connector model for tidss

[Tomi Valkeinen]

Hi Nikhil,

On 16/10/2020 13:39, Nikhil Devshatwar wrote:
> This series moves the tidss to using new connectoe model, where the 
> SoC driver (tidss) creates the connector and all the bridges are 
> attached with the flag DRM_BRIDGE_ATTACH_NO_CONNECTOR
> 
> Since the bridges do not create the connector, the bus format and
> bus_flag is set after the format negotiation.
> Support format negotiations in the tfp410 and mhdp bridge drivers.
> 
> Nikhil Devshatwar (5):
>   drm/tidss: Move to newer connector model
>   drm/tidss: Set bus_format correctly from bridge/connector
>   drm: bridge: Propagate the bus flags from bridge->timings
>   drm/bridge: tfp410: Support format negotiation
>   drm/bridge: mhdp8564: Support format negotiation

I think the patch order could be a bit different. If you first change the tidss to use the new
connector model, and only afterwards fix the bridges we use, then there's a time when the displays
won't work.

Also, with J7 EVM and DP, when I unload the modules I see:

[   43.238895] irq 31: nobody cared (try booting with the "irqpoll" option)
[   43.245591] CPU: 0 PID: 349 Comm: irq/31-mhdp8546 Not tainted 5.9.0-rc5-00605-g08a291316f86 #4
[   43.254186] Hardware name: Texas Instruments K3 J721E SoC (DT)
[   43.260006] Call trace:
[   43.262453]  dump_backtrace+0x0/0x1d8
[   43.266107]  show_stack+0x18/0x28
[   43.269416]  dump_stack+0xe0/0x14c
[   43.272810]  __report_bad_irq+0x4c/0xdc
[   43.276637]  note_interrupt+0x2cc/0x388
[   43.280465]  handle_irq_event_percpu+0x88/0x90
[   43.284898]  handle_irq_event+0x48/0xf8
[   43.288725]  handle_fasteoi_irq+0xcc/0x180
[   43.292811]  generic_handle_irq+0x30/0x48
[   43.296811]  __handle_domain_irq+0x94/0x108
[   43.300986]  gic_handle_irq+0x60/0x158
[   43.304726]  el1_irq+0xbc/0x180
[   43.307862]  _raw_spin_unlock_irq+0x48/0x90
[   43.312035]  irq_finalize_oneshot.part.0+0x68/0x108
[   43.316903]  irq_thread_fn+0x60/0xa0
[   43.320469]  irq_thread+0x1b8/0x248
[   43.323949]  kthread+0x128/0x160
[   43.327169]  ret_from_fork+0x10/0x34
[   43.330735] handlers:
[   43.333020] [<000000005367c4f9>] irq_default_primary_handler threaded [<000000007e02b601>]
cdns_mhdp_irq_handler [cdns_mhdp8546]
[   43.344607] Disabling IRQ #31

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/5] drm/tidss: Move to newer connector model

[Tomi Valkeinen]

On 16/10/2020 13:39, Nikhil Devshatwar wrote:
> To be able to support connector operations across multiple
> bridges, it is recommended that the connector should be
> created by the SoC driver instead of the bridges.
> 
> Modify the tidss modesetting initialization sequence to
> create the connector and attach bridges with flag
> DRM_BRIDGE_ATTACH_NO_CONNECTOR
> 
> Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
> ---
>  drivers/gpu/drm/tidss/tidss_drv.h |  3 +++
>  drivers/gpu/drm/tidss/tidss_kms.c | 15 ++++++++++++++-
>  2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_drv.h b/drivers/gpu/drm/tidss/tidss_drv.h
> index 7de4bba52e6f..cfbf85a4d92b 100644
> --- a/drivers/gpu/drm/tidss/tidss_drv.h
> +++ b/drivers/gpu/drm/tidss/tidss_drv.h
> @@ -27,6 +27,9 @@ struct tidss_device {
>  	unsigned int num_planes;
>  	struct drm_plane *planes[TIDSS_MAX_PLANES];
>  
> +	unsigned int num_connectors;
> +	struct drm_connector *connectors[TIDSS_MAX_PORTS];
> +
>  	spinlock_t wait_lock;	/* protects the irq masks */
>  	dispc_irq_t irq_mask;	/* enabled irqs in addition to wait_list */
>  };
> diff --git a/drivers/gpu/drm/tidss/tidss_kms.c b/drivers/gpu/drm/tidss/tidss_kms.c
> index 09485c7f0d6f..51c24b4a6a21 100644
> --- a/drivers/gpu/drm/tidss/tidss_kms.c
> +++ b/drivers/gpu/drm/tidss/tidss_kms.c
> @@ -7,6 +7,7 @@
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_bridge.h>
> +#include <drm/drm_bridge_connector.h>
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_fb_cma_helper.h>
>  #include <drm/drm_fb_helper.h>
> @@ -192,6 +193,7 @@ static int tidss_dispc_modeset_init(struct tidss_device *tidss)
>  	for (i = 0; i < num_pipes; ++i) {
>  		struct tidss_plane *tplane;
>  		struct tidss_crtc *tcrtc;
> +		struct drm_connector *connector;
>  		struct drm_encoder *enc;
>  		u32 hw_plane_id = feat->vid_order[tidss->num_planes];
>  		int ret;
> @@ -222,11 +224,22 @@ static int tidss_dispc_modeset_init(struct tidss_device *tidss)
>  			return PTR_ERR(enc);
>  		}
>  
> -		ret = drm_bridge_attach(enc, pipes[i].bridge, NULL, 0);
> +		ret = drm_bridge_attach(enc, pipes[i].bridge, NULL,
> +					DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>  		if (ret) {
>  			dev_err(tidss->dev, "bridge attach failed: %d\n", ret);
>  			return ret;
>  		}
> +
> +		connector = drm_bridge_connector_init(&tidss->ddev, enc);
> +		if (IS_ERR(connector)) {
> +			dev_err(tidss->dev, "bridge_connector create failed\n");
> +			return PTR_ERR(connector);
> +		}
> +
> +		tidss->connectors[tidss->num_connectors++] = connector;
> +
> +		drm_connector_attach_encoder(connector, enc);

This call may return an error.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/5] drm: bridge: Propagate the bus flags from bridge->timings

[Tomi Valkeinen]

On 16/10/2020 13:39, Nikhil Devshatwar wrote:
> When the next bridge does not specify any bus flags, use the
> bridge->timings->input_bus_flags as fallback when propagating
> bus flags from next bridge to current bridge.
> 
> Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
> ---
>  drivers/gpu/drm/drm_bridge.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 64f0effb52ac..8353723323ab 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -975,6 +975,13 @@ drm_atomic_bridge_propagate_bus_flags(struct drm_bridge *bridge,
>  	 * duplicate the "dummy propagation" logic.
>  	 */
>  	bridge_state->input_bus_cfg.flags = output_flags;
> +
> +	/*
> +	 * Use the bridge->timings->input_bus_flags as fallback if the next bridge
> +	 * does not specify the flags
> +	 */
> +	if (!bridge_state->input_bus_cfg.flags)
> +		bridge_state->input_bus_cfg.flags = bridge->timings->input_bus_flags;

According to docs, timings can be NULL.

And, hmm... It's too easy to get confused with these, but... If the bridge defines timings, and
timings->input_bus_flags != 0, should we always pick that, even if we got something via
output_flags? Logic being, if this bridge defines timings->input_bus_flags, it probably wants that
to be used regardless whether we got something from the next bridge.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/5] drm/tidss: Use new connector model for tidss

[Nikhil Devshatwar]

On 15:11-20201019, Tomi Valkeinen wrote:
> Hi Nikhil,
> 
> On 16/10/2020 13:39, Nikhil Devshatwar wrote:
> > This series moves the tidss to using new connectoe model, where the 
> > SoC driver (tidss) creates the connector and all the bridges are 
> > attached with the flag DRM_BRIDGE_ATTACH_NO_CONNECTOR
> > 
> > Since the bridges do not create the connector, the bus format and
> > bus_flag is set after the format negotiation.
> > Support format negotiations in the tfp410 and mhdp bridge drivers.
> > 
> > Nikhil Devshatwar (5):
> >   drm/tidss: Move to newer connector model
> >   drm/tidss: Set bus_format correctly from bridge/connector
> >   drm: bridge: Propagate the bus flags from bridge->timings
> >   drm/bridge: tfp410: Support format negotiation
> >   drm/bridge: mhdp8564: Support format negotiation
> 
> I think the patch order could be a bit different. If you first change the tidss to use the new
> connector model, and only afterwards fix the bridges we use, then there's a time when the displays
> won't work.
> 
Agreed. I will change the order in next version

> Also, with J7 EVM and DP, when I unload the modules I see:
> 
I confirm the same issue.
I doubt if it is because of the change I did.
Will try to revert the patches and confirm again

> [   43.238895] irq 31: nobody cared (try booting with the "irqpoll" option)
> [   43.245591] CPU: 0 PID: 349 Comm: irq/31-mhdp8546 Not tainted 5.9.0-rc5-00605-g08a291316f86 #4
> [   43.254186] Hardware name: Texas Instruments K3 J721E SoC (DT)
> [   43.260006] Call trace:
> [   43.262453]  dump_backtrace+0x0/0x1d8
> [   43.266107]  show_stack+0x18/0x28
> [   43.269416]  dump_stack+0xe0/0x14c
> [   43.272810]  __report_bad_irq+0x4c/0xdc
> [   43.276637]  note_interrupt+0x2cc/0x388
> [   43.280465]  handle_irq_event_percpu+0x88/0x90
> [   43.284898]  handle_irq_event+0x48/0xf8
> [   43.288725]  handle_fasteoi_irq+0xcc/0x180
> [   43.292811]  generic_handle_irq+0x30/0x48
> [   43.296811]  __handle_domain_irq+0x94/0x108
> [   43.300986]  gic_handle_irq+0x60/0x158
> [   43.304726]  el1_irq+0xbc/0x180
> [   43.307862]  _raw_spin_unlock_irq+0x48/0x90
> [   43.312035]  irq_finalize_oneshot.part.0+0x68/0x108
> [   43.316903]  irq_thread_fn+0x60/0xa0
> [   43.320469]  irq_thread+0x1b8/0x248
> [   43.323949]  kthread+0x128/0x160
> [   43.327169]  ret_from_fork+0x10/0x34
> [   43.330735] handlers:
> [   43.333020] [<000000005367c4f9>] irq_default_primary_handler threaded [<000000007e02b601>]
> cdns_mhdp_irq_handler [cdns_mhdp8546]
> [   43.344607] Disabling IRQ #31
> 
>  Tomi
> 
> -- 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/5] drm: bridge: Propagate the bus flags from bridge->timings

[Nikhil Devshatwar]

On 14:31-20201021, Tomi Valkeinen wrote:
> On 16/10/2020 13:39, Nikhil Devshatwar wrote:
> > When the next bridge does not specify any bus flags, use the
> > bridge->timings->input_bus_flags as fallback when propagating
> > bus flags from next bridge to current bridge.
> > 
> > Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
> > ---
> >  drivers/gpu/drm/drm_bridge.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index 64f0effb52ac..8353723323ab 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -975,6 +975,13 @@ drm_atomic_bridge_propagate_bus_flags(struct drm_bridge *bridge,
> >  	 * duplicate the "dummy propagation" logic.
> >  	 */
> >  	bridge_state->input_bus_cfg.flags = output_flags;
> > +
> > +	/*
> > +	 * Use the bridge->timings->input_bus_flags as fallback if the next bridge
> > +	 * does not specify the flags
> > +	 */
> > +	if (!bridge_state->input_bus_cfg.flags)
> > +		bridge_state->input_bus_cfg.flags = bridge->timings->input_bus_flags;
> 
> According to docs, timings can be NULL.
> 
> And, hmm... It's too easy to get confused with these, but... If the bridge defines timings, and
> timings->input_bus_flags != 0, should we always pick that, even if we got something via
> output_flags? Logic being, if this bridge defines timings->input_bus_flags, it probably wants that
> to be used regardless whether we got something from the next bridge.

Got it, the flags from timings if present should be prioritized rather
than treating them as fallback.

Nikhil D

> 
>  Tomi
> 
> -- 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [PATCH 5/5] drm/bridge: mhdp8564: Support format negotiation

[Swapnil Kashinath Jakhade]

Hi,

> -----Original Message-----
> From: Nikhil Devshatwar <nikhil.nd@ti.com>
> Sent: Friday, October 16, 2020 4:09 PM
> To: dri-devel@lists.freedesktop.org; Tomi Valkeinen
> <tomi.valkeinen@ti.com>
> Cc: Sekhar Nori <nsekhar@ti.com>; Laurent Pinchart
> <laurent.pinchart@ideasonboard.com>; Swapnil Kashinath Jakhade
> <sjakhade@cadence.com>
> Subject: [PATCH 5/5] drm/bridge: mhdp8564: Support format negotiation
> 

s/mhdp8564/mhdp8546

> EXTERNAL MAIL
> 
> 
> With new connector model, mhdp bridge will not create the connector and
> SoC driver will rely on format negotiation to setup the encoder format.
> 
> Support format negotiations hooks in the drm_bridge_funcs.
> Support a single format for input.
> 
> Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
> ---
>  .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 29 +++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> index d0c65610ebb5..230f6e28f82f 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> @@ -2078,6 +2078,34 @@ cdns_mhdp_bridge_atomic_reset(struct
> drm_bridge *bridge)
>  	return &cdns_mhdp_state->base;
>  }
> 
> +static u32 *cdns_mhdp_get_input_bus_fmts(struct drm_bridge *bridge,
> +				      struct drm_bridge_state *bridge_state,
> +				      struct drm_crtc_state *crtc_state,
> +				      struct drm_connector_state *conn_state,
> +				      u32 output_fmt,
> +				      unsigned int *num_input_fmts) {
> +	u32 *input_fmts;
> +	u32 default_bus_format = MEDIA_BUS_FMT_RGB121212_1X36;
> +
> +	*num_input_fmts = 0;
> +
> +	/*
> +	 * This bridge does not support media_bus_format conversion
> +	 * Propagate only if supported
> +	 */
> +	if (output_fmt != default_bus_format && output_fmt !=
> MEDIA_BUS_FMT_FIXED)
> +		return NULL;
> +
> +	input_fmts = kzalloc(sizeof(*input_fmts), GFP_KERNEL);
> +	if (!input_fmts)
> +		return NULL;
> +
> +	*num_input_fmts = 1;
> +	input_fmts[0] = default_bus_format;
> +	return input_fmts;
> +}
> +
>  static int cdns_mhdp_atomic_check(struct drm_bridge *bridge,
>  				  struct drm_bridge_state *bridge_state,
>  				  struct drm_crtc_state *crtc_state, @@ -
> 2142,6 +2170,7 @@ static const struct drm_bridge_funcs
> cdns_mhdp_bridge_funcs = {
>  	.atomic_duplicate_state =
> cdns_mhdp_bridge_atomic_duplicate_state,
>  	.atomic_destroy_state = cdns_mhdp_bridge_atomic_destroy_state,
>  	.atomic_reset = cdns_mhdp_bridge_atomic_reset,
> +	.atomic_get_input_bus_fmts = cdns_mhdp_get_input_bus_fmts,
>  	.detect = cdns_mhdp_bridge_detect,
>  	.get_edid = cdns_mhdp_bridge_get_edid,
>  	.hpd_enable = cdns_mhdp_bridge_hpd_enable,
> --
> 2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/5] drm/tidss: Use new connector model for tidss

[Tomi Valkeinen]

On 28/10/2020 16:19, Nikhil Devshatwar wrote:

>> Also, with J7 EVM and DP, when I unload the modules I see:
>>
> I confirm the same issue.
> I doubt if it is because of the change I did.
> Will try to revert the patches and confirm again
My guess is that it's not your patches as such, but that the mhdp driver does not do irq related
cleanups properly and your patches bring the issue up.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/5] drm/bridge: tfp410: Support format negotiation

[Laurent Pinchart]

Hi Nikhil,

Thank you for the patch.

On Fri, Oct 16, 2020 at 04:09:16PM +0530, Nikhil Devshatwar wrote:
> With new connector model, tfp410 will not create the connector and
> SoC driver will rely on format negotiation to setup the encoder format.
> 
> Support format negotiations hooks in the drm_bridge_funcs.
> Use helper functions for state management.
> Support one of the two RGB formats as selected from DT bindings.
> 
> Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
> ---
>  drivers/gpu/drm/bridge/ti-tfp410.c | 32 ++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c
> index ba3fa2a9b8a4..b65e48e080c7 100644
> --- a/drivers/gpu/drm/bridge/ti-tfp410.c
> +++ b/drivers/gpu/drm/bridge/ti-tfp410.c
> @@ -204,12 +204,44 @@ static enum drm_mode_status tfp410_mode_valid(struct drm_bridge *bridge,
>  	return MODE_OK;
>  }
>  
> +static u32 *tfp410_get_input_bus_fmts(struct drm_bridge *bridge,
> +				      struct drm_bridge_state *bridge_state,
> +				      struct drm_crtc_state *crtc_state,
> +				      struct drm_connector_state *conn_state,
> +				      u32 output_fmt,
> +				      unsigned int *num_input_fmts)
> +{
> +	struct tfp410 *dvi = drm_bridge_to_tfp410(bridge);
> +	u32 *input_fmts;
> +
> +	*num_input_fmts = 0;
> +
> +	/*
> +	 * This bridge does not support media_bus_format conversion
> +	 * Propagate only if supported
> +	 */
> +	if (output_fmt != dvi->bus_format && output_fmt != MEDIA_BUS_FMT_FIXED)
> +		return NULL;

On the output side, DVI transmits RGB24 data over three TMDS links (plus
one link for the clock). There's no media bus format that specifically
describe this, but among the possible values for dvi->bus_format
(MEDIA_BUS_FMT_RGB888_2X12_LE and MEDIA_BUS_FMT_RGB888_1X24),
MEDIA_BUS_FMT_RGB888_2X12_LE doesn't make any sense to describe the
output. We probably would need to introduce a specific media bus format
if we wanted to describe this accurately
(MEDIA_BUS_FMT_RGB888_DVI_SINGLE ?), which seems a bit overkill to
support single link DVI. If we take dual link DVI into account, more bit
depths are supported, as well as faster rates by transmitting to RGB888
pixels per clock, so more codes would be needed.

With support for single-link DVI only, we could decide, subsystem-wide,
to always use MEDIA_BUS_FMT_FIXED for DVI. We could also decide to
additional accept MEDIA_BUS_FMT_RGB888_1X24 to describe single-link DVI,
as a convention.

If the goal of the above check is to make the format negotiation fail
when the desired output format can't be supported by the TFP410, then I
would use

	if (output_fmt != dvi->MEDIA_BUS_FMT_RGB888_1X24 &&
	    output_fmt != MEDIA_BUS_FMT_FIXED)
		return NULL;

or simply

	if (output_fmt != MEDIA_BUS_FMT_FIXED)
		return NULL;

depending on what convention we enforce in the subsystem for DVI media
bus formats. I however wonder if this is needed at all, are there cases
where the output could support multiple bus formats and the tfp410
driver would need to make sure that only the 24-bit single link DVI gets
selected ? I suppose there are if we consider dual link DVI, but the DVI
connector driver (drivers/gpu/drm/bridge/display-connector.c) doesn't
report bus formats anyway.

> +
> +	input_fmts = kzalloc(sizeof(*input_fmts), GFP_KERNEL);
> +	if (!input_fmts)
> +		return NULL;
> +
> +	*num_input_fmts = 1;
> +	input_fmts[0] = dvi->bus_format;
> +	return input_fmts;
> +}
> +
>  static const struct drm_bridge_funcs tfp410_bridge_funcs = {
>  	.attach		= tfp410_attach,
>  	.detach		= tfp410_detach,
>  	.enable		= tfp410_enable,
>  	.disable	= tfp410_disable,
>  	.mode_valid	= tfp410_mode_valid,
> +	.atomic_reset = drm_atomic_helper_bridge_reset,
> +	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> +	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> +	.atomic_get_input_bus_fmts = tfp410_get_input_bus_fmts,
>  };
>  
>  static const struct drm_bridge_timings tfp410_default_timings = {

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] drm/bridge: mhdp8564: Support format negotiation

[Laurent Pinchart]

Hi Nikhil,

Thank you for the patch.

On Fri, Oct 16, 2020 at 04:09:17PM +0530, Nikhil Devshatwar wrote:
> With new connector model, mhdp bridge will not create the connector and
> SoC driver will rely on format negotiation to setup the encoder format.
> 
> Support format negotiations hooks in the drm_bridge_funcs.
> Support a single format for input.
> 
> Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
> ---
>  .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 29 +++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> index d0c65610ebb5..230f6e28f82f 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> @@ -2078,6 +2078,34 @@ cdns_mhdp_bridge_atomic_reset(struct drm_bridge *bridge)
>  	return &cdns_mhdp_state->base;
>  }
>  
> +static u32 *cdns_mhdp_get_input_bus_fmts(struct drm_bridge *bridge,
> +				      struct drm_bridge_state *bridge_state,
> +				      struct drm_crtc_state *crtc_state,
> +				      struct drm_connector_state *conn_state,
> +				      u32 output_fmt,
> +				      unsigned int *num_input_fmts)
> +{
> +	u32 *input_fmts;
> +	u32 default_bus_format = MEDIA_BUS_FMT_RGB121212_1X36;

Display port supports quite a few different formats. See my reply to
4/5, except it's worse in the DP case :-) Especially given that multiple
displays need to be taken into account. I'm afraid we need to decide how
to map media bus formats to different DP use cases, possibly adding new
bus formats as part of this exercise, and then revisit this patch.

> +
> +	*num_input_fmts = 0;
> +
> +	/*
> +	 * This bridge does not support media_bus_format conversion
> +	 * Propagate only if supported
> +	 */
> +	if (output_fmt != default_bus_format && output_fmt != MEDIA_BUS_FMT_FIXED)
> +		return NULL;
> +
> +	input_fmts = kzalloc(sizeof(*input_fmts), GFP_KERNEL);
> +	if (!input_fmts)
> +		return NULL;
> +
> +	*num_input_fmts = 1;
> +	input_fmts[0] = default_bus_format;
> +	return input_fmts;
> +}
> +
>  static int cdns_mhdp_atomic_check(struct drm_bridge *bridge,
>  				  struct drm_bridge_state *bridge_state,
>  				  struct drm_crtc_state *crtc_state,
> @@ -2142,6 +2170,7 @@ static const struct drm_bridge_funcs cdns_mhdp_bridge_funcs = {
>  	.atomic_duplicate_state = cdns_mhdp_bridge_atomic_duplicate_state,
>  	.atomic_destroy_state = cdns_mhdp_bridge_atomic_destroy_state,
>  	.atomic_reset = cdns_mhdp_bridge_atomic_reset,
> +	.atomic_get_input_bus_fmts = cdns_mhdp_get_input_bus_fmts,
>  	.detect = cdns_mhdp_bridge_detect,
>  	.get_edid = cdns_mhdp_bridge_get_edid,
>  	.hpd_enable = cdns_mhdp_bridge_hpd_enable,

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/5] drm: bridge: Propagate the bus flags from bridge->timings

[Laurent Pinchart]

Hello,

On Wed, Oct 28, 2020 at 08:04:53PM +0530, Nikhil Devshatwar wrote:
> On 14:31-20201021, Tomi Valkeinen wrote:
> > On 16/10/2020 13:39, Nikhil Devshatwar wrote:
> > > When the next bridge does not specify any bus flags, use the
> > > bridge->timings->input_bus_flags as fallback when propagating
> > > bus flags from next bridge to current bridge.
> > > 
> > > Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
> > > ---
> > >  drivers/gpu/drm/drm_bridge.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > > index 64f0effb52ac..8353723323ab 100644
> > > --- a/drivers/gpu/drm/drm_bridge.c
> > > +++ b/drivers/gpu/drm/drm_bridge.c
> > > @@ -975,6 +975,13 @@ drm_atomic_bridge_propagate_bus_flags(struct drm_bridge *bridge,
> > >  	 * duplicate the "dummy propagation" logic.
> > >  	 */
> > >  	bridge_state->input_bus_cfg.flags = output_flags;
> > > +
> > > +	/*
> > > +	 * Use the bridge->timings->input_bus_flags as fallback if the next bridge
> > > +	 * does not specify the flags
> > > +	 */
> > > +	if (!bridge_state->input_bus_cfg.flags)
> > > +		bridge_state->input_bus_cfg.flags = bridge->timings->input_bus_flags;
> > 
> > According to docs, timings can be NULL.

Correct.

> > And, hmm... It's too easy to get confused with these, but... If the bridge defines timings, and
> > timings->input_bus_flags != 0, should we always pick that, even if we got something via
> > output_flags? Logic being, if this bridge defines timings->input_bus_flags, it probably wants that
> > to be used regardless whether we got something from the next bridge.

timings->input_bus_flags is an API that predates format and flags
propagation along the pipeline. I would assume that drivers that
implement propagation should do so in a way that prioritize that API,
and either not report flags in the timings (in which case giving
priority to one or another wouldn't make a difference as both would
never be provided together), or would report flags in the timings as a
best effort fallback for display controller drivers that would look at
them exclusively without supporting the new API. I would thus think that
the flags obtained through format negotiation should be prioritized.

> Got it, the flags from timings if present should be prioritized rather
> than treating them as fallback.

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/5] drm/tidss: Move to newer connector model

[Laurent Pinchart]

Hi Nikhil,

Thank you for the patch.

On Fri, Oct 16, 2020 at 04:09:13PM +0530, Nikhil Devshatwar wrote:
> To be able to support connector operations across multiple
> bridges, it is recommended that the connector should be
> created by the SoC driver instead of the bridges.
> 
> Modify the tidss modesetting initialization sequence to
> create the connector and attach bridges with flag
> DRM_BRIDGE_ATTACH_NO_CONNECTOR
> 
> Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
> ---
>  drivers/gpu/drm/tidss/tidss_drv.h |  3 +++
>  drivers/gpu/drm/tidss/tidss_kms.c | 15 ++++++++++++++-
>  2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_drv.h b/drivers/gpu/drm/tidss/tidss_drv.h
> index 7de4bba52e6f..cfbf85a4d92b 100644
> --- a/drivers/gpu/drm/tidss/tidss_drv.h
> +++ b/drivers/gpu/drm/tidss/tidss_drv.h
> @@ -27,6 +27,9 @@ struct tidss_device {
>  	unsigned int num_planes;
>  	struct drm_plane *planes[TIDSS_MAX_PLANES];
>  
> +	unsigned int num_connectors;
> +	struct drm_connector *connectors[TIDSS_MAX_PORTS];
> +
>  	spinlock_t wait_lock;	/* protects the irq masks */
>  	dispc_irq_t irq_mask;	/* enabled irqs in addition to wait_list */
>  };
> diff --git a/drivers/gpu/drm/tidss/tidss_kms.c b/drivers/gpu/drm/tidss/tidss_kms.c
> index 09485c7f0d6f..51c24b4a6a21 100644
> --- a/drivers/gpu/drm/tidss/tidss_kms.c
> +++ b/drivers/gpu/drm/tidss/tidss_kms.c
> @@ -7,6 +7,7 @@
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_bridge.h>
> +#include <drm/drm_bridge_connector.h>
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_fb_cma_helper.h>
>  #include <drm/drm_fb_helper.h>
> @@ -192,6 +193,7 @@ static int tidss_dispc_modeset_init(struct tidss_device *tidss)
>  	for (i = 0; i < num_pipes; ++i) {
>  		struct tidss_plane *tplane;
>  		struct tidss_crtc *tcrtc;
> +		struct drm_connector *connector;
>  		struct drm_encoder *enc;
>  		u32 hw_plane_id = feat->vid_order[tidss->num_planes];
>  		int ret;
> @@ -222,11 +224,22 @@ static int tidss_dispc_modeset_init(struct tidss_device *tidss)
>  			return PTR_ERR(enc);
>  		}
>  
> -		ret = drm_bridge_attach(enc, pipes[i].bridge, NULL, 0);
> +		ret = drm_bridge_attach(enc, pipes[i].bridge, NULL,
> +					DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>  		if (ret) {
>  			dev_err(tidss->dev, "bridge attach failed: %d\n", ret);
>  			return ret;
>  		}
> +
> +		connector = drm_bridge_connector_init(&tidss->ddev, enc);
> +		if (IS_ERR(connector)) {
> +			dev_err(tidss->dev, "bridge_connector create failed\n");
> +			return PTR_ERR(connector);
> +		}
> +
> +		tidss->connectors[tidss->num_connectors++] = connector;
> +
> +		drm_connector_attach_encoder(connector, enc);

Apart from the issue reported by Tomi, the patch looks goood to me. Fix
this fixed, and the series reordered to move this to the end,

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

>  	}
>  
>  	/* create overlay planes of the leftover planes */

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/5] drm/tidss: Set bus_format correctly from bridge/connector

[Laurent Pinchart]

Hi Nikhil,

Thank you for the patch.

On Fri, Oct 16, 2020 at 04:09:14PM +0530, Nikhil Devshatwar wrote:
> When there is a chain of bridges attached to the encoder,
> the bus_format should be ideally set from the input format of the
> first bridge in the chain.
> 
> Use the bridge state to get the negotiated bus_format.
> If the bridge does not support format negotiation, error out
> and fail.
> 
> Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
> ---
>  drivers/gpu/drm/tidss/tidss_encoder.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_encoder.c b/drivers/gpu/drm/tidss/tidss_encoder.c
> index e278a9c89476..ae7f134754b7 100644
> --- a/drivers/gpu/drm/tidss/tidss_encoder.c
> +++ b/drivers/gpu/drm/tidss/tidss_encoder.c
> @@ -22,6 +22,7 @@ static int tidss_encoder_atomic_check(struct drm_encoder *encoder,
>  	struct drm_device *ddev = encoder->dev;
>  	struct tidss_crtc_state *tcrtc_state = to_tidss_crtc_state(crtc_state);
>  	struct drm_display_info *di = &conn_state->connector->display_info;
> +	struct drm_bridge_state *bstate;
>  	struct drm_bridge *bridge;
>  	bool bus_flags_set = false;
>  
> @@ -41,14 +42,19 @@ static int tidss_encoder_atomic_check(struct drm_encoder *encoder,
>  		break;
>  	}
>  
> -	if (!di->bus_formats || di->num_bus_formats == 0)  {
> -		dev_err(ddev->dev, "%s: No bus_formats in connected display\n",
> -			__func__);
> +	/* Copy the bus_format from the input_bus_format of first bridge */
> +	bridge = drm_bridge_chain_get_first_bridge(encoder);
> +	bstate = drm_atomic_get_new_bridge_state(crtc_state->state, bridge);
> +	if (bstate)
> +		tcrtc_state->bus_format = bstate->input_bus_cfg.format;
> +
> +	if (tcrtc_state->bus_format == 0 ||
> +	    tcrtc_state->bus_format == MEDIA_BUS_FMT_FIXED) {
> +
> +		dev_err(ddev->dev, "Bridge connected to the encoder did not specify media bus format\n");
>  		return -EINVAL;
>  	}
>  
> -	// XXX any cleaner way to set bus format and flags?
> -	tcrtc_state->bus_format = di->bus_formats[0];
>  	if (!bus_flags_set)
>  		tcrtc_state->bus_flags = di->bus_flags;

Shouldn't the flags also be retrieved from the bridge state ?

>  

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/5] drm: bridge: Propagate the bus flags from bridge->timings

[Tomi Valkeinen]

On 30/10/2020 00:48, Laurent Pinchart wrote:

>>> And, hmm... It's too easy to get confused with these, but... If the bridge defines timings, and
>>> timings->input_bus_flags != 0, should we always pick that, even if we got something via
>>> output_flags? Logic being, if this bridge defines timings->input_bus_flags, it probably wants that
>>> to be used regardless whether we got something from the next bridge.
> 
> timings->input_bus_flags is an API that predates format and flags
> propagation along the pipeline. I would assume that drivers that
> implement propagation should do so in a way that prioritize that API,
> and either not report flags in the timings (in which case giving
> priority to one or another wouldn't make a difference as both would
> never be provided together), or would report flags in the timings as a
> best effort fallback for display controller drivers that would look at
> them exclusively without supporting the new API. I would thus think that
> the flags obtained through format negotiation should be prioritized.

What do you mean "drivers that implement propagation"? Doesn't that come from the framework, not
from the drivers?

Say, we have two bridges, A -> B. A has timings->input_bus_flags.

When propagating the flags, we get something as B's input flags. Should A use B's input flags as A's
input flags, or should A use its timings->input_bus_flags? I was suggesting the latter. Nikhil's
patch does the latter, but only if B's input flags was 0.

A can override its input flags manually in atomic_check, but if the timings->input_bus_flags exists,
isn't it a sane choice to just pick that by default?

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/5] drm: bridge: Propagate the bus flags from bridge->timings

[Boris Brezillon]

On Fri, 30 Oct 2020 09:30:01 +0200
Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:

> On 30/10/2020 00:48, Laurent Pinchart wrote:
> 
> >>> And, hmm... It's too easy to get confused with these, but... If the bridge defines timings, and
> >>> timings->input_bus_flags != 0, should we always pick that, even if we got something via
> >>> output_flags? Logic being, if this bridge defines timings->input_bus_flags, it probably wants that
> >>> to be used regardless whether we got something from the next bridge.  
> > 
> > timings->input_bus_flags is an API that predates format and flags
> > propagation along the pipeline. I would assume that drivers that
> > implement propagation should do so in a way that prioritize that API,
> > and either not report flags in the timings (in which case giving
> > priority to one or another wouldn't make a difference as both would
> > never be provided together), or would report flags in the timings as a
> > best effort fallback for display controller drivers that would look at
> > them exclusively without supporting the new API. I would thus think that
> > the flags obtained through format negotiation should be prioritized.  
> 
> What do you mean "drivers that implement propagation"? Doesn't that come from the framework, not
> from the drivers?
> 
> Say, we have two bridges, A -> B. A has timings->input_bus_flags.
> 
> When propagating the flags, we get something as B's input flags. Should A use B's input flags as A's
> input flags, or should A use its timings->input_bus_flags? I was suggesting the latter. Nikhil's
> patch does the latter, but only if B's input flags was 0.

A should definitely use timings->input_bus_flags in that case.

> 
> A can override its input flags manually in atomic_check, but if the timings->input_bus_flags exists,
> isn't it a sane choice to just pick that by default?

The "propagate output flags" and soon to be added "use
timing->input_flags if present" logic should only be used as a fallback
for bridges that do not support dynamic bus format/flags negotiation
IMHO. Ideally we'd want to convert all bridges to do this dynamic bus
format/flags negotiation and get rid of timings->input_bus_flags once
this is done, but that's likely to take time. So, if your driver
implements the ->atomic_check() hook and needs specific input flags,
I'd recommend setting the input flags there instead of specifying it
through timings->input_bus_flags.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/5] drm: bridge: Propagate the bus flags from bridge->timings

[Tomi Valkeinen]

Hi Boris,

On 30/10/2020 10:08, Boris Brezillon wrote:
> The "propagate output flags" and soon to be added "use
> timing->input_flags if present" logic should only be used as a fallback
> for bridges that do not support dynamic bus format/flags negotiation
> IMHO. Ideally we'd want to convert all bridges to do this dynamic bus
> format/flags negotiation and get rid of timings->input_bus_flags once
> this is done, but that's likely to take time. So, if your driver
> implements the ->atomic_check() hook and needs specific input flags,
> I'd recommend setting the input flags there instead of specifying it
> through timings->input_bus_flags.

What is bus flags negotiation? Don't we have negotiation only for bus formats? Bus flags are just
set, and the previous bridge in the chain has to use those flags.

Or do you just refer to setting the bus flags dynamically in atomic_check, versus static in
input_bus_flags?

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/5] drm: bridge: Propagate the bus flags from bridge->timings

[Boris Brezillon]

On Fri, 30 Oct 2020 10:40:46 +0200
Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:

> Hi Boris,
> 
> On 30/10/2020 10:08, Boris Brezillon wrote:
> > The "propagate output flags" and soon to be added "use
> > timing->input_flags if present" logic should only be used as a fallback
> > for bridges that do not support dynamic bus format/flags negotiation
> > IMHO. Ideally we'd want to convert all bridges to do this dynamic bus
> > format/flags negotiation and get rid of timings->input_bus_flags once
> > this is done, but that's likely to take time. So, if your driver
> > implements the ->atomic_check() hook and needs specific input flags,
> > I'd recommend setting the input flags there instead of specifying it
> > through timings->input_bus_flags.  
> 
> What is bus flags negotiation? Don't we have negotiation only for bus formats? Bus flags are just
> set, and the previous bridge in the chain has to use those flags.

Well, there's currently no such negotiation, but I don't see why there
wouldn't be one at some point if bridges can configure it dynamically
(in you A -> B example, A output flags must match B input flags, and if
both A and B can configure those dynamically, they need to negotiate
that part too).

> 
> Or do you just refer to setting the bus flags dynamically in atomic_check, versus static in
> input_bus_flags?

Yes, that's what I suggest, even if those flags are static right now.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] drm/bridge: mhdp8564: Support format negotiation

[Tomi Valkeinen]

On 30/10/2020 00:39, Laurent Pinchart wrote:
> Hi Nikhil,
> 
> Thank you for the patch.
> 
> On Fri, Oct 16, 2020 at 04:09:17PM +0530, Nikhil Devshatwar wrote:
>> With new connector model, mhdp bridge will not create the connector and
>> SoC driver will rely on format negotiation to setup the encoder format.
>>
>> Support format negotiations hooks in the drm_bridge_funcs.
>> Support a single format for input.
>>
>> Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
>> ---
>>  .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 29 +++++++++++++++++++
>>  1 file changed, 29 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>> index d0c65610ebb5..230f6e28f82f 100644
>> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>> @@ -2078,6 +2078,34 @@ cdns_mhdp_bridge_atomic_reset(struct drm_bridge *bridge)
>>  	return &cdns_mhdp_state->base;
>>  }
>>  
>> +static u32 *cdns_mhdp_get_input_bus_fmts(struct drm_bridge *bridge,
>> +				      struct drm_bridge_state *bridge_state,
>> +				      struct drm_crtc_state *crtc_state,
>> +				      struct drm_connector_state *conn_state,
>> +				      u32 output_fmt,
>> +				      unsigned int *num_input_fmts)
>> +{
>> +	u32 *input_fmts;
>> +	u32 default_bus_format = MEDIA_BUS_FMT_RGB121212_1X36;
> 
> Display port supports quite a few different formats. See my reply to
> 4/5, except it's worse in the DP case :-) Especially given that multiple
> displays need to be taken into account. I'm afraid we need to decide how
> to map media bus formats to different DP use cases, possibly adding new
> bus formats as part of this exercise, and then revisit this patch.

I agree with the points you make here and for the tfp410 patch, but the point of these patches are
just to keep drivers working with the new connector model.

With the old model both tfp410 and mhdp create their own connector, and set the input bus format to
connector's display_info. With the new model, that doesn't happen and there's no bus format, and so
the display controller fails.

I see these more as fixes than implementing new features.

Nikhil, I think the output_fmt at the moment should always be FMT_FIXED. Maybe we can just check for
that.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/5] drm/tidss: Set bus_format correctly from bridge/connector

[Nikhil Devshatwar]

On 00:57-20201030, Laurent Pinchart wrote:
> Hi Nikhil,
> 
> Thank you for the patch.
> 
> On Fri, Oct 16, 2020 at 04:09:14PM +0530, Nikhil Devshatwar wrote:
> > When there is a chain of bridges attached to the encoder,
> > the bus_format should be ideally set from the input format of the
> > first bridge in the chain.
> > 
> > Use the bridge state to get the negotiated bus_format.
> > If the bridge does not support format negotiation, error out
> > and fail.
> > 
> > Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
> > ---
> >  drivers/gpu/drm/tidss/tidss_encoder.c | 16 +++++++++++-----
> >  1 file changed, 11 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/tidss/tidss_encoder.c b/drivers/gpu/drm/tidss/tidss_encoder.c
> > index e278a9c89476..ae7f134754b7 100644
> > --- a/drivers/gpu/drm/tidss/tidss_encoder.c
> > +++ b/drivers/gpu/drm/tidss/tidss_encoder.c
> > @@ -22,6 +22,7 @@ static int tidss_encoder_atomic_check(struct drm_encoder *encoder,
> >  	struct drm_device *ddev = encoder->dev;
> >  	struct tidss_crtc_state *tcrtc_state = to_tidss_crtc_state(crtc_state);
> >  	struct drm_display_info *di = &conn_state->connector->display_info;
> > +	struct drm_bridge_state *bstate;
> >  	struct drm_bridge *bridge;
> >  	bool bus_flags_set = false;
> >  
> > @@ -41,14 +42,19 @@ static int tidss_encoder_atomic_check(struct drm_encoder *encoder,
> >  		break;
> >  	}
> >  
> > -	if (!di->bus_formats || di->num_bus_formats == 0)  {
> > -		dev_err(ddev->dev, "%s: No bus_formats in connected display\n",
> > -			__func__);
> > +	/* Copy the bus_format from the input_bus_format of first bridge */
> > +	bridge = drm_bridge_chain_get_first_bridge(encoder);
> > +	bstate = drm_atomic_get_new_bridge_state(crtc_state->state, bridge);
> > +	if (bstate)
> > +		tcrtc_state->bus_format = bstate->input_bus_cfg.format;
> > +
> > +	if (tcrtc_state->bus_format == 0 ||
> > +	    tcrtc_state->bus_format == MEDIA_BUS_FMT_FIXED) {
> > +
> > +		dev_err(ddev->dev, "Bridge connected to the encoder did not specify media bus format\n");
> >  		return -EINVAL;
> >  	}
> >  
> > -	// XXX any cleaner way to set bus format and flags?
> > -	tcrtc_state->bus_format = di->bus_formats[0];
> >  	if (!bus_flags_set)
> >  		tcrtc_state->bus_flags = di->bus_flags;
> 
> Shouldn't the flags also be retrieved from the bridge state ?

Yes, the code does that above, not covered in the diff context.
When no bridges have reported the timings,
it uses the display_info as fallback (when bus_flags_set is false)

Nikhil D

> 
> >  
> 
> -- 
> Regards,
> 
> Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel