Re: [PATCH 2/2] drm/i915: Remove delayed FBC activation.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Op 26-06-18 om 19:59 schreef Ville Syrjälä:
> On Mon, Jun 25, 2018 at 06:37:58PM +0200, Maarten Lankhorst wrote:
>> The only time we should start FBC is when we have waited a vblank
>> after the atomic update.
> What about front buffer tracking? Is this going to leave FBC permanently
> disabled unless there are flips/plane updates?
No, see intel_fbc_flush. If there's a race with frontbuffer tracking and page flip,
we will not enable FBC in intel_fbc_flush(), but in that case we would enable it from
intel_fbc_post_update().

Or the other way around, intel_fbc_post_update won't enable FBC if the fb is dirty, but
intel_fbc_flush() afterwards will.
> I think there are a few cases we need to consider:
> 1. plane update which doesn't need fbc disable
> 2. plane update which needs to disable fbc but can re-enable it after the flip
>    has happended (eg. need to reallocate the cfb due to plane size/format change)
> 3. plane update which needs to disable fbc and can't re-enable it (eg. the new
>    plane state no longer fbc compatible)
> 4. front buffer invalidate + flush
>
> HW nuke will handle case 1. Case 2 could do the fbc re-enable after the
> post-flip vblank wait. Case 3 would ideally let us move FBC to another
> plane (thinking of pre-HSW hardware here). And case 4 must re-enable fbc
> after the flush has happened.
I don't see how this code will break any case. :)

~Maarten

>> We've already forced a vblank wait by doing
>> wait_for_flip_done before intel_post_plane_update(), so we don't need
>> to wait a second time before enabling.
>>
>> Removing the worker simplifies the code and removes possible race
>> conditions, like happening in 103167.
>>
>> Cc: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
>> Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103167
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
>> ---
>>  drivers/gpu/drm/i915/i915_debugfs.c |  5 --
>>  drivers/gpu/drm/i915/i915_drv.h     |  6 --
>>  drivers/gpu/drm/i915/intel_fbc.c    | 96 +----------------------------
>>  3 files changed, 1 insertion(+), 106 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> index c400f42a54ec..48a57c0636bf 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -1659,11 +1659,6 @@ static int i915_fbc_status(struct seq_file *m, void *unused)
>>  	else
>>  		seq_printf(m, "FBC disabled: %s\n", fbc->no_fbc_reason);
>>  
>> -	if (fbc->work.scheduled)
>> -		seq_printf(m, "FBC worker scheduled on vblank %llu, now %llu\n",
>> -			   fbc->work.scheduled_vblank,
>> -			   drm_crtc_vblank_count(&fbc->crtc->base));
>> -
>>  	if (intel_fbc_is_active(dev_priv)) {
>>  		u32 mask;
>>  
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 328d4312c438..9645dcb30454 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -580,12 +580,6 @@ struct intel_fbc {
>>  		unsigned int gen9_wa_cfb_stride;
>>  	} params;
>>  
>> -	struct intel_fbc_work {
>> -		bool scheduled;
>> -		u64 scheduled_vblank;
>> -		struct work_struct work;
>> -	} work;
>> -
>>  	const char *no_fbc_reason;
>>  };
>>  
>> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
>> index 9f9ea0b5452f..01d1d2088f04 100644
>> --- a/drivers/gpu/drm/i915/intel_fbc.c
>> +++ b/drivers/gpu/drm/i915/intel_fbc.c
>> @@ -399,89 +399,6 @@ bool intel_fbc_is_active(struct drm_i915_private *dev_priv)
>>  	return dev_priv->fbc.active;
>>  }
>>  
>> -static void intel_fbc_work_fn(struct work_struct *__work)
>> -{
>> -	struct drm_i915_private *dev_priv =
>> -		container_of(__work, struct drm_i915_private, fbc.work.work);
>> -	struct intel_fbc *fbc = &dev_priv->fbc;
>> -	struct intel_fbc_work *work = &fbc->work;
>> -	struct intel_crtc *crtc = fbc->crtc;
>> -	struct drm_vblank_crtc *vblank = &dev_priv->drm.vblank[crtc->pipe];
>> -
>> -	if (drm_crtc_vblank_get(&crtc->base)) {
>> -		/* CRTC is now off, leave FBC deactivated */
>> -		mutex_lock(&fbc->lock);
>> -		work->scheduled = false;
>> -		mutex_unlock(&fbc->lock);
>> -		return;
>> -	}
>> -
>> -retry:
>> -	/* Delay the actual enabling to let pageflipping cease and the
>> -	 * display to settle before starting the compression. Note that
>> -	 * this delay also serves a second purpose: it allows for a
>> -	 * vblank to pass after disabling the FBC before we attempt
>> -	 * to modify the control registers.
>> -	 *
>> -	 * WaFbcWaitForVBlankBeforeEnable:ilk,snb
>> -	 *
>> -	 * It is also worth mentioning that since work->scheduled_vblank can be
>> -	 * updated multiple times by the other threads, hitting the timeout is
>> -	 * not an error condition. We'll just end up hitting the "goto retry"
>> -	 * case below.
>> -	 */
>> -	wait_event_timeout(vblank->queue,
>> -		drm_crtc_vblank_count(&crtc->base) != work->scheduled_vblank,
>> -		msecs_to_jiffies(50));
>> -
>> -	mutex_lock(&fbc->lock);
>> -
>> -	/* Were we cancelled? */
>> -	if (!work->scheduled)
>> -		goto out;
>> -
>> -	/* Were we delayed again while this function was sleeping? */
>> -	if (drm_crtc_vblank_count(&crtc->base) == work->scheduled_vblank) {
>> -		mutex_unlock(&fbc->lock);
>> -		goto retry;
>> -	}
>> -
>> -	intel_fbc_hw_activate(dev_priv);
>> -
>> -	work->scheduled = false;
>> -
>> -out:
>> -	mutex_unlock(&fbc->lock);
>> -	drm_crtc_vblank_put(&crtc->base);
>> -}
>> -
>> -static void intel_fbc_schedule_activation(struct intel_crtc *crtc)
>> -{
>> -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> -	struct intel_fbc *fbc = &dev_priv->fbc;
>> -	struct intel_fbc_work *work = &fbc->work;
>> -
>> -	WARN_ON(!mutex_is_locked(&fbc->lock));
>> -	if (WARN_ON(!fbc->enabled))
>> -		return;
>> -
>> -	if (drm_crtc_vblank_get(&crtc->base)) {
>> -		DRM_ERROR("vblank not available for FBC on pipe %c\n",
>> -			  pipe_name(crtc->pipe));
>> -		return;
>> -	}
>> -
>> -	/* It is useless to call intel_fbc_cancel_work() or cancel_work() in
>> -	 * this function since we're not releasing fbc.lock, so it won't have an
>> -	 * opportunity to grab it to discover that it was cancelled. So we just
>> -	 * update the expected jiffy count. */
>> -	work->scheduled = true;
>> -	work->scheduled_vblank = drm_crtc_vblank_count(&crtc->base);
>> -	drm_crtc_vblank_put(&crtc->base);
>> -
>> -	schedule_work(&work->work);
>> -}
>> -
>>  static void intel_fbc_deactivate(struct drm_i915_private *dev_priv,
>>  				 const char *reason)
>>  {
>> @@ -489,11 +406,6 @@ static void intel_fbc_deactivate(struct drm_i915_private *dev_priv,
>>  
>>  	WARN_ON(!mutex_is_locked(&fbc->lock));
>>  
>> -	/* Calling cancel_work() here won't help due to the fact that the work
>> -	 * function grabs fbc->lock. Just set scheduled to false so the work
>> -	 * function can know it was cancelled. */
>> -	fbc->work.scheduled = false;
>> -
>>  	if (fbc->active)
>>  		intel_fbc_hw_deactivate(dev_priv);
>>  
>> @@ -1005,7 +917,7 @@ static void __intel_fbc_post_update(struct intel_crtc *crtc)
>>  
>>  	if (!fbc->busy_bits) {
>>  		intel_fbc_deactivate(dev_priv, "FBC enabled (active or scheduled)");
>> -		intel_fbc_schedule_activation(crtc);
>> +		intel_fbc_hw_activate(dev_priv);
>>  	} else
>>  		intel_fbc_deactivate(dev_priv, "frontbuffer write");
>>  }
>> @@ -1212,8 +1124,6 @@ void intel_fbc_disable(struct intel_crtc *crtc)
>>  	if (fbc->crtc == crtc)
>>  		__intel_fbc_disable(dev_priv);
>>  	mutex_unlock(&fbc->lock);
>> -
>> -	cancel_work_sync(&fbc->work.work);
>>  }
>>  
>>  /**
>> @@ -1235,8 +1145,6 @@ void intel_fbc_global_disable(struct drm_i915_private *dev_priv)
>>  		__intel_fbc_disable(dev_priv);
>>  	}
>>  	mutex_unlock(&fbc->lock);
>> -
>> -	cancel_work_sync(&fbc->work.work);
>>  }
>>  
>>  static void intel_fbc_underrun_work_fn(struct work_struct *work)
>> @@ -1387,12 +1295,10 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
>>  {
>>  	struct intel_fbc *fbc = &dev_priv->fbc;
>>  
>> -	INIT_WORK(&fbc->work.work, intel_fbc_work_fn);
>>  	INIT_WORK(&fbc->underrun_work, intel_fbc_underrun_work_fn);
>>  	mutex_init(&fbc->lock);
>>  	fbc->enabled = false;
>>  	fbc->active = false;
>> -	fbc->work.scheduled = false;
>>  
>>  	if (need_fbc_vtd_wa(dev_priv))
>>  		mkwrite_device_info(dev_priv)->has_fbc = false;
>> -- 
>> 2.18.0
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx


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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux