* [PATCH v3 0/3] drm/bridge: ti-sn65dsi86: Support non-eDP DisplayPort connectors
@ 2022-03-10 15:22 ` Kieran Bingham
0 siblings, 0 replies; 33+ messages in thread
From: Kieran Bingham @ 2022-03-10 15:22 UTC (permalink / raw)
To: dri-devel, linux-renesas-soc, Sam Ravnborg, Douglas Anderson,
linux-kernel
Cc: Jonas Karlman, David Airlie, Robert Foss, Neil Armstrong,
Jernej Skrabec, Kieran Bingham, Laurent Pinchart, Andrzej Hajda
Implement support for non eDP connectors on the TI-SN65DSI86 bridge, and
provide IRQ based hotplug detect to identify when the connector is
present.
no-hpd is extended to be the default behaviour for non DisplayPort
connectors.
This series is based upon Sam Ravnborgs and Rob Clarks series [0] to
support DRM_BRIDGE_STATE_OPS and NO_CONNECTOR support on the SN65DSI86,
however some extra modifications have been made on the top of Sam's
series to fix compile breakage and the NO_CONNECTOR support.
A full branch with these changes is available at [1]
[0] https://lore.kernel.org/all/20220206154405.1243333-1-sam@ravnborg.org/
[1] git://git.kernel.org/pub/scm/linux/kernel/git/kbingham/rcar.git
branch: kbingham/drm-misc/next/sn65dsi86/hpd
Kieran Bingham (1):
drm/bridge: ti-sn65dsi86: Support hotplug detection
Laurent Pinchart (2):
drm/bridge: ti-sn65dsi86: Support DisplayPort (non-eDP) mode
drm/bridge: ti-sn65dsi86: Implement bridge connector operations
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 180 +++++++++++++++++++++++---
1 file changed, 165 insertions(+), 15 deletions(-)
--
2.32.0
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v3 0/3] drm/bridge: ti-sn65dsi86: Support non-eDP DisplayPort connectors
@ 2022-03-10 15:22 ` Kieran Bingham
0 siblings, 0 replies; 33+ messages in thread
From: Kieran Bingham @ 2022-03-10 15:22 UTC (permalink / raw)
To: dri-devel, linux-renesas-soc, Sam Ravnborg, Douglas Anderson,
linux-kernel
Cc: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter,
Kieran Bingham
Implement support for non eDP connectors on the TI-SN65DSI86 bridge, and
provide IRQ based hotplug detect to identify when the connector is
present.
no-hpd is extended to be the default behaviour for non DisplayPort
connectors.
This series is based upon Sam Ravnborgs and Rob Clarks series [0] to
support DRM_BRIDGE_STATE_OPS and NO_CONNECTOR support on the SN65DSI86,
however some extra modifications have been made on the top of Sam's
series to fix compile breakage and the NO_CONNECTOR support.
A full branch with these changes is available at [1]
[0] https://lore.kernel.org/all/20220206154405.1243333-1-sam@ravnborg.org/
[1] git://git.kernel.org/pub/scm/linux/kernel/git/kbingham/rcar.git
branch: kbingham/drm-misc/next/sn65dsi86/hpd
Kieran Bingham (1):
drm/bridge: ti-sn65dsi86: Support hotplug detection
Laurent Pinchart (2):
drm/bridge: ti-sn65dsi86: Support DisplayPort (non-eDP) mode
drm/bridge: ti-sn65dsi86: Implement bridge connector operations
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 180 +++++++++++++++++++++++---
1 file changed, 165 insertions(+), 15 deletions(-)
--
2.32.0
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v3 1/3] drm/bridge: ti-sn65dsi86: Support DisplayPort (non-eDP) mode
2022-03-10 15:22 ` Kieran Bingham
@ 2022-03-10 15:22 ` Kieran Bingham
-1 siblings, 0 replies; 33+ messages in thread
From: Kieran Bingham @ 2022-03-10 15:22 UTC (permalink / raw)
To: dri-devel, linux-renesas-soc, Sam Ravnborg, Douglas Anderson,
linux-kernel
Cc: Laurent Pinchart, Jonas Karlman, David Airlie, Robert Foss,
Neil Armstrong, Jernej Skrabec, Kieran Bingham, Laurent Pinchart,
Andrzej Hajda
From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Despite the SN65DSI86 being an eDP bridge, on some systems its output is
routed to a DisplayPort connector. Enable DisplayPort mode when the next
component in the display pipeline is detected as a DisplayPort
connector, and disable eDP features in that case.
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Reworked to set bridge type based on the next bridge/connector.
Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
--
Changes since v1/RFC:
- Rebased on top of "drm/bridge: ti-sn65dsi86: switch to
devm_drm_of_get_bridge"
- eDP/DP mode determined from the next bridge connector type.
Changes since v2:
- Remove setting of Standard DP Scrambler Seed. (It's read-only).
- Prevent setting DP_EDP_CONFIGURATION_SET in
ti_sn_bridge_atomic_enable()
- Use Doug's suggested text for disabling ASSR on DP mode.
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 23 ++++++++++++++++++++---
1 file changed, 20 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index c892ecba91c7..93b54fcba8ba 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -62,6 +62,7 @@
#define SN_LN_ASSIGN_REG 0x59
#define LN_ASSIGN_WIDTH 2
#define SN_ENH_FRAME_REG 0x5A
+#define ASSR_CONTROL BIT(0)
#define VSTREAM_ENABLE BIT(3)
#define LN_POLRS_OFFSET 4
#define LN_POLRS_MASK 0xf0
@@ -93,6 +94,8 @@
#define SN_DATARATE_CONFIG_REG 0x94
#define DP_DATARATE_MASK GENMASK(7, 5)
#define DP_DATARATE(x) ((x) << 5)
+#define SN_TRAINING_SETTING_REG 0x95
+#define SCRAMBLE_DISABLE BIT(4)
#define SN_ML_TX_MODE_REG 0x96
#define ML_TX_MAIN_LINK_OFF 0
#define ML_TX_NORMAL_MODE BIT(0)
@@ -982,6 +985,17 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx,
goto exit;
}
+ /*
+ * eDP panels use an Alternate Scrambler Seed compared to displays
+ * hooked up via a full DisplayPort connector. SN65DSI86 only supports
+ * the alternate scrambler seed, not the normal one, so the only way we
+ * can support full DisplayPort displays is by fully turning off the
+ * scrambler.
+ */
+ if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
+ regmap_update_bits(pdata->regmap, SN_TRAINING_SETTING_REG,
+ SCRAMBLE_DISABLE, SCRAMBLE_DISABLE);
+
/*
* We'll try to link train several times. As part of link training
* the bridge chip will write DP_SET_POWER_D0 to DP_SET_POWER. If
@@ -1046,12 +1060,13 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
/*
* The SN65DSI86 only supports ASSR Display Authentication method and
- * this method is enabled by default. An eDP panel must support this
+ * this method is enabled for eDP panels. An eDP panel must support this
* authentication method. We need to enable this method in the eDP panel
* at DisplayPort address 0x0010A prior to link training.
*/
- drm_dp_dpcd_writeb(&pdata->aux, DP_EDP_CONFIGURATION_SET,
- DP_ALTERNATE_SCRAMBLER_RESET_ENABLE);
+ if (pdata->bridge.type == DRM_MODE_CONNECTOR_eDP)
+ drm_dp_dpcd_writeb(&pdata->aux, DP_EDP_CONFIGURATION_SET,
+ DP_ALTERNATE_SCRAMBLER_RESET_ENABLE);
/* Set the DP output format (18 bpp or 24 bpp) */
val = (ti_sn_bridge_get_bpp(old_bridge_state) == 18) ? BPP_18_RGB : 0;
@@ -1215,6 +1230,8 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
pdata->bridge.funcs = &ti_sn_bridge_funcs;
pdata->bridge.of_node = np;
+ pdata->bridge.type = pdata->next_bridge->type == DRM_MODE_CONNECTOR_DisplayPort
+ ? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;
drm_bridge_add(&pdata->bridge);
--
2.32.0
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v3 1/3] drm/bridge: ti-sn65dsi86: Support DisplayPort (non-eDP) mode
@ 2022-03-10 15:22 ` Kieran Bingham
0 siblings, 0 replies; 33+ messages in thread
From: Kieran Bingham @ 2022-03-10 15:22 UTC (permalink / raw)
To: dri-devel, linux-renesas-soc, Sam Ravnborg, Douglas Anderson,
linux-kernel
Cc: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter,
Laurent Pinchart, Kieran Bingham
From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Despite the SN65DSI86 being an eDP bridge, on some systems its output is
routed to a DisplayPort connector. Enable DisplayPort mode when the next
component in the display pipeline is detected as a DisplayPort
connector, and disable eDP features in that case.
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Reworked to set bridge type based on the next bridge/connector.
Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
--
Changes since v1/RFC:
- Rebased on top of "drm/bridge: ti-sn65dsi86: switch to
devm_drm_of_get_bridge"
- eDP/DP mode determined from the next bridge connector type.
Changes since v2:
- Remove setting of Standard DP Scrambler Seed. (It's read-only).
- Prevent setting DP_EDP_CONFIGURATION_SET in
ti_sn_bridge_atomic_enable()
- Use Doug's suggested text for disabling ASSR on DP mode.
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 23 ++++++++++++++++++++---
1 file changed, 20 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index c892ecba91c7..93b54fcba8ba 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -62,6 +62,7 @@
#define SN_LN_ASSIGN_REG 0x59
#define LN_ASSIGN_WIDTH 2
#define SN_ENH_FRAME_REG 0x5A
+#define ASSR_CONTROL BIT(0)
#define VSTREAM_ENABLE BIT(3)
#define LN_POLRS_OFFSET 4
#define LN_POLRS_MASK 0xf0
@@ -93,6 +94,8 @@
#define SN_DATARATE_CONFIG_REG 0x94
#define DP_DATARATE_MASK GENMASK(7, 5)
#define DP_DATARATE(x) ((x) << 5)
+#define SN_TRAINING_SETTING_REG 0x95
+#define SCRAMBLE_DISABLE BIT(4)
#define SN_ML_TX_MODE_REG 0x96
#define ML_TX_MAIN_LINK_OFF 0
#define ML_TX_NORMAL_MODE BIT(0)
@@ -982,6 +985,17 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx,
goto exit;
}
+ /*
+ * eDP panels use an Alternate Scrambler Seed compared to displays
+ * hooked up via a full DisplayPort connector. SN65DSI86 only supports
+ * the alternate scrambler seed, not the normal one, so the only way we
+ * can support full DisplayPort displays is by fully turning off the
+ * scrambler.
+ */
+ if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
+ regmap_update_bits(pdata->regmap, SN_TRAINING_SETTING_REG,
+ SCRAMBLE_DISABLE, SCRAMBLE_DISABLE);
+
/*
* We'll try to link train several times. As part of link training
* the bridge chip will write DP_SET_POWER_D0 to DP_SET_POWER. If
@@ -1046,12 +1060,13 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
/*
* The SN65DSI86 only supports ASSR Display Authentication method and
- * this method is enabled by default. An eDP panel must support this
+ * this method is enabled for eDP panels. An eDP panel must support this
* authentication method. We need to enable this method in the eDP panel
* at DisplayPort address 0x0010A prior to link training.
*/
- drm_dp_dpcd_writeb(&pdata->aux, DP_EDP_CONFIGURATION_SET,
- DP_ALTERNATE_SCRAMBLER_RESET_ENABLE);
+ if (pdata->bridge.type == DRM_MODE_CONNECTOR_eDP)
+ drm_dp_dpcd_writeb(&pdata->aux, DP_EDP_CONFIGURATION_SET,
+ DP_ALTERNATE_SCRAMBLER_RESET_ENABLE);
/* Set the DP output format (18 bpp or 24 bpp) */
val = (ti_sn_bridge_get_bpp(old_bridge_state) == 18) ? BPP_18_RGB : 0;
@@ -1215,6 +1230,8 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
pdata->bridge.funcs = &ti_sn_bridge_funcs;
pdata->bridge.of_node = np;
+ pdata->bridge.type = pdata->next_bridge->type == DRM_MODE_CONNECTOR_DisplayPort
+ ? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;
drm_bridge_add(&pdata->bridge);
--
2.32.0
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v3 2/3] drm/bridge: ti-sn65dsi86: Implement bridge connector operations
2022-03-10 15:22 ` Kieran Bingham
@ 2022-03-10 15:22 ` Kieran Bingham
-1 siblings, 0 replies; 33+ messages in thread
From: Kieran Bingham @ 2022-03-10 15:22 UTC (permalink / raw)
To: dri-devel, linux-renesas-soc, Sam Ravnborg, Douglas Anderson,
linux-kernel
Cc: Laurent Pinchart, Jonas Karlman, David Airlie, Robert Foss,
Neil Armstrong, Jernej Skrabec, Kieran Bingham, Laurent Pinchart,
Andrzej Hajda
From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Implement the bridge connector-related .get_edid() operation, and report
the related bridge capabilities and type.
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
Changes since v1:
- The connector .get_modes() operation doesn't rely on EDID anymore,
__ti_sn_bridge_get_edid() and ti_sn_bridge_get_edid() got merged
together
- Fix on top of Sam Ravnborg's DRM_BRIDGE_STATE_OPS
Changes since v2: [Kieran]
- Only support EDID on DRM_MODE_CONNECTOR_DisplayPort modes.
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 93b54fcba8ba..d581c820e5d8 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -1135,10 +1135,24 @@ static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
pm_runtime_put_sync(pdata->dev);
}
+static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge,
+ struct drm_connector *connector)
+{
+ struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
+ struct edid *edid;
+
+ pm_runtime_get_sync(pdata->dev);
+ edid = drm_get_edid(connector, &pdata->aux.ddc);
+ pm_runtime_put_autosuspend(pdata->dev);
+
+ return edid;
+}
+
static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
.attach = ti_sn_bridge_attach,
.detach = ti_sn_bridge_detach,
.mode_valid = ti_sn_bridge_mode_valid,
+ .get_edid = ti_sn_bridge_get_edid,
.atomic_pre_enable = ti_sn_bridge_atomic_pre_enable,
.atomic_enable = ti_sn_bridge_atomic_enable,
.atomic_disable = ti_sn_bridge_atomic_disable,
@@ -1233,6 +1247,9 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
pdata->bridge.type = pdata->next_bridge->type == DRM_MODE_CONNECTOR_DisplayPort
? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;
+ if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
+ pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
+
drm_bridge_add(&pdata->bridge);
ret = ti_sn_attach_host(pdata);
--
2.32.0
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v3 2/3] drm/bridge: ti-sn65dsi86: Implement bridge connector operations
@ 2022-03-10 15:22 ` Kieran Bingham
0 siblings, 0 replies; 33+ messages in thread
From: Kieran Bingham @ 2022-03-10 15:22 UTC (permalink / raw)
To: dri-devel, linux-renesas-soc, Sam Ravnborg, Douglas Anderson,
linux-kernel
Cc: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter,
Laurent Pinchart, Kieran Bingham
From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Implement the bridge connector-related .get_edid() operation, and report
the related bridge capabilities and type.
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
Changes since v1:
- The connector .get_modes() operation doesn't rely on EDID anymore,
__ti_sn_bridge_get_edid() and ti_sn_bridge_get_edid() got merged
together
- Fix on top of Sam Ravnborg's DRM_BRIDGE_STATE_OPS
Changes since v2: [Kieran]
- Only support EDID on DRM_MODE_CONNECTOR_DisplayPort modes.
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 93b54fcba8ba..d581c820e5d8 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -1135,10 +1135,24 @@ static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
pm_runtime_put_sync(pdata->dev);
}
+static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge,
+ struct drm_connector *connector)
+{
+ struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
+ struct edid *edid;
+
+ pm_runtime_get_sync(pdata->dev);
+ edid = drm_get_edid(connector, &pdata->aux.ddc);
+ pm_runtime_put_autosuspend(pdata->dev);
+
+ return edid;
+}
+
static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
.attach = ti_sn_bridge_attach,
.detach = ti_sn_bridge_detach,
.mode_valid = ti_sn_bridge_mode_valid,
+ .get_edid = ti_sn_bridge_get_edid,
.atomic_pre_enable = ti_sn_bridge_atomic_pre_enable,
.atomic_enable = ti_sn_bridge_atomic_enable,
.atomic_disable = ti_sn_bridge_atomic_disable,
@@ -1233,6 +1247,9 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
pdata->bridge.type = pdata->next_bridge->type == DRM_MODE_CONNECTOR_DisplayPort
? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;
+ if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
+ pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
+
drm_bridge_add(&pdata->bridge);
ret = ti_sn_attach_host(pdata);
--
2.32.0
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v3 3/3] drm/bridge: ti-sn65dsi86: Support hotplug detection
2022-03-10 15:22 ` Kieran Bingham
@ 2022-03-10 15:22 ` Kieran Bingham
-1 siblings, 0 replies; 33+ messages in thread
From: Kieran Bingham @ 2022-03-10 15:22 UTC (permalink / raw)
To: dri-devel, linux-renesas-soc, Sam Ravnborg, Douglas Anderson,
linux-kernel
Cc: Laurent Pinchart, Jonas Karlman, David Airlie, Robert Foss,
Neil Armstrong, Jernej Skrabec, Kieran Bingham, Laurent Pinchart,
Andrzej Hajda
When the SN65DSI86 is used in DisplayPort mode, its output is likely
routed to a DisplayPort connector, which can benefit from hotplug
detection. Support it in such cases, with polling mode only for now.
The implementation is limited to the bridge operations, as the connector
operations are legacy and new users should use
DRM_BRIDGE_ATTACH_NO_CONNECTOR.
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
Changes since v1:
- Document the no_hpd field
- Rely on the SN_HPD_DISABLE_REG default value in the HPD case
- Add a TODO comment regarding IRQ support
[Kieran]
- Fix spelling s/assrted/asserted/
- Only enable HPD on DisplayPort connector.
- Support IRQ based hotplug detect
Changes since v2: [Kieran]
- Use unsigned int for values read by regmap
- Update HPD support warning message
- Only enable OP_HPD if IRQ support enabled.
- Only register IRQ handler during ti_sn_bridge_probe()
- Store IRQ in the struct ti_sn65dsi86
- Register IRQ only when !no-hpd
- Refactor DRM_BRIDGE_OP_DETECT and DRM_BRIDGE_OP_HPD handling
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 142 +++++++++++++++++++++++---
1 file changed, 129 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index d581c820e5d8..328a48f09f97 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -70,6 +70,7 @@
#define BPP_18_RGB BIT(0)
#define SN_HPD_DISABLE_REG 0x5C
#define HPD_DISABLE BIT(0)
+#define HPD_DEBOUNCED_STATE BIT(4)
#define SN_GPIO_IO_REG 0x5E
#define SN_GPIO_INPUT_SHIFT 4
#define SN_GPIO_OUTPUT_SHIFT 0
@@ -106,10 +107,24 @@
#define SN_PWM_EN_INV_REG 0xA5
#define SN_PWM_INV_MASK BIT(0)
#define SN_PWM_EN_MASK BIT(1)
+#define SN_IRQ_EN_REG 0xE0
+#define IRQ_EN BIT(0)
+#define SN_IRQ_HPD_REG 0xE6
+#define IRQ_HPD_EN BIT(0)
+#define IRQ_HPD_INSERTION_EN BIT(1)
+#define IRQ_HPD_REMOVAL_EN BIT(2)
+#define IRQ_HPD_REPLUG_EN BIT(3)
+#define IRQ_HPD_PLL_UNLOCK_EN BIT(5)
#define SN_AUX_CMD_STATUS_REG 0xF4
#define AUX_IRQ_STATUS_AUX_RPLY_TOUT BIT(3)
#define AUX_IRQ_STATUS_AUX_SHORT BIT(5)
#define AUX_IRQ_STATUS_NAT_I2C_FAIL BIT(6)
+#define SN_IRQ_HPD_STATUS_REG 0xF5
+#define IRQ_HPD_STATUS BIT(0)
+#define IRQ_HPD_INSERTION_STATUS BIT(1)
+#define IRQ_HPD_REMOVAL_STATUS BIT(2)
+#define IRQ_HPD_REPLUG_STATUS BIT(3)
+#define IRQ_PLL_UNLOCK BIT(5)
#define MIN_DSI_CLK_FREQ_MHZ 40
@@ -168,6 +183,12 @@
* @pwm_enabled: Used to track if the PWM signal is currently enabled.
* @pwm_pin_busy: Track if GPIO4 is currently requested for GPIO or PWM.
* @pwm_refclk_freq: Cache for the reference clock input to the PWM.
+ *
+ * @no_hpd: Disable hot-plug detection as instructed by device tree (used
+ * for instance for eDP panels whose HPD signal won't be asserted
+ * until the panel is turned on, and is thus not usable for
+ * downstream device detection).
+ * @irq: IRQ number for the device.
*/
struct ti_sn65dsi86 {
struct auxiliary_device bridge_aux;
@@ -202,6 +223,9 @@ struct ti_sn65dsi86 {
atomic_t pwm_pin_busy;
#endif
unsigned int pwm_refclk_freq;
+
+ bool no_hpd;
+ int irq;
};
static const struct regmap_range ti_sn65dsi86_volatile_ranges[] = {
@@ -316,23 +340,25 @@ static void ti_sn65dsi86_enable_comms(struct ti_sn65dsi86 *pdata)
ti_sn_bridge_set_refclk_freq(pdata);
/*
- * HPD on this bridge chip is a bit useless. This is an eDP bridge
- * so the HPD is an internal signal that's only there to signal that
- * the panel is done powering up. ...but the bridge chip debounces
- * this signal by between 100 ms and 400 ms (depending on process,
- * voltage, and temperate--I measured it at about 200 ms). One
+ * As this is an eDP bridge, the output will be connected to a fixed
+ * panel in most systems. HPD is in that case only an internal signal
+ * to signal that the panel is done powering up. The bridge chip
+ * debounces this signal by between 100 ms and 400 ms (depending on
+ * process, voltage, and temperate--I measured it at about 200 ms). One
* particular panel asserted HPD 84 ms after it was powered on meaning
* that we saw HPD 284 ms after power on. ...but the same panel said
* that instead of looking at HPD you could just hardcode a delay of
- * 200 ms. We'll assume that the panel driver will have the hardcoded
- * delay in its prepare and always disable HPD.
+ * 200 ms. HPD is thus a bit useless. For this type of use cases, we'll
+ * assume that the panel driver will have the hardcoded delay in its
+ * prepare and always disable HPD.
*
- * If HPD somehow makes sense on some future panel we'll have to
- * change this to be conditional on someone specifying that HPD should
- * be used.
+ * However, on some systems, the output is connected to a DisplayPort
+ * connector. HPD is needed in such cases. To accommodate both use
+ * cases, enable HPD only when requested.
*/
- regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
- HPD_DISABLE);
+ if (pdata->no_hpd)
+ regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG,
+ HPD_DISABLE, HPD_DISABLE);
pdata->comms_enabled = true;
@@ -1135,6 +1161,36 @@ static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
pm_runtime_put_sync(pdata->dev);
}
+static enum drm_connector_status ti_sn_bridge_detect(struct drm_bridge *bridge)
+{
+ struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
+ int val;
+
+ regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val);
+
+ return val & HPD_DEBOUNCED_STATE ? connector_status_connected
+ : connector_status_disconnected;
+}
+
+static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
+{
+ struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
+
+ /* The device must remain active for HPD to function */
+ pm_runtime_get_sync(pdata->dev);
+ regmap_write(pdata->regmap, SN_IRQ_HPD_REG,
+ IRQ_HPD_EN | IRQ_HPD_INSERTION_EN |
+ IRQ_HPD_REMOVAL_EN | IRQ_HPD_REPLUG_EN);
+}
+
+static void ti_sn_bridge_hpd_disable(struct drm_bridge *bridge)
+{
+ struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
+
+ regmap_write(pdata->regmap, SN_IRQ_HPD_REG, 0);
+ pm_runtime_put_autosuspend(pdata->dev);
+}
+
static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge,
struct drm_connector *connector)
{
@@ -1153,6 +1209,9 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
.detach = ti_sn_bridge_detach,
.mode_valid = ti_sn_bridge_mode_valid,
.get_edid = ti_sn_bridge_get_edid,
+ .detect = ti_sn_bridge_detect,
+ .hpd_enable = ti_sn_bridge_hpd_enable,
+ .hpd_disable = ti_sn_bridge_hpd_disable,
.atomic_pre_enable = ti_sn_bridge_atomic_pre_enable,
.atomic_enable = ti_sn_bridge_atomic_enable,
.atomic_disable = ti_sn_bridge_atomic_disable,
@@ -1223,6 +1282,34 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
return 0;
}
+static irqreturn_t ti_sn65dsi86_irq_handler(int irq, void *arg)
+{
+ struct ti_sn65dsi86 *pdata = arg;
+ int ret;
+ unsigned int hpd;
+
+ ret = regmap_read(pdata->regmap, SN_IRQ_HPD_STATUS_REG, &hpd);
+ if (ret || !hpd)
+ return IRQ_NONE;
+
+ if (hpd & IRQ_HPD_INSERTION_STATUS)
+ drm_bridge_hpd_notify(&pdata->bridge, connector_status_connected);
+
+ if (hpd & IRQ_HPD_REMOVAL_STATUS)
+ drm_bridge_hpd_notify(&pdata->bridge, connector_status_disconnected);
+
+ /* When replugged, ensure we trigger a detect to update the display */
+ if (hpd & IRQ_HPD_REPLUG_STATUS)
+ drm_bridge_hpd_notify(&pdata->bridge, connector_status_disconnected);
+
+ /* reset the status registers */
+ regmap_write(pdata->regmap, SN_IRQ_HPD_STATUS_REG,
+ IRQ_HPD_STATUS | IRQ_HPD_INSERTION_STATUS |
+ IRQ_HPD_REMOVAL_STATUS | IRQ_HPD_REPLUG_STATUS);
+
+ return IRQ_HANDLED;
+}
+
static int ti_sn_bridge_probe(struct auxiliary_device *adev,
const struct auxiliary_device_id *id)
{
@@ -1236,6 +1323,14 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
return PTR_ERR(pdata->next_bridge);
}
+ pdata->no_hpd = of_property_read_bool(np, "no-hpd");
+ if (pdata->next_bridge->type != DRM_MODE_CONNECTOR_DisplayPort &&
+ !pdata->no_hpd) {
+ dev_warn(pdata->dev,
+ "HPD support only implemented for full DP connectors\n");
+ pdata->no_hpd = true;
+ }
+
ti_sn_bridge_parse_lanes(pdata, np);
ret = ti_sn_bridge_parse_dsi_host(pdata);
@@ -1247,9 +1342,29 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
pdata->bridge.type = pdata->next_bridge->type == DRM_MODE_CONNECTOR_DisplayPort
? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;
- if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
+ if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort) {
pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
+ if (!pdata->no_hpd)
+ pdata->bridge.ops |= DRM_BRIDGE_OP_DETECT;
+ }
+
+ if (!pdata->no_hpd && pdata->irq > 0) {
+ dev_err(pdata->dev, "registering IRQ %d\n", pdata->irq);
+
+ ret = devm_request_threaded_irq(pdata->dev, pdata->irq, NULL,
+ ti_sn65dsi86_irq_handler,
+ IRQF_ONESHOT, "sn65dsi86-irq",
+ pdata);
+ if (ret)
+ return dev_err_probe(pdata->dev, ret,
+ "Failed to register DP interrupt\n");
+
+ /* Enable IRQ based HPD */
+ regmap_write(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN);
+ pdata->bridge.ops |= DRM_BRIDGE_OP_HPD;
+ }
+
drm_bridge_add(&pdata->bridge);
ret = ti_sn_attach_host(pdata);
@@ -1831,6 +1946,7 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
return -ENOMEM;
dev_set_drvdata(dev, pdata);
pdata->dev = dev;
+ pdata->irq = client->irq;
mutex_init(&pdata->comms_mutex);
--
2.32.0
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v3 3/3] drm/bridge: ti-sn65dsi86: Support hotplug detection
@ 2022-03-10 15:22 ` Kieran Bingham
0 siblings, 0 replies; 33+ messages in thread
From: Kieran Bingham @ 2022-03-10 15:22 UTC (permalink / raw)
To: dri-devel, linux-renesas-soc, Sam Ravnborg, Douglas Anderson,
linux-kernel
Cc: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter,
Kieran Bingham, Laurent Pinchart
When the SN65DSI86 is used in DisplayPort mode, its output is likely
routed to a DisplayPort connector, which can benefit from hotplug
detection. Support it in such cases, with polling mode only for now.
The implementation is limited to the bridge operations, as the connector
operations are legacy and new users should use
DRM_BRIDGE_ATTACH_NO_CONNECTOR.
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
Changes since v1:
- Document the no_hpd field
- Rely on the SN_HPD_DISABLE_REG default value in the HPD case
- Add a TODO comment regarding IRQ support
[Kieran]
- Fix spelling s/assrted/asserted/
- Only enable HPD on DisplayPort connector.
- Support IRQ based hotplug detect
Changes since v2: [Kieran]
- Use unsigned int for values read by regmap
- Update HPD support warning message
- Only enable OP_HPD if IRQ support enabled.
- Only register IRQ handler during ti_sn_bridge_probe()
- Store IRQ in the struct ti_sn65dsi86
- Register IRQ only when !no-hpd
- Refactor DRM_BRIDGE_OP_DETECT and DRM_BRIDGE_OP_HPD handling
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 142 +++++++++++++++++++++++---
1 file changed, 129 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index d581c820e5d8..328a48f09f97 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -70,6 +70,7 @@
#define BPP_18_RGB BIT(0)
#define SN_HPD_DISABLE_REG 0x5C
#define HPD_DISABLE BIT(0)
+#define HPD_DEBOUNCED_STATE BIT(4)
#define SN_GPIO_IO_REG 0x5E
#define SN_GPIO_INPUT_SHIFT 4
#define SN_GPIO_OUTPUT_SHIFT 0
@@ -106,10 +107,24 @@
#define SN_PWM_EN_INV_REG 0xA5
#define SN_PWM_INV_MASK BIT(0)
#define SN_PWM_EN_MASK BIT(1)
+#define SN_IRQ_EN_REG 0xE0
+#define IRQ_EN BIT(0)
+#define SN_IRQ_HPD_REG 0xE6
+#define IRQ_HPD_EN BIT(0)
+#define IRQ_HPD_INSERTION_EN BIT(1)
+#define IRQ_HPD_REMOVAL_EN BIT(2)
+#define IRQ_HPD_REPLUG_EN BIT(3)
+#define IRQ_HPD_PLL_UNLOCK_EN BIT(5)
#define SN_AUX_CMD_STATUS_REG 0xF4
#define AUX_IRQ_STATUS_AUX_RPLY_TOUT BIT(3)
#define AUX_IRQ_STATUS_AUX_SHORT BIT(5)
#define AUX_IRQ_STATUS_NAT_I2C_FAIL BIT(6)
+#define SN_IRQ_HPD_STATUS_REG 0xF5
+#define IRQ_HPD_STATUS BIT(0)
+#define IRQ_HPD_INSERTION_STATUS BIT(1)
+#define IRQ_HPD_REMOVAL_STATUS BIT(2)
+#define IRQ_HPD_REPLUG_STATUS BIT(3)
+#define IRQ_PLL_UNLOCK BIT(5)
#define MIN_DSI_CLK_FREQ_MHZ 40
@@ -168,6 +183,12 @@
* @pwm_enabled: Used to track if the PWM signal is currently enabled.
* @pwm_pin_busy: Track if GPIO4 is currently requested for GPIO or PWM.
* @pwm_refclk_freq: Cache for the reference clock input to the PWM.
+ *
+ * @no_hpd: Disable hot-plug detection as instructed by device tree (used
+ * for instance for eDP panels whose HPD signal won't be asserted
+ * until the panel is turned on, and is thus not usable for
+ * downstream device detection).
+ * @irq: IRQ number for the device.
*/
struct ti_sn65dsi86 {
struct auxiliary_device bridge_aux;
@@ -202,6 +223,9 @@ struct ti_sn65dsi86 {
atomic_t pwm_pin_busy;
#endif
unsigned int pwm_refclk_freq;
+
+ bool no_hpd;
+ int irq;
};
static const struct regmap_range ti_sn65dsi86_volatile_ranges[] = {
@@ -316,23 +340,25 @@ static void ti_sn65dsi86_enable_comms(struct ti_sn65dsi86 *pdata)
ti_sn_bridge_set_refclk_freq(pdata);
/*
- * HPD on this bridge chip is a bit useless. This is an eDP bridge
- * so the HPD is an internal signal that's only there to signal that
- * the panel is done powering up. ...but the bridge chip debounces
- * this signal by between 100 ms and 400 ms (depending on process,
- * voltage, and temperate--I measured it at about 200 ms). One
+ * As this is an eDP bridge, the output will be connected to a fixed
+ * panel in most systems. HPD is in that case only an internal signal
+ * to signal that the panel is done powering up. The bridge chip
+ * debounces this signal by between 100 ms and 400 ms (depending on
+ * process, voltage, and temperate--I measured it at about 200 ms). One
* particular panel asserted HPD 84 ms after it was powered on meaning
* that we saw HPD 284 ms after power on. ...but the same panel said
* that instead of looking at HPD you could just hardcode a delay of
- * 200 ms. We'll assume that the panel driver will have the hardcoded
- * delay in its prepare and always disable HPD.
+ * 200 ms. HPD is thus a bit useless. For this type of use cases, we'll
+ * assume that the panel driver will have the hardcoded delay in its
+ * prepare and always disable HPD.
*
- * If HPD somehow makes sense on some future panel we'll have to
- * change this to be conditional on someone specifying that HPD should
- * be used.
+ * However, on some systems, the output is connected to a DisplayPort
+ * connector. HPD is needed in such cases. To accommodate both use
+ * cases, enable HPD only when requested.
*/
- regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
- HPD_DISABLE);
+ if (pdata->no_hpd)
+ regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG,
+ HPD_DISABLE, HPD_DISABLE);
pdata->comms_enabled = true;
@@ -1135,6 +1161,36 @@ static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
pm_runtime_put_sync(pdata->dev);
}
+static enum drm_connector_status ti_sn_bridge_detect(struct drm_bridge *bridge)
+{
+ struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
+ int val;
+
+ regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val);
+
+ return val & HPD_DEBOUNCED_STATE ? connector_status_connected
+ : connector_status_disconnected;
+}
+
+static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
+{
+ struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
+
+ /* The device must remain active for HPD to function */
+ pm_runtime_get_sync(pdata->dev);
+ regmap_write(pdata->regmap, SN_IRQ_HPD_REG,
+ IRQ_HPD_EN | IRQ_HPD_INSERTION_EN |
+ IRQ_HPD_REMOVAL_EN | IRQ_HPD_REPLUG_EN);
+}
+
+static void ti_sn_bridge_hpd_disable(struct drm_bridge *bridge)
+{
+ struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
+
+ regmap_write(pdata->regmap, SN_IRQ_HPD_REG, 0);
+ pm_runtime_put_autosuspend(pdata->dev);
+}
+
static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge,
struct drm_connector *connector)
{
@@ -1153,6 +1209,9 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
.detach = ti_sn_bridge_detach,
.mode_valid = ti_sn_bridge_mode_valid,
.get_edid = ti_sn_bridge_get_edid,
+ .detect = ti_sn_bridge_detect,
+ .hpd_enable = ti_sn_bridge_hpd_enable,
+ .hpd_disable = ti_sn_bridge_hpd_disable,
.atomic_pre_enable = ti_sn_bridge_atomic_pre_enable,
.atomic_enable = ti_sn_bridge_atomic_enable,
.atomic_disable = ti_sn_bridge_atomic_disable,
@@ -1223,6 +1282,34 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
return 0;
}
+static irqreturn_t ti_sn65dsi86_irq_handler(int irq, void *arg)
+{
+ struct ti_sn65dsi86 *pdata = arg;
+ int ret;
+ unsigned int hpd;
+
+ ret = regmap_read(pdata->regmap, SN_IRQ_HPD_STATUS_REG, &hpd);
+ if (ret || !hpd)
+ return IRQ_NONE;
+
+ if (hpd & IRQ_HPD_INSERTION_STATUS)
+ drm_bridge_hpd_notify(&pdata->bridge, connector_status_connected);
+
+ if (hpd & IRQ_HPD_REMOVAL_STATUS)
+ drm_bridge_hpd_notify(&pdata->bridge, connector_status_disconnected);
+
+ /* When replugged, ensure we trigger a detect to update the display */
+ if (hpd & IRQ_HPD_REPLUG_STATUS)
+ drm_bridge_hpd_notify(&pdata->bridge, connector_status_disconnected);
+
+ /* reset the status registers */
+ regmap_write(pdata->regmap, SN_IRQ_HPD_STATUS_REG,
+ IRQ_HPD_STATUS | IRQ_HPD_INSERTION_STATUS |
+ IRQ_HPD_REMOVAL_STATUS | IRQ_HPD_REPLUG_STATUS);
+
+ return IRQ_HANDLED;
+}
+
static int ti_sn_bridge_probe(struct auxiliary_device *adev,
const struct auxiliary_device_id *id)
{
@@ -1236,6 +1323,14 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
return PTR_ERR(pdata->next_bridge);
}
+ pdata->no_hpd = of_property_read_bool(np, "no-hpd");
+ if (pdata->next_bridge->type != DRM_MODE_CONNECTOR_DisplayPort &&
+ !pdata->no_hpd) {
+ dev_warn(pdata->dev,
+ "HPD support only implemented for full DP connectors\n");
+ pdata->no_hpd = true;
+ }
+
ti_sn_bridge_parse_lanes(pdata, np);
ret = ti_sn_bridge_parse_dsi_host(pdata);
@@ -1247,9 +1342,29 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
pdata->bridge.type = pdata->next_bridge->type == DRM_MODE_CONNECTOR_DisplayPort
? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;
- if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
+ if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort) {
pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
+ if (!pdata->no_hpd)
+ pdata->bridge.ops |= DRM_BRIDGE_OP_DETECT;
+ }
+
+ if (!pdata->no_hpd && pdata->irq > 0) {
+ dev_err(pdata->dev, "registering IRQ %d\n", pdata->irq);
+
+ ret = devm_request_threaded_irq(pdata->dev, pdata->irq, NULL,
+ ti_sn65dsi86_irq_handler,
+ IRQF_ONESHOT, "sn65dsi86-irq",
+ pdata);
+ if (ret)
+ return dev_err_probe(pdata->dev, ret,
+ "Failed to register DP interrupt\n");
+
+ /* Enable IRQ based HPD */
+ regmap_write(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN);
+ pdata->bridge.ops |= DRM_BRIDGE_OP_HPD;
+ }
+
drm_bridge_add(&pdata->bridge);
ret = ti_sn_attach_host(pdata);
@@ -1831,6 +1946,7 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
return -ENOMEM;
dev_set_drvdata(dev, pdata);
pdata->dev = dev;
+ pdata->irq = client->irq;
mutex_init(&pdata->comms_mutex);
--
2.32.0
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH v3 3/3] drm/bridge: ti-sn65dsi86: Support hotplug detection
2022-03-10 15:22 ` Kieran Bingham
@ 2022-03-10 15:26 ` Kieran Bingham
-1 siblings, 0 replies; 33+ messages in thread
From: Kieran Bingham @ 2022-03-10 15:26 UTC (permalink / raw)
To: Douglas Anderson, Sam Ravnborg, dri-devel, linux-kernel,
linux-renesas-soc
Cc: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter,
Laurent Pinchart
Quoting Kieran Bingham (2022-03-10 15:22:27)
> When the SN65DSI86 is used in DisplayPort mode, its output is likely
> routed to a DisplayPort connector, which can benefit from hotplug
> detection. Support it in such cases, with polling mode only for now.
>
> The implementation is limited to the bridge operations, as the connector
> operations are legacy and new users should use
> DRM_BRIDGE_ATTACH_NO_CONNECTOR.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
> Changes since v1:
>
> - Document the no_hpd field
> - Rely on the SN_HPD_DISABLE_REG default value in the HPD case
> - Add a TODO comment regarding IRQ support
> [Kieran]
> - Fix spelling s/assrted/asserted/
> - Only enable HPD on DisplayPort connector.
> - Support IRQ based hotplug detect
>
> Changes since v2: [Kieran]
> - Use unsigned int for values read by regmap
> - Update HPD support warning message
> - Only enable OP_HPD if IRQ support enabled.
> - Only register IRQ handler during ti_sn_bridge_probe()
> - Store IRQ in the struct ti_sn65dsi86
> - Register IRQ only when !no-hpd
> - Refactor DRM_BRIDGE_OP_DETECT and DRM_BRIDGE_OP_HPD handling
>
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 142 +++++++++++++++++++++++---
> 1 file changed, 129 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index d581c820e5d8..328a48f09f97 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -70,6 +70,7 @@
> #define BPP_18_RGB BIT(0)
> #define SN_HPD_DISABLE_REG 0x5C
> #define HPD_DISABLE BIT(0)
> +#define HPD_DEBOUNCED_STATE BIT(4)
> #define SN_GPIO_IO_REG 0x5E
> #define SN_GPIO_INPUT_SHIFT 4
> #define SN_GPIO_OUTPUT_SHIFT 0
> @@ -106,10 +107,24 @@
> #define SN_PWM_EN_INV_REG 0xA5
> #define SN_PWM_INV_MASK BIT(0)
> #define SN_PWM_EN_MASK BIT(1)
> +#define SN_IRQ_EN_REG 0xE0
> +#define IRQ_EN BIT(0)
> +#define SN_IRQ_HPD_REG 0xE6
> +#define IRQ_HPD_EN BIT(0)
> +#define IRQ_HPD_INSERTION_EN BIT(1)
> +#define IRQ_HPD_REMOVAL_EN BIT(2)
> +#define IRQ_HPD_REPLUG_EN BIT(3)
> +#define IRQ_HPD_PLL_UNLOCK_EN BIT(5)
> #define SN_AUX_CMD_STATUS_REG 0xF4
> #define AUX_IRQ_STATUS_AUX_RPLY_TOUT BIT(3)
> #define AUX_IRQ_STATUS_AUX_SHORT BIT(5)
> #define AUX_IRQ_STATUS_NAT_I2C_FAIL BIT(6)
> +#define SN_IRQ_HPD_STATUS_REG 0xF5
> +#define IRQ_HPD_STATUS BIT(0)
> +#define IRQ_HPD_INSERTION_STATUS BIT(1)
> +#define IRQ_HPD_REMOVAL_STATUS BIT(2)
> +#define IRQ_HPD_REPLUG_STATUS BIT(3)
> +#define IRQ_PLL_UNLOCK BIT(5)
>
> #define MIN_DSI_CLK_FREQ_MHZ 40
>
> @@ -168,6 +183,12 @@
> * @pwm_enabled: Used to track if the PWM signal is currently enabled.
> * @pwm_pin_busy: Track if GPIO4 is currently requested for GPIO or PWM.
> * @pwm_refclk_freq: Cache for the reference clock input to the PWM.
> + *
> + * @no_hpd: Disable hot-plug detection as instructed by device tree (used
> + * for instance for eDP panels whose HPD signal won't be asserted
> + * until the panel is turned on, and is thus not usable for
> + * downstream device detection).
> + * @irq: IRQ number for the device.
> */
> struct ti_sn65dsi86 {
> struct auxiliary_device bridge_aux;
> @@ -202,6 +223,9 @@ struct ti_sn65dsi86 {
> atomic_t pwm_pin_busy;
> #endif
> unsigned int pwm_refclk_freq;
> +
> + bool no_hpd;
> + int irq;
> };
>
> static const struct regmap_range ti_sn65dsi86_volatile_ranges[] = {
> @@ -316,23 +340,25 @@ static void ti_sn65dsi86_enable_comms(struct ti_sn65dsi86 *pdata)
> ti_sn_bridge_set_refclk_freq(pdata);
>
> /*
> - * HPD on this bridge chip is a bit useless. This is an eDP bridge
> - * so the HPD is an internal signal that's only there to signal that
> - * the panel is done powering up. ...but the bridge chip debounces
> - * this signal by between 100 ms and 400 ms (depending on process,
> - * voltage, and temperate--I measured it at about 200 ms). One
> + * As this is an eDP bridge, the output will be connected to a fixed
> + * panel in most systems. HPD is in that case only an internal signal
> + * to signal that the panel is done powering up. The bridge chip
> + * debounces this signal by between 100 ms and 400 ms (depending on
> + * process, voltage, and temperate--I measured it at about 200 ms). One
> * particular panel asserted HPD 84 ms after it was powered on meaning
> * that we saw HPD 284 ms after power on. ...but the same panel said
> * that instead of looking at HPD you could just hardcode a delay of
> - * 200 ms. We'll assume that the panel driver will have the hardcoded
> - * delay in its prepare and always disable HPD.
> + * 200 ms. HPD is thus a bit useless. For this type of use cases, we'll
> + * assume that the panel driver will have the hardcoded delay in its
> + * prepare and always disable HPD.
> *
> - * If HPD somehow makes sense on some future panel we'll have to
> - * change this to be conditional on someone specifying that HPD should
> - * be used.
> + * However, on some systems, the output is connected to a DisplayPort
> + * connector. HPD is needed in such cases. To accommodate both use
> + * cases, enable HPD only when requested.
> */
> - regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
> - HPD_DISABLE);
> + if (pdata->no_hpd)
> + regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG,
> + HPD_DISABLE, HPD_DISABLE);
>
> pdata->comms_enabled = true;
>
> @@ -1135,6 +1161,36 @@ static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
> pm_runtime_put_sync(pdata->dev);
> }
>
> +static enum drm_connector_status ti_sn_bridge_detect(struct drm_bridge *bridge)
> +{
> + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> + int val;
> +
> + regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val);
> +
> + return val & HPD_DEBOUNCED_STATE ? connector_status_connected
> + : connector_status_disconnected;
> +}
> +
> +static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
> +{
> + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> +
> + /* The device must remain active for HPD to function */
> + pm_runtime_get_sync(pdata->dev);
> + regmap_write(pdata->regmap, SN_IRQ_HPD_REG,
> + IRQ_HPD_EN | IRQ_HPD_INSERTION_EN |
> + IRQ_HPD_REMOVAL_EN | IRQ_HPD_REPLUG_EN);
> +}
> +
> +static void ti_sn_bridge_hpd_disable(struct drm_bridge *bridge)
> +{
> + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> +
> + regmap_write(pdata->regmap, SN_IRQ_HPD_REG, 0);
> + pm_runtime_put_autosuspend(pdata->dev);
> +}
> +
> static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge,
> struct drm_connector *connector)
> {
> @@ -1153,6 +1209,9 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
> .detach = ti_sn_bridge_detach,
> .mode_valid = ti_sn_bridge_mode_valid,
> .get_edid = ti_sn_bridge_get_edid,
> + .detect = ti_sn_bridge_detect,
> + .hpd_enable = ti_sn_bridge_hpd_enable,
> + .hpd_disable = ti_sn_bridge_hpd_disable,
> .atomic_pre_enable = ti_sn_bridge_atomic_pre_enable,
> .atomic_enable = ti_sn_bridge_atomic_enable,
> .atomic_disable = ti_sn_bridge_atomic_disable,
> @@ -1223,6 +1282,34 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
> return 0;
> }
>
> +static irqreturn_t ti_sn65dsi86_irq_handler(int irq, void *arg)
> +{
> + struct ti_sn65dsi86 *pdata = arg;
> + int ret;
> + unsigned int hpd;
> +
> + ret = regmap_read(pdata->regmap, SN_IRQ_HPD_STATUS_REG, &hpd);
> + if (ret || !hpd)
> + return IRQ_NONE;
> +
> + if (hpd & IRQ_HPD_INSERTION_STATUS)
> + drm_bridge_hpd_notify(&pdata->bridge, connector_status_connected);
> +
> + if (hpd & IRQ_HPD_REMOVAL_STATUS)
> + drm_bridge_hpd_notify(&pdata->bridge, connector_status_disconnected);
> +
> + /* When replugged, ensure we trigger a detect to update the display */
> + if (hpd & IRQ_HPD_REPLUG_STATUS)
> + drm_bridge_hpd_notify(&pdata->bridge, connector_status_disconnected);
> +
> + /* reset the status registers */
> + regmap_write(pdata->regmap, SN_IRQ_HPD_STATUS_REG,
> + IRQ_HPD_STATUS | IRQ_HPD_INSERTION_STATUS |
> + IRQ_HPD_REMOVAL_STATUS | IRQ_HPD_REPLUG_STATUS);
> +
> + return IRQ_HANDLED;
> +}
> +
> static int ti_sn_bridge_probe(struct auxiliary_device *adev,
> const struct auxiliary_device_id *id)
> {
> @@ -1236,6 +1323,14 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
> return PTR_ERR(pdata->next_bridge);
> }
>
> + pdata->no_hpd = of_property_read_bool(np, "no-hpd");
> + if (pdata->next_bridge->type != DRM_MODE_CONNECTOR_DisplayPort &&
> + !pdata->no_hpd) {
> + dev_warn(pdata->dev,
> + "HPD support only implemented for full DP connectors\n");
> + pdata->no_hpd = true;
> + }
> +
> ti_sn_bridge_parse_lanes(pdata, np);
>
> ret = ti_sn_bridge_parse_dsi_host(pdata);
> @@ -1247,9 +1342,29 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
> pdata->bridge.type = pdata->next_bridge->type == DRM_MODE_CONNECTOR_DisplayPort
> ? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;
>
> - if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
> + if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort) {
> pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
>
> + if (!pdata->no_hpd)
> + pdata->bridge.ops |= DRM_BRIDGE_OP_DETECT;
> + }
> +
> + if (!pdata->no_hpd && pdata->irq > 0) {
> + dev_err(pdata->dev, "registering IRQ %d\n", pdata->irq);
Appologies, my debug print has slipped through. This should be removed.
--
Kieran
> +
> + ret = devm_request_threaded_irq(pdata->dev, pdata->irq, NULL,
> + ti_sn65dsi86_irq_handler,
> + IRQF_ONESHOT, "sn65dsi86-irq",
> + pdata);
> + if (ret)
> + return dev_err_probe(pdata->dev, ret,
> + "Failed to register DP interrupt\n");
> +
> + /* Enable IRQ based HPD */
> + regmap_write(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN);
> + pdata->bridge.ops |= DRM_BRIDGE_OP_HPD;
> + }
> +
> drm_bridge_add(&pdata->bridge);
>
> ret = ti_sn_attach_host(pdata);
> @@ -1831,6 +1946,7 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
> return -ENOMEM;
> dev_set_drvdata(dev, pdata);
> pdata->dev = dev;
> + pdata->irq = client->irq;
>
> mutex_init(&pdata->comms_mutex);
>
> --
> 2.32.0
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v3 3/3] drm/bridge: ti-sn65dsi86: Support hotplug detection
@ 2022-03-10 15:26 ` Kieran Bingham
0 siblings, 0 replies; 33+ messages in thread
From: Kieran Bingham @ 2022-03-10 15:26 UTC (permalink / raw)
To: Douglas Anderson, Sam Ravnborg, dri-devel, linux-kernel,
linux-renesas-soc
Cc: Laurent Pinchart, Jonas Karlman, David Airlie, Robert Foss,
Neil Armstrong, Jernej Skrabec, Laurent Pinchart, Andrzej Hajda
Quoting Kieran Bingham (2022-03-10 15:22:27)
> When the SN65DSI86 is used in DisplayPort mode, its output is likely
> routed to a DisplayPort connector, which can benefit from hotplug
> detection. Support it in such cases, with polling mode only for now.
>
> The implementation is limited to the bridge operations, as the connector
> operations are legacy and new users should use
> DRM_BRIDGE_ATTACH_NO_CONNECTOR.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
> Changes since v1:
>
> - Document the no_hpd field
> - Rely on the SN_HPD_DISABLE_REG default value in the HPD case
> - Add a TODO comment regarding IRQ support
> [Kieran]
> - Fix spelling s/assrted/asserted/
> - Only enable HPD on DisplayPort connector.
> - Support IRQ based hotplug detect
>
> Changes since v2: [Kieran]
> - Use unsigned int for values read by regmap
> - Update HPD support warning message
> - Only enable OP_HPD if IRQ support enabled.
> - Only register IRQ handler during ti_sn_bridge_probe()
> - Store IRQ in the struct ti_sn65dsi86
> - Register IRQ only when !no-hpd
> - Refactor DRM_BRIDGE_OP_DETECT and DRM_BRIDGE_OP_HPD handling
>
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 142 +++++++++++++++++++++++---
> 1 file changed, 129 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index d581c820e5d8..328a48f09f97 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -70,6 +70,7 @@
> #define BPP_18_RGB BIT(0)
> #define SN_HPD_DISABLE_REG 0x5C
> #define HPD_DISABLE BIT(0)
> +#define HPD_DEBOUNCED_STATE BIT(4)
> #define SN_GPIO_IO_REG 0x5E
> #define SN_GPIO_INPUT_SHIFT 4
> #define SN_GPIO_OUTPUT_SHIFT 0
> @@ -106,10 +107,24 @@
> #define SN_PWM_EN_INV_REG 0xA5
> #define SN_PWM_INV_MASK BIT(0)
> #define SN_PWM_EN_MASK BIT(1)
> +#define SN_IRQ_EN_REG 0xE0
> +#define IRQ_EN BIT(0)
> +#define SN_IRQ_HPD_REG 0xE6
> +#define IRQ_HPD_EN BIT(0)
> +#define IRQ_HPD_INSERTION_EN BIT(1)
> +#define IRQ_HPD_REMOVAL_EN BIT(2)
> +#define IRQ_HPD_REPLUG_EN BIT(3)
> +#define IRQ_HPD_PLL_UNLOCK_EN BIT(5)
> #define SN_AUX_CMD_STATUS_REG 0xF4
> #define AUX_IRQ_STATUS_AUX_RPLY_TOUT BIT(3)
> #define AUX_IRQ_STATUS_AUX_SHORT BIT(5)
> #define AUX_IRQ_STATUS_NAT_I2C_FAIL BIT(6)
> +#define SN_IRQ_HPD_STATUS_REG 0xF5
> +#define IRQ_HPD_STATUS BIT(0)
> +#define IRQ_HPD_INSERTION_STATUS BIT(1)
> +#define IRQ_HPD_REMOVAL_STATUS BIT(2)
> +#define IRQ_HPD_REPLUG_STATUS BIT(3)
> +#define IRQ_PLL_UNLOCK BIT(5)
>
> #define MIN_DSI_CLK_FREQ_MHZ 40
>
> @@ -168,6 +183,12 @@
> * @pwm_enabled: Used to track if the PWM signal is currently enabled.
> * @pwm_pin_busy: Track if GPIO4 is currently requested for GPIO or PWM.
> * @pwm_refclk_freq: Cache for the reference clock input to the PWM.
> + *
> + * @no_hpd: Disable hot-plug detection as instructed by device tree (used
> + * for instance for eDP panels whose HPD signal won't be asserted
> + * until the panel is turned on, and is thus not usable for
> + * downstream device detection).
> + * @irq: IRQ number for the device.
> */
> struct ti_sn65dsi86 {
> struct auxiliary_device bridge_aux;
> @@ -202,6 +223,9 @@ struct ti_sn65dsi86 {
> atomic_t pwm_pin_busy;
> #endif
> unsigned int pwm_refclk_freq;
> +
> + bool no_hpd;
> + int irq;
> };
>
> static const struct regmap_range ti_sn65dsi86_volatile_ranges[] = {
> @@ -316,23 +340,25 @@ static void ti_sn65dsi86_enable_comms(struct ti_sn65dsi86 *pdata)
> ti_sn_bridge_set_refclk_freq(pdata);
>
> /*
> - * HPD on this bridge chip is a bit useless. This is an eDP bridge
> - * so the HPD is an internal signal that's only there to signal that
> - * the panel is done powering up. ...but the bridge chip debounces
> - * this signal by between 100 ms and 400 ms (depending on process,
> - * voltage, and temperate--I measured it at about 200 ms). One
> + * As this is an eDP bridge, the output will be connected to a fixed
> + * panel in most systems. HPD is in that case only an internal signal
> + * to signal that the panel is done powering up. The bridge chip
> + * debounces this signal by between 100 ms and 400 ms (depending on
> + * process, voltage, and temperate--I measured it at about 200 ms). One
> * particular panel asserted HPD 84 ms after it was powered on meaning
> * that we saw HPD 284 ms after power on. ...but the same panel said
> * that instead of looking at HPD you could just hardcode a delay of
> - * 200 ms. We'll assume that the panel driver will have the hardcoded
> - * delay in its prepare and always disable HPD.
> + * 200 ms. HPD is thus a bit useless. For this type of use cases, we'll
> + * assume that the panel driver will have the hardcoded delay in its
> + * prepare and always disable HPD.
> *
> - * If HPD somehow makes sense on some future panel we'll have to
> - * change this to be conditional on someone specifying that HPD should
> - * be used.
> + * However, on some systems, the output is connected to a DisplayPort
> + * connector. HPD is needed in such cases. To accommodate both use
> + * cases, enable HPD only when requested.
> */
> - regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
> - HPD_DISABLE);
> + if (pdata->no_hpd)
> + regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG,
> + HPD_DISABLE, HPD_DISABLE);
>
> pdata->comms_enabled = true;
>
> @@ -1135,6 +1161,36 @@ static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
> pm_runtime_put_sync(pdata->dev);
> }
>
> +static enum drm_connector_status ti_sn_bridge_detect(struct drm_bridge *bridge)
> +{
> + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> + int val;
> +
> + regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val);
> +
> + return val & HPD_DEBOUNCED_STATE ? connector_status_connected
> + : connector_status_disconnected;
> +}
> +
> +static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
> +{
> + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> +
> + /* The device must remain active for HPD to function */
> + pm_runtime_get_sync(pdata->dev);
> + regmap_write(pdata->regmap, SN_IRQ_HPD_REG,
> + IRQ_HPD_EN | IRQ_HPD_INSERTION_EN |
> + IRQ_HPD_REMOVAL_EN | IRQ_HPD_REPLUG_EN);
> +}
> +
> +static void ti_sn_bridge_hpd_disable(struct drm_bridge *bridge)
> +{
> + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> +
> + regmap_write(pdata->regmap, SN_IRQ_HPD_REG, 0);
> + pm_runtime_put_autosuspend(pdata->dev);
> +}
> +
> static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge,
> struct drm_connector *connector)
> {
> @@ -1153,6 +1209,9 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
> .detach = ti_sn_bridge_detach,
> .mode_valid = ti_sn_bridge_mode_valid,
> .get_edid = ti_sn_bridge_get_edid,
> + .detect = ti_sn_bridge_detect,
> + .hpd_enable = ti_sn_bridge_hpd_enable,
> + .hpd_disable = ti_sn_bridge_hpd_disable,
> .atomic_pre_enable = ti_sn_bridge_atomic_pre_enable,
> .atomic_enable = ti_sn_bridge_atomic_enable,
> .atomic_disable = ti_sn_bridge_atomic_disable,
> @@ -1223,6 +1282,34 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
> return 0;
> }
>
> +static irqreturn_t ti_sn65dsi86_irq_handler(int irq, void *arg)
> +{
> + struct ti_sn65dsi86 *pdata = arg;
> + int ret;
> + unsigned int hpd;
> +
> + ret = regmap_read(pdata->regmap, SN_IRQ_HPD_STATUS_REG, &hpd);
> + if (ret || !hpd)
> + return IRQ_NONE;
> +
> + if (hpd & IRQ_HPD_INSERTION_STATUS)
> + drm_bridge_hpd_notify(&pdata->bridge, connector_status_connected);
> +
> + if (hpd & IRQ_HPD_REMOVAL_STATUS)
> + drm_bridge_hpd_notify(&pdata->bridge, connector_status_disconnected);
> +
> + /* When replugged, ensure we trigger a detect to update the display */
> + if (hpd & IRQ_HPD_REPLUG_STATUS)
> + drm_bridge_hpd_notify(&pdata->bridge, connector_status_disconnected);
> +
> + /* reset the status registers */
> + regmap_write(pdata->regmap, SN_IRQ_HPD_STATUS_REG,
> + IRQ_HPD_STATUS | IRQ_HPD_INSERTION_STATUS |
> + IRQ_HPD_REMOVAL_STATUS | IRQ_HPD_REPLUG_STATUS);
> +
> + return IRQ_HANDLED;
> +}
> +
> static int ti_sn_bridge_probe(struct auxiliary_device *adev,
> const struct auxiliary_device_id *id)
> {
> @@ -1236,6 +1323,14 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
> return PTR_ERR(pdata->next_bridge);
> }
>
> + pdata->no_hpd = of_property_read_bool(np, "no-hpd");
> + if (pdata->next_bridge->type != DRM_MODE_CONNECTOR_DisplayPort &&
> + !pdata->no_hpd) {
> + dev_warn(pdata->dev,
> + "HPD support only implemented for full DP connectors\n");
> + pdata->no_hpd = true;
> + }
> +
> ti_sn_bridge_parse_lanes(pdata, np);
>
> ret = ti_sn_bridge_parse_dsi_host(pdata);
> @@ -1247,9 +1342,29 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
> pdata->bridge.type = pdata->next_bridge->type == DRM_MODE_CONNECTOR_DisplayPort
> ? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;
>
> - if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
> + if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort) {
> pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
>
> + if (!pdata->no_hpd)
> + pdata->bridge.ops |= DRM_BRIDGE_OP_DETECT;
> + }
> +
> + if (!pdata->no_hpd && pdata->irq > 0) {
> + dev_err(pdata->dev, "registering IRQ %d\n", pdata->irq);
Appologies, my debug print has slipped through. This should be removed.
--
Kieran
> +
> + ret = devm_request_threaded_irq(pdata->dev, pdata->irq, NULL,
> + ti_sn65dsi86_irq_handler,
> + IRQF_ONESHOT, "sn65dsi86-irq",
> + pdata);
> + if (ret)
> + return dev_err_probe(pdata->dev, ret,
> + "Failed to register DP interrupt\n");
> +
> + /* Enable IRQ based HPD */
> + regmap_write(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN);
> + pdata->bridge.ops |= DRM_BRIDGE_OP_HPD;
> + }
> +
> drm_bridge_add(&pdata->bridge);
>
> ret = ti_sn_attach_host(pdata);
> @@ -1831,6 +1946,7 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
> return -ENOMEM;
> dev_set_drvdata(dev, pdata);
> pdata->dev = dev;
> + pdata->irq = client->irq;
>
> mutex_init(&pdata->comms_mutex);
>
> --
> 2.32.0
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v3 1/3] drm/bridge: ti-sn65dsi86: Support DisplayPort (non-eDP) mode
2022-03-10 15:22 ` Kieran Bingham
@ 2022-03-10 16:18 ` Laurent Pinchart
-1 siblings, 0 replies; 33+ messages in thread
From: Laurent Pinchart @ 2022-03-10 16:18 UTC (permalink / raw)
To: Kieran Bingham
Cc: Jernej Skrabec, Neil Armstrong, David Airlie, Jonas Karlman,
Douglas Anderson, dri-devel, linux-kernel, linux-renesas-soc,
Robert Foss, Andrzej Hajda, Sam Ravnborg
Hi Kieran,
Thank you for the patch.
On Thu, Mar 10, 2022 at 03:22:25PM +0000, Kieran Bingham wrote:
> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>
> Despite the SN65DSI86 being an eDP bridge, on some systems its output is
> routed to a DisplayPort connector. Enable DisplayPort mode when the next
> component in the display pipeline is detected as a DisplayPort
> connector, and disable eDP features in that case.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Reworked to set bridge type based on the next bridge/connector.
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
(I know I'm listed as the author, but your changes look good :-))
> --
> Changes since v1/RFC:
> - Rebased on top of "drm/bridge: ti-sn65dsi86: switch to
> devm_drm_of_get_bridge"
> - eDP/DP mode determined from the next bridge connector type.
>
> Changes since v2:
> - Remove setting of Standard DP Scrambler Seed. (It's read-only).
> - Prevent setting DP_EDP_CONFIGURATION_SET in
> ti_sn_bridge_atomic_enable()
> - Use Doug's suggested text for disabling ASSR on DP mode.
>
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 23 ++++++++++++++++++++---
> 1 file changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index c892ecba91c7..93b54fcba8ba 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -62,6 +62,7 @@
> #define SN_LN_ASSIGN_REG 0x59
> #define LN_ASSIGN_WIDTH 2
> #define SN_ENH_FRAME_REG 0x5A
> +#define ASSR_CONTROL BIT(0)
> #define VSTREAM_ENABLE BIT(3)
> #define LN_POLRS_OFFSET 4
> #define LN_POLRS_MASK 0xf0
> @@ -93,6 +94,8 @@
> #define SN_DATARATE_CONFIG_REG 0x94
> #define DP_DATARATE_MASK GENMASK(7, 5)
> #define DP_DATARATE(x) ((x) << 5)
> +#define SN_TRAINING_SETTING_REG 0x95
> +#define SCRAMBLE_DISABLE BIT(4)
> #define SN_ML_TX_MODE_REG 0x96
> #define ML_TX_MAIN_LINK_OFF 0
> #define ML_TX_NORMAL_MODE BIT(0)
> @@ -982,6 +985,17 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx,
> goto exit;
> }
>
> + /*
> + * eDP panels use an Alternate Scrambler Seed compared to displays
> + * hooked up via a full DisplayPort connector. SN65DSI86 only supports
> + * the alternate scrambler seed, not the normal one, so the only way we
> + * can support full DisplayPort displays is by fully turning off the
> + * scrambler.
> + */
> + if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
> + regmap_update_bits(pdata->regmap, SN_TRAINING_SETTING_REG,
> + SCRAMBLE_DISABLE, SCRAMBLE_DISABLE);
> +
> /*
> * We'll try to link train several times. As part of link training
> * the bridge chip will write DP_SET_POWER_D0 to DP_SET_POWER. If
> @@ -1046,12 +1060,13 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
>
> /*
> * The SN65DSI86 only supports ASSR Display Authentication method and
> - * this method is enabled by default. An eDP panel must support this
> + * this method is enabled for eDP panels. An eDP panel must support this
> * authentication method. We need to enable this method in the eDP panel
> * at DisplayPort address 0x0010A prior to link training.
> */
> - drm_dp_dpcd_writeb(&pdata->aux, DP_EDP_CONFIGURATION_SET,
> - DP_ALTERNATE_SCRAMBLER_RESET_ENABLE);
> + if (pdata->bridge.type == DRM_MODE_CONNECTOR_eDP)
> + drm_dp_dpcd_writeb(&pdata->aux, DP_EDP_CONFIGURATION_SET,
> + DP_ALTERNATE_SCRAMBLER_RESET_ENABLE);
>
> /* Set the DP output format (18 bpp or 24 bpp) */
> val = (ti_sn_bridge_get_bpp(old_bridge_state) == 18) ? BPP_18_RGB : 0;
> @@ -1215,6 +1230,8 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
>
> pdata->bridge.funcs = &ti_sn_bridge_funcs;
> pdata->bridge.of_node = np;
> + pdata->bridge.type = pdata->next_bridge->type == DRM_MODE_CONNECTOR_DisplayPort
> + ? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;
>
> drm_bridge_add(&pdata->bridge);
>
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v3 1/3] drm/bridge: ti-sn65dsi86: Support DisplayPort (non-eDP) mode
@ 2022-03-10 16:18 ` Laurent Pinchart
0 siblings, 0 replies; 33+ messages in thread
From: Laurent Pinchart @ 2022-03-10 16:18 UTC (permalink / raw)
To: Kieran Bingham
Cc: dri-devel, linux-renesas-soc, Sam Ravnborg, Douglas Anderson,
linux-kernel, Andrzej Hajda, Neil Armstrong, Robert Foss,
Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter
Hi Kieran,
Thank you for the patch.
On Thu, Mar 10, 2022 at 03:22:25PM +0000, Kieran Bingham wrote:
> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>
> Despite the SN65DSI86 being an eDP bridge, on some systems its output is
> routed to a DisplayPort connector. Enable DisplayPort mode when the next
> component in the display pipeline is detected as a DisplayPort
> connector, and disable eDP features in that case.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Reworked to set bridge type based on the next bridge/connector.
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
(I know I'm listed as the author, but your changes look good :-))
> --
> Changes since v1/RFC:
> - Rebased on top of "drm/bridge: ti-sn65dsi86: switch to
> devm_drm_of_get_bridge"
> - eDP/DP mode determined from the next bridge connector type.
>
> Changes since v2:
> - Remove setting of Standard DP Scrambler Seed. (It's read-only).
> - Prevent setting DP_EDP_CONFIGURATION_SET in
> ti_sn_bridge_atomic_enable()
> - Use Doug's suggested text for disabling ASSR on DP mode.
>
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 23 ++++++++++++++++++++---
> 1 file changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index c892ecba91c7..93b54fcba8ba 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -62,6 +62,7 @@
> #define SN_LN_ASSIGN_REG 0x59
> #define LN_ASSIGN_WIDTH 2
> #define SN_ENH_FRAME_REG 0x5A
> +#define ASSR_CONTROL BIT(0)
> #define VSTREAM_ENABLE BIT(3)
> #define LN_POLRS_OFFSET 4
> #define LN_POLRS_MASK 0xf0
> @@ -93,6 +94,8 @@
> #define SN_DATARATE_CONFIG_REG 0x94
> #define DP_DATARATE_MASK GENMASK(7, 5)
> #define DP_DATARATE(x) ((x) << 5)
> +#define SN_TRAINING_SETTING_REG 0x95
> +#define SCRAMBLE_DISABLE BIT(4)
> #define SN_ML_TX_MODE_REG 0x96
> #define ML_TX_MAIN_LINK_OFF 0
> #define ML_TX_NORMAL_MODE BIT(0)
> @@ -982,6 +985,17 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx,
> goto exit;
> }
>
> + /*
> + * eDP panels use an Alternate Scrambler Seed compared to displays
> + * hooked up via a full DisplayPort connector. SN65DSI86 only supports
> + * the alternate scrambler seed, not the normal one, so the only way we
> + * can support full DisplayPort displays is by fully turning off the
> + * scrambler.
> + */
> + if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
> + regmap_update_bits(pdata->regmap, SN_TRAINING_SETTING_REG,
> + SCRAMBLE_DISABLE, SCRAMBLE_DISABLE);
> +
> /*
> * We'll try to link train several times. As part of link training
> * the bridge chip will write DP_SET_POWER_D0 to DP_SET_POWER. If
> @@ -1046,12 +1060,13 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
>
> /*
> * The SN65DSI86 only supports ASSR Display Authentication method and
> - * this method is enabled by default. An eDP panel must support this
> + * this method is enabled for eDP panels. An eDP panel must support this
> * authentication method. We need to enable this method in the eDP panel
> * at DisplayPort address 0x0010A prior to link training.
> */
> - drm_dp_dpcd_writeb(&pdata->aux, DP_EDP_CONFIGURATION_SET,
> - DP_ALTERNATE_SCRAMBLER_RESET_ENABLE);
> + if (pdata->bridge.type == DRM_MODE_CONNECTOR_eDP)
> + drm_dp_dpcd_writeb(&pdata->aux, DP_EDP_CONFIGURATION_SET,
> + DP_ALTERNATE_SCRAMBLER_RESET_ENABLE);
>
> /* Set the DP output format (18 bpp or 24 bpp) */
> val = (ti_sn_bridge_get_bpp(old_bridge_state) == 18) ? BPP_18_RGB : 0;
> @@ -1215,6 +1230,8 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
>
> pdata->bridge.funcs = &ti_sn_bridge_funcs;
> pdata->bridge.of_node = np;
> + pdata->bridge.type = pdata->next_bridge->type == DRM_MODE_CONNECTOR_DisplayPort
> + ? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;
>
> drm_bridge_add(&pdata->bridge);
>
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v3 2/3] drm/bridge: ti-sn65dsi86: Implement bridge connector operations
2022-03-10 15:22 ` Kieran Bingham
@ 2022-03-10 16:23 ` Laurent Pinchart
-1 siblings, 0 replies; 33+ messages in thread
From: Laurent Pinchart @ 2022-03-10 16:23 UTC (permalink / raw)
To: Kieran Bingham
Cc: dri-devel, linux-renesas-soc, Sam Ravnborg, Douglas Anderson,
linux-kernel, Andrzej Hajda, Neil Armstrong, Robert Foss,
Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter
Hi Kieran,
Thank you for the patch.
On Thu, Mar 10, 2022 at 03:22:26PM +0000, Kieran Bingham wrote:
> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>
> Implement the bridge connector-related .get_edid() operation, and report
> the related bridge capabilities and type.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
> Changes since v1:
>
> - The connector .get_modes() operation doesn't rely on EDID anymore,
> __ti_sn_bridge_get_edid() and ti_sn_bridge_get_edid() got merged
> together
> - Fix on top of Sam Ravnborg's DRM_BRIDGE_STATE_OPS
>
> Changes since v2: [Kieran]
> - Only support EDID on DRM_MODE_CONNECTOR_DisplayPort modes.
>
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 93b54fcba8ba..d581c820e5d8 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -1135,10 +1135,24 @@ static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
> pm_runtime_put_sync(pdata->dev);
> }
>
> +static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge,
> + struct drm_connector *connector)
> +{
> + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> + struct edid *edid;
> +
> + pm_runtime_get_sync(pdata->dev);
> + edid = drm_get_edid(connector, &pdata->aux.ddc);
> + pm_runtime_put_autosuspend(pdata->dev);
> +
> + return edid;
> +}
> +
> static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
> .attach = ti_sn_bridge_attach,
> .detach = ti_sn_bridge_detach,
> .mode_valid = ti_sn_bridge_mode_valid,
> + .get_edid = ti_sn_bridge_get_edid,
> .atomic_pre_enable = ti_sn_bridge_atomic_pre_enable,
> .atomic_enable = ti_sn_bridge_atomic_enable,
> .atomic_disable = ti_sn_bridge_atomic_disable,
> @@ -1233,6 +1247,9 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
> pdata->bridge.type = pdata->next_bridge->type == DRM_MODE_CONNECTOR_DisplayPort
> ? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;
>
> + if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
> + pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
You could write this as
if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
pdata->bridge.ops |= DRM_BRIDGE_OP_EDID;
to be ready to support other ops, but that doesn't help
DRM_BRIDGE_OP_DETECT in 3/3, and I don't foresee the need for other ops.
In any case,
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> +
> drm_bridge_add(&pdata->bridge);
>
> ret = ti_sn_attach_host(pdata);
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v3 2/3] drm/bridge: ti-sn65dsi86: Implement bridge connector operations
@ 2022-03-10 16:23 ` Laurent Pinchart
0 siblings, 0 replies; 33+ messages in thread
From: Laurent Pinchart @ 2022-03-10 16:23 UTC (permalink / raw)
To: Kieran Bingham
Cc: Jernej Skrabec, Neil Armstrong, David Airlie, Jonas Karlman,
Douglas Anderson, dri-devel, linux-kernel, linux-renesas-soc,
Robert Foss, Andrzej Hajda, Sam Ravnborg
Hi Kieran,
Thank you for the patch.
On Thu, Mar 10, 2022 at 03:22:26PM +0000, Kieran Bingham wrote:
> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>
> Implement the bridge connector-related .get_edid() operation, and report
> the related bridge capabilities and type.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
> Changes since v1:
>
> - The connector .get_modes() operation doesn't rely on EDID anymore,
> __ti_sn_bridge_get_edid() and ti_sn_bridge_get_edid() got merged
> together
> - Fix on top of Sam Ravnborg's DRM_BRIDGE_STATE_OPS
>
> Changes since v2: [Kieran]
> - Only support EDID on DRM_MODE_CONNECTOR_DisplayPort modes.
>
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 93b54fcba8ba..d581c820e5d8 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -1135,10 +1135,24 @@ static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
> pm_runtime_put_sync(pdata->dev);
> }
>
> +static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge,
> + struct drm_connector *connector)
> +{
> + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> + struct edid *edid;
> +
> + pm_runtime_get_sync(pdata->dev);
> + edid = drm_get_edid(connector, &pdata->aux.ddc);
> + pm_runtime_put_autosuspend(pdata->dev);
> +
> + return edid;
> +}
> +
> static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
> .attach = ti_sn_bridge_attach,
> .detach = ti_sn_bridge_detach,
> .mode_valid = ti_sn_bridge_mode_valid,
> + .get_edid = ti_sn_bridge_get_edid,
> .atomic_pre_enable = ti_sn_bridge_atomic_pre_enable,
> .atomic_enable = ti_sn_bridge_atomic_enable,
> .atomic_disable = ti_sn_bridge_atomic_disable,
> @@ -1233,6 +1247,9 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
> pdata->bridge.type = pdata->next_bridge->type == DRM_MODE_CONNECTOR_DisplayPort
> ? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;
>
> + if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
> + pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
You could write this as
if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
pdata->bridge.ops |= DRM_BRIDGE_OP_EDID;
to be ready to support other ops, but that doesn't help
DRM_BRIDGE_OP_DETECT in 3/3, and I don't foresee the need for other ops.
In any case,
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> +
> drm_bridge_add(&pdata->bridge);
>
> ret = ti_sn_attach_host(pdata);
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v3 3/3] drm/bridge: ti-sn65dsi86: Support hotplug detection
2022-03-10 15:22 ` Kieran Bingham
@ 2022-03-10 16:42 ` Laurent Pinchart
-1 siblings, 0 replies; 33+ messages in thread
From: Laurent Pinchart @ 2022-03-10 16:42 UTC (permalink / raw)
To: Kieran Bingham
Cc: dri-devel, linux-renesas-soc, Sam Ravnborg, Douglas Anderson,
linux-kernel, Andrzej Hajda, Neil Armstrong, Robert Foss,
Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter
Hi Kieran,
Thank you for the patch.
On Thu, Mar 10, 2022 at 03:22:27PM +0000, Kieran Bingham wrote:
> When the SN65DSI86 is used in DisplayPort mode, its output is likely
> routed to a DisplayPort connector, which can benefit from hotplug
> detection. Support it in such cases, with polling mode only for now.
Don't we support IRQ mode too now ?
> The implementation is limited to the bridge operations, as the connector
> operations are legacy and new users should use
> DRM_BRIDGE_ATTACH_NO_CONNECTOR.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
> Changes since v1:
>
> - Document the no_hpd field
> - Rely on the SN_HPD_DISABLE_REG default value in the HPD case
> - Add a TODO comment regarding IRQ support
> [Kieran]
> - Fix spelling s/assrted/asserted/
> - Only enable HPD on DisplayPort connector.
> - Support IRQ based hotplug detect
>
> Changes since v2: [Kieran]
> - Use unsigned int for values read by regmap
> - Update HPD support warning message
> - Only enable OP_HPD if IRQ support enabled.
> - Only register IRQ handler during ti_sn_bridge_probe()
> - Store IRQ in the struct ti_sn65dsi86
> - Register IRQ only when !no-hpd
> - Refactor DRM_BRIDGE_OP_DETECT and DRM_BRIDGE_OP_HPD handling
>
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 142 +++++++++++++++++++++++---
> 1 file changed, 129 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index d581c820e5d8..328a48f09f97 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -70,6 +70,7 @@
> #define BPP_18_RGB BIT(0)
> #define SN_HPD_DISABLE_REG 0x5C
> #define HPD_DISABLE BIT(0)
> +#define HPD_DEBOUNCED_STATE BIT(4)
> #define SN_GPIO_IO_REG 0x5E
> #define SN_GPIO_INPUT_SHIFT 4
> #define SN_GPIO_OUTPUT_SHIFT 0
> @@ -106,10 +107,24 @@
> #define SN_PWM_EN_INV_REG 0xA5
> #define SN_PWM_INV_MASK BIT(0)
> #define SN_PWM_EN_MASK BIT(1)
> +#define SN_IRQ_EN_REG 0xE0
> +#define IRQ_EN BIT(0)
> +#define SN_IRQ_HPD_REG 0xE6
> +#define IRQ_HPD_EN BIT(0)
> +#define IRQ_HPD_INSERTION_EN BIT(1)
> +#define IRQ_HPD_REMOVAL_EN BIT(2)
> +#define IRQ_HPD_REPLUG_EN BIT(3)
> +#define IRQ_HPD_PLL_UNLOCK_EN BIT(5)
> #define SN_AUX_CMD_STATUS_REG 0xF4
> #define AUX_IRQ_STATUS_AUX_RPLY_TOUT BIT(3)
> #define AUX_IRQ_STATUS_AUX_SHORT BIT(5)
> #define AUX_IRQ_STATUS_NAT_I2C_FAIL BIT(6)
> +#define SN_IRQ_HPD_STATUS_REG 0xF5
> +#define IRQ_HPD_STATUS BIT(0)
> +#define IRQ_HPD_INSERTION_STATUS BIT(1)
> +#define IRQ_HPD_REMOVAL_STATUS BIT(2)
> +#define IRQ_HPD_REPLUG_STATUS BIT(3)
> +#define IRQ_PLL_UNLOCK BIT(5)
>
> #define MIN_DSI_CLK_FREQ_MHZ 40
>
> @@ -168,6 +183,12 @@
> * @pwm_enabled: Used to track if the PWM signal is currently enabled.
> * @pwm_pin_busy: Track if GPIO4 is currently requested for GPIO or PWM.
> * @pwm_refclk_freq: Cache for the reference clock input to the PWM.
> + *
> + * @no_hpd: Disable hot-plug detection as instructed by device tree (used
> + * for instance for eDP panels whose HPD signal won't be asserted
> + * until the panel is turned on, and is thus not usable for
> + * downstream device detection).
> + * @irq: IRQ number for the device.
> */
> struct ti_sn65dsi86 {
> struct auxiliary_device bridge_aux;
> @@ -202,6 +223,9 @@ struct ti_sn65dsi86 {
> atomic_t pwm_pin_busy;
> #endif
> unsigned int pwm_refclk_freq;
> +
> + bool no_hpd;
> + int irq;
> };
>
> static const struct regmap_range ti_sn65dsi86_volatile_ranges[] = {
> @@ -316,23 +340,25 @@ static void ti_sn65dsi86_enable_comms(struct ti_sn65dsi86 *pdata)
> ti_sn_bridge_set_refclk_freq(pdata);
>
> /*
> - * HPD on this bridge chip is a bit useless. This is an eDP bridge
> - * so the HPD is an internal signal that's only there to signal that
> - * the panel is done powering up. ...but the bridge chip debounces
> - * this signal by between 100 ms and 400 ms (depending on process,
> - * voltage, and temperate--I measured it at about 200 ms). One
> + * As this is an eDP bridge, the output will be connected to a fixed
> + * panel in most systems. HPD is in that case only an internal signal
> + * to signal that the panel is done powering up. The bridge chip
> + * debounces this signal by between 100 ms and 400 ms (depending on
> + * process, voltage, and temperate--I measured it at about 200 ms). One
> * particular panel asserted HPD 84 ms after it was powered on meaning
> * that we saw HPD 284 ms after power on. ...but the same panel said
> * that instead of looking at HPD you could just hardcode a delay of
> - * 200 ms. We'll assume that the panel driver will have the hardcoded
> - * delay in its prepare and always disable HPD.
> + * 200 ms. HPD is thus a bit useless. For this type of use cases, we'll
> + * assume that the panel driver will have the hardcoded delay in its
> + * prepare and always disable HPD.
> *
> - * If HPD somehow makes sense on some future panel we'll have to
> - * change this to be conditional on someone specifying that HPD should
> - * be used.
> + * However, on some systems, the output is connected to a DisplayPort
> + * connector. HPD is needed in such cases. To accommodate both use
> + * cases, enable HPD only when requested.
> */
> - regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
> - HPD_DISABLE);
> + if (pdata->no_hpd)
> + regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG,
> + HPD_DISABLE, HPD_DISABLE);
>
> pdata->comms_enabled = true;
>
> @@ -1135,6 +1161,36 @@ static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
> pm_runtime_put_sync(pdata->dev);
> }
>
> +static enum drm_connector_status ti_sn_bridge_detect(struct drm_bridge *bridge)
> +{
> + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> + int val;
> +
> + regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val);
> +
> + return val & HPD_DEBOUNCED_STATE ? connector_status_connected
> + : connector_status_disconnected;
> +}
> +
> +static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
> +{
> + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> +
> + /* The device must remain active for HPD to function */
> + pm_runtime_get_sync(pdata->dev);
> + regmap_write(pdata->regmap, SN_IRQ_HPD_REG,
> + IRQ_HPD_EN | IRQ_HPD_INSERTION_EN |
> + IRQ_HPD_REMOVAL_EN | IRQ_HPD_REPLUG_EN);
> +}
> +
> +static void ti_sn_bridge_hpd_disable(struct drm_bridge *bridge)
> +{
> + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> +
> + regmap_write(pdata->regmap, SN_IRQ_HPD_REG, 0);
> + pm_runtime_put_autosuspend(pdata->dev);
> +}
> +
> static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge,
> struct drm_connector *connector)
> {
> @@ -1153,6 +1209,9 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
> .detach = ti_sn_bridge_detach,
> .mode_valid = ti_sn_bridge_mode_valid,
> .get_edid = ti_sn_bridge_get_edid,
> + .detect = ti_sn_bridge_detect,
> + .hpd_enable = ti_sn_bridge_hpd_enable,
> + .hpd_disable = ti_sn_bridge_hpd_disable,
> .atomic_pre_enable = ti_sn_bridge_atomic_pre_enable,
> .atomic_enable = ti_sn_bridge_atomic_enable,
> .atomic_disable = ti_sn_bridge_atomic_disable,
> @@ -1223,6 +1282,34 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
> return 0;
> }
>
> +static irqreturn_t ti_sn65dsi86_irq_handler(int irq, void *arg)
> +{
> + struct ti_sn65dsi86 *pdata = arg;
> + int ret;
> + unsigned int hpd;
> +
> + ret = regmap_read(pdata->regmap, SN_IRQ_HPD_STATUS_REG, &hpd);
> + if (ret || !hpd)
> + return IRQ_NONE;
> +
> + if (hpd & IRQ_HPD_INSERTION_STATUS)
> + drm_bridge_hpd_notify(&pdata->bridge, connector_status_connected);
> +
> + if (hpd & IRQ_HPD_REMOVAL_STATUS)
> + drm_bridge_hpd_notify(&pdata->bridge, connector_status_disconnected);
> +
> + /* When replugged, ensure we trigger a detect to update the display */
> + if (hpd & IRQ_HPD_REPLUG_STATUS)
> + drm_bridge_hpd_notify(&pdata->bridge, connector_status_disconnected);
> +
> + /* reset the status registers */
s/registers/register/
> + regmap_write(pdata->regmap, SN_IRQ_HPD_STATUS_REG,
> + IRQ_HPD_STATUS | IRQ_HPD_INSERTION_STATUS |
> + IRQ_HPD_REMOVAL_STATUS | IRQ_HPD_REPLUG_STATUS);
> +
> + return IRQ_HANDLED;
> +}
> +
> static int ti_sn_bridge_probe(struct auxiliary_device *adev,
> const struct auxiliary_device_id *id)
> {
> @@ -1236,6 +1323,14 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
> return PTR_ERR(pdata->next_bridge);
> }
>
> + pdata->no_hpd = of_property_read_bool(np, "no-hpd");
> + if (pdata->next_bridge->type != DRM_MODE_CONNECTOR_DisplayPort &&
> + !pdata->no_hpd) {
> + dev_warn(pdata->dev,
> + "HPD support only implemented for full DP connectors\n");
> + pdata->no_hpd = true;
> + }
> +
> ti_sn_bridge_parse_lanes(pdata, np);
>
> ret = ti_sn_bridge_parse_dsi_host(pdata);
> @@ -1247,9 +1342,29 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
> pdata->bridge.type = pdata->next_bridge->type == DRM_MODE_CONNECTOR_DisplayPort
> ? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;
>
> - if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
> + if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort) {
> pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
>
> + if (!pdata->no_hpd)
> + pdata->bridge.ops |= DRM_BRIDGE_OP_DETECT;
> + }
> +
> + if (!pdata->no_hpd && pdata->irq > 0) {
> + dev_err(pdata->dev, "registering IRQ %d\n", pdata->irq);
As you've noted, this should be removed.
> +
> + ret = devm_request_threaded_irq(pdata->dev, pdata->irq, NULL,
> + ti_sn65dsi86_irq_handler,
> + IRQF_ONESHOT, "sn65dsi86-irq",
> + pdata);
> + if (ret)
> + return dev_err_probe(pdata->dev, ret,
> + "Failed to register DP interrupt\n");
> +
> + /* Enable IRQ based HPD */
> + regmap_write(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN);
Do we have a guarantee that the device isn't PM-suspended here ? Should
this be done in the PM resume handler ?
> + pdata->bridge.ops |= DRM_BRIDGE_OP_HPD;
> + }
> +
> drm_bridge_add(&pdata->bridge);
>
> ret = ti_sn_attach_host(pdata);
> @@ -1831,6 +1946,7 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
> return -ENOMEM;
> dev_set_drvdata(dev, pdata);
> pdata->dev = dev;
> + pdata->irq = client->irq;
>
> mutex_init(&pdata->comms_mutex);
>
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v3 3/3] drm/bridge: ti-sn65dsi86: Support hotplug detection
@ 2022-03-10 16:42 ` Laurent Pinchart
0 siblings, 0 replies; 33+ messages in thread
From: Laurent Pinchart @ 2022-03-10 16:42 UTC (permalink / raw)
To: Kieran Bingham
Cc: Jernej Skrabec, Neil Armstrong, David Airlie, Jonas Karlman,
Douglas Anderson, dri-devel, linux-kernel, linux-renesas-soc,
Robert Foss, Andrzej Hajda, Sam Ravnborg
Hi Kieran,
Thank you for the patch.
On Thu, Mar 10, 2022 at 03:22:27PM +0000, Kieran Bingham wrote:
> When the SN65DSI86 is used in DisplayPort mode, its output is likely
> routed to a DisplayPort connector, which can benefit from hotplug
> detection. Support it in such cases, with polling mode only for now.
Don't we support IRQ mode too now ?
> The implementation is limited to the bridge operations, as the connector
> operations are legacy and new users should use
> DRM_BRIDGE_ATTACH_NO_CONNECTOR.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
> Changes since v1:
>
> - Document the no_hpd field
> - Rely on the SN_HPD_DISABLE_REG default value in the HPD case
> - Add a TODO comment regarding IRQ support
> [Kieran]
> - Fix spelling s/assrted/asserted/
> - Only enable HPD on DisplayPort connector.
> - Support IRQ based hotplug detect
>
> Changes since v2: [Kieran]
> - Use unsigned int for values read by regmap
> - Update HPD support warning message
> - Only enable OP_HPD if IRQ support enabled.
> - Only register IRQ handler during ti_sn_bridge_probe()
> - Store IRQ in the struct ti_sn65dsi86
> - Register IRQ only when !no-hpd
> - Refactor DRM_BRIDGE_OP_DETECT and DRM_BRIDGE_OP_HPD handling
>
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 142 +++++++++++++++++++++++---
> 1 file changed, 129 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index d581c820e5d8..328a48f09f97 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -70,6 +70,7 @@
> #define BPP_18_RGB BIT(0)
> #define SN_HPD_DISABLE_REG 0x5C
> #define HPD_DISABLE BIT(0)
> +#define HPD_DEBOUNCED_STATE BIT(4)
> #define SN_GPIO_IO_REG 0x5E
> #define SN_GPIO_INPUT_SHIFT 4
> #define SN_GPIO_OUTPUT_SHIFT 0
> @@ -106,10 +107,24 @@
> #define SN_PWM_EN_INV_REG 0xA5
> #define SN_PWM_INV_MASK BIT(0)
> #define SN_PWM_EN_MASK BIT(1)
> +#define SN_IRQ_EN_REG 0xE0
> +#define IRQ_EN BIT(0)
> +#define SN_IRQ_HPD_REG 0xE6
> +#define IRQ_HPD_EN BIT(0)
> +#define IRQ_HPD_INSERTION_EN BIT(1)
> +#define IRQ_HPD_REMOVAL_EN BIT(2)
> +#define IRQ_HPD_REPLUG_EN BIT(3)
> +#define IRQ_HPD_PLL_UNLOCK_EN BIT(5)
> #define SN_AUX_CMD_STATUS_REG 0xF4
> #define AUX_IRQ_STATUS_AUX_RPLY_TOUT BIT(3)
> #define AUX_IRQ_STATUS_AUX_SHORT BIT(5)
> #define AUX_IRQ_STATUS_NAT_I2C_FAIL BIT(6)
> +#define SN_IRQ_HPD_STATUS_REG 0xF5
> +#define IRQ_HPD_STATUS BIT(0)
> +#define IRQ_HPD_INSERTION_STATUS BIT(1)
> +#define IRQ_HPD_REMOVAL_STATUS BIT(2)
> +#define IRQ_HPD_REPLUG_STATUS BIT(3)
> +#define IRQ_PLL_UNLOCK BIT(5)
>
> #define MIN_DSI_CLK_FREQ_MHZ 40
>
> @@ -168,6 +183,12 @@
> * @pwm_enabled: Used to track if the PWM signal is currently enabled.
> * @pwm_pin_busy: Track if GPIO4 is currently requested for GPIO or PWM.
> * @pwm_refclk_freq: Cache for the reference clock input to the PWM.
> + *
> + * @no_hpd: Disable hot-plug detection as instructed by device tree (used
> + * for instance for eDP panels whose HPD signal won't be asserted
> + * until the panel is turned on, and is thus not usable for
> + * downstream device detection).
> + * @irq: IRQ number for the device.
> */
> struct ti_sn65dsi86 {
> struct auxiliary_device bridge_aux;
> @@ -202,6 +223,9 @@ struct ti_sn65dsi86 {
> atomic_t pwm_pin_busy;
> #endif
> unsigned int pwm_refclk_freq;
> +
> + bool no_hpd;
> + int irq;
> };
>
> static const struct regmap_range ti_sn65dsi86_volatile_ranges[] = {
> @@ -316,23 +340,25 @@ static void ti_sn65dsi86_enable_comms(struct ti_sn65dsi86 *pdata)
> ti_sn_bridge_set_refclk_freq(pdata);
>
> /*
> - * HPD on this bridge chip is a bit useless. This is an eDP bridge
> - * so the HPD is an internal signal that's only there to signal that
> - * the panel is done powering up. ...but the bridge chip debounces
> - * this signal by between 100 ms and 400 ms (depending on process,
> - * voltage, and temperate--I measured it at about 200 ms). One
> + * As this is an eDP bridge, the output will be connected to a fixed
> + * panel in most systems. HPD is in that case only an internal signal
> + * to signal that the panel is done powering up. The bridge chip
> + * debounces this signal by between 100 ms and 400 ms (depending on
> + * process, voltage, and temperate--I measured it at about 200 ms). One
> * particular panel asserted HPD 84 ms after it was powered on meaning
> * that we saw HPD 284 ms after power on. ...but the same panel said
> * that instead of looking at HPD you could just hardcode a delay of
> - * 200 ms. We'll assume that the panel driver will have the hardcoded
> - * delay in its prepare and always disable HPD.
> + * 200 ms. HPD is thus a bit useless. For this type of use cases, we'll
> + * assume that the panel driver will have the hardcoded delay in its
> + * prepare and always disable HPD.
> *
> - * If HPD somehow makes sense on some future panel we'll have to
> - * change this to be conditional on someone specifying that HPD should
> - * be used.
> + * However, on some systems, the output is connected to a DisplayPort
> + * connector. HPD is needed in such cases. To accommodate both use
> + * cases, enable HPD only when requested.
> */
> - regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
> - HPD_DISABLE);
> + if (pdata->no_hpd)
> + regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG,
> + HPD_DISABLE, HPD_DISABLE);
>
> pdata->comms_enabled = true;
>
> @@ -1135,6 +1161,36 @@ static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
> pm_runtime_put_sync(pdata->dev);
> }
>
> +static enum drm_connector_status ti_sn_bridge_detect(struct drm_bridge *bridge)
> +{
> + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> + int val;
> +
> + regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val);
> +
> + return val & HPD_DEBOUNCED_STATE ? connector_status_connected
> + : connector_status_disconnected;
> +}
> +
> +static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
> +{
> + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> +
> + /* The device must remain active for HPD to function */
> + pm_runtime_get_sync(pdata->dev);
> + regmap_write(pdata->regmap, SN_IRQ_HPD_REG,
> + IRQ_HPD_EN | IRQ_HPD_INSERTION_EN |
> + IRQ_HPD_REMOVAL_EN | IRQ_HPD_REPLUG_EN);
> +}
> +
> +static void ti_sn_bridge_hpd_disable(struct drm_bridge *bridge)
> +{
> + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> +
> + regmap_write(pdata->regmap, SN_IRQ_HPD_REG, 0);
> + pm_runtime_put_autosuspend(pdata->dev);
> +}
> +
> static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge,
> struct drm_connector *connector)
> {
> @@ -1153,6 +1209,9 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
> .detach = ti_sn_bridge_detach,
> .mode_valid = ti_sn_bridge_mode_valid,
> .get_edid = ti_sn_bridge_get_edid,
> + .detect = ti_sn_bridge_detect,
> + .hpd_enable = ti_sn_bridge_hpd_enable,
> + .hpd_disable = ti_sn_bridge_hpd_disable,
> .atomic_pre_enable = ti_sn_bridge_atomic_pre_enable,
> .atomic_enable = ti_sn_bridge_atomic_enable,
> .atomic_disable = ti_sn_bridge_atomic_disable,
> @@ -1223,6 +1282,34 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
> return 0;
> }
>
> +static irqreturn_t ti_sn65dsi86_irq_handler(int irq, void *arg)
> +{
> + struct ti_sn65dsi86 *pdata = arg;
> + int ret;
> + unsigned int hpd;
> +
> + ret = regmap_read(pdata->regmap, SN_IRQ_HPD_STATUS_REG, &hpd);
> + if (ret || !hpd)
> + return IRQ_NONE;
> +
> + if (hpd & IRQ_HPD_INSERTION_STATUS)
> + drm_bridge_hpd_notify(&pdata->bridge, connector_status_connected);
> +
> + if (hpd & IRQ_HPD_REMOVAL_STATUS)
> + drm_bridge_hpd_notify(&pdata->bridge, connector_status_disconnected);
> +
> + /* When replugged, ensure we trigger a detect to update the display */
> + if (hpd & IRQ_HPD_REPLUG_STATUS)
> + drm_bridge_hpd_notify(&pdata->bridge, connector_status_disconnected);
> +
> + /* reset the status registers */
s/registers/register/
> + regmap_write(pdata->regmap, SN_IRQ_HPD_STATUS_REG,
> + IRQ_HPD_STATUS | IRQ_HPD_INSERTION_STATUS |
> + IRQ_HPD_REMOVAL_STATUS | IRQ_HPD_REPLUG_STATUS);
> +
> + return IRQ_HANDLED;
> +}
> +
> static int ti_sn_bridge_probe(struct auxiliary_device *adev,
> const struct auxiliary_device_id *id)
> {
> @@ -1236,6 +1323,14 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
> return PTR_ERR(pdata->next_bridge);
> }
>
> + pdata->no_hpd = of_property_read_bool(np, "no-hpd");
> + if (pdata->next_bridge->type != DRM_MODE_CONNECTOR_DisplayPort &&
> + !pdata->no_hpd) {
> + dev_warn(pdata->dev,
> + "HPD support only implemented for full DP connectors\n");
> + pdata->no_hpd = true;
> + }
> +
> ti_sn_bridge_parse_lanes(pdata, np);
>
> ret = ti_sn_bridge_parse_dsi_host(pdata);
> @@ -1247,9 +1342,29 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
> pdata->bridge.type = pdata->next_bridge->type == DRM_MODE_CONNECTOR_DisplayPort
> ? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;
>
> - if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
> + if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort) {
> pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
>
> + if (!pdata->no_hpd)
> + pdata->bridge.ops |= DRM_BRIDGE_OP_DETECT;
> + }
> +
> + if (!pdata->no_hpd && pdata->irq > 0) {
> + dev_err(pdata->dev, "registering IRQ %d\n", pdata->irq);
As you've noted, this should be removed.
> +
> + ret = devm_request_threaded_irq(pdata->dev, pdata->irq, NULL,
> + ti_sn65dsi86_irq_handler,
> + IRQF_ONESHOT, "sn65dsi86-irq",
> + pdata);
> + if (ret)
> + return dev_err_probe(pdata->dev, ret,
> + "Failed to register DP interrupt\n");
> +
> + /* Enable IRQ based HPD */
> + regmap_write(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN);
Do we have a guarantee that the device isn't PM-suspended here ? Should
this be done in the PM resume handler ?
> + pdata->bridge.ops |= DRM_BRIDGE_OP_HPD;
> + }
> +
> drm_bridge_add(&pdata->bridge);
>
> ret = ti_sn_attach_host(pdata);
> @@ -1831,6 +1946,7 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
> return -ENOMEM;
> dev_set_drvdata(dev, pdata);
> pdata->dev = dev;
> + pdata->irq = client->irq;
>
> mutex_init(&pdata->comms_mutex);
>
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v3 3/3] drm/bridge: ti-sn65dsi86: Support hotplug detection
2022-03-10 16:42 ` Laurent Pinchart
@ 2022-03-10 17:39 ` Kieran Bingham
-1 siblings, 0 replies; 33+ messages in thread
From: Kieran Bingham @ 2022-03-10 17:39 UTC (permalink / raw)
To: Laurent Pinchart
Cc: dri-devel, linux-renesas-soc, Sam Ravnborg, Douglas Anderson,
linux-kernel, Andrzej Hajda, Neil Armstrong, Robert Foss,
Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter
Hi Laurent
Quoting Laurent Pinchart (2022-03-10 16:42:48)
> Hi Kieran,
>
> Thank you for the patch.
>
> On Thu, Mar 10, 2022 at 03:22:27PM +0000, Kieran Bingham wrote:
> > When the SN65DSI86 is used in DisplayPort mode, its output is likely
> > routed to a DisplayPort connector, which can benefit from hotplug
> > detection. Support it in such cases, with polling mode only for now.
>
> Don't we support IRQ mode too now ?
Ah yes, I missed this for an update.
> > The implementation is limited to the bridge operations, as the connector
> > operations are legacy and new users should use
> > DRM_BRIDGE_ATTACH_NO_CONNECTOR.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > ---
> > Changes since v1:
> >
> > - Document the no_hpd field
> > - Rely on the SN_HPD_DISABLE_REG default value in the HPD case
> > - Add a TODO comment regarding IRQ support
> > [Kieran]
> > - Fix spelling s/assrted/asserted/
> > - Only enable HPD on DisplayPort connector.
> > - Support IRQ based hotplug detect
> >
> > Changes since v2: [Kieran]
> > - Use unsigned int for values read by regmap
> > - Update HPD support warning message
> > - Only enable OP_HPD if IRQ support enabled.
> > - Only register IRQ handler during ti_sn_bridge_probe()
> > - Store IRQ in the struct ti_sn65dsi86
> > - Register IRQ only when !no-hpd
> > - Refactor DRM_BRIDGE_OP_DETECT and DRM_BRIDGE_OP_HPD handling
> >
> > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 142 +++++++++++++++++++++++---
> > 1 file changed, 129 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index d581c820e5d8..328a48f09f97 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > @@ -70,6 +70,7 @@
> > #define BPP_18_RGB BIT(0)
> > #define SN_HPD_DISABLE_REG 0x5C
> > #define HPD_DISABLE BIT(0)
> > +#define HPD_DEBOUNCED_STATE BIT(4)
> > #define SN_GPIO_IO_REG 0x5E
> > #define SN_GPIO_INPUT_SHIFT 4
> > #define SN_GPIO_OUTPUT_SHIFT 0
> > @@ -106,10 +107,24 @@
> > #define SN_PWM_EN_INV_REG 0xA5
> > #define SN_PWM_INV_MASK BIT(0)
> > #define SN_PWM_EN_MASK BIT(1)
> > +#define SN_IRQ_EN_REG 0xE0
> > +#define IRQ_EN BIT(0)
> > +#define SN_IRQ_HPD_REG 0xE6
> > +#define IRQ_HPD_EN BIT(0)
> > +#define IRQ_HPD_INSERTION_EN BIT(1)
> > +#define IRQ_HPD_REMOVAL_EN BIT(2)
> > +#define IRQ_HPD_REPLUG_EN BIT(3)
> > +#define IRQ_HPD_PLL_UNLOCK_EN BIT(5)
> > #define SN_AUX_CMD_STATUS_REG 0xF4
> > #define AUX_IRQ_STATUS_AUX_RPLY_TOUT BIT(3)
> > #define AUX_IRQ_STATUS_AUX_SHORT BIT(5)
> > #define AUX_IRQ_STATUS_NAT_I2C_FAIL BIT(6)
> > +#define SN_IRQ_HPD_STATUS_REG 0xF5
> > +#define IRQ_HPD_STATUS BIT(0)
> > +#define IRQ_HPD_INSERTION_STATUS BIT(1)
> > +#define IRQ_HPD_REMOVAL_STATUS BIT(2)
> > +#define IRQ_HPD_REPLUG_STATUS BIT(3)
> > +#define IRQ_PLL_UNLOCK BIT(5)
> >
> > #define MIN_DSI_CLK_FREQ_MHZ 40
> >
> > @@ -168,6 +183,12 @@
> > * @pwm_enabled: Used to track if the PWM signal is currently enabled.
> > * @pwm_pin_busy: Track if GPIO4 is currently requested for GPIO or PWM.
> > * @pwm_refclk_freq: Cache for the reference clock input to the PWM.
> > + *
> > + * @no_hpd: Disable hot-plug detection as instructed by device tree (used
> > + * for instance for eDP panels whose HPD signal won't be asserted
> > + * until the panel is turned on, and is thus not usable for
> > + * downstream device detection).
> > + * @irq: IRQ number for the device.
> > */
> > struct ti_sn65dsi86 {
> > struct auxiliary_device bridge_aux;
> > @@ -202,6 +223,9 @@ struct ti_sn65dsi86 {
> > atomic_t pwm_pin_busy;
> > #endif
> > unsigned int pwm_refclk_freq;
> > +
> > + bool no_hpd;
> > + int irq;
> > };
> >
> > static const struct regmap_range ti_sn65dsi86_volatile_ranges[] = {
> > @@ -316,23 +340,25 @@ static void ti_sn65dsi86_enable_comms(struct ti_sn65dsi86 *pdata)
> > ti_sn_bridge_set_refclk_freq(pdata);
> >
> > /*
> > - * HPD on this bridge chip is a bit useless. This is an eDP bridge
> > - * so the HPD is an internal signal that's only there to signal that
> > - * the panel is done powering up. ...but the bridge chip debounces
> > - * this signal by between 100 ms and 400 ms (depending on process,
> > - * voltage, and temperate--I measured it at about 200 ms). One
> > + * As this is an eDP bridge, the output will be connected to a fixed
> > + * panel in most systems. HPD is in that case only an internal signal
> > + * to signal that the panel is done powering up. The bridge chip
> > + * debounces this signal by between 100 ms and 400 ms (depending on
> > + * process, voltage, and temperate--I measured it at about 200 ms). One
> > * particular panel asserted HPD 84 ms after it was powered on meaning
> > * that we saw HPD 284 ms after power on. ...but the same panel said
> > * that instead of looking at HPD you could just hardcode a delay of
> > - * 200 ms. We'll assume that the panel driver will have the hardcoded
> > - * delay in its prepare and always disable HPD.
> > + * 200 ms. HPD is thus a bit useless. For this type of use cases, we'll
> > + * assume that the panel driver will have the hardcoded delay in its
> > + * prepare and always disable HPD.
> > *
> > - * If HPD somehow makes sense on some future panel we'll have to
> > - * change this to be conditional on someone specifying that HPD should
> > - * be used.
> > + * However, on some systems, the output is connected to a DisplayPort
> > + * connector. HPD is needed in such cases. To accommodate both use
> > + * cases, enable HPD only when requested.
> > */
> > - regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
> > - HPD_DISABLE);
> > + if (pdata->no_hpd)
> > + regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG,
> > + HPD_DISABLE, HPD_DISABLE);
> >
> > pdata->comms_enabled = true;
> >
> > @@ -1135,6 +1161,36 @@ static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
> > pm_runtime_put_sync(pdata->dev);
> > }
> >
> > +static enum drm_connector_status ti_sn_bridge_detect(struct drm_bridge *bridge)
> > +{
> > + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> > + int val;
> > +
> > + regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val);
> > +
> > + return val & HPD_DEBOUNCED_STATE ? connector_status_connected
> > + : connector_status_disconnected;
> > +}
> > +
> > +static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
> > +{
> > + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> > +
> > + /* The device must remain active for HPD to function */
> > + pm_runtime_get_sync(pdata->dev);
> > + regmap_write(pdata->regmap, SN_IRQ_HPD_REG,
> > + IRQ_HPD_EN | IRQ_HPD_INSERTION_EN |
> > + IRQ_HPD_REMOVAL_EN | IRQ_HPD_REPLUG_EN);
> > +}
> > +
> > +static void ti_sn_bridge_hpd_disable(struct drm_bridge *bridge)
> > +{
> > + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> > +
> > + regmap_write(pdata->regmap, SN_IRQ_HPD_REG, 0);
> > + pm_runtime_put_autosuspend(pdata->dev);
> > +}
> > +
> > static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge,
> > struct drm_connector *connector)
> > {
> > @@ -1153,6 +1209,9 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
> > .detach = ti_sn_bridge_detach,
> > .mode_valid = ti_sn_bridge_mode_valid,
> > .get_edid = ti_sn_bridge_get_edid,
> > + .detect = ti_sn_bridge_detect,
> > + .hpd_enable = ti_sn_bridge_hpd_enable,
> > + .hpd_disable = ti_sn_bridge_hpd_disable,
> > .atomic_pre_enable = ti_sn_bridge_atomic_pre_enable,
> > .atomic_enable = ti_sn_bridge_atomic_enable,
> > .atomic_disable = ti_sn_bridge_atomic_disable,
> > @@ -1223,6 +1282,34 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
> > return 0;
> > }
> >
> > +static irqreturn_t ti_sn65dsi86_irq_handler(int irq, void *arg)
> > +{
> > + struct ti_sn65dsi86 *pdata = arg;
> > + int ret;
> > + unsigned int hpd;
> > +
> > + ret = regmap_read(pdata->regmap, SN_IRQ_HPD_STATUS_REG, &hpd);
> > + if (ret || !hpd)
> > + return IRQ_NONE;
> > +
> > + if (hpd & IRQ_HPD_INSERTION_STATUS)
> > + drm_bridge_hpd_notify(&pdata->bridge, connector_status_connected);
> > +
> > + if (hpd & IRQ_HPD_REMOVAL_STATUS)
> > + drm_bridge_hpd_notify(&pdata->bridge, connector_status_disconnected);
> > +
> > + /* When replugged, ensure we trigger a detect to update the display */
> > + if (hpd & IRQ_HPD_REPLUG_STATUS)
> > + drm_bridge_hpd_notify(&pdata->bridge, connector_status_disconnected);
> > +
> > + /* reset the status registers */
>
> s/registers/register/
Ack.
>
> > + regmap_write(pdata->regmap, SN_IRQ_HPD_STATUS_REG,
> > + IRQ_HPD_STATUS | IRQ_HPD_INSERTION_STATUS |
> > + IRQ_HPD_REMOVAL_STATUS | IRQ_HPD_REPLUG_STATUS);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > static int ti_sn_bridge_probe(struct auxiliary_device *adev,
> > const struct auxiliary_device_id *id)
> > {
> > @@ -1236,6 +1323,14 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
> > return PTR_ERR(pdata->next_bridge);
> > }
> >
> > + pdata->no_hpd = of_property_read_bool(np, "no-hpd");
> > + if (pdata->next_bridge->type != DRM_MODE_CONNECTOR_DisplayPort &&
> > + !pdata->no_hpd) {
> > + dev_warn(pdata->dev,
> > + "HPD support only implemented for full DP connectors\n");
> > + pdata->no_hpd = true;
> > + }
> > +
> > ti_sn_bridge_parse_lanes(pdata, np);
> >
> > ret = ti_sn_bridge_parse_dsi_host(pdata);
> > @@ -1247,9 +1342,29 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
> > pdata->bridge.type = pdata->next_bridge->type == DRM_MODE_CONNECTOR_DisplayPort
> > ? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;
> >
> > - if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
> > + if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort) {
> > pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
> >
> > + if (!pdata->no_hpd)
> > + pdata->bridge.ops |= DRM_BRIDGE_OP_DETECT;
> > + }
> > +
> > + if (!pdata->no_hpd && pdata->irq > 0) {
> > + dev_err(pdata->dev, "registering IRQ %d\n", pdata->irq);
>
> As you've noted, this should be removed.
Already gone in my tree.
>
> > +
> > + ret = devm_request_threaded_irq(pdata->dev, pdata->irq, NULL,
> > + ti_sn65dsi86_irq_handler,
> > + IRQF_ONESHOT, "sn65dsi86-irq",
> > + pdata);
> > + if (ret)
> > + return dev_err_probe(pdata->dev, ret,
> > + "Failed to register DP interrupt\n");
> > +
> > + /* Enable IRQ based HPD */
> > + regmap_write(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN);
>
> Do we have a guarantee that the device isn't PM-suspended here ? Should
> this be done in the PM resume handler ?
This is tricky I fear. The ti_sn_bridge_probe() is called after the main
ti_sn65dsi86_probe(), which is where the pm resume handler would get
called first.
I moved the handler registration here, because we can't handle the IRQ
until we have the bridge set up, but if we were to enable interrupts in
the pm resume handler, it would get enabled during ti_sn65dsi86_probe()
before the handler is registered.
Now at that point nothing should fire an interrupt, but still - enabling
interrupts before the handler is registered doesn't sound good to me.
--
Kieran
>
> > + pdata->bridge.ops |= DRM_BRIDGE_OP_HPD;
> > + }
> > +
> > drm_bridge_add(&pdata->bridge);
> >
> > ret = ti_sn_attach_host(pdata);
> > @@ -1831,6 +1946,7 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
> > return -ENOMEM;
> > dev_set_drvdata(dev, pdata);
> > pdata->dev = dev;
> > + pdata->irq = client->irq;
> >
> > mutex_init(&pdata->comms_mutex);
> >
>
> --
> Regards,
>
> Laurent Pinchart
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v3 3/3] drm/bridge: ti-sn65dsi86: Support hotplug detection
@ 2022-03-10 17:39 ` Kieran Bingham
0 siblings, 0 replies; 33+ messages in thread
From: Kieran Bingham @ 2022-03-10 17:39 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Jernej Skrabec, Neil Armstrong, David Airlie, Jonas Karlman,
Douglas Anderson, dri-devel, linux-kernel, linux-renesas-soc,
Robert Foss, Andrzej Hajda, Sam Ravnborg
Hi Laurent
Quoting Laurent Pinchart (2022-03-10 16:42:48)
> Hi Kieran,
>
> Thank you for the patch.
>
> On Thu, Mar 10, 2022 at 03:22:27PM +0000, Kieran Bingham wrote:
> > When the SN65DSI86 is used in DisplayPort mode, its output is likely
> > routed to a DisplayPort connector, which can benefit from hotplug
> > detection. Support it in such cases, with polling mode only for now.
>
> Don't we support IRQ mode too now ?
Ah yes, I missed this for an update.
> > The implementation is limited to the bridge operations, as the connector
> > operations are legacy and new users should use
> > DRM_BRIDGE_ATTACH_NO_CONNECTOR.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > ---
> > Changes since v1:
> >
> > - Document the no_hpd field
> > - Rely on the SN_HPD_DISABLE_REG default value in the HPD case
> > - Add a TODO comment regarding IRQ support
> > [Kieran]
> > - Fix spelling s/assrted/asserted/
> > - Only enable HPD on DisplayPort connector.
> > - Support IRQ based hotplug detect
> >
> > Changes since v2: [Kieran]
> > - Use unsigned int for values read by regmap
> > - Update HPD support warning message
> > - Only enable OP_HPD if IRQ support enabled.
> > - Only register IRQ handler during ti_sn_bridge_probe()
> > - Store IRQ in the struct ti_sn65dsi86
> > - Register IRQ only when !no-hpd
> > - Refactor DRM_BRIDGE_OP_DETECT and DRM_BRIDGE_OP_HPD handling
> >
> > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 142 +++++++++++++++++++++++---
> > 1 file changed, 129 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index d581c820e5d8..328a48f09f97 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > @@ -70,6 +70,7 @@
> > #define BPP_18_RGB BIT(0)
> > #define SN_HPD_DISABLE_REG 0x5C
> > #define HPD_DISABLE BIT(0)
> > +#define HPD_DEBOUNCED_STATE BIT(4)
> > #define SN_GPIO_IO_REG 0x5E
> > #define SN_GPIO_INPUT_SHIFT 4
> > #define SN_GPIO_OUTPUT_SHIFT 0
> > @@ -106,10 +107,24 @@
> > #define SN_PWM_EN_INV_REG 0xA5
> > #define SN_PWM_INV_MASK BIT(0)
> > #define SN_PWM_EN_MASK BIT(1)
> > +#define SN_IRQ_EN_REG 0xE0
> > +#define IRQ_EN BIT(0)
> > +#define SN_IRQ_HPD_REG 0xE6
> > +#define IRQ_HPD_EN BIT(0)
> > +#define IRQ_HPD_INSERTION_EN BIT(1)
> > +#define IRQ_HPD_REMOVAL_EN BIT(2)
> > +#define IRQ_HPD_REPLUG_EN BIT(3)
> > +#define IRQ_HPD_PLL_UNLOCK_EN BIT(5)
> > #define SN_AUX_CMD_STATUS_REG 0xF4
> > #define AUX_IRQ_STATUS_AUX_RPLY_TOUT BIT(3)
> > #define AUX_IRQ_STATUS_AUX_SHORT BIT(5)
> > #define AUX_IRQ_STATUS_NAT_I2C_FAIL BIT(6)
> > +#define SN_IRQ_HPD_STATUS_REG 0xF5
> > +#define IRQ_HPD_STATUS BIT(0)
> > +#define IRQ_HPD_INSERTION_STATUS BIT(1)
> > +#define IRQ_HPD_REMOVAL_STATUS BIT(2)
> > +#define IRQ_HPD_REPLUG_STATUS BIT(3)
> > +#define IRQ_PLL_UNLOCK BIT(5)
> >
> > #define MIN_DSI_CLK_FREQ_MHZ 40
> >
> > @@ -168,6 +183,12 @@
> > * @pwm_enabled: Used to track if the PWM signal is currently enabled.
> > * @pwm_pin_busy: Track if GPIO4 is currently requested for GPIO or PWM.
> > * @pwm_refclk_freq: Cache for the reference clock input to the PWM.
> > + *
> > + * @no_hpd: Disable hot-plug detection as instructed by device tree (used
> > + * for instance for eDP panels whose HPD signal won't be asserted
> > + * until the panel is turned on, and is thus not usable for
> > + * downstream device detection).
> > + * @irq: IRQ number for the device.
> > */
> > struct ti_sn65dsi86 {
> > struct auxiliary_device bridge_aux;
> > @@ -202,6 +223,9 @@ struct ti_sn65dsi86 {
> > atomic_t pwm_pin_busy;
> > #endif
> > unsigned int pwm_refclk_freq;
> > +
> > + bool no_hpd;
> > + int irq;
> > };
> >
> > static const struct regmap_range ti_sn65dsi86_volatile_ranges[] = {
> > @@ -316,23 +340,25 @@ static void ti_sn65dsi86_enable_comms(struct ti_sn65dsi86 *pdata)
> > ti_sn_bridge_set_refclk_freq(pdata);
> >
> > /*
> > - * HPD on this bridge chip is a bit useless. This is an eDP bridge
> > - * so the HPD is an internal signal that's only there to signal that
> > - * the panel is done powering up. ...but the bridge chip debounces
> > - * this signal by between 100 ms and 400 ms (depending on process,
> > - * voltage, and temperate--I measured it at about 200 ms). One
> > + * As this is an eDP bridge, the output will be connected to a fixed
> > + * panel in most systems. HPD is in that case only an internal signal
> > + * to signal that the panel is done powering up. The bridge chip
> > + * debounces this signal by between 100 ms and 400 ms (depending on
> > + * process, voltage, and temperate--I measured it at about 200 ms). One
> > * particular panel asserted HPD 84 ms after it was powered on meaning
> > * that we saw HPD 284 ms after power on. ...but the same panel said
> > * that instead of looking at HPD you could just hardcode a delay of
> > - * 200 ms. We'll assume that the panel driver will have the hardcoded
> > - * delay in its prepare and always disable HPD.
> > + * 200 ms. HPD is thus a bit useless. For this type of use cases, we'll
> > + * assume that the panel driver will have the hardcoded delay in its
> > + * prepare and always disable HPD.
> > *
> > - * If HPD somehow makes sense on some future panel we'll have to
> > - * change this to be conditional on someone specifying that HPD should
> > - * be used.
> > + * However, on some systems, the output is connected to a DisplayPort
> > + * connector. HPD is needed in such cases. To accommodate both use
> > + * cases, enable HPD only when requested.
> > */
> > - regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
> > - HPD_DISABLE);
> > + if (pdata->no_hpd)
> > + regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG,
> > + HPD_DISABLE, HPD_DISABLE);
> >
> > pdata->comms_enabled = true;
> >
> > @@ -1135,6 +1161,36 @@ static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
> > pm_runtime_put_sync(pdata->dev);
> > }
> >
> > +static enum drm_connector_status ti_sn_bridge_detect(struct drm_bridge *bridge)
> > +{
> > + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> > + int val;
> > +
> > + regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val);
> > +
> > + return val & HPD_DEBOUNCED_STATE ? connector_status_connected
> > + : connector_status_disconnected;
> > +}
> > +
> > +static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
> > +{
> > + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> > +
> > + /* The device must remain active for HPD to function */
> > + pm_runtime_get_sync(pdata->dev);
> > + regmap_write(pdata->regmap, SN_IRQ_HPD_REG,
> > + IRQ_HPD_EN | IRQ_HPD_INSERTION_EN |
> > + IRQ_HPD_REMOVAL_EN | IRQ_HPD_REPLUG_EN);
> > +}
> > +
> > +static void ti_sn_bridge_hpd_disable(struct drm_bridge *bridge)
> > +{
> > + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> > +
> > + regmap_write(pdata->regmap, SN_IRQ_HPD_REG, 0);
> > + pm_runtime_put_autosuspend(pdata->dev);
> > +}
> > +
> > static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge,
> > struct drm_connector *connector)
> > {
> > @@ -1153,6 +1209,9 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
> > .detach = ti_sn_bridge_detach,
> > .mode_valid = ti_sn_bridge_mode_valid,
> > .get_edid = ti_sn_bridge_get_edid,
> > + .detect = ti_sn_bridge_detect,
> > + .hpd_enable = ti_sn_bridge_hpd_enable,
> > + .hpd_disable = ti_sn_bridge_hpd_disable,
> > .atomic_pre_enable = ti_sn_bridge_atomic_pre_enable,
> > .atomic_enable = ti_sn_bridge_atomic_enable,
> > .atomic_disable = ti_sn_bridge_atomic_disable,
> > @@ -1223,6 +1282,34 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
> > return 0;
> > }
> >
> > +static irqreturn_t ti_sn65dsi86_irq_handler(int irq, void *arg)
> > +{
> > + struct ti_sn65dsi86 *pdata = arg;
> > + int ret;
> > + unsigned int hpd;
> > +
> > + ret = regmap_read(pdata->regmap, SN_IRQ_HPD_STATUS_REG, &hpd);
> > + if (ret || !hpd)
> > + return IRQ_NONE;
> > +
> > + if (hpd & IRQ_HPD_INSERTION_STATUS)
> > + drm_bridge_hpd_notify(&pdata->bridge, connector_status_connected);
> > +
> > + if (hpd & IRQ_HPD_REMOVAL_STATUS)
> > + drm_bridge_hpd_notify(&pdata->bridge, connector_status_disconnected);
> > +
> > + /* When replugged, ensure we trigger a detect to update the display */
> > + if (hpd & IRQ_HPD_REPLUG_STATUS)
> > + drm_bridge_hpd_notify(&pdata->bridge, connector_status_disconnected);
> > +
> > + /* reset the status registers */
>
> s/registers/register/
Ack.
>
> > + regmap_write(pdata->regmap, SN_IRQ_HPD_STATUS_REG,
> > + IRQ_HPD_STATUS | IRQ_HPD_INSERTION_STATUS |
> > + IRQ_HPD_REMOVAL_STATUS | IRQ_HPD_REPLUG_STATUS);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > static int ti_sn_bridge_probe(struct auxiliary_device *adev,
> > const struct auxiliary_device_id *id)
> > {
> > @@ -1236,6 +1323,14 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
> > return PTR_ERR(pdata->next_bridge);
> > }
> >
> > + pdata->no_hpd = of_property_read_bool(np, "no-hpd");
> > + if (pdata->next_bridge->type != DRM_MODE_CONNECTOR_DisplayPort &&
> > + !pdata->no_hpd) {
> > + dev_warn(pdata->dev,
> > + "HPD support only implemented for full DP connectors\n");
> > + pdata->no_hpd = true;
> > + }
> > +
> > ti_sn_bridge_parse_lanes(pdata, np);
> >
> > ret = ti_sn_bridge_parse_dsi_host(pdata);
> > @@ -1247,9 +1342,29 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
> > pdata->bridge.type = pdata->next_bridge->type == DRM_MODE_CONNECTOR_DisplayPort
> > ? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;
> >
> > - if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
> > + if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort) {
> > pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
> >
> > + if (!pdata->no_hpd)
> > + pdata->bridge.ops |= DRM_BRIDGE_OP_DETECT;
> > + }
> > +
> > + if (!pdata->no_hpd && pdata->irq > 0) {
> > + dev_err(pdata->dev, "registering IRQ %d\n", pdata->irq);
>
> As you've noted, this should be removed.
Already gone in my tree.
>
> > +
> > + ret = devm_request_threaded_irq(pdata->dev, pdata->irq, NULL,
> > + ti_sn65dsi86_irq_handler,
> > + IRQF_ONESHOT, "sn65dsi86-irq",
> > + pdata);
> > + if (ret)
> > + return dev_err_probe(pdata->dev, ret,
> > + "Failed to register DP interrupt\n");
> > +
> > + /* Enable IRQ based HPD */
> > + regmap_write(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN);
>
> Do we have a guarantee that the device isn't PM-suspended here ? Should
> this be done in the PM resume handler ?
This is tricky I fear. The ti_sn_bridge_probe() is called after the main
ti_sn65dsi86_probe(), which is where the pm resume handler would get
called first.
I moved the handler registration here, because we can't handle the IRQ
until we have the bridge set up, but if we were to enable interrupts in
the pm resume handler, it would get enabled during ti_sn65dsi86_probe()
before the handler is registered.
Now at that point nothing should fire an interrupt, but still - enabling
interrupts before the handler is registered doesn't sound good to me.
--
Kieran
>
> > + pdata->bridge.ops |= DRM_BRIDGE_OP_HPD;
> > + }
> > +
> > drm_bridge_add(&pdata->bridge);
> >
> > ret = ti_sn_attach_host(pdata);
> > @@ -1831,6 +1946,7 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
> > return -ENOMEM;
> > dev_set_drvdata(dev, pdata);
> > pdata->dev = dev;
> > + pdata->irq = client->irq;
> >
> > mutex_init(&pdata->comms_mutex);
> >
>
> --
> Regards,
>
> Laurent Pinchart
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v3 3/3] drm/bridge: ti-sn65dsi86: Support hotplug detection
2022-03-10 15:22 ` Kieran Bingham
@ 2022-03-10 23:10 ` Doug Anderson
-1 siblings, 0 replies; 33+ messages in thread
From: Doug Anderson @ 2022-03-10 23:10 UTC (permalink / raw)
To: Kieran Bingham
Cc: dri-devel, Linux-Renesas, Sam Ravnborg, LKML, Andrzej Hajda,
Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman,
Jernej Skrabec, David Airlie, Daniel Vetter, Laurent Pinchart
Hi,
On Thu, Mar 10, 2022 at 7:22 AM Kieran Bingham
<kieran.bingham+renesas@ideasonboard.com> wrote:
>
> @@ -1135,6 +1161,36 @@ static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
> pm_runtime_put_sync(pdata->dev);
> }
>
> +static enum drm_connector_status ti_sn_bridge_detect(struct drm_bridge *bridge)
> +{
> + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> + int val;
> +
> + regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val);
Don't you need a pm_runtime_get_sync() before this and a
put_autosuspend() after? The "detect" will be used in the yes-HPD but
no-IRQ case, right? In that case there's nobody holding the pm_runtime
reference.
Also, a nit that it'd be great if you error checked the regmap_read().
I know this driver isn't very good about it, but it's probably
something to get better. i2c transactions can fail. I guess another
alternative would be to init "val" to 0...
> + return val & HPD_DEBOUNCED_STATE ? connector_status_connected
> + : connector_status_disconnected;
> +}
> +
> +static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
> +{
> + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> +
> + /* The device must remain active for HPD to function */
> + pm_runtime_get_sync(pdata->dev);
> + regmap_write(pdata->regmap, SN_IRQ_HPD_REG,
> + IRQ_HPD_EN | IRQ_HPD_INSERTION_EN |
> + IRQ_HPD_REMOVAL_EN | IRQ_HPD_REPLUG_EN);
> +}
> +
> +static void ti_sn_bridge_hpd_disable(struct drm_bridge *bridge)
> +{
> + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> +
> + regmap_write(pdata->regmap, SN_IRQ_HPD_REG, 0);
> + pm_runtime_put_autosuspend(pdata->dev);
Before doing the pm_runtime_put_autosuspend() it feels like you should
ensure that the interrupt has finished. Otherwise we could be midway
through processing an interrupt and the pm_runtime reference could go
away, right? Maybe we just disable the irq which I think will wait for
anything outstanding to finish?
> @@ -1223,6 +1282,34 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
> return 0;
> }
>
> +static irqreturn_t ti_sn65dsi86_irq_handler(int irq, void *arg)
> +{
> + struct ti_sn65dsi86 *pdata = arg;
> + int ret;
> + unsigned int hpd;
> +
> + ret = regmap_read(pdata->regmap, SN_IRQ_HPD_STATUS_REG, &hpd);
> + if (ret || !hpd)
> + return IRQ_NONE;
> +
> + if (hpd & IRQ_HPD_INSERTION_STATUS)
> + drm_bridge_hpd_notify(&pdata->bridge, connector_status_connected);
> +
> + if (hpd & IRQ_HPD_REMOVAL_STATUS)
> + drm_bridge_hpd_notify(&pdata->bridge, connector_status_disconnected);
> +
> + /* When replugged, ensure we trigger a detect to update the display */
> + if (hpd & IRQ_HPD_REPLUG_STATUS)
> + drm_bridge_hpd_notify(&pdata->bridge, connector_status_disconnected);
How does the ordering work here if _both_ insertion and removal are
asserted? Is that somehow not possible? Should this be "else if" type
statements then, or give a warn if more than one bit is set, or ... ?
> + /* reset the status registers */
> + regmap_write(pdata->regmap, SN_IRQ_HPD_STATUS_REG,
> + IRQ_HPD_STATUS | IRQ_HPD_INSERTION_STATUS |
> + IRQ_HPD_REMOVAL_STATUS | IRQ_HPD_REPLUG_STATUS);
IMO this regmap_write() belongs right after the read and should be
based on what you read--you shouldn't just clear all of them. AKA:
a) Read to see what interrupt are asserted.
b) Ack the interrupts that you saw asserted.
c) Process the interrupts that you saw asserted.
If you process before acking then you can miss interrupts (in other
words if you do "a" then "c" then "b" then you can miss interrupts
that come in after "b" but before "c".
> @@ -1247,9 +1342,29 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
> pdata->bridge.type = pdata->next_bridge->type == DRM_MODE_CONNECTOR_DisplayPort
> ? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;
>
> - if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
> + if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort) {
> pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
>
> + if (!pdata->no_hpd)
> + pdata->bridge.ops |= DRM_BRIDGE_OP_DETECT;
> + }
> +
> + if (!pdata->no_hpd && pdata->irq > 0) {
> + dev_err(pdata->dev, "registering IRQ %d\n", pdata->irq);
> +
> + ret = devm_request_threaded_irq(pdata->dev, pdata->irq, NULL,
> + ti_sn65dsi86_irq_handler,
> + IRQF_ONESHOT, "sn65dsi86-irq",
> + pdata);
> + if (ret)
> + return dev_err_probe(pdata->dev, ret,
> + "Failed to register DP interrupt\n");
> +
> + /* Enable IRQ based HPD */
> + regmap_write(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN);
Why not put the above regmap_write() in the ti_sn_bridge_hpd_enable() call?
-Doug
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v3 3/3] drm/bridge: ti-sn65dsi86: Support hotplug detection
@ 2022-03-10 23:10 ` Doug Anderson
0 siblings, 0 replies; 33+ messages in thread
From: Doug Anderson @ 2022-03-10 23:10 UTC (permalink / raw)
To: Kieran Bingham
Cc: Jernej Skrabec, Neil Armstrong, David Airlie, Jonas Karlman,
LKML, dri-devel, Linux-Renesas, Laurent Pinchart, Robert Foss,
Andrzej Hajda, Sam Ravnborg, Laurent Pinchart
Hi,
On Thu, Mar 10, 2022 at 7:22 AM Kieran Bingham
<kieran.bingham+renesas@ideasonboard.com> wrote:
>
> @@ -1135,6 +1161,36 @@ static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
> pm_runtime_put_sync(pdata->dev);
> }
>
> +static enum drm_connector_status ti_sn_bridge_detect(struct drm_bridge *bridge)
> +{
> + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> + int val;
> +
> + regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val);
Don't you need a pm_runtime_get_sync() before this and a
put_autosuspend() after? The "detect" will be used in the yes-HPD but
no-IRQ case, right? In that case there's nobody holding the pm_runtime
reference.
Also, a nit that it'd be great if you error checked the regmap_read().
I know this driver isn't very good about it, but it's probably
something to get better. i2c transactions can fail. I guess another
alternative would be to init "val" to 0...
> + return val & HPD_DEBOUNCED_STATE ? connector_status_connected
> + : connector_status_disconnected;
> +}
> +
> +static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
> +{
> + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> +
> + /* The device must remain active for HPD to function */
> + pm_runtime_get_sync(pdata->dev);
> + regmap_write(pdata->regmap, SN_IRQ_HPD_REG,
> + IRQ_HPD_EN | IRQ_HPD_INSERTION_EN |
> + IRQ_HPD_REMOVAL_EN | IRQ_HPD_REPLUG_EN);
> +}
> +
> +static void ti_sn_bridge_hpd_disable(struct drm_bridge *bridge)
> +{
> + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> +
> + regmap_write(pdata->regmap, SN_IRQ_HPD_REG, 0);
> + pm_runtime_put_autosuspend(pdata->dev);
Before doing the pm_runtime_put_autosuspend() it feels like you should
ensure that the interrupt has finished. Otherwise we could be midway
through processing an interrupt and the pm_runtime reference could go
away, right? Maybe we just disable the irq which I think will wait for
anything outstanding to finish?
> @@ -1223,6 +1282,34 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
> return 0;
> }
>
> +static irqreturn_t ti_sn65dsi86_irq_handler(int irq, void *arg)
> +{
> + struct ti_sn65dsi86 *pdata = arg;
> + int ret;
> + unsigned int hpd;
> +
> + ret = regmap_read(pdata->regmap, SN_IRQ_HPD_STATUS_REG, &hpd);
> + if (ret || !hpd)
> + return IRQ_NONE;
> +
> + if (hpd & IRQ_HPD_INSERTION_STATUS)
> + drm_bridge_hpd_notify(&pdata->bridge, connector_status_connected);
> +
> + if (hpd & IRQ_HPD_REMOVAL_STATUS)
> + drm_bridge_hpd_notify(&pdata->bridge, connector_status_disconnected);
> +
> + /* When replugged, ensure we trigger a detect to update the display */
> + if (hpd & IRQ_HPD_REPLUG_STATUS)
> + drm_bridge_hpd_notify(&pdata->bridge, connector_status_disconnected);
How does the ordering work here if _both_ insertion and removal are
asserted? Is that somehow not possible? Should this be "else if" type
statements then, or give a warn if more than one bit is set, or ... ?
> + /* reset the status registers */
> + regmap_write(pdata->regmap, SN_IRQ_HPD_STATUS_REG,
> + IRQ_HPD_STATUS | IRQ_HPD_INSERTION_STATUS |
> + IRQ_HPD_REMOVAL_STATUS | IRQ_HPD_REPLUG_STATUS);
IMO this regmap_write() belongs right after the read and should be
based on what you read--you shouldn't just clear all of them. AKA:
a) Read to see what interrupt are asserted.
b) Ack the interrupts that you saw asserted.
c) Process the interrupts that you saw asserted.
If you process before acking then you can miss interrupts (in other
words if you do "a" then "c" then "b" then you can miss interrupts
that come in after "b" but before "c".
> @@ -1247,9 +1342,29 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
> pdata->bridge.type = pdata->next_bridge->type == DRM_MODE_CONNECTOR_DisplayPort
> ? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;
>
> - if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
> + if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort) {
> pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
>
> + if (!pdata->no_hpd)
> + pdata->bridge.ops |= DRM_BRIDGE_OP_DETECT;
> + }
> +
> + if (!pdata->no_hpd && pdata->irq > 0) {
> + dev_err(pdata->dev, "registering IRQ %d\n", pdata->irq);
> +
> + ret = devm_request_threaded_irq(pdata->dev, pdata->irq, NULL,
> + ti_sn65dsi86_irq_handler,
> + IRQF_ONESHOT, "sn65dsi86-irq",
> + pdata);
> + if (ret)
> + return dev_err_probe(pdata->dev, ret,
> + "Failed to register DP interrupt\n");
> +
> + /* Enable IRQ based HPD */
> + regmap_write(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN);
Why not put the above regmap_write() in the ti_sn_bridge_hpd_enable() call?
-Doug
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v3 2/3] drm/bridge: ti-sn65dsi86: Implement bridge connector operations
2022-03-10 15:22 ` Kieran Bingham
@ 2022-03-10 23:10 ` Doug Anderson
-1 siblings, 0 replies; 33+ messages in thread
From: Doug Anderson @ 2022-03-10 23:10 UTC (permalink / raw)
To: Kieran Bingham
Cc: dri-devel, Linux-Renesas, Sam Ravnborg, LKML, Andrzej Hajda,
Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman,
Jernej Skrabec, David Airlie, Daniel Vetter, Laurent Pinchart
Hi,
On Thu, Mar 10, 2022 at 7:22 AM Kieran Bingham
<kieran.bingham+renesas@ideasonboard.com> wrote:
>
> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>
> Implement the bridge connector-related .get_edid() operation, and report
> the related bridge capabilities and type.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
> Changes since v1:
>
> - The connector .get_modes() operation doesn't rely on EDID anymore,
> __ti_sn_bridge_get_edid() and ti_sn_bridge_get_edid() got merged
> together
> - Fix on top of Sam Ravnborg's DRM_BRIDGE_STATE_OPS
>
> Changes since v2: [Kieran]
> - Only support EDID on DRM_MODE_CONNECTOR_DisplayPort modes.
>
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 93b54fcba8ba..d581c820e5d8 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -1135,10 +1135,24 @@ static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
> pm_runtime_put_sync(pdata->dev);
> }
>
> +static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge,
> + struct drm_connector *connector)
> +{
> + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> + struct edid *edid;
> +
> + pm_runtime_get_sync(pdata->dev);
> + edid = drm_get_edid(connector, &pdata->aux.ddc);
> + pm_runtime_put_autosuspend(pdata->dev);
I'm 99% sure that the pm_runtime calls here are not needed and can be
removed.. The AUX transfer function handles the pm_runtime_get_sync()
and it also does the "put" with autosuspend so that the whole EDID can
be read without constantly powering the bridge up and down again.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v3 2/3] drm/bridge: ti-sn65dsi86: Implement bridge connector operations
@ 2022-03-10 23:10 ` Doug Anderson
0 siblings, 0 replies; 33+ messages in thread
From: Doug Anderson @ 2022-03-10 23:10 UTC (permalink / raw)
To: Kieran Bingham
Cc: Jernej Skrabec, Neil Armstrong, David Airlie, Jonas Karlman,
LKML, dri-devel, Linux-Renesas, Laurent Pinchart, Robert Foss,
Andrzej Hajda, Sam Ravnborg, Laurent Pinchart
Hi,
On Thu, Mar 10, 2022 at 7:22 AM Kieran Bingham
<kieran.bingham+renesas@ideasonboard.com> wrote:
>
> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>
> Implement the bridge connector-related .get_edid() operation, and report
> the related bridge capabilities and type.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
> Changes since v1:
>
> - The connector .get_modes() operation doesn't rely on EDID anymore,
> __ti_sn_bridge_get_edid() and ti_sn_bridge_get_edid() got merged
> together
> - Fix on top of Sam Ravnborg's DRM_BRIDGE_STATE_OPS
>
> Changes since v2: [Kieran]
> - Only support EDID on DRM_MODE_CONNECTOR_DisplayPort modes.
>
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 93b54fcba8ba..d581c820e5d8 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -1135,10 +1135,24 @@ static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
> pm_runtime_put_sync(pdata->dev);
> }
>
> +static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge,
> + struct drm_connector *connector)
> +{
> + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> + struct edid *edid;
> +
> + pm_runtime_get_sync(pdata->dev);
> + edid = drm_get_edid(connector, &pdata->aux.ddc);
> + pm_runtime_put_autosuspend(pdata->dev);
I'm 99% sure that the pm_runtime calls here are not needed and can be
removed.. The AUX transfer function handles the pm_runtime_get_sync()
and it also does the "put" with autosuspend so that the whole EDID can
be read without constantly powering the bridge up and down again.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v3 1/3] drm/bridge: ti-sn65dsi86: Support DisplayPort (non-eDP) mode
2022-03-10 15:22 ` Kieran Bingham
@ 2022-03-10 23:10 ` Doug Anderson
-1 siblings, 0 replies; 33+ messages in thread
From: Doug Anderson @ 2022-03-10 23:10 UTC (permalink / raw)
To: Kieran Bingham
Cc: dri-devel, Linux-Renesas, Sam Ravnborg, LKML, Andrzej Hajda,
Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman,
Jernej Skrabec, David Airlie, Daniel Vetter, Laurent Pinchart
Hi,
On Thu, Mar 10, 2022 at 7:22 AM Kieran Bingham
<kieran.bingham+renesas@ideasonboard.com> wrote:
>
> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>
> Despite the SN65DSI86 being an eDP bridge, on some systems its output is
> routed to a DisplayPort connector. Enable DisplayPort mode when the next
> component in the display pipeline is detected as a DisplayPort
> connector, and disable eDP features in that case.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Reworked to set bridge type based on the next bridge/connector.
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> --
> Changes since v1/RFC:
> - Rebased on top of "drm/bridge: ti-sn65dsi86: switch to
> devm_drm_of_get_bridge"
> - eDP/DP mode determined from the next bridge connector type.
>
> Changes since v2:
> - Remove setting of Standard DP Scrambler Seed. (It's read-only).
> - Prevent setting DP_EDP_CONFIGURATION_SET in
> ti_sn_bridge_atomic_enable()
> - Use Doug's suggested text for disabling ASSR on DP mode.
>
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 23 ++++++++++++++++++++---
> 1 file changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index c892ecba91c7..93b54fcba8ba 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -62,6 +62,7 @@
> #define SN_LN_ASSIGN_REG 0x59
> #define LN_ASSIGN_WIDTH 2
> #define SN_ENH_FRAME_REG 0x5A
> +#define ASSR_CONTROL BIT(0)
nit that the ASSR_CONTROL define is no longer used.
Other than that, this patch looks fine to me.
Reviewed-by: Douglas Anderson <dianders@chromium.org>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v3 1/3] drm/bridge: ti-sn65dsi86: Support DisplayPort (non-eDP) mode
@ 2022-03-10 23:10 ` Doug Anderson
0 siblings, 0 replies; 33+ messages in thread
From: Doug Anderson @ 2022-03-10 23:10 UTC (permalink / raw)
To: Kieran Bingham
Cc: Jernej Skrabec, Neil Armstrong, David Airlie, Jonas Karlman,
LKML, dri-devel, Linux-Renesas, Laurent Pinchart, Robert Foss,
Andrzej Hajda, Sam Ravnborg, Laurent Pinchart
Hi,
On Thu, Mar 10, 2022 at 7:22 AM Kieran Bingham
<kieran.bingham+renesas@ideasonboard.com> wrote:
>
> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>
> Despite the SN65DSI86 being an eDP bridge, on some systems its output is
> routed to a DisplayPort connector. Enable DisplayPort mode when the next
> component in the display pipeline is detected as a DisplayPort
> connector, and disable eDP features in that case.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Reworked to set bridge type based on the next bridge/connector.
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> --
> Changes since v1/RFC:
> - Rebased on top of "drm/bridge: ti-sn65dsi86: switch to
> devm_drm_of_get_bridge"
> - eDP/DP mode determined from the next bridge connector type.
>
> Changes since v2:
> - Remove setting of Standard DP Scrambler Seed. (It's read-only).
> - Prevent setting DP_EDP_CONFIGURATION_SET in
> ti_sn_bridge_atomic_enable()
> - Use Doug's suggested text for disabling ASSR on DP mode.
>
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 23 ++++++++++++++++++++---
> 1 file changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index c892ecba91c7..93b54fcba8ba 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -62,6 +62,7 @@
> #define SN_LN_ASSIGN_REG 0x59
> #define LN_ASSIGN_WIDTH 2
> #define SN_ENH_FRAME_REG 0x5A
> +#define ASSR_CONTROL BIT(0)
nit that the ASSR_CONTROL define is no longer used.
Other than that, this patch looks fine to me.
Reviewed-by: Douglas Anderson <dianders@chromium.org>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v3 0/3] drm/bridge: ti-sn65dsi86: Support non-eDP DisplayPort connectors
2022-03-10 15:22 ` Kieran Bingham
@ 2022-03-10 23:21 ` Doug Anderson
-1 siblings, 0 replies; 33+ messages in thread
From: Doug Anderson @ 2022-03-10 23:21 UTC (permalink / raw)
To: Kieran Bingham
Cc: Jernej Skrabec, Neil Armstrong, David Airlie, Jonas Karlman,
LKML, dri-devel, Linux-Renesas, Robert Foss, Andrzej Hajda,
Sam Ravnborg, Laurent Pinchart
Hi,
On Thu, Mar 10, 2022 at 7:22 AM Kieran Bingham
<kieran.bingham+renesas@ideasonboard.com> wrote:
>
> Implement support for non eDP connectors on the TI-SN65DSI86 bridge, and
> provide IRQ based hotplug detect to identify when the connector is
> present.
>
> no-hpd is extended to be the default behaviour for non DisplayPort
> connectors.
>
> This series is based upon Sam Ravnborgs and Rob Clarks series [0] to
> support DRM_BRIDGE_STATE_OPS and NO_CONNECTOR support on the SN65DSI86,
> however some extra modifications have been made on the top of Sam's
> series to fix compile breakage and the NO_CONNECTOR support.
This confused me a little bit. As far as I know Rob's series is
abandoned and he's not working on it. I assume that Sam will
eventually re-post his series, but it had unsolved problems and the
bpp solution he had totally didn't work because nobody was setting
"output_bus_cfg.format" [1]. Did you solve that? ...or you're just
going to let your patches sit there and hope that Sam will solve the
problem and re-post his series?
I'll admit I didn't go through your git tree to figure out if you
solved it some way. If you did, I would have assumed you'd have
re-posted his patches in your series w/ the solution...
[1] https://lore.kernel.org/r/CAD=FV=WW6HWLOD9AzTpjwva9UHY=AR+LABEWqJQznz6Nbb4sOw@mail.gmail.com/
-Doug
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v3 0/3] drm/bridge: ti-sn65dsi86: Support non-eDP DisplayPort connectors
@ 2022-03-10 23:21 ` Doug Anderson
0 siblings, 0 replies; 33+ messages in thread
From: Doug Anderson @ 2022-03-10 23:21 UTC (permalink / raw)
To: Kieran Bingham
Cc: dri-devel, Linux-Renesas, Sam Ravnborg, LKML, Andrzej Hajda,
Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman,
Jernej Skrabec, David Airlie, Daniel Vetter
Hi,
On Thu, Mar 10, 2022 at 7:22 AM Kieran Bingham
<kieran.bingham+renesas@ideasonboard.com> wrote:
>
> Implement support for non eDP connectors on the TI-SN65DSI86 bridge, and
> provide IRQ based hotplug detect to identify when the connector is
> present.
>
> no-hpd is extended to be the default behaviour for non DisplayPort
> connectors.
>
> This series is based upon Sam Ravnborgs and Rob Clarks series [0] to
> support DRM_BRIDGE_STATE_OPS and NO_CONNECTOR support on the SN65DSI86,
> however some extra modifications have been made on the top of Sam's
> series to fix compile breakage and the NO_CONNECTOR support.
This confused me a little bit. As far as I know Rob's series is
abandoned and he's not working on it. I assume that Sam will
eventually re-post his series, but it had unsolved problems and the
bpp solution he had totally didn't work because nobody was setting
"output_bus_cfg.format" [1]. Did you solve that? ...or you're just
going to let your patches sit there and hope that Sam will solve the
problem and re-post his series?
I'll admit I didn't go through your git tree to figure out if you
solved it some way. If you did, I would have assumed you'd have
re-posted his patches in your series w/ the solution...
[1] https://lore.kernel.org/r/CAD=FV=WW6HWLOD9AzTpjwva9UHY=AR+LABEWqJQznz6Nbb4sOw@mail.gmail.com/
-Doug
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v3 0/3] drm/bridge: ti-sn65dsi86: Support non-eDP DisplayPort connectors
2022-03-10 23:21 ` Doug Anderson
(?)
@ 2022-03-11 5:33 ` Kieran Bingham
-1 siblings, 0 replies; 33+ messages in thread
From: Kieran Bingham @ 2022-03-11 5:33 UTC (permalink / raw)
To: Doug Anderson, Sam Ravnborg
Cc: Jonas Karlman, David Airlie, Robert Foss, Neil Armstrong, LKML,
dri-devel, Linux-Renesas, Jernej Skrabec, Andrzej Hajda,
Sam Ravnborg, Laurent Pinchart
Hi Doug, Sam,
Quoting Doug Anderson (2022-03-10 23:21:38)
> Hi,
>
> On Thu, Mar 10, 2022 at 7:22 AM Kieran Bingham
> <kieran.bingham+renesas@ideasonboard.com> wrote:
> >
> > Implement support for non eDP connectors on the TI-SN65DSI86 bridge, and
> > provide IRQ based hotplug detect to identify when the connector is
> > present.
> >
> > no-hpd is extended to be the default behaviour for non DisplayPort
> > connectors.
> >
> > This series is based upon Sam Ravnborgs and Rob Clarks series [0] to
> > support DRM_BRIDGE_STATE_OPS and NO_CONNECTOR support on the SN65DSI86,
> > however some extra modifications have been made on the top of Sam's
> > series to fix compile breakage and the NO_CONNECTOR support.
>
> This confused me a little bit. As far as I know Rob's series is
> abandoned and he's not working on it. I assume that Sam will
> eventually re-post his series, but it had unsolved problems and the
> bpp solution he had totally didn't work because nobody was setting
> "output_bus_cfg.format" [1]. Did you solve that? ...or you're just
> going to let your patches sit there and hope that Sam will solve the
> problem and re-post his series?
I applied Sam's series, and fixed it to work for me. It looked like
Rob's patch had been rolled into Sam's series, and I didn't take
ownership of Sam's series, as I assume he'll continue on it, but I
haven't asked or heard either way yet. Sam's series is only from
February, so I would not presume to consider that it is abandoned yet.
The changes I made have either already been highlighted by the build
bots on Sam's series, or I have replied to his series with the fixes I
made.
> I'll admit I didn't go through your git tree to figure out if you
> solved it some way. If you did, I would have assumed you'd have
> re-posted his patches in your series w/ the solution...
My changes to his series are on my branch as separate squash: commits so
they're easy to see or take if Sam wants to collect the fixes, but
there's nothing complex there, that isn't easily solved locally.
If Sam's series is abandoned, then I guess my 'Implement SN65DSI86 IRQ
HPD' patch will now have a 11:1 ratio of patches that I have to take
custodianship of vs author myself ;-) (And even most of the work I have
done has been squashed into Laurent's patches already)
Sam, What is your plan on your series at [1], are you still actively
planning to work on it?
--
Kieran
> [1] https://lore.kernel.org/r/CAD=FV=WW6HWLOD9AzTpjwva9UHY=AR+LABEWqJQznz6Nbb4sOw@mail.gmail.com/
>
> -Doug
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v3 3/3] drm/bridge: ti-sn65dsi86: Support hotplug detection
2022-03-10 23:10 ` Doug Anderson
@ 2022-03-11 5:47 ` Kieran Bingham
-1 siblings, 0 replies; 33+ messages in thread
From: Kieran Bingham @ 2022-03-11 5:47 UTC (permalink / raw)
To: Doug Anderson
Cc: dri-devel, Linux-Renesas, Sam Ravnborg, LKML, Andrzej Hajda,
Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman,
Jernej Skrabec, David Airlie, Daniel Vetter, Laurent Pinchart
Hi Doug,
Quoting Doug Anderson (2022-03-10 23:10:12)
> Hi,
>
> On Thu, Mar 10, 2022 at 7:22 AM Kieran Bingham
> <kieran.bingham+renesas@ideasonboard.com> wrote:
> >
> > @@ -1135,6 +1161,36 @@ static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
> > pm_runtime_put_sync(pdata->dev);
> > }
> >
> > +static enum drm_connector_status ti_sn_bridge_detect(struct drm_bridge *bridge)
> > +{
> > + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> > + int val;
> > +
> > + regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val);
>
> Don't you need a pm_runtime_get_sync() before this and a
> put_autosuspend() after? The "detect" will be used in the yes-HPD but
> no-IRQ case, right? In that case there's nobody holding the pm_runtime
> reference.
Hrm ... I'll have to dig on this a bit. The polling is done by the DRM
core, so indeed I suspect it could be done outside of a context that
holds the pm runtime reference.
Equally a get and put on the reference doesn't hurt even if it's already
taken, so perhaps it's best to add it, but I'll try to confirm it's
requirement first.
> Also, a nit that it'd be great if you error checked the regmap_read().
> I know this driver isn't very good about it, but it's probably
> something to get better. i2c transactions can fail. I guess another
> alternative would be to init "val" to 0...
It's a good point indeed. If we can't read the device we should return
disconnected.
>
>
> > + return val & HPD_DEBOUNCED_STATE ? connector_status_connected
> > + : connector_status_disconnected;
> > +}
> > +
> > +static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
> > +{
> > + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> > +
> > + /* The device must remain active for HPD to function */
> > + pm_runtime_get_sync(pdata->dev);
> > + regmap_write(pdata->regmap, SN_IRQ_HPD_REG,
> > + IRQ_HPD_EN | IRQ_HPD_INSERTION_EN |
> > + IRQ_HPD_REMOVAL_EN | IRQ_HPD_REPLUG_EN);
> > +}
> > +
> > +static void ti_sn_bridge_hpd_disable(struct drm_bridge *bridge)
> > +{
> > + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> > +
> > + regmap_write(pdata->regmap, SN_IRQ_HPD_REG, 0);
> > + pm_runtime_put_autosuspend(pdata->dev);
>
> Before doing the pm_runtime_put_autosuspend() it feels like you should
> ensure that the interrupt has finished. Otherwise we could be midway
> through processing an interrupt and the pm_runtime reference could go
> away, right? Maybe we just disable the irq which I think will wait for
> anything outstanding to finish?
Should the IRQ handler also call pm_runtime_get/put then?
> > @@ -1223,6 +1282,34 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
> > return 0;
> > }
> >
> > +static irqreturn_t ti_sn65dsi86_irq_handler(int irq, void *arg)
> > +{
> > + struct ti_sn65dsi86 *pdata = arg;
> > + int ret;
> > + unsigned int hpd;
> > +
> > + ret = regmap_read(pdata->regmap, SN_IRQ_HPD_STATUS_REG, &hpd);
> > + if (ret || !hpd)
> > + return IRQ_NONE;
> > +
> > + if (hpd & IRQ_HPD_INSERTION_STATUS)
> > + drm_bridge_hpd_notify(&pdata->bridge, connector_status_connected);
> > +
> > + if (hpd & IRQ_HPD_REMOVAL_STATUS)
> > + drm_bridge_hpd_notify(&pdata->bridge, connector_status_disconnected);
> > +
> > + /* When replugged, ensure we trigger a detect to update the display */
> > + if (hpd & IRQ_HPD_REPLUG_STATUS)
> > + drm_bridge_hpd_notify(&pdata->bridge, connector_status_disconnected);
>
> How does the ordering work here if _both_ insertion and removal are
> asserted? Is that somehow not possible? Should this be "else if" type
> statements then, or give a warn if more than one bit is set, or ... ?
As I understand it, that would trigger a REPLUG IRQ. However this is one
part I quite disliked about the drm_bridge_hpd_notify. The values here
are not taken as the hardware state anyway. A call to drm_bridge_hpd_notify will
trigger a call on the detect function so a further read will occur to
determine the current state using the same function as is used with
polling.
The IRQ handler only cuts out the polling as far as I see.
> > + /* reset the status registers */
> > + regmap_write(pdata->regmap, SN_IRQ_HPD_STATUS_REG,
> > + IRQ_HPD_STATUS | IRQ_HPD_INSERTION_STATUS |
> > + IRQ_HPD_REMOVAL_STATUS | IRQ_HPD_REPLUG_STATUS);
>
> IMO this regmap_write() belongs right after the read and should be
> based on what you read--you shouldn't just clear all of them. AKA:
>
> a) Read to see what interrupt are asserted.
> b) Ack the interrupts that you saw asserted.
> c) Process the interrupts that you saw asserted.
>
> If you process before acking then you can miss interrupts (in other
> words if you do "a" then "c" then "b" then you can miss interrupts
> that come in after "b" but before "c".
Agreed, I'll respin.
> > @@ -1247,9 +1342,29 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
> > pdata->bridge.type = pdata->next_bridge->type == DRM_MODE_CONNECTOR_DisplayPort
> > ? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;
> >
> > - if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
> > + if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort) {
> > pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
> >
> > + if (!pdata->no_hpd)
> > + pdata->bridge.ops |= DRM_BRIDGE_OP_DETECT;
> > + }
> > +
> > + if (!pdata->no_hpd && pdata->irq > 0) {
> > + dev_err(pdata->dev, "registering IRQ %d\n", pdata->irq);
> > +
> > + ret = devm_request_threaded_irq(pdata->dev, pdata->irq, NULL,
> > + ti_sn65dsi86_irq_handler,
> > + IRQF_ONESHOT, "sn65dsi86-irq",
> > + pdata);
> > + if (ret)
> > + return dev_err_probe(pdata->dev, ret,
> > + "Failed to register DP interrupt\n");
> > +
> > + /* Enable IRQ based HPD */
> > + regmap_write(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN);
>
> Why not put the above regmap_write() in the ti_sn_bridge_hpd_enable() call?
I assumed the IRQ handler may get used by other non-HPD events. Which is
also why it was originally registered in the main probe(). HPD is just
one feature of the interrupts. Of course it's only used for HPD now
though. I guess I could have solved the bridge dependency by splitting
the IRQ handler to have a dedicated HPD handler function which would
return if the bridge wasn't initialised, but went with the deferred
registration of the handler.
I can move this and then leave it to anyone else implementing further
IRQ features to refactor if needed.
>
> -Doug
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v3 3/3] drm/bridge: ti-sn65dsi86: Support hotplug detection
@ 2022-03-11 5:47 ` Kieran Bingham
0 siblings, 0 replies; 33+ messages in thread
From: Kieran Bingham @ 2022-03-11 5:47 UTC (permalink / raw)
To: Doug Anderson
Cc: Jernej Skrabec, Neil Armstrong, David Airlie, Jonas Karlman,
LKML, dri-devel, Linux-Renesas, Laurent Pinchart, Robert Foss,
Andrzej Hajda, Sam Ravnborg, Laurent Pinchart
Hi Doug,
Quoting Doug Anderson (2022-03-10 23:10:12)
> Hi,
>
> On Thu, Mar 10, 2022 at 7:22 AM Kieran Bingham
> <kieran.bingham+renesas@ideasonboard.com> wrote:
> >
> > @@ -1135,6 +1161,36 @@ static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
> > pm_runtime_put_sync(pdata->dev);
> > }
> >
> > +static enum drm_connector_status ti_sn_bridge_detect(struct drm_bridge *bridge)
> > +{
> > + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> > + int val;
> > +
> > + regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val);
>
> Don't you need a pm_runtime_get_sync() before this and a
> put_autosuspend() after? The "detect" will be used in the yes-HPD but
> no-IRQ case, right? In that case there's nobody holding the pm_runtime
> reference.
Hrm ... I'll have to dig on this a bit. The polling is done by the DRM
core, so indeed I suspect it could be done outside of a context that
holds the pm runtime reference.
Equally a get and put on the reference doesn't hurt even if it's already
taken, so perhaps it's best to add it, but I'll try to confirm it's
requirement first.
> Also, a nit that it'd be great if you error checked the regmap_read().
> I know this driver isn't very good about it, but it's probably
> something to get better. i2c transactions can fail. I guess another
> alternative would be to init "val" to 0...
It's a good point indeed. If we can't read the device we should return
disconnected.
>
>
> > + return val & HPD_DEBOUNCED_STATE ? connector_status_connected
> > + : connector_status_disconnected;
> > +}
> > +
> > +static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
> > +{
> > + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> > +
> > + /* The device must remain active for HPD to function */
> > + pm_runtime_get_sync(pdata->dev);
> > + regmap_write(pdata->regmap, SN_IRQ_HPD_REG,
> > + IRQ_HPD_EN | IRQ_HPD_INSERTION_EN |
> > + IRQ_HPD_REMOVAL_EN | IRQ_HPD_REPLUG_EN);
> > +}
> > +
> > +static void ti_sn_bridge_hpd_disable(struct drm_bridge *bridge)
> > +{
> > + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> > +
> > + regmap_write(pdata->regmap, SN_IRQ_HPD_REG, 0);
> > + pm_runtime_put_autosuspend(pdata->dev);
>
> Before doing the pm_runtime_put_autosuspend() it feels like you should
> ensure that the interrupt has finished. Otherwise we could be midway
> through processing an interrupt and the pm_runtime reference could go
> away, right? Maybe we just disable the irq which I think will wait for
> anything outstanding to finish?
Should the IRQ handler also call pm_runtime_get/put then?
> > @@ -1223,6 +1282,34 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
> > return 0;
> > }
> >
> > +static irqreturn_t ti_sn65dsi86_irq_handler(int irq, void *arg)
> > +{
> > + struct ti_sn65dsi86 *pdata = arg;
> > + int ret;
> > + unsigned int hpd;
> > +
> > + ret = regmap_read(pdata->regmap, SN_IRQ_HPD_STATUS_REG, &hpd);
> > + if (ret || !hpd)
> > + return IRQ_NONE;
> > +
> > + if (hpd & IRQ_HPD_INSERTION_STATUS)
> > + drm_bridge_hpd_notify(&pdata->bridge, connector_status_connected);
> > +
> > + if (hpd & IRQ_HPD_REMOVAL_STATUS)
> > + drm_bridge_hpd_notify(&pdata->bridge, connector_status_disconnected);
> > +
> > + /* When replugged, ensure we trigger a detect to update the display */
> > + if (hpd & IRQ_HPD_REPLUG_STATUS)
> > + drm_bridge_hpd_notify(&pdata->bridge, connector_status_disconnected);
>
> How does the ordering work here if _both_ insertion and removal are
> asserted? Is that somehow not possible? Should this be "else if" type
> statements then, or give a warn if more than one bit is set, or ... ?
As I understand it, that would trigger a REPLUG IRQ. However this is one
part I quite disliked about the drm_bridge_hpd_notify. The values here
are not taken as the hardware state anyway. A call to drm_bridge_hpd_notify will
trigger a call on the detect function so a further read will occur to
determine the current state using the same function as is used with
polling.
The IRQ handler only cuts out the polling as far as I see.
> > + /* reset the status registers */
> > + regmap_write(pdata->regmap, SN_IRQ_HPD_STATUS_REG,
> > + IRQ_HPD_STATUS | IRQ_HPD_INSERTION_STATUS |
> > + IRQ_HPD_REMOVAL_STATUS | IRQ_HPD_REPLUG_STATUS);
>
> IMO this regmap_write() belongs right after the read and should be
> based on what you read--you shouldn't just clear all of them. AKA:
>
> a) Read to see what interrupt are asserted.
> b) Ack the interrupts that you saw asserted.
> c) Process the interrupts that you saw asserted.
>
> If you process before acking then you can miss interrupts (in other
> words if you do "a" then "c" then "b" then you can miss interrupts
> that come in after "b" but before "c".
Agreed, I'll respin.
> > @@ -1247,9 +1342,29 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
> > pdata->bridge.type = pdata->next_bridge->type == DRM_MODE_CONNECTOR_DisplayPort
> > ? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;
> >
> > - if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
> > + if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort) {
> > pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
> >
> > + if (!pdata->no_hpd)
> > + pdata->bridge.ops |= DRM_BRIDGE_OP_DETECT;
> > + }
> > +
> > + if (!pdata->no_hpd && pdata->irq > 0) {
> > + dev_err(pdata->dev, "registering IRQ %d\n", pdata->irq);
> > +
> > + ret = devm_request_threaded_irq(pdata->dev, pdata->irq, NULL,
> > + ti_sn65dsi86_irq_handler,
> > + IRQF_ONESHOT, "sn65dsi86-irq",
> > + pdata);
> > + if (ret)
> > + return dev_err_probe(pdata->dev, ret,
> > + "Failed to register DP interrupt\n");
> > +
> > + /* Enable IRQ based HPD */
> > + regmap_write(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN);
>
> Why not put the above regmap_write() in the ti_sn_bridge_hpd_enable() call?
I assumed the IRQ handler may get used by other non-HPD events. Which is
also why it was originally registered in the main probe(). HPD is just
one feature of the interrupts. Of course it's only used for HPD now
though. I guess I could have solved the bridge dependency by splitting
the IRQ handler to have a dedicated HPD handler function which would
return if the bridge wasn't initialised, but went with the deferred
registration of the handler.
I can move this and then leave it to anyone else implementing further
IRQ features to refactor if needed.
>
> -Doug
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v3 3/3] drm/bridge: ti-sn65dsi86: Support hotplug detection
2022-03-11 5:47 ` Kieran Bingham
@ 2022-03-11 15:24 ` Doug Anderson
-1 siblings, 0 replies; 33+ messages in thread
From: Doug Anderson @ 2022-03-11 15:24 UTC (permalink / raw)
To: Kieran Bingham
Cc: dri-devel, Linux-Renesas, Sam Ravnborg, LKML, Andrzej Hajda,
Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman,
Jernej Skrabec, David Airlie, Daniel Vetter, Laurent Pinchart
Hi,
On Thu, Mar 10, 2022 at 9:47 PM Kieran Bingham
<kieran.bingham+renesas@ideasonboard.com> wrote:
>
> > > +static void ti_sn_bridge_hpd_disable(struct drm_bridge *bridge)
> > > +{
> > > + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> > > +
> > > + regmap_write(pdata->regmap, SN_IRQ_HPD_REG, 0);
> > > + pm_runtime_put_autosuspend(pdata->dev);
> >
> > Before doing the pm_runtime_put_autosuspend() it feels like you should
> > ensure that the interrupt has finished. Otherwise we could be midway
> > through processing an interrupt and the pm_runtime reference could go
> > away, right? Maybe we just disable the irq which I think will wait for
> > anything outstanding to finish?
>
> Should the IRQ handler also call pm_runtime_get/put then?
I thought about that, but I suspect it's cleaner to disable the IRQ
handler (and block waiting for it to finish if it was running). That
will ensure that the core isn't notified about HPD after HPD was
disabled. Once you do that then there's no need to get/put in the irq
handler since we always hold a pm_runtime reference when the IRQ
handler is enabled.
> > > @@ -1247,9 +1342,29 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
> > > pdata->bridge.type = pdata->next_bridge->type == DRM_MODE_CONNECTOR_DisplayPort
> > > ? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;
> > >
> > > - if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
> > > + if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort) {
> > > pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
> > >
> > > + if (!pdata->no_hpd)
> > > + pdata->bridge.ops |= DRM_BRIDGE_OP_DETECT;
> > > + }
> > > +
> > > + if (!pdata->no_hpd && pdata->irq > 0) {
> > > + dev_err(pdata->dev, "registering IRQ %d\n", pdata->irq);
> > > +
> > > + ret = devm_request_threaded_irq(pdata->dev, pdata->irq, NULL,
> > > + ti_sn65dsi86_irq_handler,
> > > + IRQF_ONESHOT, "sn65dsi86-irq",
> > > + pdata);
> > > + if (ret)
> > > + return dev_err_probe(pdata->dev, ret,
> > > + "Failed to register DP interrupt\n");
> > > +
> > > + /* Enable IRQ based HPD */
> > > + regmap_write(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN);
> >
> > Why not put the above regmap_write() in the ti_sn_bridge_hpd_enable() call?
>
> I assumed the IRQ handler may get used by other non-HPD events. Which is
> also why it was originally registered in the main probe(). HPD is just
> one feature of the interrupts. Of course it's only used for HPD now
> though. I guess I could have solved the bridge dependency by splitting
> the IRQ handler to have a dedicated HPD handler function which would
> return if the bridge wasn't initialised, but went with the deferred
> registration of the handler.
>
> I can move this and then leave it to anyone else implementing further
> IRQ features to refactor if needed.
Sounds good. In general the pm_runtime_get reference need to go with
the IRQ enabling, so if someone else finds a non-HPD need then they'll
have to move that too.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v3 3/3] drm/bridge: ti-sn65dsi86: Support hotplug detection
@ 2022-03-11 15:24 ` Doug Anderson
0 siblings, 0 replies; 33+ messages in thread
From: Doug Anderson @ 2022-03-11 15:24 UTC (permalink / raw)
To: Kieran Bingham
Cc: Jernej Skrabec, Neil Armstrong, David Airlie, Jonas Karlman,
LKML, dri-devel, Linux-Renesas, Laurent Pinchart, Robert Foss,
Andrzej Hajda, Sam Ravnborg, Laurent Pinchart
Hi,
On Thu, Mar 10, 2022 at 9:47 PM Kieran Bingham
<kieran.bingham+renesas@ideasonboard.com> wrote:
>
> > > +static void ti_sn_bridge_hpd_disable(struct drm_bridge *bridge)
> > > +{
> > > + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> > > +
> > > + regmap_write(pdata->regmap, SN_IRQ_HPD_REG, 0);
> > > + pm_runtime_put_autosuspend(pdata->dev);
> >
> > Before doing the pm_runtime_put_autosuspend() it feels like you should
> > ensure that the interrupt has finished. Otherwise we could be midway
> > through processing an interrupt and the pm_runtime reference could go
> > away, right? Maybe we just disable the irq which I think will wait for
> > anything outstanding to finish?
>
> Should the IRQ handler also call pm_runtime_get/put then?
I thought about that, but I suspect it's cleaner to disable the IRQ
handler (and block waiting for it to finish if it was running). That
will ensure that the core isn't notified about HPD after HPD was
disabled. Once you do that then there's no need to get/put in the irq
handler since we always hold a pm_runtime reference when the IRQ
handler is enabled.
> > > @@ -1247,9 +1342,29 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
> > > pdata->bridge.type = pdata->next_bridge->type == DRM_MODE_CONNECTOR_DisplayPort
> > > ? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;
> > >
> > > - if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
> > > + if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort) {
> > > pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
> > >
> > > + if (!pdata->no_hpd)
> > > + pdata->bridge.ops |= DRM_BRIDGE_OP_DETECT;
> > > + }
> > > +
> > > + if (!pdata->no_hpd && pdata->irq > 0) {
> > > + dev_err(pdata->dev, "registering IRQ %d\n", pdata->irq);
> > > +
> > > + ret = devm_request_threaded_irq(pdata->dev, pdata->irq, NULL,
> > > + ti_sn65dsi86_irq_handler,
> > > + IRQF_ONESHOT, "sn65dsi86-irq",
> > > + pdata);
> > > + if (ret)
> > > + return dev_err_probe(pdata->dev, ret,
> > > + "Failed to register DP interrupt\n");
> > > +
> > > + /* Enable IRQ based HPD */
> > > + regmap_write(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN);
> >
> > Why not put the above regmap_write() in the ti_sn_bridge_hpd_enable() call?
>
> I assumed the IRQ handler may get used by other non-HPD events. Which is
> also why it was originally registered in the main probe(). HPD is just
> one feature of the interrupts. Of course it's only used for HPD now
> though. I guess I could have solved the bridge dependency by splitting
> the IRQ handler to have a dedicated HPD handler function which would
> return if the bridge wasn't initialised, but went with the deferred
> registration of the handler.
>
> I can move this and then leave it to anyone else implementing further
> IRQ features to refactor if needed.
Sounds good. In general the pm_runtime_get reference need to go with
the IRQ enabling, so if someone else finds a non-HPD need then they'll
have to move that too.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v3 2/3] drm/bridge: ti-sn65dsi86: Implement bridge connector operations
2022-03-10 23:10 ` Doug Anderson
@ 2022-03-16 23:24 ` Kieran Bingham
-1 siblings, 0 replies; 33+ messages in thread
From: Kieran Bingham @ 2022-03-16 23:24 UTC (permalink / raw)
To: Doug Anderson
Cc: dri-devel, Linux-Renesas, Sam Ravnborg, LKML, Andrzej Hajda,
Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman,
Jernej Skrabec, David Airlie, Daniel Vetter, Laurent Pinchart
Hi Doug,
Quoting Doug Anderson (2022-03-10 23:10:20)
> Hi,
>
> On Thu, Mar 10, 2022 at 7:22 AM Kieran Bingham
> <kieran.bingham+renesas@ideasonboard.com> wrote:
> >
> > From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> >
> > Implement the bridge connector-related .get_edid() operation, and report
> > the related bridge capabilities and type.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > ---
> > Changes since v1:
> >
> > - The connector .get_modes() operation doesn't rely on EDID anymore,
> > __ti_sn_bridge_get_edid() and ti_sn_bridge_get_edid() got merged
> > together
> > - Fix on top of Sam Ravnborg's DRM_BRIDGE_STATE_OPS
> >
> > Changes since v2: [Kieran]
> > - Only support EDID on DRM_MODE_CONNECTOR_DisplayPort modes.
> >
> > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 17 +++++++++++++++++
> > 1 file changed, 17 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index 93b54fcba8ba..d581c820e5d8 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > @@ -1135,10 +1135,24 @@ static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
> > pm_runtime_put_sync(pdata->dev);
> > }
> >
> > +static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge,
> > + struct drm_connector *connector)
> > +{
> > + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> > + struct edid *edid;
> > +
> > + pm_runtime_get_sync(pdata->dev);
> > + edid = drm_get_edid(connector, &pdata->aux.ddc);
> > + pm_runtime_put_autosuspend(pdata->dev);
>
> I'm 99% sure that the pm_runtime calls here are not needed and can be
> removed.. The AUX transfer function handles the pm_runtime_get_sync()
> and it also does the "put" with autosuspend so that the whole EDID can
> be read without constantly powering the bridge up and down again.
Yes, digging through I agree - It does look like this may be the case.
ti_sn_aux_transfer() certainly looks like it handles the pm_runtime_
calls, and drm_get_edid() looks like it goes through there from the core
using the standard i2c interface, with nothing else expected to touch
the hw between.
So that more or less simplifies this function to just
return drm_get_edid(connector, &pdata->aux.ddc);
Thanks
--
Kieran
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v3 2/3] drm/bridge: ti-sn65dsi86: Implement bridge connector operations
@ 2022-03-16 23:24 ` Kieran Bingham
0 siblings, 0 replies; 33+ messages in thread
From: Kieran Bingham @ 2022-03-16 23:24 UTC (permalink / raw)
To: Doug Anderson
Cc: Jernej Skrabec, Neil Armstrong, David Airlie, Jonas Karlman,
LKML, dri-devel, Linux-Renesas, Laurent Pinchart, Robert Foss,
Andrzej Hajda, Sam Ravnborg, Laurent Pinchart
Hi Doug,
Quoting Doug Anderson (2022-03-10 23:10:20)
> Hi,
>
> On Thu, Mar 10, 2022 at 7:22 AM Kieran Bingham
> <kieran.bingham+renesas@ideasonboard.com> wrote:
> >
> > From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> >
> > Implement the bridge connector-related .get_edid() operation, and report
> > the related bridge capabilities and type.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > ---
> > Changes since v1:
> >
> > - The connector .get_modes() operation doesn't rely on EDID anymore,
> > __ti_sn_bridge_get_edid() and ti_sn_bridge_get_edid() got merged
> > together
> > - Fix on top of Sam Ravnborg's DRM_BRIDGE_STATE_OPS
> >
> > Changes since v2: [Kieran]
> > - Only support EDID on DRM_MODE_CONNECTOR_DisplayPort modes.
> >
> > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 17 +++++++++++++++++
> > 1 file changed, 17 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index 93b54fcba8ba..d581c820e5d8 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > @@ -1135,10 +1135,24 @@ static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
> > pm_runtime_put_sync(pdata->dev);
> > }
> >
> > +static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge,
> > + struct drm_connector *connector)
> > +{
> > + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> > + struct edid *edid;
> > +
> > + pm_runtime_get_sync(pdata->dev);
> > + edid = drm_get_edid(connector, &pdata->aux.ddc);
> > + pm_runtime_put_autosuspend(pdata->dev);
>
> I'm 99% sure that the pm_runtime calls here are not needed and can be
> removed.. The AUX transfer function handles the pm_runtime_get_sync()
> and it also does the "put" with autosuspend so that the whole EDID can
> be read without constantly powering the bridge up and down again.
Yes, digging through I agree - It does look like this may be the case.
ti_sn_aux_transfer() certainly looks like it handles the pm_runtime_
calls, and drm_get_edid() looks like it goes through there from the core
using the standard i2c interface, with nothing else expected to touch
the hw between.
So that more or less simplifies this function to just
return drm_get_edid(connector, &pdata->aux.ddc);
Thanks
--
Kieran
^ permalink raw reply [flat|nested] 33+ messages in thread
end of thread, other threads:[~2022-03-16 23:24 UTC | newest]
Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-10 15:22 [PATCH v3 0/3] drm/bridge: ti-sn65dsi86: Support non-eDP DisplayPort connectors Kieran Bingham
2022-03-10 15:22 ` Kieran Bingham
2022-03-10 15:22 ` [PATCH v3 1/3] drm/bridge: ti-sn65dsi86: Support DisplayPort (non-eDP) mode Kieran Bingham
2022-03-10 15:22 ` Kieran Bingham
2022-03-10 16:18 ` Laurent Pinchart
2022-03-10 16:18 ` Laurent Pinchart
2022-03-10 23:10 ` Doug Anderson
2022-03-10 23:10 ` Doug Anderson
2022-03-10 15:22 ` [PATCH v3 2/3] drm/bridge: ti-sn65dsi86: Implement bridge connector operations Kieran Bingham
2022-03-10 15:22 ` Kieran Bingham
2022-03-10 16:23 ` Laurent Pinchart
2022-03-10 16:23 ` Laurent Pinchart
2022-03-10 23:10 ` Doug Anderson
2022-03-10 23:10 ` Doug Anderson
2022-03-16 23:24 ` Kieran Bingham
2022-03-16 23:24 ` Kieran Bingham
2022-03-10 15:22 ` [PATCH v3 3/3] drm/bridge: ti-sn65dsi86: Support hotplug detection Kieran Bingham
2022-03-10 15:22 ` Kieran Bingham
2022-03-10 15:26 ` Kieran Bingham
2022-03-10 15:26 ` Kieran Bingham
2022-03-10 16:42 ` Laurent Pinchart
2022-03-10 16:42 ` Laurent Pinchart
2022-03-10 17:39 ` Kieran Bingham
2022-03-10 17:39 ` Kieran Bingham
2022-03-10 23:10 ` Doug Anderson
2022-03-10 23:10 ` Doug Anderson
2022-03-11 5:47 ` Kieran Bingham
2022-03-11 5:47 ` Kieran Bingham
2022-03-11 15:24 ` Doug Anderson
2022-03-11 15:24 ` Doug Anderson
2022-03-10 23:21 ` [PATCH v3 0/3] drm/bridge: ti-sn65dsi86: Support non-eDP DisplayPort connectors Doug Anderson
2022-03-10 23:21 ` Doug Anderson
2022-03-11 5:33 ` Kieran Bingham
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.