<div dir="ltr"><div>Hi Sourab,<br><br></div>As Sergio Martinez has started experimenting with this in gputop and reported seeing lots of ENOSPC errors being reported when reading I had a look into this and saw a few issues with how we check that there's data available to read in command stream mode, and a I think there's a possibility of incorrectly sorting the samples sometimes...<br><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Feb 16, 2016 at 5:27 AM, <span dir="ltr"><<a href="mailto:sourab.gupta@intel.com" target="_blank">sourab.gupta@intel.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">From: Sourab Gupta <<a href="mailto:sourab.gupta@intel.com">sourab.gupta@intel.com</a>><br>
<br>
<br>
-static bool i915_oa_can_read(struct i915_perf_stream *stream)<br>
+static bool append_oa_rcs_sample(struct i915_perf_stream *stream,<br>
+ struct i915_perf_read_state *read_state,<br>
+ struct i915_perf_cs_data_node *node)<br>
+{<br>
+ struct drm_i915_private *dev_priv = stream->dev_priv;<br>
+ struct oa_sample_data data = { 0 };<br>
+ const u8 *report = dev_priv->perf.command_stream_buf.addr +<br>
+ node->offset;<br>
+ u32 sample_flags = stream->sample_flags;<br>
+ u32 report_ts;<br>
+<br>
+ /*<br>
+ * Forward the periodic OA samples which have the timestamp lower<br>
+ * than timestamp of this sample, before forwarding this sample.<br>
+ * This ensures samples read by user are order acc. to their timestamps<br>
+ */<br>
+ report_ts = *(u32 *)(report + 4);<br>
+ dev_priv->perf.oa.ops.read(stream, read_state, report_ts);<br>
+<br>
+ if (sample_flags & SAMPLE_OA_SOURCE_INFO)<br>
+ data.source = I915_PERF_OA_EVENT_SOURCE_RCS;<br>
+<br>
+ if (sample_flags & SAMPLE_CTX_ID)<br>
+ data.ctx_id = node->ctx_id;<br>
+<br>
+ if (sample_flags & SAMPLE_OA_REPORT)<br>
+ data.report = report;<br>
+<br>
+ append_oa_sample(stream, read_state, &data);<br>
+<br>
+ return true;<br>
+}<br>
+<br>
+static void oa_rcs_append_reports(struct i915_perf_stream *stream,<br>
+ struct i915_perf_read_state *read_state)<br>
+{<br>
+ struct drm_i915_private *dev_priv = stream->dev_priv;<br>
+ struct i915_perf_cs_data_node *entry, *next;<br>
+<br>
+ list_for_each_entry_safe(entry, next,<br>
+ &dev_priv->perf.node_list, link) {<br>
+ if (!i915_gem_request_completed(entry->request, true))<br>
+ break;<br>
+<br>
+ if (!append_oa_rcs_sample(stream, read_state, entry))<br>
+ break;<br>
+<br>
+ spin_lock(&dev_priv->perf.node_list_lock);<br>
+ list_del(&entry->link);<br>
+ spin_unlock(&dev_priv->perf.node_list_lock);<br>
+<br>
+ i915_gem_request_unreference__unlocked(entry->request);<br>
+ kfree(entry);<br>
+ }<br>
+<br>
+ /* Flush any remaining periodic reports */<br>
+ dev_priv->perf.oa.ops.read(stream, read_state, U32_MAX);<br></blockquote><div> </div><div>I don't think we can flush all remaining periodic reports here - at least not if we have any in-flight MI_RPC commands - in case the next request to complete might have reports with earlier timestamps than some of these periodic reports.<br><br></div><div>Even if we have periodic reports available I think we need to throttle forwarding them based on the command stream requests completing.<br><br></div><div>This is something that userspace should understand when it explicitly decides to use command stream mode in conjunction with periodic sampling.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+}<br>
+<br>
+static bool command_stream_buf_is_empty(struct i915_perf_stream *stream)<br>
{<br>
struct drm_i915_private *dev_priv = stream->dev_priv;<br>
<br>
- return !dev_priv->perf.oa.ops.oa_buffer_is_empty(dev_priv);<br>
+ if (stream->cs_mode)<br>
+ return list_empty(&dev_priv->perf.node_list);<br>
+ else<br>
+ return true;<br>
}<br></blockquote><div><br></div><div>I think this list_empty() check needs a lock around it, as it's called from stream_have_data__unlocked().<br><br></div><div>Currently only checking list_empty() can lead to some false positives that may confuse the current i915_perf_read() (the false positives come from not checking that at least one entry->request has also completed).<br><br>False positives here will affect the error reporting in i915_perf_read_locked(). The first thing i915_perf_read_locked() does is check for available data so it can return -EAGAIN for non-blocking file descriptors or block, waiting for data, for blocking fds. Once we believe there is data then if stream->read() returns zero then that's translated to a -ENOSPC error because we 'know' there's data but weren't able to copy anything to the user's buffer because it wasn't large enough to hold a complete record.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
-static int i915_oa_wait_unlocked(struct i915_perf_stream *stream)<br>
+static bool stream_have_data__unlocked(struct i915_perf_stream *stream)<br>
{<br>
struct drm_i915_private *dev_priv = stream->dev_priv;<br>
<br>
@@ -397,8 +739,29 @@ static int i915_oa_wait_unlocked(struct i915_perf_stream *stream)<br>
* can't be destroyed until completion (such as a read()) that ensures<br>
* the device + OA buffer can't disappear<br>
*/<br>
+ return !(dev_priv->perf.oa.ops.oa_buffer_is_empty(dev_priv) &&<br>
+ command_stream_buf_is_empty(stream));<br>
+}<br></blockquote><div><br></div><div>In command stream mode, due to how we need to merge sort reports I think we need a more involved set of checks. We need to check that at least one entry->request in the node_list has completed, or else if the node_list is empty then we can check if !oa_buffer_is_empty().<br><br></div><div>There's going to be a thorny race to consider though: if this code ends up reporting that there is data to read because the node_list is empty and there are periodic samples, but by the time we call into stream->read() there's a new node_list entry that's not completed that will end up returning 0 which could again result in an incorrect ENOSPC error.<br><br>We should consider changing stream->read() so it can return -ENOSPC itself if appropriate and when it returns 0 we would instead either return -EAGAIN or loop back to waiting for blocking fds.<br></div><div><br></div><div>B.t.w for reference the stream->read() interface is a little different in the series I sent out recently for Haswell vs the gen8+ oa-next branch (which I haven't rebased yet) because I had to make some last minute changes to consider -EFAULT errors if there was a copy_to_user() error while reading. If you look at this issue, it could be good to also compare with these related changes I made recently.<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
+static bool i915_oa_can_read(struct i915_perf_stream *stream)<br>
+{<br>
+<br>
+ return stream_have_data__unlocked(stream);<br>
+}<br>
<span class=""><font color="#888888"><br>
</font></span></blockquote></div><br></div><div class="gmail_extra">Kind regards,<br></div><div class="gmail_extra">- Robert<br></div></div></div></div>