Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751644AbdFAJQo (ORCPT ); Thu, 1 Jun 2017 05:16:44 -0400 Received: from mail.fireflyinternet.com ([109.228.58.192]:52176 "EHLO fireflyinternet.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751348AbdFAJQm (ORCPT ); Thu, 1 Jun 2017 05:16:42 -0400 X-Default-Received-SPF: pass (skip=forwardok (res=PASS)) x-ip-name=78.156.65.138; Date: Thu, 1 Jun 2017 10:15:47 +0100 From: Chris Wilson To: Xiaoguang Chen Cc: alex.williamson@redhat.com, kraxel@redhat.com, intel-gfx@lists.freedesktop.org, linux-kernel@vger.kernel.org, zhenyuw@linux.intel.com, zhiyuan.lv@intel.com, intel-gvt-dev@lists.freedesktop.org, zhi.a.wang@intel.com, kevin.tian@intel.com Subject: Re: [PATCH v6 5/6] drm/i915/gvt: Dmabuf support for GVT-g Message-ID: <20170601091547.GB11742@nuc-i3427.alporthouse.com> Mail-Followup-To: Chris Wilson , Xiaoguang Chen , alex.williamson@redhat.com, kraxel@redhat.com, intel-gfx@lists.freedesktop.org, linux-kernel@vger.kernel.org, zhenyuw@linux.intel.com, zhiyuan.lv@intel.com, intel-gvt-dev@lists.freedesktop.org, zhi.a.wang@intel.com, kevin.tian@intel.com References: <1495874332-2851-1-git-send-email-xiaoguang.chen@intel.com> <1495874332-2851-6-git-send-email-xiaoguang.chen@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1495874332-2851-6-git-send-email-xiaoguang.chen@intel.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 11180 Lines: 340 On Sat, May 27, 2017 at 04:38:51PM +0800, Xiaoguang Chen wrote: > dmabuf for GVT-g can be exported to users who can use the dmabuf to show > the desktop of vm which use intel vgpu. > > Currently we provide query and create new dmabuf operations. > > Users of dmabuf can cache some created dmabufs and related information > such as the framebuffer's address, size, tiling mode, width, height etc. > When refresh the screen first query the currnet vgpu's frambuffer and > compare with the cached ones(address, size, tiling, width, height etc) > if found one then reuse the found dmabuf to gain performance improvment. > > If there is no dmabuf created yet or not found in the cached dmabufs then > need to create a new dmabuf. To create a dmabuf first a gem object will > be created and the backing storage of this gem object is the vgpu's > framebuffer(primary/cursor). > Set caching mode, change tiling mode and set domains of this gem object > is not supported. > Then associate this gem object to a dmabuf and export this dmabuf. > A file descriptor will be generated for this dmabuf and this file > descriptor can be sent to user space to do display. > > Signed-off-by: Xiaoguang Chen > --- > drivers/gpu/drm/i915/gvt/Makefile | 2 +- > drivers/gpu/drm/i915/gvt/dmabuf.c | 269 +++++++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/gvt/dmabuf.h | 37 +++++ > drivers/gpu/drm/i915/gvt/gvt.h | 1 + > drivers/gpu/drm/i915/i915_gem.c | 26 +++- > drivers/gpu/drm/i915/i915_gem_object.h | 9 ++ > 6 files changed, 342 insertions(+), 2 deletions(-) > create mode 100644 drivers/gpu/drm/i915/gvt/dmabuf.c > create mode 100644 drivers/gpu/drm/i915/gvt/dmabuf.h > > diff --git a/drivers/gpu/drm/i915/gvt/Makefile b/drivers/gpu/drm/i915/gvt/Makefile > index 192ca26..e480f7d 100644 > --- a/drivers/gpu/drm/i915/gvt/Makefile > +++ b/drivers/gpu/drm/i915/gvt/Makefile > @@ -2,7 +2,7 @@ GVT_DIR := gvt > GVT_SOURCE := gvt.o aperture_gm.o handlers.o vgpu.o trace_points.o firmware.o \ > interrupt.o gtt.o cfg_space.o opregion.o mmio.o display.o edid.o \ > execlist.o scheduler.o sched_policy.o render.o cmd_parser.o \ > - fb_decoder.o > + fb_decoder.o dmabuf.o > > ccflags-y += -I$(src) -I$(src)/$(GVT_DIR) -Wall > i915-y += $(addprefix $(GVT_DIR)/, $(GVT_SOURCE)) > diff --git a/drivers/gpu/drm/i915/gvt/dmabuf.c b/drivers/gpu/drm/i915/gvt/dmabuf.c > new file mode 100644 > index 0000000..c831e91 > --- /dev/null > +++ b/drivers/gpu/drm/i915/gvt/dmabuf.c > @@ -0,0 +1,269 @@ > +/* > + * Copyright 2017 Intel Corporation. All rights reserved. > + * > + * Permission is hereby granted, free of charge, to any person obtaining a > + * copy of this software and associated documentation files (the "Software"), > + * to deal in the Software without restriction, including without limitation > + * the rights to use, copy, modify, merge, publish, distribute, sublicense, > + * and/or sell copies of the Software, and to permit persons to whom the > + * Software is furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice (including the next > + * paragraph) shall be included in all copies or substantial portions of the > + * Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > + * DEALINGS IN THE SOFTWARE. > + * > + * Authors: > + * Zhiyuan Lv > + * > + * Contributors: > + * Xiaoguang Chen > + */ > + > +#include > +#include > +#include > + > +#include "i915_drv.h" > +#include "gvt.h" > + > +#define GEN8_DECODE_PTE(pte) (pte & GENMASK_ULL(63, 12)) > + > +static struct sg_table *intel_vgpu_gem_get_pages( > + struct drm_i915_gem_object *obj) > +{ > + struct drm_i915_private *dev_priv = to_i915(obj->base.dev); > + struct sg_table *st; > + struct scatterlist *sg; > + int i, ret; > + gen8_pte_t __iomem *gtt_entries; > + struct intel_vgpu_fb_info *fb_info; > + > + fb_info = (struct intel_vgpu_fb_info *)obj->gvt_info; > + if (WARN_ON(!fb_info)) > + return ERR_PTR(-ENODEV); > + > + st = kmalloc(sizeof(*st), GFP_KERNEL); > + if (!st) { > + ret = -ENOMEM; > + return ERR_PTR(ret); Tidier. > + } > + > + ret = sg_alloc_table(st, fb_info->fb_size, GFP_KERNEL); > + if (ret) { > + kfree(st); > + return ERR_PTR(ret); > + } > + gtt_entries = (gen8_pte_t __iomem *)dev_priv->ggtt.gsm + > + (fb_info->fb_addr >> PAGE_SHIFT); > + for_each_sg(st->sgl, sg, fb_info->fb_size, i) { > + sg->offset = 0; > + sg->length = PAGE_SIZE; > + sg_dma_address(sg) = > + GEN8_DECODE_PTE(readq(>t_entries[i])); > + sg_dma_len(sg) = PAGE_SIZE; > + } > + > + return st; > +} > + > +static void intel_vgpu_gem_put_pages(struct drm_i915_gem_object *obj, > + struct sg_table *pages) > +{ > + sg_free_table(pages); > + kfree(pages); > +} > + > +static const struct drm_i915_gem_object_ops intel_vgpu_gem_ops = { > + .flags = I915_GEM_OBJECT_IS_PROXY, > + .get_pages = intel_vgpu_gem_get_pages, > + .put_pages = intel_vgpu_gem_put_pages, > +}; > + > +static struct drm_i915_gem_object *intel_vgpu_create_gem(struct drm_device *dev, > + struct vfio_vgpu_plane_info *info) > +{ > + struct drm_i915_private *dev_priv = to_i915(dev); > + struct drm_i915_gem_object *obj; > + > + obj = i915_gem_object_alloc(dev_priv); > + if (obj == NULL) > + return NULL; > + > + drm_gem_private_object_init(dev, &obj->base, > + info->size << PAGE_SHIFT); > + i915_gem_object_init(obj, &intel_vgpu_gem_ops); > + > + obj->base.read_domains = I915_GEM_DOMAIN_GTT; > + obj->base.write_domain = 0; > + /* Change tiling mode from user space is not supported to a GVT-g's > + * dma-buf gem object. > + * We increase the obj's framebuffer_references to disable this kind > + * of operation. > + */ > + obj->framebuffer_references++; If it is only for that, don't. Just use the is_proxy: return -EPERM. So that you don't make the mistake of leaving the counter dangling and the error code is consistent. What you probably should consider though is setting obj->pin_display. That will fix up coherency along a few more paths. > + if (IS_SKYLAKE(dev_priv)) { > + unsigned int tiling_mode = 0; > + unsigned int stride = 0; > + > + switch (info->drm_format_mod << 10) { > + case PLANE_CTL_TILED_LINEAR: > + tiling_mode = I915_TILING_NONE; > + break; > + case PLANE_CTL_TILED_X: > + tiling_mode = I915_TILING_X; > + stride = info->stride; > + break; > + case PLANE_CTL_TILED_Y: > + tiling_mode = I915_TILING_Y; > + stride = info->stride; > + break; > + default: > + gvt_dbg_core("not supported tiling mode\n"); > + } > + obj->tiling_and_stride = tiling_mode | stride; > + } else { > + obj->tiling_and_stride = info->drm_format_mod ? > + I915_TILING_X : 0; > + } > + > + return obj; > +} > + > +int intel_vgpu_query_plane(struct intel_vgpu *vgpu, void *args) > +{ > + struct drm_device *dev = &vgpu->gvt->dev_priv->drm; > + struct vfio_vgpu_plane_info *info = args; > + > + return intel_vgpu_get_plane_info(dev, vgpu, info); > +} > + > +int intel_vgpu_create_dmabuf(struct intel_vgpu *vgpu, void *args) > +{ > + struct dma_buf *dmabuf; > + struct drm_i915_gem_object *obj; > + struct drm_device *dev = &vgpu->gvt->dev_priv->drm; > + struct vfio_vgpu_dmabuf_info *gvt_dmabuf = args; > + struct intel_vgpu_fb_info *fb_info; > + int ret; > + > + ret = intel_vgpu_get_plane_info(dev, vgpu, &gvt_dmabuf->plane_info); > + if (ret != 0) > + return ret; > + > + obj = intel_vgpu_create_gem(dev, &gvt_dmabuf->plane_info); > + if (obj == NULL) { > + gvt_vgpu_err("create gvt gem obj failed:%d\n", vgpu->id); > + return -ENOMEM; > + } > + fb_info = kmalloc(sizeof(*fb_info), GFP_KERNEL); > + if (!fb_info) { > + i915_gem_object_put(obj); > + gvt_vgpu_err("allocate intel vgpu fb info failed\n"); > + return -ENOMEM; > + } > + fb_info->fb_addr = gvt_dmabuf->plane_info.start; > + fb_info->fb_size = gvt_dmabuf->plane_info.size; > + fb_info->vgpu = vgpu; > + obj->gvt_info = fb_info; > + > + dmabuf = i915_gem_prime_export(dev, &obj->base, DRM_CLOEXEC | DRM_RDWR); > + > + if (IS_ERR(dmabuf)) { > + kfree(fb_info); > + i915_gem_object_free(obj); > + gvt_vgpu_err("export dma-buf failed\n"); Develop a habit for using onion unwind for errors. > + return PTR_ERR(dmabuf); > + } > + > + ret = dma_buf_fd(dmabuf, DRM_CLOEXEC | DRM_RDWR); > + if (ret < 0) { > + kfree(fb_info); > + i915_gem_object_free(obj); > + gvt_vgpu_err("create dma-buf fd failed ret:%d\n", ret); > + return ret; > + } > + gvt_dmabuf->fd = ret; > + > + return 0; > +} > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 0c1cbe9..14be3b0 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -1609,6 +1609,12 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data, > if (!obj) > return -ENOENT; > > + /* proxy gem object does not support setting domain */ > + if (i915_gem_object_is_proxy(obj)) { > + i915_gem_object_put(obj); > + return -EPERM; Hook into the onion. > + } > + > /* Try to flush the object off the GPU without holding the lock. > * We will repeat the flush holding the lock in the normal manner > * to catch cases where we are gazumped. > @@ -1677,6 +1683,12 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data, > if (!obj) > return -ENOENT; > > + /* proxy gem obj does not support this operation */ > + if (i915_gem_object_is_proxy(obj)) { > + i915_gem_object_put(obj); > + return -EPERM; > + } > + > /* Pinned buffers may be scanout, so flush the cache */ > i915_gem_object_flush_if_display(obj); > i915_gem_object_put(obj); > @@ -1727,7 +1739,7 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data, > */ > if (!obj->base.filp) { > i915_gem_object_put(obj); > - return -EINVAL; > + return -EPERM; > } > > addr = vm_mmap(obj->base.filp, 0, args->size, > @@ -3717,6 +3729,12 @@ int i915_gem_set_caching_ioctl(struct drm_device *dev, void *data, > if (!obj) > return -ENOENT; > > + /* proxy gem obj does not support setting caching mode */ > + if (i915_gem_object_is_proxy(obj)) { > + i915_gem_object_put(obj); > + return -EPERM; Use the onion. > + } > + > if (obj->cache_level == level) > goto out; > > @@ -4168,6 +4186,12 @@ i915_gem_madvise_ioctl(struct drm_device *dev, void *data, > if (!obj) > return -ENOENT; > > + /* proxy gem obj does not support changing backding storage */ > + if (i915_gem_object_is_proxy(obj)) { > + i915_gem_object_put(obj); More onion. > + return -EPERM; > + } > + > err = mutex_lock_interruptible(&obj->mm.lock); > if (err) > goto out; -- Chris Wilson, Intel Open Source Technology Centre