[Mesa-dev] [RFC PATCH 15/26] i965: expose sample locations

Chris Forbes chrisf at ijw.co.nz
Mon Jan 7 14:28:45 PST 2013


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 <stereotype441 at gmail.com> wrote:
> 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.
>


More information about the mesa-dev mailing list