[PATCH v2] drm/i915/gvt: refine vgpu gtt sync policy to reduce invalidate times

[Li, Weinan Z]

-----Original Message-----
From: Li, Weinan Z 
Sent: Friday, November 3, 2017 10:48 AM
To: 'Zhenyu Wang' <zhenyuw at linux.intel.com>; Tian, Kevin <kevin.tian at intel.com>
Cc: intel-gvt-dev at lists.freedesktop.org; Wang, Zhi A <zhi.a.wang at intel.com>
Subject: RE: [PATCH v2] drm/i915/gvt: refine vgpu gtt sync policy to reduce invalidate times

Is there any other paths need to access the GTT but not via GPU command beside "display"?
How about adding flush check before virtual vblank event injection to avoid the risk from display.

Regards
Weinan

-----Original Message-----
From: Zhenyu Wang [mailto:zhenyuw at linux.intel.com]
Sent: Friday, November 3, 2017 10:16 AM
To: Tian, Kevin <kevin.tian at intel.com>
Cc: Li, Weinan Z <weinan.z.li at intel.com>; intel-gvt-dev at lists.freedesktop.org; Wang, Zhi A <zhi.a.wang at intel.com>
Subject: Re: [PATCH v2] drm/i915/gvt: refine vgpu gtt sync policy to reduce invalidate times

On 2017.11.03 02:03:59 +0000, Tian, Kevin wrote:
> The assumption of this change is that, all GGTT addresses beyond 
> aperture are only accessed by device when executing a workload, so we 
> can delay update of those entries until the point of detecting a 
> workload submission. However such assumption may not be true, at least 
> for display. What about guest chooses a way to flip framebuffers not 
> by changing base address while instead by changing GGTT mapping on the 
> same base address? If such guest framebuffer is already programmed to 
> physical display plane (as required in local display usage e.g.
> automotive), then delay of update would lead to incorrect content 
> being shown. Current driver might not behave like that, but in GVT-g 
> side we should make sure it's sane.
>

ggtt mapping will still be trapped so gvt still know the change, but as you said assuming only for workload execution is wrong that ignored display. Maybe we can add invalidate in display plane update for that.
Then ensure to cover both MI & reg update.

> > From: Weinan Li
> > Sent: Friday, November 3, 2017 9:01 AM
> > 
> > Add delayed_update flag in gvt->gtt to indicate there is cached gtt 
> > entries need to be flushed before vgpu workload submittion, then 
> > don't need to trigger invalidate gtt every time gtt entry write, it 
> > can save time in vgpu reset or vgpu has frequently gtt entries updating case.
> > 
> > v2 : use flag instead of atomic_t, code refine (Zhi)
> > 
> > Signed-off-by: Weinan Li <weinan.z.li at intel.com>
> > Cc: Zhi Wang <zhi.a.wang at intel.com>
> > Cc: Zhenyu Wang <zhenyuw at linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/gvt/gtt.c | 20 ++++++++++++++++++--  
> > drivers/gpu/drm/i915/gvt/gtt.h |  1 +
> >  2 files changed, 19 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gvt/gtt.c 
> > b/drivers/gpu/drm/i915/gvt/gtt.c index 6fa9271..b4cb5ae 100644
> > --- a/drivers/gpu/drm/i915/gvt/gtt.c
> > +++ b/drivers/gpu/drm/i915/gvt/gtt.c
> > @@ -1293,6 +1293,15 @@ static int ppgtt_set_guest_page_oos(struct 
> > intel_vgpu *vgpu,
> >  	return intel_gvt_hypervisor_disable_page_track(vgpu, &gpt->track);  
> > }
> > 
> > +int intel_vgpu_flush_mm(struct intel_gvt *gvt, bool force) {
> > +	if (force || gvt->gtt.delayed_update) {
> > +		gvt->gtt.delayed_update = false;
> > +		gtt_invalidate(gvt->dev_priv);
> > +	}
> > +	return 0;
> > +}
> > +
> >  /**
> >   * intel_vgpu_sync_oos_pages - sync all the out-of-synced shadow 
> > for vGPU
> >   * @vgpu: a vGPU
> > @@ -1405,6 +1414,8 @@ int intel_vgpu_flush_post_shadow(struct
> > intel_vgpu *vgpu)
> >  	unsigned long index;
> >  	int ret;
> > 
> > +	intel_vgpu_flush_mm(vgpu->gvt, false);
> > +
> >  	list_for_each_safe(pos, n, &vgpu->gtt.post_shadow_list_head) {
> >  		spt = container_of(pos, struct intel_vgpu_ppgtt_spt,
> >  				post_shadow_list);
> > @@ -1953,7 +1964,6 @@ static int emulate_gtt_mmio_write(struct 
> > intel_vgpu *vgpu, unsigned int off,
> >  	/* the VM may configure the whole GM space when ballooning is used 
> > */
> >  	if (!vgpu_gmadr_is_valid(vgpu, gma))
> >  		return 0;
> > -
> >  	ggtt_get_guest_entry(ggtt_mm, &e, g_gtt_index);
> > 
> >  	memcpy((void *)&e.val64 + (off & (info->gtt_entry_size - 1)), 
> > p_data, @@ -1975,8 +1985,14 @@ static int 
> > emulate_gtt_mmio_write(struct intel_vgpu *vgpu, unsigned int off,
> >  	}
> > 
> >  	ggtt_set_shadow_entry(ggtt_mm, &m, g_gtt_index);
> > -	gtt_invalidate(gvt->dev_priv);
> > +
> > +	if (vgpu_gmadr_is_aperture(vgpu, gma))
> > +		gtt_invalidate(gvt->dev_priv);
> > +	else
> > +		gvt->gtt.delayed_update = true;
> > +
> >  	ggtt_set_guest_entry(ggtt_mm, &e, g_gtt_index);
> > +
> >  	return 0;
> >  }
> > 
> > diff --git a/drivers/gpu/drm/i915/gvt/gtt.h 
> > b/drivers/gpu/drm/i915/gvt/gtt.h index 416b2f8..f36f485 100644
> > --- a/drivers/gpu/drm/i915/gvt/gtt.h
> > +++ b/drivers/gpu/drm/i915/gvt/gtt.h
> > @@ -88,6 +88,7 @@ struct intel_gvt_gtt {
> > 
> >  	struct page *scratch_page;
> >  	unsigned long scratch_mfn;
> > +	bool delayed_update;
> >  };
> > 
> >  enum {
> > --
> > 1.9.1
> > 
> > _______________________________________________
> > intel-gvt-dev mailing list
> > intel-gvt-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev

--
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827