[Mesa-dev] [PATCH v2 19/35] i965/blorp: Move surface offset calculations into a helper

[Jason Ekstrand]

On Thu, Jul 28, 2016 at 12:21 AM, Pohjolainen, Topi <
topi.pohjolainen at intel.com> wrote:

> On Thu, Jul 28, 2016 at 10:10:29AM +0300, Pohjolainen, Topi wrote:
> > On Tue, Jul 26, 2016 at 03:02:10PM -0700, Jason Ekstrand wrote:
> > > The helper does a full transformation on the surface to turn it into a
> new
> > > 2-D single-layer single-level surface representing the original layer
> and
> > > level in memory.
> > > ---
> > >  src/mesa/drivers/dri/i965/brw_blorp.c | 84
> ++++++++++++++++++-----------------
> > >  1 file changed, 43 insertions(+), 41 deletions(-)
> > >
> > > diff --git a/src/mesa/drivers/dri/i965/brw_blorp.c
> b/src/mesa/drivers/dri/i965/brw_blorp.c
> > > index c8cb41a..8ccb8da 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_blorp.c
> > > +++ b/src/mesa/drivers/dri/i965/brw_blorp.c
> > > @@ -114,6 +114,46 @@ blorp_get_image_offset_sa(struct isl_device
> *dev, const struct isl_surf *surf,
> > >     }
> > >  }
> > >
> > > +static void
> > > +surf_apply_level_layer_offsets(struct isl_device *dev, struct
> isl_surf *surf,
> > > +                               struct isl_view *view, uint32_t
> *byte_offset,
> > > +                               uint32_t *tile_x_sa, uint32_t
> *tile_y_sa)
> > > +{
> > > +   /* This only makes sense for a single level and array slice */
> > > +   assert(view->levels == 1 && view->array_len == 1);
> > > +
> > > +   uint32_t x_offset_sa, y_offset_sa;
> > > +   blorp_get_image_offset_sa(dev, surf, view->base_level,
> > > +                             view->base_array_layer,
> > > +                             &x_offset_sa, &y_offset_sa);
> > > +
> > > +   isl_tiling_get_intratile_offset_sa(dev, surf->tiling,
> view->format,
> > > +                                      surf->row_pitch, x_offset_sa,
> y_offset_sa,
> > > +                                      byte_offset, tile_x_sa,
> tile_y_sa);
> > > +
> > > +   /* Now that that's done, we have a very bare 2-D surface */
> > > +   surf->dim = ISL_SURF_DIM_2D;
> > > +   surf->dim_layout = ISL_DIM_LAYOUT_GEN4_2D;
> > > +
> > > +   surf->logical_level0_px.width =
> > > +      minify(surf->logical_level0_px.width, view->base_level);
> > > +   surf->logical_level0_px.height =
> > > +      minify(surf->logical_level0_px.height, view->base_level);
> > > +   surf->logical_level0_px.depth = 1;
> > > +   surf->logical_level0_px.array_len = 1;
> > > +   surf->levels = 1;
> > > +
> > > +   /* Alignment doesn't matter since we have 1 miplevel and 1 array
> slice so
> > > +    * just pick something that works for everybody.
> >
> > I should have commented this in the original patch that introduced this
> > explanation. You explained it some time ago that this is needed to
> prevent
> > asserts from firing, but not for anything else - "works for everybody"
> is a
> > little vague.
> >
> > > +    */
> > > +   surf->image_alignment_el = isl_extent3d(4, 4, 1);
> > > +
> > > +   /* TODO: surf->physcal_level0_extent_sa? */
> > > +
> > > +   view->base_level = 0;
> > > +   view->base_array_layer = 0;
> > > +}
> > > +
> > >  void
> > >  brw_blorp_surface_info_init(struct brw_context *brw,
> > >                              struct brw_blorp_surface_info *info,
> > > @@ -206,20 +246,9 @@ brw_blorp_surface_info_init(struct brw_context
> *brw,
> > >     }
> > >     }
> > >
> > > -   uint32_t x_offset, y_offset;
> > > -   blorp_get_image_offset_sa(&brw->isl_dev, &info->surf,
> > > -                             level, layer / layer_multiplier,
> > > -                             &x_offset, &y_offset);
> > > -
> > > -   uint32_t mt_x, mt_y;
> > > -   intel_miptree_get_image_offset(mt, level, layer, &mt_x, &mt_y);
> > > -   assert(mt_x == x_offset && mt_y == y_offset);
> > > -
> > > -   isl_tiling_get_intratile_offset_sa(&brw->isl_dev,
> info->surf.tiling,
> > > -                                      info->view.format,
> > > -                                      info->surf.row_pitch, x_offset,
> y_offset,
> > > -                                      &info->bo_offset,
> > > -                                      &info->tile_x_sa,
> &info->tile_y_sa);
> > > +   surf_apply_level_layer_offsets(&brw->isl_dev, &info->surf,
> &info->view,
> > > +                                  &info->bo_offset,
> > > +                                  &info->tile_x_sa, &info->tile_y_sa);
> > >  }
> > >
> > >
> > > @@ -345,35 +374,8 @@ brw_blorp_emit_surface_state(struct brw_context
> *brw,
> > >     struct isl_surf surf = surface->surf;
> > >
> > >     /* Stomp surface dimensions and tiling (if needed) with info from
> blorp */
> > > -   surf.dim = ISL_SURF_DIM_2D;
> > > -   surf.dim_layout = ISL_DIM_LAYOUT_GEN4_2D;
> > >     surf.logical_level0_px.width = surface->width;
> > >     surf.logical_level0_px.height = surface->height;
> > > -   surf.logical_level0_px.depth = 1;
> > > -   surf.logical_level0_px.array_len = 1;
> > > -   surf.levels = 1;
> > > -
> > > -   /* Alignment doesn't matter since we have 1 miplevel and 1 array
> slice so
> > > -    * just pick something that works for everybody.
> > > -    */
> > > -   surf.image_alignment_el = isl_extent3d(4, 4, 1);
> > > -
> > > -   if (brw->gen == 6 && surf.samples > 1) {
> > > -      /* Since gen6 uses INTEL_MSAA_LAYOUT_IMS, width and height are
> measured
> > > -       * in samples.  But SURFACE_STATE wants them in pixels, so we
> need to
> > > -       * divide them each by 2.
> > > -       */
> > > -      surf.logical_level0_px.width /= 2;
> > > -      surf.logical_level0_px.height /= 2;
> >
> > Does this work on its own? While surf_apply_level_layer_offsets() sets
> these
> > you still have the original override in place a few lines earlier?
>
> If this was dropped in the next patch of the series that would make sense.
>

You're right.  I moved the hunk.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160817/8706d2a1/attachment-0001.html>