Re: [RFC 0/4] Introduce drmfs pseudo filesystem for drm subsystem
On Mon, 2016-12-19 at 17:15 -0800, Laurent Pinchart wrote: > Hi Swati, > > On Monday 19 Dec 2016 16:12:22 swati.dhin...@intel.com wrote: > > From: Swati Dhingra <swati.dhin...@intel.com> > > > > Currently, we don't have a stable ABI which can be used for the purpose of > > providing output debug/loggging/crc and other such data from DRM. > > The ABI in current use (filesystems, ioctls, et al.) have their own > > constraints and are intended to output a particular type of data. > > Few cases in point: > > sysfs - stable ABI, but constrained to one textual value per file > > debugfs - unstable ABI, free-for-all > > ioctls- not as suitable to many single purpose continuous data > > dumping, we would very quickly run out ioctl space; requires more > > userspace support than "cat" > > device nodes - a real possibilty, kernel instantiation is more tricky, > > requires udev (+udev.rules) or userspace discovery of the > > dynamic major:minor (via sysfs) [mounting a registered > > filesystem is easy in comparison] > > netlink - stream based, therefore involves numerous copies. > > > > Debugfs is the lesser among the evils here, thereby we have grown used to > > the convenience and flexibility in presentation that debugfs gives us > > (including relayfs inodes) that we want some of that hierachy in stable user > > ABI form. > > Seriously, why ? A subsystem growing its own file system sounds so wrong. It > seems that you want to have all the benefits of a stable ABI without going > through the standardization effort that this requires. I can see so many ways > that drmfs could be abused, with drivers throwing in new data with little or > no review. You'll need very compelling arguments to convince me. > Hi Laurent, Can you please let us know how to address the standardization issues? As per our (limited) knowledge, drmfs seemed to be the most suitable solution for exposing usecases such as microprocessor logs in i915, and possibly other such usecases, which ideally can't fit in with sysfs/debugfs/ioctls due to reasons mentioned above. But having said that, standardization requires a lot more effort (defining the constraints etc.), which we're not familiar with, frankly. Can you please provide your views on how to proceed as such, since the idea seemed worth pursuing to us (It's a drm based filesystem, whose existence depends on drm, and directory contents solely controlled by the corresponding drm driver - as such the contents shouldn't be controllable by an external driver). Or, should this be dropped, if the idea of a subsystem having its own filesystem is fundamentally wrong to its core? Regards, Sourab > > Due to these limitations, there is a need for a new pseudo filesytem, that > > would act as a stable 'free-for-all' ABI, with the heirarchial structure and > > thus convenience of debugfs. This will be managed by drm, thus named > > 'drmfs'. DRM would register this filesystem to manage a canonical > > mountpoint, but this wouldn't limit everyone to only using that pseudofs > > underneath. > > > > This can serve to hold various kinds of output data from Linux DRM > > subsystems, for the files which can't truely fit anywhere else with > > existing ABI's but present so, for the lack of a better place. > > > > In this patch series, we have introduced a pseudo filesystem named as > > 'drmfs' for now. The filesystem is introduced in the first patch, and the > > subsequent patches make use of the filesystem interfaces, in drm driver, > > and making them available for use by the drm subsystem components, one of > > which is i915. We've moved the location of i915 GuC logs from debugfs to > > drmfs in the last patch. Subsequently, more such files such as pipe_crc, > > error states, memory stats, etc. can be move to this filesystem, if the > > idea introduced here is acceptable per se. The filesystem introduced is > > being used to house the data generated by i915 driver in this patch series, > > but will hopefully be generic enough to provide scope for usage by any > > other drm subsystem component. > > > > The patch series is being floated as RFC to gather feedback on the idea and > > infrastructure proposed here and it's suitability to address the specific > > problem statement/use case. > > > > v2: fix the bat failures caused due to missing config check > > > > v3: Changes made: > > - Move the location of drmfs from fs/ to drivers/gpu/drm/ (Chris) > > - Moving config checks to header (Chris,Daniel) > > > > v4: Added the kernel
Re: [RFC 0/4] Introduce drmfs pseudo filesystem for drm subsystem
atch series, > >> but will hopefully be generic enough to provide scope for usage by any > >> other drm subsystem component. > >> > >> The patch series is being floated as RFC to gather feedback on the idea and > >> infrastructure proposed here and it's suitability to address the specific > >> problem statement/use case. > >> > >> v2: fix the bat failures caused due to missing config check > >> > >> v3: Changes made: > >> - Move the location of drmfs from fs/ to drivers/gpu/drm/ (Chris) > >> - Moving config checks to header (Chris,Daniel) > >> > >> v4: Added the kernel Documentaion (using Sphinx). > >> > >> Sourab Gupta (4): > >> drm: Introduce drmfs pseudo filesystem interfaces > >> drm: Register drmfs filesystem from drm init > >> drm: Create driver specific root directory inside drmfs > >> drm/i915: Creating guc log file in drmfs instead of debugfs > >> > >> Documentation/gpu/drm-uapi.rst | 76 > >> drivers/gpu/drm/Kconfig| 9 + > >> drivers/gpu/drm/Makefile | 1 + > >> drivers/gpu/drm/drm_drv.c | 26 ++ > >> drivers/gpu/drm/drmfs.c| 566 > >> ++ > >> drivers/gpu/drm/i915/i915_guc_submission.c | 33 +- > >> include/drm/drm_drv.h | 3 + > >> include/drm/drmfs.h| 77 > >> include/uapi/linux/magic.h | 3 + > >> 9 files changed, 773 insertions(+), 21 deletions(-) > >> create mode 100644 drivers/gpu/drm/drmfs.c > >> create mode 100644 include/drm/drmfs.h > > -- > Jani Nikula, Intel Open Source Technology Center ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[RFC 0/4] Introduce drmfs pseudo filesystem for drm subsystem
On Mon, 2016-12-12 at 07:33 -0800, Alex Deucher wrote: > On Mon, Dec 12, 2016 at 1:14 AM, sourab gupta > wrote: > > On Mon, 2016-12-05 at 03:06 -0800, Dhingra, Swati wrote: > >> From: Swati Dhingra > >> > >> Currently, we don't have a stable ABI which can be used for the purpose of > >> providing output debug/loggging/crc and other such data from DRM. > >> The ABI in current use (filesystems, ioctls, et al.) have their own > >> constraints and are intended to output a particular type of data. > >> Few cases in point: > >> sysfs - stable ABI, but constrained to one textual value per file > >> debugfs - unstable ABI, free-for-all > >> ioctls - not as suitable to many single purpose continuous data > >> dumping, we would very quickly run out ioctl space; requires more > >> userspace support than "cat" > >> device nodes - a real possibilty, kernel instantiation is more tricky, > >> requires udev (+udev.rules) or userspace discovery of the > >> dynamic major:minor (via sysfs) [mounting a registered > >> filesystem is easy in comparison] > >> netlink - stream based, therefore involves numerous copies. > >> > >> Debugfs is the lesser among the evils here, thereby we have grown used to > >> the > >> convenience and flexibility in presentation that debugfs gives us > >> (including relayfs inodes) that we want some of that hierachy in stable > >> user > >> ABI form. > >> > >> Due to these limitations, there is a need for a new pseudo filesytem, that > >> would act as a stable 'free-for-all' ABI, with the heirarchial structure > >> and > >> thus convenience of debugfs. This will be managed by drm, thus named > >> 'drmfs'. > >> DRM would register this filesystem to manage a canonical mountpoint, but > >> this > >> wouldn't limit everyone to only using that pseudofs underneath. > >> > >> This can serve to hold various kinds of output data from Linux DRM > >> subsystems, > >> for the files which can't truely fit anywhere else with existing ABI's but > >> present so, for the lack of a better place. > >> > >> In this patch series, we have introduced a pseudo filesystem named as > >> 'drmfs' > >> for now. The filesystem is introduced in the first patch, and the > >> subsequent > >> patches make use of the filesystem interfaces, in drm driver, and making > >> them > >> available for use by the drm subsystem components, one of which is i915. > >> We've moved the location of i915 GuC logs from debugfs to drmfs in the last > >> patch. Subsequently, more such files such as pipe_crc, error states, memory > >> stats, etc. can be move to this filesystem, if the idea introduced here is > >> acceptable per se. The filesystem introduced is being used to house the > >> data > >> generated by i915 driver in this patch series, but will hopefully be > >> generic > >> enough to provide scope for usage by any other drm subsystem component. > >> > >> The patch series is being floated as RFC to gather feedback on the idea and > >> infrastructure proposed here and it's suitability to address the specific > >> problem statement/use case. > >> > >> TODO: Create documentation. Will do so in next version. > >> > >> v2: fix the bat failures caused due to missing config check > >> > >> v3: Changes made: > >> - Move the location of drmfs from fs/ to drivers/gpu/drm/ (Chris) > >> - Moving config checks to header (Chris,Daniel) > >> > >> > >> Sourab Gupta (4): > >> drm: Introduce drmfs pseudo filesystem interfaces > >> drm: Register drmfs filesystem from drm init > >> drm: Create driver specific root directory inside drmfs > >> drm/i915: Creating guc log file in drmfs instead of debugfs > >> > >> drivers/gpu/drm/Kconfig| 9 + > >> drivers/gpu/drm/Makefile | 1 + > >> drivers/gpu/drm/drm_drv.c | 12 + > >> drivers/gpu/drm/drmfs.c| 555 > >> + > >> drivers/gpu/drm/i915/i915_guc_submission.c | 33 +- > >> include/drm/drm_drv.h | 3 + > >> include/drm/drmfs.h| 77 > >> include/uapi/linux/magic.h | 3 + > >
[RFC 0/4] Introduce drmfs pseudo filesystem for drm subsystem
On Mon, 2016-12-05 at 03:06 -0800, Dhingra, Swati wrote: > From: Swati Dhingra > > Currently, we don't have a stable ABI which can be used for the purpose of > providing output debug/loggging/crc and other such data from DRM. > The ABI in current use (filesystems, ioctls, et al.) have their own > constraints and are intended to output a particular type of data. > Few cases in point: > sysfs - stable ABI, but constrained to one textual value per file > debugfs - unstable ABI, free-for-all > ioctls - not as suitable to many single purpose continuous data > dumping, we would very quickly run out ioctl space; requires more > userspace support than "cat" > device nodes - a real possibilty, kernel instantiation is more tricky, > requires udev (+udev.rules) or userspace discovery of the > dynamic major:minor (via sysfs) [mounting a registered > filesystem is easy in comparison] > netlink - stream based, therefore involves numerous copies. > > Debugfs is the lesser among the evils here, thereby we have grown used to the > convenience and flexibility in presentation that debugfs gives us > (including relayfs inodes) that we want some of that hierachy in stable user > ABI form. > > Due to these limitations, there is a need for a new pseudo filesytem, that > would act as a stable 'free-for-all' ABI, with the heirarchial structure and > thus convenience of debugfs. This will be managed by drm, thus named 'drmfs'. > DRM would register this filesystem to manage a canonical mountpoint, but this > wouldn't limit everyone to only using that pseudofs underneath. > > This can serve to hold various kinds of output data from Linux DRM subsystems, > for the files which can't truely fit anywhere else with existing ABI's but > present so, for the lack of a better place. > > In this patch series, we have introduced a pseudo filesystem named as 'drmfs' > for now. The filesystem is introduced in the first patch, and the subsequent > patches make use of the filesystem interfaces, in drm driver, and making them > available for use by the drm subsystem components, one of which is i915. > We've moved the location of i915 GuC logs from debugfs to drmfs in the last > patch. Subsequently, more such files such as pipe_crc, error states, memory > stats, etc. can be move to this filesystem, if the idea introduced here is > acceptable per se. The filesystem introduced is being used to house the data > generated by i915 driver in this patch series, but will hopefully be generic > enough to provide scope for usage by any other drm subsystem component. > > The patch series is being floated as RFC to gather feedback on the idea and > infrastructure proposed here and it's suitability to address the specific > problem statement/use case. > > TODO: Create documentation. Will do so in next version. > > v2: fix the bat failures caused due to missing config check > > v3: Changes made: > - Move the location of drmfs from fs/ to drivers/gpu/drm/ (Chris) > - Moving config checks to header (Chris,Daniel) > > > Sourab Gupta (4): > drm: Introduce drmfs pseudo filesystem interfaces > drm: Register drmfs filesystem from drm init > drm: Create driver specific root directory inside drmfs > drm/i915: Creating guc log file in drmfs instead of debugfs > > drivers/gpu/drm/Kconfig| 9 + > drivers/gpu/drm/Makefile | 1 + > drivers/gpu/drm/drm_drv.c | 12 + > drivers/gpu/drm/drmfs.c| 555 > + > drivers/gpu/drm/i915/i915_guc_submission.c | 33 +- > include/drm/drm_drv.h | 3 + > include/drm/drmfs.h| 77 > include/uapi/linux/magic.h | 3 + > 8 files changed, 672 insertions(+), 21 deletions(-) > create mode 100644 drivers/gpu/drm/drmfs.c > create mode 100644 include/drm/drmfs.h > > -- > 2.7.4 > Hi dri-devel folks, Any feedback on the proposed drmfs infrastructure being proposed here? Regards, Sourab
[PATCH v9 06/11] drm/i915: Enable i915 perf stream for Haswell OA unit
On Mon, 2016-11-07 at 11:49 -0800, Robert Bragg wrote: > Gen graphics hardware can be set up to periodically write snapshots of > performance counters into a circular buffer via its Observation > Architecture and this patch exposes that capability to userspace via > the > i915 perf interface. > > v2: >Make sure to initialize ->specific_ctx_id when opening, without >relying on _pin_notify hook, in case ctx already pinned. > v3: >Revert back to pinning ctx upfront when opening stream, removing >need to hook in to pinning and to update OACONTROL on the fly. > > Signed-off-by: Robert Bragg > Signed-off-by: Zhenyu Wang > Cc: Chris Wilson > Reviewed-by: Matthew Auld Have been working for quite some time on extending the interfaces per the usecase of multiple concurrent streams (on different engines), and infrastructure fits quite well for these usecases. With Chris' comments addressed, the patch can have my r-b. Reviewed-by: Sourab Gupta
[PATCH v9 09/11] drm/i915: add dev.i915.oa_max_sample_rate sysctl
On Tue, 2016-11-08 at 03:47 -0800, Robert Bragg wrote:
>
>
> On Tue, Nov 8, 2016 at 6:19 AM, sourab gupta
> wrote:
> On Mon, 2016-11-07 at 11:49 -0800, Robert Bragg wrote:
> > The maximum OA sampling frequency is now configurable via a
> > dev.i915.oa_max_sample_rate sysctl parameter.
> >
> > Following the precedent set by perf's similar
> > kernel.perf_event_max_sample_rate the default maximum rate
> is 10Hz
> >
> > Signed-off-by: Robert Bragg
> > ---
> > drivers/gpu/drm/i915/i915_perf.c | 61
>
> > 1 file changed, 50 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_perf.c
> b/drivers/gpu/drm/i915/i915_perf.c
> > index e51c1d8..1a87fe9 100644
> > --- a/drivers/gpu/drm/i915/i915_perf.c
> > +++ b/drivers/gpu/drm/i915/i915_perf.c
> > @@ -82,6 +82,21 @@ static u32 i915_perf_stream_paranoid =
> true;
> > #define INVALID_CTX_ID 0x
> >
> >
> > +/* For sysctl proc_dointvec_minmax of
> i915_oa_max_sample_rate
> > + *
> > + * 160ns is the smallest sampling period we can
> theoretically program the OA
> > + * unit with on Haswell, corresponding to 6.25MHz.
> > + */
> > +static int oa_sample_rate_hard_limit = 625;
> There's no check for 'oa_sample_rate_hard_limit' anywhere
> below.
>
>
> It's in the struct ctl_table oa_table[] declaration of the
> "oa_max_sample_rate" paramater, assigned to .extra2 which is
> referenced by the proc_dointvec_minmax validation handler for the
> parameter.
>
Ok. Seems fine then.
>
>
> > +
> > +/* Theoretically we can program the OA unit to sample every
> 160ns but don't
> > + * allow that by default unless root...
> > + *
> > + * The default threshold of 10Hz is based on perf's
> similar
> > + * kernel.perf_event_max_sample_rate sysctl parameter.
> > + */
> > +static u32 i915_oa_max_sample_rate = 10;
> > +
> > /* XXX: beware if future OA HW adds new report formats that
> the current
> > * code assumes all reports have a power-of-two size and
> ~(size - 1) can
> > * be used as a mask to align the OA tail pointer.
> > @@ -1314,6 +1329,7 @@ static int
> read_properties_unlocked(struct drm_i915_private *dev_priv,
> > }
> >
> > for (i = 0; i < n_props; i++) {
> > + u64 oa_period, oa_freq_hz;
> > u64 id, value;
> > int ret;
> >
> > @@ -1359,21 +1375,35 @@ static int
> read_properties_unlocked(struct drm_i915_private *dev_priv,
> > return -EINVAL;
> > }
> >
> > - /* NB: The exponent represents a
> period as follows:
> > - *
> > - * 80ns * 2^(period_exponent + 1)
> > - *
> > - * Theoretically we can program the OA
> unit to sample
> > + /* Theoretically we can program the OA
> unit to sample
> >* every 160ns but don't allow that by
> default unless
> >* root.
> >*
> > - * Referring to perf's
> > - * kernel.perf_event_max_sample_rate
> for a precedent
> > - * (10 by default); with an OA
> exponent of 6 we get
> > - * a period of 10.240 microseconds
> -just under 10Hz
> > + * On Haswell the period is derived
> from the exponent
> > + * as:
> > + *
> > + * period = 80ns * 2^(exponent + 1)
> > + */
> > + BUILD_BUG_ON(sizeof(oa_period) != 8);
> > + oa_period = 80ull * (2ull << value);
>
[PATCH v9 11/11] drm/i915: Add a kerneldoc summary for i915_perf.c
On Mon, 2016-11-07 at 11:49 -0800, Robert Bragg wrote: > In particular this tries to capture for posterity some of the early > challenges we had with using the core perf infrastructure in case we > ever want to revisit adapting perf for device metrics. > > Cc: Chris Wilson > Signed-off-by: Robert Bragg > Reviewed-by: Matthew Auld > --- Good summary of early challenges faced while adapting core perf. Reviewed-by: Sourab Gupta
[PATCH v9 09/11] drm/i915: add dev.i915.oa_max_sample_rate sysctl
On Mon, 2016-11-07 at 11:49 -0800, Robert Bragg wrote:
> The maximum OA sampling frequency is now configurable via a
> dev.i915.oa_max_sample_rate sysctl parameter.
>
> Following the precedent set by perf's similar
> kernel.perf_event_max_sample_rate the default maximum rate is 10Hz
>
> Signed-off-by: Robert Bragg
> ---
> drivers/gpu/drm/i915/i915_perf.c | 61
>
> 1 file changed, 50 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_perf.c
> b/drivers/gpu/drm/i915/i915_perf.c
> index e51c1d8..1a87fe9 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -82,6 +82,21 @@ static u32 i915_perf_stream_paranoid = true;
> #define INVALID_CTX_ID 0x
>
>
> +/* For sysctl proc_dointvec_minmax of i915_oa_max_sample_rate
> + *
> + * 160ns is the smallest sampling period we can theoretically program the OA
> + * unit with on Haswell, corresponding to 6.25MHz.
> + */
> +static int oa_sample_rate_hard_limit = 625;
There's no check for 'oa_sample_rate_hard_limit' anywhere below.
> +
> +/* Theoretically we can program the OA unit to sample every 160ns but don't
> + * allow that by default unless root...
> + *
> + * The default threshold of 10Hz is based on perf's similar
> + * kernel.perf_event_max_sample_rate sysctl parameter.
> + */
> +static u32 i915_oa_max_sample_rate = 10;
> +
> /* XXX: beware if future OA HW adds new report formats that the current
> * code assumes all reports have a power-of-two size and ~(size - 1) can
> * be used as a mask to align the OA tail pointer.
> @@ -1314,6 +1329,7 @@ static int read_properties_unlocked(struct
> drm_i915_private *dev_priv,
> }
>
> for (i = 0; i < n_props; i++) {
> + u64 oa_period, oa_freq_hz;
> u64 id, value;
> int ret;
>
> @@ -1359,21 +1375,35 @@ static int read_properties_unlocked(struct
> drm_i915_private *dev_priv,
> return -EINVAL;
> }
>
> - /* NB: The exponent represents a period as follows:
> - *
> - * 80ns * 2^(period_exponent + 1)
> - *
> - * Theoretically we can program the OA unit to sample
> + /* Theoretically we can program the OA unit to sample
>* every 160ns but don't allow that by default unless
>* root.
>*
> - * Referring to perf's
> - * kernel.perf_event_max_sample_rate for a precedent
> - * (10 by default); with an OA exponent of 6 we get
> - * a period of 10.240 microseconds -just under 10Hz
> + * On Haswell the period is derived from the exponent
> + * as:
> + *
> + * period = 80ns * 2^(exponent + 1)
> + */
> + BUILD_BUG_ON(sizeof(oa_period) != 8);
> + oa_period = 80ull * (2ull << value);
I assume now that there'll be a platform specific check for 80ull, while
programming oa_period, for subquent Gen8+ platforms, which should be
fine.
> +
> + /* This check is primarily to ensure that oa_period <=
> + * UINT32_MAX (before passing to do_div which only
> + * accepts a u32 denominator), but we can also skip
> + * checking anything < 1Hz which implicitly can't be
> + * limited via an integer oa_max_sample_rate.
>*/
> - if (value < 6 && !capable(CAP_SYS_ADMIN)) {
> - DRM_ERROR("Minimum OA sampling exponent is 6
> without root privileges\n");
> + if (oa_period <= NSEC_PER_SEC) {
> + u64 tmp = NSEC_PER_SEC;
> + do_div(tmp, oa_period);
> + oa_freq_hz = tmp;
> + } else
> + oa_freq_hz = 0;
> +
> + if (oa_freq_hz > i915_oa_max_sample_rate &&
> + !capable(CAP_SYS_ADMIN)) {
> + DRM_ERROR("OA exponent would exceed the max
> sampling frequency (sysctl dev.i915.oa_max_sample_rate) %uHz without root
> privileges\n",
> + i915_oa_max_sample_rate);
> return -EACCES;
> }
>
> @@ -1481,6 +1511,15 @@ static struct ctl_table oa_table[] = {
>.extra1 = ,
>.extra2 = ,
>},
> + {
> + .procname = "oa_max_sample_rate",
> + .data = _oa_max_sample_rate,
> + .maxlen = sizeof(i915_oa_max_sample_rate),
> + .mode = 0644,
> + .proc_handler = proc_dointvec_minmax,
> +
[PATCH v9 05/11] drm/i915: Add 'render basic' Haswell OA unit config
On Mon, 2016-11-07 at 11:49 -0800, Robert Bragg wrote: > Adds a static OA unit, MUX + B Counter configuration for basic render > metrics on Haswell. This is auto generated from an XML > description of metric sets, currently maintained in gputop, ref: > > https://github.com/rib/gputop > > gputop-data/oa-*.xml > > scripts/i915-perf-kernelgen.py > > $ make -C gputop-data -f Makefile.xml SYSFS=0 WHITELIST=RenderBasic > > Signed-off-by: Robert Bragg > Reviewed-by: Matthew Auld > --- Looks good. Reviewed-by: Sourab Gupta
[PATCH v8 02/12] drm/i915: Add i915 perf infrastructure
On Fri, 2016-11-04 at 06:19 -0700, Robert Bragg wrote:
>
>
> On Fri, Nov 4, 2016 at 8:59 AM, sourab gupta
> wrote:
> On Thu, 2016-10-27 at 19:14 -0700, Robert Bragg wrote:
> > Adds base i915 perf infrastructure for Gen performance
> metrics.
> >
> > This adds a DRM_IOCTL_I915_PERF_OPEN ioctl that takes an
> array of uint64
> > properties to configure a stream of metrics and returns a
> new fd usable
> > with standard VFS system calls including read() to read
> typed and sized
> > records; ioctl() to enable or disable capture and poll() to
> wait for
> > data.
> >
> > A stream is opened something like:
> >
> > uint64_t properties[] = {
> > /* Single context sampling */
> > DRM_I915_PERF_PROP_CTX_HANDLE,ctx_handle,
> >
> > /* Include OA reports in samples */
> > DRM_I915_PERF_PROP_SAMPLE_OA, true,
> >
> > /* OA unit configuration */
> > DRM_I915_PERF_PROP_OA_METRICS_SET,metrics_set_id,
> > DRM_I915_PERF_PROP_OA_FORMAT, report_format,
> > DRM_I915_PERF_PROP_OA_EXPONENT, period_exponent,
> >};
> >struct drm_i915_perf_open_param parm = {
> > .flags = I915_PERF_FLAG_FD_CLOEXEC |
> >I915_PERF_FLAG_FD_NONBLOCK |
> >I915_PERF_FLAG_DISABLED,
> > .properties_ptr = (uint64_t)properties,
> > .num_properties = sizeof(properties) / 16,
> >};
> >int fd = drmIoctl(drm_fd, DRM_IOCTL_I915_PERF_OPEN,
> );
> >
> > Records read all start with a common { type, size } header
> with
> > DRM_I915_PERF_RECORD_SAMPLE being of most interest. Sample
> records
> > contain an extensible number of fields and it's the
> > DRM_I915_PERF_PROP_SAMPLE_xyz properties given when opening
> that
> > determine what's included in every sample.
> >
> > No specific streams are supported yet so any attempt to open
> a stream
> > will return an error.
> >
> > v2:
> > use i915_gem_context_get() - Chris Wilson
> > v3:
> > update read() interface to avoid passing state struct -
> Chris Wilson
> > fix some rebase fallout, with i915-perf init/deinit
> > v4:
> > s/DRM_IORW/DRM_IOW/ - Emil Velikov
> >
> > Signed-off-by: Robert Bragg
> > ---
> > drivers/gpu/drm/i915/Makefile| 3 +
> > drivers/gpu/drm/i915/i915_drv.c | 4 +
> > drivers/gpu/drm/i915/i915_drv.h | 91
> > drivers/gpu/drm/i915/i915_perf.c | 443
> +++
> > include/uapi/drm/i915_drm.h | 67 ++
> > 5 files changed, 608 insertions(+)
> > create mode 100644 drivers/gpu/drm/i915/i915_perf.c
> >
> > diff --git a/drivers/gpu/drm/i915/Makefile
> b/drivers/gpu/drm/i915/Makefile
> > index 6123400..8d4e25f 100644
> > --- a/drivers/gpu/drm/i915/Makefile
> > +++ b/drivers/gpu/drm/i915/Makefile
> > @@ -113,6 +113,9 @@ i915-$(CONFIG_DRM_I915_CAPTURE_ERROR) +=
> i915_gpu_error.o
> > # virtual gpu code
> > i915-y += i915_vgpu.o
> >
> > +# perf code
> > +i915-y += i915_perf.o
> > +
> > ifeq ($(CONFIG_DRM_I915_GVT),y)
> > i915-y += intel_gvt.o
> > include $(src)/gvt/Makefile
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> b/drivers/gpu/drm/i915/i915_drv.c
> > index af3559d..685c96e 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -836,6 +836,8 @@ static int i915_driver_init_early(struct
> drm_i915_private *dev_priv,
> >
> > intel_detect_preproduction_hw(dev_priv);
> >
> > + i915_perf_init(dev_priv);
> > +
> > return 0;
> >
> > err_workqueues:
> > @@ -849,6 +851,7 @@ static int i915_driver_init_early(struct
> drm_
[PATCH v8 05/12] drm/i915: don't whitelist oacontrol in cmd parser
On Thu, 2016-10-27 at 19:14 -0700, Robert Bragg wrote: > Being able to program OACONTROL from a non-privileged batch buffer is > not sufficient to be able to configure the OA unit. This was originally > allowed to help enable Mesa to expose OA counters via the > INTEL_performance_query extension, but the current implementation based > on programming OACONTROL via a batch buffer isn't able to report useable > data without a more complete OA unit configuration. Mesa handles the > possibility that writes to OACONTROL may not be allowed and so only > advertises the extension after explicitly testing that a write to > OACONTROL succeeds. Based on this; removing OACONTROL from the whitelist > should be ok for userspace. > > Removing this simplifies adding a new kernel api for configuring the OA > unit without needing to consider the possibility that userspace might > trample on OACONTROL state which we'd like to start managing within > the kernel instead. In particular running any Mesa based GL application > currently results in clearing OACONTROL when initializing which would > disable the capturing of metrics. > > Signed-off-by: Robert Bragg > Reviewed-by: Matthew Auld Seems reasonable. Reviewed-by: Sourab Gupta
[PATCH v8 09/12] drm/i915: Add dev.i915.perf_stream_paranoid sysctl option
On Thu, 2016-10-27 at 19:14 -0700, Robert Bragg wrote:
> Consistent with the kernel.perf_event_paranoid sysctl option that can
> allow non-root users to access system wide cpu metrics, this can
> optionally allow non-root users to access system wide OA counter metrics
> from Gen graphics hardware.
>
> Signed-off-by: Robert Bragg
> Reviewed-by: Matthew Auld
> ---
> drivers/gpu/drm/i915/i915_drv.h | 1 +
> drivers/gpu/drm/i915/i915_perf.c | 50
> +++-
> 2 files changed, 50 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 01438fb..a138f86 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2171,6 +2171,7 @@ struct drm_i915_private {
> bool initialized;
>
> struct kobject *metrics_kobj;
> + struct ctl_table_header *sysctl_header;
>
> struct mutex lock;
> struct list_head streams;
> diff --git a/drivers/gpu/drm/i915/i915_perf.c
> b/drivers/gpu/drm/i915/i915_perf.c
> index 8d07c41..4e42073 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -64,6 +64,11 @@
> #define POLL_FREQUENCY 200
> #define POLL_PERIOD (NSEC_PER_SEC / POLL_FREQUENCY)
>
> +/* for sysctl proc_dointvec_minmax of dev.i915.perf_stream_paranoid */
> +static int zero;
> +static int one = 1;
> +static u32 i915_perf_stream_paranoid = true;
> +
> /* The maximum exponent the hardware accepts is 63 (essentially it selects
> one
> * of the 64bit timestamp bits to trigger reports from) but there's currently
> * no known use case for sampling as infrequently as once per 47 thousand
> years.
> @@ -1207,7 +1212,13 @@ i915_perf_open_ioctl_locked(struct drm_i915_private
> *dev_priv,
> }
> }
>
> - if (!specific_ctx && !capable(CAP_SYS_ADMIN)) {
> + /* Similar to perf's kernel.perf_paranoid_cpu sysctl option
> + * we check a dev.i915.perf_stream_paranoid sysctl option
> + * to determine if it's ok to access system wide OA counters
> + * without CAP_SYS_ADMIN privileges.
> + */
> + if (!specific_ctx &&
> + i915_perf_stream_paranoid && !capable(CAP_SYS_ADMIN)) {
> DRM_ERROR("Insufficient privileges to open system-wide i915
> perf stream\n");
> ret = -EACCES;
> goto err_ctx;
> @@ -1454,6 +1465,39 @@ void i915_perf_unregister(struct drm_i915_private
> *dev_priv)
> dev_priv->perf.metrics_kobj = NULL;
> }
>
> +static struct ctl_table oa_table[] = {
> + {
> + .procname = "perf_stream_paranoid",
> + .data = _perf_stream_paranoid,
> + .maxlen = sizeof(i915_perf_stream_paranoid),
> + .mode = 0644,
> + .proc_handler = proc_dointvec_minmax,
> + .extra1 = ,
> + .extra2 = ,
> + },
> + {}
> +};
> +
> +static struct ctl_table i915_root[] = {
> + {
> + .procname = "i915",
> + .maxlen = 0,
> + .mode = 0555,
> + .child = oa_table,
> + },
> + {}
> +};
> +
> +static struct ctl_table dev_root[] = {
> + {
> + .procname = "dev",
> + .maxlen = 0,
> + .mode = 0555,
> + .child = i915_root,
> + },
> + {}
> +};
> +
> void i915_perf_init(struct drm_i915_private *dev_priv)
> {
> if (!IS_HASWELL(dev_priv))
> @@ -1484,6 +1528,8 @@ void i915_perf_init(struct drm_i915_private *dev_priv)
> dev_priv->perf.oa.n_builtin_sets =
> i915_oa_n_builtin_metric_sets_hsw;
>
> + dev_priv->perf.sysctl_header = register_sysctl_table(dev_root);
> +
> dev_priv->perf.initialized = true;
> }
>
> @@ -1492,6 +1538,8 @@ void i915_perf_fini(struct drm_i915_private *dev_priv)
> if (!dev_priv->perf.initialized)
> return;
>
> + unregister_sysctl_table(dev_priv->perf.sysctl_header);
> +
> memset(_priv->perf.oa.ops, 0, sizeof(dev_priv->perf.oa.ops));
> dev_priv->perf.initialized = false;
> }
Looks fine.
Reviewed-by: Sourab Gupta
[PATCH v8 08/12] drm/i915: advertise available metrics via sysfs
On Thu, 2016-10-27 at 19:14 -0700, Robert Bragg wrote: > Each metric set is given a sysfs entry like: > > /sys/class/drm/card0/metrics//id > > This allows userspace to enumerate the specific sets that are available > for the current system. The 'id' file contains an unsigned integer that > can be used to open the associated metric set via > DRM_IOCTL_I915_PERF_OPEN. The is a globally unique ID for a > specific OA unit register configuration that can be reliably used by > userspace as a key to lookup corresponding counter meta data and > normalization equations. > > The guid registry is currently maintained as part of gputop along with > the XML metric set descriptions and code generation scripts, ref: > > https://github.com/rib/gputop > > gputop-data/guids.xml > > scripts/update-guids.py > > gputop-data/oa-*.xml > > scripts/i915-perf-kernelgen.py > > $ make -C gputop-data -f Makefile.xml SYSFS=1 WHITELIST=RenderBasic > > Signed-off-by: Robert Bragg > Reviewed-by: Matthew Auld Looks good to me. Reviewed-by: Sourab Gupta
[PATCH v8 02/12] drm/i915: Add i915 perf infrastructure
On Thu, 2016-10-27 at 19:14 -0700, Robert Bragg wrote:
> Adds base i915 perf infrastructure for Gen performance metrics.
>
> This adds a DRM_IOCTL_I915_PERF_OPEN ioctl that takes an array of uint64
> properties to configure a stream of metrics and returns a new fd usable
> with standard VFS system calls including read() to read typed and sized
> records; ioctl() to enable or disable capture and poll() to wait for
> data.
>
> A stream is opened something like:
>
> uint64_t properties[] = {
> /* Single context sampling */
> DRM_I915_PERF_PROP_CTX_HANDLE,ctx_handle,
>
> /* Include OA reports in samples */
> DRM_I915_PERF_PROP_SAMPLE_OA, true,
>
> /* OA unit configuration */
> DRM_I915_PERF_PROP_OA_METRICS_SET,metrics_set_id,
> DRM_I915_PERF_PROP_OA_FORMAT, report_format,
> DRM_I915_PERF_PROP_OA_EXPONENT, period_exponent,
>};
>struct drm_i915_perf_open_param parm = {
> .flags = I915_PERF_FLAG_FD_CLOEXEC |
>I915_PERF_FLAG_FD_NONBLOCK |
>I915_PERF_FLAG_DISABLED,
> .properties_ptr = (uint64_t)properties,
> .num_properties = sizeof(properties) / 16,
>};
>int fd = drmIoctl(drm_fd, DRM_IOCTL_I915_PERF_OPEN, );
>
> Records read all start with a common { type, size } header with
> DRM_I915_PERF_RECORD_SAMPLE being of most interest. Sample records
> contain an extensible number of fields and it's the
> DRM_I915_PERF_PROP_SAMPLE_xyz properties given when opening that
> determine what's included in every sample.
>
> No specific streams are supported yet so any attempt to open a stream
> will return an error.
>
> v2:
> use i915_gem_context_get() - Chris Wilson
> v3:
> update read() interface to avoid passing state struct - Chris Wilson
> fix some rebase fallout, with i915-perf init/deinit
> v4:
> s/DRM_IORW/DRM_IOW/ - Emil Velikov
>
> Signed-off-by: Robert Bragg
> ---
> drivers/gpu/drm/i915/Makefile| 3 +
> drivers/gpu/drm/i915/i915_drv.c | 4 +
> drivers/gpu/drm/i915/i915_drv.h | 91
> drivers/gpu/drm/i915/i915_perf.c | 443
> +++
> include/uapi/drm/i915_drm.h | 67 ++
> 5 files changed, 608 insertions(+)
> create mode 100644 drivers/gpu/drm/i915/i915_perf.c
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 6123400..8d4e25f 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -113,6 +113,9 @@ i915-$(CONFIG_DRM_I915_CAPTURE_ERROR) += i915_gpu_error.o
> # virtual gpu code
> i915-y += i915_vgpu.o
>
> +# perf code
> +i915-y += i915_perf.o
> +
> ifeq ($(CONFIG_DRM_I915_GVT),y)
> i915-y += intel_gvt.o
> include $(src)/gvt/Makefile
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index af3559d..685c96e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -836,6 +836,8 @@ static int i915_driver_init_early(struct drm_i915_private
> *dev_priv,
>
> intel_detect_preproduction_hw(dev_priv);
>
> + i915_perf_init(dev_priv);
> +
> return 0;
>
> err_workqueues:
> @@ -849,6 +851,7 @@ static int i915_driver_init_early(struct drm_i915_private
> *dev_priv,
> */
> static void i915_driver_cleanup_early(struct drm_i915_private *dev_priv)
> {
> + i915_perf_fini(dev_priv);
> i915_gem_load_cleanup(_priv->drm);
> i915_workqueues_cleanup(dev_priv);
> }
> @@ -2556,6 +2559,7 @@ static const struct drm_ioctl_desc i915_ioctls[] = {
> DRM_IOCTL_DEF_DRV(I915_GEM_USERPTR, i915_gem_userptr_ioctl,
> DRM_RENDER_ALLOW),
> DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_GETPARAM,
> i915_gem_context_getparam_ioctl, DRM_RENDER_ALLOW),
> DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_SETPARAM,
> i915_gem_context_setparam_ioctl, DRM_RENDER_ALLOW),
> + DRM_IOCTL_DEF_DRV(I915_PERF_OPEN, i915_perf_open_ioctl,
> DRM_RENDER_ALLOW),
> };
>
> static struct drm_driver driver = {
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 5a260db..7a65c0b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1767,6 +1767,84 @@ struct intel_wm_config {
> bool sprites_scaled;
> };
>
> +struct i915_perf_stream;
> +
> +struct i915_perf_stream_ops {
> + /* Enables the collection of HW samples, either in response to
> + * I915_PERF_IOCTL_ENABLE or implicitly called when stream is
> + * opened without I915_PERF_FLAG_DISABLED.
> + */
> + void (*enable)(struct i915_perf_stream *stream);
> +
> + /* Disables the collection of HW samples, either in response to
> + * I915_PERF_IOCTL_DISABLE or implicitly called before
> + * destroying the stream.
> + */
> + void (*disable)(struct i915_perf_stream *stream);
> +
> + /* Return: true if any i915 perf records are ready to read()
> + * for this stream.
> + */
> +
[PATCH v8 04/12] drm/i915: return EACCES for check_cmd() failures
On Thu, 2016-10-27 at 19:14 -0700, Robert Bragg wrote: > check_cmd() is checking whether a command adheres to certain > restrictions that ensure it's safe to execute within a privileged batch > buffer. Returning false implies a privilege problem, not that the > command is invalid. > > The distinction makes the difference between allowing the buffer to be > executed as an unprivileged batch buffer or returning an EINVAL error to > userspace without executing anything. > > In a case where userspace may want to test whether it can successfully > write to a register that needs privileges the distinction may be > important and an EINVAL error may be considered fatal. > > In particular this is currently true for Mesa, which includes a test for > whether OACONTROL can be written too, but Mesa treats any error when > flushing a batch buffer as fatal, calling exit(1). > > As it is currently Mesa can gracefully handle a failure to write to > OACONTROL if the command parser is disabled, but if we were to remove > OACONTROL from the parser's whitelist then the returned EINVAL would > break Mesa applications as they attempt an OACONTROL write. > > This bumps the command parser version from 7 to 8, as the change is > visible to userspace. > > Signed-off-by: Robert Bragg > Reviewed-by: Matthew Auld Well, looks reasonable to me. Reviewed-by: Sourab Gupta
[PATCH v8 03/12] drm/i915: rename OACONTROL GEN7_OACONTROL
On Thu, 2016-10-27 at 19:14 -0700, Robert Bragg wrote: > OACONTROL changes quite a bit for gen8, with some bits split out into a > per-context OACTXCONTROL register. Rename now before adding more gen7 OA > registers > > Signed-off-by: Robert Bragg > Reviewed-by: Matthew Auld Reviewed-by: Sourab Gupta
[PATCH v8 10/12] drm/i915: add oa_event_min_timer_exponent sysctl
On Thu, 2016-10-27 at 19:14 -0700, Robert Bragg wrote:
> The minimal sampling period is now configurable via a
> dev.i915.oa_min_timer_exponent sysctl parameter.
>
> Following the precedent set by perf, the default is the minimum that
> won't (on its own) exceed the default kernel.perf_event_max_sample_rate
> default of 10 samples/s.
>
> Signed-off-by: Robert Bragg
> Reviewed-by: Matthew Auld
> ---
> drivers/gpu/drm/i915/i915_perf.c | 42
>
> 1 file changed, 30 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_perf.c
> b/drivers/gpu/drm/i915/i915_perf.c
> index 4e42073..e3c6f51 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -82,6 +82,22 @@ static u32 i915_perf_stream_paranoid = true;
> #define INVALID_CTX_ID 0x
>
>
> +/* for sysctl proc_dointvec_minmax of i915_oa_min_timer_exponent */
> +static int oa_exponent_max = OA_EXPONENT_MAX;
> +
> +/* Theoretically we can program the OA unit to sample every 160ns but don't
> + * allow that by default unless root...
> + *
> + * The period is derived from the exponent as:
> + *
> + * period = 80ns * 2^(exponent + 1)
> + *
> + * Referring to perf's kernel.perf_event_max_sample_rate for a precedent
> + * (10 by default); with an OA exponent of 6 we get a period of 10.240
> + * microseconds - just under 10Hz
> + */
> +static u32 i915_oa_min_timer_exponent = 6;
For HSW, the timestamp period is 80ns, so the exponent of 6 translates
to sampling rate of ~10Hz. But the timestamp period may change for
other platforms, leading to different values of oa_min_timer_exponent
corresponding to sampling rate of ~10Hz. Do we plan to have this
value platform specific subsequently, or the guidance value of ~10Hz
min sampling rate needn't be strictly followed?
> +
> /* XXX: beware if future OA HW adds new report formats that the current
> * code assumes all reports have a power-of-two size and ~(size - 1) can
> * be used as a mask to align the OA tail pointer.
> @@ -1353,21 +1369,14 @@ static int read_properties_unlocked(struct
> drm_i915_private *dev_priv,
> return -EINVAL;
> }
>
> - /* NB: The exponent represents a period as follows:
> - *
> - * 80ns * 2^(period_exponent + 1)
> - *
> - * Theoretically we can program the OA unit to sample
> + /* Theoretically we can program the OA unit to sample
>* every 160ns but don't allow that by default unless
>* root.
> - *
> - * Referring to perf's
> - * kernel.perf_event_max_sample_rate for a precedent
> - * (10 by default); with an OA exponent of 6 we get
> - * a period of 10.240 microseconds -just under 10Hz
>*/
> - if (value < 6 && !capable(CAP_SYS_ADMIN)) {
> - DRM_ERROR("Minimum OA sampling exponent is 6
> without root privileges\n");
> + if (value < i915_oa_min_timer_exponent &&
> + !capable(CAP_SYS_ADMIN)) {
> + DRM_ERROR("Minimum OA sampling exponent (sysctl
> dev.i915.oa_min_timer_exponent) is %u without root privileges\n",
> + i915_oa_min_timer_exponent);
> return -EACCES;
> }
>
> @@ -1475,6 +1484,15 @@ static struct ctl_table oa_table[] = {
>.extra1 = ,
>.extra2 = ,
>},
> + {
> + .procname = "oa_min_timer_exponent",
> + .data = _oa_min_timer_exponent,
> + .maxlen = sizeof(i915_oa_min_timer_exponent),
> + .mode = 0644,
> + .proc_handler = proc_dointvec_minmax,
> + .extra1 = ,
> + .extra2 = _exponent_max,
> + },
> {}
> };
>
