[Mesa-dev] [PATCH 5/7] i965/fs: move the fs_reg::smear() from get_timestamp() to the callers

[Connor Abbott]

On Tue, Oct 20, 2015 at 1:00 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> We're about to reuse get_timestamp() for the nir_intrinsic_shader_clock.
> In the latter the generalisation does not apply, so move the smear()
> where needed. This also makes the function analogous to the vec4 one.
>
> v2: Tweak the comment - The caller -> We (Matt, Connor).
>
> Signed-off-by: Emil Velikov <emil.l.velikov at gmail.com>
> ---
>
> Connor,
>
> The diff might be a bit hard to read, but the patch does remove the
> comment from get_timestamp(), If you guys prefer I can also drop it from
> shader_time_begin() and(?) delay the smear until emit_shader_time_end().

You can't delay it until emit_shader_time_end() -- both functions are
using the smear, so both need to use it. I think we don't need to give
the rationale for why we only use the low 32 bits more than once,
though. See below for what I'd do wrt the comments.

>
> -Emil
>
>  src/mesa/drivers/dri/i965/brw_fs.cpp | 32 ++++++++++++++++++++++----------
>  1 file changed, 22 insertions(+), 10 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
> index a2fd441..93bb55d 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> @@ -521,7 +521,15 @@ fs_visitor::get_timestamp(const fs_builder &bld)
>      */
>     bld.group(4, 0).exec_all().MOV(dst, ts);
>
> -   /* The caller wants the low 32 bits of the timestamp.  Since it's running
> +   return dst;
> +}
> +
> +void
> +fs_visitor::emit_shader_time_begin()
> +{
> +   shader_start_time = get_timestamp(bld.annotate("shader time start"));
> +
> +   /* We want only the low 32 bits of the timestamp.  Since it's running
>      * at the GPU clock rate of ~1.2ghz, it will roll over every ~3 seconds,
>      * which is plenty of time for our purposes.  It is identical across the
>      * EUs, but since it's tracking GPU core speed it will increment at a
> @@ -531,15 +539,7 @@ fs_visitor::get_timestamp(const fs_builder &bld)
>      * else that might disrupt timing) by setting smear to 2 and checking if
>      * that field is != 0.
>      */
> -   dst.set_smear(0);
> -
> -   return dst;
> -}
> -
> -void
> -fs_visitor::emit_shader_time_begin()
> -{
> -   shader_start_time = get_timestamp(bld.annotate("shader time start"));
> +   shader_start_time.set_smear(0);
>  }
>
>  void
> @@ -553,6 +553,18 @@ fs_visitor::emit_shader_time_end()
>
>     fs_reg shader_end_time = get_timestamp(ibld);
>
> +   /* We want only the low 32 bits of the timestamp.  Since it's running
> +    * at the GPU clock rate of ~1.2ghz, it will roll over every ~3 seconds,
> +    * which is plenty of time for our purposes.  It is identical across the
> +    * EUs, but since it's tracking GPU core speed it will increment at a
> +    * varying rate as render P-states change.
> +    *
> +    * The caller could also check if render P-states have changed (or anything

The caller => We

Also, this sentence is only relevant for emit_shader_time_end(), since
we only care if the measurement was invalidated during the period
we're measuring, so we can delete it from emit_shader_time_begin(). In
turn, you can summarize the first paragraph here with something like
"We only use the low 32 bits of the timestamp (see
emit_shader_time_begin())". With that bikeshedding aside,

Reviewed-by: Connor Abbott <cwabbott0 at gmail.com>

> +    * else that might disrupt timing) by setting smear to 2 and checking if
> +    * that field is != 0.
> +    */
> +   shader_end_time.set_smear(0);
> +
>     /* Check that there weren't any timestamp reset events (assuming these
>      * were the only two timestamp reads that happened).
>      */
> --
> 2.6.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev