| Message ID | 1432720354-20230-3-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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);