Re: [PATCH v2] drm/i915: don't whitelist oacontrol in cmd parser

From: Robert Bragg <robert@sixbynine.org>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: David Airlie <airlied@linux.ie>,
	Daniel Vetter <daniel.vetter@intel.com>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	Sourab Gupta <sourab.gupta@intel.com>,
	ML dri-devel <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH v2] drm/i915: don't whitelist oacontrol in cmd parser
Date: Tue, 22 Nov 2016 14:09:08 +0000	[thread overview]
Message-ID: <CAMou1-3LJ_M8=OTYhsdhuTMQHG49=NsesAzTdxv4gccXeM+vVg@mail.gmail.com> (raw)
In-Reply-To: <20161122133426.tuoc25skxbx3246o@phenom.ffwll.local>

[-- Attachment #1.1: Type: text/plain, Size: 7333 bytes --]

On Tue, Nov 22, 2016 at 1:34 PM, Daniel Vetter <daniel@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

- 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@sixbynine.org>
> > Reviewed-by: Matthew Auld <matthew.auld@intel.com>
> > Reviewed-by: Sourab Gupta <sourab.gupta@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@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
>

[-- Attachment #1.2: Type: text/html, Size: 9917 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx