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

[Ben Widawsky]

On 17-07-07 09:23:26, Jason Ekstrand wrote:
>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 think we're all agreed here.

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

Please direct comments to my response to Chris if you have more. To me it's 6
one way, half dozen the other - and I believe version has a more direct meaning
(and the ability to potentially, albeit a terrible idea, rewrite entries).

If people are going to block a review based on this, I will change it, but I'd
rather not.