[Intel-gfx] [PATCH 03/19] drm/i915: Remove intel_prepare_page_flip, v2.

[Maarten Lankhorst]

Op 26-04-16 om 01:14 schreef Patrik Jakobsson:
> 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 at linux.intel.com>
>> ...
>> @@ -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.
On earlier generations there was no fixed mapping, but for ilk+ yeah should be removable.
> Btw, intel_complete_page_flips() is only called from intel_finish_reset() so one
> could question it's usefulness as well.
intel_complete_page_flips can be removed later on too, it isn't required for mmio flips
since the requests will complete anyway.
>>  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.
I thought of re-using it when adding more than 1 flip to the queue, but I probably won't need it then.
Wouldn't be a bad idea to remove it. :)

~Maarten