* [Intel-gfx] [PATCH 0/3] drm/i915/perf: add OA interrupt support
@ 2020-03-19 22:52 Umesh Nerlige Ramappa
2020-03-19 22:52 ` [Intel-gfx] [PATCH 1/3] drm/i915/perf: rework aging tail workaround Umesh Nerlige Ramappa
` (5 more replies)
0 siblings, 6 replies; 15+ messages in thread
From: Umesh Nerlige Ramappa @ 2020-03-19 22:52 UTC (permalink / raw)
To: intel-gfx, Lionel G Landwerlin, Ashutosh Dixit
Hi all,
This is a revival of an earlier patch series submitted by Lionel
Landwerlin - https://patchwork.freedesktop.org/series/54280/
The patches enable interrupt support for the perf OA unit in
i915, further details can be found in the orignal series linked
above.
v2: (Umesh)
- This series will split into 2. This revision will only support the addition of
poll delay parameter to perf OA. If needed a future revision will incorporate
interrupt support.
Regards,
Umesh
Lionel Landwerlin (3):
drm/i915/perf: rework aging tail workaround
drm/i915/perf: move pollin setup to non hw specific code
drm/i915/perf: add new open param to configure polling of OA buffer
drivers/gpu/drm/i915/i915_perf.c | 243 +++++++++++--------------
drivers/gpu/drm/i915/i915_perf_types.h | 35 ++--
include/uapi/drm/i915_drm.h | 13 ++
3 files changed, 143 insertions(+), 148 deletions(-)
--
2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread* [Intel-gfx] [PATCH 1/3] drm/i915/perf: rework aging tail workaround 2020-03-19 22:52 [Intel-gfx] [PATCH 0/3] drm/i915/perf: add OA interrupt support Umesh Nerlige Ramappa @ 2020-03-19 22:52 ` Umesh Nerlige Ramappa 2020-03-21 23:26 ` Dixit, Ashutosh 2020-03-19 22:52 ` [Intel-gfx] [PATCH 2/3] drm/i915/perf: move pollin setup to non hw specific code Umesh Nerlige Ramappa ` (4 subsequent siblings) 5 siblings, 1 reply; 15+ messages in thread From: Umesh Nerlige Ramappa @ 2020-03-19 22:52 UTC (permalink / raw) To: intel-gfx, Lionel G Landwerlin, Ashutosh Dixit From: Lionel Landwerlin <lionel.g.landwerlin@intel.com> We're about to introduce an options to open the perf stream, giving the user ability to configure how often it wants the kernel to poll the OA registers for available data. Right now the workaround against the OA tail pointer race condition requires at least twice the internal kernel polling timer to make any data available. This changes introduce checks on the OA data written into the circular buffer to make as much data as possible available on the first iteration of the polling timer. v2: Use OA_TAKEN macro without the gtt_offset (Lionel) v3: (Umesh) - Rebase - Change report to report32 from below review https://patchwork.freedesktop.org/patch/330704/?series=66697&rev=1 v4: (Ashutosh, Lionel) - Fix checkpatch errors - Fix aging_timestamp initialization - Check for only one valid landed report - Fix check for unlanded report Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> --- drivers/gpu/drm/i915/i915_perf.c | 197 ++++++++++--------------- drivers/gpu/drm/i915/i915_perf_types.h | 29 ++-- 2 files changed, 96 insertions(+), 130 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c index 3222f6cd8255..c1429d3acaf9 100644 --- a/drivers/gpu/drm/i915/i915_perf.c +++ b/drivers/gpu/drm/i915/i915_perf.c @@ -223,26 +223,17 @@ * * Although this can be observed explicitly while copying reports to userspace * by checking for a zeroed report-id field in tail reports, we want to account - * for this earlier, as part of the oa_buffer_check to avoid lots of redundant - * read() attempts. - * - * In effect we define a tail pointer for reading that lags the real tail - * pointer by at least %OA_TAIL_MARGIN_NSEC nanoseconds, which gives enough - * time for the corresponding reports to become visible to the CPU. - * - * To manage this we actually track two tail pointers: - * 1) An 'aging' tail with an associated timestamp that is tracked until we - * can trust the corresponding data is visible to the CPU; at which point - * it is considered 'aged'. - * 2) An 'aged' tail that can be used for read()ing. - * - * The two separate pointers let us decouple read()s from tail pointer aging. - * - * The tail pointers are checked and updated at a limited rate within a hrtimer - * callback (the same callback that is used for delivering EPOLLIN events) - * - * Initially the tails are marked invalid with %INVALID_TAIL_PTR which - * indicates that an updated tail pointer is needed. + * for this earlier, as part of the oa_buffer_check_unlocked to avoid lots of + * redundant read() attempts. + * + * We workaround this issue in oa_buffer_check_unlocked() by reading the reports + * in the OA buffer, starting from the tail reported by the HW until we find 2 + * consecutive reports with their first 2 dwords of not at 0. Those dwords are + * also set to 0 once read and the whole buffer is cleared upon OA buffer + * initialization. The first dword is the reason for this report while the + * second is the timestamp, making the chances of having those 2 fields at 0 + * fairly unlikely. A more detailed explanation is available in + * oa_buffer_check_unlocked(). * * Most of the implementation details for this workaround are in * oa_buffer_check_unlocked() and _append_oa_reports() @@ -465,10 +456,10 @@ static u32 gen7_oa_hw_tail_read(struct i915_perf_stream *stream) */ static bool oa_buffer_check_unlocked(struct i915_perf_stream *stream) { + u32 gtt_offset = i915_ggtt_offset(stream->oa_buffer.vma); int report_size = stream->oa_buffer.format_size; unsigned long flags; - unsigned int aged_idx; - u32 head, hw_tail, aged_tail, aging_tail; + u32 hw_tail; u64 now; /* We have to consider the (unlikely) possibility that read() errors @@ -477,16 +468,6 @@ static bool oa_buffer_check_unlocked(struct i915_perf_stream *stream) */ spin_lock_irqsave(&stream->oa_buffer.ptr_lock, flags); - /* NB: The head we observe here might effectively be a little out of - * date (between head and tails[aged_idx].offset if there is currently - * a read() in progress. - */ - head = stream->oa_buffer.head; - - aged_idx = stream->oa_buffer.aged_tail_idx; - aged_tail = stream->oa_buffer.tails[aged_idx].offset; - aging_tail = stream->oa_buffer.tails[!aged_idx].offset; - hw_tail = stream->perf->ops.oa_hw_tail_read(stream); /* The tail pointer increases in 64 byte increments, @@ -496,64 +477,64 @@ static bool oa_buffer_check_unlocked(struct i915_perf_stream *stream) now = ktime_get_mono_fast_ns(); - /* Update the aged tail - * - * Flip the tail pointer available for read()s once the aging tail is - * old enough to trust that the corresponding data will be visible to - * the CPU... - * - * Do this before updating the aging pointer in case we may be able to - * immediately start aging a new pointer too (if new data has become - * available) without needing to wait for a later hrtimer callback. - */ - if (aging_tail != INVALID_TAIL_PTR && - ((now - stream->oa_buffer.aging_timestamp) > - OA_TAIL_MARGIN_NSEC)) { - - aged_idx ^= 1; - stream->oa_buffer.aged_tail_idx = aged_idx; + if (hw_tail == stream->oa_buffer.aging_tail && + (now - stream->oa_buffer.aging_timestamp) > OA_TAIL_MARGIN_NSEC) { + /* If the HW tail hasn't move since the last check and the HW + * tail has been aging for long enough, declare it the new + * tail. + */ + stream->oa_buffer.tail = stream->oa_buffer.aging_tail; + } else { + u32 head, tail; - aged_tail = aging_tail; + /* NB: The head we observe here might effectively be a little + * out of date. If a read() is in progress, the head could be + * anywhere between this head and stream->oa_buffer.tail. + */ + head = stream->oa_buffer.head - gtt_offset; - /* Mark that we need a new pointer to start aging... */ - stream->oa_buffer.tails[!aged_idx].offset = INVALID_TAIL_PTR; - aging_tail = INVALID_TAIL_PTR; - } + hw_tail -= gtt_offset; + tail = hw_tail; - /* Update the aging tail - * - * We throttle aging tail updates until we have a new tail that - * represents >= one report more data than is already available for - * reading. This ensures there will be enough data for a successful - * read once this new pointer has aged and ensures we will give the new - * pointer time to age. - */ - if (aging_tail == INVALID_TAIL_PTR && - (aged_tail == INVALID_TAIL_PTR || - OA_TAKEN(hw_tail, aged_tail) >= report_size)) { - struct i915_vma *vma = stream->oa_buffer.vma; - u32 gtt_offset = i915_ggtt_offset(vma); - - /* Be paranoid and do a bounds check on the pointer read back - * from hardware, just in case some spurious hardware condition - * could put the tail out of bounds... + /* Walk the stream backward until we find a report with dword 0 + * & 1 not at 0. Since the circular buffer pointers progress by + * increments of 64 bytes and that reports can be up to 256 + * bytes long, we can't tell whether a report has fully landed + * in memory before the first 2 dwords of the following report + * have effectively landed. + * + * This is assuming that the writes of the OA unit land in + * memory in the order they were written to. + * If not : (╯°□°）╯︵ ┻━┻ */ - if (hw_tail >= gtt_offset && - hw_tail < (gtt_offset + OA_BUFFER_SIZE)) { - stream->oa_buffer.tails[!aged_idx].offset = - aging_tail = hw_tail; - stream->oa_buffer.aging_timestamp = now; - } else { - drm_err(&stream->perf->i915->drm, - "Ignoring spurious out of range OA buffer tail pointer = %x\n", - hw_tail); + while (OA_TAKEN(tail, head) >= report_size) { + u32 previous_tail = (tail - report_size) & (OA_BUFFER_SIZE - 1); + u32 *report32 = (void *)(stream->oa_buffer.vaddr + previous_tail); + + /* Head of the report indicated by the HW tail register has + * indeed landed into memory. + */ + if (report32[0] != 0 || report32[1] != 0) + break; + + tail = previous_tail; } + + if (((tail - hw_tail) & (OA_BUFFER_SIZE - 1)) > report_size && + __ratelimit(&stream->perf->tail_pointer_race)) + DRM_NOTE("unlanded report(s) head=0x%x " + "tail=0x%x hw_tail=0x%x\n", + head, tail, hw_tail); + + stream->oa_buffer.tail = gtt_offset + tail; + stream->oa_buffer.aging_tail = gtt_offset + hw_tail; + stream->oa_buffer.aging_timestamp = now; } spin_unlock_irqrestore(&stream->oa_buffer.ptr_lock, flags); - return aged_tail == INVALID_TAIL_PTR ? - false : OA_TAKEN(aged_tail, head) >= report_size; + return OA_TAKEN(stream->oa_buffer.tail - gtt_offset, + stream->oa_buffer.head - gtt_offset) >= report_size; } /** @@ -671,7 +652,6 @@ static int gen8_append_oa_reports(struct i915_perf_stream *stream, u32 mask = (OA_BUFFER_SIZE - 1); size_t start_offset = *offset; unsigned long flags; - unsigned int aged_tail_idx; u32 head, tail; u32 taken; int ret = 0; @@ -682,18 +662,10 @@ static int gen8_append_oa_reports(struct i915_perf_stream *stream, spin_lock_irqsave(&stream->oa_buffer.ptr_lock, flags); head = stream->oa_buffer.head; - aged_tail_idx = stream->oa_buffer.aged_tail_idx; - tail = stream->oa_buffer.tails[aged_tail_idx].offset; + tail = stream->oa_buffer.tail; spin_unlock_irqrestore(&stream->oa_buffer.ptr_lock, flags); - /* - * An invalid tail pointer here means we're still waiting for the poll - * hrtimer callback to give us a pointer - */ - if (tail == INVALID_TAIL_PTR) - return -EAGAIN; - /* * NB: oa_buffer.head/tail include the gtt_offset which we don't want * while indexing relative to oa_buf_base. @@ -827,13 +799,11 @@ static int gen8_append_oa_reports(struct i915_perf_stream *stream, } /* - * The above reason field sanity check is based on - * the assumption that the OA buffer is initially - * zeroed and we reset the field after copying so the - * check is still meaningful once old reports start - * being overwritten. + * Clear out the first 2 dword as a mean to detect unlanded + * reports. */ report32[0] = 0; + report32[1] = 0; } if (start_offset != *offset) { @@ -974,7 +944,6 @@ static int gen7_append_oa_reports(struct i915_perf_stream *stream, u32 mask = (OA_BUFFER_SIZE - 1); size_t start_offset = *offset; unsigned long flags; - unsigned int aged_tail_idx; u32 head, tail; u32 taken; int ret = 0; @@ -985,17 +954,10 @@ static int gen7_append_oa_reports(struct i915_perf_stream *stream, spin_lock_irqsave(&stream->oa_buffer.ptr_lock, flags); head = stream->oa_buffer.head; - aged_tail_idx = stream->oa_buffer.aged_tail_idx; - tail = stream->oa_buffer.tails[aged_tail_idx].offset; + tail = stream->oa_buffer.tail; spin_unlock_irqrestore(&stream->oa_buffer.ptr_lock, flags); - /* An invalid tail pointer here means we're still waiting for the poll - * hrtimer callback to give us a pointer - */ - if (tail == INVALID_TAIL_PTR) - return -EAGAIN; - /* NB: oa_buffer.head/tail include the gtt_offset which we don't want * while indexing relative to oa_buf_base. */ @@ -1053,13 +1015,11 @@ static int gen7_append_oa_reports(struct i915_perf_stream *stream, if (ret) break; - /* The above report-id field sanity check is based on - * the assumption that the OA buffer is initially - * zeroed and we reset the field after copying so the - * check is still meaningful once old reports start - * being overwritten. + /* Clear out the first 2 dwords as a mean to detect unlanded + * reports. */ report32[0] = 0; + report32[1] = 0; } if (start_offset != *offset) { @@ -1438,8 +1398,8 @@ static void gen7_init_oa_buffer(struct i915_perf_stream *stream) gtt_offset | OABUFFER_SIZE_16M); /* Mark that we need updated tail pointers to read from... */ - stream->oa_buffer.tails[0].offset = INVALID_TAIL_PTR; - stream->oa_buffer.tails[1].offset = INVALID_TAIL_PTR; + stream->oa_buffer.aging_tail = INVALID_TAIL_PTR; + stream->oa_buffer.tail = gtt_offset; spin_unlock_irqrestore(&stream->oa_buffer.ptr_lock, flags); @@ -1492,8 +1452,8 @@ static void gen8_init_oa_buffer(struct i915_perf_stream *stream) intel_uncore_write(uncore, GEN8_OATAILPTR, gtt_offset & GEN8_OATAILPTR_MASK); /* Mark that we need updated tail pointers to read from... */ - stream->oa_buffer.tails[0].offset = INVALID_TAIL_PTR; - stream->oa_buffer.tails[1].offset = INVALID_TAIL_PTR; + stream->oa_buffer.aging_tail = INVALID_TAIL_PTR; + stream->oa_buffer.tail = gtt_offset; /* * Reset state used to recognise context switches, affecting which @@ -1548,8 +1508,8 @@ static void gen12_init_oa_buffer(struct i915_perf_stream *stream) gtt_offset & GEN12_OAG_OATAILPTR_MASK); /* Mark that we need updated tail pointers to read from... */ - stream->oa_buffer.tails[0].offset = INVALID_TAIL_PTR; - stream->oa_buffer.tails[1].offset = INVALID_TAIL_PTR; + stream->oa_buffer.aging_tail = INVALID_TAIL_PTR; + stream->oa_buffer.tail = gtt_offset; /* * Reset state used to recognise context switches, affecting which @@ -4398,6 +4358,11 @@ void i915_perf_init(struct drm_i915_private *i915) ratelimit_set_flags(&perf->spurious_report_rs, RATELIMIT_MSG_ON_RELEASE); + ratelimit_state_init(&perf->tail_pointer_race, + 5 * HZ, 10); + ratelimit_set_flags(&perf->tail_pointer_race, + RATELIMIT_MSG_ON_RELEASE); + atomic64_set(&perf->noa_programming_delay, 500 * 1000 /* 500us */); diff --git a/drivers/gpu/drm/i915/i915_perf_types.h b/drivers/gpu/drm/i915/i915_perf_types.h index 32289cbda648..6dbb5ee4c55c 100644 --- a/drivers/gpu/drm/i915/i915_perf_types.h +++ b/drivers/gpu/drm/i915/i915_perf_types.h @@ -273,21 +273,10 @@ struct i915_perf_stream { spinlock_t ptr_lock; /** - * @tails: One 'aging' tail pointer and one 'aged' tail pointer ready to - * used for reading. - * - * Initial values of 0xffffffff are invalid and imply that an - * update is required (and should be ignored by an attempted - * read) + * @aging_tail: The last HW tail reported by HW. The data + * might not have made it to memory yet though. */ - struct { - u32 offset; - } tails[2]; - - /** - * @aged_tail_idx: Index for the aged tail ready to read() data up to. - */ - unsigned int aged_tail_idx; + u32 aging_tail; /** * @aging_timestamp: A monotonic timestamp for when the current aging tail pointer @@ -303,6 +292,12 @@ struct i915_perf_stream { * OA buffer data to userspace. */ u32 head; + + /** + * @tail: The last tail verified tail that can be read by + * userspace. + */ + u32 tail; } oa_buffer; /** @@ -420,6 +415,12 @@ struct i915_perf { */ struct ratelimit_state spurious_report_rs; + /** + * For rate limiting any notifications of tail pointer + * race. + */ + struct ratelimit_state tail_pointer_race; + u32 gen7_latched_oastatus1; u32 ctx_oactxctrl_offset; u32 ctx_flexeu0_offset; -- 2.20.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [Intel-gfx] [PATCH 1/3] drm/i915/perf: rework aging tail workaround 2020-03-19 22:52 ` [Intel-gfx] [PATCH 1/3] drm/i915/perf: rework aging tail workaround Umesh Nerlige Ramappa @ 2020-03-21 23:26 ` Dixit, Ashutosh 2020-03-22 4:44 ` Dixit, Ashutosh 2020-03-24 3:15 ` Umesh Nerlige Ramappa 0 siblings, 2 replies; 15+ messages in thread From: Dixit, Ashutosh @ 2020-03-21 23:26 UTC (permalink / raw) To: Umesh Nerlige Ramappa; +Cc: intel-gfx On Thu, 19 Mar 2020 15:52:01 -0700, Umesh Nerlige Ramappa wrote: > > From: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > > We're about to introduce an options to open the perf stream, giving > the user ability to configure how often it wants the kernel to poll > the OA registers for available data. > > Right now the workaround against the OA tail pointer race condition > requires at least twice the internal kernel polling timer to make any > data available. > > This changes introduce checks on the OA data written into the circular > buffer to make as much data as possible available on the first > iteration of the polling timer. /snip/ > diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c > index 3222f6cd8255..c1429d3acaf9 100644 > --- a/drivers/gpu/drm/i915/i915_perf.c > +++ b/drivers/gpu/drm/i915/i915_perf.c > @@ -223,26 +223,17 @@ > * > * Although this can be observed explicitly while copying reports to userspace > * by checking for a zeroed report-id field in tail reports, we want to account > - * for this earlier, as part of the oa_buffer_check to avoid lots of redundant > - * read() attempts. > - * > - * In effect we define a tail pointer for reading that lags the real tail > - * pointer by at least %OA_TAIL_MARGIN_NSEC nanoseconds, which gives enough > - * time for the corresponding reports to become visible to the CPU. > - * > - * To manage this we actually track two tail pointers: > - * 1) An 'aging' tail with an associated timestamp that is tracked until we > - * can trust the corresponding data is visible to the CPU; at which point > - * it is considered 'aged'. > - * 2) An 'aged' tail that can be used for read()ing. > - * > - * The two separate pointers let us decouple read()s from tail pointer aging. > - * > - * The tail pointers are checked and updated at a limited rate within a hrtimer > - * callback (the same callback that is used for delivering EPOLLIN events) > - * > - * Initially the tails are marked invalid with %INVALID_TAIL_PTR which > - * indicates that an updated tail pointer is needed. > + * for this earlier, as part of the oa_buffer_check_unlocked to avoid lots of > + * redundant read() attempts. > + * > + * We workaround this issue in oa_buffer_check_unlocked() by reading the reports > + * in the OA buffer, starting from the tail reported by the HW until we find 2 > + * consecutive reports with their first 2 dwords of not at 0. Those dwords are until we find a report with its first 2 dwords not 0 meaning its previous report is completely in memory and ready to be read. > + * also set to 0 once read and the whole buffer is cleared upon OA buffer > + * initialization. The first dword is the reason for this report while the > + * second is the timestamp, making the chances of having those 2 fields at 0 > + * fairly unlikely. A more detailed explanation is available in > + * oa_buffer_check_unlocked(). > @@ -477,16 +468,6 @@ static bool oa_buffer_check_unlocked(struct i915_perf_stream *stream) > */ > spin_lock_irqsave(&stream->oa_buffer.ptr_lock, flags); > > hw_tail = stream->perf->ops.oa_hw_tail_read(stream); > > hw_tail &= ~(report_size - 1); > > @@ -496,64 +477,64 @@ static bool oa_buffer_check_unlocked(struct i915_perf_stream *stream) > > now = ktime_get_mono_fast_ns(); > > + if (hw_tail == stream->oa_buffer.aging_tail && > + (now - stream->oa_buffer.aging_timestamp) > OA_TAIL_MARGIN_NSEC) { > + /* If the HW tail hasn't move since the last check and the HW > + * tail has been aging for long enough, declare it the new > + * tail. > + */ > + stream->oa_buffer.tail = stream->oa_buffer.aging_tail; > + } else { > + u32 head, tail; > > + /* NB: The head we observe here might effectively be a little > + * out of date. If a read() is in progress, the head could be > + * anywhere between this head and stream->oa_buffer.tail. > + */ > + head = stream->oa_buffer.head - gtt_offset; > > + hw_tail -= gtt_offset; > + tail = hw_tail; > > + /* Walk the stream backward until we find a report with dword 0 > + * & 1 not at 0. Since the circular buffer pointers progress by > + * increments of 64 bytes and that reports can be up to 256 > + * bytes long, we can't tell whether a report has fully landed > + * in memory before the first 2 dwords of the following report > + * have effectively landed. > + * > + * This is assuming that the writes of the OA unit land in > + * memory in the order they were written to. > + * If not : (╯°□°）╯︵ ┻━┻ > */ > - if (hw_tail >= gtt_offset && > - hw_tail < (gtt_offset + OA_BUFFER_SIZE)) { > - stream->oa_buffer.tails[!aged_idx].offset = > - aging_tail = hw_tail; > - stream->oa_buffer.aging_timestamp = now; > - } else { > - drm_err(&stream->perf->i915->drm, > - "Ignoring spurious out of range OA buffer tail pointer = %x\n", > - hw_tail); > + while (OA_TAKEN(tail, head) >= report_size) { > + u32 previous_tail = (tail - report_size) & (OA_BUFFER_SIZE - 1); > + u32 *report32 = (void *)(stream->oa_buffer.vaddr + previous_tail); Sorry, this is wrong. This should just be: tail = (tail - report_size) & (OA_BUFFER_SIZE - 1); report32 = (void *)(stream->oa_buffer.vaddr + tail); Otherwise when we break out of the loop below tail is still set one report_size ahead. previous_tail is not needed. (In the previous version of the patch this used to work out correctly). > + > + /* Head of the report indicated by the HW tail register has > + * indeed landed into memory. > + */ > + if (report32[0] != 0 || report32[1] != 0) > + break; > + > + tail = previous_tail; > } > + > + if (((tail - hw_tail) & (OA_BUFFER_SIZE - 1)) > report_size && nit: OA_TAKEN(hw_tail, tail) > report_size? > + __ratelimit(&stream->perf->tail_pointer_race)) > + DRM_NOTE("unlanded report(s) head=0x%x " > + "tail=0x%x hw_tail=0x%x\n", > + head, tail, hw_tail); > + > + stream->oa_buffer.tail = gtt_offset + tail; > + stream->oa_buffer.aging_tail = gtt_offset + hw_tail; > + stream->oa_buffer.aging_timestamp = now; > } > > spin_unlock_irqrestore(&stream->oa_buffer.ptr_lock, flags); > > - return aged_tail == INVALID_TAIL_PTR ? > - false : OA_TAKEN(aged_tail, head) >= report_size; > + return OA_TAKEN(stream->oa_buffer.tail - gtt_offset, > + stream->oa_buffer.head - gtt_offset) >= report_size; > } > @@ -303,6 +292,12 @@ struct i915_perf_stream { > * OA buffer data to userspace. > */ > u32 head; > + > + /** > + * @tail: The last tail verified tail that can be read by The last verified tail _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Intel-gfx] [PATCH 1/3] drm/i915/perf: rework aging tail workaround 2020-03-21 23:26 ` Dixit, Ashutosh @ 2020-03-22 4:44 ` Dixit, Ashutosh 2020-03-22 19:47 ` Dixit, Ashutosh 2020-03-24 3:17 ` Umesh Nerlige Ramappa 2020-03-24 3:15 ` Umesh Nerlige Ramappa 1 sibling, 2 replies; 15+ messages in thread From: Dixit, Ashutosh @ 2020-03-22 4:44 UTC (permalink / raw) To: Umesh Nerlige Ramappa; +Cc: intel-gfx On Sat, 21 Mar 2020 16:26:42 -0700, Dixit, Ashutosh wrote: > > On Thu, 19 Mar 2020 15:52:01 -0700, Umesh Nerlige Ramappa wrote: > > > > From: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > > > @@ -477,16 +468,6 @@ static bool oa_buffer_check_unlocked(struct i915_perf_stream *stream) > > */ > > spin_lock_irqsave(&stream->oa_buffer.ptr_lock, flags); > > > > hw_tail = stream->perf->ops.oa_hw_tail_read(stream); > > > > hw_tail &= ~(report_size - 1); > > > > @@ -496,64 +477,64 @@ static bool oa_buffer_check_unlocked(struct i915_perf_stream *stream) > > > > now = ktime_get_mono_fast_ns(); > > > > + if (hw_tail == stream->oa_buffer.aging_tail && > > + (now - stream->oa_buffer.aging_timestamp) > OA_TAIL_MARGIN_NSEC) { > > + /* If the HW tail hasn't move since the last check and the HW > > + * tail has been aging for long enough, declare it the new > > + * tail. > > + */ > > + stream->oa_buffer.tail = stream->oa_buffer.aging_tail; > > + } else { > > + u32 head, tail; > > > > + /* NB: The head we observe here might effectively be a little > > + * out of date. If a read() is in progress, the head could be > > + * anywhere between this head and stream->oa_buffer.tail. > > + */ > > + head = stream->oa_buffer.head - gtt_offset; > > > > + hw_tail -= gtt_offset; > > + tail = hw_tail; > > > > + /* Walk the stream backward until we find a report with dword 0 > > + * & 1 not at 0. Since the circular buffer pointers progress by > > + * increments of 64 bytes and that reports can be up to 256 > > + * bytes long, we can't tell whether a report has fully landed > > + * in memory before the first 2 dwords of the following report > > + * have effectively landed. > > + * > > + * This is assuming that the writes of the OA unit land in > > + * memory in the order they were written to. > > + * If not : (╯°□°）╯︵ ┻━┻ > > */ > > + while (OA_TAKEN(tail, head) >= report_size) { > > + u32 previous_tail = (tail - report_size) & (OA_BUFFER_SIZE - 1); > > + u32 *report32 = (void *)(stream->oa_buffer.vaddr + previous_tail); > > Sorry, this is wrong. This should just be: > > tail = (tail - report_size) & (OA_BUFFER_SIZE - 1); > report32 = (void *)(stream->oa_buffer.vaddr + tail); > > Otherwise when we break out of the loop below tail is still set one > report_size ahead. previous_tail is not needed. (In the previous version of > the patch this used to work out correctly). > > > + > > + /* Head of the report indicated by the HW tail register has > > + * indeed landed into memory. > > + */ > > + if (report32[0] != 0 || report32[1] != 0) > > + break; > > + > > + tail = previous_tail; > > } Actually a couple of further improvements to the loop above are possible. First there is no reason to start at previous_tail, we can just start at the aligned hw_tail itself. Therefore the loop becomes: while (OA_TAKEN(tail, head) >= report_size) { u32 *report32 = (void *)(stream->oa_buffer.vaddr + tail); if (report32[0] != 0 || report32[1] != 0) break; tail = (tail - report_size) & (OA_BUFFER_SIZE - 1); } Further, there is no reason to go back to the head but only to the old tail. Therefore: head = stream->oa_buffer.head - gtt_offset; old_tail = stream->oa_buffer.tail - gtt_offset; hw_tail -= gtt_offset; tail = hw_tail; while (OA_TAKEN(tail, old_tail) >= report_size) { u32 *report32 = (void *)(stream->oa_buffer.vaddr + tail); if (report32[0] != 0 || report32[1] != 0) break; tail = (tail - report_size) & (OA_BUFFER_SIZE - 1); } Please review and see if these two improvements are possible. Thanks! _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Intel-gfx] [PATCH 1/3] drm/i915/perf: rework aging tail workaround 2020-03-22 4:44 ` Dixit, Ashutosh @ 2020-03-22 19:47 ` Dixit, Ashutosh 2020-03-24 3:17 ` Umesh Nerlige Ramappa 1 sibling, 0 replies; 15+ messages in thread From: Dixit, Ashutosh @ 2020-03-22 19:47 UTC (permalink / raw) To: Umesh Nerlige Ramappa, Lionel G Landwerlin; +Cc: intel-gfx On Sat, 21 Mar 2020 21:44:43 -0700, Dixit, Ashutosh wrote: > > Actually a couple of further improvements to the loop above are > possible. First there is no reason to start at previous_tail, we can just > start at the aligned hw_tail itself. Unless we deliberately want to wait and delay declaring the data as valid, i.e. we are really not sure what is happening and it seems "safer" to wait for an extra report. _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Intel-gfx] [PATCH 1/3] drm/i915/perf: rework aging tail workaround 2020-03-22 4:44 ` Dixit, Ashutosh 2020-03-22 19:47 ` Dixit, Ashutosh @ 2020-03-24 3:17 ` Umesh Nerlige Ramappa 1 sibling, 0 replies; 15+ messages in thread From: Umesh Nerlige Ramappa @ 2020-03-24 3:17 UTC (permalink / raw) To: Dixit, Ashutosh; +Cc: intel-gfx On Sat, Mar 21, 2020 at 09:44:43PM -0700, Dixit, Ashutosh wrote: >On Sat, 21 Mar 2020 16:26:42 -0700, Dixit, Ashutosh wrote: >> >> On Thu, 19 Mar 2020 15:52:01 -0700, Umesh Nerlige Ramappa wrote: >> > >> > From: Lionel Landwerlin <lionel.g.landwerlin@intel.com> >> >> > @@ -477,16 +468,6 @@ static bool oa_buffer_check_unlocked(struct i915_perf_stream *stream) >> > */ >> > spin_lock_irqsave(&stream->oa_buffer.ptr_lock, flags); >> > >> > hw_tail = stream->perf->ops.oa_hw_tail_read(stream); >> > >> > hw_tail &= ~(report_size - 1); >> > >> > @@ -496,64 +477,64 @@ static bool oa_buffer_check_unlocked(struct i915_perf_stream *stream) >> > >> > now = ktime_get_mono_fast_ns(); >> > >> > + if (hw_tail == stream->oa_buffer.aging_tail && >> > + (now - stream->oa_buffer.aging_timestamp) > OA_TAIL_MARGIN_NSEC) { >> > + /* If the HW tail hasn't move since the last check and the HW >> > + * tail has been aging for long enough, declare it the new >> > + * tail. >> > + */ >> > + stream->oa_buffer.tail = stream->oa_buffer.aging_tail; >> > + } else { >> > + u32 head, tail; >> > >> > + /* NB: The head we observe here might effectively be a little >> > + * out of date. If a read() is in progress, the head could be >> > + * anywhere between this head and stream->oa_buffer.tail. >> > + */ >> > + head = stream->oa_buffer.head - gtt_offset; >> > >> > + hw_tail -= gtt_offset; >> > + tail = hw_tail; >> > >> > + /* Walk the stream backward until we find a report with dword 0 >> > + * & 1 not at 0. Since the circular buffer pointers progress by >> > + * increments of 64 bytes and that reports can be up to 256 >> > + * bytes long, we can't tell whether a report has fully landed >> > + * in memory before the first 2 dwords of the following report >> > + * have effectively landed. >> > + * >> > + * This is assuming that the writes of the OA unit land in >> > + * memory in the order they were written to. >> > + * If not : (╯°□°）╯︵ ┻━┻ >> > */ >> > + while (OA_TAKEN(tail, head) >= report_size) { >> > + u32 previous_tail = (tail - report_size) & (OA_BUFFER_SIZE - 1); >> > + u32 *report32 = (void *)(stream->oa_buffer.vaddr + previous_tail); >> >> Sorry, this is wrong. This should just be: >> >> tail = (tail - report_size) & (OA_BUFFER_SIZE - 1); >> report32 = (void *)(stream->oa_buffer.vaddr + tail); >> >> Otherwise when we break out of the loop below tail is still set one >> report_size ahead. previous_tail is not needed. (In the previous version of >> the patch this used to work out correctly). >> >> > + >> > + /* Head of the report indicated by the HW tail register has >> > + * indeed landed into memory. >> > + */ >> > + if (report32[0] != 0 || report32[1] != 0) >> > + break; >> > + >> > + tail = previous_tail; >> > } > >Actually a couple of further improvements to the loop above are >possible. First there is no reason to start at previous_tail, we can just >start at the aligned hw_tail itself. Therefore the loop becomes: > > while (OA_TAKEN(tail, head) >= report_size) { > u32 *report32 = (void *)(stream->oa_buffer.vaddr + tail); > > if (report32[0] != 0 || report32[1] != 0) > break; > > tail = (tail - report_size) & (OA_BUFFER_SIZE - 1); > } > >Further, there is no reason to go back to the head but only to the old >tail. Therefore: > > head = stream->oa_buffer.head - gtt_offset; > old_tail = stream->oa_buffer.tail - gtt_offset; > > hw_tail -= gtt_offset; > tail = hw_tail; > > while (OA_TAKEN(tail, old_tail) >= report_size) { > u32 *report32 = (void *)(stream->oa_buffer.vaddr + tail); > > if (report32[0] != 0 || report32[1] != 0) > break; > > tail = (tail - report_size) & (OA_BUFFER_SIZE - 1); > } > >Please review and see if these two improvements are possible. Thanks! I think that this is possible. We could check reports between old_tail and tail to optimize the number of reports we read. I will give this a try. Thanks, Umesh _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Intel-gfx] [PATCH 1/3] drm/i915/perf: rework aging tail workaround 2020-03-21 23:26 ` Dixit, Ashutosh 2020-03-22 4:44 ` Dixit, Ashutosh @ 2020-03-24 3:15 ` Umesh Nerlige Ramappa 1 sibling, 0 replies; 15+ messages in thread From: Umesh Nerlige Ramappa @ 2020-03-24 3:15 UTC (permalink / raw) To: Dixit, Ashutosh; +Cc: intel-gfx On Sat, Mar 21, 2020 at 04:26:42PM -0700, Dixit, Ashutosh wrote: >On Thu, 19 Mar 2020 15:52:01 -0700, Umesh Nerlige Ramappa wrote: >> >> From: Lionel Landwerlin <lionel.g.landwerlin@intel.com> >> >> We're about to introduce an options to open the perf stream, giving >> the user ability to configure how often it wants the kernel to poll >> the OA registers for available data. >> >> Right now the workaround against the OA tail pointer race condition >> requires at least twice the internal kernel polling timer to make any >> data available. >> >> This changes introduce checks on the OA data written into the circular >> buffer to make as much data as possible available on the first >> iteration of the polling timer. > >/snip/ > >> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c >> index 3222f6cd8255..c1429d3acaf9 100644 >> --- a/drivers/gpu/drm/i915/i915_perf.c >> +++ b/drivers/gpu/drm/i915/i915_perf.c >> @@ -223,26 +223,17 @@ >> * >> * Although this can be observed explicitly while copying reports to userspace >> * by checking for a zeroed report-id field in tail reports, we want to account >> - * for this earlier, as part of the oa_buffer_check to avoid lots of redundant >> - * read() attempts. >> - * >> - * In effect we define a tail pointer for reading that lags the real tail >> - * pointer by at least %OA_TAIL_MARGIN_NSEC nanoseconds, which gives enough >> - * time for the corresponding reports to become visible to the CPU. >> - * >> - * To manage this we actually track two tail pointers: >> - * 1) An 'aging' tail with an associated timestamp that is tracked until we >> - * can trust the corresponding data is visible to the CPU; at which point >> - * it is considered 'aged'. >> - * 2) An 'aged' tail that can be used for read()ing. >> - * >> - * The two separate pointers let us decouple read()s from tail pointer aging. >> - * >> - * The tail pointers are checked and updated at a limited rate within a hrtimer >> - * callback (the same callback that is used for delivering EPOLLIN events) >> - * >> - * Initially the tails are marked invalid with %INVALID_TAIL_PTR which >> - * indicates that an updated tail pointer is needed. >> + * for this earlier, as part of the oa_buffer_check_unlocked to avoid lots of >> + * redundant read() attempts. >> + * >> + * We workaround this issue in oa_buffer_check_unlocked() by reading the reports >> + * in the OA buffer, starting from the tail reported by the HW until we find 2 >> + * consecutive reports with their first 2 dwords of not at 0. Those dwords are > >until we find a report with its first 2 dwords not 0 meaning its previous >report is completely in memory and ready to be read. > >> + * also set to 0 once read and the whole buffer is cleared upon OA buffer >> + * initialization. The first dword is the reason for this report while the >> + * second is the timestamp, making the chances of having those 2 fields at 0 >> + * fairly unlikely. A more detailed explanation is available in >> + * oa_buffer_check_unlocked(). > >> @@ -477,16 +468,6 @@ static bool oa_buffer_check_unlocked(struct i915_perf_stream *stream) >> */ >> spin_lock_irqsave(&stream->oa_buffer.ptr_lock, flags); >> >> hw_tail = stream->perf->ops.oa_hw_tail_read(stream); >> >> hw_tail &= ~(report_size - 1); >> >> @@ -496,64 +477,64 @@ static bool oa_buffer_check_unlocked(struct i915_perf_stream *stream) >> >> now = ktime_get_mono_fast_ns(); >> >> + if (hw_tail == stream->oa_buffer.aging_tail && >> + (now - stream->oa_buffer.aging_timestamp) > OA_TAIL_MARGIN_NSEC) { >> + /* If the HW tail hasn't move since the last check and the HW >> + * tail has been aging for long enough, declare it the new >> + * tail. >> + */ >> + stream->oa_buffer.tail = stream->oa_buffer.aging_tail; >> + } else { >> + u32 head, tail; >> >> + /* NB: The head we observe here might effectively be a little >> + * out of date. If a read() is in progress, the head could be >> + * anywhere between this head and stream->oa_buffer.tail. >> + */ >> + head = stream->oa_buffer.head - gtt_offset; >> >> + hw_tail -= gtt_offset; >> + tail = hw_tail; >> >> + /* Walk the stream backward until we find a report with dword 0 >> + * & 1 not at 0. Since the circular buffer pointers progress by >> + * increments of 64 bytes and that reports can be up to 256 >> + * bytes long, we can't tell whether a report has fully landed >> + * in memory before the first 2 dwords of the following report >> + * have effectively landed. >> + * >> + * This is assuming that the writes of the OA unit land in >> + * memory in the order they were written to. >> + * If not : (╯°□°）╯︵ ┻━┻ >> */ >> - if (hw_tail >= gtt_offset && >> - hw_tail < (gtt_offset + OA_BUFFER_SIZE)) { >> - stream->oa_buffer.tails[!aged_idx].offset = >> - aging_tail = hw_tail; >> - stream->oa_buffer.aging_timestamp = now; >> - } else { >> - drm_err(&stream->perf->i915->drm, >> - "Ignoring spurious out of range OA buffer tail pointer = %x\n", >> - hw_tail); >> + while (OA_TAKEN(tail, head) >= report_size) { >> + u32 previous_tail = (tail - report_size) & (OA_BUFFER_SIZE - 1); >> + u32 *report32 = (void *)(stream->oa_buffer.vaddr + previous_tail); > >Sorry, this is wrong. This should just be: > > tail = (tail - report_size) & (OA_BUFFER_SIZE - 1); > report32 = (void *)(stream->oa_buffer.vaddr + tail); > >Otherwise when we break out of the loop below tail is still set one >report_size ahead. previous_tail is not needed. (In the previous version of >the patch this used to work out correctly). oh, that's right. tail should point to the first report that has non-zero dwords. everything before that should be considered landed. Thanks, Umesh > >> + >> + /* Head of the report indicated by the HW tail register has >> + * indeed landed into memory. >> + */ >> + if (report32[0] != 0 || report32[1] != 0) >> + break; >> + >> + tail = previous_tail; >> } >> + >> + if (((tail - hw_tail) & (OA_BUFFER_SIZE - 1)) > report_size && > >nit: OA_TAKEN(hw_tail, tail) > report_size? > >> + __ratelimit(&stream->perf->tail_pointer_race)) >> + DRM_NOTE("unlanded report(s) head=0x%x " >> + "tail=0x%x hw_tail=0x%x\n", >> + head, tail, hw_tail); >> + >> + stream->oa_buffer.tail = gtt_offset + tail; >> + stream->oa_buffer.aging_tail = gtt_offset + hw_tail; >> + stream->oa_buffer.aging_timestamp = now; >> } >> >> spin_unlock_irqrestore(&stream->oa_buffer.ptr_lock, flags); >> >> - return aged_tail == INVALID_TAIL_PTR ? >> - false : OA_TAKEN(aged_tail, head) >= report_size; >> + return OA_TAKEN(stream->oa_buffer.tail - gtt_offset, >> + stream->oa_buffer.head - gtt_offset) >= report_size; >> } > >> @@ -303,6 +292,12 @@ struct i915_perf_stream { >> * OA buffer data to userspace. >> */ >> u32 head; >> + >> + /** >> + * @tail: The last tail verified tail that can be read by > >The last verified tail _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 15+ messages in thread
* [Intel-gfx] [PATCH 2/3] drm/i915/perf: move pollin setup to non hw specific code 2020-03-19 22:52 [Intel-gfx] [PATCH 0/3] drm/i915/perf: add OA interrupt support Umesh Nerlige Ramappa 2020-03-19 22:52 ` [Intel-gfx] [PATCH 1/3] drm/i915/perf: rework aging tail workaround Umesh Nerlige Ramappa @ 2020-03-19 22:52 ` Umesh Nerlige Ramappa 2020-03-19 22:52 ` [Intel-gfx] [PATCH 3/3] drm/i915/perf: add new open param to configure polling of OA buffer Umesh Nerlige Ramappa ` (3 subsequent siblings) 5 siblings, 0 replies; 15+ messages in thread From: Umesh Nerlige Ramappa @ 2020-03-19 22:52 UTC (permalink / raw) To: intel-gfx, Lionel G Landwerlin, Ashutosh Dixit From: Lionel Landwerlin <lionel.g.landwerlin@intel.com> This isn't really gen specific stuff, so just move it to the common code. v2: Rebase (Umesh) v3: Remove comment, pollin is a per stream state already (Ashutosh) Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com> --- drivers/gpu/drm/i915/i915_perf.c | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c index c1429d3acaf9..22a1a7300180 100644 --- a/drivers/gpu/drm/i915/i915_perf.c +++ b/drivers/gpu/drm/i915/i915_perf.c @@ -1421,8 +1421,6 @@ static void gen7_init_oa_buffer(struct i915_perf_stream *stream) * memory... */ memset(stream->oa_buffer.vaddr, 0, OA_BUFFER_SIZE); - - stream->pollin = false; } static void gen8_init_oa_buffer(struct i915_perf_stream *stream) @@ -1477,8 +1475,6 @@ static void gen8_init_oa_buffer(struct i915_perf_stream *stream) * memory... */ memset(stream->oa_buffer.vaddr, 0, OA_BUFFER_SIZE); - - stream->pollin = false; } static void gen12_init_oa_buffer(struct i915_perf_stream *stream) @@ -1534,8 +1530,6 @@ static void gen12_init_oa_buffer(struct i915_perf_stream *stream) */ memset(stream->oa_buffer.vaddr, 0, stream->oa_buffer.vma->size); - - stream->pollin = false; } static int alloc_oa_buffer(struct i915_perf_stream *stream) @@ -2603,6 +2597,8 @@ static void gen12_oa_enable(struct i915_perf_stream *stream) */ static void i915_oa_stream_enable(struct i915_perf_stream *stream) { + stream->pollin = false; + stream->perf->ops.oa_enable(stream); if (stream->periodic) @@ -3026,12 +3022,8 @@ static ssize_t i915_perf_read(struct file *file, * effectively ensures we back off until the next hrtimer callback * before reporting another EPOLLIN event. */ - if (ret >= 0 || ret == -EAGAIN) { - /* Maybe make ->pollin per-stream state if we support multiple - * concurrent streams in the future. - */ + if (ret >= 0 || ret == -EAGAIN) stream->pollin = false; - } return ret; } -- 2.20.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [Intel-gfx] [PATCH 3/3] drm/i915/perf: add new open param to configure polling of OA buffer 2020-03-19 22:52 [Intel-gfx] [PATCH 0/3] drm/i915/perf: add OA interrupt support Umesh Nerlige Ramappa 2020-03-19 22:52 ` [Intel-gfx] [PATCH 1/3] drm/i915/perf: rework aging tail workaround Umesh Nerlige Ramappa 2020-03-19 22:52 ` [Intel-gfx] [PATCH 2/3] drm/i915/perf: move pollin setup to non hw specific code Umesh Nerlige Ramappa @ 2020-03-19 22:52 ` Umesh Nerlige Ramappa 2020-03-21 7:16 ` Dixit, Ashutosh 2020-03-19 23:40 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915/perf: add OA interrupt support (rev7) Patchwork ` (2 subsequent siblings) 5 siblings, 1 reply; 15+ messages in thread From: Umesh Nerlige Ramappa @ 2020-03-19 22:52 UTC (permalink / raw) To: intel-gfx, Lionel G Landwerlin, Ashutosh Dixit From: Lionel Landwerlin <lionel.g.landwerlin@intel.com> This new parameter let's the application choose how often the OA buffer should be checked on the CPU side for data availability. Longer polling period tend to reduce CPU overhead if the application does not care about somewhat real time data collection. v2: Allow disabling polling completely with 0 value (Lionel) v3: Version the new parameter (Joonas) v4: Rebase (Umesh) v5: Make poll delay value of 0 invalid (Umesh) v6: - Describe poll_oa_period (Ashutosh) - Fix comment for new poll parameter (Lionel) - Drop open_flags in read_properties_unlocked (Lionel) - Rename uapi parameter (Ashutosh) Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> --- drivers/gpu/drm/i915/i915_perf.c | 32 ++++++++++++++++++++------ drivers/gpu/drm/i915/i915_perf_types.h | 6 +++++ include/uapi/drm/i915_drm.h | 13 +++++++++++ 3 files changed, 44 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c index 22a1a7300180..83de9fbca203 100644 --- a/drivers/gpu/drm/i915/i915_perf.c +++ b/drivers/gpu/drm/i915/i915_perf.c @@ -248,11 +248,11 @@ #define OA_TAIL_MARGIN_NSEC 100000ULL #define INVALID_TAIL_PTR 0xffffffff -/* frequency for checking whether the OA unit has written new reports to the - * circular OA buffer... +/* The default frequency for checking whether the OA unit has written new + * reports to the circular OA buffer... */ -#define POLL_FREQUENCY 200 -#define POLL_PERIOD (NSEC_PER_SEC / POLL_FREQUENCY) +#define DEFAULT_POLL_FREQUENCY_HZ 200 +#define DEFAULT_POLL_PERIOD_NS (NSEC_PER_SEC / DEFAULT_POLL_FREQUENCY_HZ) /* for sysctl proc_dointvec_minmax of dev.i915.perf_stream_paranoid */ static u32 i915_perf_stream_paranoid = true; @@ -339,6 +339,8 @@ static const struct i915_oa_format gen12_oa_formats[I915_OA_FORMAT_MAX] = { * @sseu: internal SSEU configuration computed either from the userspace * specified configuration in the opening parameters or a default value * (see get_default_sseu_config()) + * @poll_oa_period: The period in nanoseconds at which the CPU will check for OA + * data availability * * As read_properties_unlocked() enumerates and validates the properties given * to open a stream of metrics the configuration is built up in the structure @@ -361,6 +363,8 @@ struct perf_open_properties { bool has_sseu; struct intel_sseu sseu; + + u64 poll_oa_period; }; struct i915_oa_config_bo { @@ -2603,7 +2607,7 @@ static void i915_oa_stream_enable(struct i915_perf_stream *stream) if (stream->periodic) hrtimer_start(&stream->poll_check_timer, - ns_to_ktime(POLL_PERIOD), + ns_to_ktime(stream->poll_oa_period), HRTIMER_MODE_REL_PINNED); } @@ -3038,7 +3042,8 @@ static enum hrtimer_restart oa_poll_check_timer_cb(struct hrtimer *hrtimer) wake_up(&stream->poll_wq); } - hrtimer_forward_now(hrtimer, ns_to_ktime(POLL_PERIOD)); + hrtimer_forward_now(hrtimer, + ns_to_ktime(stream->poll_oa_period)); return HRTIMER_RESTART; } @@ -3427,6 +3432,7 @@ i915_perf_open_ioctl_locked(struct i915_perf *perf, stream->perf = perf; stream->ctx = specific_ctx; + stream->poll_oa_period = props->poll_oa_period; ret = i915_oa_stream_init(stream, param, props); if (ret) @@ -3505,6 +3511,7 @@ static int read_properties_unlocked(struct i915_perf *perf, int ret; memset(props, 0, sizeof(struct perf_open_properties)); + props->poll_oa_period = DEFAULT_POLL_PERIOD_NS; if (!n_props) { DRM_DEBUG("No i915 perf properties given\n"); @@ -3637,6 +3644,14 @@ static int read_properties_unlocked(struct i915_perf *perf, props->has_sseu = true; break; } + case DRM_I915_PERF_PROP_POLL_OA_PERIOD: + if (value < 100000 /* 100us */) { + DRM_DEBUG("OA availability timer too small (%lluns < 100us)\n", + value); + return -EINVAL; + } + props->poll_oa_period = value; + break; case DRM_I915_PERF_PROP_MAX: MISSING_CASE(id); return -EINVAL; @@ -4419,8 +4434,11 @@ int i915_perf_ioctl_version(void) * 4: Add DRM_I915_PERF_PROP_ALLOWED_SSEU to limit what contexts can * be run for the duration of the performance recording based on * their SSEU configuration. + * + * 5: Add DRM_I915_PERF_PROP_POLL_OA_PERIOD parameter that controls the + * interval for the hrtimer used to check for OA data. */ - return 4; + return 5; } #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST) diff --git a/drivers/gpu/drm/i915/i915_perf_types.h b/drivers/gpu/drm/i915/i915_perf_types.h index 6dbb5ee4c55c..1b469ff13abf 100644 --- a/drivers/gpu/drm/i915/i915_perf_types.h +++ b/drivers/gpu/drm/i915/i915_perf_types.h @@ -305,6 +305,12 @@ struct i915_perf_stream { * reprogrammed. */ struct i915_vma *noa_wait; + + /** + * @poll_oa_period: The period in nanoseconds at which the OA + * buffer should be checked for available data. + */ + u64 poll_oa_period; }; /** diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index db649d03ab52..c781b9f31b3c 100644 --- a/include/uapi/drm/i915_drm.h +++ b/include/uapi/drm/i915_drm.h @@ -1980,6 +1980,19 @@ enum drm_i915_perf_property_id { */ DRM_I915_PERF_PROP_GLOBAL_SSEU, + /** + * This optional parameter specifies the timer interval in nanoseconds + * at which the i915 driver will check the OA buffer for available data. + * Minimum allowed value is 100 microseconds. A default value is used by + * the driver if this parameter is not specified. Note that a large + * value may reduce cpu consumption during OA perf captures, but it + * would also potentially result in OA buffer overwrite as the captures + * reach end of the OA buffer. + * + * This property is available in perf revision 5. + */ + DRM_I915_PERF_PROP_POLL_OA_PERIOD, + DRM_I915_PERF_PROP_MAX /* non-ABI */ }; -- 2.20.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [Intel-gfx] [PATCH 3/3] drm/i915/perf: add new open param to configure polling of OA buffer 2020-03-19 22:52 ` [Intel-gfx] [PATCH 3/3] drm/i915/perf: add new open param to configure polling of OA buffer Umesh Nerlige Ramappa @ 2020-03-21 7:16 ` Dixit, Ashutosh 0 siblings, 0 replies; 15+ messages in thread From: Dixit, Ashutosh @ 2020-03-21 7:16 UTC (permalink / raw) To: Umesh Nerlige Ramappa; +Cc: intel-gfx On Thu, 19 Mar 2020 15:52:03 -0700, Umesh Nerlige Ramappa wrote: > > From: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > > This new parameter let's the application choose how often the OA > buffer should be checked on the CPU side for data availability. Longer > polling period tend to reduce CPU overhead if the application does not > care about somewhat real time data collection. /snip/ > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > index db649d03ab52..c781b9f31b3c 100644 > --- a/include/uapi/drm/i915_drm.h > +++ b/include/uapi/drm/i915_drm.h > @@ -1980,6 +1980,19 @@ enum drm_i915_perf_property_id { > */ > DRM_I915_PERF_PROP_GLOBAL_SSEU, > > + /** > + * This optional parameter specifies the timer interval in nanoseconds > + * at which the i915 driver will check the OA buffer for available data. > + * Minimum allowed value is 100 microseconds. A default value is used by > + * the driver if this parameter is not specified. Note that a large > + * value may reduce cpu consumption during OA perf captures, but it > + * would also potentially result in OA buffer overwrite as the captures > + * reach end of the OA buffer. I would like to change the wording here a bit because the wording above seems to say that the /same/ timer period which reduces cpu consumption will also potentially result in OA buffer overwrites. This is misleading and not true. Instead we should say something like: "Note that larger timer values will reduce cpu consumption during OA perf captures. However, excessively large values would potentially result in OA buffer overwrites as captures reach end of the OA buffer". Maybe some guidance to selecting the value, say based on sampling rate and OA buffer size, could also be added here but at the minimum we should fix up the wording as indicated above before pushing. Otherwise: Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com> > + * > + * This property is available in perf revision 5. > + */ > + DRM_I915_PERF_PROP_POLL_OA_PERIOD, > + > DRM_I915_PERF_PROP_MAX /* non-ABI */ > }; > > -- > 2.20.1 > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 15+ messages in thread
* [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915/perf: add OA interrupt support (rev7) 2020-03-19 22:52 [Intel-gfx] [PATCH 0/3] drm/i915/perf: add OA interrupt support Umesh Nerlige Ramappa ` (2 preceding siblings ...) 2020-03-19 22:52 ` [Intel-gfx] [PATCH 3/3] drm/i915/perf: add new open param to configure polling of OA buffer Umesh Nerlige Ramappa @ 2020-03-19 23:40 ` Patchwork 2020-03-19 23:43 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork 2020-03-20 0:04 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork 5 siblings, 0 replies; 15+ messages in thread From: Patchwork @ 2020-03-19 23:40 UTC (permalink / raw) To: Lionel Landwerlin; +Cc: intel-gfx == Series Details == Series: drm/i915/perf: add OA interrupt support (rev7) URL : https://patchwork.freedesktop.org/series/54280/ State : warning == Summary == $ dim checkpatch origin/drm-tip d7bd4aaf5bf3 drm/i915/perf: rework aging tail workaround -:125: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis #125: FILE: drivers/gpu/drm/i915/i915_perf.c:481: + if (hw_tail == stream->oa_buffer.aging_tail && + (now - stream->oa_buffer.aging_timestamp) > OA_TAIL_MARGIN_NSEC) { total: 0 errors, 0 warnings, 1 checks, 350 lines checked 3874f27479a6 drm/i915/perf: move pollin setup to non hw specific code 4bfbbb959b1f drm/i915/perf: add new open param to configure polling of OA buffer _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 15+ messages in thread
* [Intel-gfx] ✗ Fi.CI.SPARSE: warning for drm/i915/perf: add OA interrupt support (rev7) 2020-03-19 22:52 [Intel-gfx] [PATCH 0/3] drm/i915/perf: add OA interrupt support Umesh Nerlige Ramappa ` (3 preceding siblings ...) 2020-03-19 23:40 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915/perf: add OA interrupt support (rev7) Patchwork @ 2020-03-19 23:43 ` Patchwork 2020-03-20 0:04 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork 5 siblings, 0 replies; 15+ messages in thread From: Patchwork @ 2020-03-19 23:43 UTC (permalink / raw) To: Lionel Landwerlin; +Cc: intel-gfx == Series Details == Series: drm/i915/perf: add OA interrupt support (rev7) URL : https://patchwork.freedesktop.org/series/54280/ State : warning == Summary == $ dim sparse origin/drm-tip Sparse version: v0.6.0 Commit: drm/i915/perf: rework aging tail workaround Okay! Commit: drm/i915/perf: move pollin setup to non hw specific code -O:drivers/gpu/drm/i915/i915_perf.c:1423:15: warning: memset with byte count of 16777216 -O:drivers/gpu/drm/i915/i915_perf.c:1479:15: warning: memset with byte count of 16777216 +drivers/gpu/drm/i915/i915_perf.c:1423:15: warning: memset with byte count of 16777216 +drivers/gpu/drm/i915/i915_perf.c:1477:15: warning: memset with byte count of 16777216 Commit: drm/i915/perf: add new open param to configure polling of OA buffer Okay! _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 15+ messages in thread
* [Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915/perf: add OA interrupt support (rev7) 2020-03-19 22:52 [Intel-gfx] [PATCH 0/3] drm/i915/perf: add OA interrupt support Umesh Nerlige Ramappa ` (4 preceding siblings ...) 2020-03-19 23:43 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork @ 2020-03-20 0:04 ` Patchwork 5 siblings, 0 replies; 15+ messages in thread From: Patchwork @ 2020-03-20 0:04 UTC (permalink / raw) To: Lionel Landwerlin; +Cc: intel-gfx == Series Details == Series: drm/i915/perf: add OA interrupt support (rev7) URL : https://patchwork.freedesktop.org/series/54280/ State : success == Summary == CI Bug Log - changes from CI_DRM_8161 -> Patchwork_17030 ==================================================== Summary ------- **SUCCESS** No regressions found. External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17030/index.html Known issues ------------ Here are the changes found in Patchwork_17030 that come from known issues: ### IGT changes ### #### Possible fixes #### * igt@i915_pm_rpm@module-reload: - fi-icl-dsi: [INCOMPLETE][1] ([i915#189]) -> [PASS][2] [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8161/fi-icl-dsi/igt@i915_pm_rpm@module-reload.html [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17030/fi-icl-dsi/igt@i915_pm_rpm@module-reload.html * igt@i915_selftest@live@execlists: - {fi-ehl-1}: [INCOMPLETE][3] -> [PASS][4] [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8161/fi-ehl-1/igt@i915_selftest@live@execlists.html [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17030/fi-ehl-1/igt@i915_selftest@live@execlists.html - fi-kbl-soraka: [INCOMPLETE][5] ([fdo#112259] / [i915#656]) -> [PASS][6] [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8161/fi-kbl-soraka/igt@i915_selftest@live@execlists.html [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17030/fi-kbl-soraka/igt@i915_selftest@live@execlists.html {name}: This element is suppressed. This means it is ignored when computing the status of the difference (SUCCESS, WARNING, or FAILURE). [fdo#112259]: https://bugs.freedesktop.org/show_bug.cgi?id=112259 [i915#189]: https://gitlab.freedesktop.org/drm/intel/issues/189 [i915#656]: https://gitlab.freedesktop.org/drm/intel/issues/656 Participating hosts (36 -> 40) ------------------------------ Additional (7): fi-bdw-5557u fi-kbl-7560u fi-bsw-n3050 fi-ivb-3770 fi-cfl-8109u fi-blb-e6850 fi-skl-6600u Missing (3): fi-byt-clapper fi-bsw-cyan fi-bwr-2160 Build changes ------------- * CI: CI-20190529 -> None * Linux: CI_DRM_8161 -> Patchwork_17030 CI-20190529: 20190529 CI_DRM_8161: b2b8d8634bf653904ef3268ca7ccedf51f6405af @ git://anongit.freedesktop.org/gfx-ci/linux IGT_5523: cf6d524007ac51a7d5a48503ea3dd5f01fd4ebab @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools Patchwork_17030: 4bfbbb959b1f835f6857b7b5320171291e0ebc3e @ git://anongit.freedesktop.org/gfx-ci/linux == Linux commits == 4bfbbb959b1f drm/i915/perf: add new open param to configure polling of OA buffer 3874f27479a6 drm/i915/perf: move pollin setup to non hw specific code d7bd4aaf5bf3 drm/i915/perf: rework aging tail workaround == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17030/index.html _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 15+ messages in thread
* [Intel-gfx] [PATCH 0/3] drm/i915/perf: add OA interrupt support @ 2020-03-24 18:54 Umesh Nerlige Ramappa 2020-03-24 18:54 ` [Intel-gfx] [PATCH 1/3] drm/i915/perf: rework aging tail workaround Umesh Nerlige Ramappa 0 siblings, 1 reply; 15+ messages in thread From: Umesh Nerlige Ramappa @ 2020-03-24 18:54 UTC (permalink / raw) To: intel-gfx, Lionel G Landwerlin, Ashutosh Dixit Hi all, This is a revival of an earlier patch series submitted by Lionel Landwerlin - https://patchwork.freedesktop.org/series/54280/ The patches enable interrupt support for the perf OA unit in i915, further details can be found in the orignal series linked above. v2: (Umesh) - This series will split into 2. This revision will only support the addition of poll delay parameter to perf OA. If needed a future revision will incorporate interrupt support. Regards, Umesh Lionel Landwerlin (3): drm/i915/perf: rework aging tail workaround drm/i915/perf: move pollin setup to non hw specific code drm/i915/perf: add new open param to configure polling of OA buffer drivers/gpu/drm/i915/i915_perf.c | 250 +++++++++++-------------- drivers/gpu/drm/i915/i915_perf_types.h | 34 ++-- include/uapi/drm/i915_drm.h | 13 ++ 3 files changed, 144 insertions(+), 153 deletions(-) -- 2.20.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 15+ messages in thread
* [Intel-gfx] [PATCH 1/3] drm/i915/perf: rework aging tail workaround 2020-03-24 18:54 [Intel-gfx] [PATCH 0/3] drm/i915/perf: add OA interrupt support Umesh Nerlige Ramappa @ 2020-03-24 18:54 ` Umesh Nerlige Ramappa 2020-03-24 21:25 ` Dixit, Ashutosh 0 siblings, 1 reply; 15+ messages in thread From: Umesh Nerlige Ramappa @ 2020-03-24 18:54 UTC (permalink / raw) To: intel-gfx, Lionel G Landwerlin, Ashutosh Dixit From: Lionel Landwerlin <lionel.g.landwerlin@intel.com> We're about to introduce an options to open the perf stream, giving the user ability to configure how often it wants the kernel to poll the OA registers for available data. Right now the workaround against the OA tail pointer race condition requires at least twice the internal kernel polling timer to make any data available. This changes introduce checks on the OA data written into the circular buffer to make as much data as possible available on the first iteration of the polling timer. v2: Use OA_TAKEN macro without the gtt_offset (Lionel) v3: (Umesh) - Rebase - Change report to report32 from below review https://patchwork.freedesktop.org/patch/330704/?series=66697&rev=1 v4: (Ashutosh, Lionel) - Fix checkpatch errors - Fix aging_timestamp initialization - Check for only one valid landed report - Fix check for unlanded report v5: (Ashutosh) - Fix bug in accurately determining landed report. - Optimize the check for landed reports by going as far as the previously determined aged tail. Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> --- drivers/gpu/drm/i915/i915_perf.c | 204 ++++++++++--------------- drivers/gpu/drm/i915/i915_perf_types.h | 28 ++-- 2 files changed, 97 insertions(+), 135 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c index 3222f6cd8255..4583ae9b77be 100644 --- a/drivers/gpu/drm/i915/i915_perf.c +++ b/drivers/gpu/drm/i915/i915_perf.c @@ -223,26 +223,17 @@ * * Although this can be observed explicitly while copying reports to userspace * by checking for a zeroed report-id field in tail reports, we want to account - * for this earlier, as part of the oa_buffer_check to avoid lots of redundant - * read() attempts. - * - * In effect we define a tail pointer for reading that lags the real tail - * pointer by at least %OA_TAIL_MARGIN_NSEC nanoseconds, which gives enough - * time for the corresponding reports to become visible to the CPU. - * - * To manage this we actually track two tail pointers: - * 1) An 'aging' tail with an associated timestamp that is tracked until we - * can trust the corresponding data is visible to the CPU; at which point - * it is considered 'aged'. - * 2) An 'aged' tail that can be used for read()ing. - * - * The two separate pointers let us decouple read()s from tail pointer aging. - * - * The tail pointers are checked and updated at a limited rate within a hrtimer - * callback (the same callback that is used for delivering EPOLLIN events) - * - * Initially the tails are marked invalid with %INVALID_TAIL_PTR which - * indicates that an updated tail pointer is needed. + * for this earlier, as part of the oa_buffer_check_unlocked to avoid lots of + * redundant read() attempts. + * + * We workaround this issue in oa_buffer_check_unlocked() by reading the reports + * in the OA buffer, starting from the tail reported by the HW until we find a + * report with its first 2 dwords not 0 meaning its previous report is + * completely in memory and ready to be read. Those dwords are also set to 0 + * once read and the whole buffer is cleared upon OA buffer initialization. The + * first dword is the reason for this report while the second is the timestamp, + * making the chances of having those 2 fields at 0 fairly unlikely. A more + * detailed explanation is available in oa_buffer_check_unlocked(). * * Most of the implementation details for this workaround are in * oa_buffer_check_unlocked() and _append_oa_reports() @@ -454,8 +445,8 @@ static u32 gen7_oa_hw_tail_read(struct i915_perf_stream *stream) * (See description of OA_TAIL_MARGIN_NSEC above for further details.) * * Besides returning true when there is data available to read() this function - * also has the side effect of updating the oa_buffer.tails[], .aging_timestamp - * and .aged_tail_idx state used for reading. + * also updates the tail, aging_tail and aging_timestamp in the oa_buffer + * object. * * Note: It's safe to read OA config state here unlocked, assuming that this is * only called while the stream is enabled, while the global OA configuration @@ -465,28 +456,18 @@ static u32 gen7_oa_hw_tail_read(struct i915_perf_stream *stream) */ static bool oa_buffer_check_unlocked(struct i915_perf_stream *stream) { + u32 gtt_offset = i915_ggtt_offset(stream->oa_buffer.vma); int report_size = stream->oa_buffer.format_size; unsigned long flags; - unsigned int aged_idx; - u32 head, hw_tail, aged_tail, aging_tail; + u32 hw_tail; u64 now; /* We have to consider the (unlikely) possibility that read() errors - * could result in an OA buffer reset which might reset the head, - * tails[] and aged_tail state. + * could result in an OA buffer reset which might reset the head and + * tail state. */ spin_lock_irqsave(&stream->oa_buffer.ptr_lock, flags); - /* NB: The head we observe here might effectively be a little out of - * date (between head and tails[aged_idx].offset if there is currently - * a read() in progress. - */ - head = stream->oa_buffer.head; - - aged_idx = stream->oa_buffer.aged_tail_idx; - aged_tail = stream->oa_buffer.tails[aged_idx].offset; - aging_tail = stream->oa_buffer.tails[!aged_idx].offset; - hw_tail = stream->perf->ops.oa_hw_tail_read(stream); /* The tail pointer increases in 64 byte increments, @@ -496,64 +477,61 @@ static bool oa_buffer_check_unlocked(struct i915_perf_stream *stream) now = ktime_get_mono_fast_ns(); - /* Update the aged tail - * - * Flip the tail pointer available for read()s once the aging tail is - * old enough to trust that the corresponding data will be visible to - * the CPU... - * - * Do this before updating the aging pointer in case we may be able to - * immediately start aging a new pointer too (if new data has become - * available) without needing to wait for a later hrtimer callback. - */ - if (aging_tail != INVALID_TAIL_PTR && - ((now - stream->oa_buffer.aging_timestamp) > - OA_TAIL_MARGIN_NSEC)) { - - aged_idx ^= 1; - stream->oa_buffer.aged_tail_idx = aged_idx; + if (hw_tail == stream->oa_buffer.aging_tail && + (now - stream->oa_buffer.aging_timestamp) > OA_TAIL_MARGIN_NSEC) { + /* If the HW tail hasn't move since the last check and the HW + * tail has been aging for long enough, declare it the new + * tail. + */ + stream->oa_buffer.tail = stream->oa_buffer.aging_tail; + } else { + u32 head, tail, aged_tail; - aged_tail = aging_tail; + /* NB: The head we observe here might effectively be a little + * out of date. If a read() is in progress, the head could be + * anywhere between this head and stream->oa_buffer.tail. + */ + head = stream->oa_buffer.head - gtt_offset; + aged_tail = stream->oa_buffer.tail - gtt_offset; + + hw_tail -= gtt_offset; + tail = hw_tail; + + /* Walk the stream backward until we find a report with dword 0 + * & 1 not at 0. Since the circular buffer pointers progress by + * increments of 64 bytes and that reports can be up to 256 + * bytes long, we can't tell whether a report has fully landed + * in memory before the first 2 dwords of the following report + * have effectively landed. + * + * This is assuming that the writes of the OA unit land in + * memory in the order they were written to. + * If not : (╯°□°）╯︵ ┻━┻ + */ + while (OA_TAKEN(tail, aged_tail) >= report_size) { + u32 *report32 = (void *)(stream->oa_buffer.vaddr + tail); - /* Mark that we need a new pointer to start aging... */ - stream->oa_buffer.tails[!aged_idx].offset = INVALID_TAIL_PTR; - aging_tail = INVALID_TAIL_PTR; - } + if (report32[0] != 0 || report32[1] != 0) + break; - /* Update the aging tail - * - * We throttle aging tail updates until we have a new tail that - * represents >= one report more data than is already available for - * reading. This ensures there will be enough data for a successful - * read once this new pointer has aged and ensures we will give the new - * pointer time to age. - */ - if (aging_tail == INVALID_TAIL_PTR && - (aged_tail == INVALID_TAIL_PTR || - OA_TAKEN(hw_tail, aged_tail) >= report_size)) { - struct i915_vma *vma = stream->oa_buffer.vma; - u32 gtt_offset = i915_ggtt_offset(vma); - - /* Be paranoid and do a bounds check on the pointer read back - * from hardware, just in case some spurious hardware condition - * could put the tail out of bounds... - */ - if (hw_tail >= gtt_offset && - hw_tail < (gtt_offset + OA_BUFFER_SIZE)) { - stream->oa_buffer.tails[!aged_idx].offset = - aging_tail = hw_tail; - stream->oa_buffer.aging_timestamp = now; - } else { - drm_err(&stream->perf->i915->drm, - "Ignoring spurious out of range OA buffer tail pointer = %x\n", - hw_tail); + tail = (tail - report_size) & (OA_BUFFER_SIZE - 1); } + + if (OA_TAKEN(hw_tail, tail) > report_size && + __ratelimit(&stream->perf->tail_pointer_race)) + DRM_NOTE("unlanded report(s) head=0x%x " + "tail=0x%x hw_tail=0x%x\n", + head, tail, hw_tail); + + stream->oa_buffer.tail = gtt_offset + tail; + stream->oa_buffer.aging_tail = gtt_offset + hw_tail; + stream->oa_buffer.aging_timestamp = now; } spin_unlock_irqrestore(&stream->oa_buffer.ptr_lock, flags); - return aged_tail == INVALID_TAIL_PTR ? - false : OA_TAKEN(aged_tail, head) >= report_size; + return OA_TAKEN(stream->oa_buffer.tail - gtt_offset, + stream->oa_buffer.head - gtt_offset) >= report_size; } /** @@ -671,7 +649,6 @@ static int gen8_append_oa_reports(struct i915_perf_stream *stream, u32 mask = (OA_BUFFER_SIZE - 1); size_t start_offset = *offset; unsigned long flags; - unsigned int aged_tail_idx; u32 head, tail; u32 taken; int ret = 0; @@ -682,18 +659,10 @@ static int gen8_append_oa_reports(struct i915_perf_stream *stream, spin_lock_irqsave(&stream->oa_buffer.ptr_lock, flags); head = stream->oa_buffer.head; - aged_tail_idx = stream->oa_buffer.aged_tail_idx; - tail = stream->oa_buffer.tails[aged_tail_idx].offset; + tail = stream->oa_buffer.tail; spin_unlock_irqrestore(&stream->oa_buffer.ptr_lock, flags); - /* - * An invalid tail pointer here means we're still waiting for the poll - * hrtimer callback to give us a pointer - */ - if (tail == INVALID_TAIL_PTR) - return -EAGAIN; - /* * NB: oa_buffer.head/tail include the gtt_offset which we don't want * while indexing relative to oa_buf_base. @@ -827,13 +796,11 @@ static int gen8_append_oa_reports(struct i915_perf_stream *stream, } /* - * The above reason field sanity check is based on - * the assumption that the OA buffer is initially - * zeroed and we reset the field after copying so the - * check is still meaningful once old reports start - * being overwritten. + * Clear out the first 2 dword as a mean to detect unlanded + * reports. */ report32[0] = 0; + report32[1] = 0; } if (start_offset != *offset) { @@ -974,7 +941,6 @@ static int gen7_append_oa_reports(struct i915_perf_stream *stream, u32 mask = (OA_BUFFER_SIZE - 1); size_t start_offset = *offset; unsigned long flags; - unsigned int aged_tail_idx; u32 head, tail; u32 taken; int ret = 0; @@ -985,17 +951,10 @@ static int gen7_append_oa_reports(struct i915_perf_stream *stream, spin_lock_irqsave(&stream->oa_buffer.ptr_lock, flags); head = stream->oa_buffer.head; - aged_tail_idx = stream->oa_buffer.aged_tail_idx; - tail = stream->oa_buffer.tails[aged_tail_idx].offset; + tail = stream->oa_buffer.tail; spin_unlock_irqrestore(&stream->oa_buffer.ptr_lock, flags); - /* An invalid tail pointer here means we're still waiting for the poll - * hrtimer callback to give us a pointer - */ - if (tail == INVALID_TAIL_PTR) - return -EAGAIN; - /* NB: oa_buffer.head/tail include the gtt_offset which we don't want * while indexing relative to oa_buf_base. */ @@ -1053,13 +1012,11 @@ static int gen7_append_oa_reports(struct i915_perf_stream *stream, if (ret) break; - /* The above report-id field sanity check is based on - * the assumption that the OA buffer is initially - * zeroed and we reset the field after copying so the - * check is still meaningful once old reports start - * being overwritten. + /* Clear out the first 2 dwords as a mean to detect unlanded + * reports. */ report32[0] = 0; + report32[1] = 0; } if (start_offset != *offset) { @@ -1438,8 +1395,8 @@ static void gen7_init_oa_buffer(struct i915_perf_stream *stream) gtt_offset | OABUFFER_SIZE_16M); /* Mark that we need updated tail pointers to read from... */ - stream->oa_buffer.tails[0].offset = INVALID_TAIL_PTR; - stream->oa_buffer.tails[1].offset = INVALID_TAIL_PTR; + stream->oa_buffer.aging_tail = INVALID_TAIL_PTR; + stream->oa_buffer.tail = gtt_offset; spin_unlock_irqrestore(&stream->oa_buffer.ptr_lock, flags); @@ -1492,8 +1449,8 @@ static void gen8_init_oa_buffer(struct i915_perf_stream *stream) intel_uncore_write(uncore, GEN8_OATAILPTR, gtt_offset & GEN8_OATAILPTR_MASK); /* Mark that we need updated tail pointers to read from... */ - stream->oa_buffer.tails[0].offset = INVALID_TAIL_PTR; - stream->oa_buffer.tails[1].offset = INVALID_TAIL_PTR; + stream->oa_buffer.aging_tail = INVALID_TAIL_PTR; + stream->oa_buffer.tail = gtt_offset; /* * Reset state used to recognise context switches, affecting which @@ -1548,8 +1505,8 @@ static void gen12_init_oa_buffer(struct i915_perf_stream *stream) gtt_offset & GEN12_OAG_OATAILPTR_MASK); /* Mark that we need updated tail pointers to read from... */ - stream->oa_buffer.tails[0].offset = INVALID_TAIL_PTR; - stream->oa_buffer.tails[1].offset = INVALID_TAIL_PTR; + stream->oa_buffer.aging_tail = INVALID_TAIL_PTR; + stream->oa_buffer.tail = gtt_offset; /* * Reset state used to recognise context switches, affecting which @@ -4398,6 +4355,11 @@ void i915_perf_init(struct drm_i915_private *i915) ratelimit_set_flags(&perf->spurious_report_rs, RATELIMIT_MSG_ON_RELEASE); + ratelimit_state_init(&perf->tail_pointer_race, + 5 * HZ, 10); + ratelimit_set_flags(&perf->tail_pointer_race, + RATELIMIT_MSG_ON_RELEASE); + atomic64_set(&perf->noa_programming_delay, 500 * 1000 /* 500us */); diff --git a/drivers/gpu/drm/i915/i915_perf_types.h b/drivers/gpu/drm/i915/i915_perf_types.h index 32289cbda648..c3ab184c604a 100644 --- a/drivers/gpu/drm/i915/i915_perf_types.h +++ b/drivers/gpu/drm/i915/i915_perf_types.h @@ -273,21 +273,10 @@ struct i915_perf_stream { spinlock_t ptr_lock; /** - * @tails: One 'aging' tail pointer and one 'aged' tail pointer ready to - * used for reading. - * - * Initial values of 0xffffffff are invalid and imply that an - * update is required (and should be ignored by an attempted - * read) + * @aging_tail: The last HW tail reported by HW. The data + * might not have made it to memory yet though. */ - struct { - u32 offset; - } tails[2]; - - /** - * @aged_tail_idx: Index for the aged tail ready to read() data up to. - */ - unsigned int aged_tail_idx; + u32 aging_tail; /** * @aging_timestamp: A monotonic timestamp for when the current aging tail pointer @@ -303,6 +292,11 @@ struct i915_perf_stream { * OA buffer data to userspace. */ u32 head; + + /** + * @tail: The last verified tail that can be read by userspace. + */ + u32 tail; } oa_buffer; /** @@ -420,6 +414,12 @@ struct i915_perf { */ struct ratelimit_state spurious_report_rs; + /** + * For rate limiting any notifications of tail pointer + * race. + */ + struct ratelimit_state tail_pointer_race; + u32 gen7_latched_oastatus1; u32 ctx_oactxctrl_offset; u32 ctx_flexeu0_offset; -- 2.20.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [Intel-gfx] [PATCH 1/3] drm/i915/perf: rework aging tail workaround 2020-03-24 18:54 ` [Intel-gfx] [PATCH 1/3] drm/i915/perf: rework aging tail workaround Umesh Nerlige Ramappa @ 2020-03-24 21:25 ` Dixit, Ashutosh 0 siblings, 0 replies; 15+ messages in thread From: Dixit, Ashutosh @ 2020-03-24 21:25 UTC (permalink / raw) To: Umesh Nerlige Ramappa; +Cc: intel-gfx On Tue, 24 Mar 2020 11:54:55 -0700, Umesh Nerlige Ramappa wrote: > > From: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > > We're about to introduce an options to open the perf stream, giving > the user ability to configure how often it wants the kernel to poll > the OA registers for available data. > > Right now the workaround against the OA tail pointer race condition > requires at least twice the internal kernel polling timer to make any > data available. > > This changes introduce checks on the OA data written into the circular > buffer to make as much data as possible available on the first > iteration of the polling timer. Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com> > > v2: Use OA_TAKEN macro without the gtt_offset (Lionel) > v3: (Umesh) > - Rebase > - Change report to report32 from below review > https://patchwork.freedesktop.org/patch/330704/?series=66697&rev=1 > v4: (Ashutosh, Lionel) > - Fix checkpatch errors > - Fix aging_timestamp initialization > - Check for only one valid landed report > - Fix check for unlanded report > v5: (Ashutosh) > - Fix bug in accurately determining landed report. > - Optimize the check for landed reports by going as far as the > previously determined aged tail. > > Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> > --- > drivers/gpu/drm/i915/i915_perf.c | 204 ++++++++++--------------- > drivers/gpu/drm/i915/i915_perf_types.h | 28 ++-- > 2 files changed, 97 insertions(+), 135 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c > index 3222f6cd8255..4583ae9b77be 100644 > --- a/drivers/gpu/drm/i915/i915_perf.c > +++ b/drivers/gpu/drm/i915/i915_perf.c > @@ -223,26 +223,17 @@ > * > * Although this can be observed explicitly while copying reports to userspace > * by checking for a zeroed report-id field in tail reports, we want to account > - * for this earlier, as part of the oa_buffer_check to avoid lots of redundant > - * read() attempts. > - * > - * In effect we define a tail pointer for reading that lags the real tail > - * pointer by at least %OA_TAIL_MARGIN_NSEC nanoseconds, which gives enough > - * time for the corresponding reports to become visible to the CPU. > - * > - * To manage this we actually track two tail pointers: > - * 1) An 'aging' tail with an associated timestamp that is tracked until we > - * can trust the corresponding data is visible to the CPU; at which point > - * it is considered 'aged'. > - * 2) An 'aged' tail that can be used for read()ing. > - * > - * The two separate pointers let us decouple read()s from tail pointer aging. > - * > - * The tail pointers are checked and updated at a limited rate within a hrtimer > - * callback (the same callback that is used for delivering EPOLLIN events) > - * > - * Initially the tails are marked invalid with %INVALID_TAIL_PTR which > - * indicates that an updated tail pointer is needed. > + * for this earlier, as part of the oa_buffer_check_unlocked to avoid lots of > + * redundant read() attempts. > + * > + * We workaround this issue in oa_buffer_check_unlocked() by reading the reports > + * in the OA buffer, starting from the tail reported by the HW until we find a > + * report with its first 2 dwords not 0 meaning its previous report is > + * completely in memory and ready to be read. Those dwords are also set to 0 > + * once read and the whole buffer is cleared upon OA buffer initialization. The > + * first dword is the reason for this report while the second is the timestamp, > + * making the chances of having those 2 fields at 0 fairly unlikely. A more > + * detailed explanation is available in oa_buffer_check_unlocked(). > * > * Most of the implementation details for this workaround are in > * oa_buffer_check_unlocked() and _append_oa_reports() > @@ -454,8 +445,8 @@ static u32 gen7_oa_hw_tail_read(struct i915_perf_stream *stream) > * (See description of OA_TAIL_MARGIN_NSEC above for further details.) > * > * Besides returning true when there is data available to read() this function > - * also has the side effect of updating the oa_buffer.tails[], .aging_timestamp > - * and .aged_tail_idx state used for reading. > + * also updates the tail, aging_tail and aging_timestamp in the oa_buffer > + * object. > * > * Note: It's safe to read OA config state here unlocked, assuming that this is > * only called while the stream is enabled, while the global OA configuration > @@ -465,28 +456,18 @@ static u32 gen7_oa_hw_tail_read(struct i915_perf_stream *stream) > */ > static bool oa_buffer_check_unlocked(struct i915_perf_stream *stream) > { > + u32 gtt_offset = i915_ggtt_offset(stream->oa_buffer.vma); > int report_size = stream->oa_buffer.format_size; > unsigned long flags; > - unsigned int aged_idx; > - u32 head, hw_tail, aged_tail, aging_tail; > + u32 hw_tail; > u64 now; > > /* We have to consider the (unlikely) possibility that read() errors > - * could result in an OA buffer reset which might reset the head, > - * tails[] and aged_tail state. > + * could result in an OA buffer reset which might reset the head and > + * tail state. > */ > spin_lock_irqsave(&stream->oa_buffer.ptr_lock, flags); > > - /* NB: The head we observe here might effectively be a little out of > - * date (between head and tails[aged_idx].offset if there is currently > - * a read() in progress. > - */ > - head = stream->oa_buffer.head; > - > - aged_idx = stream->oa_buffer.aged_tail_idx; > - aged_tail = stream->oa_buffer.tails[aged_idx].offset; > - aging_tail = stream->oa_buffer.tails[!aged_idx].offset; > - > hw_tail = stream->perf->ops.oa_hw_tail_read(stream); > > /* The tail pointer increases in 64 byte increments, > @@ -496,64 +477,61 @@ static bool oa_buffer_check_unlocked(struct i915_perf_stream *stream) > > now = ktime_get_mono_fast_ns(); > > - /* Update the aged tail > - * > - * Flip the tail pointer available for read()s once the aging tail is > - * old enough to trust that the corresponding data will be visible to > - * the CPU... > - * > - * Do this before updating the aging pointer in case we may be able to > - * immediately start aging a new pointer too (if new data has become > - * available) without needing to wait for a later hrtimer callback. > - */ > - if (aging_tail != INVALID_TAIL_PTR && > - ((now - stream->oa_buffer.aging_timestamp) > > - OA_TAIL_MARGIN_NSEC)) { > - > - aged_idx ^= 1; > - stream->oa_buffer.aged_tail_idx = aged_idx; > + if (hw_tail == stream->oa_buffer.aging_tail && > + (now - stream->oa_buffer.aging_timestamp) > OA_TAIL_MARGIN_NSEC) { > + /* If the HW tail hasn't move since the last check and the HW > + * tail has been aging for long enough, declare it the new > + * tail. > + */ > + stream->oa_buffer.tail = stream->oa_buffer.aging_tail; > + } else { > + u32 head, tail, aged_tail; > > - aged_tail = aging_tail; > + /* NB: The head we observe here might effectively be a little > + * out of date. If a read() is in progress, the head could be > + * anywhere between this head and stream->oa_buffer.tail. > + */ > + head = stream->oa_buffer.head - gtt_offset; > + aged_tail = stream->oa_buffer.tail - gtt_offset; > + > + hw_tail -= gtt_offset; > + tail = hw_tail; > + > + /* Walk the stream backward until we find a report with dword 0 > + * & 1 not at 0. Since the circular buffer pointers progress by > + * increments of 64 bytes and that reports can be up to 256 > + * bytes long, we can't tell whether a report has fully landed > + * in memory before the first 2 dwords of the following report > + * have effectively landed. > + * > + * This is assuming that the writes of the OA unit land in > + * memory in the order they were written to. > + * If not : (╯°□°）╯︵ ┻━┻ > + */ > + while (OA_TAKEN(tail, aged_tail) >= report_size) { > + u32 *report32 = (void *)(stream->oa_buffer.vaddr + tail); > > - /* Mark that we need a new pointer to start aging... */ > - stream->oa_buffer.tails[!aged_idx].offset = INVALID_TAIL_PTR; > - aging_tail = INVALID_TAIL_PTR; > - } > + if (report32[0] != 0 || report32[1] != 0) > + break; > > - /* Update the aging tail > - * > - * We throttle aging tail updates until we have a new tail that > - * represents >= one report more data than is already available for > - * reading. This ensures there will be enough data for a successful > - * read once this new pointer has aged and ensures we will give the new > - * pointer time to age. > - */ > - if (aging_tail == INVALID_TAIL_PTR && > - (aged_tail == INVALID_TAIL_PTR || > - OA_TAKEN(hw_tail, aged_tail) >= report_size)) { > - struct i915_vma *vma = stream->oa_buffer.vma; > - u32 gtt_offset = i915_ggtt_offset(vma); > - > - /* Be paranoid and do a bounds check on the pointer read back > - * from hardware, just in case some spurious hardware condition > - * could put the tail out of bounds... > - */ > - if (hw_tail >= gtt_offset && > - hw_tail < (gtt_offset + OA_BUFFER_SIZE)) { > - stream->oa_buffer.tails[!aged_idx].offset = > - aging_tail = hw_tail; > - stream->oa_buffer.aging_timestamp = now; > - } else { > - drm_err(&stream->perf->i915->drm, > - "Ignoring spurious out of range OA buffer tail pointer = %x\n", > - hw_tail); > + tail = (tail - report_size) & (OA_BUFFER_SIZE - 1); > } > + > + if (OA_TAKEN(hw_tail, tail) > report_size && > + __ratelimit(&stream->perf->tail_pointer_race)) > + DRM_NOTE("unlanded report(s) head=0x%x " > + "tail=0x%x hw_tail=0x%x\n", > + head, tail, hw_tail); > + > + stream->oa_buffer.tail = gtt_offset + tail; > + stream->oa_buffer.aging_tail = gtt_offset + hw_tail; > + stream->oa_buffer.aging_timestamp = now; > } > > spin_unlock_irqrestore(&stream->oa_buffer.ptr_lock, flags); > > - return aged_tail == INVALID_TAIL_PTR ? > - false : OA_TAKEN(aged_tail, head) >= report_size; > + return OA_TAKEN(stream->oa_buffer.tail - gtt_offset, > + stream->oa_buffer.head - gtt_offset) >= report_size; > } > > /** > @@ -671,7 +649,6 @@ static int gen8_append_oa_reports(struct i915_perf_stream *stream, > u32 mask = (OA_BUFFER_SIZE - 1); > size_t start_offset = *offset; > unsigned long flags; > - unsigned int aged_tail_idx; > u32 head, tail; > u32 taken; > int ret = 0; > @@ -682,18 +659,10 @@ static int gen8_append_oa_reports(struct i915_perf_stream *stream, > spin_lock_irqsave(&stream->oa_buffer.ptr_lock, flags); > > head = stream->oa_buffer.head; > - aged_tail_idx = stream->oa_buffer.aged_tail_idx; > - tail = stream->oa_buffer.tails[aged_tail_idx].offset; > + tail = stream->oa_buffer.tail; > > spin_unlock_irqrestore(&stream->oa_buffer.ptr_lock, flags); > > - /* > - * An invalid tail pointer here means we're still waiting for the poll > - * hrtimer callback to give us a pointer > - */ > - if (tail == INVALID_TAIL_PTR) > - return -EAGAIN; > - > /* > * NB: oa_buffer.head/tail include the gtt_offset which we don't want > * while indexing relative to oa_buf_base. > @@ -827,13 +796,11 @@ static int gen8_append_oa_reports(struct i915_perf_stream *stream, > } > > /* > - * The above reason field sanity check is based on > - * the assumption that the OA buffer is initially > - * zeroed and we reset the field after copying so the > - * check is still meaningful once old reports start > - * being overwritten. > + * Clear out the first 2 dword as a mean to detect unlanded > + * reports. > */ > report32[0] = 0; > + report32[1] = 0; > } > > if (start_offset != *offset) { > @@ -974,7 +941,6 @@ static int gen7_append_oa_reports(struct i915_perf_stream *stream, > u32 mask = (OA_BUFFER_SIZE - 1); > size_t start_offset = *offset; > unsigned long flags; > - unsigned int aged_tail_idx; > u32 head, tail; > u32 taken; > int ret = 0; > @@ -985,17 +951,10 @@ static int gen7_append_oa_reports(struct i915_perf_stream *stream, > spin_lock_irqsave(&stream->oa_buffer.ptr_lock, flags); > > head = stream->oa_buffer.head; > - aged_tail_idx = stream->oa_buffer.aged_tail_idx; > - tail = stream->oa_buffer.tails[aged_tail_idx].offset; > + tail = stream->oa_buffer.tail; > > spin_unlock_irqrestore(&stream->oa_buffer.ptr_lock, flags); > > - /* An invalid tail pointer here means we're still waiting for the poll > - * hrtimer callback to give us a pointer > - */ > - if (tail == INVALID_TAIL_PTR) > - return -EAGAIN; > - > /* NB: oa_buffer.head/tail include the gtt_offset which we don't want > * while indexing relative to oa_buf_base. > */ > @@ -1053,13 +1012,11 @@ static int gen7_append_oa_reports(struct i915_perf_stream *stream, > if (ret) > break; > > - /* The above report-id field sanity check is based on > - * the assumption that the OA buffer is initially > - * zeroed and we reset the field after copying so the > - * check is still meaningful once old reports start > - * being overwritten. > + /* Clear out the first 2 dwords as a mean to detect unlanded > + * reports. > */ > report32[0] = 0; > + report32[1] = 0; > } > > if (start_offset != *offset) { > @@ -1438,8 +1395,8 @@ static void gen7_init_oa_buffer(struct i915_perf_stream *stream) > gtt_offset | OABUFFER_SIZE_16M); > > /* Mark that we need updated tail pointers to read from... */ > - stream->oa_buffer.tails[0].offset = INVALID_TAIL_PTR; > - stream->oa_buffer.tails[1].offset = INVALID_TAIL_PTR; > + stream->oa_buffer.aging_tail = INVALID_TAIL_PTR; > + stream->oa_buffer.tail = gtt_offset; > > spin_unlock_irqrestore(&stream->oa_buffer.ptr_lock, flags); > > @@ -1492,8 +1449,8 @@ static void gen8_init_oa_buffer(struct i915_perf_stream *stream) > intel_uncore_write(uncore, GEN8_OATAILPTR, gtt_offset & GEN8_OATAILPTR_MASK); > > /* Mark that we need updated tail pointers to read from... */ > - stream->oa_buffer.tails[0].offset = INVALID_TAIL_PTR; > - stream->oa_buffer.tails[1].offset = INVALID_TAIL_PTR; > + stream->oa_buffer.aging_tail = INVALID_TAIL_PTR; > + stream->oa_buffer.tail = gtt_offset; > > /* > * Reset state used to recognise context switches, affecting which > @@ -1548,8 +1505,8 @@ static void gen12_init_oa_buffer(struct i915_perf_stream *stream) > gtt_offset & GEN12_OAG_OATAILPTR_MASK); > > /* Mark that we need updated tail pointers to read from... */ > - stream->oa_buffer.tails[0].offset = INVALID_TAIL_PTR; > - stream->oa_buffer.tails[1].offset = INVALID_TAIL_PTR; > + stream->oa_buffer.aging_tail = INVALID_TAIL_PTR; > + stream->oa_buffer.tail = gtt_offset; > > /* > * Reset state used to recognise context switches, affecting which > @@ -4398,6 +4355,11 @@ void i915_perf_init(struct drm_i915_private *i915) > ratelimit_set_flags(&perf->spurious_report_rs, > RATELIMIT_MSG_ON_RELEASE); > > + ratelimit_state_init(&perf->tail_pointer_race, > + 5 * HZ, 10); > + ratelimit_set_flags(&perf->tail_pointer_race, > + RATELIMIT_MSG_ON_RELEASE); > + > atomic64_set(&perf->noa_programming_delay, > 500 * 1000 /* 500us */); > > diff --git a/drivers/gpu/drm/i915/i915_perf_types.h b/drivers/gpu/drm/i915/i915_perf_types.h > index 32289cbda648..c3ab184c604a 100644 > --- a/drivers/gpu/drm/i915/i915_perf_types.h > +++ b/drivers/gpu/drm/i915/i915_perf_types.h > @@ -273,21 +273,10 @@ struct i915_perf_stream { > spinlock_t ptr_lock; > > /** > - * @tails: One 'aging' tail pointer and one 'aged' tail pointer ready to > - * used for reading. > - * > - * Initial values of 0xffffffff are invalid and imply that an > - * update is required (and should be ignored by an attempted > - * read) > + * @aging_tail: The last HW tail reported by HW. The data > + * might not have made it to memory yet though. > */ > - struct { > - u32 offset; > - } tails[2]; > - > - /** > - * @aged_tail_idx: Index for the aged tail ready to read() data up to. > - */ > - unsigned int aged_tail_idx; > + u32 aging_tail; > > /** > * @aging_timestamp: A monotonic timestamp for when the current aging tail pointer > @@ -303,6 +292,11 @@ struct i915_perf_stream { > * OA buffer data to userspace. > */ > u32 head; > + > + /** > + * @tail: The last verified tail that can be read by userspace. > + */ > + u32 tail; > } oa_buffer; > > /** > @@ -420,6 +414,12 @@ struct i915_perf { > */ > struct ratelimit_state spurious_report_rs; > > + /** > + * For rate limiting any notifications of tail pointer > + * race. > + */ > + struct ratelimit_state tail_pointer_race; > + > u32 gen7_latched_oastatus1; > u32 ctx_oactxctrl_offset; > u32 ctx_flexeu0_offset; > -- > 2.20.1 > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2020-03-24 21:25 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-19 22:52 [Intel-gfx] [PATCH 0/3] drm/i915/perf: add OA interrupt support Umesh Nerlige Ramappa 2020-03-19 22:52 ` [Intel-gfx] [PATCH 1/3] drm/i915/perf: rework aging tail workaround Umesh Nerlige Ramappa 2020-03-21 23:26 ` Dixit, Ashutosh 2020-03-22 4:44 ` Dixit, Ashutosh 2020-03-22 19:47 ` Dixit, Ashutosh 2020-03-24 3:17 ` Umesh Nerlige Ramappa 2020-03-24 3:15 ` Umesh Nerlige Ramappa 2020-03-19 22:52 ` [Intel-gfx] [PATCH 2/3] drm/i915/perf: move pollin setup to non hw specific code Umesh Nerlige Ramappa 2020-03-19 22:52 ` [Intel-gfx] [PATCH 3/3] drm/i915/perf: add new open param to configure polling of OA buffer Umesh Nerlige Ramappa 2020-03-21 7:16 ` Dixit, Ashutosh 2020-03-19 23:40 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915/perf: add OA interrupt support (rev7) Patchwork 2020-03-19 23:43 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork 2020-03-20 0:04 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork 2020-03-24 18:54 [Intel-gfx] [PATCH 0/3] drm/i915/perf: add OA interrupt support Umesh Nerlige Ramappa 2020-03-24 18:54 ` [Intel-gfx] [PATCH 1/3] drm/i915/perf: rework aging tail workaround Umesh Nerlige Ramappa 2020-03-24 21:25 ` Dixit, Ashutosh
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.