Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
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 <oleksandr_andrushchenko@epam.com>
References: <1519200222-20623-1-git-send-email-andr2000@gmail.com>
 <1519200222-20623-9-git-send-email-andr2000@gmail.com>
 <20180305093225.GK22212@phenom.ffwll.local>
 <f0da23ae-e268-ca0c-fdcf-09244a3b598c@gmail.com>
 <20180306072616.GQ22212@phenom.ffwll.local>
From:   Oleksandr Andrushchenko <andr2000@gmail.com>
Message-ID: <a3f2b7bd-5ec0-3dc3-f11b-5fb00f55a35e@gmail.com>
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
Precedence: bulk

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 <oleksandr_andrushchenko@epam.com>
>>>>
>>>> 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 <oleksandr_andrushchenko@epam.com>
>>> 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 <oleksandr_andrushchenko@epam.com>
>>>> + */
>>>> +
>>>> +#include "xen_drm_front_gem.h"
>>>> +
>>>> +#include <drm/drmP.h>
>>>> +#include <drm/drm_crtc_helper.h>
>>>> +#include <drm/drm_fb_helper.h>
>>>> +#include <drm/drm_gem.h>
>>>> +
>>>> +#include <linux/dma-buf.h>
>>>> +#include <linux/scatterlist.h>
>>>> +#include <linux/shmem_fs.h>
>>>> +
>>>> +#include <xen/balloon.h>
>>>> +
>>>> +#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 <oleksandr_andrushchenko@epam.com>
>>>> + */
>>>> +
>>>> +#ifndef __XEN_DRM_FRONT_GEM_H
>>>> +#define __XEN_DRM_FRONT_GEM_H
>>>> +
>>>> +#include <drm/drmP.h>
>>>> +
>>>> +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 <oleksandr_andrushchenko@epam.com>
>>>> + */
>>>> +
>>>> +#include <drm/drmP.h>
>>>> +#include <drm/drm_gem.h>
>>>> +#include <drm/drm_fb_cma_helper.h>
>>>> +#include <drm/drm_gem_cma_helper.h>
>>>> +
>>>> +#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