| Message ID | 1358179560-26799-5-git-send-email-thierry.reding@avionic-design.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. 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
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@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
On Tue, Jan 22, 2013 at 10:39 AM, Lucas Stach <dev@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.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 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
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@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
On Tue, Jan 22, 2013 at 11:20 AM, Mario Kleiner <mario.kleiner.de@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.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 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
On 22.01.13 19:49, Jon Mayo wrote: > On Tue, Jan 22, 2013 at 10:39 AM, Lucas Stach <dev@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.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 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
On 22.01.13 20:27, Jon Mayo wrote: > On Tue, Jan 22, 2013 at 11:20 AM, Mario Kleiner > <mario.kleiner.de@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.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 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
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
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
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. 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
On 11.02.2013 01:08, Thierry Reding wrote: > 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. Yes, the sync point increment signal to host1x is independent of VBLANK interrupt. Definitely not needed yet, so that was a comment for future improvement. Terje
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
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c index 8dd7d8a..3aa7ccc 100644 --- a/drivers/gpu/drm/tegra/dc.c +++ b/drivers/gpu/drm/tegra/dc.c @@ -135,6 +135,32 @@ static int tegra_dc_set_base(struct tegra_dc *dc, int x, int y, return 0; } +void tegra_dc_enable_vblank(struct tegra_dc *dc) +{ + unsigned long value, flags; + + spin_lock_irqsave(&dc->lock, flags); + + value = tegra_dc_readl(dc, DC_CMD_INT_MASK); + value |= VBLANK_INT; + tegra_dc_writel(dc, value, DC_CMD_INT_MASK); + + spin_unlock_irqrestore(&dc->lock, flags); +} + +void tegra_dc_disable_vblank(struct tegra_dc *dc) +{ + unsigned long value, flags; + + spin_lock_irqsave(&dc->lock, flags); + + value = tegra_dc_readl(dc, DC_CMD_INT_MASK); + value &= ~VBLANK_INT; + tegra_dc_writel(dc, value, DC_CMD_INT_MASK); + + spin_unlock_irqrestore(&dc->lock, flags); +} + static const struct drm_crtc_funcs tegra_crtc_funcs = { .set_config = drm_crtc_helper_set_config, .destroy = drm_crtc_cleanup, @@ -375,6 +401,8 @@ static int tegra_crtc_mode_set(struct drm_crtc *crtc, unsigned long div, value; int err; + drm_vblank_pre_modeset(crtc->dev, dc->pipe); + err = tegra_crtc_setup_clk(crtc, mode, &div); if (err) { dev_err(dc->dev, "failed to setup clock for CRTC: %d\n", err); @@ -476,31 +504,23 @@ static void tegra_crtc_prepare(struct drm_crtc *crtc) tegra_dc_writel(dc, value, DC_DISP_DISP_MEM_HIGH_PRIORITY_TIMER); value = VBLANK_INT | WIN_A_UF_INT | WIN_B_UF_INT | WIN_C_UF_INT; - tegra_dc_writel(dc, value, DC_CMD_INT_MASK); - - value = VBLANK_INT | WIN_A_UF_INT | WIN_B_UF_INT | WIN_C_UF_INT; tegra_dc_writel(dc, value, DC_CMD_INT_ENABLE); + + value = WIN_A_UF_INT | WIN_B_UF_INT | WIN_C_UF_INT; + tegra_dc_writel(dc, value, DC_CMD_INT_MASK); } static void tegra_crtc_commit(struct drm_crtc *crtc) { struct tegra_dc *dc = to_tegra_dc(crtc); - unsigned long update_mask; unsigned long value; - update_mask = GENERAL_ACT_REQ | WIN_A_ACT_REQ; - - tegra_dc_writel(dc, update_mask << 8, DC_CMD_STATE_CONTROL); - - value = tegra_dc_readl(dc, DC_CMD_INT_ENABLE); - value |= FRAME_END_INT; - tegra_dc_writel(dc, value, DC_CMD_INT_ENABLE); + value = GENERAL_ACT_REQ | WIN_A_ACT_REQ | + GENERAL_UPDATE | WIN_A_UPDATE; - value = tegra_dc_readl(dc, DC_CMD_INT_MASK); - value |= FRAME_END_INT; - tegra_dc_writel(dc, value, DC_CMD_INT_MASK); + tegra_dc_writel(dc, value, DC_CMD_STATE_CONTROL); - tegra_dc_writel(dc, update_mask, DC_CMD_STATE_CONTROL); + drm_vblank_post_modeset(crtc->dev, dc->pipe); } static void tegra_crtc_load_lut(struct drm_crtc *crtc) @@ -517,7 +537,7 @@ static const struct drm_crtc_helper_funcs tegra_crtc_helper_funcs = { .load_lut = tegra_crtc_load_lut, }; -static irqreturn_t tegra_drm_irq(int irq, void *data) +static irqreturn_t tegra_dc_irq(int irq, void *data) { struct tegra_dc *dc = data; unsigned long status; @@ -862,7 +882,7 @@ static int tegra_dc_drm_init(struct host1x_client *client, dev_err(dc->dev, "debugfs setup failed: %d\n", err); } - err = devm_request_irq(dc->dev, dc->irq, tegra_drm_irq, 0, + err = devm_request_irq(dc->dev, dc->irq, tegra_dc_irq, 0, dev_name(dc->dev), dc); if (err < 0) { dev_err(dc->dev, "failed to request IRQ#%u: %d\n", dc->irq, @@ -911,6 +931,7 @@ static int tegra_dc_probe(struct platform_device *pdev) if (!dc) return -ENOMEM; + spin_lock_init(&dc->lock); INIT_LIST_HEAD(&dc->list); dc->dev = &pdev->dev; diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c index 3a503c9..62f8121 100644 --- a/drivers/gpu/drm/tegra/drm.c +++ b/drivers/gpu/drm/tegra/drm.c @@ -40,6 +40,10 @@ static int tegra_drm_load(struct drm_device *drm, unsigned long flags) if (err < 0) return err; + err = drm_vblank_init(drm, drm->mode_config.num_crtc); + if (err < 0) + return err; + err = tegra_drm_fb_init(drm); if (err < 0) return err; @@ -89,6 +93,42 @@ static const struct file_operations tegra_drm_fops = { .llseek = noop_llseek, }; +static struct drm_crtc *tegra_crtc_from_pipe(struct drm_device *drm, int pipe) +{ + struct drm_crtc *crtc; + + list_for_each_entry(crtc, &drm->mode_config.crtc_list, head) { + struct tegra_dc *dc = to_tegra_dc(crtc); + + if (dc->pipe == pipe) + return crtc; + } + + return NULL; +} + +static int tegra_drm_enable_vblank(struct drm_device *drm, int pipe) +{ + struct drm_crtc *crtc = tegra_crtc_from_pipe(drm, pipe); + struct tegra_dc *dc = to_tegra_dc(crtc); + + if (!crtc) + return -ENODEV; + + tegra_dc_enable_vblank(dc); + + return 0; +} + +static void tegra_drm_disable_vblank(struct drm_device *drm, int pipe) +{ + struct drm_crtc *crtc = tegra_crtc_from_pipe(drm, pipe); + struct tegra_dc *dc = to_tegra_dc(crtc); + + if (crtc) + tegra_dc_disable_vblank(dc); +} + 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, + .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 index 835f9a3..ca742b2 100644 --- a/drivers/gpu/drm/tegra/drm.h +++ b/drivers/gpu/drm/tegra/drm.h @@ -80,6 +80,7 @@ struct tegra_output; struct tegra_dc { struct host1x_client client; + spinlock_t lock; struct host1x *host1x; struct device *dev; @@ -146,6 +147,8 @@ struct tegra_dc_window { extern unsigned int tegra_dc_format(uint32_t format); extern int tegra_dc_setup_window(struct tegra_dc *dc, unsigned int index, const struct tegra_dc_window *window); +extern void tegra_dc_enable_vblank(struct tegra_dc *dc); +extern void tegra_dc_disable_vblank(struct tegra_dc *dc); struct tegra_output_ops { int (*enable)(struct tegra_output *output);
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> --- drivers/gpu/drm/tegra/dc.c | 55 +++++++++++++++++++++++++++++++-------------- drivers/gpu/drm/tegra/drm.c | 44 ++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/tegra/drm.h | 3 +++ 3 files changed, 85 insertions(+), 17 deletions(-)