Re: [PATCH v4 7/7] drm/i915: Tidy up execbuffer command parsing code

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Nov 21, 2014 at 05:17:50PM -0800, Michael H. Nguyen wrote:
> Hi Chris,
> 
> >>
> >>+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;
> >>+
> >>+	shadow_batch_obj->madv = I915_MADV_WILLNEED;
> >>+
> >>+	ret = i915_gem_obj_ggtt_pin(shadow_batch_obj, 4096, 0);
> >>+	if (ret)
> >>+		goto err;
> >
> >Pardon? This feels an implementation issue of i915_parse_cmds() and should
> >be resolved there. Presumably you are not actually reading back through
> >the GTT? That would be insane...
> >
> >>+	ret = i915_parse_cmds(ring,
> >>+			      batch_obj,
> >>+			      shadow_batch_obj,
> >>+			      batch_start_offset,
> >>+			      batch_len,
> >>+			      is_master);
> >>+	i915_gem_object_ggtt_unpin(shadow_batch_obj);
> >
> >Yet pin+unpin around the parser seems to serve no other purpose.
> Are you suggesting to remove the pin/unpin calls? If so, isn't pinning
> needed to ensure the backing store pages are available in vmap_batch()? i.e.
> obj->pages->sgl is populated w/ physical pages.
> 
> Or, are you suggesting to move the pin/unpin calls inside i915_parse_cmds()
> ?

Yeah please push them down into the cmd parser around the part that
copies/checks the shadow batch. If we ever change the way we do that
copy/parsing (likely due to performance issues on vlv) then we also might
need to change the type of pinning. So better to keep things together.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx