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@xxxxxxxxxx>
> ---
> 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
Attachment:
signature.asc
Description: PGP signature
