All of lore.kernel.org
 help / color / mirror / Atom feed
From: Francisco Jerez <currojerez@riseup.net>
To: David Weinehall <david.weinehall@linux.intel.com>,
	intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/i915: Update MOCS settings for gen 9
Date: Wed, 26 Apr 2017 10:25:20 -0700	[thread overview]
Message-ID: <877f27f5dr.fsf@riseup.net> (raw)
In-Reply-To: <20170426150041.15896-1-david.weinehall@linux.intel.com>


[-- Attachment #1.1.1: Type: text/plain, Size: 8800 bytes --]

Hi David,

David Weinehall <david.weinehall@linux.intel.com> writes:

> Add a bunch of MOCS entries for gen 9 that were missing from intel_mocs.
> Some of these are used by media-sdk; if these entries are missing
> the default will instead be to do everything uncached.
>

Keep in mind that MOCS entries are not for free -- Once you expose a new
entry it will become part of the kernel ABI and we won't ever be able to
re-use them for another purpose.  What this means is we probably don't
want to add a bunch of entries if only "some" of them are actually
useful to the open-source stack.  Some more comments below.

> This patch improves media-sdk performance with up to 60%
> with the (admittedly synthetic) benchmarks we use in our nightly
> testing, without regressing any other benchmarks.
>
> Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>
>
> diff --git a/drivers/gpu/drm/i915/intel_mocs.c b/drivers/gpu/drm/i915/intel_mocs.c
> index 92e461c68385..5236a442a14f 100644
> --- a/drivers/gpu/drm/i915/intel_mocs.c
> +++ b/drivers/gpu/drm/i915/intel_mocs.c
> @@ -103,7 +103,6 @@ static const struct drm_i915_mocs_entry skylake_mocs_table[] = {
>  			   LE_TGT_CACHE(LE_TC_LLC_ELLC) |
>  			   LE_LRUM(0) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) |
>  			   LE_PFM(0) | LE_SCF(0),
> -
>  	  /* 0x0010 */
>  	  .l3cc_value =    L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_UC),
>  	},
> @@ -123,7 +122,133 @@ static const struct drm_i915_mocs_entry skylake_mocs_table[] = {
>  			   LE_LRUM(3) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) |
>  			   LE_PFM(0) | LE_SCF(0),
>  	  /* 0x0030 */
> -	  .l3cc_value =   L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB),
> +	  .l3cc_value =    L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB),
> +	},
> +	{

Apparently somebody started defining symbolic enums for the current MOCS
entries in i915_drm.h, which I guess makes this more obviously tied to
the kernel ABI.  I suppose it wouldn't hurt if you did the same.

> +	  /* 0x00000037 */
> +	  .control_value = LE_CACHEABILITY(LE_WB) |
> +			   LE_TGT_CACHE(LE_TC_LLC) |
> +			   LE_LRUM(3) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) |
> +			   LE_PFM(0) | LE_SCF(0),
> +	  /* 0x0030 */
> +	  .l3cc_value =    L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB),
> +	},
> +	{
> +	  /* 0x00000039 */
> +	  .control_value = LE_CACHEABILITY(LE_UC) |
> +			   LE_TGT_CACHE(LE_TC_LLC_ELLC) |
> +			   LE_LRUM(3) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) |
> +			   LE_PFM(0) | LE_SCF(0),
> +	  /* 0x0030 */
> +	  .l3cc_value =    L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB),
> +	},
> +	{
> +	  /* 0x00000033 */
> +	  .control_value = LE_CACHEABILITY(LE_WB) |
> +			   LE_TGT_CACHE(LE_TC_PAGETABLE) |
> +			   LE_LRUM(3) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) |
> +			   LE_PFM(0) | LE_SCF(0),
> +	  /* 0x0030 */
> +	  .l3cc_value =    L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB),
> +	},

What's the use of an entry with PTE-controlled target cache if you're
overriding the cacheability control to WB anyway?  Seems effectively
redundant with the current I915_MOCS_CACHED entry.  Same for the
0x33:0x10, 0x13:0x10 and 0x3:0x10 entries below.

> +	{
> +	  /* 0x00000017 */
> +	  .control_value = LE_CACHEABILITY(LE_WB) |
> +			   LE_TGT_CACHE(LE_TC_LLC) |
> +			   LE_LRUM(1) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) |
> +			   LE_PFM(0) | LE_SCF(0),
> +	  /* 0x0030 */
> +	  .l3cc_value =    L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB),
> +	},
> +	{
> +	  /* 0x00000037 */
> +	  .control_value = LE_CACHEABILITY(LE_WB) |
> +			   LE_TGT_CACHE(LE_TC_LLC) |
> +			   LE_LRUM(3) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) |
> +			   LE_PFM(0) | LE_SCF(0),
> +	  /* 0x0010 */
> +	  .l3cc_value =    L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_UC),
> +	},
> +	{
> +	  /* 0x00000039 */
> +	  .control_value = LE_CACHEABILITY(LE_UC) |
> +			   LE_TGT_CACHE(LE_TC_LLC_ELLC) |
> +			   LE_LRUM(3) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) |
> +			   LE_PFM(0) | LE_SCF(0),
> +	  /* 0x0010 */
> +	  .l3cc_value =    L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_UC),
> +	},

This entry is UC in all caches and seems redundant with the current
I915_MOCS_UNCACHED entry.  Why do you need it?

> +	{
> +	  /* 0x00000017 */
> +	  .control_value = LE_CACHEABILITY(LE_WB) |
> +			   LE_TGT_CACHE(LE_TC_LLC) |
> +			   LE_LRUM(1) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) |
> +			   LE_PFM(0) | LE_SCF(0),
> +	  /* 0x0010 */
> +	  .l3cc_value =    L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_UC),
> +	},
> +	{
> +	  /* 0x0000003b */
> +	  .control_value = LE_CACHEABILITY(LE_WB) |
> +			   LE_TGT_CACHE(LE_TC_LLC_ELLC) |
> +			   LE_LRUM(3) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) |
> +			   LE_PFM(0) | LE_SCF(0),
> +	  /* 0x0010 */
> +	  .l3cc_value =    L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_UC),
> +	},
> +	{
> +	  /* 0x00000033 */
> +	  .control_value = LE_CACHEABILITY(LE_WB) |
> +			   LE_TGT_CACHE(LE_TC_PAGETABLE) |
> +			   LE_LRUM(3) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) |
> +			   LE_PFM(0) | LE_SCF(0),
> +	  /* 0x0010 */
> +	  .l3cc_value =    L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_UC),
> +	},
> +	{
> +	  /* 0x00000019 */
> +	  .control_value = LE_CACHEABILITY(LE_UC) |
> +			   LE_TGT_CACHE(LE_TC_LLC_ELLC) |
> +			   LE_LRUM(1) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) |
> +			   LE_PFM(0) | LE_SCF(0),
> +	  /* 0x0010 */
> +	  .l3cc_value =    L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_UC),
> +	},

Another entry marked UC for all caches.

> +	{
> +	  /* 0x00000003 */
> +	  .control_value = LE_CACHEABILITY(LE_WB) |
> +			   LE_TGT_CACHE(LE_TC_PAGETABLE) |
> +			   LE_LRUM(0) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) |
> +			   LE_PFM(0) | LE_SCF(0),
> +	  /* 0x0010 */
> +	  .l3cc_value =    L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_UC),
> +	},
> +	{
> +	  /* 0x0000001b */
> +	  .control_value = LE_CACHEABILITY(LE_WB) |
> +			   LE_TGT_CACHE(LE_TC_LLC_ELLC) |
> +			   LE_LRUM(1) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) |
> +			   LE_PFM(0) | LE_SCF(0),
> +	  /* 0x0010 */
> +	  .l3cc_value =    L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_UC),
> +	},
> +	{
> +	  /* 0x0000001b */
> +	  .control_value = LE_CACHEABILITY(LE_WB) |
> +			   LE_TGT_CACHE(LE_TC_LLC_ELLC) |
> +			   LE_LRUM(1) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) |
> +			   LE_PFM(0) | LE_SCF(0),
> +	  /* 0x0030 */
> +	  .l3cc_value =    L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB),
> +	},
> +	{
> +	  /* 0x00000013 */
> +	  .control_value = LE_CACHEABILITY(LE_WB) |
> +			   LE_TGT_CACHE(LE_TC_PAGETABLE) |
> +			   LE_LRUM(1) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) |
> +			   LE_PFM(0) | LE_SCF(0),
> +	  /* 0x0010 */
> +	  .l3cc_value =    L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_UC),
>  	},
>  };
>
> @@ -135,7 +260,6 @@ static const struct drm_i915_mocs_entry broxton_mocs_table[] = {
>  			   LE_TGT_CACHE(LE_TC_LLC_ELLC) |
>  			   LE_LRUM(0) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) |
>  			   LE_PFM(0) | LE_SCF(0),
> -
>  	  /* 0x0010 */
>  	  .l3cc_value =    L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_UC),
>  	},
> @@ -145,7 +269,6 @@ static const struct drm_i915_mocs_entry broxton_mocs_table[] = {
>  			   LE_TGT_CACHE(LE_TC_LLC_ELLC) |
>  			   LE_LRUM(3) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) |
>  			   LE_PFM(0) | LE_SCF(0),
> -
>  	  /* 0x0030 */
>  	  .l3cc_value =    L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB),
>  	},
> @@ -155,10 +278,36 @@ static const struct drm_i915_mocs_entry broxton_mocs_table[] = {
>  			   LE_TGT_CACHE(LE_TC_LLC_ELLC) |
>  			   LE_LRUM(3) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) |
>  			   LE_PFM(0) | LE_SCF(0),
> -
>  	  /* 0x0030 */
>  	  .l3cc_value =    L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB),
>  	},
> +	{
> +	  /* 0x00000005 */
> +	  .control_value = LE_CACHEABILITY(LE_UC) |
> +			   LE_TGT_CACHE(LE_TC_LLC) |
> +			   LE_LRUM(0) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) |
> +			   LE_PFM(0) | LE_SCF(0),
> +	  /* 0x0030 */
> +	  .l3cc_value =    L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB),
> +	},
> +	{
> +	  /* 0x00000001 */
> +	  .control_value = LE_CACHEABILITY(LE_UC) |
> +			   LE_TGT_CACHE(LE_TC_PAGETABLE) |
> +			   LE_LRUM(0) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) |
> +			   LE_PFM(0) | LE_SCF(0),
> +	  /* 0x0010 */
> +	  .l3cc_value =    L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_UC),
> +	},
> +	{
> +	  /* 0x00000005 */
> +	  .control_value = LE_CACHEABILITY(LE_UC) |
> +			   LE_TGT_CACHE(LE_TC_LLC) |
> +			   LE_LRUM(0) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) |
> +			   LE_PFM(0) | LE_SCF(0),
> +	  /* 0x0010 */
> +	  .l3cc_value =    L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_UC),
> +	},
>  };
>
>  /**
> ---
>  drivers/gpu/drm/i915/intel_mocs.c | 159 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 154 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_mocs.c b/drivers/gpu/drm/i915/intel_mocs.c
> index 92e461c68385..5236a442a14f 100644

Huh?  Duplicate patch follows?

> [...]

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 212 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

  reply	other threads:[~2017-04-26 17:34 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-04-26 15:00 [PATCH] drm/i915: Update MOCS settings for gen 9 David Weinehall
2017-04-26 17:25 ` Francisco Jerez [this message]
2017-04-27 14:55 ` Arkadiusz Hiler
2017-04-27 15:30   ` David Weinehall
2017-04-27 16:23     ` Chris Wilson
2017-05-04  8:35       ` Arkadiusz Hiler
2017-05-04  8:53         ` Tvrtko Ursulin
2017-05-04  9:21           ` Eero Tamminen
2017-05-04  9:34             ` Tvrtko Ursulin
2017-05-04 14:47         ` David Weinehall
2017-05-04 16:51           ` Kenneth Graunke
2017-05-04 20:32             ` Ewins, Jon
2017-05-05  2:46             ` Dmitry Rogozhkin
2017-05-05 11:31               ` Arkadiusz Hiler
2017-05-05 11:49                 ` Chris Wilson
2017-05-05 12:53                   ` Arkadiusz Hiler
2017-05-05 15:44               ` Kenneth Graunke
2017-05-05 16:21                 ` Dmitry Rogozhkin
2017-05-05 19:36                   ` Kenneth Graunke
2017-05-08  8:09 ` Daniel Vetter

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=877f27f5dr.fsf@riseup.net \
    --to=currojerez@riseup.net \
    --cc=david.weinehall@linux.intel.com \
    --cc=intel-gfx@lists.freedesktop.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.