| Message ID | 20211021234044.3071069-3-John.C.Harrison@Intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fixes for gem_exec_capture | expand |
On Thu, Oct 21, 2021 at 04:40:38PM -0700, John.C.Harrison@Intel.com wrote: > From: John Harrison <John.C.Harrison@Intel.com> > > At some point, larger than 4KB page sizes were added to the i915 > driver. This included adding an informational line to the buffer > entries in error capture logs. However, the error capture test was not > updated to skip this string, thus it would silently abort processing. > > Signed-off-by: John Harrison <John.C.Harrison@Intel.com> > --- > tests/i915/gem_exec_capture.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/tests/i915/gem_exec_capture.c b/tests/i915/gem_exec_capture.c > index 53649cdb2..47ca64dd6 100644 > --- a/tests/i915/gem_exec_capture.c > +++ b/tests/i915/gem_exec_capture.c > @@ -484,6 +484,12 @@ static void many(int fd, int dir, uint64_t size, unsigned int flags) > addr |= strtoul(str + 1, &str, 16); > igt_assert(*str++ == '\n'); > > + /* gtt_page_sizes = 0x00010000 */ > + if (strncmp(str, "gtt_page_sizes = 0x", 19) == 0) { > + str += 19 + 8; > + igt_assert(*str++ == '\n'); > + } Can you explain this logic to me, for the life of me I can't figure out what this doing. That probably warrent's a more detailed comment too. Matt > + > if (!(*str == ':' || *str == '~')) > continue; > > -- > 2.25.1 >
On 10/29/2021 10:39, Matthew Brost wrote: > On Thu, Oct 21, 2021 at 04:40:38PM -0700, John.C.Harrison@Intel.com wrote: >> From: John Harrison <John.C.Harrison@Intel.com> >> >> At some point, larger than 4KB page sizes were added to the i915 >> driver. This included adding an informational line to the buffer >> entries in error capture logs. However, the error capture test was not >> updated to skip this string, thus it would silently abort processing. >> >> Signed-off-by: John Harrison <John.C.Harrison@Intel.com> >> --- >> tests/i915/gem_exec_capture.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/tests/i915/gem_exec_capture.c b/tests/i915/gem_exec_capture.c >> index 53649cdb2..47ca64dd6 100644 >> --- a/tests/i915/gem_exec_capture.c >> +++ b/tests/i915/gem_exec_capture.c >> @@ -484,6 +484,12 @@ static void many(int fd, int dir, uint64_t size, unsigned int flags) >> addr |= strtoul(str + 1, &str, 16); >> igt_assert(*str++ == '\n'); >> >> + /* gtt_page_sizes = 0x00010000 */ >> + if (strncmp(str, "gtt_page_sizes = 0x", 19) == 0) { >> + str += 19 + 8; >> + igt_assert(*str++ == '\n'); >> + } > Can you explain this logic to me, for the life of me I can't figure out > what this doing. That probably warrent's a more detailed comment too. It's no different to the rest of the processing that this code was already doing. if( start_of_current_line == "gtt_page_sizes = 0x") { current_line += strlen(above_string) + strlen(8-digit hex string); assert( next_character_of_current_line == end_of_line); } I.e. skip over any line that just contains the page size message. John. > > Matt > >> + >> if (!(*str == ':' || *str == '~')) >> continue; >> >> -- >> 2.25.1 >>
On Fri, Oct 29, 2021 at 05:32:40PM -0700, John Harrison wrote: > On 10/29/2021 10:39, Matthew Brost wrote: > > On Thu, Oct 21, 2021 at 04:40:38PM -0700, John.C.Harrison@Intel.com wrote: > > > From: John Harrison <John.C.Harrison@Intel.com> > > > > > > At some point, larger than 4KB page sizes were added to the i915 > > > driver. This included adding an informational line to the buffer > > > entries in error capture logs. However, the error capture test was not > > > updated to skip this string, thus it would silently abort processing. > > > > > > Signed-off-by: John Harrison <John.C.Harrison@Intel.com> > > > --- > > > tests/i915/gem_exec_capture.c | 6 ++++++ > > > 1 file changed, 6 insertions(+) > > > > > > diff --git a/tests/i915/gem_exec_capture.c b/tests/i915/gem_exec_capture.c > > > index 53649cdb2..47ca64dd6 100644 > > > --- a/tests/i915/gem_exec_capture.c > > > +++ b/tests/i915/gem_exec_capture.c > > > @@ -484,6 +484,12 @@ static void many(int fd, int dir, uint64_t size, unsigned int flags) > > > addr |= strtoul(str + 1, &str, 16); > > > igt_assert(*str++ == '\n'); > > > + /* gtt_page_sizes = 0x00010000 */ > > > + if (strncmp(str, "gtt_page_sizes = 0x", 19) == 0) { > > > + str += 19 + 8; > > > + igt_assert(*str++ == '\n'); > > > + } > > Can you explain this logic to me, for the life of me I can't figure out > > what this doing. That probably warrent's a more detailed comment too. > It's no different to the rest of the processing that this code was already > doing. > > if( start_of_current_line == "gtt_page_sizes = 0x") { > current_line += strlen(above_string) + strlen(8-digit hex string); > assert( next_character_of_current_line == end_of_line); > } > > I.e. skip over any line that just contains the page size message. > Ok, got it. Not sure I missed that. The magic numbers 19 and 8 where confusing me but I understand this now. With that: Reviewed-by: Matthew Brost <matthew.brost@intel.com> > John. > > > > > Matt > > > > > + > > > if (!(*str == ':' || *str == '~')) > > > continue; > > > -- > > > 2.25.1 > > > >
diff --git a/tests/i915/gem_exec_capture.c b/tests/i915/gem_exec_capture.c index 53649cdb2..47ca64dd6 100644 --- a/tests/i915/gem_exec_capture.c +++ b/tests/i915/gem_exec_capture.c @@ -484,6 +484,12 @@ static void many(int fd, int dir, uint64_t size, unsigned int flags) addr |= strtoul(str + 1, &str, 16); igt_assert(*str++ == '\n'); + /* gtt_page_sizes = 0x00010000 */ + if (strncmp(str, "gtt_page_sizes = 0x", 19) == 0) { + str += 19 + 8; + igt_assert(*str++ == '\n'); + } + if (!(*str == ':' || *str == '~')) continue;