From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kenneth Graunke Subject: Re: [PATCH 1/3] drm/i915: BUG_ON() when cmd/reg tables are not sorted Date: Thu, 27 Mar 2014 14:47:03 -0700 Message-ID: <53349C57.8090405@whitecape.org> References: <1395945820-20376-1-git-send-email-bradley.d.volkin@intel.com> <1395945820-20376-2-git-send-email-bradley.d.volkin@intel.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0466726701==" Return-path: Received: from homiemail-a99.g.dreamhost.com (caiajhbdcaib.dreamhost.com [208.97.132.81]) by gabe.freedesktop.org (Postfix) with ESMTP id F25636EB11 for ; Thu, 27 Mar 2014 14:46:20 -0700 (PDT) In-Reply-To: <1395945820-20376-2-git-send-email-bradley.d.volkin@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: bradley.d.volkin@intel.com, intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --===============0466726701== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="h2JPOfC9BgabL1rSTaGD4Gc0nrxH94Q8q" This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --h2JPOfC9BgabL1rSTaGD4Gc0nrxH94Q8q Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable On 03/27/2014 11:43 AM, bradley.d.volkin@intel.com wrote: > From: Brad Volkin >=20 > As suggested during review, this makes it much more obvious > when the tables are not sorted. >=20 > Cc: Jani Nikula > Signed-off-by: Brad Volkin > --- > drivers/gpu/drm/i915/i915_cmd_parser.c | 31 +++++++++++++++++++++-----= ----- > 1 file changed, 21 insertions(+), 10 deletions(-) >=20 > diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i= 915/i915_cmd_parser.c > index 788bd96..8a93db3 100644 > --- a/drivers/gpu/drm/i915/i915_cmd_parser.c > +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c > @@ -493,12 +493,13 @@ static u32 gen7_blt_get_cmd_length_mask(u32 cmd_h= eader) > return 0; > } > =20 > -static void validate_cmds_sorted(struct intel_ring_buffer *ring) > +static bool validate_cmds_sorted(struct intel_ring_buffer *ring) > { > int i; > + bool ret =3D true; > =20 > if (!ring->cmd_tables || ring->cmd_table_count =3D=3D 0) > - return; > + return true; > =20 > for (i =3D 0; i < ring->cmd_table_count; i++) { > const struct drm_i915_cmd_table *table =3D &ring->cmd_tables[i]; > @@ -510,35 +511,45 @@ static void validate_cmds_sorted(struct intel_rin= g_buffer *ring) > &table->table[i]; > u32 curr =3D desc->cmd.value & desc->cmd.mask; > =20 > - if (curr < previous) > + if (curr < previous) { > DRM_ERROR("CMD: table not sorted ring=3D%d table=3D%d entry=3D%d c= md=3D0x%08X prev=3D0x%08X\n", > ring->id, i, j, curr, previous); > + ret =3D false; > + } > =20 > previous =3D curr; > } > } > + > + return ret; > } > =20 > -static void check_sorted(int ring_id, const u32 *reg_table, int reg_co= unt) > +static bool check_sorted(int ring_id, const u32 *reg_table, int reg_co= unt) > { > int i; > u32 previous =3D 0; > + bool ret =3D true; > =20 > for (i =3D 0; i < reg_count; i++) { > u32 curr =3D reg_table[i]; > =20 > - if (curr < previous) > + if (curr < previous) { > DRM_ERROR("CMD: table not sorted ring=3D%d entry=3D%d reg=3D0x%08X = prev=3D0x%08X\n", > ring_id, i, curr, previous); > + ret =3D false; > + } > =20 > previous =3D curr; > } > + > + return ret; > } > =20 > -static void validate_regs_sorted(struct intel_ring_buffer *ring) > +static bool validate_regs_sorted(struct intel_ring_buffer *ring) > { > - check_sorted(ring->id, ring->reg_table, ring->reg_count); > - check_sorted(ring->id, ring->master_reg_table, ring->master_reg_count= ); > + return check_sorted(ring->id, ring->reg_table, ring->reg_count) && > + check_sorted(ring->id, ring->master_reg_table, > + ring->master_reg_count); > } > =20 > /** > @@ -617,8 +628,8 @@ void i915_cmd_parser_init_ring(struct intel_ring_bu= ffer *ring) > BUG(); > } > =20 > - validate_cmds_sorted(ring); > - validate_regs_sorted(ring); > + BUG_ON(!validate_cmds_sorted(ring)); > + BUG_ON(!validate_regs_sorted(ring)); > } > =20 > static const struct drm_i915_cmd_descriptor* Does any code actually rely on the tables being sorted? I didn't see any early breaks or returns once the parser has gone past a command...it seems to keep looking through the entire list. As an aside: It seems like find_cmd could be implemented a lot more efficiently. Currently, most 3DSTATE commands are not in the tables - they only appear to contain commands that need special handling. This means that find_cmd will walk through every command in every table before falling back to the default info struct. As an example, my window manager (KWin) submitted a render ring batch with 700 commands in it. Most of those commands need to walk through every command in every table, which is about 48 entries. That's 33,600 iterations through tables, which seems...kind of excessive. For example, 3D commands all have the form 0x78xx or 0x79xx. We could use the xx as an array index into a lookup table, and get O(1) access instead of O(n). Or, at least something similar. Thanks again for your work on this, Brad. --Ken --h2JPOfC9BgabL1rSTaGD4Gc0nrxH94Q8q Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJTNJxbAAoJEFtb2gcdScw4O4MP/1FLuCwBtSrLYOjGHMN48z7I 9aJgtKdBBDp5I4GmUMDcCUOzjBxv4hqgfXeL9x2Vlf+hmtVY+pyO7WWgIS6rqA33 gEUSDFU9yqsEFUf1j1w9PC6z+UgfPsLzOWGg954D2LGxcJ5TOOXuKgfLifREtw7r bICSybfFBp6p8eH3ntl7+nRqApHo6/IcfP8SJe/hld5VZT4TaCzFt+3Dxkj3bp6r km0PAJ78fU6qOy0kbaStU73O26oqolDTidN9nexZv/Z9s7O1cS+mBpzDm+tJcxv5 cSb1jGi3xxv65LunNg3wMlxZRGlszn2isNHov70MZY36BAFUGg9WcLXyf0uDX1zO zihzjUIuqSXI+aNk1L1KzgDQvHAi7tCa93RdXF7jljsP1KFzeHhTBdLpac2hRXgj abgH1WwpWTAyRliCRmN1q2oJBpTIdJYiUG8Rf7Pla7QIN8hMOJtm2obAvkCL0J2E RMzv2DZ3Wmq4gXtdhGOEqZhBvEeB8WtNA/mCTiIDtph+XqQNxj++yXsEaMN3OEO6 uX+ui2fJ7WqqEtLcVSF6uvDjyjYFC8ERCeJyzSGYmIMTNiTI05a8WEdxzG5CTNyA jbiCdvZZJIR1bIq8EsouTP9mE5MH/rh9p2zmdxOm5j6qEdMTLhPelimrd+uSbR6q WhABUlx1hM33jvh8Itf1 =U6QL -----END PGP SIGNATURE----- --h2JPOfC9BgabL1rSTaGD4Gc0nrxH94Q8q-- --===============0466726701== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx --===============0466726701==--