Re: [PATCH 05/10] drm/i915: Use drm_vblank_count() on gen2 for crc frame count

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

On Wed, Dec 16, 2015 at 11:30:19AM +0100, Daniel Vetter wrote:
> On Mon, Dec 14, 2015 at 06:23:44PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Gen2 doesn't have a hardware frame counter, so let's use the sw
> > counter value instead.
> > 
> > Testcase: igt/kms_pipe_crc_basic/read-crc-pipe-?-frame-sequence
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> I think the better test is skip the testcase if all frame numbers are 0.
> Not sure it's worth it to hack this up.

What's the problem with it? A few extra lines of code?

> 
> But if you disagree I'd just throw out all the max_vblank_count checks and
> unconditionally enable the vblank counter and just always use
> drm_vblank_count. I don't think this can hurt use while we use the CRC,
> since it shouldn't generate more interrupts.

I'd rather not do that since the vblank interrupts might hide some bugs
(eg. missing crc done interrupts). Also I'm not convinced the software
counter isn't racy wrt. the crc done interrupt. Would need to check on
every platform to make sure the vblank interrupt is raised and processed
before the crc done interrupt, or we'd need to switch over to using
the vblank interrupt for crc gathering.

Note that I didn't actually check which order the interrupts fire on
gen2, but this was enough to make the current tests pass at least.

> -Daniel
> 
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c | 11 +++++++++++
> >  drivers/gpu/drm/i915/i915_irq.c     |  5 ++++-
> >  2 files changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 96d6e5de0811..695c69e85374 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -4021,6 +4021,14 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
> >  		if (!entries)
> >  			return -ENOMEM;
> >  
> > +		if (dev->max_vblank_count == 0) {
> > +			ret = drm_vblank_get(dev, pipe);
> > +			if (ret) {
> > +				kfree(entries);
> > +				return ret;
> > +			}
> > +		}
> > +
> >  		/*
> >  		 * When IPS gets enabled, the pipe CRC changes. Since IPS gets
> >  		 * enabled and disabled dynamically based on package C states,
> > @@ -4073,6 +4081,9 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
> >  			hsw_trans_edp_pipe_A_crc_wa(dev, false);
> >  
> >  		hsw_enable_ips(crtc);
> > +
> > +		if (dev->max_vblank_count == 0)
> > +			drm_vblank_put(dev, pipe);
> >  	}
> >  
> >  	return 0;
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 86664d1b3389..37ec8427359a 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -1533,7 +1533,10 @@ static void display_pipe_crc_irq_handler(struct drm_device *dev, enum pipe pipe,
> >  
> >  	entry = &pipe_crc->entries[head];
> >  
> > -	entry->frame = dev->driver->get_vblank_counter(dev, pipe);
> > +	if (dev->max_vblank_count == 0)
> > +		entry->frame = drm_vblank_count(dev, pipe);
> > +	else
> > +		entry->frame = dev->driver->get_vblank_counter(dev, pipe);
> >  	entry->crc[0] = crc0;
> >  	entry->crc[1] = crc1;
> >  	entry->crc[2] = crc2;
> > -- 
> > 2.4.10
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx