linux-hyperv.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] video: hyperv: hyperv_fb: Use physical memory for fb on HyperV Gen 1 VMs.
@ 2019-11-22  8:24 Wei Hu
  2019-11-25  7:04 ` kbuild test robot
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Wei Hu @ 2019-11-22  8:24 UTC (permalink / raw)
  To: b.zolnierkie, kys, haiyangz, sthemmin, sashal, hch, m.szyprowski,
	mchehab+samsung, sam, gregkh, alexandre.belloni, info, arnd,
	dri-devel, linux-fbdev, linux-kernel, linux-hyperv, decui,
	mikelley
  Cc: Wei Hu

On Hyper-V, Generation 1 VMs can directly use VM's physical memory for
their framebuffers. This can improve the efficiency of framebuffer and
overall performence for VM. The physical memory assigned to framebuffer
must be contiguous. We use CMA allocator to get contiguouse physicial
memory when the framebuffer size is greater than 4MB. For size under
4MB, we use alloc_pages to achieve this.

To enable framebuffer memory allocation from CMA, supply a kernel
parameter to give enough space to CMA allocator at boot time. For
example:
    cma=130m
This gives 130MB memory to CAM allocator that can be allocated to
framebuffer. If this fails, we fall back to the old way of using
mmio for framebuffer.

Signed-off-by: Wei Hu <weh@microsoft.com>
---
    v2: Incorporated review comments form hch@lst.de, Michael Kelley and
    Dexuan Cui
    - Use dma_alloc_coherent to allocate large contiguous memory
    - Use phys_addr_t for physical addresses
    - Corrected a few spelling errors and minor cleanups
    - Also tested on 32 bit Ubuntu guest 

 drivers/video/fbdev/Kconfig     |   1 +
 drivers/video/fbdev/hyperv_fb.c | 196 +++++++++++++++++++++++++-------
 2 files changed, 158 insertions(+), 39 deletions(-)

diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
index aa9541bf964b..87b82de4598d 100644
--- a/drivers/video/fbdev/Kconfig
+++ b/drivers/video/fbdev/Kconfig
@@ -2215,6 +2215,7 @@ config FB_HYPERV
 	select FB_CFB_COPYAREA
 	select FB_CFB_IMAGEBLIT
 	select FB_DEFERRED_IO
+	select DMA_CMA if HAVE_DMA_CONTIGUOUS
 	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 3f60b7bc8589..8ba96764c749 100644
--- a/drivers/video/fbdev/hyperv_fb.c
+++ b/drivers/video/fbdev/hyperv_fb.c
@@ -31,6 +31,16 @@
  * "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
@@ -227,7 +237,6 @@ struct synthvid_msg {
 } __packed;
 
 
-
 /* FB driver definitions and structures */
 #define HVFB_WIDTH 1152 /* default screen width */
 #define HVFB_HEIGHT 864 /* default screen height */
@@ -256,12 +265,15 @@ 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;
-	unsigned long mmio_pp;
+	phys_addr_t mmio_pp;
 
 	/* Dirty rectangle, protected by delayed_refresh_lock */
 	int x1, y1, x2, y2;
@@ -432,7 +444,7 @@ static void synthvid_deferred_io(struct fb_info *p,
 		maxy = max_t(int, maxy, y2);
 
 		/* Copy from dio space to mmio address */
-		if (par->fb_ready)
+		if (par->fb_ready && par->need_docopy)
 			hvfb_docopy(par, start, PAGE_SIZE);
 	}
 
@@ -749,12 +761,12 @@ static void hvfb_update_work(struct work_struct *w)
 		return;
 
 	/* Copy the dirty rectangle to frame buffer memory */
-	for (j = y1; j < y2; j++) {
-		hvfb_docopy(par,
-			    j * info->fix.line_length +
-			    (x1 * screen_depth / 8),
-			    (x2 - x1) * screen_depth / 8);
-	}
+	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)
@@ -799,7 +811,8 @@ 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;
-	hvfb_docopy(par, 0, dio_fb_size);
+	if (par->need_docopy)
+		hvfb_docopy(par, 0, dio_fb_size);
 	synthvid_update(info, 0, 0, INT_MAX, INT_MAX);
 
 	return NOTIFY_DONE;
@@ -938,6 +951,74 @@ 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;
+	unsigned int request_pages;
+	phys_addr_t paddr = 0;
+	unsigned int order = get_order(request_size);
+
+	if (request_size == 0)
+		return -1;
+
+	/* Try to call alloc_pages if the size is less than 2^MAX_ORDER */
+	if (order < MAX_ORDER) {
+		page = alloc_pages(GFP_KERNEL | __GFP_ZERO, order);
+		if (!page)
+			return -1;
+
+		paddr = (page_to_pfn(page) << PAGE_SHIFT);
+		request_pages = (1 << order);
+		goto get_phymem1;
+	}
+
+	/* Allocate from CMA */
+	if (hdev == NULL)
+		return -1;
+
+	hdev->device.coherent_dma_mask = DMA_BIT_MASK(64);
+
+	request_pages = (round_up(request_size, PAGE_SIZE) >> PAGE_SHIFT);
+
+	vmem = dma_alloc_coherent(&hdev->device,
+				 request_pages * PAGE_SIZE,
+				 &dma_handle,
+				 GFP_KERNEL | __GFP_NOWARN);
+
+	if (!vmem)
+		return -1;
+
+	paddr = virt_to_phys(vmem);
+
+get_phymem1:
+	pr_info("Allocated %d pages starts at physical addr 0x%llx\n",
+		request_pages, paddr);
+
+	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)
@@ -947,8 +1028,57 @@ 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;
 
+	if (!gen2vm) {
+		pdev = pci_get_device(PCI_VENDOR_ID_MICROSOFT,
+			PCI_DEVICE_ID_HYPERV_VIDEO, NULL);
+		if (!pdev) {
+			pr_err("Unable to find PCI Hyper-V video\n");
+			return -ENODEV;
+		}
+	}
+
+	info->apertures = alloc_apertures(1);
+	if (!info->apertures)
+		goto err1;
+
+	if (gen2vm) {
+		info->apertures->ranges[0].base = screen_info.lfb_base;
+		info->apertures->ranges[0].size = screen_info.lfb_size;
+	} else {
+		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.
+	 */
+	if (!gen2vm) {
+		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 getmem1;
+		}
+		pr_info("Unable to allocate enough contiguous physical memory on Gen 1 VM. Use MMIO instead.\n");
+	}
+
+	/*
+	 * Cannot use the contiguous physical memory.
+	 * Allocate mmio space for framebuffer.
+	 */
 	dio_fb_size =
 		screen_width * screen_height * screen_depth / 8;
 
@@ -956,13 +1086,6 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info)
 		pot_start = 0;
 		pot_end = -1;
 	} else {
-		pdev = pci_get_device(PCI_VENDOR_ID_MICROSOFT,
-			      PCI_DEVICE_ID_HYPERV_VIDEO, NULL);
-		if (!pdev) {
-			pr_err("Unable to find PCI Hyper-V video\n");
-			return -ENODEV;
-		}
-
 		if (!(pci_resource_flags(pdev, 0) & IORESOURCE_MEM) ||
 		    pci_resource_len(pdev, 0) < screen_fb_size) {
 			pr_err("Resource not available or (0x%lx < 0x%lx)\n",
@@ -991,20 +1114,6 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info)
 	if (par->dio_vp == NULL)
 		goto err3;
 
-	info->apertures = alloc_apertures(1);
-	if (!info->apertures)
-		goto err4;
-
-	if (gen2vm) {
-		info->apertures->ranges[0].base = screen_info.lfb_base;
-		info->apertures->ranges[0].size = screen_info.lfb_size;
-		remove_conflicting_framebuffers(info->apertures,
-						KBUILD_MODNAME, false);
-	} else {
-		info->apertures->ranges[0].base = pci_resource_start(pdev, 0);
-		info->apertures->ranges[0].size = pci_resource_len(pdev, 0);
-	}
-
 	/* Physical address of FB device */
 	par->mmio_pp = par->mem->start;
 	/* Virtual address of FB device */
@@ -1015,13 +1124,15 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info)
 	info->screen_base = par->dio_vp;
 	info->screen_size = dio_fb_size;
 
+getmem1:
+	remove_conflicting_framebuffers(info->apertures,
+					KBUILD_MODNAME, false);
+
 	if (!gen2vm)
 		pci_dev_put(pdev);
 
 	return 0;
 
-err4:
-	vfree(par->dio_vp);
 err3:
 	iounmap(fb_virt);
 err2:
@@ -1035,13 +1146,19 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info)
 }
 
 /* Release the framebuffer */
-static void hvfb_putmem(struct fb_info *info)
+static void hvfb_putmem(struct hv_device *hdev, struct fb_info *info)
 {
 	struct hvfb_par *par = info->par;
 
-	vfree(par->dio_vp);
-	iounmap(info->screen_base);
-	vmbus_free_mmio(par->mem->start, screen_fb_size);
+	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);
+	}
+
 	par->mem = NULL;
 }
 
@@ -1060,6 +1177,7 @@ 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);
 
@@ -1145,7 +1263,7 @@ static int hvfb_probe(struct hv_device *hdev,
 
 error:
 	fb_deferred_io_cleanup(info);
-	hvfb_putmem(info);
+	hvfb_putmem(hdev, info);
 error2:
 	vmbus_close(hdev->channel);
 error1:
@@ -1175,7 +1293,7 @@ static int hvfb_remove(struct hv_device *hdev)
 	vmbus_close(hdev->channel);
 	hv_set_drvdata(hdev, NULL);
 
-	hvfb_putmem(info);
+	hvfb_putmem(hdev, info);
 	framebuffer_release(info);
 
 	return 0;
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] video: hyperv: hyperv_fb: Use physical memory for fb on HyperV Gen 1 VMs.
  2019-11-22  8:24 [PATCH v2] video: hyperv: hyperv_fb: Use physical memory for fb on HyperV Gen 1 VMs Wei Hu
@ 2019-11-25  7:04 ` kbuild test robot
  2019-11-25 13:07 ` kbuild test robot
  2019-11-27 18:14 ` Michael Kelley
  2 siblings, 0 replies; 4+ messages in thread
From: kbuild test robot @ 2019-11-25  7:04 UTC (permalink / raw)
  To: Wei Hu
  Cc: kbuild-all, b.zolnierkie, kys, haiyangz, sthemmin, sashal, hch,
	m.szyprowski, mchehab+samsung, sam, gregkh, alexandre.belloni,
	info, arnd, dri-devel, linux-fbdev, linux-kernel, linux-hyperv,
	decui, mikelley, Wei Hu

[-- Attachment #1: Type: text/plain, Size: 3741 bytes --]

Hi Wei,

I love your patch! Perhaps something to improve:

[auto build test WARNING on next-20191122]
[cannot apply to linus/master v5.4-rc8 v5.4-rc7 v5.4-rc6 v5.4]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Wei-Hu/video-hyperv-hyperv_fb-Use-physical-memory-for-fb-on-HyperV-Gen-1-VMs/20191124-163533
base:    b9d3d01405061bb42358fe53f824e894a1922ced
config: i386-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.4.0-14) 7.4.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from include/linux/printk.h:7:0,
                    from include/linux/kernel.h:15,
                    from include/linux/list.h:9,
                    from include/linux/module.h:12,
                    from drivers/video/fbdev/hyperv_fb.c:48:
   drivers/video/fbdev/hyperv_fb.c: In function 'hvfb_get_phymem':
   include/linux/kern_levels.h:5:18: warning: format '%llx' expects argument of type 'long long unsigned int', but argument 3 has type 'phys_addr_t {aka unsigned int}' [-Wformat=]
    #define KERN_SOH "\001"  /* ASCII Start Of Header */
                     ^
   include/linux/kern_levels.h:14:19: note: in expansion of macro 'KERN_SOH'
    #define KERN_INFO KERN_SOH "6" /* informational */
                      ^~~~~~~~
   include/linux/printk.h:311:9: note: in expansion of macro 'KERN_INFO'
     printk(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
            ^~~~~~~~~
>> drivers/video/fbdev/hyperv_fb.c:1003:2: note: in expansion of macro 'pr_info'
     pr_info("Allocated %d pages starts at physical addr 0x%llx\n",
     ^~~~~~~

vim +/pr_info +1003 drivers/video/fbdev/hyperv_fb.c

   955	
   956	/*
   957	 * Allocate enough contiguous physical memory.
   958	 * Return physical address if succeeded or -1 if failed.
   959	 */
   960	static phys_addr_t hvfb_get_phymem(struct hv_device *hdev,
   961					   unsigned int request_size)
   962	{
   963		struct page *page = NULL;
   964		dma_addr_t dma_handle;
   965		void *vmem;
   966		unsigned int request_pages;
   967		phys_addr_t paddr = 0;
   968		unsigned int order = get_order(request_size);
   969	
   970		if (request_size == 0)
   971			return -1;
   972	
   973		/* Try to call alloc_pages if the size is less than 2^MAX_ORDER */
   974		if (order < MAX_ORDER) {
   975			page = alloc_pages(GFP_KERNEL | __GFP_ZERO, order);
   976			if (!page)
   977				return -1;
   978	
   979			paddr = (page_to_pfn(page) << PAGE_SHIFT);
   980			request_pages = (1 << order);
   981			goto get_phymem1;
   982		}
   983	
   984		/* Allocate from CMA */
   985		if (hdev == NULL)
   986			return -1;
   987	
   988		hdev->device.coherent_dma_mask = DMA_BIT_MASK(64);
   989	
   990		request_pages = (round_up(request_size, PAGE_SIZE) >> PAGE_SHIFT);
   991	
   992		vmem = dma_alloc_coherent(&hdev->device,
   993					 request_pages * PAGE_SIZE,
   994					 &dma_handle,
   995					 GFP_KERNEL | __GFP_NOWARN);
   996	
   997		if (!vmem)
   998			return -1;
   999	
  1000		paddr = virt_to_phys(vmem);
  1001	
  1002	get_phymem1:
> 1003		pr_info("Allocated %d pages starts at physical addr 0x%llx\n",
  1004			request_pages, paddr);
  1005	
  1006		return paddr;
  1007	}
  1008	

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 71011 bytes --]

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] video: hyperv: hyperv_fb: Use physical memory for fb on HyperV Gen 1 VMs.
  2019-11-22  8:24 [PATCH v2] video: hyperv: hyperv_fb: Use physical memory for fb on HyperV Gen 1 VMs Wei Hu
  2019-11-25  7:04 ` kbuild test robot
@ 2019-11-25 13:07 ` kbuild test robot
  2019-11-27 18:14 ` Michael Kelley
  2 siblings, 0 replies; 4+ messages in thread
From: kbuild test robot @ 2019-11-25 13:07 UTC (permalink / raw)
  To: Wei Hu
  Cc: kbuild-all, b.zolnierkie, kys, haiyangz, sthemmin, sashal, hch,
	m.szyprowski, mchehab+samsung, sam, gregkh, alexandre.belloni,
	info, arnd, dri-devel, linux-fbdev, linux-kernel, linux-hyperv,
	decui, mikelley, Wei Hu

[-- Attachment #1: Type: text/plain, Size: 10810 bytes --]

Hi Wei,

I love your patch! Yet something to improve:

[auto build test ERROR on next-20191122]
[cannot apply to linus/master v5.4-rc8 v5.4-rc7 v5.4-rc6 v5.4]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Wei-Hu/video-hyperv-hyperv_fb-Use-physical-memory-for-fb-on-HyperV-Gen-1-VMs/20191124-163533
base:    b9d3d01405061bb42358fe53f824e894a1922ced
config: i386-randconfig-b003-20191125 (attached as .config)
compiler: gcc-7 (Debian 7.4.0-14) 7.4.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   ld: kernel/dma/contiguous.o: in function `dma_alloc_from_contiguous':
>> kernel/dma/contiguous.c:199: undefined reference to `cma_alloc'
>> ld: kernel/dma/contiguous.c:199: undefined reference to `cma_alloc'
   ld: kernel/dma/contiguous.o: in function `dma_release_from_contiguous':
>> kernel/dma/contiguous.c:215: undefined reference to `cma_release'
>> ld: kernel/dma/contiguous.c:215: undefined reference to `cma_release'
   ld: kernel/dma/contiguous.o: in function `dma_alloc_contiguous':
   kernel/dma/contiguous.c:248: undefined reference to `cma_alloc'
   ld: kernel/dma/contiguous.o: in function `dma_free_contiguous':
   kernel/dma/contiguous.c:267: undefined reference to `cma_release'
   ld: kernel/dma/contiguous.c:267: undefined reference to `cma_release'
   ld: kernel/dma/contiguous.o: in function `dma_contiguous_reserve_area':
>> kernel/dma/contiguous.c:169: undefined reference to `cma_declare_contiguous'
>> ld: kernel/dma/contiguous.c:175: undefined reference to `cma_get_size'
>> ld: kernel/dma/contiguous.c:175: undefined reference to `cma_get_base'

vim +199 kernel/dma/contiguous.c

c64be2bb1c6eb4 drivers/base/dma-contiguous.c Marek Szyprowski 2011-12-29  145  
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  146  /**
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  147   * dma_contiguous_reserve_area() - reserve custom contiguous area
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  148   * @size: Size of the reserved area (in bytes),
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  149   * @base: Base address of the reserved area optional, use 0 for any
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  150   * @limit: End address of the reserved memory (optional, 0 for any).
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  151   * @res_cma: Pointer to store the created cma region.
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  152   * @fixed: hint about where to place the reserved area
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  153   *
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  154   * This function reserves memory from early allocator. It should be
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  155   * called by arch specific code once the early allocator (memblock or bootmem)
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  156   * has been activated and all other subsystems have already allocated/reserved
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  157   * memory. This function allows to create custom reserved areas for specific
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  158   * devices.
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  159   *
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  160   * If @fixed is true, reserve contiguous area at exactly @base.  If false,
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  161   * reserve in range from @base to @limit.
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  162   */
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  163  int __init dma_contiguous_reserve_area(phys_addr_t size, phys_addr_t base,
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  164  				       phys_addr_t limit, struct cma **res_cma,
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  165  				       bool fixed)
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  166  {
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  167  	int ret;
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  168  
f318dd083c8128 drivers/base/dma-contiguous.c Laura Abbott     2017-04-18 @169  	ret = cma_declare_contiguous(base, size, limit, 0, 0, fixed,
f318dd083c8128 drivers/base/dma-contiguous.c Laura Abbott     2017-04-18  170  					"reserved", res_cma);
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  171  	if (ret)
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  172  		return ret;
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  173  
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  174  	/* Architecture specific contiguous memory fixup. */
a254129e8686bf drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06 @175  	dma_contiguous_early_fixup(cma_get_base(*res_cma),
a254129e8686bf drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  176  				cma_get_size(*res_cma));
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  177  
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  178  	return 0;
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  179  }
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  180  
c64be2bb1c6eb4 drivers/base/dma-contiguous.c Marek Szyprowski 2011-12-29  181  /**
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  182   * dma_alloc_from_contiguous() - allocate pages from contiguous area
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  183   * @dev:   Pointer to device for which the allocation is performed.
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  184   * @count: Requested number of pages.
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  185   * @align: Requested alignment of pages (in PAGE_SIZE order).
d834c5ab83febf kernel/dma/contiguous.c       Marek Szyprowski 2018-08-17  186   * @no_warn: Avoid printing message about failed allocation.
c64be2bb1c6eb4 drivers/base/dma-contiguous.c Marek Szyprowski 2011-12-29  187   *
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  188   * This function allocates memory buffer for specified device. It uses
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  189   * device specific contiguous memory area if available or the default
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  190   * global one. Requires architecture specific dev_get_cma_area() helper
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  191   * function.
c64be2bb1c6eb4 drivers/base/dma-contiguous.c Marek Szyprowski 2011-12-29  192   */
67a2e213e7e937 drivers/base/dma-contiguous.c Rohit Vaswani    2015-10-22  193  struct page *dma_alloc_from_contiguous(struct device *dev, size_t count,
d834c5ab83febf kernel/dma/contiguous.c       Marek Szyprowski 2018-08-17  194  				       unsigned int align, bool no_warn)
c64be2bb1c6eb4 drivers/base/dma-contiguous.c Marek Szyprowski 2011-12-29  195  {
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  196  	if (align > CONFIG_CMA_ALIGNMENT)
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  197  		align = CONFIG_CMA_ALIGNMENT;
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  198  
d834c5ab83febf kernel/dma/contiguous.c       Marek Szyprowski 2018-08-17 @199  	return cma_alloc(dev_get_cma_area(dev), count, align, no_warn);
c64be2bb1c6eb4 drivers/base/dma-contiguous.c Marek Szyprowski 2011-12-29  200  }
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  201  
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  202  /**
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  203   * dma_release_from_contiguous() - release allocated pages
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  204   * @dev:   Pointer to device for which the pages were allocated.
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  205   * @pages: Allocated pages.
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  206   * @count: Number of allocated pages.
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  207   *
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  208   * This function releases memory allocated by dma_alloc_from_contiguous().
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  209   * It returns false when provided pages do not belong to contiguous area and
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  210   * true otherwise.
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  211   */
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  212  bool dma_release_from_contiguous(struct device *dev, struct page *pages,
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  213  				 int count)
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  214  {
a254129e8686bf drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06 @215  	return cma_release(dev_get_cma_area(dev), pages, count);
3162bbd7e65b9c drivers/base/dma-contiguous.c Joonsoo Kim      2014-08-06  216  }
de9e14eebf33a6 drivers/base/dma-contiguous.c Marek Szyprowski 2014-10-13  217  

