On Tue, Nov 15, 2016 at 08:58:14AM +0000, Chris Wilson wrote:
> The generic atomic helper likes to skip a prepare_plane_fb() if it
> decides that the plane->fb is unchanged. This is wrong for us for a
> couple of reasons:
>
> - if the pipe is reconfigured (i.e. a size change) but the framebuffer
> is untouched, we still have to flush any rendering prior to the
> reconfiguration to prevent wait-for-scanline GPU hangs
>
> - if the framebuffer is rotated, it remains the same but has a
> different view and a different address.
>
> Finally, even if the framebuffer is unchanged the flip/modeset should be
> ordered with respect to rendering to the frontbuffer.
>
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
This is directly against
commit fcc60b413d14dd06ddbd79ec50e83c4fb2a097ba
Author: Keith Packard <keithp@xxxxxxxxxx>
Date: Sat Jun 4 01:16:22 2016 -0700
drm: Don't prepare or cleanup unchanging frame buffers [v3]
and I'm pretty sure that was tested on i915. Do we need to instead revert
the core change? Iirc the idea was to make cursors block less, but that
didn't really pan out fully.
-Daniel
> ---
> drivers/gpu/drm/i915/intel_atomic_plane.c | 2 --
> drivers/gpu/drm/i915/intel_display.c | 60 ++++++++++++++++++++++++-------
> drivers/gpu/drm/i915/intel_drv.h | 4 ---
> 3 files changed, 47 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> index ff821649486e..89a34350a35d 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -201,8 +201,6 @@ static void intel_plane_atomic_update(struct drm_plane *plane,
> }
>
> const struct drm_plane_helper_funcs intel_plane_helper_funcs = {
> - .prepare_fb = intel_prepare_plane_fb,
> - .cleanup_fb = intel_cleanup_plane_fb,
> .atomic_check = intel_plane_atomic_check,
> .atomic_update = intel_plane_atomic_update,
> };
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8e04f31bf12e..dca1e0b512e5 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14158,7 +14158,7 @@ static int intel_atomic_check(struct drm_device *dev,
> *
> * Returns 0 on success, negative error code on failure.
> */
> -int
> +static int
> intel_prepare_plane_fb(struct drm_plane *plane,
> struct drm_plane_state *new_state)
> {
> @@ -14252,7 +14252,7 @@ intel_prepare_plane_fb(struct drm_plane *plane,
> *
> * Must be called with struct_mutex held.
> */
> -void
> +static void
> intel_cleanup_plane_fb(struct drm_plane *plane,
> struct drm_plane_state *old_state)
> {
> @@ -14271,6 +14271,49 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
> intel_unpin_fb_obj(old_state->fb, old_state->rotation);
> }
>
> +static int intel_atomic_commit_prepare_planes(struct drm_atomic_state *state)
> +{
> + struct drm_plane *plane;
> + struct drm_plane_state *plane_state;
> + int i, j, ret;
> +
> + ret = mutex_lock_interruptible(&state->dev->struct_mutex);
> + if (ret)
> + return ret;
> +
> + for_each_plane_in_state(state, plane, plane_state, i) {
> + ret = intel_prepare_plane_fb(plane, plane_state);
> + if (ret)
> + break;
> + }
> +
> + if (ret) {
> + for_each_plane_in_state(state, plane, plane_state, j) {
> + if (j >= i)
> + break;
> +
> + intel_cleanup_plane_fb(plane, plane_state);
> + }
> + }
> +
> + mutex_unlock(&state->dev->struct_mutex);
> +
> + return ret;
> +}
> +
> +static void intel_atomic_commit_cleanup_planes(struct drm_atomic_state *state)
> +{
> + struct drm_plane *plane;
> + struct drm_plane_state *plane_state;
> + int i;
> +
> + mutex_lock(&state->dev->struct_mutex);
> +
> + for_each_plane_in_state(state, plane, plane_state, i)
> + intel_cleanup_plane_fb(plane, plane_state);
> +
> + mutex_unlock(&state->dev->struct_mutex);
> +}
>
> static int intel_atomic_prepare_commit(struct drm_device *dev,
> struct drm_atomic_state *state)
> @@ -14292,14 +14335,7 @@ static int intel_atomic_prepare_commit(struct drm_device *dev,
> flush_workqueue(dev_priv->wq);
> }
>
> - ret = mutex_lock_interruptible(&dev->struct_mutex);
> - if (ret)
> - return ret;
> -
> - ret = drm_atomic_helper_prepare_planes(dev, state);
> - mutex_unlock(&dev->struct_mutex);
> -
> - return ret;
> + return intel_atomic_commit_prepare_planes(state);
> }
>
> u32 intel_crtc_get_vblank_counter(struct intel_crtc *crtc)
> @@ -14622,9 +14658,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
> if (intel_state->modeset)
> intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET);
>
> - mutex_lock(&dev->struct_mutex);
> - drm_atomic_helper_cleanup_planes(dev, state);
> - mutex_unlock(&dev->struct_mutex);
> + intel_atomic_commit_cleanup_planes(state);
>
> drm_atomic_helper_commit_cleanup_done(state);
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 4cb8254c66dd..fff55d93ca73 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1281,10 +1281,6 @@ __intel_framebuffer_create(struct drm_device *dev,
> void intel_finish_page_flip_cs(struct drm_i915_private *dev_priv, int pipe);
> void intel_finish_page_flip_mmio(struct drm_i915_private *dev_priv, int pipe);
> void intel_check_page_flip(struct drm_i915_private *dev_priv, int pipe);
> -int intel_prepare_plane_fb(struct drm_plane *plane,
> - struct drm_plane_state *new_state);
> -void intel_cleanup_plane_fb(struct drm_plane *plane,
> - struct drm_plane_state *old_state);
> int intel_plane_atomic_get_property(struct drm_plane *plane,
> const struct drm_plane_state *state,
> struct drm_property *property,
> --
> 2.10.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
