On Tue, Nov 15, 2016 at 10:02:23AM +0000, Chris Wilson wrote:
> On Tue, Nov 15, 2016 at 10:52:00AM +0100, Daniel Vetter wrote:
> > On Tue, Nov 15, 2016 at 08:58:15AM +0000, Chris Wilson wrote:
> > > 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.
> > >
> > > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> >
> > A few questions and comments below. Of course review assumes some form of
> > patch 2 lands first (but leaning towards just reverting the core one for
> > that).
> > -Daniel
> >
> > > ---
> > > drivers/gpu/drm/i915/i915_drv.h | 16 ++---
> > > drivers/gpu/drm/i915/intel_atomic_plane.c | 2 +
> > > drivers/gpu/drm/i915/intel_display.c | 97 +++++++++++++------------------
> > > 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, 78 insertions(+), 110 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index 4e7148a3ee8b..2bc285e2ff82 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -989,6 +989,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;
> > > @@ -1002,15 +1004,14 @@ struct intel_fbc {
> > > } plane;
> > >
> > > struct {
> > > - u64 ilk_ggtt_offset;
> > > uint32_t pixel_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;
> > > @@ -1018,10 +1019,8 @@ struct intel_fbc {
> > > } crtc;
> > >
> > > struct {
> > > - u64 ggtt_offset;
> > > uint32_t pixel_format;
> > > unsigned int stride;
> > > - int fence_reg;
> > > } fb;
> > >
> > > int cfb_size;
> > > @@ -3150,13 +3149,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 89a34350a35d..5a86e0d52409 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;
> > > +
> > > return state;
> > > }
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index dca1e0b512e5..cb52116f8577 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -2241,24 +2241,19 @@ intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, unsigned int rotation)
> > > i915_vma_pin_fence(vma);
> > > }
> > >
> > > + i915_vma_get(vma);
> >
> > I'm not entirely clear on why we no need a refcount for this? Vestige code
> > from when you wanted to refcount plane_state->fb like other refcounted
> > state pointers? If we restrict the lifetime to just in between
> > prepare_plane/cleanup_plane like you do I don't think this is needed ...
>
> The lifetime is quite broad since we aquire it on takeover and it is not
> finally released until plane state destruction. It's not as simple as
> saying they only exist between two atomic commits :| Since the vma is
> being tracked outside of the parent fb (and it is independent since it
> is a view of that fb), it is safer to hold a reference.
Takeover = prepare_planes done by someone else. And assuming our current
refcounting is correct we shouldn't need this additional refcount either.
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index fff55d93ca73..d3d26d69930a 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -372,6 +372,7 @@ struct intel_atomic_state {
> > > struct intel_plane_state {
> > > struct drm_plane_state base;
> > > struct drm_rect clip;
> >
> > I think a comment here about the special refcounting rules for this would
> > be good, e.g.
> >
> > "Note that despite that vmas are refcounted using i915_vma_get() and
> > i915_vma_put() we don't refcount them as part of the state like other
> > objects. Instead their lifetime is only between prepare_planes and
> > cleanup_planes, and hence vma must be set to NULL when duplicating the
> > plane state."
>
> That would be false :-p
What's the false part? Ignoring the takeover corner case, where we have
lots of other cases where we need to adjust refcounts and pointers and
make sure it all looks like boot-up never happened, and it matches state
that a normal atomic commit could have produced.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
