[Intel-gfx] [PATCH] drm/i915/sysfs: Adding mocs_state

Fri May 6 13:26:53 UTC 2016

On Fri, 6 May 2016, Ville Syrjälä wrote:

> On Thu, May 05, 2016 at 07:17:02AM +0000, Antoine, Peter wrote:
>> It's a little overkill?
>>
>> They just need to know if the cache tables have changed and to be able to sync their indexes to the KMD.
>
> We already shot ourselves in the foot with this MOCS ABI stuff. This
> sysfs stuff just feels like digging the hole deeper, as in more legacy
> baggage when we eventually have to change the whole apporach anyway.
> Given our track record here I have a feeling that will happen at some
> point.
The way the MOCS hardware works has pointed us into this direction. A 
fixed table is all that works for all use-cases. Other platforms have 
gotten around the hardware limitations by fixing the table in the "one 
true" userspace library and have a matching table in the kernel/driver.

We don't have the luxury of being able to change the table at will for 
performance gains, security issues and have the userspace match-up. We 
don't control both the userspace and the driver to update them both at 
the same time and this is against the ABI.

So to support the improved optimisation and handle backward compatibility 
we need to be able to allow the KMD and UMD to sync.

The MOCS hardware was not designed from an ease of programming point of 
view and compromises have had to be made in the current gen to make this 
work.

>
>> Also you have to decode the L3CC (two 16 bit values in a 32 bit register) and seems a bit unfriendly. Also you will need to know what the bits mean to detect where the table ends (we fill with uncached entries). That means they need to understand a lot of the workings of the hardware, which we are trying to hide as they don't need to know. It also may change and then we have a support/maintenance issue which would be hidden behind the sysfs.
>
> So you're worried that having to know the layout of the MOCS registers
> is too much burden for userspace which already has to know pretty much
> every other detail about the hardware?
Yes. It's not just the layout of the registers but when they are valid and 
not valid. Any work arounds that have been applied, availability in the 
power saving modes and probably some other issues. Which registers do they 
read? If they read the BSpec they will find registers for WiDi and GuC, we 
don't program these.

You are right, we could let the userspace work this out for themselves but 
I think this is going to cause more grief in the long term.
>
>>
>> Also, this way future proofs the user-space from some of the changes that may come in the future.
>>
>> I think the sysfs is nicer and easier to access for the users.
>>
>> On that note, what should the format of sysfs files be?
>
> If we would use sysfs then I think the only viable way might be to
> dump out the entire table as a raw blob. And if you do that, userspace
> will need to understand the contents, and at that point the whole thing
> becomes pointless since you could just as well use SRM.
Sorry, I don't understand why a raw blob has to be given. We list the 
values that are required for use. This is enough for synchronisation and any 
performance tuning with regards to the current fixed KMD table. They then 
don't need to know the register layout just what each value means and 
select which entry is best for them to use.

Also, there are a set of MOCS values for each of the engines and a shared 
set (currently only used by RCS). As we currently ensure that all these 
register sets (well for the engines that we use) are consistent with the 
KMD table, it is simpler just to dump the table that we set. This is what 
the sysfs patch does. Ok, the format of the sysfs files have been argued 
over it and needs to be settled on, but this seems the best currently 
available solution to the problem.

>
> In any case, if you go to the trouble of enshrining a new ABI, then
> maybe just go all the way and come up with a way for userspace to
> reconfigure the MOCS table at will?
Oh I wish so hard for this.

The very first version of the MOCS almost a year ago was programmed this 
way, it's the only sensible way that cache values for an application 
should be programmed. But, it does not work. Due to context limitations 
with the hardware and problems with usage for virtualisation - because of 
the context issues, a fixed table is the only acceptable way for all use 
cases. Even this causes problems in virtualisation, but ones that can 
be managed by the virtualiser (manual save/restore on non-saved engines).

Hopefully in the future the sysfs and the MOCS table will be there only 
for legacy support and won't need to change again.

But for now we need to allow user space to sync with KMD. We can let them 
do it themselves and let the hilarity ensue or give them an easy table to 
ingest. I'd prefer the easy table to reduce the support costs.

Peter.
>
>> Peter.
>> -----Original Message-----
>> From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com]
>> Sent: Wednesday, May 4, 2016 5:56 PM
>> To: Antoine, Peter <peter.antoine at intel.com>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>; intel-gfx at lists.freedesktop.org; Widawsky, Benjamin <benjamin.widawsky at intel.com>
>> Subject: Re: [Intel-gfx] [PATCH] drm/i915/sysfs: Adding mocs_state
>>
>> On Wed, May 04, 2016 at 03:51:21PM +0100, Peter Antoine wrote:
>>>
>>> Sorry Ville,
>>>
>>> What is SRM?
>>
>> MI_STORE_REGISTER_MEM
>>
>>>
>>> Peter.
>>>
>>> On Wed, 4 May 2016, Ville Syrjälä wrote:
>>>
>>>> On Wed, May 04, 2016 at 02:23:35PM +0000, Antoine, Peter wrote:
>>>>> No, It's not debug.
>>>>> It's for syncing and aligning (and validating) the open-source userspace with the kernel cache policy.
>>>>
>>>> Why doesn't userspace just use SRM to read registers? The spec gives
>>>> me the impression that SRM doesn't care whether the register is
>>>> privileged or not.
>>>>
>>>>>
>>>>> As for the name being wrong, I'll change that.
>>>>>
>>>>> As for the sysfs, would you prefer the following structure:
>>>>>
>>>>> mocs/size
>>>>> mocs/control_state
>>>>> mocs/l3cc_state
>>>>>
>>>>> for the different tables?
>>>>>
>>>>> Peter.
>>>>>
>>>>> -----Original Message-----
>>>>> From: Chris Wilson [mailto:chris at chris-wilson.co.uk]
>>>>> Sent: Wednesday, May 4, 2016 2:47 PM
>>>>> To: Antoine, Peter <peter.antoine at intel.com>
>>>>> Cc: intel-gfx at lists.freedesktop.org; Widawsky, Benjamin
>>>>> <benjamin.widawsky at intel.com>
>>>>> Subject: Re: [Intel-gfx] [PATCH] drm/i915/sysfs: Adding mocs_state
>>>>>
>>>>> On Wed, May 04, 2016 at 02:32:53PM +0100, Peter Antoine wrote:
>>>>>> Will wait for more comments, then will respin with a different
>>>>>> commit message. Is the rest of the patch ok?
>>>>>
>>>>> No, you've put debug information into sysfs. (Also sysfs is one
>>>>> value per
>>>>> file.) sysfs does not match your goal of validation. And you exported an internal function (get_mocs...) without giving it a proper name.
>>>>> -Chris
>>>>>
>>>>> --
>>>>> Chris Wilson, Intel Open Source Technology Centre
>>>>> _______________________________________________
>>>>> Intel-gfx mailing list
>>>>> Intel-gfx at lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>>
>>>>
>>>
>>> --
>>>     Peter Antoine (Android Graphics Driver Software Engineer)
>>>     ---------------------------------------------------------------------
>>>     Intel Corporation (UK) Limited
>>>     Registered No. 1134945 (England)
>>>     Registered Office: Pipers Way, Swindon SN3 1RJ
>>>     VAT No: 860 2173 47
>>
>>
>> --
>> Ville Syrjälä
>> Intel OTC
>
>

--
    Peter Antoine (Android Graphics Driver Software Engineer)
    ---------------------------------------------------------------------
    Intel Corporation (UK) Limited
    Registered No. 1134945 (England)
    Registered Office: Pipers Way, Swindon SN3 1RJ
    VAT No: 860 2173 47