Re: [PATCH] nv50, nvc0: don't base decisions on available pushbuf space

[Samuel Pitoiset <samuel.pitoiset-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

On 10/10/2015 09:42 PM, Ilia Mirkin wrote:
> On Sat, Oct 10, 2015 at 3:41 PM, Samuel Pitoiset
> <samuel.pitoiset@gmail.com> wrote:
>> This patch looks fine except that it should be a bit more normalized. I
>> mean, sometimes you break when PUSH_SPACE fails, sometimes not. Same for
>> PUSH_SPACE calls, sometimes you add it sometimes not.
> Meh. We need to get our error checking situation straight, but this
> isn't the patch to do it in.

Yeah, but this needs to be clarified.

>
>> Did you run a full piglit test this time ? :)
> Nope, but I ran a full piglit before this patch. Almost took down my
> box. Probably won't be running it again for this patch.

Ok, I'll run a full piglit this night then.

>
>> See my comment below.
>>
>>
>> On 10/10/2015 11:09 AM, Ilia Mirkin wrote:
>>> We still have to push everything out, might as well kick earlier and
>>> flip pushbufs when we know we'll need it. This resolves some issues with
>>> the new policy of making sure that we always leave a bit of room at the
>>> end for fences.
>>>
>>> Signed-off-by: Ilia Mirkin <imirkin@alum.mit.edu>
>>> Cc: mesa-stable@lists.freedesktop.org
>>> ---
>>>    src/gallium/drivers/nouveau/nv50/nv50_shader_state.c |  9 ++-------
>>>    src/gallium/drivers/nouveau/nv50/nv50_transfer.c     | 16
>>> +++-------------
>>>    src/gallium/drivers/nouveau/nvc0/nvc0_transfer.c     | 20
>>> +++++---------------
>>>    3 files changed, 10 insertions(+), 35 deletions(-)
>>>
>>> diff --git a/src/gallium/drivers/nouveau/nv50/nv50_shader_state.c
>>> b/src/gallium/drivers/nouveau/nv50/nv50_shader_state.c
>>> index fdde11f..941555f 100644
>>> --- a/src/gallium/drivers/nouveau/nv50/nv50_shader_state.c
>>> +++ b/src/gallium/drivers/nouveau/nv50/nv50_shader_state.c
>>> @@ -65,14 +65,9 @@ nv50_constbufs_validate(struct nv50_context *nv50)
>>>                   PUSH_DATA (push, (b << 12) | (i << 8) | p | 1);
>>>                }
>>>                while (words) {
>>> -               unsigned nr;
>>> -
>>> -               if (!PUSH_SPACE(push, 16))
>>> -                  break;
>>> -               nr = PUSH_AVAIL(push);
>>> -               assert(nr >= 16);
>>> -               nr = MIN2(MIN2(nr - 3, words), NV04_PFIFO_MAX_PACKET_LEN);
>>> +               unsigned nr = MIN2(words, NV04_PFIFO_MAX_PACKET_LEN);
>>>    +               PUSH_SPACE(push, nr + 3);
>>
>> This PUSH_SPACE call doesn't seem to be needed for me because
>> NV50_PUSH_EXPLICIT_SPACE_CHECKING is not set and the following BEGIN_XXX
>> calls will allocate space.
> I want to ensure that both of the below commands are in the same
> batch. Not sure if it's necessary, but... don't want to find out. They
> were in the same batch before. And this batch stuff is what was
> causing the M2MF errors I was seeing earlier.
>
>>
>>>                   BEGIN_NV04(push, NV50_3D(CB_ADDR), 1);
>>>                   PUSH_DATA (push, (start << 8) | b);
>>>                   BEGIN_NI04(push, NV50_3D(CB_DATA(0)), nr);
>>> diff --git a/src/gallium/drivers/nouveau/nv50/nv50_transfer.c
>>> b/src/gallium/drivers/nouveau/nv50/nv50_transfer.c
>>> index be51407..9a3fd1e 100644
>>> --- a/src/gallium/drivers/nouveau/nv50/nv50_transfer.c
>>> +++ b/src/gallium/drivers/nouveau/nv50/nv50_transfer.c
>>> @@ -187,14 +187,7 @@ nv50_sifc_linear_u8(struct nouveau_context *nv,
>>>       PUSH_DATA (push, 0);
>>>         while (count) {
>>> -      unsigned nr;
>>> -
>>> -      if (!PUSH_SPACE(push, 16))
>>> -         break;
>>> -      nr = PUSH_AVAIL(push);
>>> -      assert(nr >= 16);
>>> -      nr = MIN2(count, nr - 1);
>>> -      nr = MIN2(nr, NV04_PFIFO_MAX_PACKET_LEN);
>>> +      unsigned nr = MIN2(count, NV04_PFIFO_MAX_PACKET_LEN);
>>>            BEGIN_NI04(push, NV50_2D(SIFC_DATA), nr);
>>>          PUSH_DATAp(push, src, nr);
>>> @@ -395,12 +388,9 @@ nv50_cb_push(struct nouveau_context *nv,
>>>       nouveau_pushbuf_validate(push);
>>>         while (words) {
>>> -      unsigned nr;
>>> -
>>> -      nr = PUSH_AVAIL(push);
>>> -      nr = MIN2(nr - 7, words);
>>> -      nr = MIN2(nr, NV04_PFIFO_MAX_PACKET_LEN - 1);
>>> +      unsigned nr = MIN2(words, NV04_PFIFO_MAX_PACKET_LEN);
>>>    +      PUSH_SPACE(push, nr + 7);
>>>          BEGIN_NV04(push, NV50_3D(CB_DEF_ADDRESS_HIGH), 3);
>>>          PUSH_DATAh(push, bo->offset + base);
>>>          PUSH_DATA (push, bo->offset + base);
>>> diff --git a/src/gallium/drivers/nouveau/nvc0/nvc0_transfer.c
>>> b/src/gallium/drivers/nouveau/nvc0/nvc0_transfer.c
>>> index aaec60a..d459dd6 100644
>>> --- a/src/gallium/drivers/nouveau/nvc0/nvc0_transfer.c
>>> +++ b/src/gallium/drivers/nouveau/nvc0/nvc0_transfer.c
>>> @@ -188,14 +188,10 @@ nvc0_m2mf_push_linear(struct nouveau_context *nv,
>>>       nouveau_pushbuf_validate(push);
>>>         while (count) {
>>> -      unsigned nr;
>>> +      unsigned nr = MIN2(count, NV04_PFIFO_MAX_PACKET_LEN);
>>>    -      if (!PUSH_SPACE(push, 16))
>>> +      if (!PUSH_SPACE(push, nr + 9))
>>>             break;
>>> -      nr = PUSH_AVAIL(push);
>>> -      assert(nr >= 16);
>>> -      nr = MIN2(count, nr - 9);
>>> -      nr = MIN2(nr, NV04_PFIFO_MAX_PACKET_LEN);
>>>            BEGIN_NVC0(push, NVC0_M2MF(OFFSET_OUT_HIGH), 2);
>>>          PUSH_DATAh(push, dst->offset + offset);
>>> @@ -234,14 +230,10 @@ nve4_p2mf_push_linear(struct nouveau_context *nv,
>>>       nouveau_pushbuf_validate(push);
>>>         while (count) {
>>> -      unsigned nr;
>>> +      unsigned nr = MIN2(count, (NV04_PFIFO_MAX_PACKET_LEN - 1));
>>>    -      if (!PUSH_SPACE(push, 16))
>>> +      if (!PUSH_SPACE(push, nr + 10))
>>>             break;
>>> -      nr = PUSH_AVAIL(push);
>>> -      assert(nr >= 16);
>>> -      nr = MIN2(count, nr - 8);
>>> -      nr = MIN2(nr, (NV04_PFIFO_MAX_PACKET_LEN - 1));
>>>            BEGIN_NVC0(push, NVE4_P2MF(UPLOAD_DST_ADDRESS_HIGH), 2);
>>>          PUSH_DATAh(push, dst->offset + offset);
>>> @@ -571,9 +563,7 @@ nvc0_cb_bo_push(struct nouveau_context *nv,
>>>       PUSH_DATA (push, bo->offset + base);
>>>         while (words) {
>>> -      unsigned nr = PUSH_AVAIL(push);
>>> -      nr = MIN2(nr, words);
>>> -      nr = MIN2(nr, NV04_PFIFO_MAX_PACKET_LEN - 1);
>>> +      unsigned nr = MIN2(words, NV04_PFIFO_MAX_PACKET_LEN - 1);
>>>            PUSH_SPACE(push, nr + 2);
>>>          PUSH_REFN (push, bo, NOUVEAU_BO_WR | domain);
>>

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau