host1x_cdma_update_sync_queue() clean up
v2 cover host1x_cdma_update_sync_queue() clean up
v2: I've added two more patches on top of "Cancel only job that actually got
stuck" patch. Hence it is a patch-series now.
Dmitry Osipenko (3):
gpu: host1x: Cancel only job that actually got stuck
gpu: host1x: Don't complete a completed job
gpu: host1x: Continue CDMA execution starting with a next job
drivers/gpu/host1x/cdma.c | 62 +++++++++++++-------------------- drivers/gpu/host1x/hw/cdma_hw.c | 13 ------- 2 files changed, 24 insertions(+), 51 deletions(-)
Signature · 1 line
2.18.0
PATCH v2 1/3 gpu: host1x: Cancel only job that actually got stuck
Host1x doesn't have information about jobs inter-dependency, that is something that will become available once host1x will get a proper jobs scheduler implementation. Currently a hang job causes other unrelated jobs to be canceled, that is a relic from downstream driver which is irrelevant to upstream. Let's cancel only the hanging job and not to touch other jobs in queue.
Signed-off-by Dmitry Osipenko <[email protected]>
Reviewed-by Mikko Perttunen <[email protected]>
--- drivers/gpu/host1x/cdma.c | 33 +++++++-------------------------- 1 file changed, 7 insertions(+), 26 deletions(-)
drivers/gpu/host1x/cdma.c +7 -26
@@ -348,13 +348,11 @@ void host1x_cdma_update_sync_queue(struct host1x_cdma *cdma, }/* -* Walk the sync_queue, first incrementing with the CPU syncpts that -* are partially executed (the first buffer) or fully skipped while -* still in the current context (slots are also NOP-ed). +* Increment with CPU the remaining syncpts of a partially executed job. * -* At the point contexts are interleaved, syncpt increments must be -* done inline with the pushbuffer from a GATHER buffer to maintain -* the order (slots are modified to be a GATHER of syncpt incrs). +* Syncpt increments must be done inline with the pushbuffer from a +* GATHER buffer to maintain the order (slots are modified to be a +* GATHER of syncpt incrs). * * Note: save in restart_addr the location where the timed out buffer * started in the PB, so we can start the refetch from there (with the
@@ -362,20 +360,15 @@ void host1x_cdma_update_sync_queue(struct host1x_cdma *cdma,* properly for this buffer and resources are freed. */ -dev_dbg (dev,"%s: perform CPU incr on pending same ctx buffers\n" , - __func__); +dev_dbg (dev,"%s: perform CPU incr on pending buffers\n" , __func__);if (!list_empty (&cdma->sync_queue)) restart_addr = job->first_get;else restart_addr = cdma->last_pos; -/* do CPU increments as long as this context continues */ -list_for_each_entry_from (job, &cdma->sync_queue, list) { -/* different context, gets us out of this loop */ -if (job->client != cdma->timeout.client) -break ; - +/* do CPU increments for the remaining syncpts */ +if (job) {/* won't need a timeout when replayed */ job->timeout =0 ;
@@ -388,20 +381,8 @@ void host1x_cdma_update_sync_queue(struct host1x_cdma *cdma,host1x_hw_cdma_timeout_cpu_incr (host1x, cdma, job->first_get, syncpt_incrs, job->syncpt_end, job->num_slots); - - syncpt_val += syncpt_incrs; } -/* -* The following sumbits from the same client may be dependent on the -* failed submit and therefore they may fail. Force a small timeout -* to make the queue cleanup faster. -*/ - -list_for_each_entry_from (job, &cdma->sync_queue, list) -if (job->client == cdma->timeout.client) - job->timeout = min_t(unsigned int, job->timeout, 500); -dev_dbg (dev,"%s: finished sync_queue modification\n" , __func__);/* roll back DMAGET and start up channel again */
Signature · 1 line
2.18.0
PATCH v2 2/3 gpu: host1x: Don't complete a completed job
In reply to Dmitry OsipenkoThere is a chance that the last job has been completed at the time of CDMA timeout handler invocation. In this case there is no need to complete the completed job.
Signed-off-by Dmitry Osipenko <[email protected]>
--- drivers/gpu/host1x/cdma.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-)
drivers/gpu/host1x/cdma.c +14 -8
@@ -323,7 +323,7 @@ void host1x_cdma_update_sync_queue(struct host1x_cdma *cdma, {struct host1x *host1x =cdma_to_host1x (cdma);u32 restart_addr, syncpt_incrs, syncpt_val; -struct host1x_job *job =NULL ; +struct host1x_job *job; syncpt_val =host1x_syncpt_load (cdma->timeout.syncpt);
@@ -342,11 +342,16 @@ void host1x_cdma_update_sync_queue(struct host1x_cdma *cdma,list_for_each_entry (job, &cdma->sync_queue, list) {if (syncpt_val < job->syncpt_end) -break ; + goto syncpt_incr;host1x_job_dump (dev, job); } +/* all jobs have been completed */ + job =NULL ; + +syncpt_incr: +/* * Increment with CPU the remaining syncpts of a partially executed job. *
@@ -359,16 +364,16 @@ void host1x_cdma_update_sync_queue(struct host1x_cdma *cdma,* modified NOP-ed PB slots). This lets things appear to have completed * properly for this buffer and resources are freed. */ - -dev_dbg (dev,"%s: perform CPU incr on pending buffers\n" , __func__); - -if (!list_empty (&cdma->sync_queue)) +if (job) restart_addr = job->first_get;else restart_addr = cdma->last_pos;/* do CPU increments for the remaining syncpts */ if (job) { +dev_dbg (dev,"%s: perform CPU incr on pending buffers\n" , + __func__); +/* won't need a timeout when replayed */ job->timeout =0 ;
@@ -381,9 +386,10 @@ void host1x_cdma_update_sync_queue(struct host1x_cdma *cdma,host1x_hw_cdma_timeout_cpu_incr (host1x, cdma, job->first_get, syncpt_incrs, job->syncpt_end, job->num_slots); - } -dev_dbg (dev,"%s: finished sync_queue modification\n" , __func__); +dev_dbg (dev,"%s: finished sync_queue modification\n" , + __func__); + }/* roll back DMAGET and start up channel again */ host1x_hw_cdma_resume (host1x, cdma, restart_addr);
Signature · 1 line
2.18.0
Re: [PATCH v2 2/3] gpu: host1x: Don't complete a completed job
Reviewed-by Mikko Perttunen <[email protected]>
Quoted 75 lines · Dmitry Osipenko
There is a chance that the last job has been completed at the time of CDMA timeout handler invocation. In this case there is no need to complete the completed job.
Signed-off-by Dmitry Osipenko <[email protected]>--- drivers/gpu/host1x/cdma.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-)drivers/gpu/host1x/cdma.c +14
-8@@ -323,7 +323,7 @@ void host1x_cdma_update_sync_queue(struct host1x_cdma *cdma, {struct host1x *host1x =cdma_to_host1x (cdma);u32 restart_addr, syncpt_incrs, syncpt_val; -struct host1x_job *job =NULL ; +struct host1x_job *job; syncpt_val =host1x_syncpt_load (cdma->timeout.syncpt);@@ -342,11 +342,16 @@ void host1x_cdma_update_sync_queue(struct host1x_cdma *cdma,list_for_each_entry (job, &cdma->sync_queue, list) {if (syncpt_val < job->syncpt_end) -break ; + goto syncpt_incr;host1x_job_dump (dev, job); } +/* all jobs have been completed */ + job =NULL ; + +syncpt_incr: +/* * Increment with CPU the remaining syncpts of a partially executed job. * @@ -359,16 +364,16 @@ void host1x_cdma_update_sync_queue(struct host1x_cdma *cdma,* modified NOP-ed PB slots). This lets things appear to have completed * properly for this buffer and resources are freed. */ - -dev_dbg (dev,"%s: perform CPU incr on pending buffers\n" , __func__); - -if (!list_empty (&cdma->sync_queue)) +if (job) restart_addr = job->first_get;else restart_addr = cdma->last_pos;/* do CPU increments for the remaining syncpts */ if (job) { +dev_dbg (dev,"%s: perform CPU incr on pending buffers\n" , + __func__); +/* won't need a timeout when replayed */ job->timeout =0 ;@@ -381,9 +386,10 @@ void host1x_cdma_update_sync_queue(struct host1x_cdma *cdma,host1x_hw_cdma_timeout_cpu_incr (host1x, cdma, job->first_get, syncpt_incrs, job->syncpt_end, job->num_slots); - } -dev_dbg (dev,"%s: finished sync_queue modification\n" , __func__); +dev_dbg (dev,"%s: finished sync_queue modification\n" , + __func__); + }/* roll back DMAGET and start up channel again */ host1x_hw_cdma_resume (host1x, cdma, restart_addr);
PATCH v2 3/3 gpu: host1x: Continue CDMA execution starting with a next job
In reply to Dmitry OsipenkoCurrently gathers of a hung job are getting NOP'ed and a restarted CDMA executes the NOP'ed gathers. There shouldn't be a reason to not restart CDMA execution starting with a next job, avoiding the unnecessary churning with gathers NOP'ing.
Signed-off-by Dmitry Osipenko <[email protected]>
--- drivers/gpu/host1x/cdma.c | 23 +++++++++++------------ drivers/gpu/host1x/hw/cdma_hw.c | 13 ------------- 2 files changed, 11 insertions(+), 25 deletions(-)
drivers/gpu/host1x/cdma.c +11 -12
@@ -323,7 +323,7 @@ void host1x_cdma_update_sync_queue(struct host1x_cdma *cdma, {struct host1x *host1x =cdma_to_host1x (cdma);u32 restart_addr, syncpt_incrs, syncpt_val; -struct host1x_job *job; +struct host1x_job *job, *next_job =NULL ; syncpt_val =host1x_syncpt_load (cdma->timeout.syncpt);
@@ -341,8 +341,13 @@ void host1x_cdma_update_sync_queue(struct host1x_cdma *cdma, __func__);list_for_each_entry (job, &cdma->sync_queue, list) { -if (syncpt_val < job->syncpt_end) +if (syncpt_val < job->syncpt_end) { + +if (!list_is_last (&job->list, &cdma->sync_queue)) + next_job =list_next_entry (job, list); + goto syncpt_incr; + }host1x_job_dump (dev, job); }
@@ -355,17 +360,11 @@ void host1x_cdma_update_sync_queue(struct host1x_cdma *cdma,/* * Increment with CPU the remaining syncpts of a partially executed job. * -* Syncpt increments must be done inline with the pushbuffer from a -* GATHER buffer to maintain the order (slots are modified to be a -* GATHER of syncpt incrs). -* -* Note: save in restart_addr the location where the timed out buffer -* started in the PB, so we can start the refetch from there (with the -* modified NOP-ed PB slots). This lets things appear to have completed -* properly for this buffer and resources are freed. +* CDMA will continue execution starting with the next job or will get +* into idle state. */ -if (job) - restart_addr = job->first_get; +if (next_job) + restart_addr = next_job->first_get;else restart_addr = cdma->last_pos;
drivers/gpu/host1x/hw/cdma_hw.c +0 -13
@@ -40,7 +40,6 @@ static void cdma_timeout_cpu_incr(struct host1x_cdma *cdma, u32 getptr, u32 syncpt_incrs, u32 syncval, u32 nr_slots) { struct host1x *host1x = cdma_to_host1x(cdma); -struct push_buffer *pb = &cdma->push_buffer;unsigned int i;for (i =0 ; i < syncpt_incrs; i++)
@@ -48,18 +47,6 @@ static void cdma_timeout_cpu_incr(struct host1x_cdma *cdma, u32 getptr,/* after CPU incr, ensure shadow is up to date */ host1x_syncpt_load (cdma->timeout.syncpt); - -/* NOP all the PB slots */ -while (nr_slots--) { -u32 *p = (u32 *)(pb->mapped + getptr); - *(p++) =HOST1X_OPCODE_NOP ; - *(p++) =HOST1X_OPCODE_NOP ; -dev_dbg (host1x->dev,"%s: NOP at %pad+%#x\n" , __func__, - &pb->dma, getptr); - getptr = (getptr +8 ) & (pb->size -1 ); - } - -wmb (); }/*
Signature · 1 line
2.18.0
Re: [PATCH v2 3/3] gpu: host1x: Continue CDMA execution starting with a next job
Quoted 66 lines · Dmitry Osipenko
Currently gathers of a hung job are getting NOP'ed and a restarted CDMA executes the NOP'ed gathers. There shouldn't be a reason to not restart CDMA execution starting with a next job, avoiding the unnecessary churning with gathers NOP'ing.
Signed-off-by Dmitry Osipenko <[email protected]>--- drivers/gpu/host1x/cdma.c | 23 +++++++++++------------ drivers/gpu/host1x/hw/cdma_hw.c | 13 ------------- 2 files changed, 11 insertions(+), 25 deletions(-)drivers/gpu/host1x/cdma.c +11
-12@@ -323,7 +323,7 @@ void host1x_cdma_update_sync_queue(struct host1x_cdma *cdma, {struct host1x *host1x =cdma_to_host1x (cdma);u32 restart_addr, syncpt_incrs, syncpt_val; -struct host1x_job *job; +struct host1x_job *job, *next_job =NULL ; syncpt_val =host1x_syncpt_load (cdma->timeout.syncpt);@@ -341,8 +341,13 @@ void host1x_cdma_update_sync_queue(struct host1x_cdma *cdma, __func__);list_for_each_entry (job, &cdma->sync_queue, list) { -if (syncpt_val < job->syncpt_end) +if (syncpt_val < job->syncpt_end) { + +if (!list_is_last (&job->list, &cdma->sync_queue)) + next_job =list_next_entry (job, list); + goto syncpt_incr; + }host1x_job_dump (dev, job); }@@ -355,17 +360,11 @@ void host1x_cdma_update_sync_queue(struct host1x_cdma *cdma,/* * Increment with CPU the remaining syncpts of a partially executed job. * -* Syncpt increments must be done inline with the pushbuffer from a -* GATHER buffer to maintain the order (slots are modified to be a -* GATHER of syncpt incrs). -* -* Note: save in restart_addr the location where the timed out buffer -* started in the PB, so we can start the refetch from there (with the -* modified NOP-ed PB slots). This lets things appear to have completed -* properly for this buffer and resources are freed. +* CDMA will continue execution starting with the next job or will get +* into idle state. */ -if (job) - restart_addr = job->first_get; +if (next_job) + restart_addr = next_job->first_get;else restart_addr = cdma->last_pos;drivers/gpu/host1x/hw/cdma_hw.c +0
-0
drivers/gpu/host1x/hw/cdma_hw.c static void cdma_timeout_cpu_incr(struct host1x_cdma *cdma, u32 getptr, · line 42
Quoted 3 lines
drivers/gpu/host1x/hw/cdma_hw.c +0
-0@@ -40,7 +40,6 @@ static void cdma_timeout_cpu_incr(struct host1x_cdma *cdma, u32 getptr, u32 syncpt_incrs, u32 syncval, u32 nr_slots) { struct host1x *host1x = cdma_to_host1x(cdma);
The *host1x is now unused and should be removed as well, I'll fix it in v3. Mikko, could you take a look at the patches?
Re: [PATCH v2 3/3] gpu: host1x: Continue CDMA execution starting with a next job
In reply to Dmitry OsipenkoReviewed-by Mikko Perttnuen <[email protected]>
Quoted 83 lines · Dmitry Osipenko
Currently gathers of a hung job are getting NOP'ed and a restarted CDMA executes the NOP'ed gathers. There shouldn't be a reason to not restart CDMA execution starting with a next job, avoiding the unnecessary churning with gathers NOP'ing.
Signed-off-by Dmitry Osipenko <[email protected]>--- drivers/gpu/host1x/cdma.c | 23 +++++++++++------------ drivers/gpu/host1x/hw/cdma_hw.c | 13 ------------- 2 files changed, 11 insertions(+), 25 deletions(-)drivers/gpu/host1x/cdma.c +11
-12@@ -323,7 +323,7 @@ void host1x_cdma_update_sync_queue(struct host1x_cdma *cdma, {struct host1x *host1x =cdma_to_host1x (cdma);u32 restart_addr, syncpt_incrs, syncpt_val; -struct host1x_job *job; +struct host1x_job *job, *next_job =NULL ; syncpt_val =host1x_syncpt_load (cdma->timeout.syncpt);@@ -341,8 +341,13 @@ void host1x_cdma_update_sync_queue(struct host1x_cdma *cdma, __func__);list_for_each_entry (job, &cdma->sync_queue, list) { -if (syncpt_val < job->syncpt_end) +if (syncpt_val < job->syncpt_end) { + +if (!list_is_last (&job->list, &cdma->sync_queue)) + next_job =list_next_entry (job, list); + goto syncpt_incr; + }host1x_job_dump (dev, job); }@@ -355,17 +360,11 @@ void host1x_cdma_update_sync_queue(struct host1x_cdma *cdma,/* * Increment with CPU the remaining syncpts of a partially executed job. * -* Syncpt increments must be done inline with the pushbuffer from a -* GATHER buffer to maintain the order (slots are modified to be a -* GATHER of syncpt incrs). -* -* Note: save in restart_addr the location where the timed out buffer -* started in the PB, so we can start the refetch from there (with the -* modified NOP-ed PB slots). This lets things appear to have completed -* properly for this buffer and resources are freed. +* CDMA will continue execution starting with the next job or will get +* into idle state. */ -if (job) - restart_addr = job->first_get; +if (next_job) + restart_addr = next_job->first_get;else restart_addr = cdma->last_pos;drivers/gpu/host1x/hw/cdma_hw.c +0
-6@@ -40,7 +40,6 @@ static void cdma_timeout_cpu_incr(struct host1x_cdma *cdma, u32 getptr, u32 syncpt_incrs, u32 syncval, u32 nr_slots) { struct host1x *host1x = cdma_to_host1x(cdma); -struct push_buffer *pb = &cdma->push_buffer;unsigned int i;for (i =0 ; i < syncpt_incrs; i++)@@ -48,18 +47,6 @@ static void cdma_timeout_cpu_incr(struct host1x_cdma *cdma, u32 getptr,/* after CPU incr, ensure shadow is up to date */ host1x_syncpt_load (cdma->timeout.syncpt); - -/* NOP all the PB slots */ -while (nr_slots--) { -u32 *p = (u32 *)(pb->mapped + getptr); - *(p++) =HOST1X_OPCODE_NOP ;
drivers/gpu/host1x/hw/cdma_hw.c static void cdma_timeout_cpu_incr(struct host1x_cdma *cdma, u32 getptr, · line 52
Quoted 10 lines
drivers/gpu/host1x/hw/cdma_hw.c +0
-7- *(p++) =HOST1X_OPCODE_NOP ; -dev_dbg (host1x->dev,"%s: NOP at %pad+%#x\n" , __func__, - &pb->dma, getptr); - getptr = (getptr +8 ) & (pb->size -1 ); - } - -wmb (); }/*
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 3/3] gpu: host1x: Continue CDMA execution starting with a next job
Quoted 1 line · Mikko Perttunen
Reviewed-by Mikko Perttnuen <[email protected]>
Thank you :)