Re: [PATCH 1/4] drm/amdgpu: add stride to calculate oss ring offsets

2020-03-20 Thread Christian König 

Am 20.03.20 um 15:20 schrieb Felix Kuehling:

On 2020-03-20 10:06, Deucher, Alexander wrote:


[AMD Public Use]


This seems kind of complicated and error prone.  I didn't realize the 
extent to the changes required.  I think it would be better to either 
add arcturus specific versions of these functions or just go with 
your original approach and add a new arcturus_ih.c.  If you go with 
the second route however, no need to show all your intermediate 
steps, just add the new files in one commit.


Hi Alex,


I suggested the approach in this patch series since to minimize code 
duplication and maintain readability of the code. I don't think it's 
very error prone. I believe this is more maintainable than a separate 
arcturus_ih.c. I'll have some more specific comments on Alejandro's 
patches.




Question is rather if Arcturus has really the same OSS block than Vega10 
or if the registers are just the same and at a different offset?


If the later (which I suspect) than that should really be the same file.

Regards,
Christian.



Regards,
  Felix




Alex


*From:* amd-gfx  on behalf of 
Alex Sierra 

*Sent:* Thursday, March 19, 2020 8:22 PM
*To:* amd-gfx@lists.freedesktop.org 
*Cc:* Sierra Guiza, Alejandro (Alex) 
*Subject:* [PATCH 1/4] drm/amdgpu: add stride to calculate oss ring 
offsets

Arcturus and vega10 share the same vega10_ih, however both
have different register offsets at the ih ring section.
This variable is used to help calculate ih ring register addresses
from the osssys, that corresponds to the current asic type.

Signed-off-by: Alex Sierra 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c | 4 
 drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h | 1 +
 2 files changed, 5 insertions(+)

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

index 5ed4227f304b..fa384ae9a9bc 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
@@ -279,6 +279,10 @@ int amdgpu_irq_init(struct amdgpu_device *adev)
amdgpu_hotplug_work_func);
 }

+   if (adev->asic_type == CHIP_ARCTURUS)
+   adev->irq.ring_stride = 1;
+   else
+   adev->irq.ring_stride = 0;
 INIT_WORK(&adev->irq.ih1_work, amdgpu_irq_handle_ih1);
 INIT_WORK(&adev->irq.ih2_work, amdgpu_irq_handle_ih2);

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h

index c718e94a55c9..1ec5b735cd9e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
@@ -97,6 +97,7 @@ struct amdgpu_irq {
 struct irq_domain   *domain; /* GPU irq 
controller domain */

 unsigned virq[AMDGPU_MAX_IRQ_SRC_ID];
 uint32_t srbm_soft_reset;
+   unsigned    ring_stride;
 };

 void amdgpu_irq_disable_all(struct amdgpu_device *adev);
--
2.17.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=02%7C01%7Calexander.deucher%40amd.com%7C17d5391c86ff4ceee12b08d7cc64f056%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637202606831789803&sdata=B%2BbtLEKN5A65OEp8se5m1M4aQGX7kxsqYYGTTukF5m8%3D&reserved=0< 
/a>





 


___ amd-gfx mailing list

Re: [PATCH 1/4] drm/amdgpu: add stride to calculate oss ring offsets

2020-03-20 Thread Deucher, Alexander 
[AMD Public Use]

Yes, something like that.

Alex

From: Kuehling, Felix 
Sent: Friday, March 20, 2020 10:47 AM
To: Deucher, Alexander ; Sierra Guiza, Alejandro 
(Alex) ; amd-gfx@lists.freedesktop.org 

Subject: Re: [PATCH 1/4] drm/amdgpu: add stride to calculate oss ring offsets



On 2020-03-20 10:39, Deucher, Alexander wrote:

[AMD Public Use]

I'm worried we'll miss a register by accident.  We went with per IP sub drivers 
to avoid handling complexities around IP differences if possible.  Also the 
scheme seems like kind of a one off compared to what we do for other IPs.  Can 
we structure it more like how we handle SDMA instancing since it seems to 
mainly affect IH RB instances?

That's more or less what I had in mind, but haven't looked at the SDMA 
implementation in detail. So do you mean defining macros WREG32_IH_RING(ring, 
offset, value) and RREG32_IH_RING(ring, offset) analogous to WREG32_SDMA and 
RREG32_SDMA? It would only apply to IH ring-specific registers. Not to other 
general IH registers.


Regards,
  Felix


Alex


From: Kuehling, Felix <mailto:felix.kuehl...@amd.com>
Sent: Friday, March 20, 2020 10:20 AM
To: Deucher, Alexander 
<mailto:alexander.deuc...@amd.com>; Sierra Guiza, 
Alejandro (Alex) <mailto:alex.sie...@amd.com>; 
amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org> 
<mailto:amd-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 1/4] drm/amdgpu: add stride to calculate oss ring offsets

On 2020-03-20 10:06, Deucher, Alexander wrote:

[AMD Public Use]

This seems kind of complicated and error prone.  I didn't realize the extent to 
the changes required.  I think it would be better to either add arcturus 
specific versions of these functions or just go with your original approach and 
add a new arcturus_ih.c.  If you go with the second route however, no need to 
show all your intermediate steps, just add the new files in one commit.

Hi Alex,


I suggested the approach in this patch series since to minimize code 
duplication and maintain readability of the code. I don't think it's very error 
prone. I believe this is more maintainable than a separate arcturus_ih.c. I'll 
have some more specific comments on Alejandro's patches.


Regards,
  Felix


Alex


From: amd-gfx 
<mailto:amd-gfx-boun...@lists.freedesktop.org>
 on behalf of Alex Sierra <mailto:alex.sie...@amd.com>
Sent: Thursday, March 19, 2020 8:22 PM
To: amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org> 
<mailto:amd-gfx@lists.freedesktop.org>
Cc: Sierra Guiza, Alejandro (Alex) 
<mailto:alex.sie...@amd.com>
Subject: [PATCH 1/4] drm/amdgpu: add stride to calculate oss ring offsets

Arcturus and vega10 share the same vega10_ih, however both
have different register offsets at the ih ring section.
This variable is used to help calculate ih ring register addresses
from the osssys, that corresponds to the current asic type.

Signed-off-by: Alex Sierra <mailto:alex.sie...@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c | 4 
 drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
index 5ed4227f304b..fa384ae9a9bc 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
@@ -279,6 +279,10 @@ int amdgpu_irq_init(struct amdgpu_device *adev)
 amdgpu_hotplug_work_func);
 }

+   if (adev->asic_type == CHIP_ARCTURUS)
+   adev->irq.ring_stride = 1;
+   else
+   adev->irq.ring_stride = 0;
 INIT_WORK(&adev->irq.ih1_work, amdgpu_irq_handle_ih1);
 INIT_WORK(&adev->irq.ih2_work, amdgpu_irq_handle_ih2);

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
index c718e94a55c9..1ec5b735cd9e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
@@ -97,6 +97,7 @@ struct amdgpu_irq {
 struct irq_domain   *domain; /* GPU irq controller domain 
*/
 unsignedvirq[AMDGPU_MAX_IRQ_SRC_ID];
 uint32_tsrbm_soft_reset;
+   unsignedring_stride;
 };

 void amdgpu_irq_disable_all(struct amdgpu_device *adev);
--
2.17.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Calexander.deucher%40amd.com%7C17d5391c86ff4ceee12b08d7cc64f056%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637202606831789803&sdata=B%2BbtLEKN5A65OEp8se

Re: [PATCH 1/4] drm/amdgpu: add stride to calculate oss ring offsets

2020-03-20 Thread Felix Kuehling 


On 2020-03-20 10:39, Deucher, Alexander wrote:


[AMD Public Use]


I'm worried we'll miss a register by accident.  We went with per IP 
sub drivers to avoid handling complexities around IP differences if 
possible.  Also the scheme seems like kind of a one off compared to 
what we do for other IPs.  Can we structure it more like how we handle 
SDMA instancing since it seems to mainly affect IH RB instances?


That's more or less what I had in mind, but haven't looked at the SDMA 
implementation in detail. So do you mean defining macros 
WREG32_IH_RING(ring, offset, value) and RREG32_IH_RING(ring, offset) 
analogous to WREG32_SDMA and RREG32_SDMA? It would only apply to IH 
ring-specific registers. Not to other general IH registers.



Regards,
  Felix




Alex


*From:* Kuehling, Felix 
*Sent:* Friday, March 20, 2020 10:20 AM
*To:* Deucher, Alexander ; Sierra Guiza, 
Alejandro (Alex) ; amd-gfx@lists.freedesktop.org 

*Subject:* Re: [PATCH 1/4] drm/amdgpu: add stride to calculate oss 
ring offsets

On 2020-03-20 10:06, Deucher, Alexander wrote:


[AMD Public Use]


This seems kind of complicated and error prone.  I didn't realize the 
extent to the changes required.  I think it would be better to either 
add arcturus specific versions of these functions or just go with 
your original approach and add a new arcturus_ih.c.  If you go with 
the second route however, no need to show all your intermediate 
steps, just add the new files in one commit.


Hi Alex,


I suggested the approach in this patch series since to minimize code 
duplication and maintain readability of the code. I don't think it's 
very error prone. I believe this is more maintainable than a separate 
arcturus_ih.c. I'll have some more specific comments on Alejandro's 
patches.



Regards,
  Felix




Alex


*From:* amd-gfx  
<mailto:amd-gfx-boun...@lists.freedesktop.org> on behalf of Alex 
Sierra  <mailto:alex.sie...@amd.com>

*Sent:* Thursday, March 19, 2020 8:22 PM
*To:* amd-gfx@lists.freedesktop.org 
<mailto:amd-gfx@lists.freedesktop.org> 
 <mailto:amd-gfx@lists.freedesktop.org>
*Cc:* Sierra Guiza, Alejandro (Alex)  
<mailto:alex.sie...@amd.com>
*Subject:* [PATCH 1/4] drm/amdgpu: add stride to calculate oss ring 
offsets

Arcturus and vega10 share the same vega10_ih, however both
have different register offsets at the ih ring section.
This variable is used to help calculate ih ring register addresses
from the osssys, that corresponds to the current asic type.

Signed-off-by: Alex Sierra  
<mailto:alex.sie...@amd.com>

---
 drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c | 4 
 drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h | 1 +
 2 files changed, 5 insertions(+)

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

index 5ed4227f304b..fa384ae9a9bc 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
@@ -279,6 +279,10 @@ int amdgpu_irq_init(struct amdgpu_device *adev)
amdgpu_hotplug_work_func);
 }

+   if (adev->asic_type == CHIP_ARCTURUS)
+   adev->irq.ring_stride = 1;
+   else
+   adev->irq.ring_stride = 0;
 INIT_WORK(&adev->irq.ih1_work, amdgpu_irq_handle_ih1);
 INIT_WORK(&adev->irq.ih2_work, amdgpu_irq_handle_ih2);

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h

index c718e94a55c9..1ec5b735cd9e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
@@ -97,6 +97,7 @@ struct amdgpu_irq {
 struct irq_domain   *domain; /* GPU irq 
controller domain */

 unsigned virq[AMDGPU_MAX_IRQ_SRC_ID];
 uint32_t srbm_soft_reset;
+   unsigned ring_stride;
 };

 void amdgpu_irq_disable_all(struct amdgpu_device *adev);
--
2.17.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org <mailto:amd-gfx@lists.freedesktop.org>
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Calexander.deucher%40amd.com%7C17d5391c86ff4ceee12b08d7cc64f056%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637202606831789803&sdata=B%2BbtLEKN5A65OEp8se5m1M4aQGX7kxsqYYGTTukF5m8%3D&reserved=0 
<https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cfelix.kuehling%40amd.com%7C7e44179e2a0d49c972ba08d7ccd7e626%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637203100032276037&sdata=bs%2F33P5feC0SRxcy6JyiVLkLG6uA7fSWQ4EeCmGItU0%3D&reserved=0>


___
amd-gfx mailing list
amd-gfx@lists.freedesktop.o

Re: [PATCH 1/4] drm/amdgpu: add stride to calculate oss ring offsets

2020-03-20 Thread Deucher, Alexander 
[AMD Public Use]

I'm worried we'll miss a register by accident.  We went with per IP sub drivers 
to avoid handling complexities around IP differences if possible.  Also the 
scheme seems like kind of a one off compared to what we do for other IPs.  Can 
we structure it more like how we handle SDMA instancing since it seems to 
mainly affect IH RB instances?

Alex


From: Kuehling, Felix 
Sent: Friday, March 20, 2020 10:20 AM
To: Deucher, Alexander ; Sierra Guiza, Alejandro 
(Alex) ; amd-gfx@lists.freedesktop.org 

Subject: Re: [PATCH 1/4] drm/amdgpu: add stride to calculate oss ring offsets

On 2020-03-20 10:06, Deucher, Alexander wrote:

[AMD Public Use]

This seems kind of complicated and error prone.  I didn't realize the extent to 
the changes required.  I think it would be better to either add arcturus 
specific versions of these functions or just go with your original approach and 
add a new arcturus_ih.c.  If you go with the second route however, no need to 
show all your intermediate steps, just add the new files in one commit.

Hi Alex,


I suggested the approach in this patch series since to minimize code 
duplication and maintain readability of the code. I don't think it's very error 
prone. I believe this is more maintainable than a separate arcturus_ih.c. I'll 
have some more specific comments on Alejandro's patches.


Regards,
  Felix


Alex


From: amd-gfx 
<mailto:amd-gfx-boun...@lists.freedesktop.org>
 on behalf of Alex Sierra <mailto:alex.sie...@amd.com>
Sent: Thursday, March 19, 2020 8:22 PM
To: amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org> 
<mailto:amd-gfx@lists.freedesktop.org>
Cc: Sierra Guiza, Alejandro (Alex) 
<mailto:alex.sie...@amd.com>
Subject: [PATCH 1/4] drm/amdgpu: add stride to calculate oss ring offsets

Arcturus and vega10 share the same vega10_ih, however both
have different register offsets at the ih ring section.
This variable is used to help calculate ih ring register addresses
from the osssys, that corresponds to the current asic type.

Signed-off-by: Alex Sierra <mailto:alex.sie...@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c | 4 
 drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
index 5ed4227f304b..fa384ae9a9bc 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
@@ -279,6 +279,10 @@ int amdgpu_irq_init(struct amdgpu_device *adev)
 amdgpu_hotplug_work_func);
 }

+   if (adev->asic_type == CHIP_ARCTURUS)
+   adev->irq.ring_stride = 1;
+   else
+   adev->irq.ring_stride = 0;
 INIT_WORK(&adev->irq.ih1_work, amdgpu_irq_handle_ih1);
 INIT_WORK(&adev->irq.ih2_work, amdgpu_irq_handle_ih2);

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
index c718e94a55c9..1ec5b735cd9e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
@@ -97,6 +97,7 @@ struct amdgpu_irq {
 struct irq_domain   *domain; /* GPU irq controller domain 
*/
 unsignedvirq[AMDGPU_MAX_IRQ_SRC_ID];
 uint32_tsrbm_soft_reset;
+   unsignedring_stride;
 };

 void amdgpu_irq_disable_all(struct amdgpu_device *adev);
--
2.17.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Calexander.deucher%40amd.com%7C17d5391c86ff4ceee12b08d7cc64f056%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637202606831789803&sdata=B%2BbtLEKN5A65OEp8se5m1M4aQGX7kxsqYYGTTukF5m8%3D&reserved=0<https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cfelix.kuehling%40amd.com%7C7e44179e2a0d49c972ba08d7ccd7e626%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637203100032276037&sdata=bs%2F33P5feC0SRxcy6JyiVLkLG6uA7fSWQ4EeCmGItU0%3D&reserved=0>



___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cfelix.kuehling%40amd.com%7C7e44179e2a0d49c972ba08d7ccd7e626%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637203100032296023&sdata=bil9pUebulcGpl5YhTi9k6yqK8wYDzw6XN%2FSZ9YbR44%3D&reserved=0

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

Re: [PATCH 1/4] drm/amdgpu: add stride to calculate oss ring offsets

2020-03-20 Thread Felix Kuehling 

On 2020-03-19 20:22, Alex Sierra wrote:

Arcturus and vega10 share the same vega10_ih, however both
have different register offsets at the ih ring section.
This variable is used to help calculate ih ring register addresses
from the osssys, that corresponds to the current asic type.

Signed-off-by: Alex Sierra 
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c | 4 
  drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h | 1 +
  2 files changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
index 5ed4227f304b..fa384ae9a9bc 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
@@ -279,6 +279,10 @@ int amdgpu_irq_init(struct amdgpu_device *adev)
amdgpu_hotplug_work_func);
}
  
+	if (adev->asic_type == CHIP_ARCTURUS)

+   adev->irq.ring_stride = 1;
+   else
+   adev->irq.ring_stride = 0;


This can't be right. ring_stride==0 would result in all mmIH_RING(...) 
register access to map to the same physical registers. So effectively 
everything would go to ring0.


Regards,
  Felix



INIT_WORK(&adev->irq.ih1_work, amdgpu_irq_handle_ih1);
INIT_WORK(&adev->irq.ih2_work, amdgpu_irq_handle_ih2);
  
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h

index c718e94a55c9..1ec5b735cd9e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
@@ -97,6 +97,7 @@ struct amdgpu_irq {
struct irq_domain   *domain; /* GPU irq controller domain */
unsignedvirq[AMDGPU_MAX_IRQ_SRC_ID];
uint32_tsrbm_soft_reset;
+   unsignedring_stride;
  };
  
  void amdgpu_irq_disable_all(struct amdgpu_device *adev);

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

Re: [PATCH 1/4] drm/amdgpu: add stride to calculate oss ring offsets

2020-03-20 Thread Felix Kuehling 

On 2020-03-20 10:06, Deucher, Alexander wrote:


[AMD Public Use]


This seems kind of complicated and error prone.  I didn't realize the 
extent to the changes required.  I think it would be better to either 
add arcturus specific versions of these functions or just go with your 
original approach and add a new arcturus_ih.c.  If you go with the 
second route however, no need to show all your intermediate steps, 
just add the new files in one commit.


Hi Alex,


I suggested the approach in this patch series since to minimize code 
duplication and maintain readability of the code. I don't think it's 
very error prone. I believe this is more maintainable than a separate 
arcturus_ih.c. I'll have some more specific comments on Alejandro's patches.



Regards,
  Felix




Alex


*From:* amd-gfx  on behalf of 
Alex Sierra 

*Sent:* Thursday, March 19, 2020 8:22 PM
*To:* amd-gfx@lists.freedesktop.org 
*Cc:* Sierra Guiza, Alejandro (Alex) 
*Subject:* [PATCH 1/4] drm/amdgpu: add stride to calculate oss ring 
offsets

Arcturus and vega10 share the same vega10_ih, however both
have different register offsets at the ih ring section.
This variable is used to help calculate ih ring register addresses
from the osssys, that corresponds to the current asic type.

Signed-off-by: Alex Sierra 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c | 4 
 drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h | 1 +
 2 files changed, 5 insertions(+)

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

