[PATCH 1/2] drm/amdgpu: unify the way to judge RAS feature readiness

[Quan, Evan]

> -----Original Message-----
> From: Pan, Xinhui
> Sent: 2019年3月11日 13:08
> To: Quan, Evan <Evan.Quan at amd.com>; amd-gfx at lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher at amd.com>; Quan, Evan
> <Evan.Quan at amd.com>
> Subject: RE: [PATCH 1/2] drm/amdgpu: unify the way to judge RAS feature
> readiness
> 
> 
> Inline.
> 
> -----Original Message-----
> From: Evan Quan <evan.quan at amd.com>
> Sent: 2019年3月11日 12:31
> To: amd-gfx at lists.freedesktop.org
> Cc: Pan, Xinhui <Xinhui.Pan at amd.com>; Deucher, Alexander
> <Alexander.Deucher at amd.com>; Quan, Evan <Evan.Quan at amd.com>
> Subject: [PATCH 1/2] drm/amdgpu: unify the way to judge RAS feature
> readiness
> 
> Unify the way to judge whether a specific RAS feature is supported.
> 
> Change-Id: I14bb19db49f06e134de903376b14eb27e0e038c7
> Signed-off-by: Evan Quan <evan.quan at amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c | 10 ++--------
> drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h |  2 +-
>  2 files changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> index bf462c59cb76..7f9cbd64cb20 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> @@ -466,12 +466,6 @@ static struct ras_manager
> *amdgpu_ras_find_obj(struct amdgpu_device *adev,
>  /* obj end */
> 
>  /* feature ctl begin */
> -static int amdgpu_ras_is_feature_allowed(struct amdgpu_device *adev,
> -		struct ras_common_if *head)
> -{
> -	return amdgpu_ras_enable && (amdgpu_ras_mask & BIT(head-
> >block));
> -}
> -
>  static int amdgpu_ras_is_feature_enabled(struct amdgpu_device *adev,
>  		struct ras_common_if *head)
>  {
> @@ -490,7 +484,7 @@ static int __amdgpu_ras_feature_enable(struct
> amdgpu_device *adev,
>  	struct amdgpu_ras *con = amdgpu_ras_get_context(adev);
>  	struct ras_manager *obj = amdgpu_ras_find_obj(adev, head);
> 
> -	if (!amdgpu_ras_is_feature_allowed(adev, head))
> +	if (!amdgpu_ras_is_supported(adev, head->block))
>  		return 0;
> 
> [XH] I am thinking of splitting supported to hw_supported and sw_supported.
> The code become a little clearer now.
> For hw_supported case, we need allow IP disable ras.
> For sw_suppporeted case, we need allow IP disable/enable/inject/query,
> etc .
> 
[Quan, Evan] "sw_supported" here means amdgpu_ras_enable/mask module parameters. Right?
[Quan, Evan] And do you mean even with "sw_supported" disabled(ras_enable/mask = 0), there will be still RAS disable functionality as long as
hardware supports it? Not quite sure, that sounds a little confusing.
Anyway, I think you can add one more flag to existing amdgpu_ras_is_supported() to support this. Maintaining two APIs which do almost the
same thing is error-prone.
> 
>  	if (!(!!enable ^ !!amdgpu_ras_is_feature_enabled(adev, head)))
>  		return 0;
> @@ -539,7 +533,7 @@ int amdgpu_ras_feature_enable(struct
> amdgpu_device *adev,
>  	}
> 
>  	/* Do not enable if it is not allowed. */
> -	WARN_ON(enable && !amdgpu_ras_is_feature_allowed(adev,
> head));
> +	WARN_ON(enable && !amdgpu_ras_is_supported(adev, head-
> >block));
>  	/* Are we alerady in that state we are going to set? */
>  	if (!(!!enable ^ !!amdgpu_ras_is_feature_enabled(adev, head)))
>  		return 0;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h
> index 02cb9a13ddc5..0ef2b91b8fcd 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h
> @@ -163,7 +163,7 @@ struct ras_debug_if {
> 
>  /* check if ras is supported on block, say, sdma, gfx */  static inline int
> amdgpu_ras_is_supported(struct amdgpu_device *adev,
> -		unsigned int block)
> +		enum amdgpu_ras_block block)
>  {
>  	struct amdgpu_ras *ras = amdgpu_ras_get_context(adev);
> 
> --
> 2.21.0