On Sat 13 May 2017, Jason Ekstrand wrote: > On Fri, May 12, 2017 at 3:35 PM, Chad Versace <chadvers...@chromium.org> > wrote: > > > On Thu 11 May 2017, Jason Ekstrand wrote: > > > On Thu, May 11, 2017 at 9:08 PM, Chad Versace <chadvers...@chromium.org> > > > wrote: > > > > > > > On Thu 11 May 2017, Jason Ekstrand wrote: > > > > > On Thu, May 11, 2017 at 7:03 AM, Pohjolainen, Topi < > > > > > topi.pohjolai...@gmail.com> wrote: > > > > > > > > > > > On Wed, May 10, 2017 at 02:30:31PM -0700, Jason Ekstrand wrote: > > > > > > > --- > > > > > > > src/intel/isl/isl_gen6.c | 30 ++++++++++++------------------ > > > > > > > 1 file changed, 12 insertions(+), 18 deletions(-) > > > > > > > > > > > > > > diff --git a/src/intel/isl/isl_gen6.c b/src/intel/isl/isl_gen6.c > > > > > > > index b746903..0de9620 100644 > > > > > > > --- a/src/intel/isl/isl_gen6.c > > > > > > > +++ b/src/intel/isl/isl_gen6.c > > > > > > > @@ -88,6 +88,8 @@ isl_gen6_choose_image_alignment_el(const > > struct > > > > > > isl_device *dev, > > > > > > > * | format | halign | valign | > > > > > > > * +------------------------+--------+--------+ > > > > > > > * | YUV 4:2:2 formats | 4 | * | > > > > > > > + * | BC1-5 | 4 | 4 | > > > > > > > + * | FXT1 | 8 | 4 | > > > > > > > * | uncompressed formats | 4 | * | > > > > > > > * +------------------------+--------+--------+ > > > > > > > * > > > > > > > @@ -110,29 +112,13 @@ isl_gen6_choose_image_alignment_el(const > > > > struct > > > > > > isl_device *dev, > > > > > > > */ > > > > > > > > > > > > > > if (isl_format_is_compressed(info->format)) { > > > > > > > + /* Compressed formats have an alignment equal to their > > block > > > > size > > > > > > */ > > > > > > > *image_align_el = isl_extent3d(1, 1, 1); > > > > > > > return; > > > > > > > } > > > > > > > > > > > > > > - if (isl_format_is_yuv(info->format)) { > > > > > > > - *image_align_el = isl_extent3d(4, 2, 1); > > > > > > > - return; > > > > > > > - } > > > > > > > - > > > > > > > - if (info->samples > 1) { > > > > > > > - *image_align_el = isl_extent3d(4, 4, 1); > > > > > > > - return; > > > > > > > - } > > > > > > > - > > > > > > > - if (isl_surf_usage_is_depth_or_stencil(info->usage) && > > > > > > > - !ISL_DEV_USE_SEPARATE_STENCIL(dev)) { > > > > > > > > > > > > Maybe mention in the commit that we drop this as it is always > > false on > > > > > > gen6+? > > > > > > In isl.c: "dev->use_separate_stencil = ISL_DEV_GEN(dev) >= 6;" > > > > > > > > > > > > > > > > No, I dropped it not because we always use separate stencil but > > because > > > > > it's redundant with the regular depth case. The PRM says to use a > > 4x4 > > > > > alignment for all depth buffers but 4x2 for separate stencil. > > Combined > > > > > depth-stencil falls under the "depth" case so I didn't think we > > needed to > > > > > call it out explicitly. > > > > > > > > I admit that the condition I wrote > > > > > > > > if (isl_surf_usage_is_depth_or_stencil(info->usage) && > > > > !ISL_DEV_USE_SEPARATE_STENCIL(dev)) > > > > > > > > was poorly chosen. I should've written it without relying on > > > > ISL_DEV_USE_SEPARATE_STENCIL. > > > > > > > > Topi has a point because a surface with > > > > format=ISL_FORMAT_X24_TYPELESS_G8_UINT will necessarily have > > > > usage=ISL_SURF_USAGE_STENCIL_BIT (no depth bit). The hardware requires > > > > the surface to have valign=4, and my badly written (but correct) > > > > condition ensured valign=4 in this case. After this patch, the function > > > > wrongly chooses valign=2. > > > > > > > > > > No it won't. It's hard to see in patch form, but after this patch is > > > applied we check for depth first and then stencil. If it has ANY depth > > > component, then it will get 4x4 aligned. Only separate stencil gets 4x2. > > > > This function, pre-patch and post-patch, does not inspect the format for > > a depth component. It checks if the depth usage flag is set. That's the > > key difference. > > > > On gen6, it's possible to create a usable surface with format > > X24_TYPELESS_G8_UINT with ISL_SURF_USAGE_STENCIL_BIT and no depth bit. > > And, if I understand this patch correctly, that surface does not satisfy > > the depth check in this patch: > > > > if (isl_surf_usage_is_depth(info->usage)) { > > /* depth buffer (possibly interleaved with stencil) */ > > *image_align_el = isl_extent3d(4, 4, 1); > > return; > > } > > > > Instead, that surface eventually hits this at the bottom of the > > function: > > > > *image_align_el = isl_extent3d(4, 2, 1); > > > > Of course, such a surface is only usable as a stencil buffer if hiz is > > disabled. So we'll probably never observe such a surface in real life. > > But we should at least assert that's the case. > > > > Since this verions preserves the original broken behavior, how about I make > a follow-on patch which changes it to do > > /* Separate stencil requires 4x2 alignment */ > if (isl_surf_usage_is_stencil(info->usage) && info->format == > ISL_FORMAT_R8) { > *image_align_el = isl_extent3d(4, 2, 1); > return; > } > > /* Depth or combined depth stencil surfaces require 4x4 alignment */ > if (isl_surf_usage_is_depth_or_stencil(info->usage) { > *image_align_el = isl_extent3d(4, 4, 1); > return; > } > > which I believe is suggestion b. below. I'd rather keep the actual changes > out of the refactor patch if you don't mind. since the gen6 ISL layout > code isn't currently being used, there's no reason for a backport to stable > or anything like that.
Sounds good to me. With the valign=2 comment updated at the bottom of the function, this patch is: Reviewed-by: Chad Versace <chadvers...@chromium.org> > > > > > > I suggest either > > > > > > > > a. Asserting that separate stencil is true in the neighborhood of > > > > > > > > if (isl_surf_usage_is_depth(info->usage)) { > > > > /* depth buffer (possibly interleaved with stencil) */ > > > > *image_align_el = isl_extent3d(4, 4, 1); > > > > return; > > > > } > > > > > > > > and dropping the reference to possible interleaved stencil. > > > > > > > > b. Or, better, rewrite the original logic to be clearer. > > > > > > > > if ((info->usage & ISL_SURF_USAGE_STENCIL_BIT) > > > > && info->format == ISL_FORMAT_R8_UINT) { > > > > /* separate stencil surface */ > > > > *image_align_el = isl_extent3d(4, 2, 1); > > > > return; > > > > } > > > > > > > > if (info->usage & (ISL_SURF_USAGE_DEPTH_BIT | > > > > ISL_SURF_USAGE_STENCIL_BIT)) > > > > /* separate depth surface or interleaved depthstencil */ > > > > *image_align_el = isl_extent3d(4, 4, 1); > > > > return; > > > > } > > > > > > > > > > Other than that: > > > > > > > > > > > > Reviewed-by: Topi Pohjolainen <topi.pohjolai...@intel.com> > > > > > > > > > > > > > - /* interleaved depthstencil buffer */ > > > > > > > - *image_align_el = isl_extent3d(4, 4, 1); > > > > > > > - return; > > > > > > > - } > > > > > > > - > > > > > > > if (isl_surf_usage_is_depth(info->usage)) { > > > > > > > - /* separate depth buffer */ > > > > > > > + /* depth buffer (possibly interleaved with stencil) */ > > > > > > > *image_align_el = isl_extent3d(4, 4, 1); > > > > > > > return; > > > > > > > } > > > > > > > @@ -143,5 +129,13 @@ isl_gen6_choose_image_alignment_el(const > > struct > > > > > > isl_device *dev, > > > > > > > return; > > > > > > > } > > > > > > > > > > > > > > + if (info->samples > 1) { > > > > > > > + *image_align_el = isl_extent3d(4, 4, 1); > > > > > > > + return; > > > > > > > + } > > > > > > > + > > > > > > > + /* For everything else, we can chose any vertical alignment > > we > > > > > > want. We > > > > > > > + * choose an alignment of 2 because it uses the least memory. > > > > > > > + */ > > > > > > > *image_align_el = isl_extent3d(4, 2, 1); > > > > > > > > The code is correct, but the comment is wrong. We cannot choose any > > > > valign we wish for the remaining cases. It is true that 2 is *valid* > > for > > > > all remaining cases, but it is also *required* for some of those cases, > > > > as explained in the big comment block above. Namely, all render target > > > > surfaces and also separate stencil require valign=2. > > > > > > > > > > Hrm... Ok, I'll try and come up with a better comment. > > > > > _______________________________________________ > > > mesa-dev mailing list > > > mesa-dev@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > > > > > _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev