[Mesa-dev] [PATCH 4/6] isl: Let isl_surf_init's caller set the exact row pitch

[Chad Versace]

On Mon 06 Mar 2017, Jason Ekstrand wrote:
> On Mon, Mar 6, 2017 at 10:12 AM, Chad Versace <chadversary at chromium.org>
> wrote:
> 
> > The caller does so by setting the new field
> > isl_surf_init_info::row_pitch, which overrides isl's row pitch
> > calculation.
> > ---
> >  src/intel/isl/isl.c | 11 ++++++++++-
> >  src/intel/isl/isl.h |  9 +++++++++
> >  2 files changed, 19 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/intel/isl/isl.c b/src/intel/isl/isl.c
> > index a91c3f92bcf..b3ac305b81b 100644
> > --- a/src/intel/isl/isl.c
> > +++ b/src/intel/isl/isl.c
> > @@ -1005,6 +1005,9 @@ isl_calc_linear_row_pitch(const struct isl_device
> > *dev,
> >  {
> >     const struct isl_format_layout *fmtl = isl_format_get_layout(info->
> > format);
> >
> > +   /* Any override of row_pitch should happen earlier. */
> > +   assert(info->row_pitch == 0);
> > +
> >     uint32_t row_pitch = info->min_row_pitch;
> >
> >     /* First, align the surface to a cache line boundary, as the PRM
> > explains
> > @@ -1088,6 +1091,9 @@ isl_calc_tiled_row_pitch(const struct isl_device
> > *dev,
> >  {
> >     const struct isl_format_layout *fmtl = isl_format_get_layout(info->
> > format);
> >
> > +   /* Any override of row_pitch should happen earlier. */
> > +   assert(info->row_pitch == 0);
> > +
> >     assert(fmtl->bpb % tile_info->format_bpb == 0);
> >     assert(phys_slice0_sa->w % fmtl->bw == 0);
> >
> > @@ -1112,7 +1118,10 @@ isl_calc_row_pitch(const struct isl_device *dev,
> >                     const struct isl_tile_info *tile_info,
> >                     const struct isl_extent2d *phys_slice0_sa)
> >  {
> > -   if (tile_info->tiling == ISL_TILING_LINEAR) {
> > +   if (info->row_pitch != 0) {
> > +      /* Override the usual calculation and validation. */
> > +      return info->row_pitch;
> > +   } else if (tile_info->tiling == ISL_TILING_LINEAR) {
> >        return isl_calc_linear_row_pitch(dev, info, phys_slice0_sa);
> >     } else {
> >        return isl_calc_tiled_row_pitch(dev, info, tile_info,
> > phys_slice0_sa);
> > diff --git a/src/intel/isl/isl.h b/src/intel/isl/isl.h
> > index e517f11cf5e..5f73639fb48 100644
> > --- a/src/intel/isl/isl.h
> > +++ b/src/intel/isl/isl.h
> > @@ -813,6 +813,15 @@ struct isl_surf_init_info {
> >     /** Lower bound for isl_surf::row_pitch, in bytes. */
> >     uint32_t min_row_pitch;
> >
> > +   /**
> > +    * Exact value for isl_surf::row_pitch. Ignored if zero.
> > +    *
> > +    * WARNING: If set, then isl_surf_init() skips many calculations and
> > +    * validations. If the given row pitch is incompatible with the
> > requested
> > +    * surface, then behavior is undefined.
> >
> 
> I'm not a fan.  I would much rather supplying a row_pitch mean "please
> validate and use this" rather than "trust me".  I don't want isl_surf_init
> to succeed and still give you a bad surface.  Sorry if that means real
> work. :-/

Ok. Here's my suggestion.

I can fix this patch so that isl_surf_init() continues to calculate the
row pitch, just as it always has. But treat that calculated row pitch as
the minimum required pitch. If the requested row pitch is 0, use the min
pitch. If the user actually requests a row pitch, then check that the
requested row pitch is aligned correctly and meets the min pitch
requirement.

Do you see a cleaner way?