Re: [PATCH 04/16] drm/i915: Sample the frame counter instead of a timestamp for CRCs

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Oct 16, 2013 at 04:29:32PM +0300, Ville Syrjälä wrote:
> On Tue, Oct 15, 2013 at 06:55:30PM +0100, Damien Lespiau wrote:
> > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c | 4 ++--
> >  drivers/gpu/drm/i915/i915_drv.h     | 2 +-
> >  drivers/gpu/drm/i915/i915_irq.c     | 8 ++------
> >  3 files changed, 5 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 991abff..58c6fd4 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -1748,14 +1748,14 @@ static int i915_pipe_crc(struct seq_file *m, void *data)
> >  		return 0;
> >  	}
> >  
> > -	seq_puts(m, " timestamp     CRC1     CRC2     CRC3     CRC4     CRC5\n");
> > +	seq_puts(m, "  frame    CRC1     CRC2     CRC3     CRC4     CRC5\n");
> >  	head = atomic_read(&pipe_crc->head);
> >  	tail = atomic_read(&pipe_crc->tail);
> >  
> >  	while (CIRC_CNT(head, tail, INTEL_PIPE_CRC_ENTRIES_NR) >= 1) {
> >  		struct intel_pipe_crc_entry *entry = &pipe_crc->entries[tail];
> >  
> > -		seq_printf(m, "%12u %8x %8x %8x %8x %8x\n", entry->timestamp,
> > +		seq_printf(m, "%8u %8x %8x %8x %8x %8x\n", entry->frame,
> >  			   entry->crc[0], entry->crc[1], entry->crc[2],
> >  			   entry->crc[3], entry->crc[4]);
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 7a1ed3a..cd87919 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1228,7 +1228,7 @@ enum intel_pipe_crc_source {
> >  };
> >  
> >  struct intel_pipe_crc_entry {
> > -	uint32_t timestamp;
> > +	uint32_t frame;
> >  	uint32_t crc[5];
> >  };
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 73d76af..0b21828 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -1195,8 +1195,7 @@ static void ivb_pipe_crc_update(struct drm_device *dev, enum pipe pipe)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[pipe];
> >  	struct intel_pipe_crc_entry *entry;
> > -	ktime_t now;
> > -	int ts, head, tail;
> > +	int head, tail;
> >  
> >  	head = atomic_read(&pipe_crc->head);
> >  	tail = atomic_read(&pipe_crc->tail);
> > @@ -1208,10 +1207,7 @@ static void ivb_pipe_crc_update(struct drm_device *dev, enum pipe pipe)
> >  
> >  	entry = &pipe_crc->entries[head];
> >  
> > -	now = ktime_get();
> > -	ts = ktime_to_us(now);
> > -
> > -	entry->timestamp = ts;
> > +	entry->frame = I915_READ(PIPEFRAME(pipe));
> 
> BTW that's the wrong register for ctg+. It should be PIPE_FRMCOUNT_GM45.
> But it would be better if you just call
> dev->driver->get_vblank_counter(). Then you get the correct answer for
> everything except gen2, and for gen2 we could implement a software
> frame counter if need be.

Hm, maybe we should even cobble it out of the drm vblank support code, to
make sure that the vblank frame numbers from the crc debugfs align with
timestamps for pageflips. But this is definitely a good idea. And afaik
gen2 doesn't have crc support (not even gen3 iirc). I'll do this in my
bikeshed series.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch