On Fri, May 06, 2016 at 10:19:58AM +0300, Pohjolainen, Topi wrote: > On Thu, May 05, 2016 at 10:59:16AM -0700, Ben Widawsky wrote: > > On Thu, May 05, 2016 at 10:51:32AM -0700, Ben Widawsky wrote: > > > On Thu, Apr 21, 2016 at 02:58:57PM +0300, Topi Pohjolainen wrote: > > > > Signed-off-by: Topi Pohjolainen <topi.pohjolai...@intel.com> > > > > --- > > > > src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 7 ++++++- > > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c > > > > b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c > > > > index ae08300..b68575f 100644 > > > > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c > > > > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c > > > > @@ -556,8 +556,13 @@ intel_miptree_create_layout(struct brw_context > > > > *brw, > > > > } else if (brw->gen >= 9 && num_samples > 1) { > > > > layout_flags |= MIPTREE_LAYOUT_FORCE_HALIGN16; > > > > } else { > > > > + const bool is_lossless_compressed_aux = > > > > + brw->gen >= 9 && num_samples == 1 && > > > > + mt->format == MESA_FORMAT_R_UINT32; > > > > + > > > > /* For now, nothing else has this requirement */ > > > > - assert((layout_flags & MIPTREE_LAYOUT_FORCE_HALIGN16) == 0); > > > > + assert(is_lossless_compressed_aux || > > > > + (layout_flags & MIPTREE_LAYOUT_FORCE_HALIGN16) == 0); > > > > } > > > > > > > > brw_miptree_layout(brw, mt, layout_flags); > > > > > > I'd just drop the else condition entirely instead. The else case > > > assertion also > > > becomes invalid when we want to fast clear multi LOD and arrayed > > > surfaces. I'm > > > slightly worried that the assertion is somewhat frail and not future > > > proof. > > > > > > Also the format check makes me wonder if you want to add some assertion or > > > condition to intel_miptree_is_lossless_compressed? ie. > > > > > > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c > > > b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c > > > index 17c87a2..467a60b 100644 > > > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c > > > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c > > > @@ -296,6 +296,9 @@ intel_miptree_is_lossless_compressed(const struct > > > brw_context *brw, > > > if (mt->msaa_layout != INTEL_MSAA_LAYOUT_CMS) > > > return false; > > > > > > + if (mt->format == MESA_FORMAT_R_UINT32) > > > + return false; > > > + > > > /* And finally distinguish between msaa and single sample case. */ > > > return mt->num_samples <= 1; > > > } > > > > > > Either way: > > > Reviewed-by: Ben Widawsky <b...@bwidawsk.net> > > > > Hmm. On second thought, doesn't this still violate what the spec says? > > "When Auxiliary Surface Mode is set to AUX_CCS_D or AUX_CCS_E, HALIGN 16 > > must be > > used." > > I'm not sure if I understood this question correctly. The rational for the > patch is that intel_miptree_alloc_non_msrt_mcs() calls miptree creation > for the aux the buffer with MIPTREE_LAYOUT_FORCE_HALIGN16. This ends up into > the branch addressed above. Format in question in MESA_FORMAT_R_UINT32 for > which neither of the first two conditions apply: > > if (intel_miptree_supports_non_msrt_fast_clear(brw, mt)) { > ... > } else if (brw->gen >= 9 && num_samples > 1) { > ... > } else { > > and hence it drops to the default where the assertion hits unless it is > relaxed. >
Forgive me, I misread the assertion actually. You already have my r-b whether or not you drop the else entirely... -- Ben Widawsky, Intel Open Source Technology Center _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev