<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jun 16, 2016 at 10:57 AM, Chad Versace <span dir="ltr"><<a href="mailto:chad.versace@intel.com" target="_blank">chad.versace@intel.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class=""><div class="h5">On Thu 16 Jun 2016, Jason Ekstrand wrote:<br>
> On Thu, Jun 16, 2016 at 10:39 AM, Chad Versace <<a href="mailto:chad.versace@intel.com">chad.versace@intel.com</a>><br>
> wrote:<br>
><br>
> > On Sat 11 Jun 2016, Jason Ekstrand wrote:<br>
> > > ---<br>
> > >  src/intel/isl/isl_surface_state.c | 28 ++++++++--------------------<br>
> > >  1 file changed, 8 insertions(+), 20 deletions(-)<br>
> > ><br>
> > > diff --git a/src/intel/isl/isl_surface_state.c<br>
> > b/src/intel/isl/isl_surface_state.c<br>
> > > index 50570aa..1e94e60 100644<br>
> > > --- a/src/intel/isl/isl_surface_state.c<br>
> > > +++ b/src/intel/isl/isl_surface_state.c<br>
> > > @@ -110,9 +110,8 @@ get_surftype(enum isl_surf_dim dim,<br>
> > isl_surf_usage_flags_t usage)<br>
> > >  /*<br>
> > >   * Get the values to pack into<br>
> > RENDER_SUFFACE_STATE.SurfaceHorizontalAlignment<br>
> > >   * and SurfaceVerticalAlignment.<br>
> > >   */<br>
> > > -static void<br>
> > > -get_halign_valign(const struct isl_surf *surf,<br>
> > > -                  uint32_t *halign, uint32_t *valign)<br>
> > > +static struct isl_extent3d<br>
> > > +get_image_alignment(const struct isl_surf *surf)<br>
> ><br>
> ><br>
> > The function comment is incorrect post-patch. It should say something to<br>
> > the tune of "Returns indices into isl_to_gen_halign, isl_to_gen_valign".<br>
> > Specifically, the function comment needs to clarify (with as few words<br>
> > as possible) that the units of the returned extent is neither samples,<br>
> > pixels, nor elements, but something entirely different--array indices--<br>
> > because it's not really an extent at all.<br>
> ><br>
><br>
> Right.  It's the "logical" halign/valign values not the actual hardware<br>
> enums.<br>
<br>
</div></div>But even the term "logical values" is overly ambiguous. Logical values<br>
of *what*? On some gens, the returned value is in units of surface<br>
samples; other gens, surface elements. So, in effect, the returned value<br>
is a *unitless* array index.<br>
</blockquote></div><br></div><div class="gmail_extra">I've updated the comment to the following:<br><br>Get the horizontal and vertical alignment in the units expected by the<br>hardware.  Note that this does NOT give you the actual hardware enum values<br>but an index into the isl_to_gen_[hv]align arrays above.<br><br></div><div class="gmail_extra">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?<br><br></div><div class="gmail_extra">--Jason<br></div></div>