| Message ID | 20210111130019.3515669-6-mperttunen@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Host1x/TegraDRM UAPI | expand |
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
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; } int host1x_syncpt_init(struct host1x *host)
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(-)