[Intel-gfx] [PATCH 3/3] drm/i915: Track OACONTROL register enable/disable during parsing

[Daniel Vetter]

On Thu, Mar 27, 2014 at 05:06:33PM -0700, Volkin, Bradley D wrote:
> On Thu, Mar 27, 2014 at 02:58:01PM -0700, Kenneth Graunke wrote:
> > On 03/27/2014 11:43 AM, bradley.d.volkin at intel.com wrote:
> > > From: Brad Volkin <bradley.d.volkin at intel.com>
> > > 
> > > There is some thought that the data from the performance counters enabled
> > > via OACONTROL should only be available to the process that enabled counting.
> > > To limit snooping, require that any batch buffer which sets OACONTROL to a
> > > non-zero value also sets it back to 0 before the end of the batch.
> > > 
> > > This requires limiting OACONTROL writes to happen via MI_LOAD_REGISTER_IMM
> > > so that we can access the value being written. This should be in line with
> > > the expected use case for writing OACONTROL.
> > > 
> > > Cc: Kenneth Graunke <kenneth at whitecape.org>
> > > Signed-off-by: Brad Volkin <bradley.d.volkin at intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_cmd_parser.c | 35 ++++++++++++++++++++++++++--------
> > >  1 file changed, 27 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
> > > index 2eb2aca..779e14c 100644
> > > --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> > > +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> > > @@ -407,12 +407,7 @@ static const u32 gen7_render_regs[] = {
> > >  	REG64(CL_PRIMITIVES_COUNT),
> > >  	REG64(PS_INVOCATION_COUNT),
> > >  	REG64(PS_DEPTH_COUNT),
> > > -	/*
> > > -	 * FIXME: This is just to keep mesa working for now, we need to check
> > > -	 * that mesa resets this again and that it doesn't use any of the
> > > -	 * special modes which write into the gtt.
> > > -	 */
> > > -	OACONTROL,
> > > +	OACONTROL, /* Only allowed for LRI and SRM. See below. */
> > >  	REG64(GEN7_SO_NUM_PRIMS_WRITTEN(0)),
> > >  	REG64(GEN7_SO_NUM_PRIMS_WRITTEN(1)),
> > >  	REG64(GEN7_SO_NUM_PRIMS_WRITTEN(2)),
> > > @@ -761,7 +756,8 @@ bool i915_needs_cmd_parser(struct intel_ring_buffer *ring)
> > >  static bool check_cmd(const struct intel_ring_buffer *ring,
> > >  		      const struct drm_i915_cmd_descriptor *desc,
> > >  		      const u32 *cmd,
> > > -		      const bool is_master)
> > > +		      const bool is_master,
> > > +		      bool *oacontrol_set)
> > >  {
> > >  	if (desc->flags & CMD_DESC_REJECT) {
> > >  		DRM_DEBUG_DRIVER("CMD: Rejected command: 0x%08X\n", *cmd);
> > > @@ -777,6 +773,23 @@ static bool check_cmd(const struct intel_ring_buffer *ring,
> > >  	if (desc->flags & CMD_DESC_REGISTER) {
> > >  		u32 reg_addr = cmd[desc->reg.offset] & desc->reg.mask;
> > >  
> > > +		/*
> > > +		 * OACONTROL requires some special handling for writes. We
> > > +		 * want to make sure that any batch which enables OA also
> > > +		 * disables it before the end of the batch. The goal is to
> > > +		 * prevent one process from snooping on the perf data from
> > > +		 * another process. To do that, we need to check the value
> > > +		 * that will be written to the register. Hence, limit
> > > +		 * OACONTROL writes to only MI_LOAD_REGISTER_IMM commands.
> > > +		 */
> > > +		if (reg_addr == OACONTROL) {
> > > +			if (desc->cmd.value == MI_LOAD_REGISTER_MEM)
> > > +				return false;
> > 
> > While I don't need the ability to use LRM on OACONTROL, I don't see any
> > specific reason to prohibit it, as long as there's a later LRI of 0.
> > 
> > I think you could just do:
> > 
> > if (desc->cmd.value == MI_LOAD_REGISTER_MEM)
> >    oacontrol_set = true;
> > 
> > which will later get cleared if it sees a LRI of 0, and if not, you
> > reject the batch.
> 
> Fair enough. Rejecting LRM was as much a carryover from thinking we needed to
> check bits within the value. I'll make this change if there are no objections.

Imo if there's no user there's no need for that.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch