diff mbox

drm/i915/perf: use DRM_INFO for userspace issues

Message ID 20161129165759.27448-1-robert@sixbynine.org (mailing list archive)
State New, archived
Headers show

Commit Message

Robert Bragg Nov. 29, 2016, 4:57 p.m. UTC
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.

DRM_INFO was used over DRM_DEBUG since it's proven convenient while
working on gputop and mesa to have a message on the console for a
malformed config without needing to explicitly enable drm debug messages
which can be very verbose.

One message relating to seeing a spurious zeroed report was changed to
use DRM_WARN 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@sixbynine.org:
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_perf.c | 46 ++++++++++++++++++++--------------------
 1 file changed, 23 insertions(+), 23 deletions(-)

Comments

Chris Wilson Nov. 29, 2016, 8:42 p.m. UTC | #1
On Tue, Nov 29, 2016 at 04:57:59PM +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.
> 
> DRM_INFO was used over DRM_DEBUG since it's proven convenient while
> working on gputop and mesa to have a message on the console for a
> malformed config without needing to explicitly enable drm debug messages
> which can be very verbose.

The opposite. If they are only there for debugging userspace, make them
debug.
 
> One message relating to seeing a spurious zeroed report was changed to
> use DRM_WARN 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.

If it is expected and handled (i.e. has no impact on the driver), it is
just a notice.
-Chris
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index 9551282..68b7c27 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_WARN("Skipping spurious, invalid OA report\n");
 			continue;
 		}
 
@@ -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_INFO("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_INFO("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_INFO("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_INFO("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_INFO("OA unit already in use\n");
 		return -EBUSY;
 	}
 
 	if (!props->metrics_set) {
-		DRM_ERROR("OA metric set not specified\n");
+		DRM_INFO("OA metric set not specified\n");
 		return -EINVAL;
 	}
 
 	if (!props->oa_format) {
-		DRM_ERROR("OA report format not specified\n");
+		DRM_INFO("OA report format not specified\n");
 		return -EINVAL;
 	}
 
@@ -1384,8 +1384,8 @@  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",
-					  ctx_handle);
+				DRM_INFO("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_INFO("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_INFO("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_INFO("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_INFO("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_INFO("Invalid OA report format\n");
 				return -EINVAL;
 			}
 			if (!dev_priv->perf.oa.oa_formats[value].size) {
-				DRM_ERROR("Invalid OA report format\n");
+				DRM_INFO("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_INFO("OA timer exponent too high (> %u)\n",
+					 OA_EXPONENT_MAX);
 				return -EINVAL;
 			}
 
@@ -1565,8 +1565,8 @@  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",
-					  i915_oa_max_sample_rate);
+				DRM_INFO("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_INFO("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_INFO("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_INFO("Unknown drm_i915_perf_open_param flag\n");
 		return -EINVAL;
 	}