Re: [Mesa-dev] [PATCH 3/4] intel/isl: Refactor gen6_choose_image_alignment_el

2017-05-15 Thread Chad Versace 
On Sat 13 May 2017, Jason Ekstrand wrote:
> On Fri, May 12, 2017 at 3:35 PM, Chad Versace 
> wrote:
> 
> > On Thu 11 May 2017, Jason Ekstrand wrote:
> > > On Thu, May 11, 2017 at 9:08 PM, Chad Versace 
> > > 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.
> >
> 
>

Re: [Mesa-dev] [PATCH 3/4] intel/isl: Refactor gen6_choose_image_alignment_el

2017-05-13 Thread Jason Ekstrand 
On Fri, May 12, 2017 at 3:35 PM, Chad Versace 
wrote:

> On Thu 11 May 2017, Jason Ekstrand wrote:
> > On Thu, May 11, 2017 at 9:08 PM, Chad Versace 
> > 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);

Re: [Mesa-dev] [PATCH 3/4] intel/isl: Refactor gen6_choose_image_alignment_el

2017-05-12 Thread Chad Versace 
On Thu 11 May 2017, Jason Ekstrand wrote:
> On Thu, May 11, 2017 at 9:08 PM, Chad Versace 
> 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.

> > 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) {

Re: [Mesa-dev] [PATCH 3/4] intel/isl: Refactor gen6_choose_image_alignment_el

2017-05-12 Thread Jason Ekstrand 
On Thu, May 11, 2017 at 9:08 PM, Chad Versace 
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.


> 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 
> > >
> > > > -  /* 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 =

Re: [Mesa-dev] [PATCH 3/4] intel/isl: Refactor gen6_choose_image_alignment_el

2017-05-11 Thread Chad Versace 
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.

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 
> >
> > > -  /* 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.

Re: [Mesa-dev] [PATCH 3/4] intel/isl: Refactor gen6_choose_image_alignment_el

2017-05-11 Thread Jason Ekstrand 
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.


> Other than that:
>
> Reviewed-by: Topi Pohjolainen 
>
> > -  /* 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);
> >  }
> > --
> > 2.5.0.400.gff86faf
> >
> > ___
> > 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

Re: [Mesa-dev] [PATCH 3/4] intel/isl: Refactor gen6_choose_image_alignment_el

2017-05-11 Thread Pohjolainen, Topi 
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;"

Other than that:

Reviewed-by: Topi Pohjolainen 

> -  /* 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);
>  }
> -- 
> 2.5.0.400.gff86faf
> 
> ___
> 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] [PATCH 3/4] intel/isl: Refactor gen6_choose_image_alignment_el

2017-05-10 Thread Jason Ekstrand 
---
 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)) {
-  /* 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);
 }
-- 
2.5.0.400.gff86faf

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev