On Tue, Jul 30, 2024 at 02:06:08PM +0200, Christian König wrote: > Am 30.07.24 um 10:36 schrieb Daniel Vetter: > > > In the end you have a really nice circle dependency. > > Maybe a follow up, so for arb robustness or vk context where we want the > > context to die and refuse to accept any more jobs: We can get at that > > error somehow? I think that's really the only worry I have with a job > > error approach for all this ... > > See drm_sched_entity_error(). The idea is that the driver uses this function > in two ways: Ah that's the other piece I missed ... > 1. In it's prepare callback so that when one submission fails all following > from the same ctx are marked with an error number as well. > > This is intentionally done in a driver callback so that driver decides if > they want subsequent submissions to fail or not. That can be helpful for > example for in kernel paging queues where submissions don't depend on each > other and a failed submission shouldn't cancel all following. > > For an example see amdgpu_job_prepare_job(). > > 2. In it's submission IOCTL to reject new submissions and inform userspace > that it needs to kick of some error handling. Would be good to add that to the docs, I think just one sentence in the drm_sched_start should fish out the errno with drm_sched_entity_error() would have avoided my confusion. Plus I think your above text would make a good addition to the kerneldoc for drm_sched_entity_error() itself. Cheers, Sima > > Cheers, > Christian. > > > > > > > If we really want to stuff this into per-job fences then I think we should > > > > at least try to document this mess in the sync_file uapi, for a bit of > > > > consistency. > > > Good point. Going to add some documentation. > > Sounds good. > > > > Cheers, Sima > > > > > Regards, > > > Christian. > > > > > > > But yeah without the full picture no idea really what we want here. > > > > -Sima > > > > > > > > > --- > > > > > drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c | 2 +- > > > > > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 4 ++-- > > > > > drivers/gpu/drm/etnaviv/etnaviv_sched.c | 4 ++-- > > > > > drivers/gpu/drm/imagination/pvr_queue.c | 4 ++-- > > > > > drivers/gpu/drm/lima/lima_sched.c | 2 +- > > > > > drivers/gpu/drm/nouveau/nouveau_sched.c | 2 +- > > > > > drivers/gpu/drm/panfrost/panfrost_job.c | 2 +- > > > > > drivers/gpu/drm/panthor/panthor_mmu.c | 2 +- > > > > > drivers/gpu/drm/scheduler/sched_main.c | 7 ++++--- > > > > > drivers/gpu/drm/v3d/v3d_sched.c | 2 +- > > > > > include/drm/gpu_scheduler.h | 2 +- > > > > > 11 files changed, 17 insertions(+), 16 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c > > > > > index 2320df51c914..18135d8235f9 100644 > > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c > > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c > > > > > @@ -300,7 +300,7 @@ static int suspend_resume_compute_scheduler(struct amdgpu_device *adev, bool sus > > > > > if (r) > > > > > goto out; > > > > > } else { > > > > > - drm_sched_start(&ring->sched); > > > > > + drm_sched_start(&ring->sched, 0); > > > > > } > > > > > } > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > > > > > index c186fdb198ad..861827deb03f 100644 > > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > > > > > @@ -5862,7 +5862,7 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev, > > > > > if (!amdgpu_ring_sched_ready(ring)) > > > > > continue; > > > > > - drm_sched_start(&ring->sched); > > > > > + drm_sched_start(&ring->sched, 0); > > > > > } > > > > > if (!drm_drv_uses_atomic_modeset(adev_to_drm(tmp_adev)) && !job_signaled) > > > > > @@ -6360,7 +6360,7 @@ void amdgpu_pci_resume(struct pci_dev *pdev) > > > > > if (!amdgpu_ring_sched_ready(ring)) > > > > > continue; > > > > > - drm_sched_start(&ring->sched); > > > > > + drm_sched_start(&ring->sched, 0); > > > > > } > > > > > amdgpu_device_unset_mp1_state(adev); > > > > > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_sched.c b/drivers/gpu/drm/etnaviv/etnaviv_sched.c > > > > > index c53641aa146f..2c8666f8ec4a 100644 > > > > > --- a/drivers/gpu/drm/etnaviv/etnaviv_sched.c > > > > > +++ b/drivers/gpu/drm/etnaviv/etnaviv_sched.c > > > > > @@ -72,12 +72,12 @@ static enum drm_gpu_sched_stat etnaviv_sched_timedout_job(struct drm_sched_job > > > > > drm_sched_resubmit_jobs(&gpu->sched); > > > > > - drm_sched_start(&gpu->sched); > > > > > + drm_sched_start(&gpu->sched, 0); > > > > > return DRM_GPU_SCHED_STAT_NOMINAL; > > > > > out_no_timeout: > > > > > /* restart scheduler after GPU is usable again */ > > > > > - drm_sched_start(&gpu->sched); > > > > > + drm_sched_start(&gpu->sched, 0); > > > > > return DRM_GPU_SCHED_STAT_NOMINAL; > > > > > } > > > > > diff --git a/drivers/gpu/drm/imagination/pvr_queue.c b/drivers/gpu/drm/imagination/pvr_queue.c > > > > > index 20cb46012082..c4f08432882b 100644 > > > > > --- a/drivers/gpu/drm/imagination/pvr_queue.c > > > > > +++ b/drivers/gpu/drm/imagination/pvr_queue.c > > > > > @@ -782,7 +782,7 @@ static void pvr_queue_start(struct pvr_queue *queue) > > > > > } > > > > > } > > > > > - drm_sched_start(&queue->scheduler); > > > > > + drm_sched_start(&queue->scheduler, 0); > > > > > } > > > > > /** > > > > > @@ -842,7 +842,7 @@ pvr_queue_timedout_job(struct drm_sched_job *s_job) > > > > > } > > > > > mutex_unlock(&pvr_dev->queues.lock); > > > > > - drm_sched_start(sched); > > > > > + drm_sched_start(sched, 0); > > > > > return DRM_GPU_SCHED_STAT_NOMINAL; > > > > > } > > > > > diff --git a/drivers/gpu/drm/lima/lima_sched.c b/drivers/gpu/drm/lima/lima_sched.c > > > > > index 1a944edb6ddc..b40c90e97d7e 100644 > > > > > --- a/drivers/gpu/drm/lima/lima_sched.c > > > > > +++ b/drivers/gpu/drm/lima/lima_sched.c > > > > > @@ -463,7 +463,7 @@ static enum drm_gpu_sched_stat lima_sched_timedout_job(struct drm_sched_job *job > > > > > lima_pm_idle(ldev); > > > > > drm_sched_resubmit_jobs(&pipe->base); > > > > > - drm_sched_start(&pipe->base); > > > > > + drm_sched_start(&pipe->base, 0); > > > > > return DRM_GPU_SCHED_STAT_NOMINAL; > > > > > } > > > > > diff --git a/drivers/gpu/drm/nouveau/nouveau_sched.c b/drivers/gpu/drm/nouveau/nouveau_sched.c > > > > > index eb6c3f9a01f5..4412f2711fb5 100644 > > > > > --- a/drivers/gpu/drm/nouveau/nouveau_sched.c > > > > > +++ b/drivers/gpu/drm/nouveau/nouveau_sched.c > > > > > @@ -379,7 +379,7 @@ nouveau_sched_timedout_job(struct drm_sched_job *sched_job) > > > > > else > > > > > NV_PRINTK(warn, job->cli, "Generic job timeout.\n"); > > > > > - drm_sched_start(sched); > > > > > + drm_sched_start(sched, 0); > > > > > return stat; > > > > > } > > > > > diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c > > > > > index df49d37d0e7e..d140800606bf 100644 > > > > > --- a/drivers/gpu/drm/panfrost/panfrost_job.c > > > > > +++ b/drivers/gpu/drm/panfrost/panfrost_job.c > > > > > @@ -727,7 +727,7 @@ panfrost_reset(struct panfrost_device *pfdev, > > > > > /* Restart the schedulers */ > > > > > for (i = 0; i < NUM_JOB_SLOTS; i++) > > > > > - drm_sched_start(&pfdev->js->queue[i].sched); > > > > > + drm_sched_start(&pfdev->js->queue[i].sched, 0); > > > > > /* Re-enable job interrupts now that everything has been restarted. */ > > > > > job_write(pfdev, JOB_INT_MASK, > > > > > diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c > > > > > index d47972806d50..e630cdf47f99 100644 > > > > > --- a/drivers/gpu/drm/panthor/panthor_mmu.c > > > > > +++ b/drivers/gpu/drm/panthor/panthor_mmu.c > > > > > @@ -827,7 +827,7 @@ static void panthor_vm_stop(struct panthor_vm *vm) > > > > > static void panthor_vm_start(struct panthor_vm *vm) > > > > > { > > > > > - drm_sched_start(&vm->sched); > > > > > + drm_sched_start(&vm->sched, 0); > > > > > } > > > > > /** > > > > > diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c > > > > > index ab53ab486fe6..f093616fe53c 100644 > > > > > --- a/drivers/gpu/drm/scheduler/sched_main.c > > > > > +++ b/drivers/gpu/drm/scheduler/sched_main.c > > > > > @@ -674,9 +674,10 @@ EXPORT_SYMBOL(drm_sched_stop); > > > > > * drm_sched_start - recover jobs after a reset > > > > > * > > > > > * @sched: scheduler instance > > > > > + * @errno: error to set on the pending fences > > > > > * > > > > > */ > > > > > -void drm_sched_start(struct drm_gpu_scheduler *sched) > > > > > +void drm_sched_start(struct drm_gpu_scheduler *sched, int errno) > > > > > { > > > > > struct drm_sched_job *s_job, *tmp; > > > > > @@ -691,13 +692,13 @@ void drm_sched_start(struct drm_gpu_scheduler *sched) > > > > > atomic_add(s_job->credits, &sched->credit_count); > > > > > if (!fence) { > > > > > - drm_sched_job_done(s_job, -ECANCELED); > > > > > + drm_sched_job_done(s_job, errno ?: -ECANCELED); > > > > > continue; > > > > > } > > > > > if (dma_fence_add_callback(fence, &s_job->cb, > > > > > drm_sched_job_done_cb)) > > > > > - drm_sched_job_done(s_job, fence->error); > > > > > + drm_sched_job_done(s_job, fence->error ?: errno); > > > > > } > > > > > drm_sched_start_timeout_unlocked(sched); > > > > > diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c > > > > > index 42d4f4a2dba2..cac02284cd19 100644 > > > > > --- a/drivers/gpu/drm/v3d/v3d_sched.c > > > > > +++ b/drivers/gpu/drm/v3d/v3d_sched.c > > > > > @@ -653,7 +653,7 @@ v3d_gpu_reset_for_timeout(struct v3d_dev *v3d, struct drm_sched_job *sched_job) > > > > > /* Unblock schedulers and restart their jobs. */ > > > > > for (q = 0; q < V3D_MAX_QUEUES; q++) { > > > > > - drm_sched_start(&v3d->queue[q].sched); > > > > > + drm_sched_start(&v3d->queue[q].sched, 0); > > > > > } > > > > > mutex_unlock(&v3d->reset_lock); > > > > > diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h > > > > > index fe8edb917360..a8d19b10f9b8 100644 > > > > > --- a/include/drm/gpu_scheduler.h > > > > > +++ b/include/drm/gpu_scheduler.h > > > > > @@ -579,7 +579,7 @@ bool drm_sched_wqueue_ready(struct drm_gpu_scheduler *sched); > > > > > void drm_sched_wqueue_stop(struct drm_gpu_scheduler *sched); > > > > > void drm_sched_wqueue_start(struct drm_gpu_scheduler *sched); > > > > > void drm_sched_stop(struct drm_gpu_scheduler *sched, struct drm_sched_job *bad); > > > > > -void drm_sched_start(struct drm_gpu_scheduler *sched); > > > > > +void drm_sched_start(struct drm_gpu_scheduler *sched, int errno); > > > > > void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched); > > > > > void drm_sched_increase_karma(struct drm_sched_job *bad); > > > > > void drm_sched_reset_karma(struct drm_sched_job *bad); > > > > > -- > > > > > 2.34.1 > > > > > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch