For i915 there is a need to invalidate some cached state after resuming or reseting the GPU. This is not quite the same as simply restoring saved state (i.e. the standard suspend resume method), so do not seem to merit reusing the save|restore vfuncs. Instead I propose a
drm_mode_config_reset(struct drm_device *);
routine to iterate over all the attached CRTCs, encoders and connectors and call any supplied reset vfunc.
This is required to fix some modesetting regressions across resume in 2.6.38: https://bugzilla.kernel.org/show_bug.cgi?id=26952 https://bugzilla.kernel.org/show_bug.cgi?id=27272
Please review, -Chris
drivers/gpu/drm/drm_crtc.c | 19 +++++++++++++++++++ drivers/gpu/drm/i915/i915_drv.c | 2 ++ drivers/gpu/drm/i915/intel_crt.c | 10 ++++++++++ drivers/gpu/drm/i915/intel_display.c | 17 ++++++++++++++--- include/drm/drm_crtc.h | 7 +++++++ 5 files changed, 52 insertions(+), 3 deletions(-)
Iterate over the attached CRTCs, encoders and connectors and call the supplied reset vfunc in order to reset any cached state back to unknown. Useful after an invalidation event such as a GPU reset or resuming.
Signed-off-by: Chris Wilson chris@chris-wilson.co.uk --- drivers/gpu/drm/drm_crtc.c | 19 +++++++++++++++++++ include/drm/drm_crtc.h | 7 +++++++ 2 files changed, 26 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 2baa670..6d7323d 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -2674,3 +2674,22 @@ out: mutex_unlock(&dev->mode_config.mutex); return ret; } + +void drm_mode_config_reset(struct drm_device *dev) +{ + struct drm_crtc *crtc; + struct drm_encoder *encoder; + struct drm_connector *connector; + + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) + if (crtc->funcs->reset) + crtc->funcs->reset(crtc); + + list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) + if (encoder->funcs->reset) + encoder->funcs->reset(encoder); + + list_for_each_entry(connector, &dev->mode_config.connector_list, head) + if (connector->funcs->reset) + connector->funcs->reset(connector); +} diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index acd7fad..801be59 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -275,6 +275,7 @@ struct drm_pending_vblank_event;
/** * drm_crtc_funcs - control CRTCs for a given device + * @reset: reset CRTC after state has been invalidate (e.g. resume) * @dpms: control display power levels * @save: save CRTC state * @resore: restore CRTC state @@ -302,6 +303,8 @@ struct drm_crtc_funcs { void (*save)(struct drm_crtc *crtc); /* suspend? */ /* Restore CRTC state */ void (*restore)(struct drm_crtc *crtc); /* resume? */ + /* Reset CRTC state */ + void (*reset)(struct drm_crtc *crtc);
/* cursor controls */ int (*cursor_set)(struct drm_crtc *crtc, struct drm_file *file_priv, @@ -379,6 +382,7 @@ struct drm_crtc { * @dpms: set power state (see drm_crtc_funcs above) * @save: save connector state * @restore: restore connector state + * @reset: reset connector after state has been invalidate (e.g. resume) * @mode_valid: is this mode valid on the given connector? * @mode_fixup: try to fixup proposed mode for this connector * @mode_set: set this mode @@ -396,6 +400,7 @@ struct drm_connector_funcs { void (*dpms)(struct drm_connector *connector, int mode); void (*save)(struct drm_connector *connector); void (*restore)(struct drm_connector *connector); + void (*reset)(struct drm_connector *connector);
/* Check to see if anything is attached to the connector. * @force is set to false whilst polling, true when checking the @@ -413,6 +418,7 @@ struct drm_connector_funcs { };
struct drm_encoder_funcs { + void (*reset)(struct drm_encoder *encoder); void (*destroy)(struct drm_encoder *encoder); };
@@ -656,6 +662,7 @@ extern struct drm_display_mode *drm_mode_duplicate(struct drm_device *dev, struct drm_display_mode *mode); extern void drm_mode_debug_printmodeline(struct drm_display_mode *mode); extern void drm_mode_config_init(struct drm_device *dev); +extern void drm_mode_config_reset(struct drm_device *dev); extern void drm_mode_config_cleanup(struct drm_device *dev); extern void drm_mode_set_name(struct drm_display_mode *mode); extern bool drm_mode_equal(struct drm_display_mode *mode1, struct drm_display_mode *mode2);
At Mon, 24 Jan 2011 15:55:28 +0000, Chris Wilson wrote:
Iterate over the attached CRTCs, encoders and connectors and call the supplied reset vfunc in order to reset any cached state back to unknown. Useful after an invalidation event such as a GPU reset or resuming.
Signed-off-by: Chris Wilson chris@chris-wilson.co.uk
drivers/gpu/drm/drm_crtc.c | 19 +++++++++++++++++++ include/drm/drm_crtc.h | 7 +++++++ 2 files changed, 26 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 2baa670..6d7323d 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -2674,3 +2674,22 @@ out: mutex_unlock(&dev->mode_config.mutex); return ret; }
+void drm_mode_config_reset(struct drm_device *dev) +{
- struct drm_crtc *crtc;
- struct drm_encoder *encoder;
- struct drm_connector *connector;
- list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
if (crtc->funcs->reset)crtc->funcs->reset(crtc);- list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
if (encoder->funcs->reset)encoder->funcs->reset(encoder);- list_for_each_entry(connector, &dev->mode_config.connector_list, head)
if (connector->funcs->reset)connector->funcs->reset(connector);+}
Missing EXPORT_SYMBOL()?
thanks,
Takashi
On Mon, 24 Jan 2011 17:18:06 +0100, Takashi Iwai tiwai@suse.de wrote:
Missing EXPORT_SYMBOL()?
Thanks. Modules are for the weak ;-) -Chris
On Mon, Jan 24, 2011 at 10:55 AM, Chris Wilson chris@chris-wilson.co.uk wrote:
Iterate over the attached CRTCs, encoders and connectors and call the supplied reset vfunc in order to reset any cached state back to unknown. Useful after an invalidation event such as a GPU reset or resuming.
Can't you just reprogram the modes at resume? I guess it would help to see what you are actually doing with this hook.
Alex
Signed-off-by: Chris Wilson chris@chris-wilson.co.uk
drivers/gpu/drm/drm_crtc.c | 19 +++++++++++++++++++ include/drm/drm_crtc.h | 7 +++++++ 2 files changed, 26 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 2baa670..6d7323d 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -2674,3 +2674,22 @@ out: mutex_unlock(&dev->mode_config.mutex); return ret; }
+void drm_mode_config_reset(struct drm_device *dev) +{
- struct drm_crtc *crtc;
- struct drm_encoder *encoder;
- struct drm_connector *connector;
- list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
- if (crtc->funcs->reset)
- crtc->funcs->reset(crtc);
- list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
- if (encoder->funcs->reset)
- encoder->funcs->reset(encoder);
- list_for_each_entry(connector, &dev->mode_config.connector_list, head)
- if (connector->funcs->reset)
- connector->funcs->reset(connector);
+} diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index acd7fad..801be59 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -275,6 +275,7 @@ struct drm_pending_vblank_event;
/** * drm_crtc_funcs - control CRTCs for a given device
- @reset: reset CRTC after state has been invalidate (e.g. resume)
* @dpms: control display power levels * @save: save CRTC state * @resore: restore CRTC state @@ -302,6 +303,8 @@ struct drm_crtc_funcs { void (*save)(struct drm_crtc *crtc); /* suspend? */ /* Restore CRTC state */ void (*restore)(struct drm_crtc *crtc); /* resume? */
- /* Reset CRTC state */
- void (*reset)(struct drm_crtc *crtc);
/* cursor controls */ int (*cursor_set)(struct drm_crtc *crtc, struct drm_file *file_priv, @@ -379,6 +382,7 @@ struct drm_crtc { * @dpms: set power state (see drm_crtc_funcs above) * @save: save connector state * @restore: restore connector state
- @reset: reset connector after state has been invalidate (e.g. resume)
* @mode_valid: is this mode valid on the given connector? * @mode_fixup: try to fixup proposed mode for this connector * @mode_set: set this mode @@ -396,6 +400,7 @@ struct drm_connector_funcs { void (*dpms)(struct drm_connector *connector, int mode); void (*save)(struct drm_connector *connector); void (*restore)(struct drm_connector *connector);
- void (*reset)(struct drm_connector *connector);
/* Check to see if anything is attached to the connector. * @force is set to false whilst polling, true when checking the @@ -413,6 +418,7 @@ struct drm_connector_funcs { };
struct drm_encoder_funcs {
- void (*reset)(struct drm_encoder *encoder);
void (*destroy)(struct drm_encoder *encoder); };
@@ -656,6 +662,7 @@ extern struct drm_display_mode *drm_mode_duplicate(struct drm_device *dev, struct drm_display_mode *mode); extern void drm_mode_debug_printmodeline(struct drm_display_mode *mode); extern void drm_mode_config_init(struct drm_device *dev); +extern void drm_mode_config_reset(struct drm_device *dev); extern void drm_mode_config_cleanup(struct drm_device *dev); extern void drm_mode_set_name(struct drm_display_mode *mode); extern bool drm_mode_equal(struct drm_display_mode *mode1, struct drm_display_mode *mode2); -- 1.7.2.3
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Mon, 24 Jan 2011 12:08:29 -0500, Alex Deucher alexdeucher@gmail.com wrote:
On Mon, Jan 24, 2011 at 10:55 AM, Chris Wilson chris@chris-wilson.co.uk wrote:
Iterate over the attached CRTCs, encoders and connectors and call the supplied reset vfunc in order to reset any cached state back to unknown. Useful after an invalidation event such as a GPU reset or resuming.
Can't you just reprogram the modes at resume? I guess it would help to see what you are actually doing with this hook.
I tried to show just how I intended to use during resume and resetting a hung GPU, and there is also a similarity with boot up (which may share the same code). In these 2 instances were the GPU state becomes inconsistent with the state in the connectors/crtc and so we need to reset. (Not doing so means that the mode programming on resume fails.)
There is a large semantic difference between restore and reset, and since reset was useful outside of resume, I felt the need for an additional interface. -Chris
Call drm_mode_config_reset() after an invalidation event to restore any cached state to unknown.
Signed-off-by: Chris Wilson chris@chris-wilson.co.uk --- drivers/gpu/drm/i915/i915_drv.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 66796bb..e517447 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -354,6 +354,7 @@ static int i915_drm_thaw(struct drm_device *dev) error = i915_gem_init_ringbuffer(dev); mutex_unlock(&dev->struct_mutex);
+ drm_mode_config_reset(dev); drm_irq_install(dev);
/* Resume the modeset for every activated CRTC */ @@ -542,6 +543,7 @@ int i915_reset(struct drm_device *dev, u8 flags)
mutex_unlock(&dev->struct_mutex); drm_irq_uninstall(dev); + drm_mode_config_reset(dev); drm_irq_install(dev); mutex_lock(&dev->struct_mutex); }
Upon resume, like after a cold boot, we need to forcibly probe the analog connector and cannot rely on the hotplug status.
Based on a patch by Takashi Iwai.
Reported-by: Stefan Dirsch sndirsch@suse.de Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=26952 Signed-off-by: Chris Wilson chris@chris-wilson.co.uk --- drivers/gpu/drm/i915/intel_crt.c | 10 ++++++++++ 1 files changed, 10 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c index 17035b8..8a77ff4 100644 --- a/drivers/gpu/drm/i915/intel_crt.c +++ b/drivers/gpu/drm/i915/intel_crt.c @@ -535,6 +535,15 @@ static int intel_crt_set_property(struct drm_connector *connector, return 0; }
+static void intel_crt_reset(struct drm_connector *connector) +{ + struct drm_device *dev = connector->dev; + struct intel_crt *crt = intel_attached_crt(connector); + + if (HAS_PCH_SPLIT(dev)) + crt->force_hotplug_required = 1; +} + /* * Routines for controlling stuff on the analog port */ @@ -548,6 +557,7 @@ static const struct drm_encoder_helper_funcs intel_crt_helper_funcs = { };
static const struct drm_connector_funcs intel_crt_connector_funcs = { + .reset = intel_crt_reset, .dpms = drm_helper_connector_dpms, .detect = intel_crt_detect, .fill_modes = drm_helper_probe_single_connector_modes,
Based on a patch by Takashi Iwai.
Reported-by: Matthias Hopf mat@mshopf.de Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=27272 Signed-off-by: Chris Wilson chris@chris-wilson.co.uk --- drivers/gpu/drm/i915/intel_display.c | 17 ++++++++++++++--- 1 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index d7f237d..7e42aa5 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -5551,6 +5551,18 @@ cleanup_work: return ret; }
+static void intel_crtc_reset(struct drm_crtc *crtc) +{ + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + + /* Reset flags back to the 'unknown' status so that they + * will be correctly set on the initial modeset. + */ + intel_crtc->cursor_addr = 0; + intel_crtc->dpms_mode = -1; + intel_crtc->active = true; /* force the pipe off on setup_init_config */ +} + static struct drm_crtc_helper_funcs intel_helper_funcs = { .dpms = intel_crtc_dpms, .mode_fixup = intel_crtc_mode_fixup, @@ -5562,6 +5574,7 @@ static struct drm_crtc_helper_funcs intel_helper_funcs = { };
static const struct drm_crtc_funcs intel_crtc_funcs = { + .reset = intel_crtc_reset, .cursor_set = intel_crtc_cursor_set, .cursor_move = intel_crtc_cursor_move, .gamma_set = intel_crtc_gamma_set, @@ -5652,9 +5665,7 @@ static void intel_crtc_init(struct drm_device *dev, int pipe) dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base; dev_priv->pipe_to_crtc_mapping[intel_crtc->pipe] = &intel_crtc->base;
- intel_crtc->cursor_addr = 0; - intel_crtc->dpms_mode = -1; - intel_crtc->active = true; /* force the pipe off on setup_init_config */ + intel_crtc_reset(&intel_crtc->base);
if (HAS_PCH_SPLIT(dev)) { intel_helper_funcs.prepare = ironlake_crtc_prepare;
At Mon, 24 Jan 2011 15:55:27 +0000, Chris Wilson wrote:
For i915 there is a need to invalidate some cached state after resuming or reseting the GPU. This is not quite the same as simply restoring saved state (i.e. the standard suspend resume method), so do not seem to merit reusing the save|restore vfuncs. Instead I propose a
drm_mode_config_reset(struct drm_device *);
routine to iterate over all the attached CRTCs, encoders and connectors and call any supplied reset vfunc.
This is required to fix some modesetting regressions across resume in 2.6.38: https://bugzilla.kernel.org/show_bug.cgi?id=26952 https://bugzilla.kernel.org/show_bug.cgi?id=27272
I quickly tried these patches. After adding the missing EXPORT_SYMBOL, it seems working fine. Tested on a SNB laptop and a PineView laptop.
Put my tag to all patches: Tested-by: Takashi Iwai tiwai@suse.de
Thanks!
Takashi
On Mon, 24 Jan 2011 15:55:27 +0000, Chris Wilson chris@chris-wilson.co.uk wrote:
For i915 there is a need to invalidate some cached state after resuming or reseting the GPU. This is not quite the same as simply restoring saved state (i.e. the standard suspend resume method), so do not seem to merit reusing the save|restore vfuncs. Instead I propose a
drm_mode_config_reset(struct drm_device *);
routine to iterate over all the attached CRTCs, encoders and connectors and call any supplied reset vfunc.
This is required to fix some modesetting regressions across resume in 2.6.38: https://bugzilla.kernel.org/show_bug.cgi?id=26952 https://bugzilla.kernel.org/show_bug.cgi?id=27272
Dave, what's your take on adding a new (crtc|encoder|connector)->reset() vfunc to drm core? Do I need to code up an Intel specific alternative?
(Poking since this fixes a regression in .38.)
Thanks, -Chris
On Sun, Jan 30, 2011 at 9:50 PM, Chris Wilson chris@chris-wilson.co.uk wrote:
On Mon, 24 Jan 2011 15:55:27 +0000, Chris Wilson chris@chris-wilson.co.uk wrote:
For i915 there is a need to invalidate some cached state after resuming or reseting the GPU. This is not quite the same as simply restoring saved state (i.e. the standard suspend resume method), so do not seem to merit reusing the save|restore vfuncs. Instead I propose a
drm_mode_config_reset(struct drm_device *);
routine to iterate over all the attached CRTCs, encoders and connectors and call any supplied reset vfunc.
This is required to fix some modesetting regressions across resume in 2.6.38: https://bugzilla.kernel.org/show_bug.cgi?id=26952 https://bugzilla.kernel.org/show_bug.cgi?id=27272
Dave, what's your take on adding a new (crtc|encoder|connector)->reset() vfunc to drm core? Do I need to code up an Intel specific alternative?
It seems like an interface we could use, I just wasn't sure how a regression fix for something in. 38 isn't a revert but requires a whole new interface in the core drm to fix. Perhaps if you can enlighten me on that I'd be happier.
Dave.
On Mon, 31 Jan 2011 06:03:25 +1000, Dave Airlie airlied@gmail.com wrote:
It seems like an interface we could use, I just wasn't sure how a regression fix for something in. 38 isn't a revert but requires a whole new interface in the core drm to fix. Perhaps if you can enlighten me on that I'd be happier.
We've made several mistakes along the way, which can be surmised as forgetting to reset state upon resume. (The same bug affects the GPU reset paths, but I don't expect to receive regression reports following a hang...)
The bug fixes are themselves just a couple of missing lines (excluding the many more lines actually required to access the right structures), but the failure is systematic, and I think there may be more bugs (or at least susceptible to the same bugs). And so it will be useful to have the infrastructure in place to fix those bugs.
The other aspect is that reverting the commits responsible reintroduces undesirable behaviour during normal operation; flickering during output polling and fdi training failure (due to multiple calls). The failures upon resume are by comparison the lesser of the evils.
But trivial to fix. -Chris
dri-devel@lists.freedesktop.org
-
Alex Deucher -
Chris Wilson -
Dave Airlie -
Takashi Iwai