Re: [PATCH 1/5] drm/i915: Refactor work that can sleep out of commit (v5)

Ander Conselvan de Oliveira
2014-12-22T14:12:34Z

On 12/16/2014 02:23 AM, Matt Roper wrote:
> Once we integrate our work into the atomic pipeline, plane commit
> operations will need to happen with interrupts disabled, due to vblank
> evasion. Our commit functions today include sleepable work, so those
> operations need to be split out and run either before or after the
> atomic register programming.
>
> The solution here calculates which of those operations will need to be
> performed during the 'check' phase and sets flags in an intel_crtc
> sub-struct. New intel_begin_crtc_commit() and
> intel_finish_crtc_commit() functions are added before and after the
> actual register programming; these will eventually be called from the
> atomic plane helper's .atomic_begin() and .atomic_end() entrypoints.
>
> v2: Fix broken sprite code split
>
> v3: Make the pre/post commit work crtc-based to match how we eventually
> want this to be called from the atomic plane helpers.
>
> v4: Some platforms that haven't had their watermark code reworked were
> waiting for vblank, then calling update_sprite_watermarks in their
> platform-specific disable code. These also need to be flagged out
> of the critical section.
>
> v5: Sprite plane test for primary show/hide should just set the flag to
> wait for pending flips, not actually perform the wait. (Ander)
>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 180 ++++++++++++++++++++++-------------
> drivers/gpu/drm/i915/intel_drv.h | 31 ++++++
> drivers/gpu/drm/i915/intel_sprite.c | 78 ++++++---------
> 3 files changed, 178 insertions(+), 111 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3044af5..5d90114 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11737,7 +11737,11 @@ static int
> intel_check_primary_plane(struct drm_plane *plane,
> struct intel_plane_state *state)
> {
> + 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_plane *intel_plane = to_intel_plane(plane);
> struct drm_framebuffer *fb = state->base.fb;
> struct drm_rect *dest = &state->dst;
> struct drm_rect *src = &state->src;
> @@ -11758,6 +11762,40 @@ intel_check_primary_plane(struct drm_plane *plane,
> return -EBUSY;
> }
>
> + if (intel_crtc->active) {
> + /*
> + * FBC does not work on some platforms for rotated
> + * planes, so disable it when rotation is not 0 and
> + * update it when rotation is set back to 0.
> + *
> + * FIXME: This is redundant with the fbc update done in
> + * the primary plane enable function except that that
> + * one is done too late. We eventually need to unify
> + * this.
> + */
> + if (intel_crtc->primary_enabled &&
> + INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
> + dev_priv->fbc.plane == intel_crtc->plane &&
> + intel_plane->rotation != BIT(DRM_ROTATE_0)) {
> + intel_crtc->atomic.disable_fbc = true;
> + }
> +
> + if (state->visible) {
> + /*
> + * BDW signals flip done immediately if the plane
> + * is disabled, even if the plane enable is already
> + * armed to occur at the next vblank :(
> + */
> + if (IS_BROADWELL(dev) && !intel_crtc->primary_enabled)
> + intel_crtc->atomic.wait_vblank = true;
> + }
> +
> + intel_crtc->atomic.fb_bits |=
> + INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
> +
> + intel_crtc->atomic.update_fbc = true;
> + }
> +
> return 0;
> }
>
> @@ -11773,18 +11811,6 @@ intel_commit_primary_plane(struct drm_plane *plane,
> 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;
> - enum pipe pipe = intel_plane->pipe;
> -
> - if (!fb) {
> - /*
> - * '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,
> - INTEL_FRONTBUFFER_PRIMARY(pipe));
> - mutex_unlock(&dev->struct_mutex);
> - }
>
> plane->fb = fb;
> crtc->x = src->x1 >> 16;
> @@ -11801,26 +11827,7 @@ intel_commit_primary_plane(struct drm_plane *plane,
> intel_plane->obj = obj;
>
> if (intel_crtc->active) {
> - /*
> - * FBC does not work on some platforms for rotated
> - * planes, so disable it when rotation is not 0 and
> - * update it when rotation is set back to 0.
> - *
> - * FIXME: This is redundant with the fbc update done in
> - * the primary plane enable function except that that
> - * one is done too late. We eventually need to unify
> - * this.
> - */
> - if (intel_crtc->primary_enabled &&
> - INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
> - dev_priv->fbc.plane == intel_crtc->plane &&
> - intel_plane->rotation != BIT(DRM_ROTATE_0)) {
> - intel_fbc_disable(dev);
> - }
> -
> if (state->visible) {
> - bool was_enabled = intel_crtc->primary_enabled;
> -
> /* FIXME: kill this fastboot hack */
> intel_update_pipe_size(intel_crtc);
>
> @@ -11828,14 +11835,6 @@ intel_commit_primary_plane(struct drm_plane *plane,
>
> dev_priv->display.update_primary_plane(crtc, plane->fb,
> crtc->x, crtc->y);
> -
> - /*
> - * BDW signals flip done immediately if the plane
> - * is disabled, even if the plane enable is already
> - * armed to occur at the next vblank :(
> - */
> - if (IS_BROADWELL(dev) && !was_enabled)
> - intel_wait_for_vblank(dev, intel_crtc->pipe);
> } else {
> /*
> * If clipping results in a non-visible primary plane,
> @@ -11846,13 +11845,50 @@ intel_commit_primary_plane(struct drm_plane *plane,
> */
> intel_disable_primary_hw_plane(plane, crtc);
> }
> + }
> +}
>
> - intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_PRIMARY(pipe));
> +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);
> +
> + if (intel_crtc->atomic.disable_fbc)
> + intel_fbc_disable(dev);
> +
> + if (intel_crtc->atomic.pre_disable_primary)
> + intel_pre_disable_primary(crtc);
> +
> + if (intel_crtc->atomic.update_wm)
> + intel_update_watermarks(crtc);
> +}
>
> +static void intel_finish_crtc_commit(struct drm_crtc *crtc)
> +{
> + struct drm_device *dev = crtc->dev;
> + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> + struct drm_plane *p;
> +
> + if (intel_crtc->atomic.wait_vblank)
> + intel_wait_for_vblank(dev, intel_crtc->pipe);
> +
> + intel_frontbuffer_flip(dev, intel_crtc->atomic.fb_bits);
> +
> + if (intel_crtc->atomic.update_fbc) {
> mutex_lock(&dev->struct_mutex);
> intel_fbc_update(dev);
> mutex_unlock(&dev->struct_mutex);
> }
> +
> + if (intel_crtc->atomic.post_enable_primary)
> + intel_post_enable_primary(crtc);
> +
> + drm_for_each_legacy_plane(p, &dev->mode_config.plane_list)
> + if (intel_crtc->atomic.update_sprite_watermarks & drm_plane_index(p))
> + intel_update_sprite_watermarks(p, crtc, 0, 0, 0,
> + false, false);
> +
> + memset(&intel_crtc->atomic, 0, sizeof(intel_crtc->atomic));
> }
>
> int
> @@ -11864,7 +11900,7 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> {
> struct drm_device *dev = plane->dev;
> struct drm_framebuffer *old_fb = plane->fb;
> - struct intel_plane_state state;
> + 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;
> @@ -11902,7 +11938,33 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> 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);

This needs to be rebased on top of the patch that added the
intel_runtime_pm_get() and _put() calls here.

> if (fb != old_fb && old_fb) {
> if (intel_crtc->active)
> @@ -12009,6 +12071,7 @@ 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);
> + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> int crtc_w, crtc_h;
> unsigned stride;
> int ret;
> @@ -12024,7 +12087,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
>
> /* if we want to turn off the cursor ignore width and height */
> if (!obj)
> - return 0;
> + goto finish;
>
> /* Check for which cursor types we support */
> crtc_w = drm_rect_width(&state->orig_dst);
> @@ -12051,6 +12114,16 @@ intel_check_cursor_plane(struct drm_plane *plane,
> }
> mutex_unlock(&dev->struct_mutex);
>
> +finish:
> + if (intel_crtc->active) {
> + if (intel_crtc->cursor_width !=
> + drm_rect_width(&state->orig_dst))
> + intel_crtc->atomic.update_wm = true;
> +
> + intel_crtc->atomic.fb_bits |=
> + INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe);
> + }
> +
> return ret;
> }
>
> @@ -12063,9 +12136,6 @@ intel_commit_cursor_plane(struct drm_plane *plane,
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> struct intel_plane *intel_plane = to_intel_plane(plane);
> struct drm_i915_gem_object *obj = intel_fb_obj(state->base.fb);
> - struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
> - enum pipe pipe = intel_crtc->pipe;
> - unsigned old_width;
> uint32_t addr;
>
> plane->fb = state->base.fb;
> @@ -12085,17 +12155,6 @@ intel_commit_cursor_plane(struct drm_plane *plane,
> if (intel_crtc->cursor_bo == obj)
> goto update;
>
> - /*
> - * 'prepare' is only called when fb != NULL; we still need to update
> - * frontbuffer tracking for the 'disable' case here.
> - */
> - if (!obj) {
> - mutex_lock(&dev->struct_mutex);
> - i915_gem_track_fb(old_obj, NULL,
> - INTEL_FRONTBUFFER_CURSOR(pipe));
> - mutex_unlock(&dev->struct_mutex);
> - }
> -
> if (!obj)
> addr = 0;
> else if (!INTEL_INFO(dev)->cursor_needs_physical)
> @@ -12106,18 +12165,11 @@ intel_commit_cursor_plane(struct drm_plane *plane,
> intel_crtc->cursor_addr = addr;
> intel_crtc->cursor_bo = obj;
> update:
> - old_width = intel_crtc->cursor_width;
> -
> intel_crtc->cursor_width = drm_rect_width(&state->orig_dst);
> intel_crtc->cursor_height = drm_rect_height(&state->orig_dst);
>
> - if (intel_crtc->active) {
> - if (old_width != intel_crtc->cursor_width)
> - intel_update_watermarks(crtc);
> + if (intel_crtc->active)
> intel_crtc_update_cursor(crtc, state->visible);
> -
> - intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
> - }
> }
>
> static const struct drm_plane_funcs intel_cursor_plane_funcs = {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 588b618..a03bd72 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -251,6 +251,12 @@ struct intel_plane_state {
> struct drm_rect orig_src;
> struct drm_rect orig_dst;
> bool visible;
> +
> + /*
> + * used only for sprite planes to determine when to implicitly
> + * enable/disable the primary plane
> + */
> + bool hides_primary;
> };
>
> struct intel_plane_config {
> @@ -415,6 +421,27 @@ struct skl_pipe_wm {
> uint32_t linetime;
> };
>
> +/*
> + * Tracking of operations that need to be performed at the beginning/end of an
> + * atomic commit, outside the atomic section where interrupts are disabled.
> + * These are generally operations that grab mutexes or might otherwise sleep
> + * and thus can't be run with interrupts disabled.
> + */
> +struct intel_crtc_atomic_commit {
> + /* Sleepable operations to perform before commit */
> + bool wait_for_flips;

In intel_begin_crtc_commit(), the three fields below are handled, but
not the one above.

> + bool disable_fbc;
> + bool pre_disable_primary;
> + bool update_wm;
> +
> + /* Sleepable operations to perform after commit */
> + unsigned fb_bits;
> + bool wait_vblank;
> + bool update_fbc;
> + bool post_enable_primary;
> + unsigned update_sprite_watermarks;
> +};
> +
> struct intel_crtc {
> struct drm_crtc base;
> enum pipe pipe;
> @@ -468,6 +495,8 @@ struct intel_crtc {
>
> int scanline_offset;
> struct intel_mmio_flip mmio_flip;
> +
> + struct intel_crtc_atomic_commit atomic;
> };
>
> struct intel_plane_wm_parameters {
> @@ -1213,6 +1242,8 @@ int intel_sprite_get_colorkey(struct drm_device *dev, void *data,
> bool intel_pipe_update_start(struct intel_crtc *crtc,
> uint32_t *start_vbl_count);
> void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count);
> +void intel_post_enable_primary(struct drm_crtc *crtc);
> +void intel_pre_disable_primary(struct drm_crtc *crtc);
>
> /* intel_tv.c */
> void intel_tv_init(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index c18e57d..ff7d6a1 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -771,9 +771,8 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
> * Avoid underruns when disabling the sprite.
> * FIXME remove once watermark updates are done properly.
> */
> - intel_wait_for_vblank(dev, pipe);
> -
> - intel_update_sprite_watermarks(plane, crtc, 0, 0, 0, false, false);
> + intel_crtc->atomic.wait_vblank = true;
> + intel_crtc->atomic.update_sprite_watermarks |= (1 << drm_plane_index(plane));
> }
>
> static int
> @@ -976,12 +975,11 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
> * Avoid underruns when disabling the sprite.
> * FIXME remove once watermark updates are done properly.
> */
> - intel_wait_for_vblank(dev, pipe);
> -
> - intel_update_sprite_watermarks(plane, crtc, 0, 0, 0, false, false);
> + intel_crtc->atomic.wait_vblank = true;
> + intel_crtc->atomic.update_sprite_watermarks |= (1 << drm_plane_index(plane));
> }
>
> -static void
> +void
> intel_post_enable_primary(struct drm_crtc *crtc)
> {
> struct drm_device *dev = crtc->dev;
> @@ -1008,7 +1006,7 @@ intel_post_enable_primary(struct drm_crtc *crtc)
> mutex_unlock(&dev->struct_mutex);
> }
>
> -static void
> +void

Should this have kernel doc?

> intel_pre_disable_primary(struct drm_crtc *crtc)
> {
> struct drm_device *dev = crtc->dev;
> @@ -1113,7 +1111,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
>
> if (!fb) {
> state->visible = false;
> - return 0;
> + goto finish;
> }
>
> /* Don't modify another pipe's plane */
> @@ -1260,6 +1258,29 @@ intel_check_sprite_plane(struct drm_plane *plane,
> dst->y1 = crtc_y;
> dst->y2 = crtc_y + crtc_h;
>
> +finish:
> + /*
> + * If the sprite is completely covering the primary plane,
> + * we can disable the primary and save power.
> + */
> + state->hides_primary = drm_rect_equals(dst, clip) &&
> + !colorkey_enabled(intel_plane);
> + WARN_ON(state->hides_primary && !state->visible && intel_crtc->active);
> +
> + if (intel_crtc->active) {
> + if (intel_crtc->primary_enabled == state->hides_primary)
> + intel_crtc->atomic.wait_for_flips = true;
> +
> + if (intel_crtc->primary_enabled && state->hides_primary)
> + intel_crtc->atomic.pre_disable_primary = true;
> +
> + intel_crtc->atomic.fb_bits |=
> + INTEL_FRONTBUFFER_SPRITE(intel_crtc->pipe);
> +
> + if (!intel_crtc->primary_enabled && !state->hides_primary)
> + intel_crtc->atomic.post_enable_primary = true;
> + }
> +
> return 0;
> }
>
> @@ -1267,37 +1288,14 @@ static void
> intel_commit_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);
> int crtc_x, crtc_y;
> unsigned int crtc_w, crtc_h;
> uint32_t src_x, src_y, src_w, src_h;
> - struct drm_rect *dst = &state->dst;
> - const struct drm_rect *clip = &state->clip;
> - bool primary_enabled;
> -
> - /*
> - * 'prepare' is never called when plane is being disabled, so we need
> - * to handle frontbuffer tracking here
> - */
> - if (!fb) {
> - mutex_lock(&dev->struct_mutex);
> - i915_gem_track_fb(intel_fb_obj(plane->fb), NULL,
> - INTEL_FRONTBUFFER_SPRITE(pipe));
> - mutex_unlock(&dev->struct_mutex);
> - }
> -
> - /*
> - * If the sprite is completely covering the primary plane,
> - * we can disable the primary and save power.
> - */
> - primary_enabled = !drm_rect_equals(dst, clip) || colorkey_enabled(intel_plane);
> - WARN_ON(!primary_enabled && !state->visible && intel_crtc->active);
>
> intel_plane->crtc_x = state->orig_dst.x1;
> intel_plane->crtc_y = state->orig_dst.y1;
> @@ -1310,15 +1308,7 @@ intel_commit_sprite_plane(struct drm_plane *plane,
> intel_plane->obj = obj;
>
> if (intel_crtc->active) {
> - bool primary_was_enabled = intel_crtc->primary_enabled;
> -
> - intel_crtc->primary_enabled = primary_enabled;
> -
> - if (primary_was_enabled != primary_enabled)
> - intel_crtc_wait_for_pending_flips(crtc);

As noted above, it seems that this patch removes the wait for pending flips.

Ander

> -
> - if (primary_was_enabled && !primary_enabled)
> - intel_pre_disable_primary(crtc);
> + intel_crtc->primary_enabled = !state->hides_primary;
>
> if (state->visible) {
> crtc_x = state->dst.x1;
> @@ -1335,12 +1325,6 @@ intel_commit_sprite_plane(struct drm_plane *plane,
> } else {
> intel_plane->disable_plane(plane, crtc);
> }
> -
> -
> - intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_SPRITE(pipe));
> -
> - if (!primary_was_enabled && primary_enabled)
> - intel_post_enable_primary(crtc);
> }
> }
>
>