On Fri, 28 Jun 2024 22:34:57 +0100
Liviu Dudau <liviu.du...@arm.com> wrote:
> On Fri, Jun 28, 2024 at 04:55:36PM +0200, Boris Brezillon wrote:
> > A sync-only job is meant to provide a synchronization point on a
> > queue, so we can't return a NULL fence there, we have to add a signal
> > operation to the command stream which executes after all other
> > previously submitted jobs are done.
> >
> > Fixes: de8548813824 ("drm/panthor: Add the scheduler logical block")
> > Signed-off-by: Boris Brezillon <boris.brezil...@collabora.com>
>
> Took me a bit longer to read, lets blame Friday.
>
> > ---
> > drivers/gpu/drm/panthor/panthor_sched.c | 41 ++++++++++++++++++++-----
> > include/uapi/drm/panthor_drm.h | 5 +++
> > 2 files changed, 38 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/panthor/panthor_sched.c
> > b/drivers/gpu/drm/panthor/panthor_sched.c
> > index 79ffcbc41d78..951ff7e63ea8 100644
> > --- a/drivers/gpu/drm/panthor/panthor_sched.c
> > +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> > @@ -458,6 +458,16 @@ struct panthor_queue {
> > /** @seqno: Sequence number of the last initialized fence. */
> > atomic64_t seqno;
> >
> > + /**
> > + * @last_fence: Fence of the last submitted job.
> > + *
> > + * We return this fence when we get an empty command stream.
> > + * This way, we are guaranteed that all earlier jobs have
> > completed
> > + * when drm_sched_job::s_fence::finished without having to feed
> > + * the CS ring buffer with a dummy job that only signals the
> > fence.
> > + */
> > + struct dma_fence *last_fence;
> > +
> > /**
> > * @in_flight_jobs: List containing all in-flight jobs.
> > *
> > @@ -829,6 +839,9 @@ static void group_free_queue(struct panthor_group
> > *group, struct panthor_queue *
> > panthor_kernel_bo_destroy(queue->ringbuf);
> > panthor_kernel_bo_destroy(queue->iface.mem);
> >
> > + /* Release the last_fence we were holding, if any. */
> > + dma_fence_put(queue->fence_ctx.last_fence);
> > +
> > kfree(queue);
> > }
> >
> > @@ -2865,11 +2878,14 @@ queue_run_job(struct drm_sched_job *sched_job)
> > static_assert(sizeof(call_instrs) % 64 == 0,
> > "call_instrs is not aligned on a cacheline");
> >
> > - /* Stream size is zero, nothing to do => return a NULL fence and let
> > - * drm_sched signal the parent.
> > + /* Stream size is zero, nothing to do except making sure all previously
> > + * submitted jobs are done before we signal the
> > + * drm_sched_job::s_fence::finished fence.
> > */
> > - if (!job->call_info.size)
> > - return NULL;
> > + if (!job->call_info.size) {
> > + job->done_fence = dma_fence_get(queue->fence_ctx.last_fence);
> > + return job->done_fence;
>
> What happens if the last job's done_fence was cancelled or timed out? Is the
> sync job's done_fence going to be signalled with the same error?
It's the same object, so yes, the job will also be considered faulty
(the error propagated to the job::s_fence::finished fence). I guess
synchronization jobs are not supposed to fail/timeout in theory, because
they don't do anything, but I don't think that's an issue in
practice, because dma_fence errors are never propagated to user-space
(only the queue status is).
>
> Now that we're returning a fence here, should the job be also added into the
> in_flight_jobs?
Yeah, that's done on purpose, such that we don't end up signalling the
same dma_fence object twice (which is forbidden). This makes me realize
I should probably drop the 'is_cs_empty()' check in
group_sync_upd_work(), since we're not supposed to have a job with an
empty CS in the in_flight_jobs list.
diff --git a/drivers/gpu/drm/panthor/panthor_sched.c
b/drivers/gpu/drm/panthor/panthor_sched.c
index 951ff7e63ea8..8bf01b7b1596 100644
--- a/drivers/gpu/drm/panthor/panthor_sched.c
+++ b/drivers/gpu/drm/panthor/panthor_sched.c
@@ -2797,9 +2797,6 @@ static void group_sync_upd_work(struct work_struct *work)
spin_lock(&queue->fence_ctx.lock);
list_for_each_entry_safe(job, job_tmp,
&queue->fence_ctx.in_flight_jobs, node) {
- if (!job->call_info.size)
- continue;
-
if (syncobj->seqno < job->done_fence->seqno)
break;
>
> If you're happy with depending on the previous job's done_fence and not
> track the sync job in in_flight_jobs, then you can have my
>
> Reviewed-by: Liviu Dudau <liviu.du...@arm.com>
>
> Best regards,
> Liviu
>
> > + }
> >
> > ret = pm_runtime_resume_and_get(ptdev->base.dev);
> > if (drm_WARN_ON(&ptdev->base, ret))
> > @@ -2928,6 +2944,10 @@ queue_run_job(struct drm_sched_job *sched_job)
> > }
> > }
> >
> > + /* Update the last fence. */
> > + dma_fence_put(queue->fence_ctx.last_fence);
> > + queue->fence_ctx.last_fence = dma_fence_get(job->done_fence);
> > +
> > done_fence = dma_fence_get(job->done_fence);
> >
> > out_unlock:
> > @@ -3378,10 +3398,15 @@ panthor_job_create(struct panthor_file *pfile,
> > goto err_put_job;
> > }
> >
> > - job->done_fence = kzalloc(sizeof(*job->done_fence), GFP_KERNEL);
> > - if (!job->done_fence) {
> > - ret = -ENOMEM;
> > - goto err_put_job;
> > + /* Empty command streams don't need a fence, they'll pick the one from
> > + * the previously submitted job.
> > + */
> > + if (job->call_info.size) {
> > + job->done_fence = kzalloc(sizeof(*job->done_fence), GFP_KERNEL);
> > + if (!job->done_fence) {
> > + ret = -ENOMEM;
> > + goto err_put_job;
> > + }
> > }
> >
> > ret = drm_sched_job_init(&job->base,
> > diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h
> > index aaed8e12ad0b..926b1deb1116 100644
> > --- a/include/uapi/drm/panthor_drm.h
> > +++ b/include/uapi/drm/panthor_drm.h
> > @@ -802,6 +802,9 @@ struct drm_panthor_queue_submit {
> > * Must be 64-bit/8-byte aligned (the size of a CS instruction)
> > *
> > * Can be zero if stream_addr is zero too.
> > + *
> > + * When the stream size is zero, the queue submit serves as a
> > + * synchronization point.
> > */
> > __u32 stream_size;
> >
> > @@ -822,6 +825,8 @@ struct drm_panthor_queue_submit {
> > * ensure the GPU doesn't get garbage when reading the indirect command
> > * stream buffers. If you want the cache flush to happen
> > * unconditionally, pass a zero here.
> > + *
> > + * Ignored when stream_size is zero.
> > */
> > __u32 latest_flush;
> >
> > --
> > 2.45.0
> >
>
