Re: [Mesa-dev] [PATCH 1/2] draw: simplify (and correct) aaline fallback (v2)

2018-03-07 Thread Roland Scheidegger 
Am 07.03.2018 um 11:34 schrieb Jose Fonseca:
> On 06/03/18 20:52, Brian Paul wrote:
>> Looks good.  That certainly does simplify things.  Two minor
>> suggestions below.
>>
>> In any case, for both, Reviewed-by: Brian Paul 
> 
> Looks great cleanup to me too.  Just minor issues inline. Both are
> 
> Reviewed-by: Jose Fonseca 
>>
>> On 03/06/2018 01:34 PM, srol...@vmware.com wrote:
>>> From: Roland Scheidegger 
>>>
>>> The motivation actually was to get rid of the additional tex
>>> instruction, since that requires the draw fallback code to intercept
>>> all sampler / view calls (even if the fallback is never hit).
>>> Basically, the idea is to use coverage of the pixel to calculate
>>> the alpha value, and coverage is simply based on the distance
>>> to the center of the line (in both line direction, which is useful
>>> for wide lines, as well as perpendicular to the line).
>>> This is much closer to what hw supporting this natively actually does.
>>> It also fixes an issue with line width not quite being correct, as
>>> well as endpoints getting stretched too far (in line direction) with
>>> wide lines, which is apparent with mesa demo line-width.
>>> (For llvmpipe, it would probably make sense to do something like this
>>> directly when drawing lines, since rendering two tris is twice as
>>> expensive as a line, but it would need some changes with state
>>> management.)
>>> Since we're no longer relying on mipmapping to get the alpha value,
>>> we also don't need to draw 3 rects (6 tris), one is sufficient.
>>>
>>> There's still issues (as before):
>>> - quite sure it's not correct without half_pixel_center, but can't test
>>> this with GL.
>>> - aaline + line stipple is incorrect (evident with line-width demo).
>>> Looking at the spec the stipple pattern should actually be based on
>>> distance (not just dx or dy for x/y major lines as without aa).
>>> - outputs (other than pos + the one used for line aa) should be
>>> reinterpolated since we actually increase line length by half a pixel
>>> (but there's no tests which would care).
>>>
>>> v2: simplify the math (should be equivalent), don't need immediate
>>> ---
>>>   src/gallium/auxiliary/draw/draw_pipe_aaline.c | 504
>>> +-
>>>   1 file changed, 100 insertions(+), 404 deletions(-)
>>>
>>> + aactx->tempsUsed |= (1ULL << i);
> UINT64_C(1)
Generally, this notation isn't used in mesa. Ok that's not entirely
true, but 1ULL outnumbers UINT64_C(1) by about 50 : 1. (There's exactly
5 uses of UINT64_C in mesa, among those 3 unique ones the remaining 2
are due to copy/paste.)
I guess UINT64_C() is technically better because ULL might not be 64bit,
but that assumption is so wide spread I'm not sure it's worth bothering
anymore...


>>>   -   /* XXX the ends of lines aren't quite perfect yet, but probably
>>> passable */
>>> -   dx = 0.5F * half_width;
>>> -   dy = half_width;
>>> +   half_length = sqrtf(dx * dx + dy * dy) / 2.0f + 0.5f;
> 
> 0.5f * ... ?
Ok. (Not that it would make a difference.)


>>> -   tex = v[2]->data[texPos];
>>> -   ASSIGN_4V(tex, .5, 0, 0, 1);
>>> +   tex = v[0]->data[coordPos];
>>> +   /* XXX need to adjust for pixel center? */
> 
> You could just check for pixel_center and write an warning.
Will do.

Roland
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/2] draw: simplify (and correct) aaline fallback (v2)

2018-03-07 Thread Jose Fonseca 

On 06/03/18 20:52, Brian Paul wrote:
Looks good.  That certainly does simplify things.  Two minor 
suggestions below.


In any case, for both, Reviewed-by: Brian Paul 


Looks great cleanup to me too.  Just minor issues inline. Both are

Reviewed-by: Jose Fonseca 


On 03/06/2018 01:34 PM, srol...@vmware.com wrote:

From: Roland Scheidegger 

