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 at nvidia.com>
>>>
>>> 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 at nvidia.com>
>>> ---
>>> 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
