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

Tue Feb 27 20:06:16 UTC 2018

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

> On Tue, Feb 27, 2018 at 11:46:12AM -0800, Jason Ekstrand wrote:
> > 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?
>
> Hmm... I was planning to use clear_value_size on gen11+ as well. If I'm
> not wrong, there are some loops where we cycle through the dwords in the
> clear color state buffer using clear_value_size as a limit, but we can
> simply drop it and use 4 (dwords). But yeah, I can drop it...
>

I see what you mean.  I'm not quite sure what the right solution is without
knowing exactly what piece of code you're looking at.  The only one I can
think of is init_fast_clear_color and I think the right thing there might
be to just structure the gen split a little differently and do

if (GEN_GEN <= 8) {
   /* Fill out the one dword */
} else {
   for (unsigned i = 0; i < 4; i++) {
      /* Fill out the dwords */
   }
}

> I agree with clear_color_state_size, though.
>
> >     > 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 =
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180227/43bbe10d/attachment-0001.html>