On 13/07/16 13:44, Chris Wilson wrote:
On Wed, Jul 13, 2016 at 02:38:16PM +0200, Daniel Vetter wrote:
On Thu, Jun 30, 2016 at 04:12:49PM +0100, Dave Gordon wrote:
Precursor for fix to secure batch execution. We will need to be able to
retrieve the batch VMA (as well as the batch itself) from the eb list,
so this patch extracts that part of eb_get_batch() into a separate
function, and moves both parts to a more logical place in the file, near
where the eb list is created.
Also, it may not be obvious, but the current execbuffer2 ioctl interface
requires that the buffer object containing the batch-to-be-executed be
the LAST entry in the exec2_list[] array (I expected it to be the first!).
To clarify this, we can replace the rather obscure construct
"list_entry(eb->vmas.prev, ...)"
in the old version of eb_get_batch() with the equivalent but more explicit
"list_last_entry(&eb->vmas,...)"
in the new eb_get_batch_vma() and of course add an explanatory comment.
Signed-off-by: Dave Gordon <david.s.gordon@xxxxxxxxx>
I have no context on the secure batch fix you're talking about, but this
here makes sense as an independent cleanup.
It won't help though, so this is just churn for no purpose.
-Chris
At the very least, it replaces a confusing construct with
a comprehensible one annotated with an explanatory comment.
Separating finding the VMA for the batch from finding the batch itself
also improves clarity and costs nothing (compiler inlines it anyway).
Comprehensibility -- and hence maintainability -- is always
a worthwhile purpose :)
BTW, do the comments in this code from patch
d23db88 drm/i915: Prevent negative relocation deltas from wrapping
still apply? 'Cos I think it's pretty ugly to be setting a flag
on a VMA as a side-effect of a "lookup" type operation :( Surely
cleaner to do that sort of think at the top level i.e. inside
i915_gem_do_execbuffer() ?
.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
