Re: [Mesa-dev] [PATCH v3 3/7] intel/isl: Add support for 1-D compressed textures

[Nanley Chery]

On Thu, Sep 22, 2016 at 02:12:08PM -0700, Jason Ekstrand wrote:
> On Sep 22, 2016 11:11 PM, "Nanley Chery"  wrote:
> >
> > On Wed, Sep 14, 2016 at 01:28:23PM -0700, Jason Ekstrand wrote:
> > > Compressed 1-D textures are a well-defined thing in both GL and Vulkan.
> >
> > Could you provide a reference? I could not find any documentation on 1D
> > compressed textures. Instead I found that OpenGL disallows them and
> > Vulkan does not explicitly mention them. In OpenGL CompressedTexImage1D
> > returns INVALID_ENUM for ASTC, LATC, S3TC, RGTC, BPTC, and FXT1. The same
^
Sorry, there's an extra ASTC here.

> > return value is implied for ASTC. In Vulkan, drivers are required to
> > support one of the following: BPTC for 2D and 3D images, ASTC LDR for 2D
> > images, or ETC2 and EAC for 2D images.
> 
> OK, so they don't normally exist...
> 
> > Our HW also doesn't support this. RENDER_SURFACE_STATE says:
> > * This field cannot be a compressed (BC*, DXT*, FXT*, ETC*, EAC*) format
> > if the Surface Type is SURFTYPE_1D.
> > * This field cannot be ASTC format if the Surface Type is SURFTYPE_1D.
> 
> Right.  Maybe assert in surface state setup about that.
> 
> > I retract my earlier review feedback - the isl_*_gen9_1d functions
> > should not be updated because ISL_DIM_LAYOUT_GEN9_1D compressed
> > textures cannot exist.
> >
> > >
> > > v2: Fix some asserts (Nanley)
> > >
> > > Signed-off-by: Jason Ekstrand 
> > > ---
> > >  src/intel/isl/isl.c | 12 +++-
> > >  1 file changed, 7 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/src/intel/isl/isl.c b/src/intel/isl/isl.c
> > > index a75fddf..710c990 100644
> > > --- a/src/intel/isl/isl.c
> > > +++ b/src/intel/isl/isl.c
> > > @@ -518,7 +518,6 @@ isl_calc_phys_level0_extent_sa(const struct
> isl_device *dev,
> > >assert(info->height == 1);
> > >assert(info->depth == 1);
> > >assert(info->samples == 1);
> > > -  assert(!isl_format_is_compressed(info->format));
> >
> > If you'd like to make this change for HiZ, I think we should keep this
> > assert and OR in a requirement that the format or tiling is HiZ.
> 
> Eh... We also need it for CCS (both gen9 compression and fast clears) and I
> think the calculations are fairly well-defined.  I don't see why we
> shouldn't just allow it in the calculation code.  It wouldn't hurt to have
> an assert somewhere that prevents creating "normal" compressed 1D
> surfaces.  As I said above, maybe assert in the surface state setup code or
> something.

I think it's fine to allow it in the calculation code for auxiliary
surfaces. The commit message and removal of the assertion implied a
much greater change (to me at least). An assertion in the surface state
file sounds like a good place.

-Nanley

> 
> --Jason
> 
> > -Nanley
> >
> > >
> > >switch (dim_layout) {
> > >case ISL_DIM_LAYOUT_GEN4_3D:
> > > @@ -527,8 +526,8 @@ isl_calc_phys_level0_extent_sa(const struct
> isl_device *dev,
> > >case ISL_DIM_LAYOUT_GEN9_1D:
> > >case ISL_DIM_LAYOUT_GEN4_2D:
> > >   *phys_level0_sa = (struct isl_extent4d) {
> > > -.w = info->width,
> > > -.h = 1,
> > > +.w = isl_align_npot(info->width, fmtl->bw),
> > > +.h = fmtl->bh,
> > >  .d = 1,
> > >  .a = info->array_len,
> > >   };
> > > @@ -757,7 +756,7 @@ isl_calc_phys_slice0_extent_sa_gen9_1d(
> > >  {
> > > MAYBE_UNUSED const struct isl_format_layout *fmtl =
> isl_format_get_layout(info->format);
> > >
> > > -   assert(phys_level0_sa->height == 1);
> > > +   assert(phys_level0_sa->height == fmtl->bh);
> > > assert(phys_level0_sa->depth == 1);
> > > assert(info->samples == 1);
> > > assert(image_align_sa->w >= fmtl->bw);
> > > @@ -1567,9 +1566,12 @@ get_image_offset_sa_gen9_1d(const struct
> isl_surf *surf,
> > >  uint32_t *x_offset_sa,
> > >  uint32_t *y_offset_sa)
> > >  {
> > > +   MAYBE_UNUSED const struct isl_format_layout *fmtl =
> > > +  isl_format_get_layout(surf->format);
> > > +
> > > assert(level < surf->levels);
> > > assert(layer < surf->phys_level0_sa.array_len);
> > > -   assert(surf->phys_level0_sa.height == 1);
> > > +   assert(surf->phys_level0_sa.height == fmtl->bh);
> > > assert(surf->phys_level0_sa.depth == 1);
> > > assert(surf->samples == 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 v3 3/7] intel/isl: Add support for 1-D compressed textures

[Jason Ekstrand]

On Sep 22, 2016 11:11 PM, "Nanley Chery"  wrote:
>
> On Wed, Sep 14, 2016 at 01:28:23PM -0700, Jason Ekstrand wrote:
> > Compressed 1-D textures are a well-defined thing in both GL and Vulkan.
>
> Could you provide a reference? I could not find any documentation on 1D
> compressed textures. Instead I found that OpenGL disallows them and
> Vulkan does not explicitly mention them. In OpenGL CompressedTexImage1D
> returns INVALID_ENUM for ASTC, LATC, S3TC, RGTC, BPTC, and FXT1. The same
> return value is implied for ASTC. In Vulkan, drivers are required to
> support one of the following: BPTC for 2D and 3D images, ASTC LDR for 2D
> images, or ETC2 and EAC for 2D images.

OK, so they don't normally exist...

> Our HW also doesn't support this. RENDER_SURFACE_STATE says:
> * This field cannot be a compressed (BC*, DXT*, FXT*, ETC*, EAC*) format
> if the Surface Type is SURFTYPE_1D.
> * This field cannot be ASTC format if the Surface Type is SURFTYPE_1D.

Right.  Maybe assert in surface state setup about that.

> I retract my earlier review feedback - the isl_*_gen9_1d functions
> should not be updated because ISL_DIM_LAYOUT_GEN9_1D compressed
> textures cannot exist.
>
> >
> > v2: Fix some asserts (Nanley)
> >
> > Signed-off-by: Jason Ekstrand 
> > ---
> >  src/intel/isl/isl.c | 12 +++-
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/intel/isl/isl.c b/src/intel/isl/isl.c
> > index a75fddf..710c990 100644
> > --- a/src/intel/isl/isl.c
> > +++ b/src/intel/isl/isl.c
> > @@ -518,7 +518,6 @@ isl_calc_phys_level0_extent_sa(const struct
isl_device *dev,
> >assert(info->height == 1);
> >assert(info->depth == 1);
> >assert(info->samples == 1);
> > -  assert(!isl_format_is_compressed(info->format));
>
> If you'd like to make this change for HiZ, I think we should keep this
> assert and OR in a requirement that the format or tiling is HiZ.

Eh... We also need it for CCS (both gen9 compression and fast clears) and I
think the calculations are fairly well-defined.  I don't see why we
shouldn't just allow it in the calculation code.  It wouldn't hurt to have
an assert somewhere that prevents creating "normal" compressed 1D
surfaces.  As I said above, maybe assert in the surface state setup code or
something.

--Jason

> -Nanley
>
> >
> >switch (dim_layout) {
> >case ISL_DIM_LAYOUT_GEN4_3D:
> > @@ -527,8 +526,8 @@ isl_calc_phys_level0_extent_sa(const struct
isl_device *dev,
> >case ISL_DIM_LAYOUT_GEN9_1D:
> >case ISL_DIM_LAYOUT_GEN4_2D:
> >   *phys_level0_sa = (struct isl_extent4d) {
> > -.w = info->width,
> > -.h = 1,
> > +.w = isl_align_npot(info->width, fmtl->bw),
> > +.h = fmtl->bh,
> >  .d = 1,
> >  .a = info->array_len,
> >   };
> > @@ -757,7 +756,7 @@ isl_calc_phys_slice0_extent_sa_gen9_1d(
> >  {
> > MAYBE_UNUSED const struct isl_format_layout *fmtl =
isl_format_get_layout(info->format);
> >
> > -   assert(phys_level0_sa->height == 1);
> > +   assert(phys_level0_sa->height == fmtl->bh);
> > assert(phys_level0_sa->depth == 1);
> > assert(info->samples == 1);
> > assert(image_align_sa->w >= fmtl->bw);
> > @@ -1567,9 +1566,12 @@ get_image_offset_sa_gen9_1d(const struct
isl_surf *surf,
> >  uint32_t *x_offset_sa,
> >  uint32_t *y_offset_sa)
> >  {
> > +   MAYBE_UNUSED const struct isl_format_layout *fmtl =
> > +  isl_format_get_layout(surf->format);
> > +
> > assert(level < surf->levels);
> > assert(layer < surf->phys_level0_sa.array_len);
> > -   assert(surf->phys_level0_sa.height == 1);
> > +   assert(surf->phys_level0_sa.height == fmtl->bh);
> > assert(surf->phys_level0_sa.depth == 1);
> > assert(surf->samples == 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 v3 3/7] intel/isl: Add support for 1-D compressed textures

[Nanley Chery]

On Wed, Sep 14, 2016 at 01:28:23PM -0700, Jason Ekstrand wrote:
> Compressed 1-D textures are a well-defined thing in both GL and Vulkan.

Could you provide a reference? I could not find any documentation on 1D
compressed textures. Instead I found that OpenGL disallows them and
Vulkan does not explicitly mention them. In OpenGL CompressedTexImage1D
returns INVALID_ENUM for ASTC, LATC, S3TC, RGTC, BPTC, and FXT1. The same
return value is implied for ASTC. In Vulkan, drivers are required to
support one of the following: BPTC for 2D and 3D images, ASTC LDR for 2D
images, or ETC2 and EAC for 2D images.

Our HW also doesn't support this. RENDER_SURFACE_STATE says: 
* This field cannot be a compressed (BC*, DXT*, FXT*, ETC*, EAC*) format
if the Surface Type is SURFTYPE_1D.
* This field cannot be ASTC format if the Surface Type is SURFTYPE_1D.

I retract my earlier review feedback - the isl_*_gen9_1d functions
should not be updated because ISL_DIM_LAYOUT_GEN9_1D compressed
textures cannot exist.

> 
> v2: Fix some asserts (Nanley)
> 
> Signed-off-by: Jason Ekstrand 
> ---
>  src/intel/isl/isl.c | 12 +++-
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/src/intel/isl/isl.c b/src/intel/isl/isl.c
> index a75fddf..710c990 100644
> --- a/src/intel/isl/isl.c
> +++ b/src/intel/isl/isl.c
> @@ -518,7 +518,6 @@ isl_calc_phys_level0_extent_sa(const struct isl_device 
> *dev,
>assert(info->height == 1);
>assert(info->depth == 1);
>assert(info->samples == 1);
> -  assert(!isl_format_is_compressed(info->format));

If you'd like to make this change for HiZ, I think we should keep this
assert and OR in a requirement that the format or tiling is HiZ.

-Nanley

>  
>switch (dim_layout) {
>case ISL_DIM_LAYOUT_GEN4_3D:
> @@ -527,8 +526,8 @@ isl_calc_phys_level0_extent_sa(const struct isl_device 
> *dev,
>case ISL_DIM_LAYOUT_GEN9_1D:
>case ISL_DIM_LAYOUT_GEN4_2D:
>   *phys_level0_sa = (struct isl_extent4d) {
> -.w = info->width,
> -.h = 1,
> +.w = isl_align_npot(info->width, fmtl->bw),
> +.h = fmtl->bh,
>  .d = 1,
>  .a = info->array_len,
>   };
> @@ -757,7 +756,7 @@ isl_calc_phys_slice0_extent_sa_gen9_1d(
>  {
> MAYBE_UNUSED const struct isl_format_layout *fmtl = 
> isl_format_get_layout(info->format);
>  
> -   assert(phys_level0_sa->height == 1);
> +   assert(phys_level0_sa->height == fmtl->bh);
> assert(phys_level0_sa->depth == 1);
> assert(info->samples == 1);
> assert(image_align_sa->w >= fmtl->bw);
> @@ -1567,9 +1566,12 @@ get_image_offset_sa_gen9_1d(const struct isl_surf 
> *surf,
>  uint32_t *x_offset_sa,
>  uint32_t *y_offset_sa)
>  {
> +   MAYBE_UNUSED const struct isl_format_layout *fmtl =
> +  isl_format_get_layout(surf->format);
> +
> assert(level < surf->levels);
> assert(layer < surf->phys_level0_sa.array_len);
> -   assert(surf->phys_level0_sa.height == 1);
> +   assert(surf->phys_level0_sa.height == fmtl->bh);
> assert(surf->phys_level0_sa.depth == 1);
> assert(surf->samples == 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