On 12/17/2014 10:37 AM, Ville Syrjälä wrote:
On Wed, Dec 17, 2014 at 03:57:51AM +0100, Mario Kleiner wrote:
On 12/15/2014 04:56 PM, Thierry Reding wrote:
From: Thierry Reding <treding@xxxxxxxxxx>
The QXL driver duplicates part of the core's drm_vblank_count(), so it
might as well use the core's variant for the extra goodies.
Signed-off-by: Thierry Reding <treding@xxxxxxxxxx>
---
drivers/gpu/drm/qxl/qxl_drv.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
index 1d9b80c91a15..497024461a3c 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.c
+++ b/drivers/gpu/drm/qxl/qxl_drv.c
@@ -196,11 +196,6 @@ static int qxl_pm_restore(struct device *dev)
return qxl_drm_resume(drm_dev, false);
}
-static u32 qxl_noop_get_vblank_counter(struct drm_device *dev, int crtc)
-{
- return dev->vblank[crtc].count.counter;
-}
-
static int qxl_noop_enable_vblank(struct drm_device *dev, int crtc)
{
return 0;
@@ -231,7 +226,7 @@ static struct drm_driver qxl_driver = {
DRIVER_HAVE_IRQ | DRIVER_IRQ_SHARED,
.load = qxl_driver_load,
.unload = qxl_driver_unload,
- .get_vblank_counter = qxl_noop_get_vblank_counter,
+ .get_vblank_counter = drm_vblank_count,
.enable_vblank = qxl_noop_enable_vblank,
.disable_vblank = qxl_noop_disable_vblank,
Hi
That doesn't really help, although it doesn't hurt either. Just wanted
to point out that both the old and new method implement a no-op. The
get_vblank_counter() driver function is meant to implement a hardware
vblank counter query. It's only use case atm. is to reinitialize the
dev->vblank[crtc].count.counter counter returned by drm_vblank_count().
The most honest implementation if there isn't any way to get a hw vblank
count would be to just "return 0;" - Same net effect, but at least a
marker in the code that there is future work to do.
Yeah 'return 0' is what we do in i915 when there's no hw counter. I did
consider changing it to drm_vblank_count() since that seems to be the
current fad. I was hoping it might allow removing some code from
drm_irq.c, but after some more thought that might not be the case.
I'll probably need to take another look at it.
I think a better solution would be if we wouldn't require
.get_vblank_counter to be non-NULL, don't fake implement it in
kms-drivers which can't do it, and make the drm core deal with lack of
hw counter queries, e.g., by not disabling vblank irqs.
That seems a bit drastic. The current delayed disable
seems quite reasonable to me. The count will remain accurate as long
as the vblank irq is enabled, and if you wait for so long that the
irq gets disabled, well, I don't think a very precise answer was
needed anyway.
Agreed. The 5 second timeout is imho reasonable in practice. I just
meant maybe we just check in the drm if that function is non-NULL, so
drivers are not forced to implement no-op stubs of that function if they
don't actually support it, just to avoid oopses.
I was hunting some bugs in the vblank code recently, and while doing
that I thought that I might change the code to use the timestamp
difference between disable->enable to calculate an approximate number
of vblanks lost and bump the counter appropriately. Didn't try it
yet though, but seems like a reasonable idea when there's no hw
counter. Though some care will be needed when dealing with
drm_vblank_off/on.
Would be an option. Unsure if it is worth it or not in practice.
-mario
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
