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

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

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.

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