[v4,01/18] anv/image: Do not override lower bits of dword.

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

Details

Message ID 20180308164911.15375-2-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 lower bits seem to have extra fields in every platform but gen8
(even though we don't use them in gen9). So just go ahead and avoid
using them for the address.

v4: Use Jason's suggestion for comment explaining the change.

Signed-off-by: Rafael Antognolli <rafael.antognolli@intel.com>
Reviewed-by: Jason Ekstrand <jason@jlekstrand.net>
Reviewed-by: Topi Pohjolainen <topi.pohjolainen@intel.com>
---
 src/intel/vulkan/anv_image.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/intel/vulkan/anv_image.c b/src/intel/vulkan/anv_image.c
index 4d60f872c1e..b20d791751d 100644
--- a/src/intel/vulkan/anv_image.c
+++ b/src/intel/vulkan/anv_image.c
@@ -1149,19 +1149,16 @@  anv_image_fill_surface_state(struct anv_device *device,
                           .x_offset_sa = tile_x_sa,
                           .y_offset_sa = tile_y_sa);
       state_inout->address = address + offset_B;
-      if (device->info.gen >= 8) {
-         state_inout->aux_address = aux_address;
-      } else {
-         /* On gen7 and prior, the bottom 12 bits of the MCS base address are
-          * used to store other information.  This should be ok, however,
-          * because surface buffer addresses are always 4K page alinged.
-          */
-         uint32_t *aux_addr_dw = state_inout->state.map +
-                                 device->isl_dev.ss.aux_addr_offset;
-         assert((aux_address & 0xfff) == 0);
-         assert(aux_address == (*aux_addr_dw & 0xfffff000));
-         state_inout->aux_address = *aux_addr_dw;
-      }
+
+      /* With the exception of gen8, the bottom 12 bits of the MCS base address
+       * are used to store other information.  This should be ok, however,
+       * because the surface buffer addresses are always 4K page aligned.
+       */
+      uint32_t *aux_addr_dw = state_inout->state.map +
+         device->isl_dev.ss.aux_addr_offset;
+      assert((aux_address & 0xfff) == 0);
+      assert(aux_address == (*aux_addr_dw & 0xfffff000));
+      state_inout->aux_address = *aux_addr_dw;
    }
 
    anv_state_flush(device, state_inout->state);

Comments

On Thu, Mar 8, 2018 at 8:48 AM, Rafael Antognolli <
rafael.antognolli@intel.com> wrote:

> The lower bits seem to have extra fields in every platform but gen8
> (even though we don't use them in gen9). So just go ahead and avoid
> using them for the address.
>
> v4: Use Jason's suggestion for comment explaining the change.
>
> Signed-off-by: Rafael Antognolli <rafael.antognolli@intel.com>
> Reviewed-by: Jason Ekstrand <jason@jlekstrand.net>
> Reviewed-by: Topi Pohjolainen <topi.pohjolainen@intel.com>
> ---
>  src/intel/vulkan/anv_image.c | 23 ++++++++++-------------
>  1 file changed, 10 insertions(+), 13 deletions(-)
>
> diff --git a/src/intel/vulkan/anv_image.c b/src/intel/vulkan/anv_image.c
> index 4d60f872c1e..b20d791751d 100644
> --- a/src/intel/vulkan/anv_image.c
> +++ b/src/intel/vulkan/anv_image.c
> @@ -1149,19 +1149,16 @@ anv_image_fill_surface_state(struct anv_device
> *device,
>                            .x_offset_sa = tile_x_sa,
>                            .y_offset_sa = tile_y_sa);
>        state_inout->address = address + offset_B;
> -      if (device->info.gen >= 8) {
> -         state_inout->aux_address = aux_address;
> -      } else {
> -         /* On gen7 and prior, the bottom 12 bits of the MCS base address
> are
> -          * used to store other information.  This should be ok, however,
> -          * because surface buffer addresses are always 4K page alinged.
> -          */
> -         uint32_t *aux_addr_dw = state_inout->state.map +
> -                                 device->isl_dev.ss.aux_addr_offset;
> -         assert((aux_address & 0xfff) == 0);
> -         assert(aux_address == (*aux_addr_dw & 0xfffff000));
> -         state_inout->aux_address = *aux_addr_dw;
> -      }
> +
> +      /* With the exception of gen8, the bottom 12 bits of the MCS base
> address
> +       * are used to store other information.  This should be ok, however,
> +       * because the surface buffer addresses are always 4K page aligned.
> +       */
>

We should also update the comment on the aux_address field in anv_private.h


> +      uint32_t *aux_addr_dw = state_inout->state.map +
> +         device->isl_dev.ss.aux_addr_offset;
> +      assert((aux_address & 0xfff) == 0);
> +      assert(aux_address == (*aux_addr_dw & 0xfffff000));
> +      state_inout->aux_address = *aux_addr_dw;
>

Now that I think about this a bit harder, I think this is going to get
sticky in the future since it throws away all but the bottom 32 bits.  I
think it's probably ok since we do have a good assert there.  Feel free to
ignore this comment for now.


>     }
>
>     anv_state_flush(device, state_inout->state);
> --
> 2.14.3
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>