Re: [PATCH 2/2] drm/i915: refactor eb_get_batch()

[Dave Gordon <david.s.gordon@intel.com>]

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@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.

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