[Intel-gfx] [PATCH] drm/i915: Only check PPGTT bits when using PPGTT
Volkin, Bradley D
bradley.d.volkin at intel.com
Wed May 28 23:45:26 CEST 2014
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... :)
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
>
More information about the Intel-gfx
mailing list