Re: [Mesa-dev] [PATCH 15/64] isl/state: Return an extent3d from the halign/valign helper

2016-06-20 Thread Chad Versace 
On Fri 17 Jun 2016, Jason Ekstrand wrote:
> On Thu, Jun 16, 2016 at 10:57 AM, Chad Versace 
> wrote:
> 
> > On Thu 16 Jun 2016, Jason Ekstrand wrote:
> > > On Thu, Jun 16, 2016 at 10:39 AM, Chad Versace 
> > > wrote:
> > >
> > > > On Sat 11 Jun 2016, Jason Ekstrand wrote:
> > > > > ---
> > > > >  src/intel/isl/isl_surface_state.c | 28 
> > > > >  1 file changed, 8 insertions(+), 20 deletions(-)
> > > > >
> > > > > diff --git a/src/intel/isl/isl_surface_state.c
> > > > b/src/intel/isl/isl_surface_state.c
> > > > > index 50570aa..1e94e60 100644
> > > > > --- a/src/intel/isl/isl_surface_state.c
> > > > > +++ b/src/intel/isl/isl_surface_state.c
> > > > > @@ -110,9 +110,8 @@ get_surftype(enum isl_surf_dim dim,
> > > > isl_surf_usage_flags_t usage)
> > > > >  /*
> > > > >   * Get the values to pack into
> > > > RENDER_SUFFACE_STATE.SurfaceHorizontalAlignment
> > > > >   * and SurfaceVerticalAlignment.
> > > > >   */
> > > > > -static void
> > > > > -get_halign_valign(const struct isl_surf *surf,
> > > > > -  uint32_t *halign, uint32_t *valign)
> > > > > +static struct isl_extent3d
> > > > > +get_image_alignment(const struct isl_surf *surf)
> > > >
> > > >
> > > > The function comment is incorrect post-patch. It should say something
> > to
> > > > the tune of "Returns indices into isl_to_gen_halign,
> > isl_to_gen_valign".
> > > > Specifically, the function comment needs to clarify (with as few words
> > > > as possible) that the units of the returned extent is neither samples,
> > > > pixels, nor elements, but something entirely different--array indices--
> > > > because it's not really an extent at all.
> > > >
> > >
> > > Right.  It's the "logical" halign/valign values not the actual hardware
> > > enums.
> >
> > But even the term "logical values" is overly ambiguous. Logical values
> > of *what*? On some gens, the returned value is in units of surface
> > samples; other gens, surface elements. So, in effect, the returned value
> > is a *unitless* array index.
> >
> 
> I've updated the comment to the following:
> 
> Get the horizontal and vertical alignment in the units expected by the
> hardware.  Note that this does NOT give you the actual hardware enum values
> but an index into the isl_to_gen_[hv]align arrays above.
> 
> I use the term "units expected by the hardware" because they are integer
> values and a unit conversion is involved in their evaluation.  I was
> careful, however, to not exactly how they should be used.  Good enough?

Sounds good to me.
Reviewed-by: Chad Versace 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 15/64] isl/state: Return an extent3d from the halign/valign helper

2016-06-17 Thread Jason Ekstrand 
On Thu, Jun 16, 2016 at 10:57 AM, Chad Versace 
wrote:

> On Thu 16 Jun 2016, Jason Ekstrand wrote:
> > On Thu, Jun 16, 2016 at 10:39 AM, Chad Versace 
> > wrote:
> >
> > > On Sat 11 Jun 2016, Jason Ekstrand wrote:
> > > > ---
> > > >  src/intel/isl/isl_surface_state.c | 28 
> > > >  1 file changed, 8 insertions(+), 20 deletions(-)
> > > >
> > > > diff --git a/src/intel/isl/isl_surface_state.c
> > > b/src/intel/isl/isl_surface_state.c
> > > > index 50570aa..1e94e60 100644
> > > > --- a/src/intel/isl/isl_surface_state.c
> > > > +++ b/src/intel/isl/isl_surface_state.c
> > > > @@ -110,9 +110,8 @@ get_surftype(enum isl_surf_dim dim,
> > > isl_surf_usage_flags_t usage)
> > > >  /*
> > > >   * Get the values to pack into
> > > RENDER_SUFFACE_STATE.SurfaceHorizontalAlignment
> > > >   * and SurfaceVerticalAlignment.
> > > >   */
> > > > -static void
> > > > -get_halign_valign(const struct isl_surf *surf,
> > > > -  uint32_t *halign, uint32_t *valign)
> > > > +static struct isl_extent3d
> > > > +get_image_alignment(const struct isl_surf *surf)
> > >
> > >
> > > The function comment is incorrect post-patch. It should say something
> to
> > > the tune of "Returns indices into isl_to_gen_halign,
> isl_to_gen_valign".
> > > Specifically, the function comment needs to clarify (with as few words
> > > as possible) that the units of the returned extent is neither samples,
> > > pixels, nor elements, but something entirely different--array indices--
> > > because it's not really an extent at all.
> > >
> >
> > Right.  It's the "logical" halign/valign values not the actual hardware
> > enums.
>
> But even the term "logical values" is overly ambiguous. Logical values
> of *what*? On some gens, the returned value is in units of surface
> samples; other gens, surface elements. So, in effect, the returned value
> is a *unitless* array index.
>

I've updated the comment to the following:

Get the horizontal and vertical alignment in the units expected by the
hardware.  Note that this does NOT give you the actual hardware enum values
but an index into the isl_to_gen_[hv]align arrays above.

I use the term "units expected by the hardware" because they are integer
values and a unit conversion is involved in their evaluation.  I was
careful, however, to not exactly how they should be used.  Good enough?

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

Re: [Mesa-dev] [PATCH 15/64] isl/state: Return an extent3d from the halign/valign helper

2016-06-16 Thread Chad Versace 
On Thu 16 Jun 2016, Jason Ekstrand wrote:
> On Thu, Jun 16, 2016 at 10:39 AM, Chad Versace 
> wrote:
> 
> > On Sat 11 Jun 2016, Jason Ekstrand wrote:
> > > ---
> > >  src/intel/isl/isl_surface_state.c | 28 
> > >  1 file changed, 8 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/src/intel/isl/isl_surface_state.c
> > b/src/intel/isl/isl_surface_state.c
> > > index 50570aa..1e94e60 100644
> > > --- a/src/intel/isl/isl_surface_state.c
> > > +++ b/src/intel/isl/isl_surface_state.c
> > > @@ -110,9 +110,8 @@ get_surftype(enum isl_surf_dim dim,
> > isl_surf_usage_flags_t usage)
> > >  /*
> > >   * Get the values to pack into
> > RENDER_SUFFACE_STATE.SurfaceHorizontalAlignment
> > >   * and SurfaceVerticalAlignment.
> > >   */
> > > -static void
> > > -get_halign_valign(const struct isl_surf *surf,
> > > -  uint32_t *halign, uint32_t *valign)
> > > +static struct isl_extent3d
> > > +get_image_alignment(const struct isl_surf *surf)
> >
> >
> > The function comment is incorrect post-patch. It should say something to
> > the tune of "Returns indices into isl_to_gen_halign, isl_to_gen_valign".
> > Specifically, the function comment needs to clarify (with as few words
> > as possible) that the units of the returned extent is neither samples,
> > pixels, nor elements, but something entirely different--array indices--
> > because it's not really an extent at all.
> >
> 
> Right.  It's the "logical" halign/valign values not the actual hardware
> enums.

But even the term "logical values" is overly ambiguous. Logical values
of *what*? On some gens, the returned value is in units of surface
samples; other gens, surface elements. So, in effect, the returned value
is a *unitless* array index.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 15/64] isl/state: Return an extent3d from the halign/valign helper

2016-06-16 Thread Jason Ekstrand 
On Thu, Jun 16, 2016 at 10:39 AM, Chad Versace 
wrote:

> On Sat 11 Jun 2016, Jason Ekstrand wrote:
> > ---
> >  src/intel/isl/isl_surface_state.c | 28 
> >  1 file changed, 8 insertions(+), 20 deletions(-)
> >
> > diff --git a/src/intel/isl/isl_surface_state.c
> b/src/intel/isl/isl_surface_state.c
> > index 50570aa..1e94e60 100644
> > --- a/src/intel/isl/isl_surface_state.c
> > +++ b/src/intel/isl/isl_surface_state.c
> > @@ -110,9 +110,8 @@ get_surftype(enum isl_surf_dim dim,
> isl_surf_usage_flags_t usage)
> >  /*
> >   * Get the values to pack into
> RENDER_SUFFACE_STATE.SurfaceHorizontalAlignment
> >   * and SurfaceVerticalAlignment.
> >   */
> > -static void
> > -get_halign_valign(const struct isl_surf *surf,
> > -  uint32_t *halign, uint32_t *valign)
> > +static struct isl_extent3d
> > +get_image_alignment(const struct isl_surf *surf)
>
>
> The function comment is incorrect post-patch. It should say something to
> the tune of "Returns indices into isl_to_gen_halign, isl_to_gen_valign".
> Specifically, the function comment needs to clarify (with as few words
> as possible) that the units of the returned extent is neither samples,
> pixels, nor elements, but something entirely different--array indices--
> because it's not really an extent at all.
>

Right.  It's the "logical" halign/valign values not the actual hardware
enums.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 15/64] isl/state: Return an extent3d from the halign/valign helper

2016-06-16 Thread Chad Versace 
On Sat 11 Jun 2016, Jason Ekstrand wrote:
> ---
>  src/intel/isl/isl_surface_state.c | 28 
>  1 file changed, 8 insertions(+), 20 deletions(-)
> 
> diff --git a/src/intel/isl/isl_surface_state.c 
> b/src/intel/isl/isl_surface_state.c
> index 50570aa..1e94e60 100644
> --- a/src/intel/isl/isl_surface_state.c
> +++ b/src/intel/isl/isl_surface_state.c
> @@ -110,9 +110,8 @@ get_surftype(enum isl_surf_dim dim, 
> isl_surf_usage_flags_t usage)
>  /*
>   * Get the values to pack into 
> RENDER_SUFFACE_STATE.SurfaceHorizontalAlignment
>   * and SurfaceVerticalAlignment.
>   */
> -static void
> -get_halign_valign(const struct isl_surf *surf,
> -  uint32_t *halign, uint32_t *valign)
> +static struct isl_extent3d
> +get_image_alignment(const struct isl_surf *surf)


The function comment is incorrect post-patch. It should say something to
the tune of "Returns indices into isl_to_gen_halign, isl_to_gen_valign".
Specifically, the function comment needs to clarify (with as few words
as possible) that the units of the returned extent is neither samples,
pixels, nor elements, but something entirely different--array indices--
because it's not really an extent at all.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 15/64] isl/state: Return an extent3d from the halign/valign helper

2016-06-11 Thread Jason Ekstrand 
---
 src/intel/isl/isl_surface_state.c | 28 
 1 file changed, 8 insertions(+), 20 deletions(-)

diff --git a/src/intel/isl/isl_surface_state.c 
b/src/intel/isl/isl_surface_state.c
index 50570aa..1e94e60 100644
--- a/src/intel/isl/isl_surface_state.c
+++ b/src/intel/isl/isl_surface_state.c
@@ -110,9 +110,8 @@ get_surftype(enum isl_surf_dim dim, isl_surf_usage_flags_t 
usage)
  * Get the values to pack into RENDER_SUFFACE_STATE.SurfaceHorizontalAlignment
  * and SurfaceVerticalAlignment.
  */
-static void
-get_halign_valign(const struct isl_surf *surf,
-  uint32_t *halign, uint32_t *valign)
+static struct isl_extent3d
+get_image_alignment(const struct isl_surf *surf)
 {
if (GEN_GEN >= 9) {
   if (isl_tiling_is_std_y(surf->tiling) ||
@@ -121,8 +120,7 @@ get_halign_valign(const struct isl_surf *surf,
   * true alignment is likely outside the enum range of HALIGN* and
   * VALIGN*.
   */
- *halign = 0;
- *valign = 0;
+ return isl_extent3d(0, 0, 0);
   } else {
  /* In Skylake, RENDER_SUFFACE_STATE.SurfaceVerticalAlignment is in 
units
   * of surface elements (not pixels nor samples). For compressed 
formats,
@@ -131,11 +129,7 @@ get_halign_valign(const struct isl_surf *surf,
   * format (ETC2 has a block height of 4), then the vertical alignment 
is
   * 4 compression blocks or, equivalently, 16 pixels.
   */
- struct isl_extent3d image_align_el
-= isl_surf_get_image_alignment_el(surf);
-
- *halign = isl_to_gen_halign[image_align_el.width];
- *valign = isl_to_gen_valign[image_align_el.height];
+ return isl_surf_get_image_alignment_el(surf);
   }
} else {
   /* Pre-Skylake, RENDER_SUFFACE_STATE.SurfaceVerticalAlignment is in
@@ -144,11 +138,7 @@ get_halign_valign(const struct isl_surf *surf,
* format (compressed or not) the vertical alignment is
* 4 pixels.
*/
-  struct isl_extent3d image_align_sa
- = isl_surf_get_image_alignment_sa(surf);
-
-  *halign = isl_to_gen_halign[image_align_sa.width];
-  *valign = isl_to_gen_valign[image_align_sa.height];
+  return isl_surf_get_image_alignment_sa(surf);
}
 }
 
@@ -199,9 +189,6 @@ void
 isl_genX(surf_fill_state_s)(const struct isl_device *dev, void *state,
 const struct isl_surf_fill_state_info *restrict 
info)
 {
-   uint32_t halign, valign;
-   get_halign_valign(info->surf, , );
-
struct GENX(RENDER_SURFACE_STATE) s = { 0 };
 
s.SurfaceType = get_surftype(info->surf->dim, info->view->usage);
@@ -288,8 +275,9 @@ isl_genX(surf_fill_state_s)(const struct isl_device *dev, 
void *state,
   s.MIPCountLOD = MAX(info->view->levels, 1) - 1;
}
 
-   s.SurfaceVerticalAlignment = valign;
-   s.SurfaceHorizontalAlignment = halign;
+   const struct isl_extent3d image_align = get_image_alignment(info->surf);
+   s.SurfaceVerticalAlignment = isl_to_gen_valign[image_align.height];
+   s.SurfaceHorizontalAlignment = isl_to_gen_halign[image_align.width];
 
if (info->surf->tiling == ISL_TILING_W) {
   /* From the Broadwell PRM documentation for this field:
-- 
2.5.0.400.gff86faf

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