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

[Mario Kleiner <mario.kleiner.de-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

On 22.01.13 20:27, Jon Mayo wrote:
> On Tue, Jan 22, 2013 at 11:20 AM, Mario Kleiner
> <mario.kleiner.de-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On 22.01.13 19:39, Lucas Stach wrote:
>>>
>>> Am Dienstag, den 22.01.2013, 18:37 +0100 schrieb Mario Kleiner:
>>>>
>>>> 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-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
>>>>
>>>>
>>>> ... 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 looked this up in the TRM a while ago as I know we have the same
>>> problem in nouveau, but it seems there is no HW vblank counter on Tegra.
>>>
>>> Mario, you know a fair bit more about this than I do, what is the
>>> preferred way of handling this if we are sure we are not able to
>>> implement anything meaningful here? Just return 0?
>>>
>>
>> I think 0 would be better, just to make it clear to readers of the source
>> code that this function is basically unimplemented. For correctness it
>> doesn't matter what you return, drm_vblank_count() is ok as well.
>>
>> If we wanted, we could probably implement a good enough emulated hw-vblank
>> counter, at least on nouveau? If there is some on-chip clock register that
>> is driven by the same hardware oscillator as the crtc's so it doesn't drift,
>> we could store the clock time in the vblank_disable() hook, and some
>> measured duration of a video refresh, wrt. that clock. Then in
>> .get_vblank_counter() calculate how many vblanks have elapsed from
>> (current_clock_time - vblank_off_clock_time) / frame_duration and increment
>> our own private software vblank counter. Something along that line...
>>
>> -mario
>>
>
> Looking through the T30 manuals, I see all the
> CLK_RST_CONTROLLER_CLK_SOURCE_xxx entries that support PLLD as a
> parent are display related. You won't find any timers or counters that
> use the same exact clock. Being out-of-sync is a real possibility, and
> something adaptive would have to be implement to hide the desync
> between a fake and a real vblank once you make the transition.
>
> That said, I think being inaccurate here doesn't have a very high
> cost. Please give me some examples if you disagree though, I'd be
> interested in some good use cases to throw into my test plan.
>
> - Jon
>

Agreed. Fwiw at least i can't think of applications which would need 
stability over more than maybe a couple of seconds or a minute.

-mario