Re: [PATCH 1/1] drm/amdgpu: wrap kiq ring ops with kiq spinlock

2021-03-15 Thread Nirmoy 


On 3/15/21 11:32 AM, Wang, Kevin(Yang) wrote:


[AMD Official Use Only - Internal Distribution Only]


Hi Das,

It seems more reasonable to put spin lock in ring structure, so that 
KIQ ring can be treated like other normal ring.




We don't use locks for other rings though.


and do we have other paths to call kiq ring except for kernel 
initialization? it seems the kfd and kgd will use kiq in one thread..




This patch is for consistency. It is not fixing any issue/bug.



Nirmoy



thanks.

Best Regards,
Kevin

*From:* amd-gfx  on behalf of 
Nirmoy Das 

*Sent:* Saturday, March 13, 2021 1:08 AM
*To:* Koenig, Christian 
*Cc:* Das, Nirmoy ; amd-gfx@lists.freedesktop.org 


*Subject:* [PATCH 1/1] drm/amdgpu: wrap kiq ring ops with kiq spinlock
KIQ ring is being operated by kfd as well as amdgpu.
KFD is using kiq lock, we should the same from amdgpu side
as well.

Signed-off-by: Nirmoy Das 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | 15 +++
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c

index 1915b9b95106..892dc6b269fb 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
@@ -462,20 +462,25 @@ int amdgpu_gfx_disable_kcq(struct amdgpu_device 
*adev)

 {
 struct amdgpu_kiq *kiq = &adev->gfx.kiq;
 struct amdgpu_ring *kiq_ring = &kiq->ring;
-   int i;
+   int i, r;

 if (!kiq->pmf || !kiq->pmf->kiq_unmap_queues)
 return -EINVAL;

+   spin_lock(&adev->gfx.kiq.ring_lock);
 if (amdgpu_ring_alloc(kiq_ring, kiq->pmf->unmap_queues_size *
- adev->gfx.num_compute_rings))
+ adev->gfx.num_compute_rings)) {
+ spin_unlock(&adev->gfx.kiq.ring_lock);
 return -ENOMEM;
+   }

 for (i = 0; i < adev->gfx.num_compute_rings; i++)
kiq->pmf->kiq_unmap_queues(kiq_ring, &adev->gfx.compute_ring[i],
RESET_QUEUES, 0, 0);
+   r = amdgpu_ring_test_helper(kiq_ring);
+   spin_unlock(&adev->gfx.kiq.ring_lock);

-   return amdgpu_ring_test_helper(kiq_ring);
+   return r;
 }

 int amdgpu_queue_mask_bit_to_set_resource_bit(struct amdgpu_device *adev,
@@ -518,12 +523,13 @@ int amdgpu_gfx_enable_kcq(struct amdgpu_device 
*adev)


 DRM_INFO("kiq ring mec %d pipe %d q %d\n", kiq_ring->me, 
kiq_ring->pipe,

kiq_ring->queue);
-
+   spin_lock(&adev->gfx.kiq.ring_lock);
 r = amdgpu_ring_alloc(kiq_ring, kiq->pmf->map_queues_size *
adev->gfx.num_compute_rings +
kiq->pmf->set_resources_size);
 if (r) {
 DRM_ERROR("Failed to lock KIQ (%d).\n", r);
+ spin_unlock(&adev->gfx.kiq.ring_lock);
 return r;
 }

@@ -532,6 +538,7 @@ int amdgpu_gfx_enable_kcq(struct amdgpu_device *adev)
kiq->pmf->kiq_map_queues(kiq_ring, &adev->gfx.compute_ring[i]);

 r = amdgpu_ring_test_helper(kiq_ring);
+   spin_unlock(&adev->gfx.kiq.ring_lock);
 if (r)
 DRM_ERROR("KCQ enable failed\n");

--
2.30.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=04%7C01%7CKevin1.Wang%40amd.com%7C0a90c6383d634a5226a108d8e5797c08%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637511657234189238%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=sfa27Sz8wcIyHJqaE9f8aXQMeUxbyynpAONZHUYoogY%3D&reserved=0 

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/1] drm/amdgpu: wrap kiq ring ops with kiq spinlock

2021-03-15 Thread Wang, Kevin(Yang) 
[AMD Official Use Only - Internal Distribution Only]

Hi Das,

It seems more reasonable to put spin lock in ring structure, so that KIQ ring 
can be treated like other normal ring.
and do we have other paths to call kiq ring except for kernel initialization? 
it seems the kfd and kgd will use kiq in one thread..

thanks.

Best Regards,
Kevin

From: amd-gfx  on behalf of Nirmoy Das 

Sent: Saturday, March 13, 2021 1:08 AM
To: Koenig, Christian 
Cc: Das, Nirmoy ; amd-gfx@lists.freedesktop.org 

Subject: [PATCH 1/1] drm/amdgpu: wrap kiq ring ops with kiq spinlock

KIQ ring is being operated by kfd as well as amdgpu.
KFD is using kiq lock, we should the same from amdgpu side
as well.

Signed-off-by: Nirmoy Das 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | 15 +++
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
index 1915b9b95106..892dc6b269fb 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
@@ -462,20 +462,25 @@ int amdgpu_gfx_disable_kcq(struct amdgpu_device *adev)
 {
 struct amdgpu_kiq *kiq = &adev->gfx.kiq;
 struct amdgpu_ring *kiq_ring = &kiq->ring;
-   int i;
+   int i, r;

 if (!kiq->pmf || !kiq->pmf->kiq_unmap_queues)
 return -EINVAL;

+   spin_lock(&adev->gfx.kiq.ring_lock);
 if (amdgpu_ring_alloc(kiq_ring, kiq->pmf->unmap_queues_size *
-   adev->gfx.num_compute_rings))
+   adev->gfx.num_compute_rings)) {
+   spin_unlock(&adev->gfx.kiq.ring_lock);
 return -ENOMEM;
+   }

 for (i = 0; i < adev->gfx.num_compute_rings; i++)
 kiq->pmf->kiq_unmap_queues(kiq_ring, 
&adev->gfx.compute_ring[i],
RESET_QUEUES, 0, 0);
+   r = amdgpu_ring_test_helper(kiq_ring);
+   spin_unlock(&adev->gfx.kiq.ring_lock);

-   return amdgpu_ring_test_helper(kiq_ring);
+   return r;
 }

 int amdgpu_queue_mask_bit_to_set_resource_bit(struct amdgpu_device *adev,
@@ -518,12 +523,13 @@ int amdgpu_gfx_enable_kcq(struct amdgpu_device *adev)

 DRM_INFO("kiq ring mec %d pipe %d q %d\n", kiq_ring->me, 
kiq_ring->pipe,
 kiq_ring->queue);
-
+   spin_lock(&adev->gfx.kiq.ring_lock);
 r = amdgpu_ring_alloc(kiq_ring, kiq->pmf->map_queues_size *
 adev->gfx.num_compute_rings +
 kiq->pmf->set_resources_size);
 if (r) {
 DRM_ERROR("Failed to lock KIQ (%d).\n", r);
+   spin_unlock(&adev->gfx.kiq.ring_lock);
 return r;
 }

@@ -532,6 +538,7 @@ int amdgpu_gfx_enable_kcq(struct amdgpu_device *adev)
 kiq->pmf->kiq_map_queues(kiq_ring, &adev->gfx.compute_ring[i]);

 r = amdgpu_ring_test_helper(kiq_ring);
+   spin_unlock(&adev->gfx.kiq.ring_lock);
 if (r)
 DRM_ERROR("KCQ enable failed\n");

--
2.30.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=04%7C01%7CKevin1.Wang%40amd.com%7C0a90c6383d634a5226a108d8e5797c08%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637511657234189238%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=sfa27Sz8wcIyHJqaE9f8aXQMeUxbyynpAONZHUYoogY%3D&reserved=0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/1] drm/amdgpu: wrap kiq ring ops with kiq spinlock

2021-03-15 Thread Christian König 

Am 12.03.21 um 18:08 schrieb Nirmoy Das:

KIQ ring is being operated by kfd as well as amdgpu.
KFD is using kiq lock, we should the same from amdgpu side
as well.

Signed-off-by: Nirmoy Das 


Acked-by: Christian König 


---
  drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | 15 +++
  1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
index 1915b9b95106..892dc6b269fb 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
@@ -462,20 +462,25 @@ int amdgpu_gfx_disable_kcq(struct amdgpu_device *adev)
  {
struct amdgpu_kiq *kiq = &adev->gfx.kiq;
struct amdgpu_ring *kiq_ring = &kiq->ring;
-   int i;
+   int i, r;
  
  	if (!kiq->pmf || !kiq->pmf->kiq_unmap_queues)

return -EINVAL;
  
+	spin_lock(&adev->gfx.kiq.ring_lock);

if (amdgpu_ring_alloc(kiq_ring, kiq->pmf->unmap_queues_size *
-   adev->gfx.num_compute_rings))
+   adev->gfx.num_compute_rings)) {
+   spin_unlock(&adev->gfx.kiq.ring_lock);
return -ENOMEM;
+   }
  
  	for (i = 0; i < adev->gfx.num_compute_rings; i++)

kiq->pmf->kiq_unmap_queues(kiq_ring, &adev->gfx.compute_ring[i],
   RESET_QUEUES, 0, 0);
+   r = amdgpu_ring_test_helper(kiq_ring);
+   spin_unlock(&adev->gfx.kiq.ring_lock);
  
-	return amdgpu_ring_test_helper(kiq_ring);

+   return r;
  }
  
  int amdgpu_queue_mask_bit_to_set_resource_bit(struct amdgpu_device *adev,

@@ -518,12 +523,13 @@ int amdgpu_gfx_enable_kcq(struct amdgpu_device *adev)
  
  	DRM_INFO("kiq ring mec %d pipe %d q %d\n", kiq_ring->me, kiq_ring->pipe,

kiq_ring->queue);
-
+   spin_lock(&adev->gfx.kiq.ring_lock);
r = amdgpu_ring_alloc(kiq_ring, kiq->pmf->map_queues_size *
adev->gfx.num_compute_rings +
kiq->pmf->set_resources_size);
if (r) {
DRM_ERROR("Failed to lock KIQ (%d).\n", r);
+   spin_unlock(&adev->gfx.kiq.ring_lock);
return r;
}
  
@@ -532,6 +538,7 @@ int amdgpu_gfx_enable_kcq(struct amdgpu_device *adev)

kiq->pmf->kiq_map_queues(kiq_ring, &adev->gfx.compute_ring[i]);
  
  	r = amdgpu_ring_test_helper(kiq_ring);

+   spin_unlock(&adev->gfx.kiq.ring_lock);
if (r)
DRM_ERROR("KCQ enable failed\n");
  


___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/1] drm/amdgpu: wrap kiq ring ops with kiq spinlock

2021-03-12 Thread Christian König 



Am 12.03.21 um 11:24 schrieb Nirmoy:


On 3/12/21 10:52 AM, Christian König wrote:

Am 12.03.21 um 10:49 schrieb Nirmoy Das:

KIQ ring is being operated by kfd as well as amdgpu.
KFD is using kiq lock, we should the same from amdgpu side
as well.


Ah, now I knew which functions you mean. This is not strictly 
necessary because that stuff is only called during bootup and not 
later on.



OK, not so serious issue as I was thinking then.






Signed-off-by: Nirmoy Das 
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | 10 --
  1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c

index 1915b9b95106..8fe370e5358d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
@@ -467,13 +467,17 @@ int amdgpu_gfx_disable_kcq(struct 
amdgpu_device *adev)

  if (!kiq->pmf || !kiq->pmf->kiq_unmap_queues)
  return -EINVAL;
  +    spin_lock(&adev->gfx.kiq.ring_lock);
  if (amdgpu_ring_alloc(kiq_ring, kiq->pmf->unmap_queues_size *
-    adev->gfx.num_compute_rings))
+    adev->gfx.num_compute_rings)) {
+    spin_unlock(&adev->gfx.kiq.ring_lock);
  return -ENOMEM;
+    }
    for (i = 0; i < adev->gfx.num_compute_rings; i++)
  kiq->pmf->kiq_unmap_queues(kiq_ring, 
&adev->gfx.compute_ring[i],

 RESET_QUEUES, 0, 0);
+    spin_unlock(&adev->gfx.kiq.ring_lock);
    return amdgpu_ring_test_helper(kiq_ring);


The ring test accesses the ring buffer as well.



Shall I send a v2 ?


I'm not 100% sure. Please test this with lockdep enabled, if that 
doesn't raise anything we can probably commit it just to be on the clean 
side.


Regards,
Christian.




Thanks,

Nirmoy





Regards,
Christian.


  }
@@ -518,18 +522,20 @@ int amdgpu_gfx_enable_kcq(struct amdgpu_device 
*adev)
    DRM_INFO("kiq ring mec %d pipe %d q %d\n", kiq_ring->me, 
kiq_ring->pipe,

  kiq_ring->queue);
-
+    spin_lock(&adev->gfx.kiq.ring_lock);
  r = amdgpu_ring_alloc(kiq_ring, kiq->pmf->map_queues_size *
  adev->gfx.num_compute_rings +
  kiq->pmf->set_resources_size);
  if (r) {
  DRM_ERROR("Failed to lock KIQ (%d).\n", r);
+    spin_unlock(&adev->gfx.kiq.ring_lock);
  return r;
  }
    kiq->pmf->kiq_set_resources(kiq_ring, queue_mask);
  for (i = 0; i < adev->gfx.num_compute_rings; i++)
  kiq->pmf->kiq_map_queues(kiq_ring, 
&adev->gfx.compute_ring[i]);

+    spin_unlock(&adev->gfx.kiq.ring_lock);
    r = amdgpu_ring_test_helper(kiq_ring);
  if (r)




___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/1] drm/amdgpu: wrap kiq ring ops with kiq spinlock

2021-03-12 Thread Nirmoy 


On 3/12/21 10:52 AM, Christian König wrote:

Am 12.03.21 um 10:49 schrieb Nirmoy Das:

KIQ ring is being operated by kfd as well as amdgpu.
KFD is using kiq lock, we should the same from amdgpu side
as well.


Ah, now I knew which functions you mean. This is not strictly 
necessary because that stuff is only called during bootup and not 
later on.



OK, not so serious issue as I was thinking then.






Signed-off-by: Nirmoy Das 
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | 10 --
  1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c

index 1915b9b95106..8fe370e5358d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
@@ -467,13 +467,17 @@ int amdgpu_gfx_disable_kcq(struct amdgpu_device 
*adev)

  if (!kiq->pmf || !kiq->pmf->kiq_unmap_queues)
  return -EINVAL;
  +    spin_lock(&adev->gfx.kiq.ring_lock);
  if (amdgpu_ring_alloc(kiq_ring, kiq->pmf->unmap_queues_size *
-    adev->gfx.num_compute_rings))
+    adev->gfx.num_compute_rings)) {
+    spin_unlock(&adev->gfx.kiq.ring_lock);
  return -ENOMEM;
+    }
    for (i = 0; i < adev->gfx.num_compute_rings; i++)
  kiq->pmf->kiq_unmap_queues(kiq_ring, 
&adev->gfx.compute_ring[i],

 RESET_QUEUES, 0, 0);
+    spin_unlock(&adev->gfx.kiq.ring_lock);
    return amdgpu_ring_test_helper(kiq_ring);


The ring test accesses the ring buffer as well.



Shall I send a v2 ?


Thanks,

Nirmoy





Regards,
Christian.


  }
@@ -518,18 +522,20 @@ int amdgpu_gfx_enable_kcq(struct amdgpu_device 
*adev)
    DRM_INFO("kiq ring mec %d pipe %d q %d\n", kiq_ring->me, 
kiq_ring->pipe,

  kiq_ring->queue);
-
+    spin_lock(&adev->gfx.kiq.ring_lock);
  r = amdgpu_ring_alloc(kiq_ring, kiq->pmf->map_queues_size *
  adev->gfx.num_compute_rings +
  kiq->pmf->set_resources_size);
  if (r) {
  DRM_ERROR("Failed to lock KIQ (%d).\n", r);
+    spin_unlock(&adev->gfx.kiq.ring_lock);
  return r;
  }
    kiq->pmf->kiq_set_resources(kiq_ring, queue_mask);
  for (i = 0; i < adev->gfx.num_compute_rings; i++)
  kiq->pmf->kiq_map_queues(kiq_ring, 
&adev->gfx.compute_ring[i]);

+    spin_unlock(&adev->gfx.kiq.ring_lock);
    r = amdgpu_ring_test_helper(kiq_ring);
  if (r)



___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/1] drm/amdgpu: wrap kiq ring ops with kiq spinlock

2021-03-12 Thread Christian König 

Am 12.03.21 um 10:49 schrieb Nirmoy Das:

KIQ ring is being operated by kfd as well as amdgpu.
KFD is using kiq lock, we should the same from amdgpu side
as well.


Ah, now I knew which functions you mean. This is not strictly necessary 
because that stuff is only called during bootup and not later on.




Signed-off-by: Nirmoy Das 
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | 10 --
  1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
index 1915b9b95106..8fe370e5358d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
@@ -467,13 +467,17 @@ int amdgpu_gfx_disable_kcq(struct amdgpu_device *adev)
if (!kiq->pmf || !kiq->pmf->kiq_unmap_queues)
return -EINVAL;
  
+	spin_lock(&adev->gfx.kiq.ring_lock);

if (amdgpu_ring_alloc(kiq_ring, kiq->pmf->unmap_queues_size *
-   adev->gfx.num_compute_rings))
+   adev->gfx.num_compute_rings)) {
+   spin_unlock(&adev->gfx.kiq.ring_lock);
return -ENOMEM;
+   }
  
  	for (i = 0; i < adev->gfx.num_compute_rings; i++)

kiq->pmf->kiq_unmap_queues(kiq_ring, &adev->gfx.compute_ring[i],
   RESET_QUEUES, 0, 0);
+   spin_unlock(&adev->gfx.kiq.ring_lock);
  
  	return amdgpu_ring_test_helper(kiq_ring);


The ring test accesses the ring buffer as well.

Regards,
Christian.


  }
@@ -518,18 +522,20 @@ int amdgpu_gfx_enable_kcq(struct amdgpu_device *adev)
  
  	DRM_INFO("kiq ring mec %d pipe %d q %d\n", kiq_ring->me, kiq_ring->pipe,

kiq_ring->queue);
-
+   spin_lock(&adev->gfx.kiq.ring_lock);
r = amdgpu_ring_alloc(kiq_ring, kiq->pmf->map_queues_size *
adev->gfx.num_compute_rings +
kiq->pmf->set_resources_size);
if (r) {
DRM_ERROR("Failed to lock KIQ (%d).\n", r);
+   spin_unlock(&adev->gfx.kiq.ring_lock);
return r;
}
  
  	kiq->pmf->kiq_set_resources(kiq_ring, queue_mask);

for (i = 0; i < adev->gfx.num_compute_rings; i++)
kiq->pmf->kiq_map_queues(kiq_ring, &adev->gfx.compute_ring[i]);
+   spin_unlock(&adev->gfx.kiq.ring_lock);
  
  	r = amdgpu_ring_test_helper(kiq_ring);

if (r)


___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx