Re: [PATCH] drm/i915: Add OACONTROL to the command parser register whitelist.

["Volkin, Bradley D" <bradley.d.volkin@intel.com>]

On Tue, Mar 25, 2014 at 11:21:23PM -0700, Daniel Vetter wrote:
> On Tue, Mar 25, 2014 at 10:52:03PM -0700, Kenneth Graunke wrote:
> > Mesa needs to be able to write OACONTROL in order to expose the
> > Observability Architecture's performance counters via OpenGL.
> > 
> > Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>
> 
> Thanks a lot for quickly tracking this down. Now when we've talked about
> OA a little while ago we concluded that mesa should clear OACONTROL again
> before the batch ends to make sure that userspace can't unduly observe
> other processes. So I think it'd be worth to keep track of this with a
> flag (set when OACONTROL is != 0 and reset when the batch loads 0). Also
> we need to make sure that userspace sets the right OACONTROL modes (not
> the one which streams into a global gtt buffer essentially). So some
> additional work required.

Ok, I'll look into this. And apologies for not catching it myself.

If we have to do additional checks on fields within the registers then I
suppose we'll need to limit those registers to MI_LOAD_REGISTER_IMM. That
might require separate whitelists for MI_LOAD_REGISTER_IMM/MEM. Not the end
of the world, but certainly some additional complexity.

For the resetting check, are there other registers in the current list that
should have this tracking? If so, is 0 the reset value in all cases?

Let me know if there is anything in the works that would require additional
registers or different uses of any registers.

Thanks,
Brad

> 
> I've added a FIXME comment to the code.
> 
> Brad, can you please look into this, including the corresponding i-g-t
> which tries to enable OA but doesn't disable it and one which tries to
> enable it in the continuous mode (which writes to gtt)? Ken's previous
> mails has the cmds mesa emits.
> 
> Thanks, Daniel
> > ---
> >  drivers/gpu/drm/i915/i915_cmd_parser.c | 1 +
> >  drivers/gpu/drm/i915/i915_reg.h        | 2 ++
> >  2 files changed, 3 insertions(+)
> > 
> > This patch needs to go before
> > 
> >    commit 6d42f94084b8c69813d7ecd0466c33fe561bf127
> >    Author: Brad Volkin <bradley.d.volkin@intel.com>
> >    Date:   Tue Feb 18 10:15:57 2014 -0800
> > 
> >        drm/i915: Enable command parsing by default
> > 
> > in whatever branch gets submitted to Dave Airlie.  Or, that commit needs
> > to be reverted.  Otherwise, every OpenGL program will abort.  Examples
> > of programs that abort include GNOME, KDE, Firefox, and glxgears.
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
> > index bae7c2f..d4a50b9 100644
> > --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> > +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> > @@ -415,6 +415,7 @@ static const u32 gen7_render_regs[] = {
> >  	GEN7_SO_WRITE_OFFSET(1),
> >  	GEN7_SO_WRITE_OFFSET(2),
> >  	GEN7_SO_WRITE_OFFSET(3),
> > +	OACONTROL,
> >  };
> >  
> >  static const u32 gen7_blt_regs[] = {
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 9f9e2b7..0ebc20d 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -427,6 +427,8 @@
> >  /* There are the 4 64-bit counter registers, one for each stream output */
> >  #define GEN7_SO_NUM_PRIMS_WRITTEN(n) (0x5200 + (n) * 8)
> >  
> > +#define OACONTROL 0x2360
> > +
> >  #define _GEN7_PIPEA_DE_LOAD_SL	0x70068
> >  #define _GEN7_PIPEB_DE_LOAD_SL	0x71068
> >  #define GEN7_PIPE_DE_LOAD_SL(pipe) _PIPE(pipe, \
> > -- 
> > 1.9.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch