linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] drm/radeon: Update pitch for page flip
@ 2021-08-02  7:43 Zhenneng Li
       [not found] ` <e6e77cfb-4e6b-c30e-ae7c-ac84b82c9a75@amd.com>
  0 siblings, 1 reply; 7+ messages in thread
From: Zhenneng Li @ 2021-08-02  7:43 UTC (permalink / raw)
  Cc: Zhenneng Li, Alex Deucher, Christian König, Pan, Xinhui,
	David Airlie, Daniel Vetter, amd-gfx, dri-devel, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1892 bytes --]


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.

Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: "Christian König" <christian.koenig@amd.com>
Cc: "Pan, Xinhui" <Xinhui.Pan@amd.com>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: amd-gfx@lists.freedesktop.org
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Zhenneng Li <lizhenneng@kylinos.cn>
---
 drivers/gpu/drm/radeon/evergreen.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/radeon/evergreen.c b/drivers/gpu/drm/radeon/evergreen.c
index 36a888e1b179..eeb590d2dec2 100644
--- a/drivers/gpu/drm/radeon/evergreen.c
+++ b/drivers/gpu/drm/radeon/evergreen.c
@@ -28,6 +28,7 @@
 
 #include <drm/drm_vblank.h>
 #include <drm/radeon_drm.h>
+#include <drm/drm_fourcc.h>
 
 #include "atom.h"
 #include "avivod.h"
@@ -1414,10 +1415,15 @@ void evergreen_page_flip(struct radeon_device *rdev, int crtc_id, u64 crtc_base,
 			 bool async)
 {
 	struct radeon_crtc *radeon_crtc = rdev->mode_info.crtcs[crtc_id];
+	struct drm_framebuffer *fb = radeon_crtc->base.primary->fb;
 
-	/* update the scanout addresses */
+	/* flip at hsync for async, default is vsync */
 	WREG32(EVERGREEN_GRPH_FLIP_CONTROL + radeon_crtc->crtc_offset,
 	       async ? EVERGREEN_GRPH_SURFACE_UPDATE_H_RETRACE_EN : 0);
+	/* update pitch */
+	WREG32(EVERGREEN_GRPH_PITCH + radeon_crtc->crtc_offset,
+	       fb->pitches[0] / fb->format->cpp[0]);
+	/* update the scanout addresses */
 	WREG32(EVERGREEN_GRPH_PRIMARY_SURFACE_ADDRESS_HIGH + radeon_crtc->crtc_offset,
 	       upper_32_bits(crtc_base));
 	WREG32(EVERGREEN_GRPH_PRIMARY_SURFACE_ADDRESS + radeon_crtc->crtc_offset,
-- 
2.25.1


[-- Attachment #2: Type: text/plain, Size: 82 bytes --]

Content-type: Text/plain

No virus found
		Checked by Hillstone Network AntiVirus

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] drm/radeon: Update pitch for page flip
       [not found] ` <e6e77cfb-4e6b-c30e-ae7c-ac84b82c9a75@amd.com>
@ 2021-08-02  8:31   ` Daniel Vetter
  2021-08-02 14:51     ` Alex Deucher
  0 siblings, 1 reply; 7+ messages in thread
From: Daniel Vetter @ 2021-08-02  8:31 UTC (permalink / raw)
  To: Christian König
  Cc: Zhenneng Li, Alex Deucher, Pan, Xinhui, David Airlie,
	Daniel Vetter, amd-gfx, dri-devel, linux-kernel

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.
-Daniel

> 
> Thanks,
> Christian.
> 
> > 
> > Cc: Alex Deucher <alexander.deucher@amd.com>
> > Cc: "Christian König" <christian.koenig@amd.com>
> > Cc: "Pan, Xinhui" <Xinhui.Pan@amd.com>
> > Cc: David Airlie <airlied@linux.ie>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Cc: amd-gfx@lists.freedesktop.org
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: linux-kernel@vger.kernel.org
> > Signed-off-by: Zhenneng Li <lizhenneng@kylinos.cn>
> > ---
> >   drivers/gpu/drm/radeon/evergreen.c | 8 +++++++-
> >   1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/radeon/evergreen.c b/drivers/gpu/drm/radeon/evergreen.c
> > index 36a888e1b179..eeb590d2dec2 100644
> > --- a/drivers/gpu/drm/radeon/evergreen.c
> > +++ b/drivers/gpu/drm/radeon/evergreen.c
> > @@ -28,6 +28,7 @@
> >   #include <drm/drm_vblank.h>
> >   #include <drm/radeon_drm.h>
> > +#include <drm/drm_fourcc.h>
> >   #include "atom.h"
> >   #include "avivod.h"
> > @@ -1414,10 +1415,15 @@ void evergreen_page_flip(struct radeon_device *rdev, int crtc_id, u64 crtc_base,
> >   			 bool async)
> >   {
> >   	struct radeon_crtc *radeon_crtc = rdev->mode_info.crtcs[crtc_id];
> > +	struct drm_framebuffer *fb = radeon_crtc->base.primary->fb;
> > -	/* update the scanout addresses */
> > +	/* flip at hsync for async, default is vsync */
> >   	WREG32(EVERGREEN_GRPH_FLIP_CONTROL + radeon_crtc->crtc_offset,
> >   	       async ? EVERGREEN_GRPH_SURFACE_UPDATE_H_RETRACE_EN : 0);
> > +	/* update pitch */
> > +	WREG32(EVERGREEN_GRPH_PITCH + radeon_crtc->crtc_offset,
> > +	       fb->pitches[0] / fb->format->cpp[0]);
> > +	/* update the scanout addresses */
> >   	WREG32(EVERGREEN_GRPH_PRIMARY_SURFACE_ADDRESS_HIGH + radeon_crtc->crtc_offset,
> >   	       upper_32_bits(crtc_base));
> >   	WREG32(EVERGREEN_GRPH_PRIMARY_SURFACE_ADDRESS + radeon_crtc->crtc_offset,
> > 
> > No virus found
> > 		Checked by Hillstone Network AntiVirus
> 

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] drm/radeon: Update pitch for page flip
  2021-08-02  8:31   ` Daniel Vetter
@ 2021-08-02 14:51     ` Alex Deucher
  2021-08-03  8:00       ` 李真能
  2021-08-03  8:34       ` Michel Dänzer
  0 siblings, 2 replies; 7+ messages in thread
From: Alex Deucher @ 2021-08-02 14:51 UTC (permalink / raw)
  To: Christian König, Zhenneng Li, Alex Deucher, Pan, Xinhui,
	David Airlie, amd-gfx list, Maling list - DRI developers, LKML
  Cc: Daniel Vetter

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.

Alex

> -Daniel
>
> >
> > Thanks,
> > Christian.
> >
> > >
> > > Cc: Alex Deucher <alexander.deucher@amd.com>
> > > Cc: "Christian König" <christian.koenig@amd.com>
> > > Cc: "Pan, Xinhui" <Xinhui.Pan@amd.com>
> > > Cc: David Airlie <airlied@linux.ie>
> > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > Cc: amd-gfx@lists.freedesktop.org
> > > Cc: dri-devel@lists.freedesktop.org
> > > Cc: linux-kernel@vger.kernel.org
> > > Signed-off-by: Zhenneng Li <lizhenneng@kylinos.cn>
> > > ---
> > >   drivers/gpu/drm/radeon/evergreen.c | 8 +++++++-
> > >   1 file changed, 7 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/radeon/evergreen.c b/drivers/gpu/drm/radeon/evergreen.c
> > > index 36a888e1b179..eeb590d2dec2 100644
> > > --- a/drivers/gpu/drm/radeon/evergreen.c
> > > +++ b/drivers/gpu/drm/radeon/evergreen.c
> > > @@ -28,6 +28,7 @@
> > >   #include <drm/drm_vblank.h>
> > >   #include <drm/radeon_drm.h>
> > > +#include <drm/drm_fourcc.h>
> > >   #include "atom.h"
> > >   #include "avivod.h"
> > > @@ -1414,10 +1415,15 @@ void evergreen_page_flip(struct radeon_device *rdev, int crtc_id, u64 crtc_base,
> > >                      bool async)
> > >   {
> > >     struct radeon_crtc *radeon_crtc = rdev->mode_info.crtcs[crtc_id];
> > > +   struct drm_framebuffer *fb = radeon_crtc->base.primary->fb;
> > > -   /* update the scanout addresses */
> > > +   /* flip at hsync for async, default is vsync */
> > >     WREG32(EVERGREEN_GRPH_FLIP_CONTROL + radeon_crtc->crtc_offset,
> > >            async ? EVERGREEN_GRPH_SURFACE_UPDATE_H_RETRACE_EN : 0);
> > > +   /* update pitch */
> > > +   WREG32(EVERGREEN_GRPH_PITCH + radeon_crtc->crtc_offset,
> > > +          fb->pitches[0] / fb->format->cpp[0]);
> > > +   /* update the scanout addresses */
> > >     WREG32(EVERGREEN_GRPH_PRIMARY_SURFACE_ADDRESS_HIGH + radeon_crtc->crtc_offset,
> > >            upper_32_bits(crtc_base));
> > >     WREG32(EVERGREEN_GRPH_PRIMARY_SURFACE_ADDRESS + radeon_crtc->crtc_offset,
> > >
> > > No virus found
> > >             Checked by Hillstone Network AntiVirus
> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] drm/radeon: Update pitch for page flip
  2021-08-02 14:51     ` Alex Deucher
@ 2021-08-03  8:00       ` 李真能
  2021-08-03  8:34       ` Michel Dänzer
  1 sibling, 0 replies; 7+ messages in thread
From: 李真能 @ 2021-08-03  8:00 UTC (permalink / raw)
  To: Alex Deucher, Christian König, Alex Deucher, Pan, Xinhui,
	David Airlie, amd-gfx list, Maling list - DRI developers, LKML
  Cc: Daniel Vetter

[-- Attachment #1: Type: text/plain, Size: 10354 bytes --]


在 2021/8/2 下午10:51, Alex Deucher 写道:
> 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.
Thanks for your reply!

  In theory , pitch is updated only when the scanout address is 
updated,  but we can trigger this bug on r5230 when using two screens 
and changing modeset,  you can execute shell:


#!/bin/bash

cnt=0;
while [ 1 ]
do
xrandr --output HDMI-0 --right-of VGA-0 --auto
echo "hdmi----vga"
sleep  8
xrandr --output VGA-0 --auto --output HDMI-0 --off
sleep 3
let cnt+=1;
echo $cnt
done

at the same time, open a new terminal, press F11 to fast switch this 
window between maximize size and normal size to improve the 
possibility,attachement picture shows the phenomenon.

I add some debug messages:

[   89.065206] lzn debug, 
drivers/gpu/drm/drm_crtc_helper.c:drm_crtc_helper_set_config:589, 
set->crtc->primary->fb: 0xffffffa0e5956100, set->fb: 0xffffffa0e701e400
[   89.065209] lzn debug, 
drivers/gpu/drm/drm_crtc_helper.c:drm_crtc_helper_set_config:601
[   89.065213] lzn debug, 
drivers/gpu/drm/drm_crtc_helper.c:drm_crtc_helper_set_config:713, 
fb_changed: 1, mode_changed: 0
[   89.065216] lzn debug, 
drivers/gpu/drm/radeon/atombios_crtc.c:dce4_crtc_do_set_base:1148, x: 0: 
y: 0, crtc: 0xffffffa0e62dc000, atomic: 0
[   89.065222] lzn debug, 
drivers/gpu/drm/radeon/atombios_crtc.c:dce4_crtc_do_set_base:1426, 
fb_location: 0x117f2000, target_fb->width: 1920, target_fb->height: 1080
[   89.065223] lzn debug, 
drivers/gpu/drm/radeon/atombios_crtc.c:dce4_crtc_do_set_base:1430, 
fb_pitch_pixels: 1920, target_fb->pitches[0]: 7680, 
target_fb->format->cpp[0]: 4
[   89.065240] lzn debug, 
drivers/gpu/drm/drm_crtc_helper.c:drm_crtc_helper_set_config:752*****************************************************************
[   92.079288] lzn debug, 
drivers/gpu/drm/drm_crtc_helper.c:drm_crtc_helper_set_config:589, 
set->crtc->primary->fb: 0xffffffa0e701e400, set->fb: 0xffffffa0eafced00
[   92.079291] lzn debug, 
drivers/gpu/drm/drm_crtc_helper.c:drm_crtc_helper_set_config:601
[   92.079295] lzn debug, 
drivers/gpu/drm/drm_crtc_helper.c:drm_crtc_helper_set_config:713, 
fb_changed: 1, mode_changed: 0
[   92.079298] lzn debug, 
drivers/gpu/drm/radeon/atombios_crtc.c:dce4_crtc_do_set_base:1148, x: 0: 
y: 0, crtc: 0xffffffa0e62dc000, atomic: 0
[   92.079304] lzn debug, 
drivers/gpu/drm/radeon/atombios_crtc.c:dce4_crtc_do_set_base:1426, 
fb_location: 0x10000000, target_fb->width: 3840, target_fb->height: 1080
[   92.079306] lzn debug, 
drivers/gpu/drm/radeon/atombios_crtc.c:dce4_crtc_do_set_base:1430, 
fb_pitch_pixels: 3840, target_fb->pitches[0]: 15360, 
target_fb->format->cpp[0]: 4
[   92.079323] lzn debug, 
drivers/gpu/drm/drm_crtc_helper.c:drm_crtc_helper_set_config:752*****************************************************************
[   92.080382] lzn debug, 
drivers/gpu/drm/drm_crtc_helper.c:drm_crtc_helper_set_config:589, 
set->crtc->primary->fb: 0x0, set->fb: 0xffffffa0eafced00
[   92.080384] lzn debug, 
drivers/gpu/drm/drm_crtc_helper.c:drm_crtc_helper_set_config:595
[   92.080387] lzn debug, 
drivers/gpu/drm/drm_crtc_helper.c:drm_crtc_helper_set_config:713, 
fb_changed: 0, mode_changed: 1
[   92.080389] lzn debug, 
drivers/gpu/drm/drm_crtc_helper.c:drm_crtc_helper_set_config:715
[   92.083143] lzn debug, 
drivers/gpu/drm/radeon/atombios_crtc.c:dce4_crtc_do_set_base:1148, x: 
1920: y: 0, crtc: 0xffffffa0e62db000, atomic: 0
[   92.083149] lzn debug, 
drivers/gpu/drm/radeon/atombios_crtc.c:dce4_crtc_do_set_base:1426, 
fb_location: 0x10000000, target_fb->width: 3840, target_fb->height: 1080
[   92.083164] lzn debug, 
drivers/gpu/drm/radeon/atombios_crtc.c:dce4_crtc_do_set_base:1430, 
fb_pitch_pixels: 3840, target_fb->pitches[0]: 15360, 
target_fb->format->cpp[0]: 4
[   92.120595] lzn debug, 
drivers/gpu/drm/drm_crtc_helper.c:drm_crtc_helper_set_config:752*****************************************************************
[  100.157744] lzn debug, 
drivers/gpu/drm/drm_crtc_helper.c:drm_crtc_helper_set_config:589, 
set->crtc->primary->fb: 0xffffffa0eafced00, set->fb: 0xffffffa0f3a5bb00
[  100.157746] lzn debug, 
drivers/gpu/drm/drm_crtc_helper.c:drm_crtc_helper_set_config:601
[  100.157749] lzn debug, 
drivers/gpu/drm/drm_crtc_helper.c:drm_crtc_helper_set_config:713, 
fb_changed: 1, mode_changed: 0
[  100.157752] lzn debug, 
drivers/gpu/drm/radeon/atombios_crtc.c:dce4_crtc_do_set_base:1148, x: 0: 
y: 0, crtc: 0xffffffa0e62dc000, atomic: 0
[  100.157758] lzn debug, 
drivers/gpu/drm/radeon/atombios_crtc.c:dce4_crtc_do_set_base:1426, 
fb_location: 0x1790c000, target_fb->width: 1920, target_fb->height: 1080
[  100.157760] lzn debug, 
drivers/gpu/drm/radeon/atombios_crtc.c:dce4_crtc_do_set_base:1430, 
fb_pitch_pixels: 1920, target_fb->pitches[0]: 7680, 
target_fb->format->cpp[0]: 4
[  100.157777] lzn debug, 
drivers/gpu/drm/drm_crtc_helper.c:drm_crtc_helper_set_config:752*****************************************************************
[  103.166751] lzn debug, 
drivers/gpu/drm/drm_crtc_helper.c:drm_crtc_helper_set_config:589, 
set->crtc->primary->fb: 0xffffffa0e9a0cd00, set->fb: 0xffffffa0e9a0cd00
[  103.166757] lzn debug, 
drivers/gpu/drm/drm_crtc_helper.c:drm_crtc_helper_set_config:713, 
fb_changed: 0, mode_changed: 0
[  103.166758] lzn debug, 
drivers/gpu/drm/drm_crtc_helper.c:drm_crtc_helper_set_config:752*****************************************************************
[  103.169353] lzn debug, 
drivers/gpu/drm/drm_crtc_helper.c:drm_crtc_helper_set_config:589, 
set->crtc->primary->fb: 0x0, set->fb: 0xffffffa0e9a0cd00
[  103.169355] lzn debug, 
drivers/gpu/drm/drm_crtc_helper.c:drm_crtc_helper_set_config:595
[  103.169359] lzn debug, 
drivers/gpu/drm/drm_crtc_helper.c:drm_crtc_helper_set_config:713, 
fb_changed: 0, mode_changed: 1
[  103.169360] lzn debug, 
drivers/gpu/drm/drm_crtc_helper.c:drm_crtc_helper_set_config:715
[  103.170396] lzn debug, 
drivers/gpu/drm/radeon/atombios_crtc.c:dce4_crtc_do_set_base:1148, x: 
1920: y: 0, crtc: 0xffffffa0e62db000, atomic: 0
[  103.170403] lzn debug, 
drivers/gpu/drm/radeon/atombios_crtc.c:dce4_crtc_do_set_base:1426, 
fb_location: 0x11fea000, target_fb->width: 3840, target_fb->height: 1080
[  103.170404] lzn debug, 
drivers/gpu/drm/radeon/atombios_crtc.c:dce4_crtc_do_set_base:1430, 
fb_pitch_pixels: 3840, target_fb->pitches[0]: 15360, 
target_fb->format->cpp[0]: 4
[  103.224592] lzn debug, 
drivers/gpu/drm/drm_crtc_helper.c:drm_crtc_helper_set_config:752*****************************************************************

After 103s of os started,  bug is triggerd, fb_changed and mode_changed 
are zero, and crtc's pitch has not been updated.

Amdgpu has the same problem,  but amdgpu's page_flip will update pitch, 
so this bug can't trigger on amdgpu.

you can refer to dce_v6_0_page_flip in amdgpu,all amdgpu drivers update 
pitch in page_flip.

test os: focal-desktop-arm64.iso

Zhenneng Li
>
> Alex
>
>> -Daniel
>>
>>> Thanks,
>>> Christian.
>>>
>>>> Cc: Alex Deucher <alexander.deucher@amd.com>
>>>> Cc: "Christian König" <christian.koenig@amd.com>
>>>> Cc: "Pan, Xinhui" <Xinhui.Pan@amd.com>
>>>> Cc: David Airlie <airlied@linux.ie>
>>>> Cc: Daniel Vetter <daniel@ffwll.ch>
>>>> Cc: amd-gfx@lists.freedesktop.org
>>>> Cc: dri-devel@lists.freedesktop.org
>>>> Cc: linux-kernel@vger.kernel.org
>>>> Signed-off-by: Zhenneng Li <lizhenneng@kylinos.cn>
>>>> ---
>>>>    drivers/gpu/drm/radeon/evergreen.c | 8 +++++++-
>>>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/radeon/evergreen.c b/drivers/gpu/drm/radeon/evergreen.c
>>>> index 36a888e1b179..eeb590d2dec2 100644
>>>> --- a/drivers/gpu/drm/radeon/evergreen.c
>>>> +++ b/drivers/gpu/drm/radeon/evergreen.c
>>>> @@ -28,6 +28,7 @@
>>>>    #include <drm/drm_vblank.h>
>>>>    #include <drm/radeon_drm.h>
>>>> +#include <drm/drm_fourcc.h>
>>>>    #include "atom.h"
>>>>    #include "avivod.h"
>>>> @@ -1414,10 +1415,15 @@ void evergreen_page_flip(struct radeon_device *rdev, int crtc_id, u64 crtc_base,
>>>>                       bool async)
>>>>    {
>>>>      struct radeon_crtc *radeon_crtc = rdev->mode_info.crtcs[crtc_id];
>>>> +   struct drm_framebuffer *fb = radeon_crtc->base.primary->fb;
>>>> -   /* update the scanout addresses */
>>>> +   /* flip at hsync for async, default is vsync */
>>>>      WREG32(EVERGREEN_GRPH_FLIP_CONTROL + radeon_crtc->crtc_offset,
>>>>             async ? EVERGREEN_GRPH_SURFACE_UPDATE_H_RETRACE_EN : 0);
>>>> +   /* update pitch */
>>>> +   WREG32(EVERGREEN_GRPH_PITCH + radeon_crtc->crtc_offset,
>>>> +          fb->pitches[0] / fb->format->cpp[0]);
>>>> +   /* update the scanout addresses */
>>>>      WREG32(EVERGREEN_GRPH_PRIMARY_SURFACE_ADDRESS_HIGH + radeon_crtc->crtc_offset,
>>>>             upper_32_bits(crtc_base));
>>>>      WREG32(EVERGREEN_GRPH_PRIMARY_SURFACE_ADDRESS + radeon_crtc->crtc_offset,
>>>>
>>>> No virus found
>>>>              Checked by Hillstone Network AntiVirus
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch

[-- Attachment #2: r5230.jpg --]
[-- Type: image/jpeg, Size: 2956751 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] drm/radeon: Update pitch for page flip
  2021-08-02 14:51     ` Alex Deucher
  2021-08-03  8:00       ` 李真能
@ 2021-08-03  8:34       ` Michel Dänzer
  2021-08-03 14:49         ` Alex Deucher
  1 sibling, 1 reply; 7+ messages in thread
From: Michel Dänzer @ 2021-08-03  8:34 UTC (permalink / raw)
  To: Alex Deucher, Christian König, Zhenneng Li, Alex Deucher,
	Pan, Xinhui, David Airlie, amd-gfx list,
	Maling list - DRI developers, LKML
  Cc: Daniel Vetter

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).


-- 
Earthling Michel Dänzer               |               https://redhat.com
Libre software enthusiast             |             Mesa and X developer

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] drm/radeon: Update pitch for page flip
  2021-08-03  8:34       ` Michel Dänzer
@ 2021-08-03 14:49         ` Alex Deucher
  2021-08-04 15:46           ` Daniel Vetter
  0 siblings, 1 reply; 7+ messages in thread
From: Alex Deucher @ 2021-08-03 14:49 UTC (permalink / raw)
  To: Michel Dänzer
  Cc: Christian König, Zhenneng Li, Alex Deucher, Pan, Xinhui,
	David Airlie, amd-gfx list, Maling list - DRI developers, LKML,
	Daniel Vetter

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?

Thanks,

Alex


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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] drm/radeon: Update pitch for page flip
  2021-08-03 14:49         ` Alex Deucher
@ 2021-08-04 15:46           ` Daniel Vetter
  0 siblings, 0 replies; 7+ messages in thread
From: Daniel Vetter @ 2021-08-04 15:46 UTC (permalink / raw)
  To: Alex Deucher
  Cc: Michel Dänzer, Christian König, Zhenneng Li,
	Alex Deucher, Pan, Xinhui, David Airlie, amd-gfx list,
	Maling list - DRI developers, LKML, Daniel Vetter

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-08-04 15:46 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-02  7:43 [PATCH] drm/radeon: Update pitch for page flip Zhenneng Li
     [not found] ` <e6e77cfb-4e6b-c30e-ae7c-ac84b82c9a75@amd.com>
2021-08-02  8:31   ` Daniel Vetter
2021-08-02 14:51     ` Alex Deucher
2021-08-03  8:00       ` 李真能
2021-08-03  8:34       ` Michel Dänzer
2021-08-03 14:49         ` Alex Deucher
2021-08-04 15:46           ` Daniel Vetter

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).