[Mesa-dev] [PATCH v5 05/19] intel: Use Clear Color struct size.
Jason Ekstrand
jason at jlekstrand.net
Tue Apr 3 21:52:23 UTC 2018
On Thu, Mar 29, 2018 at 10:58 AM, Rafael Antognolli <
rafael.antognolli at 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.
>
> v5 (Jason):
> - Split genxml changes to another commit.
> - Remove unnecessary gen checks.
> - Bring back missing offset increment to init_fast_clear_color().
>
> [jordan.l.justen at intel.com: isl_device_init changes]
> Signed-off-by: Rafael Antognolli <rafael.antognolli at intel.com>
> Signed-off-by: Jordan Justen <jordan.l.justen at intel.com>
> ---
> src/intel/blorp/blorp_genX_exec.h | 5 +++--
> src/intel/isl/isl.c | 4 ++++
> 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 | 23 ++++++++++++-----------
> 6 files changed, 35 insertions(+), 15 deletions(-)
>
> diff --git a/src/intel/blorp/blorp_genX_exec.h
> b/src/intel/blorp/blorp_genX_exec.h
> index 992bc9959a1..f3a96fbd58c 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/isl/isl.c b/src/intel/isl/isl.c
> index 1a32c028183..875c691b43e 100644
> --- a/src/intel/isl/isl.c
> +++ b/src/intel/isl/isl.c
> @@ -73,6 +73,10 @@ 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_color_state_size = CLEAR_COLOR_length(info) * 4;
> + dev->ss.clear_color_state_offset =
> + RENDER_SURFACE_STATE_ClearValueAddress_start(info) / 32 * 4;
> +
> dev->ss.clear_value_size =
> isl_align(RENDER_SURFACE_STATE_RedClearColor_bits(info) +
> RENDER_SURFACE_STATE_GreenClearColor_bits(info) +
> 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 3c803178c41..08e4362b028 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..9411854b7e5 100644
> --- a/src/intel/vulkan/genX_cmd_buffer.c
> +++ b/src/intel/vulkan/genX_cmd_buffer.c
> @@ -826,35 +826,36 @@ 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;
>
How about removing "addr.offset += 4" from below and just add
sdi.Address.offset += i * 4;
here?
> /* MCS buffers on SKL+ can only have 1/0 clear colors. */
> assert(image->samples > 1);
> sdi.ImmediateData = 0;
> - } else if (GEN_VERSIONx10 >= 75) {
> + addr.offset += 4;
> + }
> + }
> + } else {
> + anv_batch_emit(&cmd_buffer->batch, GENX(MI_STORE_DATA_IMM), sdi) {
> + sdi.Address = addr;
> + if (GEN_VERSIONx10 >= 75) {
>
GEN_GEN >= 8 || GEN_IS_HASWELL
We try to avoid using the GEN_VERSIONx10 macros directly as we may want to
do something else in the future besides multiplying by 10. This also
better matches the kind of gen check you would see if you were using
devinfo.
> /* 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) {
>
} else {
With those three changes made, 4 and 5 are
Reviewed-by: Jason Ekstrand <jason at jlekstrand.net>
> /* 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;
> }
> }
>
> --
> 2.14.3
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180403/97f3287c/attachment.html>
More information about the mesa-dev
mailing list