[Mesa-dev] [PATCH] i965: Only enable ARB_query_buffer_object for newer kernels on Haswell.

[Kenneth Graunke]

On Thursday, May 5, 2016 8:17:03 AM PDT Jordan Justen wrote:
> On 2016-05-05 03:15:51, Kenneth Graunke wrote:
> > On Haswell, we need version 6 of the kernel command parser in order to
> > write the math registers.  Our implementation of ARB_query_buffer_object
> > heavily relies on MI_MATH, so we should only advertise it when MI_MATH
> > is available.
> > 
> 
> Doh! Command Parser!! :(
> 
> > We're going to want to use MI_MATH in more places in the future.
> > To make checking for it easier, introduce a screen->has_mi_math flag.
> > 
> > Cc: Jordan Justen <jljusten at gmail.com>
> > Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> > ---
> >  src/mesa/drivers/dri/i965/intel_extensions.c | 2 +-
> >  src/mesa/drivers/dri/i965/intel_screen.c     | 8 ++++++++
> >  src/mesa/drivers/dri/i965/intel_screen.h     | 5 +++++
> >  3 files changed, 14 insertions(+), 1 deletion(-)
> > 
> > Note: kernel command parser version 7 isn't actually a real thing yet.
> > You need the patch I just sent to intel-gfx:
> > [PATCH] drm/i915: Allow MI_LOAD_REGISTER_REG between whitelisted 
registers.
> > 
> > Also, Piglit's qbo test's
> > query-GL_TRANSFORM_FEEDBACK_PRIMITIVES_WRITTEN-ASYNC subtest fails for me
> > on Haswell.  I haven't looked into why yet.
> > 
> > diff --git a/src/mesa/drivers/dri/i965/intel_extensions.c b/src/mesa/
drivers/dri/i965/intel_extensions.c
> > index 588df1a..7794c2f 100644
> > --- a/src/mesa/drivers/dri/i965/intel_extensions.c
> > +++ b/src/mesa/drivers/dri/i965/intel_extensions.c
> > @@ -366,7 +366,7 @@ intelInitExtensions(struct gl_context *ctx)
> >        }
> >     }
> >  
> > -   if (brw->gen >= 8 || brw->is_haswell) {
> > +   if (brw->intelScreen->has_mi_math) {
> 
> Should we bump the check to gen >= 8 for now?
> 
> With just changing the check to not include hsw:
> 
> Reviewed-by: Jordan Justen <jordan.l.justen at intel.com>
> 
> >        ctx->Extensions.ARB_query_buffer_object = true;
> >     }
> >  
> > diff --git a/src/mesa/drivers/dri/i965/intel_screen.c b/src/mesa/drivers/
dri/i965/intel_screen.c
> > index 878901a..24fe523 100644
> > --- a/src/mesa/drivers/dri/i965/intel_screen.c
> > +++ b/src/mesa/drivers/dri/i965/intel_screen.c
> > @@ -1541,6 +1541,14 @@ __DRIconfig **intelInitScreen2(__DRIscreen *psp)
> >     if (ret == -1)
> >        intelScreen->cmd_parser_version = 0;
> >  
> > +   /* Haswell requires command parser version 6 in order to write to the
> > +    * MI_MATH GPR registers, and version 7 in order to use
> > +    * MI_LOAD_REGISTER_REG (which all users of MI_MATH use).
> > +    */
> > +   intelScreen->has_mi_math = intelScreen->devinfo->gen >= 8 ||
> > +                              (intelScreen->devinfo->is_haswell &&
> > +                               intelScreen->cmd_parser_version >= 7);
> 
> Do you think has_mi_math should imply LRR? Command parser version 6
> should allow MI_MATH to be used. I guess the Vulkan driver uses
> MI_MATH without LRR.
> 
> -Jordan

I'll admit, it's a little weird...originally this just checked for
version 6.  But then I discovered that we needed MI_LOAD_REGISTER_REG
too...both your query buffer code and my new ARB_transform_feedback2
implementation use both commands.

I suppose we could rename it to has_mi_math_and_load_reg_reg?
-------------- 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: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160505/5a233e3d/attachment.sig>