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: <b31fc44a-5827-5213-2a8f-9f0529155542@amd.com>
Date:   Fri, 18 Nov 2022 10:58:28 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Subject: Re: [PATCH v5] udmabuf: add vmap and vunmap methods to udmabuf_ops
Content-Language: en-US
To:     Lukasz Wiecaszek <lukasz.wiecaszek@googlemail.com>,
        Gerd Hoffmann <kraxel@redhat.com>
Cc:     Dmitry Osipenko <dmitry.osipenko@collabora.com>,
        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: <20221117171810.75637-1-lukasz.wiecaszek@gmail.com>
From:   =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>
In-Reply-To: <20221117171810.75637-1-lukasz.wiecaszek@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?R2FxTWM4SERvUE9CT1VmMzVPanoyTWNla2tGbDNXcCs0c0RUMlE1bzdIenFv?=
 =?utf-8?B?Zyt6TjJxaEMxQnYvUUs1SllNUkgrY3VRL3FlRjZ4R09tVk0wa2dVV1hCdVg5?=
 =?utf-8?B?S1g3aUQxcEFFWkFheVBMT1AzcGY0WTA2TmtPeStMWUd2LzJ0VEJzSWFSWGhM?=
 =?utf-8?B?V0w2TW5KSFZna3kwaEUyK1l5M1dndzc3ZzFpcjNtT0h4OUg1dkdyM1BVZmtH?=
 =?utf-8?B?eS9mWFNkYkZ0YzBqTTJPTFdFcEtXcjV5UGkzMi9STUZ5VjI1TVZGVUF4NXll?=
 =?utf-8?B?SXlsMEZQcGZKTVRwbHZmbzhJK2NESk03UVdnWXozQWYyeW91YXZhMFJSdGg3?=
 =?utf-8?B?NmVKa3Y1eTVvekowTzRBYmN0bGVWbmtQR2x6ZFdiWDlYVmZ1am9XU28zOU9u?=
 =?utf-8?B?a283VmVRVnBVRHpKY0FPV24yV0czNG1RQW4zMEJXUHV2VVgvc2dYZ0ZxZ0tU?=
 =?utf-8?B?eUpDUTJzSENBdFI1MHVhZU9YM0gzTUduNHBaNDJPcVJIeFBneXpNY2kyTW1I?=
 =?utf-8?B?QmRxb0xpWUUyV1ZzNStMMDFsVXBIYlVsQ082TlNJZ2xVN3lOcnV5U3FHclJE?=
 =?utf-8?B?NGFwVUsxKzU0VGc1SHJqSHp5T3h6aDc3amo4MkJpWlhCN2MrT0tESGM1bEN4?=
 =?utf-8?B?NHRIWHZWZFhsQTQ3ZG1tQk1CUnU0QXZHanl3eTQ2c3BZZXRycVBFOGFYN1Nv?=
 =?utf-8?B?NG9HZEtxcWhETDRoVnNlK2FMNkVSSjlEcWpQLzNDTW1TWExxTmx4bTRoMzEr?=
 =?utf-8?B?TjZUelVXajdUL1czbk15dmhLQ0VaSXEwYjN3ak54Ly9qSjVSTXpHeGtjVGtv?=
 =?utf-8?B?dHY3VE1ZWnN2bVNRSWQxcWZGbWpOeksvb25tM3d3UTNTeVk0azBnUXVrOERH?=
 =?utf-8?B?ZlBFZHRzK2VNTVlwRTlRVkdCeEpPVnhJR3liY3JkNUhEM2d1QkpnL1BMdDJs?=
 =?utf-8?B?bkxlYVhkRHhSbFZMdTB3TFpkOEtZVTVKYnNiaERTZHNxSEFJaFJHZllsL3Rz?=
 =?utf-8?B?bXRXZWlRQ2ZGRUMrdWpibHdkd3BkNTVRZVZETzdYU3JJamZmYzdvS3hZSkhN?=
 =?utf-8?B?Qi9CQ2MvZlpEdVJqSXVOUXZBbW43MWpldGY0cXg4bGFBSDN4Smo4SFo4elZa?=
 =?utf-8?B?OFdqZnFnRWlGUE1rdUVKcW1kMkRCdjd2L1ZZQ3Rrakl3WE5uYW92dE9sTysw?=
 =?utf-8?B?Z2FQRlMxZ1A1endObU9vU2pUczFEOTBNQTF6TFAwOHJJNzlTS05nTXVqRklo?=
 =?utf-8?B?WmpNUUhVTnZtQlRWQUN4Mk83eVBXalU2Q3R1bFYxMGR4cTBNdnlldmhxNWl2?=
 =?utf-8?B?Q2ZKdzVIYTdoejhjYVdDNVU2NlorQ1VnNTk1QVdrajR0dG92R1h3UlAzb1Zl?=
 =?utf-8?B?VHZ2NDRlUC9mNTNRZHlsQUlIbG9qS1p2Q0FBN1I0MnAvRG1yNmk5R1M5ZUVx?=
 =?utf-8?B?N2ZGTlEwNjBCbVVzVG9GbzlWOXE5ei83RnpNVjBXNUZoUEw1UTFpODdmVU8z?=
 =?utf-8?B?U0YzbGFGWHhkS0J5dXh5VVAxYnFZeDVGNGs4VWN5U3NqNEdjbXZlYmVmLzIv?=
 =?utf-8?B?M09tWlZlVk1NRmVlT1ozWmY0bkk5N09DTHo2M0FIV3lTUFY5UlZ2U1Fxc3FP?=
 =?utf-8?B?V3J4SDBDRWc1ZndFN2lhVkVJMEh4VnZwdjhwYnk5bmNXMG1JSEN1TWNTbnU0?=
 =?utf-8?B?d2g1SUNsQW5Qb2M4TlZKZ25KOXNEY3J1YzJHaEVWRDQvZWpMbG1wMmQvNGtZ?=
 =?utf-8?B?eWJ0Yjdib2VyU2EvcnRPTjhLTjRlb2o5em9tcmREZXBQeExNL1BaVjJDZEdx?=
 =?utf-8?B?Q0Izb094dkIxc3Y4U0RPQndxL1AxL2FxTmpFRG5JRHlZWWdWNkdhd3hjUHdI?=
 =?utf-8?B?M0F0MkxLTFZONlhLa0FrVTZMUlZ5aHdvUU9DdFlYV0ROSk5pa2xiWUVRY2l0?=
 =?utf-8?B?OFQ4aVpLVUV4RDczRDZvekFCSVFTZFE4a1VrdDZtcGxjVXdhYTFSWnhLamlP?=
 =?utf-8?B?TmM3M3dzdml0aEZmSFZKMFp4VjVkQ3lNNlVhbXp4VDViKzZDRnhVTWEvQTJS?=
 =?utf-8?B?Qkh4Ym9lL1l3U0hzQVJ0M1UxcE1qc1pJeE4zd1F0YkMzeDA3K1dtL3ZKYVk0?=
 =?utf-8?B?TDlpM3ZsMkU0MzE5S1A3Q2x2OFV4T3AwTDB4bDFrVjFWT2lOTEZ4bTlldWJk?=
 =?utf-8?Q?6Q8ag+yHZik1EaQpGbnUMrrzEeSYovmgZH7eDjawgQqS?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 97b1c632-0298-484f-fe22-08dac94b73a6
X-MS-Exchange-CrossTenant-AuthSource: BN8PR12MB3587.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Nov 2022 09:58:32.8119
 (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: ishwQu9LVjOPABzrB1BgVbi9yhk0pgISx7RQE8DM5YMsBo8fi0LRFTD/ZQ5h3Wdf
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR12MB5140
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

Pushed this one here to drm-misc-next.

Thanks,
Christian.

Am 17.11.22 um 18:18 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: __vb2_queue_alloc: allocated 3 buffers, 1 plane(s) each
> videobuf2_common: __prepare_dmabuf: buffer for plane 0 changed
> videobuf2_common: __prepare_dmabuf: failed to map dmabuf for plane 0
> videobuf2_common: __buf_prepare: buffer preparation failed: -14
>
> The patch itself seems to be strighforward.
> It adds implementation of .vmap and .vunmap methods
> to 'struct dma_buf_ops udmabuf_ops'.
> .vmap method itself uses vm_map_ram() to map pages linearly
> into the kernel virtual address space.
> .vunmap removes mapping created earlier by .vmap.
> All locking and 'vmapping counting' is done in dma_buf.c
> so it seems to be redundant/unnecessary in .vmap/.vunmap.
>
> Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> Acked-by: Christian König <christian.koenig@amd.com>
> Signed-off-by: Lukasz Wiecaszek <lukasz.wiecaszek@gmail.com>
> ---
> v1: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flinux-media%2F202211120352.G7WPASoP-lkp%40intel.com%2FT%2F%23t&amp;data=05%7C01%7Cchristian.koenig%40amd.com%7C9ef170d657a94849986f08dac8bfeb0f%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638043023852257558%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=yml6LFzivvU1Vv4cHRf47BEeqRN%2BkH1Sy%2FN4h%2BMpRxU%3D&amp;reserved=0
> v2: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flinux-media%2F20221114052944.GA7264%40thinkpad-p72%2FT%2F%23t&amp;data=05%7C01%7Cchristian.koenig%40amd.com%7C9ef170d657a94849986f08dac8bfeb0f%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638043023852257558%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=mBEfWp7w8wTIhO2qF7ad9GrfMyX29EM3neHNHm0i2Zc%3D&amp;reserved=0
> v3: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flinux-media%2F4f92e95f-a0dc-4eac-4c08-0df85de78ae7%40collabora.com%2FT%2F%23t&amp;data=05%7C01%7Cchristian.koenig%40amd.com%7C9ef170d657a94849986f08dac8bfeb0f%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638043023852257558%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=2LTn8lzg48%2B%2BTK4w3vctyQ6PjUva%2BZKS8eZeLG%2FWR7I%3D&amp;reserved=0
> v4: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flinux-media%2F970e798d-ea26-5e1e-ace8-7915a866f7c7%40collabora.com%2FT%2F%23t&amp;data=05%7C01%7Cchristian.koenig%40amd.com%7C9ef170d657a94849986f08dac8bfeb0f%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638043023852257558%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=tO4Tgd5jPeu6%2BNuWSl%2BSqF%2FEh4dI7vaah4%2FICimHvgo%3D&amp;reserved=0
>
> v4 -> v5: Added Acked-by and Reviewed-by to the commit message
> v3 -> v4: Removed line/info 'reported by kernel test robot'
> v2 -> v3: Added .vunmap to 'struct dma_buf_ops udmabuf_ops'
> v1 -> v2: Patch prepared and tested against 6.1.0-rc2+
>
>   drivers/dma-buf/udmabuf.c | 28 ++++++++++++++++++++++++++++
>   1 file changed, 28 insertions(+)
>
> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
> index 283816fbd72f..740d6e426ee9 100644
> --- a/drivers/dma-buf/udmabuf.c
> +++ b/drivers/dma-buf/udmabuf.c
> @@ -13,6 +13,8 @@
>   #include <linux/slab.h>
>   #include <linux/udmabuf.h>
>   #include <linux/hugetlb.h>
> +#include <linux/vmalloc.h>
> +#include <linux/iosys-map.h>
>   
>   static int list_limit = 1024;
>   module_param(list_limit, int, 0644);
> @@ -60,6 +62,30 @@ static int mmap_udmabuf(struct dma_buf *buf, struct vm_area_struct *vma)
>   	return 0;
>   }
>   
> +static int vmap_udmabuf(struct dma_buf *buf, struct iosys_map *map)
> +{
> +	struct udmabuf *ubuf = buf->priv;
> +	void *vaddr;
> +
> +	dma_resv_assert_held(buf->resv);
> +
> +	vaddr = vm_map_ram(ubuf->pages, ubuf->pagecount, -1);
> +	if (!vaddr)
> +		return -EINVAL;
> +
> +	iosys_map_set_vaddr(map, vaddr);
> +	return 0;
> +}
> +
> +static void vunmap_udmabuf(struct dma_buf *buf, struct iosys_map *map)
> +{
> +	struct udmabuf *ubuf = buf->priv;
> +
> +	dma_resv_assert_held(buf->resv);
> +
> +	vm_unmap_ram(map->vaddr, ubuf->pagecount);
> +}
> +
>   static struct sg_table *get_sg_table(struct device *dev, struct dma_buf *buf,
>   				     enum dma_data_direction direction)
>   {
> @@ -162,6 +188,8 @@ static const struct dma_buf_ops udmabuf_ops = {
>   	.unmap_dma_buf	   = unmap_udmabuf,
>   	.release	   = release_udmabuf,
>   	.mmap		   = mmap_udmabuf,
> +	.vmap		   = vmap_udmabuf,
> +	.vunmap		   = vunmap_udmabuf,
>   	.begin_cpu_access  = begin_cpu_udmabuf,
>   	.end_cpu_access    = end_cpu_udmabuf,
>   };