[Mesa-dev] [PATCH 3/3] llvmpipe: increase number of subpixel bits to eight

[Roland Scheidegger]

Am 19.09.2013 20:43, schrieb Zack Rusin:
> Unfortunately d3d10 requires a lot higher precision (e.g.
> wgf11clipping tests for it). The smallest number of precision
> bits with which it passes is 8. That means that we need to
> decrease the maximum length of an edge that we can handle without
> subdivision by 4 bits. Abstracted the code a bit to make it easier
> to change once to switch to 64bit rasterization.
> 
> Signed-off-by: Zack Rusin <zackr at vmware.com>
> ---
>  src/gallium/drivers/llvmpipe/lp_rast.h      | 12 +++++++++++-
>  src/gallium/drivers/llvmpipe/lp_setup.c     | 14 +++++---------
>  src/gallium/drivers/llvmpipe/lp_setup_tri.c |  2 +-
>  3 files changed, 17 insertions(+), 11 deletions(-)
> 
> diff --git a/src/gallium/drivers/llvmpipe/lp_rast.h b/src/gallium/drivers/llvmpipe/lp_rast.h
> index c57f2ea..b72be55 100644
> --- a/src/gallium/drivers/llvmpipe/lp_rast.h
> +++ b/src/gallium/drivers/llvmpipe/lp_rast.h
> @@ -46,10 +46,20 @@ struct lp_scene;
>  struct lp_fence;
>  struct cmd_bin;
>  
> +#define FIXED_TYPE_WIDTH 32
>  /** For sub-pixel positioning */
> -#define FIXED_ORDER 4
> +#define FIXED_ORDER 8
>  #define FIXED_ONE (1<<FIXED_ORDER)
>  
> +/** Maximum length of an edge in a primitive in pixels.
> + *  If the framebuffer is large we have to think about fixed-point
> + *  integer overflow.  Coordinates need ((FIXED_TYPE_WIDTH/2) - 1) bits
> + *  to be able to fit product of two such coordinates inside 
> + *  FIXED_TYPE_WIDTH, any larger and we could overflow a 
> + *  FIXED_TYPE_WIDTH_-bit int.
> + */
> +#define MAX_FIXED_LENGTH (1 << (((FIXED_TYPE_WIDTH/2) - 1) - FIXED_ORDER))
> +
>  /* Rasterizer output size going to jit fs, width/height */
>  #define LP_RASTER_BLOCK_SIZE 4
>  
> diff --git a/src/gallium/drivers/llvmpipe/lp_setup.c b/src/gallium/drivers/llvmpipe/lp_setup.c
> index 5fde01f..edb55ad 100644
> --- a/src/gallium/drivers/llvmpipe/lp_setup.c
> +++ b/src/gallium/drivers/llvmpipe/lp_setup.c
> @@ -1007,16 +1007,12 @@ try_update_scene_state( struct lp_setup_context *setup )
>                                           &setup->draw_regions[i]);
>           }
>        }
> -      /* If the framebuffer is large we have to think about fixed-point
> -       * integer overflow.  For 2K by 2K images, coordinates need 15 bits
> -       * (2^11 + 4 subpixel bits).  The product of two such numbers would
> -       * use 30 bits.  Any larger and we could overflow a 32-bit int.
> -       *
> -       * To cope with this problem we check if triangles are large and
> -       * subdivide them if needed.
> +      /* 
> +       * Subdivide triangles if the framebuffer is larger than our 
> +       * MAX_FIXED_LENGTH cab accomodate.
can
Also I think this should read more like it was, "Check if subdivision of
triangles is needed if the framebuffer..." (because if that's enabled it
won't just subdivide every triangle though there's quite a lot of math
to figure out if subdivision is actually needed). Not a biggie though.


>         */
> -      setup->subdivide_large_triangles = (setup->fb.width > 2048 &&
> -                                          setup->fb.height > 2048);
> +      setup->subdivide_large_triangles = (setup->fb.width > MAX_FIXED_LENGTH &&
> +                                          setup->fb.height > MAX_FIXED_LENGTH);
>     }
>                                        
>     setup->dirty = 0;
> diff --git a/src/gallium/drivers/llvmpipe/lp_setup_tri.c b/src/gallium/drivers/llvmpipe/lp_setup_tri.c
> index e61efd4..ee30a64 100644
> --- a/src/gallium/drivers/llvmpipe/lp_setup_tri.c
> +++ b/src/gallium/drivers/llvmpipe/lp_setup_tri.c
> @@ -988,7 +988,7 @@ check_subdivide_triangle(struct lp_setup_context *setup,
>                           const float (*v2)[4],
>                           triangle_func_t tri)
>  {
> -   const float maxLen = 2048.0f;  /* longest permissible edge, in pixels */
> +   const float maxLen = MAX_FIXED_LENGTH;  /* longest permissible edge, in pixels */
>     float dx10, dy10, len10;
>     float dx21, dy21, len21;
>     float dx02, dy02, len02;
> 

Otherwise looks good to me. Though I suspect we really need to fix the
rasterizer, as those tiny maximum 128x128 chunks are going to hammer the
subdivision code (two tris covering a 2048x2048 screen are going to
generate some _thousands_ tris if I see that right - actually I think
subdivision is too pessimistic since it will break up a tri with coords
0/0,0/128,128/0 as one of the edges is longer than 128 but I believe it
would not be necessary as long as each x/y component of any edge is not
above 128, that's at least what I think because up to now we wouldn't
have considered splitting with a fb size of 2048 (without errors?)
whereas such a fb-matching tri would have triggered subdivision if enabled).

Roland