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

[Chad Versace]

On Wed 08 Mar 2017, Jason Ekstrand wrote:
> On Wed, Mar 8, 2017 at 4:16 PM, Chad Versace <chadversary at chromium.org>
> wrote:
> 
> > 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?
> >
> 
> Row pitch, in general, is defined by a minimum and an alignment.  When you
> want to choose a row pitch, you allign the minimum to the alignment.  If
> you want to validate a row pitch, you check that it's large enough and
> aligned.  Am I missing something?

Nope. I believe we're seeing the same thing.