On Tue, 08 Feb 2022, Zhi Wang <[email protected]> wrote:
> From: Zhi Wang <[email protected]>
>
> 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 <[email protected]>
> Cc: Jason Gunthorpe <[email protected]>
> Cc: Jani Nikula <[email protected]>
> Cc: Joonas Lahtinen <[email protected]>
> Cc: Vivi Rodrigo <[email protected]>
> Cc: Zhenyu Wang <[email protected]>
> Cc: Zhi Wang <[email protected]>
> Tested-by: Terrence Xu <[email protected]>
> Signed-off-by: Zhi Wang <[email protected]>
> ---
> 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
On Tue, Feb 08, 2022 at 05:15:00PM +0200, Jani Nikula wrote:
> > #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.
I think we could actually kill them off entirely. They are used as
arguments to the macros that setup the mmio table.
Thefunctions to build these tabls are already organized by families,
so we'd need relatively few conditions to just build them the right
way. There also are some runtime checks in the callbacks, but they
seem entirely superflous as far as I can tell.
Only the cmd parser is a bit messy. So maybe we could keep these
constants just for the cmd parser inside of gvt for now (and clean
that up later) and remove them entirely from the mmio table.
On 2/9/22 7:28 AM, Christoph Hellwig wrote:
> On Tue, Feb 08, 2022 at 05:15:00PM +0200, Jani Nikula wrote:
>>> #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.
>
> I think we could actually kill them off entirely. They are used as
> arguments to the macros that setup the mmio table.
>
> Thefunctions to build these tabls are already organized by families,
> so we'd need relatively few conditions to just build them the right
> way. There also are some runtime checks in the callbacks, but they
> seem entirely superflous as far as I can tell.
>
> Only the cmd parser is a bit messy. So maybe we could keep these
> constants just for the cmd parser inside of gvt for now (and clean
> that up later) and remove them entirely from the mmio table.
>
I agree that's the correct way for not exporting this to i915 by just organizing them in the functions, like what you said.
But I guess it's also matter of time and schedule as well. If we go that direction, it might take longer time for coding as
this is a big re-factor. Also, we need our QA to do another full test run. That needs to be considered. (If we are ok with that)
Besides, we have to have a methodology to make sure everything is the same as before.
Currently I am comparing the numbers of tracked mmio and the mmio snapshot. It would be nice to have more insight. :)
Thanks,
Zhi.
Just curious, what is the state of this seris? It would be good to
have it ready early on for the next merge window as there is quite
a backlog that depends on it.