Re: [Intel-gfx] [PULL] topic/vblank-rework, take 2
On Fri, Sep 12, 2014 at 09:48:59AM +0200, Daniel Vetter wrote:
[...]
Ville Syrjälä (16):
drm: Always reject drm_vblank_get() after drm_vblank_off()
This particular commit seems to be wreaking havoc. Inki already reported
this at some point and Andrzej and Joonyoung tried to fix this on Exynos
already, but it seems like a couple more drivers would suffer from this.
At least those using drm_vblank_off() but not drm_vblank_on() would see
the same issue.
A quick grep indicate that Armada, Exynos, GMA500 and STI would all
suffer from the same problem. Adding the respective maintainers. I've
used the attached patch to fix the issue on Tegra.
Ville, does that conversion look right? Perhaps it would be safer to
call drm_crtc_vblank_on() before activating the CRTC to avoid a
potential race? It seems like drm_vblank_on() and drm_vblank_off()
would replace drm_vblank_{pre,post}_modeset() completely, so I've
removed them as part of the patch as well.
I suppose the attached patch and equivalent ones for the other drivers
would need to be carried in this series to avoid regressions.
Thierry
From 1b7539953054f8f4b102570c21ddee36a65f4a06 Mon Sep 17 00:00:00 2001
From: Thierry Reding tred...@nvidia.com
Date: Wed, 8 Oct 2014 14:48:51 +0200
Subject: [PATCH] drm/tegra: dc: Add missing call to drm_vblank_on()
When the CRTC is enabled, make sure the VBLANK machinery is enabled.
Failure to do so will cause drm_vblank_get() to not enable the VBLANK on
the CRTC and VBLANK-synchronized page-flips won't work.
While at it, get rid of the legacy drm_vblank_pre_modeset() and
drm_vblank_post_modeset() calls that are replaced by drm_vblank_on()
and drm_vblank_off().
Reported-by: Alexandre Courbot acour...@nvidia.com
Signed-off-by: Thierry Reding tred...@nvidia.com
---
drivers/gpu/drm/tegra/dc.c | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index 2bc344b1fcd5..2aeaa4960d21 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -741,7 +741,6 @@ static const struct drm_crtc_funcs tegra_crtc_funcs = {
static void tegra_crtc_disable(struct drm_crtc *crtc)
{
- struct tegra_dc *dc = to_tegra_dc(crtc);
struct drm_device *drm = crtc-dev;
struct drm_plane *plane;
@@ -757,7 +756,7 @@ static void tegra_crtc_disable(struct drm_crtc *crtc)
}
}
- drm_vblank_off(drm, dc-pipe);
+ drm_crtc_vblank_off(crtc);
}
static bool tegra_crtc_mode_fixup(struct drm_crtc *crtc,
@@ -846,8 +845,6 @@ static int tegra_crtc_mode_set(struct drm_crtc *crtc,
u32 value;
int err;
- drm_vblank_pre_modeset(crtc-dev, dc-pipe);
-
err = tegra_crtc_setup_clk(crtc, mode);
if (err) {
dev_err(dc-dev, failed to setup clock for CRTC: %d\n, err);
@@ -948,7 +945,7 @@ static void tegra_crtc_commit(struct drm_crtc *crtc)
value = GENERAL_ACT_REQ | WIN_A_ACT_REQ;
tegra_dc_writel(dc, value, DC_CMD_STATE_CONTROL);
- drm_vblank_post_modeset(crtc-dev, dc-pipe);
+ drm_crtc_vblank_on(crtc);
}
static void tegra_crtc_load_lut(struct drm_crtc *crtc)
--
2.1.0
pgpLIqqfurVnb.pgp
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PULL] topic/vblank-rework, take 2
Hi Dave, So updated vblank-rework pull request, now with the polish that Mario requested applied (and reviewed by him). Also with backmerge like you've requested for easier merging. The neat thing this finally allows is to immediately disable the vblank interrupt on the last drm_vblank_put if the hardware has perfectly accurate vblank counter and timestamp readout support. On i915 that required piles of small adjustements from Ville since depending upon the platform and port the vblank happens at different scanout lines. Of course this is fully opt-in and per-device (we need that since gen2 doesn't have a hw vblank counter). Cheers, Daniel The following changes since commit fdcaa1dbb7c6ed419b10fb8cdb5001ab0a00538f: Merge tag 'ipu-3.18' of git://git.pengutronix.de/git/pza/linux into drm-next (2014-09-10 19:43:29 +1000) are available in the git repository at: git://anongit.freedesktop.org/drm-intel tags/topic/vblank-rework-2014-09-12 for you to fetch changes up to 336879b1da97fffc097f77c6d6f818660f2826f0: Merge remote-tracking branch 'airlied/drm-next' into topic/vblank-rework (2014-09-11 14:46:53 +0200) Daniel Vetter (5): drm: Really never disable vblank irqs for offdelay==0 drm: Only update final vblank count when precise ts is available drm: Simplify return value of drm_get_last_vbltimestamp drm: Clarify vblank ts/scanoutpos sampling #defines Merge remote-tracking branch 'airlied/drm-next' into topic/vblank-rework Mario Kleiner (2): drm: Remove drm_vblank_cleanup from drm_vblank_init error path. drm: Use vblank_disable_and_save in drm_vblank_cleanup() Ville Syrjälä (16): drm: Always reject drm_vblank_get() after drm_vblank_off() drm/i915: Warn if drm_vblank_get() still works after drm_vblank_off() drm: Don't clear vblank timestamps when vblank interrupt is disabled drm: Move drm_update_vblank_count() drm: Have the vblank counter account for the time between vblank irq disable and drm_vblank_off() drm: Avoid random vblank counter jumps if the hardware counter has been reset drm: Reduce the amount of dev-vblank[crtc] in the code drm: Fix deadlock between event_lock and vbl_lock/vblank_time_lock drm: Fix race between drm_vblank_off() and drm_queue_vblank_event() drm: Disable vblank interrupt immediately when drm_vblank_offdelay0 drm: Add dev-vblank_disable_immediate flag drm/i915: Opt out of vblank disable timer on gen2 drm: Kick start vblank interrupts at drm_vblank_on() drm/i915: Update scanline_offset only for active crtcs drm: Fix confusing debug message in drm_update_vblank_count() drm: Store the vblank timestamp when adjusting the counter during disable Documentation/DocBook/drm.tmpl| 7 + drivers/gpu/drm/drm_drv.c | 4 +- drivers/gpu/drm/drm_irq.c | 379 +++--- drivers/gpu/drm/i915/i915_irq.c | 10 +- drivers/gpu/drm/i915/intel_display.c | 17 +- drivers/gpu/drm/nouveau/nouveau_display.c | 2 +- drivers/gpu/drm/radeon/radeon_display.c | 2 +- drivers/gpu/drm/radeon/radeon_pm.c| 2 +- include/drm/drmP.h| 18 +- 9 files changed, 286 insertions(+), 155 deletions(-) -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PULL] topic/vblank-rework
Hi Mario,
Can you please take a look at the patches I've submitted and review
them (at least the first 2)? Dave will close the 3.18 drm-next merge
window at the end of this week and I'd like to really get this in.
Thanks, Daniel
On Wed, Sep 10, 2014 at 5:45 PM, Mario Kleiner
mario.kleiner...@gmail.com wrote:
On Wed, Sep 10, 2014 at 5:29 PM, Daniel Vetter daniel.vet...@ffwll.ch wrote:
On Wed, Sep 10, 2014 at 4:19 PM, Mario Kleiner
mario.kleiner...@gmail.com wrote:
Hmm, not quite an ack from my side for the pull in its current form. I
said if the two remaining issues i mentioned are addressed, then i'm
happy with it and can have my reviewed/acked-by. Looking at the code
they haven't been adressed.
Sorry about the confusion, I've somehow thought that you've retracted
those comments in Message-ID:
caesyxygk4foqhky1wcerak_hybex2ogpftjyhu_zfhlbx46...@mail.gmail.com
But I've missed that that was about just one of the issues.
Thought so. That one patch turns out to be crucial. My own software
immediately complained loudly about broken vblank irqs and switched to
lower performance fallbacks when that patch was missing.
I'll test the patches on a few more cards in the next days - but so
far things look good at least as far as my special test cases go.
However, this is easily fixable on top of the current patches:
1. A vblank_disable_timeout module parameter of zero should always
leave vblank irq's enabled and also override the drivers choice,
otherwise a user can't override the driver on a broken driver/gpu
combo, which is the only use case for having that module parameter.
Currenty the disable_immediately flag overrides the users override -
Ouch.
So in drm_vblank_put():
...
/* Last user schedules interrupt disable */
if (atomic_dec_and_test(vblank-refcount)) {
Insert zero - opt-out check
if (drm_vblank_offdelay == 0)
return;
Remaining code continues
if (dev-vblank_disable_immediate || drm_vblank_offdelay 0)
vblank_disable_fn((unsigned long)vblank);
else if (drm_vblank_offdelay 0)
mod_timer(vblank-disable_timer, jiffies +
((drm_vblank_offdelay * HZ)/1000));
Yeah, I guess that makes sense. I'm not really a fan of giving users
too powerful module options to hack around driver bugs since often
that means they'll never report the bug :( But we have the support now
to mark certain module options as debug-only and they'll taint the
kernel if set, so this is fixable.
I'll follow up with the patch you've suggested.
Thanks. I think the modules parameters i usually care about will get
proper testing and reporting, because while my software and users are
good at detecting such problems, they wouldn't know how to fix them
themselves, and at the same time they crucially depend on this stuff
working, so this gets reported to me quickly and i can give them the
module param workaround in private e-mail and take it from there with
proper bug reports or patches.
...
2. For the drm: Have the vblank counter account for the time ...
patch, we must opt-out of that last timestamp/counter update/bump if
the driver doesn't support high-precision vblank timestamping,
otherwise the vblank count and timestamp will be inconsistent with
each other - or outright wrong in case of the timestamp. Rather
deliver a slightly outdated, but correct count+timestamp pair to
userspace, which is still useable for practical purposes, than a pair
that's outright wrong and will definitely confuse clients.
A simple fix in static void vblank_disable_and_save() would be to
replace the new...
if (!vblank-enabled) {
... check by ...
if (!vblank-enabled
) {
Yeah, makes sense (well the follow-up one ofc). I'll do a patch which
adds this and adds a comment. Aside I think it would be useful to add
a #define for the 0 return value, since the magic checks all over are
imo fairly hard to understand.
I'll also float a patch for rfc about that.
Good!
thanks,
-mario
Thanks for your comments and again my apologies for missing that
there's still outstanding work left to do on this.
Cheers, Daniel
On Wed, Sep 10, 2014 at 2:05 PM, Daniel Vetter daniel.vet...@ffwll.ch
wrote:
Hi Dave,
So here's the final bits of Ville's vblank rework with a bit of cleanup
from Mario on top.
The neat thing this finally allows is to immediately disable the vblank
interrupt on the last drm_vblank_put if the hardware has perfectly
accurate vblank counter and timestamp readout support. On i915 that
required piles of small adjustements from Ville since depending upon the
platform and port the vblank happens at different scanout lines.
Of course this is fully opt-in and per-device (we need that since gen2
doesn't have a hw vblank counter).
Mario reviewed the entire pile too and after some initial hesitation
(about drivers without accurate timestampt support) acked it.
Cheers, Daniel
The following changes since commit
[Intel-gfx] [PULL] topic/vblank-rework
Hi Dave, So here's the final bits of Ville's vblank rework with a bit of cleanup from Mario on top. The neat thing this finally allows is to immediately disable the vblank interrupt on the last drm_vblank_put if the hardware has perfectly accurate vblank counter and timestamp readout support. On i915 that required piles of small adjustements from Ville since depending upon the platform and port the vblank happens at different scanout lines. Of course this is fully opt-in and per-device (we need that since gen2 doesn't have a hw vblank counter). Mario reviewed the entire pile too and after some initial hesitation (about drivers without accurate timestampt support) acked it. Cheers, Daniel The following changes since commit 21d70354bba9965a098382fc4d7fb17e138111f3: drm: move drm_stub.c to drm_drv.c (2014-08-06 19:10:44 +1000) are available in the git repository at: git://anongit.freedesktop.org/drm-intel tags/topic/vblank-rework-2014-09-10 for you to fetch changes up to 2368ffb18b1d2b04eb80478d225676caa7a3c4c8: drm: Use vblank_disable_and_save in drm_vblank_cleanup() (2014-09-10 09:41:29 +0200) Mario Kleiner (2): drm: Remove drm_vblank_cleanup from drm_vblank_init error path. drm: Use vblank_disable_and_save in drm_vblank_cleanup() Ville Syrjälä (16): drm: Always reject drm_vblank_get() after drm_vblank_off() drm/i915: Warn if drm_vblank_get() still works after drm_vblank_off() drm: Don't clear vblank timestamps when vblank interrupt is disabled drm: Move drm_update_vblank_count() drm: Have the vblank counter account for the time between vblank irq disable and drm_vblank_off() drm: Avoid random vblank counter jumps if the hardware counter has been reset drm: Reduce the amount of dev-vblank[crtc] in the code drm: Fix deadlock between event_lock and vbl_lock/vblank_time_lock drm: Fix race between drm_vblank_off() and drm_queue_vblank_event() drm: Disable vblank interrupt immediately when drm_vblank_offdelay0 drm: Add dev-vblank_disable_immediate flag drm/i915: Opt out of vblank disable timer on gen2 drm: Kick start vblank interrupts at drm_vblank_on() drm/i915: Update scanline_offset only for active crtcs drm: Fix confusing debug message in drm_update_vblank_count() drm: Store the vblank timestamp when adjusting the counter during disable Documentation/DocBook/drm.tmpl | 7 + drivers/gpu/drm/drm_drv.c| 4 +- drivers/gpu/drm/drm_irq.c| 345 ++- drivers/gpu/drm/i915/i915_irq.c | 8 + drivers/gpu/drm/i915/intel_display.c | 17 +- include/drm/drmP.h | 12 +- 6 files changed, 256 insertions(+), 137 deletions(-) -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PULL] topic/vblank-rework
Hmm, not quite an ack from my side for the pull in its current form. I
said if the two remaining issues i mentioned are addressed, then i'm
happy with it and can have my reviewed/acked-by. Looking at the code
they haven't been adressed.
However, this is easily fixable on top of the current patches:
1. A vblank_disable_timeout module parameter of zero should always
leave vblank irq's enabled and also override the drivers choice,
otherwise a user can't override the driver on a broken driver/gpu
combo, which is the only use case for having that module parameter.
Currenty the disable_immediately flag overrides the users override -
Ouch.
So in drm_vblank_put():
...
/* Last user schedules interrupt disable */
if (atomic_dec_and_test(vblank-refcount)) {
Insert zero - opt-out check
if (drm_vblank_offdelay == 0)
return;
Remaining code continues
if (dev-vblank_disable_immediate || drm_vblank_offdelay 0)
vblank_disable_fn((unsigned long)vblank);
else if (drm_vblank_offdelay 0)
mod_timer(vblank-disable_timer, jiffies +
((drm_vblank_offdelay * HZ)/1000));
...
2. For the drm: Have the vblank counter account for the time ...
patch, we must opt-out of that last timestamp/counter update/bump if
the driver doesn't support high-precision vblank timestamping,
otherwise the vblank count and timestamp will be inconsistent with
each other - or outright wrong in case of the timestamp. Rather
deliver a slightly outdated, but correct count+timestamp pair to
userspace, which is still useable for practical purposes, than a pair
that's outright wrong and will definitely confuse clients.
A simple fix in static void vblank_disable_and_save() would be to
replace the new...
if (!vblank-enabled) {
... check by ...
if (!vblank-enabled
) {
On Wed, Sep 10, 2014 at 2:05 PM, Daniel Vetter daniel.vet...@ffwll.ch wrote:
Hi Dave,
So here's the final bits of Ville's vblank rework with a bit of cleanup
from Mario on top.
The neat thing this finally allows is to immediately disable the vblank
interrupt on the last drm_vblank_put if the hardware has perfectly
accurate vblank counter and timestamp readout support. On i915 that
required piles of small adjustements from Ville since depending upon the
platform and port the vblank happens at different scanout lines.
Of course this is fully opt-in and per-device (we need that since gen2
doesn't have a hw vblank counter).
Mario reviewed the entire pile too and after some initial hesitation
(about drivers without accurate timestampt support) acked it.
Cheers, Daniel
The following changes since commit 21d70354bba9965a098382fc4d7fb17e138111f3:
drm: move drm_stub.c to drm_drv.c (2014-08-06 19:10:44 +1000)
are available in the git repository at:
git://anongit.freedesktop.org/drm-intel tags/topic/vblank-rework-2014-09-10
for you to fetch changes up to 2368ffb18b1d2b04eb80478d225676caa7a3c4c8:
drm: Use vblank_disable_and_save in drm_vblank_cleanup() (2014-09-10
09:41:29 +0200)
Mario Kleiner (2):
drm: Remove drm_vblank_cleanup from drm_vblank_init error path.
drm: Use vblank_disable_and_save in drm_vblank_cleanup()
Ville Syrjälä (16):
drm: Always reject drm_vblank_get() after drm_vblank_off()
drm/i915: Warn if drm_vblank_get() still works after drm_vblank_off()
drm: Don't clear vblank timestamps when vblank interrupt is disabled
drm: Move drm_update_vblank_count()
drm: Have the vblank counter account for the time between vblank irq
disable and drm_vblank_off()
drm: Avoid random vblank counter jumps if the hardware counter has been
reset
drm: Reduce the amount of dev-vblank[crtc] in the code
drm: Fix deadlock between event_lock and vbl_lock/vblank_time_lock
drm: Fix race between drm_vblank_off() and drm_queue_vblank_event()
drm: Disable vblank interrupt immediately when drm_vblank_offdelay0
drm: Add dev-vblank_disable_immediate flag
drm/i915: Opt out of vblank disable timer on gen2
drm: Kick start vblank interrupts at drm_vblank_on()
drm/i915: Update scanline_offset only for active crtcs
drm: Fix confusing debug message in drm_update_vblank_count()
drm: Store the vblank timestamp when adjusting the counter during
disable
Documentation/DocBook/drm.tmpl | 7 +
drivers/gpu/drm/drm_drv.c| 4 +-
drivers/gpu/drm/drm_irq.c| 345
++-
drivers/gpu/drm/i915/i915_irq.c | 8 +
drivers/gpu/drm/i915/intel_display.c | 17 +-
include/drm/drmP.h | 12 +-
6 files changed, 256 insertions(+), 137 deletions(-)
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
Re: [Intel-gfx] [PULL] topic/vblank-rework
e-mail snafu, sent it too early by accident, and from a gmail web
interface which i'm apparently incapable of using properly...
The second fix should look like this:
A simple fix in static void vblank_disable_and_save() would be to
replace the new...
if (!vblank-enabled) {
... check by ...
if (!vblank-enabled
drm_get_last_vbltimestamp(dev, crtc, tvblank, 0)) {
... We need to make sure timestamp queries work and are actually
locked to the vblank, otherwise we can't do that last update there in
vblank_disable_and_save().
With these two fixes or similar applied i'd be happy, otherwise it
will inflict pain and real bugs on real users.
thanks,
-mario
On Wed, Sep 10, 2014 at 4:19 PM, Mario Kleiner
mario.kleiner...@gmail.com wrote:
Hmm, not quite an ack from my side for the pull in its current form. I
said if the two remaining issues i mentioned are addressed, then i'm
happy with it and can have my reviewed/acked-by. Looking at the code
they haven't been adressed.
However, this is easily fixable on top of the current patches:
1. A vblank_disable_timeout module parameter of zero should always
leave vblank irq's enabled and also override the drivers choice,
otherwise a user can't override the driver on a broken driver/gpu
combo, which is the only use case for having that module parameter.
Currenty the disable_immediately flag overrides the users override -
Ouch.
So in drm_vblank_put():
...
/* Last user schedules interrupt disable */
if (atomic_dec_and_test(vblank-refcount)) {
Insert zero - opt-out check
if (drm_vblank_offdelay == 0)
return;
Remaining code continues
if (dev-vblank_disable_immediate || drm_vblank_offdelay 0)
vblank_disable_fn((unsigned long)vblank);
else if (drm_vblank_offdelay 0)
mod_timer(vblank-disable_timer, jiffies +
((drm_vblank_offdelay * HZ)/1000));
...
2. For the drm: Have the vblank counter account for the time ...
patch, we must opt-out of that last timestamp/counter update/bump if
the driver doesn't support high-precision vblank timestamping,
otherwise the vblank count and timestamp will be inconsistent with
each other - or outright wrong in case of the timestamp. Rather
deliver a slightly outdated, but correct count+timestamp pair to
userspace, which is still useable for practical purposes, than a pair
that's outright wrong and will definitely confuse clients.
A simple fix in static void vblank_disable_and_save() would be to
replace the new...
if (!vblank-enabled) {
... check by ...
if (!vblank-enabled
) {
On Wed, Sep 10, 2014 at 2:05 PM, Daniel Vetter daniel.vet...@ffwll.ch wrote:
Hi Dave,
So here's the final bits of Ville's vblank rework with a bit of cleanup
from Mario on top.
The neat thing this finally allows is to immediately disable the vblank
interrupt on the last drm_vblank_put if the hardware has perfectly
accurate vblank counter and timestamp readout support. On i915 that
required piles of small adjustements from Ville since depending upon the
platform and port the vblank happens at different scanout lines.
Of course this is fully opt-in and per-device (we need that since gen2
doesn't have a hw vblank counter).
Mario reviewed the entire pile too and after some initial hesitation
(about drivers without accurate timestampt support) acked it.
Cheers, Daniel
The following changes since commit 21d70354bba9965a098382fc4d7fb17e138111f3:
drm: move drm_stub.c to drm_drv.c (2014-08-06 19:10:44 +1000)
are available in the git repository at:
git://anongit.freedesktop.org/drm-intel tags/topic/vblank-rework-2014-09-10
for you to fetch changes up to 2368ffb18b1d2b04eb80478d225676caa7a3c4c8:
drm: Use vblank_disable_and_save in drm_vblank_cleanup() (2014-09-10
09:41:29 +0200)
Mario Kleiner (2):
drm: Remove drm_vblank_cleanup from drm_vblank_init error path.
drm: Use vblank_disable_and_save in drm_vblank_cleanup()
Ville Syrjälä (16):
drm: Always reject drm_vblank_get() after drm_vblank_off()
drm/i915: Warn if drm_vblank_get() still works after drm_vblank_off()
drm: Don't clear vblank timestamps when vblank interrupt is disabled
drm: Move drm_update_vblank_count()
drm: Have the vblank counter account for the time between vblank irq
disable and drm_vblank_off()
drm: Avoid random vblank counter jumps if the hardware counter has
been reset
drm: Reduce the amount of dev-vblank[crtc] in the code
drm: Fix deadlock between event_lock and vbl_lock/vblank_time_lock
drm: Fix race between drm_vblank_off() and drm_queue_vblank_event()
drm: Disable vblank interrupt immediately when drm_vblank_offdelay0
drm: Add dev-vblank_disable_immediate flag
drm/i915: Opt out of vblank disable timer on gen2
drm: Kick start vblank interrupts at drm_vblank_on()
drm/i915: Update scanline_offset
Re: [Intel-gfx] [PULL] topic/vblank-rework
On Wed, Sep 10, 2014 at 4:19 PM, Mario Kleiner
mario.kleiner...@gmail.com wrote:
Hmm, not quite an ack from my side for the pull in its current form. I
said if the two remaining issues i mentioned are addressed, then i'm
happy with it and can have my reviewed/acked-by. Looking at the code
they haven't been adressed.
Sorry about the confusion, I've somehow thought that you've retracted
those comments in Message-ID:
caesyxygk4foqhky1wcerak_hybex2ogpftjyhu_zfhlbx46...@mail.gmail.com
But I've missed that that was about just one of the issues.
However, this is easily fixable on top of the current patches:
1. A vblank_disable_timeout module parameter of zero should always
leave vblank irq's enabled and also override the drivers choice,
otherwise a user can't override the driver on a broken driver/gpu
combo, which is the only use case for having that module parameter.
Currenty the disable_immediately flag overrides the users override -
Ouch.
So in drm_vblank_put():
...
/* Last user schedules interrupt disable */
if (atomic_dec_and_test(vblank-refcount)) {
Insert zero - opt-out check
if (drm_vblank_offdelay == 0)
return;
Remaining code continues
if (dev-vblank_disable_immediate || drm_vblank_offdelay 0)
vblank_disable_fn((unsigned long)vblank);
else if (drm_vblank_offdelay 0)
mod_timer(vblank-disable_timer, jiffies +
((drm_vblank_offdelay * HZ)/1000));
Yeah, I guess that makes sense. I'm not really a fan of giving users
too powerful module options to hack around driver bugs since often
that means they'll never report the bug :( But we have the support now
to mark certain module options as debug-only and they'll taint the
kernel if set, so this is fixable.
I'll follow up with the patch you've suggested.
...
2. For the drm: Have the vblank counter account for the time ...
patch, we must opt-out of that last timestamp/counter update/bump if
the driver doesn't support high-precision vblank timestamping,
otherwise the vblank count and timestamp will be inconsistent with
each other - or outright wrong in case of the timestamp. Rather
deliver a slightly outdated, but correct count+timestamp pair to
userspace, which is still useable for practical purposes, than a pair
that's outright wrong and will definitely confuse clients.
A simple fix in static void vblank_disable_and_save() would be to
replace the new...
if (!vblank-enabled) {
... check by ...
if (!vblank-enabled
) {
Yeah, makes sense (well the follow-up one ofc). I'll do a patch which
adds this and adds a comment. Aside I think it would be useful to add
a #define for the 0 return value, since the magic checks all over are
imo fairly hard to understand.
I'll also float a patch for rfc about that.
Thanks for your comments and again my apologies for missing that
there's still outstanding work left to do on this.
Cheers, Daniel
On Wed, Sep 10, 2014 at 2:05 PM, Daniel Vetter daniel.vet...@ffwll.ch wrote:
Hi Dave,
So here's the final bits of Ville's vblank rework with a bit of cleanup
from Mario on top.
The neat thing this finally allows is to immediately disable the vblank
interrupt on the last drm_vblank_put if the hardware has perfectly
accurate vblank counter and timestamp readout support. On i915 that
required piles of small adjustements from Ville since depending upon the
platform and port the vblank happens at different scanout lines.
Of course this is fully opt-in and per-device (we need that since gen2
doesn't have a hw vblank counter).
Mario reviewed the entire pile too and after some initial hesitation
(about drivers without accurate timestampt support) acked it.
Cheers, Daniel
The following changes since commit 21d70354bba9965a098382fc4d7fb17e138111f3:
drm: move drm_stub.c to drm_drv.c (2014-08-06 19:10:44 +1000)
are available in the git repository at:
git://anongit.freedesktop.org/drm-intel tags/topic/vblank-rework-2014-09-10
for you to fetch changes up to 2368ffb18b1d2b04eb80478d225676caa7a3c4c8:
drm: Use vblank_disable_and_save in drm_vblank_cleanup() (2014-09-10
09:41:29 +0200)
Mario Kleiner (2):
drm: Remove drm_vblank_cleanup from drm_vblank_init error path.
drm: Use vblank_disable_and_save in drm_vblank_cleanup()
Ville Syrjälä (16):
drm: Always reject drm_vblank_get() after drm_vblank_off()
drm/i915: Warn if drm_vblank_get() still works after drm_vblank_off()
drm: Don't clear vblank timestamps when vblank interrupt is disabled
drm: Move drm_update_vblank_count()
drm: Have the vblank counter account for the time between vblank irq
disable and drm_vblank_off()
drm: Avoid random vblank counter jumps if the hardware counter has
been reset
drm: Reduce the amount of dev-vblank[crtc] in the code
drm: Fix deadlock between event_lock and vbl_lock/vblank_time_lock
drm: Fix race
Re: [Intel-gfx] [PULL] topic/vblank-rework
On Wed, Sep 10, 2014 at 5:29 PM, Daniel Vetter daniel.vet...@ffwll.ch wrote:
On Wed, Sep 10, 2014 at 4:19 PM, Mario Kleiner
mario.kleiner...@gmail.com wrote:
Hmm, not quite an ack from my side for the pull in its current form. I
said if the two remaining issues i mentioned are addressed, then i'm
happy with it and can have my reviewed/acked-by. Looking at the code
they haven't been adressed.
Sorry about the confusion, I've somehow thought that you've retracted
those comments in Message-ID:
caesyxygk4foqhky1wcerak_hybex2ogpftjyhu_zfhlbx46...@mail.gmail.com
But I've missed that that was about just one of the issues.
Thought so. That one patch turns out to be crucial. My own software
immediately complained loudly about broken vblank irqs and switched to
lower performance fallbacks when that patch was missing.
I'll test the patches on a few more cards in the next days - but so
far things look good at least as far as my special test cases go.
However, this is easily fixable on top of the current patches:
1. A vblank_disable_timeout module parameter of zero should always
leave vblank irq's enabled and also override the drivers choice,
otherwise a user can't override the driver on a broken driver/gpu
combo, which is the only use case for having that module parameter.
Currenty the disable_immediately flag overrides the users override -
Ouch.
So in drm_vblank_put():
...
/* Last user schedules interrupt disable */
if (atomic_dec_and_test(vblank-refcount)) {
Insert zero - opt-out check
if (drm_vblank_offdelay == 0)
return;
Remaining code continues
if (dev-vblank_disable_immediate || drm_vblank_offdelay 0)
vblank_disable_fn((unsigned long)vblank);
else if (drm_vblank_offdelay 0)
mod_timer(vblank-disable_timer, jiffies +
((drm_vblank_offdelay * HZ)/1000));
Yeah, I guess that makes sense. I'm not really a fan of giving users
too powerful module options to hack around driver bugs since often
that means they'll never report the bug :( But we have the support now
to mark certain module options as debug-only and they'll taint the
kernel if set, so this is fixable.
I'll follow up with the patch you've suggested.
Thanks. I think the modules parameters i usually care about will get
proper testing and reporting, because while my software and users are
good at detecting such problems, they wouldn't know how to fix them
themselves, and at the same time they crucially depend on this stuff
working, so this gets reported to me quickly and i can give them the
module param workaround in private e-mail and take it from there with
proper bug reports or patches.
...
2. For the drm: Have the vblank counter account for the time ...
patch, we must opt-out of that last timestamp/counter update/bump if
the driver doesn't support high-precision vblank timestamping,
otherwise the vblank count and timestamp will be inconsistent with
each other - or outright wrong in case of the timestamp. Rather
deliver a slightly outdated, but correct count+timestamp pair to
userspace, which is still useable for practical purposes, than a pair
that's outright wrong and will definitely confuse clients.
A simple fix in static void vblank_disable_and_save() would be to
replace the new...
if (!vblank-enabled) {
... check by ...
if (!vblank-enabled
) {
Yeah, makes sense (well the follow-up one ofc). I'll do a patch which
adds this and adds a comment. Aside I think it would be useful to add
a #define for the 0 return value, since the magic checks all over are
imo fairly hard to understand.
I'll also float a patch for rfc about that.
Good!
thanks,
-mario
Thanks for your comments and again my apologies for missing that
there's still outstanding work left to do on this.
Cheers, Daniel
On Wed, Sep 10, 2014 at 2:05 PM, Daniel Vetter daniel.vet...@ffwll.ch
wrote:
Hi Dave,
So here's the final bits of Ville's vblank rework with a bit of cleanup
from Mario on top.
The neat thing this finally allows is to immediately disable the vblank
interrupt on the last drm_vblank_put if the hardware has perfectly
accurate vblank counter and timestamp readout support. On i915 that
required piles of small adjustements from Ville since depending upon the
platform and port the vblank happens at different scanout lines.
Of course this is fully opt-in and per-device (we need that since gen2
doesn't have a hw vblank counter).
Mario reviewed the entire pile too and after some initial hesitation
(about drivers without accurate timestampt support) acked it.
Cheers, Daniel
The following changes since commit 21d70354bba9965a098382fc4d7fb17e138111f3:
drm: move drm_stub.c to drm_drv.c (2014-08-06 19:10:44 +1000)
are available in the git repository at:
git://anongit.freedesktop.org/drm-intel
tags/topic/vblank-rework-2014-09-10
for you to fetch changes up to 2368ffb18b1d2b04eb80478d225676caa7a3c4c8:
drm: Use vblank_disable_and_save in
