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

--Jason
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160617/e54d225a/attachment.html>