On 02/11/2013 10:13 AM, Thierry Reding wrote: > On Tue, Jan 22, 2013 at 06:37:39PM +0100, Mario Kleiner wrote: >> On 14.01.13 17:05, Thierry Reding wrote: >>> Implement support for the VBLANK IOCTL. Note that Tegra is somewhat >>> special in this case because it doesn't use the generic IRQ support >>> provided by the DRM core (DRIVER_HAVE_IRQ) but rather registers one >>> interrupt handler for each display controller. >>> >>> While at it, clean up the way that interrupts are enabled to ensure >>> that the VBLANK interrupt only gets enabled when required. >>> >>> Signed-off-by: Thierry Reding <thierry.reding at avionic-design.de> >> ... snip ... >> >>> struct drm_driver tegra_drm_driver = { >>> .driver_features = DRIVER_BUS_PLATFORM | DRIVER_MODESET | DRIVER_GEM, >>> .load = tegra_drm_load, >>> @@ -96,6 +136,10 @@ struct drm_driver tegra_drm_driver = { >>> .open = tegra_drm_open, >>> .lastclose = tegra_drm_lastclose, >>> >>> + .get_vblank_counter = drm_vblank_count, >> -> .get_vblank_counter = drm_vblank_count is a no-op. >> >> .get_vblank_counter() is supposed to return some real hardware >> vblank counter value to reinitialize the software vblank counter at >> vbl irq enable time. That software counter gets queried via >> drm_vblank_count(). If you hook this up to drm_vblank_count() it >> essentially returns a constant, frozen vblank count, it has the same >> effect as returning zero or any other constant value -- You lose all >> vblank counter increments during vblank irq off time. The same >> problem is present in nouveau-kms. >> >> I think it would be better to either implement a real hw counter >> query, or some function with a /* TODO: Implement me properly */ >> comment which returns zero, so it is clear something is missing >> here. > I've finally managed to find some time to look into this some more. The > comment atop the drm_driver.get_vblank_counter() field actually suggests > that drivers should set this to drm_vblank_count if no real hardware > counter is supported.
It certainly works fine that way. I just think it hides that some implementation is missing there, whereas an extra no-op function makes that clear to the reader. > Now it seems like we may get functionality to obtain the real VBLANK > counter once the syncpoint support is merged, so maybe we can leave this > as-is until then and replace it with a proper implementation at that > point in time? Perfectly fine with me. ciao, -mario > Alternatively I could use a small wrapper with an explicit comment that > this should be implemented using the upcoming syncpoint support. > > Thierry > > > _______________________________________________ > dri-devel mailing list > dri-devel at lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20130215/e1ee47c7/attachment.html>