[Intel-gfx] [PATCH v2 2/2] drm/i915/icl: Add IOCTL for getting MOCS table version

[Lis, Tomasz]

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 at intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: Mika Kuoppala <mika.kuoppala at intel.com>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>> Cc: Zhenyu Wang <zhenyuw at linux.intel.com>
>> Cc: Zhi A Wang <zhi.a.wang at intel.com>
>> Cc: Anuj Phogat <anuj.phogat at 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;
>>       /*
>>