Re: [PATCH] drm/i915: Queue page flip work with high priority

[Imre Deak <imre.deak@intel.com>]

On ti, 2016-09-13 at 11:24 +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@chris-wilson.co.uk>
> > CC: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Signed-off-by: Imre Deak <imre.deak@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?

I haven't tried a dedicated normal-prio wq. Right, another work in the
queue could also hold up this one, but the system_wq is unordered, so
that kind of dependency shouldn't be a problem if that's what you
meant.

> Otherwise patch looks fine to me.
> 
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Regards,
> 
> Tvrtko
> 
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx