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@xxxxxxx>
> ---
> 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);
>
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