The motivation actually was to get rid of the additional tex
instruction, since that requires the draw fallback code to intercept
all sampler / view calls (even if the fallback is never hit).
Basically, the idea is to use coverage of the pixel to calculate
the alpha value, and coverage is simply based on the distance
to the center of the line (in both line direction, which is useful
for wide lines, as well as perpendicular to the line).
This is much closer to what hw supporting this natively actually does.
It also fixes an issue with line width not quite being correct, as
well as endpoints getting stretched too far (in line direction) with
wide lines, which is apparent with mesa demo line-width.
(For llvmpipe, it would probably make sense to do something like this
directly when drawing lines, since rendering two tris is twice as
expensive as a line, but it would need some changes with state
management.)
Since we're no longer relying on mipmapping to get the alpha value,
we also don't need to draw 3 rects (6 tris), one is sufficient.

There's still issues (as before):
- quite sure it's not correct without half_pixel_center, but can't test
this with GL.
- aaline + line stipple is incorrect (evident with line-width demo).
Looking at the spec the stipple pattern should actually be based on
distance (not just dx or dy for x/y major lines as without aa).
- outputs (other than pos + the one used for line aa) should be
reinterpolated since we actually increase line length by half a pixel
(but there's no tests which would care).

v2: simplify the math (should be equivalent), don't need immediate
---
  src/gallium/auxiliary/draw/draw_pipe_aaline.c | 504 
+-

  1 file changed, 100 insertions(+), 404 deletions(-)

diff --git a/src/gallium/auxiliary/draw/draw_pipe_aaline.c 
b/src/gallium/auxiliary/draw/draw_pipe_aaline.c

index a859dbc..591e2a3 100644
--- a/src/gallium/auxiliary/draw/draw_pipe_aaline.c
+++ b/src/gallium/auxiliary/draw/draw_pipe_aaline.c
@@ -1,6 +1,6 @@
/**
   *
- * Copyright 2007 VMware, Inc.
+ * Copyright 2007-2018 VMware, Inc.
   * All Rights Reserved.
   *
   * Permission is hereby granted, free of charge, to any person 
obtaining a

@@ -26,7 +26,7 @@
**/
    /**
- * AA line stage:  AA lines are converted to texture mapped triangles.
+ * AA line stage:  AA lines are converted triangles (with extra 
generic)

   *
   * Authors:  Brian Paul
   */
@@ -40,7 +40,6 @@
  #include "util/u_format.h"
  #include "util/u_math.h"
  #include "util/u_memory.h"
-#include "util/u_sampler.h"
    #include "tgsi/tgsi_transform.h"
  #include "tgsi/tgsi_dump.h"
@@ -55,19 +54,6 @@
      /**
- * Size for the alpha texture used for antialiasing
- */
-#define TEXTURE_SIZE_LOG2  5   /* 32 x 32 */
-
-/**
- * Max texture level for the alpha texture used for antialiasing
- *
- * Don't use the 1x1 and 2x2 mipmap levels.
- */
-#define MAX_TEXTURE_LEVEL  (TEXTURE_SIZE_LOG2 - 2)
-
-
-/**
   * Subclass of pipe_shader_state to carry extra fragment shader info.
   */
  struct aaline_fragment_shader
@@ -75,8 +61,7 @@ struct aaline_fragment_shader
 struct pipe_shader_state state;
 void *driver_fs;
 void *aaline_fs;
-   uint sampler_unit;
-   int generic_attrib;  /**< texcoord/generic used for texture */
+   int generic_attrib;  /**< generic used for distance */
  };
    @@ -89,26 +74,16 @@ struct aaline_stage
   float half_line_width;
  -   /** For AA lines, this is the vertex attrib slot for the new 
texcoords */

-   uint tex_slot;
+   /** For AA lines, this is the vertex attrib slot for new generic */
+   uint coord_slot;
 /** position, not necessarily output zero */
 uint pos_slot;
  -   void *sampler_cso;
-   struct pipe_resource *texture;
-   struct pipe_sampler_view *sampler_view;
-   uint num_samplers;
-   uint num_sampler_views;
-
   /*
  * Currently bound state
  */
 struct aaline_fragment_shader *fs;
