Re: [PATCH] drm/amdgpu: Move SR-IOV check into amdgpu_gfx_sysfs_compute_init

[Christian König <christian.koenig@xxxxxxx>]

Am 06.06.24 um 19:28 schrieb Lazar, Lijo:
> On 6/6/2024 5:35 PM, Srinivasan Shanmugam wrote:
> > Previously, this check was performed in the gfx_v9_4_3_sw_init function,
> > and the amdgpu_gfx_sysfs_compute_init function was only called if the
> > GPU was not a VF in SR-IOV mode. This is because the sysfs entries
> > created by amdgpu_gfx_sysfs_compute_init are specific to compute
> > partitions, which are only applicable on GFX9 and not on a VF in SR-IOV
> > mode.
> >
> > By moving the check into amdgpu_gfx_sysfs_compute_init, we make this
> > function responsible for deciding whether or not to create the compute
> > partition sysfs entries.
> >
> > This change improves the code organization and maintainability. If in
> > the future the  conditions for creating the compute partition sysfs
> > entries change, we  would only need to update the
> > amdgpu_gfx_sysfs_compute_init function.
> This is not correct. If this has to be true, this will reside somewhere
> in amdgpu_gfx and you would also need IP version inside this one. If for
> a new IP version say gfx v9.4.5 this needs to be created for VF also,
> then this check here won't work. This is the specific reason why we put
> the conditions inside IP code.

Yeah, but that doesn't make sense. See the partitioning mode is 
something which is fundamentally incompatible with SRIOV.

So this is not IP version specific at all.

Regards,
Christian.

> Thanks,
> Lijo
>
> > Cc: Alex Deucher <alexander.deucher@xxxxxxx>
> > Cc: Christian König <christian.koenig@xxxxxxx>
> > Suggested-by: Christian König <christian.koenig@xxxxxxx>
> > Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@xxxxxxx>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | 24 +++++++++++++++---------
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h |  4 ++--
> >   drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.c | 11 +++++------
> >   3 files changed, 22 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> > index 19b1817b55d7..72477a5aedca 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> > @@ -1376,21 +1376,27 @@ static DEVICE_ATTR(current_compute_partition, 0644,
> >   static DEVICE_ATTR(available_compute_partition, 0444,
> >   		   amdgpu_gfx_get_available_compute_partition, NULL);
> >   
> > -int amdgpu_gfx_sysfs_init(struct amdgpu_device *adev)
> > +int amdgpu_gfx_sysfs_compute_init(struct amdgpu_device *adev)
> >   {
> >   	int r;
> >   
> > -	r = device_create_file(adev->dev, &dev_attr_current_compute_partition);
> > -	if (r)
> > -		return r;
> > +	if (!amdgpu_sriov_vf(adev)) {
> > +		r = device_create_file(adev->dev, &dev_attr_current_compute_partition);
> > +		if (r)
> > +			return r;
> >   
> > -	r = device_create_file(adev->dev, &dev_attr_available_compute_partition);
> > +		r = device_create_file(adev->dev, &dev_attr_available_compute_partition);
> > +		if (r)
> > +			return r;
> > +	}
> >   
> > -	return r;
> > +	return 0;
> >   }
> >   
> > -void amdgpu_gfx_sysfs_fini(struct amdgpu_device *adev)
> > +void amdgpu_gfx_sysfs_compute_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);
> > +	if (!amdgpu_sriov_vf(adev)) {
> > +		device_remove_file(adev->dev, &dev_attr_current_compute_partition);
> > +		device_remove_file(adev->dev, &dev_attr_available_compute_partition);
> > +	}
> >   }
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
> > index 6b0416777c5b..b65c459b3aa9 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
> > @@ -533,8 +533,8 @@ int amdgpu_gfx_poison_consumption_handler(struct amdgpu_device *adev,
> >   						struct amdgpu_iv_entry *entry);
> >   
> >   bool amdgpu_gfx_is_master_xcc(struct amdgpu_device *adev, int xcc_id);
> > -int amdgpu_gfx_sysfs_init(struct amdgpu_device *adev);
> > -void amdgpu_gfx_sysfs_fini(struct amdgpu_device *adev);
> > +int amdgpu_gfx_sysfs_compute_init(struct amdgpu_device *adev);
> > +void amdgpu_gfx_sysfs_compute_fini(struct amdgpu_device *adev);
> >   void amdgpu_gfx_ras_error_func(struct amdgpu_device *adev,
> >   		void *ras_error_status,
> >   		void (*func)(struct amdgpu_device *adev, void *ras_error_status,
> > 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 aecc2bcea145..07ce614ef282 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.c
> > @@ -939,11 +939,11 @@ static int gfx_v9_4_3_sw_init(void *handle)
> >   	if (r)
> >   		return r;
> >   
> > +	r = amdgpu_gfx_sysfs_compute_init(adev);
> > +	if (r)
> > +		return r;
> >   
> > -	if (!amdgpu_sriov_vf(adev))
> > -		r = amdgpu_gfx_sysfs_init(adev);
> > -
> > -	return r;
> > +	return 0;
> >   }
> >   
> >   static int gfx_v9_4_3_sw_fini(void *handle)
> > @@ -964,8 +964,7 @@ static int gfx_v9_4_3_sw_fini(void *handle)
> >   	gfx_v9_4_3_mec_fini(adev);
> >   	amdgpu_bo_unref(&adev->gfx.rlc.clear_state_obj);
> >   	gfx_v9_4_3_free_microcode(adev);
> > -	if (!amdgpu_sriov_vf(adev))
> > -		amdgpu_gfx_sysfs_fini(adev);
> > +	amdgpu_gfx_sysfs_compute_fini(adev);
> >   
> >   	return 0;
> >   }