Re: [Intel-gfx] [PATCH v3] drm/i915/gvt: clean up intel_gvt.h as interface for i915 core

[Chris Wilson]

On Thu, Oct 20, 2016 at 04:26:14PM +0800, Zhenyu Wang wrote:
>  int intel_gvt_init_device(struct drm_i915_private *dev_priv)
>  {
> - struct intel_gvt *gvt = _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.

> +#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 
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v3] drm/i915/gvt: clean up intel_gvt.h as interface for i915 core

[Zhenyu Wang]

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 
---
 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(-)

diff --git a/drivers/gpu/drm/i915/gvt/aperture_gm.c 
b/drivers/gpu/drm/i915/gvt/aperture_gm.c
index e0211f8..db503c1 100644
--- a/drivers/gpu/drm/i915/gvt/aperture_gm.c
+++ b/drivers/gpu/drm/i915/gvt/aperture_gm.c
@@ -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)
diff --git a/drivers/gpu/drm/i915/gvt/cfg_space.c 
b/drivers/gpu/drm/i915/gvt/cfg_space.c
index 16360e4..4c68774 100644
--- a/drivers/gpu/drm/i915/gvt/cfg_space.c
+++ b/drivers/gpu/drm/i915/gvt/cfg_space.c
@@ -32,6 +32,7 @@
  */
 
 #include "i915_drv.h"
+#include "gvt.h"
 
 enum {
INTEL_GVT_PCI_BAR_GTTMMIO = 0,
diff --git a/drivers/gpu/drm/i915/gvt/cmd_parser.c 
b/drivers/gpu/drm/i915/gvt/cmd_parser.c
index 5808ee7..5b4658f 100644
--- a/drivers/gpu/drm/i915/gvt/cmd_parser.c
+++ b/drivers/gpu/drm/i915/gvt/cmd_parser.c
@@ -36,6 +36,8 @@
 
 #include 
 #include "i915_drv.h"
+#include "gvt.h"
+#include "i915_pvinfo.h"
 #include "trace.h"
 
 #define INVALID_OP(~0U)
diff --git a/drivers/gpu/drm/i915/gvt/display.c 
b/drivers/gpu/drm/i915/gvt/display.c
index 534000b..d8908d4 100644
--- a/drivers/gpu/drm/i915/gvt/display.c
+++ b/drivers/gpu/drm/i915/gvt/display.c
@@ -33,6 +33,7 @@
  */
 
 #include "i915_drv.h"
+#include "gvt.h"
 
 static int get_edp_pipe(struct intel_vgpu *vgpu)
 {
diff --git a/drivers/gpu/drm/i915/gvt/edid.c b/drivers/gpu/drm/i915/gvt/edid.c
index a07e427..7e1da1c 100644
--- a/drivers/gpu/drm/i915/gvt/edid.c
+++ b/drivers/gpu/drm/i915/gvt/edid.c
@@ -33,6 +33,7 @@
  */
 
 #include "i915_drv.h"
+#include "gvt.h"
 
 #define GMBUS1_TOTAL_BYTES_SHIFT 16
 #define GMBUS1_TOTAL_BYTES_MASK 0x1ff
diff --git a/drivers/gpu/drm/i915/gvt/execlist.c 
b/drivers/gpu/drm/i915/gvt/execlist.c
index c50a3d1..b87b4f5 100644
--- a/drivers/gpu/drm/i915/gvt/execlist.c
+++ b/drivers/gpu/drm/i915/gvt/execlist.c
@@ -33,6 +33,7 @@
  */
 
 #include "i915_drv.h"
+#include "gvt.h"
 
 #define _EL_OFFSET_STATUS   0x234
 #define _EL_OFFSET_STATUS_BUF   0x370
diff --git a/drivers/gpu/drm/i915/gvt/firmware.c 
b/drivers/gpu/drm/i915/gvt/firmware.c
index 4578a4d..d068a52 100644
--- a/drivers/gpu/drm/i915/gvt/firmware.c
+++ b/drivers/gpu/drm/i915/gvt/firmware.c
@@ -32,6 +32,8 @@
 #include 
 
 #include "i915_drv.h"
+#include "gvt.h"
+#include "i915_pvinfo.h"
 
 #define FIRMWARE_VERSION (0x0)
 
diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
index 29de179..0722d1e 100644
--- a/drivers/gpu/drm/i915/gvt/gtt.c
+++ b/drivers/gpu/drm/i915/gvt/gtt.c
@@ -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;
diff --git a/drivers/gpu/drm/i915/gvt/gvt.c b/drivers/gpu/drm/i915/gvt/gvt.c
index e72e26c..31b59d4 100644
--- a/drivers/gpu/drm/i915/gvt/gvt.c
+++ b/drivers/gpu/drm/i915/gvt/gvt.c
@@ -35,6 +35,7 @@
 #include 
 
 #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 = _priv->gvt;
+   struct intel_gvt *gvt = to_gvt(dev_priv);
 
-   if (WARN_ON(!gvt->initialized))
+   if (WARN_ON(!gvt))
return;
 
clean_service_thread(gvt);
@@