[Mesa-dev] [PATCH 2/2] genxml: Make BLEND_STATE command support variable length array.
Rafael Antognolli
rafael.antognolli at intel.com
Wed Apr 12 21:40:32 UTC 2017
On Wed, Apr 12, 2017 at 10:45:58AM -0700, Jason Ekstrand wrote:
> On Fri, Apr 7, 2017 at 9:52 AM, Rafael Antognolli <rafael.antognolli at intel.com>
> wrote:
>
> We need to emit BLEND_STATE, which size is 1 + 2 * nr_draw_buffers
> dwords (on gen8+), but the BLEND_STATE struct length is always 17. By
> marking it size 1, which is actually the size of the struct minus the
> BLEND_STATE_ENTRY's, we can emit a BLEND_STATE of variable number of
> entries.
>
> For gen6 and gen7 we set length to 0, since it only contains
> BLEND_STATE_ENTRY's, and no other data.
>
> With this change, we also change the code for blorp and anv to emit only
> the needed BLEND_STATE_ENTRY's, instead of always emitting 16 dwords on
> gen6-7 and 17 dwords on gen8+.
>
> Signed-off-by: Rafael Antognolli <rafael.antognolli at intel.com>
> ---
> src/intel/blorp/blorp_genX_exec.h | 35 ++++++++++++---------
> src/intel/genxml/gen6.xml | 4 +-
> src/intel/genxml/gen7.xml | 4 +-
> src/intel/genxml/gen75.xml | 4 +-
> src/intel/genxml/gen8.xml | 4 +-
> src/intel/genxml/gen9.xml | 4 +-
> src/intel/vulkan/genX_pipeline.c | 53 ++++++++++++++++----------------
> 7 files changed, 58 insertions(+), 50 deletions(-)
>
> diff --git a/src/intel/blorp/blorp_genX_exec.h b/src/intel/blorp/
> blorp_genX_exec.h
> index 3791462..fc1856f 100644
> --- a/src/intel/blorp/blorp_genX_exec.h
> +++ b/src/intel/blorp/blorp_genX_exec.h
> @@ -902,23 +902,30 @@ blorp_emit_blend_state(struct blorp_batch *batch,
> struct GENX(BLEND_STATE) blend;
> memset(&blend, 0, sizeof(blend));
>
> + uint32_t offset;
> + int size = GENX(BLEND_STATE_length) * 4;
> + size += GENX(BLEND_STATE_ENTRY_length) * 4 * params->num_draw_buffers;
> + uint32_t *state = blorp_alloc_dynamic_state(batch, size, 64, &offset);
> + uint32_t *pos = state;
> +
> + GENX(BLEND_STATE_pack)(NULL, pos, &blend);
> + pos += GENX(BLEND_STATE_length);
> +
> for (unsigned i = 0; i < params->num_draw_buffers; ++i) {
> - blend.Entry[i].PreBlendColorClampEnable = true;
> - blend.Entry[i].PostBlendColorClampEnable = true;
> - blend.Entry[i].ColorClampRange = COLORCLAMP_RTFORMAT;
> -
> - blend.Entry[i].WriteDisableRed = params->color_write_disable[0];
> - blend.Entry[i].WriteDisableGreen = params->color_write_disable[1];
> - blend.Entry[i].WriteDisableBlue = params->color_write_disable[2];
> - blend.Entry[i].WriteDisableAlpha = params->color_write_disable[3];
> + struct GENX(BLEND_STATE_ENTRY) entry = { 0 };
> + entry.PreBlendColorClampEnable = true;
> + entry.PostBlendColorClampEnable = true;
> + entry.ColorClampRange = COLORCLAMP_RTFORMAT;
> +
> + entry.WriteDisableRed = params->color_write_disable[0];
> + entry.WriteDisableGreen = params->color_write_disable[1];
> + entry.WriteDisableBlue = params->color_write_disable[2];
> + entry.WriteDisableAlpha = params->color_write_disable[3];
>
>
> Might be a tiny bit nicer to use a designated initializer here.
Ack.
>
>
> + GENX(BLEND_STATE_ENTRY_pack)(NULL, pos, &entry);
> + pos += GENX(BLEND_STATE_ENTRY_length);
> }
>
> - uint32_t offset;
> - void *state = blorp_alloc_dynamic_state(batch,
> - GENX(BLEND_STATE_length) * 4,
> - 64, &offset);
> - GENX(BLEND_STATE_pack)(NULL, state, &blend);
> - blorp_flush_range(batch, state, GENX(BLEND_STATE_length) * 4);
> + blorp_flush_range(batch, state, size);
>
> #if GEN_GEN >= 7
> blorp_emit(batch, GENX(3DSTATE_BLEND_STATE_POINTERS), sp) {
> diff --git a/src/intel/genxml/gen6.xml b/src/intel/genxml/gen6.xml
> index 5083f07..3059bfc 100644
> --- a/src/intel/genxml/gen6.xml
> +++ b/src/intel/genxml/gen6.xml
> @@ -452,8 +452,8 @@
> <field name="Post-Blend Color Clamp Enable" start="32" end="32" type=
> "bool"/>
> </struct>
>
> - <struct name="BLEND_STATE" length="16">
> - <group count="8" start="0" size="64">
> + <struct name="BLEND_STATE" length="0">
> + <group count="0" start="0" size="64">
> <field name="Entry" start="0" end="63" type="BLEND_STATE_ENTRY"/>
> </group>
> </struct>
> diff --git a/src/intel/genxml/gen7.xml b/src/intel/genxml/gen7.xml
> index ada8f74..867a1d4 100644
> --- a/src/intel/genxml/gen7.xml
> +++ b/src/intel/genxml/gen7.xml
> @@ -507,8 +507,8 @@
> <field name="Post-Blend Color Clamp Enable" start="32" end="32" type=
> "bool"/>
> </struct>
>
> - <struct name="BLEND_STATE" length="16">
> - <group count="8" start="0" size="64">
> + <struct name="BLEND_STATE" length="0">
> + <group count="0" start="0" size="64">
> <field name="Entry" start="0" end="63" type="BLEND_STATE_ENTRY"/>
> </group>
> </struct>
> diff --git a/src/intel/genxml/gen75.xml b/src/intel/genxml/gen75.xml
> index 16d2d74..594e539 100644
> --- a/src/intel/genxml/gen75.xml
> +++ b/src/intel/genxml/gen75.xml
> @@ -517,8 +517,8 @@
> <field name="Post-Blend Color Clamp Enable" start="32" end="32" type=
> "bool"/>
> </struct>
>
> - <struct name="BLEND_STATE" length="16">
> - <group count="8" start="0" size="64">
> + <struct name="BLEND_STATE" length="0">
> + <group count="0" start="0" size="64">
> <field name="Entry" start="0" end="63" type="BLEND_STATE_ENTRY"/>
> </group>
> </struct>
> diff --git a/src/intel/genxml/gen8.xml b/src/intel/genxml/gen8.xml
> index 1390fe6..4985342 100644
> --- a/src/intel/genxml/gen8.xml
> +++ b/src/intel/genxml/gen8.xml
> @@ -546,7 +546,7 @@
> <field name="Write Disable Blue" start="0" end="0" type="bool"/>
> </struct>
>
> - <struct name="BLEND_STATE" length="17">
> + <struct name="BLEND_STATE" length="1">
> <field name="Alpha To Coverage Enable" start="31" end="31" type="bool"
> />
> <field name="Independent Alpha Blend Enable" start="30" end="30" type=
> "bool"/>
> <field name="Alpha To One Enable" start="29" end="29" type="bool"/>
> @@ -556,7 +556,7 @@
> <field name="Color Dither Enable" start="23" end="23" type="bool"/>
> <field name="X Dither Offset" start="21" end="22" type="uint"/>
> <field name="Y Dither Offset" start="19" end="20" type="uint"/>
> - <group count="8" start="32" size="64">
> + <group count="0" start="32" size="64">
> <field name="Entry" start="0" end="63" type="BLEND_STATE_ENTRY"/>
> </group>
> </struct>
> diff --git a/src/intel/genxml/gen9.xml b/src/intel/genxml/gen9.xml
> index 4bf0fb6..a620e78 100644
> --- a/src/intel/genxml/gen9.xml
> +++ b/src/intel/genxml/gen9.xml
> @@ -555,7 +555,7 @@
> <field name="Write Disable Blue" start="0" end="0" type="bool"/>
> </struct>
>
> - <struct name="BLEND_STATE" length="17">
> + <struct name="BLEND_STATE" length="1">
> <field name="Alpha To Coverage Enable" start="31" end="31" type="bool"
> />
> <field name="Independent Alpha Blend Enable" start="30" end="30" type=
> "bool"/>
> <field name="Alpha To One Enable" start="29" end="29" type="bool"/>
> @@ -565,7 +565,7 @@
> <field name="Color Dither Enable" start="23" end="23" type="bool"/>
> <field name="X Dither Offset" start="21" end="22" type="uint"/>
> <field name="Y Dither Offset" start="19" end="20" type="uint"/>
> - <group count="8" start="32" size="64">
> + <group count="0" start="32" size="64">
> <field name="Entry" start="0" end="63" type="BLEND_STATE_ENTRY"/>
> </group>
> </struct>
> diff --git a/src/intel/vulkan/genX_pipeline.c b/src/intel/vulkan/genX_
> pipeline.c
> index 3fd1333..894d584 100644
> --- a/src/intel/vulkan/genX_pipeline.c
> +++ b/src/intel/vulkan/genX_pipeline.c
> @@ -862,28 +862,14 @@ emit_cb_state(struct anv_pipeline *pipeline,
> {
> struct anv_device *device = pipeline->device;
>
> - const uint32_t num_dwords = GENX(BLEND_STATE_length);
> - pipeline->blend_state =
> - anv_state_pool_alloc(&device->dynamic_state_pool, num_dwords * 4,
> 64);
>
> struct GENX(BLEND_STATE) blend_state = {
> #if GEN_GEN >= 8
> .AlphaToCoverageEnable = ms_info && ms_info->alphaToCoverageEnable,
> .AlphaToOneEnable = ms_info && ms_info->alphaToOneEnable,
> -#else
> - /* Make sure it gets zeroed */
> - .Entry = { { 0, }, },
> #endif
> };
>
> - /* Default everything to disabled */
> - for (uint32_t i = 0; i < 8; i++) {
> - blend_state.Entry[i].WriteDisableAlpha = true;
> - blend_state.Entry[i].WriteDisableRed = true;
> - blend_state.Entry[i].WriteDisableGreen = true;
> - blend_state.Entry[i].WriteDisableBlue = true;
>
>
> I think there was some reason why I filled out all 8 of them but I can't
> remember what it was. If this passes the Vulkan CTS, it's probably fine.
It doesn't show up in the patch, but when 'entry' is declared later with
designated initalizers, it already sets these to:
.WriteDisableRed = params->color_write_disable[0],
So I thought it should be fine to remove this initialization. But I
might be missing something else. I'm waiting for results from the Vulkan
CTS anyway.
>
> - }
> -
> uint32_t surface_count = 0;
> struct anv_pipeline_bind_map *map;
> if (anv_pipeline_has_stage(pipeline, MESA_SHADER_FRAGMENT)) {
> @@ -891,7 +877,17 @@ emit_cb_state(struct anv_pipeline *pipeline,
> surface_count = map->surface_count;
> }
>
> + const uint32_t num_dwords = GENX(BLEND_STATE_length) +
> + GENX(BLEND_STATE_ENTRY_length) * surface_count;
> + pipeline->blend_state =
> + anv_state_pool_alloc(&device->dynamic_state_pool, num_dwords * 4,
> 64);
> +
> bool has_writeable_rt = false;
> + uint32_t *state_pos = pipeline->blend_state.map;
> + state_pos += GENX(BLEND_STATE_length);
> +#if GEN_GEN >= 8
> + struct GENX(BLEND_STATE_ENTRY) bs0 = { 0 };
> +#endif
> for (unsigned i = 0; i < surface_count; i++) {
> struct anv_pipeline_binding *binding = &map->surface_to_descriptor
> [i];
>
> @@ -909,7 +905,7 @@ emit_cb_state(struct anv_pipeline *pipeline,
> const VkPipelineColorBlendAttachmentState *a =
> &info->pAttachments[binding->index];
>
> - blend_state.Entry[i] = (struct GENX(BLEND_STATE_ENTRY)) {
> + struct GENX(BLEND_STATE_ENTRY) entry = { 0,
>
>
> You don't need the extra 0 there. Designated initializers (as used below) will
> take care of initializing the rest to 0.
Ugh, OK, sorry for this. Will fix on next version.
>
> #if GEN_GEN < 8
> .AlphaToCoverageEnable = ms_info && ms_info->
> alphaToCoverageEnable,
> .AlphaToOneEnable = ms_info && ms_info->alphaToOneEnable,
> @@ -938,7 +934,7 @@ emit_cb_state(struct anv_pipeline *pipeline,
> #if GEN_GEN >= 8
> blend_state.IndependentAlphaBlendEnable = true;
> #else
> - blend_state.Entry[i].IndependentAlphaBlendEnable = true;
> + entry.IndependentAlphaBlendEnable = true;
> #endif
> }
>
> @@ -953,26 +949,31 @@ emit_cb_state(struct anv_pipeline *pipeline,
> */
> if (a->colorBlendOp == VK_BLEND_OP_MIN ||
> a->colorBlendOp == VK_BLEND_OP_MAX) {
> - blend_state.Entry[i].SourceBlendFactor = BLENDFACTOR_ONE;
> - blend_state.Entry[i].DestinationBlendFactor = BLENDFACTOR_ONE;
> + entry.SourceBlendFactor = BLENDFACTOR_ONE;
> + entry.DestinationBlendFactor = BLENDFACTOR_ONE;
> }
> if (a->alphaBlendOp == VK_BLEND_OP_MIN ||
> a->alphaBlendOp == VK_BLEND_OP_MAX) {
> - blend_state.Entry[i].SourceAlphaBlendFactor = BLENDFACTOR_ONE;
> - blend_state.Entry[i].DestinationAlphaBlendFactor =
> BLENDFACTOR_ONE;
> + entry.SourceAlphaBlendFactor = BLENDFACTOR_ONE;
> + entry.DestinationAlphaBlendFactor = BLENDFACTOR_ONE;
> }
> + GENX(BLEND_STATE_ENTRY_pack)(NULL, state_pos, &entry);
> + state_pos += GENX(BLEND_STATE_ENTRY_length);
> +#if GEN_GEN >= 8
> + if (i == 0)
> + bs0 = entry;
> +#endif
>
>
> This is a bit annoying but I don't see a good way around it.
Yeah, unfortunately :(
Lionel's idea would be the closest solution imho. But we would have to
add code to read each field manually, unless we had the genxml also
generate code to unpack such structs.
>
> }
>
> #if GEN_GEN >= 8
> - struct GENX(BLEND_STATE_ENTRY) *bs0 = &blend_state.Entry[0];
> anv_batch_emit(&pipeline->batch, GENX(3DSTATE_PS_BLEND), blend) {
> blend.AlphaToCoverageEnable = blend_state.
> AlphaToCoverageEnable;
> blend.HasWriteableRT = has_writeable_rt;
> - blend.ColorBufferBlendEnable = bs0->ColorBufferBlendEnable;
> - blend.SourceAlphaBlendFactor = bs0->SourceAlphaBlendFactor;
> - blend.DestinationAlphaBlendFactor = bs0->
> DestinationAlphaBlendFactor;
> - blend.SourceBlendFactor = bs0->SourceBlendFactor;
> - blend.DestinationBlendFactor = bs0->DestinationBlendFactor;
> + blend.ColorBufferBlendEnable = bs0.ColorBufferBlendEnable;
> + blend.SourceAlphaBlendFactor = bs0.SourceAlphaBlendFactor;
> + blend.DestinationAlphaBlendFactor = bs0.
> DestinationAlphaBlendFactor;
> + blend.SourceBlendFactor = bs0.SourceBlendFactor;
> + blend.DestinationBlendFactor = bs0.DestinationBlendFactor;
> blend.AlphaTestEnable = false;
> blend.IndependentAlphaBlendEnable =
> blend_state.IndependentAlphaBlendEnable;
> --
> git-series 0.9.1
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>
More information about the mesa-dev
mailing list