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;
Date:   Wed, 4 Aug 2021 17:46:40 +0200
From:   Daniel Vetter <daniel@ffwll.ch>
To:     Alex Deucher <alexdeucher@gmail.com>
Cc:     Michel =?iso-8859-1?Q?D=E4nzer?= <michel@daenzer.net>,
        Christian =?iso-8859-1?Q?K=F6nig?= <christian.koenig@amd.com>,
        Zhenneng Li <lizhenneng@kylinos.cn>,
        Alex Deucher <alexander.deucher@amd.com>,
        "Pan, Xinhui" <Xinhui.Pan@amd.com>,
        David Airlie <airlied@linux.ie>,
        amd-gfx list <amd-gfx@lists.freedesktop.org>,
        Maling list - DRI developers 
        <dri-devel@lists.freedesktop.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Daniel Vetter <daniel@ffwll.ch>
Subject: Re: [PATCH] drm/radeon: Update pitch for page flip
Message-ID: <YQq2YAi4w79JNO4H@phenom.ffwll.local>
Mail-Followup-To: Alex Deucher <alexdeucher@gmail.com>,
        Michel =?iso-8859-1?Q?D=E4nzer?= <michel@daenzer.net>,
        Christian =?iso-8859-1?Q?K=F6nig?= <christian.koenig@amd.com>,
        Zhenneng Li <lizhenneng@kylinos.cn>,
        Alex Deucher <alexander.deucher@amd.com>,
        "Pan, Xinhui" <Xinhui.Pan@amd.com>, David Airlie <airlied@linux.ie>,
        amd-gfx list <amd-gfx@lists.freedesktop.org>,
        Maling list - DRI developers <dri-devel@lists.freedesktop.org>,
        LKML <linux-kernel@vger.kernel.org>
References: <20210802074310.1526526-1-lizhenneng@kylinos.cn>
 <e6e77cfb-4e6b-c30e-ae7c-ac84b82c9a75@amd.com>
 <YQetXMaASz/F2EyS@phenom.ffwll.local>
 <CADnq5_PDtEn1y5HJBRHXw8o11LVwSRDKNtQgZtN5u0CW5ZspnQ@mail.gmail.com>
 <6a34fcc3-0aa3-85ff-21c4-86b9b5a48fd5@daenzer.net>
 <CADnq5_MJ=m2_VvzCQ7wJaFx2=OfFfLpoJtc_Ofb+a-v3eujtXg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CADnq5_MJ=m2_VvzCQ7wJaFx2=OfFfLpoJtc_Ofb+a-v3eujtXg@mail.gmail.com>
Precedence: bulk

On Tue, Aug 03, 2021 at 10:49:39AM -0400, Alex Deucher wrote:
> On Tue, Aug 3, 2021 at 4:34 AM Michel D?nzer <michel@daenzer.net> wrote:
> >
> > On 2021-08-02 4:51 p.m., Alex Deucher wrote:
> > > On Mon, Aug 2, 2021 at 4:31 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > >>
> > >> On Mon, Aug 02, 2021 at 10:12:47AM +0200, Christian K?nig wrote:
> > >>> Am 02.08.21 um 09:43 schrieb Zhenneng Li:
> > >>>> When primary bo is updated, crtc's pitch may
> > >>>> have not been updated, this will lead to show
> > >>>> disorder content when user changes display mode,
> > >>>> we update crtc's pitch in page flip to avoid
> > >>>> this bug.
> > >>>> This refers to amdgpu's pageflip.
> > >>>
> > >>> Alex is the expert to ask about that code, but I'm not sure if that is
> > >>> really correct for the old hardware.
> > >>>
> > >>> As far as I know the crtc's pitch should not change during a page flip, but
> > >>> only during a full mode set.
> > >>>
> > >>> So could you elaborate a bit more how you trigger this?
> > >>
> > >> legacy page_flip ioctl only verifies that the fb->format stays the same.
> > >> It doesn't check anything else (afair never has), this is all up to
> > >> drivers to verify.
> > >>
> > >> Personally I'd say add a check to reject this, since testing this and
> > >> making sure it really works everywhere is probably a bit much on this old
> > >> hw.
> > >
> > > If just the pitch changed, that probably wouldn't be much of a
> > > problem, but if the pitch is changing, that probably implies other
> > > stuff has changed as well and we'll just be chasing changes.  I agree
> > > it would be best to just reject anything other than updating the
> > > scanout address.
> >
> > FWIW, that means page flipping cannot be used in some cases which work fine by changing the pitch, which can result in tearing: https://gitlab.freedesktop.org/xorg/xserver/-/issues/839 (which says the i915 driver handles this as well).
> >
> 
> Ok.  In that case, @Zhenneng can you update all of the pitch in all of
> the page_flip functions in radeon rather than just the evergreen one?

atomic drivers handle this fairly ok-ish, since we have a proper
atomic_check there.

For legacy kms I just wouldn't bother, too many corners to validate. But
also if you're bored, just do it :-)
-Daniel

> 
> Thanks,
> 
> Alex
> 
> 
> >
> > --
> > Earthling Michel D?nzer               |               https://redhat.com
> > Libre software enthusiast             |             Mesa and X developer

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch