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

[Chris Wilson]

Quoting Ben Widawsky (2017-07-07 19:42:25)
> On 17-07-07 11:34:48, Chris Wilson 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.
> >
> 
> Yeah, that seems like an improvement to me as well.
> 
> >> +/* 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.
> >
> >I don't think you want to expose the updated constant here, but symbolic
> >names for each version? (What would be the point?)
> >
> 
> At least one thing wrong here is we would need per GEN constants, which is 
> maybe
> what you meant and I misunderstood. Assuming you had per GEN constants, which 
> I
> don't like, I believe everything works out fine. So, I'll remove this compile
> time MOCS versioning.

I figured you were going towards per-gen versioning, which is kind of
why I liked the idea of table size -- but that only makes sense if
somehow the index has the same meaning across gen (which it won't).

> >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.
> >
> >In the future, if we finally get user defined mocs, the table_size will
> >then give the start of the user modifiable indices (presming they want
> >to keep the predefined entries for compatibility?))
> >-Chris
> 
> Yes, I considered this as well. I see no difference really as to one versus 
> the
> other. In fact, if you're to support multiple table versions, I think it's
> actually easier with a pure version:
> 
> switch (kernel_mocs_version) {
> case 3:
> return new_best_cacheing_index;
> case 2:
> return old_best_cacheing_index;
> case 1:
> return naive_best_index;
> }

Indeed 6 of one, half a dozen of the other. Whichever you pick, 3 years
down the line you wish you picked the other. The big advantage of using
an absolute version is that you can just stuff these into tables. Ok, I
like that more, a version parameter (that may be per-gen) worksforme.
-Chris
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [Intel-gfx] [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 
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.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Ben Widawsky]

On 17-07-07 11:34:48, Chris Wilson 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.



Yeah, that seems like an improvement to me as well.


+/* 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.

I don't think you want to expose the updated constant here, but symbolic
names for each version? (What would be the point?)



At least one thing wrong here is we would need per GEN constants, which is maybe
what you meant and I misunderstood. Assuming you had per GEN constants, which I
don't like, I believe everything works out fine. So, I'll remove this compile
time MOCS versioning.


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.

In the future, if we finally get user defined mocs, the table_size will
then give the start of the user modifiable indices (presming they want
to keep the predefined entries for compatibility?))
-Chris


Yes, I considered this as well. I see no difference really as to one versus the
other. In fact, if you're to support multiple table versions, I think it's
actually easier with a pure version:

switch (kernel_mocs_version) {
case 3:
return new_best_cacheing_index;
case 2:
return old_best_cacheing_index;
case 1:
return naive_best_index;
}
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [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 
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
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Emil Velikov]

On 7 July 2017 at 11:34, Chris Wilson  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.
>
> I don't think you want to expose the updated constant here, but symbolic
> names for each version? (What would be the point?)
>
FWIW I have to agree with Chris here - having the value is of limited
use. Furthermore it mostly confuses people when writing the user space
parts.

For example:
Mesa implements v1 and uses the define. Kernel headers get updated to
v2 and Mesa supporting v1 gets rebuild against them.
Mesa stores/treats as the MOCS version has "v2" when the actual
hardware/kernel supports "v1".

The expected issues vary depending on the implementation, but I
suspect it won't be fun :-)

IMHO it's better if user space is explicit on the versions it supports
and kernel should avoid exposing such defines unless really needed.

HTH
Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Chris Wilson]

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.

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.

In the future, if we finally get user defined mocs, the table_size will
then give the start of the user modifiable indices (presming they want
to keep the predefined entries for compatibility?))
-Chris
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev