Skip to contentSkip to conversation

drm/i915/gvt: clean up intel_gvt.h as interface for i915 core

PATCH In review Zhenyu Wang 3 participants 3 messages Oct 20, 2016

Subsystems (3)
DRM drivers Intel i915 Intel GVT-g drivers (Intel GPU Virtualization)
lore
Share & tools
lore

Zhenyu Wang

author
#0 lore raw

PATCH v3 1/1 drm/i915/gvt: clean up intel_gvt.h as interface for i915 core

i915 core should only call functions and structures exposed through intel_gvt.h. Remove internal gvt.h and i915_pvinfo.h.

Change for internal intel_gvt structure as private handler which not requires to expose gvt internal structure for i915 core.

v2: Fix per Chris's comment
- carefully handle dev_priv->gvt assignment
- add necessary bracket for macro helper
- forward declartion struct intel_gvt
- keep free operation within same file handling alloc

v3: fix use after free and remove intel_gvt.initialized

Signed-off-by Zhenyu Wang <[email protected]>
---
 drivers/gpu/drm/i915/gvt/aperture_gm.c  |  1 +
 drivers/gpu/drm/i915/gvt/cfg_space.c    |  1 +
 drivers/gpu/drm/i915/gvt/cmd_parser.c   |  2 ++
 drivers/gpu/drm/i915/gvt/display.c      |  1 +
 drivers/gpu/drm/i915/gvt/edid.c         |  1 +
 drivers/gpu/drm/i915/gvt/execlist.c     |  1 +
 drivers/gpu/drm/i915/gvt/firmware.c     |  2 ++
 drivers/gpu/drm/i915/gvt/gtt.c          |  2 ++
 drivers/gpu/drm/i915/gvt/gvt.c          | 19 +++++++++++++------
 drivers/gpu/drm/i915/gvt/gvt.h          |  4 ++--
 drivers/gpu/drm/i915/gvt/handlers.c     |  2 ++
 drivers/gpu/drm/i915/gvt/interrupt.c    |  1 +
 drivers/gpu/drm/i915/gvt/mmio.c         |  1 +
 drivers/gpu/drm/i915/gvt/opregion.c     |  1 +
 drivers/gpu/drm/i915/gvt/render.c       |  1 +
 drivers/gpu/drm/i915/gvt/sched_policy.c |  1 +
 drivers/gpu/drm/i915/gvt/scheduler.c    |  5 +++--
 drivers/gpu/drm/i915/gvt/vgpu.c         |  2 ++
 drivers/gpu/drm/i915/i915_drv.h         |  4 ++--
 drivers/gpu/drm/i915/intel_gvt.h        |  3 +--
 20 files changed, 41 insertions(+), 14 deletions(-)
drivers/gpu/drm/i915/gvt/aperture_gm.c +1 -0
index e0211f8..db503c1 100644
@@ -35,6 +35,7 @@
  */
 
 #include "i915_drv.h"
+#include "gvt.h"
 
 #define MB_TO_BYTES(mb) ((mb) << 20ULL)
 #define BYTES_TO_MB(b) ((b) >> 20ULL)
drivers/gpu/drm/i915/gvt/cfg_space.c +1 -0
index 16360e4..4c68774 100644
@@ -32,6 +32,7 @@
  */
 
 #include "i915_drv.h"
+#include "gvt.h"
 
 enum {
 	INTEL_GVT_PCI_BAR_GTTMMIO = 0,
drivers/gpu/drm/i915/gvt/cmd_parser.c +2 -0
index 5808ee7..5b4658f 100644
@@ -36,6 +36,8 @@
 
 #include <linux/slab.h>
 #include "i915_drv.h"
+#include "gvt.h"
+#include "i915_pvinfo.h"
 #include "trace.h"
 
 #define INVALID_OP    (~0U)
drivers/gpu/drm/i915/gvt/display.c +1 -0
index 534000b..d8908d4 100644
@@ -33,6 +33,7 @@
  */
 
 #include "i915_drv.h"
+#include "gvt.h"
 
 static int get_edp_pipe(struct intel_vgpu *vgpu)
 {
drivers/gpu/drm/i915/gvt/edid.c +1 -0
index a07e427..7e1da1c 100644
@@ -33,6 +33,7 @@
  */
 
 #include "i915_drv.h"
+#include "gvt.h"
 
 #define GMBUS1_TOTAL_BYTES_SHIFT 16
 #define GMBUS1_TOTAL_BYTES_MASK 0x1ff
drivers/gpu/drm/i915/gvt/execlist.c +1 -0
index c50a3d1..b87b4f5 100644
@@ -33,6 +33,7 @@
  */
 
 #include "i915_drv.h"
+#include "gvt.h"
 
 #define _EL_OFFSET_STATUS       0x234
 #define _EL_OFFSET_STATUS_BUF   0x370
drivers/gpu/drm/i915/gvt/firmware.c +2 -0
index 4578a4d..d068a52 100644
@@ -32,6 +32,8 @@
 #include <linux/crc32.h>
 
 #include "i915_drv.h"
+#include "gvt.h"
+#include "i915_pvinfo.h"
 
 #define FIRMWARE_VERSION (0x0)
drivers/gpu/drm/i915/gvt/gtt.c +2 -0
index 29de179..0722d1e 100644
@@ -34,6 +34,8 @@
  */
 
 #include "i915_drv.h"
+#include "gvt.h"
+#include "i915_pvinfo.h"
 #include "trace.h"
 
 static bool enable_out_of_sync = false;
drivers/gpu/drm/i915/gvt/gvt.c +13 -6
index e72e26c..31b59d4 100644
@@ -35,6 +35,7 @@
 #include <linux/kthread.h>
 
 #include "i915_drv.h"
+#include "gvt.h"
 
 struct intel_gvt_host intel_gvt_host;
 

@@ -173,9 +174,9 @@ static int init_service_thread(struct intel_gvt *gvt)
  */
 void intel_gvt_clean_device(struct drm_i915_private *dev_priv)
 {
-	struct intel_gvt *gvt = &dev_priv->gvt;
+	struct intel_gvt *gvt = to_gvt(dev_priv);
 
-	if (WARN_ON(!gvt->initialized))
+	if (WARN_ON(!gvt))
 		return;
 
 	clean_service_thread(gvt);

@@ -188,7 +189,8 @@ void intel_gvt_clean_device(struct drm_i915_private *dev_priv)
 	intel_gvt_clean_mmio_info(gvt);
 	intel_gvt_free_firmware(gvt);
 
-	gvt->initialized = false;
+	kfree(dev_priv->gvt);
+	dev_priv->gvt = NULL;
 }
 
 /**

@@ -204,7 +206,7 @@ void intel_gvt_clean_device(struct drm_i915_private *dev_priv)
  */
 int intel_gvt_init_device(struct drm_i915_private *dev_priv)
 {
-	struct intel_gvt *gvt = &dev_priv->gvt;
+	struct intel_gvt *gvt;
 	int ret;
 
 	/*

@@ -214,9 +216,13 @@ int intel_gvt_init_device(struct drm_i915_private *dev_priv)
 	if (WARN_ON(!intel_gvt_host.initialized))
 		return -EINVAL;
 
-	if (WARN_ON(gvt->initialized))
+	if (WARN_ON(dev_priv->gvt))
 		return -EEXIST;
 
+	gvt = kzalloc(sizeof(struct intel_gvt), GFP_KERNEL);
+	if (!gvt)
+		return -ENOMEM;
+
 	gvt_dbg_core("init gvt device\n");
 
 	mutex_init(&gvt->lock);

@@ -261,7 +267,7 @@ int intel_gvt_init_device(struct drm_i915_private *dev_priv)
 		goto out_clean_cmd_parser;
 
 	gvt_dbg_core("gvt device creation is done\n");
-	gvt->initialized = true;
+	dev_priv->gvt = gvt;
 	return 0;
 
 out_clean_cmd_parser:

@@ -280,5 +286,6 @@ out_free_firmware:
 	intel_gvt_free_firmware(gvt);
 out_clean_mmio_info:
 	intel_gvt_clean_mmio_info(gvt);
+	kfree(gvt);
 	return ret;
 }
drivers/gpu/drm/i915/gvt/gvt.h +2 -2
index 1564554..e2ebc79 100644
@@ -192,8 +192,6 @@ struct intel_gvt_opregion {
 
 struct intel_gvt {
 	struct mutex lock;
-	bool initialized;
-
 	struct drm_i915_private *dev_priv;
 	struct idr vgpu_idr;	/* vGPU IDR pool */
 

@@ -213,6 +211,8 @@ struct intel_gvt {
 	unsigned long service_request;
 };
 
+#define to_gvt(dev_priv) (struct intel_gvt *)((dev_priv)->gvt)
+
 enum {
 	INTEL_GVT_REQUEST_EMULATE_VBLANK = 0,
 };
drivers/gpu/drm/i915/gvt/handlers.c +2 -0
index e8ec403..b21115f 100644
@@ -37,6 +37,8 @@
  */
 
 #include "i915_drv.h"
+#include "gvt.h"
+#include "i915_pvinfo.h"
 
 /* XXX FIXME i915 has changed PP_XXX definition */
 #define PCH_PP_STATUS  _MMIO(0xc7200)
drivers/gpu/drm/i915/gvt/interrupt.c +1 -0
index 84d7174..e43ef72 100644
@@ -30,6 +30,7 @@
  */
 
 #include "i915_drv.h"
+#include "gvt.h"
 
 /* common offset among interrupt control registers */
 #define regbase_to_isr(base)	(base)
drivers/gpu/drm/i915/gvt/mmio.c +1 -0
index ce3af95..585b01f 100644
@@ -34,6 +34,7 @@
  */
 
 #include "i915_drv.h"
+#include "gvt.h"
 
 /**
  * intel_vgpu_gpa_to_mmio_offset - translate a GPA to MMIO offset
drivers/gpu/drm/i915/gvt/opregion.c +1 -0
index 46cc240..53ac81f 100644
@@ -23,6 +23,7 @@
 
 #include <linux/acpi.h>
 #include "i915_drv.h"
+#include "gvt.h"
 
 static int init_vgpu_opregion(struct intel_vgpu *vgpu, u32 gpa)
 {
drivers/gpu/drm/i915/gvt/render.c +1 -0
index f54ab85..feebb65 100644
@@ -34,6 +34,7 @@
  */
 
 #include "i915_drv.h"
+#include "gvt.h"
 
 struct render_mmio {
 	int ring_id;
drivers/gpu/drm/i915/gvt/sched_policy.c +1 -0
index c607354..278db0c 100644
@@ -32,6 +32,7 @@
  */
 
 #include "i915_drv.h"
+#include "gvt.h"
 
 static bool vgpu_has_pending_workload(struct intel_vgpu *vgpu)
 {
drivers/gpu/drm/i915/gvt/scheduler.c +3 -2
index b15cdf5..01d23ad 100644
@@ -33,10 +33,11 @@
  *
  */
 
-#include "i915_drv.h"
-
 #include <linux/kthread.h>
 
+#include "i915_drv.h"
+#include "gvt.h"
+
 #define RING_CTX_OFF(x) \
 	offsetof(struct execlist_ring_context, x)
drivers/gpu/drm/i915/gvt/vgpu.c +2 -0
index e5e0a72..9401436 100644
@@ -32,6 +32,8 @@
  */
 
 #include "i915_drv.h"
+#include "gvt.h"
+#include "i915_pvinfo.h"
 
 static void clean_vgpu_mmio(struct intel_vgpu *vgpu)
 {
drivers/gpu/drm/i915/i915_drv.h +2 -2
index 4d1133f..5024ad9 100644
@@ -1778,7 +1778,7 @@ struct drm_i915_private {
 
 	struct i915_virtual_gpu vgpu;
 
-	struct intel_gvt gvt;
+	struct intel_gvt *gvt;
 
 	struct intel_guc guc;
 

@@ -2992,7 +2992,7 @@ int intel_wait_for_register_fw(struct drm_i915_private *dev_priv,
 
 static inline bool intel_gvt_active(struct drm_i915_private *dev_priv)
 {
-	return dev_priv->gvt.initialized;
+	return dev_priv->gvt;
 }
 
 static inline bool intel_vgpu_active(struct drm_i915_private *dev_priv)
drivers/gpu/drm/i915/intel_gvt.h +1 -2
index 0f00105..25df2d6 100644
@@ -24,8 +24,7 @@
 #ifndef _INTEL_GVT_H_
 #define _INTEL_GVT_H_
 
-#include "i915_pvinfo.h"
-#include "gvt/gvt.h"
+struct intel_gvt;
 
 #ifdef CONFIG_DRM_I915_GVT
 int intel_gvt_init(struct drm_i915_private *dev_priv);
Signature · 6 lines
2.9.3

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Reviewed-by on v3 1/1
#1 lore raw

drivers/gpu/drm/i915/gvt/gvt.c int intel_gvt_init_device(struct drm_i915_private *dev_priv) · line 219

Quoted 13 lines · Zhenyu Wang

int intel_gvt_init_device(struct drm_i915_private *dev_priv) {

drivers/gpu/drm/i915/gvt/gvt.c +2 -2
-	struct intel_gvt *gvt = &dev_priv->gvt;
+	struct intel_gvt *gvt;
 	int ret;
 
 	/*

@@ -214,9 +216,13 @@ int intel_gvt_init_device(struct drm_i915_private *dev_priv)
 	if (WARN_ON(!intel_gvt_host.initialized))
 		return -EINVAL;
 
-	if (WARN_ON(gvt->initialized))
+	if (WARN_ON(dev_priv->gvt))

This is the odd one out

if (WARN_ON(to_gvt(dev_priv)) ?

On the other hand it matches with the assignment. but you could do to_gvt(dev_priv) = gvt; there, that's getting ugly. So I think the current code is sensible.

drivers/gpu/drm/i915/gvt/gvt.h struct intel_gvt { · line 214

Quoted 1 line
drivers/gpu/drm/i915/gvt/gvt.h +1 -0
+#define to_gvt(dev_priv) (struct intel_gvt *)((dev_priv)->gvt)

You can now dispense with the cast. I'd prefer if this was an inline for clearer type checking.

static inline struct intel_gvt *to_gvt(struct drm_i915_private *i915)

{
	return i915->gvt;
}

Tidy up to_gvt() (whatever takes your fancy) and

Reviewed-by Chris Wilson <[email protected]>

-Chris

Signature · 5 lines
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
CI
#2 lore raw

✗ Fi.CI.BAT: failure for drm/i915/gvt: clean up intel_gvt.h as interface for i915 core (rev3)

In reply to Zhenyu Wang
== Series Details ==

== Series Details ==

Series: drm/i915/gvt: clean up intel_gvt.h as interface for i915 core (rev3)
URL : https://patchwork.freedesktop.org/series/14078/
State : failure

== Summary ==

Series 14078v3 drm/i915/gvt: clean up intel_gvt.h as interface for i915 core
https://patchwork.freedesktop.org/api/1.0/series/14078/revisions/3/mbox/

Test drv_module_reload_basic:

                dmesg-warn -> PASS       (fi-skl-6700hq)
Test gem_exec_suspend:
        Subgroup basic-s3:
                pass       -> DMESG-WARN (fi-skl-6700hq)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-a:
                pass       -> DMESG-WARN (fi-skl-6700hq)
        Subgroup suspend-read-crc-pipe-b:
                pass       -> DMESG-WARN (fi-skl-6700hq)
                pass       -> INCOMPLETE (fi-snb-2600)
        Subgroup suspend-read-crc-pipe-c:
                pass       -> DMESG-WARN (fi-skl-6700hq)
fi-bdw-5557u     total:246  pass:231  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:246  pass:204  dwarn:0   dfail:0   fail:0   skip:42 
fi-bxt-t5700     total:246  pass:216  dwarn:0   dfail:0   fail:0   skip:30 
fi-byt-j1900     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
fi-byt-n2820     total:246  pass:211  dwarn:0   dfail:0   fail:0   skip:35 
fi-hsw-4770      total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22 
fi-hsw-4770r     total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22 
fi-ilk-650       total:246  pass:185  dwarn:0   dfail:0   fail:1   skip:60 
fi-ivb-3520m     total:246  pass:221  dwarn:0   dfail:0   fail:0   skip:25 
fi-ivb-3770      total:246  pass:221  dwarn:0   dfail:0   fail:0   skip:25 
fi-kbl-7200u     total:246  pass:222  dwarn:0   dfail:0   fail:0   skip:24 
fi-skl-6260u     total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:246  pass:219  dwarn:4   dfail:0   fail:0   skip:23 
fi-skl-6700k     total:246  pass:221  dwarn:1   dfail:0   fail:0   skip:24 
fi-skl-6770hq    total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14 
fi-snb-2520m     total:246  pass:210  dwarn:0   dfail:0   fail:0   skip:36 
fi-snb-2600      total:209  pass:176  dwarn:0   dfail:0   fail:0   skip:32

Results at /archive/results/CI_IGT_test/Patchwork_2773/

0b12b48da3bc580102665ee9efd7e834f6def00f drm-intel-nightly: 2016y-10m-20d-07h-52m-31s UTC integration manifest f589a37 drm/i915/gvt: clean up intel_gvt.h as interface for i915 core

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx