[igt-dev] [PATCH i-g-t] tests/kms_concurrent: For i915 devices run allocator in multiprocess mode

[Petri Latvala]

On Fri, May 20, 2022 at 12:45:46PM +0300, Coelho, Luciano wrote:
> On Fri, 2022-05-20 at 11:50 +0300, Petri Latvala wrote:
> > On Fri, May 20, 2022 at 09:11:18AM +0200, Zbigniew Kempczyński wrote:
> > > On Fri, May 20, 2022 at 08:56:29AM +0300, Petri Latvala wrote:
> > > > On Thu, May 19, 2022 at 04:17:45PM +0200, Zbigniew Kempczyński wrote:
> > > > > Test calls igt_fork() so for i915 requires offset allocation arbitration
> > > > > (allocator in multiprocess mode) especially when same drm fd is used
> > > > > in children. Dedicated thread (intel_allocator_multiprocess_start())
> > > > > is required to be started on the very beginning to handle offset
> > > > > allocations as well as stopping it (intel_allocator_multiprocess_stop())
> > > > > before test exits.
> > > > >
> > > > > Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
> > > > > Cc: Luciano Coelho <luciano.coelho at intel.com>
> > > > > Cc: Swati Sharma <swati2.sharma at intel.com>
> > > >
> > > > Acked-by: Petri Latvala <petri.latvala at intel.com>
> > > >
> > > > Now, having said that:
> > > >
> > > > The test itself doesn't touch the allocator at all, it's done by
> > > > igt_fb.c's blitcopy() under the hood. Did you check if there's any
> > > > other tests that could have this problem? Suggesting a common way to
> > > > have tests communicate their intention to fork is overkill at this
> > > > point, unless the problem is widespread.
> > >
> > > Take a look to kms_busy.c function flip_to_fb.c. There's igt_fork()
> > > but we don't need to run additional allocator thread.
> > >
> > > I think we can try:
> > > 1. run multiprocess allocator thread on all igt run but this is imo
> > >    overkill, especially if test works on separate vm's it can "detach"
> > >    and become standalone allocator,
> > > 2. add some debug information instead of segfaulting that multiprocess
> > >    allocator is likely required in the test. IGT already got similar
> > >    code (like internal_assert in igt_describe_f()).
> >
> > Debug information sounds really good. It's easy to detect the crashing
> > case in the allocator code after all, from what I remember about the
> > backtrace.
> 
> I'm not familiar with any of this, but it seems that checking for a
> NULL-pointer in msgqueue_send_req() and returning an error would be
> super-simple, since it already igt_warn()s on errors.  That should
> obviously be done in other places where needed, but seems to be pretty
> straight-forward to me.
> 
> This would obviously not fix the problem of not initializing this in
> certain tests, but it would at least make it pretty clear what is going
> on at first glance.

Yeah, exactly. Print "multiprocess_start should have been called, this
is a test bug" in there and let bug reports speak for themselves.


-- 
Petri Latvala