<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Feb 27, 2018 at 11:55 AM, Rafael Antognolli <span dir="ltr"><<a href="mailto:rafael.antognolli@intel.com" target="_blank">rafael.antognolli@intel.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Tue, Feb 27, 2018 at 11:46:12AM -0800, Jason Ekstrand wrote:<br> > On Tue, Feb 27, 2018 at 9:35 AM, Rafael Antognolli <<a href="mailto:rafael.antognolli@intel.com">rafael.antognolli@intel.com</a><br> > > wrote:<br> ><br> > On Mon, Feb 26, 2018 at 05:04:37PM -0800, Jason Ekstrand wrote:<br> > > On Wed, Feb 21, 2018 at 1:45 PM, Rafael Antognolli <<br> > <a href="mailto:rafael.antognolli@intel.com">rafael.antognolli@intel.com</a><br> > > > wrote:<br> > ><br> > > The size of the clear color struct (expected by the hardware) is 8<br> > > dwords (isl_dev.ss.clear_value_state_<wbr>size here). But we still need to<br> > > track the size of the clear color, used when memcopying it to/from<br> > the<br> > > state buffer. For that we keep isl_dev.ss.clear_value_size.<br> > ><br> > > Signed-off-by: Rafael Antognolli <<a href="mailto:rafael.antognolli@intel.com">rafael.antognolli@intel.com</a>><br> > > ---<br> > > src/intel/genxml/gen10.xml | 8 ++++++++<br> > > src/intel/isl/isl.c | 4 +++-<br> > > src/intel/isl/isl.h | 5 +++++<br> > > src/intel/vulkan/anv_image.c | 2 +-<br> > > src/intel/vulkan/anv_private.h | 2 +-<br> > > 5 files changed, 18 insertions(+), 3 deletions(-)<br> > ><br> > > diff --git a/src/intel/genxml/gen10.xml b/src/intel/genxml/gen10.xml<br> > > index b434d1b0f66..58b83954c4c 100644<br> > > --- a/src/intel/genxml/gen10.xml<br> > > +++ b/src/intel/genxml/gen10.xml<br> > ><br> > ><br> > > We need gen11 as well<br> > ><br> > ><br> > > @@ -809,6 +809,14 @@<br> > > <field name="Alpha Clear Color" start="480" end="511" type="int"<br> > /><br> > > </struct><br> > ><br> > > + <struct name="CLEAR_COLOR" length="8"><br> > > + <field name="Raw Clear Color Red" start="0" end="31" type="int"<br> > /><br> > > + <field name="Raw Clear Color Green" start="32" end="63" type=<br> > "int"/><br> > > + <field name="Raw Clear Color Blue" start="64" end="95" type=<br> > "int"/><br> > > + <field name="Raw Clear Color Alpha" start="96" end="127" type=<br> > "int"/><br> > ><br> > ><br> > > Might be good to put Converted Clear Value Hi/Low in here as well.<br> > ><br> > ><br> > > + <br> > > + </struct><br> > > +<br> > > <struct name="SAMPLER_INDIRECT_STATE_<wbr>BORDER_COLOR" length="4"><br> > > <field name="Border Color Red As S31" start="0" end="31" type=<br> > "int"/><br> > > <field name="Border Color Red As U32" start="0" end="31" type=<br> > "uint"/><br> > > diff --git a/src/intel/isl/isl.c b/src/intel/isl/isl.c<br> > > index 77641a89f86..e94470362e2 100644<br> > > --- a/src/intel/isl/isl.c<br> > > +++ b/src/intel/isl/isl.c<br> > > @@ -79,9 +79,10 @@ isl_device_init(struct isl_device *dev,<br> > > * - 2 dwords that can be used by the hardware for converted<br> > clear<br> > > color<br> > > * + some extra bits.<br> > > */<br> > > - dev->ss.clear_value_size = 8 * 4;<br> > > + dev->ss.clear_value_size = 4 * 4;<br> > > dev->ss.clear_value_offset =<br> > > RENDER_SURFACE_STATE_<wbr>ClearValueAddress_start(info) / 32 *<br> > 4;<br> > > + dev->ss.clear_value_state_size = CLEAR_COLOR_length(info) * 4;<br> > > } else {<br> > > dev->ss.clear_value_size =<br> > > isl_align(RENDER_SURFACE_<wbr>STATE_RedClearColor_bits(info) +<br> > > @@ -90,6 +91,7 @@ isl_device_init(struct isl_device *dev,<br> > > RENDER_SURFACE_STATE_<wbr>AlphaClearColor_bits(info),<br> > 32) /<br> > > 8;<br> > > dev->ss.clear_value_offset =<br> > > RENDER_SURFACE_STATE_<wbr>RedClearColor_start(info) / 32 * 4;<br> > > + dev->ss.clear_value_state_size = dev->ss.clear_value_size;<br> > ><br> > ><br> > > Ugh... Let's just make these two separate things. clear_value_size will<br> > be 4 *<br> > > 4 on gen9-10 and clear_color_state_size will be 8*4 on gen10+<br> ><br> > I'm not sure I understand/agree with you here. clear_value_size should<br> > be 4 * 4 everywhere, since we use this to memcpy the 4 dwords of the<br> > clear color. So, are you suggesting we remove clear_value_state_size<br> > from gen9?<br> ><br> ><br> > I mean that we have two separate things here: clear_value_size which is 4 B on<br> > gen7-8, 16 B on gen9-10, and doesn't exist on gen11+ and clear_color_state_size<br> > which doesn't exist prior to gen10 and is 32 B on gen10+. Does that make more<br> > sense?<br> <br> </div></div>Hmm... I was planning to use clear_value_size on gen11+ as well. If I'm<br> not wrong, there are some loops where we cycle through the dwords in the<br> clear color state buffer using clear_value_size as a limit, but we can<br> simply drop it and use 4 (dwords). But yeah, I can drop it...<br></blockquote><div><br></div><div>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<br><br></div><div>if (GEN_GEN <= 8) {<br></div><div> /* Fill out the one dword */<br></div><div>} else {<br></div><div> for (unsigned i = 0; i < 4; i++) {<br></div><div> /* Fill out the dwords */<br></div><div> }<br></div><div>}<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> I agree with clear_color_state_size, though.<br> <div class="HOEnZb"><div class="h5"><br> > > I think this means dropping the previous patch entirely and just making<br> > this<br> > > patch add a size field.<br> ><br> > Ack.<br> ><br> > > }<br> > > assert(RENDER_SURFACE_STATE_<wbr>SurfaceBaseAddress_start(info) % 8 ==<br> > 0);<br> > > dev->ss.addr_offset =<br> </div></div></blockquote></div><br></div></div>