On Fri, Dec 02, 2016 at 12:18:44PM +0000, Robert Bragg wrote:
> On Fri, Dec 2, 2016 at 8:35 AM, Daniel Vetter <daniel@xxxxxxxx> wrote:
>
> > On Thu, Dec 01, 2016 at 05:21:52PM +0000, Robert Bragg wrote:
> > > Avoid using DRM_ERROR for conditions userspace can trigger with a bad
> > > config when opening a stream or from not reading data in a timely
> > > fashion (whereby the OA buffer fills up). These conditions are tested
> > > by i-g-t which treats error messages as failures if using the test
> > > runner. This wasn't an issue while the i915-perf igt tests were being
> > > run in isolation.
> > >
> > > One message relating to seeing a spurious zeroed report was changed to
> > > use DRM_NOTE instead of DRM_ERROR. Ideally this warning shouldn't be
> > > seen, but it's not a serious problem if it is. Considering that the
> > > tail margin mechanism is only a heuristic it's possible we might see
> > > this from time to time.
> > >
> > > Signed-off-by: Robert Bragg <robert@xxxxxxxxxxxxx:
> > > Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > >
> > > fix i915_perf dbg messages
> > > ---
> > > drivers/gpu/drm/i915/i915_perf.c | 42 ++++++++++++++++++++----------
> > ----------
> > > 1 file changed, 21 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_perf.c
> > b/drivers/gpu/drm/i915/i915_perf.c
> > > index 9551282..5705005 100644
> > > --- a/drivers/gpu/drm/i915/i915_perf.c
> > > +++ b/drivers/gpu/drm/i915/i915_perf.c
> > > @@ -474,7 +474,7 @@ static int gen7_append_oa_reports(struct
> > i915_perf_stream *stream,
> > > * copying it to userspace...
> > > */
> > > if (report32[0] == 0) {
> > > - DRM_ERROR("Skipping spurious, invalid OA
> > report\n");
> > > + DRM_NOTE("Skipping spurious, invalid OA report\n");
> > > continue;
> >
> > The above looks like a genuine hw/kernel fail, which we shouldn't put
> > under the carpet. I'd leave it at DRM_ERROR - I can bikeshed that while
> > applying if you're ok. Otherwise lgtm, will apply as soon as we've
> > clarified that.
> >
>
> It's something that is unfortunately expected to be possible from time to
> time due to a hardware race condition between the OA unit updating the tail
> pointer for a new report and that report actually becoming visible to the
> cpu in memory.
>
> If/when it happens it's not really a significant problem for userspace
> (assuming it's rare/intermittent given what the driver does as a
> best-effort workaround here). Userspace sees a briefly lower sampling
> resolution but the metrics can still be normalized.
>
> We wouldn't want i-g-t failing in this case, so that's why I was changing
> it.
>
> It's not really something you want to see ideally (it implies our
> heuristic-based software workaround isn't perfect). If it's seen a lot then
> that certainly should be considered a warning that we need to try and
> improve how we workaround the race condition. If you see it rarely then is
> somewhere between a note, and a warning I suppose.
Ok makes sense, patch applied.
-Daniel
>
> Regards,
> - Robert
>
>
> > -Daniel
> >
> > > }
> > >
> > > @@ -551,7 +551,7 @@ static int gen7_oa_read(struct i915_perf_stream
> > *stream,
> > > if (ret)
> > > return ret;
> > >
> > > - DRM_ERROR("OA buffer overflow: force restart\n");
> > > + DRM_DEBUG("OA buffer overflow: force restart\n");
> > >
> > > dev_priv->perf.oa.ops.oa_disable(dev_priv);
> > > dev_priv->perf.oa.ops.oa_enable(dev_priv);
> > > @@ -1000,17 +1000,17 @@ static int i915_oa_stream_init(struct
> > i915_perf_stream *stream,
> > > * IDs
> > > */
> > > if (!dev_priv->perf.metrics_kobj) {
> > > - DRM_ERROR("OA metrics weren't advertised via sysfs\n");
> > > + DRM_DEBUG("OA metrics weren't advertised via sysfs\n");
> > > return -EINVAL;
> > > }
> > >
> > > if (!(props->sample_flags & SAMPLE_OA_REPORT)) {
> > > - DRM_ERROR("Only OA report sampling supported\n");
> > > + DRM_DEBUG("Only OA report sampling supported\n");
> > > return -EINVAL;
> > > }
> > >
> > > if (!dev_priv->perf.oa.ops.init_oa_buffer) {
> > > - DRM_ERROR("OA unit not supported\n");
> > > + DRM_DEBUG("OA unit not supported\n");
> > > return -ENODEV;
> > > }
> > >
> > > @@ -1019,17 +1019,17 @@ static int i915_oa_stream_init(struct
> > i915_perf_stream *stream,
> > > * we currently only allow exclusive access
> > > */
> > > if (dev_priv->perf.oa.exclusive_stream) {
> > > - DRM_ERROR("OA unit already in use\n");
> > > + DRM_DEBUG("OA unit already in use\n");
> > > return -EBUSY;
> > > }
> > >
> > > if (!props->metrics_set) {
> > > - DRM_ERROR("OA metric set not specified\n");
> > > + DRM_DEBUG("OA metric set not specified\n");
> > > return -EINVAL;
> > > }
> > >
> > > if (!props->oa_format) {
> > > - DRM_ERROR("OA report format not specified\n");
> > > + DRM_DEBUG("OA report format not specified\n");
> > > return -EINVAL;
> > > }
> > >
> > > @@ -1384,7 +1384,7 @@ i915_perf_open_ioctl_locked(struct
> > drm_i915_private *dev_priv,
> > > if (IS_ERR(specific_ctx)) {
> > > ret = PTR_ERR(specific_ctx);
> > > if (ret != -EINTR)
> > > - DRM_ERROR("Failed to look up context with
> > ID %u for opening perf stream\n",
> > > + DRM_DEBUG("Failed to look up context with
> > ID %u for opening perf stream\n",
> > > ctx_handle);
> > > goto err;
> > > }
> > > @@ -1397,7 +1397,7 @@ i915_perf_open_ioctl_locked(struct
> > drm_i915_private *dev_priv,
> > > */
> > > if (!specific_ctx &&
> > > i915_perf_stream_paranoid && !capable(CAP_SYS_ADMIN)) {
> > > - DRM_ERROR("Insufficient privileges to open system-wide
> > i915 perf stream\n");
> > > + DRM_DEBUG("Insufficient privileges to open system-wide
> > i915 perf stream\n");
> > > ret = -EACCES;
> > > goto err_ctx;
> > > }
> > > @@ -1476,7 +1476,7 @@ static int read_properties_unlocked(struct
> > drm_i915_private *dev_priv,
> > > memset(props, 0, sizeof(struct perf_open_properties));
> > >
> > > if (!n_props) {
> > > - DRM_ERROR("No i915 perf properties given");
> > > + DRM_DEBUG("No i915 perf properties given\n");
> > > return -EINVAL;
> > > }
> > >
> > > @@ -1487,7 +1487,7 @@ static int read_properties_unlocked(struct
> > drm_i915_private *dev_priv,
> > > * from userspace.
> > > */
> > > if (n_props >= DRM_I915_PERF_PROP_MAX) {
> > > - DRM_ERROR("More i915 perf properties specified than
> > exist");
> > > + DRM_DEBUG("More i915 perf properties specified than
> > exist\n");
> > > return -EINVAL;
> > > }
> > >
> > > @@ -1515,26 +1515,26 @@ static int read_properties_unlocked(struct
> > drm_i915_private *dev_priv,
> > > case DRM_I915_PERF_PROP_OA_METRICS_SET:
> > > if (value == 0 ||
> > > value > dev_priv->perf.oa.n_builtin_sets) {
> > > - DRM_ERROR("Unknown OA metric set ID");
> > > + DRM_DEBUG("Unknown OA metric set ID\n");
> > > return -EINVAL;
> > > }
> > > props->metrics_set = value;
> > > break;
> > > case DRM_I915_PERF_PROP_OA_FORMAT:
> > > if (value == 0 || value >= I915_OA_FORMAT_MAX) {
> > > - DRM_ERROR("Invalid OA report format\n");
> > > + DRM_DEBUG("Invalid OA report format\n");
> > > return -EINVAL;
> > > }
> > > if (!dev_priv->perf.oa.oa_formats[value].size) {
> > > - DRM_ERROR("Invalid OA report format\n");
> > > + DRM_DEBUG("Invalid OA report format\n");
> > > return -EINVAL;
> > > }
> > > props->oa_format = value;
> > > break;
> > > case DRM_I915_PERF_PROP_OA_EXPONENT:
> > > if (value > OA_EXPONENT_MAX) {
> > > - DRM_ERROR("OA timer exponent too high (>
> > %u)\n",
> > > - OA_EXPONENT_MAX);
> > > + DRM_DEBUG("OA timer exponent too high (>
> > %u)\n",
> > > + OA_EXPONENT_MAX);
> > > return -EINVAL;
> > > }
> > >
> > > @@ -1565,7 +1565,7 @@ static int read_properties_unlocked(struct
> > drm_i915_private *dev_priv,
> > >
> > > 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",
> > > + DRM_DEBUG("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;
> > > }
> > > @@ -1575,7 +1575,7 @@ static int read_properties_unlocked(struct
> > drm_i915_private *dev_priv,
> > > break;
> > > default:
> > > MISSING_CASE(id);
> > > - DRM_ERROR("Unknown i915 perf property ID");
> > > + DRM_DEBUG("Unknown i915 perf property ID\n");
> > > return -EINVAL;
> > > }
> > >
> > > @@ -1595,7 +1595,7 @@ int i915_perf_open_ioctl(struct drm_device *dev,
> > void *data,
> > > int ret;
> > >
> > > if (!dev_priv->perf.initialized) {
> > > - DRM_ERROR("i915 perf interface not available for this
> > system");
> > > + DRM_DEBUG("i915 perf interface not available for this
> > system\n");
> > > return -ENOTSUPP;
> > > }
> > >
> > > @@ -1603,7 +1603,7 @@ int i915_perf_open_ioctl(struct drm_device *dev,
> > void *data,
> > > I915_PERF_FLAG_FD_NONBLOCK |
> > > I915_PERF_FLAG_DISABLED;
> > > if (param->flags & ~known_open_flags) {
> > > - DRM_ERROR("Unknown drm_i915_perf_open_param flag\n");
> > > + DRM_DEBUG("Unknown drm_i915_perf_open_param flag\n");
> > > return -EINVAL;
> > > }
> > >
> > > --
> > > 2.10.2
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> >
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
