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

[Connor Abbott]

On Mon, Oct 19, 2015 at 2:29 PM, Matt Turner <mattst88 at gmail.com> wrote:
> On Mon, Oct 19, 2015 at 7:45 AM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>> We're about to reuse get_timestamp() for the nir_intrinsic_shader_clock.
>> The in the latter the generalisation does not apply, so move the smear()
>> where needed. This also makes the function analogous to the vec4 one.
>>
>> Signed-off-by: Emil Velikov <emil.l.velikov at gmail.com>
>> ---
>>  src/mesa/drivers/dri/i965/brw_fs.cpp | 30 +++++++++++++++++++++---------
>>  1 file changed, 21 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> index a2fd441..81a6a29 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> @@ -521,6 +521,14 @@ fs_visitor::get_timestamp(const fs_builder &bld)
>>      */
>>     bld.group(4, 0).exec_all().MOV(dst, ts);
>>
>> +   return dst;
>> +}
>> +
>> +void
>> +fs_visitor::emit_shader_time_begin()
>> +{
>> +   shader_start_time = get_timestamp(bld.annotate("shader time start"));
>> +
>>     /* The caller wants 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
>> @@ -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);
>>
>> +   /* The caller wants 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
>> +    * else that might disrupt timing) by setting smear to 2 and checking if
>> +    * that field is != 0.
>> +    */
>
> I wouldn't bother copying the comment. It's immediately above this
> function and very visible if reading this code.

Wouldn't it be better to leave the comment here, replacing "The
caller" with "we" and delete it from get_timestamp though? Especially
since get_timestamp() is going to be used from the shader_clock
implementation which actually wants the high 32 bits too.

> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev