On Tue, Apr 19, 2016 at 09:52:23AM +0200, Maarten Lankhorst wrote:
> Instead of calling prepare_flip right before calling finish_page_flip
> do everything from prepare_page_flip in finish_page_flip.
>
> Putting prepare and finish page_flip in a single step removes the need
> for INTEL_FLIP_COMPLETE, so it can be removed. This simplifies the code
> slightly.
>
> Changes since v1:
> - Invert if case to simplify code.
> - Add missing barrier.
> - Reword commit message.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 3 --
> drivers/gpu/drm/i915/i915_irq.c | 18 ++-----
> drivers/gpu/drm/i915/intel_display.c | 92 ++++++++++++++----------------------
> drivers/gpu/drm/i915/intel_drv.h | 2 -
> 4 files changed, 39 insertions(+), 76 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 0092aaf47c43..def95532d421 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -619,9 +619,6 @@ static int i915_gem_pageflip_info(struct seq_file *m, void *data)
> if (pending == INTEL_FLIP_INACTIVE) {
> seq_printf(m, "Flip ioctl preparing on pipe %c (plane %c)\n",
> pipe, plane);
> - } else if (pending >= INTEL_FLIP_COMPLETE) {
> - seq_printf(m, "Flip queued on pipe %c (plane %c)\n",
> - pipe, plane);
> } else {
> seq_printf(m, "Flip pending (waiting for vsync) on pipe %c (plane %c)\n",
> pipe, plane);
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 93da4feb3048..86f7060d3ddb 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1721,10 +1721,8 @@ static void valleyview_pipestat_irq_handler(struct drm_device *dev,
> intel_pipe_handle_vblank(dev, pipe))
> intel_check_page_flip(dev, pipe);
>
> - if (pipe_stats[pipe] & PLANE_FLIP_DONE_INT_STATUS_VLV) {
> - intel_prepare_page_flip(dev, pipe);
> + if (pipe_stats[pipe] & PLANE_FLIP_DONE_INT_STATUS_VLV)
> intel_finish_page_flip(dev, pipe);
> - }
>
> if (pipe_stats[pipe] & PIPE_CRC_DONE_INTERRUPT_STATUS)
> i9xx_pipe_crc_irq_handler(dev, pipe);
> @@ -2182,10 +2180,8 @@ static void ilk_display_irq_handler(struct drm_device *dev, u32 de_iir)
> i9xx_pipe_crc_irq_handler(dev, pipe);
>
> /* plane/pipes map 1:1 on ilk+ */
> - if (de_iir & DE_PLANE_FLIP_DONE(pipe)) {
> - intel_prepare_page_flip(dev, pipe);
> + if (de_iir & DE_PLANE_FLIP_DONE(pipe))
> intel_finish_page_flip_plane(dev, pipe);
> - }
> }
>
> /* check event from PCH */
> @@ -2229,10 +2225,8 @@ static void ivb_display_irq_handler(struct drm_device *dev, u32 de_iir)
> intel_check_page_flip(dev, pipe);
>
> /* plane/pipes map 1:1 on ilk+ */
> - if (de_iir & DE_PLANE_FLIP_DONE_IVB(pipe)) {
> - intel_prepare_page_flip(dev, pipe);
> + if (de_iir & DE_PLANE_FLIP_DONE_IVB(pipe))
> intel_finish_page_flip_plane(dev, pipe);
> - }
> }
>
> /* check event from PCH */
> @@ -2436,10 +2430,8 @@ gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
> else
> flip_done &= GEN8_PIPE_PRIMARY_FLIP_DONE;
>
> - if (flip_done) {
> - intel_prepare_page_flip(dev, pipe);
> + if (flip_done)
> intel_finish_page_flip_plane(dev, pipe);
> - }
>
> if (iir & GEN8_PIPE_CDCLK_CRC_DONE)
> hsw_pipe_crc_irq_handler(dev, pipe);
> @@ -4025,7 +4017,6 @@ static bool i8xx_handle_vblank(struct drm_device *dev,
> if (I915_READ16(ISR) & flip_pending)
> goto check_page_flip;
>
> - intel_prepare_page_flip(dev, plane);
> intel_finish_page_flip(dev, pipe);
> return true;
>
> @@ -4216,7 +4207,6 @@ static bool i915_handle_vblank(struct drm_device *dev,
> if (I915_READ(ISR) & flip_pending)
> goto check_page_flip;
>
> - intel_prepare_page_flip(dev, plane);
> intel_finish_page_flip(dev, pipe);
> return true;
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 97a8418f6539..ccbc2a448258 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3118,7 +3118,6 @@ static void intel_complete_page_flips(struct drm_device *dev)
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> enum plane plane = intel_crtc->plane;
>
> - intel_prepare_page_flip(dev, plane);
> intel_finish_page_flip_plane(dev, plane);
> }
> }
> @@ -10960,50 +10959,6 @@ static void intel_unpin_work_fn(struct work_struct *__work)
> kfree(work);
> }
>
> -static void do_intel_finish_page_flip(struct drm_device *dev,
> - struct drm_crtc *crtc)
> -{
> - struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> - struct intel_unpin_work *work;
> - unsigned long flags;
> -
> - /* Ignore early vblank irqs */
> - if (intel_crtc == NULL)
> - return;
> -
> - /*
> - * This is called both by irq handlers and the reset code (to complete
> - * lost pageflips) so needs the full irqsave spinlocks.
> - */
> - spin_lock_irqsave(&dev->event_lock, flags);
> - work = intel_crtc->unpin_work;
> -
> - if (work && atomic_read(&work->pending) >= INTEL_FLIP_COMPLETE) {
> - /* ensure that the unpin work is consistent wrt ->pending. */
> - smp_mb__after_atomic();
> -
> - page_flip_completed(intel_crtc);
> - }
> -
> - spin_unlock_irqrestore(&dev->event_lock, flags);
> -}
> -
> -void intel_finish_page_flip(struct drm_device *dev, int pipe)
> -{
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
> -
> - do_intel_finish_page_flip(dev, crtc);
> -}
> -
> -void intel_finish_page_flip_plane(struct drm_device *dev, int plane)
> -{
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - struct drm_crtc *crtc = dev_priv->plane_to_crtc_mapping[plane];
> -
> - do_intel_finish_page_flip(dev, crtc);
> -}
> -
> /* Is 'a' after or equal to 'b'? */
> static bool g4x_flip_count_after_eq(u32 a, u32 b)
> {
> @@ -11016,6 +10971,9 @@ static bool page_flip_finished(struct intel_crtc *crtc)
> struct drm_i915_private *dev_priv = dev->dev_private;
> unsigned reset_counter;
>
> + /* ensure that the unpin work is consistent wrt ->pending. */
> + smp_mb__after_atomic();
> +
> reset_counter = i915_reset_counter(&dev_priv->gpu_error);
> if (crtc->reset_counter != reset_counter)
> return true;
> @@ -11057,28 +11015,48 @@ static bool page_flip_finished(struct intel_crtc *crtc)
> crtc->unpin_work->flip_count);
> }
>
> -void intel_prepare_page_flip(struct drm_device *dev, int plane)
> +static void do_intel_finish_page_flip(struct drm_device *dev,
> + struct drm_crtc *crtc)
> {
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - struct intel_crtc *intel_crtc =
> - to_intel_crtc(dev_priv->plane_to_crtc_mapping[plane]);
> + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> + struct intel_unpin_work *work;
> unsigned long flags;
>
> + /* Ignore early vblank irqs */
> + if (intel_crtc == NULL)
> + return;
>
> /*
> * This is called both by irq handlers and the reset code (to complete
> * lost pageflips) so needs the full irqsave spinlocks.
> - *
> - * NB: An MMIO update of the plane base pointer will also
> - * generate a page-flip completion irq, i.e. every modeset
> - * is also accompanied by a spurious intel_prepare_page_flip().
> */
> spin_lock_irqsave(&dev->event_lock, flags);
> - if (intel_crtc->unpin_work && page_flip_finished(intel_crtc))
> - atomic_inc_not_zero(&intel_crtc->unpin_work->pending);
> + work = intel_crtc->unpin_work;
> +
> + if (work != NULL &&
> + atomic_read(&work->pending) == INTEL_FLIP_PENDING &&
> + page_flip_finished(intel_crtc))
> + page_flip_completed(intel_crtc);
> +
> spin_unlock_irqrestore(&dev->event_lock, flags);
> }
>
> +void intel_finish_page_flip(struct drm_device *dev, int pipe)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
> +
> + do_intel_finish_page_flip(dev, crtc);
> +}
> +
> +void intel_finish_page_flip_plane(struct drm_device *dev, int plane)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + struct drm_crtc *crtc = dev_priv->plane_to_crtc_mapping[plane];
> +
> + do_intel_finish_page_flip(dev, crtc);
> +}
> +
Do we really need a _plane version of this function? intel_complete_page_flips()
and ilk+ irq handlers are the only ones using it and the irq handlers claim
there's a 1:1 plane-pipe mapping anyway. That single call in
intel_complete_page_flips() already have the crtc and can easily do the
dev_priv->plane_to_crtc_mapping[plane] there if it's really needed.
Btw, intel_complete_page_flips() is only called from intel_finish_reset() so one
could question it's usefulness as well.
> static inline void intel_mark_page_flip_active(struct intel_unpin_work *work)
> {
> /* Ensure that the work item is consistent when activating it ... */
> @@ -11523,8 +11501,8 @@ static bool __intel_pageflip_stall_check(struct drm_device *dev,
> /* ensure that the unpin work is consistent wrt ->pending. */
> smp_mb__after_atomic();
>
> - if (pending != INTEL_FLIP_PENDING)
> - return pending == INTEL_FLIP_COMPLETE;
> + if (pending == INTEL_FLIP_INACTIVE)
> + return false;
With INTEL_FLIP_COMPLETE removed I would prefer ->pending to just be true or
false.
>
> if (work->flip_ready_vblank == 0) {
> if (work->flip_queued_req &&
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 517baebb2399..fecc89600667 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -951,7 +951,6 @@ struct intel_unpin_work {
> atomic_t pending;
> #define INTEL_FLIP_INACTIVE 0
> #define INTEL_FLIP_PENDING 1
> -#define INTEL_FLIP_COMPLETE 2
> u32 flip_count;
> u32 gtt_offset;
> struct drm_i915_gem_request *flip_queued_req;
> @@ -1164,7 +1163,6 @@ struct drm_framebuffer *
> __intel_framebuffer_create(struct drm_device *dev,
> struct drm_mode_fb_cmd2 *mode_cmd,
> struct drm_i915_gem_object *obj);
> -void intel_prepare_page_flip(struct drm_device *dev, int plane);
> void intel_finish_page_flip(struct drm_device *dev, int pipe);
> void intel_finish_page_flip_plane(struct drm_device *dev, int plane);
> void intel_check_page_flip(struct drm_device *dev, int pipe);
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Intel Sweden AB Registered Office: Knarrarnasgatan 15, 164 40 Kista, Stockholm, Sweden Registration Number: 556189-6027
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
