| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAOf5uwmn4UM8iE71DjcGpX+pQU_wkU6bBNV-=b6kT-x-LtsnMg@mail.gmail.com>
Date: Fri, 10 Dec 2021 10:05:42 +0100
From: Michael Nazzareno Trimarchi <michael@...rulasolutions.com>
To: Dave Stevenson <dave.stevenson@...pberrypi.com>
Cc: Thierry Reding <thierry.reding@...il.com>,
Sam Ravnborg <sam@...nborg.org>,
David Airlie <airlied@...ux.ie>,
DRI Development <dri-devel@...ts.freedesktop.org>,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] drm/panel: ilitek-ili9881c: Avoid unbalance prepare/unprepare
Hi Dave
some questions below
On Thu, Dec 9, 2021 at 7:10 PM Michael Nazzareno Trimarchi
<michael@...rulasolutions.com> wrote:
>
> Hi Dave
>
> On Thu, Dec 9, 2021 at 6:58 PM Dave Stevenson
> <dave.stevenson@...pberrypi.com> wrote:
> >
> > Hi Michael
> >
> > On Thu, 9 Dec 2021 at 16:58, Michael Nazzareno Trimarchi
> > <michael@...rulasolutions.com> wrote:
> > >
> > > Hi all
> > >
> > > On Sat, Oct 16, 2021 at 4:58 PM Michael Trimarchi
> > > <michael@...rulasolutions.com> wrote:
> > > >
> > > > All the panel driver check the fact that their prepare/unprepare
> > > > call was already called. It's not an ideal solution but fix
> > > > for now the problem on ili9881c
> > > >
> > > > [ 9862.283296] ------------[ cut here ]------------
> > > > [ 9862.288490] unbalanced disables for vcc3v3_lcd
> > > > [ 9862.293555] WARNING: CPU: 0 PID: 1 at drivers/regulator/core.c:2851
> > > > _regulator_disable+0xd4/0x190
> > > >
> > > > from:
> > > >
> > > > [ 9862.038619] drm_panel_unprepare+0x2c/0x4c
> > > > [ 9862.043212] panel_bridge_post_disable+0x18/0x24
> > > > [ 9862.048390] dw_mipi_dsi_bridge_post_disable+0x3c/0xf0
> > > > [ 9862.054153] drm_atomic_bridge_chain_post_disable+0x8c/0xd0
> > > >
> > > > and:
> > > >
> > > > [ 9862.183103] drm_panel_unprepare+0x2c/0x4c
> > > > [ 9862.187695] panel_bridge_post_disable+0x18/0x24
> > > > [ 9862.192872] drm_atomic_bridge_chain_post_disable+0x8c/0xd0
> > > > [ 9862.199117] disable_outputs+0x120/0x31c
> >
> > This is down to the dw-mipi-dsi driver calling the post_disable hook
> > explicitly at [1], but then also allowing the framework to call it.
> > The explicit call is down to limitations in the DSI support, so we
> > can't control the DSI host state to a fine enough degree (an ongoing
> > discussion [2] [3]). There shouldn't be a need to handle mismatched
> > calling in individual panel drivers.
> >
> > Dave
> >
> > [1] https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L894
> > [2] https://lists.freedesktop.org/archives/dri-devel/2021-November/332060.html
> > [3] https://lists.freedesktop.org/archives/dri-devel/2021-December/334007.html
>
> I'm in the second case. I need to enable HS mode after the panel is
> initialized. Time to time I have timeout
> on dsi command or I have wrong panel initialization. So I explicit call from
> the bridge but I understand that is not correct in the design point of view.
>
> So this patch can not be queued because it's a known problem that
> people are discussing
>
Author: Michael Trimarchi <michael@...rulasolutions.com>
Date: Thu Dec 9 15:45:48 2021 +0100
drm: bridge: samsung-dsim: Enable panel/bridge before exist from standby
We need to exist from standby as last operation to have a proper video
working. This code implement the same code was before the bridge
migration
Signed-off-by: Michael Trimarchi <michael@...rulasolutions.com>
diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
b/drivers/gpu/drm/bridge/samsung-dsim.c
index 654851edbd9b..21265ae80022 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -1838,6 +1838,7 @@ static void samsung_dsim_atomic_enable(struct
drm_bridge *bridge,
struct drm_bridge_state
*old_bridge_state)
{
struct samsung_dsim *dsi = bridge_to_dsi(bridge);
+ struct drm_atomic_state old_state;
int ret;
if (dsi->state & DSIM_STATE_ENABLED)
@@ -1859,6 +1860,9 @@ static void samsung_dsim_atomic_enable(struct
drm_bridge *bridge,
}
samsung_dsim_set_display_mode(dsi);
+
+ drm_atomic_bridge_chain_enable(dsi->out_bridge, &old_state);
+
samsung_dsim_set_display_enable(dsi, true);
dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
Right now I'm doing this to enable the change. I must change the panel
to avoid double enabled
I have some questions:
- the chain is an element (bridge/panel) linked together via some
connector (I hope I understand) when I enable
a bridge chain, all the elements should move from some status to
another. If we mark them already this should
not avoid that one element can be enabled two times? An element that
sources two other elements should for instance
receive the enable from two times before switching on.
Michael
> Michael
>
> >
> >
> > > > Signed-off-by: Michael Trimarchi <michael@...rulasolutions.com>
> > > > ---
> > > > drivers/gpu/drm/panel/panel-ilitek-ili9881c.c | 14 ++++++++++++++
> > > > 1 file changed, 14 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c b/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c
> > > > index 103a16018975..f75eecb0e65c 100644
> > > > --- a/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c
> > > > +++ b/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c
> > > > @@ -52,6 +52,8 @@ struct ili9881c {
> > > >
> > > > struct regulator *power;
> > > > struct gpio_desc *reset;
> > > > +
> > > > + bool prepared;
> > > > };
> > > >
> > >
> > > I found that this can be a general problem. Should not mandatory to
> > > track panel status
> > >
> > > DRM_PANEL_PREPARED
> > > DRM_PANEL_ENABLED
> > >
> > > Michael
> > > > #define ILI9881C_SWITCH_PAGE_INSTR(_page) \
> > > > @@ -707,6 +709,10 @@ static int ili9881c_prepare(struct drm_panel *panel)
> > > > unsigned int i;
> > > > int ret;
> > > >
> > > > + /* Preparing when already prepared is a no-op */
> > > > + if (ctx->prepared)
> > > > + return 0;
> > > > +
> > > > /* Power the panel */
> > > > ret = regulator_enable(ctx->power);
> > > > if (ret)
> > > > @@ -745,6 +751,8 @@ static int ili9881c_prepare(struct drm_panel *panel)
> > > > if (ret)
> > > > return ret;
> > > >
> > > > + ctx->prepared = true;
> > > > +
> > > > return 0;
> > > > }
> > > >
> > > > @@ -770,10 +778,16 @@ static int ili9881c_unprepare(struct drm_panel *panel)
> > > > {
> > > > struct ili9881c *ctx = panel_to_ili9881c(panel);
> > > >
> > > > + /* Unpreparing when already unprepared is a no-op */
> > > > + if (!ctx->prepared)
> > > > + return 0;
> > > > +
> > > > mipi_dsi_dcs_enter_sleep_mode(ctx->dsi);
> > > > regulator_disable(ctx->power);
> > > > gpiod_set_value(ctx->reset, 1);
> > > >
> > > > + ctx->prepared = false;
> > > > +
> > > > return 0;
> > > > }
> > > >
> > > > --
> > > > 2.25.1
> > > >
> > >
> > >
> > > --
> > > Michael Nazzareno Trimarchi
> > > Co-Founder & Chief Executive Officer
> > > M. +39 347 913 2170
> > > michael@...rulasolutions.com
> > > __________________________________
> > >
> > > Amarula Solutions BV
> > > Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> > > T. +31 (0)85 111 9172
> > > info@...rulasolutions.com
> > > www.amarulasolutions.com
>
>
>
> --
> Michael Nazzareno Trimarchi
> Co-Founder & Chief Executive Officer
> M. +39 347 913 2170
> michael@...rulasolutions.com
> __________________________________
>
> Amarula Solutions BV
> Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> T. +31 (0)85 111 9172
> info@...rulasolutions.com
> www.amarulasolutions.com
--
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
michael@...rulasolutions.com
__________________________________
Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
info@...rulasolutions.com
www.amarulasolutions.com
Powered by blists - more mailing lists