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. > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
