Re: [PATCH v2 4/5] drm/tegra: Implement VBLANK support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxxxx>
... 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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux