* [PATCH 1/3] drm/nouveau: Use drm_vblank_on/off consistently
@ 2015-05-27 9:04 Daniel Vetter
2015-05-27 9:04 ` [PATCH 2/3] drm/radeon: Switch to drm_vblank_on/off Daniel Vetter
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Daniel Vetter @ 2015-05-27 9:04 UTC (permalink / raw
To: DRI Development
Cc: Daniel Vetter, Daniel Vetter, Intel Graphics Development,
Ben Skeggs
In
commit 9cba5efab5a8145ae6c52ea273553f069c294482
Author: Mario Kleiner <mario.kleiner.de@gmail.com>
Date: Tue Jul 29 02:36:44 2014 +0200
drm/nouveau: Dis/Enable vblank irqs during suspend/resume
drm_vblank_on/off calls where added around suspend/resume to make sure
vblank stay doesn't go boom over that transition. But nouveau already
used drm_vblank_pre/post_modeset over modesets. Instead use
drm_vblank_on/off everyhwere. The slight change here is that after
_off drm_vblank_get will refuse to work right away, but nouveau
doesn't seem to depend upon that anywhere outside of the pageflip
paths.
The longer-term plan here is to switch all kms drivers to
drm_vblank_on/off so that common code like pending event cleanup can
be done there, while drm_vblank_pre/post_modeset will be purely
drm internal for the old UMS ioctl.
Note that the drm_vblank_off still seems required in the suspend path
since nouveau doesn't explicitly disable crtcs. But on the resume side
drm_helper_resume_force_mode should end up calling drm_vblank_on
through the nouveau crtc hooks already. Hence remove the call in the
resume code.
Cc: Mario Kleiner <mario.kleiner.de@gmail.com>
Cc: Ben Skeggs <bskeggs@redhat.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
drivers/gpu/drm/nouveau/dispnv04/crtc.c | 4 ++--
drivers/gpu/drm/nouveau/nouveau_display.c | 4 ----
2 files changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/dispnv04/crtc.c b/drivers/gpu/drm/nouveau/dispnv04/crtc.c
index 3d96b49fe662..dab24066fa21 100644
--- a/drivers/gpu/drm/nouveau/dispnv04/crtc.c
+++ b/drivers/gpu/drm/nouveau/dispnv04/crtc.c
@@ -708,7 +708,7 @@ static void nv_crtc_prepare(struct drm_crtc *crtc)
if (nv_two_heads(dev))
NVSetOwner(dev, nv_crtc->index);
- drm_vblank_pre_modeset(dev, nv_crtc->index);
+ drm_vblank_off(dev, nv_crtc->index);
funcs->dpms(crtc, DRM_MODE_DPMS_OFF);
NVBlankScreen(dev, nv_crtc->index, true);
@@ -740,7 +740,7 @@ static void nv_crtc_commit(struct drm_crtc *crtc)
#endif
funcs->dpms(crtc, DRM_MODE_DPMS_ON);
- drm_vblank_post_modeset(dev, nv_crtc->index);
+ drm_vblank_on(dev, nv_crtc->index);
}
static void nv_crtc_destroy(struct drm_crtc *crtc)
diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
index 8670d90cdc11..d824023f9fc6 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.c
+++ b/drivers/gpu/drm/nouveau/nouveau_display.c
@@ -620,10 +620,6 @@ nouveau_display_resume(struct drm_device *dev, bool runtime)
nv_crtc->lut.depth = 0;
}
- /* Make sure that drm and hw vblank irqs get resumed if needed. */
- for (head = 0; head < dev->mode_config.num_crtc; head++)
- drm_vblank_on(dev, head);
-
/* This should ensure we don't hit a locking problem when someone
* wakes us up via a connector. We should never go into suspend
* while the display is on anyways.
--
2.1.4
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/3] drm/radeon: Switch to drm_vblank_on/off
2015-05-27 9:04 [PATCH 1/3] drm/nouveau: Use drm_vblank_on/off consistently Daniel Vetter
@ 2015-05-27 9:04 ` Daniel Vetter
2015-05-27 9:21 ` Michel Dänzer
2015-05-27 9:04 ` [PATCH 3/3] drm/irq: Make drm_vblank_pre/post_modeset internal Daniel Vetter
2015-05-29 16:50 ` [PATCH 1/3] drm/nouveau: Use drm_vblank_on/off consistently Mario Kleiner
2 siblings, 1 reply; 14+ messages in thread
From: Daniel Vetter @ 2015-05-27 9:04 UTC (permalink / raw
To: DRI Development
Cc: Daniel Vetter, Intel Graphics Development, Michel Dänzer,
Daniel Vetter
These should be functionally equivalent to the older per/post modeset
functions, except that they block out drm_vblank_get right away.
There's only the clock adjusting code (outside of pageflips) in
readone which uses drm_vblank_get. But that code doesn't synchronize
against concurrent modesets and instead handles any such races by
waiting for the right vblank to arrive with a short timetout.
The longer-term plan here is to switch all kms drivers to
drm_vblank_on/off so that common code like pending event cleanup can
be done there, while drm_vblank_pre/post_modeset will be purely
drm internal for the old UMS ioctl.
Note that the kerneldoc for pre/post_modeset is wrong since as Michel
Dänzer correctly pointed out it works if only using pre/post_modeset.
The trouble that lead to this comment is the very old version of
drm_vblank_off to clear out pending events when disabling a pipe,
which did seem to wreak havoc with the trick used by pre/post_modeset.
Michel also expressed dissatisfaction with intel folks pushing new
interfaces with bogus justifications. I still maintain that having a
consistent set of vblank behaviour across kms drivers, separate from
any old UMS functions is a useful goal.
Cc: Michel Dänzer <michel.daenzer@amd.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
drivers/gpu/drm/radeon/atombios_crtc.c | 4 ++--
drivers/gpu/drm/radeon/radeon_legacy_crtc.c | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/radeon/atombios_crtc.c b/drivers/gpu/drm/radeon/atombios_crtc.c
index 42b2ea3fdcf3..77912fd48b69 100644
--- a/drivers/gpu/drm/radeon/atombios_crtc.c
+++ b/drivers/gpu/drm/radeon/atombios_crtc.c
@@ -274,13 +274,13 @@ void atombios_crtc_dpms(struct drm_crtc *crtc, int mode)
if (ASIC_IS_DCE3(rdev) && !ASIC_IS_DCE6(rdev))
atombios_enable_crtc_memreq(crtc, ATOM_ENABLE);
atombios_blank_crtc(crtc, ATOM_DISABLE);
- drm_vblank_post_modeset(dev, radeon_crtc->crtc_id);
+ drm_vblank_on(dev, radeon_crtc->crtc_id);
radeon_crtc_load_lut(crtc);
break;
case DRM_MODE_DPMS_STANDBY:
case DRM_MODE_DPMS_SUSPEND:
case DRM_MODE_DPMS_OFF:
- drm_vblank_pre_modeset(dev, radeon_crtc->crtc_id);
+ drm_vblank_off(dev, radeon_crtc->crtc_id);
if (radeon_crtc->enabled)
atombios_blank_crtc(crtc, ATOM_ENABLE);
if (ASIC_IS_DCE3(rdev) && !ASIC_IS_DCE6(rdev))
diff --git a/drivers/gpu/drm/radeon/radeon_legacy_crtc.c b/drivers/gpu/drm/radeon/radeon_legacy_crtc.c
index 678b4386540d..4259e27f3983 100644
--- a/drivers/gpu/drm/radeon/radeon_legacy_crtc.c
+++ b/drivers/gpu/drm/radeon/radeon_legacy_crtc.c
@@ -330,13 +330,13 @@ static void radeon_crtc_dpms(struct drm_crtc *crtc, int mode)
RADEON_CRTC_DISP_REQ_EN_B));
WREG32_P(RADEON_CRTC_EXT_CNTL, crtc_ext_cntl, ~(mask | crtc_ext_cntl));
}
- drm_vblank_post_modeset(dev, radeon_crtc->crtc_id);
+ drm_vblank_on(dev, radeon_crtc->crtc_id);
radeon_crtc_load_lut(crtc);
break;
case DRM_MODE_DPMS_STANDBY:
case DRM_MODE_DPMS_SUSPEND:
case DRM_MODE_DPMS_OFF:
- drm_vblank_pre_modeset(dev, radeon_crtc->crtc_id);
+ drm_vblank_off(dev, radeon_crtc->crtc_id);
if (radeon_crtc->crtc_id)
WREG32_P(RADEON_CRTC2_GEN_CNTL, mask, ~(RADEON_CRTC2_EN | mask));
else {
--
2.1.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 3/3] drm/irq: Make drm_vblank_pre/post_modeset internal
2015-05-27 9:04 [PATCH 1/3] drm/nouveau: Use drm_vblank_on/off consistently Daniel Vetter
2015-05-27 9:04 ` [PATCH 2/3] drm/radeon: Switch to drm_vblank_on/off Daniel Vetter
@ 2015-05-27 9:04 ` Daniel Vetter
2015-05-29 16:50 ` [PATCH 1/3] drm/nouveau: Use drm_vblank_on/off consistently Mario Kleiner
2 siblings, 0 replies; 14+ messages in thread
From: Daniel Vetter @ 2015-05-27 9:04 UTC (permalink / raw
To: DRI Development; +Cc: Daniel Vetter, Intel Graphics Development, Daniel Vetter
Now that all drivers are switched over to drm_vblank_on/off we can
relegate pre/post_modeset to the purely drm_irq.c internal role of
supporting on userspace.
As usual switch to the drm_legacy_ prefix to make it clear this is
for old drivers only.
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
Documentation/DocBook/drm.tmpl | 2 --
drivers/gpu/drm/drm_irq.c | 41 ++++-------------------------------------
include/drm/drmP.h | 4 ----
3 files changed, 4 insertions(+), 43 deletions(-)
diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index b6fc354a20e7..5eef6323dbb3 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -3803,8 +3803,6 @@ int num_ioctls;</synopsis>
drm_helper_connector_dpms() in reaction to fbdev blanking events. Do drivers
that don't implement (or just don't use) fbcon compatibility need to call
those functions themselves?
-- KMS drivers must call drm_vblank_pre_modeset() and drm_vblank_post_modeset()
- around mode setting. Should this be done in the DRM core?
- vblank_disable_allowed is set to 1 in the first drm_vblank_post_modeset()
call and never set back to 0. It seems to be safe to permanently set it to 1
in drm_vblank_init() for KMS driver, and it might be safe for UMS drivers as
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 1967e7fc9805..038c8b3256da 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -1359,30 +1359,7 @@ void drm_crtc_vblank_on(struct drm_crtc *crtc)
}
EXPORT_SYMBOL(drm_crtc_vblank_on);
-/**
- * drm_vblank_pre_modeset - account for vblanks across mode sets
- * @dev: DRM device
- * @crtc: CRTC in question
- *
- * Account for vblank events across mode setting events, which will likely
- * reset the hardware frame counter.
- *
- * This is done by grabbing a temporary vblank reference to ensure that the
- * vblank interrupt keeps running across the modeset sequence. With this the
- * software-side vblank frame counting will ensure that there are no jumps or
- * discontinuities.
- *
- * Unfortunately this approach is racy and also doesn't work when the vblank
- * interrupt stops running, e.g. across system suspend resume. It is therefore
- * highly recommended that drivers use the newer drm_vblank_off() and
- * drm_vblank_on() instead. drm_vblank_pre_modeset() only works correctly when
- * using "cooked" software vblank frame counters and not relying on any hardware
- * counters.
- *
- * Drivers must call drm_vblank_post_modeset() when re-enabling the same crtc
- * again.
- */
-void drm_vblank_pre_modeset(struct drm_device *dev, int crtc)
+static void drm_legacy_vblank_pre_modeset(struct drm_device *dev, int crtc)
{
struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
@@ -1406,17 +1383,8 @@ void drm_vblank_pre_modeset(struct drm_device *dev, int crtc)
vblank->inmodeset |= 0x2;
}
}
-EXPORT_SYMBOL(drm_vblank_pre_modeset);
-/**
- * drm_vblank_post_modeset - undo drm_vblank_pre_modeset changes
- * @dev: DRM device
- * @crtc: CRTC in question
- *
- * This function again drops the temporary vblank reference acquired in
- * drm_vblank_pre_modeset.
- */
-void drm_vblank_post_modeset(struct drm_device *dev, int crtc)
+static void drm_legacy_vblank_post_modeset(struct drm_device *dev, int crtc)
{
struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
unsigned long irqflags;
@@ -1436,7 +1404,6 @@ void drm_vblank_post_modeset(struct drm_device *dev, int crtc)
vblank->inmodeset = 0;
}
}
-EXPORT_SYMBOL(drm_vblank_post_modeset);
/*
* drm_modeset_ctl - handle vblank event counter changes across mode switch
@@ -1469,10 +1436,10 @@ int drm_modeset_ctl(struct drm_device *dev, void *data,
switch (modeset->cmd) {
case _DRM_PRE_MODESET:
- drm_vblank_pre_modeset(dev, crtc);
+ drm_legacy_vblank_pre_modeset(dev, crtc);
break;
case _DRM_POST_MODESET:
- drm_vblank_post_modeset(dev, crtc);
+ drm_legacy_vblank_post_modeset(dev, crtc);
break;
default:
return -EINVAL;
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index df6d9970d9a4..bf22298559d1 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -962,10 +962,6 @@ static inline wait_queue_head_t *drm_crtc_vblank_waitqueue(struct drm_crtc *crtc
return &crtc->dev->vblank[drm_crtc_index(crtc)].queue;
}
-/* Modesetting support */
-extern void drm_vblank_pre_modeset(struct drm_device *dev, int crtc);
-extern void drm_vblank_post_modeset(struct drm_device *dev, int crtc);
-
/* Stub support (drm_stub.h) */
extern struct drm_master *drm_master_get(struct drm_master *master);
extern void drm_master_put(struct drm_master **master);
--
2.1.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] drm/radeon: Switch to drm_vblank_on/off
2015-05-27 9:04 ` [PATCH 2/3] drm/radeon: Switch to drm_vblank_on/off Daniel Vetter
@ 2015-05-27 9:21 ` Michel Dänzer
2015-05-27 9:41 ` Daniel Vetter
0 siblings, 1 reply; 14+ messages in thread
From: Michel Dänzer @ 2015-05-27 9:21 UTC (permalink / raw
To: Daniel Vetter; +Cc: Daniel Vetter, Intel Graphics Development, DRI Development
On 27.05.2015 18:04, Daniel Vetter wrote:
> These should be functionally equivalent to the older per/post modeset
> functions, except that they block out drm_vblank_get right away.
> There's only the clock adjusting code (outside of pageflips) in
> readone which uses drm_vblank_get. But that code doesn't synchronize
> against concurrent modesets and instead handles any such races by
> waiting for the right vblank to arrive with a short timetout.
>
> The longer-term plan here is to switch all kms drivers to
> drm_vblank_on/off so that common code like pending event cleanup can
> be done there, while drm_vblank_pre/post_modeset will be purely
> drm internal for the old UMS ioctl.
>
> Note that the kerneldoc for pre/post_modeset is wrong since as Michel
> Dänzer correctly pointed out it works if only using pre/post_modeset.
> The trouble that lead to this comment is the very old version of
> drm_vblank_off to clear out pending events when disabling a pipe,
> which did seem to wreak havoc with the trick used by pre/post_modeset.
> Michel also expressed dissatisfaction with intel folks pushing new
> interfaces with bogus justifications. I still maintain that having a
> consistent set of vblank behaviour across kms drivers, separate from
> any old UMS functions is a useful goal.
>
> Cc: Michel Dänzer <michel.daenzer@amd.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Can you describe at least one tangible benefit this change provides for
the radeon driver?
Because I'm afraid that this might cause subtle breakage, and since we
don't have any rigorous tests for this like in intel-gpu-tools (yet?),
it might be painful to track it down.
So, I'd like to have a good reason for taking the risk.
> drivers/gpu/drm/radeon/atombios_crtc.c | 4 ++--
> drivers/gpu/drm/radeon/radeon_legacy_crtc.c | 4 ++--
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/atombios_crtc.c b/drivers/gpu/drm/radeon/atombios_crtc.c
> index 42b2ea3fdcf3..77912fd48b69 100644
> --- a/drivers/gpu/drm/radeon/atombios_crtc.c
> +++ b/drivers/gpu/drm/radeon/atombios_crtc.c
> @@ -274,13 +274,13 @@ void atombios_crtc_dpms(struct drm_crtc *crtc, int mode)
> if (ASIC_IS_DCE3(rdev) && !ASIC_IS_DCE6(rdev))
> atombios_enable_crtc_memreq(crtc, ATOM_ENABLE);
> atombios_blank_crtc(crtc, ATOM_DISABLE);
> - drm_vblank_post_modeset(dev, radeon_crtc->crtc_id);
> + drm_vblank_on(dev, radeon_crtc->crtc_id);
> radeon_crtc_load_lut(crtc);
> break;
> case DRM_MODE_DPMS_STANDBY:
> case DRM_MODE_DPMS_SUSPEND:
> case DRM_MODE_DPMS_OFF:
> - drm_vblank_pre_modeset(dev, radeon_crtc->crtc_id);
> + drm_vblank_off(dev, radeon_crtc->crtc_id);
> if (radeon_crtc->enabled)
> atombios_blank_crtc(crtc, ATOM_ENABLE);
> if (ASIC_IS_DCE3(rdev) && !ASIC_IS_DCE6(rdev))
> diff --git a/drivers/gpu/drm/radeon/radeon_legacy_crtc.c b/drivers/gpu/drm/radeon/radeon_legacy_crtc.c
> index 678b4386540d..4259e27f3983 100644
> --- a/drivers/gpu/drm/radeon/radeon_legacy_crtc.c
> +++ b/drivers/gpu/drm/radeon/radeon_legacy_crtc.c
> @@ -330,13 +330,13 @@ static void radeon_crtc_dpms(struct drm_crtc *crtc, int mode)
> RADEON_CRTC_DISP_REQ_EN_B));
> WREG32_P(RADEON_CRTC_EXT_CNTL, crtc_ext_cntl, ~(mask | crtc_ext_cntl));
> }
> - drm_vblank_post_modeset(dev, radeon_crtc->crtc_id);
> + drm_vblank_on(dev, radeon_crtc->crtc_id);
> radeon_crtc_load_lut(crtc);
> break;
> case DRM_MODE_DPMS_STANDBY:
> case DRM_MODE_DPMS_SUSPEND:
> case DRM_MODE_DPMS_OFF:
> - drm_vblank_pre_modeset(dev, radeon_crtc->crtc_id);
> + drm_vblank_off(dev, radeon_crtc->crtc_id);
> if (radeon_crtc->crtc_id)
> WREG32_P(RADEON_CRTC2_GEN_CNTL, mask, ~(RADEON_CRTC2_EN | mask));
> else {
>
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] drm/radeon: Switch to drm_vblank_on/off
2015-05-27 9:21 ` Michel Dänzer
@ 2015-05-27 9:41 ` Daniel Vetter
2015-05-28 7:11 ` Michel Dänzer
0 siblings, 1 reply; 14+ messages in thread
From: Daniel Vetter @ 2015-05-27 9:41 UTC (permalink / raw
To: Michel Dänzer
Cc: Daniel Vetter, Intel Graphics Development, DRI Development,
Daniel Vetter
On Wed, May 27, 2015 at 06:21:24PM +0900, Michel Dänzer wrote:
> On 27.05.2015 18:04, Daniel Vetter wrote:
> > These should be functionally equivalent to the older per/post modeset
> > functions, except that they block out drm_vblank_get right away.
> > There's only the clock adjusting code (outside of pageflips) in
> > readone which uses drm_vblank_get. But that code doesn't synchronize
> > against concurrent modesets and instead handles any such races by
> > waiting for the right vblank to arrive with a short timetout.
> >
> > The longer-term plan here is to switch all kms drivers to
> > drm_vblank_on/off so that common code like pending event cleanup can
> > be done there, while drm_vblank_pre/post_modeset will be purely
> > drm internal for the old UMS ioctl.
> >
> > Note that the kerneldoc for pre/post_modeset is wrong since as Michel
> > Dänzer correctly pointed out it works if only using pre/post_modeset.
> > The trouble that lead to this comment is the very old version of
> > drm_vblank_off to clear out pending events when disabling a pipe,
> > which did seem to wreak havoc with the trick used by pre/post_modeset.
> > Michel also expressed dissatisfaction with intel folks pushing new
> > interfaces with bogus justifications. I still maintain that having a
> > consistent set of vblank behaviour across kms drivers, separate from
> > any old UMS functions is a useful goal.
> >
> > Cc: Michel Dänzer <michel.daenzer@amd.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>
> Can you describe at least one tangible benefit this change provides for
> the radeon driver?
>
> Because I'm afraid that this might cause subtle breakage, and since we
> don't have any rigorous tests for this like in intel-gpu-tools (yet?),
> it might be painful to track it down.
>
> So, I'd like to have a good reason for taking the risk.
right now at most a bit of code to clean out pending events on modeset
disable, for somewhat consistent behaviour with other drivers. But in
general it's fairly ill-defined what happens with vblank events.
Longer-term Rob&I have a pile of patches and refactorings for drm_irq.c
planned so that drivers don't need to open-code all the event handling and
cleanup code (for both vblank and pageflips) and maybe also some
infrastructure for vblank callbacks (which also a lot of drivers
hand-roll). But drm_irq.c has the trouble of being used by both kms and
ums drivers, and I don't want to change any of the ums code too much.
Hence first step (which has been ongoing for a while now) is trying to
create a clean split between functions/features used by kms drivers and
those used by the ums ioctls.
Anyway that's just my overall plan, there's not going to be a problem if
you decide not to apply these patches. I just want to make sure that any
such drm core refactoring projects aren't seen as intel-only, hence I send
around these patches. And ofc I'd be nice to know if this does
unintentionally break something, to make sure the new stuff is as widely
suitable as possible. But in the end it's your call.
-Daniel
> > drivers/gpu/drm/radeon/atombios_crtc.c | 4 ++--
> > drivers/gpu/drm/radeon/radeon_legacy_crtc.c | 4 ++--
> > 2 files changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/radeon/atombios_crtc.c b/drivers/gpu/drm/radeon/atombios_crtc.c
> > index 42b2ea3fdcf3..77912fd48b69 100644
> > --- a/drivers/gpu/drm/radeon/atombios_crtc.c
> > +++ b/drivers/gpu/drm/radeon/atombios_crtc.c
> > @@ -274,13 +274,13 @@ void atombios_crtc_dpms(struct drm_crtc *crtc, int mode)
> > if (ASIC_IS_DCE3(rdev) && !ASIC_IS_DCE6(rdev))
> > atombios_enable_crtc_memreq(crtc, ATOM_ENABLE);
> > atombios_blank_crtc(crtc, ATOM_DISABLE);
> > - drm_vblank_post_modeset(dev, radeon_crtc->crtc_id);
> > + drm_vblank_on(dev, radeon_crtc->crtc_id);
> > radeon_crtc_load_lut(crtc);
> > break;
> > case DRM_MODE_DPMS_STANDBY:
> > case DRM_MODE_DPMS_SUSPEND:
> > case DRM_MODE_DPMS_OFF:
> > - drm_vblank_pre_modeset(dev, radeon_crtc->crtc_id);
> > + drm_vblank_off(dev, radeon_crtc->crtc_id);
> > if (radeon_crtc->enabled)
> > atombios_blank_crtc(crtc, ATOM_ENABLE);
> > if (ASIC_IS_DCE3(rdev) && !ASIC_IS_DCE6(rdev))
> > diff --git a/drivers/gpu/drm/radeon/radeon_legacy_crtc.c b/drivers/gpu/drm/radeon/radeon_legacy_crtc.c
> > index 678b4386540d..4259e27f3983 100644
> > --- a/drivers/gpu/drm/radeon/radeon_legacy_crtc.c
> > +++ b/drivers/gpu/drm/radeon/radeon_legacy_crtc.c
> > @@ -330,13 +330,13 @@ static void radeon_crtc_dpms(struct drm_crtc *crtc, int mode)
> > RADEON_CRTC_DISP_REQ_EN_B));
> > WREG32_P(RADEON_CRTC_EXT_CNTL, crtc_ext_cntl, ~(mask | crtc_ext_cntl));
> > }
> > - drm_vblank_post_modeset(dev, radeon_crtc->crtc_id);
> > + drm_vblank_on(dev, radeon_crtc->crtc_id);
> > radeon_crtc_load_lut(crtc);
> > break;
> > case DRM_MODE_DPMS_STANDBY:
> > case DRM_MODE_DPMS_SUSPEND:
> > case DRM_MODE_DPMS_OFF:
> > - drm_vblank_pre_modeset(dev, radeon_crtc->crtc_id);
> > + drm_vblank_off(dev, radeon_crtc->crtc_id);
> > if (radeon_crtc->crtc_id)
> > WREG32_P(RADEON_CRTC2_GEN_CNTL, mask, ~(RADEON_CRTC2_EN | mask));
> > else {
> >
>
>
> --
> Earthling Michel Dänzer | http://www.amd.com
> Libre software enthusiast | Mesa and X developer
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] drm/radeon: Switch to drm_vblank_on/off
2015-05-27 9:41 ` Daniel Vetter
@ 2015-05-28 7:11 ` Michel Dänzer
2015-05-28 8:38 ` Daniel Vetter
0 siblings, 1 reply; 14+ messages in thread
From: Michel Dänzer @ 2015-05-28 7:11 UTC (permalink / raw
To: Daniel Vetter; +Cc: Daniel Vetter, Intel Graphics Development, DRI Development
On 27.05.2015 18:41, Daniel Vetter wrote:
> On Wed, May 27, 2015 at 06:21:24PM +0900, Michel Dänzer wrote:
>> On 27.05.2015 18:04, Daniel Vetter wrote:
>>> These should be functionally equivalent to the older per/post modeset
>>> functions, except that they block out drm_vblank_get right away.
>>> There's only the clock adjusting code (outside of pageflips) in
>>> readone which uses drm_vblank_get. But that code doesn't synchronize
>>> against concurrent modesets and instead handles any such races by
>>> waiting for the right vblank to arrive with a short timetout.
>>>
>>> The longer-term plan here is to switch all kms drivers to
>>> drm_vblank_on/off so that common code like pending event cleanup can
>>> be done there, while drm_vblank_pre/post_modeset will be purely
>>> drm internal for the old UMS ioctl.
>>>
>>> Note that the kerneldoc for pre/post_modeset is wrong since as Michel
>>> Dänzer correctly pointed out it works if only using pre/post_modeset.
>>> The trouble that lead to this comment is the very old version of
>>> drm_vblank_off to clear out pending events when disabling a pipe,
>>> which did seem to wreak havoc with the trick used by pre/post_modeset.
>>> Michel also expressed dissatisfaction with intel folks pushing new
>>> interfaces with bogus justifications. I still maintain that having a
>>> consistent set of vblank behaviour across kms drivers, separate from
>>> any old UMS functions is a useful goal.
>>>
>>> Cc: Michel Dänzer <michel.daenzer@amd.com>
>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>>
>> Can you describe at least one tangible benefit this change provides for
>> the radeon driver?
>>
>> Because I'm afraid that this might cause subtle breakage, and since we
>> don't have any rigorous tests for this like in intel-gpu-tools (yet?),
>> it might be painful to track it down.
>>
>> So, I'd like to have a good reason for taking the risk.
>
> right now at most a bit of code to clean out pending events on modeset
> disable, for somewhat consistent behaviour with other drivers. But in
> general it's fairly ill-defined what happens with vblank events.
Yeah, while that's nice to have, I don't think it makes too much
difference in practice.
Anyway, I'm giving this patch a spin, and it does indeed cause userspace
fallout, at least with DRI3/Present enabled, because the vblank and
pageflip ioctls now return -EINVAL while the CRTC is off. However, it
looks like fixing that up might not be too bad, so I'm cautiously
optimistic for this change. But I'd like some more time for testing and
fixing userspace.
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] drm/radeon: Switch to drm_vblank_on/off
2015-05-28 7:11 ` Michel Dänzer
@ 2015-05-28 8:38 ` Daniel Vetter
2015-05-28 9:03 ` Michel Dänzer
0 siblings, 1 reply; 14+ messages in thread
From: Daniel Vetter @ 2015-05-28 8:38 UTC (permalink / raw
To: Michel Dänzer
Cc: Daniel Vetter, Intel Graphics Development, DRI Development
On Thu, May 28, 2015 at 04:11:53PM +0900, Michel Dänzer wrote:
> On 27.05.2015 18:41, Daniel Vetter wrote:
> > On Wed, May 27, 2015 at 06:21:24PM +0900, Michel Dänzer wrote:
> >> On 27.05.2015 18:04, Daniel Vetter wrote:
> >>> These should be functionally equivalent to the older per/post modeset
> >>> functions, except that they block out drm_vblank_get right away.
> >>> There's only the clock adjusting code (outside of pageflips) in
> >>> readone which uses drm_vblank_get. But that code doesn't synchronize
> >>> against concurrent modesets and instead handles any such races by
> >>> waiting for the right vblank to arrive with a short timetout.
> >>>
> >>> The longer-term plan here is to switch all kms drivers to
> >>> drm_vblank_on/off so that common code like pending event cleanup can
> >>> be done there, while drm_vblank_pre/post_modeset will be purely
> >>> drm internal for the old UMS ioctl.
> >>>
> >>> Note that the kerneldoc for pre/post_modeset is wrong since as Michel
> >>> Dänzer correctly pointed out it works if only using pre/post_modeset.
> >>> The trouble that lead to this comment is the very old version of
> >>> drm_vblank_off to clear out pending events when disabling a pipe,
> >>> which did seem to wreak havoc with the trick used by pre/post_modeset.
> >>> Michel also expressed dissatisfaction with intel folks pushing new
> >>> interfaces with bogus justifications. I still maintain that having a
> >>> consistent set of vblank behaviour across kms drivers, separate from
> >>> any old UMS functions is a useful goal.
> >>>
> >>> Cc: Michel Dänzer <michel.daenzer@amd.com>
> >>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> >>
> >> Can you describe at least one tangible benefit this change provides for
> >> the radeon driver?
> >>
> >> Because I'm afraid that this might cause subtle breakage, and since we
> >> don't have any rigorous tests for this like in intel-gpu-tools (yet?),
> >> it might be painful to track it down.
> >>
> >> So, I'd like to have a good reason for taking the risk.
> >
> > right now at most a bit of code to clean out pending events on modeset
> > disable, for somewhat consistent behaviour with other drivers. But in
> > general it's fairly ill-defined what happens with vblank events.
>
> Yeah, while that's nice to have, I don't think it makes too much
> difference in practice.
>
> Anyway, I'm giving this patch a spin, and it does indeed cause userspace
> fallout, at least with DRI3/Present enabled, because the vblank and
> pageflip ioctls now return -EINVAL while the CRTC is off. However, it
> looks like fixing that up might not be too bad, so I'm cautiously
> optimistic for this change. But I'd like some more time for testing and
> fixing userspace.
Hm that's a bummer, since it means the abi userspace sees has already
diverged between drivers :(
And because drm_irq.c is still a giant mess with no clear drm core ->
driver interface there's also no way for drivers to overwrite the normal
behaviour to keep old, driver-specific userspace happy. Adding such a hook
for kms drivers is definitely something I want to do. With all the modeset
entry points and atomic helpers you can do that and so keep old userspace
working even if the more strict semantics of atomic would break it (which
was an important goal with all the super-flexible helper library).
Otoh asking for a vblank event on a dead pipe smells like a userspace bug
and could result in stuck compositors. Not sure what's best here really.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] drm/radeon: Switch to drm_vblank_on/off
2015-05-28 8:38 ` Daniel Vetter
@ 2015-05-28 9:03 ` Michel Dänzer
2015-07-14 8:13 ` Michel Dänzer
0 siblings, 1 reply; 14+ messages in thread
From: Michel Dänzer @ 2015-05-28 9:03 UTC (permalink / raw
To: Daniel Vetter; +Cc: Daniel Vetter, Intel Graphics Development, DRI Development
On 28.05.2015 17:38, Daniel Vetter wrote:
> On Thu, May 28, 2015 at 04:11:53PM +0900, Michel Dänzer wrote:
>> On 27.05.2015 18:41, Daniel Vetter wrote:
>>> On Wed, May 27, 2015 at 06:21:24PM +0900, Michel Dänzer wrote:
>>>> On 27.05.2015 18:04, Daniel Vetter wrote:
>>>>> These should be functionally equivalent to the older per/post modeset
>>>>> functions, except that they block out drm_vblank_get right away.
>>>>> There's only the clock adjusting code (outside of pageflips) in
>>>>> readone which uses drm_vblank_get. But that code doesn't synchronize
>>>>> against concurrent modesets and instead handles any such races by
>>>>> waiting for the right vblank to arrive with a short timetout.
>>>>>
>>>>> The longer-term plan here is to switch all kms drivers to
>>>>> drm_vblank_on/off so that common code like pending event cleanup can
>>>>> be done there, while drm_vblank_pre/post_modeset will be purely
>>>>> drm internal for the old UMS ioctl.
>>>>>
>>>>> Note that the kerneldoc for pre/post_modeset is wrong since as Michel
>>>>> Dänzer correctly pointed out it works if only using pre/post_modeset.
>>>>> The trouble that lead to this comment is the very old version of
>>>>> drm_vblank_off to clear out pending events when disabling a pipe,
>>>>> which did seem to wreak havoc with the trick used by pre/post_modeset.
>>>>> Michel also expressed dissatisfaction with intel folks pushing new
>>>>> interfaces with bogus justifications. I still maintain that having a
>>>>> consistent set of vblank behaviour across kms drivers, separate from
>>>>> any old UMS functions is a useful goal.
>>>>>
>>>>> Cc: Michel Dänzer <michel.daenzer@amd.com>
>>>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>>>>
>>>> Can you describe at least one tangible benefit this change provides for
>>>> the radeon driver?
>>>>
>>>> Because I'm afraid that this might cause subtle breakage, and since we
>>>> don't have any rigorous tests for this like in intel-gpu-tools (yet?),
>>>> it might be painful to track it down.
>>>>
>>>> So, I'd like to have a good reason for taking the risk.
>>>
>>> right now at most a bit of code to clean out pending events on modeset
>>> disable, for somewhat consistent behaviour with other drivers. But in
>>> general it's fairly ill-defined what happens with vblank events.
>>
>> Yeah, while that's nice to have, I don't think it makes too much
>> difference in practice.
>>
>> Anyway, I'm giving this patch a spin, and it does indeed cause userspace
>> fallout, at least with DRI3/Present enabled, because the vblank and
>> pageflip ioctls now return -EINVAL while the CRTC is off. However, it
>> looks like fixing that up might not be too bad, so I'm cautiously
>> optimistic for this change. But I'd like some more time for testing and
>> fixing userspace.
[...]
> Otoh asking for a vblank event on a dead pipe smells like a userspace bug
> and could result in stuck compositors. Not sure what's best here really.
Agreed, and we're already careful not to do that with DRI2, just not yet
with DRI3/Present (which isn't in any xf86-video-ati release yet).
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] drm/nouveau: Use drm_vblank_on/off consistently
2015-05-27 9:04 [PATCH 1/3] drm/nouveau: Use drm_vblank_on/off consistently Daniel Vetter
2015-05-27 9:04 ` [PATCH 2/3] drm/radeon: Switch to drm_vblank_on/off Daniel Vetter
2015-05-27 9:04 ` [PATCH 3/3] drm/irq: Make drm_vblank_pre/post_modeset internal Daniel Vetter
@ 2015-05-29 16:50 ` Mario Kleiner
2015-05-29 17:19 ` Daniel Vetter
2 siblings, 1 reply; 14+ messages in thread
From: Mario Kleiner @ 2015-05-29 16:50 UTC (permalink / raw
To: Daniel Vetter, DRI Development
Cc: Daniel Vetter, Intel Graphics Development, Ben Skeggs
On 05/27/2015 11:04 AM, Daniel Vetter wrote:
> In
>
> commit 9cba5efab5a8145ae6c52ea273553f069c294482
> Author: Mario Kleiner <mario.kleiner.de@gmail.com>
> Date: Tue Jul 29 02:36:44 2014 +0200
>
> drm/nouveau: Dis/Enable vblank irqs during suspend/resume
>
> drm_vblank_on/off calls where added around suspend/resume to make sure
> vblank stay doesn't go boom over that transition. But nouveau already
> used drm_vblank_pre/post_modeset over modesets. Instead use
> drm_vblank_on/off everyhwere. The slight change here is that after
> _off drm_vblank_get will refuse to work right away, but nouveau
> doesn't seem to depend upon that anywhere outside of the pageflip
> paths.
>
> The longer-term plan here is to switch all kms drivers to
> drm_vblank_on/off so that common code like pending event cleanup can
> be done there, while drm_vblank_pre/post_modeset will be purely
> drm internal for the old UMS ioctl.
>
> Note that the drm_vblank_off still seems required in the suspend path
> since nouveau doesn't explicitly disable crtcs. But on the resume side
> drm_helper_resume_force_mode should end up calling drm_vblank_on
> through the nouveau crtc hooks already. Hence remove the call in the
> resume code.
>
> Cc: Mario Kleiner <mario.kleiner.de@gmail.com>
> Cc: Ben Skeggs <bskeggs@redhat.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
> drivers/gpu/drm/nouveau/dispnv04/crtc.c | 4 ++--
> drivers/gpu/drm/nouveau/nouveau_display.c | 4 ----
> 2 files changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/dispnv04/crtc.c b/drivers/gpu/drm/nouveau/dispnv04/crtc.c
> index 3d96b49fe662..dab24066fa21 100644
> --- a/drivers/gpu/drm/nouveau/dispnv04/crtc.c
> +++ b/drivers/gpu/drm/nouveau/dispnv04/crtc.c
> @@ -708,7 +708,7 @@ static void nv_crtc_prepare(struct drm_crtc *crtc)
> if (nv_two_heads(dev))
> NVSetOwner(dev, nv_crtc->index);
>
> - drm_vblank_pre_modeset(dev, nv_crtc->index);
> + drm_vblank_off(dev, nv_crtc->index);
> funcs->dpms(crtc, DRM_MODE_DPMS_OFF);
>
> NVBlankScreen(dev, nv_crtc->index, true);
> @@ -740,7 +740,7 @@ static void nv_crtc_commit(struct drm_crtc *crtc)
> #endif
>
> funcs->dpms(crtc, DRM_MODE_DPMS_ON);
> - drm_vblank_post_modeset(dev, nv_crtc->index);
> + drm_vblank_on(dev, nv_crtc->index);
> }
The above hunk is probably correct, but i couldn't test it without
sufficiently old pre-nv 50 hardware.
>
> static void nv_crtc_destroy(struct drm_crtc *crtc)
> diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
> index 8670d90cdc11..d824023f9fc6 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_display.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_display.c
> @@ -620,10 +620,6 @@ nouveau_display_resume(struct drm_device *dev, bool runtime)
> nv_crtc->lut.depth = 0;
> }
>
> - /* Make sure that drm and hw vblank irqs get resumed if needed. */
> - for (head = 0; head < dev->mode_config.num_crtc; head++)
> - drm_vblank_on(dev, head);
> -
> /* This should ensure we don't hit a locking problem when someone
> * wakes us up via a connector. We should never go into suspend
> * while the display is on anyways.
>
Tested this one and this hunk breaks suspend/resume. After a
suspend/resume cycle, all OpenGL apps and composited desktop are dead,
as the core can't get any vblank irq's enabled anymore.
So the drm_vblank_on() is still needed here.
thanks,
-mario
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] drm/nouveau: Use drm_vblank_on/off consistently
2015-05-29 16:50 ` [PATCH 1/3] drm/nouveau: Use drm_vblank_on/off consistently Mario Kleiner
@ 2015-05-29 17:19 ` Daniel Vetter
2015-05-29 17:23 ` Mario Kleiner
0 siblings, 1 reply; 14+ messages in thread
From: Daniel Vetter @ 2015-05-29 17:19 UTC (permalink / raw
To: Mario Kleiner
Cc: Daniel Vetter, Intel Graphics Development, Ben Skeggs,
DRI Development, Daniel Vetter
On Fri, May 29, 2015 at 06:50:06PM +0200, Mario Kleiner wrote:
> On 05/27/2015 11:04 AM, Daniel Vetter wrote:
> >In
> >
> >commit 9cba5efab5a8145ae6c52ea273553f069c294482
> >Author: Mario Kleiner <mario.kleiner.de@gmail.com>
> >Date: Tue Jul 29 02:36:44 2014 +0200
> >
> > drm/nouveau: Dis/Enable vblank irqs during suspend/resume
> >
> >drm_vblank_on/off calls where added around suspend/resume to make sure
> >vblank stay doesn't go boom over that transition. But nouveau already
> >used drm_vblank_pre/post_modeset over modesets. Instead use
> >drm_vblank_on/off everyhwere. The slight change here is that after
> >_off drm_vblank_get will refuse to work right away, but nouveau
> >doesn't seem to depend upon that anywhere outside of the pageflip
> >paths.
> >
> >The longer-term plan here is to switch all kms drivers to
> >drm_vblank_on/off so that common code like pending event cleanup can
> >be done there, while drm_vblank_pre/post_modeset will be purely
> >drm internal for the old UMS ioctl.
> >
> >Note that the drm_vblank_off still seems required in the suspend path
> >since nouveau doesn't explicitly disable crtcs. But on the resume side
> >drm_helper_resume_force_mode should end up calling drm_vblank_on
> >through the nouveau crtc hooks already. Hence remove the call in the
> >resume code.
> >
> >Cc: Mario Kleiner <mario.kleiner.de@gmail.com>
> >Cc: Ben Skeggs <bskeggs@redhat.com>
> >Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> >---
> > drivers/gpu/drm/nouveau/dispnv04/crtc.c | 4 ++--
> > drivers/gpu/drm/nouveau/nouveau_display.c | 4 ----
> > 2 files changed, 2 insertions(+), 6 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/nouveau/dispnv04/crtc.c b/drivers/gpu/drm/nouveau/dispnv04/crtc.c
> >index 3d96b49fe662..dab24066fa21 100644
> >--- a/drivers/gpu/drm/nouveau/dispnv04/crtc.c
> >+++ b/drivers/gpu/drm/nouveau/dispnv04/crtc.c
> >@@ -708,7 +708,7 @@ static void nv_crtc_prepare(struct drm_crtc *crtc)
> > if (nv_two_heads(dev))
> > NVSetOwner(dev, nv_crtc->index);
> >
> >- drm_vblank_pre_modeset(dev, nv_crtc->index);
> >+ drm_vblank_off(dev, nv_crtc->index);
> > funcs->dpms(crtc, DRM_MODE_DPMS_OFF);
> >
> > NVBlankScreen(dev, nv_crtc->index, true);
> >@@ -740,7 +740,7 @@ static void nv_crtc_commit(struct drm_crtc *crtc)
> > #endif
> >
> > funcs->dpms(crtc, DRM_MODE_DPMS_ON);
> >- drm_vblank_post_modeset(dev, nv_crtc->index);
> >+ drm_vblank_on(dev, nv_crtc->index);
> > }
>
> The above hunk is probably correct, but i couldn't test it without
> sufficiently old pre-nv 50 hardware.
>
> >
> > static void nv_crtc_destroy(struct drm_crtc *crtc)
> >diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
> >index 8670d90cdc11..d824023f9fc6 100644
> >--- a/drivers/gpu/drm/nouveau/nouveau_display.c
> >+++ b/drivers/gpu/drm/nouveau/nouveau_display.c
> >@@ -620,10 +620,6 @@ nouveau_display_resume(struct drm_device *dev, bool runtime)
> > nv_crtc->lut.depth = 0;
> > }
> >
> >- /* Make sure that drm and hw vblank irqs get resumed if needed. */
> >- for (head = 0; head < dev->mode_config.num_crtc; head++)
> >- drm_vblank_on(dev, head);
> >-
> > /* This should ensure we don't hit a locking problem when someone
> > * wakes us up via a connector. We should never go into suspend
> > * while the display is on anyways.
> >
>
> Tested this one and this hunk breaks suspend/resume. After a suspend/resume
> cycle, all OpenGL apps and composited desktop are dead, as the core can't
> get any vblank irq's enabled anymore.
>
> So the drm_vblank_on() is still needed here.
Hm that's very surprising. As mentioned above the force_mode_restore
should be calling nv_crtc_prepare already and fix this all up for us. I
guess I need to dig out my nv card and trace what's really going on here.
Enabling interrupts when the crtc is off isn't a good idea.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] drm/nouveau: Use drm_vblank_on/off consistently
2015-05-29 17:19 ` Daniel Vetter
@ 2015-05-29 17:23 ` Mario Kleiner
2015-05-29 17:35 ` Daniel Vetter
0 siblings, 1 reply; 14+ messages in thread
From: Mario Kleiner @ 2015-05-29 17:23 UTC (permalink / raw
To: Daniel Vetter
Cc: Daniel Vetter, Intel Graphics Development, Ben Skeggs,
DRI Development, Daniel Vetter
On 05/29/2015 07:19 PM, Daniel Vetter wrote:
> On Fri, May 29, 2015 at 06:50:06PM +0200, Mario Kleiner wrote:
>> On 05/27/2015 11:04 AM, Daniel Vetter wrote:
>>> In
>>>
>>> commit 9cba5efab5a8145ae6c52ea273553f069c294482
>>> Author: Mario Kleiner <mario.kleiner.de@gmail.com>
>>> Date: Tue Jul 29 02:36:44 2014 +0200
>>>
>>> drm/nouveau: Dis/Enable vblank irqs during suspend/resume
>>>
>>> drm_vblank_on/off calls where added around suspend/resume to make sure
>>> vblank stay doesn't go boom over that transition. But nouveau already
>>> used drm_vblank_pre/post_modeset over modesets. Instead use
>>> drm_vblank_on/off everyhwere. The slight change here is that after
>>> _off drm_vblank_get will refuse to work right away, but nouveau
>>> doesn't seem to depend upon that anywhere outside of the pageflip
>>> paths.
>>>
>>> The longer-term plan here is to switch all kms drivers to
>>> drm_vblank_on/off so that common code like pending event cleanup can
>>> be done there, while drm_vblank_pre/post_modeset will be purely
>>> drm internal for the old UMS ioctl.
>>>
>>> Note that the drm_vblank_off still seems required in the suspend path
>>> since nouveau doesn't explicitly disable crtcs. But on the resume side
>>> drm_helper_resume_force_mode should end up calling drm_vblank_on
>>> through the nouveau crtc hooks already. Hence remove the call in the
>>> resume code.
>>>
>>> Cc: Mario Kleiner <mario.kleiner.de@gmail.com>
>>> Cc: Ben Skeggs <bskeggs@redhat.com>
>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>>> ---
>>> drivers/gpu/drm/nouveau/dispnv04/crtc.c | 4 ++--
>>> drivers/gpu/drm/nouveau/nouveau_display.c | 4 ----
>>> 2 files changed, 2 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/nouveau/dispnv04/crtc.c b/drivers/gpu/drm/nouveau/dispnv04/crtc.c
>>> index 3d96b49fe662..dab24066fa21 100644
>>> --- a/drivers/gpu/drm/nouveau/dispnv04/crtc.c
>>> +++ b/drivers/gpu/drm/nouveau/dispnv04/crtc.c
>>> @@ -708,7 +708,7 @@ static void nv_crtc_prepare(struct drm_crtc *crtc)
>>> if (nv_two_heads(dev))
>>> NVSetOwner(dev, nv_crtc->index);
>>>
>>> - drm_vblank_pre_modeset(dev, nv_crtc->index);
>>> + drm_vblank_off(dev, nv_crtc->index);
>>> funcs->dpms(crtc, DRM_MODE_DPMS_OFF);
>>>
>>> NVBlankScreen(dev, nv_crtc->index, true);
>>> @@ -740,7 +740,7 @@ static void nv_crtc_commit(struct drm_crtc *crtc)
>>> #endif
>>>
>>> funcs->dpms(crtc, DRM_MODE_DPMS_ON);
>>> - drm_vblank_post_modeset(dev, nv_crtc->index);
>>> + drm_vblank_on(dev, nv_crtc->index);
>>> }
>>
>> The above hunk is probably correct, but i couldn't test it without
>> sufficiently old pre-nv 50 hardware.
>>
>>>
>>> static void nv_crtc_destroy(struct drm_crtc *crtc)
>>> diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
>>> index 8670d90cdc11..d824023f9fc6 100644
>>> --- a/drivers/gpu/drm/nouveau/nouveau_display.c
>>> +++ b/drivers/gpu/drm/nouveau/nouveau_display.c
>>> @@ -620,10 +620,6 @@ nouveau_display_resume(struct drm_device *dev, bool runtime)
>>> nv_crtc->lut.depth = 0;
>>> }
>>>
>>> - /* Make sure that drm and hw vblank irqs get resumed if needed. */
>>> - for (head = 0; head < dev->mode_config.num_crtc; head++)
>>> - drm_vblank_on(dev, head);
>>> -
>>> /* This should ensure we don't hit a locking problem when someone
>>> * wakes us up via a connector. We should never go into suspend
>>> * while the display is on anyways.
>>>
>>
>> Tested this one and this hunk breaks suspend/resume. After a suspend/resume
>> cycle, all OpenGL apps and composited desktop are dead, as the core can't
>> get any vblank irq's enabled anymore.
>>
>> So the drm_vblank_on() is still needed here.
>
> Hm that's very surprising. As mentioned above the force_mode_restore
> should be calling nv_crtc_prepare already and fix this all up for us. I
> guess I need to dig out my nv card and trace what's really going on here.
>
> Enabling interrupts when the crtc is off isn't a good idea.
> -Daniel
>
I think the nv_crtc_prepare() path modified in your first hunk is only
for the original nv04 display engine for very old cards. nv50+
(GeForce-8 and later) take different paths.
-mario
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] drm/nouveau: Use drm_vblank_on/off consistently
2015-05-29 17:23 ` Mario Kleiner
@ 2015-05-29 17:35 ` Daniel Vetter
2015-06-05 19:40 ` Mario Kleiner
0 siblings, 1 reply; 14+ messages in thread
From: Daniel Vetter @ 2015-05-29 17:35 UTC (permalink / raw
To: Mario Kleiner
Cc: Daniel Vetter, Intel Graphics Development, DRI Development,
Ben Skeggs, Daniel Vetter
On Fri, May 29, 2015 at 07:23:35PM +0200, Mario Kleiner wrote:
>
>
> On 05/29/2015 07:19 PM, Daniel Vetter wrote:
> >On Fri, May 29, 2015 at 06:50:06PM +0200, Mario Kleiner wrote:
> >>On 05/27/2015 11:04 AM, Daniel Vetter wrote:
> >>>In
> >>>
> >>>commit 9cba5efab5a8145ae6c52ea273553f069c294482
> >>>Author: Mario Kleiner <mario.kleiner.de@gmail.com>
> >>>Date: Tue Jul 29 02:36:44 2014 +0200
> >>>
> >>> drm/nouveau: Dis/Enable vblank irqs during suspend/resume
> >>>
> >>>drm_vblank_on/off calls where added around suspend/resume to make sure
> >>>vblank stay doesn't go boom over that transition. But nouveau already
> >>>used drm_vblank_pre/post_modeset over modesets. Instead use
> >>>drm_vblank_on/off everyhwere. The slight change here is that after
> >>>_off drm_vblank_get will refuse to work right away, but nouveau
> >>>doesn't seem to depend upon that anywhere outside of the pageflip
> >>>paths.
> >>>
> >>>The longer-term plan here is to switch all kms drivers to
> >>>drm_vblank_on/off so that common code like pending event cleanup can
> >>>be done there, while drm_vblank_pre/post_modeset will be purely
> >>>drm internal for the old UMS ioctl.
> >>>
> >>>Note that the drm_vblank_off still seems required in the suspend path
> >>>since nouveau doesn't explicitly disable crtcs. But on the resume side
> >>>drm_helper_resume_force_mode should end up calling drm_vblank_on
> >>>through the nouveau crtc hooks already. Hence remove the call in the
> >>>resume code.
> >>>
> >>>Cc: Mario Kleiner <mario.kleiner.de@gmail.com>
> >>>Cc: Ben Skeggs <bskeggs@redhat.com>
> >>>Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> >>>---
> >>> drivers/gpu/drm/nouveau/dispnv04/crtc.c | 4 ++--
> >>> drivers/gpu/drm/nouveau/nouveau_display.c | 4 ----
> >>> 2 files changed, 2 insertions(+), 6 deletions(-)
> >>>
> >>>diff --git a/drivers/gpu/drm/nouveau/dispnv04/crtc.c b/drivers/gpu/drm/nouveau/dispnv04/crtc.c
> >>>index 3d96b49fe662..dab24066fa21 100644
> >>>--- a/drivers/gpu/drm/nouveau/dispnv04/crtc.c
> >>>+++ b/drivers/gpu/drm/nouveau/dispnv04/crtc.c
> >>>@@ -708,7 +708,7 @@ static void nv_crtc_prepare(struct drm_crtc *crtc)
> >>> if (nv_two_heads(dev))
> >>> NVSetOwner(dev, nv_crtc->index);
> >>>
> >>>- drm_vblank_pre_modeset(dev, nv_crtc->index);
> >>>+ drm_vblank_off(dev, nv_crtc->index);
> >>> funcs->dpms(crtc, DRM_MODE_DPMS_OFF);
> >>>
> >>> NVBlankScreen(dev, nv_crtc->index, true);
> >>>@@ -740,7 +740,7 @@ static void nv_crtc_commit(struct drm_crtc *crtc)
> >>> #endif
> >>>
> >>> funcs->dpms(crtc, DRM_MODE_DPMS_ON);
> >>>- drm_vblank_post_modeset(dev, nv_crtc->index);
> >>>+ drm_vblank_on(dev, nv_crtc->index);
> >>> }
> >>
> >>The above hunk is probably correct, but i couldn't test it without
> >>sufficiently old pre-nv 50 hardware.
> >>
> >>>
> >>> static void nv_crtc_destroy(struct drm_crtc *crtc)
> >>>diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
> >>>index 8670d90cdc11..d824023f9fc6 100644
> >>>--- a/drivers/gpu/drm/nouveau/nouveau_display.c
> >>>+++ b/drivers/gpu/drm/nouveau/nouveau_display.c
> >>>@@ -620,10 +620,6 @@ nouveau_display_resume(struct drm_device *dev, bool runtime)
> >>> nv_crtc->lut.depth = 0;
> >>> }
> >>>
> >>>- /* Make sure that drm and hw vblank irqs get resumed if needed. */
> >>>- for (head = 0; head < dev->mode_config.num_crtc; head++)
> >>>- drm_vblank_on(dev, head);
> >>>-
> >>> /* This should ensure we don't hit a locking problem when someone
> >>> * wakes us up via a connector. We should never go into suspend
> >>> * while the display is on anyways.
> >>>
> >>
> >>Tested this one and this hunk breaks suspend/resume. After a suspend/resume
> >>cycle, all OpenGL apps and composited desktop are dead, as the core can't
> >>get any vblank irq's enabled anymore.
> >>
> >>So the drm_vblank_on() is still needed here.
> >
> >Hm that's very surprising. As mentioned above the force_mode_restore
> >should be calling nv_crtc_prepare already and fix this all up for us. I
> >guess I need to dig out my nv card and trace what's really going on here.
> >
> >Enabling interrupts when the crtc is off isn't a good idea.
> >-Daniel
> >
>
> I think the nv_crtc_prepare() path modified in your first hunk is only for
> the original nv04 display engine for very old cards. nv50+ (GeForce-8 and
> later) take different paths.
Oh right totally missed the nv50+ code. I only grepped for
pre/post_modeset ...
Below untested diff should help. I also realized that the pre-nv50 code
lacks drm_vblank_on/off in the dpms callback, so there's more work to do
anyway for this one here.
Thanks, Daniel
diff --git a/drivers/gpu/drm/nouveau/nv50_display.c b/drivers/gpu/drm/nouveau/nv50_display.c
index 7da7958556a3..a16c37d8f7e1 100644
--- a/drivers/gpu/drm/nouveau/nv50_display.c
+++ b/drivers/gpu/drm/nouveau/nv50_display.c
@@ -997,6 +997,10 @@ nv50_crtc_cursor_show_hide(struct nouveau_crtc *nv_crtc, bool show, bool update)
static void
nv50_crtc_dpms(struct drm_crtc *crtc, int mode)
{
+ if (mode == DRM_MODE_DPMS_ON)
+ drm_crtc_vblank_on(crtc);
+ else
+ drm_crtc_vblank_off(crtc);
}
static void
@@ -1036,6 +1040,8 @@ nv50_crtc_prepare(struct drm_crtc *crtc)
}
nv50_crtc_cursor_show_hide(nv_crtc, false, false);
+
+ drm_crtc_vblank_on(crtc);
}
static void
@@ -1045,6 +1051,8 @@ nv50_crtc_commit(struct drm_crtc *crtc)
struct nv50_mast *mast = nv50_mast(crtc->dev);
u32 *push;
+ drm_crtc_vblank_on(crtc);
+
push = evo_wait(mast, 32);
if (push) {
if (nv50_vers(mast) < G82_DISP_CORE_CHANNEL_DMA) {
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] drm/nouveau: Use drm_vblank_on/off consistently
2015-05-29 17:35 ` Daniel Vetter
@ 2015-06-05 19:40 ` Mario Kleiner
0 siblings, 0 replies; 14+ messages in thread
From: Mario Kleiner @ 2015-06-05 19:40 UTC (permalink / raw
To: Daniel Vetter
Cc: Daniel Vetter, Intel Graphics Development, Ben Skeggs,
DRI Development, Daniel Vetter
On 05/29/2015 07:35 PM, Daniel Vetter wrote:
> On Fri, May 29, 2015 at 07:23:35PM +0200, Mario Kleiner wrote:
>>
>>
>> On 05/29/2015 07:19 PM, Daniel Vetter wrote:
>>> On Fri, May 29, 2015 at 06:50:06PM +0200, Mario Kleiner wrote:
>>>> On 05/27/2015 11:04 AM, Daniel Vetter wrote:
>>>>> In
>>>>>
>>>>> commit 9cba5efab5a8145ae6c52ea273553f069c294482
>>>>> Author: Mario Kleiner <mario.kleiner.de@gmail.com>
>>>>> Date: Tue Jul 29 02:36:44 2014 +0200
>>>>>
>>>>> drm/nouveau: Dis/Enable vblank irqs during suspend/resume
>>>>>
>>>>> drm_vblank_on/off calls where added around suspend/resume to make sure
>>>>> vblank stay doesn't go boom over that transition. But nouveau already
>>>>> used drm_vblank_pre/post_modeset over modesets. Instead use
>>>>> drm_vblank_on/off everyhwere. The slight change here is that after
>>>>> _off drm_vblank_get will refuse to work right away, but nouveau
>>>>> doesn't seem to depend upon that anywhere outside of the pageflip
>>>>> paths.
>>>>>
>>>>> The longer-term plan here is to switch all kms drivers to
>>>>> drm_vblank_on/off so that common code like pending event cleanup can
>>>>> be done there, while drm_vblank_pre/post_modeset will be purely
>>>>> drm internal for the old UMS ioctl.
>>>>>
>>>>> Note that the drm_vblank_off still seems required in the suspend path
>>>>> since nouveau doesn't explicitly disable crtcs. But on the resume side
>>>>> drm_helper_resume_force_mode should end up calling drm_vblank_on
>>>>> through the nouveau crtc hooks already. Hence remove the call in the
>>>>> resume code.
>>>>>
>>>>> Cc: Mario Kleiner <mario.kleiner.de@gmail.com>
>>>>> Cc: Ben Skeggs <bskeggs@redhat.com>
>>>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>>>>> ---
>>>>> drivers/gpu/drm/nouveau/dispnv04/crtc.c | 4 ++--
>>>>> drivers/gpu/drm/nouveau/nouveau_display.c | 4 ----
>>>>> 2 files changed, 2 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/nouveau/dispnv04/crtc.c b/drivers/gpu/drm/nouveau/dispnv04/crtc.c
>>>>> index 3d96b49fe662..dab24066fa21 100644
>>>>> --- a/drivers/gpu/drm/nouveau/dispnv04/crtc.c
>>>>> +++ b/drivers/gpu/drm/nouveau/dispnv04/crtc.c
>>>>> @@ -708,7 +708,7 @@ static void nv_crtc_prepare(struct drm_crtc *crtc)
>>>>> if (nv_two_heads(dev))
>>>>> NVSetOwner(dev, nv_crtc->index);
>>>>>
>>>>> - drm_vblank_pre_modeset(dev, nv_crtc->index);
>>>>> + drm_vblank_off(dev, nv_crtc->index);
>>>>> funcs->dpms(crtc, DRM_MODE_DPMS_OFF);
>>>>>
>>>>> NVBlankScreen(dev, nv_crtc->index, true);
>>>>> @@ -740,7 +740,7 @@ static void nv_crtc_commit(struct drm_crtc *crtc)
>>>>> #endif
>>>>>
>>>>> funcs->dpms(crtc, DRM_MODE_DPMS_ON);
>>>>> - drm_vblank_post_modeset(dev, nv_crtc->index);
>>>>> + drm_vblank_on(dev, nv_crtc->index);
>>>>> }
>>>>
>>>> The above hunk is probably correct, but i couldn't test it without
>>>> sufficiently old pre-nv 50 hardware.
>>>>
>>>>>
>>>>> static void nv_crtc_destroy(struct drm_crtc *crtc)
>>>>> diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
>>>>> index 8670d90cdc11..d824023f9fc6 100644
>>>>> --- a/drivers/gpu/drm/nouveau/nouveau_display.c
>>>>> +++ b/drivers/gpu/drm/nouveau/nouveau_display.c
>>>>> @@ -620,10 +620,6 @@ nouveau_display_resume(struct drm_device *dev, bool runtime)
>>>>> nv_crtc->lut.depth = 0;
>>>>> }
>>>>>
>>>>> - /* Make sure that drm and hw vblank irqs get resumed if needed. */
>>>>> - for (head = 0; head < dev->mode_config.num_crtc; head++)
>>>>> - drm_vblank_on(dev, head);
>>>>> -
>>>>> /* This should ensure we don't hit a locking problem when someone
>>>>> * wakes us up via a connector. We should never go into suspend
>>>>> * while the display is on anyways.
>>>>>
>>>>
>>>> Tested this one and this hunk breaks suspend/resume. After a suspend/resume
>>>> cycle, all OpenGL apps and composited desktop are dead, as the core can't
>>>> get any vblank irq's enabled anymore.
>>>>
>>>> So the drm_vblank_on() is still needed here.
>>>
>>> Hm that's very surprising. As mentioned above the force_mode_restore
>>> should be calling nv_crtc_prepare already and fix this all up for us. I
>>> guess I need to dig out my nv card and trace what's really going on here.
>>>
>>> Enabling interrupts when the crtc is off isn't a good idea.
>>> -Daniel
>>>
>>
>> I think the nv_crtc_prepare() path modified in your first hunk is only for
>> the original nv04 display engine for very old cards. nv50+ (GeForce-8 and
>> later) take different paths.
>
> Oh right totally missed the nv50+ code. I only grepped for
> pre/post_modeset ...
>
> Below untested diff should help. I also realized that the pre-nv50 code
> lacks drm_vblank_on/off in the dpms callback, so there's more work to do
> anyway for this one here.
>
> Thanks, Daniel
>
The diff on top of your patch is now tested and helps. suspend->resume
is now fine on nv50. In your patch, nouveau_display_resume() would also
need to get a now unused "int head" removed to make the compiler happy.
-mario
> diff --git a/drivers/gpu/drm/nouveau/nv50_display.c b/drivers/gpu/drm/nouveau/nv50_display.c
> index 7da7958556a3..a16c37d8f7e1 100644
> --- a/drivers/gpu/drm/nouveau/nv50_display.c
> +++ b/drivers/gpu/drm/nouveau/nv50_display.c
> @@ -997,6 +997,10 @@ nv50_crtc_cursor_show_hide(struct nouveau_crtc *nv_crtc, bool show, bool update)
> static void
> nv50_crtc_dpms(struct drm_crtc *crtc, int mode)
> {
> + if (mode == DRM_MODE_DPMS_ON)
> + drm_crtc_vblank_on(crtc);
> + else
> + drm_crtc_vblank_off(crtc);
> }
>
> static void
> @@ -1036,6 +1040,8 @@ nv50_crtc_prepare(struct drm_crtc *crtc)
> }
>
> nv50_crtc_cursor_show_hide(nv_crtc, false, false);
> +
> + drm_crtc_vblank_on(crtc);
> }
>
> static void
> @@ -1045,6 +1051,8 @@ nv50_crtc_commit(struct drm_crtc *crtc)
> struct nv50_mast *mast = nv50_mast(crtc->dev);
> u32 *push;
>
> + drm_crtc_vblank_on(crtc);
> +
> push = evo_wait(mast, 32);
> if (push) {
> if (nv50_vers(mast) < G82_DISP_CORE_CHANNEL_DMA) {
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] drm/radeon: Switch to drm_vblank_on/off
2015-05-28 9:03 ` Michel Dänzer
@ 2015-07-14 8:13 ` Michel Dänzer
0 siblings, 0 replies; 14+ messages in thread
From: Michel Dänzer @ 2015-07-14 8:13 UTC (permalink / raw
To: Daniel Vetter; +Cc: Daniel Vetter, Intel Graphics Development, DRI Development
On 28.05.2015 18:03, Michel Dänzer wrote:
> On 28.05.2015 17:38, Daniel Vetter wrote:
>> On Thu, May 28, 2015 at 04:11:53PM +0900, Michel Dänzer wrote:
>>> On 27.05.2015 18:41, Daniel Vetter wrote:
>>>> On Wed, May 27, 2015 at 06:21:24PM +0900, Michel Dänzer wrote:
>>>>> On 27.05.2015 18:04, Daniel Vetter wrote:
>>>>>> These should be functionally equivalent to the older per/post modeset
>>>>>> functions, except that they block out drm_vblank_get right away.
>>>>>> There's only the clock adjusting code (outside of pageflips) in
>>>>>> readone which uses drm_vblank_get. But that code doesn't synchronize
>>>>>> against concurrent modesets and instead handles any such races by
>>>>>> waiting for the right vblank to arrive with a short timetout.
>>>>>>
>>>>>> The longer-term plan here is to switch all kms drivers to
>>>>>> drm_vblank_on/off so that common code like pending event cleanup can
>>>>>> be done there, while drm_vblank_pre/post_modeset will be purely
>>>>>> drm internal for the old UMS ioctl.
>>>>>>
>>>>>> Note that the kerneldoc for pre/post_modeset is wrong since as Michel
>>>>>> Dänzer correctly pointed out it works if only using pre/post_modeset.
>>>>>> The trouble that lead to this comment is the very old version of
>>>>>> drm_vblank_off to clear out pending events when disabling a pipe,
>>>>>> which did seem to wreak havoc with the trick used by pre/post_modeset.
>>>>>> Michel also expressed dissatisfaction with intel folks pushing new
>>>>>> interfaces with bogus justifications. I still maintain that having a
>>>>>> consistent set of vblank behaviour across kms drivers, separate from
>>>>>> any old UMS functions is a useful goal.
>>>>>>
>>>>>> Cc: Michel Dänzer <michel.daenzer@amd.com>
>>>>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>>>>>
>>>>> Can you describe at least one tangible benefit this change provides for
>>>>> the radeon driver?
>>>>>
>>>>> Because I'm afraid that this might cause subtle breakage, and since we
>>>>> don't have any rigorous tests for this like in intel-gpu-tools (yet?),
>>>>> it might be painful to track it down.
>>>>>
>>>>> So, I'd like to have a good reason for taking the risk.
>>>>
>>>> right now at most a bit of code to clean out pending events on modeset
>>>> disable, for somewhat consistent behaviour with other drivers. But in
>>>> general it's fairly ill-defined what happens with vblank events.
>>>
>>> Yeah, while that's nice to have, I don't think it makes too much
>>> difference in practice.
>>>
>>> Anyway, I'm giving this patch a spin, and it does indeed cause userspace
>>> fallout, at least with DRI3/Present enabled, because the vblank and
>>> pageflip ioctls now return -EINVAL while the CRTC is off. However, it
>>> looks like fixing that up might not be too bad, so I'm cautiously
>>> optimistic for this change. But I'd like some more time for testing and
>>> fixing userspace.
>
> [...]
>
>> Otoh asking for a vblank event on a dead pipe smells like a userspace bug
>> and could result in stuck compositors. Not sure what's best here really.
>
> Agreed, and we're already careful not to do that with DRI2, just not yet
> with DRI3/Present (which isn't in any xf86-video-ati release yet).
I've fixed up the DRI3/Present code as well. This patch (with maybe some
cleanups to the commit log, in particular I'm not sure the third
paragraph should be there) is
Acked-and-Tested-by: Michel Dänzer <michel.daenzer@amd.com>
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2015-07-14 8:13 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-05-27 9:04 [PATCH 1/3] drm/nouveau: Use drm_vblank_on/off consistently Daniel Vetter
2015-05-27 9:04 ` [PATCH 2/3] drm/radeon: Switch to drm_vblank_on/off Daniel Vetter
2015-05-27 9:21 ` Michel Dänzer
2015-05-27 9:41 ` Daniel Vetter
2015-05-28 7:11 ` Michel Dänzer
2015-05-28 8:38 ` Daniel Vetter
2015-05-28 9:03 ` Michel Dänzer
2015-07-14 8:13 ` Michel Dänzer
2015-05-27 9:04 ` [PATCH 3/3] drm/irq: Make drm_vblank_pre/post_modeset internal Daniel Vetter
2015-05-29 16:50 ` [PATCH 1/3] drm/nouveau: Use drm_vblank_on/off consistently Mario Kleiner
2015-05-29 17:19 ` Daniel Vetter
2015-05-29 17:23 ` Mario Kleiner
2015-05-29 17:35 ` Daniel Vetter
2015-06-05 19:40 ` Mario Kleiner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).