From: Boris Brezillon <boris.brezillon@collabora.com>
To: Marek Szyprowski <m.szyprowski@samsung.com>
Cc: dri-devel@lists.freedesktop.org,
linux-samsung-soc@vger.kernel.org, linux-kernel@vger.kernel.org,
Inki Dae <inki.dae@samsung.com>,
Seung-Woo Kim <sw0312.kim@samsung.com>,
Andrzej Hajda <a.hajda@samsung.com>,
Krzysztof Kozlowski <krzk@kernel.org>,
Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>,
Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
Neil Armstrong <narmstrong@baylibre.com>,
Sam Ravnborg <sam@ravnborg.org>,
Thierry Reding <thierry.reding@gmail.com>
Subject: Re: [PATCH] drm/bridge: Fix Exynos DSI after making bridge chain a double-linked list
Date: Fri, 27 Dec 2019 14:23:13 +0100 [thread overview]
Message-ID: <20191227142313.5032ff0f@collabora.com> (raw)
In-Reply-To: <20191227130004.69d7dcad@collabora.com>
On Fri, 27 Dec 2019 13:00:04 +0100
Boris Brezillon <boris.brezillon@collabora.com> wrote:
> On Fri, 27 Dec 2019 12:01:35 +0100
> Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>
> > Exynos DSI DRM driver uses private calls to out bridge to force certain
> > order of operations during init/exit sequences. This no longer works after
> > conversion of bridge chain to a double-linked list. To fix the regression
> > call bridge related operations manually instead of the generic
> > drm_bridge_chain_*() operations.
>
> I think it'd be worth explaining what the problem is (infinite loop
> caused by list_for_each_entry() use when the bridge is no longer part
> of the chain attached to the encoder).
>
> >
> > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked list")
>
> We also need to fix that in VC4.
>
> > ---
> > This patch is a result of the following discussion:
> > https://www.spinics.net/lists/dri-devel/msg239256.html
> > ---
> > drivers/gpu/drm/exynos/exynos_drm_dsi.c | 21 ++++++++++++---------
> > 1 file changed, 12 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> > index 3955f84dc893..f5905c239a86 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> > @@ -255,7 +255,6 @@ struct exynos_dsi {
> > struct mipi_dsi_host dsi_host;
> > struct drm_connector connector;
> > struct drm_panel *panel;
> > - struct list_head bridge_chain;
> > struct drm_bridge *out_bridge;
> > struct device *dev;
> >
> > @@ -1391,7 +1390,8 @@ 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);
> > + if (dsi->out_bridge->funcs->pre_enable)
> > + dsi->out_bridge->funcs->pre_enable(dsi->out_bridge);
>
> Okay, so you're calling ->{pre_enable,enable,disable,post_disable}() on
> the first bridge element which only works if the chain contains one
> bridge (see below). Maybe you should keep exynos_dsi.bridge_chain and
> create custom helpers to iterate over chain elements instead of calling
> those hooks only on out_bridge.
The following diff should fix the problem while keeping the solution
generic enough to support chains containing more than one bridge.
--->8---
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index 3955f84dc893..c861b640fc59 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, &encoder->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, &encoder->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, &encoder->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, &encoder->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);
next prev parent reply other threads:[~2019-12-27 13:23 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <CGME20191227110216eucas1p17cbf91afa905852d3c0b1efeec0f6f8d@eucas1p1.samsung.com>
2019-12-27 11:01 ` [PATCH] drm/bridge: Fix Exynos DSI after making bridge chain a double-linked list Marek Szyprowski
2019-12-27 12:00 ` Boris Brezillon
2019-12-27 13:23 ` Boris Brezillon [this message]
2019-12-27 14:03 ` Boris Brezillon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191227142313.5032ff0f@collabora.com \
--to=boris.brezillon@collabora.com \
--cc=a.hajda@samsung.com \
--cc=b.zolnierkie@samsung.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=inki.dae@samsung.com \
--cc=krzk@kernel.org \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-samsung-soc@vger.kernel.org \
--cc=m.szyprowski@samsung.com \
--cc=narmstrong@baylibre.com \
--cc=sam@ravnborg.org \
--cc=sw0312.kim@samsung.com \
--cc=thierry.reding@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).