[PATCH i-g-t] tests/xe_exec_threads: Fill in GT field for second balancer thread
Lucas De Marchi
lucas.demarchi at intel.com
Thu Mar 21 06:01:22 UTC 2024
On Thu, Mar 21, 2024 at 01:04:37AM +0000, Matthew Brost wrote:
>On Wed, Mar 20, 2024 at 03:47:57PM -0500, Lucas De Marchi wrote:
>> On Wed, Mar 20, 2024 at 12:23:56PM -0700, Matt Roper wrote:
>> > The balancer subtests spawn two pthreads per engine class if there are
>> > multiple instances of the class. The GT field of the data structure is
>> > filled in properly for the first thread, but not for the second,
>> > effectively leaving it set to "0."
>> >
>> > For platforms with standalone media, this will result in failures when
>> > the thread tries to find the instances of a media class on GT0 and trips
>> > the "igt_assert(num_placements > 1)" assertion in test_balancer().
>> >
>> > Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
>>
>>
>> Reviewed-by: Lucas De Marchi <lucas.demarchi at intel.com>
>>
>> do you know why we unrolled a loop in there and did a wrong copy and
>
>I think the for_gt loop was added after the original version of this.
>Likely an ommision when that was added.
>
>> paste? That together with vertical spacing on flow control seems to be a
>> good source of bugs. Good find.
>>
>> The only difference between the first and second thread data I'm seeing
>> is flags VIRTUAL vs PARALLEL. Matt Brost, is this test from you? Could
>
>Yes, I wrote this one. Not my finest work in terms of copy / pasting,
>indeed the only difference should be VIRTUAL vs. PARALLEL. I think this
>should be consolidated into a loop.
sounds good. Something like this on top of this patch? (completely untested, it builds)
diff --git a/tests/intel/xe_exec_threads.c b/tests/intel/xe_exec_threads.c
index 977e8c600..b95870714 100644
--- a/tests/intel/xe_exec_threads.c
+++ b/tests/intel/xe_exec_threads.c
@@ -1030,6 +1030,7 @@ static void threads(int fd, int flags)
xe_for_each_gt(fd, gt)
xe_for_each_engine_class(class) {
int num_placements = 0;
+ int *data_flags = (int[]){ VIRTUAL, PARALLEL, -1 };
xe_for_each_engine(fd, hwe) {
if (hwe->engine_class != class ||
@@ -1038,36 +1039,10 @@ static void threads(int fd, int flags)
++num_placements;
}
- if (num_placements > 1) {
- threads_data[i].mutex = &mutex;
- threads_data[i].cond = &cond;
- if (flags & SHARED_VM)
- threads_data[i].addr = addr |
- (i << ADDRESS_SHIFT);
- else
- threads_data[i].addr = addr;
- threads_data[i].userptr = userptr |
- (i << ADDRESS_SHIFT);
- if (flags & FD)
- threads_data[i].fd = 0;
- else
- threads_data[i].fd = fd;
- threads_data[i].gt = gt;
- threads_data[i].vm_legacy_mode =
- vm_legacy_mode;
- threads_data[i].class = class;
- threads_data[i].n_exec_queue = N_EXEC_QUEUE;
- threads_data[i].n_exec = N_EXEC;
- threads_data[i].flags = flags;
- threads_data[i].flags &= ~BALANCER;
- threads_data[i].flags |= VIRTUAL;
- threads_data[i].go = &go;
-
- ++n_threads;
- pthread_create(&threads_data[i].thread, 0,
- thread, &threads_data[i]);
- ++i;
+ if (num_placements <= 1)
+ continue;
+ while (*data_flags >= 0) {
threads_data[i].mutex = &mutex;
threads_data[i].cond = &cond;
if (flags & SHARED_VM)
@@ -1089,13 +1064,14 @@ static void threads(int fd, int flags)
threads_data[i].n_exec = N_EXEC;
threads_data[i].flags = flags;
threads_data[i].flags &= ~BALANCER;
- threads_data[i].flags |= PARALLEL;
+ threads_data[i].flags |= *data_flags;
threads_data[i].go = &go;
++n_threads;
pthread_create(&threads_data[i].thread, 0,
thread, &threads_data[i]);
++i;
+ data_flags++;
}
}
}
>
>> you add an overview to its documentation? The boilerplate doc added
>> there after the fact looks less than helpful.
>>
>
>What kinda of documentation were you thinking of? Most all the IGTs are
>light on doc but updating the doc for ones I wrote is on my todo list...
It would be good to have an overview on what the test is checking and
how the different subtests are different from each other.
thanks
Lucas De Marchi
>
>Matt
>
>> Lucas De Marchi
>>
>> > ---
>> > tests/intel/xe_exec_threads.c | 1 +
>> > 1 file changed, 1 insertion(+)
>> >
>> > diff --git a/tests/intel/xe_exec_threads.c b/tests/intel/xe_exec_threads.c
>> > index 55907e2b3..977e8c600 100644
>> > --- a/tests/intel/xe_exec_threads.c
>> > +++ b/tests/intel/xe_exec_threads.c
>> > @@ -1081,6 +1081,7 @@ static void threads(int fd, int flags)
>> > threads_data[i].fd = 0;
>> > else
>> > threads_data[i].fd = fd;
>> > + threads_data[i].gt = gt;
>> > threads_data[i].vm_legacy_mode =
>> > vm_legacy_mode;
>> > threads_data[i].class = class;
>> > --
>> > 2.43.0
>> >
More information about the igt-dev
mailing list