Re: [PATCH] drm/i915: Track pinned vma in intel_plane_state

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Jan 04, 2017 at 05:47:50PM +0200, Ville Syrjälä wrote:
> On Wed, Jan 04, 2017 at 03:14:45PM +0000, Chris Wilson wrote:
> > On Wed, Jan 04, 2017 at 05:06:46PM +0200, Ville Syrjälä wrote:
> > > On Wed, Jan 04, 2017 at 02:31:26PM +0100, Maarten Lankhorst wrote:
> > > > From: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > > > 
> > > > With atomic plane states we are able to track an allocation right from
> > > > preparation, during use and through to the final free after being
> > > > swapped out for a new plane. We can couple the VMA we pin for the
> > > > framebuffer (and its rotation) to this lifetime and avoid all the clumsy
> > > > lookups in between.
> > > > 
> > > > v2: Remove residual vma on plane cleanup (Chris)
> > > > v3: Add a description for the vma destruction in
> > > >     intel_plane_destroy_state (Maarten)
> > > > 
> > > > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_drv.h           |  16 +---
> > > >  drivers/gpu/drm/i915/intel_atomic_plane.c |  20 +++++
> > > >  drivers/gpu/drm/i915/intel_display.c      | 129 +++++++++++-------------------
> > > >  drivers/gpu/drm/i915/intel_drv.h          |   9 ++-
> > > >  drivers/gpu/drm/i915/intel_fbc.c          |  52 +++++-------
> > > >  drivers/gpu/drm/i915/intel_fbdev.c        |   4 +-
> > > >  drivers/gpu/drm/i915/intel_sprite.c       |   8 +-
> > > >  7 files changed, 103 insertions(+), 135 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > > index 22d3f610212c..5369f5f9ce3a 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -1079,6 +1079,8 @@ struct intel_fbc {
> > > >  	struct work_struct underrun_work;
> > > >  
> > > >  	struct intel_fbc_state_cache {
> > > > +		struct i915_vma *vma;
> > > > +
> > > >  		struct {
> > > >  			unsigned int mode_flags;
> > > >  			uint32_t hsw_bdw_pixel_rate;
> > > > @@ -1092,15 +1094,14 @@ struct intel_fbc {
> > > >  		} plane;
> > > >  
> > > >  		struct {
> > > > -			u64 ilk_ggtt_offset;
> > > >  			const struct drm_format_info *format;
> > > >  			unsigned int stride;
> > > > -			int fence_reg;
> > > > -			unsigned int tiling_mode;
> > > >  		} fb;
> > > >  	} state_cache;
> > > >  
> > > >  	struct intel_fbc_reg_params {
> > > > +		struct i915_vma *vma;
> > > > +
> > > >  		struct {
> > > >  			enum pipe pipe;
> > > >  			enum plane plane;
> > > > @@ -1108,10 +1109,8 @@ struct intel_fbc {
> > > >  		} crtc;
> > > >  
> > > >  		struct {
> > > > -			u64 ggtt_offset;
> > > >  			const struct drm_format_info *format;
> > > >  			unsigned int stride;
> > > > -			int fence_reg;
> > > >  		} fb;
> > > >  
> > > >  		int cfb_size;
> > > > @@ -3406,13 +3405,6 @@ i915_gem_object_to_ggtt(struct drm_i915_gem_object *obj,
> > > >  	return i915_gem_obj_to_vma(obj, &to_i915(obj->base.dev)->ggtt.base, view);
> > > >  }
> > > >  
> > > > -static inline unsigned long
> > > > -i915_gem_object_ggtt_offset(struct drm_i915_gem_object *o,
> > > > -			    const struct i915_ggtt_view *view)
> > > > -{
> > > > -	return i915_ggtt_offset(i915_gem_object_to_ggtt(o, view));
> > > > -}
> > > > -
> > > >  /* i915_gem_fence_reg.c */
> > > >  int __must_check i915_vma_get_fence(struct i915_vma *vma);
> > > >  int __must_check i915_vma_put_fence(struct i915_vma *vma);
> > > > diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > > > index 4612ffd555a7..41fd94e62d3c 100644
> > > > --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> > > > +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > > > @@ -85,6 +85,8 @@ intel_plane_duplicate_state(struct drm_plane *plane)
> > > >  
> > > >  	__drm_atomic_helper_plane_duplicate_state(plane, state);
> > > >  
> > > > +	intel_state->vma = NULL;
> > > 
> > > Shouldn't we be doing vma_get() instead?
> > 
> > I went with NULL (dropping the copy) for simplicity. Before the plane
> > can be used it must be prepared, so vma will always be set before
> > commiting, and using NULL avoided having the reference counting dance.
> > It also allowed detection of when we didn't prepare the plane as
> > required.
> 
> Hmm. Does that risk some kind of failure at prepare_fb time if the vma
> got nuked in the meantime? I guess that might ahve to involve
> suspend/resume or some other case where we duplicate the state and
> don't hang on to the old state.

If we are restoring the same plane_state, the old_plane_state will not
be unpinned until after the swap. So prepare_fb will return the
duplicate VMA with incremented pin_count.

So long as prepare_fb is always called, I don't think there is a hole.
So long as.
-Chris

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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux