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

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

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

.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx