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@xxxxxxxxxxx> wrote: > > > > On 2021-08-02 4:51 p.m., Alex Deucher wrote: > > > On Mon, Aug 2, 2021 at 4:31 AM Daniel Vetter <daniel@xxxxxxxx> 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
