[PATCH] drm/amdgpu: Skip some registers config for SRIOV

[Luben Tuikov]

On 2020-08-06 3:40 a.m., Liu ChengZhe wrote:
> For VF, registers L2_CNTL, L2_CONTEXT1_IDENTITY_APERTURE
> L2_PROTECTION_FAULT_CNTL are not accesible, skip the

Spelling: "accessible".

I'd rather move the "For VF" to the end, after "accessible",
like this. I also believe it should be "to VF" as opposed
to "for VF". Both are correct, but mean different things.
Maybe like this:

Some registers are not accessible to virtual function
setup, so skip their initialization when in VF-SRIOV mode.

> configuration for them in SRIOV mode.
> 
> Signed-off-by: Liu ChengZhe <ChengZhe.Liu at amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/gfxhub_v2_1.c | 12 ++++++++++--
>  drivers/gpu/drm/amd/amdgpu/mmhub_v2_0.c  | 12 ++++++++++--
>  2 files changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfxhub_v2_1.c b/drivers/gpu/drm/amd/amdgpu/gfxhub_v2_1.c
> index 1f6112b7fa49..6b96f45fde2a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfxhub_v2_1.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfxhub_v2_1.c
> @@ -330,10 +330,13 @@ int gfxhub_v2_1_gart_enable(struct amdgpu_device *adev)
>  	gfxhub_v2_1_init_gart_aperture_regs(adev);
>  	gfxhub_v2_1_init_system_aperture_regs(adev);
>  	gfxhub_v2_1_init_tlb_regs(adev);
> -	gfxhub_v2_1_init_cache_regs(adev);
> +	if (!amdgpu_sriov_vf(adev))
> +		gfxhub_v2_1_init_cache_regs(adev);

Yes, that's the literal meaning of the commit description,
but it would be cleaner if gfxhub_v2_1_init_cache_regs(adev)
did that check instead. (Some might even argue it'd be more
object-oriented...)

So then, this code would look like a sequence of
statements, unchanged, and each method of initialization
would know/check whether it is applicable to "adev".
It also makes it more centralized, less code duplication
and less code clutter.

>  
>  	gfxhub_v2_1_enable_system_domain(adev);
> -	gfxhub_v2_1_disable_identity_aperture(adev);
> +	if (!amdgpu_sriov_vf(adev))
> +		gfxhub_v2_1_disable_identity_aperture(adev);
> +

Ditto.

>  	gfxhub_v2_1_setup_vmid_config(adev);
>  	gfxhub_v2_1_program_invalidation(adev);
>  
> @@ -372,7 +375,12 @@ void gfxhub_v2_1_gart_disable(struct amdgpu_device *adev)
>  void gfxhub_v2_1_set_fault_enable_default(struct amdgpu_device *adev,
>  					  bool value)
>  {
> +	/*These regs are not accessible for VF, PF will program in SRIOV */

Add a space after the asterisk and before the beginning of the sentence.
Format the comment in one of the two acceptable Linux Kernel Coding styles.
End the sentence with a full stop. It also helps to spell out "registers".
Like this perhaps:

	/* These registers are not accessible to VF-SRIOV.
	 * The PF will program them instead.
	 */
> +	if (amdgpu_sriov_vf(adev))
> +		return;
> +
>  	u32 tmp;

The definition of this automatic variable should precede
all other code in this function. The compiler will optimize
it away anyway.

> +
>  	tmp = RREG32_SOC15(GC, 0, mmGCVM_L2_PROTECTION_FAULT_CNTL);
>  	tmp = REG_SET_FIELD(tmp, GCVM_L2_PROTECTION_FAULT_CNTL,
>  			    RANGE_PROTECTION_FAULT_ENABLE_DEFAULT, value);
> diff --git a/drivers/gpu/drm/amd/amdgpu/mmhub_v2_0.c b/drivers/gpu/drm/amd/amdgpu/mmhub_v2_0.c
> index d83912901f73..9cfde9b81600 100644
> --- a/drivers/gpu/drm/amd/amdgpu/mmhub_v2_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/mmhub_v2_0.c
> @@ -321,10 +321,13 @@ int mmhub_v2_0_gart_enable(struct amdgpu_device *adev)
>  	mmhub_v2_0_init_gart_aperture_regs(adev);
>  	mmhub_v2_0_init_system_aperture_regs(adev);
>  	mmhub_v2_0_init_tlb_regs(adev);
> -	mmhub_v2_0_init_cache_regs(adev);
> +	if (!amdgpu_sriov_vf(adev))
> +		mmhub_v2_0_init_cache_regs(adev);

Move this check inside said function.

>  
>  	mmhub_v2_0_enable_system_domain(adev);
> -	mmhub_v2_0_disable_identity_aperture(adev);
> +	if (!amdgpu_sriov_vf(adev))
> +		mmhub_v2_0_disable_identity_aperture(adev);
> +

Ditto.

>  	mmhub_v2_0_setup_vmid_config(adev);
>  	mmhub_v2_0_program_invalidation(adev);
>  
> @@ -364,7 +367,12 @@ void mmhub_v2_0_gart_disable(struct amdgpu_device *adev)
>   */
>  void mmhub_v2_0_set_fault_enable_default(struct amdgpu_device *adev, bool value)
>  {
> +	/*These regs are not accessible for VF, PF will program in SRIOV */

You can duplicate this comment from the one above.

> +	if (amdgpu_sriov_vf(adev))
> +		return;
> +
>  	u32 tmp;

Should be the first thing in the function body, as noted above.

Regards,
Luben

> +
>  	tmp = RREG32_SOC15(MMHUB, 0, mmMMVM_L2_PROTECTION_FAULT_CNTL);
>  	tmp = REG_SET_FIELD(tmp, MMVM_L2_PROTECTION_FAULT_CNTL,
>  			    RANGE_PROTECTION_FAULT_ENABLE_DEFAULT, value);
>