[Intel-gfx] [PATCH 3/3] drm/i915: Use intel_plane_obj_offset from more places

[Tvrtko Ursulin]

On 06/16/2015 02:53 PM, Chris Wilson wrote:
> On Tue, Jun 16, 2015 at 02:32:40PM +0100, Tvrtko Ursulin wrote:
>>
>> On 06/16/2015 12:48 PM, Chris Wilson wrote:
>>> On Tue, Jun 16, 2015 at 12:31:23PM +0100, Tvrtko Ursulin wrote:
>>>> That is partially correct, I do see it as problematic since I
>>>> assumed someone will modeset with this fb/object at some point, and
>>>> there will be state available then, which won't have the cached
>>>> display address at all since the state is not present during fbdev
>>>> setup.
>>>>
>>>> Does that never happens? I mean, the modeset with state using the
>>>> fb/object prepared in intefb_alloc?
>>>
>>> No. The setup in intelfb_alloc is only concerned with generating a GGTT
>>> mmapping that is consistent with later use by modesetting. The important
>>> detail is to make sure the alignment is correct (or else the modeset
>>> will fail as it cannot move the object as it is already pinned).
>>>
>>> As Ville has extracted the linear alignment, we can export that and all
>>> pin_to_display directly so that we can set up the fbdev without the
>>> confusion of calling intel_pin_and_fence_fb. Or we can just live with
>>> the confustion and comment appropriately.
>>
>> Ok, think I get it now. Will send three RFC patches shortly.
>>
>> 1/3 looks innocent but it actually a bugfix once display address
>> caching come along.
>>
>> 2/3 is the caching itself.
>>
>> 3/3 is what is not yet needed today, but analogous to 1/3 it fixes a
>> bug which will become apparent in the future.
>>
>> If this looks more along the lines of what you had in mind I can
>> polish the comments or whatnot. 80 char line breaks were especially
>> ugly in some of them to very long variable names. :)
>
> Not far enough. It's not actually about caching the display address,
> it's about tracking the actual VMA reference allocated for the plane.
>
> What I had in mind and suggested yonks ago is:
>
> http://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=nightly&id=2112f72b7adb94b0e038bbafe74a3c1ab6c851cf
>
> Ignore the atomic interface changes, they are from a bygone era. The
> important part is just in tracking vma and the
> simplification/robustification that provides.

Not bad, looks good to me on the high level. Especially the unpin path 
is much nicer with this approach. My only uncertainty is whether 
stuffing object pointers into the state is acceptable with the architect 
of those parts.

Regards,

Tvrtko