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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Oct 16, 2013 at 03:51:40PM +0200, Daniel Vetter wrote:
> 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.

I see crc registers in both gen2 and 3 bspec.

-- 
Ville Syrjälä
Intel OTC