All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/6] drm/tidss: Use new connector model for tidss
@ 2020-11-09 17:05 Nikhil Devshatwar
  2020-11-09 17:05 ` [PATCH v2 1/6] drm: bridge: Propagate the bus flags from bridge->timings Nikhil Devshatwar
                   ` (6 more replies)
  0 siblings, 7 replies; 21+ messages in thread
From: Nikhil Devshatwar @ 2020-11-09 17:05 UTC (permalink / raw)
  To: dri-devel, Tomi Valkeinen
  Cc: Sekhar Nori, Laurent Pinchart, Yuti Amonkar, Swapnil Jakhade

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   | 32 ++++++++++++++++--
 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         | 22 ++++++++-----
 drivers/gpu/drm/tidss/tidss_kms.c             | 19 ++++++++++-
 6 files changed, 106 insertions(+), 11 deletions(-)

-- 
2.17.1

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

^ permalink raw reply	[flat|nested] 21+ messages in thread
* [PATCH v2 1/6] drm: bridge: Propagate the bus flags from bridge->timings
  2020-11-09 17:05 [PATCH v2 0/6] drm/tidss: Use new connector model for tidss Nikhil Devshatwar
@ 2020-11-09 17:05 ` Nikhil Devshatwar
  2020-11-11 11:38   ` Tomi Valkeinen
  2020-11-09 17:05 ` [PATCH v2 2/6] drm/bridge: tfp410: Support format negotiation hooks Nikhil Devshatwar
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 21+ messages in thread
From: Nikhil Devshatwar @ 2020-11-09 17:05 UTC (permalink / raw)
  To: dri-devel, Tomi Valkeinen
  Cc: Sekhar Nori, Laurent Pinchart, Yuti Amonkar, Swapnil Jakhade

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>
---
 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..2bbd6ffe82ce 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 the bus flags are provided in timing, use those even if the next
+	 * bridge specifies something
+	 */
+	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

^ permalink raw reply related	[flat|nested] 21+ messages in thread
* [PATCH v2 2/6] drm/bridge: tfp410: Support format negotiation hooks
  2020-11-09 17:05 [PATCH v2 0/6] drm/tidss: Use new connector model for tidss Nikhil Devshatwar
  2020-11-09 17:05 ` [PATCH v2 1/6] drm: bridge: Propagate the bus flags from bridge->timings Nikhil Devshatwar
@ 2020-11-09 17:05 ` Nikhil Devshatwar
  2020-11-11 11:41   ` Tomi Valkeinen
  2020-11-09 17:05 ` [PATCH v2 3/6] drm/bridge: mhdp8546: Add minimal format negotiation Nikhil Devshatwar
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 21+ messages in thread
From: Nikhil Devshatwar @ 2020-11-09 17:05 UTC (permalink / raw)
  To: dri-devel, Tomi Valkeinen
  Cc: Sekhar Nori, Laurent Pinchart, Yuti Amonkar, Swapnil Jakhade

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

^ permalink raw reply related	[flat|nested] 21+ messages in thread
* [PATCH v2 3/6] drm/bridge: mhdp8546: Add minimal format negotiation
  2020-11-09 17:05 [PATCH v2 0/6] drm/tidss: Use new connector model for tidss Nikhil Devshatwar
  2020-11-09 17:05 ` [PATCH v2 1/6] drm: bridge: Propagate the bus flags from bridge->timings Nikhil Devshatwar
  2020-11-09 17:05 ` [PATCH v2 2/6] drm/bridge: tfp410: Support format negotiation hooks Nikhil Devshatwar
@ 2020-11-09 17:05 ` Nikhil Devshatwar
  2020-11-11 11:42   ` Tomi Valkeinen
  2020-11-09 17:05 ` [PATCH v2 4/6] drm/tidss: Set bus_format correctly from bridge/connector Nikhil Devshatwar
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 21+ messages in thread
From: Nikhil Devshatwar @ 2020-11-09 17:05 UTC (permalink / raw)
  To: dri-devel, Tomi Valkeinen
  Cc: Sekhar Nori, Laurent Pinchart, Yuti Amonkar, Swapnil Jakhade

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

^ permalink raw reply related	[flat|nested] 21+ messages in thread
* [PATCH v2 4/6] drm/tidss: Set bus_format correctly from bridge/connector
  2020-11-09 17:05 [PATCH v2 0/6] drm/tidss: Use new connector model for tidss Nikhil Devshatwar
                   ` (2 preceding siblings ...)
  2020-11-09 17:05 ` [PATCH v2 3/6] drm/bridge: mhdp8546: Add minimal format negotiation Nikhil Devshatwar
@ 2020-11-09 17:05 ` Nikhil Devshatwar
  2020-11-11 11:52   ` Tomi Valkeinen
  2020-11-09 17:06 ` [PATCH v2 5/6] drm/tidss: Move to newer connector model Nikhil Devshatwar
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 21+ messages in thread
From: Nikhil Devshatwar @ 2020-11-09 17:05 UTC (permalink / raw)
  To: dri-devel, Tomi Valkeinen
  Cc: Sekhar Nori, Laurent Pinchart, Yuti Amonkar, Swapnil Jakhade

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 | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_encoder.c b/drivers/gpu/drm/tidss/tidss_encoder.c
index e278a9c89476..ba5b6fccabe7 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,17 +42,22 @@ 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__);
-		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;
 
+	/* 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;
+	}
+
 	return 0;
 }
 
-- 
2.17.1

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

^ permalink raw reply related	[flat|nested] 21+ messages in thread
* [PATCH v2 5/6] drm/tidss: Move to newer connector model
  2020-11-09 17:05 [PATCH v2 0/6] drm/tidss: Use new connector model for tidss Nikhil Devshatwar
                   ` (3 preceding siblings ...)
  2020-11-09 17:05 ` [PATCH v2 4/6] drm/tidss: Set bus_format correctly from bridge/connector Nikhil Devshatwar
@ 2020-11-09 17:06 ` Nikhil Devshatwar
  2020-11-11 11:55   ` Tomi Valkeinen
  2020-11-09 17:06 ` [PATCH v2 6/6] drm/bridge: cdns-mhdp8546: Fix the interrupt enable/disable Nikhil Devshatwar
  2020-11-10  9:02 ` [PATCH v2 0/6] drm/tidss: Use new connector model for tidss Tomi Valkeinen
  6 siblings, 1 reply; 21+ messages in thread
From: Nikhil Devshatwar @ 2020-11-09 17:06 UTC (permalink / raw)
  To: dri-devel, Tomi Valkeinen
  Cc: Sekhar Nori, Laurent Pinchart, Yuti Amonkar, Swapnil Jakhade

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

^ permalink raw reply related	[flat|nested] 21+ messages in thread
* [PATCH v2 6/6] drm/bridge: cdns-mhdp8546: Fix the interrupt enable/disable
  2020-11-09 17:05 [PATCH v2 0/6] drm/tidss: Use new connector model for tidss Nikhil Devshatwar
                   ` (4 preceding siblings ...)
  2020-11-09 17:06 ` [PATCH v2 5/6] drm/tidss: Move to newer connector model Nikhil Devshatwar
@ 2020-11-09 17:06 ` Nikhil Devshatwar
  2020-11-10  9:21   ` Tomi Valkeinen
  2020-11-11 17:05   ` Swapnil Kashinath Jakhade
  2020-11-10  9:02 ` [PATCH v2 0/6] drm/tidss: Use new connector model for tidss Tomi Valkeinen
  6 siblings, 2 replies; 21+ messages in thread
From: Nikhil Devshatwar @ 2020-11-09 17:06 UTC (permalink / raw)
  To: dri-devel, Tomi Valkeinen
  Cc: Sekhar Nori, Laurent Pinchart, Yuti Amonkar, Swapnil Jakhade

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.
Do the same for enabling interrupts as well.

Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
---
 drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 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..6beccd2a408e 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
@@ -2146,7 +2146,8 @@ static void cdns_mhdp_bridge_hpd_enable(struct drm_bridge *bridge)
 
 	/* Enable SW event interrupts */
 	if (mhdp->bridge_attached)
-		writel(~(u32)CDNS_APB_INT_MASK_SW_EVENT_INT,
+		writel(readl(mhdp->regs + CDNS_APB_INT_MASK) &
+		       ~CDNS_APB_INT_MASK_SW_EVENT_INT,
 		       mhdp->regs + CDNS_APB_INT_MASK);
 }
 
@@ -2154,7 +2155,9 @@ 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);
+	writel(readl(mhdp->regs + CDNS_APB_INT_MASK) |
+	       CDNS_APB_INT_MASK_SW_EVENT_INT,
+	       mhdp->regs + CDNS_APB_INT_MASK);
 }
 
 static const struct drm_bridge_funcs cdns_mhdp_bridge_funcs = {
-- 
2.17.1

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

^ permalink raw reply related	[flat|nested] 21+ messages in thread
* Re: [PATCH v2 0/6] drm/tidss: Use new connector model for tidss
  2020-11-09 17:05 [PATCH v2 0/6] drm/tidss: Use new connector model for tidss Nikhil Devshatwar
                   ` (5 preceding siblings ...)
  2020-11-09 17:06 ` [PATCH v2 6/6] drm/bridge: cdns-mhdp8546: Fix the interrupt enable/disable Nikhil Devshatwar
@ 2020-11-10  9:02 ` Tomi Valkeinen
  2020-11-10 10:30   ` Nikhil Devshatwar
  6 siblings, 1 reply; 21+ messages in thread
From: Tomi Valkeinen @ 2020-11-10  9:02 UTC (permalink / raw)
  To: Nikhil Devshatwar, dri-devel
  Cc: Sekhar Nori, Laurent Pinchart, Yuti Amonkar, Swapnil Jakhade

Hi Nikhil,

On 09/11/2020 19:05, 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
> 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   | 32 ++++++++++++++++--
>  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         | 22 ++++++++-----
>  drivers/gpu/drm/tidss/tidss_kms.c             | 19 ++++++++++-
>  6 files changed, 106 insertions(+), 11 deletions(-)
> 

Please add a change log when sending new versions of a series.

 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

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH v2 6/6] drm/bridge: cdns-mhdp8546: Fix the interrupt enable/disable
  2020-11-09 17:06 ` [PATCH v2 6/6] drm/bridge: cdns-mhdp8546: Fix the interrupt enable/disable Nikhil Devshatwar
@ 2020-11-10  9:21   ` Tomi Valkeinen
  2020-11-10 10:27     ` Nikhil Devshatwar
  2020-11-11 17:05   ` Swapnil Kashinath Jakhade
  1 sibling, 1 reply; 21+ messages in thread
From: Tomi Valkeinen @ 2020-11-10  9:21 UTC (permalink / raw)
  To: Nikhil Devshatwar, dri-devel, Swapnil Jakhade
  Cc: Sekhar Nori, Laurent Pinchart, Yuti Amonkar

On 09/11/2020 19:06, 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.
> Do the same for enabling interrupts as well.
> 
> Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
> ---
>  drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 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..6beccd2a408e 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> @@ -2146,7 +2146,8 @@ static void cdns_mhdp_bridge_hpd_enable(struct drm_bridge *bridge)
>  
>  	/* Enable SW event interrupts */
>  	if (mhdp->bridge_attached)
> -		writel(~(u32)CDNS_APB_INT_MASK_SW_EVENT_INT,
> +		writel(readl(mhdp->regs + CDNS_APB_INT_MASK) &
> +		       ~CDNS_APB_INT_MASK_SW_EVENT_INT,
>  		       mhdp->regs + CDNS_APB_INT_MASK);
>  }
>  
> @@ -2154,7 +2155,9 @@ 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);
> +	writel(readl(mhdp->regs + CDNS_APB_INT_MASK) |
> +	       CDNS_APB_INT_MASK_SW_EVENT_INT,
> +	       mhdp->regs + CDNS_APB_INT_MASK);
>  }
>  
>  static const struct drm_bridge_funcs cdns_mhdp_bridge_funcs = {

Good catch. I wonder why we need the above functions... We already enable and disable the interrupts
when attaching/detaching the driver. And I think we want to get the interrupt even if we won't
report HPD (but I think we always do report it), as we need the interrupts to track the link status.

 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

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH v2 6/6] drm/bridge: cdns-mhdp8546: Fix the interrupt enable/disable
  2020-11-10  9:21   ` Tomi Valkeinen
@ 2020-11-10 10:27     ` Nikhil Devshatwar
  2020-11-10 12:27       ` Tomi Valkeinen
  0 siblings, 1 reply; 21+ messages in thread
From: Nikhil Devshatwar @ 2020-11-10 10:27 UTC (permalink / raw)
  To: Tomi Valkeinen
  Cc: Yuti Amonkar, Sekhar Nori, Laurent Pinchart, dri-devel, Swapnil Jakhade

On 11:21-20201110, Tomi Valkeinen wrote:
> On 09/11/2020 19:06, 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.
> > Do the same for enabling interrupts as well.
> > 
> > Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
> > ---
> >  drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 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..6beccd2a408e 100644
> > --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> > +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> > @@ -2146,7 +2146,8 @@ static void cdns_mhdp_bridge_hpd_enable(struct drm_bridge *bridge)
> >  
> >  	/* Enable SW event interrupts */
> >  	if (mhdp->bridge_attached)
> > -		writel(~(u32)CDNS_APB_INT_MASK_SW_EVENT_INT,
> > +		writel(readl(mhdp->regs + CDNS_APB_INT_MASK) &
> > +		       ~CDNS_APB_INT_MASK_SW_EVENT_INT,
> >  		       mhdp->regs + CDNS_APB_INT_MASK);
> >  }
> >  
> > @@ -2154,7 +2155,9 @@ 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);
> > +	writel(readl(mhdp->regs + CDNS_APB_INT_MASK) |
> > +	       CDNS_APB_INT_MASK_SW_EVENT_INT,
> > +	       mhdp->regs + CDNS_APB_INT_MASK);
> >  }
> >  
> >  static const struct drm_bridge_funcs cdns_mhdp_bridge_funcs = {
> 
> Good catch. I wonder why we need the above functions... We already enable and disable the interrupts
> when attaching/detaching the driver. And I think we want to get the interrupt even if we won't
> report HPD (but I think we always do report it), as we need the interrupts to track the link status.
> 

I read from the code that there is TODO for handling the mailbox
interrupts in the driver. Once that is supported, you will be able to
explictily enable/disable interrupts for SW_EVENTS (like hotplug) as
well as mailbox events. This enabling specific bits in the interrupt
status.


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

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH v2 0/6] drm/tidss: Use new connector model for tidss
  2020-11-10  9:02 ` [PATCH v2 0/6] drm/tidss: Use new connector model for tidss Tomi Valkeinen
@ 2020-11-10 10:30   ` Nikhil Devshatwar
  0 siblings, 0 replies; 21+ messages in thread
From: Nikhil Devshatwar @ 2020-11-10 10:30 UTC (permalink / raw)
  To: Tomi Valkeinen
  Cc: Yuti Amonkar, Sekhar Nori, Laurent Pinchart, dri-devel, Swapnil Jakhade

On 11:02-20201110, Tomi Valkeinen wrote:
> Hi Nikhil,
> 
> On 09/11/2020 19:05, 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
> > as a first step before moving the connector model.
> > 
> > Nikhil Devshatwar (6):
> >   drm: bridge: Propagate the bus flags from bridge->timings
    changes from v1:
    * Check for timings
    * Prioritize timings flags over next bridge's flags

