On Fri, Dec 2, 2016 at 8:35 AM, Daniel Vetter <daniel@xxxxxxxx> wrote:
The above looks like a genuine hw/kernel fail, which we shouldn't putOn 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;
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.
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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
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
