[Mesa-dev] [PATCH] i965: setup address rounding enable bits

[Ian Romanick]

On 10/12/2011 08:34 PM, Yuanhan Liu wrote:
> The patch(based on the reading of the emulator) came from while I was
> trying to fix the oglc pbo texImage.1PBODefaults fail. This case
> generates a texture with the width and height equal to window's width
> and height respectively, then try to texture it on the whole window.
> So, it's exactly one texel for one pixel.  And, the min filter and mag
> filter are GL_LINEAR. It runs with swrast OK, as expected. But it failed
> with i965 driver.
>
> Well, you can't tell the difference from the screen, as the error is
> quite tiny. From my digging, it seems that there are some tiny error
> happened while getting tex address. This will break the one texel for
> one pixel rule in this case. Thus the linear result is taken, with tiny
> error.
>
> This patch would fix several oglc pbo subcase fail on both ILK, SNB and
> IVB.

I don't know about this patch, but this led me to notice something else 
in the docs.  Starting with Sandybridge, bit 27 in dword 0 is "Min and 
Mag State Not Equal."  This is ss0.min_mag_neq in our structure.  The 
hardware docs say that this bit must be set to 0 if the min and mag 
filter state are not the same, min and mag U round modes are not the 
same, min and mag V round modes are not the same, or min and mag R round 
modes are not the same.  I can't see that this bit is set anywhere in 
the driver.

Does adding the following code at about line 227 of brw_wm_sampler.c 
make any additional tests pass on SNB?

    if (intel->gen >= 6)
       sampler->ss0.min_mag_neq =
          sampler->ss0.min_filter != sampler->ss0.mag_filter;

> Signed-off-by: Yuanhan Liu<yuanhan.liu at linux.intel.com>
> ---
>   src/mesa/drivers/dri/i965/brw_defines.h          |    7 +++++++
>   src/mesa/drivers/dri/i965/brw_wm_sampler_state.c |    9 +++++++++
>   src/mesa/drivers/dri/i965/gen7_sampler_state.c   |    9 +++++++++
>   3 files changed, 25 insertions(+), 0 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_defines.h b/src/mesa/drivers/dri/i965/brw_defines.h
> index a111630..e4647ff 100644
> --- a/src/mesa/drivers/dri/i965/brw_defines.h
> +++ b/src/mesa/drivers/dri/i965/brw_defines.h
> @@ -196,6 +196,13 @@
>   #define BRW_MIPFILTER_NEAREST     1
>   #define BRW_MIPFILTER_LINEAR      3
>
> +#define BRW_ADDRESS_ROUNDING_ENABLE_U_MAG	0x20
> +#define BRW_ADDRESS_ROUNDING_ENABLE_U_MIN	0x10
> +#define BRW_ADDRESS_ROUNDING_ENABLE_V_MAG	0x08
> +#define BRW_ADDRESS_ROUNDING_ENABLE_V_MIN	0x04
> +#define BRW_ADDRESS_ROUNDING_ENABLE_R_MAG	0x02
> +#define BRW_ADDRESS_ROUNDING_ENABLE_R_MIN	0x01
> +
>   #define BRW_POLYGON_FRONT_FACING     0
>   #define BRW_POLYGON_BACK_FACING      1
>
> diff --git a/src/mesa/drivers/dri/i965/brw_wm_sampler_state.c b/src/mesa/drivers/dri/i965/brw_wm_sampler_state.c
> index 6834eba..9fc1c1a 100644
> --- a/src/mesa/drivers/dri/i965/brw_wm_sampler_state.c
> +++ b/src/mesa/drivers/dri/i965/brw_wm_sampler_state.c
> @@ -312,6 +312,15 @@ static void brw_update_sampler_state(struct brw_context *brw,
>   			      intel->batch.bo, brw->wm.sdc_offset[unit],
>   			      I915_GEM_DOMAIN_SAMPLER, 0);
>      }
> +
> +   if (sampler->ss0.min_filter != BRW_MAPFILTER_NEAREST)
> +      sampler->ss3.address_round = BRW_ADDRESS_ROUNDING_ENABLE_U_MIN |
> +                                   BRW_ADDRESS_ROUNDING_ENABLE_V_MIN |
> +                                   BRW_ADDRESS_ROUNDING_ENABLE_R_MIN;
> +   if (sampler->ss0.mag_filter != BRW_MAPFILTER_NEAREST)
> +      sampler->ss3.address_round |= BRW_ADDRESS_ROUNDING_ENABLE_U_MAG |
> +                                    BRW_ADDRESS_ROUNDING_ENABLE_V_MAG |
> +                                    BRW_ADDRESS_ROUNDING_ENABLE_R_MAG;

It looks weird that one of these cases use = and the other uses |=. It
would be better to assign 0 to ss3.address_round before the first case
and use |= in both cases.

>   }
>
>
> diff --git a/src/mesa/drivers/dri/i965/gen7_sampler_state.c b/src/mesa/drivers/dri/i965/gen7_sampler_state.c
> index aee67c8..5fab91c 100644
> --- a/src/mesa/drivers/dri/i965/gen7_sampler_state.c
> +++ b/src/mesa/drivers/dri/i965/gen7_sampler_state.c
> @@ -167,6 +167,15 @@ gen7_update_sampler_state(struct brw_context *brw, int unit,
>      upload_default_color(brw, gl_sampler, unit);
>
>      sampler->ss2.default_color_pointer = brw->wm.sdc_offset[unit]>>  5;
> +
> +   if (sampler->ss0.min_filter != BRW_MAPFILTER_NEAREST)
> +      sampler->ss3.address_round = BRW_ADDRESS_ROUNDING_ENABLE_U_MIN |
> +                                   BRW_ADDRESS_ROUNDING_ENABLE_V_MIN |
> +                                   BRW_ADDRESS_ROUNDING_ENABLE_R_MIN;
> +   if (sampler->ss0.mag_filter != BRW_MAPFILTER_NEAREST)
> +      sampler->ss3.address_round |= BRW_ADDRESS_ROUNDING_ENABLE_U_MAG |
> +                                    BRW_ADDRESS_ROUNDING_ENABLE_V_MAG |
> +                                    BRW_ADDRESS_ROUNDING_ENABLE_R_MAG;

Same comment here as above.

>   }
>
>