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@suse.com
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;
page = (unsigned long)evtchnl->u.evt.page;
page = evtchnl->u.evt.page;@@ -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;
evtchnl->gref = gref;
ret = xenbus_alloc_evtchn(xb_dev, &evtchnl->port); if (ret < 0)