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

[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

[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

[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

[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

[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