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

2015-10-20 Thread Connor Abbott 
On Tue, Oct 20, 2015 at 1:00 PM, Emil Velikov  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 
> ---
>
> 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.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 )
>  * 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 

> +* 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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2015-10-20 Thread Emil Velikov 
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 
---

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().

-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.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 )
 * 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
+* 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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2015-10-19 Thread Emil Velikov 
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 
---
 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.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 )
 * 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.
+*/
+   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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2015-10-19 Thread Matt Turner 
On Mon, Oct 19, 2015 at 7:45 AM, Emil Velikov  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 
> ---
>  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.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 )
>  * 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.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2015-10-19 Thread Connor Abbott 
On Mon, Oct 19, 2015 at 2:29 PM, Matt Turner  wrote:
> On Mon, Oct 19, 2015 at 7:45 AM, Emil Velikov  
> 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 
>> ---
>>  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.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 )
>>  * 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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2015-10-19 Thread Matt Turner 
On Mon, Oct 19, 2015 at 11:55 AM, Connor Abbott  wrote:
> On Mon, Oct 19, 2015 at 2:29 PM, Matt Turner  wrote:
>> On Mon, Oct 19, 2015 at 7:45 AM, Emil Velikov  
>> 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 
>>> ---
>>>  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.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 )
>>>  * 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.

Yep, that seems like a good plan.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev