Re: [PATCH v2 4/5] drm/i915/gvt: Dmabuf support for GVT-g

[Chris Wilson]

On Thu, May 18, 2017 at 05:50:04PM +0800, Xiaoguang Chen wrote:
> +#define GEN8_DECODE_PTE(pte) \
> +	((dma_addr_t)(((((u64)pte) >> 12) & 0x7ffffffULL) << 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 intel_gvt *gvt = dev_priv->gvt;
> +	struct sg_table *st;
> +	struct scatterlist *sg;
> +	int i, ret;
> +	gen8_pte_t __iomem *gtt_entries;
> +	struct intel_vgpu *vgpu;
> +	unsigned int fb_gma = 0, fb_size = 0;
> +	bool found = false;
> +
> +	mutex_lock(&gvt->lock);
> +	for_each_active_vgpu(gvt, vgpu, i) {
> +		if (vgpu->obj_dmabuf == obj) {
> +			fb_gma = vgpu->plane_info->start;
> +			fb_size = vgpu->plane_info->size;
> +			found = true;
> +			break;
> +		}
> +	}
> +	mutex_unlock(&gvt->lock);
> +
> +	if (!found) {
> +		gvt_vgpu_err("no vgpu found\n");
> +		return NULL;
> +	}
> +
> +	st = kmalloc(sizeof(*st), GFP_KERNEL);
> +	if (!st) {
> +		ret = -ENOMEM;
> +		return ERR_PTR(ret);
> +	}
> +
> +	ret = sg_alloc_table(st, fb_size, GFP_KERNEL);
> +	if (ret) {
> +		kfree(st);
> +		return ERR_PTR(ret);
> +	}
> +	gtt_entries = (gen8_pte_t __iomem *)dev_priv->ggtt.gsm +
> +		(fb_gma >> PAGE_SHIFT);
> +	for_each_sg(st->sgl, sg, fb_size, i) {
> +		sg->offset = 0;
> +		sg->length = PAGE_SIZE;
> +		sg_dma_address(sg) =
> +			GEN8_DECODE_PTE(readq(&gtt_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)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
> +	struct intel_gvt *gvt = dev_priv->gvt;
> +	int i;
> +	struct intel_vgpu *vgpu;
> +
> +	mutex_lock(&gvt->lock);
> +	for_each_active_vgpu(gvt, vgpu, i) {
> +		if (vgpu->obj_dmabuf == obj) {
> +			kfree(vgpu->plane_info);
> +			break;
> +		}

We have a union for private data in i915_gem_object that you can use to
store the backpointer.

> +	}
> +	mutex_unlock(&gvt->lock);
> +
> +	sg_free_table(pages);
> +	kfree(pages);
> +
> +	i915_gem_object_unpin_pages(obj);

That's confused and should be triggering at least 2 WARNs when the
object is freed.

> +}
> +
> +static const struct drm_i915_gem_object_ops intel_vgpu_gem_ops = {
> +	.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 intel_vgpu *vgpu)
> +{
> +	struct drm_i915_private *pri = dev->dev_private;
> +	struct drm_i915_gem_object *obj;
> +	struct intel_vgpu_plane_info *info = vgpu->plane_info;
> +
> +	obj = i915_gem_object_alloc(pri);
> +	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;
> +
> +	if (IS_SKYLAKE(pri)) {
> +		unsigned int tiling_mode = 0;
> +
> +		switch (info->tiled << 10) {
> +		case PLANE_CTL_TILED_LINEAR:
> +			tiling_mode = I915_TILING_NONE;
> +			break;
> +		case PLANE_CTL_TILED_X:
> +			tiling_mode = I915_TILING_X;
> +			break;
> +		case PLANE_CTL_TILED_Y:
> +			tiling_mode = I915_TILING_Y;
> +			break;
> +		default:
> +			gvt_vgpu_err("tile %d not supported\n", info->tiled);
> +		}
> +		obj->tiling_and_stride = tiling_mode | info->stride;
> +	} else {
> +		obj->tiling_and_stride = info->tiled ? I915_TILING_X : 0;
> +	}

How good a first class object is this? If I understand this correctly,
we have a valid DMA mapping for the object, so we can access it via GTT
mmaps and rendering on the GPU. We just can't use CPU mmaps.

However, we should reject things like tiling changes, cache level
changes, set-domain is also problematic.

> +
> +	return obj;
> +}

> +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 intel_vgpu_dmabuf *gvt_dmabuf = args;
> +	struct intel_vgpu_plane_info *info;
> +	int ret;
> +
> +	info = intel_vgpu_get_plane_info(dev, vgpu, gvt_dmabuf->plane_id);
> +	if (info == NULL)
> +		return -EINVAL;
> +
> +	vgpu->plane_info = info;
> +	obj = intel_vgpu_create_gem(dev, vgpu);
> +	if (obj == NULL) {
> +		gvt_vgpu_err("create gvt gem obj failed:%d\n", vgpu->id);
> +		return -EINVAL;
> +	}
> +	vgpu->obj_dmabuf = obj;
> +
> +	ret = i915_gem_object_pin_pages(obj);

Why do you need to pin the pages? It has no relevance to the backing
storage, that can be discard freely by the other end of vgpu. get_pages
is just allocating an sg_table...
-chris

-- 
Chris Wilson, Intel Open Source Technology Centre