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

[Jason Ekstrand]

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?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170308/e67d1dd2/attachment.html>