| Message ID | 1462367783-4342-1-git-send-email-peter.antoine@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, May 04, 2016 at 02:16:23PM +0100, Peter Antoine wrote: > The following patch adds the current MOCS state to the sysfs file > system. This patch has been added so that userspace can validate the > MOCS settings that have been programmed. These only correspond to the state the kernel may program, not the state that has been programmed. -Chris
Will wait for more comments, then will respin with a different commit message. Is the rest of the patch ok? Peter. On Wed, 4 May 2016, Chris Wilson wrote: > On Wed, May 04, 2016 at 02:16:23PM +0100, Peter Antoine wrote: >> The following patch adds the current MOCS state to the sysfs file >> system. This patch has been added so that userspace can validate the >> MOCS settings that have been programmed. > > These only correspond to the state the kernel may program, not the state > that has been programmed. > -Chris > > -- 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
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
No, It's not debug. It's for syncing and aligning (and validating) the open-source userspace with the kernel cache policy. 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@chris-wilson.co.uk] Sent: Wednesday, May 4, 2016 2:47 PM To: Antoine, Peter <peter.antoine@intel.com> Cc: intel-gfx@lists.freedesktop.org; Widawsky, Benjamin <benjamin.widawsky@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
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@chris-wilson.co.uk] > Sent: Wednesday, May 4, 2016 2:47 PM > To: Antoine, Peter <peter.antoine@intel.com> > Cc: intel-gfx@lists.freedesktop.org; Widawsky, Benjamin <benjamin.widawsky@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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Sorry Ville, What is SRM? 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@chris-wilson.co.uk] >> Sent: Wednesday, May 4, 2016 2:47 PM >> To: Antoine, Peter <peter.antoine@intel.com> >> Cc: intel-gfx@lists.freedesktop.org; Widawsky, Benjamin <benjamin.widawsky@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@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
On Wed, May 04, 2016 at 05:38:41PM +0300, 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. The SRM we do in igt to validate the mocs settings are indeed unprivileged. -Chris
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@chris-wilson.co.uk] > >> Sent: Wednesday, May 4, 2016 2:47 PM > >> To: Antoine, Peter <peter.antoine@intel.com> > >> Cc: intel-gfx@lists.freedesktop.org; Widawsky, Benjamin <benjamin.widawsky@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@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
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. 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. 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? Peter. -----Original Message----- From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] Sent: Wednesday, May 4, 2016 5:56 PM To: Antoine, Peter <peter.antoine@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk>; intel-gfx@lists.freedesktop.org; Widawsky, Benjamin <benjamin.widawsky@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@chris-wilson.co.uk] > >> Sent: Wednesday, May 4, 2016 2:47 PM > >> To: Antoine, Peter <peter.antoine@intel.com> > >> Cc: intel-gfx@lists.freedesktop.org; Widawsky, Benjamin > >> <benjamin.widawsky@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@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
Anymore feedback on this? -----Original Message----- From: Antoine, Peter Sent: Thursday, May 5, 2016 8:17 AM To: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk>; intel-gfx@lists.freedesktop.org; Widawsky, Benjamin <benjamin.widawsky@intel.com> Subject: RE: [Intel-gfx] [PATCH] drm/i915/sysfs: Adding mocs_state 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. 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. 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? Peter. -----Original Message----- From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] Sent: Wednesday, May 4, 2016 5:56 PM To: Antoine, Peter <peter.antoine@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk>; intel-gfx@lists.freedesktop.org; Widawsky, Benjamin <benjamin.widawsky@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@chris-wilson.co.uk] > >> Sent: Wednesday, May 4, 2016 2:47 PM > >> To: Antoine, Peter <peter.antoine@intel.com> > >> Cc: intel-gfx@lists.freedesktop.org; Widawsky, Benjamin > >> <benjamin.widawsky@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@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
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. > 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? > > 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. 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? > Peter. > -----Original Message----- > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > Sent: Wednesday, May 4, 2016 5:56 PM > To: Antoine, Peter <peter.antoine@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk>; intel-gfx@lists.freedesktop.org; Widawsky, Benjamin <benjamin.widawsky@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@chris-wilson.co.uk] > > >> Sent: Wednesday, May 4, 2016 2:47 PM > > >> To: Antoine, Peter <peter.antoine@intel.com> > > >> Cc: intel-gfx@lists.freedesktop.org; Widawsky, Benjamin > > >> <benjamin.widawsky@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@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
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@linux.intel.com] >> Sent: Wednesday, May 4, 2016 5:56 PM >> To: Antoine, Peter <peter.antoine@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk>; intel-gfx@lists.freedesktop.org; Widawsky, Benjamin <benjamin.widawsky@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@chris-wilson.co.uk] >>>>> Sent: Wednesday, May 4, 2016 2:47 PM >>>>> To: Antoine, Peter <peter.antoine@intel.com> >>>>> Cc: intel-gfx@lists.freedesktop.org; Widawsky, Benjamin >>>>> <benjamin.widawsky@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@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
On Fri, May 06, 2016 at 02:16:29PM +0300, 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. Yeah, sysfs for MOCS given how bad we just failed at making the ABI solid sounds like a very bad idea. More incremental ABI changes like Ville suggested please. This also needs review from Imre to make sure we have a consistent story here. Thanks, Daniel
diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c index 2d576b7..f5de921 100644 --- a/drivers/gpu/drm/i915/i915_sysfs.c +++ b/drivers/gpu/drm/i915/i915_sysfs.c @@ -31,6 +31,7 @@ #include <linux/sysfs.h> #include "intel_drv.h" #include "i915_drv.h" +#include "intel_mocs.h" #define dev_to_drm_minor(d) dev_get_drvdata((d)) @@ -591,6 +592,32 @@ static struct bin_attribute error_state_attr = { .write = error_state_write, }; +static ssize_t mocs_state_show_attr(struct device *kdev, + struct device_attribute *attr, + char *buf) +{ + struct drm_minor *minor = dev_to_drm_minor(kdev); + struct drm_device *dev = minor->dev; + struct drm_i915_mocs_table table; + ssize_t ret = 0; + u32 count; + + if (!get_mocs_settings(dev->dev_private, &table)) + return 0; + + for (count = 0; count < table.size; count++) { + ret += snprintf(buf + ret, + PAGE_SIZE - ret, + "0x%08x 0x%04x\n", + table.table[count].control_value, + table.table[count].l3cc_value); + } + + return ret; +} + +static DEVICE_ATTR(mocs_state, S_IRUGO, mocs_state_show_attr, NULL); + void i915_setup_sysfs(struct drm_device *dev) { int ret; @@ -640,6 +667,10 @@ void i915_setup_sysfs(struct drm_device *dev) &error_state_attr); if (ret) DRM_ERROR("error_state sysfs setup failed\n"); + + ret = device_create_file(dev->primary->kdev, &dev_attr_mocs_state); + if (ret) + DRM_ERROR("mocs_state failed to create: %d\n", ret); } void i915_teardown_sysfs(struct drm_device *dev) @@ -655,4 +686,5 @@ void i915_teardown_sysfs(struct drm_device *dev) sysfs_unmerge_group(&dev->primary->kdev->kobj, &rc6_attr_group); sysfs_unmerge_group(&dev->primary->kdev->kobj, &rc6p_attr_group); #endif + device_remove_file(dev->primary->kdev, &dev_attr_mocs_state); } diff --git a/drivers/gpu/drm/i915/intel_mocs.c b/drivers/gpu/drm/i915/intel_mocs.c index 6ba4bf7..199c5d7 100644 --- a/drivers/gpu/drm/i915/intel_mocs.c +++ b/drivers/gpu/drm/i915/intel_mocs.c @@ -24,17 +24,6 @@ #include "intel_lrc.h" #include "intel_ringbuffer.h" -/* structures required */ -struct drm_i915_mocs_entry { - u32 control_value; - u16 l3cc_value; -}; - -struct drm_i915_mocs_table { - u32 size; - const struct drm_i915_mocs_entry *table; -}; - /* Defines for the tables (XXX_MOCS_0 - XXX_MOCS_63) */ #define LE_CACHEABILITY(value) ((value) << 0) #define LE_TGT_CACHE(value) ((value) << 2) @@ -138,8 +127,8 @@ static const struct drm_i915_mocs_entry broxton_mocs_table[] = { * * Return: true if there are applicable MOCS settings for the device. */ -static bool get_mocs_settings(struct drm_i915_private *dev_priv, - struct drm_i915_mocs_table *table) +bool get_mocs_settings(struct drm_i915_private *dev_priv, + struct drm_i915_mocs_table *table) { bool result = false; diff --git a/drivers/gpu/drm/i915/intel_mocs.h b/drivers/gpu/drm/i915/intel_mocs.h index 4640299..ba39b8a 100644 --- a/drivers/gpu/drm/i915/intel_mocs.h +++ b/drivers/gpu/drm/i915/intel_mocs.h @@ -52,8 +52,21 @@ #include <drm/drmP.h> #include "i915_drv.h" +/* structures required */ +struct drm_i915_mocs_entry { + u32 control_value; + u16 l3cc_value; +}; + +struct drm_i915_mocs_table { + u32 size; + const struct drm_i915_mocs_entry *table; +}; + int intel_rcs_context_init_mocs(struct drm_i915_gem_request *req); void intel_mocs_init_l3cc_table(struct drm_device *dev); int intel_mocs_init_engine(struct intel_engine_cs *ring); +bool get_mocs_settings(struct drm_i915_private *dev_priv, + struct drm_i915_mocs_table *table); #endif
The following patch adds the current MOCS state to the sysfs file system. This patch has been added so that userspace can validate the MOCS settings that have been programmed. Tracked-On: https://jira01.devtools.intel.com/browse/VIZ-7767 Signed-off-by: Peter Antoine <peter.antoine@intel.com> --- drivers/gpu/drm/i915/i915_sysfs.c | 32 ++++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_mocs.c | 15 ++------------- drivers/gpu/drm/i915/intel_mocs.h | 13 +++++++++++++ 3 files changed, 47 insertions(+), 13 deletions(-)