From mboxrd@z Thu Jan 1 00:00:00 1970 From: Francisco Jerez Subject: Re: [PATCH] drm/i915/execlists: Track begin/end of execlists submission sequences Date: Thu, 29 Mar 2018 10:20:23 -0700 Message-ID: <87tvsyol2g.fsf@riseup.net> References: <20180329102631.11015-1-chris@chris-wilson.co.uk> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0912026713==" Return-path: Received: from mx1.riseup.net (mx1.riseup.net [198.252.153.129]) by gabe.freedesktop.org (Postfix) with ESMTPS id C3AE66E757 for ; Thu, 29 Mar 2018 17:35:54 +0000 (UTC) In-Reply-To: <20180329102631.11015-1-chris@chris-wilson.co.uk> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Chris Wilson , intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org --===============0912026713== Content-Type: multipart/signed; boundary="==-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" --==-=-= Content-Type: multipart/mixed; boundary="=-=-=" --=-=-= Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Chris Wilson writes: > We would like to start doing some bookkeeping at the beginning, between > contexts and at the end of execlists submission. We already mark the > beginning and end using EXECLISTS_ACTIVE_USER, to provide an indication > when the HW is idle. This give us a pair of sequence points we can then > expand on for further bookkeeping. > > Signed-off-by: Chris Wilson > Cc: Mika Kuoppala > Cc: Francisco Jerez > --- > drivers/gpu/drm/i915/intel_lrc.c | 42 ++++++++++++++++++++++++---= ------ > drivers/gpu/drm/i915/intel_ringbuffer.h | 11 ++++++++- > 2 files changed, 41 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/inte= l_lrc.c > index 654634254b64..61fb1387feb3 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -374,6 +374,19 @@ execlists_context_status_change(struct i915_request = *rq, unsigned long status) > status, rq); > } >=20=20 > +static inline void > +execlists_user_begin(struct intel_engine_execlists *execlists, > + const struct execlist_port *port) > +{ > + execlists_set_active_once(execlists, EXECLISTS_ACTIVE_USER); > +} > + > +static inline void > +execlists_user_end(struct intel_engine_execlists *execlists) > +{ > + execlists_clear_active(execlists, EXECLISTS_ACTIVE_USER); > +} > + > static inline void > execlists_context_schedule_in(struct i915_request *rq) > { > @@ -710,7 +723,7 @@ static void execlists_dequeue(struct intel_engine_cs = *engine) > spin_unlock_irq(&engine->timeline->lock); >=20=20 > if (submit) { > - execlists_set_active(execlists, EXECLISTS_ACTIVE_USER); > + execlists_user_begin(execlists, execlists->port); > execlists_submit_ports(engine); > } >=20=20 > @@ -741,7 +754,7 @@ execlists_cancel_port_requests(struct intel_engine_ex= eclists * const execlists) > port++; > } >=20=20 > - execlists_clear_active(execlists, EXECLISTS_ACTIVE_USER); > + execlists_user_end(execlists); > } >=20=20 > static void clear_gtiir(struct intel_engine_cs *engine) > @@ -872,7 +885,7 @@ static void execlists_submission_tasklet(unsigned lon= g data) > { > struct intel_engine_cs * const engine =3D (struct intel_engine_cs *)dat= a; > struct intel_engine_execlists * const execlists =3D &engine->execlists; > - struct execlist_port * const port =3D execlists->port; > + struct execlist_port *port =3D execlists->port; > struct drm_i915_private *dev_priv =3D engine->i915; > bool fw =3D false; >=20=20 > @@ -1010,9 +1023,19 @@ static void execlists_submission_tasklet(unsigned = long data) >=20=20 > GEM_BUG_ON(count =3D=3D 0); > if (--count =3D=3D 0) { > + /* > + * On the final event corresponding to the > + * submission of this context, we expect either > + * an element-switch event or a completion > + * event (and on completion, the active-idle > + * marker). No more preemptions, lite-restore > + * or otherwise Missing punctuation in the last sentence. > + */ > GEM_BUG_ON(status & GEN8_CTX_STATUS_PREEMPTED); > GEM_BUG_ON(port_isset(&port[1]) && > !(status & GEN8_CTX_STATUS_ELEMENT_SWITCH)); > + GEM_BUG_ON(!port_isset(&port[1]) && > + !(status & GEN8_CTX_STATUS_ACTIVE_IDLE)); > GEM_BUG_ON(!i915_request_completed(rq)); > execlists_context_schedule_out(rq); > trace_i915_request_out(rq); > @@ -1021,17 +1044,14 @@ static void execlists_submission_tasklet(unsigned= long data) > GEM_TRACE("%s completed ctx=3D%d\n", > engine->name, port->context_id); >=20=20 > - execlists_port_complete(execlists, port); > + port =3D execlists_port_complete(execlists, port); > + if (port_isset(port)) > + execlists_user_begin(execlists, port); > + else > + execlists_user_end(execlists); > } else { > port_set(port, port_pack(rq, count)); > } > - > - /* After the final element, the hw should be idle */ > - GEM_BUG_ON(port_count(port) =3D=3D 0 && > - !(status & GEN8_CTX_STATUS_ACTIVE_IDLE)); > - if (port_count(port) =3D=3D 0) > - execlists_clear_active(execlists, > - EXECLISTS_ACTIVE_USER); Do we want to update the EXECLISTS_ACTIVE_USER tracking done in intel_guc_submission.c to use the same wrappers? Either way looks okay to me: Reviewed-by: Francisco Jerez > } >=20=20 > if (head !=3D execlists->csb_head) { > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i9= 15/intel_ringbuffer.h > index a02c7b3b9d55..2e20627e254b 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h > @@ -638,6 +638,13 @@ execlists_set_active(struct intel_engine_execlists *= execlists, > __set_bit(bit, (unsigned long *)&execlists->active); > } >=20=20 > +static inline bool > +execlists_set_active_once(struct intel_engine_execlists *execlists, > + unsigned int bit) > +{ > + return !__test_and_set_bit(bit, (unsigned long *)&execlists->active); > +} > + > static inline void > execlists_clear_active(struct intel_engine_execlists *execlists, > unsigned int bit) > @@ -664,7 +671,7 @@ execlists_num_ports(const struct intel_engine_execlis= ts * const execlists) > return execlists->port_mask + 1; > } >=20=20 > -static inline void > +static inline struct execlist_port * > execlists_port_complete(struct intel_engine_execlists * const execlists, > struct execlist_port * const port) > { > @@ -675,6 +682,8 @@ execlists_port_complete(struct intel_engine_execlists= * const execlists, >=20=20 > memmove(port, port + 1, m * sizeof(struct execlist_port)); > memset(port + m, 0, sizeof(struct execlist_port)); > + > + return port; > } >=20=20 > static inline unsigned int > --=20 > 2.16.3 --=-=-=-- --==-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iHUEAREIAB0WIQST8OekYz69PM20/4aDmTidfVK/WwUCWr0gVwAKCRCDmTidfVK/ WwRsAP9KrqXDhr20WYngY8uZXWVqfp6b4oMtvGi6J7iL2/A7qAD+JndvhB/gPFe4 Scff9g2UMBFRYAtSzGeV+HHHKz8WxEg= =wwZj -----END PGP SIGNATURE----- --==-=-=-- --===============0912026713== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KSW50ZWwtZ2Z4 IG1haWxpbmcgbGlzdApJbnRlbC1nZnhAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vaW50ZWwtZ2Z4Cg== --===============0912026713==--