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

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

[Ilia Mirkin]

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);
                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);
-- 
2.4.9

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

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

[Samuel Pitoiset]

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.

Did you run a full piglit test this time ? :)

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.

>                  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);

_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Ilia Mirkin]

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.

>
> 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.

>
> 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

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

[Samuel Pitoiset]

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

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

[Ilia Mirkin]

On Sat, Oct 10, 2015 at 3:55 PM, Samuel Pitoiset
<samuel.pitoiset@gmail.com> wrote:
>
>
> 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.

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

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

[Ilia Mirkin]

On Sat, Oct 10, 2015 at 4:21 PM, Samuel Pitoiset
<samuel.pitoiset@gmail.com> wrote:
>
>
> On 10/10/2015 09:58 PM, Ilia Mirkin wrote:
>>
>> On Sat, Oct 10, 2015 at 3:55 PM, Samuel Pitoiset
>> <samuel.pitoiset@gmail.com> wrote:
>>>
>>>
>>> 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.
>>
>> What does?
>
>
> I mean, we should either use PUSH_SPACE everywhere or not at all, and always
> breaks (or not) when PUSH_SPACE fails.
> That's really a minor issue.

It's actually a major issue. Error-handling is practically
non-existent. There are a couple of spots here and there, but it
doesn't really scale up. I guess I (semi-)accidentally removed a
couple of spots that error checked, but, again, meh. Doing this for
real will require some careful thought.

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

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

[Samuel Pitoiset]

On 10/10/2015 09:58 PM, Ilia Mirkin wrote:
> On Sat, Oct 10, 2015 at 3:55 PM, Samuel Pitoiset
> <samuel.pitoiset@gmail.com> wrote:
>>
>> 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.
> What does?

I mean, we should either use PUSH_SPACE everywhere or not at all, and 
always breaks (or not) when PUSH_SPACE fails.
That's really a minor issue.


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

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

[Samuel Pitoiset]

On 10/10/2015 10:17 PM, Ilia Mirkin wrote:
> On Sat, Oct 10, 2015 at 4:21 PM, Samuel Pitoiset
> <samuel.pitoiset@gmail.com> wrote:
>>
>> On 10/10/2015 09:58 PM, Ilia Mirkin wrote:
>>> On Sat, Oct 10, 2015 at 3:55 PM, Samuel Pitoiset
>>> <samuel.pitoiset@gmail.com> wrote:
>>>>
>>>> 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.
>>> What does?
>>
>> I mean, we should either use PUSH_SPACE everywhere or not at all, and always
>> breaks (or not) when PUSH_SPACE fails.
>> That's really a minor issue.
> It's actually a major issue. Error-handling is practically
> non-existent. There are a couple of spots here and there, but it
> doesn't really scale up. I guess I (semi-)accidentally removed a
> couple of spots that error checked, but, again, meh. Doing this for
> real will require some careful thought.

Yeah, okay. So we really need to improve error-handling. :)

>    -ilia

_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Samuel Pitoiset]

I did a full piglit run on Fermi. There are no regressions and you fixed 
texelFetch tests and other ones which failed with that assert.

I'm lazy to do it on Tesla, so:

Reviewed-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>

Thanks!

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);
>                  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