[Mesa-dev] [Intel-gfx] [PATCH 1/1] drm/i915: Version the MOCS settings

[Jason Ekstrand]

On Fri, Jul 7, 2017 at 3:34 AM, Chris Wilson <chris at chris-wilson.co.uk>
wrote:

> Quoting Ben Widawsky (2017-07-07 00:27:01)
> >  drivers/gpu/drm/i915/i915_drv.c |  3 +++
> >  drivers/gpu/drm/i915/i915_drv.h |  2 ++
>   drivers/gpu/drm/i915/i915_pci.c | 13 +++++++++----
> >  include/uapi/drm/i915_drm.h     |  8 ++++++++
> >  4 files changed, 22 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> b/drivers/gpu/drm/i915/i915_drv.c
> > index 9167a73f3c69..26c27b6ae814 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -401,6 +401,9 @@ static int i915_getparam(struct drm_device *dev,
> void *data,
> >                 if (!value)
> >                         return -ENODEV;
> >                 break;
> > +       case I915_PARAM_MOCS_TABLE_VERSION:
> > +               value = INTEL_INFO(dev_priv)->mocs_version;
>
> If we use intel_mocs_get_table_version() we can put this magic number
> in intel_mocs.c next to the tables, where we can keep its history and
> hopefully be able to remember to update it.
>
> > +/* What version of the MOCS table we have. For GEN9 GPUs, the PRM
> defined
> > + * non-optimal settings for the MOCS table. As a result, we were
> required to use a
> > + * small subset, and later add new settings. This param allows
> userspace to
> > + * determine which settings are there.
> > + */
> > +#define MOCS_TABLE_VERSION               1 /* Build time MOCS table
> version */
>
> How are you planing to share this? When we update we bump this number,
> and then mesa copies it across and uses it after verifying it as 0,1 on
> an old kernel.
>

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).


> I don't think you want to expose the updated constant here, but symbolic
> names for each version? (What would be the point?)
>
> Next question, why a version number and not just the number of entries
> defined? Each index is defined by ABI once assigned, so the number of
> entries still operates as a version number and allows easy checking.
>
>         if (advanced_cacheing_idx < kernel_max_mocs)
>                 return advanced_cacheing_idx;
>         if (default_cacheing_idx < kernel_max_mocs)
>                 return default_cacheing_idx;
>
>         return follow_pte_idx;
>
> give or take the smarts to choose the preferred indices for any
> particular scenario.
>

I'll have to think about it a bit more but this sounds like a fairly good
idea.  I see two major benefits:

 1. The kernel can return ARRAY_SIZE(mocs_table_for_your_gen) and we will
never forget to update it.
 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.

That said, having it be a version may have it's advantages, I just don't
know what they are yet.

--Jason
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170707/6bf1ec8c/attachment.html>