| Message ID | 1415904206-22548-7-git-send-email-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 13 Nov 2014 10:43:25 -0800 Matt Roper <matthew.d.roper@intel.com> wrote: > Now that we have hooks to enable the atomic plane helpers, we can use > the plane helpers for our .update_plane() and .disable_plane() > entrypoints. > > Even though we'd already refactored our plane handling code into > check/commit, we still have to rework some of that logic here as we make > the transition. The atomic plane helpers now call prepare_fb/cleanup_fb > for us, so note the following: > - 'commit' should not longer unpin the old object; the cleanup_fb will s/not/no/ > take care of this for us > - prepare_fb only gets called when we actually have an fb, so the > internal 'disable' code needs to update frontbuffer tracking > - the cursor plane code never had 'prepare' code separated from its > 'commit' function, so a lot of the cursor's 'commit' needs to be > dropped since prepare_fb already does it for us > - with all the pinning work removed from cursor commit, it can no > longer fail and should return void rather than int > > Testcase: igt/kms_plane > Testcase: igt/kms_universal_plane > Testcase: igt/kms_cursor_crc > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> With the above typo fixed this is Acknowledged-by: Bob Paauwe <bob.j.paauwe@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 255 +++++------------------------------ > drivers/gpu/drm/i915/intel_sprite.c | 122 ++--------------- > 2 files changed, 41 insertions(+), 336 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 32a2b12..8d5a3b4 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -8285,17 +8285,15 @@ static bool cursor_size_ok(struct drm_device *dev, > return true; > } > > -static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc, > - struct drm_i915_gem_object *obj, > - uint32_t width, uint32_t height) > +static void intel_crtc_cursor_set_obj(struct drm_crtc *crtc, > + struct drm_i915_gem_object *obj, > + uint32_t width, uint32_t height) > { > struct drm_device *dev = crtc->dev; > - struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > enum pipe pipe = intel_crtc->pipe; > unsigned old_width; > uint32_t addr; > - int ret; > > /* if we want to turn off the cursor ignore width and height */ > if (!obj) { > @@ -8305,64 +8303,14 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc, > goto finish; > } > > - /* we only need to pin inside GTT if cursor is non-phy */ > + /* plane helper's prepare_fb() already handled pinning */ > mutex_lock(&dev->struct_mutex); > - if (!INTEL_INFO(dev)->cursor_needs_physical) { > - unsigned alignment; > - > - /* > - * Global gtt pte registers are special registers which actually > - * forward writes to a chunk of system memory. Which means that > - * there is no risk that the register values disappear as soon > - * as we call intel_runtime_pm_put(), so it is correct to wrap > - * only the pin/unpin/fence and not more. > - */ > - intel_runtime_pm_get(dev_priv); > - > - /* Note that the w/a also requires 2 PTE of padding following > - * the bo. We currently fill all unused PTE with the shadow > - * page and so we should always have valid PTE following the > - * cursor preventing the VT-d warning. > - */ > - alignment = 0; > - if (need_vtd_wa(dev)) > - alignment = 64*1024; > - > - ret = i915_gem_object_pin_to_display_plane(obj, alignment, NULL); > - if (ret) { > - DRM_DEBUG_KMS("failed to move cursor bo into the GTT\n"); > - intel_runtime_pm_put(dev_priv); > - goto fail_locked; > - } > - > - ret = i915_gem_object_put_fence(obj); > - if (ret) { > - DRM_DEBUG_KMS("failed to release fence for cursor"); > - intel_runtime_pm_put(dev_priv); > - goto fail_unpin; > - } > - > + if (!INTEL_INFO(dev)->cursor_needs_physical) > addr = i915_gem_obj_ggtt_offset(obj); > - > - intel_runtime_pm_put(dev_priv); > - } else { > - int align = IS_I830(dev) ? 16 * 1024 : 256; > - ret = i915_gem_object_attach_phys(obj, align); > - if (ret) { > - DRM_DEBUG_KMS("failed to attach phys object\n"); > - goto fail_locked; > - } > + else > addr = obj->phys_handle->busaddr; > - } > > finish: > - if (intel_crtc->cursor_bo) { > - if (!INTEL_INFO(dev)->cursor_needs_physical) > - i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo); > - } > - > - i915_gem_track_fb(intel_crtc->cursor_bo, obj, > - INTEL_FRONTBUFFER_CURSOR(pipe)); > mutex_unlock(&dev->struct_mutex); > > old_width = intel_crtc->cursor_width; > @@ -8379,13 +8327,6 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc, > > intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe)); > } > - > - return 0; > -fail_unpin: > - i915_gem_object_unpin_from_display_plane(obj); > -fail_locked: > - mutex_unlock(&dev->struct_mutex); > - return ret; > } > > static void intel_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16 *green, > @@ -11544,7 +11485,6 @@ disable_unpin: > mutex_lock(&dev->struct_mutex); > i915_gem_track_fb(intel_fb_obj(plane->fb), NULL, > INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe)); > - intel_unpin_fb_obj(intel_fb_obj(plane->fb)); > mutex_unlock(&dev->struct_mutex); > plane->fb = NULL; > > @@ -11568,42 +11508,6 @@ intel_check_primary_plane(struct drm_plane *plane, > false, true, &state->visible); > } > > -static int > -intel_prepare_primary_plane(struct drm_plane *plane, > - struct intel_plane_state *state) > -{ > - struct drm_crtc *crtc = state->base.crtc; > - struct drm_framebuffer *fb = state->base.fb; > - struct drm_device *dev = crtc->dev; > - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > - enum pipe pipe = intel_crtc->pipe; > - struct drm_i915_gem_object *obj = intel_fb_obj(fb); > - struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb); > - int ret; > - > - intel_crtc_wait_for_pending_flips(crtc); > - > - if (intel_crtc_has_pending_flip(crtc)) { > - DRM_ERROR("pipe is still busy with an old pageflip\n"); > - return -EBUSY; > - } > - > - if (old_obj != obj) { > - mutex_lock(&dev->struct_mutex); > - ret = intel_pin_and_fence_fb_obj(plane, fb, NULL); > - if (ret == 0) > - i915_gem_track_fb(old_obj, obj, > - INTEL_FRONTBUFFER_PRIMARY(pipe)); > - mutex_unlock(&dev->struct_mutex); > - if (ret != 0) { > - DRM_DEBUG_KMS("pin & fence failed\n"); > - return ret; > - } > - } > - > - return 0; > -} > - > static void > intel_commit_primary_plane(struct drm_plane *plane, > struct intel_plane_state *state) > @@ -11614,9 +11518,7 @@ intel_commit_primary_plane(struct drm_plane *plane, > struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > enum pipe pipe = intel_crtc->pipe; > - struct drm_framebuffer *old_fb = plane->fb; > struct drm_i915_gem_object *obj = intel_fb_obj(fb); > - struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb); > struct intel_plane *intel_plane = to_intel_plane(plane); > struct drm_rect *src = &state->src; > > @@ -11687,62 +11589,6 @@ intel_commit_primary_plane(struct drm_plane *plane, > intel_update_fbc(dev); > mutex_unlock(&dev->struct_mutex); > } > - > - if (old_fb && old_fb != fb) { > - if (intel_crtc->active) > - intel_wait_for_vblank(dev, intel_crtc->pipe); > - > - mutex_lock(&dev->struct_mutex); > - intel_unpin_fb_obj(old_obj); > - mutex_unlock(&dev->struct_mutex); > - } > -} > - > -static int > -intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc, > - struct drm_framebuffer *fb, int crtc_x, int crtc_y, > - unsigned int crtc_w, unsigned int crtc_h, > - uint32_t src_x, uint32_t src_y, > - uint32_t src_w, uint32_t src_h) > -{ > - struct intel_plane_state state; > - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > - int ret; > - > - state.base.crtc = crtc; > - state.base.fb = fb; > - > - /* sample coordinates in 16.16 fixed point */ > - state.src.x1 = src_x; > - state.src.x2 = src_x + src_w; > - state.src.y1 = src_y; > - state.src.y2 = src_y + src_h; > - > - /* integer pixels */ > - state.dst.x1 = crtc_x; > - state.dst.x2 = crtc_x + crtc_w; > - state.dst.y1 = crtc_y; > - state.dst.y2 = crtc_y + crtc_h; > - > - state.clip.x1 = 0; > - state.clip.y1 = 0; > - state.clip.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0; > - state.clip.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0; > - > - state.orig_src = state.src; > - state.orig_dst = state.dst; > - > - ret = intel_check_primary_plane(plane, &state); > - if (ret) > - return ret; > - > - ret = intel_prepare_primary_plane(plane, &state); > - if (ret) > - return ret; > - > - intel_commit_primary_plane(plane, &state); > - > - return 0; > } > > /* Common destruction function for both primary and cursor planes */ > @@ -11755,8 +11601,8 @@ void intel_plane_destroy(struct drm_plane *plane) > } > > static const struct drm_plane_funcs intel_primary_plane_funcs = { > - .update_plane = intel_primary_plane_setplane, > - .disable_plane = intel_primary_plane_disable, > + .update_plane = drm_plane_helper_update, > + .disable_plane = drm_plane_helper_disable, > .destroy = intel_plane_destroy, > .set_property = intel_plane_set_property, > .atomic_duplicate_state = intel_plane_duplicate_state, > @@ -11816,15 +11662,22 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev, > return &primary->base; > } > > -static int > +static void > intel_cursor_plane_disable(struct drm_plane *plane) > { > + struct intel_plane *intel_plane = to_intel_plane(plane); > + > if (!plane->fb) > - return 0; > + return; > > BUG_ON(!plane->crtc); > > - return intel_crtc_cursor_set_obj(plane->crtc, NULL, 0, 0); > + intel_crtc_cursor_set_obj(plane->crtc, NULL, 0, 0); > + > + mutex_lock(&plane->dev->struct_mutex); > + i915_gem_track_fb(intel_fb_obj(plane->fb), NULL, > + INTEL_FRONTBUFFER_CURSOR(intel_plane->pipe)); > + mutex_unlock(&plane->dev->struct_mutex); > } > > static int > @@ -11838,7 +11691,6 @@ intel_check_cursor_plane(struct drm_plane *plane, > struct drm_rect *src = &state->src; > const struct drm_rect *clip = &state->clip; > struct drm_i915_gem_object *obj = intel_fb_obj(fb); > - int crtc_w, crtc_h; > unsigned stride; > int ret; > > @@ -11856,15 +11708,14 @@ intel_check_cursor_plane(struct drm_plane *plane, > return 0; > > /* Check for which cursor types we support */ > - crtc_w = drm_rect_width(&state->orig_dst); > - crtc_h = drm_rect_height(&state->orig_dst); > - if (!cursor_size_ok(dev, crtc_w, crtc_h)) { > - DRM_DEBUG("Cursor dimension not supported\n"); > + if (!cursor_size_ok(dev, state->base.crtc_w, state->base.crtc_h)) { > + DRM_DEBUG("Cursor dimension %dx%d not supported\n", > + state->base.crtc_w, state->base.crtc_h); > return -EINVAL; > } > > - stride = roundup_pow_of_two(crtc_w) * 4; > - if (obj->base.size < stride * crtc_h) { > + stride = roundup_pow_of_two(state->base.crtc_w) * 4; > + if (obj->base.size < stride * state->base.crtc_h) { > DRM_DEBUG_KMS("buffer is too small\n"); > return -ENOMEM; > } > @@ -11883,7 +11734,7 @@ intel_check_cursor_plane(struct drm_plane *plane, > return ret; > } > > -static int > +static void > intel_commit_cursor_plane(struct drm_plane *plane, > struct intel_plane_state *state) > { > @@ -11893,10 +11744,9 @@ intel_commit_cursor_plane(struct drm_plane *plane, > struct intel_plane *intel_plane = to_intel_plane(plane); > struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb); > struct drm_i915_gem_object *obj = intel_fb->obj; > - int crtc_w, crtc_h; > > - crtc->cursor_x = state->orig_dst.x1; > - crtc->cursor_y = state->orig_dst.y1; > + crtc->cursor_x = state->base.crtc_x; > + crtc->cursor_y = state->base.crtc_y; > > intel_plane->crtc_x = state->orig_dst.x1; > intel_plane->crtc_y = state->orig_dst.y1; > @@ -11909,63 +11759,20 @@ intel_commit_cursor_plane(struct drm_plane *plane, > intel_plane->obj = obj; > > if (fb != crtc->cursor->fb) { > - crtc_w = drm_rect_width(&state->orig_dst); > - crtc_h = drm_rect_height(&state->orig_dst); > - return intel_crtc_cursor_set_obj(crtc, obj, crtc_w, crtc_h); > + intel_crtc_cursor_set_obj(crtc, obj, > + state->base.crtc_w, > + state->base.crtc_h); > } else { > intel_crtc_update_cursor(crtc, state->visible); > > intel_frontbuffer_flip(crtc->dev, > INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe)); > - > - return 0; > } > } > > -static int > -intel_cursor_plane_update(struct drm_plane *plane, struct drm_crtc *crtc, > - struct drm_framebuffer *fb, int crtc_x, int crtc_y, > - unsigned int crtc_w, unsigned int crtc_h, > - uint32_t src_x, uint32_t src_y, > - uint32_t src_w, uint32_t src_h) > -{ > - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > - struct intel_plane_state state; > - int ret; > - > - state.base.crtc = crtc; > - state.base.fb = fb; > - > - /* sample coordinates in 16.16 fixed point */ > - state.src.x1 = src_x; > - state.src.x2 = src_x + src_w; > - state.src.y1 = src_y; > - state.src.y2 = src_y + src_h; > - > - /* integer pixels */ > - state.dst.x1 = crtc_x; > - state.dst.x2 = crtc_x + crtc_w; > - state.dst.y1 = crtc_y; > - state.dst.y2 = crtc_y + crtc_h; > - > - state.clip.x1 = 0; > - state.clip.y1 = 0; > - state.clip.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0; > - state.clip.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0; > - > - state.orig_src = state.src; > - state.orig_dst = state.dst; > - > - ret = intel_check_cursor_plane(plane, &state); > - if (ret) > - return ret; > - > - return intel_commit_cursor_plane(plane, &state); > -} > - > static const struct drm_plane_funcs intel_cursor_plane_funcs = { > - .update_plane = intel_cursor_plane_update, > - .disable_plane = intel_cursor_plane_disable, > + .update_plane = drm_plane_helper_update, > + .disable_plane = drm_plane_helper_disable, > .destroy = intel_plane_destroy, > .set_property = intel_plane_set_property, > .atomic_duplicate_state = intel_plane_duplicate_state, > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > index d3677c3..28ccaf7 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -1106,7 +1106,6 @@ intel_check_sprite_plane(struct drm_plane *plane, > uint32_t src_x, src_y, src_w, src_h; > struct drm_rect *src = &state->src; > struct drm_rect *dst = &state->dst; > - struct drm_rect *orig_src = &state->orig_src; > const struct drm_rect *clip = &state->clip; > int hscale, vscale; > int max_scale, min_scale; > @@ -1187,10 +1186,10 @@ intel_check_sprite_plane(struct drm_plane *plane, > intel_plane->rotation); > > /* sanity check to make sure the src viewport wasn't enlarged */ > - WARN_ON(src->x1 < (int) orig_src->x1 || > - src->y1 < (int) orig_src->y1 || > - src->x2 > (int) orig_src->x2 || > - src->y2 > (int) orig_src->y2); > + WARN_ON(src->x1 < (int) state->base.src_x || > + src->y1 < (int) state->base.src_y || > + src->x2 > (int) state->base.src_x + state->base.src_w || > + src->y2 > (int) state->base.src_y + state->base.src_h); > > /* > * Hardware doesn't handle subpixel coordinates. > @@ -1258,41 +1257,6 @@ intel_check_sprite_plane(struct drm_plane *plane, > return 0; > } > > -static int > -intel_prepare_sprite_plane(struct drm_plane *plane, > - struct intel_plane_state *state) > -{ > - struct drm_device *dev = plane->dev; > - struct drm_crtc *crtc = state->base.crtc; > - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > - struct intel_plane *intel_plane = to_intel_plane(plane); > - enum pipe pipe = intel_crtc->pipe; > - struct drm_framebuffer *fb = state->base.fb; > - struct drm_i915_gem_object *obj = intel_fb_obj(fb); > - struct drm_i915_gem_object *old_obj = intel_plane->obj; > - int ret; > - > - if (old_obj != obj) { > - mutex_lock(&dev->struct_mutex); > - > - /* Note that this will apply the VT-d workaround for scanouts, > - * which is more restrictive than required for sprites. (The > - * primary plane requires 256KiB alignment with 64 PTE padding, > - * the sprite planes only require 128KiB alignment and 32 PTE > - * padding. > - */ > - ret = intel_pin_and_fence_fb_obj(plane, fb, NULL); > - if (ret == 0) > - i915_gem_track_fb(old_obj, obj, > - INTEL_FRONTBUFFER_SPRITE(pipe)); > - mutex_unlock(&dev->struct_mutex); > - if (ret) > - return ret; > - } > - > - return 0; > -} > - > static void > intel_commit_sprite_plane(struct drm_plane *plane, > struct intel_plane_state *state) > @@ -1304,7 +1268,6 @@ intel_commit_sprite_plane(struct drm_plane *plane, > enum pipe pipe = intel_crtc->pipe; > struct drm_framebuffer *fb = state->base.fb; > struct drm_i915_gem_object *obj = intel_fb_obj(fb); > - struct drm_i915_gem_object *old_obj = intel_plane->obj; > int crtc_x, crtc_y; > unsigned int crtc_w, crtc_h; > uint32_t src_x, src_y, src_w, src_h; > @@ -1362,68 +1325,6 @@ intel_commit_sprite_plane(struct drm_plane *plane, > if (!primary_was_enabled && primary_enabled) > intel_post_enable_primary(crtc); > } > - > - /* Unpin old obj after new one is active to avoid ugliness */ > - if (old_obj && old_obj != obj) { > - > - /* > - * It's fairly common to simply update the position of > - * an existing object. In that case, we don't need to > - * wait for vblank to avoid ugliness, we only need to > - * do the pin & ref bookkeeping. > - */ > - if (intel_crtc->active) > - intel_wait_for_vblank(dev, intel_crtc->pipe); > - > - mutex_lock(&dev->struct_mutex); > - intel_unpin_fb_obj(old_obj); > - mutex_unlock(&dev->struct_mutex); > - } > -} > - > -static int > -intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, > - struct drm_framebuffer *fb, int crtc_x, int crtc_y, > - unsigned int crtc_w, unsigned int crtc_h, > - uint32_t src_x, uint32_t src_y, > - uint32_t src_w, uint32_t src_h) > -{ > - struct intel_plane_state state; > - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > - int ret; > - > - state.base.crtc = crtc; > - state.base.fb = fb; > - > - /* sample coordinates in 16.16 fixed point */ > - state.src.x1 = src_x; > - state.src.x2 = src_x + src_w; > - state.src.y1 = src_y; > - state.src.y2 = src_y + src_h; > - > - /* integer pixels */ > - state.dst.x1 = crtc_x; > - state.dst.x2 = crtc_x + crtc_w; > - state.dst.y1 = crtc_y; > - state.dst.y2 = crtc_y + crtc_h; > - > - state.clip.x1 = 0; > - state.clip.y1 = 0; > - state.clip.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0; > - state.clip.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0; > - state.orig_src = state.src; > - state.orig_dst = state.dst; > - > - ret = intel_check_sprite_plane(plane, &state); > - if (ret) > - return ret; > - > - ret = intel_prepare_sprite_plane(plane, &state); > - if (ret) > - return ret; > - > - intel_commit_sprite_plane(plane, &state); > - return 0; > } > > int > @@ -1459,7 +1360,6 @@ intel_sprite_plane_disable(struct drm_plane *plane) > intel_wait_for_vblank(dev, intel_plane->pipe); > > mutex_lock(&dev->struct_mutex); > - intel_unpin_fb_obj(intel_plane->obj); > i915_gem_track_fb(intel_plane->obj, NULL, > INTEL_FRONTBUFFER_SPRITE(pipe)); > mutex_unlock(&dev->struct_mutex); > @@ -1557,21 +1457,19 @@ int intel_plane_set_property(struct drm_plane *plane, > > int intel_plane_restore(struct drm_plane *plane) > { > - struct intel_plane *intel_plane = to_intel_plane(plane); > - > if (!plane->crtc || !plane->fb) > return 0; > > return plane->funcs->update_plane(plane, plane->crtc, plane->fb, > - intel_plane->crtc_x, intel_plane->crtc_y, > - intel_plane->crtc_w, intel_plane->crtc_h, > - intel_plane->src_x, intel_plane->src_y, > - intel_plane->src_w, intel_plane->src_h); > + plane->state->crtc_x, plane->state->crtc_y, > + plane->state->crtc_w, plane->state->crtc_h, > + plane->state->src_x, plane->state->src_y, > + plane->state->src_w, plane->state->src_h); > } > > static const struct drm_plane_funcs intel_plane_funcs = { > - .update_plane = intel_update_plane, > - .disable_plane = intel_sprite_plane_disable, > + .update_plane = drm_plane_helper_update, > + .disable_plane = drm_plane_helper_disable, > .destroy = intel_plane_destroy, > .set_property = intel_plane_set_property, > .atomic_duplicate_state = intel_plane_duplicate_state,
On Thu, Nov 13, 2014 at 10:43:25AM -0800, Matt Roper wrote: > Now that we have hooks to enable the atomic plane helpers, we can use > the plane helpers for our .update_plane() and .disable_plane() > entrypoints. > > Even though we'd already refactored our plane handling code into > check/commit, we still have to rework some of that logic here as we make > the transition. The atomic plane helpers now call prepare_fb/cleanup_fb > for us, so note the following: > - 'commit' should not longer unpin the old object; the cleanup_fb will > take care of this for us > - prepare_fb only gets called when we actually have an fb, so the > internal 'disable' code needs to update frontbuffer tracking > - the cursor plane code never had 'prepare' code separated from its > 'commit' function, so a lot of the cursor's 'commit' needs to be > dropped since prepare_fb already does it for us > - with all the pinning work removed from cursor commit, it can no > longer fail and should return void rather than int > > Testcase: igt/kms_plane > Testcase: igt/kms_universal_plane > Testcase: igt/kms_cursor_crc > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 255 +++++------------------------------ > drivers/gpu/drm/i915/intel_sprite.c | 122 ++--------------- > 2 files changed, 41 insertions(+), 336 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 32a2b12..8d5a3b4 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -8285,17 +8285,15 @@ static bool cursor_size_ok(struct drm_device *dev, > return true; > } > > -static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc, > - struct drm_i915_gem_object *obj, > - uint32_t width, uint32_t height) > +static void intel_crtc_cursor_set_obj(struct drm_crtc *crtc, > + struct drm_i915_gem_object *obj, > + uint32_t width, uint32_t height) > { > struct drm_device *dev = crtc->dev; > - struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > enum pipe pipe = intel_crtc->pipe; > unsigned old_width; > uint32_t addr; > - int ret; > > /* if we want to turn off the cursor ignore width and height */ > if (!obj) { > @@ -8305,64 +8303,14 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc, > goto finish; > } > > - /* we only need to pin inside GTT if cursor is non-phy */ > + /* plane helper's prepare_fb() already handled pinning */ > mutex_lock(&dev->struct_mutex); > - if (!INTEL_INFO(dev)->cursor_needs_physical) { > - unsigned alignment; > - > - /* > - * Global gtt pte registers are special registers which actually > - * forward writes to a chunk of system memory. Which means that > - * there is no risk that the register values disappear as soon > - * as we call intel_runtime_pm_put(), so it is correct to wrap > - * only the pin/unpin/fence and not more. > - */ > - intel_runtime_pm_get(dev_priv); > - > - /* Note that the w/a also requires 2 PTE of padding following > - * the bo. We currently fill all unused PTE with the shadow > - * page and so we should always have valid PTE following the > - * cursor preventing the VT-d warning. > - */ > - alignment = 0; > - if (need_vtd_wa(dev)) > - alignment = 64*1024; > - > - ret = i915_gem_object_pin_to_display_plane(obj, alignment, NULL); > - if (ret) { > - DRM_DEBUG_KMS("failed to move cursor bo into the GTT\n"); > - intel_runtime_pm_put(dev_priv); > - goto fail_locked; > - } > - > - ret = i915_gem_object_put_fence(obj); > - if (ret) { > - DRM_DEBUG_KMS("failed to release fence for cursor"); > - intel_runtime_pm_put(dev_priv); > - goto fail_unpin; > - } > - So I'm now wondering where all this code is disappearing into? Anyway this conflicts pretty harshly with Gutavo's pending patches. I think at least the particular patch touching this had the r-b already (apart from a stale bo unre left behind, which can easily be fixed up when applying the patch). So I would like to get Gustavo's patches in first, especially as they move the code already to the right direction. We also have bugs in the current code that need to fixed. Gustavo's patches came close, but not quite there IIRC. So I think it would be nice to get that stuff done first, and then toss this atomic stuff on top. > + if (!INTEL_INFO(dev)->cursor_needs_physical) > addr = i915_gem_obj_ggtt_offset(obj); > - > - intel_runtime_pm_put(dev_priv); > - } else { > - int align = IS_I830(dev) ? 16 * 1024 : 256; > - ret = i915_gem_object_attach_phys(obj, align); > - if (ret) { > - DRM_DEBUG_KMS("failed to attach phys object\n"); > - goto fail_locked; > - } > + else > addr = obj->phys_handle->busaddr; > - } > > finish: > - if (intel_crtc->cursor_bo) { > - if (!INTEL_INFO(dev)->cursor_needs_physical) > - i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo); > - } > - > - i915_gem_track_fb(intel_crtc->cursor_bo, obj, > - INTEL_FRONTBUFFER_CURSOR(pipe)); > mutex_unlock(&dev->struct_mutex); > > old_width = intel_crtc->cursor_width; > @@ -8379,13 +8327,6 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc, > > intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe)); > } > - > - return 0; > -fail_unpin: > - i915_gem_object_unpin_from_display_plane(obj); > -fail_locked: > - mutex_unlock(&dev->struct_mutex); > - return ret; > } > > static void intel_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16 *green, > @@ -11544,7 +11485,6 @@ disable_unpin: > mutex_lock(&dev->struct_mutex); > i915_gem_track_fb(intel_fb_obj(plane->fb), NULL, > INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe)); > - intel_unpin_fb_obj(intel_fb_obj(plane->fb)); > mutex_unlock(&dev->struct_mutex); > plane->fb = NULL; > > @@ -11568,42 +11508,6 @@ intel_check_primary_plane(struct drm_plane *plane, > false, true, &state->visible); > } > > -static int > -intel_prepare_primary_plane(struct drm_plane *plane, > - struct intel_plane_state *state) > -{ > - struct drm_crtc *crtc = state->base.crtc; > - struct drm_framebuffer *fb = state->base.fb; > - struct drm_device *dev = crtc->dev; > - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > - enum pipe pipe = intel_crtc->pipe; > - struct drm_i915_gem_object *obj = intel_fb_obj(fb); > - struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb); > - int ret; > - > - intel_crtc_wait_for_pending_flips(crtc); > - > - if (intel_crtc_has_pending_flip(crtc)) { > - DRM_ERROR("pipe is still busy with an old pageflip\n"); > - return -EBUSY; > - } > - > - if (old_obj != obj) { > - mutex_lock(&dev->struct_mutex); > - ret = intel_pin_and_fence_fb_obj(plane, fb, NULL); > - if (ret == 0) > - i915_gem_track_fb(old_obj, obj, > - INTEL_FRONTBUFFER_PRIMARY(pipe)); > - mutex_unlock(&dev->struct_mutex); > - if (ret != 0) { > - DRM_DEBUG_KMS("pin & fence failed\n"); > - return ret; > - } > - } > - > - return 0; > -} > - > static void > intel_commit_primary_plane(struct drm_plane *plane, > struct intel_plane_state *state) > @@ -11614,9 +11518,7 @@ intel_commit_primary_plane(struct drm_plane *plane, > struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > enum pipe pipe = intel_crtc->pipe; > - struct drm_framebuffer *old_fb = plane->fb; > struct drm_i915_gem_object *obj = intel_fb_obj(fb); > - struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb); > struct intel_plane *intel_plane = to_intel_plane(plane); > struct drm_rect *src = &state->src; > > @@ -11687,62 +11589,6 @@ intel_commit_primary_plane(struct drm_plane *plane, > intel_update_fbc(dev); > mutex_unlock(&dev->struct_mutex); > } > - > - if (old_fb && old_fb != fb) { > - if (intel_crtc->active) > - intel_wait_for_vblank(dev, intel_crtc->pipe); > - > - mutex_lock(&dev->struct_mutex); > - intel_unpin_fb_obj(old_obj); > - mutex_unlock(&dev->struct_mutex); > - } > -} > - > -static int > -intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc, > - struct drm_framebuffer *fb, int crtc_x, int crtc_y, > - unsigned int crtc_w, unsigned int crtc_h, > - uint32_t src_x, uint32_t src_y, > - uint32_t src_w, uint32_t src_h) > -{ > - struct intel_plane_state state; > - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > - int ret; > - > - state.base.crtc = crtc; > - state.base.fb = fb; > - > - /* sample coordinates in 16.16 fixed point */ > - state.src.x1 = src_x; > - state.src.x2 = src_x + src_w; > - state.src.y1 = src_y; > - state.src.y2 = src_y + src_h; > - > - /* integer pixels */ > - state.dst.x1 = crtc_x; > - state.dst.x2 = crtc_x + crtc_w; > - state.dst.y1 = crtc_y; > - state.dst.y2 = crtc_y + crtc_h; > - > - state.clip.x1 = 0; > - state.clip.y1 = 0; > - state.clip.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0; > - state.clip.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0; > - > - state.orig_src = state.src; > - state.orig_dst = state.dst; > - > - ret = intel_check_primary_plane(plane, &state); > - if (ret) > - return ret; > - > - ret = intel_prepare_primary_plane(plane, &state); > - if (ret) > - return ret; > - > - intel_commit_primary_plane(plane, &state); > - > - return 0; > } > > /* Common destruction function for both primary and cursor planes */ > @@ -11755,8 +11601,8 @@ void intel_plane_destroy(struct drm_plane *plane) > } > > static const struct drm_plane_funcs intel_primary_plane_funcs = { > - .update_plane = intel_primary_plane_setplane, > - .disable_plane = intel_primary_plane_disable, > + .update_plane = drm_plane_helper_update, > + .disable_plane = drm_plane_helper_disable, > .destroy = intel_plane_destroy, > .set_property = intel_plane_set_property, > .atomic_duplicate_state = intel_plane_duplicate_state, > @@ -11816,15 +11662,22 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev, > return &primary->base; > } > > -static int > +static void > intel_cursor_plane_disable(struct drm_plane *plane) > { > + struct intel_plane *intel_plane = to_intel_plane(plane); > + > if (!plane->fb) > - return 0; > + return; > > BUG_ON(!plane->crtc); > > - return intel_crtc_cursor_set_obj(plane->crtc, NULL, 0, 0); > + intel_crtc_cursor_set_obj(plane->crtc, NULL, 0, 0); > + > + mutex_lock(&plane->dev->struct_mutex); > + i915_gem_track_fb(intel_fb_obj(plane->fb), NULL, > + INTEL_FRONTBUFFER_CURSOR(intel_plane->pipe)); > + mutex_unlock(&plane->dev->struct_mutex); > } > > static int > @@ -11838,7 +11691,6 @@ intel_check_cursor_plane(struct drm_plane *plane, > struct drm_rect *src = &state->src; > const struct drm_rect *clip = &state->clip; > struct drm_i915_gem_object *obj = intel_fb_obj(fb); > - int crtc_w, crtc_h; > unsigned stride; > int ret; > > @@ -11856,15 +11708,14 @@ intel_check_cursor_plane(struct drm_plane *plane, > return 0; > > /* Check for which cursor types we support */ > - crtc_w = drm_rect_width(&state->orig_dst); > - crtc_h = drm_rect_height(&state->orig_dst); > - if (!cursor_size_ok(dev, crtc_w, crtc_h)) { > - DRM_DEBUG("Cursor dimension not supported\n"); > + if (!cursor_size_ok(dev, state->base.crtc_w, state->base.crtc_h)) { > + DRM_DEBUG("Cursor dimension %dx%d not supported\n", > + state->base.crtc_w, state->base.crtc_h); > return -EINVAL; > } > > - stride = roundup_pow_of_two(crtc_w) * 4; > - if (obj->base.size < stride * crtc_h) { > + stride = roundup_pow_of_two(state->base.crtc_w) * 4; > + if (obj->base.size < stride * state->base.crtc_h) { > DRM_DEBUG_KMS("buffer is too small\n"); > return -ENOMEM; > } > @@ -11883,7 +11734,7 @@ intel_check_cursor_plane(struct drm_plane *plane, > return ret; > } > > -static int > +static void > intel_commit_cursor_plane(struct drm_plane *plane, > struct intel_plane_state *state) > { > @@ -11893,10 +11744,9 @@ intel_commit_cursor_plane(struct drm_plane *plane, > struct intel_plane *intel_plane = to_intel_plane(plane); > struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb); > struct drm_i915_gem_object *obj = intel_fb->obj; > - int crtc_w, crtc_h; > > - crtc->cursor_x = state->orig_dst.x1; > - crtc->cursor_y = state->orig_dst.y1; > + crtc->cursor_x = state->base.crtc_x; > + crtc->cursor_y = state->base.crtc_y; > > intel_plane->crtc_x = state->orig_dst.x1; > intel_plane->crtc_y = state->orig_dst.y1; > @@ -11909,63 +11759,20 @@ intel_commit_cursor_plane(struct drm_plane *plane, > intel_plane->obj = obj; > > if (fb != crtc->cursor->fb) { > - crtc_w = drm_rect_width(&state->orig_dst); > - crtc_h = drm_rect_height(&state->orig_dst); > - return intel_crtc_cursor_set_obj(crtc, obj, crtc_w, crtc_h); > + intel_crtc_cursor_set_obj(crtc, obj, > + state->base.crtc_w, > + state->base.crtc_h); > } else { > intel_crtc_update_cursor(crtc, state->visible); > > intel_frontbuffer_flip(crtc->dev, > INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe)); > - > - return 0; > } > } > > -static int > -intel_cursor_plane_update(struct drm_plane *plane, struct drm_crtc *crtc, > - struct drm_framebuffer *fb, int crtc_x, int crtc_y, > - unsigned int crtc_w, unsigned int crtc_h, > - uint32_t src_x, uint32_t src_y, > - uint32_t src_w, uint32_t src_h) > -{ > - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > - struct intel_plane_state state; > - int ret; > - > - state.base.crtc = crtc; > - state.base.fb = fb; > - > - /* sample coordinates in 16.16 fixed point */ > - state.src.x1 = src_x; > - state.src.x2 = src_x + src_w; > - state.src.y1 = src_y; > - state.src.y2 = src_y + src_h; > - > - /* integer pixels */ > - state.dst.x1 = crtc_x; > - state.dst.x2 = crtc_x + crtc_w; > - state.dst.y1 = crtc_y; > - state.dst.y2 = crtc_y + crtc_h; > - > - state.clip.x1 = 0; > - state.clip.y1 = 0; > - state.clip.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0; > - state.clip.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0; > - > - state.orig_src = state.src; > - state.orig_dst = state.dst; > - > - ret = intel_check_cursor_plane(plane, &state); > - if (ret) > - return ret; > - > - return intel_commit_cursor_plane(plane, &state); > -} > - > static const struct drm_plane_funcs intel_cursor_plane_funcs = { > - .update_plane = intel_cursor_plane_update, > - .disable_plane = intel_cursor_plane_disable, > + .update_plane = drm_plane_helper_update, > + .disable_plane = drm_plane_helper_disable, > .destroy = intel_plane_destroy, > .set_property = intel_plane_set_property, > .atomic_duplicate_state = intel_plane_duplicate_state, > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > index d3677c3..28ccaf7 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -1106,7 +1106,6 @@ intel_check_sprite_plane(struct drm_plane *plane, > uint32_t src_x, src_y, src_w, src_h; > struct drm_rect *src = &state->src; > struct drm_rect *dst = &state->dst; > - struct drm_rect *orig_src = &state->orig_src; > const struct drm_rect *clip = &state->clip; > int hscale, vscale; > int max_scale, min_scale; > @@ -1187,10 +1186,10 @@ intel_check_sprite_plane(struct drm_plane *plane, > intel_plane->rotation); > > /* sanity check to make sure the src viewport wasn't enlarged */ > - WARN_ON(src->x1 < (int) orig_src->x1 || > - src->y1 < (int) orig_src->y1 || > - src->x2 > (int) orig_src->x2 || > - src->y2 > (int) orig_src->y2); > + WARN_ON(src->x1 < (int) state->base.src_x || > + src->y1 < (int) state->base.src_y || > + src->x2 > (int) state->base.src_x + state->base.src_w || > + src->y2 > (int) state->base.src_y + state->base.src_h); > > /* > * Hardware doesn't handle subpixel coordinates. > @@ -1258,41 +1257,6 @@ intel_check_sprite_plane(struct drm_plane *plane, > return 0; > } > > -static int > -intel_prepare_sprite_plane(struct drm_plane *plane, > - struct intel_plane_state *state) > -{ > - struct drm_device *dev = plane->dev; > - struct drm_crtc *crtc = state->base.crtc; > - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > - struct intel_plane *intel_plane = to_intel_plane(plane); > - enum pipe pipe = intel_crtc->pipe; > - struct drm_framebuffer *fb = state->base.fb; > - struct drm_i915_gem_object *obj = intel_fb_obj(fb); > - struct drm_i915_gem_object *old_obj = intel_plane->obj; > - int ret; > - > - if (old_obj != obj) { > - mutex_lock(&dev->struct_mutex); > - > - /* Note that this will apply the VT-d workaround for scanouts, > - * which is more restrictive than required for sprites. (The > - * primary plane requires 256KiB alignment with 64 PTE padding, > - * the sprite planes only require 128KiB alignment and 32 PTE > - * padding. > - */ > - ret = intel_pin_and_fence_fb_obj(plane, fb, NULL); > - if (ret == 0) > - i915_gem_track_fb(old_obj, obj, > - INTEL_FRONTBUFFER_SPRITE(pipe)); > - mutex_unlock(&dev->struct_mutex); > - if (ret) > - return ret; > - } > - > - return 0; > -} > - > static void > intel_commit_sprite_plane(struct drm_plane *plane, > struct intel_plane_state *state) > @@ -1304,7 +1268,6 @@ intel_commit_sprite_plane(struct drm_plane *plane, > enum pipe pipe = intel_crtc->pipe; > struct drm_framebuffer *fb = state->base.fb; > struct drm_i915_gem_object *obj = intel_fb_obj(fb); > - struct drm_i915_gem_object *old_obj = intel_plane->obj; > int crtc_x, crtc_y; > unsigned int crtc_w, crtc_h; > uint32_t src_x, src_y, src_w, src_h; > @@ -1362,68 +1325,6 @@ intel_commit_sprite_plane(struct drm_plane *plane, > if (!primary_was_enabled && primary_enabled) > intel_post_enable_primary(crtc); > } > - > - /* Unpin old obj after new one is active to avoid ugliness */ > - if (old_obj && old_obj != obj) { > - > - /* > - * It's fairly common to simply update the position of > - * an existing object. In that case, we don't need to > - * wait for vblank to avoid ugliness, we only need to > - * do the pin & ref bookkeeping. > - */ > - if (intel_crtc->active) > - intel_wait_for_vblank(dev, intel_crtc->pipe); > - > - mutex_lock(&dev->struct_mutex); > - intel_unpin_fb_obj(old_obj); > - mutex_unlock(&dev->struct_mutex); > - } > -} > - > -static int > -intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, > - struct drm_framebuffer *fb, int crtc_x, int crtc_y, > - unsigned int crtc_w, unsigned int crtc_h, > - uint32_t src_x, uint32_t src_y, > - uint32_t src_w, uint32_t src_h) > -{ > - struct intel_plane_state state; > - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > - int ret; > - > - state.base.crtc = crtc; > - state.base.fb = fb; > - > - /* sample coordinates in 16.16 fixed point */ > - state.src.x1 = src_x; > - state.src.x2 = src_x + src_w; > - state.src.y1 = src_y; > - state.src.y2 = src_y + src_h; > - > - /* integer pixels */ > - state.dst.x1 = crtc_x; > - state.dst.x2 = crtc_x + crtc_w; > - state.dst.y1 = crtc_y; > - state.dst.y2 = crtc_y + crtc_h; > - > - state.clip.x1 = 0; > - state.clip.y1 = 0; > - state.clip.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0; > - state.clip.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0; > - state.orig_src = state.src; > - state.orig_dst = state.dst; > - > - ret = intel_check_sprite_plane(plane, &state); > - if (ret) > - return ret; > - > - ret = intel_prepare_sprite_plane(plane, &state); > - if (ret) > - return ret; > - > - intel_commit_sprite_plane(plane, &state); > - return 0; > } > > int > @@ -1459,7 +1360,6 @@ intel_sprite_plane_disable(struct drm_plane *plane) > intel_wait_for_vblank(dev, intel_plane->pipe); > > mutex_lock(&dev->struct_mutex); > - intel_unpin_fb_obj(intel_plane->obj); > i915_gem_track_fb(intel_plane->obj, NULL, > INTEL_FRONTBUFFER_SPRITE(pipe)); > mutex_unlock(&dev->struct_mutex); > @@ -1557,21 +1457,19 @@ int intel_plane_set_property(struct drm_plane *plane, > > int intel_plane_restore(struct drm_plane *plane) > { > - struct intel_plane *intel_plane = to_intel_plane(plane); > - > if (!plane->crtc || !plane->fb) > return 0; > > return plane->funcs->update_plane(plane, plane->crtc, plane->fb, > - intel_plane->crtc_x, intel_plane->crtc_y, > - intel_plane->crtc_w, intel_plane->crtc_h, > - intel_plane->src_x, intel_plane->src_y, > - intel_plane->src_w, intel_plane->src_h); > + plane->state->crtc_x, plane->state->crtc_y, > + plane->state->crtc_w, plane->state->crtc_h, > + plane->state->src_x, plane->state->src_y, > + plane->state->src_w, plane->state->src_h); > } > > static const struct drm_plane_funcs intel_plane_funcs = { > - .update_plane = intel_update_plane, > - .disable_plane = intel_sprite_plane_disable, > + .update_plane = drm_plane_helper_update, > + .disable_plane = drm_plane_helper_disable, > .destroy = intel_plane_destroy, > .set_property = intel_plane_set_property, > .atomic_duplicate_state = intel_plane_duplicate_state, > -- > 1.8.5.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 32a2b12..8d5a3b4 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -8285,17 +8285,15 @@ static bool cursor_size_ok(struct drm_device *dev, return true; } -static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc, - struct drm_i915_gem_object *obj, - uint32_t width, uint32_t height) +static void intel_crtc_cursor_set_obj(struct drm_crtc *crtc, + struct drm_i915_gem_object *obj, + uint32_t width, uint32_t height) { struct drm_device *dev = crtc->dev; - struct drm_i915_private *dev_priv = dev->dev_private; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); enum pipe pipe = intel_crtc->pipe; unsigned old_width; uint32_t addr; - int ret; /* if we want to turn off the cursor ignore width and height */ if (!obj) { @@ -8305,64 +8303,14 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc, goto finish; } - /* we only need to pin inside GTT if cursor is non-phy */ + /* plane helper's prepare_fb() already handled pinning */ mutex_lock(&dev->struct_mutex); - if (!INTEL_INFO(dev)->cursor_needs_physical) { - unsigned alignment; - - /* - * Global gtt pte registers are special registers which actually - * forward writes to a chunk of system memory. Which means that - * there is no risk that the register values disappear as soon - * as we call intel_runtime_pm_put(), so it is correct to wrap - * only the pin/unpin/fence and not more. - */ - intel_runtime_pm_get(dev_priv); - - /* Note that the w/a also requires 2 PTE of padding following - * the bo. We currently fill all unused PTE with the shadow - * page and so we should always have valid PTE following the - * cursor preventing the VT-d warning. - */ - alignment = 0; - if (need_vtd_wa(dev)) - alignment = 64*1024; - - ret = i915_gem_object_pin_to_display_plane(obj, alignment, NULL); - if (ret) { - DRM_DEBUG_KMS("failed to move cursor bo into the GTT\n"); - intel_runtime_pm_put(dev_priv); - goto fail_locked; - } - - ret = i915_gem_object_put_fence(obj); - if (ret) { - DRM_DEBUG_KMS("failed to release fence for cursor"); - intel_runtime_pm_put(dev_priv); - goto fail_unpin; - } - + if (!INTEL_INFO(dev)->cursor_needs_physical) addr = i915_gem_obj_ggtt_offset(obj); - - intel_runtime_pm_put(dev_priv); - } else { - int align = IS_I830(dev) ? 16 * 1024 : 256; - ret = i915_gem_object_attach_phys(obj, align); - if (ret) { - DRM_DEBUG_KMS("failed to attach phys object\n"); - goto fail_locked; - } + else addr = obj->phys_handle->busaddr; - } finish: - if (intel_crtc->cursor_bo) { - if (!INTEL_INFO(dev)->cursor_needs_physical) - i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo); - } - - i915_gem_track_fb(intel_crtc->cursor_bo, obj, - INTEL_FRONTBUFFER_CURSOR(pipe)); mutex_unlock(&dev->struct_mutex); old_width = intel_crtc->cursor_width; @@ -8379,13 +8327,6 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc, intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe)); } - - return 0; -fail_unpin: - i915_gem_object_unpin_from_display_plane(obj); -fail_locked: - mutex_unlock(&dev->struct_mutex); - return ret; } static void intel_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16 *green, @@ -11544,7 +11485,6 @@ disable_unpin: mutex_lock(&dev->struct_mutex); i915_gem_track_fb(intel_fb_obj(plane->fb), NULL, INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe)); - intel_unpin_fb_obj(intel_fb_obj(plane->fb)); mutex_unlock(&dev->struct_mutex); plane->fb = NULL; @@ -11568,42 +11508,6 @@ intel_check_primary_plane(struct drm_plane *plane, false, true, &state->visible); } -static int -intel_prepare_primary_plane(struct drm_plane *plane, - struct intel_plane_state *state) -{ - struct drm_crtc *crtc = state->base.crtc; - struct drm_framebuffer *fb = state->base.fb; - struct drm_device *dev = crtc->dev; - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); - enum pipe pipe = intel_crtc->pipe; - struct drm_i915_gem_object *obj = intel_fb_obj(fb); - struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb); - int ret; - - intel_crtc_wait_for_pending_flips(crtc); - - if (intel_crtc_has_pending_flip(crtc)) { - DRM_ERROR("pipe is still busy with an old pageflip\n"); - return -EBUSY; - } - - if (old_obj != obj) { - mutex_lock(&dev->struct_mutex); - ret = intel_pin_and_fence_fb_obj(plane, fb, NULL); - if (ret == 0) - i915_gem_track_fb(old_obj, obj, - INTEL_FRONTBUFFER_PRIMARY(pipe)); - mutex_unlock(&dev->struct_mutex); - if (ret != 0) { - DRM_DEBUG_KMS("pin & fence failed\n"); - return ret; - } - } - - return 0; -} - static void intel_commit_primary_plane(struct drm_plane *plane, struct intel_plane_state *state) @@ -11614,9 +11518,7 @@ intel_commit_primary_plane(struct drm_plane *plane, struct drm_i915_private *dev_priv = dev->dev_private; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); enum pipe pipe = intel_crtc->pipe; - struct drm_framebuffer *old_fb = plane->fb; struct drm_i915_gem_object *obj = intel_fb_obj(fb); - struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb); struct intel_plane *intel_plane = to_intel_plane(plane); struct drm_rect *src = &state->src; @@ -11687,62 +11589,6 @@ intel_commit_primary_plane(struct drm_plane *plane, intel_update_fbc(dev); mutex_unlock(&dev->struct_mutex); } - - if (old_fb && old_fb != fb) { - if (intel_crtc->active) - intel_wait_for_vblank(dev, intel_crtc->pipe); - - mutex_lock(&dev->struct_mutex); - intel_unpin_fb_obj(old_obj); - mutex_unlock(&dev->struct_mutex); - } -} - -static int -intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc, - struct drm_framebuffer *fb, int crtc_x, int crtc_y, - unsigned int crtc_w, unsigned int crtc_h, - uint32_t src_x, uint32_t src_y, - uint32_t src_w, uint32_t src_h) -{ - struct intel_plane_state state; - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); - int ret; - - state.base.crtc = crtc; - state.base.fb = fb; - - /* sample coordinates in 16.16 fixed point */ - state.src.x1 = src_x; - state.src.x2 = src_x + src_w; - state.src.y1 = src_y; - state.src.y2 = src_y + src_h; - - /* integer pixels */ - state.dst.x1 = crtc_x; - state.dst.x2 = crtc_x + crtc_w; - state.dst.y1 = crtc_y; - state.dst.y2 = crtc_y + crtc_h; - - state.clip.x1 = 0; - state.clip.y1 = 0; - state.clip.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0; - state.clip.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0; - - state.orig_src = state.src; - state.orig_dst = state.dst; - - ret = intel_check_primary_plane(plane, &state); - if (ret) - return ret; - - ret = intel_prepare_primary_plane(plane, &state); - if (ret) - return ret; - - intel_commit_primary_plane(plane, &state); - - return 0; } /* Common destruction function for both primary and cursor planes */ @@ -11755,8 +11601,8 @@ void intel_plane_destroy(struct drm_plane *plane) } static const struct drm_plane_funcs intel_primary_plane_funcs = { - .update_plane = intel_primary_plane_setplane, - .disable_plane = intel_primary_plane_disable, + .update_plane = drm_plane_helper_update, + .disable_plane = drm_plane_helper_disable, .destroy = intel_plane_destroy, .set_property = intel_plane_set_property, .atomic_duplicate_state = intel_plane_duplicate_state, @@ -11816,15 +11662,22 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev, return &primary->base; } -static int +static void intel_cursor_plane_disable(struct drm_plane *plane) { + struct intel_plane *intel_plane = to_intel_plane(plane); + if (!plane->fb) - return 0; + return; BUG_ON(!plane->crtc); - return intel_crtc_cursor_set_obj(plane->crtc, NULL, 0, 0); + intel_crtc_cursor_set_obj(plane->crtc, NULL, 0, 0); + + mutex_lock(&plane->dev->struct_mutex); + i915_gem_track_fb(intel_fb_obj(plane->fb), NULL, + INTEL_FRONTBUFFER_CURSOR(intel_plane->pipe)); + mutex_unlock(&plane->dev->struct_mutex); } static int @@ -11838,7 +11691,6 @@ intel_check_cursor_plane(struct drm_plane *plane, struct drm_rect *src = &state->src; const struct drm_rect *clip = &state->clip; struct drm_i915_gem_object *obj = intel_fb_obj(fb); - int crtc_w, crtc_h; unsigned stride; int ret; @@ -11856,15 +11708,14 @@ intel_check_cursor_plane(struct drm_plane *plane, return 0; /* Check for which cursor types we support */ - crtc_w = drm_rect_width(&state->orig_dst); - crtc_h = drm_rect_height(&state->orig_dst); - if (!cursor_size_ok(dev, crtc_w, crtc_h)) { - DRM_DEBUG("Cursor dimension not supported\n"); + if (!cursor_size_ok(dev, state->base.crtc_w, state->base.crtc_h)) { + DRM_DEBUG("Cursor dimension %dx%d not supported\n", + state->base.crtc_w, state->base.crtc_h); return -EINVAL; } - stride = roundup_pow_of_two(crtc_w) * 4; - if (obj->base.size < stride * crtc_h) { + stride = roundup_pow_of_two(state->base.crtc_w) * 4; + if (obj->base.size < stride * state->base.crtc_h) { DRM_DEBUG_KMS("buffer is too small\n"); return -ENOMEM; } @@ -11883,7 +11734,7 @@ intel_check_cursor_plane(struct drm_plane *plane, return ret; } -static int +static void intel_commit_cursor_plane(struct drm_plane *plane, struct intel_plane_state *state) { @@ -11893,10 +11744,9 @@ intel_commit_cursor_plane(struct drm_plane *plane, struct intel_plane *intel_plane = to_intel_plane(plane); struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb); struct drm_i915_gem_object *obj = intel_fb->obj; - int crtc_w, crtc_h; - crtc->cursor_x = state->orig_dst.x1; - crtc->cursor_y = state->orig_dst.y1; + crtc->cursor_x = state->base.crtc_x; + crtc->cursor_y = state->base.crtc_y; intel_plane->crtc_x = state->orig_dst.x1; intel_plane->crtc_y = state->orig_dst.y1; @@ -11909,63 +11759,20 @@ intel_commit_cursor_plane(struct drm_plane *plane, intel_plane->obj = obj; if (fb != crtc->cursor->fb) { - crtc_w = drm_rect_width(&state->orig_dst); - crtc_h = drm_rect_height(&state->orig_dst); - return intel_crtc_cursor_set_obj(crtc, obj, crtc_w, crtc_h); + intel_crtc_cursor_set_obj(crtc, obj, + state->base.crtc_w, + state->base.crtc_h); } else { intel_crtc_update_cursor(crtc, state->visible); intel_frontbuffer_flip(crtc->dev, INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe)); - - return 0; } } -static int -intel_cursor_plane_update(struct drm_plane *plane, struct drm_crtc *crtc, - struct drm_framebuffer *fb, int crtc_x, int crtc_y, - unsigned int crtc_w, unsigned int crtc_h, - uint32_t src_x, uint32_t src_y, - uint32_t src_w, uint32_t src_h) -{ - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); - struct intel_plane_state state; - int ret; - - state.base.crtc = crtc; - state.base.fb = fb; - - /* sample coordinates in 16.16 fixed point */ - state.src.x1 = src_x; - state.src.x2 = src_x + src_w; - state.src.y1 = src_y; - state.src.y2 = src_y + src_h; - - /* integer pixels */ - state.dst.x1 = crtc_x; - state.dst.x2 = crtc_x + crtc_w; - state.dst.y1 = crtc_y; - state.dst.y2 = crtc_y + crtc_h; - - state.clip.x1 = 0; - state.clip.y1 = 0; - state.clip.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0; - state.clip.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0; - - state.orig_src = state.src; - state.orig_dst = state.dst; - - ret = intel_check_cursor_plane(plane, &state); - if (ret) - return ret; - - return intel_commit_cursor_plane(plane, &state); -} - static const struct drm_plane_funcs intel_cursor_plane_funcs = { - .update_plane = intel_cursor_plane_update, - .disable_plane = intel_cursor_plane_disable, + .update_plane = drm_plane_helper_update, + .disable_plane = drm_plane_helper_disable, .destroy = intel_plane_destroy, .set_property = intel_plane_set_property, .atomic_duplicate_state = intel_plane_duplicate_state, diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index d3677c3..28ccaf7 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -1106,7 +1106,6 @@ intel_check_sprite_plane(struct drm_plane *plane, uint32_t src_x, src_y, src_w, src_h; struct drm_rect *src = &state->src; struct drm_rect *dst = &state->dst; - struct drm_rect *orig_src = &state->orig_src; const struct drm_rect *clip = &state->clip; int hscale, vscale; int max_scale, min_scale; @@ -1187,10 +1186,10 @@ intel_check_sprite_plane(struct drm_plane *plane, intel_plane->rotation); /* sanity check to make sure the src viewport wasn't enlarged */ - WARN_ON(src->x1 < (int) orig_src->x1 || - src->y1 < (int) orig_src->y1 || - src->x2 > (int) orig_src->x2 || - src->y2 > (int) orig_src->y2); + WARN_ON(src->x1 < (int) state->base.src_x || + src->y1 < (int) state->base.src_y || + src->x2 > (int) state->base.src_x + state->base.src_w || + src->y2 > (int) state->base.src_y + state->base.src_h); /* * Hardware doesn't handle subpixel coordinates. @@ -1258,41 +1257,6 @@ intel_check_sprite_plane(struct drm_plane *plane, return 0; } -static int -intel_prepare_sprite_plane(struct drm_plane *plane, - struct intel_plane_state *state) -{ - struct drm_device *dev = plane->dev; - struct drm_crtc *crtc = state->base.crtc; - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); - struct intel_plane *intel_plane = to_intel_plane(plane); - enum pipe pipe = intel_crtc->pipe; - struct drm_framebuffer *fb = state->base.fb; - struct drm_i915_gem_object *obj = intel_fb_obj(fb); - struct drm_i915_gem_object *old_obj = intel_plane->obj; - int ret; - - if (old_obj != obj) { - mutex_lock(&dev->struct_mutex); - - /* Note that this will apply the VT-d workaround for scanouts, - * which is more restrictive than required for sprites. (The - * primary plane requires 256KiB alignment with 64 PTE padding, - * the sprite planes only require 128KiB alignment and 32 PTE - * padding. - */ - ret = intel_pin_and_fence_fb_obj(plane, fb, NULL); - if (ret == 0) - i915_gem_track_fb(old_obj, obj, - INTEL_FRONTBUFFER_SPRITE(pipe)); - mutex_unlock(&dev->struct_mutex); - if (ret) - return ret; - } - - return 0; -} - static void intel_commit_sprite_plane(struct drm_plane *plane, struct intel_plane_state *state) @@ -1304,7 +1268,6 @@ intel_commit_sprite_plane(struct drm_plane *plane, enum pipe pipe = intel_crtc->pipe; struct drm_framebuffer *fb = state->base.fb; struct drm_i915_gem_object *obj = intel_fb_obj(fb); - struct drm_i915_gem_object *old_obj = intel_plane->obj; int crtc_x, crtc_y; unsigned int crtc_w, crtc_h; uint32_t src_x, src_y, src_w, src_h; @@ -1362,68 +1325,6 @@ intel_commit_sprite_plane(struct drm_plane *plane, if (!primary_was_enabled && primary_enabled) intel_post_enable_primary(crtc); } - - /* Unpin old obj after new one is active to avoid ugliness */ - if (old_obj && old_obj != obj) { - - /* - * It's fairly common to simply update the position of - * an existing object. In that case, we don't need to - * wait for vblank to avoid ugliness, we only need to - * do the pin & ref bookkeeping. - */ - if (intel_crtc->active) - intel_wait_for_vblank(dev, intel_crtc->pipe); - - mutex_lock(&dev->struct_mutex); - intel_unpin_fb_obj(old_obj); - mutex_unlock(&dev->struct_mutex); - } -} - -static int -intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, - struct drm_framebuffer *fb, int crtc_x, int crtc_y, - unsigned int crtc_w, unsigned int crtc_h, - uint32_t src_x, uint32_t src_y, - uint32_t src_w, uint32_t src_h) -{ - struct intel_plane_state state; - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); - int ret; - - state.base.crtc = crtc; - state.base.fb = fb; - - /* sample coordinates in 16.16 fixed point */ - state.src.x1 = src_x; - state.src.x2 = src_x + src_w; - state.src.y1 = src_y; - state.src.y2 = src_y + src_h; - - /* integer pixels */ - state.dst.x1 = crtc_x; - state.dst.x2 = crtc_x + crtc_w; - state.dst.y1 = crtc_y; - state.dst.y2 = crtc_y + crtc_h; - - state.clip.x1 = 0; - state.clip.y1 = 0; - state.clip.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0; - state.clip.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0; - state.orig_src = state.src; - state.orig_dst = state.dst; - - ret = intel_check_sprite_plane(plane, &state); - if (ret) - return ret; - - ret = intel_prepare_sprite_plane(plane, &state); - if (ret) - return ret; - - intel_commit_sprite_plane(plane, &state); - return 0; } int @@ -1459,7 +1360,6 @@ intel_sprite_plane_disable(struct drm_plane *plane) intel_wait_for_vblank(dev, intel_plane->pipe); mutex_lock(&dev->struct_mutex); - intel_unpin_fb_obj(intel_plane->obj); i915_gem_track_fb(intel_plane->obj, NULL, INTEL_FRONTBUFFER_SPRITE(pipe)); mutex_unlock(&dev->struct_mutex); @@ -1557,21 +1457,19 @@ int intel_plane_set_property(struct drm_plane *plane, int intel_plane_restore(struct drm_plane *plane) { - struct intel_plane *intel_plane = to_intel_plane(plane); - if (!plane->crtc || !plane->fb) return 0; return plane->funcs->update_plane(plane, plane->crtc, plane->fb, - intel_plane->crtc_x, intel_plane->crtc_y, - intel_plane->crtc_w, intel_plane->crtc_h, - intel_plane->src_x, intel_plane->src_y, - intel_plane->src_w, intel_plane->src_h); + plane->state->crtc_x, plane->state->crtc_y, + plane->state->crtc_w, plane->state->crtc_h, + plane->state->src_x, plane->state->src_y, + plane->state->src_w, plane->state->src_h); } static const struct drm_plane_funcs intel_plane_funcs = { - .update_plane = intel_update_plane, - .disable_plane = intel_sprite_plane_disable, + .update_plane = drm_plane_helper_update, + .disable_plane = drm_plane_helper_disable, .destroy = intel_plane_destroy, .set_property = intel_plane_set_property, .atomic_duplicate_state = intel_plane_duplicate_state,
Now that we have hooks to enable the atomic plane helpers, we can use the plane helpers for our .update_plane() and .disable_plane() entrypoints. Even though we'd already refactored our plane handling code into check/commit, we still have to rework some of that logic here as we make the transition. The atomic plane helpers now call prepare_fb/cleanup_fb for us, so note the following: - 'commit' should not longer unpin the old object; the cleanup_fb will take care of this for us - prepare_fb only gets called when we actually have an fb, so the internal 'disable' code needs to update frontbuffer tracking - the cursor plane code never had 'prepare' code separated from its 'commit' function, so a lot of the cursor's 'commit' needs to be dropped since prepare_fb already does it for us - with all the pinning work removed from cursor commit, it can no longer fail and should return void rather than int Testcase: igt/kms_plane Testcase: igt/kms_universal_plane Testcase: igt/kms_cursor_crc Signed-off-by: Matt Roper <matthew.d.roper@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 255 +++++------------------------------ drivers/gpu/drm/i915/intel_sprite.c | 122 ++--------------- 2 files changed, 41 insertions(+), 336 deletions(-)