[Mesa-dev] [PATCH resend 2/7] i965: Pass resource streamer enable flags on batchbuffer start

[Kenneth Graunke]

On Monday, June 01, 2015 03:14:25 PM Abdiel Janulgue wrote:
> This is passed on the kernel to enable the resource streamer enable bit
> on MI_BATCHBUFFER_START
> 
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue at linux.intel.com>
> ---
>  src/mesa/drivers/dri/i965/brw_context.h       | 1 +
>  src/mesa/drivers/dri/i965/intel_batchbuffer.c | 8 +++++++-
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_context.h b/src/mesa/drivers/dri/i965/brw_context.h
> index abc11f6..3f8e59d 100644
> --- a/src/mesa/drivers/dri/i965/brw_context.h
> +++ b/src/mesa/drivers/dri/i965/brw_context.h
> @@ -1140,6 +1140,7 @@ struct brw_context
>     bool no_simd8;
>     bool use_rep_send;
>     bool scalar_vs;
> +   bool has_resource_streamer;

So...you've introduced a new has_resource_streamer boolean.  I wanted to
review the conditions where you set it to true...but unless I've missed
something, no patch in this series actually ever sets it to true.  It'll
always be false by virtue of us zero-allocating brw_context.

This means that your new code is entirely dead, and untestable, which
is obviously not good.

What about adding an INTEL_USE_HW_BT environment variable, and setting
it to env_var_as_boolean("INTEL_USE_HW_BT", false)?  You could add a
second environment variable for gather constants, and do:

   brw->use_resource_streamer = devinfo->has_resource_streamer &&
      (env_var_as_boolean("INTEL_USE_HW_BT", false) ||
       env_var_as_boolean("INTEL_USE_GATHER", false));

That way, once your patches land, we can easily do Piglit runs with and
without hardware binding tables and gather constants.  Easy to try out.

We'd also need to check if the user's kernel is new enough to recognize
the execbuf flag - we still need to work on older kernels.

Also, could we call this "use_resource_streamer"?  Haswell always *has*
resource streamer hardware, even if we're not using it.

I'd suggest adding a brw_device_info::has_resource_streamer bit as well,
and setting it to true on Haswell, Broadwell, Cherryview, Skylake, and
Broxton.

>     /**
>      * Some versions of Gen hardware don't do centroid interpolation correctly
> diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> index ed659ed..a2a3a95 100644
> --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> @@ -267,6 +267,11 @@ throttle(struct brw_context *brw)
>     }
>  }
>  
> +/* Drop when RS headers get pulled to libdrm */
> +#ifndef I915_EXEC_RESOURCE_STREAMER
> +#define I915_EXEC_RESOURCE_STREAMER (1<<16)
> +#endif
> +

This is fine for an RFC, but when actually pushing the code, we need to:

1. Push the kernel patches.
2. Push the libdrm patch and do a libdrm release.
3. Create a configure.ac patch that makes Mesa depend on the new libdrm.
4. Remove this code from the Mesa patch.

(You probably knew that already, but I figured I'd reiterate it just to
be safe - we don't do this too often...)

>  /* TODO: Push this whole function into bufmgr.
>   */
>  static int
> @@ -293,7 +298,8 @@ do_flush_locked(struct brw_context *brw)
>        if (brw->gen >= 6 && batch->ring == BLT_RING) {
>           flags = I915_EXEC_BLT;
>        } else {
> -         flags = I915_EXEC_RENDER;
> +         flags = I915_EXEC_RENDER |
> +            (brw->has_resource_streamer ? I915_EXEC_RESOURCE_STREAMER : 0);
>        }
>        if (batch->needs_sol_reset)
>  	 flags |= I915_EXEC_GEN7_SOL_RESET;
> 
-------------- 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/20150601/43de77e9/attachment.sig>