drm/i915/gvt: Make MI_USER_INTERRUPT nop in cmd parser

Submitted by Zhipeng Gong on March 22, 2018, 1:44 a.m.

Details

Message ID 1521683046-26390-1-git-send-email-zhipeng.gong@intel.com
State New
Headers show
Series "drm/i915/gvt: Make MI_USER_INTERRUPT nop in cmd parser" ( rev: 1 ) in Intel GVT devel

Not browsing as part of any series.

Commit Message

Zhipeng Gong March 22, 2018, 1:44 a.m.
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

Signed-off-by: Zhipeng Gong <zhipeng.gong@intel.com>
---
 drivers/gpu/drm/i915/gvt/cmd_parser.c | 1 +
 1 file changed, 1 insertion(+)

Patch hide | download patch | download mbox

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);
 	return 0;
 }
 

Comments

Zhenyu and Zhi

Could you please review this patch?

Thanks
Zhipeng
> -----Original Message-----
> From: Gong, Zhipeng
> Sent: Thursday, March 22, 2018 9:44 AM
> To: intel-gvt-dev@lists.freedesktop.org
> Cc: Gong, Zhipeng <zhipeng.gong@intel.com>; He, Min <min.he@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
> 
> Signed-off-by: Zhipeng Gong <zhipeng.gong@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);
>  	return 0;
>  }
> 
> --
> 2.7.4
> From: Gong, Zhipeng

> Sent: Monday, March 26, 2018 10:53 AM

 
> Zhenyu and Zhi

> 

> Could you please review this patch?

> 

> Thanks

> Zhipeng

> > -----Original Message-----

> > From: Gong, Zhipeng

> > Sent: Thursday, March 22, 2018 9:44 AM

> > To: intel-gvt-dev@lists.freedesktop.org

> > Cc: Gong, Zhipeng <zhipeng.gong@intel.com>; He, Min

> <min.he@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?

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?

> >

> > Signed-off-by: Zhipeng Gong <zhipeng.gong@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);

> >  	return 0;

> >  }

> >

> > --

> > 2.7.4

> 

> _______________________________________________

> intel-gvt-dev mailing list

> intel-gvt-dev@lists.freedesktop.org

> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
> -----Original Message-----

> From: Tian, Kevin

> Sent: Monday, March 26, 2018 11:08 AM

> To: Gong, Zhipeng <zhipeng.gong@intel.com>; Zhenyu Wang

> <zhenyuw@linux.intel.com>; Wang, Zhi A <zhi.a.wang@intel.com>

> Cc: He, Min <min.he@intel.com>; intel-gvt-dev@lists.freedesktop.org

> Subject: RE: [PATCH] drm/i915/gvt: Make MI_USER_INTERRUPT nop in cmd

> parser

> 

> > From: Gong, Zhipeng

> > Sent: Monday, March 26, 2018 10:53 AM

> 

> > Zhenyu and Zhi

> >

> > Could you please review this patch?

> >

> > Thanks

> > Zhipeng

> > > -----Original Message-----

> > > From: Gong, Zhipeng

> > > Sent: Thursday, March 22, 2018 9:44 AM

> > > To: intel-gvt-dev@lists.freedesktop.org

> > > Cc: Gong, Zhipeng <zhipeng.gong@intel.com>; He, Min

> > <min.he@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. 

> > >

> > > Signed-off-by: Zhipeng Gong <zhipeng.gong@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);

> > >  	return 0;

> > >  }

> > >

> > > --

> > > 2.7.4

> >

> > _______________________________________________

> > intel-gvt-dev mailing list

> > intel-gvt-dev@lists.freedesktop.org

> > https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
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@lists.freedesktop.org
> > > > Cc: Gong, Zhipeng <zhipeng.gong@intel.com>; He, Min
> > > <min.he@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@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@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev