Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 10 Jan 2019 20:25:22 +0100
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
Message-ID: <20190110202522.10206f2d@bbrezillon>
In-Reply-To: <a31e6514-1643-9e47-cbb4-3323183fd370@axentia.se>
References: <20190110151020.30468-1-peda@axentia.se>
        <20190110151020.30468-5-peda@axentia.se>
        <20190110182904.5efd08f2@bbrezillon>
        <a31e6514-1643-9e47-cbb4-3323183fd370@axentia.se>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.