[Mesa-dev] [PATCH 3/4] intel/isl: Refactor gen6_choose_image_alignment_el
Chad Versace
chadversary at chromium.org
Mon May 15 20:28:06 UTC 2017
On Sat 13 May 2017, Jason Ekstrand wrote:
> On Fri, May 12, 2017 at 3:35 PM, Chad Versace <chadversary at chromium.org>
> wrote:
>
> > On Thu 11 May 2017, Jason Ekstrand wrote:
> > > On Thu, May 11, 2017 at 9:08 PM, Chad Versace <chadversary at chromium.org>
> > > wrote:
> > >
> > > > On Thu 11 May 2017, Jason Ekstrand wrote:
> > > > > On Thu, May 11, 2017 at 7:03 AM, Pohjolainen, Topi <
> > > > > topi.pohjolainen at 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 <chadversary at 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.pohjolainen at 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 at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >
> >
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list