diff mbox series

[RFC,05/11] drm/bridge: ti-sn65dsi86: Wrap panel with panel-bridge

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

Commit Message

Laurent Pinchart March 22, 2021, 3:01 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>
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 30 +++++++++++++++++++--------
 1 file changed, 21 insertions(+), 9 deletions(-)

Comments

Jagan Teki March 22, 2021, 10:19 a.m. UTC | #1
On Mon, Mar 22, 2021 at 8:32 AM Laurent Pinchart
<laurent.pinchart+renesas@ideasonboard.com> wrote:
>
> 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>
Stephen Boyd March 23, 2021, 7:14 a.m. UTC | #2
Quoting Laurent Pinchart (2021-03-21 20:01:22)
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 1d1be791d5ba..c21a7f7d452b 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -418,8 +420,18 @@ 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) {
> +               DRM_ERROR("failed to attach next bridge\n");

Can this be pushed into drm_bridge_attach() instead of in each caller?

> +               goto err_dsi_detach;
> +       }
> +
>         return 0;
>  
> +err_dsi_detach:
> +       mipi_dsi_detach(dsi);
>  err_dsi_attach:
>         mipi_dsi_device_unregister(dsi);
>  err_dsi_host:
>  static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
> @@ -1245,6 +1249,14 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
>                 return ret;
>         }
>  
> +       pdata->next_bridge = devm_drm_panel_bridge_add(pdata->dev,
> +                                                      pdata->panel);
> +       if (IS_ERR(pdata->next_bridge)) {
> +               DRM_ERROR("failed to create panel bridge\n");
> +               ret = PTR_ERR(pdata->next_bridge);
> +               return ret;

Just return PTR_ERR(pdata->next_bridge)?

> +       }
> +
>         dev_set_drvdata(&client->dev, pdata);
Laurent Pinchart March 23, 2021, 9:50 p.m. UTC | #3
Hi Stephen,

On Tue, Mar 23, 2021 at 12:14:04AM -0700, Stephen Boyd wrote:
> Quoting Laurent Pinchart (2021-03-21 20:01:22)
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index 1d1be791d5ba..c21a7f7d452b 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > @@ -418,8 +420,18 @@ 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) {
> > +               DRM_ERROR("failed to attach next bridge\n");
> 
> Can this be pushed into drm_bridge_attach() instead of in each caller?

Good idea.

> > +               goto err_dsi_detach;
> > +       }
> > +
> >         return 0;
> >  
> > +err_dsi_detach:
> > +       mipi_dsi_detach(dsi);
> >  err_dsi_attach:
> >         mipi_dsi_device_unregister(dsi);
> >  err_dsi_host:
> >  static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
> > @@ -1245,6 +1249,14 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
> >                 return ret;
> >         }
> >  
> > +       pdata->next_bridge = devm_drm_panel_bridge_add(pdata->dev,
> > +                                                      pdata->panel);
> > +       if (IS_ERR(pdata->next_bridge)) {
> > +               DRM_ERROR("failed to create panel bridge\n");
> > +               ret = PTR_ERR(pdata->next_bridge);
> > +               return ret;
> 
> Just return PTR_ERR(pdata->next_bridge)?

Indeed. Bad copy and paste.

> > +       }
> > +
> >         dev_set_drvdata(&client->dev, pdata);
Doug Anderson March 24, 2021, 10:44 p.m. UTC | #4
Hi,

On Sun, Mar 21, 2021 at 8:02 PM Laurent Pinchart
<laurent.pinchart+renesas@ideasonboard.com> wrote:
>
> 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>
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 30 +++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 1d1be791d5ba..c21a7f7d452b 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -124,6 +124,7 @@
>   * @edid:         Detected EDID of eDP panel.
>   * @refclk:       Our reference clock.
>   * @panel:        Our panel.
> + * @next_bridge:  The next bridge.

To make it easier for folks who don't work with DRM all day, could you
somehow clarify which direction "next" is talking about. AKA the next
"outward" (towards the sink) or the next "inward" (toward the source)?


>   * @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.
> @@ -152,6 +153,7 @@ struct ti_sn_bridge {
>         struct mipi_dsi_device          *dsi;
>         struct clk                      *refclk;
>         struct drm_panel                *panel;
> +       struct drm_bridge               *next_bridge;

There's no reason to store the "panel" pointer anymore, right? It can
just be a local variable in probe?


> @@ -850,8 +856,6 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
>          */
>         regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
>                            HPD_DISABLE);
> -
> -       drm_panel_prepare(pdata->panel);

Ugh, I guess conflicts with my EDID patch [1] which assumes that this
function will directly turn the panel on. I'll see if I can find some
solution...

[1] https://lore.kernel.org/r/20210304155144.3.I60a7fb23ce4589006bc95c64ab8d15c74b876e68@changeid/
Laurent Pinchart March 26, 2021, 1:06 a.m. UTC | #5
Hi Doug,

On Wed, Mar 24, 2021 at 03:44:39PM -0700, Doug Anderson wrote:
> On Sun, Mar 21, 2021 at 8:02 PM Laurent Pinchart wrote:
> >
> > 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>
> > ---
> >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 30 +++++++++++++++++++--------
> >  1 file changed, 21 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index 1d1be791d5ba..c21a7f7d452b 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > @@ -124,6 +124,7 @@
> >   * @edid:         Detected EDID of eDP panel.
> >   * @refclk:       Our reference clock.
> >   * @panel:        Our panel.
> > + * @next_bridge:  The next bridge.
> 
> To make it easier for folks who don't work with DRM all day, could you
> somehow clarify which direction "next" is talking about. AKA the next
> "outward" (towards the sink) or the next "inward" (toward the source)?

Sure, I'll expand the comment.

> >   * @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.
> > @@ -152,6 +153,7 @@ struct ti_sn_bridge {
> >         struct mipi_dsi_device          *dsi;
> >         struct clk                      *refclk;
> >         struct drm_panel                *panel;
> > +       struct drm_bridge               *next_bridge;
> 
> There's no reason to store the "panel" pointer anymore, right? It can
> just be a local variable in probe?

Good point, I'll fix that.

> > @@ -850,8 +856,6 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
> >          */
> >         regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
> >                            HPD_DISABLE);
> > -
> > -       drm_panel_prepare(pdata->panel);
> 
> Ugh, I guess conflicts with my EDID patch [1] which assumes that this
> function will directly turn the panel on. I'll see if I can find some
> solution...
> 
> [1] https://lore.kernel.org/r/20210304155144.3.I60a7fb23ce4589006bc95c64ab8d15c74b876e68@changeid/

Would using the drm_bridge_connector help ? It's a helper that creates a
connector based on a chain of bridges. It implements the .get_modes()
connector operation (see drm_bridge_connector_get_modes()), based on the
.get_edid() operation provided by the bridges. As it has a full view of
the chain, it could enable bridges prior to reading the EDID, and then
power them off, including the panel-bridge.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 1d1be791d5ba..c21a7f7d452b 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -124,6 +124,7 @@ 
  * @edid:         Detected EDID of eDP panel.
  * @refclk:       Our reference clock.
  * @panel:        Our panel.
+ * @next_bridge:  The next bridge.
  * @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.
@@ -152,6 +153,7 @@  struct ti_sn_bridge {
 	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;
@@ -287,7 +289,7 @@  static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector)
 		}
 	}
 
-	return drm_panel_get_modes(pdata->panel, connector);
+	return drm_bridge_get_modes(pdata->next_bridge, connector);
 }
 
 static enum drm_mode_status
@@ -418,8 +420,18 @@  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) {
+		DRM_ERROR("failed to attach next bridge\n");
+		goto err_dsi_detach;
+	}
+
 	return 0;
 
+err_dsi_detach:
+	mipi_dsi_detach(dsi);
 err_dsi_attach:
 	mipi_dsi_device_unregister(dsi);
 err_dsi_host:
@@ -431,16 +443,12 @@  static void ti_sn_bridge_disable(struct drm_bridge *bridge)
 {
 	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(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 */
 	regmap_write(pdata->regmap, SN_ML_TX_MODE_REG, 0);
 	/* disable DP PLL */
 	regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 0);
-
-	drm_panel_unprepare(pdata->panel);
 }
 
 static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn_bridge *pdata)
@@ -819,8 +827,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)
@@ -850,8 +856,6 @@  static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
 	 */
 	regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
 			   HPD_DISABLE);
-
-	drm_panel_prepare(pdata->panel);
 }
 
 static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
@@ -1245,6 +1249,14 @@  static int ti_sn_bridge_probe(struct i2c_client *client,
 		return ret;
 	}
 
+	pdata->next_bridge = devm_drm_panel_bridge_add(pdata->dev,
+						       pdata->panel);
+	if (IS_ERR(pdata->next_bridge)) {
+		DRM_ERROR("failed to create panel bridge\n");
+		ret = PTR_ERR(pdata->next_bridge);
+		return ret;
+	}
+
 	dev_set_drvdata(&client->dev, pdata);
 
 	pdata->enable_gpio = devm_gpiod_get_optional(pdata->dev, "enable",