On Wed, May 20, 2020 at 5:59 AM Christian König <ckoenig.leichtzumerken@xxxxxxxxx> wrote: > > Am 18.05.20 um 16:06 schrieb Alex Deucher: > > On Mon, May 18, 2020 at 3:31 AM Christian König > > <ckoenig.leichtzumerken@xxxxxxxxx> wrote: > >> Am 15.05.20 um 20:31 schrieb Alex Deucher: > >>> Add some APU flags to simplify handling of different APU > >>> variants. It's easier to understand the special cases > >>> if we use names flags rather than checking device ids and > >>> silicon revisions. > >> Wouldn't it be easier and cleaner to add CHIP_RAVEN2 and CHIP_PICASSO > >> instead? > >> > >> We could keep the detection as it is and just change the asic_type in > >> soc15_common_early_init(). > > I thought about that, but then we end up having to add a bunch more > > checks in the code for all the cases where we just check for > > CHIP_RAVEN today. It also doesn't solve the fact that you can have > > multiple apu flags set for a particular asic. E.g., there are RAVEN > > based RAVEN2 chips and PICASSO based RAVEN2 chips. To account for all > > of those we'd end up needing five new CHIP_ variants just for all of > > the RAVEN chips which are pretty much all the same other than the > > firmware they load. > > Yeah, those are rather good arguments. > > I'm not 100% convinced this is the best approach, but it's better than > what we have so feel free to add an Acked-by: Christian König > <christian.koenig@xxxxxxx> to the patch. > Thanks. Anyone want to review the first two patches of this series? Alex > I would just change the "unsigned long" into either u32 or u64, some for > the other flags BTW. > > Christian. > > > > > Alex > > > >> Christian. > >> > >>> Signed-off-by: Alex Deucher <alexander.deucher@xxxxxxx> > >>> --- > >>> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 + > >>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 4 ++-- > >>> drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 3 ++- > >>> drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c | 4 ++-- > >>> drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c | 16 ++++++++-------- > >>> drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c | 2 +- > >>> drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 5 ++--- > >>> drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c | 2 +- > >>> drivers/gpu/drm/amd/amdgpu/psp_v10_0.c | 4 ++-- > >>> drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c | 6 +++--- > >>> drivers/gpu/drm/amd/amdgpu/soc15.c | 17 +++++++++++++---- > >>> drivers/gpu/drm/amd/include/amd_shared.h | 7 +++++++ > >>> .../gpu/drm/amd/powerplay/hwmgr/smu10_hwmgr.c | 3 +-- > >>> .../gpu/drm/amd/powerplay/smumgr/smu10_smumgr.c | 3 ++- > >>> 14 files changed, 47 insertions(+), 30 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h > >>> index 4a03a24348f4..2a73364f4da4 100644 > >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h > >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h > >>> @@ -738,6 +738,7 @@ struct amdgpu_device { > >>> uint32_t rev_id; > >>> uint32_t external_rev_id; > >>> unsigned long flags; > >>> + unsigned long apu_flags; > >>> int usec_timeout; > >>> const struct amdgpu_asic_funcs *asic_funcs; > >>> bool shutdown; > >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > >>> index a3a58e9f9e9c..d81b79f9b009 100644 > >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > >>> @@ -1567,9 +1567,9 @@ static int amdgpu_device_parse_gpu_info_fw(struct amdgpu_device *adev) > >>> chip_name = "vega12"; > >>> break; > >>> case CHIP_RAVEN: > >>> - if (adev->rev_id >= 8) > >>> + if (adev->apu_flags & AMD_APU_IS_RAVEN2) > >>> chip_name = "raven2"; > >>> - else if (adev->pdev->device == 0x15d8) > >>> + else if (adev->apu_flags & AMD_APU_IS_PICASSO) > >>> chip_name = "picasso"; > >>> else > >>> chip_name = "raven"; > >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c > >>> index 84cee27cd7ef..f7143d927b6d 100644 > >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c > >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c > >>> @@ -523,7 +523,8 @@ uint32_t amdgpu_display_supported_domains(struct amdgpu_device *adev, > >>> break; > >>> case CHIP_RAVEN: > >>> /* enable S/G on PCO and RV2 */ > >>> - if (adev->rev_id >= 0x8 || adev->pdev->device == 0x15d8) > >>> + if ((adev->apu_flags & AMD_APU_IS_RAVEN2) || > >>> + (adev->apu_flags & AMD_APU_IS_PICASSO)) > >>> domain |= AMDGPU_GEM_DOMAIN_GTT; > >>> break; > >>> default: > >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c > >>> index 2de99b441601..2badbc0355f2 100644 > >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c > >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c > >>> @@ -70,9 +70,9 @@ int amdgpu_vcn_sw_init(struct amdgpu_device *adev) > >>> > >>> switch (adev->asic_type) { > >>> case CHIP_RAVEN: > >>> - if (adev->rev_id >= 8) > >>> + if (adev->apu_flags & AMD_APU_IS_RAVEN2) > >>> fw_name = FIRMWARE_RAVEN2; > >>> - else if (adev->pdev->device == 0x15d8) > >>> + else if (adev->apu_flags & AMD_APU_IS_PICASSO) > >>> fw_name = FIRMWARE_PICASSO; > >>> else > >>> fw_name = FIRMWARE_RAVEN; > >>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c > >>> index 1573ac1f03b2..711e9dd19705 100644 > >>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c > >>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c > >>> @@ -959,7 +959,7 @@ static void gfx_v9_0_init_golden_registers(struct amdgpu_device *adev) > >>> case CHIP_RAVEN: > >>> soc15_program_register_sequence(adev, golden_settings_gc_9_1, > >>> ARRAY_SIZE(golden_settings_gc_9_1)); > >>> - if (adev->rev_id >= 8) > >>> + if (adev->apu_flags & AMD_APU_IS_RAVEN2) > >>> soc15_program_register_sequence(adev, > >>> golden_settings_gc_9_1_rv2, > >>> ARRAY_SIZE(golden_settings_gc_9_1_rv2)); > >>> @@ -1274,7 +1274,8 @@ static void gfx_v9_0_check_if_need_gfxoff(struct amdgpu_device *adev) > >>> case CHIP_VEGA20: > >>> break; > >>> case CHIP_RAVEN: > >>> - if (!(adev->rev_id >= 0x8 || adev->pdev->device == 0x15d8) && > >>> + if (!((adev->apu_flags & AMD_APU_IS_RAVEN2) || > >>> + (adev->apu_flags & AMD_APU_IS_PICASSO)) && > >>> ((!is_raven_kicker(adev) && > >>> adev->gfx.rlc_fw_version < 531) || > >>> (adev->gfx.rlc_feature_version < 1) || > >>> @@ -1617,9 +1618,9 @@ static int gfx_v9_0_init_microcode(struct amdgpu_device *adev) > >>> chip_name = "vega20"; > >>> break; > >>> case CHIP_RAVEN: > >>> - if (adev->rev_id >= 8) > >>> + if (adev->apu_flags & AMD_APU_IS_RAVEN2) > >>> chip_name = "raven2"; > >>> - else if (adev->pdev->device == 0x15d8) > >>> + else if (adev->apu_flags & AMD_APU_IS_PICASSO) > >>> chip_name = "picasso"; > >>> else > >>> chip_name = "raven"; > >>> @@ -2119,7 +2120,7 @@ static int gfx_v9_0_gpu_early_init(struct amdgpu_device *adev) > >>> adev->gfx.config.sc_prim_fifo_size_backend = 0x100; > >>> adev->gfx.config.sc_hiz_tile_fifo_size = 0x30; > >>> adev->gfx.config.sc_earlyz_tile_fifo_size = 0x4C0; > >>> - if (adev->rev_id >= 8) > >>> + if (adev->apu_flags & AMD_APU_IS_RAVEN2) > >>> gb_addr_config = RAVEN2_GB_ADDR_CONFIG_GOLDEN; > >>> else > >>> gb_addr_config = RAVEN_GB_ADDR_CONFIG_GOLDEN; > >>> @@ -2968,8 +2969,7 @@ static void gfx_v9_0_init_pg(struct amdgpu_device *adev) > >>> */ > >>> if (adev->gfx.rlc.is_rlc_v2_1) { > >>> if (adev->asic_type == CHIP_VEGA12 || > >>> - (adev->asic_type == CHIP_RAVEN && > >>> - adev->rev_id >= 8)) > >>> + (adev->apu_flags & AMD_APU_IS_RAVEN2)) > >>> gfx_v9_1_init_rlc_save_restore_list(adev); > >>> gfx_v9_0_enable_save_restore_machine(adev); > >>> } > >>> @@ -6881,7 +6881,7 @@ static void gfx_v9_0_set_gds_init(struct amdgpu_device *adev) > >>> adev->gds.gds_compute_max_wave_id = 0x27f; > >>> break; > >>> case CHIP_RAVEN: > >>> - if (adev->rev_id >= 0x8) > >>> + if (adev->apu_flags & AMD_APU_IS_RAVEN2) > >>> adev->gds.gds_compute_max_wave_id = 0x77; /* raven2 */ > >>> else > >>> adev->gds.gds_compute_max_wave_id = 0x15f; /* raven1 */ > >>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c > >>> index 1a2f18b908fe..6682b843bafe 100644 > >>> --- a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c > >>> +++ b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c > >>> @@ -80,7 +80,7 @@ static void gfxhub_v1_0_init_system_aperture_regs(struct amdgpu_device *adev) > >>> WREG32_SOC15_RLC(GC, 0, mmMC_VM_SYSTEM_APERTURE_LOW_ADDR, > >>> min(adev->gmc.fb_start, adev->gmc.agp_start) >> 18); > >>> > >>> - if (adev->asic_type == CHIP_RAVEN && adev->rev_id >= 0x8) > >>> + if (adev->apu_flags & AMD_APU_IS_RAVEN2) > >>> /* > >>> * Raven2 has a HW issue that it is unable to use the > >>> * vram which is out of MC_VM_SYSTEM_APERTURE_HIGH_ADDR. > >>> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c > >>> index 055ecba754ff..11e93a82131d 100644 > >>> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c > >>> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c > >>> @@ -441,9 +441,8 @@ static bool gmc_v9_0_use_invalidate_semaphore(struct amdgpu_device *adev, > >>> return ((vmhub == AMDGPU_MMHUB_0 || > >>> vmhub == AMDGPU_MMHUB_1) && > >>> (!amdgpu_sriov_vf(adev)) && > >>> - (!(adev->asic_type == CHIP_RAVEN && > >>> - adev->rev_id < 0x8 && > >>> - adev->pdev->device == 0x15d8))); > >>> + (!(!(adev->apu_flags & AMD_APU_IS_RAVEN2) && > >>> + (adev->apu_flags & AMD_APU_IS_PICASSO)))); > >>> } > >>> > >>> static bool gmc_v9_0_get_atc_vmid_pasid_mapping_info(struct amdgpu_device *adev, > >>> diff --git a/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c b/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c > >>> index bd2d2b8d1f42..405767208a4d 100755 > >>> --- a/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c > >>> +++ b/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c > >>> @@ -96,7 +96,7 @@ static void mmhub_v1_0_init_system_aperture_regs(struct amdgpu_device *adev) > >>> WREG32_SOC15(MMHUB, 0, mmMC_VM_SYSTEM_APERTURE_LOW_ADDR, > >>> min(adev->gmc.fb_start, adev->gmc.agp_start) >> 18); > >>> > >>> - if (adev->asic_type == CHIP_RAVEN && adev->rev_id >= 0x8) > >>> + if (adev->apu_flags & AMD_APU_IS_RAVEN2) > >>> /* > >>> * Raven2 has a HW issue that it is unable to use the vram which > >>> * is out of MC_VM_SYSTEM_APERTURE_HIGH_ADDR. So here is the > >>> diff --git a/drivers/gpu/drm/amd/amdgpu/psp_v10_0.c b/drivers/gpu/drm/amd/amdgpu/psp_v10_0.c > >>> index 90727cfb4447..d7f92634eba2 100644 > >>> --- a/drivers/gpu/drm/amd/amdgpu/psp_v10_0.c > >>> +++ b/drivers/gpu/drm/amd/amdgpu/psp_v10_0.c > >>> @@ -55,9 +55,9 @@ static int psp_v10_0_init_microcode(struct psp_context *psp) > >>> > >>> switch (adev->asic_type) { > >>> case CHIP_RAVEN: > >>> - if (adev->rev_id >= 0x8) > >>> + if (adev->apu_flags & AMD_APU_IS_RAVEN2) > >>> chip_name = "raven2"; > >>> - else if (adev->pdev->device == 0x15d8) > >>> + else if (adev->apu_flags & AMD_APU_IS_PICASSO) > >>> chip_name = "picasso"; > >>> else > >>> chip_name = "raven"; > >>> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c > >>> index 12a77b31fad2..9077507b425a 100644 > >>> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c > >>> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c > >>> @@ -486,7 +486,7 @@ static void sdma_v4_0_init_golden_registers(struct amdgpu_device *adev) > >>> soc15_program_register_sequence(adev, > >>> golden_settings_sdma_4_1, > >>> ARRAY_SIZE(golden_settings_sdma_4_1)); > >>> - if (adev->rev_id >= 8) > >>> + if (adev->apu_flags & AMD_APU_IS_RAVEN2) > >>> soc15_program_register_sequence(adev, > >>> golden_settings_sdma_rv2, > >>> ARRAY_SIZE(golden_settings_sdma_rv2)); > >>> @@ -575,9 +575,9 @@ static int sdma_v4_0_init_microcode(struct amdgpu_device *adev) > >>> chip_name = "vega20"; > >>> break; > >>> case CHIP_RAVEN: > >>> - if (adev->rev_id >= 8) > >>> + if (adev->apu_flags & AMD_APU_IS_RAVEN2) > >>> chip_name = "raven2"; > >>> - else if (adev->pdev->device == 0x15d8) > >>> + else if (adev->apu_flags & AMD_APU_IS_PICASSO) > >>> chip_name = "picasso"; > >>> else > >>> chip_name = "raven"; > >>> diff --git a/drivers/gpu/drm/amd/amdgpu/soc15.c b/drivers/gpu/drm/amd/amdgpu/soc15.c > >>> index 58a440a15525..c7c9e07962b9 100644 > >>> --- a/drivers/gpu/drm/amd/amdgpu/soc15.c > >>> +++ b/drivers/gpu/drm/amd/amdgpu/soc15.c > >>> @@ -564,7 +564,8 @@ soc15_asic_reset_method(struct amdgpu_device *adev) > >>> static int soc15_asic_reset(struct amdgpu_device *adev) > >>> { > >>> /* original raven doesn't have full asic reset */ > >>> - if (adev->pdev->device == 0x15dd && adev->rev_id < 0x8) > >>> + if ((adev->apu_flags & AMD_APU_IS_RAVEN) && > >>> + !(adev->apu_flags & AMD_APU_IS_RAVEN2)) > >>> return 0; > >>> > >>> switch (soc15_asic_reset_method(adev)) { > >>> @@ -1129,16 +1130,23 @@ static int soc15_common_early_init(void *handle) > >>> break; > >>> case CHIP_RAVEN: > >>> adev->asic_funcs = &soc15_asic_funcs; > >>> + if (adev->pdev->device == 0x15dd) > >>> + adev->apu_flags |= AMD_APU_IS_RAVEN; > >>> + if (adev->pdev->device == 0x15d8) > >>> + adev->apu_flags |= AMD_APU_IS_PICASSO; > >>> if (adev->rev_id >= 0x8) > >>> + adev->apu_flags |= AMD_APU_IS_RAVEN2; > >>> + > >>> + if (adev->apu_flags & AMD_APU_IS_RAVEN2) > >>> adev->external_rev_id = adev->rev_id + 0x79; > >>> - else if (adev->pdev->device == 0x15d8) > >>> + else if (adev->apu_flags & AMD_APU_IS_PICASSO) > >>> adev->external_rev_id = adev->rev_id + 0x41; > >>> else if (adev->rev_id == 1) > >>> adev->external_rev_id = adev->rev_id + 0x20; > >>> else > >>> adev->external_rev_id = adev->rev_id + 0x01; > >>> > >>> - if (adev->rev_id >= 0x8) { > >>> + if (adev->apu_flags & AMD_APU_IS_RAVEN2) { > >>> adev->cg_flags = AMD_CG_SUPPORT_GFX_MGCG | > >>> AMD_CG_SUPPORT_GFX_MGLS | > >>> AMD_CG_SUPPORT_GFX_CP_LS | > >>> @@ -1156,7 +1164,7 @@ static int soc15_common_early_init(void *handle) > >>> AMD_CG_SUPPORT_VCN_MGCG; > >>> > >>> adev->pg_flags = AMD_PG_SUPPORT_SDMA | AMD_PG_SUPPORT_VCN; > >>> - } else if (adev->pdev->device == 0x15d8) { > >>> + } else if (adev->apu_flags & AMD_APU_IS_PICASSO) { > >>> adev->cg_flags = AMD_CG_SUPPORT_GFX_MGCG | > >>> AMD_CG_SUPPORT_GFX_MGLS | > >>> AMD_CG_SUPPORT_GFX_CP_LS | > >>> @@ -1222,6 +1230,7 @@ static int soc15_common_early_init(void *handle) > >>> break; > >>> case CHIP_RENOIR: > >>> adev->asic_funcs = &soc15_asic_funcs; > >>> + adev->apu_flags |= AMD_APU_IS_RENOIR; > >>> adev->cg_flags = AMD_CG_SUPPORT_GFX_MGCG | > >>> AMD_CG_SUPPORT_GFX_MGLS | > >>> AMD_CG_SUPPORT_GFX_3D_CGCG | > >>> diff --git a/drivers/gpu/drm/amd/include/amd_shared.h b/drivers/gpu/drm/amd/include/amd_shared.h > >>> index d655a76bedc6..258e41890a15 100644 > >>> --- a/drivers/gpu/drm/amd/include/amd_shared.h > >>> +++ b/drivers/gpu/drm/amd/include/amd_shared.h > >>> @@ -40,6 +40,13 @@ enum amd_chip_flags { > >>> AMD_EXP_HW_SUPPORT = 0x00080000UL, > >>> }; > >>> > >>> +enum amd_apu_flags { > >>> + AMD_APU_IS_RAVEN = 0x00000001UL, > >>> + AMD_APU_IS_RAVEN2 = 0x00000002UL, > >>> + AMD_APU_IS_PICASSO = 0x00000004UL, > >>> + AMD_APU_IS_RENOIR = 0x00000008UL, > >>> +}; > >>> + > >>> enum amd_ip_block_type { > >>> AMD_IP_BLOCK_TYPE_COMMON, > >>> AMD_IP_BLOCK_TYPE_GMC, > >>> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu10_hwmgr.c b/drivers/gpu/drm/amd/powerplay/hwmgr/smu10_hwmgr.c > >>> index 4f8c1b85e688..246bb9ac557d 100644 > >>> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu10_hwmgr.c > >>> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu10_hwmgr.c > >>> @@ -1304,8 +1304,7 @@ static int smu10_get_power_profile_mode(struct pp_hwmgr *hwmgr, char *buf) > >>> static bool smu10_is_raven1_refresh(struct pp_hwmgr *hwmgr) > >>> { > >>> struct amdgpu_device *adev = hwmgr->adev; > >>> - if ((adev->asic_type == CHIP_RAVEN) && > >>> - (adev->rev_id != 0x15d8) && > >>> + if ((adev->apu_flags & AMD_APU_IS_RAVEN) && > >>> (hwmgr->smu_version >= 0x41e2b)) > >>> return true; > >>> else > >>> diff --git a/drivers/gpu/drm/amd/powerplay/smumgr/smu10_smumgr.c b/drivers/gpu/drm/amd/powerplay/smumgr/smu10_smumgr.c > >>> index 801ba9ca6094..ea2279bb8cbf 100644 > >>> --- a/drivers/gpu/drm/amd/powerplay/smumgr/smu10_smumgr.c > >>> +++ b/drivers/gpu/drm/amd/powerplay/smumgr/smu10_smumgr.c > >>> @@ -226,7 +226,8 @@ static int smu10_start_smu(struct pp_hwmgr *hwmgr) > >>> smum_send_msg_to_smc(hwmgr, PPSMC_MSG_GetSmuVersion, &hwmgr->smu_version); > >>> adev->pm.fw_version = hwmgr->smu_version >> 8; > >>> > >>> - if (adev->rev_id < 0x8 && adev->pdev->device != 0x15d8 && > >>> + if (!(adev->apu_flags & AMD_APU_IS_RAVEN2) && > >>> + (adev->apu_flags & AMD_APU_IS_RAVEN) && > >>> adev->pm.fw_version < 0x1e45) > >>> adev->pm.pp_feature &= ~PP_GFXOFF_MASK; > >>> > > _______________________________________________ > > amd-gfx mailing list > > amd-gfx@xxxxxxxxxxxxxxxxxxxxx > > https://lists.freedesktop.org/mailman/listinfo/amd-gfx > _______________________________________________ amd-gfx mailing list amd-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/amd-gfx