On Thu, Mar 8, 2018 at 8:48 AM, Rafael Antognolli <
rafael.antogno...@intel.com> wrote:
> The size of the clear color struct (expected by the hardware) is 8
> dwords (isl_dev.ss.clear_value_state_size here). But we still need to
> track the size of the clear color, used when memcopying it to/from the
> state buffer. For that we keep isl_dev.ss.clear_value_size.
>
> v4:
> - Add struct to gen11 too (Jason, Jordan)
> - Add field for Converted Clear Color to gen11 (Jason)
> - Add clear_color_state_offset to differentiate from
> clear_value_offset.
> - Fix all the places where clear_value_size was used.
>
> [jordan.l.jus...@intel.com: isl_device_init changes]
> Signed-off-by: Rafael Antognolli <rafael.antogno...@intel.com>
> Signed-off-by: Jordan Justen <jordan.l.jus...@intel.com>
> ---
> src/intel/blorp/blorp_genX_exec.h | 5 +++--
> src/intel/genxml/gen10.xml | 8 ++++++++
> src/intel/genxml/gen11.xml | 10 ++++++++++
> src/intel/isl/isl.c | 23 +++++++++++++++--------
> src/intel/isl/isl.h | 6 ++++++
> src/intel/vulkan/anv_image.c | 6 +++++-
> src/intel/vulkan/anv_private.h | 6 +++++-
> src/intel/vulkan/genX_cmd_buffer.c | 22 +++++++++++-----------
> 8 files changed, 63 insertions(+), 23 deletions(-)
>
> diff --git a/src/intel/blorp/blorp_genX_exec.h
> b/src/intel/blorp/blorp_genX_exec.h
> index 1348659233c..7f5a4539e8d 100644
> --- a/src/intel/blorp/blorp_genX_exec.h
> +++ b/src/intel/blorp/blorp_genX_exec.h
> @@ -310,10 +310,11 @@ blorp_emit_vertex_buffers(struct blorp_batch *batch,
>
> uint32_t num_vbs = 2;
> if (params->dst_clear_color_as_input) {
> + const unsigned clear_color_size =
> + GEN_GEN < 10 ? batch->blorp->isl_dev->ss.clear_value_size : 4 *
> 4;
> blorp_fill_vertex_buffer_state(batch, vb, num_vbs++,
> params->dst.clear_color_addr,
> - batch->blorp->isl_dev->ss.
> clear_value_size,
> - 0);
> + clear_color_size, 0);
> }
>
> const unsigned num_dwords = 1 + num_vbs * GENX(VERTEX_BUFFER_STATE_
> length);
> diff --git a/src/intel/genxml/gen10.xml b/src/intel/genxml/gen10.xml
> index 6302497a5e9..fdb12bace03 100644
> --- a/src/intel/genxml/gen10.xml
> +++ b/src/intel/genxml/gen10.xml
> @@ -584,6 +584,14 @@
> <field name="Alpha Clear Color" start="480" end="511" type="int"/>
> </struct>
>
> + <struct name="CLEAR_COLOR" length="8">
> + <field name="Raw Clear Color Red" start="0" end="31" type="int"/>
> + <field name="Raw Clear Color Green" start="32" end="63" type="int"/>
> + <field name="Raw Clear Color Blue" start="64" end="95" type="int"/>
> + <field name="Raw Clear Color Alpha" start="96" end="127" type="int"/>
> + <!-- Reserved - MBZ -->
> + </struct>
>
genxml changes should probably go in their own commit.
> +
> <struct name="SAMPLER_INDIRECT_STATE_BORDER_COLOR" length="4">
> <field name="Border Color Red As S31" start="0" end="31" type="int"/>
> <field name="Border Color Red As U32" start="0" end="31" type="uint"/>
> diff --git a/src/intel/genxml/gen11.xml b/src/intel/genxml/gen11.xml
> index 023b56f83f1..28b06b8b2e2 100644
> --- a/src/intel/genxml/gen11.xml
> +++ b/src/intel/genxml/gen11.xml
> @@ -586,6 +586,16 @@
> <field name="Alpha Clear Color" start="480" end="511" type="int"/>
> </struct>
>
> + <struct name="CLEAR_COLOR" length="8">
> + <field name="Raw Clear Color Red" start="0" end="31" type="int"/>
> + <field name="Raw Clear Color Green" start="32" end="63" type="int"/>
> + <field name="Raw Clear Color Blue" start="64" end="95" type="int"/>
> + <field name="Raw Clear Color Alpha" start="96" end="127" type="int"/>
> + <!-- This field is used only by the hardware -->
> + <field name="Converted Clear Value Hi/Low" start="128" end="191"
> type="uint"/>
> + <!-- Reserved - MBZ -->
> + </struct>
> +
> <struct name="SAMPLER_INDIRECT_STATE_BORDER_COLOR" length="4">
> <field name="Border Color Red As S31" start="0" end="31" type="int"/>
> <field name="Border Color Red As U32" start="0" end="31" type="uint"/>
> diff --git a/src/intel/isl/isl.c b/src/intel/isl/isl.c
> index 1a32c028183..3566bd3f0dd 100644
> --- a/src/intel/isl/isl.c
> +++ b/src/intel/isl/isl.c
> @@ -73,14 +73,21 @@ isl_device_init(struct isl_device *dev,
> dev->ss.size = RENDER_SURFACE_STATE_length(info) * 4;
> dev->ss.align = isl_align(dev->ss.size, 32);
>
> - dev->ss.clear_value_size =
> - isl_align(RENDER_SURFACE_STATE_RedClearColor_bits(info) +
> - RENDER_SURFACE_STATE_GreenClearColor_bits(info) +
> - RENDER_SURFACE_STATE_BlueClearColor_bits(info) +
> - RENDER_SURFACE_STATE_AlphaClearColor_bits(info), 32) / 8;
> -
> - dev->ss.clear_value_offset =
> - RENDER_SURFACE_STATE_RedClearColor_start(info) / 32 * 4;
> + if (ISL_DEV_GEN(dev) >= 10) {
> + dev->ss.clear_color_state_size = CLEAR_COLOR_length(info) * 4;
> + dev->ss.clear_color_state_offset =
> + RENDER_SURFACE_STATE_ClearValueAddress_start(info) / 32 * 4;
> + }
> +
> + if (ISL_DEV_GEN(dev) <= 10) {
> + dev->ss.clear_value_size =
> + isl_align(RENDER_SURFACE_STATE_RedClearColor_bits(info) +
> + RENDER_SURFACE_STATE_GreenClearColor_bits(info) +
> + RENDER_SURFACE_STATE_BlueClearColor_bits(info) +
> + RENDER_SURFACE_STATE_AlphaClearColor_bits(info), 32)
> / 8;
> + dev->ss.clear_value_offset =
> + RENDER_SURFACE_STATE_RedClearColor_start(info) / 32 * 4;
> + }
>
You don't need the gen checks here the _bits and _start functions will
return 0 on older platforms.
>
> assert(RENDER_SURFACE_STATE_SurfaceBaseAddress_start(info) % 8 == 0);
> dev->ss.addr_offset =
> diff --git a/src/intel/isl/isl.h b/src/intel/isl/isl.h
> index 0da6abb71d4..2edf0522e32 100644
> --- a/src/intel/isl/isl.h
> +++ b/src/intel/isl/isl.h
> @@ -963,6 +963,12 @@ struct isl_device {
> uint8_t aux_addr_offset;
>
> /* Rounded up to the nearest dword to simplify GPU memcpy
> operations. */
> +
> + /* size of the state buffer used to store the clear color + extra
> + * additional space used by the hardware */
> + uint8_t clear_color_state_size;
> + uint8_t clear_color_state_offset;
> + /* size of the clear color itself - used to copy it to/from a BO */
> uint8_t clear_value_size;
> uint8_t clear_value_offset;
> } ss;
> diff --git a/src/intel/vulkan/anv_image.c b/src/intel/vulkan/anv_image.c
> index b20d791751d..d9b5d266020 100644
> --- a/src/intel/vulkan/anv_image.c
> +++ b/src/intel/vulkan/anv_image.c
> @@ -267,8 +267,12 @@ add_aux_state_tracking_buffer(struct anv_image
> *image,
> (image->planes[plane].offset + image->planes[plane].size));
> }
>
> + const unsigned clear_color_state_size = device->info.gen >= 10 ?
> + device->isl_dev.ss.clear_color_state_size :
> + device->isl_dev.ss.clear_value_size;
> +
> /* Clear color and fast clear type */
> - unsigned state_size = device->isl_dev.ss.clear_value_size + 4;
> + unsigned state_size = clear_color_state_size + 4;
>
> /* We only need to track compression on CCS_E surfaces. */
> if (image->planes[plane].aux_usage == ISL_AUX_USAGE_CCS_E) {
> diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_
> private.h
> index ee533581ab4..3af81e16025 100644
> --- a/src/intel/vulkan/anv_private.h
> +++ b/src/intel/vulkan/anv_private.h
> @@ -2606,7 +2606,11 @@ anv_image_get_fast_clear_type_addr(const struct
> anv_device *device,
> {
> struct anv_address addr =
> anv_image_get_clear_color_addr(device, image, aspect);
> - addr.offset += device->isl_dev.ss.clear_value_size;
> +
> + const unsigned clear_color_state_size = device->info.gen >= 10 ?
> + device->isl_dev.ss.clear_color_state_size :
> + device->isl_dev.ss.clear_value_size;
> + addr.offset += clear_color_state_size;
> return addr;
> }
>
> diff --git a/src/intel/vulkan/genX_cmd_buffer.c
> b/src/intel/vulkan/genX_cmd_buffer.c
> index b5741fb8dc1..53d095a6ee2 100644
> --- a/src/intel/vulkan/genX_cmd_buffer.c
> +++ b/src/intel/vulkan/genX_cmd_buffer.c
> @@ -826,35 +826,35 @@ init_fast_clear_color(struct anv_cmd_buffer
> *cmd_buffer,
> */
> struct anv_address addr =
> anv_image_get_clear_color_addr(cmd_buffer->device, image, aspect);
> - unsigned i = 0;
> - for (; i < cmd_buffer->device->isl_dev.ss.clear_value_size; i += 4) {
> - anv_batch_emit(&cmd_buffer->batch, GENX(MI_STORE_DATA_IMM), sdi) {
> - sdi.Address = addr;
>
> - if (GEN_GEN >= 9) {
> + if (GEN_GEN >= 9) {
> + for (unsigned i = 0; i < 4; i++) {
> + anv_batch_emit(&cmd_buffer->batch, GENX(MI_STORE_DATA_IMM),
> sdi) {
> + sdi.Address = addr;
> /* MCS buffers on SKL+ can only have 1/0 clear colors. */
> assert(image->samples > 1);
> sdi.ImmediateData = 0;
> - } else if (GEN_VERSIONx10 >= 75) {
> + }
> + }
> + } else {
> + anv_batch_emit(&cmd_buffer->batch, GENX(MI_STORE_DATA_IMM), sdi) {
> + sdi.Address = addr;
> + if (GEN_VERSIONx10 >= 75) {
> /* Pre-SKL, the dword containing the clear values also
> contains
> * other fields, so we need to initialize those fields to
> match the
> * values that would be in a color attachment.
> */
> - assert(i == 0);
> sdi.ImmediateData = ISL_CHANNEL_SELECT_RED << 25 |
> ISL_CHANNEL_SELECT_GREEN << 22 |
> ISL_CHANNEL_SELECT_BLUE << 19 |
> ISL_CHANNEL_SELECT_ALPHA << 16;
> - } else if (GEN_VERSIONx10 == 70) {
> + } else if (GEN_VERSIONx10 == 70) {
> /* On IVB, the dword containing the clear values also contains
> * other fields that must be zero or can be zero.
> */
> - assert(i == 0);
> sdi.ImmediateData = 0;
> }
> }
> -
> - addr.offset += 4;
>
This needs to be part of your gen >= 9 loop somehow.
> }
> }
>
> --
> 2.14.3
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
