diff mbox series

[RFC,11/11] drm/bridge: ti-sn65dsi86: Support hotplug detection

Message ID 20210322030128.2283-12-laurent.pinchart+renesas@ideasonboard.com (mailing list archive)
State New, archived
Headers show
Series drm/bridge: ti-sn65dsi86: Support DisplayPort mode | expand

Commit Message

Laurent Pinchart March 22, 2021, 3:01 a.m. UTC
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>
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 46 +++++++++++++++++++--------
 1 file changed, 33 insertions(+), 13 deletions(-)

Comments

Stephen Boyd March 23, 2021, 7:21 a.m. UTC | #1
Quoting Laurent Pinchart (2021-03-21 20:01:28)
> 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>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 46 +++++++++++++++++++--------
>  1 file changed, 33 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index f792227142a7..72f6362adf44 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -890,6 +897,15 @@ 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_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
> +       int val;
> +
> +       regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val);
> +       return val ? connector_status_connected : connector_status_disconnected;
> +}
> +
>  static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge,
>                                           struct drm_connector *connector)
>  {
> @@ -904,6 +920,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,
> +       .detect = ti_sn_bridge_detect,
>         .get_edid = ti_sn_bridge_get_edid,
>  };
>  
> @@ -1327,6 +1344,8 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
>                 return ret;
>         }
>  
> +       pdata->no_hpd = of_property_read_bool(pdata->dev->of_node, "no-hpd");

I see that we missed adding this property to the DTS file but skated by
because it was the default in the driver. I don't think it's a big deal
just something we should fix in sc7180-trogdor.dtsi before this patch is
merged.

> +
>         ti_sn_bridge_parse_lanes(pdata, client->dev.of_node);
>  
>         ret = ti_sn_bridge_parse_regulators(pdata);
Doug Anderson March 24, 2021, 10:47 p.m. UTC | #2
Hi,

On Sun, Mar 21, 2021 at 8:02 PM Laurent Pinchart
<laurent.pinchart+renesas@ideasonboard.com> 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.
>
> 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>
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 46 +++++++++++++++++++--------
>  1 file changed, 33 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index f792227142a7..72f6362adf44 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -167,6 +167,8 @@ struct ti_sn_bridge {
>         struct gpio_chip                gchip;
>         DECLARE_BITMAP(gchip_output, SN_NUM_GPIOS);
>  #endif
> +
> +       bool                            no_hpd;

This structure is documented by kernel-doc, but you didn't add your new member.


>  };
>
>  static const struct regmap_range ti_sn_bridge_volatile_ranges[] = {
> @@ -862,23 +864,28 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
>         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);
> +       else
> +               regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG,
> +                                  HPD_DISABLE, 0);

Optionally you could skip the "else". HPD enabled is the default state
and, in general, we don't exhaustively init all registers and rely on
the power-on defaults for ones we don't explicitly control.


>  }
>
>  static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
> @@ -890,6 +897,15 @@ 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_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
> +       int val;
> +
> +       regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val);
> +       return val ? connector_status_connected : connector_status_disconnected;

I would have expected that you would have used the interrupt signal,
but I guess it just polls in this case. I suppose polling has the
advantage that it's simpler... Maybe throw in a comment about why IRQ
isn't being used?


> +}
> +
>  static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge,
>                                           struct drm_connector *connector)
>  {
> @@ -904,6 +920,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,
> +       .detect = ti_sn_bridge_detect,
>         .get_edid = ti_sn_bridge_get_edid,
>  };
>
> @@ -1327,6 +1344,8 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
>                 return ret;
>         }
>
> +       pdata->no_hpd = of_property_read_bool(pdata->dev->of_node, "no-hpd");
> +
>         ti_sn_bridge_parse_lanes(pdata, client->dev.of_node);
>
>         ret = ti_sn_bridge_parse_regulators(pdata);
> @@ -1365,7 +1384,8 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
>
>         pdata->bridge.funcs = &ti_sn_bridge_funcs;
>         pdata->bridge.of_node = client->dev.of_node;
> -       pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
> +       pdata->bridge.ops = (pdata->no_hpd ? 0 : DRM_BRIDGE_OP_DETECT)

Checking for "no_hpd" here is not the right test IIUC. You want to
check for eDP vs. DP (AKA whether a panel is downstream of you or a
connector). Specifically if downstream of you is a panel then (I
believe) HPD won't assert until you turn on the panel and you won't
turn on the panel (which happens in pre_enable, right?) until HPD
fires, so you've got a chicken-and-egg problem. If downstream of you
is a connector, though, then by definition HPD has to just work
without pre_enable running so then you're OK.

I guess then you'd need to figure out what to do if someone wants to
use "HPD" on eDP. Do you need to put a polling loop in pre_enable
then? Or you could just punt not support this case until someone needs
it.


> +                         | DRM_BRIDGE_OP_EDID;

IMO somewhere in here if HPD is being used like this you should throw
in a call to pm_runtime_get_sync(). I guess in your solution the
regulators (for the bridge, not the panel) and enable pin are just
left on all the time, but plausibly someone might want to build a
system to use HPD and also have the enable pin and/or regulators
controlled by this driver, right?


-Doug
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index f792227142a7..72f6362adf44 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -167,6 +167,8 @@  struct ti_sn_bridge {
 	struct gpio_chip		gchip;
 	DECLARE_BITMAP(gchip_output, SN_NUM_GPIOS);
 #endif
+
+	bool				no_hpd;
 };
 
 static const struct regmap_range ti_sn_bridge_volatile_ranges[] = {
@@ -862,23 +864,28 @@  static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
 	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);
+	else
+		regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG,
+				   HPD_DISABLE, 0);
 }
 
 static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
@@ -890,6 +897,15 @@  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_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
+	int val;
+
+	regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val);
+	return val ? connector_status_connected : connector_status_disconnected;
+}
+
 static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge,
 					  struct drm_connector *connector)
 {
@@ -904,6 +920,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,
+	.detect = ti_sn_bridge_detect,
 	.get_edid = ti_sn_bridge_get_edid,
 };
 
@@ -1327,6 +1344,8 @@  static int ti_sn_bridge_probe(struct i2c_client *client,
 		return ret;
 	}
 
+	pdata->no_hpd = of_property_read_bool(pdata->dev->of_node, "no-hpd");
+
 	ti_sn_bridge_parse_lanes(pdata, client->dev.of_node);
 
 	ret = ti_sn_bridge_parse_regulators(pdata);
@@ -1365,7 +1384,8 @@  static int ti_sn_bridge_probe(struct i2c_client *client,
 
 	pdata->bridge.funcs = &ti_sn_bridge_funcs;
 	pdata->bridge.of_node = client->dev.of_node;
-	pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
+	pdata->bridge.ops = (pdata->no_hpd ? 0 : DRM_BRIDGE_OP_DETECT)
+			  | DRM_BRIDGE_OP_EDID;
 	pdata->bridge.type = pdata->panel ? DRM_MODE_CONNECTOR_eDP
 			   : DRM_MODE_CONNECTOR_DisplayPort;