[Mesa-dev] [PATCH 36/64] isl: Add support for filling out surface states all the way back to gen4

[Chad Versace]

On Mon 20 Jun 2016, Jason Ekstrand wrote:
> On Mon, Jun 20, 2016 at 3:53 PM, Chad Versace <chad.versace at intel.com>
> wrote:
> 
> > On Sat 11 Jun 2016, Jason Ekstrand wrote:
> > > ---
> > >  src/intel/isl/Makefile.am         | 12 ++++++++
> > >  src/intel/isl/Makefile.sources    | 13 +++++++--
> > >  src/intel/isl/isl.c               | 28 +++++++++++++++++++
> > >  src/intel/isl/isl_priv.h          | 24 ++++++++++++++++
> > >  src/intel/isl/isl_surface_state.c | 58


> > > +   case 4:
> > > +      if (ISL_DEV_IS_G4X(dev)) {
> > > +         isl_gen4_surf_fill_state_s(dev, state, info);
> > > +      } else {
> > > +         /* G45 surface state is the same as gen5 */
> > > +         isl_gen5_surf_fill_state_s(dev, state, info);
> > > +      }
> > > +      break;
> >
> > The above can't be correct, because...
> >
> >    #define GEN_IS_G4X ((GEN_VERSIONx10) == 45)
> >
> > I think you meant this:
> >
> >    if (ISL_DEV_IS_G4X(dev)) {
> >       /* G45 surface state is the same as gen5 */
> >       isl_gen5_surf_fill_state_s(dev, state, info);
> >    } else {
> >       isl_gen4_surf_fill_state_s(dev, state, info);
> >    }
> >
> 
> You are correct.  The reason it didn't trigger a bug is that the only
> difference bertween gen4 and 4.5 is that x/y offsets are introduced in
> 4.5.  Since we never use ISL for filling out surfaces with x/y offsets on
> gen4.5 with this series, I never caught it.  Thanks!

[snip]

> > >     switch (ISL_DEV_GEN(dev)) {
> > > +   case 4:
> > > +      if (ISL_DEV_IS_G4X(dev)) {
> > > +         isl_gen4_buffer_fill_state_s(state, info);
> > > +      } else {
> > > +         /* G45 surface state is the same as gen5 */
> > > +         isl_gen5_buffer_fill_state_s(state, info);
> > > +      }
> > > +      break;
> >
> > Same as above.
> >
> 
> My local fix just changes buffers to use the gen5 version for 4, 4.5, and 5

Sounds good, as buffers never use x/y offsets.

> > > -static struct isl_extent3d
> > > +static inline struct isl_extent3d
> > >  get_image_alignment(const struct isl_surf *surf)
> > >  {
> > >     if (GEN_GEN >= 9) {
> > > @@ -199,6 +201,23 @@ isl_genX(surf_fill_state_s)(const struct isl_device
> > *dev, void *state,
> > >     s.Width = info->surf->logical_level0_px.width - 1;
> > >     s.Height = info->surf->logical_level0_px.height - 1;
> > >
> > > +   /* In the gen6 PRM Volume 1 Part 1: Graphics Core, Section 7.18.3.7.1
> > > +    * (Surface Arrays For all surfaces other than separate stencil
> > buffer):
> > > +    *
> > > +    * "[DevSNB] Errata: Sampler MSAA Qpitch will be 4 greater than the
> > value
> > > +    *  calculated in the equation above , for every other odd Surface
> > Height
> > > +    *  starting from 1 i.e. 1,5,9,13"
> > > +    *
> > > +    * Since this Qpitch errata only impacts the sampler, we have to
> > adjust the
> > > +    * input for the rendering surface to achieve the same qpitch. For
> > the
> > > +    * affected heights, we increment the height by 1 for the rendering
> > > +    * surface.
> > > +    */
> > > +   if (GEN_GEN == 6 && (info->view->usage &
> > ISL_SURF_USAGE_RENDER_TARGET_BIT) &&
> > > +       info->surf->samples > 1 &&
> > > +       (info->surf->logical_level0_px.height % 4) == 1)
> > > +      s.Height++;
> > > +
> >
> > Ouch, that's a surprising workaround.
> >
> 
> Yes, yes it is.  We'll have to add a similar hack to isl's gen6 layout code.

I believe isl's gen6 layout code already handles this errata, though
I could be remembering incorrectly. Specifically, grep shows that the
prm quote is already present in isl.

> > Waiting to hear your reply on the G4X comment.

With your local G4X changes,
Reviewed-by: Chad Versace <chad.versace at intel.com>