index 5ed4227f304b..fa384ae9a9bc 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
@@ -279,6 +279,10 @@ int amdgpu_irq_init(struct amdgpu_device *adev)
amdgpu_hotplug_work_func);
 }

+   if (adev->asic_type == CHIP_ARCTURUS)
+   adev->irq.ring_stride = 1;
+   else
+   adev->irq.ring_stride = 0;
 INIT_WORK(&adev->irq.ih1_work, amdgpu_irq_handle_ih1);
 INIT_WORK(&adev->irq.ih2_work, amdgpu_irq_handle_ih2);

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h

index c718e94a55c9..1ec5b735cd9e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
@@ -97,6 +97,7 @@ struct amdgpu_irq {
 struct irq_domain   *domain; /* GPU irq 
controller domain */

 unsigned virq[AMDGPU_MAX_IRQ_SRC_ID];
 uint32_t srbm_soft_reset;
+   unsigned    ring_stride;
 };

 void amdgpu_irq_disable_all(struct amdgpu_device *adev);
--
2.17.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=02%7C01%7Calexander.deucher%40amd.com%7C17d5391c86ff4ceee12b08d7cc64f056%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637202606831789803&sdata=B%2BbtLEKN5A65OEp8se5m1M4aQGX7kxsqYYGTTukF5m8%3D&reserved=0 



___
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=02%7C01%7Cfelix.kuehling%40amd.com%7C7e44179e2a0d49c972ba08d7ccd7e626%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637203100032296023&sdata=bil9pUebulcGpl5YhTi9k6yqK8wYDzw6XN%2FSZ9YbR44%3D&reserved=0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/4] drm/amdgpu: add stride to calculate oss ring offsets

2020-03-20 Thread Deucher, Alexander 
[AMD Public Use]

This seems kind of complicated and error prone.  I didn't realize the extent to 
the changes required.  I think it would be better to either add arcturus 
specific versions of these functions or just go with your original approach and 
add a new arcturus_ih.c.  If you go with the second route however, no need to 
show all your intermediate steps, just add the new files in one commit.

Alex


From: amd-gfx  on behalf of Alex Sierra 

Sent: Thursday, March 19, 2020 8:22 PM
To: amd-gfx@lists.freedesktop.org 
Cc: Sierra Guiza, Alejandro (Alex) 
Subject: [PATCH 1/4] drm/amdgpu: add stride to calculate oss ring offsets

Arcturus and vega10 share the same vega10_ih, however both
have different register offsets at the ih ring section.
This variable is used to help calculate ih ring register addresses
from the osssys, that corresponds to the current asic type.

Signed-off-by: Alex Sierra 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c | 4 
 drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
index 5ed4227f304b..fa384ae9a9bc 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
@@ -279,6 +279,10 @@ int amdgpu_irq_init(struct amdgpu_device *adev)
 amdgpu_hotplug_work_func);
 }

+   if (adev->asic_type == CHIP_ARCTURUS)
+   adev->irq.ring_stride = 1;
+   else
+   adev->irq.ring_stride = 0;
 INIT_WORK(&adev->irq.ih1_work, amdgpu_irq_handle_ih1);
 INIT_WORK(&adev->irq.ih2_work, amdgpu_irq_handle_ih2);

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
index c718e94a55c9..1ec5b735cd9e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
@@ -97,6 +97,7 @@ struct amdgpu_irq {
 struct irq_domain   *domain; /* GPU irq controller domain 
*/
 unsignedvirq[AMDGPU_MAX_IRQ_SRC_ID];
 uint32_tsrbm_soft_reset;
+   unsignedring_stride;
 };

 void amdgpu_irq_disable_all(struct amdgpu_device *adev);
--
2.17.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=02%7C01%7Calexander.deucher%40amd.com%7C17d5391c86ff4ceee12b08d7cc64f056%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637202606831789803&sdata=B%2BbtLEKN5A65OEp8se5m1M4aQGX7kxsqYYGTTukF5m8%3D&reserved=0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx