diff mbox series

[14/15] drm/amdgpu/ring: move debugfs init into core amdgpu debugfs

Message ID 20200207195058.2354-15-alexander.deucher@amd.com (mailing list archive)
State New, archived
Headers show
Series amdgpu: remove load and unload callbacks (v3) | expand

Commit Message

Alex Deucher Feb. 7, 2020, 7:50 p.m. UTC 
In order to remove the load and unload drm callbacks,
we need to reorder the init sequence to move all the drm
debugfs file handling.  Do this for rings.

Acked-by: Christian König <christian.koenig@amd.com>
Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 23 ++++++++++++++++++++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c    | 15 +++-----------
 drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h    |  4 ++++
 3 files changed, 29 insertions(+), 13 deletions(-)

Comments

Daniel Vetter Feb. 13, 2020, 9:54 a.m. UTC | #1 
On Fri, Feb 07, 2020 at 02:50:57PM -0500, Alex Deucher wrote:
> In order to remove the load and unload drm callbacks,
> we need to reorder the init sequence to move all the drm
> debugfs file handling.  Do this for rings.
> 
> Acked-by: Christian König <christian.koenig@amd.com>
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 23 ++++++++++++++++++++-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c    | 15 +++-----------
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h    |  4 ++++
>  3 files changed, 29 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> index df3919ef886b..7379910790c9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> @@ -1218,7 +1218,7 @@ DEFINE_SIMPLE_ATTRIBUTE(fops_ib_preempt, NULL,
>  
>  int amdgpu_debugfs_init(struct amdgpu_device *adev)
>  {
> -	int r;
> +	int r, i;
>  
>  	adev->debugfs_preempt =
>  		debugfs_create_file("amdgpu_preempt_ib", 0600,
> @@ -1268,12 +1268,33 @@ int amdgpu_debugfs_init(struct amdgpu_device *adev)
>  	}
>  #endif
>  
> +	for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
> +		struct amdgpu_ring *ring = adev->rings[i];
> +
> +		if (!ring)
> +			continue;
> +
> +		if (amdgpu_debugfs_ring_init(adev, ring)) {
> +			DRM_ERROR("Failed to register debugfs file for rings !\n");
> +		}
> +	}
> +
>  	return amdgpu_debugfs_add_files(adev, amdgpu_debugfs_list,
>  					ARRAY_SIZE(amdgpu_debugfs_list));
>  }
>  
>  void amdgpu_debugfs_fini(struct amdgpu_device *adev)

btw debugfs_fini shouldn't be needed anymore, Greg KH removed this all.
drm core removes all debugfs files recusrively for you, there should be 0
need for debugfs cleanup.

Also at least my tree doesn't even have this, where does this apply to?
-Daniel

>  {
> +	int i;
> +
> +	for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
> +		struct amdgpu_ring *ring = adev->rings[i];
> +
> +		if (!ring)
> +			continue;
> +
> +		amdgpu_debugfs_ring_fini(ring);
> +	}
>  	amdgpu_ttm_debugfs_fini(adev);
>  	debugfs_remove(adev->debugfs_preempt);
>  }
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> index e5c83e164d82..539be138260e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> @@ -48,9 +48,6 @@
>   * wptr.  The GPU then starts fetching commands and executes
>   * them until the pointers are equal again.
>   */
> -static int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
> -				    struct amdgpu_ring *ring);
> -static void amdgpu_debugfs_ring_fini(struct amdgpu_ring *ring);
>  
>  /**
>   * amdgpu_ring_alloc - allocate space on the ring buffer
> @@ -334,10 +331,6 @@ int amdgpu_ring_init(struct amdgpu_device *adev, struct amdgpu_ring *ring,
>  	for (i = 0; i < DRM_SCHED_PRIORITY_MAX; ++i)
>  		atomic_set(&ring->num_jobs[i], 0);
>  
> -	if (amdgpu_debugfs_ring_init(adev, ring)) {
> -		DRM_ERROR("Failed to register debugfs file for rings !\n");
> -	}
> -
>  	return 0;
>  }
>  
> @@ -367,8 +360,6 @@ void amdgpu_ring_fini(struct amdgpu_ring *ring)
>  			      &ring->gpu_addr,
>  			      (void **)&ring->ring);
>  
> -	amdgpu_debugfs_ring_fini(ring);
> -
>  	dma_fence_put(ring->vmid_wait);
>  	ring->vmid_wait = NULL;
>  	ring->me = 0;
> @@ -485,8 +476,8 @@ static const struct file_operations amdgpu_debugfs_ring_fops = {
>  
>  #endif
>  
> -static int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
> -				    struct amdgpu_ring *ring)
> +int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
> +			     struct amdgpu_ring *ring)
>  {
>  #if defined(CONFIG_DEBUG_FS)
>  	struct drm_minor *minor = adev->ddev->primary;
> @@ -507,7 +498,7 @@ static int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
>  	return 0;
>  }
>  
> -static void amdgpu_debugfs_ring_fini(struct amdgpu_ring *ring)
> +void amdgpu_debugfs_ring_fini(struct amdgpu_ring *ring)
>  {
>  #if defined(CONFIG_DEBUG_FS)
>  	debugfs_remove(ring->ent);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> index 5134d0dd6dc2..0d098dafd23c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> @@ -329,4 +329,8 @@ static inline void amdgpu_ring_write_multiple(struct amdgpu_ring *ring,
>  
>  int amdgpu_ring_test_helper(struct amdgpu_ring *ring);
>  
> +int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
> +			     struct amdgpu_ring *ring);
> +void amdgpu_debugfs_ring_fini(struct amdgpu_ring *ring);
> +
>  #endif
> -- 
> 2.24.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Christian König Feb. 13, 2020, 10:17 a.m. UTC | #2 
Am 13.02.20 um 10:54 schrieb Daniel Vetter:
> On Fri, Feb 07, 2020 at 02:50:57PM -0500, Alex Deucher wrote:
>> In order to remove the load and unload drm callbacks,
>> we need to reorder the init sequence to move all the drm
>> debugfs file handling.  Do this for rings.
>>
>> Acked-by: Christian König <christian.koenig@amd.com>
>> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 23 ++++++++++++++++++++-
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c    | 15 +++-----------
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h    |  4 ++++
>>   3 files changed, 29 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>> index df3919ef886b..7379910790c9 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>> @@ -1218,7 +1218,7 @@ DEFINE_SIMPLE_ATTRIBUTE(fops_ib_preempt, NULL,
>>   
>>   int amdgpu_debugfs_init(struct amdgpu_device *adev)
>>   {
>> -	int r;
>> +	int r, i;
>>   
>>   	adev->debugfs_preempt =
>>   		debugfs_create_file("amdgpu_preempt_ib", 0600,
>> @@ -1268,12 +1268,33 @@ int amdgpu_debugfs_init(struct amdgpu_device *adev)
>>   	}
>>   #endif
>>   
>> +	for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
>> +		struct amdgpu_ring *ring = adev->rings[i];
>> +
>> +		if (!ring)
>> +			continue;
>> +
>> +		if (amdgpu_debugfs_ring_init(adev, ring)) {
>> +			DRM_ERROR("Failed to register debugfs file for rings !\n");
>> +		}
>> +	}
>> +
>>   	return amdgpu_debugfs_add_files(adev, amdgpu_debugfs_list,
>>   					ARRAY_SIZE(amdgpu_debugfs_list));
>>   }
>>   
>>   void amdgpu_debugfs_fini(struct amdgpu_device *adev)
> btw debugfs_fini shouldn't be needed anymore, Greg KH removed this all.
> drm core removes all debugfs files recusrively for you, there should be 0
> need for debugfs cleanup.

Oh, yes please. Removing that was on my TODO list for an eternity as well.

>
> Also at least my tree doesn't even have this, where does this apply to?

I would guess amd-staging-drm-next, but it might be that Alex is waiting 
for the next rebase to land.

Christian.

> -Daniel
>
>>   {
>> +	int i;
>> +
>> +	for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
>> +		struct amdgpu_ring *ring = adev->rings[i];
>> +
>> +		if (!ring)
>> +			continue;
>> +
>> +		amdgpu_debugfs_ring_fini(ring);
>> +	}
>>   	amdgpu_ttm_debugfs_fini(adev);
>>   	debugfs_remove(adev->debugfs_preempt);
>>   }
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
>> index e5c83e164d82..539be138260e 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
>> @@ -48,9 +48,6 @@
>>    * wptr.  The GPU then starts fetching commands and executes
>>    * them until the pointers are equal again.
>>    */
>> -static int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
>> -				    struct amdgpu_ring *ring);
>> -static void amdgpu_debugfs_ring_fini(struct amdgpu_ring *ring);
>>   
>>   /**
>>    * amdgpu_ring_alloc - allocate space on the ring buffer
>> @@ -334,10 +331,6 @@ int amdgpu_ring_init(struct amdgpu_device *adev, struct amdgpu_ring *ring,
>>   	for (i = 0; i < DRM_SCHED_PRIORITY_MAX; ++i)
>>   		atomic_set(&ring->num_jobs[i], 0);
>>   
>> -	if (amdgpu_debugfs_ring_init(adev, ring)) {
>> -		DRM_ERROR("Failed to register debugfs file for rings !\n");
>> -	}
>> -
>>   	return 0;
>>   }
>>   
>> @@ -367,8 +360,6 @@ void amdgpu_ring_fini(struct amdgpu_ring *ring)
>>   			      &ring->gpu_addr,
>>   			      (void **)&ring->ring);
>>   
>> -	amdgpu_debugfs_ring_fini(ring);
>> -
>>   	dma_fence_put(ring->vmid_wait);
>>   	ring->vmid_wait = NULL;
>>   	ring->me = 0;
>> @@ -485,8 +476,8 @@ static const struct file_operations amdgpu_debugfs_ring_fops = {
>>   
>>   #endif
>>   
>> -static int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
>> -				    struct amdgpu_ring *ring)
>> +int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
>> +			     struct amdgpu_ring *ring)
>>   {
>>   #if defined(CONFIG_DEBUG_FS)
>>   	struct drm_minor *minor = adev->ddev->primary;
>> @@ -507,7 +498,7 @@ static int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
>>   	return 0;
>>   }
>>   
>> -static void amdgpu_debugfs_ring_fini(struct amdgpu_ring *ring)
>> +void amdgpu_debugfs_ring_fini(struct amdgpu_ring *ring)
>>   {
>>   #if defined(CONFIG_DEBUG_FS)
>>   	debugfs_remove(ring->ent);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>> index 5134d0dd6dc2..0d098dafd23c 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>> @@ -329,4 +329,8 @@ static inline void amdgpu_ring_write_multiple(struct amdgpu_ring *ring,
>>   
>>   int amdgpu_ring_test_helper(struct amdgpu_ring *ring);
>>   
>> +int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
>> +			     struct amdgpu_ring *ring);
>> +void amdgpu_debugfs_ring_fini(struct amdgpu_ring *ring);
>> +
>>   #endif
>> -- 
>> 2.24.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C0fa2f611ed814861bf6908d7b06abf6a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637171844839285121&amp;sdata=ey6I%2B8fjxpNeFT%2ByFEl3rNubbG4S%2F5hdSkRdX8%2BPJLk%3D&amp;reserved=0
Alex Deucher Feb. 13, 2020, 2:32 p.m. UTC | #3 
On Thu, Feb 13, 2020 at 5:17 AM Christian König
<christian.koenig@amd.com> wrote:
>
> Am 13.02.20 um 10:54 schrieb Daniel Vetter:
> > On Fri, Feb 07, 2020 at 02:50:57PM -0500, Alex Deucher wrote:
> >> In order to remove the load and unload drm callbacks,
> >> we need to reorder the init sequence to move all the drm
> >> debugfs file handling.  Do this for rings.
> >>
> >> Acked-by: Christian König <christian.koenig@amd.com>
> >> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> >> ---
> >>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 23 ++++++++++++++++++++-
> >>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c    | 15 +++-----------
> >>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h    |  4 ++++
> >>   3 files changed, 29 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> >> index df3919ef886b..7379910790c9 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> >> @@ -1218,7 +1218,7 @@ DEFINE_SIMPLE_ATTRIBUTE(fops_ib_preempt, NULL,
> >>
> >>   int amdgpu_debugfs_init(struct amdgpu_device *adev)
> >>   {
> >> -    int r;
> >> +    int r, i;
> >>
> >>      adev->debugfs_preempt =
> >>              debugfs_create_file("amdgpu_preempt_ib", 0600,
> >> @@ -1268,12 +1268,33 @@ int amdgpu_debugfs_init(struct amdgpu_device *adev)
> >>      }
> >>   #endif
> >>
> >> +    for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
> >> +            struct amdgpu_ring *ring = adev->rings[i];
> >> +
> >> +            if (!ring)
> >> +                    continue;
> >> +
> >> +            if (amdgpu_debugfs_ring_init(adev, ring)) {
> >> +                    DRM_ERROR("Failed to register debugfs file for rings !\n");
> >> +            }
> >> +    }
> >> +
> >>      return amdgpu_debugfs_add_files(adev, amdgpu_debugfs_list,
> >>                                      ARRAY_SIZE(amdgpu_debugfs_list));
> >>   }
> >>
> >>   void amdgpu_debugfs_fini(struct amdgpu_device *adev)
> > btw debugfs_fini shouldn't be needed anymore, Greg KH removed this all.
> > drm core removes all debugfs files recusrively for you, there should be 0
> > need for debugfs cleanup.
>
> Oh, yes please. Removing that was on my TODO list for an eternity as well.
>
> >
> > Also at least my tree doesn't even have this, where does this apply to?
>
> I would guess amd-staging-drm-next, but it might be that Alex is waiting
> for the next rebase to land.
>

Patches are against my drm-next branch which is based on fdo drm-next.
There are a number of files which the driver creates directly rather
than through drm.

Alex

> Christian.
>
> > -Daniel
> >
> >>   {
> >> +    int i;
> >> +
> >> +    for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
> >> +            struct amdgpu_ring *ring = adev->rings[i];
> >> +
> >> +            if (!ring)
> >> +                    continue;
> >> +
> >> +            amdgpu_debugfs_ring_fini(ring);
> >> +    }
> >>      amdgpu_ttm_debugfs_fini(adev);
> >>      debugfs_remove(adev->debugfs_preempt);
> >>   }
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> >> index e5c83e164d82..539be138260e 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> >> @@ -48,9 +48,6 @@
> >>    * wptr.  The GPU then starts fetching commands and executes
> >>    * them until the pointers are equal again.
> >>    */
> >> -static int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
> >> -                                struct amdgpu_ring *ring);
> >> -static void amdgpu_debugfs_ring_fini(struct amdgpu_ring *ring);
> >>
> >>   /**
> >>    * amdgpu_ring_alloc - allocate space on the ring buffer
> >> @@ -334,10 +331,6 @@ int amdgpu_ring_init(struct amdgpu_device *adev, struct amdgpu_ring *ring,
> >>      for (i = 0; i < DRM_SCHED_PRIORITY_MAX; ++i)
> >>              atomic_set(&ring->num_jobs[i], 0);
> >>
> >> -    if (amdgpu_debugfs_ring_init(adev, ring)) {
> >> -            DRM_ERROR("Failed to register debugfs file for rings !\n");
> >> -    }
> >> -
> >>      return 0;
> >>   }
> >>
> >> @@ -367,8 +360,6 @@ void amdgpu_ring_fini(struct amdgpu_ring *ring)
> >>                            &ring->gpu_addr,
> >>                            (void **)&ring->ring);
> >>
> >> -    amdgpu_debugfs_ring_fini(ring);
> >> -
> >>      dma_fence_put(ring->vmid_wait);
> >>      ring->vmid_wait = NULL;
> >>      ring->me = 0;
> >> @@ -485,8 +476,8 @@ static const struct file_operations amdgpu_debugfs_ring_fops = {
> >>
> >>   #endif
> >>
> >> -static int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
> >> -                                struct amdgpu_ring *ring)
> >> +int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
> >> +                         struct amdgpu_ring *ring)
> >>   {
> >>   #if defined(CONFIG_DEBUG_FS)
> >>      struct drm_minor *minor = adev->ddev->primary;
> >> @@ -507,7 +498,7 @@ static int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
> >>      return 0;
> >>   }
> >>
> >> -static void amdgpu_debugfs_ring_fini(struct amdgpu_ring *ring)
> >> +void amdgpu_debugfs_ring_fini(struct amdgpu_ring *ring)
> >>   {
> >>   #if defined(CONFIG_DEBUG_FS)
> >>      debugfs_remove(ring->ent);
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> >> index 5134d0dd6dc2..0d098dafd23c 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> >> @@ -329,4 +329,8 @@ static inline void amdgpu_ring_write_multiple(struct amdgpu_ring *ring,
> >>
> >>   int amdgpu_ring_test_helper(struct amdgpu_ring *ring);
> >>
> >> +int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
> >> +                         struct amdgpu_ring *ring);
> >> +void amdgpu_debugfs_ring_fini(struct amdgpu_ring *ring);
> >> +
> >>   #endif
> >> --
> >> 2.24.1
> >>
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel@lists.freedesktop.org
> >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C0fa2f611ed814861bf6908d7b06abf6a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637171844839285121&amp;sdata=ey6I%2B8fjxpNeFT%2ByFEl3rNubbG4S%2F5hdSkRdX8%2BPJLk%3D&amp;reserved=0
>
Christian König Feb. 13, 2020, 5:28 p.m. UTC | #4 
Am 13.02.20 um 15:32 schrieb Alex Deucher:
> On Thu, Feb 13, 2020 at 5:17 AM Christian König
> <christian.koenig@amd.com> wrote:
>> Am 13.02.20 um 10:54 schrieb Daniel Vetter:
>>> On Fri, Feb 07, 2020 at 02:50:57PM -0500, Alex Deucher wrote:
>>>> In order to remove the load and unload drm callbacks,
>>>> we need to reorder the init sequence to move all the drm
>>>> debugfs file handling.  Do this for rings.
>>>>
>>>> Acked-by: Christian König <christian.koenig@amd.com>
>>>> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
>>>> ---
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 23 ++++++++++++++++++++-
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c    | 15 +++-----------
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h    |  4 ++++
>>>>    3 files changed, 29 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>> index df3919ef886b..7379910790c9 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>> @@ -1218,7 +1218,7 @@ DEFINE_SIMPLE_ATTRIBUTE(fops_ib_preempt, NULL,
>>>>
>>>>    int amdgpu_debugfs_init(struct amdgpu_device *adev)
>>>>    {
>>>> -    int r;
>>>> +    int r, i;
>>>>
>>>>       adev->debugfs_preempt =
>>>>               debugfs_create_file("amdgpu_preempt_ib", 0600,
>>>> @@ -1268,12 +1268,33 @@ int amdgpu_debugfs_init(struct amdgpu_device *adev)
>>>>       }
>>>>    #endif
>>>>
>>>> +    for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
>>>> +            struct amdgpu_ring *ring = adev->rings[i];
>>>> +
>>>> +            if (!ring)
>>>> +                    continue;
>>>> +
>>>> +            if (amdgpu_debugfs_ring_init(adev, ring)) {
>>>> +                    DRM_ERROR("Failed to register debugfs file for rings !\n");
>>>> +            }
>>>> +    }
>>>> +
>>>>       return amdgpu_debugfs_add_files(adev, amdgpu_debugfs_list,
>>>>                                       ARRAY_SIZE(amdgpu_debugfs_list));
>>>>    }
>>>>
>>>>    void amdgpu_debugfs_fini(struct amdgpu_device *adev)
>>> btw debugfs_fini shouldn't be needed anymore, Greg KH removed this all.
>>> drm core removes all debugfs files recusrively for you, there should be 0
>>> need for debugfs cleanup.
>> Oh, yes please. Removing that was on my TODO list for an eternity as well.
>>
>>> Also at least my tree doesn't even have this, where does this apply to?
>> I would guess amd-staging-drm-next, but it might be that Alex is waiting
>> for the next rebase to land.
>>
> Patches are against my drm-next branch which is based on fdo drm-next.
> There are a number of files which the driver creates directly rather
> than through drm.

The last time I locked it I was about to completely nuke creating 
anything through DRM and just create it directly.

As Daniel wrote we also don't have to remove anything explicitly, that 
is done implicitly when the whole directory is removed.

Christian.

>
> Alex
>
>> Christian.
>>
>>> -Daniel
>>>
>>>>    {
>>>> +    int i;
>>>> +
>>>> +    for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
>>>> +            struct amdgpu_ring *ring = adev->rings[i];
>>>> +
>>>> +            if (!ring)
>>>> +                    continue;
>>>> +
>>>> +            amdgpu_debugfs_ring_fini(ring);
>>>> +    }
>>>>       amdgpu_ttm_debugfs_fini(adev);
>>>>       debugfs_remove(adev->debugfs_preempt);
>>>>    }
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
>>>> index e5c83e164d82..539be138260e 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
>>>> @@ -48,9 +48,6 @@
>>>>     * wptr.  The GPU then starts fetching commands and executes
>>>>     * them until the pointers are equal again.
>>>>     */
>>>> -static int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
>>>> -                                struct amdgpu_ring *ring);
>>>> -static void amdgpu_debugfs_ring_fini(struct amdgpu_ring *ring);
>>>>
>>>>    /**
>>>>     * amdgpu_ring_alloc - allocate space on the ring buffer
>>>> @@ -334,10 +331,6 @@ int amdgpu_ring_init(struct amdgpu_device *adev, struct amdgpu_ring *ring,
>>>>       for (i = 0; i < DRM_SCHED_PRIORITY_MAX; ++i)
>>>>               atomic_set(&ring->num_jobs[i], 0);
>>>>
>>>> -    if (amdgpu_debugfs_ring_init(adev, ring)) {
>>>> -            DRM_ERROR("Failed to register debugfs file for rings !\n");
>>>> -    }
>>>> -
>>>>       return 0;
>>>>    }
>>>>
>>>> @@ -367,8 +360,6 @@ void amdgpu_ring_fini(struct amdgpu_ring *ring)
>>>>                             &ring->gpu_addr,
>>>>                             (void **)&ring->ring);
>>>>
>>>> -    amdgpu_debugfs_ring_fini(ring);
>>>> -
>>>>       dma_fence_put(ring->vmid_wait);
>>>>       ring->vmid_wait = NULL;
>>>>       ring->me = 0;
>>>> @@ -485,8 +476,8 @@ static const struct file_operations amdgpu_debugfs_ring_fops = {
>>>>
>>>>    #endif
>>>>
>>>> -static int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
>>>> -                                struct amdgpu_ring *ring)
>>>> +int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
>>>> +                         struct amdgpu_ring *ring)
>>>>    {
>>>>    #if defined(CONFIG_DEBUG_FS)
>>>>       struct drm_minor *minor = adev->ddev->primary;
>>>> @@ -507,7 +498,7 @@ static int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
>>>>       return 0;
>>>>    }
>>>>
>>>> -static void amdgpu_debugfs_ring_fini(struct amdgpu_ring *ring)
>>>> +void amdgpu_debugfs_ring_fini(struct amdgpu_ring *ring)
>>>>    {
>>>>    #if defined(CONFIG_DEBUG_FS)
>>>>       debugfs_remove(ring->ent);
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>>>> index 5134d0dd6dc2..0d098dafd23c 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>>>> @@ -329,4 +329,8 @@ static inline void amdgpu_ring_write_multiple(struct amdgpu_ring *ring,
>>>>
>>>>    int amdgpu_ring_test_helper(struct amdgpu_ring *ring);
>>>>
>>>> +int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
>>>> +                         struct amdgpu_ring *ring);
>>>> +void amdgpu_debugfs_ring_fini(struct amdgpu_ring *ring);
>>>> +
>>>>    #endif
>>>> --
>>>> 2.24.1
>>>>
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel@lists.freedesktop.org
>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C0647e5ecad344fb4d19208d7b09198d6%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637172011693912378&amp;sdata=dBwdfJW2nPzwPZJYklNexgu0NWT3vGSOgbO79SLQ%2B74%3D&amp;reserved=0
Alex Deucher Feb. 13, 2020, 5:32 p.m. UTC | #5 
On Thu, Feb 13, 2020 at 12:28 PM Christian König
<christian.koenig@amd.com> wrote:
>
> Am 13.02.20 um 15:32 schrieb Alex Deucher:
> > On Thu, Feb 13, 2020 at 5:17 AM Christian König
> > <christian.koenig@amd.com> wrote:
> >> Am 13.02.20 um 10:54 schrieb Daniel Vetter:
> >>> On Fri, Feb 07, 2020 at 02:50:57PM -0500, Alex Deucher wrote:
> >>>> In order to remove the load and unload drm callbacks,
> >>>> we need to reorder the init sequence to move all the drm
> >>>> debugfs file handling.  Do this for rings.
> >>>>
> >>>> Acked-by: Christian König <christian.koenig@amd.com>
> >>>> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> >>>> ---
> >>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 23 ++++++++++++++++++++-
> >>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c    | 15 +++-----------
> >>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h    |  4 ++++
> >>>>    3 files changed, 29 insertions(+), 13 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> >>>> index df3919ef886b..7379910790c9 100644
> >>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> >>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> >>>> @@ -1218,7 +1218,7 @@ DEFINE_SIMPLE_ATTRIBUTE(fops_ib_preempt, NULL,
> >>>>
> >>>>    int amdgpu_debugfs_init(struct amdgpu_device *adev)
> >>>>    {
> >>>> -    int r;
> >>>> +    int r, i;
> >>>>
> >>>>       adev->debugfs_preempt =
> >>>>               debugfs_create_file("amdgpu_preempt_ib", 0600,
> >>>> @@ -1268,12 +1268,33 @@ int amdgpu_debugfs_init(struct amdgpu_device *adev)
> >>>>       }
> >>>>    #endif
> >>>>
> >>>> +    for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
> >>>> +            struct amdgpu_ring *ring = adev->rings[i];
> >>>> +
> >>>> +            if (!ring)
> >>>> +                    continue;
> >>>> +
> >>>> +            if (amdgpu_debugfs_ring_init(adev, ring)) {
> >>>> +                    DRM_ERROR("Failed to register debugfs file for rings !\n");
> >>>> +            }
> >>>> +    }
> >>>> +
> >>>>       return amdgpu_debugfs_add_files(adev, amdgpu_debugfs_list,
> >>>>                                       ARRAY_SIZE(amdgpu_debugfs_list));
> >>>>    }
> >>>>
> >>>>    void amdgpu_debugfs_fini(struct amdgpu_device *adev)
> >>> btw debugfs_fini shouldn't be needed anymore, Greg KH removed this all.
> >>> drm core removes all debugfs files recusrively for you, there should be 0
> >>> need for debugfs cleanup.
> >> Oh, yes please. Removing that was on my TODO list for an eternity as well.
> >>
> >>> Also at least my tree doesn't even have this, where does this apply to?
> >> I would guess amd-staging-drm-next, but it might be that Alex is waiting
> >> for the next rebase to land.
> >>
> > Patches are against my drm-next branch which is based on fdo drm-next.
> > There are a number of files which the driver creates directly rather
> > than through drm.
>
> The last time I locked it I was about to completely nuke creating
> anything through DRM and just create it directly.
>
> As Daniel wrote we also don't have to remove anything explicitly, that
> is done implicitly when the whole directory is removed.

I dunno.  Most of this stuff has been there for years.  Maybe someone
wants to take a look if it can be further cleaned up.  It builds fine
against current kernels.

Alex


>
> Christian.
>
> >
> > Alex
> >
> >> Christian.
> >>
> >>> -Daniel
> >>>
> >>>>    {
> >>>> +    int i;
> >>>> +
> >>>> +    for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
> >>>> +            struct amdgpu_ring *ring = adev->rings[i];
> >>>> +
> >>>> +            if (!ring)
> >>>> +                    continue;
> >>>> +
> >>>> +            amdgpu_debugfs_ring_fini(ring);
> >>>> +    }
> >>>>       amdgpu_ttm_debugfs_fini(adev);
> >>>>       debugfs_remove(adev->debugfs_preempt);
> >>>>    }
> >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> >>>> index e5c83e164d82..539be138260e 100644
> >>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> >>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> >>>> @@ -48,9 +48,6 @@
> >>>>     * wptr.  The GPU then starts fetching commands and executes
> >>>>     * them until the pointers are equal again.
> >>>>     */
> >>>> -static int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
> >>>> -                                struct amdgpu_ring *ring);
> >>>> -static void amdgpu_debugfs_ring_fini(struct amdgpu_ring *ring);
> >>>>
> >>>>    /**
> >>>>     * amdgpu_ring_alloc - allocate space on the ring buffer
> >>>> @@ -334,10 +331,6 @@ int amdgpu_ring_init(struct amdgpu_device *adev, struct amdgpu_ring *ring,
> >>>>       for (i = 0; i < DRM_SCHED_PRIORITY_MAX; ++i)
> >>>>               atomic_set(&ring->num_jobs[i], 0);
> >>>>
> >>>> -    if (amdgpu_debugfs_ring_init(adev, ring)) {
> >>>> -            DRM_ERROR("Failed to register debugfs file for rings !\n");
> >>>> -    }
> >>>> -
> >>>>       return 0;
> >>>>    }
> >>>>
> >>>> @@ -367,8 +360,6 @@ void amdgpu_ring_fini(struct amdgpu_ring *ring)
> >>>>                             &ring->gpu_addr,
> >>>>                             (void **)&ring->ring);
> >>>>
> >>>> -    amdgpu_debugfs_ring_fini(ring);
> >>>> -
> >>>>       dma_fence_put(ring->vmid_wait);
> >>>>       ring->vmid_wait = NULL;
> >>>>       ring->me = 0;
> >>>> @@ -485,8 +476,8 @@ static const struct file_operations amdgpu_debugfs_ring_fops = {
> >>>>
> >>>>    #endif
> >>>>
> >>>> -static int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
> >>>> -                                struct amdgpu_ring *ring)
> >>>> +int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
> >>>> +                         struct amdgpu_ring *ring)
> >>>>    {
> >>>>    #if defined(CONFIG_DEBUG_FS)
> >>>>       struct drm_minor *minor = adev->ddev->primary;
> >>>> @@ -507,7 +498,7 @@ static int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
> >>>>       return 0;
> >>>>    }
> >>>>
> >>>> -static void amdgpu_debugfs_ring_fini(struct amdgpu_ring *ring)
> >>>> +void amdgpu_debugfs_ring_fini(struct amdgpu_ring *ring)
> >>>>    {
> >>>>    #if defined(CONFIG_DEBUG_FS)
> >>>>       debugfs_remove(ring->ent);
> >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> >>>> index 5134d0dd6dc2..0d098dafd23c 100644
> >>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> >>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> >>>> @@ -329,4 +329,8 @@ static inline void amdgpu_ring_write_multiple(struct amdgpu_ring *ring,
> >>>>
> >>>>    int amdgpu_ring_test_helper(struct amdgpu_ring *ring);
> >>>>
> >>>> +int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
> >>>> +                         struct amdgpu_ring *ring);
> >>>> +void amdgpu_debugfs_ring_fini(struct amdgpu_ring *ring);
> >>>> +
> >>>>    #endif
> >>>> --
> >>>> 2.24.1
> >>>>
> >>>> _______________________________________________
> >>>> dri-devel mailing list
> >>>> dri-devel@lists.freedesktop.org
> >>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C0647e5ecad344fb4d19208d7b09198d6%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637172011693912378&amp;sdata=dBwdfJW2nPzwPZJYklNexgu0NWT3vGSOgbO79SLQ%2B74%3D&amp;reserved=0
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
index df3919ef886b..7379910790c9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
@@ -1218,7 +1218,7 @@  DEFINE_SIMPLE_ATTRIBUTE(fops_ib_preempt, NULL,
 
 int amdgpu_debugfs_init(struct amdgpu_device *adev)
 {
-	int r;
+	int r, i;
 
 	adev->debugfs_preempt =
 		debugfs_create_file("amdgpu_preempt_ib", 0600,
@@ -1268,12 +1268,33 @@  int amdgpu_debugfs_init(struct amdgpu_device *adev)
 	}
 #endif
 
+	for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
+		struct amdgpu_ring *ring = adev->rings[i];
+
+		if (!ring)
+			continue;
+
+		if (amdgpu_debugfs_ring_init(adev, ring)) {
+			DRM_ERROR("Failed to register debugfs file for rings !\n");
+		}
+	}
+
 	return amdgpu_debugfs_add_files(adev, amdgpu_debugfs_list,
 					ARRAY_SIZE(amdgpu_debugfs_list));
 }
 
 void amdgpu_debugfs_fini(struct amdgpu_device *adev)
 {
+	int i;
+
+	for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
+		struct amdgpu_ring *ring = adev->rings[i];
+
+		if (!ring)
+			continue;
+
+		amdgpu_debugfs_ring_fini(ring);
+	}
 	amdgpu_ttm_debugfs_fini(adev);
 	debugfs_remove(adev->debugfs_preempt);
 }
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
index e5c83e164d82..539be138260e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
@@ -48,9 +48,6 @@ 
  * wptr.  The GPU then starts fetching commands and executes
  * them until the pointers are equal again.
  */
