[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 <chad.versace at intel.com>
> wrote:
> 
> > On Thu 16 Jun 2016, Jason Ekstrand wrote:
> > > On Thu, Jun 16, 2016 at 10:39 AM, Chad Versace <chad.versace at intel.com>
> > > 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 <chad.versace at intel.com>