> >   drm/bridge: tfp410: Support format negotiation hooks
    Changes from v1:
    * Use only MEDIA_BUS_FMT_FIXED for output

> >   drm/bridge: mhdp8546: Add minimal format negotiation
    Changes from v1:
    * cosmetic fixes, commit message update

> >   drm/tidss: Set bus_format correctly from bridge/connector
> >   drm/tidss: Move to newer connector model
    Changes from v1:
    * Add error handling

> >   drm/bridge: cdns-mhdp8546: Fix the interrupt enable/disable

> > 
> >  .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 32 ++++++++++++++++--
> >  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         | 22 ++++++++-----
> >  drivers/gpu/drm/tidss/tidss_kms.c             | 19 ++++++++++-
> >  6 files changed, 106 insertions(+), 11 deletions(-)
> > 
> 
> Please add a change log when sending new versions of a series.
> 

Actually, I had added the git notes, but missed to add --notes while
sending the patches using git send-email.

I am updating the change log here manually.

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

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH v2 6/6] drm/bridge: cdns-mhdp8546: Fix the interrupt enable/disable
  2020-11-10 10:27     ` Nikhil Devshatwar
@ 2020-11-10 12:27       ` Tomi Valkeinen
  2020-11-10 13:53         ` Nikhil Devshatwar
  0 siblings, 1 reply; 21+ messages in thread
From: Tomi Valkeinen @ 2020-11-10 12:27 UTC (permalink / raw)
  To: Nikhil Devshatwar
  Cc: Yuti Amonkar, Sekhar Nori, Laurent Pinchart, dri-devel, Swapnil Jakhade

On 10/11/2020 12:27, Nikhil Devshatwar wrote:
> On 11:21-20201110, Tomi Valkeinen wrote:
>> On 09/11/2020 19:06, 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.
>>> Do the same for enabling interrupts as well.
>>>
>>> Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
>>> ---
>>>  drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c | 7 +++++--
>>>  1 file changed, 5 insertions(+), 2 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..6beccd2a408e 100644
>>> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>>> @@ -2146,7 +2146,8 @@ static void cdns_mhdp_bridge_hpd_enable(struct drm_bridge *bridge)
>>>  
>>>  	/* Enable SW event interrupts */
>>>  	if (mhdp->bridge_attached)
>>> -		writel(~(u32)CDNS_APB_INT_MASK_SW_EVENT_INT,
>>> +		writel(readl(mhdp->regs + CDNS_APB_INT_MASK) &
>>> +		       ~CDNS_APB_INT_MASK_SW_EVENT_INT,
>>>  		       mhdp->regs + CDNS_APB_INT_MASK);
>>>  }
>>>  
>>> @@ -2154,7 +2155,9 @@ 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);
>>> +	writel(readl(mhdp->regs + CDNS_APB_INT_MASK) |
>>> +	       CDNS_APB_INT_MASK_SW_EVENT_INT,
>>> +	       mhdp->regs + CDNS_APB_INT_MASK);
>>>  }
>>>  
>>>  static const struct drm_bridge_funcs cdns_mhdp_bridge_funcs = {
>>
>> Good catch. I wonder why we need the above functions... We already enable and disable the interrupts
>> when attaching/detaching the driver. And I think we want to get the interrupt even if we won't
>> report HPD (but I think we always do report it), as we need the interrupts to track the link status.
>>
> 
> I read from the code that there is TODO for handling the mailbox
> interrupts in the driver. Once that is supported, you will be able to
> explictily enable/disable interrupts for SW_EVENTS (like hotplug) as
> well as mailbox events. This enabling specific bits in the interrupt
> status.

But SW_EVENTS is not the same as HPD, at least in theory. If we disable SW_EVENT_INT in
hpd_disable(), we lose all SW_EVENT interrupts.

 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

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH v2 6/6] drm/bridge: cdns-mhdp8546: Fix the interrupt enable/disable
  2020-11-10 12:27       ` Tomi Valkeinen
@ 2020-11-10 13:53         ` Nikhil Devshatwar
  2020-11-10 14:32           ` Swapnil Kashinath Jakhade
  0 siblings, 1 reply; 21+ messages in thread
From: Nikhil Devshatwar @ 2020-11-10 13:53 UTC (permalink / raw)
  To: Tomi Valkeinen, Swapnil Jakhade, Yuti Amonkar
  Cc: Yuti Amonkar, Sekhar Nori, Laurent Pinchart, dri-devel, Swapnil Jakhade

On 14:27-20201110, Tomi Valkeinen wrote:
> On 10/11/2020 12:27, Nikhil Devshatwar wrote:
> > On 11:21-20201110, Tomi Valkeinen wrote:
> >> On 09/11/2020 19:06, 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.
> >>> Do the same for enabling interrupts as well.
> >>>
> >>> Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
> >>> ---
> >>>  drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c | 7 +++++--
> >>>  1 file changed, 5 insertions(+), 2 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..6beccd2a408e 100644
> >>> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> >>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> >>> @@ -2146,7 +2146,8 @@ static void cdns_mhdp_bridge_hpd_enable(struct drm_bridge *bridge)
> >>>  
> >>>  	/* Enable SW event interrupts */
> >>>  	if (mhdp->bridge_attached)
> >>> -		writel(~(u32)CDNS_APB_INT_MASK_SW_EVENT_INT,
> >>> +		writel(readl(mhdp->regs + CDNS_APB_INT_MASK) &
> >>> +		       ~CDNS_APB_INT_MASK_SW_EVENT_INT,
> >>>  		       mhdp->regs + CDNS_APB_INT_MASK);
> >>>  }
> >>>  
> >>> @@ -2154,7 +2155,9 @@ 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);
> >>> +	writel(readl(mhdp->regs + CDNS_APB_INT_MASK) |
> >>> +	       CDNS_APB_INT_MASK_SW_EVENT_INT,
> >>> +	       mhdp->regs + CDNS_APB_INT_MASK);
> >>>  }
> >>>  
> >>>  static const struct drm_bridge_funcs cdns_mhdp_bridge_funcs = {
> >>
> >> Good catch. I wonder why we need the above functions... We already enable and disable the interrupts
> >> when attaching/detaching the driver. And I think we want to get the interrupt even if we won't
> >> report HPD (but I think we always do report it), as we need the interrupts to track the link status.
> >>
> > 
> > I read from the code that there is TODO for handling the mailbox
> > interrupts in the driver. Once that is supported, you will be able to
> > explictily enable/disable interrupts for SW_EVENTS (like hotplug) as
> > well as mailbox events. This enabling specific bits in the interrupt
> > status.
> 
> But SW_EVENTS is not the same as HPD, at least in theory. If we disable SW_EVENT_INT in
> hpd_disable(), we lose all SW_EVENT interrupts.

I am not sure, what exactly is covered in the SW events apart from the hotplug.

Swapnil, Yuti, Please fill in..

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

^ permalink raw reply	[flat|nested] 21+ messages in thread
* RE: [PATCH v2 6/6] drm/bridge: cdns-mhdp8546: Fix the interrupt enable/disable
  2020-11-10 13:53         ` Nikhil Devshatwar
@ 2020-11-10 14:32           ` Swapnil Kashinath Jakhade
  2020-11-10 14:50             ` Nikhil Devshatwar
  0 siblings, 1 reply; 21+ messages in thread
From: Swapnil Kashinath Jakhade @ 2020-11-10 14:32 UTC (permalink / raw)
  To: Nikhil Devshatwar, Tomi Valkeinen, Yuti Suresh Amonkar
  Cc: Yuti Suresh Amonkar, Sekhar Nori, Laurent Pinchart, dri-devel



> -----Original Message-----
> From: Nikhil Devshatwar <nikhil.nd@ti.com>
> Sent: Tuesday, November 10, 2020 7:23 PM
> To: Tomi Valkeinen <tomi.valkeinen@ti.com>; Swapnil Kashinath Jakhade
> <sjakhade@cadence.com>; Yuti Suresh Amonkar <yamonkar@cadence.com>
> Cc: dri-devel@lists.freedesktop.org; Swapnil Kashinath Jakhade
> <sjakhade@cadence.com>; Sekhar Nori <nsekhar@ti.com>; Laurent Pinchart
> <laurent.pinchart@ideasonboard.com>; Yuti Suresh Amonkar
> <yamonkar@cadence.com>
> Subject: Re: [PATCH v2 6/6] drm/bridge: cdns-mhdp8546: Fix the interrupt
> enable/disable
> 
> EXTERNAL MAIL
> 
> 
> On 14:27-20201110, Tomi Valkeinen wrote:
> > On 10/11/2020 12:27, Nikhil Devshatwar wrote:
> > > On 11:21-20201110, Tomi Valkeinen wrote:
> > >> On 09/11/2020 19:06, 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.
> > >>> Do the same for enabling interrupts as well.
> > >>>
> > >>> Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
> > >>> ---
> > >>>  drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c | 7 +++++--
> > >>>  1 file changed, 5 insertions(+), 2 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..6beccd2a408e 100644
> > >>> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> > >>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> > >>> @@ -2146,7 +2146,8 @@ static void
> > >>> cdns_mhdp_bridge_hpd_enable(struct drm_bridge *bridge)
> > >>>
> > >>>  	/* Enable SW event interrupts */
> > >>>  	if (mhdp->bridge_attached)
> > >>> -		writel(~(u32)CDNS_APB_INT_MASK_SW_EVENT_INT,
> > >>> +		writel(readl(mhdp->regs + CDNS_APB_INT_MASK) &
> > >>> +		       ~CDNS_APB_INT_MASK_SW_EVENT_INT,
> > >>>  		       mhdp->regs + CDNS_APB_INT_MASK);  }
> > >>>
> > >>> @@ -2154,7 +2155,9 @@ 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);
> > >>> +	writel(readl(mhdp->regs + CDNS_APB_INT_MASK) |
> > >>> +	       CDNS_APB_INT_MASK_SW_EVENT_INT,
> > >>> +	       mhdp->regs + CDNS_APB_INT_MASK);
> > >>>  }
> > >>>
> > >>>  static const struct drm_bridge_funcs cdns_mhdp_bridge_funcs = {
> > >>
> > >> Good catch. I wonder why we need the above functions... We already
> > >> enable and disable the interrupts when attaching/detaching the
> > >> driver. And I think we want to get the interrupt even if we won't report
> HPD (but I think we always do report it), as we need the interrupts to track
> the link status.
> > >>
> > >
> > > I read from the code that there is TODO for handling the mailbox
> > > interrupts in the driver. Once that is supported, you will be able
> > > to explictily enable/disable interrupts for SW_EVENTS (like hotplug)
> > > as well as mailbox events. This enabling specific bits in the
> > > interrupt status.
> >
> > But SW_EVENTS is not the same as HPD, at least in theory. If we
> > disable SW_EVENT_INT in hpd_disable(), we lose all SW_EVENT interrupts.
> 
> I am not sure, what exactly is covered in the SW events apart from the
> hotplug.
> 
> Swapnil, Yuti, Please fill in..

hpd_enable/hpd_disable callbacks were implemented as a part of supporting
DRM_BRIDGE_OP_HPD bridge operation. The existing implementation could
work with current features set supported by MHDP driver. But Tomi's point is
valid, as there are some HDCP interrupts which are part of SW_EVENT interrupts
and this might not be the control to just enable/disable HPD.

Swapnil

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

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH v2 6/6] drm/bridge: cdns-mhdp8546: Fix the interrupt enable/disable
  2020-11-10 14:32           ` Swapnil Kashinath Jakhade
@ 2020-11-10 14:50             ` Nikhil Devshatwar
  0 siblings, 0 replies; 21+ messages in thread
From: Nikhil Devshatwar @ 2020-11-10 14:50 UTC (permalink / raw)
  To: Swapnil Kashinath Jakhade
  Cc: Tomi Valkeinen, Sekhar Nori, dri-devel, Yuti Suresh Amonkar,
	Laurent Pinchart

On 14:32-20201110, Swapnil Jakhade wrote:
> 
> 
> > -----Original Message-----
> > From: Nikhil Devshatwar <nikhil.nd@ti.com>
> > Sent: Tuesday, November 10, 2020 7:23 PM
> > To: Tomi Valkeinen <tomi.valkeinen@ti.com>; Swapnil Kashinath Jakhade
> > <sjakhade@cadence.com>; Yuti Suresh Amonkar <yamonkar@cadence.com>
> > Cc: dri-devel@lists.freedesktop.org; Swapnil Kashinath Jakhade
> > <sjakhade@cadence.com>; Sekhar Nori <nsekhar@ti.com>; Laurent Pinchart
> > <laurent.pinchart@ideasonboard.com>; Yuti Suresh Amonkar
> > <yamonkar@cadence.com>
> > Subject: Re: [PATCH v2 6/6] drm/bridge: cdns-mhdp8546: Fix the interrupt
> > enable/disable
> > 
> > EXTERNAL MAIL
> > 
> > 
> > On 14:27-20201110, Tomi Valkeinen wrote:
> > > On 10/11/2020 12:27, Nikhil Devshatwar wrote:
> > > > On 11:21-20201110, Tomi Valkeinen wrote:
> > > >> On 09/11/2020 19:06, 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.
> > > >>> Do the same for enabling interrupts as well.
> > > >>>
> > > >>> Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
> > > >>> ---
> > > >>>  drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c | 7 +++++--
> > > >>>  1 file changed, 5 insertions(+), 2 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..6beccd2a408e 100644
> > > >>> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> > > >>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> > > >>> @@ -2146,7 +2146,8 @@ static void
> > > >>> cdns_mhdp_bridge_hpd_enable(struct drm_bridge *bridge)
> > > >>>
> > > >>>  	/* Enable SW event interrupts */
> > > >>>  	if (mhdp->bridge_attached)
> > > >>> -		writel(~(u32)CDNS_APB_INT_MASK_SW_EVENT_INT,
> > > >>> +		writel(readl(mhdp->regs + CDNS_APB_INT_MASK) &
> > > >>> +		       ~CDNS_APB_INT_MASK_SW_EVENT_INT,
> > > >>>  		       mhdp->regs + CDNS_APB_INT_MASK);  }
> > > >>>
> > > >>> @@ -2154,7 +2155,9 @@ 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);
> > > >>> +	writel(readl(mhdp->regs + CDNS_APB_INT_MASK) |
> > > >>> +	       CDNS_APB_INT_MASK_SW_EVENT_INT,
> > > >>> +	       mhdp->regs + CDNS_APB_INT_MASK);
> > > >>>  }
> > > >>>
> > > >>>  static const struct drm_bridge_funcs cdns_mhdp_bridge_funcs = {
> > > >>
> > > >> Good catch. I wonder why we need the above functions... We already
> > > >> enable and disable the interrupts when attaching/detaching the
> > > >> driver. And I think we want to get the interrupt even if we won't report
> > HPD (but I think we always do report it), as we need the interrupts to track
> > the link status.
> > > >>
> > > >
> > > > I read from the code that there is TODO for handling the mailbox
> > > > interrupts in the driver. Once that is supported, you will be able
> > > > to explictily enable/disable interrupts for SW_EVENTS (like hotplug)
> > > > as well as mailbox events. This enabling specific bits in the
> > > > interrupt status.
> > >
> > > But SW_EVENTS is not the same as HPD, at least in theory. If we
> > > disable SW_EVENT_INT in hpd_disable(), we lose all SW_EVENT interrupts.
> > 
> > I am not sure, what exactly is covered in the SW events apart from the
> > hotplug.
> > 
> > Swapnil, Yuti, Please fill in..
> 
> hpd_enable/hpd_disable callbacks were implemented as a part of supporting
> DRM_BRIDGE_OP_HPD bridge operation. The existing implementation could
> work with current features set supported by MHDP driver. But Tomi's point is
> valid, as there are some HDCP interrupts which are part of SW_EVENT interrupts
> and this might not be the control to just enable/disable HPD.

So do you think something should change now?
I assume that you will modify this part when supporting HDCP.
Also, please provide your ack if this patch is the right fix.


Nikhil D
> 
> Swapnil
> 
> > 
> > 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

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/6] drm: bridge: Propagate the bus flags from bridge->timings
  2020-11-09 17:05 ` [PATCH v2 1/6] drm: bridge: Propagate the bus flags from bridge->timings Nikhil Devshatwar
@ 2020-11-11 11:38   ` Tomi Valkeinen
  0 siblings, 0 replies; 21+ messages in thread
From: Tomi Valkeinen @ 2020-11-11 11:38 UTC (permalink / raw)
  To: Nikhil Devshatwar, dri-devel
  Cc: Sekhar Nori, Laurent Pinchart, Yuti Amonkar, Swapnil Jakhade

On 09/11/2020 19:05, 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>
> ---
>  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..2bbd6ffe82ce 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 the bus flags are provided in timing, use those even if the next
> +	 * bridge specifies something
> +	 */
> +	if (bridge->timings && bridge->timings->input_bus_flags)
> +		bridge_state->input_bus_cfg.flags =
> +			bridge->timings->input_bus_flags;
>  }

I think the comment could be more clear. Maybe something like:

If legacy bus flags are provided in bridge->timings, use those as input flags instead of propagating
the output flags.

Other than that:

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

 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

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/6] drm/bridge: tfp410: Support format negotiation hooks
  2020-11-09 17:05 ` [PATCH v2 2/6] drm/bridge: tfp410: Support format negotiation hooks Nikhil Devshatwar
@ 2020-11-11 11:41   ` Tomi Valkeinen
  0 siblings, 0 replies; 21+ messages in thread
From: Tomi Valkeinen @ 2020-11-11 11:41 UTC (permalink / raw)
  To: Nikhil Devshatwar, dri-devel
  Cc: Sekhar Nori, Laurent Pinchart, Yuti Amonkar, Swapnil Jakhade

On 09/11/2020 19:05, 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>
> ---
>  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..f56dbe246e57 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
> +	 */

Please fix the missing punctiation marks.

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

 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

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/6] drm/bridge: mhdp8546: Add minimal format negotiation
  2020-11-09 17:05 ` [PATCH v2 3/6] drm/bridge: mhdp8546: Add minimal format negotiation Nikhil Devshatwar
@ 2020-11-11 11:42   ` Tomi Valkeinen
  0 siblings, 0 replies; 21+ messages in thread
From: Tomi Valkeinen @ 2020-11-11 11:42 UTC (permalink / raw)
  To: Nikhil Devshatwar, dri-devel
  Cc: Sekhar Nori, Laurent Pinchart, Yuti Amonkar, Swapnil Jakhade

On 09/11/2020 19:05, 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 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>
> ---
>  .../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,
> 

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

 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

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH v2 4/6] drm/tidss: Set bus_format correctly from bridge/connector
  2020-11-09 17:05 ` [PATCH v2 4/6] drm/tidss: Set bus_format correctly from bridge/connector Nikhil Devshatwar
@ 2020-11-11 11:52   ` Tomi Valkeinen
  0 siblings, 0 replies; 21+ messages in thread
From: Tomi Valkeinen @ 2020-11-11 11:52 UTC (permalink / raw)
  To: Nikhil Devshatwar, dri-devel
  Cc: Sekhar Nori, Laurent Pinchart, Yuti Amonkar, Swapnil Jakhade

On 09/11/2020 19:05, 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 | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_encoder.c b/drivers/gpu/drm/tidss/tidss_encoder.c
> index e278a9c89476..ba5b6fccabe7 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,17 +42,22 @@ 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__);
> -		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;
>  
> +	/* 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;
> +	}
> +
>  	return 0;

We should also take the bus flags from the bridge state (next bridge's input_bus_cfg.flags).

And... If I read this right, we always fail if there's no bstate. So we could just do "if (!bstate)
fail;"

And if we always expect to have bstate, and get bus flags and formats from there, we don't need the
current drm_for_each_bridge_in_chain() loop or taking bus_flags from display info.

 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

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH v2 5/6] drm/tidss: Move to newer connector model
  2020-11-09 17:06 ` [PATCH v2 5/6] drm/tidss: Move to newer connector model Nikhil Devshatwar
@ 2020-11-11 11:55   ` Tomi Valkeinen
  0 siblings, 0 replies; 21+ messages in thread
From: Tomi Valkeinen @ 2020-11-11 11:55 UTC (permalink / raw)
  To: Nikhil Devshatwar, dri-devel
  Cc: Sekhar Nori, Laurent Pinchart, Yuti Amonkar, Swapnil Jakhade

On 09/11/2020 19:06, 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 | 19 ++++++++++++++++++-
>  2 files changed, 21 insertions(+), 1 deletion(-)
> 

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

You're missing Laurent's r-by, which he gave earlier.

 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

^ permalink raw reply	[flat|nested] 21+ messages in thread
* RE: [PATCH v2 6/6] drm/bridge: cdns-mhdp8546: Fix the interrupt enable/disable
  2020-11-09 17:06 ` [PATCH v2 6/6] drm/bridge: cdns-mhdp8546: Fix the interrupt enable/disable Nikhil Devshatwar
  2020-11-10  9:21   ` Tomi Valkeinen
@ 2020-11-11 17:05   ` Swapnil Kashinath Jakhade
  1 sibling, 0 replies; 21+ messages in thread
From: Swapnil Kashinath Jakhade @ 2020-11-11 17:05 UTC (permalink / raw)
  To: Nikhil Devshatwar, dri-devel, Tomi Valkeinen
  Cc: Sekhar Nori, Laurent Pinchart, Yuti Suresh Amonkar

Hi Nikhil,

> -----Original Message-----
> From: Nikhil Devshatwar <nikhil.nd@ti.com>
> Sent: Monday, November 9, 2020 10:36 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>; Yuti Suresh Amonkar <yamonkar@cadence.com>
> Subject: [PATCH v2 6/6] drm/bridge: cdns-mhdp8546: Fix the interrupt
> enable/disable
> 
> EXTERNAL MAIL
> 
> 
> 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.
> Do the same for enabling interrupts as well.
> 
> Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
> ---
>  drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 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..6beccd2a408e 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> @@ -2146,7 +2146,8 @@ static void cdns_mhdp_bridge_hpd_enable(struct
> drm_bridge *bridge)
> 
>  	/* Enable SW event interrupts */
>  	if (mhdp->bridge_attached)
> -		writel(~(u32)CDNS_APB_INT_MASK_SW_EVENT_INT,
> +		writel(readl(mhdp->regs + CDNS_APB_INT_MASK) &
> +		       ~CDNS_APB_INT_MASK_SW_EVENT_INT,
>  		       mhdp->regs + CDNS_APB_INT_MASK);  }
> 
> @@ -2154,7 +2155,9 @@ 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);
> +	writel(readl(mhdp->regs + CDNS_APB_INT_MASK) |
> +	       CDNS_APB_INT_MASK_SW_EVENT_INT,
> +	       mhdp->regs + CDNS_APB_INT_MASK);
>  }
> 

Can we do similar change at other places in driver too?
Other than that:
Reviewed-by: Swapnil Jakhade <sjakhade@cadence.com>

Thanks & regards,
Swapnil

>  static const struct drm_bridge_funcs cdns_mhdp_bridge_funcs = {
> --
> 2.17.1

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

^ permalink raw reply	[flat|nested] 21+ messages in thread
end of thread, other threads:[~2020-11-12  8:16 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-09 17:05 [PATCH v2 0/6] drm/tidss: Use new connector model for tidss Nikhil Devshatwar
2020-11-09 17:05 ` [PATCH v2 1/6] drm: bridge: Propagate the bus flags from bridge->timings Nikhil Devshatwar
2020-11-11 11:38   ` Tomi Valkeinen
2020-11-09 17:05 ` [PATCH v2 2/6] drm/bridge: tfp410: Support format negotiation hooks Nikhil Devshatwar
2020-11-11 11:41   ` Tomi Valkeinen
2020-11-09 17:05 ` [PATCH v2 3/6] drm/bridge: mhdp8546: Add minimal format negotiation Nikhil Devshatwar
2020-11-11 11:42   ` Tomi Valkeinen
2020-11-09 17:05 ` [PATCH v2 4/6] drm/tidss: Set bus_format correctly from bridge/connector Nikhil Devshatwar
2020-11-11 11:52   ` Tomi Valkeinen
2020-11-09 17:06 ` [PATCH v2 5/6] drm/tidss: Move to newer connector model Nikhil Devshatwar
2020-11-11 11:55   ` Tomi Valkeinen
2020-11-09 17:06 ` [PATCH v2 6/6] drm/bridge: cdns-mhdp8546: Fix the interrupt enable/disable Nikhil Devshatwar
2020-11-10  9:21   ` Tomi Valkeinen
2020-11-10 10:27     ` Nikhil Devshatwar
2020-11-10 12:27       ` Tomi Valkeinen
2020-11-10 13:53         ` Nikhil Devshatwar
2020-11-10 14:32           ` Swapnil Kashinath Jakhade
2020-11-10 14:50             ` Nikhil Devshatwar
2020-11-11 17:05   ` Swapnil Kashinath Jakhade
2020-11-10  9:02 ` [PATCH v2 0/6] drm/tidss: Use new connector model for tidss Tomi Valkeinen
2020-11-10 10:30   ` Nikhil Devshatwar

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.