Re: [PATCH] drm/radeon: Update pitch for page flip

From: 李真能
Date: Tue Aug 03 2021 - 04:01:42 EST

Next message: ken manthey: "Hello"
Previous message: Krzysztof Kozlowski: "Re: [PATCH v5 3/5] dt-bindings: vendor-prefixes: Add another prefix for Mediatek Co."
In reply to: Alex Deucher: "Re: [PATCH] drm/radeon: Update pitch for page flip"
Next in thread: Michel Dänzer: "Re: [PATCH] drm/radeon: Update pitch for page flip"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

在 2021/8/2 下午10:51, Alex Deucher 写道:

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.

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@xxxxxxx>
Cc: "Christian König" <christian.koenig@xxxxxxx>
Cc: "Pan, Xinhui" <Xinhui.Pan@xxxxxxx>
Cc: David Airlie <airlied@xxxxxxxx>
Cc: Daniel Vetter <daniel@xxxxxxxx>
Cc: amd-gfx@xxxxxxxxxxxxxxxxxxxxx
Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
Cc: linux-kernel@xxxxxxxxxxxxxxx
Signed-off-by: Zhenneng Li <lizhenneng@xxxxxxxxxx>
---
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: r5230.jpg
Description: JPEG image

Next message: ken manthey: "Hello"
Previous message: Krzysztof Kozlowski: "Re: [PATCH v5 3/5] dt-bindings: vendor-prefixes: Add another prefix for Mediatek Co."
In reply to: Alex Deucher: "Re: [PATCH] drm/radeon: Update pitch for page flip"
Next in thread: Michel Dänzer: "Re: [PATCH] drm/radeon: Update pitch for page flip"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]