On 9/4/2024 6:57 PM, Srinivasan Shanmugam wrote:
> This commit modifies the initialization only if the cleaner shader
> object has been allocated. This is done by adding checks for
> adev->gfx.cleaner_shader_obj before calling
> amdgpu_gfx_cleaner_shader_init
>
> The changes are made in the gfx_v9_4_3_sw_init, gfx_v9_4_3_sw_fini, and
> gfx_v9_4_3_hw_init functions. These functions are responsible for
> initializing software components of the GFX v9.4.3 engines.
>
> This change prevents unnecessary function calls and makes the control
> flow of the program clearer. It also ensures that the cleaner shader is
> only initialized when it has been properly allocated.
>
> Fixes: 1b66421d29b7 ("drm/amdgpu/gfx9: Implement cleaner shader support for GFX9.4.3 hardware")
> Cc: Christian König <christian.koenig@xxxxxxx>
> Cc: Alex Deucher <alexander.deucher@xxxxxxx>
> Suggested-by: Christian König <christian.koenig@xxxxxxx>
> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@xxxxxxx>
> ---
> drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.c | 17 ++++++++++-------
> 1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.c b/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.c
> index 408e5600bb61..abf934863421 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.c
> @@ -1061,10 +1061,12 @@ static int gfx_v9_4_3_sw_init(void *handle)
> adev->gfx.cleaner_shader_size = sizeof(gfx_9_4_3_cleaner_shader_hex);
> if (adev->gfx.mec_fw_version >= 153) {
It's better to bring inside this one assignment of shader binary/size
> adev->gfx.enable_cleaner_shader = true;
> - r = amdgpu_gfx_cleaner_shader_sw_init(adev, adev->gfx.cleaner_shader_size);
> - if (r) {
> - adev->gfx.enable_cleaner_shader = false;
> - dev_err(adev->dev, "Failed to initialize cleaner shader\n");
> + if (adev->gfx.cleaner_shader_obj) {
Keep this outside and check if a valid shader binary is available (size
! = 0). If so, do a sw_init
> + r = amdgpu_gfx_cleaner_shader_sw_init(adev);> + if (r) {
> + adev->gfx.enable_cleaner_shader = false;
Move this state change inside amdgpu_gfx_cleaner_shader_sw_init such
that cleaner shader API keeps the state - cleaner shader is enabled if a
valid CPU and GPU pointer is available.
Any further calls to cleaner shader API - - ex:
amdgpu_gfx_cleaner_shader_init() - just need to check this state and
take action. Basically, all the checks may be moved inside the cleaner
shader API rather than implementing this at every client interface.
Thanks,
Lijo
> + dev_err(adev->dev, "Failed to initialize cleaner shader\n");
> + }
> }
> }
> break;
> @@ -1196,7 +1198,8 @@ static int gfx_v9_4_3_sw_fini(void *handle)
> amdgpu_gfx_kiq_fini(adev, i);
> }
>
> - amdgpu_gfx_cleaner_shader_sw_fini(adev);
> + if (adev->gfx.cleaner_shader_obj)
> + amdgpu_gfx_cleaner_shader_sw_fini(adev);
>
> gfx_v9_4_3_mec_fini(adev);
> amdgpu_bo_unref(&adev->gfx.rlc.clear_state_obj);
> @@ -2344,8 +2347,8 @@ static int gfx_v9_4_3_hw_init(void *handle)
> int r;
> struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>
> - amdgpu_gfx_cleaner_shader_init(adev, adev->gfx.cleaner_shader_size,
> - adev->gfx.cleaner_shader_ptr);
> + if (adev->gfx.cleaner_shader_obj)
> + amdgpu_gfx_cleaner_shader_init(adev);
>
> if (!amdgpu_sriov_vf(adev))
> gfx_v9_4_3_init_golden_registers(adev);
