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;
Message-ID: <a55b91f01a2e795fe2dd38d860e63a63c8c8871c.camel@pengutronix.de>
Subject: Re: [PATCH] drm: imx: Fix occasional screen corruption on modeset.
From:   Philipp Zabel <p.zabel@pengutronix.de>
To:     Martin Fuzzey <martin.fuzzey@flowbird.group>
Cc:     Shawn Guo <shawnguo@kernel.org>,
        Sascha Hauer <s.hauer@pengutronix.de>,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
Date:   Wed, 08 Jul 2020 10:31:33 +0200
In-Reply-To: <1594137527-15201-1-git-send-email-martin.fuzzey@flowbird.group>
References: <1594137527-15201-1-git-send-email-martin.fuzzey@flowbird.group>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
User-Agent: Evolution 3.30.5-1.1 
MIME-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Martin,

On Tue, 2020-07-07 at 17:56 +0200, Martin Fuzzey wrote:
> When performing a modeset the atomic core calls
> ipu_crtc_atomic_disable() which switches off the DC and DI.
> 
> When we immediately restart as in the modeset case this sometimes
> leads to corruption at the bottom of the screen looking like a mirror
> image of the top.

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?

> The exact reason isn't understood but it seems timing related.
> 
> This was observed on i.MX6DL on a system that does 2 mode
> transitions on boot (fbcon->android boot animation->android homescreen)
> then no more during normal operation resulting in corruption
> about once every 10 boots that lasted for variable durations
> from a few seconds to several hours.
> 
> Dumping the buffers confirmed that they were correct in memory,
> just the display was wrong.
> 
> For tests the problem was reproduced systematically by forcing
> a full modeset every 10 frames even if when not required.
> 
> Leaving the DC and DI on if the CRTC is staying on fixes this.
> 
> Signed-off-by: Martin Fuzzey <martin.fuzzey@flowbird.group>
> ---
>  drivers/gpu/drm/imx/ipuv3-crtc.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
> index 63c0284..9137b64 100644
> --- a/drivers/gpu/drm/imx/ipuv3-crtc.c
> +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
> @@ -84,8 +84,15 @@ static void ipu_crtc_atomic_disable(struct drm_crtc *crtc,
>  	struct ipu_crtc *ipu_crtc = to_ipu_crtc(crtc);
>  	struct ipu_soc *ipu = dev_get_drvdata(ipu_crtc->dev->parent);
>  
> -	ipu_dc_disable_channel(ipu_crtc->dc);
> -	ipu_di_disable(ipu_crtc->di);
> +	/*
> +	 * If we are just doing a modeset don't disable dc/di as that
> +	 * sometimes leads to corruption at the bottom of the screen
> +	 */
> +	if (!crtc->state->active) {
> +		ipu_dc_disable_channel(ipu_crtc->dc);
> +		ipu_di_disable(ipu_crtc->di);

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.

> +	}
> +
>  	/*
>  	 * Planes must be disabled before DC clock is removed, as otherwise the
>  	 * attached IDMACs will be left in undefined state, possibly hanging

regards
Philipp