On Fri 17 Jun 2016, Jason Ekstrand wrote: > On Thu, Jun 16, 2016 at 10:57 AM, Chad Versace <chad.vers...@intel.com> > wrote: > > > On Thu 16 Jun 2016, Jason Ekstrand wrote: > > > On Thu, Jun 16, 2016 at 10:39 AM, Chad Versace <chad.vers...@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.vers...@intel.com> _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
