[PATCH] drm/i915/gvt: Make MI_USER_INTERRUPT nop in cmd parser

[Zhenyu Wang]

On 2018.03.26 05:50:48 +0000, Gong, Zhipeng wrote:
> > > > -----Original Message-----
> > > > From: Gong, Zhipeng
> > > > Sent: Thursday, March 22, 2018 9:44 AM
> > > > To: intel-gvt-dev at lists.freedesktop.org
> > > > Cc: Gong, Zhipeng <zhipeng.gong at intel.com>; He, Min
> > > <min.he at intel.com>
> > > > Subject: [PATCH] drm/i915/gvt: Make MI_USER_INTERRUPT nop in cmd
> > > parser
> > > >
> > > > GVT-g cmd parser passes through MI_USER_INTERRUPT instruction, while
> > > > i915 adds MI_USER_INTERRUPT again for each request from GVT-g,
> > > > which causes some unnecessary interrupt handling.
> > > > This patch makes MI_USER_INTERRUPT nop to save some interrupt
> > > > handling.
> > > >
> > > > Here is test result to run glmark2 on guest:
> > > > host master interrupts number is reduced from 16021 to 11162
> > > > host user interrupts number is reduced from 7936 to 3536
> > 
> > the number is a nice improvement. however simply counting
> > on current knowledge may get GVT-g broken in case future
> > i915 is changed to not use MI_USER_INTERRUPT (e.g. using
> > PIPE_CONTROL). Is there a way that we can detect and then
> > optionally do patching only in case the assumption holds?
> gvt-g dispatches request to host i915 and depends on i915 notify 
> ring interrupt mechanism to check completion of request. 
> gvt-g won't get broken if i915 is changed to not use MI_USER_INTERRUPT.
> will revise the commit message.
> 
> > another thought is, what about guest cmd buffer contains
> > multiple MI_USER_INTERRUPT then nop all of them may
> > cause some latency issue (if one of them is in middle of
> > execution)? should we restrict nop to last one only?
> If several requests from guest are combined into one request in gvt-g 
> and contain MI_USER_INTERRUPT in the middle of combined request.
> gvt-g still has to wait on the whole request to complete to inject
> user interrupt to guest. It won't cause the latency issue. 
>

My gut feeling is that this might be ok for now, as we don't have
accurate handling of user interrupt for vGPU anyway, which needs to
ask for monitor i915 ring change. So no-op it doesn't do more badness...

> > > >
> > > > Signed-off-by: Zhipeng Gong <zhipeng.gong at intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/gvt/cmd_parser.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > > b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > > index febb814..ab4a670 100644
> > > > --- a/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > > +++ b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > > @@ -1064,6 +1064,7 @@ static int
> > > cmd_handler_mi_user_interrupt(struct
> > > > parser_exec_state *s)
> > > >  {
> > > >  	set_bit(cmd_interrupt_events[s->ring_id].mi_user_interrupt,
> > > >  			s->workload->pending_events);
> > > > +	patch_value(s, cmd_ptr(s, 0), MI_NOOP);

At least put a comment to tell the reason for no surprise in future.

> > > >  	return 0;
> > > >  }
> > > >
> > > > --
> > > > 2.7.4
> > >
> > > _______________________________________________
> > > intel-gvt-dev mailing list
> > > intel-gvt-dev at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
> _______________________________________________
> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/intel-gvt-dev/attachments/20180326/864beb10/attachment.sig>