[Intel-gfx] [PATCH] drm/i915: Queue page flip work with high priority

[Imre Deak]

On ti, 2016-09-13 at 11:32 +0100, Chris Wilson wrote:
> On Tue, Sep 13, 2016 at 11:24:52AM +0100, Tvrtko Ursulin wrote:
> > 
> > On 12/09/16 15:09, Imre Deak wrote:
> > > While user space has control over the scheduling priority of its
> > > page
> > > flipping thread, the corresponding work the driver schedules for
> > > MMIO
> > > flips always runs with normal scheduling priority. This would
> > > hinder an
> > > application that wants more stringent guarantees over flip timing
> > > (to
> > > avoid missing a flip at the next frame count).
> > > 
> > > Fix this by scheduling the work with high priority, meaning
> > > normal
> > > scheduling policy with -20 nice level.
> > > 
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97775
> > > Testcase: igt/kms_cursor_legacy
> > > CC: Chris Wilson <chris at chris-wilson.co.uk>
> > > CC: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > > Signed-off-by: Imre Deak <imre.deak at intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.c      | 7 +++++++
> > >  drivers/gpu/drm/i915/i915_drv.h      | 4 ++++
> > >  drivers/gpu/drm/i915/intel_display.c | 2 +-
> > >  3 files changed, 12 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > > b/drivers/gpu/drm/i915/i915_drv.c
> > > index 02c34d6..381ef23 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > @@ -756,8 +756,14 @@ static int i915_workqueues_init(struct
> > > drm_i915_private *dev_priv)
> > >  	if (dev_priv->hotplug.dp_wq == NULL)
> > >  		goto out_free_wq;
> > > 
> > > +	dev_priv->flip_wq = alloc_workqueue("i915-flip",
> > > WQ_HIGHPRI, 0);
> > > +	if (dev_priv->flip_wq == NULL)
> > > +		goto out_free_dp_wq;
> > > +
> > >  	return 0;
> > > 
> > > +out_free_dp_wq:
> > > +	destroy_workqueue(dev_priv->hotplug.dp_wq);
> > >  out_free_wq:
> > >  	destroy_workqueue(dev_priv->wq);
> > >  out_err:
> > > @@ -768,6 +774,7 @@ out_err:
> > > 
> > >  static void i915_workqueues_cleanup(struct drm_i915_private
> > > *dev_priv)
> > >  {
> > > +	destroy_workqueue(dev_priv->flip_wq);
> > >  	destroy_workqueue(dev_priv->hotplug.dp_wq);
> > >  	destroy_workqueue(dev_priv->wq);
> > >  }
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index f499fa5..3653ce4 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -1844,6 +1844,10 @@ struct drm_i915_private {
> > >  	 * result in deadlocks.
> > >  	 */
> > >  	struct workqueue_struct *wq;
> > > +	/**
> > > +	 * flip_wq - High priority flip workqueue.
> > > +	 */
> > > +	struct workqueue_struct *flip_wq;
> > > 
> > >  	/* Display functions */
> > >  	struct drm_i915_display_funcs display;
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index 3c367d0..48433e1 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -12278,7 +12278,7 @@ static int intel_crtc_page_flip(struct
> > > drm_crtc *crtc,
> > > 
> > >  		work->flip_queued_req =
> > > i915_gem_active_get(&obj->last_write,
> > >  							    &obj
> > > ->base.dev->struct_mutex);
> > > -		schedule_work(&work->mmio_work);
> > > +		queue_work(dev_priv->flip_wq, &work->mmio_work);
> > >  	} else {
> > >  		request = i915_gem_request_alloc(engine, engine-
> > > >last_context);
> > >  		if (IS_ERR(request)) {
> > > 
> > 
> > I am curious if just a dedicated wq would be enough, or you have
> > found that it has to be a high-prio one?
> > 
> > Otherwise patch looks fine to me.
> 
> The problem is that this just the tip of the iceberg. mmioflips will
> be
> driven by atomic in the near future. This isn't a viable solution as
> not
> everything in that work is suitable for high priority.

In what sense isn't it viable? The intention is to complete the work
before the next vblank, so it is by the definition a high-prio work.

--Imre