[Mesa-dev] [PATCH v3 05/13] intel/genxml: Add Clear Color struct.

Jason Ekstrand jason at jlekstrand.net
Tue Feb 27 19:46:12 UTC 2018


On Tue, Feb 27, 2018 at 9:35 AM, Rafael Antognolli <
rafael.antognolli at intel.com> wrote:

> On Mon, Feb 26, 2018 at 05:04:37PM -0800, Jason Ekstrand wrote:
> > On Wed, Feb 21, 2018 at 1:45 PM, 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.
> >
> >     Signed-off-by: Rafael Antognolli <rafael.antognolli at intel.com>
> >     ---
> >      src/intel/genxml/gen10.xml     | 8 ++++++++
> >      src/intel/isl/isl.c            | 4 +++-
> >      src/intel/isl/isl.h            | 5 +++++
> >      src/intel/vulkan/anv_image.c   | 2 +-
> >      src/intel/vulkan/anv_private.h | 2 +-
> >      5 files changed, 18 insertions(+), 3 deletions(-)
> >
> >     diff --git a/src/intel/genxml/gen10.xml b/src/intel/genxml/gen10.xml
> >     index b434d1b0f66..58b83954c4c 100644
> >     --- a/src/intel/genxml/gen10.xml
> >     +++ b/src/intel/genxml/gen10.xml
> >
> >
> > We need gen11 as well
> >
> >
> >     @@ -809,6 +809,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"/>
> >
> >
> > Might be good to put Converted Clear Value Hi/Low in here as well.
> >
> >
> >     +    <!-- 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 77641a89f86..e94470362e2 100644
> >     --- a/src/intel/isl/isl.c
> >     +++ b/src/intel/isl/isl.c
> >     @@ -79,9 +79,10 @@ isl_device_init(struct isl_device *dev,
> >             * - 2 dwords that can be used by the hardware for converted
> clear
> >     color
> >             *   + some extra bits.
> >             */
> >     -      dev->ss.clear_value_size = 8 * 4;
> >     +      dev->ss.clear_value_size = 4 * 4;
> >            dev->ss.clear_value_offset =
> >               RENDER_SURFACE_STATE_ClearValueAddress_start(info) / 32 *
> 4;
> >     +      dev->ss.clear_value_state_size = CLEAR_COLOR_length(info) * 4;
> >         } else {
> >            dev->ss.clear_value_size =
> >               isl_align(RENDER_SURFACE_STATE_RedClearColor_bits(info) +
> >     @@ -90,6 +91,7 @@ isl_device_init(struct isl_device *dev,
> >                         RENDER_SURFACE_STATE_AlphaClearColor_bits(info),
> 32) /
> >     8;
> >            dev->ss.clear_value_offset =
> >               RENDER_SURFACE_STATE_RedClearColor_start(info) / 32 * 4;
> >     +      dev->ss.clear_value_state_size = dev->ss.clear_value_size;
> >
> >
> > Ugh... Let's just make these two separate things.  clear_value_size will
> be 4 *
> > 4 on gen9-10 and clear_color_state_size will be 8*4 on gen10+
>
> I'm not sure I understand/agree with you here. clear_value_size should
> be 4 * 4 everywhere, since we use this to memcpy the 4 dwords of the
> clear color. So, are you suggesting we remove clear_value_state_size
> from gen9?
>

I mean that we have two separate things here: clear_value_size which is 4 B
on gen7-8, 16 B on gen9-10, and doesn't exist on gen11+ and
clear_color_state_size which doesn't exist prior to gen10 and is 32 B on
gen10+.  Does that make more sense?


> > I think this means dropping the previous patch entirely and just making
> this
> > patch add a size field.
>
> Ack.
>
> >         }
> >         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 209769a9a99..f1b38efed44 100644
> >     --- a/src/intel/isl/isl.h
> >     +++ b/src/intel/isl/isl.h
> >     @@ -963,6 +963,11 @@ 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 value +
> extra
> >     +       * additional space used by the hardware */
> >     +      uint8_t clear_value_state_size;
> >
> >
> > Maybe call this clear_color_state_size since it is the size of the
> CLEAR_COLOR
> > state.
>
> Ack.
>
> >
> >     +      /* 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 a0aee43bd21..0dafe03442d 100644
> >     --- a/src/intel/vulkan/anv_image.c
> >     +++ b/src/intel/vulkan/anv_image.c
> >     @@ -264,7 +264,7 @@ add_aux_state_tracking_buffer(struct anv_image
> *image,
> >         }
> >
> >         /* Clear color and fast clear type */
> >     -   unsigned state_size = device->isl_dev.ss.clear_value_size + 4;
> >     +   unsigned state_size = device->isl_dev.ss.clear_value_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 37e63f56aa0..b8c381d2665 100644
> >     --- a/src/intel/vulkan/anv_private.h
> >     +++ b/src/intel/vulkan/anv_private.h
> >     @@ -2566,7 +2566,7 @@ 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;
> >     +   addr.offset += device->isl_dev.ss.clear_value_state_size;
> >         return addr;
> >      }
> >
> >     --
> >     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/20180227/f0b40c24/attachment.html>


More information about the mesa-dev mailing list