[v4,04/18] intel/genxml: Add Clear Color struct.

Submitted by Rafael Antognolli on March 8, 2018, 4:48 p.m.

Details

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

Not browsing as part of any series.

Commit Message

Rafael Antognolli March 8, 2018, 4:48 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.

[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/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(-)

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 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>
+
   <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;
+   }
 
    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;
    }
 }
 

Comments

On Thu, Mar 8, 2018 at 8:48 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.
>
> [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/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
>