Am 11.12.20 um 21:44 schrieb Luben Tuikov:
On 2020-12-10 4:41 a.m., Christian König wrote:
Am 10.12.20 um 10:31 schrieb Lucas Stach:
Hi Luben,
Am Mittwoch, den 09.12.2020, 21:14 -0500 schrieb Luben Tuikov:
[SNIP]
-static void etnaviv_sched_timedout_job(struct drm_sched_job *sched_job)
+static enum drm_task_status etnaviv_sched_timedout_job(struct drm_sched_job
+ *sched_job)
{
struct etnaviv_gem_submit *submit = to_etnaviv_submit(sched_job);
struct etnaviv_gpu *gpu = submit->gpu;
@@ -120,9 +121,16 @@ static void etnaviv_sched_timedout_job(struct drm_sched_job *sched_job)
drm_sched_resubmit_jobs(&gpu->sched);
+ /* Tell the DRM scheduler that this task needs
+ * more time.
+ */
This comment doesn't match the kernel coding style, but it's also moot
as the whole added code block isn't needed. The code just below is
identical, so letting execution continue here instead of returning
would be the right thing to do, but maybe you mean to return
DRM_TASK_STATUS_COMPLETE? It's a bit confusing that aborted and job
successfully finished should be signaled with the same return code.
Yes and no. As I tried to describe in my previous mail the naming of the
enum values is also not very good.
I tried to make the naming as minimal as possible:
COMPLETE: the task is out of the hardware.
ALIVE: the task is in the hardware.
Yeah in general that is a good idea, but in this special case that
information is not useful to the scheduler since it needs to restart the
timer anyway.
See even when the job has completed we need to restart the timer for the
potential next job.
Sure, yes. But this is something which the DRM decides--why should
drivers know of the internals of the DRM? (i.e. that it restarts
the timer or that there is a timer, etc.) Return minimal
value and let the DRM decide what to do next.
That's correct, but the driver should also not provide useless
information to the scheduler.
Only when the device is completely gone and unrecoverable we should not
restart the timer.
Yes, agreed.
I suggest to either make this an int and return -ENODEV when that
happens or rename the enum to something like DRM_SCHED_NODEV.
It was an int, but you suggested that it'd be a macro, so I made
it an enum so that the complier can check the values against the macros
returned.
Well what I suggested was to use something better than 0/1 as return value.
And that was a good idea since I didn't noticed before that you want to
return the job status here, which in turn is not necessary a good idea.
I suggested, DRM_TASK_SCHED_ENODEV, but let Andrey add it
when he adds his patches on top of my patch here, because his
work adds hotplug/unplug support.
Also, note that if the pending list is freed, while the DRM
had been blocked, i.e. notified that the device is gone,
then returning DRM_TASK_SCHED_ENODEV would be moot, as there
are no tasks in the pending list.
This patch here is good as it is, since it is minimal and doesn't make
assumptions on DRM behaviour.
The problem with this patch is that it's adding a functionality which we
don't immediately use and as far as I can see won't use in the future
either.
Regards,
Christian.
Regards,
Luben
Regards,
Christian.
+ drm_sched_start(&gpu->sched, true);
+ return DRM_TASK_STATUS_ALIVE;
+
out_no_timeout:
/* restart scheduler after GPU is usable again */
drm_sched_start(&gpu->sched, true);
+ return DRM_TASK_STATUS_ALIVE;
}
Regards,
Lucas
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
