Re: [PATCH v6 1/3] i915/gvt: Introduce the mmio table to support VFIO new mdev API

[Jani Nikula]

On Tue, 08 Feb 2022, Zhi Wang <zhi.wang.linux@xxxxxxxxx> wrote:
> From: Zhi Wang <zhi.wang.linux@xxxxxxxxx>
>
> 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
> initialization path has to be separated into two phases:
>
> a) Early initialization.
>
> The early initialization of GVT requires to be done when loading i915.
> Mostly it's because the initial clean HW state needs to be saved before
> i915 touches the HW.
>
> b) Late initalization.
>
> This phases of initalization will setup the rest components of GVT-g,
> which can be done later when the dedicated module is being loaded.

What's the baseline for this series?

>
> 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 <hch@xxxxxx>
> Cc: Jason Gunthorpe <jgg@xxxxxxxxxx>
> Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> Cc: Vivi Rodrigo <rodrigo.vivi@xxxxxxxxx>
> Cc: Zhenyu Wang <zhenyuw@xxxxxxxxxxxxxxx>
> Cc: Zhi Wang <zhi.a.wang@xxxxxxxxx>
> Tested-by: Terrence Xu <terrence.xu@xxxxxxxxx>
> Signed-off-by: Zhi Wang <zhi.wang.linux@xxxxxxxxx>
> ---
>  drivers/gpu/drm/i915/Makefile               |    2 +-
>  drivers/gpu/drm/i915/gvt/cmd_parser.c       |    2 +-
>  drivers/gpu/drm/i915/gvt/gvt.h              |    3 +-
>  drivers/gpu/drm/i915/gvt/handlers.c         | 1062 ++-------------
>  drivers/gpu/drm/i915/gvt/mmio.h             |   17 -
>  drivers/gpu/drm/i915/gvt/reg.h              |    9 +-
>  drivers/gpu/drm/i915/intel_gvt.c            |   20 +-
>  drivers/gpu/drm/i915/intel_gvt.h            |   37 +
>  drivers/gpu/drm/i915/intel_gvt_mmio_table.c | 1308 +++++++++++++++++++
>  9 files changed, 1501 insertions(+), 959 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/intel_gvt_mmio_table.c
>

> diff --git a/drivers/gpu/drm/i915/intel_gvt.h b/drivers/gpu/drm/i915/intel_gvt.h
> index d7d3fb6186fd..6d3031f3ac25 100644
> --- a/drivers/gpu/drm/i915/intel_gvt.h
> +++ b/drivers/gpu/drm/i915/intel_gvt.h
> @@ -26,7 +26,32 @@
>  
>  struct drm_i915_private;
>  
> +#include <linux/kernel.h>

Please use minimal includes. Looks like linux/types.h is enough. Please
also put the includes before the forward declarations.

> +
>  #ifdef CONFIG_DRM_I915_GVT
> +
> +#define D_BDW   (1 << 0)
> +#define D_SKL	(1 << 1)
> +#define D_KBL	(1 << 2)
> +#define D_BXT	(1 << 3)
> +#define D_CFL	(1 << 4)
> +
> +#define D_GEN9PLUS	(D_SKL | D_KBL | D_BXT | D_CFL)
> +#define D_GEN8PLUS	(D_BDW | D_SKL | D_KBL | D_BXT | D_CFL)
> +
> +#define D_SKL_PLUS	(D_SKL | D_KBL | D_BXT | D_CFL)
> +#define D_BDW_PLUS	(D_BDW | D_SKL | D_KBL | D_BXT | D_CFL)
> +
> +#define D_PRE_SKL	(D_BDW)
> +#define D_ALL		(D_BDW | D_SKL | D_KBL | D_BXT | D_CFL)

If these really need to be in a header in i915/, I think they need to be
longer with some namespacing or something. I do wish these could be
hidden though.

> +
> +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 device, u32 size);
> +};

We're heavily transitioning towards having a corresponding .h for each
.c instead of catch all headers. It's still a work in progress, but I'd
prefer having the declarations for stuff in intel_gvt_mmio_table.c
placed in intel_gvt_mmio_table.h, and named accordingly. Like I
suggested in my previous mails.

> +
>  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 +59,8 @@ 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);
> +unsigned long intel_gvt_get_device_type(struct drm_i915_private *i915);
> +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 +78,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;
> +}
> +
> +int intel_gvt_iterate_mmio_table(struct intel_gvt_mmio_table_iter *iter)
> +{
> +	return 0;
> +}

Stubs need to be static inlines.

>  #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 000000000000..b9de72e939d3
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_gvt_mmio_table.c
> @@ -0,0 +1,1308 @@
> +/*
> + * Copyright(c) 2021 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.

Please use SPDX headers for new files.


BR,
Jani.

-- 
Jani Nikula, Intel Open Source Graphics Center