[PATCH i-g-t v6 16/17] tests/xe_eudebug_online: Debug client which runs workloads on EU

[Zbigniew Kempczyński]

On Tue, Sep 17, 2024 at 09:34:20PM +0200, Grzegorzek, Dominik wrote:

<cut>

> > > +/**
> > > + * SUBTEST: preempt-breakpoint
> > > + * Description:
> > > + *	Verify that eu debugger disables preemption timeout to
> > > + *	prevent reset of workload stopped on breakpoint.
> > > + */
> > > +static void test_preemption(int fd, struct drm_xe_engine_class_instance *hwe)
> > > +{
> > > +	int flags = SHADER_BREAKPOINT | TRIGGER_RESUME_DELAYED;
> > > +	struct xe_eudebug_session *s;
> > > +	struct online_debug_data *data;
> > > +	struct xe_eudebug_client *other;
> > > +
> > > +	data = online_debug_data_create(hwe);
> > > +	s = xe_eudebug_session_create(fd, run_online_client, flags, data);
> > > +	other = xe_eudebug_client_create(fd, run_online_client, SHADER_NOP, data);
> > > +
> > > +	xe_eudebug_debugger_add_trigger(s->debugger, DRM_XE_EUDEBUG_EVENT_EU_ATTENTION,
> > > +					eu_attention_debug_trigger);
> > > +	xe_eudebug_debugger_add_trigger(s->debugger, DRM_XE_EUDEBUG_EVENT_EU_ATTENTION,
> > > +					eu_attention_resume_trigger);
> > > +	xe_eudebug_debugger_add_trigger(s->debugger, DRM_XE_EUDEBUG_EVENT_VM_BIND_UFENCE,
> > > +					ufence_ack_trigger);
> > > +
> > > +	igt_assert_eq(xe_eudebug_debugger_attach(s->debugger, s->client), 0);
> > > +	xe_eudebug_debugger_start_worker(s->debugger);
> > > +
> > > +	xe_eudebug_client_start(s->client);
> > > +	sleep(1); /* make sure s->client starts first */
> > 
> > If client would write token it has started this sleep wouldn't be
> > necessary. I mean inside xe_eudebug_client_start() do
> > token_signal/wait_for_client.
> To ensure that the first client executes its workload first, we would need to signal it after
> calling xe_exec. This means that the signaling would need to be incorporated within the client's
> work function. We cannot place wait_for_client inside the generic xe_eudebug_client_start() because
> the work function is defined by the caller. While I could implement a similar mechanism specifically
> for this test, it would require creating a brand new run_online_client()-like function or adding
> wait_for_client in every test that reuses run_online_client(). I decided to keep it simple, albeit
> imperfect. Let me know if you would rather have it changed. 

Ok, now I understand. That's not the client process start is interesting from
our point of view (that's why I suggested to inform caller that is created)
but xe_exec() itself. Signalling to the spawner xe_exec() was called is not
so easy with current code shape. sleep(1) is however long time and likely
first client will start its execution before 'other' (I'm not sure shouldn't
it be called 'another', but that's not important).

Keep as it is now. If we encounter the race win by 'other' then we'll think
how to prevent this.

--

Zbigniew