2020-11-21 01:49:50

by Dexuan Cui

[permalink] [raw]
Subject: [PATCH] video: hyperv_fb: Directly use the MMIO VRAM

Late in 2019, 2 commits (see the 2 Fixes tags) were introduced to
mitigate the slow framebuffer issue. Now that we have fixed the
slowness issue by correctly mapping the MMIO VRAM (see
commit 5f1251a48c17 ("video: hyperv_fb: Fix the cache type when mapping
the VRAM")), we can remove most of the code introduced by the 2 commits,
i.e. we no longer need to allocate physical memory and use it to back up
the VRAM in Generation-1 VM, and we also no longer need to allocate
physical memory to back up the framebuffer in a Generation-2 VM and copy
the framebuffer to the real VRAM.

synthvid_deferred_io() is kept, because it's still desirable to send the
SYNTHVID_DIRT message only for the exact dirty rectangle, and only when
needed.

Fixes: d21987d709e8 ("video: hyperv: hyperv_fb: Support deferred IO for Hyper-V frame buffer driver")
Fixes: 3a6fb6c4255c ("video: hyperv: hyperv_fb: Use physical memory for fb on HyperV Gen 1 VMs.")
Cc: Wei Hu <[email protected]>
Cc: Boqun Feng <[email protected]>
Signed-off-by: Dexuan Cui <[email protected]>
---

This patch changes drivers/video/fbdev/Kconfig, but I hope this can
still go through the Hyper-V tree
https://git.kernel.org/pub/scm/linux/kernel/git/hyperv/linux.git/log/?h=hyperv-next
because it's unlikely to cause any build issue to other fbdev drivers
(that line was introduced by 3a6fb6c4255c only for hyperv_fb.c)

Note: this patch is based on the Hyper-V tree's hyperv-fixes branch, but
it should also apply cleanly to the branch hyperv-next if the commit
5f1251a48c17 is applied first. This patch is for v5.11 rather than
v5.10.

drivers/video/fbdev/Kconfig | 1 -
drivers/video/fbdev/hyperv_fb.c | 170 ++------------------------------
2 files changed, 9 insertions(+), 162 deletions(-)

diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
index 402e85450bb5..05b37fb3c6d6 100644
--- a/drivers/video/fbdev/Kconfig
+++ b/drivers/video/fbdev/Kconfig
@@ -2205,7 +2205,6 @@ config FB_HYPERV
select FB_CFB_COPYAREA
select FB_CFB_IMAGEBLIT
select FB_DEFERRED_IO
- select DMA_CMA if HAVE_DMA_CONTIGUOUS && CMA
help
This framebuffer driver supports Microsoft Hyper-V Synthetic Video.

diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
index 58c74d2356ba..8131f4e66f98 100644
--- a/drivers/video/fbdev/hyperv_fb.c
+++ b/drivers/video/fbdev/hyperv_fb.c
@@ -31,16 +31,6 @@
* "set-vmvideo" command. For example
* set-vmvideo -vmname name -horizontalresolution:1920 \
* -verticalresolution:1200 -resolutiontype single
- *
- * Gen 1 VMs also support direct using VM's physical memory for framebuffer.
- * It could improve the efficiency and performance for framebuffer and VM.
- * This requires to allocate contiguous physical memory from Linux kernel's
- * CMA memory allocator. To enable this, supply a kernel parameter to give
- * enough memory space to CMA allocator for framebuffer. For example:
- * cma=130m
- * This gives 130MB memory to CMA allocator that can be allocated to
- * framebuffer. For reference, 8K resolution (7680x4320) takes about
- * 127MB memory.
*/

#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
@@ -267,16 +257,8 @@ struct hvfb_par {
/* If true, the VSC notifies the VSP on every framebuffer change */
bool synchronous_fb;

- /* If true, need to copy from deferred IO mem to framebuffer mem */
- bool need_docopy;
-
struct notifier_block hvfb_panic_nb;

- /* Memory for deferred IO and frame buffer itself */
- unsigned char *dio_vp;
- unsigned char *mmio_vp;
- phys_addr_t mmio_pp;
-
/* Dirty rectangle, protected by delayed_refresh_lock */
int x1, y1, x2, y2;
bool delayed_refresh;
@@ -405,21 +387,6 @@ synthvid_update(struct fb_info *info, int x1, int y1, int x2, int y2)
return 0;
}

-static void hvfb_docopy(struct hvfb_par *par,
- unsigned long offset,
- unsigned long size)
-{
- if (!par || !par->mmio_vp || !par->dio_vp || !par->fb_ready ||
- size == 0 || offset >= dio_fb_size)
- return;
-
- if (offset + size > dio_fb_size)
- size = dio_fb_size - offset;
-
- memcpy(par->mmio_vp + offset, par->dio_vp + offset, size);
-}
-
-/* Deferred IO callback */
static void synthvid_deferred_io(struct fb_info *p,
struct list_head *pagelist)
{
@@ -444,10 +411,6 @@ static void synthvid_deferred_io(struct fb_info *p,
y2 = end / p->fix.line_length;
miny = min_t(int, miny, y1);
maxy = max_t(int, maxy, y2);
-
- /* Copy from dio space to mmio address */
- if (par->fb_ready && par->need_docopy)
- hvfb_docopy(par, start, PAGE_SIZE);
}

if (par->fb_ready && par->update)
@@ -704,7 +667,7 @@ static int synthvid_send_config(struct hv_device *hdev)
msg->vid_hdr.type = SYNTHVID_VRAM_LOCATION;
msg->vid_hdr.size = sizeof(struct synthvid_msg_hdr) +
sizeof(struct synthvid_vram_location);
- msg->vram.user_ctx = msg->vram.vram_gpa = par->mmio_pp;
+ msg->vram.user_ctx = msg->vram.vram_gpa = info->fix.smem_start;
msg->vram.is_vram_gpa_specified = 1;
synthvid_send(hdev, msg);

@@ -714,7 +677,7 @@ static int synthvid_send_config(struct hv_device *hdev)
ret = -ETIMEDOUT;
goto out;
}
- if (msg->vram_ack.user_ctx != par->mmio_pp) {
+ if (msg->vram_ack.user_ctx != info->fix.smem_start) {
pr_err("Unable to set VRAM location\n");
ret = -ENODEV;
goto out;
@@ -740,7 +703,6 @@ static void hvfb_update_work(struct work_struct *w)
struct fb_info *info = par->info;
unsigned long flags;
int x1, x2, y1, y2;
- int j;

spin_lock_irqsave(&par->delayed_refresh_lock, flags);
/* Reset the request flag */
@@ -762,14 +724,6 @@ static void hvfb_update_work(struct work_struct *w)
y1 > info->var.yres || y2 > info->var.yres || x2 <= x1)
return;

- /* Copy the dirty rectangle to frame buffer memory */
- if (par->need_docopy)
- for (j = y1; j < y2; j++)
- hvfb_docopy(par,
- j * info->fix.line_length +
- (x1 * screen_depth / 8),
- (x2 - x1) * screen_depth / 8);
-
/* Refresh */
if (par->fb_ready && par->update)
synthvid_update(info, x1, y1, x2, y2);
@@ -813,8 +767,6 @@ static int hvfb_on_panic(struct notifier_block *nb,
par = container_of(nb, struct hvfb_par, hvfb_panic_nb);
par->synchronous_fb = true;
info = par->info;
- if (par->need_docopy)
- hvfb_docopy(par, 0, dio_fb_size);
synthvid_update(info, 0, 0, INT_MAX, INT_MAX);

return NOTIFY_DONE;
@@ -953,62 +905,6 @@ static void hvfb_get_option(struct fb_info *info)
return;
}

-/*
- * Allocate enough contiguous physical memory.
- * Return physical address if succeeded or -1 if failed.
- */
-static phys_addr_t hvfb_get_phymem(struct hv_device *hdev,
- unsigned int request_size)
-{
- struct page *page = NULL;
- dma_addr_t dma_handle;
- void *vmem;
- phys_addr_t paddr = 0;
- unsigned int order = get_order(request_size);
-
- if (request_size == 0)
- return -1;
-
- if (order < MAX_ORDER) {
- /* Call alloc_pages if the size is less than 2^MAX_ORDER */
- page = alloc_pages(GFP_KERNEL | __GFP_ZERO, order);
- if (!page)
- return -1;
-
- paddr = (page_to_pfn(page) << PAGE_SHIFT);
- } else {
- /* Allocate from CMA */
- hdev->device.coherent_dma_mask = DMA_BIT_MASK(64);
-
- vmem = dma_alloc_coherent(&hdev->device,
- round_up(request_size, PAGE_SIZE),
- &dma_handle,
- GFP_KERNEL | __GFP_NOWARN);
-
- if (!vmem)
- return -1;
-
- paddr = virt_to_phys(vmem);
- }
-
- return paddr;
-}
-
-/* Release contiguous physical memory */
-static void hvfb_release_phymem(struct hv_device *hdev,
- phys_addr_t paddr, unsigned int size)
-{
- unsigned int order = get_order(size);
-
- if (order < MAX_ORDER)
- __free_pages(pfn_to_page(paddr >> PAGE_SHIFT), order);
- else
- dma_free_coherent(&hdev->device,
- round_up(size, PAGE_SIZE),
- phys_to_virt(paddr),
- paddr);
-}
-

/* Get framebuffer memory from Hyper-V video pci space */
static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info)
@@ -1018,7 +914,6 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info)
void __iomem *fb_virt;
int gen2vm = efi_enabled(EFI_BOOT);
resource_size_t pot_start, pot_end;
- phys_addr_t paddr;
int ret;

info->apertures = alloc_apertures(1);
@@ -1036,38 +931,12 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info)

info->apertures->ranges[0].base = pci_resource_start(pdev, 0);
info->apertures->ranges[0].size = pci_resource_len(pdev, 0);
-
- /*
- * For Gen 1 VM, we can directly use the contiguous memory
- * from VM. If we succeed, deferred IO happens directly
- * on this allocated framebuffer memory, avoiding extra
- * memory copy.
- */
- paddr = hvfb_get_phymem(hdev, screen_fb_size);
- if (paddr != (phys_addr_t) -1) {
- par->mmio_pp = paddr;
- par->mmio_vp = par->dio_vp = __va(paddr);
-
- info->fix.smem_start = paddr;
- info->fix.smem_len = screen_fb_size;
- info->screen_base = par->mmio_vp;
- info->screen_size = screen_fb_size;
-
- par->need_docopy = false;
- goto getmem_done;
- }
- pr_info("Unable to allocate enough contiguous physical memory on Gen 1 VM. Using MMIO instead.\n");
} else {
info->apertures->ranges[0].base = screen_info.lfb_base;
info->apertures->ranges[0].size = screen_info.lfb_size;
}

- /*
- * Cannot use the contiguous physical memory.
- * Allocate mmio space for framebuffer.
- */
- dio_fb_size =
- screen_width * screen_height * screen_depth / 8;
+ dio_fb_size = screen_width * screen_height * screen_depth / 8;

if (gen2vm) {
pot_start = 0;
@@ -1101,22 +970,11 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info)
if (!fb_virt)
goto err2;

- /* Allocate memory for deferred IO */
- par->dio_vp = vzalloc(round_up(dio_fb_size, PAGE_SIZE));
- if (par->dio_vp == NULL)
- goto err3;
-
- /* Physical address of FB device */
- par->mmio_pp = par->mem->start;
- /* Virtual address of FB device */
- par->mmio_vp = (unsigned char *) fb_virt;
-
info->fix.smem_start = par->mem->start;
info->fix.smem_len = dio_fb_size;
- info->screen_base = par->dio_vp;
+ info->screen_base = fb_virt;
info->screen_size = dio_fb_size;

-getmem_done:
remove_conflicting_framebuffers(info->apertures,
KBUILD_MODNAME, false);
if (!gen2vm)
@@ -1125,8 +983,6 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info)

return 0;

-err3:
- iounmap(fb_virt);
err2:
vmbus_free_mmio(par->mem->start, screen_fb_size);
par->mem = NULL;
@@ -1139,19 +995,12 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info)
}

/* Release the framebuffer */
-static void hvfb_putmem(struct hv_device *hdev, struct fb_info *info)
+static void hvfb_putmem(struct fb_info *info)
{
struct hvfb_par *par = info->par;

- if (par->need_docopy) {
- vfree(par->dio_vp);
- iounmap(info->screen_base);
- vmbus_free_mmio(par->mem->start, screen_fb_size);
- } else {
- hvfb_release_phymem(hdev, info->fix.smem_start,
- screen_fb_size);
- }
-
+ iounmap(info->screen_base);
+ vmbus_free_mmio(par->mem->start, screen_fb_size);
par->mem = NULL;
}

@@ -1170,7 +1019,6 @@ static int hvfb_probe(struct hv_device *hdev,
par = info->par;
par->info = info;
par->fb_ready = false;
- par->need_docopy = true;
init_completion(&par->wait);
INIT_DELAYED_WORK(&par->dwork, hvfb_update_work);

@@ -1256,7 +1104,7 @@ static int hvfb_probe(struct hv_device *hdev,

error:
fb_deferred_io_cleanup(info);
- hvfb_putmem(hdev, info);
+ hvfb_putmem(info);
error2:
vmbus_close(hdev->channel);
error1:
@@ -1286,7 +1134,7 @@ static int hvfb_remove(struct hv_device *hdev)
vmbus_close(hdev->channel);
hv_set_drvdata(hdev, NULL);

- hvfb_putmem(hdev, info);
+ hvfb_putmem(info);
framebuffer_release(info);

return 0;
--
2.19.1


2020-11-21 14:58:59

by Boqun Feng

[permalink] [raw]
Subject: Re: [PATCH] video: hyperv_fb: Directly use the MMIO VRAM

Hi Dexuan,

On Fri, Nov 20, 2020 at 05:45:47PM -0800, Dexuan Cui wrote:
> Late in 2019, 2 commits (see the 2 Fixes tags) were introduced to
> mitigate the slow framebuffer issue. Now that we have fixed the
> slowness issue by correctly mapping the MMIO VRAM (see
> commit 5f1251a48c17 ("video: hyperv_fb: Fix the cache type when mapping
> the VRAM")), we can remove most of the code introduced by the 2 commits,
> i.e. we no longer need to allocate physical memory and use it to back up
> the VRAM in Generation-1 VM, and we also no longer need to allocate
> physical memory to back up the framebuffer in a Generation-2 VM and copy
> the framebuffer to the real VRAM.
>
> synthvid_deferred_io() is kept, because it's still desirable to send the
> SYNTHVID_DIRT message only for the exact dirty rectangle, and only when
> needed.
>
> Fixes: d21987d709e8 ("video: hyperv: hyperv_fb: Support deferred IO for Hyper-V frame buffer driver")
> Fixes: 3a6fb6c4255c ("video: hyperv: hyperv_fb: Use physical memory for fb on HyperV Gen 1 VMs.")
> Cc: Wei Hu <[email protected]>
> Cc: Boqun Feng <[email protected]>
> Signed-off-by: Dexuan Cui <[email protected]>

After I applied this patch and patch ("video: hyperv_fb: Fix the cache
type when mapping the VRAM") on my development branch (with Michael's
patchset for ARM64 core support on Hyper-V), and everything worked fine.
So feel free to add:

Tested-by: Boqun Feng <[email protected]>

Regards,
Bqoun

> ---
>
> This patch changes drivers/video/fbdev/Kconfig, but I hope this can
> still go through the Hyper-V tree
> https://git.kernel.org/pub/scm/linux/kernel/git/hyperv/linux.git/log/?h=hyperv-next
> because it's unlikely to cause any build issue to other fbdev drivers
> (that line was introduced by 3a6fb6c4255c only for hyperv_fb.c)
>
> Note: this patch is based on the Hyper-V tree's hyperv-fixes branch, but
> it should also apply cleanly to the branch hyperv-next if the commit
> 5f1251a48c17 is applied first. This patch is for v5.11 rather than
> v5.10.
>
> drivers/video/fbdev/Kconfig | 1 -
> drivers/video/fbdev/hyperv_fb.c | 170 ++------------------------------
> 2 files changed, 9 insertions(+), 162 deletions(-)
>
> diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
> index 402e85450bb5..05b37fb3c6d6 100644
> --- a/drivers/video/fbdev/Kconfig
> +++ b/drivers/video/fbdev/Kconfig
> @@ -2205,7 +2205,6 @@ config FB_HYPERV
> select FB_CFB_COPYAREA
> select FB_CFB_IMAGEBLIT
> select FB_DEFERRED_IO
> - select DMA_CMA if HAVE_DMA_CONTIGUOUS && CMA
> help
> This framebuffer driver supports Microsoft Hyper-V Synthetic Video.
>
> diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
> index 58c74d2356ba..8131f4e66f98 100644
> --- a/drivers/video/fbdev/hyperv_fb.c
> +++ b/drivers/video/fbdev/hyperv_fb.c
> @@ -31,16 +31,6 @@
> * "set-vmvideo" command. For example
> * set-vmvideo -vmname name -horizontalresolution:1920 \
> * -verticalresolution:1200 -resolutiontype single
> - *
> - * Gen 1 VMs also support direct using VM's physical memory for framebuffer.
> - * It could improve the efficiency and performance for framebuffer and VM.
> - * This requires to allocate contiguous physical memory from Linux kernel's
> - * CMA memory allocator. To enable this, supply a kernel parameter to give
> - * enough memory space to CMA allocator for framebuffer. For example:
> - * cma=130m
> - * This gives 130MB memory to CMA allocator that can be allocated to
> - * framebuffer. For reference, 8K resolution (7680x4320) takes about
> - * 127MB memory.
> */
>
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> @@ -267,16 +257,8 @@ struct hvfb_par {
> /* If true, the VSC notifies the VSP on every framebuffer change */
> bool synchronous_fb;
>
> - /* If true, need to copy from deferred IO mem to framebuffer mem */
> - bool need_docopy;
> -
> struct notifier_block hvfb_panic_nb;
>
> - /* Memory for deferred IO and frame buffer itself */
> - unsigned char *dio_vp;
> - unsigned char *mmio_vp;
> - phys_addr_t mmio_pp;
> -
> /* Dirty rectangle, protected by delayed_refresh_lock */
> int x1, y1, x2, y2;
> bool delayed_refresh;
> @@ -405,21 +387,6 @@ synthvid_update(struct fb_info *info, int x1, int y1, int x2, int y2)
> return 0;
> }
>
> -static void hvfb_docopy(struct hvfb_par *par,
> - unsigned long offset,
> - unsigned long size)
> -{
> - if (!par || !par->mmio_vp || !par->dio_vp || !par->fb_ready ||
> - size == 0 || offset >= dio_fb_size)
> - return;
> -
> - if (offset + size > dio_fb_size)
> - size = dio_fb_size - offset;
> -
> - memcpy(par->mmio_vp + offset, par->dio_vp + offset, size);
> -}
> -
> -/* Deferred IO callback */
> static void synthvid_deferred_io(struct fb_info *p,
> struct list_head *pagelist)
> {
> @@ -444,10 +411,6 @@ static void synthvid_deferred_io(struct fb_info *p,
> y2 = end / p->fix.line_length;
> miny = min_t(int, miny, y1);
> maxy = max_t(int, maxy, y2);
> -
> - /* Copy from dio space to mmio address */
> - if (par->fb_ready && par->need_docopy)
> - hvfb_docopy(par, start, PAGE_SIZE);
> }
>
> if (par->fb_ready && par->update)
> @@ -704,7 +667,7 @@ static int synthvid_send_config(struct hv_device *hdev)
> msg->vid_hdr.type = SYNTHVID_VRAM_LOCATION;
> msg->vid_hdr.size = sizeof(struct synthvid_msg_hdr) +
> sizeof(struct synthvid_vram_location);
> - msg->vram.user_ctx = msg->vram.vram_gpa = par->mmio_pp;
> + msg->vram.user_ctx = msg->vram.vram_gpa = info->fix.smem_start;
> msg->vram.is_vram_gpa_specified = 1;
> synthvid_send(hdev, msg);
>
> @@ -714,7 +677,7 @@ static int synthvid_send_config(struct hv_device *hdev)
> ret = -ETIMEDOUT;
> goto out;
> }
> - if (msg->vram_ack.user_ctx != par->mmio_pp) {
> + if (msg->vram_ack.user_ctx != info->fix.smem_start) {
> pr_err("Unable to set VRAM location\n");
> ret = -ENODEV;
> goto out;
> @@ -740,7 +703,6 @@ static void hvfb_update_work(struct work_struct *w)
> struct fb_info *info = par->info;
> unsigned long flags;
> int x1, x2, y1, y2;
> - int j;
>
> spin_lock_irqsave(&par->delayed_refresh_lock, flags);
> /* Reset the request flag */
> @@ -762,14 +724,6 @@ static void hvfb_update_work(struct work_struct *w)
> y1 > info->var.yres || y2 > info->var.yres || x2 <= x1)
> return;
>
> - /* Copy the dirty rectangle to frame buffer memory */
> - if (par->need_docopy)
> - for (j = y1; j < y2; j++)
> - hvfb_docopy(par,
> - j * info->fix.line_length +
> - (x1 * screen_depth / 8),
> - (x2 - x1) * screen_depth / 8);
> -
> /* Refresh */
> if (par->fb_ready && par->update)
> synthvid_update(info, x1, y1, x2, y2);
> @@ -813,8 +767,6 @@ static int hvfb_on_panic(struct notifier_block *nb,
> par = container_of(nb, struct hvfb_par, hvfb_panic_nb);
> par->synchronous_fb = true;
> info = par->info;
> - if (par->need_docopy)
> - hvfb_docopy(par, 0, dio_fb_size);
> synthvid_update(info, 0, 0, INT_MAX, INT_MAX);
>
> return NOTIFY_DONE;
> @@ -953,62 +905,6 @@ static void hvfb_get_option(struct fb_info *info)
> return;
> }
>
> -/*
> - * Allocate enough contiguous physical memory.
> - * Return physical address if succeeded or -1 if failed.
> - */
> -static phys_addr_t hvfb_get_phymem(struct hv_device *hdev,
> - unsigned int request_size)
> -{
> - struct page *page = NULL;
> - dma_addr_t dma_handle;
> - void *vmem;
> - phys_addr_t paddr = 0;
> - unsigned int order = get_order(request_size);
> -
> - if (request_size == 0)
> - return -1;
> -
> - if (order < MAX_ORDER) {
> - /* Call alloc_pages if the size is less than 2^MAX_ORDER */
> - page = alloc_pages(GFP_KERNEL | __GFP_ZERO, order);
> - if (!page)
> - return -1;
> -
> - paddr = (page_to_pfn(page) << PAGE_SHIFT);
> - } else {
> - /* Allocate from CMA */
> - hdev->device.coherent_dma_mask = DMA_BIT_MASK(64);
> -
> - vmem = dma_alloc_coherent(&hdev->device,
> - round_up(request_size, PAGE_SIZE),
> - &dma_handle,
> - GFP_KERNEL | __GFP_NOWARN);
> -
> - if (!vmem)
> - return -1;
> -
> - paddr = virt_to_phys(vmem);
> - }
> -
> - return paddr;
> -}
> -
> -/* Release contiguous physical memory */
> -static void hvfb_release_phymem(struct hv_device *hdev,
> - phys_addr_t paddr, unsigned int size)
> -{
> - unsigned int order = get_order(size);
> -
> - if (order < MAX_ORDER)
> - __free_pages(pfn_to_page(paddr >> PAGE_SHIFT), order);
> - else
> - dma_free_coherent(&hdev->device,
> - round_up(size, PAGE_SIZE),
> - phys_to_virt(paddr),
> - paddr);
> -}
> -
>
> /* Get framebuffer memory from Hyper-V video pci space */
> static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info)
> @@ -1018,7 +914,6 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info)
> void __iomem *fb_virt;
> int gen2vm = efi_enabled(EFI_BOOT);
> resource_size_t pot_start, pot_end;
> - phys_addr_t paddr;
> int ret;
>
> info->apertures = alloc_apertures(1);
> @@ -1036,38 +931,12 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info)
>
> info->apertures->ranges[0].base = pci_resource_start(pdev, 0);
> info->apertures->ranges[0].size = pci_resource_len(pdev, 0);
> -
> - /*
> - * For Gen 1 VM, we can directly use the contiguous memory
> - * from VM. If we succeed, deferred IO happens directly
> - * on this allocated framebuffer memory, avoiding extra
> - * memory copy.
> - */
> - paddr = hvfb_get_phymem(hdev, screen_fb_size);
> - if (paddr != (phys_addr_t) -1) {
> - par->mmio_pp = paddr;
> - par->mmio_vp = par->dio_vp = __va(paddr);
> -
> - info->fix.smem_start = paddr;
> - info->fix.smem_len = screen_fb_size;
> - info->screen_base = par->mmio_vp;
> - info->screen_size = screen_fb_size;
> -
> - par->need_docopy = false;
> - goto getmem_done;
> - }
> - pr_info("Unable to allocate enough contiguous physical memory on Gen 1 VM. Using MMIO instead.\n");
> } else {
> info->apertures->ranges[0].base = screen_info.lfb_base;
> info->apertures->ranges[0].size = screen_info.lfb_size;
> }
>
> - /*
> - * Cannot use the contiguous physical memory.
> - * Allocate mmio space for framebuffer.
> - */
> - dio_fb_size =
> - screen_width * screen_height * screen_depth / 8;
> + dio_fb_size = screen_width * screen_height * screen_depth / 8;
>
> if (gen2vm) {
> pot_start = 0;
> @@ -1101,22 +970,11 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info)
> if (!fb_virt)
> goto err2;
>
> - /* Allocate memory for deferred IO */
> - par->dio_vp = vzalloc(round_up(dio_fb_size, PAGE_SIZE));
> - if (par->dio_vp == NULL)
> - goto err3;
> -
> - /* Physical address of FB device */
> - par->mmio_pp = par->mem->start;
> - /* Virtual address of FB device */
> - par->mmio_vp = (unsigned char *) fb_virt;
> -
> info->fix.smem_start = par->mem->start;
> info->fix.smem_len = dio_fb_size;
> - info->screen_base = par->dio_vp;
> + info->screen_base = fb_virt;
> info->screen_size = dio_fb_size;
>
> -getmem_done:
> remove_conflicting_framebuffers(info->apertures,
> KBUILD_MODNAME, false);
> if (!gen2vm)
> @@ -1125,8 +983,6 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info)
>
> return 0;
>
> -err3:
> - iounmap(fb_virt);
> err2:
> vmbus_free_mmio(par->mem->start, screen_fb_size);
> par->mem = NULL;
> @@ -1139,19 +995,12 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info)
> }
>
> /* Release the framebuffer */
> -static void hvfb_putmem(struct hv_device *hdev, struct fb_info *info)
> +static void hvfb_putmem(struct fb_info *info)
> {
> struct hvfb_par *par = info->par;
>
> - if (par->need_docopy) {
> - vfree(par->dio_vp);
> - iounmap(info->screen_base);
> - vmbus_free_mmio(par->mem->start, screen_fb_size);
> - } else {
> - hvfb_release_phymem(hdev, info->fix.smem_start,
> - screen_fb_size);
> - }
> -
> + iounmap(info->screen_base);
> + vmbus_free_mmio(par->mem->start, screen_fb_size);
> par->mem = NULL;
> }
>
> @@ -1170,7 +1019,6 @@ static int hvfb_probe(struct hv_device *hdev,
> par = info->par;
> par->info = info;
> par->fb_ready = false;
> - par->need_docopy = true;
> init_completion(&par->wait);
> INIT_DELAYED_WORK(&par->dwork, hvfb_update_work);
>
> @@ -1256,7 +1104,7 @@ static int hvfb_probe(struct hv_device *hdev,
>
> error:
> fb_deferred_io_cleanup(info);
> - hvfb_putmem(hdev, info);
> + hvfb_putmem(info);
> error2:
> vmbus_close(hdev->channel);
> error1:
> @@ -1286,7 +1134,7 @@ static int hvfb_remove(struct hv_device *hdev)
> vmbus_close(hdev->channel);
> hv_set_drvdata(hdev, NULL);
>
> - hvfb_putmem(hdev, info);
> + hvfb_putmem(info);
> framebuffer_release(info);
>
> return 0;
> --
> 2.19.1
>

2020-11-24 10:33:40

by Wei Liu

[permalink] [raw]
Subject: Re: [PATCH] video: hyperv_fb: Directly use the MMIO VRAM

On Tue, Nov 24, 2020 at 08:33:32AM +0000, Dexuan Cui wrote:
> Hi Wei Liu,
> Please do not pick up this patch, because actually MMIO VRAM can not work
> with fb_deferred_io.
>

No problem. Thanks for the heads-up.

Wei.

2020-11-25 01:58:21

by Dexuan Cui

[permalink] [raw]
Subject: RE: [PATCH] video: hyperv_fb: Directly use the MMIO VRAM

Hi Wei Liu,
Please do not pick up this patch, because actually MMIO VRAM can not work
with fb_deferred_io.

Previously I didn't test Xorg -- sorry. As soon as I tested it, I got the below
warning and the Xorg program ternimated immediately:

[ 28.148432] WARNING: CPU: 19 PID: 1410 at mm/vmalloc.c:383 vmalloc_to_page+0x14b/0x150
...
[ 28.192959] CPU: 19 PID: 1410 Comm: Xorg Tainted: G E 5.10.0-rc1+ #4
...
[ 28.208720] RIP: 0010:vmalloc_to_page+0x14b/0x150
...
[ 28.299231] Call Trace:
[ 28.301428] fb_deferred_io_fault+0x3a/0xa0
[ 28.305276] __do_fault+0x36/0x120
[ 28.308276] handle_mm_fault+0x1144/0x1950
[ 28.311963] exc_page_fault+0x290/0x510
[ 28.315551] ? asm_exc_page_fault+0x8/0x30
[ 28.319186] asm_exc_page_fault+0x1e/0x30
[ 28.322969] RIP: 0033:0x7fbeda3ec2f5

The issue is that fb_deferred_io_page() requires that the PFN be backed by a
struct page, but it looks the MMIO address does not have the struct page backed.

So I have to drop this patch.
Thanks Wei Hu and Michael for pointing this out!

FYI: drivers/video/fbdev/core/fb_defio.c:
static struct page *fb_deferred_io_page(struct fb_info *info, unsigned long offs)
{
void *screen_base = (void __force *) info->screen_base;
struct page *page;

if (is_vmalloc_addr(screen_base + offs))
page = vmalloc_to_page(screen_base + offs);
else
page = pfn_to_page((info->fix.smem_start + offs) >> PAGE_SHIFT);

return page;
}

/* this is to find and return the vmalloc-ed fb pages */
static vm_fault_t fb_deferred_io_fault(struct vm_fault *vmf)
{
unsigned long offset;
struct page *page;
struct fb_info *info = vmf->vma->vm_private_data;

offset = vmf->pgoff << PAGE_SHIFT;
if (offset >= info->fix.smem_len)
return VM_FAULT_SIGBUS;

page = fb_deferred_io_page(info, offset);
if (!page)
return VM_FAULT_SIGBUS;

Thanks,
-- Dexuan