Re: [Intel-gfx] [PATCH driver/intel] sna/cursor: Make sure hw cursors are disabled before disabling secondary planes

[Egbert Eich]

On Tue, Jun 21, 2016 at 09:25:36PM +0100, Chris Wilson wrote:
> On Tue, Jun 21, 2016 at 07:34:34PM +0200, Egbert Eich wrote:
> > When the hw cursors are not disabled before the cursor planes get disabled
> > we may lose the cursor later on. Thus make sure the cursors are disabled
> > before the cursor planes are.
> 
> The cursor would already be controlled by the xf86SetDesiredModes(), so
> we can skip disabling entirely. What we should do instead is add the
> paranoia check, but I can't see an easy way to inquire what the kernel
> thinks the legacy cursor handle should be.
> 
> commit f1c757e4518f6835bbff6c940269a5c6be75f202
> Author: Chris Wilson <ch...@chris-wilson.co.uk>
> Date:   Tue Jun 21 21:17:15 2016 +0100
> 
> sna: Only shutdown unknown secondary planes on CRTC we control
> 
> In a ZaphodHead scenario, we do not own all the CRTC and so we should
> not be making changes outside of our zone of control. Also, we only want
> to disable secondary overlay planes and ignore the secondary cursor
> planes which are controlled through the normal modesetting.
> 
> As we are now tracking all sprite planes on a CRTC, this leads to much
> simpler code.

Chris, thanks for the patch! I've been able to test it now - it works.

Cheers,
Egbert.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH driver/intel] sna/cursor: Make sure hw cursors are disabled before disabling secondary planes

[Egbert Eich]

When the hw cursors are not disabled before the cursor planes get disabled
we may lose the cursor later on. Thus make sure the cursors are disabled
before the cursor planes are.

Signed-off-by: Egbert Eich <e...@freedesktop.org>
---
 src/sna/sna_display.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/sna/sna_display.c b/src/sna/sna_display.c
index d790975..412c192 100644
--- a/src/sna/sna_display.c
+++ b/src/sna/sna_display.c
@@ -8081,6 +8081,9 @@ void sna_mode_check(struct sna *sna)
if (sna->mode.hidden)
return;
 
+/* make sure the hw cursors are disabled before disabling
+  the secondary planes which include the cursor plane */
+   sna_hide_cursors(sna->scrn);
disabled = sna_mode_disable_secondary_planes(sna);
 
/* Validate CRTC attachments and force consistency upon the kernel */
-- 
2.7.0

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC 3/8] drm/i915/ivb: Move WaCxSRDisabledForSpriteScaling w/a to atomic check

[Egbert Eich]

Regarding commit 7809e5ae35b9d8d0710f0874b2e3f10be144e38b

On Wed, Jul 01, 2015 at 07:25:56PM -0700, Matt Roper wrote:
> Determine whether we need to apply this workaround at atomic check time
> and just set a flag that will be used by the main watermark update
> routine.
> 
> Moving this workaround into the atomic framework reduces
> ilk_update_sprite_wm() to just a standard watermark update, so drop it
> completely and just ensure that ilk_update_wm() is called whenever a
> sprite plane is updated in a way that would affect watermarks.
> 
> Signed-off-by: Matt Roper 


This patch causes intel_update_watermarks() to be called much more
frequently although the watermark values don't change. 
The change responsible for this is:

> index 5de1ded..46ef981 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11560,23 +11560,38 @@ retry:
>  static bool intel_wm_need_update(struct drm_plane *plane,
>struct drm_plane_state *state)
>  {
> - /* Update watermarks on tiling changes. */
> + struct intel_plane_state *new = to_intel_plane_state(state);
> + struct intel_plane_state *cur = to_intel_plane_state(plane->state);
> +
> + /* Update watermarks on tiling or size changes. */
>   if (!plane->state->fb || !state->fb ||
>   plane->state->fb->modifier[0] != state->fb->modifier[0] ||
> - plane->state->rotation != state->rotation)
> - return true;
> -

> - if (plane->state->crtc_w != state->crtc_w)

A quick look thru intel_pm.c reveals that this is  relevant for
WM caluclations for gen=4 and any chipsets for which is_g4x is true.
Should this really be removed?

> + plane->state->rotation != state->rotation ||

> + drm_rect_width(>src) != drm_rect_width(>src) ||
> + drm_rect_height(>src) != drm_rect_height(>src) ||
> + drm_rect_width(>dst) != drm_rect_width(>dst) ||
> + drm_rect_height(>dst) != drm_rect_height(>dst))

these values seem to be used only for watermark calculations on gen < 9 when
HAS_PCH_SPLIT() is true.

Still these are responsible for most of the watermark recalculations (when the 
mouse
cursor is moved towards the edge for example). On the system I'm looking at the 
moment
(Q35) changes in these values don't change the WMs.

Since WM calculation is very chip generation specific and differs considerably 
between
generations, wouldn't it make sense to either have chipset specific functions 
to determin
whether a recalculation is needed - or even perfrom this in the update_wm() 
function
itself?

Cheers,
Egbert.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [drm-intel:for-linux-next-fixes 3/4] DockBook: drivers/gpu/drm/drm_probe_helper.c:107: warning: Excess function parameter 'dev' description in 'DRM_OUTPUT_POLL_PERIOD'

[Egbert Eich]

Jani Nikula writes:
 > On Wed, 30 Sep 2015, Daniel Vetter  wrote:
 > > On Wed, Sep 30, 2015 at 05:09:04PM +0800, kbuild test robot wrote:
 > >> tree:   git://anongit.freedesktop.org/drm-intel for-linux-next-fixes
 > >> head:   ad96c5f13442b17fafccc30f81efae2f08351f99
 > >> commit: 10d3a5618b3aba24d6388ccdff2d0182b72a6e8d [3/4] drm: Add a 
 > >> non-locking version of drm_kms_helper_poll_enable(), v2
 > >> reproduce: make htmldocs
 > >> 
 > >> All warnings (new ones prefixed by >>):
 > 
 > Cc: Jonathan and Danilo, and including the kernel-doc in question for
 > reference:
 > 
 > /**
 >  * drm_kms_helper_poll_enable_locked - re-enable output polling.
 >  * @dev: drm_device
 >  *
 >  * This function re-enables the output polling work without
 >  * locking the mode_config mutex.
 >  *
 >  * This is like drm_kms_helper_poll_enable() however it is to be
 >  * called from a context where the mode_config mutex is locked
 >  * already.
 >  */
 > #define DRM_OUTPUT_POLL_PERIOD (10*HZ)
 > void drm_kms_helper_poll_enable_locked(struct drm_device *dev)
 > {
 >  ...
 > 
 > >> >> drivers/gpu/drm/drm_probe_helper.c:107: warning: Excess function 
 > >> >> parameter 'dev' description in 'DRM_OUTPUT_POLL_PERIOD'
 > >> >> drivers/gpu/drm/drm_probe_helper.c:107: warning: Excess function 
 > >> >> parameter 'dev' description in 'DRM_OUTPUT_POLL_PERIOD'
 > >
 > > I think this should be fixed by moving the DRM_OUTPUT_POLL_PERIOD #define
 > > before the kerneldoc for drm_kms_helper_poll_enable_locked. Jani, can you
 > > please do that fixup and check that make htmldocs is happy with it?
 > 
 > Can do.
 > 
 > However, having such #defines right above the only function that uses
 > them is not uncommon. Since there is no documentation for
 > DRM_OUTPUT_POLL_PERIOD, and the documentation for the function includes
 > the function name, I am wondering if kernel-doc could be made smarter
 > about this.
 > 

It is actually used twice in this file by two functions not immediately 
adjacent.
Why not move it to the beginning of the file?

Cheers,
Egbert.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915: On reset/suspend disable hpd pins & cancel pending delayed work

[Egbert Eich]

This makes sure no hpd interrupt or reenable worker fires when
resetting or suspending.
We already call intel_hpd_init() in most cases on resume and
after reset to undo this.

Signed-off-by: Egbert Eich <e...@suse.de>
---
 drivers/gpu/drm/i915/i915_drv.c  |  3 +++
 drivers/gpu/drm/i915/i915_drv.h  |  1 +
 drivers/gpu/drm/i915/intel_display.c |  5 +++--
 drivers/gpu/drm/i915/intel_hotplug.c | 28 
 4 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index e2bf9e2..cb8d75f 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -620,6 +620,7 @@ static int i915_drm_suspend(struct drm_device *dev)
 
intel_dp_mst_suspend(dev);
 
+   intel_hpd_uninit(dev_priv);
intel_runtime_pm_disable_interrupts(dev_priv);
intel_hpd_cancel_work(dev_priv);
 
@@ -1477,12 +1478,14 @@ static int intel_runtime_suspend(struct device *device)
mutex_unlock(>struct_mutex);
 
intel_suspend_gt_powersave(dev);
+   intel_hpd_uninit(dev_priv);
intel_runtime_pm_disable_interrupts(dev_priv);
 
ret = intel_suspend_complete(dev_priv);
if (ret) {
DRM_ERROR("Runtime suspend failed, disabling it (%d)\n", ret);
intel_runtime_pm_enable_interrupts(dev_priv);
+   intel_hpd_init(dev_priv);
 
return ret;
}
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a6b7576..3c06629 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2688,6 +2688,7 @@ void i915_firmware_load_error_print(const char *fw_path, 
int err);
 /* intel_hotplug.c */
 void intel_hpd_irq_handler(struct drm_device *dev, u32 pin_mask, u32 
long_mask);
 void intel_hpd_init(struct drm_i915_private *dev_priv);
+void intel_hpd_uninit(struct drm_i915_private *dev_priv);
 void intel_hpd_init_work(struct drm_i915_private *dev_priv);
 void intel_hpd_cancel_work(struct drm_i915_private *dev_priv);
 bool intel_hpd_pin_to_port(enum hpd_pin pin, enum port *port);
diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index fc00867..fadd4f2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3207,16 +3207,17 @@ void intel_finish_reset(struct drm_device *dev)
 * The display has been reset as well,
 * so need a full re-initialization.
 */
+   intel_hpd_uninit(dev_priv);
intel_runtime_pm_disable_interrupts(dev_priv);
intel_runtime_pm_enable_interrupts(dev_priv);
 
intel_modeset_init_hw(dev);
-
+#if 0
spin_lock_irq(_priv->irq_lock);
if (dev_priv->display.hpd_irq_setup)
dev_priv->display.hpd_irq_setup(dev);
spin_unlock_irq(_priv->irq_lock);
-
+#endif
intel_display_resume(dev);
 
intel_hpd_init(dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_hotplug.c 
b/drivers/gpu/drm/i915/intel_hotplug.c
index b177857..4f2a179 100644
--- a/drivers/gpu/drm/i915/intel_hotplug.c
+++ b/drivers/gpu/drm/i915/intel_hotplug.c
@@ -484,6 +484,34 @@ void intel_hpd_init(struct drm_i915_private *dev_priv)
spin_unlock_irq(_priv->irq_lock);
 }
 
+/**
+ * intel_hpd_uninit - disable and deinitialize hpd support
+ * @dev_priv: i915 device instance
+ *
+ * This function disables any pending delayed work for HPD and
+ * disables all hpd pins. Calling this during suspend/reset
+ * avoids conflicts with the reenable worker of interrupt handler
+ * firing.
+ * At resume we call intel_hpd_init() to reenable things.
+ */
+void intel_hpd_uninit(struct drm_i915_private *dev_priv)
+{
+   struct drm_device *dev = dev_priv->dev;
+   int i;
+
+   cancel_delayed_work_sync(_priv->hotplug.reenable_work);
+
+   spin_lock_irq(_priv->irq_lock);
+
+   for_each_hpd_pin(i) {
+   dev_priv->hotplug.stats[i].state = HPD_DISABLED;
+   }
+   if (dev_priv->display.hpd_irq_setup)
+   dev_priv->display.hpd_irq_setup(dev);
+
+   spin_unlock_irq(_priv->irq_lock);
+}
+
 void intel_hpd_init_work(struct drm_i915_private *dev_priv)
 {
INIT_WORK(_priv->hotplug.hotplug_work, i915_hotplug_work_func);
-- 
1.8.4.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/2] drm: Add a non-locking version of drm_kms_helper_poll_enable(), v2

[Egbert Eich]

Jani Nikula writes:
 > 
 > Shouldn't this be _unlocked?
 > 
 > I thought the convention was that functions that do not acquire locks
 > are called _unlocked (although they may require a lock to be held when
 > called). And you might have foo() that grabs locks around a call to
 > foo_unlocked().
 > 

Looking into this, functions that are to be called in a context where
the lock is already held should receive the suffix _locked while
those which do locking themselves and thus need to be called from
a context that doesn't hold this lock already receive the suffix 
_unlocked: the past tense refers to what has happened before.

Cheers,
Egbert.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 2/2] drm/i915: Call non-locking version of drm_kms_helper_poll_enable(), v2

[Egbert Eich]

drm_kms_helper_poll_enable() is called from a context in
intel_hpd_irq_storm_disable() where the the mode_config mutex is
already locked.
When this function was converted to lock this mutex in
commit 8c4ccc4ab6f6 ("drm/probe-helper: Grab mode_config.mutex
in poll_init/enable") a deadlock occurred.
Call the newly implemented non-locking version of this function.

Changes since v1:
- use function name suffix '_locked' for the function that
  is to be called from a locked context.

Signed-off-by: Egbert Eich <e...@suse.de>
Reviewed-by: Daniel Vetter <daniel.vet...@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_hotplug.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_hotplug.c 
b/drivers/gpu/drm/i915/intel_hotplug.c
index 53c0173..b177857 100644
--- a/drivers/gpu/drm/i915/intel_hotplug.c
+++ b/drivers/gpu/drm/i915/intel_hotplug.c
@@ -180,7 +180,7 @@ static void intel_hpd_irq_storm_disable(struct 
drm_i915_private *dev_priv)
 
/* Enable polling and queue hotplug re-enabling. */
if (hpd_disabled) {
-   drm_kms_helper_poll_enable(dev);
+   drm_kms_helper_poll_enable_locked(dev);
mod_delayed_work(system_wq, _priv->hotplug.reenable_work,
 msecs_to_jiffies(HPD_STORM_REENABLE_DELAY));
}
-- 
1.8.4.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 1/2] drm: Add a non-locking version of drm_kms_helper_poll_enable(), v2

[Egbert Eich]

drm_kms_helper_poll_enable() was converted to lock the mode_config
mutex in commit 8c4ccc4ab6f64e859d4ff8d7c02c2ed2e956e07f
("drm/probe-helper: Grab mode_config.mutex in poll_init/enable").

This disregarded the cases where this function is called from a context
where this mutex is already locked.

Add a non-locking version as well.

Changes since v1:
- use function name suffix '_locked' for the function that
  is to be called from a locked context.

Signed-off-by: Egbert Eich <e...@suse.de>
---
 drivers/gpu/drm/drm_probe_helper.c | 19 ---
 include/drm/drm_crtc_helper.h  |  1 +
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_probe_helper.c 
b/drivers/gpu/drm/drm_probe_helper.c
index d734780..2b9ce37 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -93,8 +93,19 @@ static int drm_helper_probe_add_cmdline_mode(struct 
drm_connector *connector)
return 1;
 }
 
+/**
+ * drm_kms_helper_poll_enable_locked - re-enable output polling.
+ * @dev: drm_device
+ *
+ * This function re-enables the output polling work without
+ * locking the mode_config mutex.
+ *
+ * This is like drm_kms_helper_poll_enable() however it is to be
+ * called from a context where the mode_config mutex is locked
+ * already.
+ */
 #define DRM_OUTPUT_POLL_PERIOD (10*HZ)
-static void __drm_kms_helper_poll_enable(struct drm_device *dev)
+void drm_kms_helper_poll_enable_locked(struct drm_device *dev)
 {
bool poll = false;
struct drm_connector *connector;
@@ -113,6 +124,8 @@ static void __drm_kms_helper_poll_enable(struct drm_device 
*dev)
if (poll)
schedule_delayed_work(>mode_config.output_poll_work, 
DRM_OUTPUT_POLL_PERIOD);
 }
+EXPORT_SYMBOL(drm_kms_helper_poll_enable_locked);
+
 
 static int drm_helper_probe_single_connector_modes_merge_bits(struct 
drm_connector *connector,
  uint32_t maxX, 
uint32_t maxY, bool merge_type_bits)
@@ -174,7 +187,7 @@ static int 
drm_helper_probe_single_connector_modes_merge_bits(struct drm_connect
 
/* Re-enable polling in case the global poll config changed. */
if (drm_kms_helper_poll != dev->mode_config.poll_running)
-   __drm_kms_helper_poll_enable(dev);
+   drm_kms_helper_poll_enable_locked(dev);
 
dev->mode_config.poll_running = drm_kms_helper_poll;
 
@@ -428,7 +441,7 @@ EXPORT_SYMBOL(drm_kms_helper_poll_disable);
 void drm_kms_helper_poll_enable(struct drm_device *dev)
 {
mutex_lock(>mode_config.mutex);
-   __drm_kms_helper_poll_enable(dev);
+   drm_kms_helper_poll_enable_locked(dev);
mutex_unlock(>mode_config.mutex);
 }
 EXPORT_SYMBOL(drm_kms_helper_poll_enable);
diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h
index 2a747a9..3febb4b 100644
--- a/include/drm/drm_crtc_helper.h
+++ b/include/drm/drm_crtc_helper.h
@@ -240,5 +240,6 @@ extern void drm_kms_helper_hotplug_event(struct drm_device 
*dev);
 
 extern void drm_kms_helper_poll_disable(struct drm_device *dev);
 extern void drm_kms_helper_poll_enable(struct drm_device *dev);
+extern void drm_kms_helper_poll_enable_locked(struct drm_device *dev);
 
 #endif
-- 
1.8.4.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915: Avoid race of intel_crt_detect_hotplug() with HPD interrupt, v2

[Egbert Eich]

An HPD interrupt may fire while we are in a function that changes
the PORT_HOTPLUG_EN register - especially when an HPD interrupt
storm occurs.
Since the interrupt handler changes the enabled HPD lines when it
detects such a storm the read-modify-write cycles may interfere.
To avoid this, shiled the rmw cycles with IRQ save spinlocks.

Changes since v1:
- Implement a function which takes care of accessing PORT_HOTPLUG_EN.

Signed-off-by: Egbert Eich <e...@suse.de>
---
 drivers/gpu/drm/i915/i915_drv.h  |  3 ++
 drivers/gpu/drm/i915/i915_irq.c  | 64 
 drivers/gpu/drm/i915/intel_crt.c | 11 ---
 3 files changed, 59 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index bf33d6e..a6b7576 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2737,6 +2737,9 @@ i915_disable_pipestat(struct drm_i915_private *dev_priv, 
enum pipe pipe,
 
 void valleyview_enable_display_irqs(struct drm_i915_private *dev_priv);
 void valleyview_disable_display_irqs(struct drm_i915_private *dev_priv);
+void i915_hotplug_interrupt_update(struct drm_i915_private *dev_priv,
+  uint32_t mask,
+  uint32_t bits);
 void
 ironlake_enable_display_irq(struct drm_i915_private *dev_priv, u32 mask);
 void
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index a8aa797..ff85eae 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -167,6 +167,44 @@ static const u32 hpd_bxt[HPD_NUM_PINS] = {
 
 static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 
pm_iir);
 
+/* For display hotplug interrupt */
+static inline void
+i915_hotplug_interrupt_update_locked(struct drm_i915_private *dev_priv,
+uint32_t mask,
+uint32_t bits)
+{
+   uint32_t val;
+
+   assert_spin_locked(_priv->irq_lock);
+   WARN_ON(bits & ~mask);
+
+   val = I915_READ(PORT_HOTPLUG_EN);
+   val &= ~mask;
+   val |= bits;
+   I915_WRITE(PORT_HOTPLUG_EN, val);
+}
+
+/**
+ * i915_hotplug_interrupt_update - update hotplug interrupt enable
+ * @dev_priv: driver private
+ * @mask: bits to update
+ * @bits: bits to enable
+ * NOTE: the HPD enable bits are modified both inside and outside
+ * of an interrupt context. To avoid that read-modify-write cycles
+ * interfer, these bits are protected by a spinlock. Since this
+ * function is usually not called from a context where the lock is
+ * held already, this function acquires the lock itself. A non-locking
+ * version is also available.
+ */
+void i915_hotplug_interrupt_update(struct drm_i915_private *dev_priv,
+  uint32_t mask,
+  uint32_t bits)
+{
+   spin_lock_irq(_priv->irq_lock);
+   i915_hotplug_interrupt_update_locked(dev_priv, mask, bits);
+   spin_unlock_irq(_priv->irq_lock);
+}
+
 /**
  * ilk_update_display_irq - update DEIMR
  * @dev_priv: driver private
@@ -3074,7 +3112,7 @@ static void vlv_display_irq_reset(struct drm_i915_private 
*dev_priv)
 {
enum pipe pipe;
 
-   I915_WRITE(PORT_HOTPLUG_EN, 0);
+   i915_hotplug_interrupt_update(dev_priv, 0x, 0);
I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
 
for_each_pipe(dev_priv, pipe)
@@ -3490,7 +3528,7 @@ static void vlv_display_irq_postinstall(struct 
drm_i915_private *dev_priv)
 {
dev_priv->irq_mask = ~0;
 
-   I915_WRITE(PORT_HOTPLUG_EN, 0);
+   i915_hotplug_interrupt_update(dev_priv, 0x, 0);
POSTING_READ(PORT_HOTPLUG_EN);
 
I915_WRITE(VLV_IIR, 0x);
@@ -3864,7 +3902,7 @@ static void i915_irq_preinstall(struct drm_device * dev)
int pipe;
 
if (I915_HAS_HOTPLUG(dev)) {
-   I915_WRITE(PORT_HOTPLUG_EN, 0);
+   i915_hotplug_interrupt_update(dev_priv, 0x, 0);
I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
}
 
@@ -3898,7 +3936,7 @@ static int i915_irq_postinstall(struct drm_device *dev)
I915_USER_INTERRUPT;
 
if (I915_HAS_HOTPLUG(dev)) {
-   I915_WRITE(PORT_HOTPLUG_EN, 0);
+   i915_hotplug_interrupt_update(dev_priv, 0x, 0);
POSTING_READ(PORT_HOTPLUG_EN);
 
/* Enable in IER... */
@@ -4060,7 +4098,7 @@ static void i915_irq_uninstall(struct drm_device * dev)
int pipe;
 
if (I915_HAS_HOTPLUG(dev)) {
-   I915_WRITE(PORT_HOTPLUG_EN, 0);
+   i915_hotplug_interrupt_update(dev_priv, 0x, 0);
I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
}
 
@@ -4081,7 +4119,7 @@ static void i965_irq_preinstall(struct drm_device * dev)
struct drm_i915_private *dev_priv = dev->

Re: [Intel-gfx] [PATCH] drm/i915: Avoid race of intel_crt_detect_hotplug() with HPD interrupt, v2

[Egbert Eich]

Daniel Vetter writes:
 > On Wed, Sep 23, 2015 at 04:15:27PM +0200, Egbert Eich wrote:
 > > An HPD interrupt may fire while we are in a function that changes
 > > the PORT_HOTPLUG_EN register - especially when an HPD interrupt
 > > storm occurs.
 > > Since the interrupt handler changes the enabled HPD lines when it
 > > detects such a storm the read-modify-write cycles may interfere.
 > > To avoid this, shiled the rmw cycles with IRQ save spinlocks.
 > > 
 > > Changes since v1:
 > > - Implement a function which takes care of accessing PORT_HOTPLUG_EN.
 > > 
 > > Signed-off-by: Egbert Eich <e...@suse.de>
 > 
 > Looks pretty. Queued for -next, thanks for the patch (assuming that we
 > don't need this for -fixes since there's no bug report linked). Please
 > correct me so I can drop this and let Jani pick it up instead.

I didn't bother to file a bug report. I know only one machine that's
affected. 
However the problem this fixes seems to be what caused spurious warnings
which we tried to get rid of with
WARN_ONCE( >>>> INTEL_INFO(dev)->gen >= 5 && !IS_VALLEYVIEW(dev) <<<< ,
  "Received HPD interrupt on pin %d although 
disabled\n", i);

as I did not see these warnings on my gen3 when I removed these tests.

BTW: Using 
 i915_hotplug_interrupt_update(dev_priv, 0x, 0) 
in the *_irq_pre/post/uninstall() functions does not help us much 
in terms of avoiding races. 
It can still happen that an interrupts or reenable worker gets fired 
and resets these values after the spinlock is released in
i915_hotplug_interrupt_update().

IHMO one must 
a. cancel the delayed worker, 
b. disable all interrupt pins and
c. call hpd_irq_setup() 
before calling intel_runtime_pm_disable_interrupts() to avoid this race.

Cheers,
Egbert.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 4/4] drm/i915: Avoid race of intel_crt_detect_hotplug() with HPD interrupt

[Egbert Eich]

Daniel Vetter writes:
 > On Wed, Sep 02, 2015 at 04:19:00PM +0200, Egbert Eich wrote:
 > 
 > Hm I missed that this same register is also accessed by the irq handler
 > code, and it's not just that touching these bits can cause interrupts. So
 > yeah we need your patch, but it needs to be clearer in the commit message
 > that there's also trouble with concurrent register access to
 > PORT_HOTPLUG_EN.
 > 
 > Also I think a commen in the code why we grab that spinlock would be good.
 > For that extracting a small helper to manipulate the register (like we do
 > with other irq mask registers with functions like ilk_update_gt_irq) would
 > be good - then we have just one place to put that commment.

OK, I will come up with a suggestion.

Cheers,
Egbert.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 4/4] drm/i915: Avoid race of intel_crt_detect_hotplug() with HPD interrupt

[Egbert Eich]

Jani Nikula writes:
 > On Wed, 02 Sep 2015, Egbert Eich <e...@suse.com> wrote:
 > > This is exactly the scenatio I'm getting here. I get HPD interrupts at an 
 > > order of 10^4 / sec.
 > 
 > Makes you wonder if either you have faulty hardware or we are
 > configuring the hardware wrong (we overlook some configuration about
 > some voltage/duration threshold maybe, or get irqs from a line that's
 > floating, or something).

It is faulty hardware. But it is not a single machine that broke.
It is an entire series. IMHO due to bad signal routing and poor shilding
there is crosstalk on the SDVO lines signaling the plug status.
Since SDVO uses PCIe lines it is AC coupled, if I recall correctly
from reading the specs long time ago, one status is signalled by a
10MHz signal, the other by 20MHz.

At the time when I implemented this I've seen other reports from systems 
which showed similar problems under certain conditions(*) - although not 
quite as bad, therefore I thought of a general solution to get rid of 
this once and for all. If this had only been one system with this problem, 
I would just have blacklisted it.

(*) It seems that this somewhat depends on the video mode set (supports
the crosstalk theory) but I also had a report where this occurred at
certain charging levels and whether a power supply was connected or
not.

Cheers,
Egbert.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 4/4] drm/i915: Avoid race of intel_crt_detect_hotplug() with HPD interrupt

[Egbert Eich]

Daniel Vetter writes:
 > On Tue, Sep 01, 2015 at 10:21:35PM +0200, Egbert Eich wrote:
 > > A HPD interrupt may fire during intel_crt_detect_hotplug() - especially
 > > when HPD interrupt storms occur.
 > > Since the interrupt handler changes the enabled interrupt lines when it
 > > detects a storm this races with intel_crt_detect_hotplug().
 > > To avoid this, shiled the rmw cycles with IRQ save spinlocks.
 > > 
 > > Signed-off-by: Egbert Eich <e...@suse.de>
 > 
 > I think this only reduces one source of such races, but fundamentally we
 > can't avoid them. E.g. if you're _very_ unlucky you might cause a real hpd
 > right when the strom code frobs around. Plugging the race with this known

This is exactly the scenatio I'm getting here. I get HPD interrupts at an 
order of 10^4 / sec.

 > interrupt source here doesn't fix the fundamental issue.
 > 
 > Also I think the actual interrupt deliver is fairly asynchronous, both in
 > the hardware and in the sw handling. E.g. spin_lock_irq does not
 > synchronize with the interrupt handler on SMP systems, it only guarantees
 > that it's not running concurrently on the same cpu (which would deadlock).
 > 
 > I think fundamentally this race is unfixable.


There is one important race we avoid with this patch: It is that
we change the PORT_HOTPLUG_EN concurrently in the interrupt handler
(thru xxx_hpd_irq_setup() and in intel_crt_detect_hotplug()).

With the test system that I have here the old version of the code
easily runs into this as the time spent inside intel_crt_detect_hotplug() 
is quite long - especially when no CRT is connected.

What happens intel_crt_detect_hotplug() reads the value of PORT_HOTPLUG_EN
at entry, then frobs around for ages, during this time several HPD interrupts
occur, the storm is detected, the bit related to the stormy line is unset
then after ages intel_crt_detect_hotplug() decides to give up and restores
the value it read on entry.

To remedy this this patch reads the value of PORT_HOTPLUG_EN whenever 
it is going to change it and adds the spin locks to make sure the 
read-modify-write cycles don't happen concurrently.

PORT_HOTPLUG_EN is only touched in two places: in xxx_hpd_irq_setup()
and in intel_crt_detect_hotplug(), the former can be called from an
interrupt handler.

Not sure why you see a problem here.

Cheers,
Egbert.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 1/4] drm: Add a non-locking version of drm_kms_helper_poll_enable().

[Egbert Eich]

drm_kms_helper_poll_enable() was converted to lock the mode_config
mutex in commit 8c4ccc4ab6f64e859d4ff8d7c02c2ed2e956e07f
("drm/probe-helper: Grab mode_config.mutex in poll_init/enable").

This disregarded the cases where this function is called from a context
where this mutex is already locked.

Add a non-locking version as well.

Signed-off-by: Egbert Eich <e...@suse.de>
---
 drivers/gpu/drm/drm_probe_helper.c | 19 ---
 include/drm/drm_crtc_helper.h  |  1 +
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_probe_helper.c 
b/drivers/gpu/drm/drm_probe_helper.c
index d734780..a93ad1b 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -93,8 +93,19 @@ static int drm_helper_probe_add_cmdline_mode(struct 
drm_connector *connector)
return 1;
 }
 
+/**
+ * drm_kms_helper_poll_enable_no_lock - re-enable output polling.
+ * @dev: drm_device
+ *
+ * This function re-enables the output polling work without
+ * locking the mode_config mutex.
+ *
+ * This is like drm_kms_helper_poll_enable() however it is to be
+ * called from a context where the mode_config mutex is locked
+ * already.
+ */
 #define DRM_OUTPUT_POLL_PERIOD (10*HZ)
-static void __drm_kms_helper_poll_enable(struct drm_device *dev)
+void drm_kms_helper_poll_enable_no_lock(struct drm_device *dev)
 {
bool poll = false;
struct drm_connector *connector;
@@ -113,6 +124,8 @@ static void __drm_kms_helper_poll_enable(struct drm_device 
*dev)
if (poll)
schedule_delayed_work(>mode_config.output_poll_work, 
DRM_OUTPUT_POLL_PERIOD);
 }
+EXPORT_SYMBOL(drm_kms_helper_poll_enable_no_lock);
+
 
 static int drm_helper_probe_single_connector_modes_merge_bits(struct 
drm_connector *connector,
  uint32_t maxX, 
uint32_t maxY, bool merge_type_bits)
@@ -174,7 +187,7 @@ static int 
drm_helper_probe_single_connector_modes_merge_bits(struct drm_connect
 
/* Re-enable polling in case the global poll config changed. */
if (drm_kms_helper_poll != dev->mode_config.poll_running)
-   __drm_kms_helper_poll_enable(dev);
+   drm_kms_helper_poll_enable_no_lock(dev);
 
dev->mode_config.poll_running = drm_kms_helper_poll;
 
@@ -428,7 +441,7 @@ EXPORT_SYMBOL(drm_kms_helper_poll_disable);
 void drm_kms_helper_poll_enable(struct drm_device *dev)
 {
mutex_lock(>mode_config.mutex);
-   __drm_kms_helper_poll_enable(dev);
+   drm_kms_helper_poll_enable_no_lock(dev);
mutex_unlock(>mode_config.mutex);
 }
 EXPORT_SYMBOL(drm_kms_helper_poll_enable);
diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h
index 2a747a9..f96703d 100644
--- a/include/drm/drm_crtc_helper.h
+++ b/include/drm/drm_crtc_helper.h
@@ -240,5 +240,6 @@ extern void drm_kms_helper_hotplug_event(struct drm_device 
*dev);
 
 extern void drm_kms_helper_poll_disable(struct drm_device *dev);
 extern void drm_kms_helper_poll_enable(struct drm_device *dev);
+extern void drm_kms_helper_poll_enable_no_lock(struct drm_device *dev);
 
 #endif
-- 
1.8.4.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 0/4] Fix numerous issues with HPDstorm handling

[Egbert Eich]

The following 4 patches fix issues with the HPD storm detection.
Two of them have been introduced quite recently, one has been
around since this code was implemented.
Work arounds like:
commit 3ff04a160a891e56cdcee5c198d4c764d1c8c78b
("drm/i915: Don't WARN nor handle unexpected hpd interrupts on gmch platforms")
and
commit 3432087ef846d760427eceff0ff4e7d0a2565b8a
("drm/i915: Only WARN about a stuck hotplug irq ONCE")
may actually become obsolete now.

Egbert Eich (4):
  drm: Add a non-locking version of drm_kms_helper_poll_enable().
  drm/i915: Call non-locking version of drm_kms_helper_poll_enable()
  drm/i915: Use the correct hpd_status list for non-G4xx/VLV
  drm/i915: Avoid race of intel_crt_detect_hotplug() with HPD interrupt

 drivers/gpu/drm/drm_probe_helper.c   | 19 ---
 drivers/gpu/drm/i915/i915_irq.c  |  2 +-
 drivers/gpu/drm/i915/intel_crt.c | 17 -
 drivers/gpu/drm/i915/intel_hotplug.c |  2 +-
 include/drm/drm_crtc_helper.h|  1 +
 5 files changed, 31 insertions(+), 10 deletions(-)

-- 
1.8.4.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 3/4] drm/i915: Use the correct hpd_status list for non-G4xx/VLV

[Egbert Eich]

This copy-and-past error was introduced in:

commit fd63e2a972c670887e5e8a08440111d3812c0996
Author: Imre Deak <imre.d...@intel.com>
Date:   Tue Jul 21 15:32:44 2015 -0700

drm/i915: combine i9xx_get_hpd_pins and pch_get_hpd_pins

Signed-off-by: Egbert Eich <e...@suse.de>
---
 drivers/gpu/drm/i915/i915_irq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 8485bea..ad99dae 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1559,7 +1559,7 @@ static void i9xx_hpd_irq_handler(struct drm_device *dev)
u32 hotplug_trigger = hotplug_status & HOTPLUG_INT_STATUS_I915;
 
intel_get_hpd_pins(_mask, _mask, hotplug_trigger,
-  hotplug_trigger, hpd_status_g4x,
+  hotplug_trigger, hpd_status_i915,
   i9xx_port_hotplug_long_detect);
intel_hpd_irq_handler(dev, pin_mask, long_mask);
}
-- 
1.8.4.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 2/4] drm/i915: Call non-locking version of drm_kms_helper_poll_enable()

[Egbert Eich]

drm_kms_helper_poll_enable() is called from a context in
intel_hpd_irq_storm_disable() where the the mode_config mutex is
already locked.
When this function was converted to lock this mutex in:

commit 8c4ccc4ab6f64e859d4ff8d7c02c2ed2e956e07f
Author: Daniel Vetter <daniel.vet...@ffwll.ch>
Date:   Thu Jul 9 23:44:26 2015 +0200

drm/probe-helper: Grab mode_config.mutex in poll_init/enable

a deadlock occurred.
Call the newly implemented non-locking version of this function.

Signed-off-by: Egbert Eich <e...@suse.de>
---
 drivers/gpu/drm/i915/intel_hotplug.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_hotplug.c 
b/drivers/gpu/drm/i915/intel_hotplug.c
index 53c0173..77dd5b6 100644
--- a/drivers/gpu/drm/i915/intel_hotplug.c
+++ b/drivers/gpu/drm/i915/intel_hotplug.c
@@ -180,7 +180,7 @@ static void intel_hpd_irq_storm_disable(struct 
drm_i915_private *dev_priv)
 
/* Enable polling and queue hotplug re-enabling. */
if (hpd_disabled) {
-   drm_kms_helper_poll_enable(dev);
+   drm_kms_helper_poll_enable_no_lock(dev);
mod_delayed_work(system_wq, _priv->hotplug.reenable_work,
 msecs_to_jiffies(HPD_STORM_REENABLE_DELAY));
}
-- 
1.8.4.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 4/4] drm/i915: Avoid race of intel_crt_detect_hotplug() with HPD interrupt

[Egbert Eich]

A HPD interrupt may fire during intel_crt_detect_hotplug() - especially
when HPD interrupt storms occur.
Since the interrupt handler changes the enabled interrupt lines when it
detects a storm this races with intel_crt_detect_hotplug().
To avoid this, shiled the rmw cycles with IRQ save spinlocks.

Signed-off-by: Egbert Eich <e...@suse.de>
---
 drivers/gpu/drm/i915/intel_crt.c | 17 -
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index af5e43b..685f3de 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -376,9 +376,10 @@ static bool intel_crt_detect_hotplug(struct drm_connector 
*connector)
 {
struct drm_device *dev = connector->dev;
struct drm_i915_private *dev_priv = dev->dev_private;
-   u32 hotplug_en, orig, stat;
+   u32 hotplug_en, stat;
bool ret = false;
int i, tries = 0;
+   unsigned long irqflags;
 
if (HAS_PCH_SPLIT(dev))
return intel_ironlake_crt_detect_hotplug(connector);
@@ -395,12 +396,14 @@ static bool intel_crt_detect_hotplug(struct drm_connector 
*connector)
tries = 2;
else
tries = 1;
-   hotplug_en = orig = I915_READ(PORT_HOTPLUG_EN);
-   hotplug_en |= CRT_HOTPLUG_FORCE_DETECT;
 
for (i = 0; i < tries ; i++) {
/* turn on the FORCE_DETECT */
-   I915_WRITE(PORT_HOTPLUG_EN, hotplug_en);
+   spin_lock_irqsave(_priv->irq_lock, irqflags);
+   hotplug_en = I915_READ(PORT_HOTPLUG_EN);
+   I915_WRITE(PORT_HOTPLUG_EN, hotplug_en |
+  CRT_HOTPLUG_FORCE_DETECT);
+   spin_unlock_irqrestore(_priv->irq_lock, irqflags);
/* wait for FORCE_DETECT to go off */
if (wait_for((I915_READ(PORT_HOTPLUG_EN) &
  CRT_HOTPLUG_FORCE_DETECT) == 0,
@@ -416,7 +419,11 @@ static bool intel_crt_detect_hotplug(struct drm_connector 
*connector)
I915_WRITE(PORT_HOTPLUG_STAT, CRT_HOTPLUG_INT_STATUS);
 
/* and put the bits back */
-   I915_WRITE(PORT_HOTPLUG_EN, orig);
+   spin_lock_irqsave(_priv->irq_lock, irqflags);
+   hotplug_en = I915_READ(PORT_HOTPLUG_EN);
+   hotplug_en &= ~CRT_HOTPLUG_FORCE_DETECT;
+   I915_WRITE(PORT_HOTPLUG_EN, hotplug_en);
+   spin_unlock_irqrestore(_priv->irq_lock, irqflags);
 
return ret;
 }
-- 
1.8.4.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/4] drm: Add a non-locking version of drm_kms_helper_poll_enable().

[Egbert Eich]

Lukas Wunner writes:
 > 
 > It seems DRM convention is to append _locked or _unlocked, e.g.:
 > drm_fb_helper_restore_fbdev_mode_unlocked
 > drm_gem_object_unreference_unlocked
 > 

Oh, I missed that. 
Did you check what these functions actually do - and compare it to
what I try to achieve?

Egbert.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/4] drm: Add a non-locking version of drm_kms_helper_poll_enable().

[Egbert Eich]

Lukas Wunner writes:
 > Hi Egbert,
 > 
 > On Tue, Sep 01, 2015 at 10:21:32PM +0200, Egbert Eich wrote:
 > > drm_kms_helper_poll_enable() was converted to lock the mode_config
 > > mutex in commit 8c4ccc4ab6f64e859d4ff8d7c02c2ed2e956e07f
 > > ("drm/probe-helper: Grab mode_config.mutex in poll_init/enable").
 > > 
 > > This disregarded the cases where this function is called from a context
 > > where this mutex is already locked.
 > 
 > Which ones would that be?

Have you missed the next patch in the series?

Egbert.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/4] drm: Add a non-locking version of drm_kms_helper_poll_enable().

[Egbert Eich]

Lukas Wunner writes:
 > Hi Egbert,
 > 
 > On Wed, Sep 02, 2015 at 12:10:19AM +0200, Egbert Eich wrote:
 > > Lukas Wunner writes:
 > >  > On Tue, Sep 01, 2015 at 10:21:32PM +0200, Egbert Eich wrote:
 > >  > > drm_kms_helper_poll_enable() was converted to lock the mode_config
 > >  > > mutex in commit 8c4ccc4ab6f64e859d4ff8d7c02c2ed2e956e07f
 > >  > > ("drm/probe-helper: Grab mode_config.mutex in poll_init/enable").
 > >  > > 
 > >  > > This disregarded the cases where this function is called from a 
 > > context
 > >  > > where this mutex is already locked.
 > >  > 
 > >  > Which ones would that be?
 > > 
 > > Have you missed the next patch in the series?
 > 
 > Sorry, I should have asked more precisely: Is this the only one?
 > If so it may make sense to modify i915_hotplug_work_func or
 > intel_hpd_irq_storm_disable instead.
 > 

The non-locking version existed before. I've just exported it so it cah
be used by drivers.
Moreover this is a helper function - why should it be restricted to 
callers which don't have the mode_config mutex grabbed?
I could be talked into changing the log message.

Egbert.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915/eDP: When enabling panel VDD cancel pending disable worker

[Egbert Eich]

Daniel Vetter writes:
  On Mon, Nov 24, 2014 at 5:56 PM, Egbert Eich e...@suse.de wrote:
   Before testing if the panel VDD is enabled on eDP cancel any pending
   disable worker. This makes sure the worker doesn't fire when we expect
   VDD to be enabled.
  
   https://bugs.freedesktop.org/show_bug.cgi?id=86201
  
   Signed-off-by: Egbert Eich e...@suse.de
  
  This shouldn't be needed at all:
  - The vdd off rechecks -want_panel_vdd under the pps lock.
  - The off function sets that and also reschedules the work (to make
  sure it doesn't kill vdd to early) again all under the same lock.
 
No. edp_panel_vdd_off() calls edp_panel_vdd_schedule_off() when not called
with sync == true. edp_panel_vdd_schedule_off() calls schedule_delayed_work()
which doesn't reschedule pending work.

  
  So no one can sneak in and the work racing with us isn't an issue. Or
  shouldn't be at least. So if this helps we need to dig a bit deeper.

Daniel, I came across this when I was looking for the problem in fdo#86201.
And I agree, it is not strictly needed, however if you follow
fdo#86201 you will see a list of calls to edp_panel_vdd_off_sync()
soon to be followed by calls to edp_panel_vdd_on(). Many of them are 
unnecessary and can be gotten rid of the uneeded ones by canelling the work 
queue. (When you follow fdo#86201 you will see why there was an abnormal
situation and what caused it). 
Of course due to the locking we already have serialization and canelling 
pending workers did not resolve the issue but it at least got rid of some
unneeded overhead.

Cheers,
Egbert.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v2] drm/i915/eDP: When enabling panel VDD cancel pending disable worker

[Egbert Eich]

Before testing if the panel VDD is enabled on eDP cancel any pending
disable worker. This makes sure the worker will be triggered with a
delay from the last time edp_panel_vdd_schedule_off() is called, not
the first time. This avoids unnecessary overhead.

https://bugs.freedesktop.org/show_bug.cgi?id=86201

v2: use cancel_delayed_work() instead of cancel_delayed_work_sync()
as the pps_mutexes will provide the required serialization with
edp_panel_vdd_work() while the sync variant may deadlock. Suggested
by Ville Syrjälä ville.syrj...@linux.intel.com.
Made commit message a bit clearer.

Signed-off-by: Egbert Eich e...@suse.de
---
 drivers/gpu/drm/i915/intel_dp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 70bb8d0b..7ab39d4 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1503,6 +1503,7 @@ static bool edp_panel_vdd_on(struct intel_dp *intel_dp)
if (!is_edp(intel_dp))
return false;
 
+   cancel_delayed_work(intel_dp-panel_vdd_work);
intel_dp-want_panel_vdd = true;
 
if (edp_have_panel_vdd(intel_dp))
-- 
1.8.4.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 0/5] drm/i915 Avoid long delays when reading EDID on eDP

[Egbert Eich]

Daniel Vetter writes:

  Imo this approach with overwrite all the entry points won't scale since
  besides i2c and dpcd there will be more sooner or later (oui, dp mst, some
  debugfs userspace dp aux tools, ...).
  
  I think what we need is the same as in the i2c layer has with the
  xfer_pre/post functions. To make this as painless as possible we should
  probably refcount that in the dp helper, protected by aux-hw_mutex. That
  way normal dp reads could just do the xfer_pre/post around the call to
  aux-transfer while i2c could do it around the entire i2c transaction.

I agree with your idea of having xfer_pre/post functions. 
I'm not sure though if I understand your idea about reference counters:
are you trying to protect from xfer_pre/post being called at different
levels here? 
With my proposal entire transactions (I2C) are serialized thru the pps_mutex 
lock. This is a side effect that's probably unwanted. Is this what you are
trying to avoid by using ref counters?

Cheers,
Egbert.

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915/eDP: When enabling panel VDD cancel pending disable worker

[Egbert Eich]

Before testing if the panel VDD is enabled on eDP cancel any pending
disable worker. This makes sure the worker doesn't fire when we expect
VDD to be enabled.

https://bugs.freedesktop.org/show_bug.cgi?id=86201

Signed-off-by: Egbert Eich e...@suse.de
---
 drivers/gpu/drm/i915/intel_dp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 70bb8d0b..81f959d 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1503,6 +1503,7 @@ static bool edp_panel_vdd_on(struct intel_dp *intel_dp)
if (!is_edp(intel_dp))
return false;
 
+   cancel_delayed_work_sync(intel_dp-panel_vdd_work);
intel_dp-want_panel_vdd = true;
 
if (edp_have_panel_vdd(intel_dp))
-- 
1.8.4.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 3/5] drm/DP: Export drm_dp_i2c_xfer() DP helper function

[Egbert Eich]

It may be required to wrap the generic DP I2C transfer function to
perfrom certain operations before of after this function is called.
Make this function available to the driver.

Signed-off-by: Egbert Eich e...@suse.de
---
 drivers/gpu/drm/drm_dp_helper.c | 3 ++-
 include/drm/drm_dp_helper.h | 2 ++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index d132838..f17ac01 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -481,7 +481,7 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct 
drm_dp_aux_msg *msg)
return -EREMOTEIO;
 }
 
-static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
+int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
   int num)
 {
struct drm_dp_aux *aux = adapter-algo_data;
@@ -537,6 +537,7 @@ static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, 
struct i2c_msg *msgs,
 
return err;
 }
+EXPORT_SYMBOL(drm_dp_i2c_xfer);
 
 static const struct i2c_algorithm drm_dp_i2c_algo = {
.functionality = drm_dp_i2c_functionality,
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index cd7b464..0103b7f 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -423,6 +423,8 @@ void drm_dp_link_train_channel_eq_delay(const u8 
dpcd[DP_RECEIVER_CAP_SIZE]);
 
 u8 drm_dp_link_rate_to_bw_code(int link_rate);
 int drm_dp_bw_code_to_link_rate(u8 link_bw);
+int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
+   int num);
 
 struct edp_sdp_header {
u8 HB0; /* Secondary Data Packet ID */
-- 
1.8.4.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 2/5] drm/DP: Create pointer to generic DPCD access function

[Egbert Eich]

This way a driver can replace this function with its own version.

Signed-off-by: Egbert Eich e...@suse.de
---
 drivers/gpu/drm/drm_dp_helper.c | 5 +++--
 include/drm/drm_dp_helper.h | 8 
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 959e207..d132838 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -238,7 +238,7 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 
request,
 ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
 void *buffer, size_t size)
 {
-   return drm_dp_dpcd_access(aux, DP_AUX_NATIVE_READ, offset, buffer,
+   return aux-dpcd_access(aux, DP_AUX_NATIVE_READ, offset, buffer,
  size);
 }
 EXPORT_SYMBOL(drm_dp_dpcd_read);
@@ -260,7 +260,7 @@ EXPORT_SYMBOL(drm_dp_dpcd_read);
 ssize_t drm_dp_dpcd_write(struct drm_dp_aux *aux, unsigned int offset,
  void *buffer, size_t size)
 {
-   return drm_dp_dpcd_access(aux, DP_AUX_NATIVE_WRITE, offset, buffer,
+   return aux-dpcd_access(aux, DP_AUX_NATIVE_WRITE, offset, buffer,
  size);
 }
 EXPORT_SYMBOL(drm_dp_dpcd_write);
@@ -552,6 +552,7 @@ static const struct i2c_algorithm drm_dp_i2c_algo = {
 int drm_dp_aux_register(struct drm_dp_aux *aux)
 {
mutex_init(aux-hw_mutex);
+   aux-dpcd_access = drm_dp_dpcd_access;
 
aux-ddc.algo = drm_dp_i2c_algo;
aux-ddc.algo_data = aux;
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 11f8c84..cd7b464 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -497,6 +497,8 @@ struct drm_dp_aux_msg {
  * @dev: pointer to struct device that is the parent for this AUX channel
  * @hw_mutex: internal mutex used for locking transfers
  * @transfer: transfers a message representing a single AUX transaction
+ * @dp_dpcd_access: hook to optionally wrap or replace high level dpcd message
+ *  function.
  *
  * The .dev field should be set to a pointer to the device that implements
  * the AUX channel.
@@ -519,6 +521,10 @@ struct drm_dp_aux_msg {
  * transactions. The drm_dp_aux_register_i2c_bus() function registers an
  * I2C adapter that can be passed to drm_probe_ddc(). Upon removal, drivers
  * should call drm_dp_aux_unregister_i2c_bus() to remove the I2C adapter.
+ * dpcd_access() can be used by the driver to wrap or replace the generic
+ * dpcd access function in case certain operations should be performed before
+ * a dpcd transaction is started if this cannot reasonably performed in
+ * .transfer(). It is initialized to its default value by 
drm_dp_aux_register().
  *
  * Note that the aux helper code assumes that the .transfer() function
  * only modifies the reply field of the drm_dp_aux_msg structure.  The
@@ -531,6 +537,8 @@ struct drm_dp_aux {
struct mutex hw_mutex;
ssize_t (*transfer)(struct drm_dp_aux *aux,
struct drm_dp_aux_msg *msg);
+   int (*dpcd_access)(struct drm_dp_aux *aux, u8 request,
+unsigned int offset, void *buffer, size_t size);
unsigned i2c_nack_count, i2c_defer_count;
 };
 
-- 
1.8.4.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 1/5] drm/i915: Try to avoid pps_{lock, unlock}() on DP ports

[Egbert Eich]

From: Ville Syrjälä ville.syrj...@linux.intel.com

Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
 drivers/gpu/drm/i915/intel_dp.c | 48 -
 1 file changed, 28 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 81f959d..a24c8cc7 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -807,17 +807,19 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
uint32_t status;
int try, clock = 0;
bool has_aux_irq = HAS_AUX_IRQ(dev);
-   bool vdd;
+   bool vdd = false;
 
-   pps_lock(intel_dp);
+   if (is_edp(intel_dp)) {
+   pps_lock(intel_dp);
 
-   /*
-* We will be called with VDD already enabled for dpcd/edid/oui reads.
-* In such cases we want to leave VDD enabled and it's up to upper 
layers
-* to turn it off. But for eg. i2c-dev access we need to turn it on/off
-* ourselves.
-*/
-   vdd = edp_panel_vdd_on(intel_dp);
+   /*
+* We will be called with VDD already enabled for dpcd/edid/oui 
reads.
+* In such cases we want to leave VDD enabled and it's up to 
upper layers
+* to turn it off. But for eg. i2c-dev access we need to turn 
it on/off
+* ourselves.
+*/
+   vdd = edp_panel_vdd_on(intel_dp);
+   }
 
/* dp aux is extremely sensitive to irq latency, hence request the
 * lowest possible wakeup latency and so prevent the cpu from going into
@@ -924,10 +926,12 @@ out:
pm_qos_update_request(dev_priv-pm_qos, PM_QOS_DEFAULT_VALUE);
intel_aux_display_runtime_put(dev_priv);
 
-   if (vdd)
-   edp_panel_vdd_off(intel_dp, false);
+   if (is_edp(intel_dp)) {
+   if (vdd)
+   edp_panel_vdd_off(intel_dp, false);
 
-   pps_unlock(intel_dp);
+   pps_unlock(intel_dp);
+   }
 
return ret;
 }
@@ -2291,18 +2295,22 @@ static void intel_enable_dp(struct intel_encoder 
*encoder)
if (WARN_ON(dp_reg  DP_PORT_EN))
return;
 
-   pps_lock(intel_dp);
+   if (is_edp(intel_dp)) {
+   pps_lock(intel_dp);
 
-   if (IS_VALLEYVIEW(dev))
-   vlv_init_panel_power_sequencer(intel_dp);
+   if (IS_VALLEYVIEW(dev))
+   vlv_init_panel_power_sequencer(intel_dp);
 
-   intel_dp_enable_port(intel_dp);
+   intel_dp_enable_port(intel_dp);
 
-   edp_panel_vdd_on(intel_dp);
-   edp_panel_on(intel_dp);
-   edp_panel_vdd_off(intel_dp, true);
+   edp_panel_vdd_on(intel_dp);
+   edp_panel_on(intel_dp);
+   edp_panel_vdd_off(intel_dp, true);
 
-   pps_unlock(intel_dp);
+   pps_unlock(intel_dp);
+   } else {
+   intel_dp_enable_port(intel_dp);
+   }
 
if (IS_VALLEYVIEW(dev))
vlv_wait_port_ready(dev_priv, dp_to_dig_port(intel_dp));
-- 
1.8.4.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 5/5] drm/i915/eDP: Move pps_lock() and edp_panel_vdd_on() to top

[Egbert Eich]

On eDP each low level transfer is wrapped by pps_lock()/unlock()
and edp_panel_vdd_on()/off(). Since PPS locking can be quite expensive
and each call to master_xfer() or dpcd_access() may call these low level
transfer functions many times it may introduce a considerable delay.
Move locking/VDD enablement to the top so that it is performed only
once per master_xfer() or dpcd_access().
This fixes https://bugs.freedesktop.org/show_bug.cgi?id=86201

Signed-off-by: Egbert Eich e...@suse.de
---
 drivers/gpu/drm/i915/intel_dp.c  | 116 ---
 drivers/gpu/drm/i915/intel_drv.h |   5 ++
 2 files changed, 100 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index a24c8cc7..164f80e 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -807,19 +807,6 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
uint32_t status;
int try, clock = 0;
bool has_aux_irq = HAS_AUX_IRQ(dev);
-   bool vdd = false;
-
-   if (is_edp(intel_dp)) {
-   pps_lock(intel_dp);
-
-   /*
-* We will be called with VDD already enabled for dpcd/edid/oui 
reads.
-* In such cases we want to leave VDD enabled and it's up to 
upper layers
-* to turn it off. But for eg. i2c-dev access we need to turn 
it on/off
-* ourselves.
-*/
-   vdd = edp_panel_vdd_on(intel_dp);
-   }
 
/* dp aux is extremely sensitive to irq latency, hence request the
 * lowest possible wakeup latency and so prevent the cpu from going into
@@ -926,13 +913,6 @@ out:
pm_qos_update_request(dev_priv-pm_qos, PM_QOS_DEFAULT_VALUE);
intel_aux_display_runtime_put(dev_priv);
 
-   if (is_edp(intel_dp)) {
-   if (vdd)
-   edp_panel_vdd_off(intel_dp, false);
-
-   pps_unlock(intel_dp);
-   }
-
return ret;
 }
 
@@ -1001,6 +981,97 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, struct 
drm_dp_aux_msg *msg)
return ret;
 }
 
+int intel_edp_dpcd_access(struct drm_dp_aux *aux, u8 request,
+ unsigned int offset, void *buffer, size_t size)
+{
+   struct intel_dp *intel_dp = container_of(aux, struct intel_dp, aux);
+   bool vdd = false;
+   int ret;
+
+   pps_lock(intel_dp);
+   /*
+* We will be called with VDD already enabled for dpcd/edid/oui reads.
+* In such cases we want to leave VDD enabled and it's up to upper
+* layers to turn it off. But for eg. i2c-dev access we need to turn
+* it on/off ourselves.
+*/
+   vdd = edp_panel_vdd_on(intel_dp);
+
+   ret = drm_dp_dpcd_access(aux, request, offset, buffer, size);
+
+   if (vdd)
+   edp_panel_vdd_off(intel_dp, false);
+
+   pps_unlock(intel_dp);
+
+   return ret;
+}
+
+static int
+intel_edp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
+ int num)
+{
+   struct drm_dp_aux *aux = adapter-algo_data;
+   struct intel_dp *intel_dp = container_of(aux, struct intel_dp, aux);
+   bool vdd = false;
+   int ret;
+
+   pps_lock(intel_dp);
+   /*
+* We will be called with VDD already enabled for dpcd/edid/oui reads.
+* In such cases we want to leave VDD enabled and it's up to upper
+* layers to turn it off. But for eg. i2c-dev access we need to turn
+* it on/off ourselves.
+*/
+   vdd = edp_panel_vdd_on(intel_dp);
+
+   ret = drm_dp_i2c_xfer(adapter, msgs, num);
+
+   if (vdd)
+   edp_panel_vdd_off(intel_dp, false);
+
+   pps_unlock(intel_dp);
+
+   return ret;
+}
+
+static u32 intel_edp_i2c_functionality(struct i2c_adapter *adapter)
+{
+   return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL |
+  I2C_FUNC_SMBUS_READ_BLOCK_DATA |
+  I2C_FUNC_SMBUS_BLOCK_PROC_CALL |
+  I2C_FUNC_10BIT_ADDR;
+}
+
+static const struct i2c_algorithm intel_edp_i2c_algo = {
+   .functionality = intel_edp_i2c_functionality,
+   .master_xfer = intel_edp_i2c_xfer,
+};
+
+/*
+ * intel_edp_aux_register() - Intel replacement for
+ *   drm_dp_aux_register().
+ */
+static int intel_edp_aux_register(struct drm_dp_aux *aux)
+{
+   mutex_init(aux-hw_mutex);
+   aux-dpcd_access = intel_edp_dpcd_access;
+
+   aux-ddc.algo = intel_edp_i2c_algo;
+   aux-ddc.algo_data = aux;
+   aux-ddc.retries = 3;
+
+   aux-ddc.class = I2C_CLASS_DDC;
+   aux-ddc.owner = THIS_MODULE;
+   aux-ddc.dev.parent = aux-dev;
+   aux-ddc.dev.of_node = aux-dev-of_node;
+
+   strlcpy(aux-ddc.name, aux-name ? aux-name : dev_name(aux-dev),
+   sizeof(aux-ddc.name));
+
+   return i2c_add_adapter(aux-ddc);
+}
+
 static void
 intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector

[Intel-gfx] [PATCH 0/5] drm/i915 Avoid long delays when reading EDID on eDP

[Egbert Eich]

For eDP in the Intel driver pps_lock()/unlock() need to be called before  
initiating an I2C/AUX channel transfer. These operations can be quite 
expensive - especially on values for HZ lower than 1000.  
It is therefore better to perfrom this locking/unlocking only once,   
ie at the beginning and at the end of the entire I2C transfer.
The current design of drm_dp_helper.c doesn't allow this. 
This patchset modifies drm_dp_helper.c and moves the locking/unlocking
operation to the top. 
This fixes the long delay observed in 
   https://bugs.freedesktop.org/show_bug.cgi?id=86201 

Egbert Eich (4):
  drm/DP: Create pointer to generic DPCD access function
  drm/DP: Export drm_dp_i2c_xfer() DP helper function
  drm/DP: Export drm_dp_dpcd_access() DP helper function
  drm/i915/eDP: Move pps_lock() and edp_panel_vdd_on() to top

Ville Syrjälä (1):
  drm/i915: Try to avoid pps_{lock,unlock}() on DP ports

 drivers/gpu/drm/drm_dp_helper.c  |  11 ++--
 drivers/gpu/drm/i915/intel_dp.c  | 132 +++
 drivers/gpu/drm/i915/intel_drv.h |   5 ++
 include/drm/drm_dp_helper.h  |  14 +
 4 files changed, 133 insertions(+), 29 deletions(-)

-- 
1.8.4.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915/eDP: When enabling panel VDD cancel pending disable worker

[Egbert Eich]

Ville Syrjälä writes:
  On Mon, Nov 24, 2014 at 07:32:49PM +0200, Ville Syrjälä wrote:
   On Mon, Nov 24, 2014 at 05:56:20PM +0100, Egbert Eich wrote:
Before testing if the panel VDD is enabled on eDP cancel any pending
disable worker. This makes sure the worker doesn't fire when we expect
VDD to be enabled.

https://bugs.freedesktop.org/show_bug.cgi?id=86201

Signed-off-by: Egbert Eich e...@suse.de
---
 drivers/gpu/drm/i915/intel_dp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c 
b/drivers/gpu/drm/i915/intel_dp.c
index 70bb8d0b..81f959d 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1503,6 +1503,7 @@ static bool edp_panel_vdd_on(struct intel_dp 
*intel_dp)
 if (!is_edp(intel_dp))
 return false;
 
+cancel_delayed_work_sync(intel_dp-panel_vdd_work);
   
   This can deadlock since we're already holding pps_mutex and
   edp_panel_vdd_work() also grabs it.
   
   I'm thinking we may want something like mod_delayed_work() instead.
  
  Or rather just cancel_delayed_work() is enough here I guess. W/o the
  _sync we shouldn't deadlock. And if we always cancel here the timer
  shoduln't be pending in edp_panel_vdd_schedule_off() anymore, so
  mod_delayed_work() there would anyway just be the same as
  schedule_delayed_work().

Right. I was thinking we need to make sure we are not in the midst of
the worker but the pps_mutexes indeed serialize this as well.

Will resend the patch.

Thanks!
Cheers,
Egbert.
  
   
   
 intel_dp-want_panel_vdd = true;
 
 if (edp_have_panel_vdd(intel_dp))
-- 
1.8.4.5
   
   -- 
   Ville Syrjälä
   Intel OTC
  
  -- 
  Ville Syrjälä
  Intel OTC

-- 
Any jackass can report a bug, but it takes a good engineer to fix one.
   Loosely base on 
 Sam Rayburn (48th, 50th and 52nd Speaker of the US House of Rep.)

Egbert Eich (Res.  Dev.)SUSE LINUX Products GmbH
SUSE Labs KMS / DRM / X Window System Development
Tel: +49 911-740 53 0   http://www.suse.de 
-
SUSE LINUX GmbH, GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer, HRB 21284 
(AG Nürnberg) 
Maxfeldstraße 5 
90409 Nürnberg 
Germany 

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] DRM/i915: Remove magic to prevent blank screen on gen4 chipsets

[Egbert Eich]

Since the root cause is understood now and with the fix

   commit 564ed191f5d816d24501664296991ec70327e2bc
   Author: Imre Deak imre.d...@intel.com
   Date:   Fri Jun 13 14:54:21 2014 +0300

   drm/i915: gmch: fix stuck primary plane due to memory self-refresh mode

in place the magic for G4x chipsets introduced with commit

   commit 61bc95c1fbbb6a08b55bbe161fdf1ea5493fc595
   Author: Egbert Eich e...@suse.com
   Date:   Mon Mar 4 09:24:38 2013 -0500

   DRM/i915: On G45 enable cursor plane briefly after enabling the display 
plane.

to avoided occasional screen blanking on mode changes can finally
be removed.
It's been verified that Imre's fix also resolves the said issue.

Signed-off-by: Egbert Eich e...@suse.de
Tested-by: Stefan Dirsch sndir...@suse.de
---
 drivers/gpu/drm/i915/intel_display.c | 27 ---
 1 file changed, 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index b9bc030..8c3dcdf 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3894,30 +3894,6 @@ static void intel_crtc_dpms_overlay(struct intel_crtc 
*intel_crtc, bool enable)
 */
 }
 
-/**
- * i9xx_fixup_plane - ugly workaround for G45 to fire up the hardware
- * cursor plane briefly if not already running after enabling the display
- * plane.
- * This workaround avoids occasional blank screens when self refresh is
- * enabled.
- */
-static void
-g4x_fixup_plane(struct drm_i915_private *dev_priv, enum pipe pipe)
-{
-   u32 cntl = I915_READ(CURCNTR(pipe));
-
-   if ((cntl  CURSOR_MODE) == 0) {
-   u32 fw_bcl_self = I915_READ(FW_BLC_SELF);
-
-   I915_WRITE(FW_BLC_SELF, fw_bcl_self  ~FW_BLC_SELF_EN);
-   I915_WRITE(CURCNTR(pipe), CURSOR_MODE_64_ARGB_AX);
-   intel_wait_for_vblank(dev_priv-dev, pipe);
-   I915_WRITE(CURCNTR(pipe), cntl);
-   I915_WRITE(CURBASE(pipe), I915_READ(CURBASE(pipe)));
-   I915_WRITE(FW_BLC_SELF, fw_bcl_self);
-   }
-}
-
 static void intel_crtc_enable_planes(struct drm_crtc *crtc)
 {
struct drm_device *dev = crtc-dev;
@@ -3930,9 +3906,6 @@ static void intel_crtc_enable_planes(struct drm_crtc 
*crtc)
 
intel_enable_primary_hw_plane(dev_priv, plane, pipe);
intel_enable_planes(crtc);
-   /* The fixup needs to happen before cursor is enabled */
-   if (IS_G4X(dev))
-   g4x_fixup_plane(dev_priv, pipe);
intel_crtc_update_cursor(crtc, true);
intel_crtc_dpms_overlay(intel_crtc, true);
 
-- 
1.8.4.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 3/3] drm/i915: gmch: fix stuck primary plane due to memory self-refresh mode

[Egbert Eich]

Chris Wilson writes:
  On Fri, Jun 27, 2014 at 12:07:47AM +0200, Egbert Eich wrote:
   
   Hi Daniel, hi Imre,
   
   Daniel Vetter writes:
 Adding Egbert since he's done the original hack here. Imre please keep
 him on cc.
 -Daniel
   
   I finally managed to get this set of patches tested on the platform that
   exhibited the intermittent blanking problem when terminating the Xserver.
   
   I can confirm that Imre's patches resolve the issue and that 
   g4x_fixup_plane()
   which I had introduced after extensive experiments is no longer needed to
   prevent the blanking from happening.
   If you want I can provide a patch to back this out with the appropriate
   comments once Imre's patches are in.
  
  That would be ideal.

Is there a chance that Imre's patches will go into the
Intel repo any time soon? Then I could use the commit Id in
the patch description.

Cheers,
Egbert.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 3/3] drm/i915: gmch: fix stuck primary plane due to memory self-refresh mode

[Egbert Eich]

Hi Daniel, hi Imre,

Daniel Vetter writes:
  Adding Egbert since he's done the original hack here. Imre please keep
  him on cc.
  -Daniel

I finally managed to get this set of patches tested on the platform that
exhibited the intermittent blanking problem when terminating the Xserver.

I can confirm that Imre's patches resolve the issue and that g4x_fixup_plane()
which I had introduced after extensive experiments is no longer needed to
prevent the blanking from happening.
If you want I can provide a patch to back this out with the appropriate
comments once Imre's patches are in.

Cheers,
Egbert.

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v3] drm/i915: Break encoder-crtc link separately in intel_sanitize_crtc()

[Egbert Eich]

Depending on the SDVO output_flags SDVO may have multiple connectors
linking to the same encoder (in intel_connector-encoder-base).
Only one of those connectors should be active (ie link to the encoder
thru drm_connector-encoder).
If intel_connector_break_all_links() is called from intel_sanitize_crtc()
we may break the crtc connection of an encoder thru an inactive connector
in which case intel_connector_break_all_links() will not be called again
for the active connector if this happens to come later in the list due to:
if (connector-encoder-base.crtc != crtc-base)
 continue;
in intel_sanitize_crtc().
This will however leave the drm_connector-encoder linkage for this
active connector in place. Subsequently this will cause multiple
warnings in intel_connector_check_state() to trigger and the driver
will eventually die in drm_encoder_crtc_ok() (because of crtc == NULL).

To avoid this remove intel_connector_break_all_links() and move its
code to its two calling functions: intel_sanitize_crtc() and
intel_sanitize_encoder().
This allows to implement the link breaking more flexibly matching
the surrounding code: ie. in intel_sanitize_crtc() we can break the
crtc link separatly after the links to the encoders have been
broken which avoids above problem.

This regression has been introduced in:

commit 24929352481f085c5f85d4d4cbc919ddf106d381
Author: Daniel Vetter daniel.vet...@ffwll.ch
Date:   Mon Jul 2 20:28:59 2012 +0200

drm/i915: read out the modeset hw state at load and resume time

so goes back to the very beginning of the modeset rework.

Signed-off-by: Egbert Eich e...@suse.de
Reviewed-by: Daniel Vetter daniel.vet...@ffwll.ch
Cc: sta...@vger.kernel.org
Cc: Jani Nikula jani.nik...@linux.intel.com

v2: This patch takes care of the concernes voiced by Chris Wilson
and Daniel Vetter that only breaking links if the drm_connector
is linked to an encoder may miss some links.
v3: move all encoder handling to encoder loop as suggested by
Daniel Vetter.
---
 drivers/gpu/drm/i915/intel_display.c | 26 ++
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index b57210c..7ba8bf5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11373,15 +11373,6 @@ void intel_modeset_init(struct drm_device *dev)
}
 }
 
-static void
-intel_connector_break_all_links(struct intel_connector *connector)
-{
-   connector-base.dpms = DRM_MODE_DPMS_OFF;
-   connector-base.encoder = NULL;
-   connector-encoder-connectors_active = false;
-   connector-encoder-base.crtc = NULL;
-}
-
 static void intel_enable_pipe_a(struct drm_device *dev)
 {
struct intel_connector *connector;
@@ -11463,8 +11454,17 @@ static void intel_sanitize_crtc(struct intel_crtc 
*crtc)
if (connector-encoder-base.crtc != crtc-base)
continue;
 
-   intel_connector_break_all_links(connector);
+   connector-base.dpms = DRM_MODE_DPMS_OFF;
+   connector-base.encoder = NULL;
}
+   /* multiple connectors may have the same encoder:
+*  handle them and break crtc link separately */
+   list_for_each_entry(connector, dev-mode_config.connector_list,
+   base.head)
+   if (connector-encoder-base.crtc == crtc-base) {
+   connector-encoder-base.crtc = NULL;
+   connector-encoder-connectors_active = false;
+   }
 
WARN_ON(crtc-active);
crtc-base.enabled = false;
@@ -11546,6 +11546,8 @@ static void intel_sanitize_encoder(struct intel_encoder 
*encoder)
  drm_get_encoder_name(encoder-base));
encoder-disable(encoder);
}
+   encoder-base.crtc = NULL;
+   encoder-connectors_active = false;
 
/* Inconsistent output/port/pipe state happens presumably due to
 * a bug in one of the get_hw_state functions. Or someplace else
@@ -11556,8 +11558,8 @@ static void intel_sanitize_encoder(struct intel_encoder 
*encoder)
base.head) {
if (connector-encoder != encoder)
continue;
-
-   intel_connector_break_all_links(connector);
+   connector-base.dpms = DRM_MODE_DPMS_OFF;
+   connector-base.encoder = NULL;
}
}
/* Enabled encoders without active connectors will be fixed in
-- 
1.8.4.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/2] drm/i915: Set up SDVO encoder type only at detect

[Egbert Eich]

Daniel Vetter writes:
  On Mon, Apr 14, 2014 at 07:26:09PM +0200, Egbert Eich wrote:
   Depending on the SDVO output_flags SDVO may have multiple connectors.
   These are all initialized in intel_sdvo_output_setup(). The connector
   that is initialized later will override the encoder_type that has been
   set up by an earlier connector type initialization. Eventually the
   one that comes last wins.
   Eventually when intel_sdvo_detect() is called the active connector is
   determined.
   Delay encoder type initialization until the active connector is known
   and set it to the type that corresponds to this connector.
   
   Signed-off-by: Egbert Eich e...@suse.de
  
  Hm, has this any effect on the code itself? I think if we want to fix this
  just for optics we should add a new DRM_MODE_ENCODER_MULTI. At least I
  have an sdvo here which has a dac, hdmi and tv-out ...

With the present logic the last connector in the list matching a flag bit
will win the encoder type. The encoder type is presently just used for
(debug) messages, so it is cosmetic.

  
  This also reminds that I should finally polish of the multi-sdvo fixes I
  have around here - currently the last encoder detected wins on a
  multi-encoder chip, which means if you have an lvds+hdmi combo and plug in
  a screeen you'll never be able to use the lvds again until the hdmi is
  unplugged.

You know, from looking at the code I was wondering already if this was 
possible at all. 
I stayed away tinkering with this - there doesn't seem to be documentation 
on the SDVO command set publically available. Moreover I'm moslty dealing 
with cash registers here - so I doubt they have all the SVIDEO connectors 
they advertise ;) But I can't tell as I don't even have physical access!

As I read the current code it seems like bad things could happen 
if more than one bit is set in intel_sdvo-attached_output: after all
tv-out should have quite different timing requirements than a TMDS.
  
  Much worse if there's a tv-out detect issue ;-)

;p

Cheers,
Egbert.
  
  Cheers, Daniel
  
   ---
drivers/gpu/drm/i915/intel_sdvo.c | 23 ++-
1 file changed, 18 insertions(+), 5 deletions(-)
   
   diff --git a/drivers/gpu/drm/i915/intel_sdvo.c 
   b/drivers/gpu/drm/i915/intel_sdvo.c
   index d27155a..5043f16 100644
   --- a/drivers/gpu/drm/i915/intel_sdvo.c
   +++ b/drivers/gpu/drm/i915/intel_sdvo.c
   @@ -154,6 +154,9 @@ struct intel_sdvo_connector {
  /* Mark the type of connector */
  uint16_t output_flag;

   +  /* store encoder type for convenience */
   +  int encoder_type;
   +
  enum hdmi_force_audio force_audio;

  /* This contains all current supported TV format */
   @@ -1746,6 +1749,7 @@ intel_sdvo_detect(struct drm_connector *connector, 
   bool force)
  if (response == 0)
  return connector_status_disconnected;

   +  intel_sdvo-base.base.encoder_type = intel_sdvo_connector-encoder_type;
  intel_sdvo-attached_output = response;

  intel_sdvo-has_hdmi_monitor = false;
   @@ -2489,7 +2493,9 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, 
   int device)
  } else {
  intel_connector-polled = DRM_CONNECTOR_POLL_CONNECT | 
   DRM_CONNECTOR_POLL_DISCONNECT;
  }
   -  encoder-encoder_type = DRM_MODE_ENCODER_TMDS;
   +  /* delay encoder_type setting until detection */
   +  intel_sdvo_connector-encoder_type = DRM_MODE_ENCODER_TMDS;
   +  encoder-encoder_type = DRM_MODE_ENCODER_NONE;
  connector-connector_type = DRM_MODE_CONNECTOR_DVID;

  if (intel_sdvo_is_hdmi_connector(intel_sdvo, device)) {
   @@ -2524,7 +2530,9 @@ intel_sdvo_tv_init(struct intel_sdvo *intel_sdvo, 
   int type)

  intel_connector = intel_sdvo_connector-base;
  connector = intel_connector-base;
   -  encoder-encoder_type = DRM_MODE_ENCODER_TVDAC;
   +  /* delay encoder_type setting until detection */
   +  intel_sdvo_connector-encoder_type = DRM_MODE_ENCODER_TVDAC;
   +  encoder-encoder_type = DRM_MODE_ENCODER_NONE;
  connector-connector_type = DRM_MODE_CONNECTOR_SVIDEO;

  intel_sdvo-controlled_output |= type;
   @@ -2568,7 +2576,9 @@ intel_sdvo_analog_init(struct intel_sdvo 
   *intel_sdvo, int device)
  intel_connector = intel_sdvo_connector-base;
  connector = intel_connector-base;
  intel_connector-polled = DRM_CONNECTOR_POLL_CONNECT;
   -  encoder-encoder_type = DRM_MODE_ENCODER_DAC;
   +  /* delay encoder_type setting until detection */
   +  intel_sdvo_connector-encoder_type = DRM_MODE_ENCODER_DAC;
   +  encoder-encoder_type = DRM_MODE_ENCODER_NONE;
  connector-connector_type = DRM_MODE_CONNECTOR_VGA;

  if (device == 0) {
   @@ -2603,7 +2613,9 @@ intel_sdvo_lvds_init(struct intel_sdvo *intel_sdvo, 
   int device)

  intel_connector = intel_sdvo_connector-base;
  connector = intel_connector-base;
   -  encoder-encoder_type = DRM_MODE_ENCODER_LVDS;
   +  /* delay encoder_type setting until detection

Re: [Intel-gfx] [PATCH 1/2] drm/i915: Only break encoder linked when linked to connector

[Egbert Eich]

Daniel Vetter writes:
  
  Hm, I think to address Chris' concern we should split this into two
  pieces: connector_break_all links which only breaks connector-encoder
  stuff, used in both places as is. And a new encoder_break_all links which
  we'll use in sanitize_crtc in a new encoder loop.

I've got a different solution, although it requires a bit more 
code: Instead of having a single break_all_links() function I 
move the  link breaking code into the two functions where it 
is used (ie sanitize_encoder() and sanitize_crtc()).
This gives one a bit more flexibility in implementing what is 
needed and makes the code a bit clearer.
I will send later - once I had the opportunity to test.

Cheers,
Egbert.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915: Break encoder-crtc link separately in intel_sanitize_crtc()

[Egbert Eich]

Depending on the SDVO output_flags SDVO may have multiple connectors
linking to the same encoder (in intel_connector-encoder-base).
Only one of those connectors should be active (ie link to the encoder
thru drm_connector-encoder).
If intel_connector_break_all_links() is called from intel_sanitize_crtc()
we may break the crtc connection of an encoder thru an inactive connector
in which case intel_connector_break_all_links() will not be called again
for the active connector if this happens to come later in the list due to:
if (connector-encoder-base.crtc != crtc-base)
 continue;
in intel_sanitize_crtc().
This will however leave the drm_connector-encoder linkage for this
active connector in place. Subsequently this will cause multiple
warnings in intel_connector_check_state() to trigger and the driver
will eventually die in drm_encoder_crtc_ok() (because of crtc == NULL).

To avoid this remove intel_connector_break_all_links() and move its
code to its two calling functions: intel_sanitize_crtc() and
intel_sanitize_encoder().
This allows to implement the link breaking more flexibly matching
the surrounding code: ie. in intel_sanitize_crtc() we can break the
crtc link separatly after the links to the encoders have been
broken which avoids above problem.

Signed-off-by: Egbert Eich e...@suse.de

v2: This patch takes care of the concernes voiced by Chris Wilson
and Daniel Vetter that only breaking links if the drm_connector
is linked to an encoder may miss some links.
---
 drivers/gpu/drm/i915/intel_display.c | 25 +
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 1390ab5..c276733 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11386,15 +11386,6 @@ void intel_modeset_init(struct drm_device *dev)
}
 }
 
-static void
-intel_connector_break_all_links(struct intel_connector *connector)
-{
-   connector-base.dpms = DRM_MODE_DPMS_OFF;
-   connector-base.encoder = NULL;
-   connector-encoder-connectors_active = false;
-   connector-encoder-base.crtc = NULL;
-}
-
 static void intel_enable_pipe_a(struct drm_device *dev)
 {
struct intel_connector *connector;
@@ -11476,8 +11467,16 @@ static void intel_sanitize_crtc(struct intel_crtc 
*crtc)
if (connector-encoder-base.crtc != crtc-base)
continue;
 
-   intel_connector_break_all_links(connector);
+   connector-base.dpms = DRM_MODE_DPMS_OFF;
+   connector-encoder-connectors_active = false;
+   connector-base.encoder = NULL;
}
+   /* multiple connectors may have the same encoder:
+*  break crtc link separately */
+   list_for_each_entry(connector, dev-mode_config.connector_list,
+   base.head)
+   if (connector-encoder-base.crtc == crtc-base)
+   connector-encoder-base.crtc = NULL;
 
WARN_ON(crtc-active);
crtc-base.enabled = false;
@@ -11559,6 +11558,8 @@ static void intel_sanitize_encoder(struct intel_encoder 
*encoder)
  drm_get_encoder_name(encoder-base));
encoder-disable(encoder);
}
+   encoder-base.crtc = NULL;
+   encoder-connectors_active = false;
 
/* Inconsistent output/port/pipe state happens presumably due to
 * a bug in one of the get_hw_state functions. Or someplace else
@@ -11569,8 +11570,8 @@ static void intel_sanitize_encoder(struct intel_encoder 
*encoder)
base.head) {
if (connector-encoder != encoder)
continue;
-
-   intel_connector_break_all_links(connector);
+   connector-base.dpms = DRM_MODE_DPMS_OFF;
+   connector-base.encoder = NULL;
}
}
/* Enabled encoders without active connectors will be fixed in
-- 
1.8.4.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915: Resurrect warning from intel_encoder_crtc_ok()

[Egbert Eich]

Bail out if crtc is NULL to keep the driver from crashing.

Signed-off-by: Egbert Eich e...@suse.de
---
 drivers/gpu/drm/i915/intel_display.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index c276733..dfebced 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10257,6 +10257,11 @@ intel_modeset_stage_output_state(struct drm_device 
*dev,
new_crtc = set-crtc;
}
 
+   if (!new_crtc) {
+   WARN(1, crtc not set!);
+   return -EINVAL;
+   }
+
/* Make sure the new CRTC will work with the encoder */
if (!drm_encoder_crtc_ok(connector-new_encoder-base,
 new_crtc)) {
-- 
1.8.4.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/2] drm/i915: Only break encoder linked when linked to connector

[Egbert Eich]

Chris Wilson writes:
  On Mon, Apr 14, 2014 at 07:26:08PM +0200, Egbert Eich wrote:
   Depending on the SDVO output_flags SDVO may have multiple connectors
   linking to the same encoder (in intel_connector-encoder-base).
   Only one of those connectors should be active (ie link to the encoder
   thru drm_connector-encoder.
   If intel_connector_break_all_links() is called from intel_sanitize_crtc()
   we may brake the crtc connection of an encoder thru an inactive connector
   in which case intel_connector_break_all_links() will not be called again
   for the active connector due to
  if (connector-encoder-base.crtc != crtc-base)
   continue;
   in intel_sanitize_crtc().
   This will however leave the drm_connector-encoder linkage for this
   active connector in place. Subsequently this will cause multiple
   warnings in intel_connector_check_state() to trigger and the driver
   will eventually die in drm_encoder_crtc_ok() (because of crtc == NULL).
   
   To avoid this break the encoder links only if the connector is active
   (ie. has drm_connector-encoder set).
   
   Signed-off-by: Egbert Eich e...@suse.de
  
  This just leaves me with a nagging doubt that not all links will then be
  broken. Do we have sufficient sanity checks to detect the obverse? Would
  it be worth adding a second pass over the connector_list to assert that
  all links are then broken?

Possibly. Maybe we should rework intel_sanitize_encoder() a bit:
loop over all drm_connectors, see if a drm_connector-encoder points
to the encoder in question and make sure that the intel_encoder state
(ie connectors_active and base.crtc) is in sync with the results.

I'll think about it some more ...

Cheers,
Egbert.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v2] drm/i915: Set up SDVO encoder type only at detect

[Egbert Eich]

Depending on the SDVO output_flags SDVO may have multiple connectors.
These are all initialized in intel_sdvo_output_setup(). The connector
that is initialized later will override the encoder_type that has been
set up by an earlier connector type initialization. Eventually the
one that comes last wins.
Eventually when intel_sdvo_detect() is called the active connector is
determined.
Delay encoder type initialization until the active connector is known
and set it to the type that corresponds to this connector.

v2: Remove explicit encoder type initialization to
DRM_MODE_ENCODER_NONE in the SDVO connector setup functions
as suggested by Chris Wilson ch...@chris-wilson.co.uk.

Signed-off-by: Egbert Eich e...@suse.de
---
 drivers/gpu/drm/i915/intel_sdvo.c | 21 +++--
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sdvo.c 
b/drivers/gpu/drm/i915/intel_sdvo.c
index d27155a..f324ca1 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -154,6 +154,9 @@ struct intel_sdvo_connector {
/* Mark the type of connector */
uint16_t output_flag;
 
+   /* store encoder type for convenience */
+   int encoder_type;
+
enum hdmi_force_audio force_audio;
 
/* This contains all current supported TV format */
@@ -1746,6 +1749,7 @@ intel_sdvo_detect(struct drm_connector *connector, bool 
force)
if (response == 0)
return connector_status_disconnected;
 
+   intel_sdvo-base.base.encoder_type = intel_sdvo_connector-encoder_type;
intel_sdvo-attached_output = response;
 
intel_sdvo-has_hdmi_monitor = false;
@@ -2489,7 +2493,8 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int 
device)
} else {
intel_connector-polled = DRM_CONNECTOR_POLL_CONNECT | 
DRM_CONNECTOR_POLL_DISCONNECT;
}
-   encoder-encoder_type = DRM_MODE_ENCODER_TMDS;
+   /* delay encoder_type setting until detection */
+   intel_sdvo_connector-encoder_type = DRM_MODE_ENCODER_TMDS;
connector-connector_type = DRM_MODE_CONNECTOR_DVID;
 
if (intel_sdvo_is_hdmi_connector(intel_sdvo, device)) {
@@ -2524,7 +2529,8 @@ intel_sdvo_tv_init(struct intel_sdvo *intel_sdvo, int 
type)
 
intel_connector = intel_sdvo_connector-base;
connector = intel_connector-base;
-   encoder-encoder_type = DRM_MODE_ENCODER_TVDAC;
+   /* delay encoder_type setting until detection */
+   intel_sdvo_connector-encoder_type = DRM_MODE_ENCODER_TVDAC;
connector-connector_type = DRM_MODE_CONNECTOR_SVIDEO;
 
intel_sdvo-controlled_output |= type;
@@ -2568,7 +2574,8 @@ intel_sdvo_analog_init(struct intel_sdvo *intel_sdvo, int 
device)
intel_connector = intel_sdvo_connector-base;
connector = intel_connector-base;
intel_connector-polled = DRM_CONNECTOR_POLL_CONNECT;
-   encoder-encoder_type = DRM_MODE_ENCODER_DAC;
+   /* delay encoder_type setting until detection */
+   intel_sdvo_connector-encoder_type = DRM_MODE_ENCODER_DAC;
connector-connector_type = DRM_MODE_CONNECTOR_VGA;
 
if (device == 0) {
@@ -2603,7 +2610,8 @@ intel_sdvo_lvds_init(struct intel_sdvo *intel_sdvo, int 
device)
 
intel_connector = intel_sdvo_connector-base;
connector = intel_connector-base;
-   encoder-encoder_type = DRM_MODE_ENCODER_LVDS;
+   /* delay encoder_type setting until detection */
+   intel_sdvo_connector-encoder_type = DRM_MODE_ENCODER_LVDS;
connector-connector_type = DRM_MODE_CONNECTOR_LVDS;
 
if (device == 0) {
@@ -2981,10 +2989,11 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t 
sdvo_reg, bool is_sdvob)
if (!intel_sdvo_init_ddc_proxy(intel_sdvo, dev))
goto err_i2c_bus;
 
-   /* encoder type will be decided later */
+   /* encoder type will be decided in intel_sdvo_detect() */
intel_encoder = intel_sdvo-base;
intel_encoder-type = INTEL_OUTPUT_SDVO;
-   drm_encoder_init(dev, intel_encoder-base, intel_sdvo_enc_funcs, 0);
+   drm_encoder_init(dev, intel_encoder-base, intel_sdvo_enc_funcs,
+DRM_MODE_ENCODER_NONE);
 
/* Read the regs to test if we can talk to the device */
for (i = 0; i  0x40; i++) {
-- 
1.8.4.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 2/2] drm/i915: Set up SDVO encoder type only at detect

[Egbert Eich]

Depending on the SDVO output_flags SDVO may have multiple connectors.
These are all initialized in intel_sdvo_output_setup(). The connector
that is initialized later will override the encoder_type that has been
set up by an earlier connector type initialization. Eventually the
one that comes last wins.
Eventually when intel_sdvo_detect() is called the active connector is
determined.
Delay encoder type initialization until the active connector is known
and set it to the type that corresponds to this connector.

Signed-off-by: Egbert Eich e...@suse.de
---
 drivers/gpu/drm/i915/intel_sdvo.c | 23 ++-
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sdvo.c 
b/drivers/gpu/drm/i915/intel_sdvo.c
index d27155a..5043f16 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -154,6 +154,9 @@ struct intel_sdvo_connector {
/* Mark the type of connector */
uint16_t output_flag;
 
+   /* store encoder type for convenience */
+   int encoder_type;
+
enum hdmi_force_audio force_audio;
 
/* This contains all current supported TV format */
@@ -1746,6 +1749,7 @@ intel_sdvo_detect(struct drm_connector *connector, bool 
force)
if (response == 0)
return connector_status_disconnected;
 
+   intel_sdvo-base.base.encoder_type = intel_sdvo_connector-encoder_type;
intel_sdvo-attached_output = response;
 
intel_sdvo-has_hdmi_monitor = false;
@@ -2489,7 +2493,9 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int 
device)
} else {
intel_connector-polled = DRM_CONNECTOR_POLL_CONNECT | 
DRM_CONNECTOR_POLL_DISCONNECT;
}
-   encoder-encoder_type = DRM_MODE_ENCODER_TMDS;
+   /* delay encoder_type setting until detection */
+   intel_sdvo_connector-encoder_type = DRM_MODE_ENCODER_TMDS;
+   encoder-encoder_type = DRM_MODE_ENCODER_NONE;
connector-connector_type = DRM_MODE_CONNECTOR_DVID;
 
if (intel_sdvo_is_hdmi_connector(intel_sdvo, device)) {
@@ -2524,7 +2530,9 @@ intel_sdvo_tv_init(struct intel_sdvo *intel_sdvo, int 
type)
 
intel_connector = intel_sdvo_connector-base;
connector = intel_connector-base;
-   encoder-encoder_type = DRM_MODE_ENCODER_TVDAC;
+   /* delay encoder_type setting until detection */
+   intel_sdvo_connector-encoder_type = DRM_MODE_ENCODER_TVDAC;
+   encoder-encoder_type = DRM_MODE_ENCODER_NONE;
connector-connector_type = DRM_MODE_CONNECTOR_SVIDEO;
 
intel_sdvo-controlled_output |= type;
@@ -2568,7 +2576,9 @@ intel_sdvo_analog_init(struct intel_sdvo *intel_sdvo, int 
device)
intel_connector = intel_sdvo_connector-base;
connector = intel_connector-base;
intel_connector-polled = DRM_CONNECTOR_POLL_CONNECT;
-   encoder-encoder_type = DRM_MODE_ENCODER_DAC;
+   /* delay encoder_type setting until detection */
+   intel_sdvo_connector-encoder_type = DRM_MODE_ENCODER_DAC;
+   encoder-encoder_type = DRM_MODE_ENCODER_NONE;
connector-connector_type = DRM_MODE_CONNECTOR_VGA;
 
if (device == 0) {
@@ -2603,7 +2613,9 @@ intel_sdvo_lvds_init(struct intel_sdvo *intel_sdvo, int 
device)
 
intel_connector = intel_sdvo_connector-base;
connector = intel_connector-base;
-   encoder-encoder_type = DRM_MODE_ENCODER_LVDS;
+   /* delay encoder_type setting until detection */
+   intel_sdvo_connector-encoder_type = DRM_MODE_ENCODER_LVDS;
+   encoder-encoder_type = DRM_MODE_ENCODER_NONE;
connector-connector_type = DRM_MODE_CONNECTOR_LVDS;
 
if (device == 0) {
@@ -2984,7 +2996,8 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t 
sdvo_reg, bool is_sdvob)
/* encoder type will be decided later */
intel_encoder = intel_sdvo-base;
intel_encoder-type = INTEL_OUTPUT_SDVO;
-   drm_encoder_init(dev, intel_encoder-base, intel_sdvo_enc_funcs, 0);
+   drm_encoder_init(dev, intel_encoder-base, intel_sdvo_enc_funcs,
+DRM_MODE_ENCODER_NONE);
 
/* Read the regs to test if we can talk to the device */
for (i = 0; i  0x40; i++) {
-- 
1.8.4.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 0/2] Fix if multiple SDVO outputs are flagged

[Egbert Eich]

Currently the i915 driver can only handle well if only a single
SDVO output is flagged (ie output_flags has only one bit set).
If multiple outputs are flagged the side effects are only cosmetic
in most cases (ie. the encoder may have the wrong type set),
but there are situations (namely when intel_connector_break_all_links()
is called) where this may lead to an inconsistent driver state.

The following two patches fix both situations.

Egbert Eich (2):
  drm/i915: Only break encoder linked when linked to connector
  drm/i915: Set up SDVO encoder type only at detect

 drivers/gpu/drm/i915/intel_display.c |  2 ++
 drivers/gpu/drm/i915/intel_sdvo.c| 23 ++-
 2 files changed, 20 insertions(+), 5 deletions(-)

-- 
1.8.4.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 1/2] drm/i915: Only break encoder linked when linked to connector

[Egbert Eich]

Depending on the SDVO output_flags SDVO may have multiple connectors
linking to the same encoder (in intel_connector-encoder-base).
Only one of those connectors should be active (ie link to the encoder
thru drm_connector-encoder.
If intel_connector_break_all_links() is called from intel_sanitize_crtc()
we may brake the crtc connection of an encoder thru an inactive connector
in which case intel_connector_break_all_links() will not be called again
for the active connector due to
   if (connector-encoder-base.crtc != crtc-base)
continue;
in intel_sanitize_crtc().
This will however leave the drm_connector-encoder linkage for this
active connector in place. Subsequently this will cause multiple
warnings in intel_connector_check_state() to trigger and the driver
will eventually die in drm_encoder_crtc_ok() (because of crtc == NULL).

To avoid this break the encoder links only if the connector is active
(ie. has drm_connector-encoder set).

Signed-off-by: Egbert Eich e...@suse.de
---
 drivers/gpu/drm/i915/intel_display.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 1390ab5..041f847 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11390,6 +11390,8 @@ static void
 intel_connector_break_all_links(struct intel_connector *connector)
 {
connector-base.dpms = DRM_MODE_DPMS_OFF;
+   if (!connector-base.encoder)
+   return;
connector-base.encoder = NULL;
connector-encoder-connectors_active = false;
connector-encoder-base.crtc = NULL;
-- 
1.8.4.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Don't WARN about unexpected hpd interrupts on gmch platforms

[Egbert Eich]

Daniel Vetter writes:
  The status bits are unconditionally set, the control bits only enable
  the actual interrupt generation. Which means if we get some random
  other interrupts we'll bogusly complain about them.
  
  So restrict the WARN to platforms with a sane hotplug interrupt
  handling scheme.
  
  This WARN has been introduced in
  
  commit b8f102e8bf71cacf33326360fdf9dcfd1a63925b
  Author: Egbert Eich e...@suse.de
  Date:   Fri Jul 26 14:14:24 2013 +0200
  
  drm/i915: Add messages useful for HPD storm detection debugging (v2)
  
  Cc: Egbert Eich e...@suse.de
  Cc: bitlord bitlord0...@gmail.com
  Reported-by: bitlord bitlord0...@gmail.com
  Cc: sta...@vger.kernel.org
  Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
  ---
   drivers/gpu/drm/i915/i915_irq.c | 18 ++
   1 file changed, 14 insertions(+), 4 deletions(-)
  
  diff --git a/drivers/gpu/drm/i915/i915_irq.c 
  b/drivers/gpu/drm/i915/i915_irq.c
  index 7753249b3a95..f98ba4e6e70b 100644
  --- a/drivers/gpu/drm/i915/i915_irq.c
  +++ b/drivers/gpu/drm/i915/i915_irq.c
  @@ -1362,10 +1362,20 @@ static inline void intel_hpd_irq_handler(struct 
  drm_device *dev,
   spin_lock(dev_priv-irq_lock);
   for (i = 1; i  HPD_NUM_PINS; i++) {
   
  -WARN_ONCE(hpd[i]  hotplug_trigger 
  -  dev_priv-hpd_stats[i].hpd_mark == HPD_DISABLED,
  -  Received HPD interrupt (0x%08x) on pin %d (0x%08x) 
  although disabled\n,
  -  hotplug_trigger, i, hpd[i]);
  +if (hpd[i]  hotplug_trigger 
  +dev_priv-hpd_stats[i].hpd_mark == HPD_DISABLED) {
  +/*
  + * On GMCH platforms the interrupt mask bits only
  + * prevent irq generation, not the setting of the
  + * hotplug bits itself. So only WARN about unexpected
  + * interrupts on saner platforms.
  + */
  +WARN_ONCE(INTEL_INFO(dev)-gen = 5  
  !IS_VALLEYVIEW(dev),
  +  Received HPD interrupt (0x%08x) on pin %d 
  (0x%08x) although disabled\n,
  +  hotplug_trigger, i, hpd[i]);

Personally I'd prefer the condition in the WARN..() macro to be the 
unexpected condition you want to warn about. This makes it easier for
anybody not up to speed with the details of hotplug handling to understand
the code. 
Of course the way you structure this avoids a lot of unnecessary tests.
But if this is a concern maybe the entire for loop should be restructured
with 

if (!(hpd[i]  hotplug_trigger))
 continue;

right at the beginning.

  +
  +continue;
  +}
   
   if (!(hpd[i]  hotplug_trigger) ||
   dev_priv-hpd_stats[i].hpd_mark != HPD_ENABLED)
  -- 
  1.8.5.2

Cheers,
Egbert.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915/SDVO: For sysfs link put directory and target in correct order

[Egbert Eich]

When linking the i2c sysfs file into the connector's directory
pass directory and link target in the right order.
This code was introduced with:

  commit 931c1c26983b4f84e33b78579fc8d57e4a14c6b4
  Author: Imre Deak imre.d...@intel.com
  Date:   Tue Feb 11 17:12:51 2014 +0200

drm/i915: sdvo: add i2c sysfs symlink to the connector's directory

This is the same what we do for DP connectors, so make things more
consistent.

Signed-off-by: Egbert Eich e...@suse.de
---
 drivers/gpu/drm/i915/intel_sdvo.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sdvo.c 
b/drivers/gpu/drm/i915/intel_sdvo.c
index 5043f16..2d4d461 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -2428,8 +2428,8 @@ intel_sdvo_connector_init(struct intel_sdvo_connector 
*connector,
if (ret  0)
goto err1;
 
-   ret = sysfs_create_link(encoder-ddc.dev.kobj,
-   drm_connector-kdev-kobj,
+   ret = sysfs_create_link(drm_connector-kdev-kobj,
+   encoder-ddc.dev.kobj,
encoder-ddc.dev.kobj.name);
if (ret  0)
goto err2;
-- 
1.8.4.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] HPD flood warning since b8f102e8b

[Egbert Eich]

Jiri Kosina writes:
  On Mon, 21 Oct 2013, Egbert Eich wrote:
  
 On Thu, 3 Oct 2013, Daniel Vetter wrote:
 
  Can you please attach full dmesg from boot up to the first WARN with
  drm.debug=0xe? This really shouldn't happen and indicates a bug
  somewhere ...
 
 A bit difficult ... I originally thought that it was reliably 
 reproducible, but now I didn't get it after 10 suspend/resume cycles. 
   Will 
 keep following it, and once it appears, will send you the dmesg.
 
   Could you check if you get any messages regarding HPD storms after 
   suspend/resume ie messages like:
   [drm] HPD interrupt storm detected on connector HDMI-A-1
   but without the annouing warn messages?
  
  I have this:
  
  [357128.184113] [drm] HPD interrupt storm detected on connector DP-3: 
  switching from hotplug detection to polling
  
  It appeared in the log approximately 5 seconds after resume has been 
  completed.
  

:( You seem to get this on different connector.

Any chance for a 'lspci -n' output?

Cheers,
Egbert.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] HPD flood warning since b8f102e8b

[Egbert Eich]

Hi Jiri,

Just found your email, it got missed do to a temporary inaccessibility to
my email.

Jiri Kosina writes:
  On Thu, 3 Oct 2013, Daniel Vetter wrote:
  
   Can you please attach full dmesg from boot up to the first WARN with
   drm.debug=0xe? This really shouldn't happen and indicates a bug
   somewhere ...
  
  A bit difficult ... I originally thought that it was reliably 
  reproducible, but now I didn't get it after 10 suspend/resume cycles. Will 
  keep following it, and once it appears, will send you the dmesg.
  
Could you check if you get any messages regarding HPD storms after 
suspend/resume ie messages like:
[drm] HPD interrupt storm detected on connector HDMI-A-1
but without the annouing warn messages?

I cannot find anything obviously wrong in the code. 
However there are several code paths for different hardware though - could 
you give me an 'lspci -n' output so I can narrow them down?

Thanks!
Cheers,
Egbert.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915: Add messages usful for HPD strom detection debugging

[Egbert Eich]

For HPD storm detection we now mask out individual interrupt source
bits. We have already seen a case where HPD interrupt enable bits
were assigned to the wrong pins. To track these conditions more
easily add some debugging messages.

Signed-off-by: Egbert Eich e...@suse.de
---
 drivers/gpu/drm/i915/i915_irq.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index ee3e49c..1f3a971 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -919,6 +919,10 @@ static inline void intel_hpd_irq_handler(struct drm_device 
*dev,
spin_lock(dev_priv-irq_lock);
for (i = 1; i  HPD_NUM_PINS; i++) {
 
+   WARN(((hpd[i]  hotplug_trigger) 
+ dev_priv-hpd_stats[i].hpd_mark != HPD_ENABLED),
+Received HPD intterupt although disabled\n);
+
if (!(hpd[i]  hotplug_trigger) ||
dev_priv-hpd_stats[i].hpd_mark != HPD_ENABLED)
continue;
@@ -929,6 +933,7 @@ static inline void intel_hpd_irq_handler(struct drm_device 
*dev,
   + 
msecs_to_jiffies(HPD_STORM_DETECT_PERIOD))) {
dev_priv-hpd_stats[i].hpd_last_jiffies = jiffies;
dev_priv-hpd_stats[i].hpd_cnt = 0;
+   DRM_DEBUG_KMS(Received HPD interrupt on PIN %d - cnt: 
0\n, i);
} else if (dev_priv-hpd_stats[i].hpd_cnt  
HPD_STORM_THRESHOLD) {
dev_priv-hpd_stats[i].hpd_mark = HPD_MARK_DISABLED;
dev_priv-hpd_event_bits = ~(1  i);
@@ -936,6 +941,8 @@ static inline void intel_hpd_irq_handler(struct drm_device 
*dev,
storm_detected = true;
} else {
dev_priv-hpd_stats[i].hpd_cnt++;
+   DRM_DEBUG_KMS(Received HPD interrupt on PIN %d - cnt: 
%d\n, i,
+ dev_priv-hpd_stats[i].hpd_cnt);
}
}
 
-- 
1.8.1.4

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915: Make sure PORT_HOTPLUG_EN is written

[Egbert Eich]

Add posting read to make sure PORT_HOTPLUG_EN is written in
i915_hpd_irq_setup().

Signed-off-by: Egbert Eich e...@suse.de
---
 drivers/gpu/drm/i915/i915_irq.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 1f3a971..a38372b 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2805,6 +2805,7 @@ static void i915_hpd_irq_setup(struct drm_device *dev)
 
/* Ignore TV since it's buggy */
I915_WRITE(PORT_HOTPLUG_EN, hotplug_en);
+   POSTING_READ(PORT_HOTPLUG_EN);
}
 }
 
-- 
1.8.1.4

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Make sure PORT_HOTPLUG_EN is written

[Egbert Eich]

Chris Wilson writes:
  On Fri, Jul 26, 2013 at 12:31:30PM +0200, Egbert Eich wrote:
   Add posting read to make sure PORT_HOTPLUG_EN is written in
   i915_hpd_irq_setup().
  
  It's not vital that the write be flushed here. On the deferred reenable
  path a further delay in rearming is not significant, and inside the irq
  handler (for disabling) the write will be flushed.
  
  In terms of the patch itself, you would also need to fixup
  ibx_hpd_irq_setup as well.
  -Chris

We can probably drop this patch. It was one try figuring why HPD interrupts
are still seeen on pins which have been disabled. It turned out that the
root cause was a mixup in the mapping of pins to enable bits due to incorrect
documentation.

Cheers,
Egbert.

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH V2] drm/i915: Add messages useful for HPD storm detection debugging (v2)

[Egbert Eich]

For HPD storm detection we now mask out individual interrupt source
bits. We have already seen a case where HPD interrupt enable bits
were assigned to the wrong pins. To track these conditions more
easily add some debugging messages.

v2: Spelling fixes as suggested by Jani Nikula jani.nik...@linux.intel.com

Signed-off-by: Egbert Eich e...@suse.de
---
 drivers/gpu/drm/i915/i915_irq.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index ee3e49c..6a1c207 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -919,6 +919,10 @@ static inline void intel_hpd_irq_handler(struct drm_device 
*dev,
spin_lock(dev_priv-irq_lock);
for (i = 1; i  HPD_NUM_PINS; i++) {
 
+   WARN(((hpd[i]  hotplug_trigger) 
+ dev_priv-hpd_stats[i].hpd_mark != HPD_ENABLED),
+Received HPD interrupt although disabled\n);
+
if (!(hpd[i]  hotplug_trigger) ||
dev_priv-hpd_stats[i].hpd_mark != HPD_ENABLED)
continue;
@@ -929,6 +933,7 @@ static inline void intel_hpd_irq_handler(struct drm_device 
*dev,
   + 
msecs_to_jiffies(HPD_STORM_DETECT_PERIOD))) {
dev_priv-hpd_stats[i].hpd_last_jiffies = jiffies;
dev_priv-hpd_stats[i].hpd_cnt = 0;
+   DRM_DEBUG_KMS(Received HPD interrupt on PIN %d - cnt: 
0\n, i);
} else if (dev_priv-hpd_stats[i].hpd_cnt  
HPD_STORM_THRESHOLD) {
dev_priv-hpd_stats[i].hpd_mark = HPD_MARK_DISABLED;
dev_priv-hpd_event_bits = ~(1  i);
@@ -936,6 +941,8 @@ static inline void intel_hpd_irq_handler(struct drm_device 
*dev,
storm_detected = true;
} else {
dev_priv-hpd_stats[i].hpd_cnt++;
+   DRM_DEBUG_KMS(Received HPD interrupt on PIN %d - cnt: 
%d\n, i,
+ dev_priv-hpd_stats[i].hpd_cnt);
}
}
 
-- 
1.8.1.4

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] i915 irq storm mitigation in 3.10

[Egbert Eich]

Hi Jan!

Jan Niggemann writes:
  Hi Egbert,

[...]

Thanks for generating the logs! Hope you had a nice birthday dinner :)

  
  I applied this patch (but not the one you sent on Monday), recompiled 
  and logged:
  uncompressed (8,2M) http://files.hz6.de/kern_20130724.log
  bz2 (288 KB) http://files.hz6.de/kern_20130724.log.bz2

These logs show clearly that we are seeing interrupts which
should be disabled.
Could it be that we we have either the status or enable bits
mixed up? Unfortunately the publically available docs for GEN4
don't show the HOTPLUG_EN and HOTPLUG_STAT registers.

Daniel, could you please help me out here?

Thanks a lot!

Cheers,
Egbert.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] i915 irq storm mitigation in 3.10

[Egbert Eich]

Daniel Vetter writes:
  
  I've checked the docs again for gm45 and we seem to have the right
  values. But on early gen4 (i.e. i965g/gm) the Bspec has been wrong
  about these, so I wouldn't be surprised at all if they're wrong for
  the digital ports on gm45, too.
  
  Iirc we've already had a case like that, but there was no real
  conclusion (and atm I can't find the bug). Can you pls try the below
  patch (on top of Egbert's debug stuff)?
  
If this doesn't help, lets disable all the digital ports together for
testing.

  
  
  Egbert I think your debug patch has fairly useful information for
  debugging the storm code in general, can you please submit a patch
  against drm-intel-nightly?
  

Will do :)

Cheers,
Egbert.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] i915 irq storm mitigation in 3.10

[Egbert Eich]

Jan Niggemann writes:
  Hi Daniel,
  
  Am 25.07.2013 11:58, schrieb Daniel Vetter:
   Can you pls try the below
   patch (on top of Egbert's debug stuff)?
  
   diff --git a/drivers/gpu/drm/i915/i915_reg.h
   b/drivers/gpu/drm/i915/i915_reg.h
   index 6caa748..2d4c884 100644
   --- a/drivers/gpu/drm/i915/i915_reg.h
   +++ b/drivers/gpu/drm/i915/i915_reg.h
   @@ -1925,9 +1925,9 @@
  
#define PORT_HOTPLUG_STAT  (dev_priv-info-display_mmio_offset
   + 0x61114)
/* HDMI/DP bits are gen4+ */
   -#define   PORTB_HOTPLUG_LIVE_STATUS   (1  29)
   +#define   PORTD_HOTPLUG_LIVE_STATUS   (1  29)
#define   PORTC_HOTPLUG_LIVE_STATUS   (1  28)
   -#define   PORTD_HOTPLUG_LIVE_STATUS   (1  27)
   +#define   PORTB_HOTPLUG_LIVE_STATUS   (1  27)
#define   PORTD_HOTPLUG_INT_STATUS (3  21)
#define   PORTC_HOTPLUG_INT_STATUS (3  19)
#define   PORTB_HOTPLUG_INT_STATUS (3  17)
  I did, here are the logs:
  364K http://files.hz6.de/kern_20130724_2.log

I get a 'forbidden' when I try to access this.

  I don't understand what exactly this patch does, but I noticed:
  - much less drm debug info, resulting in a much smaller log
  - no more noticeable lag on my system (even though a storm was 
  detected).

Ok, so this indicates that the storm detection works now :)

  I double checked the latter and the lag seems indeed to be gone...

Before we actually masked out individual events it was irrelevant
if the bits were mixed up. Now we need to be correct on these.

Jan, thanks for your help!

Cheers,
Egbert.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] i915 irq storm mitigation in 3.10

[Egbert Eich]

Hi Jan -

Jan Niggemann writes:
  
  As to the log: I messed up the kernel parameters this morning... was 
  out of coffee this morning and my 1,5y daughter played around me :-)
  
  Here's my kernel log with drm.debug and printk.time enabled:
  Uncompressed (22M): http://files.hz6.de/kern_20130722.log
  bzip2'd (some 600 KB): http://files.hz6.de/kern_20130722.log.bz2

I've looked at the logs a bit more. Here's a patch adding some more
debug information. Would you please apply this to your 3.10 kernel
and generate a log file the same way as you did before.
The driver will be even more chatty - but I don't expect any problems
from this.

Cheers,
Egbert.

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index e43d809..46bb77c 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -919,6 +919,11 @@ static inline void intel_hpd_irq_handler(struct drm_device 
*dev,
spin_lock(dev_priv-irq_lock);
for (i = 1; i  HPD_NUM_PINS; i++) {
 
+   if (IS_G4X(dev)  (hpd[i]  hotplug_trigger) 
+   dev_priv-hpd_stats[i].hpd_mark != HPD_ENABLED)
+   DRM_DEBUG_KMS(Received HPD intterupt although 
disabled\n,
+ I915_READ(PORT_HOTPLUG_EN));
+
if (!(hpd[i]  hotplug_trigger) ||
dev_priv-hpd_stats[i].hpd_mark != HPD_ENABLED)
continue;
@@ -929,6 +934,8 @@ static inline void intel_hpd_irq_handler(struct drm_device 
*dev,
   + 
msecs_to_jiffies(HPD_STORM_DETECT_PERIOD))) {
dev_priv-hpd_stats[i].hpd_last_jiffies = jiffies;
dev_priv-hpd_stats[i].hpd_cnt = 0;
+   DRM_DEBUG_KMS(Received HPD interrupt on PIN %d - 
jiffies: %d\n, i,
+ dev_priv-hpd_stats[i].hpd_last_jiffies);
} else if (dev_priv-hpd_stats[i].hpd_cnt  
HPD_STORM_THRESHOLD) {
dev_priv-hpd_stats[i].hpd_mark = HPD_MARK_DISABLED;
dev_priv-hpd_event_bits = ~(1  i);
@@ -936,6 +943,8 @@ static inline void intel_hpd_irq_handler(struct drm_device 
*dev,
storm_detected = true;
} else {
dev_priv-hpd_stats[i].hpd_cnt++;
+   DRM_DEBUG_KMS(Received HPD interrupt on PIN %d - cnt: 
%d\n, i,
+ dev_priv-hpd_stats[i].hpd_cnt);
}
}
 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] i915 irq storm mitigation in 3.10

[Egbert Eich]

Daniel Vetter writes:
  On Sun, Jul 21, 2013 at 10:23 PM, Jan Niggemann j...@hz6.de wrote:
   But every time this happens we only let through a few interrupts, so this
   shouldn't affect you badly. Can you please check whether those slowdowns
   line up with 2 minute intervalls?
  
   I observed these slowdowns for a couple of weeks now. On my machine, they
   only happen once, some minutes after a cold boot.
   They last for a minute or two, and then they are gone.
   I'd have guessed that the storm detection kicks in pretty quickly after a
   storm is detected and that it would go unnoticed.
  
  Hm, that sounds like something doesn't quite work as expected. We
  should kill things once we get 5 interrupts or so in 1 second. So if
  it's bad enough that it slows your machine down it really should only
  be barely noticeable.
  

The logs show that the disable mechanism got triggered, so there was
a storm that got detected.
The respective message is generated by the worker, everything up to 
there (detection and marking disabled) seems to be fine.
I bet we are still getting interrupts but the respective bit in 
hpd_event_bits doesn't get set any more. Since we unconditionally 
queue the worker on interrupt there is surprise it is so busy.

Then this points to the call to hpd_irq_setup() in intel_hpd_irq_handler()
not doing what is expected, ie masking out the stormy interrupt.
Could it be that we can't mask/disable an interrupt before ACKing
it?

@Jan, could you also specify what hardware you are using (ie give us
an output of lspci -n)?


Cheers,
Egbert.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 04/24] drm/i915: s/hotplug_irq_storm_detect/intel_hpd_irq_handler/

[Egbert Eich]

Daniel Vetter writes:
  The combination of Paulo's fifo underrun detection code and Egbert's
  hpd storm handling code unfortunately made the hpd storm handling code
  racy.
  
  To avoid duplicating tricky interrupt locking code over all platforms
  start with a bit of refactoring. This patch is the very first step
  since in the end the irq storm handling code will handle all hotplug
  logic (and so also encapsulate the locking nicely).
  
  Cc: Egbert Eich e...@suse.de
  Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
  ---
   drivers/gpu/drm/i915/i915_irq.c | 16 
   1 file changed, 8 insertions(+), 8 deletions(-)
  

[reviewed code deleted]

Reviewed-by: Egbert Eich e...@suse.de
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 06/24] drm/i915: fold the queue_work into intel_hpd_irq_handler

[Egbert Eich]

Daniel Vetter writes:
  Everywhere the same.
  
  Cc: Egbert Eich e...@suse.de
  Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
  ---
   drivers/gpu/drm/i915/i915_irq.c | 11 +++
   1 file changed, 3 insertions(+), 8 deletions(-)

[reviewed code deleted]

Reviewed-by: Egbert Eich e...@suse.de
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 08/24] drm/i915: fix hpd interrupt register locking

[Egbert Eich]

Daniel Vetter writes:
  Our interrupt handler (in hardird context) could race with the timer
  Nitpick: s/d/q/

  (in softirq context), hence we need to hold the spinlock around the
  call to -hdp_irq_setup in intel_hpd_irq_handler, too.
  
  But as an optimization (and more so to clarify things) we don't need
  to do the irqsave/restore dance in the hardirq context.
  
  Note also that on ilk+ the race isn't just against the hotplug
  reenable timer, but also against the fifo underrun reporting. That one
  also modifies the SDEIMR register (again protected by the same
  dev_priv-irq_lock).
  
  To lock things down again sprinkle a assert_spin_locked. But exclude
  the functions touching SDEIMR for now, I want to extract them all into
  a new helper function (like we do already for pipestate, display
  interrupts and all the various gt interrupts).
  
  Cc: Egbert Eich e...@suse.de
  Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
  ---
   drivers/gpu/drm/i915/i915_irq.c | 15 ++-
   1 file changed, 10 insertions(+), 5 deletions(-)
  
  diff --git a/drivers/gpu/drm/i915/i915_irq.c 
  b/drivers/gpu/drm/i915/i915_irq.c
  index 1815891..95e15cd 100644
  --- a/drivers/gpu/drm/i915/i915_irq.c
  +++ b/drivers/gpu/drm/i915/i915_irq.c
  @@ -880,15 +880,13 @@ static inline void intel_hpd_irq_handler(struct 
  drm_device *dev,
const u32 *hpd)
   {
   drm_i915_private_t *dev_priv = dev-dev_private;
  -unsigned long irqflags;
   int i;
   bool storm_detected = false;
   
   if (!hotplug_trigger)
   return;
   
  -spin_lock_irqsave(dev_priv-irq_lock, irqflags);
  -
  +spin_lock(dev_priv-irq_lock);
   for (i = 1; i  HPD_NUM_PINS; i++) {
   
   if (!(hpd[i]  hotplug_trigger) ||
  @@ -911,10 +909,9 @@ static inline void intel_hpd_irq_handler(struct 
  drm_device *dev,
   }
   }
   
  -spin_unlock_irqrestore(dev_priv-irq_lock, irqflags);
  -
   if (storm_detected)
   dev_priv-display.hpd_irq_setup(dev);
  +spin_unlock(dev_priv-irq_lock);
   
   queue_work(dev_priv-wq,
  dev_priv-hotplug_work);
  @@ -3327,6 +3324,8 @@ static void i915_hpd_irq_setup(struct drm_device *dev)
   struct intel_encoder *intel_encoder;
   u32 hotplug_en;
   
  +assert_spin_locked(dev_priv-irq_lock);
  +
   if (I915_HAS_HOTPLUG(dev)) {
   hotplug_en = I915_READ(PORT_HOTPLUG_EN);
   hotplug_en = ~HOTPLUG_INT_EN_MASK;


Didn't you want to do the same for ibx_hpd_irq_setup() ?

  @@ -3610,6 +3609,7 @@ void intel_hpd_init(struct drm_device *dev)
   struct drm_i915_private *dev_priv = dev-dev_private;
   struct drm_mode_config *mode_config = dev-mode_config;
   struct drm_connector *connector;
  +unsigned long irqflags;
   int i;
   
   for (i = 1; i  HPD_NUM_PINS; i++) {
  @@ -3622,6 +3622,11 @@ void intel_hpd_init(struct drm_device *dev)
   if (!connector-polled  I915_HAS_HOTPLUG(dev)  
  intel_connector-encoder-hpd_pin  HPD_NONE)
   connector-polled = DRM_CONNECTOR_POLL_HPD;
   }
  +
  +/* Interrup setup is already guaranteed to be single-threaded, this is

Nitpick -  missed a 't'.

  + * just to make the assert_spin_locked checks happy. */
  +spin_lock_irqsave(dev_priv-irq_lock, irqflags);
   if (dev_priv-display.hpd_irq_setup)
   dev_priv-display.hpd_irq_setup(dev);
  +spin_unlock_irqrestore(dev_priv-irq_lock, irqflags);
   }
  -- 
  1.8.1.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 05/24] drm/i915: fold the hpd_irq_setup call into intel_hpd_irq_handler

[Egbert Eich]

Daniel Vetter writes:
  We already have a vfunc for this (and other parts of the hpd storm
  handling code already use it).
  
  Cc: Egbert Eich e...@suse.de
  Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
  ---
   drivers/gpu/drm/i915/i915_irq.c | 29 +++--
   1 file changed, 11 insertions(+), 18 deletions(-)
  

[ reviewed code deleted ]

Reviewed-by: Egbert Eich e...@suse.de


___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: prefer VBT modes for SVDO-LVDS over EDID

[Egbert Eich]

Chris Wilson writes:
  On Sun, Jun 09, 2013 at 11:28:12PM +0200, Daniel Vetter wrote:
   On Sun, Jun 9, 2013 at 11:16 PM, Chris Wilson ch...@chris-wilson.co.uk 
   wrote:
On Sun, Jun 09, 2013 at 10:58:38PM +0200, Daniel Vetter wrote:
In
   
commit 53d3b4d7778daf15900867336c85d3f8dd70600c
Author: Egbert Eich e...@suse.de
Date:   Tue Jun 4 17:13:21 2013 +0200
   
drm/i915/sdvo: Use intel_sdvo-ddc instead of intel_sdvo-i2c for 
DDC
   
Ebgert Eich fixed a long-standing bug where we simply used a
non-working i2c controller to read the EDID for SDVO-LVDS panels.
Unfortunately some machines seem to not be able to cope with the mode
provided in the EDID (specifically they seem to not be able to cope
with a 4x pixel mutliplier instead of a 2x one).
   
Since it took forever to notice the breakage it's fairly safe to
assume that at least for SDVO-LVDS panels the VBT contains fairly sane
data. So just switch around the order and use VBT modes first.
   
Cc: Egbert Eich e...@suse.de
Cc: Chris Wilson ch...@chris-wilson.co.uk
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=65524
Cc: sta...@vger.kernel.org
Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
   
I can accept the argument that we need to prefer the VBT mode here to
paper over the apparent regression, but to not pass on the full EDID
modes seems dubious.
   
   I'm not sure there's any value in additional modes. We can't really
   support frequency switching over sdvo (it'd very likely require a
   mutliplier change) and for everything else we only ever let the fixed
   mode through the fixup hook.
  
  I am trying not to generalise from the broken behaviour on this machine.
  On another machine, there may be some value in the extra modes.
  Select the sanest default we can, then let the user go nuts with the
  extra information.

While I'm not a fan of setting non-native modes on panels this seems
to be a requirement in some environments - not sure if there are any
real world use cases but at least many QA test scenarios seem to include 
such modes.
So adding the EDID modes (unflagging the preferred bit!) would be what 
I'd opt for - admittedly for a very selfish reason: it will spare me 
of lengthy, pointless discussions with some partners' QA departments 
next time we update the Intel driver.

Once again we go out of our ways to accomodate the most broken
devices by imposing more limitations on all others as well. At 
one point we may even have to give in face the grim reality and 
start blacklisting some of the broken crap.

And since we are so much into bikeshedding here - maybe you could
fix my name in the comment ;)

Cheers,
Egbert.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/6] drm/i915: Always enable the cursor right after the primary plane

[Egbert Eich]

On Tue, May 21, 2013 at 05:17:27PM +0200, Egbert Eich wrote:
 On Wed, May 08, 2013 at 12:50:24PM +0300, ville.syrj...@linux.intel.com wrote:
  From: Ville Syrjälä ville.syrj...@linux.intel.com
  
  Follow the same sequence when enabling the cursor plane during
  modeset. No point in doing this stuff in different order on different
  generations.
  
  This should also avoid a needless wait for vblank for the g4x cursor
  workaround when the cursor gets enabled anyway.
  
  Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
  ---
   drivers/gpu/drm/i915/intel_display.c | 10 --
   1 file changed, 4 insertions(+), 6 deletions(-)
  
  @@ -3727,6 +3725,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
   
  intel_enable_pipe(dev_priv, pipe, false);
  intel_enable_plane(dev_priv, plane, pipe);
  +   intel_crtc_update_cursor(crtc, true);
  if (IS_G4X(dev))
  g4x_fixup_plane(dev_priv, pipe);
   
 
 As discussed on IRC this may interfere with 
 
 commit 61bc95c1fbbb6a08b55bbe161fdf1ea5493fc595
 Author: Egbert Eich e...@suse.com
 Date:   Mon Mar 4 09:24:38 2013 -0500
 
 DRM/i915: On G45 enable cursor plane briefly after enabling the display 
 plane.
 
 described in https://bugs.freedesktop.org/show_bug.cgi?id=61457
 

Today I had the chance to test this. First I tried
if I can still reproduce the blank with this patch
added when I disable my voodoo g4x_fixup_plane():
It turned out it still happens however very rarely
(like 1 out of 20 tries). When I reenabled my voodoo
the issue still occurred.
I had to switch two lines around, ie:

intel_enable_plane(dev_priv, plane, pipe);
if (IS_G4X(dev))
g4x_fixup_plane(dev_priv, pipe);
+   intel_crtc_update_cursor(crtc, true);

to avoid the blank screen issue - which is it didn't
happen in ~75 tries.

With this change:

Acked-by: Egbert Eich e...@suse.com


Cheers,
Egbert.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915/sdvo: Use intel_sdvo-ddc instead of intel_sdvo-i2c for DDC.

[Egbert Eich]

In intel_sdvo_get_lvds_modes() the wrong i2c adapter record is used
for DDC. Thus the code will always have to rely on a LVDS panel
mode supplied by VBT.
In most cases this succeeds, so this didn't get detected for quite
a while.

Signed-off-by: Egbert Eich e...@suse.de
---
 drivers/gpu/drm/i915/intel_sdvo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_sdvo.c 
b/drivers/gpu/drm/i915/intel_sdvo.c
index 7068195..8618b15 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -1848,7 +1848,7 @@ static void intel_sdvo_get_lvds_modes(struct 
drm_connector *connector)
 * Assume that the preferred modes are
 * arranged in priority order.
 */
-   intel_ddc_get_modes(connector, intel_sdvo-i2c);
+   intel_ddc_get_modes(connector, intel_sdvo-ddc);
if (list_empty(connector-probed_modes) == false)
goto end;
 
-- 
1.8.1.4

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/6] drm/i915: Always enable the cursor right after the primary plane

[Egbert Eich]

On Wed, May 08, 2013 at 12:50:24PM +0300, ville.syrj...@linux.intel.com wrote:
 From: Ville Syrjälä ville.syrj...@linux.intel.com
 
 Follow the same sequence when enabling the cursor plane during
 modeset. No point in doing this stuff in different order on different
 generations.
 
 This should also avoid a needless wait for vblank for the g4x cursor
 workaround when the cursor gets enabled anyway.
 
 Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
 ---
  drivers/gpu/drm/i915/intel_display.c | 10 --
  1 file changed, 4 insertions(+), 6 deletions(-)
 
 @@ -3727,6 +3725,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
  
   intel_enable_pipe(dev_priv, pipe, false);
   intel_enable_plane(dev_priv, plane, pipe);
 + intel_crtc_update_cursor(crtc, true);
   if (IS_G4X(dev))
   g4x_fixup_plane(dev_priv, pipe);
  

As discussed on IRC this may interfere with 

commit 61bc95c1fbbb6a08b55bbe161fdf1ea5493fc595
Author: Egbert Eich e...@suse.com
Date:   Mon Mar 4 09:24:38 2013 -0500

DRM/i915: On G45 enable cursor plane briefly after enabling the display 
plane.

described in https://bugs.freedesktop.org/show_bug.cgi?id=61457

I cannot test on the affected hardware as it's not available to
me at the moment. If Ville's G4x doesn't show the issue I will 
test this next time I have access to the HW in question.
Therefore:

Acked-by: Egbert Eich e...@suse.com

Cheers,
Egbert.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: force full modeset if the connector is in DPMS OFF mode

[Egbert Eich]

On Fri, May 03, 2013 at 02:22:09PM +0300, Imre Deak wrote:
 Currently the driver's assumed behavior for a modeset with an attached
 FB is that the corresponding connector will be switched to DPMS ON mode
 if it happened to be in DPMS OFF (or another power save mode). This
 wasn't enforced though if only the FB changed, everything else (format,
 connector etc.) remaining the same. In this case we only set the new FB
 base and left the connector in the old power save mode.
 
 Fix this by forcing a full modeset whenever there is an attached FB and
 any affected connector is in a power save mode.
 
 Signed-off-by: Imre Deak imre.d...@intel.com

[..]

 @@ -8397,8 +8412,12 @@ intel_set_config_compute_mode_changes(struct 
 drm_mode_set *set,
   } else if (set-fb-pixel_format !=
  set-crtc-fb-pixel_format) {
   config-mode_changed = true;
 - } else
 + } else if (crtc_connector_off(set-crtc, *set-connectors,
 +   set-num_connectors)) {
 + config-mode_changed = true;
 + } else {
   config-fb_changed = true;
 + }
   }
  
   if (set-fb  (set-x != set-crtc-x || set-y != set-crtc-y))

On https://bugs.freedesktop.org/show_bug.cgi?id=64178
I had a similar problem for which I found a very similar 'workaround':

Assuming the Xserver is displaying a mode a on output A, doing a:

xrandr --output A --off
xrandr --output A --mode a

will not light up the screen.

This all sounds quite similar, to the issues describe by this patch,
however the patch as is will not fix this issue: With this setup the
fb does not change. The crtc_connector_off() test however only runs if
set-crtc-fb != set-fb is true.

Thus the connector_off() test needs to happen before:

-   if (set-crtc-fb != set-fb) {
+   if (set-connectors != NULL  
+   connector_off(set-crtc, *set-connectors,
+ set-num_connectors)) {
+   config-mode_changed = true;
+   } else if (set-crtc-fb != set-fb) {

Mind the test for set-connectors != NULL as now the connectors list
may be empty.

Cheers,
Egbert.

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 1/7] drm/i915: Add HPD IRQ storm detection (v4)

[Egbert Eich]

Hi Jani,

On Thu, Apr 11, 2013 at 12:32:29PM +0300, Jani Nikula wrote:
 
 Hi Egbert -
 
 Up front, I haven't been following this series or read any of the
 previous review comments. Please bear with me, and feel free to direct
 me to earlier comments if I'm in contradiction.

Sorry for the late reply - last week I was quite busy with other stuff,
this week i'm a bit preoccupied.

 
 On Tue, 09 Apr 2013, Egbert Eich e...@freedesktop.org wrote:
  From: Egbert Eich e...@suse.de
 
  Add a hotplug IRQ storm detection (triggered when a hotplug interrupt
  fires more than 5 times / sec).
 
 Okay, this is theoretical, but a display port sink could do more than
 that many hpd irq requests when connected. Which leads me to wonder in
 general if the storm detection should be different for hot plug
 vs. unplug and hpd irq events.

Agreed. 
During my tests I did not see any issues with the statistics I've 
implemented: 5/sec was an ad-hoc choice and I wanted to start with 
something simple.
I've tested it and it seemed to work, so I didn't bother to look into
this more deeply, however if you feel 5 events  / sec are too few to
really do a good distinction, we could easily increase this number.

There have been two situations where I have seen 'interrupt storms':

1. On G35: some boxes of the affected systems do not show this issue 
   at all while others see a very high load but are still usable.
   In my recollection there were in the order of some 100 interrupts/sec
   on these machines. Then there were systems which would 'stall' in
   the worker during boot due to high load. At one point the NMI 
   watchdog kicked in and stopped this mess. There the interrupts
   happened at an order of magnitude if 10k!

2. A laptop with a Sandybridge chipset where the system load went
   high at certain stages of charging levels - when the power supply
   was connected. I would assume the frequency there also was around
   some 100 / sec.

Thus if we increase the threshold frequency to some 10 / sec we
would still cover all those cases.

Some other issue I've seen is 'bouncing' during manual plugging
I've been contemplating how to address this. There are two things 
to look at:
1. multiple hotplug events due to not getting a perfect connection at first
2. EDID readout happening to early when the EDID lines are not yet fully
   and 'permanently' connected.
It might well be, that a fix for these issues might actually also adress 
the issues you are pointing out.

I have not seen them on Intel hardware - but this may be due to the 
fact that the hardware I saw it on was a separate gfx card which did 
not have the usual mounting bracket and thus the entire setup was a 
bit fragile and not really suitable for hot-plugging.
However I believe that these things might happen everywere for people
not quite used to plugging monitors too much.

 
 Have you observed difference between hot plug/unplug?

There seems to be a difference between monitor connected/not connected:

On DVI (G35) one doesn't distinguish between plug/unplug: when the hotplug
line on the connector changes state an interrupt is sent. On this system
storms only happened when a monitor was conneted - since the state of the
HPD pin is signalled thru different frequencies on a line across SDVO (in 
my recollection it was 10 vs. 20 MHz) I believe that due to cross talk
the higher(?) frequency could not always reliably be measured.

I did not have access to the laptop system and the customer was not 
patient enough to help me to debug this further with me.

Generally I think we would still adress the 'strom condition' if we
raised the threshold to 20 or 30 /sec.

What do you think?

 
 Has this been a problem on PCH split platforms, i.e. since ilk/gen5?

I've also observed this on Sandybridge - which would be past GEN5, 
wouldn't it?

I will address some of the other issues mentioned in a new patch.

Thanks a lot for looking at it!

Cheers,
Egbert.

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 1/7] drm/i915: Add HPD IRQ storm detection (v4)

[Egbert Eich]

Hi Jani,

I've rebased and regenerated all the patches now, as there 
were some other bikesheds i had not adressed. I've also
included all Reviewed-By: 
This should make it easier to integrate the patches.

Some comments below:


On Thu, Apr 11, 2013 at 12:32:29PM +0300, Jani Nikula wrote:
  +   struct {
  +   unsigned long hpd_last_jiffies;
  +   int hpd_cnt;
  +   enum {
  +   HPD_ENABLED = 0,
  +   HPD_DISABLED = 1,
  +   HPD_MARK_DISABLED = 2
  +   } hpd_mark;
  +   } hpd_stats[HPD_NUM_PINS];
 
 With all the hotplug stuff being added, I think it's getting time to
 group all the hotplug stuff under a struct.

I will postpone this until I address the issues that I have on my TODO.

 
  int num_pch_pll;
  int num_plane;
  diff --git a/drivers/gpu/drm/i915/i915_irq.c 
  b/drivers/gpu/drm/i915/i915_irq.c
  index 4c5bdd0..32b5527 100644
  --- a/drivers/gpu/drm/i915/i915_irq.c
  +++ b/drivers/gpu/drm/i915/i915_irq.c
  @@ -582,6 +582,37 @@ static void gen6_queue_rps_work(struct 
  drm_i915_private *dev_priv,
  queue_work(dev_priv-wq, dev_priv-rps.work);
   }
   
  +#define HPD_STORM_DETECT_PERIOD 1000
  +
  +static inline void hotplug_irq_storm_detect(struct drm_device *dev,
  +   u32 hotplug_trigger,
  +   const u32 *hpd)
 
 I think you should just add the bool return status right here instead of
 postponing until the later patch that needs it.

I thought about this and left it as it is: Returning a bool status now
will raise other questions as the logic in this patch doesn't require it.
I'd rather have a bigger patch later which will however clearly explains
the reason for the change to the function.

 
  +{
  +   drm_i915_private_t *dev_priv = dev-dev_private;
  +   unsigned long irqflags;
  +   int i;
  +
  +   spin_lock_irqsave(dev_priv-irq_lock, irqflags);
  +
  +   for (i = 1; i  HPD_NUM_PINS; i++) {
  +   if ((hpd[i]  hotplug_trigger) 
  +   dev_priv-hpd_stats[i].hpd_mark == HPD_ENABLED) {
 
 Bikeshed: You might get a nicer layout below by negating that if and
 adding continue.

Fixed.

 
  +   if (!time_in_range(jiffies, 
  dev_priv-hpd_stats[i].hpd_last_jiffies,
  + 
  dev_priv-hpd_stats[i].hpd_last_jiffies
  + + 
  msecs_to_jiffies(HPD_STORM_DETECT_PERIOD))) {
  +   dev_priv-hpd_stats[i].hpd_last_jiffies = 
  jiffies;
  +   dev_priv-hpd_stats[i].hpd_cnt = 0;
  +   } else if (dev_priv-hpd_stats[i].hpd_cnt  5) {
  +   dev_priv-hpd_stats[i].hpd_mark = 
  HPD_MARK_DISABLED;
  +   DRM_DEBUG_KMS(HPD interrupt storm detected on  
  PIN %d\n, i);
 
 Extra space before PIN.

Fixed.
 
  +   } else
  +   dev_priv-hpd_stats[i].hpd_cnt++;
 
 If one branch requires braces, then all do.

Fixed.

Cheers,
Egbert.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 1/7] drm/i915: Add HPD IRQ storm detection (v5)

[Egbert Eich]

Add a hotplug IRQ storm detection (triggered when a hotplug interrupt
fires more than 5 times / sec).
Rationale:
Despite of the many attempts to fix the problem with noisy hotplug
interrupt lines we are still seeing systems which have issues:
Once cause of noise seems to be bad routing of the hotplug line
on the board: cross talk from other signals seems to cause erronous
hotplug interrupts. This has been documented as an erratum for the
the i945GM chipset and thus hotplug support was disabled for this
chipset model but others seem to have this problem, too.

We have seen this issue on a G35 motherboard for example:
Even different motherboards of the same model seem to behave
differently: while some only see only around 10-100 interrupts/s
others seem to see 5k or more.
We've also observed a dependency on the selected video mode.

Also on certain laptops interrupt noise seems to occur duing
battery charging when the battery is at a certain charge levels.

Thus we add a simple algorithm here that detects an 'interrupt storm'
condition.

v2: Fixed comment.
v3: Reordered drm_i915_private: moved hpd state tracking to hotplug work stuff.
v4: Followed by Jesse Barnes to use a time_..() macro.
v5: Fixed coding style as suggested by Jani Nikula.

Signed-off-by: Egbert Eich e...@suse.de
Acked-by: Chris Wilson ch...@chris-wilson.co.uk
Reviewed-by: Jesse Barnes jbar...@virtuousgeek.org
---
 drivers/gpu/drm/i915/i915_drv.h |  9 ++
 drivers/gpu/drm/i915/i915_irq.c | 69 ++---
 2 files changed, 66 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 44fca0b..83fc1a6 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -929,6 +929,15 @@ typedef struct drm_i915_private {
 
struct work_struct hotplug_work;
bool enable_hotplug_processing;
+   struct {
+   unsigned long hpd_last_jiffies;
+   int hpd_cnt;
+   enum {
+   HPD_ENABLED = 0,
+   HPD_DISABLED = 1,
+   HPD_MARK_DISABLED = 2
+   } hpd_mark;
+   } hpd_stats[HPD_NUM_PINS];
 
int num_pch_pll;
int num_plane;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 4c5bdd0..71fc238 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -582,6 +582,40 @@ static void gen6_queue_rps_work(struct drm_i915_private 
*dev_priv,
queue_work(dev_priv-wq, dev_priv-rps.work);
 }
 
+#define HPD_STORM_DETECT_PERIOD 1000
+#define HPD_STORM_THRESHOLD 5
+
+static inline void hotplug_irq_storm_detect(struct drm_device *dev,
+   u32 hotplug_trigger,
+   const u32 *hpd)
+{
+   drm_i915_private_t *dev_priv = dev-dev_private;
+   unsigned long irqflags;
+   int i;
+
+   spin_lock_irqsave(dev_priv-irq_lock, irqflags);
+
+   for (i = 1; i  HPD_NUM_PINS; i++) {
+   if (!(hpd[i]  hotplug_trigger) ||
+   dev_priv-hpd_stats[i].hpd_mark != HPD_ENABLED)
+   continue;
+
+   if (!time_in_range(jiffies, 
dev_priv-hpd_stats[i].hpd_last_jiffies,
+  dev_priv-hpd_stats[i].hpd_last_jiffies
+  + 
msecs_to_jiffies(HPD_STORM_DETECT_PERIOD))) {
+   dev_priv-hpd_stats[i].hpd_last_jiffies = jiffies;
+   dev_priv-hpd_stats[i].hpd_cnt = 0;
+   } else if (dev_priv-hpd_stats[i].hpd_cnt  
HPD_STORM_THRESHOLD) {
+   dev_priv-hpd_stats[i].hpd_mark = HPD_MARK_DISABLED;
+   DRM_DEBUG_KMS(HPD interrupt storm detected on PIN 
%d\n, i);
+   } else {
+   dev_priv-hpd_stats[i].hpd_cnt++;
+   }
+   }
+
+   spin_unlock_irqrestore(dev_priv-irq_lock, irqflags);
+}
+
 static void gmbus_irq_handler(struct drm_device *dev)
 {
struct drm_i915_private *dev_priv = (drm_i915_private_t *) 
dev-dev_private;
@@ -650,13 +684,15 @@ static irqreturn_t valleyview_irq_handler(int irq, void 
*arg)
/* Consume port.  Then clear IIR or we'll miss events */
if (iir  I915_DISPLAY_PORT_INTERRUPT) {
u32 hotplug_status = I915_READ(PORT_HOTPLUG_STAT);
+   u32 hotplug_trigger = hotplug_status  
HOTPLUG_INT_STATUS_I915;
 
DRM_DEBUG_DRIVER(hotplug event received, stat 
0x%08x\n,
 hotplug_status);
-   if (hotplug_status  HOTPLUG_INT_STATUS_I915)
+   if (hotplug_trigger) {
+   hotplug_irq_storm_detect(dev, hotplug_trigger, 
hpd_status_i915);
queue_work(dev_priv-wq

[Intel-gfx] [PATCH 2/7] drm/i915: (re)init HPD interrupt storm statistics

[Egbert Eich]

When an encoder is shared on several connectors there is only
one hotplug line, thus this line needs to be shared among these
connectors.
If HPD detect only works reliably on a subset of those connectors,
we want to poll the others. Thus we need to make sure that storm
detection doesn't mess up the settings for those connectors.
Therefore we store the settings in the intel_connector struct and
restore them from there.
If nothing is set but the encoder has a hpd_pin set we assume this
connector is hotplug capable.
On init/reset we make sure the polled state of the connectors
is (re)set to the default value, the HPD interrupts are marked
enabled.

Signed-off-by: Egbert Eich e...@suse.de
Reviewed-by: Jani Nikula jani.nik...@intel.com
---
 drivers/gpu/drm/i915/i915_irq.c   | 14 ++
 drivers/gpu/drm/i915/intel_crt.c  |  6 ++
 drivers/gpu/drm/i915/intel_dp.c   |  1 -
 drivers/gpu/drm/i915/intel_drv.h  |  4 
 drivers/gpu/drm/i915/intel_hdmi.c |  1 -
 drivers/gpu/drm/i915/intel_sdvo.c |  5 ++---
 drivers/gpu/drm/i915/intel_tv.c   |  2 +-
 7 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 71fc238..bc00532 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -596,6 +596,7 @@ static inline void hotplug_irq_storm_detect(struct 
drm_device *dev,
spin_lock_irqsave(dev_priv-irq_lock, irqflags);
 
for (i = 1; i  HPD_NUM_PINS; i++) {
+
if (!(hpd[i]  hotplug_trigger) ||
dev_priv-hpd_stats[i].hpd_mark != HPD_ENABLED)
continue;
@@ -3048,7 +3049,20 @@ void intel_irq_init(struct drm_device *dev)
 void intel_hpd_init(struct drm_device *dev)
 {
struct drm_i915_private *dev_priv = dev-dev_private;
+   struct drm_mode_config *mode_config = dev-mode_config;
+   struct drm_connector *connector;
+   int i;
 
+   for (i = 1; i  HPD_NUM_PINS; i++) {
+   dev_priv-hpd_stats[i].hpd_cnt = 0;
+   dev_priv-hpd_stats[i].hpd_mark = HPD_ENABLED;
+   }
+   list_for_each_entry(connector, mode_config-connector_list, head) {
+   struct intel_connector *intel_connector = 
to_intel_connector(connector);
+   connector-polled = intel_connector-polled;
+   if (!connector-polled  I915_HAS_HOTPLUG(dev)  
intel_connector-encoder-hpd_pin  HPD_NONE)
+   connector-polled = DRM_CONNECTOR_POLL_HPD;
+   }
if (dev_priv-display.hpd_irq_setup)
dev_priv-display.hpd_irq_setup(dev);
 }
diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 1ae2d7f..c063b9f 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -793,10 +793,8 @@ void intel_crt_init(struct drm_device *dev)
 
drm_sysfs_connector_add(connector);
 
-   if (I915_HAS_HOTPLUG(dev))
-   connector-polled = DRM_CONNECTOR_POLL_HPD;
-   else
-   connector-polled = DRM_CONNECTOR_POLL_CONNECT;
+   if (!I915_HAS_HOTPLUG(dev))
+   intel_connector-polled = DRM_CONNECTOR_POLL_CONNECT;
 
/*
 * Configure the automatic hotplug detection stuff
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 482b5e5..1e9b19a 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2786,7 +2786,6 @@ intel_dp_init_connector(struct intel_digital_port 
*intel_dig_port,
drm_connector_init(dev, connector, intel_dp_connector_funcs, type);
drm_connector_helper_add(connector, intel_dp_connector_helper_funcs);
 
-   connector-polled = DRM_CONNECTOR_POLL_HPD;
connector-interlace_allowed = true;
connector-doublescan_allowed = 0;
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d7bd031..a05fde7 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -171,6 +171,10 @@ struct intel_connector {
 
/* Cached EDID for eDP and LVDS. May hold ERR_PTR for invalid EDID. */
struct edid *edid;
+
+   /* since POLL and HPD connectors may use the same HPD line keep the 
native
+  state of connector-polled in case hotplug storm detection changes 
it */
+   u8 polled;
 };
 
 struct intel_crtc_config {
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c 
b/drivers/gpu/drm/i915/intel_hdmi.c
index ee4a8da..8912201 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -998,7 +998,6 @@ void intel_hdmi_init_connector(struct intel_digital_port 
*intel_dig_port,
   DRM_MODE_CONNECTOR_HDMIA);
drm_connector_helper_add(connector, intel_hdmi_connector_helper_funcs);
 
-   connector-polled = DRM_CONNECTOR_POLL_HPD;
connector-interlace_allowed = 1;
connector-doublescan_allowed = 0;
 
diff --git a/drivers/gpu/drm/i915

[Intel-gfx] [PATCH 3/7] drm/i915: Mask out the HPD irq bits before setting them individually.

[Egbert Eich]

To disable previously enabled HPD IRQs we need to reset them and
set the enabled ones individually.

Signed-off-by: Egbert Eich e...@suse.de
Reviewed-by: Jani Nikula jani.nik...@intel.com
---
 drivers/gpu/drm/i915/i915_irq.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index bc00532..4cf33b3 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2121,9 +2121,11 @@ static void ibx_hpd_irq_setup(struct drm_device *dev)
u32 hotplug;
 
if (HAS_PCH_IBX(dev)) {
+   mask = ~SDE_HOTPLUG_MASK;
list_for_each_entry(intel_encoder, mode_config-encoder_list, 
base.head)
mask |= hpd_ibx[intel_encoder-hpd_pin];
} else {
+   mask = ~SDE_HOTPLUG_MASK_CPT;
list_for_each_entry(intel_encoder, mode_config-encoder_list, 
base.head)
mask |= hpd_cpt[intel_encoder-hpd_pin];
}
-- 
1.8.1.4

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 4/7] drm/i915: Disable HPD interrupt on pin when irq storm is detected (v3)

[Egbert Eich]

This patch disables hotplug interrupts if an 'interrupt storm'
has been detected.
Noise on the interrupt line renders the hotplug interrupt useless:
each hotplug event causes the devices to be rescanned which will
will only increase the system load.
Thus disable the hotplug interrupts and fall back to periodic
device polling.

v2: Fixed cleanup typo.
v3: Fixed format issues, clarified a variable name,
changed pr_warn() to DRM_INFO() as suggested by
Jani Nikula jani.nik...@linux.intel.com.

Signed-off-by: Egbert Eich e...@suse.de
Reviewed-by: Jani Nikula jani.nik...@intel.com
---
 drivers/gpu/drm/i915/i915_irq.c | 75 +++--
 1 file changed, 58 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 4cf33b3..d059c5d 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -88,7 +88,8 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview 
are the same */
[HPD_PORT_D] = PORTD_HOTPLUG_INT_STATUS
 };
 
-
+static void ibx_hpd_irq_setup(struct drm_device *dev);
+static void i915_hpd_irq_setup(struct drm_device *dev);
 
 /* For display hotplug interrupt */
 static void
@@ -342,7 +343,11 @@ static void i915_hotplug_work_func(struct work_struct 
*work)
hotplug_work);
struct drm_device *dev = dev_priv-dev;
struct drm_mode_config *mode_config = dev-mode_config;
-   struct intel_encoder *encoder;
+   struct intel_connector *intel_connector;
+   struct intel_encoder *intel_encoder;
+   struct drm_connector *connector;
+   unsigned long irqflags;
+   bool hpd_disabled = false;
 
/* HPD irq before everything is fully set up. */
if (!dev_priv-enable_hotplug_processing)
@@ -351,9 +356,33 @@ static void i915_hotplug_work_func(struct work_struct 
*work)
mutex_lock(mode_config-mutex);
DRM_DEBUG_KMS(running encoder hotplug functions\n);
 
-   list_for_each_entry(encoder, mode_config-encoder_list, base.head)
-   if (encoder-hot_plug)
-   encoder-hot_plug(encoder);
+   spin_lock_irqsave(dev_priv-irq_lock, irqflags);
+   list_for_each_entry(connector, mode_config-connector_list, head) {
+   intel_connector = to_intel_connector(connector);
+   intel_encoder = intel_connector-encoder;
+   if (intel_encoder-hpd_pin  HPD_NONE 
+   dev_priv-hpd_stats[intel_encoder-hpd_pin].hpd_mark == 
HPD_MARK_DISABLED 
+   connector-polled == DRM_CONNECTOR_POLL_HPD) {
+   DRM_INFO(HPD interrupt storm detected on connector %s: 

+switching from hotplug detection to 
polling\n,
+   drm_get_connector_name(connector));
+   dev_priv-hpd_stats[intel_encoder-hpd_pin].hpd_mark = 
HPD_DISABLED;
+   connector-polled = DRM_CONNECTOR_POLL_CONNECT
+   | DRM_CONNECTOR_POLL_DISCONNECT;
+   hpd_disabled = true;
+   }
+   }
+/* if there were no outputs to poll, poll was disabled,
+ * therefore make sure it's enabled when disabling HPD on
+ * some connectors */
+   if (hpd_disabled)
+   drm_kms_helper_poll_enable(dev);
+
+   spin_unlock_irqrestore(dev_priv-irq_lock, irqflags);
+
+   list_for_each_entry(intel_encoder, mode_config-encoder_list, 
base.head)
+   if (intel_encoder-hot_plug)
+   intel_encoder-hot_plug(intel_encoder);
 
mutex_unlock(mode_config-mutex);
 
@@ -585,13 +614,14 @@ static void gen6_queue_rps_work(struct drm_i915_private 
*dev_priv,
 #define HPD_STORM_DETECT_PERIOD 1000
 #define HPD_STORM_THRESHOLD 5
 
-static inline void hotplug_irq_storm_detect(struct drm_device *dev,
+static inline bool hotplug_irq_storm_detect(struct drm_device *dev,
u32 hotplug_trigger,
const u32 *hpd)
 {
drm_i915_private_t *dev_priv = dev-dev_private;
unsigned long irqflags;
int i;
+   bool ret = false;
 
spin_lock_irqsave(dev_priv-irq_lock, irqflags);
 
@@ -609,12 +639,15 @@ static inline void hotplug_irq_storm_detect(struct 
drm_device *dev,
} else if (dev_priv-hpd_stats[i].hpd_cnt  
HPD_STORM_THRESHOLD) {
dev_priv-hpd_stats[i].hpd_mark = HPD_MARK_DISABLED;
DRM_DEBUG_KMS(HPD interrupt storm detected on PIN 
%d\n, i);
+   ret = true;
} else {
dev_priv-hpd_stats[i].hpd_cnt++;
}
}
 
spin_unlock_irqrestore(dev_priv-irq_lock, irqflags);
+
+   return ret;
 }
 
 static void gmbus_irq_handler(struct drm_device *dev)
@@ -690,7 +723,8 @@ static irqreturn_t

[Intel-gfx] [PATCH 5/7] drm/i915: Add Reenable Timer to turn Hotplug Detection back on (v4)

[Egbert Eich]

We disable hoptplug detection when we encounter a hotplug event
storm. Still hotplug detection is required on some outputs (like
Display Port). The interrupt storm may be only temporary (on certain
Dell Laptops for instance it happens at certain charging states of
the system). Thus we enable it after a certain grace period (2 minutes).
Should the interrupt storm persist it will be detected immediately
and it will be disabled again.

v2: Reordered drm_i915_private: moved hotplug_reenable_timer to hpd state 
tracker.
v3: Clarified loop start value,
Removed superfluous test for Ivybridge and Haswell,
Restructured loop to avoid deep nesting (all suggested by Ville Syrjälä)
v4: Fixed two bugs pointed out by Jani Nikula.

Signed-off-by: Egbert Eich e...@suse.de
Reviewed-by: Jani Nikula jani.nik...@intel.com
---
 drivers/gpu/drm/i915/i915_drv.h |  1 +
 drivers/gpu/drm/i915/i915_irq.c | 52 -
 2 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 83fc1a6..195b9fe 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -938,6 +938,7 @@ typedef struct drm_i915_private {
HPD_MARK_DISABLED = 2
} hpd_mark;
} hpd_stats[HPD_NUM_PINS];
+   struct timer_list hotplug_reenable_timer;
 
int num_pch_pll;
int num_plane;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index d059c5d..ae759ac 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -337,6 +337,8 @@ static int i915_get_vblank_timestamp(struct drm_device 
*dev, int pipe,
 /*
  * Handle hotplug events outside the interrupt handler proper.
  */
+#define I915_REENABLE_HOTPLUG_DELAY (2*60*1000)
+
 static void i915_hotplug_work_func(struct work_struct *work)
 {
drm_i915_private_t *dev_priv = container_of(work, drm_i915_private_t,
@@ -375,8 +377,11 @@ static void i915_hotplug_work_func(struct work_struct 
*work)
 /* if there were no outputs to poll, poll was disabled,
  * therefore make sure it's enabled when disabling HPD on
  * some connectors */
-   if (hpd_disabled)
+   if (hpd_disabled) {
drm_kms_helper_poll_enable(dev);
+   mod_timer(dev_priv-hotplug_reenable_timer,
+ jiffies + 
msecs_to_jiffies(I915_REENABLE_HOTPLUG_DELAY));
+   }
 
spin_unlock_irqrestore(dev_priv-irq_lock, irqflags);
 
@@ -2356,6 +2361,8 @@ static void valleyview_irq_uninstall(struct drm_device 
*dev)
if (!dev_priv)
return;
 
+   del_timer_sync(dev_priv-hotplug_reenable_timer);
+
for_each_pipe(pipe)
I915_WRITE(PIPESTAT(pipe), 0x);
 
@@ -2377,6 +2384,8 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
if (!dev_priv)
return;
 
+   del_timer_sync(dev_priv-hotplug_reenable_timer);
+
I915_WRITE(HWSTAM, 0x);
 
I915_WRITE(DEIMR, 0x);
@@ -2753,6 +2762,8 @@ static void i915_irq_uninstall(struct drm_device * dev)
drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev-dev_private;
int pipe;
 
+   del_timer_sync(dev_priv-hotplug_reenable_timer);
+
if (I915_HAS_HOTPLUG(dev)) {
I915_WRITE(PORT_HOTPLUG_EN, 0);
I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
@@ -2997,6 +3008,8 @@ static void i965_irq_uninstall(struct drm_device * dev)
if (!dev_priv)
return;
 
+   del_timer_sync(dev_priv-hotplug_reenable_timer);
+
I915_WRITE(PORT_HOTPLUG_EN, 0);
I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
 
@@ -3012,6 +3025,41 @@ static void i965_irq_uninstall(struct drm_device * dev)
I915_WRITE(IIR, I915_READ(IIR));
 }
 
+static void i915_reenable_hotplug_timer_func(unsigned long data)
+{
+   drm_i915_private_t *dev_priv = (drm_i915_private_t *)data;
+   struct drm_device *dev = dev_priv-dev;
+   struct drm_mode_config *mode_config = dev-mode_config;
+   unsigned long irqflags;
+   int i;
+
+   spin_lock_irqsave(dev_priv-irq_lock, irqflags);
+   for (i = (HPD_NONE + 1); i  HPD_NUM_PINS; i++) {
+   struct drm_connector *connector;
+
+   if (dev_priv-hpd_stats[i].hpd_mark != HPD_DISABLED)
+   continue;
+
+   dev_priv-hpd_stats[i].hpd_mark = HPD_ENABLED;
+
+   list_for_each_entry(connector, mode_config-connector_list, 
head) {
+   struct intel_connector *intel_connector = 
to_intel_connector(connector);
+
+   if (intel_connector-encoder-hpd_pin == i) {
+   if (connector-polled != 
intel_connector-polled)
+   DRM_DEBUG_DRIVER(Reenabling HPD on 
connector %s\n

[Intel-gfx] [PATCH 6/7] drm/i915: Add bit field to record which pins have received HPD events (v3)

[Egbert Eich]

This allows to add code which limits 're'-detect() of displays to connectors
that have received an HPD event.

v2: Reordered drm_i915_private: Move hpd_event_bits to hpd state tracking.
v3: Fixed merge conflicts with previous patches, removed some noisy debug
logging as suggested by Jani Nikula.

Signed-off-by: Egbert Eich e...@suse.de
Reviewed-by: Jani Nikula jani.nik...@intel.com
---
 drivers/gpu/drm/i915/i915_drv.h |  1 +
 drivers/gpu/drm/i915/i915_irq.c | 11 +++
 2 files changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 195b9fe..c0d9fb9 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -938,6 +938,7 @@ typedef struct drm_i915_private {
HPD_MARK_DISABLED = 2
} hpd_mark;
} hpd_stats[HPD_NUM_PINS];
+   u32 hpd_event_bits;
struct timer_list hotplug_reenable_timer;
 
int num_pch_pll;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index ae759ac..e108729 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -350,6 +350,7 @@ static void i915_hotplug_work_func(struct work_struct *work)
struct drm_connector *connector;
unsigned long irqflags;
bool hpd_disabled = false;
+   u32 hpd_event_bits;
 
/* HPD irq before everything is fully set up. */
if (!dev_priv-enable_hotplug_processing)
@@ -359,6 +360,9 @@ static void i915_hotplug_work_func(struct work_struct *work)
DRM_DEBUG_KMS(running encoder hotplug functions\n);
 
spin_lock_irqsave(dev_priv-irq_lock, irqflags);
+
+   hpd_event_bits = dev_priv-hpd_event_bits;
+   dev_priv-hpd_event_bits = 0;
list_for_each_entry(connector, mode_config-connector_list, head) {
intel_connector = to_intel_connector(connector);
intel_encoder = intel_connector-encoder;
@@ -373,6 +377,10 @@ static void i915_hotplug_work_func(struct work_struct 
*work)
| DRM_CONNECTOR_POLL_DISCONNECT;
hpd_disabled = true;
}
+   if (hpd_event_bits  (1  intel_encoder-hpd_pin)) {
+   DRM_DEBUG_KMS(Connector %s (pin %i) received hotplug 
event.\n,
+ drm_get_connector_name(connector), 
intel_encoder-hpd_pin);
+   }
}
 /* if there were no outputs to poll, poll was disabled,
  * therefore make sure it's enabled when disabling HPD on
@@ -636,6 +644,8 @@ static inline bool hotplug_irq_storm_detect(struct 
drm_device *dev,
dev_priv-hpd_stats[i].hpd_mark != HPD_ENABLED)
continue;
 
+   dev_priv-hpd_event_bits |= (1  i);
+
if (!time_in_range(jiffies, 
dev_priv-hpd_stats[i].hpd_last_jiffies,
   dev_priv-hpd_stats[i].hpd_last_jiffies
   + 
msecs_to_jiffies(HPD_STORM_DETECT_PERIOD))) {
@@ -643,6 +653,7 @@ static inline bool hotplug_irq_storm_detect(struct 
drm_device *dev,
dev_priv-hpd_stats[i].hpd_cnt = 0;
} else if (dev_priv-hpd_stats[i].hpd_cnt  
HPD_STORM_THRESHOLD) {
dev_priv-hpd_stats[i].hpd_mark = HPD_MARK_DISABLED;
+   dev_priv-hpd_event_bits = ~(1  i);
DRM_DEBUG_KMS(HPD interrupt storm detected on PIN 
%d\n, i);
ret = true;
} else {
-- 
1.8.1.4

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 7/7] drm/i915: Only reprobe display on encoder which has received an HPD event (v2)

[Egbert Eich]

Instead of calling into the DRM helper layer to poll all connectors for
changes in connected displays probe only those connectors which have
received a hotplug event.

Signed-off-by: Egbert Eich e...@suse.de
Reviewed-by: Jani Nikula jani.nik...@intel.com

v2: Resolved conflicts with changes in previous commits.
Renamed function and and added a WARN_ON() to warn of
intel_hpd_irq_event() from being called without
mode_config.mutex held - suggested by Jani Nikula.
---
 drivers/gpu/drm/i915/i915_irq.c | 34 --
 1 file changed, 28 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index e108729..dd579d7 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -334,6 +334,21 @@ static int i915_get_vblank_timestamp(struct drm_device 
*dev, int pipe,
 crtc);
 }
 
+static int intel_hpd_irq_event(struct drm_device *dev, struct drm_connector 
*connector)
+{
+   enum drm_connector_status old_status;
+
+   WARN_ON(!mutex_is_locked(dev-mode_config.mutex));
+   old_status = connector-status;
+
+   connector-status = connector-funcs-detect(connector, false);
+   DRM_DEBUG_KMS([CONNECTOR:%d:%s] status updated from %d to %d\n,
+ connector-base.id,
+ drm_get_connector_name(connector),
+ old_status, connector-status);
+   return (old_status != connector-status);
+}
+
 /*
  * Handle hotplug events outside the interrupt handler proper.
  */
@@ -350,6 +365,7 @@ static void i915_hotplug_work_func(struct work_struct *work)
struct drm_connector *connector;
unsigned long irqflags;
bool hpd_disabled = false;
+   bool changed = false;
u32 hpd_event_bits;
 
/* HPD irq before everything is fully set up. */
@@ -393,14 +409,20 @@ static void i915_hotplug_work_func(struct work_struct 
*work)
 
spin_unlock_irqrestore(dev_priv-irq_lock, irqflags);
 
-   list_for_each_entry(intel_encoder, mode_config-encoder_list, 
base.head)
-   if (intel_encoder-hot_plug)
-   intel_encoder-hot_plug(intel_encoder);
-
+   list_for_each_entry(connector, mode_config-connector_list, head) {
+   intel_connector = to_intel_connector(connector);
+   intel_encoder = intel_connector-encoder;
+   if (hpd_event_bits  (1  intel_encoder-hpd_pin)) {
+   if (intel_encoder-hot_plug)
+   intel_encoder-hot_plug(intel_encoder);
+   if (intel_hpd_irq_event(dev, connector))
+   changed = true;
+   }
+   }
mutex_unlock(mode_config-mutex);
 
-   /* Just fire off a uevent and let userspace tell us what to do */
-   drm_helper_hpd_irq_event(dev);
+   if (changed)
+   drm_kms_helper_hotplug_event(dev);
 }
 
 static void ironlake_handle_rps_change(struct drm_device *dev)
-- 
1.8.1.4

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 5/7] drm/i915: Add Reenable Timer to turn Hotplug Detection back on (v4)

[Egbert Eich]

On Tue, Apr 16, 2013 at 08:07:09PM +0200, Daniel Vetter wrote:
 On Tue, Apr 16, 2013 at 01:36:58PM +0200, Egbert Eich wrote:
  We disable hoptplug detection when we encounter a hotplug event
  storm. Still hotplug detection is required on some outputs (like
  Display Port). The interrupt storm may be only temporary (on certain
  Dell Laptops for instance it happens at certain charging states of
  the system). Thus we enable it after a certain grace period (2 minutes).
  Should the interrupt storm persist it will be detected immediately
  and it will be disabled again.
  
  v2: Reordered drm_i915_private: moved hotplug_reenable_timer to hpd state 
  tracker.
  v3: Clarified loop start value,
  Removed superfluous test for Ivybridge and Haswell,
  Restructured loop to avoid deep nesting (all suggested by Ville Syrjälä)
  v4: Fixed two bugs pointed out by Jani Nikula.
  
  Signed-off-by: Egbert Eich e...@suse.de
  Reviewed-by: Jani Nikula jani.nik...@intel.com
 
 I've queued up patches 1-5 of this series, thanks a lot for doing all
 this. Wrt bikesheds: checkpatch seemed a bit unhappy about some of them,
 but I've decided that I want this more for 3.10 than care about checkpatch
 ;-)

I've run checkpatch on every patch - here it only complains about lines longer 
than 80 characters.
Now - if you can prove that my file introduced lines with more than 80 
characters
to a file that had no such lines already we can talk about fixing this ;p

Cheers,
Egbert.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 5/7] drm/i915: Add Reenable Timer to turn Hotplug Detection back on (v3)

[Egbert Eich]

On Thu, Apr 11, 2013 at 01:44:51PM +0300, Jani Nikula wrote:
  +
  +   spin_lock_irqsave(dev_priv-irq_lock, irqflags);
  +   for (i = (HPD_NONE + 1); i  HPD_NUM_PINS; i++) {
  +   struct drm_connector *connector;
  +
  +   if (dev_priv-hpd_stats[i].hpd_mark != HPD_MARK_DISABLED)
 
 I think this is wrong, skipping HPD_DISABLED.

Right, this was indeed wrong.

 
  +   continue;
  +
  +   dev_priv-hpd_stats[i].hpd_mark = HPD_ENABLED;
  +
  +   list_for_each_entry(connector, mode_config-connector_list, 
  head) {
  +   struct intel_connector *intel_connector = 
  to_intel_connector(connector);
  +
  +   if (intel_connector-encoder-hpd_pin == i) {
  +   if (connector-polled != 
  intel_connector-polled)
  +   DRM_DEBUG_DRIVER(Reenabling HPD on 
  connector %s\n,
  +
  drm_get_connector_name(connector));
  +   connector-polled = intel_connector-polled;
  +   if (!connector-polled)
  +   connector-polled = 
  DRM_CONNECTOR_POLL_HPD;
  +   }
  +   }
  +   dev_priv-display.hpd_irq_setup(dev);
 
 You don't need to call this at each iteration, do you?

Right, you are right here as well.
 
  +   }
 
 In fact, couldn't you just call intel_hpd_init(), or modify it to do
 what you want? Keep it simple.

Well, intel_hpd_init() is initializing every to dev_priv-hpd_stats[i].hpd_mark
to HPD_ENABLED. Can we rule out that the timer runs between the interrupt
handler and the worker - as in this state this variable might me set to
HPD_MARK_DISABLED?
In fact it would probably not even hurt if we changed the HPD_MARK_DISABLED
to HPD_ENABLED as in a storm condition this condition will soon reappear but
I'd rather avoid it.
Of course we could pass an argument to the function to distinguish both
conditions. This is a simplification which can still be introduced - when
I'm in fact able to do some testing.

Thanks a lot for the review!

Cheers,
Egbert.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v3] drm/i915: Disable HPD interrupt on pin when irq storm is detected (v3)

[Egbert Eich]

This patch disables hotplug interrupts if an 'interrupt storm'
has been detected.
Noise on the interrupt line renders the hotplug interrupt useless:
each hotplug event causes the devices to be rescanned which will
will only increase the system load.
Thus disable the hotplug interrupts and fall back to periodic
device polling.

v2: Fixed cleanup typo.
v3: Fixed format issues, clarified a variable name,
changed pr_warn() to DRM_INFO() as suggested by
Jani Nikula jani.nik...@linux.intel.com.

Signed-off-by: Egbert Eich e...@suse.de
---
 drivers/gpu/drm/i915/i915_irq.c | 75 +++--
 1 file changed, 58 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index a3f1ac4..834c717 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -88,7 +88,8 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview 
are the same */
[HPD_PORT_D] = PORTD_HOTPLUG_INT_STATUS
 };
 
-
+static void ibx_hpd_irq_setup(struct drm_device *dev);
+static void i915_hpd_irq_setup(struct drm_device *dev);
 
 /* For display hotplug interrupt */
 static void
@@ -342,7 +343,11 @@ static void i915_hotplug_work_func(struct work_struct 
*work)
hotplug_work);
struct drm_device *dev = dev_priv-dev;
struct drm_mode_config *mode_config = dev-mode_config;
-   struct intel_encoder *encoder;
+   struct intel_connector *intel_connector;
+   struct intel_encoder *intel_encoder;
+   struct drm_connector *connector;
+   unsigned long irqflags;
+   bool hpd_disabled = false;
 
/* HPD irq before everything is fully set up. */
if (!dev_priv-enable_hotplug_processing)
@@ -351,9 +356,33 @@ static void i915_hotplug_work_func(struct work_struct 
*work)
mutex_lock(mode_config-mutex);
DRM_DEBUG_KMS(running encoder hotplug functions\n);
 
-   list_for_each_entry(encoder, mode_config-encoder_list, base.head)
-   if (encoder-hot_plug)
-   encoder-hot_plug(encoder);
+   spin_lock_irqsave(dev_priv-irq_lock, irqflags);
+   list_for_each_entry(connector, mode_config-connector_list, head) {
+   intel_connector = to_intel_connector(connector);
+   intel_encoder = intel_connector-encoder;
+   if (intel_encoder-hpd_pin  HPD_NONE 
+   dev_priv-hpd_stats[intel_encoder-hpd_pin].hpd_mark == 
HPD_MARK_DISABLED 
+   connector-polled == DRM_CONNECTOR_POLL_HPD) {
+   DRM_INFO(HPD interrupt storm detected on connector %s: 

+switching from hotplug detection to 
polling\n,
+   drm_get_connector_name(connector));
+   dev_priv-hpd_stats[intel_encoder-hpd_pin].hpd_mark = 
HPD_DISABLED;
+   connector-polled = DRM_CONNECTOR_POLL_CONNECT
+   | DRM_CONNECTOR_POLL_DISCONNECT;
+   hpd_disabled = true;
+   }
+   }
+/* if there were no outputs to poll, poll was disabled,
+ * therefore make sure it's enabled when disabling HPD on
+ * some connectors */
+   if (hpd_disabled) {
+   drm_kms_helper_poll_enable(dev);
+   }
+   spin_unlock_irqrestore(dev_priv-irq_lock, irqflags);
+
+   list_for_each_entry(intel_encoder, mode_config-encoder_list, 
base.head)
+   if (intel_encoder-hot_plug)
+   intel_encoder-hot_plug(intel_encoder);
 
mutex_unlock(mode_config-mutex);
 
@@ -584,13 +613,14 @@ static void gen6_queue_rps_work(struct drm_i915_private 
*dev_priv,
 
 #define HPD_STORM_DETECT_PERIOD 1000
 
-static inline void hotplug_irq_storm_detect(struct drm_device *dev,
+static inline bool hotplug_irq_storm_detect(struct drm_device *dev,
u32 hotplug_trigger,
const u32 *hpd)
 {
drm_i915_private_t *dev_priv = dev-dev_private;
unsigned long irqflags;
int i;
+   bool ret = false;
 
spin_lock_irqsave(dev_priv-irq_lock, irqflags);
 
@@ -605,12 +635,15 @@ static inline void hotplug_irq_storm_detect(struct 
drm_device *dev,
} else if (dev_priv-hpd_stats[i].hpd_cnt  5) {
dev_priv-hpd_stats[i].hpd_mark = 
HPD_MARK_DISABLED;
DRM_DEBUG_KMS(HPD interrupt storm detected on  
PIN %d\n, i);
+   ret = true;
} else
dev_priv-hpd_stats[i].hpd_cnt++;
}
}
 
spin_unlock_irqrestore(dev_priv-irq_lock, irqflags);
+
+   return ret;
 }
 
 static void gmbus_irq_handler(struct drm_device *dev)
@@ -686,7 +719,8 @@ static irqreturn_t valleyview_irq_handler(int irq, void

[Intel-gfx] [PATCH v4] drm/i915: Add Reenable Timer to turn Hotplug Detection back on (v4)

[Egbert Eich]

We disable hoptplug detection when we encounter a hotplug event
storm. Still hotplug detection is required on some outputs (like
Display Port). The interrupt storm may be only temporary (on certain
Dell Laptops for instance it happens at certain charging states of
the system). Thus we enable it after a certain grace period (2 minutes).
Should the interrupt storm persist it will be detected immediately
and it will be disabled again.

v2: Reordered drm_i915_private: moved hotplug_reenable_timer to hpd state 
tracker.
v3: Clarified loop start value,
Removed superfluous test for Ivybridge and Haswell,
Restructured loop to avoid deep nesting (all suggested by Ville Syrjälä)
v4: Fixed two bugs pointed out by Jani Nikula.

Signed-off-by: Egbert Eich e...@suse.de
---
 drivers/gpu/drm/i915/i915_drv.h |  1 +
 drivers/gpu/drm/i915/i915_irq.c | 50 +
 2 files changed, 51 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 83fc1a6..195b9fe 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -938,6 +938,7 @@ typedef struct drm_i915_private {
HPD_MARK_DISABLED = 2
} hpd_mark;
} hpd_stats[HPD_NUM_PINS];
+   struct timer_list hotplug_reenable_timer;
 
int num_pch_pll;
int num_plane;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 834c717..f60c643 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -337,6 +337,8 @@ static int i915_get_vblank_timestamp(struct drm_device 
*dev, int pipe,
 /*
  * Handle hotplug events outside the interrupt handler proper.
  */
+#define I915_REENABLE_HOTPLUG_DELAY (2*60*1000)
+
 static void i915_hotplug_work_func(struct work_struct *work)
 {
drm_i915_private_t *dev_priv = container_of(work, drm_i915_private_t,
@@ -377,7 +379,10 @@ static void i915_hotplug_work_func(struct work_struct 
*work)
  * some connectors */
if (hpd_disabled) {
drm_kms_helper_poll_enable(dev);
+   mod_timer(dev_priv-hotplug_reenable_timer,
+ jiffies + 
msecs_to_jiffies(I915_REENABLE_HOTPLUG_DELAY));
}
+
spin_unlock_irqrestore(dev_priv-irq_lock, irqflags);
 
list_for_each_entry(intel_encoder, mode_config-encoder_list, 
base.head)
@@ -2352,6 +2357,8 @@ static void valleyview_irq_uninstall(struct drm_device 
*dev)
if (!dev_priv)
return;
 
+   del_timer_sync(dev_priv-hotplug_reenable_timer);
+
for_each_pipe(pipe)
I915_WRITE(PIPESTAT(pipe), 0x);
 
@@ -2373,6 +2380,8 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
if (!dev_priv)
return;
 
+   del_timer_sync(dev_priv-hotplug_reenable_timer);
+
I915_WRITE(HWSTAM, 0x);
 
I915_WRITE(DEIMR, 0x);
@@ -2749,6 +2758,8 @@ static void i915_irq_uninstall(struct drm_device * dev)
drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev-dev_private;
int pipe;
 
+   del_timer_sync(dev_priv-hotplug_reenable_timer);
+
if (I915_HAS_HOTPLUG(dev)) {
I915_WRITE(PORT_HOTPLUG_EN, 0);
I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
@@ -2993,6 +3004,8 @@ static void i965_irq_uninstall(struct drm_device * dev)
if (!dev_priv)
return;
 
+   del_timer_sync(dev_priv-hotplug_reenable_timer);
+
I915_WRITE(PORT_HOTPLUG_EN, 0);
I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
 
@@ -3008,6 +3021,41 @@ static void i965_irq_uninstall(struct drm_device * dev)
I915_WRITE(IIR, I915_READ(IIR));
 }
 
+static void i915_reenable_hotplug_timer_func(unsigned long data)
+{
+   drm_i915_private_t *dev_priv = (drm_i915_private_t *)data;
+   struct drm_device *dev = dev_priv-dev;
+   struct drm_mode_config *mode_config = dev-mode_config;
+   unsigned long irqflags;
+   int i;
+
+   spin_lock_irqsave(dev_priv-irq_lock, irqflags);
+   for (i = (HPD_NONE + 1); i  HPD_NUM_PINS; i++) {
+   struct drm_connector *connector;
+
+   if (dev_priv-hpd_stats[i].hpd_mark != HPD_DISABLED)
+   continue;
+
+   dev_priv-hpd_stats[i].hpd_mark = HPD_ENABLED;
+
+   list_for_each_entry(connector, mode_config-connector_list, 
head) {
+   struct intel_connector *intel_connector = 
to_intel_connector(connector);
+
+   if (intel_connector-encoder-hpd_pin == i) {
+   if (connector-polled != 
intel_connector-polled)
+   DRM_DEBUG_DRIVER(Reenabling HPD on 
connector %s\n,
+
drm_get_connector_name(connector));
+   connector-polled = intel_connector

Re: [Intel-gfx] [PATCH v3 6/7] drm/i915: Add bit field to record which pins have received HPD events (v2)

[Egbert Eich]

On Thu, Apr 11, 2013 at 04:21:54PM +0300, Jani Nikula wrote:
  }
  +   if (hpd_event_bits  (1  intel_encoder-hpd_pin)) {
  +   DRM_DEBUG_KMS(Connector %s (pin %i) received hotplug 
  event.\n,
  + drm_get_connector_name(connector), 
  intel_encoder-hpd_pin);
  +   }
 
 I fear this may end up being a bit excessive debug printing.
 

In a storm condition this is definitely true.
I still would like to keep this code in for debugging as I have 
some more ideas I would like to implement and test - however I
will wrap this code with #if 0 .. #endif so it can be enabled
easily.
Eventually I will remove it entirely.
Would this be ok with you?

Cheers,
Egbert.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v3] drm/i915: Add bit field to record which pins have received HPD events (v3)

[Egbert Eich]

This way it is possible to limit 're'-detect() of displays to connectors
which have received an HPD event.

v2: Reordered drm_i915_private: Move hpd_event_bits to hpd state tracking.
v3: Fixed merge conflicts with previous patches.

Signed-off-by: Egbert Eich e...@suse.de
---
 drivers/gpu/drm/i915/i915_drv.h |  1 +
 drivers/gpu/drm/i915/i915_irq.c | 10 ++
 2 files changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 195b9fe..c0d9fb9 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -938,6 +938,7 @@ typedef struct drm_i915_private {
HPD_MARK_DISABLED = 2
} hpd_mark;
} hpd_stats[HPD_NUM_PINS];
+   u32 hpd_event_bits;
struct timer_list hotplug_reenable_timer;
 
int num_pch_pll;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index f60c643..f7219d8 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -350,6 +350,7 @@ static void i915_hotplug_work_func(struct work_struct *work)
struct drm_connector *connector;
unsigned long irqflags;
bool hpd_disabled = false;
+   u32 hpd_event_bits;
 
/* HPD irq before everything is fully set up. */
if (!dev_priv-enable_hotplug_processing)
@@ -359,6 +360,9 @@ static void i915_hotplug_work_func(struct work_struct *work)
DRM_DEBUG_KMS(running encoder hotplug functions\n);
 
spin_lock_irqsave(dev_priv-irq_lock, irqflags);
+
+   hpd_event_bits = dev_priv-hpd_event_bits;
+   dev_priv-hpd_event_bits = 0;
list_for_each_entry(connector, mode_config-connector_list, head) {
intel_connector = to_intel_connector(connector);
intel_encoder = intel_connector-encoder;
@@ -373,6 +377,10 @@ static void i915_hotplug_work_func(struct work_struct 
*work)
| DRM_CONNECTOR_POLL_DISCONNECT;
hpd_disabled = true;
}
+   if (hpd_event_bits  (1  intel_encoder-hpd_pin)) {
+   DRM_DEBUG_KMS(Connector %s (pin %i) received hotplug 
event.\n,
+ drm_get_connector_name(connector), 
intel_encoder-hpd_pin);
+   }
}
 /* if there were no outputs to poll, poll was disabled,
  * therefore make sure it's enabled when disabling HPD on
@@ -632,6 +640,7 @@ static inline bool hotplug_irq_storm_detect(struct 
drm_device *dev,
for (i = 1; i  HPD_NUM_PINS; i++) {
if ((hpd[i]  hotplug_trigger) 
dev_priv-hpd_stats[i].hpd_mark == HPD_ENABLED) {
+   dev_priv-hpd_event_bits |= (1  i);
if (!time_in_range(jiffies, 
dev_priv-hpd_stats[i].hpd_last_jiffies,
  
dev_priv-hpd_stats[i].hpd_last_jiffies
  + 
msecs_to_jiffies(HPD_STORM_DETECT_PERIOD))) {
@@ -639,6 +648,7 @@ static inline bool hotplug_irq_storm_detect(struct 
drm_device *dev,
dev_priv-hpd_stats[i].hpd_cnt = 0;
} else if (dev_priv-hpd_stats[i].hpd_cnt  5) {
dev_priv-hpd_stats[i].hpd_mark = 
HPD_MARK_DISABLED;
+   dev_priv-hpd_event_bits = ~(1  i);
DRM_DEBUG_KMS(HPD interrupt storm detected on  
PIN %d\n, i);
ret = true;
} else
-- 
1.8.1.4

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v2] drm/i915: Only reprobe display on encoder which has received an HPD event (v2)

[Egbert Eich]

Instead of calling into the DRM helper layer to poll all connectors for
changes in connected displays probe only those connectors which have
received a hotplug event.

Signed-off-by: Egbert Eich e...@suse.de
Reviewed-by: Jani Nikula jani.nik...@intel.com

v2: Resolved conflicts with changes in previous commits.
Renamed function and and added a WARN_ON() to warn of
intel_hpd_irq_event() from being called without
mode_config.mutex held - suggested by Jani Nikula.
---
 drivers/gpu/drm/i915/i915_irq.c | 34 --
 1 file changed, 28 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 6e643d7..54eca33 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -334,6 +334,21 @@ static int i915_get_vblank_timestamp(struct drm_device 
*dev, int pipe,
 crtc);
 }
 
+static int intel_hpd_irq_event(struct drm_device *dev, struct drm_connector 
*connector)
+{
+   enum drm_connector_status old_status;
+
+   WARN_ON(!mutex_is_locked(dev-mode_config.mutex));
+   old_status = connector-status;
+
+   connector-status = connector-funcs-detect(connector, false);
+   DRM_DEBUG_KMS([CONNECTOR:%d:%s] status updated from %d to %d\n,
+ connector-base.id,
+ drm_get_connector_name(connector),
+ old_status, connector-status);
+   return (old_status != connector-status);
+}
+
 /*
  * Handle hotplug events outside the interrupt handler proper.
  */
@@ -350,6 +365,7 @@ static void i915_hotplug_work_func(struct work_struct *work)
struct drm_connector *connector;
unsigned long irqflags;
bool hpd_disabled = false;
+   bool changed = false;
u32 hpd_event_bits;
 
/* HPD irq before everything is fully set up. */
@@ -395,14 +411,20 @@ static void i915_hotplug_work_func(struct work_struct 
*work)
 
spin_unlock_irqrestore(dev_priv-irq_lock, irqflags);
 
-   list_for_each_entry(intel_encoder, mode_config-encoder_list, 
base.head)
-   if (intel_encoder-hot_plug)
-   intel_encoder-hot_plug(intel_encoder);
-
+   list_for_each_entry(connector, mode_config-connector_list, head) {
+   intel_connector = to_intel_connector(connector);
+   intel_encoder = intel_connector-encoder;
+   if (hpd_event_bits  (1  intel_encoder-hpd_pin)) {
+   if (intel_encoder-hot_plug)
+   intel_encoder-hot_plug(intel_encoder);
+   if (intel_hpd_irq_event(dev, connector))
+   changed = true;
+   }
+   }
mutex_unlock(mode_config-mutex);
 
-   /* Just fire off a uevent and let userspace tell us what to do */
-   drm_helper_hpd_irq_event(dev);
+   if (changed)
+   drm_kms_helper_hotplug_event(dev);
 }
 
 static void ironlake_handle_rps_change(struct drm_device *dev)
-- 
1.8.1.4

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v3 Update] drm/i915: Add bit field to record which pins have received HPD events (v3)

[Egbert Eich]

This allows to add code which limits 're'-detect() of displays to connectors
that have received an HPD event.

v2: Reordered drm_i915_private: Move hpd_event_bits to hpd state tracking.
v3: Fixed merge conflicts with previous patches, removed some noisy debug
logging as suggested by Jani Nikula.

Signed-off-by: Egbert Eich e...@suse.de
---
(Sorry for the spam, I had accidenty went the wrong version of this patch)

 drivers/gpu/drm/i915/i915_drv.h |  1 +
 drivers/gpu/drm/i915/i915_irq.c | 12 
 2 files changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 195b9fe..c0d9fb9 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -938,6 +938,7 @@ typedef struct drm_i915_private {
HPD_MARK_DISABLED = 2
} hpd_mark;
} hpd_stats[HPD_NUM_PINS];
+   u32 hpd_event_bits;
struct timer_list hotplug_reenable_timer;
 
int num_pch_pll;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index f60c643..6e643d7 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -350,6 +350,7 @@ static void i915_hotplug_work_func(struct work_struct *work)
struct drm_connector *connector;
unsigned long irqflags;
bool hpd_disabled = false;
+   u32 hpd_event_bits;
 
/* HPD irq before everything is fully set up. */
if (!dev_priv-enable_hotplug_processing)
@@ -359,6 +360,9 @@ static void i915_hotplug_work_func(struct work_struct *work)
DRM_DEBUG_KMS(running encoder hotplug functions\n);
 
spin_lock_irqsave(dev_priv-irq_lock, irqflags);
+
+   hpd_event_bits = dev_priv-hpd_event_bits;
+   dev_priv-hpd_event_bits = 0;
list_for_each_entry(connector, mode_config-connector_list, head) {
intel_connector = to_intel_connector(connector);
intel_encoder = intel_connector-encoder;
@@ -373,6 +377,12 @@ static void i915_hotplug_work_func(struct work_struct 
*work)
| DRM_CONNECTOR_POLL_DISCONNECT;
hpd_disabled = true;
}
+#if 0
+   if (hpd_event_bits  (1  intel_encoder-hpd_pin)) {
+   DRM_DEBUG_KMS(Connector %s (pin %i) received hotplug 
event.\n,
+ drm_get_connector_name(connector), 
intel_encoder-hpd_pin);
+   }
+#endif
}
 /* if there were no outputs to poll, poll was disabled,
  * therefore make sure it's enabled when disabling HPD on
@@ -632,6 +642,7 @@ static inline bool hotplug_irq_storm_detect(struct 
drm_device *dev,
for (i = 1; i  HPD_NUM_PINS; i++) {
if ((hpd[i]  hotplug_trigger) 
dev_priv-hpd_stats[i].hpd_mark == HPD_ENABLED) {
+   dev_priv-hpd_event_bits |= (1  i);
if (!time_in_range(jiffies, 
dev_priv-hpd_stats[i].hpd_last_jiffies,
  
dev_priv-hpd_stats[i].hpd_last_jiffies
  + 
msecs_to_jiffies(HPD_STORM_DETECT_PERIOD))) {
@@ -639,6 +650,7 @@ static inline bool hotplug_irq_storm_detect(struct 
drm_device *dev,
dev_priv-hpd_stats[i].hpd_cnt = 0;
} else if (dev_priv-hpd_stats[i].hpd_cnt  5) {
dev_priv-hpd_stats[i].hpd_mark = 
HPD_MARK_DISABLED;
+   dev_priv-hpd_event_bits = ~(1  i);
DRM_DEBUG_KMS(HPD interrupt storm detected on  
PIN %d\n, i);
ret = true;
} else
-- 
1.8.1.4

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v3 0/7] Add HPD interrupt storm detection.

[Egbert Eich]

This set of patches contains the remaining bits to add IRQ strom detection and
disabling.
Daniel has asked me to send this batch as soon as possible even though this
latest batch has not yet been retested, yet.


Egbert Eich (7):
  drm/i915: Add HPD IRQ storm detection (v4)
  drm/i915: (re)init HPD interrupt storm statistics
  drm/i915: Mask out the HPD irq bits before setting them individually.
  drm/i915: Disable HPD interrupt on pin when irq storm is detected (v2)
  drm/i915: Add Reenable Timer to turn Hotplug Detection back on (v3)
  drm/i915: Add bit field to record which pins have received HPD events
(v2)
  drm/i915: Only reprobe display on encoder which has received an HPD
event

 drivers/gpu/drm/i915/i915_drv.h   |  12 ++
 drivers/gpu/drm/i915/i915_irq.c   | 224 ++
 drivers/gpu/drm/i915/intel_crt.c  |   6 +-
 drivers/gpu/drm/i915/intel_dp.c   |   1 -
 drivers/gpu/drm/i915/intel_drv.h  |   4 +
 drivers/gpu/drm/i915/intel_hdmi.c |   1 -
 drivers/gpu/drm/i915/intel_sdvo.c |   5 +-
 drivers/gpu/drm/i915/intel_tv.c   |   2 +-
 8 files changed, 221 insertions(+), 34 deletions(-)

-- 
1.8.1.4

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v3 1/7] drm/i915: Add HPD IRQ storm detection (v4)

[Egbert Eich]

From: Egbert Eich e...@suse.de

Add a hotplug IRQ storm detection (triggered when a hotplug interrupt
fires more than 5 times / sec).
Rationale:
Despite of the many attempts to fix the problem with noisy hotplug
interrupt lines we are still seeing systems which have issues:
Once cause of noise seems to be bad routing of the hotplug line
on the board: cross talk from other signals seems to cause erronous
hotplug interrupts. This has been documented as an erratum for the
the i945GM chipset and thus hotplug support was disabled for this
chipset model but others seem to have this problem, too.

We have seen this issue on a G35 motherboard for example:
Even different motherboards of the same model seem to behave
differently: while some only see only around 10-100 interrupts/s
others seem to see 5k or more.
We've also observed a dependency on the selected video mode.

Also on certain laptops interrupt noise seems to occur duing
battery charging when the battery is at a certain charge levels.

Thus we add a simple algorithm here that detects an 'interrupt storm'
condition.

v2: Fixed comment.
v3: Reordered drm_i915_private: moved hpd state tracking to hotplug work stuff.
v4: Followed by Jesse Barnes to use a time_..() macro.

Signed-off-by: Egbert Eich e...@suse.de
Acked-by: Chris Wilson ch...@chris-wilson.co.uk
Reviewed-by: Jesse Barnes jbar...@virtuousgeek.org
---
 drivers/gpu/drm/i915/i915_drv.h |  9 ++
 drivers/gpu/drm/i915/i915_irq.c | 66 +
 2 files changed, 63 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 44fca0b..83fc1a6 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -929,6 +929,15 @@ typedef struct drm_i915_private {
 
struct work_struct hotplug_work;
bool enable_hotplug_processing;
+   struct {
+   unsigned long hpd_last_jiffies;
+   int hpd_cnt;
+   enum {
+   HPD_ENABLED = 0,
+   HPD_DISABLED = 1,
+   HPD_MARK_DISABLED = 2
+   } hpd_mark;
+   } hpd_stats[HPD_NUM_PINS];
 
int num_pch_pll;
int num_plane;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 4c5bdd0..32b5527 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -582,6 +582,37 @@ static void gen6_queue_rps_work(struct drm_i915_private 
*dev_priv,
queue_work(dev_priv-wq, dev_priv-rps.work);
 }
 
+#define HPD_STORM_DETECT_PERIOD 1000
+
+static inline void hotplug_irq_storm_detect(struct drm_device *dev,
+   u32 hotplug_trigger,
+   const u32 *hpd)
+{
+   drm_i915_private_t *dev_priv = dev-dev_private;
+   unsigned long irqflags;
+   int i;
+
+   spin_lock_irqsave(dev_priv-irq_lock, irqflags);
+
+   for (i = 1; i  HPD_NUM_PINS; i++) {
+   if ((hpd[i]  hotplug_trigger) 
+   dev_priv-hpd_stats[i].hpd_mark == HPD_ENABLED) {
+   if (!time_in_range(jiffies, 
dev_priv-hpd_stats[i].hpd_last_jiffies,
+ 
dev_priv-hpd_stats[i].hpd_last_jiffies
+ + 
msecs_to_jiffies(HPD_STORM_DETECT_PERIOD))) {
+   dev_priv-hpd_stats[i].hpd_last_jiffies = 
jiffies;
+   dev_priv-hpd_stats[i].hpd_cnt = 0;
+   } else if (dev_priv-hpd_stats[i].hpd_cnt  5) {
+   dev_priv-hpd_stats[i].hpd_mark = 
HPD_MARK_DISABLED;
+   DRM_DEBUG_KMS(HPD interrupt storm detected on  
PIN %d\n, i);
+   } else
+   dev_priv-hpd_stats[i].hpd_cnt++;
+   }
+   }
+
+   spin_unlock_irqrestore(dev_priv-irq_lock, irqflags);
+}
+
 static void gmbus_irq_handler(struct drm_device *dev)
 {
struct drm_i915_private *dev_priv = (drm_i915_private_t *) 
dev-dev_private;
@@ -650,13 +681,15 @@ static irqreturn_t valleyview_irq_handler(int irq, void 
*arg)
/* Consume port.  Then clear IIR or we'll miss events */
if (iir  I915_DISPLAY_PORT_INTERRUPT) {
u32 hotplug_status = I915_READ(PORT_HOTPLUG_STAT);
+   u32 hotplug_trigger = hotplug_status  
HOTPLUG_INT_STATUS_I915;
 
DRM_DEBUG_DRIVER(hotplug event received, stat 
0x%08x\n,
 hotplug_status);
-   if (hotplug_status  HOTPLUG_INT_STATUS_I915)
+   if (hotplug_trigger) {
+   hotplug_irq_storm_detect(dev, hotplug_trigger, 
hpd_status_i915);
queue_work(dev_priv-wq,
   dev_priv-hotplug_work

[Intel-gfx] [PATCH v3 2/7] drm/i915: (re)init HPD interrupt storm statistics

[Egbert Eich]

From: Egbert Eich e...@suse.de

When an encoder is shared on several connectors there is only
one hotplug line, thus this line needs to be shared among these
connectors.
If HPD detect only works reliably on a subset of those connectors,
we want to poll the others. Thus we need to make sure that storm
detection doesn't mess up the settings for those connectors.
Therefore we store the settings in the intel_connector struct and
restore them from there.
If nothing is set but the encoder has a hpd_pin set we assume this
connector is hotplug capable.
On init/reset we make sure the polled state of the connectors
is (re)set to the default value, the HPD interrupts are marked
enabled.

Signed-off-by: Egbert Eich e...@suse.de
---
 drivers/gpu/drm/i915/i915_irq.c   | 13 +
 drivers/gpu/drm/i915/intel_crt.c  |  6 ++
 drivers/gpu/drm/i915/intel_dp.c   |  1 -
 drivers/gpu/drm/i915/intel_drv.h  |  4 
 drivers/gpu/drm/i915/intel_hdmi.c |  1 -
 drivers/gpu/drm/i915/intel_sdvo.c |  5 ++---
 drivers/gpu/drm/i915/intel_tv.c   |  2 +-
 7 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 32b5527..5408a3a 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3045,7 +3045,20 @@ void intel_irq_init(struct drm_device *dev)
 void intel_hpd_init(struct drm_device *dev)
 {
struct drm_i915_private *dev_priv = dev-dev_private;
+   struct drm_mode_config *mode_config = dev-mode_config;
+   struct drm_connector *connector;
+   int i;
 
+   for (i = 1; i  HPD_NUM_PINS; i++) {
+   dev_priv-hpd_stats[i].hpd_cnt = 0;
+   dev_priv-hpd_stats[i].hpd_mark = HPD_ENABLED;
+   }
+   list_for_each_entry(connector, mode_config-connector_list, head) {
+   struct intel_connector *intel_connector = 
to_intel_connector(connector);
+   connector-polled = intel_connector-polled;
+   if (!connector-polled  I915_HAS_HOTPLUG(dev)  
intel_connector-encoder-hpd_pin  HPD_NONE)
+   connector-polled = DRM_CONNECTOR_POLL_HPD;
+   }
if (dev_priv-display.hpd_irq_setup)
dev_priv-display.hpd_irq_setup(dev);
 }
diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 1ae2d7f..c063b9f 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -793,10 +793,8 @@ void intel_crt_init(struct drm_device *dev)
 
drm_sysfs_connector_add(connector);
 
-   if (I915_HAS_HOTPLUG(dev))
-   connector-polled = DRM_CONNECTOR_POLL_HPD;
-   else
-   connector-polled = DRM_CONNECTOR_POLL_CONNECT;
+   if (!I915_HAS_HOTPLUG(dev))
+   intel_connector-polled = DRM_CONNECTOR_POLL_CONNECT;
 
/*
 * Configure the automatic hotplug detection stuff
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 482b5e5..1e9b19a 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2786,7 +2786,6 @@ intel_dp_init_connector(struct intel_digital_port 
*intel_dig_port,
drm_connector_init(dev, connector, intel_dp_connector_funcs, type);
drm_connector_helper_add(connector, intel_dp_connector_helper_funcs);
 
-   connector-polled = DRM_CONNECTOR_POLL_HPD;
connector-interlace_allowed = true;
connector-doublescan_allowed = 0;
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d7bd031..a05fde7 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -171,6 +171,10 @@ struct intel_connector {
 
/* Cached EDID for eDP and LVDS. May hold ERR_PTR for invalid EDID. */
struct edid *edid;
+
+   /* since POLL and HPD connectors may use the same HPD line keep the 
native
+  state of connector-polled in case hotplug storm detection changes 
it */
+   u8 polled;
 };
 
 struct intel_crtc_config {
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c 
b/drivers/gpu/drm/i915/intel_hdmi.c
index ee4a8da..8912201 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -998,7 +998,6 @@ void intel_hdmi_init_connector(struct intel_digital_port 
*intel_dig_port,
   DRM_MODE_CONNECTOR_HDMIA);
drm_connector_helper_add(connector, intel_hdmi_connector_helper_funcs);
 
-   connector-polled = DRM_CONNECTOR_POLL_HPD;
connector-interlace_allowed = 1;
connector-doublescan_allowed = 0;
 
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c 
b/drivers/gpu/drm/i915/intel_sdvo.c
index 298dc85..64b8b40 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -2276,7 +2276,6 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int 
device)
connector = intel_connector-base;
if (intel_sdvo_get_hotplug_support(intel_sdvo

[Intel-gfx] [PATCH v3 3/7] drm/i915: Mask out the HPD irq bits before setting them individually.

[Egbert Eich]

From: Egbert Eich e...@suse.de

To disable previously enabled HPD IRQs we need to reset them and
set the enabled ones individually.

Signed-off-by: Egbert Eich e...@suse.de
---
 drivers/gpu/drm/i915/i915_irq.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 5408a3a..a3f1ac4 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2117,9 +2117,11 @@ static void ibx_hpd_irq_setup(struct drm_device *dev)
u32 hotplug;
 
if (HAS_PCH_IBX(dev)) {
+   mask = ~SDE_HOTPLUG_MASK;
list_for_each_entry(intel_encoder, mode_config-encoder_list, 
base.head)
mask |= hpd_ibx[intel_encoder-hpd_pin];
} else {
+   mask = ~SDE_HOTPLUG_MASK_CPT;
list_for_each_entry(intel_encoder, mode_config-encoder_list, 
base.head)
mask |= hpd_cpt[intel_encoder-hpd_pin];
}
-- 
1.8.1.4

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v3 6/7] drm/i915: Add bit field to record which pins have received HPD events (v2)

[Egbert Eich]

From: Egbert Eich e...@suse.de

This way it is possible to limit 're'-detect() of displays to connectors
which have received an HPD event.

v2: Reordered drm_i915_private: Move hpd_event_bits to hpd state tracking.

Signed-off-by: Egbert Eich e...@suse.de
---
 drivers/gpu/drm/i915/i915_drv.h |  1 +
 drivers/gpu/drm/i915/i915_irq.c | 10 ++
 2 files changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a3ed2e3..907e290 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -938,6 +938,7 @@ typedef struct drm_i915_private {
HPD_MARK_DISABLED = 2
} hpd_mark;
} hpd_stats[HPD_NUM_PINS];
+   u32 hpd_event_bits;
 #define I915_REENABLE_HOTPLUG_DELAY (2*60*1000)
struct timer_list hotplug_reenable_timer;
 
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 1a00533..92041b9 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -348,6 +348,7 @@ static void i915_hotplug_work_func(struct work_struct *work)
struct drm_connector *connector;
unsigned long irqflags;
bool connector_disabled = false;
+   u32 hpd_event_bits;
 
/* HPD irq before everything is fully set up. */
if (!dev_priv-enable_hotplug_processing)
@@ -357,6 +358,9 @@ static void i915_hotplug_work_func(struct work_struct *work)
DRM_DEBUG_KMS(running encoder hotplug functions\n);
 
spin_lock_irqsave(dev_priv-irq_lock, irqflags);
+
+   hpd_event_bits = dev_priv-hpd_event_bits;
+   dev_priv-hpd_event_bits = 0;
list_for_each_entry(connector, mode_config-connector_list, head) {
intel_connector = to_intel_connector(connector);
intel_encoder = intel_connector-encoder;
@@ -370,6 +374,10 @@ static void i915_hotplug_work_func(struct work_struct 
*work)
| DRM_CONNECTOR_POLL_DISCONNECT;
connector_disabled = true;
}
+   if (hpd_event_bits  (1  intel_encoder-hpd_pin)) {
+   DRM_DEBUG_KMS(Connector %s (pin %i) received hotplug 
event.\n,
+ drm_get_connector_name(connector), 
intel_encoder-hpd_pin);
+   }
}
if (connector_disabled) {
drm_kms_helper_poll_enable(dev); /* if there were no outputs to 
poll, poll is disabled */
@@ -626,6 +634,7 @@ static inline bool hotplug_irq_storm_detect(struct 
drm_device *dev,
for (i = 1; i  HPD_NUM_PINS; i++) {
if ((hpd[i]  hotplug_trigger) 
dev_priv-hpd_stats[i].hpd_mark == HPD_ENABLED) {
+   dev_priv-hpd_event_bits |= (1  i);
if (!time_in_range(jiffies, 
dev_priv-hpd_stats[i].hpd_last_jiffies,
  
dev_priv-hpd_stats[i].hpd_last_jiffies
  + 
msecs_to_jiffies(HPD_STORM_DETECT_PERIOD))) {
@@ -633,6 +642,7 @@ static inline bool hotplug_irq_storm_detect(struct 
drm_device *dev,
dev_priv-hpd_stats[i].hpd_cnt = 0;
} else if (dev_priv-hpd_stats[i].hpd_cnt  5) {
dev_priv-hpd_stats[i].hpd_mark = 
HPD_MARK_DISABLED;
+   dev_priv-hpd_event_bits = ~(1  i);
DRM_DEBUG_KMS(HPD interrupt storm detected on  
PIN %d\n, i);
ret = true;
} else
-- 
1.8.1.4

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v3 7/7] drm/i915: Only reprobe display on encoder which has received an HPD event

[Egbert Eich]

From: Egbert Eich e...@suse.de

Instead of calling into the DRM helper layer to poll all connectors for
changes in connected displays probe only those connectors which have
received a hotplug event.

Signed-off-by: Egbert Eich e...@suse.de
---
 drivers/gpu/drm/i915/i915_irq.c | 37 +++--
 1 file changed, 31 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 92041b9..7788536 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -334,6 +334,24 @@ static int i915_get_vblank_timestamp(struct drm_device 
*dev, int pipe,
 crtc);
 }
 
+/**
+ * drm_helper_hpd_irq_single_connector_event() - call this function with 
mode_config.mutex lock held
+ */
+
+static int intel_hpd_irq_single_connector_event(struct drm_device *dev, struct 
drm_connector *connector)
+{
+   enum drm_connector_status old_status;
+
+   old_status = connector-status;
+
+   connector-status = connector-funcs-detect(connector, false);
+   DRM_DEBUG_KMS([CONNECTOR:%d:%s] status updated from %d to %d\n,
+ connector-base.id,
+ drm_get_connector_name(connector),
+ old_status, connector-status);
+   return (old_status != connector-status);
+}
+
 /*
  * Handle hotplug events outside the interrupt handler proper.
  */
@@ -348,6 +366,7 @@ static void i915_hotplug_work_func(struct work_struct *work)
struct drm_connector *connector;
unsigned long irqflags;
bool connector_disabled = false;
+   bool changed = false;
u32 hpd_event_bits;
 
/* HPD irq before everything is fully set up. */
@@ -387,14 +406,20 @@ static void i915_hotplug_work_func(struct work_struct 
*work)
 
spin_unlock_irqrestore(dev_priv-irq_lock, irqflags);
 
-   list_for_each_entry(intel_encoder, mode_config-encoder_list, 
base.head)
-   if (intel_encoder-hot_plug)
-   intel_encoder-hot_plug(intel_encoder);
-
+   list_for_each_entry(connector, mode_config-connector_list, head) {
+   intel_connector = to_intel_connector(connector);
+   intel_encoder = intel_connector-encoder;
+   if (hpd_event_bits  (1  intel_encoder-hpd_pin)) {
+   if (intel_encoder-hot_plug)
+   intel_encoder-hot_plug(intel_encoder);
+   if (intel_hpd_irq_single_connector_event(dev, 
connector))
+   changed = true;
+   }
+   }
mutex_unlock(mode_config-mutex);
 
-   /* Just fire off a uevent and let userspace tell us what to do */
-   drm_helper_hpd_irq_event(dev);
+   if (changed)
+   drm_kms_helper_hotplug_event(dev);
 }
 
 static void ironlake_handle_rps_change(struct drm_device *dev)
-- 
1.8.1.4

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v3 5/7] drm/i915: Add Reenable Timer to turn Hotplug Detection back on (v3)

[Egbert Eich]

From: Egbert Eich e...@suse.de

We disable hoptplug detection when we encounter a hotplug event
storm. Still hotplug detection is required on some outputs (like
Display Port). The interrupt storm may be only temporary (on certain
Dell Laptops for instance it happens at certain charging states of
the system). Thus we enable it after a certain grace period (2 minutes).
Should the interrupt storm persist it will be detected immediately
and it will be disabled again.

v2: Reordered drm_i915_private: moved hotplug_reenable_timer to hpd state 
tracker.
v3: Clarified loop start value,
Removed superfluous test for Ivybridge and Haswell,
Restructured loop to avoid deep nesting (all suggested by Ville Syrjälä)

Signed-off-by: Egbert Eich e...@suse.de
---
 drivers/gpu/drm/i915/i915_drv.h |  2 ++
 drivers/gpu/drm/i915/i915_irq.c | 49 -
 2 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 83fc1a6..a3ed2e3 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -938,6 +938,8 @@ typedef struct drm_i915_private {
HPD_MARK_DISABLED = 2
} hpd_mark;
} hpd_stats[HPD_NUM_PINS];
+#define I915_REENABLE_HOTPLUG_DELAY (2*60*1000)
+   struct timer_list hotplug_reenable_timer;
 
int num_pch_pll;
int num_plane;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index d0e6f6d..1a00533 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -371,8 +371,11 @@ static void i915_hotplug_work_func(struct work_struct 
*work)
connector_disabled = true;
}
}
-   if (connector_disabled)
+   if (connector_disabled) {
drm_kms_helper_poll_enable(dev); /* if there were no outputs to 
poll, poll is disabled */
+   mod_timer(dev_priv-hotplug_reenable_timer,
+ jiffies + 
msecs_to_jiffies(I915_REENABLE_HOTPLUG_DELAY));
+   }
 
spin_unlock_irqrestore(dev_priv-irq_lock, irqflags);
 
@@ -2348,6 +2351,8 @@ static void valleyview_irq_uninstall(struct drm_device 
*dev)
if (!dev_priv)
return;
 
+   del_timer_sync(dev_priv-hotplug_reenable_timer);
+
for_each_pipe(pipe)
I915_WRITE(PIPESTAT(pipe), 0x);
 
@@ -2369,6 +2374,8 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
if (!dev_priv)
return;
 
+   del_timer_sync(dev_priv-hotplug_reenable_timer);
+
I915_WRITE(HWSTAM, 0x);
 
I915_WRITE(DEIMR, 0x);
@@ -2745,6 +2752,8 @@ static void i915_irq_uninstall(struct drm_device * dev)
drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev-dev_private;
int pipe;
 
+   del_timer_sync(dev_priv-hotplug_reenable_timer);
+
if (I915_HAS_HOTPLUG(dev)) {
I915_WRITE(PORT_HOTPLUG_EN, 0);
I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
@@ -2989,6 +2998,8 @@ static void i965_irq_uninstall(struct drm_device * dev)
if (!dev_priv)
return;
 
+   del_timer_sync(dev_priv-hotplug_reenable_timer);
+
I915_WRITE(PORT_HOTPLUG_EN, 0);
I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
 
@@ -3004,6 +3015,40 @@ static void i965_irq_uninstall(struct drm_device * dev)
I915_WRITE(IIR, I915_READ(IIR));
 }
 
+static void i915_reenable_hotplug_timer_func(unsigned long data)
+{
+   drm_i915_private_t *dev_priv = (drm_i915_private_t *)data;
+   struct drm_device *dev = dev_priv-dev;
+   struct drm_mode_config *mode_config = dev-mode_config;
+   unsigned long irqflags;
+   int i;
+
+   spin_lock_irqsave(dev_priv-irq_lock, irqflags);
+   for (i = (HPD_NONE + 1); i  HPD_NUM_PINS; i++) {
+   struct drm_connector *connector;
+
+   if (dev_priv-hpd_stats[i].hpd_mark != HPD_MARK_DISABLED)
+   continue;
+
+   dev_priv-hpd_stats[i].hpd_mark = HPD_ENABLED;
+
+   list_for_each_entry(connector, mode_config-connector_list, 
head) {
+   struct intel_connector *intel_connector = 
to_intel_connector(connector);
+
+   if (intel_connector-encoder-hpd_pin == i) {
+   if (connector-polled != 
intel_connector-polled)
+   DRM_DEBUG_DRIVER(Reenabling HPD on 
connector %s\n,
+
drm_get_connector_name(connector));
+   connector-polled = intel_connector-polled;
+   if (!connector-polled)
+   connector-polled = 
DRM_CONNECTOR_POLL_HPD;
+   }
+   }
+   dev_priv-display.hpd_irq_setup(dev

[Intel-gfx] [PATCH v3 4/7] drm/i915: Disable HPD interrupt on pin when irq storm is detected (v2)

[Egbert Eich]

From: Egbert Eich e...@suse.de

This patch disables hotplug interrupts if an 'interrupt storm'
has been detected.
Noise on the interrupt line renders the hotplug interrupt useless:
each hotplug event causes the devices to be rescanned which will
will only increase the system load.
Thus disable the hotplug interrupts and fall back to periodic
device polling.

v2: Fixed cleanup typo.

Signed-off-by: Egbert Eich e...@suse.de
---
 drivers/gpu/drm/i915/i915_irq.c | 71 +++--
 1 file changed, 54 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index a3f1ac4..d0e6f6d 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -88,7 +88,8 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview 
are the same */
[HPD_PORT_D] = PORTD_HOTPLUG_INT_STATUS
 };
 
-
+static void ibx_hpd_irq_setup(struct drm_device *dev);
+static void i915_hpd_irq_setup(struct drm_device *dev);
 
 /* For display hotplug interrupt */
 static void
@@ -342,7 +343,11 @@ static void i915_hotplug_work_func(struct work_struct 
*work)
hotplug_work);
struct drm_device *dev = dev_priv-dev;
struct drm_mode_config *mode_config = dev-mode_config;
-   struct intel_encoder *encoder;
+   struct intel_connector *intel_connector;
+   struct intel_encoder *intel_encoder;
+   struct drm_connector *connector;
+   unsigned long irqflags;
+   bool connector_disabled = false;
 
/* HPD irq before everything is fully set up. */
if (!dev_priv-enable_hotplug_processing)
@@ -351,9 +356,29 @@ static void i915_hotplug_work_func(struct work_struct 
*work)
mutex_lock(mode_config-mutex);
DRM_DEBUG_KMS(running encoder hotplug functions\n);
 
-   list_for_each_entry(encoder, mode_config-encoder_list, base.head)
-   if (encoder-hot_plug)
-   encoder-hot_plug(encoder);
+   spin_lock_irqsave(dev_priv-irq_lock, irqflags);
+   list_for_each_entry(connector, mode_config-connector_list, head) {
+   intel_connector = to_intel_connector(connector);
+   intel_encoder = intel_connector-encoder;
+   if (intel_encoder-hpd_pin  HPD_NONE 
+   dev_priv-hpd_stats[intel_encoder-hpd_pin].hpd_mark == 
HPD_MARK_DISABLED 
+   connector-polled == DRM_CONNECTOR_POLL_HPD) {
+   pr_warn(HPD interrupt storm detected on connector %s 
disabling\n,
+   drm_get_connector_name(connector));
+   dev_priv-hpd_stats[intel_encoder-hpd_pin].hpd_mark = 
HPD_DISABLED;
+   connector-polled = DRM_CONNECTOR_POLL_CONNECT
+   | DRM_CONNECTOR_POLL_DISCONNECT;
+   connector_disabled = true;
+   }
+   }
+   if (connector_disabled)
+   drm_kms_helper_poll_enable(dev); /* if there were no outputs to 
poll, poll is disabled */
+
+   spin_unlock_irqrestore(dev_priv-irq_lock, irqflags);
+
+   list_for_each_entry(intel_encoder, mode_config-encoder_list, 
base.head)
+   if (intel_encoder-hot_plug)
+   intel_encoder-hot_plug(intel_encoder);
 
mutex_unlock(mode_config-mutex);
 
@@ -584,13 +609,14 @@ static void gen6_queue_rps_work(struct drm_i915_private 
*dev_priv,
 
 #define HPD_STORM_DETECT_PERIOD 1000
 
-static inline void hotplug_irq_storm_detect(struct drm_device *dev,
+static inline bool hotplug_irq_storm_detect(struct drm_device *dev,
u32 hotplug_trigger,
const u32 *hpd)
 {
drm_i915_private_t *dev_priv = dev-dev_private;
unsigned long irqflags;
int i;
+   bool ret = false;
 
spin_lock_irqsave(dev_priv-irq_lock, irqflags);
 
@@ -605,12 +631,15 @@ static inline void hotplug_irq_storm_detect(struct 
drm_device *dev,
} else if (dev_priv-hpd_stats[i].hpd_cnt  5) {
dev_priv-hpd_stats[i].hpd_mark = 
HPD_MARK_DISABLED;
DRM_DEBUG_KMS(HPD interrupt storm detected on  
PIN %d\n, i);
+   ret = true;
} else
dev_priv-hpd_stats[i].hpd_cnt++;
}
}
 
spin_unlock_irqrestore(dev_priv-irq_lock, irqflags);
+
+   return ret;
 }
 
 static void gmbus_irq_handler(struct drm_device *dev)
@@ -686,7 +715,8 @@ static irqreturn_t valleyview_irq_handler(int irq, void 
*arg)
DRM_DEBUG_DRIVER(hotplug event received, stat 
0x%08x\n,
 hotplug_status);
if (hotplug_trigger) {
-   hotplug_irq_storm_detect(dev, hotplug_trigger

[Intel-gfx] [PATCH] drm/i915: Fix SDVO connector and encoder get_hw_state functions

[Egbert Eich]

From: Egbert Eich e...@suse.de

The connector associated with the encoder is considered active when the
output associtated with this connector is active on the encoder. The
encoder itself is considered active when either there is an active
output on  it or the  respective SDVO channel is active.
Having active outputs when the SDVO channel is inactive seems to be
inconsistent: such states can be found when intel_modeset_setup_hw_state()
collects the hardware state set by the BIOS.
This inconsistency will be fixed in intel_sanitize_crtc()
(when intel_crtc_update_dpms() is called), this however only happens
when the encoder is associated with a crtc.

This patch also reverts:

 commit bd6946e87a98fea11907b2a47368e13044458a35
 Author: Daniel Vetter daniel.vet...@ffwll.ch
 Date:   Tue Apr 2 21:30:34 2013 +0200

 drm/i915: Fix sdvo connector get_hw_state function

Signed-off-by: Egbert Eich e...@suse.de
Suggested-By: Daniel Vetter daniel.vet...@ffwll.ch
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=63031
---
 drivers/gpu/drm/i915/intel_sdvo.c |9 +++--
 1 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sdvo.c 
b/drivers/gpu/drm/i915/intel_sdvo.c
index 298dc85..f6a9f4a 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -1231,12 +1231,8 @@ static bool intel_sdvo_connector_get_hw_state(struct 
intel_connector *connector)
struct intel_sdvo_connector *intel_sdvo_connector =
to_intel_sdvo_connector(connector-base);
struct intel_sdvo *intel_sdvo = intel_attached_sdvo(connector-base);
-   struct drm_i915_private *dev_priv = 
intel_sdvo-base.base.dev-dev_private;
u16 active_outputs;
 
-   if (!(I915_READ(intel_sdvo-sdvo_reg)  SDVO_ENABLE))
-   return false;
-
intel_sdvo_get_active_outputs(intel_sdvo, active_outputs);
 
if (active_outputs  intel_sdvo_connector-output_flag)
@@ -1251,11 +1247,13 @@ static bool intel_sdvo_get_hw_state(struct 
intel_encoder *encoder,
struct drm_device *dev = encoder-base.dev;
struct drm_i915_private *dev_priv = dev-dev_private;
struct intel_sdvo *intel_sdvo = to_intel_sdvo(encoder-base);
+   u16 active_outputs;
u32 tmp;
 
tmp = I915_READ(intel_sdvo-sdvo_reg);
+   intel_sdvo_get_active_outputs(intel_sdvo, active_outputs);
 
-   if (!(tmp  SDVO_ENABLE))
+   if (!(tmp  SDVO_ENABLE)  (active_outputs == 0))
return false;
 
if (HAS_PCH_CPT(dev))
@@ -2746,7 +2744,6 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t 
sdvo_reg, bool is_sdvob)
struct intel_sdvo *intel_sdvo;
u32 hotplug_mask;
int i;
-
intel_sdvo = kzalloc(sizeof(struct intel_sdvo), GFP_KERNEL);
if (!intel_sdvo)
return false;
-- 
1.7.7

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: implement ibx_hpd_irq_setup

[Egbert Eich]

Hi Daniel,

Daniel Vetter writes:
  On Fri, Mar 29, 2013 at 5:35 PM, Egbert Eich e...@suse.com wrote:
  
  Yeah, makes sense now that I think about it - I've simply didn't look
  ahead in your patch series while writing this little fixup ;-) Can you
  just re-add this when resending your patches again please?
  
Sure, I have prepared all the patches. I just wanted to give them
a try before sending them. 
Unfortunately I did not get around to do so over the Easter holidays.

Cheers,
Egbert.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: implement ibx_hpd_irq_setup

[Egbert Eich]

Sorry for replying so late, I wasn't able to task switch my brain
towards this when it was discussed:

Daniel Vetter writes:
  diff --git a/drivers/gpu/drm/i915/i915_irq.c 
  b/drivers/gpu/drm/i915/i915_irq.c
  index 43436e0..1279a44 100644
  --- a/drivers/gpu/drm/i915/i915_irq.c
  +++ b/drivers/gpu/drm/i915/i915_irq.c
  @@ -2084,7 +2084,7 @@ static void ibx_enable_hotplug(struct drm_device *dev)
   I915_WRITE(PCH_PORT_HOTPLUG, hotplug);
   }
   
  -static void ibx_irq_postinstall(struct drm_device *dev)
  +static void ibx_hpd_irq_setup(struct drm_device *dev)
   {
   drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev-dev_private;
   struct drm_mode_config *mode_config = dev-mode_config;
  @@ -2095,12 +2095,10 @@ static void ibx_irq_postinstall(struct drm_device 
  *dev)
   mask = ~SDE_HOTPLUG_MASK;
^^^
I'm missing those lines in the committed version of the patch.

   list_for_each_entry(intel_encoder, mode_config-encoder_list, 
  base.head)
   mask |= hpd_ibx[intel_encoder-hpd_pin];
  -mask |= SDE_GMBUS | SDE_AUX_MASK;
   } else {
   mask = ~SDE_HOTPLUG_MASK_CPT;
^^^
   list_for_each_entry(intel_encoder, mode_config-encoder_list, 
  base.head)
   mask |= hpd_cpt[intel_encoder-hpd_pin];
  -mask |= SDE_GMBUS_CPT | SDE_AUX_MASK_CPT;
   }
   I915_WRITE(SDEIIR, I915_READ(SDEIIR));

These are not really relevant in the present code, however they are
important once I've got the hotplug stuff refitted as one needs to be
able to turn off individual interrupts.
I'm going to prepare a commit for this and will send it with the hpd 
irq storm patches.

Cheers,
Egbert.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v.2 05/12] DRM/i915: Get rid if the 'hotplug_supported_mask' in struct drm_i915_private.

[Egbert Eich]

Daniel Vetter writes:
  On Mon, Feb 25, 2013 at 12:06:52PM -0500, Egbert Eich wrote:
   Now since we have replaced the bits to show interest in hotplug IRQs
   we can go and nuke the 'hotplug_supported_mask'.
   
   Signed-off-by: Egbert Eich e...@suse.de
  
  I've applied your patch up to this one. Patch four needed some manual
  convincing to fit correctly, but the new hpd infrastructure is now in
  place.
  
  To get the actual hpd irq storm detection going I think it'd be good to
  resend the remaining patches on top of latest drm-intel-nightly. I'll try
  to yell at people a bit harder to review things more timely this time
  around.
  
  Thanks for the patches.

Thanks guys for reviewing the patches. I will try to spare some time tonight
to look into the issues that came up. 
I got dragged into other tasks as well so I didn't even find the time to 
follow up on the patches myself.

For now I need to go back and debug yet another issue with SDVO on GM45 :(

Cheers,
Egbert.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v.3 06/12] DRM/i915: Add HPD IRQ storm detection (v3)

[Egbert Eich]

Add a hotplug IRQ storm detection (triggered when a hotplug interrupt
fires more than 5 times / sec).
Rationale:
Despite of the many attempts to fix the problem with noisy hotplug
interrupt lines we are still seeing systems which have issues:
Once cause of noise seems to be bad routing of the hotplug line
on the board: cross talk from other signals seems to cause erronous
hotplug interrupts. This has been documented as an erratum for the
the i945GM chipset and thus hotplug support was disabled for this
chipset model but others seem to have this problem, too.

We have seen this issue on a G35 motherboard for example:
Even different motherboards of the same model seem to behave
differently: while some only see only around 10-100 interrupts/s
others seem to see 5k or more.
We've also observed a dependency on the selected video mode.

Also on certain laptops interrupt noise seems to occur duing
battery charging when the battery is at a certain charge levels.

Thus we add a simple algorithm here that detects an 'interrupt storm'
condition.

v2: Fixed comment.
v3: Reordered drm_i915_private: moved hpd state tracking to hotplug work stuff.

Signed-off-by: Egbert Eich e...@suse.de
Acked-by: Chris Wilson ch...@chris-wilson.co.uk
---
 drivers/gpu/drm/i915/i915_drv.h |9 +
 drivers/gpu/drm/i915/i915_irq.c |   63 +++---
 2 files changed, 60 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d8604a6..296278f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -924,6 +924,15 @@ typedef struct drm_i915_private {
 
struct work_struct hotplug_work;
bool enable_hotplug_processing;
+   struct {
+   unsigned long hpd_last_jiffies;
+   int hpd_cnt;
+   enum {
+   HPD_ENABLED = 0,
+   HPD_DISABLED = 1,
+   HPD_MARK_DISABLED = 2
+   } hpd_mark;
+   } hpd_stats[HPD_NUM_PINS];
 
int num_pipe;
int num_pch_pll;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index c40c7cc..24cb6ed 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -578,6 +578,34 @@ static void gen6_queue_rps_work(struct drm_i915_private 
*dev_priv,
queue_work(dev_priv-wq, dev_priv-rps.work);
 }
 
+static inline void hotplug_irq_storm_detect(struct drm_device *dev,
+   u32 hotplug_trigger,
+   const u32 *hpd)
+{
+   drm_i915_private_t *dev_priv = dev-dev_private;
+   unsigned long irqflags;
+   int i;
+
+   spin_lock_irqsave(dev_priv-irq_lock, irqflags);
+
+   for (i = 1; i  HPD_NUM_PINS; i++) {
+   if ((hpd[i]  hotplug_trigger) 
+   dev_priv-hpd_stats[i].hpd_mark == HPD_ENABLED) {
+   if (jiffies  (dev_priv-hpd_stats[i].hpd_last_jiffies 
+ msecs_to_jiffies(1000)) ||
+   jiffies  dev_priv-hpd_stats[i].hpd_last_jiffies) {
+   dev_priv-hpd_stats[i].hpd_last_jiffies = 
jiffies;
+   dev_priv-hpd_stats[i].hpd_cnt = 0;
+   } else if (dev_priv-hpd_stats[i].hpd_cnt  5) {
+   dev_priv-hpd_stats[i].hpd_mark = 
HPD_MARK_DISABLED;
+   DRM_DEBUG_KMS(HPD interrupt storm detected on  
PIN %d\n, i);
+   } else
+   dev_priv-hpd_stats[i].hpd_cnt++;
+   }
+   }
+
+   spin_unlock_irqrestore(dev_priv-irq_lock, irqflags);
+}
+
 static void gmbus_irq_handler(struct drm_device *dev)
 {
struct drm_i915_private *dev_priv = (drm_i915_private_t *) 
dev-dev_private;
@@ -646,13 +674,15 @@ static irqreturn_t valleyview_irq_handler(int irq, void 
*arg)
/* Consume port.  Then clear IIR or we'll miss events */
if (iir  I915_DISPLAY_PORT_INTERRUPT) {
u32 hotplug_status = I915_READ(PORT_HOTPLUG_STAT);
+   u32 hotplug_trigger = hotplug_status  
HOTPLUG_INT_STATUS_I915;
 
DRM_DEBUG_DRIVER(hotplug event received, stat 
0x%08x\n,
 hotplug_status);
-   if (hotplug_status  HOTPLUG_INT_STATUS_I915)
+   if (hotplug_trigger) {
+   hotplug_irq_storm_detect(dev, hotplug_trigger, 
hpd_status_i915);
queue_work(dev_priv-wq,
   dev_priv-hotplug_work);
-
+   }
I915_WRITE(PORT_HOTPLUG_STAT, hotplug_status);
I915_READ(PORT_HOTPLUG_STAT);
}
@@ -676,10 +706,12 @@ static void ibx_irq_handler(struct drm_device *dev, u32 
pch_iir

[Intel-gfx] [PATCH v.2 10/12] DRM/i915: Add Reenable Timer to turn Hotplug Detection back on (v2).

[Egbert Eich]

We disable hoptplug detection when we encounter a hotplug event
storm. Still hotplug detection is required on some outputs (like
Display Port). The interrupt storm may be only temporary (on certain
Dell Laptops for instance it happens at certain charging states of
the system). Thus we enable it after a certain grace period (2 minutes).
Should the interrupt storm persist it will be detected immediately
and it will be disabled again.

v2: Reordered drm_i915_private: moved hotplug_reenable_timer to hpd state 
tracker.

Signed-off-by: Egbert Eich e...@suse.de
---
 drivers/gpu/drm/i915/i915_drv.h |2 +
 drivers/gpu/drm/i915/i915_irq.c |   53 ++-
 2 files changed, 54 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 296278f..1fb7c44 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -933,6 +933,8 @@ typedef struct drm_i915_private {
HPD_MARK_DISABLED = 2
} hpd_mark;
} hpd_stats[HPD_NUM_PINS];
+#define I915_REENABLE_HOTPLUG_DELAY (2*60*1000)
+   struct timer_list hotplug_reenable_timer;
 
int num_pipe;
int num_pch_pll;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index d11534c..c688b27 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -367,8 +367,11 @@ static void i915_hotplug_work_func(struct work_struct 
*work)
connector_disabled = true;
}
}
-   if (connector_disabled)
+   if (connector_disabled) {
drm_kms_helper_poll_enable(dev); /* if there were no outputs to 
poll, poll is disabled */
+   mod_timer(dev_priv-hotplug_reenable_timer,
+ jiffies + 
msecs_to_jiffies(I915_REENABLE_HOTPLUG_DELAY));
+   }
 
spin_unlock_irqrestore(dev_priv-irq_lock, irqflags);
 
@@ -2244,6 +2247,8 @@ static void valleyview_irq_uninstall(struct drm_device 
*dev)
if (!dev_priv)
return;
 
+   del_timer_sync(dev_priv-hotplug_reenable_timer);
+
for_each_pipe(pipe)
I915_WRITE(PIPESTAT(pipe), 0x);
 
@@ -2265,6 +2270,8 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
if (!dev_priv)
return;
 
+   del_timer_sync(dev_priv-hotplug_reenable_timer);
+
I915_WRITE(HWSTAM, 0x);
 
I915_WRITE(DEIMR, 0x);
@@ -2603,6 +2610,8 @@ static void i915_irq_uninstall(struct drm_device * dev)
drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev-dev_private;
int pipe;
 
+   del_timer_sync(dev_priv-hotplug_reenable_timer);
+
if (I915_HAS_HOTPLUG(dev)) {
I915_WRITE(PORT_HOTPLUG_EN, 0);
I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
@@ -2851,6 +2860,8 @@ static void i965_irq_uninstall(struct drm_device * dev)
if (!dev_priv)
return;
 
+   del_timer_sync(dev_priv-hotplug_reenable_timer);
+
I915_WRITE(PORT_HOTPLUG_EN, 0);
I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
 
@@ -2866,6 +2877,44 @@ static void i965_irq_uninstall(struct drm_device * dev)
I915_WRITE(IIR, I915_READ(IIR));
 }
 
+static void i915_reenable_hotplug_timer_func(unsigned long data)
+{
+   drm_i915_private_t *dev_priv = (drm_i915_private_t *)data;
+   struct drm_device *dev = dev_priv-dev;
+   struct drm_mode_config *mode_config = dev-mode_config;
+   unsigned long irqflags;
+   int i;
+
+   spin_lock_irqsave(dev_priv-irq_lock, irqflags);
+   for (i = 1; i  HPD_NUM_PINS; i++) {
+   if (dev_priv-hpd_stats[i].hpd_mark == HPD_MARK_DISABLED) {
+   struct drm_connector *connector;
+
+   dev_priv-hpd_stats[i].hpd_mark = HPD_ENABLED;
+   list_for_each_entry(connector, 
mode_config-connector_list, head) {
+   struct intel_connector *intel_connector = 
to_intel_connector(connector);
+
+   if (intel_connector-encoder-hpd_pin == i) {
+   if (connector-polled != 
intel_connector-polled)
+   DRM_DEBUG_DRIVER(Reenabling 
HPD on connector %s\n,
+
drm_get_connector_name(connector));
+   connector-polled = 
intel_connector-polled;
+   if (!connector-polled)
+   connector-polled = 
DRM_CONNECTOR_POLL_HPD;
+   }
+   }
+
+   if (IS_HASWELL(dev) ||
+   IS_IVYBRIDGE(dev) ||
+   (HAS_PCH_SPLIT(dev)))
+   ibx_hpd_irq_setup(dev

[Intel-gfx] [PATCH v.3 10/12] DRM/i915: Add Reenable Timer to turn Hotplug Detection back on (v3).

[Egbert Eich]

We disable hoptplug detection when we encounter a hotplug event
storm. Still hotplug detection is required on some outputs (like
Display Port). The interrupt storm may be only temporary (on certain
Dell Laptops for instance it happens at certain charging states of
the system). Thus we enable it after a certain grace period (2 minutes).
Should the interrupt storm persist it will be detected immediately
and it will be disabled again.

v2: Reordered drm_i915_private: moved hotplug_reenable_timer to hpd state 
tracker.
v3: Clarified loop start value,
Removed superfluous test for Ivybridge and Haswell,
Restructured loop to avoid deep nesting (all suggested by Ville Syrjälä)

Signed-off-by: Egbert Eich e...@suse.de
---
 drivers/gpu/drm/i915/i915_drv.h |2 +
 drivers/gpu/drm/i915/i915_irq.c |   53 ++-
 2 files changed, 54 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 296278f..1fb7c44 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -933,6 +933,8 @@ typedef struct drm_i915_private {
HPD_MARK_DISABLED = 2
} hpd_mark;
} hpd_stats[HPD_NUM_PINS];
+#define I915_REENABLE_HOTPLUG_DELAY (2*60*1000)
+   struct timer_list hotplug_reenable_timer;
 
int num_pipe;
int num_pch_pll;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index d11534c..76903af 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -367,8 +367,11 @@ static void i915_hotplug_work_func(struct work_struct 
*work)
connector_disabled = true;
}
}
-   if (connector_disabled)
+   if (connector_disabled) {
drm_kms_helper_poll_enable(dev); /* if there were no outputs to 
poll, poll is disabled */
+   mod_timer(dev_priv-hotplug_reenable_timer,
+ jiffies + 
msecs_to_jiffies(I915_REENABLE_HOTPLUG_DELAY));
+   }
 
spin_unlock_irqrestore(dev_priv-irq_lock, irqflags);
 
@@ -2244,6 +2247,8 @@ static void valleyview_irq_uninstall(struct drm_device 
*dev)
if (!dev_priv)
return;
 
+   del_timer_sync(dev_priv-hotplug_reenable_timer);
+
for_each_pipe(pipe)
I915_WRITE(PIPESTAT(pipe), 0x);
 
@@ -2265,6 +2270,8 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
if (!dev_priv)
return;
 
+   del_timer_sync(dev_priv-hotplug_reenable_timer);
+
I915_WRITE(HWSTAM, 0x);
 
I915_WRITE(DEIMR, 0x);
@@ -2603,6 +2610,8 @@ static void i915_irq_uninstall(struct drm_device * dev)
drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev-dev_private;
int pipe;
 
+   del_timer_sync(dev_priv-hotplug_reenable_timer);
+
if (I915_HAS_HOTPLUG(dev)) {
I915_WRITE(PORT_HOTPLUG_EN, 0);
I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
@@ -2851,6 +2860,8 @@ static void i965_irq_uninstall(struct drm_device * dev)
if (!dev_priv)
return;
 
+   del_timer_sync(dev_priv-hotplug_reenable_timer);
+
I915_WRITE(PORT_HOTPLUG_EN, 0);
I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
 
@@ -2866,6 +2877,44 @@ static void i965_irq_uninstall(struct drm_device * dev)
I915_WRITE(IIR, I915_READ(IIR));
 }
 
+static void i915_reenable_hotplug_timer_func(unsigned long data)
+{
+   drm_i915_private_t *dev_priv = (drm_i915_private_t *)data;
+   struct drm_device *dev = dev_priv-dev;
+   struct drm_mode_config *mode_config = dev-mode_config;
+   unsigned long irqflags;
+   int i;
+
+   spin_lock_irqsave(dev_priv-irq_lock, irqflags);
+   for (i = (HPD_NONE + 1); i  HPD_NUM_PINS; i++) {
+   struct drm_connector *connector;
+
+   if (dev_priv-hpd_stats[i].hpd_mark != HPD_MARK_DISABLED)
+   continue;
+
+   dev_priv-hpd_stats[i].hpd_mark = HPD_ENABLED;
+
+   list_for_each_entry(connector, mode_config-connector_list, 
head) {
+   struct intel_connector *intel_connector = 
to_intel_connector(connector);
+
+   if (intel_connector-encoder-hpd_pin == i) {
+   if (connector-polled != 
intel_connector-polled)
+   DRM_DEBUG_DRIVER(Reenabling HPD on 
connector %s\n,
+
drm_get_connector_name(connector));
+   connector-polled = intel_connector-polled;
+   if (!connector-polled)
+   connector-polled = 
DRM_CONNECTOR_POLL_HPD;
+   }
+   }
+
+   if (HAS_PCH_SPLIT(dev))
+   ibx_hpd_irq_setup

[Intel-gfx] [PATCH v.2 09/12] DRM/i915: Disable HPD interrupt on pin when irq storm is detected (v2)

[Egbert Eich]

This patch disables hotplug interrupts if an 'interrupt storm'
has been detected.
Noise on the interrupt line renders the hotplug interrupt useless:
each hotplug event causes the devices to be rescanned which will
will only increase the system load.
Thus disable the hotplug interrupts and fall back to periodic
device polling.

v2: Fixed cleanup typo.

Signed-off-by: Egbert Eich e...@suse.de
---
 drivers/gpu/drm/i915/i915_irq.c |   69 ++-
 1 files changed, 53 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index d9c99d7..0abef6a 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -87,7 +87,8 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview 
are the same */
[HPD_PORT_D] = PORTD_HOTPLUG_INT_STATUS
 };
 
-
+static void ibx_hpd_irq_setup(struct drm_device *dev);
+static void i915_hpd_irq_setup(struct drm_device *dev);
 
 /* For display hotplug interrupt */
 static void
@@ -338,7 +339,11 @@ static void i915_hotplug_work_func(struct work_struct 
*work)
hotplug_work);
struct drm_device *dev = dev_priv-dev;
struct drm_mode_config *mode_config = dev-mode_config;
-   struct intel_encoder *encoder;
+   struct intel_connector *intel_connector;
+   struct intel_encoder *intel_encoder;
+   struct drm_connector *connector;
+   unsigned long irqflags;
+   bool connector_disabled = false;
 
/* HPD irq before everything is fully set up. */
if (!dev_priv-enable_hotplug_processing)
@@ -347,9 +352,29 @@ static void i915_hotplug_work_func(struct work_struct 
*work)
mutex_lock(mode_config-mutex);
DRM_DEBUG_KMS(running encoder hotplug functions\n);
 
-   list_for_each_entry(encoder, mode_config-encoder_list, base.head)
-   if (encoder-hot_plug)
-   encoder-hot_plug(encoder);
+   spin_lock_irqsave(dev_priv-irq_lock, irqflags);
+   list_for_each_entry(connector, mode_config-connector_list, head) {
+   intel_connector = to_intel_connector(connector);
+   intel_encoder = intel_connector-encoder;
+   if (intel_encoder-hpd_pin  HPD_NONE 
+   dev_priv-hpd_stats[intel_encoder-hpd_pin].hpd_mark == 
HPD_MARK_DISABLED 
+   connector-polled == DRM_CONNECTOR_POLL_HPD) {
+   pr_warn(HPD interrupt storm detected on connector %s 
disabling\n,
+   drm_get_connector_name(connector));
+   dev_priv-hpd_stats[intel_encoder-hpd_pin].hpd_mark = 
HPD_DISABLED;
+   connector-polled = DRM_CONNECTOR_POLL_CONNECT
+   | DRM_CONNECTOR_POLL_DISCONNECT;
+   connector_disabled = true;
+   }
+   }
+   if (connector_disabled)
+   drm_kms_helper_poll_enable(dev); /* if there were no outputs to 
poll, poll is disabled */
+
+   spin_unlock_irqrestore(dev_priv-irq_lock, irqflags);
+
+   list_for_each_entry(intel_encoder, mode_config-encoder_list, 
base.head)
+   if (intel_encoder-hot_plug)
+   intel_encoder-hot_plug(intel_encoder);
 
mutex_unlock(mode_config-mutex);
 
@@ -578,13 +603,14 @@ static void gen6_queue_rps_work(struct drm_i915_private 
*dev_priv,
queue_work(dev_priv-wq, dev_priv-rps.work);
 }
 
-static inline void hotplug_irq_storm_detect(struct drm_device *dev,
+static inline bool hotplug_irq_storm_detect(struct drm_device *dev,
u32 hotplug_trigger,
const u32 *hpd)
 {
drm_i915_private_t *dev_priv = dev-dev_private;
unsigned long irqflags;
int i;
+   bool ret = false;
 
spin_lock_irqsave(dev_priv-irq_lock, irqflags);
 
@@ -598,12 +624,15 @@ static inline void hotplug_irq_storm_detect(struct 
drm_device *dev,
} else if (dev_priv-hpd_stats[i].hpd_cnt  5) {
dev_priv-hpd_stats[i].hpd_mark = 
HPD_MARK_DISABLED;
DRM_DEBUG_KMS(HPD interrupt storm detected on  
PIN %d\n, i);
+   ret = true;
} else
dev_priv-hpd_stats[i].hpd_cnt++;
}
}
 
spin_unlock_irqrestore(dev_priv-irq_lock, irqflags);
+
+   return ret;
 }
 
 static void gmbus_irq_handler(struct drm_device *dev)
@@ -679,7 +708,8 @@ static irqreturn_t valleyview_irq_handler(int irq, void 
*arg)
DRM_DEBUG_DRIVER(hotplug event received, stat 
0x%08x\n,
 hotplug_status);
if (hotplug_trigger) {
-   hotplug_irq_storm_detect(dev, hotplug_trigger, 
hpd_status_i915

[Intel-gfx] [PATCH v.3 11/12] DRM/i915: Add bit field to record which pins have received HPD events (v3)

[Egbert Eich]

This way it is possible to limit 're'-detect() of displays to connectors
which have received an HPD event.

v2: Reordered drm_i915_private: Move hpd_event_bits to hpd state tracking.
v3: Fix patch.

Signed-off-by: Egbert Eich e...@suse.de
---
 drivers/gpu/drm/i915/i915_drv.h |1 +
 drivers/gpu/drm/i915/i915_irq.c |   47 ++-
 2 files changed, 42 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1fb7c44..d48d1e6 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -933,6 +933,7 @@ typedef struct drm_i915_private {
HPD_MARK_DISABLED = 2
} hpd_mark;
} hpd_stats[HPD_NUM_PINS];
+   u32 hpd_event_bits;
 #define I915_REENABLE_HOTPLUG_DELAY (2*60*1000)
struct timer_list hotplug_reenable_timer;
 
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index fd68b21..9e8d5b4 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -330,6 +330,24 @@ static int i915_get_vblank_timestamp(struct drm_device 
*dev, int pipe,
 crtc);
 }
 
+/**
+ * drm_helper_hpd_irq_single_connector_event() - call this function with 
mode_config.mutex lock held
+ */
+
+static int intel_hpd_irq_single_connector_event(struct drm_device *dev, struct 
drm_connector *connector)
+{
+   enum drm_connector_status old_status;
+
+   old_status = connector-status;
+
+   connector-status = connector-funcs-detect(connector, false);
+   DRM_DEBUG_KMS([CONNECTOR:%d:%s] status updated from %d to %d\n,
+ connector-base.id,
+ drm_get_connector_name(connector),
+ old_status, connector-status);
+   return (old_status != connector-status);
+}
+
 /*
  * Handle hotplug events outside the interrupt handler proper.
  */
@@ -344,6 +362,8 @@ static void i915_hotplug_work_func(struct work_struct *work)
struct drm_connector *connector;
unsigned long irqflags;
bool connector_disabled = false;
+   bool changed = false;
+   u32 hpd_event_bits;
 
/* HPD irq before everything is fully set up. */
if (!dev_priv-enable_hotplug_processing)
@@ -353,6 +373,9 @@ static void i915_hotplug_work_func(struct work_struct *work)
DRM_DEBUG_KMS(running encoder hotplug functions\n);
 
spin_lock_irqsave(dev_priv-irq_lock, irqflags);
+
+   hpd_event_bits = dev_priv-hpd_event_bits;
+   dev_priv-hpd_event_bits = 0;
list_for_each_entry(connector, mode_config-connector_list, head) {
intel_connector = to_intel_connector(connector);
intel_encoder = intel_connector-encoder;
@@ -366,6 +389,10 @@ static void i915_hotplug_work_func(struct work_struct 
*work)
| DRM_CONNECTOR_POLL_DISCONNECT;
connector_disabled = true;
}
+   if (hpd_event_bits  (1  intel_encoder-hpd_pin)) {
+   DRM_DEBUG_KMS(Connector %s (pin %i) received hotplug 
event.\n,
+ drm_get_connector_name(connector), 
intel_encoder-hpd_pin);
+   }
}
if (connector_disabled) {
drm_kms_helper_poll_enable(dev); /* if there were no outputs to 
poll, poll is disabled */
@@ -375,14 +402,20 @@ static void i915_hotplug_work_func(struct work_struct 
*work)
 
spin_unlock_irqrestore(dev_priv-irq_lock, irqflags);
 
-   list_for_each_entry(intel_encoder, mode_config-encoder_list, 
base.head)
-   if (intel_encoder-hot_plug)
-   intel_encoder-hot_plug(intel_encoder);
-
+   list_for_each_entry(connector, mode_config-connector_list, head) {
+   intel_connector = to_intel_connector(connector);
+   intel_encoder = intel_connector-encoder;
+   if (hpd_event_bits  (1  intel_encoder-hpd_pin)) {
+   if (intel_encoder-hot_plug)
+   intel_encoder-hot_plug(intel_encoder);
+   if (intel_hpd_irq_single_connector_event(dev, 
connector))
+   changed = true;
+   }
+   }
mutex_unlock(mode_config-mutex);
 
-   /* Just fire off a uevent and let userspace tell us what to do */
-   drm_helper_hpd_irq_event(dev);
+   if (changed)
+   drm_kms_helper_hotplug_event(dev);
 }
 
 static void ironlake_handle_rps_change(struct drm_device *dev)
@@ -620,12 +653,14 @@ static inline bool hotplug_irq_storm_detect(struct 
drm_device *dev,
for (i = 1; i  HPD_NUM_PINS; i++) {
if ((hpd[i]  hotplug_trigger) 
dev_priv-hpd_stats[i].hpd_mark == HPD_ENABLED) {
+   dev_priv-hpd_event_bits |= (1  i);
if (jiffies  (dev_priv