| Message ID | 1540228395-27377-2-git-send-email-tomasz.lis@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] drm/i915/icl: Define MOCS table for Icelake | expand |
On 22/10/18 10:13, Tomasz Lis wrote: > For Icelake and above, MOCS table for each platform is published > within bspec. The table is versioned, and new entries are assigned > a version number. Existing entries do not change and their version > is constant. > > This introduces a parameter which allows getting max version number > of the MOCS entries currently supported, ie. value of 2 would mean > only version 1 and version 2 entries are initialized and can be used > by the user mode clients. > > BSpec: 34007 > Signed-off-by: Tomasz Lis <tomasz.lis@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Mika Kuoppala <mika.kuoppala@intel.com> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Zhenyu Wang <zhenyuw@linux.intel.com> > Cc: Zhi A Wang <zhi.a.wang@intel.com> > Cc: Anuj Phogat <anuj.phogat@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.c | 6 ++++++ > drivers/gpu/drm/i915/intel_mocs.c | 12 ++++++++++++ > drivers/gpu/drm/i915/intel_mocs.h | 1 + > include/uapi/drm/i915_drm.h | 11 +++++++++++ > 4 files changed, 30 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index baac35f..92fa8fd 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -53,6 +53,7 @@ > #include "i915_vgpu.h" > #include "intel_drv.h" > #include "intel_uc.h" > +#include "intel_mocs.h" > > static struct drm_driver driver; > > @@ -444,6 +445,11 @@ static int i915_getparam_ioctl(struct drm_device *dev, void *data, > case I915_PARAM_MMAP_GTT_COHERENT: > value = INTEL_INFO(dev_priv)->has_coherent_ggtt; > break; > + case I915_PARAM_MOCS_TABLE_VERSION: > + value = intel_mocs_table_version(dev_priv); > + if (!value) > + return -ENODEV; Do we really want to return -ENODEV for platforms that do have a MOCS table programmed, but the table is not one versioned in the specs (i.e. Gen9-10)? I think returning "0" for those to indicate "kernel-defined table" would be ok and we could limit -ENODEV for platforms that don't have a table at all. But what wins is what the callers of the ioctl would like to get from the kernel ;) > + break; > default: > DRM_DEBUG("Unknown parameter %d\n", param->param); > return -EINVAL; > diff --git a/drivers/gpu/drm/i915/intel_mocs.c b/drivers/gpu/drm/i915/intel_mocs.c > index dc34e83..fc1e98b 100644 > --- a/drivers/gpu/drm/i915/intel_mocs.c > +++ b/drivers/gpu/drm/i915/intel_mocs.c > @@ -469,6 +469,18 @@ static i915_reg_t mocs_register(enum intel_engine_id engine_id, int index) > } > > /** > + * intel_mocs_table_version() - get version of mocs table implementation > + * @i915: i915 device struct. > + */ > +int intel_mocs_table_version(struct drm_i915_private *i915) > +{ > + if (IS_ICELAKE(i915)) > + return 1; Can we add this version value as a define above the table, to keep them close to each other? If we agree on my suggestion above to differentiate between no table at all (< 0), kernel-defined table (= 0) and spec-defined versioned table (> 0), it might also be useful to store the version with the table info in drm_i915_mocs_table and then have intel_mocs_table_version call get_mocs_settings(), e.g: int intel_mocs_table_version(struct drm_i915_private *i915) { struct drm_i915_mocs_table table; if (!get_mocs_settings(i915, &table)) return -ENODEV; return table->version; } Thanks, Daniele > + else > + return 0; > +} > + > +/** > * intel_mocs_init_engine() - emit the mocs control table > * @engine: The engine for whom to emit the registers. > * > diff --git a/drivers/gpu/drm/i915/intel_mocs.h b/drivers/gpu/drm/i915/intel_mocs.h > index d89080d..dc1d64a 100644 > --- a/drivers/gpu/drm/i915/intel_mocs.h > +++ b/drivers/gpu/drm/i915/intel_mocs.h > @@ -55,5 +55,6 @@ > int intel_rcs_context_init_mocs(struct i915_request *rq); > void intel_mocs_init_l3cc_table(struct drm_i915_private *dev_priv); > void intel_mocs_init_engine(struct intel_engine_cs *engine); > +int intel_mocs_table_version(struct drm_i915_private *i915); > > #endif > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > index 298b2e1..16aafc4 100644 > --- a/include/uapi/drm/i915_drm.h > +++ b/include/uapi/drm/i915_drm.h > @@ -559,6 +559,17 @@ typedef struct drm_i915_irq_wait { > */ > #define I915_PARAM_MMAP_GTT_COHERENT 52 > > +/* > + * Query MOCS table version used during hardware initialization. > + * > + * The MOCS table for each platform is published as part of bspec. Entries in > + * the table are supposed to never be modified, but new enties are added, making > + * more indexes in the table valid. This parameter informs which version > + * of the table was used to initialize the currently used graphics hardware, > + * and therefore which MOCS indexes are useable. > + */ > +#define I915_PARAM_MOCS_TABLE_VERSION 53 > + > typedef struct drm_i915_getparam { > __s32 param; > /* >
Quoting Tomasz Lis (2018-10-22 20:13:15) > For Icelake and above, MOCS table for each platform is published > within bspec. The table is versioned, and new entries are assigned > a version number. Existing entries do not change and their version > is constant. > > This introduces a parameter which allows getting max version number > of the MOCS entries currently supported, ie. value of 2 would mean > only version 1 and version 2 entries are initialized and can be used > by the user mode clients. > > BSpec: 34007 > Signed-off-by: Tomasz Lis <tomasz.lis@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Mika Kuoppala <mika.kuoppala@intel.com> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Zhenyu Wang <zhenyuw@linux.intel.com> > Cc: Zhi A Wang <zhi.a.wang@intel.com> > Cc: Anuj Phogat <anuj.phogat@intel.com> As a heads-up, as this adds to the uAPI, this needs Acked-by from the respective userspace that will use the information, a link to the userspace change series and IGT test series all linked from the patch text. But as we're hoping to stay at version 1, I also would see that we only need to add this IOCTL if we get a version 2 of the tables. So there should be no need for this interface before we have some variance. Regards, Joonas
On 2018-10-22 20:18, Daniele Ceraolo Spurio wrote: > > > On 22/10/18 10:13, Tomasz Lis wrote: >> For Icelake and above, MOCS table for each platform is published >> within bspec. The table is versioned, and new entries are assigned >> a version number. Existing entries do not change and their version >> is constant. >> >> This introduces a parameter which allows getting max version number >> of the MOCS entries currently supported, ie. value of 2 would mean >> only version 1 and version 2 entries are initialized and can be used >> by the user mode clients. >> >> BSpec: 34007 >> Signed-off-by: Tomasz Lis <tomasz.lis@intel.com> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Mika Kuoppala <mika.kuoppala@intel.com> >> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> Cc: Zhenyu Wang <zhenyuw@linux.intel.com> >> Cc: Zhi A Wang <zhi.a.wang@intel.com> >> Cc: Anuj Phogat <anuj.phogat@intel.com> >> --- >> drivers/gpu/drm/i915/i915_drv.c | 6 ++++++ >> drivers/gpu/drm/i915/intel_mocs.c | 12 ++++++++++++ >> drivers/gpu/drm/i915/intel_mocs.h | 1 + >> include/uapi/drm/i915_drm.h | 11 +++++++++++ >> 4 files changed, 30 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c >> b/drivers/gpu/drm/i915/i915_drv.c >> index baac35f..92fa8fd 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.c >> +++ b/drivers/gpu/drm/i915/i915_drv.c >> @@ -53,6 +53,7 @@ >> #include "i915_vgpu.h" >> #include "intel_drv.h" >> #include "intel_uc.h" >> +#include "intel_mocs.h" >> static struct drm_driver driver; >> @@ -444,6 +445,11 @@ static int i915_getparam_ioctl(struct >> drm_device *dev, void *data, >> case I915_PARAM_MMAP_GTT_COHERENT: >> value = INTEL_INFO(dev_priv)->has_coherent_ggtt; >> break; >> + case I915_PARAM_MOCS_TABLE_VERSION: >> + value = intel_mocs_table_version(dev_priv); >> + if (!value) >> + return -ENODEV; > > Do we really want to return -ENODEV for platforms that do have a MOCS > table programmed, but the table is not one versioned in the specs > (i.e. Gen9-10)? I think returning "0" for those to indicate > "kernel-defined table" would be ok and we could limit -ENODEV for > platforms that don't have a table at all. But what wins is what the > callers of the ioctl would like to get from the kernel ;) > >> + break; >> default: >> DRM_DEBUG("Unknown parameter %d\n", param->param); >> return -EINVAL; >> diff --git a/drivers/gpu/drm/i915/intel_mocs.c >> b/drivers/gpu/drm/i915/intel_mocs.c >> index dc34e83..fc1e98b 100644 >> --- a/drivers/gpu/drm/i915/intel_mocs.c >> +++ b/drivers/gpu/drm/i915/intel_mocs.c >> @@ -469,6 +469,18 @@ static i915_reg_t mocs_register(enum >> intel_engine_id engine_id, int index) >> } >> /** >> + * intel_mocs_table_version() - get version of mocs table >> implementation >> + * @i915: i915 device struct. >> + */ >> +int intel_mocs_table_version(struct drm_i915_private *i915) >> +{ >> + if (IS_ICELAKE(i915)) >> + return 1; > > Can we add this version value as a define above the table, to keep > them close to each other? > > If we agree on my suggestion above to differentiate between no table > at all (< 0), kernel-defined table (= 0) and spec-defined versioned > table (> 0), it might also be useful to store the version with the > table info in drm_i915_mocs_table and then have > intel_mocs_table_version call get_mocs_settings(), e.g: > > int intel_mocs_table_version(struct drm_i915_private *i915) > { > struct drm_i915_mocs_table table; > > if (!get_mocs_settings(i915, &table)) > return -ENODEV; > > return table->version; > } > > Thanks, > Daniele That does sound reasonable. And I agree we should really ask userland what exactly they want. Right now there is no request from any UMD for such interface; we will get back to this patch when it is requested. Joonas also mentioned there is no need for an interface which always returns "1", and is expected to return only that value for future platforms as well. That's a good argument. I will remove this patch from the current series. -Tomasz > >> + else >> + return 0; >> +} >> + >> +/** >> * intel_mocs_init_engine() - emit the mocs control table >> * @engine: The engine for whom to emit the registers. >> * >> diff --git a/drivers/gpu/drm/i915/intel_mocs.h >> b/drivers/gpu/drm/i915/intel_mocs.h >> index d89080d..dc1d64a 100644 >> --- a/drivers/gpu/drm/i915/intel_mocs.h >> +++ b/drivers/gpu/drm/i915/intel_mocs.h >> @@ -55,5 +55,6 @@ >> int intel_rcs_context_init_mocs(struct i915_request *rq); >> void intel_mocs_init_l3cc_table(struct drm_i915_private *dev_priv); >> void intel_mocs_init_engine(struct intel_engine_cs *engine); >> +int intel_mocs_table_version(struct drm_i915_private *i915); >> #endif >> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h >> index 298b2e1..16aafc4 100644 >> --- a/include/uapi/drm/i915_drm.h >> +++ b/include/uapi/drm/i915_drm.h >> @@ -559,6 +559,17 @@ typedef struct drm_i915_irq_wait { >> */ >> #define I915_PARAM_MMAP_GTT_COHERENT 52 >> +/* >> + * Query MOCS table version used during hardware initialization. >> + * >> + * The MOCS table for each platform is published as part of bspec. >> Entries in >> + * the table are supposed to never be modified, but new enties are >> added, making >> + * more indexes in the table valid. This parameter informs which >> version >> + * of the table was used to initialize the currently used graphics >> hardware, >> + * and therefore which MOCS indexes are useable. >> + */ >> +#define I915_PARAM_MOCS_TABLE_VERSION 53 >> + >> typedef struct drm_i915_getparam { >> __s32 param; >> /* >>
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index baac35f..92fa8fd 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -53,6 +53,7 @@ #include "i915_vgpu.h" #include "intel_drv.h" #include "intel_uc.h" +#include "intel_mocs.h" static struct drm_driver driver; @@ -444,6 +445,11 @@ static int i915_getparam_ioctl(struct drm_device *dev, void *data, case I915_PARAM_MMAP_GTT_COHERENT: value = INTEL_INFO(dev_priv)->has_coherent_ggtt; break; + case I915_PARAM_MOCS_TABLE_VERSION: + value = intel_mocs_table_version(dev_priv); + if (!value) + return -ENODEV; + break; default: DRM_DEBUG("Unknown parameter %d\n", param->param); return -EINVAL; diff --git a/drivers/gpu/drm/i915/intel_mocs.c b/drivers/gpu/drm/i915/intel_mocs.c index dc34e83..fc1e98b 100644 --- a/drivers/gpu/drm/i915/intel_mocs.c +++ b/drivers/gpu/drm/i915/intel_mocs.c @@ -469,6 +469,18 @@ static i915_reg_t mocs_register(enum intel_engine_id engine_id, int index) } /** + * intel_mocs_table_version() - get version of mocs table implementation + * @i915: i915 device struct. + */ +int intel_mocs_table_version(struct drm_i915_private *i915) +{ + if (IS_ICELAKE(i915)) + return 1; + else + return 0; +} + +/** * intel_mocs_init_engine() - emit the mocs control table * @engine: The engine for whom to emit the registers. * diff --git a/drivers/gpu/drm/i915/intel_mocs.h b/drivers/gpu/drm/i915/intel_mocs.h index d89080d..dc1d64a 100644 --- a/drivers/gpu/drm/i915/intel_mocs.h +++ b/drivers/gpu/drm/i915/intel_mocs.h @@ -55,5 +55,6 @@ int intel_rcs_context_init_mocs(struct i915_request *rq); void intel_mocs_init_l3cc_table(struct drm_i915_private *dev_priv); void intel_mocs_init_engine(struct intel_engine_cs *engine); +int intel_mocs_table_version(struct drm_i915_private *i915); #endif diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index 298b2e1..16aafc4 100644 --- a/include/uapi/drm/i915_drm.h +++ b/include/uapi/drm/i915_drm.h @@ -559,6 +559,17 @@ typedef struct drm_i915_irq_wait { */ #define I915_PARAM_MMAP_GTT_COHERENT 52 +/* + * Query MOCS table version used during hardware initialization. + * + * The MOCS table for each platform is published as part of bspec. Entries in + * the table are supposed to never be modified, but new enties are added, making + * more indexes in the table valid. This parameter informs which version + * of the table was used to initialize the currently used graphics hardware, + * and therefore which MOCS indexes are useable. + */ +#define I915_PARAM_MOCS_TABLE_VERSION 53 + typedef struct drm_i915_getparam { __s32 param; /*
For Icelake and above, MOCS table for each platform is published within bspec. The table is versioned, and new entries are assigned a version number. Existing entries do not change and their version is constant. This introduces a parameter which allows getting max version number of the MOCS entries currently supported, ie. value of 2 would mean only version 1 and version 2 entries are initialized and can be used by the user mode clients. BSpec: 34007 Signed-off-by: Tomasz Lis <tomasz.lis@intel.com> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Mika Kuoppala <mika.kuoppala@intel.com> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Cc: Zhenyu Wang <zhenyuw@linux.intel.com> Cc: Zhi A Wang <zhi.a.wang@intel.com> Cc: Anuj Phogat <anuj.phogat@intel.com> --- drivers/gpu/drm/i915/i915_drv.c | 6 ++++++ drivers/gpu/drm/i915/intel_mocs.c | 12 ++++++++++++ drivers/gpu/drm/i915/intel_mocs.h | 1 + include/uapi/drm/i915_drm.h | 11 +++++++++++ 4 files changed, 30 insertions(+)