Re: [PATCH 3/5] drm/i915: Make wait_for_flips interruptible.

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 19-10-15 om 15:16 schreef Ander Conselvan De Oliveira:
> On Wed, 2015-09-23 at 13:27 +0200, Maarten Lankhorst wrote:
>> Move it from intel_crtc_atomic_commit to prepare_plane_fb.
>> Waiting is done before committing, otherwise it's too late
>> to undo the changes.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_atomic.c  |   2 -
>>  drivers/gpu/drm/i915/intel_display.c | 107 ++++++++++++++++++++--------------
>> -
>>  drivers/gpu/drm/i915/intel_drv.h     |   2 -
>>  3 files changed, 62 insertions(+), 49 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
>> b/drivers/gpu/drm/i915/intel_atomic.c
>> index f1975f267710..25a891aa3824 100644
>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>> @@ -205,8 +205,6 @@ int intel_atomic_setup_scalers(struct drm_device *dev,
>>  				 * but since this plane is unchanged just do
>> the
>>  				 * minimum required validation.
>>  				 */
>> -				if (plane->type == DRM_PLANE_TYPE_PRIMARY)
>> -					intel_crtc->atomic.wait_for_flips =
>> true;
>>  				crtc_state->base.planes_changed = true;
>>  			}
>>  
>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index 25e1eac260fd..cd651ff6c15b 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -3221,32 +3221,6 @@ void intel_finish_reset(struct drm_device *dev)
>>  	drm_modeset_unlock_all(dev);
>>  }
>>  
>> -static void
>> -intel_finish_fb(struct drm_framebuffer *old_fb)
>> -{
>> -	struct drm_i915_gem_object *obj = intel_fb_obj(old_fb);
>> -	struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
>> -	bool was_interruptible = dev_priv->mm.interruptible;
>> -	int ret;
>> -
>> -	/* Big Hammer, we also need to ensure that any pending
>> -	 * MI_WAIT_FOR_EVENT inside a user batch buffer on the
>> -	 * current scanout is retired before unpinning the old
>> -	 * framebuffer. Note that we rely on userspace rendering
>> -	 * into the buffer attached to the pipe they are waiting
>> -	 * on. If not, userspace generates a GPU hang with IPEHR
>> -	 * point to the MI_WAIT_FOR_EVENT.
>> -	 *
>> -	 * This should only fail upon a hung GPU, in which case we
>> -	 * can safely continue.
>> -	 */
>> -	dev_priv->mm.interruptible = false;
>> -	ret = i915_gem_object_wait_rendering(obj, true);
>> -	dev_priv->mm.interruptible = was_interruptible;
>> -
>> -	WARN_ON(ret);
>> -}
>> -
>>  static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
>>  {
>>  	struct drm_device *dev = crtc->dev;
>> @@ -3867,15 +3841,23 @@ static void page_flip_completed(struct intel_crtc
>> *intel_crtc)
>>  				 work->pending_flip_obj);
>>  }
>>  
>> -void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc)
>> +static int intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc)
>>  {
>>  	struct drm_device *dev = crtc->dev;
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>> +	long ret;
>>  
>>  	WARN_ON(waitqueue_active(&dev_priv->pending_flip_queue));
>> -	if (WARN_ON(wait_event_timeout(dev_priv->pending_flip_queue,
>> -				       !intel_crtc_has_pending_flip(crtc),
>> -				       60*HZ) == 0)) {
>> +
>> +	ret = wait_event_interruptible_timeout(
>> +					dev_priv->pending_flip_queue,
>> +					!intel_crtc_has_pending_flip(crtc),
>> +					60*HZ);
>> +
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	if (ret == 0) {
>>  		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>  
>>  		spin_lock_irq(&dev->event_lock);
>> @@ -3886,11 +3868,7 @@ void intel_crtc_wait_for_pending_flips(struct drm_crtc
>> *crtc)
>>  		spin_unlock_irq(&dev->event_lock);
>>  	}
>>  
>> -	if (crtc->primary->fb) {
>> -		mutex_lock(&dev->struct_mutex);
>> -		intel_finish_fb(crtc->primary->fb);
>> -		mutex_unlock(&dev->struct_mutex);
>> -	}
> There is another caller of intel_crtc_wait_for_pending_flips() besides the one
> touched in this patch: intel_crtc_disable_noatomic(). In your previous series
> you dropped that call based on the fact that there shouldn't be any pending
> flips at that point, but that patch has been dropped.
>
> Wouldn't it be better to add a WARN_ON as Chris suggested then instead of
> keeping the wait for flips but without the work around?
>
Yeah that would be a good idea. I'll fix up this patch.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx