Re: [PATCH 2/3] drm/xe: Exec queue op's to enable/disable preemption and timeslicing

[Matthew Brost <matthew.brost@intel.com>]

On Tue, Jun 25, 2024 at 05:24:59PM -0700, Dixit, Ashutosh wrote:
> On Tue, 25 Jun 2024 15:01:16 -0700, Matthew Brost wrote:
> >
> > On Tue, Jun 25, 2024 at 02:57:15PM -0700, Umesh Nerlige Ramappa wrote:
> > > On Tue, Jun 25, 2024 at 01:15:17PM -0700, Ashutosh Dixit wrote:
> > > > +static int guc_exec_queue_set_no_preempt(struct xe_exec_queue *q)
> > > > +{
> > > > +	struct xe_sched_msg *msg;
> > > > +
> > > > +	if ((!q->sched_props.preempt_timeout_us && !q->sched_props.timeslice_us) ||
> > > > +	    exec_queue_killed_or_banned_or_wedged(q))
> > > > +		return 0;
> > > > +
> > > > +	msg = kmalloc(sizeof(*msg), GFP_KERNEL);
> > > > +	if (!msg)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	/* Setting values to 0 will disable preemption and timeslicing */
> > > > +	q->sched_props.preempt_timeout_us = 0;
> > > > +	q->sched_props.timeslice_us = 0;
> > > > +
> > > > +	guc_exec_queue_add_msg(q, msg, SET_SCHED_PROPS);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int guc_exec_queue_clear_no_preempt(struct xe_exec_queue *q)
> > > > +{
> > > > +	struct xe_sched_msg *msg;
> > > > +
> > > > +	if ((q->sched_props.preempt_timeout_us ==
> > > > +	    q->hwe->eclass->sched_props.preempt_timeout_us &&
> > > > +	    q->sched_props.timeslice_us == q->hwe->eclass->sched_props.timeslice_us) ||
> > > > +	    exec_queue_killed_or_banned_or_wedged(q))
> > > > +		return 0;
> > > > +
> > > > +	msg = kmalloc(sizeof(*msg), GFP_KERNEL);
> > > > +	if (!msg)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	q->sched_props.preempt_timeout_us = q->hwe->eclass->sched_props.preempt_timeout_us;
> > > > +	q->sched_props.timeslice_us = q->hwe->eclass->sched_props.timeslice_us;
> > > > +
> > > > +	guc_exec_queue_add_msg(q, msg, SET_SCHED_PROPS);
> > > > +
> > > > +	return 0;
> > > > +}
> > >
> > > Why not just use the .set_timeslice and .set_preempt_timeout hooks instead
> > > of defining a new one to do both?
> > >
> > Umesh's thinking seems correct.
> >
> > Just use the existing set_timeslice & set_preempt_timeout hooks with 0
> > for disable and q->hwe->eclass->sched_props.timeslice_us for enable.
> 
> This was the approach I had taken in v1, I just resurrected v1 as v4 and
> sent it out.
> 
> > > Also how do you check if this operation succeeeded? Is there a response from
> > > GuC indicating success?
> > >
> 
> .set_timeslice and .set_preempt_timeout have an error return. But otherwise
> I don't know what happens after these ops call
> guc_exec_queue_add_msg(). Maybe Matt Brost knows? Matt, is this operation
> guaranteed to always succeed? Since there is no return code from these ops
> except -ENOMEM.

The only failure point is the kmalloc.

Once the op is submitted via guc_exec_queue_add_msg it cannot fail. The
H2G which communicates the change can get lost but in that case
something will trigger a GT reset and upon next register of the exec
queue the correct value for timeslice / preempt_timeout will be setup.

Matt

> 
> Thanks.
> --
> Ashutosh
> 
> 
> >
> > Matt
> >
> > > Thanks,
> > > Umesh
> > >
> > > > +
> > > > static int guc_exec_queue_suspend(struct xe_exec_queue *q)
> > > > {
> > > >	struct xe_sched_msg *msg = q->guc->static_msgs + STATIC_MSG_SUSPEND;
> > > > @@ -1598,6 +1641,8 @@ static const struct xe_exec_queue_ops guc_exec_queue_ops = {
> > > >	.set_priority = guc_exec_queue_set_priority,
> > > >	.set_timeslice = guc_exec_queue_set_timeslice,
> > > >	.set_preempt_timeout = guc_exec_queue_set_preempt_timeout,
> > > > +	.set_no_preempt = guc_exec_queue_set_no_preempt,
> > > > +	.clear_no_preempt = guc_exec_queue_clear_no_preempt,
> > > >	.suspend = guc_exec_queue_suspend,
> > > >	.suspend_wait = guc_exec_queue_suspend_wait,
> > > >	.resume = guc_exec_queue_resume,
> > > > --
> > > > 2.41.0
> > > >