:::::: The code at line 199 was first introduced by commit
:::::: d834c5ab83febf9624ad3b16c3c348aa1e02014c kernel/dma: remove unsupported gfp_mask parameter from dma_alloc_from_contiguous()

:::::: TO: Marek Szyprowski <m.szyprowski@samsung.com>
:::::: CC: Linus Torvalds <torvalds@linux-foundation.org>

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 35695 bytes --]

^ permalink raw reply	[flat|nested] 4+ messages in thread

* RE: [PATCH v2] video: hyperv: hyperv_fb: Use physical memory for fb on HyperV Gen 1 VMs.
  2019-11-22  8:24 [PATCH v2] video: hyperv: hyperv_fb: Use physical memory for fb on HyperV Gen 1 VMs Wei Hu
  2019-11-25  7:04 ` kbuild test robot
  2019-11-25 13:07 ` kbuild test robot
@ 2019-11-27 18:14 ` Michael Kelley
  2 siblings, 0 replies; 4+ messages in thread
From: Michael Kelley @ 2019-11-27 18:14 UTC (permalink / raw)
  To: Wei Hu, b.zolnierkie, KY Srinivasan, Haiyang Zhang,
	Stephen Hemminger, sashal, hch, m.szyprowski, mchehab+samsung,
	sam, gregkh, alexandre.belloni, info, arnd, dri-devel,
	linux-fbdev, linux-kernel, linux-hyperv, Dexuan Cui

From: Wei Hu <weh@microsoft.com> Sent: Friday, November 22, 2019 12:24 AM
> 
> On Hyper-V, Generation 1 VMs can directly use VM's physical memory for
> their framebuffers. This can improve the efficiency of framebuffer and
> overall performence for VM. The physical memory assigned to framebuffer
> must be contiguous. We use CMA allocator to get contiguouse physicial
> memory when the framebuffer size is greater than 4MB. For size under
> 4MB, we use alloc_pages to achieve this.
> 
> To enable framebuffer memory allocation from CMA, supply a kernel
> parameter to give enough space to CMA allocator at boot time. For
> example:
>     cma=130m
> This gives 130MB memory to CAM allocator that can be allocated to
> framebuffer. If this fails, we fall back to the old way of using
> mmio for framebuffer.
> 
> Signed-off-by: Wei Hu <weh@microsoft.com>
> ---
>     v2: Incorporated review comments form hch@lst.de, Michael Kelley and
>     Dexuan Cui
>     - Use dma_alloc_coherent to allocate large contiguous memory
>     - Use phys_addr_t for physical addresses
>     - Corrected a few spelling errors and minor cleanups
>     - Also tested on 32 bit Ubuntu guest
> 
>  drivers/video/fbdev/Kconfig     |   1 +
>  drivers/video/fbdev/hyperv_fb.c | 196 +++++++++++++++++++++++++-------
>  2 files changed, 158 insertions(+), 39 deletions(-)
> 

[snip]

> +/*
> + * 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;
> +	unsigned int request_pages;
> +	phys_addr_t paddr = 0;
> +	unsigned int order = get_order(request_size);
> +
> +	if (request_size == 0)
> +		return -1;
> +
> +	/* Try to call alloc_pages if the size is less than 2^MAX_ORDER */
> +	if (order < MAX_ORDER) {
> +		page = alloc_pages(GFP_KERNEL | __GFP_ZERO, order);
> +		if (!page)
> +			return -1;
> +
> +		paddr = (page_to_pfn(page) << PAGE_SHIFT);
> +		request_pages = (1 << order);
> +		goto get_phymem1;
> +	}

Could you use an 'else' clause here and eliminate the above
'goto' statement?  I know that makes the below code be indented
one level deeper, but that doesn't seem particularly problematic
here.  The reason we have 'else' clauses is to avoid 'goto's and
labels.  :-)

> +
> +	/* Allocate from CMA */
> +	if (hdev == NULL)
> +		return -1;

The above test seems unnecessary.  A lot of things would have
broken before getting to this function if hdev was NULL.

> +
> +	hdev->device.coherent_dma_mask = DMA_BIT_MASK(64);
> +
> +	request_pages = (round_up(request_size, PAGE_SIZE) >> PAGE_SHIFT);
> +
> +	vmem = dma_alloc_coherent(&hdev->device,
> +				 request_pages * PAGE_SIZE,
> +				 &dma_handle,
> +				 GFP_KERNEL | __GFP_NOWARN);
> +
> +	if (!vmem)
> +		return -1;
> +
> +	paddr = virt_to_phys(vmem);
> +
> +get_phymem1:
> +	pr_info("Allocated %d pages starts at physical addr 0x%llx\n",
> +		request_pages, paddr);

I wonder if we want to show the physical address here.  The Linux kernel
definitely does not show kernel virtual addresses due to security
concerns, and I'm wondering if the same applies to physical addresses.
What's the benefit to showing the physical address?

And in the message "starts" should be "starting".

> +
> +	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)
> @@ -947,8 +1028,57 @@ 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;
> 
> +	if (!gen2vm) {
> +		pdev = pci_get_device(PCI_VENDOR_ID_MICROSOFT,
> +			PCI_DEVICE_ID_HYPERV_VIDEO, NULL);
> +		if (!pdev) {
> +			pr_err("Unable to find PCI Hyper-V video\n");
> +			return -ENODEV;
> +		}
> +	}
> +
> +	info->apertures = alloc_apertures(1);
> +	if (!info->apertures)
> +		goto err1;

There's a small memory leak here.  The apertures are never freed in any
of the error cases in this function, or in hvfb_putmem().  This is not a bug you
introduced -- the original code had the same leak.

> +
> +	if (gen2vm) {
> +		info->apertures->ranges[0].base = screen_info.lfb_base;
> +		info->apertures->ranges[0].size = screen_info.lfb_size;
> +	} else {
> +		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.
> +	 */
> +	if (!gen2vm) {
> +		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 getmem1;

Maybe change the 'getmem1' label to 'done' or something similarly
indicative having successfully completed everything that needs to be
done?

> +		}
> +		pr_info("Unable to allocate enough contiguous physical memory on Gen 1
> VM. Use MMIO instead.\n");

I'd suggest changing the message to say "Using MMIO instead".  This is just an
informative message indicating what the driver is doing.  "Use MMIO instead"
sounds like a directive to the user to do something different, like change his
kernel configuration, and that's not the intent.

> +	}

In the above code, there are three, almost consecutive, tests of the "gen2vm"
variable.  It seems like the apertures could be allocated first, and then the three
tests combined into one test.  Then you have one range of code for Gen 1 and
another range for Gen 2 and fewer lines 'if' statements, 'else' statements, and
curly braces.

> +
> +	/*
> +	 * Cannot use the contiguous physical memory.
> +	 * Allocate mmio space for framebuffer.
> +	 */
>  	dio_fb_size =
>  		screen_width * screen_height * screen_depth / 8;
> 
> @@ -956,13 +1086,6 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info
> *info)
>  		pot_start = 0;
>  		pot_end = -1;
>  	} else {
> -		pdev = pci_get_device(PCI_VENDOR_ID_MICROSOFT,
> -			      PCI_DEVICE_ID_HYPERV_VIDEO, NULL);
> -		if (!pdev) {
> -			pr_err("Unable to find PCI Hyper-V video\n");
> -			return -ENODEV;
> -		}
> -
>  		if (!(pci_resource_flags(pdev, 0) & IORESOURCE_MEM) ||
>  		    pci_resource_len(pdev, 0) < screen_fb_size) {
>  			pr_err("Resource not available or (0x%lx < 0x%lx)\n",
> @@ -991,20 +1114,6 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info
> *info)
>  	if (par->dio_vp == NULL)
>  		goto err3;
> 
> -	info->apertures = alloc_apertures(1);
> -	if (!info->apertures)
> -		goto err4;
> -
> -	if (gen2vm) {
> -		info->apertures->ranges[0].base = screen_info.lfb_base;
> -		info->apertures->ranges[0].size = screen_info.lfb_size;
> -		remove_conflicting_framebuffers(info->apertures,
> -						KBUILD_MODNAME, false);
> -	} else {
> -		info->apertures->ranges[0].base = pci_resource_start(pdev, 0);
> -		info->apertures->ranges[0].size = pci_resource_len(pdev, 0);
> -	}
> -
>  	/* Physical address of FB device */
>  	par->mmio_pp = par->mem->start;
>  	/* Virtual address of FB device */
> @@ -1015,13 +1124,15 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info
> *info)
>  	info->screen_base = par->dio_vp;
>  	info->screen_size = dio_fb_size;
> 
> +getmem1:
> +	remove_conflicting_framebuffers(info->apertures,
> +					KBUILD_MODNAME, false);

With your change, remove_conflicting_framebuffers() is called for both
Gen 1 and Gen 2 VMs.  In the old code, it was called only for Gen 2 VMs.
Is this change intentional?  If so, why?  I haven't delved into the details
of what remove_conflicting_framebuffers() does, so my question is 
more of a double-check rather than my definitely thinking something
is wrong.

> +
>  	if (!gen2vm)
>  		pci_dev_put(pdev);
> 
>  	return 0;
> 
> -err4:
> -	vfree(par->dio_vp);
>  err3:
>  	iounmap(fb_virt);
>  err2:
> @@ -1035,13 +1146,19 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info
> *info)
>  }
> 
>  /* Release the framebuffer */
> -static void hvfb_putmem(struct fb_info *info)
> +static void hvfb_putmem(struct hv_device *hdev, struct fb_info *info)
>  {
>  	struct hvfb_par *par = info->par;
> 
> -	vfree(par->dio_vp);
> -	iounmap(info->screen_base);
> -	vmbus_free_mmio(par->mem->start, screen_fb_size);
> +	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);
> +	}
> +
>  	par->mem = NULL;

There's a small memory leak in the above statement.  The data
structure pointed to by "mem" is not freed.   The same problem
occurs in hvfb_getmem() in the "err2" path.   This bug existed in
the old code as well, so it was not introduced by your changes.

Michael

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2019-11-27 18:18 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-22  8:24 [PATCH v2] video: hyperv: hyperv_fb: Use physical memory for fb on HyperV Gen 1 VMs Wei Hu
2019-11-25  7:04 ` kbuild test robot
2019-11-25 13:07 ` kbuild test robot
2019-11-27 18:14 ` Michael Kelley

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).