-   struct {
-  void *sampler[PIPE_MAX_SAMPLERS];
-  struct pipe_sampler_view 
*sampler_views[PIPE_MAX_SHADER_SAMPLER_VIEWS];

-   } state;
   /*
  * Driver interface/override functions
@@ -117,15 +92,6 @@ struct aaline_stage
  const struct pipe_shader_state *);
 void (*driver_bind_fs_state)(struct pipe_context *, void *);
 void (*driver_delete_fs_state)(struct pipe_context *, void *);
-
-   void (*driver_bind_sampler_states)(struct pipe_context *,
-

Re: [Mesa-dev] [PATCH 1/2] draw: simplify (and correct) aaline fallback (v2)

2018-03-06 Thread Roland Scheidegger 
Am 06.03.2018 um 21:52 schrieb Brian Paul:
> Looks good.  That certainly does simplify things.  Two minor suggestions
> below.
> 
> In any case, for both, Reviewed-by: Brian Paul 
> 
> On 03/06/2018 01:34 PM, srol...@vmware.com wrote:
>> From: Roland Scheidegger 
>>
>> The motivation actually was to get rid of the additional tex
>> instruction, since that requires the draw fallback code to intercept
>> all sampler / view calls (even if the fallback is never hit).
>> Basically, the idea is to use coverage of the pixel to calculate
>> the alpha value, and coverage is simply based on the distance
>> to the center of the line (in both line direction, which is useful
>> for wide lines, as well as perpendicular to the line).
>> This is much closer to what hw supporting this natively actually does.
>> It also fixes an issue with line width not quite being correct, as
>> well as endpoints getting stretched too far (in line direction) with
>> wide lines, which is apparent with mesa demo line-width.
>> (For llvmpipe, it would probably make sense to do something like this
>> directly when drawing lines, since rendering two tris is twice as
>> expensive as a line, but it would need some changes with state
>> management.)
>> Since we're no longer relying on mipmapping to get the alpha value,
>> we also don't need to draw 3 rects (6 tris), one is sufficient.
>>
>> There's still issues (as before):
>> - quite sure it's not correct without half_pixel_center, but can't test
>> this with GL.
>> - aaline + line stipple is incorrect (evident with line-width demo).
>> Looking at the spec the stipple pattern should actually be based on
>> distance (not just dx or dy for x/y major lines as without aa).
>> - outputs (other than pos + the one used for line aa) should be
>> reinterpolated since we actually increase line length by half a pixel
>> (but there's no tests which would care).
>>
>> v2: simplify the math (should be equivalent), don't need immediate
>> ---
>>   src/gallium/auxiliary/draw/draw_pipe_aaline.c | 504
>> +-
>>   1 file changed, 100 insertions(+), 404 deletions(-)
>>



>> diff --git a/src/gallium/auxiliary/draw/draw_pipe_aaline.c
>> b/src/gallium/auxiliary/draw/draw_pipe_aaline.c
>> index a859dbc..591e2a3 100644
>> @@ -210,49 +155,29 @@ aa_transform_prolog(struct
>> tgsi_transform_context *ctx)
>>  struct aa_transform_context *aactx = (struct aa_transform_context
>> *) ctx;
>>  uint i;
>>   -   STATIC_ASSERT(sizeof(aactx->samplersUsed) * 8 >=
>> PIPE_MAX_SAMPLERS);
>> -
>> -   /* find free sampler */
>> -   aactx->freeSampler = free_bit(aactx->samplersUsed);
>> -   if (aactx->freeSampler < 0 || aactx->freeSampler >=
>> PIPE_MAX_SAMPLERS)
>> -  aactx->freeSampler = PIPE_MAX_SAMPLERS - 1;
>>    /* find two free temp regs */
>> -   for (i = 0; i < 32; i++) {
>> +   for (i = 0; i < 64; i++) {
>>     if ((aactx->tempsUsed & (1 << i)) == 0) {
>>     /* found a free temp */
>>     if (aactx->colorTemp < 0)
>>    aactx->colorTemp  = i;
>> -  else if (aactx->texTemp < 0)
>> - aactx->texTemp  = i;
>> +  else if (aactx->aaTemp < 0)
>> + aactx->aaTemp  = i;
>>     else
>>    break;
>>     }
>>  }
> 
> The loop could probably be eliminated and replaced with a couple
> ffsll(~tempsUsed) calls.
Right.


>>   @@ -266,13 +191,41 @@ aa_transform_epilog(struct
>> tgsi_transform_context *ctx)
>>  struct aa_transform_context *aactx = (struct aa_transform_context
>> *) ctx;
>>    if (aactx->colorOutput != -1) {
>> -  /* insert texture sampling code for antialiasing. */
>> -
>> -  /* TEX texTemp, input_coord, sampler, 2D */
>> -  tgsi_transform_tex_inst(ctx,
>> -  TGSI_FILE_TEMPORARY, aactx->texTemp,
>> -  TGSI_FILE_INPUT, aactx->maxInput + 1,
>> -  TGSI_TEXTURE_2D, aactx->freeSampler);
>> +  struct tgsi_full_instruction inst;
>> +  /* insert distance-based coverage code for antialiasing. */
>> +
>> +  /* saturate(linewidth - fabs(interpx), linelength -
>> fabs(interpz) */
>> +  inst = tgsi_default_full_instruction();
>> +  inst.Instruction.Saturate = true;
>> +  inst.Instruction.Opcode = TGSI_OPCODE_ADD;
>> +  inst.Instruction.NumDstRegs = 1;
>> +  tgsi_transform_dst_reg([0], TGSI_FILE_TEMPORARY,
>> + aactx->aaTemp, TGSI_WRITEMASK_XZ);
>> +  inst.Instruction.NumSrcRegs = 2;
>> +  tgsi_transform_src_reg([1], TGSI_FILE_INPUT,
>> aactx->maxInput + 1,
>> + TGSI_SWIZZLE_X, TGSI_SWIZZLE_X,
>> + TGSI_SWIZZLE_Z, TGSI_SWIZZLE_Z);
>> +  tgsi_transform_src_reg([0], TGSI_FILE_INPUT,
>> aactx->maxInput + 1,
>> + TGSI_SWIZZLE_Y, TGSI_SWIZZLE_Y,
>> + TGSI_SWIZZLE_W, TGSI_SWIZZLE_W);
>> +  inst.Src[1].Register.Absolute =

Re: [Mesa-dev] [PATCH 1/2] draw: simplify (and correct) aaline fallback (v2)

2018-03-06 Thread Brian Paul 
Looks good.  That certainly does simplify things.  Two minor suggestions 
below.


In any case, for both, Reviewed-by: Brian Paul 

On 03/06/2018 01:34 PM, srol...@vmware.com wrote:

From: Roland Scheidegger 

The motivation actually was to get rid of the additional tex
instruction, since that requires the draw fallback code to intercept
all sampler / view calls (even if the fallback is never hit).
Basically, the idea is to use coverage of the pixel to calculate
the alpha value, and coverage is simply based on the distance
to the center of the line (in both line direction, which is useful
for wide lines, as well as perpendicular to the line).
This is much closer to what hw supporting this natively actually does.
It also fixes an issue with line width not quite being correct, as
well as endpoints getting stretched too far (in line direction) with
wide lines, which is apparent with mesa demo line-width.
(For llvmpipe, it would probably make sense to do something like this
directly when drawing lines, since rendering two tris is twice as
expensive as a line, but it would need some changes with state
management.)
Since we're no longer relying on mipmapping to get the alpha value,
we also don't need to draw 3 rects (6 tris), one is sufficient.

There's still issues (as before):
- quite sure it's not correct without half_pixel_center, but can't test
this with GL.
- aaline + line stipple is incorrect (evident with line-width demo).
Looking at the spec the stipple pattern should actually be based on
distance (not just dx or dy for x/y major lines as without aa).
- outputs (other than pos + the one used for line aa) should be
reinterpolated since we actually increase line length by half a pixel
(but there's no tests which would care).

v2: simplify the math (should be equivalent), don't need immediate
---
  src/gallium/auxiliary/draw/draw_pipe_aaline.c | 504 +-
  1 file changed, 100 insertions(+), 404 deletions(-)

diff --git a/src/gallium/auxiliary/draw/draw_pipe_aaline.c 
b/src/gallium/auxiliary/draw/draw_pipe_aaline.c
index a859dbc..591e2a3 100644
--- a/src/gallium/auxiliary/draw/draw_pipe_aaline.c
+++ b/src/gallium/auxiliary/draw/draw_pipe_aaline.c
@@ -1,6 +1,6 @@
  /**
   *
- * Copyright 2007 VMware, Inc.
+ * Copyright 2007-2018 VMware, Inc.
   * All Rights Reserved.
   *
   * Permission is hereby granted, free of charge, to any person obtaining a
@@ -26,7 +26,7 @@
   **/
  
  /**

- * AA line stage:  AA lines are converted to texture mapped triangles.
+ * AA line stage:  AA lines are converted triangles (with extra generic)
   *
   * Authors:  Brian Paul
   */
@@ -40,7 +40,6 @@
  #include "util/u_format.h"
  #include "util/u_math.h"
  #include "util/u_memory.h"
-#include "util/u_sampler.h"
  
  #include "tgsi/tgsi_transform.h"

  #include "tgsi/tgsi_dump.h"
@@ -55,19 +54,6 @@
  
  
  /**

- * Size for the alpha texture used for antialiasing
- */
-#define TEXTURE_SIZE_LOG2  5   /* 32 x 32 */
-
-/**
- * Max texture level for the alpha texture used for antialiasing
- *
- * Don't use the 1x1 and 2x2 mipmap levels.
- */
-#define MAX_TEXTURE_LEVEL  (TEXTURE_SIZE_LOG2 - 2)
-
-
-/**
   * Subclass of pipe_shader_state to carry extra fragment shader info.
   */
  struct aaline_fragment_shader
@@ -75,8 +61,7 @@ struct aaline_fragment_shader
 struct pipe_shader_state state;
 void *driver_fs;
 void *aaline_fs;
-   uint sampler_unit;
-   int generic_attrib;  /**< texcoord/generic used for texture */
+   int generic_attrib;  /**< generic used for distance */
  };
  
  
@@ -89,26 +74,16 @@ struct aaline_stage
  
 float half_line_width;
  
-   /** For AA lines, this is the vertex attrib slot for the new texcoords */

-   uint tex_slot;
+   /** For AA lines, this is the vertex attrib slot for new generic */
+   uint coord_slot;
 /** position, not necessarily output zero */
 uint pos_slot;
  
-   void *sampler_cso;

-   struct pipe_resource *texture;
-   struct pipe_sampler_view *sampler_view;
-   uint num_samplers;
-   uint num_sampler_views;
-
  
 /*

  * Currently bound state
  */
 struct aaline_fragment_shader *fs;
-   struct {
-  void *sampler[PIPE_MAX_SAMPLERS];
-  struct pipe_sampler_view *sampler_views[PIPE_MAX_SHADER_SAMPLER_VIEWS];
-   } state;
  
 /*

  * Driver interface/override functions
@@ -117,15 +92,6 @@ struct aaline_stage
  const struct pipe_shader_state *);
 void (*driver_bind_fs_state)(struct pipe_context *, void *);
 void (*driver_delete_fs_state)(struct pipe_context *, void *);
-
-   void (*driver_bind_sampler_states)(struct pipe_context *,
-  enum pipe_shader_type, unsigned,
-  unsigned, void **);
-
-   void

[Mesa-dev] [PATCH 1/2] draw: simplify (and correct) aaline fallback (v2)

2018-03-06 Thread sroland 
From: Roland Scheidegger 

The motivation actually was to get rid of the additional tex
instruction, since that requires the draw fallback code to intercept
all sampler / view calls (even if the fallback is never hit).
Basically, the idea is to use coverage of the pixel to calculate
the alpha value, and coverage is simply based on the distance
to the center of the line (in both line direction, which is useful
for wide lines, as well as perpendicular to the line).
This is much closer to what hw supporting this natively actually does.
It also fixes an issue with line width not quite being correct, as
well as endpoints getting stretched too far (in line direction) with
wide lines, which is apparent with mesa demo line-width.
(For llvmpipe, it would probably make sense to do something like this
directly when drawing lines, since rendering two tris is twice as
expensive as a line, but it would need some changes with state
management.)
Since we're no longer relying on mipmapping to get the alpha value,
we also don't need to draw 3 rects (6 tris), one is sufficient.

There's still issues (as before):
- quite sure it's not correct without half_pixel_center, but can't test
this with GL.
- aaline + line stipple is incorrect (evident with line-width demo).
Looking at the spec the stipple pattern should actually be based on
distance (not just dx or dy for x/y major lines as without aa).
- outputs (other than pos + the one used for line aa) should be
reinterpolated since we actually increase line length by half a pixel
(but there's no tests which would care).

v2: simplify the math (should be equivalent), don't need immediate
---
 src/gallium/auxiliary/draw/draw_pipe_aaline.c | 504 +-
 1 file changed, 100 insertions(+), 404 deletions(-)

diff --git a/src/gallium/auxiliary/draw/draw_pipe_aaline.c 
b/src/gallium/auxiliary/draw/draw_pipe_aaline.c
index a859dbc..591e2a3 100644
--- a/src/gallium/auxiliary/draw/draw_pipe_aaline.c
+++ b/src/gallium/auxiliary/draw/draw_pipe_aaline.c
@@ -1,6 +1,6 @@
 /**
  *
- * Copyright 2007 VMware, Inc.
+ * Copyright 2007-2018 VMware, Inc.
  * All Rights Reserved.
  *
  * Permission is hereby granted, free of charge, to any person obtaining a
@@ -26,7 +26,7 @@
  **/
 
 /**
- * AA line stage:  AA lines are converted to texture mapped triangles.
+ * AA line stage:  AA lines are converted triangles (with extra generic)
  *
  * Authors:  Brian Paul
  */
@@ -40,7 +40,6 @@
 #include "util/u_format.h"
 #include "util/u_math.h"
 #include "util/u_memory.h"
-#include "util/u_sampler.h"
 
 #include "tgsi/tgsi_transform.h"
 #include "tgsi/tgsi_dump.h"
@@ -55,19 +54,6 @@
 
 
 /**
- * Size for the alpha texture used for antialiasing
- */
-#define TEXTURE_SIZE_LOG2  5   /* 32 x 32 */
-
-/**
- * Max texture level for the alpha texture used for antialiasing
- *
- * Don't use the 1x1 and 2x2 mipmap levels.
- */
-#define MAX_TEXTURE_LEVEL  (TEXTURE_SIZE_LOG2 - 2)
-
-
-/**
  * Subclass of pipe_shader_state to carry extra fragment shader info.
  */
 struct aaline_fragment_shader
@@ -75,8 +61,7 @@ struct aaline_fragment_shader
struct pipe_shader_state state;
void *driver_fs;
void *aaline_fs;
-   uint sampler_unit;
-   int generic_attrib;  /**< texcoord/generic used for texture */
+   int generic_attrib;  /**< generic used for distance */
 };
 
 
@@ -89,26 +74,16 @@ struct aaline_stage
 
float half_line_width;
 
-   /** For AA lines, this is the vertex attrib slot for the new texcoords */
-   uint tex_slot;
+   /** For AA lines, this is the vertex attrib slot for new generic */
+   uint coord_slot;
/** position, not necessarily output zero */
uint pos_slot;
 
-   void *sampler_cso;
-   struct pipe_resource *texture;
-   struct pipe_sampler_view *sampler_view;
-   uint num_samplers;
-   uint num_sampler_views;
-
 
/*
 * Currently bound state
 */
struct aaline_fragment_shader *fs;
-   struct {
-  void *sampler[PIPE_MAX_SAMPLERS];
-  struct pipe_sampler_view *sampler_views[PIPE_MAX_SHADER_SAMPLER_VIEWS];
-   } state;
 
/*
 * Driver interface/override functions
@@ -117,15 +92,6 @@ struct aaline_stage
 const struct pipe_shader_state *);
void (*driver_bind_fs_state)(struct pipe_context *, void *);
void (*driver_delete_fs_state)(struct pipe_context *, void *);
-
-   void (*driver_bind_sampler_states)(struct pipe_context *,
-  enum pipe_shader_type, unsigned,
-  unsigned, void **);
-
-   void (*driver_set_sampler_views)(struct pipe_context *,
-enum pipe_shader_type shader,
-unsigned start, unsigned count,
-struct pipe_sampler_view **);
 };
 
 
@@ -136,41 +102,27