On Sat, 05 Sep 2020, Xiaolin Zhang <xiaolin.zh...@intel.com> wrote:
> to enable vgpu pv feature, pv capability is introduced for guest by
> new pv_caps member in struct i915_virtual_gpu and for host GVT by
> new pv_caps register in struct vgt_if.
>
> both of them are used to control different pv feature support in each
> domain and the final pv caps runtime negotiated between guest and host.
>
> it also adds VGT_CAPS_PV capability BIT useb by guest to query host GVTg
> whether support any PV feature or not.
>
> Signed-off-by: Xiaolin Zhang <xiaolin.zh...@intel.com>
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 3 ++
> drivers/gpu/drm/i915/i915_drv.h | 1 +
> drivers/gpu/drm/i915/i915_pvinfo.h | 5 ++-
> drivers/gpu/drm/i915/i915_vgpu.c | 63
> ++++++++++++++++++++++++++++++++++++-
> drivers/gpu/drm/i915/i915_vgpu.h | 10 ++++++
> 5 files changed, 80 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 7842199..fd1e0fc 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -48,6 +48,7 @@
> #include "i915_trace.h"
> #include "intel_pm.h"
> #include "intel_sideband.h"
> +#include "i915_vgpu.h"
>
> static inline struct drm_i915_private *node_to_i915(struct drm_info_node
> *node)
> {
> @@ -60,6 +61,8 @@ static int i915_capabilities(struct seq_file *m, void *data)
> struct drm_printer p = drm_seq_file_printer(m);
>
> seq_printf(m, "pch: %d\n", INTEL_PCH_TYPE(i915));
> + if (intel_vgpu_active(i915))
> + seq_printf(m, "vgpu pv_caps: 0x%x\n", i915->vgpu.pv_caps);
I think the placement here over-emphasizes the importance of the
caps. Maybe you also want to print something if vgpu isn't active?
>
> intel_device_info_print_static(INTEL_INFO(i915), &p);
> intel_device_info_print_runtime(RUNTIME_INFO(i915), &p);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index a455752..16d1b51 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -808,6 +808,7 @@ struct i915_virtual_gpu {
> struct mutex lock; /* serialises sending of g2v_notify command pkts */
> bool active;
> u32 caps;
> + u32 pv_caps;
> };
>
> struct intel_cdclk_config {
> diff --git a/drivers/gpu/drm/i915/i915_pvinfo.h
> b/drivers/gpu/drm/i915/i915_pvinfo.h
> index 683e97a..8b0dc25 100644
> --- a/drivers/gpu/drm/i915/i915_pvinfo.h
> +++ b/drivers/gpu/drm/i915/i915_pvinfo.h
> @@ -57,6 +57,7 @@ enum vgt_g2v_type {
> #define VGT_CAPS_FULL_PPGTT BIT(2)
> #define VGT_CAPS_HWSP_EMULATION BIT(3)
> #define VGT_CAPS_HUGE_GTT BIT(4)
> +#define VGT_CAPS_PV BIT(5)
>
> struct vgt_if {
> u64 magic; /* VGT_MAGIC */
> @@ -109,7 +110,9 @@ struct vgt_if {
> u32 execlist_context_descriptor_lo;
> u32 execlist_context_descriptor_hi;
>
> - u32 rsv7[0x200 - 24]; /* pad to one page */
> + u32 pv_caps;
> +
> + u32 rsv7[0x200 - 25]; /* pad to one page */
> } __packed;
>
> #define vgtif_offset(x) (offsetof(struct vgt_if, x))
> diff --git a/drivers/gpu/drm/i915/i915_vgpu.c
> b/drivers/gpu/drm/i915/i915_vgpu.c
> index 70fca72..10960125 100644
> --- a/drivers/gpu/drm/i915/i915_vgpu.c
> +++ b/drivers/gpu/drm/i915/i915_vgpu.c
> @@ -98,7 +98,13 @@ void intel_vgpu_detect(struct drm_i915_private *dev_priv)
>
> dev_priv->vgpu.active = true;
> mutex_init(&dev_priv->vgpu.lock);
> - drm_info(&dev_priv->drm, "Virtual GPU for Intel GVT-g detected.\n");
> +
> + if (!intel_vgpu_detect_pv_caps(dev_priv, shared_area)) {
> + DRM_INFO("Virtual GPU for Intel GVT-g detected.\n");
> + goto out;
Seems clearer without the goto. It's not like one is an error path,
right?
> + }
> +
> + DRM_INFO("Virtual GPU for Intel GVT-g detected with PV Optimized.\n");
Please retain use of drm_info().
>
> out:
> pci_iounmap(pdev, shared_area);
> @@ -134,6 +140,18 @@ bool intel_vgpu_has_huge_gtt(struct drm_i915_private
> *dev_priv)
> return dev_priv->vgpu.caps & VGT_CAPS_HUGE_GTT;
> }
>
> +static bool intel_vgpu_check_pv_cap(struct drm_i915_private *dev_priv,
> + enum pv_caps cap)
The indentation is off here, and all over the place, as reported by
checkpatch. Please address them everywhere.
> +{
> + return (dev_priv->vgpu.active && (dev_priv->vgpu.caps & VGT_CAPS_PV)
> + && (dev_priv->vgpu.pv_caps & cap));
> +}
> +
> +static bool intel_vgpu_has_pv_caps(struct drm_i915_private *dev_priv)
> +{
> + return dev_priv->vgpu.caps & VGT_CAPS_PV;
> +}
> +
> struct _balloon_info_ {
> /*
> * There are up to 2 regions per mappable/unmappable graphic
> @@ -336,3 +354,46 @@ int intel_vgt_balloon(struct i915_ggtt *ggtt)
> drm_err(&dev_priv->drm, "VGT balloon fail\n");
> return ret;
> }
> +
> +/*
> + * i915 vgpu PV support for Linux
> + */
> +
> +/*
> + * Config vgpu PV ops for different PV capabilities
> + */
> +void intel_vgpu_config_pv_caps(struct drm_i915_private *i915,
> + enum pv_caps cap, void *data)
> +{
> +
> + if (!intel_vgpu_check_pv_cap(i915, cap))
> + return;
> +}
> +
> +/**
> + * intel_vgpu_detect_pv_caps - detect virtual GPU PV capabilities
> + * @dev_priv: i915 device private
If you use kernel-doc, please write proper kernel-doc comments. Again,
please see the report sent to you by our CI.
> + *
> + * This function is called at the initialization stage, to detect VGPU
> + * PV capabilities
> + */
> +bool intel_vgpu_detect_pv_caps(struct drm_i915_private *i915,
> + void __iomem *shared_area)
> +{
> + u32 gvt_pvcaps;
> + u32 pvcaps = 0;
> +
> + if (!intel_vgpu_has_pv_caps(i915))
> + return false;
> +
> + /* PV capability negotiation between PV guest and GVT */
> + gvt_pvcaps = readl(shared_area + vgtif_offset(pv_caps));
> + pvcaps = i915->vgpu.pv_caps & gvt_pvcaps;
> + i915->vgpu.pv_caps = pvcaps;
> + writel(pvcaps, shared_area + vgtif_offset(pv_caps));
> +
> + if (!pvcaps)
> + return false;
> +
> + return true;
> +}
> diff --git a/drivers/gpu/drm/i915/i915_vgpu.h
> b/drivers/gpu/drm/i915/i915_vgpu.h
> index ffbb77d..1b10175 100644
> --- a/drivers/gpu/drm/i915/i915_vgpu.h
> +++ b/drivers/gpu/drm/i915/i915_vgpu.h
> @@ -29,6 +29,11 @@
> struct drm_i915_private;
> struct i915_ggtt;
>
> +/* define different PV capabilities */
The comment adds nothing.
> +enum pv_caps {
Please prefix the type name and the enumerations with intel_ or
something.
> + PV_NONE = 0,
> +};
> +
> void intel_vgpu_detect(struct drm_i915_private *i915);
> bool intel_vgpu_active(struct drm_i915_private *i915);
> void intel_vgpu_register(struct drm_i915_private *i915);
> @@ -39,4 +44,9 @@ bool intel_vgpu_has_huge_gtt(struct drm_i915_private *i915);
> int intel_vgt_balloon(struct i915_ggtt *ggtt);
> void intel_vgt_deballoon(struct i915_ggtt *ggtt);
>
> +/* i915 vgpu pv related functions */
> +bool intel_vgpu_detect_pv_caps(struct drm_i915_private *i915,
> + void __iomem *shared_area);
> +void intel_vgpu_config_pv_caps(struct drm_i915_private *i915,
> + enum pv_caps cap, void *data);
> #endif /* _I915_VGPU_H_ */
--
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
