[Mesa-dev] [PATCH 3/4] intel/isl: Refactor gen6_choose_image_alignment_el
Jason Ekstrand
jason at jlekstrand.net
Sat May 13 17:48:36 UTC 2017
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.
> > > 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170513/55255a72/attachment-0001.html>
More information about the mesa-dev
mailing list