<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Feb 27, 2018 at 9:35 AM, Rafael Antognolli <<a href="mailto:rafael.antognolli@intel.com" target="_blank">rafael.antognolli@intel.com</a>> wrote: <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 Mon, Feb 26, 2018 at 05:04:37PM -0800, Jason Ekstrand wrote: > On Wed, Feb 21, 2018 at 1:45 PM, Rafael Antognolli <<a href="mailto:rafael.antognolli@intel.com">rafael.antognolli@intel.com</a> > > 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 <<a href="mailto:rafael.antognolli@intel.com">rafael.antognolli@intel.com</a>> > --- > 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. > > > +  > + </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+ </div></div>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? </blockquote><div> </div><div>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? </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > 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. <div class="HOEnZb"><div class="h5"> > > + /* 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 > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> > > </div></div></blockquote></div> </div></div>