Add a reset interface

Add a reset interface

[Chris Wilson]

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(-)

[PATCH 1/4] drm: Add an interface to reset the device

[Chris Wilson]

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);
-- 
1.7.2.3

Re: [PATCH 1/4] drm: Add an interface to reset the device

[Takashi Iwai]

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

Re: [PATCH 1/4] drm: Add an interface to reset the device

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/4] drm: Add an interface to reset the device

[Alex Deucher]

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
>

Re: [PATCH 1/4] drm: Add an interface to reset the device

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 2/4] drm/i915: Reset state after a GPU reset or resume

[Chris Wilson]

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);
 	}
-- 
1.7.2.3

[PATCH 3/4] drm/i915/crt: Force the initial probe after reset

[Chris Wilson]

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,
-- 
1.7.2.3

[PATCH 4/4] drm/i915: Reset crtc after resume

[Chris Wilson]

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;
-- 
1.7.2.3

Re: Add a reset interface

[Takashi Iwai]

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

Re: Add a reset interface

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: Add a reset interface

[Dave Airlie]

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.

Re: Add a reset interface

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre