[PATCH v2] drm/i915/execlists: Set queue priority from secondary port

[PATCH v2] drm/i915/execlists: Set queue priority from secondary port

[Chris Wilson]

We can refine our current execlists->queue_priority if we inspect
ELSP[1] rather than the head of the unsubmitted queue. Currently, we use
the unsubmitted queue and say that if a subsequent request is more
important than the current queue, we will rerun the submission tasklet
to evaluate the need for preemption. However, we only want to preempt if
we need to jump ahead of a currently executing request in ELSP. The
second reason for running the submission tasklet is amalgamate requests
into the active context on ELSP[0] to avoid a stall when ELSP[0] drains.
(Though repeatedly amalgamating requests into the active context and
triggering many lite-restore is off question gain, the goal really is to
put a context into ELSP[1] to cover the interrupt.) So if instead of
looking at the head of the queue, we look at the context in ELSP[1] we
can answer both of the questions more accurately -- we don't need to
rerun the submission tasklet unless our new request is important enough
to feed into, at least, ELSP[1].

v2: Add some comments from the discussion with Tvrtko.

References: f6322eddaff7 ("drm/i915/preemption: Allow preemption between submission ports")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_engine_cs.c |  3 +++
 drivers/gpu/drm/i915/intel_lrc.c       | 16 +++++++++++++++-
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index 12486d8f534b..a217b3fe5f0b 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -1687,6 +1687,9 @@ void intel_engines_park(struct drm_i915_private *i915)
 			intel_engine_dump(engine, &p, NULL);
 		}
 
+		/* Must be reset upon idling, or we may miss the busy wakeup. */
+		GEM_BUG_ON(engine->execlists.queue_priority != INT_MIN);
+
 		if (engine->park)
 			engine->park(engine);
 
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 02b25bf2378a..84c29c7b0183 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -713,8 +713,22 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 		if (p->priority != I915_PRIORITY_NORMAL)
 			kmem_cache_free(engine->i915->priorities, p);
 	}
+
 done:
-	execlists->queue_priority = rb ? to_priolist(rb)->priority : INT_MIN;
+	/*
+	 * Here be a bit of magic! Or sleight-of-hand, whichever you prefer.
+	 *
+	 * We choose queue_priority such that if we add a request of greater
+	 * priority than this, we kick the submission tasklet to decide on
+	 * the right order of submitting the requests to hardware. We must
+	 * also be prepared to reorder requests as they are in-flight on the
+	 * HW. We derive the queue_priority then as the first "hole" in
+	 * the HW submission ports and if there are no available slots,
+	 * the priority of the lowest executing request, i.e. last.
+	 */
+	execlists->queue_priority =
+		port != execlists->port ? rq_prio(last) : INT_MIN;
+
 	execlists->first = rb;
 	if (submit)
 		port_assign(port, last);
-- 
2.17.0

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

Re: [PATCH v2] drm/i915/execlists: Set queue priority from secondary port

[Chris Wilson]

Quoting Chris Wilson (2018-04-11 10:55:30)
> We can refine our current execlists->queue_priority if we inspect
> ELSP[1] rather than the head of the unsubmitted queue. Currently, we use
> the unsubmitted queue and say that if a subsequent request is more
> important than the current queue, we will rerun the submission tasklet
> to evaluate the need for preemption. However, we only want to preempt if
> we need to jump ahead of a currently executing request in ELSP. The
> second reason for running the submission tasklet is amalgamate requests
> into the active context on ELSP[0] to avoid a stall when ELSP[0] drains.
> (Though repeatedly amalgamating requests into the active context and
> triggering many lite-restore is off question gain, the goal really is to
> put a context into ELSP[1] to cover the interrupt.) So if instead of
> looking at the head of the queue, we look at the context in ELSP[1] we
> can answer both of the questions more accurately -- we don't need to
> rerun the submission tasklet unless our new request is important enough
> to feed into, at least, ELSP[1].
> 
> v2: Add some comments from the discussion with Tvrtko.
> 
> References: f6322eddaff7 ("drm/i915/preemption: Allow preemption between submission ports")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Aieee, I've realised that the test cases that demonstrated the bug for
the original patch still haven't been reviewed!
-Chris

> ---
>  drivers/gpu/drm/i915/intel_engine_cs.c |  3 +++
>  drivers/gpu/drm/i915/intel_lrc.c       | 16 +++++++++++++++-
>  2 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 12486d8f534b..a217b3fe5f0b 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -1687,6 +1687,9 @@ void intel_engines_park(struct drm_i915_private *i915)
>                         intel_engine_dump(engine, &p, NULL);
>                 }
>  
> +               /* Must be reset upon idling, or we may miss the busy wakeup. */
> +               GEM_BUG_ON(engine->execlists.queue_priority != INT_MIN);
> +
>                 if (engine->park)
>                         engine->park(engine);
>  
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 02b25bf2378a..84c29c7b0183 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -713,8 +713,22 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>                 if (p->priority != I915_PRIORITY_NORMAL)
>                         kmem_cache_free(engine->i915->priorities, p);
>         }
> +
>  done:
> -       execlists->queue_priority = rb ? to_priolist(rb)->priority : INT_MIN;
> +       /*
> +        * Here be a bit of magic! Or sleight-of-hand, whichever you prefer.
> +        *
> +        * We choose queue_priority such that if we add a request of greater
> +        * priority than this, we kick the submission tasklet to decide on
> +        * the right order of submitting the requests to hardware. We must
> +        * also be prepared to reorder requests as they are in-flight on the
> +        * HW. We derive the queue_priority then as the first "hole" in
> +        * the HW submission ports and if there are no available slots,
> +        * the priority of the lowest executing request, i.e. last.
> +        */
> +       execlists->queue_priority =
> +               port != execlists->port ? rq_prio(last) : INT_MIN;
> +
>         execlists->first = rb;
>         if (submit)
>                 port_assign(port, last);
> -- 
> 2.17.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915/execlists: Set queue priority from secondary port

[Chris Wilson]

Quoting Chris Wilson (2018-04-11 11:39:29)
> We can refine our current execlists->queue_priority if we inspect
> ELSP[1] rather than the head of the unsubmitted queue. Currently, we use
> the unsubmitted queue and say that if a subsequent request is more
> important than the current queue, we will rerun the submission tasklet
> to evaluate the need for preemption. However, we only want to preempt if
> we need to jump ahead of a currently executing request in ELSP. The
> second reason for running the submission tasklet is amalgamate requests
> into the active context on ELSP[0] to avoid a stall when ELSP[0] drains.
> (Though repeatedly amalgamating requests into the active context and
> triggering many lite-restore is off question gain, the goal really is to
> put a context into ELSP[1] to cover the interrupt.) So if instead of
> looking at the head of the queue, we look at the context in ELSP[1] we
> can answer both of the questions more accurately -- we don't need to
> rerun the submission tasklet unless our new request is important enough
> to feed into, at least, ELSP[1].
> 
> v2: Add some comments from the discussion with Tvrtko.
> v3: More commentary to cross-reference queue_request()
> 
> References: f6322eddaff7 ("drm/i915/preemption: Allow preemption between submission ports")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

From the discussion thread,

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

I assume Tvrtko is happy with the comments added based on that
conversation.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915/execlists: Set queue priority from secondary port

[Tvrtko Ursulin]

On 11/04/2018 12:34, Chris Wilson wrote:
> Quoting Chris Wilson (2018-04-11 11:39:29)
>> We can refine our current execlists->queue_priority if we inspect
>> ELSP[1] rather than the head of the unsubmitted queue. Currently, we use
>> the unsubmitted queue and say that if a subsequent request is more
>> important than the current queue, we will rerun the submission tasklet
>> to evaluate the need for preemption. However, we only want to preempt if
>> we need to jump ahead of a currently executing request in ELSP. The
>> second reason for running the submission tasklet is amalgamate requests
>> into the active context on ELSP[0] to avoid a stall when ELSP[0] drains.
>> (Though repeatedly amalgamating requests into the active context and
>> triggering many lite-restore is off question gain, the goal really is to
>> put a context into ELSP[1] to cover the interrupt.) So if instead of
>> looking at the head of the queue, we look at the context in ELSP[1] we
>> can answer both of the questions more accurately -- we don't need to
>> rerun the submission tasklet unless our new request is important enough
>> to feed into, at least, ELSP[1].
>>
>> v2: Add some comments from the discussion with Tvrtko.
>> v3: More commentary to cross-reference queue_request()
>>
>> References: f6322eddaff7 ("drm/i915/preemption: Allow preemption between submission ports")
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Michał Winiarski <michal.winiarski@intel.com>
>> Cc: Michel Thierry <michel.thierry@intel.com>
>> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
>  From the discussion thread,
> 
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> I assume Tvrtko is happy with the comments added based on that
> conversation.

Yes, all fine!

Regards,

Tvrtko

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

✗ Fi.CI.CHECKPATCH: warning for drm/i915/execlists: Set queue priority from secondary port (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915/execlists: Set queue priority from secondary port (rev3)
URL   : https://patchwork.freedesktop.org/series/40869/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
7aa8f3485832 drm/i915/execlists: Set queue priority from secondary port
-:28: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#28: 
References: f6322eddaff7 ("drm/i915/preemption: Allow preemption between submission ports")

-:28: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit f6322eddaff7 ("drm/i915/preemption: Allow preemption between submission ports")'
#28: 
References: f6322eddaff7 ("drm/i915/preemption: Allow preemption between submission ports")

total: 1 errors, 1 warnings, 0 checks, 37 lines checked

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

✓ Fi.CI.BAT: success for drm/i915/execlists: Set queue priority from secondary port (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915/execlists: Set queue priority from secondary port (rev3)
URL   : https://patchwork.freedesktop.org/series/40869/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4045 -> Patchwork_8666 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8666/

== Known issues ==

  Here are the changes found in Patchwork_8666 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_exec_suspend@basic-s3:
      fi-cnl-y3:          NOTRUN -> INCOMPLETE (fdo#105086)

    
    ==== Possible fixes ====

    igt@gem_exec_fence@basic-await-default:
      fi-cnl-y3:          INCOMPLETE (fdo#105086) -> PASS

    igt@prime_vgem@basic-fence-flip:
      fi-pnv-d510:        FAIL (fdo#104008) -> PASS

    
  fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008
  fdo#105086 https://bugs.freedesktop.org/show_bug.cgi?id=105086


== Participating hosts (35 -> 32) ==

  Missing    (3): fi-ilk-m540 fi-bxt-dsi fi-skl-6700hq 


== Build changes ==

    * Linux: CI_DRM_4045 -> Patchwork_8666

  CI_DRM_4045: f736eddc7f0ba0eb82a43adc674d7eb2523e6de4 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4422: a914075d55dd089095121906bf4c3e825a3cecf2 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_8666: 7aa8f3485832f8c5ae0bdf3cff4075720146110e @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4422: 45e115f293fd6acc0c9647cf2d3b76be78819ba5 @ git://anongit.freedesktop.org/piglit


== Linux commits ==

7aa8f3485832 drm/i915/execlists: Set queue priority from secondary port

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8666/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for drm/i915/execlists: Set queue priority from secondary port (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915/execlists: Set queue priority from secondary port (rev3)
URL   : https://patchwork.freedesktop.org/series/40869/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4045_full -> Patchwork_8666_full =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_8666_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_8666_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8666/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_8666_full:

  === IGT changes ===

    ==== Warnings ====

    igt@pm_rc6_residency@rc6-accuracy:
      shard-snb:          PASS -> SKIP

    
== Known issues ==

  Here are the changes found in Patchwork_8666_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_flip@flip-vs-expired-vblank-interruptible:
      shard-hsw:          PASS -> FAIL (fdo#105189)

    
    ==== Possible fixes ====

    igt@kms_flip@2x-flip-vs-expired-vblank-interruptible:
      shard-hsw:          FAIL (fdo#102887) -> PASS

    igt@kms_flip@flip-vs-absolute-wf_vblank-interruptible:
      shard-hsw:          FAIL (fdo#103928) -> PASS

    igt@kms_sysfs_edid_timing:
      shard-apl:          WARN (fdo#100047) -> PASS

    
  fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047
  fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
  fdo#103928 https://bugs.freedesktop.org/show_bug.cgi?id=103928
  fdo#105189 https://bugs.freedesktop.org/show_bug.cgi?id=105189


== Participating hosts (6 -> 3) ==

  Missing    (3): shard-glk shard-glkb shard-kbl 


== Build changes ==

    * Linux: CI_DRM_4045 -> Patchwork_8666

  CI_DRM_4045: f736eddc7f0ba0eb82a43adc674d7eb2523e6de4 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4422: a914075d55dd089095121906bf4c3e825a3cecf2 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_8666: 7aa8f3485832f8c5ae0bdf3cff4075720146110e @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4422: 45e115f293fd6acc0c9647cf2d3b76be78819ba5 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8666/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915/execlists: Set queue priority from secondary port

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-04-11 13:03:00)
> 
> On 11/04/2018 12:34, Chris Wilson wrote:
> > Quoting Chris Wilson (2018-04-11 11:39:29)
> >> We can refine our current execlists->queue_priority if we inspect
> >> ELSP[1] rather than the head of the unsubmitted queue. Currently, we use
> >> the unsubmitted queue and say that if a subsequent request is more
> >> important than the current queue, we will rerun the submission tasklet
> >> to evaluate the need for preemption. However, we only want to preempt if
> >> we need to jump ahead of a currently executing request in ELSP. The
> >> second reason for running the submission tasklet is amalgamate requests
> >> into the active context on ELSP[0] to avoid a stall when ELSP[0] drains.
> >> (Though repeatedly amalgamating requests into the active context and
> >> triggering many lite-restore is off question gain, the goal really is to
> >> put a context into ELSP[1] to cover the interrupt.) So if instead of
> >> looking at the head of the queue, we look at the context in ELSP[1] we
> >> can answer both of the questions more accurately -- we don't need to
> >> rerun the submission tasklet unless our new request is important enough
> >> to feed into, at least, ELSP[1].
> >>
> >> v2: Add some comments from the discussion with Tvrtko.
> >> v3: More commentary to cross-reference queue_request()
> >>
> >> References: f6322eddaff7 ("drm/i915/preemption: Allow preemption between submission ports")
> >> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >> Cc: Michał Winiarski <michal.winiarski@intel.com>
> >> Cc: Michel Thierry <michel.thierry@intel.com>
> >> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> >> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > 
> >  From the discussion thread,
> > 
> > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > 
> > I assume Tvrtko is happy with the comments added based on that
> > conversation.
> 
> Yes, all fine!

Forgot to say, thanks and pushed.

If you are feeling better, there's the igt that purports to hit the issue
as well: https://patchwork.freedesktop.org/series/41535/
Or one of the Ms!
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx