[Intel-gfx] [PATCH] drm/i915: Only check PPGTT bits when using PPGTT

Daniel Vetter daniel at ffwll.ch
Thu May 29 00:07:14 CEST 2014


On Wed, May 28, 2014 at 02:45:26PM -0700, Volkin, Bradley D wrote:
> On Wed, May 28, 2014 at 02:29:34PM -0700, Daniel Vetter wrote:
> > Jesse reportedly has a patch somewhere to (finally!) enable ppgtt on
> > vlv. Would we still need any part of this with ppgtt support on vlv?
> > -Daniel
> 
> Of course, as soon as I accept that it'll never happen... :)

Jesse just submitted his patch, please review it to make sure it happens
;-)

Cheers, Daniel

> 
> Off the top of my head, keeping this would let us run the command parser in
> the case where ppgtt is force disabled, which might be nice. Otherwise the
> parser stops working and you lose the ability to LRI whitelisted registers,
> etc, which might not be what you expected.
> 
> My motivation was enabling the parser for vlv, which would happen without
> any parser changes if it gets ppgtt, so I'll leave it up to you.
> 
> Brad
> 
> > 
> > On Wed, May 28, 2014 at 10:47 PM,  <bradley.d.volkin at intel.com> wrote:
> > > From: Brad Volkin <bradley.d.volkin at intel.com>
> > >
> > > This extends use of the command parser to VLV.
> > >
> > > Note that the patch checks that the PPGTT bit is set appropriately when
> > > PPGTT is enabled but ignores it when PPGTT is disabled. It would be
> > > awkward to correctly invert the expected value to check that the bit is
> > > set appropriately in that case, and of limited value anyhow.
> > >
> > > Signed-off-by: Brad Volkin <bradley.d.volkin at intel.com>
> > > ---
> > >
> > > I've confirmed that the shmem pread setup stuff we added does fix the
> > > caching issues I saw previously. I've done some basic testing with this
> > > on both IVB and VLV and don't see regressions. I don't have any data on
> > > the VLV perf impact though.
> > >
> > > Also, I considered splitting the patch up a bit differently but decided
> > > that a single patch seemed ok. I'm happy to split it up a bit if that's
> > > what people prefer.
> > >
> > >  drivers/gpu/drm/i915/i915_cmd_parser.c | 187 +++++++++++++++++----------------
> > >  drivers/gpu/drm/i915/i915_drv.h        |   8 +-
> > >  2 files changed, 104 insertions(+), 91 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
> > > index 9d79543..fd35900 100644
> > > --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> > > +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> > > @@ -110,6 +110,7 @@
> > >  #define W CMD_DESC_REGISTER
> > >  #define B CMD_DESC_BITMASK
> > >  #define M CMD_DESC_MASTER
> > > +#define P CMD_DESC_PPGTT
> > >
> > >  /*            Command                          Mask   Fixed Len   Action
> > >               ---------------------------------------------------------- */
> > > @@ -124,20 +125,20 @@ static const struct drm_i915_cmd_descriptor common_cmds[] = {
> > >         CMD(  MI_STORE_DWORD_INDEX,             SMI,   !F,  0xFF,   R  ),
> > >         CMD(  MI_LOAD_REGISTER_IMM(1),          SMI,   !F,  0xFF,   W,
> > >               .reg = { .offset = 1, .mask = 0x007FFFFC }               ),
> > > -       CMD(  MI_STORE_REGISTER_MEM(1),         SMI,   !F,  0xFF,   W | B,
> > > +       CMD(  MI_STORE_REGISTER_MEM(1),         SMI,   !F,  0xFF,   W | P,
> > >               .reg = { .offset = 1, .mask = 0x007FFFFC },
> > > -             .bits = {{
> > > +             .ppgtt = {
> > >                         .offset = 0,
> > >                         .mask = MI_GLOBAL_GTT,
> > >                         .expected = 0,
> > > -             }},                                                      ),
> > > -       CMD(  MI_LOAD_REGISTER_MEM,             SMI,   !F,  0xFF,   W | B,
> > > +             },                                                       ),
> > > +       CMD(  MI_LOAD_REGISTER_MEM,             SMI,   !F,  0xFF,   W | P,
> > >               .reg = { .offset = 1, .mask = 0x007FFFFC },
> > > -             .bits = {{
> > > +             .ppgtt = {
> > >                         .offset = 0,
> > >                         .mask = MI_GLOBAL_GTT,
> > >                         .expected = 0,
> > > -             }},                                                      ),
> > > +             },                                                       ),
> > >         CMD(  MI_BATCH_BUFFER_START,            SMI,   !F,  0xFF,   S  ),
> > >  };
> > >
> > > @@ -149,31 +150,31 @@ static const struct drm_i915_cmd_descriptor render_cmds[] = {
> > >         CMD(  MI_DISPLAY_FLIP,                  SMI,   !F,  0xFF,   R  ),
> > >         CMD(  MI_SET_CONTEXT,                   SMI,   !F,  0xFF,   R  ),
> > >         CMD(  MI_URB_CLEAR,                     SMI,   !F,  0xFF,   S  ),
> > > -       CMD(  MI_STORE_DWORD_IMM,               SMI,   !F,  0x3F,   B,
> > > -             .bits = {{
> > > +       CMD(  MI_STORE_DWORD_IMM,               SMI,   !F,  0x3F,   P,
> > > +             .ppgtt = {
> > >                         .offset = 0,
> > >                         .mask = MI_GLOBAL_GTT,
> > >                         .expected = 0,
> > > -             }},                                                      ),
> > > +             },                                                       ),
> > >         CMD(  MI_UPDATE_GTT,                    SMI,   !F,  0xFF,   R  ),
> > > -       CMD(  MI_CLFLUSH,                       SMI,   !F,  0x3FF,  B,
> > > -             .bits = {{
> > > +       CMD(  MI_CLFLUSH,                       SMI,   !F,  0x3FF,  P,
> > > +             .ppgtt = {
> > >                         .offset = 0,
> > >                         .mask = MI_GLOBAL_GTT,
> > >                         .expected = 0,
> > > -             }},                                                      ),
> > > -       CMD(  MI_REPORT_PERF_COUNT,             SMI,   !F,  0x3F,   B,
> > > -             .bits = {{
> > > +             },                                                       ),
> > > +       CMD(  MI_REPORT_PERF_COUNT,             SMI,   !F,  0x3F,   P,
> > > +             .ppgtt = {
> > >                         .offset = 1,
> > >                         .mask = MI_REPORT_PERF_COUNT_GGTT,
> > >                         .expected = 0,
> > > -             }},                                                      ),
> > > -       CMD(  MI_CONDITIONAL_BATCH_BUFFER_END,  SMI,   !F,  0xFF,   B,
> > > -             .bits = {{
> > > +             },                                                       ),
> > > +       CMD(  MI_CONDITIONAL_BATCH_BUFFER_END,  SMI,   !F,  0xFF,   P,
> > > +             .ppgtt = {
> > >                         .offset = 0,
> > >                         .mask = MI_GLOBAL_GTT,
> > >                         .expected = 0,
> > > -             }},                                                      ),
> > > +             },                                                       ),
> > >         CMD(  GFX_OP_3DSTATE_VF_STATISTICS,     S3D,    F,  1,      S  ),
> > >         CMD(  PIPELINE_SELECT,                  S3D,    F,  1,      S  ),
> > >         CMD(  MEDIA_VFE_STATE,                  S3D,   !F,  0xFFFF, B,
> > > @@ -185,7 +186,14 @@ static const struct drm_i915_cmd_descriptor render_cmds[] = {
> > >         CMD(  GPGPU_OBJECT,                     S3D,   !F,  0xFF,   S  ),
> > >         CMD(  GPGPU_WALKER,                     S3D,   !F,  0xFF,   S  ),
> > >         CMD(  GFX_OP_3DSTATE_SO_DECL_LIST,      S3D,   !F,  0x1FF,  S  ),
> > > -       CMD(  GFX_OP_PIPE_CONTROL(5),           S3D,   !F,  0xFF,   B,
> > > +       CMD(  GFX_OP_PIPE_CONTROL(5),           S3D,   !F,  0xFF,   B | P,
> > > +             .ppgtt = {
> > > +                       .offset = 1,
> > > +                       .mask = PIPE_CONTROL_GLOBAL_GTT_IVB,
> > > +                       .expected = 0,
> > > +                       .condition_offset = 1,
> > > +                       .condition_mask = PIPE_CONTROL_POST_SYNC_OP_MASK,
> > > +             },
> > >               .bits = {{
> > >                         .offset = 1,
> > >                         .mask = (PIPE_CONTROL_MMIO_WRITE | PIPE_CONTROL_NOTIFY),
> > > @@ -193,8 +201,7 @@ static const struct drm_i915_cmd_descriptor render_cmds[] = {
> > >               },
> > >               {
> > >                         .offset = 1,
> > > -                       .mask = (PIPE_CONTROL_GLOBAL_GTT_IVB |
> > > -                                PIPE_CONTROL_STORE_DATA_INDEX),
> > > +                       .mask = PIPE_CONTROL_STORE_DATA_INDEX,
> > >                         .expected = 0,
> > >                         .condition_offset = 1,
> > >                         .condition_mask = PIPE_CONTROL_POST_SYNC_OP_MASK,
> > > @@ -224,26 +231,26 @@ static const struct drm_i915_cmd_descriptor hsw_render_cmds[] = {
> > >
> > >  static const struct drm_i915_cmd_descriptor video_cmds[] = {
> > >         CMD(  MI_ARB_ON_OFF,                    SMI,    F,  1,      R  ),
> > > -       CMD(  MI_STORE_DWORD_IMM,               SMI,   !F,  0xFF,   B,
> > > -             .bits = {{
> > > +       CMD(  MI_STORE_DWORD_IMM,               SMI,   !F,  0xFF,   P,
> > > +             .ppgtt = {
> > >                         .offset = 0,
> > >                         .mask = MI_GLOBAL_GTT,
> > >                         .expected = 0,
> > > -             }},                                                      ),
> > > +             },                                                       ),
> > >         CMD(  MI_UPDATE_GTT,                    SMI,   !F,  0x3F,   R  ),
> > > -       CMD(  MI_FLUSH_DW,                      SMI,   !F,  0x3F,   B,
> > > -             .bits = {{
> > > -                       .offset = 0,
> > > -                       .mask = MI_FLUSH_DW_NOTIFY,
> > > -                       .expected = 0,
> > > -             },
> > > -             {
> > > +       CMD(  MI_FLUSH_DW,                      SMI,   !F,  0x3F,   B | P,
> > > +             .ppgtt = {
> > >                         .offset = 1,
> > >                         .mask = MI_FLUSH_DW_USE_GTT,
> > >                         .expected = 0,
> > >                         .condition_offset = 0,
> > >                         .condition_mask = MI_FLUSH_DW_OP_MASK,
> > >               },
> > > +             .bits = {{
> > > +                       .offset = 0,
> > > +                       .mask = MI_FLUSH_DW_NOTIFY,
> > > +                       .expected = 0,
> > > +             },
> > >               {
> > >                         .offset = 0,
> > >                         .mask = MI_FLUSH_DW_STORE_INDEX,
> > > @@ -251,12 +258,12 @@ static const struct drm_i915_cmd_descriptor video_cmds[] = {
> > >                         .condition_offset = 0,
> > >                         .condition_mask = MI_FLUSH_DW_OP_MASK,
> > >               }},                                                      ),
> > > -       CMD(  MI_CONDITIONAL_BATCH_BUFFER_END,  SMI,   !F,  0xFF,   B,
> > > -             .bits = {{
> > > +       CMD(  MI_CONDITIONAL_BATCH_BUFFER_END,  SMI,   !F,  0xFF,   P,
> > > +             .ppgtt = {
> > >                         .offset = 0,
> > >                         .mask = MI_GLOBAL_GTT,
> > >                         .expected = 0,
> > > -             }},                                                      ),
> > > +             },                                                       ),
> > >         /*
> > >          * MFX_WAIT doesn't fit the way we handle length for most commands.
> > >          * It has a length field but it uses a non-standard length bias.
> > > @@ -267,26 +274,26 @@ static const struct drm_i915_cmd_descriptor video_cmds[] = {
> > >
> > >  static const struct drm_i915_cmd_descriptor vecs_cmds[] = {
> > >         CMD(  MI_ARB_ON_OFF,                    SMI,    F,  1,      R  ),
> > > -       CMD(  MI_STORE_DWORD_IMM,               SMI,   !F,  0xFF,   B,
> > > -             .bits = {{
> > > +       CMD(  MI_STORE_DWORD_IMM,               SMI,   !F,  0xFF,   B | P,
> > > +             .ppgtt = {
> > >                         .offset = 0,
> > >                         .mask = MI_GLOBAL_GTT,
> > >                         .expected = 0,
> > > -             }},                                                      ),
> > > +             },                                                       ),
> > >         CMD(  MI_UPDATE_GTT,                    SMI,   !F,  0x3F,   R  ),
> > > -       CMD(  MI_FLUSH_DW,                      SMI,   !F,  0x3F,   B,
> > > -             .bits = {{
> > > -                       .offset = 0,
> > > -                       .mask = MI_FLUSH_DW_NOTIFY,
> > > -                       .expected = 0,
> > > -             },
> > > -             {
> > > +       CMD(  MI_FLUSH_DW,                      SMI,   !F,  0x3F,   B | P,
> > > +             .ppgtt = {
> > >                         .offset = 1,
> > >                         .mask = MI_FLUSH_DW_USE_GTT,
> > >                         .expected = 0,
> > >                         .condition_offset = 0,
> > >                         .condition_mask = MI_FLUSH_DW_OP_MASK,
> > >               },
> > > +             .bits = {{
> > > +                       .offset = 0,
> > > +                       .mask = MI_FLUSH_DW_NOTIFY,
> > > +                       .expected = 0,
> > > +             },
> > >               {
> > >                         .offset = 0,
> > >                         .mask = MI_FLUSH_DW_STORE_INDEX,
> > > @@ -294,36 +301,36 @@ static const struct drm_i915_cmd_descriptor vecs_cmds[] = {
> > >                         .condition_offset = 0,
> > >                         .condition_mask = MI_FLUSH_DW_OP_MASK,
> > >               }},                                                      ),
> > > -       CMD(  MI_CONDITIONAL_BATCH_BUFFER_END,  SMI,   !F,  0xFF,   B,
> > > -             .bits = {{
> > > +       CMD(  MI_CONDITIONAL_BATCH_BUFFER_END,  SMI,   !F,  0xFF,   P,
> > > +             .ppgtt = {
> > >                         .offset = 0,
> > >                         .mask = MI_GLOBAL_GTT,
> > >                         .expected = 0,
> > > -             }},                                                      ),
> > > +             },                                                       ),
> > >  };
> > >
> > >  static const struct drm_i915_cmd_descriptor blt_cmds[] = {
> > >         CMD(  MI_DISPLAY_FLIP,                  SMI,   !F,  0xFF,   R  ),
> > > -       CMD(  MI_STORE_DWORD_IMM,               SMI,   !F,  0x3FF,  B,
> > > -             .bits = {{
> > > +       CMD(  MI_STORE_DWORD_IMM,               SMI,   !F,  0x3FF,  P,
> > > +             .ppgtt = {
> > >                         .offset = 0,
> > >                         .mask = MI_GLOBAL_GTT,
> > >                         .expected = 0,
> > > -             }},                                                      ),
> > > +             },                                                       ),
> > >         CMD(  MI_UPDATE_GTT,                    SMI,   !F,  0x3F,   R  ),
> > > -       CMD(  MI_FLUSH_DW,                      SMI,   !F,  0x3F,   B,
> > > -             .bits = {{
> > > -                       .offset = 0,
> > > -                       .mask = MI_FLUSH_DW_NOTIFY,
> > > -                       .expected = 0,
> > > -             },
> > > -             {
> > > +       CMD(  MI_FLUSH_DW,                      SMI,   !F,  0x3F,   B | P,
> > > +             .ppgtt = {
> > >                         .offset = 1,
> > >                         .mask = MI_FLUSH_DW_USE_GTT,
> > >                         .expected = 0,
> > >                         .condition_offset = 0,
> > >                         .condition_mask = MI_FLUSH_DW_OP_MASK,
> > >               },
> > > +             .bits = {{
> > > +                       .offset = 0,
> > > +                       .mask = MI_FLUSH_DW_NOTIFY,
> > > +                       .expected = 0,
> > > +             },
> > >               {
> > >                         .offset = 0,
> > >                         .mask = MI_FLUSH_DW_STORE_INDEX,
> > > @@ -351,6 +358,7 @@ static const struct drm_i915_cmd_descriptor hsw_blt_cmds[] = {
> > >  #undef W
> > >  #undef B
> > >  #undef M
> > > +#undef P
> > >
> > >  static const struct drm_i915_cmd_table gen7_render_cmds[] = {
> > >         { common_cmds, ARRAY_SIZE(common_cmds) },
> > > @@ -797,6 +805,34 @@ static bool valid_reg(const u32 *table, int count, u32 addr)
> > >         return false;
> > >  }
> > >
> > > +static bool valid_bits(const int ring_id,
> > > +                      const struct drm_i915_cmd_bits *bits,
> > > +                      const u32 *cmd)
> > > +{
> > > +       u32 dword;
> > > +
> > > +       if (bits->condition_mask != 0) {
> > > +               u32 offset = bits->condition_offset;
> > > +               u32 condition = cmd[offset] & bits->condition_mask;
> > > +
> > > +               if (condition == 0)
> > > +                       return true;
> > > +       }
> > > +
> > > +       dword = cmd[bits->offset] & bits->mask;
> > > +
> > > +       if (dword != bits->expected) {
> > > +               DRM_DEBUG_DRIVER("CMD: Rejected command 0x%08X for bitmask 0x%08X (exp=0x%08X act=0x%08X) (ring=%d)\n",
> > > +                                *cmd,
> > > +                                bits->mask,
> > > +                                bits->expected,
> > > +                                dword, ring_id);
> > > +               return false;
> > > +       }
> > > +
> > > +       return true;
> > > +}
> > > +
> > >  static u32 *vmap_batch(struct drm_i915_gem_object *obj)
> > >  {
> > >         int i;
> > > @@ -839,19 +875,9 @@ finish:
> > >   */
> > >  bool i915_needs_cmd_parser(struct intel_engine_cs *ring)
> > >  {
> > > -       struct drm_i915_private *dev_priv = ring->dev->dev_private;
> > > -
> > >         if (!ring->needs_cmd_parser)
> > >                 return false;
> > >
> > > -       /*
> > > -        * XXX: VLV is Gen7 and therefore has cmd_tables, but has PPGTT
> > > -        * disabled. That will cause all of the parser's PPGTT checks to
> > > -        * fail. For now, disable parsing when PPGTT is off.
> > > -        */
> > > -       if (!dev_priv->mm.aliasing_ppgtt)
> > > -               return false;
> > > -
> > >         return (i915.enable_cmd_parser == 1);
> > >  }
> > >
> > > @@ -907,36 +933,19 @@ static bool check_cmd(const struct intel_engine_cs *ring,
> > >                 }
> > >         }
> > >
> > > +       if ((desc->flags & CMD_DESC_PPGTT) && USES_PPGTT(ring->dev))
> > > +               if (!valid_bits(ring->id, &desc->ppgtt, cmd))
> > > +                       return false;
> > > +
> > >         if (desc->flags & CMD_DESC_BITMASK) {
> > >                 int i;
> > >
> > >                 for (i = 0; i < MAX_CMD_DESC_BITMASKS; i++) {
> > > -                       u32 dword;
> > > -
> > >                         if (desc->bits[i].mask == 0)
> > >                                 break;
> > >
> > > -                       if (desc->bits[i].condition_mask != 0) {
> > > -                               u32 offset =
> > > -                                       desc->bits[i].condition_offset;
> > > -                               u32 condition = cmd[offset] &
> > > -                                       desc->bits[i].condition_mask;
> > > -
> > > -                               if (condition == 0)
> > > -                                       continue;
> > > -                       }
> > > -
> > > -                       dword = cmd[desc->bits[i].offset] &
> > > -                               desc->bits[i].mask;
> > > -
> > > -                       if (dword != desc->bits[i].expected) {
> > > -                               DRM_DEBUG_DRIVER("CMD: Rejected command 0x%08X for bitmask 0x%08X (exp=0x%08X act=0x%08X) (ring=%d)\n",
> > > -                                                *cmd,
> > > -                                                desc->bits[i].mask,
> > > -                                                desc->bits[i].expected,
> > > -                                                dword, ring->id);
> > > +                       if (!valid_bits(ring->id, &desc->bits[i], cmd))
> > >                                 return false;
> > > -                       }
> > >                 }
> > >         }
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index bea9ab40..6eead7a 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -1787,6 +1787,9 @@ struct drm_i915_cmd_descriptor {
> > >          *                    register whitelist for the appropriate ring
> > >          * CMD_DESC_MASTER: The command is allowed if the submitting process
> > >          *                  is the DRM master
> > > +        * CMD_DESC_PPGTT: The command contains a bit that indicates GGTT or
> > > +        *                 PPGTT access, which must be checked only when
> > > +        *                 PPGTT is enabled
> > >          */
> > >         u32 flags;
> > >  #define CMD_DESC_FIXED    (1<<0)
> > > @@ -1795,6 +1798,7 @@ struct drm_i915_cmd_descriptor {
> > >  #define CMD_DESC_REGISTER (1<<3)
> > >  #define CMD_DESC_BITMASK  (1<<4)
> > >  #define CMD_DESC_MASTER   (1<<5)
> > > +#define CMD_DESC_PPGTT    (1<<6)
> > >
> > >         /*
> > >          * The command's unique identification bits and the bitmask to get them.
> > > @@ -1840,13 +1844,13 @@ struct drm_i915_cmd_descriptor {
> > >          * only performs the check when the bits specified by condition_mask
> > >          * are non-zero.
> > >          */
> > > -       struct {
> > > +       struct drm_i915_cmd_bits {
> > >                 u32 offset;
> > >                 u32 mask;
> > >                 u32 expected;
> > >                 u32 condition_offset;
> > >                 u32 condition_mask;
> > > -       } bits[MAX_CMD_DESC_BITMASKS];
> > > +       } bits[MAX_CMD_DESC_BITMASKS], ppgtt;
> > >  };
> > >
> > >  /*
> > > --
> > > 1.8.3.2
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx at 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
> > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch



More information about the Intel-gfx mailing list