Re: [PATCH 4/4] drm/atmel-hlcdc: do not immediately disable planes, wait for next frame

From: Boris Brezillon <bbrezillon@kernel.org>
To: Peter Rosin <peda@axentia.se>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>,
	David Airlie <airlied@linux.ie>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	Boris Brezillon <boris.brezillon@bootlin.com>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
	Nicolas Ferre <nicolas.ferre@microchip.com>
Subject: Re: [PATCH 4/4] drm/atmel-hlcdc: do not immediately disable planes, wait for next frame
Date: Thu, 10 Jan 2019 20:25:22 +0100	[thread overview]
Message-ID: <20190110202522.10206f2d@bbrezillon> (raw)
In-Reply-To: <a31e6514-1643-9e47-cbb4-3323183fd370@axentia.se>

On Thu, 10 Jan 2019 18:51:21 +0000
Peter Rosin <peda@axentia.se> wrote:

> On 2019-01-10 18:29, Boris Brezillon wrote:
> > On Thu, 10 Jan 2019 15:10:48 +0000
> > Peter Rosin <peda@axentia.se> wrote:
> >   
> >> The A2Q and UPDATE bits have no effect in the channel disable registers.
> >> However, since they are present, assume that the intention is to disable
> >> planes, not immediately as indicated by the RST bit, but on the next
> >> frame shift since that is what A2Q and UPDATE means in the channel enable
> >> registers.
> >>
> >> Disabling the plane on the next frame shift is done with the EN bit,
> >> so use that.  
> > 
> > It's been a long time, but I think I had a good reason for forcing a
> > reset. IIRC, when you don't do that and the CRTC is disabled before the
> > plane, the EN bit stays around, and next time you queue a plane update,
> > you'll start with an invalid buf pointer.  
> 
> It might be possible to clear the EN bit in ...CHDR before enabling the
> plane in ...CHER. Or is that too late?

I think I tried that, but I'm not sure (BTW, this change was done in
bd4248bb5e8b ("drm: atmel-hlcdc: reset layer A2Q and UPDATE bits when
disabling it")). Anyway, I'm not even sure this is still needed now
that atomic updates have a wait_for_flip_done/vblank() in the commit
path.

> But this patch is not overly
> important, I just wanted to avoid the resulting "black hole" when the
> plane DMA is disabled mid-frame. But disabling planes is probably not
> something that happens frequently and will perhaps not be noticed at
> all...

Okay. Other patches look good to me, I'm just waiting for Nicolas
feedback before applying them.