On Thu, Jul 16, 2015 at 03:06:48PM -0700, Matt Turner wrote:
> On Tue, Jul 14, 2015 at 9:56 AM, Ben Widawsky
> <benjamin.widaw...@intel.com> wrote:
> > This patch rewrites the logic for determining which method we using for
> > mapping
> > a miptree. It is my intention that that this patch, the required patches
> > before
> > this do not change functionality, or if they do, it's in very obscure an
> > unobservable cases.
> >
> > I have two reasons why I decided to write this patch. The existing logic
> > was way
> > too tricky. In particular, the way in which it evaluated which operation to
> > use
> > was out of order - specifically when it checked to use the blitter in
> > use_intel_mipree_map_blit(), part of the check is to determine if it will
> > later
> > be unable to use the GTT. The other reason is to make playing with the
> > various
> > operations much easier. For example, there are some theories being thrown
> > around
> > that we might actually want to use the blitter where we use the GTT today,
> > and
> > vice versa. After this patch, benchmarking those changes is much more
> > straightforward.
> >
> > It's pretty difficult for me to prove there is no real change going on. I
> > ran a
> > subset of my benchmarks on this though. The following benchmarks show no
> > perf
> > difference on BDW with ministat with n=5 and CI=.95:
> > OglBatch7
> > OglDeferred
> > OglFillPixel
> > OglGeomPoint
> > OglGeomTriList
> > OglHdrBloom
> > OglPSBump2
> > OglPSPhong
> > OglPSPom
> > OglShMapPcf
> > OglTerrainFlyInst
> > OglTexMem512
> > OglVSDiffuse8
> > OglVSInstancing
> > OglZBuffer
> > plot3d
> > trex
> >
> > It's important to point out that much of the changes effect non-LLC
> > platform,
>
> s/effect/affect/
>
> > and I do not yet have data for that. I'll be collecting it over the next few
> > days, but I figure this patch can get some comments meanwhile.
> >
> > Signed-off-by: Ben Widawsky <b...@bwidawsk.net>
> > ---
> > src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 76
> > +++++++++++++--------------
> > 1 file changed, 37 insertions(+), 39 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > index 2788270..545fbf3 100644
> > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > @@ -2283,6 +2283,8 @@ intel_miptree_unmap_movntdqa(struct brw_context *brw,
> > map->buffer = NULL;
> > map->ptr = NULL;
> > }
> > +#else
> > +#define intel_miptree_map_movntdqa(x,y,z,w,a) abort()
>
> Yuck.
>
> > #endif
> >
> > static void
> > @@ -2621,36 +2623,6 @@ can_blit_slice(struct brw_context *brw,
> > return true;
> > }
> >
> > -static bool
> > -use_intel_mipree_map_blit(struct brw_context *brw,
> > - struct intel_mipmap_tree *mt,
> > - GLbitfield mode,
> > - unsigned int level,
> > - unsigned int slice)
> > -{
> > - if (brw->has_llc &&
> > - !(mode & GL_MAP_WRITE_BIT) &&
> > - can_blit_slice(brw, mt, level, slice))
> > - return true;
> > -
> > - if (mt->tiling != I915_TILING_NONE &&
> > - mt->bo->size >= brw->max_gtt_map_object_size) {
> > - /* XXX: This assertion is actually the final condition for platforms
> > - * without SSE4.1. Returning false is not the right thing to do with
> > - * the current code. On those platforms, the goal of this function
> > is to give
> > - * preference to the GTT, and at this point we've determined we
> > cannot use
> > - * the GTT, and we cannot blit, so we are out of options.
> > - *
> > - * NOTE: It should be possible to actually handle the case, but
> > AFAIK, we
> > - * never get this assertion.
> > - */
> > - assert(can_blit_slice(brw, mt, level, slice));
> > - return true;
> > - }
> > -
> > - return false;
> > -}
> > -
> > /**
> > * Parameter \a out_stride has type ptrdiff_t not because the buffer
> > stride may
> > * exceed 32 bits but to diminish the likelihood subtle bugs in pointer
> > @@ -2706,18 +2678,44 @@ intel_miptree_map(struct brw_context *brw,
> > goto done;
> > }
> >
> > - if (use_intel_mipree_map_blit(brw, mt, mode, level, slice)) {
> > - intel_miptree_map_blit(brw, mt, map, level, slice);
> > + /* First determine what the available option are, then pick from the
> > best
> > + * option based on the platform.
> > + */
> > + bool can_hw_blit = can_blit_slice(brw, mt, level, slice);
> > + bool can_use_gtt = mt->bo->size < brw->max_gtt_map_object_size;
> > #if defined(USE_SSE41)
> > - } else if (!(mode & GL_MAP_WRITE_BIT) &&
> > - !mt->compressed && cpu_has_sse4_1 &&
> > - (mt->pitch % 16 == 0)) {
> > - intel_miptree_map_movntdqa(brw, mt, map, level, slice);
> > + bool can_stream_map = cpu_has_sse4_1 && mt->pitch % 16 == 0;
> > +#else
> > + bool can_stream_map = false;
> > #endif
> > - } else {
> > - assert(mode & GL_MAP_WRITE_BIT == 0);
> > - assert(!mt->compressed);
> > +
> > + if (can_stream_map) {
> > + /* BENCHMARK_ME: GTT maps for non-llc */
> > + intel_miptree_map_movntdqa(brw, mt, map, level, slice);
> > + goto done;
> > + }
>
> Just put this block inside the #if defined(USE_SSE41) where
> can_stream_map is set and remove the abort(). I don't see any
> advantage of separating them.
Yes, I agree with you. However, based on Chad's feedback, I don't plan to go
ahead with this patch unless someone speaks in favor of it.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
