<div dir="ltr">We can reject invalid modifiers elsewhere, but it can't be here because it's also the non-modifier path. <div>We expose 256KB_R_X or 64KB_R_X modifiers depending on chip-specific settings, but not both. Only the optimal option is exposed. This is OK for modifiers, but not OK with AMD-specific BO metadata where the UMD determines the swizzle mode. </div><div> </div><div>Marek </div></div> <div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Jun 14, 2022 at 8:38 PM Bas Nieuwenhuizen <<a href="mailto:bas@basnieuwenhuizen.nl" target="_blank">bas@basnieuwenhuizen.nl</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Mon, Jun 13, 2022 at 1:47 PM Marek Olšák <<a href="mailto:maraeo@gmail.com" target="_blank">maraeo@gmail.com</a>> wrote: > > Bas, the code was literally rejecting swizzle modes that were not in the modifier list, which was incorrect. That's because the modifier list is a subset of all supported swizzle modes. That was WAI. The kernel is now in charge of rejecting stuff that is not capable of being displayed. Allowing all in format_mod_supported has several implications on exposed & accepted modifiers as well, that should be avoided even if we should do a behavior change for non-modifiers: We now expose (i.e. list) modifiers for formats which they don't support and we removed the check that the modifier is in the list for commits with modifiers too. Hence this logic would need a serious rework instead of the patch that was sent. What combinations were failing, and can't we just add modifiers for them? > > Marek > > On Sun, Jun 12, 2022 at 7:54 PM Bas Nieuwenhuizen <<a href="mailto:bas@basnieuwenhuizen.nl" target="_blank">bas@basnieuwenhuizen.nl</a>> wrote: >> >> On Thu, Jun 9, 2022 at 4:27 PM Aurabindo Pillai >> <<a href="mailto:aurabindo.pillai@amd.com" target="_blank">aurabindo.pillai@amd.com</a>> wrote: >> > >> > [Why&How] >> > There are cases where swizzle modes are set but modifiers arent. For >> > such a userspace, we need not check modifiers while checking >> > compatibilty in the drm hook for checking plane format. >> > >> > Ignore checking modifiers but check the DCN generation for the >> > supported swizzle mode. >> > >> > Signed-off-by: Aurabindo Pillai <<a href="mailto:aurabindo.pillai@amd.com" target="_blank">aurabindo.pillai@amd.com</a>> >> > --- >> > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 51 +++++++++++++++++-- >> > 1 file changed, 46 insertions(+), 5 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >> > index 2023baf41b7e..1322df491736 100644 >> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >> > @@ -4938,6 +4938,7 @@ static bool dm_plane_format_mod_supported(struct drm_plane *plane, >> > { >> > struct amdgpu_device *adev = drm_to_adev(plane->dev); >> > const struct drm_format_info *info = drm_format_info(format); >> > + struct hw_asic_id asic_id = adev->dm.dc->ctx->asic_id; >> > int i; >> > >> > enum dm_micro_swizzle microtile = modifier_gfx9_swizzle_mode(modifier) & 3; >> > @@ -4955,13 +4956,53 @@ static bool dm_plane_format_mod_supported(struct drm_plane *plane, >> > return true; >> > } >> > >> > - /* Check that the modifier is on the list of the plane's supported modifiers. */ >> > - for (i = 0; i < plane->modifier_count; i++) { >> > - if (modifier == plane->modifiers[i]) >> > + /* check if swizzle mode is supported by this version of DCN */ >> > + switch (asic_id.chip_family) { >> > + case FAMILY_SI: >> > + case FAMILY_CI: >> > + case FAMILY_KV: >> > + case FAMILY_CZ: >> > + case FAMILY_VI: >> > + /* AI and earlier asics does not have modifier support */ >> > + return false; >> > + break; >> > + case FAMILY_AI: >> > + case FAMILY_RV: >> > + case FAMILY_NV: >> > + case FAMILY_VGH: >> > + case FAMILY_YELLOW_CARP: >> > + case AMDGPU_FAMILY_GC_10_3_6: >> > + case AMDGPU_FAMILY_GC_10_3_7: >> > + switch (AMD_FMT_MOD_GET(TILE, modifier)) { >> > + case AMD_FMT_MOD_TILE_GFX9_64K_R_X: >> > + case AMD_FMT_MOD_TILE_GFX9_64K_D_X: >> > + case AMD_FMT_MOD_TILE_GFX9_64K_S_X: >> > + case AMD_FMT_MOD_TILE_GFX9_64K_D: >> > + return true; >> > + break; >> > + default: >> > + return false; >> > + break; >> > + } >> > + break; >> > + case AMDGPU_FAMILY_GC_11_0_0: >> > + switch (AMD_FMT_MOD_GET(TILE, modifier)) { >> > + case AMD_FMT_MOD_TILE_GFX11_256K_R_X: >> > + case AMD_FMT_MOD_TILE_GFX9_64K_R_X: >> > + case AMD_FMT_MOD_TILE_GFX9_64K_D_X: >> > + case AMD_FMT_MOD_TILE_GFX9_64K_S_X: >> > + case AMD_FMT_MOD_TILE_GFX9_64K_D: >> > + return true; >> > + break; >> > + default: >> > + return false; >> > + break; >> > + } >> > + break; >> > + default: >> > + ASSERT(0); /* Unknown asic */ >> > break; >> > } >> >> This seems broken to me. AFAICT we always return in the switch so the >> code after this that checks for valid DCC usage isn't executed. >> Checking the list of modifiers is also essential to make sure other >> stuff in the modifier is set properly. >> >> If you have userspace that is not using modifiers that is giving you >> issues, a better place to look might be >> convert_tiling_flags_to_modifier in amdgpu_display.c >> >> > - if (i == plane->modifier_count) >> > - return false; >> > >> > /* >> > * For D swizzle the canonical modifier depends on the bpp, so check >> > -- >> > 2.36.1 >> > </blockquote></div>