| Message ID | 20161115085817.4210-3-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@chris-wilson.co.uk> 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 ... > err: > intel_runtime_pm_put(dev_priv); > return vma; > } > > -void intel_unpin_fb_obj(struct drm_framebuffer *fb, unsigned int rotation) > +void intel_unpin_fb_vma(struct i915_vma *vma) > { > - struct drm_i915_gem_object *obj = intel_fb_obj(fb); > - struct i915_ggtt_view view; > - struct i915_vma *vma; > - > - WARN_ON(!mutex_is_locked(&obj->base.dev->struct_mutex)); > - > - intel_fill_fb_ggtt_view(&view, fb, rotation); > - vma = i915_gem_object_to_ggtt(obj, &view); > + lockdep_assert_held(&vma->vm->dev->struct_mutex); > > i915_vma_unpin_fence(vma); > i915_gem_object_unpin_from_display_plane(vma); > + i915_vma_put(vma); > } > > static int intel_fb_pitch(const struct drm_framebuffer *fb, int plane, > @@ -2752,7 +2747,6 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc, > struct drm_device *dev = intel_crtc->base.dev; > struct drm_i915_private *dev_priv = to_i915(dev); > struct drm_crtc *c; > - struct intel_crtc *i; > struct drm_i915_gem_object *obj; > struct drm_plane *primary = intel_crtc->base.primary; > struct drm_plane_state *plane_state = primary->state; > @@ -2777,20 +2771,20 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc, > * an fb with another CRTC instead > */ > for_each_crtc(dev, c) { > - i = to_intel_crtc(c); > + struct intel_plane_state *state; > > if (c == &intel_crtc->base) > continue; > > - if (!i->active) > + if (!to_intel_crtc(c)->active) > continue; > > - fb = c->primary->fb; > - if (!fb) > + state = to_intel_plane_state(c->primary->state); > + if (!state->vma) > continue; > > - obj = intel_fb_obj(fb); > - if (i915_gem_object_ggtt_offset(obj, NULL) == plane_config->base) { > + if (i915_ggtt_offset(state->vma) == plane_config->base) { > + fb = c->primary->fb; > drm_framebuffer_reference(fb); > goto valid_fb; > } > @@ -2830,6 +2824,12 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc, > > drm_framebuffer_reference(fb); > primary->fb = primary->state->fb = fb; > + > + mutex_lock(&dev->struct_mutex); > + intel_state->vma = > + intel_pin_and_fence_fb_obj(fb, primary->state->rotation); > + mutex_unlock(&dev->struct_mutex); > + > primary->crtc = primary->state->crtc = &intel_crtc->base; > intel_crtc->base.state->plane_mask |= (1 << drm_plane_index(primary)); > atomic_or(to_intel_plane(primary)->frontbuffer_bit, > @@ -3104,13 +3104,13 @@ static void i9xx_update_primary_plane(struct drm_plane *primary, > I915_WRITE(DSPSTRIDE(plane), fb->pitches[0]); > if (INTEL_INFO(dev)->gen >= 4) { > I915_WRITE(DSPSURF(plane), > - intel_fb_gtt_offset(fb, rotation) + > + intel_plane_ggtt_offset(plane_state) + > intel_crtc->dspaddr_offset); > I915_WRITE(DSPTILEOFF(plane), (y << 16) | x); > I915_WRITE(DSPLINOFF(plane), linear_offset); > } else { > I915_WRITE(DSPADDR(plane), > - intel_fb_gtt_offset(fb, rotation) + > + intel_plane_ggtt_offset(plane_state) + > intel_crtc->dspaddr_offset); > } > POSTING_READ(reg); > @@ -3207,7 +3207,7 @@ static void ironlake_update_primary_plane(struct drm_plane *primary, > > I915_WRITE(DSPSTRIDE(plane), fb->pitches[0]); > I915_WRITE(DSPSURF(plane), > - intel_fb_gtt_offset(fb, rotation) + > + intel_plane_ggtt_offset(plane_state) + > intel_crtc->dspaddr_offset); > if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) { > I915_WRITE(DSPOFFSET(plane), (y << 16) | x); > @@ -3230,23 +3230,6 @@ u32 intel_fb_stride_alignment(const struct drm_i915_private *dev_priv, > } > } > > -u32 intel_fb_gtt_offset(struct drm_framebuffer *fb, > - unsigned int rotation) > -{ > - struct drm_i915_gem_object *obj = intel_fb_obj(fb); > - struct i915_ggtt_view view; > - struct i915_vma *vma; > - > - intel_fill_fb_ggtt_view(&view, fb, rotation); > - > - vma = i915_gem_object_to_ggtt(obj, &view); > - if (WARN(!vma, "ggtt vma for display object not found! (view=%u)\n", > - view.type)) > - return -1; > - > - return i915_ggtt_offset(vma); > -} > - > static void skl_detach_scaler(struct intel_crtc *intel_crtc, int id) > { > struct drm_device *dev = intel_crtc->base.dev; > @@ -3447,7 +3430,7 @@ static void skylake_update_primary_plane(struct drm_plane *plane, > } > > I915_WRITE(PLANE_SURF(pipe, 0), > - intel_fb_gtt_offset(fb, rotation) + surf_addr); > + intel_plane_ggtt_offset(plane_state) + surf_addr); > > POSTING_READ(PLANE_SURF(pipe, 0)); > } > @@ -11576,7 +11559,7 @@ static void intel_unpin_work_fn(struct work_struct *__work) > flush_work(&work->mmio_work); > > mutex_lock(&dev->struct_mutex); > - intel_unpin_fb_obj(work->old_fb, primary->state->rotation); > + intel_unpin_fb_vma(work->old_vma); > i915_gem_object_put(work->pending_flip_obj); > mutex_unlock(&dev->struct_mutex); > > @@ -12286,8 +12269,10 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, > goto cleanup_pending; > } > > - work->gtt_offset = intel_fb_gtt_offset(fb, primary->state->rotation); > - work->gtt_offset += intel_crtc->dspaddr_offset; > + work->old_vma = to_intel_plane_state(primary->state)->vma; > + to_intel_plane_state(primary->state)->vma = vma; > + > + work->gtt_offset = i915_ggtt_offset(vma) + intel_crtc->dspaddr_offset; > work->rotation = crtc->primary->state->rotation; > > /* > @@ -12340,7 +12325,8 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, > cleanup_request: > i915_add_request_no_flush(request); > cleanup_unpin: > - intel_unpin_fb_obj(fb, crtc->primary->state->rotation); > + to_intel_plane_state(primary->state)->vma = work->old_vma; > + intel_unpin_fb_vma(vma); > cleanup_pending: > atomic_dec(&intel_crtc->unpin_work_count); > mutex_unlock(&dev->struct_mutex); > @@ -14234,10 +14220,10 @@ intel_prepare_plane_fb(struct drm_plane *plane, > struct i915_vma *vma; > > vma = intel_pin_and_fence_fb_obj(fb, new_state->rotation); > - if (IS_ERR(vma)) { > - DRM_DEBUG_KMS("failed to pin object\n"); > - return PTR_ERR(vma); > - } > + if (IS_ERR(vma)) > + ret = PTR_ERR(vma); > + > + to_intel_plane_state(new_state)->vma = vma; > } > > return 0; > @@ -14256,19 +14242,12 @@ static void > intel_cleanup_plane_fb(struct drm_plane *plane, > struct drm_plane_state *old_state) > { > - struct drm_i915_private *dev_priv = to_i915(plane->dev); > - struct intel_plane_state *old_intel_state; > - struct drm_i915_gem_object *old_obj = intel_fb_obj(old_state->fb); > - struct drm_i915_gem_object *obj = intel_fb_obj(plane->state->fb); > - > - old_intel_state = to_intel_plane_state(old_state); > - > - if (!obj && !old_obj) > - return; > + struct i915_vma *vma; > > - if (old_obj && (plane->type != DRM_PLANE_TYPE_CURSOR || > - !INTEL_INFO(dev_priv)->cursor_needs_physical)) > - intel_unpin_fb_obj(old_state->fb, old_state->rotation); > + /* Should only called after a successful intel_prepare_plane_fb()! */ > + vma = fetch_and_zero(&to_intel_plane_state(old_state)->vma); > + if (vma) WARN_ON(!vma); maybe even instead of the comment? > + intel_unpin_fb_vma(vma); > } > > static int intel_atomic_commit_prepare_planes(struct drm_atomic_state *state) > @@ -15215,7 +15194,7 @@ intel_update_cursor_plane(struct drm_plane *plane, > if (!obj) > addr = 0; > else if (!INTEL_INFO(dev_priv)->cursor_needs_physical) > - addr = i915_gem_object_ggtt_offset(obj, NULL); > + addr = i915_ggtt_offset(state->vma); > else > addr = obj->phys_handle->busaddr; > > @@ -17135,6 +17114,8 @@ void intel_modeset_gem_init(struct drm_device *dev) > update_state_fb(c->primary); > c->state->plane_mask &= ~(1 << drm_plane_index(c->primary)); > } > + > + to_intel_plane_state(c->primary->state)->vma = vma; > } > } > > 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." > + struct i915_vma *vma; > > struct { > u32 offset; > @@ -1046,6 +1047,7 @@ struct intel_flip_work { > struct work_struct mmio_work; > > struct drm_crtc *crtc; > + struct i915_vma *old_vma; > struct drm_framebuffer *old_fb; > struct drm_i915_gem_object *pending_flip_obj; > struct drm_pending_vblank_event *event; > @@ -1273,7 +1275,7 @@ void intel_release_load_detect_pipe(struct drm_connector *connector, > struct drm_modeset_acquire_ctx *ctx); > struct i915_vma * > intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, unsigned int rotation); > -void intel_unpin_fb_obj(struct drm_framebuffer *fb, unsigned int rotation); > +void intel_unpin_fb_vma(struct i915_vma *vma); > struct drm_framebuffer * > __intel_framebuffer_create(struct drm_device *dev, > struct drm_mode_fb_cmd2 *mode_cmd, > @@ -1358,7 +1360,10 @@ void intel_mode_from_pipe_config(struct drm_display_mode *mode, > int skl_update_scaler_crtc(struct intel_crtc_state *crtc_state); > int skl_max_scale(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state); > > -u32 intel_fb_gtt_offset(struct drm_framebuffer *fb, unsigned int rotation); > +static inline u32 intel_plane_ggtt_offset(const struct intel_plane_state *state) > +{ > + return i915_ggtt_offset(state->vma); > +} > > u32 skl_plane_ctl_format(uint32_t pixel_format); > u32 skl_plane_ctl_tiling(uint64_t fb_modifier); > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c > index 62f215b12eb5..f3a1d6a5cabe 100644 > --- a/drivers/gpu/drm/i915/intel_fbc.c > +++ b/drivers/gpu/drm/i915/intel_fbc.c > @@ -173,7 +173,7 @@ static void i8xx_fbc_activate(struct drm_i915_private *dev_priv) > if (IS_I945GM(dev_priv)) > fbc_ctl |= FBC_CTL_C3_IDLE; /* 945 needs special SR handling */ > fbc_ctl |= (cfb_pitch & 0xff) << FBC_CTL_STRIDE_SHIFT; > - fbc_ctl |= params->fb.fence_reg; > + fbc_ctl |= params->vma->fence->id; > I915_WRITE(FBC_CONTROL, fbc_ctl); > } > > @@ -193,8 +193,8 @@ static void g4x_fbc_activate(struct drm_i915_private *dev_priv) > else > dpfc_ctl |= DPFC_CTL_LIMIT_1X; > > - if (params->fb.fence_reg != I915_FENCE_REG_NONE) { > - dpfc_ctl |= DPFC_CTL_FENCE_EN | params->fb.fence_reg; > + if (params->vma->fence) { > + dpfc_ctl |= DPFC_CTL_FENCE_EN | params->vma->fence->id; > I915_WRITE(DPFC_FENCE_YOFF, params->crtc.fence_y_offset); > } else { > I915_WRITE(DPFC_FENCE_YOFF, 0); > @@ -251,13 +251,14 @@ static void ilk_fbc_activate(struct drm_i915_private *dev_priv) > break; > } > > - if (params->fb.fence_reg != I915_FENCE_REG_NONE) { > + if (params->vma->fence) { > dpfc_ctl |= DPFC_CTL_FENCE_EN; > if (IS_GEN5(dev_priv)) > - dpfc_ctl |= params->fb.fence_reg; > + dpfc_ctl |= params->vma->fence->id; > if (IS_GEN6(dev_priv)) { > I915_WRITE(SNB_DPFC_CTL_SA, > - SNB_CPU_FENCE_ENABLE | params->fb.fence_reg); > + SNB_CPU_FENCE_ENABLE | > + params->vma->fence->id); > I915_WRITE(DPFC_CPU_FENCE_OFFSET, > params->crtc.fence_y_offset); > } > @@ -269,7 +270,8 @@ static void ilk_fbc_activate(struct drm_i915_private *dev_priv) > } > > I915_WRITE(ILK_DPFC_FENCE_YOFF, params->crtc.fence_y_offset); > - I915_WRITE(ILK_FBC_RT_BASE, params->fb.ggtt_offset | ILK_FBC_RT_VALID); > + I915_WRITE(ILK_FBC_RT_BASE, > + i915_ggtt_offset(params->vma) | ILK_FBC_RT_VALID); > /* enable it... */ > I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl | DPFC_CTL_EN); > > @@ -319,10 +321,11 @@ static void gen7_fbc_activate(struct drm_i915_private *dev_priv) > break; > } > > - if (params->fb.fence_reg != I915_FENCE_REG_NONE) { > + if (params->vma->fence) { > dpfc_ctl |= IVB_DPFC_CTL_FENCE_EN; > I915_WRITE(SNB_DPFC_CTL_SA, > - SNB_CPU_FENCE_ENABLE | params->fb.fence_reg); > + SNB_CPU_FENCE_ENABLE | > + params->vma->fence->id); > I915_WRITE(DPFC_CPU_FENCE_OFFSET, params->crtc.fence_y_offset); > } else { > I915_WRITE(SNB_DPFC_CTL_SA,0); > @@ -727,14 +730,6 @@ static bool intel_fbc_hw_tracking_covers_screen(struct intel_crtc *crtc) > return effective_w <= max_w && effective_h <= max_h; > } > > -/* XXX replace me when we have VMA tracking for intel_plane_state */ > -static int get_fence_id(struct drm_framebuffer *fb) > -{ > - struct i915_vma *vma = i915_gem_object_to_ggtt(intel_fb_obj(fb), NULL); > - > - return vma && vma->fence ? vma->fence->id : I915_FENCE_REG_NONE; > -} > - > static void intel_fbc_update_state_cache(struct intel_crtc *crtc, > struct intel_crtc_state *crtc_state, > struct intel_plane_state *plane_state) > @@ -743,7 +738,8 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc, > struct intel_fbc *fbc = &dev_priv->fbc; > struct intel_fbc_state_cache *cache = &fbc->state_cache; > struct drm_framebuffer *fb = plane_state->base.fb; > - struct drm_i915_gem_object *obj; > + > + cache->vma = NULL; > > cache->crtc.mode_flags = crtc_state->base.adjusted_mode.flags; > if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) > @@ -758,16 +754,10 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc, > if (!cache->plane.visible) > return; > > - obj = intel_fb_obj(fb); > - > - /* FIXME: We lack the proper locking here, so only run this on the > - * platforms that need. */ > - if (IS_GEN(dev_priv, 5, 6)) > - cache->fb.ilk_ggtt_offset = i915_gem_object_ggtt_offset(obj, NULL); > cache->fb.pixel_format = fb->pixel_format; > cache->fb.stride = fb->pitches[0]; > - cache->fb.fence_reg = get_fence_id(fb); > - cache->fb.tiling_mode = i915_gem_object_get_tiling(obj); > + > + cache->vma = plane_state->vma; > } > > static bool intel_fbc_can_activate(struct intel_crtc *crtc) > @@ -784,7 +774,7 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc) > return false; > } > > - if (!cache->plane.visible) { > + if (!cache->vma) { > fbc->no_fbc_reason = "primary plane not visible"; > return false; > } > @@ -807,8 +797,7 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc) > * so have no fence associated with it) due to aperture constaints > * at the time of pinning. > */ > - if (cache->fb.tiling_mode != I915_TILING_X || > - cache->fb.fence_reg == I915_FENCE_REG_NONE) { > + if (!cache->vma->fence) { > fbc->no_fbc_reason = "framebuffer not tiled or fenced"; > return false; > } > @@ -888,17 +877,16 @@ static void intel_fbc_get_reg_params(struct intel_crtc *crtc, > * zero. */ > memset(params, 0, sizeof(*params)); > > + params->vma = cache->vma; > + > params->crtc.pipe = crtc->pipe; > params->crtc.plane = crtc->plane; > params->crtc.fence_y_offset = get_crtc_fence_y_offset(crtc); > > params->fb.pixel_format = cache->fb.pixel_format; > params->fb.stride = cache->fb.stride; > - params->fb.fence_reg = cache->fb.fence_reg; > > params->cfb_size = intel_fbc_calculate_cfb_size(dev_priv, cache); > - > - params->fb.ggtt_offset = cache->fb.ilk_ggtt_offset; > } > > static bool intel_fbc_reg_params_equal(struct intel_fbc_reg_params *params1, > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c > index fc958d5ed0dc..ca29a5314f0e 100644 > --- a/drivers/gpu/drm/i915/intel_fbdev.c > +++ b/drivers/gpu/drm/i915/intel_fbdev.c > @@ -284,7 +284,7 @@ static int intelfb_create(struct drm_fb_helper *helper, > out_destroy_fbi: > drm_fb_helper_release_fbi(helper); > out_unpin: > - intel_unpin_fb_obj(&ifbdev->fb->base, DRM_ROTATE_0); > + intel_unpin_fb_vma(vma); > out_unlock: > mutex_unlock(&dev->struct_mutex); > return ret; > @@ -549,7 +549,7 @@ static void intel_fbdev_destroy(struct intel_fbdev *ifbdev) > > if (ifbdev->fb) { > mutex_lock(&ifbdev->helper.dev->struct_mutex); > - intel_unpin_fb_obj(&ifbdev->fb->base, DRM_ROTATE_0); > + intel_unpin_fb_vma(ifbdev->vma); > mutex_unlock(&ifbdev->helper.dev->struct_mutex); > > drm_framebuffer_remove(&ifbdev->fb->base); > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > index 8b2fc67acbba..ae8ce621e3fc 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -281,7 +281,7 @@ skl_update_plane(struct drm_plane *drm_plane, > > I915_WRITE(PLANE_CTL(pipe, plane), plane_ctl); > I915_WRITE(PLANE_SURF(pipe, plane), > - intel_fb_gtt_offset(fb, rotation) + surf_addr); > + intel_plane_ggtt_offset(plane_state) + surf_addr); > POSTING_READ(PLANE_SURF(pipe, plane)); > } > > @@ -476,7 +476,7 @@ vlv_update_plane(struct drm_plane *dplane, > I915_WRITE(SPSIZE(pipe, plane), (crtc_h << 16) | crtc_w); > I915_WRITE(SPCNTR(pipe, plane), sprctl); > I915_WRITE(SPSURF(pipe, plane), > - intel_fb_gtt_offset(fb, rotation) + sprsurf_offset); > + intel_plane_ggtt_offset(plane_state) + sprsurf_offset); > POSTING_READ(SPSURF(pipe, plane)); > } > > @@ -612,7 +612,7 @@ ivb_update_plane(struct drm_plane *plane, > I915_WRITE(SPRSCALE(pipe), sprscale); > I915_WRITE(SPRCTL(pipe), sprctl); > I915_WRITE(SPRSURF(pipe), > - intel_fb_gtt_offset(fb, rotation) + sprsurf_offset); > + intel_plane_ggtt_offset(plane_state) + sprsurf_offset); > POSTING_READ(SPRSURF(pipe)); > } > > @@ -739,7 +739,7 @@ ilk_update_plane(struct drm_plane *plane, > I915_WRITE(DVSSCALE(pipe), dvsscale); > I915_WRITE(DVSCNTR(pipe), dvscntr); > I915_WRITE(DVSSURF(pipe), > - intel_fb_gtt_offset(fb, rotation) + dvssurf_offset); > + intel_plane_ggtt_offset(plane_state) + dvssurf_offset); > POSTING_READ(DVSSURF(pipe)); > } > > -- > 2.10.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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@chris-wilson.co.uk> > > 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. > > 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 -Chris
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@chris-wilson.co.uk> > > > > 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
On Tue, Nov 15, 2016 at 08:58:15AM +0000, Chris Wilson wrote: > @@ -12340,7 +12325,8 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, > cleanup_request: > i915_add_request_no_flush(request); > cleanup_unpin: > - intel_unpin_fb_obj(fb, crtc->primary->state->rotation); > + to_intel_plane_state(primary->state)->vma = work->old_vma; > + intel_unpin_fb_vma(vma); > cleanup_pending: > atomic_dec(&intel_crtc->unpin_work_count); > mutex_unlock(&dev->struct_mutex); > @@ -14234,10 +14220,10 @@ intel_prepare_plane_fb(struct drm_plane *plane, > struct i915_vma *vma; > > vma = intel_pin_and_fence_fb_obj(fb, new_state->rotation); > - if (IS_ERR(vma)) { > - DRM_DEBUG_KMS("failed to pin object\n"); > - return PTR_ERR(vma); > - } > + if (IS_ERR(vma)) > + ret = PTR_ERR(vma); Uncessary change above that you then go ahead and partially undo in the next patch. Please remove. -Daniel
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); err: intel_runtime_pm_put(dev_priv); return vma; } -void intel_unpin_fb_obj(struct drm_framebuffer *fb, unsigned int rotation) +void intel_unpin_fb_vma(struct i915_vma *vma) { - struct drm_i915_gem_object *obj = intel_fb_obj(fb); - struct i915_ggtt_view view; - struct i915_vma *vma; - - WARN_ON(!mutex_is_locked(&obj->base.dev->struct_mutex)); - - intel_fill_fb_ggtt_view(&view, fb, rotation); - vma = i915_gem_object_to_ggtt(obj, &view); + lockdep_assert_held(&vma->vm->dev->struct_mutex); i915_vma_unpin_fence(vma); i915_gem_object_unpin_from_display_plane(vma); + i915_vma_put(vma); } static int intel_fb_pitch(const struct drm_framebuffer *fb, int plane, @@ -2752,7 +2747,6 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc, struct drm_device *dev = intel_crtc->base.dev; struct drm_i915_private *dev_priv = to_i915(dev); struct drm_crtc *c; - struct intel_crtc *i; struct drm_i915_gem_object *obj; struct drm_plane *primary = intel_crtc->base.primary; struct drm_plane_state *plane_state = primary->state; @@ -2777,20 +2771,20 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc, * an fb with another CRTC instead */ for_each_crtc(dev, c) { - i = to_intel_crtc(c); + struct intel_plane_state *state; if (c == &intel_crtc->base) continue; - if (!i->active) + if (!to_intel_crtc(c)->active) continue; - fb = c->primary->fb; - if (!fb) + state = to_intel_plane_state(c->primary->state); + if (!state->vma) continue; - obj = intel_fb_obj(fb); - if (i915_gem_object_ggtt_offset(obj, NULL) == plane_config->base) { + if (i915_ggtt_offset(state->vma) == plane_config->base) { + fb = c->primary->fb; drm_framebuffer_reference(fb); goto valid_fb; } @@ -2830,6 +2824,12 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc, drm_framebuffer_reference(fb); primary->fb = primary->state->fb = fb; + + mutex_lock(&dev->struct_mutex); + intel_state->vma = + intel_pin_and_fence_fb_obj(fb, primary->state->rotation); + mutex_unlock(&dev->struct_mutex); + primary->crtc = primary->state->crtc = &intel_crtc->base; intel_crtc->base.state->plane_mask |= (1 << drm_plane_index(primary)); atomic_or(to_intel_plane(primary)->frontbuffer_bit, @@ -3104,13 +3104,13 @@ static void i9xx_update_primary_plane(struct drm_plane *primary, I915_WRITE(DSPSTRIDE(plane), fb->pitches[0]); if (INTEL_INFO(dev)->gen >= 4) { I915_WRITE(DSPSURF(plane), - intel_fb_gtt_offset(fb, rotation) + + intel_plane_ggtt_offset(plane_state) + intel_crtc->dspaddr_offset); I915_WRITE(DSPTILEOFF(plane), (y << 16) | x); I915_WRITE(DSPLINOFF(plane), linear_offset); } else { I915_WRITE(DSPADDR(plane), - intel_fb_gtt_offset(fb, rotation) + + intel_plane_ggtt_offset(plane_state) + intel_crtc->dspaddr_offset); } POSTING_READ(reg); @@ -3207,7 +3207,7 @@ static void ironlake_update_primary_plane(struct drm_plane *primary, I915_WRITE(DSPSTRIDE(plane), fb->pitches[0]); I915_WRITE(DSPSURF(plane), - intel_fb_gtt_offset(fb, rotation) + + intel_plane_ggtt_offset(plane_state) + intel_crtc->dspaddr_offset); if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) { I915_WRITE(DSPOFFSET(plane), (y << 16) | x); @@ -3230,23 +3230,6 @@ u32 intel_fb_stride_alignment(const struct drm_i915_private *dev_priv, } } -u32 intel_fb_gtt_offset(struct drm_framebuffer *fb, - unsigned int rotation) -{ - struct drm_i915_gem_object *obj = intel_fb_obj(fb); - struct i915_ggtt_view view; - struct i915_vma *vma; - - intel_fill_fb_ggtt_view(&view, fb, rotation); - - vma = i915_gem_object_to_ggtt(obj, &view); - if (WARN(!vma, "ggtt vma for display object not found! (view=%u)\n", - view.type)) - return -1; - - return i915_ggtt_offset(vma); -} - static void skl_detach_scaler(struct intel_crtc *intel_crtc, int id) { struct drm_device *dev = intel_crtc->base.dev; @@ -3447,7 +3430,7 @@ static void skylake_update_primary_plane(struct drm_plane *plane, } I915_WRITE(PLANE_SURF(pipe, 0), - intel_fb_gtt_offset(fb, rotation) + surf_addr); + intel_plane_ggtt_offset(plane_state) + surf_addr); POSTING_READ(PLANE_SURF(pipe, 0)); } @@ -11576,7 +11559,7 @@ static void intel_unpin_work_fn(struct work_struct *__work) flush_work(&work->mmio_work); mutex_lock(&dev->struct_mutex); - intel_unpin_fb_obj(work->old_fb, primary->state->rotation); + intel_unpin_fb_vma(work->old_vma); i915_gem_object_put(work->pending_flip_obj); mutex_unlock(&dev->struct_mutex); @@ -12286,8 +12269,10 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, goto cleanup_pending; } - work->gtt_offset = intel_fb_gtt_offset(fb, primary->state->rotation); - work->gtt_offset += intel_crtc->dspaddr_offset; + work->old_vma = to_intel_plane_state(primary->state)->vma; + to_intel_plane_state(primary->state)->vma = vma; + + work->gtt_offset = i915_ggtt_offset(vma) + intel_crtc->dspaddr_offset; work->rotation = crtc->primary->state->rotation; /* @@ -12340,7 +12325,8 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, cleanup_request: i915_add_request_no_flush(request); cleanup_unpin: - intel_unpin_fb_obj(fb, crtc->primary->state->rotation); + to_intel_plane_state(primary->state)->vma = work->old_vma; + intel_unpin_fb_vma(vma); cleanup_pending: atomic_dec(&intel_crtc->unpin_work_count); mutex_unlock(&dev->struct_mutex); @@ -14234,10 +14220,10 @@ intel_prepare_plane_fb(struct drm_plane *plane, struct i915_vma *vma; vma = intel_pin_and_fence_fb_obj(fb, new_state->rotation); - if (IS_ERR(vma)) { - DRM_DEBUG_KMS("failed to pin object\n"); - return PTR_ERR(vma); - } + if (IS_ERR(vma)) + ret = PTR_ERR(vma); + + to_intel_plane_state(new_state)->vma = vma; } return 0; @@ -14256,19 +14242,12 @@ static void intel_cleanup_plane_fb(struct drm_plane *plane, struct drm_plane_state *old_state) { - struct drm_i915_private *dev_priv = to_i915(plane->dev); - struct intel_plane_state *old_intel_state; - struct drm_i915_gem_object *old_obj = intel_fb_obj(old_state->fb); - struct drm_i915_gem_object *obj = intel_fb_obj(plane->state->fb); - - old_intel_state = to_intel_plane_state(old_state); - - if (!obj && !old_obj) - return; + struct i915_vma *vma; - if (old_obj && (plane->type != DRM_PLANE_TYPE_CURSOR || - !INTEL_INFO(dev_priv)->cursor_needs_physical)) - intel_unpin_fb_obj(old_state->fb, old_state->rotation); + /* Should only called after a successful intel_prepare_plane_fb()! */ + vma = fetch_and_zero(&to_intel_plane_state(old_state)->vma); + if (vma) + intel_unpin_fb_vma(vma); } static int intel_atomic_commit_prepare_planes(struct drm_atomic_state *state) @@ -15215,7 +15194,7 @@ intel_update_cursor_plane(struct drm_plane *plane, if (!obj) addr = 0; else if (!INTEL_INFO(dev_priv)->cursor_needs_physical) - addr = i915_gem_object_ggtt_offset(obj, NULL); + addr = i915_ggtt_offset(state->vma); else addr = obj->phys_handle->busaddr; @@ -17135,6 +17114,8 @@ void intel_modeset_gem_init(struct drm_device *dev) update_state_fb(c->primary); c->state->plane_mask &= ~(1 << drm_plane_index(c->primary)); } + + to_intel_plane_state(c->primary->state)->vma = vma; } } 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; + struct i915_vma *vma; struct { u32 offset; @@ -1046,6 +1047,7 @@ struct intel_flip_work { struct work_struct mmio_work; struct drm_crtc *crtc; + struct i915_vma *old_vma; struct drm_framebuffer *old_fb; struct drm_i915_gem_object *pending_flip_obj; struct drm_pending_vblank_event *event; @@ -1273,7 +1275,7 @@ void intel_release_load_detect_pipe(struct drm_connector *connector, struct drm_modeset_acquire_ctx *ctx); struct i915_vma * intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, unsigned int rotation); -void intel_unpin_fb_obj(struct drm_framebuffer *fb, unsigned int rotation); +void intel_unpin_fb_vma(struct i915_vma *vma); struct drm_framebuffer * __intel_framebuffer_create(struct drm_device *dev, struct drm_mode_fb_cmd2 *mode_cmd, @@ -1358,7 +1360,10 @@ void intel_mode_from_pipe_config(struct drm_display_mode *mode, int skl_update_scaler_crtc(struct intel_crtc_state *crtc_state); int skl_max_scale(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state); -u32 intel_fb_gtt_offset(struct drm_framebuffer *fb, unsigned int rotation); +static inline u32 intel_plane_ggtt_offset(const struct intel_plane_state *state) +{ + return i915_ggtt_offset(state->vma); +} u32 skl_plane_ctl_format(uint32_t pixel_format); u32 skl_plane_ctl_tiling(uint64_t fb_modifier); diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c index 62f215b12eb5..f3a1d6a5cabe 100644 --- a/drivers/gpu/drm/i915/intel_fbc.c +++ b/drivers/gpu/drm/i915/intel_fbc.c @@ -173,7 +173,7 @@ static void i8xx_fbc_activate(struct drm_i915_private *dev_priv) if (IS_I945GM(dev_priv)) fbc_ctl |= FBC_CTL_C3_IDLE; /* 945 needs special SR handling */ fbc_ctl |= (cfb_pitch & 0xff) << FBC_CTL_STRIDE_SHIFT; - fbc_ctl |= params->fb.fence_reg; + fbc_ctl |= params->vma->fence->id; I915_WRITE(FBC_CONTROL, fbc_ctl); } @@ -193,8 +193,8 @@ static void g4x_fbc_activate(struct drm_i915_private *dev_priv) else dpfc_ctl |= DPFC_CTL_LIMIT_1X; - if (params->fb.fence_reg != I915_FENCE_REG_NONE) { - dpfc_ctl |= DPFC_CTL_FENCE_EN | params->fb.fence_reg; + if (params->vma->fence) { + dpfc_ctl |= DPFC_CTL_FENCE_EN | params->vma->fence->id; I915_WRITE(DPFC_FENCE_YOFF, params->crtc.fence_y_offset); } else { I915_WRITE(DPFC_FENCE_YOFF, 0); @@ -251,13 +251,14 @@ static void ilk_fbc_activate(struct drm_i915_private *dev_priv) break; } - if (params->fb.fence_reg != I915_FENCE_REG_NONE) { + if (params->vma->fence) { dpfc_ctl |= DPFC_CTL_FENCE_EN; if (IS_GEN5(dev_priv)) - dpfc_ctl |= params->fb.fence_reg; + dpfc_ctl |= params->vma->fence->id; if (IS_GEN6(dev_priv)) { I915_WRITE(SNB_DPFC_CTL_SA, - SNB_CPU_FENCE_ENABLE | params->fb.fence_reg); + SNB_CPU_FENCE_ENABLE | + params->vma->fence->id); I915_WRITE(DPFC_CPU_FENCE_OFFSET, params->crtc.fence_y_offset); } @@ -269,7 +270,8 @@ static void ilk_fbc_activate(struct drm_i915_private *dev_priv) } I915_WRITE(ILK_DPFC_FENCE_YOFF, params->crtc.fence_y_offset); - I915_WRITE(ILK_FBC_RT_BASE, params->fb.ggtt_offset | ILK_FBC_RT_VALID); + I915_WRITE(ILK_FBC_RT_BASE, + i915_ggtt_offset(params->vma) | ILK_FBC_RT_VALID); /* enable it... */ I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl | DPFC_CTL_EN); @@ -319,10 +321,11 @@ static void gen7_fbc_activate(struct drm_i915_private *dev_priv) break; } - if (params->fb.fence_reg != I915_FENCE_REG_NONE) { + if (params->vma->fence) { dpfc_ctl |= IVB_DPFC_CTL_FENCE_EN; I915_WRITE(SNB_DPFC_CTL_SA, - SNB_CPU_FENCE_ENABLE | params->fb.fence_reg); + SNB_CPU_FENCE_ENABLE | + params->vma->fence->id); I915_WRITE(DPFC_CPU_FENCE_OFFSET, params->crtc.fence_y_offset); } else { I915_WRITE(SNB_DPFC_CTL_SA,0); @@ -727,14 +730,6 @@ static bool intel_fbc_hw_tracking_covers_screen(struct intel_crtc *crtc) return effective_w <= max_w && effective_h <= max_h; } -/* XXX replace me when we have VMA tracking for intel_plane_state */ -static int get_fence_id(struct drm_framebuffer *fb) -{ - struct i915_vma *vma = i915_gem_object_to_ggtt(intel_fb_obj(fb), NULL); - - return vma && vma->fence ? vma->fence->id : I915_FENCE_REG_NONE; -} - static void intel_fbc_update_state_cache(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state, struct intel_plane_state *plane_state) @@ -743,7 +738,8 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc, struct intel_fbc *fbc = &dev_priv->fbc; struct intel_fbc_state_cache *cache = &fbc->state_cache; struct drm_framebuffer *fb = plane_state->base.fb; - struct drm_i915_gem_object *obj; + + cache->vma = NULL; cache->crtc.mode_flags = crtc_state->base.adjusted_mode.flags; if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) @@ -758,16 +754,10 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc, if (!cache->plane.visible) return; - obj = intel_fb_obj(fb); - - /* FIXME: We lack the proper locking here, so only run this on the - * platforms that need. */ - if (IS_GEN(dev_priv, 5, 6)) - cache->fb.ilk_ggtt_offset = i915_gem_object_ggtt_offset(obj, NULL); cache->fb.pixel_format = fb->pixel_format; cache->fb.stride = fb->pitches[0]; - cache->fb.fence_reg = get_fence_id(fb); - cache->fb.tiling_mode = i915_gem_object_get_tiling(obj); + + cache->vma = plane_state->vma; } static bool intel_fbc_can_activate(struct intel_crtc *crtc) @@ -784,7 +774,7 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc) return false; } - if (!cache->plane.visible) { + if (!cache->vma) { fbc->no_fbc_reason = "primary plane not visible"; return false; } @@ -807,8 +797,7 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc) * so have no fence associated with it) due to aperture constaints * at the time of pinning. */ - if (cache->fb.tiling_mode != I915_TILING_X || - cache->fb.fence_reg == I915_FENCE_REG_NONE) { + if (!cache->vma->fence) { fbc->no_fbc_reason = "framebuffer not tiled or fenced"; return false; } @@ -888,17 +877,16 @@ static void intel_fbc_get_reg_params(struct intel_crtc *crtc, * zero. */ memset(params, 0, sizeof(*params)); + params->vma = cache->vma; + params->crtc.pipe = crtc->pipe; params->crtc.plane = crtc->plane; params->crtc.fence_y_offset = get_crtc_fence_y_offset(crtc); params->fb.pixel_format = cache->fb.pixel_format; params->fb.stride = cache->fb.stride; - params->fb.fence_reg = cache->fb.fence_reg; params->cfb_size = intel_fbc_calculate_cfb_size(dev_priv, cache); - - params->fb.ggtt_offset = cache->fb.ilk_ggtt_offset; } static bool intel_fbc_reg_params_equal(struct intel_fbc_reg_params *params1, diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c index fc958d5ed0dc..ca29a5314f0e 100644 --- a/drivers/gpu/drm/i915/intel_fbdev.c +++ b/drivers/gpu/drm/i915/intel_fbdev.c @@ -284,7 +284,7 @@ static int intelfb_create(struct drm_fb_helper *helper, out_destroy_fbi: drm_fb_helper_release_fbi(helper); out_unpin: - intel_unpin_fb_obj(&ifbdev->fb->base, DRM_ROTATE_0); + intel_unpin_fb_vma(vma); out_unlock: mutex_unlock(&dev->struct_mutex); return ret; @@ -549,7 +549,7 @@ static void intel_fbdev_destroy(struct intel_fbdev *ifbdev) if (ifbdev->fb) { mutex_lock(&ifbdev->helper.dev->struct_mutex); - intel_unpin_fb_obj(&ifbdev->fb->base, DRM_ROTATE_0); + intel_unpin_fb_vma(ifbdev->vma); mutex_unlock(&ifbdev->helper.dev->struct_mutex); drm_framebuffer_remove(&ifbdev->fb->base); diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index 8b2fc67acbba..ae8ce621e3fc 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -281,7 +281,7 @@ skl_update_plane(struct drm_plane *drm_plane, I915_WRITE(PLANE_CTL(pipe, plane), plane_ctl); I915_WRITE(PLANE_SURF(pipe, plane), - intel_fb_gtt_offset(fb, rotation) + surf_addr); + intel_plane_ggtt_offset(plane_state) + surf_addr); POSTING_READ(PLANE_SURF(pipe, plane)); } @@ -476,7 +476,7 @@ vlv_update_plane(struct drm_plane *dplane, I915_WRITE(SPSIZE(pipe, plane), (crtc_h << 16) | crtc_w); I915_WRITE(SPCNTR(pipe, plane), sprctl); I915_WRITE(SPSURF(pipe, plane), - intel_fb_gtt_offset(fb, rotation) + sprsurf_offset); + intel_plane_ggtt_offset(plane_state) + sprsurf_offset); POSTING_READ(SPSURF(pipe, plane)); } @@ -612,7 +612,7 @@ ivb_update_plane(struct drm_plane *plane, I915_WRITE(SPRSCALE(pipe), sprscale); I915_WRITE(SPRCTL(pipe), sprctl); I915_WRITE(SPRSURF(pipe), - intel_fb_gtt_offset(fb, rotation) + sprsurf_offset); + intel_plane_ggtt_offset(plane_state) + sprsurf_offset); POSTING_READ(SPRSURF(pipe)); } @@ -739,7 +739,7 @@ ilk_update_plane(struct drm_plane *plane, I915_WRITE(DVSSCALE(pipe), dvsscale); I915_WRITE(DVSCNTR(pipe), dvscntr); I915_WRITE(DVSSURF(pipe), - intel_fb_gtt_offset(fb, rotation) + dvssurf_offset); + intel_plane_ggtt_offset(plane_state) + dvssurf_offset); POSTING_READ(DVSSURF(pipe)); }
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@chris-wilson.co.uk> --- 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(-)