[3/3] drm/i915: Use intel_plane_obj_offset from more places
diff mbox

Message ID 1432720354-20230-3-git-send-email-tvrtko.ursulin@linux.intel.com
State New
Headers show

Commit Message

Tvrtko Ursulin May 27, 2015, 9:52 a.m. UTC
From: Tvrtko Ursulin <tvrtko.ursulin@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.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 8 +++++---
 drivers/gpu/drm/i915/intel_fbdev.c   | 9 +++++----
 2 files changed, 10 insertions(+), 7 deletions(-)

Comments

Chris Wilson May 27, 2015, 9:15 p.m. UTC | #1
On Wed, May 27, 2015 at 10:52:34AM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@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...
-Chris
Tvrtko Ursulin May 28, 2015, 8:58 a.m. UTC | #2
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@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.

Regards,

Tvrtko
Daniel Vetter May 28, 2015, 12:24 p.m. UTC | #3
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@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.
-Daniel
Chris Wilson May 28, 2015, 12:36 p.m. UTC | #4
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@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.
-Chris
Daniel Vetter May 28, 2015, 2:09 p.m. UTC | #5
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@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.
-Daniel
Tvrtko Ursulin June 16, 2015, 10:17 a.m. UTC | #6
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@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.

I could keep intel_plan_obj_offsets at all call sites and either return 
cached (say plane_state->disp_addr) address or "compute" and cache it, 
but, how would I know cached address is valid or not on startup?

I assume plane state is all initialized to zeros? And zero as a display 
address is valid AFAIK?

Do we have a intel_plane_state "constructor" somewhere which could set 
"intel_plane_state->disp_addr = -1", and so later intel_plane_obj_offset 
would know what to do?

And whatever can be done can also be done on top of this patch, which 
actually fixes things from the current state.

Regards,

Tvrtko
Chris Wilson June 16, 2015, 11:02 a.m. UTC | #7
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@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.

> I could keep intel_plan_obj_offsets at all call sites and either
> return cached (say plane_state->disp_addr) address or "compute" and
> cache it, but, how would I know cached address is valid or not on
> startup?

They all disappear. Prepare gets the vma, then commit can just use
vma->node.start + offset (and when the plane_state is released, so is
the ggtt pinning for the fb).
-Chris
Chris Wilson June 16, 2015, 11:06 a.m. UTC | #8
On Tue, Jun 16, 2015 at 12:02:00PM +0100, Chris Wilson wrote:
> 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.

Or even from the atomic perspective, the plane state for fbdev can be
kept alive for emergency restoration. We still need to keep the ggtt_pin
for the persistent mmaping we use for fbcon.
-Chris
Tvrtko Ursulin June 16, 2015, 11:18 a.m. UTC | #9
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@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?

>> I could keep intel_plan_obj_offsets at all call sites and either
>> return cached (say plane_state->disp_addr) address or "compute" and
>> cache it, but, how would I know cached address is valid or not on
>> startup?
>
> They all disappear. Prepare gets the vma, then commit can just use
> vma->node.start + offset (and when the plane_state is released, so is
> the ggtt pinning for the fb).

Yeah they disappear once one figures out how to handle fbdev paths. I am 
not there yet. :)

Tvrtko
Chris Wilson June 16, 2015, 11:22 a.m. UTC | #10
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@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.
-Chris
Tvrtko Ursulin June 16, 2015, 11:31 a.m. UTC | #11
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@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?

Regards,

Tvrtko
Chris Wilson June 16, 2015, 11:48 a.m. UTC | #12
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@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
Tvrtko Ursulin June 16, 2015, 1:32 p.m. UTC | #13
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. :)

Regards,

Tvrtko
Chris Wilson June 16, 2015, 1:53 p.m. UTC | #14
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.
-Chris
Tvrtko Ursulin June 16, 2015, 3:10 p.m. UTC | #15
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
Chris Wilson June 16, 2015, 4:07 p.m. UTC | #16
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
Daniel Vetter June 17, 2015, 11:34 a.m. UTC | #17
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

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 657a333..0dc872b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2626,7 +2626,8 @@  intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
 			continue;
 
 		obj = intel_fb_obj(fb);
-		if (i915_gem_obj_ggtt_offset(obj) == plane_config->base) {
+		if (intel_plane_obj_offset(to_intel_plane(c->primary), obj) ==
+		    plane_config->base) {
 			drm_framebuffer_reference(fb);
 			goto valid_fb;
 		}
@@ -2911,7 +2912,8 @@  unsigned long intel_plane_obj_offset(struct intel_plane *intel_plane,
 {
 	const struct i915_ggtt_view *view = &i915_ggtt_view_normal;
 
-	if (intel_rotation_90_or_270(intel_plane->base.state->rotation))
+	if (intel_plane &&
+	    intel_rotation_90_or_270(intel_plane->base.state->rotation))
 		view = &i915_ggtt_view_rotated;
 
 	return i915_gem_obj_ggtt_offset_view(obj, view);
@@ -13661,7 +13663,7 @@  intel_commit_cursor_plane(struct drm_plane *plane,
 	if (!obj)
 		addr = 0;
 	else if (!INTEL_INFO(dev)->cursor_needs_physical)
-		addr = i915_gem_obj_ggtt_offset(obj);
+		addr = intel_plane_obj_offset(to_intel_plane(plane), obj);
 	else
 		addr = obj->phys_handle->busaddr;
 
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 4e7e7da..4338324 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -180,6 +180,7 @@  static int intelfb_create(struct drm_fb_helper *helper,
 	struct fb_info *info;
 	struct drm_framebuffer *fb;
 	struct drm_i915_gem_object *obj;
+	unsigned long disp_addr;
 	int size, ret;
 	bool prealloc = false;
 
@@ -243,12 +244,12 @@  static int intelfb_create(struct drm_fb_helper *helper,
 	info->apertures->ranges[0].base = dev->mode_config.fb_base;
 	info->apertures->ranges[0].size = dev_priv->gtt.mappable_end;
 
-	info->fix.smem_start = dev->mode_config.fb_base + i915_gem_obj_ggtt_offset(obj);
+	disp_addr = intel_plane_obj_offset(NULL, obj);
+	info->fix.smem_start = dev->mode_config.fb_base + disp_addr;
 	info->fix.smem_len = size;
 
 	info->screen_base =
-		ioremap_wc(dev_priv->gtt.mappable_base + i915_gem_obj_ggtt_offset(obj),
-			   size);
+		ioremap_wc(dev_priv->gtt.mappable_base + disp_addr, size);
 	if (!info->screen_base) {
 		ret = -ENOSPC;
 		goto out_unpin;
@@ -272,7 +273,7 @@  static int intelfb_create(struct drm_fb_helper *helper,
 
 	DRM_DEBUG_KMS("allocated %dx%d fb: 0x%08lx, bo %p\n",
 		      fb->width, fb->height,
-		      i915_gem_obj_ggtt_offset(obj), obj);
+		      disp_addr, obj);
 
 	mutex_unlock(&dev->struct_mutex);
 	vga_switcheroo_client_fb_set(dev->pdev, info);