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

[PATCH v3 0/6] 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
as a first step before moving the connector model.

Nikhil Devshatwar (6):
  drm: bridge: Propagate the bus flags from bridge->timings
  drm/bridge: tfp410: Support format negotiation hooks
  drm/bridge: mhdp8546: Add minimal format negotiation
  drm/tidss: Set bus_format correctly from bridge/connector
  drm/tidss: Move to newer connector model
  drm/bridge: cdns-mhdp8546: Fix the interrupt enable/disable

 .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 69 +++++++++++++------
 drivers/gpu/drm/bridge/ti-tfp410.c            | 33 +++++++++
 drivers/gpu/drm/drm_bridge.c                  |  8 +++
 drivers/gpu/drm/tidss/tidss_drv.h             |  3 +
 drivers/gpu/drm/tidss/tidss_encoder.c         | 36 ++++------
 drivers/gpu/drm/tidss/tidss_kms.c             | 19 ++++-
 6 files changed, 123 insertions(+), 45 deletions(-)

-- 
2.17.1

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

[PATCH v3 1/6] drm: bridge: Propagate the bus flags from bridge->timings

[Nikhil Devshatwar]

bus_flags can be specified by a bridge in the timings.
If the bridge provides it, Override the bus_flags when propagating
from next bridge.

Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---

Notes:
    changes from v2:
    * update comment
    changes from v1:
    * Check for timings
    * Prioritize timings flags over next bridge's flags

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

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 64f0effb52ac..13b67fc0dad3 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -975,6 +975,14 @@ drm_atomic_bridge_propagate_bus_flags(struct drm_bridge *bridge,
 	 * duplicate the "dummy propagation" logic.
 	 */
 	bridge_state->input_bus_cfg.flags = output_flags;
+
+	/*
+	 * If legacy bus flags are provided in bridge->timings, use those as
+	 * input flags instead of propagating the output flags.
+	 */
+	if (bridge->timings && bridge->timings->input_bus_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 v3 2/6] drm/bridge: tfp410: Support format negotiation hooks

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

Output format is expected to be MEDIA_BUS_FMT_FIXED
Input format is the one selected by the bridge from DT properties.

Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---

Notes:
    changes from v1:
    * Use only MEDIA_BUS_FMT_FIXED for output

 drivers/gpu/drm/bridge/ti-tfp410.c | 33 ++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c
index ba3fa2a9b8a4..3def9acba86b 100644
--- a/drivers/gpu/drm/bridge/ti-tfp410.c
+++ b/drivers/gpu/drm/bridge/ti-tfp410.c
@@ -204,12 +204,45 @@ 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;
+
+	/*
+	 * Output of tfp410 is DVI, which is TMDS.
+	 * There is no media format defined for this.
+	 * Using MEDIA_BUS_FMT_FIXED for now.
+	 */
+	if (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 v3 3/6] drm/bridge: mhdp8546: Add minimal 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 minimal format negotiations hooks in the drm_bridge_funcs.
Complete format negotiation can be added based on EDID data.
This patch adds the minimal required support to avoid failure
after moving to new connector model.

Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---

Notes:
    changes from v1:
    * cosmetic fixes, commit message update

 .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 25 +++++++++++++++++++
 1 file changed, 25 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..2cd809eed827 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
@@ -2078,6 +2078,30 @@ 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;
+
+	if (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 +2166,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

[PATCH v3 4/6] drm/tidss: Set bus_format correctly from bridge/connector

[Nikhil Devshatwar]

Remove the old code to iterate over the bridge chain, as this is
already done by the framework.
The bridge state should have the negotiated bus format and flags.
Use these from the bridge's state.
If the bridge does not support format negotiation, error out
and fail.

Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---

Notes:
    changes from v2:
    * Remove the old code and use the flags from the bridge state

 drivers/gpu/drm/tidss/tidss_encoder.c | 36 +++++++++++----------------
 1 file changed, 14 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_encoder.c b/drivers/gpu/drm/tidss/tidss_encoder.c
index e278a9c89476..08d5083c5508 100644
--- a/drivers/gpu/drm/tidss/tidss_encoder.c
+++ b/drivers/gpu/drm/tidss/tidss_encoder.c
@@ -21,37 +21,29 @@ 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;
 
 	dev_dbg(ddev->dev, "%s\n", __func__);
 
-	/*
-	 * Take the bus_flags from the first bridge that defines
-	 * bridge timings, or from the connector's display_info if no
-	 * bridge defines the timings.
-	 */
-	drm_for_each_bridge_in_chain(encoder, bridge) {
-		if (!bridge->timings)
-			continue;
-
-		tcrtc_state->bus_flags = bridge->timings->input_bus_flags;
-		bus_flags_set = true;
-		break;
+	/* Copy the bus_format and flags from the first bridge's state */
+	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;
+		tcrtc_state->bus_flags = bstate->input_bus_cfg.flags;
+	} else {
+		dev_err(ddev->dev, "Could not get the bridge state\n");
+		return -EINVAL;
 	}
 
-	if (!di->bus_formats || di->num_bus_formats == 0)  {
-		dev_err(ddev->dev, "%s: No bus_formats in connected display\n",
-			__func__);
+	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;
-
 	return 0;
 }
 
-- 
2.17.1

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

[PATCH v3 5/6] 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>
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---

Notes:
    changes from v1:
    * Add error handling

 drivers/gpu/drm/tidss/tidss_drv.h |  3 +++
 drivers/gpu/drm/tidss/tidss_kms.c | 19 ++++++++++++++++++-
 2 files changed, 21 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..1f5ae153b114 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,26 @@ 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;
+
+		ret = drm_connector_attach_encoder(connector, enc);
+		if (ret) {
+			dev_err(tidss->dev, "attaching encoder to connector failed\n");
+			return ret;
+		}
 	}
 
 	/* 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 v3 6/6] drm/bridge: cdns-mhdp8546: Fix the interrupt enable/disable

[Nikhil Devshatwar]

When removing the tidss driver, there is a warning reported by
kernel about an unhandled interrupt for mhdp driver.

[   43.238895] irq 31: nobody cared (try booting with the "irqpoll" option)
... [snipped backtrace]
[   43.330735] handlers:
[   43.333020] [<000000005367c4f9>] irq_default_primary_handler threaded [<000000007e02b601>]
cdns_mhdp_irq_handler [cdns_mhdp8546]
[   43.344607] Disabling IRQ #31

This happens because as part of cdns_mhdp_bridge_hpd_disable, driver tries
to disable the interrupts. While disabling the SW_EVENT interrupts,
it accidentally enables the MBOX interrupts, which are not handled by
the driver.

Fix this with a read-modify-write to update only required bits.
Use the enable / disable function as required in other places.

Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
Reviewed-by: Swapnil Jakhade <sjakhade@cadence.com>
---

Notes:
    changes from v2:
    * Fix the interrupt enable logic at other places in code
    * Reorder functions so that they can be used earlier in the program

 .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 44 +++++++++----------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
index 2cd809eed827..0442269aeb03 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
@@ -52,6 +52,26 @@
 
 #include "cdns-mhdp8546-j721e.h"
 
+static void cdns_mhdp_bridge_hpd_enable(struct drm_bridge *bridge)
+{
+	struct cdns_mhdp_device *mhdp = bridge_to_mhdp(bridge);
+
+	/* Enable SW event interrupts */
+	if (mhdp->bridge_attached)
+		writel(readl(mhdp->regs + CDNS_APB_INT_MASK) &
+		       ~CDNS_APB_INT_MASK_SW_EVENT_INT,
+		       mhdp->regs + CDNS_APB_INT_MASK);
+}
+
+static void cdns_mhdp_bridge_hpd_disable(struct drm_bridge *bridge)
+{
+	struct cdns_mhdp_device *mhdp = bridge_to_mhdp(bridge);
+
+	writel(readl(mhdp->regs + CDNS_APB_INT_MASK) |
+	       CDNS_APB_INT_MASK_SW_EVENT_INT,
+	       mhdp->regs + CDNS_APB_INT_MASK);
+}
+
 static int cdns_mhdp_mailbox_read(struct cdns_mhdp_device *mhdp)
 {
 	int ret, empty;
@@ -747,9 +767,7 @@ static int cdns_mhdp_fw_activate(const struct firmware *fw,
 	 * MHDP_HW_STOPPED happens only due to driver removal when
 	 * bridge should already be detached.
 	 */
-	if (mhdp->bridge_attached)
-		writel(~(u32)CDNS_APB_INT_MASK_SW_EVENT_INT,
-		       mhdp->regs + CDNS_APB_INT_MASK);
+	cdns_mhdp_bridge_hpd_enable(&mhdp->bridge);
 
 	spin_unlock(&mhdp->start_lock);
 
@@ -1689,8 +1707,7 @@ static int cdns_mhdp_attach(struct drm_bridge *bridge,
 
 	/* Enable SW event interrupts */
 	if (hw_ready)
-		writel(~(u32)CDNS_APB_INT_MASK_SW_EVENT_INT,
-		       mhdp->regs + CDNS_APB_INT_MASK);
+		cdns_mhdp_bridge_hpd_enable(bridge);
 
 	return 0;
 }
@@ -2140,23 +2157,6 @@ static struct edid *cdns_mhdp_bridge_get_edid(struct drm_bridge *bridge,
 	return cdns_mhdp_get_edid(mhdp, connector);
 }
 
-static void cdns_mhdp_bridge_hpd_enable(struct drm_bridge *bridge)
-{
-	struct cdns_mhdp_device *mhdp = bridge_to_mhdp(bridge);
-
-	/* Enable SW event interrupts */
-	if (mhdp->bridge_attached)
-		writel(~(u32)CDNS_APB_INT_MASK_SW_EVENT_INT,
-		       mhdp->regs + CDNS_APB_INT_MASK);
-}
-
-static void cdns_mhdp_bridge_hpd_disable(struct drm_bridge *bridge)
-{
-	struct cdns_mhdp_device *mhdp = bridge_to_mhdp(bridge);
-
-	writel(CDNS_APB_INT_MASK_SW_EVENT_INT, mhdp->regs + CDNS_APB_INT_MASK);
-}
-
 static const struct drm_bridge_funcs cdns_mhdp_bridge_funcs = {
 	.atomic_enable = cdns_mhdp_atomic_enable,
 	.atomic_disable = cdns_mhdp_atomic_disable,
-- 
2.17.1

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

Re: [PATCH v3 6/6] drm/bridge: cdns-mhdp8546: Fix the interrupt enable/disable

[Tomi Valkeinen]

On 19/11/2020 18:01, Nikhil Devshatwar wrote:
> When removing the tidss driver, there is a warning reported by
> kernel about an unhandled interrupt for mhdp driver.
> 
> [   43.238895] irq 31: nobody cared (try booting with the "irqpoll" option)
> ... [snipped backtrace]
> [   43.330735] handlers:
> [   43.333020] [<000000005367c4f9>] irq_default_primary_handler threaded [<000000007e02b601>]
> cdns_mhdp_irq_handler [cdns_mhdp8546]
> [   43.344607] Disabling IRQ #31
> 
> This happens because as part of cdns_mhdp_bridge_hpd_disable, driver tries
> to disable the interrupts. While disabling the SW_EVENT interrupts,
> it accidentally enables the MBOX interrupts, which are not handled by
> the driver.
> 
> Fix this with a read-modify-write to update only required bits.
> Use the enable / disable function as required in other places.
> 
> Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
> Reviewed-by: Swapnil Jakhade <sjakhade@cadence.com>

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

I think this is fine as a fix for this issue, but at some point the irq management needs some work.
E.g. we call cdns_mhdp_bridge_hpd_enable when attaching/enabling the hw, but also via
drm_bridge_funcs->hpd_enable. This doesn't make sense, as one of those calls doesn't achieve
anything, as cdns_mhdp_bridge_hpd_enable has already been called.

 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 v3 4/6] drm/tidss: Set bus_format correctly from bridge/connector

[Tomi Valkeinen]

Hi Nikhil,

On 19/11/2020 18:01, Nikhil Devshatwar wrote:
> Remove the old code to iterate over the bridge chain, as this is
> already done by the framework.
> The bridge state should have the negotiated bus format and flags.
> Use these from the bridge's state.
> If the bridge does not support format negotiation, error out
> and fail.
> 
> Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
> 
> Notes:
>     changes from v2:
>     * Remove the old code and use the flags from the bridge state
> 
>  drivers/gpu/drm/tidss/tidss_encoder.c | 36 +++++++++++----------------
>  1 file changed, 14 insertions(+), 22 deletions(-)

If a first bridge (after the crtc) supports two bus formats as input, how does tidss choose between
those? This patch just picks bstate->input_bus_cfg.format, and it's not clear to me which one that
is (the first one?). Also, we don't check if tidss actually supports the bus format.

 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 v3 4/6] drm/tidss: Set bus_format correctly from bridge/connector

[Nikhil Devshatwar]

On 14:51-20201125, Tomi Valkeinen wrote:
> Hi Nikhil,
> 
> On 19/11/2020 18:01, Nikhil Devshatwar wrote:
> > Remove the old code to iterate over the bridge chain, as this is
> > already done by the framework.
> > The bridge state should have the negotiated bus format and flags.
> > Use these from the bridge's state.
> > If the bridge does not support format negotiation, error out
> > and fail.
> > 
> > Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
> > Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > ---
> > 
> > Notes:
> >     changes from v2:
> >     * Remove the old code and use the flags from the bridge state
> > 
> >  drivers/gpu/drm/tidss/tidss_encoder.c | 36 +++++++++++----------------
> >  1 file changed, 14 insertions(+), 22 deletions(-)
> 
> If a first bridge (after the crtc) supports two bus formats as input, how does tidss choose between
> those? This patch just picks bstate->input_bus_cfg.format, and it's not clear to me which one that
> is (the first one?). Also, we don't check if tidss actually supports the bus format.

The selection is done by the framework in
select_bus_fmt_recursive at drivers/gpu/drm/drm_bridge.c:810

My understanding is that currently, the format negotiation logic does
not negotiate all the way till encoder, it stops only at the
first_bridge.

Nikhil Devshatwar

> 
>  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 v3 1/6] drm: bridge: Propagate the bus flags from bridge->timings

[Laurent Pinchart]

Hi Nikhil,

Thank you for the patch.

On Thu, Nov 19, 2020 at 09:31:29PM +0530, Nikhil Devshatwar wrote:
> bus_flags can be specified by a bridge in the timings.
> If the bridge provides it, Override the bus_flags when propagating
> from next bridge.
> 
> Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
> 
> Notes:
>     changes from v2:
>     * update comment
>     changes from v1:
>     * Check for timings
>     * Prioritize timings flags over next bridge's flags
> 
>  drivers/gpu/drm/drm_bridge.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 64f0effb52ac..13b67fc0dad3 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -975,6 +975,14 @@ drm_atomic_bridge_propagate_bus_flags(struct drm_bridge *bridge,
>  	 * duplicate the "dummy propagation" logic.
>  	 */
>  	bridge_state->input_bus_cfg.flags = output_flags;
> +
> +	/*
> +	 * If legacy bus flags are provided in bridge->timings, use those as
> +	 * input flags instead of propagating the output flags.
> +	 */
> +	if (bridge->timings && bridge->timings->input_bus_flags)
> +		bridge_state->input_bus_cfg.flags =
> +			bridge->timings->input_bus_flags;

Hasn't Boris commented in his review of v1 that bus flags should be set
in atomic_check, even when they're static ? We're moving towards
removing timings->input_bus_flags, so this patch goes in the wrong
direction :-S

>  }
>  
>  /**

-- 
Regards,

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

Re: [PATCH v3 2/6] drm/bridge: tfp410: Support format negotiation hooks

[Laurent Pinchart]

Hi Nikhil,

Thank you for the patch.

On Thu, Nov 19, 2020 at 09:31:30PM +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.
> 
> Output format is expected to be MEDIA_BUS_FMT_FIXED
> Input format is the one selected by the bridge from DT properties.
> 
> Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

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

> ---
> 
> Notes:
>     changes from v1:
>     * Use only MEDIA_BUS_FMT_FIXED for output
> 
>  drivers/gpu/drm/bridge/ti-tfp410.c | 33 ++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c
> index ba3fa2a9b8a4..3def9acba86b 100644
> --- a/drivers/gpu/drm/bridge/ti-tfp410.c
> +++ b/drivers/gpu/drm/bridge/ti-tfp410.c
> @@ -204,12 +204,45 @@ 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;
> +
> +	/*
> +	 * Output of tfp410 is DVI, which is TMDS.
> +	 * There is no media format defined for this.
> +	 * Using MEDIA_BUS_FMT_FIXED for now.
> +	 */
> +	if (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 = {

-- 
Regards,

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

Re: [PATCH v3 4/6] drm/tidss: Set bus_format correctly from bridge/connector

[Laurent Pinchart]

Hi Nikhil,

Thank you for the patch.

On Thu, Nov 19, 2020 at 09:31:32PM +0530, Nikhil Devshatwar wrote:
> Remove the old code to iterate over the bridge chain, as this is
> already done by the framework.
> The bridge state should have the negotiated bus format and flags.
> Use these from the bridge's state.
> If the bridge does not support format negotiation, error out
> and fail.
> 
> Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
> 
> Notes:
>     changes from v2:
>     * Remove the old code and use the flags from the bridge state
> 
>  drivers/gpu/drm/tidss/tidss_encoder.c | 36 +++++++++++----------------
>  1 file changed, 14 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_encoder.c b/drivers/gpu/drm/tidss/tidss_encoder.c
> index e278a9c89476..08d5083c5508 100644
> --- a/drivers/gpu/drm/tidss/tidss_encoder.c
> +++ b/drivers/gpu/drm/tidss/tidss_encoder.c
> @@ -21,37 +21,29 @@ 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;
>  
>  	dev_dbg(ddev->dev, "%s\n", __func__);
>  
> -	/*
> -	 * Take the bus_flags from the first bridge that defines
> -	 * bridge timings, or from the connector's display_info if no
> -	 * bridge defines the timings.
> -	 */
> -	drm_for_each_bridge_in_chain(encoder, bridge) {
> -		if (!bridge->timings)
> -			continue;
> -
> -		tcrtc_state->bus_flags = bridge->timings->input_bus_flags;
> -		bus_flags_set = true;
> -		break;
> +	/* Copy the bus_format and flags from the first bridge's state */
> +	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;
> +		tcrtc_state->bus_flags = bstate->input_bus_cfg.flags;
> +	} else {
> +		dev_err(ddev->dev, "Could not get the bridge state\n");
> +		return -EINVAL;
>  	}

I'd write this

	bstate = drm_atomic_get_new_bridge_state(crtc_state->state, bridge);
	if (!bstate) {
		dev_err(ddev->dev, "Could not get the bridge state\n");
		return -EINVAL;
	}

	tcrtc_state->bus_format = bstate->input_bus_cfg.format;
	tcrtc_state->bus_flags = bstate->input_bus_cfg.flags;
>  
> -	if (!di->bus_formats || di->num_bus_formats == 0)  {
> -		dev_err(ddev->dev, "%s: No bus_formats in connected display\n",
> -			__func__);
> +	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;
> -
>  	return 0;
>  }
>  

-- 
Regards,

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

Re: [PATCH v3 4/6] drm/tidss: Set bus_format correctly from bridge/connector

[Laurent Pinchart]

Hi Nikhil,

On Mon, Nov 30, 2020 at 12:05:03PM +0530, Nikhil Devshatwar wrote:
> On 14:51-20201125, Tomi Valkeinen wrote:
> > On 19/11/2020 18:01, Nikhil Devshatwar wrote:
> > > Remove the old code to iterate over the bridge chain, as this is
> > > already done by the framework.
> > > The bridge state should have the negotiated bus format and flags.
> > > Use these from the bridge's state.
> > > If the bridge does not support format negotiation, error out
> > > and fail.
> > > 
> > > Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
> > > Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > > ---
> > > 
> > > Notes:
> > >     changes from v2:
> > >     * Remove the old code and use the flags from the bridge state
> > > 
> > >  drivers/gpu/drm/tidss/tidss_encoder.c | 36 +++++++++++----------------
> > >  1 file changed, 14 insertions(+), 22 deletions(-)
> > 
> > If a first bridge (after the crtc) supports two bus formats as input, how does tidss choose between
> > those? This patch just picks bstate->input_bus_cfg.format, and it's not clear to me which one that
> > is (the first one?). Also, we don't check if tidss actually supports the bus format.
> 
> The selection is done by the framework in
> select_bus_fmt_recursive at drivers/gpu/drm/drm_bridge.c:810
> 
> My understanding is that currently, the format negotiation logic does
> not negotiate all the way till encoder, it stops only at the
> first_bridge.

Should we then implement a bridge in the tidss driver to model the
internal encoder, in order to support format negotiation all the way to
the tidss ?

-- 
Regards,

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

Re: [PATCH v3 1/6] drm: bridge: Propagate the bus flags from bridge->timings

[Tomi Valkeinen]

On 30/11/2020 11:36, Laurent Pinchart wrote:
> Hi Nikhil,
> 
> Thank you for the patch.
> 
> On Thu, Nov 19, 2020 at 09:31:29PM +0530, Nikhil Devshatwar wrote:
>> bus_flags can be specified by a bridge in the timings.
>> If the bridge provides it, Override the bus_flags when propagating
>> from next bridge.
>>
>> Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
>> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>>
>> Notes:
>>     changes from v2:
>>     * update comment
>>     changes from v1:
>>     * Check for timings
>>     * Prioritize timings flags over next bridge's flags
>>
>>  drivers/gpu/drm/drm_bridge.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>> index 64f0effb52ac..13b67fc0dad3 100644
>> --- a/drivers/gpu/drm/drm_bridge.c
>> +++ b/drivers/gpu/drm/drm_bridge.c
>> @@ -975,6 +975,14 @@ drm_atomic_bridge_propagate_bus_flags(struct drm_bridge *bridge,
>>  	 * duplicate the "dummy propagation" logic.
>>  	 */
>>  	bridge_state->input_bus_cfg.flags = output_flags;
>> +
>> +	/*
>> +	 * If legacy bus flags are provided in bridge->timings, use those as
>> +	 * input flags instead of propagating the output flags.
>> +	 */
>> +	if (bridge->timings && bridge->timings->input_bus_flags)
>> +		bridge_state->input_bus_cfg.flags =
>> +			bridge->timings->input_bus_flags;
> 
> Hasn't Boris commented in his review of v1 that bus flags should be set
> in atomic_check, even when they're static ? We're moving towards
> removing timings->input_bus_flags, so this patch goes in the wrong
> direction :-S

We have atomic_check only if the bridge has implemented atomic funcs. And even if there's
atomic_check, not all bridges set the bus_flags there. So we need to either 1) fix the issue for now
as in this patch, or 2) convert all bridges to use atomic funcs and fix all the bridges to set the
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 v3 1/6] drm: bridge: Propagate the bus flags from bridge->timings

[Laurent Pinchart]

Hi Tomi,

On Mon, Nov 30, 2020 at 11:46:31AM +0200, Tomi Valkeinen wrote:
> On 30/11/2020 11:36, Laurent Pinchart wrote:
> > On Thu, Nov 19, 2020 at 09:31:29PM +0530, Nikhil Devshatwar wrote:
> >> bus_flags can be specified by a bridge in the timings.
> >> If the bridge provides it, Override the bus_flags when propagating
> >> from next bridge.
> >>
> >> Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
> >> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> >> ---
> >>
> >> Notes:
> >>     changes from v2:
> >>     * update comment
> >>     changes from v1:
> >>     * Check for timings
> >>     * Prioritize timings flags over next bridge's flags
> >>
> >>  drivers/gpu/drm/drm_bridge.c | 8 ++++++++
> >>  1 file changed, 8 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> >> index 64f0effb52ac..13b67fc0dad3 100644
> >> --- a/drivers/gpu/drm/drm_bridge.c
> >> +++ b/drivers/gpu/drm/drm_bridge.c
> >> @@ -975,6 +975,14 @@ drm_atomic_bridge_propagate_bus_flags(struct drm_bridge *bridge,
> >>  	 * duplicate the "dummy propagation" logic.
> >>  	 */
> >>  	bridge_state->input_bus_cfg.flags = output_flags;
> >> +
> >> +	/*
> >> +	 * If legacy bus flags are provided in bridge->timings, use those as
> >> +	 * input flags instead of propagating the output flags.
> >> +	 */
> >> +	if (bridge->timings && bridge->timings->input_bus_flags)
> >> +		bridge_state->input_bus_cfg.flags =
> >> +			bridge->timings->input_bus_flags;
> > 
> > Hasn't Boris commented in his review of v1 that bus flags should be set
> > in atomic_check, even when they're static ? We're moving towards
> > removing timings->input_bus_flags, so this patch goes in the wrong
> > direction :-S
> 
> We have atomic_check only if the bridge has implemented atomic funcs. And even if there's
> atomic_check, not all bridges set the bus_flags there. So we need to either 1) fix the issue for now
> as in this patch, or 2) convert all bridges to use atomic funcs and fix all the bridges to set the
> bus_flags.

The second option is what we'd like to achieve. Wouldn't it be best to
already start going in that direction ? We don't need to convert all
bridge drivers in one go here, just the ones that are used by tidss.

-- 
Regards,

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

Re: [PATCH v3 1/6] drm: bridge: Propagate the bus flags from bridge->timings

[Tomi Valkeinen]

On 30/11/2020 11:47, Laurent Pinchart wrote:

>>> Hasn't Boris commented in his review of v1 that bus flags should be set
>>> in atomic_check, even when they're static ? We're moving towards
>>> removing timings->input_bus_flags, so this patch goes in the wrong
>>> direction :-S
>>
>> We have atomic_check only if the bridge has implemented atomic funcs. And even if there's
>> atomic_check, not all bridges set the bus_flags there. So we need to either 1) fix the issue for now
>> as in this patch, or 2) convert all bridges to use atomic funcs and fix all the bridges to set the
>> bus_flags.
> 
> The second option is what we'd like to achieve. Wouldn't it be best to
> already start going in that direction ? We don't need to convert all
> bridge drivers in one go here, just the ones that are used by tidss.

I think that sounds fine, except that this is blocking the DisplayPort support for J7. We have
everything in for DP except dts changes (can be added only when the drivers work), and the connector
stuff.

The connector stuff includes this series (so that tidss supports the new connector model), and
"[PATCH RESEND v3 0/2] drm: add DisplayPort connector", which adds the connector driver.

The bridges currently used (that I know of) with tidss are cdns-mhdp, tfp410 and sii9022. I don't
expect converting those would be a huge job, but I'd still really like to get the DP working in
upstream without starting to expand the scope of the patches we need to enable it.

That said, we missed 5.11 so perhaps we have the time.

 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 v3 1/6] drm: bridge: Propagate the bus flags from bridge->timings

[Tomi Valkeinen]

On 30/11/2020 12:02, Tomi Valkeinen wrote:
> On 30/11/2020 11:47, Laurent Pinchart wrote:
> 
>>>> Hasn't Boris commented in his review of v1 that bus flags should be set
>>>> in atomic_check, even when they're static ? We're moving towards
>>>> removing timings->input_bus_flags, so this patch goes in the wrong
>>>> direction :-S
>>>
>>> We have atomic_check only if the bridge has implemented atomic funcs. And even if there's
>>> atomic_check, not all bridges set the bus_flags there. So we need to either 1) fix the issue for now
>>> as in this patch, or 2) convert all bridges to use atomic funcs and fix all the bridges to set the
>>> bus_flags.
>>
>> The second option is what we'd like to achieve. Wouldn't it be best to
>> already start going in that direction ? We don't need to convert all
>> bridge drivers in one go here, just the ones that are used by tidss.
> 
> I think that sounds fine, except that this is blocking the DisplayPort support for J7. We have
> everything in for DP except dts changes (can be added only when the drivers work), and the connector
> stuff.
> 
> The connector stuff includes this series (so that tidss supports the new connector model), and
> "[PATCH RESEND v3 0/2] drm: add DisplayPort connector", which adds the connector driver.
> 
> The bridges currently used (that I know of) with tidss are cdns-mhdp, tfp410 and sii9022. I don't
> expect converting those would be a huge job, but I'd still really like to get the DP working in
> upstream without starting to expand the scope of the patches we need to enable it.
> 
> That said, we missed 5.11 so perhaps we have the time.

Looks like Boris was missing from Cc in this series. Adding him.

 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 v3 1/6] drm: bridge: Propagate the bus flags from bridge->timings

[Laurent Pinchart]

Hi Tomi,

On Mon, Nov 30, 2020 at 12:04:27PM +0200, Tomi Valkeinen wrote:
> On 30/11/2020 12:02, Tomi Valkeinen wrote:
> > On 30/11/2020 11:47, Laurent Pinchart wrote:
> > 
> >>>> Hasn't Boris commented in his review of v1 that bus flags should be set
> >>>> in atomic_check, even when they're static ? We're moving towards
> >>>> removing timings->input_bus_flags, so this patch goes in the wrong
> >>>> direction :-S
> >>>
> >>> We have atomic_check only if the bridge has implemented atomic funcs. And even if there's
> >>> atomic_check, not all bridges set the bus_flags there. So we need to either 1) fix the issue for now
> >>> as in this patch, or 2) convert all bridges to use atomic funcs and fix all the bridges to set the
> >>> bus_flags.
> >>
> >> The second option is what we'd like to achieve. Wouldn't it be best to
> >> already start going in that direction ? We don't need to convert all
> >> bridge drivers in one go here, just the ones that are used by tidss.
> > 
> > I think that sounds fine, except that this is blocking the DisplayPort support for J7. We have
> > everything in for DP except dts changes (can be added only when the drivers work), and the connector
> > stuff.
> > 
> > The connector stuff includes this series (so that tidss supports the new connector model), and
> > "[PATCH RESEND v3 0/2] drm: add DisplayPort connector", which adds the connector driver.
> > 
> > The bridges currently used (that I know of) with tidss are cdns-mhdp, tfp410 and sii9022. I don't
> > expect converting those would be a huge job, but I'd still really like to get the DP working in
> > upstream without starting to expand the scope of the patches we need to enable it.
> > 
> > That said, we missed 5.11 so perhaps we have the time.

If there's not enough time to address the bridges, I'm fine with this
series assuming the bridge changes will go on top. If we have enough
time, let's go for it :-)

> Looks like Boris was missing from Cc in this series. Adding him.

-- 
Regards,

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

Re: [PATCH v3 1/6] drm: bridge: Propagate the bus flags from bridge->timings

[Nikhil Devshatwar]

On 20:59-20201130, Laurent Pinchart wrote:
> Hi Tomi,
> 
> On Mon, Nov 30, 2020 at 12:04:27PM +0200, Tomi Valkeinen wrote:
> > On 30/11/2020 12:02, Tomi Valkeinen wrote:
> > > On 30/11/2020 11:47, Laurent Pinchart wrote:
> > > 
> > >>>> Hasn't Boris commented in his review of v1 that bus flags should be set
> > >>>> in atomic_check, even when they're static ? We're moving towards
> > >>>> removing timings->input_bus_flags, so this patch goes in the wrong
> > >>>> direction :-S
> > >>>
> > >>> We have atomic_check only if the bridge has implemented atomic funcs. And even if there's
> > >>> atomic_check, not all bridges set the bus_flags there. So we need to either 1) fix the issue for now
> > >>> as in this patch, or 2) convert all bridges to use atomic funcs and fix all the bridges to set the
> > >>> bus_flags.
> > >>
> > >> The second option is what we'd like to achieve. Wouldn't it be best to
> > >> already start going in that direction ? We don't need to convert all
> > >> bridge drivers in one go here, just the ones that are used by tidss.

I took this as a future approach to eventually start supporting
atomic_funcs.
I will respin v4 of this series with updates to the other bridges
supporting atomic functions.

Nikhil Devshatwar

> > > 
> > > I think that sounds fine, except that this is blocking the DisplayPort support for J7. We have
> > > everything in for DP except dts changes (can be added only when the drivers work), and the connector
> > > stuff.
> > > 
> > > The connector stuff includes this series (so that tidss supports the new connector model), and
> > > "[PATCH RESEND v3 0/2] drm: add DisplayPort connector", which adds the connector driver.
> > > 
> > > The bridges currently used (that I know of) with tidss are cdns-mhdp, tfp410 and sii9022. I don't
> > > expect converting those would be a huge job, but I'd still really like to get the DP working in
> > > upstream without starting to expand the scope of the patches we need to enable it.
> > > 
> > > That said, we missed 5.11 so perhaps we have the time.
> 
> If there's not enough time to address the bridges, I'm fine with this
> series assuming the bridge changes will go on top. If we have enough
> time, let's go for it :-)
> 
> > Looks like Boris was missing from Cc in this series. Adding him.
> 
> -- 
> Regards,
> 
> Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 4/6] drm/tidss: Set bus_format correctly from bridge/connector

[Nikhil Devshatwar]

On 11:45-20201130, Laurent Pinchart wrote:
> Hi Nikhil,
> 
> Thank you for the patch.
> 
> On Thu, Nov 19, 2020 at 09:31:32PM +0530, Nikhil Devshatwar wrote:
> > Remove the old code to iterate over the bridge chain, as this is
> > already done by the framework.
> > The bridge state should have the negotiated bus format and flags.
> > Use these from the bridge's state.
> > If the bridge does not support format negotiation, error out
> > and fail.
> > 
> > Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
> > Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > ---
> > 
> > Notes:
> >     changes from v2:
> >     * Remove the old code and use the flags from the bridge state
> > 
> >  drivers/gpu/drm/tidss/tidss_encoder.c | 36 +++++++++++----------------
> >  1 file changed, 14 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/tidss/tidss_encoder.c b/drivers/gpu/drm/tidss/tidss_encoder.c
> > index e278a9c89476..08d5083c5508 100644
> > --- a/drivers/gpu/drm/tidss/tidss_encoder.c
> > +++ b/drivers/gpu/drm/tidss/tidss_encoder.c
> > @@ -21,37 +21,29 @@ 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;
> >  
> >  	dev_dbg(ddev->dev, "%s\n", __func__);
> >  
> > -	/*
> > -	 * Take the bus_flags from the first bridge that defines
> > -	 * bridge timings, or from the connector's display_info if no
> > -	 * bridge defines the timings.
> > -	 */
> > -	drm_for_each_bridge_in_chain(encoder, bridge) {
> > -		if (!bridge->timings)
> > -			continue;
> > -
> > -		tcrtc_state->bus_flags = bridge->timings->input_bus_flags;
> > -		bus_flags_set = true;
> > -		break;
> > +	/* Copy the bus_format and flags from the first bridge's state */
> > +	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;
> > +		tcrtc_state->bus_flags = bstate->input_bus_cfg.flags;
> > +	} else {
> > +		dev_err(ddev->dev, "Could not get the bridge state\n");
> > +		return -EINVAL;
> >  	}
> 
> I'd write this
> 
> 	bstate = drm_atomic_get_new_bridge_state(crtc_state->state, bridge);
> 	if (!bstate) {
> 		dev_err(ddev->dev, "Could not get the bridge state\n");
> 		return -EINVAL;
> 	}
> 
> 	tcrtc_state->bus_format = bstate->input_bus_cfg.format;
> 	tcrtc_state->bus_flags = bstate->input_bus_cfg.flags;

Looks better this way. I'll update

Nikhil Devshatwar

> >  
> > -	if (!di->bus_formats || di->num_bus_formats == 0)  {
> > -		dev_err(ddev->dev, "%s: No bus_formats in connected display\n",
> > -			__func__);
> > +	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;
> > -
> >  	return 0;
> >  }
> >  
> 
> -- 
> Regards,
> 
> Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 4/6] drm/tidss: Set bus_format correctly from bridge/connector

[Nikhil Devshatwar]

On 11:46-20201130, Laurent Pinchart wrote:
> Hi Nikhil,
> 
> On Mon, Nov 30, 2020 at 12:05:03PM +0530, Nikhil Devshatwar wrote:
> > On 14:51-20201125, Tomi Valkeinen wrote:
> > > On 19/11/2020 18:01, Nikhil Devshatwar wrote:
> > > > Remove the old code to iterate over the bridge chain, as this is
> > > > already done by the framework.
> > > > The bridge state should have the negotiated bus format and flags.
> > > > Use these from the bridge's state.
> > > > If the bridge does not support format negotiation, error out
> > > > and fail.
> > > > 
> > > > Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
> > > > Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > > > ---
> > > > 
> > > > Notes:
> > > >     changes from v2:
> > > >     * Remove the old code and use the flags from the bridge state
> > > > 
> > > >  drivers/gpu/drm/tidss/tidss_encoder.c | 36 +++++++++++----------------
> > > >  1 file changed, 14 insertions(+), 22 deletions(-)
> > > 
> > > If a first bridge (after the crtc) supports two bus formats as input, how does tidss choose between
> > > those? This patch just picks bstate->input_bus_cfg.format, and it's not clear to me which one that
> > > is (the first one?). Also, we don't check if tidss actually supports the bus format.
> > 
> > The selection is done by the framework in
> > select_bus_fmt_recursive at drivers/gpu/drm/drm_bridge.c:810
> > 
> > My understanding is that currently, the format negotiation logic does
> > not negotiate all the way till encoder, it stops only at the
> > first_bridge.
> 
> Should we then implement a bridge in the tidss driver to model the
> internal encoder, in order to support format negotiation all the way to
> the tidss ?

I am not sure. Scope of this series is to enable tidss with new
connector model. As of now, there aren't any bridges that report
unsupported format, so nothing is broken.

When the bridge drivers start reporting unsupported formats, we can
evaluate if it makes sense to implement the internal encoder as a bridge.

Nikhi Devshatwar

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

Re: [PATCH v3 4/6] drm/tidss: Set bus_format correctly from bridge/connector

[Tomi Valkeinen]

On 30/11/2020 11:46, Laurent Pinchart wrote:
> Hi Nikhil,
> 
> On Mon, Nov 30, 2020 at 12:05:03PM +0530, Nikhil Devshatwar wrote:
>> On 14:51-20201125, Tomi Valkeinen wrote:
>>> On 19/11/2020 18:01, Nikhil Devshatwar wrote:
>>>> Remove the old code to iterate over the bridge chain, as this is
>>>> already done by the framework.
>>>> The bridge state should have the negotiated bus format and flags.
>>>> Use these from the bridge's state.
>>>> If the bridge does not support format negotiation, error out
>>>> and fail.
>>>>
>>>> Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
>>>> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>>> ---
>>>>
>>>> Notes:
>>>>     changes from v2:
>>>>     * Remove the old code and use the flags from the bridge state
>>>>
>>>>  drivers/gpu/drm/tidss/tidss_encoder.c | 36 +++++++++++----------------
>>>>  1 file changed, 14 insertions(+), 22 deletions(-)
>>>
>>> If a first bridge (after the crtc) supports two bus formats as input, how does tidss choose between
>>> those? This patch just picks bstate->input_bus_cfg.format, and it's not clear to me which one that
>>> is (the first one?). Also, we don't check if tidss actually supports the bus format.
>>
>> The selection is done by the framework in
>> select_bus_fmt_recursive at drivers/gpu/drm/drm_bridge.c:810
>>
>> My understanding is that currently, the format negotiation logic does
>> not negotiate all the way till encoder, it stops only at the
>> first_bridge.
> 
> Should we then implement a bridge in the tidss driver to model the
> internal encoder, in order to support format negotiation all the way to
> the tidss ?

I don't know, but it feels perhaps a bit odd. Then we would have crtc + encoder + bridge, which are
actually all about the same HW block. And this would have to be done for all DRM drivers.

 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