[igt-dev] [PATCH i-g-t] i915/perf: Make __perf_open() and friends public

Thu Feb 9 09:56:04 UTC 2023

On Wednesday, 8 February 2023 20:34:06 CET Umesh Nerlige Ramappa wrote:
> On Wed, Feb 08, 2023 at 11:09:23AM +0100, Janusz Krzysztofik wrote:
> >On Tuesday, 7 February 2023 21:15:26 CET Dixit, Ashutosh wrote:
> >> On Tue, 07 Feb 2023 12:04:17 -0800, Janusz Krzysztofik wrote:
> >> >
> >> > Hi Umesh,
> >> >
> >> > On Tuesday, 7 February 2023 20:33:50 CET Umesh Nerlige Ramappa wrote:
> >> > > On Tue, Feb 07, 2023 at 11:25:00AM -0800, Umesh Nerlige Ramappa 
wrote:
> >> > > >I wouldn't do this. Please keep the changes local to the specific 
test
> >> > > >that you implemented in your first rev. While it is a good idea to
> >> > > >have the some of the perf capabilities in the library, this is way 
too
> >> > > >much churn to implement a specific test for the original failure.
> >> > > >Unless multiple IGT subsytems area already dependent on perf APIs to
> >> > > >implement multiple tests, let's not do this.
> >> > > >
> >> > >
> >> > > Also note that the perf library implemented in IGT is not entirely 
used
> >> > > by IGT tests alone. The library is also linked to GPUvis software. 
Only
> >> > > a few pieces of reusable code in the perf library is used by IGT 
tests.
> >> >
> >> > Do you think that my changes will break other users?  How?
> >> >
> >> > Also, it looks like there are somehow conflicting expectations from 
different
> >> > reviewers.  Ashutosh wanted the new subtest to be implemented outside 
of i915/
> >> > perf test.  That's why I proposed to extend the library with open/close 
and
> >> > related helpers, just to avoid code duplication, and I'm about to 
resend it in
> >> > series with the new subtest implemented inside gem_ctx_exec.  Now, 
after I
> >> > submitted this patch for initial review, you say that a specific test 
is not
> >> > the way to go.  What are you afraid of?
> >> >
> >> > Whose expectations should I try to satisfy in order to have a subtest 
accepted
> >> > and merged?  Or should I just give up and duplicate the code from i915/
perf in
> >> > another test?  Or maybe you can have a look at the whole series before 
you
> >> > decide?
> >>
> >> Hi Janusz,
> >>
> >> I agree with Umesh. Given that here perf is just being used as a 'dummy
> >> workload' let's just duplicate the minimal code required for perf
> >> open/close wherever we are adding the new test. This will keep the real
> >> perf functionality undisturbed for reasons Umesh cited.
> >
> >TBH, I can't see any good justification among those reasons mentioned: "too
> >much churn", "unless ... already dependent", "not entirely used by IGT 
tests",
> >"linked to GPUvis software", "only a few pieces of reusable code ... used 
by
> >IGT" -- which of those justifies duplication of i915 perf code in IGT 
tests?
> 
> You yourself mentioned that this is not related to perf. It's just that 
> perf uses some code in i915 that does barrier related stuff which helps 
> you to reproduce the issue. Why can't that barrier-related-stuff be 
> implemented in IGT without the use of perf APIs?

Because inside the driver I found no ways to trigger the issue (within a time 
period reasonable from CI perspective) other than calling
intel_context_prepare_remote_request().  Only perf and gen8 sseu call that 
function.  Out of the two, perf was my choice because:
- perf matched the user scenario reported as the one that could trigger the 
  bug,
- we already had some work in progress subtest added to tests/i915/perf.c 
  still before my root cause analysis was completed.

> If that's a lot of 
> effort and it's quicker to reproduce this issue using perf APIs, then 
> that's fine with me, but keep it outside of perf library 

Why?
(assuming by "it" you mean some useful functions now in tests/i915/perf.c)

> and maybe add a 
> note saying that this test can be improved by figuring out how to do 
> barrier related execution in IGT in future. 

I don't understand what you mean by "in IGT" here.  Isn't lib/i915/perf.c "in 
IGT" (a part of IGT)?

> I don't see any 
> justification to modify perf library for an issue that's not even perf 
> related.

Not modify, only extend with a wrapper around DRM_IOCTL_I915_PERF_OPEN and 
helpers it depends on.

Justification why we need to call DRM_IOCTL_I915_PERF_OPEN from some new 
subtests: extend CI coverage over some rarely used processing paths.

Justification why add reusable code to a library: avoid code duplication.

Justification why add it to perf library: no doubt DRM_IOCTL_I915_PERF_OPEN is 
perf related, I believe.

> I believe you also mentioned somewhere that the issue was fixed by some 
> 'unknown' code changes to i915 and you are not able to reproduce
> it consistently with this test now.

No, I must have missed my point while clarifying things if that's how you've 
read them, sorry.  The issue is reproducible.  CI results from my trybot 
attempt clearly confirm that:
https://intel-gfx-ci.01.org/tree/drm-tip/TrybotIGT_699/bat-all.html?
testfilter=barrier-race

Results from preliminary fixes tested with the new IGT subtest on trybot:
https://intel-gfx-ci.01.org/tree/drm-tip/Trybot_113662v1/bat-all.html?
testfilter=barrier-race

> 
> I have considered the above factors to suggest that this should not 
> reside in perf library.
> 
> >Again, do you think that my changes can break other (non-IGT) users?  
> >How?
> 
> I didn't say that your changes break anything. I was stating that the 
> library code is shared across different tools.

But why do you think that an IGT library shared across different tools can't 
be extended with a few functions needed by IGT tests?

Thanks,
Janusz

> 
> Thanks,
> Umesh
> 
> >
> >Anyway, assuming you are the "owner" of lib/i915/perf.c, in order to 
satisfy
> >your (still not clear for me) requirements I'm already working on a new
> >version of my patch, with the i915 perf code duplicated as needed.
> >
> >Thanks,
> >Janusz
> >
> >>
> >> Thanks.
> >> --
> >> Ashutosh
> >>
> >
> >
> >
> >
> 