[Intel-gfx] [PATCH] lib/drmtest: skip suspend tests in simulation

[Daniel Vetter]

The simulator doesn't like this nor really support it :(

Cc: Ben Widawsky 
Cc: Ville Syrjälä 
Cc: Damien Lespiau 
Signed-off-by: Daniel Vetter 
---
 lib/drmtest.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/drmtest.c b/lib/drmtest.c
index 9d4c5da..509db13 100644
--- a/lib/drmtest.c
+++ b/lib/drmtest.c
@@ -2088,6 +2088,8 @@ void igt_system_suspend_autoresume(void)
 {
int ret;
 
+   igt_skip_on_simulation();
+
ret = system("rtcwake -s 30 -m mem");
igt_assert(ret == 0);
 }
-- 
1.8.4.rc3

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

Re: [Intel-gfx] [PATCH] lib/drmtest: skip suspend tests in simulation

[Damien Lespiau]

On Thu, Sep 12, 2013 at 02:02:30PM +0200, Daniel Vetter wrote:
> The simulator doesn't like this nor really support it :(
> 
> Cc: Ben Widawsky 
> Cc: Ville Syrjälä 
> Cc: Damien Lespiau 
> Signed-off-by: Daniel Vetter 

Aye! (I recall we don't really need that for i-g-t, but can't hurt)

Reviewed-by: Damien Lespiau 

-- 
Damien

> ---
>  lib/drmtest.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/lib/drmtest.c b/lib/drmtest.c
> index 9d4c5da..509db13 100644
> --- a/lib/drmtest.c
> +++ b/lib/drmtest.c
> @@ -2088,6 +2088,8 @@ void igt_system_suspend_autoresume(void)
>  {
>   int ret;
>  
> + igt_skip_on_simulation();
> +
>   ret = system("rtcwake -s 30 -m mem");
>   igt_assert(ret == 0);
>  }
> -- 
> 1.8.4.rc3
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] lib/drmtest: skip suspend tests in simulation

[Daniel Vetter]

On Thu, Sep 12, 2013 at 2:30 PM, Damien Lespiau
 wrote:
> On Thu, Sep 12, 2013 at 02:02:30PM +0200, Daniel Vetter wrote:
>> The simulator doesn't like this nor really support it :(
>>
>> Cc: Ben Widawsky 
>> Cc: Ville Syrjälä 
>> Cc: Damien Lespiau 
>> Signed-off-by: Daniel Vetter 
>
> Aye! (I recall we don't really need that for i-g-t, but can't hurt)
>
> Reviewed-by: Damien Lespiau 

Actually on second thought we should try to at least run our driver
suspend/resume code a bit, maybe using the pm debug stuff in sysfs,
i.e.

echo devices > /sys/power/pm_test
echo mem > /sys/power/state

That should avoid any issues due to the platform being bonghits. And
we could use it even on really early SDVs when the firmware code isn't
stable yet (or other drivers not yet up to the task).

Comments?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 0/5] drm/i915: VGA vs. power well

[ville . syrjala]

Match of a lifetime! In the red corner VGA, the underdog. In the blue corner,
the killer of registers, the bane of memory access, the POWER WELL! Who will
win?

This series eliminates the unclaimed register access warning introduced by
the delayed VGA memory disable. And now we actually disable VGA memory decode
on machines where we turn the power well off during init, making it slightly
more probable that vga arbitration will work.

Fixing pc8 entry when the last power well reference disappears is left as
an exercise for the reader.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 5/5] drm/i915: Fix unclaimed register access due to delayed VGA memroy disable

[ville . syrjala]

From: Ville Syrjälä 

VGA registers live inside the power well on HSW, so in order to write
the VGA MSR register we need the power well to be on.

We really must write to the register to properly clear the
VGA_MSR_MEM_EN enable bit, even if all VGA registers get zeroed when
the power well is down. It seems that the implicit zeroing done by
the power well is not enough to propagate the VGA_MSR_MEM_EN bit to
whomever is actually responsible for the memory decode ranges.

If we leave VGA memory decode enabled, and then turn off the power well,
all VGA memory reads will return zeroes. But if we first disable VGA
memory deocde and then turn off the power well, VGA memory reads
return all ones, indicating that the access wasn't claimed by anyone.
For the vga arbiter to function correctly the IGD must not claim the
VGA memory accesses.

Previously we were doing the VGA_MSR register access while the power well
was excplicitly powered up during driver init. But ever since

 commit 6e1b4fdad5157bb9e88777d525704aba24389bee
 Author: Ville Syrjälä 
 Date:   Thu Sep 5 20:40:52 2013 +0300

drm/i915: Delay disabling of VGA memory until vgacon->fbcon handoff is done

we delay the VGA memory disable until fbcon has initialized, and so
there's a possibility that the power well got turned off during the
fbcon modeset. Also vgacon_save_screen() will need the power well to be
on to be able to read the VGA memory.

So immediately after enabling the power well during init grab a refence
for VGA purposes, and after all the VGA handling is done, release it.

Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/i915/i915_dma.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index e5c7b10..23f965d 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1326,6 +1326,9 @@ static int i915_load_modeset_init(struct drm_device *dev)
 
intel_init_power_well(dev);
 
+   /* Keep VGA alive until i915_disable_vga_mem() */
+   intel_display_power_get(dev, POWER_DOMAIN_VGA);
+
intel_modeset_gem_init(dev);
 
/* Always safe in the mode setting case. */
@@ -1358,6 +1361,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
 * vgacon_save_screen() works during the handover.
 */
i915_disable_vga_mem(dev);
+   intel_display_power_put(dev, POWER_DOMAIN_VGA);
 
/* Only enable hotplug handling once the fbdev is fully set up. */
dev_priv->enable_hotplug_processing = true;
-- 
1.8.1.5

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

[Intel-gfx] [PATCH 4/5] drm/i915: Pull intel_init_power_well() out of intel_modeset_init_hw()

[ville . syrjala]

From: Ville Syrjälä 

The init and resume codepaths want to handel the power well in slightly
different ways, so pull the power well init out from
intel_modeset_init_hw() which gets called in both cases.

Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/i915/i915_dma.c  | 2 ++
 drivers/gpu/drm/i915/i915_drv.c  | 2 ++
 drivers/gpu/drm/i915/intel_display.c | 2 --
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 9b265a4..e5c7b10 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1324,6 +1324,8 @@ static int i915_load_modeset_init(struct drm_device *dev)
 
INIT_WORK(&dev_priv->console_resume_work, intel_console_resume);
 
+   intel_init_power_well(dev);
+
intel_modeset_gem_init(dev);
 
/* Always safe in the mode setting case. */
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index ec690ca..cd5a66d 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -596,6 +596,8 @@ static int __i915_drm_thaw(struct drm_device *dev)
/* We need working interrupts for modeset enabling ... */
drm_irq_install(dev);
 
+   intel_init_power_well(dev);
+
intel_modeset_init_hw(dev);
 
drm_modeset_lock_all(dev);
diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index d06e3b4..53f5a1f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10343,8 +10343,6 @@ void i915_disable_vga_mem(struct drm_device *dev)
 
 void intel_modeset_init_hw(struct drm_device *dev)
 {
-   intel_init_power_well(dev);
-
intel_prepare_ddi(dev);
 
intel_init_clock_gating(dev);
-- 
1.8.1.5

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

[Intel-gfx] [PATCH 2/5] drm/i915: Refactor power well refcount inc/dec operations

[ville . syrjala]

From: Ville Syrjälä 

We increase/decrease the power well refcount in several places now, and
all of those places need to do the same thing, so pull that code into
a few small helper functions.

Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/i915/intel_pm.c | 39 +--
 1 file changed, 21 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 4962303..1aa604c 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5333,6 +5333,19 @@ static void __intel_set_power_well(struct drm_device 
*dev, bool enable)
}
 }
 
+static void __intel_inc_power_well_refcount(struct i915_power_well *power_well)
+{
+   if (!power_well->count++)
+   __intel_set_power_well(power_well->device, true);
+}
+
+static void __intel_dec_power_well_refcount(struct i915_power_well *power_well)
+{
+   WARN_ON(!power_well->count);
+   if (!--power_well->count)
+   __intel_set_power_well(power_well->device, false);
+}
+
 void intel_display_power_get(struct drm_device *dev,
 enum intel_display_power_domain domain)
 {
@@ -5355,8 +5368,7 @@ void intel_display_power_get(struct drm_device *dev,
case POWER_DOMAIN_TRANSCODER_B:
case POWER_DOMAIN_TRANSCODER_C:
spin_lock_irq(&power_well->lock);
-   if (!power_well->count++)
-   __intel_set_power_well(power_well->device, true);
+   __intel_inc_power_well_refcount(power_well);
spin_unlock_irq(&power_well->lock);
return;
default:
@@ -5386,9 +5398,7 @@ void intel_display_power_put(struct drm_device *dev,
case POWER_DOMAIN_TRANSCODER_B:
case POWER_DOMAIN_TRANSCODER_C:
spin_lock_irq(&power_well->lock);
-   WARN_ON(!power_well->count);
-   if (!--power_well->count)
-   __intel_set_power_well(power_well->device, false);
+   __intel_dec_power_well_refcount(power_well);
spin_unlock_irq(&power_well->lock);
return;
default:
@@ -5405,8 +5415,7 @@ void i915_request_power_well(void)
return;
 
spin_lock_irq(&hsw_pwr->lock);
-   if (!hsw_pwr->count++)
-   __intel_set_power_well(hsw_pwr->device, true);
+   __intel_inc_power_well_refcount(hsw_pwr);
spin_unlock_irq(&hsw_pwr->lock);
 }
 EXPORT_SYMBOL_GPL(i915_request_power_well);
@@ -5418,9 +5427,7 @@ void i915_release_power_well(void)
return;
 
spin_lock_irq(&hsw_pwr->lock);
-   WARN_ON(!hsw_pwr->count);
-   if (!--hsw_pwr->count)
-   __intel_set_power_well(hsw_pwr->device, false);
+   __intel_dec_power_well_refcount(hsw_pwr);
spin_unlock_irq(&hsw_pwr->lock);
 }
 EXPORT_SYMBOL_GPL(i915_release_power_well);
@@ -5467,14 +5474,10 @@ void intel_set_power_well(struct drm_device *dev, bool 
enable)
 
power_well->i915_request = enable;
 
-   if (enable) {
-   if (!power_well->count++)
-   __intel_set_power_well(dev, true);
-   } else {
-   WARN_ON(!power_well->count);
-   if (!--power_well->count)
-   __intel_set_power_well(dev, false);
-   }
+   if (enable)
+   __intel_inc_power_well_refcount(power_well);
+   else
+   __intel_dec_power_well_refcount(power_well);
 
  out:
spin_unlock_irq(&power_well->lock);
-- 
1.8.1.5

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

[Intel-gfx] [PATCH 3/5] drm/i915: Add POWER_DOMAIN_VGA

[ville . syrjala]

From: Ville Syrjälä 

VGA registers/memory live inside the the display power well. Add a power
domain for VGA.

Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/i915/i915_drv.h | 1 +
 drivers/gpu/drm/i915/intel_pm.c | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 152291c..54b1966 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -99,6 +99,7 @@ enum intel_display_power_domain {
POWER_DOMAIN_TRANSCODER_B,
POWER_DOMAIN_TRANSCODER_C,
POWER_DOMAIN_TRANSCODER_EDP = POWER_DOMAIN_TRANSCODER_A + 0xF,
+   POWER_DOMAIN_VGA,
 };
 
 #define POWER_DOMAIN_PIPE(pipe) ((pipe) + POWER_DOMAIN_PIPE_A)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 1aa604c..f08fe52 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5271,6 +5271,7 @@ bool intel_display_power_enabled(struct drm_device *dev,
case POWER_DOMAIN_PIPE_A:
case POWER_DOMAIN_TRANSCODER_EDP:
return true;
+   case POWER_DOMAIN_VGA:
case POWER_DOMAIN_PIPE_B:
case POWER_DOMAIN_PIPE_C:
case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
@@ -5359,6 +5360,7 @@ void intel_display_power_get(struct drm_device *dev,
case POWER_DOMAIN_PIPE_A:
case POWER_DOMAIN_TRANSCODER_EDP:
return;
+   case POWER_DOMAIN_VGA:
case POWER_DOMAIN_PIPE_B:
case POWER_DOMAIN_PIPE_C:
case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
@@ -5389,6 +5391,7 @@ void intel_display_power_put(struct drm_device *dev,
case POWER_DOMAIN_PIPE_A:
case POWER_DOMAIN_TRANSCODER_EDP:
return;
+   case POWER_DOMAIN_VGA:
case POWER_DOMAIN_PIPE_B:
case POWER_DOMAIN_PIPE_C:
case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
-- 
1.8.1.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: Add intel_display_power_{get, put} to request power for specific domains

[ville . syrjala]

From: Ville Syrjälä 

Add APIs to get/put power well references for specific purposes.

Also reorganize the internal i915_request power well handling to use the
reference count just like everyone else. This way all we need to do is
check the reference count and we know whether the power well needs to be
enabled of disabled.

Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/i915/intel_drv.h |  4 ++
 drivers/gpu/drm/i915/intel_pm.c  | 92 
 2 files changed, 87 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 774ebb6..2ecd3d2 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -763,6 +763,10 @@ extern void i915_remove_power_well(struct drm_device *dev);
 
 extern bool intel_display_power_enabled(struct drm_device *dev,
enum intel_display_power_domain domain);
+extern void intel_display_power_get(struct drm_device *dev,
+   enum intel_display_power_domain domain);
+extern void intel_display_power_put(struct drm_device *dev,
+   enum intel_display_power_domain domain);
 extern void intel_init_power_well(struct drm_device *dev);
 extern void intel_set_power_well(struct drm_device *dev, bool enable);
 extern void intel_enable_gt_powersave(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 8cffef4..4962303 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5333,6 +5333,69 @@ static void __intel_set_power_well(struct drm_device 
*dev, bool enable)
}
 }
 
+void intel_display_power_get(struct drm_device *dev,
+enum intel_display_power_domain domain)
+{
+   struct drm_i915_private *dev_priv = dev->dev_private;
+   struct i915_power_well *power_well = &dev_priv->power_well;
+
+   if (!HAS_POWER_WELL(dev))
+   return;
+
+   switch (domain) {
+   case POWER_DOMAIN_PIPE_A:
+   case POWER_DOMAIN_TRANSCODER_EDP:
+   return;
+   case POWER_DOMAIN_PIPE_B:
+   case POWER_DOMAIN_PIPE_C:
+   case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
+   case POWER_DOMAIN_PIPE_B_PANEL_FITTER:
+   case POWER_DOMAIN_PIPE_C_PANEL_FITTER:
+   case POWER_DOMAIN_TRANSCODER_A:
+   case POWER_DOMAIN_TRANSCODER_B:
+   case POWER_DOMAIN_TRANSCODER_C:
+   spin_lock_irq(&power_well->lock);
+   if (!power_well->count++)
+   __intel_set_power_well(power_well->device, true);
+   spin_unlock_irq(&power_well->lock);
+   return;
+   default:
+   BUG();
+   }
+}
+
+void intel_display_power_put(struct drm_device *dev,
+enum intel_display_power_domain domain)
+{
+   struct drm_i915_private *dev_priv = dev->dev_private;
+   struct i915_power_well *power_well = &dev_priv->power_well;
+
+   if (!HAS_POWER_WELL(dev))
+   return;
+
+   switch (domain) {
+   case POWER_DOMAIN_PIPE_A:
+   case POWER_DOMAIN_TRANSCODER_EDP:
+   return;
+   case POWER_DOMAIN_PIPE_B:
+   case POWER_DOMAIN_PIPE_C:
+   case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
+   case POWER_DOMAIN_PIPE_B_PANEL_FITTER:
+   case POWER_DOMAIN_PIPE_C_PANEL_FITTER:
+   case POWER_DOMAIN_TRANSCODER_A:
+   case POWER_DOMAIN_TRANSCODER_B:
+   case POWER_DOMAIN_TRANSCODER_C:
+   spin_lock_irq(&power_well->lock);
+   WARN_ON(!power_well->count);
+   if (!--power_well->count)
+   __intel_set_power_well(power_well->device, false);
+   spin_unlock_irq(&power_well->lock);
+   return;
+   default:
+   BUG();
+   }
+}
+
 static struct i915_power_well *hsw_pwr;
 
 /* Display audio driver power well request */
@@ -5342,8 +5405,7 @@ void i915_request_power_well(void)
return;
 
spin_lock_irq(&hsw_pwr->lock);
-   if (!hsw_pwr->count++ &&
-   !hsw_pwr->i915_request)
+   if (!hsw_pwr->count++)
__intel_set_power_well(hsw_pwr->device, true);
spin_unlock_irq(&hsw_pwr->lock);
 }
@@ -5357,8 +5419,7 @@ void i915_release_power_well(void)
 
spin_lock_irq(&hsw_pwr->lock);
WARN_ON(!hsw_pwr->count);
-   if (!--hsw_pwr->count &&
-  !hsw_pwr->i915_request)
+   if (!--hsw_pwr->count)
__intel_set_power_well(hsw_pwr->device, false);
spin_unlock_irq(&hsw_pwr->lock);
 }
@@ -5394,15 +5455,28 @@ void intel_set_power_well(struct drm_device *dev, bool 
enable)
return;
 
spin_lock_irq(&power_well->lock);
+
+   /*
+* This function will only ever contribute one
+* to the power well reference count. i915_request
+* is what tracks w

Re: [Intel-gfx] [BUG] completely bonkers use of set_need_resched + VM_FAULT_NOPAGE

[Maarten Lankhorst]

Op 12-09-13 17:06, Peter Zijlstra schreef:
> Hi Dave,
>
> So I'm poking around the preemption code and stumbled upon:
>
> drivers/gpu/drm/i915/i915_gem.c:set_need_resched();
> drivers/gpu/drm/ttm/ttm_bo_vm.c:set_need_resched();
> drivers/gpu/drm/ttm/ttm_bo_vm.c:set_need_resched();
> drivers/gpu/drm/udl/udl_gem.c:  set_need_resched();
>
> All these sites basically do:
>
>   while (!trylock())
>   yield();
>
> which is a horrible and broken locking pattern. 
>
> Firstly its deadlock prone, suppose the faulting process is a FIFOn+1
> task that preempted the lock holder at FIFOn.
>
> Secondly the implementation is worse than usual by abusing
> VM_FAULT_NOPAGE, which is supposed to install a PTE so that the fault
> doesn't retry, but you're using it as a get out of fault path. And
> you're using set_need_resched() which is not something a driver should
> _ever_ touch.
>
> Now I'm going to take away set_need_resched() -- and while you can
> 'reimplement' it using set_thread_flag() you're not going to do that
> because it will be broken due to changes to the preempt code.
>
> So please as to fix ASAP and don't allow anybody to trick you into
> merging silly things like that again ;-)
>
Agreed, but this is a case of locking inversion. How do you propose to get 
around that?

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

Re: [Intel-gfx] [BUG] completely bonkers use of set_need_resched + VM_FAULT_NOPAGE

[Daniel Vetter]

On Thu, Sep 12, 2013 at 5:06 PM, Peter Zijlstra  wrote:
>
> So I'm poking around the preemption code and stumbled upon:
>
> drivers/gpu/drm/i915/i915_gem.c:set_need_resched();
> drivers/gpu/drm/ttm/ttm_bo_vm.c:set_need_resched();
> drivers/gpu/drm/ttm/ttm_bo_vm.c:set_need_resched();
> drivers/gpu/drm/udl/udl_gem.c:  set_need_resched();
>
> All these sites basically do:
>
>   while (!trylock())
> yield();
>
> which is a horrible and broken locking pattern.
>
> Firstly its deadlock prone, suppose the faulting process is a FIFOn+1
> task that preempted the lock holder at FIFOn.
>
> Secondly the implementation is worse than usual by abusing
> VM_FAULT_NOPAGE, which is supposed to install a PTE so that the fault
> doesn't retry, but you're using it as a get out of fault path. And
> you're using set_need_resched() which is not something a driver should
> _ever_ touch.
>
> Now I'm going to take away set_need_resched() -- and while you can
> 'reimplement' it using set_thread_flag() you're not going to do that
> because it will be broken due to changes to the preempt code.
>
> So please as to fix ASAP and don't allow anybody to trick you into
> merging silly things like that again ;-)

The set_need_resched in i915_gem.c:i915_gem_fault can actually be
removed. It was there to give the error handler a chance to sneak in
and reset the hw/sw tracking when the gpu is dead. That hack goes back
to the days when the locking around our error handler was somewhere
between nonexistent and totally broken, nowadays we keep things from
live-locking by a bit of magic in i915_mutex_lock_interruptible. I'll
whip up a patch to rip this out. I'll also check that our testsuite
properly exercises this path (needs a bit of work on a quick look for
better coverage).

The one in ttm is just bonghits to shut up lockdep: ttm can recurse
into it's own pagefault handler and then deadlock, the trylock just
keeps lockdep quiet. We've had that bug arise in drm/i915 due to some
fun userspace did and now have testcases for them. The right solution
to fix this is to use copy_to|from_user_atomic in ttm everywhere it
holds locks and have slowpaths which drops locks, copies stuff into a
temp allocation and then continues. At least that's how we've fixed
all those inversions in i915-gem. I'm not volunteering to fix this ;-)

The one in udl just looks like copypasta from i915, without any
justification (at least I don't see any) for why it's there. Probably
can die, too, since there isn't any gpu to reset on usb display-link
devices ...

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [BUG] completely bonkers use of set_need_resched + VM_FAULT_NOPAGE

[Daniel Vetter]

On Thu, Sep 12, 2013 at 5:43 PM, Peter Zijlstra  wrote:
>> The one in ttm is just bonghits to shut up lockdep: ttm can recurse
>> into it's own pagefault handler and then deadlock, the trylock just
>> keeps lockdep quiet. We've had that bug arise in drm/i915 due to some
>> fun userspace did and now have testcases for them. The right solution
>> to fix this is to use copy_to|from_user_atomic in ttm everywhere it
>> holds locks and have slowpaths which drops locks, copies stuff into a
>> temp allocation and then continues. At least that's how we've fixed
>> all those inversions in i915-gem. I'm not volunteering to fix this ;-)
>
> Yikes.. so how common is it? If I simply rip the set_need_resched() out
> it will 'spin' on the fault a little longer until a 'natural' preemption
> point -- if such a thing is every going to happen.

It's a case of "our userspace doesn't do this", so as long as you're
not evil and frob the drm device nodes of ttm drivers directly the
deadlock will never happen. No idea how much contention actually
happens on e.g. shared buffer objects - in i915 we have just one lock
and so suffer quite a bit more from contention. So no idea how much
removing the yield would hurt.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [BUG] completely bonkers use of set_need_resched + VM_FAULT_NOPAGE

[Chris Wilson]

On Thu, Sep 12, 2013 at 06:22:10PM +0200, Peter Zijlstra wrote:
> On Thu, Sep 12, 2013 at 05:58:49PM +0200, Daniel Vetter wrote:
> > On Thu, Sep 12, 2013 at 5:43 PM, Peter Zijlstra  
> > wrote:
> > >> The one in ttm is just bonghits to shut up lockdep: ttm can recurse
> > >> into it's own pagefault handler and then deadlock, the trylock just
> > >> keeps lockdep quiet. We've had that bug arise in drm/i915 due to some
> > >> fun userspace did and now have testcases for them. The right solution
> > >> to fix this is to use copy_to|from_user_atomic in ttm everywhere it
> > >> holds locks and have slowpaths which drops locks, copies stuff into a
> > >> temp allocation and then continues. At least that's how we've fixed
> > >> all those inversions in i915-gem. I'm not volunteering to fix this ;-)
> > >
> > > Yikes.. so how common is it? If I simply rip the set_need_resched() out
> > > it will 'spin' on the fault a little longer until a 'natural' preemption
> > > point -- if such a thing is every going to happen.
> > 
> > It's a case of "our userspace doesn't do this", so as long as you're
> > not evil and frob the drm device nodes of ttm drivers directly the
> > deadlock will never happen. No idea how much contention actually
> > happens on e.g. shared buffer objects - in i915 we have just one lock
> > and so suffer quite a bit more from contention. So no idea how much
> > removing the yield would hurt.
> 
> If 'sane' userspace is never supposed to do this, then only insane
> userspace is going to hurt from this and that's a GOOD (tm) thing,
> right? ;-)

Not quite, as it would be possible for the evil userspace to trigger a
GPU hang that would cause the sane userspace to spin indefinitely 
waiting for the error recovery to kick in.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 1/2] drm/i915: kill set_need_resched

[Daniel Vetter]

This is just a remnant from the old days when our reset handling was
horribly racy, suffered from terribly locking issues and often happily
live-locked. Those days are now gone so we can drop the hacks and just
rip the reschedule-point out.

Reported-by: Peter Zijlstra 
Cc: Peter Zijlstra 
Cc: Linux Kernel Mailing List 
Signed-off-by: Daniel Vetter 
---
 drivers/gpu/drm/i915/i915_gem.c | 11 ---
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index d80f33d..3871060 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1389,14 +1389,11 @@ out:
if (i915_terminally_wedged(&dev_priv->gpu_error))
return VM_FAULT_SIGBUS;
case -EAGAIN:
-   /* Give the error handler a chance to run and move the
-* objects off the GPU active list. Next time we service the
-* fault, we should be able to transition the page into the
-* GTT without touching the GPU (and so avoid further
-* EIO/EGAIN). If the GPU is wedged, then there is no issue
-* with coherency, just lost writes.
+   /*
+* EAGAIN means the gpu is hung and we'll wait for the error
+* handler to reset everything when re-faulting in
+* i915_mutex_lock_interruptible.
 */
-   set_need_resched();
case 0:
case -ERESTARTSYS:
case -EINTR:
-- 
1.8.4.rc3

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

[Intel-gfx] [PATCH] drm/i915: move intel_init_pch_refclk into intel_modeset_init_hw

[Daniel Vetter]

Sprinkling random special functions all over the place always
has the risk that we miss it somewhere. Furthermore in this
case the ordering wrt gem_init_hw was actually different between
the driver load sequence and the resume sequence.

So just call this at the beginning of modeset_init_hw.

Cc: Paulo Zanoni 
Signed-off-by: Daniel Vetter 
---
 drivers/gpu/drm/i915/i915_drv.c  | 2 --
 drivers/gpu/drm/i915/i915_drv.h  | 1 -
 drivers/gpu/drm/i915/intel_display.c | 6 +++---
 3 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index ec690ca..4ed63cc 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -586,8 +586,6 @@ static int __i915_drm_thaw(struct drm_device *dev)
 
/* KMS EnterVT equivalent */
if (drm_core_check_feature(dev, DRIVER_MODESET)) {
-   intel_init_pch_refclk(dev);
-
mutex_lock(&dev->struct_mutex);
 
error = i915_gem_init_hw(dev);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7caf71d..5208419 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2247,7 +2247,6 @@ extern void i915_redisable_vga(struct drm_device *dev);
 extern bool intel_fbc_enabled(struct drm_device *dev);
 extern void intel_disable_fbc(struct drm_device *dev);
 extern bool ironlake_set_drps(struct drm_device *dev, u8 val);
-extern void intel_init_pch_refclk(struct drm_device *dev);
 extern void gen6_set_rps(struct drm_device *dev, u8 val);
 extern void valleyview_set_rps(struct drm_device *dev, u8 val);
 extern int valleyview_rps_max_freq(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 378174f..7959640 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5376,7 +5376,7 @@ static void lpt_init_pch_refclk(struct drm_device *dev)
 /*
  * Initialize reference clocks when the driver loads
  */
-void intel_init_pch_refclk(struct drm_device *dev)
+static void intel_init_pch_refclk(struct drm_device *dev)
 {
if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev))
ironlake_init_pch_refclk(dev);
@@ -9614,8 +9614,6 @@ static void intel_setup_outputs(struct drm_device *dev)
intel_encoder_clones(encoder);
}
 
-   intel_init_pch_refclk(dev);
-
drm_helper_move_panel_connectors_to_head(dev);
 }
 
@@ -10086,6 +10084,8 @@ static void i915_enable_vga(struct drm_device *dev)
 
 void intel_modeset_init_hw(struct drm_device *dev)
 {
+   intel_init_pch_refclk(dev);
+
intel_init_power_well(dev);
 
intel_prepare_ddi(dev);
-- 
1.8.4.rc3

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

[Intel-gfx] [PATCH] drm/i915: Enable VLV to work in BIOS-less system

[Chon Ming Lee]

In non PC system, such as IVI, may not use BIOS, instead it uses boot
loader with only minimal system initialization.  Most of the time, boot
loader doesn't come with VBIOS, and depends on device driver to fully
initialize the display controller and GPU.

For Valleyview, without VBIOS, i915 fails to work.  The patch add some
missing init code, such as doing a DPIO CMNRESET and program the GMBUS
frequency.

Signed-off-by: Chon Ming Lee 
---
 drivers/gpu/drm/i915/i915_reg.h  |8 +
 drivers/gpu/drm/i915/intel_display.c |   51 ++
 2 files changed, 59 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index bcee89b..8ddf58a 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -382,6 +382,8 @@
 #define   FB_FMAX_VMIN_FREQ_LO_MASK0xf800
 
 /* vlv2 north clock has */
+#define CCK_FUSE_REG   0x8
+#define  CCK_FUSE_HPLL_FREQ_MASK   0x3
 #define CCK_REG_DSI_PLL_FUSE   0x44
 #define CCK_REG_DSI_PLL_CONTROL0x48
 #define  DSI_PLL_VCO_EN(1 << 31)
@@ -1424,6 +1426,12 @@
 
 #define MI_ARB_VLV (VLV_DISPLAY_BASE + 0x6504)
 
+#define CZCLK_CDCLK_FREQ_RATIO (dev_priv->info->display_mmio_offset + 0x6508)
+#define   CDCLK_FREQ_SHIFT 4
+#define   CDCLK_FREQ_MASK  0x1f
+#define   CZCLK_FREQ_MASK  0xf
+#define GMBUS_FREQ (dev_priv->info->display_mmio_offset + 0x6510)
+
 /*
  * Palette regs
  */
diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 3c0e0cf..9ef1d28 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9497,6 +9497,31 @@ static bool has_edp_a(struct drm_device *dev)
return true;
 }
 
+static void gmbus_set_freq(struct drm_i915_private *dev_priv, u32 hpll_freq)
+{
+   int cdclk_ratio[] = { 10, 15, 20, 25, 30, 0, 40, 45, 50, 0,
+   60, 0, 0, 75, 80, 0, 90, 0, 100, 0,
+   0, 0, 120, 0, 0, 0, 0, 0, 150, 0, 160 };
+   int vco_freq[] = { 800, 1600, 2000, 2400 };
+   int gmbus_freq = 0, cdclk;
+   u32 reg_val;
+
+   BUG_ON(hpll_freq > ARRAY_SIZE(vco_freq));
+
+   reg_val = I915_READ(CZCLK_CDCLK_FREQ_RATIO);
+
+   cdclk = ((reg_val >> CDCLK_FREQ_SHIFT) & CDCLK_FREQ_MASK) - 1;
+
+   if (cdclk_ratio[cdclk])
+   gmbus_freq = vco_freq[hpll_freq] / cdclk_ratio[cdclk] * 10;
+
+   WARN_ON(gmbus_freq == 0);
+
+   if (gmbus_freq != 0)
+   I915_WRITE(GMBUS_FREQ, gmbus_freq);
+
+}
+
 static void intel_setup_outputs(struct drm_device *dev)
 {
struct drm_i915_private *dev_priv = dev->dev_private;
@@ -9555,6 +9580,32 @@ static void intel_setup_outputs(struct drm_device *dev)
if (I915_READ(PCH_DP_D) & DP_DETECTED)
intel_dp_init(dev, PCH_DP_D, PORT_D);
} else if (IS_VALLEYVIEW(dev)) {
+   u32 reg_val;
+
+   /* Trigger DPIO CMN RESET, require especially in BIOS less
+* system
+*/
+   reg_val = I915_READ(DPIO_CTL);
+   if (!(reg_val & 0x1)) {
+   I915_WRITE(DPIO_CTL, 0x0);
+   I915_WRITE(DPIO_CTL, 0x1);
+   POSTING_READ(DPIO_CTL);
+   }
+
+   /* In BIOS-less system, program the correct gmbus frequency
+* before reading edid.
+*/
+
+   /* Obtain SKU information to determine the correct CDCLK */
+   mutex_lock(&dev_priv->dpio_lock);
+   reg_val = vlv_cck_read(dev_priv, CCK_FUSE_REG);
+   mutex_unlock(&dev_priv->dpio_lock);
+
+   reg_val &= CCK_FUSE_HPLL_FREQ_MASK;
+
+   /* Write CDCLK to GMBUS freq for GMBUS clk generation. */
+   gmbus_set_freq(dev_priv, reg_val);
+
/* Check for built-in panel first. Shares lanes with HDMI on 
SDVOC */
if (I915_READ(VLV_DISPLAY_BASE + GEN4_HDMIC) & SDVO_DETECTED) {
intel_hdmi_init(dev, VLV_DISPLAY_BASE + GEN4_HDMIC,
-- 
1.7.7.6

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

Re: [Intel-gfx] [PATCH] drm/i915: Enable VLV to work in BIOS-less system

[Daniel Vetter]

On Fri, Sep 13, 2013 at 5:59 AM, Chon Ming Lee  wrote:
> In non PC system, such as IVI, may not use BIOS, instead it uses boot
> loader with only minimal system initialization.  Most of the time, boot
> loader doesn't come with VBIOS, and depends on device driver to fully
> initialize the display controller and GPU.
>
> For Valleyview, without VBIOS, i915 fails to work.  The patch add some
> missing init code, such as doing a DPIO CMNRESET and program the GMBUS
> frequency.
>
> Signed-off-by: Chon Ming Lee 

Generally I prefer patches with tighter topics, grouped into a series.
For this one I'd split it up into the gmbus setup and the dpio reset,
so 2 patches.

> ---
>  drivers/gpu/drm/i915/i915_reg.h  |8 +
>  drivers/gpu/drm/i915/intel_display.c |   51 
> ++
>  2 files changed, 59 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index bcee89b..8ddf58a 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -382,6 +382,8 @@
>  #define   FB_FMAX_VMIN_FREQ_LO_MASK0xf800
>
>  /* vlv2 north clock has */
> +#define CCK_FUSE_REG   0x8
> +#define  CCK_FUSE_HPLL_FREQ_MASK   0x3
>  #define CCK_REG_DSI_PLL_FUSE   0x44
>  #define CCK_REG_DSI_PLL_CONTROL0x48
>  #define  DSI_PLL_VCO_EN(1 << 31)
> @@ -1424,6 +1426,12 @@
>
>  #define MI_ARB_VLV (VLV_DISPLAY_BASE + 0x6504)
>
> +#define CZCLK_CDCLK_FREQ_RATIO (dev_priv->info->display_mmio_offset + 0x6508)
> +#define   CDCLK_FREQ_SHIFT 4
> +#define   CDCLK_FREQ_MASK  0x1f
> +#define   CZCLK_FREQ_MASK  0xf
> +#define GMBUS_FREQ (dev_priv->info->display_mmio_offset + 0x6510)
> +
>  /*
>   * Palette regs
>   */
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index 3c0e0cf..9ef1d28 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9497,6 +9497,31 @@ static bool has_edp_a(struct drm_device *dev)
> return true;
>  }
>
> +static void gmbus_set_freq(struct drm_i915_private *dev_priv, u32 hpll_freq)
> +{
> +   int cdclk_ratio[] = { 10, 15, 20, 25, 30, 0, 40, 45, 50, 0,
> +   60, 0, 0, 75, 80, 0, 90, 0, 100, 0,
> +   0, 0, 120, 0, 0, 0, 0, 0, 150, 0, 160 };
> +   int vco_freq[] = { 800, 1600, 2000, 2400 };
> +   int gmbus_freq = 0, cdclk;
> +   u32 reg_val;
> +
> +   BUG_ON(hpll_freq > ARRAY_SIZE(vco_freq));
> +
> +   reg_val = I915_READ(CZCLK_CDCLK_FREQ_RATIO);
> +
> +   cdclk = ((reg_val >> CDCLK_FREQ_SHIFT) & CDCLK_FREQ_MASK) - 1;
> +
> +   if (cdclk_ratio[cdclk])
> +   gmbus_freq = vco_freq[hpll_freq] / cdclk_ratio[cdclk] * 10;
> +
> +   WARN_ON(gmbus_freq == 0);
> +
> +   if (gmbus_freq != 0)
> +   I915_WRITE(GMBUS_FREQ, gmbus_freq);
> +
> +}
> +
>  static void intel_setup_outputs(struct drm_device *dev)
>  {
> struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -9555,6 +9580,32 @@ static void intel_setup_outputs(struct drm_device *dev)

Doing this in setup_outputs feels like hiding stuff ;-) I know that
there's already the pch refclock setup here, but I've just submitted a
patch to get it out of there.

The problem is that setup_outputs isn't called on resume, and I
suspect we actually need this.

The dpio reset is probably best stuffed into intel_uncore_sanitize
> if (I915_READ(PCH_DP_D) & DP_DETECTED)
> intel_dp_init(dev, PCH_DP_D, PORT_D);
> } else if (IS_VALLEYVIEW(dev)) {
> +   u32 reg_val;
> +
> +   /* Trigger DPIO CMN RESET, require especially in BIOS less
> +* system
> +*/
> +   reg_val = I915_READ(DPIO_CTL);
> +   if (!(reg_val & 0x1)) {
> +   I915_WRITE(DPIO_CTL, 0x0);
> +   I915_WRITE(DPIO_CTL, 0x1);
> +   POSTING_READ(DPIO_CTL);
> +   }

Atm we don't have a suitable "early platform setup" function which is
called from the right places. I'd shove this into intel_uncore.c:
intel_uncore_sanitize for now, that should work.

Maybe once your patches are in I'll throw a intel_ealy_init_hw
refactoring on top to avoid sprinkling things all over the place
between the driver load and the resume codepaths.

> +
> +   /* In BIOS-less system, program the correct gmbus frequency
> +* before reading edid.
> +*/
> +
> +   /* Obtain SKU information to determine the correct CDCLK */
> +   mutex_lock(&dev_priv->dpio_lock);
> +   reg_val = vlv_cck_read(dev_priv, CCK_FUSE_REG);
> +   mutex_unlock(&dev_priv->dpio_lock);

I'd move this into the gmbus_freq setup function.

> +
> +

[Intel-gfx] [PATCH 2/2] drm/udl: rip out set_need_resched

[Daniel Vetter]

This very much looks like copypasta from drm/i915's fault handler.
It was used there to duct-tape over issues around gpu reset handling.

Since that can't ever happen for udl and there's seemingly no other
reason for this just drop it.

Reported-by: Peter Zijlstra 
Cc: Peter Zijlstra 
Cc: Linux Kernel Mailing List 
Cc: Dave Airlie 
Signed-off-by: Daniel Vetter 
---
 drivers/gpu/drm/udl/udl_gem.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c
index 8dbe9d0..8bf6461 100644
--- a/drivers/gpu/drm/udl/udl_gem.c
+++ b/drivers/gpu/drm/udl/udl_gem.c
@@ -97,7 +97,6 @@ int udl_gem_fault(struct vm_area_struct *vma, struct vm_fault 
*vmf)
ret = vm_insert_page(vma, (unsigned long)vmf->virtual_address, page);
switch (ret) {
case -EAGAIN:
-   set_need_resched();
case 0:
case -ERESTARTSYS:
return VM_FAULT_NOPAGE;
-- 
1.8.4.rc3

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

Re: [Intel-gfx] [PATCH 2/5] drm/i915: use the HDMI DDI buffer translations from VBT

[Paulo Zanoni]

2013/9/11 Chris Wilson :
> On Wed, Sep 11, 2013 at 06:02:48PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni 
>>
>> We currently use the recommended values from BSpec, but the VBT
>> specifies the correct value to use for the hardware we have, so use
>> it. We also fall back to the recommended value in case we can't find
>> the VBT.
>>
>> In addition, this code also provides some infrastructure to parse more
>> information about the DDI ports. There's a lot more information we
>> could extract and use in the future.
>>
>> v2: - Move some code to init_vbt_defaults.
>> v3: - Rebase
>> - Clarify the "DVO Port" matching code
>>
>> Signed-off-by: Paulo Zanoni 
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h   |  6 +++
>>  drivers/gpu/drm/i915/intel_bios.c | 78 
>> +++
>>  drivers/gpu/drm/i915/intel_bios.h | 13 +++
>>  drivers/gpu/drm/i915/intel_ddi.c  | 24 +++-
>>  4 files changed, 119 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h 
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index ca8856a..298c671 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1052,6 +1052,10 @@ enum modeset_restore {
>>   MODESET_SUSPENDED,
>>  };
>>
>> +struct ddi_vbt_port_info {
>> + uint8_t hdmi_level_shift;
>> +};
>> +
>>  struct intel_vbt_data {
>>   struct drm_display_mode *lfp_lvds_vbt_mode; /* if any */
>>   struct drm_display_mode *sdvo_lvds_vbt_mode; /* if any */
>> @@ -1086,6 +1090,8 @@ struct intel_vbt_data {
>>
>>   int child_dev_num;
>>   union child_device_config *child_dev;
>> +
>> + struct ddi_vbt_port_info ddi_port_info[5];
>
> s/5/PORT_E+1/ or better #define NUM_DDI_PORTS PORT_E+1.

Looks like I can just use the already-defined I915_MAX_PORTS. It also
matches the I915_MAX_PIPES usage.


>
>>  };
>>
>>  enum intel_ddb_partitioning {
>> diff --git a/drivers/gpu/drm/i915/intel_bios.c 
>> b/drivers/gpu/drm/i915/intel_bios.c
>> index 33003b9..d7ff8fc 100644
>> --- a/drivers/gpu/drm/i915/intel_bios.c
>> +++ b/drivers/gpu/drm/i915/intel_bios.c
>> @@ -583,6 +583,72 @@ parse_mipi(struct drm_i915_private *dev_priv, struct 
>> bdb_header *bdb)
>>   dev_priv->vbt.dsi.panel_id = mipi->panel_id;
>>  }
>>
>> +static void parse_ddi_port(struct drm_i915_private *dev_priv, enum port 
>> port,
>> +struct bdb_header *bdb)
>> +{
>> + union child_device_config *it, *child = NULL;
>> + struct ddi_vbt_port_info *info = &dev_priv->vbt.ddi_port_info[port];
>> + uint8_t hdmi_level_shift;
>> + int i, j;
>> + /* Each DDI port can have more than one value on the "DVO Port" field,
>> +  * so look for all the possible values for each port and abort if more
>> +  * than one is found. */
>> + int dvo_ports[][2] = {
>> + {DVO_PORT_HDMIA, DVO_PORT_DPA},
>> + {DVO_PORT_HDMIB, DVO_PORT_DPB},
>> + {DVO_PORT_HDMIC, DVO_PORT_DPC},
>> + {DVO_PORT_HDMID, DVO_PORT_DPD},
>> + {DVO_PORT_CRT, -1 /* Port E can only be DVO_PORT_CRT */ },
>> + };
>> +
>
> I don't see the point of calling this function if bdb->version < 158 ?

I built the whole function as a single patch, then I split it in 5
later, and chose this chunk to be the first since it was more
important than the others. So this patch looks weird, but things make
more sense if you see the next patches.


>
>> + /* Find the child device to use, abort if more than one found. */
>> + for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
>> + it = dev_priv->vbt.child_dev + i;
>> +
>> + for (j = 0; j < 2; j++) {
>> + if (dvo_ports[port][j] == -1)
>> + break;
>> +
>> + if (it->common.dvo_port == dvo_ports[port][j]) {
>> + if (child) {
>> + DRM_ERROR("More than one child device 
>> for port %c in VBT.\n",
>> +port_name(port));
>> + return;
>> + }
>> + child = it;
>> + }
>> + }
>> + }
>> + if (!child)
>> + return;
>> +
>> + if (bdb->version >= 158) {
>> + /* The VBT HDMI level shift values match the table we have. */
>> + hdmi_level_shift = child->raw[7] & 0xF;
>> + if (hdmi_level_shift < 0xC) {
>> + DRM_DEBUG_KMS("VBT HDMI level shift for port %c: %d\n",
>> +   port_name(port),
>> +   hdmi_level_shift);
>> + info->hdmi_level_shift = hdmi_level_shift;
>> + }
>> + }
>> +}
>> +
>> +static void parse_ddi_ports(struct drm_i915_private *dev_priv,
>> + struct bdb_header *bdb)
>> +{
>> + enum port port;
>> +
>> + if (!d

Re: [Intel-gfx] [PATCH 2/5] drm/i915: WARN if the DP aux read is too big

[Chris Wilson]

On Thu, Sep 12, 2013 at 01:58:18PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni 
> 
> So far we control all the reads an none of them exceeds the current
> limit of 20 bytes, but we never think about this when reviewing
> patches, so add a big WARN. In case we ever hit that WARN, we whould
> change the size of the reply array.
> 
> Signed-off-by: Paulo Zanoni 
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 20e468c..f30b691 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -577,6 +577,8 @@ intel_dp_aux_native_read(struct intel_dp *intel_dp,
>  
>   msg_bytes = 4;
>   reply_bytes = recv_bytes + 1;
> + if (WARN_ON(reply_bytes > sizeof(reply)))
> + return -E2BIG;

Meh, this would be a programming mistake, and coule be eliminated
entirely with:

uint8_t reply[recv_bytes+1];
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 5/5] drm/i915: check for errors on i915_drm_thaw

[Chris Wilson]

On Thu, Sep 12, 2013 at 01:58:21PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni 
> 
> Before this patch, i915_drm_thaw would always return 0. After this
> patch, it returns the error code from __i915_drm_thaw(), since it's
> the only sub-function that can return an error code.

You could go a step further here and pass a parameter to
__i915_drm_thaw() to unify the two thaw paths.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/5] drm/i915: don't save/restore LBB on Gen5+

[Ville Syrjälä]

On Thu, Sep 12, 2013 at 01:58:17PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni 
> 
> Because this PCI config register doesn't exist on Gen5+.
> 
> Signed-off-by: Paulo Zanoni 

Yep. Can't see it in configdb. Actually it's only listed there for CTG
and CLN, which I guess makes sense as non-mobile platforms probably
don't have backlights. For BLB and ELK it's listed as reserved and RO,
so I guess it doesn't hurt to poke it on them. If I'm wrong we could
probably slap on an IS_MOBILE() check there too. Older stuff doesn't
exist in configdb, so that's as far back as I can go.

Reviewed-by: Ville Syrjälä 

> ---
>  drivers/gpu/drm/i915/i915_suspend.c | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_suspend.c 
> b/drivers/gpu/drm/i915/i915_suspend.c
> index 70db618..3538370 100644
> --- a/drivers/gpu/drm/i915/i915_suspend.c
> +++ b/drivers/gpu/drm/i915/i915_suspend.c
> @@ -340,7 +340,9 @@ int i915_save_state(struct drm_device *dev)
>   struct drm_i915_private *dev_priv = dev->dev_private;
>   int i;
>  
> - pci_read_config_byte(dev->pdev, LBB, &dev_priv->regfile.saveLBB);
> + if (INTEL_INFO(dev)->gen <= 4)
> + pci_read_config_byte(dev->pdev, LBB,
> +  &dev_priv->regfile.saveLBB);
>  
>   mutex_lock(&dev->struct_mutex);
>  
> @@ -390,7 +392,9 @@ int i915_restore_state(struct drm_device *dev)
>   struct drm_i915_private *dev_priv = dev->dev_private;
>   int i;
>  
> - pci_write_config_byte(dev->pdev, LBB, dev_priv->regfile.saveLBB);
> + if (INTEL_INFO(dev)->gen <= 4)
> + pci_write_config_byte(dev->pdev, LBB,
> +   dev_priv->regfile.saveLBB);
>  
>   mutex_lock(&dev->struct_mutex);
>  
> -- 
> 1.8.3.1
> 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 1/5] drm/i915: don't save/restore LBB on Gen5+

[Paulo Zanoni]

From: Paulo Zanoni 

Because this PCI config register doesn't exist on Gen5+.

Signed-off-by: Paulo Zanoni 
---
 drivers/gpu/drm/i915/i915_suspend.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_suspend.c 
b/drivers/gpu/drm/i915/i915_suspend.c
index 70db618..3538370 100644
--- a/drivers/gpu/drm/i915/i915_suspend.c
+++ b/drivers/gpu/drm/i915/i915_suspend.c
@@ -340,7 +340,9 @@ int i915_save_state(struct drm_device *dev)
struct drm_i915_private *dev_priv = dev->dev_private;
int i;
 
-   pci_read_config_byte(dev->pdev, LBB, &dev_priv->regfile.saveLBB);
+   if (INTEL_INFO(dev)->gen <= 4)
+   pci_read_config_byte(dev->pdev, LBB,
+&dev_priv->regfile.saveLBB);
 
mutex_lock(&dev->struct_mutex);
 
@@ -390,7 +392,9 @@ int i915_restore_state(struct drm_device *dev)
struct drm_i915_private *dev_priv = dev->dev_private;
int i;
 
-   pci_write_config_byte(dev->pdev, LBB, dev_priv->regfile.saveLBB);
+   if (INTEL_INFO(dev)->gen <= 4)
+   pci_write_config_byte(dev->pdev, LBB,
+ dev_priv->regfile.saveLBB);
 
mutex_lock(&dev->struct_mutex);
 
-- 
1.8.3.1

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

[Intel-gfx] [PATCH 4/5] drm/i915: clear opregon->lid_state after we unmap it

[Paulo Zanoni]

From: Paulo Zanoni 

We don't seem to be using the pointer after it's unmapped, so this
patch doesn't fix any bug I can reproduce.

Signed-off-by: Paulo Zanoni 
---
 drivers/gpu/drm/i915/intel_opregion.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/intel_opregion.c 
b/drivers/gpu/drm/i915/intel_opregion.c
index a01e0f8..dd88c08 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -668,6 +668,7 @@ void intel_opregion_fini(struct drm_device *dev)
opregion->swsci = NULL;
opregion->asle = NULL;
opregion->vbt = NULL;
+   opregion->lid_state = NULL;
 }
 
 static void swsci_setup(struct drm_device *dev)
-- 
1.8.3.1

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

[Intel-gfx] [PATCH 5/5] drm/i915: check for errors on i915_drm_thaw

[Paulo Zanoni]

From: Paulo Zanoni 

Before this patch, i915_drm_thaw would always return 0. After this
patch, it returns the error code from __i915_drm_thaw(), since it's
the only sub-function that can return an error code.

Signed-off-by: Paulo Zanoni 
---
 drivers/gpu/drm/i915/i915_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index ec690ca..f6dc6b8 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -650,7 +650,7 @@ static int i915_drm_thaw(struct drm_device *dev)
mutex_unlock(&dev->struct_mutex);
}
 
-   __i915_drm_thaw(dev);
+   error = __i915_drm_thaw(dev);
 
return error;
 }
-- 
1.8.3.1

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

[Intel-gfx] [PATCH 2/5] drm/i915: WARN if the DP aux read is too big

[Paulo Zanoni]

From: Paulo Zanoni 

So far we control all the reads an none of them exceeds the current
limit of 20 bytes, but we never think about this when reviewing
patches, so add a big WARN. In case we ever hit that WARN, we whould
change the size of the reply array.

Signed-off-by: Paulo Zanoni 
---
 drivers/gpu/drm/i915/intel_dp.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 20e468c..f30b691 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -577,6 +577,8 @@ intel_dp_aux_native_read(struct intel_dp *intel_dp,
 
msg_bytes = 4;
reply_bytes = recv_bytes + 1;
+   if (WARN_ON(reply_bytes > sizeof(reply)))
+   return -E2BIG;
 
for (;;) {
ret = intel_dp_aux_ch(intel_dp, msg, msg_bytes,
-- 
1.8.3.1

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

[Intel-gfx] [PATCH 3/5] drm/i915: check for more ASLC interrupts

[Paulo Zanoni]

From: Paulo Zanoni 

Sometimes I see the "non asle set request??" message on my Haswell
machine, so I decided to get the spec and see if some bits are missing
from the mask. We do have some bits missing from the mask, so this
patch adds them. But I still see the "non asle set request??" message
on my machine :(

Also use the proper ASLC name to indicate the registers we're talking
about.

Signed-off-by: Paulo Zanoni 
---
 drivers/gpu/drm/i915/intel_opregion.c | 33 ++---
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_opregion.c 
b/drivers/gpu/drm/i915/intel_opregion.c
index c4fb2ae..a01e0f8 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -117,12 +117,17 @@ struct opregion_asle {
 #define ASLE_ARDY_READY(1 << 0)
 #define ASLE_ARDY_NOT_READY(0 << 0)
 
-/* ASLE irq request bits */
-#define ASLE_SET_ALS_ILLUM (1 << 0)
-#define ASLE_SET_BACKLIGHT (1 << 1)
-#define ASLE_SET_PFIT  (1 << 2)
-#define ASLE_SET_PWM_FREQ  (1 << 3)
-#define ASLE_REQ_MSK   0xf
+/* ASLE Interrupt Command (ASLC) bits */
+#define ASLC_SET_ALS_ILLUM (1 << 0)
+#define ASLC_SET_BACKLIGHT (1 << 1)
+#define ASLC_SET_PFIT  (1 << 2)
+#define ASLC_SET_PWM_FREQ  (1 << 3)
+#define ASLC_SUPPORTED_ROTATION_ANGLES (1 << 4)
+#define ASLC_BUTTON_ARRAY  (1 << 5)
+#define ASLC_CONVERTIBLE_INDICATOR (1 << 6)
+#define ASLC_DOCKING_INDICATOR (1 << 7)
+#define ASLC_ISCT_STATE_CHANGE (1 << 8)
+#define ASLC_REQ_MSK   0x1ff
 
 /* response bits of ASLE irq request */
 #define ASLE_ALS_ILLUM_FAILED  (1<<10)
@@ -421,25 +426,31 @@ void intel_opregion_asle_intr(struct drm_device *dev)
if (!asle)
return;
 
-   asle_req = ioread32(&asle->aslc) & ASLE_REQ_MSK;
+   asle_req = ioread32(&asle->aslc) & ASLC_REQ_MSK;
 
if (!asle_req) {
DRM_DEBUG_DRIVER("non asle set request??\n");
return;
}
 
-   if (asle_req & ASLE_SET_ALS_ILLUM)
+   if (asle_req & ASLC_SET_ALS_ILLUM)
asle_stat |= asle_set_als_illum(dev, ioread32(&asle->alsi));
 
-   if (asle_req & ASLE_SET_BACKLIGHT)
+   if (asle_req & ASLC_SET_BACKLIGHT)
asle_stat |= asle_set_backlight(dev, ioread32(&asle->bclp));
 
-   if (asle_req & ASLE_SET_PFIT)
+   if (asle_req & ASLC_SET_PFIT)
asle_stat |= asle_set_pfit(dev, ioread32(&asle->pfit));
 
-   if (asle_req & ASLE_SET_PWM_FREQ)
+   if (asle_req & ASLC_SET_PWM_FREQ)
asle_stat |= asle_set_pwm_freq(dev, ioread32(&asle->pfmb));
 
+   if (asle_req & (ASLC_SUPPORTED_ROTATION_ANGLES | ASLC_BUTTON_ARRAY |
+   ASLC_CONVERTIBLE_INDICATOR | ASLC_DOCKING_INDICATOR |
+   ASLC_ISCT_STATE_CHANGE)) {
+   DRM_DEBUG_DRIVER("ASLC interrupt not supported\n");
+   }
+
iowrite32(asle_stat, &asle->aslc);
 }
 
-- 
1.8.3.1

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

[Intel-gfx] [PATCH 1/3] drm/i915: do not update cursor in crtc mode set

[ville . syrjala]

From: Jani Nikula 

The cursor is disabled before crtc mode set in crtc disable (and we
assert this is the case), and enabled afterwards in crtc enable. Do not
update it in crtc mode set.

On HSW enabling a plane on a disabled pipe may hang the entire system.
And there's no good reason for doing it ever, so just don't.

v2: Add note about HSW hangs - vsyrjala

Cc: sta...@vger.kernel.org
Suggested-by: Ville Syrjälä 
Reviewed-by: Ville Syrjälä 
Signed-off-by: Jani Nikula 
---
 drivers/gpu/drm/i915/intel_display.c | 9 -
 1 file changed, 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 3c0e0cf..18043a2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4888,9 +4888,6 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
}
}
 
-   /* Ensure that the cursor is valid for the new mode before changing... 
*/
-   intel_crtc_update_cursor(crtc, true);
-
if (!is_dsi && is_lvds && dev_priv->lvds_downclock_avail) {
/*
 * Ensure we match the reduced clock's P to the target clock.
@@ -5780,9 +5777,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
intel_crtc->config.dpll.p2 = clock.p2;
}
 
-   /* Ensure that the cursor is valid for the new mode before changing... 
*/
-   intel_crtc_update_cursor(crtc, true);
-
/* CPU eDP is the only output that doesn't need a PCH PLL of its own. */
if (intel_crtc->config.has_pch_encoder) {
fp = i9xx_dpll_compute_fp(&intel_crtc->config.dpll);
@@ -6270,9 +6264,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
if (!intel_ddi_pll_mode_set(crtc))
return -EINVAL;
 
-   /* Ensure that the cursor is valid for the new mode before changing... 
*/
-   intel_crtc_update_cursor(crtc, true);
-
if (intel_crtc->config.has_dp_encoder)
intel_dp_set_m_n(intel_crtc);
 
-- 
1.8.1.5

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

[Intel-gfx] [PATCH 2/3] drm/i915: Don't enable the cursor on a disable pipe

[ville . syrjala]

From: Ville Syrjälä 

On HSW enabling a plane on a disabled pipe may hang the entire system.
And there's no good reason for doing it ever, so just don't.

Cc: sta...@vger.kernel.org
Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/i915/intel_display.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 18043a2..d0137b6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6793,6 +6793,9 @@ static void intel_crtc_update_cursor(struct drm_crtc 
*crtc,
u32 base, pos;
bool visible;
 
+   if (!intel_crtc->active)
+   return;
+
pos = 0;
 
if (on && crtc->enabled && crtc->fb) {
-- 
1.8.1.5

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

Re: [Intel-gfx] [BUG] completely bonkers use of set_need_resched + VM_FAULT_NOPAGE

[Peter Zijlstra]

On Thu, Sep 12, 2013 at 05:36:43PM +0200, Daniel Vetter wrote:
> The set_need_resched in i915_gem.c:i915_gem_fault can actually be
> removed. It was there to give the error handler a chance to sneak in
> and reset the hw/sw tracking when the gpu is dead. That hack goes back
> to the days when the locking around our error handler was somewhere
> between nonexistent and totally broken, nowadays we keep things from
> live-locking by a bit of magic in i915_mutex_lock_interruptible. I'll
> whip up a patch to rip this out. I'll also check that our testsuite
> properly exercises this path (needs a bit of work on a quick look for
> better coverage).
> 
> The one in udl just looks like copypasta from i915, without any
> justification (at least I don't see any) for why it's there. Probably
> can die, too, since there isn't any gpu to reset on usb display-link
> devices ...

OK, awesome that. 2 down.

> The one in ttm is just bonghits to shut up lockdep: ttm can recurse
> into it's own pagefault handler and then deadlock, the trylock just
> keeps lockdep quiet. We've had that bug arise in drm/i915 due to some
> fun userspace did and now have testcases for them. The right solution
> to fix this is to use copy_to|from_user_atomic in ttm everywhere it
> holds locks and have slowpaths which drops locks, copies stuff into a
> temp allocation and then continues. At least that's how we've fixed
> all those inversions in i915-gem. I'm not volunteering to fix this ;-)

Yikes.. so how common is it? If I simply rip the set_need_resched() out
it will 'spin' on the fault a little longer until a 'natural' preemption
point -- if such a thing is every going to happen.

It would make that path take longer, but not be more or less broken.

So if its a rare path, I'll just rip the set_need_resched() out and you
DRM people can then fix up at your own pace.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [BUG] completely bonkers use of set_need_resched + VM_FAULT_NOPAGE

[Daniel Vetter]

On Thu, Sep 12, 2013 at 6:44 PM, Thomas Hellstrom  wrote:
>
> I think a possible fix would be if fault() were allowed to return an error
> and drop the mmap_sem() before returning.
>
> Otherwise we need to track down all copy_to_user / copy_from_user which
> happen with bo::reserve held.

For maximal evilness submit the relocation list (or whatever data
execbuf slurps in with copy_from_user while holding bo::reserve) of a
bo in the execbuf list. At least that's the testcase we have for
drm/i915. Then make sure that the execbuf wants the bo somewhere it
can't be mmaped from userspace, so needs to be moved both in the fault
handler and then back for the execbuf to continue ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 3/5] drm/i915: check the DDC and AUX bits of the VBT on DDI machines

[Paulo Zanoni]

From: Paulo Zanoni 

Our code currently assumes that port X will use the DP AUX channel X
and the DDC pin X. The VBT should tell us how things are mapped, so
add some WARNs in case we discover our assumptions are wrong (or in
case the VBT is just wrong, which is also perfectly possible).

Why would someone wire port B to AUX C and DDC D?

v2: Rebase
v3: Convert WARNs to DRM_DEBUG_KMS

Signed-off-by: Paulo Zanoni 
Reviewed-by: Rodrigo Vivi  (v1)
---
 drivers/gpu/drm/i915/intel_bios.c | 27 +++
 1 file changed, 27 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_bios.c 
b/drivers/gpu/drm/i915/intel_bios.c
index 2f43429..12e4fd1 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -590,6 +590,8 @@ static void parse_ddi_port(struct drm_i915_private 
*dev_priv, enum port port,
struct ddi_vbt_port_info *info = &dev_priv->vbt.ddi_port_info[port];
uint8_t hdmi_level_shift;
int i, j;
+   bool is_dvi, is_dp;
+   uint8_t aux_channel;
/* Each DDI port can have more than one value on the "DVO Port" field,
 * so look for all the possible values for each port and abort if more
 * than one is found. */
@@ -622,6 +624,31 @@ static void parse_ddi_port(struct drm_i915_private 
*dev_priv, enum port port,
if (!child)
return;
 
+   aux_channel = child->raw[25];
+
+   is_dvi = child->common.device_type & (1 << 4);
+   is_dp = child->common.device_type & (1 << 2);
+
+   if (is_dvi) {
+   if (child->common.ddc_pin == 0x05 && port != PORT_B)
+   DRM_DEBUG_KMS("Unexpected DDC pin for port B\n");
+   if (child->common.ddc_pin == 0x04 && port != PORT_C)
+   DRM_DEBUG_KMS("Unexpected DDC pin for port C\n");
+   if (child->common.ddc_pin == 0x06 && port != PORT_D)
+   DRM_DEBUG_KMS("Unexpected DDC pin for port D\n");
+   }
+
+   if (is_dp) {
+   if (aux_channel == 0x40 && port != PORT_A)
+   DRM_DEBUG_KMS("Unexpected AUX channel for port A\n");
+   if (aux_channel == 0x10 && port != PORT_B)
+   DRM_DEBUG_KMS("Unexpected AUX channel for port B\n");
+   if (aux_channel == 0x20 && port != PORT_C)
+   DRM_DEBUG_KMS("Unexpected AUX channel for port C\n");
+   if (aux_channel == 0x30 && port != PORT_D)
+   DRM_DEBUG_KMS("Unexpected AUX channel for port D\n");
+   }
+
if (bdb->version >= 158) {
/* The VBT HDMI level shift values match the table we have. */
hdmi_level_shift = child->raw[7] & 0xF;
-- 
1.8.3.1

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

Re: [Intel-gfx] [PATCH 1/3] drm/i915: do not update cursor in crtc mode set

[Chris Wilson]

On Thu, Sep 12, 2013 at 10:45:41PM +0300, ville.syrj...@linux.intel.com wrote:
> From: Jani Nikula 
> 
> The cursor is disabled before crtc mode set in crtc disable (and we
> assert this is the case), and enabled afterwards in crtc enable. Do not
> update it in crtc mode set.
> 
> On HSW enabling a plane on a disabled pipe may hang the entire system.
> And there's no good reason for doing it ever, so just don't.
> 
> v2: Add note about HSW hangs - vsyrjala
> 
> Cc: sta...@vger.kernel.org
> Suggested-by: Ville Syrjälä 
> Reviewed-by: Ville Syrjälä 
> Signed-off-by: Jani Nikula 

Where did the asserts go? :)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [BUG] completely bonkers use of set_need_resched + VM_FAULT_NOPAGE

[Peter Zijlstra]

On Thu, Sep 12, 2013 at 05:58:49PM +0200, Daniel Vetter wrote:
> On Thu, Sep 12, 2013 at 5:43 PM, Peter Zijlstra  wrote:
> >> The one in ttm is just bonghits to shut up lockdep: ttm can recurse
> >> into it's own pagefault handler and then deadlock, the trylock just
> >> keeps lockdep quiet. We've had that bug arise in drm/i915 due to some
> >> fun userspace did and now have testcases for them. The right solution
> >> to fix this is to use copy_to|from_user_atomic in ttm everywhere it
> >> holds locks and have slowpaths which drops locks, copies stuff into a
> >> temp allocation and then continues. At least that's how we've fixed
> >> all those inversions in i915-gem. I'm not volunteering to fix this ;-)
> >
> > Yikes.. so how common is it? If I simply rip the set_need_resched() out
> > it will 'spin' on the fault a little longer until a 'natural' preemption
> > point -- if such a thing is every going to happen.
> 
> It's a case of "our userspace doesn't do this", so as long as you're
> not evil and frob the drm device nodes of ttm drivers directly the
> deadlock will never happen. No idea how much contention actually
> happens on e.g. shared buffer objects - in i915 we have just one lock
> and so suffer quite a bit more from contention. So no idea how much
> removing the yield would hurt.

If 'sane' userspace is never supposed to do this, then only insane
userspace is going to hurt from this and that's a GOOD (tm) thing,
right? ;-)

And it won't actually deadlock if you don't use FIFO, for the regular
scheduler class it'll just spin a little longer before getting preempted
so no real worries there.


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

[Intel-gfx] [BUG] completely bonkers use of set_need_resched + VM_FAULT_NOPAGE

[Peter Zijlstra]

Hi Dave,

So I'm poking around the preemption code and stumbled upon:

drivers/gpu/drm/i915/i915_gem.c:set_need_resched();
drivers/gpu/drm/ttm/ttm_bo_vm.c:set_need_resched();
drivers/gpu/drm/ttm/ttm_bo_vm.c:set_need_resched();
drivers/gpu/drm/udl/udl_gem.c:  set_need_resched();

All these sites basically do:

  while (!trylock())
yield();

which is a horrible and broken locking pattern. 

Firstly its deadlock prone, suppose the faulting process is a FIFOn+1
task that preempted the lock holder at FIFOn.

Secondly the implementation is worse than usual by abusing
VM_FAULT_NOPAGE, which is supposed to install a PTE so that the fault
doesn't retry, but you're using it as a get out of fault path. And
you're using set_need_resched() which is not something a driver should
_ever_ touch.

Now I'm going to take away set_need_resched() -- and while you can
'reimplement' it using set_thread_flag() you're not going to do that
because it will be broken due to changes to the preempt code.

So please as to fix ASAP and don't allow anybody to trick you into
merging silly things like that again ;-)
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/2] drm/udl: rip out set_need_resched

[Peter Zijlstra]

On Thu, Sep 12, 2013 at 05:57:29PM +0200, Daniel Vetter wrote:
> This very much looks like copypasta from drm/i915's fault handler.
> It was used there to duct-tape over issues around gpu reset handling.
> 
> Since that can't ever happen for udl and there's seemingly no other
> reason for this just drop it.
> 
> Reported-by: Peter Zijlstra 

Thanks!

Acked-by: Peter Zijlstra 
___
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: kill set_need_resched

[Peter Zijlstra]

On Thu, Sep 12, 2013 at 05:57:28PM +0200, Daniel Vetter wrote:
> This is just a remnant from the old days when our reset handling was
> horribly racy, suffered from terribly locking issues and often happily
> live-locked. Those days are now gone so we can drop the hacks and just
> rip the reschedule-point out.
> 
> Reported-by: Peter Zijlstra 

Thanks!

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

Re: [Intel-gfx] [PATCH 2/3] drm/i915: Don't enable the cursor on a disable pipe

[Chris Wilson]

On Thu, Sep 12, 2013 at 10:45:42PM +0300, ville.syrj...@linux.intel.com wrote:
> From: Ville Syrjälä 
> 
> On HSW enabling a plane on a disabled pipe may hang the entire system.
> And there's no good reason for doing it ever, so just don't.
> 
> Cc: sta...@vger.kernel.org
> Signed-off-by: Ville Syrjälä 
> ---
>  drivers/gpu/drm/i915/intel_display.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index 18043a2..d0137b6 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6793,6 +6793,9 @@ static void intel_crtc_update_cursor(struct drm_crtc 
> *crtc,
>   u32 base, pos;
>   bool visible;
>  
> + if (!intel_crtc->active)
> + return;

This is misleading since we do expect to call this function whilst
turning off the crtc. This check makes it appear that such calls might
be wrong. Also the !crtc->enabled following intel_crtc->active makes
ones question their sanity.

So I feel this check detracts from readability of the function.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [BUG] completely bonkers use of set_need_resched + VM_FAULT_NOPAGE

[Peter Zijlstra]

On Thu, Sep 12, 2013 at 05:11:25PM +0200, Maarten Lankhorst wrote:
> Op 12-09-13 17:06, Peter Zijlstra schreef:
> > Hi Dave,
> >
> > So I'm poking around the preemption code and stumbled upon:
> >
> > drivers/gpu/drm/i915/i915_gem.c:set_need_resched();
> > drivers/gpu/drm/ttm/ttm_bo_vm.c:set_need_resched();
> > drivers/gpu/drm/ttm/ttm_bo_vm.c:set_need_resched();
> > drivers/gpu/drm/udl/udl_gem.c:  set_need_resched();
> >
> > All these sites basically do:
> >
> >   while (!trylock())
> > yield();
> >
> > which is a horrible and broken locking pattern. 
> >
> > Firstly its deadlock prone, suppose the faulting process is a FIFOn+1
> > task that preempted the lock holder at FIFOn.
> >
> > Secondly the implementation is worse than usual by abusing
> > VM_FAULT_NOPAGE, which is supposed to install a PTE so that the fault
> > doesn't retry, but you're using it as a get out of fault path. And
> > you're using set_need_resched() which is not something a driver should
> > _ever_ touch.
> >
> > Now I'm going to take away set_need_resched() -- and while you can
> > 'reimplement' it using set_thread_flag() you're not going to do that
> > because it will be broken due to changes to the preempt code.
> >
> > So please as to fix ASAP and don't allow anybody to trick you into
> > merging silly things like that again ;-)
> >
> Agreed, but this is a case of locking inversion. How do you propose to get 
> around that?

me? No idea, I've never looked at the actual locking in DRM. Someone
who's familiar with that code would have to tackle that.

I just spotted the fail because I was going to remove set_need_resched()
and had a WTF moment when I tripped over this stuff.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 2/5] drm/i915: use the HDMI DDI buffer translations from VBT

[Paulo Zanoni]

From: Paulo Zanoni 

We currently use the recommended values from BSpec, but the VBT
specifies the correct value to use for the hardware we have, so use
it. We also fall back to the recommended value in case we can't find
the VBT.

In addition, this code also provides some infrastructure to parse more
information about the DDI ports. There's a lot more information we
could extract and use in the future.

v2: - Move some code to init_vbt_defaults.
v3: - Rebase
- Clarify the "DVO Port" matching code
v4: - Use I915_MAX_PORTS
- Change the HAS_DDI checks
- Replace DRM_ERROR with DRM_DEBUG_KMS

Signed-off-by: Paulo Zanoni 
---
 drivers/gpu/drm/i915/i915_drv.h   |  6 +++
 drivers/gpu/drm/i915/intel_bios.c | 77 +++
 drivers/gpu/drm/i915/intel_bios.h | 13 +++
 drivers/gpu/drm/i915/intel_ddi.c  | 24 +++-
 4 files changed, 118 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ca8856a..f159df0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1052,6 +1052,10 @@ enum modeset_restore {
MODESET_SUSPENDED,
 };
 
+struct ddi_vbt_port_info {
+   uint8_t hdmi_level_shift;
+};
+
 struct intel_vbt_data {
struct drm_display_mode *lfp_lvds_vbt_mode; /* if any */
struct drm_display_mode *sdvo_lvds_vbt_mode; /* if any */
@@ -1086,6 +1090,8 @@ struct intel_vbt_data {
 
int child_dev_num;
union child_device_config *child_dev;
+
+   struct ddi_vbt_port_info ddi_port_info[I915_MAX_PORTS];
 };
 
 enum intel_ddb_partitioning {
diff --git a/drivers/gpu/drm/i915/intel_bios.c 
b/drivers/gpu/drm/i915/intel_bios.c
index 33003b9..2f43429 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -583,6 +583,76 @@ parse_mipi(struct drm_i915_private *dev_priv, struct 
bdb_header *bdb)
dev_priv->vbt.dsi.panel_id = mipi->panel_id;
 }
 
+static void parse_ddi_port(struct drm_i915_private *dev_priv, enum port port,
+  struct bdb_header *bdb)
+{
+   union child_device_config *it, *child = NULL;
+   struct ddi_vbt_port_info *info = &dev_priv->vbt.ddi_port_info[port];
+   uint8_t hdmi_level_shift;
+   int i, j;
+   /* Each DDI port can have more than one value on the "DVO Port" field,
+* so look for all the possible values for each port and abort if more
+* than one is found. */
+   int dvo_ports[][2] = {
+   {DVO_PORT_HDMIA, DVO_PORT_DPA},
+   {DVO_PORT_HDMIB, DVO_PORT_DPB},
+   {DVO_PORT_HDMIC, DVO_PORT_DPC},
+   {DVO_PORT_HDMID, DVO_PORT_DPD},
+   {DVO_PORT_CRT, -1 /* Port E can only be DVO_PORT_CRT */ },
+   };
+
+   /* Find the child device to use, abort if more than one found. */
+   for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
+   it = dev_priv->vbt.child_dev + i;
+
+   for (j = 0; j < 2; j++) {
+   if (dvo_ports[port][j] == -1)
+   break;
+
+   if (it->common.dvo_port == dvo_ports[port][j]) {
+   if (child) {
+   DRM_DEBUG_KMS("More than one child 
device for port %c in VBT.\n",
+ port_name(port));
+   return;
+   }
+   child = it;
+   }
+   }
+   }
+   if (!child)
+   return;
+
+   if (bdb->version >= 158) {
+   /* The VBT HDMI level shift values match the table we have. */
+   hdmi_level_shift = child->raw[7] & 0xF;
+   if (hdmi_level_shift < 0xC) {
+   DRM_DEBUG_KMS("VBT HDMI level shift for port %c: %d\n",
+ port_name(port),
+ hdmi_level_shift);
+   info->hdmi_level_shift = hdmi_level_shift;
+   }
+   }
+}
+
+static void parse_ddi_ports(struct drm_i915_private *dev_priv,
+   struct bdb_header *bdb)
+{
+   struct drm_device *dev = dev_priv->dev;
+   enum port port;
+
+   if (!HAS_DDI(dev))
+   return;
+
+   if (!dev_priv->vbt.child_dev_num)
+   return;
+
+   if (bdb->version < 155)
+   return;
+
+   for (port = PORT_A; port < I915_MAX_PORTS; port++)
+   parse_ddi_port(dev_priv, port, bdb);
+}
+
 static void
 parse_device_mapping(struct drm_i915_private *dev_priv,
   struct bdb_header *bdb)
@@ -652,6 +722,7 @@ static void
 init_vbt_defaults(struct drm_i915_private *dev_priv)
 {
struct drm_device *dev = dev_priv->dev;
+   enum port port;
 
dev_priv->vbt.crt_ddc_pin = GMBUS_PORT_VGADDC;
 
@@ -670,6 +741,11 @@ init_

[Intel-gfx] [PATCH 3/3] drm/i915: Don't enable sprites on a disabled pipe

[ville . syrjala]

From: Ville Syrjälä 

On HSW enabling a plane on a disabled pipe may hang the entire system.
And there's no good reason for doing it ever, so just don't.

Cc: sta...@vger.kernel.org
Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/i915/intel_sprite.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c 
b/drivers/gpu/drm/i915/intel_sprite.c
index d9c7a66..4f11eb1 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -652,8 +652,8 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc 
*crtc,
.y2 = crtc_y + crtc_h,
};
const struct drm_rect clip = {
-   .x2 = crtc->mode.hdisplay,
-   .y2 = crtc->mode.vdisplay,
+   .x2 = intel_crtc->active ? crtc->mode.hdisplay : 0,
+   .y2 = intel_crtc->active ? crtc->mode.vdisplay : 0,
};
 
intel_fb = to_intel_framebuffer(fb);
-- 
1.8.1.5

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

Re: [Intel-gfx] [BUG] completely bonkers use of set_need_resched + VM_FAULT_NOPAGE

[Thomas Hellstrom]

On 09/12/2013 05:58 PM, Daniel Vetter wrote:

On Thu, Sep 12, 2013 at 5:43 PM, Peter Zijlstra  wrote:

The one in ttm is just bonghits to shut up lockdep: ttm can recurse
into it's own pagefault handler and then deadlock, the trylock just
keeps lockdep quiet.


Could you describe how it could recurse into it's own pagefault handler?
IIRC the VM flags of the TTM VMAs makes get_user_pages() refrain from 
touching these VMAs,
hence I don't think this code can deadlock, but admittedly it's far from 
the optimal solution.


Never mind, more on the set_need_resched() below.



  We've had that bug arise in drm/i915 due to some
fun userspace did and now have testcases for them. The right solution
to fix this is to use copy_to|from_user_atomic in ttm everywhere it
holds locks and have slowpaths which drops locks, copies stuff into a
temp allocation and then continues. At least that's how we've fixed
all those inversions in i915-gem. I'm not volunteering to fix this ;-)

Yikes.. so how common is it? If I simply rip the set_need_resched() out
it will 'spin' on the fault a little longer until a 'natural' preemption
point -- if such a thing is every going to happen.


A typical case is if a process is throwing out a buffer from the GPU or 
system memory while another
process pagefaults while writing to it. It's not a common situation, and 
it's by no means a fastpath situation.

For correctness purposes, I think set_need_resched() can be safely removed.


It's a case of "our userspace doesn't do this", so as long as you're
not evil and frob the drm device nodes of ttm drivers directly the
deadlock will never happen. No idea how much contention actually
happens on e.g. shared buffer objects - in i915 we have just one lock
and so suffer quite a bit more from contention. So no idea how much
removing the yield would hurt.
-Daniel


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

Re: [Intel-gfx] [PATCH 3/3] drm/i915: Don't enable sprites on a disabled pipe

[Chris Wilson]

On Thu, Sep 12, 2013 at 10:45:43PM +0300, ville.syrj...@linux.intel.com wrote:
> From: Ville Syrjälä 
> 
> On HSW enabling a plane on a disabled pipe may hang the entire system.
> And there's no good reason for doing it ever, so just don't.
> 
> Cc: sta...@vger.kernel.org
> Signed-off-by: Ville Syrjälä 
> ---
>  drivers/gpu/drm/i915/intel_sprite.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c 
> b/drivers/gpu/drm/i915/intel_sprite.c
> index d9c7a66..4f11eb1 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -652,8 +652,8 @@ intel_update_plane(struct drm_plane *plane, struct 
> drm_crtc *crtc,
>   .y2 = crtc_y + crtc_h,
>   };
>   const struct drm_rect clip = {
> - .x2 = crtc->mode.hdisplay,
> - .y2 = crtc->mode.vdisplay,
> + .x2 = intel_crtc->active ? crtc->mode.hdisplay : 0,
> + .y2 = intel_crtc->active ? crtc->mode.vdisplay : 0,
>   };

Too much magic that looks like it would have interesting effects later
in the function. This function should only be called on an active CRTC,
so declare it so:

  if (WARN_ON(!intel_crtc->active))
return -EMONKEY;

-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 5/5] drm/i915: don't init DP or HDMI when not supported by DDI port

[Paulo Zanoni]

From: Paulo Zanoni 

There's no reason to init a DP connector if the encoder just supports
HDMI: we'll just waste hundreds and hundreds of cycles trying to do DP
AUX transactions to detect if there's something there. Same goes for a
DP connector that doesn't support HDMI, but I'm not sure these
actually exist.

v2: - Use bit fields
- Remove useless identation level
- Replace DRM_ERROR with DRM_DEBUG_KMS

Signed-off-by: Paulo Zanoni 
Reviewed-by: Rodrigo Vivi  (v1)
---
 drivers/gpu/drm/i915/i915_drv.h   |  4 
 drivers/gpu/drm/i915/intel_bios.c | 13 -
 drivers/gpu/drm/i915/intel_ddi.c  | 20 
 3 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f159df0..dd94731 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1054,6 +1054,10 @@ enum modeset_restore {
 
 struct ddi_vbt_port_info {
uint8_t hdmi_level_shift;
+
+   uint8_t supports_dvi:1;
+   uint8_t supports_hdmi:1;
+   uint8_t supports_dp:1;
 };
 
 struct intel_vbt_data {
diff --git a/drivers/gpu/drm/i915/intel_bios.c 
b/drivers/gpu/drm/i915/intel_bios.c
index 7ce1c3c..0492b6f 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -632,6 +632,10 @@ static void parse_ddi_port(struct drm_i915_private 
*dev_priv, enum port port,
is_hdmi = is_dvi && (child->common.device_type & (1 << 11)) == 0;
is_edp = is_dp && (child->common.device_type & (1 << 12));
 
+   info->supports_dvi = is_dvi;
+   info->supports_hdmi = is_hdmi;
+   info->supports_dp = is_dp;
+
DRM_DEBUG_KMS("Port %c VBT info: DP:%d HDMI:%d DVI:%d EDP:%d CRT:%d\n",
  port_name(port), is_dp, is_hdmi, is_dvi, is_edp, is_crt);
 
@@ -792,8 +796,15 @@ init_vbt_defaults(struct drm_i915_private *dev_priv)
DRM_DEBUG_KMS("Set default to SSC at %dMHz\n", 
dev_priv->vbt.lvds_ssc_freq);
 
for (port = PORT_A; port < I915_MAX_PORTS; port++) {
+   struct ddi_vbt_port_info *info =
+   &dev_priv->vbt.ddi_port_info[port];
+
/* Recommended BSpec default: 800mV 0dB. */
-   dev_priv->vbt.ddi_port_info[port].hdmi_level_shift = 6;
+   info->hdmi_level_shift = 6;
+
+   info->supports_dvi = (port != PORT_A && port != PORT_E);
+   info->supports_hdmi = info->supports_dvi;
+   info->supports_dp = (port != PORT_E);
}
 }
 
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index a9887eb..488f4a4 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1325,6 +1325,17 @@ void intel_ddi_init(struct drm_device *dev, enum port 
port)
struct drm_encoder *encoder;
struct intel_connector *hdmi_connector = NULL;
struct intel_connector *dp_connector = NULL;
+   bool init_hdmi, init_dp;
+
+   init_hdmi = (dev_priv->vbt.ddi_port_info[port].supports_dvi ||
+dev_priv->vbt.ddi_port_info[port].supports_hdmi);
+   init_dp = dev_priv->vbt.ddi_port_info[port].supports_dp;
+   if (!init_dp && !init_hdmi) {
+   DRM_DEBUG_KMS("VBT says port %c is not DVI/HDMI/DP 
compatible\n",
+ port_name(port));
+   init_hdmi = true;
+   init_dp = true;
+   }
 
intel_dig_port = kzalloc(sizeof(struct intel_digital_port), GFP_KERNEL);
if (!intel_dig_port)
@@ -1362,19 +1373,20 @@ void intel_ddi_init(struct drm_device *dev, enum port 
port)
intel_encoder->cloneable = false;
intel_encoder->hot_plug = intel_ddi_hot_plug;
 
-   if (!intel_dp_init_connector(intel_dig_port, dp_connector)) {
+   if (init_dp && !intel_dp_init_connector(intel_dig_port, dp_connector)) {
drm_encoder_cleanup(encoder);
kfree(intel_dig_port);
kfree(dp_connector);
return;
}
 
-   if (intel_encoder->type != INTEL_OUTPUT_EDP) {
+   /* In theory we don't need the encoder->type check, but leave it just in
+* case we have some really bad VBTs... */
+   if (intel_encoder->type != INTEL_OUTPUT_EDP && init_hdmi) {
hdmi_connector = kzalloc(sizeof(struct intel_connector),
 GFP_KERNEL);
-   if (!hdmi_connector) {
+   if (!hdmi_connector)
return;
-   }
 
intel_dig_port->hdmi.hdmi_reg = DDI_BUF_CTL(port);
intel_hdmi_init_connector(intel_dig_port, hdmi_connector);
-- 
1.8.3.1

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

Re: [Intel-gfx] [BUG] completely bonkers use of set_need_resched + VM_FAULT_NOPAGE

[Maarten Lankhorst]

Op 12-09-13 17:36, Daniel Vetter schreef:
> On Thu, Sep 12, 2013 at 5:06 PM, Peter Zijlstra  wrote:
>> So I'm poking around the preemption code and stumbled upon:
>>
>> drivers/gpu/drm/i915/i915_gem.c:set_need_resched();
>> drivers/gpu/drm/ttm/ttm_bo_vm.c:set_need_resched();
>> drivers/gpu/drm/ttm/ttm_bo_vm.c:set_need_resched();
>> drivers/gpu/drm/udl/udl_gem.c:  set_need_resched();
>>
>> All these sites basically do:
>>
>>   while (!trylock())
>> yield();
>>
>> which is a horrible and broken locking pattern.
>>
>> Firstly its deadlock prone, suppose the faulting process is a FIFOn+1
>> task that preempted the lock holder at FIFOn.
>>
>> Secondly the implementation is worse than usual by abusing
>> VM_FAULT_NOPAGE, which is supposed to install a PTE so that the fault
>> doesn't retry, but you're using it as a get out of fault path. And
>> you're using set_need_resched() which is not something a driver should
>> _ever_ touch.
>>
>> Now I'm going to take away set_need_resched() -- and while you can
>> 'reimplement' it using set_thread_flag() you're not going to do that
>> because it will be broken due to changes to the preempt code.
>>
>> So please as to fix ASAP and don't allow anybody to trick you into
>> merging silly things like that again ;-)
> The set_need_resched in i915_gem.c:i915_gem_fault can actually be
> removed. It was there to give the error handler a chance to sneak in
> and reset the hw/sw tracking when the gpu is dead. That hack goes back
> to the days when the locking around our error handler was somewhere
> between nonexistent and totally broken, nowadays we keep things from
> live-locking by a bit of magic in i915_mutex_lock_interruptible. I'll
> whip up a patch to rip this out. I'll also check that our testsuite
> properly exercises this path (needs a bit of work on a quick look for
> better coverage).
>
> The one in ttm is just bonghits to shut up lockdep: ttm can recurse
> into it's own pagefault handler and then deadlock, the trylock just
> keeps lockdep quiet. We've had that bug arise in drm/i915 due to some
> fun userspace did and now have testcases for them. The right solution
> to fix this is to use copy_to|from_user_atomic in ttm everywhere it
> holds locks and have slowpaths which drops locks, copies stuff into a
> temp allocation and then continues. At least that's how we've fixed
> all those inversions in i915-gem. I'm not volunteering to fix this ;-)
Ah the case where a mmap'd address is passed to the execbuf ioctl? :P

Fine I'll look into it a bit, hopefully before tuesday. Else it might take a 
bit longer since I'll be on my way to plumbers..
> The one in udl just looks like copypasta from i915, without any
> justification (at least I don't see any) for why it's there. Probably
> can die, too, since there isn't any gpu to reset on usb display-link
> devices ...
>
> Cheers, Daniel

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

Re: [Intel-gfx] [BUG] completely bonkers use of set_need_resched + VM_FAULT_NOPAGE

[Thomas Hellstrom]

On 09/12/2013 05:45 PM, Maarten Lankhorst wrote:

Op 12-09-13 17:36, Daniel Vetter schreef:

On Thu, Sep 12, 2013 at 5:06 PM, Peter Zijlstra  wrote:

So I'm poking around the preemption code and stumbled upon:

drivers/gpu/drm/i915/i915_gem.c:set_need_resched();
drivers/gpu/drm/ttm/ttm_bo_vm.c:set_need_resched();
drivers/gpu/drm/ttm/ttm_bo_vm.c:set_need_resched();
drivers/gpu/drm/udl/udl_gem.c:  set_need_resched();

All these sites basically do:

   while (!trylock())
 yield();

which is a horrible and broken locking pattern.

Firstly its deadlock prone, suppose the faulting process is a FIFOn+1
task that preempted the lock holder at FIFOn.

Secondly the implementation is worse than usual by abusing
VM_FAULT_NOPAGE, which is supposed to install a PTE so that the fault
doesn't retry, but you're using it as a get out of fault path. And
you're using set_need_resched() which is not something a driver should
_ever_ touch.

Now I'm going to take away set_need_resched() -- and while you can
'reimplement' it using set_thread_flag() you're not going to do that
because it will be broken due to changes to the preempt code.

So please as to fix ASAP and don't allow anybody to trick you into
merging silly things like that again ;-)

The set_need_resched in i915_gem.c:i915_gem_fault can actually be
removed. It was there to give the error handler a chance to sneak in
and reset the hw/sw tracking when the gpu is dead. That hack goes back
to the days when the locking around our error handler was somewhere
between nonexistent and totally broken, nowadays we keep things from
live-locking by a bit of magic in i915_mutex_lock_interruptible. I'll
whip up a patch to rip this out. I'll also check that our testsuite
properly exercises this path (needs a bit of work on a quick look for
better coverage).

The one in ttm is just bonghits to shut up lockdep: ttm can recurse
into it's own pagefault handler and then deadlock, the trylock just
keeps lockdep quiet. We've had that bug arise in drm/i915 due to some
fun userspace did and now have testcases for them. The right solution
to fix this is to use copy_to|from_user_atomic in ttm everywhere it
holds locks and have slowpaths which drops locks, copies stuff into a
temp allocation and then continues. At least that's how we've fixed
all those inversions in i915-gem. I'm not volunteering to fix this ;-)

Ah the case where a mmap'd address is passed to the execbuf ioctl? :P

Fine I'll look into it a bit, hopefully before tuesday. Else it might take a 
bit longer since I'll be on my way to plumbers..


I think a possible fix would be if fault() were allowed to return an 
error and drop the mmap_sem() before returning.


Otherwise we need to track down all copy_to_user / copy_from_user which 
happen with bo::reserve held.


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

Re: [Intel-gfx] [BUG] completely bonkers use of set_need_resched + VM_FAULT_NOPAGE

[Daniel Vetter]

On Thu, Sep 12, 2013 at 6:22 PM, Peter Zijlstra  wrote:
> If 'sane' userspace is never supposed to do this, then only insane
> userspace is going to hurt from this and that's a GOOD (tm) thing,
> right? ;-)

Afaik sane userspace doesn't hit the _deadlock_ (or lifelock if we
have the set_need_resched in there). drm/i915 is a bit different since
we have just one lock, and so the same design would actually deadlock
even for sane userspace. But hitting contention there and yielding is
somewhat expected. Obviously shouldn't happen too often since it'll
hurt performance, with either blocking or the yield spinning loop.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 4/5] drm/i915: add some assertions about VBT DDI port types

[Paulo Zanoni]

From: Paulo Zanoni 

Our code makes a lot of assumptions regarding what each DDI port
actually supports, and the VBT should tell us what is really happening
in the hardware. So parse the information provided by the VBT and
check if any of our assumptions is wrong.

Our driver also has a history of not really trusting the VBT, so a
WARN here could mean that:
 a) our coding assumptions are wrong
 b) the VBT is wrong
 c) we're incorrectly parsing the VBT
 d) the checks are wrong

But I really hope we won't ever trigger any of those WARNs.

v2: Don't check the redundant "Capabilities" field from byte 24 since
it doesn't seem to be used.
v3: Rebase
v4: Replace WARN with DRM_DEBUG_KMS

Signed-off-by: Paulo Zanoni 
Reviewed-by: Rodrigo Vivi  (v2)
---
 drivers/gpu/drm/i915/intel_bios.c | 24 +++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_bios.c 
b/drivers/gpu/drm/i915/intel_bios.c
index 12e4fd1..7ce1c3c 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -590,7 +590,7 @@ static void parse_ddi_port(struct drm_i915_private 
*dev_priv, enum port port,
struct ddi_vbt_port_info *info = &dev_priv->vbt.ddi_port_info[port];
uint8_t hdmi_level_shift;
int i, j;
-   bool is_dvi, is_dp;
+   bool is_dvi, is_hdmi, is_dp, is_edp, is_crt;
uint8_t aux_channel;
/* Each DDI port can have more than one value on the "DVO Port" field,
 * so look for all the possible values for each port and abort if more
@@ -628,6 +628,28 @@ static void parse_ddi_port(struct drm_i915_private 
*dev_priv, enum port port,
 
is_dvi = child->common.device_type & (1 << 4);
is_dp = child->common.device_type & (1 << 2);
+   is_crt = child->common.device_type & (1 << 0);
+   is_hdmi = is_dvi && (child->common.device_type & (1 << 11)) == 0;
+   is_edp = is_dp && (child->common.device_type & (1 << 12));
+
+   DRM_DEBUG_KMS("Port %c VBT info: DP:%d HDMI:%d DVI:%d EDP:%d CRT:%d\n",
+ port_name(port), is_dp, is_hdmi, is_dvi, is_edp, is_crt);
+
+   if (is_edp && is_dvi)
+   DRM_DEBUG_KMS("Internal DP port %c is TMDS compatible\n",
+ port_name(port));
+   if (is_crt && port != PORT_E)
+   DRM_DEBUG_KMS("Port %c is analog\n", port_name(port));
+   if (is_crt && (is_dvi || is_dp))
+   DRM_DEBUG_KMS("Analog port %c is also DP or TMDS compatible\n",
+ port_name(port));
+   if (is_dvi && (port == PORT_A || port == PORT_E))
+   DRM_DEBUG_KMS("Port %c is TMDS compabile\n", port_name(port));
+   if (!is_dvi && !is_dp && !is_crt)
+   DRM_DEBUG_KMS("Port %c is not DP/TMDS/CRT compatible\n",
+ port_name(port));
+   if (is_edp && (port == PORT_B || port == PORT_C || port == PORT_E))
+   DRM_DEBUG_KMS("Port %c is internal DP\n", port_name(port));
 
if (is_dvi) {
if (child->common.ddc_pin == 0x05 && port != PORT_B)
-- 
1.8.3.1

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

Re: [Intel-gfx] [BUG] completely bonkers use of set_need_resched + VM_FAULT_NOPAGE

[Peter Zijlstra]

On Thu, Sep 12, 2013 at 05:35:43PM +0100, Chris Wilson wrote:
> Not quite, as it would be possible for the evil userspace to trigger a
> GPU hang that would cause the sane userspace to spin indefinitely 
> waiting for the error recovery to kick in.

So with FIFOn+1 preempting FIFOn its a live-lock because the faulting
thread will forever keep yielding to itself since its the highest
priority task around, therefore the set_need_resched() is an absolute
NOP in that case.

For OTHER it might run another task with set_need_resched(), without
set_need_resched() it'll simply spin on the fault until it runs out of
time and gets force preempted and another task gets to run.

So for either case, the set_need_resched() doesn't make an appreciable
difference.

Removing it will not make evil userspace much worse -- at worst it will
cause slightly more wasted cycles.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [BUG] completely bonkers use of set_need_resched + VM_FAULT_NOPAGE

[Daniel Vetter]

On Thu, Sep 12, 2013 at 10:20 PM, Thomas Gleixner  wrote:
>> I think for ttm drivers it's just execbuf being exploitable. But on
>> drm/i915 we've
>> had the same issue with the pwrite/pread ioctls, so a simple
>> glBufferData(glMap) kind of recursion from gl clients blew the kernel
>> to pieces ...
>
> And the only answer you folks came up with is set_need_resched() and
> yield()? Oh well

The yield was for a different lifelock, and that one is also fixed by
now. The fault handler deadlock was fixed in the usual "drop locks and
jump into slowpath" fasion, at least in drm/i915.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/2] drm/i915: kill set_need_resched

[Daniel Vetter]

On Thu, Sep 12, 2013 at 05:59:51PM +0200, Peter Zijlstra wrote:
> On Thu, Sep 12, 2013 at 05:57:28PM +0200, Daniel Vetter wrote:
> > This is just a remnant from the old days when our reset handling was
> > horribly racy, suffered from terribly locking issues and often happily
> > live-locked. Those days are now gone so we can drop the hacks and just
> > rip the reschedule-point out.
> > 
> > Reported-by: Peter Zijlstra 
> 
> Thanks!
> 
> Acked-by: Peter Zijlstra 

Merged to drm-intel-fixes with Chris' irc-review that we indeed don't need
this any more to duct-tape over bugs.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [BUG] completely bonkers use of set_need_resched + VM_FAULT_NOPAGE

[Daniel Vetter]

On Thu, Sep 12, 2013 at 9:58 PM, Thomas Gleixner  wrote:
>
>> On Thu, Sep 12, 2013 at 6:22 PM, Peter Zijlstra  wrote:
>> > If 'sane' userspace is never supposed to do this, then only insane
>> > userspace is going to hurt from this and that's a GOOD (tm) thing,
>> > right? ;-)
>>
>> Afaik sane userspace doesn't hit the _deadlock_ (or lifelock if we
>> have the set_need_resched in there). drm/i915 is a bit different since
>> we have just one lock, and so the same design would actually deadlock
>> even for sane userspace. But hitting contention there and yielding is
>> somewhat expected. Obviously shouldn't happen too often since it'll
>> hurt performance, with either blocking or the yield spinning loop.
>
> So this is actually a non priviledged DoS interface, right?

I think for ttm drivers it's just execbuf being exploitable. But on
drm/i915 we've
had the same issue with the pwrite/pread ioctls, so a simple
glBufferData(glMap) kind of recursion from gl clients blew the kernel
to pieces ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] 3.11.0+ Laptop screen goes blank during kernelboot

[Hans de Bruin]

On 09/10/2013 01:00 PM, Jani Nikula wrote:


Hi Hans -

On Sat, 07 Sep 2013, Hans de Bruin  wrote:

Since a day or two the screen of my laptop goes blank when the kernel
switches resolution in the boot process. Happily the x-server turns it
on again.


Please try the drm-intel-nightly branch of
git://people.freedesktop.org/~danvet/drm-intel


Well the drm-intel-testing branch works fine but that was not the 
question. lets see if I am able to use gits -b option and clone 
drm-intel-nightly.


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

Re: [Intel-gfx] [PATCH] drm/i915/vlv: disable rc6p and rc6pp residency reporting on BYT

[Daniel Vetter]

On Wed, Sep 11, 2013 at 01:43:20PM -0700, Jesse Barnes wrote:
> Unsupported; we just do RC6.
> 
> Signed-off-by: Jesse Barnes 

You only change the sysfs stuff here, so I wondered where the hunk for
intel_pm.c is. And noticed that we don't actually obey the enable_rc6
parameter! Aside: We don't do any such force-to-0 stuff on other
platforms, so why do we need this?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [BUG] completely bonkers use of set_need_resched + VM_FAULT_NOPAGE

[Thomas Hellstrom]

On 09/12/2013 10:39 PM, Thomas Gleixner wrote:

On Thu, 12 Sep 2013, Daniel Vetter wrote:


On Thu, Sep 12, 2013 at 10:20 PM, Thomas Gleixner  wrote:

I think for ttm drivers it's just execbuf being exploitable. But on
drm/i915 we've
had the same issue with the pwrite/pread ioctls, so a simple
glBufferData(glMap) kind of recursion from gl clients blew the kernel
to pieces ...

And the only answer you folks came up with is set_need_resched() and
yield()? Oh well

The yield was for a different lifelock, and that one is also fixed by
now. The fault handler deadlock was fixed in the usual "drop locks and
jump into slowpath" fasion, at least in drm/i915.

So we can remove that whole yield/set_need_resched() mess completely ?

Thanks,

tglx

No.

The while(trylock) is there to address a potential locking inversion 
deadlock. If the trylock fails, the code returns to user-space which 
retries the fault. This code needs to stay until we can come up with 
either a way to drop the mmap_sem and sleep before returning to 
user-space, or a bunch of code is fixed with a different locking order.


The set_need_resched() can (and should according to Peter) go.

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

Re: [Intel-gfx] 3.11.0+ Laptop screen goes blank during kernelboot

[Daniel Vetter]

On Thu, Sep 12, 2013 at 10:45:10PM +0200, Hans de Bruin wrote:
> On 09/10/2013 01:00 PM, Jani Nikula wrote:
> >
> >Hi Hans -
> >
> >On Sat, 07 Sep 2013, Hans de Bruin  wrote:
> >>Since a day or two the screen of my laptop goes blank when the kernel
> >>switches resolution in the boot process. Happily the x-server turns it
> >>on again.
> >
> >Please try the drm-intel-nightly branch of
> >git://people.freedesktop.org/~danvet/drm-intel
> 
> Well the drm-intel-testing branch works fine but that was not the
> question. lets see if I am able to use gits -b option and clone
> drm-intel-nightly.

-testing should be recent enough for the commit Jani mentioned. It's also
already in Linus' git, so hopefully that works again for you.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915/vlv: disable rc6p and rc6pp residency reporting on BYT

[Jesse Barnes]

On Thu, 12 Sep 2013 22:38:41 +0200
Daniel Vetter  wrote:

> On Wed, Sep 11, 2013 at 01:43:20PM -0700, Jesse Barnes wrote:
> > Unsupported; we just do RC6.
> > 
> > Signed-off-by: Jesse Barnes 
> 
> You only change the sysfs stuff here, so I wondered where the hunk for
> intel_pm.c is. And noticed that we don't actually obey the enable_rc6
> parameter! Aside: We don't do any such force-to-0 stuff on other
> platforms, so why do we need this?

Gosh all these questions for such a simple patch.

I was just annoyed that the sysfs rc6 residency test was getting bogus
values from registers that happened to be there but don't correlate
with rc6.

-- 
Jesse Barnes, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 6/5] drm/i915: move more code to __i915_drm_thaw

[Paulo Zanoni]

From: Paulo Zanoni 

Both callers had code to sanitize the uncore and restore the GTT
mappings just before calling __i915_drm_thaw, so Chris suggested I
should unify the code.

Signed-off-by: Paulo Zanoni 
---
 drivers/gpu/drm/i915/i915_drv.c | 39 ++-
 1 file changed, 14 insertions(+), 25 deletions(-)

I'm not sure if this is really what Chris had in mind, but it's an idea...


diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index f6dc6b8..75e7550 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -576,11 +576,20 @@ static void intel_resume_hotplug(struct drm_device *dev)
drm_helper_hpd_irq_event(dev);
 }
 
-static int __i915_drm_thaw(struct drm_device *dev)
+static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
 {
struct drm_i915_private *dev_priv = dev->dev_private;
int error = 0;
 
+   intel_uncore_sanitize(dev);
+
+   if (drm_core_check_feature(dev, DRIVER_MODESET) &&
+   restore_gtt_mappings) {
+   mutex_lock(&dev->struct_mutex);
+   i915_gem_restore_gtt_mappings(dev);
+   mutex_unlock(&dev->struct_mutex);
+   }
+
i915_restore_state(dev);
intel_opregion_setup(dev);
 
@@ -640,19 +649,7 @@ static int __i915_drm_thaw(struct drm_device *dev)
 
 static int i915_drm_thaw(struct drm_device *dev)
 {
-   int error = 0;
-
-   intel_uncore_sanitize(dev);
-
-   if (drm_core_check_feature(dev, DRIVER_MODESET)) {
-   mutex_lock(&dev->struct_mutex);
-   i915_gem_restore_gtt_mappings(dev);
-   mutex_unlock(&dev->struct_mutex);
-   }
-
-   error = __i915_drm_thaw(dev);
-
-   return error;
+   return __i915_drm_thaw(dev, true);
 }
 
 int i915_resume(struct drm_device *dev)
@@ -668,20 +665,12 @@ int i915_resume(struct drm_device *dev)
 
pci_set_master(dev->pdev);
 
-   intel_uncore_sanitize(dev);
-
/*
 * Platforms with opregion should have sane BIOS, older ones (gen3 and
-* earlier) need this since the BIOS might clear all our scratch PTEs.
+* earlier) need to restore the GTT mappings since the BIOS might clear
+* all our scratch PTEs.
 */
-   if (drm_core_check_feature(dev, DRIVER_MODESET) &&
-   !dev_priv->opregion.header) {
-   mutex_lock(&dev->struct_mutex);
-   i915_gem_restore_gtt_mappings(dev);
-   mutex_unlock(&dev->struct_mutex);
-   }
-
-   ret = __i915_drm_thaw(dev);
+   ret = __i915_drm_thaw(dev, !dev_priv->opregion.header);
if (ret)
return ret;
 
-- 
1.8.3.1

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

[Intel-gfx] [PATCH 2/5] drm/i915: fix intel_dp_aux_native_read's reply array size

[Paulo Zanoni]

From: Paulo Zanoni 

So far we control all the reads an none of them exceeds the current
limit of 20 bytes, but we never think about this when reviewing
patches, so we may at some point in the future overflow the buffer.

My initial patch just added a WARN in case we were about to overflow
the buffer, but Chris suggested to make the size of the array dynamic.

Signed-off-by: Paulo Zanoni 
---
 drivers/gpu/drm/i915/intel_dp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 20e468c..bf0b260 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -564,7 +564,7 @@ intel_dp_aux_native_read(struct intel_dp *intel_dp,
 {
uint8_t msg[4];
int msg_bytes;
-   uint8_t reply[20];
+   uint8_t reply[recv_bytes + 1];
int reply_bytes;
uint8_t ack;
int ret;
-- 
1.8.3.1

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

Re: [Intel-gfx] [PATCH 6/5] drm/i915: move more code to __i915_drm_thaw

[Chris Wilson]

On Thu, Sep 12, 2013 at 06:06:43PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni 
> 
> Both callers had code to sanitize the uncore and restore the GTT
> mappings just before calling __i915_drm_thaw, so Chris suggested I
> should unify the code.
> 
> Signed-off-by: Paulo Zanoni 

Slightly more tricky to prove would be that neither i915_restore_state()
nor intel_opregion_setup depends upon the GTT (when modesetting is
enabled) and so coalesce the check+lock into the main MODESET block.
I think it's okay, there is a general ordering issue where we may point
a few registers at the GTT before it is set, but I don't think they are
used until afterwards. However, it is uncertain enough that I wouldn't
reorder the code without the set of init flags Daniel keeps muttering on
about to assert that we don't use partially restored state.

If Jesse wants to shave off a couple of microseconds from the resume
time by doing so, he is more than welcome. :)

Reviewed-by: Chris Wilson 
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [BUG] completely bonkers use of set_need_resched + VM_FAULT_NOPAGE

[Maarten Lankhorst]

Op 12-09-13 18:44, Thomas Hellstrom schreef:
> On 09/12/2013 05:45 PM, Maarten Lankhorst wrote:
>> Op 12-09-13 17:36, Daniel Vetter schreef:
>>> On Thu, Sep 12, 2013 at 5:06 PM, Peter Zijlstra  
>>> wrote:
 So I'm poking around the preemption code and stumbled upon:

 drivers/gpu/drm/i915/i915_gem.c:set_need_resched();
 drivers/gpu/drm/ttm/ttm_bo_vm.c:set_need_resched();
 drivers/gpu/drm/ttm/ttm_bo_vm.c:set_need_resched();
 drivers/gpu/drm/udl/udl_gem.c:  set_need_resched();

 All these sites basically do:

while (!trylock())
  yield();

 which is a horrible and broken locking pattern.

 Firstly its deadlock prone, suppose the faulting process is a FIFOn+1
 task that preempted the lock holder at FIFOn.

 Secondly the implementation is worse than usual by abusing
 VM_FAULT_NOPAGE, which is supposed to install a PTE so that the fault
 doesn't retry, but you're using it as a get out of fault path. And
 you're using set_need_resched() which is not something a driver should
 _ever_ touch.

 Now I'm going to take away set_need_resched() -- and while you can
 'reimplement' it using set_thread_flag() you're not going to do that
 because it will be broken due to changes to the preempt code.

 So please as to fix ASAP and don't allow anybody to trick you into
 merging silly things like that again ;-)
>>> The set_need_resched in i915_gem.c:i915_gem_fault can actually be
>>> removed. It was there to give the error handler a chance to sneak in
>>> and reset the hw/sw tracking when the gpu is dead. That hack goes back
>>> to the days when the locking around our error handler was somewhere
>>> between nonexistent and totally broken, nowadays we keep things from
>>> live-locking by a bit of magic in i915_mutex_lock_interruptible. I'll
>>> whip up a patch to rip this out. I'll also check that our testsuite
>>> properly exercises this path (needs a bit of work on a quick look for
>>> better coverage).
>>>
>>> The one in ttm is just bonghits to shut up lockdep: ttm can recurse
>>> into it's own pagefault handler and then deadlock, the trylock just
>>> keeps lockdep quiet. We've had that bug arise in drm/i915 due to some
>>> fun userspace did and now have testcases for them. The right solution
>>> to fix this is to use copy_to|from_user_atomic in ttm everywhere it
>>> holds locks and have slowpaths which drops locks, copies stuff into a
>>> temp allocation and then continues. At least that's how we've fixed
>>> all those inversions in i915-gem. I'm not volunteering to fix this ;-)
>> Ah the case where a mmap'd address is passed to the execbuf ioctl? :P
>>
>> Fine I'll look into it a bit, hopefully before tuesday. Else it might take a 
>> bit longer since I'll be on my way to plumbers..
>
> I think a possible fix would be if fault() were allowed to return an error 
> and drop the mmap_sem() before returning.
>
> Otherwise we need to track down all copy_to_user / copy_from_user which 
> happen with bo::reserve held.
CONFIG_PROVE_LOCKING=y

and hard grab that reserve lock within the fault handler, done.. lockdep will 
spit it out for you :p

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

[Intel-gfx] [PATCH 0/3] drm/i915: HSW modeset hang fix

[ville . syrjala]

As Paulo explained at [1], there's a modeset related hang occuring on HSW.
I bisected it down to removing a workaround meant for pre-production hardware,
and Paulo cooked up a workaround that worked for him. Unfortunately I was still 
able to trigger the hangs by disabling all planes on pipe B but leaving the 
pipe 
active, and the doing a modeset on pipe A.

The actual problem can occur when we enable any plane on a pipe that is not
fully running yet. When a pipe is considered running depends on a bit on the 
type of encoder used. But in any case we should not really be enabling planes
on disabled pipes anyway, so implementing an actual workaround should not
even be necessary. We just need to be careful when we enable planes.

For me it seems Jani already fixed the problem when he proposed killing the 
cursor
enable calls from the .mode_set hooks. So it looks like our normal plane 
enabling
during .crtc_enable happens late enough for the pipe to be fully up.

I actually want to refactor the plane enabling/disabling during our
.crtc_{enable,disable} a bit. But as that doesn't seem necessary to prevent the
hangs, I'm posting these minimal fixes first, and then we can go crazy later.

[1] http://lists.freedesktop.org/archives/intel-gfx/2013-September/033040.html
___
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: kill set_need_resched

[Rob Clark]

hmm, looks like I cargo-cult'd the same into msm.

I guess in i915 (and ttm) case, the issue arises due to need for CPU
access to buffer via GTT?  In which case I should be safe to drop the
set_need_resched() as well? (Since CPU always has direct access to the
pages.)  Or am I missing something about the original issue that
necessitated set_need_resched()?

BR,
-R


On Thu, Sep 12, 2013 at 11:57 AM, Daniel Vetter  wrote:
> This is just a remnant from the old days when our reset handling was
> horribly racy, suffered from terribly locking issues and often happily
> live-locked. Those days are now gone so we can drop the hacks and just
> rip the reschedule-point out.
>
> Reported-by: Peter Zijlstra 
> Cc: Peter Zijlstra 
> Cc: Linux Kernel Mailing List 
> Signed-off-by: Daniel Vetter 
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 11 ---
>  1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index d80f33d..3871060 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1389,14 +1389,11 @@ out:
> if (i915_terminally_wedged(&dev_priv->gpu_error))
> return VM_FAULT_SIGBUS;
> case -EAGAIN:
> -   /* Give the error handler a chance to run and move the
> -* objects off the GPU active list. Next time we service the
> -* fault, we should be able to transition the page into the
> -* GTT without touching the GPU (and so avoid further
> -* EIO/EGAIN). If the GPU is wedged, then there is no issue
> -* with coherency, just lost writes.
> +   /*
> +* EAGAIN means the gpu is hung and we'll wait for the error
> +* handler to reset everything when re-faulting in
> +* i915_mutex_lock_interruptible.
>  */
> -   set_need_resched();
> case 0:
> case -ERESTARTSYS:
> case -EINTR:
> --
> 1.8.4.rc3
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Enable VLV to work in BIOS-less system

[Lee, Chon Ming]

On 09/12 18:30, Daniel Vetter wrote:
> On Fri, Sep 13, 2013 at 5:59 AM, Chon Ming Lee  
> wrote:
> > In non PC system, such as IVI, may not use BIOS, instead it uses boot
> > loader with only minimal system initialization.  Most of the time, boot
> > loader doesn't come with VBIOS, and depends on device driver to fully
> > initialize the display controller and GPU.
> >
> > For Valleyview, without VBIOS, i915 fails to work.  The patch add some
> > missing init code, such as doing a DPIO CMNRESET and program the GMBUS
> > frequency.
> >
> > Signed-off-by: Chon Ming Lee 
> 
> Generally I prefer patches with tighter topics, grouped into a series.
> For this one I'd split it up into the gmbus setup and the dpio reset,
> so 2 patches.
> 
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h  |8 +
> >  drivers/gpu/drm/i915/intel_display.c |   51 
> > ++
> >  2 files changed, 59 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h 
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index bcee89b..8ddf58a 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -382,6 +382,8 @@
> >  #define   FB_FMAX_VMIN_FREQ_LO_MASK0xf800
> >
> >  /* vlv2 north clock has */
> > +#define CCK_FUSE_REG   0x8
> > +#define  CCK_FUSE_HPLL_FREQ_MASK   0x3
> >  #define CCK_REG_DSI_PLL_FUSE   0x44
> >  #define CCK_REG_DSI_PLL_CONTROL0x48
> >  #define  DSI_PLL_VCO_EN(1 << 31)
> > @@ -1424,6 +1426,12 @@
> >
> >  #define MI_ARB_VLV (VLV_DISPLAY_BASE + 0x6504)
> >
> > +#define CZCLK_CDCLK_FREQ_RATIO (dev_priv->info->display_mmio_offset + 
> > 0x6508)
> > +#define   CDCLK_FREQ_SHIFT 4
> > +#define   CDCLK_FREQ_MASK  0x1f
> > +#define   CZCLK_FREQ_MASK  0xf
> > +#define GMBUS_FREQ (dev_priv->info->display_mmio_offset + 
> > 0x6510)
> > +
> >  /*
> >   * Palette regs
> >   */
> > diff --git a/drivers/gpu/drm/i915/intel_display.c 
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 3c0e0cf..9ef1d28 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -9497,6 +9497,31 @@ static bool has_edp_a(struct drm_device *dev)
> > return true;
> >  }
> >
> > +static void gmbus_set_freq(struct drm_i915_private *dev_priv, u32 
> > hpll_freq)
> > +{
> > +   int cdclk_ratio[] = { 10, 15, 20, 25, 30, 0, 40, 45, 50, 0,
> > +   60, 0, 0, 75, 80, 0, 90, 0, 100, 0,
> > +   0, 0, 120, 0, 0, 0, 0, 0, 150, 0, 160 };
> > +   int vco_freq[] = { 800, 1600, 2000, 2400 };
> > +   int gmbus_freq = 0, cdclk;
> > +   u32 reg_val;
> > +
> > +   BUG_ON(hpll_freq > ARRAY_SIZE(vco_freq));
> > +
> > +   reg_val = I915_READ(CZCLK_CDCLK_FREQ_RATIO);
> > +
> > +   cdclk = ((reg_val >> CDCLK_FREQ_SHIFT) & CDCLK_FREQ_MASK) - 1;
> > +
> > +   if (cdclk_ratio[cdclk])
> > +   gmbus_freq = vco_freq[hpll_freq] / cdclk_ratio[cdclk] * 10;
> > +
> > +   WARN_ON(gmbus_freq == 0);
> > +
> > +   if (gmbus_freq != 0)
> > +   I915_WRITE(GMBUS_FREQ, gmbus_freq);
> > +
> > +}
> > +
> >  static void intel_setup_outputs(struct drm_device *dev)
> >  {
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -9555,6 +9580,32 @@ static void intel_setup_outputs(struct drm_device 
> > *dev)
> 
> Doing this in setup_outputs feels like hiding stuff ;-) I know that
> there's already the pch refclock setup here, but I've just submitted a
> patch to get it out of there.
> 
> The problem is that setup_outputs isn't called on resume, and I
> suspect we actually need this.
> 
> The dpio reset is probably best stuffed into intel_uncore_sanitize
> > if (I915_READ(PCH_DP_D) & DP_DETECTED)
> > intel_dp_init(dev, PCH_DP_D, PORT_D);
> > } else if (IS_VALLEYVIEW(dev)) {
> > +   u32 reg_val;
> > +
> > +   /* Trigger DPIO CMN RESET, require especially in BIOS less
> > +* system
> > +*/
> > +   reg_val = I915_READ(DPIO_CTL);
> > +   if (!(reg_val & 0x1)) {
> > +   I915_WRITE(DPIO_CTL, 0x0);
> > +   I915_WRITE(DPIO_CTL, 0x1);
> > +   POSTING_READ(DPIO_CTL);
> > +   }
> 
> Atm we don't have a suitable "early platform setup" function which is
> called from the right places. I'd shove this into intel_uncore.c:
> intel_uncore_sanitize for now, that should work.
> 
> Maybe once your patches are in I'll throw a intel_ealy_init_hw
> refactoring on top to avoid sprinkling things all over the place
> between the driver load and the resume codepaths.
> 
I have moved according what you suggest.  Now, the system resume is able to 
work now.
Will send out the revise patch.
> > +
> > +   /* In BIOS-le

[Intel-gfx] [PATCH 0/8] DPF (GPU l3 parity detection) improvements

[Ben Widawsky]

Since IVB, our driver has supported GPU L3 cacheline remapping for
parity errors. This is known as, "DPF" for Dynamic Parity Feature. I am
told such an error is a good predictor for a subsequent error in the
same part of the cache.  To address this possible issue for workloads
requiring precise and correct data, like GPGPU workloads the HW has
extra space in the cache which can be dynamically remapped to fill in
the old, faulting parts of the cache. I should also note, to my
knowledge, no such error has actually been seen on either Ivybridge or
Haswell in the wild.

Note, and reminder: GPU L3 is not the same thing as "L3." It is a
special (usually incoherent) cache that is only used by certain
components within the GPU.

Included in the patches:
1. Fix HSW test cases previously submitted and bikeshedded by Ville.
2. Support for an extra area of L3 added in certain HSW SKUs
3. Error injection support from the user space for test.
4. A reference daemon for listening to the parity error events.

Caveats:
* I've not implemented the "hang" injection. I was not clear what it does, and
  I don't really see how it benefits testing the software I have written.

* I am currently missing a test which uses the error injection.
  Volunteers who want to help, please raise your hand. If not, I'll get
  to it as soon as possible.

* We do have a race with the udev mechanism of error delivery. If I
  understand the way udev works, if we have more than 1 event before the
  daemon is woken, the properties will get us the failing cache location
  of the last error only. I think this is okay because of the earlier statement
  that a parity error is a good indicator of a future parity error. One thing
  which I've not done is trying to track when there are missed errors which
  should be possible even if the info about the location of the error can't be
  retrieved.

* There is no way to read out the per context remapping information through
  sysfs. I only expose whether or not a context has outstanding remaps through
  debugfs. This does effect the testability a bit, but the implementation is
  simple enough that I'm not terrible worried.

Ben Widawsky (8):
  drm/i915: Remove extra "ring"
  drm/i915: Round l3 parity reads down
  drm/i915: Fix l3 parity user buffer offset
  drm/i915: Fix HSW parity test
  drm/i915: Add second slice l3 remapping
  drm/i915: Make l3 remapping use the ring
  drm/i915: Keep a list of all contexts
  drm/i915: Do remaps for all contexts

 drivers/gpu/drm/i915/i915_debugfs.c | 23 ++---
 drivers/gpu/drm/i915/i915_drv.h | 13 +++--
 drivers/gpu/drm/i915/i915_gem.c | 46 +-
 drivers/gpu/drm/i915/i915_gem_context.c | 20 +++-
 drivers/gpu/drm/i915/i915_irq.c | 84 +
 drivers/gpu/drm/i915/i915_reg.h |  6 +++
 drivers/gpu/drm/i915/i915_sysfs.c   | 57 +++---
 drivers/gpu/drm/i915/intel_ringbuffer.c |  6 +--
 include/uapi/drm/i915_drm.h |  8 ++--
 9 files changed, 175 insertions(+), 88 deletions(-)

-- 
1.8.4

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

[Intel-gfx] [PATCH 1/8] drm/i915: Remove extra "ring"

[Ben Widawsky]

Sadly, this isn't the first time we've done this:
http://lists.freedesktop.org/archives/intel-gfx/2013-June/029065.html

Signed-off-by: Ben Widawsky 
---
 drivers/gpu/drm/i915/i915_debugfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
b/drivers/gpu/drm/i915/i915_debugfs.c
index 9ac4e31..1d77624 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1462,7 +1462,7 @@ static int i915_context_status(struct seq_file *m, void 
*unused)
 
for_each_ring(ring, dev_priv, i) {
if (ring->default_context) {
-   seq_printf(m, "HW default context %s ring ", 
ring->name);
+   seq_printf(m, "HW default context %s ", ring->name);
describe_obj(m, ring->default_context->obj);
seq_putc(m, '\n');
}
-- 
1.8.4

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

[Intel-gfx] [PATCH 2/8] drm/i915: Round l3 parity reads down

[Ben Widawsky]

We always read a register for l3 parity reads, and we don't really want
to ever let userspace trick us into giving back less than the dword.

Writes are okay because we assume everything will be 0 filled, and as
such, if a user really wants to write less than a dword, let them.

Signed-off-by: Ben Widawsky 
---
 drivers/gpu/drm/i915/i915_sysfs.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_sysfs.c 
b/drivers/gpu/drm/i915/i915_sysfs.c
index c8c4112..9070f50 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -121,6 +121,8 @@ i915_l3_read(struct file *filp, struct kobject *kobj,
uint32_t misccpctl;
int i, ret;
 
+   count = round_down(count, 4);
+
ret = l3_access_valid(drm_dev, offset);
if (ret)
return ret;
-- 
1.8.4

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

[Intel-gfx] [PATCH 4/8] drm/i915: Fix HSW parity test

[Ben Widawsky]

Haswell changed the log registers to be WO, so we can no longer read
them to determine the programming (which sucks, see later note). For
now, simply use the cached value, and hope HW doesn't screw us over.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=57441
Signed-off-by: Ben Widawsky 
---
 drivers/gpu/drm/i915/i915_sysfs.c | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_sysfs.c 
b/drivers/gpu/drm/i915/i915_sysfs.c
index d572435..43c2e81 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -133,6 +133,19 @@ i915_l3_read(struct file *filp, struct kobject *kobj,
if (ret)
return ret;
 
+   if (IS_HASWELL(drm_dev)) {
+   int last = min_t(int, GEN7_L3LOG_SIZE, count + offset);
+   if ((!dev_priv->l3_parity.remap_info))
+   memset(buf + offset, 0, last - offset);
+   else
+   memcpy(buf + offset,
+  dev_priv->l3_parity.remap_info + (offset/4),
+  last - offset);
+
+   i = last;
+   goto out;
+   }
+
misccpctl = I915_READ(GEN7_MISCCPCTL);
I915_WRITE(GEN7_MISCCPCTL, misccpctl & ~GEN7_DOP_CLOCK_GATE_ENABLE);
 
@@ -141,6 +154,7 @@ i915_l3_read(struct file *filp, struct kobject *kobj,
 
I915_WRITE(GEN7_MISCCPCTL, misccpctl);
 
+out:
mutex_unlock(&drm_dev->struct_mutex);
 
return i;
-- 
1.8.4

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

[Intel-gfx] [PATCH 3/8] drm/i915: Fix l3 parity user buffer offset

[Ben Widawsky]

The buf pointer used during l3_write is just char *, therefore it does
not require the silly any addition of offset.

v2: Also fix i915_l3_read with a suggested logic from Ville

Cc: Ville Syrjälä 
Signed-off-by: Ben Widawsky 
---
 drivers/gpu/drm/i915/i915_sysfs.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_sysfs.c 
b/drivers/gpu/drm/i915/i915_sysfs.c
index 9070f50..d572435 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -127,6 +127,8 @@ i915_l3_read(struct file *filp, struct kobject *kobj,
if (ret)
return ret;
 
+   count = min_t(int, GEN7_L3LOG_SIZE-offset, count);
+
ret = i915_mutex_lock_interruptible(drm_dev);
if (ret)
return ret;
@@ -134,14 +136,14 @@ i915_l3_read(struct file *filp, struct kobject *kobj,
misccpctl = I915_READ(GEN7_MISCCPCTL);
I915_WRITE(GEN7_MISCCPCTL, misccpctl & ~GEN7_DOP_CLOCK_GATE_ENABLE);
 
-   for (i = offset; count >= 4 && i < GEN7_L3LOG_SIZE; i += 4, count -= 4)
-   *((uint32_t *)(&buf[i])) = I915_READ(GEN7_L3LOG_BASE + i);
+   for (i = 0; i < count; i += 4)
+   *((uint32_t *)(&buf[i])) = I915_READ(GEN7_L3LOG_BASE + offset + 
i);
 
I915_WRITE(GEN7_MISCCPCTL, misccpctl);
 
mutex_unlock(&drm_dev->struct_mutex);
 
-   return i - offset;
+   return i;
 }
 
 static ssize_t
@@ -186,9 +188,7 @@ i915_l3_write(struct file *filp, struct kobject *kobj,
if (temp)
dev_priv->l3_parity.remap_info = temp;
 
-   memcpy(dev_priv->l3_parity.remap_info + (offset/4),
-  buf + (offset/4),
-  count);
+   memcpy(dev_priv->l3_parity.remap_info + (offset/4), buf, count);
 
i915_gem_l3_remap(drm_dev);
 
-- 
1.8.4

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

[Intel-gfx] [PATCH 5/8] drm/i915: Add second slice l3 remapping

[Ben Widawsky]

Certain HSW SKUs have a second bank of L3. This L3 remapping has a
separate register set, and interrupt from the first "slice". A slice is
simply a term to define some subset of the GPU's l3 cache. This patch
implements both the interrupt handler, and ability to communicate with
userspace about this second slice.

Signed-off-by: Ben Widawsky 
---
 drivers/gpu/drm/i915/i915_drv.h |  9 ++--
 drivers/gpu/drm/i915/i915_gem.c | 26 ++
 drivers/gpu/drm/i915/i915_irq.c | 84 +
 drivers/gpu/drm/i915/i915_reg.h |  6 +++
 drivers/gpu/drm/i915/i915_sysfs.c   | 34 ++---
 drivers/gpu/drm/i915/intel_ringbuffer.c |  6 +--
 include/uapi/drm/i915_drm.h |  8 ++--
 7 files changed, 115 insertions(+), 58 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 81ba5bb..eb90461 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -918,9 +918,11 @@ struct i915_ums_state {
int mm_suspended;
 };
 
+#define MAX_L3_SLICES 2
 struct intel_l3_parity {
-   u32 *remap_info;
+   u32 *remap_info[MAX_L3_SLICES];
struct work_struct error_work;
+   int which_slice;
 };
 
 struct i915_gem_mm {
@@ -1686,7 +1688,8 @@ struct drm_i915_file_private {
 
 #define HAS_FORCE_WAKE(dev) (INTEL_INFO(dev)->has_force_wake)
 
-#define HAS_L3_GPU_CACHE(dev) (IS_IVYBRIDGE(dev) || IS_HASWELL(dev))
+#define HAS_L3_GPU_CACHE(dev) (INTEL_INFO(dev)->gen >= 7)
+#define NUM_L3_SLICES(dev) (IS_HSW_GT3(dev) ? 2 : HAS_L3_GPU_CACHE(dev))
 
 #define GT_FREQUENCY_MULTIPLIER 50
 
@@ -1947,7 +1950,7 @@ bool i915_gem_clflush_object(struct drm_i915_gem_object 
*obj, bool force);
 int __must_check i915_gem_object_finish_gpu(struct drm_i915_gem_object *obj);
 int __must_check i915_gem_init(struct drm_device *dev);
 int __must_check i915_gem_init_hw(struct drm_device *dev);
-void i915_gem_l3_remap(struct drm_device *dev);
+void i915_gem_l3_remap(struct drm_device *dev, int slice);
 void i915_gem_init_swizzling(struct drm_device *dev);
 void i915_gem_cleanup_ringbuffer(struct drm_device *dev);
 int __must_check i915_gpu_idle(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 5b510a3..b11f7d6c 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4256,16 +4256,21 @@ i915_gem_idle(struct drm_device *dev)
return 0;
 }
 
-void i915_gem_l3_remap(struct drm_device *dev)
+void i915_gem_l3_remap(struct drm_device *dev, int slice)
 {
drm_i915_private_t *dev_priv = dev->dev_private;
+   u32 reg_base = GEN7_L3LOG_BASE + (slice * 0x200);
+   u32 *remap_info = dev_priv->l3_parity.remap_info[slice];
u32 misccpctl;
int i;
 
if (!HAS_L3_GPU_CACHE(dev))
return;
 
-   if (!dev_priv->l3_parity.remap_info)
+   if (NUM_L3_SLICES(dev) < 2 && slice)
+   return;
+
+   if (!remap_info)
return;
 
misccpctl = I915_READ(GEN7_MISCCPCTL);
@@ -4273,17 +4278,17 @@ void i915_gem_l3_remap(struct drm_device *dev)
POSTING_READ(GEN7_MISCCPCTL);
 
for (i = 0; i < GEN7_L3LOG_SIZE; i += 4) {
-   u32 remap = I915_READ(GEN7_L3LOG_BASE + i);
-   if (remap && remap != dev_priv->l3_parity.remap_info[i/4])
+   u32 remap = I915_READ(reg_base + i);
+   if (remap && remap != remap_info[i/4])
DRM_DEBUG("0x%x was already programmed to %x\n",
- GEN7_L3LOG_BASE + i, remap);
-   if (remap && !dev_priv->l3_parity.remap_info[i/4])
+ reg_base + i, remap);
+   if (remap && !remap_info[i/4])
DRM_DEBUG_DRIVER("Clearing remapped register\n");
-   I915_WRITE(GEN7_L3LOG_BASE + i, 
dev_priv->l3_parity.remap_info[i/4]);
+   I915_WRITE(reg_base + i, remap_info[i/4]);
}
 
/* Make sure all the writes land before disabling dop clock gating */
-   POSTING_READ(GEN7_L3LOG_BASE);
+   POSTING_READ(reg_base);
 
I915_WRITE(GEN7_MISCCPCTL, misccpctl);
 }
@@ -4377,7 +4382,7 @@ int
 i915_gem_init_hw(struct drm_device *dev)
 {
drm_i915_private_t *dev_priv = dev->dev_private;
-   int ret;
+   int ret, i;
 
if (INTEL_INFO(dev)->gen < 6 && !intel_enable_gtt())
return -EIO;
@@ -4396,7 +4401,8 @@ i915_gem_init_hw(struct drm_device *dev)
I915_WRITE(GEN7_MSG_CTL, temp);
}
 
-   i915_gem_l3_remap(dev);
+   for (i = 0; i < NUM_L3_SLICES(dev); i++)
+   i915_gem_l3_remap(dev, i);
 
i915_gem_init_swizzling(dev);
 
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 13d26cf..62cdf05 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -882,9 +882,10 @@ static void ivybr

[Intel-gfx] [PATCH 6/8] drm/i915: Make l3 remapping use the ring

[Ben Widawsky]

Using LRI for setting the remapping registers allows us to stream l3
remapping information. This is necessary to handle per context remaps as
we'll see implemented in an upcoming patch.

Using the ring also means we don't need to frob the DOP clock gating
bits.

Signed-off-by: Ben Widawsky 
---
 drivers/gpu/drm/i915/i915_drv.h   |  2 +-
 drivers/gpu/drm/i915/i915_gem.c   | 39 +--
 drivers/gpu/drm/i915/i915_sysfs.c |  3 ++-
 3 files changed, 20 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index eb90461..493a9cd 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1950,7 +1950,7 @@ bool i915_gem_clflush_object(struct drm_i915_gem_object 
*obj, bool force);
 int __must_check i915_gem_object_finish_gpu(struct drm_i915_gem_object *obj);
 int __must_check i915_gem_init(struct drm_device *dev);
 int __must_check i915_gem_init_hw(struct drm_device *dev);
-void i915_gem_l3_remap(struct drm_device *dev, int slice);
+int i915_gem_l3_remap(struct intel_ring_buffer *ring, int slice);
 void i915_gem_init_swizzling(struct drm_device *dev);
 void i915_gem_cleanup_ringbuffer(struct drm_device *dev);
 int __must_check i915_gpu_idle(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index b11f7d6c..fa01c69 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4256,41 +4256,36 @@ i915_gem_idle(struct drm_device *dev)
return 0;
 }
 
-void i915_gem_l3_remap(struct drm_device *dev, int slice)
+int i915_gem_l3_remap(struct intel_ring_buffer *ring, int slice)
 {
+   struct drm_device *dev = ring->dev;
drm_i915_private_t *dev_priv = dev->dev_private;
u32 reg_base = GEN7_L3LOG_BASE + (slice * 0x200);
u32 *remap_info = dev_priv->l3_parity.remap_info[slice];
-   u32 misccpctl;
-   int i;
+   int i, ret;
 
if (!HAS_L3_GPU_CACHE(dev))
-   return;
+   return 0;
 
if (NUM_L3_SLICES(dev) < 2 && slice)
-   return;
+   return 0;
 
if (!remap_info)
-   return;
+   return 0;
 
-   misccpctl = I915_READ(GEN7_MISCCPCTL);
-   I915_WRITE(GEN7_MISCCPCTL, misccpctl & ~GEN7_DOP_CLOCK_GATE_ENABLE);
-   POSTING_READ(GEN7_MISCCPCTL);
+   ret = intel_ring_begin(ring, GEN7_L3LOG_SIZE / 4 * 3);
+   if (ret)
+   return ret;
 
for (i = 0; i < GEN7_L3LOG_SIZE; i += 4) {
-   u32 remap = I915_READ(reg_base + i);
-   if (remap && remap != remap_info[i/4])
-   DRM_DEBUG("0x%x was already programmed to %x\n",
- reg_base + i, remap);
-   if (remap && !remap_info[i/4])
-   DRM_DEBUG_DRIVER("Clearing remapped register\n");
-   I915_WRITE(reg_base + i, remap_info[i/4]);
+   intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
+   intel_ring_emit(ring, reg_base + i);
+   intel_ring_emit(ring, remap_info[i/4]);
}
 
-   /* Make sure all the writes land before disabling dop clock gating */
-   POSTING_READ(reg_base);
+   intel_ring_advance(ring);
 
-   I915_WRITE(GEN7_MISCCPCTL, misccpctl);
+   return ret;
 }
 
 void i915_gem_init_swizzling(struct drm_device *dev)
@@ -4401,15 +4396,15 @@ i915_gem_init_hw(struct drm_device *dev)
I915_WRITE(GEN7_MSG_CTL, temp);
}
 
-   for (i = 0; i < NUM_L3_SLICES(dev); i++)
-   i915_gem_l3_remap(dev, i);
-
i915_gem_init_swizzling(dev);
 
ret = i915_gem_init_rings(dev);
if (ret)
return ret;
 
+   for (i = 0; i < NUM_L3_SLICES(dev); i++)
+   i915_gem_l3_remap(&dev_priv->ring[RCS], i);
+
/*
 * XXX: There was some w/a described somewhere suggesting loading
 * contexts before PPGTT.
diff --git a/drivers/gpu/drm/i915/i915_sysfs.c 
b/drivers/gpu/drm/i915/i915_sysfs.c
index d208f2d..65a7274 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -206,7 +206,8 @@ i915_l3_write(struct file *filp, struct kobject *kobj,
 
memcpy(dev_priv->l3_parity.remap_info[slice] + (offset/4), buf, count);
 
-   i915_gem_l3_remap(drm_dev, slice);
+   if (i915_gem_l3_remap(&dev_priv->ring[RCS], slice))
+   count = 0;
 
mutex_unlock(&drm_dev->struct_mutex);
 
-- 
1.8.4

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

[Intel-gfx] [PATCH 7/8] drm/i915: Keep a list of all contexts

[Ben Widawsky]

I have implemented this patch before without creating a separate list
(I'm having trouble finding the links, but the messages ids are:
<1364942743-6041-2-git-send-email-...@bwidawsk.net>
<1365118914-15753-9-git-send-email-...@bwidawsk.net>)

However, the code is much simpler to just use a list and it makes the
code from the next patch a lot more pretty.

As you'll see in the next patch, the reason for this is to be able to
specify when a context needs to get L3 remapping. More details there.

Signed-off-by: Ben Widawsky 
---
 drivers/gpu/drm/i915/i915_debugfs.c | 15 +--
 drivers/gpu/drm/i915/i915_drv.h |  3 +++
 drivers/gpu/drm/i915/i915_gem.c |  1 +
 drivers/gpu/drm/i915/i915_gem_context.c |  3 +++
 4 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
b/drivers/gpu/drm/i915/i915_debugfs.c
index 1d77624..ada0950 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1442,6 +1442,7 @@ static int i915_context_status(struct seq_file *m, void 
*unused)
struct drm_device *dev = node->minor->dev;
drm_i915_private_t *dev_priv = dev->dev_private;
struct intel_ring_buffer *ring;
+   struct i915_hw_context *ctx;
int ret, i;
 
ret = mutex_lock_interruptible(&dev->mode_config.mutex);
@@ -1460,12 +1461,14 @@ static int i915_context_status(struct seq_file *m, void 
*unused)
seq_putc(m, '\n');
}
 
-   for_each_ring(ring, dev_priv, i) {
-   if (ring->default_context) {
-   seq_printf(m, "HW default context %s ", ring->name);
-   describe_obj(m, ring->default_context->obj);
-   seq_putc(m, '\n');
-   }
+   list_for_each_entry(ctx, &dev_priv->context_list, link) {
+   seq_puts(m, "HW context ");
+   for_each_ring(ring, dev_priv, i)
+   if (ring->default_context == ctx)
+   seq_printf(m, "(default context %s) ", 
ring->name);
+
+   describe_obj(m, ctx->obj);
+   seq_putc(m, '\n');
}
 
mutex_unlock(&dev->mode_config.mutex);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 493a9cd..68f992b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -606,6 +606,8 @@ struct i915_hw_context {
struct intel_ring_buffer *ring;
struct drm_i915_gem_object *obj;
struct i915_ctx_hang_stats hang_stats;
+
+   struct list_head link;
 };
 
 struct i915_fbc {
@@ -1344,6 +1346,7 @@ typedef struct drm_i915_private {
 
bool hw_contexts_disabled;
uint32_t hw_context_size;
+   struct list_head context_list;
 
u32 fdi_rx_config;
 
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index fa01c69..fe4e579 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4576,6 +4576,7 @@ i915_gem_load(struct drm_device *dev)
INIT_LIST_HEAD(&dev_priv->vm_list);
i915_init_vm(dev_priv, &dev_priv->gtt.base);
 
+   INIT_LIST_HEAD(&dev_priv->context_list);
INIT_LIST_HEAD(&dev_priv->mm.unbound_list);
INIT_LIST_HEAD(&dev_priv->mm.bound_list);
INIT_LIST_HEAD(&dev_priv->mm.fence_list);
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c 
b/drivers/gpu/drm/i915/i915_gem_context.c
index 26c3fcc..2bbdce8 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -129,6 +129,7 @@ void i915_gem_context_free(struct kref *ctx_ref)
struct i915_hw_context *ctx = container_of(ctx_ref,
   typeof(*ctx), ref);
 
+   list_del(&ctx->link);
drm_gem_object_unreference(&ctx->obj->base);
kfree(ctx);
 }
@@ -147,6 +148,7 @@ create_hw_context(struct drm_device *dev,
 
kref_init(&ctx->ref);
ctx->obj = i915_gem_alloc_object(dev, dev_priv->hw_context_size);
+   INIT_LIST_HEAD(&ctx->link);
if (ctx->obj == NULL) {
kfree(ctx);
DRM_DEBUG_DRIVER("Context object allocated failed\n");
@@ -166,6 +168,7 @@ create_hw_context(struct drm_device *dev,
 * assertion in the context switch code.
 */
ctx->ring = &dev_priv->ring[RCS];
+   list_add_tail(&ctx->link, &dev_priv->context_list);
 
/* Default context will never have a file_priv */
if (file_priv == NULL)
-- 
1.8.4

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

[Intel-gfx] [PATCH 8/8] drm/i915: Do remaps for all contexts

[Ben Widawsky]

On both Ivybridge and Haswell, row remapping information is saved and
restored with context. This means, we never actually properly supported
the l3 remapping because our sysfs interface is asynchronous (and not
tied to any context), and the known faulty HW would be reused by the
next context to run.

Not that due to the asynchronous nature of the sysfs entry, there is no
point modifying the registers for the existing context. Instead we set a
flag for all contexts to load the correct remapping information on the
next run. Interested clients can use debugfs to determine whether or not
the row has been remapped.

One could propose at this point that we just do the remapping in the
kernel. I guess since we have to maintain the sysfs interface anyway,
I'm not sure how useful it is, and I do like keeping the policy in
userspace; (it wasn't my original decision to make the
interface the way it is, so I'm not attached).

Signed-off-by: Ben Widawsky 
---
 drivers/gpu/drm/i915/i915_debugfs.c |  8 +++
 drivers/gpu/drm/i915/i915_drv.h |  1 +
 drivers/gpu/drm/i915/i915_gem_context.c | 17 +--
 drivers/gpu/drm/i915/i915_sysfs.c   | 38 +++--
 4 files changed, 36 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
b/drivers/gpu/drm/i915/i915_debugfs.c
index ada0950..80bed69 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -145,6 +145,13 @@ describe_obj(struct seq_file *m, struct 
drm_i915_gem_object *obj)
seq_printf(m, " (%s)", obj->ring->name);
 }
 
+static void describe_ctx(struct seq_file *m, struct i915_hw_context *ctx)
+{
+   seq_putc(m, ctx->is_initialized ? 'I' : 'i');
+   seq_putc(m, ctx->remap_slice ? 'R' : 'r');
+   seq_putc(m, ' ');
+}
+
 static int i915_gem_object_list_info(struct seq_file *m, void *data)
 {
struct drm_info_node *node = (struct drm_info_node *) m->private;
@@ -1463,6 +1470,7 @@ static int i915_context_status(struct seq_file *m, void 
*unused)
 
list_for_each_entry(ctx, &dev_priv->context_list, link) {
seq_puts(m, "HW context ");
+   describe_ctx(m, ctx);
for_each_ring(ring, dev_priv, i)
if (ring->default_context == ctx)
seq_printf(m, "(default context %s) ", 
ring->name);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 68f992b..6ba78cd 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -602,6 +602,7 @@ struct i915_hw_context {
struct kref ref;
int id;
bool is_initialized;
+   uint8_t remap_slice;
struct drm_i915_file_private *file_priv;
struct intel_ring_buffer *ring;
struct drm_i915_gem_object *obj;
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c 
b/drivers/gpu/drm/i915/i915_gem_context.c
index 2bbdce8..72b7202 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -140,7 +140,7 @@ create_hw_context(struct drm_device *dev,
 {
struct drm_i915_private *dev_priv = dev->dev_private;
struct i915_hw_context *ctx;
-   int ret;
+   int ret, i;
 
ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
if (ctx == NULL)
@@ -181,6 +181,8 @@ create_hw_context(struct drm_device *dev,
 
ctx->file_priv = file_priv;
ctx->id = ret;
+   for (i = 0; i < NUM_L3_SLICES(dev); i++)
+   ctx->remap_slice |= 1 << 1;
 
return ctx;
 
@@ -396,7 +398,7 @@ static int do_switch(struct i915_hw_context *to)
struct intel_ring_buffer *ring = to->ring;
struct i915_hw_context *from = ring->last_context;
u32 hw_flags = 0;
-   int ret;
+   int ret, i;
 
BUG_ON(from != NULL && from->obj != NULL && from->obj->pin_count == 0);
 
@@ -432,6 +434,17 @@ static int do_switch(struct i915_hw_context *to)
return ret;
}
 
+   for (i = 0; i < MAX_L3_SLICES; i++) {
+   if (!(to->remap_slice & (1dev_private;
-   uint32_t misccpctl;
int slice = (int)(uintptr_t)attr->private;
-   int i, ret;
+   int ret;
 
count = round_down(count, 4);
 
@@ -134,31 +133,16 @@ i915_l3_read(struct file *filp, struct kobject *kobj,
if (ret)
return ret;
 
-   if (IS_HASWELL(drm_dev)) {
-   int last = min_t(int, GEN7_L3LOG_SIZE, count + offset);
-   if ((!dev_priv->l3_parity.remap_info[slice]))
-   memset(buf + offset, 0, last - offset);
-   else
-   memcpy(buf + offset,
-  dev_priv->l3_parity.remap_info[slice] + 
(offset/4),
-  last - offset);
-
-   i = last;
-   goto out;
-   }
-
-   mis

[Intel-gfx] [PATCH 10/16] intel_l3_parity: Assert all GEN7+ support

[Ben Widawsky]

Signed-off-by: Ben Widawsky 
---
 tools/intel_l3_parity.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/intel_l3_parity.c b/tools/intel_l3_parity.c
index 970dcd6..a3d268b 100644
--- a/tools/intel_l3_parity.c
+++ b/tools/intel_l3_parity.c
@@ -120,7 +120,7 @@ int main(int argc, char *argv[])
assert(ret != -1);
 
fd = open(path, O_RDWR);
-   if (fd == -1 && IS_IVYBRIDGE(devid)) {
+   if (fd == -1 && intel_gen(devid) > 6) {
perror("Opening sysfs");
exit(EXIT_FAILURE);
} else if (fd == -1)
-- 
1.8.4

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

[Intel-gfx] [PATCH 09/16] intel_l3_parity: Fix indentation

[Ben Widawsky]

Signed-off-by: Ben Widawsky 
---
 tools/intel_l3_parity.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/intel_l3_parity.c b/tools/intel_l3_parity.c
index ad027ac..970dcd6 100644
--- a/tools/intel_l3_parity.c
+++ b/tools/intel_l3_parity.c
@@ -58,12 +58,12 @@ static void dumpit(void)
for (j = 0; j < NUM_SUBBANKS; j++) {
struct l3_log_register *reg = &l3log[i][j];
 
-   if (reg->row0_enable)
-   printf("Row %d, Bank %d, Subbank %d is disabled\n",
-  reg->row0, i, j);
-   if (reg->row1_enable)
-   printf("Row %d, Bank %d, Subbank %d is disabled\n",
-  reg->row1, i, j);
+   if (reg->row0_enable)
+   printf("Row %d, Bank %d, Subbank %d is 
disabled\n",
+  reg->row0, i, j);
+   if (reg->row1_enable)
+   printf("Row %d, Bank %d, Subbank %d is 
disabled\n",
+  reg->row1, i, j);
}
}
 }
-- 
1.8.4

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

[Intel-gfx] [PATCH 11/16] intel_l3_parity: Use getopt for the l3 parity tool

[Ben Widawsky]

Add new command line arguments in addition to supporting the old
features. This patch only introduces one feature, the -e argument to
enable a specific row/bank/subbank. Previously you could only enable
all. Otherwise, it has what you expect (we prefer -r -b -s for
specifying the row/bank/subbank).

Signed-off-by: Ben Widawsky 
---
 tests/sysfs_l3_parity   |  12 ++---
 tools/intel_l3_parity.c | 122 
 2 files changed, 109 insertions(+), 25 deletions(-)

diff --git a/tests/sysfs_l3_parity b/tests/sysfs_l3_parity
index 6f814a1..a0dfad9 100755
--- a/tests/sysfs_l3_parity
+++ b/tests/sysfs_l3_parity
@@ -8,20 +8,20 @@ fi
 SOURCE_DIR="$( dirname "${BASH_SOURCE[0]}" )"
 . $SOURCE_DIR/drm_lib.sh
 
-$SOURCE_DIR/../tools/intel_l3_parity -c
+$SOURCE_DIR/../tools/intel_l3_parity -r 0 -b 0 -s 0 -e
 
 #Check that we can remap a row
-$SOURCE_DIR/../tools/intel_l3_parity 0,0,0
-disabled=`$SOURCE_DIR/../tools/intel_l3_parity | grep -c 'Row 0, Bank 0, 
Subbank 0 is disabled'`
+$SOURCE_DIR/../tools/intel_l3_parity -r 0 -b 0 -s 0 -d
+disabled=`$SOURCE_DIR/../tools/intel_l3_parity -l | grep -c 'Row 0, Bank 0, 
Subbank 0 is disabled'`
 if [ "$disabled" != "1" ] ; then
echo "Fail"
exit 1
 fi
 
-$SOURCE_DIR/../tools/intel_l3_parity -c
+$SOURCE_DIR/../tools/intel_l3_parity -r 0 -b 0 -s 0 -e
 
 #Check that we can clear remaps
-if [ `$SOURCE_DIR/../tools/intel_l3_parity | wc -c` != "0" ] ; then
-   echo "Fail"
+if [ `$SOURCE_DIR/../tools/intel_l3_parity -l | wc -c` != "0" ] ; then
+   echo "Fail 2"
exit 1
 fi
diff --git a/tools/intel_l3_parity.c b/tools/intel_l3_parity.c
index a3d268b..fa5adaa 100644
--- a/tools/intel_l3_parity.c
+++ b/tools/intel_l3_parity.c
@@ -33,12 +33,14 @@
 #include 
 #include 
 #include 
+#include 
 #include "intel_chipset.h"
 #include "intel_gpu_tools.h"
 #include "drmtest.h"
 
 #define NUM_BANKS 4
 #define NUM_SUBBANKS 8
+#define MAX_ROW (1<<12)
 #define NUM_REGS (NUM_BANKS * NUM_SUBBANKS)
 
 struct __attribute__ ((__packed__)) l3_log_register {
@@ -93,17 +95,34 @@ static int disable_rbs(int row, int bank, int sbank)
return 0;
 }
 
-static int do_parse(int argc, char *argv[])
+static void enables_rbs(int row, int bank, int sbank)
 {
-   int row, bank, sbank, i, ret;
+   struct l3_log_register *reg = &l3log[bank][sbank];
 
-   for (i = 1; i < argc; i++) {
-   ret = sscanf(argv[i], "%d,%d,%d", &row, &bank, &sbank);
-   if (ret != 3)
-   return i;
-   assert(disable_rbs(row, bank, sbank) == 0);
-   }
-   return 0;
+   if (!reg->row0_enable && !reg->row1_enable)
+   return;
+
+   if (reg->row1_enable && reg->row1 == row)
+   reg->row1_enable = 0;
+   else if (reg->row0_enable && reg->row0 == row)
+   reg->row0_enable = 0;
+}
+
+static void usage(const char *name)
+{
+   printf("usage: %s [OPTIONS] [ACTION]\n"
+   "Operate on the i915 L3 GPU cache (should be run as root)\n\n"
+   " OPTIONS:\n"
+   "  -r, --row=[row]  The row to act upon 
(default 0)\n"
+   "  -b, --bank=[bank]The bank to act upon 
(default 0)\n"
+   "  -s, --subbank=[subbank]  The subbank to act upon 
(default 0)\n"
+   " ACTIONS (only 1 may be specified at a time):\n"
+   "  -h, --help   Display this help\n"
+   "  -l, --list   List the current L3 
logs\n"
+   "  -a, --clear-all  Clear all disabled 
rows\n"
+   "  -e, --enable Enable row, bank, 
subbank (undo -d)\n"
+   "  -d, --disable= Disable row, bank, 
subbank (inline arguments are deprecated. Please use -r, -b, -s instead\n",
+   name);
 }
 
 int main(int argc, char *argv[])
@@ -111,7 +130,9 @@ int main(int argc, char *argv[])
const int device = drm_get_card();
char *path;
unsigned int devid;
+   int row = 0, bank = 0, sbank = 0;
int drm_fd, fd, ret;
+   int action = '0';
 
drm_fd = drm_open_any();
devid = intel_get_drm_devid(drm_fd);
@@ -134,19 +155,82 @@ int main(int argc, char *argv[])
 
assert(lseek(fd, 0, SEEK_SET) == 0);
 
-   if (argc == 1) {
-   dumpit();
-   exit(EXIT_SUCCESS);
-   } else if (!strncmp("-c", argv[1], 2)) {
-   memset(l3log, 0, sizeof(l3log));
-   } else {
-   ret = do_parse(argc, argv);
-   if (ret != 0) {
-   fprintf(stderr, "Malformed command line at %s\n", 
argv[ret]);
-   exit(EXIT_FAILURE);
+   while (1) {
+   int c, option_index = 0;
+   static struct option long_options[] = {
+   { "help", no_argument, 0, 'h' },
+   { "l

[Intel-gfx] [PATCH 12/16] intel_l3_parity: Hardware info argument

[Ben Widawsky]

Add a new command line argument to the tool which will spit out various
parameters for the giving hardware. As a result of this, some new
defines are added to help with the various info.

Signed-off-by: Ben Widawsky 
---
 tools/intel_l3_parity.c | 24 
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/tools/intel_l3_parity.c b/tools/intel_l3_parity.c
index fa5adaa..5031fbd 100644
--- a/tools/intel_l3_parity.c
+++ b/tools/intel_l3_parity.c
@@ -38,9 +38,19 @@
 #include "intel_gpu_tools.h"
 #include "drmtest.h"
 
-#define NUM_BANKS 4
+static unsigned int devid;
+/* L3 size is always a function of banks. The number of banks cannot be
+ * determined by number of slices however */
+#define MAX_BANKS 4
+#define NUM_BANKS \
+   ((devid == PCI_CHIP_IVYBRIDGE_GT1 || devid == PCI_CHIP_IVYBRIDGE_M_GT1) 
? 2 : 4)
 #define NUM_SUBBANKS 8
+#define BYTES_PER_BANK (128 << 10)
+/* Each row addresses [up to] 4b. This multiplied by the number of subbanks
+ * will give the L3 size per bank.
+ * TODO: Row size is fixed on IVB, and variable on HSW.*/
 #define MAX_ROW (1<<12)
+#define L3_SIZE ((MAX_ROW * 4) * NUM_SUBBANKS *  NUM_BANKS)
 #define NUM_REGS (NUM_BANKS * NUM_SUBBANKS)
 
 struct __attribute__ ((__packed__)) l3_log_register {
@@ -50,7 +60,7 @@ struct __attribute__ ((__packed__)) l3_log_register {
uint32_t row1_enable: 1;
uint32_t rsvd1  : 4;
uint32_t row1   : 11;
-} l3log[NUM_BANKS][NUM_SUBBANKS];
+} l3log[MAX_BANKS][NUM_SUBBANKS];
 
 static void dumpit(void)
 {
@@ -118,6 +128,7 @@ static void usage(const char *name)
"  -s, --subbank=[subbank]  The subbank to act upon 
(default 0)\n"
" ACTIONS (only 1 may be specified at a time):\n"
"  -h, --help   Display this help\n"
+   "  -H, --hw-infoDisplay the 
current L3 properties\n"
"  -l, --list   List the current L3 
logs\n"
"  -a, --clear-all  Clear all disabled 
rows\n"
"  -e, --enable Enable row, bank, 
subbank (undo -d)\n"
@@ -129,7 +140,6 @@ int main(int argc, char *argv[])
 {
const int device = drm_get_card();
char *path;
-   unsigned int devid;
int row = 0, bank = 0, sbank = 0;
int drm_fd, fd, ret;
int action = '0';
@@ -163,13 +173,14 @@ int main(int argc, char *argv[])
{ "clear-all", no_argument, 0, 'a' },
{ "enable", no_argument, 0, 'e' },
{ "disable", optional_argument, 0, 'd' },
+   { "hw-info", no_argument, 0, 'H' },
{ "row", required_argument, 0, 'r' },
{ "bank", required_argument, 0, 'b' },
{ "subbank", required_argument, 0, 's' },
{0, 0, 0, 0}
};
 
-   c = getopt_long(argc, argv, "hr:b:s:aled::", long_options,
+   c = getopt_long(argc, argv, "hHr:b:s:aled::", long_options,
&option_index);
if (c == -1)
break;
@@ -179,6 +190,11 @@ int main(int argc, char *argv[])
case 'h':
usage(argv[0]);
exit(EXIT_SUCCESS);
+   case 'H':
+   printf("Number of banks: %d\n", NUM_BANKS);
+   printf("Subbanks per bank: %d\n", NUM_SUBBANKS);
+   printf("L3 size: %dK\n", L3_SIZE >> 10);
+   exit(EXIT_SUCCESS);
case 'r':
row = atoi(optarg);
if (row >= MAX_ROW)
-- 
1.8.4

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

[Intel-gfx] [PATCH 13/16] intel_l3_parity: slice support

[Ben Widawsky]

Haswell GT3 adds a new slice which is kept distinct from the old
register interface. Plumb it into the code, though it's only 1 slice
still.

Signed-off-by: Ben Widawsky 
---
 tools/intel_l3_parity.c | 113 +---
 1 file changed, 68 insertions(+), 45 deletions(-)

diff --git a/tools/intel_l3_parity.c b/tools/intel_l3_parity.c
index 5031fbd..fd09d39 100644
--- a/tools/intel_l3_parity.c
+++ b/tools/intel_l3_parity.c
@@ -52,6 +52,8 @@ static unsigned int devid;
 #define MAX_ROW (1<<12)
 #define L3_SIZE ((MAX_ROW * 4) * NUM_SUBBANKS *  NUM_BANKS)
 #define NUM_REGS (NUM_BANKS * NUM_SUBBANKS)
+#define MAX_SLICES 1
+#define REAL_MAX_SLICES 1
 
 struct __attribute__ ((__packed__)) l3_log_register {
uint32_t row0_enable: 1;
@@ -60,15 +62,21 @@ struct __attribute__ ((__packed__)) l3_log_register {
uint32_t row1_enable: 1;
uint32_t rsvd1  : 4;
uint32_t row1   : 11;
-} l3log[MAX_BANKS][NUM_SUBBANKS];
+} l3logs[REAL_MAX_SLICES][MAX_BANKS][NUM_SUBBANKS];
 
-static void dumpit(void)
+static int which_slice = -1;
+#define for_each_slice(__i) \
+   for ((__i) = (which_slice == -1) ? 0 : which_slice; \
+   (__i) < ((which_slice == -1) ? MAX_SLICES : 
(which_slice + 1)); \
+   (__i)++)
+
+static void dumpit(int slice)
 {
int i, j;
 
for (i = 0; i < NUM_BANKS; i++) {
for (j = 0; j < NUM_SUBBANKS; j++) {
-   struct l3_log_register *reg = &l3log[i][j];
+   struct l3_log_register *reg = &l3logs[slice][i][j];
 
if (reg->row0_enable)
printf("Row %d, Bank %d, Subbank %d is 
disabled\n",
@@ -80,9 +88,9 @@ static void dumpit(void)
}
 }
 
-static int disable_rbs(int row, int bank, int sbank)
+static int disable_rbs(int row, int bank, int sbank, int slice)
 {
-   struct l3_log_register *reg = &l3log[bank][sbank];
+   struct l3_log_register *reg = &l3logs[slice][bank][sbank];
 
// can't map more than 2 rows
if (reg->row0_enable && reg->row1_enable)
@@ -105,9 +113,9 @@ static int disable_rbs(int row, int bank, int sbank)
return 0;
 }
 
-static void enables_rbs(int row, int bank, int sbank)
+static void enables_rbs(int row, int bank, int sbank, int slice)
 {
-   struct l3_log_register *reg = &l3log[bank][sbank];
+   struct l3_log_register *reg = &l3logs[slice][bank][sbank];
 
if (!reg->row0_enable && !reg->row1_enable)
return;
@@ -126,6 +134,7 @@ static void usage(const char *name)
"  -r, --row=[row]  The row to act upon 
(default 0)\n"
"  -b, --bank=[bank]The bank to act upon 
(default 0)\n"
"  -s, --subbank=[subbank]  The subbank to act upon 
(default 0)\n"
+   "  -w, --slice=[slice]  Which slice to act on 
(default: -1 [all])"
" ACTIONS (only 1 may be specified at a time):\n"
"  -h, --help   Display this help\n"
"  -H, --hw-infoDisplay the 
current L3 properties\n"
@@ -139,31 +148,30 @@ static void usage(const char *name)
 int main(int argc, char *argv[])
 {
const int device = drm_get_card();
-   char *path;
+   char *path[REAL_MAX_SLICES];
int row = 0, bank = 0, sbank = 0;
-   int drm_fd, fd, ret;
+   int fd[REAL_MAX_SLICES] = {0}, ret, i;
int action = '0';
-
-   drm_fd = drm_open_any();
+   int drm_fd = drm_open_any();
devid = intel_get_drm_devid(drm_fd);
 
-   ret = asprintf(&path, "/sys/class/drm/card%d/l3_parity", device);
-   assert(ret != -1);
-
-   fd = open(path, O_RDWR);
-   if (fd == -1 && intel_gen(devid) > 6) {
-   perror("Opening sysfs");
-   exit(EXIT_FAILURE);
-   } else if (fd == -1)
+   if (intel_gen(devid) < 7)
exit(EXIT_SUCCESS);
 
-   ret = read(fd, l3log, NUM_REGS * sizeof(uint32_t));
-   if (ret == -1) {
-   perror("Reading sysfs");
-   exit(EXIT_FAILURE);
+   ret = asprintf(&path[0], "/sys/class/drm/card%d/l3_parity", device);
+   assert(ret != -1);
+
+   for_each_slice(i) {
+   fd[i] = open(path[i], O_RDWR);
+   assert(fd[i]);
+   ret = read(fd[i], l3logs[i], NUM_REGS * sizeof(uint32_t));
+   if (ret == -1) {
+   perror("Reading sysfs");
+   exit(EXIT_FAILURE);
+   }
+   assert(lseek(fd[i], 0, SEEK_SET) == 0);
}
 
-   assert(lseek(fd, 0, SEEK_SET) == 0);
 
while (1) {
int c, option_index = 0;
@@ -177,10 +185,11 @@ int main(int argc, char *argv[])
{ "row", required_argument, 0, 'r' },
{ "bank", r

[Intel-gfx] [PATCH 15/16] intel_l3_parity: Support error injection

[Ben Widawsky]

Haswell added the ability to inject errors which is extremely useful for
testing. Add two arguments to the tool to inject, and uninject.

Signed-off-by: Ben Widawsky 
---
 tests/sysfs_l3_parity   |  2 +-
 tools/intel_l3_parity.c | 69 +++--
 2 files changed, 68 insertions(+), 3 deletions(-)

diff --git a/tests/sysfs_l3_parity b/tests/sysfs_l3_parity
index a0dfad9..e9d4411 100755
--- a/tests/sysfs_l3_parity
+++ b/tests/sysfs_l3_parity
@@ -21,7 +21,7 @@ fi
 $SOURCE_DIR/../tools/intel_l3_parity -r 0 -b 0 -s 0 -e
 
 #Check that we can clear remaps
-if [ `$SOURCE_DIR/../tools/intel_l3_parity -l | wc -c` != "0" ] ; then
+if [ `$SOURCE_DIR/../tools/intel_l3_parity -l | wc -l` != 1 ] ; then
echo "Fail 2"
exit 1
 fi
diff --git a/tools/intel_l3_parity.c b/tools/intel_l3_parity.c
index cf15541..cd6754e 100644
--- a/tools/intel_l3_parity.c
+++ b/tools/intel_l3_parity.c
@@ -79,6 +79,20 @@ static int which_slice = -1;
(__i) < ((which_slice == -1) ? MAX_SLICES : 
(which_slice + 1)); \
(__i)++)
 
+static void decode_dft(uint32_t dft)
+{
+   if (IS_IVYBRIDGE(devid) || !(dft & 1)) {
+   printf("Error injection disabled\n");
+   return;
+   }
+   printf("Error injection enabled\n");
+   printf("  Hang = %s\n", (dft >> 28) & 0x1 ? "yes" : "no");
+   printf("  Row = %d\n", (dft >> 7) & 0x7ff);
+   printf("  Bank = %d\n", (dft >> 2) & 0x3);
+   printf("  Subbank = %d\n", (dft >> 4) & 0x7);
+   printf("  Slice = %d\n", (dft >> 1) & 0x1);
+}
+
 static void dumpit(int slice)
 {
int i, j;
@@ -150,7 +164,9 @@ static void usage(const char *name)
"  -l, --list   List the current L3 
logs\n"
"  -a, --clear-all  Clear all disabled 
rows\n"
"  -e, --enable Enable row, bank, 
subbank (undo -d)\n"
-   "  -d, --disable= Disable row, bank, 
subbank (inline arguments are deprecated. Please use -r, -b, -s instead\n",
+   "  -d, --disable= Disable row, bank, 
subbank (inline arguments are deprecated. Please use -r, -b, -s instead\n"
+   "  -i, --inject [HSW only] Cause 
hardware to inject a row errors\n"
+   "  -u, --uninject   [HSW only] Turn off 
hardware error injectection (undo -i)\n",
name);
 }
 
@@ -158,6 +174,7 @@ int main(int argc, char *argv[])
 {
const int device = drm_get_card();
char *path[REAL_MAX_SLICES];
+   uint32_t dft;
int row = 0, bank = 0, sbank = 0;
int fd[REAL_MAX_SLICES] = {0}, ret, i;
int action = '0';
@@ -167,6 +184,8 @@ int main(int argc, char *argv[])
if (intel_gen(devid) < 7)
exit(EXIT_SUCCESS);
 
+   assert(intel_register_access_init(intel_get_pci_device(), 0) == 0);
+
ret = asprintf(&path[0], "/sys/class/drm/card%d/l3_parity", device);
assert(ret != -1);
ret = asprintf(&path[1], "/sys/class/drm/card%d/l3_parity_slice_1", 
device);
@@ -183,6 +202,7 @@ int main(int argc, char *argv[])
assert(lseek(fd[i], 0, SEEK_SET) == 0);
}
 
+   dft = intel_register_read(0xb038);
 
while (1) {
int c, option_index = 0;
@@ -192,6 +212,8 @@ int main(int argc, char *argv[])
{ "clear-all", no_argument, 0, 'a' },
{ "enable", no_argument, 0, 'e' },
{ "disable", optional_argument, 0, 'd' },
+   { "inject", no_argument, 0, 'i' },
+   { "uninject", no_argument, 0, 'u' },
{ "hw-info", no_argument, 0, 'H' },
{ "row", required_argument, 0, 'r' },
{ "bank", required_argument, 0, 'b' },
@@ -200,7 +222,7 @@ int main(int argc, char *argv[])
{0, 0, 0, 0}
};
 
-   c = getopt_long(argc, argv, "hHr:b:s:w:aled::", long_options,
+   c = getopt_long(argc, argv, "hHr:b:s:w:aled::iu", long_options,
&option_index);
if (c == -1)
break;
@@ -215,6 +237,7 @@ int main(int argc, char *argv[])
printf("Number of banks: %d\n", num_banks());
printf("Subbanks per bank: %d\n", NUM_SUBBANKS);
printf("Max L3 size: %dK\n", L3_SIZE >> 10);
+   printf("Has error injection: %s\n", 
IS_HASWELL(devid) ? "yes" : "no");
exit(EXIT_SUCCESS);
case 'r':
row = atoi(optarg);
@@ -236,6 +259,12 @@ int main(int argc, char *argv[])
if (which_slice >= MAX_SLICES)
 

[Intel-gfx] [PATCH 14/16] intel_l3_parity: Actually support multiple slices

[Ben Widawsky]

Signed-off-by: Ben Widawsky 
---
 tools/intel_l3_parity.c | 45 -
 1 file changed, 28 insertions(+), 17 deletions(-)

diff --git a/tools/intel_l3_parity.c b/tools/intel_l3_parity.c
index fd09d39..cf15541 100644
--- a/tools/intel_l3_parity.c
+++ b/tools/intel_l3_parity.c
@@ -41,19 +41,28 @@
 static unsigned int devid;
 /* L3 size is always a function of banks. The number of banks cannot be
  * determined by number of slices however */
-#define MAX_BANKS 4
-#define NUM_BANKS \
-   ((devid == PCI_CHIP_IVYBRIDGE_GT1 || devid == PCI_CHIP_IVYBRIDGE_M_GT1) 
? 2 : 4)
+static inline int num_banks(void) {
+   if (IS_HSW_GT3(devid))
+   return 8; /* 4 per each slice */
+   else if (IS_HSW_GT1(devid) ||
+   devid == PCI_CHIP_IVYBRIDGE_GT1 ||
+   devid == PCI_CHIP_IVYBRIDGE_M_GT1)
+   return 2;
+   else
+   return 4;
+}
 #define NUM_SUBBANKS 8
 #define BYTES_PER_BANK (128 << 10)
 /* Each row addresses [up to] 4b. This multiplied by the number of subbanks
  * will give the L3 size per bank.
  * TODO: Row size is fixed on IVB, and variable on HSW.*/
 #define MAX_ROW (1<<12)
-#define L3_SIZE ((MAX_ROW * 4) * NUM_SUBBANKS *  NUM_BANKS)
-#define NUM_REGS (NUM_BANKS * NUM_SUBBANKS)
-#define MAX_SLICES 1
-#define REAL_MAX_SLICES 1
+#define MAX_BANKS_PER_SLICE 4
+#define NUM_REGS (MAX_BANKS_PER_SLICE * NUM_SUBBANKS)
+#define MAX_SLICES (IS_HSW_GT3(devid) ? 2 : 1)
+#define REAL_MAX_SLICES 2
+/* TODO support SLM config */
+#define L3_SIZE ((MAX_ROW * 4) * NUM_SUBBANKS *  num_banks())
 
 struct __attribute__ ((__packed__)) l3_log_register {
uint32_t row0_enable: 1;
@@ -62,7 +71,7 @@ struct __attribute__ ((__packed__)) l3_log_register {
uint32_t row1_enable: 1;
uint32_t rsvd1  : 4;
uint32_t row1   : 11;
-} l3logs[REAL_MAX_SLICES][MAX_BANKS][NUM_SUBBANKS];
+} l3logs[REAL_MAX_SLICES][MAX_BANKS_PER_SLICE][NUM_SUBBANKS];
 
 static int which_slice = -1;
 #define for_each_slice(__i) \
@@ -74,16 +83,16 @@ static void dumpit(int slice)
 {
int i, j;
 
-   for (i = 0; i < NUM_BANKS; i++) {
+   for (i = 0; i < MAX_BANKS_PER_SLICE; i++) {
for (j = 0; j < NUM_SUBBANKS; j++) {
struct l3_log_register *reg = &l3logs[slice][i][j];
 
if (reg->row0_enable)
-   printf("Row %d, Bank %d, Subbank %d is 
disabled\n",
-  reg->row0, i, j);
+   printf("Slice %d, Row %d, Bank %d, Subbank %d 
is disabled\n",
+  slice, reg->row0, i, j);
if (reg->row1_enable)
-   printf("Row %d, Bank %d, Subbank %d is 
disabled\n",
-  reg->row1, i, j);
+   printf("Slice %d, Row %d, Bank %d, Subbank %d 
is disabled\n",
+  slice, reg->row1, i, j);
}
}
 }
@@ -160,6 +169,8 @@ int main(int argc, char *argv[])
 
ret = asprintf(&path[0], "/sys/class/drm/card%d/l3_parity", device);
assert(ret != -1);
+   ret = asprintf(&path[1], "/sys/class/drm/card%d/l3_parity_slice_1", 
device);
+   assert(ret != -1);
 
for_each_slice(i) {
fd[i] = open(path[i], O_RDWR);
@@ -201,9 +212,9 @@ int main(int argc, char *argv[])
exit(EXIT_SUCCESS);
case 'H':
printf("Number of slices: %d\n", MAX_SLICES);
-   printf("Number of banks: %d\n", NUM_BANKS);
+   printf("Number of banks: %d\n", num_banks());
printf("Subbanks per bank: %d\n", NUM_SUBBANKS);
-   printf("L3 size: %dK\n", L3_SIZE >> 10);
+   printf("Max L3 size: %dK\n", L3_SIZE >> 10);
exit(EXIT_SUCCESS);
case 'r':
row = atoi(optarg);
@@ -212,7 +223,7 @@ int main(int argc, char *argv[])
break;
case 'b':
bank = atoi(optarg);
-   if (bank >= NUM_BANKS)
+   if (bank >= num_banks() || bank >= 
MAX_BANKS_PER_SLICE)
exit(EXIT_FAILURE);
break;
case 's':
@@ -222,7 +233,7 @@ int main(int argc, char *argv[])
break;
case 'w':
which_slice = atoi(optarg);
-   if (which_slice > 1)
+   if (which_slice >= MAX_SLICES)
exit(EXIT_F

[Intel-gfx] [PATCH 16/16] intel_l3_parity: Support a daemonic mode

[Ben Widawsky]

Signed-off-by: Ben Widawsky 
---
 tools/Makefile.am  |   6 ++-
 tools/intel_l3_parity.c|  39 +--
 tools/intel_l3_parity.h|  31 
 tools/intel_l3_udev_listener.c | 108 +
 4 files changed, 179 insertions(+), 5 deletions(-)
 create mode 100644 tools/intel_l3_parity.h
 create mode 100644 tools/intel_l3_udev_listener.c

diff --git a/tools/Makefile.am b/tools/Makefile.am
index 47bd5b3..19810cf 100644
--- a/tools/Makefile.am
+++ b/tools/Makefile.am
@@ -39,7 +39,7 @@ dist_bin_SCRIPTS = intel_gpu_abrt
 
 AM_CPPFLAGS = -I$(top_srcdir) -I$(top_srcdir)/lib
 AM_CFLAGS = $(DRM_CFLAGS) $(PCIACCESS_CFLAGS) $(CWARNFLAGS) $(CAIRO_CFLAGS)
-LDADD = $(top_builddir)/lib/libintel_tools.la $(DRM_LIBS) $(PCIACCESS_LIBS) 
$(CAIRO_LIBS)
+LDADD = $(top_builddir)/lib/libintel_tools.la $(DRM_LIBS) $(PCIACCESS_LIBS) 
$(CAIRO_LIBS) $(LIBUDEV_LIBS)
 
 intel_dump_decode_SOURCES =\
intel_dump_decode.c
@@ -50,3 +50,7 @@ intel_error_decode_SOURCES =  \
 intel_bios_reader_SOURCES =\
intel_bios_reader.c \
intel_bios.h
+
+intel_l3_parity_SOURCES =  \
+   intel_l3_parity.c   \
+   intel_l3_udev_listener.c
diff --git a/tools/intel_l3_parity.c b/tools/intel_l3_parity.c
index cd6754e..1af424e 100644
--- a/tools/intel_l3_parity.c
+++ b/tools/intel_l3_parity.c
@@ -37,6 +37,14 @@
 #include "intel_chipset.h"
 #include "intel_gpu_tools.h"
 #include "drmtest.h"
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#endif
+#if HAVE_UDEV
+#include 
+#include 
+#endif
+#include "intel_l3_parity.h"
 
 static unsigned int devid;
 /* L3 size is always a function of banks. The number of banks cannot be
@@ -157,7 +165,8 @@ static void usage(const char *name)
"  -r, --row=[row]  The row to act upon 
(default 0)\n"
"  -b, --bank=[bank]The bank to act upon 
(default 0)\n"
"  -s, --subbank=[subbank]  The subbank to act upon 
(default 0)\n"
-   "  -w, --slice=[slice]  Which slice to act on 
(default: -1 [all])"
+   "  -w, --slice=[slice]  Which slice to act on 
(default: -1 [all])\n"
+   ", --daemon Run the listener (-L) 
as a daemon\n"
" ACTIONS (only 1 may be specified at a time):\n"
"  -h, --help   Display this help\n"
"  -H, --hw-infoDisplay the 
current L3 properties\n"
@@ -166,7 +175,8 @@ static void usage(const char *name)
"  -e, --enable Enable row, bank, 
subbank (undo -d)\n"
"  -d, --disable= Disable row, bank, 
subbank (inline arguments are deprecated. Please use -r, -b, -s instead\n"
"  -i, --inject [HSW only] Cause 
hardware to inject a row errors\n"
-   "  -u, --uninject   [HSW only] Turn off 
hardware error injectection (undo -i)\n",
+   "  -u, --uninject   [HSW only] Turn off 
hardware error injectection (undo -i)\n"
+   "  -L, --listen Listen for uevent 
errors\n",
name);
 }
 
@@ -179,6 +189,7 @@ int main(int argc, char *argv[])
int fd[REAL_MAX_SLICES] = {0}, ret, i;
int action = '0';
int drm_fd = drm_open_any();
+   int daemonize = 0;
devid = intel_get_drm_devid(drm_fd);
 
if (intel_gen(devid) < 7)
@@ -206,7 +217,7 @@ int main(int argc, char *argv[])
 
while (1) {
int c, option_index = 0;
-   static struct option long_options[] = {
+   struct option long_options[] = {
{ "help", no_argument, 0, 'h' },
{ "list", no_argument, 0, 'l' },
{ "clear-all", no_argument, 0, 'a' },
@@ -215,18 +226,23 @@ int main(int argc, char *argv[])
{ "inject", no_argument, 0, 'i' },
{ "uninject", no_argument, 0, 'u' },
{ "hw-info", no_argument, 0, 'H' },
+   { "listen", no_argument, 0, 'L' },
{ "row", required_argument, 0, 'r' },
{ "bank", required_argument, 0, 'b' },
{ "subbank", required_argument, 0, 's' },
{ "slice", required_argument, 0, 'w' },
+   { "daemon", no_argument, &daemonize, 1 },
{0, 0, 0, 0}
};
 
-   c = getopt_long(argc, argv, "hHr:b:s:w:aled::iu", long_options,
+   c = getopt_long(argc, argv, "hHr:b:s:w:aled::iuL", long_options,
&option_index);
if (c == -1)
break;
 
+   if (c == 0)
+   contin

Re: [Intel-gfx] [BUG] completely bonkers use of set_need_resched + VM_FAULT_NOPAGE

[Thomas Hellstrom]

On 09/12/2013 11:50 PM, Maarten Lankhorst wrote:

Op 12-09-13 18:44, Thomas Hellstrom schreef:

On 09/12/2013 05:45 PM, Maarten Lankhorst wrote:

Op 12-09-13 17:36, Daniel Vetter schreef:

On Thu, Sep 12, 2013 at 5:06 PM, Peter Zijlstra  wrote:

So I'm poking around the preemption code and stumbled upon:

drivers/gpu/drm/i915/i915_gem.c:set_need_resched();
drivers/gpu/drm/ttm/ttm_bo_vm.c:set_need_resched();
drivers/gpu/drm/ttm/ttm_bo_vm.c:set_need_resched();
drivers/gpu/drm/udl/udl_gem.c:  set_need_resched();

All these sites basically do:

while (!trylock())
  yield();

which is a horrible and broken locking pattern.

Firstly its deadlock prone, suppose the faulting process is a FIFOn+1
task that preempted the lock holder at FIFOn.

Secondly the implementation is worse than usual by abusing
VM_FAULT_NOPAGE, which is supposed to install a PTE so that the fault
doesn't retry, but you're using it as a get out of fault path. And
you're using set_need_resched() which is not something a driver should
_ever_ touch.

Now I'm going to take away set_need_resched() -- and while you can
'reimplement' it using set_thread_flag() you're not going to do that
because it will be broken due to changes to the preempt code.

So please as to fix ASAP and don't allow anybody to trick you into
merging silly things like that again ;-)

The set_need_resched in i915_gem.c:i915_gem_fault can actually be
removed. It was there to give the error handler a chance to sneak in
and reset the hw/sw tracking when the gpu is dead. That hack goes back
to the days when the locking around our error handler was somewhere
between nonexistent and totally broken, nowadays we keep things from
live-locking by a bit of magic in i915_mutex_lock_interruptible. I'll
whip up a patch to rip this out. I'll also check that our testsuite
properly exercises this path (needs a bit of work on a quick look for
better coverage).

The one in ttm is just bonghits to shut up lockdep: ttm can recurse
into it's own pagefault handler and then deadlock, the trylock just
keeps lockdep quiet. We've had that bug arise in drm/i915 due to some
fun userspace did and now have testcases for them. The right solution
to fix this is to use copy_to|from_user_atomic in ttm everywhere it
holds locks and have slowpaths which drops locks, copies stuff into a
temp allocation and then continues. At least that's how we've fixed
all those inversions in i915-gem. I'm not volunteering to fix this ;-)

Ah the case where a mmap'd address is passed to the execbuf ioctl? :P

Fine I'll look into it a bit, hopefully before tuesday. Else it might take a 
bit longer since I'll be on my way to plumbers..

I think a possible fix would be if fault() were allowed to return an error and 
drop the mmap_sem() before returning.

Otherwise we need to track down all copy_to_user / copy_from_user which happen 
with bo::reserve held.

CONFIG_PROVE_LOCKING=y

and hard grab that reserve lock within the fault handler, done.. lockdep will 
spit it out for you :p

~Maarten


Given that all copy_to_user / copy_from_user paths are actually hit 
during testing, right?


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

[Intel-gfx] [PATCH 0/2] Enable VLV to work in BIOS-less system.

[Chon Ming Lee]

In non PC system, such as IVI, may not use BIOS, instead it uses boot 
loader with only minimal system initialization.  Most of the time, boot 
loader doesn't come with VBIOS, and depends on device driver to fully 
initialize the display controller and GPU.

For Valleyview, without VBIOS, i915 fails to work.  The patch add some 
missing init code, such as doing a DPIO CMNRESET and program the GMBUS 
frequency. 

Chon Ming Lee (2):
  drm/i915: Send a DPIO cmnreset during driver load or system resume.
  drm/i915: Program GMBUS Frequency based on the CDCLK

 drivers/gpu/drm/i915/i915_reg.h |8 ++
 drivers/gpu/drm/i915/intel_i2c.c|   43 +++
 drivers/gpu/drm/i915/intel_uncore.c |   15 
 3 files changed, 66 insertions(+), 0 deletions(-)

-- 
1.7.7.6

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

[Intel-gfx] [PATCH 1/2] drm/i915: Send a DPIO cmnreset during driver load or system resume.

[Chon Ming Lee]

Without the DPIO cmnreset, the PLL fail to lock.  This should have
done by BIOS.

v2: Move this to intel_uncore_sanitize to allow it to get call during
resume path. (Daniel)

Signed-off-by: Chon Ming Lee 
---
 drivers/gpu/drm/i915/intel_uncore.c |   15 +++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c 
b/drivers/gpu/drm/i915/intel_uncore.c
index 8649f1c..b1f53f3 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -276,10 +276,25 @@ static void intel_uncore_forcewake_reset(struct 
drm_device *dev)
 
 void intel_uncore_sanitize(struct drm_device *dev)
 {
+   struct drm_i915_private *dev_priv = dev->dev_private;
+   u32 reg_val;
+
intel_uncore_forcewake_reset(dev);
 
/* BIOS often leaves RC6 enabled, but disable it for hw init */
intel_disable_gt_powersave(dev);
+
+   /* Trigger DPIO CMN RESET, require especially in BIOS less
+* system
+*/
+   if (IS_VALLEYVIEW(dev)) {
+   reg_val = I915_READ(DPIO_CTL);
+   if (!(reg_val & 0x1)) {
+   I915_WRITE(DPIO_CTL, 0x0);
+   I915_WRITE(DPIO_CTL, 0x1);
+   POSTING_READ(DPIO_CTL);
+   }
+   }
 }
 
 /*
-- 
1.7.7.6

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

[Intel-gfx] [PATCH 2/2] drm/i915: Program GMBUS Frequency based on the CDCLK

[Chon Ming Lee]

CDCLK is used to generate the gmbus clock.  This is normally done by
BIOS. This is only for valleyview platform.

v2: Move this to intel_i2c_reset to allow reprogram the gmbus frequency
during resume. (Daniel)

Signed-off-by: Chon Ming Lee 
---
 drivers/gpu/drm/i915/i915_reg.h  |8 +++
 drivers/gpu/drm/i915/intel_i2c.c |   43 ++
 2 files changed, 51 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index bcee89b..8ddf58a 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -382,6 +382,8 @@
 #define   FB_FMAX_VMIN_FREQ_LO_MASK0xf800
 
 /* vlv2 north clock has */
+#define CCK_FUSE_REG   0x8
+#define  CCK_FUSE_HPLL_FREQ_MASK   0x3
 #define CCK_REG_DSI_PLL_FUSE   0x44
 #define CCK_REG_DSI_PLL_CONTROL0x48
 #define  DSI_PLL_VCO_EN(1 << 31)
@@ -1424,6 +1426,12 @@
 
 #define MI_ARB_VLV (VLV_DISPLAY_BASE + 0x6504)
 
+#define CZCLK_CDCLK_FREQ_RATIO (dev_priv->info->display_mmio_offset + 0x6508)
+#define   CDCLK_FREQ_SHIFT 4
+#define   CDCLK_FREQ_MASK  0x1f
+#define   CZCLK_FREQ_MASK  0xf
+#define GMBUS_FREQ (dev_priv->info->display_mmio_offset + 0x6510)
+
 /*
  * Palette regs
  */
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index d1c1e0f7..a8c4165 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -58,10 +58,53 @@ to_intel_gmbus(struct i2c_adapter *i2c)
return container_of(i2c, struct intel_gmbus, adapter);
 }
 
+static void gmbus_set_freq(struct drm_i915_private *dev_priv)
+{
+   int cdclk_ratio[] = { 10, 15, 20, 25, 30, 0, 40, 45, 50, 0,
+   60, 0, 0, 75, 80, 0, 90, 0, 100, 0,
+   0, 0, 120, 0, 0, 0, 0, 0, 150, 0, 160 };
+   int vco_freq[] = { 800, 1600, 2000, 2400 };
+   int gmbus_freq = 0, cdclk, hpll_freq;
+   u32 reg_val;
+
+   BUG_ON(!IS_VALLEYVIEW(dev_priv->dev));
+
+   /* Obtain SKU information to determine the correct CDCLK */
+   mutex_lock(&dev_priv->dpio_lock);
+   reg_val = vlv_cck_read(dev_priv, CCK_FUSE_REG);
+   mutex_unlock(&dev_priv->dpio_lock);
+
+   hpll_freq = reg_val & CCK_FUSE_HPLL_FREQ_MASK;
+
+   /* Get the CDCLK frequency */
+   reg_val = I915_READ(CZCLK_CDCLK_FREQ_RATIO);
+
+   cdclk = ((reg_val >> CDCLK_FREQ_SHIFT) & CDCLK_FREQ_MASK) - 1;
+
+   /* To enable hotplug detect, the gmbus frequency need to set as
+* cdclk/1.01
+*/
+   if (cdclk_ratio[cdclk])
+   gmbus_freq = vco_freq[hpll_freq] / cdclk_ratio[cdclk] * 101 / 
10;
+
+   WARN_ON(gmbus_freq == 0);
+
+   if (gmbus_freq != 0)
+   I915_WRITE(GMBUS_FREQ, gmbus_freq);
+
+}
+
 void
 intel_i2c_reset(struct drm_device *dev)
 {
struct drm_i915_private *dev_priv = dev->dev_private;
+
+   /* In BIOS-less system, program the correct gmbus frequency
+* before reading edid.
+*/
+   if (IS_VALLEYVIEW(dev))
+   gmbus_set_freq(dev_priv);
+
I915_WRITE(dev_priv->gpio_mmio_base + GMBUS0, 0);
I915_WRITE(dev_priv->gpio_mmio_base + GMBUS4, 0);
 }
-- 
1.7.7.6

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

Re: [Intel-gfx] [BUG] completely bonkers use of set_need_resched + VM_FAULT_NOPAGE

[Thomas Hellstrom]

On 09/12/2013 11:50 PM, Maarten Lankhorst wrote:

Op 12-09-13 18:44, Thomas Hellstrom schreef:

On 09/12/2013 05:45 PM, Maarten Lankhorst wrote:

Op 12-09-13 17:36, Daniel Vetter schreef:

On Thu, Sep 12, 2013 at 5:06 PM, Peter Zijlstra  wrote:

So I'm poking around the preemption code and stumbled upon:

drivers/gpu/drm/i915/i915_gem.c:set_need_resched();
drivers/gpu/drm/ttm/ttm_bo_vm.c:set_need_resched();
drivers/gpu/drm/ttm/ttm_bo_vm.c:set_need_resched();
drivers/gpu/drm/udl/udl_gem.c:  set_need_resched();

All these sites basically do:

while (!trylock())
  yield();

which is a horrible and broken locking pattern.

Firstly its deadlock prone, suppose the faulting process is a FIFOn+1
task that preempted the lock holder at FIFOn.

Secondly the implementation is worse than usual by abusing
VM_FAULT_NOPAGE, which is supposed to install a PTE so that the fault
doesn't retry, but you're using it as a get out of fault path. And
you're using set_need_resched() which is not something a driver should
_ever_ touch.

Now I'm going to take away set_need_resched() -- and while you can
'reimplement' it using set_thread_flag() you're not going to do that
because it will be broken due to changes to the preempt code.

So please as to fix ASAP and don't allow anybody to trick you into
merging silly things like that again ;-)

The set_need_resched in i915_gem.c:i915_gem_fault can actually be
removed. It was there to give the error handler a chance to sneak in
and reset the hw/sw tracking when the gpu is dead. That hack goes back
to the days when the locking around our error handler was somewhere
between nonexistent and totally broken, nowadays we keep things from
live-locking by a bit of magic in i915_mutex_lock_interruptible. I'll
whip up a patch to rip this out. I'll also check that our testsuite
properly exercises this path (needs a bit of work on a quick look for
better coverage).

The one in ttm is just bonghits to shut up lockdep: ttm can recurse
into it's own pagefault handler and then deadlock, the trylock just
keeps lockdep quiet. We've had that bug arise in drm/i915 due to some
fun userspace did and now have testcases for them. The right solution
to fix this is to use copy_to|from_user_atomic in ttm everywhere it
holds locks and have slowpaths which drops locks, copies stuff into a
temp allocation and then continues. At least that's how we've fixed
all those inversions in i915-gem. I'm not volunteering to fix this ;-)

Ah the case where a mmap'd address is passed to the execbuf ioctl? :P

Fine I'll look into it a bit, hopefully before tuesday. Else it might take a 
bit longer since I'll be on my way to plumbers..

I think a possible fix would be if fault() were allowed to return an error and 
drop the mmap_sem() before returning.

Otherwise we need to track down all copy_to_user / copy_from_user which happen 
with bo::reserve held.


Actually, from looking at the mm code, it seems OK to do the following:

if (!bo_tryreserve()) {
up_read mmap_sem(); // Release the mmap_sem to avoid deadlocks.
bo_reserve();   // Wait for the BO to become available 
(interruptible)
bo_unreserve();   // Where is bo_wait_unreserved() when we 
need it, Maarten :P
return VM_FAULT_RETRY; // Go ahead and retry the VMA walk, after 
regrabbing

}

Somebody conveniently added a VM_FAULT_RETRY, but for a different purpose.

If possible, I suggest to take this route for now to avoid the mess of 
changing locking order in all TTM drivers, with
all give-up-locking slowpaths that comes with it. IIRC it took some time 
for i915 to get that right, and completely get rid of all lockdep warnings.


This will keep the official locking order
bo::reserve
mmap_sem

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

[Intel-gfx] [QA] Testing report for `drm-intel-testing` (was: Updated -next) on ww37

[Yang, Guang A]

Summary
We covered the platform: Baytrail-M, Haswell mobile, HSW desktop, HSW ULT, 
IvyBridge, SandyBridge, IronLake. in this circle, 10 new bugs are filed, 31 
bugs are still opened, no WONTFIX bugs, no NOTABUG bugs, no NOTOURBUG bugs, no 
Duplicated bugs, no REOPENED bugs, 2 bugs are fixed, 12 bugs are closed.
In this round, we focus on Baytrail-M and file some bugs.



Test Environment

Kernel: (drm-intel-testing)9c016d8172eb290af7c9be4ca3d0d6885055f73e

Some additional commit info:

Merge: 4b16afd 86a7e12

Author: Daniel Vetter 

Date:   Fri Sep 6 10:50:21 2013 +0200



Finding

New Bugs:   10 bugs

Bug 69301 - 
[SNB/IVB/HSW]igt/gem_persistent_relocs/forked-thrashing aborted

Bug 69300 - [IVB HDMI]HDMI 
can't light up

Bug 69250 - [Baytrail-M] 
eDP responsiveness issues of slow i915 module init , slow resume time and slow 
suspend time

Bug 69248 - [Baytrail-M] 
Boot up with "[drm:intel_pipe_config_compare] *ERROR* mismatch in clock" and 
"Call Trace"

Bug 69247 - 
[ILK/SNB]igt/gem_evict_everything/forked-swapping-multifd-mempressure-normal 
causes OOM killer

Bug 69166 - [Baytrail-M] 
Monitor can't light up after resuming from S3

Bug 69161 - 
igt/kms_flip/2x-flip-vs-absolute-wf_vblank fail with 2-pipe

Bug 69130 - 
[Baytrail-M]I-G-T/kms_flip subtest：'flip_rcs-flip-vs-panning' fail

Bug 69128 - [IVB Apple 
MacBook pro] miniDP-to-DP cause machine boot up with "ERROR"

Bug 69012 - 
[Bisected]igt/kms_flip/flip-vs-absolute-wf_vblank fails





Opened Bugs:   31 bugs

Bug 68643 - [SNB DP]Some 
modes can't light up

Bug 68640 - [HSW 
nv]igt/prime_nv_pcopy/test3_1 timeout with debug kernel

Bug 68638 - [HSW 
nv]igt/prime_nv_test/i915_import_cpu_mmap fails with debug kernel

Bug 68004 - 
igt/prime_self_import/with_one_bo_two_files aborted

Bug 67891 - 
igt/prime_self_import/export-vs-gem_close-race fail and causes call trace

Bug 67889 - [Baytrail-M] 
Blackscreen occurred after running xrandr setting twice

Bug 67813 - [HSW 
bisected]igt/module_reload causes [drm:hsw_unclaimed_reg_check] *ERROR* 
Unclaimed write to 44004 and system hang with headless, with power well disabled

Bug 67732 - [Baytrail-M] 
eDP and VGA can't light up simultaneously

Bug 67462 - [SNB] Call 
trace appears after system boots with DP

Bug 67345 - [BayTrail-M] 
[drm:intel_pipe_config_compare] *ERROR* mismatch in clock (expected 146250, 
found 0)

Bug 67287 - 
igt/gem_flink_race fails

Bug 67243 - 
[ILK]igt/kms_render/gpu-blit randomly causes system hang

Bug 67030 - [HSW] no modes 
bigger than 1080p in the parsed EDID for 4k TV on both HDMI/VGA

Bug 67027 - [HSW] some 
modes oversan, on a 4K HDMI TV

Bug 66301 - [HSW mobile] 
resume from s4 sporadically causes call trace and machine is reachable

Bug 65995 - [HSW mobile] 
eDP can't light up when boot up

Bug 65927 - [HSW] crash 
when vga output set to mirror mode

Bug 65700 - 
[SNB]<3>[drm:ironlake_disable_pch_transcoder] *ERROR* failed to disable 
transcoder 0

Bug 65496 - [HSW] 
Probabilistic resume from s4 causes call trace and system hang, with warm boot

Bug 64415 - [ILK] Some 
modes unable to light up on DP display

Bug 64379 - [HSW desktop 
bisected] ddi pll tracking botch-up due to vt-switchless resume

Bug 63981 - MacBook Pro 
retina 13 inch early 2013 jittery display

Bug 61041 - 
[Pineview]I-