Sounds good to me, especially since it's trivial to do. I'll roll this
into the next version of the series.
-- Chris
On Tue, Jan 8, 2013 at 11:02 AM, Paul Berry <stereotype...@gmail.com> wrote:
> On 4 January 2013 13:04, Paul Berry <stereotype...@gmail.com> wrote:
>>
>> On 29 December 2012 04:35, Chris Forbes <chr...@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 <chr...@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.
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
