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@xxxxxxxxxxxxxxx]
Sent: Wednesday, May 4, 2016 5:56 PM
To: Antoine, Peter <peter.antoine@xxxxxxxxx>
Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Widawsky, Benjamin <benjamin.widawsky@xxxxxxxxx>
Subject: Re: [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@xxxxxxxxxxxxxxxxxx]
Sent: Wednesday, May 4, 2016 2:47 PM
To: Antoine, Peter <peter.antoine@xxxxxxxxx>
Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Widawsky, Benjamin
<benjamin.widawsky@xxxxxxxxx>
Subject: Re: [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@xxxxxxxxxxxxxxxxxxxxx
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
