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 on top of José Expósito's patch [0] "drm/bridge: ti-sn65dsi86: switch to devm_drm_of_get_bridge" and Nikita Yushchenko's patch [1] "drm/bridge_connector: enable HPD by default if supported".
[0] https://lore.kernel.org/all/20220228183955.25508-1-jose.exposito89@gmail.com... [1] https://lore.kernel.org/all/20211225063151.2110878-1-nikita.yoush@cogentembe...
Kieran Bingham (1): drm/bridge: ti-sn65dsi86: Support hotplug detection
Laurent Pinchart (3): drm/bridge: ti-sn65dsi86: Implement bridge connector operations drm/bridge: ti-sn65dsi86: Make connector creation optional drm/bridge: ti-sn65dsi86: Support DisplayPort (non-eDP) mode
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 178 ++++++++++++++++++++++---- 1 file changed, 156 insertions(+), 22 deletions(-)
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 Reviewed-by: Stephen Boyd swboyd@chromium.org Reviewed-by: Douglas Anderson dianders@chromium.org 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
Notes from Kieran:
RB Tags collected from: https://lore.kernel.org/all/20210322030128.2283-9-laurent.pinchart+renesas@i...
However this was over a year ago, so let me know if other patches now superceed this one or otherwise invalidate this update.
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index c55848588123..ffb6c04f6c46 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -1154,6 +1154,19 @@ static void ti_sn_bridge_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, @@ -1162,6 +1175,7 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = { .enable = ti_sn_bridge_enable, .disable = ti_sn_bridge_disable, .post_disable = ti_sn_bridge_post_disable, + .get_edid = ti_sn_bridge_get_edid, };
static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata, @@ -1248,6 +1262,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.ops = DRM_BRIDGE_OP_EDID; + pdata->bridge.type = DRM_MODE_CONNECTOR_eDP;
drm_bridge_add(&pdata->bridge);
Hi,
On Mon, Mar 7, 2022 at 10:00 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 Reviewed-by: Stephen Boyd swboyd@chromium.org Reviewed-by: Douglas Anderson dianders@chromium.org 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
Notes from Kieran:
RB Tags collected from: https://lore.kernel.org/all/20210322030128.2283-9-laurent.pinchart+renesas@i...
However this was over a year ago, so let me know if other patches now superceed this one or otherwise invalidate this update.
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index c55848588123..ffb6c04f6c46 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -1154,6 +1154,19 @@ static void ti_sn_bridge_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, @@ -1162,6 +1175,7 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = { .enable = ti_sn_bridge_enable, .disable = ti_sn_bridge_disable, .post_disable = ti_sn_bridge_post_disable,
.get_edid = ti_sn_bridge_get_edid,};
static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata, @@ -1248,6 +1262,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.ops = DRM_BRIDGE_OP_EDID;pdata->bridge.type = DRM_MODE_CONNECTOR_eDP;
This doesn't look right to me. In the eDP case the EDID reading is driven by the panel.
-Doug
Hi Doug,
Quoting Doug Anderson (2022-03-07 19:52:08)
Hi,
On Mon, Mar 7, 2022 at 10:00 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 Reviewed-by: Stephen Boyd swboyd@chromium.org Reviewed-by: Douglas Anderson dianders@chromium.org 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
Notes from Kieran:
RB Tags collected from: https://lore.kernel.org/all/20210322030128.2283-9-laurent.pinchart+renesas@i...
However this was over a year ago, so let me know if other patches now superceed this one or otherwise invalidate this update.
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index c55848588123..ffb6c04f6c46 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -1154,6 +1154,19 @@ static void ti_sn_bridge_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, @@ -1162,6 +1175,7 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = { .enable = ti_sn_bridge_enable, .disable = ti_sn_bridge_disable, .post_disable = ti_sn_bridge_post_disable,
.get_edid = ti_sn_bridge_get_edid,};
static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata, @@ -1248,6 +1262,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.ops = DRM_BRIDGE_OP_EDID;pdata->bridge.type = DRM_MODE_CONNECTOR_eDP;This doesn't look right to me. In the eDP case the EDID reading is driven by the panel.
Now that I have the optional connector working based on Sam's series I think this is the last issue to solve before reposting the DP/HPD support.
Are you saying that the bridge.ops should only set DRM_BRIDGE_OP_EDID when pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort?
-- Regards
Kieran
-Doug
Hi,
On Thu, Mar 10, 2022 at 3:43 AM Kieran Bingham kieran.bingham+renesas@ideasonboard.com wrote:
Hi Doug,
Quoting Doug Anderson (2022-03-07 19:52:08)
Hi,
On Mon, Mar 7, 2022 at 10:00 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 Reviewed-by: Stephen Boyd swboyd@chromium.org Reviewed-by: Douglas Anderson dianders@chromium.org 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
Notes from Kieran:
RB Tags collected from: https://lore.kernel.org/all/20210322030128.2283-9-laurent.pinchart+renesas@i...
However this was over a year ago, so let me know if other patches now superceed this one or otherwise invalidate this update.
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index c55848588123..ffb6c04f6c46 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -1154,6 +1154,19 @@ static void ti_sn_bridge_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, @@ -1162,6 +1175,7 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = { .enable = ti_sn_bridge_enable, .disable = ti_sn_bridge_disable, .post_disable = ti_sn_bridge_post_disable,
.get_edid = ti_sn_bridge_get_edid,};
static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata, @@ -1248,6 +1262,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.ops = DRM_BRIDGE_OP_EDID;pdata->bridge.type = DRM_MODE_CONNECTOR_eDP;This doesn't look right to me. In the eDP case the EDID reading is driven by the panel.
Now that I have the optional connector working based on Sam's series I think this is the last issue to solve before reposting the DP/HPD support.
Are you saying that the bridge.ops should only set DRM_BRIDGE_OP_EDID when pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort?
Yes.
-Doug
From: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com
Now that the driver supports the connector-related bridge operations, make the connector creation optional. This enables usage of the sn65dsi86 with the DRM bridge connector helper.
Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com Signed-off-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com
--- Changes since v1: - None
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index ffb6c04f6c46..29f5f7123ed9 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -745,11 +745,6 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge, struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge); int ret;
- if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) { - DRM_ERROR("Fix bridge driver to make connector optional!"); - return -EINVAL; - } - pdata->aux.drm_dev = bridge->dev; ret = drm_dp_aux_register(&pdata->aux); if (ret < 0) { @@ -757,12 +752,14 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge, return ret; }
- ret = ti_sn_bridge_connector_init(pdata); - if (ret < 0) - goto err_conn_init; + if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) { + ret = ti_sn_bridge_connector_init(pdata); + if (ret < 0) + goto err_conn_init;
- /* We never want the next bridge to *also* create a connector: */ - flags |= DRM_BRIDGE_ATTACH_NO_CONNECTOR; + /* We never want the next bridge to *also* create a connector: */ + flags |= DRM_BRIDGE_ATTACH_NO_CONNECTOR; + }
/* Attach the next bridge */ ret = drm_bridge_attach(bridge->encoder, pdata->next_bridge,
Hi,
On Mon, Mar 7, 2022 at 10:00 AM Kieran Bingham kieran.bingham+renesas@ideasonboard.com wrote:
From: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com
Now that the driver supports the connector-related bridge operations, make the connector creation optional. This enables usage of the sn65dsi86 with the DRM bridge connector helper.
Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com Signed-off-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com
Changes since v1:
- None
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-)
Can you please CC me on this series next time you send it out? I was pretty involved in previous reviews here. Luckily I got CCed on one of the patches so I knew to look, but since that one is (probably) getting dropped it'd be good to make sure I was on the others. It's also pretty important to include +Sam Ravnborg since there's a lot of overlap with his series [1]:
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index ffb6c04f6c46..29f5f7123ed9 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -745,11 +745,6 @@ static int (struct drm_bridge *bridge, struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge); int ret;
if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {DRM_ERROR("Fix bridge driver to make connector optional!");return -EINVAL;}
That won't work without some other fixes, sorry!
The problem is that you'll break ti_sn_bridge_get_bpp(). That absolutely relies on having our own connector so you need to fix that _before_ making the connector optional.
Rob Clark made an attempt on this [2] but Laurent didn't like the solution he came up with. Sam's series that I mentioned [1] also made an attempt at this, specifically in patch 5 ("drm/bridge: ti-sn65dsi86: Fetch bpc via drm_bridge_state") of his series. Unfortunately, it didn't work because the "output_bus_cfg.format" wasn't set to anything. Personally I wouldn't mind Rob's solution as a stopgap if Laurent removes his NAK. Then we're not stuck while someone tries to find time to fix this correctly.
One last problem here is that your patch doesn't actually apply to drm-misc/drm-misc-next, which is likely where it would land. I believe it conflicts with my recent commit e283820cbf80 ("drm/bridge: ti-sn65dsi86: Use drm_bridge_connector"). It looks like you based your series on mainline, but you should really be targeting drm-misc-next.
-Doug
[1] https://lore.kernel.org/r/20220206154405.1243333-1-sam@ravnborg.org/ [2] https://lore.kernel.org/all/20210920225801.227211-4-robdclark@gmail.com/
Hi Doug,
Quoting Doug Anderson (2022-03-07 23:21:28)
Hi,
On Mon, Mar 7, 2022 at 10:00 AM Kieran Bingham kieran.bingham+renesas@ideasonboard.com wrote:
From: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com
Now that the driver supports the connector-related bridge operations, make the connector creation optional. This enables usage of the sn65dsi86 with the DRM bridge connector helper.
Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com Signed-off-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com
Changes since v1:
- None
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-)
Can you please CC me on this series next time you send it out? I was pretty involved in previous reviews here. Luckily I got CCed on one of the patches so I knew to look, but since that one is (probably) getting dropped it'd be good to make sure I was on the others. It's also pretty important to include +Sam Ravnborg since there's a lot of overlap with his series [1]:
Absolutely - I was assuming you were the main target for reviews. I'm sorry - I also assumed get_maintainer.pl had / would add you - and I didn't check getting the patches out last night.
I'll be sure to manually add you.
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index ffb6c04f6c46..29f5f7123ed9 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -745,11 +745,6 @@ static int (struct drm_bridge *bridge, struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge); int ret;
if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {DRM_ERROR("Fix bridge driver to make connector optional!");return -EINVAL;}That won't work without some other fixes, sorry!
Ok ;-)
To be clear, my goal here has been to get the IRQ HPD working - so my main focus is there. I guess I now have to handle custodianship of these dependencies somehow too though.
The problem is that you'll break ti_sn_bridge_get_bpp(). That absolutely relies on having our own connector so you need to fix that _before_ making the connector optional.
Rob Clark made an attempt on this [2] but Laurent didn't like the solution he came up with. Sam's series that I mentioned [1] also made an attempt at this, specifically in patch 5 ("drm/bridge: ti-sn65dsi86: Fetch bpc via drm_bridge_state") of his series. Unfortunately, it didn't work because the "output_bus_cfg.format" wasn't set to anything. Personally I wouldn't mind Rob's solution as a stopgap if Laurent removes his NAK. Then we're not stuck while someone tries to find time to fix this correctly.
Ok - all of that is going to take some time for me to review and process.
One last problem here is that your patch doesn't actually apply to drm-misc/drm-misc-next, which is likely where it would land. I believe it conflicts with my recent commit e283820cbf80 ("drm/bridge: ti-sn65dsi86: Use drm_bridge_connector"). It looks like you based your series on mainline, but you should really be targeting drm-misc-next.
Ah sorry - I thought I had merged in drm-misc-next, but it was a week ago or so so I guess I'm already outdated.
I'll cleanup for a new base now.
-Doug
[1] https://lore.kernel.org/r/20220206154405.1243333-1-sam@ravnborg.org/ [2] https://lore.kernel.org/all/20210920225801.227211-4-robdclark@gmail.com/
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.
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 29f5f7123ed9..461f963faa0b 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -60,6 +60,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 @@ -91,6 +92,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) @@ -1005,6 +1008,11 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx, regmap_update_bits(pdata->regmap, SN_DATARATE_CONFIG_REG, DP_DATARATE_MASK, DP_DATARATE(dp_rate_idx));
+ /* For DisplayPort, use the standard DP scrambler seed. */ + if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort) + regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG, + ASSR_CONTROL, 0); + /* enable DP PLL */ regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 1);
@@ -1016,6 +1024,11 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx, goto exit; }
+ /* For DisplayPort, disable scrambling mode. */ + 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 @@ -1260,7 +1273,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.ops = DRM_BRIDGE_OP_EDID; - pdata->bridge.type = DRM_MODE_CONNECTOR_eDP; + pdata->bridge.type = pdata->next_bridge->type == DRM_MODE_CONNECTOR_DisplayPort + ? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;
drm_bridge_add(&pdata->bridge);
Hi,
On Mon, Mar 7, 2022 at 10:00 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.
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 29f5f7123ed9..461f963faa0b 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -60,6 +60,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 @@ -91,6 +92,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) @@ -1005,6 +1008,11 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx, regmap_update_bits(pdata->regmap, SN_DATARATE_CONFIG_REG, DP_DATARATE_MASK, DP_DATARATE(dp_rate_idx));
/* For DisplayPort, use the standard DP scrambler seed. */if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG,ASSR_CONTROL, 0);
I thought it was agreed that this hunk wasn't doing anything and should be removed? I did some research previously [1] and the manual said that this bit is "read only" unless "TEST2" is pulled high. In the previous discussion Laurent said that it wasn't. I also pointed out that this conflicts with the bit of code in ti_sn_bridge_enable() which tells the sink to enable the alternate scrambler.
/* enable DP PLL */ regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 1);@@ -1016,6 +1024,11 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx, goto exit; }
/* For DisplayPort, disable scrambling mode. */if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)regmap_update_bits(pdata->regmap, SN_TRAINING_SETTING_REG,SCRAMBLE_DISABLE, SCRAMBLE_DISABLE);
In my previous review I asked for some comments to include the "why" we disabling scrambling mode for DP. Can you please add that?
I also presume that for DP it's probably a good idea to avoid the code in ti_sn_bridge_enable() that tells the sink to use the alternate scrambler.
[1] https://lore.kernel.org/r/CAD=FV=Wwayx1Y-xv=RPuJbG+Q1wHrUWgh4P7wuzy_bAL=_FN0...
-Doug
Quoting Doug Anderson (2022-03-07 23:22:00)
Hi,
On Mon, Mar 7, 2022 at 10:00 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.
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 29f5f7123ed9..461f963faa0b 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -60,6 +60,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 @@ -91,6 +92,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) @@ -1005,6 +1008,11 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx, regmap_update_bits(pdata->regmap, SN_DATARATE_CONFIG_REG, DP_DATARATE_MASK, DP_DATARATE(dp_rate_idx));
/* For DisplayPort, use the standard DP scrambler seed. */if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG,ASSR_CONTROL, 0);I thought it was agreed that this hunk wasn't doing anything and should be removed? I did some research previously [1] and the manual said that this bit is "read only" unless "TEST2" is pulled high. In the previous discussion Laurent said that it wasn't. I also pointed out that this conflicts with the bit of code in ti_sn_bridge_enable() which tells the sink to enable the alternate scrambler.
Sorry - I had misremembered indeed, and looking back I confirmed that this hunk was not required. I had incorrectly remembered that the second part was required (below) and assumed that had meant both were.
Of course if we're disabling the scrambling mode, then it likely doesn't matter what the seed is!.
Looking at the datasheet though, register 0x5a, clearly says the default is 01 (ASSR) which we're not using. So the datasheet implies we want the DP scrambler seed (if it were enabled?)
Reading the 0x5a register here shows we have 0x05. Indeed, re-reading after attempting to write '0' to it still shows 0x05. So it's read-only and not relevant regardless.
I've removed this hunk now.
/* enable DP PLL */ regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 1);@@ -1016,6 +1024,11 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx, goto exit; }
/* For DisplayPort, disable scrambling mode. */if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)regmap_update_bits(pdata->regmap, SN_TRAINING_SETTING_REG,SCRAMBLE_DISABLE, SCRAMBLE_DISABLE);In my previous review I asked for some comments to include the "why" we disabling scrambling mode for DP. Can you please add that?
I also presume that for DP it's probably a good idea to avoid the code in ti_sn_bridge_enable() that tells the sink to use the alternate scrambler.
looking at it yes. I've added a check to avoid that in my DP connector case, and there doesn't seem to be any effect on the output. But I'll add it to the patch for the next version.
I think it was done for DRM purpose, to prevent signals meant for a panel to be connected to a device that could capture video from a DP source.
Is this better:
/* * Only eDP pannels which support Alternate Scrambler Seed Reset (ASSR) * are supported by the SN65DSI86. For DisplayPort, disable scrambling * mode. */
I don't know if it answers your 'why' ... and I don't think adding "We think it is for DRM protection"
really suits the comment.
[1] https://lore.kernel.org/r/CAD=FV=Wwayx1Y-xv=RPuJbG+Q1wHrUWgh4P7wuzy_bAL=_FN0...
-Doug
Hi,
On Wed, Mar 9, 2022 at 9:05 AM Kieran Bingham kieran.bingham+renesas@ideasonboard.com wrote:
I think it was done for DRM purpose, to prevent signals meant for a panel to be connected to a device that could capture video from a DP source.
Is this better:
/* * Only eDP pannels which support Alternate Scrambler Seed Reset (ASSR)
s/pannels/panels
* are supported by the SN65DSI86. For DisplayPort, disable scrambling * mode. */I don't know if it answers your 'why' ... and I don't think adding "We think it is for DRM protection"
really suits the comment.
Yeah, that looks pretty good. ...or even:
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.
-Doug
Quoting Kieran Bingham (2022-03-07 17:59:54)
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.
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 29f5f7123ed9..461f963faa0b 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -60,6 +60,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 @@ -91,6 +92,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) @@ -1005,6 +1008,11 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx, regmap_update_bits(pdata->regmap, SN_DATARATE_CONFIG_REG, DP_DATARATE_MASK, DP_DATARATE(dp_rate_idx));
/* For DisplayPort, use the standard DP scrambler seed. */if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG,ASSR_CONTROL, 0);/* enable DP PLL */ regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 1);@@ -1016,6 +1024,11 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx, goto exit; }
/* For DisplayPort, disable scrambling mode. */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@@ -1260,7 +1273,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.ops = DRM_BRIDGE_OP_EDID;
pdata->bridge.type = DRM_MODE_CONNECTOR_eDP;
pdata->bridge.type = pdata->next_bridge->type == DRM_MODE_CONNECTOR_DisplayPort? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;
Anyone know if there's any expectation of other types here? Or is it just as safe to do:
pdata->bridge.type = pdata->next_bridge->type;
To achieve the same effect?
-- Kieran
drm_bridge_add(&pdata->bridge);-- 2.32.0
Quoting Kieran Bingham (2022-03-10 14:04:09)
Quoting Kieran Bingham (2022-03-07 17:59:54)
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.
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 29f5f7123ed9..461f963faa0b 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -60,6 +60,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 @@ -91,6 +92,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) @@ -1005,6 +1008,11 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx, regmap_update_bits(pdata->regmap, SN_DATARATE_CONFIG_REG, DP_DATARATE_MASK, DP_DATARATE(dp_rate_idx));
/* For DisplayPort, use the standard DP scrambler seed. */if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG,ASSR_CONTROL, 0);/* enable DP PLL */ regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 1);@@ -1016,6 +1024,11 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx, goto exit; }
/* For DisplayPort, disable scrambling mode. */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@@ -1260,7 +1273,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.ops = DRM_BRIDGE_OP_EDID;
pdata->bridge.type = DRM_MODE_CONNECTOR_eDP;
pdata->bridge.type = pdata->next_bridge->type == DRM_MODE_CONNECTOR_DisplayPort? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;Anyone know if there's any expectation of other types here? Or is it just as safe to do:
pdata->bridge.type = pdata->next_bridge->type;To achieve the same effect?
Unfortunately, it would create nicer code - but I don't think it's exactly right. It leaves the definition of our bridge type to the connector type of the panel. While the SN65DSI86 can /only/ be either eDP or DisplayPort.
So I'll keep this conditional on if the next connector is DRM_MODE_CONNECTOR_DisplayPort.
-- Kieran
drm_bridge_add(&pdata->bridge);-- 2.32.0
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 --- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 133 +++++++++++++++++++++++--- 1 file changed, 120 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 461f963faa0b..febb4e672ece 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -68,6 +68,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 @@ -104,10 +105,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
@@ -166,6 +181,11 @@ * @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). */ struct ti_sn65dsi86 { struct auxiliary_device bridge_aux; @@ -200,6 +220,8 @@ struct ti_sn65dsi86 { atomic_t pwm_pin_busy; #endif unsigned int pwm_refclk_freq; + + bool no_hpd; };
static const struct regmap_range ti_sn65dsi86_volatile_ranges[] = { @@ -314,23 +336,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;
@@ -1164,6 +1188,36 @@ static void ti_sn_bridge_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) { @@ -1185,6 +1239,9 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = { .enable = ti_sn_bridge_enable, .disable = ti_sn_bridge_disable, .post_disable = ti_sn_bridge_post_disable, + .detect = ti_sn_bridge_detect, + .hpd_enable = ti_sn_bridge_hpd_enable, + .hpd_disable = ti_sn_bridge_hpd_disable, .get_edid = ti_sn_bridge_get_edid, };
@@ -1264,6 +1321,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 requires a DisplayPort connector\n"); + pdata->no_hpd = true; + } + ti_sn_bridge_parse_lanes(pdata, np);
ret = ti_sn_bridge_parse_dsi_host(pdata); @@ -1272,7 +1337,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.ops = DRM_BRIDGE_OP_EDID; + pdata->bridge.ops = (pdata->no_hpd ? 0 : DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_HPD) + | DRM_BRIDGE_OP_EDID; pdata->bridge.type = pdata->next_bridge->type == DRM_MODE_CONNECTOR_DisplayPort ? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;
@@ -1840,6 +1906,34 @@ static int ti_sn65dsi86_parse_regulators(struct ti_sn65dsi86 *pdata) pdata->supplies); }
+static irqreturn_t ti_sn65dsi86_irq_handler(int irq, void *arg) +{ + struct ti_sn65dsi86 *pdata = arg; + int ret; + 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_sn65dsi86_probe(struct i2c_client *client, const struct i2c_device_id *id) { @@ -1881,6 +1975,16 @@ static int ti_sn65dsi86_probe(struct i2c_client *client, return dev_err_probe(dev, PTR_ERR(pdata->refclk), "failed to get reference clock\n");
+ if (client->irq > 0) { + ret = devm_request_threaded_irq(dev, client->irq, NULL, + ti_sn65dsi86_irq_handler, + IRQF_ONESHOT, "sn65dsi86-irq", + pdata); + if (ret) + return dev_err_probe(dev, ret, + "Failed to register DP interrupt\n"); + } + pm_runtime_enable(dev); ret = devm_add_action_or_reset(dev, ti_sn65dsi86_runtime_disable, dev); if (ret) @@ -1888,6 +1992,9 @@ static int ti_sn65dsi86_probe(struct i2c_client *client, pm_runtime_set_autosuspend_delay(pdata->dev, 500); pm_runtime_use_autosuspend(pdata->dev);
+ /* Enable interrupt handling */ + regmap_write(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN); + ti_sn65dsi86_debugfs_init(pdata);
/*
Hi,
On Mon, Mar 7, 2022 at 10:00 AM Kieran Bingham kieran.bingham+renesas@ideasonboard.com wrote:
@@ -1264,6 +1321,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 requires a DisplayPort connector\n");
Maybe "HPD support only implemented for full DP connectors".
Plausibly someone could come up with a reason to hook HPD up for eDP one day, but so far we haven't seen it.
@@ -1272,7 +1337,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.ops = DRM_BRIDGE_OP_EDID;
pdata->bridge.ops = (pdata->no_hpd ? 0 : DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_HPD)| DRM_BRIDGE_OP_EDID;
Seems like "OP_HPD" ought to be dependent on whether the IRQ was provided, right? AKA you could have "detect" because HPD is plumbed through to the bridge but not "HPD" if the IRQ from the bridge isn't hooked up to the main processor...
@@ -1840,6 +1906,34 @@ static int ti_sn65dsi86_parse_regulators(struct ti_sn65dsi86 *pdata) pdata->supplies); }
+static irqreturn_t ti_sn65dsi86_irq_handler(int irq, void *arg) +{
struct ti_sn65dsi86 *pdata = arg;int ret;int hpd;
`hpd` should be an `unsigned int` to match the prototype of regmap-read.
@@ -1881,6 +1975,16 @@ static int ti_sn65dsi86_probe(struct i2c_client *client, return dev_err_probe(dev, PTR_ERR(pdata->refclk), "failed to get reference clock\n");
if (client->irq > 0) {ret = devm_request_threaded_irq(dev, client->irq, NULL,ti_sn65dsi86_irq_handler,IRQF_ONESHOT, "sn65dsi86-irq",pdata);if (ret)return dev_err_probe(dev, ret,"Failed to register DP interrupt\n");}
Is this the right place to request the IRQ, or should it be in ti_sn_bridge_probe(). As soon as you request it the interrupt can go off, but you're relying on a bunch of bridge stuff to have been initted, right?
@@ -1888,6 +1992,9 @@ static int ti_sn65dsi86_probe(struct i2c_client *client, pm_runtime_set_autosuspend_delay(pdata->dev, 500); pm_runtime_use_autosuspend(pdata->dev);
/* Enable interrupt handling */regmap_write(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN);
Shouldn't we only enable interrupt handling if client->irq > 0? AKA combine this with the "if" statement?
-Doug
Quoting Doug Anderson (2022-03-07 23:22:17)
Hi,
On Mon, Mar 7, 2022 at 10:00 AM Kieran Bingham kieran.bingham+renesas@ideasonboard.com wrote:
@@ -1264,6 +1321,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 requires a DisplayPort connector\n");Maybe "HPD support only implemented for full DP connectors".
Plausibly someone could come up with a reason to hook HPD up for eDP one day, but so far we haven't seen it.
Ok, updated.
@@ -1272,7 +1337,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.ops = DRM_BRIDGE_OP_EDID;
pdata->bridge.ops = (pdata->no_hpd ? 0 : DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_HPD)| DRM_BRIDGE_OP_EDID;Seems like "OP_HPD" ought to be dependent on whether the IRQ was provided, right? AKA you could have "detect" because HPD is plumbed through to the bridge but not "HPD" if the IRQ from the bridge isn't hooked up to the main processor...
Yes, I think that's right. If there's no IRQ, then OP_HPD shouldn't be set, and it will fall back to polling.
I'll fix that up.
@@ -1840,6 +1906,34 @@ static int ti_sn65dsi86_parse_regulators(struct ti_sn65dsi86 *pdata) pdata->supplies); }
+static irqreturn_t ti_sn65dsi86_irq_handler(int irq, void *arg) +{
struct ti_sn65dsi86 *pdata = arg;int ret;int hpd;`hpd` should be an `unsigned int` to match the prototype of regmap-read.
Agreed, and updated.
@@ -1881,6 +1975,16 @@ static int ti_sn65dsi86_probe(struct i2c_client *client, return dev_err_probe(dev, PTR_ERR(pdata->refclk), "failed to get reference clock\n");
if (client->irq > 0) {ret = devm_request_threaded_irq(dev, client->irq, NULL,ti_sn65dsi86_irq_handler,IRQF_ONESHOT, "sn65dsi86-irq",pdata);if (ret)return dev_err_probe(dev, ret,"Failed to register DP interrupt\n");}Is this the right place to request the IRQ, or should it be in ti_sn_bridge_probe(). As soon as you request it the interrupt can go off, but you're relying on a bunch of bridge stuff to have been initted, right?
Indeed, it will be relying upon the bridge to have been set up.
You're right I believe, ti_sn_bridge_probe() sounds reasonable. And only after that should we enable the interrupts.
Fixing ... (But getting stuck/blocked by the connector changes, so .. I'll keep plowing through).
@@ -1888,6 +1992,9 @@ static int ti_sn65dsi86_probe(struct i2c_client *client, pm_runtime_set_autosuspend_delay(pdata->dev, 500); pm_runtime_use_autosuspend(pdata->dev);
/* Enable interrupt handling */regmap_write(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN);Shouldn't we only enable interrupt handling if client->irq > 0? AKA combine this with the "if" statement?
Agreed.
-Doug
dri-devel@lists.freedesktop.org
-
Doug Anderson -
Kieran Bingham