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

[Umesh Nerlige Ramappa]

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? 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 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 see any 
justification to modify perf library for an issue that's not even perf 
related.

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.

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.

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
>>
>
>
>
>