[Mesa-dev] [PATCH 13/18] i965: Pass slice details as parameters for surface setup

[Kenneth Graunke]

On Wednesday, April 29, 2015 08:58:18 AM Pohjolainen, Topi wrote:
> On Tue, Apr 28, 2015 at 02:45:27PM -0700, Kenneth Graunke wrote:
> > On Wednesday, April 22, 2015 11:47:33 PM Topi Pohjolainen wrote:
> > > Also changed a couple of direct shifts into SET_FIELD().
> > > 
> > > Signed-off-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
> > > ---
> > >  src/mesa/drivers/dri/i965/brw_context.h           |  3 ++-
> > >  src/mesa/drivers/dri/i965/brw_wm_surface_state.c  | 30 +++++++++++++----------
> > >  src/mesa/drivers/dri/i965/gen7_wm_surface_state.c | 14 +++++------
> > >  src/mesa/drivers/dri/i965/gen8_surface_state.c    | 10 +++-----
> > >  4 files changed, 29 insertions(+), 28 deletions(-)
> > > 
> > > diff --git a/src/mesa/drivers/dri/i965/brw_context.h b/src/mesa/drivers/dri/i965/brw_context.h
> > > index b90d329..ae28955 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_context.h
> > > +++ b/src/mesa/drivers/dri/i965/brw_context.h
> > > @@ -964,10 +964,11 @@ struct brw_context
> > >     {
> > >        void (*update_texture_surface)(struct brw_context *brw,
> > >                                       const struct intel_mipmap_tree *mt,
> > > -                                     struct gl_texture_object *tObj,
> > >                                       uint32_t tex_format,
> > >                                       bool is_integer_format,
> > >                                       GLenum target, uint32_t effective_depth,
> > > +                                     uint32_t min_layer,
> > > +                                     uint32_t min_lod, uint32_t mip_count, 
> > >                                       int swizzle, uint32_t *surf_offset,
> > >                                       bool for_gather);
> > >        uint32_t (*update_renderbuffer_surface)(struct brw_context *brw,
> > > diff --git a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> > > index f7acad4..ad5ddb5 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> > > +++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> > > @@ -310,16 +310,16 @@ update_buffer_texture_surface(struct gl_context *ctx,
> > >  static void
> > >  brw_update_texture_surface(struct brw_context *brw,
> > >                             const struct intel_mipmap_tree *mt,
> > > -                           struct gl_texture_object *tObj,
> > >                             uint32_t tex_format,
> > >                             bool is_integer_format /* unused */,
> > >                             GLenum target,
> > >                             uint32_t effective_depth /* unused */,
> > > +                           uint32_t min_layer /* unused */,
> > > +                           uint32_t min_lod, uint32_t mip_count, 
> > >                             int swizzle /* unused */,
> > >                             uint32_t *surf_offset,
> > >                             bool for_gather)
> > >  {
> > > -   struct intel_texture_object *intelObj = intel_texture_object(tObj);
> > >     uint32_t *surf;
> > >  
> > >     surf = brw_state_batch(brw, AUB_TRACE_SURFACE_STATE,
> > > @@ -361,16 +361,16 @@ brw_update_texture_surface(struct brw_context *brw,
> > >  
> > >     surf[1] = mt->bo->offset64 + mt->offset; /* reloc */
> > >  
> > > -   surf[2] = ((intelObj->_MaxLevel - tObj->BaseLevel) << BRW_SURFACE_LOD_SHIFT |
> > > -	      (mt->logical_width0 - 1) << BRW_SURFACE_WIDTH_SHIFT |
> > > -	      (mt->logical_height0 - 1) << BRW_SURFACE_HEIGHT_SHIFT);
> > > +   surf[2] = SET_FIELD(mip_count, BRW_SURFACE_LOD) |
> > > +             SET_FIELD(mt->logical_width0 - 1, BRW_SURFACE_WIDTH) |
> > > +             SET_FIELD(mt->logical_height0 - 1, BRW_SURFACE_HEIGHT);
> > >  
> > > -   surf[3] = (brw_get_surface_tiling_bits(mt->tiling) |
> > > -	      (mt->logical_depth0 - 1) << BRW_SURFACE_DEPTH_SHIFT |
> > > -	      (mt->pitch - 1) << BRW_SURFACE_PITCH_SHIFT);
> > > +   surf[3] = brw_get_surface_tiling_bits(mt->tiling) |
> > > +	     SET_FIELD(mt->logical_depth0 - 1, BRW_SURFACE_DEPTH) |
> > > +	     SET_FIELD(mt->pitch - 1, BRW_SURFACE_PITCH);
> > >  
> > > -   surf[4] = (brw_get_surface_num_multisamples(mt->num_samples) |
> > > -              SET_FIELD(tObj->BaseLevel - mt->first_level, BRW_SURFACE_MIN_LOD));
> > > +   surf[4] = brw_get_surface_num_multisamples(mt->num_samples) |
> > > +             SET_FIELD(min_lod, BRW_SURFACE_MIN_LOD);
> > 
> > This is not equivalent...Min Lod used to be:
> > 
> >    tObj->BaseLevel - mt->first_level
> > 
> > and now it is:
> > 
> >    tObj->MinLevel + tObj->BaseLevel - mt->first_level
> > 
> > I would really appreciate it if you could make this a separate patch
> > from the refactoring, for easier bisectability.  (First add tObj->MinLevel
> > to the Gen4-6 code, then do this refactor.)
> > 
> > It seems like a fine change, but is certainly worth noting in the commit
> > message.  Perhaps this is what fixed some tests?
> 
> Mark helped me to bisect that with Jenkins, it wasn't this patch. It was
> 
> i965: Pass slice details as parameters for surface setup
> 
> 
> Anyway, as the series grew I started forgetting things I meant to say or
> fix. This was something I meant to address, my apologies for you to need to
> figure it out manually. I actually thought adding assert and comment here:
> 
>    /* Mininum level setting is only used for ARB_texture_view which isn't
>     * enabled before gen7.
>     */
>    assert(ctx->Extensions.ARB_texture_view || tObj->MinLevel == 0);
> 
> How would you feel about that?

Ahh, right, that is only used for texture view.  So they are equivalent.

Cool.  Seems like a reasonable assertion to add.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150429/277ea0cb/attachment.sig>