[PATCH v2 03/11] drm/i915: Remove intel_prepare_page_flip.

Maarten Lankhorst maarten.lankhorst at linux.intel.com
Mon Apr 18 05:27:42 UTC 2016


Op 15-04-16 om 14:21 schreef Ander Conselvan De Oliveira:
> On Wed, 2016-04-13 at 11:18 +0200, Maarten Lankhorst wrote:
>> Do it in 1 step instead, use atomic_read since INTEL_FLIP_COMPLETE
>> is no longer useful.
> What's the deal with "use atomic_read"? I couldn't find where that is different
> from the old code.
This could probably use some rewording, what I believe was that with the removal of
prepare_page_flip, do_intel_finish_page_flip only needs a single atomic_read to complete.
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_debugfs.c  |  3 --
>>  drivers/gpu/drm/i915/i915_irq.c      | 18 ++-----
>>  drivers/gpu/drm/i915/intel_display.c | 96 ++++++++++++++---------------------
>> -
>>  drivers/gpu/drm/i915/intel_drv.h     |  2 -
>>  4 files changed, 40 insertions(+), 79 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
>> b/drivers/gpu/drm/i915/i915_debugfs.c
>> index df8073a2ffbe..c3b029e7bffd 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -589,9 +589,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 679f08c944ef..5ed2f73a7ea9 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -1707,10 +1707,8 @@ static void valleyview_pipestat_irq_handler(struct
>> drm_device *dev, u32 iir)
>>  		    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);
>> @@ -2109,10 +2107,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 */
>> @@ -2156,10 +2152,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 */
>> @@ -2363,10 +2357,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);
>> @@ -4024,7 +4016,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;
>>  
>> @@ -4215,7 +4206,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 618e034a7a5e..dc42335409b3 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -3101,7 +3101,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);
>>  	}
>>  }
>> @@ -10925,53 +10924,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;
>> -
>> -	/* Ensure we don't miss a work->pending update ... */
>> -	smp_rmb();
> If this is not needed anymore, should the matching smp_wmb() from 
> intel_mark_page_flip_active() be removed?
>
>
>> -
>> -	if (work == NULL || atomic_read(&work->pending) <
>> INTEL_FLIP_COMPLETE) {
>> -		spin_unlock_irqrestore(&dev->event_lock, flags);
>> -		return;
>> -	}
>> -
>> -	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)
>>  {
>> @@ -11024,28 +10976,52 @@ 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_INACTIVE ||
>> +	   !page_flip_finished(intel_crtc)) {
> This line is misaligned. Also, if the condition is was inverted you could save
> the extra unlock and return.
>
>
> Ander
>
>> +		spin_unlock_irqrestore(&dev->event_lock, flags);
>> +		return;
>> +	}
>> +
>> +	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);
>> +}
>> +
>>  static inline void intel_mark_page_flip_active(struct intel_unpin_work *work)
>>  {
>>  	/* Ensure that the work item is consistent when activating it ... */
>> @@ -11495,8 +11471,8 @@ static bool __intel_pageflip_stall_check(struct
>> drm_device *dev,
>>  	u32 pending;
>>  
>>  	pending = atomic_read(&work->pending);
>> -	if (pending != INTEL_FLIP_PENDING)
>> -		return pending == INTEL_FLIP_COMPLETE;
>> +	if (pending == INTEL_FLIP_INACTIVE)
>> +		return 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 ce2a6d985bbe..a74edecfd179 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -946,7 +946,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;
>> @@ -1159,7 +1158,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);



More information about the dri-devel mailing list