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

[Ian Romanick]

On 10/13/2011 10:40 PM, Yuanhan Liu wrote:
> On Thu, Oct 13, 2011 at 09:12:09PM -0700, Ian Romanick wrote:
>> 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.
>
>  From my check, this bit was already set in our driver. It only use
> the mag and min filter as the condition. But in this patch, the round
> mode is set according to the mag and min filter. So, the results would
> be the same. And, IVB don't have this bit.

I see it now.  I don't know how I missed it before.

>> Does adding the following code at about line 227 of brw_wm_sampler.c
>> make any additional tests pass on SNB?
>
> Sorry, I didn't make it clear. Here the word 'several' means all the
> fails with the same issue. As the there are still about three fails that
> not belong this issue in the pbo test suite. My bad.

You were clear. :)  I was wondering if setting that bit would fix even 
more tests.  Since the bit is already set correctly, it shouldn't matter.

>>     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.
>
> Yes, will fix it later.
>
> Thanks,
> Yuanhan Liu
>
>>
>>>   }
>>>
>>>
>>> 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.
>>
>>>   }