[PATCH 1/3] drm/bridge: Fix drm_bridge_chain_pre_enable()

[PATCH 1/3] drm/bridge: Fix drm_bridge_chain_pre_enable()

[Boris Brezillon]

Stop iterating on the bridge chain when we reach the bridge element.
That's what other helpers do and should allow bridge implementations
to execute a pre_enable operation on a sub-chain.

Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked list")
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/gpu/drm/drm_bridge.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index c2cf0c90fa26..b3b269ec6a39 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -357,6 +357,9 @@ void drm_bridge_chain_pre_enable(struct drm_bridge *bridge)
 	list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
 		if (iter->funcs->pre_enable)
 			iter->funcs->pre_enable(iter);
+
+		if (iter == bridge)
+			break;
 	}
 }
 EXPORT_SYMBOL(drm_bridge_chain_pre_enable);
-- 
2.23.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/3] drm/vc4: dsi: Fix bridge chain handling

[Boris Brezillon]

Commit 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked
list") patched the bridge chain logic to use a double-linked list instead
of a single-linked list. This change induced changes to the VC4 driver
which was manually resetting the encoder->bridge element to NULL to
control the enable/disable sequence of the bridge chain. During this
conversion, 2 bugs were introduced:

1/ list_splice() was used to move chain elements to our own internal
   chain, but list_splice() does not reset the source list to an empty
   state, leading to unexpected bridge hook calls when
   drm_bridge_chain_xxx() helpers were called by the core. Replacing
   those list_splice() calls by list_splice_init() ones fixes this
   problem.

2/ drm_bridge_chain_xxx() helpers operate on the
   bridge->encoder->bridge_chain list, which is now empty. When the
   helper uses list_for_each_entry_reverse() we end up with no operation
   done which is not what we want. But that's even worse when the helper
   uses list_for_each_entry_from(), because in that case we end up in
   an infinite loop searching for the list head element which is no
   longer encoder->bridge_chain but vc4_dsi->bridge_chain. To address
   that problem we stop using the bridge chain helpers and call the
   hooks directly.

Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
Cc: Eric Anholt <eric@anholt.net>
Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked list")
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/gpu/drm/vc4/vc4_dsi.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
index 6c5b80ad6154..fd8a2eb60505 100644
--- a/drivers/gpu/drm/vc4/vc4_dsi.c
+++ b/drivers/gpu/drm/vc4/vc4_dsi.c
@@ -753,10 +753,19 @@ static void vc4_dsi_encoder_disable(struct drm_encoder *encoder)
 	struct vc4_dsi_encoder *vc4_encoder = to_vc4_dsi_encoder(encoder);
 	struct vc4_dsi *dsi = vc4_encoder->dsi;
 	struct device *dev = &dsi->pdev->dev;
+	struct drm_bridge *iter;
+
+	list_for_each_entry_reverse(iter, &dsi->bridge_chain, chain_node) {
+		if (iter->funcs->disable)
+			iter->funcs->disable(iter);
+	}
 
-	drm_bridge_chain_disable(dsi->bridge);
 	vc4_dsi_ulps(dsi, true);
-	drm_bridge_chain_post_disable(dsi->bridge);
+
+	list_for_each_entry_from(iter, &dsi->bridge_chain, chain_node) {
+		if (iter->funcs->post_disable)
+			iter->funcs->post_disable(iter);
+	}
 
 	clk_disable_unprepare(dsi->pll_phy_clock);
 	clk_disable_unprepare(dsi->escape_clock);
@@ -824,6 +833,7 @@ static void vc4_dsi_encoder_enable(struct drm_encoder *encoder)
 	struct vc4_dsi *dsi = vc4_encoder->dsi;
 	struct device *dev = &dsi->pdev->dev;
 	bool debug_dump_regs = false;
+	struct drm_bridge *iter;
 	unsigned long hs_clock;
 	u32 ui_ns;
 	/* Minimum LP state duration in escape clock cycles. */
@@ -1056,7 +1066,10 @@ static void vc4_dsi_encoder_enable(struct drm_encoder *encoder)
 
 	vc4_dsi_ulps(dsi, false);
 
-	drm_bridge_chain_pre_enable(dsi->bridge);
+	list_for_each_entry_reverse(iter, &dsi->bridge_chain, chain_node) {
+		if (iter->funcs->pre_enable)
+			iter->funcs->pre_enable(iter);
+	}
 
 	if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO) {
 		DSI_PORT_WRITE(DISP0_CTRL,
@@ -1073,7 +1086,10 @@ static void vc4_dsi_encoder_enable(struct drm_encoder *encoder)
 			       DSI_DISP0_ENABLE);
 	}
 
-	drm_bridge_chain_enable(dsi->bridge);
+	list_for_each_entry(iter, &dsi->bridge_chain, chain_node) {
+		if (iter->funcs->enable)
+			iter->funcs->enable(iter);
+	}
 
 	if (debug_dump_regs) {
 		struct drm_printer p = drm_info_printer(&dsi->pdev->dev);
@@ -1613,7 +1629,7 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data)
 	 * from our driver, since we need to sequence them within the
 	 * encoder's enable/disable paths.
 	 */
-	list_splice(&dsi->encoder->bridge_chain, &dsi->bridge_chain);
+	list_splice_init(&dsi->encoder->bridge_chain, &dsi->bridge_chain);
 
 	if (dsi->port == 0)
 		vc4_debugfs_add_regset32(drm, "dsi0_regs", &dsi->regset);
@@ -1639,7 +1655,7 @@ static void vc4_dsi_unbind(struct device *dev, struct device *master,
 	 * Restore the bridge_chain so the bridge detach procedure can happen
 	 * normally.
 	 */
-	list_splice(&dsi->bridge_chain, &dsi->encoder->bridge_chain);
+	list_splice_init(&dsi->bridge_chain, &dsi->encoder->bridge_chain);
 	vc4_dsi_encoder_destroy(dsi->encoder);
 
 	if (dsi->port == 1)
-- 
2.23.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/3] drm/exynos: dsi: Fix bridge chain handling

[Boris Brezillon]

Commit 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked
list") patched the bridge chain logic to use a double-linked list instead
of a single-linked list. This change induced changes to the Exynos driver
which was manually resetting the encoder->bridge element to NULL to
control the enable/disable sequence of the bridge chain. During this
conversion, 2 bugs were introduced:

1/ list_splice() was used to move chain elements to our own internal
   chain, but list_splice() does not reset the source list to an empty
   state, leading to unexpected bridge hook calls when
   drm_bridge_chain_xxx() helpers were called by the core. Replacing
   the list_splice() call by list_splice_init() fixes this problem.

2/ drm_bridge_chain_xxx() helpers operate on the
   bridge->encoder->bridge_chain list, which is now empty. When the
   helper uses list_for_each_entry_reverse() we end up with no operation
   done which is not what we want. But that's even worse when the helper
   uses list_for_each_entry_from(), because in that case we end up in
   an infinite loop searching for the list head element which is no
   longer encoder->bridge_chain but exynos_dsi->bridge_chain. To address
   that problem we stop using the bridge chain helpers and call the
   hooks directly.

Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked list")
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
Hello Marek,

I'm perfectly fine applying your patch instead of this one if you prefer
to restrict the logic to a single bridge per chain. I just sent this
patch in case your okay with the slightly different version I propose
here.

Let me know what you want to do.

Regards,

Boris
---
 drivers/gpu/drm/exynos/exynos_drm_dsi.c | 29 ++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index 3955f84dc893..33628d85edad 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -1378,6 +1378,7 @@ static void exynos_dsi_unregister_te_irq(struct exynos_dsi *dsi)
 static void exynos_dsi_enable(struct drm_encoder *encoder)
 {
 	struct exynos_dsi *dsi = encoder_to_dsi(encoder);
+	struct drm_bridge *iter;
 	int ret;
 
 	if (dsi->state & DSIM_STATE_ENABLED)
@@ -1391,7 +1392,11 @@ static void exynos_dsi_enable(struct drm_encoder *encoder)
 		if (ret < 0)
 			goto err_put_sync;
 	} else {
-		drm_bridge_chain_pre_enable(dsi->out_bridge);
+		list_for_each_entry_reverse(iter, &dsi->bridge_chain,
+					    chain_node) {
+			if (iter->funcs->pre_enable)
+				iter->funcs->pre_enable(iter);
+		}
 	}
 
 	exynos_dsi_set_display_mode(dsi);
@@ -1402,7 +1407,10 @@ static void exynos_dsi_enable(struct drm_encoder *encoder)
 		if (ret < 0)
 			goto err_display_disable;
 	} else {
-		drm_bridge_chain_enable(dsi->out_bridge);
+		list_for_each_entry(iter, &dsi->bridge_chain, chain_node) {
+			if (iter->funcs->enable)
+				iter->funcs->enable(iter);
+		}
 	}
 
 	dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
@@ -1420,6 +1428,7 @@ static void exynos_dsi_enable(struct drm_encoder *encoder)
 static void exynos_dsi_disable(struct drm_encoder *encoder)
 {
 	struct exynos_dsi *dsi = encoder_to_dsi(encoder);
+	struct drm_bridge *iter;
 
 	if (!(dsi->state & DSIM_STATE_ENABLED))
 		return;
@@ -1427,10 +1436,20 @@ static void exynos_dsi_disable(struct drm_encoder *encoder)
 	dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
 
 	drm_panel_disable(dsi->panel);
-	drm_bridge_chain_disable(dsi->out_bridge);
+
+	list_for_each_entry_reverse(iter, &dsi->bridge_chain, chain_node) {
+		if (iter->funcs->disable)
+			iter->funcs->disable(iter);
+	}
+
 	exynos_dsi_set_display_enable(dsi, false);
 	drm_panel_unprepare(dsi->panel);
-	drm_bridge_chain_post_disable(dsi->out_bridge);
+
+	list_for_each_entry(iter, &dsi->bridge_chain, chain_node) {
+		if (iter->funcs->post_disable)
+			iter->funcs->post_disable(iter);
+	}
+
 	dsi->state &= ~DSIM_STATE_ENABLED;
 	pm_runtime_put_sync(dsi->dev);
 }
@@ -1523,7 +1542,7 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
 	if (out_bridge) {
 		drm_bridge_attach(encoder, out_bridge, NULL);
 		dsi->out_bridge = out_bridge;
-		list_splice(&encoder->bridge_chain, &dsi->bridge_chain);
+		list_splice_init(&encoder->bridge_chain, &dsi->bridge_chain);
 	} else {
 		int ret = exynos_dsi_create_connector(encoder);
 
-- 
2.23.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] drm/exynos: dsi: Fix bridge chain handling

[Boris Brezillon]

On Fri, 27 Dec 2019 15:41:24 +0100
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> Commit 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked
> list") patched the bridge chain logic to use a double-linked list instead
> of a single-linked list. This change induced changes to the Exynos driver
> which was manually resetting the encoder->bridge element to NULL to
> control the enable/disable sequence of the bridge chain. During this
> conversion, 2 bugs were introduced:
> 
> 1/ list_splice() was used to move chain elements to our own internal
>    chain, but list_splice() does not reset the source list to an empty
>    state, leading to unexpected bridge hook calls when
>    drm_bridge_chain_xxx() helpers were called by the core. Replacing
>    the list_splice() call by list_splice_init() fixes this problem.
> 
> 2/ drm_bridge_chain_xxx() helpers operate on the
>    bridge->encoder->bridge_chain list, which is now empty. When the
>    helper uses list_for_each_entry_reverse() we end up with no operation
>    done which is not what we want. But that's even worse when the helper
>    uses list_for_each_entry_from(), because in that case we end up in
>    an infinite loop searching for the list head element which is no
>    longer encoder->bridge_chain but exynos_dsi->bridge_chain. To address
>    that problem we stop using the bridge chain helpers and call the
>    hooks directly.
> 
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked list")
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
> Hello Marek,
> 
> I'm perfectly fine applying your patch instead of this one if you prefer
> to restrict the logic to a single bridge per chain. I just sent this
> patch in case your okay with the slightly different version I propose
> here.

Marek, Andrzej, did you have time to look at this patch (or respin
"drm/bridge: Fix Exynos DSI after making bridge chain a  double-linked
list" if you don't like this version)?

I'd really like to apply this fix (and its vc4 equivalent) to
drm-misc-next as soon as possible.

Thanks,

Boris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm/bridge: Fix drm_bridge_chain_pre_enable()

[Laurent Pinchart]

Hi Boris,

Thank you for the patch.

On Fri, Dec 27, 2019 at 03:41:22PM +0100, Boris Brezillon wrote:
> Stop iterating on the bridge chain when we reach the bridge element.
> That's what other helpers do and should allow bridge implementations
> to execute a pre_enable operation on a sub-chain.

The code looks fine to me, but I think you should update the
documentation to explain this. It currently states:

 * Calls &drm_bridge_funcs.pre_enable op for all the bridges in the encoder
 * chain, starting from the last bridge to the first. These are called
 * before calling the encoder's commit op.
 *
 * Note: the bridge passed should be the one closest to the encoder

I suggest stating instead that the operation is called from the last
bridge to the bridge passed as the argument. The note should then either
be removed, or updated to state that bridge is usually the bridge
closest to the encoder, but can be any other bridge if the caller only
wants to execute the operation on a subset of the chain. It's also
probably worth it updating the other functions accordingly.

> Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked list")
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
>  drivers/gpu/drm/drm_bridge.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index c2cf0c90fa26..b3b269ec6a39 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -357,6 +357,9 @@ void drm_bridge_chain_pre_enable(struct drm_bridge *bridge)
>  	list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
>  		if (iter->funcs->pre_enable)
>  			iter->funcs->pre_enable(iter);
> +
> +		if (iter == bridge)
> +			break;
>  	}
>  }
>  EXPORT_SYMBOL(drm_bridge_chain_pre_enable);

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] drm/exynos: dsi: Fix bridge chain handling

[Marek Szyprowski]

Hi Boris,

Sorry, for the late reply, I've just got back from my prolonged Chrismas 
holidays.

On 27.12.2019 15:41, Boris Brezillon wrote:
> Commit 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked
> list") patched the bridge chain logic to use a double-linked list instead
> of a single-linked list. This change induced changes to the Exynos driver
> which was manually resetting the encoder->bridge element to NULL to
> control the enable/disable sequence of the bridge chain. During this
> conversion, 2 bugs were introduced:
>
> 1/ list_splice() was used to move chain elements to our own internal
>     chain, but list_splice() does not reset the source list to an empty
>     state, leading to unexpected bridge hook calls when
>     drm_bridge_chain_xxx() helpers were called by the core. Replacing
>     the list_splice() call by list_splice_init() fixes this problem.
>
> 2/ drm_bridge_chain_xxx() helpers operate on the
>     bridge->encoder->bridge_chain list, which is now empty. When the
>     helper uses list_for_each_entry_reverse() we end up with no operation
>     done which is not what we want. But that's even worse when the helper
>     uses list_for_each_entry_from(), because in that case we end up in
>     an infinite loop searching for the list head element which is no
>     longer encoder->bridge_chain but exynos_dsi->bridge_chain. To address
>     that problem we stop using the bridge chain helpers and call the
>     hooks directly.
>
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked list")
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>

Works fine on Exynos5250-based Arndale board.

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

> ---
> Hello Marek,
>
> I'm perfectly fine applying your patch instead of this one if you prefer
> to restrict the logic to a single bridge per chain. I just sent this
> patch in case your okay with the slightly different version I propose
> here.
>
> Let me know what you want to do.

I'm fine with your approach.

> Regards,
>
> Boris
> ---
>   drivers/gpu/drm/exynos/exynos_drm_dsi.c | 29 ++++++++++++++++++++-----
>   1 file changed, 24 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> index 3955f84dc893..33628d85edad 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> @@ -1378,6 +1378,7 @@ static void exynos_dsi_unregister_te_irq(struct exynos_dsi *dsi)
>   static void exynos_dsi_enable(struct drm_encoder *encoder)
>   {
>   	struct exynos_dsi *dsi = encoder_to_dsi(encoder);
> +	struct drm_bridge *iter;
>   	int ret;
>   
>   	if (dsi->state & DSIM_STATE_ENABLED)
> @@ -1391,7 +1392,11 @@ static void exynos_dsi_enable(struct drm_encoder *encoder)
>   		if (ret < 0)
>   			goto err_put_sync;
>   	} else {
> -		drm_bridge_chain_pre_enable(dsi->out_bridge);
> +		list_for_each_entry_reverse(iter, &dsi->bridge_chain,
> +					    chain_node) {
> +			if (iter->funcs->pre_enable)
> +				iter->funcs->pre_enable(iter);
> +		}
>   	}
>   
>   	exynos_dsi_set_display_mode(dsi);
> @@ -1402,7 +1407,10 @@ static void exynos_dsi_enable(struct drm_encoder *encoder)
>   		if (ret < 0)
>   			goto err_display_disable;
>   	} else {
> -		drm_bridge_chain_enable(dsi->out_bridge);
> +		list_for_each_entry(iter, &dsi->bridge_chain, chain_node) {
> +			if (iter->funcs->enable)
> +				iter->funcs->enable(iter);
> +		}
>   	}
>   
>   	dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
> @@ -1420,6 +1428,7 @@ static void exynos_dsi_enable(struct drm_encoder *encoder)
>   static void exynos_dsi_disable(struct drm_encoder *encoder)
>   {
>   	struct exynos_dsi *dsi = encoder_to_dsi(encoder);
> +	struct drm_bridge *iter;
>   
>   	if (!(dsi->state & DSIM_STATE_ENABLED))
>   		return;
> @@ -1427,10 +1436,20 @@ static void exynos_dsi_disable(struct drm_encoder *encoder)
>   	dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
>   
>   	drm_panel_disable(dsi->panel);
> -	drm_bridge_chain_disable(dsi->out_bridge);
> +
> +	list_for_each_entry_reverse(iter, &dsi->bridge_chain, chain_node) {
> +		if (iter->funcs->disable)
> +			iter->funcs->disable(iter);
> +	}
> +
>   	exynos_dsi_set_display_enable(dsi, false);
>   	drm_panel_unprepare(dsi->panel);
> -	drm_bridge_chain_post_disable(dsi->out_bridge);
> +
> +	list_for_each_entry(iter, &dsi->bridge_chain, chain_node) {
> +		if (iter->funcs->post_disable)
> +			iter->funcs->post_disable(iter);
> +	}
> +
>   	dsi->state &= ~DSIM_STATE_ENABLED;
>   	pm_runtime_put_sync(dsi->dev);
>   }
> @@ -1523,7 +1542,7 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
>   	if (out_bridge) {
>   		drm_bridge_attach(encoder, out_bridge, NULL);
>   		dsi->out_bridge = out_bridge;
> -		list_splice(&encoder->bridge_chain, &dsi->bridge_chain);
> +		list_splice_init(&encoder->bridge_chain, &dsi->bridge_chain);
>   	} else {
>   		int ret = exynos_dsi_create_connector(encoder);
>   

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] drm/exynos: dsi: Fix bridge chain handling

[Boris Brezillon]

Hi Marek,

On Tue, 7 Jan 2020 10:11:43 +0100
Marek Szyprowski <m.szyprowski@samsung.com> wrote:

> Hi Boris,
> 
> Sorry, for the late reply, I've just got back from my prolonged Chrismas 
> holidays.
> 
> On 27.12.2019 15:41, Boris Brezillon wrote:
> > Commit 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked
> > list") patched the bridge chain logic to use a double-linked list instead
> > of a single-linked list. This change induced changes to the Exynos driver
> > which was manually resetting the encoder->bridge element to NULL to
> > control the enable/disable sequence of the bridge chain. During this
> > conversion, 2 bugs were introduced:
> >
> > 1/ list_splice() was used to move chain elements to our own internal
> >     chain, but list_splice() does not reset the source list to an empty
> >     state, leading to unexpected bridge hook calls when
> >     drm_bridge_chain_xxx() helpers were called by the core. Replacing
> >     the list_splice() call by list_splice_init() fixes this problem.
> >
> > 2/ drm_bridge_chain_xxx() helpers operate on the
> >     bridge->encoder->bridge_chain list, which is now empty. When the
> >     helper uses list_for_each_entry_reverse() we end up with no operation
> >     done which is not what we want. But that's even worse when the helper
> >     uses list_for_each_entry_from(), because in that case we end up in
> >     an infinite loop searching for the list head element which is no
> >     longer encoder->bridge_chain but exynos_dsi->bridge_chain. To address
> >     that problem we stop using the bridge chain helpers and call the
> >     hooks directly.
> >
> > Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked list")
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>  
> 
> Works fine on Exynos5250-based Arndale board.
> 
> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

Thanks for testing, but can you also add your R-b (I need it to make
dim happy)? While you're at it, maybe you can review patch 2 (it's very
similar to this patch).
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] drm/exynos: dsi: Fix bridge chain handling

[Andrzej Hajda]

On 27.12.2019 15:41, Boris Brezillon wrote:
> Commit 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked
> list") patched the bridge chain logic to use a double-linked list instead
> of a single-linked list. This change induced changes to the Exynos driver
> which was manually resetting the encoder->bridge element to NULL to
> control the enable/disable sequence of the bridge chain. During this
> conversion, 2 bugs were introduced:
>
> 1/ list_splice() was used to move chain elements to our own internal
>    chain, but list_splice() does not reset the source list to an empty
>    state, leading to unexpected bridge hook calls when
>    drm_bridge_chain_xxx() helpers were called by the core. Replacing
>    the list_splice() call by list_splice_init() fixes this problem.
>
> 2/ drm_bridge_chain_xxx() helpers operate on the
>    bridge->encoder->bridge_chain list, which is now empty. When the
>    helper uses list_for_each_entry_reverse() we end up with no operation
>    done which is not what we want. But that's even worse when the helper
>    uses list_for_each_entry_from(), because in that case we end up in
>    an infinite loop searching for the list head element which is no
>    longer encoder->bridge_chain but exynos_dsi->bridge_chain. To address
>    that problem we stop using the bridge chain helpers and call the
>    hooks directly.
>
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked list")
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>


Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/vc4: dsi: Fix bridge chain handling

[Andrzej Hajda]

On 27.12.2019 15:41, Boris Brezillon wrote:
> Commit 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked
> list") patched the bridge chain logic to use a double-linked list instead
> of a single-linked list. This change induced changes to the VC4 driver
> which was manually resetting the encoder->bridge element to NULL to
> control the enable/disable sequence of the bridge chain. During this
> conversion, 2 bugs were introduced:
>
> 1/ list_splice() was used to move chain elements to our own internal
>    chain, but list_splice() does not reset the source list to an empty
>    state, leading to unexpected bridge hook calls when
>    drm_bridge_chain_xxx() helpers were called by the core. Replacing
>    those list_splice() calls by list_splice_init() ones fixes this
>    problem.
>
> 2/ drm_bridge_chain_xxx() helpers operate on the
>    bridge->encoder->bridge_chain list, which is now empty. When the
>    helper uses list_for_each_entry_reverse() we end up with no operation
>    done which is not what we want. But that's even worse when the helper
>    uses list_for_each_entry_from(), because in that case we end up in
>    an infinite loop searching for the list head element which is no
>    longer encoder->bridge_chain but vc4_dsi->bridge_chain. To address
>    that problem we stop using the bridge chain helpers and call the
>    hooks directly.
>
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Cc: Eric Anholt <eric@anholt.net>
> Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked list")
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm/bridge: Fix drm_bridge_chain_pre_enable()

[Andrzej Hajda]

On 06.01.2020 11:29, Laurent Pinchart wrote:
> Hi Boris,
>
> Thank you for the patch.
>
> On Fri, Dec 27, 2019 at 03:41:22PM +0100, Boris Brezillon wrote:
>> Stop iterating on the bridge chain when we reach the bridge element.
>> That's what other helpers do and should allow bridge implementations
>> to execute a pre_enable operation on a sub-chain.
> The code looks fine to me, but I think you should update the
> documentation to explain this. It currently states:
>
>  * Calls &drm_bridge_funcs.pre_enable op for all the bridges in the encoder
>  * chain, starting from the last bridge to the first. These are called
>  * before calling the encoder's commit op.
>  *
>  * Note: the bridge passed should be the one closest to the encoder
>
> I suggest stating instead that the operation is called from the last
> bridge to the bridge passed as the argument. The note should then either
> be removed, or updated to state that bridge is usually the bridge
> closest to the encoder, but can be any other bridge if the caller only
> wants to execute the operation on a subset of the chain. It's also
> probably worth it updating the other functions accordingly.


Apparently drm_(atomic_)bridge_chain_* helpers are always called on the
1st bridge so you can try to remove bridge argument, if it is true.

Moreover after patches 2 and 3 drm_bridge_chain_* helpers have no users.


Regards

Andrzej


>
>> Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked list")
>> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
>> ---
>>  drivers/gpu/drm/drm_bridge.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>> index c2cf0c90fa26..b3b269ec6a39 100644
>> --- a/drivers/gpu/drm/drm_bridge.c
>> +++ b/drivers/gpu/drm/drm_bridge.c
>> @@ -357,6 +357,9 @@ void drm_bridge_chain_pre_enable(struct drm_bridge *bridge)
>>  	list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
>>  		if (iter->funcs->pre_enable)
>>  			iter->funcs->pre_enable(iter);
>> +
>> +		if (iter == bridge)
>> +			break;
>>  	}
>>  }
>>  EXPORT_SYMBOL(drm_bridge_chain_pre_enable);


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm/bridge: Fix drm_bridge_chain_pre_enable()

[Boris Brezillon]

On Tue, 7 Jan 2020 16:27:10 +0100
Andrzej Hajda <a.hajda@samsung.com> wrote:

> On 06.01.2020 11:29, Laurent Pinchart wrote:
> > Hi Boris,
> >
> > Thank you for the patch.
> >
> > On Fri, Dec 27, 2019 at 03:41:22PM +0100, Boris Brezillon wrote:  
> >> Stop iterating on the bridge chain when we reach the bridge element.
> >> That's what other helpers do and should allow bridge implementations
> >> to execute a pre_enable operation on a sub-chain.  
> > The code looks fine to me, but I think you should update the
> > documentation to explain this. It currently states:
> >
> >  * Calls &drm_bridge_funcs.pre_enable op for all the bridges in the encoder
> >  * chain, starting from the last bridge to the first. These are called
> >  * before calling the encoder's commit op.
> >  *
> >  * Note: the bridge passed should be the one closest to the encoder
> >
> > I suggest stating instead that the operation is called from the last
> > bridge to the bridge passed as the argument. The note should then either
> > be removed, or updated to state that bridge is usually the bridge
> > closest to the encoder, but can be any other bridge if the caller only
> > wants to execute the operation on a subset of the chain. It's also
> > probably worth it updating the other functions accordingly.  
> 
> 
> Apparently drm_(atomic_)bridge_chain_* helpers are always called on the
> 1st bridge so you can try to remove bridge argument, if it is true.

You mean passing an encoder instead of a bridge? I think that's what I
initially did and was told we might want to execute operations on a
sub-chain at some point.

> 
> Moreover after patches 2 and 3 drm_bridge_chain_* helpers have no users.

Well, the core is still using it, but there's no external users, you're
right. Do you want me to stop exporting those helpers?

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel