diff mbox series

[v2] drm/amdgpu: Do not change amdgpu framebuffer format during page flip

Message ID 20201229211044.109020-1-zhan.liu@amd.com (mailing list archive)
State New, archived
Headers show
Series [v2] drm/amdgpu: Do not change amdgpu framebuffer format during page flip | expand

Commit Message

Liu, Zhan Dec. 29, 2020, 9:10 p.m. UTC 
[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.

Signed-off-by: Zhan Liu <zhan.liu@amd.com>
---
 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.

Comments

Christian König Jan. 3, 2021, 3:43 p.m. UTC | #1 
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 <zhan.liu@amd.com>
> ---
>   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;
>   		}
>   	}
>
Dan Carpenter Jan. 5, 2021, 12:51 p.m. UTC | #2 
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 <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

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 Nieuwenhuizen 2020-09-02  725  		case 1: /* S microtiling */
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  726  			if (!has_xor)
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  727  				version = AMD_FMT_MOD_TILE_VER_GFX9;
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  728  			break;
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  729  		case 2:
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  730  			if (!has_xor && afb->base.format->cpp[0] != 4)
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  731  				version = AMD_FMT_MOD_TILE_VER_GFX9;
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  732  			break;
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  733  		case 3:
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  734  			break;
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  735  		}
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  736  
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  737  		if (has_xor) {
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  738  			switch (version) {
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  739  			case AMD_FMT_MOD_TILE_VER_GFX10_RBPLUS:
1331e6304f5d924a Bas Nieuwenhuizen 2020-11-11  740  				pipe_xor_bits = min(block_size_bits - 8, pipes);
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  741  				packers = min(block_size_bits - 8 - pipe_xor_bits,
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  742  					      ilog2(adev->gfx.config.gb_addr_config_fields.num_pkrs));
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  743  				break;
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  744  			case AMD_FMT_MOD_TILE_VER_GFX10:
1331e6304f5d924a Bas Nieuwenhuizen 2020-11-11  745  				pipe_xor_bits = min(block_size_bits - 8, pipes);
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  746  				break;
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  747  			case AMD_FMT_MOD_TILE_VER_GFX9:
1331e6304f5d924a Bas Nieuwenhuizen 2020-11-11  748  				rb = ilog2(adev->gfx.config.gb_addr_config_fields.num_se) +
1331e6304f5d924a Bas Nieuwenhuizen 2020-11-11  749  				     ilog2(adev->gfx.config.gb_addr_config_fields.num_rb_per_se);
1331e6304f5d924a Bas Nieuwenhuizen 2020-11-11  750  				pipe_xor_bits = min(block_size_bits - 8, pipes +
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  751  						    ilog2(adev->gfx.config.gb_addr_config_fields.num_se));
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  752  				bank_xor_bits = min(block_size_bits - 8 - pipe_xor_bits,
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  753  						    ilog2(adev->gfx.config.gb_addr_config_fields.num_banks));
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  754  				break;
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  755  			}
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  756  		}
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  757  
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  758  		modifier = AMD_FMT_MOD |
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  759  			   AMD_FMT_MOD_SET(TILE, AMDGPU_TILING_GET(afb->tiling_flags, SWIZZLE_MODE)) |
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  760  			   AMD_FMT_MOD_SET(TILE_VERSION, version) |
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  761  			   AMD_FMT_MOD_SET(PIPE_XOR_BITS, pipe_xor_bits) |
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  762  			   AMD_FMT_MOD_SET(BANK_XOR_BITS, bank_xor_bits) |
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  763  			   AMD_FMT_MOD_SET(PACKERS, packers);
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  764  
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  765  		if (dcc_offset != 0) {
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  766  			bool dcc_i64b = AMDGPU_TILING_GET(afb->tiling_flags, DCC_INDEPENDENT_64B) != 0;
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  767  			bool dcc_i128b = version >= AMD_FMT_MOD_TILE_VER_GFX10_RBPLUS;
816853f9dc4057b6 Bas Nieuwenhuizen 2020-11-11  768  			const struct drm_format_info *format_info;
1331e6304f5d924a Bas Nieuwenhuizen 2020-11-11  769  			u64 render_dcc_offset;
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  770  
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  771  			/* Enable constant encode on RAVEN2 and later. */
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  772  			bool dcc_constant_encode = adev->asic_type > CHIP_RAVEN ||
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  773  						   (adev->asic_type == CHIP_RAVEN &&
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  774  						    adev->external_rev_id >= 0x81);
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  775  
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  776  			int max_cblock_size = dcc_i64b ? AMD_FMT_MOD_DCC_BLOCK_64B :
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  777  					      dcc_i128b ? AMD_FMT_MOD_DCC_BLOCK_128B :
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  778  					      AMD_FMT_MOD_DCC_BLOCK_256B;
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  779  
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  780  			modifier |= AMD_FMT_MOD_SET(DCC, 1) |
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  781  				    AMD_FMT_MOD_SET(DCC_CONSTANT_ENCODE, dcc_constant_encode) |
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  782  				    AMD_FMT_MOD_SET(DCC_INDEPENDENT_64B, dcc_i64b) |
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  783  				    AMD_FMT_MOD_SET(DCC_INDEPENDENT_128B, dcc_i128b) |
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  784  				    AMD_FMT_MOD_SET(DCC_MAX_COMPRESSED_BLOCK, max_cblock_size);
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  785  
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  786  			afb->base.offsets[1] = dcc_offset * 256 + afb->base.offsets[0];
1331e6304f5d924a Bas Nieuwenhuizen 2020-11-11  787  			afb->base.pitches[1] =
1331e6304f5d924a Bas Nieuwenhuizen 2020-11-11  788  				AMDGPU_TILING_GET(afb->tiling_flags, DCC_PITCH_MAX) + 1;
1331e6304f5d924a Bas Nieuwenhuizen 2020-11-11  789  
1331e6304f5d924a Bas Nieuwenhuizen 2020-11-11  790  			/*
1331e6304f5d924a Bas Nieuwenhuizen 2020-11-11  791  			 * If the userspace driver uses retiling the tiling flags do not contain
1331e6304f5d924a Bas Nieuwenhuizen 2020-11-11  792  			 * info on the renderable DCC buffer. Luckily the opaque metadata contains
1331e6304f5d924a Bas Nieuwenhuizen 2020-11-11  793  			 * the info so we can try to extract it. The kernel does not use this info
1331e6304f5d924a Bas Nieuwenhuizen 2020-11-11  794  			 * but we should convert it to a modifier plane for getfb2, so the
1331e6304f5d924a Bas Nieuwenhuizen 2020-11-11  795  			 * userspace driver that gets it doesn't have to juggle around another DCC
1331e6304f5d924a Bas Nieuwenhuizen 2020-11-11  796  			 * plane internally.
1331e6304f5d924a Bas Nieuwenhuizen 2020-11-11  797  			 */
1331e6304f5d924a Bas Nieuwenhuizen 2020-11-11  798  			if (extract_render_dcc_offset(adev, afb->base.obj[0],
1331e6304f5d924a Bas Nieuwenhuizen 2020-11-11  799  						      &render_dcc_offset) == 0 &&
1331e6304f5d924a Bas Nieuwenhuizen 2020-11-11  800  			    render_dcc_offset != 0 &&
1331e6304f5d924a Bas Nieuwenhuizen 2020-11-11  801  			    render_dcc_offset != afb->base.offsets[1] &&
1331e6304f5d924a Bas Nieuwenhuizen 2020-11-11  802  			    render_dcc_offset < UINT_MAX) {
1331e6304f5d924a Bas Nieuwenhuizen 2020-11-11  803  				uint32_t dcc_block_bits;  /* of base surface data */
816853f9dc4057b6 Bas Nieuwenhuizen 2020-11-11  804  
1331e6304f5d924a Bas Nieuwenhuizen 2020-11-11  805  				modifier |= AMD_FMT_MOD_SET(DCC_RETILE, 1);
1331e6304f5d924a Bas Nieuwenhuizen 2020-11-11  806  				afb->base.offsets[2] = render_dcc_offset;
1331e6304f5d924a Bas Nieuwenhuizen 2020-11-11  807  
1331e6304f5d924a Bas Nieuwenhuizen 2020-11-11  808  				if (adev->family >= AMDGPU_FAMILY_NV) {
1331e6304f5d924a Bas Nieuwenhuizen 2020-11-11  809  					int extra_pipe = 0;
1331e6304f5d924a Bas Nieuwenhuizen 2020-11-11  810  
1331e6304f5d924a Bas Nieuwenhuizen 2020-11-11  811  					if (adev->asic_type >= CHIP_SIENNA_CICHLID &&
1331e6304f5d924a Bas Nieuwenhuizen 2020-11-11  812  					    pipes == packers && pipes > 1)
1331e6304f5d924a Bas Nieuwenhuizen 2020-11-11  813  						extra_pipe = 1;
1331e6304f5d924a Bas Nieuwenhuizen 2020-11-11  814  
1331e6304f5d924a Bas Nieuwenhuizen 2020-11-11  815  					dcc_block_bits = max(20, 16 + pipes + extra_pipe);
1331e6304f5d924a Bas Nieuwenhuizen 2020-11-11  816  				} else {
1331e6304f5d924a Bas Nieuwenhuizen 2020-11-11  817  					modifier |= AMD_FMT_MOD_SET(RB, rb) |
1331e6304f5d924a Bas Nieuwenhuizen 2020-11-11  818  						    AMD_FMT_MOD_SET(PIPE, pipes);
1331e6304f5d924a Bas Nieuwenhuizen 2020-11-11  819  					dcc_block_bits = max(20, 18 + rb);
1331e6304f5d924a Bas Nieuwenhuizen 2020-11-11  820  				}
1331e6304f5d924a Bas Nieuwenhuizen 2020-11-11  821  
1331e6304f5d924a Bas Nieuwenhuizen 2020-11-11  822  				dcc_block_bits -= ilog2(afb->base.format->cpp[0]);
1331e6304f5d924a Bas Nieuwenhuizen 2020-11-11  823  				afb->base.pitches[2] = ALIGN(afb->base.width,
1331e6304f5d924a Bas Nieuwenhuizen 2020-11-11  824  							     1u << ((dcc_block_bits + 1) / 2));
1331e6304f5d924a Bas Nieuwenhuizen 2020-11-11  825  			}
816853f9dc4057b6 Bas Nieuwenhuizen 2020-11-11 @826  			format_info = amdgpu_lookup_format_info(afb->base.format->format,
                                                                                                                ^^^^^^^^^^^^^^^^^^
Dereferenced here

816853f9dc4057b6 Bas Nieuwenhuizen 2020-11-11  827  								modifier);
816853f9dc4057b6 Bas Nieuwenhuizen 2020-11-11  828  			if (!format_info)
816853f9dc4057b6 Bas Nieuwenhuizen 2020-11-11  829  				return -EINVAL;
816853f9dc4057b6 Bas Nieuwenhuizen 2020-11-11  830  
0a6703fd10d106b3 Zhan Liu          2020-12-29 @831  			if (!afb->base.format)
                                                                            ^^^^^^^^^^^^^^^^^
New NULL check is too late.

816853f9dc4057b6 Bas Nieuwenhuizen 2020-11-11  832  				afb->base.format = format_info;
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  833  		}
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  834  	}
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  835  
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  836  	afb->base.modifier = modifier;
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  837  	afb->base.flags |= DRM_MODE_FB_MODIFIERS;
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  838  	return 0;
08d769151dc9690a Bas Nieuwenhuizen 2020-09-02  839  }

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Daniel Vetter Jan. 7, 2021, 5:32 p.m. UTC | #3 
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 <zhan.liu@amd.com>
> > ---
> >   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-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Liu, Zhan Jan. 7, 2021, 5:38 p.m. UTC | #4 
> -----Original Message-----
> From: Daniel Vetter <daniel@ffwll.ch>
> Sent: 2021/January/07, Thursday 12:33 PM
> To: Koenig, Christian <Christian.Koenig@amd.com>
> Cc: Liu, Zhan <Zhan.Liu@amd.com>; amd-gfx@lists.freedesktop.org; Cornij,
> Nikola <Nikola.Cornij@amd.com>; Wang, Chao-kai (Stylon)
> <Stylon.Wang@amd.com>; Wang, Chao-kai (Stylon)
> <Stylon.Wang@amd.com>; dri-devel@lists.freedesktop.org; Kazlauskas,
> Nicholas <Nicholas.Kazlauskas@amd.com>; bas@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 <zhan.liu@amd.com>
> > > ---
> > >   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-devel@lists.freedesktop.org
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flist
> > s.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-
> devel&amp;data=04%7C01%7C
> >
> zhan.liu%40amd.com%7Cda23e6e33a7e46dfc4e308d8b33242c8%7C3dd896
> 1fe4884e
> >
> 608e11a82d994e183d%7C0%7C0%7C637456375746425509%7CUnknown%
> 7CTWFpbGZsb3
> >
> d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0
> %3D%7
> >
> C1000&amp;sdata=5lCm4d6FHihfFHUf5mVym0O6lKmZHgR89%2F2Eqj2ojhg
> %3D&amp;r
> > eserved=0
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fblog.ff
> wll.ch%2F&amp;data=04%7C01%7Czhan.liu%40amd.com%7Cda23e6e33a7e
> 46dfc4e308d8b33242c8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7
> C0%7C637456375746425509%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4
> wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&a
> mp;sdata=44x858klbIcVeRtP%2BuJST2K3xuCLisjbfhV9rEQrzpA%3D&amp;rese
> rved=0
diff mbox series

Patch

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;
 		}
 	}