<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Apr 3, 2018 at 8:23 PM, Pohjolainen, Topi <span dir="ltr"><<a href="mailto:topi.pohjolainen@gmail.com" target="_blank">topi.pohjolainen@gmail.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, Apr 03, 2018 at 02:55:31PM -0700, Jason Ekstrand wrote:<br>
> On Tue, Apr 3, 2018 at 8:05 AM, Pohjolainen, Topi <<br>
> <a href="mailto:topi.pohjolainen@gmail.com">topi.pohjolainen@gmail.com</a>> wrote:<br>
><br>
> > On Thu, Mar 29, 2018 at 10:58:40AM -0700, Rafael Antognolli wrote:<br>
> > > gen10 can emit the clear color by setting it on a buffer somewhere, and<br>
> > > then adding only the address to the surface state.<br>
> > ><br>
> > > This commit add support for that on isl_surf_fill_state, and if that is<br>
> > > requested, skip setting the clear value itself.<br>
> > ><br>
> > > v2: Add assert to make sure we are at least on gen10.<br>
> > ><br>
> > > Signed-off-by: Rafael Antognolli <<a href="mailto:rafael.antognolli@intel.com">rafael.antognolli@intel.com</a>><br>
> > > Reviewed-by: Jordan Justen <<a href="mailto:jordan.l.justen@intel.com">jordan.l.justen@intel.com</a>><br>
> > > ---<br>
> > >  src/intel/isl/isl.h               |  9 +++++++++<br>
> > >  src/intel/isl/isl_surface_<wbr>state.c | 18 ++++++++++++++----<br>
> > >  2 files changed, 23 insertions(+), 4 deletions(-)<br>
> > ><br>
> > > diff --git a/src/intel/isl/isl.h b/src/intel/isl/isl.h<br>
> > > index 2edf0522e32..c50b78d4701 100644<br>
> > > --- a/src/intel/isl/isl.h<br>
> > > +++ b/src/intel/isl/isl.h<br>
> > > @@ -1307,6 +1307,15 @@ struct isl_surf_fill_state_info {<br>
> > >      */<br>
> > >     union isl_color_value clear_color;<br>
> > ><br>
> > > +   /**<br>
> > > +    * Send only the clear value address<br>
> > > +    *<br>
> > > +    * If set, we only pass the clear address to the GPU and it will<br>
> > fetch it<br>
> > > +    * from wherever it is.<br>
> > > +    */<br>
> > > +   bool use_clear_address;<br>
> > > +   uint64_t clear_address;<br>
> > > +<br>
> > >     /**<br>
> > >      * Surface write disables for gen4-5<br>
> > >      */<br>
> > > diff --git a/src/intel/isl/isl_surface_<wbr>state.c<br>
> > b/src/intel/isl/isl_surface_<wbr>state.c<br>
> > > index 32a5429f2bf..bff9693f02d 100644<br>
> > > --- a/src/intel/isl/isl_surface_<wbr>state.c<br>
> > > +++ b/src/intel/isl/isl_surface_<wbr>state.c<br>
> > > @@ -637,11 +637,21 @@ isl_genX(surf_fill_state_s)(<wbr>const struct<br>
> > isl_device *dev, void *state,<br>
> > >  #endif<br>
> > ><br>
> > >     if (info->aux_usage != ISL_AUX_USAGE_NONE) {<br>
> > > +      if (info->use_clear_address) {<br>
> > > +#if GEN_GEN >= 10<br>
> > > +         s.ClearValueAddressEnable = true;<br>
> ><br>
> > This will set it for multisampled also and upset piglit tests. We need<br>
> > something of this sort:<br>
> ><br>
> >             s.ClearValueAddressEnable = info->aux_usage !=<br>
> > ISL_AUX_USAGE_MCS;<br>
> ><br>
><br>
> Can we assert instead?  If the caller asks for a clear address to be set<br>
> they should get it and not have it magically disabled when they ask for MCS.<br>
<br>
</div></div>Right, here an assert would be just fine. I should have made the comment<br>
in patch in 19 where it belongs. There we start setting the flag<br>
unconditionally for "info->aux_usage != ISL_AUX_USAGE_NONE". If we don't,<br>
then on ICL we start failing multisampling tests.<br></blockquote><div><br></div><div>Is this a known hardware restriction or is there just something weird going on with MCS?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">
><br>
><br>
> > > +         s.ClearValueAddress = info->clear_address;<br>
> > > +#else<br>
> > > +         unreachable("Gen9 and earlier do not support indirect clear<br>
> > colors");<br>
> > > +#endif<br>
> > > +      }<br>
> > >  #if GEN_GEN >= 9<br>
> > > -      s.RedClearColor = info->clear_color.u32[0];<br>
> > > -      s.GreenClearColor = info->clear_color.u32[1];<br>
> > > -      s.BlueClearColor = info->clear_color.u32[2];<br>
> > > -      s.AlphaClearColor = info->clear_color.u32[3];<br>
> > > +      if (!info->use_clear_address) {<br>
> > > +         s.RedClearColor = info->clear_color.u32[0];<br>
> > > +         s.GreenClearColor = info->clear_color.u32[1];<br>
> > > +         s.BlueClearColor = info->clear_color.u32[2];<br>
> > > +         s.AlphaClearColor = info->clear_color.u32[3];<br>
> > > +      }<br>
> > >  #elif GEN_GEN >= 7<br>
> > >        /* Prior to Sky Lake, we only have one bit for the clear color<br>
> > which<br>
> > >         * gives us 0 or 1 in whatever the surface's format happens to be.<br>
> > > --<br>
> > > 2.14.3<br>
> > ><br>
> > > ______________________________<wbr>_________________<br>
> > > mesa-dev mailing list<br>
> > > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a><br>
> > > <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/<wbr>mailman/listinfo/mesa-dev</a><br>
> > ______________________________<wbr>_________________<br>
> > mesa-dev mailing list<br>
> > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a><br>
> > <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/<wbr>mailman/listinfo/mesa-dev</a><br>
> ><br>
</div></div></blockquote></div><br></div></div>