-static int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
-				    struct amdgpu_ring *ring);
-static void amdgpu_debugfs_ring_fini(struct amdgpu_ring *ring);
 
 /**
  * amdgpu_ring_alloc - allocate space on the ring buffer
@@ -334,10 +331,6 @@  int amdgpu_ring_init(struct amdgpu_device *adev, struct amdgpu_ring *ring,
 	for (i = 0; i < DRM_SCHED_PRIORITY_MAX; ++i)
 		atomic_set(&ring->num_jobs[i], 0);
 
-	if (amdgpu_debugfs_ring_init(adev, ring)) {
-		DRM_ERROR("Failed to register debugfs file for rings !\n");
-	}
-
 	return 0;
 }
 
@@ -367,8 +360,6 @@  void amdgpu_ring_fini(struct amdgpu_ring *ring)
 			      &ring->gpu_addr,
 			      (void **)&ring->ring);
 
-	amdgpu_debugfs_ring_fini(ring);
-
 	dma_fence_put(ring->vmid_wait);
 	ring->vmid_wait = NULL;
 	ring->me = 0;
@@ -485,8 +476,8 @@  static const struct file_operations amdgpu_debugfs_ring_fops = {
 
 #endif
 
-static int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
-				    struct amdgpu_ring *ring)
+int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
+			     struct amdgpu_ring *ring)
 {
 #if defined(CONFIG_DEBUG_FS)
 	struct drm_minor *minor = adev->ddev->primary;
@@ -507,7 +498,7 @@  static int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
 	return 0;
 }
 
-static void amdgpu_debugfs_ring_fini(struct amdgpu_ring *ring)
+void amdgpu_debugfs_ring_fini(struct amdgpu_ring *ring)
 {
 #if defined(CONFIG_DEBUG_FS)
 	debugfs_remove(ring->ent);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
index 5134d0dd6dc2..0d098dafd23c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
@@ -329,4 +329,8 @@  static inline void amdgpu_ring_write_multiple(struct amdgpu_ring *ring,
 
 int amdgpu_ring_test_helper(struct amdgpu_ring *ring);
 
+int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
+			     struct amdgpu_ring *ring);
+void amdgpu_debugfs_ring_fini(struct amdgpu_ring *ring);
+
 #endif