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

2016-11-23 Thread Robert Bragg 
On Nov 22, 2016 23:49, "Chris Wilson"  wrote:
>
> On Tue, Nov 22, 2016 at 11:32:38PM +, Robert Bragg wrote:
> >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.
>
> Hmm, I thought do_div() was u64 / u64, but no it is u64 / u32. Looks
> like the appropriate function would be div64_u64().
>
> >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?
>
> Seems like picking the right function would help!

Or that, yep. Sounds good to me, thanks.
- Robert

> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

2016-11-22 Thread Chris Wilson 
On Tue, Nov 22, 2016 at 11:32:38PM +, Robert Bragg wrote:
>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.

Hmm, I thought do_div() was u64 / u64, but no it is u64 / u32. Looks
like the appropriate function would be div64_u64().

>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?

Seems like picking the right function would help!
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

2016-11-22 Thread 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 
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 
> Fixes: d79651522e89 ("drm/i915: Enable i915 perf stream for Haswell OA
> unit")
> Signed-off-by: Chris Wilson 
> Cc: Robert Bragg 
> ---
>  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 10ULL * (2ULL << exponent) /
> -   dev_priv->perf.oa.timestamp_frequency;
> +   u64 interval;
> +
> +   interval = 10ULL * (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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

2016-11-22 Thread Chris Wilson 
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 
Fixes: d79651522e89 ("drm/i915: Enable i915 perf stream for Haswell OA unit")
Signed-off-by: Chris Wilson 
Cc: Robert Bragg 
---
 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 10ULL * (2ULL << exponent) /
-   dev_priv->perf.oa.timestamp_frequency;
+   u64 interval;
+
+   interval = 10ULL * (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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx