[Intel-gfx] [PATCH v2] drm/i915: don't whitelist oacontrol in cmd parser
Daniel Vetter
daniel at ffwll.ch
Tue Nov 22 15:36:49 UTC 2016
On Tue, Nov 22, 2016 at 02:09:08PM +0000, Robert Bragg wrote:
> On Tue, Nov 22, 2016 at 1:34 PM, Daniel Vetter <daniel at ffwll.ch> wrote:
>
> > On Tue, Nov 08, 2016 at 12:51:48PM +0000, Robert Bragg wrote:
> > > This v2 patch bumps the command parser version so it can be referenced in
> > > corresponding i-g-t gem_exec_parse changes.
> > >
> > > --- >8 ---
> >
> > Scissors cut everything below, not everything above, hence next time
> > around pls switch around your comment and the commit message, as-is not
> > much left ;-)
> >
>
> Hmm, they cut away what's above and keep what's below in my experience -
> what command are you seeing the opposite with?
>
> I just double checked this with git am --scissors
Plain git am works the other way round, that's why I never noticed there's
a special option. TIL ;-)
-Daniel
>
> - Robert
>
>
>
> >
> > Fixed up while applying.
> > -Daniel
> >
> > >
> > > Being able to program OACONTROL from a non-privileged batch buffer is
> > > not sufficient to be able to configure the OA unit. This was originally
> > > allowed to help enable Mesa to expose OA counters via the
> > > INTEL_performance_query extension, but the current implementation based
> > > on programming OACONTROL via a batch buffer isn't able to report useable
> > > data without a more complete OA unit configuration. Mesa handles the
> > > possibility that writes to OACONTROL may not be allowed and so only
> > > advertises the extension after explicitly testing that a write to
> > > OACONTROL succeeds. Based on this; removing OACONTROL from the whitelist
> > > should be ok for userspace.
> > >
> > > Removing this simplifies adding a new kernel api for configuring the OA
> > > unit without needing to consider the possibility that userspace might
> > > trample on OACONTROL state which we'd like to start managing within
> > > the kernel instead. In particular running any Mesa based GL application
> > > currently results in clearing OACONTROL when initializing which would
> > > disable the capturing of metrics.
> > >
> > > v2:
> > > This bumps the command parser version from 8 to 9, as the change is
> > > visible to userspace.
> > >
> > > Signed-off-by: Robert Bragg <robert at sixbynine.org>
> > > Reviewed-by: Matthew Auld <matthew.auld at intel.com>
> > > Reviewed-by: Sourab Gupta <sourab.gupta at intel.com>
> > > ---
> > > drivers/gpu/drm/i915/i915_cmd_parser.c | 42
> > ++++------------------------------
> > > 1 file changed, 5 insertions(+), 37 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c
> > b/drivers/gpu/drm/i915/i915_cmd_parser.c
> > > index c9d2ecd..f5762cd 100644
> > > --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> > > +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> > > @@ -450,7 +450,6 @@ static const struct drm_i915_reg_descriptor
> > gen7_render_regs[] = {
> > > REG64(PS_INVOCATION_COUNT),
> > > REG64(PS_DEPTH_COUNT),
> > > REG64_IDX(RING_TIMESTAMP, RENDER_RING_BASE),
> > > - REG32(GEN7_OACONTROL), /* Only allowed for LRI and SRM. See below.
> > */
> > > REG64(MI_PREDICATE_SRC0),
> > > REG64(MI_PREDICATE_SRC1),
> > > REG32(GEN7_3DPRIM_END_OFFSET),
> > > @@ -1060,8 +1059,7 @@ bool intel_engine_needs_cmd_parser(struct
> > intel_engine_cs *engine)
> > > static bool check_cmd(const struct intel_engine_cs *engine,
> > > const struct drm_i915_cmd_descriptor *desc,
> > > const u32 *cmd, u32 length,
> > > - const bool is_master,
> > > - bool *oacontrol_set)
> > > + const bool is_master)
> > > {
> > > if (desc->flags & CMD_DESC_SKIP)
> > > return true;
> > > @@ -1099,31 +1097,6 @@ static bool check_cmd(const struct
> > intel_engine_cs *engine,
> > > }
> > >
> > > /*
> > > - * 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 == i915_mmio_reg_offset(GEN7_OACONTROL))
> > {
> > > - if (desc->cmd.value ==
> > MI_LOAD_REGISTER_MEM) {
> > > - DRM_DEBUG_DRIVER("CMD: Rejected
> > LRM to OACONTROL\n");
> > > - return false;
> > > - }
> > > -
> > > - if (desc->cmd.value ==
> > MI_LOAD_REGISTER_REG) {
> > > - DRM_DEBUG_DRIVER("CMD: Rejected
> > LRR to OACONTROL\n");
> > > - return false;
> > > - }
> > > -
> > > - if (desc->cmd.value ==
> > MI_LOAD_REGISTER_IMM(1))
> > > - *oacontrol_set = (cmd[offset + 1]
> > != 0);
> > > - }
> > > -
> > > - /*
> > > * Check the value written to the register against
> > the
> > > * allowed mask/value pair given in the whitelist
> > entry.
> > > */
> > > @@ -1214,7 +1187,6 @@ int intel_engine_cmd_parser(struct intel_engine_cs
> > *engine,
> > > u32 *cmd, *batch_end;
> > > struct drm_i915_cmd_descriptor default_desc = noop_desc;
> > > const struct drm_i915_cmd_descriptor *desc = &default_desc;
> > > - bool oacontrol_set = false; /* OACONTROL tracking. See check_cmd()
> > */
> > > bool needs_clflush_after = false;
> > > int ret = 0;
> > >
> > > @@ -1270,8 +1242,7 @@ int intel_engine_cmd_parser(struct intel_engine_cs
> > *engine,
> > > break;
> > > }
> > >
> > > - if (!check_cmd(engine, desc, cmd, length, is_master,
> > > - &oacontrol_set)) {
> > > + if (!check_cmd(engine, desc, cmd, length, is_master)) {
> > > ret = -EACCES;
> > > break;
> > > }
> > > @@ -1279,11 +1250,6 @@ int intel_engine_cmd_parser(struct
> > intel_engine_cs *engine,
> > > 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;
> > > @@ -1336,6 +1302,8 @@ int i915_cmd_parser_get_version(struct
> > drm_i915_private *dev_priv)
> > > * 8. Don't report cmd_check() failures as EINVAL errors to
> > userspace;
> > > * rely on the HW to NOOP disallowed commands as it would
> > without
> > > * the parser enabled.
> > > + * 9. Don't whitelist or handle oacontrol specially, as ownership
> > > + * for oacontrol state is moving to i915-perf.
> > > */
> > > - return 8;
> > > + return 9;
> > > }
> > > --
> > > 2.10.1
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> >
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the Intel-gfx
mailing list