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
