| Message ID | 1418078030-17690-6-git-send-email-michael.h.nguyen@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
PNV 364/364 364/364
ILK +1 365/366 366/366
SNB 450/450 450/450
IVB -17 498/498 481/498
BYT 289/289 289/289
HSW 564/564 564/564
BDW 417/417 417/417
-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
ILK igt_kms_flip_wf_vblank-ts-check DMESG_WARN(2, M26)PASS(18, M26M37) PASS(1, M37)
IVB igt_kms_3d DMESG_WARN(4, M34)PASS(1, M34) DMESG_WARN(1, M34)
IVB igt_kms_cursor_crc_cursor-128x128-onscreen NSPT(4, M34)PASS(1, M34) NSPT(1, M34)
IVB igt_kms_cursor_crc_cursor-128x128-random NSPT(4, M34)PASS(1, M34) NSPT(1, M34)
IVB igt_kms_cursor_crc_cursor-128x128-sliding NSPT(4, M34)PASS(1, M34) NSPT(1, M34)
IVB igt_kms_cursor_crc_cursor-256x256-offscreen NSPT(4, M34)PASS(1, M34) NSPT(1, M34)
IVB igt_kms_cursor_crc_cursor-256x256-onscreen NSPT(4, M34)PASS(1, M34) NSPT(1, M34)
IVB igt_kms_cursor_crc_cursor-256x256-sliding NSPT(4, M34)PASS(1, M34) NSPT(1, M34)
IVB igt_kms_cursor_crc_cursor-64x64-offscreen NSPT(4, M34)PASS(2, M34M21) NSPT(1, M34)
IVB igt_kms_cursor_crc_cursor-64x64-onscreen NSPT(4, M34)PASS(1, M34) NSPT(1, M34)
IVB igt_kms_cursor_crc_cursor-64x64-random NSPT(4, M34)PASS(1, M34) NSPT(1, M34)
IVB igt_kms_cursor_crc_cursor-64x64-sliding FAIL(1, M21)NSPT(4, M34)PASS(2, M34M21) NSPT(1, M34)
IVB igt_kms_cursor_crc_cursor-size-change NSPT(4, M34)PASS(1, M34) NSPT(1, M34)
IVB igt_kms_fence_pin_leak NSPT(4, M34)PASS(1, M34) NSPT(1, M34)
IVB igt_kms_mmio_vs_cs_flip_setcrtc_vs_cs_flip NSPT(4, M34)PASS(1, M34) NSPT(1, M34)
IVB igt_kms_mmio_vs_cs_flip_setplane_vs_cs_flip NSPT(4, M34)PASS(1, M34) NSPT(1, M34)
IVB igt_kms_rotation_crc_primary-rotation NSPT(4, M34)PASS(1, M34) NSPT(1, M34)
IVB igt_kms_rotation_crc_sprite-rotation NSPT(4, M34)PASS(1, M34) NSPT(1, M34)
Note: You need to pay more attention to line start with '*'
On Mon, Dec 08, 2014 at 02:33:50PM -0800, michael.h.nguyen@intel.com wrote: > From: Brad Volkin <bradley.d.volkin@intel.com> > > Move it to a separate function since the main do_execbuffer function > already has so much going on. > > v2: > - Move pin/unpin calls inside i915_parse_cmds() (Chris W, v4 7/7 > feedback) > > Issue: VIZ-4719 > Signed-off-by: Brad Volkin <bradley.d.volkin@intel.com> > > Conflicts: > drivers/gpu/drm/i915/i915_gem_execbuffer.c Please remove those when rebasing. When actually something changed please mention that in the patch revlog, e.g. v3: Rebase (conflicts with patch "foo" in execbuf code). But if it's a boring rebase without any functional conflicts I wouldn't bother with a changelog entry. -Daniel > --- > drivers/gpu/drm/i915/i915_cmd_parser.c | 8 ++ > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 126 ++++++++++++++++------------- > 2 files changed, 77 insertions(+), 57 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c > index 118079d..80b1b28 100644 > --- a/drivers/gpu/drm/i915/i915_cmd_parser.c > +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c > @@ -1042,10 +1042,17 @@ int i915_parse_cmds(struct intel_engine_cs *ring, > struct drm_i915_cmd_descriptor default_desc = { 0 }; > bool oacontrol_set = false; /* OACONTROL tracking. See check_cmd() */ > > + ret = i915_gem_obj_ggtt_pin(shadow_batch_obj, 4096, 0); > + if (ret) { > + DRM_DEBUG_DRIVER("CMD: Failed to pin shadow batch\n"); > + return -1; > + } > + > batch_base = copy_batch(shadow_batch_obj, batch_obj, > batch_start_offset, batch_len); > if (IS_ERR(batch_base)) { > DRM_DEBUG_DRIVER("CMD: Failed to copy batch\n"); > + i915_gem_object_ggtt_unpin(shadow_batch_obj); > return PTR_ERR(batch_base); > } > > @@ -1116,6 +1123,7 @@ int i915_parse_cmds(struct intel_engine_cs *ring, > } > > vunmap(batch_base); > + i915_gem_object_ggtt_unpin(shadow_batch_obj); > > return ret; > } > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > index 2071938..3d36465 100644 > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > @@ -1069,6 +1069,65 @@ i915_emit_box(struct intel_engine_cs *ring, > return 0; > } > > +static struct drm_i915_gem_object* > +i915_gem_execbuffer_parse(struct intel_engine_cs *ring, > + struct drm_i915_gem_exec_object2 *shadow_exec_entry, > + struct eb_vmas *eb, > + struct drm_i915_gem_object *batch_obj, > + u32 batch_start_offset, > + u32 batch_len, > + bool is_master, > + u32 *flags) > +{ > + struct drm_i915_private *dev_priv = to_i915(batch_obj->base.dev); > + struct drm_i915_gem_object *shadow_batch_obj; > + int ret; > + > + shadow_batch_obj = i915_gem_batch_pool_get(&dev_priv->mm.batch_pool, > + batch_obj->base.size); > + if (IS_ERR(shadow_batch_obj)) > + return shadow_batch_obj; > + > + ret = i915_parse_cmds(ring, > + batch_obj, > + shadow_batch_obj, > + batch_start_offset, > + batch_len, > + is_master); > + if (ret) { > + if (ret == -EACCES) > + return batch_obj; > + } else { > + struct i915_vma *vma; > + > + memset(shadow_exec_entry, 0, sizeof(*shadow_exec_entry)); > + > + vma = i915_gem_obj_to_ggtt(shadow_batch_obj); > + vma->exec_entry = shadow_exec_entry; > + vma->exec_entry->flags = __EXEC_OBJECT_PURGEABLE; > + drm_gem_object_reference(&shadow_batch_obj->base); > + list_add_tail(&vma->exec_list, &eb->vmas); > + > + shadow_batch_obj->base.pending_read_domains = > + batch_obj->base.pending_read_domains; > + > + /* > + * Set the DISPATCH_SECURE bit to remove the NON_SECURE > + * bit from MI_BATCH_BUFFER_START commands issued in the > + * dispatch_execbuffer implementations. We specifically > + * don't want that set when the command parser is > + * enabled. > + * > + * FIXME: with aliasing ppgtt, buffers that should only > + * be in ggtt still end up in the aliasing ppgtt. remove > + * this check when that is fixed. > + */ > + if (USES_FULL_PPGTT(dev)) > + *flags |= I915_DISPATCH_SECURE; > + } > + > + return ret ? ERR_PTR(ret) : shadow_batch_obj; > +} > > int > i915_gem_ringbuffer_submission(struct drm_device *dev, struct drm_file *file, > @@ -1286,7 +1345,6 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, > struct drm_i915_private *dev_priv = dev->dev_private; > struct eb_vmas *eb; > struct drm_i915_gem_object *batch_obj; > - struct drm_i915_gem_object *shadow_batch_obj = NULL; > struct drm_i915_gem_exec_object2 shadow_exec_entry; > struct intel_engine_cs *ring; > struct intel_context *ctx; > @@ -1406,63 +1464,17 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, > } > > if (i915_needs_cmd_parser(ring)) { > - shadow_batch_obj = > - i915_gem_batch_pool_get(&dev_priv->mm.batch_pool, > - batch_obj->base.size); > - if (IS_ERR(shadow_batch_obj)) { > - ret = PTR_ERR(shadow_batch_obj); > - /* Don't try to clean up the obj in the error path */ > - shadow_batch_obj = NULL; > - goto err; > - } > - > - shadow_batch_obj->madv = I915_MADV_WILLNEED; > - > - ret = i915_gem_obj_ggtt_pin(shadow_batch_obj, 4096, 0); > - if (ret) > + batch_obj = i915_gem_execbuffer_parse(ring, > + &shadow_exec_entry, > + eb, > + batch_obj, > + args->batch_start_offset, > + args->batch_len, > + file->is_master, > + &flags); > + if (IS_ERR(batch_obj)) { > + ret = PTR_ERR(batch_obj); > goto err; > - > - ret = i915_parse_cmds(ring, > - batch_obj, > - shadow_batch_obj, > - args->batch_start_offset, > - args->batch_len, > - file->is_master); > - i915_gem_object_ggtt_unpin(shadow_batch_obj); > - > - if (ret) { > - if (ret != -EACCES) > - goto err; > - } else { > - struct i915_vma *vma; > - > - memset(&shadow_exec_entry, 0, > - sizeof(shadow_exec_entry)); > - > - vma = i915_gem_obj_to_ggtt(shadow_batch_obj); > - vma->exec_entry = &shadow_exec_entry; > - vma->exec_entry->flags = __EXEC_OBJECT_PURGEABLE; > - drm_gem_object_reference(&shadow_batch_obj->base); > - list_add_tail(&vma->exec_list, &eb->vmas); > - > - shadow_batch_obj->base.pending_read_domains = > - batch_obj->base.pending_read_domains; > - > - batch_obj = shadow_batch_obj; > - > - /* > - * Set the DISPATCH_SECURE bit to remove the NON_SECURE > - * bit from MI_BATCH_BUFFER_START commands issued in the > - * dispatch_execbuffer implementations. We specifically > - * don't want that set when the command parser is > - * enabled. > - * > - * FIXME: with aliasing ppgtt, buffers that should only > - * be in ggtt still end up in the aliasing ppgtt. remove > - * this check when that is fixed. > - */ > - if (USES_FULL_PPGTT(dev)) > - flags |= I915_DISPATCH_SECURE; > } > } > > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 12/09/2014 05:22 AM, Daniel Vetter wrote: > On Mon, Dec 08, 2014 at 02:33:50PM -0800, michael.h.nguyen@intel.com wrote: >> From: Brad Volkin <bradley.d.volkin@intel.com> >> >> Move it to a separate function since the main do_execbuffer function >> already has so much going on. >> >> v2: >> - Move pin/unpin calls inside i915_parse_cmds() (Chris W, v4 7/7 >> feedback) >> >> Issue: VIZ-4719 >> Signed-off-by: Brad Volkin <bradley.d.volkin@intel.com> >> >> Conflicts: >> drivers/gpu/drm/i915/i915_gem_execbuffer.c > > Please remove those when rebasing. When actually something changed please > mention that in the patch revlog, e.g. > > v3: Rebase (conflicts with patch "foo" in execbuf code). > > But if it's a boring rebase without any functional conflicts I wouldn't > bother with a changelog entry. Noted. In this case, its boring. Thanks, Mike > -Daniel > >> --- >> drivers/gpu/drm/i915/i915_cmd_parser.c | 8 ++ >> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 126 ++++++++++++++++------------- >> 2 files changed, 77 insertions(+), 57 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c >> index 118079d..80b1b28 100644 >> --- a/drivers/gpu/drm/i915/i915_cmd_parser.c >> +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c >> @@ -1042,10 +1042,17 @@ int i915_parse_cmds(struct intel_engine_cs *ring, >> struct drm_i915_cmd_descriptor default_desc = { 0 }; >> bool oacontrol_set = false; /* OACONTROL tracking. See check_cmd() */ >> >> + ret = i915_gem_obj_ggtt_pin(shadow_batch_obj, 4096, 0); >> + if (ret) { >> + DRM_DEBUG_DRIVER("CMD: Failed to pin shadow batch\n"); >> + return -1; >> + } >> + >> batch_base = copy_batch(shadow_batch_obj, batch_obj, >> batch_start_offset, batch_len); >> if (IS_ERR(batch_base)) { >> DRM_DEBUG_DRIVER("CMD: Failed to copy batch\n"); >> + i915_gem_object_ggtt_unpin(shadow_batch_obj); >> return PTR_ERR(batch_base); >> } >> >> @@ -1116,6 +1123,7 @@ int i915_parse_cmds(struct intel_engine_cs *ring, >> } >> >> vunmap(batch_base); >> + i915_gem_object_ggtt_unpin(shadow_batch_obj); >> >> return ret; >> } >> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c >> index 2071938..3d36465 100644 >> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c >> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c >> @@ -1069,6 +1069,65 @@ i915_emit_box(struct intel_engine_cs *ring, >> return 0; >> } >> >> +static struct drm_i915_gem_object* >> +i915_gem_execbuffer_parse(struct intel_engine_cs *ring, >> + struct drm_i915_gem_exec_object2 *shadow_exec_entry, >> + struct eb_vmas *eb, >> + struct drm_i915_gem_object *batch_obj, >> + u32 batch_start_offset, >> + u32 batch_len, >> + bool is_master, >> + u32 *flags) >> +{ >> + struct drm_i915_private *dev_priv = to_i915(batch_obj->base.dev); >> + struct drm_i915_gem_object *shadow_batch_obj; >> + int ret; >> + >> + shadow_batch_obj = i915_gem_batch_pool_get(&dev_priv->mm.batch_pool, >> + batch_obj->base.size); >> + if (IS_ERR(shadow_batch_obj)) >> + return shadow_batch_obj; >> + >> + ret = i915_parse_cmds(ring, >> + batch_obj, >> + shadow_batch_obj, >> + batch_start_offset, >> + batch_len, >> + is_master); >> + if (ret) { >> + if (ret == -EACCES) >> + return batch_obj; >> + } else { >> + struct i915_vma *vma; >> + >> + memset(shadow_exec_entry, 0, sizeof(*shadow_exec_entry)); >> + >> + vma = i915_gem_obj_to_ggtt(shadow_batch_obj); >> + vma->exec_entry = shadow_exec_entry; >> + vma->exec_entry->flags = __EXEC_OBJECT_PURGEABLE; >> + drm_gem_object_reference(&shadow_batch_obj->base); >> + list_add_tail(&vma->exec_list, &eb->vmas); >> + >> + shadow_batch_obj->base.pending_read_domains = >> + batch_obj->base.pending_read_domains; >> + >> + /* >> + * Set the DISPATCH_SECURE bit to remove the NON_SECURE >> + * bit from MI_BATCH_BUFFER_START commands issued in the >> + * dispatch_execbuffer implementations. We specifically >> + * don't want that set when the command parser is >> + * enabled. >> + * >> + * FIXME: with aliasing ppgtt, buffers that should only >> + * be in ggtt still end up in the aliasing ppgtt. remove >> + * this check when that is fixed. >> + */ >> + if (USES_FULL_PPGTT(dev)) >> + *flags |= I915_DISPATCH_SECURE; >> + } >> + >> + return ret ? ERR_PTR(ret) : shadow_batch_obj; >> +} >> >> int >> i915_gem_ringbuffer_submission(struct drm_device *dev, struct drm_file *file, >> @@ -1286,7 +1345,6 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, >> struct drm_i915_private *dev_priv = dev->dev_private; >> struct eb_vmas *eb; >> struct drm_i915_gem_object *batch_obj; >> - struct drm_i915_gem_object *shadow_batch_obj = NULL; >> struct drm_i915_gem_exec_object2 shadow_exec_entry; >> struct intel_engine_cs *ring; >> struct intel_context *ctx; >> @@ -1406,63 +1464,17 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, >> } >> >> if (i915_needs_cmd_parser(ring)) { >> - shadow_batch_obj = >> - i915_gem_batch_pool_get(&dev_priv->mm.batch_pool, >> - batch_obj->base.size); >> - if (IS_ERR(shadow_batch_obj)) { >> - ret = PTR_ERR(shadow_batch_obj); >> - /* Don't try to clean up the obj in the error path */ >> - shadow_batch_obj = NULL; >> - goto err; >> - } >> - >> - shadow_batch_obj->madv = I915_MADV_WILLNEED; >> - >> - ret = i915_gem_obj_ggtt_pin(shadow_batch_obj, 4096, 0); >> - if (ret) >> + batch_obj = i915_gem_execbuffer_parse(ring, >> + &shadow_exec_entry, >> + eb, >> + batch_obj, >> + args->batch_start_offset, >> + args->batch_len, >> + file->is_master, >> + &flags); >> + if (IS_ERR(batch_obj)) { >> + ret = PTR_ERR(batch_obj); >> goto err; >> - >> - ret = i915_parse_cmds(ring, >> - batch_obj, >> - shadow_batch_obj, >> - args->batch_start_offset, >> - args->batch_len, >> - file->is_master); >> - i915_gem_object_ggtt_unpin(shadow_batch_obj); >> - >> - if (ret) { >> - if (ret != -EACCES) >> - goto err; >> - } else { >> - struct i915_vma *vma; >> - >> - memset(&shadow_exec_entry, 0, >> - sizeof(shadow_exec_entry)); >> - >> - vma = i915_gem_obj_to_ggtt(shadow_batch_obj); >> - vma->exec_entry = &shadow_exec_entry; >> - vma->exec_entry->flags = __EXEC_OBJECT_PURGEABLE; >> - drm_gem_object_reference(&shadow_batch_obj->base); >> - list_add_tail(&vma->exec_list, &eb->vmas); >> - >> - shadow_batch_obj->base.pending_read_domains = >> - batch_obj->base.pending_read_domains; >> - >> - batch_obj = shadow_batch_obj; >> - >> - /* >> - * Set the DISPATCH_SECURE bit to remove the NON_SECURE >> - * bit from MI_BATCH_BUFFER_START commands issued in the >> - * dispatch_execbuffer implementations. We specifically >> - * don't want that set when the command parser is >> - * enabled. >> - * >> - * FIXME: with aliasing ppgtt, buffers that should only >> - * be in ggtt still end up in the aliasing ppgtt. remove >> - * this check when that is fixed. >> - */ >> - if (USES_FULL_PPGTT(dev)) >> - flags |= I915_DISPATCH_SECURE; >> } >> } >> >> -- >> 1.9.1 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
> -----Original Message----- > From: Nguyen, Michael H > Sent: Monday, December 08, 2014 10:34 PM > To: intel-gfx@lists.freedesktop.org > Cc: Bloomfield, Jon; Brad Volkin > Subject: [PATCH v6 5/5] drm/i915: Tidy up execbuffer command parsing code > > From: Brad Volkin <bradley.d.volkin@intel.com> > > Move it to a separate function since the main do_execbuffer function > already has so much going on. > > v2: > - Move pin/unpin calls inside i915_parse_cmds() (Chris W, v4 7/7 > feedback) > > Issue: VIZ-4719 > Signed-off-by: Brad Volkin <bradley.d.volkin@intel.com> > > Conflicts: > drivers/gpu/drm/i915/i915_gem_execbuffer.c > --- > drivers/gpu/drm/i915/i915_cmd_parser.c | 8 ++ > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 126 ++++++++++++++++--- > ---------- > 2 files changed, 77 insertions(+), 57 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c > b/drivers/gpu/drm/i915/i915_cmd_parser.c > index 118079d..80b1b28 100644 > --- a/drivers/gpu/drm/i915/i915_cmd_parser.c > +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c > @@ -1042,10 +1042,17 @@ int i915_parse_cmds(struct intel_engine_cs > *ring, > struct drm_i915_cmd_descriptor default_desc = { 0 }; > bool oacontrol_set = false; /* OACONTROL tracking. See > check_cmd() */ > > + ret = i915_gem_obj_ggtt_pin(shadow_batch_obj, 4096, 0); > + if (ret) { > + DRM_DEBUG_DRIVER("CMD: Failed to pin shadow batch\n"); > + return -1; > + } > + > batch_base = copy_batch(shadow_batch_obj, batch_obj, > batch_start_offset, batch_len); > if (IS_ERR(batch_base)) { > DRM_DEBUG_DRIVER("CMD: Failed to copy batch\n"); > + i915_gem_object_ggtt_unpin(shadow_batch_obj); > return PTR_ERR(batch_base); > } > > @@ -1116,6 +1123,7 @@ int i915_parse_cmds(struct intel_engine_cs *ring, > } > > vunmap(batch_base); > + i915_gem_object_ggtt_unpin(shadow_batch_obj); > > return ret; > } > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > index 2071938..3d36465 100644 > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > @@ -1069,6 +1069,65 @@ i915_emit_box(struct intel_engine_cs *ring, > return 0; > } > > +static struct drm_i915_gem_object* > +i915_gem_execbuffer_parse(struct intel_engine_cs *ring, > + struct drm_i915_gem_exec_object2 > *shadow_exec_entry, > + struct eb_vmas *eb, > + struct drm_i915_gem_object *batch_obj, > + u32 batch_start_offset, > + u32 batch_len, > + bool is_master, > + u32 *flags) > +{ > + struct drm_i915_private *dev_priv = to_i915(batch_obj->base.dev); > + struct drm_i915_gem_object *shadow_batch_obj; > + int ret; > + > + shadow_batch_obj = i915_gem_batch_pool_get(&dev_priv- > >mm.batch_pool, > + batch_obj->base.size); > + if (IS_ERR(shadow_batch_obj)) > + return shadow_batch_obj; > + > + ret = i915_parse_cmds(ring, > + batch_obj, > + shadow_batch_obj, > + batch_start_offset, > + batch_len, > + is_master); > + if (ret) { > + if (ret == -EACCES) > + return batch_obj; Shouldn't this be returning an error code ? > + } else { > + struct i915_vma *vma; > + > + memset(shadow_exec_entry, 0, > sizeof(*shadow_exec_entry)); > + > + vma = i915_gem_obj_to_ggtt(shadow_batch_obj); > + vma->exec_entry = shadow_exec_entry; > + vma->exec_entry->flags = __EXEC_OBJECT_PURGEABLE; > + drm_gem_object_reference(&shadow_batch_obj->base); > + list_add_tail(&vma->exec_list, &eb->vmas); > + > + shadow_batch_obj->base.pending_read_domains = > + batch_obj->base.pending_read_domains; > + > + /* > + * Set the DISPATCH_SECURE bit to remove the > NON_SECURE > + * bit from MI_BATCH_BUFFER_START commands issued in > the > + * dispatch_execbuffer implementations. We specifically > + * don't want that set when the command parser is > + * enabled. > + * > + * FIXME: with aliasing ppgtt, buffers that should only > + * be in ggtt still end up in the aliasing ppgtt. remove > + * this check when that is fixed. > + */ > + if (USES_FULL_PPGTT(dev)) > + *flags |= I915_DISPATCH_SECURE; > + } > + > + return ret ? ERR_PTR(ret) : shadow_batch_obj; } > > int > i915_gem_ringbuffer_submission(struct drm_device *dev, struct drm_file > *file, @@ -1286,7 +1345,6 @@ i915_gem_do_execbuffer(struct drm_device > *dev, void *data, > struct drm_i915_private *dev_priv = dev->dev_private; > struct eb_vmas *eb; > struct drm_i915_gem_object *batch_obj; > - struct drm_i915_gem_object *shadow_batch_obj = NULL; > struct drm_i915_gem_exec_object2 shadow_exec_entry; > struct intel_engine_cs *ring; > struct intel_context *ctx; > @@ -1406,63 +1464,17 @@ i915_gem_do_execbuffer(struct drm_device > *dev, void *data, > } > > if (i915_needs_cmd_parser(ring)) { > - shadow_batch_obj = > - i915_gem_batch_pool_get(&dev_priv- > >mm.batch_pool, > - batch_obj->base.size); > - if (IS_ERR(shadow_batch_obj)) { > - ret = PTR_ERR(shadow_batch_obj); > - /* Don't try to clean up the obj in the error path */ > - shadow_batch_obj = NULL; > - goto err; > - } > - > - shadow_batch_obj->madv = I915_MADV_WILLNEED; > - > - ret = i915_gem_obj_ggtt_pin(shadow_batch_obj, 4096, 0); > - if (ret) > + batch_obj = i915_gem_execbuffer_parse(ring, > + &shadow_exec_entry, > + eb, > + batch_obj, > + args->batch_start_offset, > + args->batch_len, > + file->is_master, > + &flags); > + if (IS_ERR(batch_obj)) { > + ret = PTR_ERR(batch_obj); > goto err; > - > - ret = i915_parse_cmds(ring, > - batch_obj, > - shadow_batch_obj, > - args->batch_start_offset, > - args->batch_len, > - file->is_master); > - i915_gem_object_ggtt_unpin(shadow_batch_obj); > - > - if (ret) { > - if (ret != -EACCES) > - goto err; > - } else { > - struct i915_vma *vma; > - > - memset(&shadow_exec_entry, 0, > - sizeof(shadow_exec_entry)); > - > - vma = i915_gem_obj_to_ggtt(shadow_batch_obj); > - vma->exec_entry = &shadow_exec_entry; > - vma->exec_entry->flags = > __EXEC_OBJECT_PURGEABLE; > - drm_gem_object_reference(&shadow_batch_obj- > >base); > - list_add_tail(&vma->exec_list, &eb->vmas); > - > - shadow_batch_obj->base.pending_read_domains = > - batch_obj->base.pending_read_domains; > - > - batch_obj = shadow_batch_obj; > - > - /* > - * Set the DISPATCH_SECURE bit to remove the > NON_SECURE > - * bit from MI_BATCH_BUFFER_START commands > issued in the > - * dispatch_execbuffer implementations. We > specifically > - * don't want that set when the command parser is > - * enabled. > - * > - * FIXME: with aliasing ppgtt, buffers that should only > - * be in ggtt still end up in the aliasing ppgtt. remove > - * this check when that is fixed. > - */ > - if (USES_FULL_PPGTT(dev)) > - flags |= I915_DISPATCH_SECURE; > } > } > > -- > 1.9.1
On 12/11/2014 05:49 AM, Bloomfield, Jon wrote: > > >> -----Original Message----- >> From: Nguyen, Michael H >> Sent: Monday, December 08, 2014 10:34 PM >> To: intel-gfx@lists.freedesktop.org >> Cc: Bloomfield, Jon; Brad Volkin >> Subject: [PATCH v6 5/5] drm/i915: Tidy up execbuffer command parsing code >> >> From: Brad Volkin <bradley.d.volkin@intel.com> >> >> Move it to a separate function since the main do_execbuffer function >> already has so much going on. >> >> v2: >> - Move pin/unpin calls inside i915_parse_cmds() (Chris W, v4 7/7 >> feedback) >> >> Issue: VIZ-4719 >> Signed-off-by: Brad Volkin <bradley.d.volkin@intel.com> >> >> Conflicts: >> drivers/gpu/drm/i915/i915_gem_execbuffer.c >> --- >> drivers/gpu/drm/i915/i915_cmd_parser.c | 8 ++ >> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 126 ++++++++++++++++--- >> ---------- >> 2 files changed, 77 insertions(+), 57 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c >> b/drivers/gpu/drm/i915/i915_cmd_parser.c >> index 118079d..80b1b28 100644 >> --- a/drivers/gpu/drm/i915/i915_cmd_parser.c >> +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c >> @@ -1042,10 +1042,17 @@ int i915_parse_cmds(struct intel_engine_cs >> *ring, >> struct drm_i915_cmd_descriptor default_desc = { 0 }; >> bool oacontrol_set = false; /* OACONTROL tracking. See >> check_cmd() */ >> >> + ret = i915_gem_obj_ggtt_pin(shadow_batch_obj, 4096, 0); >> + if (ret) { >> + DRM_DEBUG_DRIVER("CMD: Failed to pin shadow batch\n"); >> + return -1; >> + } >> + >> batch_base = copy_batch(shadow_batch_obj, batch_obj, >> batch_start_offset, batch_len); >> if (IS_ERR(batch_base)) { >> DRM_DEBUG_DRIVER("CMD: Failed to copy batch\n"); >> + i915_gem_object_ggtt_unpin(shadow_batch_obj); >> return PTR_ERR(batch_base); >> } >> >> @@ -1116,6 +1123,7 @@ int i915_parse_cmds(struct intel_engine_cs *ring, >> } >> >> vunmap(batch_base); >> + i915_gem_object_ggtt_unpin(shadow_batch_obj); >> >> return ret; >> } >> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c >> b/drivers/gpu/drm/i915/i915_gem_execbuffer.c >> index 2071938..3d36465 100644 >> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c >> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c >> @@ -1069,6 +1069,65 @@ i915_emit_box(struct intel_engine_cs *ring, >> return 0; >> } >> >> +static struct drm_i915_gem_object* >> +i915_gem_execbuffer_parse(struct intel_engine_cs *ring, >> + struct drm_i915_gem_exec_object2 >> *shadow_exec_entry, >> + struct eb_vmas *eb, >> + struct drm_i915_gem_object *batch_obj, >> + u32 batch_start_offset, >> + u32 batch_len, >> + bool is_master, >> + u32 *flags) >> +{ >> + struct drm_i915_private *dev_priv = to_i915(batch_obj->base.dev); >> + struct drm_i915_gem_object *shadow_batch_obj; >> + int ret; >> + >> + shadow_batch_obj = i915_gem_batch_pool_get(&dev_priv- >>> mm.batch_pool, >> + batch_obj->base.size); >> + if (IS_ERR(shadow_batch_obj)) >> + return shadow_batch_obj; >> + >> + ret = i915_parse_cmds(ring, >> + batch_obj, >> + shadow_batch_obj, >> + batch_start_offset, >> + batch_len, >> + is_master); >> + if (ret) { >> + if (ret == -EACCES) >> + return batch_obj; > > Shouldn't this be returning an error code ? Good question. -EACCESS tells the caller of i915_parse_cmds() of a special case. There are some comments in its fnc header and implementation. -EACCESS indicates that batch_obj contains a chained batch in which case we dispatch batch_obj as non-secure. We return batch_obj here b/c the logic below doesn't apply. I believe the reason we safely dispatch chained batches as non-secure is b/c Brad and others probably determined that there weren't any valid use cases where chained batches needed to be dispatched as secure. Leaving them non-secure, HW will NOP bad cmds and its not necessary for i915 to do parsing. Thanks, Mike > > > >> + } else { >> + struct i915_vma *vma; >> + >> + memset(shadow_exec_entry, 0, >> sizeof(*shadow_exec_entry)); >> + >> + vma = i915_gem_obj_to_ggtt(shadow_batch_obj); >> + vma->exec_entry = shadow_exec_entry; >> + vma->exec_entry->flags = __EXEC_OBJECT_PURGEABLE; >> + drm_gem_object_reference(&shadow_batch_obj->base); >> + list_add_tail(&vma->exec_list, &eb->vmas); >> + >> + shadow_batch_obj->base.pending_read_domains = >> + batch_obj->base.pending_read_domains; >> + >> + /* >> + * Set the DISPATCH_SECURE bit to remove the >> NON_SECURE >> + * bit from MI_BATCH_BUFFER_START commands issued in >> the >> + * dispatch_execbuffer implementations. We specifically >> + * don't want that set when the command parser is >> + * enabled. >> + * >> + * FIXME: with aliasing ppgtt, buffers that should only >> + * be in ggtt still end up in the aliasing ppgtt. remove >> + * this check when that is fixed. >> + */ >> + if (USES_FULL_PPGTT(dev)) >> + *flags |= I915_DISPATCH_SECURE; >> + } >> + >> + return ret ? ERR_PTR(ret) : shadow_batch_obj; } >> >> int >> i915_gem_ringbuffer_submission(struct drm_device *dev, struct drm_file >> *file, @@ -1286,7 +1345,6 @@ i915_gem_do_execbuffer(struct drm_device >> *dev, void *data, >> struct drm_i915_private *dev_priv = dev->dev_private; >> struct eb_vmas *eb; >> struct drm_i915_gem_object *batch_obj; >> - struct drm_i915_gem_object *shadow_batch_obj = NULL; >> struct drm_i915_gem_exec_object2 shadow_exec_entry; >> struct intel_engine_cs *ring; >> struct intel_context *ctx; >> @@ -1406,63 +1464,17 @@ i915_gem_do_execbuffer(struct drm_device >> *dev, void *data, >> } >> >> if (i915_needs_cmd_parser(ring)) { >> - shadow_batch_obj = >> - i915_gem_batch_pool_get(&dev_priv- >>> mm.batch_pool, >> - batch_obj->base.size); >> - if (IS_ERR(shadow_batch_obj)) { >> - ret = PTR_ERR(shadow_batch_obj); >> - /* Don't try to clean up the obj in the error path */ >> - shadow_batch_obj = NULL; >> - goto err; >> - } >> - >> - shadow_batch_obj->madv = I915_MADV_WILLNEED; >> - >> - ret = i915_gem_obj_ggtt_pin(shadow_batch_obj, 4096, 0); >> - if (ret) >> + batch_obj = i915_gem_execbuffer_parse(ring, >> + &shadow_exec_entry, >> + eb, >> + batch_obj, >> + args->batch_start_offset, >> + args->batch_len, >> + file->is_master, >> + &flags); >> + if (IS_ERR(batch_obj)) { >> + ret = PTR_ERR(batch_obj); >> goto err; >> - >> - ret = i915_parse_cmds(ring, >> - batch_obj, >> - shadow_batch_obj, >> - args->batch_start_offset, >> - args->batch_len, >> - file->is_master); >> - i915_gem_object_ggtt_unpin(shadow_batch_obj); >> - >> - if (ret) { >> - if (ret != -EACCES) >> - goto err; >> - } else { >> - struct i915_vma *vma; >> - >> - memset(&shadow_exec_entry, 0, >> - sizeof(shadow_exec_entry)); >> - >> - vma = i915_gem_obj_to_ggtt(shadow_batch_obj); >> - vma->exec_entry = &shadow_exec_entry; >> - vma->exec_entry->flags = >> __EXEC_OBJECT_PURGEABLE; >> - drm_gem_object_reference(&shadow_batch_obj- >>> base); >> - list_add_tail(&vma->exec_list, &eb->vmas); >> - >> - shadow_batch_obj->base.pending_read_domains = >> - batch_obj->base.pending_read_domains; >> - >> - batch_obj = shadow_batch_obj; >> - >> - /* >> - * Set the DISPATCH_SECURE bit to remove the >> NON_SECURE >> - * bit from MI_BATCH_BUFFER_START commands >> issued in the >> - * dispatch_execbuffer implementations. We >> specifically >> - * don't want that set when the command parser is >> - * enabled. >> - * >> - * FIXME: with aliasing ppgtt, buffers that should only >> - * be in ggtt still end up in the aliasing ppgtt. remove >> - * this check when that is fixed. >> - */ >> - if (USES_FULL_PPGTT(dev)) >> - flags |= I915_DISPATCH_SECURE; >> } >> } >> >> -- >> 1.9.1 >
diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c index 118079d..80b1b28 100644 --- a/drivers/gpu/drm/i915/i915_cmd_parser.c +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c @@ -1042,10 +1042,17 @@ int i915_parse_cmds(struct intel_engine_cs *ring, struct drm_i915_cmd_descriptor default_desc = { 0 }; bool oacontrol_set = false; /* OACONTROL tracking. See check_cmd() */ + ret = i915_gem_obj_ggtt_pin(shadow_batch_obj, 4096, 0); + if (ret) { + DRM_DEBUG_DRIVER("CMD: Failed to pin shadow batch\n"); + return -1; + } + batch_base = copy_batch(shadow_batch_obj, batch_obj, batch_start_offset, batch_len); if (IS_ERR(batch_base)) { DRM_DEBUG_DRIVER("CMD: Failed to copy batch\n"); + i915_gem_object_ggtt_unpin(shadow_batch_obj); return PTR_ERR(batch_base); } @@ -1116,6 +1123,7 @@ int i915_parse_cmds(struct intel_engine_cs *ring, } vunmap(batch_base); + i915_gem_object_ggtt_unpin(shadow_batch_obj); return ret; } diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index 2071938..3d36465 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -1069,6 +1069,65 @@ i915_emit_box(struct intel_engine_cs *ring, return 0; } +static struct drm_i915_gem_object* +i915_gem_execbuffer_parse(struct intel_engine_cs *ring, + struct drm_i915_gem_exec_object2 *shadow_exec_entry, + struct eb_vmas *eb, + struct drm_i915_gem_object *batch_obj, + u32 batch_start_offset, + u32 batch_len, + bool is_master, + u32 *flags) +{ + struct drm_i915_private *dev_priv = to_i915(batch_obj->base.dev); + struct drm_i915_gem_object *shadow_batch_obj; + int ret; + + shadow_batch_obj = i915_gem_batch_pool_get(&dev_priv->mm.batch_pool, + batch_obj->base.size); + if (IS_ERR(shadow_batch_obj)) + return shadow_batch_obj; + + ret = i915_parse_cmds(ring, + batch_obj, + shadow_batch_obj, + batch_start_offset, + batch_len, + is_master); + if (ret) { + if (ret == -EACCES) + return batch_obj; + } else { + struct i915_vma *vma; + + memset(shadow_exec_entry, 0, sizeof(*shadow_exec_entry)); + + vma = i915_gem_obj_to_ggtt(shadow_batch_obj); + vma->exec_entry = shadow_exec_entry; + vma->exec_entry->flags = __EXEC_OBJECT_PURGEABLE; + drm_gem_object_reference(&shadow_batch_obj->base); + list_add_tail(&vma->exec_list, &eb->vmas); + + shadow_batch_obj->base.pending_read_domains = + batch_obj->base.pending_read_domains; + + /* + * Set the DISPATCH_SECURE bit to remove the NON_SECURE + * bit from MI_BATCH_BUFFER_START commands issued in the + * dispatch_execbuffer implementations. We specifically + * don't want that set when the command parser is + * enabled. + * + * FIXME: with aliasing ppgtt, buffers that should only + * be in ggtt still end up in the aliasing ppgtt. remove + * this check when that is fixed. + */ + if (USES_FULL_PPGTT(dev)) + *flags |= I915_DISPATCH_SECURE; + } + + return ret ? ERR_PTR(ret) : shadow_batch_obj; +} int i915_gem_ringbuffer_submission(struct drm_device *dev, struct drm_file *file, @@ -1286,7 +1345,6 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, struct drm_i915_private *dev_priv = dev->dev_private; struct eb_vmas *eb; struct drm_i915_gem_object *batch_obj; - struct drm_i915_gem_object *shadow_batch_obj = NULL; struct drm_i915_gem_exec_object2 shadow_exec_entry; struct intel_engine_cs *ring; struct intel_context *ctx; @@ -1406,63 +1464,17 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, } if (i915_needs_cmd_parser(ring)) { - shadow_batch_obj = - i915_gem_batch_pool_get(&dev_priv->mm.batch_pool, - batch_obj->base.size); - if (IS_ERR(shadow_batch_obj)) { - ret = PTR_ERR(shadow_batch_obj); - /* Don't try to clean up the obj in the error path */ - shadow_batch_obj = NULL; - goto err; - } - - shadow_batch_obj->madv = I915_MADV_WILLNEED; - - ret = i915_gem_obj_ggtt_pin(shadow_batch_obj, 4096, 0); - if (ret) + batch_obj = i915_gem_execbuffer_parse(ring, + &shadow_exec_entry, + eb, + batch_obj, + args->batch_start_offset, + args->batch_len, + file->is_master, + &flags); + if (IS_ERR(batch_obj)) { + ret = PTR_ERR(batch_obj); goto err; - - ret = i915_parse_cmds(ring, - batch_obj, - shadow_batch_obj, - args->batch_start_offset, - args->batch_len, - file->is_master); - i915_gem_object_ggtt_unpin(shadow_batch_obj); - - if (ret) { - if (ret != -EACCES) - goto err; - } else { - struct i915_vma *vma; - - memset(&shadow_exec_entry, 0, - sizeof(shadow_exec_entry)); - - vma = i915_gem_obj_to_ggtt(shadow_batch_obj); - vma->exec_entry = &shadow_exec_entry; - vma->exec_entry->flags = __EXEC_OBJECT_PURGEABLE; - drm_gem_object_reference(&shadow_batch_obj->base); - list_add_tail(&vma->exec_list, &eb->vmas); - - shadow_batch_obj->base.pending_read_domains = - batch_obj->base.pending_read_domains; - - batch_obj = shadow_batch_obj; - - /* - * Set the DISPATCH_SECURE bit to remove the NON_SECURE - * bit from MI_BATCH_BUFFER_START commands issued in the - * dispatch_execbuffer implementations. We specifically - * don't want that set when the command parser is - * enabled. - * - * FIXME: with aliasing ppgtt, buffers that should only - * be in ggtt still end up in the aliasing ppgtt. remove - * this check when that is fixed. - */ - if (USES_FULL_PPGTT(dev)) - flags |= I915_DISPATCH_SECURE; } }