[PATCH] kernel/cpu: Distinct name for cpu_hotplug.dep_map

[Joonas Lahtinen]

Use distinctive name for cpu_hotplug.dep_map to avoid the actual
cpu_hotplug.lock appearing as cpu_hotplug.lock#2 in lockdep splats.

Cc: Ingo Molnar <mi...@kernel.org>
Cc: Peter Zijlstra <pet...@infradead.org>
Cc: Gautham R. Shenoy <e...@linux.vnet.ibm.com>
Cc: intel-...@lists.freedesktop.org
Cc: triv...@kernel.org
Acked-by: Gautham R. Shenoy <e...@linux.vnet.ibm.com>
Signed-off-by: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
---
This time CC'ing triv...@kernel.org too in the hopes of finally getting this in.
---
 kernel/cpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/cpu.c b/kernel/cpu.c
index d948e44..d74199d 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -155,7 +155,7 @@ static struct {
.wq = __WAIT_QUEUE_HEAD_INITIALIZER(cpu_hotplug.wq),
.lock = __MUTEX_INITIALIZER(cpu_hotplug.lock),
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
-   .dep_map = {.name = "cpu_hotplug.lock" },
+   .dep_map = STATIC_LOCKDEP_MAP_INIT("cpu_hotplug.dep_map", 
_hotplug.dep_map),
 #endif
 };
 
-- 
2.5.5

Re: [PATCH v3 2/2] iommu: Remove cpu-local spinlock

[Joonas Lahtinen]

On ke, 2016-06-01 at 12:10 +0100, Chris Wilson wrote:
> By avoiding cross-CPU usage of the per-cpu iova cache, we can forgo
> having a spinlock inside the per-cpu struct. The only place where we
> actually may touch another CPU's data is when performing a cache flush
> after running out of memory. Here, we can instead schedule a task to run
> on the other CPU to do the flush before trying again.
> 
> Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
> Cc: Joerg Roedel <j...@8bytes.org>
> Cc: io...@lists.linux-foundation.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/iommu/iova.c | 29 ++---
>  1 file changed, 6 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
> index e23001bfcfee..36cdc8eeab1c 100644
> --- a/drivers/iommu/iova.c
> +++ b/drivers/iommu/iova.c
> @@ -390,6 +390,11 @@ free_iova(struct iova_domain *iovad, unsigned long pfn)
>  }
>  EXPORT_SYMBOL_GPL(free_iova);
>  
> +static void free_this_cached_iovas(void *info)
> +{
> + free_cpu_cached_iovas(smp_processor_id(), info);
> +}
> +
>  /**
>   * alloc_iova_fast - allocates an iova from rcache
>   * @iovad: - iova domain in question
> @@ -413,17 +418,12 @@ alloc_iova_fast(struct iova_domain *iovad, unsigned 
> long size,
>  retry:
>   new_iova = alloc_iova(iovad, size, limit_pfn, true);
>   if (!new_iova) {
> - unsigned int cpu;
> -
>   if (flushed_rcache)
>   return 0;
>  
>   /* Try replenishing IOVAs by flushing rcache. */
>   flushed_rcache = true;
> - preempt_disable();
> - for_each_online_cpu(cpu)
> - free_cpu_cached_iovas(cpu, iovad);
> - preempt_enable();
> + on_each_cpu(free_this_cached_iovas, iovad, true);

This is not on a hot path, so should be worthy change.

Reviewed-by: Joonas Lahtinen <joonas.lahti...@linux.intel.com>

Regards, Joonas

>   goto retry;
>   }
>  
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: [PATCH v3 1/2] iommu: Disable preemption around use of this_cpu_ptr()

[Joonas Lahtinen]

On ke, 2016-06-01 at 12:10 +0100, Chris Wilson wrote:
> Between acquiring the this_cpu_ptr() and using it, ideally we don't want
> to be preempted and work on another CPU's private data. this_cpu_ptr()
> checks whether or not preemption is disable, and get_cpu_ptr() provides
> a convenient wrapper for operating on the cpu ptr inside a preemption
> disabled critical section (which currently is provided by the
> spinlock). Indeed if we disable preemption around this_cpu_ptr,
> we do not need the CPU local spinlock - so long as take care that no other
> CPU is running that code as do perform the cross-CPU cache flushing and
> teardown, but that is a subject for another patch.
> 
> [  167.997877] BUG: using smp_processor_id() in preemptible [] code: 
> usb-storage/216
> [  167.997940] caller is debug_smp_processor_id+0x17/0x20
> [  167.997945] CPU: 7 PID: 216 Comm: usb-storage Tainted: G U  
> 4.7.0-rc1-gfxbench-RO_Patchwork_1057+ #1
> [  167.997948] Hardware name: Hewlett-Packard HP Pro 3500 Series/2ABF, BIOS 
> 8.11 10/24/2012
> [  167.997951]   880118b7f9c8 8140dca5 
> 0007
> [  167.997958]  81a3a7e9 880118b7f9f8 8142a927 
> 
> [  167.997965]  8800d499ed58 0001 000f 
> 880118b7fa08
> [  167.997971] Call Trace:
> [  167.997977]  [] dump_stack+0x67/0x92
> [  167.997981]  [] check_preemption_disabled+0xd7/0xe0
> [  167.997985]  [] debug_smp_processor_id+0x17/0x20
> [  167.997990]  [] alloc_iova_fast+0xb7/0x210
> [  167.997994]  [] intel_alloc_iova+0x7f/0xd0
> [  167.997998]  [] intel_map_sg+0xbd/0x240
> [  167.998002]  [] ? debug_lockdep_rcu_enabled+0x1d/0x20
> [  167.998009]  [] usb_hcd_map_urb_for_dma+0x4b9/0x5a0
> [  167.998013]  [] usb_hcd_submit_urb+0xe9/0xaa0
> [  167.998017]  [] ? mark_held_locks+0x6f/0xa0
> [  167.998022]  [] ? __raw_spin_lock_init+0x1c/0x50
> [  167.998025]  [] ? debug_lockdep_rcu_enabled+0x1d/0x20
> [  167.998028]  [] usb_submit_urb+0x3f3/0x5a0
> [  167.998032]  [] ? trace_hardirqs_on_caller+0x122/0x1b0
> [  167.998035]  [] usb_sg_wait+0x67/0x150
> [  167.998039]  [] usb_stor_bulk_transfer_sglist.part.3+0x82/0xd0
> [  167.998042]  [] usb_stor_bulk_srb+0x4c/0x60
> [  167.998045]  [] usb_stor_Bulk_transport+0x17e/0x420
> [  167.998049]  [] usb_stor_invoke_transport+0x242/0x540
> [  167.998052]  [] ? debug_lockdep_rcu_enabled+0x1d/0x20
> [  167.998058]  [] usb_stor_transparent_scsi_command+0x9/0x10
> [  167.998061]  [] usb_stor_control_thread+0x158/0x260
> [  167.998064]  [] ? fill_inquiry_response+0x20/0x20
> [  167.998067]  [] ? fill_inquiry_response+0x20/0x20
> [  167.998071]  [] kthread+0xea/0x100
> [  167.998078]  [] ret_from_fork+0x1f/0x40
> [  167.998081]  [] ? kthread_create_on_node+0x1f0/0x1f0
> 
> v2: convert preempt_disable(); var = this_cpu_ptr() to var = get_cpu_ptr()
> v3: Actually use get_cpu_ptr (not get_cpu_var). Drop the spinlock
> removal, concentrate on the immediate bug fix.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96293
> Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahti...@linux.intel.com>

Reviewed-by: Joonas Lahtinen <joonas.lahti...@linux.intel.com>

