RE: [PATCH v2] drm/amdgpu: Do not change amdgpu framebuffer format during page flip
> -Original Message- > From: Daniel Vetter > Sent: 2021/January/07, Thursday 12:33 PM > To: Koenig, Christian > Cc: Liu, Zhan ; amd-gfx@lists.freedesktop.org; Cornij, > Nikola ; Wang, Chao-kai (Stylon) > ; Wang, Chao-kai (Stylon) > ; dri-de...@lists.freedesktop.org; Kazlauskas, > Nicholas ; b...@basnieuwenhuizen.nl > Subject: Re: [PATCH v2] drm/amdgpu: Do not change amdgpu framebuffer > format during page flip > > On Sun, Jan 03, 2021 at 04:43:37PM +0100, Christian König wrote: > > Am 29.12.20 um 22:10 schrieb Zhan Liu: > > > [Why] > > > Driver cannot change amdgpu framebuffer (afb) format while doing > > > page flip. Force system doing so will cause ioctl error, and result > > > in breaking several functionalities including FreeSync. > > > > > > If afb format is forced to change during page flip, following > > > message will appear in dmesg.log: > > > > > > "[drm:drm_mode_page_flip_ioctl [drm]] Page flip is not allowed to > > > change frame buffer format." > > > > > > [How] > > > Do not change afb format while doing page flip. It is okay to check > > > whether afb format is valid here, however, forcing afb format change > > > shouldn't happen here. > > > > I don't think this we can do this. > > > > It is perfectly valid for a page flip to switch between linear and > > tiled formats on an I+A or A+A laptop. > > It is, but that's not the bug here. struct drm_framebuffer.format is supposed > to be invariant over the lifetime of a drm_fb object, you need to set it when > the fb is created and never change it afterwards. So the patch here isn't yet > the real deal. > > Also this means the implicit tiling information cannot be changed after a fb > is > created for a given bo, but we've discussed this at length and that limitation > should be all ok. > -Daniel Thank you Christian and Daniel for the input! Bas recently submitted an alternative patch ([PATCH] drm: Check actual format for legacy pageflip.) which addresses the same issue, and his patch makes more sense to me, so I will abandon my patch in this case. Thanks, Zhan > > > > > Christian. > > > > > > > > Signed-off-by: Zhan Liu > > > --- > > > drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > Thanks Nick and Bas. Here is my second patch for review. > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c > > > index a638709e9c92..8a12e27ff4ec 100644 > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c > > > @@ -832,7 +832,8 @@ static int convert_tiling_flags_to_modifier(struct > amdgpu_framebuffer *afb) > > > if (!format_info) > > > return -EINVAL; > > > - afb->base.format = format_info; > > > + if (!afb->base.format) > > > + afb->base.format = format_info; > > > } > > > } > > > > ___ > > dri-devel mailing list > > dri-de...@lists.freedesktop.org > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flist > > s.freedesktop.org%2Fmailman%2Flistinfo%2Fdri- > develdata=04%7C01%7C > > > zhan.liu%40amd.com%7Cda23e6e33a7e46dfc4e308d8b33242c8%7C3dd896 > 1fe4884e > > > 608e11a82d994e183d%7C0%7C0%7C637456375746425509%7CUnknown% > 7CTWFpbGZsb3 > > > d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0 > %3D%7 > > > C1000sdata=5lCm4d6FHihfFHUf5mVym0O6lKmZHgR89%2F2Eqj2ojhg > %3Dr > > eserved=0 > > -- > Daniel Vetter > Software Engineer, Intel Corporation > https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fblog.ff > wll.ch%2Fdata=04%7C01%7Czhan.liu%40amd.com%7Cda23e6e33a7e > 46dfc4e308d8b33242c8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7 > C0%7C637456375746425509%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4 > wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000 > mp;sdata=44x858klbIcVeRtP%2BuJST2K3xuCLisjbfhV9rEQrzpA%3Drese > rved=0 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2] drm/amdgpu: Do not change amdgpu framebuffer format during page flip
On Sun, Jan 03, 2021 at 04:43:37PM +0100, Christian König wrote: > Am 29.12.20 um 22:10 schrieb Zhan Liu: > > [Why] > > Driver cannot change amdgpu framebuffer (afb) format while doing > > page flip. Force system doing so will cause ioctl error, and result in > > breaking several functionalities including FreeSync. > > > > If afb format is forced to change during page flip, following message > > will appear in dmesg.log: > > > > "[drm:drm_mode_page_flip_ioctl [drm]] > > Page flip is not allowed to change frame buffer format." > > > > [How] > > Do not change afb format while doing page flip. It is okay to check > > whether afb format is valid here, however, forcing afb format change > > shouldn't happen here. > > I don't think this we can do this. > > It is perfectly valid for a page flip to switch between linear and tiled > formats on an I+A or A+A laptop. It is, but that's not the bug here. struct drm_framebuffer.format is supposed to be invariant over the lifetime of a drm_fb object, you need to set it when the fb is created and never change it afterwards. So the patch here isn't yet the real deal. Also this means the implicit tiling information cannot be changed after a fb is created for a given bo, but we've discussed this at length and that limitation should be all ok. -Daniel > > Christian. > > > > > Signed-off-by: Zhan Liu > > --- > > drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > Thanks Nick and Bas. Here is my second patch for review. > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c > > index a638709e9c92..8a12e27ff4ec 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c > > @@ -832,7 +832,8 @@ static int convert_tiling_flags_to_modifier(struct > > amdgpu_framebuffer *afb) > > if (!format_info) > > return -EINVAL; > > - afb->base.format = format_info; > > + if (!afb->base.format) > > + afb->base.format = format_info; > > } > > } > > ___ > dri-devel mailing list > dri-de...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2] drm/amdgpu: Do not change amdgpu framebuffer format during page flip
Hi Zhan,
url:
https://github.com/0day-ci/linux/commits/Zhan-Liu/drm-amdgpu-Do-not-change-amdgpu-framebuffer-format-during-page-flip/20201230-051134
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
dea8dcf2a9fa8cc540136a6cd885c3beece16ec3
config: x86_64-randconfig-m031-20201229 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
Reported-by: Dan Carpenter
New smatch warnings:
drivers/gpu/drm/amd/amdgpu/amdgpu_display.c:831
convert_tiling_flags_to_modifier() warn: variable dereferenced before check
'afb->base.format' (see line 826)
vim +831 drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02 678 static int
convert_tiling_flags_to_modifier(struct amdgpu_framebuffer *afb)
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02 679 {
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02 680 struct amdgpu_device
*adev = drm_to_adev(afb->base.dev);
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02 681 uint64_t modifier = 0;
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02 682
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02 683 if (!afb->tiling_flags
|| !AMDGPU_TILING_GET(afb->tiling_flags, SWIZZLE_MODE)) {
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02 684 modifier =
DRM_FORMAT_MOD_LINEAR;
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02 685 } else {
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02 686 int swizzle =
AMDGPU_TILING_GET(afb->tiling_flags, SWIZZLE_MODE);
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02 687 bool has_xor =
swizzle >= 16;
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02 688 int
block_size_bits;
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02 689 int version;
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02 690 int
pipe_xor_bits = 0;
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02 691 int
bank_xor_bits = 0;
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02 692 int packers = 0;
1331e6304f5d924a Bas Nieuwenhuizen 2020-11-11 693 int rb = 0;
1331e6304f5d924a Bas Nieuwenhuizen 2020-11-11 694 int pipes =
ilog2(adev->gfx.config.gb_addr_config_fields.num_pipes);
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02 695 uint32_t
dcc_offset = AMDGPU_TILING_GET(afb->tiling_flags, DCC_OFFSET_256B);
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02 696
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02 697 switch (swizzle
>> 2) {
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02 698 case 0: /* 256B
*/
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02 699
block_size_bits = 8;
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02 700 break;
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02 701 case 1: /* 4KiB
*/
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02 702 case 5: /* 4KiB
_X */
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02 703
block_size_bits = 12;
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02 704 break;
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02 705 case 2: /*
64KiB */
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02 706 case 4: /* 64
KiB _T */
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02 707 case 6: /* 64
KiB _X */
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02 708
block_size_bits = 16;
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02 709 break;
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02 710 default:
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02 711 /*
RESERVED or VAR */
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02 712 return
-EINVAL;
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02 713 }
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02 714
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02 715 if
(adev->asic_type >= CHIP_SIENNA_CICHLID)
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02 716 version
= AMD_FMT_MOD_TILE_VER_GFX10_RBPLUS;
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02 717 else if
(adev->family == AMDGPU_FAMILY_NV)
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02 718 version
= AMD_FMT_MOD_TILE_VER_GFX10;
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02 719 else
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02 720 version
= AMD_FMT_MOD_TILE_VER_GFX9;
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02 721
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02 722 switch (swizzle
& 3) {
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02 723 case 0: /* Z
microtiling */
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02 724 return
-EINVAL;
08d769151dc9690a Bas
Re: [PATCH v2] drm/amdgpu: Do not change amdgpu framebuffer format during page flip
Am 29.12.20 um 22:10 schrieb Zhan Liu: [Why] Driver cannot change amdgpu framebuffer (afb) format while doing page flip. Force system doing so will cause ioctl error, and result in breaking several functionalities including FreeSync. If afb format is forced to change during page flip, following message will appear in dmesg.log: "[drm:drm_mode_page_flip_ioctl [drm]] Page flip is not allowed to change frame buffer format." [How] Do not change afb format while doing page flip. It is okay to check whether afb format is valid here, however, forcing afb format change shouldn't happen here. I don't think this we can do this. It is perfectly valid for a page flip to switch between linear and tiled formats on an I+A or A+A laptop. Christian. Signed-off-by: Zhan Liu --- drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) Thanks Nick and Bas. Here is my second patch for review. diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c index a638709e9c92..8a12e27ff4ec 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c @@ -832,7 +832,8 @@ static int convert_tiling_flags_to_modifier(struct amdgpu_framebuffer *afb) if (!format_info) return -EINVAL; - afb->base.format = format_info; + if (!afb->base.format) + afb->base.format = format_info; } } ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
