| Message ID | 1450110229-30450-6-git-send-email-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. 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. -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
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
On Wed, Dec 16, 2015 at 02:51:20PM +0200, Ville Syrjälä wrote: > 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? Well the idea of sampling the hw irq counter is that we'd notice when we start missing something. Sampling the baked vblank counter isn't really useful in that regard I think. Otoh we could just sample the official vblank counter, and then a test could schedule a flip for a given frame and check that the crc changes at the right frame (using continuous sampling). That would indeed be useful. This here just looks like a few random changes to appease a fairly arbitrary testcase. -Daniel
On Wed, Dec 16, 2015 at 04:28:36PM +0100, Daniel Vetter wrote: > On Wed, Dec 16, 2015 at 02:51:20PM +0200, Ville Syrjälä wrote: > > 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? > > Well the idea of sampling the hw irq counter is that we'd notice when we > start missing something. Sampling the baked vblank counter isn't really > useful in that regard I think. I think only if we'd also miss the vblank interrupt somehow. > > Otoh we could just sample the official vblank counter, and then a test > could schedule a flip for a given frame and check that the crc changes at > the right frame (using continuous sampling). That would indeed be useful. That's a decent point. Maybe we would want to expose both counters actually and nag if they don't match? That sort of thing could be useful just to validate the vblank code too, but we don't have any interface to expose the hw counter alongside the sw counter for that purpose. > This here just looks like a few random changes to appease a fairly > arbitrary testcase. > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Wed, Dec 16, 2015 at 07:22:25PM +0200, Ville Syrjälä wrote: > On Wed, Dec 16, 2015 at 04:28:36PM +0100, Daniel Vetter wrote: > > On Wed, Dec 16, 2015 at 02:51:20PM +0200, Ville Syrjälä wrote: > > > 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? > > > > Well the idea of sampling the hw irq counter is that we'd notice when we > > start missing something. Sampling the baked vblank counter isn't really > > useful in that regard I think. > > I think only if we'd also miss the vblank interrupt somehow. > > > > > Otoh we could just sample the official vblank counter, and then a test > > could schedule a flip for a given frame and check that the crc changes at > > the right frame (using continuous sampling). That would indeed be useful. > > That's a decent point. Maybe we would want to expose both counters > actually and nag if they don't match? That sort of thing could be useful > just to validate the vblank code too, but we don't have any interface > to expose the hw counter alongside the sw counter for that purpose. Hm yeah that makes sense. Makes the backward compat dance in igt a bit more annoying though. But should be doable by first trying to parse the new layout and then falling back to the old one if it fails. -Daniel
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;