Re: [Intel-gfx] [PATCH v9 1/3] i915/gvt: Separate the MMIO tracking table from GVT-g

2022-08-08 Thread Alex Williamson 
On Thu,  7 Apr 2022 03:19:43 -0400
Zhi Wang  wrote:

> From: Zhi Wang 
> 
> To support the new mdev interfaces and the re-factor patches from
> Christoph, which moves the GVT-g code into a dedicated module, the GVT-g
> MMIO tracking table needs to be separated from GVT-g.
> 

Since this commit I'm unable to make use of GVT-g on a Xeon W-1290 IGD.
The following in dmesg is the first sign of trouble:

[ cut here ]
assign a handler to a non-tracked mmio 4ab8
WARNING: CPU: 16 PID: 504 at drivers/gpu/drm/i915/gvt/handlers.c:123 
setup_mmio_info.constprop.0+0xd1/0xf0 [i915]
ixgbe :02:00.0: 31.504 Gb/s available PCIe bandwidth (8.0 GT/s PCIe x4 link)
Modules linked in: nouveau(+) i915(+) mdev vfio_iommu_type1 vfio prime_numbers 
intel_gtt ast drm_buddy mxm_wmi drm_dp_helper drm_vram_helper drm_ttm_helper 
drm_kms_helper sd_mod t10_pi syscopyarea sysfillrect sysimgblt sg fb_sys_fops 
cec ttm crct10dif_pclmul drm ixgbe(+) crc32_pclmul igb crc32c_intel ahci e1000e 
libahci mdio libata ghash_clmulni_intel i2c_algo_bit dca wmi video 
pinctrl_cannonlake rndis_host cdc_ether usbnet mii dm_mirror dm_region_hash 
dm_log dm_mod fuse
ixgbe :02:00.0: MAC: 4, PHY: 0, PBA No: 020C08-0F8
CPU: 16 PID: 504 Comm: systemd-udevd Not tainted 5.18.0-rc1+ #16
ixgbe :02:00.0: 3c:ec:ef:27:ef:0e
Hardware name: Supermicro Super Server/X12SCZ-F, BIOS 1.0 06/16/2020
RIP: 0010:setup_mmio_info.constprop.0+0xd1/0xf0 [i915]
Code: 83 c6 04 81 ef e4 e6 dd 78 39 f5 77 a2 31 c0 48 83 c4 08 5b 5d 41 5c 41 
5d 41 5e 41 5f c3 48 c7 c7 d8 c1 07 c1 e8 fe 83 cc c9 <0f> 0b 48 83 c4 08 b8 ed 
ff ff ff 5b 5d 41 5c 41 5d 41 5e 41 5f c3
RSP: 0018:a2014090fa28 EFLAGS: 00010282
RAX:  RBX: 934754a38000 RCX: 
RDX: 9346ce42c740 RSI: 9346ce41fca0 RDI: 9346ce41fca0
RBP: 4abc R08:  R09: 7fff
R10: a2014090f868 R11: 8bfe65e8 R12: 
R13:  R14: 0008 R15: 
FS:  7f345634d540() GS:9346ce40() knlGS:
CS:  0010 DS:  ES:  CR0: 80050033
CR2: 562eacfa8980 CR3: 002058ac0001 CR4: 007706e0
PKRU: 5554
Call Trace:
 
 init_skl_mmio_info+0x1532/0x15a0 [i915]
 intel_gvt_setup_mmio_info+0x1aa/0x240 [i915]
 ? gen9_dbuf_ctl_mmio_write+0x40/0x40 [i915]
 intel_gvt_init_device+0x106/0x300 [i915]
 intel_gvt_init+0x41/0xa0 [i915]
 i915_driver_hw_probe+0x2b2/0x340 [i915]
 i915_driver_probe+0x1fd/0x570 [i915]
 ? drm_privacy_screen_get+0x15f/0x190 [drm]
 i915_pci_probe+0x56/0x1e0 [i915]
 local_pci_probe+0x42/0x80
 pci_call_probe+0x56/0x160
 pci_device_probe+0x75/0xf0
 ? driver_sysfs_add+0x6f/0xd0
 really_probe+0x199/0x380
ixgbe :02:00.0: Intel(R) 10 Gigabit Network Connection
 __driver_probe_device+0xfe/0x180
 driver_probe_device+0x1e/0x90
 __driver_attach+0xc0/0x1c0
 ? __device_attach_driver+0xe0/0xe0
 ? __device_attach_driver+0xe0/0xe0
 bus_for_each_dev+0x75/0xc0
 bus_add_driver+0x149/0x1e0
 driver_register+0x8f/0xe0
 i915_init+0x1d/0x7c [i915]
 ? 0xc0768000
 do_one_initcall+0x41/0x200
 ? kmem_cache_alloc_trace+0x174/0x2f0
 do_init_module+0x4c/0x250
 __do_sys_finit_module+0xb4/0x120
 do_syscall_64+0x59/0x80
 ? syscall_exit_to_user_mode+0x12/0x30
 ? do_syscall_64+0x69/0x80
 ? do_syscall_64+0x69/0x80
 ? do_syscall_64+0x69/0x80
 ? sysvec_call_function+0x3c/0x90
 entry_SYSCALL_64_after_hwframe+0x44/0xae
RIP: 0033:0x7f3456e5a3ed
Code: ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 48 89 f8 48 89 f7 48 
89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 
c3 48 8b 0d 03 aa 1b 00 f7 d8 64 89 01 48
RSP: 002b:7fff4df686b8 EFLAGS: 0246 ORIG_RAX: 0139
RAX: ffda RBX: 562eacf62920 RCX: 7f3456e5a3ed
RDX:  RSI: 562eacf886b0 RDI: 001a
RBP: 0002 R08:  R09: 0002
R10: 001a R11: 0246 R12: 562eacf886b0
R13: 562eacf627c0 R14:  R15: 562eacf8d2d0
 
---[ end trace  ]---


> diff --git a/drivers/gpu/drm/i915/gvt/handlers.c 
> b/drivers/gpu/drm/i915/gvt/handlers.c
> index 520a7e1942f3..9bd3c15bfab6 100644
> --- a/drivers/gpu/drm/i915/gvt/handlers.c
> +++ b/drivers/gpu/drm/i915/gvt/handlers.c
...
> @@ -3440,7 +2729,6 @@ static int init_skl_mmio_info(struct intel_gvt *gvt)
>NULL, NULL);
>  
>   MMIO_DFH(GAMT_CHKN_BIT_REG, D_KBL | D_CFL, F_CMD_ACCESS, NULL, NULL);
> - MMIO_D(GEN9_CTX_PREEMPT_REG, D_SKL_PLUS & ~D_BXT);
>   MMIO_DFH(_MMIO(0xe4cc), D_BDW_PLUS, F_CMD_ACCESS, NULL, NULL);
>  
>   return 0;

I tracked this to the above code segment, where the untouched line
provides this mmio address:

#define GAMT_CHKN_BIT_REG   _MMIO(0x4ab8)

If I comment out setup of this register, GVT-g appears to initialize
and I can list available mdev types.  I haven't tried assignment of the
resulting

Re: [Intel-gfx] [PATCH v9 1/3] i915/gvt: Separate the MMIO tracking table from GVT-g

2022-04-13 Thread Jani Nikula 
On Wed, 13 Apr 2022, "Wang, Zhi A"  wrote:
> Hi Jani:
>
> Previously when I sent a pull request, it will be top of a tag in 
> drm-intel-next. The following patches move the DMC related registers, which 
> is used by GVT-g into intel_dmc_regs.h and it is not included in the latest 
> tag. 
>
> commit 9c67d9e84c7d4a3a2371a54ee2dddc4699002000
> Author: Jani Nikula 
> Date:   Wed Mar 30 14:34:17 2022 +0300
>
> drm/i915/dmc: split out dmc registers to a separate file
> 
> Clean up the massive i915_reg.h a bit with this isolated set of
> registers.
> 
> v2: Remove stale comment (Lucas)
> 
> Signed-off-by: Jani Nikula 
> Reviewed-by: Lucas De Marchi 
> Link: 
> https://patchwork.freedesktop.org/patch/msgid/20220330113417.220964-3-jani.nik...@intel.com
>
> If I still sent the pull request based on the latest tag, after the pull got 
> merged, the compiling of the GVT-g module will be broken, as a new header 
> needs to be included.
>
> Can I sent my pull request not based on a tag in drm-intel-next, just the 
> latest drm-intel-next?

Yes.

I think you can also backmerge drm-intel-next to your branch whenever
there's a dependency you need, or otherwise want to sync up. (Please do
*not* backmerge drm-intel-gt-next or drm-next directly.)


BR,
Jani.

>
> Thanks,
> Zhi.
>
> On 4/13/22 9:31 AM, Jani Nikula wrote:
>> On Fri, 08 Apr 2022, "Wang, Zhi A"  wrote:
>>> Hi Jani:
>>>
>>> Thanks so much for the help. Can you generate a new tag on
>>> drm-intel-next? I noticed that there was one patch moving the DMC
>>> related registers into display/intel_dmc_regs.h, which is not included
>>> in the latest tag on drm-intel-next.
>> 
>> Sorry, I'm not sure what you're asking exactly. We do tags when we
>> create pull requests for drm-next, but I don't see the connection to
>> gvt.
>> 
>> BR,
>> Jani.
>> 
>>>
>>> Guess it would be better that I can change this patch according to it
>>> when checking in. This would prevent a conflict in future.
>>>
>>> Thanks,
>>> Zhi.
>>>
>>> On 4/7/22 3:03 PM, Jani Nikula wrote:
 On Thu, 07 Apr 2022, Zhi Wang  wrote:
> diff --git a/drivers/gpu/drm/i915/intel_gvt.h 
> b/drivers/gpu/drm/i915/intel_gvt.h
> index d7d3fb6186fd..7665d7cf0bdd 100644
> --- a/drivers/gpu/drm/i915/intel_gvt.h
> +++ b/drivers/gpu/drm/i915/intel_gvt.h
> @@ -26,7 +26,17 @@
>  
>  struct drm_i915_private;
>  
> +#include 

 You only need . Please add it before the forward
 declaration above.

> +
>  #ifdef CONFIG_DRM_I915_GVT
> +
> +struct intel_gvt_mmio_table_iter {
> + struct drm_i915_private *i915;
> + void *data;
> + int (*handle_mmio_cb)(struct intel_gvt_mmio_table_iter *iter,
> +   u32 offset, u32 size);
> +};
> +
>  int intel_gvt_init(struct drm_i915_private *dev_priv);
>  void intel_gvt_driver_remove(struct drm_i915_private *dev_priv);
>  int intel_gvt_init_device(struct drm_i915_private *dev_priv);
> @@ -34,6 +44,7 @@ void intel_gvt_clean_device(struct drm_i915_private 
> *dev_priv);
>  int intel_gvt_init_host(void);
>  void intel_gvt_sanitize_options(struct drm_i915_private *dev_priv);
>  void intel_gvt_resume(struct drm_i915_private *dev_priv);
> +int intel_gvt_iterate_mmio_table(struct intel_gvt_mmio_table_iter *iter);
>  #else
>  static inline int intel_gvt_init(struct drm_i915_private *dev_priv)
>  {
> @@ -51,6 +62,16 @@ static inline void intel_gvt_sanitize_options(struct 
> drm_i915_private *dev_priv)
>  static inline void intel_gvt_resume(struct drm_i915_private *dev_priv)
>  {
>  }
> +
> +unsigned long intel_gvt_get_device_type(struct drm_i915_private *i915)
> +{
> + return 0;
> +}

 The CONFIG_DRM_I915_GVT=y counterpart for this is in mmio.h. Should be
 both in the same header.

> +
> +int intel_gvt_iterate_mmio_table(struct intel_gvt_mmio_table_iter *iter)
> +{
> + return 0;
> +}
>  #endif
>  
>  #endif /* _INTEL_GVT_H_ */
> diff --git a/drivers/gpu/drm/i915/intel_gvt_mmio_table.c 
> b/drivers/gpu/drm/i915/intel_gvt_mmio_table.c
> new file mode 100644
> index ..d29491a6d209
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_gvt_mmio_table.c
> @@ -0,0 +1,1290 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2020 Intel Corporation
> + */
> +
> +#include "i915_drv.h"
> +#include "i915_reg.h"
> +#include "display/vlv_dsi_pll_regs.h"
> +#include "gt/intel_gt_regs.h"
> +#include "intel_mchbar_regs.h"
> +#include "i915_pvinfo.h"
> +#include "intel_gvt.h"
> +#include "gvt/gvt.h"

 Generally we have the include lists sorted.

 Other than the nitpicks above, the series is

 Acked-by: Jani Nikula 


 BR,
 Jani.


>>>
>> 
>

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [PATCH v9 1/3] i915/gvt: Separate the MMIO tracking table from GVT-g

2022-04-13 Thread Wang, Zhi A 
Hi Jani:

Previously when I sent a pull request, it will be top of a tag in 
drm-intel-next. The following patches move the DMC related registers, which is 
used by GVT-g into intel_dmc_regs.h and it is not included in the latest tag. 

commit 9c67d9e84c7d4a3a2371a54ee2dddc4699002000
Author: Jani Nikula 
Date:   Wed Mar 30 14:34:17 2022 +0300

drm/i915/dmc: split out dmc registers to a separate file

Clean up the massive i915_reg.h a bit with this isolated set of
registers.

v2: Remove stale comment (Lucas)

Signed-off-by: Jani Nikula 
Reviewed-by: Lucas De Marchi 
Link: 
https://patchwork.freedesktop.org/patch/msgid/20220330113417.220964-3-jani.nik...@intel.com

If I still sent the pull request based on the latest tag, after the pull got 
merged, the compiling of the GVT-g module will be broken, as a new header needs 
to be included.

Can I sent my pull request not based on a tag in drm-intel-next, just the 
latest drm-intel-next?

Thanks,
Zhi.

On 4/13/22 9:31 AM, Jani Nikula wrote:
> On Fri, 08 Apr 2022, "Wang, Zhi A"  wrote:
>> Hi Jani:
>>
>> Thanks so much for the help. Can you generate a new tag on
>> drm-intel-next? I noticed that there was one patch moving the DMC
>> related registers into display/intel_dmc_regs.h, which is not included
>> in the latest tag on drm-intel-next.
> 
> Sorry, I'm not sure what you're asking exactly. We do tags when we
> create pull requests for drm-next, but I don't see the connection to
> gvt.
> 
> BR,
> Jani.
> 
>>
>> Guess it would be better that I can change this patch according to it
>> when checking in. This would prevent a conflict in future.
>>
>> Thanks,
>> Zhi.
>>
>> On 4/7/22 3:03 PM, Jani Nikula wrote:
>>> On Thu, 07 Apr 2022, Zhi Wang  wrote:
 diff --git a/drivers/gpu/drm/i915/intel_gvt.h 
 b/drivers/gpu/drm/i915/intel_gvt.h
 index d7d3fb6186fd..7665d7cf0bdd 100644
 --- a/drivers/gpu/drm/i915/intel_gvt.h
 +++ b/drivers/gpu/drm/i915/intel_gvt.h
 @@ -26,7 +26,17 @@
  
  struct drm_i915_private;
  
 +#include 
>>>
>>> You only need . Please add it before the forward
>>> declaration above.
>>>
 +
  #ifdef CONFIG_DRM_I915_GVT
 +
 +struct intel_gvt_mmio_table_iter {
 +  struct drm_i915_private *i915;
 +  void *data;
 +  int (*handle_mmio_cb)(struct intel_gvt_mmio_table_iter *iter,
 +u32 offset, u32 size);
 +};
 +
  int intel_gvt_init(struct drm_i915_private *dev_priv);
  void intel_gvt_driver_remove(struct drm_i915_private *dev_priv);
  int intel_gvt_init_device(struct drm_i915_private *dev_priv);
 @@ -34,6 +44,7 @@ void intel_gvt_clean_device(struct drm_i915_private 
 *dev_priv);
  int intel_gvt_init_host(void);
  void intel_gvt_sanitize_options(struct drm_i915_private *dev_priv);
  void intel_gvt_resume(struct drm_i915_private *dev_priv);
 +int intel_gvt_iterate_mmio_table(struct intel_gvt_mmio_table_iter *iter);
  #else
  static inline int intel_gvt_init(struct drm_i915_private *dev_priv)
  {
 @@ -51,6 +62,16 @@ static inline void intel_gvt_sanitize_options(struct 
 drm_i915_private *dev_priv)
  static inline void intel_gvt_resume(struct drm_i915_private *dev_priv)
  {
  }
 +
 +unsigned long intel_gvt_get_device_type(struct drm_i915_private *i915)
 +{
 +  return 0;
 +}
>>>
>>> The CONFIG_DRM_I915_GVT=y counterpart for this is in mmio.h. Should be
>>> both in the same header.
>>>
 +
 +int intel_gvt_iterate_mmio_table(struct intel_gvt_mmio_table_iter *iter)
 +{
 +  return 0;
 +}
  #endif
  
  #endif /* _INTEL_GVT_H_ */
 diff --git a/drivers/gpu/drm/i915/intel_gvt_mmio_table.c 
 b/drivers/gpu/drm/i915/intel_gvt_mmio_table.c
 new file mode 100644
 index ..d29491a6d209
 --- /dev/null
 +++ b/drivers/gpu/drm/i915/intel_gvt_mmio_table.c
 @@ -0,0 +1,1290 @@
 +// SPDX-License-Identifier: MIT
 +/*
 + * Copyright © 2020 Intel Corporation
 + */
 +
 +#include "i915_drv.h"
 +#include "i915_reg.h"
 +#include "display/vlv_dsi_pll_regs.h"
 +#include "gt/intel_gt_regs.h"
 +#include "intel_mchbar_regs.h"
 +#include "i915_pvinfo.h"
 +#include "intel_gvt.h"
 +#include "gvt/gvt.h"
>>>
>>> Generally we have the include lists sorted.
>>>
>>> Other than the nitpicks above, the series is
>>>
>>> Acked-by: Jani Nikula 
>>>
>>>
>>> BR,
>>> Jani.
>>>
>>>
>>
>

Re: [Intel-gfx] [PATCH v9 1/3] i915/gvt: Separate the MMIO tracking table from GVT-g

2022-04-13 Thread Jani Nikula 
On Fri, 08 Apr 2022, "Wang, Zhi A"  wrote:
> Hi Jani:
>
> Thanks so much for the help. Can you generate a new tag on
> drm-intel-next? I noticed that there was one patch moving the DMC
> related registers into display/intel_dmc_regs.h, which is not included
> in the latest tag on drm-intel-next.

Sorry, I'm not sure what you're asking exactly. We do tags when we
create pull requests for drm-next, but I don't see the connection to
gvt.

BR,
Jani.

>
> Guess it would be better that I can change this patch according to it
> when checking in. This would prevent a conflict in future.
>
> Thanks,
> Zhi.
>
> On 4/7/22 3:03 PM, Jani Nikula wrote:
>> On Thu, 07 Apr 2022, Zhi Wang  wrote:
>>> diff --git a/drivers/gpu/drm/i915/intel_gvt.h 
>>> b/drivers/gpu/drm/i915/intel_gvt.h
>>> index d7d3fb6186fd..7665d7cf0bdd 100644
>>> --- a/drivers/gpu/drm/i915/intel_gvt.h
>>> +++ b/drivers/gpu/drm/i915/intel_gvt.h
>>> @@ -26,7 +26,17 @@
>>>  
>>>  struct drm_i915_private;
>>>  
>>> +#include 
>> 
>> You only need . Please add it before the forward
>> declaration above.
>> 
>>> +
>>>  #ifdef CONFIG_DRM_I915_GVT
>>> +
>>> +struct intel_gvt_mmio_table_iter {
>>> +   struct drm_i915_private *i915;
>>> +   void *data;
>>> +   int (*handle_mmio_cb)(struct intel_gvt_mmio_table_iter *iter,
>>> + u32 offset, u32 size);
>>> +};
>>> +
>>>  int intel_gvt_init(struct drm_i915_private *dev_priv);
>>>  void intel_gvt_driver_remove(struct drm_i915_private *dev_priv);
>>>  int intel_gvt_init_device(struct drm_i915_private *dev_priv);
>>> @@ -34,6 +44,7 @@ void intel_gvt_clean_device(struct drm_i915_private 
>>> *dev_priv);
>>>  int intel_gvt_init_host(void);
>>>  void intel_gvt_sanitize_options(struct drm_i915_private *dev_priv);
>>>  void intel_gvt_resume(struct drm_i915_private *dev_priv);
>>> +int intel_gvt_iterate_mmio_table(struct intel_gvt_mmio_table_iter *iter);
>>>  #else
>>>  static inline int intel_gvt_init(struct drm_i915_private *dev_priv)
>>>  {
>>> @@ -51,6 +62,16 @@ static inline void intel_gvt_sanitize_options(struct 
>>> drm_i915_private *dev_priv)
>>>  static inline void intel_gvt_resume(struct drm_i915_private *dev_priv)
>>>  {
>>>  }
>>> +
>>> +unsigned long intel_gvt_get_device_type(struct drm_i915_private *i915)
>>> +{
>>> +   return 0;
>>> +}
>> 
>> The CONFIG_DRM_I915_GVT=y counterpart for this is in mmio.h. Should be
>> both in the same header.
>> 
>>> +
>>> +int intel_gvt_iterate_mmio_table(struct intel_gvt_mmio_table_iter *iter)
>>> +{
>>> +   return 0;
>>> +}
>>>  #endif
>>>  
>>>  #endif /* _INTEL_GVT_H_ */
>>> diff --git a/drivers/gpu/drm/i915/intel_gvt_mmio_table.c 
>>> b/drivers/gpu/drm/i915/intel_gvt_mmio_table.c
>>> new file mode 100644
>>> index ..d29491a6d209
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/i915/intel_gvt_mmio_table.c
>>> @@ -0,0 +1,1290 @@
>>> +// SPDX-License-Identifier: MIT
>>> +/*
>>> + * Copyright © 2020 Intel Corporation
>>> + */
>>> +
>>> +#include "i915_drv.h"
>>> +#include "i915_reg.h"
>>> +#include "display/vlv_dsi_pll_regs.h"
>>> +#include "gt/intel_gt_regs.h"
>>> +#include "intel_mchbar_regs.h"
>>> +#include "i915_pvinfo.h"
>>> +#include "intel_gvt.h"
>>> +#include "gvt/gvt.h"
>> 
>> Generally we have the include lists sorted.
>> 
>> Other than the nitpicks above, the series is
>> 
>> Acked-by: Jani Nikula 
>> 
>> 
>> BR,
>> Jani.
>> 
>> 
>

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [PATCH v9 1/3] i915/gvt: Separate the MMIO tracking table from GVT-g

2022-04-11 Thread Wang, Zhi A 
Ping. :)

On 4/8/22 2:07 PM, Zhi Wang wrote:
> Hi Jani:
> 
> Thanks so much for the help. Can you generate a new tag on drm-intel-next? I 
> noticed that there was one patch moving the DMC related registers into 
> display/intel_dmc_regs.h, which is not included in the latest tag on 
> drm-intel-next.
> 
> Guess it would be better that I can change this patch according to it when 
> checking in. This would prevent a conflict in future.
> 
> Thanks,
> Zhi.
> 
> On 4/7/22 3:03 PM, Jani Nikula wrote:
>> On Thu, 07 Apr 2022, Zhi Wang  wrote:
>>> diff --git a/drivers/gpu/drm/i915/intel_gvt.h 
>>> b/drivers/gpu/drm/i915/intel_gvt.h
>>> index d7d3fb6186fd..7665d7cf0bdd 100644
>>> --- a/drivers/gpu/drm/i915/intel_gvt.h
>>> +++ b/drivers/gpu/drm/i915/intel_gvt.h
>>> @@ -26,7 +26,17 @@
>>>  
>>>  struct drm_i915_private;
>>>  
>>> +#include 
>>
>> You only need . Please add it before the forward
>> declaration above.
>>
>>> +
>>>  #ifdef CONFIG_DRM_I915_GVT
>>> +
>>> +struct intel_gvt_mmio_table_iter {
>>> +   struct drm_i915_private *i915;
>>> +   void *data;
>>> +   int (*handle_mmio_cb)(struct intel_gvt_mmio_table_iter *iter,
>>> + u32 offset, u32 size);
>>> +};
>>> +
>>>  int intel_gvt_init(struct drm_i915_private *dev_priv);
>>>  void intel_gvt_driver_remove(struct drm_i915_private *dev_priv);
>>>  int intel_gvt_init_device(struct drm_i915_private *dev_priv);
>>> @@ -34,6 +44,7 @@ void intel_gvt_clean_device(struct drm_i915_private 
>>> *dev_priv);
>>>  int intel_gvt_init_host(void);
>>>  void intel_gvt_sanitize_options(struct drm_i915_private *dev_priv);
>>>  void intel_gvt_resume(struct drm_i915_private *dev_priv);
>>> +int intel_gvt_iterate_mmio_table(struct intel_gvt_mmio_table_iter *iter);
>>>  #else
>>>  static inline int intel_gvt_init(struct drm_i915_private *dev_priv)
>>>  {
>>> @@ -51,6 +62,16 @@ static inline void intel_gvt_sanitize_options(struct 
>>> drm_i915_private *dev_priv)
>>>  static inline void intel_gvt_resume(struct drm_i915_private *dev_priv)
>>>  {
>>>  }
>>> +
>>> +unsigned long intel_gvt_get_device_type(struct drm_i915_private *i915)
>>> +{
>>> +   return 0;
>>> +}
>>
>> The CONFIG_DRM_I915_GVT=y counterpart for this is in mmio.h. Should be
>> both in the same header.
>>
>>> +
>>> +int intel_gvt_iterate_mmio_table(struct intel_gvt_mmio_table_iter *iter)
>>> +{
>>> +   return 0;
>>> +}
>>>  #endif
>>>  
>>>  #endif /* _INTEL_GVT_H_ */
>>> diff --git a/drivers/gpu/drm/i915/intel_gvt_mmio_table.c 
>>> b/drivers/gpu/drm/i915/intel_gvt_mmio_table.c
>>> new file mode 100644
>>> index ..d29491a6d209
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/i915/intel_gvt_mmio_table.c
>>> @@ -0,0 +1,1290 @@
>>> +// SPDX-License-Identifier: MIT
>>> +/*
>>> + * Copyright © 2020 Intel Corporation
>>> + */
>>> +
>>> +#include "i915_drv.h"
>>> +#include "i915_reg.h"
>>> +#include "display/vlv_dsi_pll_regs.h"
>>> +#include "gt/intel_gt_regs.h"
>>> +#include "intel_mchbar_regs.h"
>>> +#include "i915_pvinfo.h"
>>> +#include "intel_gvt.h"
>>> +#include "gvt/gvt.h"
>>
>> Generally we have the include lists sorted.
>>
>> Other than the nitpicks above, the series is
>>
>> Acked-by: Jani Nikula 
>>
>>
>> BR,
>> Jani.
>>
>>
>

Re: [Intel-gfx] [PATCH v9 1/3] i915/gvt: Separate the MMIO tracking table from GVT-g

2022-04-08 Thread Wang, Zhi A 
Hi Jani:

Thanks so much for the help. Can you generate a new tag on drm-intel-next? I 
noticed that there was one patch moving the DMC related registers into 
display/intel_dmc_regs.h, which is not included in the latest tag on 
drm-intel-next.

Guess it would be better that I can change this patch according to it when 
checking in. This would prevent a conflict in future.

Thanks,
Zhi.

On 4/7/22 3:03 PM, Jani Nikula wrote:
> On Thu, 07 Apr 2022, Zhi Wang  wrote:
>> diff --git a/drivers/gpu/drm/i915/intel_gvt.h 
>> b/drivers/gpu/drm/i915/intel_gvt.h
>> index d7d3fb6186fd..7665d7cf0bdd 100644
>> --- a/drivers/gpu/drm/i915/intel_gvt.h
>> +++ b/drivers/gpu/drm/i915/intel_gvt.h
>> @@ -26,7 +26,17 @@
>>  
>>  struct drm_i915_private;
>>  
>> +#include 
> 
> You only need . Please add it before the forward
> declaration above.
> 
>> +
>>  #ifdef CONFIG_DRM_I915_GVT
>> +
>> +struct intel_gvt_mmio_table_iter {
>> +struct drm_i915_private *i915;
>> +void *data;
>> +int (*handle_mmio_cb)(struct intel_gvt_mmio_table_iter *iter,
>> +  u32 offset, u32 size);
>> +};
>> +
>>  int intel_gvt_init(struct drm_i915_private *dev_priv);
>>  void intel_gvt_driver_remove(struct drm_i915_private *dev_priv);
>>  int intel_gvt_init_device(struct drm_i915_private *dev_priv);
>> @@ -34,6 +44,7 @@ void intel_gvt_clean_device(struct drm_i915_private 
>> *dev_priv);
>>  int intel_gvt_init_host(void);
>>  void intel_gvt_sanitize_options(struct drm_i915_private *dev_priv);
>>  void intel_gvt_resume(struct drm_i915_private *dev_priv);
>> +int intel_gvt_iterate_mmio_table(struct intel_gvt_mmio_table_iter *iter);
>>  #else
>>  static inline int intel_gvt_init(struct drm_i915_private *dev_priv)
>>  {
>> @@ -51,6 +62,16 @@ static inline void intel_gvt_sanitize_options(struct 
>> drm_i915_private *dev_priv)
>>  static inline void intel_gvt_resume(struct drm_i915_private *dev_priv)
>>  {
>>  }
>> +
>> +unsigned long intel_gvt_get_device_type(struct drm_i915_private *i915)
>> +{
>> +return 0;
>> +}
> 
> The CONFIG_DRM_I915_GVT=y counterpart for this is in mmio.h. Should be
> both in the same header.
> 
>> +
>> +int intel_gvt_iterate_mmio_table(struct intel_gvt_mmio_table_iter *iter)
>> +{
>> +return 0;
>> +}
>>  #endif
>>  
>>  #endif /* _INTEL_GVT_H_ */
>> diff --git a/drivers/gpu/drm/i915/intel_gvt_mmio_table.c 
>> b/drivers/gpu/drm/i915/intel_gvt_mmio_table.c
>> new file mode 100644
>> index ..d29491a6d209
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/intel_gvt_mmio_table.c
>> @@ -0,0 +1,1290 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>> + * Copyright © 2020 Intel Corporation
>> + */
>> +
>> +#include "i915_drv.h"
>> +#include "i915_reg.h"
>> +#include "display/vlv_dsi_pll_regs.h"
>> +#include "gt/intel_gt_regs.h"
>> +#include "intel_mchbar_regs.h"
>> +#include "i915_pvinfo.h"
>> +#include "intel_gvt.h"
>> +#include "gvt/gvt.h"
> 
> Generally we have the include lists sorted.
> 
> Other than the nitpicks above, the series is
> 
> Acked-by: Jani Nikula 
> 
> 
> BR,
> Jani.
> 
>

Re: [Intel-gfx] [PATCH v9 1/3] i915/gvt: Separate the MMIO tracking table from GVT-g

2022-04-07 Thread Jani Nikula 
On Thu, 07 Apr 2022, Zhi Wang  wrote:
> diff --git a/drivers/gpu/drm/i915/intel_gvt.h 
> b/drivers/gpu/drm/i915/intel_gvt.h
> index d7d3fb6186fd..7665d7cf0bdd 100644
> --- a/drivers/gpu/drm/i915/intel_gvt.h
> +++ b/drivers/gpu/drm/i915/intel_gvt.h
> @@ -26,7 +26,17 @@
>  
>  struct drm_i915_private;
>  
> +#include 

You only need . Please add it before the forward
declaration above.

> +
>  #ifdef CONFIG_DRM_I915_GVT
> +
> +struct intel_gvt_mmio_table_iter {
> + struct drm_i915_private *i915;
> + void *data;
> + int (*handle_mmio_cb)(struct intel_gvt_mmio_table_iter *iter,
> +   u32 offset, u32 size);
> +};
> +
>  int intel_gvt_init(struct drm_i915_private *dev_priv);
>  void intel_gvt_driver_remove(struct drm_i915_private *dev_priv);
>  int intel_gvt_init_device(struct drm_i915_private *dev_priv);
> @@ -34,6 +44,7 @@ void intel_gvt_clean_device(struct drm_i915_private 
> *dev_priv);
>  int intel_gvt_init_host(void);
>  void intel_gvt_sanitize_options(struct drm_i915_private *dev_priv);
>  void intel_gvt_resume(struct drm_i915_private *dev_priv);
> +int intel_gvt_iterate_mmio_table(struct intel_gvt_mmio_table_iter *iter);
>  #else
>  static inline int intel_gvt_init(struct drm_i915_private *dev_priv)
>  {
> @@ -51,6 +62,16 @@ static inline void intel_gvt_sanitize_options(struct 
> drm_i915_private *dev_priv)
>  static inline void intel_gvt_resume(struct drm_i915_private *dev_priv)
>  {
>  }
> +
> +unsigned long intel_gvt_get_device_type(struct drm_i915_private *i915)
> +{
> + return 0;
> +}

The CONFIG_DRM_I915_GVT=y counterpart for this is in mmio.h. Should be
both in the same header.

> +
> +int intel_gvt_iterate_mmio_table(struct intel_gvt_mmio_table_iter *iter)
> +{
> + return 0;
> +}
>  #endif
>  
>  #endif /* _INTEL_GVT_H_ */
> diff --git a/drivers/gpu/drm/i915/intel_gvt_mmio_table.c 
> b/drivers/gpu/drm/i915/intel_gvt_mmio_table.c
> new file mode 100644
> index ..d29491a6d209
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_gvt_mmio_table.c
> @@ -0,0 +1,1290 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2020 Intel Corporation
> + */
> +
> +#include "i915_drv.h"
> +#include "i915_reg.h"
> +#include "display/vlv_dsi_pll_regs.h"
> +#include "gt/intel_gt_regs.h"
> +#include "intel_mchbar_regs.h"
> +#include "i915_pvinfo.h"
> +#include "intel_gvt.h"
> +#include "gvt/gvt.h"

Generally we have the include lists sorted.

Other than the nitpicks above, the series is

Acked-by: Jani Nikula 


BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [PATCH v9 1/3] i915/gvt: Separate the MMIO tracking table from GVT-g

2022-04-07 Thread Zhenyu Wang 
On 2022.04.07 03:19:43 -0400, Zhi Wang wrote:
> From: Zhi Wang 
> 
> To support the new mdev interfaces and the re-factor patches from
> Christoph, which moves the GVT-g code into a dedicated module, the GVT-g
> MMIO tracking table needs to be separated from GVT-g.
>

Looks fine to me. Thanks!

Reviewed-by: Zhenyu Wang 


signature.asc
Description: PGP signature

Re: [Intel-gfx] [PATCH v9 1/3] i915/gvt: Separate the MMIO tracking table from GVT-g

2022-04-07 Thread Wang, Zhi A 
Thanks so much!

Jani and Joonas, it would be better to have one rb from i915 maintainers as 
this patches also modify i915 code.

Thanks,
Zhi.

On 4/7/22 8:06 AM, Christoph Hellwig wrote:
> The whole series looks good and works fine on by Kaby Lake Thinkpad:
> 
> Reviewed-by: Christoph Hellwig 
> Tested-by: Christoph Hellwig 
>

Re: [Intel-gfx] [PATCH v9 1/3] i915/gvt: Separate the MMIO tracking table from GVT-g

2022-04-07 Thread Christoph Hellwig 
The whole series looks good and works fine on by Kaby Lake Thinkpad:

Reviewed-by: Christoph Hellwig 
Tested-by: Christoph Hellwig

[Intel-gfx] [PATCH v9 1/3] i915/gvt: Separate the MMIO tracking table from GVT-g

2022-04-07 Thread Zhi Wang 
From: Zhi Wang 

To support the new mdev interfaces and the re-factor patches from
Christoph, which moves the GVT-g code into a dedicated module, the GVT-g
MMIO tracking table needs to be separated from GVT-g.

v9:
- Fix a problem might cause kernel panic.

v8:
- Use SPDX header in the intel_gvt_mmio_table.c
- Reference the gvt.h with path. (Jani)
- Add a missing fix on mmio emulation path during the debug.
- Fix a building problem on refreshed gvt-staging branch. (Christoph)

v7:
- Keep the marcos of device generation in GVT-g. (Christoph, Jani)

v6:
- Move the mmio_table.c into i915. (Christoph)
- Keep init_device_info and related structures in GVT-g. (Christoph)
- Refine the callbacks of the iterator. (Christoph)
- Move the flags of MMIO register defination to GVT-g. (Chrsitoph)
- Move the mmio block handling to GVT-g.

v5:
- Re-design the mmio table framework. (Christoph)

v4:
- Fix the errors of patch checking scripts.

v3:
- Fix the errors when CONFIG_DRM_I915_WERROR is turned on. (Jani)

v2:
- Implement a mmio table instead of generating it by marco in i915. (Jani)

Cc: Christoph Hellwig 
Cc: Jason Gunthorpe 
Cc: Jani Nikula 
Cc: Joonas Lahtinen 
Cc: Vivi Rodrigo 
Cc: Zhenyu Wang 
Cc: Zhi Wang 
Signed-off-by: Zhi Wang 
---
 drivers/gpu/drm/i915/Makefile   |2 +-
 drivers/gpu/drm/i915/gvt/gvt.h  |3 +-
 drivers/gpu/drm/i915/gvt/handlers.c | 1033 ++-
 drivers/gpu/drm/i915/gvt/mmio.h |1 -
 drivers/gpu/drm/i915/gvt/reg.h  |9 +-
 drivers/gpu/drm/i915/intel_gvt.h|   21 +
 drivers/gpu/drm/i915/intel_gvt_mmio_table.c | 1290 +++
 7 files changed, 1460 insertions(+), 899 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/intel_gvt_mmio_table.c

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 1a771ee5b1d0..b4abe0650ba1 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -321,7 +321,7 @@ i915-$(CONFIG_DRM_I915_SELFTEST) += \
 i915-y += i915_vgpu.o
 
 ifeq ($(CONFIG_DRM_I915_GVT),y)
-i915-y += intel_gvt.o
+i915-y += intel_gvt.o intel_gvt_mmio_table.o
 include $(src)/gvt/Makefile
 endif
 
diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
index 0ebffc327528..bfe07c69cfd2 100644
--- a/drivers/gpu/drm/i915/gvt/gvt.h
+++ b/drivers/gpu/drm/i915/gvt/gvt.h
@@ -36,6 +36,7 @@
 #include 
 
 #include "i915_drv.h"
+#include "intel_gvt.h"
 
 #include "debug.h"
 #include "hypercall.h"
@@ -272,7 +273,7 @@ struct intel_gvt_mmio {
 /* Value of command write of this reg needs to be patched */
 #define F_CMD_WRITE_PATCH  (1 << 8)
 
-   const struct gvt_mmio_block *mmio_block;
+   struct gvt_mmio_block *mmio_block;
unsigned int num_mmio_block;
 
DECLARE_HASHTABLE(mmio_info_table, INTEL_GVT_MMIO_HASH_BITS);
diff --git a/drivers/gpu/drm/i915/gvt/handlers.c 
b/drivers/gpu/drm/i915/gvt/handlers.c
index 520a7e1942f3..9bd3c15bfab6 100644
--- a/drivers/gpu/drm/i915/gvt/handlers.c
+++ b/drivers/gpu/drm/i915/gvt/handlers.c
@@ -101,12 +101,11 @@ struct intel_gvt_mmio_info 
*intel_gvt_find_mmio_info(struct intel_gvt *gvt,
return NULL;
 }
 
-static int new_mmio_info(struct intel_gvt *gvt,
-   u32 offset, u16 flags, u32 size,
-   u32 addr_mask, u32 ro_mask, u32 device,
-   gvt_mmio_func read, gvt_mmio_func write)
+static int setup_mmio_info(struct intel_gvt *gvt, u32 offset, u32 size,
+  u16 flags, u32 addr_mask, u32 ro_mask, u32 device,
+  gvt_mmio_func read, gvt_mmio_func write)
 {
-   struct intel_gvt_mmio_info *info, *p;
+   struct intel_gvt_mmio_info *p;
u32 start, end, i;
 
if (!intel_gvt_match_device(gvt, device))
@@ -119,32 +118,18 @@ static int new_mmio_info(struct intel_gvt *gvt,
end = offset + size;
 
for (i = start; i < end; i += 4) {
-   info = kzalloc(sizeof(*info), GFP_KERNEL);
-   if (!info)
-   return -ENOMEM;
-
-   info->offset = i;
-   p = intel_gvt_find_mmio_info(gvt, info->offset);
-   if (p) {
-   WARN(1, "dup mmio definition offset %x\n",
-   info->offset);
-   kfree(info);
-
-   /* We return -EEXIST here to make GVT-g load fail.
-* So duplicated MMIO can be found as soon as
-* possible.
-*/
-   return -EEXIST;
+   p = intel_gvt_find_mmio_info(gvt, i);
+   if (!p) {
+   WARN(1, "assign a handler to a non-tracked mmio %x\n",
+   i);
+   return -ENODEV;
}
-
-   info->ro_mask = ro_mask;
-   info->device = device;
-   info->read = read ? read : intel_vgpu_default_mmio_read;