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

[Daniel Vetter]

On Tue, Jun 16, 2015 at 05:07:46PM +0100, Chris Wilson wrote:
> On Tue, Jun 16, 2015 at 04:10:19PM +0100, Tvrtko Ursulin wrote:
> > 
> > 
> > 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.
> 
> Make it a unsigned long cookie then! :)
> 
> I thought Maarten was thinking of doing callbacks for duplicating state
> which we could use to keep the pin_count accurate etc.

plane_state holds a reference on the fb, which means that won't go poof
before the plane state disappears. And the fb makes sure the obj doesn't
go poof. Which means we just need pin_count to make sure the vma doesn't
disapper.

atomic states have very clear ownership rules and an objects tangling off
them are properly refcounted. I don't see any issues here at all.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch