Nouveau - Sep 2015 - [PATCH mesa 1/3] nv30: Fix max width / height checks in nv30 sifm code

The sifm object has a limit of 1024x1024 for its input size and 2048x2048
for its output. The code checking this was trying to be clever resulting
in it seeing a surface of e.g 1024x256 being outside of the input size
limit.

This commit fixes this.

Signed-off-by: Hans de Goede <hdegoede at redhat.com>
---
 src/gallium/drivers/nouveau/nv30/nv30_transfer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/gallium/drivers/nouveau/nv30/nv30_transfer.c
b/src/gallium/drivers/nouveau/nv30/nv30_transfer.c
index 214da65..2452071 100644
--- a/src/gallium/drivers/nouveau/nv30/nv30_transfer.c
+++ b/src/gallium/drivers/nouveau/nv30/nv30_transfer.c
@@ -371,7 +371,7 @@ nv30_transfer_rect_blit(XFER_ARGS)
 static bool
 nv30_transfer_sifm(XFER_ARGS)
 {
-   if (!src->pitch || (src->w | src->h) > 1024 || src->w < 2
|| src->h < 2)
+   if (!src->pitch || src->w > 1024 || src->h > 1024 ||
src->w < 2 || src->h < 2)
       return false;
 
    if (src->d > 1 || dst->d > 1)
@@ -381,7 +381,7 @@ nv30_transfer_sifm(XFER_ARGS)
       return false;
 
    if (!dst->pitch) {
-      if ((dst->w | dst->h) > 2048 || dst->w < 2 || dst->h
< 2)
+      if (dst->w > 2048 || dst->h > 2048 || dst->w < 2 ||
dst->h < 2)
          return false;
    } else {
       if (dst->domain != NOUVEAU_BO_VRAM)
-- 
2.4.3

We do not have a generic blitter on nv3x cards, so we must use the
sifm object for color resolving.

This commit divides the sources and dest surfaces in to tiles which
match the constraints of the sifm object, so that color resolving
will work properly on nv3x cards.

Signed-off-by: Hans de Goede <hdegoede at redhat.com>
---
 src/gallium/drivers/nouveau/nv30/nv30_miptree.c | 55 ++++++++++++++++++++++++-
 1 file changed, 54 insertions(+), 1 deletion(-)

diff --git a/src/gallium/drivers/nouveau/nv30/nv30_miptree.c
b/src/gallium/drivers/nouveau/nv30/nv30_miptree.c
index 76bb8b8..c240030 100644
--- a/src/gallium/drivers/nouveau/nv30/nv30_miptree.c
+++ b/src/gallium/drivers/nouveau/nv30/nv30_miptree.c
@@ -149,6 +149,56 @@ static void
 nv30_resource_resolve(struct nv30_context *nv30,
                       const struct pipe_blit_info *info)
 {
+   struct nv30_miptree *src_mt = nv30_miptree(info->src.resource);
+   struct nv30_rect src, dst;
+   unsigned x, x0, x1, y, y1, w, h;
+
+   define_rect(info->src.resource, 0, info->src.box.z,
info->src.box.x,
+      info->src.box.y, info->src.box.width, info->src.box.height,
&src);
+   define_rect(info->dst.resource, 0, info->dst.box.z,
info->dst.box.x,
+      info->dst.box.y, info->dst.box.width, info->dst.box.height,
&dst);
+
+   x0 = src.x0;
+   x1 = src.x1;
+   y1 = src.y1;
+
+   /* On nv3x we must use sifm which is restricted to 512x512 tiles */
+   for (y = src.y0; y < y1; y += h) {
+      h = y1 - y;
+      if (h > 512)
+         h = 512;
+
+      src.y0 = 0;
+      src.y1 = h;
+      src.h = h;
+
+      dst.y1 = dst.y0 + (h >> src_mt->ms_y);
+      dst.h = h >> src_mt->ms_y;
+
+      for (x = x0; x < x1; x += w) {
+         w = x1 - x;
+         if (w > 512)
+            w = 512;
+
+         src.offset = y * src.pitch + x * src.cpp;
+         src.x0 = 0;
+         src.x1 = w;
+         src.w = w;
+
+         dst.offset = (y >> src_mt->ms_y) * dst.pitch +
+                      (x >> src_mt->ms_x) * dst.cpp;
+         dst.x1 = dst.x0 + (w >> src_mt->ms_x);
+         dst.w = w >> src_mt->ms_x;
+
+         nv30_transfer_rect(nv30, BILINEAR, &src, &dst);
+      }
+   }
+}
+
+static void
+nv40_resource_resolve(struct nv30_context *nv30,
+                      const struct pipe_blit_info *info)
+{
    struct nv30_rect src, dst;
 
    define_rect(info->src.resource, 0, info->src.box.z,
info->src.box.x,
@@ -170,7 +220,10 @@ nv30_blit(struct pipe_context *pipe,
        info.dst.resource->nr_samples <= 1 &&
        !util_format_is_depth_or_stencil(info.src.resource->format)
&&
        !util_format_is_pure_integer(info.src.resource->format)) {
-      nv30_resource_resolve(nv30, blit_info);
+      if (nv30->screen->eng3d->oclass >= NV40_3D_CLASS)
+         nv40_resource_resolve(nv30, blit_info);
+      else
+         nv30_resource_resolve(nv30, blit_info);
       return;
    }
 
-- 
2.4.3

msaa use on nv30 may trigger a (mesa?) bug where dmesg says:
 [ 1197.850642] nouveau E[soffice.bin[3785]] fail ttm_validate
 [ 1197.850648] nouveau E[soffice.bin[3785]] validating bo list
 [ 1197.850654] nouveau E[soffice.bin[3785]] validate: -12
 [ 1201.766955] nouveau E[soffice.bin[3785]] fail ttm_validate
 [ 1201.766961] nouveau E[soffice.bin[3785]] validating bo list
 [ 1201.766968] nouveau E[soffice.bin[3785]] validate: -12

After which the program using the msaa visual freezes, and eventually
the entire system freezes. Disable msaa until this is fixed.

This happens on both nv3x and nv4x cards.

Signed-off-by: Hans de Goede <hdegoede at redhat.com>
---
 src/gallium/drivers/nouveau/nv30/nv30_screen.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/src/gallium/drivers/nouveau/nv30/nv30_screen.c
b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
index 7aad26b..7a16e72 100644
--- a/src/gallium/drivers/nouveau/nv30/nv30_screen.c
+++ b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
@@ -319,8 +319,25 @@ nv30_screen_is_format_supported(struct pipe_screen
*pscreen,
                                 unsigned sample_count,
                                 unsigned bindings)
 {
+   /*
+    * msaa use on nv30 may trigger a (mesa?) bug where dmesg says:
+    * [ 1197.850642] nouveau E[soffice.bin[3785]] fail ttm_validate
+    * [ 1197.850648] nouveau E[soffice.bin[3785]] validating bo list
+    * [ 1197.850654] nouveau E[soffice.bin[3785]] validate: -12
+    * [ 1201.766955] nouveau E[soffice.bin[3785]] fail ttm_validate
+    * [ 1201.766961] nouveau E[soffice.bin[3785]] validating bo list
+    * [ 1201.766968] nouveau E[soffice.bin[3785]] validate: -12
+    *
+    * After which the program using the msaa visual freezes, and eventually
+    * the entire system freezes. Disable msaa until this is fixed.
+    */
+#if 1
+   if (sample_count > 0)
+#else
    if (sample_count > 4)
+#endif
       return false;
+
    if (!(0x00000017 & (1 << sample_count)))
       return false;
 
-- 
2.4.3

Yeah, I noticed this was odd too when looking over the code earlier.
Glad you picked up on that as well.

Reviewed-by: Ilia Mirkin <imirkin at alum.mit.edu>
Cc: "10.6 11.0" <mesa-stable at lists.freedesktop.org>

On Mon, Sep 7, 2015 at 3:50 PM, Hans de Goede <hdegoede at redhat.com>
wrote:
> The sifm object has a limit of 1024x1024 for its input size and 2048x2048
> for its output. The code checking this was trying to be clever resulting
> in it seeing a surface of e.g 1024x256 being outside of the input size
> limit.
>
> This commit fixes this.
>
> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> ---
>  src/gallium/drivers/nouveau/nv30/nv30_transfer.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/gallium/drivers/nouveau/nv30/nv30_transfer.c
b/src/gallium/drivers/nouveau/nv30/nv30_transfer.c
> index 214da65..2452071 100644
> --- a/src/gallium/drivers/nouveau/nv30/nv30_transfer.c
> +++ b/src/gallium/drivers/nouveau/nv30/nv30_transfer.c
> @@ -371,7 +371,7 @@ nv30_transfer_rect_blit(XFER_ARGS)
>  static bool
>  nv30_transfer_sifm(XFER_ARGS)
>  {
> -   if (!src->pitch || (src->w | src->h) > 1024 || src->w
< 2 || src->h < 2)
> +   if (!src->pitch || src->w > 1024 || src->h > 1024 ||
src->w < 2 || src->h < 2)
>        return false;
>
>     if (src->d > 1 || dst->d > 1)
> @@ -381,7 +381,7 @@ nv30_transfer_sifm(XFER_ARGS)
>        return false;
>
>     if (!dst->pitch) {
> -      if ((dst->w | dst->h) > 2048 || dst->w < 2 ||
dst->h < 2)
> +      if (dst->w > 2048 || dst->h > 2048 || dst->w < 2
|| dst->h < 2)
>           return false;
>     } else {
>        if (dst->domain != NOUVEAU_BO_VRAM)
> --
> 2.4.3
>

May I ask why you're doing 512x512 instead of 1024x1024? These are
already scaled up coordinates, so 1024x1024 should work no? Or is it
because of the seams on the edges? Do those not also appear with
512x512 or does it sample outside of the box?

Separately, why not use this approach on nv40 as well? I can't imagine
the blitter would be faster... does this result in lower quality?

On Mon, Sep 7, 2015 at 3:50 PM, Hans de Goede <hdegoede at redhat.com>
wrote:
> We do not have a generic blitter on nv3x cards, so we must use the
> sifm object for color resolving.
>
> This commit divides the sources and dest surfaces in to tiles which
> match the constraints of the sifm object, so that color resolving
> will work properly on nv3x cards.
>
> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> ---
>  src/gallium/drivers/nouveau/nv30/nv30_miptree.c | 55
++++++++++++++++++++++++-
>  1 file changed, 54 insertions(+), 1 deletion(-)
>
> diff --git a/src/gallium/drivers/nouveau/nv30/nv30_miptree.c
b/src/gallium/drivers/nouveau/nv30/nv30_miptree.c
> index 76bb8b8..c240030 100644
> --- a/src/gallium/drivers/nouveau/nv30/nv30_miptree.c
> +++ b/src/gallium/drivers/nouveau/nv30/nv30_miptree.c
> @@ -149,6 +149,56 @@ static void
>  nv30_resource_resolve(struct nv30_context *nv30,
>                        const struct pipe_blit_info *info)
>  {
> +   struct nv30_miptree *src_mt = nv30_miptree(info->src.resource);
> +   struct nv30_rect src, dst;
> +   unsigned x, x0, x1, y, y1, w, h;
> +
> +   define_rect(info->src.resource, 0, info->src.box.z,
info->src.box.x,
> +      info->src.box.y, info->src.box.width, info->src.box.height,
&src);
> +   define_rect(info->dst.resource, 0, info->dst.box.z,
info->dst.box.x,
> +      info->dst.box.y, info->dst.box.width, info->dst.box.height,
&dst);
> +
> +   x0 = src.x0;
> +   x1 = src.x1;
> +   y1 = src.y1;
> +
> +   /* On nv3x we must use sifm which is restricted to 512x512 tiles */
> +   for (y = src.y0; y < y1; y += h) {
> +      h = y1 - y;
> +      if (h > 512)
> +         h = 512;
> +
> +      src.y0 = 0;
> +      src.y1 = h;
> +      src.h = h;
> +
> +      dst.y1 = dst.y0 + (h >> src_mt->ms_y);
> +      dst.h = h >> src_mt->ms_y;
> +
> +      for (x = x0; x < x1; x += w) {
> +         w = x1 - x;
> +         if (w > 512)
> +            w = 512;
> +
> +         src.offset = y * src.pitch + x * src.cpp;
> +         src.x0 = 0;
> +         src.x1 = w;
> +         src.w = w;
> +
> +         dst.offset = (y >> src_mt->ms_y) * dst.pitch +
> +                      (x >> src_mt->ms_x) * dst.cpp;
> +         dst.x1 = dst.x0 + (w >> src_mt->ms_x);
> +         dst.w = w >> src_mt->ms_x;
> +
> +         nv30_transfer_rect(nv30, BILINEAR, &src, &dst);
> +      }
> +   }
> +}
> +
> +static void
> +nv40_resource_resolve(struct nv30_context *nv30,
> +                      const struct pipe_blit_info *info)
> +{
>     struct nv30_rect src, dst;
>
>     define_rect(info->src.resource, 0, info->src.box.z,
info->src.box.x,
> @@ -170,7 +220,10 @@ nv30_blit(struct pipe_context *pipe,
>         info.dst.resource->nr_samples <= 1 &&
>         !util_format_is_depth_or_stencil(info.src.resource->format)
&&
>         !util_format_is_pure_integer(info.src.resource->format)) {
> -      nv30_resource_resolve(nv30, blit_info);
> +      if (nv30->screen->eng3d->oclass >= NV40_3D_CLASS)
> +         nv40_resource_resolve(nv30, blit_info);
> +      else
> +         nv30_resource_resolve(nv30, blit_info);
>        return;
>     }
>
> --
> 2.4.3
>

On Mon, Sep 7, 2015 at 3:50 PM, Hans de Goede <hdegoede at redhat.com>
wrote:
> msaa use on nv30 may trigger a (mesa?) bug where dmesg says:
>  [ 1197.850642] nouveau E[soffice.bin[3785]] fail ttm_validate
>  [ 1197.850648] nouveau E[soffice.bin[3785]] validating bo list
>  [ 1197.850654] nouveau E[soffice.bin[3785]] validate: -12
>  [ 1201.766955] nouveau E[soffice.bin[3785]] fail ttm_validate
>  [ 1201.766961] nouveau E[soffice.bin[3785]] validating bo list
>  [ 1201.766968] nouveau E[soffice.bin[3785]] validate: -12
>
> After which the program using the msaa visual freezes, and eventually
> the entire system freezes. Disable msaa until this is fixed.
>
> This happens on both nv3x and nv4x cards.
Ugh. This is aka "you ran out of vram, goodbye". We don't really
handle that case extremely well. I feel really bad doing this :( The
issue is anachronistic applications like soffice that don't keep with
the limitations of GPUs of the days of yore. So we end up penalizing
people who do use applications of the day.

But the practical issue is that people do upgrade, and people do run
these applications, and so it makes sense to keep it off by default.
Could I convince you to use debug_get_int_option (or something along
those lines, forget the function name) to still allow an env var
override? Like NV30_MAX_MSAA or something (and clamp it to 4 so people
don't get ideas).
>
> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> ---
>  src/gallium/drivers/nouveau/nv30/nv30_screen.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/src/gallium/drivers/nouveau/nv30/nv30_screen.c
b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
> index 7aad26b..7a16e72 100644
> --- a/src/gallium/drivers/nouveau/nv30/nv30_screen.c
> +++ b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
> @@ -319,8 +319,25 @@ nv30_screen_is_format_supported(struct pipe_screen
*pscreen,
>                                  unsigned sample_count,
>                                  unsigned bindings)
>  {
> +   /*
> +    * msaa use on nv30 may trigger a (mesa?) bug where dmesg says:
> +    * [ 1197.850642] nouveau E[soffice.bin[3785]] fail ttm_validate
> +    * [ 1197.850648] nouveau E[soffice.bin[3785]] validating bo list
> +    * [ 1197.850654] nouveau E[soffice.bin[3785]] validate: -12
> +    * [ 1201.766955] nouveau E[soffice.bin[3785]] fail ttm_validate
> +    * [ 1201.766961] nouveau E[soffice.bin[3785]] validating bo list
> +    * [ 1201.766968] nouveau E[soffice.bin[3785]] validate: -12
> +    *
> +    * After which the program using the msaa visual freezes, and
eventually
> +    * the entire system freezes. Disable msaa until this is fixed.
> +    */
> +#if 1
> +   if (sample_count > 0)
> +#else
>     if (sample_count > 4)
> +#endif
>        return false;
> +
>     if (!(0x00000017 & (1 << sample_count)))
>        return false;
>
> --
> 2.4.3
>