[PATCH v2 2/2] drm/i915/gvt: Support PPGTT table load command

[Yan Zhao]

On Tue, Apr 28, 2020 at 11:02:53AM +0800, Zhenyu Wang wrote:
> On 2020.04.28 00:41:48 +0000, Zhao, Yan Y wrote:
> > 
> > > On 2020.04.27 03:21:49 -0400, Yan Zhao wrote:
> > > > > >
> > > > > > I think the step is just to restore guest context and run through
> > > > > > loaded shadow mm, we only need to recover last guest pdp after
> > > > > > workload run.
> > > > > >
> > > > > sorry. sent too fast.
> > > > > I mean why not just update workload->shadow_mm and
> > > > > workload->shadow_mm->ppgtt_mm.guest_pdps, when lri pdp is found?
> > > > >
> > > > 1. if lri pdp1 (not the same as shadow_mm) and lri pdp2 (the same as
> > > > shadow_mm) are executed successively, we should not update the guest
> > > > pdp.
> > > 
> > > Actually we should, to replace with shadow ppgtt ptr also for same as
> > > shadow_mm case, otherwise wrong pdps would be loaded to HW. Next
> > > version will fix that.
> > > 
> > > > 2. if there's a save inhibit bit set in the workload, we cannot update
> > > > the guest context.
> > > 
> > > yep, looks currently we don't care about save inhibit but just update guest
> > > context. We might do this later.
> > > 
> > > > 3. there may be other cases to cause last entry in lri_shadow_mm no
> > > > aligning with the final one in workload.
> > > > so I think the safest way to update guest context is to find the
> > > > guest_pdps according to pdps in shadow context.
> > > 
> > > Hmm, I think it's safe now to mark last ppgtt LRI cmd as last context state
> > > because I can't think of other ways might affect that. And to find guest pdps,
> > > we have to walk through all shadow mm which seems not worth to me.
> > > 
> > No need to walk through all shadow mms.
> > Just shadow mms for current workload, and you can start from the most possible one.
> 
> Current shadow mm list is for per vgpu instead of per workload, so in this patch
> which adds new list for all possible shadow mm per workload.
> 
I think in update_guest_context(), shadow mm is still kept in
workload->shadow_mm.

so the choice is within workload->shadow_mm and entries in workload->lri_shadow_mm.


> > Why not choose the most robust way ?  (like save inhibit. it's possible that we might miss something)
> >
> 
> I think it's robust because we know exactly what the result should be
> for hw behavior emulation. How about to add a debug func to double
> check if context pdps matches? So in normal operation it won't bother.
> 
> Thanks
> 
> > > 
> > > >
> > > > > > > > > >
> > > > > > > > > > and what's the reason for us to use a lri_shadow_mm list?
> > > > > > > > >
> > > > > > > > > As in theory there could be multiple ppgtt loads in one ring
> > > > > > > > > submission, and previous code always assumes one e.g put
> > > > > > > > > older scanned shadow mm if found another one, which was not
> > > > > > > > > right to me. So here just uses a link to track all possible loads.
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > 
> > > --
> > > Open Source Technology Center, Intel ltd.
> > > 
> > > $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
> 
> -- 
> Open Source Technology Center, Intel ltd.
> 
> $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827



> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev