On Tue, Feb 27, 2018 at 11:55 AM, Rafael Antognolli <
rafael.antogno...@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.antogno...@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.antogno...@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.antogno...@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 =
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
