From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Volkin, Bradley D" Subject: Re: [PATCH 3/3] drm/i915: Track OACONTROL register enable/disable during parsing Date: Thu, 27 Mar 2014 17:06:33 -0700 Message-ID: <20140328000633.GC21132@bdvolkin-ubuntu-desktop> References: <1395945820-20376-1-git-send-email-bradley.d.volkin@intel.com> <1395945820-20376-4-git-send-email-bradley.d.volkin@intel.com> <53349EE9.6090509@whitecape.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTP id A65176EB59 for ; Thu, 27 Mar 2014 17:06:46 -0700 (PDT) Content-Disposition: inline In-Reply-To: <53349EE9.6090509@whitecape.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Kenneth Graunke Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Thu, Mar 27, 2014 at 02:58:01PM -0700, Kenneth Graunke wrote: > On 03/27/2014 11:43 AM, bradley.d.volkin@intel.com wrote: > > From: Brad Volkin > > > > 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 > > Signed-off-by: Brad Volkin > > --- > > 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. > > > + > > + if (desc->cmd.value == MI_LOAD_REGISTER_IMM(1)) > > + *oacontrol_set = (cmd[2] != 0) ? true : false; > > You shouldn't need to do both != 0 and ? true : false... > > *oacontrol_set = cmd[2] != 0; Will fix. > > Thanks for implementing this! That was surprisingly less code than I > thought. This is the "you told me there's only one register that needs this" implementation :) A more general solution would probably be uglier. Thanks, Brad > > > + } > > + > > if (!valid_reg(ring->reg_table, > > ring->reg_count, reg_addr)) { > > if (!is_master || > > @@ -851,6 +864,7 @@ int i915_parse_cmds(struct intel_ring_buffer *ring, > > u32 *cmd, *batch_base, *batch_end; > > struct drm_i915_cmd_descriptor default_desc = { 0 }; > > int needs_clflush = 0; > > + bool oacontrol_set = false; /* OACONTROL tracking. See check_cmd() */ > > > > ret = i915_gem_obj_prepare_shmem_read(batch_obj, &needs_clflush); > > if (ret) { > > @@ -900,7 +914,7 @@ int i915_parse_cmds(struct intel_ring_buffer *ring, > > break; > > } > > > > - if (!check_cmd(ring, desc, cmd, is_master)) { > > + if (!check_cmd(ring, desc, cmd, is_master, &oacontrol_set)) { > > ret = -EINVAL; > > break; > > } > > @@ -908,6 +922,11 @@ int i915_parse_cmds(struct intel_ring_buffer *ring, > > cmd += length; > > } > > > > + if (oacontrol_set) { > > + DRM_DEBUG_DRIVER("CMD: batch set OACONTROL but did not clear it\n"); > > + ret = -EINVAL; > > + } > > + > > if (cmd >= batch_end) { > > DRM_DEBUG_DRIVER("CMD: Got to the end of the buffer w/o a BBE cmd!\n"); > > ret = -EINVAL; > > > >