Re: [Intel-gfx] [PATCH v6 2/3] i915/gvt: Save the initial HW state snapshot in i915

2022-02-09 Thread Wang, Zhi A
On 2/9/22 7:32 AM, Christoph Hellwig wrote:
> On Tue, Feb 08, 2022 at 06:11:50AM -0500, Zhi Wang wrote:
>> +struct drm_i915_private *dev_priv = iter->i915;
>> +u32 *mmio, i;
>> +
>> +for (i = offset; i < offset + size; i += 4) {
>> +mmio = iter->data + i;
>> +*mmio = intel_uncore_read_notrace(to_gt(dev_priv)->uncore,
>> +  _MMIO(i));
> 
> This reads much stranger than:
> 
>   u32 *mmio = iter->data;
> 
>   for (i = offset; i < offset + size; i += 4) {
>   mmio[i] = intel_uncore_read_notrace(to_gt(dev_priv)->uncore,
>   _MMIO(i));
>   }
> 
I am not sure the suggestion is correct. That's the reason why I didn't take 
the comments in the previous review.

if mmio is u32 *, the step of mmio[0] -> mmio[1] will be 4, and i will be 
increased by 4 in each loop.
I guess the correct one would be mmio[i/4] = x; would that looks better? if 
yes, I will do that in the next version.

>> +static int handle_mmio(struct intel_gvt_mmio_table_iter *iter,
>> +   u32 offset, u32 device, u32 size)
>> +{
>> +if (WARN_ON(!IS_ALIGNED(offset, 4)))
>> +return -EINVAL;
> 
> Shouldn't this be in the caller of the method?
> 
>> +save_mmio(iter, offset, size);
>> +return 0;
> 
Yes. You are right. It's because I get rid of the mmio_block in intel_gvt.c

> Now that the block callback is gone save_mmio and handle_mmio
> can be merged.
> 
>> +mem = vzalloc(2 * SZ_1M);
> 
> Don't we want a driver-wide constant for this instead of a magic number?
> 

We actually have one in i915, but it's not exported. Should we export that one?

Thanks,
Zhi.


Re: [Intel-gfx] [PATCH v6 2/3] i915/gvt: Save the initial HW state snapshot in i915

2022-02-08 Thread Christoph Hellwig
On Tue, Feb 08, 2022 at 06:11:50AM -0500, Zhi Wang wrote:
> + struct drm_i915_private *dev_priv = iter->i915;
> + u32 *mmio, i;
> +
> + for (i = offset; i < offset + size; i += 4) {
> + mmio = iter->data + i;
> + *mmio = intel_uncore_read_notrace(to_gt(dev_priv)->uncore,
> +   _MMIO(i));

This reads much stranger than:

u32 *mmio = iter->data;

for (i = offset; i < offset + size; i += 4) {
mmio[i] = intel_uncore_read_notrace(to_gt(dev_priv)->uncore,
_MMIO(i));
}

> +static int handle_mmio(struct intel_gvt_mmio_table_iter *iter,
> +u32 offset, u32 device, u32 size)
> +{
> + if (WARN_ON(!IS_ALIGNED(offset, 4)))
> + return -EINVAL;

Shouldn't this be in the caller of the method?

> + save_mmio(iter, offset, size);
> + return 0;

Now that the block callback is gone save_mmio and handle_mmio
can be merged.

> + mem = vzalloc(2 * SZ_1M);

Don't we want a driver-wide constant for this instead of a magic number?


[Intel-gfx] [PATCH v6 2/3] i915/gvt: Save the initial HW state snapshot in i915

2022-02-08 Thread Zhi Wang
Save the initial HW state snapshot in i915 so that the rest code of GVT-g
can be moved into a dedicated module while it can still get a clean
initial HW state saved at the correct time during the initialization of
i915. The futhrer vGPU created by GVT-g will use this HW state as the
initial HW state.

v6:

- Remove the reference of intel_gvt_device_info.(Christoph)
- Refine the save_mmio() function. (Christoph)

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/i915_drv.h  |  2 +
 drivers/gpu/drm/i915/intel_gvt.c | 92 +++-
 2 files changed, 92 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 44c1f98144b4..2a230840cdfa 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -606,6 +606,8 @@ struct i915_virtual_gpu {
struct mutex lock; /* serialises sending of g2v_notify command pkts */
bool active;
u32 caps;
+   u32 *initial_mmio;
+   u8 *initial_cfg_space;
 };
 
 struct i915_selftest_stash {
diff --git a/drivers/gpu/drm/i915/intel_gvt.c b/drivers/gpu/drm/i915/intel_gvt.c
index 32aa6f111d6b..c5019173ac44 100644
--- a/drivers/gpu/drm/i915/intel_gvt.c
+++ b/drivers/gpu/drm/i915/intel_gvt.c
@@ -68,6 +68,85 @@ void intel_gvt_sanitize_options(struct drm_i915_private 
*dev_priv)
dev_priv->params.enable_gvt = 0;
 }
 
+static void free_initial_hw_state(struct drm_i915_private *dev_priv)
+{
+   struct i915_virtual_gpu *vgpu = _priv->vgpu;
+
+   vfree(vgpu->initial_mmio);
+   vgpu->initial_mmio = NULL;
+
+   kfree(vgpu->initial_cfg_space);
+   vgpu->initial_cfg_space = NULL;
+}
+
+static void save_mmio(struct intel_gvt_mmio_table_iter *iter, u32 offset,
+ u32 size)
+{
+   struct drm_i915_private *dev_priv = iter->i915;
+   u32 *mmio, i;
+
+   for (i = offset; i < offset + size; i += 4) {
+   mmio = iter->data + i;
+   *mmio = intel_uncore_read_notrace(to_gt(dev_priv)->uncore,
+ _MMIO(i));
+   }
+}
+
+static int handle_mmio(struct intel_gvt_mmio_table_iter *iter,
+  u32 offset, u32 device, u32 size)
+{
+   if (WARN_ON(!IS_ALIGNED(offset, 4)))
+   return -EINVAL;
+
+   save_mmio(iter, offset, size);
+   return 0;
+}
+
+static int save_initial_hw_state(struct drm_i915_private *dev_priv)
+{
+   struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
+   struct i915_virtual_gpu *vgpu = _priv->vgpu;
+   struct intel_gvt_mmio_table_iter iter;
+   void *mem;
+   int i, ret;
+
+   mem = kzalloc(PCI_CFG_SPACE_EXP_SIZE, GFP_KERNEL);
+   if (!mem)
+   return -ENOMEM;
+
+   vgpu->initial_cfg_space = mem;
+
+   for (i = 0; i < PCI_CFG_SPACE_EXP_SIZE; i += 4)
+   pci_read_config_dword(pdev, i, mem + i);
+
+   mem = vzalloc(2 * SZ_1M);
+   if (!mem) {
+   ret = -ENOMEM;
+   goto err_mmio;
+   }
+
+   vgpu->initial_mmio = mem;
+
+   iter.i915 = dev_priv;
+   iter.data = vgpu->initial_mmio;
+   iter.handle_mmio_cb = handle_mmio;
+
+   ret = intel_gvt_iterate_mmio_table();
+   if (ret)
+   goto err_iterate;
+
+   return 0;
+
+err_iterate:
+   vfree(vgpu->initial_mmio);
+   vgpu->initial_mmio = NULL;
+err_mmio:
+   kfree(vgpu->initial_cfg_space);
+   vgpu->initial_cfg_space = NULL;
+
+   return ret;
+}
+
 /**
  * intel_gvt_init - initialize GVT components
  * @dev_priv: drm i915 private data
@@ -97,15 +176,23 @@ int intel_gvt_init(struct drm_i915_private *dev_priv)
return -EIO;
}
 
+   ret = save_initial_hw_state(dev_priv);
+   if (ret) {
+   drm_dbg(_priv->drm, "Fail to save initial HW state\n");
+   goto err_save_hw_state;
+   }
+
ret = intel_gvt_init_device(dev_priv);
if (ret) {
drm_dbg(_priv->drm, "Fail to init GVT device\n");
-   goto bail;
+   goto err_init_device;
}
 
return 0;
 
-bail:
+err_init_device:
+   free_initial_hw_state(dev_priv);
+err_save_hw_state:
dev_priv->params.enable_gvt = 0;
return 0;
 }
@@ -129,6 +216,7 @@ void intel_gvt_driver_remove(struct drm_i915_private 
*dev_priv)
return;
 
intel_gvt_clean_device(dev_priv);
+   free_initial_hw_state(dev_priv);
 }
 
 /**
-- 
2.25.1