[Intel-gfx] [PATCH 2/2] drm/i915: refactor eb_get_batch()

[Chris Wilson]

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 at intel.com>
> >>
> >>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.
 
> 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.
 
> Comprehensibility -- and hence maintainability -- is always
> a worthwhile purpose :)

s/comprehensibility/greater confusion/

> 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

-- 
Chris Wilson, Intel Open Source Technology Centre