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

[Chris Wilson]

On Tue, Jun 16, 2015 at 12:31:23PM +0100, Tvrtko Ursulin wrote:
> 
> On 06/16/2015 12:22 PM, Chris Wilson wrote:
> >On Tue, Jun 16, 2015 at 12:18:49PM +0100, Tvrtko Ursulin wrote:
> >>
> >>On 06/16/2015 12:02 PM, Chris Wilson wrote:
> >>>On Tue, Jun 16, 2015 at 11:17:11AM +0100, Tvrtko Ursulin wrote:
> >>>>
> >>>>On 05/28/2015 03:09 PM, Daniel Vetter wrote:
> >>>>>On Thu, May 28, 2015 at 01:36:54PM +0100, Chris Wilson wrote:
> >>>>>>On Thu, May 28, 2015 at 02:24:40PM +0200, Daniel Vetter wrote:
> >>>>>>>On Thu, May 28, 2015 at 09:58:30AM +0100, Tvrtko Ursulin wrote:
> >>>>>>>>
> >>>>>>>>On 05/27/2015 10:15 PM, Chris Wilson wrote:
> >>>>>>>>>On Wed, May 27, 2015 at 10:52:34AM +0100, Tvrtko Ursulin wrote:
> >>>>>>>>>>From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >>>>>>>>>>
> >>>>>>>>>>These are the display call sites so should use the proper helper.
> >>>>>>>>>>
> >>>>>>>>>>Also requires intel_plane_obj_offset to assume normal view when
> >>>>>>>>>>plane pointer is not available.
> >>>>>>>>>
> >>>>>>>>>Eugh. If only the plane stored the offset, bonus marks for storing the
> >>>>>>>>>vma cookie, then we would not have to keep recomputing the view and
> >>>>>>>>>searching every single time...
> >>>>>>>>
> >>>>>>>>Well, the patch even decreases the number of searches! :)
> >>>>>>>>
> >>>>>>>>And we don't recompute when querying the offset - it just figures out what
> >>>>>>>>type of vma it should look for. So I think it doesn't prevent any future
> >>>>>>>>caching improvements. It actually makes it easier since it consolidates the
> >>>>>>>>query.
> >>>>>>>>
> >>>>>>>>I'll need this, or something like it, for some future work. So at the very
> >>>>>>>>moment I am not too bothered if this goes in or not.
> >>>>>>>
> >>>>>>>Yeah unfortunately we can't eliminate the view searches completely since
> >>>>>>>the view depends upon fb + plane_state. So not perfectly aligned with our
> >>>>>>>hw ... Otherwise I'd agree, caching the view in the fb would be neat.
> >>>>>>
> >>>>>>Not in the fb, in the plane_state. We acquire the vma for the plane
> >>>>>>during prepare and then we should be using that vma reference for the
> >>>>>>lifetime of that atomic plane state.
> >>>>>
> >>>>>Hm right that should work. And the pin will make sure it won't go poof
> >>>>>prematurely.
> >>>>
> >>>>Maybe I could do this, but at the moment I have no idea how that
> >>>>would work from intelfb_alloc who calls pin_and_fence with no state.
> >>>
> >>>fbdev is a little special. All that we actually require is a plain
> >>>ggtt_pin to ensure that the fb is still around for panics. We can leave
> >>>the plane state in modesetting. We have have full control over the
> >>>struct we associated with the ifbdev, so can easily stash the vma in
> >>>there as well.
> >>
> >>What do you mean by "We can leave the plane state in modesetting." ?
> >>
> >>Where is the connection between ifbdev and plane state ie. our modeset?
> >
> >There is none. I thought you were arguing that the use of
> >pin_and_fence_fb in fbdev was problematic and that you wanted to use its
> >offset for the subsequent modesetting.
> 
> 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.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre