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

[Robert Bragg <robert@xxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxx>
Fixes: d79651522e89 ("drm/i915: Enable i915 perf stream for Haswell OA unit")
Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Cc: Robert Bragg <robert@xxxxxxxxxxxxx>
---
 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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx