[Beignet] [PATCH] Fix cl_event_get_timestamp for submit and queued

[michael.j.ferguson at L-3com.com]

commit e14b07d538140a5751fc1b978442a4a9ff424ee1
Author: Michael Ferguson <michael.j.ferguson at l-3com.com>
Date:   Fri May 30 11:32:36 2014 -0600

    Fix cl_event_get_timestamp for submit and queued
    
    The cl_gpgpu_event_get_gpu_cur_timestamp function did not apply the same logic as the cl_gpgpu_event_get_exec_timestamp regarding the timestamp counter on the Baytrail, which resulted in a bogus GPU current timestamp.
    
    Tests on the Baytrail-I E3827 indicated the following clock values in the profiling_exec test before this patch:
    
        queued = 1920
        submit = 1920
        start  = 2762442307840
        end    = 2762442351360
    
    Obviously these values were not correct for the queued and submit counters. After applying this patch the values in the profiling_exec test indicated:
    
        queued = 320306542080
        submit = 320306617600
        start  = 320308817920
        end    = 320308857600

diff --git a/src/intel/intel_gpgpu.c b/src/intel/intel_gpgpu.c
index a1bd672..568d96c 100644
--- a/src/intel/intel_gpgpu.c
+++ b/src/intel/intel_gpgpu.c
@@ -1172,8 +1172,17 @@ intel_gpgpu_event_get_gpu_cur_timestamp(intel_gpgpu_t* gpgpu, uint64_t* ret_ts)
   drm_intel_reg_read(bufmgr, TIMESTAMP_ADDR, &result);
   if (IS_HASWELL(gpgpu->drv->device_id)) {
     result = result & 0x0000000FFFFFFFFF;
+  } else if (IS_BAYTRAIL_T(gpgpu->drv->device_id)) {
+    /* According to BSpec, the timestamp counter should be 36 bits,
+       but comparing to the timestamp counter from IO control reading,
+       we find the first 4 bits seems to be fake. In order to keep the
+       timestamp counter conformable, we just skip the first 4 bits.
+     */
+    result = (result & 0x0FFFFFFFF) << 4;
   } else {
-    result = result & 0xFFFFFFFFF0000000;
+    /* We could mask result by 0xFFFFFFFFF0000000 before shifting, but that is
+       unnecessary because we shift off the lower 28 bits anyway.
+     */
     result = result >> 28;
   }
   result *= 80;



-----Original Message-----
From: Zhigang Gong [mailto:zhigang.gong at linux.intel.com] 
Sent: Sunday, June 08, 2014 11:13 PM
To: He Junyan
Cc: Ferguson, Michael J @ CSG - CSW; beignet at lists.freedesktop.org
Subject: Re: [Beignet] [PATCH] Fix cl_event_get_timestamp for submit and queued

Junyan, the only supported baytrail pci id is:
#define PCI_CHIP_BAYTRAIL_T 0x0F31
You may consider to refine this patch and submit a new version.

Thanks.

On Thu, Jun 05, 2014 at 12:47:28PM +0800, He Junyan wrote:
> So, you apply the same logic in the function 
> intel_gpgpu_event_get_exec_timestamp here.
> 
> BSpec really have some mistakes.
> But for most of the IVB platform,
> >-    result = result & 0xFFFFFFFFF0000000;
> >-    result = result >> 28;
> works very well.
> 
> So I think if you want to correct this, you should add the PCIID check 
> like HSW.
>  IS_XXXX(gpgpu->drv->device_id) should be added here.
> 
> If you do not know how to do it, please notify the PCIID of your 
> Baytrail-I E3827
> 
> 
> 
> On Wed, 2014-06-04 at 16:15 +0000, michael.j.ferguson at L-3com.com wrote:
> > commit a9ab94503348068579e8e816e80eb62598fd7f5f
> > Author: Michael Ferguson <michael.j.ferguson at l-3com.com>
> > Date:   Fri May 30 11:32:36 2014 -0600
> > 
> >     Fix cl_event_get_timestamp for submit and queued
> >     
> >     The cl_gpgpu_event_get_gpu_cur_timestamp function did not apply the same logic as the cl_gpgpu_event_get_exec_timestamp regarding the timestamp counter on the Baytrail, which resulted in a bogus GPU current timestamp.
> >     
> >     Tests on the Baytrail-I E3827 indicated the following clock values in the profiling_exec test before this patch:
> >     
> >         queued = 1920
> >         submit = 1920
> >         start  = 2762442307840
> >         end    = 2762442351360
> >     
> >     Obviously these values were not correct for the queued and submit counters. After applying this patch the values in the profiling_exec test indicated:
> >     
> >         queued = 320306542080
> >         submit = 320306617600
> >         start  = 320308817920
> >         end    = 320308857600
> > 
> > diff --git a/src/intel/intel_gpgpu.c b/src/intel/intel_gpgpu.c index 
> > bde9bd5..22e04f5 100644
> > --- a/src/intel/intel_gpgpu.c
> > +++ b/src/intel/intel_gpgpu.c
> > @@ -1138,8 +1138,12 @@ intel_gpgpu_event_get_gpu_cur_timestamp(intel_gpgpu_t* gpgpu, uint64_t* ret_ts)
> >    if (IS_HASWELL(gpgpu->drv->device_id)) {
> >      result = result & 0x0000000FFFFFFFFF;
> >    } else {
> > -    result = result & 0xFFFFFFFFF0000000;
> > -    result = result >> 28;
> > +    /* According to BSpec, the timestamp counter should be 36 bits,
> > +       but comparing to the timestamp counter from IO control reading,
> > +       we find the first 4 bits seems to be fake. In order to keep the
> > +       timestamp counter conformable, we just skip the first 4 bits.
> > +     */
> > +    result = (result & 0x0FFFFFFFF) << 4;
> >    }
> >    result *= 80;
> > 
> > _______________________________________________
> > Beignet mailing list
> > Beignet at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/beignet
> 
> 
> 
> _______________________________________________
> Beignet mailing list
> Beignet at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/beignet