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

[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 <Felix.Kuehling at amd.com>
> *Sent:* Friday, March 20, 2020 10:20 AM
> *To:* Deucher, Alexander <Alexander.Deucher at amd.com>; Sierra Guiza, 
> Alejandro (Alex) <Alex.Sierra at amd.com>; amd-gfx at lists.freedesktop.org 
> <amd-gfx at 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 <amd-gfx-bounces at lists.freedesktop.org> 
>> <mailto:amd-gfx-bounces at lists.freedesktop.org> on behalf of Alex 
>> Sierra <alex.sierra at amd.com> <mailto:alex.sierra at amd.com>
>> *Sent:* Thursday, March 19, 2020 8:22 PM
>> *To:* amd-gfx at lists.freedesktop.org 
>> <mailto:amd-gfx at lists.freedesktop.org> 
>> <amd-gfx at lists.freedesktop.org> <mailto:amd-gfx at lists.freedesktop.org>
>> *Cc:* Sierra Guiza, Alejandro (Alex) <Alex.Sierra at amd.com> 
>> <mailto:Alex.Sierra at 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 <alex.sierra at amd.com> 
>> <mailto:alex.sierra at 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 at lists.freedesktop.org <mailto:amd-gfx at 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 at lists.freedesktop.org  <mailto:amd-gfx at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20200320/64577922/attachment-0001.htm>