[Mesa-dev] [PATCH] i965: Skip register write checks if cmd_parser_version >= 2.

[Kenneth Graunke]

On Wednesday, October 26, 2016 10:42:32 AM PDT Francisco Jerez wrote:
> Daniel Vetter <daniel at ffwll.ch> writes:
> 
> > On Tue, Oct 25, 2016 at 11:16:56AM -0700, Francisco Jerez wrote:
> >> Kenneth Graunke <kenneth at whitecape.org> writes:
> >> 
> >> > If the kernel advertises a new enough command parser version, then we
> >> > can just assume that register writes will work and not bother executing
> >> > commands on the GPU to test it.
> >> >
> >> > This should speed up context creation.
> >> >
> >> > From the command parser version history (i915_cmd_parser.c):
> >> >
> >> >   * 1. Initial version. Checks batches and reports violations, but leaves
> >> >   *    hardware parsing enabled (so does not allow new use cases).
> >> >   * 2. Allow access to the MI_PREDICATE_SRC0 and
> >> >   *    MI_PREDICATE_SRC1 registers.
> >> >
> >> > Both of the things we check for were added before version 2, but
> >> > 1 doesn't sound sufficient.  The point is to skip work on modern
> >> > kernels, so requiring version 2 will work.
> >> >
> >> > Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> >> > ---
> >> >  src/mesa/drivers/dri/i965/intel_extensions.c | 8 ++++++++
> >> >  1 file changed, 8 insertions(+)
> >> >
> >> > diff --git a/src/mesa/drivers/dri/i965/intel_extensions.c b/src/mesa/drivers/dri/i965/intel_extensions.c
> >> > index 66079b5..2a5a3b7 100644
> >> > --- a/src/mesa/drivers/dri/i965/intel_extensions.c
> >> > +++ b/src/mesa/drivers/dri/i965/intel_extensions.c
> >> > @@ -46,6 +46,10 @@ can_do_pipelined_register_writes(struct brw_context *brw)
> >> >     if (brw->gen != 7)
> >> >        return true;
> >> >  
> >> > +   /* No need to execute commands to check if the kernel advertises it. */
> >> > +   if (brw->screen->cmd_parser_version >= 2)
> >> > +      return true;
> >> > +
> >> >     static int result = -1;
> >> >     if (result != -1)
> >> >        return result;
> >> > @@ -106,6 +110,10 @@ can_write_oacontrol(struct brw_context *brw)
> >> >     if (brw->gen < 6 || brw->gen >= 8)
> >> >        return false;
> >> >  
> >> > +   /* No need to execute commands to check if the kernel advertises it. */
> >> > +   if (brw->screen->cmd_parser_version >= 2)
> >> > +      return true;
> >> > +
> >> 
> >> I'm not sure this gives you any guarantees that the command parser is
> >> going to be active?  Has the command parser been enabled by default on
> >> all kernel versions exposing cmd_parser_version >= 2?  Wouldn't this
> >> break things if the command parser is disabled in the kernel command
> >> line?  [Currently it would just cause some extensions to be disabled]
> >
> > We have the stance that touching any module option is an unsupported
> > configuration, don't do that (and we'll ingore bug reports). Either way,
> > the kernel reports 0 when the cmd parser is disabled.
> 
> I don't think it reports 0 in that case -- At least it didn't last time
> I checked.  And ISTR that the command parser wasn't actually in control
> on at least some platforms until PPGTT was enabled, so I doubt that
> 'cmd_parser_version >= 2' gives you any guarantees that the whitelisted
> commands are actually going to get through the hardware command checker.

It does, as of:

commit 1ca3712ca3429a617ed6c5f87718e4f6fe4ae0c6
Author: Chris Wilson <chris at chris-wilson.co.uk>
Date:   Wed May 4 14:25:36 2016 +0100

    drm/i915: Report command parser version 0 if disabled

Previously cmd_parser_version would be reported as 6.  So I think
checking for anything less than 7 doesn't guarantee whether it's
enabled or not. :(

So, we either have to check for cmd_parser_version >= 7, which would
bump our kernel requirement to 4.8 (not going to happen), or we have
to execute a batch and try to write something (make sure writes can
happen at all)...but can use version checks to look for registers
so we don't have to try every single one of them.

This is particularly awful because in order to advertise OpenGL 4.0,
we need to know whether writes work at screen creation time, when we
don't even have an OpenGL context at all, which means we can't use any
of our normal command submission mechanism.  We don't even have a
hw_ctx to run with - though we have the implicit one created by opening
the file descriptor.  I suppose we just need to bite the bullet and
create a new brw_check_register_writes.c file that directly creates a
batch, calls execbuf(), and checks the results.

What a disaster.  Consider this patch NAK'd...

--Ken
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20161026/b76f10bc/attachment.sig>