Re: [PATCH v6 10/13] drm/xe/guc_submit: Allow calling guc_exec_queue_resume with pending resume

[Francois Dugast <francois.dugast@intel.com>]

On Thu, Aug 08, 2024 at 03:44:17AM +0000, Matthew Brost wrote:
> On Tue, Aug 06, 2024 at 09:27:08PM +0200, Francois Dugast wrote:
> > Make it safe to have multiple calls to guc_exec_queue_resume() for the
> > same exec queue. This allows the use of this function from a context where
> > knowledge about prior calls is missing. In this case the function does
> > nothing but at least it does not fail on assert.
> > 
> > Signed-off-by: Francois Dugast <francois.dugast@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_guc_submit.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> > index 50013e1e7455..782561b21d24 100644
> > --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> > +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> > @@ -1623,10 +1623,9 @@ static int guc_exec_queue_suspend_wait(struct xe_exec_queue *q)
> >  static void guc_exec_queue_resume(struct xe_exec_queue *q)
> >  {
> >  	struct xe_sched_msg *msg = q->guc->static_msgs + STATIC_MSG_RESUME;
> > -	struct xe_guc *guc = exec_queue_to_guc(q);
> > -	struct xe_device *xe = guc_to_xe(guc);
> >  
> > -	xe_assert(xe, !q->guc->suspend_pending);
> 
> I think the existing assert is correct. We certainly don't want to call
> resume until suspend_wait returns (sets q->guc->suspend_pending ==
> false). 

In the end this was a workaround for this issue [1] which is now fixed
so I will drop this patch in the next revision.

Francois

[1] https://patchwork.freedesktop.org/patch/606817/?series=136192&rev=5#comment_1103293

> 
> I think if we want to make this safe to be called multiple times we need
> a new function 'guc_exec_queue_try_add_msg'. This function would check the
> msg->link link under lock for empty and if empty add the message. It is
> harmless for __guc_exec_queue_process_msg_resume to be called multple
> times, it is not harmless to use the list twice.
> 
> Matt
> 
> > +	if (q->guc->suspend_pending)
> > +		return;
> >  
> >  	guc_exec_queue_add_msg(q, msg, RESUME);
> >  }
> > -- 
> > 2.43.0
> >