Re: [PATCH 02/17] drm/amdgpu: Add infrastructure for Cleaner Shader feature

[Christian König <ckoenig.leichtzumerken@xxxxxxxxx>]

Am 15.08.24 um 02:04 schrieb Alex Deucher:

> From: Srinivasan Shanmugam <srinivasan.shanmugam@xxxxxxx>
>
> The cleaner shader is used by the CP firmware to clean LDS and GPRs
> between processes on the CUs.
>
> This adds an internal API for GFX IP code to allocate and initialize the
> cleaner shader.
>
> Cc: Christian König <christian.koenig@xxxxxxx>
> Cc: Alex Deucher <alexander.deucher@xxxxxxx>
> Signed-off-by: Alex Deucher <alexander.deucher@xxxxxxx>
> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@xxxxxxx>
> Suggested-by: Christian König <christian.koenig@xxxxxxx>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | 35 +++++++++++++++++++++++++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h | 14 ++++++++++
>  2 files changed, 49 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> index 9be8cafdcecc..4ed69fcfe9c1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> @@ -1416,3 +1416,38 @@ void amdgpu_gfx_sysfs_fini(struct amdgpu_device *adev)
>  	device_remove_file(adev->dev, &dev_attr_current_compute_partition);
>  	device_remove_file(adev->dev, &dev_attr_available_compute_partition);
>  }
> +
> +int amdgpu_gfx_cleaner_shader_sw_init(struct amdgpu_device *adev,
> +				      unsigned int cleaner_shader_size)
> +{
> +	if (!adev->gfx.enable_cleaner_shader)
> +		return -EOPNOTSUPP;
>     
    
    It's probably better to not call the function in the first place
    instead of returning an error.
    
> +
> +	return amdgpu_bo_create_kernel(adev, cleaner_shader_size, PAGE_SIZE,
> +				       AMDGPU_GEM_DOMAIN_VRAM | AMDGPU_GEM_DOMAIN_GTT,
> +				       &adev->gfx.cleaner_shader_obj,
> +				       &adev->gfx.cleaner_shader_gpu_addr,
> +				       (void **)&adev->gfx.cleaner_shader_cpu_ptr);
> +}
> +
> +void amdgpu_gfx_cleaner_shader_sw_fini(struct amdgpu_device *adev)
> +{
> +	if (!adev->gfx.enable_cleaner_shader)
> +		return;
> +
> +	amdgpu_bo_free_kernel(&adev->gfx.cleaner_shader_obj,
> +			      &adev->gfx.cleaner_shader_gpu_addr,
> +			      (void **)&adev->gfx.cleaner_shader_cpu_ptr);
> +}
> +
> +void amdgpu_gfx_cleaner_shader_init(struct amdgpu_device *adev,
> +				    unsigned int cleaner_shader_size,
> +				    const void *cleaner_shader_ptr)
> +{
> +	if (!adev->gfx.enable_cleaner_shader)
> +		return;
> +
> +	if (adev->gfx.cleaner_shader_cpu_ptr && cleaner_shader_ptr)
> +		memcpy_toio(adev->gfx.cleaner_shader_cpu_ptr, cleaner_shader_ptr,
> +			    cleaner_shader_size);
> +}
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
> index 6fe77e483bb7..5ff3ab7d429a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
> @@ -444,6 +444,14 @@ struct amdgpu_gfx {
>  	uint32_t			*ip_dump_core;
>  	uint32_t			*ip_dump_compute_queues;
>  	uint32_t			*ip_dump_gfx_queues;
> +
> +	/* cleaner shader */
> +	struct amdgpu_bo		*cleaner_shader_obj;
>     
    

> +	unsigned int                    cleaner_shader_size;
>     
    This is a parameter now and the member unused.
    
> +	u64				cleaner_shader_gpu_addr;
> +	void				*cleaner_shader_cpu_ptr;
> +	const void			*cleaner_shader_ptr;
>     
    
> +	bool				enable_cleaner_shader;
>     
    Probably better to test if cleaner_shader_obj is allocated or not
    instead of having a separate bool.
    
    Regards,
    Christian.
    
>  };
>  
>  struct amdgpu_gfx_ras_reg_entry {
> @@ -545,6 +553,12 @@ void amdgpu_gfx_ras_error_func(struct amdgpu_device *adev,
>  		void *ras_error_status,
>  		void (*func)(struct amdgpu_device *adev, void *ras_error_status,
>  				int xcc_id));
> +int amdgpu_gfx_cleaner_shader_sw_init(struct amdgpu_device *adev,
> +				      unsigned int cleaner_shader_size);
> +void amdgpu_gfx_cleaner_shader_sw_fini(struct amdgpu_device *adev);
> +void amdgpu_gfx_cleaner_shader_init(struct amdgpu_device *adev,
> +				    unsigned int cleaner_shader_size,
> +				    const void *cleaner_shader_ptr);
>  
>  static inline const char *amdgpu_gfx_compute_mode_desc(int mode)
>  {
>