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
On Wed, May 28, 2014 at 10:47 PM, <bradley.d.volkin@xxxxxxxxx> wrote:
> From: Brad Volkin <bradley.d.volkin@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> ---
>
> 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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
