Re: [PATCH] video: hyperv: hyperv_fb: refresh the VM screen by force on VM panic

Re: [PATCH] video: hyperv: hyperv_fb: refresh the VM screen by force on VM panic

[Greg KH]

On Tue, Jun 24, 2014 at 08:29:17AM +0800, Dexuan Cui wrote:
> Currently the VSC has no chance to notify the VSP of the dirty rectangle on VM
> panic because the notification work is done in a workqueue, and in panic() the
> kernel typically ends up in an infinite loop, and a typical kernel config has
> CONFIG_PREEMPT_VOLUNTARY=y and CONFIG_PREEMPT is not set, so a context switch
> can't happen in panic() and the workqueue won't have a chance to run. As a
> result, the VM Connection window can't refresh until it's closed and we
> re-connect to the VM.
> 
> We can register a handler on panic_notifier_list: the handler can notify
> the VSC and switch the framebuffer driver to a "synchronous mode", meaning
> the VSC flushes any future framebuffer change to the VSP immediately.
> 
> MS-TFS: 157532

What is this line for?

thanks,

greg k-h

RE: [PATCH] video: hyperv: hyperv_fb: refresh the VM screen by force on VM panic

[Dexuan Cui]

>On Tue, Jun 24, 2014 at 08:29:17AM +0800, Dexuan Cui wrote:
>> Currently the VSC has no chance to notify the VSP of the dirty rectangle on VM
>> panic because the notification work is done in a workqueue, and in panic() the
>> kernel typically ends up in an infinite loop, and a typical kernel config has
>> CONFIG_PREEMPT_VOLUNTARY=y and CONFIG_PREEMPT is not set, so a context switch
>> can't happen in panic() and the workqueue won't have a chance to run. As a
>> result, the VM Connection window can't refresh until it's closed and we
>> re-connect to the VM.
>> 
>> We can register a handler on panic_notifier_list: the handler can notify
>> the VSC and switch the framebuffer driver to a "synchronous mode", meaning
>> the VSC flushes any future framebuffer change to the VSP immediately.
>> 
>> MS-TFS: 157532

> What is this line for?

Hi Greg,
This line is for our internal bug repository.
We have an automated system to correlate bugs with fixes so that our test
team knows when a bug fix has been accepted upstream and they need to
write a new test case for it. 

The MS-TFS line has appeared in the commit description for a while if you
search for it in 'git log'  of linux-next.

Please let us know if you have further comments.

BTW, it looks the previous patch mail was not sent properly so only part of
the people received it. I'll re-send it. Sorry for the possible confusion.

Thanks,
-- Dexuan

[PATCH] video: hyperv: hyperv_fb: refresh the VM screen by force on VM panic

[Dexuan Cui]

Currently the VSC has no chance to notify the VSP of the dirty rectangle on VM
panic because the notification work is done in a workqueue, and in panic() the
kernel typically ends up in an infinite loop, and a typical kernel config has
CONFIG_PREEMPT_VOLUNTARY=y and CONFIG_PREEMPT is not set, so a context switch
can't happen in panic() and the workqueue won't have a chance to run. As a
result, the VM Connection window can't refresh until it's closed and we
re-connect to the VM.

We can register a handler on panic_notifier_list: the handler can notify
the VSC and switch the framebuffer driver to a "synchronous mode", meaning
the VSC flushes any future framebuffer change to the VSP immediately.
MS-TFS: 157532

Signed-off-by: Dexuan Cui <decui@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
---
 drivers/video/fbdev/hyperv_fb.c | 58 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 55 insertions(+), 3 deletions(-)

diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
index e23392e..291d171 100644
--- a/drivers/video/fbdev/hyperv_fb.c
+++ b/drivers/video/fbdev/hyperv_fb.c
@@ -226,11 +226,16 @@ struct hvfb_par {
 	u8 recv_buf[MAX_VMBUS_PKT_SIZE];
 };
 
+static struct fb_info *hvfb_info;
+
 static uint screen_width = HVFB_WIDTH;
 static uint screen_height = HVFB_HEIGHT;
 static uint screen_depth;
 static uint screen_fb_size;
 
+/* If true, the VSC notifies the VSP on every framebuffer change */
+static bool synchronous_fb;
+
 /* Send message to Hyper-V host */
 static inline int synthvid_send(struct hv_device *hdev,
 				struct synthvid_msg *msg)
@@ -532,6 +537,20 @@ static void hvfb_update_work(struct work_struct *w)
 		schedule_delayed_work(&par->dwork, HVFB_UPDATE_DELAY);
 }
 
+static int hvfb_on_panic(struct notifier_block *nb,
+			unsigned long e, void *p)
+{
+	if (hvfb_info)
+		synthvid_update(hvfb_info);
+
+	synchronous_fb = true;
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block hvfb_panic_nb = {
+	.notifier_call = hvfb_on_panic,
+};
 
 /* Framebuffer operation handlers */
 
@@ -582,14 +601,41 @@ static int hvfb_blank(int blank, struct fb_info *info)
 	return 1;	/* get fb_blank to set the colormap to all black */
 }
 
+static void hvfb_cfb_fillrect(struct fb_info *p,
+				const struct fb_fillrect *rect)
+{
+	cfb_fillrect(p, rect);
+
+	if (unlikely(synchronous_fb))
+		synthvid_update(p);
+}
+
+static void hvfb_cfb_copyarea(struct fb_info *p,
+				const struct fb_copyarea *area)
+{
+	cfb_copyarea(p, area);
+
+	if (unlikely(synchronous_fb))
+		synthvid_update(p);
+}
+
+static void hvfb_cfb_imageblit(struct fb_info *p,
+				const struct fb_image *image)
+{
+	cfb_imageblit(p, image);
+
+	if (unlikely(synchronous_fb))
+		synthvid_update(p);
+}
+
 static struct fb_ops hvfb_ops = {
 	.owner = THIS_MODULE,
 	.fb_check_var = hvfb_check_var,
 	.fb_set_par = hvfb_set_par,
 	.fb_setcolreg = hvfb_setcolreg,
-	.fb_fillrect = cfb_fillrect,
-	.fb_copyarea = cfb_copyarea,
-	.fb_imageblit = cfb_imageblit,
+	.fb_fillrect = hvfb_cfb_fillrect,
+	.fb_copyarea = hvfb_cfb_copyarea,
+	.fb_imageblit = hvfb_cfb_imageblit,
 	.fb_blank = hvfb_blank,
 };
 
@@ -801,6 +847,9 @@ static int hvfb_probe(struct hv_device *hdev,
 
 	par->fb_ready = true;
 
+	hvfb_info = info;
+	atomic_notifier_chain_register(&panic_notifier_list, &hvfb_panic_nb);
+
 	return 0;
 
 error:
@@ -820,6 +869,9 @@ static int hvfb_remove(struct hv_device *hdev)
 	struct fb_info *info = hv_get_drvdata(hdev);
 	struct hvfb_par *par = info->par;
 
+	atomic_notifier_chain_unregister(&panic_notifier_list, &hvfb_panic_nb);
+	hvfb_info = NULL;
+
 	par->update = false;
 	par->fb_ready = false;
 
-- 
1.9.1

Re: [PATCH] video: hyperv: hyperv_fb: refresh the VM screen by force on VM panic

[Greg KH]

On Tue, Jun 24, 2014 at 09:44:14PM +0000, Dexuan Cui wrote:
> >On Tue, Jun 24, 2014 at 08:29:17AM +0800, Dexuan Cui wrote:
> >> Currently the VSC has no chance to notify the VSP of the dirty rectangle on VM
> >> panic because the notification work is done in a workqueue, and in panic() the
> >> kernel typically ends up in an infinite loop, and a typical kernel config has
> >> CONFIG_PREEMPT_VOLUNTARY=y and CONFIG_PREEMPT is not set, so a context switch
> >> can't happen in panic() and the workqueue won't have a chance to run. As a
> >> result, the VM Connection window can't refresh until it's closed and we
> >> re-connect to the VM.
> >> 
> >> We can register a handler on panic_notifier_list: the handler can notify
> >> the VSC and switch the framebuffer driver to a "synchronous mode", meaning
> >> the VSC flushes any future framebuffer change to the VSP immediately.
> >> 
> >> MS-TFS: 157532
> 
> > What is this line for?
> 
> Hi Greg,
> This line is for our internal bug repository.
> We have an automated system to correlate bugs with fixes so that our test
> team knows when a bug fix has been accepted upstream and they need to
> write a new test case for it. 
> 
> The MS-TFS line has appeared in the commit description for a while if you
> search for it in 'git log'  of linux-next.
> 
> Please let us know if you have further comments.

Please don't add marker lines like this that provide no relevancy to
anyone else.  I don't allow gerrit ids for the same reason.  If you want
to refer to a public bug tracker, that's great, otherwise, don't
include it.

greg k-h

search by phrase a-la "MS-TFS: 157532" -- "has relevance for Hyper-V sysadmins" Re: [PATCH] video: h

[Victor Miasnikov]

Hi!

 Short:

 1)
greg k-h> don't add marker lines like this that provide no relevancy to anyone else.

 "no relevancy to anyone else" -- strange: it "has relevance for Hyper-V sysadmins"
For search by keywords in web archives of this mail list

 1b)

greg k-h> If you want to refer to a public bug tracker,
greg k-h> that's great
 
 Yes: public bug tracker are welcom


 Full:

D.C>>>>
D.C>>>> MS-TFS: 157532
D.C>>>>

Greg K-h>>>
Greg K-h>>> What is this line for?
Greg K-h>>>

D.C.>> This line is for our internal bug repository.
D.C.>>
D.C.>> The MS-TFS line has appeared in the commit description for a while if you
D.C.>> search for it in 'git log'  of linux-next.
D.C.>>

greg k-h>
greg k-h> Please
greg k-h> don't add marker lines like this that provide no relevancy to anyone else.

 "no relevancy to anyone else" -- strange: it "has relevance for Hyper-V sysadmins"

For search by keywords in web archives of this mail list

 See example:
Results of search sesssion by phrase "MS-TFS: 157532" :
=
LKML: Dexuan Cui: RE: [PATCH] video: hyperv: hyperv_fb ...
lkml.org/lkml/2014/6/24/844
1 day ago - MS-TFS: 157532 > What is this line for? Hi Greg, This line is for our internal bug repository. We have an 
automated system to correlate bugs ...
=
 +
=
[PATCH] video: hyperv: hyperv_fb: refresh the VM screen by force ...
www.gossamer-threads.com/lists/linux/kernel/1952989
5 hours ago - the VSC flushes any future framebuffer change to the VSP immediately. 
MS-TFS: 157532. Signed-off-by: Dexuan Cui
=


greg k-h> I don't allow gerrit ids for the same reason.
greg k-h> If you want to refer to a public bug tracker,
greg k-h> that's great

 
 Yes: public bug tracker are welcom


greg k-h>, otherwise, don't include it.




Best regards, Victor Miasnikov
Blog:  http://vvm.blog.tut.by/


----- Original Message ----- 
From: "Greg KH"
To: "Dexuan Cui"
Cc: . . .
Sent: Thursday, June 26, 2014 6:36 AM
Subject: Re: [PATCH] video: hyperv: hyperv_fb: refresh the VM screen by force on VM panic

On Tue, Jun 24, 2014 at 09:44:14PM +0000, Dexuan Cui wrote:
> >On Tue, Jun 24, 2014 at 08:29:17AM +0800, Dexuan Cui wrote:
> >> Currently the VSC has no chance to notify the VSP of the dirty rectangle on VM
> >> panic because the notification work is done in a workqueue, and in panic() the
> >> kernel typically ends up in an infinite loop, and a typical kernel config has
> >> CONFIG_PREEMPT_VOLUNTARY=y and CONFIG_PREEMPT is not set, so a context switch
> >> can't happen in panic() and the workqueue won't have a chance to run. As a
> >> result, the VM Connection window can't refresh until it's closed and we
> >> re-connect to the VM.
> >>
> >> We can register a handler on panic_notifier_list: the handler can notify
> >> the VSC and switch the framebuffer driver to a "synchronous mode", meaning
> >> the VSC flushes any future framebuffer change to the VSP immediately.
> >>
> >> MS-TFS: 157532
>
> > What is this line for?
>
> Hi Greg,
> This line is for our internal bug repository.
> We have an automated system to correlate bugs with fixes so that our test
> team knows when a bug fix has been accepted upstream and they need to
> write a new test case for it.
>
> The MS-TFS line has appeared in the commit description for a while if you
> search for it in 'git log'  of linux-next.
>
> Please let us know if you have further comments.

Please don't add marker lines like this that provide no relevancy to
anyone else.  I don't allow gerrit ids for the same reason.  If you want
to refer to a public bug tracker, that's great, otherwise, don't
include it.

greg k-h
--

RE: [PATCH] video: hyperv: hyperv_fb: refresh the VM screen by force on VM panic

[Dexuan Cui]

> > >> MS-TFS: 157532
> >
> > > What is this line for?
> >
> > Hi Greg,
> > This line is for our internal bug repository.
> > We have an automated system to correlate bugs with fixes so that our test
> > team knows when a bug fix has been accepted upstream and they need to
> > write a new test case for it.
> >
> > The MS-TFS line has appeared in the commit description for a while if you
> > search for it in 'git log'  of linux-next.
> >
> > Please let us know if you have further comments.
> 
> Please don't add marker lines like this that provide no relevancy to
> anyone else.  I don't allow gerrit ids for the same reason.  If you want
> to refer to a public bug tracker, that's great, otherwise, don't
> include it.
> 
> greg k-h

Ok, I'll remove the line and re-submit a v2 patch(the patch itself will be
the same).

Thanks,
-- Dexuan