From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751194AbeCFHnW (ORCPT ); Tue, 6 Mar 2018 02:43:22 -0500 Received: from mail-lf0-f66.google.com ([209.85.215.66]:46057 "EHLO mail-lf0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750730AbeCFHnU (ORCPT ); Tue, 6 Mar 2018 02:43:20 -0500 X-Google-Smtp-Source: AG47ELutIIB0Y3PG0ioYoQRDFKOvk08Tyzhua6Ykh1TnvpT3X3sDSODt9BfUnUGSJ1Ds34zA9Qp2vA== Subject: Re: [PATCH 8/9] drm/xen-front: Implement GEM operations To: xen-devel@lists.xenproject.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, airlied@linux.ie, daniel.vetter@intel.com, seanpaul@chromium.org, gustavo@padovan.org, jgross@suse.com, boris.ostrovsky@oracle.com, konrad.wilk@oracle.com, Oleksandr Andrushchenko References: <1519200222-20623-1-git-send-email-andr2000@gmail.com> <1519200222-20623-9-git-send-email-andr2000@gmail.com> <20180305093225.GK22212@phenom.ffwll.local> <20180306072616.GQ22212@phenom.ffwll.local> From: Oleksandr Andrushchenko Message-ID: Date: Tue, 6 Mar 2018 09:43:16 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <20180306072616.GQ22212@phenom.ffwll.local> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 03/06/2018 09:26 AM, Daniel Vetter wrote: > On Mon, Mar 05, 2018 at 03:46:07PM +0200, Oleksandr Andrushchenko wrote: >> On 03/05/2018 11:32 AM, Daniel Vetter wrote: >>> On Wed, Feb 21, 2018 at 10:03:41AM +0200, Oleksandr Andrushchenko wrote: >>>> From: Oleksandr Andrushchenko >>>> >>>> Implement GEM handling depending on driver mode of operation: >>>> depending on the requirements for the para-virtualized environment, namely >>>> requirements dictated by the accompanying DRM/(v)GPU drivers running in both >>>> host and guest environments, number of operating modes of para-virtualized >>>> display driver are supported: >>>> - display buffers can be allocated by either frontend driver or backend >>>> - display buffers can be allocated to be contiguous in memory or not >>>> >>>> Note! Frontend driver itself has no dependency on contiguous memory for >>>> its operation. >>>> >>>> 1. Buffers allocated by the frontend driver. >>>> >>>> The below modes of operation are configured at compile-time via >>>> frontend driver's kernel configuration. >>>> >>>> 1.1. Front driver configured to use GEM CMA helpers >>>> This use-case is useful when used with accompanying DRM/vGPU driver in >>>> guest domain which was designed to only work with contiguous buffers, >>>> e.g. DRM driver based on GEM CMA helpers: such drivers can only import >>>> contiguous PRIME buffers, thus requiring frontend driver to provide >>>> such. In order to implement this mode of operation para-virtualized >>>> frontend driver can be configured to use GEM CMA helpers. >>>> >>>> 1.2. Front driver doesn't use GEM CMA >>>> If accompanying drivers can cope with non-contiguous memory then, to >>>> lower pressure on CMA subsystem of the kernel, driver can allocate >>>> buffers from system memory. >>>> >>>> Note! If used with accompanying DRM/(v)GPU drivers this mode of operation >>>> may require IOMMU support on the platform, so accompanying DRM/vGPU >>>> hardware can still reach display buffer memory while importing PRIME >>>> buffers from the frontend driver. >>>> >>>> 2. Buffers allocated by the backend >>>> >>>> This mode of operation is run-time configured via guest domain configuration >>>> through XenStore entries. >>>> >>>> For systems which do not provide IOMMU support, but having specific >>>> requirements for display buffers it is possible to allocate such buffers >>>> at backend side and share those with the frontend. >>>> For example, if host domain is 1:1 mapped and has DRM/GPU hardware expecting >>>> physically contiguous memory, this allows implementing zero-copying >>>> use-cases. >>>> >>>> Note! Configuration options 1.1 (contiguous display buffers) and 2 (backend >>>> allocated buffers) are not supported at the same time. >>>> >>>> Signed-off-by: Oleksandr Andrushchenko >>> Some suggestions below for some larger cleanup work. >>> -Daniel >>> >>>> --- >>>> drivers/gpu/drm/xen/Kconfig | 13 + >>>> drivers/gpu/drm/xen/Makefile | 6 + >>>> drivers/gpu/drm/xen/xen_drm_front.h | 74 ++++++ >>>> drivers/gpu/drm/xen/xen_drm_front_drv.c | 80 ++++++- >>>> drivers/gpu/drm/xen/xen_drm_front_drv.h | 1 + >>>> drivers/gpu/drm/xen/xen_drm_front_gem.c | 360 ++++++++++++++++++++++++++++ >>>> drivers/gpu/drm/xen/xen_drm_front_gem.h | 46 ++++ >>>> drivers/gpu/drm/xen/xen_drm_front_gem_cma.c | 93 +++++++ >>>> 8 files changed, 667 insertions(+), 6 deletions(-) >>>> create mode 100644 drivers/gpu/drm/xen/xen_drm_front_gem.c >>>> create mode 100644 drivers/gpu/drm/xen/xen_drm_front_gem.h >>>> create mode 100644 drivers/gpu/drm/xen/xen_drm_front_gem_cma.c >>>> >>>> diff --git a/drivers/gpu/drm/xen/Kconfig b/drivers/gpu/drm/xen/Kconfig >>>> index 4cca160782ab..4f4abc91f3b6 100644 >>>> --- a/drivers/gpu/drm/xen/Kconfig >>>> +++ b/drivers/gpu/drm/xen/Kconfig >>>> @@ -15,3 +15,16 @@ config DRM_XEN_FRONTEND >>>> help >>>> Choose this option if you want to enable a para-virtualized >>>> frontend DRM/KMS driver for Xen guest OSes. >>>> + >>>> +config DRM_XEN_FRONTEND_CMA >>>> + bool "Use DRM CMA to allocate dumb buffers" >>>> + depends on DRM_XEN_FRONTEND >>>> + select DRM_KMS_CMA_HELPER >>>> + select DRM_GEM_CMA_HELPER >>>> + help >>>> + Use DRM CMA helpers to allocate display buffers. >>>> + This is useful for the use-cases when guest driver needs to >>>> + share or export buffers to other drivers which only expect >>>> + contiguous buffers. >>>> + Note: in this mode driver cannot use buffers allocated >>>> + by the backend. >>>> diff --git a/drivers/gpu/drm/xen/Makefile b/drivers/gpu/drm/xen/Makefile >>>> index 4fcb0da1a9c5..12376ec78fbc 100644 >>>> --- a/drivers/gpu/drm/xen/Makefile >>>> +++ b/drivers/gpu/drm/xen/Makefile >>>> @@ -8,4 +8,10 @@ drm_xen_front-objs := xen_drm_front.o \ >>>> xen_drm_front_shbuf.o \ >>>> xen_drm_front_cfg.o >>>> +ifeq ($(CONFIG_DRM_XEN_FRONTEND_CMA),y) >>>> + drm_xen_front-objs += xen_drm_front_gem_cma.o >>>> +else >>>> + drm_xen_front-objs += xen_drm_front_gem.o >>>> +endif >>>> + >>>> obj-$(CONFIG_DRM_XEN_FRONTEND) += drm_xen_front.o >>>> diff --git a/drivers/gpu/drm/xen/xen_drm_front.h b/drivers/gpu/drm/xen/xen_drm_front.h >>>> index 9ed5bfb248d0..c6f52c892434 100644 >>>> --- a/drivers/gpu/drm/xen/xen_drm_front.h >>>> +++ b/drivers/gpu/drm/xen/xen_drm_front.h >>>> @@ -34,6 +34,80 @@ >>>> struct xen_drm_front_drm_pipeline; >>>> +/* >>>> + ******************************************************************************* >>>> + * Para-virtualized DRM/KMS frontend driver >>>> + ******************************************************************************* >>>> + * This frontend driver implements Xen para-virtualized display >>>> + * according to the display protocol described at >>>> + * include/xen/interface/io/displif.h >>>> + * >>>> + ******************************************************************************* >>>> + * Driver modes of operation in terms of display buffers used >>>> + ******************************************************************************* >>>> + * Depending on the requirements for the para-virtualized environment, namely >>>> + * requirements dictated by the accompanying DRM/(v)GPU drivers running in both >>>> + * host and guest environments, number of operating modes of para-virtualized >>>> + * display driver are supported: >>>> + * - display buffers can be allocated by either frontend driver or backend >>>> + * - display buffers can be allocated to be contiguous in memory or not >>>> + * >>>> + * Note! Frontend driver itself has no dependency on contiguous memory for >>>> + * its operation. >>>> + * >>>> + ******************************************************************************* >>>> + * 1. Buffers allocated by the frontend driver. >>>> + ******************************************************************************* >>>> + * >>>> + * The below modes of operation are configured at compile-time via >>>> + * frontend driver's kernel configuration. >>>> + * >>>> + * 1.1. Front driver configured to use GEM CMA helpers >>>> + * This use-case is useful when used with accompanying DRM/vGPU driver in >>>> + * guest domain which was designed to only work with contiguous buffers, >>>> + * e.g. DRM driver based on GEM CMA helpers: such drivers can only import >>>> + * contiguous PRIME buffers, thus requiring frontend driver to provide >>>> + * such. In order to implement this mode of operation para-virtualized >>>> + * frontend driver can be configured to use GEM CMA helpers. >>>> + * >>>> + * 1.2. Front driver doesn't use GEM CMA >>>> + * If accompanying drivers can cope with non-contiguous memory then, to >>>> + * lower pressure on CMA subsystem of the kernel, driver can allocate >>>> + * buffers from system memory. >>>> + * >>>> + * Note! If used with accompanying DRM/(v)GPU drivers this mode of operation >>>> + * may require IOMMU support on the platform, so accompanying DRM/vGPU >>>> + * hardware can still reach display buffer memory while importing PRIME >>>> + * buffers from the frontend driver. >>>> + * >>>> + ******************************************************************************* >>>> + * 2. Buffers allocated by the backend >>>> + ******************************************************************************* >>>> + * >>>> + * This mode of operation is run-time configured via guest domain configuration >>>> + * through XenStore entries. >>>> + * >>>> + * For systems which do not provide IOMMU support, but having specific >>>> + * requirements for display buffers it is possible to allocate such buffers >>>> + * at backend side and share those with the frontend. >>>> + * For example, if host domain is 1:1 mapped and has DRM/GPU hardware expecting >>>> + * physically contiguous memory, this allows implementing zero-copying >>>> + * use-cases. >>>> + * >>>> + ******************************************************************************* >>>> + * Driver limitations >>>> + ******************************************************************************* >>>> + * 1. Configuration options 1.1 (contiguous display buffers) and 2 (backend >>>> + * allocated buffers) are not supported at the same time. >>>> + * >>>> + * 2. Only primary plane without additional properties is supported. >>>> + * >>>> + * 3. Only one video mode supported which is configured via XenStore. >>>> + * >>>> + * 4. All CRTCs operate at fixed frequency of 60Hz. >>>> + * >>>> + ******************************************************************************/ >>> Since you've typed this all up, pls convert it to kernel-doc and pull it >>> into a xen-front.rst driver section in Documentation/gpu/ There's a few >>> examples for i915 and vc4 already. >> Do you mean to move or to keep in the driver and add in the >> Documentation? I would prefer to move to have the description >> at single place. > Keep it where it is, but reformat as a correct kerneldoc (it's RST format) > and pull it in as a DOC: section. See > > https://dri.freedesktop.org/docs/drm/doc-guide/kernel-doc.html > > and the other sections in that chapter. Thank you, already tried playing with that and I see that it is way easier to move the description to an rst file than keeping it in the header: for such a description that I have in the header it is not easy to format text properly. For example, if I had those small sections in different files, then probably that have worked fine. So, I will move to xen-front.rst under Documentation/gpu >>>> + >>>> struct xen_drm_front_ops { >>>> int (*mode_set)(struct xen_drm_front_drm_pipeline *pipeline, >>>> uint32_t x, uint32_t y, uint32_t width, uint32_t height, >>>> diff --git a/drivers/gpu/drm/xen/xen_drm_front_drv.c b/drivers/gpu/drm/xen/xen_drm_front_drv.c >>>> index e8862d26ba27..35e7e9cda9d1 100644 >>>> --- a/drivers/gpu/drm/xen/xen_drm_front_drv.c >>>> +++ b/drivers/gpu/drm/xen/xen_drm_front_drv.c >>>> @@ -23,12 +23,58 @@ >>>> #include "xen_drm_front.h" >>>> #include "xen_drm_front_cfg.h" >>>> #include "xen_drm_front_drv.h" >>>> +#include "xen_drm_front_gem.h" >>>> #include "xen_drm_front_kms.h" >>>> static int dumb_create(struct drm_file *filp, >>>> struct drm_device *dev, struct drm_mode_create_dumb *args) >>>> { >>>> - return -EINVAL; >>>> + struct xen_drm_front_drm_info *drm_info = dev->dev_private; >>>> + struct drm_gem_object *obj; >>>> + int ret; >>>> + >>>> + ret = drm_info->gem_ops->dumb_create(filp, dev, args); >>>> + if (ret) >>>> + goto fail; >>>> + >>>> + obj = drm_gem_object_lookup(filp, args->handle); >>>> + if (!obj) { >>>> + ret = -ENOENT; >>>> + goto fail_destroy; >>>> + } >>>> + >>>> + drm_gem_object_unreference_unlocked(obj); >>>> + >>>> + /* >>>> + * In case of CONFIG_DRM_XEN_FRONTEND_CMA gem_obj is constructed >>>> + * via DRM CMA helpers and doesn't have ->pages allocated >>>> + * (xendrm_gem_get_pages will return NULL), but instead can provide >>>> + * sg table >>>> + */ >>> My recommendation is to use an sg table for everything if you deal with >>> mixed objects (CMA, special blocks 1:1 mapped from host, normal pages). >>> That avoids the constant get_pages vs. get_sgt differences. For examples >>> see how e.g. i915 handles the various gem object backends. >> Indeed, I tried to do that this way before, e.g. have all sgt based. >> But at the end of the day Xen shared buffer code in the driver works >> with pages (Xen API is page based there), so sgt then will anyway need >> to be converted into page array. >> For that reason I prefer to work with pages from the beginning, not sgt. >> As to constant get_pages etc. - this is the only expected place in the >> driver for that, so the _from_sgt/_from_pages API is only used here. > Yeah was just a suggestion to simplify the code. But if you have to deal > with both, there's not much point. Agreed >>>> + if (drm_info->gem_ops->get_pages(obj)) >>>> + ret = drm_info->front_ops->dbuf_create_from_pages( >>>> + drm_info->front_info, >>>> + xen_drm_front_dbuf_to_cookie(obj), >>>> + args->width, args->height, args->bpp, >>>> + args->size, >>>> + drm_info->gem_ops->get_pages(obj)); >>>> + else >>>> + ret = drm_info->front_ops->dbuf_create_from_sgt( >>>> + drm_info->front_info, >>>> + xen_drm_front_dbuf_to_cookie(obj), >>>> + args->width, args->height, args->bpp, >>>> + args->size, >>>> + drm_info->gem_ops->prime_get_sg_table(obj)); >>>> + if (ret) >>>> + goto fail_destroy; >>>> + >>>> + return 0; >>>> + >>>> +fail_destroy: >>>> + drm_gem_dumb_destroy(filp, dev, args->handle); >>>> +fail: >>>> + DRM_ERROR("Failed to create dumb buffer: %d\n", ret); >>>> + return ret; >>>> } >>>> static void free_object(struct drm_gem_object *obj) >>>> @@ -37,6 +83,7 @@ static void free_object(struct drm_gem_object *obj) >>>> drm_info->front_ops->dbuf_destroy(drm_info->front_info, >>>> xen_drm_front_dbuf_to_cookie(obj)); >>>> + drm_info->gem_ops->free_object_unlocked(obj); >>>> } >>>> static void on_frame_done(struct platform_device *pdev, >>>> @@ -60,32 +107,52 @@ static void lastclose(struct drm_device *dev) >>>> static int gem_mmap(struct file *filp, struct vm_area_struct *vma) >>>> { >>>> - return -EINVAL; >>>> + struct drm_file *file_priv = filp->private_data; >>>> + struct drm_device *dev = file_priv->minor->dev; >>>> + struct xen_drm_front_drm_info *drm_info = dev->dev_private; >>>> + >>>> + return drm_info->gem_ops->mmap(filp, vma); >>> Uh, so 1 midlayer for the kms stuff and another midlayer for the gem >>> stuff. That's way too much indirection. >> If by KMS you mean front_ops then -1: I will remove front_ops. >> As to gem_ops, please see below >>>> } >>>> static struct sg_table *prime_get_sg_table(struct drm_gem_object *obj) >>>> { >>>> - return NULL; >>>> + struct xen_drm_front_drm_info *drm_info; >>>> + >>>> + drm_info = obj->dev->dev_private; >>>> + return drm_info->gem_ops->prime_get_sg_table(obj); >>>> } >>>> static struct drm_gem_object *prime_import_sg_table(struct drm_device *dev, >>>> struct dma_buf_attachment *attach, struct sg_table *sgt) >>>> { >>>> - return NULL; >>>> + struct xen_drm_front_drm_info *drm_info; >>>> + >>>> + drm_info = dev->dev_private; >>>> + return drm_info->gem_ops->prime_import_sg_table(dev, attach, sgt); >>>> } >>>> static void *prime_vmap(struct drm_gem_object *obj) >>>> { >>>> - return NULL; >>>> + struct xen_drm_front_drm_info *drm_info; >>>> + >>>> + drm_info = obj->dev->dev_private; >>>> + return drm_info->gem_ops->prime_vmap(obj); >>>> } >>>> static void prime_vunmap(struct drm_gem_object *obj, void *vaddr) >>>> { >>>> + struct xen_drm_front_drm_info *drm_info; >>>> + >>>> + drm_info = obj->dev->dev_private; >>>> + drm_info->gem_ops->prime_vunmap(obj, vaddr); >>>> } >>>> static int prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma) >>>> { >>>> - return -EINVAL; >>>> + struct xen_drm_front_drm_info *drm_info; >>>> + >>>> + drm_info = obj->dev->dev_private; >>>> + return drm_info->gem_ops->prime_mmap(obj, vma); >>>> } >>>> static const struct file_operations xendrm_fops = { >>>> @@ -147,6 +214,7 @@ int xen_drm_front_drv_probe(struct platform_device *pdev, >>>> drm_info->front_ops = front_ops; >>>> drm_info->front_ops->on_frame_done = on_frame_done; >>>> + drm_info->gem_ops = xen_drm_front_gem_get_ops(); >>>> drm_info->front_info = cfg->front_info; >>>> dev = drm_dev_alloc(&xen_drm_driver, &pdev->dev); >>>> diff --git a/drivers/gpu/drm/xen/xen_drm_front_drv.h b/drivers/gpu/drm/xen/xen_drm_front_drv.h >>>> index 563318b19f34..34228eb86255 100644 >>>> --- a/drivers/gpu/drm/xen/xen_drm_front_drv.h >>>> +++ b/drivers/gpu/drm/xen/xen_drm_front_drv.h >>>> @@ -43,6 +43,7 @@ struct xen_drm_front_drm_pipeline { >>>> struct xen_drm_front_drm_info { >>>> struct xen_drm_front_info *front_info; >>>> struct xen_drm_front_ops *front_ops; >>>> + const struct xen_drm_front_gem_ops *gem_ops; >>>> struct drm_device *drm_dev; >>>> struct xen_drm_front_cfg *cfg; >>>> diff --git a/drivers/gpu/drm/xen/xen_drm_front_gem.c b/drivers/gpu/drm/xen/xen_drm_front_gem.c >>>> new file mode 100644 >>>> index 000000000000..367e08f6a9ef >>>> --- /dev/null >>>> +++ b/drivers/gpu/drm/xen/xen_drm_front_gem.c >>>> @@ -0,0 +1,360 @@ >>>> +/* >>>> + * Xen para-virtual DRM device >>>> + * >>>> + * This program is free software; you can redistribute it and/or modify >>>> + * it under the terms of the GNU General Public License as published by >>>> + * the Free Software Foundation; either version 2 of the License, or >>>> + * (at your option) any later version. >>>> + * >>>> + * This program is distributed in the hope that it will be useful, >>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >>>> + * GNU General Public License for more details. >>>> + * >>>> + * Copyright (C) 2016-2018 EPAM Systems Inc. >>>> + * >>>> + * Author: Oleksandr Andrushchenko >>>> + */ >>>> + >>>> +#include "xen_drm_front_gem.h" >>>> + >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> + >>>> +#include >>>> +#include >>>> +#include >>>> + >>>> +#include >>>> + >>>> +#include "xen_drm_front.h" >>>> +#include "xen_drm_front_drv.h" >>>> +#include "xen_drm_front_shbuf.h" >>>> + >>>> +struct xen_gem_object { >>>> + struct drm_gem_object base; >>>> + >>>> + size_t num_pages; >>>> + struct page **pages; >>>> + >>>> + /* set for buffers allocated by the backend */ >>>> + bool be_alloc; >>>> + >>>> + /* this is for imported PRIME buffer */ >>>> + struct sg_table *sgt_imported; >>>> +}; >>>> + >>>> +static inline struct xen_gem_object *to_xen_gem_obj( >>>> + struct drm_gem_object *gem_obj) >>>> +{ >>>> + return container_of(gem_obj, struct xen_gem_object, base); >>>> +} >>>> + >>>> +static int gem_alloc_pages_array(struct xen_gem_object *xen_obj, >>>> + size_t buf_size) >>>> +{ >>>> + xen_obj->num_pages = DIV_ROUND_UP(buf_size, PAGE_SIZE); >>>> + xen_obj->pages = kvmalloc_array(xen_obj->num_pages, >>>> + sizeof(struct page *), GFP_KERNEL); >>>> + return xen_obj->pages == NULL ? -ENOMEM : 0; >>>> +} >>>> + >>>> +static void gem_free_pages_array(struct xen_gem_object *xen_obj) >>>> +{ >>>> + kvfree(xen_obj->pages); >>>> + xen_obj->pages = NULL; >>>> +} >>>> + >>>> +static struct xen_gem_object *gem_create_obj(struct drm_device *dev, >>>> + size_t size) >>>> +{ >>>> + struct xen_gem_object *xen_obj; >>>> + int ret; >>>> + >>>> + xen_obj = kzalloc(sizeof(*xen_obj), GFP_KERNEL); >>>> + if (!xen_obj) >>>> + return ERR_PTR(-ENOMEM); >>>> + >>>> + ret = drm_gem_object_init(dev, &xen_obj->base, size); >>>> + if (ret < 0) { >>>> + kfree(xen_obj); >>>> + return ERR_PTR(ret); >>>> + } >>>> + >>>> + return xen_obj; >>>> +} >>>> + >>>> +static struct xen_gem_object *gem_create(struct drm_device *dev, size_t size) >>>> +{ >>>> + struct xen_drm_front_drm_info *drm_info = dev->dev_private; >>>> + struct xen_gem_object *xen_obj; >>>> + int ret; >>>> + >>>> + size = round_up(size, PAGE_SIZE); >>>> + xen_obj = gem_create_obj(dev, size); >>>> + if (IS_ERR_OR_NULL(xen_obj)) >>>> + return xen_obj; >>>> + >>>> + if (drm_info->cfg->be_alloc) { >>>> + /* >>>> + * backend will allocate space for this buffer, so >>>> + * only allocate array of pointers to pages >>>> + */ >>>> + xen_obj->be_alloc = true; >>>> + ret = gem_alloc_pages_array(xen_obj, size); >>>> + if (ret < 0) { >>>> + gem_free_pages_array(xen_obj); >>>> + goto fail; >>>> + } >>>> + >>>> + ret = alloc_xenballooned_pages(xen_obj->num_pages, >>>> + xen_obj->pages); >>>> + if (ret < 0) { >>>> + DRM_ERROR("Cannot allocate %zu ballooned pages: %d\n", >>>> + xen_obj->num_pages, ret); >>>> + goto fail; >>>> + } >>>> + >>>> + return xen_obj; >>>> + } >>>> + /* >>>> + * need to allocate backing pages now, so we can share those >>>> + * with the backend >>>> + */ >>>> + xen_obj->num_pages = DIV_ROUND_UP(size, PAGE_SIZE); >>>> + xen_obj->pages = drm_gem_get_pages(&xen_obj->base); >>>> + if (IS_ERR_OR_NULL(xen_obj->pages)) { >>>> + ret = PTR_ERR(xen_obj->pages); >>>> + xen_obj->pages = NULL; >>>> + goto fail; >>>> + } >>>> + >>>> + return xen_obj; >>>> + >>>> +fail: >>>> + DRM_ERROR("Failed to allocate buffer with size %zu\n", size); >>>> + return ERR_PTR(ret); >>>> +} >>>> + >>>> +static struct xen_gem_object *gem_create_with_handle(struct drm_file *filp, >>>> + struct drm_device *dev, size_t size, uint32_t *handle) >>>> +{ >>>> + struct xen_gem_object *xen_obj; >>>> + struct drm_gem_object *gem_obj; >>>> + int ret; >>>> + >>>> + xen_obj = gem_create(dev, size); >>>> + if (IS_ERR_OR_NULL(xen_obj)) >>>> + return xen_obj; >>>> + >>>> + gem_obj = &xen_obj->base; >>>> + ret = drm_gem_handle_create(filp, gem_obj, handle); >>>> + /* handle holds the reference */ >>>> + drm_gem_object_unreference_unlocked(gem_obj); >>>> + if (ret < 0) >>>> + return ERR_PTR(ret); >>>> + >>>> + return xen_obj; >>>> +} >>>> + >>>> +static int gem_dumb_create(struct drm_file *filp, struct drm_device *dev, >>>> + struct drm_mode_create_dumb *args) >>>> +{ >>>> + struct xen_gem_object *xen_obj; >>>> + >>>> + args->pitch = DIV_ROUND_UP(args->width * args->bpp, 8); >>>> + args->size = args->pitch * args->height; >>>> + >>>> + xen_obj = gem_create_with_handle(filp, dev, args->size, &args->handle); >>>> + if (IS_ERR_OR_NULL(xen_obj)) >>>> + return xen_obj == NULL ? -ENOMEM : PTR_ERR(xen_obj); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static void gem_free_object(struct drm_gem_object *gem_obj) >>>> +{ >>>> + struct xen_gem_object *xen_obj = to_xen_gem_obj(gem_obj); >>>> + >>>> + if (xen_obj->base.import_attach) { >>>> + drm_prime_gem_destroy(&xen_obj->base, xen_obj->sgt_imported); >>>> + gem_free_pages_array(xen_obj); >>>> + } else { >>>> + if (xen_obj->pages) { >>>> + if (xen_obj->be_alloc) { >>>> + free_xenballooned_pages(xen_obj->num_pages, >>>> + xen_obj->pages); >>>> + gem_free_pages_array(xen_obj); >>>> + } else >>>> + drm_gem_put_pages(&xen_obj->base, >>>> + xen_obj->pages, true, false); >>>> + } >>>> + } >>>> + drm_gem_object_release(gem_obj); >>>> + kfree(xen_obj); >>>> +} >>>> + >>>> +static struct page **gem_get_pages(struct drm_gem_object *gem_obj) >>>> +{ >>>> + struct xen_gem_object *xen_obj = to_xen_gem_obj(gem_obj); >>>> + >>>> + return xen_obj->pages; >>>> +} >>>> + >>>> +static struct sg_table *gem_get_sg_table(struct drm_gem_object *gem_obj) >>>> +{ >>>> + struct xen_gem_object *xen_obj = to_xen_gem_obj(gem_obj); >>>> + >>>> + if (!xen_obj->pages) >>>> + return NULL; >>>> + >>>> + return drm_prime_pages_to_sg(xen_obj->pages, xen_obj->num_pages); >>>> +} >>>> + >>>> +static struct drm_gem_object *gem_import_sg_table(struct drm_device *dev, >>>> + struct dma_buf_attachment *attach, struct sg_table *sgt) >>>> +{ >>>> + struct xen_drm_front_drm_info *drm_info = dev->dev_private; >>>> + struct xen_gem_object *xen_obj; >>>> + size_t size; >>>> + int ret; >>>> + >>>> + size = attach->dmabuf->size; >>>> + xen_obj = gem_create_obj(dev, size); >>>> + if (IS_ERR_OR_NULL(xen_obj)) >>>> + return ERR_CAST(xen_obj); >>>> + >>>> + ret = gem_alloc_pages_array(xen_obj, size); >>>> + if (ret < 0) >>>> + return ERR_PTR(ret); >>>> + >>>> + xen_obj->sgt_imported = sgt; >>>> + >>>> + ret = drm_prime_sg_to_page_addr_arrays(sgt, xen_obj->pages, >>>> + NULL, xen_obj->num_pages); >>>> + if (ret < 0) >>>> + return ERR_PTR(ret); >>>> + >>>> + /* >>>> + * N.B. Although we have an API to create display buffer from sgt >>>> + * we use pages API, because we still need those for GEM handling, >>>> + * e.g. for mapping etc. >>>> + */ >>>> + ret = drm_info->front_ops->dbuf_create_from_pages( >>>> + drm_info->front_info, >>>> + xen_drm_front_dbuf_to_cookie(&xen_obj->base), >>>> + 0, 0, 0, size, xen_obj->pages); >>>> + if (ret < 0) >>>> + return ERR_PTR(ret); >>>> + >>>> + DRM_DEBUG("Imported buffer of size %zu with nents %u\n", >>>> + size, sgt->nents); >>>> + >>>> + return &xen_obj->base; >>>> +} >>>> + >>>> +static int gem_mmap_obj(struct xen_gem_object *xen_obj, >>>> + struct vm_area_struct *vma) >>>> +{ >>>> + unsigned long addr = vma->vm_start; >>>> + int i; >>>> + >>>> + /* >>>> + * clear the VM_PFNMAP flag that was set by drm_gem_mmap(), and set the >>>> + * vm_pgoff (used as a fake buffer offset by DRM) to 0 as we want to map >>>> + * the whole buffer. >>>> + */ >>>> + vma->vm_flags &= ~VM_PFNMAP; >>>> + vma->vm_flags |= VM_MIXEDMAP; >>>> + vma->vm_pgoff = 0; >>>> + vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags)); >>>> + >>>> + /* >>>> + * vm_operations_struct.fault handler will be called if CPU access >>>> + * to VM is here. For GPUs this isn't the case, because CPU >>>> + * doesn't touch the memory. Insert pages now, so both CPU and GPU are >>>> + * happy. >>>> + * FIXME: as we insert all the pages now then no .fault handler must >>>> + * be called, so don't provide one >>>> + */ >>>> + for (i = 0; i < xen_obj->num_pages; i++) { >>>> + int ret; >>>> + >>>> + ret = vm_insert_page(vma, addr, xen_obj->pages[i]); >>>> + if (ret < 0) { >>>> + DRM_ERROR("Failed to insert pages into vma: %d\n", ret); >>>> + return ret; >>>> + } >>>> + >>>> + addr += PAGE_SIZE; >>>> + } >>>> + return 0; >>>> +} >>>> + >>>> +static int gem_mmap(struct file *filp, struct vm_area_struct *vma) >>>> +{ >>>> + struct xen_gem_object *xen_obj; >>>> + struct drm_gem_object *gem_obj; >>>> + int ret; >>>> + >>>> + ret = drm_gem_mmap(filp, vma); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>>> + gem_obj = vma->vm_private_data; >>>> + xen_obj = to_xen_gem_obj(gem_obj); >>>> + return gem_mmap_obj(xen_obj, vma); >>>> +} >>>> + >>>> +static void *gem_prime_vmap(struct drm_gem_object *gem_obj) >>>> +{ >>>> + struct xen_gem_object *xen_obj = to_xen_gem_obj(gem_obj); >>>> + >>>> + if (!xen_obj->pages) >>>> + return NULL; >>>> + >>>> + return vmap(xen_obj->pages, xen_obj->num_pages, >>>> + VM_MAP, pgprot_writecombine(PAGE_KERNEL)); >>>> +} >>>> + >>>> +static void gem_prime_vunmap(struct drm_gem_object *gem_obj, void *vaddr) >>>> +{ >>>> + vunmap(vaddr); >>>> +} >>>> + >>>> +static int gem_prime_mmap(struct drm_gem_object *gem_obj, >>>> + struct vm_area_struct *vma) >>>> +{ >>>> + struct xen_gem_object *xen_obj; >>>> + int ret; >>>> + >>>> + ret = drm_gem_mmap_obj(gem_obj, gem_obj->size, vma); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>>> + xen_obj = to_xen_gem_obj(gem_obj); >>>> + return gem_mmap_obj(xen_obj, vma); >>>> +} >>>> + >>>> +static const struct xen_drm_front_gem_ops xen_drm_gem_ops = { >>>> + .free_object_unlocked = gem_free_object, >>>> + .prime_get_sg_table = gem_get_sg_table, >>>> + .prime_import_sg_table = gem_import_sg_table, >>>> + >>>> + .prime_vmap = gem_prime_vmap, >>>> + .prime_vunmap = gem_prime_vunmap, >>>> + .prime_mmap = gem_prime_mmap, >>>> + >>>> + .dumb_create = gem_dumb_create, >>>> + >>>> + .mmap = gem_mmap, >>>> + >>>> + .get_pages = gem_get_pages, >>>> +}; >>>> + >>>> +const struct xen_drm_front_gem_ops *xen_drm_front_gem_get_ops(void) >>>> +{ >>>> + return &xen_drm_gem_ops; >>>> +} >>>> diff --git a/drivers/gpu/drm/xen/xen_drm_front_gem.h b/drivers/gpu/drm/xen/xen_drm_front_gem.h >>>> new file mode 100644 >>>> index 000000000000..d1e1711cc3fc >>>> --- /dev/null >>>> +++ b/drivers/gpu/drm/xen/xen_drm_front_gem.h >>>> @@ -0,0 +1,46 @@ >>>> +/* >>>> + * Xen para-virtual DRM device >>>> + * >>>> + * This program is free software; you can redistribute it and/or modify >>>> + * it under the terms of the GNU General Public License as published by >>>> + * the Free Software Foundation; either version 2 of the License, or >>>> + * (at your option) any later version. >>>> + * >>>> + * This program is distributed in the hope that it will be useful, >>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >>>> + * GNU General Public License for more details. >>>> + * >>>> + * Copyright (C) 2016-2018 EPAM Systems Inc. >>>> + * >>>> + * Author: Oleksandr Andrushchenko >>>> + */ >>>> + >>>> +#ifndef __XEN_DRM_FRONT_GEM_H >>>> +#define __XEN_DRM_FRONT_GEM_H >>>> + >>>> +#include >>>> + >>>> +struct xen_drm_front_gem_ops { >>>> + void (*free_object_unlocked)(struct drm_gem_object *obj); >>>> + >>>> + struct sg_table *(*prime_get_sg_table)(struct drm_gem_object *obj); >>>> + struct drm_gem_object *(*prime_import_sg_table)(struct drm_device *dev, >>>> + struct dma_buf_attachment *attach, >>>> + struct sg_table *sgt); >>>> + void *(*prime_vmap)(struct drm_gem_object *obj); >>>> + void (*prime_vunmap)(struct drm_gem_object *obj, void *vaddr); >>>> + int (*prime_mmap)(struct drm_gem_object *obj, >>>> + struct vm_area_struct *vma); >>>> + >>>> + int (*dumb_create)(struct drm_file *file_priv, struct drm_device *dev, >>>> + struct drm_mode_create_dumb *args); >>>> + >>>> + int (*mmap)(struct file *filp, struct vm_area_struct *vma); >>>> + >>>> + struct page **(*get_pages)(struct drm_gem_object *obj); >>>> +}; >>>> + >>>> +const struct xen_drm_front_gem_ops *xen_drm_front_gem_get_ops(void); >>>> + >>>> +#endif /* __XEN_DRM_FRONT_GEM_H */ >>>> diff --git a/drivers/gpu/drm/xen/xen_drm_front_gem_cma.c b/drivers/gpu/drm/xen/xen_drm_front_gem_cma.c >>>> new file mode 100644 >>>> index 000000000000..5ffcbfa652d5 >>>> --- /dev/null >>>> +++ b/drivers/gpu/drm/xen/xen_drm_front_gem_cma.c >>>> @@ -0,0 +1,93 @@ >>>> +/* >>>> + * Xen para-virtual DRM device >>>> + * >>>> + * This program is free software; you can redistribute it and/or modify >>>> + * it under the terms of the GNU General Public License as published by >>>> + * the Free Software Foundation; either version 2 of the License, or >>>> + * (at your option) any later version. >>>> + * >>>> + * This program is distributed in the hope that it will be useful, >>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >>>> + * GNU General Public License for more details. >>>> + * >>>> + * Copyright (C) 2016-2018 EPAM Systems Inc. >>>> + * >>>> + * Author: Oleksandr Andrushchenko >>>> + */ >>>> + >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> + >>>> +#include "xen_drm_front.h" >>>> +#include "xen_drm_front_drv.h" >>>> +#include "xen_drm_front_gem.h" >>>> + >>>> +static struct drm_gem_object *gem_import_sg_table(struct drm_device *dev, >>>> + struct dma_buf_attachment *attach, struct sg_table *sgt) >>>> +{ >>>> + struct xen_drm_front_drm_info *drm_info = dev->dev_private; >>>> + struct drm_gem_object *gem_obj; >>>> + struct drm_gem_cma_object *cma_obj; >>>> + int ret; >>>> + >>>> + gem_obj = drm_gem_cma_prime_import_sg_table(dev, attach, sgt); >>>> + if (IS_ERR_OR_NULL(gem_obj)) >>>> + return gem_obj; >>>> + >>>> + cma_obj = to_drm_gem_cma_obj(gem_obj); >>>> + >>>> + ret = drm_info->front_ops->dbuf_create_from_sgt( >>>> + drm_info->front_info, >>>> + xen_drm_front_dbuf_to_cookie(gem_obj), >>>> + 0, 0, 0, gem_obj->size, >>>> + drm_gem_cma_prime_get_sg_table(gem_obj)); >>>> + if (ret < 0) >>>> + return ERR_PTR(ret); >>>> + >>>> + DRM_DEBUG("Imported CMA buffer of size %zu\n", gem_obj->size); >>>> + >>>> + return gem_obj; >>>> +} >>>> + >>>> +static int gem_dumb_create(struct drm_file *filp, struct drm_device *dev, >>>> + struct drm_mode_create_dumb *args) >>>> +{ >>>> + struct xen_drm_front_drm_info *drm_info = dev->dev_private; >>>> + >>>> + if (drm_info->cfg->be_alloc) { >>>> + /* This use-case is not yet supported and probably won't be */ >>>> + DRM_ERROR("Backend allocated buffers and CMA helpers are not supported at the same time\n"); >>>> + return -EINVAL; >>>> + } >>>> + >>>> + return drm_gem_cma_dumb_create(filp, dev, args); >>>> +} >>>> + >>>> +static struct page **gem_get_pages(struct drm_gem_object *gem_obj) >>>> +{ >>>> + return NULL; >>>> +} >>>> + >>>> +static const struct xen_drm_front_gem_ops xen_drm_front_gem_cma_ops = { >>>> + .free_object_unlocked = drm_gem_cma_free_object, >>>> + .prime_get_sg_table = drm_gem_cma_prime_get_sg_table, >>>> + .prime_import_sg_table = gem_import_sg_table, >>>> + >>>> + .prime_vmap = drm_gem_cma_prime_vmap, >>>> + .prime_vunmap = drm_gem_cma_prime_vunmap, >>>> + .prime_mmap = drm_gem_cma_prime_mmap, >>>> + >>>> + .dumb_create = gem_dumb_create, >>>> + >>>> + .mmap = drm_gem_cma_mmap, >>>> + >>>> + .get_pages = gem_get_pages, >>>> +}; >>> Again quite a midlayer you have here. Please inline this to avoid >>> confusion for other people (since it looks like you only have 1 >>> implementation). >> There are 2 implementations depending on driver compile time options: >> you can have the GEM operations implemented with DRM CMA helpers >> or driver's cooked GEMs. For this reason this midlayer exists, e.g. >> to eliminate the need for something like >> #ifdef DRM_XEN_FRONTEND_CMA >> drm_gem_cma_...() >> #else >> xen_drm_front_gem_...() >> #endif >> So, I would prefer to have ops rather then having ifdefs > Ok, makes sense, but please review whether you really need all of them, > since for a lot of them (all except get_pages really) we already have > vfuncs. And if you only switch at compile time I think it's cleaner to > simply have 2 vfunc tables for those (e.g. struct drm_driver). That avoids > the indirection. Ok, makes sense. So then I'll only have one #ifdef while defining struct drm_driver. And will take care of get_pages separately > Cheers, Daniel >>>> + >>>> +const struct xen_drm_front_gem_ops *xen_drm_front_gem_get_ops(void) >>>> +{ >>>> + return &xen_drm_front_gem_cma_ops; >>>> +} >>>> -- >>>> 2.7.4 >>>> >>>> _______________________________________________ >>>> dri-devel mailing list >>>> dri-devel@lists.freedesktop.org >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel Thank you, Oleksandr