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

2013-02-15 Thread Mario Kleiner

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.red...@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


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


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

2013-02-11 Thread Thierry Reding
On Wed, Jan 23, 2013 at 10:02:20AM +0200, Terje Bergström wrote:
 On 22.01.2013 21:59, Mario Kleiner wrote:
  The current swap scheduling is based on having an accurate software 
  vblank counter. Atm. that counter is maintained in software, incremented 
  during vblank irq. At irq off - on transition we need a hw counter to 
  reinitialize. And there is a timeout between dropping the last reference 
  to a counter via drm_vblank_put() and actually disabling the vblank irq. 
  If the timeout is long enough and a timing sensitive app is aware that 
  vblank counters may be inaccurate/unreliable over long time periods, it 
  can work around the problem.
 
 We have a hardware counter that can be used as VBLANK counter - host1x
 sync point register numbers 26 and 27. tegradrm already sets them up in
 dc init (DC_CMD_CONT_SYNCPT_VSYNC). Basic syncpt support (read, wait) is
 part of my patch set to implement 2D acceleration.

Are the syncpoints incremented even if the VBLANK interrupts are turned
off? If that's the case they could indeed be used as a hardware counter
rather than the fake approach used right now.

Maybe we should leave the code as it is right now and fix that up once
the syncpoint support has been merged.

Thierry


pgpPLP3mU2EF1.pgp
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel


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

2013-02-11 Thread Thierry Reding
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.red...@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.

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?

Alternatively I could use a small wrapper with an explicit comment that
this should be implemented using the upcoming syncpoint support.

Thierry


pgpIljOxVCLqi.pgp
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel


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

2013-01-22 Thread Lucas Stach
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.red...@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 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?

Regards,
Lucas


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


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

2013-01-22 Thread Mario Kleiner

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.red...@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 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

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


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

2013-01-22 Thread Mario Kleiner

On 22.01.13 20:27, Jon Mayo wrote:

On Tue, Jan 22, 2013 at 11:20 AM, Mario Kleiner
mario.kleiner...@gmail.com 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.red...@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 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

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


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

2013-01-22 Thread 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.red...@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.


thanks,
-mario


+   .enable_vblank = tegra_drm_enable_vblank,
+   .disable_vblank = tegra_drm_disable_vblank,
+
.gem_free_object = drm_gem_cma_free_object,
.gem_vm_ops = drm_gem_cma_vm_ops,
.dumb_create = drm_gem_cma_dumb_create,
diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h

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


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

2013-01-22 Thread Jon Mayo
On Tue, Jan 22, 2013 at 10:39 AM, Lucas Stach d...@lynxeye.de 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.red...@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 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?

 Regards,
 Lucas



In my branch for the old non-DRM version of the tegra driver, I clock
gate and power gate display when using a one-shot smart panel. So not
only are there no more IRQs, but even if Tegra had a hardware vblank
counter it would also be dead too. (it doesn't have one, but I could
make the case to add one in the next chip if we could actually make
use of it, given my previous statement, I don't think it will help)

How badly do people need this feature? Because I really do think smart
panels are going to been the norm in a few years. A bit off-topic to
Thierry's submission, but I'd really like to discourage apps from
relying on this feature, does anyone else agree?

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


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

2013-01-22 Thread Jon Mayo
On Tue, Jan 22, 2013 at 11:20 AM, Mario Kleiner
mario.kleiner...@gmail.com 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.red...@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 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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel


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

2013-01-22 Thread Mario Kleiner



On 22.01.13 19:49, Jon Mayo wrote:

On Tue, Jan 22, 2013 at 10:39 AM, Lucas Stach d...@lynxeye.de 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.red...@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 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?

Regards,
Lucas




In my branch for the old non-DRM version of the tegra driver, I clock
gate and power gate display when using a one-shot smart panel. So not
only are there no more IRQs, but even if Tegra had a hardware vblank
counter it would also be dead too. (it doesn't have one, but I could
make the case to add one in the next chip if we could actually make
use of it, given my previous statement, I don't think it will help)

How badly do people need this feature? Because I really do think smart
panels are going to been the norm in a few years.


How do smart panels work? Any reference to learn about these?

 A bit off-topic to

Thierry's submission, but I'd really like to discourage apps from
relying on this feature, does anyone else agree?


The current swap scheduling is based on having an accurate software 
vblank counter. Atm. that counter is maintained in software, incremented 
during vblank irq. At irq off - on transition we need a hw counter to 
reinitialize. And there is a timeout between dropping the last reference 
to a counter via drm_vblank_put() and actually disabling the vblank irq. 
If the timeout is long enough and a timing sensitive app is aware that 
vblank counters may be inaccurate/unreliable over long time periods, it 
can work around the problem.


My app, e.g., which is a very timing sensitive scientific application 
toolkit, only assumes that vblank counts are consistent over a short 
period of time. It queries the vblank count and time as a baseline, 
calculates a target vblank count for swap from it and then schedules the 
swap for that target count. If vblank irq's stay active at least between 
the query call and scheduling a swap, all is good, so a timeout to irq 
disable of a couple of video refresh cycles would be safe, even if a 
driver doesn't have a working hw counter.


I think at least some media-players and some of the open source vdpau or 
vaapi implementations and maybe some compositors may rely on this as 
well for audio-video sync etc.


If the vblank disable mechanism is too aggressive in its power saving (= 
too short timeout) or the app is very trusting of the specs being 
robustly implemented it will go wrong.


It's a tradeoff between power-saving and robustness of the implementation.

The other way around this would be to have some new kernel api that 
executes swaps based on a target system time instead of a target vblank 
count. I assume that, e.g., vdpau's presentation api uses time-based 
scheduling for that reason, to avoid dependency on vblank counters.


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


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

2013-01-22 Thread Terje Bergström
On 22.01.2013 21:59, Mario Kleiner wrote:
 The current swap scheduling is based on having an accurate software 
 vblank counter. Atm. that counter is maintained in software, incremented 
 during vblank irq. At irq off - on transition we need a hw counter to 
 reinitialize. And there is a timeout between dropping the last reference 
 to a counter via drm_vblank_put() and actually disabling the vblank irq. 
 If the timeout is long enough and a timing sensitive app is aware that 
 vblank counters may be inaccurate/unreliable over long time periods, it 
 can work around the problem.

We have a hardware counter that can be used as VBLANK counter - host1x
sync point register numbers 26 and 27. tegradrm already sets them up in
dc init (DC_CMD_CONT_SYNCPT_VSYNC). Basic syncpt support (read, wait) is
part of my patch set to implement 2D acceleration.

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