Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <1594137527-15201-1-git-send-email-martin.fuzzey@flowbird.group> <a55b91f01a2e795fe2dd38d860e63a63c8c8871c.camel@pengutronix.de>
In-Reply-To: <a55b91f01a2e795fe2dd38d860e63a63c8c8871c.camel@pengutronix.de>
From:   "Fuzzey, Martin" <martin.fuzzey@flowbird.group>
Date:   Wed, 8 Jul 2020 18:53:59 +0200
Message-ID: <CANh8QzwEgVG5AvJ4s7ydfPMDKpK5pb0TSaz9i9dgZ+1ZMJw3vQ@mail.gmail.com>
Subject: Re: [PATCH] drm: imx: Fix occasional screen corruption on modeset.
To:     Philipp Zabel <p.zabel@pengutronix.de>
Cc:     Shawn Guo <shawnguo@kernel.org>,
        Sascha Hauer <s.hauer@pengutronix.de>,
        ML dri-devel <dri-devel@lists.freedesktop.org>,
        "Linux-Kernel@Vger. Kernel. Org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Philipp,

thanks for the quick reply.

On Wed, 8 Jul 2020 at 10:31, Philipp Zabel <p.zabel@pengutronix.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