> Cc: Joerg Roedel <j...@8bytes.org>
> Cc: io...@lists.linux-foundation.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/iommu/iova.c | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
> index ba764a0835d3..e23001bfcfee 100644
> --- a/drivers/iommu/iova.c
> +++ b/drivers/iommu/iova.c
> @@ -420,8 +420,10 @@ retry:
>  
>   /* Try replenishing IOVAs by flushing rcache. */
>   flushed_rcache = true;
> + preempt_disable();
>   for_each_online_cpu(cpu)
>   free_cpu_cached_iovas(cpu, iovad);
> + preempt_enable();
>   goto retry;
>   }
>  
> @@ -749,7 +751,7 @@ static bool __iova_rcache_insert(struct iova_domain 
> *iovad,
>   bool can_insert = false;
>   unsigned long flags;
>  
> - cpu_rcache = this_cpu_ptr(rcache->cpu_rcaches);
> + cpu_rcache = get_cpu_ptr(rcache->cpu_rcaches);
>   spin_lock_irqsave(_rcache->lock, flags);
>  
>   if (!iova_magazine_full(cpu_rcache->loaded)) {
> @@ -779,6 +781,7 @@ static bool __iova_rcache_insert(struct iova_domain 
> *iovad,
>   iova_magazine_push(cpu_rcache->loaded, iova_pfn);
>  
>   spin_unlock_irqrestore(_rcache->lock, flags);
> + put_cpu_ptr(rcache->cpu_rcaches);
>  
>   if (mag_to_free) {
>   iova_magazin

[PATCH] kernel/cpu: Distinctive name for cpu_hotplug.dep_map

[Joonas Lahtinen]

Use distinctive name for cpu_hotplug.dep_map to avoid the actual
cpu_hotplug.lock appearing as cpu_hotplug.lock#2 in lockdep splats.

Cc: Gautham R. Shenoy <e...@linux.vnet.ibm.com>
Cc: Rafael J. Wysocki <rafael.j.wyso...@intel.com>
Cc: Intel graphics driver community testing & development 
<intel-...@lists.freedesktop.org>
Signed-off-by: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
---
 kernel/cpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/cpu.c b/kernel/cpu.c
index 5b9d396..6a13f24 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -78,7 +78,7 @@ static struct {
.wq = __WAIT_QUEUE_HEAD_INITIALIZER(cpu_hotplug.wq),
.lock = __MUTEX_INITIALIZER(cpu_hotplug.lock),
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
-   .dep_map = {.name = "cpu_hotplug.lock" },
+   .dep_map = STATIC_LOCKDEP_MAP_INIT("cpu_hotplug.dep_map", 
_hotplug.dep_map),
 #endif
 };
 
-- 
2.5.0

Re: [PATCH] [RFC] kernel/cpu: Use lockref for online CPU reference counting

[Joonas Lahtinen]

On ti, 2016-02-16 at 12:07 +0100, Peter Zijlstra wrote:
> On Tue, Feb 16, 2016 at 12:51:03PM +0200, Joonas Lahtinen wrote:
> > Quoting my original patch;
> > 
> > "See the Bugzilla link for more details.
> 
> If its not in the Changelog it doesn't exist. Patches should be self
> contained and not refer to external sources for critical information.

The exact locking case in CPUfreq drivers causing a splat is described
in the patch. Details were already included, that's why term "more
details" was used.

This is not exactly taking us closer to a fix, so I'd rather like to
discuss about the possibility of adding one more member to task_struct.
I added Oleg as CC to catch his attention to re-evaluate based on the
previous discussion in this thread.

Regards, Joonas

-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: [Intel-gfx] [PATCH] [RFC] kernel/cpu: Use lockref for online CPU reference counting

[Joonas Lahtinen]

On ke, 2016-02-17 at 17:37 +0100, Peter Zijlstra wrote:
> On Wed, Feb 17, 2016 at 05:33:51PM +0100, Daniel Vetter wrote:
> > On Wed, Feb 17, 2016 at 05:14:57PM +0100, Peter Zijlstra wrote:
> > > On Wed, Feb 17, 2016 at 05:13:21PM +0100, Daniel Vetter wrote:
> > > > And for context we're hitting this on CI in a bunch of our machines, 
> > > > which
> > > 
> > > What's CI ?
> > 
> > Continuous integration, aka our own farm of machines dedicated to running
> > i915.ko testcases. Kinda like 0day (it does pre-merge testing on the m-l
> > and also post-merge on our own little integration tree), but for just the
> > graphics team and our needs.
> 
> So what patch triggered this new issue? Did cpufreq change or what?

It appeared right after enabling lockdep debugging on the continuous
integration system. So we do not have a history of it not being there.

Taking an another look at my code, it could indeed end up in double-
wait-looping scenario if suspend and initialization was performed
simultaneously (it had a couple of other bugs too, fixed in v2).
Strange thing is, I think that should have been caught by cpuhp_lock_*
lockdep tracking.

So I'll move the discussion to linux-pm list to change the CPUfreq code. Thanks 
for the comments.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: [PATCH] [RFC] kernel/cpu: Use lockref for online CPU reference counting

[Joonas Lahtinen]

Hi,

On ma, 2016-02-15 at 18:06 +0100, Peter Zijlstra wrote:
> On Mon, Feb 15, 2016 at 03:17:55PM +0100, Peter Zijlstra wrote:
> > On Mon, Feb 15, 2016 at 02:36:43PM +0200, Joonas Lahtinen wrote:
> > > Instead of implementing a custom locked reference counting, use lockref.
> > > 
> > > Current implementation leads to a deadlock splat on Intel SKL platforms
> > > when lockdep debugging is enabled.
> > > 
> > > This is due to few of CPUfreq drivers (including Intel P-state) having 
> > > this;
> > > policy->rwsem is locked during driver initialization and the functions 
> > > called
> > > during init that actually apply CPU limits use get_online_cpus (because 
> > > they
> > > have other calling paths too), which will briefly lock cpu_hotplug.lock to
> > > increase cpu_hotplug.refcount.
> > > 
> > > On later calling path, when doing a suspend, when cpu_hotplug_begin() is 
> > > called
> > > in disable_nonboot_cpus(), callbacks to CPUfreq functions get called 
> > > after,
> > > which will lock policy->rwsem and cpu_hotplug.lock is already held by
> > > cpu_hotplug_begin() and we do have a potential deadlock scenario reported 
> > > by
> > > our CI system (though it is a very unlikely one). See the Bugzilla link 
> > > for more
> > > details.
> > 
> > I've been meaning to change the thing into a percpu-rwsem, I just
> > haven't had time to look into the lockdep splat that generated.
> 
> 
> The below has plenty lockdep issues because percpu-rwsem is
> reader-writer fair (like the regular rwsem), so it does throw up a fair
> number of very icky issues.
> 
> If at all possible, I'd really rather fix those and have a 'saner'
> hotplug lock, rather than muddle on with open-coded horror lock we have
> now.
> 

I do still agree the below would be a worthy change to proceed with.

CC'd Oleg here too to give a comment.

Regards, Joonas

> 
> --- a/include/linux/cpu.h
> +++ b/include/linux/cpu.h
> @@ -225,6 +225,8 @@ extern struct bus_type cpu_subsys;
>  #ifdef CONFIG_HOTPLUG_CPU
>  /* Stop CPUs going up and down. */
>  
> +extern void cpu_hotplug_init_task(struct task_struct *p);
> +
>  extern void cpu_hotplug_begin(void);
>  extern void cpu_hotplug_done(void);
>  extern void get_online_cpus(void);
> @@ -242,6 +244,8 @@ int cpu_down(unsigned int cpu);
>  
>  #else/* CONFIG_HOTPLUG_CPU */
>  
> +static inline void cpu_hotplug_init_task(struct task_struct *p) {}
> +
>  static inline void cpu_hotplug_begin(void) {}
>  static inline void cpu_hotplug_done(void) {}
>  #define get_online_cpus()do { } while (0)
> --- a/include/linux/percpu-rwsem.h
> +++ b/include/linux/percpu-rwsem.h
> @@ -16,6 +16,15 @@ struct percpu_rw_semaphore {
>   wait_queue_head_t   write_waitq;
>  };
>  
> +#define DEFINE_STATIC_PERCPU_RWSEM(name) \
> +static DEFINE_PER_CPU(unsigned int, __percpu_rwsem_frc_##name);  \
> +static struct percpu_rw_semaphore name = {   \
> + .rss = __RCU_SYNC_INITIALIZER(name.rss, RCU_SCHED_SYNC),\
> + .fast_read_ctr = &__percpu_rwsem_frc_##name,\
> + .rw_sem = __RWSEM_INITIALIZER(name.rw_sem), \
> + .write_waitq = __WAIT_QUEUE_HEAD_INITIALIZER(name.write_waitq), \
> +}
> +
>  extern void percpu_down_read(struct percpu_rw_semaphore *);
>  extern int  percpu_down_read_trylock(struct percpu_rw_semaphore *);
>  extern void percpu_up_read(struct percpu_rw_semaphore *);
> @@ -33,9 +42,11 @@ extern void percpu_free_rwsem(struct per
>   __percpu_init_rwsem(brw, #brw, _key); \
>  })
>  
> -
>  #define percpu_rwsem_is_held(sem) lockdep_is_held(&(sem)->rw_sem)
>  
> +#define percpu_rwsem_assert_held(sem)  \
> + lockdep_assert_held(&(sem)->rw_sem)
> +
>  static inline void percpu_rwsem_release(struct percpu_rw_semaphore *sem,
>   bool read, unsigned long ip)
>  {
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1403,6 +1403,9 @@ struct task_struct {
>   struct task_struct *last_wakee;
>  
>   int wake_cpu;
> +#ifdef CONFIG_HOTPLUG_CPU
> + int cpuhp_ref;
> +#endif
>  #endif
>   int on_rq;
>  
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -23,6 +23,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include "smpboot.h"
>  
> @@ -51,121 +52,52 @@ EXPORT_SYMBOL(cpu_notifier_register_done
>  
>  static RAW_NOTIFIER_HEAD(cpu_chain);
&

Re: [PATCH] kernel/cpu: Distinctive name for cpu_hotplug.dep_map

[Joonas Lahtinen]

Hi,

According to scripts/get_maintainer.pl Ingo or Peter would be more
appropriate to merge.

Added them as To:

On ke, 2016-02-03 at 22:42 +0530, Gautham R Shenoy wrote:
> Hello Joonas,
> 
> On Wed, Feb 03, 2016 at 04:24:28PM +0200, Joonas Lahtinen wrote:
> > Use distinctive name for cpu_hotplug.dep_map to avoid the actual
> > cpu_hotplug.lock appearing as cpu_hotplug.lock#2 in lockdep splats.
> > 
> > Cc: Gautham R. Shenoy <e...@linux.vnet.ibm.com>
> > Cc: Rafael J. Wysocki <rafael.j.wyso...@intel.com>
> > Cc: Intel graphics driver community testing & development  > x...@lists.freedesktop.org>
> > Signed-off-by: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
> > ---
> >  kernel/cpu.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/kernel/cpu.c b/kernel/cpu.c
> > index 5b9d396..6a13f24 100644
> > --- a/kernel/cpu.c
> > +++ b/kernel/cpu.c
> > @@ -78,7 +78,7 @@ static struct {
> >     .wq = __WAIT_QUEUE_HEAD_INITIALIZER(cpu_hotplug.wq),
> >     .lock = __MUTEX_INITIALIZER(cpu_hotplug.lock),
> >  #ifdef CONFIG_DEBUG_LOCK_ALLOC
> > -   .dep_map = {.name = "cpu_hotplug.lock" },
> > +   .dep_map = STATIC_LOCKDEP_MAP_INIT("cpu_hotplug.dep_map",
> > _hotplug.dep_map),
> 
> Looks good to me!
> Acked-by: Gautham R. Shenoy <e...@linux.vnet.ibm.com>
> 

Thanks!

Regards, Joonas

> --
> Thanks and Regards
> gautham.
> 
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

[PATCH] [RFC] kernel/cpu: Use lockref for online CPU reference counting

[Joonas Lahtinen]

Instead of implementing a custom locked reference counting, use lockref.

Current implementation leads to a deadlock splat on Intel SKL platforms
when lockdep debugging is enabled.

This is due to few of CPUfreq drivers (including Intel P-state) having this;
policy->rwsem is locked during driver initialization and the functions called
during init that actually apply CPU limits use get_online_cpus (because they
have other calling paths too), which will briefly lock cpu_hotplug.lock to
increase cpu_hotplug.refcount.

On later calling path, when doing a suspend, when cpu_hotplug_begin() is called
in disable_nonboot_cpus(), callbacks to CPUfreq functions get called after,
which will lock policy->rwsem and cpu_hotplug.lock is already held by
cpu_hotplug_begin() and we do have a potential deadlock scenario reported by
our CI system (though it is a very unlikely one). See the Bugzilla link for more
details.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93294
Cc: Linux kernel development <linux-kernel@vger.kernel.org>
Cc: Ingo Molnar <mi...@kernel.org>
Cc: Peter Zijlstra <pet...@infradead.org>
Cc: David Hildenbrand <d...@linux.vnet.ibm.com>
Cc: Paul E. McKenney <paul...@linux.vnet.ibm.com>
Cc: Gautham R. Shenoy <e...@linux.vnet.ibm.com>
Cc: Chris Wilson <ch...@chris-wilson.co.uk>
Signed-off-by: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
---
 kernel/cpu.c | 87 +---
 1 file changed, 54 insertions(+), 33 deletions(-)

diff --git a/kernel/cpu.c b/kernel/cpu.c
index 5b9d396..8acec83 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -17,6 +17,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -62,13 +63,10 @@ static struct {
struct task_struct *active_writer;
/* wait queue to wake up the active_writer */
wait_queue_head_t wq;
-   /* verifies that no writer will get active while readers are active */
-   struct mutex lock;
-   /*
-* Also blocks the new readers during
-* an ongoing cpu hotplug operation.
-*/
-   atomic_t refcount;
+   /* wait queue to wake up readers */
+   wait_queue_head_t reader_wq;
+   /* track online CPU users */
+   struct lockref lockref;
 
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
struct lockdep_map dep_map;
@@ -76,7 +74,7 @@ static struct {
 } cpu_hotplug = {
.active_writer = NULL,
.wq = __WAIT_QUEUE_HEAD_INITIALIZER(cpu_hotplug.wq),
-   .lock = __MUTEX_INITIALIZER(cpu_hotplug.lock),
+   .reader_wq = __WAIT_QUEUE_HEAD_INITIALIZER(cpu_hotplug.reader_wq),
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
.dep_map = {.name = "cpu_hotplug.lock" },
 #endif
@@ -92,52 +90,64 @@ static struct {
 
 void get_online_cpus(void)
 {
-   might_sleep();
+   DEFINE_WAIT(wait);
+
if (cpu_hotplug.active_writer == current)
return;
+
cpuhp_lock_acquire_read();
-   mutex_lock(_hotplug.lock);
-   atomic_inc(_hotplug.refcount);
-   mutex_unlock(_hotplug.lock);
+
+   /* First to get might have to wait over a hotplug operation. */
+   while (!lockref_get_or_lock(_hotplug.lockref)) {
+   if (cpu_hotplug.lockref.count == 0) {
+   cpu_hotplug.lockref.count++;
+   spin_unlock(_hotplug.lockref.lock);
+   break;
+   }
+   spin_unlock(_hotplug.lockref.lock);
+
+   prepare_to_wait(_hotplug.reader_wq, , 
TASK_UNINTERRUPTIBLE);
+   schedule();
+   finish_wait(_hotplug.reader_wq, );
+   }
 }
 EXPORT_SYMBOL_GPL(get_online_cpus);
 
 void put_online_cpus(void)
 {
-   int refcount;
-
if (cpu_hotplug.active_writer == current)
return;
 
-   refcount = atomic_dec_return(_hotplug.refcount);
-   if (WARN_ON(refcount < 0)) /* try to fix things up */
-   atomic_inc(_hotplug.refcount);
+   /* Last to release might have to wake queued hotplug operation. */
+   if (!lockref_put_or_lock(_hotplug.lockref)) {
+   WARN_ON(cpu_hotplug.lockref.count <= 0);
+   cpu_hotplug.lockref.count = 0;
+   spin_unlock(_hotplug.lockref.lock);
 
-   if (refcount <= 0 && waitqueue_active(_hotplug.wq))
-   wake_up(_hotplug.wq);
+   if (waitqueue_active(_hotplug.wq))
+   wake_up(_hotplug.wq);
+   }
 
cpuhp_lock_release();
-
 }
 EXPORT_SYMBOL_GPL(put_online_cpus);
 
 /*
  * This ensures that the hotplug operation can begin only when the
- * refcount goes to zero.
+ * cpu_hotplug.lockref goes to zero.
  *
  * Note that during a cpu-hotplug operation, the new readers, if any,
- * will be blocked by the cpu_hotplug.lock
+ * will be blocked by the cpu_hotplug.lockref being dead.
  *
  * Since cpu_hotplug_begin() is always called after invoking
  * cpu

Re: [PATCH] [RFC] kernel/cpu: Use lockref for online CPU reference counting

[Joonas Lahtinen]

Hi,

On ma, 2016-02-15 at 18:06 +0100, Peter Zijlstra wrote:
> On Mon, Feb 15, 2016 at 03:17:55PM +0100, Peter Zijlstra wrote:
> > On Mon, Feb 15, 2016 at 02:36:43PM +0200, Joonas Lahtinen wrote:
> > > Instead of implementing a custom locked reference counting, use lockref.
> > > 
> > > Current implementation leads to a deadlock splat on Intel SKL platforms
> > > when lockdep debugging is enabled.
> > > 
> > > This is due to few of CPUfreq drivers (including Intel P-state) having 
> > > this;
> > > policy->rwsem is locked during driver initialization and the functions 
> > > called
> > > during init that actually apply CPU limits use get_online_cpus (because 
> > > they
> > > have other calling paths too), which will briefly lock cpu_hotplug.lock to
> > > increase cpu_hotplug.refcount.
> > > 
> > > On later calling path, when doing a suspend, when cpu_hotplug_begin() is 
> > > called
> > > in disable_nonboot_cpus(), callbacks to CPUfreq functions get called 
> > > after,
> > > which will lock policy->rwsem and cpu_hotplug.lock is already held by
> > > cpu_hotplug_begin() and we do have a potential deadlock scenario reported 
> > > by
> > > our CI system (though it is a very unlikely one). See the Bugzilla link 
> > > for more
> > > details.
> > 
> > I've been meaning to change the thing into a percpu-rwsem, I just
> > haven't had time to look into the lockdep splat that generated.
> 
> 
> The below has plenty lockdep issues because percpu-rwsem is
> reader-writer fair (like the regular rwsem), so it does throw up a fair
> number of very icky issues.
> 

I originally thought of implementing this more similar to what you
specify, but then I came across a discussion in the mailing list where
it was NAKed adding more members to task_struct;

http://comments.gmane.org/gmane.linux.kernel/970273

Adding proper recursion (the way my initial implementation was going)
got ugly without modifying task_struct because get_online_cpus() is a
speed critical code path.

So I'm all for fixing the current code in a different way if that will
then be merged.

Regards, Joonas

> If at all possible, I'd really rather fix those and have a 'saner'
> hotplug lock, rather than muddle on with open-coded horror lock we have
> now.
> 
> 



-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: [PATCH] [RFC] kernel/cpu: Use lockref for online CPU reference counting

[Joonas Lahtinen]

On ti, 2016-02-16 at 10:14 +0100, Peter Zijlstra wrote:
> On Tue, Feb 16, 2016 at 10:49:36AM +0200, Joonas Lahtinen wrote:
> > I originally thought of implementing this more similar to what you
> > specify, but then I came across a discussion in the mailing list where
> > it was NAKed adding more members to task_struct;
> > 
> > http://comments.gmane.org/gmane.linux.kernel/970273
> > 
> > Adding proper recursion (the way my initial implementation was going)
> > got ugly without modifying task_struct because get_online_cpus() is a
> > speed critical code path.
> 
> Yeah, just don't let Linus hear you say that. get_online_cpus() is _not_
> considered performance critical.

Oh well, at least changes to it added quite noticeably to the bootup
time of a system.

> 
> > So I'm all for fixing the current code in a different way if that will
> > then be merged.
> 
> So I'm not sure why you're poking at this horror show to begin with.
> ISTR you mentioning a lockdep splat for SKL, but failed to provide
> detail.
> 

Quoting my original patch;

"See the Bugzilla link for more details.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93294;

The improvement my patch implements is to use lockref for locked
reference counting (hotplug code previously rolled its own mutex +
atomic combo), which gets rid of the deadlock scenario described and
linked in the initial patch. Trace for the scenario;

https://bugs.freedesktop.org/attachment.cgi?id=121490

I think using lockref makes it substantially less special, lockref code
being a lot more battle-tested in the FS code than the previous
cpu_hotplug.lock mess.

> Making the hotplug lock _more_ special to fix that is just wrong. Fix
> the retarded locking that lead to it.
> 

I do agree that it's still not pretty, but now it does correctly what
the previous code was trying to do with custom mutex + atomic.

I'm all for fixing the code further, but prior to proceeding there
needs to be some sort of an agreement on either making
get_online_cpus() slower (which does not seem like a good idea) or
adding more members to task_struct.

Regards, Joonas

> 
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: [PATCH] intel-pstate: Update frequencies of policy->cpus only from ->set_policy()

[Joonas Lahtinen]

Hi,

This fixes the issue for my machine, we'll try in our CI system, too.

CC'd Daniel for that. By R-b and T-b below.

On ma, 2016-02-22 at 10:27 +0530, Viresh Kumar wrote:
> The intel-pstate driver is using intel_pstate_hwp_set() from two
> separate paths, i.e. ->set_policy() callback and sysfs update path for
> the files present in /sys/devices/system/cpu/intel_pstate/ directory.
> 
> While an update to the sysfs path applies to all the CPUs being managed
> by the driver (which essentially means all the online CPUs), the update
> via the ->set_policy() callback applies to a smaller group of CPUs
> managed by the policy for which ->set_policy() is called.
> 
> And so, intel_pstate_hwp_set() should update frequencies of only the
> CPUs that are part of policy->cpus mask, while it is called from
> ->set_policy() callback.
> 
> In order to do that, add a parameter (cpumask) to intel_pstate_hwp_set()
> and apply the frequency changes only to the concerned CPUs.
> 
> For ->set_policy() path, we are only concerned about policy->cpus, and
> so policy->rwsem lock taken by the core prior to calling ->set_policy()
> is enough to take care of any races. The larger lock acquired by
> get_online_cpus() is required only for the updates to sysfs files.
> 
> Add another routine, intel_pstate_hwp_set_online_cpus(), and call it
> from the sysfs update paths.
> 
> This also fixes a lockdep reported recently, where policy->rwsem and
> get_online_cpus() could have been acquired in any order causing an ABBA
> deadlock. The sequence of events leading to that was:
> 
> intel_pstate_init(...)
>   ...cpufreq_online(...)
>   down_write(>rwsem); // Locks policy->rwsem
>   ...
>   cpufreq_init_policy(policy);
>   ...intel_pstate_hwp_set();
>   get_online_cpus(); // Temporarily locks 
> cpu_hotplug.lock
>   ...
>   up_write(>rwsem);
> 
> pm_suspend(...)
>   ...disable_nonboot_cpus()
>   _cpu_down()
>   cpu_hotplug_begin(); // Locks cpu_hotplug.lock
>   __cpu_notify(CPU_DOWN_PREPARE, ...);
>   ...cpufreq_offline_prepare();
>   down_write(>rwsem); // Locks 
> policy->rwsem
> 
> Reported-by: Joonas Lahtinen <joonas.lahti...@linux.intel.com>

Tested-by: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
Reviewed-by: Joonas Lahtinen <joonas.lahti...@linux.intel.com>

> Signed-off-by: Viresh Kumar <viresh.ku...@linaro.org>
> ---
>  drivers/cpufreq/intel_pstate.c | 21 -
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> index f4d85c2ae7b1..2e7058a2479d 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -287,7 +287,7 @@ static inline void update_turbo_state(void)
>    cpu->pstate.max_pstate == cpu->pstate.turbo_pstate);
>  }
>  
> -static void intel_pstate_hwp_set(void)
> +static void intel_pstate_hwp_set(const struct cpumask *cpumask)
>  {
>   int min, hw_min, max, hw_max, cpu, range, adj_range;
>   u64 value, cap;
> @@ -297,9 +297,7 @@ static void intel_pstate_hwp_set(void)
>   hw_max = HWP_HIGHEST_PERF(cap);
>   range = hw_max - hw_min;
>  
> - get_online_cpus();
> -
> - for_each_online_cpu(cpu) {
> + for_each_cpu(cpu, cpumask) {
>   rdmsrl_on_cpu(cpu, MSR_HWP_REQUEST, );
>   adj_range = limits->min_perf_pct * range / 100;
>   min = hw_min + adj_range;
> @@ -318,7 +316,12 @@ static void intel_pstate_hwp_set(void)
>   value |= HWP_MAX_PERF(max);
>   wrmsrl_on_cpu(cpu, MSR_HWP_REQUEST, value);
>   }
> +}
>  
> +static void intel_pstate_hwp_set_online_cpus(void)
> +{
> + get_online_cpus();
> + intel_pstate_hwp_set(cpu_online_mask);
>   put_online_cpus();
>  }
>  
> @@ -440,7 +443,7 @@ static ssize_t store_no_turbo(struct kobject *a, struct 
> attribute *b,
>   limits->no_turbo = clamp_t(int, input, 0, 1);
>  
>   if (hwp_active)
> - intel_pstate_hwp_set();
> + intel_pstate_hwp_set_online_cpus();
>  
>   return count;
>  }
> @@ -466,7 +469,7 @@ static ssize_t store_max_perf_pct(struct kobject *a, 
> struct attribute *b,
>     int_tofp(100));
>  
>   if (hwp_active)
> - intel_pstate_hwp_set();
> + intel_pstate_hwp_set_online_cpus();
>   return

Re: [PATCH v2 3/3] drm/i915/shrinker: Hook up vmap allocation failure notifier

[Joonas Lahtinen]

On ma, 2016-04-04 at 14:46 +0100, Chris Wilson wrote:
> If the core runs out of vmap address space, it will call a notifier in
> case any driver can reap some of its vmaps. As i915.ko is possibily
> holding onto vmap address space that could be recovered, hook into the
> notifier chain and try and reap objects holding onto vmaps.
> 
> Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
> Cc: Andrew Morton <a...@linux-foundation.org>
> Cc: David Rientjes <rient...@google.com>
> Cc: Roman Pen <r.peni...@gmail.com>
> Cc: Mel Gorman <mgor...@techsingularity.net>
> Cc: linux...@kvack.org
> Cc: linux-kernel@vger.kernel.org
> Cc: Joonas Lahtinen <joonas.lahti...@linux.intel.com>

A comment below. But regardless;

Reviewed-by: Joonas Lahtinen <joonas.lahti...@linux.intel.com>

> Cc: Tvrtko Ursulin <tvrtko.ursu...@intel.com>
> Cc: Mika Kahola <mika.kah...@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  1 +
>  drivers/gpu/drm/i915/i915_gem_shrinker.c | 39 
> 
>  2 files changed, 40 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index dd187727c813..6443745d4182 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1257,6 +1257,7 @@ struct i915_gem_mm {
>   struct i915_hw_ppgtt *aliasing_ppgtt;
>  
>   struct notifier_block oom_notifier;
> + struct notifier_block vmap_notifier;
>   struct shrinker shrinker;
>   bool shrinker_no_lock_stealing;
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c 
> b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> index e391ee3ec486..be7501afb59e 100644
> --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
> +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> @@ -28,6 +28,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  
> @@ -356,6 +357,40 @@ i915_gem_shrinker_oom(struct notifier_block *nb, 
> unsigned long event, void *ptr)
>   return NOTIFY_DONE;
>  }
>  
> +static int
> +i915_gem_shrinker_vmap(struct notifier_block *nb, unsigned long event, void 
> *ptr)
> +{
> + struct drm_i915_private *dev_priv =
> + container_of(nb, struct drm_i915_private, mm.vmap_notifier);
> + struct drm_device *dev = dev_priv->dev;
> + unsigned long timeout = msecs_to_jiffies(5000) + 1;
> + unsigned long freed_pages;
> + bool was_interruptible;
> + bool unlock;
> +
> + while (!i915_gem_shrinker_lock(dev, ) && --timeout) {
> + schedule_timeout_killable(1);
> + if (fatal_signal_pending(current))
> + return NOTIFY_DONE;
> + }
> + if (timeout == 0) {
> + pr_err("Unable to purge GPU vmaps due to lock contention.\n");
> + return NOTIFY_DONE;
> + }
> +
> + was_interruptible = dev_priv->mm.interruptible;
> + dev_priv->mm.interruptible = false;
> +
> + freed_pages = i915_gem_shrink_all(dev_priv);
> +
> + dev_priv->mm.interruptible = was_interruptible;

Up until here this is same function as the oom shrinker, so I would
combine these two and here do "if (vmap) goto out;" sort of thing.

Would just need a way to distinct between two calling sites. I did not
come up with a quick solution as both are passing 0 as event.

Regards, Joonas

> + if (unlock)
> + mutex_unlock(>struct_mutex);
> +
> + *(unsigned long *)ptr += freed_pages;
> + return NOTIFY_DONE;
> +}
> +
>  /**
>   * i915_gem_shrinker_init - Initialize i915 shrinker
>   * @dev_priv: i915 device
> @@ -371,6 +406,9 @@ void i915_gem_shrinker_init(struct drm_i915_private 
> *dev_priv)
>  
>   dev_priv->mm.oom_notifier.notifier_call = i915_gem_shrinker_oom;
>   WARN_ON(register_oom_notifier(_priv->mm.oom_notifier));
> +
> + dev_priv->mm.vmap_notifier.notifier_call = i915_gem_shrinker_vmap;
> + WARN_ON(register_vmap_purge_notifier(_priv->mm.vmap_notifier));
>  }
>  
>  /**
> @@ -381,6 +419,7 @@ void i915_gem_shrinker_init(struct drm_i915_private 
> *dev_priv)
>   */
>  void i915_gem_shrinker_cleanup(struct drm_i915_private *dev_priv)
>  {
> + WARN_ON(unregister_vmap_purge_notifier(_priv->mm.vmap_notifier));
>   WARN_ON(unregister_oom_notifier(_priv->mm.oom_notifier));
>   unregister_shrinker(_priv->mm.shrinker);
>  }
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: [PATCH v2 2/3] mm/vmap: Add a notifier for when we run out of vmap address space

[Joonas Lahtinen]

On ma, 2016-04-04 at 14:46 +0100, Chris Wilson wrote:
> vmaps are temporary kernel mappings that may be of long duration.
> Reusing a vmap on an object is preferrable for a driver as the cost of
> setting up the vmap can otherwise dominate the operation on the object.
> However, the vmap address space is rather limited on 32bit systems and
> so we add a notification for vmap pressure in order for the driver to
> release any cached vmappings.
> 
> The interface is styled after the oom-notifier where the callees are
> passed a pointer to an unsigned long counter for them to indicate if they
> have freed any space.
> 
> v2: Guard the blocking notifier call with gfpflags_allow_blocking()
> v3: Correct typo in forward declaration and move to head of file
> 
> Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
> Cc: Andrew Morton <a...@linux-foundation.org>
> Cc: David Rientjes <rient...@google.com>
> Cc: Roman Peniaev <r.peni...@gmail.com>
> Cc: Mel Gorman <mgor...@techsingularity.net>
> Cc: linux...@kvack.org
> Cc: linux-kernel@vger.kernel.org
> Acked-by: Andrew Morton <a...@linux-foundation.org> # for inclusion via DRM
> Cc: Joonas Lahtinen <joonas.lahti...@linux.intel.com>

Reviewed-by: Joonas Lahtinen <joonas.lahti...@linux.intel.com>

> Cc: Tvrtko Ursulin <tvrtko.ursu...@intel.com>
> ---
>  include/linux/vmalloc.h |  4 
>  mm/vmalloc.c| 27 +++
>  2 files changed, 31 insertions(+)
> 
> diff --git a/include/linux/vmalloc.h b/include/linux/vmalloc.h
> index d1f1d338af20..8b51df3ab334 100644
> --- a/include/linux/vmalloc.h
> +++ b/include/linux/vmalloc.h
> @@ -8,6 +8,7 @@
>  #include 
>  
>  struct vm_area_struct;   /* vma defining user mapping in 
> mm_types.h */
> +struct notifier_block;   /* in notifier.h */
>  
>  /* bits in flags of vmalloc's vm_struct below */
>  #define VM_IOREMAP   0x0001  /* ioremap() and friends */
> @@ -187,4 +188,7 @@ pcpu_free_vm_areas(struct vm_struct **vms, int nr_vms)
>  #define VMALLOC_TOTAL 0UL
>  #endif
>  
> +int register_vmap_purge_notifier(struct notifier_block *nb);
> +int unregister_vmap_purge_notifier(struct notifier_block *nb);
> +
>  #endif /* _LINUX_VMALLOC_H */
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index ae7d20b447ff..293889d7f482 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -21,6 +21,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -344,6 +345,8 @@ static void __insert_vmap_area(struct vmap_area *va)
>  
>  static void purge_vmap_area_lazy(void);
>  
> +static BLOCKING_NOTIFIER_HEAD(vmap_notify_list);
> +
>  /*
>   * Allocate a region of KVA of the specified size and alignment, within the
>   * vstart and vend.
> @@ -363,6 +366,8 @@ static struct vmap_area *alloc_vmap_area(unsigned long 
> size,
>   BUG_ON(offset_in_page(size));
>   BUG_ON(!is_power_of_2(align));
>  
> + might_sleep_if(gfpflags_allow_blocking(gfp_mask));
> +
>   va = kmalloc_node(sizeof(struct vmap_area),
>   gfp_mask & GFP_RECLAIM_MASK, node);
>   if (unlikely(!va))
> @@ -468,6 +473,16 @@ overflow:
>   purged = 1;
>   goto retry;
>   }
> +
> + if (gfpflags_allow_blocking(gfp_mask)) {
> + unsigned long freed = 0;
> + blocking_notifier_call_chain(_notify_list, 0, );
> + if (freed > 0) {
> + purged = 0;
> + goto retry;
> + }
> + }
> +
>   if (printk_ratelimit())
>   pr_warn("vmap allocation for size %lu failed: use vmalloc= to 
> increase size\n",
>   size);
> @@ -475,6 +490,18 @@ overflow:
>   return ERR_PTR(-EBUSY);
>  }
>  
> +int register_vmap_purge_notifier(struct notifier_block *nb)
> +{
> + return blocking_notifier_chain_register(_notify_list, nb);
> +}
> +EXPORT_SYMBOL_GPL(register_vmap_purge_notifier);
> +
> +int unregister_vmap_purge_notifier(struct notifier_block *nb)
> +{
> + return blocking_notifier_chain_unregister(_notify_list, nb);
> +}
> +EXPORT_SYMBOL_GPL(unregister_vmap_purge_notifier);
> +
>  static void __free_vmap_area(struct vmap_area *va)
>  {
>   BUG_ON(RB_EMPTY_NODE(>rb_node));
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: [PATCH] kernfs: Move faulting copy_user operations outside of the mutex

[Joonas Lahtinen]

On to, 2016-03-31 at 12:49 -0400, Tejun Heo wrote:
> On Thu, Mar 31, 2016 at 11:45:06AM +0100, Chris Wilson wrote:
> > 
> > A fault in a user provided buffer may lead anywhere, and lockdep warns
> > that we have a potential deadlock between the mm->mmap_sem and the
> > kernfs file mutex:
> ...
> > 
> > Reported-by: Ville Syrjälä <ville.syrj...@linux.intel.com>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94350
> > Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
> > Reviewed-by: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
> > Cc: Ville Syrjälä <ville.syrj...@linux.intel.com>
> > Cc: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
> > Cc: Tejun Heo <t...@kernel.org>
> > Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>
> > Cc: NeilBrown <ne...@suse.de>
> > Cc: linux-kernel@vger.kernel.org
> Acked-by: Tejun Heo <t...@kernel.org>
> 

Thanks.

I have applied this locally to our repo to be included into our CI
builds.

We will drop the local patch once this waterfalls from upstream to our
drm-intel-nightly repo.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: [PATCH] kernfs: Move faulting copy_user operations outside of the mutex

[Joonas Lahtinen]

On to, 2016-03-31 at 13:15 -0700, Greg Kroah-Hartman wrote:
> On Thu, Mar 31, 2016 at 08:30:05PM +0300, Joonas Lahtinen wrote:
> > 
> > On to, 2016-03-31 at 12:49 -0400, Tejun Heo wrote:
> > > 
> > > On Thu, Mar 31, 2016 at 11:45:06AM +0100, Chris Wilson wrote:
> > > > 
> > > > 
> > > > A fault in a user provided buffer may lead anywhere, and lockdep warns
> > > > that we have a potential deadlock between the mm->mmap_sem and the
> > > > kernfs file mutex:
> > > ...
> > > > 
> > > > 
> > > > Reported-by: Ville Syrjälä <ville.syrj...@linux.intel.com>
> > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94350
> > > > Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
> > > > Reviewed-by: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
> > > > Cc: Ville Syrjälä <ville.syrj...@linux.intel.com>
> > > > Cc: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
> > > > Cc: Tejun Heo <t...@kernel.org>
> > > > Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>
> > > > Cc: NeilBrown <ne...@suse.de>
> > > > Cc: linux-kernel@vger.kernel.org
> > > Acked-by: Tejun Heo <t...@kernel.org>
> > > 
> > Thanks.
> > 
> > I have applied this locally to our repo to be included into our CI
> > builds.
> > 
> > We will drop the local patch once this waterfalls from upstream to our
> > drm-intel-nightly repo.
> So is this something that needs to get into 4.6-final because it
> resolves a reported issue?  Or can it wait for 4.7-rc1?
> 

It is only a getting rid of a lockdep splat describing a scenario very
unlikely to ever happen, so it can wait for 4.7-rc1.

Regards, Joonas

> thanks,
> 
> greg k-h
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: [Intel-gfx] drm/i915: WARN_ON_ONCE(!crtc_clock || cdclk < crtc_clock)

[Joonas Lahtinen]

On ke, 2016-10-12 at 11:56 +0200, Paul Bolle wrote:
> On a laptop that tracks the latest stable release (Ie, it now runs
> v4.8.1) I see this WARNING
>     WARN_ON_ONCE(!crtc_clock || cdclk < crtc_clock)
> 
> Full trace pasted below. I never saw this WARNING before v4.8. Since
> v4.8 I've had it in all (four, actually) boots.
> 
> What am I expected to do about this WARNING?
> 

Bisecting the offending commit between v4.8 and v4.8.1 would be a good
start.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: [PATCH 1/3] drm/i915: allocate mock file pointer dynamically

[Joonas Lahtinen]

On ma, 2017-03-20 at 10:40 +0100, Arnd Bergmann wrote:
> A struct file is a bit too large to put on the kernel stack in general
> and triggers a warning for low settings of CONFIG_FRAME_WARN:
> 
> drivers/gpu/drm/i915/selftests/mock_drm.c: In function 'mock_file':
> drivers/gpu/drm/i915/selftests/mock_drm.c:46:1: error: the frame size of 1328 
> bytes is larger than 1280 bytes [-Werror=frame-larger-than=]
> drivers/gpu/drm/i915/selftests/mock_drm.c: In function 'mock_file_free':
> drivers/gpu/drm/i915/selftests/mock_drm.c:54:1: error: the frame size of 1312 
> bytes is larger than 1280 bytes [-Werror=frame-larger-than=]
> 
> It's also slightly dangerous to leave a reference to a stack object
> in the drm_file structure after leaving the stack frame.
> This changes the code to just allocate the object dynamically
> and release it when we are done with it.
> 
> Fixes: 66d9cb5d805a ("drm/i915: Mock the GEM device for self-testing")
> Signed-off-by: Arnd Bergmann <a...@arndb.de>



> ---
>  drivers/gpu/drm/i915/selftests/mock_drm.c | 12 +++-
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/selftests/mock_drm.c 
> b/drivers/gpu/drm/i915/selftests/mock_drm.c
> index 113dec05c7dc..18514065c93d 100644
> --- a/drivers/gpu/drm/i915/selftests/mock_drm.c
> +++ b/drivers/gpu/drm/i915/selftests/mock_drm.c
> @@ -32,15 +32,15 @@ static inline struct inode fake_inode(struct 
> drm_i915_private *i915)
>  struct drm_file *mock_file(struct drm_i915_private *i915)
>  {
> >     struct inode inode = fake_inode(i915);
> > -   struct file filp = {};
> > +   struct file *filp = kzalloc(sizeof(struct file), GFP_KERNEL);
> >     struct drm_file *file;
> >     int err;
>  

filp = kzalloc(sizeof(*filp), GFP_KERNEL);
if (unlikely(!filp)) {
err = -ENOMEM;
    goto err;
}

And appropriate onion teardown in case drm_open fails, so that we don't
leak memory.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: [Intel-gfx] [PATCH 1/5] i915: avoid kernel hang caused by synchronize rcu struct_mutex deadlock

[Joonas Lahtinen]

On pe, 2017-04-07 at 01:23 +0200, Andrea Arcangeli wrote:
> synchronize_rcu/synchronize_sched/synchronize_rcu_expedited() will
> hang until its own workqueues are run. The i915 gem workqueues will
> wait on the struct_mutex to be released. So we cannot wait for a
> quiescent state using those rcu primitives while holding the
> struct_mutex or it creates a circular lock dependency resulting in
> kernel hangs (which is reproducible but goes undetected by lockdep).
> 
> This started in commit 3d3d18f086cdda72ee18a454db70ca72c6e3246c and
> lockdep didn't detect it apparently.

The right format is;

Fixes: 3d3d18f086cd ("drm/i915: Avoid rcu_barrier() from reclaim paths 
(shrinker)")

> @@ -324,6 +320,16 @@ i915_gem_shrinker_scan(struct shrinker *shrinker, struct 
> shrink_control *sc)
>   if (unlock)
>   mutex_unlock(>struct_mutex);
>  
> + if (likely(__mutex_owner(>struct_mutex) != current))

This check can be dropped and synchronize_rcu_expedited() should be
embedded directly to the if (unlock) branch as it's functionally
equivalent. This can be applied to all the unlock cases, not just this
one. That should be the correct action to avoid the deadlock. I've sent
a patch to do this (Cc'd you), can you verify that it gets rid of the
problem for you?

> + /*
> +  * If reclaim was invoked by an allocation done while
> +  * holding the struct mutex, we cannot call
> +  * synchronize_rcu_expedited() as it depends on
> +  * workqueues to run but the running workqueue may be
> +  * blocked waiting on us to release struct_mutex.
> +  */
> + synchronize_rcu_expedited();
> +
>   return freed;
>  }
>  
> ___
> Intel-gfx mailing list
> intel-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: Q. drm/i915 shrinker, synchronize_rcu_expedited() from handlers

[Joonas Lahtinen]

On ke, 2017-05-10 at 12:43 +0200, Andrea Arcangeli wrote:
> It works for me too. I'm running my workstation also with
> synchronize_rcu removed from i915_gem_shrink_all in addition to the
> above. Isn't the oom method invoked from reclaim context too? As far
> as I can tell synchronize_rcu can end up throttling on a background
> synchronize_rcu_expedited(), so it might end up in the same issue
> unless removed too.

Thanks for testing and spotting my bad grepping, I'll add your T-b and
s
end v3.

Regards, Joonas

> Tested-by: Andrea Arcangeli <aarca...@redhat.com>
> 
> (I can't reproduce the lockups 100% of the time, but they never
> happened again with this patch and I happened to run the load that
> reproduces them a couple of times already with v4.11 and this patch
> applied)
> 
> It's also certainly improving performance by removing the
> synchronize_rcu_expedited from the _count methods where it was useless
> (in addition to unsafe).
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: Q. drm/i915 shrinker, synchronize_rcu_expedited() from handlers

[Joonas Lahtinen]

On ti, 2017-05-09 at 20:04 -0700, Hugh Dickins wrote:
> On Mon, 8 May 2017, Joonas Lahtinen wrote:
> > On pe, 2017-05-05 at 14:57 -0700, Hugh Dickins wrote:
> > > On Fri, 5 May 2017, Joonas Lahtinen wrote:
> > > > On ma, 2017-05-01 at 11:05 +0900, J. R. Okajima wrote:
> > > > > Thanx for the reply.
> > > > > 
> > > > > Andrea Arcangeli:
> > > > > > 
> > > > > > Yes I already reported this, my original fix was way more efficient
> > > > > > (and also safer considering the above) than what landed upstream. My
> > > > > > feedback was ignored though.
> > > > > > 
> > > > > > https://lists.freedesktop.org/archives/intel-gfx/2017-April/125414.html
> > > > > 
> > > > > I see.
> > > > > Actually on my test system for v4.11-rc8, kthreadd, kworker, kswapd 
> > > > > and
> > > > > others all stopped working due to the synchronize_rcu_expedited call
> > > > > from i915_gem_shrinker_count. It is definitly a show stopper for me as
> > > > > an i915 user.
> > > > 
> > > > Filing a bug in freedesktop.org with all the details is the fastest way
> > > > of getting help. Without the bug (and with such little information as
> > > > the previous e-mail) it's hard to estimate the extent and nature of the
> > > > bug.
> > > > 
> > > > I've anyway gone and prepared a patch to drop the RCU sync completely
> > > > from shrinker phase, as discussed originally with Chris.
> > > 
> > > Is that a patch that will be suitable for 4.11-stable?  Please do post
> > > it here.  I had not experienced this i915-induced hang at all when
> > > Andrea first mentioned it, nor even on 4.11-rc8; but now with 4.11
> > > final I can get it fairly easily (I haven't tried Andrea's fix yet).
> > 
> > Please try:
> > 
> > https://patchwork.freedesktop.org/patch/154713/
> > 
> > If it works, a Tested-by: would be appreciated.
> 
> Yes, that works for me, thank you.
> 
> Tested-by: Hugh Dickins <hu...@google.com>
> 
> But the linked patch seems to be lacking a Reported-by (not me) tag,
> a Fixes tag, a Cc stable tag, and any indication in the Subject or
> commit message that this patch is something needed to fix hangs
> observed by several people - it just sounds like a minor cleanup.

It is a patch that was agreed to be pushed anyway, so if it wouldn't
have resolved the problem, I'd have pushed it as is.

I'll add J. R. Okajima as Reported-by and refer to the bisected commit,
even though so far Freedesktop Bugzilla or intel-gfx mailing list has
no other reports.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: Q. drm/i915 shrinker, synchronize_rcu_expedited() from handlers

[Joonas Lahtinen]

On la, 2017-05-20 at 10:56 +0900, J. R. Okajima wrote:
> "J. R. Okajima":
> > 
> > I don't know whether the fix is good to me or not yet. I will test your
> > fix, but I am busy now and my test will be a few weeks later. Other
> > people may want the fix soon. So I'd suggest you to reproduce the
> > problem on your side. I guess "mem=1G" or "mem=512M" will make it easier
> > to reproduce the problem.
> > Of course, if you are sure the fix is correct, then you don't have to
> > wait for my test. Release it soon for other people.
> 
> Now I am going to able to run my local test.
> But V3 patch failed to apply onto v4.11.0.
> 
> Would you provide us two versions of the patch, one for v4.11 series and
> the other of v4.12-rcN?

For v4.12-rc2 the backport is here:

https://patchwork.freedesktop.org/patch/156990/

For quick testing on older kernels, just remove all lines with "_rcu"
in drivers/gpu/drm/i915/i915_gem_shrinker.c .

Regards, Joonas

PS: It'll help to subscribe to and track our mailing list at
intel-...@lists.freedesktop.org for future convenience.
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: Q. drm/i915 shrinker, synchronize_rcu_expedited() from handlers

[Joonas Lahtinen]

On ti, 2017-05-30 at 13:00 -0700, Hugh Dickins wrote:
> On Mon, 22 May 2017, Joonas Lahtinen wrote:
> > On la, 2017-05-20 at 10:56 +0900, J. R. Okajima wrote:
> > > "J. R. Okajima":
> > > > 
> > > > I don't know whether the fix is good to me or not yet. I will test your
> > > > fix, but I am busy now and my test will be a few weeks later. Other
> > > > people may want the fix soon. So I'd suggest you to reproduce the
> > > > problem on your side. I guess "mem=1G" or "mem=512M" will make it easier
> > > > to reproduce the problem.
> > > > Of course, if you are sure the fix is correct, then you don't have to
> > > > wait for my test. Release it soon for other people.
> > > 
> > > Now I am going to able to run my local test.
> > > But V3 patch failed to apply onto v4.11.0.
> > > 
> > > Would you provide us two versions of the patch, one for v4.11 series and
> > > the other of v4.12-rcN?
> > 
> > For v4.12-rc2 the backport is here:
> > 
> > https://patchwork.freedesktop.org/patch/156990/
> 
> This fix seems to have got lost: we've been waiting a month,
> and 4.12 is now at rc3: please expedite the unexpedited :)

Don't worry, it's not lost. It was merged to drm-intel-fixes and thus is in the 
pipeline.

There were some unexpected delays getting fixes in, sorry for that.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: Q. drm/i915 shrinker, synchronize_rcu_expedited() from handlers

[Joonas Lahtinen]

On pe, 2017-05-05 at 14:57 -0700, Hugh Dickins wrote:
> On Fri, 5 May 2017, Joonas Lahtinen wrote:
> > On ma, 2017-05-01 at 11:05 +0900, J. R. Okajima wrote:
> > > Thanx for the reply.
> > > 
> > > Andrea Arcangeli:
> > > > 
> > > > Yes I already reported this, my original fix was way more efficient
> > > > (and also safer considering the above) than what landed upstream. My
> > > > feedback was ignored though.
> > > > 
> > > > https://lists.freedesktop.org/archives/intel-gfx/2017-April/125414.html
> > > 
> > > I see.
> > > Actually on my test system for v4.11-rc8, kthreadd, kworker, kswapd and
> > > others all stopped working due to the synchronize_rcu_expedited call
> > > from i915_gem_shrinker_count. It is definitly a show stopper for me as
> > > an i915 user.
> > 
> > Filing a bug in freedesktop.org with all the details is the fastest way
> > of getting help. Without the bug (and with such little information as
> > the previous e-mail) it's hard to estimate the extent and nature of the
> > bug.
> > 
> > I've anyway gone and prepared a patch to drop the RCU sync completely
> > from shrinker phase, as discussed originally with Chris.
> 
> Is that a patch that will be suitable for 4.11-stable?  Please do post
> it here.  I had not experienced this i915-induced hang at all when
> Andrea first mentioned it, nor even on 4.11-rc8; but now with 4.11
> final I can get it fairly easily (I haven't tried Andrea's fix yet).

Please try:

https://patchwork.freedesktop.org/patch/154713/

If it works, a Tested-by: would be appreciated.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: Q. drm/i915 shrinker, synchronize_rcu_expedited() from handlers

[Joonas Lahtinen]

On ma, 2017-05-01 at 11:05 +0900, J. R. Okajima wrote:
> Thanx for the reply.
> 
> Andrea Arcangeli:
> > 
> > Yes I already reported this, my original fix was way more efficient
> > (and also safer considering the above) than what landed upstream. My
> > feedback was ignored though.
> > 
> > https://lists.freedesktop.org/archives/intel-gfx/2017-April/125414.html
> 
> I see.
> Actually on my test system for v4.11-rc8, kthreadd, kworker, kswapd and
> others all stopped working due to the synchronize_rcu_expedited call
> from i915_gem_shrinker_count. It is definitly a show stopper for me as
> an i915 user.

Filing a bug in freedesktop.org with all the details is the fastest way
of getting help. Without the bug (and with such little information as
the previous e-mail) it's hard to estimate the extent and nature of the
bug.

I've anyway gone and prepared a patch to drop the RCU sync completely
from shrinker phase, as discussed originally with Chris.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly

[Joonas Lahtinen]

On Thu, 2017-09-21 at 16:17 +, Wang, Zhi A wrote:
> Hi Joonas:
> 
> Thanks for the introduction. I have been thinking about the
> possibility of introducing GEM_BUG_ON into GVT-g recently and
> investigating on it. I'm just a bit confused about the usage between
> GEM_BUG_ON and WARN_ON.

GEM_BUG_ON is basically there to catch things that we do not expect
ever to happen within the driver. So we often list the function
preconditions as GEM_BUG_ON. It's there for the same reason as the
lockdep_assert_held and KASAN. It's sometimes heavy checks that we
really want to run when functionally validating kernel.

GEM_BUG_ON became to existence because adding checks for obvious
conditions at the critical command submission path GEM is not
sustainable for performance in production.

The expectation is that each GEM_BUG_ON has a testcase in I-G-T that
has the potential to hit it if driver was modified not to respect those
preconditions. So once our testest passes, we can disable the
GEM_BUG_ONs and be confident of the internal driver quality and get the
release performance.

WARN_ON is mostly used for the cases when the hardware is behaving
differently than we expect. We can't remove them as we don't have all
the hardware in the world to test, but we try to exercise them too
through I-G-Ts. The test will often be the subtest that was written to
reproduce the problem with our expectations of hardware in case of
hangs and other bugs. After we've corrected the driver behaviour, or
got a hardware W/A assigned, we keep the test and add a WARN_ON to make
sure there will be no regression back to the same situation.

This is at least what should happen, given time constraints, there may
be variations.

User behaving unexpectedly should never result in WARN_ON (or even
worse, BUG_ON), should always just be debug messages displayed (not to
trigger the CI) and errors propagated back to user:

https://01.org/linuxgraphics/gfx-docs/drm/gpu/drm-uapi.html#recommended
-ioctl-return-values

Bare BUG_ON should only be used when there's the danger of corrupting
system memory or filesystems, so from graphics driver, that's not very
often. Controlled propagation of errors and maybe WARN_ON is always
preferred if possible.


> GEM_BUG_ON is only enabled when kernel debug is enabled, which mostly
> is disabled in a production kernel. In the case of i915, I'm sure it
> will be enabled in CI test so that it can catch broken code path.
> Looking into GVT-g, the similar scenario is we enable it in QA test.
> 
> Let's say GEM_BUG_ON can do its work very well in QA test but QA test
> is not fully covered all the condition, then something might be still
> broken when it comes to the production kernel for user and GEM_BUG_ON
> will be disabled and will not catch that, I guess.
> 
> That's my confusion which scratched my mind during the investigation:
> If GEM_BUG_ON is not always working, then it looks WARN_ON should
> always be used Expected to learn more about the story behind. :)

So if the saying is some object is "never going to be bigger than 2G",
there should be either:

1. GEM_BUG_ON like assertion for it and a test that tries to hit it, by
trying to allocate a huge object for example, and should get rejection
as -EINVAL

2. Test to see if the object is bigger, and propagate back the error if
it is. Either resulting in user reported error if the origin of the
object is outside of kernel <-> hardware. Or a WARN_ON if it's strange
hardware or kernel driver behavior.

You should choose depending on how often your function gets called, and
how critical the execution time is.

Hopefully this clarified things.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly

[Joonas Lahtinen]

On Thu, 2017-09-21 at 06:44 +0800, Zhenyu Wang wrote:
> On 2017.09.19 19:35:23 -0700, Joe Perches wrote:
> > On Wed, 2017-09-20 at 05:46 +0800, Zhenyu Wang wrote:
> > > On 2017.09.19 16:55:34 +0100, Colin King wrote:
> > > > From: Colin Ian King <colin.k...@canonical.com>
> > > > 
> > > > An earlier fix changed the return type from find_bb_size however the
> > > > integer return is being assigned to a unsigned int so the -ve error
> > > > check will never be detected. Make bb_size an int to fix this.
> > > > 
> > > > Detected by CoverityScan CID#1456886 ("Unsigned compared against 0")
> > > > 
> > > > Fixes: 1e3197d6ad73 ("drm/i915/gvt: Refine error handling for 
> > > > perform_bb_shadow")
> > > > Signed-off-by: Colin Ian King <colin.k...@canonical.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/gvt/cmd_parser.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/gvt/cmd_parser.c 
> > > > b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > > index 2c0ccbb817dc..f41cbf664b69 100644
> > > > --- a/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > > +++ b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > > @@ -1628,7 +1628,7 @@ static int perform_bb_shadow(struct 
> > > > parser_exec_state *s)
> > > > struct intel_shadow_bb_entry *entry_obj;
> > > > struct intel_vgpu *vgpu = s->vgpu;
> > > > unsigned long gma = 0;
> > > > -   uint32_t bb_size;
> > > > +   int bb_size;
> > > > void *dst = NULL;
> > > > int ret = 0;
> > > >  
> > > 
> > > Applied this, thanks!
> > 
> > Is it possible for bb_size to be both >= 2g and valid?
> 
> Never be possible in practise and if really that big I think something
> is already insane indeed.

It's good idea to document these assumptions as WARN_ON's. In i915, if
the value is completely internal to kernel, we're using GEM_BUG_ON for
these so that our CI will notice breakage. If it's not a driver
internal value only, a WARN_ON is the appropriate action.

Otherwise the information is lost and the next person reading the code
will have the same question in mind.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: [PATCH v15 6/7] drm/i915: Introduce GEM proxy

[Joonas Lahtinen]

On Tue, 2017-10-10 at 17:50 +0800, Tina Zhang wrote:
> GEM proxy is a kind of GEM, whose backing physical memory is pinned
> and produced by guest VM and is used by host as read only. With GEM
> proxy, host is able to access guest physical memory through GEM object
> interface. As GEM proxy is such a special kind of GEM, a new flag
> I915_GEM_OBJECT_IS_PROXY is introduced to ban host from changing the
> backing storage of GEM proxy.
> 
> v14:
> - return -ENXIO when gem proxy object is banned by ioctl.
>   (Chris) (Daniel)
> 
> v13:
> - add comments to GEM proxy. (Chris)
> - don't ban GEM proxy in i915_gem_sw_finish_ioctl. (Chris)
> - check GEM proxy bar after finishing i915_gem_object_wait. (Chris)
> - remove GEM proxy bar in i915_gem_madvise_ioctl.
> 
> v6:
> - add gem proxy barrier in the following ioctls. (Chris)
>   i915_gem_set_caching_ioctl
>   i915_gem_set_domain_ioctl
>   i915_gem_sw_finish_ioctl
>   i915_gem_set_tiling_ioctl
>   i915_gem_madvise_ioctl
> 
> Signed-off-by: Tina Zhang <tina.zh...@intel.com>
> Cc: Daniel Vetter <daniel.vet...@ffwll.ch>
> Cc: Chris Wilson <ch...@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahti...@linux.intel.com>



> +++ b/drivers/gpu/drm/i915/i915_gem_object.h
> @@ -39,6 +39,7 @@ struct drm_i915_gem_object_ops {
> unsigned int flags;
>  #define I915_GEM_OBJECT_HAS_STRUCT_PAGE BIT(0)
>  #define I915_GEM_OBJECT_IS_SHRINKABLE   BIT(1)
> +#define I915_GEM_OBJECT_IS_PROXY   BIT(2)

Please fix the indent to match. Do convert the above two lines to use
TAB character too.



> @@ -1690,7 +1704,7 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data,
>*/
>   if (!obj->base.filp) {
>   i915_gem_object_put(obj);
> -     return -EINVAL;
> + return -ENXIO;
>   }

This still needs to be a separate patch.

With those fixes, this is;

Reviewed-by: Joonas Lahtinen <joonas.lahti...@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: [Intel-gfx] [PATCH v15 6/7] drm/i915: Introduce GEM proxy

[Joonas Lahtinen]

On Tue, 2017-10-10 at 13:58 +0300, Joonas Lahtinen wrote:
> On Tue, 2017-10-10 at 17:50 +0800, Tina Zhang wrote:
> > GEM proxy is a kind of GEM, whose backing physical memory is pinned
> > and produced by guest VM and is used by host as read only. With GEM
> > proxy, host is able to access guest physical memory through GEM object
> > interface. As GEM proxy is such a special kind of GEM, a new flag
> > I915_GEM_OBJECT_IS_PROXY is introduced to ban host from changing the
> > backing storage of GEM proxy.
> > 
> > v14:
> > - return -ENXIO when gem proxy object is banned by ioctl.
> >   (Chris) (Daniel)
> > 
> > v13:
> > - add comments to GEM proxy. (Chris)
> > - don't ban GEM proxy in i915_gem_sw_finish_ioctl. (Chris)
> > - check GEM proxy bar after finishing i915_gem_object_wait. (Chris)
> > - remove GEM proxy bar in i915_gem_madvise_ioctl.
> > 
> > v6:
> > - add gem proxy barrier in the following ioctls. (Chris)
> >   i915_gem_set_caching_ioctl
> >   i915_gem_set_domain_ioctl
> >   i915_gem_sw_finish_ioctl
> >   i915_gem_set_tiling_ioctl
> >   i915_gem_madvise_ioctl
> > 
> > Signed-off-by: Tina Zhang <tina.zh...@intel.com>
> > Cc: Daniel Vetter <daniel.vet...@ffwll.ch>
> > Cc: Chris Wilson <ch...@chris-wilson.co.uk>
> > Cc: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
> 
> 
> 
> > +++ b/drivers/gpu/drm/i915/i915_gem_object.h
> > @@ -39,6 +39,7 @@ struct drm_i915_gem_object_ops {
> > unsigned int flags;
> >  #define I915_GEM_OBJECT_HAS_STRUCT_PAGE BIT(0)
> >  #define I915_GEM_OBJECT_IS_SHRINKABLE   BIT(1)
> > +#define I915_GEM_OBJECT_IS_PROXY   BIT(2)
> 
> Please fix the indent to match. Do convert the above two lines to use
> TAB character too.
> 
> 
> 
> > @@ -1690,7 +1704,7 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void 
> > *data,
> >  */
> > if (!obj->base.filp) {
> > i915_gem_object_put(obj);
> > -   return -EINVAL;
> > +   return -ENXIO;
> > }
> 
> This still needs to be a separate patch.
> 
> With those fixes, this is;
> 
> Reviewed-by: Joonas Lahtinen <joonas.lahti...@linux.intel.com>

Also, send the produced two patches (this and the split patch) as a
standalone series for easier testing and merging.

CI seems to have hard time applying the whole series.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: [PATCH] drm/i915: remove redundant check on has_aliasing_ppgtt

[Joonas Lahtinen]

On Tue, 2017-10-10 at 14:47 +0100, Colin King wrote:
> From: Colin Ian King <colin.k...@canonical.com>
> 
> There is a previous check to on has_aliasing_ppgtt that returns
> 0 if it is false, so it is impossible for has_aliasing_ppgtt to
> be false on the final return of function intel_sanitize_enable_ppgtt,
> so final return in the function always will return 1.  Hence the
> redundant ternary operator can be replaced with a return 1.
> 
> Detected by CoverityScan, CID#1357136 ("Logically dead code")
> 
> Signed-off-by: Colin Ian King <colin.k...@canonical.com>

Thanks, I took it a few steps further and removed the variable
altogether. I Cc'd you on the patch.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: [PATCH] drm/i915: Convert timers to use timer_setup()

[Joonas Lahtinen]

On Wed, 2017-10-04 at 17:54 -0700, Kees Cook wrote:
> In preparation for unconditionally passing the struct timer_list pointer to
> all timer callbacks, switch to using the new timer_setup() and from_timer()
> to pass the timer pointer explicitly.
> 
> Cc: Daniel Vetter <daniel.vet...@intel.com>
> Cc: Jani Nikula <jani.nik...@linux.intel.com>
> Cc: David Airlie <airl...@linux.ie>
> Cc: Chris Wilson <ch...@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursu...@intel.com>
> Cc: Oscar Mateo <oscar.ma...@intel.com>
> Cc: intel-...@lists.freedesktop.org
> Cc: dri-de...@lists.freedesktop.org
> Cc: Thomas Gleixner <t...@linutronix.de>
> Signed-off-by: Kees Cook <keesc...@chromium.org>



> @@ -32,9 +32,9 @@ static struct mock_request *first_request(struct 
> mock_engine *engine)
>   link);
>  }
>  
> -static void hw_delay_complete(unsigned long data)
> +static void hw_delay_complete(struct timer_list *t)
>  {
> - struct mock_engine *engine = (typeof(engine))data;
> + struct mock_engine *engine = from_timer(engine, t, hw_delay);

The order is bit strange to me, it's not same as with container_of, but
I guess GCC will complain for getting it wrong. It's also slightly
different doing the typeof for you, so I guess it makes sense, so:

Reviewed-by: Joonas Lahtinen <joonas.lahti...@linux.intel.com>

Do you expect for us to merge or are you looking to merge all timer
changes from single tree?

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: [PATCH] drm/i915: Convert timers to use timer_setup()

[Joonas Lahtinen]

On Mon, 2017-10-16 at 15:55 -0700, Kees Cook wrote:
> In preparation for unconditionally passing the struct timer_list pointer to
> all timer callbacks, switch to using the new timer_setup() and from_timer()
> to pass the timer pointer explicitly.
> 
> Cc: Daniel Vetter <daniel.vet...@intel.com>
> Cc: Jani Nikula <jani.nik...@linux.intel.com>
> Cc: David Airlie <airl...@linux.ie>
> Cc: Chris Wilson <ch...@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursu...@intel.com>
> Cc: Oscar Mateo <oscar.ma...@intel.com>
> Cc: intel-...@lists.freedesktop.org
> Cc: dri-de...@lists.freedesktop.org
> Signed-off-by: Kees Cook <keesc...@chromium.org>
> Reviewed-by: Joonas Lahtinen <joonas.lahti...@linux.intel.com> # for 
> mock_engine

Reviewed-by: Joonas Lahtinen <joonas.lahti...@linux.intel.com>

For some reason our CI didn't pick this up even though it was correctly
sent to the mailing list, so I will re-send with my refreshed R-b for
our CI and then we can merge this.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly

[Joonas Lahtinen]

On Fri, 2017-09-22 at 17:50 +, Wang, Zhi A wrote:
> Thanks for the reply. Learned a lot. :)
> 
> GEM_BUG_ON is new to me since it wasn't there at the beginning of
> GVT-g upstream. It showed up later. So I left a lot of WARN_ON in the
> code and some of them should be GEM_BUG_ON now.
> 
> Now I can figure out those differences. We can discuss with our QA to
> see if they would like to enable I915_GEM_DEBUG and then we can move
> to GEM_BUG_ON also, or maybe we can have a dedicated GVT_BUG_ON. :)
> Thank you so much. Have a great weekend.

GVT_BUG_ON is probably the way to go :)

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: [PATCH V3 09/29] drm/i915: deprecate pci_get_bus_and_slot()

[Joonas Lahtinen]

On Tue, 2017-12-12 at 19:07 -0500, Sinan Kaya wrote:
> On 12/12/2017 9:04 AM, Joonas Lahtinen wrote:
> > Hi,
> > 
> > I sent this individual i915 patch to our CI, and it is passing on
> > all platforms:
> > 
> > https://patchwork.freedesktop.org/series/34822/
> > 
> > Is it ok if I merge this to drm-tip already?
> 
> As long as you have this change in your tree, it should be safe.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/include/linux/pci.h?id=7912af5c835bd86f2b0347a480e0f40e2fab30d0
> 

We don't yet.

Rodrigo, can you please pull the above patch in once we get a
backmerge?

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: [PATCH] drm/i915: Mark expected switch fall-throughs

[Joonas Lahtinen]

On Mon, 2017-11-27 at 16:17 -0600, Gustavo A. R. Silva wrote:
> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> where we are expecting to fall through.

I have to say I'm totally not sold on regexps matching comment
contents. Was something more explicit ever considered? Like:

#define FALLTHROUGH __attribute__((fallthrough));

With the appropriate version checks, of course.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: [PATCH] drm/i915: Increase max texture to 16k for gen9+

[Joonas Lahtinen]

+ Ville as Jani is OoO

On Thu, 2017-12-07 at 17:26 +0800, Alex Tu wrote:
> Rrefer to another patch [1] on mesa to extend width/height to 16384.
> For issue :
>  - https://bugs.freedesktop.org/show_bug.cgi?id=102508
>  - LP: #1714178 Triple monitor display failed with Dell Dock (HiDPI)
> 
> [1] https://patchwork.freedesktop.org/patch/124918/
> 
> Signed-off-by: Alex Tu <alex...@canonical.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index 47a2f6acee50..556fa57b18b8 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13905,7 +13905,7 @@ u32 intel_fb_pitch_limit(struct drm_i915_private 
> *dev_priv,
>   /* "The stride in bytes must not exceed the of the size of 8K
>*  pixels and 32K bytes."
>*/
> - return min(8192 * cpp, 32768);
> + return min(16384 * cpp, 65536);
>   } else if (gen >= 5 && !HAS_GMCH_DISPLAY(dev_priv)) {
>   return 32*1024;
>   } else if (gen >= 4) {
> @@ -14604,8 +14604,8 @@ int intel_modeset_init(struct drm_device *dev)
>   dev->mode_config.max_width = 4096;
>   dev->mode_config.max_height = 4096;
>   } else {
> - dev->mode_config.max_width = 8192;
> - dev->mode_config.max_height = 8192;
> + dev->mode_config.max_width = 16384;
> + dev->mode_config.max_height = 16384;
>   }
>  
>   if (IS_I845G(dev_priv) || IS_I865G(dev_priv)) {
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: [PATCH 1/8] x86/early-quirks: Extend Intel graphics stolen memory placement to 64bit

[Joonas Lahtinen]

Hi Ingo & Thomas,

Now would be a great moment to slap the final Acked-bys (first two
patches of this series) as the comments have been addressed and
Reviewed-by was refreshed by Chris. I consider the series ready to be
merged in this state.

Once acked, I will then proceed to merge these through the drm-tip tree
as previously discussed.

Regards, Joonas

On Mon, 2017-12-11 at 12:14 +, Matthew Auld wrote:
> From: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
> 
> To give upcoming SKU BIOSes more flexibility in placing the Intel
> graphics stolen memory, make all variables storing the placement or size
> compatible with full 64 bit range. Also by exporting the stolen region
> as a resource, we can then nuke the duplicated stolen discovery in i915.
> 
> Signed-off-by: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
> Signed-off-by: Matthew Auld <matthew.a...@intel.com>
> Cc: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrj...@linux.intel.com>
> Cc: Chris Wilson <ch...@chris-wilson.co.uk>
> Cc: Paulo Zanoni <paulo.r.zan...@intel.com>
> Cc: Thomas Gleixner <t...@linutronix.de>
> Cc: Ingo Molnar <mi...@kernel.org>
> Cc: H. Peter Anvin <h...@zytor.com>
> Cc: x...@kernel.org
> Cc: linux-kernel@vger.kernel.org
> Reviewed-by: Chris Wilson <ch...@chris-wilson.co.uk> #v3
> ---
> 
> v2: export the stolen region as a resource
> fix u16 << 16 (Chris)
> v3: actually fix u16 << 16
> v4: mark intel_graphics_stolen_res as __ro_after_init (Thomas)
> 
>  arch/x86/kernel/early-quirks.c | 86 
> +++---
>  include/drm/i915_drm.h |  3 ++
>  2 files changed, 50 insertions(+), 39 deletions(-)
> 
> diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
> index 1e82f787c160..6c1624889011 100644
> --- a/arch/x86/kernel/early-quirks.c
> +++ b/arch/x86/kernel/early-quirks.c
> @@ -243,7 +243,7 @@ static void __init intel_remapping_check(int num, int 
> slot, int func)
>  #define KB(x)((x) * 1024UL)
>  #define MB(x)(KB (KB (x)))
>  
> -static size_t __init i830_tseg_size(void)
> +static resource_size_t __init i830_tseg_size(void)
>  {
>   u8 esmramc = read_pci_config_byte(0, 0, 0, I830_ESMRAMC);
>  
> @@ -256,7 +256,7 @@ static size_t __init i830_tseg_size(void)
>   return KB(512);
>  }
>  
> -static size_t __init i845_tseg_size(void)
> +static resource_size_t __init i845_tseg_size(void)
>  {
>   u8 esmramc = read_pci_config_byte(0, 0, 0, I845_ESMRAMC);
>   u8 tseg_size = esmramc & I845_TSEG_SIZE_MASK;
> @@ -273,7 +273,7 @@ static size_t __init i845_tseg_size(void)
>   return 0;
>  }
>  
> -static size_t __init i85x_tseg_size(void)
> +static resource_size_t __init i85x_tseg_size(void)
>  {
>   u8 esmramc = read_pci_config_byte(0, 0, 0, I85X_ESMRAMC);
>  
> @@ -283,12 +283,12 @@ static size_t __init i85x_tseg_size(void)
>   return MB(1);
>  }
>  
> -static size_t __init i830_mem_size(void)
> +static resource_size_t __init i830_mem_size(void)
>  {
>   return read_pci_config_byte(0, 0, 0, I830_DRB3) * MB(32);
>  }
>  
> -static size_t __init i85x_mem_size(void)
> +static resource_size_t __init i85x_mem_size(void)
>  {
>   return read_pci_config_byte(0, 0, 1, I85X_DRB3) * MB(32);
>  }
> @@ -297,36 +297,36 @@ static size_t __init i85x_mem_size(void)
>   * On 830/845/85x the stolen memory base isn't available in any
>   * register. We need to calculate it as TOM-TSEG_SIZE-stolen_size.
>   */
> -static phys_addr_t __init i830_stolen_base(int num, int slot, int func,
> -size_t stolen_size)
> +static resource_size_t __init i830_stolen_base(int num, int slot, int func,
> +resource_size_t stolen_size)
>  {
> - return (phys_addr_t)i830_mem_size() - i830_tseg_size() - stolen_size;
> + return i830_mem_size() - i830_tseg_size() - stolen_size;
>  }
>  
> -static phys_addr_t __init i845_stolen_base(int num, int slot, int func,
> -size_t stolen_size)
> +static resource_size_t __init i845_stolen_base(int num, int slot, int func,
> +resource_size_t stolen_size)
>  {
> - return (phys_addr_t)i830_mem_size() - i845_tseg_size() - stolen_size;
> + return i830_mem_size() - i845_tseg_size() - stolen_size;
>  }
>  
> -static phys_addr_t __init i85x_stolen_base(int num, int slot, int func,
> -size_t stolen_size)
> +static resource_size_t __init i85x_stolen_base(int num, int s

Re: [Intel-gfx] [PATCH] drm/i915: Use copy_from_user() in fence copying

[Joonas Lahtinen]

On Wed, 2017-12-06 at 12:28 -0800, Kees Cook wrote:
> There's no good reason to separate the access_ok() from the copy,
> especially since the access_ok() size is hard-coded instead of using
> sizeof(). Instead, just use copy_from_user() directly.
> 
> Fixes: cf6e7bac6357 ("drm/i915: Add support for drm syncobjs")

There's been request to reduce the amount of Fixes: tags that are not
actually fixing bugs. This seems more like an optimization.

References: has been suggested for these cases instead.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: [Intel-gfx] linux-next: Signed-off-by missing for commit in the drm-intel-fixes tree

[Joonas Lahtinen]

+ GVT folks.

On Fri, 2017-12-08 at 09:15 +1100, Stephen Rothwell wrote:
> Hi all,
> 
> Commit
> 
>   365ad5df9caa ("drm/i915/gvt: Export
> intel_gvt_render_mmio_to_ring_id()")
> 
> is missing a Signed-off-by from its committer.
> 
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: [PATCH V3 09/29] drm/i915: deprecate pci_get_bus_and_slot()

[Joonas Lahtinen]

Hi,

I sent this individual i915 patch to our CI, and it is passing on all platforms:

https://patchwork.freedesktop.org/series/34822/

Is it ok if I merge this to drm-tip already?

Regards, Joonas

On Mon, 2017-11-27 at 13:50 -0500, Sinan Kaya wrote:
> +dri-de...@lists.freedesktop.org
> 
> On 11/27/2017 11:57 AM, Sinan Kaya wrote:
> > pci_get_bus_and_slot() is restrictive such that it assumes domain=0 as
> > where a PCI device is present. This restricts the device drivers to be
> > reused for other domain numbers.
> > 
> > Getting ready to remove pci_get_bus_and_slot() function in favor of
> > pci_get_domain_bus_and_slot().
> > 
> > Extract the domain number from drm_device and pass it into
> > pci_get_domain_bus_and_slot() function.
> > 
> > Signed-off-by: Sinan Kaya <ok...@codeaurora.org>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c | 5 -
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c 
> > b/drivers/gpu/drm/i915/i915_drv.c
> > index 9f45cfe..5a8cb79 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -419,7 +419,10 @@ static int i915_getparam(struct drm_device *dev, void 
> > *data,
> >  
> >  static int i915_get_bridge_dev(struct drm_i915_private *dev_priv)
> >  {
> > -   dev_priv->bridge_dev = pci_get_bus_and_slot(0, PCI_DEVFN(0, 0));
> > +   int domain = pci_domain_nr(dev_priv->drm.pdev->bus);
> > +
> > +   dev_priv->bridge_dev =
> > +   pci_get_domain_bus_and_slot(domain, 0, PCI_DEVFN(0, 0));
> > if (!dev_priv->bridge_dev) {
> > DRM_ERROR("bridge device not found\n");
> > return -1;
> > 
> 
> 
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: [RESEND PATCH 1/1] drm/i915/glk: Add MODULE_FIRMWARE for Geminilake

[Joonas Lahtinen]

Quoting Jani Nikula (2018-04-27 12:20:55)
> On Wed, 25 Apr 2018, Ian W MORRISON  wrote:
> > Can I ask if this is on anyone's radar as I'm concerned this patch will
> > stall otherwise?
> 
> Pushed to drm-intel-next-queued, thanks for the patch.
> 
> I opted to drop the Cc: stable for now. This doesn't mean it can't be
> backported later on, I'm just punting on that call right now to make
> some forward progress here.
> 
> Joonas, please do pick f6d3e06f0747 ("drm/i915/glk: Add MODULE_FIRMWARE
> for Geminilake") to drm-intel-fixes to queue it to v4.17.

Done. That's the only drm-intel-fixes patch there seems to be for -rc4.

Regards, Joonas

> 
> BR,
> Jani.
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

Re: [PATCH V3 09/29] drm/i915: deprecate pci_get_bus_and_slot()

[Joonas Lahtinen]

Quoting Jani Nikula (2018-02-19 11:34:34)
> On Fri, 16 Feb 2018, Bjorn Helgaas  wrote:
> > On Mon, Nov 27, 2017 at 11:57:46AM -0500, Sinan Kaya wrote:
> >> pci_get_bus_and_slot() is restrictive such that it assumes domain=0 as
> >> where a PCI device is present. This restricts the device drivers to be
> >> reused for other domain numbers.
> >> 
> >> Getting ready to remove pci_get_bus_and_slot() function in favor of
> >> pci_get_domain_bus_and_slot().
> >> 
> >> Extract the domain number from drm_device and pass it into
> >> pci_get_domain_bus_and_slot() function.
> >> 
> >> Signed-off-by: Sinan Kaya 
> >
> > I don't know what happened to this, and it didn't make it into
> > v4.16-rc1.  I applied it to pci/deprecate-get-bus-and-slot for v4.17
> > along with the patch that actually removes pci_get_bus_and_slot().
> 
> It fell between the cracks as we couldn't apply it before getting a
> backmerge on the dependency. Sorry about that.
> 
> Ack for merging through your tree.

I just retested the patch and it still passes CI. We also now have the
dependency in our tree through the backmerge, so I can send this for the
next drm-next pull request. Either way suits me.

Regards, Joonas

Re: [PATCH] drm/i915: drop various VLAs in i915_debugfs.c

[Joonas Lahtinen]

Quoting Salvatore Mesoraca (2018-03-13 21:51:28)
> Avoid 3 VLAs[1] by using real constant expressions instead of variables.
> The compiler should be able to optimize the original code and avoid using
> any actual VLAs. Anyway this change is useful because it will avoid a false
> positives with -Wvla, it might also help the compiler generating better
> code.
> 
> [1] https://lkml.org/lkml/2018/3/7/621
> 
> Signed-off-by: Salvatore Mesoraca 
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 26 --
>  1 file changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index e968aea..bf0a8e3 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -4259,19 +4259,20 @@ static ssize_t cur_wm_latency_write(struct file 
> *file, const char __user *ubuf,
> i915_cache_sharing_get, i915_cache_sharing_set,
> "%llu\n");
>  
> +#define CHERRYVIEW_SS_MAX 2

CHV_SS_MAX should be good enough. Make these function scoped (so #define
at the beginning and #undef at the end of function).

Do use ARRAY_SIZE() instead of repeating.

Regards, Joonas

RE: [RESEND PATCH 1/1] drm/i915/glk: Add MODULE_FIRMWARE for Geminilake

[Joonas Lahtinen]

Quoting Jani Nikula (2018-04-17 12:02:52)
> On Mon, 16 Apr 2018, "Srivatsa, Anusha"  wrote:
> >>-Original Message-
> >>From: Jani Nikula [mailto:jani.nik...@linux.intel.com]
> >>Sent: Wednesday, April 11, 2018 5:27 AM
> >>To: Ian W MORRISON 
> >>Cc: Vivi, Rodrigo ; Srivatsa, Anusha
> >>; Wajdeczko, Michal
> >>; Greg KH ;
> >>airl...@linux.ie; joonas.lahti...@linux.intel.com; 
> >>linux-kernel@vger.kernel.org;
> >>sta...@vger.kernel.org; intel-...@lists.freedesktop.org; dri-
> >>de...@lists.freedesktop.org
> >>Subject: Re: [RESEND PATCH 1/1] drm/i915/glk: Add MODULE_FIRMWARE for
> >>Geminilake
> >>
> >>On Wed, 11 Apr 2018, Ian W MORRISON  wrote:
> >>> 
> >>>
> 
>  NAK on indiscriminate Cc: stable. There are zero guarantees that
>  older kernels will work with whatever firmware you throw at them.
> 
> >>>
> >>> I included 'Cc: stable' so the patch would get added to the v4.16 and
> >>> v4.15 kernels which I have tested with the patch. I found that earlier
> >>> kernels didn't support the 'linux-firmware' package required to get
> >>> wifi working on Intel's new Gemini Lake NUC.
> >>
> >>You realize that this patch should have nothing to do with wifi?
> >>
> >>Rodrigo, Anusha, if you think Cc: stable is appropriate, please indicate 
> >>the specific
> >>versions of stable it is appropriate for.
> >
> > Hi Jani,
> >
> > The stable kernel version is 4.12 and beyond.
> > It is appropriate to add the CC: stable in my opinion
> 
> Who tested the firmware with v4.12 and later? We only have the CI
> results against *current* drm-tip. We don't even know about v4.16.
> 
> I'm not going to ack and take responsibility for the stable backports
> unless someone actually comes forward with credible Tested-bys.

And even then, some distros will be surprised of the new MODULE_FIRMWARE
and will need to update the linux-firmware package, too.

Regards, Joonas

> 
> BR,
> Jani.
> 
> 
> >
> > Anusha
> >>BR,
> >>Jani.
> >>
> >>>
> 
>  PS. How is this a "RESEND"? I haven't seen this before.
> 
> >>>
> >>> It is a 'RESEND' for that very reason. I initially sent the patch to
> >>> the same people as a similar patch
> >>> (https://patchwork.kernel.org/patch/10143637/) however after realising
> >>> this omitted required addresses I added them and resent the patch.
> >>>
> >>> Best regards,
> >>> Ian
> >>
> >>--
> >>Jani Nikula, Intel Open Source Technology Center
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

Re: [Intel-gfx] linux-next: manual merge of the drm-intel tree with Linus' tree

[Joonas Lahtinen]

Quoting Stephen Rothwell (2018-03-23 02:50:18)
> Hi all,
> 
> On Thu, 22 Mar 2018 13:21:29 +1100 Stephen Rothwell  
> wrote:
> >
> > Today's linux-next merge of the drm-intel tree got a conflict in:
> > 
> >   drivers/gpu/drm/i915/gvt/scheduler.c
> > 
> > between commit:
> > 
> >   fa3dd623e559 ("drm/i915/gvt: keep oa config in shadow ctx")
> > 
> > from Linus' tree and commit:
> > 
> >   b20c0d5ce104 ("drm/i915/gvt: Update PDPs after a vGPU mm object is 
> > pinned.")
> > 
> > from the drm-intel tree.
> > 
> > I fixed it up (see below) and can carry the fix as necessary. This
> > is now fixed as far as linux-next is concerned, but any non trivial
> > conflicts should be mentioned to your upstream maintainer when your tree
> > is submitted for merging.  You may also want to consider cooperating
> > with the maintainer of the conflicting tree to minimise any particularly
> > complex conflicts.

Hi Stephen,

Thanks for solving this, the resolution is correct.

You may want to replace Daniel, as the recipient here, with the current
i915 maintainers to get a faster feedback next time :)



> This is now a conflict between the drm tree and Linus' tree.
> 

My bad for not highlighting the merge conflict in my PR to Dave. He
probably did not notice, getting the resolution automatically from
drm-rerere, I'd guess. I've noted it in the ever improving draft of
things to remember with the PRs.

I'm very much currently flying based on what the previous PR authors
have remembered to tell me. But after 4.17, the cycle is complete and we
all "have been there, done that", and you can expect less of a turbulence.

(We'll probably have more magnificent documentation, too.)

Regards, Joonas

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

Re: [PATCH] drm/i915: avoid rebuilding i915_gpu_error.o on version string updates

[Joonas Lahtinen]

Quoting Hans Holmberg (2018-11-21 13:35:19)
> On Wed, Nov 21, 2018 at 11:10 AM Joonas Lahtinen
>  wrote:
> >
> > Quoting Hans Holmberg (2018-11-21 11:54:23)
> > > From: Hans Holmberg 
> > >
> > > There is no need to rebuild i915_gpu_error.o when the version string
> > > changes as the version is available in init_utsname()->release.
> > >
> > > Signed-off-by: Hans Holmberg 
> >
> > Seems reasonable to me.
> >
> > Reviewed-by: Joonas Lahtinen 
> >
> > Out of curiosity, are you by any chance hashing the i915_gpu_error.o
> > file (or the contents elsewhere) for some purpose?
> 
> Oh no, I was just moderately annoyed by the file being rebuilt every
> time the version was updated(I use my current branch name as
> LOCALVERSION when building).

That's a reasonable explanation, too :)

I unblocked the message from moderation queue so that our CI picks this
up for testing. I will then proceed to merge this once the results are
back.

Thanks for the patch!

Regards, Joonas

> 
> Thanks,
> Hans
> >
> > Regards, Joonas
> >
> > > ---
> > >  drivers/gpu/drm/i915/i915_gpu_error.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c 
> > > b/drivers/gpu/drm/i915/i915_gpu_error.c
> > > index 8762d17b6659..958e1484a3dd 100644
> > > --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> > > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> > > @@ -27,7 +27,7 @@
> > >   *
> > >   */
> > >
> > > -#include 
> > > +#include 
> > >  #include 
> > >  #include 
> > >  #include 
> > > @@ -650,7 +650,7 @@ int i915_error_state_to_str(struct 
> > > drm_i915_error_state_buf *m,
> > >
> > > if (*error->error_msg)
> > > err_printf(m, "%s\n", error->error_msg);
> > > -   err_printf(m, "Kernel: " UTS_RELEASE "\n");
> > > +   err_printf(m, "Kernel: %s\n", init_utsname()->release);
> > > ts = ktime_to_timespec64(error->time);
> > > err_printf(m, "Time: %lld s %ld us\n",
> > >(s64)ts.tv_sec, ts.tv_nsec / NSEC_PER_USEC);
> > > --
> > > 2.17.1
> > >

Re: [PATCH] drm/i915: drop various VLAs in i915_debugfs.c

[Joonas Lahtinen]

Quoting Salvatore Mesoraca (2018-03-13 21:51:28)
> Avoid 3 VLAs[1] by using real constant expressions instead of variables.
> The compiler should be able to optimize the original code and avoid using
> any actual VLAs. Anyway this change is useful because it will avoid a false
> positives with -Wvla, it might also help the compiler generating better
> code.
> 
> [1] https://lkml.org/lkml/2018/3/7/621
> 
> Signed-off-by: Salvatore Mesoraca 
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 26 --
>  1 file changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index e968aea..bf0a8e3 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -4259,19 +4259,20 @@ static ssize_t cur_wm_latency_write(struct file 
> *file, const char __user *ubuf,
> i915_cache_sharing_get, i915_cache_sharing_set,
> "%llu\n");
>  
> +#define CHERRYVIEW_SS_MAX 2

CHV_SS_MAX should be good enough. Make these function scoped (so #define
at the beginning and #undef at the end of function).

Do use ARRAY_SIZE() instead of repeating.

Regards, Joonas

Re: [Intel-gfx] linux-next: manual merge of the drm-intel tree with Linus' tree

[Joonas Lahtinen]

Quoting Stephen Rothwell (2018-03-23 02:50:18)
> Hi all,
> 
> On Thu, 22 Mar 2018 13:21:29 +1100 Stephen Rothwell  
> wrote:
> >
> > Today's linux-next merge of the drm-intel tree got a conflict in:
> > 
> >   drivers/gpu/drm/i915/gvt/scheduler.c
> > 
> > between commit:
> > 
> >   fa3dd623e559 ("drm/i915/gvt: keep oa config in shadow ctx")
> > 
> > from Linus' tree and commit:
> > 
> >   b20c0d5ce104 ("drm/i915/gvt: Update PDPs after a vGPU mm object is 
> > pinned.")
> > 
> > from the drm-intel tree.
> > 
> > I fixed it up (see below) and can carry the fix as necessary. This
> > is now fixed as far as linux-next is concerned, but any non trivial
> > conflicts should be mentioned to your upstream maintainer when your tree
> > is submitted for merging.  You may also want to consider cooperating
> > with the maintainer of the conflicting tree to minimise any particularly
> > complex conflicts.

Hi Stephen,

Thanks for solving this, the resolution is correct.

You may want to replace Daniel, as the recipient here, with the current
i915 maintainers to get a faster feedback next time :)



> This is now a conflict between the drm tree and Linus' tree.
> 

My bad for not highlighting the merge conflict in my PR to Dave. He
probably did not notice, getting the resolution automatically from
drm-rerere, I'd guess. I've noted it in the ever improving draft of
things to remember with the PRs.

I'm very much currently flying based on what the previous PR authors
have remembered to tell me. But after 4.17, the cycle is complete and we
all "have been there, done that", and you can expect less of a turbulence.

(We'll probably have more magnificent documentation, too.)

Regards, Joonas

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

Re: [RESEND PATCH 1/1] drm/i915/glk: Add MODULE_FIRMWARE for Geminilake

[Joonas Lahtinen]

Quoting Jani Nikula (2018-04-27 12:20:55)
> On Wed, 25 Apr 2018, Ian W MORRISON  wrote:
> > Can I ask if this is on anyone's radar as I'm concerned this patch will
> > stall otherwise?
> 
> Pushed to drm-intel-next-queued, thanks for the patch.
> 
> I opted to drop the Cc: stable for now. This doesn't mean it can't be
> backported later on, I'm just punting on that call right now to make
> some forward progress here.
> 
> Joonas, please do pick f6d3e06f0747 ("drm/i915/glk: Add MODULE_FIRMWARE
> for Geminilake") to drm-intel-fixes to queue it to v4.17.

Done. That's the only drm-intel-fixes patch there seems to be for -rc4.

Regards, Joonas

> 
> BR,
> Jani.
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

Re: [PATCH] drm/i915: Convert timers to use timer_setup()

[Joonas Lahtinen]

On Mon, 2017-10-16 at 15:55 -0700, Kees Cook wrote:
> In preparation for unconditionally passing the struct timer_list pointer to
> all timer callbacks, switch to using the new timer_setup() and from_timer()
> to pass the timer pointer explicitly.
> 
> Cc: Daniel Vetter 
> Cc: Jani Nikula 
> Cc: David Airlie 
> Cc: Chris Wilson 
> Cc: Joonas Lahtinen 
> Cc: Tvrtko Ursulin 
> Cc: Oscar Mateo 
> Cc: intel-...@lists.freedesktop.org
> Cc: dri-de...@lists.freedesktop.org
> Signed-off-by: Kees Cook 
> Reviewed-by: Joonas Lahtinen  # for 
> mock_engine

Reviewed-by: Joonas Lahtinen 

For some reason our CI didn't pick this up even though it was correctly
sent to the mailing list, so I will re-send with my refreshed R-b for
our CI and then we can merge this.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: [PATCH] drm/i915: Convert timers to use timer_setup()

[Joonas Lahtinen]

On Wed, 2017-10-04 at 17:54 -0700, Kees Cook wrote:
> In preparation for unconditionally passing the struct timer_list pointer to
> all timer callbacks, switch to using the new timer_setup() and from_timer()
> to pass the timer pointer explicitly.
> 
> Cc: Daniel Vetter 
> Cc: Jani Nikula 
> Cc: David Airlie 
> Cc: Chris Wilson 
> Cc: Joonas Lahtinen 
> Cc: Tvrtko Ursulin 
> Cc: Oscar Mateo 
> Cc: intel-...@lists.freedesktop.org
> Cc: dri-de...@lists.freedesktop.org
> Cc: Thomas Gleixner 
> Signed-off-by: Kees Cook 



> @@ -32,9 +32,9 @@ static struct mock_request *first_request(struct 
> mock_engine *engine)
>   link);
>  }
>  
> -static void hw_delay_complete(unsigned long data)
> +static void hw_delay_complete(struct timer_list *t)
>  {
> - struct mock_engine *engine = (typeof(engine))data;
> + struct mock_engine *engine = from_timer(engine, t, hw_delay);

The order is bit strange to me, it's not same as with container_of, but
I guess GCC will complain for getting it wrong. It's also slightly
different doing the typeof for you, so I guess it makes sense, so:

Reviewed-by: Joonas Lahtinen 

Do you expect for us to merge or are you looking to merge all timer
changes from single tree?

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly

[Joonas Lahtinen]

On Thu, 2017-09-21 at 06:44 +0800, Zhenyu Wang wrote:
> On 2017.09.19 19:35:23 -0700, Joe Perches wrote:
> > On Wed, 2017-09-20 at 05:46 +0800, Zhenyu Wang wrote:
> > > On 2017.09.19 16:55:34 +0100, Colin King wrote:
> > > > From: Colin Ian King 
> > > > 
> > > > An earlier fix changed the return type from find_bb_size however the
> > > > integer return is being assigned to a unsigned int so the -ve error
> > > > check will never be detected. Make bb_size an int to fix this.
> > > > 
> > > > Detected by CoverityScan CID#1456886 ("Unsigned compared against 0")
> > > > 
> > > > Fixes: 1e3197d6ad73 ("drm/i915/gvt: Refine error handling for 
> > > > perform_bb_shadow")
> > > > Signed-off-by: Colin Ian King 
> > > > ---
> > > >  drivers/gpu/drm/i915/gvt/cmd_parser.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/gvt/cmd_parser.c 
> > > > b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > > index 2c0ccbb817dc..f41cbf664b69 100644
> > > > --- a/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > > +++ b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > > @@ -1628,7 +1628,7 @@ static int perform_bb_shadow(struct 
> > > > parser_exec_state *s)
> > > > struct intel_shadow_bb_entry *entry_obj;
> > > > struct intel_vgpu *vgpu = s->vgpu;
> > > > unsigned long gma = 0;
> > > > -   uint32_t bb_size;
> > > > +   int bb_size;
> > > > void *dst = NULL;
> > > > int ret = 0;
> > > >  
> > > 
> > > Applied this, thanks!
> > 
> > Is it possible for bb_size to be both >= 2g and valid?
> 
> Never be possible in practise and if really that big I think something
> is already insane indeed.

It's good idea to document these assumptions as WARN_ON's. In i915, if
the value is completely internal to kernel, we're using GEM_BUG_ON for
these so that our CI will notice breakage. If it's not a driver
internal value only, a WARN_ON is the appropriate action.

Otherwise the information is lost and the next person reading the code
will have the same question in mind.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly

[Joonas Lahtinen]

On Thu, 2017-09-21 at 16:17 +, Wang, Zhi A wrote:
> Hi Joonas:
> 
> Thanks for the introduction. I have been thinking about the
> possibility of introducing GEM_BUG_ON into GVT-g recently and
> investigating on it. I'm just a bit confused about the usage between
> GEM_BUG_ON and WARN_ON.

GEM_BUG_ON is basically there to catch things that we do not expect
ever to happen within the driver. So we often list the function
preconditions as GEM_BUG_ON. It's there for the same reason as the
lockdep_assert_held and KASAN. It's sometimes heavy checks that we
really want to run when functionally validating kernel.

GEM_BUG_ON became to existence because adding checks for obvious
conditions at the critical command submission path GEM is not
sustainable for performance in production.

The expectation is that each GEM_BUG_ON has a testcase in I-G-T that
has the potential to hit it if driver was modified not to respect those
preconditions. So once our testest passes, we can disable the
GEM_BUG_ONs and be confident of the internal driver quality and get the
release performance.

WARN_ON is mostly used for the cases when the hardware is behaving
differently than we expect. We can't remove them as we don't have all
the hardware in the world to test, but we try to exercise them too
through I-G-Ts. The test will often be the subtest that was written to
reproduce the problem with our expectations of hardware in case of
hangs and other bugs. After we've corrected the driver behaviour, or
got a hardware W/A assigned, we keep the test and add a WARN_ON to make
sure there will be no regression back to the same situation.

This is at least what should happen, given time constraints, there may
be variations.

User behaving unexpectedly should never result in WARN_ON (or even
worse, BUG_ON), should always just be debug messages displayed (not to
trigger the CI) and errors propagated back to user:

https://01.org/linuxgraphics/gfx-docs/drm/gpu/drm-uapi.html#recommended
-ioctl-return-values

Bare BUG_ON should only be used when there's the danger of corrupting
system memory or filesystems, so from graphics driver, that's not very
often. Controlled propagation of errors and maybe WARN_ON is always
preferred if possible.


> GEM_BUG_ON is only enabled when kernel debug is enabled, which mostly
> is disabled in a production kernel. In the case of i915, I'm sure it
> will be enabled in CI test so that it can catch broken code path.
> Looking into GVT-g, the similar scenario is we enable it in QA test.
> 
> Let's say GEM_BUG_ON can do its work very well in QA test but QA test
> is not fully covered all the condition, then something might be still
> broken when it comes to the production kernel for user and GEM_BUG_ON
> will be disabled and will not catch that, I guess.
> 
> That's my confusion which scratched my mind during the investigation:
> If GEM_BUG_ON is not always working, then it looks WARN_ON should
> always be used Expected to learn more about the story behind. :)

So if the saying is some object is "never going to be bigger than 2G",
there should be either:

1. GEM_BUG_ON like assertion for it and a test that tries to hit it, by
trying to allocate a huge object for example, and should get rejection
as -EINVAL

2. Test to see if the object is bigger, and propagate back the error if
it is. Either resulting in user reported error if the origin of the
object is outside of kernel <-> hardware. Or a WARN_ON if it's strange
hardware or kernel driver behavior.

You should choose depending on how often your function gets called, and
how critical the execution time is.

Hopefully this clarified things.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

RE: [RESEND PATCH 1/1] drm/i915/glk: Add MODULE_FIRMWARE for Geminilake

[Joonas Lahtinen]

Quoting Jani Nikula (2018-04-17 12:02:52)
> On Mon, 16 Apr 2018, "Srivatsa, Anusha"  wrote:
> >>-Original Message-
> >>From: Jani Nikula [mailto:jani.nik...@linux.intel.com]
> >>Sent: Wednesday, April 11, 2018 5:27 AM
> >>To: Ian W MORRISON 
> >>Cc: Vivi, Rodrigo ; Srivatsa, Anusha
> >>; Wajdeczko, Michal
> >>; Greg KH ;
> >>airl...@linux.ie; joonas.lahti...@linux.intel.com; 
> >>linux-kernel@vger.kernel.org;
> >>sta...@vger.kernel.org; intel-...@lists.freedesktop.org; dri-
> >>de...@lists.freedesktop.org
> >>Subject: Re: [RESEND PATCH 1/1] drm/i915/glk: Add MODULE_FIRMWARE for
> >>Geminilake
> >>
> >>On Wed, 11 Apr 2018, Ian W MORRISON  wrote:
> >>> 
> >>>
> 
>  NAK on indiscriminate Cc: stable. There are zero guarantees that
>  older kernels will work with whatever firmware you throw at them.
> 
> >>>
> >>> I included 'Cc: stable' so the patch would get added to the v4.16 and
> >>> v4.15 kernels which I have tested with the patch. I found that earlier
> >>> kernels didn't support the 'linux-firmware' package required to get
> >>> wifi working on Intel's new Gemini Lake NUC.
> >>
> >>You realize that this patch should have nothing to do with wifi?
> >>
> >>Rodrigo, Anusha, if you think Cc: stable is appropriate, please indicate 
> >>the specific
> >>versions of stable it is appropriate for.
> >
> > Hi Jani,
> >
> > The stable kernel version is 4.12 and beyond.
> > It is appropriate to add the CC: stable in my opinion
> 
> Who tested the firmware with v4.12 and later? We only have the CI
> results against *current* drm-tip. We don't even know about v4.16.
> 
> I'm not going to ack and take responsibility for the stable backports
> unless someone actually comes forward with credible Tested-bys.

And even then, some distros will be surprised of the new MODULE_FIRMWARE
and will need to update the linux-firmware package, too.

Regards, Joonas

> 
> BR,
> Jani.
> 
> 
> >
> > Anusha
> >>BR,
> >>Jani.
> >>
> >>>
> 
>  PS. How is this a "RESEND"? I haven't seen this before.
> 
> >>>
> >>> It is a 'RESEND' for that very reason. I initially sent the patch to
> >>> the same people as a similar patch
> >>> (https://patchwork.kernel.org/patch/10143637/) however after realising
> >>> this omitted required addresses I added them and resent the patch.
> >>>
> >>> Best regards,
> >>> Ian
> >>
> >>--
> >>Jani Nikula, Intel Open Source Technology Center
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

Re: [PATCH V3 09/29] drm/i915: deprecate pci_get_bus_and_slot()

[Joonas Lahtinen]

Quoting Jani Nikula (2018-02-19 11:34:34)
> On Fri, 16 Feb 2018, Bjorn Helgaas  wrote:
> > On Mon, Nov 27, 2017 at 11:57:46AM -0500, Sinan Kaya wrote:
> >> pci_get_bus_and_slot() is restrictive such that it assumes domain=0 as
> >> where a PCI device is present. This restricts the device drivers to be
> >> reused for other domain numbers.
> >> 
> >> Getting ready to remove pci_get_bus_and_slot() function in favor of
> >> pci_get_domain_bus_and_slot().
> >> 
> >> Extract the domain number from drm_device and pass it into
> >> pci_get_domain_bus_and_slot() function.
> >> 
> >> Signed-off-by: Sinan Kaya 
> >
> > I don't know what happened to this, and it didn't make it into
> > v4.16-rc1.  I applied it to pci/deprecate-get-bus-and-slot for v4.17
> > along with the patch that actually removes pci_get_bus_and_slot().
> 
> It fell between the cracks as we couldn't apply it before getting a
> backmerge on the dependency. Sorry about that.
> 
> Ack for merging through your tree.

I just retested the patch and it still passes CI. We also now have the
dependency in our tree through the backmerge, so I can send this for the
next drm-next pull request. Either way suits me.

Regards, Joonas

Re: [PATCH] drm/i915: Increase max texture to 16k for gen9+

[Joonas Lahtinen]

+ Ville as Jani is OoO

On Thu, 2017-12-07 at 17:26 +0800, Alex Tu wrote:
> Rrefer to another patch [1] on mesa to extend width/height to 16384.
> For issue :
>  - https://bugs.freedesktop.org/show_bug.cgi?id=102508
>  - LP: #1714178 Triple monitor display failed with Dell Dock (HiDPI)
> 
> [1] https://patchwork.freedesktop.org/patch/124918/
> 
> Signed-off-by: Alex Tu 
> ---
>  drivers/gpu/drm/i915/intel_display.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index 47a2f6acee50..556fa57b18b8 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13905,7 +13905,7 @@ u32 intel_fb_pitch_limit(struct drm_i915_private 
> *dev_priv,
>   /* "The stride in bytes must not exceed the of the size of 8K
>*  pixels and 32K bytes."
>*/
> - return min(8192 * cpp, 32768);
> + return min(16384 * cpp, 65536);
>   } else if (gen >= 5 && !HAS_GMCH_DISPLAY(dev_priv)) {
>   return 32*1024;
>   } else if (gen >= 4) {
> @@ -14604,8 +14604,8 @@ int intel_modeset_init(struct drm_device *dev)
>   dev->mode_config.max_width = 4096;
>   dev->mode_config.max_height = 4096;
>   } else {
> - dev->mode_config.max_width = 8192;
> - dev->mode_config.max_height = 8192;
> + dev->mode_config.max_width = 16384;
> + dev->mode_config.max_height = 16384;
>   }
>  
>   if (IS_I845G(dev_priv) || IS_I865G(dev_priv)) {
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: [PATCH] drm/i915: Mark expected switch fall-throughs

[Joonas Lahtinen]

On Mon, 2017-11-27 at 16:17 -0600, Gustavo A. R. Silva wrote:
> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> where we are expecting to fall through.

I have to say I'm totally not sold on regexps matching comment
contents. Was something more explicit ever considered? Like:

#define FALLTHROUGH __attribute__((fallthrough));

With the appropriate version checks, of course.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: [Intel-gfx] [PATCH] drm/i915: Use copy_from_user() in fence copying

[Joonas Lahtinen]

On Wed, 2017-12-06 at 12:28 -0800, Kees Cook wrote:
> There's no good reason to separate the access_ok() from the copy,
> especially since the access_ok() size is hard-coded instead of using
> sizeof(). Instead, just use copy_from_user() directly.
> 
> Fixes: cf6e7bac6357 ("drm/i915: Add support for drm syncobjs")

There's been request to reduce the amount of Fixes: tags that are not
actually fixing bugs. This seems more like an optimization.

References: has been suggested for these cases instead.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: [Intel-gfx] linux-next: Signed-off-by missing for commit in the drm-intel-fixes tree

[Joonas Lahtinen]

+ GVT folks.

On Fri, 2017-12-08 at 09:15 +1100, Stephen Rothwell wrote:
> Hi all,
> 
> Commit
> 
>   365ad5df9caa ("drm/i915/gvt: Export
> intel_gvt_render_mmio_to_ring_id()")
> 
> is missing a Signed-off-by from its committer.
> 
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: [PATCH 1/8] x86/early-quirks: Extend Intel graphics stolen memory placement to 64bit

[Joonas Lahtinen]

Hi Ingo & Thomas,

Now would be a great moment to slap the final Acked-bys (first two
patches of this series) as the comments have been addressed and
Reviewed-by was refreshed by Chris. I consider the series ready to be
merged in this state.

Once acked, I will then proceed to merge these through the drm-tip tree
as previously discussed.

Regards, Joonas

On Mon, 2017-12-11 at 12:14 +, Matthew Auld wrote:
> From: Joonas Lahtinen 
> 
> To give upcoming SKU BIOSes more flexibility in placing the Intel
> graphics stolen memory, make all variables storing the placement or size
> compatible with full 64 bit range. Also by exporting the stolen region
> as a resource, we can then nuke the duplicated stolen discovery in i915.
> 
> Signed-off-by: Joonas Lahtinen 
> Signed-off-by: Matthew Auld 
> Cc: Joonas Lahtinen 
> Cc: Ville Syrjälä 
> Cc: Chris Wilson 
> Cc: Paulo Zanoni 
> Cc: Thomas Gleixner 
> Cc: Ingo Molnar 
> Cc: H. Peter Anvin 
> Cc: x...@kernel.org
> Cc: linux-kernel@vger.kernel.org
> Reviewed-by: Chris Wilson  #v3
> ---
> 
> v2: export the stolen region as a resource
> fix u16 << 16 (Chris)
> v3: actually fix u16 << 16
> v4: mark intel_graphics_stolen_res as __ro_after_init (Thomas)
> 
>  arch/x86/kernel/early-quirks.c | 86 
> +++---
>  include/drm/i915_drm.h |  3 ++
>  2 files changed, 50 insertions(+), 39 deletions(-)
> 
> diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
> index 1e82f787c160..6c1624889011 100644
> --- a/arch/x86/kernel/early-quirks.c
> +++ b/arch/x86/kernel/early-quirks.c
> @@ -243,7 +243,7 @@ static void __init intel_remapping_check(int num, int 
> slot, int func)
>  #define KB(x)((x) * 1024UL)
>  #define MB(x)(KB (KB (x)))
>  
> -static size_t __init i830_tseg_size(void)
> +static resource_size_t __init i830_tseg_size(void)
>  {
>   u8 esmramc = read_pci_config_byte(0, 0, 0, I830_ESMRAMC);
>  
> @@ -256,7 +256,7 @@ static size_t __init i830_tseg_size(void)
>   return KB(512);
>  }
>  
> -static size_t __init i845_tseg_size(void)
> +static resource_size_t __init i845_tseg_size(void)
>  {
>   u8 esmramc = read_pci_config_byte(0, 0, 0, I845_ESMRAMC);
>   u8 tseg_size = esmramc & I845_TSEG_SIZE_MASK;
> @@ -273,7 +273,7 @@ static size_t __init i845_tseg_size(void)
>   return 0;
>  }
>  
> -static size_t __init i85x_tseg_size(void)
> +static resource_size_t __init i85x_tseg_size(void)
>  {
>   u8 esmramc = read_pci_config_byte(0, 0, 0, I85X_ESMRAMC);
>  
> @@ -283,12 +283,12 @@ static size_t __init i85x_tseg_size(void)
>   return MB(1);
>  }
>  
> -static size_t __init i830_mem_size(void)
> +static resource_size_t __init i830_mem_size(void)
>  {
>   return read_pci_config_byte(0, 0, 0, I830_DRB3) * MB(32);
>  }
>  
> -static size_t __init i85x_mem_size(void)
> +static resource_size_t __init i85x_mem_size(void)
>  {
>   return read_pci_config_byte(0, 0, 1, I85X_DRB3) * MB(32);
>  }
> @@ -297,36 +297,36 @@ static size_t __init i85x_mem_size(void)
>   * On 830/845/85x the stolen memory base isn't available in any
>   * register. We need to calculate it as TOM-TSEG_SIZE-stolen_size.
>   */
> -static phys_addr_t __init i830_stolen_base(int num, int slot, int func,
> -size_t stolen_size)
> +static resource_size_t __init i830_stolen_base(int num, int slot, int func,
> +resource_size_t stolen_size)
>  {
> - return (phys_addr_t)i830_mem_size() - i830_tseg_size() - stolen_size;
> + return i830_mem_size() - i830_tseg_size() - stolen_size;
>  }
>  
> -static phys_addr_t __init i845_stolen_base(int num, int slot, int func,
> -size_t stolen_size)
> +static resource_size_t __init i845_stolen_base(int num, int slot, int func,
> +resource_size_t stolen_size)
>  {
> - return (phys_addr_t)i830_mem_size() - i845_tseg_size() - stolen_size;
> + return i830_mem_size() - i845_tseg_size() - stolen_size;
>  }
>  
> -static phys_addr_t __init i85x_stolen_base(int num, int slot, int func,
> -size_t stolen_size)
> +static resource_size_t __init i85x_stolen_base(int num, int slot, int func,
> +resource_size_t stolen_size)
>  {
> - return (phys_addr_t)i85x_mem_size() - i85x_tseg_size() - stolen_size;
> + return i85x_mem_size() - i85x_tseg_size() - stolen_size;
>  }
>  
> -static phys_addr_t __init i865_stolen_base(int num, int slot, int func,
> - 

Re: [PATCH v15 6/7] drm/i915: Introduce GEM proxy

[Joonas Lahtinen]

On Tue, 2017-10-10 at 17:50 +0800, Tina Zhang wrote:
> GEM proxy is a kind of GEM, whose backing physical memory is pinned
> and produced by guest VM and is used by host as read only. With GEM
> proxy, host is able to access guest physical memory through GEM object
> interface. As GEM proxy is such a special kind of GEM, a new flag
> I915_GEM_OBJECT_IS_PROXY is introduced to ban host from changing the
> backing storage of GEM proxy.
> 
> v14:
> - return -ENXIO when gem proxy object is banned by ioctl.
>   (Chris) (Daniel)
> 
> v13:
> - add comments to GEM proxy. (Chris)
> - don't ban GEM proxy in i915_gem_sw_finish_ioctl. (Chris)
> - check GEM proxy bar after finishing i915_gem_object_wait. (Chris)
> - remove GEM proxy bar in i915_gem_madvise_ioctl.
> 
> v6:
> - add gem proxy barrier in the following ioctls. (Chris)
>   i915_gem_set_caching_ioctl
>   i915_gem_set_domain_ioctl
>   i915_gem_sw_finish_ioctl
>   i915_gem_set_tiling_ioctl
>   i915_gem_madvise_ioctl
> 
> Signed-off-by: Tina Zhang 
> Cc: Daniel Vetter 
> Cc: Chris Wilson 
> Cc: Joonas Lahtinen 



> +++ b/drivers/gpu/drm/i915/i915_gem_object.h
> @@ -39,6 +39,7 @@ struct drm_i915_gem_object_ops {
> unsigned int flags;
>  #define I915_GEM_OBJECT_HAS_STRUCT_PAGE BIT(0)
>  #define I915_GEM_OBJECT_IS_SHRINKABLE   BIT(1)
> +#define I915_GEM_OBJECT_IS_PROXY   BIT(2)

Please fix the indent to match. Do convert the above two lines to use
TAB character too.



> @@ -1690,7 +1704,7 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data,
>*/
>   if (!obj->base.filp) {
>   i915_gem_object_put(obj);
> - return -EINVAL;
> +     return -ENXIO;
>   }

This still needs to be a separate patch.

With those fixes, this is;

Reviewed-by: Joonas Lahtinen 

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: [Intel-gfx] [PATCH v15 6/7] drm/i915: Introduce GEM proxy

[Joonas Lahtinen]

On Tue, 2017-10-10 at 13:58 +0300, Joonas Lahtinen wrote:
> On Tue, 2017-10-10 at 17:50 +0800, Tina Zhang wrote:
> > GEM proxy is a kind of GEM, whose backing physical memory is pinned
> > and produced by guest VM and is used by host as read only. With GEM
> > proxy, host is able to access guest physical memory through GEM object
> > interface. As GEM proxy is such a special kind of GEM, a new flag
> > I915_GEM_OBJECT_IS_PROXY is introduced to ban host from changing the
> > backing storage of GEM proxy.
> > 
> > v14:
> > - return -ENXIO when gem proxy object is banned by ioctl.
> >   (Chris) (Daniel)
> > 
> > v13:
> > - add comments to GEM proxy. (Chris)
> > - don't ban GEM proxy in i915_gem_sw_finish_ioctl. (Chris)
> > - check GEM proxy bar after finishing i915_gem_object_wait. (Chris)
> > - remove GEM proxy bar in i915_gem_madvise_ioctl.
> > 
> > v6:
> > - add gem proxy barrier in the following ioctls. (Chris)
> >   i915_gem_set_caching_ioctl
> >   i915_gem_set_domain_ioctl
> >   i915_gem_sw_finish_ioctl
> >   i915_gem_set_tiling_ioctl
> >   i915_gem_madvise_ioctl
> > 
> > Signed-off-by: Tina Zhang 
> > Cc: Daniel Vetter 
> > Cc: Chris Wilson 
> > Cc: Joonas Lahtinen 
> 
> 
> 
> > +++ b/drivers/gpu/drm/i915/i915_gem_object.h
> > @@ -39,6 +39,7 @@ struct drm_i915_gem_object_ops {
> > unsigned int flags;
> >  #define I915_GEM_OBJECT_HAS_STRUCT_PAGE BIT(0)
> >  #define I915_GEM_OBJECT_IS_SHRINKABLE   BIT(1)
> > +#define I915_GEM_OBJECT_IS_PROXY   BIT(2)
> 
> Please fix the indent to match. Do convert the above two lines to use
> TAB character too.
> 
> 
> 
> > @@ -1690,7 +1704,7 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void 
> > *data,
> >  */
> > if (!obj->base.filp) {
> > i915_gem_object_put(obj);
> > -   return -EINVAL;
> > +   return -ENXIO;
> > }
> 
> This still needs to be a separate patch.
> 
> With those fixes, this is;
> 
> Reviewed-by: Joonas Lahtinen 

Also, send the produced two patches (this and the split patch) as a
standalone series for easier testing and merging.

CI seems to have hard time applying the whole series.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: [PATCH] drm/i915: remove redundant check on has_aliasing_ppgtt

[Joonas Lahtinen]

On Tue, 2017-10-10 at 14:47 +0100, Colin King wrote:
> From: Colin Ian King 
> 
> There is a previous check to on has_aliasing_ppgtt that returns
> 0 if it is false, so it is impossible for has_aliasing_ppgtt to
> be false on the final return of function intel_sanitize_enable_ppgtt,
> so final return in the function always will return 1.  Hence the
> redundant ternary operator can be replaced with a return 1.
> 
> Detected by CoverityScan, CID#1357136 ("Logically dead code")
> 
> Signed-off-by: Colin Ian King 

Thanks, I took it a few steps further and removed the variable
altogether. I Cc'd you on the patch.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: [PATCH V3 09/29] drm/i915: deprecate pci_get_bus_and_slot()

[Joonas Lahtinen]

Hi,

I sent this individual i915 patch to our CI, and it is passing on all platforms:

https://patchwork.freedesktop.org/series/34822/

Is it ok if I merge this to drm-tip already?

Regards, Joonas

On Mon, 2017-11-27 at 13:50 -0500, Sinan Kaya wrote:
> +dri-de...@lists.freedesktop.org
> 
> On 11/27/2017 11:57 AM, Sinan Kaya wrote:
> > pci_get_bus_and_slot() is restrictive such that it assumes domain=0 as
> > where a PCI device is present. This restricts the device drivers to be
> > reused for other domain numbers.
> > 
> > Getting ready to remove pci_get_bus_and_slot() function in favor of
> > pci_get_domain_bus_and_slot().
> > 
> > Extract the domain number from drm_device and pass it into
> > pci_get_domain_bus_and_slot() function.
> > 
> > Signed-off-by: Sinan Kaya 
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c | 5 -
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c 
> > b/drivers/gpu/drm/i915/i915_drv.c
> > index 9f45cfe..5a8cb79 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -419,7 +419,10 @@ static int i915_getparam(struct drm_device *dev, void 
> > *data,
> >  
> >  static int i915_get_bridge_dev(struct drm_i915_private *dev_priv)
> >  {
> > -   dev_priv->bridge_dev = pci_get_bus_and_slot(0, PCI_DEVFN(0, 0));
> > +   int domain = pci_domain_nr(dev_priv->drm.pdev->bus);
> > +
> > +   dev_priv->bridge_dev =
> > +       pci_get_domain_bus_and_slot(domain, 0, PCI_DEVFN(0, 0));
> > if (!dev_priv->bridge_dev) {
> > DRM_ERROR("bridge device not found\n");
> > return -1;
> > 
> 
> 
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: [PATCH V3 09/29] drm/i915: deprecate pci_get_bus_and_slot()

[Joonas Lahtinen]

On Tue, 2017-12-12 at 19:07 -0500, Sinan Kaya wrote:
> On 12/12/2017 9:04 AM, Joonas Lahtinen wrote:
> > Hi,
> > 
> > I sent this individual i915 patch to our CI, and it is passing on
> > all platforms:
> > 
> > https://patchwork.freedesktop.org/series/34822/
> > 
> > Is it ok if I merge this to drm-tip already?
> 
> As long as you have this change in your tree, it should be safe.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/include/linux/pci.h?id=7912af5c835bd86f2b0347a480e0f40e2fab30d0
> 

We don't yet.

Rodrigo, can you please pull the above patch in once we get a
backmerge?

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: linux-next: manual merge of the akpm tree with the drm-intel tree

[Joonas Lahtinen]

Quoting Daniel Vetter (2020-10-01 18:13:26)
> On Thu, Oct 1, 2020 at 5:08 PM Jani Nikula  
> wrote:
> >
> > On Thu, 01 Oct 2020, Daniel Vetter  wrote:
> > > On Thu, Oct 1, 2020 at 3:53 PM Christoph Hellwig  wrote:
> > >>
> > >> On Thu, Oct 01, 2020 at 08:39:17PM +1000, Stephen Rothwell wrote:
> > >> > Hi all,
> > >> >
> > >> > Today's linux-next merge of the akpm tree got a conflict in:
> > >> >
> > >> >   drivers/gpu/drm/i915/gem/i915_gem_pages.c
> > >> >
> > >> > between commit:
> > >> >
> > >> >   4caf017ee937 ("drm/i915/gem: Avoid implicit vmap for highmem on 
> > >> > x86-32")
> > >> >   ba2ebf605d5f ("drm/i915/gem: Prevent using pgprot_writecombine() if 
> > >> > PAT is not supported")
> > >
> > > Uh these patches shouldn't be in linux-next because they're for 5.11,
> > > not the 5.10 merge window that will open soon. Joonas?
> >
> > I don't know anything else, but both are tagged Cc: stable.
> 
> Uh right I got confused, they're on -fixes now. Well -next-fixes,
> which seems like the wrong one for a cc: stable, I guess this should
> go into 5.9 even. Apologies for my confusion.

Yep, they happen to be Fixes: (Cc: stable even) so I asked Rodrigo to
pull them to drm-intel-next-fixes.

If they weren't Fixes: then indeed they would only have been queued for
5.11.

With regards to 5.9, due to the hiccup of doing the split PR, all the
-fixes for GT area were in limbo until -rc7. We didn't feel comfortable
including all the new commits this late in the cycle, so we agreed stable
porting those will be more appropriate.

Regards, Joonas

> -Daniel
> 
> >
> > BR,
> > Jani.
> >
> > >
> > >> > from the drm-intel tree and patch:
> > >> >
> > >> >   "drm/i915: use vmap in i915_gem_object_map"
> > >> >
> > >> > from the akpm tree.
> > >> >
> > >> > I fixed it up (I just dropped the changes in the former commits) and
> > >>
> > >> Sigh.  The solution is a bit more complicated, but I just redid my
> > >> patches to not depend on the above ones.  I can revert back to the old
> > >> version, though.  Andrew, let me know what works for you.
> > >
> > > Imo ignore, rebasing onto linux-next without those intel patches was
> > > the right thing for the 5.10 merge window.
> > > -Daniel
> >
> > --
> > Jani Nikula, Intel Open Source Graphics Center
> 
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Re: REGRESSION: in intel video driver following introduction of mm_struct.has_pinned

[Joonas Lahtinen]

(+ intel-gfx for being i915 related)
(+ Chris who has looked into the issue)

Hi,

Thanks for reporting!

Could you open a bug report according to following instructions:

https://gitlab.freedesktop.org/drm/intel/-/wikis/How-to-file-i915-bugs

A full dmesg of a bad boot and git bisect logs will be helpful.

Also, please describe when the problem happens, is it at boot? Are you
getting the OOPS on every boot?

For future reference, replying to a single thread helps keeping the
attention focused.

Regards, Joonas

Quoting Tony Fischetti (2020-09-28 21:14:16)
> After a length git bisection, I determined the commit that introduced
> a change that ultimately caused a bug/oops null dereference (see below
> for relevant syslog entries) was 008cfe4418b3dbda2ff.. (mm: Introduce
> mm_struct.has_pinned)
> 
> The RIP (according to syslog) occurs in function
> `__get_user_pages_remote` and the last function to call it from the
> i915 code is `gem_userptr_get_pages_worker`
> More specifically, it appears to be the call to
> `pin_user_pages_remote` in `gem_userptr_get_pages_worker` in
> drivers/gpu/drm/i915/gem/i915_gem_userptr.c that directly leads to the
> oops.
> 
> Unfortunately, I don't know enough to try to fix and share the fix
> myself, but I hope the information I provided is helpful. Please let
> me know if there is any further information I can provide that might
> be of use.
> 
> BUG: kernel NULL pointer dereference, address: 0054
> #PF: supervisor write access in kernel mode
> #PF: error_code(0x0002) - not-present page
> Oops: 0002 [#1] PREEMPT SMP NOPTI
> CPU: 8 PID: 497 Comm: kworker/u25:0 Not tainted
> 5.9.0-rc7-alice-investigate-3+ #2
> Hardware name: LENOVO 10ST001QUS/312A, BIOS M1UKT4BA 11/11/2019
> Workqueue: i915-userptr-acquire __i915_gem_userptr_get_pages_worker [i915]
> RIP: 0010:__get_user_pages_remote+0xa0/0x2d0
> Code: 85 e7 01 00 00 83 3b 01 0f 85 e0 01 00 00 f7 c1 00 00 04 00 0f
> 84 12 01 00 00 65 48 8b 04 25 00 6d 01 00 48 8b 80 58 03 00 00  40
> 54 01 00 00 00 c6 04 24 00 4d 8d 6f 68 48 c7 44 24 10 00 00
> RSP: 0018:a1a58086bde0 EFLAGS: 00010206
> RAX:  RBX: a1a58086be64 RCX: 00040001
> RDX: 07e9 RSI: 7f532f80 RDI: 92f22d89c480
> RBP: 7f532f80 R08: 92f23a188000 R09: 
> R10:  R11: a1a58086bcfd R12: 92f23a188000
> R13: 92f22d89c480 R14: 00042003 R15: 92f22d89c480
> FS:  () GS:92f23e40() knlGS:
> CS:  0010 DS:  ES:  CR0: 80050033
> CR2: 0054 CR3: 16c0a002 CR4: 001706e0
> DR0:  DR1:  DR2: 
> DR3:  DR6: fffe0ff0 DR7: 0400
> Call Trace:
>  __i915_gem_userptr_get_pages_worker+0x1ec/0x392 [i915]
>  process_one_work+0x1c7/0x310
>  worker_thread+0x28/0x3c0
>  ? set_worker_desc+0xb0/0xb0
>  kthread+0x123/0x140
>  ? kthread_use_mm+0xe0/0xe0
>  ret_from_fork+0x1f/0x30
> Modules linked in: snd_hda_codec_hdmi snd_hda_codec_realtek
> snd_hda_codec_generic ledtrig_audio iwlmvm mac80211 libarc4
> x86_pkg_temp_thermal intel_powerclamp iwlwifi coretemp i915
> crct10dif_pclmul crc32_pclmul crc32c_intel i2c_algo_bit
> ghash_clmulni_intel drm_kms_helper syscopyarea sysfillrect sysimgblt
> fb_sys_fops cec mei_hdcp wmi_bmof snd_hda_intel drm tpm_crb
> snd_intel_dspcfg intel_wmi_thunderbolt snd_hda_codec snd_hwdep
> aesni_intel crypto_simd glue_helper snd_hda_core cfg80211 i2c_i801
> snd_pcm intel_cstate pcspkr snd_timer mei_me i2c_smbus mei i2c_core
> thermal wmi tpm_tis tpm_tis_core tpm rng_core acpi_pad ppdev lp
> ip_tables x_tables
> CR2: 0054
> ---[ end trace 8d080e8b96289c9e ]---

Re: remove alloc_vm_area v2

[Joonas Lahtinen]

Quoting Christoph Hellwig (2020-09-28 15:37:41)
> On Mon, Sep 28, 2020 at 01:13:38PM +0300, Joonas Lahtinen wrote:
> > I think we have a gap that after splitting the drm-intel-next pull requests 
> > into
> > two the drm-intel/for-linux-next branch is now missing material from
> > drm-intel/drm-intel-gt-next.
> > 
> > I think a simple course of action might be to start including 
> > drm-intel-gt-next
> > in linux-next, which would mean that we should update DIM tooling to add
> > extra branch "drm-intel/gt-for-linux-next" or so.
> > 
> > Which specific patches are missing in this case?
> 
> The two dependencies required by my series not in mainline are:
> 
> drm/i915/gem: Avoid implicit vmap for highmem on x86-32
> drm/i915/gem: Prevent using pgprot_writecombine() if PAT is not supported
> 
> so it has to be one or both of those.

Hmm, those are both committed after our last -next pull request, so they
would normally only target next merge window. drm-next closes the merge
window around -rc5 already.

But, in this specific case those are both Fixes: patches with Cc: stable,
so they should be pulled into drm-intel-next-fixes PR.

Rodrigo, can you cherry-pick those patches to -next-fixes that you send
to Dave?

Regards, Joonas

Re: [PATCH tip/core/rcu 11/15] drm/i915: Cleanup PREEMPT_COUNT leftovers

[Joonas Lahtinen]

Quoting paul...@kernel.org (2020-09-29 02:30:58)
> From: Thomas Gleixner 
> 
> CONFIG_PREEMPT_COUNT is now unconditionally enabled and will be
> removed. Cleanup the leftovers before doing so.

Change looks fine:

Reviewed-by: Joonas Lahtinen 

Are you looking for us to merge or merge through another tree?

If us, did the base patch always enabling PREEMPT_COUNT go into 5.9 or is
it heading to 5.10? We can queue this earliest for 5.11 as drm-next closed
for 5.10 at week of -rc5.

Regards, Joonas

> Signed-off-by: Thomas Gleixner 
> Cc: Jani Nikula 
> Cc: Joonas Lahtinen 
> Cc: Rodrigo Vivi 
> Cc: David Airlie 
> Cc: Daniel Vetter 
> Cc: intel-...@lists.freedesktop.org
> Cc: dri-de...@lists.freedesktop.org
> Signed-off-by: Paul E. McKenney 
> ---
>  drivers/gpu/drm/i915/Kconfig.debug | 1 -
>  drivers/gpu/drm/i915/i915_utils.h  | 3 +--
>  2 files changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/Kconfig.debug 
> b/drivers/gpu/drm/i915/Kconfig.debug
> index 1cb28c2..17d9b00 100644
> --- a/drivers/gpu/drm/i915/Kconfig.debug
> +++ b/drivers/gpu/drm/i915/Kconfig.debug
> @@ -20,7 +20,6 @@ config DRM_I915_DEBUG
> bool "Enable additional driver debugging"
> depends on DRM_I915
> select DEBUG_FS
> -   select PREEMPT_COUNT
> select I2C_CHARDEV
> select STACKDEPOT
> select DRM_DP_AUX_CHARDEV
> diff --git a/drivers/gpu/drm/i915/i915_utils.h 
> b/drivers/gpu/drm/i915/i915_utils.h
> index 5477337..ecfed86 100644
> --- a/drivers/gpu/drm/i915/i915_utils.h
> +++ b/drivers/gpu/drm/i915/i915_utils.h
> @@ -337,8 +337,7 @@ wait_remaining_ms_from_jiffies(unsigned long 
> timestamp_jiffies, int to_wait_ms)
>(Wmax))
>  #define wait_for(COND, MS) _wait_for((COND), (MS) * 1000, 10, 
> 1000)
>  
> -/* If CONFIG_PREEMPT_COUNT is disabled, in_atomic() always reports false. */
> -#if defined(CONFIG_DRM_I915_DEBUG) && defined(CONFIG_PREEMPT_COUNT)
> +#ifdef CONFIG_DRM_I915_DEBUG
>  # define _WAIT_FOR_ATOMIC_CHECK(ATOMIC) WARN_ON_ONCE((ATOMIC) && 
> !in_atomic())
>  #else
>  # define _WAIT_FOR_ATOMIC_CHECK(ATOMIC) do { } while (0)
> -- 
> 2.9.5
> 

Re: [PATCH v6 44/80] docs: gpu: i915.rst: Fix several C duplication warnings

[Joonas Lahtinen]

+ Lionel

Can you please take a look at best resolving the below problem.

Maybe we should eliminate the duplicate declarations? Updating such
a list manually seems error prone to me.

Regards, Joonas

Quoting Mauro Carvalho Chehab (2020-10-13 14:53:59)
> As reported by Sphinx:
> 
> ./Documentation/gpu/i915:646: 
> ./drivers/gpu/drm/i915/i915_perf.c:1147: WARNING: Duplicate C declaration, 
> also defined in 'gpu/i915'.
> Declaration is 'i915_oa_wait_unlocked'.
> ./Documentation/gpu/i915:646: 
> ./drivers/gpu/drm/i915/i915_perf.c:1169: WARNING: Duplicate C declaration, 
> also defined in 'gpu/i915'.
> Declaration is 'i915_oa_poll_wait'.
> ./Documentation/gpu/i915:646: 
> ./drivers/gpu/drm/i915/i915_perf.c:1189: WARNING: Duplicate C declaration, 
> also defined in 'gpu/i915'.
> Declaration is 'i915_oa_read'.
> ./Documentation/gpu/i915:646: 
> ./drivers/gpu/drm/i915/i915_perf.c:2669: WARNING: Duplicate C declaration, 
> also defined in 'gpu/i915'.
> Declaration is 'i915_oa_stream_enable'.
> ./Documentation/gpu/i915:646: 
> ./drivers/gpu/drm/i915/i915_perf.c:2734: WARNING: Duplicate C declaration, 
> also defined in 'gpu/i915'.
> Declaration is 'i915_oa_stream_disable'.
> ./Documentation/gpu/i915:646: 
> ./drivers/gpu/drm/i915/i915_perf.c:2820: WARNING: Duplicate C declaration, 
> also defined in 'gpu/i915'.
> Declaration is 'i915_oa_stream_init'.
> ./Documentation/gpu/i915:646: 
> ./drivers/gpu/drm/i915/i915_perf.c:3010: WARNING: Duplicate C declaration, 
> also defined in 'gpu/i915'.
> Declaration is 'i915_perf_read'.
> ./Documentation/gpu/i915:646: 
> ./drivers/gpu/drm/i915/i915_perf.c:3098: WARNING: Duplicate C declaration, 
> also defined in 'gpu/i915'.
> Declaration is 'i915_perf_poll_locked'.
> ./Documentation/gpu/i915:646: 
> ./drivers/gpu/drm/i915/i915_perf.c:3129: WARNING: Duplicate C declaration, 
> also defined in 'gpu/i915'.
> Declaration is 'i915_perf_poll'.
> ./Documentation/gpu/i915:646: 
> ./drivers/gpu/drm/i915/i915_perf.c:3152: WARNING: Duplicate C declaration, 
> also defined in 'gpu/i915'.
> Declaration is 'i915_perf_enable_locked'.
> ./Documentation/gpu/i915:646: 
> ./drivers/gpu/drm/i915/i915_perf.c:3181: WARNING: Duplicate C declaration, 
> also defined in 'gpu/i915'.
> Declaration is 'i915_perf_disable_locked'.
> ./Documentation/gpu/i915:646: 
> ./drivers/gpu/drm/i915/i915_perf.c:3273: WARNING: Duplicate C declaration, 
> also defined in 'gpu/i915'.
> Declaration is 'i915_perf_ioctl'.
> ./Documentation/gpu/i915:646: 
> ./drivers/gpu/drm/i915/i915_perf.c:3296: WARNING: Duplicate C declaration, 
> also defined in 'gpu/i915'.
> Declaration is 'i915_perf_destroy_locked'.
> ./Documentation/gpu/i915:646: 
> ./drivers/gpu/drm/i915/i915_perf.c:3321: WARNING: Duplicate C declaration, 
> also defined in 'gpu/i915'.
> Declaration is 'i915_perf_release'.
> ./Documentation/gpu/i915:646: 
> ./drivers/gpu/drm/i915/i915_perf.c:3379: WARNING: Duplicate C declaration, 
> also defined in 'gpu/i915'.
> Declaration is 'i915_perf_open_ioctl_locked'.
> ./Documentation/gpu/i915:646: 
> ./drivers/gpu/drm/i915/i915_perf.c:3534: WARNING: Duplicate C declaration, 
> also defined in 'gpu/i915'.
> Declaration is 'read_properties_unlocked'.
> ./Documentation/gpu/i915:646: 
> ./drivers/gpu/drm/i915/i915_perf.c:3717: WARNING: Duplicate C declaration, 
> also defined in 'gpu/i915'.
> Declaration is 'i915_perf_open_ioctl'.
> ./Documentation/gpu/i915:646: 
> ./drivers/gpu/drm/i915/i915_perf.c:3760: WARNING: Duplicate C declaration, 
> also defined in 'gpu/i915'.
> Declaration is 'i915_perf_register'.
> ./Documentation/gpu/i915:646: 
> ./drivers/gpu/drm/i915/i915_perf.c:3789: WARNING: Duplicate C declaration, 
> also defined in 'gpu/i915'.
> Declaration is 'i915_perf_unregister'.
> ./Documentation/gpu/i915:646: 
> ./drivers/gpu/drm/i915/i915_perf.c:4009: WARNING: Duplicate C declaration, 
> also defined in 'gpu/i915'.
> Declaration is 'i915_perf_add_config_ioctl'.
> ./Documentation/gpu/i915:646: 
> ./drivers/gpu/drm/i915/i915_perf.c:4162: WARNING: Duplicate C declaration, 
> also defined in 'gpu/i915'.
> Declaration is 'i915_perf_remove_config_ioctl'.
> ./Documentation/gpu/i915:646: 
> ./drivers/gpu/drm/i915/i915_perf.c:4260: WARNING: Duplicate C declaration, 
> also defined in 'gpu/i915'.
> Declaration is 'i915_perf_init'.
> ./Documentation/gpu/i915:646: 
> ./drivers/gpu/drm/i915/i915_perf.c:4423: WARNING: Duplicate C declaration, 
> also defined in 'gpu/i915'.
> Declaration is 'i915_perf_fini'.
> 
> With Sphinx 3, C declarations can't be duplicated anymore,
> so let's exclude those from the other internals found on
> i915_perf.c file.
> 
> Signed-off-by: 

Re: [Intel-gfx] [PATCH v3 2/4] drm/i915/pmu: Use kstat_irqs to get interrupt count

[Joonas Lahtinen]

+ Tvrtko and Chris for comments

Code seems to be added in:

commit 0cd4684d6ea9a4ffec33fc19de4dd667bb90d0a5
Author: Tvrtko Ursulin 
Date:   Tue Nov 21 18:18:50 2017 +

drm/i915/pmu: Add interrupt count metric

I think later in the thread there was a suggestion to replace this with
simple counter increment in IRQ handler.

Regards, Joonas

Quoting Thomas Gleixner (2020-12-06 18:38:44)
> On Fri, Dec 04 2020 at 18:43, Jerry Snitselaar wrote:
> 
> > Now that kstat_irqs is exported, get rid of count_interrupts in
> > i915_pmu.c
> > --- a/drivers/gpu/drm/i915/i915_pmu.c
> > +++ b/drivers/gpu/drm/i915/i915_pmu.c
> > @@ -423,22 +423,6 @@ static enum hrtimer_restart i915_sample(struct hrtimer 
> > *hrtimer)
> >   return HRTIMER_RESTART;
> >  }
> >  
> > -static u64 count_interrupts(struct drm_i915_private *i915)
> > -{
> > - /* open-coded kstat_irqs() */
> > - struct irq_desc *desc = irq_to_desc(i915->drm.pdev->irq);
> > - u64 sum = 0;
> > - int cpu;
> > -
> > - if (!desc || !desc->kstat_irqs)
> > - return 0;
> > -
> > - for_each_possible_cpu(cpu)
> > - sum += *per_cpu_ptr(desc->kstat_irqs, cpu);
> > -
> > - return sum;
> > -}
> 
> May I ask why this has been merged in the first place?
> 
> Nothing in a driver has ever to fiddle with the internals of an irq
> descriptor. We have functions for properly accessing them. Just because
> C allows to fiddle with everything is not a justification. If the
> required function is not exported then adding the export with a proper
> explanation is not asked too much.
> 
> Also this lacks protection or at least a comment why this can be called
> safely and is not subject to a concurrent removal of the irq descriptor.
> The same problem exists when calling kstat_irqs(). It's even documented
> at the top of the function.
> 
> Thanks,
> 
> tglx
> 
> 
> ___
> Intel-gfx mailing list
> intel-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] x86/gpu: add JSL stolen memory support

[Joonas Lahtinen]

Quoting Bjorn Helgaas (2020-12-02 22:22:53)
> On Wed, Dec 02, 2020 at 05:21:58AM +, Surendrakumar Upadhyay, TejaskumarX 
> wrote:
> > Yes it fails all the tests which are allocating from this stolen
> > memory bunch. For example IGT tests like "
> > igt@kms_frontbuffer_tracking@-[fbc|fbcpsr].* |
> > igt@kms_fbcon_fbt@fbc.* " are failing as they totally depend to work
> > on stolen memory.

That's just because we have de-duped the stolen memory detection code.
If it's not detected at the early quirks, it's not detected by the
driver at all.

So if the patch is not merged to early quirks, we'd have to refactor the
code to add alternative detection path to i915. Before that is done, the
failures are expected.

> I'm sure that means something to graphics developers, but I have no
> idea!  Do you have URLs for the test case source, outputs, dmesg log,
> lspci info, bug reports, etc?

The thing is, the bug reports for stuff like this would only start to
flow after Jasperlake systems are shipping widely and the less common
OEMs start integrating it to into strangely behaving BIOSes. Or that
is the assumption.

If it's fine to merge this through i915 for now with an Acked-by, like
the previous patches, that'd be great. We can start a discussion on if
the new platforms are affected anymore. But I'd rather not drop it
before we have that understanding, as the previous problems have
included boot time memory corruption.

Would that work?

Regards, Joonas

> > > -Original Message-
> > > From: Bjorn Helgaas 
> > > Sent: 30 November 2020 22:21
> > > To: Surendrakumar Upadhyay, TejaskumarX
> > > 
> > > Cc: Jesse Barnes ; Daniel Vetter ;
> > > Joonas Lahtinen ; Linux PCI  > > p...@vger.kernel.org>; Linux Kernel Mailing List  > > ker...@vger.kernel.org>; X86 ML ; Borislav Petkov
> > > ; De Marchi, Lucas ; Roper,
> > > Matthew D ; Pandey, Hariom
> > > ; Jani Nikula ; 
> > > Vivi,
> > > Rodrigo ; David Airlie 
> > > Subject: Re: [PATCH] x86/gpu: add JSL stolen memory support
> > > 
> > > On Mon, Nov 30, 2020 at 10:44:14AM +, Surendrakumar Upadhyay,
> > > TejaskumarX wrote:
> > > > Hi All,
> > > >
> > > > Are we merging this patch in?
> > > 
> > > Does it fix something?  If something is broken without this patch, can we
> > > collect information about exactly what is broken and how it fails?
> > > 
> > > But I don't object if somebody else wants to apply this.
> > > 
> > > > > -Original Message-
> > > > > From: Jesse Barnes 
> > > > > Sent: 20 November 2020 03:32
> > > > > To: Bjorn Helgaas 
> > > > > Cc: Daniel Vetter ; Joonas Lahtinen
> > > > > ; Surendrakumar Upadhyay,
> > > > > TejaskumarX ; Linux
> > > > > PCI ; Linux Kernel Mailing List  > > > > ker...@vger.kernel.org>; X86 ML ; Borislav Petkov
> > > > > ; De Marchi, Lucas ; Roper,
> > > > > Matthew D ; Pandey, Hariom
> > > > > ; Jani Nikula
> > > > > ; Vivi, Rodrigo
> > > > > ; David Airlie 
> > > > > Subject: Re: [PATCH] x86/gpu: add JSL stolen memory support
> > > > >
> > > > > On Thu, Nov 19, 2020 at 11:19 AM Bjorn Helgaas 
> > > > > wrote:
> > > > > >
> > > > > > [+cc Jesse]
> > > > > >
> > > > > > On Thu, Nov 19, 2020 at 10:37:10AM +0100, Daniel Vetter wrote:
> > > > > > > On Thu, Nov 19, 2020 at 12:14 AM Bjorn Helgaas
> > > > > > > 
> > > > > wrote:
> > > > > > > > On Wed, Nov 18, 2020 at 10:57:26PM +0100, Daniel Vetter wrote:
> > > > > > > > > On Wed, Nov 18, 2020 at 5:02 PM Bjorn Helgaas
> > > > >  wrote:
> > > > > > > > > > On Fri, Nov 06, 2020 at 10:39:16AM +0100, Daniel Vetter 
> > > > > > > > > > wrote:
> > > > > > > > > > > On Thu, Nov 5, 2020 at 3:17 PM Bjorn Helgaas
> > > > >  wrote:
> > > > > > > > > > > > On Thu, Nov 05, 2020 at 11:46:06AM +0200, Joonas
> > > > > > > > > > > > Lahtinen
> > > > > wrote:
> > > > > > > > > > > > > Quoting Bjorn Helgaas (2020-11-04 19:35:56)
> > > > > > > > > > > > > > [+cc Jani, Joonas, Rodrigo, David, Daniel]
> > > > > > > > > > > &g

Re: [PATCH] x86/gpu: add JSL stolen memory support

[Joonas Lahtinen]

Quoting Bjorn Helgaas (2020-11-04 19:35:56)
> [+cc Jani, Joonas, Rodrigo, David, Daniel]
> 
> On Wed, Nov 04, 2020 at 05:35:06PM +0530, Tejas Upadhyay wrote:
> > JSL re-uses the same stolen memory as ICL and EHL.
> > 
> > Cc: Lucas De Marchi 
> > Cc: Matt Roper 
> > Signed-off-by: Tejas Upadhyay 
> 
> I don't plan to do anything with this since previous similar patches
> have gone through some other tree, so this is just kibitzing.
> 
> But the fact that we have this long list of Intel devices [1] that
> constantly needs updates [2] is a hint that something is wrong.

We add an entry for every new integrated graphics platform. Once the
platform is added, there have not been changes lately.

> IIUC the general idea is that we need to discover Intel gfx memory by
> looking at device-dependent config space and add it to the E820 map.
> Apparently the quirks discover this via PCI config registers like
> I830_ESMRAMC, I845_ESMRAMC, etc, and tell the driver about it via the
> global "intel_graphics_stolen_res"?

We discover what is called the graphics data stolen memory. It is regular
system memory range that is not CPU accessible. It is accessible by the
integrated graphics only.

See: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/arch/x86/kernel/early-quirks.c?h=v5.10-rc2=814c5f1f52a4beb3710317022acd6ad34fc0b6b9

> That's not the way this should work.  There should some generic, non
> device-dependent PCI or ACPI method to discover the memory used, or at
> least some way to do it in the driver instead of early arch code.

It's used by the early BIOS/UEFI code to set up initial framebuffer.
Even if i915 driver is never loaded, the memory ranges still need to
be fixed. They source of the problem is that the OEM BIOS which are
not under our control get the programming wrong.

We used to detect the memory region size again at i915 initialization
but wanted to eliminate the code duplication and resulting subtle bugs
that caused. Conclusion back then was that storing the struct resource
in memory is the best trade-off.

> How is this *supposed* to work?  Is there something we can do in E820
> or other resource management that would make this easier?

The code was added around Haswell (HSW) device generation to mitigate
bugs in BIOS. It is traditionally hard to get all OEMs to fix their
BIOS when things work for Windows. It's only later years when some
laptop models are intended to be sold with Linux.

The alternative would be to get all the OEM to fix their BIOS for Linux,
but that is not very realistic given past experiences. So it seems
a better choice to to add new line per platform generation to make
sure the users can boot to Linux.

Regards, Joonas

> > ---
> >  arch/x86/kernel/early-quirks.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
> > index a4b5af03dcc1..534cc3f78c6b 100644
> > --- a/arch/x86/kernel/early-quirks.c
> > +++ b/arch/x86/kernel/early-quirks.c
> > @@ -549,6 +549,7 @@ static const struct pci_device_id intel_early_ids[] 
> > __initconst = {
> >   INTEL_CNL_IDS(_early_ops),
> >   INTEL_ICL_11_IDS(_early_ops),
> >   INTEL_EHL_IDS(_early_ops),
> > + INTEL_JSL_IDS(_early_ops),
> >   INTEL_TGL_12_IDS(_early_ops),
> >   INTEL_RKL_IDS(_early_ops),
> >  };
> 
> [1] 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kernel/early-quirks.c?h=v5.10-rc2#n518
> 
> [2]
>   May 2020 efbee021ad02 ("x86/gpu: add RKL stolen memory support")
>   Jul 2019 6b2436aeb945 ("x86/gpu: add TGL stolen memory support")
>   Mar 2019 d53fef0be4a5 ("x86/gpu: add ElkhartLake to gen11 early quirks")
>   May 2018 db0c8d8b031d ("x86/gpu: reserve ICL's graphics stolen memory")
>   Dec 2017 33aa69ed8aac ("x86/gpu: add CFL to early quirks")
>   Jul 2017 2e1e9d48939e ("x86/gpu: CNL uses the same GMS values as SKL")
>   Jan 2017 bc384c77e3bb ("x86/gpu: GLK uses the same GMS values as SKL")
>   Oct 2015 00ce5c8a66fb ("drm/i915/kbl: Kabylake uses the same GMS values as 
> Skylake")
>   Mar 2015 31d4dcf705c3 ("drm/i915/bxt: Broxton uses the same GMS values as 
> Skylake")
>   ...

Re: remove alloc_vm_area v2

[Joonas Lahtinen]

+ Dave and Daniel
+ Stephen

Quoting Christoph Hellwig (2020-09-26 09:29:59)
> On Fri, Sep 25, 2020 at 07:43:49PM -0700, Andrew Morton wrote:
> > On Thu, 24 Sep 2020 15:58:42 +0200 Christoph Hellwig  wrote:
> > 
> > > this series removes alloc_vm_area, which was left over from the big
> > > vmalloc interface rework.  It is a rather arkane interface, basicaly
> > > the equivalent of get_vm_area + actually faulting in all PTEs in
> > > the allocated area.  It was originally addeds for Xen (which isn't
> > > modular to start with), and then grew users in zsmalloc and i915
> > > which seems to mostly qualify as abuses of the interface, especially
> > > for i915 as a random driver should not set up PTE bits directly.
> > > 
> > > Note that the i915 patches apply to the drm-tip branch of the drm-tip
> > > tree, as that tree has recent conflicting commits in the same area.
> > 
> > Is the drm-tip material in linux-next yet?  I'm still seeing a non-trivial
> > reject in there at present.
> 
> I assumed it was, but the reject imply that they aren't.  Tvrtko, do you
> know the details?

I think we have a gap that after splitting the drm-intel-next pull requests into
two the drm-intel/for-linux-next branch is now missing material from
drm-intel/drm-intel-gt-next.

I think a simple course of action might be to start including drm-intel-gt-next
in linux-next, which would mean that we should update DIM tooling to add
extra branch "drm-intel/gt-for-linux-next" or so.

Which specific patches are missing in this case?

Regards, Joonas

Re: [PATCH v4 0/7] Convert the intel iommu driver to the dma-iommu api

[Joonas Lahtinen]

Quoting Tvrtko Ursulin (2020-11-03 11:14:32)
> 
> 
> On 03/11/2020 02:53, Lu Baolu wrote:
> > On 11/2/20 7:52 PM, Tvrtko Ursulin wrote:
> >>
> >> On 02/11/2020 02:00, Lu Baolu wrote:
> >>> Hi Tvrtko,
> >>> On 10/12/20 4:44 PM, Tvrtko Ursulin wrote:
> 
>  On 29/09/2020 01:11, Lu Baolu wrote:



>  FYI we have merged the required i915 patches to out tree last week 
>  or so. I *think* this means they will go into 5.11. So the i915 
>  specific workaround patch will not be needed in Intel IOMMU.
> >>>
> >>> Do you mind telling me what's the status of this fix patch? I tried this
> >>> series on v5.10-rc1 with the graphic quirk patch dropped. I am still
> >>> seeing dma faults from graphic device.
> >>
> >> Hmm back then I thought i915 fixes for this would land in 5.11 so I 
> >> will stick with that. :) (See my quoted text a paragraph above yours.)
> > 
> > What size are those fixes? I am considering pushing this series for
> > v5.11. Is it possible to get some acks for those patches and let them
> > go to Linus through iommu tree?
> 
> For 5.10 you mean? They feel a bit too large for comfort to go via a 
> non-i915/drm tree. These are the two patches required:
> 
> https://cgit.freedesktop.org/drm-intel/commit/?h=drm-intel-gt-next=8a473dbadccfc6206150de3db3223c40785da348
> https://cgit.freedesktop.org/drm-intel/commit/?h=drm-intel-gt-next=934941ed5a3070a7833c688c9b1d71484fc01a68
> 
> I'll copy Joonas as our maintainer - how does the idea of taking the 
> above two patches through the iommu tree sound to you?

Hi Lu,

The patches have already been merged into our tree and are heading
towards 5.11, so I don't think we should merge them elsewhere. DRM
subsystem had the feature freeze for 5.10 at the time of 5.9-rc5
and only drm-intel-fixes pull requests are sent after that.

The patches seem to target to eliminate need for a previously used
workaround. To me it seems more appropriate for the patches to follow
the regular process as new feature for 5.11 to make sure the changes
get validated as part of linux-next.

Would that work for you? We intend to send the feature pull requests
to DRM for 5.11 in the upcoming weeks.

Regards, Joonas

Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly

[Joonas Lahtinen]

On Fri, 2017-09-22 at 17:50 +, Wang, Zhi A wrote:
> Thanks for the reply. Learned a lot. :)
> 
> GEM_BUG_ON is new to me since it wasn't there at the beginning of
> GVT-g upstream. It showed up later. So I left a lot of WARN_ON in the
> code and some of them should be GEM_BUG_ON now.
> 
> Now I can figure out those differences. We can discuss with our QA to
> see if they would like to enable I915_GEM_DEBUG and then we can move
> to GEM_BUG_ON also, or maybe we can have a dedicated GVT_BUG_ON. :)
> Thank you so much. Have a great weekend.

GVT_BUG_ON is probably the way to go :)

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: linux-next: Fixes tag needs some work in the drm-intel tree

[Joonas Lahtinen]

Quoting Stephen Rothwell (2019-05-20 15:15:38)
> Hi all,
> 
> In commit
> 
>   0d90ccb70211 ("drm/i915: Delay semaphore submission until the start of the 
> signaler")
> 
> Fixes tag
> 
>   Fixes: e88619646971 ("drm/i915: Use HW semaphores for inter-engine synchroni
> 
> has these problem(s):
> 
>   - Subject has leading but no trailing parentheses
>   - Subject has leading but no trailing quotes
> 
> Please don't split Fixes tags across more than one line.

Thanks for the report.

This was a copy'n paste mishap, detected by our tooling (and fixed by
me) at the time of creating a PR. Unfortunately the check was not being
enforced by tooling at commit time. We'll fix that.

Regards, Joonas

Re: [PATCH 06/34] drm/i915: convert put_page() to put_user_page*()

[Joonas Lahtinen]

Quoting john.hubb...@gmail.com (2019-08-02 05:19:37)
> From: John Hubbard 
> 
> For pages that were retained via get_user_pages*(), release those pages
> via the new put_user_page*() routines, instead of via put_page() or
> release_pages().
> 
> This is part a tree-wide conversion, as described in commit fc1d8e7cca2d
> ("mm: introduce put_user_page*(), placeholder versions").
> 
> Note that this effectively changes the code's behavior in
> i915_gem_userptr_put_pages(): it now calls set_page_dirty_lock(),
> instead of set_page_dirty(). This is probably more accurate.

We've already fixed this in drm-tip where the current code uses
set_page_dirty_lock().

This would conflict with our tree. Rodrigo is handling
drm-intel-next for 5.4, so you guys want to coordinate how
to merge.

Regards, Joonas

Re: linux-next: build failure after merge of the drm-intel-fixes tree

[Joonas Lahtinen]

Quoting Stephen Rothwell (2020-06-16 02:39:12)
> Hi all,
> 
> After merging the drm-intel-fixes tree, today's linux-next build (x86_64
> allmodconfig) failed like this:
> 
> In file included from drivers/gpu/drm/i915/gt/intel_lrc.c:5972:
> drivers/gpu/drm/i915/gt/selftest_lrc.c: In function 
> 'live_timeslice_nopreempt':
> drivers/gpu/drm/i915/gt/selftest_lrc.c:1333:3: error: too few arguments to 
> function 'engine_heartbeat_disable'
>  1333 |   engine_heartbeat_disable(engine);
>   |   ^~~~
> drivers/gpu/drm/i915/gt/selftest_lrc.c:54:13: note: declared here
>54 | static void engine_heartbeat_disable(struct intel_engine_cs *engine,
>   | ^~~~
> drivers/gpu/drm/i915/gt/selftest_lrc.c:1402:3: error: too few arguments to 
> function 'engine_heartbeat_enable'
>  1402 |   engine_heartbeat_enable(engine);
>   |   ^~~
> drivers/gpu/drm/i915/gt/selftest_lrc.c:64:13: note: declared here
>64 | static void engine_heartbeat_enable(struct intel_engine_cs *engine,
>   | ^~~
> 
> Caused by commit
> 
>   04dc41776145 ("drm/i915/gt: Prevent timeslicing into unpreemptable 
> requests")
> 
> I have reverted that commit for today.

Thanks for reporting. I had my drm-intel-fixes build tree configured
without selftests. I've now corrected that and added a missing dependency
patch.

Regards, Joonas

> 
> -- 
> Cheers,
> Stephen Rothwell

Re: [PATCH] drm/i915: avoid rebuilding i915_gpu_error.o on version string updates

[Joonas Lahtinen]

Quoting Hans Holmberg (2018-11-21 13:35:19)
> On Wed, Nov 21, 2018 at 11:10 AM Joonas Lahtinen
>  wrote:
> >
> > Quoting Hans Holmberg (2018-11-21 11:54:23)
> > > From: Hans Holmberg 
> > >
> > > There is no need to rebuild i915_gpu_error.o when the version string
> > > changes as the version is available in init_utsname()->release.
> > >
> > > Signed-off-by: Hans Holmberg 
> >
> > Seems reasonable to me.
> >
> > Reviewed-by: Joonas Lahtinen 
> >
> > Out of curiosity, are you by any chance hashing the i915_gpu_error.o
> > file (or the contents elsewhere) for some purpose?
> 
> Oh no, I was just moderately annoyed by the file being rebuilt every
> time the version was updated(I use my current branch name as
> LOCALVERSION when building).

That's a reasonable explanation, too :)

I unblocked the message from moderation queue so that our CI picks this
up for testing. I will then proceed to merge this once the results are
back.

Thanks for the patch!

Regards, Joonas

> 
> Thanks,
> Hans
> >
> > Regards, Joonas
> >
> > > ---
> > >  drivers/gpu/drm/i915/i915_gpu_error.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c 
> > > b/drivers/gpu/drm/i915/i915_gpu_error.c
> > > index 8762d17b6659..958e1484a3dd 100644
> > > --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> > > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> > > @@ -27,7 +27,7 @@
> > >   *
> > >   */
> > >
> > > -#include 
> > > +#include 
> > >  #include 
> > >  #include 
> > >  #include 
> > > @@ -650,7 +650,7 @@ int i915_error_state_to_str(struct 
> > > drm_i915_error_state_buf *m,
> > >
> > > if (*error->error_msg)
> > > err_printf(m, "%s\n", error->error_msg);
> > > -   err_printf(m, "Kernel: " UTS_RELEASE "\n");
> > > +   err_printf(m, "Kernel: %s\n", init_utsname()->release);
> > > ts = ktime_to_timespec64(error->time);
> > > err_printf(m, "Time: %lld s %ld us\n",
> > >(s64)ts.tv_sec, ts.tv_nsec / NSEC_PER_USEC);
> > > --
> > > 2.17.1
> > >

Re: 4.20.0-rc6-next-20181210, v4.20-rc1: list_del corruption on thinkpad x220, graphics related?

[Joonas Lahtinen]

Quoting Pavel Machek (2018-12-12 20:29:02)
> Hi!
> 
> > > > > > > > There's one similar for nouveau in Bugzilla, but it seems like 
> > > > > > > > a genuine
> > > > > > > > memory corruption (1 bit flipped):
> > > > > > > > 
> > > > > > > > https://bugs.freedesktop.org/show_bug.cgi?id=84880
> > > > > > > > 
> > > > > > > > Any extra information would be of use :)
> > > > > > > > 
> > > > > > > > Regards, Joonas
> > > > > > > > 
> > > > > > > > PS. Could you open a bug to Bugzilla, it'll help to collect the
> > > > > > > > information in one consolidated place:
> > > > > > > > 
> > > > > > > > https://01.org/linuxgraphics/documentation/how-report-bugs
> > > > > > > 
> > > > > > > I prefer email... certainly for bugs that can't be reproduced.
> > > > > > 
> > > > > > By adding it to the Bugzilla it may be recognized by somebody else
> > > > > > who is experiencing a similar issue. Internet points are not 
> > > > > > deducted
> > > > > > for submitting bugs in good faith, even if they get closed as
> > > > > > NOTABUG.
> > > > 
> > > > Well, your documentation suggests you'll deduce my internet points:
> > > > 
> > > >   Before filing the bug, please try to reproduce your issue with the
> > > >   latest kernel. Use the latest drm-tip branch from
> > > >   http://cgit.freedesktop.org/drm-tip and build as instructed on our
> > > >   Build Guide.
> > > > 
> > > > :-)
> > > 
> > > I'd prefer not to run drm-tip. I'll update to 2.6.20-rc5+ and see if
> > > it re-appears (but it takes long time to reproduce :-().
> > 
> > If we can or can not reproduce the issue with drm-tip, is a very useful
> > datapoint for us. If we can not reproduce, it'll be possible to bisect
> > which commit fixed it, and backport that. On the other hand, if it's
> > still reproducible, we know we're not spending time on something we
> > already fixed, and the priority gets a bump.
> 
> bisect ... is not practical on something that takes 2 days to reproduce.
> 
> > > If you think it is useful, I can try to update my machine to
> > > linux-next.
> > 
> > linux-next is closer to drm-tip, so it's better. Do you have some
> > specific reason for not wanting to run drm-tip (but linux-next is still
> > ok)?
> 
> I already have build/update scripts for -next, and I trust -next not
> to store screenshots of my desktop in my master boot record :-).
> 
> Anyway, it does happen with -next. This time, chromiums were running,
> and crash happened minute? after I exited flightgear. It can be seen
> in the logs.
> 
> Oh and I might want to mention -- machine was rather deep in swap this
> time, as in "mouse jumping when starting fgfs" and "could feel the
> chromium being swapped back in". I might have had this situation
> before, and just powercycled the machine "because it is so deep in
> swap that it will not recover".
> 
> top says:
> 
> top - 19:18:24 up 2 days,  8:03,  2 users,  load average: 3.02, 3.45,
> 3.21
> Tasks: 141 total,   1 running,  86 sleeping,   0 stopped,   2 zombie
> %Cpu(s): 18.8 us,  7.6 sy,  3.0 ni, 68.4 id,  1.3 wa,  0.0 hi,  0.9
> si,  0.0 st
> KiB Mem:   5967968 total,   663244 used,  5304724 free,48876
> buffers
> KiB Swap:  1681428 total,   170904 used,  1510524 free.   446280
> cached Mem
> 
> but of course that memory is free once everything died.
> 
> Any ideas? Should I go back to v4.19 to see if it happens there, too?

linux-next includes very much the same code as drm-tip. There's nobody
magically reviewing the code more than it is reviewed for inclusion into
drm-tip, when it is fed into linux-next. So thinking linux-next would be
some way safer is an illusion.

It sounds like having memory pressure expedites the corruption, which
should make it easier to reproduce and thus fix.

So if you could please try drm-tip reproducing AND open a bug in Bugzilla.
If you are unwilling to do that, it is very difficult to help you more.

Regards, Joonas

> 
> 
> Pavel
> -- 
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) 
> http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PULL] topic/mei-hdcp

[Joonas Lahtinen]

Quoting Daniel Vetter (2019-02-19 09:55:27)
> Hi all,
> 
> topic/mei-hdcp-2019-02-19:
> Prep patches + headers for the mei-hdcp/i915 component interfaces
> 
> Also contains the prep work in the component helpers plus adjustements
> for the snd-hda/i915 component interface.
> 
> Plus one small static inline in the drm_hdcp.h header that both i915
> and mei_hdcp will need.
> 
> Joonas, please pull into drm-intel-next-queued so I can apply the i915
> hdcp patches.

This is now pulled.

Regards, Joonas

> 
> Greg/Arnd, I think there's two options to get the mei-hdcp patches landed
> into drivers-misc:
> - You pull this topic pull and then apply the mei-hdcp patches on top in
>   char-misc-next.
> - I also pull in char-misc-next to get at 32ea33a04484 ("mei: bus: export
>   to_mei_cl_device for mei client devices drivers") and then apply all the
>   mei-hdcp stuff into a new topic branch.
> 
> I think 2nd option would be better, allows us to integration test easier,
> and we'll have a topic branch in case we need a fixup spanning mei-hdcp
> and i915. Also would allow me to start merging the patches, since I think
> it's too late for 5.1.
> 
> Cheers, Daniel
> 
> The following changes since commit 8834f5600cf3c8db365e18a3d5cac2c2780c81e5:
> 
>   Linux 5.0-rc5 (2019-02-03 13:48:04 -0800)
> 
> are available in the Git repository at:
> 
>   git://anongit.freedesktop.org/drm/drm-intel tags/topic/mei-hdcp-2019-02-19
> 
> for you to fetch changes up to 35c0272502cca0a1b461d310c23aac94a503983d:
> 
>   drm/audio: declaration of struct device (2019-02-18 20:19:28 +0100)
> 
> 
> Prep patches + headers for the mei-hdcp/i915 component interfaces
> 
> Also contains the prep work in the component helpers plus adjustements
> for the snd-hda/i915 component interface.
> 
> Plus one small static inline in the drm_hdcp.h header that both i915
> and mei_hdcp will need.
> 
> 
> Daniel Vetter (3):
>   component: Add documentation
>   components: multiple components for a device
>   i915/snd_hdac: I915 subcomponent for the snd_hdac
> 
> Ramalingam C (5):
>   drm/i915: enum port definition is moved into i915_drm.h
>   drm/i915: header for i915 - MEI_HDCP interface
>   drm/i915: MEI interface definition
>   drm: helper functions for hdcp2 seq_num to from u32
>   drm/audio: declaration of struct device
> 
>  Documentation/driver-api/component.rst   |  17 +++
>  Documentation/driver-api/device_link.rst |   3 +
>  Documentation/driver-api/index.rst   |   1 +
>  drivers/base/component.c | 206 
> +--
>  drivers/gpu/drm/i915/intel_audio.c   |   4 +-
>  drivers/gpu/drm/i915/intel_display.h |  16 +--
>  include/drm/drm_audio_component.h|   1 +
>  include/drm/drm_hdcp.h   |  18 +++
>  include/drm/i915_component.h |   5 +
>  include/drm/i915_drm.h   |  15 +++
>  include/drm/i915_mei_hdcp_interface.h| 149 ++
>  include/linux/component.h|  76 
>  include/sound/hda_component.h|   5 +-
>  sound/hda/hdac_component.c   |   4 +-
>  sound/hda/hdac_i915.c|   6 +-
>  15 files changed, 493 insertions(+), 33 deletions(-)
>  create mode 100644 Documentation/driver-api/component.rst
>  create mode 100644 include/drm/i915_mei_hdcp_interface.h
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Re: [PATCH] iommu/intel: quirk to disable DMAR for QM57 igfx

[Joonas Lahtinen]

Quoting Joerg Roedel (2019-01-22 13:01:09)
> Hi Daniel,
> 
> On Tue, Jan 22, 2019 at 11:46:39AM +0100, Daniel Vetter wrote:
> > Note that the string of platforms which have various issues with iommu
> > and igfx is very long, thus far we only disabled it where there's no
> > workaround to stop it from hanging the box, but otherwise left it
> > enabled. So if we make a policy change to also disable it anywhere
> > where it doesn't work well (instead of not at all), there's a pile
> > more platforms to switch.
> 
> I think its best to just disable iommu for the igfx devices on these
> platforms. Can you pick up Eric's patch and extend it with the list of
> affected platforms?

We've been discussing this again more actively since a few months ago,
and the discussion is still ongoing internally.

According to our IOMMU folks there exists some desire to be able to assign
the iGFX device aka have intel_iommu=on instead of intel_iommu=igfx_off
due to how the devices might be grouped in IOMMU groups. Even when you
would not be using the iGFX device.

So for some uses, the fact that the device (group) is assignable seems
to be more important than the iGFX device to be working. I'm afraid
that retroactively disabling the assignment for such an old platform
might break those usage scenarios. By my quick reading of the code,
there's no way for user to turn the iGFX DMAR on once the quirk
disables it.

I guess one solution would be to default to intel_iommu=igfx_off for
platforms that are older than certain threshold. But still allow
user to enable. But that then requires duplicating the PCI ID database
into iommu code.

I don't really have winning moves to present, but I'm open to hearing
how we can avoid more damage than starting to default to intel_iommu=on
did already.

Regards, Joonas

> 
> Thanks,
> 
> Joerg

Re: [PATCH] iommu/intel: quirk to disable DMAR for QM57 igfx

[Joonas Lahtinen]

Quoting Joerg Roedel (2019-01-22 18:51:35)
> On Tue, Jan 22, 2019 at 04:48:26PM +0200, Joonas Lahtinen wrote:
> > According to our IOMMU folks there exists some desire to be able to assign
> > the iGFX device aka have intel_iommu=on instead of intel_iommu=igfx_off
> > due to how the devices might be grouped in IOMMU groups. Even when you
> > would not be using the iGFX device.
> 
> You can force the igfx device into a SI domain, or does that also
> trigger the iommu issues on the chipset?

To be honest, we've had a mixture different issues on different SKUs
that have not been hit in the past when intel_iommu was just disabled by
default.

I know that in one group of the problems, the issue has been debugged
into the GPU having its own set of virtualization mapping translation
hardware with caching and it fails to track changes to the mapping. So
if a identity mapping was established and never changed, I'd assume that
to fix at least that class of problems.

Would just passing intel_iommu=on already cause a non-identity mapping to
possibly be used for the integrated GPU? If it did, then it would
explain quite few of the issues.

We have many reports where just having intel_iommu=on (and using the
system normally, without any virtualization stuff going on) will cause
unexplained GPU hangs. For those users, simply switching to
intel_iommu=igfx_off solves the problems, and the debug often ends
there.

Regards, Joonas

> In any case, if iommu=on breaks these systems I want to make them work
> again with opt-out, even at the cost of breaking assignability.
> 
> Regards,
> 
> Joerg

Re: [PATCH v4 8/9] gpu/drm/i915: optimize out the case when a range is updated to read only

[Joonas Lahtinen]

Hi Jerome,

This patch seems to have plenty of Cc:s, but none of the right ones :)

For further iterations, I guess you could use git option --cc to make
sure everyone gets the whole series, and still keep the Cc:s in the
patches themselves relevant to subsystems.

This doesn't seem to be on top of drm-tip, but on top of your previous
patches(?) that I had some comments about. Could you take a moment to
first address the couple of question I had, before proceeding to discuss
what is built on top of that base.

My reply's Message-ID is:
154289518994.19402.3481838548028068...@jlahtine-desk.ger.corp.intel.com

Regards, Joonas

PS. Please keep me Cc:d in the following patches, I'm keen on
understanding the motive and benefits.

Quoting jgli...@redhat.com (2019-01-24 00:23:14)
> From: Jérôme Glisse 
> 
> When range of virtual address is updated read only and corresponding
> user ptr object are already read only it is pointless to do anything.
> Optimize this case out.
> 
> Signed-off-by: Jérôme Glisse 
> Cc: Christian König 
> Cc: Jan Kara 
> Cc: Felix Kuehling 
> Cc: Jason Gunthorpe 
> Cc: Andrew Morton 
> Cc: Matthew Wilcox 
> Cc: Ross Zwisler 
> Cc: Dan Williams 
> Cc: Paolo Bonzini 
> Cc: Radim Krčmář 
> Cc: Michal Hocko 
> Cc: Ralph Campbell 
> Cc: John Hubbard 
> Cc: k...@vger.kernel.org
> Cc: dri-de...@lists.freedesktop.org
> Cc: linux-r...@vger.kernel.org
> Cc: linux-fsde...@vger.kernel.org
> Cc: Arnd Bergmann 
> ---
>  drivers/gpu/drm/i915/i915_gem_userptr.c | 16 
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c 
> b/drivers/gpu/drm/i915/i915_gem_userptr.c
> index 9558582c105e..23330ac3d7ea 100644
> --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> @@ -59,6 +59,7 @@ struct i915_mmu_object {
> struct interval_tree_node it;
> struct list_head link;
> struct work_struct work;
> +   bool read_only;
> bool attached;
>  };
>  
> @@ -119,6 +120,7 @@ static int 
> i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
> container_of(_mn, struct i915_mmu_notifier, mn);
> struct i915_mmu_object *mo;
> struct interval_tree_node *it;
> +   bool update_to_read_only;
> LIST_HEAD(cancelled);
> unsigned long end;
>  
> @@ -128,6 +130,8 @@ static int 
> i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
> /* interval ranges are inclusive, but invalidate range is exclusive */
> end = range->end - 1;
>  
> +   update_to_read_only = mmu_notifier_range_update_to_read_only(range);
> +
> spin_lock(>lock);
> it = interval_tree_iter_first(>objects, range->start, end);
> while (it) {
> @@ -145,6 +149,17 @@ static int 
> i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
>  * object if it is not in the process of being destroyed.
>  */
> mo = container_of(it, struct i915_mmu_object, it);
> +
> +   /*
> +* If it is already read only and we are updating to
> +* read only then we do not need to change anything.
> +* So save time and skip this one.
> +*/
> +   if (update_to_read_only && mo->read_only) {
> +   it = interval_tree_iter_next(it, range->start, end);
> +   continue;
> +   }
> +
> if (kref_get_unless_zero(>obj->base.refcount))
> queue_work(mn->wq, >work);
>  
> @@ -270,6 +285,7 @@ i915_gem_userptr_init__mmu_notifier(struct 
> drm_i915_gem_object *obj,
> mo->mn = mn;
> mo->obj = obj;
> mo->it.start = obj->userptr.ptr;
> +   mo->read_only = i915_gem_object_is_readonly(obj);
> mo->it.last = obj->userptr.ptr + obj->base.size - 1;
> INIT_WORK(>work, cancel_userptr);
>  
> -- 
> 2.17.2
> 
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 07/16] tools headers uapi: Update i915_drm.h

[Joonas Lahtinen]

Quoting Arnaldo Carvalho de Melo (2019-01-04 20:33:28)
> From: Arnaldo Carvalho de Melo 
> 
> To get the changes in these csets:
> 
>   fe841686470d Revert "drm/i915/perf: add a parameter to control the size of 
> OA buffer"
>   cd956bfcd0f5 drm/i915/perf: add a parameter to control the size of OA buffer
>   4bdafb9ddfa4 drm/i915: Remove i915.enable_ppgtt override
> 
> Not one of them result in any changes in tools/perf/, this is just to
> silence this perf build warning:
> 
>   Warning: Kernel ABI header at 'tools/include/uapi/drm/i915_drm.h' differs 
> from latest version at 'include/uapi/drm/i915_drm.h'
>   diff -u tools/include/uapi/drm/i915_drm.h include/uapi/drm/i915_drm.h
> 
> Cc: Adrian Hunter 
> Cc: Chris Wilson 
> Cc: Jiri Olsa 
> Cc: Joonas Lahtinen 
> Cc: Lionel Landwerlin 
> Cc: Namhyung Kim 
> Link: https://lkml.kernel.org/n/tip-mdw7ta6qz7d2rl77gf00u...@git.kernel.org
> Signed-off-by: Arnaldo Carvalho de Melo 

This is:

Acked-by: Joonas Lahtinen 

I assume this'll be merged through tip tree along the other changes.

Regards, Joonas

> ---
>  tools/include/uapi/drm/i915_drm.h | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/tools/include/uapi/drm/i915_drm.h 
> b/tools/include/uapi/drm/i915_drm.h
> index a4446f452040..298b2e197744 100644
> --- a/tools/include/uapi/drm/i915_drm.h
> +++ b/tools/include/uapi/drm/i915_drm.h
> @@ -412,6 +412,14 @@ typedef struct drm_i915_irq_wait {
> int irq_seq;
>  } drm_i915_irq_wait_t;
>  
> +/*
> + * Different modes of per-process Graphics Translation Table,
> + * see I915_PARAM_HAS_ALIASING_PPGTT
> + */
> +#define I915_GEM_PPGTT_NONE0
> +#define I915_GEM_PPGTT_ALIASING1
> +#define I915_GEM_PPGTT_FULL2
> +
>  /* Ioctl to query kernel params:
>   */
>  #define I915_PARAM_IRQ_ACTIVE1
> -- 
> 2.20.1
> 

Re: [regression from v4.19] Re: 4.20.0-rc6-next-20181210, v4.20-rc1: list_del corruption on thinkpad x220, graphics related?

[Joonas Lahtinen]

Quoting Pavel Machek (2018-12-27 10:34:39)
> Hi!
> 
> > > > > If you think it is useful, I can try to update my machine to
> > > > > linux-next.
> > > > 
> > > > linux-next is closer to drm-tip, so it's better. Do you have some
> > > > specific reason for not wanting to run drm-tip (but linux-next is still
> > > > ok)?
> > > 
> > > I already have build/update scripts for -next, and I trust -next not
> > > to store screenshots of my desktop in my master boot record :-).
> > > 
> > > Anyway, it does happen with -next. This time, chromiums were running,
> > > and crash happened minute? after I exited flightgear. It can be seen
> > > in the logs.
> > > 
> > > Oh and I might want to mention -- machine was rather deep in swap this
> > > time, as in "mouse jumping when starting fgfs" and "could feel the
> > > chromium being swapped back in". I might have had this situation
> > > before, and just powercycled the machine "because it is so deep in
> > > swap that it will not recover".
> > > 
> > > top says:
> > > 
> > > top - 19:18:24 up 2 days,  8:03,  2 users,  load average: 3.02, 3.45,
> > > 3.21
> > > Tasks: 141 total,   1 running,  86 sleeping,   0 stopped,   2 zombie
> > > %Cpu(s): 18.8 us,  7.6 sy,  3.0 ni, 68.4 id,  1.3 wa,  0.0 hi,  0.9
> > > si,  0.0 st
> > > KiB Mem:   5967968 total,   663244 used,  5304724 free,48876
> > > buffers
> > > KiB Swap:  1681428 total,   170904 used,  1510524 free.   446280
> > > cached Mem
> > > 
> > > but of course that memory is free once everything died.
> > > 
> > > Any ideas? Should I go back to v4.19 to see if it happens there, too?
> > 
> > linux-next includes very much the same code as drm-tip. There's nobody
> > magically reviewing the code more than it is reviewed for inclusion into
> > drm-tip, when it is fed into linux-next. So thinking linux-next would be
> > some way safer is an illusion.
> > 
> > It sounds like having memory pressure expedites the corruption, which
> > should make it easier to reproduce and thus fix.
> > 
> > So if you could please try drm-tip reproducing AND open a bug in Bugzilla.
> > If you are unwilling to do that, it is very difficult to help you
> > more.
> 
> Website says I have to read and agree to two different pieces of
> legalesee, and I'd need to keep track of yet another password... so
> you can "communicate" with me.
> 
> But you can already communicate with me, over email.

I've listed all the reasons why our bug handling process is what it is.

If registering to the Bugzilla is too much of an effort for you, then I
won't be able to help you further on this.

Regards, Joonas

> I verified v4.19 is stable -- it worked ok for way more than two days
> it usually takes to crash.
> 
> Pavel
> -- 
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) 
> http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: iommu_intel or i915 regression in 4.18, 4.19.12 and drm-tip

[Joonas Lahtinen]

Quoting Eric Wong (2018-12-27 13:49:48)
> I just got a used Thinkpad X201 (Core i5 M 520, Intel QM57
> chipset) and hit some kernel panics while trying to view
> image/animation-intensive stuff in Firefox (X11) unless I use
> "iommu_intel=igfx_off".
> 
> With Debian stable backport kernels, "linux-image-4.17.0-0.bpo.3-amd64"
> (4.17.17-1~bpo9+1) has no problems.  But "linux-image-4.18.0-0.bpo.3-amd64"
> (4.18.20-2~bpo9+1) gives a blank screen before I can login via agetty
> and run startx.

Could you open a new bug at (and attach relevant information there):

https://01.org/linuxgraphics/documentation/how-report-bugs

Most confusing about this is that 4.17 would have worked to begin with,
without intel_iommu=igfx_off (unless it was the default for older
kernel?)

Did you maybe update other parts of the system while updating the
kernel?

If you could attach full boot dmesg from working and non-working kernel +
have config file of both kernel's in Bugzilla. That'd be a good start!

Regards, Joonas

> Building 4.19.12 myself got me into X11 and able to start
> Firefox to panic the kernel.  I also updated to the latest BIOS
> (1.40), but it's an EOL laptop (but it's still the most powerful
> laptop I use).  I intend to replace the BIOS with Coreboot soon...
> 
> Initially, I thought I was hitting another GPU hang from 4.18+:
> 
> https://bugs.freedesktop.org/show_bug.cgi?id=107945
> 
> But building drm-tip @ commit 28bb1fc015cedadf3b099b8bd0bb27609849f362
> ("drm-tip: 2018y-12m-25d-08h-12m-37s UTC integration manifest")
> I was still able to reproduce the panic unless I use iommu_intel=igfx_off
> "i915.reset=1" did not help matters, either.
> 
> Below is what I got from netconsole while on drm-tip:
> 
> Kernel panic - not syncing: DMAR hardware is malfunctioning
> Shutting down cpus with NMI
> Kernel Offset: disabled
> ---[ end Kernel panic - not syncing: DMAR hardware is malfunctioning ]---
> [ cut here ]
> sched: Unexpected reschedule of offline CPU#3!
> WARNING: CPU: 0 PID: 105 at native_smp_send_reschedule+0x34/0x40
> Modules linked in: netconsole ccm snd_hda_codec_hdmi snd_hda_codec_conexant 
> snd_hda_codec_generic intel_powerclamp coretemp kvm_intel kvm irqbypass 
> crc32_pclmul crc32c_intel ghash_clmulni_intel arc4 iwldvm aesni_intel 
> aes_x86_64 crypto_simd cryptd mac80211 glue_helper intel_cstate iwlwifi 
> intel_uncore i915 intel_gtt i2c_algo_bit iosf_mbi drm_kms_helper cfbfillrect 
> syscopyarea intel_ips cfbimgblt sysfillrect sysimgblt fb_sys_fops cfbcopyarea 
> thinkpad_acpi prime_numbers cfg80211 ledtrig_audio i2c_i801 sg snd_hda_intel 
> led_class snd_hda_codec drm ac drm_panel_orientation_quirks snd_hwdep battery 
> e1000e agpgart snd_hda_core snd_pcm snd_timer ptp snd soundcore pps_core 
> ehci_pci ehci_hcd lpc_ich video mfd_core button acpi_cpufreq ecryptfs 
> ip_tables x_tables ipv6 evdev thermal [last unloaded: netconsole]
> CPU: 0 PID: 105 Comm: kworker/u8:3 Not tainted 4.20.0-rc7b1+ #1
> Hardware name: LENOVO 3680FBU/3680FBU, BIOS 6QET70WW (1.40 ) 10/11/2012
> Workqueue: i915 __i915_gem_free_work [i915]
> RIP: 0010:native_smp_send_reschedule+0x34/0x40
> Code: 05 69 c6 c9 00 73 15 48 8b 05 18 2d b3 00 be fd 00 00 00 48 8b 40 30 e9 
> 9a 58 7d 00 89 fe 48 c7 c7 78 73 af 81 e8 dc c2 01 00 <0f> 0b c3 66 0f 1f 84 
> 00 00 00 00 00 66 66 66 66 90 8b 05 0d 7d df
> RSP: 0018:888075003d98 EFLAGS: 00010092
> RAX: 002e RBX: 8880751a0740 RCX: 0006
> RDX: 0007 RSI: 0082 RDI: 888075015440
> RBP: 88806e823700 R08:  R09: 888072fc07c0
> R10: 888075003d60 R11: fff5c002 R12: 8880751a0740
> R13: 8880751a0740 R14:  R15: 0003
> FS:  () GS:88807500() knlGS:
> CS:  0010 DS:  ES:  CR0: 80050033
> CR2: 7fdb1f53f000 CR3: 01c0a004 CR4: 000206f0
> Call Trace:
>  
>  ? check_preempt_curr+0x4e/0x90
>  ? ttwu_do_wakeup.isra.19+0x14/0xf0
>  ? try_to_wake_up+0x323/0x410
>  ? autoremove_wake_function+0xe/0x30
>  ? __wake_up_common+0x8d/0x140
>  ? __wake_up_common_lock+0x6c/0x90
>  ? irq_work_run_list+0x49/0x80
>  ? tick_sched_handle.isra.6+0x50/0x50
>  ? update_process_times+0x3b/0x50
>  ? tick_sched_handle.isra.6+0x30/0x50
>  ? tick_sched_timer+0x3b/0x80
>  ? __hrtimer_run_queues+0xea/0x270
>  ? hrtimer_interrupt+0x101/0x240
>  ? smp_apic_timer_interrupt+0x6a/0x150
>  ? apic_timer_interrupt+0xf/0x20
>  
>  ? panic+0x1ca/0x212
>  ? panic+0x1c7/0x212
>  ? __iommu_flush_iotlb+0x19e/0x1c0
>  ? iommu_flush_iotlb_psi+0x96/0xf0
>  ? intel_unmap+0xbf/0xf0
>  ? i915_gem_object_put_pages_gtt+0x36/0x220 [i915]
>  ? drm_ht_remove+0x20/0x20 [drm]
>  ? drm_mm_remove_node+0x1ad/0x310 [drm]
>  ? __pm_runtime_resume+0x54/0x70
>  ? __i915_gem_object_unset_pages+0x129/0x170 [i915]
>  ? __i915_gem_object_put_pages+0x70/0xa0 [i915]
>  ? __i915_gem_free_objects+0x245/0x4e0 [i915]
>  ? 

Re: iommu_intel or i915 regression in 4.18, 4.19.12 and drm-tip

[Joonas Lahtinen]

Quoting Eric Wong (2019-01-04 03:06:26)
> Joonas Lahtinen  wrote:
> > Quoting Eric Wong (2018-12-27 13:49:48)
> > > I just got a used Thinkpad X201 (Core i5 M 520, Intel QM57
> > > chipset) and hit some kernel panics while trying to view
> > > image/animation-intensive stuff in Firefox (X11) unless I use
> > > "iommu_intel=igfx_off".
> > > 
> > > With Debian stable backport kernels, "linux-image-4.17.0-0.bpo.3-amd64"
> > > (4.17.17-1~bpo9+1) has no problems.  But 
> > > "linux-image-4.18.0-0.bpo.3-amd64"
> > > (4.18.20-2~bpo9+1) gives a blank screen before I can login via agetty
> > > and run startx.
> 
> > Most confusing about this is that 4.17 would have worked to begin with,
> > without intel_iommu=igfx_off (unless it was the default for older
> > kernel?)
> 
> Yeah, so the Debian bpo 4.17(.17) kernel did not set
> CONFIG_INTEL_IOMMU_DEFAULT_ON, so I didn't encounter problems.
> My self-built kernels all set CONFIG_INTEL_IOMMU_DEFAULT_ON.

So it's the case that IOMMU never worked on your machine.

My recommendation would be to simply use intel_iommu=igfx_off if you
need IOMMU.

Old hardware is known to have issues with IOMMU, and retroactively
enabling IOMMU on those machines just brings them up :/

Regards, Joonas

> Booting the Debian 4.17 kernel with "intel_iommu=on" gives the
> same hanging problem I hit with self-built 4.19.{12,13} kernels.
> 
> I'm not sure how far back the problem goes (maybe forever),
> since I only got this hardware.  Not sure what's the problem
> with Debian 4.18, either; but (self-built) 4.19.13 is fine w/o
> CONFIG_INTEL_IOMMU_DEFAULT_ON.
> 
> Debian backports doesn't have kernels for 4.19 or 4.20, yet.
> 
> > Did you maybe update other parts of the system while updating the
> > kernel?
> 
> Definitely not; just the kernel + headers ("make bindeb-pkg)".
> 
> > If you could attach full boot dmesg from working and non-working kernel +
> > have config file of both kernel's in Bugzilla. That'd be a good start!
> 
> Sorry, I get anxiety attacks when it comes to logins and forms.
> Anyways, I managed to get the Debian kernel dmesg output uploaded
> with and without iommu_intel=on:
> https://bugs.freedesktop.org/attachment.cgi?bugid=109219

Re: v4.20-rc1: list_del corruption on thinkpad x220, graphics related?

[Joonas Lahtinen]

On Sat, 2018-12-08 at 12:24 +0100, Pavel Machek wrote:
> On Sat 2018-12-08 12:13:46, Pavel Machek wrote:
> > Hi!
> > 
> > > > > > There's one similar for nouveau in Bugzilla, but it seems like a 
> > > > > > genuine
> > > > > > memory corruption (1 bit flipped):
> > > > > > 
> > > > > > https://bugs.freedesktop.org/show_bug.cgi?id=84880
> > > > > > 
> > > > > > Any extra information would be of use :)
> > > > > > 
> > > > > > Regards, Joonas
> > > > > > 
> > > > > > PS. Could you open a bug to Bugzilla, it'll help to collect the
> > > > > > information in one consolidated place:
> > > > > > 
> > > > > > https://01.org/linuxgraphics/documentation/how-report-bugs
> > > > > 
> > > > > I prefer email... certainly for bugs that can't be reproduced.
> > > > 
> > > > By adding it to the Bugzilla it may be recognized by somebody else
> > > > who is experiencing a similar issue. Internet points are not deducted
> > > > for submitting bugs in good faith, even if they get closed as
> > > > NOTABUG.
> > 
> > Well, your documentation suggests you'll deduce my internet points:
> > 
> > Before filing the bug, please try to reproduce your issue with the
> > latest kernel. Use the latest drm-tip branch from
> > http://cgit.freedesktop.org/drm-tip and build as instructed on our
> > Build Guide.
> > 
> > :-)
> 
> I'd prefer not to run drm-tip. I'll update to 2.6.20-rc5+ and see if
> it re-appears (but it takes long time to reproduce :-().

If we can or can not reproduce the issue with drm-tip, is a very useful
datapoint for us. If we can not reproduce, it'll be possible to bisect
which commit fixed it, and backport that. On the other hand, if it's
still reproducible, we know we're not spending time on something we
already fixed, and the priority gets a bump.

> If you think it is useful, I can try to update my machine to
> linux-next.

linux-next is closer to drm-tip, so it's better. Do you have some
specific reason for not wanting to run drm-tip (but linux-next is still
ok)?

Regards, Joonas

> 
> Best regards,
>   Pavel
> 
-- 
Joonas Lahtinen
Open Source Graphics Center
Intel Corporation

Re: v4.20-rc5+ on x220: Resetting chip for hang on rcs0

[Joonas Lahtinen]

On Sun, 2018-12-09 at 12:18 +0100, Pavel Machek wrote:
> Hi!
> 
> Another day, another problem... but this one is different from the
> previous hang, as machine survives.

Please, file a bug. It says so even in the splat...

Regards, Joonas

> 
> Chromium was running with youtube video playing.
> 
> [31850.666274] [drm] GPU hangs can indicate a bug anywhere in the
> entire gfx stack, including userspace.
> [31850.666277] [drm] Please file a _new_ bug report on
> bugs.freedesktop.org against DRI -> DRM/Intel
> [31850.666279] [drm] drm/i915 developers can then reassign to the
> right component if it's not a kernel issue.
> [31850.666282] [drm] The gpu crash dump is required to analyze gpu
> hangs, so please always attach it.
> [31850.666285] [drm] GPU crash dump saved to
> /sys/class/drm/card0/error
> [31850.666394] i915 :00:02.0: Resetting chip for hang on rcs0
> [31850.668474] WARNING: CPU: 0 PID: 13675 at
> /data/fast/l/k/include/linux/dma-fence.h:503
> i915_request_skip+0x71/0x80
> [31850.668478] Modules linked in:
> [31850.668484] CPU: 0 PID: 13675 Comm: kworker/0:3 Not tainted
> 4.20.0-rc5+ #5
> [31850.668487] Hardware name: LENOVO 42872WU/42872WU, BIOS 8DET74WW
> (1.44 ) 03/13/2018
> 
> Dmesg and /sys/class/drm/card0/error are attached.
> 
> Best regards,
>   Pavel
-- 
Joonas Lahtinen
Open Source Graphics Center
Intel Corporation

Re: [PATCH 1/3] drm/i915: allocate mock file pointer dynamically

[Joonas Lahtinen]

On ma, 2017-03-20 at 10:40 +0100, Arnd Bergmann wrote:
> A struct file is a bit too large to put on the kernel stack in general
> and triggers a warning for low settings of CONFIG_FRAME_WARN:
> 
> drivers/gpu/drm/i915/selftests/mock_drm.c: In function 'mock_file':
> drivers/gpu/drm/i915/selftests/mock_drm.c:46:1: error: the frame size of 1328 
> bytes is larger than 1280 bytes [-Werror=frame-larger-than=]
> drivers/gpu/drm/i915/selftests/mock_drm.c: In function 'mock_file_free':
> drivers/gpu/drm/i915/selftests/mock_drm.c:54:1: error: the frame size of 1312 
> bytes is larger than 1280 bytes [-Werror=frame-larger-than=]
> 
> It's also slightly dangerous to leave a reference to a stack object
> in the drm_file structure after leaving the stack frame.
> This changes the code to just allocate the object dynamically
> and release it when we are done with it.
> 
> Fixes: 66d9cb5d805a ("drm/i915: Mock the GEM device for self-testing")
> Signed-off-by: Arnd Bergmann 



> ---
>  drivers/gpu/drm/i915/selftests/mock_drm.c | 12 +++-
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/selftests/mock_drm.c 
> b/drivers/gpu/drm/i915/selftests/mock_drm.c
> index 113dec05c7dc..18514065c93d 100644
> --- a/drivers/gpu/drm/i915/selftests/mock_drm.c
> +++ b/drivers/gpu/drm/i915/selftests/mock_drm.c
> @@ -32,15 +32,15 @@ static inline struct inode fake_inode(struct 
> drm_i915_private *i915)
>  struct drm_file *mock_file(struct drm_i915_private *i915)
>  {
> >     struct inode inode = fake_inode(i915);
> > -   struct file filp = {};
> > +   struct file *filp = kzalloc(sizeof(struct file), GFP_KERNEL);
> >     struct drm_file *file;
> >     int err;
>  

filp = kzalloc(sizeof(*filp), GFP_KERNEL);
if (unlikely(!filp)) {
err = -ENOMEM;
    goto err;
}

And appropriate onion teardown in case drm_open fails, so that we don't
leak memory.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: Q. drm/i915 shrinker, synchronize_rcu_expedited() from handlers

[Joonas Lahtinen]

On ti, 2017-05-30 at 13:00 -0700, Hugh Dickins wrote:
> On Mon, 22 May 2017, Joonas Lahtinen wrote:
> > On la, 2017-05-20 at 10:56 +0900, J. R. Okajima wrote:
> > > "J. R. Okajima":
> > > > 
> > > > I don't know whether the fix is good to me or not yet. I will test your
> > > > fix, but I am busy now and my test will be a few weeks later. Other
> > > > people may want the fix soon. So I'd suggest you to reproduce the
> > > > problem on your side. I guess "mem=1G" or "mem=512M" will make it easier
> > > > to reproduce the problem.
> > > > Of course, if you are sure the fix is correct, then you don't have to
> > > > wait for my test. Release it soon for other people.
> > > 
> > > Now I am going to able to run my local test.
> > > But V3 patch failed to apply onto v4.11.0.
> > > 
> > > Would you provide us two versions of the patch, one for v4.11 series and
> > > the other of v4.12-rcN?
> > 
> > For v4.12-rc2 the backport is here:
> > 
> > https://patchwork.freedesktop.org/patch/156990/
> 
> This fix seems to have got lost: we've been waiting a month,
> and 4.12 is now at rc3: please expedite the unexpedited :)

Don't worry, it's not lost. It was merged to drm-intel-fixes and thus is in the 
pipeline.

There were some unexpected delays getting fixes in, sorry for that.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: Q. drm/i915 shrinker, synchronize_rcu_expedited() from handlers

[Joonas Lahtinen]

On ma, 2017-05-01 at 11:05 +0900, J. R. Okajima wrote:
> Thanx for the reply.
> 
> Andrea Arcangeli:
> > 
> > Yes I already reported this, my original fix was way more efficient
> > (and also safer considering the above) than what landed upstream. My
> > feedback was ignored though.
> > 
> > https://lists.freedesktop.org/archives/intel-gfx/2017-April/125414.html
> 
> I see.
> Actually on my test system for v4.11-rc8, kthreadd, kworker, kswapd and
> others all stopped working due to the synchronize_rcu_expedited call
> from i915_gem_shrinker_count. It is definitly a show stopper for me as
> an i915 user.

Filing a bug in freedesktop.org with all the details is the fastest way
of getting help. Without the bug (and with such little information as
the previous e-mail) it's hard to estimate the extent and nature of the
bug.

I've anyway gone and prepared a patch to drop the RCU sync completely
from shrinker phase, as discussed originally with Chris.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: Q. drm/i915 shrinker, synchronize_rcu_expedited() from handlers

[Joonas Lahtinen]

On pe, 2017-05-05 at 14:57 -0700, Hugh Dickins wrote:
> On Fri, 5 May 2017, Joonas Lahtinen wrote:
> > On ma, 2017-05-01 at 11:05 +0900, J. R. Okajima wrote:
> > > Thanx for the reply.
> > > 
> > > Andrea Arcangeli:
> > > > 
> > > > Yes I already reported this, my original fix was way more efficient
> > > > (and also safer considering the above) than what landed upstream. My
> > > > feedback was ignored though.
> > > > 
> > > > https://lists.freedesktop.org/archives/intel-gfx/2017-April/125414.html
> > > 
> > > I see.
> > > Actually on my test system for v4.11-rc8, kthreadd, kworker, kswapd and
> > > others all stopped working due to the synchronize_rcu_expedited call
> > > from i915_gem_shrinker_count. It is definitly a show stopper for me as
> > > an i915 user.
> > 
> > Filing a bug in freedesktop.org with all the details is the fastest way
> > of getting help. Without the bug (and with such little information as
> > the previous e-mail) it's hard to estimate the extent and nature of the
> > bug.
> > 
> > I've anyway gone and prepared a patch to drop the RCU sync completely
> > from shrinker phase, as discussed originally with Chris.
> 
> Is that a patch that will be suitable for 4.11-stable?  Please do post
> it here.  I had not experienced this i915-induced hang at all when
> Andrea first mentioned it, nor even on 4.11-rc8; but now with 4.11
> final I can get it fairly easily (I haven't tried Andrea's fix yet).

Please try:

https://patchwork.freedesktop.org/patch/154713/

If it works, a Tested-by: would be appreciated.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: Q. drm/i915 shrinker, synchronize_rcu_expedited() from handlers

[Joonas Lahtinen]

On la, 2017-05-20 at 10:56 +0900, J. R. Okajima wrote:
> "J. R. Okajima":
> > 
> > I don't know whether the fix is good to me or not yet. I will test your
> > fix, but I am busy now and my test will be a few weeks later. Other
> > people may want the fix soon. So I'd suggest you to reproduce the
> > problem on your side. I guess "mem=1G" or "mem=512M" will make it easier
> > to reproduce the problem.
> > Of course, if you are sure the fix is correct, then you don't have to
> > wait for my test. Release it soon for other people.
> 
> Now I am going to able to run my local test.
> But V3 patch failed to apply onto v4.11.0.
> 
> Would you provide us two versions of the patch, one for v4.11 series and
> the other of v4.12-rcN?

For v4.12-rc2 the backport is here:

https://patchwork.freedesktop.org/patch/156990/

For quick testing on older kernels, just remove all lines with "_rcu"
in drivers/gpu/drm/i915/i915_gem_shrinker.c .

Regards, Joonas

PS: It'll help to subscribe to and track our mailing list at
intel-...@lists.freedesktop.org for future convenience.
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: Q. drm/i915 shrinker, synchronize_rcu_expedited() from handlers

[Joonas Lahtinen]

On ti, 2017-05-09 at 20:04 -0700, Hugh Dickins wrote:
> On Mon, 8 May 2017, Joonas Lahtinen wrote:
> > On pe, 2017-05-05 at 14:57 -0700, Hugh Dickins wrote:
> > > On Fri, 5 May 2017, Joonas Lahtinen wrote:
> > > > On ma, 2017-05-01 at 11:05 +0900, J. R. Okajima wrote:
> > > > > Thanx for the reply.
> > > > > 
> > > > > Andrea Arcangeli:
> > > > > > 
> > > > > > Yes I already reported this, my original fix was way more efficient
> > > > > > (and also safer considering the above) than what landed upstream. My
> > > > > > feedback was ignored though.
> > > > > > 
> > > > > > https://lists.freedesktop.org/archives/intel-gfx/2017-April/125414.html
> > > > > 
> > > > > I see.
> > > > > Actually on my test system for v4.11-rc8, kthreadd, kworker, kswapd 
> > > > > and
> > > > > others all stopped working due to the synchronize_rcu_expedited call
> > > > > from i915_gem_shrinker_count. It is definitly a show stopper for me as
> > > > > an i915 user.
> > > > 
> > > > Filing a bug in freedesktop.org with all the details is the fastest way
> > > > of getting help. Without the bug (and with such little information as
> > > > the previous e-mail) it's hard to estimate the extent and nature of the
> > > > bug.
> > > > 
> > > > I've anyway gone and prepared a patch to drop the RCU sync completely
> > > > from shrinker phase, as discussed originally with Chris.
> > > 
> > > Is that a patch that will be suitable for 4.11-stable?  Please do post
> > > it here.  I had not experienced this i915-induced hang at all when
> > > Andrea first mentioned it, nor even on 4.11-rc8; but now with 4.11
> > > final I can get it fairly easily (I haven't tried Andrea's fix yet).
> > 
> > Please try:
> > 
> > https://patchwork.freedesktop.org/patch/154713/
> > 
> > If it works, a Tested-by: would be appreciated.
> 
> Yes, that works for me, thank you.
> 
> Tested-by: Hugh Dickins 
> 
> But the linked patch seems to be lacking a Reported-by (not me) tag,
> a Fixes tag, a Cc stable tag, and any indication in the Subject or
> commit message that this patch is something needed to fix hangs
> observed by several people - it just sounds like a minor cleanup.

It is a patch that was agreed to be pushed anyway, so if it wouldn't
have resolved the problem, I'd have pushed it as is.

I'll add J. R. Okajima as Reported-by and refer to the bisected commit,
even though so far Freedesktop Bugzilla or intel-gfx mailing list has
no other reports.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: Q. drm/i915 shrinker, synchronize_rcu_expedited() from handlers

[Joonas Lahtinen]

On ke, 2017-05-10 at 12:43 +0200, Andrea Arcangeli wrote:
> It works for me too. I'm running my workstation also with
> synchronize_rcu removed from i915_gem_shrink_all in addition to the
> above. Isn't the oom method invoked from reclaim context too? As far
> as I can tell synchronize_rcu can end up throttling on a background
> synchronize_rcu_expedited(), so it might end up in the same issue
> unless removed too.

Thanks for testing and spotting my bad grepping, I'll add your T-b and
s
end v3.

Regards, Joonas

> Tested-by: Andrea Arcangeli 
> 
> (I can't reproduce the lockups 100% of the time, but they never
> happened again with this patch and I happened to run the load that
> reproduces them a couple of times already with v4.11 and this patch
> applied)
> 
> It's also certainly improving performance by removing the
> synchronize_rcu_expedited from the _count methods where it was useless
> (in addition to unsafe).
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: [Intel-gfx] drm/i915: WARN_ON_ONCE(!crtc_clock || cdclk < crtc_clock)

[Joonas Lahtinen]

On ke, 2016-10-12 at 11:56 +0200, Paul Bolle wrote:
> On a laptop that tracks the latest stable release (Ie, it now runs
> v4.8.1) I see this WARNING
>     WARN_ON_ONCE(!crtc_clock || cdclk < crtc_clock)
> 
> Full trace pasted below. I never saw this WARNING before v4.8. Since
> v4.8 I've had it in all (four, actually) boots.
> 
> What am I expected to do about this WARNING?
> 

Bisecting the offending commit between v4.8 and v4.8.1 would be a good
start.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: [Intel-gfx] [PATCH 1/5] i915: avoid kernel hang caused by synchronize rcu struct_mutex deadlock

[Joonas Lahtinen]

On pe, 2017-04-07 at 01:23 +0200, Andrea Arcangeli wrote:
> synchronize_rcu/synchronize_sched/synchronize_rcu_expedited() will
> hang until its own workqueues are run. The i915 gem workqueues will
> wait on the struct_mutex to be released. So we cannot wait for a
> quiescent state using those rcu primitives while holding the
> struct_mutex or it creates a circular lock dependency resulting in
> kernel hangs (which is reproducible but goes undetected by lockdep).
> 
> This started in commit 3d3d18f086cdda72ee18a454db70ca72c6e3246c and
> lockdep didn't detect it apparently.

The right format is;

Fixes: 3d3d18f086cd ("drm/i915: Avoid rcu_barrier() from reclaim paths 
(shrinker)")

> @@ -324,6 +320,16 @@ i915_gem_shrinker_scan(struct shrinker *shrinker, struct 
> shrink_control *sc)
>   if (unlock)
>   mutex_unlock(>struct_mutex);
>  
> + if (likely(__mutex_owner(>struct_mutex) != current))

This check can be dropped and synchronize_rcu_expedited() should be
embedded directly to the if (unlock) branch as it's functionally
equivalent. This can be applied to all the unlock cases, not just this
one. That should be the correct action to avoid the deadlock. I've sent
a patch to do this (Cc'd you), can you verify that it gets rid of the
problem for you?

> + /*
> +  * If reclaim was invoked by an allocation done while
> +  * holding the struct mutex, we cannot call
> +  * synchronize_rcu_expedited() as it depends on
> +  * workqueues to run but the running workqueue may be
> +  * blocked waiting on us to release struct_mutex.
> +  */
> + synchronize_rcu_expedited();
> +
>   return freed;
>  }
>  
> ___
> Intel-gfx mailing list
> intel-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation