On 14/07/16 15:03, Chris Wilson wrote:
On Thu, Jul 14, 2016 at 02:12:55PM +0100, Dave Gordon wrote:
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.
No. It deepens a confusion in the code that I've been trying to get
removed over the last couple of years.
?
I was referring to the list_{last_}entry() change. That's definitely a
clarification as to how things work now. Of course, if you're planning
to make the batch the first object rather than the last, I won't object.
But whichever it is, let's use the most-appropriately-named of the
available list functions when we pick an item from a list. And comment
why or what it's doing.
Separating finding the VMA for the batch from finding the batch itself
also improves clarity and costs nothing (compiler inlines it anyway).
No. That's the confusion you have here. The object is irrelevant.
Ah, so we have a function to return an irrelevant object. Let's just
delete it then ;)
Do you think we /should/ just get rid of eb_get_batch()? Maybe just
have eb_get_batch_vma() return the VMA to the [single] caller
i915_gem_do_execbuffer() instead, and then have /that/ do both
the flag-setting ugliness and the indirection to the object (which
evidently is not irrelevant to it) ?
Comprehensibility -- and hence maintainability -- is always
a worthwhile purpose :)
s/comprehensibility/greater confusion/
Spoken like a true Discordian ;)
> 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() ?
The comment is wrong since the practice is more widespread and it is
a particular hw bug on Ivybridge.
-Chris
Another reason to move it out to the caller and update the comments in
the process!
.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
