Re: refactor the i915 GVT support

[Zhenyu Wang]

On 2021.08.19 17:43:43 +0300, Joonas Lahtinen wrote:
> Quoting Zhenyu Wang (2021-08-19 11:29:29)
> > On 2021.08.17 13:22:03 +0800, Zhenyu Wang wrote:
> > > > On 2021.08.16 19:34:58 +0200, Christoph Hellwig wrote:
> > > > > Any updates on this?  I'd really hate to miss this merge window.
> > > > 
> > > > I'm still waiting for our validation team's report on this. I'm afraid
> > > > it might be missing for next version as i915 merge window is mostly
> > > > till rc5...and for any change outside of gvt, it still needs to be
> > > > acked by i915 maintainers.
> > > 
> > > Looks our validation team did have problem against recent i915 change.
> > > If you like to try, we have a gvt-staging branch on
> > > https://github.com/intel/gvt-linux which is generated against drm-tip
> > > with gvt changes for testing, currently it's broken.
> > > 
> > > One issue is with i915 export that intel_context_unpin has been
> > > changed into static inline function. Another is that intel_gvt.c
> > > should be part of i915 for gvt interface instead of depending on KVMGT
> > > config.
> > 
> > I'm working on below patch to resolve this. But I met a weird issue in
> > case when building i915 as module and also kvmgt module, it caused
> > busy wait on request_module("kvmgt") when boot, it doesn't happen if
> > building i915 into kernel. I'm not sure what could be the reason?
> > 
> > > But the problem I see is that after moving gvt device model (gvt/*.c
> > > except kvmgt.c) into kvmgt module, we'll have issue with initial mmio
> > > state which current gvt relies on, that is in design supposed to get
> > > initial HW state before i915 driver has taken any operation.
> 
> As mentioned in some past discussions, I think it would be best rely on
> golden MMIO located in /lib/firmware or elsewhere. This way we will better
> isolate the guest system from host system updates/changes.
> 
> This should also hopefully allow enabling kvmgt module after i915 has
> already loaded, as the initialization would not be conditional to
> capture the MMIO.
> 

I think the concern is that even for same GEN hw there could be many
variant platforms e.g APL with gen9, etc. To verify golden states for
them all might take too much effort...

> 
> > > Before
> > > we can ensure that, I think we may only remove MPT part first but
> > > still keep gvt device model as part of i915 with config. I'll try to
> > > split that out.
> > 
> > Sorry I misread the code that as we always request kvmgt module when
> > gvt init, so it should still apply original method that this isn't a
> > problem. Our current validation result has shown no regression as well.
> > 
> > ---8<---
> > From 58ff84572f1a0f9d79ca1d7ec0cff5ecbe78d280 Mon Sep 17 00:00:00 2001
> > From: Zhenyu Wang <zhenyuw@xxxxxxxxxxxxxxx>
> > Date: Thu, 19 Aug 2021 16:36:33 +0800
> > Subject: [PATCH] TESTONLY:drm/i915/gvt: potential fix for refactor against
> >  current tip
> > 
> > ---
> >  drivers/gpu/drm/i915/Makefile           | 4 +++-
> >  drivers/gpu/drm/i915/gt/intel_context.c | 5 +++++
> >  drivers/gpu/drm/i915/gt/intel_context.h | 3 ++-
> >  drivers/gpu/drm/i915/i915_trace.h       | 1 +
> >  4 files changed, 11 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> > index c4f953837f72..2248574428a1 100644
> > --- a/drivers/gpu/drm/i915/Makefile
> > +++ b/drivers/gpu/drm/i915/Makefile
> > @@ -296,7 +296,9 @@ i915-$(CONFIG_DRM_I915_SELFTEST) += \
> >  
> >  # virtual gpu code
> >  i915-y += i915_vgpu.o
> > -i915-$(CONFIG_DRM_I915_GVT_KVMGT) += intel_gvt.o
> > +ifneq ($(CONFIG_DRM_I915_GVT_KVMGT),)
> > +i915-y += intel_gvt.o
> > +endif
> >  
> >  kvmgt-y += gvt/kvmgt.o \
> >         gvt/gvt.o \
> > diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
> > index 745e84c72c90..20e7522fed84 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_context.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_context.c
> > @@ -328,6 +328,11 @@ void __intel_context_do_unpin(struct intel_context *ce, int sub)
> >         intel_context_put(ce);
> >  }
> >  
> > +void intel_context_unpin(struct intel_context *ce)
> > +{
> > +       _intel_context_unpin(ce);
> > +}
> > +
> >  static void __intel_context_retire(struct i915_active *active)
> >  {
> >         struct intel_context *ce = container_of(active, typeof(*ce), active);
> > diff --git a/drivers/gpu/drm/i915/gt/intel_context.h b/drivers/gpu/drm/i915/gt/intel_context.h
> > index c41098950746..f942cbf6300a 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_context.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_context.h
> > @@ -131,7 +131,7 @@ static inline void intel_context_sched_disable_unpin(struct intel_context *ce)
> >         __intel_context_do_unpin(ce, 2);
> >  }
> >  
> > -static inline void intel_context_unpin(struct intel_context *ce)
> > +static inline void _intel_context_unpin(struct intel_context *ce)
> >  {
> >         if (!ce->ops->sched_disable) {
> >                 __intel_context_do_unpin(ce, 1);
> > @@ -150,6 +150,7 @@ static inline void intel_context_unpin(struct intel_context *ce)
> >                 }
> >         }
> >  }
> > +void intel_context_unpin(struct intel_context *ce);
> >  
> >  void intel_context_enter_engine(struct intel_context *ce);
> >  void intel_context_exit_engine(struct intel_context *ce);
> > diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
> > index 806ad688274b..2c6a8bcef7c1 100644
> > --- a/drivers/gpu/drm/i915/i915_trace.h
> > +++ b/drivers/gpu/drm/i915/i915_trace.h
> > @@ -17,6 +17,7 @@
> >  
> >  #undef TRACE_SYSTEM
> >  #define TRACE_SYSTEM i915
> > +#undef TRACE_INCLUDE_FILE
> >  #define TRACE_INCLUDE_FILE i915_trace
> >  
> >  /* watermark/fifo updates */
> > -- 
> > 2.32.0
> > ---8<---
> > 
> >

Attachment: signature.asc
Description: PGP signature