On Mon, Jan 11, 2021 at 03:00:03PM +0200, Mikko Perttunen wrote:
Make syncpoint expiration checks always use the same logic used by the hardware. This ensures that there are no race conditions that could occur because of the hardware triggering a syncpoint interrupt and then the driver disagreeing.
One situation where this could occur is if a job incremented a syncpoint too many times -- then the hardware would trigger an interrupt, but the driver would assume that a syncpoint value greater than the syncpoint's max value is in the future, and not clean up the job.
Signed-off-by: Mikko Perttunen mperttunen@nvidia.com
drivers/gpu/host1x/syncpt.c | 51 ++----------------------------------- 1 file changed, 2 insertions(+), 49 deletions(-)
diff --git a/drivers/gpu/host1x/syncpt.c b/drivers/gpu/host1x/syncpt.c index e48b4595cf53..9ccdf7709946 100644 --- a/drivers/gpu/host1x/syncpt.c +++ b/drivers/gpu/host1x/syncpt.c @@ -306,59 +306,12 @@ EXPORT_SYMBOL(host1x_syncpt_wait); bool host1x_syncpt_is_expired(struct host1x_syncpt *sp, u32 thresh) { u32 current_val;
u32 future_val;
smp_rmb();
current_val = (u32)atomic_read(&sp->min_val);
future_val = (u32)atomic_read(&sp->max_val);
/* Note the use of unsigned arithmetic here (mod 1<<32).
** c = current_val = min_val = the current value of the syncpoint.* t = thresh = the value we are checking* f = future_val = max_val = the value c will reach when all* outstanding increments have completed.** Note that c always chases f until it reaches f.** Dtf = (f - t)* Dtc = (c - t)** Consider all cases:** A) .....c..t..f..... Dtf < Dtc need to wait* B) .....c.....f..t.. Dtf > Dtc expired* C) ..t..c.....f..... Dtf > Dtc expired (Dct very large)** Any case where f==c: always expired (for any t). Dtf == Dcf* Any case where t==c: always expired (for any f). Dtf >= Dtc (because Dtc==0)* Any case where t==f!=c: always wait. Dtf < Dtc (because Dtf==0,* Dtc!=0)** Other cases:** A) .....t..f..c..... Dtf < Dtc need to wait* A) .....f..c..t..... Dtf < Dtc need to wait* A) .....f..t..c..... Dtf > Dtc expired** So:* Dtf >= Dtc implies EXPIRED (return true)* Dtf < Dtc implies WAIT (return false)** Note: If t is expired then we *cannot* wait on it. We would wait* forever (hang the system).** Note: do NOT get clever and remove the -thresh from both sides. It* is NOT the same.** If future valueis zero, we have a client managed sync point. In that* case we do a direct comparison.*/if (!host1x_syncpt_client_managed(sp))
return future_val - thresh >= current_val - thresh;else
return (s32)(current_val - thresh) >= 0;
- return ((current_val - thresh) & 0x80000000U) == 0U;
Heh... now I finally understand what this is supposed to do. =)
Nice one.
Thierry