Re: [PATCH 1/1] drm/scheduler: Job timeout handler returns status (v2)

From: "Christian König" <christian.koenig@amd.com>
To: Lucas Stach <l.stach@pengutronix.de>,
	Luben Tuikov <luben.tuikov@amd.com>,
	dri-devel@lists.freedesktop.org, amd-gfx@lists.freedesktop.org
Cc: Andrey Grodzovsky <Andrey.Grodzovsky@amd.com>,
	kernel test robot <lkp@intel.com>,
	Tomeu Vizoso <tomeu.vizoso@collabora.com>,
	Rob Herring <robh@kernel.org>,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	Steven Price <steven.price@arm.com>,
	Eric Anholt <eric@anholt.net>,
	Christian Gmeiner <christian.gmeiner@gmail.com>,
	Qiang Yu <yuq825@gmail.com>,
	Russell King <linux+etnaviv@armlinux.org.uk>,
	Alexander Deucher <Alexander.Deucher@amd.com>,
	Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
Subject: Re: [PATCH 1/1] drm/scheduler: Job timeout handler returns status (v2)
Date: Thu, 10 Dec 2020 10:41:59 +0100	[thread overview]
Message-ID: <b42284c3-b774-41dc-d4fa-1a61d9d25df4@amd.com> (raw)
In-Reply-To: <39a74cea2b6f3bfcc7b86de7a7a1dbcc93e21a7f.camel@pengutronix.de>

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.

See even when the job has completed we need to restart the timer for the 
potential next job.

Only when the device is completely gone and unrecoverable we should not 
restart the timer.

I suggest to either make this an int and return -ENODEV when that 
happens or rename the enum to something like DRM_SCHED_NODEV.

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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx