diff mbox

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

Message ID 1404875063-27424-1-git-send-email-decui@microsoft.com (mailing list archive)
State New, archived
Headers show

Commit Message

Dexuan Cui July 9, 2014, 3:04 a.m. UTC
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.

v2: removed the MS-TFS line in the commit message
v3: remove some 'unlikely' markings

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(-)

Comments

Dexuan Cui July 24, 2014, 1:54 a.m. UTC | #1
Ping again -- any one has further comment for the v3 version?

It looks the framebuffer layer's Tomi and Jean-Christophe are out recently?
Recently I don't see mail from them since Jul 1 and Jul 8 in the lists.

Though the patch belongs to driver/video/fbdev/, it only changes hyper-v stuff
and only changes one file and the patch itself is straightforward IMO.

So, hi Greg and all, 
If you think the patch itself is OK, may I know if it's OK for the patch to go
into Greg's char-misc.git tree, as some other hyper-v patches did?

Please let me know what's the next step I should do. 
I appreciate your reply.

Thanks!

-- Dexuan

> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
> owner@vger.kernel.org] On Behalf Of Dexuan Cui
> Sent: Wednesday, July 9, 2014 11:04 AM
> To: gregkh@linuxfoundation.org; dan.carpenter@oracle.com; linux-
> kernel@vger.kernel.org; driverdev-devel@linuxdriverproject.org;
> plagnioj@jcrosoft.com; tomi.valkeinen@ti.com; linux-fbdev@vger.kernel.org
> Cc: olaf@aepfle.de; apw@canonical.com; jasowang@redhat.com; KY
> Srinivasan; Haiyang Zhang
> Subject: [PATCH v3] video: hyperv: hyperv_fb: refresh the VM screen by force
> on VM panic
> 
> 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.
> 
> v2: removed the MS-TFS line in the commit message
> v3: remove some 'unlikely' markings
> 
> 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..a7b98e1 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 (synchronous_fb)
> +		synthvid_update(p);
> +}
> +
> +static void hvfb_cfb_copyarea(struct fb_info *p,
> +				const struct fb_copyarea *area)
> +{
> +	cfb_copyarea(p, area);
> +
> +	if (synchronous_fb)
> +		synthvid_update(p);
> +}
> +
> +static void hvfb_cfb_imageblit(struct fb_info *p,
> +				const struct fb_image *image)
> +{
> +	cfb_imageblit(p, image);
> +
> +	if (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
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Greg Kroah-Hartman July 24, 2014, 3:53 a.m. UTC | #2
On Thu, Jul 24, 2014 at 01:54:58AM +0000, Dexuan Cui wrote:
> Ping again -- any one has further comment for the v3 version?
> 
> It looks the framebuffer layer's Tomi and Jean-Christophe are out recently?
> Recently I don't see mail from them since Jul 1 and Jul 8 in the lists.
> 
> Though the patch belongs to driver/video/fbdev/, it only changes hyper-v stuff
> and only changes one file and the patch itself is straightforward IMO.
> 
> So, hi Greg and all, 
> If you think the patch itself is OK, may I know if it's OK for the patch to go
> into Greg's char-misc.git tree, as some other hyper-v patches did?

No, it needs to go through the fb tree, not mine, sorry.

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dexuan Cui July 25, 2014, 7:15 a.m. UTC | #3
> -----Original Message-----
> From: gregkh@linuxfoundation.org [mailto:gregkh@linuxfoundation.org]
> Sent: Thursday, July 24, 2014 11:53 AM
> > So, hi Greg and all,
> > If you think the patch itself is OK, may I know if it's OK for the patch to go
> > into Greg's char-misc.git tree, as some other hyper-v patches did?
> 
> No, it needs to go through the fb tree, not mine, sorry.
> 
> greg k-h

Hi Greg,
Thank you very much for the clarification!
It's clear to me now.
Let me just wait the framebuffer maintainers' reply. :-)

Thanks,
-- Dexuan
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomi Valkeinen July 30, 2014, 2:24 p.m. UTC | #4
On 09/07/14 06:04, 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.
> 
> v2: removed the MS-TFS line in the commit message
> v3: remove some 'unlikely' markings
> 
> 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..a7b98e1 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 variables like these are usually a no-no. This prevents you from
having multiple device instances.

>  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;
> +

Same comment here.

However, if (and only if) the driver is already designed to work only
with single device instance, then this patch is probably ok. But even
then, I'd prefer this to be handled without static variables so that the
driver could eventually be changed to support multiple device instances.

 Tomi
Dexuan Cui July 31, 2014, 10:11 a.m. UTC | #5
> -----Original Message-----
> From: Tomi Valkeinen [mailto:tomi.valkeinen@ti.com]
> Sent: Wednesday, July 30, 2014 22:24 PM
> > +static struct fb_info *hvfb_info;
> 
> Static variables like these are usually a no-no. This prevents you from
> having multiple device instances.
I agree.

> >  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;
> > +
> 
> Same comment here.
> 
> However, if (and only if) the driver is already designed to work only
> with single device instance, then this patch is probably ok. But even
IMO the host should only provide at most 1 synthetic video device to a VM. :-)

> then, I'd prefer this to be handled without static variables so that the
> driver could eventually be changed to support multiple device instances.

Hi Tomi,
Maybe we can remove these static stuff:
+static struct fb_info *hvfb_info;
+static struct notifier_block hvfb_panic_nb = {
+	.notifier_call = hvfb_on_panic,
+};
by kmalloc()-ing  a new struct:
struct hv_fb_panic_nb {
        struct fb_info *hvfb_info;
        struct notifier_block nb;
}
?
I think in hvfb_on_panic() we should be able to get the
hvfb_info pointer by
hvfb_info = container_of(nb, struct hv_fb_panic_nb, nb).

If you like that or have a better idea, please let me know so
I can make a new patch.

Thanks,
-- Dexuan
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomi Valkeinen July 31, 2014, 1:38 p.m. UTC | #6
On 31/07/14 13:11, Dexuan Cui wrote:
>> -----Original Message-----
>> From: Tomi Valkeinen [mailto:tomi.valkeinen@ti.com]
>> Sent: Wednesday, July 30, 2014 22:24 PM
>>> +static struct fb_info *hvfb_info;
>>
>> Static variables like these are usually a no-no. This prevents you from
>> having multiple device instances.
> I agree.
> 
>>>  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;
>>> +
>>
>> Same comment here.
>>
>> However, if (and only if) the driver is already designed to work only
>> with single device instance, then this patch is probably ok. But even
> IMO the host should only provide at most 1 synthetic video device to a VM. :-)
> 
>> then, I'd prefer this to be handled without static variables so that the
>> driver could eventually be changed to support multiple device instances.
> 
> Hi Tomi,
> Maybe we can remove these static stuff:
> +static struct fb_info *hvfb_info;
> +static struct notifier_block hvfb_panic_nb = {
> +	.notifier_call = hvfb_on_panic,
> +};
> by kmalloc()-ing  a new struct:
> struct hv_fb_panic_nb {
>         struct fb_info *hvfb_info;
>         struct notifier_block nb;
> }
> ?
> I think in hvfb_on_panic() we should be able to get the
> hvfb_info pointer by
> hvfb_info = container_of(nb, struct hv_fb_panic_nb, nb).
> 
> If you like that or have a better idea, please let me know so
> I can make a new patch.

To be honest, I haven't been using notifiers much. But the above looks
ok to me.

Or maybe you can add the notifier_block and the synchronous_fb to
hvfb_par? From the notifier_block pointer you could then get hvfp_par,
and from there hvfb_info.

 Tomi
Dexuan Cui Aug. 1, 2014, 10:22 a.m. UTC | #7
> -----Original Message-----
> From: Tomi Valkeinen [mailto:tomi.valkeinen@ti.com]
> Sent: Thursday, July 31, 2014 21:38 PM
> > I think in hvfb_on_panic() we should be able to get the
> > hvfb_info pointer by
> > hvfb_info = container_of(nb, struct hv_fb_panic_nb, nb).
> >
> > If you like that or have a better idea, please let me know so
> > I can make a new patch.
> 
> Or maybe you can add the notifier_block and the synchronous_fb to
> hvfb_par? From the notifier_block pointer you could then get hvfp_par,
> and from there hvfb_info.
>  Tomi

Hi Tomi,
Thanks a lot for the suggestion! It's better.
I'll send out a v4 patch for your review.

Thanks,
-- Dexuan

--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
index e23392e..a7b98e1 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 (synchronous_fb)
+		synthvid_update(p);
+}
+
+static void hvfb_cfb_copyarea(struct fb_info *p,
+				const struct fb_copyarea *area)
+{
+	cfb_copyarea(p, area);
+
+	if (synchronous_fb)
+		synthvid_update(p);
+}
+
+static void hvfb_cfb_imageblit(struct fb_info *p,
+				const struct fb_image *image)
+{
+	cfb_imageblit(p, image);
+
+	if (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;