[Mesa-dev] [RFC PATCH 15/26] i965: expose sample locations
Paul Berry
stereotype441 at gmail.com
Mon Jan 7 14:02:24 PST 2013
On 4 January 2013 13:04, Paul Berry <stereotype441 at gmail.com> wrote:
> On 29 December 2012 04:35, Chris Forbes <chrisf at ijw.co.nz> wrote:
>
>> Moves the definition of the sample locations out of
>> gen6_emit_3dstate_multisample, and unpacks them in
>> gen6_get_sample_postiion.
>>
>> Signed-off-by: Chris Forbes <chrisf at ijw.co.nz>
>> ---
>> src/mesa/drivers/dri/i965/brw_context.c | 3 +
>> src/mesa/drivers/dri/i965/brw_context.h | 5 +
>> src/mesa/drivers/dri/i965/gen6_multisample_state.c | 117
>> +++++++++++++--------
>> 3 files changed, 82 insertions(+), 43 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_context.c
>> b/src/mesa/drivers/dri/i965/brw_context.c
>> index 40f7ff3..4360c22 100644
>> --- a/src/mesa/drivers/dri/i965/brw_context.c
>> +++ b/src/mesa/drivers/dri/i965/brw_context.c
>> @@ -72,6 +72,9 @@ static void brwInitDriverFunctions(struct intel_screen
>> *screen,
>> functions->EndTransformFeedback = gen7_end_transform_feedback;
>> else
>> functions->EndTransformFeedback = brw_end_transform_feedback;
>> +
>> + if (screen->gen >= 6)
>> + functions->GetSampleLocation = gen6_get_sample_position;
>> }
>>
>> bool
>> diff --git a/src/mesa/drivers/dri/i965/brw_context.h
>> b/src/mesa/drivers/dri/i965/brw_context.h
>> index 25b82c4..7771b25 100644
>> --- a/src/mesa/drivers/dri/i965/brw_context.h
>> +++ b/src/mesa/drivers/dri/i965/brw_context.h
>> @@ -1229,6 +1229,11 @@ void
>> gen6_emit_3dstate_sample_mask(struct brw_context *brw,
>> unsigned num_samples, float coverage,
>> bool coverage_invert, unsigned
>> sample_mask);
>> +void
>> +gen6_get_sample_position(struct gl_context *ctx,
>> + struct gl_framebuffer *fb,
>> + GLuint index,
>> + GLfloat *result);
>>
>> /* gen7_urb.c */
>> void
>> diff --git a/src/mesa/drivers/dri/i965/gen6_multisample_state.c
>> b/src/mesa/drivers/dri/i965/gen6_multisample_state.c
>> index 844aad1..35071e8 100644
>> --- a/src/mesa/drivers/dri/i965/gen6_multisample_state.c
>> +++ b/src/mesa/drivers/dri/i965/gen6_multisample_state.c
>> @@ -26,6 +26,77 @@
>> #include "brw_context.h"
>> #include "brw_defines.h"
>>
>> +/* Sample positions:
>> + * 2 6 a e
>> + * 2 0
>> + * 6 1
>> + * a 2
>> + * e 3
>> + */
>> +static uint32_t
>> +sample_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.
>> + *
>> + * Note: from the Ivy Bridge PRM, Vol2 Part1 p304 (3DSTATE_MULTISAMPLE:
>> + * Programming Notes):
>> + *
>> + * "When programming the sample offsets (for NUMSAMPLES_4 or _8 and
>> + * MSRASTMODE_xxx_PATTERN), the order of the samples 0 to 3 (or 7
>> + * for 8X) must have monotonically increasing distance from the
>> + * pixel center. This is required to get the correct centroid
>> + * computation in the device."
>> + *
>> + * 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
>> + */
>> +static uint32_t
>> +sample_positions_8x[] = { 0xdbb39d79, 0x3ff55117 };
>> +
>> +
>> +void
>> +gen6_get_sample_position(struct gl_context *ctx,
>> + struct gl_framebuffer *fb,
>> + GLuint index, GLfloat *result)
>> +{
>> + switch (fb->Visual.samples) {
>> + case 1:
>> + result[0] = result[1] = 0.5f;
>> + break;
>> + case 4: {
>> + uint8_t val = (uint8_t)(sample_positions_4x[0] >> (8*index));
>> + result[0] = (val & 0xf) / 16.0f;
>> + result[1] = ((val >> 4) & 0xf) / 16.0f;
>> + break;
>> + }
>> + case 8: {
>> + uint8_t val = (uint8_t)(sample_positions_8x[index>>2] >> (8*(index
>> & 3)));
>> + result[0] = (val & 0xf) / 16.0f;
>> + result[1] = ((val >> 4) & 0xf) / 16.0f;
>> + break;
>> + }
>> + default:
>> + assert(!"Not implemented");
>> + }
>> +}
>>
>
> I'm concerned that this function is going to return incorrect values for
> either FBOs or for window system framebuffers (since the origin is at the
> upper left for the former and at the lower left for the latter). Do you
> have piglit tests to verify that the returned values are correct in both
> cases?
>
Chris pointed out to me over IRC that the ARB_texture_multisample spec
contains this text:
(6) How does SAMPLE_POSITION interact with
EXT_fragment_coord_conventions?
RESOLVED: The SAMPLE_POSITION query is not in any way affected by
the shader state added in EXT_fragment_coord_conventions. It is
expected that the returned values will not actually be used within
the shader, but rather to compute filter weights on the CPU, so
whether the fragment coord is inverted or translated by 0.5 doesn't
matter.
Which seems to suggest that maybe we don't need to worry about it.
I talked this over with Ian and our feeling is that it's still worth
worrying about. Here's our rationale:
(1) ARB_fragment_coord_conventions provides the ability for a client
program to opt into a non-standard set of fragment coordinate conventions
if it makes sense for their application (e.g. an app that is ported from
DirectX might want to do this, to make the fragment coordinate conventions
more compatible with those used in DirectX). It seems reasonable that if a
client program is knowingly opting into a non-standard set of fragment
coordinate conventions, it's ok to force it to massage the sample positions
accordingly. However, the distinction in coordinate conventions between
FBO's and winsys framebuffers isn't something that the app opts into--it's
an implementation detail of Mesa (and X windows) that we try to hide from
the app developer as much as possible.
(2) A number of apps do multisampled rendering at a reduced resolution in
order to speed up rendering (e.g. if the display is 1024x768 and the app is
doing 4x MSAA, rather than doing 4x MSAA to a 1024x768 buffer, the app
might do 4x MSAA to an 800x600 buffer, and then scale the image up to
1024x768 in the process of doing the resolve). Since there is no longer a
1:1 relationship between MSAA pixels and display pixels, it's likely that
an app would want to take advantage of the SAMPLE_POSITION values to select
which samples to blend together while doing the resolve. If we
accidentally flip the Y coordinate of the sample positions, we would create
annoying artefacts in those apps.
(3) Getting the sample positions right is just a two-line change--something
like:
if (_mesa_is_winsys_fbo(framebuffer))
y = 1 - y;
It seems worth taking the tiny effort to get it right now, rather than have
to deal with a subtle bug in the future.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130107/17508aec/attachment-0001.html>
More information about the mesa-dev
mailing list