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: <2ee10e0e-a347-71a5-051a-02b9bac0bbb6@amd.com>
Date:   Fri, 11 Nov 2022 13:05:48 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.2.2
Subject: Re: [PATCH] udmabuf: add vmap method to udmabuf_ops
Content-Language: en-US
To:     Lukasz Wiecaszek <lukasz.wiecaszek@googlemail.com>,
        kraxel@redhat.com, Dmitry Osipenko <dmitry.osipenko@collabora.com>
Cc:     Lukasz Wiecaszek <lukasz.wiecaszek@gmail.com>,
        Sumit Semwal <sumit.semwal@linaro.org>,
        dri-devel@lists.freedesktop.org, linux-media@vger.kernel.org,
        linaro-mm-sig@lists.linaro.org, linux-kernel@vger.kernel.org
References: <20221111114528.608801-1-lukasz.wiecaszek@gmail.com>
From:   =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>
In-Reply-To: <20221111114528.608801-1-lukasz.wiecaszek@gmail.com>
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?N1FqeHVCdVNjbTlFaWl6NW56V1h5RFFHdGNKT1BnYnpISmFGVHc4S3VNcU5X?=
 =?utf-8?B?YjhSM2swVVRsbzFMZ3FML1VsRkpVN2N6Y1kwbXUrYkhKdFkwVTZjVCs4WGxN?=
 =?utf-8?B?TE9WUGNBYmZVdU1tRUszaVh5MytNdjk1RERWMEt4S0lzVFpsaVhVMlhCU3Zq?=
 =?utf-8?B?M3VSSWZjazNVK3RMWVc2dVFxaForcnRqYlFOOEhqaHNPTGNrOXQzd2NYQkxF?=
 =?utf-8?B?TW8vN1VVQWw5TUlsRTJ3eXZ3bjRleWM2UGIyOFM3UUpyNXhzSnVIc3U3U3pk?=
 =?utf-8?B?ZTlINjZ2SmVETUlpMTFxWU0zK3NpL2d3cTc1WFVpa3hWZytQNzZvT0t4NC9J?=
 =?utf-8?B?dG1zQnpJVGJaUnZrck1ZUDNYU3NZN2l6VWJHMmlldGN2djBxK0xLeU4vMS9o?=
 =?utf-8?B?ZjZvVmk2Q3gzaWdsOUpHQUhaMDZCZGxxUU90K2hlVHRxVW15cXpZbnA4akdG?=
 =?utf-8?B?K2djclNkWThkRllKUkloWTBoUk9XM3F4anFrTlRIQ1BQY0trZnFXSDNUZ2JY?=
 =?utf-8?B?b2c0NDI2VU1OSlFuUTIwajRrb0grUHlYMnh6bEw3VGRyS2lWZk8vMjBuNGxW?=
 =?utf-8?B?cE04Wk94T1J3TmptZVVucnMzd2lzWmJFdzBhQ05RMXNscmFZS2owSEVnSW8y?=
 =?utf-8?B?L21jM1NZZEU5THQ1bWVnS3pza1NrdEhHUWtjRGwzdFZkV3A2S3k1b2NSSEI5?=
 =?utf-8?B?SWlON05KbDlBSkw3ZTVXaEc3M0VWY3Ztelg5TUllVm03dTJWbHNzeGp3QSt5?=
 =?utf-8?B?VVJaZldtSFlVYnE3ZGpsQk4zWjNPZ3B6bk8xRTJlQjdHNTU1SzhqajVoOG9w?=
 =?utf-8?B?dENDWjBYc3N6dnZKWFBpdFNKbm44M3NSQ0x0bEw2bDk3dHpobU51ZFVrZm84?=
 =?utf-8?B?Q1AyekdKV1VsMG9oWjA5VlZKOXdMZnFxMURWR1NwRGFrVEpBdHZQTVUvWGFD?=
 =?utf-8?B?NmdwZ0dOL29ncEZwUUhDZnNDR2tvL2lCYis0SUdhZU9rSzFyVE5ZcThWbnFk?=
 =?utf-8?B?cGdYZG5JT2JoSEFqWnJTRC9YK2hvMnFhT1p4SFdsbE9WbTI5Vm9LU094Qy9Q?=
 =?utf-8?B?bG52TlQwUGpkTURSL3dCY1NSU0lyMExhdDE0WXNaNUZhamwvaGxKY3BHWitQ?=
 =?utf-8?B?d0lwdXkwQjdtUUdLdmZ5SnFTbU93ZU9lcXJkQjVIOGJ0NGlSZXRsWGpoVmN2?=
 =?utf-8?B?dGNTRGZJQ20wS1l2ZmNyOGhzcWR5SVlldFNIZnp6V1J5VW5OSEY3VUx3OE1E?=
 =?utf-8?B?VThPK0hidGhKYldtbEFvTGpIL01WR0JmNXJ0dFRhRVIwOXRXV2FPRXIwUUlX?=
 =?utf-8?B?NGpTdm9keWhZNThOaUR3MHNWNEZPWHdOR2N1Sm1jaHlpb2NaWFFEaW5VbUdv?=
 =?utf-8?B?b0JxUzJIck5jdHludkdBZzVpbmxVTnZMK2w1M3NvVk5nd2t1ejI3MWl5VTVv?=
 =?utf-8?B?MkxJZmRYVS82UDhhRWw1WnlsVWV2anNZUGNKN0IzS3RwZEoyUEh5dE1XWHZ0?=
 =?utf-8?B?Z01zN0RYOTF6dWx0RFlKTk5xTHltWjZ2T29uRDQzSWxvVGM1Q2JtZ0dCL2Ri?=
 =?utf-8?B?RVZQU0UrelhKYzhQWWNFWjV1NjBoQW8xL0N5OWpnVW1ac0hWY0RDb2d3cVpU?=
 =?utf-8?B?cm0zWlBXRWgzblZWMjFUTmlVem5Cb2x1eU9qWXg1S29jOXNUWmNVQk5lQSth?=
 =?utf-8?B?NGhKT1l5KzlqRG1oaC9RbnVydGN5WEZ3emd1TWZCdlkyblVrY0R4bFI4UkpX?=
 =?utf-8?B?NEoxSTBPWVh3NGEyUWNRWkpOSjhWYm9aVHV6NHZFblNUaytxSi9QVjE1MEJF?=
 =?utf-8?B?VFRSOUNKNVp4eDNHaDVZUXQ0OTd0VVZlNTk1aWdKdURXb1NtVC81dC9haXh6?=
 =?utf-8?B?Q0FUK2Q2cTA0ZUdYVUt1Z21xZnZGYVpQalBhalpnbTBUOU52SUVsbE85QU53?=
 =?utf-8?B?cU1jRzZCVThHRVltT05xTk4zcysxcklmZzc0QnR0R2U4aWZVd3VwNzI2akUx?=
 =?utf-8?B?aml2cGNkU25MTnVGOVNrdHN5c1NwcS9tdEE4akx0eThmVExxc0QxOU5zSUFY?=
 =?utf-8?B?bDdObkVBTE1rdE5aRGQ3dkxyNStVdE9aYVFwNnBNSFNRL09Wck5naTdMTHhD?=
 =?utf-8?B?RHM4U1VhZStlc0lUekJnTitKNDNhK2V4cTdETktnbEQvZEM4TEwzdDQrY2Rl?=
 =?utf-8?Q?9aAbRNhCdqWZAEsxI9++GcLJ9olvSwYOcmQCHEJS43Qu?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 1354d40c-a14c-465b-22c8-08dac3dd14b1
X-MS-Exchange-CrossTenant-AuthSource: BN8PR12MB3587.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Nov 2022 12:05:53.0563
 (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: 6vD2w0w8oeIreJaFvaxyG3ZLBbeV1fZptZ9SZXRlqDjHpL+JYy35E7Bwi33Mjav4
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR12MB7693
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 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

Adding Dmitry as well.

Am 11.11.22 um 12:45 schrieb Lukasz Wiecaszek:
> The reason behind that patch is associated with videobuf2 subsystem
> (or more genrally with v4l2 framework) and user created
> dma buffers (udmabuf). In some circumstances
> when dealing with V4L2_MEMORY_DMABUF buffers videobuf2 subsystem
> wants to use dma_buf_vmap() method on the attached dma buffer.
> As udmabuf does not have .vmap operation implemented,
> such dma_buf_vmap() natually fails.
>
> videobuf2_common: [cap-000000003473b2f1] __vb2_queue_alloc: allocated 3 buffers, 1 plane(s) each
> videobuf2_common: [cap-000000003473b2f1] __prepare_dmabuf: buffer for plane 0 changed
> videobuf2_common: [cap-000000003473b2f1] __prepare_dmabuf: failed to map dmabuf for plane 0
> videobuf2_common: [cap-000000003473b2f1] __buf_prepare: buffer preparation failed: -14
>
> The patch itself seems to be strighforward.
> It adds implementation of .vmap method to 'struct dma_buf_ops udmabuf_ops'.
> .vmap method itself uses vm_map_ram() to map pages linearly
> into the kernel virtual address space (only if such mapping
> hasn't been created yet).

Of hand that sounds sane to me.

You should probably mention somewhere in a code comment that the cached 
vaddr is protected by the reservation lock being taken. That's not 
necessary obvious to everybody.

Apart from that looks good to me.

Regards,
Christian.

>
> Signed-off-by: Lukasz Wiecaszek <lukasz.wiecaszek@gmail.com>
> ---
>   drivers/dma-buf/udmabuf.c | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
>
> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
> index 2bcdb935a3ac..8649fcbd05c4 100644
> --- a/drivers/dma-buf/udmabuf.c
> +++ b/drivers/dma-buf/udmabuf.c
> @@ -12,6 +12,7 @@
>   #include <linux/slab.h>
>   #include <linux/udmabuf.h>
>   #include <linux/hugetlb.h>
> +#include <linux/vmalloc.h>
>   
>   static int list_limit = 1024;
>   module_param(list_limit, int, 0644);
> @@ -26,6 +27,7 @@ struct udmabuf {
>   	struct page **pages;
>   	struct sg_table *sg;
>   	struct miscdevice *device;
> +	void *vaddr;
>   };
>   
>   static vm_fault_t udmabuf_vm_fault(struct vm_fault *vmf)
> @@ -57,6 +59,21 @@ static int mmap_udmabuf(struct dma_buf *buf, struct vm_area_struct *vma)
>   	return 0;
>   }
>   
> +static int vmap_udmabuf(struct dma_buf *buf, struct dma_buf_map *map)
> +{
> +	struct udmabuf *ubuf = buf->priv;
> +
> +	if (!ubuf->vaddr) {
> +		ubuf->vaddr = vm_map_ram(ubuf->pages, ubuf->pagecount, -1);
> +		if (!ubuf->vaddr)
> +			return -EINVAL;
> +	}
> +
> +	dma_buf_map_set_vaddr(map, ubuf->vaddr);
> +
> +	return 0;
> +}
> +
>   static struct sg_table *get_sg_table(struct device *dev, struct dma_buf *buf,
>   				     enum dma_data_direction direction)
>   {
> @@ -159,6 +176,7 @@ static const struct dma_buf_ops udmabuf_ops = {
>   	.unmap_dma_buf	   = unmap_udmabuf,
>   	.release	   = release_udmabuf,
>   	.mmap		   = mmap_udmabuf,
> +	.vmap		   = vmap_udmabuf,
>   	.begin_cpu_access  = begin_cpu_udmabuf,
>   	.end_cpu_access    = end_cpu_udmabuf,
>   };