Re: [PATCH v6 1/2] drm/sched: Refactor ring mirror list handling.
On Thu, Dec 27, 2018 at 8:28 PM Andrey Grodzovsky
wrote:
>
> Decauple sched threads stop and start and ring mirror
> list handling from the policy of what to do about the
> guilty jobs.
> When stoppping the sched thread and detaching sched fences
> from non signaled HW fenes wait for all signaled HW fences
> to complete before rerunning the jobs.
>
> v2: Fix resubmission of guilty job into HW after refactoring.
>
> v4:
> Full restart for all the jobs, not only from guilty ring.
> Extract karma increase into standalone function.
>
> v5:
> Rework waiting for signaled jobs without relying on the job
> struct itself as those might already be freed for non 'guilty'
> job's schedulers.
> Expose karma increase to drivers.
>
> v6:
> Use list_for_each_entry_safe_continue and drm_sched_process_job
> in case fence already signaled.
> Call drm_sched_increase_karma only once for amdgpu and add documentation.
>
> Suggested-by: Christian Koenig
> Signed-off-by: Andrey Grodzovsky
./drivers/gpu/drm/scheduler/sched_main.c:429: warning: Function
parameter or member 'full_recovery' not described in 'drm_sched_start'
Please fix, thanks.
-Daniel
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 20 ++-
> drivers/gpu/drm/etnaviv/etnaviv_sched.c| 11 +-
> drivers/gpu/drm/scheduler/sched_main.c | 195
> +++--
> drivers/gpu/drm/v3d/v3d_sched.c| 12 +-
> include/drm/gpu_scheduler.h| 8 +-
> 5 files changed, 157 insertions(+), 89 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 98df8e4..6a0601c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -3298,17 +3298,15 @@ static int amdgpu_device_pre_asic_reset(struct
> amdgpu_device *adev,
> if (!ring || !ring->sched.thread)
> continue;
>
> - kthread_park(ring->sched.thread);
> -
> - if (job && job->base.sched != &ring->sched)
> - continue;
> -
> - drm_sched_hw_job_reset(&ring->sched, job ? &job->base : NULL);
> + drm_sched_stop(&ring->sched, job ? &job->base : NULL);
>
> /* after all hw jobs are reset, hw fence is meaningless, so
> force_completion */
> amdgpu_fence_driver_force_completion(ring);
> }
>
> + if(job)
> + drm_sched_increase_karma(&job->base);
> +
>
>
> if (!amdgpu_sriov_vf(adev)) {
> @@ -3454,14 +3452,10 @@ static void amdgpu_device_post_asic_reset(struct
> amdgpu_device *adev,
> if (!ring || !ring->sched.thread)
> continue;
>
> - /* only need recovery sched of the given job's ring
> -* or all rings (in the case @job is NULL)
> -* after above amdgpu_reset accomplished
> -*/
> - if ((!job || job->base.sched == &ring->sched) &&
> !adev->asic_reset_res)
> - drm_sched_job_recovery(&ring->sched);
> + if (!adev->asic_reset_res)
> + drm_sched_resubmit_jobs(&ring->sched);
>
> - kthread_unpark(ring->sched.thread);
> + drm_sched_start(&ring->sched, !adev->asic_reset_res);
> }
>
> if (!amdgpu_device_has_dc_support(adev)) {
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_sched.c
> b/drivers/gpu/drm/etnaviv/etnaviv_sched.c
> index 49a6763..6f1268f 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_sched.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_sched.c
> @@ -109,16 +109,19 @@ static void etnaviv_sched_timedout_job(struct
> drm_sched_job *sched_job)
> }
>
> /* block scheduler */
> - kthread_park(gpu->sched.thread);
> - drm_sched_hw_job_reset(&gpu->sched, sched_job);
> + drm_sched_stop(&gpu->sched, sched_job);
> +
> + if(sched_job)
> + drm_sched_increase_karma(sched_job);
>
> /* get the GPU back into the init state */
> etnaviv_core_dump(gpu);
> etnaviv_gpu_recover_hang(gpu);
>
> + drm_sched_resubmit_jobs(&gpu->sched);
> +
> /* restart scheduler after GPU is usable again */
> - drm_sched_job_recovery(&gpu->sched);
> - kthread_unpark(gpu->sched.thread);
> + drm_sched_start(&gpu->sched, true);
> }
>
> static void etnaviv_sched_free_job(struct drm_sched_job *sched_job)
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
> b/drivers/gpu/drm/scheduler/sched_main.c
> index dbb6906..54e809b 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -60,8 +60,6 @@
>
> static void drm_sched_process_job(struct dma_fence *f, struct dma_fence_cb
> *cb);
>
> -static void drm_sched_expel_job_unlocked(struct drm_sched_job *s_job);
> -
> /**
> * drm_sched_rq_init - initialize a given run queue struct
> *
> @@ -335,6 +333,51 @@ s
Re: [PATCH v6 1/2] drm/sched: Refactor ring mirror list handling.
On 3/12/19 3:43 AM, Tomeu Vizoso wrote:
> On Thu, 27 Dec 2018 at 20:28, Andrey Grodzovsky
> wrote:
>> Decauple sched threads stop and start and ring mirror
>> list handling from the policy of what to do about the
>> guilty jobs.
>> When stoppping the sched thread and detaching sched fences
>> from non signaled HW fenes wait for all signaled HW fences
>> to complete before rerunning the jobs.
>>
>> v2: Fix resubmission of guilty job into HW after refactoring.
>>
>> v4:
>> Full restart for all the jobs, not only from guilty ring.
>> Extract karma increase into standalone function.
>>
>> v5:
>> Rework waiting for signaled jobs without relying on the job
>> struct itself as those might already be freed for non 'guilty'
>> job's schedulers.
>> Expose karma increase to drivers.
>>
>> v6:
>> Use list_for_each_entry_safe_continue and drm_sched_process_job
>> in case fence already signaled.
>> Call drm_sched_increase_karma only once for amdgpu and add documentation.
>>
>> Suggested-by: Christian Koenig
>> Signed-off-by: Andrey Grodzovsky
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 20 ++-
>> drivers/gpu/drm/etnaviv/etnaviv_sched.c| 11 +-
>> drivers/gpu/drm/scheduler/sched_main.c | 195
>> +++--
>> drivers/gpu/drm/v3d/v3d_sched.c| 12 +-
>> include/drm/gpu_scheduler.h| 8 +-
>> 5 files changed, 157 insertions(+), 89 deletions(-)
>>
> [snip]
>> diff --git a/drivers/gpu/drm/v3d/v3d_sched.c
>> b/drivers/gpu/drm/v3d/v3d_sched.c
>> index 445b2ef..f76d9ed 100644
>> --- a/drivers/gpu/drm/v3d/v3d_sched.c
>> +++ b/drivers/gpu/drm/v3d/v3d_sched.c
>> @@ -178,18 +178,22 @@ v3d_job_timedout(struct drm_sched_job *sched_job)
>> for (q = 0; q < V3D_MAX_QUEUES; q++) {
>> struct drm_gpu_scheduler *sched = &v3d->queue[q].sched;
>>
>> - kthread_park(sched->thread);
>> - drm_sched_hw_job_reset(sched, (sched_job->sched == sched ?
>> + drm_sched_stop(sched, (sched_job->sched == sched ?
>> sched_job : NULL));
>> +
>> + if(sched_job)
>> + drm_sched_increase_karma(sched_job);
>> }
>>
>> /* get the GPU back into the init state */
>> v3d_reset(v3d);
>>
>> + for (q = 0; q < V3D_MAX_QUEUES; q++)
>> + drm_sched_resubmit_jobs(sched_job->sched);
> Hi Andrey,
>
> I'm not sure of what was the original intent, but I guess it wasn't to
> repeatedly call resubmit_jobs on that specific job's queue?
>
> Regards,
>
> Tomeu
My bad, there is also another mistake here with increasing karma for
the guilty job's entity multiple times. I will fix that. Thanks for
pointing out.
Andrey
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v6 1/2] drm/sched: Refactor ring mirror list handling.
On Thu, 27 Dec 2018 at 20:28, Andrey Grodzovsky
wrote:
>
> Decauple sched threads stop and start and ring mirror
> list handling from the policy of what to do about the
> guilty jobs.
> When stoppping the sched thread and detaching sched fences
> from non signaled HW fenes wait for all signaled HW fences
> to complete before rerunning the jobs.
>
> v2: Fix resubmission of guilty job into HW after refactoring.
>
> v4:
> Full restart for all the jobs, not only from guilty ring.
> Extract karma increase into standalone function.
>
> v5:
> Rework waiting for signaled jobs without relying on the job
> struct itself as those might already be freed for non 'guilty'
> job's schedulers.
> Expose karma increase to drivers.
>
> v6:
> Use list_for_each_entry_safe_continue and drm_sched_process_job
> in case fence already signaled.
> Call drm_sched_increase_karma only once for amdgpu and add documentation.
>
> Suggested-by: Christian Koenig
> Signed-off-by: Andrey Grodzovsky
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 20 ++-
> drivers/gpu/drm/etnaviv/etnaviv_sched.c| 11 +-
> drivers/gpu/drm/scheduler/sched_main.c | 195
> +++--
> drivers/gpu/drm/v3d/v3d_sched.c| 12 +-
> include/drm/gpu_scheduler.h| 8 +-
> 5 files changed, 157 insertions(+), 89 deletions(-)
>
[snip]
> diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c
> index 445b2ef..f76d9ed 100644
> --- a/drivers/gpu/drm/v3d/v3d_sched.c
> +++ b/drivers/gpu/drm/v3d/v3d_sched.c
> @@ -178,18 +178,22 @@ v3d_job_timedout(struct drm_sched_job *sched_job)
> for (q = 0; q < V3D_MAX_QUEUES; q++) {
> struct drm_gpu_scheduler *sched = &v3d->queue[q].sched;
>
> - kthread_park(sched->thread);
> - drm_sched_hw_job_reset(sched, (sched_job->sched == sched ?
> + drm_sched_stop(sched, (sched_job->sched == sched ?
>sched_job : NULL));
> +
> + if(sched_job)
> + drm_sched_increase_karma(sched_job);
> }
>
> /* get the GPU back into the init state */
> v3d_reset(v3d);
>
> + for (q = 0; q < V3D_MAX_QUEUES; q++)
> + drm_sched_resubmit_jobs(sched_job->sched);
Hi Andrey,
I'm not sure of what was the original intent, but I guess it wasn't to
repeatedly call resubmit_jobs on that specific job's queue?
Regards,
Tomeu
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v6 1/2] drm/sched: Refactor ring mirror list handling.
Hi Andrey, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on linus/master] [also build test WARNING on next-20181224] [cannot apply to v4.20] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Andrey-Grodzovsky/drm-sched-Refactor-ring-mirror-list-handling/20181228-035754 reproduce: make htmldocs All warnings (new ones prefixed by >>): net/mac80211/sta_info.h:588: warning: Function parameter or member 'rx_stats_avg.chain_signal' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.filtered' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.retry_failed' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.retry_count' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.lost_packets' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.last_tdls_pkt_time' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.msdu_retries' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.msdu_failed' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.last_ack' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.last_ack_signal' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.ack_signal_filled' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.avg_ack_signal' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'tx_stats.packets' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'tx_stats.bytes' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'tx_stats.last_rate' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'tx_stats.msdu' not described in 'sta_info' kernel/rcu/tree.c:711: warning: Excess function parameter 'irq' description in 'rcu_nmi_exit' include/linux/dma-buf.h:304: warning: Function parameter or member 'cb_excl.cb' not described in 'dma_buf' include/linux/dma-buf.h:304: warning: Function parameter or member 'cb_excl.poll' not described in 'dma_buf' include/linux/dma-buf.h:304: warning: Function parameter or member 'cb_excl.active' not described in 'dma_buf' include/linux/dma-buf.h:304: warning: Function parameter or member 'cb_shared.cb' not described in 'dma_buf' include/linux/dma-buf.h:304: warning: Function parameter or member 'cb_shared.poll' not described in 'dma_buf' include/linux/dma-buf.h:304: warning: Function parameter or member 'cb_shared.active' not described in 'dma_buf' include/linux/dma-fence-array.h:54: warning: Function parameter or member 'work' not described in 'dma_fence_array' include/linux/gpio/driver.h:375: warning: Function parameter or member 'init_valid_mask' not described in 'gpio_chip' include/linux/iio/hw-consumer.h:1: warning: no structured comments found include/linux/input/sparse-keymap.h:46: warning: Function parameter or member 'sw' not described in 'key_entry' drivers/mtd/nand/raw/nand_base.c:420: warning: Function parameter or member 'chip' not described in 'nand_fill_oob' drivers/mtd/nand/raw/nand_bbt.c:173: warning: Function parameter or member 'this' not described in 'read_bbt' drivers/mtd/nand/raw/nand_bbt.c:173: warning: Excess function parameter 'chip' description in 'read_bbt' include/linux/regulator/machine.h:199: warning: Function parameter or member 'max_uV_step' not described in 'regulation_constraints' include/linux/regulator/driver.h:228: warning: Function parameter or member 'resume' not described in 'regulator_ops' arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.esw0' not described in 'irb' arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.esw1' not described in 'irb' arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.esw2' not described in 'irb' arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.esw3' not described in 'irb' arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.eadm' not described in 'irb' drivers/slimbus/stream.c:1: warning: no structured comments found include/linux/spi/spi.h:180: warning: Function parameter or member 'driver_override' not d
[PATCH v6 1/2] drm/sched: Refactor ring mirror list handling.
Decauple sched threads stop and start and ring mirror
list handling from the policy of what to do about the
guilty jobs.
When stoppping the sched thread and detaching sched fences
from non signaled HW fenes wait for all signaled HW fences
to complete before rerunning the jobs.
v2: Fix resubmission of guilty job into HW after refactoring.
v4:
Full restart for all the jobs, not only from guilty ring.
Extract karma increase into standalone function.
v5:
Rework waiting for signaled jobs without relying on the job
struct itself as those might already be freed for non 'guilty'
job's schedulers.
Expose karma increase to drivers.
v6:
Use list_for_each_entry_safe_continue and drm_sched_process_job
in case fence already signaled.
Call drm_sched_increase_karma only once for amdgpu and add documentation.
Suggested-by: Christian Koenig
Signed-off-by: Andrey Grodzovsky
---
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 20 ++-
drivers/gpu/drm/etnaviv/etnaviv_sched.c| 11 +-
drivers/gpu/drm/scheduler/sched_main.c | 195 +++--
drivers/gpu/drm/v3d/v3d_sched.c| 12 +-
include/drm/gpu_scheduler.h| 8 +-
5 files changed, 157 insertions(+), 89 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 98df8e4..6a0601c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -3298,17 +3298,15 @@ static int amdgpu_device_pre_asic_reset(struct
amdgpu_device *adev,
if (!ring || !ring->sched.thread)
continue;
- kthread_park(ring->sched.thread);
-
- if (job && job->base.sched != &ring->sched)
- continue;
-
- drm_sched_hw_job_reset(&ring->sched, job ? &job->base : NULL);
+ drm_sched_stop(&ring->sched, job ? &job->base : NULL);
/* after all hw jobs are reset, hw fence is meaningless, so
force_completion */
amdgpu_fence_driver_force_completion(ring);
}
+ if(job)
+ drm_sched_increase_karma(&job->base);
+
if (!amdgpu_sriov_vf(adev)) {
@@ -3454,14 +3452,10 @@ static void amdgpu_device_post_asic_reset(struct
amdgpu_device *adev,
if (!ring || !ring->sched.thread)
continue;
- /* only need recovery sched of the given job's ring
-* or all rings (in the case @job is NULL)
-* after above amdgpu_reset accomplished
-*/
- if ((!job || job->base.sched == &ring->sched) &&
!adev->asic_reset_res)
- drm_sched_job_recovery(&ring->sched);
+ if (!adev->asic_reset_res)
+ drm_sched_resubmit_jobs(&ring->sched);
- kthread_unpark(ring->sched.thread);
+ drm_sched_start(&ring->sched, !adev->asic_reset_res);
}
if (!amdgpu_device_has_dc_support(adev)) {
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_sched.c
b/drivers/gpu/drm/etnaviv/etnaviv_sched.c
index 49a6763..6f1268f 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_sched.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_sched.c
@@ -109,16 +109,19 @@ static void etnaviv_sched_timedout_job(struct
drm_sched_job *sched_job)
}
/* block scheduler */
- kthread_park(gpu->sched.thread);
- drm_sched_hw_job_reset(&gpu->sched, sched_job);
+ drm_sched_stop(&gpu->sched, sched_job);
+
+ if(sched_job)
+ drm_sched_increase_karma(sched_job);
/* get the GPU back into the init state */
etnaviv_core_dump(gpu);
etnaviv_gpu_recover_hang(gpu);
+ drm_sched_resubmit_jobs(&gpu->sched);
+
/* restart scheduler after GPU is usable again */
- drm_sched_job_recovery(&gpu->sched);
- kthread_unpark(gpu->sched.thread);
+ drm_sched_start(&gpu->sched, true);
}
static void etnaviv_sched_free_job(struct drm_sched_job *sched_job)
diff --git a/drivers/gpu/drm/scheduler/sched_main.c
b/drivers/gpu/drm/scheduler/sched_main.c
index dbb6906..54e809b 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -60,8 +60,6 @@
static void drm_sched_process_job(struct dma_fence *f, struct dma_fence_cb
*cb);
-static void drm_sched_expel_job_unlocked(struct drm_sched_job *s_job);
-
/**
* drm_sched_rq_init - initialize a given run queue struct
*
@@ -335,6 +333,51 @@ static void drm_sched_job_timedout(struct work_struct
*work)
spin_unlock_irqrestore(&sched->job_list_lock, flags);
}
+ /**
+ * drm_sched_increase_karma - Update sched_entity guilty flag
+ *
+ * @bad: The job guilty of time out
+ *
+ * Increment on every hang caused by the 'bad' job. If this exceeds the hang
+ * limit of the scheduler then the respective sched entity is marked guilty
and
+ * jobs from it will not be scheduled furth
