Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <76882e3a-6061-e5f3-fe69-7928de91ba17@amd.com>
Date:   Fri, 26 Aug 2022 09:29:36 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH] drm/ast: add dmabuf/prime buffer sharing support
Content-Language: en-US
To:     oushixiong <oushixiong@kylinos.cn>,
        Dave Airlie <airlied@redhat.com>
Cc:     Thomas Zimmermann <tzimmermann@suse.de>,
        David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>,
        Sumit Semwal <sumit.semwal@linaro.org>,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
        linux-media@vger.kernel.org, linaro-mm-sig@lists.linaro.org
References: <20220826013103.1519411-1-oushixiong@kylinos.cn>
From:   =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>
In-Reply-To: <20220826013103.1519411-1-oushixiong@kylinos.cn>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?eGthMXExUUdBbWdrcjJPVll4MGlIUDAxd2QrbHVseERzS0VKeGR3dkNoNmdP?=
 =?utf-8?B?T3BqK1kxLzNPenVXRnV6bWRmbFJpcFhXUGE0VHp3N0Z3alBEVGgwR1BDL21w?=
 =?utf-8?B?cFJJZnpMMndHWXByQ3FkQ1JXaEdRY0FwTmtBOERjenVyRTF1VlJ6ckZ4UC9h?=
 =?utf-8?B?TmJGK21FUmNrVW90bndyYU1jQ2Vtc2tyYlpObEtCSlIrWFlKVVBHaG5QS0Jv?=
 =?utf-8?B?ZHNBNmVsU1BKOWtVdStZbzZLSkhIaVJrK2JNM0t5M0RVYThlRjJkSDdjUFNx?=
 =?utf-8?B?UWFOUkdwbWNVZ2c1dTVMMTRlZVRpbEVwY2xlZVBZNVp6ZjBCL2hOUURIV0VQ?=
 =?utf-8?B?TEs3cnNIWFdaSkhGbFB4anVnME1QNVJnREZZUkZ3S1lxMFRNejNnejdZdWdD?=
 =?utf-8?B?Qm91LzV5VXZaMmtLZURqbnZtYnN2YS9kUDF0cTVwL2U3Q1VzUjRDTnh0bGdX?=
 =?utf-8?B?MFFUZk1hUmhjMS93L3RGRE5RbnNERHgxSEw1SWo3QnJxdTk4aDNuZUtQbGds?=
 =?utf-8?B?bHRxSWRoS2R4TGRHNERtZk5IblpwT2c3QlhzdGVOaUxqNG96S3dObnR0ZWF6?=
 =?utf-8?B?ekhPZjhSM3VqS29xUnpZZlpQd0dlY0YyQVU1SXovN21SZzRsSFJhalB4R2FN?=
 =?utf-8?B?UWlkZ0Q4Q0RpendFZnhCb1dRRXlaU2prWVY0MVNBeGxyeTRPNzFYa3dUQnFW?=
 =?utf-8?B?YjN1bkRGRjFwdDdZcThiaEplay9EUGRib3Mwc3ZNVTZxakxyRUZhRk8vOGJ3?=
 =?utf-8?B?ZTErb2hSRWdIQlBPcVJzd2VWdkRFM0JMNlY2YXRiSW5WWHQyZFJtV3Y2SGIx?=
 =?utf-8?B?Ny85WmtrRUFOcjFlZDl1ZTRQMVBHUHp2UVI4b0Y5UnZtQzlTR0Q5bUZFY0tS?=
 =?utf-8?B?TWNFU2NDcGwyOHVQOXMxR3NDMi9IWEcwbUpEM0hQMFFBMzFyeWVhRkYraXBC?=
 =?utf-8?B?MUFtMXdSUkl1a21ZYUlXN3Y4dTl3ZG40NWVwMklrbnBnMGsrdmYxcFRpUVFI?=
 =?utf-8?B?c3dvSVZudzlyYUF0ZHFaRzZRRWdsTFVwZ0c0aWkwS09KTkV4YWEzcHlvY3Y2?=
 =?utf-8?B?T25RdlRLWG1KQi9LdklxMDdJMXRZbmNWZEVUa1dGMkl3cjFsTFZRc2loa0hD?=
 =?utf-8?B?Z3JHcUxoWGN6cGljUlRVYzhITVpLdzd5TGNTSFhEQlRaRUxpdmxXMHBNSWtm?=
 =?utf-8?B?QS9DVnNlS2NVWFZ0NU5Cd3dMVkZuTFluRnVNM2FtSnRyMlY5Z3ZNelNjakIv?=
 =?utf-8?B?a0NPMzNudkEyOVhGS05uVytUNW1FZlhsRmhTYzdKYWZLdGZIbGVYc09CbWxi?=
 =?utf-8?B?b1pMTGgwZG81c3JGZFc1a0J5eVFFNmVTeFVzcGJrOHhxbTFvYlhVVmtmck0x?=
 =?utf-8?B?WUl1VmpqUmNBak1IZmgzYVNhRjJtR3N4NDVETUtDT2o3Vk1ZSGl6b3lSWHF2?=
 =?utf-8?B?bzBrcGtlU1pOUFkxemNTcmY2SFBiamtweENRWlY5aXBjT1dWcGQ0bEMwREt0?=
 =?utf-8?B?bUxvalFtRmVvZzU5RnhyNXllcFpKMEE5ZlJnQS8zMjR6T3daUnJzcW0yb1NH?=
 =?utf-8?B?cVFIVDB3eXEyTktFTjZjdy8vajc4NGpjOXQvM2VkaDNKZVhYZVB4S2k4Y1Va?=
 =?utf-8?B?YjBYTWtYb3RneWdGRVBiMGtETllqZ09pNjdMQWs0TkVYRnhRY0tjVXArcDl5?=
 =?utf-8?B?SlFDV05GK2RYaDJBWnNZbCttSjRHVUw3dkJlaHhWK3VrblEwOEFVUDlJazVN?=
 =?utf-8?B?YTE4NVVRYVp1dGpPY1ZwU1Mzd3JIMVBMbXA0aE5CZ1B0NjVTTWYzcVAxL3Zm?=
 =?utf-8?B?Qi9PamsxcUtlNGF1UTNPSUdHRCtOVndKQlIzK2dhcEtYZUowYkpGUG05K2w5?=
 =?utf-8?B?bHp4T0ZiQlJ5QXFxZ25ndmgxQ0RJMFEzdk11VUREQWhXQUszOURWMXNUS3Z1?=
 =?utf-8?B?U3VkV1lVRkF3RnNjSTFRS0EyY3ZoVkRhZDgxaUZZUkZmek1WaWxWdEZCb2hS?=
 =?utf-8?B?V2tMRTRoRnNRTUpuOCt4enZaM3FCZHlZRmxVd0thOTllUmFKUkdIVDFHdWVY?=
 =?utf-8?B?cHlNM2ZlVW1pTHBDa1ZCamdsU1dvN0pIcERtcXU5S0hDS0lGdUlZa1hXMzNs?=
 =?utf-8?Q?Uqh7bFLsQ4y8Q7dvg+AWsONDQ?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 65a3a6b1-086e-4f28-0c88-08da8734bed1
X-MS-Exchange-CrossTenant-AuthSource: BN8PR12MB3587.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Aug 2022 07:29:43.6860
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: SkfMWjPJUcjKoRt0Xds8LMJ4jxKHjYsnz8E3puMph1M2JHSRxEcpVctqIUfgXTm3
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR12MB6333
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Am 26.08.22 um 03:31 schrieb oushixiong:
> This patch adds ast specific codes for DRM prime feature, this is to
> allow for offloading of rending in one direction and outputs in other.
>
> This patch is designed to solve the problem that the AST is not displayed
> when the server plug in a discrete graphics graphics card at the same time.
> We call the dirty callback function to copy the rendering results of the
> discrete graphics card to the ast side by dma-buf.
>
> v1->v2:
>    - Fix the comment.
> v2->v3:
>    - we remove the attach function, add the drm_gem_pin() before dma_buf_vmap(),
>      and add the drm_gem_unpin() after the dma_buf_vunmap().
>
> Signed-off-by: oushixiong <oushixiong@kylinos.cn>
> ---
>   drivers/gpu/drm/ast/ast_drv.c  |  27 +++++++
>   drivers/gpu/drm/ast/ast_mode.c | 134 ++++++++++++++++++++++++++++++++-
>   drivers/gpu/drm/drm_gem.c      |   2 +
>   include/drm/drm_gem.h          |   3 +
>   4 files changed, 165 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
> index 7465c4f0156a..107383a56ca7 100644
> --- a/drivers/gpu/drm/ast/ast_drv.c
> +++ b/drivers/gpu/drm/ast/ast_drv.c
> @@ -28,6 +28,7 @@
>   
>   #include <linux/module.h>
>   #include <linux/pci.h>
> +#include <linux/dma-buf.h>
>   
>   #include <drm/drm_aperture.h>
>   #include <drm/drm_atomic_helper.h>
> @@ -50,6 +51,29 @@ module_param_named(modeset, ast_modeset, int, 0400);
>   
>   DEFINE_DRM_GEM_FOPS(ast_fops);
>   
> +struct drm_gem_object *ast_gem_prime_import(struct drm_device *dev,
> +						struct dma_buf *dma_buf)
> +{
> +	struct drm_gem_vram_object *gbo;
> +
> +	gbo = drm_gem_vram_of_gem(dma_buf->priv);
> +	if (gbo->bo.base.dev == dev) {
> +		/*
> +		* Importing dmabuf exported from out own gem increases
> +		* refcount on gem itself instead of f_count of dmabuf.
> +		*/
> +		drm_gem_object_get(&gbo->bo.base);
> +		return &gbo->bo.base;
> +	}
> +
> +	gbo = drm_gem_vram_create(dev, dma_buf->size, 0);
> +	if (IS_ERR(gbo))
> +		return NULL;
> +
> +	get_dma_buf(dma_buf);
> +	return &gbo->bo.base;
> +}
> +
>   static const struct drm_driver ast_driver = {
>   	.driver_features = DRIVER_ATOMIC |
>   			   DRIVER_GEM |
> @@ -63,6 +87,9 @@ static const struct drm_driver ast_driver = {
>   	.minor = DRIVER_MINOR,
>   	.patchlevel = DRIVER_PATCHLEVEL,
>   
> +	.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
> +	.gem_prime_import = ast_gem_prime_import,
> +
>   	DRM_GEM_VRAM_DRIVER
>   };
>   
> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
> index 45b56b39ad47..c81a6148b6df 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -48,6 +48,8 @@
>   #include "ast_drv.h"
>   #include "ast_tables.h"
>   
> +MODULE_IMPORT_NS(DMA_BUF);
> +
>   static inline void ast_load_palette_index(struct ast_private *ast,
>   				     u8 index, u8 red, u8 green,
>   				     u8 blue)
> @@ -1535,8 +1537,138 @@ static const struct drm_mode_config_helper_funcs ast_mode_config_helper_funcs =
>   	.atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
>   };
>   
> +static int ast_handle_damage(struct drm_framebuffer *fb, int x, int y,
> +					int width, int height)
> +{
> +	struct drm_gem_vram_object *dst_bo = NULL;
> +	void *dst = NULL;
> +	int ret = 0, i;
> +	unsigned long offset = 0;
> +	bool unmap = false;
> +	unsigned int bytesPerPixel;
> +	struct iosys_map map;
> +	struct iosys_map dmabuf_map;
> +
> +	bytesPerPixel = fb->format->cpp[0];
> +
> +	if (!fb->obj[0]->dma_buf)
> +		return -EINVAL;
> +
> +	if (!fb->obj[0]->dma_buf->vmap_ptr.vaddr) {
> +		ret = drm_gem_pin(fb->obj[0]->dma_buf->priv);
> +		if (ret)
> +			return ret;

This is an full blown NAK to this!

By accessing dma_buf->priv you are assuming that this points to a GEM 
buffer.

Additional to that pinning the buffer shouldn't be necessary for a call 
to dma_buf_vmap().

Regards,
Christian.

> +		ret = dma_buf_vmap(fb->obj[0]->dma_buf, &dmabuf_map);
> +		if (ret)
> +			goto err_vmap_dmabuf;
> +	} else
> +		dmabuf_map.vaddr = fb->obj[0]->dma_buf->vmap_ptr.vaddr;
> +
> +	dst_bo = drm_gem_vram_of_gem(fb->obj[0]);
> +
> +	ret = drm_gem_vram_pin(dst_bo, 0);
> +	if (ret) {
> +		DRM_ERROR("ast_bo_pin failed\n");
> +		goto err_ast_pin;
> +	}
> +
> +	if (!dst_bo->map.vaddr) {
> +		ret = drm_gem_vram_vmap(dst_bo, &map);
> +		if (ret) {
> +			DRM_ERROR("failed to vmap fbcon\n");
> +			goto err_vmap_ast_bo;
> +		}
> +		unmap = true;
> +	}
> +	dst = dst_bo->map.vaddr;
> +
> +	for (i = y; i < y + height; i++) {
> +		offset = i * fb->pitches[0] + (x * bytesPerPixel);
> +		memcpy_toio(dst + offset, dmabuf_map.vaddr + offset,
> +			width * bytesPerPixel);
> +	}
> +
> +	if (unmap)
> +		drm_gem_vram_vunmap(dst_bo, &map);
> +
> +	drm_gem_vram_unpin(dst_bo);
> +	return 0;
> +
> +err_vmap_ast_bo:
> +	drm_gem_vram_unpin(dst_bo);
> +err_ast_pin:
> +err_vmap_dmabuf:
> +	drm_gem_unpin(fb->obj[0]->dma_buf->priv);
> +	return ret;
> +}
> +
> +
> +static int ast_user_framebuffer_dirty(struct drm_framebuffer *fb,
> +				struct drm_file *file,
> +				unsigned int flags,
> +				unsigned int color,
> +				struct drm_clip_rect *clips,
> +				unsigned int num_clips)
> +{
> +	int i, ret = 0;
> +
> +	drm_modeset_lock_all(fb->dev);
> +	if (fb->obj[0]->dma_buf) {
> +		ret = dma_buf_begin_cpu_access(fb->obj[0]->dma_buf,
> +				DMA_FROM_DEVICE);
> +		if (ret)
> +			goto unlock;
> +	}
> +
> +	for (i = 0; i < num_clips; i++) {
> +		ret = ast_handle_damage(fb, clips[i].x1, clips[i].y1,
> +				clips[i].x2 - clips[i].x1, clips[i].y2 - clips[i].y1);
> +		if (ret)
> +			break;
> +	}
> +
> +	if (fb->obj[0]->dma_buf) {
> +		dma_buf_end_cpu_access(fb->obj[0]->dma_buf,
> +				DMA_FROM_DEVICE);
> +	}
> +
> +unlock:
> +	drm_modeset_unlock_all(fb->dev);
> +
> +	return ret;
> +}
> +
> +static void ast_user_framebuffer_destroy(struct drm_framebuffer *fb)
> +{
> +	struct iosys_map dmabuf_map;
> +
> +	if (fb->obj[0]->dma_buf) {
> +		dmabuf_map.is_iomem = fb->obj[0]->dma_buf->vmap_ptr.is_iomem;
> +		dmabuf_map.vaddr = fb->obj[0]->dma_buf->vmap_ptr.vaddr;
> +		if (dmabuf_map.vaddr)
> +			dma_buf_vunmap(fb->obj[0]->dma_buf, &dmabuf_map);
> +		drm_gem_unpin(fb->obj[0]->dma_buf->priv);
> +	}
> +
> +	drm_gem_fb_destroy(fb);
> +}
> +
> +static const struct drm_framebuffer_funcs ast_gem_fb_funcs_dirtyfb = {
> +	.destroy	= ast_user_framebuffer_destroy,
> +	.create_handle	= drm_gem_fb_create_handle,
> +	.dirty		= ast_user_framebuffer_dirty,
> +};
> +
> +static struct drm_framebuffer *
> +ast_gem_fb_create_with_dirty(struct drm_device *dev, struct drm_file *file,
> +				const struct drm_mode_fb_cmd2 *mode_cmd)
> +{
> +	return drm_gem_fb_create_with_funcs(dev, file, mode_cmd,
> +					&ast_gem_fb_funcs_dirtyfb);
> +}
> +
>   static const struct drm_mode_config_funcs ast_mode_config_funcs = {
> -	.fb_create = drm_gem_fb_create,
> +	.fb_create = ast_gem_fb_create_with_dirty,
>   	.mode_valid = drm_vram_helper_mode_valid,
>   	.atomic_check = drm_atomic_helper_check,
>   	.atomic_commit = drm_atomic_helper_commit,
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 56fb87885146..3a4f5137abc5 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -1159,12 +1159,14 @@ int drm_gem_pin(struct drm_gem_object *obj)
>   	else
>   		return 0;
>   }
> +EXPORT_SYMBOL(drm_gem_pin);
>   
>   void drm_gem_unpin(struct drm_gem_object *obj)
>   {
>   	if (obj->funcs->unpin)
>   		obj->funcs->unpin(obj);
>   }
> +EXPORT_SYMBOL(drm_gem_unpin);
>   
>   int drm_gem_vmap(struct drm_gem_object *obj, struct iosys_map *map)
>   {
> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> index e2941cee14b6..30c4366968bf 100644
> --- a/include/drm/drm_gem.h
> +++ b/include/drm/drm_gem.h
> @@ -352,6 +352,9 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
>   		     struct vm_area_struct *vma);
>   int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma);
>   
> +int drm_gem_pin(struct drm_gem_object *obj);
> +void drm_gem_unpin(struct drm_gem_object *obj);
> +
>   /**
>    * drm_gem_object_get - acquire a GEM buffer object reference
>    * @obj: GEM buffer object