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

[Chris Wilson]

On Fri, Jul 15, 2016 at 09:03:40AM +0100, Dave Gordon wrote:
> 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 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.
> 
> ?
> 
> 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!

Then review the patches that have already been posted several times on
the list to do everything above.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre