[Intel-gfx] [PATCH 6/7] drm/i915: Switch plane handling to atomic helpers (v5)
Ander Conselvan de Oliveira
conselvan2 at gmail.com
Fri Dec 12 04:53:40 PST 2014
On 12/09/2014 09:53 PM, 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.
>
> Note that we still need to make a few small behavioral changes to the
> driver entrypoints here as we make the transition, due to differences
> between how the atomic helpers set things up for us vs how our internal
> functions were setting things up.
>
> v2: Fix commit message typo (Bob)
>
> v3: Rebased on top of Gustavo Padovan's patches to kill off
> intel_crtc_cursor_set_obj() and intel_pipe_set_base().
>
> v4: Rebase again on latest display refactor series. Now that previous
> patches have smoothed the rough edges of the existing display code,
> the transition here is much less painful.
>
> v5: Plane helpers will re-prepare the same fb for a plane. Drop the
> WARN on this condition.
I think it would make sense to squash this patch with the previous one.
That way it becomes clearer where the intel_update_plane() goes. Also,
the atomic.track_fb crtc field is added in the previous patch but only
used here.
I would also split the change from state.orig_{dst,src} into a separate
patch, but I guess that's not really necessary.
Cheers,
Ander
> Testcase: igt/kms_plane
> Testcase: igt/kms_universal_plane
> Testcase: igt/kms_cursor_crc
> Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 222 ++++++++++++-----------------------
> drivers/gpu/drm/i915/intel_drv.h | 2 -
> drivers/gpu/drm/i915/intel_sprite.c | 38 +++---
> 3 files changed, 98 insertions(+), 164 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ea47923..2f80236 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11678,7 +11678,7 @@ intel_prepare_plane_fb(struct drm_plane *plane,
> unsigned frontbuffer_bits = 0;
> int ret = 0;
>
> - if (WARN_ON(fb == plane->fb || !obj))
> + if (!obj)
> return 0;
>
> switch (plane->type) {
> @@ -11745,7 +11745,7 @@ intel_check_primary_plane(struct drm_plane *plane,
> struct drm_device *dev = plane->dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct drm_crtc *crtc = state->base.crtc;
> - struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> + struct intel_crtc *intel_crtc;
> struct intel_plane *intel_plane = to_intel_plane(plane);
> struct drm_framebuffer *fb = state->base.fb;
> struct drm_rect *dest = &state->dst;
> @@ -11753,6 +11753,9 @@ intel_check_primary_plane(struct drm_plane *plane,
> const struct drm_rect *clip = &state->clip;
> int ret;
>
> + crtc = crtc ? crtc : plane->crtc;
> + intel_crtc = to_intel_crtc(crtc);
> +
> ret = drm_plane_helper_check_update(plane, crtc, fb,
> src, dest, clip,
> DRM_PLANE_HELPER_NO_SCALING,
> @@ -11812,23 +11815,26 @@ intel_commit_primary_plane(struct drm_plane *plane,
> struct drm_framebuffer *fb = state->base.fb;
> struct drm_device *dev = plane->dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> - struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> + struct intel_crtc *intel_crtc;
> struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> struct intel_plane *intel_plane = to_intel_plane(plane);
> struct drm_rect *src = &state->src;
>
> + crtc = crtc ? crtc : plane->crtc;
> + intel_crtc = to_intel_crtc(crtc);
> +
> plane->fb = fb;
> crtc->x = src->x1 >> 16;
> crtc->y = src->y1 >> 16;
>
> - intel_plane->crtc_x = state->orig_dst.x1;
> - intel_plane->crtc_y = state->orig_dst.y1;
> - intel_plane->crtc_w = drm_rect_width(&state->orig_dst);
> - intel_plane->crtc_h = drm_rect_height(&state->orig_dst);
> - intel_plane->src_x = state->orig_src.x1;
> - intel_plane->src_y = state->orig_src.y1;
> - intel_plane->src_w = drm_rect_width(&state->orig_src);
> - intel_plane->src_h = drm_rect_height(&state->orig_src);
> + intel_plane->crtc_x = state->base.crtc_x;
> + intel_plane->crtc_y = state->base.crtc_y;
> + intel_plane->crtc_w = state->base.crtc_w;
> + intel_plane->crtc_h = state->base.crtc_h;
> + intel_plane->src_x = state->base.src_x;
> + intel_plane->src_y = state->base.src_y;
> + intel_plane->src_w = state->base.src_w;
> + intel_plane->src_h = state->base.src_h;
> intel_plane->obj = obj;
>
> if (intel_crtc->active) {
> @@ -11857,6 +11863,32 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
> {
> struct drm_device *dev = crtc->dev;
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> + struct intel_plane *intel_plane;
> + struct drm_plane *p;
> + unsigned fb_bits = 0;
> +
> + /* Track fb's for any planes being disabled */
> + list_for_each_entry(p, &dev->mode_config.plane_list, head) {
> + intel_plane = to_intel_plane(p);
> +
> + if (intel_crtc->atomic.track_fbs & (1 << drm_plane_index(p))) {
> + switch (p->type) {
> + case DRM_PLANE_TYPE_PRIMARY:
> + fb_bits = INTEL_FRONTBUFFER_PRIMARY(intel_plane->pipe);
> + break;
> + case DRM_PLANE_TYPE_CURSOR:
> + fb_bits = INTEL_FRONTBUFFER_CURSOR(intel_plane->pipe);
> + break;
> + case DRM_PLANE_TYPE_OVERLAY:
> + fb_bits = INTEL_FRONTBUFFER_SPRITE(intel_plane->pipe);
> + break;
> + }
> +
> + mutex_lock(&dev->struct_mutex);
> + i915_gem_track_fb(intel_fb_obj(p->fb), NULL, fb_bits);
> + mutex_unlock(&dev->struct_mutex);
> + }
> + }
>
> if (intel_crtc->atomic.disable_fbc)
> intel_disable_fbc(dev);
> @@ -11900,111 +11932,6 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc)
> memset(&intel_crtc->atomic, 0, sizeof(intel_crtc->atomic));
> }
>
> -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 drm_device *dev = plane->dev;
> - struct drm_framebuffer *old_fb = plane->fb;
> - struct intel_plane_state state = {{ 0 }};
> - struct intel_plane *intel_plane = to_intel_plane(plane);
> - struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> - int ret;
> -
> - state.base.crtc = crtc ? crtc : plane->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_plane->check_plane(plane, &state);
> - if (ret)
> - return ret;
> -
> - if (fb != old_fb && fb) {
> - ret = intel_prepare_plane_fb(plane, fb);
> - if (ret)
> - return ret;
> - }
> -
> - if (!state.base.fb) {
> - unsigned fb_bits = 0;
> -
> - switch (plane->type) {
> - case DRM_PLANE_TYPE_PRIMARY:
> - fb_bits = INTEL_FRONTBUFFER_PRIMARY(intel_plane->pipe);
> - break;
> - case DRM_PLANE_TYPE_CURSOR:
> - fb_bits = INTEL_FRONTBUFFER_CURSOR(intel_plane->pipe);
> - break;
> - case DRM_PLANE_TYPE_OVERLAY:
> - fb_bits = INTEL_FRONTBUFFER_SPRITE(intel_plane->pipe);
> - break;
> - }
> -
> - /*
> - * 'prepare' is never called when plane is being disabled, so
> - * we need to handle frontbuffer tracking here
> - */
> - mutex_lock(&dev->struct_mutex);
> - i915_gem_track_fb(intel_fb_obj(plane->fb), NULL, fb_bits);
> - mutex_unlock(&dev->struct_mutex);
> - }
> -
> - intel_begin_crtc_commit(crtc);
> - intel_plane->commit_plane(plane, &state);
> - intel_finish_crtc_commit(crtc);
> -
> - if (fb != old_fb && old_fb) {
> - if (intel_crtc->active)
> - intel_wait_for_vblank(dev, intel_crtc->pipe);
> - intel_cleanup_plane_fb(plane, old_fb);
> - }
> -
> - plane->fb = fb;
> -
> - return 0;
> -}
> -
> -/**
> - * intel_disable_plane - disable a plane
> - * @plane: plane to disable
> - *
> - * General disable handler for all plane types.
> - */
> -int
> -intel_disable_plane(struct drm_plane *plane)
> -{
> - if (!plane->fb)
> - return 0;
> -
> - if (WARN_ON(!plane->crtc))
> - return -EINVAL;
> -
> - return plane->funcs->update_plane(plane, plane->crtc, NULL,
> - 0, 0, 0, 0, 0, 0, 0, 0);
> -}
> -
> /* Common destruction function for both primary and cursor planes */
> void intel_plane_destroy(struct drm_plane *plane)
> {
> @@ -12015,8 +11942,8 @@ void intel_plane_destroy(struct drm_plane *plane)
> }
>
> static const struct drm_plane_funcs intel_primary_plane_funcs = {
> - .update_plane = intel_update_plane,
> - .disable_plane = intel_disable_plane,
> + .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,
> @@ -12083,17 +12010,19 @@ intel_check_cursor_plane(struct drm_plane *plane,
> struct intel_plane_state *state)
> {
> struct drm_crtc *crtc = state->base.crtc;
> - struct drm_device *dev = crtc->dev;
> + struct drm_device *dev = plane->dev;
> struct drm_framebuffer *fb = state->base.fb;
> struct drm_rect *dest = &state->dst;
> struct drm_rect *src = &state->src;
> const struct drm_rect *clip = &state->clip;
> struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> - struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> - int crtc_w, crtc_h;
> + struct intel_crtc *intel_crtc;
> unsigned stride;
> int ret;
>
> + crtc = crtc ? crtc : plane->crtc;
> + intel_crtc = to_intel_crtc(crtc);
> +
> ret = drm_plane_helper_check_update(plane, crtc, fb,
> src, dest, clip,
> DRM_PLANE_HELPER_NO_SCALING,
> @@ -12108,15 +12037,14 @@ intel_check_cursor_plane(struct drm_plane *plane,
> goto finish;
>
> /* 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;
> }
> @@ -12134,8 +12062,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
>
> finish:
> if (intel_crtc->active) {
> - if (intel_crtc->cursor_width !=
> - drm_rect_width(&state->orig_dst))
> + if (intel_crtc->cursor_width != state->base.crtc_w)
> intel_crtc->atomic.update_wm = true;
>
> intel_crtc->atomic.fb_bits |=
> @@ -12150,24 +12077,27 @@ intel_commit_cursor_plane(struct drm_plane *plane,
> struct intel_plane_state *state)
> {
> struct drm_crtc *crtc = state->base.crtc;
> - struct drm_device *dev = crtc->dev;
> - struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> + struct drm_device *dev = plane->dev;
> + struct intel_crtc *intel_crtc;
> struct intel_plane *intel_plane = to_intel_plane(plane);
> struct drm_i915_gem_object *obj = intel_fb_obj(state->base.fb);
> uint32_t addr;
>
> + crtc = crtc ? crtc : plane->crtc;
> + intel_crtc = to_intel_crtc(crtc);
> +
> plane->fb = state->base.fb;
> - crtc->cursor_x = state->orig_dst.x1;
> - crtc->cursor_y = state->orig_dst.y1;
> -
> - intel_plane->crtc_x = state->orig_dst.x1;
> - intel_plane->crtc_y = state->orig_dst.y1;
> - intel_plane->crtc_w = drm_rect_width(&state->orig_dst);
> - intel_plane->crtc_h = drm_rect_height(&state->orig_dst);
> - intel_plane->src_x = state->orig_src.x1;
> - intel_plane->src_y = state->orig_src.y1;
> - intel_plane->src_w = drm_rect_width(&state->orig_src);
> - intel_plane->src_h = drm_rect_height(&state->orig_src);
> + crtc->cursor_x = state->base.crtc_x;
> + crtc->cursor_y = state->base.crtc_y;
> +
> + intel_plane->crtc_x = state->base.crtc_x;
> + intel_plane->crtc_y = state->base.crtc_y;
> + intel_plane->crtc_w = state->base.crtc_w;
> + intel_plane->crtc_h = state->base.crtc_h;
> + intel_plane->src_x = state->base.src_x;
> + intel_plane->src_y = state->base.src_y;
> + intel_plane->src_w = state->base.src_w;
> + intel_plane->src_h = state->base.src_h;
> intel_plane->obj = obj;
>
> if (intel_crtc->cursor_bo == obj)
> @@ -12183,16 +12113,16 @@ intel_commit_cursor_plane(struct drm_plane *plane,
> intel_crtc->cursor_addr = addr;
> intel_crtc->cursor_bo = obj;
> update:
> - intel_crtc->cursor_width = drm_rect_width(&state->orig_dst);
> - intel_crtc->cursor_height = drm_rect_height(&state->orig_dst);
> + intel_crtc->cursor_width = state->base.crtc_w;
> + intel_crtc->cursor_height = state->base.crtc_h;
>
> if (intel_crtc->active)
> intel_crtc_update_cursor(crtc, state->visible);
> }
>
> static const struct drm_plane_funcs intel_cursor_plane_funcs = {
> - .update_plane = intel_update_plane,
> - .disable_plane = intel_disable_plane,
> + .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_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index abb80e1..d8a6d6c 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -248,8 +248,6 @@ struct intel_plane_state {
> struct drm_rect src;
> struct drm_rect dst;
> struct drm_rect clip;
> - struct drm_rect orig_src;
> - struct drm_rect orig_dst;
> bool visible;
>
> /*
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 946ff10..7b7acb0 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1064,12 +1064,13 @@ 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;
> int pixel_size;
>
> + intel_crtc = intel_crtc ? intel_crtc : to_intel_crtc(plane->crtc);
> +
> if (!fb) {
> state->visible = false;
> goto finish;
> @@ -1150,10 +1151,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.
> @@ -1250,7 +1251,7 @@ intel_commit_sprite_plane(struct drm_plane *plane,
> struct intel_plane_state *state)
> {
> struct drm_crtc *crtc = state->base.crtc;
> - struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> + struct intel_crtc *intel_crtc;
> struct intel_plane *intel_plane = to_intel_plane(plane);
> struct drm_framebuffer *fb = state->base.fb;
> struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> @@ -1258,14 +1259,19 @@ intel_commit_sprite_plane(struct drm_plane *plane,
> unsigned int crtc_w, crtc_h;
> uint32_t src_x, src_y, src_w, src_h;
>
> - intel_plane->crtc_x = state->orig_dst.x1;
> - intel_plane->crtc_y = state->orig_dst.y1;
> - intel_plane->crtc_w = drm_rect_width(&state->orig_dst);
> - intel_plane->crtc_h = drm_rect_height(&state->orig_dst);
> - intel_plane->src_x = state->orig_src.x1;
> - intel_plane->src_y = state->orig_src.y1;
> - intel_plane->src_w = drm_rect_width(&state->orig_src);
> - intel_plane->src_h = drm_rect_height(&state->orig_src);
> + intel_plane->crtc_x = state->base.crtc_x;
> + intel_plane->crtc_y = state->base.crtc_y;
> + intel_plane->crtc_w = state->base.crtc_w;
> + intel_plane->crtc_h = state->base.crtc_h;
> + intel_plane->src_x = state->base.src_x;
> + intel_plane->src_y = state->base.src_y;
> + intel_plane->src_w = state->base.src_w;
> + intel_plane->src_h = state->base.src_h;
> +
> + crtc = crtc ? crtc : plane->crtc;
> + intel_crtc = to_intel_crtc(crtc);
> +
> + plane->fb = state->base.fb;
> intel_plane->obj = obj;
>
> if (intel_crtc->active) {
> @@ -1389,8 +1395,8 @@ int intel_plane_restore(struct drm_plane *plane)
> }
>
> static const struct drm_plane_funcs intel_plane_funcs = {
> - .update_plane = intel_update_plane,
> - .disable_plane = intel_disable_plane,
> + .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,
>
More information about the Intel-gfx
mailing list