<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Jul 7, 2017 at 3:34 AM, Chris Wilson <span dir="ltr"><<a href="mailto:chris@chris-wilson.co.uk" target="_blank">chris@chris-wilson.co.uk</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Quoting Ben Widawsky (2017-07-07 00:27:01)<br>
<span class="">>  drivers/gpu/drm/i915/i915_drv.<wbr>c |  3 +++<br>
>  drivers/gpu/drm/i915/i915_drv.<wbr>h |  2 ++<br> 
drivers/gpu/drm/i915/i915_pci.<wbr>c | 13 +++++++++----<br>
>  include/uapi/drm/i915_drm.h     |  8 ++++++++<br>
>  4 files changed, 22 insertions(+), 4 deletions(-)<br>
><br>
> diff --git a/drivers/gpu/drm/i915/i915_<wbr>drv.c b/drivers/gpu/drm/i915/i915_<wbr>drv.c<br>
> index 9167a73f3c69..26c27b6ae814 100644<br>
> --- a/drivers/gpu/drm/i915/i915_<wbr>drv.c<br>
> +++ b/drivers/gpu/drm/i915/i915_<wbr>drv.c<br>
> @@ -401,6 +401,9 @@ static int i915_getparam(struct drm_device *dev, void *data,<br>
>                 if (!value)<br>
>                         return -ENODEV;<br>
>                 break;<br>
> +       case I915_PARAM_MOCS_TABLE_VERSION:<br>
> +               value = INTEL_INFO(dev_priv)->mocs_<wbr>version;<br>
<br>
</span>If we use intel_mocs_get_table_version() we can put this magic number<br>
in intel_mocs.c next to the tables, where we can keep its history and<br>
hopefully be able to remember to update it.<br>
<span class=""><br>
> +/* What version of the MOCS table we have. For GEN9 GPUs, the PRM defined<br>
> + * non-optimal settings for the MOCS table. As a result, we were required to use a<br>
> + * small subset, and later add new settings. This param allows userspace to<br>
> + * determine which settings are there.<br>
> + */<br>
> +#define MOCS_TABLE_VERSION               1 /* Build time MOCS table version */<br>
<br>
</span>How are you planing to share this? When we update we bump this number,<br>
and then mesa copies it across and uses it after verifying it as 0,1 on<br>
an old kernel.<br></blockquote><div><br></div><div>Agreed.  I don't see how having a #define for compile-time mocs version is useful.  The compile-time version doesn't really matter and we wouldn't want to use that in i965/anv anyway (more on that in the other patch).<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I don't think you want to expose the updated constant here, but symbolic<br>
names for each version? (What would be the point?)<br>
<br>
Next question, why a version number and not just the number of entries<br>
defined? Each index is defined by ABI once assigned, so the number of<br>
entries still operates as a version number and allows easy checking.<br>
<br>
        if (advanced_cacheing_idx < kernel_max_mocs)<br>
                return advanced_cacheing_idx;<br>
        if (default_cacheing_idx < kernel_max_mocs)<br>
                return default_cacheing_idx;<br>
<br>
        return follow_pte_idx;<br>
<br>
give or take the smarts to choose the preferred indices for any<br>
particular scenario.<br></blockquote><div><br></div><div>I'll have to think about it a bit more but this sounds like a fairly good idea.  I see two major benefits:<br><br></div><div> 1. The kernel can return ARRAY_SIZE(mocs_table_for_your_gen) and we will never forget to update it.<br></div><div> 2. It makes the "does this MOCS value exist" check much easier.  I imagine future userspace code which chooses mocs values having some sort of "try and fall back" approach to making MOCS choices and this would be convenient.<br><br></div><div>That said, having it be a version may have it's advantages, I just don't know what they are yet.<br><br></div><div>--Jason<br></div></div></div></div>