Re: [PATCH 3/3] drm/i915: Use intel_plane_obj_offset from more places

From: Chris Wilson <chris@chris-wilson.co.uk>
To: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 3/3] drm/i915: Use intel_plane_obj_offset from more places
Date: Tue, 16 Jun 2015 17:07:46 +0100	[thread overview]
Message-ID: <20150616160746.GH11933@nuc-i3427.alporthouse.com> (raw)
In-Reply-To: <55803C5B.5070005@linux.intel.com>

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx