Re: [Mesa-dev] [PATCH 4/9] anv: Added support for non-dynamic sample locations on Gen8+
On Wed, Mar 13, 2019 at 8:43 AM Eleni Maria Stea wrote: > On Wed, 13 Mar 2019 08:16:10 -0500 > Jason Ekstrand wrote: > > > On Mon, Mar 11, 2019 at 10:05 AM Eleni Maria Stea > > wrote: > > > > > Allowing the user to set custom sample locations non-dynamically, by > > > filling the extension structs and chaining them to the pipeline > > > structs according to the Vulkan specification section [26.5. Custom > > > Sample Locations] > > [...] > > > > +void > > > +anv_calc_sample_locations(struct anv_sample *samples, > > > + uint32_t num_samples, > > > + const VkSampleLocationsInfoEXT *info) > > > +{ > > > + int i; > > > + > > > + for(i = 0; i < num_samples; i++) { > > > + float dx, dy; > > > + > > > + /* this is because the grid is 1x1, in case that > > > + * we support different grid sizes in the future > > > + * this must be changed. > > > + */ > > > + samples[i].offs_x = info->pSampleLocations[i].x; > > > + samples[i].offs_y = info->pSampleLocations[i].y; > > > + > > > + /* distance from the center */ > > > + dx = samples[i].offs_x - 0.5; > > > + dy = samples[i].offs_y - 0.5; > > > + > > > + samples[i].radius = dx * dx + dy * dy; > > > + } > > > + > > > + qsort(samples, num_samples, sizeof *samples, compare_samples); > > > > > > > Are we allowed to re-order the samples like this? The spec says: > > > > The sample location for sample i at the pixel grid location (x,y) is > > taken from pSampleLocations[(x + y * sampleLocationGridSize.width) * > > sampleLocationsPerPixel + i] > > > > Which leads me to think that they expect the ordering of samples to be > > respected. Yes, I know the HW docs say we're supposed to order them > > from nearest to furthest. However, AFAIK, that's only so we get nice > > centroids and I don't know that it's actually required. > > > > --Jason > > I wasn't sure about this to be honest. I could remove the qsort and > explain why we decided to ignore the PRM in a comment for the case that > someone decides to put this back in the future. > I think we're better off ignoring the hardware and adding a comment something like ths: The Skylake PRM Vol. 2a "3DSTATE_SAMPLE_PATTERN" says: "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, or 15 for 16X) must have monotonically increasing distance from the pixel center. This is required to get the correct centroid computation in the device." 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. --Jason ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/9] anv: Added support for non-dynamic sample locations on Gen8+
On Wed, 13 Mar 2019 08:16:10 -0500 Jason Ekstrand wrote: > On Mon, Mar 11, 2019 at 10:05 AM Eleni Maria Stea > wrote: > > > Allowing the user to set custom sample locations non-dynamically, by > > filling the extension structs and chaining them to the pipeline > > structs according to the Vulkan specification section [26.5. Custom > > Sample Locations] [...] > > +void > > +anv_calc_sample_locations(struct anv_sample *samples, > > + uint32_t num_samples, > > + const VkSampleLocationsInfoEXT *info) > > +{ > > + int i; > > + > > + for(i = 0; i < num_samples; i++) { > > + float dx, dy; > > + > > + /* this is because the grid is 1x1, in case that > > + * we support different grid sizes in the future > > + * this must be changed. > > + */ > > + samples[i].offs_x = info->pSampleLocations[i].x; > > + samples[i].offs_y = info->pSampleLocations[i].y; > > + > > + /* distance from the center */ > > + dx = samples[i].offs_x - 0.5; > > + dy = samples[i].offs_y - 0.5; > > + > > + samples[i].radius = dx * dx + dy * dy; > > + } > > + > > + qsort(samples, num_samples, sizeof *samples, compare_samples); > > > > Are we allowed to re-order the samples like this? The spec says: > > The sample location for sample i at the pixel grid location (x,y) is > taken from pSampleLocations[(x + y * sampleLocationGridSize.width) * > sampleLocationsPerPixel + i] > > Which leads me to think that they expect the ordering of samples to be > respected. Yes, I know the HW docs say we're supposed to order them > from nearest to furthest. However, AFAIK, that's only so we get nice > centroids and I don't know that it's actually required. > > --Jason I wasn't sure about this to be honest. I could remove the qsort and explain why we decided to ignore the PRM in a comment for the case that someone decides to put this back in the future. Thanks a lot for reviewing the series, BTW. I am working on the changes for all patches. Eleni ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/9] anv: Added support for non-dynamic sample locations on Gen8+
On Mon, Mar 11, 2019 at 10:05 AM Eleni Maria Stea wrote: > Allowing the user to set custom sample locations non-dynamically, by > filling the extension structs and chaining them to the pipeline structs > according to the Vulkan specification section [26.5. Custom Sample > Locations] > for the following structures: > > 'VkPipelineSampleLocationsStateCreateInfoEXT' > 'VkSampleLocationsInfoEXT' > 'VkSampleLocationEXT' > > Once custom locations are used, the default locations are lost and need to > be > re-emitted again in the next pipeline creation. For that, we emit the > 3DSTATE_SAMPLE_PATTERN at every pipeline creation. > --- > src/intel/common/gen_sample_positions.h | 53 > src/intel/vulkan/anv_genX.h | 5 ++ > src/intel/vulkan/anv_private.h | 9 +++ > src/intel/vulkan/anv_sample_locations.c | 38 +++- > src/intel/vulkan/anv_sample_locations.h | 29 + > src/intel/vulkan/genX_pipeline.c| 80 + > src/intel/vulkan/genX_state.c | 59 ++ > 7 files changed, 259 insertions(+), 14 deletions(-) > create mode 100644 src/intel/vulkan/anv_sample_locations.h > > diff --git a/src/intel/common/gen_sample_positions.h > b/src/intel/common/gen_sample_positions.h > index da48dcb5ed0..e8af2a552dc 100644 > --- a/src/intel/common/gen_sample_positions.h > +++ b/src/intel/common/gen_sample_positions.h > @@ -160,4 +160,57 @@ prefix##14YOffset = 0.9375; \ > prefix##15XOffset = 0.0625; \ > prefix##15YOffset = 0.; > > +/* Examples: > + * in case of GEN_GEN < 8: > + * SET_SAMPLE_POS(ms.Sample, 0); expands to: > + *ms.Sample0XOffset = anv_samples[0].offs_x; > + *ms.Sample0YOffset = anv_samples[0].offs_y; > + * > + * in case of GEN_GEN >= 8: > + * SET_SAMPLE_POS(sp._16xSample, 0); expands to: > + *sp._16xSample0XOffset = anv_samples[0].offs_x; > + *sp._16xSample0YOffset = anv_samples[0].offs_y; > + */ > +#define SET_SAMPLE_POS(prefix, sample_idx) \ > +prefix##sample_idx##XOffset = anv_samples[sample_idx].offs_x; \ > +prefix##sample_idx##YOffset = anv_samples[sample_idx].offs_y; > + > +#define SET_SAMPLE_POS_2X(prefix) \ > +SET_SAMPLE_POS(prefix, 0); \ > +SET_SAMPLE_POS(prefix, 1); > + > +#define SET_SAMPLE_POS_4X(prefix) \ > +SET_SAMPLE_POS(prefix, 0); \ > +SET_SAMPLE_POS(prefix, 1); \ > +SET_SAMPLE_POS(prefix, 2); \ > +SET_SAMPLE_POS(prefix, 3); > + > +#define SET_SAMPLE_POS_8X(prefix) \ > +SET_SAMPLE_POS(prefix, 0); \ > +SET_SAMPLE_POS(prefix, 1); \ > +SET_SAMPLE_POS(prefix, 2); \ > +SET_SAMPLE_POS(prefix, 3); \ > +SET_SAMPLE_POS(prefix, 4); \ > +SET_SAMPLE_POS(prefix, 5); \ > +SET_SAMPLE_POS(prefix, 6); \ > +SET_SAMPLE_POS(prefix, 7); > + > +#define SET_SAMPLE_POS_16X(prefix) \ > +SET_SAMPLE_POS(prefix, 0); \ > +SET_SAMPLE_POS(prefix, 1); \ > +SET_SAMPLE_POS(prefix, 2); \ > +SET_SAMPLE_POS(prefix, 3); \ > +SET_SAMPLE_POS(prefix, 4); \ > +SET_SAMPLE_POS(prefix, 5); \ > +SET_SAMPLE_POS(prefix, 6); \ > +SET_SAMPLE_POS(prefix, 7); \ > +SET_SAMPLE_POS(prefix, 8); \ > +SET_SAMPLE_POS(prefix, 9); \ > +SET_SAMPLE_POS(prefix, 10); \ > +SET_SAMPLE_POS(prefix, 11); \ > +SET_SAMPLE_POS(prefix, 12); \ > +SET_SAMPLE_POS(prefix, 13); \ > +SET_SAMPLE_POS(prefix, 14); \ > +SET_SAMPLE_POS(prefix, 15); > + > #endif /* GEN_SAMPLE_POSITIONS_H */ > diff --git a/src/intel/vulkan/anv_genX.h b/src/intel/vulkan/anv_genX.h > index 8fd32cabf1e..52415c04a45 100644 > --- a/src/intel/vulkan/anv_genX.h > +++ b/src/intel/vulkan/anv_genX.h > @@ -88,3 +88,8 @@ void genX(cmd_buffer_mi_memset)(struct anv_cmd_buffer > *cmd_buffer, > > void genX(blorp_exec)(struct blorp_batch *batch, >const struct blorp_params *params); > + > +void genX(emit_sample_locations)(struct anv_batch *batch, > + uint32_t num_samples, > + const VkSampleLocationsInfoEXT *sl, > + bool custom_locations); > diff --git a/src/intel/vulkan/anv_private.h > b/src/intel/vulkan/anv_private.h > index 5905299e59d..981956e5706 100644 > --- a/src/intel/vulkan/anv_private.h > +++ b/src/intel/vulkan/anv_private.h > @@ -71,6 +71,7 @@ struct anv_buffer; > struct anv_buffer_view; > struct anv_image_view; > struct anv_instance; > +struct anv_sample; > > struct gen_l3_config; > > @@ -165,6 +166,7 @@ struct gen_l3_config; > #define MAX_PUSH_DESCRIPTORS 32 /* Minimum requirement */ > #define MAX_INLINE_UNIFORM_BLOCK_SIZE 4096 > #define MAX_INLINE_UNIFORM_BLOCK_DESCRIPTORS 32 > +#define MAX_SAMPLE_LOCATIONS 16 > > /* The kernel relocation API has a limitation of a 32-bit delta value > * applied to the address before it is written which, in spite of it being > @@ -2086,6 +2088,13 @@ struct anv_push_constants { > struct brw_image_param images[MAX_GEN8_IMAGES]; > }; > > +struct > +anv_sample { > + float offs_x; > + float offs_y; > + float radius; > +}; > + > struct anv_dynamic_state { > struct { >uint32_t
Re: [Mesa-dev] [PATCH 4/9] anv: Added support for non-dynamic sample locations on Gen8+
On Mon, Mar 11, 2019 at 10:05 AM Eleni Maria Stea wrote: > Allowing the user to set custom sample locations non-dynamically, by > filling the extension structs and chaining them to the pipeline structs > according to the Vulkan specification section [26.5. Custom Sample > Locations] > for the following structures: > > 'VkPipelineSampleLocationsStateCreateInfoEXT' > 'VkSampleLocationsInfoEXT' > 'VkSampleLocationEXT' > > Once custom locations are used, the default locations are lost and need to > be > re-emitted again in the next pipeline creation. For that, we emit the > 3DSTATE_SAMPLE_PATTERN at every pipeline creation. > --- > src/intel/common/gen_sample_positions.h | 53 > src/intel/vulkan/anv_genX.h | 5 ++ > src/intel/vulkan/anv_private.h | 9 +++ > src/intel/vulkan/anv_sample_locations.c | 38 +++- > src/intel/vulkan/anv_sample_locations.h | 29 + > src/intel/vulkan/genX_pipeline.c| 80 + > src/intel/vulkan/genX_state.c | 59 ++ > 7 files changed, 259 insertions(+), 14 deletions(-) > create mode 100644 src/intel/vulkan/anv_sample_locations.h > > diff --git a/src/intel/common/gen_sample_positions.h > b/src/intel/common/gen_sample_positions.h > index da48dcb5ed0..e8af2a552dc 100644 > --- a/src/intel/common/gen_sample_positions.h > +++ b/src/intel/common/gen_sample_positions.h > @@ -160,4 +160,57 @@ prefix##14YOffset = 0.9375; \ > prefix##15XOffset = 0.0625; \ > prefix##15YOffset = 0.; > > +/* Examples: > + * in case of GEN_GEN < 8: > + * SET_SAMPLE_POS(ms.Sample, 0); expands to: > + *ms.Sample0XOffset = anv_samples[0].offs_x; > + *ms.Sample0YOffset = anv_samples[0].offs_y; > + * > + * in case of GEN_GEN >= 8: > + * SET_SAMPLE_POS(sp._16xSample, 0); expands to: > + *sp._16xSample0XOffset = anv_samples[0].offs_x; > + *sp._16xSample0YOffset = anv_samples[0].offs_y; > + */ > +#define SET_SAMPLE_POS(prefix, sample_idx) \ > +prefix##sample_idx##XOffset = anv_samples[sample_idx].offs_x; \ > +prefix##sample_idx##YOffset = anv_samples[sample_idx].offs_y; > I'm not a huge fan of hanving anv-specific stuff in gen_sample_positions.h and I also don't think I really like having the array be implicit in the macro. Maybe the best thing to do here would be to have a struct gen_sample_position { float x_offset; float y_offset; } And then #define GEN_SAMPLE_POS_ELEM(prefix, arr, sample_idx) \ prefix##sample_idx##XOffset = arr[sample_idx].x_offset; \ prefix##sample_idx##YOffset = arr[sample_idx].y_offset; And #define GEN_SAMPLE_POS_1X_ARRAY(prefix, arr) \ SET_SAMPLE_POS_ELEM(prefix, arr, 0) #define GEN_SAMPLE_POS_2X_ARRAY(prefix, arr) \ SET_SAMPLE_POS_ELEM(prefix, arr, 0) \ SET_SAMPLE_POS_ELEM(prefix, arr, 1) etc. + > +#define SET_SAMPLE_POS_2X(prefix) \ > +SET_SAMPLE_POS(prefix, 0); \ > +SET_SAMPLE_POS(prefix, 1); > + > +#define SET_SAMPLE_POS_4X(prefix) \ > +SET_SAMPLE_POS(prefix, 0); \ > +SET_SAMPLE_POS(prefix, 1); \ > +SET_SAMPLE_POS(prefix, 2); \ > +SET_SAMPLE_POS(prefix, 3); > + > +#define SET_SAMPLE_POS_8X(prefix) \ > +SET_SAMPLE_POS(prefix, 0); \ > +SET_SAMPLE_POS(prefix, 1); \ > +SET_SAMPLE_POS(prefix, 2); \ > +SET_SAMPLE_POS(prefix, 3); \ > +SET_SAMPLE_POS(prefix, 4); \ > +SET_SAMPLE_POS(prefix, 5); \ > +SET_SAMPLE_POS(prefix, 6); \ > +SET_SAMPLE_POS(prefix, 7); > + > +#define SET_SAMPLE_POS_16X(prefix) \ > +SET_SAMPLE_POS(prefix, 0); \ > +SET_SAMPLE_POS(prefix, 1); \ > +SET_SAMPLE_POS(prefix, 2); \ > +SET_SAMPLE_POS(prefix, 3); \ > +SET_SAMPLE_POS(prefix, 4); \ > +SET_SAMPLE_POS(prefix, 5); \ > +SET_SAMPLE_POS(prefix, 6); \ > +SET_SAMPLE_POS(prefix, 7); \ > +SET_SAMPLE_POS(prefix, 8); \ > +SET_SAMPLE_POS(prefix, 9); \ > +SET_SAMPLE_POS(prefix, 10); \ > +SET_SAMPLE_POS(prefix, 11); \ > +SET_SAMPLE_POS(prefix, 12); \ > +SET_SAMPLE_POS(prefix, 13); \ > +SET_SAMPLE_POS(prefix, 14); \ > +SET_SAMPLE_POS(prefix, 15); > + > #endif /* GEN_SAMPLE_POSITIONS_H */ > diff --git a/src/intel/vulkan/anv_genX.h b/src/intel/vulkan/anv_genX.h > index 8fd32cabf1e..52415c04a45 100644 > --- a/src/intel/vulkan/anv_genX.h > +++ b/src/intel/vulkan/anv_genX.h > @@ -88,3 +88,8 @@ void genX(cmd_buffer_mi_memset)(struct anv_cmd_buffer > *cmd_buffer, > > void genX(blorp_exec)(struct blorp_batch *batch, >const struct blorp_params *params); > + > +void genX(emit_sample_locations)(struct anv_batch *batch, > + uint32_t num_samples, > + const VkSampleLocationsInfoEXT *sl, > + bool custom_locations); > diff --git a/src/intel/vulkan/anv_private.h > b/src/intel/vulkan/anv_private.h > index 5905299e59d..981956e5706 100644 > --- a/src/intel/vulkan/anv_private.h > +++ b/src/intel/vulkan/anv_private.h > @@ -71,6 +71,7 @@ struct anv_buffer; > struct anv_buffer_view; > struct anv_image_view; > struct anv_instance; > +struct anv_sample; > > struct gen_l3_config; > > @@
[Mesa-dev] [PATCH 4/9] anv: Added support for non-dynamic sample locations on Gen8+
Allowing the user to set custom sample locations non-dynamically, by filling the extension structs and chaining them to the pipeline structs according to the Vulkan specification section [26.5. Custom Sample Locations] for the following structures: 'VkPipelineSampleLocationsStateCreateInfoEXT' 'VkSampleLocationsInfoEXT' 'VkSampleLocationEXT' Once custom locations are used, the default locations are lost and need to be re-emitted again in the next pipeline creation. For that, we emit the 3DSTATE_SAMPLE_PATTERN at every pipeline creation. --- src/intel/common/gen_sample_positions.h | 53 src/intel/vulkan/anv_genX.h | 5 ++ src/intel/vulkan/anv_private.h | 9 +++ src/intel/vulkan/anv_sample_locations.c | 38 +++- src/intel/vulkan/anv_sample_locations.h | 29 + src/intel/vulkan/genX_pipeline.c| 80 + src/intel/vulkan/genX_state.c | 59 ++ 7 files changed, 259 insertions(+), 14 deletions(-) create mode 100644 src/intel/vulkan/anv_sample_locations.h diff --git a/src/intel/common/gen_sample_positions.h b/src/intel/common/gen_sample_positions.h index da48dcb5ed0..e8af2a552dc 100644 --- a/src/intel/common/gen_sample_positions.h +++ b/src/intel/common/gen_sample_positions.h @@ -160,4 +160,57 @@ prefix##14YOffset = 0.9375; \ prefix##15XOffset = 0.0625; \ prefix##15YOffset = 0.; +/* Examples: + * in case of GEN_GEN < 8: + * SET_SAMPLE_POS(ms.Sample, 0); expands to: + *ms.Sample0XOffset = anv_samples[0].offs_x; + *ms.Sample0YOffset = anv_samples[0].offs_y; + * + * in case of GEN_GEN >= 8: + * SET_SAMPLE_POS(sp._16xSample, 0); expands to: + *sp._16xSample0XOffset = anv_samples[0].offs_x; + *sp._16xSample0YOffset = anv_samples[0].offs_y; + */ +#define SET_SAMPLE_POS(prefix, sample_idx) \ +prefix##sample_idx##XOffset = anv_samples[sample_idx].offs_x; \ +prefix##sample_idx##YOffset = anv_samples[sample_idx].offs_y; + +#define SET_SAMPLE_POS_2X(prefix) \ +SET_SAMPLE_POS(prefix, 0); \ +SET_SAMPLE_POS(prefix, 1); + +#define SET_SAMPLE_POS_4X(prefix) \ +SET_SAMPLE_POS(prefix, 0); \ +SET_SAMPLE_POS(prefix, 1); \ +SET_SAMPLE_POS(prefix, 2); \ +SET_SAMPLE_POS(prefix, 3); + +#define SET_SAMPLE_POS_8X(prefix) \ +SET_SAMPLE_POS(prefix, 0); \ +SET_SAMPLE_POS(prefix, 1); \ +SET_SAMPLE_POS(prefix, 2); \ +SET_SAMPLE_POS(prefix, 3); \ +SET_SAMPLE_POS(prefix, 4); \ +SET_SAMPLE_POS(prefix, 5); \ +SET_SAMPLE_POS(prefix, 6); \ +SET_SAMPLE_POS(prefix, 7); + +#define SET_SAMPLE_POS_16X(prefix) \ +SET_SAMPLE_POS(prefix, 0); \ +SET_SAMPLE_POS(prefix, 1); \ +SET_SAMPLE_POS(prefix, 2); \ +SET_SAMPLE_POS(prefix, 3); \ +SET_SAMPLE_POS(prefix, 4); \ +SET_SAMPLE_POS(prefix, 5); \ +SET_SAMPLE_POS(prefix, 6); \ +SET_SAMPLE_POS(prefix, 7); \ +SET_SAMPLE_POS(prefix, 8); \ +SET_SAMPLE_POS(prefix, 9); \ +SET_SAMPLE_POS(prefix, 10); \ +SET_SAMPLE_POS(prefix, 11); \ +SET_SAMPLE_POS(prefix, 12); \ +SET_SAMPLE_POS(prefix, 13); \ +SET_SAMPLE_POS(prefix, 14); \ +SET_SAMPLE_POS(prefix, 15); + #endif /* GEN_SAMPLE_POSITIONS_H */ diff --git a/src/intel/vulkan/anv_genX.h b/src/intel/vulkan/anv_genX.h index 8fd32cabf1e..52415c04a45 100644 --- a/src/intel/vulkan/anv_genX.h +++ b/src/intel/vulkan/anv_genX.h @@ -88,3 +88,8 @@ void genX(cmd_buffer_mi_memset)(struct anv_cmd_buffer *cmd_buffer, void genX(blorp_exec)(struct blorp_batch *batch, const struct blorp_params *params); + +void genX(emit_sample_locations)(struct anv_batch *batch, + uint32_t num_samples, + const VkSampleLocationsInfoEXT *sl, + bool custom_locations); diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h index 5905299e59d..981956e5706 100644 --- a/src/intel/vulkan/anv_private.h +++ b/src/intel/vulkan/anv_private.h @@ -71,6 +71,7 @@ struct anv_buffer; struct anv_buffer_view; struct anv_image_view; struct anv_instance; +struct anv_sample; struct gen_l3_config; @@ -165,6 +166,7 @@ struct gen_l3_config; #define MAX_PUSH_DESCRIPTORS 32 /* Minimum requirement */ #define MAX_INLINE_UNIFORM_BLOCK_SIZE 4096 #define MAX_INLINE_UNIFORM_BLOCK_DESCRIPTORS 32 +#define MAX_SAMPLE_LOCATIONS 16 /* The kernel relocation API has a limitation of a 32-bit delta value * applied to the address before it is written which, in spite of it being @@ -2086,6 +2088,13 @@ struct anv_push_constants { struct brw_image_param images[MAX_GEN8_IMAGES]; }; +struct +anv_sample { + float offs_x; + float offs_y; + float radius; +}; + struct anv_dynamic_state { struct { uint32_t count; diff --git a/src/intel/vulkan/anv_sample_locations.c b/src/intel/vulkan/anv_sample_locations.c index 1ebf280e05b..c660cb5ae84 100644 --- a/src/intel/vulkan/anv_sample_locations.c +++ b/src/intel/vulkan/anv_sample_locations.c @@ -21,7 +21,7 @@ * IN THE SOFTWARE. */ -#include "anv_private.h" +#i