[PATCH v3] drm/i915/gvt: update vreg on indirect context lri command

[Hang Yuan]

On Mon, Jul 02, 2018 at 06:06:14AM +0000, Li, Weinan Z wrote:
> > -----Original Message-----
> > From: Zhenyu Wang [mailto:zhenyuw at linux.intel.com]
> > Sent: Monday, July 2, 2018 1:56 PM
> > To: Yuan, Hang <hang.yuan at intel.com>; intel-gvt-dev at lists.freedesktop.org;
> > Li, Weinan Z <weinan.z.li at intel.com>
> > Subject: Re: [PATCH v3] drm/i915/gvt: update vreg on indirect context lri
> > command
> > 
> > On 2018.07.02 13:53:36 +0800, Hang Yuan wrote:
> > > On Mon, Jul 02, 2018 at 12:04:57PM +0800, Zhenyu Wang wrote:
> > > > On 2018.07.02 11:11:22 +0800, Zhenyu Wang wrote:
> > > > > On 2018.07.02 10:12:13 +0800, intel-gvt-dev-
> > bounces at lists.freedesktop.org wrote:
> > > > > > From: Hang Yuan <hang.yuan at linux.intel.com>
> > > > > >
> > > > > > Commit cd7e 61b9"init mmio by lri command in vgpu inhibit context"
> > > > > > initializes registers saved/restored in context with its vreg
> > > > > > value through lri command in ring buffer. It relies on vreg got
> > > > > > updated on every guest access. There is a case found that Linux
> > > > > > guest uses lri command in indirect-ctx to update the register.
> > > > > > This patch adds vreg update on this case.
> > > > > >
> > > > > > v2: move mmio_attribute functions to gvt.h (Zhenyu Wang)
> > > > > > v3: use mask_mmio_write in vreg update
> > > > > >
> > > > > > Fixes: cd7e61b9("drm/i915/gvt: init mmio by lri command in vgpu
> > > > > > inhibit context")
> > > > > > Signed-off-by: Hang Yuan <hang.yuan at linux.intel.com>
> > > > > > Signed-off-by: Weinan Li <weinan.z.li at intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/gvt/cmd_parser.c   | 13 +++++++++++++
> > > > > >  drivers/gpu/drm/i915/gvt/gvt.h          | 29
> > +++++++++++++++++++++++++++++
> > > > > >  drivers/gpu/drm/i915/gvt/handlers.c     | 24
> > ++++++++++++++++++++++++
> > > > > >  drivers/gpu/drm/i915/gvt/mmio.h         |  2 ++
> > > > > >  drivers/gpu/drm/i915/gvt/mmio_context.c |  4 +++-
> > > > > >  5 files changed, 71 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > > > > b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > > > > index 0651e63..594f10b 100644
> > > > > > --- a/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > > > > +++ b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > > > > @@ -863,6 +863,7 @@ static int cmd_reg_handler(struct
> > > > > > parser_exec_state *s,  {
> > > > > >  	struct intel_vgpu *vgpu = s->vgpu;
> > > > > >  	struct intel_gvt *gvt = vgpu->gvt;
> > > > > > +	u32 ctx_sr_ctl;
> > > > > >
> > > > > >  	if (offset + 4 > gvt->device_info.mmio_size) {
> > > > > >  		gvt_vgpu_err("%s access to (%x) outside of MMIO
> > range\n", @@
> > > > > > -895,6 +896,18 @@ static int cmd_reg_handler(struct
> > parser_exec_state *s,
> > > > > >  		patch_value(s, cmd_ptr(s, index),
> > VGT_PVINFO_PAGE);
> > > > > >  	}
> > > > > >
> > > > > > +	intel_gvt_hypervisor_read_gpa(s->vgpu,
> > > > > > +		s->workload->ring_context_gpa + 12, &ctx_sr_ctl, 4);
> > > > > > +	if (IS_KABYLAKE(s->vgpu->gvt->dev_priv) &&
> > > > > > +			(ctx_sr_ctl & 1) &&
> > > > > > +			intel_gvt_mmio_is_in_ctx(gvt, offset) &&
> > > > > > +			!strncmp(cmd, "lri", 3)) {
> > > > > > +		u32 data = cmd_val(s, index + 1);
> > > > >
> > > > > We can first check 'lri' cmd, then might need to read ctx ctl
> > > > > state with further checking, instead of always reading gpa. And
> > > > > why need to check for KBL only? I know currently only KBL is
> > > > > applied by LRI cmd for context s/r, but this checking for lri cmd
> > > > > update on vreg should still be applying for supported hw, right?
> > > Henry: I will move reading gpa after checking lri in next version.
> > > This patch just wants to fix the issue introduced by the patch "init
> > > mmio by lri command in vgpu inhibit context" which only works on KBL.
> > > So this patch's working scope has many limitations to avoid regression
> > > to other good cases. For a general vreg update solution for gpu
> > > command in context and ring buffer, right now don't see any bugs by
> > > missing it and I understand needs to consider more situations like lrm
> > command and batch buffer. So it is not implemented in this patch.
> > 
> > Then make a warning in that case instead of hiding the situation so can be
> > awared later.
> > Better with some comments for that too.
> > 
> > >
> > > > > > +
> > > > > > +		if (intel_gvt_mmio_has_mode_mask(s->vgpu->gvt,
> > offset))
> > > > > > +			intel_vgpu_mask_mmio_write(vgpu, offset,
> > &data, 4);
> > > >
> > > > And why only update for mmio with mode mask?
> > > Henry: from the debugging, only the register at offset 0x7014 needs to
> > > be handled, which is a mask-write register. But I agree it's better to
> > > consider other mmio as well. I will update in next version.
> 
> We can add is_mask_reg check, all the reg with mask field need to use mask-write API for update.
Henry: the patch already uses API intel_gvt_mmio_has_mode_mask to check MMIO. Do you mean any other place to check?