<div dir="ltr"><div dir="ltr"><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Mar 13, 2019 at 8:43 AM Eleni Maria Stea <<a href="mailto:estea@igalia.com">estea@igalia.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Wed, 13 Mar 2019 08:16:10 -0500<br>
Jason Ekstrand <<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>> wrote:<br>
<br>
> On Mon, Mar 11, 2019 at 10:05 AM Eleni Maria Stea <<a href="mailto:estea@igalia.com" target="_blank">estea@igalia.com</a>><br>
> wrote:<br>
> <br>
> > Allowing the user to set custom sample locations non-dynamically, by<br>
> > filling the extension structs and chaining them to the pipeline<br>
> > structs according to the Vulkan specification section [26.5. Custom<br>
> > Sample Locations]<br>
<br>
[...]<br>
<br>
> > +void<br>
> > +anv_calc_sample_locations(struct anv_sample *samples,<br>
> > +                          uint32_t num_samples,<br>
> > +                          const VkSampleLocationsInfoEXT *info)<br>
> > +{<br>
> > +   int i;<br>
> > +<br>
> > +   for(i = 0; i < num_samples; i++) {<br>
> > +      float dx, dy;<br>
> > +<br>
> > +      /* this is because the grid is 1x1, in case that<br>
> > +       * we support different grid sizes in the future<br>
> > +       * this must be changed.<br>
> > +       */<br>
> > +      samples[i].offs_x = info->pSampleLocations[i].x;<br>
> > +      samples[i].offs_y = info->pSampleLocations[i].y;<br>
> > +<br>
> > +      /* distance from the center */<br>
> > +      dx = samples[i].offs_x - 0.5;<br>
> > +      dy = samples[i].offs_y - 0.5;<br>
> > +<br>
> > +      samples[i].radius = dx * dx + dy * dy;<br>
> > +   }<br>
> > +<br>
> > +   qsort(samples, num_samples, sizeof *samples, compare_samples);<br>
> >  <br>
> <br>
> Are we allowed to re-order the samples like this?  The spec says:<br>
> <br>
> The sample location for sample i at the pixel grid location (x,y) is<br>
> taken from pSampleLocations[(x + y * sampleLocationGridSize.width) *<br>
> sampleLocationsPerPixel + i]<br>
> <br>
> Which leads me to think that they expect the ordering of samples to be<br>
> respected.  Yes, I know the HW docs say we're supposed to order them<br>
> from nearest to furthest.  However, AFAIK, that's only so we get nice<br>
> centroids and I don't know that it's actually required.<br>
> <br>
> --Jason<br>
<br>
I wasn't sure about this to be honest. I could remove the qsort and<br>
explain why we decided to ignore the PRM in a comment for the case that<br>
someone decides to put this back in the future.<br></blockquote><div><br></div><div>I think we're better off ignoring the hardware and adding a comment something like ths:</div><div><br></div><div>The Skylake PRM Vol. 2a "3DSTATE_SAMPLE_PATTERN" says:</div><div><br></div><div>   "When programming the sample offsets (for NUMSAMPLES_4 or _8 and MSRASTMODE_xxx_PATTERN), the<br>   order of the samples 0 to 3 (or 7 for 8X, or 15 for 16X) must have monotonically increasing distance from the<br>   pixel center. This is required to get the correct centroid computation in the device."</div><div><br></div><div>However, the Vulkan spec seems to require that the the samples occur in the order provided through the API.  The standard sample patterns have the above property that they have monotonically increasing distances from the center but client-provided ones do not.  As long as this only affects centroid calculations as the docs say, we should be ok because OpenGL and Vulkan only require that the centroid be some lit sample and that it's the same for all samples in a pixel; they have no requirement that it be the one closest to center.</div><div><br></div><div>--Jason<br></div></div></div></div>