<div dir="ltr">On 4 January 2013 13:04, Paul Berry <span dir="ltr"><<a href="mailto:stereotype441@gmail.com" target="_blank">stereotype441@gmail.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div><div class="h5">On 29 December 2012 04:35, Chris Forbes <span dir="ltr"><<a href="mailto:chrisf@ijw.co.nz" target="_blank">chrisf@ijw.co.nz</a>></span> wrote:<br>
</div></div><div class="gmail_extra"><div class="gmail_quote"><div><div class="h5"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Moves the definition of the sample locations out of<br>
gen6_emit_3dstate_multisample, and unpacks them in<br>
gen6_get_sample_postiion.<br>
<br>
Signed-off-by: Chris Forbes <<a href="mailto:chrisf@ijw.co.nz" target="_blank">chrisf@ijw.co.nz</a>><br>
---<br>
src/mesa/drivers/dri/i965/brw_context.c | 3 +<br>
src/mesa/drivers/dri/i965/brw_context.h | 5 +<br>
src/mesa/drivers/dri/i965/gen6_multisample_state.c | 117 +++++++++++++--------<br>
3 files changed, 82 insertions(+), 43 deletions(-)<br>
<br>
diff --git a/src/mesa/drivers/dri/i965/brw_context.c b/src/mesa/drivers/dri/i965/brw_context.c<br>
index 40f7ff3..4360c22 100644<br>
--- a/src/mesa/drivers/dri/i965/brw_context.c<br>
+++ b/src/mesa/drivers/dri/i965/brw_context.c<br>
@@ -72,6 +72,9 @@ static void brwInitDriverFunctions(struct intel_screen *screen,<br>
functions->EndTransformFeedback = gen7_end_transform_feedback;<br>
else<br>
functions->EndTransformFeedback = brw_end_transform_feedback;<br>
+<br>
+ if (screen->gen >= 6)<br>
+ functions->GetSampleLocation = gen6_get_sample_position;<br>
}<br>
<br>
bool<br>
diff --git a/src/mesa/drivers/dri/i965/brw_context.h b/src/mesa/drivers/dri/i965/brw_context.h<br>
index 25b82c4..7771b25 100644<br>
--- a/src/mesa/drivers/dri/i965/brw_context.h<br>
+++ b/src/mesa/drivers/dri/i965/brw_context.h<br>
@@ -1229,6 +1229,11 @@ void<br>
gen6_emit_3dstate_sample_mask(struct brw_context *brw,<br>
unsigned num_samples, float coverage,<br>
bool coverage_invert, unsigned sample_mask);<br>
+void<br>
+gen6_get_sample_position(struct gl_context *ctx,<br>
+ struct gl_framebuffer *fb,<br>
+ GLuint index,<br>
+ GLfloat *result);<br>
<br>
/* gen7_urb.c */<br>
void<br>
diff --git a/src/mesa/drivers/dri/i965/gen6_multisample_state.c b/src/mesa/drivers/dri/i965/gen6_multisample_state.c<br>
index 844aad1..35071e8 100644<br>
--- a/src/mesa/drivers/dri/i965/gen6_multisample_state.c<br>
+++ b/src/mesa/drivers/dri/i965/gen6_multisample_state.c<br>
@@ -26,6 +26,77 @@<br>
#include "brw_context.h"<br>
#include "brw_defines.h"<br>
<br>
+/* Sample positions:<br>
+ * 2 6 a e<br>
+ * 2 0<br>
+ * 6 1<br>
+ * a 2<br>
+ * e 3<br>
+ */<br>
+static uint32_t<br>
+sample_positions_4x[] = { 0xae2ae662 };<br>
+/* Sample positions are based on a solution to the "8 queens" puzzle.<br>
+ * Rationale: in a solution to the 8 queens puzzle, no two queens share<br>
+ * a row, column, or diagonal. This is a desirable property for samples<br>
+ * in a multisampling pattern, because it ensures that the samples are<br>
+ * relatively uniformly distributed through the pixel.<br>
+ *<br>
+ * There are several solutions to the 8 queens puzzle (see<br>
+ * <a href="http://en.wikipedia.org/wiki/Eight_queens_puzzle" target="_blank">http://en.wikipedia.org/wiki/Eight_queens_puzzle</a>). This solution was<br>
+ * chosen because it has a queen close to the center; this should<br>
+ * improve the accuracy of centroid interpolation, since the hardware<br>
+ * implements centroid interpolation by choosing the centermost sample<br>
+ * that overlaps with the primitive being drawn.<br>
+ *<br>
+ * Note: from the Ivy Bridge PRM, Vol2 Part1 p304 (3DSTATE_MULTISAMPLE:<br>
+ * Programming Notes):<br>
+ *<br>
+ * "When programming the sample offsets (for NUMSAMPLES_4 or _8 and<br>
+ * MSRASTMODE_xxx_PATTERN), the order of the samples 0 to 3 (or 7<br>
+ * for 8X) must have monotonically increasing distance from the<br>
+ * pixel center. This is required to get the correct centroid<br>
+ * computation in the device."<br>
+ *<br>
+ * Sample positions:<br>
+ * 1 3 5 7 9 b d f<br>
+ * 1 5<br>
+ * 3 2<br>
+ * 5 6<br>
+ * 7 4<br>
+ * 9 0<br>
+ * b 3<br>
+ * d 1<br>
+ * f 7<br>
+ */<br>
+static uint32_t<br>
+sample_positions_8x[] = { 0xdbb39d79, 0x3ff55117 };<br>
+<br>
+<br>
+void<br>
+gen6_get_sample_position(struct gl_context *ctx,<br>
+ struct gl_framebuffer *fb,<br>
+ GLuint index, GLfloat *result)<br>
+{<br>
+ switch (fb->Visual.samples) {<br>
+ case 1:<br>
+ result[0] = result[1] = 0.5f;<br>
+ break;<br>
+ case 4: {<br>
+ uint8_t val = (uint8_t)(sample_positions_4x[0] >> (8*index));<br>
+ result[0] = (val & 0xf) / 16.0f;<br>
+ result[1] = ((val >> 4) & 0xf) / 16.0f;<br>
+ break;<br>
+ }<br>
+ case 8: {<br>
+ uint8_t val = (uint8_t)(sample_positions_8x[index>>2] >> (8*(index & 3)));<br>
+ result[0] = (val & 0xf) / 16.0f;<br>
+ result[1] = ((val >> 4) & 0xf) / 16.0f;<br>
+ break;<br>
+ }<br>
+ default:<br>
+ assert(!"Not implemented");<br>
+ }<br>
+}<br></blockquote><div><br></div></div></div><div>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?<br>
</div></div></div></div></blockquote><div><br></div><div>Chris pointed out to me over IRC that the ARB_texture_multisample spec contains this text:<br><br> (6) How does SAMPLE_POSITION interact with<br> EXT_fragment_coord_conventions?<br>
<br> RESOLVED: The SAMPLE_POSITION query is not in any way affected by<br> the shader state added in EXT_fragment_coord_conventions. It is<br> expected that the returned values will not actually be used within<br>
the shader, but rather to compute filter weights on the CPU, so<br> whether the fragment coord is inverted or translated by 0.5 doesn't<br> matter.<br><br></div><div>Which seems to suggest that maybe we don't need to worry about it.<br>
</div><div><br></div><div>I talked this over with Ian and our feeling is that it's still worth worrying about. Here's our rationale:<br><br></div><div>(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.<br>
<br></div><div>(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.<br>
<br></div><div>(3) Getting the sample positions right is just a two-line change--something like:<br><br></div><div>if (_mesa_is_winsys_fbo(framebuffer))<br></div><div> y = 1 - y;<br><br></div><div>It seems worth taking the tiny effort to get it right now, rather than have to deal with a subtle bug in the future.<br>
</div><br></div></div></div>