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

[Oleksandr <olekstysh@xxxxxxxxx>]

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