[PATCH] drm/amdgpu: optimize to drop preamble IB for old GPUs

[Christian König]

Yes and exactly that is the total mess I was talking about :)

The backend should be given clear orders which it act on. In other words 
"load_global_*", "load_ce_ram" etc...

The middle layer in amdgpu_ib_schedule()  then decides based of the 
flags if the what orders the backend should execute.

If you want to do this right I would suggest that you clean it up 
thoughtfully.

If you want to just get it working for now I suggest to adjust the 
skip_preamble variable in amdgpu_ib_schedule() and make sure that we 
never skip a preamble when gfxoff is enabled.

Regards,
Christian.

Am 17.05.21 um 09:20 schrieb Chen, Jiansong (Simon):
> [AMD Official Use Only]
>
> Does't the below code in gfx_v8_ring_emit_cntxcntl do almost the same thing as dropping the preamble ib. I cannot understand why bother to duplicate the optimization and cause a mess
> In the common code.
>                  /* set load_ce_ram if preamble presented */
>                  if (AMDGPU_PREAMBLE_IB_PRESENT & flags)
>                          dw2 |= 0x10000000;
>          } else {
>                  /* still load_ce_ram if this is the first time preamble presented
>                   * although there is no context switch happens.
>                   */
>                  if (AMDGPU_PREAMBLE_IB_PRESENT_FIRST & flags)
>                          dw2 |= 0x10000000;
>          }
>
> -----Original Message-----
> From: Christian König <ckoenig.leichtzumerken at gmail.com>
> Sent: Monday, May 17, 2021 2:56 PM
> To: Chen, Jiansong (Simon) <Jiansong.Chen at amd.com>; Koenig, Christian <Christian.Koenig at amd.com>; amd-gfx at lists.freedesktop.org
> Subject: Re: [PATCH] drm/amdgpu: optimize to drop preamble IB for old GPUs
>
> Am 17.05.21 um 08:51 schrieb Chen, Jiansong (Simon):
>> [AMD Official Use Only]
>>
>> Doesn't  current solution always enable the optimization in a safe and more clear way?
> No, we also need this for gfx8 where gfxoff is currently not implemented.
>
> Additional to that we mix common frontend handling into the backend with this approach.
>
> But you could clean up the code in amdgpu_ib_schedule() quite a bit.
>
> Regards,
> Christian.
>
>> 1. for gfx8/9/10 we use load_ce_ram in context_control to control the optimization.
>> 2. for gfx6/7, we directly drop the preamble ib.
>>
>> Regards,
>> Jiansong
>> -----Original Message-----
>> From: Koenig, Christian <Christian.Koenig at amd.com>
>> Sent: Monday, May 17, 2021 2:42 PM
>> To: Chen, Jiansong (Simon) <Jiansong.Chen at amd.com>;
>> amd-gfx at lists.freedesktop.org
>> Subject: Re: [PATCH] drm/amdgpu: optimize to drop preamble IB for old
>> GPUs
>>
>> Well NAK, as discussed checking the global flag is more flexible since it will still enable the preamble drop when gfxoff is disabled.
>>
>> Christian.
>>
>> Am 17.05.21 um 06:39 schrieb Jiansong Chen:
>>> The optimization is safe for old GPUs and can help performance.
>>>
>>> Signed-off-by: Jiansong Chen <Jiansong.Chen at amd.com>
>>> Change-Id: Id3b1250f1fe46dddbe8498894fb97e9753b7cafe
>>> ---
>>>     drivers/gpu/drm/amd/amdgpu/gfx_v6_0.c | 6 ++++++
>>>     drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c | 6 ++++++
>>>     2 files changed, 12 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v6_0.c
>>> b/drivers/gpu/drm/amd/amdgpu/gfx_v6_0.c
>>> index 3a8d52a54873..c915cc439484 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v6_0.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v6_0.c
>>> @@ -1873,6 +1873,12 @@ static void gfx_v6_0_ring_emit_ib(struct amdgpu_ring *ring,
>>>                 amdgpu_ring_write(ring, 0);
>>>         }
>>>
>>> +     /* drop the CE preamble IB for the same context */
>>> +     if ((ib->flags & AMDGPU_IB_FLAG_PREAMBLE) &&
>>> +         !(flags & AMDGPU_HAVE_CTX_SWITCH) &&
>>> +         !(flags & AMDGPU_PREAMBLE_IB_PRESENT_FIRST))
>>> +             return;
>>> +
>>>         if (ib->flags & AMDGPU_IB_FLAG_CE)
>>>                 header = PACKET3(PACKET3_INDIRECT_BUFFER_CONST, 2);
>>>         else
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
>>> b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
>>> index c35fdd2ef2d4..6d9ccae48024 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
>>> @@ -2269,6 +2269,12 @@ static void gfx_v7_0_ring_emit_ib_gfx(struct amdgpu_ring *ring,
>>>                 amdgpu_ring_write(ring, 0);
>>>         }
>>>
>>> +     /* drop the CE preamble IB for the same context */
>>> +     if ((ib->flags & AMDGPU_IB_FLAG_PREAMBLE) &&
>>> +         !(flags & AMDGPU_HAVE_CTX_SWITCH) &&
>>> +         !(flags & AMDGPU_PREAMBLE_IB_PRESENT_FIRST))
>>> +             return;
>>> +
>>>         if (ib->flags & AMDGPU_IB_FLAG_CE)
>>>                 header = PACKET3(PACKET3_INDIRECT_BUFFER_CONST, 2);
>>>         else
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx at lists.freedesktop.org
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flist
>> s.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=04%7C01%7CJi
>> ansong.Chen%40amd.com%7Cf80f7d9888f4427c2b1408d91900e335%7C3dd8961fe48
>> 84e608e11a82d994e183d%7C0%7C0%7C637568313869095131%7CUnknown%7CTWFpbGZ
>> sb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3
>> D%7C1000&sdata=MF1%2BhakHpB8N9B8JwXCA9yB1hIy4CNNMok6ASz3AOU0%3D&am
>> p;reserved=0