From mboxrd@z Thu Jan 1 00:00:00 1970 From: Francisco Jerez Subject: Re: [PATCHv7] drm/i915: Added Programming of the MOCS Date: Wed, 08 Jul 2015 15:50:06 +0300 Message-ID: <87bnfmstj5.fsf@riseup.net> References: <1436296381-1174-1-git-send-email-currojerez@riseup.net> <20150707214630.GB24266@nuc-i3427.alporthouse.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1717809369==" Return-path: Received: from mx1.riseup.net (mx1.riseup.net [198.252.153.129]) by gabe.freedesktop.org (Postfix) with ESMTPS id 22FD06E304 for ; Wed, 8 Jul 2015 05:50:32 -0700 (PDT) In-Reply-To: <20150707214630.GB24266@nuc-i3427.alporthouse.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Chris Wilson Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org --===============1717809369== 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: > On Tue, Jul 07, 2015 at 10:13:01PM +0300, Francisco Jerez wrote: >> From: Peter Antoine >>=20 >> This change adds the programming of the MOCS registers to the gen 9+ >> platforms. This change set programs the MOCS register values to a set >> of values that are defined to be optimal. > > If they were optimal why weren't they defaults? I'm not feeling very > satisfied by this explanation. > Hah, yeah, they are definitely not optimal, it's just that I didn't feel fully confident modifying the original message above Peter's Signed-off line. How about we replace that sentence with: "The set of MOCS configuration entries introduced by this patch is intended to be minimal but sufficient to cover the needs of current userspace, it's expected to be extended in the future based on the userspace demand for additional caching configurations." >> +/** >> + * get_mocs_settings >> + * >> + * This function will return the values of the MOCS table that needs to >> + * be programmed for the platform. It will return the values that need >> + * to be programmed and if they need to be programmed. >> + * >> + * If the return values is false then the registers do not need program= ming. >> + */ >> +static bool get_mocs_settings(struct drm_device *dev, >> + struct drm_i915_mocs_table *table) >> +{ >> + bool result =3D false; > > This looks easy enough to extend to get_mocs_settings(ctx, &table) and > use CONTEXT_SET_PARAM to load a user defined mocs table. These defaults > have the smell of policy in the era where even CPU PAT are becoming user > defined (with per-process definitions). Is it worth shifting these to > userspace? Having sane/safe defaults is good definitely. > Yeah, I also have the feeling that we may want to switch to some dynamic set-up scheme in the future. With only three of the available 62 entries in use right now it's likely to take a *long* time until we run out of entries from the global tables though, when (if) that happens we can always switch to dynamic set-up via some new IOCTL without disrupting older userspace relying on the fixed MOCS defaults. Implementing dynamic set-up is less straightforward than it may seem though, because only the MOCS tables for some of the engines are context saved and restored automatically by the hardware, so the settings specified by one context would partially leak into other applications, unless we context-switch the remaining engines manually from the kernel. > The changelog should be more explicit on what you mean by optimal and > why other avenues are not pursued. > >> +/** >> + * emit_mocs_control_table() - emit the mocs control table >> + * @req: Request to set up the MOCS table for. >> + * @table: The values to program into the control regs. >> + * @reg_base: The base for the engine that needs to be programmed. >> + * >> + * This function simply emits a MI_LOAD_REGISTER_IMM command for the >> + * given table starting at the given address. >> + * >> + * @return 0 on success, otherwise the error status. >> + */ >> +static int emit_mocs_control_table(struct drm_i915_gem_request *req, >> + const struct drm_i915_mocs_table *table, >> + u32 reg_base) >> +{ >> + struct intel_ringbuffer *ringbuf =3D req->ringbuf; >> + unsigned int index; >> + int ret; >> + >> + ret =3D intel_logical_ring_begin(req, 2 + 2 * GEN9_NUM_MOCS_ENTRIES); >> + if (ret) { >> + DRM_DEBUG("intel_logical_ring_begin failed %d\n", ret); >> + return ret; >> + } > > Much happier now. I don't have to jump back and forth to check you have > reserved the correct amount of space. > > One last thing to do would be > > if (WARN_ON(table->size > GEN9_NUM_MOCS_ENTRIES)) > return -ENODEV; > > It's nice here as we have the two loops and this sanity check helps > explain the relationship between those loops and the ring emission. But > equally you may feel that doing that check in get_mocs_table() (or just > after) is sufficient. It needs to be done somewhere though. > > (If you go with adding the sanity check here, it should also be done in=20 > emit_mocs_l3cc_table.) OK, I'll fix that and resend. Thanks! > > Considering that's my only real critique, pretty good! > -Chris > > --=20 > Chris Wilson, Intel Open Source Technology Centre --=-=-=-- --==-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iF4EAREIAAYFAlWdHH4ACgkQg5k4nX1Sv1vapgD/Y5PdOBGl0f88gaYrh06qDvWb 2nslWkeo+Rxu43GeFlQA/1dnY80ShlpVzIz8U1Smi1VhE7xw9hsv2JTEG9tfq/MO =Ka+o -----END PGP SIGNATURE----- --==-=-=-- --===============1717809369== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KSW50ZWwtZ2Z4 IG1haWxpbmcgbGlzdApJbnRlbC1nZnhAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHA6Ly9saXN0 cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9pbnRlbC1nZngK --===============1717809369==--