linux-kernel - Re: [PATCH] drm: imx: Fix occasional screen corruption on modeset.

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 8 Jul 2020 18:53:59 +0200
From:   "Fuzzey, Martin" <martin.fuzzey@...wbird.group>
To:     Philipp Zabel <p.zabel@...gutronix.de>
Cc:     Shawn Guo <shawnguo@...nel.org>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        ML dri-devel <dri-devel@...ts.freedesktop.org>,
        "Linux-Kernel@...r. Kernel. Org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] drm: imx: Fix occasional screen corruption on modeset.

Hi Philipp,

thanks for the quick reply.

On Wed, 8 Jul 2020 at 10:31, Philipp Zabel <p.zabel@...gutronix.de> wrote:

> Could this be just a panel getting confused because the pixel clock is
> disabled, or is there really an issue with the IPU? Have you tried just
> keeping clk_di_pixel enabled in ipu_di_disable(), but continuing
> to disable DI and DC?
>

I've  tried that now but still observed the corruption.

> > The exact reason isn't understood but it seems timing related.
> >

Also just adding a msleep(2) or a mdelay(20) at the end of
ipu_crtc_atomic_disable() makes the problem go away too.
Obviously I'm not suggesting doing that, just that it may help
understand what is going on.

>
> Just removing ipu_di_disable() leaks a clk_prepare_enable refcount on
> the di->clk_di_pixel clock.
>
> Also this is followed by an ipu_dc_disable(), which should remove the DC
> module's clock if this is the only display. So the DC should be disabled
> anyway.
>

True.

How about doing just ipu_crtc_disable_planes() and
drm_crtc_vblank_off() in the active (modeset) case.
and in ipu_crtc_atomic_enable() only doing the stuff  (which doesn't
touch the planes) if old_state->active == false ?

That will fix the clock refcount problem and seems generally better as
tearing down everything just to do a modeset
seems a bit heavy handed.

I've tested that and it works too but it's probably better to discuss
this some more before sending a new patch.

Regards,

Martin