[Intel-gfx] [PATCH] drm/i915/perf: Wrap 64bit divides in do_div()

[Robert Bragg]

Thanks for sending out. It looked good to me, but testing shows a 'divide
error'.

I haven't double checked, but I think it's because the max OA exponent (31)
converted to nanoseconds is > UINT32_MAX with the lower 32bits zero and the
do_div denominator argument is only 32bit.

It corresponds to a 5 minute period which is a bit silly, so we could
reduce the max exponent. A period of UINT32_MAX is about 4 seconds where I
can't currently think of a good use case for such a low frequency.

Instead of changing the max OA exponent (where the relationship to the
period changes for gen9 and may become fuzzy if we start training our view
of the gpu timestamp frequency instead of using constants) maybe we should
set an early limit on an exponent resulting in a period > UINT32_MAX?

- Robert


On Tue, Nov 22, 2016 at 9:14 PM, Chris Wilson <chris at chris-wilson.co.uk>
wrote:

> Just a couple of naked 64bit divides causing link errors on 32bit
> builds, with:
>
>         ERROR: "__udivdi3" [drivers/gpu/drm/i915/i915.ko] undefined!
>
> Reported-by: kbuild test robot <fengguang.wu at intel.com>
> Fixes: d79651522e89 ("drm/i915: Enable i915 perf stream for Haswell OA
> unit")
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Robert Bragg <robert at sixbynine.org>
> ---
>  drivers/gpu/drm/i915/i915_perf.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_perf.c
> b/drivers/gpu/drm/i915/i915_perf.c
> index 95512824922b..7d00532ae010 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -974,8 +974,12 @@ static void i915_oa_stream_disable(struct
> i915_perf_stream *stream)
>
>  static u64 oa_exponent_to_ns(struct drm_i915_private *dev_priv, int
> exponent)
>  {
> -       return 1000000000ULL * (2ULL << exponent) /
> -               dev_priv->perf.oa.timestamp_frequency;
> +       u64 interval;
> +
> +       interval = 1000000000ULL * (2ULL << exponent);
> +       do_div(interval, dev_priv->perf.oa.timestamp_frequency);
> +
> +       return interval;
>  }
>
>  static const struct i915_perf_stream_ops i915_oa_stream_ops = {
> @@ -1051,16 +1055,17 @@ static int i915_oa_stream_init(struct
> i915_perf_stream *stream,
>
>         dev_priv->perf.oa.periodic = props->oa_periodic;
>         if (dev_priv->perf.oa.periodic) {
> -               u64 period_ns = oa_exponent_to_ns(dev_priv,
> -
>  props->oa_period_exponent);
> +               u64 margin;
>
>                 dev_priv->perf.oa.period_exponent =
> props->oa_period_exponent;
>
>                 /* See comment for OA_TAIL_MARGIN_NSEC for details
>                  * about this tail_margin...
>                  */
> -               dev_priv->perf.oa.tail_margin =
> -                       ((OA_TAIL_MARGIN_NSEC / period_ns) + 1) *
> format_size;
> +               margin = OA_TAIL_MARGIN_NSEC;
> +               do_div(margin,
> +                      oa_exponent_to_ns(dev_priv,
> props->oa_period_exponent));
> +               dev_priv->perf.oa.tail_margin = (margin + 1) * format_size;
>         }
>
>         if (stream->ctx) {
> --
> 2.10.2
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20161122/f15cc202/attachment-0001.html>