[Mesa-dev] [PATCH] i965: Change 8x multisample positions

[Anuj Phogat]

On Thu, Aug 11, 2016 at 10:53 AM, Jason Ekstrand <jason at jlekstrand.net> wrote:
>
>
> On Thu, Aug 11, 2016 at 10:41 AM, Anuj Phogat <anuj.phogat at gmail.com> wrote:
>>
>> There are no standard sample positions defined in OpenGL and OpenGL
>> ES specs. Implementations have the freedom to pick the positions
>> which give plausible results. But the Vulkan 1.0 spec does define
>> standard sample positions for different sample counts. Defined
>> positions in Vulkan for all the sample counts except 8X match with
>> the positions we set in i965. We have an upcoming plan to share the
>> blorp code between OpenGL and Vulkan driver in near future. Keeping
>> the 8X sample positions same on both the drivers will help us move
>> in that direction.
>>
>> Here is an argument by Neil Roberts (from commit 20250e85) against
>> any advantage of current 8X sample positions over the new ones:
>>
>> "The comment above for the 8x sample positions says that the hardware
>> implements centroid interpolation by picking the centre-most sample
>> that is inside the primitive. That implies that it might be worthwhile
>> to pick a pattern that includes 0.5,0.5. However by experimentation
>> this doesn't seem to actually be the case. With the sample positions
>> in this patch, if I modify the piglit test below so that it instead
>> reports the centroid position, it reports 0.492188,0.421875 which
>> doesn't match any of the positions. If I modify the sample positions
>> so that they include one at exactly 0.5,0.5 it doesn't help and it
>> reports another position which is even further from the center for
>> some reason.
>>
>> arb_gpu_shader5-interpolateAtSample-different
>>
>> Kenneth Graunke experimented with some other patterns that have a
>> higher standard deviation but I think after some discussion it was
>> decided that it would be better to pick the same pattern as the other
>> graphics API in case there are games that rely on this pattern."
>>
>> Observed no regressions in jenkins testing.
>
>
> Did you try the scaled blit tests?  I bet we have to rework 8x scaled blit
> for the new positions.
>
They all passed because I missed changing sample mapping pattern
in blorp and the piglit tests. So, the incorrect test image perfectly matched
the incorrect reference image. I'll send out patches to fix these issues.

> Thanks for working on this!  It's going to make the blorp transition way
> easier.
>
>>
>> Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
>> Cc: Jason Ekstrand <jason at jlekstrand.net>
>> ---
>>  src/mesa/drivers/dri/i965/brw_multisample_state.h | 33
>> ++++++++---------------
>>  1 file changed, 11 insertions(+), 22 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_multisample_state.h
>> b/src/mesa/drivers/dri/i965/brw_multisample_state.h
>> index 42a7fd3..78fb03a 100644
>> --- a/src/mesa/drivers/dri/i965/brw_multisample_state.h
>> +++ b/src/mesa/drivers/dri/i965/brw_multisample_state.h
>> @@ -46,20 +46,9 @@ static const uint32_t
>>  brw_multisample_positions_4x = 0xae2ae662;
>>
>>  /**
>> - * Sample positions are based on a solution to the "8 queens" puzzle.
>> - * Rationale: in a solution to the 8 queens puzzle, no two queens share
>> - * a row, column, or diagonal.  This is a desirable property for samples
>> - * in a multisampling pattern, because it ensures that the samples are
>> - * relatively uniformly distributed through the pixel.
>> - *
>> - * There are several solutions to the 8 queens puzzle (see
>> - * http://en.wikipedia.org/wiki/Eight_queens_puzzle).  This solution was
>> - * chosen because it has a queen close to the center; this should
>> - * improve the accuracy of centroid interpolation, since the hardware
>> - * implements centroid interpolation by choosing the centermost sample
>> - * that overlaps with the primitive being drawn.
>> + * Sample positions:
>
>
> Might be nice to reference the Vulkan spec here.
I'll add the Vulkan reference here.
>
>>
>>   *
>> - * Note: from the Ivy Bridge PRM, Vol2 Part1 p304 (3DSTATE_MULTISAMPLE:
>> + * From the Ivy Bridge PRM, Vol2 Part1 p304 (3DSTATE_MULTISAMPLE:
>>   * Programming Notes):
>>   *
>>   *     "When programming the sample offsets (for NUMSAMPLES_4 or _8 and
>> @@ -70,17 +59,17 @@ brw_multisample_positions_4x = 0xae2ae662;
>>   *
>>   * Sample positions:
>>   *   1 3 5 7 9 b d f
>> - * 1     5
>> - * 3           2
>> - * 5               6
>> - * 7 4
>> - * 9       0
>> - * b             3
>> - * d         1
>> - * f   7
>> + * 1               7
>> + * 3     3
>> + * 5         0
>> + * 7 5
>> + * 9             2
>> + * b       1
>> + * d   4
>> + * f           6
>>   */
>>  static const uint32_t
>> -brw_multisample_positions_8x[] = { 0xdbb39d79, 0x3ff55117 };
>> +brw_multisample_positions_8x[] = { 0x53d97b95, 0xf1bf173d };
>>
>>  /**
>>   * Sample positions:
>> --
>> 2.5.5
>>
>