[Mesa-dev] [PATCH 02/15] i965: Add helper for converting isl tiling to bufmgr tiling

[Nanley Chery]

On Thu, Jun 15, 2017 at 06:23:24PM -0700, Jason Ekstrand wrote:
> On Thu, Jun 15, 2017 at 5:18 PM, Nanley Chery <nanleychery at gmail.com> wrote:
> 
> > On Tue, Jun 13, 2017 at 05:50:00PM +0300, Topi Pohjolainen wrote:
> > > Signed-off-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
> > > ---
> > >  src/mesa/drivers/dri/i965/intel_blit.h | 13 +++++++++++++
> > >  1 file changed, 13 insertions(+)
> > >
> > > diff --git a/src/mesa/drivers/dri/i965/intel_blit.h
> > b/src/mesa/drivers/dri/i965/intel_blit.h
> > > index 2604417e2d..5e4d1f5eb4 100644
> > > --- a/src/mesa/drivers/dri/i965/intel_blit.h
> > > +++ b/src/mesa/drivers/dri/i965/intel_blit.h
> > > @@ -28,6 +28,19 @@
> > >
> > >  #include "brw_context.h"
> > >
> > > +static inline unsigned
> > > +isl_tiling_to_bufmgr_tiling(enum isl_tiling tiling)
> > > +{
> > > +   if (tiling == ISL_TILING_X)
> > > +      return I915_TILING_X;
> > > +
> > > +   if (tiling == ISL_TILING_Y0)
> >
> > I actually just read the patch where this function is used. Don't we
> > also need to return I915_TILING_Y for ISL_TILING_W? Maybe Jason can
> > confirm.
> >
> 
> I'd rather the function not lie... Also, most places in the driver today
> that do W-tiling use I915_TILING_NONE.  Yes, I915_TILING_NONE is sort of
> LINEAR but you could also interpret it as LINEAR || UNKNOWN.
> 
> 
> > > +      return I915_TILING_Y;
> > > +
> > > +   /* All other are unknown to buffer allocator. */
> >
> > It would seem that we'd like to assert for unexpected values instead of
> > failing silently.
> >
> 
> I just pulled up my version:
> 
> https://cgit.freedesktop.org/~jekstrand/mesa/commit/?h=wip/i965-ccs-mod-v3&id=6133059904aeaa7bd6d4ee364014f491f59083e2
> 
> and I went with I915_TILING_NONE there.  The reason I did so was so that
> you could call
> 
> brw_bo_alloc_tiled(brw, name, surf.size,
> isl_tiling_to_i915_tiling(surf.tiling), surf.row_pitch, 0);
> 
> without having to worry about isl_tiling_to_i915_tiling asserting on you.
> 
> The only two things that the tiling is used for are scanout and fenced
> maps.  Even when we enable things like Yf scan-out, we'll probably do it
> through modifiers and not set_tiling.  At that point, the only thing that
> the I915_TILING parameters are actually needed for is doing fenced GTT maps
> (which detile on the fly).  Since you can't do a fenced map of anything
> other than X and Y-tiled,  I don't think we'll ever need to add I915_TILING
> parameters for the others because you can't do a fenced (GTT) map of a Yf,
> Ys, or W-tiled buffer anyway.
> 
> On the other hand, when used in something such as the blit code, asserting
> is probably the right thing to do.  One option would be to add a little
> alloc_bo_for_isl_surf helper which knows to map everything other than X and
> Y to LINEAR and make the general function assert.
> 
> --Jason
> 
> 

Thanks for the feedback!

> > > +   return I915_TILING_NONE;
> > > +}
> > > +
> > >  bool
> > >  intelEmitCopyBlit(struct brw_context *brw,
> > >                    GLuint cpp,
> > > --
> > > 2.11.0
> > >
> > > _______________________________________________
> > > mesa-dev mailing list
> > > mesa-dev at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >