Re: [PATCH 2/3] drm/amdgpu: Save some cycles on the job submission path

["Timur Kristóf" <timur.kristof@gmail.com>]

On 2026. június 26., péntek 10:55:57 közép-európai nyári idő Tvrtko Ursulin 
wrote:
> Every job submission on the Steam Deck ends up walking the list of IP
> blocks looking for AMD_IP_BLOCK_TYPE_SMC. Half of the call chain is like
> the below, while the second half is from amdgpu_gfx_profile_ring_end_use:
> 
>  amdgpu_gfx_profile_ring_begin_use
>   amdgpu_dpm_is_overdrive_enabled
>    is_support_sw_smu
>     amdgpu_device_ip_is_valid
> 
> On a game menu screen at 90Hz refresh rate we end up with ~840 calls per
> second which sticks out when the submission worker is profiled with perf:
> 
>   13.78%  [kernel]  [k] __lock_text_start
>   10.86%  [kernel]  [k] __lookup_object
>    8.76%  [kernel]  [k] __mod_timer
>    4.94%  [kernel]  [k] queued_spin_lock_slowpath
>    1.66%  [kernel]  [k] amdgpu_device_ip_is_valid
>    1.54%  [kernel]  [k] preempt_count_add
>    1.42%  [kernel]  [k] amdgpu_sync_peek_fence
>    1.18%  [kernel]  [k] amdgpu_vmid_grab
>    1.17%  [kernel]  [k] amdgpu_ib_schedule
>    1.14%  [kernel]  [k] kthread_worker_fn
> 
> Lets short-circuit this walk by simply caching the result of
> is_support_sw_smu() in the device.
> 
> This is a micro-improvement but it is at least conceptually nicer to avoid
> repeating the same walk so much.

Hi,

I agree with cleaning up this thing.
Reviewed-by: Timur Kristóf <timur.kristof@gmail.com>

That being said, I think is_support_sw_smu() is horrible and should be removed 
alltogether, because it goes against how the rest of the power management code 
works.

In my opinion, we should instead:

1. Hook up some function pointers and check those instead,
For example in amdgpu_pm_acpi_event_handler() we should just hook up 
smu_set_ac_dc() to the notify_ac_dc() function pointer. There are plenty of 
other similar cases.
Another example, for amdgpu_dpm_mode1_reset() we should introduce a new 
asic_reset_mode_1() pointer in amd_pm_funcs() similar to how it works with 
MODE2 reset for consistency.

2. Eliminate redundant functions where the same thing is already done 
elsewhere.
For example in amdgpu_dpm_is_mode1_reset_supported() it checks 
smu_mode1_reset_is_support() which is redundant because the supported reset 
type is available on the ASIC functions already and we can just use that.

What do you think?

Thanks & best regards,
Timur

> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Timur Kristóf <timur.kristof@gmail.com>
> ---
> v2:
>  * Approach changed to cache sw_smu status only.
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu.h           |  1 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c    |  3 +++
>  drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c     | 14 +++++---------
>  drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h |  8 +++++++-
>  4 files changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h index 7b09410d6d8f..9803967d15f9
> 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -851,6 +851,7 @@ struct amdgpu_device {
>  	struct dev_pm_domain		vga_pm_domain;
>  	bool				have_disp_power_ref;
>  	bool                            have_atomics_support;
> +	bool				is_sw_smu;
> 
>  	/* BIOS */
>  	bool				is_atom_fw;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c index
> 1e6b75ecafe4..7f935a5778b0 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -74,6 +74,7 @@
>  #include "amdgpu_ras.h"
>  #include "amdgpu_ras_mgr.h"
>  #include "amdgpu_pmu.h"
> +#include "amdgpu_smu.h"
>  #include "amdgpu_fru_eeprom.h"
>  #include "amdgpu_reset.h"
>  #include "amdgpu_virt.h"
> @@ -2130,6 +2131,8 @@ static int amdgpu_device_ip_early_init(struct
> amdgpu_device *adev) adev->cg_flags &= amdgpu_cg_mask;
>  	adev->pg_flags &= amdgpu_pg_mask;
> 
> +	amdgpu_smu_early_init(adev);
> +
>  	return 0;
>  }
> 
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c index
> 208a2fba6d40..82c9ae6a5092 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> @@ -591,17 +591,13 @@ static int smu_get_power_num_states(void *handle,
>  	return 0;
>  }
> 
> -bool is_support_sw_smu(struct amdgpu_device *adev)
> +void amdgpu_smu_early_init(struct amdgpu_device *adev)
>  {
>  	/* vega20 is 11.0.2, but it's supported via the powerplay code */
> -	if (adev->asic_type == CHIP_VEGA20)
> -		return false;
> -
> -	if ((amdgpu_ip_version(adev, MP1_HWIP, 0) >= IP_VERSION(11, 0, 0)) 
&&
> -	    amdgpu_device_ip_is_valid(adev, AMD_IP_BLOCK_TYPE_SMC))
> -		return true;
> -
> -	return false;
> +	adev->is_sw_smu = adev->asic_type != CHIP_VEGA20 &&
> +			  (amdgpu_ip_version(adev, MP1_HWIP, 0) >=
> +			   IP_VERSION(11, 0, 0) &&
> +			   amdgpu_device_ip_is_valid(adev, 
AMD_IP_BLOCK_TYPE_SMC));
>  }
> 
>  bool is_support_cclk_dpm(struct amdgpu_device *adev)
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
> b/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h index
> d76e0b005308..efc52d97058b 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
> +++ b/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
> @@ -1952,7 +1952,13 @@ int smu_link_reset(struct smu_context *smu);
> 
>  extern const struct amd_ip_funcs smu_ip_funcs;
> 
> -bool is_support_sw_smu(struct amdgpu_device *adev);
> +void amdgpu_smu_early_init(struct amdgpu_device *adev);
> +
> +static inline bool is_support_sw_smu(struct amdgpu_device *adev)
> +{
> +	return adev->is_sw_smu;
> +}
> +
>  bool is_support_cclk_dpm(struct amdgpu_device *adev);
>  int smu_write_watermarks_table(struct smu_context *smu);