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

[Mario Kleiner <mario.kleiner.de@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2792 bytes --]

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


[-- Attachment #1.2: Type: text/html, Size: 4131 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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