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

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

Op 21-07-15 om 13:26 schreef Chris Wilson:
> On Thu, Jul 16, 2015 at 02:57:51PM +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 | 72 +++++++++++++++++-------------------
>>  drivers/gpu/drm/i915/intel_drv.h     |  2 -
>>  3 files changed, 33 insertions(+), 43 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
>> index e2531cf59266..09a0ad611002 100644
>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>> @@ -214,8 +214,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 5a462e9a4d14..8ef0dbb33afd 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -3250,32 +3250,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;
>> @@ -3890,15 +3864,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);
>> @@ -3909,11 +3891,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);
>> -	}
>> +	return 0;
>>  }
>>  
>>  /* Program iCLKIP clock to the desired frequency */
>> @@ -4773,9 +4751,6 @@ static void intel_pre_plane_update(struct intel_crtc *crtc)
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>  	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
>>  
>> -	if (atomic->wait_for_flips)
>> -		intel_crtc_wait_for_pending_flips(&crtc->base);
>> -
>>  	if (atomic->disable_fbc)
>>  		intel_fbc_disable_crtc(crtc);
>>  
>> @@ -11701,7 +11676,6 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>>  
>>  	switch (plane->type) {
>>  	case DRM_PLANE_TYPE_PRIMARY:
>> -		intel_crtc->atomic.wait_for_flips = true;
>>  		intel_crtc->atomic.pre_disable_primary = turn_off;
>>  		intel_crtc->atomic.post_enable_primary = turn_on;
>>  
>> @@ -13473,6 +13447,12 @@ intel_prepare_plane_fb(struct drm_plane *plane,
>>  	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->state->fb);
>>  	int ret = 0;
>>  
>> +	if (plane->type == DRM_PLANE_TYPE_PRIMARY && old_obj) {
> Would feel safer is we just asked if the CRTC had pending flips
> irrespective of old_obj. Do you plan on moving the pending flips from
> CRTC to plane? That would seem to be implied here.
This preserves old behavior since page flips can only happen on primary planes.
Do you want cursor updates to wait on pending primary flips?

>> +		ret = intel_crtc_wait_for_pending_flips(plane->state->crtc);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>>  	if (obj == old_obj)
>>  		return 0;
>>  
>> @@ -13492,6 +13472,20 @@ intel_prepare_plane_fb(struct drm_plane *plane,
>>  		ret = intel_pin_and_fence_fb_obj(plane, fb, new_state, NULL, NULL);
>>  	}
>>  
>> +	/* 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.
>> +	 */
>> +	if (!ret && plane->type == DRM_PLANE_TYPE_PRIMARY && old_obj)
>> +		ret = i915_gem_object_wait_rendering(old_obj, true);
> Technically I can create a batch with a WAIT_ON_EVENT for a secondary
> plane as well. This path need only be done in a modeset, not for a
> simple plane flip. (But we actually need to serialise with rendering to
> old_obj before regarding the plane flip as complete.) Is there a
> plane-prepare-modeset hook?
The comment didn't explain that, is hiding this behind a needs_modeset(crtc_state) sufficient here?

~Maarten

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx