On Tue, Oct 20, 2015 at 1:00 PM, Emil Velikov <emil.l.veli...@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.veli...@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 <cwabbo...@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@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