[Mesa-dev] [3.1/22] i965/miptree: Take interleaving into account in stencil pitch

Jason Ekstrand jason at jlekstrand.net
Thu Jul 20 04:54:27 UTC 2017


On Wed, Jul 19, 2017 at 1:37 PM, Pohjolainen, Topi <
topi.pohjolainen at gmail.com> wrote:

> On Wed, Jul 19, 2017 at 01:27:16PM -0700, Jason Ekstrand wrote:
> > On Wed, Jul 19, 2017 at 12:51 PM, Topi Pohjolainen <
> > topi.pohjolainen at gmail.com> wrote:
> >
> > > This makes intel_mipmap_tree::pitch and isl_surf::row_pitch
> > > semantically equivalent.
> > >
> > > Signed-off-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
> > > ---
> > >  src/mesa/drivers/dri/i965/gen7_misc_state.c   | 12 +------
> > >  src/mesa/drivers/dri/i965/gen8_depth_state.c  | 16 +--------
> > >  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 50
> > > ++++++++++-----------------
> > >  3 files changed, 20 insertions(+), 58 deletions(-)
> > >
> > > diff --git a/src/mesa/drivers/dri/i965/gen7_misc_state.c
> > > b/src/mesa/drivers/dri/i965/gen7_misc_state.c
> > > index 6c69fa8ba5..e189788a88 100644
> > > --- a/src/mesa/drivers/dri/i965/gen7_misc_state.c
> > > +++ b/src/mesa/drivers/dri/i965/gen7_misc_state.c
> > > @@ -170,19 +170,9 @@ gen7_emit_depth_stencil_hiz(struct brw_context
> *brw,
> > >
> > >        BEGIN_BATCH(3);
> > >        OUT_BATCH(GEN7_3DSTATE_STENCIL_BUFFER << 16 | (3 - 2));
> > > -      /* The stencil buffer has quirky pitch requirements.  From the
> > > -       * Sandybridge PRM, Volume 2 Part 1, page 329
> > > (3DSTATE_STENCIL_BUFFER
> > > -       * dword 1 bits 16:0 - Surface Pitch):
> > > -       *
> > > -       *    The pitch must be set to 2x the value computed based on
> > > width, as
> > > -       *    the stencil buffer is stored with two rows interleaved.
> > > -       *
> > > -       * While the Ivybridge PRM lacks this comment, the BSpec
> contains
> > > the
> > > -       * same text, and experiments indicate that this is necessary.
> > > -       */
> > >        OUT_BATCH(enabled |
> > >                  mocs << 25 |
> > > -               (2 * stencil_mt->pitch - 1));
> > > +               (stencil_mt->pitch - 1));
> > >        OUT_RELOC(stencil_mt->bo,
> > >                 I915_GEM_DOMAIN_RENDER, I915_GEM_DOMAIN_RENDER,
> > >                 0);
> > > diff --git a/src/mesa/drivers/dri/i965/gen8_depth_state.c
> > > b/src/mesa/drivers/dri/i965/gen8_depth_state.c
> > > index 52c6dd0787..d05e1ba32e 100644
> > > --- a/src/mesa/drivers/dri/i965/gen8_depth_state.c
> > > +++ b/src/mesa/drivers/dri/i965/gen8_depth_state.c
> > > @@ -111,22 +111,8 @@ emit_depth_packets(struct brw_context *brw,
> > >     } else {
> > >        BEGIN_BATCH(5);
> > >        OUT_BATCH(GEN7_3DSTATE_STENCIL_BUFFER << 16 | (5 - 2));
> > > -      /* The stencil buffer has quirky pitch requirements.  From the
> > > Graphics
> > > -       * BSpec: vol2a.11 3D Pipeline Windower > Early Depth/Stencil
> > > Processing
> > > -       * > Depth/Stencil Buffer State > 3DSTATE_STENCIL_BUFFER
> [DevIVB+],
> > > -       * field "Surface Pitch":
> > > -       *
> > > -       *    The pitch must be set to 2x the value computed based on
> > > width, as
> > > -       *    the stencil buffer is stored with two rows interleaved.
> > > -       *
> > > -       * (Note that it is not 100% clear whether this intended to
> apply to
> > > -       * Gen7; the BSpec flags this comment as "DevILK,DevSNB" (which
> > > would
> > > -       * imply that it doesn't), however the comment appears on a
> > > "DevIVB+"
> > > -       * page (which would imply that it does).  Experiments with the
> > > hardware
> > > -       * indicate that it does.
> > > -       */
> > >        OUT_BATCH(HSW_STENCIL_ENABLED | mocs_wb << 22 |
> > > -                (2 * stencil_mt->pitch - 1));
> > > +                (stencil_mt->pitch - 1));
> > >        OUT_RELOC64(stencil_mt->bo,
> > >                    I915_GEM_DOMAIN_RENDER, I915_GEM_DOMAIN_RENDER, 0);
> > >        OUT_BATCH(stencil_mt ? stencil_mt->qpitch >> 2 : 0);
> > > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > index 925c67fc50..9244a35d4f 100644
> > > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > @@ -858,6 +858,18 @@ miptree_create(struct brw_context *brw,
> > >                                          mt->surf.tiling),
> > >                                       &mt->pitch,
> > >                                       alloc_flags);
> > > +
> > > +      /* The stencil buffer has quirky pitch requirements.  From the
> > > +       * Sandybridge PRM, Volume 2 Part 1, page 329
> > > (3DSTATE_STENCIL_BUFFER
> > > +       * dword 1 bits 16:0 - Surface Pitch):
> > > +       *
> > > +       *    The pitch must be set to 2x the value computed based on
> > > width, as
> > > +       *    the stencil buffer is stored with two rows interleaved.
> > > +       *
> > > +       * While the Ivybridge PRM lacks this comment, the BSpec
> contains
> > > the
> > > +       * same text, and experiments indicate that this is necessary.
> > > +       */
> > > +      mt->pitch *= 2;
>
> Here we make it to what gpu wants.
>
> > >     } else {
> > >        mt->bo = brw_bo_alloc_tiled_2d(brw->bufmgr, "miptree",
> > >                                       mt->total_width,
> mt->total_height,
> > > @@ -2819,7 +2831,7 @@ intel_offset_S8(uint32_t stride, uint32_t x,
> > > uint32_t y, bool swizzled)
> > >     uint32_t tile_size = 4096;
> > >     uint32_t tile_width = 64;
> > >     uint32_t tile_height = 64;
> > > -   uint32_t row_size = 64 * stride;
> > > +   uint32_t row_size = 64 * stride / 2; /* Two rows are interleaved.
> */
>
> And here we take into account the "pitch-for-two-interleaved rows" for
> byte offset calculation.
>
> > >
> > >     uint32_t tile_x = x / tile_width;
> > >     uint32_t tile_y = y / tile_height;
> > > @@ -3197,12 +3209,8 @@ intel_miptree_map_s8(struct brw_context *brw,
> > >      * temporary buffer back out.
> > >      */
> > >     if (!(map->mode & GL_MAP_INVALIDATE_RANGE_BIT)) {
> > > -      /* ISL uses a stencil pitch value that is expected by hardware
> > > whereas
> > > -       * traditional miptree uses half of that. Below the value gets
> > > supplied
> > > -       * to intel_offset_S8() which expects the legacy interpretation.
> > > -       */
> > >        const unsigned pitch = mt->surf.size > 0 ?
> > > -                             mt->surf.row_pitch / 2 : mt->pitch;
> > > +                             mt->surf.row_pitch : mt->pitch;
> > >
> >
> > Wait, don't you want to add a "/ 2" to mt->pitch not remove it from
> > surf.row_pitch?
>
> It needs to get removed. See above.
>

Ok, I missed the interaction with offset_S8.  This looks good

Reviewed-by: Jason Ekstrand <jason at jlekstrand.net>


> >
> >
> > >        uint8_t *untiled_s8_map = map->ptr;
> > >        uint8_t *tiled_s8_map = intel_miptree_map_raw(brw, mt,
> > > GL_MAP_READ_BIT);
> > >        unsigned int image_x, image_y;
> > > @@ -3239,12 +3247,8 @@ intel_miptree_unmap_s8(struct brw_context *brw,
> > >                        unsigned int slice)
> > >  {
> > >     if (map->mode & GL_MAP_WRITE_BIT) {
> > > -      /* ISL uses a stencil pitch value that is expected by hardware
> > > whereas
> > > -       * traditional miptree uses half of that. Below the value gets
> > > supplied
> > > -       * to intel_offset_S8() which expects the legacy interpretation.
> > > -       */
> > >        const unsigned pitch = mt->surf.size > 0 ?
> > > -                             mt->surf.row_pitch / 2: mt->pitch;
> > > +                             mt->surf.row_pitch : mt->pitch;
> > >
> >
> > Same here.
> >
> >
> > >        unsigned int image_x, image_y;
> > >        uint8_t *untiled_s8_map = map->ptr;
> > >        uint8_t *tiled_s8_map = intel_miptree_map_raw(brw, mt,
> > > GL_MAP_WRITE_BIT);
> > > @@ -3352,12 +3356,8 @@ intel_miptree_map_depthstencil(struct
> brw_context
> > > *brw,
> > >      * temporary buffer back out.
> > >      */
> > >     if (!(map->mode & GL_MAP_INVALIDATE_RANGE_BIT)) {
> > > -      /* ISL uses a stencil pitch value that is expected by hardware
> > > whereas
> > > -       * traditional miptree uses half of that. Below the value gets
> > > supplied
> > > -       * to intel_offset_S8() which expects the legacy interpretation.
> > > -       */
> > >        const unsigned s_pitch = s_mt->surf.size > 0 ?
> > > -                               s_mt->surf.row_pitch / 2 : s_mt->pitch;
> > > +                               s_mt->surf.row_pitch : s_mt->pitch;
> > >
> >
> > And here.
> >
> >
> > >        uint32_t *packed_map = map->ptr;
> > >        uint8_t *s_map = intel_miptree_map_raw(brw, s_mt,
> GL_MAP_READ_BIT);
> > >        uint32_t *z_map = intel_miptree_map_raw(brw, z_mt,
> GL_MAP_READ_BIT);
> > > @@ -3419,12 +3419,8 @@ intel_miptree_unmap_depthstencil(struct
> > > brw_context *brw,
> > >     bool map_z32f_x24s8 = mt->format == MESA_FORMAT_Z_FLOAT32;
> > >
> > >     if (map->mode & GL_MAP_WRITE_BIT) {
> > > -      /* ISL uses a stencil pitch value that is expected by hardware
> > > whereas
> > > -       * traditional miptree uses half of that. Below the value gets
> > > supplied
> > > -       * to intel_offset_S8() which expects the legacy interpretation.
> > > -       */
> > >        const unsigned s_pitch = s_mt->surf.size > 0 ?
> > > -                               s_mt->surf.row_pitch / 2 : s_mt->pitch;
> > > +                               s_mt->surf.row_pitch : s_mt->pitch;
> > >
> >
> > And here.
> >
> >
> > >        uint32_t *packed_map = map->ptr;
> > >        uint8_t *s_map = intel_miptree_map_raw(brw, s_mt,
> GL_MAP_WRITE_BIT);
> > >        uint32_t *z_map = intel_miptree_map_raw(brw, z_mt,
> > > GL_MAP_WRITE_BIT);
> > > @@ -3738,17 +3734,7 @@ intel_miptree_get_isl_surf(struct brw_context
> *brw,
> > >                                           mt->array_layout);
> > >     surf->msaa_layout = mt->surf.msaa_layout;
> > >     surf->tiling = intel_miptree_get_isl_tiling(mt);
> > > -
> > > -   if (mt->format == MESA_FORMAT_S_UINT8) {
> > > -      /* The ISL definition of row_pitch matches the surface state
> pitch
> > > field
> > > -       * a bit better than intel_mipmap_tree.  In particular, ISL
> > > incorporates
> > > -       * the factor of 2 for W-tiling in row_pitch.
> > > -       */
> > > -      surf->row_pitch = 2 * mt->pitch;
> > > -   } else {
> > > -      surf->row_pitch = mt->pitch;
> > > -   }
> > > -
> > > +   surf->row_pitch = mt->pitch;
> > >     surf->format = translate_tex_format(brw, mt->format, false);
> > >
> > >     if (brw->gen >= 9) {
> > > --
> > > 2.11.0
> > >
> > > _______________________________________________
> > > mesa-dev mailing list
> > > mesa-dev at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170719/4f781e3c/attachment-0001.html>


More information about the mesa-dev mailing list