Re: [PATCH 4/8] drm/xe/oa: Add input fence dependencies

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

On Thu, 08 Aug 2024 16:25:31 -0700, Matthew Brost wrote:
>

Hi Matt,

> On Thu, Aug 08, 2024 at 10:41:35AM -0700, Ashutosh Dixit wrote:
> > Add input fence dependencies which will make OA configuration wait till
> > these dependencies are met (till input fences signal).
> >
> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_oa.c | 24 +++++++++++++++++++-----
> >  1 file changed, 19 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
> > index ba8f2e9d95b7f..416e031ac454b 100644
> > --- a/drivers/gpu/drm/xe/xe_oa.c
> > +++ b/drivers/gpu/drm/xe/xe_oa.c
> > @@ -578,10 +578,10 @@ static __poll_t xe_oa_poll(struct file *file, poll_table *wait)
> >  }
> >
> >  static int xe_oa_submit_bb(struct xe_oa_stream *stream, struct xe_bb *bb,
> > -			   struct dma_fence **fence)
> > +			   bool add_deps, struct dma_fence **fence)
>
> Bools are highly frowned upon [1] (one of many example of this). How
> about passing in 'stream->num_syncs' or 0?
>
> i.e. s/bool add_deps/u32 num_syncs/
>
> Maybe even add stream->syncs as argument then too.

I don't like this because this function is called from 3 places, 2 of which
set the arg to false and only one to true, even if stream->num_syncs > 0.
So it will not be clear why we are setting num_syncs to 0 from those 2
places and we will have to add comments pointing that out. Whereas it is
clear with the bool and no comments are needed.

But I understand the concerns about bool so I have instead converted it
into an enum in v2. That I think should take care of most of the objections
against bool. Let me know if this is ok or any other ideas regarding this?

Thanks.
--
Ashutosh

>
> Matt
>
> [1] https://patchwork.freedesktop.org/patch/607408/?series=135265&rev=6#comment_1103868

>
> >  {
> >	struct xe_sched_job *job;
> > -	int err = 0;
> > +	int i, err = 0;
> >
> >	/* Kernel configuration is issued on stream->k_exec_q, not stream->exec_q */
> >	job = xe_bb_create_job(stream->k_exec_q, bb);
> > @@ -590,9 +590,23 @@ static int xe_oa_submit_bb(struct xe_oa_stream *stream, struct xe_bb *bb,
> >		goto exit;
> >	}
> >
> > +	if (add_deps) {
> > +		for (i = 0; i < stream->num_syncs && !err; i++)
> > +			err = xe_sync_entry_add_deps(&stream->syncs[i], job);
> > +		if (err) {
> > +			drm_dbg(&stream->oa->xe->drm, "xe_sync_entry_add_deps err %d\n", err);
> > +			goto err_put_job;
> > +		}
> > +	}
> > +
> >	xe_sched_job_arm(job);
> >	*fence = dma_fence_get(&job->drm.s_fence->finished);
> >	xe_sched_job_push(job);
> > +
> > +	return 0;
> > +
> > +err_put_job:
> > +	xe_sched_job_put(job);
> >  exit:
> >	return err;
> >  }
> > @@ -670,7 +684,7 @@ static int xe_oa_modify_ctx_image(struct xe_oa_stream *stream, struct xe_lrc *lr
> >
> >	xe_oa_store_flex(stream, lrc, bb, flex, count);
> >
> > -	err = xe_oa_submit_bb(stream, bb, &fence);
> > +	err = xe_oa_submit_bb(stream, bb, false, &fence);
> >	xe_bb_free(bb, fence);
> >	dma_fence_put(fence);
> >  exit:
> > @@ -691,7 +705,7 @@ static int xe_oa_load_with_lri(struct xe_oa_stream *stream, struct xe_oa_reg *re
> >
> >	write_cs_mi_lri(bb, reg_lri, 1);
> >
> > -	err = xe_oa_submit_bb(stream, bb, &fence);
> > +	err = xe_oa_submit_bb(stream, bb, false, &fence);
> >	xe_bb_free(bb, fence);
> >	dma_fence_put(fence);
> >  exit:
> > @@ -971,7 +985,7 @@ static int xe_oa_emit_oa_config(struct xe_oa_stream *stream, struct xe_oa_config
> >		goto exit;
> >	}
> >
> > -	err = xe_oa_submit_bb(stream, oa_bo->bb, &fence);
> > +	err = xe_oa_submit_bb(stream, oa_bo->bb, true, &fence);
> >	if (err)
> >		goto exit;
> >
> > --
> > 2.41.0
> >