[v5,05/19] intel: Use Clear Color struct size.

Submitted by Rafael Antognolli on March 29, 2018, 5:58 p.m.

Details

Message ID 20180329175853.23728-6-rafael.antognolli@intel.com
State New
Headers show
Series "Use clear color address in surface state." ( rev: 7 6 5 ) in Mesa

Not browsing as part of any series.

Commit Message

Rafael Antognolli March 29, 2018, 5:58 p.m.
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@intel.com: isl_device_init changes]
Signed-off-by: Rafael Antognolli <rafael.antognolli@intel.com>
Signed-off-by: Jordan Justen <jordan.l.justen@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(-)

Patch hide | download patch | download mbox

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;
             /* 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) {
             /* 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;
    }
 }
 

Comments

On Thu, Mar 29, 2018 at 10:58 AM, Rafael Antognolli <
rafael.antognolli@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@intel.com: isl_device_init changes]
> Signed-off-by: Rafael Antognolli <rafael.antognolli@intel.com>
> Signed-off-by: Jordan Justen <jordan.l.justen@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@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>