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

From: Samuel Pitoiset <samuel.pitoiset@gmail.com>
To: Ilia Mirkin <imirkin@alum.mit.edu>,
	mesa-dev@lists.freedesktop.org, nouveau@lists.freedesktop.org
Cc: mesa-stable@lists.freedesktop.org
Subject: Re: [Nouveau] [PATCH] nv50, nvc0: don't base decisions on available pushbuf space
Date: Sat, 10 Oct 2015 21:41:37 +0200	[thread overview]
Message-ID: <561969F1.9080706@gmail.com> (raw)
In-Reply-To: <1444468187-13868-1-git-send-email-imirkin@alum.mit.edu>

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