[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 <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.