[Mesa-dev] [PATCH 2/2] radv/winsys: Fix radv_amdgpu_cs_grow min_size argument. (v2)

[Bas Nieuwenhuizen]

On Mon, Oct 10, 2016 at 9:19 PM, Gustaw Smolarczyk <wielkiegie at gmail.com> wrote:
> 2016-10-10 22:04 GMT+02:00 Bas Nieuwenhuizen <bas at basnieuwenhuizen.nl>:
>> Hi Gustaw,
>>
>> The patch is
>> reviewed-by: Bas Nieuwenhuizen <bas at basnieuwenhuizen.nl>
>
> Thanks, please push it since I don't have commit access.

>
>>
>> What needs to be done too, is checking if the resulting IB becomes too
>> large in the SI case, and handling that gracefully. I don't care if
>> that happens with this patch, or if someone writes a follow up patch
>> though
>
> By handling gracefully do you mean setting cs->failed? Or should we
> try to work around the limitation instead?
>
> Also, I am not sure what is too large for SI. From a quick look at
> cs_submit_sysmem I see that if there was a cs with cdw > 0xffff8 we
> would get assertion failure since cnt would be 0 (1Mi dwords seems to
> be the limitation here). Should we try to divide such a cs or is that
> not really possible without INDIRECT_BUFFER packet? I imagine that
> just submitting a few cs's one after another might have other
> consequences that are not really simple to reason about on this level
> and we would need to divide them on a higher level.

That limit is correct (though don't forget to multiply by 4 for words
vs bytes). We indeed want to set failed and also set cdw to 0, so the
driver does not do out of bounds accesses. (basically the same path as
if new_buf is NULL)

Dividing the CS is going to be hard, as we need to re-emit all state
at division, and radv isn't set up to handle that currently. Also IIRC
the 1 million dwords is more than a factor 10 larger than the largest
I've seen a GL app use per frame, so it probably isn't that useful
either.

>
> I am very new to the radeon driver development so I might be asking
> stupid questions. If so, I am sorry for taking your time.
>
> Regards,
> Gustaw
>
>>
>> Yours sincerely,
>> Bas Nieuwenhuizen
>>
>>
>> On Oct 10, 2016 1:22 PM, "Gustaw Smolarczyk" <wielkiegie at gmail.com> wrote:
>>>
>>> Ping.
>>>
>>> 2016-10-06 19:50 GMT+02:00 Gustaw Smolarczyk <wielkiegie at gmail.com>:
>>> > It's supposed to be how much at least we want to grow the cs, not the
>>> > minimum size of the cs after growth.
>>> >
>>> > v2: Unbreak use_ib_bos.
>>> >     Don't mask the ib_size when !use_ib_bos, since it's not needed.
>>> > ---
>>> >  src/amd/vulkan/winsys/amdgpu/radv_amdgpu_cs.c | 11 +++++++----
>>> >  1 file changed, 7 insertions(+), 4 deletions(-)
>>> >
>>> > diff --git a/src/amd/vulkan/winsys/amdgpu/radv_amdgpu_cs.c b/src/amd/vulkan/winsys/amdgpu/radv_amdgpu_cs.c
>>> > index dedc778..c07c092 100644
>>> > --- a/src/amd/vulkan/winsys/amdgpu/radv_amdgpu_cs.c
>>> > +++ b/src/amd/vulkan/winsys/amdgpu/radv_amdgpu_cs.c
>>> > @@ -178,10 +178,6 @@ radv_amdgpu_cs_create(struct radeon_winsys *ws,
>>> >  static void radv_amdgpu_cs_grow(struct radeon_winsys_cs *_cs, size_t min_size)
>>> >  {
>>> >         struct radv_amdgpu_cs *cs = radv_amdgpu_cs(_cs);
>>> > -       uint64_t ib_size = MAX2(min_size * 4 + 16, cs->base.max_dw * 4 * 2);
>>> > -
>>> > -       /* max that fits in the chain size field. */
>>> > -       ib_size = MIN2(ib_size, 0xfffff);
>>> >
>>> >         if (cs->failed) {
>>> >                 cs->base.cdw = 0;
>>> > @@ -189,6 +185,8 @@ static void radv_amdgpu_cs_grow(struct radeon_winsys_cs *_cs, size_t min_size)
>>> >         }
>>> >
>>> >         if (!cs->ws->use_ib_bos) {
>>> > +               uint64_t ib_size = MAX2((cs->base.cdw + min_size) * 4 + 16,
>>> > +                                       cs->base.max_dw * 4 * 2);
>>> >                 uint32_t *new_buf = realloc(cs->base.buf, ib_size);
>>> >                 if (new_buf) {
>>> >                         cs->base.buf = new_buf;
>>> > @@ -200,6 +198,11 @@ static void radv_amdgpu_cs_grow(struct radeon_winsys_cs *_cs, size_t min_size)
>>> >                 return;
>>> >         }
>>> >
>>> > +       uint64_t ib_size = MAX2(min_size * 4 + 16, cs->base.max_dw * 4 * 2);
>>> > +
>>> > +       /* max that fits in the chain size field. */
>>> > +       ib_size = MIN2(ib_size, 0xfffff);
>>> > +
>>> >         while (!cs->base.cdw || (cs->base.cdw & 7) != 4)
>>> >                 cs->base.buf[cs->base.cdw++] = 0xffff1000;
>>> >
>>> > --
>>> > 2.10.0
>>> >
>>> _______________________________________________
>>> mesa-dev mailing list
>>> mesa-dev at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev