Re: [PATCH] drm/i915/gvt: remove useless function

[Zhenyu Wang]

On 2021.04.13 14:18:48 +0800, Jiapeng Chong wrote:
> Fix the following clang warning:
> 
> drivers/gpu/drm/i915/gvt/gtt.c:590:20: warning: unused function
> 'ppgtt_set_guest_root_entry' [-Wunused-function].
> 
> Reported-by: Abaci Robot 
> Signed-off-by: Jiapeng Chong 
> ---
>  drivers/gpu/drm/i915/gvt/gtt.c | 6 --
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
> index 897c007..a01ff44 100644
> --- a/drivers/gpu/drm/i915/gvt/gtt.c
> +++ b/drivers/gpu/drm/i915/gvt/gtt.c
> @@ -587,12 +587,6 @@ static void _ppgtt_set_root_entry(struct intel_vgpu_mm 
> *mm,
>  entry, index, false, 0, mm->vgpu);
>  }
>  
> -static inline void ppgtt_set_guest_root_entry(struct intel_vgpu_mm *mm,
> - struct intel_gvt_gtt_entry *entry, unsigned long index)
> -{
> - _ppgtt_set_root_entry(mm, entry, index, true);
> -}
> -
>  static inline void ppgtt_set_shadow_root_entry(struct intel_vgpu_mm *mm,
>   struct intel_gvt_gtt_entry *entry, unsigned long index)
>  {
> -- 

Reviewed-by: Zhenyu Wang 

Thanks for covering me on this! Queue this up.


signature.asc
Description: PGP signature

Re: Regression: gvt: vgpu 1: MI_LOAD_REGISTER_MEM handler error

[Zhenyu Wang]

On 2021.04.12 19:23:47 -0600, Alex Williamson wrote:
> On Mon, 12 Apr 2021 10:32:14 -0600
> Alex Williamson  wrote:
> 
> > Running a Windows guest on a i915-GVTg_V4_2 from an HD 5500 IGD on
> > v5.12-rc6 results in host logs:
> > 
> > gvt: vgpu 1: lrm access to register (20c0)
> > gvt: vgpu 1: MI_LOAD_REGISTER_MEM handler error
> > gvt: vgpu 1: cmd parser error
> > 0x0 
> > 0x29 
> > 
> > gvt: vgpu 1: scan wa ctx error
> > gvt: vgpu 1: failed to submit desc 0
> > gvt: vgpu 1: fail submit workload on ring rcs0
> > gvt: vgpu 1: fail to emulate MMIO write 2230 len 4
> > 
> > The guest goes into a boot loop triggering this error before reaching
> > the desktop and rebooting.  Guest using Intel driver 20.19.15.5171
> > dated 11/4/2020 (from driver file 15.40.5171).
> > 
> > This VM works well with the same guest and userspace software stack on
> > Fedora's kernel 5.11.11-200.fc33.x86_64.  Thanks,
> 
> Bisected to:

Looks we didn't hit this one on Broadwell with recent testing. I'll double
check, maybe Broadwell missed something after our cmd parser rework.

Thanks for reporting!

> 
> commit f18d417a57438498e0de481d3a0bc900c2b0e057
> Author: Yan Zhao 
> Date:   Wed Dec 23 11:45:08 2020 +0800
> 
> drm/i915/gvt: filter cmds "srm" and "lrm" in cmd_handler
> 
> do not allow "srm" and "lrm" except for GEN8_L3SQCREG4 and 0x21f0.
> 
> Cc: Colin Xu 
> Cc: Kevin Tian 
> Signed-off-by: Yan Zhao 
> Signed-off-by: Zhenyu Wang 
> Link: 
> http://patchwork.freedesktop.org/patch/msgid/20201223034508.17031-1-yan.y.z...@intel.com
> Reviewed-by: Zhenyu Wang 
> 


signature.asc
Description: PGP signature

Re: drivers/gpu/drm/i915/gvt/gtt.c:267:19: error: unused function 'get_pt_type'

[Zhenyu Wang]

On 2021.03.23 15:15:29 -0700, Nick Desaulniers wrote:
> On Fri, Mar 19, 2021 at 11:45 PM kernel test robot  wrote:
> >
> > Hi Nick,
> >
> > FYI, the error/warning still remains.
> >
> > tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 
> > master
> > head:   1c273e10bc0cc7efb933e0ca10e260cdfc9f0b8c
> > commit: 9f4069b055d1508c833115df7493b6e0001e5c9b drm/i915: re-disable 
> > -Wframe-address
> 
> This in unrelated to my change.
> 
> + Changbin, Zhenyu (authors of 3aff3512802) and Zhi (author of
> 054f4eba2a298) in case there's any interest in fixing this up.
> Otherwise I don't think these tiny helpful functions were meant to be
> used somewhere but are not, so there's not much value in cleaning them
> up.

I'll check that, should be some left over last big gtt code refactor.
Looks lkp guys don't apply -Wunused-function for gvt tree build test...

Thanks

> 
> > date:   11 months ago
> > config: x86_64-randconfig-a016-20210319 (attached as .config)
> > compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 
> > fcc1ce00931751ac02498986feb37744e9ace8de)
> > reproduce (this is a W=1 build):
> > wget 
> > https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
> > ~/bin/make.cross
> > chmod +x ~/bin/make.cross
> > # install x86_64 cross compiling tool for clang build
> > # apt-get install binutils-x86-64-linux-gnu
> > # 
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9f4069b055d1508c833115df7493b6e0001e5c9b
> > git remote add linus 
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> > git fetch --no-tags linus master
> > git checkout 9f4069b055d1508c833115df7493b6e0001e5c9b
> > # save the attached .config to linux build tree
> > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross 
> > ARCH=x86_64
> >
> > If you fix the issue, kindly add following tag as appropriate
> > Reported-by: kernel test robot 
> >
> > All errors (new ones prefixed by >>):
> >
> > >> drivers/gpu/drm/i915/gvt/gtt.c:267:19: error: unused function 
> > >> 'get_pt_type' [-Werror,-Wunused-function]
> >static inline int get_pt_type(int type)
> >  ^
> > >> drivers/gpu/drm/i915/gvt/gtt.c:590:20: error: unused function 
> > >> 'ppgtt_set_guest_root_entry' [-Werror,-Wunused-function]
> >static inline void ppgtt_set_guest_root_entry(struct intel_vgpu_mm *mm,
> >   ^
> >2 errors generated.
> >
> >
> > vim +/get_pt_type +267 drivers/gpu/drm/i915/gvt/gtt.c
> >
> > 2707e44466881d6 Zhi Wang 2016-03-28  266
> > 054f4eba2a2985b Zhi Wang 2017-10-10 @267  static inline int get_pt_type(int 
> > type)
> > 054f4eba2a2985b Zhi Wang 2017-10-10  268  {
> > 054f4eba2a2985b Zhi Wang 2017-10-10  269return 
> > gtt_type_table[type].pt_type;
> > 054f4eba2a2985b Zhi Wang 2017-10-10  270  }
> > 054f4eba2a2985b Zhi Wang 2017-10-10  271
> >
> > :: The code at line 267 was first introduced by commit
> > :: 054f4eba2a2985b1db43353b7b5ce90e96cf9bb9 drm/i915/gvt: Introduce 
> > page table type of current level in GTT type enumerations
> >
> > :: TO: Zhi Wang 
> > :: CC: Zhenyu Wang 
> >
> > ---
> > 0-DAY CI Kernel Test Service, Intel Corporation
> > https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
> 
> 
> 
> -- 
> Thanks,
> ~Nick Desaulniers


signature.asc
Description: PGP signature

Re: [PATCH] drivers: gnu: drm: i915: gvt: Fixed couple of spellings in the file gtt.c

[Zhenyu Wang]

On 2021.02.22 06:22:37 -0800, Randy Dunlap wrote:
> On 2/22/21 6:21 AM, Randy Dunlap wrote:
> > On 2/22/21 12:18 AM, Bhaskar Chowdhury wrote:
> >>
> >> s/negtive/negative/
> >> s/possilbe/possible/
> >>
> >> Signed-off-by: Bhaskar Chowdhury 
> > 
> > Acked-by: Randy Dunlap 
> 
> except the Subject has a typo in it.
> s/gnu/gpu/
>

And pls follow gvt subsys's subject line as drm/i915/gvt: xxx in future.
I'll fix the title and queue this.

Thanks!

> >> ---
> >>  drivers/gpu/drm/i915/gvt/gtt.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/gvt/gtt.c 
> >> b/drivers/gpu/drm/i915/gvt/gtt.c
> >> index 897c007ea96a..dc5834bf4de2 100644
> >> --- a/drivers/gpu/drm/i915/gvt/gtt.c
> >> +++ b/drivers/gpu/drm/i915/gvt/gtt.c
> >> @@ -1159,8 +1159,8 @@ static inline void 
> >> ppgtt_generate_shadow_entry(struct intel_gvt_gtt_entry *se,
> >>   * @vgpu: target vgpu
> >>   * @entry: target pfn's gtt entry
> >>   *
> >> - * Return 1 if 2MB huge gtt shadowing is possilbe, 0 if miscondition,
> >> - * negtive if found err.
> >> + * Return 1 if 2MB huge gtt shadowing is possible, 0 if miscondition,
> >> + * negative if found err.
> >>   */
> >>  static int is_2MB_gtt_possible(struct intel_vgpu *vgpu,
> >>struct intel_gvt_gtt_entry *entry)
> >> --
> > 
> > 
> 
> 
> -- 
> ~Randy
> Reported-by: Randy Dunlap 
> ___
> intel-gvt-dev mailing list
> intel-gvt-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev


signature.asc
Description: PGP signature

Re: [PATCH] drm/i915/gvt/kvmgt: Fix the build failure in kvmgt.

[Zhenyu Wang]

On 2021.02.09 02:52:10 +0800, Yu Zhang wrote:
> Previously, commit 531810caa9f4 ("KVM: x86/mmu: Use
> an rwlock for the x86 MMU") replaced KVM's mmu_lock
> with type rwlock_t. This will cause a build failure
> in kvmgt, which uses the same lock when trying to add/
> remove some GFNs to/from the page tracker. Fix it with
> write_lock/unlocks in kvmgt.

Thanks for the fix! I saw Paolo has already carried one
in -next, so we are fine.

> 
> Reported-by: Stephen Rothwell 
> Signed-off-by: Yu Zhang 
> ---
>  drivers/gpu/drm/i915/gvt/kvmgt.c | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c 
> b/drivers/gpu/drm/i915/gvt/kvmgt.c
> index 60f1a386dd06..b4348256ae95 100644
> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> @@ -1703,7 +1703,7 @@ static int kvmgt_page_track_add(unsigned long handle, 
> u64 gfn)
>   return -EINVAL;
>   }
>  
> - spin_lock(>mmu_lock);
> + write_lock(>mmu_lock);
>  
>   if (kvmgt_gfn_is_write_protected(info, gfn))
>   goto out;
> @@ -1712,7 +1712,7 @@ static int kvmgt_page_track_add(unsigned long handle, 
> u64 gfn)
>   kvmgt_protect_table_add(info, gfn);
>  
>  out:
> - spin_unlock(>mmu_lock);
> + write_unlock(>mmu_lock);
>   srcu_read_unlock(>srcu, idx);
>   return 0;
>  }
> @@ -1737,7 +1737,7 @@ static int kvmgt_page_track_remove(unsigned long 
> handle, u64 gfn)
>   return -EINVAL;
>   }
>  
> - spin_lock(>mmu_lock);
> + write_lock(>mmu_lock);
>  
>   if (!kvmgt_gfn_is_write_protected(info, gfn))
>   goto out;
> @@ -1746,7 +1746,7 @@ static int kvmgt_page_track_remove(unsigned long 
> handle, u64 gfn)
>   kvmgt_protect_table_del(info, gfn);
>  
>  out:
> - spin_unlock(>mmu_lock);
> + write_unlock(>mmu_lock);
>   srcu_read_unlock(>srcu, idx);
>   return 0;
>  }
> @@ -1772,7 +1772,7 @@ static void kvmgt_page_track_flush_slot(struct kvm *kvm,
>   struct kvmgt_guest_info *info = container_of(node,
>   struct kvmgt_guest_info, track_node);
>  
> - spin_lock(>mmu_lock);
> + write_lock(>mmu_lock);
>   for (i = 0; i < slot->npages; i++) {
>   gfn = slot->base_gfn + i;
>   if (kvmgt_gfn_is_write_protected(info, gfn)) {
> @@ -1781,7 +1781,7 @@ static void kvmgt_page_track_flush_slot(struct kvm *kvm,
>   kvmgt_protect_table_del(info, gfn);
>   }
>   }
> - spin_unlock(>mmu_lock);
> + write_unlock(>mmu_lock);
>  }
>  
>  static bool __kvmgt_vgpu_exist(struct intel_vgpu *vgpu, struct kvm *kvm)
> -- 
> 2.17.1
> 


signature.asc
Description: PGP signature

Re: [FYI PATCH] i915: kvmgt: the KVM mmu_lock is now an rwlock

[Zhenyu Wang]

On 2021.02.08 06:34:37 -0500, Paolo Bonzini wrote:
> Adjust the KVMGT page tracking callbacks.
> 
> Cc: Zhenyu Wang 
> Cc: Zhi Wang 
> Cc: intel-gvt-...@lists.freedesktop.org
> Cc: intel-...@lists.freedesktop.org
> Signed-off-by: Paolo Bonzini 
> ---

Thanks for that!

Acked-by: Zhenyu Wang 

>  drivers/gpu/drm/i915/gvt/kvmgt.c | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c 
> b/drivers/gpu/drm/i915/gvt/kvmgt.c
> index 60f1a386dd06..b4348256ae95 100644
> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> @@ -1703,7 +1703,7 @@ static int kvmgt_page_track_add(unsigned long handle, 
> u64 gfn)
>   return -EINVAL;
>   }
>  
> - spin_lock(>mmu_lock);
> + write_lock(>mmu_lock);
>  
>   if (kvmgt_gfn_is_write_protected(info, gfn))
>   goto out;
> @@ -1712,7 +1712,7 @@ static int kvmgt_page_track_add(unsigned long handle, 
> u64 gfn)
>   kvmgt_protect_table_add(info, gfn);
>  
>  out:
> - spin_unlock(>mmu_lock);
> + write_unlock(>mmu_lock);
>   srcu_read_unlock(>srcu, idx);
>   return 0;
>  }
> @@ -1737,7 +1737,7 @@ static int kvmgt_page_track_remove(unsigned long 
> handle, u64 gfn)
>   return -EINVAL;
>   }
>  
> - spin_lock(>mmu_lock);
> + write_lock(>mmu_lock);
>  
>   if (!kvmgt_gfn_is_write_protected(info, gfn))
>   goto out;
> @@ -1746,7 +1746,7 @@ static int kvmgt_page_track_remove(unsigned long 
> handle, u64 gfn)
>   kvmgt_protect_table_del(info, gfn);
>  
>  out:
> - spin_unlock(>mmu_lock);
> + write_unlock(>mmu_lock);
>   srcu_read_unlock(>srcu, idx);
>   return 0;
>  }
> @@ -1772,7 +1772,7 @@ static void kvmgt_page_track_flush_slot(struct kvm *kvm,
>   struct kvmgt_guest_info *info = container_of(node,
>   struct kvmgt_guest_info, track_node);
>  
> - spin_lock(>mmu_lock);
> + write_lock(>mmu_lock);
>   for (i = 0; i < slot->npages; i++) {
>   gfn = slot->base_gfn + i;
>   if (kvmgt_gfn_is_write_protected(info, gfn)) {
> @@ -1781,7 +1781,7 @@ static void kvmgt_page_track_flush_slot(struct kvm *kvm,
>   kvmgt_protect_table_del(info, gfn);
>   }
>   }
> - spin_unlock(>mmu_lock);
> + write_unlock(>mmu_lock);
>  }
>  
>  static bool __kvmgt_vgpu_exist(struct intel_vgpu *vgpu, struct kvm *kvm)
> -- 
> 2.26.2
> 
> ___
> intel-gvt-dev mailing list
> intel-gvt-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev


signature.asc
Description: PGP signature

Re: [PATCH] drm/i915/gvt: fix uninitialized return in intel_gvt_update_reg_whitelist()

[Zhenyu Wang]

On 2021.01.25 09:44:53 +, Chris Wilson wrote:
> Quoting Dan Carpenter (2021-01-25 08:48:30)
> > Smatch found an uninitialized variable bug in this code:
> > 
> > drivers/gpu/drm/i915/gvt/cmd_parser.c:3191 
> > intel_gvt_update_reg_whitelist()
> > error: uninitialized symbol 'ret'.
> > 
> > The first thing that Smatch complains about is that "ret" isn't set if
> > we don't enter the "for_each_engine(engine, _priv->gt, id) {" loop.
> > Presumably we always have at least one engine so that's a false
> > positive.
> > 
> > But it's definitely a bug to not set "ret" if i915_gem_object_pin_map()
> > fails.
> 
> True.
>  
> > Let's fix the bug and silence the false positive.
> > 
> > Fixes: 493f30cd086e ("drm/i915/gvt: parse init context to update cmd 
> > accessible reg whitelist")
> > Signed-off-by: Dan Carpenter 
> Reviewed-by: Chris Wilson 

Thanks, Dan & Chris! Queued this up.


signature.asc
Description: PGP signature

Re: [PATCH] drm/i915/gvt: return error when failing to take the module reference

[Zhenyu Wang]

On 2020.11.12 21:22:32 +0800, Xiongfeng Wang wrote:
> When we fail to take the module reference, we go to the 'undo*' branch and
> return. But the returned variable 'ret' has been set as zero by the
> above code. Change 'ret' to '-ENODEV' in this situation.
> 
> Fixes: 9bdb073464d6 ("drm/i915/gvt: Change KVMGT as self load module")
> Reported-by: Hulk Robot 
> Signed-off-by: Xiongfeng Wang 
> ---
>  drivers/gpu/drm/i915/gvt/kvmgt.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c 
> b/drivers/gpu/drm/i915/gvt/kvmgt.c
> index ad8a9df..778eb8c 100644
> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> @@ -829,8 +829,10 @@ static int intel_vgpu_open(struct mdev_device *mdev)
>   /* Take a module reference as mdev core doesn't take
>* a reference for vendor driver.
>*/
> - if (!try_module_get(THIS_MODULE))
> + if (!try_module_get(THIS_MODULE)) {
> + ret = -ENODEV;
>   goto undo_group;
> + }
>  
>   ret = kvmgt_guest_init(mdev);
>   if (ret)
> -- 

Thanks for the fix!

Reviewed-by: Zhenyu Wang 

-- 

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [PATCH] drm/i915/gvt: replace idr_init() by idr_init_base()

[Zhenyu Wang]

On 2020.11.04 17:45:32 +0530, Deepak R Varma wrote:
> idr_init() uses base 0 which is an invalid identifier. The new function
> idr_init_base allows IDR to set the ID lookup from base 1. This avoids
> all lookups that otherwise starts from 0 since 0 is always unused.
> 
> References: commit 6ce711f27500 ("idr: Make 1-based IDRs more efficient")
> 
> Signed-off-by: Deepak R Varma 
> ---
>  drivers/gpu/drm/i915/gvt/gvt.c  | 2 +-
>  drivers/gpu/drm/i915/gvt/vgpu.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/gvt.c b/drivers/gpu/drm/i915/gvt/gvt.c
> index c7c561237883..45b492edbb19 100644
> --- a/drivers/gpu/drm/i915/gvt/gvt.c
> +++ b/drivers/gpu/drm/i915/gvt/gvt.c
> @@ -312,7 +312,7 @@ int intel_gvt_init_device(struct drm_i915_private *i915)
>  
>   gvt_dbg_core("init gvt device\n");
>  
> - idr_init(>vgpu_idr);
> + idr_init_base(>vgpu_idr, 1);
>   spin_lock_init(>scheduler.mmio_context_lock);
>   mutex_init(>lock);
>   mutex_init(>sched_lock);
> diff --git a/drivers/gpu/drm/i915/gvt/vgpu.c b/drivers/gpu/drm/i915/gvt/vgpu.c
> index f6d7e33c7099..1c8e63f84134 100644
> --- a/drivers/gpu/drm/i915/gvt/vgpu.c
> +++ b/drivers/gpu/drm/i915/gvt/vgpu.c
> @@ -393,7 +393,7 @@ static struct intel_vgpu *__intel_gvt_create_vgpu(struct 
> intel_gvt *gvt,
>   mutex_init(>dmabuf_lock);
>   INIT_LIST_HEAD(>dmabuf_obj_list_head);
>   INIT_RADIX_TREE(>page_track_tree, GFP_KERNEL);
> - idr_init(>object_idr);
> + idr_init_base(>object_idr, 1);
>   intel_vgpu_init_cfg_space(vgpu, param->primary);
>   vgpu->d3_entered = false;
>  
> -- 

Looks good to me. Thanks!

Reviewed-by: Zhenyu Wang 

-- 

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [PATCH] drm/i915/gvt: print actionable error message when gm runs out

[Zhenyu Wang]

On 2020.06.03 14:33:21 +0200, Julian Stecklina wrote:
> When a user tries to allocate too many or too big vGPUs and runs out
> of graphics memory, the resulting error message is not actionable and
> looks like an internal error.
> 
> Change the error message to clearly point out what actions a user can
> take to resolve this situation.
> 
> Cc: Thomas Prescher 
> Cc: Zhenyu Wang 
> Signed-off-by: Julian Stecklina 
> ---
>  drivers/gpu/drm/i915/gvt/aperture_gm.c | 9 ++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/aperture_gm.c 
> b/drivers/gpu/drm/i915/gvt/aperture_gm.c
> index 0d6d598713082..5c5c8e871dae2 100644
> --- a/drivers/gpu/drm/i915/gvt/aperture_gm.c
> +++ b/drivers/gpu/drm/i915/gvt/aperture_gm.c
> @@ -69,9 +69,12 @@ static int alloc_gm(struct intel_vgpu *vgpu, bool high_gm)
> start, end, flags);
>   mmio_hw_access_post(gt);
>   mutex_unlock(>ggtt->vm.mutex);
> - if (ret)
> - gvt_err("fail to alloc %s gm space from host\n",
> - high_gm ? "high" : "low");
> + if (ret) {
> + gvt_err("vgpu%d: failed to allocate %s gm space from host\n",
> + vgpu->id, high_gm ? "high" : "low");
> + gvt_err("vgpu%d: destroying vGPUs, decreasing vGPU memory size 
> or increasing GPU aperture size may resolve this\n",
> + vgpu->id);

Currently we can't decrease vGPU mem size as defined by mdev type,
so actually you may try different vGPU type. And aperture size is
also handled for supported vGPU mdev types, so assume user should
already be awared of that too. I just don't want us to be too chatty. :)

> + }
>  
>   return ret;
>  }
> -- 
> 2.26.2
> 
> ___
> intel-gvt-dev mailing list
> intel-gvt-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [PATCH] drm/i915/gvt: Use ARRAY_SIZE for vgpu_types

[Zhenyu Wang]

On 2020.05.18 22:00:52 +0100, Chris Wilson wrote:
> Quoting Aishwarya Ramakrishnan (2020-05-18 16:03:36)
> > Prefer ARRAY_SIZE instead of using sizeof
> > 
> > Fixes coccicheck warning: Use ARRAY_SIZE
> > 
> > Signed-off-by: Aishwarya Ramakrishnan 
> Reviewed-by: Chris Wilson 

Applied, thanks!

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [PATCH] drm/i915: Mark check_shadow_context_ppgtt as maybe unused

[Zhenyu Wang]

On 2020.05.15 19:35:45 -0700, Nathan Chancellor wrote:
> When CONFIG_DRM_I915_DEBUG_GEM is not set, clang warns:
> 
> drivers/gpu/drm/i915/gvt/scheduler.c:884:1: warning: function
> 'check_shadow_context_ppgtt' is not needed and will not be emitted
> [-Wunneeded-internal-declaration]
> check_shadow_context_ppgtt(struct execlist_ring_context *c, struct
> intel_vgpu_mm *m)
> ^
> 1 warning generated.
> 
> This warning is similar to -Wunused-function but rather than warning
> that the function is completely unused, it warns that it is used in some
> expression within the file but that expression will be evaluated to a
> constant or be optimized away in the final assembly, essentially making
> it appeared used but really isn't. Usually, this happens when a function
> or variable is only used in sizeof, where it will appear to be used but
> will be evaluated at compile time and not be required to be emitted.
> 
> In this case, the function is only used in GEM_BUG_ON, which is defined
> as BUILD_BUG_ON_INVALID, which intentionally follows this pattern. To
> fix this warning, add __maybe_unused to make it clear that this is
> intentional depending on the configuration.
> 
> Fixes: bec3df930fbd ("drm/i915/gvt: Support PPGTT table load command")
> Link: https://github.com/ClangBuiltLinux/linux/issues/1027
> Signed-off-by: Nathan Chancellor 
> ---
>  drivers/gpu/drm/i915/gvt/scheduler.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c 
> b/drivers/gpu/drm/i915/gvt/scheduler.c
> index f776c92de8d7..0fb1df71c637 100644
> --- a/drivers/gpu/drm/i915/gvt/scheduler.c
> +++ b/drivers/gpu/drm/i915/gvt/scheduler.c
> @@ -880,7 +880,7 @@ static void update_guest_pdps(struct intel_vgpu *vgpu,
>   gpa + i * 8, [7 - i], 4);
>  }
>  
> -static bool
> +static __maybe_unused bool
>  check_shadow_context_ppgtt(struct execlist_ring_context *c, struct 
> intel_vgpu_mm *m)
>  {
>   if (m->ppgtt_mm.root_entry_type == GTT_TYPE_PPGTT_ROOT_L4_ENTRY) {
> 
> base-commit: bdecf38f228bcca73b31ada98b5b7ba1215eb9c9

Thanks for the fix!

Acked-by: Zhenyu Wang 

I'll pick up for gvt-next-fixes pull.

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [PATCH v5 4/6] drm/i915/gvt: Deliver vGPU refresh event to userspace

[Zhenyu Wang]

On 2019.08.16 10:35:26 +0800, Tina Zhang wrote:
> Deliver the display refresh events to the user land. Userspace can use
> the irq mask/unmask mechanism to disable or enable the event delivery.
> 
> As we know, delivering refresh event at each vblank safely avoids
> tearing and unexpected event overwhelming, but there are still spaces
> to optimize.
> 
> For handling the normal case, deliver the page flip refresh
> event at each vblank, in other words, bounded by vblanks. Skipping some
> events bring performance enhancement while not hurting user experience.
> 
> For single framebuffer case, deliver the refresh events to userspace at
> all vblanks. This heuristic at each vblank leverages pageflip_count
> incresements to determine if there is no page flip happens after a certain
> period and so that the case is regarded as single framebuffer one.
> Although this heuristic makes incorrect decision sometimes and it depends
> on guest behavior, for example, when no cursor movements happen, the
> user experience does not harm and front buffer is still correctly acquired.
> Meanwhile, in actual single framebuffer case, the user experience is
> enhanced compared with page flip events only.
> 
> Addtionally, to mitigate the events delivering footprints, one eventfd and
> 8 byte eventfd counter partition are leveraged.
> 
> v2:
> - Support vfio_irq_info_cap_display_plane_events. (Tina)
> 
> Signed-off-by: Tina Zhang 
> Signed-off-by: Kechen Lu 
> ---
>  drivers/gpu/drm/i915/gvt/display.c |  22 
>  drivers/gpu/drm/i915/gvt/gvt.h |   2 +
>  drivers/gpu/drm/i915/gvt/kvmgt.c   | 159 +++--
>  3 files changed, 174 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/display.c 
> b/drivers/gpu/drm/i915/gvt/display.c
> index 1a0a4ae4826e..616285e4a014 100644
> --- a/drivers/gpu/drm/i915/gvt/display.c
> +++ b/drivers/gpu/drm/i915/gvt/display.c
> @@ -34,6 +34,8 @@
>  
>  #include "i915_drv.h"
>  #include "gvt.h"
> +#include 
> +#include 
>  
>  static int get_edp_pipe(struct intel_vgpu *vgpu)
>  {
> @@ -387,6 +389,8 @@ void intel_gvt_check_vblank_emulation(struct intel_gvt 
> *gvt)
>   mutex_unlock(>lock);
>  }
>  
> +#define PAGEFLIP_DELAY_THR 10
> +
>  static void emulate_vblank_on_pipe(struct intel_vgpu *vgpu, int pipe)
>  {
>   struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv;
> @@ -396,7 +400,10 @@ static void emulate_vblank_on_pipe(struct intel_vgpu 
> *vgpu, int pipe)
>   [PIPE_B] = PIPE_B_VBLANK,
>   [PIPE_C] = PIPE_C_VBLANK,
>   };
> + int pri_flip_event = SKL_FLIP_EVENT(pipe, PLANE_PRIMARY);
>   int event;
> + u64 eventfd_signal_val = 0;
> + static int no_pageflip_count;
>  
>   if (pipe < PIPE_A || pipe > PIPE_C)
>   return;
> @@ -407,11 +414,26 @@ static void emulate_vblank_on_pipe(struct intel_vgpu 
> *vgpu, int pipe)
>   if (!pipe_is_enabled(vgpu, pipe))
>   continue;
>  
> + if (event == pri_flip_event)
> + eventfd_signal_val |= DISPLAY_PRI_REFRESH_EVENT_VAL;
> +
>   intel_vgpu_trigger_virtual_event(vgpu, event);
>   }
>  
> + if (eventfd_signal_val)
> + no_pageflip_count = 0;
> + else if (!eventfd_signal_val && no_pageflip_count > PAGEFLIP_DELAY_THR)

extra !eventfd_signal_val

> + eventfd_signal_val |= DISPLAY_PRI_REFRESH_EVENT_VAL;
> + else
> + no_pageflip_count++;

no_pageflip_count should be per-vgpu instead of static.

> +
> + if (vgpu->vdev.vblank_trigger && !vgpu->vdev.display_event_mask &&
> + eventfd_signal_val)
> + eventfd_signal(vgpu->vdev.vblank_trigger, eventfd_signal_val);
> +
>   if (pipe_is_enabled(vgpu, pipe)) {
>   vgpu_vreg_t(vgpu, PIPE_FRMCOUNT_G4X(pipe))++;
> +

extra line

>   intel_vgpu_trigger_virtual_event(vgpu, vblank_event[pipe]);
>   }
>  }
> diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
> index cd29ea28d7ed..6c8ed030c30b 100644
> --- a/drivers/gpu/drm/i915/gvt/gvt.h
> +++ b/drivers/gpu/drm/i915/gvt/gvt.h
> @@ -205,6 +205,8 @@ struct intel_vgpu {
>   int num_irqs;
>   struct eventfd_ctx *intx_trigger;
>   struct eventfd_ctx *msi_trigger;
> + struct eventfd_ctx *vblank_trigger;
> + u32 display_event_mask;
>  
>   /*
>* Two caches are used to avoid mapping duplicated pages (eg.
> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c 
> b/drivers/gpu/drm/i915/gvt/kvmgt.c
> index fd1633342e53..9ace1f4ff9eb 100644
> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> @@ -1250,6 +1250,8 @@ static int intel_vgpu_get_irq_count(struct intel_vgpu 
> *vgpu, int type)
>  {
>   if (type == VFIO_PCI_INTX_IRQ_INDEX || type == VFIO_PCI_MSI_IRQ_INDEX)
>   return 1;
> + else if (type < VFIO_PCI_NUM_IRQS + vgpu->vdev.num_irqs)
> +   

Re: [RFC PATCH v4 1/6] vfio: Define device specific irq type capability

[Zhenyu Wang]

On 2019.07.19 09:02:33 +, Lu, Kechen wrote:
> Hi,
> 
> > -Original Message-
> > From: Zhenyu Wang [mailto:zhen...@linux.intel.com]
> > Sent: Friday, July 19, 2019 2:06 PM
> > To: Lu, Kechen 
> > Cc: intel-gvt-...@lists.freedesktop.org; k...@vger.kernel.org; linux- 
> > ker...@vger.kernel.org; Zhang, Tina ; 
> > kra...@redhat.com; zhen...@linux.intel.com; Lv, Zhiyuan 
> > ; Wang, Zhi A ; Tian, 
> > Kevin ; Yuan, Hang ; 
> > alex.william...@redhat.com; Eric Auger 
> > Subject: Re: [RFC PATCH v4 1/6] vfio: Define device specific irq type 
> > capability
> > 
> > On 2019.07.18 23:56:35 +0800, Kechen Lu wrote:
> > > From: Tina Zhang 
> > >
> > > Cap the number of irqs with fixed indexes and use capability chains 
> > > to chain device specific irqs.
> > >
> > > Signed-off-by: Tina Zhang 
> > > Signed-off-by: Eric Auger 
> > > ---
> > >  include/uapi/linux/vfio.h | 19 ++-
> > >  1 file changed, 18 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h 
> > > index 8f10748dac79..be6adab4f759 100644
> > > --- a/include/uapi/linux/vfio.h
> > > +++ b/include/uapi/linux/vfio.h
> > > @@ -448,11 +448,27 @@ struct vfio_irq_info {
> > >  #define VFIO_IRQ_INFO_MASKABLE   (1 << 1)
> > >  #define VFIO_IRQ_INFO_AUTOMASKED (1 << 2)
> > >  #define VFIO_IRQ_INFO_NORESIZE   (1 << 3)
> > > +#define VFIO_IRQ_INFO_FLAG_CAPS  (1 << 4) /* Info supports caps
> > */
> > >   __u32   index;  /* IRQ index */
> > >   __u32   count;  /* Number of IRQs within this index */
> > > + __u32   cap_offset; /* Offset within info struct of first cap */
> > 
> > This still breaks ABI as argsz would be updated with this new field, 
> > so it would cause compat issue. I think my last suggestion was to 
> > assume cap list starts after vfio_irq_info.
> >
>  
> In the common practice, the general logic is first use the "count" as the 
> "minsz" boundary to perform copy from user, and then perform following logic, 
> so that the incompatibility issue would not happen. BTW, this patch has been 
> double checked by Eric Auger before included in his patch-set. 
> 

yeah, sorry I was thinking vfio might fail in that case but it seems
current code assume argsz should be larger than minsz for count here,
so that's fine.

> 
> > >  };
> > >  #define VFIO_DEVICE_GET_IRQ_INFO _IO(VFIO_TYPE, VFIO_BASE + 9)
> > >
> > > +/*
> > > + * The irq type capability allows irqs unique to a specific device 
> > > +or
> > > + * class of devices to be exposed.
> > > + *
> > > + * The structures below define version 1 of this capability.
> > > + */
> > > +#define VFIO_IRQ_INFO_CAP_TYPE  3
> > > +
> > > +struct vfio_irq_info_cap_type {
> > > + struct vfio_info_cap_header header;
> > > + __u32 type; /* global per bus driver */
> > > + __u32 subtype;  /* type specific */ };
> > > +
> > >  /**
> > >   * VFIO_DEVICE_SET_IRQS - _IOW(VFIO_TYPE, VFIO_BASE + 10, struct
> > vfio_irq_set)
> > >   *
> > > @@ -554,7 +570,8 @@ enum {
> > >   VFIO_PCI_MSIX_IRQ_INDEX,
> > >   VFIO_PCI_ERR_IRQ_INDEX,
> > >   VFIO_PCI_REQ_IRQ_INDEX,
> > > - VFIO_PCI_NUM_IRQS
> > > + VFIO_PCI_NUM_IRQS = 5   /* Fixed user ABI, IRQ indexes >=5 use
> > */
> > > + /* device specific cap to define content */
> > >  };
> > >
> > >  /*
> > > --
> > > 2.17.1
> > >
> > 
> > --
> > Open Source Technology Center, Intel ltd.
> > 
> > $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [RFC PATCH v4 6/6] drm/i915/gvt: Add cursor plane reg update trap emulation handler

[Zhenyu Wang]

On 2019.07.18 23:56:40 +0800, Kechen Lu wrote:
> This patch adds the cursor plane CURBASE reg update trap handler
> in order to :
> 
> - Deliver the cursor refresh event at each vblank emulation,
> the flip_done_event bit check is supposed to do here. If cursor
> plane updates happen, deliver the cursor refresh events.
> 
> - Support the sync and async cursor plane updates and
> corresponding cursor plane flip interrupts reporting.
> 
> Signed-off-by: Kechen Lu 
> ---
>  drivers/gpu/drm/i915/gvt/display.c   | 11 +++
>  drivers/gpu/drm/i915/gvt/handlers.c  | 27 ---
>  drivers/gpu/drm/i915/gvt/interrupt.c |  7 +++
>  drivers/gpu/drm/i915/gvt/interrupt.h |  3 +++
>  4 files changed, 45 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/display.c 
> b/drivers/gpu/drm/i915/gvt/display.c
> index df52e4b4c1b0..a0accc51d44f 100644
> --- a/drivers/gpu/drm/i915/gvt/display.c
> +++ b/drivers/gpu/drm/i915/gvt/display.c
> @@ -399,6 +399,7 @@ static void emulate_vblank_on_pipe(struct intel_vgpu 
> *vgpu, int pipe)
>   [PIPE_C] = PIPE_C_VBLANK,
>   };
>   int pri_flip_event = SKL_FLIP_EVENT(pipe, PLANE_PRIMARY);
> + int cur_flip_event = CURSOR_A_FLIP_DONE + pipe;
>   int event;
>   u64 eventfd_signal_val = 0;
>   static int pageflip_count;
> @@ -417,6 +418,11 @@ static void emulate_vblank_on_pipe(struct intel_vgpu 
> *vgpu, int pipe)
>   pageflip_count += PAGEFLIP_INC_COUNT;
>   }
>  
> + if (event == cur_flip_event) {
> + eventfd_signal_val += DISPLAY_CUR_REFRESH_EVENT_INC;
> + pageflip_count += PAGEFLIP_INC_COUNT;
> + }
> +
>   intel_vgpu_trigger_virtual_event(vgpu, event);
>   }
>  
> @@ -430,6 +436,11 @@ static void emulate_vblank_on_pipe(struct intel_vgpu 
> *vgpu, int pipe)
>   eventfd_signal_val += DISPLAY_PRI_REFRESH_EVENT_INC;
>   pageflip_count += PAGEFLIP_INC_COUNT;
>   }
> +
> + if (event == PLANE_CURSOR) {
> + eventfd_signal_val += DISPLAY_CUR_REFRESH_EVENT_INC;
> + pageflip_count += PAGEFLIP_INC_COUNT;
> + }
>   }
>  
>   if (--pageflip_count < 0) {
> diff --git a/drivers/gpu/drm/i915/gvt/handlers.c 
> b/drivers/gpu/drm/i915/gvt/handlers.c
> index 6ad29c4f08e5..821ff88977d8 100644
> --- a/drivers/gpu/drm/i915/gvt/handlers.c
> +++ b/drivers/gpu/drm/i915/gvt/handlers.c
> @@ -767,6 +767,27 @@ static int pri_surf_mmio_write(struct intel_vgpu *vgpu, 
> unsigned int offset,
>   return 0;
>  }
>  
> +#define CURBASE_TO_PIPE(reg) \
> + calc_index(offset, _CURABASE, _CURBBASE, 0, CURBASE(PIPE_C))
> +
> +static int cur_surf_mmio_write(struct intel_vgpu *vgpu, unsigned int offset,
> + void *p_data, unsigned int bytes)
> +{
> + struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv;
> + u32 pipe = CURBASE_TO_PIPE(offset);
> + int event = CURSOR_A_FLIP_DONE + pipe;
> +
> + write_vreg(vgpu, offset, p_data, bytes);
> +
> + if (vgpu_vreg_t(vgpu, CURCNTR(pipe)) & PLANE_CTL_ASYNC_FLIP) {
> + intel_vgpu_trigger_virtual_event(vgpu, event);
> + set_bit(PLANE_CURSOR, vgpu->display.async_flip_event[pipe]);
> + } else
> + set_bit(event, vgpu->irq.flip_done_event[pipe]);
> +
> + return 0;
> +}
> +
>  #define SPRSURF_TO_PIPE(offset) \
>   calc_index(offset, _SPRA_SURF, _SPRB_SURF, 0, SPRSURF(PIPE_C))
>  
> @@ -1955,9 +1976,9 @@ static int init_generic_mmio_info(struct intel_gvt *gvt)
>   MMIO_D(CURPOS(PIPE_B), D_ALL);
>   MMIO_D(CURPOS(PIPE_C), D_ALL);
>  
> - MMIO_D(CURBASE(PIPE_A), D_ALL);
> - MMIO_D(CURBASE(PIPE_B), D_ALL);
> - MMIO_D(CURBASE(PIPE_C), D_ALL);
> + MMIO_DH(CURBASE(PIPE_A), D_ALL, NULL, cur_surf_mmio_write);
> + MMIO_DH(CURBASE(PIPE_B), D_ALL, NULL, cur_surf_mmio_write);
> + MMIO_DH(CURBASE(PIPE_C), D_ALL, NULL, cur_surf_mmio_write);

I think we should also track cursor pos change right?

>  
>   MMIO_D(CUR_FBC_CTL(PIPE_A), D_ALL);
>   MMIO_D(CUR_FBC_CTL(PIPE_B), D_ALL);
> diff --git a/drivers/gpu/drm/i915/gvt/interrupt.c 
> b/drivers/gpu/drm/i915/gvt/interrupt.c
> index 951681813230..9c2b9d2e1529 100644
> --- a/drivers/gpu/drm/i915/gvt/interrupt.c
> +++ b/drivers/gpu/drm/i915/gvt/interrupt.c
> @@ -113,6 +113,9 @@ static const char * const irq_name[INTEL_GVT_EVENT_MAX] = 
> {
>   [SPRITE_A_FLIP_DONE] = "Sprite Plane A flip done",
>   [SPRITE_B_FLIP_DONE] = "Sprite Plane B flip done",
>   [SPRITE_C_FLIP_DONE] = "Sprite Plane C flip done",
> + [CURSOR_A_FLIP_DONE] = "Cursor Plane A flip done",
> + [CURSOR_B_FLIP_DONE] = "Cursor Plane B flip done",
> + [CURSOR_C_FLIP_DONE] = "Cursor Plane C flip done",
>  
>   [PCU_THERMAL] = "PCU Thermal Event",
>   [PCU_PCODE2DRIVER_MAILBOX] = "PCU pcode2driver mailbox event",
> @@ -593,6 +596,10 @@ 

Re: [RFC PATCH v4 4/6] drm/i915/gvt: Deliver vGPU refresh event to userspace

[Zhenyu Wang]

On 2019.07.18 23:56:38 +0800, Kechen Lu wrote:
> From: Tina Zhang 
> 
> Deliver the display refresh events to the user land. Userspace can use
> the irq mask/unmask mechanism to disable or enable the event delivery.
> 
> As we know, delivering refresh event at each vblank safely avoids
> tearing and unexpected event overwhelming, but there are still spaces
> to optimize.
> 
> For handling the normal case, deliver the page flip refresh
> event at each vblank, in other words, bounded by vblanks. Skipping some
> events bring performance enhancement while not hurting user experience.
> 
> For single framebuffer case, deliver the refresh events to userspace at
> all vblanks. This heuristic at each vblank leverages pageflip_count
> incresements to determine if there is no page flip happens after a certain
> period and so that the case is regarded as single framebuffer one.
> Although this heuristic makes incorrect decision sometimes and it depends
> on guest behavior, for example, when no cursor movements happen, the
> user experience does not harm and front buffer is still correctly acquired.
> Meanwhile, in actual single framebuffer case, the user experience is
> enhanced compared with page flip events only.
> 
> Addtionally, to mitigate the events delivering footprints, one eventfd and
> 8 byte eventfd counter partition are leveraged.
> 
> Signed-off-by: Tina Zhang 
> Signed-off-by: Kechen Lu 
> ---
>  drivers/gpu/drm/i915/gvt/display.c |  21 
>  drivers/gpu/drm/i915/gvt/gvt.h |   7 ++
>  drivers/gpu/drm/i915/gvt/kvmgt.c   | 154 +++--
>  3 files changed, 173 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/display.c 
> b/drivers/gpu/drm/i915/gvt/display.c
> index 1a0a4ae4826e..036db8199983 100644
> --- a/drivers/gpu/drm/i915/gvt/display.c
> +++ b/drivers/gpu/drm/i915/gvt/display.c
> @@ -387,6 +387,8 @@ void intel_gvt_check_vblank_emulation(struct intel_gvt 
> *gvt)
>   mutex_unlock(>lock);
>  }
>  
> +#define PAGEFLIP_INC_COUNT 5
> +
>  static void emulate_vblank_on_pipe(struct intel_vgpu *vgpu, int pipe)
>  {
>   struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv;
> @@ -396,7 +398,10 @@ static void emulate_vblank_on_pipe(struct intel_vgpu 
> *vgpu, int pipe)
>   [PIPE_B] = PIPE_B_VBLANK,
>   [PIPE_C] = PIPE_C_VBLANK,
>   };
> + int pri_flip_event = SKL_FLIP_EVENT(pipe, PLANE_PRIMARY);
>   int event;
> + u64 eventfd_signal_val = 0;
> + static int pageflip_count;
>  
>   if (pipe < PIPE_A || pipe > PIPE_C)
>   return;
> @@ -407,11 +412,27 @@ static void emulate_vblank_on_pipe(struct intel_vgpu 
> *vgpu, int pipe)
>   if (!pipe_is_enabled(vgpu, pipe))
>   continue;
>  
> + if (event == pri_flip_event) {
> + eventfd_signal_val += DISPLAY_PRI_REFRESH_EVENT_INC;
> + pageflip_count += PAGEFLIP_INC_COUNT;
> + }
> +
>   intel_vgpu_trigger_virtual_event(vgpu, event);
>   }
>  
> + if (--pageflip_count < 0) {
> + eventfd_signal_val += DISPLAY_PRI_REFRESH_EVENT_INC;
> + pageflip_count = 0;
> + }

If pageflip_count has been increased to a big number from page flip
event for some time, then if guest switch for single buffer render, it
would take 5x vblank time to send refresh then..

> +
> + if (vgpu->vdev.vblank_trigger && !(vgpu->vdev.display_event_mask
> + & (DISPLAY_PRI_REFRESH_EVENT | DISPLAY_CUR_REFRESH_EVENT)) &&
> + eventfd_signal_val)
> + eventfd_signal(vgpu->vdev.vblank_trigger, eventfd_signal_val);
> +
>   if (pipe_is_enabled(vgpu, pipe)) {
>   vgpu_vreg_t(vgpu, PIPE_FRMCOUNT_G4X(pipe))++;
> +
>   intel_vgpu_trigger_virtual_event(vgpu, vblank_event[pipe]);
>   }
>  }
> diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
> index 64d1c1aaa42a..b654b6fa0663 100644
> --- a/drivers/gpu/drm/i915/gvt/gvt.h
> +++ b/drivers/gpu/drm/i915/gvt/gvt.h
> @@ -165,6 +165,11 @@ struct intel_vgpu_submission {
>   bool active;
>  };
>  
> +#define DISPLAY_PRI_REFRESH_EVENT(1 << 0)
> +#define DISPLAY_PRI_REFRESH_EVENT_INC(1UL << 56)
> +#define DISPLAY_CUR_REFRESH_EVENT(1 << 1)
> +#define DISPLAY_CUR_REFRESH_EVENT_INC(1UL << 48)
> +

As this is for eventfd interface definition, need to put in vfio header instead 
of gvt's,
as this is userspace API. And better reorder for different usage on irq masking 
and eventfd value.

For eventfd value, this looks like counter for each plane? Or do we just need a 
flag?

>  struct intel_vgpu {
>   struct intel_gvt *gvt;
>   struct mutex vgpu_lock;
> @@ -205,6 +210,8 @@ struct intel_vgpu {
>   int num_irqs;
>   struct eventfd_ctx *intx_trigger;
>   struct eventfd_ctx *msi_trigger;
> + struct eventfd_ctx *vblank_trigger;
> + u32 

Re: [RFC PATCH v4 1/6] vfio: Define device specific irq type capability

[Zhenyu Wang]

On 2019.07.18 23:56:35 +0800, Kechen Lu wrote:
> From: Tina Zhang 
> 
> Cap the number of irqs with fixed indexes and use capability chains
> to chain device specific irqs.
> 
> Signed-off-by: Tina Zhang 
> Signed-off-by: Eric Auger 
> ---
>  include/uapi/linux/vfio.h | 19 ++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 8f10748dac79..be6adab4f759 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -448,11 +448,27 @@ struct vfio_irq_info {
>  #define VFIO_IRQ_INFO_MASKABLE   (1 << 1)
>  #define VFIO_IRQ_INFO_AUTOMASKED (1 << 2)
>  #define VFIO_IRQ_INFO_NORESIZE   (1 << 3)
> +#define VFIO_IRQ_INFO_FLAG_CAPS  (1 << 4) /* Info supports caps 
> */
>   __u32   index;  /* IRQ index */
>   __u32   count;  /* Number of IRQs within this index */
> + __u32   cap_offset; /* Offset within info struct of first cap */

This still breaks ABI as argsz would be updated with this new field, so it would
cause compat issue. I think my last suggestion was to assume cap list starts 
after
vfio_irq_info.

>  };
>  #define VFIO_DEVICE_GET_IRQ_INFO _IO(VFIO_TYPE, VFIO_BASE + 9)
>  
> +/*
> + * The irq type capability allows irqs unique to a specific device or
> + * class of devices to be exposed.
> + *
> + * The structures below define version 1 of this capability.
> + */
> +#define VFIO_IRQ_INFO_CAP_TYPE  3
> +
> +struct vfio_irq_info_cap_type {
> + struct vfio_info_cap_header header;
> + __u32 type; /* global per bus driver */
> + __u32 subtype;  /* type specific */
> +};
> +
>  /**
>   * VFIO_DEVICE_SET_IRQS - _IOW(VFIO_TYPE, VFIO_BASE + 10, struct 
> vfio_irq_set)
>   *
> @@ -554,7 +570,8 @@ enum {
>   VFIO_PCI_MSIX_IRQ_INDEX,
>   VFIO_PCI_ERR_IRQ_INDEX,
>   VFIO_PCI_REQ_IRQ_INDEX,
> - VFIO_PCI_NUM_IRQS
> + VFIO_PCI_NUM_IRQS = 5   /* Fixed user ABI, IRQ indexes >=5 use   */
> + /* device specific cap to define content */
>  };
>  
>  /*
> -- 
> 2.17.1
> 

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [RFC PATCH v3 0/4] Deliver vGPU display vblank event to userspace

[Zhenyu Wang]

On 2019.06.28 12:43:47 +, Zhang, Tina wrote:
> Hi,
> 
> How about GVT-g supports both vblank and page flip events. Then QEMU UI can 
> mask/unmask the events to decide which kind of event is expected.
> For DRM UI, it can decide to mask vblank event and use page flip events. We 
> tried DRM UI with page flip events, the performance is great, even in the 
> case of front buffer rendering. For the UI like GTK, vblank event is better. 
> 
> Besides, with the irq mask/unmask mechanism, userspace can dynamically choose 
> between the UI timer and the events. 
> 

The idea is to deliver an event to tell userspace that "guest has
display update, you need to refresh your UI". How driver
implementation uses either vblank or page flip to determine that is
driver specific, as same routine of vfio gfx interface will be used to
refresh on guest display.

If userspace doesn't set irq for vfio gfx display, it would just use
own timer.

> 
> > -Original Message-
> > From: Zhang, Tina
> > Sent: Friday, June 28, 2019 4:45 PM
> > To: 'Gerd Hoffmann' ; Zhenyu Wang
> > 
> > Cc: Tian, Kevin ; k...@vger.kernel.org; linux-
> > ker...@vger.kernel.org; alex.william...@redhat.com; Lv, Zhiyuan
> > ; Yuan, Hang ; intel-gvt-
> > d...@lists.freedesktop.org; Wang, Zhi A 
> > Subject: RE: [RFC PATCH v3 0/4] Deliver vGPU display vblank event to
> > userspace
> > 
> > 
> > 
> > > -Original Message-
> > > From: intel-gvt-dev
> > > [mailto:intel-gvt-dev-boun...@lists.freedesktop.org] On Behalf Of Gerd
> > > Hoffmann
> > > Sent: Friday, June 28, 2019 1:44 PM
> > > To: Zhenyu Wang 
> > > Cc: Tian, Kevin ; k...@vger.kernel.org; linux-
> > > ker...@vger.kernel.org; Zhang, Tina ;
> > > alex.william...@redhat.com; Lv, Zhiyuan ; Yuan,
> > > Hang ; intel-gvt-...@lists.freedesktop.org; Wang,
> > > Zhi A 
> > > Subject: Re: [RFC PATCH v3 0/4] Deliver vGPU display vblank event to
> > > userspace
> > >
> > > On Fri, Jun 28, 2019 at 11:21:49AM +0800, Zhenyu Wang wrote:
> > > > On 2019.06.27 12:31:33 +0200, Gerd Hoffmann wrote:
> > > > > > >   Hi,
> > > > > > >
> > > > > > > > Instead of delivering page flip events, we choose to post
> > > > > > > > display vblank event. Handling page flip events for both
> > > > > > > > primary plane and cursor plane may make user space quite
> > > > > > > > busy, although we have the mask/unmask mechansim for
> > mitigation.
> > > > > > > > Besides, there are some cases that guest app only uses one
> > > framebuffer for both drawing and display.
> > > > > > > > In such case, guest OS won't do the plane page flip when the
> > > > > > > > framebuffer is updated, thus the user land won't be notified
> > > > > > > > about the
> > > > > > > updated framebuffer.
> > > > > > >
> > > > > > > What happens when the guest is idle and doesn't draw anything
> > > > > > > to the framebuffer?
> > > > > > The vblank event will be delivered to userspace as well, unless
> > > > > > guest OS
> > > disable the pipe.
> > > > > > Does it make sense to vfio/display?
> > > > >
> > > > > Getting notified only in case there are actual display updates
> > > > > would be a nice optimization, assuming the hardware is able to do 
> > > > > that.
> > > > > If the guest pageflips this is obviously trivial.  Not sure this
> > > > > is possible in case the guest renders directly to the frontbuffer.
> > > > >
> > > > > What exactly happens when the guest OS disables the pipe?  Is a
> > > > > vblank event delivered at least once?  That would be very useful
> > > > > because it will be possible for userspace to stop polling
> > > > > altogether without missing the "guest disabled pipe" event.
> > > > >
> > > >
> > > > It looks like purpose to use vblank here is to replace user space
> > > > polling totally by kernel event? Which just act as display update
> > > > event to replace user space timer to make it query and update planes?
> > >
> > > I think it makes sense to design it that way, so userspace will either
> > > use the events (when supported by the driver) or a timer (fallback if
> > > not) but

Re: [RFC PATCH v3 0/4] Deliver vGPU display vblank event to userspace

[Zhenyu Wang]

On 2019.06.28 07:43:46 +0200, Gerd Hoffmann wrote:
> On Fri, Jun 28, 2019 at 11:21:49AM +0800, Zhenyu Wang wrote:
> > On 2019.06.27 12:31:33 +0200, Gerd Hoffmann wrote:
> > > > >   Hi,
> > > > > 
> > > > > > Instead of delivering page flip events, we choose to post display
> > > > > > vblank event. Handling page flip events for both primary plane and
> > > > > > cursor plane may make user space quite busy, although we have the
> > > > > > mask/unmask mechansim for mitigation. Besides, there are some cases
> > > > > > that guest app only uses one framebuffer for both drawing and 
> > > > > > display.
> > > > > > In such case, guest OS won't do the plane page flip when the
> > > > > > framebuffer is updated, thus the user land won't be notified about 
> > > > > > the
> > > > > updated framebuffer.
> > > > > 
> > > > > What happens when the guest is idle and doesn't draw anything to the
> > > > > framebuffer?
> > > > The vblank event will be delivered to userspace as well, unless guest 
> > > > OS disable the pipe.
> > > > Does it make sense to vfio/display?
> > > 
> > > Getting notified only in case there are actual display updates would be
> > > a nice optimization, assuming the hardware is able to do that.  If the
> > > guest pageflips this is obviously trivial.  Not sure this is possible in
> > > case the guest renders directly to the frontbuffer.
> > > 
> > > What exactly happens when the guest OS disables the pipe?  Is a vblank
> > > event delivered at least once?  That would be very useful because it
> > > will be possible for userspace to stop polling altogether without
> > > missing the "guest disabled pipe" event.
> > > 
> > 
> > It looks like purpose to use vblank here is to replace user space
> > polling totally by kernel event? Which just act as display update
> > event to replace user space timer to make it query and update
> > planes?
> 
> I think it makes sense to design it that way, so userspace will either
> use the events (when supported by the driver) or a timer (fallback if
> not) but not both.

Agree. It's more of a userspace choice.

> 
> > Although in theory vblank is not appropriate for this which
> > doesn't align with plane update or possible front buffer rendering at
> > all, but looks it's just a compromise e.g not sending event for every
> > cursor position change, etc.
> > 
> > I think we need to define semantics for this event properly, e.g user
> > space purely depends on this event for display update, the opportunity
> > for issuing this event is controlled by driver when it's necessary for
> > update, etc. Definitely not named as vblank event or only issue at vblank,
> > that need to happen for other plane change too.
> 
> I think it should be "display update notification", i.e. userspace
> should check for plane changes and update the display.
> 
> Most events will probably come from vblank (typically plane update are
> actually committed at vblank time to avoid tearing, right?).  That is an
> implementation detail though.
> 

Yeah, vblank should be a good time, although driver might also do
optimization e.g checking actual plane change between vblank to see if
there's any real change, etc. Also that will depend on driver
implementation.

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [RFC PATCH v3 0/4] Deliver vGPU display vblank event to userspace

[Zhenyu Wang]

On 2019.06.27 12:31:33 +0200, Gerd Hoffmann wrote:
> > >   Hi,
> > > 
> > > > Instead of delivering page flip events, we choose to post display
> > > > vblank event. Handling page flip events for both primary plane and
> > > > cursor plane may make user space quite busy, although we have the
> > > > mask/unmask mechansim for mitigation. Besides, there are some cases
> > > > that guest app only uses one framebuffer for both drawing and display.
> > > > In such case, guest OS won't do the plane page flip when the
> > > > framebuffer is updated, thus the user land won't be notified about the
> > > updated framebuffer.
> > > 
> > > What happens when the guest is idle and doesn't draw anything to the
> > > framebuffer?
> > The vblank event will be delivered to userspace as well, unless guest OS 
> > disable the pipe.
> > Does it make sense to vfio/display?
> 
> Getting notified only in case there are actual display updates would be
> a nice optimization, assuming the hardware is able to do that.  If the
> guest pageflips this is obviously trivial.  Not sure this is possible in
> case the guest renders directly to the frontbuffer.
> 
> What exactly happens when the guest OS disables the pipe?  Is a vblank
> event delivered at least once?  That would be very useful because it
> will be possible for userspace to stop polling altogether without
> missing the "guest disabled pipe" event.
> 

It looks like purpose to use vblank here is to replace user space
polling totally by kernel event? Which just act as display update
event to replace user space timer to make it query and update
planes? Although in theory vblank is not appropriate for this which
doesn't align with plane update or possible front buffer rendering at
all, but looks it's just a compromise e.g not sending event for every
cursor position change, etc.

I think we need to define semantics for this event properly, e.g user
space purely depends on this event for display update, the opportunity
for issuing this event is controlled by driver when it's necessary for
update, etc. Definitely not named as vblank event or only issue at vblank,
that need to happen for other plane change too.

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [RFC PATCH v2 1/3] vfio: Use capability chains to handle device specific irq

[Zhenyu Wang]

On 2019.06.04 17:55:32 +0800, Tina Zhang wrote:
> Caps the number of irqs with fixed indexes and uses capability chains
> to chain device specific irqs.
> 
> VFIO vGPU leverages this mechanism to trigger primary plane and cursor
> plane page flip event to the user space.
> 
> Signed-off-by: Tina Zhang 
> ---
>  include/uapi/linux/vfio.h | 23 ++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 02bb7ad6e986..9b5e25937c7d 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -444,11 +444,31 @@ struct vfio_irq_info {
>  #define VFIO_IRQ_INFO_MASKABLE   (1 << 1)
>  #define VFIO_IRQ_INFO_AUTOMASKED (1 << 2)
>  #define VFIO_IRQ_INFO_NORESIZE   (1 << 3)
> +#define VFIO_IRQ_INFO_FLAG_CAPS  (1 << 4) /* Info supports caps 
> */
>   __u32   index;  /* IRQ index */
> + __u32   cap_offset; /* Offset within info struct of first cap */
>   __u32   count;  /* Number of IRQs within this index */

This would break ABI for get irq info. I think irq cap chain can just follow
vfio_irq_info.

>  };
>  #define VFIO_DEVICE_GET_IRQ_INFO _IO(VFIO_TYPE, VFIO_BASE + 9)
>  
> +/*
> + * The irq type capability allows irqs unique to a specific device or
> + * class of devices to be exposed.
> + *
> + * The structures below define version 1 of this capability.
> + */
> +#define VFIO_IRQ_INFO_CAP_TYPE  3
> +
> +struct vfio_irq_info_cap_type {
> + struct vfio_info_cap_header header;
> + __u32 type; /* global per bus driver */
> + __u32 subtype;  /* type specific */
> +};
> +
> +#define VFIO_IRQ_TYPE_GFX(1)
> +#define VFIO_IRQ_SUBTYPE_GFX_PRI_PLANE_FLIP  (1)
> +#define VFIO_IRQ_SUBTYPE_GFX_CUR_PLANE_FLIP  (2)
> +

Really need to split for different planes? I'd like a 
VFIO_IRQ_SUBTYPE_GFX_DISPLAY_EVENT
so user space can probe change for all.

>  /**
>   * VFIO_DEVICE_SET_IRQS - _IOW(VFIO_TYPE, VFIO_BASE + 10, struct 
> vfio_irq_set)
>   *
> @@ -550,7 +570,8 @@ enum {
>   VFIO_PCI_MSIX_IRQ_INDEX,
>   VFIO_PCI_ERR_IRQ_INDEX,
>   VFIO_PCI_REQ_IRQ_INDEX,
> - VFIO_PCI_NUM_IRQS
> + VFIO_PCI_NUM_IRQS = 5   /* Fixed user ABI, IRQ indexes >=5 use   */
> + /* device specific cap to define content */
>  };
>  
>  /*
> -- 
> 2.17.1
> 

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [PATCH 1/2] vfio: ABI for setting mdev display flip eventfd

[Zhenyu Wang]

On 2019.05.27 14:22:37 +0200, Gerd Hoffmann wrote:
> On Mon, May 27, 2019 at 05:07:41PM +0800, Zhenyu Wang wrote:
> > On 2019.05.27 16:43:11 +0800, Tina Zhang wrote:
> > > Add VFIO_DEVICE_SET_GFX_FLIP_EVENTFD ioctl command to set eventfd
> > > based signaling mechanism to deliver vGPU framebuffer page flip
> > > event to userspace.
> > 
> > Should we add probe to see if driver can support gfx flip event?
> 
> Userspace can simply call VFIO_DEVICE_SET_GFX_FLIP_EVENTFD and see if it
> worked.  If so -> use the eventfd.  Otherwise take the fallback path
> (timer based polling).  I can't see any advantage a separate feature
> probe steps adds.
> 

Then we need to define error return which means driver doesn't support
e.g -ENOTTY, and driver shouldn't return that for other possible
failure, so user space won't get confused.

I think if we can define this as generic display event notification?
Not necessarily just for flip, just a display change notification to
let user space query current state.

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [PATCH 2/2] drm/i915/gvt: Support delivering page flip event to userspace

[Zhenyu Wang]

On 2019.05.27 16:43:12 +0800, Tina Zhang wrote:
> Use the eventfd based signaling mechanism provided by vfio/display
> to deliver vGPU framebuffer page flip event to userspace.
> 
> Signed-off-by: Tina Zhang 
> ---
>  drivers/gpu/drm/i915/gvt/dmabuf.c   | 31 +
>  drivers/gpu/drm/i915/gvt/dmabuf.h   |  1 +
>  drivers/gpu/drm/i915/gvt/gvt.c  |  1 +
>  drivers/gpu/drm/i915/gvt/gvt.h  |  2 ++
>  drivers/gpu/drm/i915/gvt/handlers.c |  2 ++
>  drivers/gpu/drm/i915/gvt/kvmgt.c|  7 +++
>  6 files changed, 44 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/dmabuf.c 
> b/drivers/gpu/drm/i915/gvt/dmabuf.c
> index 4e1e425189ba..f2ed45616d72 100644
> --- a/drivers/gpu/drm/i915/gvt/dmabuf.c
> +++ b/drivers/gpu/drm/i915/gvt/dmabuf.c
> @@ -538,6 +538,35 @@ int intel_vgpu_get_dmabuf(struct intel_vgpu *vgpu, 
> unsigned int dmabuf_id)
>   return ret;
>  }
>  
> +static void release_flip_eventfd_ctx(struct intel_vgpu *vgpu)
> +{
> + struct eventfd_ctx **trigger = >page_flip_trigger;
> +
> + if (*trigger) {
> + eventfd_ctx_put(*trigger);
> + *trigger = NULL;
> + }

Why so twisted?

if (vgpu->page_flip_trigger) {
eventfd_ctx_put(vgpu->page_flip_trigger);
vgpu->page_flip_trigger = NULL;
}

> +}
> +
> +int intel_vgpu_set_flip_eventfd(struct intel_vgpu *vgpu, int fd)
> +{
> + struct eventfd_ctx *trigger;
> +
> + if (fd == -1) {
> + release_flip_eventfd_ctx(vgpu);
> + } else if (fd >= 0) {
> + trigger = eventfd_ctx_fdget(fd);
> + if (IS_ERR(trigger)) {
> + gvt_vgpu_err("eventfd_ctx_fdget failed\n");
> + return PTR_ERR(trigger);
> + }
> + vgpu->page_flip_trigger = trigger;
> + } else
> + return -EINVAL;

Better put (fd < 0) check earlier in ioctl handler to simplify this.

> +
> + return 0;
> +}
> +
>  void intel_vgpu_dmabuf_cleanup(struct intel_vgpu *vgpu)
>  {
>   struct list_head *pos, *n;
> @@ -561,4 +590,6 @@ void intel_vgpu_dmabuf_cleanup(struct intel_vgpu *vgpu)
>  
>   }
>   mutex_unlock(>dmabuf_lock);
> +
> + release_flip_eventfd_ctx(vgpu);
>  }
> diff --git a/drivers/gpu/drm/i915/gvt/dmabuf.h 
> b/drivers/gpu/drm/i915/gvt/dmabuf.h
> index 5f8f03fb1d1b..4d9caa3732d2 100644
> --- a/drivers/gpu/drm/i915/gvt/dmabuf.h
> +++ b/drivers/gpu/drm/i915/gvt/dmabuf.h
> @@ -62,6 +62,7 @@ struct intel_vgpu_dmabuf_obj {
>  
>  int intel_vgpu_query_plane(struct intel_vgpu *vgpu, void *args);
>  int intel_vgpu_get_dmabuf(struct intel_vgpu *vgpu, unsigned int dmabuf_id);
> +int intel_vgpu_set_flip_eventfd(struct intel_vgpu *vgpu, int fd);
>  void intel_vgpu_dmabuf_cleanup(struct intel_vgpu *vgpu);
>  
>  #endif
> diff --git a/drivers/gpu/drm/i915/gvt/gvt.c b/drivers/gpu/drm/i915/gvt/gvt.c
> index 43f4242062dd..7fd4afa432ef 100644
> --- a/drivers/gpu/drm/i915/gvt/gvt.c
> +++ b/drivers/gpu/drm/i915/gvt/gvt.c
> @@ -184,6 +184,7 @@ static const struct intel_gvt_ops intel_gvt_ops = {
>   .get_gvt_attrs = intel_get_gvt_attrs,
>   .vgpu_query_plane = intel_vgpu_query_plane,
>   .vgpu_get_dmabuf = intel_vgpu_get_dmabuf,
> + .vgpu_set_flip_eventfd = intel_vgpu_set_flip_eventfd,
>   .write_protect_handler = intel_vgpu_page_track_handler,
>   .emulate_hotplug = intel_vgpu_emulate_hotplug,
>  };
> diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
> index f5a328b5290a..86ca223f9a60 100644
> --- a/drivers/gpu/drm/i915/gvt/gvt.h
> +++ b/drivers/gpu/drm/i915/gvt/gvt.h
> @@ -229,6 +229,7 @@ struct intel_vgpu {
>   struct completion vblank_done;
>  
>   u32 scan_nonprivbb;
> + struct eventfd_ctx *page_flip_trigger;
>  };
>  
>  /* validating GM healthy status*/
> @@ -570,6 +571,7 @@ struct intel_gvt_ops {
>   struct attribute_group ***intel_vgpu_type_groups);
>   int (*vgpu_query_plane)(struct intel_vgpu *vgpu, void *);
>   int (*vgpu_get_dmabuf)(struct intel_vgpu *vgpu, unsigned int);
> + int (*vgpu_set_flip_eventfd)(struct intel_vgpu *vgpu, int fd);
>   int (*write_protect_handler)(struct intel_vgpu *, u64, void *,
>unsigned int);
>   void (*emulate_hotplug)(struct intel_vgpu *vgpu, bool connected);
> diff --git a/drivers/gpu/drm/i915/gvt/handlers.c 
> b/drivers/gpu/drm/i915/gvt/handlers.c
> index 18f01eeb2510..1b5455888bdf 100644
> --- a/drivers/gpu/drm/i915/gvt/handlers.c
> +++ b/drivers/gpu/drm/i915/gvt/handlers.c
> @@ -763,6 +763,8 @@ static int pri_surf_mmio_write(struct intel_vgpu *vgpu, 
> unsigned int offset,
>   else
>   set_bit(event, vgpu->irq.flip_done_event[pipe]);
>  
> + eventfd_signal(vgpu->page_flip_trigger, 1);

Need to check if page_flip_trigger is armed or not.

> +
>   return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c 
> b/drivers/gpu/drm/i915/gvt/kvmgt.c
> index 

Re: [PATCH 1/2] vfio: ABI for setting mdev display flip eventfd

[Zhenyu Wang]

On 2019.05.27 16:43:11 +0800, Tina Zhang wrote:
> Add VFIO_DEVICE_SET_GFX_FLIP_EVENTFD ioctl command to set eventfd
> based signaling mechanism to deliver vGPU framebuffer page flip
> event to userspace.
>

Should we add probe to see if driver can support gfx flip event?

> Signed-off-by: Tina Zhang 
> ---
>  include/uapi/linux/vfio.h | 12 
>  1 file changed, 12 insertions(+)
> 
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 02bb7ad6e986..27300597717f 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -696,6 +696,18 @@ struct vfio_device_ioeventfd {
>  
>  #define VFIO_DEVICE_IOEVENTFD_IO(VFIO_TYPE, VFIO_BASE + 16)
>  
> +/**
> + * VFIO_DEVICE_SET_GFX_FLIP_EVENTFD - _IOW(VFIO_TYPE, VFIO_BASE + 17, __s32)
> + *
> + * Set eventfd based signaling mechanism to deliver vGPU framebuffer page
> + * flip event to userspace. A value of -1 is used to stop the page flip
> + * delivering.
> + *
> + * Return: 0 on success, -errno on failure.
> + */
> +
> +#define VFIO_DEVICE_SET_GFX_FLIP_EVENTFD _IO(VFIO_TYPE, VFIO_BASE + 17)
> +
>  /*  API for Type1 VFIO IOMMU  */
>  
>  /**
> -- 
> 2.17.1
> 

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [PATCH 7/8] drm/i915/gvt: vGPU device config data save/restore interface

[Zhenyu Wang]

On 2019.02.19 02:46:32 -0500, Yan Zhao wrote:
> The patch implments the gvt interface intel_gvt_save_restore to
> save/restore vGPU's device config data for live migration.
> 
> vGPU device config data includes vreg, vggtt, vcfg space, workloads, ppgtt,
> execlist.
> It does not include dirty pages in system memory produced by vGPU.
> 
> Signed-off-by: Yulei Zhang 
> Signed-off-by: Xiao Zheng 
> Signed-off-by: Zhenyu Wang 
> Signed-off-by: Yan Zhao 

...

> +
> +#ifndef __GVT_MIGRATE_H__
> +#define __GVT_MIGRATE_H__
> +
> +#define MIGRATION_DIRTY_BITMAP_SIZE (16*1024UL)
> +
> +/* Assume 9MB is enough to descript VM kernel state */
> +#define MIGRATION_IMG_MAX_SIZE (9*1024UL*1024UL)
> +#define GVT_MMIO_SIZE (2*1024UL*1024UL)
> +#define GVT_MIGRATION_VERSION0
> +
> +enum gvt_migration_type_t {
> + GVT_MIGRATION_NONE,
> + GVT_MIGRATION_HEAD,
> + GVT_MIGRATION_CFG_SPACE,
> + GVT_MIGRATION_VREG,
> + GVT_MIGRATION_SREG,
> + GVT_MIGRATION_GTT,
> + GVT_MIGRATION_PPGTT,
> + GVT_MIGRATION_WORKLOAD,
> + GVT_MIGRATION_EXECLIST,
> +};
> +
> +struct gvt_ppgtt_entry_t {
> + int page_table_level;
> + u64 pdp[4];
> +};
> +
> +struct gvt_pending_workload_t {
> + int ring_id;
> + bool emulate_schedule_in;
> + struct execlist_ctx_descriptor_format ctx_desc;
> + struct intel_vgpu_elsp_dwords elsp_dwords;
> +};
> +
> +struct gvt_region_t {
> + enum gvt_migration_type_t type;
> + u32 size;   /* obj size of bytes to read/write */
> +};
> +
> +struct gvt_migration_obj_t {
> + void *img;
> + void *vgpu;
> + u32 offset;
> + struct gvt_region_t region;
> + /* operation func defines how data save-restore */
> + struct gvt_migration_operation_t *ops;
> + char *name;
> +};
> +
> +struct gvt_migration_operation_t {
> + /* called during pre-copy stage, VM is still alive */
> + int (*pre_copy)(const struct gvt_migration_obj_t *obj);
> + /* called before when VM was paused,
> +  * return bytes transferred
> +  */
> + int (*pre_save)(const struct gvt_migration_obj_t *obj);
> + /* called before load the state of device */
> + int (*pre_load)(const struct gvt_migration_obj_t *obj, u32 size);
> + /* called after load the state of device, VM already alive */
> + int (*post_load)(const struct gvt_migration_obj_t *obj, u32 size);
> +};
> +
> +struct gvt_image_header_t {
> + int version;
> + int data_size;
> + u64 crc_check;
> + u64 global_data[64];
> +};

I think this misses device info that should ship with the image,
currently what I can think is that each platform should have seperate
type, e.g BDW, SKL, KBL, etc. We won't allow to restore onto different
platform than the source.

> +
> +#endif
> diff --git a/drivers/gpu/drm/i915/gvt/mmio.c b/drivers/gpu/drm/i915/gvt/mmio.c
> index 43f65848ecd6..6221d2f274fc 100644
> --- a/drivers/gpu/drm/i915/gvt/mmio.c
> +++ b/drivers/gpu/drm/i915/gvt/mmio.c
> @@ -50,6 +50,19 @@ int intel_vgpu_gpa_to_mmio_offset(struct intel_vgpu *vgpu, 
> u64 gpa)
>   return gpa - gttmmio_gpa;
>  }
>  
> +/**
> + * intel_vgpu_mmio_offset_to_GPA - translate a MMIO offset to GPA
> + * @vgpu: a vGPU
> + *
> + * Returns:
> + * Zero on success, negative error code if failed
> + */
> +int intel_vgpu_mmio_offset_to_gpa(struct intel_vgpu *vgpu, u64 offset)
> +{
> + return offset + ((*(u64 *)(vgpu_cfg_space(vgpu) + PCI_BASE_ADDRESS_0)) &
> + ~GENMASK(3, 0));
> +}
> +
>  #define reg_is_mmio(gvt, reg)  \
>   (reg >= 0 && reg < gvt->device_info.mmio_size)
>  
> diff --git a/drivers/gpu/drm/i915/gvt/mmio.h b/drivers/gpu/drm/i915/gvt/mmio.h
> index 1ffc69eba30e..a2bddb0257cf 100644
> --- a/drivers/gpu/drm/i915/gvt/mmio.h
> +++ b/drivers/gpu/drm/i915/gvt/mmio.h
> @@ -82,6 +82,7 @@ void intel_vgpu_reset_mmio(struct intel_vgpu *vgpu, bool 
> dmlr);
>  void intel_vgpu_clean_mmio(struct intel_vgpu *vgpu);
>  
>  int intel_vgpu_gpa_to_mmio_offset(struct intel_vgpu *vgpu, u64 gpa);
> +int intel_vgpu_mmio_offset_to_gpa(struct intel_vgpu *vgpu, u64 offset);
>  
>  int intel_vgpu_emulate_mmio_read(struct intel_vgpu *vgpu, u64 pa,
>   void *p_data, unsigned int bytes);
> diff --git a/drivers/gpu/drm/i915/gvt/vgpu.c b/drivers/gpu/drm/i915/gvt/vgpu.c
> index fcccda35a456..7676dcfdca09 100644
> --- a/drivers/gpu/drm/i915/gvt/vgpu.c
> +++ b/drivers/gpu/drm/i915/gvt/vgpu.c
> @@ -213,6 +213,7 @@ void intel_gvt_activate_vgpu(struct intel_vgpu *vgpu)
>  {
>   mutex_lock(>gvt->lock);
>   vgpu->active = true;
> + intel_vgpu_start_schedule(vgpu);
>   mutex_unlock(>gvt->lock);
>  }
>  
> -- 
> 2.17.1
> 
> ___
> intel-gvt-dev mailing list
> intel-gvt-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [PATCH 1/2] vfio: add edid api for display (vgpu) devices.

[Zhenyu Wang]

On 2018.09.17 10:50:33 +0200, Gerd Hoffmann wrote:
> > > +#define VFIO_DEVICE_INFO_CAP_EDID1
> > > +
> > > +struct vfio_device_info_edid_cap {
> > > + struct vfio_info_cap_header header;
> > > + __u32   max_x; /* Max display height (zero == no limit) */
> > > + __u32   max_y; /* Max display height (zero == no limit) */
> > > +};
> > 
> > As current virtual display for Intel vGPU is still emulating against real HW
> > pipeline with same limitations, asked display developers that whether or not
> > specific mode can work might still depend on current or future HW behavior.
> > So could we add some hints on what kind of edid mode vfio device can 
> > operate?
> > Some may support arbitrary modes, but some may only support standard modes.
> 
> What kind of restrictions do we have here?  Really to a fixed list of
> standard modes?

Restriction is still with HW differences, e.g for skl/kbl with ddi wrpll
within min/max clock range which may generate any required frequency, but
I've been told for bxt there're some gaps in clock range that could be
generated.

> 
> Some testing (kaby lake) indicates y axis has no restrictions and x axis
> gets rounded up to the next multiple of 8 pixels (32 bytes), maybe to
> align scanlines with cachelines?

That should be plane stride requirement I think.

> 
> Oh, and btw:  Seems the resolution restriction (to 1024x768 for the
> smallest vgpu type) seems to not be enforced.  Intentional?
> 

hmm, what do you mean here? Not enforce to have only one mode for vgpu type?
Or can't change to other mode?

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [PATCH 1/2] vfio: add edid api for display (vgpu) devices.

[Zhenyu Wang]

On 2018.09.17 10:50:33 +0200, Gerd Hoffmann wrote:
> > > +#define VFIO_DEVICE_INFO_CAP_EDID1
> > > +
> > > +struct vfio_device_info_edid_cap {
> > > + struct vfio_info_cap_header header;
> > > + __u32   max_x; /* Max display height (zero == no limit) */
> > > + __u32   max_y; /* Max display height (zero == no limit) */
> > > +};
> > 
> > As current virtual display for Intel vGPU is still emulating against real HW
> > pipeline with same limitations, asked display developers that whether or not
> > specific mode can work might still depend on current or future HW behavior.
> > So could we add some hints on what kind of edid mode vfio device can 
> > operate?
> > Some may support arbitrary modes, but some may only support standard modes.
> 
> What kind of restrictions do we have here?  Really to a fixed list of
> standard modes?

Restriction is still with HW differences, e.g for skl/kbl with ddi wrpll
within min/max clock range which may generate any required frequency, but
I've been told for bxt there're some gaps in clock range that could be
generated.

> 
> Some testing (kaby lake) indicates y axis has no restrictions and x axis
> gets rounded up to the next multiple of 8 pixels (32 bytes), maybe to
> align scanlines with cachelines?

That should be plane stride requirement I think.

> 
> Oh, and btw:  Seems the resolution restriction (to 1024x768 for the
> smallest vgpu type) seems to not be enforced.  Intentional?
> 

hmm, what do you mean here? Not enforce to have only one mode for vgpu type?
Or can't change to other mode?

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [PATCH 1/2] vfio: add edid api for display (vgpu) devices.

[Zhenyu Wang]

On 2018.09.14 14:25:52 +0200, Gerd Hoffmann wrote:
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 1aa7b82e81..901f279033 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -200,12 +200,25 @@ struct vfio_device_info {
>  #define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2)  /* vfio-platform device */
>  #define VFIO_DEVICE_FLAGS_AMBA  (1 << 3) /* vfio-amba device */
>  #define VFIO_DEVICE_FLAGS_CCW(1 << 4)/* vfio-ccw device */
> +#define VFIO_DEVICE_FLAGS_CAPS   (1 << 5)/* cap_offset present */
>   __u32   num_regions;/* Max region index + 1 */
>   __u32   num_irqs;   /* Max IRQ index + 1 */
> + __u32   cap_offset; /* Offset within info struct of first cap */
>  };
>  #define VFIO_DEVICE_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 7)
>  
>  /*
> + * FIXME: docs ...
> + */
> +#define VFIO_DEVICE_INFO_CAP_EDID1
> +
> +struct vfio_device_info_edid_cap {
> + struct vfio_info_cap_header header;
> + __u32   max_x; /* Max display height (zero == no limit) */
> + __u32   max_y; /* Max display height (zero == no limit) */
> +};

As current virtual display for Intel vGPU is still emulating against real HW
pipeline with same limitations, asked display developers that whether or not
specific mode can work might still depend on current or future HW behavior.
So could we add some hints on what kind of edid mode vfio device can operate?
Some may support arbitrary modes, but some may only support standard modes.

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [PATCH 1/2] vfio: add edid api for display (vgpu) devices.

[Zhenyu Wang]

On 2018.09.14 14:25:52 +0200, Gerd Hoffmann wrote:
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 1aa7b82e81..901f279033 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -200,12 +200,25 @@ struct vfio_device_info {
>  #define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2)  /* vfio-platform device */
>  #define VFIO_DEVICE_FLAGS_AMBA  (1 << 3) /* vfio-amba device */
>  #define VFIO_DEVICE_FLAGS_CCW(1 << 4)/* vfio-ccw device */
> +#define VFIO_DEVICE_FLAGS_CAPS   (1 << 5)/* cap_offset present */
>   __u32   num_regions;/* Max region index + 1 */
>   __u32   num_irqs;   /* Max IRQ index + 1 */
> + __u32   cap_offset; /* Offset within info struct of first cap */
>  };
>  #define VFIO_DEVICE_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 7)
>  
>  /*
> + * FIXME: docs ...
> + */
> +#define VFIO_DEVICE_INFO_CAP_EDID1
> +
> +struct vfio_device_info_edid_cap {
> + struct vfio_info_cap_header header;
> + __u32   max_x; /* Max display height (zero == no limit) */
> + __u32   max_y; /* Max display height (zero == no limit) */
> +};

As current virtual display for Intel vGPU is still emulating against real HW
pipeline with same limitations, asked display developers that whether or not
specific mode can work might still depend on current or future HW behavior.
So could we add some hints on what kind of edid mode vfio device can operate?
Some may support arbitrary modes, but some may only support standard modes.

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [PATCH v4 1/2] vfio/mdev: Check globally for duplicate devices

[Zhenyu Wang]

On 2018.05.22 09:53:37 -0600, Alex Williamson wrote:
> [Cc +GVT-g maintainers/lists]
> 
> On Tue, 22 May 2018 10:13:46 +0200
> Cornelia Huck <coh...@redhat.com> wrote:
> 
> > On Fri, 18 May 2018 13:10:25 -0600
> > Alex Williamson <alex.william...@redhat.com> wrote:
> > 
> > > When we create an mdev device, we check for duplicates against the
> > > parent device and return -EEXIST if found, but the mdev device
> > > namespace is global since we'll link all devices from the bus.  We do
> > > catch this later in sysfs_do_create_link_sd() to return -EEXIST, but
> > > with it comes a kernel warning and stack trace for trying to create
> > > duplicate sysfs links, which makes it an undesirable response.
> > > 
> > > Therefore we should really be looking for duplicates across all mdev
> > > parent devices, or as implemented here, against our mdev device list.
> > > Using mdev_list to prevent duplicates means that we can remove
> > > mdev_parent.lock, but in order not to serialize mdev device creation
> > > and removal globally, we add mdev_device.active which allows UUIDs to
> > > be reserved such that we can drop the mdev_list_lock before the mdev
> > > device is fully in place.
> > > 
> > > Two behavioral notes; first, mdev_parent.lock had the side-effect of
> > > serializing mdev create and remove ops per parent device.  This was
> > > an implementation detail, not an intentional guarantee provided to
> > > the mdev vendor drivers.  Vendor drivers can trivially provide this
> > > serialization internally if necessary.  Second, review comments note
> > > the new -EAGAIN behavior when the device, and in particular the remove
> > > attribute, becomes visible in sysfs.  If a remove is triggered prior
> > > to completion of mdev_device_create() the user will see a -EAGAIN
> > > error.  While the errno is different, receiving an error during this
> > > period is not, the previous implementation returned -ENODEV for the
> > > same condition.  Furthermore, the consistency to the user is improved
> > > in the case where mdev_device_remove_ops() returns error.  Previously
> > > concurrent calls to mdev_device_remove() could see the device
> > > disappear with -ENODEV and return in the case of error.  Now a user
> > > would see -EAGAIN while the device is in this transitory state.
> > > 
> > > Signed-off-by: Alex Williamson <alex.william...@redhat.com>
> > > ---
> > >  Documentation/vfio-mediated-device.txt |5 ++
> > >  drivers/vfio/mdev/mdev_core.c  |  102 
> > > +++-
> > >  drivers/vfio/mdev/mdev_private.h   |2 -
> > >  3 files changed, 42 insertions(+), 67 deletions(-)  
> > 
> > Reviewed-by: Cornelia Huck <coh...@redhat.com>
> > 
> > I think it is better to deal with any possible vendor driver
> > implications on top of this (I still believe that vfio-ccw is fine).
> 
> Thanks Cornelia.  So if vfio-ccw is fine, presumably NVIDIA is fine,
> then this leaves GVT-g to see if there's any fallout.  Zhenyu & Zhi,
> I've linked the series under discussion here below[1].  The question to
> you is the first of the two behavioral notes listed above, does GVT-g
> have any dependency on the mdev core providing serialization per mdev
> parent device for the create and remove callbacks within the
> mdev_parent_ops?  This was never an intended feature of the
> implementation and as noted it should be trivial for for an mdev vendor
> driver to provide equivalent course grained serialization if
> necessary.  Of course it would be better to implement that sooner
> rather than later if required.
> 
> I see that __intel_gvt_create_vgpu() makes use of gvt->lock, which
> would seem to already provide this level of per-parent locking. The
> remove path makes use of this same lock, so I think we're ok, but
> looking for an explicit ack so there are no surprises.  I'd like
> to queue this series for v4.18.  Thanks,
> 

yeah, we don't expect mdev core for parent serialization for create and
remove of mdev device. Series look good to me.

Acked-by: Zhenyu Wang <zhen...@linux.intel.com>


> Alex
> 
> [1] https://lkml.org/lkml/2018/5/18/1035

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [PATCH v4 1/2] vfio/mdev: Check globally for duplicate devices

[Zhenyu Wang]

On 2018.05.22 09:53:37 -0600, Alex Williamson wrote:
> [Cc +GVT-g maintainers/lists]
> 
> On Tue, 22 May 2018 10:13:46 +0200
> Cornelia Huck  wrote:
> 
> > On Fri, 18 May 2018 13:10:25 -0600
> > Alex Williamson  wrote:
> > 
> > > When we create an mdev device, we check for duplicates against the
> > > parent device and return -EEXIST if found, but the mdev device
> > > namespace is global since we'll link all devices from the bus.  We do
> > > catch this later in sysfs_do_create_link_sd() to return -EEXIST, but
> > > with it comes a kernel warning and stack trace for trying to create
> > > duplicate sysfs links, which makes it an undesirable response.
> > > 
> > > Therefore we should really be looking for duplicates across all mdev
> > > parent devices, or as implemented here, against our mdev device list.
> > > Using mdev_list to prevent duplicates means that we can remove
> > > mdev_parent.lock, but in order not to serialize mdev device creation
> > > and removal globally, we add mdev_device.active which allows UUIDs to
> > > be reserved such that we can drop the mdev_list_lock before the mdev
> > > device is fully in place.
> > > 
> > > Two behavioral notes; first, mdev_parent.lock had the side-effect of
> > > serializing mdev create and remove ops per parent device.  This was
> > > an implementation detail, not an intentional guarantee provided to
> > > the mdev vendor drivers.  Vendor drivers can trivially provide this
> > > serialization internally if necessary.  Second, review comments note
> > > the new -EAGAIN behavior when the device, and in particular the remove
> > > attribute, becomes visible in sysfs.  If a remove is triggered prior
> > > to completion of mdev_device_create() the user will see a -EAGAIN
> > > error.  While the errno is different, receiving an error during this
> > > period is not, the previous implementation returned -ENODEV for the
> > > same condition.  Furthermore, the consistency to the user is improved
> > > in the case where mdev_device_remove_ops() returns error.  Previously
> > > concurrent calls to mdev_device_remove() could see the device
> > > disappear with -ENODEV and return in the case of error.  Now a user
> > > would see -EAGAIN while the device is in this transitory state.
> > > 
> > > Signed-off-by: Alex Williamson 
> > > ---
> > >  Documentation/vfio-mediated-device.txt |5 ++
> > >  drivers/vfio/mdev/mdev_core.c  |  102 
> > > +++-
> > >  drivers/vfio/mdev/mdev_private.h   |2 -
> > >  3 files changed, 42 insertions(+), 67 deletions(-)  
> > 
> > Reviewed-by: Cornelia Huck 
> > 
> > I think it is better to deal with any possible vendor driver
> > implications on top of this (I still believe that vfio-ccw is fine).
> 
> Thanks Cornelia.  So if vfio-ccw is fine, presumably NVIDIA is fine,
> then this leaves GVT-g to see if there's any fallout.  Zhenyu & Zhi,
> I've linked the series under discussion here below[1].  The question to
> you is the first of the two behavioral notes listed above, does GVT-g
> have any dependency on the mdev core providing serialization per mdev
> parent device for the create and remove callbacks within the
> mdev_parent_ops?  This was never an intended feature of the
> implementation and as noted it should be trivial for for an mdev vendor
> driver to provide equivalent course grained serialization if
> necessary.  Of course it would be better to implement that sooner
> rather than later if required.
> 
> I see that __intel_gvt_create_vgpu() makes use of gvt->lock, which
> would seem to already provide this level of per-parent locking. The
> remove path makes use of this same lock, so I think we're ok, but
> looking for an explicit ack so there are no surprises.  I'd like
> to queue this series for v4.18.  Thanks,
> 

yeah, we don't expect mdev core for parent serialization for create and
remove of mdev device. Series look good to me.

Acked-by: Zhenyu Wang 


> Alex
> 
> [1] https://lkml.org/lkml/2018/5/18/1035

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [PATCH] drm/i915/gvt/scheduler: Remove unnecessary NULL checks in sr_oa_regs

[Zhenyu Wang]

On 2018.03.22 21:31:33 +, Chris Wilson wrote:
> Quoting Gustavo A. R. Silva (2018-03-22 18:21:54)
> > The checks are misleading and not required [1].
> > 
> > [1] https://lkml.org/lkml/2018/3/19/1792
> > 
> > Addresses-Coverity-ID: 1466017
> > Cc: Chris Wilson 
> > Signed-off-by: Gustavo A. R. Silva 
> Reviewed-by: Chris Wilson 
> 

Looks good to me, I will pick this up, thanks!

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [PATCH] drm/i915/gvt/scheduler: Remove unnecessary NULL checks in sr_oa_regs

[Zhenyu Wang]

On 2018.03.22 21:31:33 +, Chris Wilson wrote:
> Quoting Gustavo A. R. Silva (2018-03-22 18:21:54)
> > The checks are misleading and not required [1].
> > 
> > [1] https://lkml.org/lkml/2018/3/19/1792
> > 
> > Addresses-Coverity-ID: 1466017
> > Cc: Chris Wilson 
> > Signed-off-by: Gustavo A. R. Silva 
> Reviewed-by: Chris Wilson 
> 

Looks good to me, I will pick this up, thanks!

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [PATCH][drm-next] drm/i915/gvt: fix spelling mistake: "destoried" -> "destroyed"

[Zhenyu Wang]

On 2018.03.12 12:43:58 +0100, Colin King wrote:
> From: Colin Ian King 
> 
> Trivial fix to spelling mistake in gvt_err error message text.
> 
> Signed-off-by: Colin Ian King 
> ---

Thanks Colin, will pick up.

>  drivers/gpu/drm/i915/gvt/gtt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
> index 0a100a288e6d..8eb76becd676 100644
> --- a/drivers/gpu/drm/i915/gvt/gtt.c
> +++ b/drivers/gpu/drm/i915/gvt/gtt.c
> @@ -2046,7 +2046,7 @@ static void intel_vgpu_destroy_all_ppgtt_mm(struct 
> intel_vgpu *vgpu)
>   }
>  
>   if (GEM_WARN_ON(!list_empty(>gtt.ppgtt_mm_list_head)))
> - gvt_err("vgpu ppgtt mm is not fully destoried\n");
> + gvt_err("vgpu ppgtt mm is not fully destroyed\n");
>  
>   if (GEM_WARN_ON(!radix_tree_empty(>gtt.spt_tree))) {
>   gvt_err("Why we still has spt not freed?\n");
> -- 
> 2.15.1
> 

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [PATCH][drm-next] drm/i915/gvt: fix spelling mistake: "destoried" -> "destroyed"

[Zhenyu Wang]

On 2018.03.12 12:43:58 +0100, Colin King wrote:
> From: Colin Ian King 
> 
> Trivial fix to spelling mistake in gvt_err error message text.
> 
> Signed-off-by: Colin Ian King 
> ---

Thanks Colin, will pick up.

>  drivers/gpu/drm/i915/gvt/gtt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
> index 0a100a288e6d..8eb76becd676 100644
> --- a/drivers/gpu/drm/i915/gvt/gtt.c
> +++ b/drivers/gpu/drm/i915/gvt/gtt.c
> @@ -2046,7 +2046,7 @@ static void intel_vgpu_destroy_all_ppgtt_mm(struct 
> intel_vgpu *vgpu)
>   }
>  
>   if (GEM_WARN_ON(!list_empty(>gtt.ppgtt_mm_list_head)))
> - gvt_err("vgpu ppgtt mm is not fully destoried\n");
> + gvt_err("vgpu ppgtt mm is not fully destroyed\n");
>  
>   if (GEM_WARN_ON(!radix_tree_empty(>gtt.spt_tree))) {
>   gvt_err("Why we still has spt not freed?\n");
> -- 
> 2.15.1
> 

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [PATCH] vfio: Simplify capability helper

[Zhenyu Wang]

On 2017.12.13 12:13:34 +1100, Alexey Kardashevskiy wrote:
> On 13/12/17 06:59, Alex Williamson wrote:
> > The vfio_info_add_capability() helper requires the caller to pass a
> > capability ID, which it then uses to fill in header fields, assuming
> > hard coded versions.  This makes for an awkward and rigid interface.
> > The only thing we want this helper to do is allocate sufficient
> > space in the caps buffer and chain this capability into the list.
> > Reduce it to that simple task.
> > 
> > Signed-off-by: Alex Williamson <alex.william...@redhat.com>
> 
> 
> Makes more sense now, thanks. I'll repost mine on top of this.
> 
> 
> Reviewed-by: Alexey Kardashevskiy <a...@ozlabs.ru>
> 
>

Looks good for KVMGT part.

Acked-by: Zhenyu Wang <zhen...@linux.intel.com>

> Below one observation, unrelated to this patch.
> 
> > ---
> >  drivers/gpu/drm/i915/gvt/kvmgt.c |   15 +++
> >  drivers/vfio/pci/vfio_pci.c  |   14 ++
> >  drivers/vfio/vfio.c  |   52 
> > +++---
> >  include/linux/vfio.h |3 +-
> >  4 files changed, 24 insertions(+), 60 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c 
> > b/drivers/gpu/drm/i915/gvt/kvmgt.c
> > index 96060920a6fe..0a7d084da1a2 100644
> > --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> > +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> > @@ -1012,6 +1012,8 @@ static long intel_vgpu_ioctl(struct mdev_device 
> > *mdev, unsigned int cmd,
> > if (!sparse)
> > return -ENOMEM;
> >  
> > +   sparse->header.id = VFIO_REGION_INFO_CAP_SPARSE_MMAP;
> > +   sparse->header.version = 1;
> > sparse->nr_areas = nr_areas;
> > cap_type_id = VFIO_REGION_INFO_CAP_SPARSE_MMAP;
> 
> 
> @cap_type_id is initialized in just one of many cases of switch
> (info.index) and after the entire switch, there is switch (cap_type_id). I
> wonder why compiler missed "potentially uninitialized variable here,
> although there is no bug - @cap_type_id is in sync with @spapse. It would
> make it cleaner imho just to have vfio_info_add_capability() next to the
> header initialization.
> 

yeah, we could clean that up, thanks for pointing out.

> 
> 
> > sparse->areas[0].offset =
> > @@ -1033,7 +1035,9 @@ static long intel_vgpu_ioctl(struct mdev_device 
> > *mdev, unsigned int cmd,
> > break;
> > default:
> > {
> > -   struct vfio_region_info_cap_type cap_type;
> > +   struct vfio_region_info_cap_type cap_type = {
> > +   .header.id = VFIO_REGION_INFO_CAP_TYPE,
> > +   .header.version = 1 };
> >  
> > if (info.index >= VFIO_PCI_NUM_REGIONS +
> > vgpu->vdev.num_regions)
> > @@ -1050,8 +1054,8 @@ static long intel_vgpu_ioctl(struct mdev_device 
> > *mdev, unsigned int cmd,
> > cap_type.subtype = vgpu->vdev.region[i].subtype;
> >  
> > ret = vfio_info_add_capability(,
> > -   VFIO_REGION_INFO_CAP_TYPE,
> > -   _type);
> > +   _type.header,
> > +   sizeof(cap_type));
> > if (ret)
> > return ret;
> > }
> > @@ -1061,8 +1065,9 @@ static long intel_vgpu_ioctl(struct mdev_device 
> > *mdev, unsigned int cmd,
> > switch (cap_type_id) {
> > case VFIO_REGION_INFO_CAP_SPARSE_MMAP:
> > ret = vfio_info_add_capability(,
> > -   VFIO_REGION_INFO_CAP_SPARSE_MMAP,
> > -   sparse);
> > +   >header, sizeof(*sparse) +
> > +   (sparse->nr_areas *
> > +   sizeof(*sparse->areas)));
> > kfree(sparse);
> > if (ret)
> > return ret;
> > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> > index f041b1a6cf66..a

Re: [PATCH] vfio: Simplify capability helper

[Zhenyu Wang]

On 2017.12.13 12:13:34 +1100, Alexey Kardashevskiy wrote:
> On 13/12/17 06:59, Alex Williamson wrote:
> > The vfio_info_add_capability() helper requires the caller to pass a
> > capability ID, which it then uses to fill in header fields, assuming
> > hard coded versions.  This makes for an awkward and rigid interface.
> > The only thing we want this helper to do is allocate sufficient
> > space in the caps buffer and chain this capability into the list.
> > Reduce it to that simple task.
> > 
> > Signed-off-by: Alex Williamson 
> 
> 
> Makes more sense now, thanks. I'll repost mine on top of this.
> 
> 
> Reviewed-by: Alexey Kardashevskiy 
> 
>

Looks good for KVMGT part.

Acked-by: Zhenyu Wang 

> Below one observation, unrelated to this patch.
> 
> > ---
> >  drivers/gpu/drm/i915/gvt/kvmgt.c |   15 +++
> >  drivers/vfio/pci/vfio_pci.c  |   14 ++
> >  drivers/vfio/vfio.c  |   52 
> > +++---
> >  include/linux/vfio.h |3 +-
> >  4 files changed, 24 insertions(+), 60 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c 
> > b/drivers/gpu/drm/i915/gvt/kvmgt.c
> > index 96060920a6fe..0a7d084da1a2 100644
> > --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> > +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> > @@ -1012,6 +1012,8 @@ static long intel_vgpu_ioctl(struct mdev_device 
> > *mdev, unsigned int cmd,
> > if (!sparse)
> > return -ENOMEM;
> >  
> > +   sparse->header.id = VFIO_REGION_INFO_CAP_SPARSE_MMAP;
> > +   sparse->header.version = 1;
> > sparse->nr_areas = nr_areas;
> > cap_type_id = VFIO_REGION_INFO_CAP_SPARSE_MMAP;
> 
> 
> @cap_type_id is initialized in just one of many cases of switch
> (info.index) and after the entire switch, there is switch (cap_type_id). I
> wonder why compiler missed "potentially uninitialized variable here,
> although there is no bug - @cap_type_id is in sync with @spapse. It would
> make it cleaner imho just to have vfio_info_add_capability() next to the
> header initialization.
> 

yeah, we could clean that up, thanks for pointing out.

> 
> 
> > sparse->areas[0].offset =
> > @@ -1033,7 +1035,9 @@ static long intel_vgpu_ioctl(struct mdev_device 
> > *mdev, unsigned int cmd,
> > break;
> > default:
> > {
> > -   struct vfio_region_info_cap_type cap_type;
> > +   struct vfio_region_info_cap_type cap_type = {
> > +   .header.id = VFIO_REGION_INFO_CAP_TYPE,
> > +   .header.version = 1 };
> >  
> > if (info.index >= VFIO_PCI_NUM_REGIONS +
> > vgpu->vdev.num_regions)
> > @@ -1050,8 +1054,8 @@ static long intel_vgpu_ioctl(struct mdev_device 
> > *mdev, unsigned int cmd,
> > cap_type.subtype = vgpu->vdev.region[i].subtype;
> >  
> > ret = vfio_info_add_capability(,
> > -   VFIO_REGION_INFO_CAP_TYPE,
> > -   _type);
> > +   _type.header,
> > +   sizeof(cap_type));
> > if (ret)
> > return ret;
> > }
> > @@ -1061,8 +1065,9 @@ static long intel_vgpu_ioctl(struct mdev_device 
> > *mdev, unsigned int cmd,
> > switch (cap_type_id) {
> > case VFIO_REGION_INFO_CAP_SPARSE_MMAP:
> > ret = vfio_info_add_capability(,
> > -   VFIO_REGION_INFO_CAP_SPARSE_MMAP,
> > -   sparse);
> > +   >header, sizeof(*sparse) +
> > +   (sparse->nr_areas *
> > +   sizeof(*sparse->areas)));
> > kfree(sparse);
> > if (ret)
> > return ret;
> > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> > index f041b1a6cf66..a73e40983880 100644
> > --- a/drivers/vfio/pci/vfio_pci.c
> > +++ b/dr

Re: [PATCH] drm/i915/gvt/fb_decoder: Fix out-of-bounds read

[Zhenyu Wang]

On 2017.12.09 00:37:59 -0600, Gustavo A. R. Silva wrote:
> In case function skl_format_to_drm returns -EINVAL, fmt turns into a huge
> number as fmt is of type u32, hence there is an out-of-bounds read when
> using fmt as an index for array skl_pixel_formats at line 225:
> plane->bpp = skl_pixel_formats[fmt].bpp;
> 
> Fix this by comparing the value returned by function skl_format_to_drm
> against the size of array skl_pixel_formats, so in case it is greater than
> or equal to the number of items contained in skl_pixel_formats, print an
> error message and return -EINVAL.
> 
> Addresses-Coverity-ID: 1462495
> Addresses-Coverity-ID: 1462502 ("Out-of-bounds read")
> Fixes: 9f31d1063b43 ("drm/i915/gvt: Add framebuffer decoder support")
> Signed-off-by: Gustavo A. R. Silva 
> ---
>  drivers/gpu/drm/i915/gvt/fb_decoder.c | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/fb_decoder.c 
> b/drivers/gpu/drm/i915/gvt/fb_decoder.c
> index 72f4217..aed578b 100644
> --- a/drivers/gpu/drm/i915/gvt/fb_decoder.c
> +++ b/drivers/gpu/drm/i915/gvt/fb_decoder.c
> @@ -222,6 +222,12 @@ int intel_vgpu_decode_primary_plane(struct intel_vgpu 
> *vgpu,
>   val & PLANE_CTL_ORDER_RGBX,
>   val & PLANE_CTL_ALPHA_MASK,
>   val & PLANE_CTL_YUV422_ORDER_MASK);
> +
> + if (fmt >= ARRAY_SIZE(skl_pixel_formats)) {
> + gvt_vgpu_err("Out-of-bounds pixel format index\n");
> + return -EINVAL;
> + }
> +
>   plane->bpp = skl_pixel_formats[fmt].bpp;
>   plane->drm_format = skl_pixel_formats[fmt].drm_format;
>   } else {
> -- 

Applied this, thanks!

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [PATCH] drm/i915/gvt/fb_decoder: Fix out-of-bounds read

[Zhenyu Wang]

On 2017.12.09 00:37:59 -0600, Gustavo A. R. Silva wrote:
> In case function skl_format_to_drm returns -EINVAL, fmt turns into a huge
> number as fmt is of type u32, hence there is an out-of-bounds read when
> using fmt as an index for array skl_pixel_formats at line 225:
> plane->bpp = skl_pixel_formats[fmt].bpp;
> 
> Fix this by comparing the value returned by function skl_format_to_drm
> against the size of array skl_pixel_formats, so in case it is greater than
> or equal to the number of items contained in skl_pixel_formats, print an
> error message and return -EINVAL.
> 
> Addresses-Coverity-ID: 1462495
> Addresses-Coverity-ID: 1462502 ("Out-of-bounds read")
> Fixes: 9f31d1063b43 ("drm/i915/gvt: Add framebuffer decoder support")
> Signed-off-by: Gustavo A. R. Silva 
> ---
>  drivers/gpu/drm/i915/gvt/fb_decoder.c | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/fb_decoder.c 
> b/drivers/gpu/drm/i915/gvt/fb_decoder.c
> index 72f4217..aed578b 100644
> --- a/drivers/gpu/drm/i915/gvt/fb_decoder.c
> +++ b/drivers/gpu/drm/i915/gvt/fb_decoder.c
> @@ -222,6 +222,12 @@ int intel_vgpu_decode_primary_plane(struct intel_vgpu 
> *vgpu,
>   val & PLANE_CTL_ORDER_RGBX,
>   val & PLANE_CTL_ALPHA_MASK,
>   val & PLANE_CTL_YUV422_ORDER_MASK);
> +
> + if (fmt >= ARRAY_SIZE(skl_pixel_formats)) {
> + gvt_vgpu_err("Out-of-bounds pixel format index\n");
> + return -EINVAL;
> + }
> +
>   plane->bpp = skl_pixel_formats[fmt].bpp;
>   plane->drm_format = skl_pixel_formats[fmt].drm_format;
>   } else {
> -- 

Applied this, thanks!

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: linux-next: Signed-off-by missing for commits in the drm tree

[Zhenyu Wang]

On 2017.12.04 10:57:35 +1100, Stephen Rothwell wrote:
> Hi all,
> 
> There is a series of commits
> 
>   54cff6479fd8 - c1802534e5a6
> (not all in that range)
> 
> that are missing a Signed-off-by from their committer.
> 

They were originally committed by Zhi himself, but I had to rebase
onto intel -next before they're sent for pull and missed my signed-off..
Really sorry for that! I think maybe we could still be able to fix that
before next kernel merge.

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: linux-next: Signed-off-by missing for commits in the drm tree

[Zhenyu Wang]

On 2017.12.04 10:57:35 +1100, Stephen Rothwell wrote:
> Hi all,
> 
> There is a series of commits
> 
>   54cff6479fd8 - c1802534e5a6
> (not all in that range)
> 
> that are missing a Signed-off-by from their committer.
> 

They were originally committed by Zhi himself, but I had to rebase
onto intel -next before they're sent for pull and missed my signed-off..
Really sorry for that! I think maybe we could still be able to fix that
before next kernel merge.

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [PATCH v18 0/6] drm/i915/gvt: Dma-buf support for GVT-g

[Zhenyu Wang]

On 2017.11.15 11:49:00 +0100, Gerd Hoffmann wrote:
> On Wed, Nov 15, 2017 at 05:11:49PM +0800, Tina Zhang wrote:
> > v17->v18:
> > 1) unmap vgpu's opregion when destroying vgpu.
> > 2) update comments for VFIO_DEVICE_GET_GFX_DMABUF. (Alex)
> 
> > This patch set adds the dma-buf support for intel GVT-g.
> > 
> > dma-buf is an uniform mechanism to share DMA buffers across different
> > devices and subsystems. dma-buf for intel GVT-g is mainly used to share
> > the vgpu's framebuffer to userspace to leverage userspace graphics stacks
> > to render the framebuffer to the display monitor.
> > 
> > The main idea is that we create a gem object and set vgpu's framebuffer as
> > its backing storage. Then, export a dma-buf associated with this gem object.
> > With the fd of this dma-buf, userspace can directly handle this buffer.
> > 
> > This patch set can be tried with the following example:
> > git://git.kraxel.org/qemu  branch: work/intel-vgpu
> > 
> > A topic branch with the latest patch set is:
> > https://github.com/intel/gvt-linux.git   branch: topic/dmabuf
> 
> Tested-by: Gerd Hoffmann 
> 

After debugging with Tina on one left race that fixed by
https://lists.freedesktop.org/archives/intel-gvt-dev/2017-November/002505.html

I still need below qemu fix for proper cursor handling, otherwise qemu
just crashed when I click in my terminal program which hides cursor then.

diff --git a/hw/vfio/display.c b/hw/vfio/display.c
index e500ec2cb1..d9a044b080 100644
--- a/hw/vfio/display.c
+++ b/hw/vfio/display.c
@@ -169,8 +169,9 @@ static void vfio_display_dmabuf_update(void *opaque)
 cursor = vfio_display_get_dmabuf(vdev, DRM_PLANE_TYPE_CURSOR);
 if (vdev->cursor != cursor) {
 vdev->cursor = cursor;
-dpy_gl_cursor_dmabuf(vdev->display_con,
- >buf);
+if (cursor)
+dpy_gl_cursor_dmabuf(vdev->display_con,
+ >buf);
 free_bufs = true;
 }
 if (cursor != NULL) {

And with these it seems pretty fine now that I'll queue them up for -next pull.

thanks

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [PATCH v18 0/6] drm/i915/gvt: Dma-buf support for GVT-g

[Zhenyu Wang]

On 2017.11.15 11:49:00 +0100, Gerd Hoffmann wrote:
> On Wed, Nov 15, 2017 at 05:11:49PM +0800, Tina Zhang wrote:
> > v17->v18:
> > 1) unmap vgpu's opregion when destroying vgpu.
> > 2) update comments for VFIO_DEVICE_GET_GFX_DMABUF. (Alex)
> 
> > This patch set adds the dma-buf support for intel GVT-g.
> > 
> > dma-buf is an uniform mechanism to share DMA buffers across different
> > devices and subsystems. dma-buf for intel GVT-g is mainly used to share
> > the vgpu's framebuffer to userspace to leverage userspace graphics stacks
> > to render the framebuffer to the display monitor.
> > 
> > The main idea is that we create a gem object and set vgpu's framebuffer as
> > its backing storage. Then, export a dma-buf associated with this gem object.
> > With the fd of this dma-buf, userspace can directly handle this buffer.
> > 
> > This patch set can be tried with the following example:
> > git://git.kraxel.org/qemu  branch: work/intel-vgpu
> > 
> > A topic branch with the latest patch set is:
> > https://github.com/intel/gvt-linux.git   branch: topic/dmabuf
> 
> Tested-by: Gerd Hoffmann 
> 

After debugging with Tina on one left race that fixed by
https://lists.freedesktop.org/archives/intel-gvt-dev/2017-November/002505.html

I still need below qemu fix for proper cursor handling, otherwise qemu
just crashed when I click in my terminal program which hides cursor then.

diff --git a/hw/vfio/display.c b/hw/vfio/display.c
index e500ec2cb1..d9a044b080 100644
--- a/hw/vfio/display.c
+++ b/hw/vfio/display.c
@@ -169,8 +169,9 @@ static void vfio_display_dmabuf_update(void *opaque)
 cursor = vfio_display_get_dmabuf(vdev, DRM_PLANE_TYPE_CURSOR);
 if (vdev->cursor != cursor) {
 vdev->cursor = cursor;
-dpy_gl_cursor_dmabuf(vdev->display_con,
- >buf);
+if (cursor)
+dpy_gl_cursor_dmabuf(vdev->display_con,
+ >buf);
 free_bufs = true;
 }
 if (cursor != NULL) {

And with these it seems pretty fine now that I'll queue them up for -next pull.

thanks

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [PATCH v18 5/6] vfio: ABI for mdev display dma-buf operation

[Zhenyu Wang]

On 2017.11.15 11:48:42 -0700, Alex Williamson wrote:
> On Wed, 15 Nov 2017 17:11:54 +0800
> Tina Zhang  wrote:
> 
> > Add VFIO_DEVICE_QUERY_GFX_PLANE ioctl command to let user query and get
> > a plane and its information. So far, two types of buffers are supported:
> > buffers based on dma-buf and buffers based on region.
> > 
> > This ioctl can be invoked with:
> > 1) Either DMABUF or REGION flag. Vendor driver returns a plane_info
> > successfully only when the specific kind of buffer is supported.
> > 2) Flag PROBE. And at the same time either DMABUF or REGION must be set,
> > so that vendor driver returns success only when the specific kind of
> > buffer is supported.
> > 
> > Add VFIO_DEVICE_GET_GFX_DMABUF ioctl command to let user get a specific
> > dma-buf fd of an exposed MDEV buffer provided by dmabuf_id which was
> > returned in VFIO_DEVICE_QUERY_GFX_PLANE ioctl command.
> > 
> > The life cycle of an exposed MDEV buffer is handled by userspace and
> > tracked by kernel space. The returned dmabuf_id in struct vfio_device_
> > query_gfx_plane can be a new id of a new exposed buffer or an old id of
> > a re-exported buffer. Host user can check the value of dmabuf_id to see
> > if it needs to create new resources according to the new exposed buffer
> > or just re-use the existing resource related to the old buffer.
> > 
> > v18:
> > - update comments for VFIO_DEVICE_GET_GFX_DMABUF. (Alex)
> > 
> > v17:
> > - modify VFIO_DEVICE_GET_GFX_DMABUF interface. (Alex)
> > 
> > v16:
> > - add x_hot and y_hot fields. (Gerd)
> > - add comments for VFIO_DEVICE_GET_GFX_DMABUF. (Alex)
> > - rebase to 4.14.0-rc6.
> > 
> > v15:
> > - add a ioctl to get a dmabuf for a given dmabuf id. (Gerd)
> > 
> > v14:
> > - add PROBE, DMABUF and REGION flags. (Alex)
> > 
> > v12:
> > - add drm_format_mod back. (Gerd and Zhenyu)
> > - add region_index. (Gerd)
> > 
> > v11:
> > - rename plane_type to drm_plane_type. (Gerd)
> > - move fields of vfio_device_query_gfx_plane to vfio_device_gfx_plane_info.
> >   (Gerd)
> > - remove drm_format_mod, start fields. (Daniel)
> > - remove plane_id.
> > 
> > v10:
> > - refine the ABI API VFIO_DEVICE_QUERY_GFX_PLANE. (Alex) (Gerd)
> > 
> > v3:
> > - add a field gvt_plane_info in the drm_i915_gem_obj structure to save
> >   the decoded plane information to avoid look up while need the plane
> >   info. (Gerd)
> > 
> > Signed-off-by: Tina Zhang 
> > Cc: Gerd Hoffmann 
> > Cc: Alex Williamson 
> > Cc: Daniel Vetter 
> > ---
> >  include/uapi/linux/vfio.h | 62 
> > +++
> >  1 file changed, 62 insertions(+)
> > 
> > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > index ae46105..5c1cca2 100644
> > --- a/include/uapi/linux/vfio.h
> > +++ b/include/uapi/linux/vfio.h
> > @@ -502,6 +502,68 @@ struct vfio_pci_hot_reset {
> >  
> >  #define VFIO_DEVICE_PCI_HOT_RESET  _IO(VFIO_TYPE, VFIO_BASE + 13)
> >  
> > +/**
> > + * VFIO_DEVICE_QUERY_GFX_PLANE - _IOW(VFIO_TYPE, VFIO_BASE + 14,
> > + *struct vfio_device_query_gfx_plane)
> > + *
> > + * Set the drm_plane_type and flags, then retrieve the gfx plane info.
> > + *
> > + * flags supported:
> > + * - VFIO_GFX_PLANE_TYPE_PROBE and VFIO_GFX_PLANE_TYPE_DMABUF are set
> > + *   to ask if the mdev supports dma-buf. 0 on support, -EINVAL on no
> > + *   support for dma-buf.
> > + * - VFIO_GFX_PLANE_TYPE_PROBE and VFIO_GFX_PLANE_TYPE_REGION are set
> > + *   to ask if the mdev supports region. 0 on support, -EINVAL on no
> > + *   support for region.
> > + * - VFIO_GFX_PLANE_TYPE_DMABUF or VFIO_GFX_PLANE_TYPE_REGION is set
> > + *   with each call to query the plane info.
> > + * - Others are invalid and return -EINVAL.
> > + *
> > + * Note:
> > + * 1. Plane could be disabled by guest. In that case, success will be
> > + *returned with zero-initialized drm_format, size, width and height
> > + *fields.
> > + * 2. x_hot/y_hot is set to 0x if no hotspot information available
> > + *
> > + * Return: 0 on success, -errno on other failure.
> > + */
> > +struct vfio_device_gfx_plane_info {
> > +   __u32 argsz;
> > +   __u32 flags;
> > +#define VFIO_GFX_PLANE_TYPE_PROBE (1 << 0)
> > +#define VFIO_GFX_PLANE_TYPE_DMABUF (1 << 1)
> > +#define VFIO_GFX_PLANE_TYPE_REGION (1 << 2)
> > +   /* in */
> > +   __u32 drm_plane_type;   /* type of plane: DRM_PLANE_TYPE_* */
> > +   /* out */
> > +   __u32 drm_format;   /* drm format of plane */
> > +   __u64 drm_format_mod;   /* tiled mode */
> > +   __u32 width;/* width of plane */
> > +   __u32 height;   /* height of plane */
> > +   __u32 stride;   /* stride of plane */
> > +   __u32 size; /* size of plane in bytes, align on page*/
> > +   __u32 x_pos;/* horizontal position of cursor plane */
> > +   __u32 y_pos;/* vertical position of cursor plane*/
> > +   __u32 x_hot;/* 

Re: [PATCH v18 5/6] vfio: ABI for mdev display dma-buf operation

[Zhenyu Wang]

On 2017.11.15 11:48:42 -0700, Alex Williamson wrote:
> On Wed, 15 Nov 2017 17:11:54 +0800
> Tina Zhang  wrote:
> 
> > Add VFIO_DEVICE_QUERY_GFX_PLANE ioctl command to let user query and get
> > a plane and its information. So far, two types of buffers are supported:
> > buffers based on dma-buf and buffers based on region.
> > 
> > This ioctl can be invoked with:
> > 1) Either DMABUF or REGION flag. Vendor driver returns a plane_info
> > successfully only when the specific kind of buffer is supported.
> > 2) Flag PROBE. And at the same time either DMABUF or REGION must be set,
> > so that vendor driver returns success only when the specific kind of
> > buffer is supported.
> > 
> > Add VFIO_DEVICE_GET_GFX_DMABUF ioctl command to let user get a specific
> > dma-buf fd of an exposed MDEV buffer provided by dmabuf_id which was
> > returned in VFIO_DEVICE_QUERY_GFX_PLANE ioctl command.
> > 
> > The life cycle of an exposed MDEV buffer is handled by userspace and
> > tracked by kernel space. The returned dmabuf_id in struct vfio_device_
> > query_gfx_plane can be a new id of a new exposed buffer or an old id of
> > a re-exported buffer. Host user can check the value of dmabuf_id to see
> > if it needs to create new resources according to the new exposed buffer
> > or just re-use the existing resource related to the old buffer.
> > 
> > v18:
> > - update comments for VFIO_DEVICE_GET_GFX_DMABUF. (Alex)
> > 
> > v17:
> > - modify VFIO_DEVICE_GET_GFX_DMABUF interface. (Alex)
> > 
> > v16:
> > - add x_hot and y_hot fields. (Gerd)
> > - add comments for VFIO_DEVICE_GET_GFX_DMABUF. (Alex)
> > - rebase to 4.14.0-rc6.
> > 
> > v15:
> > - add a ioctl to get a dmabuf for a given dmabuf id. (Gerd)
> > 
> > v14:
> > - add PROBE, DMABUF and REGION flags. (Alex)
> > 
> > v12:
> > - add drm_format_mod back. (Gerd and Zhenyu)
> > - add region_index. (Gerd)
> > 
> > v11:
> > - rename plane_type to drm_plane_type. (Gerd)
> > - move fields of vfio_device_query_gfx_plane to vfio_device_gfx_plane_info.
> >   (Gerd)
> > - remove drm_format_mod, start fields. (Daniel)
> > - remove plane_id.
> > 
> > v10:
> > - refine the ABI API VFIO_DEVICE_QUERY_GFX_PLANE. (Alex) (Gerd)
> > 
> > v3:
> > - add a field gvt_plane_info in the drm_i915_gem_obj structure to save
> >   the decoded plane information to avoid look up while need the plane
> >   info. (Gerd)
> > 
> > Signed-off-by: Tina Zhang 
> > Cc: Gerd Hoffmann 
> > Cc: Alex Williamson 
> > Cc: Daniel Vetter 
> > ---
> >  include/uapi/linux/vfio.h | 62 
> > +++
> >  1 file changed, 62 insertions(+)
> > 
> > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > index ae46105..5c1cca2 100644
> > --- a/include/uapi/linux/vfio.h
> > +++ b/include/uapi/linux/vfio.h
> > @@ -502,6 +502,68 @@ struct vfio_pci_hot_reset {
> >  
> >  #define VFIO_DEVICE_PCI_HOT_RESET  _IO(VFIO_TYPE, VFIO_BASE + 13)
> >  
> > +/**
> > + * VFIO_DEVICE_QUERY_GFX_PLANE - _IOW(VFIO_TYPE, VFIO_BASE + 14,
> > + *struct vfio_device_query_gfx_plane)
> > + *
> > + * Set the drm_plane_type and flags, then retrieve the gfx plane info.
> > + *
> > + * flags supported:
> > + * - VFIO_GFX_PLANE_TYPE_PROBE and VFIO_GFX_PLANE_TYPE_DMABUF are set
> > + *   to ask if the mdev supports dma-buf. 0 on support, -EINVAL on no
> > + *   support for dma-buf.
> > + * - VFIO_GFX_PLANE_TYPE_PROBE and VFIO_GFX_PLANE_TYPE_REGION are set
> > + *   to ask if the mdev supports region. 0 on support, -EINVAL on no
> > + *   support for region.
> > + * - VFIO_GFX_PLANE_TYPE_DMABUF or VFIO_GFX_PLANE_TYPE_REGION is set
> > + *   with each call to query the plane info.
> > + * - Others are invalid and return -EINVAL.
> > + *
> > + * Note:
> > + * 1. Plane could be disabled by guest. In that case, success will be
> > + *returned with zero-initialized drm_format, size, width and height
> > + *fields.
> > + * 2. x_hot/y_hot is set to 0x if no hotspot information available
> > + *
> > + * Return: 0 on success, -errno on other failure.
> > + */
> > +struct vfio_device_gfx_plane_info {
> > +   __u32 argsz;
> > +   __u32 flags;
> > +#define VFIO_GFX_PLANE_TYPE_PROBE (1 << 0)
> > +#define VFIO_GFX_PLANE_TYPE_DMABUF (1 << 1)
> > +#define VFIO_GFX_PLANE_TYPE_REGION (1 << 2)
> > +   /* in */
> > +   __u32 drm_plane_type;   /* type of plane: DRM_PLANE_TYPE_* */
> > +   /* out */
> > +   __u32 drm_format;   /* drm format of plane */
> > +   __u64 drm_format_mod;   /* tiled mode */
> > +   __u32 width;/* width of plane */
> > +   __u32 height;   /* height of plane */
> > +   __u32 stride;   /* stride of plane */
> > +   __u32 size; /* size of plane in bytes, align on page*/
> > +   __u32 x_pos;/* horizontal position of cursor plane */
> > +   __u32 y_pos;/* vertical position of cursor plane*/
> > +   __u32 x_hot;/* horizontal position of cursor hotspot */
> > +   __u32 y_hot;/* vertical position of cursor hotspot */
> > +   union {

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

[Zhenyu Wang]

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.

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

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

[Zhenyu Wang]

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.

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

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

[Zhenyu Wang]

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!

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

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

[Zhenyu Wang]

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!

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [PATCH v10] vfio: ABI for mdev display dma-buf operation

[Zhenyu Wang]

On 2017.07.11 11:12:36 +0200, Daniel Vetter wrote:
> On Tue, Jul 11, 2017 at 08:14:08AM +0200, Gerd Hoffmann wrote:
> >   Hi,
> > 
> > > > +struct vfio_device_query_gfx_plane {
> > > > +   __u32 argsz;
> > > > +   __u32 flags;
> > > > +   struct vfio_device_gfx_plane_info plane_info;
> > > > +   __u32 plane_type;
> > > > +   __s32 fd; /* dma-buf fd */
> > > > +   __u32 plane_id;
> > > > +};
> > > > +
> > > 
> > > It would be better to have comment here about what are expected
> > > values
> > > for plane_type and plane_id.
> > 
> > plane_type is DRM_PLANE_TYPE_*.
> > 
> > yes, a comment saying so would be good, same for drm_format which is
> > DRM_FORMAT_*.  While looking at these two: renaming plane_type to
> > drm_plane_type (for consistency) is probably a good idea too.

For drm universal plane, this is not in drm uapi, but uabi. I think we
can align with drm plane definition for sure, but not need to pull in
drm header for that enum type.

> > 
> > plane_id needs a specification.
> 
> Why do you need plane_type? With universal planes the plane_id along is
> sufficient to identify a plane on a given drm device instance. I'd just
> remove it.

This interface is to get vGPU display plane info, there's no normal
drm kms client involved, but vGPU device model trys to expose guest
planes for display. We need to ask for what type of plane required on
target vGPU. I think plane_id here doesn't mean like in drm kms,
but I'm not sure about plane_id here without details, what's the
purpose, etc.

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [PATCH v10] vfio: ABI for mdev display dma-buf operation

[Zhenyu Wang]

On 2017.07.11 11:12:36 +0200, Daniel Vetter wrote:
> On Tue, Jul 11, 2017 at 08:14:08AM +0200, Gerd Hoffmann wrote:
> >   Hi,
> > 
> > > > +struct vfio_device_query_gfx_plane {
> > > > +   __u32 argsz;
> > > > +   __u32 flags;
> > > > +   struct vfio_device_gfx_plane_info plane_info;
> > > > +   __u32 plane_type;
> > > > +   __s32 fd; /* dma-buf fd */
> > > > +   __u32 plane_id;
> > > > +};
> > > > +
> > > 
> > > It would be better to have comment here about what are expected
> > > values
> > > for plane_type and plane_id.
> > 
> > plane_type is DRM_PLANE_TYPE_*.
> > 
> > yes, a comment saying so would be good, same for drm_format which is
> > DRM_FORMAT_*.  While looking at these two: renaming plane_type to
> > drm_plane_type (for consistency) is probably a good idea too.

For drm universal plane, this is not in drm uapi, but uabi. I think we
can align with drm plane definition for sure, but not need to pull in
drm header for that enum type.

> > 
> > plane_id needs a specification.
> 
> Why do you need plane_type? With universal planes the plane_id along is
> sufficient to identify a plane on a given drm device instance. I'd just
> remove it.

This interface is to get vGPU display plane info, there's no normal
drm kms client involved, but vGPU device model trys to expose guest
planes for display. We need to ask for what type of plane required on
target vGPU. I think plane_id here doesn't mean like in drm kms,
but I'm not sure about plane_id here without details, what's the
purpose, etc.

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [PATCH v8 3/6] drm/i915/gvt: Frame buffer decoder support for GVT-g

[Zhenyu Wang]

On 2017.06.09 14:50:39 +0800, Xiaoguang Chen wrote:
> decode frambuffer attributes of primary, cursor and sprite plane
> 
> Signed-off-by: Xiaoguang Chen 

...

> +/**
> + * intel_vgpu_decode_primary_plane - Decode primary plane
> + * @vgpu: input vgpu
> + * @plane: primary plane to save decoded info
> + * This function is called for decoding plane
> + *
> + * Returns:
> + * 0 on success, non-zero if failed.
> + */
> +int intel_vgpu_decode_primary_plane(struct intel_vgpu *vgpu,
> + struct intel_vgpu_primary_plane_format *plane)
> +{
> + u32 val, fmt;
> + struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv;
> + int pipe;
> +
> + pipe = get_active_pipe(vgpu);
> + if (pipe >= I915_MAX_PIPES)
> + return -ENODEV;
> +
> + val = vgpu_vreg(vgpu, DSPCNTR(pipe));
> + plane->enabled = !!(val & DISPLAY_PLANE_ENABLE);
> + if (!plane->enabled)
> + return -ENODEV;
> +
> + if (IS_SKYLAKE(dev_priv)) {
> + plane->tiled = (val & PLANE_CTL_TILED_MASK) >>
> + _PLANE_CTL_TILED_SHIFT;
> + fmt = skl_format_to_drm(
> + val & PLANE_CTL_FORMAT_MASK,
> + val & PLANE_CTL_ORDER_RGBX,
> + val & PLANE_CTL_ALPHA_MASK,
> + val & PLANE_CTL_YUV422_ORDER_MASK);
> + plane->bpp = skl_pixel_formats[fmt].bpp;
> + plane->drm_format = skl_pixel_formats[fmt].drm_format;
> + } else {
> + plane->tiled = !!(val & DISPPLANE_TILED);
> + fmt = (val & DISPPLANE_PIXFORMAT_MASK) >> _PRI_PLANE_FMT_SHIFT;
> + plane->bpp = bdw_pixel_formats[fmt].bpp;
> + plane->drm_format = bdw_pixel_formats[fmt].drm_format;
> + }
> +
> + if (!skl_pixel_formats[fmt].bpp && !bdw_pixel_formats[fmt].bpp) {
> + gvt_vgpu_err("Non-supported pixel format (0x%x)\n", fmt);
> + return -EINVAL;
> + }

Is this correct? shouldn't be plane->bpp as last time comment?

> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index 55e3010..400759f 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -161,6 +161,12 @@ extern "C" {
>  #define DRM_FORMAT_YUV444fourcc_code('Y', 'U', '2', '4') /* 
> non-subsampled Cb (1) and Cr (2) planes */
>  #define DRM_FORMAT_YVU444fourcc_code('Y', 'V', '2', '4') /* 
> non-subsampled Cr (1) and Cb (2) planes */
>  
> +/*
> + * Intel GVT-g plane format definition
> + */
> +#define DRM_FORMAT_XRGB161616_GVT  fourcc_code('X', 'R', '4', '8') /* [63:0] 
> x:R:G:B 16:16:16:16 little endian */
> +#define DRM_FORMAT_XBGR161616_GVT  fourcc_code('X', 'B', '4', '8') /* [63:0] 
> x:B:G:R 16:16:16:16 little endian */
> +
>  

This should be a seperate patch and not need GVT postfix for format definition.

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [PATCH v8 3/6] drm/i915/gvt: Frame buffer decoder support for GVT-g

[Zhenyu Wang]

On 2017.06.09 14:50:39 +0800, Xiaoguang Chen wrote:
> decode frambuffer attributes of primary, cursor and sprite plane
> 
> Signed-off-by: Xiaoguang Chen 

...

> +/**
> + * intel_vgpu_decode_primary_plane - Decode primary plane
> + * @vgpu: input vgpu
> + * @plane: primary plane to save decoded info
> + * This function is called for decoding plane
> + *
> + * Returns:
> + * 0 on success, non-zero if failed.
> + */
> +int intel_vgpu_decode_primary_plane(struct intel_vgpu *vgpu,
> + struct intel_vgpu_primary_plane_format *plane)
> +{
> + u32 val, fmt;
> + struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv;
> + int pipe;
> +
> + pipe = get_active_pipe(vgpu);
> + if (pipe >= I915_MAX_PIPES)
> + return -ENODEV;
> +
> + val = vgpu_vreg(vgpu, DSPCNTR(pipe));
> + plane->enabled = !!(val & DISPLAY_PLANE_ENABLE);
> + if (!plane->enabled)
> + return -ENODEV;
> +
> + if (IS_SKYLAKE(dev_priv)) {
> + plane->tiled = (val & PLANE_CTL_TILED_MASK) >>
> + _PLANE_CTL_TILED_SHIFT;
> + fmt = skl_format_to_drm(
> + val & PLANE_CTL_FORMAT_MASK,
> + val & PLANE_CTL_ORDER_RGBX,
> + val & PLANE_CTL_ALPHA_MASK,
> + val & PLANE_CTL_YUV422_ORDER_MASK);
> + plane->bpp = skl_pixel_formats[fmt].bpp;
> + plane->drm_format = skl_pixel_formats[fmt].drm_format;
> + } else {
> + plane->tiled = !!(val & DISPPLANE_TILED);
> + fmt = (val & DISPPLANE_PIXFORMAT_MASK) >> _PRI_PLANE_FMT_SHIFT;
> + plane->bpp = bdw_pixel_formats[fmt].bpp;
> + plane->drm_format = bdw_pixel_formats[fmt].drm_format;
> + }
> +
> + if (!skl_pixel_formats[fmt].bpp && !bdw_pixel_formats[fmt].bpp) {
> + gvt_vgpu_err("Non-supported pixel format (0x%x)\n", fmt);
> + return -EINVAL;
> + }

Is this correct? shouldn't be plane->bpp as last time comment?

> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index 55e3010..400759f 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -161,6 +161,12 @@ extern "C" {
>  #define DRM_FORMAT_YUV444fourcc_code('Y', 'U', '2', '4') /* 
> non-subsampled Cb (1) and Cr (2) planes */
>  #define DRM_FORMAT_YVU444fourcc_code('Y', 'V', '2', '4') /* 
> non-subsampled Cr (1) and Cb (2) planes */
>  
> +/*
> + * Intel GVT-g plane format definition
> + */
> +#define DRM_FORMAT_XRGB161616_GVT  fourcc_code('X', 'R', '4', '8') /* [63:0] 
> x:R:G:B 16:16:16:16 little endian */
> +#define DRM_FORMAT_XBGR161616_GVT  fourcc_code('X', 'B', '4', '8') /* [63:0] 
> x:B:G:R 16:16:16:16 little endian */
> +
>  

This should be a seperate patch and not need GVT postfix for format definition.

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [PATCH] drm/i915/gvt: remove redundant -Wall

[Zhenyu Wang]

On 2017.05.21 00:15:27 -0700, Nick Desaulniers wrote:
> This flag is already set in the top level Makefile of the kernel.
> 
> Also, by having set CONFIG_DRM_I915_GVT, thereby appending -Wall to
> ccflags, you undo all the -Wno-* cflags previously set in the Make
> variable KBUILD_CFLAGS.
> 
> For example:
> 
> cc foo.c -Wall -Wno-format -Wall
> 
> resets -Wformat.
> 
> Signed-off-by: Nick Desaulniers 
> ---
> I verified that -Wall was redundant by compiling with V=1:
> make V=1 -j4 drivers/gpu/drm/i915/i915_gem_request.o
> 
>  drivers/gpu/drm/i915/gvt/Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/Makefile 
> b/drivers/gpu/drm/i915/gvt/Makefile
> index b123c20e2097..f5486cb94818 100644
> --- a/drivers/gpu/drm/i915/gvt/Makefile
> +++ b/drivers/gpu/drm/i915/gvt/Makefile
> @@ -3,6 +3,6 @@ GVT_SOURCE := gvt.o aperture_gm.o handlers.o vgpu.o 
> trace_points.o firmware.o \
>   interrupt.o gtt.o cfg_space.o opregion.o mmio.o display.o edid.o \
>   execlist.o scheduler.o sched_policy.o render.o cmd_parser.o
>  
> -ccflags-y+= -I$(src) -I$(src)/$(GVT_DIR) -Wall
> +ccflags-y+= -I$(src) -I$(src)/$(GVT_DIR)
>  i915-y   += $(addprefix $(GVT_DIR)/, 
> $(GVT_SOURCE))
>  obj-$(CONFIG_DRM_I915_GVT_KVMGT) += $(GVT_DIR)/kvmgt.o
> -- 

Pushed, thanks!

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [PATCH] drm/i915/gvt: remove redundant -Wall

[Zhenyu Wang]

On 2017.05.21 00:15:27 -0700, Nick Desaulniers wrote:
> This flag is already set in the top level Makefile of the kernel.
> 
> Also, by having set CONFIG_DRM_I915_GVT, thereby appending -Wall to
> ccflags, you undo all the -Wno-* cflags previously set in the Make
> variable KBUILD_CFLAGS.
> 
> For example:
> 
> cc foo.c -Wall -Wno-format -Wall
> 
> resets -Wformat.
> 
> Signed-off-by: Nick Desaulniers 
> ---
> I verified that -Wall was redundant by compiling with V=1:
> make V=1 -j4 drivers/gpu/drm/i915/i915_gem_request.o
> 
>  drivers/gpu/drm/i915/gvt/Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/Makefile 
> b/drivers/gpu/drm/i915/gvt/Makefile
> index b123c20e2097..f5486cb94818 100644
> --- a/drivers/gpu/drm/i915/gvt/Makefile
> +++ b/drivers/gpu/drm/i915/gvt/Makefile
> @@ -3,6 +3,6 @@ GVT_SOURCE := gvt.o aperture_gm.o handlers.o vgpu.o 
> trace_points.o firmware.o \
>   interrupt.o gtt.o cfg_space.o opregion.o mmio.o display.o edid.o \
>   execlist.o scheduler.o sched_policy.o render.o cmd_parser.o
>  
> -ccflags-y+= -I$(src) -I$(src)/$(GVT_DIR) -Wall
> +ccflags-y+= -I$(src) -I$(src)/$(GVT_DIR)
>  i915-y   += $(addprefix $(GVT_DIR)/, 
> $(GVT_SOURCE))
>  obj-$(CONFIG_DRM_I915_GVT_KVMGT) += $(GVT_DIR)/kvmgt.o
> -- 

Pushed, thanks!

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [PATCH v6 2/6] drm/i915/gvt: OpRegion support for GVT-g

[Zhenyu Wang]

On 2017.05.31 06:22:28 +, Chen, Xiaoguang wrote:
> >> @@ -467,6 +555,15 @@ static int intel_vgpu_create(struct kobject *kobj,
> >struct mdev_device *mdev)
> >>vgpu->vdev.mdev = mdev;
> >>mdev_set_drvdata(mdev, vgpu);
> >>
> >> +  ret = intel_vgpu_reg_init_opregion(vgpu);
> >> +  if (ret) {
> >> +  gvt_vgpu_err("create OpRegion failed\n");
> >> +  goto out;
> >> +  }
> >
> >Still need to handle error path for created vgpu.
> Just checked the code, if initialize the opregion failed we should first 
> release vfio/mdev releated work(maybe call intel_vgpu_release function)  and 
> then destroy the vgpu. Will update in the next version.
> 

Better to init opregion inside of create vgpu and do proper error
handling there too.

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [PATCH v6 2/6] drm/i915/gvt: OpRegion support for GVT-g

[Zhenyu Wang]

On 2017.05.31 06:22:28 +, Chen, Xiaoguang wrote:
> >> @@ -467,6 +555,15 @@ static int intel_vgpu_create(struct kobject *kobj,
> >struct mdev_device *mdev)
> >>vgpu->vdev.mdev = mdev;
> >>mdev_set_drvdata(mdev, vgpu);
> >>
> >> +  ret = intel_vgpu_reg_init_opregion(vgpu);
> >> +  if (ret) {
> >> +  gvt_vgpu_err("create OpRegion failed\n");
> >> +  goto out;
> >> +  }
> >
> >Still need to handle error path for created vgpu.
> Just checked the code, if initialize the opregion failed we should first 
> release vfio/mdev releated work(maybe call intel_vgpu_release function)  and 
> then destroy the vgpu. Will update in the next version.
> 

Better to init opregion inside of create vgpu and do proper error
handling there too.

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [PATCH v6 3/6] drm/i915/gvt: Frame buffer decoder support for GVT-g

[Zhenyu Wang]

On 2017.05.27 16:38:49 +0800, Xiaoguang Chen wrote:
> decode frambuffer attributes of primary, cursor and sprite plane
> 
> Signed-off-by: Xiaoguang Chen 
> ---
>  drivers/gpu/drm/i915/gvt/Makefile |   3 +-
>  drivers/gpu/drm/i915/gvt/display.c|   2 +-
>  drivers/gpu/drm/i915/gvt/display.h|   2 +
>  drivers/gpu/drm/i915/gvt/fb_decoder.c | 479 
> ++
>  drivers/gpu/drm/i915/gvt/fb_decoder.h | 166 
>  drivers/gpu/drm/i915/gvt/gvt.h|   1 +
>  6 files changed, 651 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/gvt/fb_decoder.c
>  create mode 100644 drivers/gpu/drm/i915/gvt/fb_decoder.h
> 
> diff --git a/drivers/gpu/drm/i915/gvt/Makefile 
> b/drivers/gpu/drm/i915/gvt/Makefile
> index b123c20..192ca26 100644
> --- a/drivers/gpu/drm/i915/gvt/Makefile
> +++ b/drivers/gpu/drm/i915/gvt/Makefile
> @@ -1,7 +1,8 @@
>  GVT_DIR := gvt
>  GVT_SOURCE := gvt.o aperture_gm.o handlers.o vgpu.o trace_points.o 
> firmware.o \
>   interrupt.o gtt.o cfg_space.o opregion.o mmio.o display.o edid.o \
> - execlist.o scheduler.o sched_policy.o render.o cmd_parser.o
> + execlist.o scheduler.o sched_policy.o render.o cmd_parser.o \
> + fb_decoder.o
>  
>  ccflags-y+= -I$(src) -I$(src)/$(GVT_DIR) -Wall
>  i915-y   += $(addprefix $(GVT_DIR)/, 
> $(GVT_SOURCE))
> diff --git a/drivers/gpu/drm/i915/gvt/display.c 
> b/drivers/gpu/drm/i915/gvt/display.c
> index e0261fc..f5f63c5 100644
> --- a/drivers/gpu/drm/i915/gvt/display.c
> +++ b/drivers/gpu/drm/i915/gvt/display.c
> @@ -67,7 +67,7 @@ static int edp_pipe_is_enabled(struct intel_vgpu *vgpu)
>   return 1;
>  }
>  
> -static int pipe_is_enabled(struct intel_vgpu *vgpu, int pipe)
> +int pipe_is_enabled(struct intel_vgpu *vgpu, int pipe)
>  {
>   struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv;
>  
> diff --git a/drivers/gpu/drm/i915/gvt/display.h 
> b/drivers/gpu/drm/i915/gvt/display.h
> index d73de22..b46b868 100644
> --- a/drivers/gpu/drm/i915/gvt/display.h
> +++ b/drivers/gpu/drm/i915/gvt/display.h
> @@ -179,4 +179,6 @@ int intel_vgpu_init_display(struct intel_vgpu *vgpu, u64 
> resolution);
>  void intel_vgpu_reset_display(struct intel_vgpu *vgpu);
>  void intel_vgpu_clean_display(struct intel_vgpu *vgpu);
>  
> +int pipe_is_enabled(struct intel_vgpu *vgpu, int pipe);
> +
>  #endif
> diff --git a/drivers/gpu/drm/i915/gvt/fb_decoder.c 
> b/drivers/gpu/drm/i915/gvt/fb_decoder.c
> new file mode 100644
> index 000..d4404fd
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gvt/fb_decoder.c
> @@ -0,0 +1,479 @@
> +/*
> + * Copyright(c) 2011-2016 Intel Corporation. All rights reserved.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
> FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN 
> THE
> + * SOFTWARE.
> + *
> + * Authors:
> + *Kevin Tian 
> + *
> + * Contributors:
> + *Bing Niu 
> + *Xu Han 
> + *Ping Gao 
> + *Xiaoguang Chen 
> + *Yang Liu 
> + *
> + */
> +
> +#include 
> +#include "i915_drv.h"
> +#include "gvt.h"
> +
> +/* The below definitions are required by guest. */
> +// [63:0] x:R:G:B 16:16:16:16 little endian
> +#define DRM_FORMAT_XRGB161616_GVT  fourcc_code('X', 'R', '4', '8')
> +// [63:0] x:B:G:R 16:16:16:16 little endian
> +#define DRM_FORMAT_XBGR161616_GVT  fourcc_code('X', 'B', '4', '8')
> +

Should be added to drm_fourcc?

> +#define FORMAT_NUM   16

PRIMARY_FORMAT_NUM

> +struct pixel_format {
> + int drm_format; /* Pixel format in DRM definition */
> + int bpp;/* Bits per pixel, 0 indicates invalid */
> + char *desc; /* The description */
> +};
> +
> +/* non-supported format has bpp default to 0 */
> +static struct pixel_format 

Re: [PATCH v6 3/6] drm/i915/gvt: Frame buffer decoder support for GVT-g

[Zhenyu Wang]

On 2017.05.27 16:38:49 +0800, Xiaoguang Chen wrote:
> decode frambuffer attributes of primary, cursor and sprite plane
> 
> Signed-off-by: Xiaoguang Chen 
> ---
>  drivers/gpu/drm/i915/gvt/Makefile |   3 +-
>  drivers/gpu/drm/i915/gvt/display.c|   2 +-
>  drivers/gpu/drm/i915/gvt/display.h|   2 +
>  drivers/gpu/drm/i915/gvt/fb_decoder.c | 479 
> ++
>  drivers/gpu/drm/i915/gvt/fb_decoder.h | 166 
>  drivers/gpu/drm/i915/gvt/gvt.h|   1 +
>  6 files changed, 651 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/gvt/fb_decoder.c
>  create mode 100644 drivers/gpu/drm/i915/gvt/fb_decoder.h
> 
> diff --git a/drivers/gpu/drm/i915/gvt/Makefile 
> b/drivers/gpu/drm/i915/gvt/Makefile
> index b123c20..192ca26 100644
> --- a/drivers/gpu/drm/i915/gvt/Makefile
> +++ b/drivers/gpu/drm/i915/gvt/Makefile
> @@ -1,7 +1,8 @@
>  GVT_DIR := gvt
>  GVT_SOURCE := gvt.o aperture_gm.o handlers.o vgpu.o trace_points.o 
> firmware.o \
>   interrupt.o gtt.o cfg_space.o opregion.o mmio.o display.o edid.o \
> - execlist.o scheduler.o sched_policy.o render.o cmd_parser.o
> + execlist.o scheduler.o sched_policy.o render.o cmd_parser.o \
> + fb_decoder.o
>  
>  ccflags-y+= -I$(src) -I$(src)/$(GVT_DIR) -Wall
>  i915-y   += $(addprefix $(GVT_DIR)/, 
> $(GVT_SOURCE))
> diff --git a/drivers/gpu/drm/i915/gvt/display.c 
> b/drivers/gpu/drm/i915/gvt/display.c
> index e0261fc..f5f63c5 100644
> --- a/drivers/gpu/drm/i915/gvt/display.c
> +++ b/drivers/gpu/drm/i915/gvt/display.c
> @@ -67,7 +67,7 @@ static int edp_pipe_is_enabled(struct intel_vgpu *vgpu)
>   return 1;
>  }
>  
> -static int pipe_is_enabled(struct intel_vgpu *vgpu, int pipe)
> +int pipe_is_enabled(struct intel_vgpu *vgpu, int pipe)
>  {
>   struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv;
>  
> diff --git a/drivers/gpu/drm/i915/gvt/display.h 
> b/drivers/gpu/drm/i915/gvt/display.h
> index d73de22..b46b868 100644
> --- a/drivers/gpu/drm/i915/gvt/display.h
> +++ b/drivers/gpu/drm/i915/gvt/display.h
> @@ -179,4 +179,6 @@ int intel_vgpu_init_display(struct intel_vgpu *vgpu, u64 
> resolution);
>  void intel_vgpu_reset_display(struct intel_vgpu *vgpu);
>  void intel_vgpu_clean_display(struct intel_vgpu *vgpu);
>  
> +int pipe_is_enabled(struct intel_vgpu *vgpu, int pipe);
> +
>  #endif
> diff --git a/drivers/gpu/drm/i915/gvt/fb_decoder.c 
> b/drivers/gpu/drm/i915/gvt/fb_decoder.c
> new file mode 100644
> index 000..d4404fd
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gvt/fb_decoder.c
> @@ -0,0 +1,479 @@
> +/*
> + * Copyright(c) 2011-2016 Intel Corporation. All rights reserved.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
> FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN 
> THE
> + * SOFTWARE.
> + *
> + * Authors:
> + *Kevin Tian 
> + *
> + * Contributors:
> + *Bing Niu 
> + *Xu Han 
> + *Ping Gao 
> + *Xiaoguang Chen 
> + *Yang Liu 
> + *
> + */
> +
> +#include 
> +#include "i915_drv.h"
> +#include "gvt.h"
> +
> +/* The below definitions are required by guest. */
> +// [63:0] x:R:G:B 16:16:16:16 little endian
> +#define DRM_FORMAT_XRGB161616_GVT  fourcc_code('X', 'R', '4', '8')
> +// [63:0] x:B:G:R 16:16:16:16 little endian
> +#define DRM_FORMAT_XBGR161616_GVT  fourcc_code('X', 'B', '4', '8')
> +

Should be added to drm_fourcc?

> +#define FORMAT_NUM   16

PRIMARY_FORMAT_NUM

> +struct pixel_format {
> + int drm_format; /* Pixel format in DRM definition */
> + int bpp;/* Bits per pixel, 0 indicates invalid */
> + char *desc; /* The description */
> +};
> +
> +/* non-supported format has bpp default to 0 */
> +static struct pixel_format primary_pixel_formats[FORMAT_NUM] = {
> + [0x2] = {DRM_FORMAT_C8, 8, "8-bit Indexed"},
> + [0x5] = {DRM_FORMAT_RGB565, 16, "16-bit BGRX (5:6:5 MSB-R:G:B)"},
> +  

Re: [PATCH v6 2/6] drm/i915/gvt: OpRegion support for GVT-g

[Zhenyu Wang]

On 2017.05.27 16:38:48 +0800, Xiaoguang Chen wrote:
> OpRegion is needed to support display related operation for
> intel vgpu.
> 
> A vfio device region is added to intel vgpu to deliver the
> host OpRegion information to user space so user space can
> construct the OpRegion for vgpu.
> 
> Signed-off-by: Bing Niu 
> Signed-off-by: Xiaoguang Chen 
> ---
>  drivers/gpu/drm/i915/gvt/kvmgt.c| 97 
> +
>  drivers/gpu/drm/i915/gvt/opregion.c |  8 ++-
>  2 files changed, 104 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c 
> b/drivers/gpu/drm/i915/gvt/kvmgt.c
> index 3c6a02b..389f072 100644
> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> @@ -53,6 +53,8 @@ static const struct intel_gvt_ops *intel_gvt_ops;
>  #define VFIO_PCI_INDEX_TO_OFFSET(index) ((u64)(index) << 
> VFIO_PCI_OFFSET_SHIFT)
>  #define VFIO_PCI_OFFSET_MASK(((u64)(1) << VFIO_PCI_OFFSET_SHIFT) - 1)
>  
> +#define OPREGION_SIGNATURE "IntelGraphicsMem"
> +
>  struct vfio_region;
>  struct intel_vgpu_regops {
>   size_t (*rw)(struct intel_vgpu *vgpu, char *buf,
> @@ -436,6 +438,92 @@ static void kvmgt_protect_table_del(struct 
> kvmgt_guest_info *info,
>   }
>  }
>  
> +static size_t intel_vgpu_reg_rw_opregion(struct intel_vgpu *vgpu, char *buf,
> + size_t count, loff_t *ppos, bool iswrite)
> +{
> + unsigned int i = VFIO_PCI_OFFSET_TO_INDEX(*ppos) -
> + VFIO_PCI_NUM_REGIONS;
> + void *base = vgpu->vdev.region[i].data;
> + loff_t pos = *ppos & VFIO_PCI_OFFSET_MASK;
> +
> + if (pos >= vgpu->vdev.region[i].size || iswrite) {
> + gvt_vgpu_err("invalid op or offset for Intel vgpu OpRegion\n");
> + return -EINVAL;
> + }
> + count = min(count, (size_t)(vgpu->vdev.region[i].size - pos));
> + memcpy(buf, base + pos, count);
> +
> + return count;
> +}
> +
> +static void intel_vgpu_reg_release_opregion(struct intel_vgpu *vgpu,
> + struct vfio_region *region)
> +{
> + memunmap(region->data);
> +}
> +
> +static const struct intel_vgpu_regops intel_vgpu_regops_opregion = {
> + .rw = intel_vgpu_reg_rw_opregion,
> + .release = intel_vgpu_reg_release_opregion,
> +};
> +
> +static int intel_vgpu_register_reg(struct intel_vgpu *vgpu,
> + unsigned int type, unsigned int subtype,
> + const struct intel_vgpu_regops *ops,
> + size_t size, u32 flags, void *data)
> +{
> + struct vfio_region *region;
> +
> + region = krealloc(vgpu->vdev.region,
> + (vgpu->vdev.num_regions + 1) * sizeof(*region),
> + GFP_KERNEL);
> + if (!region)
> + return -ENOMEM;
> +
> + vgpu->vdev.region = region;
> + vgpu->vdev.region[vgpu->vdev.num_regions].type = type;
> + vgpu->vdev.region[vgpu->vdev.num_regions].subtype = subtype;
> + vgpu->vdev.region[vgpu->vdev.num_regions].ops = ops;
> + vgpu->vdev.region[vgpu->vdev.num_regions].size = size;
> + vgpu->vdev.region[vgpu->vdev.num_regions].flags = flags;
> + vgpu->vdev.region[vgpu->vdev.num_regions].data = data;
> + vgpu->vdev.num_regions++;
> +
> + return 0;
> +}
> +
> +static int intel_vgpu_reg_init_opregion(struct intel_vgpu *vgpu)
> +{
> + unsigned int addr;
> + void *base;
> + int ret;
> +
> + addr = vgpu->gvt->opregion.opregion_pa;
> + if (!addr || !(~addr))
> + return -ENODEV;
> +
> + base = memremap(addr, OPREGION_SIZE, MEMREMAP_WB);
> + if (!base)
> + return -ENOMEM;
> +
> + if (memcmp(base, OPREGION_SIGNATURE, 16)) {
> + memunmap(base);
> + return -EINVAL;
> + }
> +
> + ret = intel_vgpu_register_reg(vgpu,
> + PCI_VENDOR_ID_INTEL | VFIO_REGION_TYPE_PCI_VENDOR_TYPE,
> + VFIO_REGION_SUBTYPE_INTEL_IGD_OPREGION,
> + _vgpu_regops_opregion, OPREGION_SIZE,
> + VFIO_REGION_INFO_FLAG_READ, base);
> + if (ret) {
> + memunmap(base);
> + return ret;
> + }
> +
> + return ret;
> +}
> +
>  static int intel_vgpu_create(struct kobject *kobj, struct mdev_device *mdev)
>  {
>   struct intel_vgpu *vgpu = NULL;
> @@ -467,6 +555,15 @@ static int intel_vgpu_create(struct kobject *kobj, 
> struct mdev_device *mdev)
>   vgpu->vdev.mdev = mdev;
>   mdev_set_drvdata(mdev, vgpu);
>  
> + ret = intel_vgpu_reg_init_opregion(vgpu);
> + if (ret) {
> + gvt_vgpu_err("create OpRegion failed\n");
> + goto out;
> + }

Still need to handle error path for created vgpu.

> +
> + gvt_dbg_core("create OpRegion succeeded for mdev:%s\n",
> + dev_name(mdev_dev(mdev)));
> +
>   gvt_dbg_core("intel_vgpu_create succeeded for mdev: %s\n",
>dev_name(mdev_dev(mdev)));
>   ret = 0;
> diff 

Re: [PATCH v6 2/6] drm/i915/gvt: OpRegion support for GVT-g

[Zhenyu Wang]

On 2017.05.27 16:38:48 +0800, Xiaoguang Chen wrote:
> OpRegion is needed to support display related operation for
> intel vgpu.
> 
> A vfio device region is added to intel vgpu to deliver the
> host OpRegion information to user space so user space can
> construct the OpRegion for vgpu.
> 
> Signed-off-by: Bing Niu 
> Signed-off-by: Xiaoguang Chen 
> ---
>  drivers/gpu/drm/i915/gvt/kvmgt.c| 97 
> +
>  drivers/gpu/drm/i915/gvt/opregion.c |  8 ++-
>  2 files changed, 104 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c 
> b/drivers/gpu/drm/i915/gvt/kvmgt.c
> index 3c6a02b..389f072 100644
> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> @@ -53,6 +53,8 @@ static const struct intel_gvt_ops *intel_gvt_ops;
>  #define VFIO_PCI_INDEX_TO_OFFSET(index) ((u64)(index) << 
> VFIO_PCI_OFFSET_SHIFT)
>  #define VFIO_PCI_OFFSET_MASK(((u64)(1) << VFIO_PCI_OFFSET_SHIFT) - 1)
>  
> +#define OPREGION_SIGNATURE "IntelGraphicsMem"
> +
>  struct vfio_region;
>  struct intel_vgpu_regops {
>   size_t (*rw)(struct intel_vgpu *vgpu, char *buf,
> @@ -436,6 +438,92 @@ static void kvmgt_protect_table_del(struct 
> kvmgt_guest_info *info,
>   }
>  }
>  
> +static size_t intel_vgpu_reg_rw_opregion(struct intel_vgpu *vgpu, char *buf,
> + size_t count, loff_t *ppos, bool iswrite)
> +{
> + unsigned int i = VFIO_PCI_OFFSET_TO_INDEX(*ppos) -
> + VFIO_PCI_NUM_REGIONS;
> + void *base = vgpu->vdev.region[i].data;
> + loff_t pos = *ppos & VFIO_PCI_OFFSET_MASK;
> +
> + if (pos >= vgpu->vdev.region[i].size || iswrite) {
> + gvt_vgpu_err("invalid op or offset for Intel vgpu OpRegion\n");
> + return -EINVAL;
> + }
> + count = min(count, (size_t)(vgpu->vdev.region[i].size - pos));
> + memcpy(buf, base + pos, count);
> +
> + return count;
> +}
> +
> +static void intel_vgpu_reg_release_opregion(struct intel_vgpu *vgpu,
> + struct vfio_region *region)
> +{
> + memunmap(region->data);
> +}
> +
> +static const struct intel_vgpu_regops intel_vgpu_regops_opregion = {
> + .rw = intel_vgpu_reg_rw_opregion,
> + .release = intel_vgpu_reg_release_opregion,
> +};
> +
> +static int intel_vgpu_register_reg(struct intel_vgpu *vgpu,
> + unsigned int type, unsigned int subtype,
> + const struct intel_vgpu_regops *ops,
> + size_t size, u32 flags, void *data)
> +{
> + struct vfio_region *region;
> +
> + region = krealloc(vgpu->vdev.region,
> + (vgpu->vdev.num_regions + 1) * sizeof(*region),
> + GFP_KERNEL);
> + if (!region)
> + return -ENOMEM;
> +
> + vgpu->vdev.region = region;
> + vgpu->vdev.region[vgpu->vdev.num_regions].type = type;
> + vgpu->vdev.region[vgpu->vdev.num_regions].subtype = subtype;
> + vgpu->vdev.region[vgpu->vdev.num_regions].ops = ops;
> + vgpu->vdev.region[vgpu->vdev.num_regions].size = size;
> + vgpu->vdev.region[vgpu->vdev.num_regions].flags = flags;
> + vgpu->vdev.region[vgpu->vdev.num_regions].data = data;
> + vgpu->vdev.num_regions++;
> +
> + return 0;
> +}
> +
> +static int intel_vgpu_reg_init_opregion(struct intel_vgpu *vgpu)
> +{
> + unsigned int addr;
> + void *base;
> + int ret;
> +
> + addr = vgpu->gvt->opregion.opregion_pa;
> + if (!addr || !(~addr))
> + return -ENODEV;
> +
> + base = memremap(addr, OPREGION_SIZE, MEMREMAP_WB);
> + if (!base)
> + return -ENOMEM;
> +
> + if (memcmp(base, OPREGION_SIGNATURE, 16)) {
> + memunmap(base);
> + return -EINVAL;
> + }
> +
> + ret = intel_vgpu_register_reg(vgpu,
> + PCI_VENDOR_ID_INTEL | VFIO_REGION_TYPE_PCI_VENDOR_TYPE,
> + VFIO_REGION_SUBTYPE_INTEL_IGD_OPREGION,
> + _vgpu_regops_opregion, OPREGION_SIZE,
> + VFIO_REGION_INFO_FLAG_READ, base);
> + if (ret) {
> + memunmap(base);
> + return ret;
> + }
> +
> + return ret;
> +}
> +
>  static int intel_vgpu_create(struct kobject *kobj, struct mdev_device *mdev)
>  {
>   struct intel_vgpu *vgpu = NULL;
> @@ -467,6 +555,15 @@ static int intel_vgpu_create(struct kobject *kobj, 
> struct mdev_device *mdev)
>   vgpu->vdev.mdev = mdev;
>   mdev_set_drvdata(mdev, vgpu);
>  
> + ret = intel_vgpu_reg_init_opregion(vgpu);
> + if (ret) {
> + gvt_vgpu_err("create OpRegion failed\n");
> + goto out;
> + }

Still need to handle error path for created vgpu.

> +
> + gvt_dbg_core("create OpRegion succeeded for mdev:%s\n",
> + dev_name(mdev_dev(mdev)));
> +
>   gvt_dbg_core("intel_vgpu_create succeeded for mdev: %s\n",
>dev_name(mdev_dev(mdev)));
>   ret = 0;
> diff --git a/drivers/gpu/drm/i915/gvt/opregion.c 
> 

Re: [PATCH] drm/i915/gvt: fix typo: "supporte" -> "support"

[Zhenyu Wang]

On 2017.04.25 10:05:12 +0100, Colin King wrote:
> From: Colin Ian King 
> 
> trivial fix to typo in WARN_ONCE message
> 
> Signed-off-by: Colin Ian King 
> ---
>  drivers/gpu/drm/i915/gvt/handlers.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/handlers.c 
> b/drivers/gpu/drm/i915/gvt/handlers.c
> index 0ad1a508e2af..c995e540ff96 100644
> --- a/drivers/gpu/drm/i915/gvt/handlers.c
> +++ b/drivers/gpu/drm/i915/gvt/handlers.c
> @@ -1244,7 +1244,7 @@ static int dma_ctrl_write(struct intel_vgpu *vgpu, 
> unsigned int offset,
>   mode = vgpu_vreg(vgpu, offset);
>  
>   if (GFX_MODE_BIT_SET_IN_MASK(mode, START_DMA)) {
> - WARN_ONCE(1, "VM(%d): iGVT-g doesn't supporte GuC\n",
> + WARN_ONCE(1, "VM(%d): iGVT-g doesn't support GuC\n",
>   vgpu->id);
>   return 0;
>   }
> -- 

applied, thanks!

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [PATCH] drm/i915/gvt: fix typo: "supporte" -> "support"

[Zhenyu Wang]

On 2017.04.25 10:05:12 +0100, Colin King wrote:
> From: Colin Ian King 
> 
> trivial fix to typo in WARN_ONCE message
> 
> Signed-off-by: Colin Ian King 
> ---
>  drivers/gpu/drm/i915/gvt/handlers.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/handlers.c 
> b/drivers/gpu/drm/i915/gvt/handlers.c
> index 0ad1a508e2af..c995e540ff96 100644
> --- a/drivers/gpu/drm/i915/gvt/handlers.c
> +++ b/drivers/gpu/drm/i915/gvt/handlers.c
> @@ -1244,7 +1244,7 @@ static int dma_ctrl_write(struct intel_vgpu *vgpu, 
> unsigned int offset,
>   mode = vgpu_vreg(vgpu, offset);
>  
>   if (GFX_MODE_BIT_SET_IN_MASK(mode, START_DMA)) {
> - WARN_ONCE(1, "VM(%d): iGVT-g doesn't supporte GuC\n",
> + WARN_ONCE(1, "VM(%d): iGVT-g doesn't support GuC\n",
>   vgpu->id);
>   return 0;
>   }
> -- 

applied, thanks!

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [PATCH] drm/i915: disable KASAN for handlers

[Zhenyu Wang]

On 2017.03.30 11:46:27 +0200, Jiri Slaby wrote:
> Handlers are currently the only blocker to compile the kernel with gcc 7
> and KASAN+use-after-scope enabled:
> drivers/gpu/drm/i915/gvt/handlers.c:2200:1: error: the frame size of 43760 
> bytes is larger than 2048 bytes [-Werror=frame-larger-than=]
> drivers/gpu/drm/i915/gvt/handlers.c:2402:1: error: the frame size of 9400 
> bytes is larger than 2048 bytes [-Werror=frame-larger-than=]
> drivers/gpu/drm/i915/gvt/handlers.c:2628:1: error: the frame size of 11256 
> bytes is larger than 2048 bytes [-Werror=frame-larger-than=]
> 
> It is due to many expansions of MMIO_* macros in init_generic_mmio_info.
> INTEL_GVT_MMIO_OFFSET generates for each such line a __reg and an
> offset. There are too many for KASAN to keep up.
> 
> So disable KASAN for this file.
> 
> Signed-off-by: Jiri Slaby <jsl...@suse.cz>
> Cc: Martin Liska <mli...@suse.cz>
> Cc: Zhenyu Wang <zhen...@linux.intel.com>
> Cc: Zhi Wang <zhi.a.w...@intel.com>
> Cc: Daniel Vetter <daniel.vet...@intel.com>
> Cc: Jani Nikula <jani.nik...@linux.intel.com>
> Cc: David Airlie <airl...@linux.ie>
> Cc: intel-gvt-...@lists.freedesktop.org
> Cc: intel-...@lists.freedesktop.org
> Cc: dri-de...@lists.freedesktop.org
> ---
>  drivers/gpu/drm/i915/gvt/Makefile | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/Makefile 
> b/drivers/gpu/drm/i915/gvt/Makefile
> index b123c20e2097..942f1849d194 100644
> --- a/drivers/gpu/drm/i915/gvt/Makefile
> +++ b/drivers/gpu/drm/i915/gvt/Makefile
> @@ -6,3 +6,5 @@ GVT_SOURCE := gvt.o aperture_gm.o handlers.o vgpu.o 
> trace_points.o firmware.o \
>  ccflags-y+= -I$(src) -I$(src)/$(GVT_DIR) -Wall
>  i915-y   += $(addprefix $(GVT_DIR)/, 
> $(GVT_SOURCE))
>  obj-$(CONFIG_DRM_I915_GVT_KVMGT) += $(GVT_DIR)/kvmgt.o
> +
> +KASAN_SANITIZE_handlers.o := n
> -- 
> 2.12.2
> 

Applied this, we'd better cleanup legacy usage to current i915 mmio reg define. 
Thanks!

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [PATCH] drm/i915: disable KASAN for handlers

[Zhenyu Wang]

On 2017.03.30 11:46:27 +0200, Jiri Slaby wrote:
> Handlers are currently the only blocker to compile the kernel with gcc 7
> and KASAN+use-after-scope enabled:
> drivers/gpu/drm/i915/gvt/handlers.c:2200:1: error: the frame size of 43760 
> bytes is larger than 2048 bytes [-Werror=frame-larger-than=]
> drivers/gpu/drm/i915/gvt/handlers.c:2402:1: error: the frame size of 9400 
> bytes is larger than 2048 bytes [-Werror=frame-larger-than=]
> drivers/gpu/drm/i915/gvt/handlers.c:2628:1: error: the frame size of 11256 
> bytes is larger than 2048 bytes [-Werror=frame-larger-than=]
> 
> It is due to many expansions of MMIO_* macros in init_generic_mmio_info.
> INTEL_GVT_MMIO_OFFSET generates for each such line a __reg and an
> offset. There are too many for KASAN to keep up.
> 
> So disable KASAN for this file.
> 
> Signed-off-by: Jiri Slaby 
> Cc: Martin Liska 
> Cc: Zhenyu Wang 
> Cc: Zhi Wang 
> Cc: Daniel Vetter 
> Cc: Jani Nikula 
> Cc: David Airlie 
> Cc: intel-gvt-...@lists.freedesktop.org
> Cc: intel-...@lists.freedesktop.org
> Cc: dri-de...@lists.freedesktop.org
> ---
>  drivers/gpu/drm/i915/gvt/Makefile | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/Makefile 
> b/drivers/gpu/drm/i915/gvt/Makefile
> index b123c20e2097..942f1849d194 100644
> --- a/drivers/gpu/drm/i915/gvt/Makefile
> +++ b/drivers/gpu/drm/i915/gvt/Makefile
> @@ -6,3 +6,5 @@ GVT_SOURCE := gvt.o aperture_gm.o handlers.o vgpu.o 
> trace_points.o firmware.o \
>  ccflags-y+= -I$(src) -I$(src)/$(GVT_DIR) -Wall
>  i915-y   += $(addprefix $(GVT_DIR)/, 
> $(GVT_SOURCE))
>  obj-$(CONFIG_DRM_I915_GVT_KVMGT) += $(GVT_DIR)/kvmgt.o
> +
> +KASAN_SANITIZE_handlers.o := n
> -- 
> 2.12.2
> 

Applied this, we'd better cleanup legacy usage to current i915 mmio reg define. 
Thanks!

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [PATCH] drm/i915/kvmgt: avoid dereferencing a potentially null info pointer

[Zhenyu Wang]

On 2017.03.23 14:43:44 +0100, Frans Klaver wrote:
> On Thu, Mar 23, 2017 at 1:22 PM, Colin King  wrote:
> > From: Colin Ian King 
> >
> > info is being checked to see if it is a null pointer, however, vpgu is
> > dereferencing info before this check, leading to a potential null
> > pointer dereference.  If info is null, then the error message being
> > printed by macro gvt_vgpu_err and this requires vpgu to exist. We can
> > use a null vpgu as the macro has a sanity check to see if vpgu is null,
> > so this is OK.
> >
> > Detected with CoverityScan, CID#1420672 ("Dereference nefore null check")
> 
> s,nefore,before,
> 
> 
> >
> > Fixes: 695fbc08d80f ("drm/i915/gvt: replace the gvt_err with gvt_vgpu_err")
> > Signed-off-by: Colin Ian King 
> > ---
> >  drivers/gpu/drm/i915/gvt/kvmgt.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c 
> > b/drivers/gpu/drm/i915/gvt/kvmgt.c
> > index 1ea3eb270de8..f8619a772c44 100644
> > --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> > +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> > @@ -1339,9 +1339,9 @@ static int kvmgt_guest_init(struct mdev_device *mdev)
> >
> >  static bool kvmgt_guest_exit(struct kvmgt_guest_info *info)
> >  {
> > -   struct intel_vgpu *vgpu = info->vgpu;
> > -
> > if (!info) {
> > +   struct intel_vgpu *vgpu = NULL;
> > +
> > gvt_vgpu_err("kvmgt_guest_info invalid\n");
> > return false;
> > }
> 
> Does this mean the original gvt_err() macro is no longer there?
> 
> And apparently gvt_vgpu_err is a macro that depends on the specifics
> of its environment? Yikes.
> 

The null check is redundant there, we can just remove that block and extra
vgpu variable.

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [PATCH] drm/i915/kvmgt: avoid dereferencing a potentially null info pointer

[Zhenyu Wang]

On 2017.03.23 14:43:44 +0100, Frans Klaver wrote:
> On Thu, Mar 23, 2017 at 1:22 PM, Colin King  wrote:
> > From: Colin Ian King 
> >
> > info is being checked to see if it is a null pointer, however, vpgu is
> > dereferencing info before this check, leading to a potential null
> > pointer dereference.  If info is null, then the error message being
> > printed by macro gvt_vgpu_err and this requires vpgu to exist. We can
> > use a null vpgu as the macro has a sanity check to see if vpgu is null,
> > so this is OK.
> >
> > Detected with CoverityScan, CID#1420672 ("Dereference nefore null check")
> 
> s,nefore,before,
> 
> 
> >
> > Fixes: 695fbc08d80f ("drm/i915/gvt: replace the gvt_err with gvt_vgpu_err")
> > Signed-off-by: Colin Ian King 
> > ---
> >  drivers/gpu/drm/i915/gvt/kvmgt.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c 
> > b/drivers/gpu/drm/i915/gvt/kvmgt.c
> > index 1ea3eb270de8..f8619a772c44 100644
> > --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> > +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> > @@ -1339,9 +1339,9 @@ static int kvmgt_guest_init(struct mdev_device *mdev)
> >
> >  static bool kvmgt_guest_exit(struct kvmgt_guest_info *info)
> >  {
> > -   struct intel_vgpu *vgpu = info->vgpu;
> > -
> > if (!info) {
> > +   struct intel_vgpu *vgpu = NULL;
> > +
> > gvt_vgpu_err("kvmgt_guest_info invalid\n");
> > return false;
> > }
> 
> Does this mean the original gvt_err() macro is no longer there?
> 
> And apparently gvt_vgpu_err is a macro that depends on the specifics
> of its environment? Yikes.
> 

The null check is redundant there, we can just remove that block and extra
vgpu variable.

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [PATCH] kvmgt: Hold struct kvm reference

[Zhenyu Wang]

On 2017.03.20 14:58:06 +0800, Jike Song wrote:
> On 03/20/2017 10:38 AM, Alex Williamson wrote:
> > The kvmgt code keeps a pointer to the struct kvm associated with the
> > device, but doesn't actually hold a reference to it.  If we do unclean
> > shutdown testing (ie. killing the user process), then we can see the
> > kvm association to the device unset, which causes kvmgt to trigger a
> > device release via a work queue.  Naturally we cannot guarantee that
> > the cached struct kvm pointer is still valid at this point without
> > holding a reference.  The observed failure in this case is a stuck
> > cpu trying to acquire the spinlock from the invalid reference, but
> > other failure modes are clearly possible.  Hold a reference to avoid
> > this.
> > 
> > Signed-off-by: Alex Williamson <alex.william...@redhat.com>
> > Cc: sta...@vger.kernel.org #v4.10
> > Cc: Jike Song <jike.s...@intel.com>
> > Cc: Paolo Bonzini <pbonz...@redhat.com>
> > Cc: Zhenyu Wang <zhen...@linux.intel.com>
> > Cc: Zhi Wang <zhi.a.w...@intel.com>
> > ---
> 
> Reviewed-by: Jike Song <jike.s...@intel.com>
> 
> Thanks for the fix!
> 

queued in fixes tree, thanks!

> 
> >  drivers/gpu/drm/i915/gvt/kvmgt.c |2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c 
> > b/drivers/gpu/drm/i915/gvt/kvmgt.c
> > index 84d801638ede..142b8bd4ba6b 100644
> > --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> > +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> > @@ -1324,6 +1324,7 @@ static int kvmgt_guest_init(struct mdev_device *mdev)
> > vgpu->handle = (unsigned long)info;
> > info->vgpu = vgpu;
> > info->kvm = kvm;
> > +   kvm_get_kvm(info->kvm);
> >  
> > kvmgt_protect_table_init(info);
> > gvt_cache_init(vgpu);
> > @@ -1343,6 +1344,7 @@ static bool kvmgt_guest_exit(struct kvmgt_guest_info 
> > *info)
> > }
> >  
> > kvm_page_track_unregister_notifier(info->kvm, >track_node);
> > +   kvm_put_kvm(info->kvm);
> > kvmgt_protect_table_destroy(info);
> > gvt_cache_destroy(info->vgpu);
> > vfree(info);
> > 

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [PATCH] kvmgt: Hold struct kvm reference

[Zhenyu Wang]

On 2017.03.20 14:58:06 +0800, Jike Song wrote:
> On 03/20/2017 10:38 AM, Alex Williamson wrote:
> > The kvmgt code keeps a pointer to the struct kvm associated with the
> > device, but doesn't actually hold a reference to it.  If we do unclean
> > shutdown testing (ie. killing the user process), then we can see the
> > kvm association to the device unset, which causes kvmgt to trigger a
> > device release via a work queue.  Naturally we cannot guarantee that
> > the cached struct kvm pointer is still valid at this point without
> > holding a reference.  The observed failure in this case is a stuck
> > cpu trying to acquire the spinlock from the invalid reference, but
> > other failure modes are clearly possible.  Hold a reference to avoid
> > this.
> > 
> > Signed-off-by: Alex Williamson 
> > Cc: sta...@vger.kernel.org #v4.10
> > Cc: Jike Song 
> > Cc: Paolo Bonzini 
> > Cc: Zhenyu Wang 
> > Cc: Zhi Wang 
> > ---
> 
> Reviewed-by: Jike Song 
> 
> Thanks for the fix!
> 

queued in fixes tree, thanks!

> 
> >  drivers/gpu/drm/i915/gvt/kvmgt.c |2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c 
> > b/drivers/gpu/drm/i915/gvt/kvmgt.c
> > index 84d801638ede..142b8bd4ba6b 100644
> > --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> > +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> > @@ -1324,6 +1324,7 @@ static int kvmgt_guest_init(struct mdev_device *mdev)
> > vgpu->handle = (unsigned long)info;
> > info->vgpu = vgpu;
> > info->kvm = kvm;
> > +   kvm_get_kvm(info->kvm);
> >  
> > kvmgt_protect_table_init(info);
> > gvt_cache_init(vgpu);
> > @@ -1343,6 +1344,7 @@ static bool kvmgt_guest_exit(struct kvmgt_guest_info 
> > *info)
> > }
> >  
> > kvm_page_track_unregister_notifier(info->kvm, >track_node);
> > +   kvm_put_kvm(info->kvm);
> > kvmgt_protect_table_destroy(info);
> > gvt_cache_destroy(info->vgpu);
> > vfree(info);
> > 

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [igvt-g-dev] [PATCH] drm/i915/gvt: Fix kmem_cache_create() name

[Zhenyu Wang]

On 2017.01.24 13:15:43 -0700, Alex Williamson wrote:
> According to kmem_cache_sanity_check(), spaces are not allowed in the
> name of a cache and results in a kernel oops with CONFIG_DEBUG_VM.
> Convert to underscores.
> 
> Signed-off-by: Alex Williamson 
> ---

Will send to 4.10 fixes. Thanks!

>  drivers/gpu/drm/i915/gvt/execlist.c |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/execlist.c 
> b/drivers/gpu/drm/i915/gvt/execlist.c
> index f32bb6f..d03b229 100644
> --- a/drivers/gpu/drm/i915/gvt/execlist.c
> +++ b/drivers/gpu/drm/i915/gvt/execlist.c
> @@ -826,7 +826,7 @@ int intel_vgpu_init_execlist(struct intel_vgpu *vgpu)
>   INIT_LIST_HEAD(>workload_q_head[i]);
>   }
>  
> - vgpu->workloads = kmem_cache_create("gvt-g vgpu workload",
> + vgpu->workloads = kmem_cache_create("gvt-g_vgpu_workload",
>   sizeof(struct intel_vgpu_workload), 0,
>   SLAB_HWCACHE_ALIGN,
>   NULL);
> 
> ___
> igvt-g-dev mailing list
> igvt-g-...@lists.01.org
> https://lists.01.org/mailman/listinfo/igvt-g-dev

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [igvt-g-dev] [PATCH] drm/i915/gvt: Fix kmem_cache_create() name

[Zhenyu Wang]

On 2017.01.24 13:15:43 -0700, Alex Williamson wrote:
> According to kmem_cache_sanity_check(), spaces are not allowed in the
> name of a cache and results in a kernel oops with CONFIG_DEBUG_VM.
> Convert to underscores.
> 
> Signed-off-by: Alex Williamson 
> ---

Will send to 4.10 fixes. Thanks!

>  drivers/gpu/drm/i915/gvt/execlist.c |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/execlist.c 
> b/drivers/gpu/drm/i915/gvt/execlist.c
> index f32bb6f..d03b229 100644
> --- a/drivers/gpu/drm/i915/gvt/execlist.c
> +++ b/drivers/gpu/drm/i915/gvt/execlist.c
> @@ -826,7 +826,7 @@ int intel_vgpu_init_execlist(struct intel_vgpu *vgpu)
>   INIT_LIST_HEAD(>workload_q_head[i]);
>   }
>  
> - vgpu->workloads = kmem_cache_create("gvt-g vgpu workload",
> + vgpu->workloads = kmem_cache_create("gvt-g_vgpu_workload",
>   sizeof(struct intel_vgpu_workload), 0,
>   SLAB_HWCACHE_ALIGN,
>   NULL);
> 
> ___
> igvt-g-dev mailing list
> igvt-g-...@lists.01.org
> https://lists.01.org/mailman/listinfo/igvt-g-dev

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [igvt-g-dev] [PATCH] drm/i915/gvt/kvmgt: mdev ABI is available_instances, not available_instance

[Zhenyu Wang]

On 2017.01.24 12:53:45 -0700, Alex Williamson wrote:
> Per the ABI specification[1], each mdev_supported_types entry should
> have an available_instances, with an "s", not available_instance.
> 
> [1] Documentation/ABI/testing/sysfs-bus-vfio-mdev
> 
> Signed-off-by: Alex Williamson 
> ---
> 
> This should really be fixed before initial release in v4.10

Will queue this up for 4.10 fixes. Thanks!

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [igvt-g-dev] [PATCH] drm/i915/gvt/kvmgt: mdev ABI is available_instances, not available_instance

[Zhenyu Wang]

On 2017.01.24 12:53:45 -0700, Alex Williamson wrote:
> Per the ABI specification[1], each mdev_supported_types entry should
> have an available_instances, with an "s", not available_instance.
> 
> [1] Documentation/ABI/testing/sysfs-bus-vfio-mdev
> 
> Signed-off-by: Alex Williamson 
> ---
> 
> This should really be fixed before initial release in v4.10

Will queue this up for 4.10 fixes. Thanks!

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

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

[Zhenyu Wang]

On 2017.01.02 21:48:57 -0700, Alex Williamson wrote:
> > Alex, I liked to have kvmgt related mdev interface change be merged through
> > vfio tree, but wasn't awared one of Jike's fix had conflict. Could you apply
> > below fix in your tree? I think in general for possible interface change in
> > future we still need a pull request for i915 to resolve dependence earlier.
> 
> Hi Zhenyu,
> 
> Hopefully this abstraction will help to isolate vendor drivers from
> mdev API changes in the future.  I can certainly roll this patch into
> the original to maintain bisectability.  I want to get these changes in
> for rc3, will a pull request for the i915 changes be sent this week?

Send to Jani who is managing i915 fixes pull.

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

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

[Zhenyu Wang]

On 2017.01.02 21:48:57 -0700, Alex Williamson wrote:
> > Alex, I liked to have kvmgt related mdev interface change be merged through
> > vfio tree, but wasn't awared one of Jike's fix had conflict. Could you apply
> > below fix in your tree? I think in general for possible interface change in
> > future we still need a pull request for i915 to resolve dependence earlier.
> 
> Hi Zhenyu,
> 
> Hopefully this abstraction will help to isolate vendor drivers from
> mdev API changes in the future.  I can certainly roll this patch into
> the original to maintain bisectability.  I want to get these changes in
> for rc3, will a pull request for the i915 changes be sent this week?

Send to Jani who is managing i915 fixes pull.

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

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

[Zhenyu Wang]

On 2017.01.03 10:42:39 +1100, Stephen Rothwell wrote:
> Hi all,
> 
> After merging the drm-intel-fixes tree, today's linux-next build (x86_64
> allmodconfig) failed like this:
> 
> drivers/gpu/drm/i915/gvt/kvmgt.c: In function 'intel_vgpu_open':
> drivers/gpu/drm/i915/gvt/kvmgt.c:511:32: error: dereferencing pointer to 
> incomplete type 'struct mdev_device'
>   vfio_unregister_notifier(>dev, VFIO_GROUP_NOTIFY,
> ^   
> 
> Caused by commit
> 
>   99e3123e3d72 ("vfio-mdev: Make mdev_device private and abstract interfaces")
> 
> from the vfio-fixes tree interacting with commit
> 
>   364fb6b789ff ("drm/i915/gvt/kvmgt: prevent double-release of vgpu")
> 
> from the drm-intel-fixes tree.

Alex, I liked to have kvmgt related mdev interface change be merged through
vfio tree, but wasn't awared one of Jike's fix had conflict. Could you apply
below fix in your tree? I think in general for possible interface change in
future we still need a pull request for i915 to resolve dependence earlier.

Thanks.

> 
> I applied this merge fix patch:
> 
> From: Stephen Rothwell 
> Date: Tue, 3 Jan 2017 10:38:48 +1100
> Subject: [PATCH] vfio-mdev: fixup for "Make mdev_device private and abstract 
> interfaces"
> 
> Signed-off-by: Stephen Rothwell 
> ---
>  drivers/gpu/drm/i915/gvt/kvmgt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c 
> b/drivers/gpu/drm/i915/gvt/kvmgt.c
> index c24b665e007b..faaae07ae487 100644
> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> @@ -508,7 +508,7 @@ static int intel_vgpu_open(struct mdev_device *mdev)
>   return ret;
>  
>  undo_group:
> - vfio_unregister_notifier(>dev, VFIO_GROUP_NOTIFY,
> + vfio_unregister_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY,
>   >vdev.group_notifier);
>  
>  undo_iommu:
> -- 
> 2.10.2
> 
> -- 
> Cheers,
> Stephen Rothwell

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

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

[Zhenyu Wang]

On 2017.01.03 10:42:39 +1100, Stephen Rothwell wrote:
> Hi all,
> 
> After merging the drm-intel-fixes tree, today's linux-next build (x86_64
> allmodconfig) failed like this:
> 
> drivers/gpu/drm/i915/gvt/kvmgt.c: In function 'intel_vgpu_open':
> drivers/gpu/drm/i915/gvt/kvmgt.c:511:32: error: dereferencing pointer to 
> incomplete type 'struct mdev_device'
>   vfio_unregister_notifier(>dev, VFIO_GROUP_NOTIFY,
> ^   
> 
> Caused by commit
> 
>   99e3123e3d72 ("vfio-mdev: Make mdev_device private and abstract interfaces")
> 
> from the vfio-fixes tree interacting with commit
> 
>   364fb6b789ff ("drm/i915/gvt/kvmgt: prevent double-release of vgpu")
> 
> from the drm-intel-fixes tree.

Alex, I liked to have kvmgt related mdev interface change be merged through
vfio tree, but wasn't awared one of Jike's fix had conflict. Could you apply
below fix in your tree? I think in general for possible interface change in
future we still need a pull request for i915 to resolve dependence earlier.

Thanks.

> 
> I applied this merge fix patch:
> 
> From: Stephen Rothwell 
> Date: Tue, 3 Jan 2017 10:38:48 +1100
> Subject: [PATCH] vfio-mdev: fixup for "Make mdev_device private and abstract 
> interfaces"
> 
> Signed-off-by: Stephen Rothwell 
> ---
>  drivers/gpu/drm/i915/gvt/kvmgt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c 
> b/drivers/gpu/drm/i915/gvt/kvmgt.c
> index c24b665e007b..faaae07ae487 100644
> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> @@ -508,7 +508,7 @@ static int intel_vgpu_open(struct mdev_device *mdev)
>   return ret;
>  
>  undo_group:
> - vfio_unregister_notifier(>dev, VFIO_GROUP_NOTIFY,
> + vfio_unregister_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY,
>   >vdev.group_notifier);
>  
>  undo_iommu:
> -- 
> 2.10.2
> 
> -- 
> Cheers,
> Stephen Rothwell

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [igvt-g-dev] [PATCH 1/1] drm/i915/gvt: verify functions types in new_mmio_info()

[Zhenyu Wang]

On 2016.12.26 14:52:23 +0100, Nicolas Iooss wrote:
> The current prototype of new_mmio_info() uses void* for parameters read
> and write, which are functions with precise calling conventions
> (argument types and return type). Write down these conventions in
> new_mmio_info() definition.
> 
> This has been reported by the following warnings when clang is used to
> build the kernel:
> 
> drivers/gpu/drm/i915/gvt/handlers.c:124:21: error: pointer type
> mismatch ('void *' and 'int (*)(struct intel_vgpu *, unsigned int,
> void *, unsigned int)') [-Werror,-Wpointer-type-mismatch]
> info->read = read ? read : intel_vgpu_default_mmio_read;
>   ^    
> drivers/gpu/drm/i915/gvt/handlers.c:125:23: error: pointer type
> mismatch ('void *' and 'int (*)(struct intel_vgpu *, unsigned int,
> void *, unsigned int)') [-Werror,-Wpointer-type-mismatch]
> info->write = write ? write : intel_vgpu_default_mmio_write;
> ^ ~   ~
> 
> This allows the compiler to detect that sbi_ctl_mmio_write() returns a
> "bool" value instead of an expected "int" one. Fix this.
>

Looks good to me. Will queue this up. Thanks

> Signed-off-by: Nicolas Iooss 
> ---
>  drivers/gpu/drm/i915/gvt/handlers.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/handlers.c 
> b/drivers/gpu/drm/i915/gvt/handlers.c
> index 522809710312..052e57124c0a 100644
> --- a/drivers/gpu/drm/i915/gvt/handlers.c
> +++ b/drivers/gpu/drm/i915/gvt/handlers.c
> @@ -93,7 +93,8 @@ static void write_vreg(struct intel_vgpu *vgpu, unsigned 
> int offset,
>  static int new_mmio_info(struct intel_gvt *gvt,
>   u32 offset, u32 flags, u32 size,
>   u32 addr_mask, u32 ro_mask, u32 device,
> - void *read, void *write)
> + int (*read)(struct intel_vgpu *, unsigned int, void *, unsigned 
> int),
> + int (*write)(struct intel_vgpu *, unsigned int, void *, 
> unsigned int))
>  {
>   struct intel_gvt_mmio_info *info, *p;
>   u32 start, end, i;
> @@ -974,7 +975,7 @@ static int sbi_data_mmio_read(struct intel_vgpu *vgpu, 
> unsigned int offset,
>   return 0;
>  }
>  
> -static bool sbi_ctl_mmio_write(struct intel_vgpu *vgpu, unsigned int offset,
> +static int sbi_ctl_mmio_write(struct intel_vgpu *vgpu, unsigned int offset,
>   void *p_data, unsigned int bytes)
>  {
>   u32 data;
> -- 
> 2.11.0
> 
> ___
> igvt-g-dev mailing list
> igvt-g-...@lists.01.org
> https://lists.01.org/mailman/listinfo/igvt-g-dev

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [igvt-g-dev] [PATCH 1/1] drm/i915/gvt: verify functions types in new_mmio_info()

[Zhenyu Wang]

On 2016.12.26 14:52:23 +0100, Nicolas Iooss wrote:
> The current prototype of new_mmio_info() uses void* for parameters read
> and write, which are functions with precise calling conventions
> (argument types and return type). Write down these conventions in
> new_mmio_info() definition.
> 
> This has been reported by the following warnings when clang is used to
> build the kernel:
> 
> drivers/gpu/drm/i915/gvt/handlers.c:124:21: error: pointer type
> mismatch ('void *' and 'int (*)(struct intel_vgpu *, unsigned int,
> void *, unsigned int)') [-Werror,-Wpointer-type-mismatch]
> info->read = read ? read : intel_vgpu_default_mmio_read;
>   ^    
> drivers/gpu/drm/i915/gvt/handlers.c:125:23: error: pointer type
> mismatch ('void *' and 'int (*)(struct intel_vgpu *, unsigned int,
> void *, unsigned int)') [-Werror,-Wpointer-type-mismatch]
> info->write = write ? write : intel_vgpu_default_mmio_write;
> ^ ~   ~
> 
> This allows the compiler to detect that sbi_ctl_mmio_write() returns a
> "bool" value instead of an expected "int" one. Fix this.
>

Looks good to me. Will queue this up. Thanks

> Signed-off-by: Nicolas Iooss 
> ---
>  drivers/gpu/drm/i915/gvt/handlers.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/handlers.c 
> b/drivers/gpu/drm/i915/gvt/handlers.c
> index 522809710312..052e57124c0a 100644
> --- a/drivers/gpu/drm/i915/gvt/handlers.c
> +++ b/drivers/gpu/drm/i915/gvt/handlers.c
> @@ -93,7 +93,8 @@ static void write_vreg(struct intel_vgpu *vgpu, unsigned 
> int offset,
>  static int new_mmio_info(struct intel_gvt *gvt,
>   u32 offset, u32 flags, u32 size,
>   u32 addr_mask, u32 ro_mask, u32 device,
> - void *read, void *write)
> + int (*read)(struct intel_vgpu *, unsigned int, void *, unsigned 
> int),
> + int (*write)(struct intel_vgpu *, unsigned int, void *, 
> unsigned int))
>  {
>   struct intel_gvt_mmio_info *info, *p;
>   u32 start, end, i;
> @@ -974,7 +975,7 @@ static int sbi_data_mmio_read(struct intel_vgpu *vgpu, 
> unsigned int offset,
>   return 0;
>  }
>  
> -static bool sbi_ctl_mmio_write(struct intel_vgpu *vgpu, unsigned int offset,
> +static int sbi_ctl_mmio_write(struct intel_vgpu *vgpu, unsigned int offset,
>   void *p_data, unsigned int bytes)
>  {
>   u32 data;
> -- 
> 2.11.0
> 
> ___
> igvt-g-dev mailing list
> igvt-g-...@lists.01.org
> https://lists.01.org/mailman/listinfo/igvt-g-dev

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

[RESEND][GIT PULL] i915/gvt KVMGT tree for 4.10

[Zhenyu Wang]

Hi, Linus

This is KVMGT pull for 4.10 as explained by Daniel. The last minute
rebase is to appease git pull-request since the diffstat was obvious
bonghits and somehow included vfio/mdev again despite that was merged
already.

Thanks.

The following changes since commit 9439b3710df688d853eb6cb4851256f2c92b1797:

  Merge tag 'drm-for-v4.10' of git://people.freedesktop.org/~airlied/linux 
(2016-12-13 09:35:09 -0800)

are available in the git repository at:

  https://github.com/01org/gvt-linux.git tags/kvmgt-vfio-mdev-for-v4.10-rc1

for you to fetch changes up to 659643f7d81432189c2c87230e2feee4c75c14c1:

  drm/i915/gvt/kvmgt: add vfio/mdev support to KVMGT (2016-12-16 16:55:26 +0800)


kvmgt-vfio-mdev-for-v4.10-rc1

This is KVMGT support depending on VFIO/mdev framework.


Jike Song (3):
  drm/i915/gvt/kvmgt: replace kmalloc() by kzalloc()
  drm/i915/gvt/kvmgt: read/write GPA via KVM API
  drm/i915/gvt/kvmgt: add vfio/mdev support to KVMGT

 drivers/gpu/drm/i915/Kconfig  |   1 +
 drivers/gpu/drm/i915/gvt/Makefile |   2 -
 drivers/gpu/drm/i915/gvt/gvt.h|   6 +-
 drivers/gpu/drm/i915/gvt/kvmgt.c  | 975 +++---
 4 files changed, 923 insertions(+), 61 deletions(-)

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

[RESEND][GIT PULL] i915/gvt KVMGT tree for 4.10

[Zhenyu Wang]

Hi, Linus

This is KVMGT pull for 4.10 as explained by Daniel. The last minute
rebase is to appease git pull-request since the diffstat was obvious
bonghits and somehow included vfio/mdev again despite that was merged
already.

Thanks.

The following changes since commit 9439b3710df688d853eb6cb4851256f2c92b1797:

  Merge tag 'drm-for-v4.10' of git://people.freedesktop.org/~airlied/linux 
(2016-12-13 09:35:09 -0800)

are available in the git repository at:

  https://github.com/01org/gvt-linux.git tags/kvmgt-vfio-mdev-for-v4.10-rc1

for you to fetch changes up to 659643f7d81432189c2c87230e2feee4c75c14c1:

  drm/i915/gvt/kvmgt: add vfio/mdev support to KVMGT (2016-12-16 16:55:26 +0800)


kvmgt-vfio-mdev-for-v4.10-rc1

This is KVMGT support depending on VFIO/mdev framework.


Jike Song (3):
  drm/i915/gvt/kvmgt: replace kmalloc() by kzalloc()
  drm/i915/gvt/kvmgt: read/write GPA via KVM API
  drm/i915/gvt/kvmgt: add vfio/mdev support to KVMGT

 drivers/gpu/drm/i915/Kconfig  |   1 +
 drivers/gpu/drm/i915/gvt/Makefile |   2 -
 drivers/gpu/drm/i915/gvt/gvt.h|   6 +-
 drivers/gpu/drm/i915/gvt/kvmgt.c  | 975 +++---
 4 files changed, 923 insertions(+), 61 deletions(-)

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [PATCH] drm/i915/gvt: fix deadlock in dispatch_workload()'s error path

[Zhenyu Wang]

On 2016.12.04 23:57:18 +, Eric Engestrom wrote:
> 90d27a1 moved the lock before this error path but forgot to add an
> unlock here.
> 
> Fixes: 90d27a1b180e51ef0713 ("drm/i915/gvt: fix deadlock in workload_thread")
> Cc: Pei Zhang <pei.zh...@intel.com>
> Cc: Zhenyu Wang <zhen...@linux.intel.com>
> Signed-off-by: Eric Engestrom <e...@engestrom.ch>
> ---

Hi, this has been fixed on 
https://cgit.freedesktop.org/drm/drm-intel/commit/?h=drm-intel-next-fixes=53d6f812c0dbf1c9cad89b1c2118e61c13ca9677

Thanks!

>  drivers/gpu/drm/i915/gvt/scheduler.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c 
> b/drivers/gpu/drm/i915/gvt/scheduler.c
> index f898df3..cd13c4b 100644
> --- a/drivers/gpu/drm/i915/gvt/scheduler.c
> +++ b/drivers/gpu/drm/i915/gvt/scheduler.c
> @@ -177,6 +177,7 @@ static int dispatch_workload(struct intel_vgpu_workload 
> *workload)
>   rq = i915_gem_request_alloc(dev_priv->engine[ring_id], shadow_ctx);
>   if (IS_ERR(rq)) {
>   gvt_err("fail to allocate gem request\n");
> + mutex_unlock(_priv->drm.struct_mutex);
>   workload->status = PTR_ERR(rq);
>   return workload->status;
>   }
> -- 
> Cheers,
>   Eric
> 

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [PATCH] drm/i915/gvt: fix deadlock in dispatch_workload()'s error path

[Zhenyu Wang]

On 2016.12.04 23:57:18 +, Eric Engestrom wrote:
> 90d27a1 moved the lock before this error path but forgot to add an
> unlock here.
> 
> Fixes: 90d27a1b180e51ef0713 ("drm/i915/gvt: fix deadlock in workload_thread")
> Cc: Pei Zhang 
> Cc: Zhenyu Wang 
> Signed-off-by: Eric Engestrom 
> ---

Hi, this has been fixed on 
https://cgit.freedesktop.org/drm/drm-intel/commit/?h=drm-intel-next-fixes=53d6f812c0dbf1c9cad89b1c2118e61c13ca9677

Thanks!

>  drivers/gpu/drm/i915/gvt/scheduler.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c 
> b/drivers/gpu/drm/i915/gvt/scheduler.c
> index f898df3..cd13c4b 100644
> --- a/drivers/gpu/drm/i915/gvt/scheduler.c
> +++ b/drivers/gpu/drm/i915/gvt/scheduler.c
> @@ -177,6 +177,7 @@ static int dispatch_workload(struct intel_vgpu_workload 
> *workload)
>   rq = i915_gem_request_alloc(dev_priv->engine[ring_id], shadow_ctx);
>   if (IS_ERR(rq)) {
>   gvt_err("fail to allocate gem request\n");
> + mutex_unlock(_priv->drm.struct_mutex);
>   workload->status = PTR_ERR(rq);
>   return workload->status;
>   }
> -- 
> Cheers,
>   Eric
> 

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [PATCH v14 00/22] Add Mediated device support

[Zhenyu Wang]

On 2016.11.17 16:51:45 -0700, Alex Williamson wrote:
> On Thu, 17 Nov 2016 23:29:38 +
> "Tian, Kevin"  wrote:
> 
> > > From: Alex Williamson [mailto:alex.william...@redhat.com]
> > > Sent: Friday, November 18, 2016 5:25 AM
> > > 
> > > On Thu, 17 Nov 2016 02:16:12 +0530
> > > Kirti Wankhede  wrote:  
> > > >
> > > >  Documentation/ABI/testing/sysfs-bus-vfio-mdev |  111 ++
> > > >  Documentation/vfio-mediated-device.txt|  399 +++
> > > >  MAINTAINERS   |9 +
> > > >  drivers/vfio/Kconfig  |1 +
> > > >  drivers/vfio/Makefile |1 +
> > > >  drivers/vfio/mdev/Kconfig |   17 +
> > > >  drivers/vfio/mdev/Makefile|5 +
> > > >  drivers/vfio/mdev/mdev_core.c |  385 +++
> > > >  drivers/vfio/mdev/mdev_driver.c   |  119 ++
> > > >  drivers/vfio/mdev/mdev_private.h  |   41 +
> > > >  drivers/vfio/mdev/mdev_sysfs.c|  286 +
> > > >  drivers/vfio/mdev/vfio_mdev.c |  180 +++
> > > >  drivers/vfio/pci/vfio_pci.c   |   83 +-
> > > >  drivers/vfio/platform/vfio_platform_common.c  |   31 +-
> > > >  drivers/vfio/vfio.c   |  340 +-
> > > >  drivers/vfio/vfio_iommu_type1.c   |  872 +++---
> > > >  include/linux/mdev.h  |  177 +++
> > > >  include/linux/vfio.h  |   32 +-
> > > >  include/uapi/linux/vfio.h |   10 +
> > > >  samples/vfio-mdev/Makefile|   13 +
> > > >  samples/vfio-mdev/mtty.c  | 1503  
> > > +  
> > > >  21 files changed, 4358 insertions(+), 257 deletions(-)
> > > >  create mode 100644 Documentation/ABI/testing/sysfs-bus-vfio-mdev
> > > >  create mode 100644 Documentation/vfio-mediated-device.txt
> > > >  create mode 100644 drivers/vfio/mdev/Kconfig
> > > >  create mode 100644 drivers/vfio/mdev/Makefile
> > > >  create mode 100644 drivers/vfio/mdev/mdev_core.c
> > > >  create mode 100644 drivers/vfio/mdev/mdev_driver.c
> > > >  create mode 100644 drivers/vfio/mdev/mdev_private.h
> > > >  create mode 100644 drivers/vfio/mdev/mdev_sysfs.c
> > > >  create mode 100644 drivers/vfio/mdev/vfio_mdev.c
> > > >  create mode 100644 include/linux/mdev.h
> > > >  create mode 100644 samples/vfio-mdev/Makefile
> > > >  create mode 100644 samples/vfio-mdev/mtty.c  
> > > 
> > > As discussed, I dropped patch 12, updated the documentation, and added
> > > 'retries' initialization.  This is now applied to my next branch for
> > > v4.10.  Thanks to the reviewers and Kirti and Neo for your hard work!
> > > Thanks,
> > >   
> > 
> > That's a great news! Alex, do you have an idea when this series may
> > hit linux-next? :-)
> 
> Whenever there's a new build, hopefully within the next 24hrs, but I
> don't really know the schedule.  Thanks,
> 

Alex, could you do a pull request of mdev for Daniel's drm-intel tree?
We need to send KVMGT mdev support pull base on that.

Thanks!

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [PATCH v14 00/22] Add Mediated device support

[Zhenyu Wang]

On 2016.11.17 16:51:45 -0700, Alex Williamson wrote:
> On Thu, 17 Nov 2016 23:29:38 +
> "Tian, Kevin"  wrote:
> 
> > > From: Alex Williamson [mailto:alex.william...@redhat.com]
> > > Sent: Friday, November 18, 2016 5:25 AM
> > > 
> > > On Thu, 17 Nov 2016 02:16:12 +0530
> > > Kirti Wankhede  wrote:  
> > > >
> > > >  Documentation/ABI/testing/sysfs-bus-vfio-mdev |  111 ++
> > > >  Documentation/vfio-mediated-device.txt|  399 +++
> > > >  MAINTAINERS   |9 +
> > > >  drivers/vfio/Kconfig  |1 +
> > > >  drivers/vfio/Makefile |1 +
> > > >  drivers/vfio/mdev/Kconfig |   17 +
> > > >  drivers/vfio/mdev/Makefile|5 +
> > > >  drivers/vfio/mdev/mdev_core.c |  385 +++
> > > >  drivers/vfio/mdev/mdev_driver.c   |  119 ++
> > > >  drivers/vfio/mdev/mdev_private.h  |   41 +
> > > >  drivers/vfio/mdev/mdev_sysfs.c|  286 +
> > > >  drivers/vfio/mdev/vfio_mdev.c |  180 +++
> > > >  drivers/vfio/pci/vfio_pci.c   |   83 +-
> > > >  drivers/vfio/platform/vfio_platform_common.c  |   31 +-
> > > >  drivers/vfio/vfio.c   |  340 +-
> > > >  drivers/vfio/vfio_iommu_type1.c   |  872 +++---
> > > >  include/linux/mdev.h  |  177 +++
> > > >  include/linux/vfio.h  |   32 +-
> > > >  include/uapi/linux/vfio.h |   10 +
> > > >  samples/vfio-mdev/Makefile|   13 +
> > > >  samples/vfio-mdev/mtty.c  | 1503  
> > > +  
> > > >  21 files changed, 4358 insertions(+), 257 deletions(-)
> > > >  create mode 100644 Documentation/ABI/testing/sysfs-bus-vfio-mdev
> > > >  create mode 100644 Documentation/vfio-mediated-device.txt
> > > >  create mode 100644 drivers/vfio/mdev/Kconfig
> > > >  create mode 100644 drivers/vfio/mdev/Makefile
> > > >  create mode 100644 drivers/vfio/mdev/mdev_core.c
> > > >  create mode 100644 drivers/vfio/mdev/mdev_driver.c
> > > >  create mode 100644 drivers/vfio/mdev/mdev_private.h
> > > >  create mode 100644 drivers/vfio/mdev/mdev_sysfs.c
> > > >  create mode 100644 drivers/vfio/mdev/vfio_mdev.c
> > > >  create mode 100644 include/linux/mdev.h
> > > >  create mode 100644 samples/vfio-mdev/Makefile
> > > >  create mode 100644 samples/vfio-mdev/mtty.c  
> > > 
> > > As discussed, I dropped patch 12, updated the documentation, and added
> > > 'retries' initialization.  This is now applied to my next branch for
> > > v4.10.  Thanks to the reviewers and Kirti and Neo for your hard work!
> > > Thanks,
> > >   
> > 
> > That's a great news! Alex, do you have an idea when this series may
> > hit linux-next? :-)
> 
> Whenever there's a new build, hopefully within the next 24hrs, but I
> don't really know the schedule.  Thanks,
> 

Alex, could you do a pull request of mdev for Daniel's drm-intel tree?
We need to send KVMGT mdev support pull base on that.

Thanks!

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [PATCH 2/2] drm/i915/gvt: fix compilation

[Zhenyu Wang]

On 2016.10.21 17:25:50 +0200, Arnd Bergmann wrote:
> Two functions in the newly added gvt render code are obviously
> broken, as they reference a variable without initialization and
> don't reference another variable at all:
> 
> drivers/gpu/drm/i915/gvt/render.c: In function 
> ???intel_gvt_load_render_mmio???:
> drivers/gpu/drm/i915/gvt/render.c:148:13: error: ???offset.reg??? may be used 
> uninitialized in this function [-Werror=maybe-uninitialized]
> drivers/gpu/drm/i915/gvt/render.c: In function 
> ???intel_gvt_restore_render_mmio???:
> drivers/gpu/drm/i915/gvt/render.c:185:13: error: ???offset.reg??? may be used 
> uninitialized in this function [-Werror=maybe-uninitialized]
> 
> This is probably not a correct fix, but it gets us a clean build
> by removing the unused arrays and initializing the offset variable
> to something that potentially might be correct.
> 
> Fixes: 178657139307 ("drm/i915/gvt: vGPU context switch")
> Signed-off-by: Arnd Bergmann 
> ---

I think the correct fix is like

diff --git a/drivers/gpu/drm/i915/gvt/render.c 
b/drivers/gpu/drm/i915/gvt/render.c
index feebb65..cc23c3f 100644
--- a/drivers/gpu/drm/i915/gvt/render.c
+++ b/drivers/gpu/drm/i915/gvt/render.c
@@ -162,6 +162,7 @@ static void load_mocs(struct intel_vgpu *vgpu, int ring_id)
if (!IS_SKYLAKE(dev_priv))
return;
 
+   offset.reg = regs[ring_id];
for (i = 0; i < 64; i++) {
gen9_render_mocs[ring_id][i] = I915_READ(offset);
I915_WRITE(offset, vgpu_vreg(vgpu, offset));
@@ -199,6 +200,7 @@ static void restore_mocs(struct intel_vgpu *vgpu, int 
ring_id)
if (!IS_SKYLAKE(dev_priv))
return;
 
+   offset.reg = regs[ring_id];
for (i = 0; i < 64; i++) {
vgpu_vreg(vgpu, offset) = I915_READ(offset);
I915_WRITE(offset, gen9_render_mocs[ring_id][i]);

Thanks for pointing this out, it's a mistake during our code preparation for 
upstream.
I'll queue this up.

>  drivers/gpu/drm/i915/gvt/render.c | 25 +++--
>  1 file changed, 3 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/render.c 
> b/drivers/gpu/drm/i915/gvt/render.c
> index feebb65ba641..79e112288065 100644
> --- a/drivers/gpu/drm/i915/gvt/render.c
> +++ b/drivers/gpu/drm/i915/gvt/render.c
> @@ -147,29 +147,20 @@ static void load_mocs(struct intel_vgpu *vgpu, int 
> ring_id)
>  {
>   struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv;
>   i915_reg_t offset, l3_offset;
> - u32 regs[] = {
> - [RCS] = 0xc800,
> - [VCS] = 0xc900,
> - [VCS2] = 0xca00,
> - [BCS] = 0xcc00,
> - [VECS] = 0xcb00,
> - };
>   int i;
>  
> - if (WARN_ON(ring_id >= ARRAY_SIZE(regs)))
> - return;
> -
>   if (!IS_SKYLAKE(dev_priv))
>   return;
>  
>   for (i = 0; i < 64; i++) {
> + offset.reg = i * 4;
>   gen9_render_mocs[ring_id][i] = I915_READ(offset);
>   I915_WRITE(offset, vgpu_vreg(vgpu, offset));
>   POSTING_READ(offset);
> - offset.reg += 4;
>   }
>  
>   if (ring_id == RCS) {
> + offset.reg = 64 * 4;
>   l3_offset.reg = 0xb020;
>   for (i = 0; i < 32; i++) {
>   gen9_render_mocs_L3[i] = I915_READ(l3_offset);
> @@ -184,26 +175,16 @@ static void restore_mocs(struct intel_vgpu *vgpu, int 
> ring_id)
>  {
>   struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv;
>   i915_reg_t offset, l3_offset;
> - u32 regs[] = {
> - [RCS] = 0xc800,
> - [VCS] = 0xc900,
> - [VCS2] = 0xca00,
> - [BCS] = 0xcc00,
> - [VECS] = 0xcb00,
> - };
>   int i;
>  
> - if (WARN_ON(ring_id >= ARRAY_SIZE(regs)))
> - return;
> -
>   if (!IS_SKYLAKE(dev_priv))
>   return;
>  
>   for (i = 0; i < 64; i++) {
> + offset.reg = i * 4;
>   vgpu_vreg(vgpu, offset) = I915_READ(offset);
>   I915_WRITE(offset, gen9_render_mocs[ring_id][i]);
>   POSTING_READ(offset);
> - offset.reg += 4;
>   }
>  
>   if (ring_id == RCS) {
> -- 
> 2.9.0
> 

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [PATCH 2/2] drm/i915/gvt: fix compilation

[Zhenyu Wang]

On 2016.10.21 17:25:50 +0200, Arnd Bergmann wrote:
> Two functions in the newly added gvt render code are obviously
> broken, as they reference a variable without initialization and
> don't reference another variable at all:
> 
> drivers/gpu/drm/i915/gvt/render.c: In function 
> ???intel_gvt_load_render_mmio???:
> drivers/gpu/drm/i915/gvt/render.c:148:13: error: ???offset.reg??? may be used 
> uninitialized in this function [-Werror=maybe-uninitialized]
> drivers/gpu/drm/i915/gvt/render.c: In function 
> ???intel_gvt_restore_render_mmio???:
> drivers/gpu/drm/i915/gvt/render.c:185:13: error: ???offset.reg??? may be used 
> uninitialized in this function [-Werror=maybe-uninitialized]
> 
> This is probably not a correct fix, but it gets us a clean build
> by removing the unused arrays and initializing the offset variable
> to something that potentially might be correct.
> 
> Fixes: 178657139307 ("drm/i915/gvt: vGPU context switch")
> Signed-off-by: Arnd Bergmann 
> ---

I think the correct fix is like

diff --git a/drivers/gpu/drm/i915/gvt/render.c 
b/drivers/gpu/drm/i915/gvt/render.c
index feebb65..cc23c3f 100644
--- a/drivers/gpu/drm/i915/gvt/render.c
+++ b/drivers/gpu/drm/i915/gvt/render.c
@@ -162,6 +162,7 @@ static void load_mocs(struct intel_vgpu *vgpu, int ring_id)
if (!IS_SKYLAKE(dev_priv))
return;
 
+   offset.reg = regs[ring_id];
for (i = 0; i < 64; i++) {
gen9_render_mocs[ring_id][i] = I915_READ(offset);
I915_WRITE(offset, vgpu_vreg(vgpu, offset));
@@ -199,6 +200,7 @@ static void restore_mocs(struct intel_vgpu *vgpu, int 
ring_id)
if (!IS_SKYLAKE(dev_priv))
return;
 
+   offset.reg = regs[ring_id];
for (i = 0; i < 64; i++) {
vgpu_vreg(vgpu, offset) = I915_READ(offset);
I915_WRITE(offset, gen9_render_mocs[ring_id][i]);

Thanks for pointing this out, it's a mistake during our code preparation for 
upstream.
I'll queue this up.

>  drivers/gpu/drm/i915/gvt/render.c | 25 +++--
>  1 file changed, 3 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/render.c 
> b/drivers/gpu/drm/i915/gvt/render.c
> index feebb65ba641..79e112288065 100644
> --- a/drivers/gpu/drm/i915/gvt/render.c
> +++ b/drivers/gpu/drm/i915/gvt/render.c
> @@ -147,29 +147,20 @@ static void load_mocs(struct intel_vgpu *vgpu, int 
> ring_id)
>  {
>   struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv;
>   i915_reg_t offset, l3_offset;
> - u32 regs[] = {
> - [RCS] = 0xc800,
> - [VCS] = 0xc900,
> - [VCS2] = 0xca00,
> - [BCS] = 0xcc00,
> - [VECS] = 0xcb00,
> - };
>   int i;
>  
> - if (WARN_ON(ring_id >= ARRAY_SIZE(regs)))
> - return;
> -
>   if (!IS_SKYLAKE(dev_priv))
>   return;
>  
>   for (i = 0; i < 64; i++) {
> + offset.reg = i * 4;
>   gen9_render_mocs[ring_id][i] = I915_READ(offset);
>   I915_WRITE(offset, vgpu_vreg(vgpu, offset));
>   POSTING_READ(offset);
> - offset.reg += 4;
>   }
>  
>   if (ring_id == RCS) {
> + offset.reg = 64 * 4;
>   l3_offset.reg = 0xb020;
>   for (i = 0; i < 32; i++) {
>   gen9_render_mocs_L3[i] = I915_READ(l3_offset);
> @@ -184,26 +175,16 @@ static void restore_mocs(struct intel_vgpu *vgpu, int 
> ring_id)
>  {
>   struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv;
>   i915_reg_t offset, l3_offset;
> - u32 regs[] = {
> - [RCS] = 0xc800,
> - [VCS] = 0xc900,
> - [VCS2] = 0xca00,
> - [BCS] = 0xcc00,
> - [VECS] = 0xcb00,
> - };
>   int i;
>  
> - if (WARN_ON(ring_id >= ARRAY_SIZE(regs)))
> - return;
> -
>   if (!IS_SKYLAKE(dev_priv))
>   return;
>  
>   for (i = 0; i < 64; i++) {
> + offset.reg = i * 4;
>   vgpu_vreg(vgpu, offset) = I915_READ(offset);
>   I915_WRITE(offset, gen9_render_mocs[ring_id][i]);
>   POSTING_READ(offset);
> - offset.reg += 4;
>   }
>  
>   if (ring_id == RCS) {
> -- 
> 2.9.0
> 

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [PATCH 1/2] drm/i915/gvt: add ACPI and 64BIT dependencies

[Zhenyu Wang]

On 2016.10.21 17:25:49 +0200, Arnd Bergmann wrote:
> The newly added gvt code produces lots of serious warnings and errors
> when either built on 32-bit x86, or built with ACPI disabled, e.g.
> 
> drivers/gpu/drm/i915/gvt/gtt.c: In function ???read_pte64???:
> drivers/gpu/drm/i915/gvt/gtt.c:277:2: error: left shift count >= width of 
> type [-Werror]
> drivers/gpu/drm/i915/gvt/gtt.c: In function ???gen8_gtt_get_pfn???:
> drivers/gpu/drm/i915/gvt/gtt.c:360:3: error: left shift count >= width of 
> type [-Werror]
> drivers/gpu/drm/i915/gvt/opregion.c: In function 
> ???intel_gvt_init_opregion???:
> drivers/gpu/drm/i915/gvt/opregion.c:183:2: error: implicit declaration of 
> function ???acpi_os_ioremap??? [-Werror=implicit-function-declaration]
> 
> This avoids the problems by simply disallowing those configurations
> in Kconfig. I'm sure it's possible to make the code more portable
> and support building GVT without those options, but it might not be
> useful to do so.
> 
> Fixes: 4d60c5fd3f87 ("drm/i915/gvt: vGPU PCI configuration space 
> virtualization")
> Signed-off-by: Arnd Bergmann 
> ---
> If the code is meant to work on 32-bit and non-ACPI kernels, please
> treat this as a bug report and disregard the patch.
> ---

Thanks, Arnd. We have to depend on 64bit now and not require for ACPI,
as we used one acpi function for opregion mem map which is not necessary,
so I queued one 64bit dependence and another to remove acpi dependence for 
Daniel.

>  drivers/gpu/drm/i915/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
> index 6d4194288d11..1b9308284dde 100644
> --- a/drivers/gpu/drm/i915/Kconfig
> +++ b/drivers/gpu/drm/i915/Kconfig
> @@ -84,6 +84,7 @@ config DRM_I915_USERPTR
>  config DRM_I915_GVT
>  bool "Enable Intel GVT-g graphics virtualization host support"
>  depends on DRM_I915
> + depends on 64BIT && ACPI
>  default n
>  help
> Choose this option if you want to enable Intel GVT-g graphics
> -- 
> 2.9.0
> 

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [PATCH 1/2] drm/i915/gvt: add ACPI and 64BIT dependencies

[Zhenyu Wang]

On 2016.10.21 17:25:49 +0200, Arnd Bergmann wrote:
> The newly added gvt code produces lots of serious warnings and errors
> when either built on 32-bit x86, or built with ACPI disabled, e.g.
> 
> drivers/gpu/drm/i915/gvt/gtt.c: In function ???read_pte64???:
> drivers/gpu/drm/i915/gvt/gtt.c:277:2: error: left shift count >= width of 
> type [-Werror]
> drivers/gpu/drm/i915/gvt/gtt.c: In function ???gen8_gtt_get_pfn???:
> drivers/gpu/drm/i915/gvt/gtt.c:360:3: error: left shift count >= width of 
> type [-Werror]
> drivers/gpu/drm/i915/gvt/opregion.c: In function 
> ???intel_gvt_init_opregion???:
> drivers/gpu/drm/i915/gvt/opregion.c:183:2: error: implicit declaration of 
> function ???acpi_os_ioremap??? [-Werror=implicit-function-declaration]
> 
> This avoids the problems by simply disallowing those configurations
> in Kconfig. I'm sure it's possible to make the code more portable
> and support building GVT without those options, but it might not be
> useful to do so.
> 
> Fixes: 4d60c5fd3f87 ("drm/i915/gvt: vGPU PCI configuration space 
> virtualization")
> Signed-off-by: Arnd Bergmann 
> ---
> If the code is meant to work on 32-bit and non-ACPI kernels, please
> treat this as a bug report and disregard the patch.
> ---

Thanks, Arnd. We have to depend on 64bit now and not require for ACPI,
as we used one acpi function for opregion mem map which is not necessary,
so I queued one 64bit dependence and another to remove acpi dependence for 
Daniel.

>  drivers/gpu/drm/i915/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
> index 6d4194288d11..1b9308284dde 100644
> --- a/drivers/gpu/drm/i915/Kconfig
> +++ b/drivers/gpu/drm/i915/Kconfig
> @@ -84,6 +84,7 @@ config DRM_I915_USERPTR
>  config DRM_I915_GVT
>  bool "Enable Intel GVT-g graphics virtualization host support"
>  depends on DRM_I915
> + depends on 64BIT && ACPI
>  default n
>  help
> Choose this option if you want to enable Intel GVT-g graphics
> -- 
> 2.9.0
> 

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [Intel-gfx] linux-next: Tree for Oct 20 (gpu/drm/i915)

[Zhenyu Wang]

On 2016.10.20 21:25:03 +0300, Jani Nikula wrote:
> On Thu, 20 Oct 2016, Daniel Vetter  wrote:
> > On Thu, Oct 20, 2016 at 7:37 PM, Randy Dunlap  wrote:
> >> On 10/19/16 20:20, Stephen Rothwell wrote:
> >>> Hi all,
> >>>
> >>> Changes since 20161019:
> >>>
> >>
> >> on i386: when CONFIG_ACPI is not enabled:
> >
> > Adding Zhenyu. Might be good to have a fix just for this that I
> > directly pick up, since I want to tag the first 4.10 pull for Dave
> > Airlie this w/e.
> 
> How about just this?
>

I'd like to not depend on acpi function any more, so just change that
for memremap. GVT-g driver currently only supports 64BIT kernel so will
fix that dependence. I'll send fix pull for Daniel later.

thanks

> diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
> index 6aedc96aa412..94914381e8ef 100644
> --- a/drivers/gpu/drm/i915/Kconfig
> +++ b/drivers/gpu/drm/i915/Kconfig
> @@ -85,7 +85,7 @@ config DRM_I915_USERPTR
>  
>  config DRM_I915_GVT
>  bool "Enable Intel GVT-g graphics virtualization host support"
> -depends on DRM_I915
> +depends on DRM_I915 && ACPI
>  default n
>  help
> Choose this option if you want to enable Intel GVT-g graphics
> 
> 
> 
> > -Daniel
> >
> >> ../drivers/gpu/drm/i915/gvt/opregion.c: In function 
> >> 'intel_gvt_init_opregion':
> >> ../drivers/gpu/drm/i915/gvt/opregion.c:183:2: error: implicit declaration 
> >> of function 'acpi_os_ioremap' [-Werror=implicit-function-declaration]
> >>   gvt->opregion.opregion_va = acpi_os_ioremap(gvt->opregion.opregion_pa,
> >>   ^
> >> ../drivers/gpu/drm/i915/gvt/opregion.c:183:28: warning: assignment makes 
> >> pointer from integer without a cast [enabled by default]
> >>   gvt->opregion.opregion_va = acpi_os_ioremap(gvt->opregion.opregion_pa,
> >> ^
> >> ../drivers/gpu/drm/i915/gvt/gtt.c: In function 'read_pte64':
> >> ../drivers/gpu/drm/i915/gvt/gtt.c:277:2: warning: left shift count >= 
> >> width of type [enabled by default]
> >>   pte |= ioread32(addr + 4) << 32;
> >>   ^
> >> ../drivers/gpu/drm/i915/gvt/gtt.c: In function 'gen8_gtt_get_pfn':
> >> ../drivers/gpu/drm/i915/gvt/gtt.c:360:3: warning: left shift count >= 
> >> width of type [enabled by default]
> >>pfn = (e->val64 & ADDR_4K_MASK) >> 12;
> >>^
> >> ../drivers/gpu/drm/i915/gvt/gtt.c: In function 'gen8_gtt_set_pfn':
> >> ../drivers/gpu/drm/i915/gvt/gtt.c:373:3: warning: left shift count >= 
> >> width of type [enabled by default]
> >>e->val64 &= ~ADDR_4K_MASK;
> >>^
> >> ../drivers/gpu/drm/i915/gvt/gtt.c:374:3: warning: left shift count >= 
> >> width of type [enabled by default]
> >>pfn &= (ADDR_4K_MASK >> 12);
> >>^
> >> ../drivers/gpu/drm/i915/gvt/gtt.c: In function 'gen8_gma_to_pml4_index':
> >> ../drivers/gpu/drm/i915/gvt/gtt.c:436:1: warning: right shift count >= 
> >> width of type [enabled by default]
> >>  DEFINE_PPGTT_GMA_TO_INDEX(gen8, pml4, (gma >> 39 & 0x1ff));
> >>  ^
> >>   CC  drivers/gpu/drm/radeon/si_smc.o
> >> In file included from ../drivers/gpu/drm/i915/i915_drv.h:46:0,
> >>  from ../drivers/gpu/drm/i915/gvt/gtt.c:36:
> >> ../drivers/gpu/drm/i915/gvt/gtt.c: In function 
> >> 'intel_gvt_create_scratch_page':
> >> ../drivers/gpu/drm/i915/gvt/gtt.c:1945:47: warning: cast from pointer to 
> >> integer of different size [-Wpointer-to-int-cast]
> >>gvt_err("fail to translate vaddr:0x%llx\n", (u64)vaddr);
> >>^
> >> ../include/drm/drmP.h:201:43: note: in definition of macro 'DRM_ERROR'
> >>   drm_printk(KERN_ERR, DRM_UT_NONE, fmt, ##__VA_ARGS__)
> >>^
> >> ../drivers/gpu/drm/i915/gvt/gtt.c:1945:3: note: in expansion of macro 
> >> 'gvt_err'
> >>gvt_err("fail to translate vaddr:0x%llx\n", (u64)vaddr);
> >>^
> >>
> >>
> >>
> >> --
> >> ~Randy
> >> ___
> >> Intel-gfx mailing list
> >> intel-...@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [Intel-gfx] linux-next: Tree for Oct 20 (gpu/drm/i915)

[Zhenyu Wang]

On 2016.10.20 21:25:03 +0300, Jani Nikula wrote:
> On Thu, 20 Oct 2016, Daniel Vetter  wrote:
> > On Thu, Oct 20, 2016 at 7:37 PM, Randy Dunlap  wrote:
> >> On 10/19/16 20:20, Stephen Rothwell wrote:
> >>> Hi all,
> >>>
> >>> Changes since 20161019:
> >>>
> >>
> >> on i386: when CONFIG_ACPI is not enabled:
> >
> > Adding Zhenyu. Might be good to have a fix just for this that I
> > directly pick up, since I want to tag the first 4.10 pull for Dave
> > Airlie this w/e.
> 
> How about just this?
>

I'd like to not depend on acpi function any more, so just change that
for memremap. GVT-g driver currently only supports 64BIT kernel so will
fix that dependence. I'll send fix pull for Daniel later.

thanks

> diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
> index 6aedc96aa412..94914381e8ef 100644
> --- a/drivers/gpu/drm/i915/Kconfig
> +++ b/drivers/gpu/drm/i915/Kconfig
> @@ -85,7 +85,7 @@ config DRM_I915_USERPTR
>  
>  config DRM_I915_GVT
>  bool "Enable Intel GVT-g graphics virtualization host support"
> -depends on DRM_I915
> +depends on DRM_I915 && ACPI
>  default n
>  help
> Choose this option if you want to enable Intel GVT-g graphics
> 
> 
> 
> > -Daniel
> >
> >> ../drivers/gpu/drm/i915/gvt/opregion.c: In function 
> >> 'intel_gvt_init_opregion':
> >> ../drivers/gpu/drm/i915/gvt/opregion.c:183:2: error: implicit declaration 
> >> of function 'acpi_os_ioremap' [-Werror=implicit-function-declaration]
> >>   gvt->opregion.opregion_va = acpi_os_ioremap(gvt->opregion.opregion_pa,
> >>   ^
> >> ../drivers/gpu/drm/i915/gvt/opregion.c:183:28: warning: assignment makes 
> >> pointer from integer without a cast [enabled by default]
> >>   gvt->opregion.opregion_va = acpi_os_ioremap(gvt->opregion.opregion_pa,
> >> ^
> >> ../drivers/gpu/drm/i915/gvt/gtt.c: In function 'read_pte64':
> >> ../drivers/gpu/drm/i915/gvt/gtt.c:277:2: warning: left shift count >= 
> >> width of type [enabled by default]
> >>   pte |= ioread32(addr + 4) << 32;
> >>   ^
> >> ../drivers/gpu/drm/i915/gvt/gtt.c: In function 'gen8_gtt_get_pfn':
> >> ../drivers/gpu/drm/i915/gvt/gtt.c:360:3: warning: left shift count >= 
> >> width of type [enabled by default]
> >>pfn = (e->val64 & ADDR_4K_MASK) >> 12;
> >>^
> >> ../drivers/gpu/drm/i915/gvt/gtt.c: In function 'gen8_gtt_set_pfn':
> >> ../drivers/gpu/drm/i915/gvt/gtt.c:373:3: warning: left shift count >= 
> >> width of type [enabled by default]
> >>e->val64 &= ~ADDR_4K_MASK;
> >>^
> >> ../drivers/gpu/drm/i915/gvt/gtt.c:374:3: warning: left shift count >= 
> >> width of type [enabled by default]
> >>pfn &= (ADDR_4K_MASK >> 12);
> >>^
> >> ../drivers/gpu/drm/i915/gvt/gtt.c: In function 'gen8_gma_to_pml4_index':
> >> ../drivers/gpu/drm/i915/gvt/gtt.c:436:1: warning: right shift count >= 
> >> width of type [enabled by default]
> >>  DEFINE_PPGTT_GMA_TO_INDEX(gen8, pml4, (gma >> 39 & 0x1ff));
> >>  ^
> >>   CC  drivers/gpu/drm/radeon/si_smc.o
> >> In file included from ../drivers/gpu/drm/i915/i915_drv.h:46:0,
> >>  from ../drivers/gpu/drm/i915/gvt/gtt.c:36:
> >> ../drivers/gpu/drm/i915/gvt/gtt.c: In function 
> >> 'intel_gvt_create_scratch_page':
> >> ../drivers/gpu/drm/i915/gvt/gtt.c:1945:47: warning: cast from pointer to 
> >> integer of different size [-Wpointer-to-int-cast]
> >>gvt_err("fail to translate vaddr:0x%llx\n", (u64)vaddr);
> >>^
> >> ../include/drm/drmP.h:201:43: note: in definition of macro 'DRM_ERROR'
> >>   drm_printk(KERN_ERR, DRM_UT_NONE, fmt, ##__VA_ARGS__)
> >>^
> >> ../drivers/gpu/drm/i915/gvt/gtt.c:1945:3: note: in expansion of macro 
> >> 'gvt_err'
> >>gvt_err("fail to translate vaddr:0x%llx\n", (u64)vaddr);
> >>^
> >>
> >>
> >>
> >> --
> >> ~Randy
> >> ___
> >> Intel-gfx mailing list
> >> intel-...@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [PATCH -next] drm/i915/gvt: fix return value check

[Zhenyu Wang]

On 2016.10.19 16:18:03 +, Wei Yongjun wrote:
> From: Wei Yongjun 
> 
> In case of error, the function i915_gem_object_create() returns
> ERR_PTR() not NULL. The NULL test in the return value check should
> be replaced with IS_ERR().
> 
> Signed-off-by: Wei Yongjun 

Hi, Yongjun, we've already had this fixed in our queue for next pull request,
will send very soon.

Thanks.

> ---
>  drivers/gpu/drm/i915/gvt/cmd_parser.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/cmd_parser.c 
> b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> index 5808ee7..6abb2a6 100644
> --- a/drivers/gpu/drm/i915/gvt/cmd_parser.c
> +++ b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> @@ -1640,8 +1640,8 @@ static int perform_bb_shadow(struct parser_exec_state 
> *s)
>  
>   entry_obj->obj = i915_gem_object_create(&(s->vgpu->gvt->dev_priv->drm),
>   round_up(bb_size, PAGE_SIZE));
> - if (entry_obj->obj == NULL)
> - return -ENOMEM;
> + if (IS_ERR(entry_obj->obj))
> + return PTR_ERR(entry_obj->obj);
>   entry_obj->len = bb_size;
>   INIT_LIST_HEAD(_obj->list);
>  
> @@ -2712,8 +2712,8 @@ static int shadow_indirect_ctx(struct 
> intel_shadow_wa_ctx *wa_ctx)
>  
>   wa_ctx->indirect_ctx.obj = i915_gem_object_create(dev,
>   round_up(ctx_size + CACHELINE_BYTES, PAGE_SIZE));
> - if (wa_ctx->indirect_ctx.obj == NULL)
> - return -ENOMEM;
> + if (IS_ERR(wa_ctx->indirect_ctx.obj))
> + return PTR_ERR(wa_ctx->indirect_ctx.obj);
>  
>   ret = i915_gem_object_get_pages(wa_ctx->indirect_ctx.obj);
>   if (ret)
> 

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [PATCH -next] drm/i915/gvt: fix return value check

[Zhenyu Wang]

On 2016.10.19 16:18:03 +, Wei Yongjun wrote:
> From: Wei Yongjun 
> 
> In case of error, the function i915_gem_object_create() returns
> ERR_PTR() not NULL. The NULL test in the return value check should
> be replaced with IS_ERR().
> 
> Signed-off-by: Wei Yongjun 

Hi, Yongjun, we've already had this fixed in our queue for next pull request,
will send very soon.

Thanks.

> ---
>  drivers/gpu/drm/i915/gvt/cmd_parser.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/cmd_parser.c 
> b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> index 5808ee7..6abb2a6 100644
> --- a/drivers/gpu/drm/i915/gvt/cmd_parser.c
> +++ b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> @@ -1640,8 +1640,8 @@ static int perform_bb_shadow(struct parser_exec_state 
> *s)
>  
>   entry_obj->obj = i915_gem_object_create(&(s->vgpu->gvt->dev_priv->drm),
>   round_up(bb_size, PAGE_SIZE));
> - if (entry_obj->obj == NULL)
> - return -ENOMEM;
> + if (IS_ERR(entry_obj->obj))
> + return PTR_ERR(entry_obj->obj);
>   entry_obj->len = bb_size;
>   INIT_LIST_HEAD(_obj->list);
>  
> @@ -2712,8 +2712,8 @@ static int shadow_indirect_ctx(struct 
> intel_shadow_wa_ctx *wa_ctx)
>  
>   wa_ctx->indirect_ctx.obj = i915_gem_object_create(dev,
>   round_up(ctx_size + CACHELINE_BYTES, PAGE_SIZE));
> - if (wa_ctx->indirect_ctx.obj == NULL)
> - return -ENOMEM;
> + if (IS_ERR(wa_ctx->indirect_ctx.obj))
> + return PTR_ERR(wa_ctx->indirect_ctx.obj);
>  
>   ret = i915_gem_object_get_pages(wa_ctx->indirect_ctx.obj);
>   if (ret)
> 

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

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

[Zhenyu Wang]

On 2016.10.19 10:57:53 +1100, Stephen Rothwell wrote:
> Hi all,
> 
> After merging the drm-intel tree, today's linux-next build (x86_64
> allmodconfig) produced these warnings:
> 
> drivers/gpu/drm/i915/gvt/execlist.c: In function 
> 'release_shadow_batch_buffer':
> drivers/gpu/drm/i915/gvt/execlist.c:501:4: warning: 
> 'drm_gem_object_unreference' is deprecated [-Wdeprecated-declarations]
> drm_gem_object_unreference(&(entry_obj->obj->base));
> ^
> In file included from drivers/gpu/drm/i915/gvt/execlist.c:35:0:
> drivers/gpu/drm/i915/i915_drv.h:2344:13: note: declared here
>  extern void drm_gem_object_unreference(struct drm_gem_object *);
>  ^
> drivers/gpu/drm/i915/gvt/execlist.c: In function 'release_shadow_wa_ctx':
> drivers/gpu/drm/i915/gvt/execlist.c:514:2: warning: 
> 'drm_gem_object_unreference' is deprecated [-Wdeprecated-declarations]
>   drm_gem_object_unreference(&(wa_ctx->indirect_ctx.obj->base));
>   ^
> In file included from drivers/gpu/drm/i915/gvt/execlist.c:35:0:
> drivers/gpu/drm/i915/i915_drv.h:2344:13: note: declared here
>  extern void drm_gem_object_unreference(struct drm_gem_object *);
>  ^
> 
> Introduced by commit
> 
>   be1da7070aea ("drm/i915/gvt: vGPU command scanner")
> 
> interacting with commit
> 
>   f8c417cdb1b8 ("drm/i915: Rename drm_gem_object_unreference in preparation 
> for lockless free")
> 
> from Linus' tree.
> 

Sorry for this, we will fix that in next pull request to replace old interface.

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

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

[Zhenyu Wang]

On 2016.10.19 10:57:53 +1100, Stephen Rothwell wrote:
> Hi all,
> 
> After merging the drm-intel tree, today's linux-next build (x86_64
> allmodconfig) produced these warnings:
> 
> drivers/gpu/drm/i915/gvt/execlist.c: In function 
> 'release_shadow_batch_buffer':
> drivers/gpu/drm/i915/gvt/execlist.c:501:4: warning: 
> 'drm_gem_object_unreference' is deprecated [-Wdeprecated-declarations]
> drm_gem_object_unreference(&(entry_obj->obj->base));
> ^
> In file included from drivers/gpu/drm/i915/gvt/execlist.c:35:0:
> drivers/gpu/drm/i915/i915_drv.h:2344:13: note: declared here
>  extern void drm_gem_object_unreference(struct drm_gem_object *);
>  ^
> drivers/gpu/drm/i915/gvt/execlist.c: In function 'release_shadow_wa_ctx':
> drivers/gpu/drm/i915/gvt/execlist.c:514:2: warning: 
> 'drm_gem_object_unreference' is deprecated [-Wdeprecated-declarations]
>   drm_gem_object_unreference(&(wa_ctx->indirect_ctx.obj->base));
>   ^
> In file included from drivers/gpu/drm/i915/gvt/execlist.c:35:0:
> drivers/gpu/drm/i915/i915_drv.h:2344:13: note: declared here
>  extern void drm_gem_object_unreference(struct drm_gem_object *);
>  ^
> 
> Introduced by commit
> 
>   be1da7070aea ("drm/i915/gvt: vGPU command scanner")
> 
> interacting with commit
> 
>   f8c417cdb1b8 ("drm/i915: Rename drm_gem_object_unreference in preparation 
> for lockless free")
> 
> from Linus' tree.
> 

Sorry for this, we will fix that in next pull request to replace old interface.

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [RFC 0/6] Non perf based Gen Graphics OA unit driver

[Zhenyu Wang]

On 2015.09.29 15:39:03 +0100, Robert Bragg wrote:
> 
> - Logistically it might be more practical to contain this to the
>   graphics stack.
> 
> It seems fair to consider that if we can't see a very compelling
> benefit to building on perf, then containing this work to
> drivers/gpu/drm/i915 may simplify the review process as well as
> future maintenance and development.
> 

I think even we all initially like to go with perf but it appears later
that we might need to stick this more close with i915 driver. Also think
about to enable global profiling for all graphics clients, extending or
enabling it within i915 specific interface seems more feasible instead of
trying to create another PMU driver like previous implementation attempt
to suit the need for different gfx perf data definition.

Robert, thanks for send and elaborate on this.

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: Digital signature

Re: [RFC 0/6] Non perf based Gen Graphics OA unit driver

[Zhenyu Wang]

On 2015.09.29 15:39:03 +0100, Robert Bragg wrote:
> 
> - Logistically it might be more practical to contain this to the
>   graphics stack.
> 
> It seems fair to consider that if we can't see a very compelling
> benefit to building on perf, then containing this work to
> drivers/gpu/drm/i915 may simplify the review process as well as
> future maintenance and development.
> 

I think even we all initially like to go with perf but it appears later
that we might need to stick this more close with i915 driver. Also think
about to enable global profiling for all graphics clients, extending or
enabling it within i915 specific interface seems more feasible instead of
trying to create another PMU driver like previous implementation attempt
to suit the need for different gfx perf data definition.

Robert, thanks for send and elaborate on this.

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: Digital signature

Re: [Intel-gfx] [RFC][PATCH] gpu:drm:i915:intel_detect_pch: back to check devfn instead of check class type

[Zhenyu Wang]

On 2014.06.19 17:53:51 +0800, Tiejun Chen wrote:
> Originally the reason to probe ISA bridge instead of Dev31:Fun0
> is to make graphics device passthrough work easy for VMM, that
> only need to expose ISA bridge to let driver know the real
> hardware underneath. This is a requirement from virtualization
> team. Especially in that virtualized environments, XEN, there
> is irrelevant ISA bridge in the system with that legacy qemu
> version specific to xen, qemu-xen-traditional. So to work
> reliably, we should scan through all the ISA bridge devices
> and check for the first match, instead of only checking the
> first one.
> 
> But actually, qemu-xen-traditional, is always enumerated with
> Dev31:Fun0, 00:1f.0 as follows:
> 
> hw/pt-graphics.c:
> 
> intel_pch_init()
> |
> + pci_isa_bridge_init(bus, PCI_DEVFN(0x1f, 0), ...);
> 
> so this mean that isa bridge is still represented with Dev31:Func0
> like the native OS. Furthermore, currently we're pushing VGA
> passthrough support into qemu upstream, and with some discussion,
> we wouldn't set the bridge class type and just expose this devfn.
> 
> So we just go back to check devfn to make life normal.
> 
> Signed-off-by: Tiejun Chen 

This was added historically when supporting graphics device passthrough.
Looks qemu upstream can't accept multiple ISA bridge and our PCH is always
on device 31: func0 as far as I know. Looks good to me.

Reviewed-by: Zhenyu Wang  

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: Digital signature

Re: [Intel-gfx] [RFC][PATCH] gpu:drm:i915:intel_detect_pch: back to check devfn instead of check class type

[Zhenyu Wang]

On 2014.06.19 17:53:51 +0800, Tiejun Chen wrote:
 Originally the reason to probe ISA bridge instead of Dev31:Fun0
 is to make graphics device passthrough work easy for VMM, that
 only need to expose ISA bridge to let driver know the real
 hardware underneath. This is a requirement from virtualization
 team. Especially in that virtualized environments, XEN, there
 is irrelevant ISA bridge in the system with that legacy qemu
 version specific to xen, qemu-xen-traditional. So to work
 reliably, we should scan through all the ISA bridge devices
 and check for the first match, instead of only checking the
 first one.
 
 But actually, qemu-xen-traditional, is always enumerated with
 Dev31:Fun0, 00:1f.0 as follows:
 
 hw/pt-graphics.c:
 
 intel_pch_init()
 |
 + pci_isa_bridge_init(bus, PCI_DEVFN(0x1f, 0), ...);
 
 so this mean that isa bridge is still represented with Dev31:Func0
 like the native OS. Furthermore, currently we're pushing VGA
 passthrough support into qemu upstream, and with some discussion,
 we wouldn't set the bridge class type and just expose this devfn.
 
 So we just go back to check devfn to make life normal.
 
 Signed-off-by: Tiejun Chen tiejun.c...@intel.com

This was added historically when supporting graphics device passthrough.
Looks qemu upstream can't accept multiple ISA bridge and our PCH is always
on device 31: func0 as far as I know. Looks good to me.

Reviewed-by: Zhenyu Wang zhen...@linux.intel.com 

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: Digital signature

Re: [agp-mm][PATCH 1/4][intel_iommu] explicit export current graphics dmar status

[Zhenyu Wang]

On 2008.01.25 10:08:20 -0800, mark gross wrote:
> > This is used for our graphics driver module to know if we have to
> > do dma remapping in iommu case, both in kernel config and kernel
> > boot time param config. 
> 
> ok. How soon do you need this export?
> 

As graphics part of these patches depend on this one, it should go
first, or along with agpgart patches, that should first be in Airlie's
agp-mm queue.

> send me your current patch and eta for the graphics module patches so I
> can better coordinate with you.

You can find my current patches against agp-mm tree (so equally against
current -mm kernel) here, http://people.freedesktop.org/~zhen/, fixed
with some warnings from checkpatch.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/