On Tue, Oct 29, 2024 at 2:58 AM SRINIVASAN SHANMUGAM <[email protected]> wrote: > > > On 10/29/2024 12:07 PM, SRINIVASAN SHANMUGAM wrote: > > > On 10/29/2024 10:57 AM, Lijo Lazar wrote: > > Make amdgpu_gfx_sysfs_init/fini functions as common entry points for all > gfx related sysfs nodes. > > Signed-off-by: Lijo Lazar <[email protected]> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | 37 ++++++++++++++++++++++--- > drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h | 2 -- > drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c | 5 ++-- > drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c | 4 +-- > drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c | 4 +-- > drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c | 4 +-- > drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.c | 5 ---- > 7 files changed, 42 insertions(+), 19 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c > index e96984c53e72..86a6fd3015c2 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c > @@ -1602,7 +1602,7 @@ 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) > +static int amdgpu_gfx_sysfs_xcp_init(struct amdgpu_device *adev) > { > struct amdgpu_xcp_mgr *xcp_mgr = adev->xcp_mgr; > bool xcp_switch_supported; > @@ -1629,7 +1629,7 @@ int amdgpu_gfx_sysfs_init(struct amdgpu_device *adev) > return r; > } > > -void amdgpu_gfx_sysfs_fini(struct amdgpu_device *adev) > +static void amdgpu_gfx_sysfs_xcp_fini(struct amdgpu_device *adev) > { > struct amdgpu_xcp_mgr *xcp_mgr = adev->xcp_mgr; > bool xcp_switch_supported; > @@ -1646,10 +1646,13 @@ void amdgpu_gfx_sysfs_fini(struct amdgpu_device *adev) > &dev_attr_available_compute_partition); > } > > -int amdgpu_gfx_sysfs_isolation_shader_init(struct amdgpu_device *adev) > +static int amdgpu_gfx_sysfs_isolation_shader_init(struct amdgpu_device *adev) > { > int r; > > + if (!adev->gfx.enable_cleaner_shader) > + return 0; > + > > This check seems to be incorrect here, because enforce_isolation and cleaner > shader are two different entities, with this check enforce_isolation node > won't be created for some of the ASIC's where we don't load cleaner shader > explictly. > > Correction, this check " > > !adev->gfx.enable_cleaner_shader" handles for ASIC's where we don't load > cleaner shader explictly, but having it here I'm not certain cz I think we > expect enforce_isolation node to be created independent of > run_cleaner_shader, would request Alex/Christian, to comment onto it further. >
We want sysfs enforce_isolation whether or not there is a cleaner shader for a chip yet or not. It's useful for serializing access to gfx. Before we added the cleaner shader stuff it was still a useful option for certain use cases. Alex > -Srini > > And moreover this grouping, its better to be done for all sysfs entires in > amdgpu ie., not only gfx, for other modules like even pm etc., so that we can > have one common sysfs amdgpu framework, improving code consistency and > maintainability > > I had initiated this https://patchwork.freedesktop.org/patch/595215/ , but I > couldn't finish it because of other work commitments. > > r = device_create_file(adev->dev, &dev_attr_enforce_isolation); > if (r) > return r; > @@ -1661,12 +1664,38 @@ int amdgpu_gfx_sysfs_isolation_shader_init(struct > amdgpu_device *adev) > return 0; > } > > -void amdgpu_gfx_sysfs_isolation_shader_fini(struct amdgpu_device *adev) > +static void amdgpu_gfx_sysfs_isolation_shader_fini(struct amdgpu_device > *adev) > { > + if (!adev->gfx.enable_cleaner_shader) > + return; > + > > Same here > > -Srini > > device_remove_file(adev->dev, &dev_attr_enforce_isolation); > device_remove_file(adev->dev, &dev_attr_run_cleaner_shader); > } > > +int amdgpu_gfx_sysfs_init(struct amdgpu_device *adev) > +{ > + int r; > + > + r = amdgpu_gfx_sysfs_xcp_init(adev); > + if (r) { > + dev_err(adev->dev, "failed to create xcp sysfs files"); > + return r; > + } > + > + r = amdgpu_gfx_sysfs_isolation_shader_init(adev); > + if (r) > + dev_err(adev->dev, "failed to create isolation sysfs files"); > + > + return r; > +} > + > +void amdgpu_gfx_sysfs_fini(struct amdgpu_device *adev) > +{ > + amdgpu_gfx_sysfs_xcp_fini(adev); > + amdgpu_gfx_sysfs_isolation_shader_fini(adev); > +} > + > int amdgpu_gfx_cleaner_shader_sw_init(struct amdgpu_device *adev, > unsigned int cleaner_shader_size) > { > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h > b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h > index f710178a21bc..b8a2f60688dc 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h > @@ -577,8 +577,6 @@ 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); > -int amdgpu_gfx_sysfs_isolation_shader_init(struct amdgpu_device *adev); > -void amdgpu_gfx_sysfs_isolation_shader_fini(struct amdgpu_device *adev); > void amdgpu_gfx_enforce_isolation_handler(struct work_struct *work); > void amdgpu_gfx_enforce_isolation_ring_begin_use(struct amdgpu_ring *ring); > void amdgpu_gfx_enforce_isolation_ring_end_use(struct amdgpu_ring *ring); > diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c > b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c > index 9da95b25e158..d1a18ca584dd 100644 > --- a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c > +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c > @@ -4853,9 +4853,10 @@ static int gfx_v10_0_sw_init(struct amdgpu_ip_block > *ip_block) > > gfx_v10_0_alloc_ip_dump(adev); > > - r = amdgpu_gfx_sysfs_isolation_shader_init(adev); > + r = amdgpu_gfx_sysfs_init(adev); > if (r) > return r; > + > return 0; > } > > @@ -4907,7 +4908,7 @@ static int gfx_v10_0_sw_fini(struct amdgpu_ip_block > *ip_block) > gfx_v10_0_rlc_backdoor_autoload_buffer_fini(adev); > > gfx_v10_0_free_microcode(adev); > - amdgpu_gfx_sysfs_isolation_shader_fini(adev); > + amdgpu_gfx_sysfs_fini(adev); > > kfree(adev->gfx.ip_dump_core); > kfree(adev->gfx.ip_dump_compute_queues); > diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c > b/drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c > index 5aff8f72de9c..22811b624532 100644 > --- a/drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c > +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c > @@ -1717,7 +1717,7 @@ static int gfx_v11_0_sw_init(struct amdgpu_ip_block > *ip_block) > > gfx_v11_0_alloc_ip_dump(adev); > > - r = amdgpu_gfx_sysfs_isolation_shader_init(adev); > + r = amdgpu_gfx_sysfs_init(adev); > if (r) > return r; > > @@ -1782,7 +1782,7 @@ static int gfx_v11_0_sw_fini(struct amdgpu_ip_block > *ip_block) > > gfx_v11_0_free_microcode(adev); > > - amdgpu_gfx_sysfs_isolation_shader_fini(adev); > + amdgpu_gfx_sysfs_fini(adev); > > kfree(adev->gfx.ip_dump_core); > kfree(adev->gfx.ip_dump_compute_queues); > diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c > b/drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c > index 9fec28d8a5fc..1b99f90cd193 100644 > --- a/drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c > +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c > @@ -1466,7 +1466,7 @@ static int gfx_v12_0_sw_init(struct amdgpu_ip_block > *ip_block) > > gfx_v12_0_alloc_ip_dump(adev); > > - r = amdgpu_gfx_sysfs_isolation_shader_init(adev); > + r = amdgpu_gfx_sysfs_init(adev); > if (r) > return r; > > @@ -1529,7 +1529,7 @@ static int gfx_v12_0_sw_fini(struct amdgpu_ip_block > *ip_block) > > gfx_v12_0_free_microcode(adev); > > - amdgpu_gfx_sysfs_isolation_shader_fini(adev); > + amdgpu_gfx_sysfs_fini(adev); > > kfree(adev->gfx.ip_dump_core); > kfree(adev->gfx.ip_dump_compute_queues); > diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c > b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c > index 66947850d7e4..987e52c47635 100644 > --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c > +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c > @@ -2402,7 +2402,7 @@ static int gfx_v9_0_sw_init(struct amdgpu_ip_block > *ip_block) > > gfx_v9_0_alloc_ip_dump(adev); > > - r = amdgpu_gfx_sysfs_isolation_shader_init(adev); > + r = amdgpu_gfx_sysfs_init(adev); > if (r) > return r; > > @@ -2443,7 +2443,7 @@ static int gfx_v9_0_sw_fini(struct amdgpu_ip_block > *ip_block) > } > gfx_v9_0_free_microcode(adev); > > - amdgpu_gfx_sysfs_isolation_shader_fini(adev); > + amdgpu_gfx_sysfs_fini(adev); > > kfree(adev->gfx.ip_dump_core); > kfree(adev->gfx.ip_dump_compute_queues); > 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 016290f00592..983088805c3a 100644 > --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.c > +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.c > @@ -1171,10 +1171,6 @@ static int gfx_v9_4_3_sw_init(struct amdgpu_ip_block > *ip_block) > > gfx_v9_4_3_alloc_ip_dump(adev); > > - r = amdgpu_gfx_sysfs_isolation_shader_init(adev); > - if (r) > - return r; > - > return 0; > } > > @@ -1199,7 +1195,6 @@ static int gfx_v9_4_3_sw_fini(struct amdgpu_ip_block > *ip_block) > amdgpu_bo_unref(&adev->gfx.rlc.clear_state_obj); > gfx_v9_4_3_free_microcode(adev); > amdgpu_gfx_sysfs_fini(adev); > - amdgpu_gfx_sysfs_isolation_shader_fini(adev); > > kfree(adev->gfx.ip_dump_core); > kfree(adev->gfx.ip_dump_compute_queues);
