Re: [PATCH v2 14/19] xen/drmfront: use xenbus_setup_ring() and xenbus_teardown_ring()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 28.04.22 11:27, Juergen Gross wrote:

Hello Juergen, all

Simplify drmfront's ring creation and removal via xenbus_setup_ring()
and xenbus_teardown_ring().

Signed-off-by: Juergen Gross <jgross@xxxxxxxx>


I am not familiar with DRM bits of this driver, but a little bit familiar with Xen bits this patch only touches and I have environment to test.

Xen specific changes looks good to me. Also I didn't see any specific to this series issues when testing virtulized display driver except one I have already pointed out in PATCH v2 08/19.

root@salvator-x-h3-4x2g-xt-domu:~# dmesg | grep drm
[    0.158190] [drm] Registering XEN PV vdispl
[    0.159620] [drm] Connector device/vdispl/0/0: resolution 1920x1080
[    0.159888] [drm] Have 1 connector(s)
[    0.289069] [drm] Creating Xen PV DRM Display Unit
[    0.289873] [drm] Initialized xendrm-du 1.0.0 20180221 for vdispl-0 on minor 0
[    0.289918] [drm] Initialized xendrm-du 1.0.0 20180221 on minor 0


root@generic-armv8-xt-dom0:~# xenstore-ls -f | grep vdispl
/local/domain/1/backend/vdispl = ""
/local/domain/1/backend/vdispl/2 = ""
/local/domain/1/backend/vdispl/2/0 = ""
/local/domain/1/backend/vdispl/2/0/frontend = "/local/domain/2/device/vdispl/0"
/local/domain/1/backend/vdispl/2/0/frontend-id = "2"
/local/domain/1/backend/vdispl/2/0/online = "1"
/local/domain/1/backend/vdispl/2/0/state = "4"
/local/domain/2/device/vdispl = ""
/local/domain/2/device/vdispl/0 = ""
/local/domain/2/device/vdispl/0/backend = "/local/domain/1/backend/vdispl/2/0"
/local/domain/2/device/vdispl/0/backend-id = "1"
/local/domain/2/device/vdispl/0/state = "4"
/local/domain/2/device/vdispl/0/be-alloc = "0"
/local/domain/2/device/vdispl/0/0 = ""
/local/domain/2/device/vdispl/0/0/resolution = "1920x1080"
/local/domain/2/device/vdispl/0/0/unique-id = "HDMI-A-1"
/local/domain/2/device/vdispl/0/0/req-ring-ref = "8"
/local/domain/2/device/vdispl/0/0/req-event-channel = "7"
/local/domain/2/device/vdispl/0/0/evt-ring-ref = "9"
/local/domain/2/device/vdispl/0/0/evt-event-channel = "8"
/libxl/2/device/vdispl = ""
/libxl/2/device/vdispl/0 = ""
/libxl/2/device/vdispl/0/frontend = "/local/domain/2/device/vdispl/0"
/libxl/2/device/vdispl/0/backend = "/local/domain/1/backend/vdispl/2/0"
/libxl/2/device/vdispl/0/frontend-id = "2"
/libxl/2/device/vdispl/0/online = "1"
/libxl/2/device/vdispl/0/state = "1"


It worth mentioning I noticed one issue, but I believe it is not related to your series.

root@salvator-x-h3-4x2g-xt-domu:~# modetest -M xendrm-du -s 31:1920x1080
[   62.431887] ------------[ cut here ]------------
[   62.431940] WARNING: CPU: 0 PID: 244 at drivers/gpu/drm/drm_gem.c:1055 drm_gem_mmap_obj+0x16c/0x180
[   62.432000] Modules linked in:
[   62.432025] CPU: 0 PID: 244 Comm: modetest Tainted: G W         5.18.0-rc4-yocto-standard-00096-g936342d8fae2 #1
[   62.432067] Hardware name: XENVM-4.17 (DT)
[   62.432089] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[   62.432126] pc : drm_gem_mmap_obj+0x16c/0x180
[   62.432153] lr : drm_gem_mmap_obj+0x78/0x180
[   62.432178] sp : ffff800009da3bb0
[   62.432196] x29: ffff800009da3bb0 x28: 0000000000000008 x27: ffff0001c3a56780 [   62.432237] x26: ffff0001c3a56f00 x25: 00000000000007e9 x24: ffff0001c23dec00 [   62.432279] x23: ffff0001c0c98000 x22: ffff0001c2162b80 x21: 0000000000000000 [   62.432320] x20: ffff0001c3a56780 x19: ffff0001c23dea00 x18: 0000000000000001 [   62.432355] x17: 0000000000000000 x16: 0000000000000000 x15: 000000000003603c [   62.432394] x14: 0000000000000000 x13: 0000000000000000 x12: 0000000000000000 [   62.432430] x11: 0000000000100000 x10: 0000ffff88071000 x9 : ffff0001c0f17e70 [   62.432470] x8 : ffff8001f65ce000 x7 : 0000000000000001 x6 : ffff0001c388a000 [   62.432505] x5 : ffff800009da3a10 x4 : 0000000000000090 x3 : 0000000010046400 [   62.432539] x2 : 00000000000007e9 x1 : 9b0023a536f4f400 x0 : 00000000100000fb
[   62.432579] Call trace:
[   62.432593]  drm_gem_mmap_obj+0x16c/0x180
[   62.432617]  drm_gem_mmap+0x128/0x228
[   62.432641]  mmap_region+0x384/0x5a0
[   62.432667]  do_mmap+0x354/0x4f0
[   62.432687]  vm_mmap_pgoff+0xdc/0x108
[   62.432710]  ksys_mmap_pgoff+0x1b8/0x208
[   62.432734]  __arm64_sys_mmap+0x30/0x48
[   62.432760]  invoke_syscall+0x44/0x108
[   62.432783]  el0_svc_common.constprop.0+0xcc/0xf0
[   62.432811]  do_el0_svc+0x24/0x88
[   62.432831]  el0_svc+0x2c/0x88
[   62.432855]  el0t_64_sync_handler+0xb0/0xb8
[   62.432875]  el0t_64_sync+0x18c/0x190
[   62.432898] ---[ end trace 0000000000000000 ]---
setting mode 1920x1080-60.00Hz@XR24 on connectors 31, crtc 34


Although we see that WARNING, the application still works. Looking into the code, I assume that problem is that frontend doesn't set the VM_DONTEXPAND flag in mmap callback.

This diff fixes an issue in my environment:

index 5a5bf4e..e31554d 100644
--- a/drivers/gpu/drm/xen/xen_drm_front_gem.c
+++ b/drivers/gpu/drm/xen/xen_drm_front_gem.c
@@ -71,7 +71,7 @@ static int xen_drm_front_gem_object_mmap(struct drm_gem_object *gem_obj,
         * the whole buffer.
         */
        vma->vm_flags &= ~VM_PFNMAP;
-       vma->vm_flags |= VM_MIXEDMAP;
+       vma->vm_flags |= VM_MIXEDMAP | VM_DONTEXPAND;
        vma->vm_pgoff = 0;

        /*


I am not 100% sure whether it is a proper fix, so I would kindly ask DRM folks to confirm. I will be able to send a formal patch then.


---
  drivers/gpu/drm/xen/xen_drm_front_evtchnl.c | 43 ++++++---------------
  1 file changed, 11 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/xen/xen_drm_front_evtchnl.c b/drivers/gpu/drm/xen/xen_drm_front_evtchnl.c
index 4006568b9e32..e52afd792346 100644
--- a/drivers/gpu/drm/xen/xen_drm_front_evtchnl.c
+++ b/drivers/gpu/drm/xen/xen_drm_front_evtchnl.c
@@ -123,12 +123,12 @@ static irqreturn_t evtchnl_interrupt_evt(int irq, void *dev_id)
  static void evtchnl_free(struct xen_drm_front_info *front_info,
  			 struct xen_drm_front_evtchnl *evtchnl)
  {
-	unsigned long page = 0;
+	void *page = NULL;
if (evtchnl->type == EVTCHNL_TYPE_REQ)
-		page = (unsigned long)evtchnl->u.req.ring.sring;
+		page = evtchnl->u.req.ring.sring;
  	else if (evtchnl->type == EVTCHNL_TYPE_EVT)
-		page = (unsigned long)evtchnl->u.evt.page;
+		page = evtchnl->u.evt.page;
  	if (!page)
  		return;
@@ -147,8 +147,7 @@ static void evtchnl_free(struct xen_drm_front_info *front_info,
  		xenbus_free_evtchn(front_info->xb_dev, evtchnl->port);
/* end access and free the page */
-	if (evtchnl->gref != INVALID_GRANT_REF)
-		gnttab_end_foreign_access(evtchnl->gref, page);
+	xenbus_teardown_ring(&page, 1, &evtchnl->gref);
memset(evtchnl, 0, sizeof(*evtchnl));
  }
@@ -158,8 +157,7 @@ static int evtchnl_alloc(struct xen_drm_front_info *front_info, int index,
  			 enum xen_drm_front_evtchnl_type type)
  {
  	struct xenbus_device *xb_dev = front_info->xb_dev;
-	unsigned long page;
-	grant_ref_t gref;
+	void *page;
  	irq_handler_t handler;
  	int ret;
@@ -168,44 +166,25 @@ static int evtchnl_alloc(struct xen_drm_front_info *front_info, int index,
  	evtchnl->index = index;
  	evtchnl->front_info = front_info;
  	evtchnl->state = EVTCHNL_STATE_DISCONNECTED;
-	evtchnl->gref = INVALID_GRANT_REF;
- page = get_zeroed_page(GFP_NOIO | __GFP_HIGH);
-	if (!page) {
-		ret = -ENOMEM;
+	ret = xenbus_setup_ring(xb_dev, GFP_NOIO | __GFP_HIGH, &page,
+				1, &evtchnl->gref);
+	if (ret)
  		goto fail;
-	}
if (type == EVTCHNL_TYPE_REQ) {
  		struct xen_displif_sring *sring;
init_completion(&evtchnl->u.req.completion);
  		mutex_init(&evtchnl->u.req.req_io_lock);
-		sring = (struct xen_displif_sring *)page;
-		SHARED_RING_INIT(sring);
-		FRONT_RING_INIT(&evtchnl->u.req.ring, sring, XEN_PAGE_SIZE);
-
-		ret = xenbus_grant_ring(xb_dev, sring, 1, &gref);
-		if (ret < 0) {
-			evtchnl->u.req.ring.sring = NULL;
-			free_page(page);
-			goto fail;
-		}
+		sring = page;
+		XEN_FRONT_RING_INIT(&evtchnl->u.req.ring, sring, XEN_PAGE_SIZE);
handler = evtchnl_interrupt_ctrl;
  	} else {
-		ret = gnttab_grant_foreign_access(xb_dev->otherend_id,
-						  virt_to_gfn((void *)page), 0);
-		if (ret < 0) {
-			free_page(page);
-			goto fail;
-		}
-
-		evtchnl->u.evt.page = (struct xendispl_event_page *)page;
-		gref = ret;
+		evtchnl->u.evt.page = page;
  		handler = evtchnl_interrupt_evt;
  	}
-	evtchnl->gref = gref;
ret = xenbus_alloc_evtchn(xb_dev, &evtchnl->port);
  	if (ret < 0)


--
Regards,

Oleksandr Tyshchenko




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux