[Intel-gfx] [PATCH] drm/i915: Track pinned vma in intel_plane_state

[Ville Syrjälä]

On Wed, Jan 04, 2017 at 04:00:49PM +0000, Chris Wilson wrote:
> 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 at chris-wilson.co.uk>
> > > > > 
> > > > > 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 at chris-wilson.co.uk>
> > > > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > > > > ---
> > > > >  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.

During suspend we throw away the old state, and resume will read in
the state from the hardware. So when we do the swap we won't have the
original old state in place anymore.

-- 
Ville Syrjälä
Intel OTC