diff mbox series

[v2,4/6] drm/bridge: ti-sn65dsi86: Wrap panel with panel-bridge

Message ID 20210624000304.16281-5-laurent.pinchart+renesas@ideasonboard.com (mailing list archive)
State New, archived
Headers show
Series drm/bridge: ti-sn65dsi86: Misc improvements | expand

Commit Message

Laurent Pinchart June 24, 2021, 12:03 a.m. UTC 
To simplify interfacing with the panel, wrap it in a panel-bridge and
let the DRM bridge helpers handle chaining of operations.

This also prepares for support of DRM_BRIDGE_ATTACH_NO_CONNECTOR, which
requires all components in the display pipeline to be represented by
bridges.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Reviewed-by: Jagan Teki <jagan@amarulasolutions.com>
---
Changes since v1:

- Drop error message when drm_bridge_attach() fails (now printed by the
  function)
- Return ERR_PTR() directly
- Clarify which bridge is the next bridge
- Drop ti_sn65dsi86.panel field
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 32 +++++++++++++++++----------
 1 file changed, 20 insertions(+), 12 deletions(-)

Comments

Stephen Boyd Aug. 11, 2021, 5:26 a.m. UTC | #1 
Quoting Laurent Pinchart (2021-06-23 17:03:02)
> To simplify interfacing with the panel, wrap it in a panel-bridge and
> let the DRM bridge helpers handle chaining of operations.
>
> This also prepares for support of DRM_BRIDGE_ATTACH_NO_CONNECTOR, which
> requires all components in the display pipeline to be represented by
> bridges.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Reviewed-by: Jagan Teki <jagan@amarulasolutions.com>
> ---

With this patch applied I get two eDP devices on Lazor sc7180 (it is the
arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor*.dts files if you're
looking for more info). As far as I can tell, we should only have one
eDP device on the board, for the bridge.

localhost ~ # ls -l /sys/class/drm/card1-eDP*
lrwxrwxrwx. 1 root root 0 Aug 10 22:24 /sys/class/drm/card1-eDP-1 ->
../../devices/platform/soc@0/ae00000.mdss/drm/card1/card1-eDP-1
lrwxrwxrwx. 1 root root 0 Aug 10 22:24 /sys/class/drm/card1-eDP-2 ->
../../devices/platform/soc@0/ae00000.mdss/drm/card1/card1-eDP-2
Laurent Pinchart Aug. 11, 2021, 12:15 p.m. UTC | #2 
Hi Stephen,

On Tue, Aug 10, 2021 at 10:26:33PM -0700, Stephen Boyd wrote:
> Quoting Laurent Pinchart (2021-06-23 17:03:02)
> > To simplify interfacing with the panel, wrap it in a panel-bridge and
> > let the DRM bridge helpers handle chaining of operations.
> >
> > This also prepares for support of DRM_BRIDGE_ATTACH_NO_CONNECTOR, which
> > requires all components in the display pipeline to be represented by
> > bridges.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > Reviewed-by: Jagan Teki <jagan@amarulasolutions.com>
> > ---
> 
> With this patch applied I get two eDP devices on Lazor sc7180 (it is the
> arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor*.dts files if you're
> looking for more info). As far as I can tell, we should only have one
> eDP device on the board, for the bridge.
> 
> localhost ~ # ls -l /sys/class/drm/card1-eDP*
> lrwxrwxrwx. 1 root root 0 Aug 10 22:24 /sys/class/drm/card1-eDP-1 ->
> ../../devices/platform/soc@0/ae00000.mdss/drm/card1/card1-eDP-1
> lrwxrwxrwx. 1 root root 0 Aug 10 22:24 /sys/class/drm/card1-eDP-2 ->
> ../../devices/platform/soc@0/ae00000.mdss/drm/card1/card1-eDP-2

Indeed.

Does the display driver use the DRM connector bridge helper and
DRM_BRIDGE_ATTACH_NO_CONNECTOR on that platform ?
Rob Clark Aug. 11, 2021, 4:20 p.m. UTC | #3 
On Wed, Aug 11, 2021 at 5:15 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Stephen,
>
> On Tue, Aug 10, 2021 at 10:26:33PM -0700, Stephen Boyd wrote:
> > Quoting Laurent Pinchart (2021-06-23 17:03:02)
> > > To simplify interfacing with the panel, wrap it in a panel-bridge and
> > > let the DRM bridge helpers handle chaining of operations.
> > >
> > > This also prepares for support of DRM_BRIDGE_ATTACH_NO_CONNECTOR, which
> > > requires all components in the display pipeline to be represented by
> > > bridges.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > Reviewed-by: Jagan Teki <jagan@amarulasolutions.com>
> > > ---
> >
> > With this patch applied I get two eDP devices on Lazor sc7180 (it is the
> > arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor*.dts files if you're
> > looking for more info). As far as I can tell, we should only have one
> > eDP device on the board, for the bridge.
> >
> > localhost ~ # ls -l /sys/class/drm/card1-eDP*
> > lrwxrwxrwx. 1 root root 0 Aug 10 22:24 /sys/class/drm/card1-eDP-1 ->
> > ../../devices/platform/soc@0/ae00000.mdss/drm/card1/card1-eDP-1
> > lrwxrwxrwx. 1 root root 0 Aug 10 22:24 /sys/class/drm/card1-eDP-2 ->
> > ../../devices/platform/soc@0/ae00000.mdss/drm/card1/card1-eDP-2
>
> Indeed.
>
> Does the display driver use the DRM connector bridge helper and
> DRM_BRIDGE_ATTACH_NO_CONNECTOR on that platform ?
>

There haven't been any recent changes about how we attach the bridge,
it doesn't pass DRM_BRIDGE_ATTACH_NO_CONNECTOR.. tbh I've not been
having time to follow too closely the recent changes with bridge stuff
myself.

But now with this patch we have both the ti bridge and the panel
bridge creating a connector..  removing the connector created by the
ti bridge "fixes" things, but not sure if that would break something
on other platforms.  I guess there should now always be a panel
bridge, so removing ti_sn_bridge_connector_init() would be a sane
thing to do?

BR,
-R
Stephen Boyd Aug. 11, 2021, 8:39 p.m. UTC | #4 
Quoting Rob Clark (2021-08-11 09:20:30)
> On Wed, Aug 11, 2021 at 5:15 AM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >
> > Hi Stephen,
> >
> > On Tue, Aug 10, 2021 at 10:26:33PM -0700, Stephen Boyd wrote:
> > > Quoting Laurent Pinchart (2021-06-23 17:03:02)
> > > > To simplify interfacing with the panel, wrap it in a panel-bridge and
> > > > let the DRM bridge helpers handle chaining of operations.
> > > >
> > > > This also prepares for support of DRM_BRIDGE_ATTACH_NO_CONNECTOR, which
> > > > requires all components in the display pipeline to be represented by
> > > > bridges.
> > > >
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > > Reviewed-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > ---
> > >
> > > With this patch applied I get two eDP devices on Lazor sc7180 (it is the
> > > arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor*.dts files if you're
> > > looking for more info). As far as I can tell, we should only have one
> > > eDP device on the board, for the bridge.
> > >
> > > localhost ~ # ls -l /sys/class/drm/card1-eDP*
> > > lrwxrwxrwx. 1 root root 0 Aug 10 22:24 /sys/class/drm/card1-eDP-1 ->
> > > ../../devices/platform/soc@0/ae00000.mdss/drm/card1/card1-eDP-1
> > > lrwxrwxrwx. 1 root root 0 Aug 10 22:24 /sys/class/drm/card1-eDP-2 ->
> > > ../../devices/platform/soc@0/ae00000.mdss/drm/card1/card1-eDP-2
> >
> > Indeed.
> >
> > Does the display driver use the DRM connector bridge helper and
> > DRM_BRIDGE_ATTACH_NO_CONNECTOR on that platform ?
> >
>
> There haven't been any recent changes about how we attach the bridge,
> it doesn't pass DRM_BRIDGE_ATTACH_NO_CONNECTOR.. tbh I've not been
> having time to follow too closely the recent changes with bridge stuff
> myself.
>
> But now with this patch we have both the ti bridge and the panel
> bridge creating a connector..  removing the connector created by the
> ti bridge "fixes" things, but not sure if that would break something
> on other platforms.  I guess there should now always be a panel
> bridge, so removing ti_sn_bridge_connector_init() would be a sane
> thing to do?
>

So this patch works. We don't want to make the connector in this driver
for the next bridge because this driver is making the connector. I guess
eventually we'll drop this flag when this driver stops making the
connector here?

---8<---
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index cd0fccdd8dfd..a8d4818484aa 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -741,7 +741,7 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,

 	/* Attach the next bridge */
 	ret = drm_bridge_attach(bridge->encoder, pdata->next_bridge,
-				&pdata->bridge, flags);
+				&pdata->bridge, flags | DRM_BRIDGE_ATTACH_NO_CONNECTOR);
 	if (ret < 0)
 		goto err_dsi_detach;
Rob Clark Aug. 11, 2021, 8:51 p.m. UTC | #5 
On Wed, Aug 11, 2021 at 1:39 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Rob Clark (2021-08-11 09:20:30)
> > On Wed, Aug 11, 2021 at 5:15 AM Laurent Pinchart
> > <laurent.pinchart@ideasonboard.com> wrote:
> > >
> > > Hi Stephen,
> > >
> > > On Tue, Aug 10, 2021 at 10:26:33PM -0700, Stephen Boyd wrote:
> > > > Quoting Laurent Pinchart (2021-06-23 17:03:02)
> > > > > To simplify interfacing with the panel, wrap it in a panel-bridge and
> > > > > let the DRM bridge helpers handle chaining of operations.
> > > > >
> > > > > This also prepares for support of DRM_BRIDGE_ATTACH_NO_CONNECTOR, which
> > > > > requires all components in the display pipeline to be represented by
> > > > > bridges.
> > > > >
> > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > > > Reviewed-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > > ---
> > > >
> > > > With this patch applied I get two eDP devices on Lazor sc7180 (it is the
> > > > arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor*.dts files if you're
> > > > looking for more info). As far as I can tell, we should only have one
> > > > eDP device on the board, for the bridge.
> > > >
> > > > localhost ~ # ls -l /sys/class/drm/card1-eDP*
> > > > lrwxrwxrwx. 1 root root 0 Aug 10 22:24 /sys/class/drm/card1-eDP-1 ->
> > > > ../../devices/platform/soc@0/ae00000.mdss/drm/card1/card1-eDP-1
> > > > lrwxrwxrwx. 1 root root 0 Aug 10 22:24 /sys/class/drm/card1-eDP-2 ->
> > > > ../../devices/platform/soc@0/ae00000.mdss/drm/card1/card1-eDP-2
> > >
> > > Indeed.
> > >
> > > Does the display driver use the DRM connector bridge helper and
> > > DRM_BRIDGE_ATTACH_NO_CONNECTOR on that platform ?
> > >
> >
> > There haven't been any recent changes about how we attach the bridge,
> > it doesn't pass DRM_BRIDGE_ATTACH_NO_CONNECTOR.. tbh I've not been
> > having time to follow too closely the recent changes with bridge stuff
> > myself.
> >
> > But now with this patch we have both the ti bridge and the panel
> > bridge creating a connector..  removing the connector created by the
> > ti bridge "fixes" things, but not sure if that would break something
> > on other platforms.  I guess there should now always be a panel
> > bridge, so removing ti_sn_bridge_connector_init() would be a sane
> > thing to do?
> >
>
> So this patch works. We don't want to make the connector in this driver
> for the next bridge because this driver is making the connector. I guess
> eventually we'll drop this flag when this driver stops making the
> connector here?
>
> ---8<---
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index cd0fccdd8dfd..a8d4818484aa 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -741,7 +741,7 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
>
>         /* Attach the next bridge */
>         ret = drm_bridge_attach(bridge->encoder, pdata->next_bridge,
> -                               &pdata->bridge, flags);
> +                               &pdata->bridge, flags | DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>         if (ret < 0)
>                 goto err_dsi_detach;


I kinda think *all* bridges that create a connector (whether optional
or not) should OR in NO_CONNECTOR when attaching the next downstream
bridge.. since you never want multiple connectors

BR,
-R
Laurent Pinchart Aug. 11, 2021, 10:40 p.m. UTC | #6 
On Wed, Aug 11, 2021 at 01:51:28PM -0700, Rob Clark wrote:
> On Wed, Aug 11, 2021 at 1:39 PM Stephen Boyd wrote:
> > Quoting Rob Clark (2021-08-11 09:20:30)
> > > On Wed, Aug 11, 2021 at 5:15 AM Laurent Pinchart wrote:
> > > > On Tue, Aug 10, 2021 at 10:26:33PM -0700, Stephen Boyd wrote:
> > > > > Quoting Laurent Pinchart (2021-06-23 17:03:02)
> > > > > > To simplify interfacing with the panel, wrap it in a panel-bridge and
> > > > > > let the DRM bridge helpers handle chaining of operations.
> > > > > >
> > > > > > This also prepares for support of DRM_BRIDGE_ATTACH_NO_CONNECTOR, which
> > > > > > requires all components in the display pipeline to be represented by
> > > > > > bridges.
> > > > > >
> > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > > > > Reviewed-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > > > ---
> > > > >
> > > > > With this patch applied I get two eDP devices on Lazor sc7180 (it is the
> > > > > arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor*.dts files if you're
> > > > > looking for more info). As far as I can tell, we should only have one
> > > > > eDP device on the board, for the bridge.
> > > > >
> > > > > localhost ~ # ls -l /sys/class/drm/card1-eDP*
> > > > > lrwxrwxrwx. 1 root root 0 Aug 10 22:24 /sys/class/drm/card1-eDP-1 ->
> > > > > ../../devices/platform/soc@0/ae00000.mdss/drm/card1/card1-eDP-1
> > > > > lrwxrwxrwx. 1 root root 0 Aug 10 22:24 /sys/class/drm/card1-eDP-2 ->
> > > > > ../../devices/platform/soc@0/ae00000.mdss/drm/card1/card1-eDP-2
> > > >
> > > > Indeed.
> > > >
> > > > Does the display driver use the DRM connector bridge helper and
> > > > DRM_BRIDGE_ATTACH_NO_CONNECTOR on that platform ?
> > >
> > > There haven't been any recent changes about how we attach the bridge,
> > > it doesn't pass DRM_BRIDGE_ATTACH_NO_CONNECTOR.. tbh I've not been
> > > having time to follow too closely the recent changes with bridge stuff
> > > myself.
> > >
> > > But now with this patch we have both the ti bridge and the panel
> > > bridge creating a connector..  removing the connector created by the
> > > ti bridge "fixes" things, but not sure if that would break something
> > > on other platforms.  I guess there should now always be a panel
> > > bridge, so removing ti_sn_bridge_connector_init() would be a sane
> > > thing to do?
> >
> > So this patch works. We don't want to make the connector in this driver
> > for the next bridge because this driver is making the connector. I guess
> > eventually we'll drop this flag when this driver stops making the
> > connector here?
> >
> > ---8<---
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index cd0fccdd8dfd..a8d4818484aa 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > @@ -741,7 +741,7 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
> >
> >         /* Attach the next bridge */
> >         ret = drm_bridge_attach(bridge->encoder, pdata->next_bridge,
> > -                               &pdata->bridge, flags);
> > +                               &pdata->bridge, flags | DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> >         if (ret < 0)
> >                 goto err_dsi_detach;
> 
> I kinda think *all* bridges that create a connector (whether optional
> or not) should OR in NO_CONNECTOR when attaching the next downstream
> bridge.. since you never want multiple connectors

Yes, that sounds reasonable to me. Stephen, would you like to set a
patch ?
Stephen Boyd Aug. 11, 2021, 10:43 p.m. UTC | #7 
Quoting Laurent Pinchart (2021-08-11 15:40:24)
> On Wed, Aug 11, 2021 at 01:51:28PM -0700, Rob Clark wrote:
> >
> > I kinda think *all* bridges that create a connector (whether optional
> > or not) should OR in NO_CONNECTOR when attaching the next downstream
> > bridge.. since you never want multiple connectors
>
> Yes, that sounds reasonable to me. Stephen, would you like to set a
> patch ?
>

Sure I'll send a proper patch for this bridge driver.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 28c1ea370ae4..18b4420cc6b3 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -127,7 +127,7 @@ 
  * @host_node:    Remote DSI node.
  * @dsi:          Our MIPI DSI source.
  * @refclk:       Our reference clock.
- * @panel:        Our panel.
+ * @next_bridge:  The bridge on the eDP side.
  * @enable_gpio:  The GPIO we toggle to enable the bridge.
  * @supplies:     Data for bulk enabling/disabling our regulators.
  * @dp_lanes:     Count of dp_lanes we're using.
@@ -159,7 +159,7 @@  struct ti_sn65dsi86 {
 	struct device_node		*host_node;
 	struct mipi_dsi_device		*dsi;
 	struct clk			*refclk;
-	struct drm_panel		*panel;
+	struct drm_bridge		*next_bridge;
 	struct gpio_desc		*enable_gpio;
 	struct regulator_bulk_data	supplies[SN_REGULATOR_SUPPLY_NUM];
 	int				dp_lanes;
@@ -404,7 +404,8 @@  connector_to_ti_sn65dsi86(struct drm_connector *connector)
 static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector)
 {
 	struct ti_sn65dsi86 *pdata = connector_to_ti_sn65dsi86(connector);
-	return drm_panel_get_modes(pdata->panel, connector);
+
+	return drm_bridge_get_modes(pdata->next_bridge, connector);
 }
 
 static enum drm_mode_status
@@ -530,8 +531,16 @@  static int ti_sn_bridge_attach(struct drm_bridge *bridge,
 	}
 	pdata->dsi = dsi;
 
+	/* Attach the next bridge */
+	ret = drm_bridge_attach(bridge->encoder, pdata->next_bridge,
+				&pdata->bridge, flags);
+	if (ret < 0)
+		goto err_dsi_detach;
+
 	return 0;
 
+err_dsi_detach:
+	mipi_dsi_detach(dsi);
 err_dsi_attach:
 	mipi_dsi_device_unregister(dsi);
 err_dsi_host:
@@ -550,8 +559,6 @@  static void ti_sn_bridge_disable(struct drm_bridge *bridge)
 {
 	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
 
-	drm_panel_disable(pdata->panel);
-
 	/* disable video stream */
 	regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG, VSTREAM_ENABLE, 0);
 	/* semi auto link training mode OFF */
@@ -878,8 +885,6 @@  static void ti_sn_bridge_enable(struct drm_bridge *bridge)
 	/* enable video stream */
 	regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG, VSTREAM_ENABLE,
 			   VSTREAM_ENABLE);
-
-	drm_panel_enable(pdata->panel);
 }
 
 static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
@@ -890,16 +895,12 @@  static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
 
 	if (!pdata->refclk)
 		ti_sn65dsi86_enable_comms(pdata);
-
-	drm_panel_prepare(pdata->panel);
 }
 
 static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
 {
 	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
 
-	drm_panel_unprepare(pdata->panel);
-
 	if (!pdata->refclk)
 		ti_sn65dsi86_disable_comms(pdata);
 
@@ -1304,13 +1305,20 @@  static int ti_sn_bridge_probe(struct auxiliary_device *adev,
 {
 	struct ti_sn65dsi86 *pdata = dev_get_drvdata(adev->dev.parent);
 	struct device_node *np = pdata->dev->of_node;
+	struct drm_panel *panel;
 	int ret;
 
-	ret = drm_of_find_panel_or_bridge(np, 1, 0, &pdata->panel, NULL);
+	ret = drm_of_find_panel_or_bridge(np, 1, 0, &panel, NULL);
 	if (ret)
 		return dev_err_probe(&adev->dev, ret,
 				     "could not find any panel node\n");
 
+	pdata->next_bridge = devm_drm_panel_bridge_add(pdata->dev, panel);
+	if (IS_ERR(pdata->next_bridge)) {
+		DRM_ERROR("failed to create panel bridge\n");
+		return PTR_ERR(pdata->next_bridge);
+	}
+
 	ti_sn_bridge_parse_lanes(pdata, np);
 
 	ret = ti_sn_bridge_parse_dsi_host(pdata);