From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTP id 35194421A1 for ; Thu, 24 Feb 2022 17:49:22 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 640B068B352; Thu, 24 Feb 2022 19:49:20 +0200 (EET) Received: from mail-wm1-f48.google.com (mail-wm1-f48.google.com [209.85.128.48]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 7BFA468B1E6 for ; Thu, 24 Feb 2022 19:49:13 +0200 (EET) Received: by mail-wm1-f48.google.com with SMTP id az26-20020a05600c601a00b0037c078db59cso250090wmb.4 for ; Thu, 24 Feb 2022 09:49:13 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to; bh=B03g/WiltiofJs4YzDYMtS/V+5NQ6f2fXhsKgDryjh0=; b=PR50I1pHraIUW1oh7gOeZxta3ko13Z8yWgq0DEWcBdTyD6QNrUpisDzwmfvBLNXLEJ 4hDrrG59Gf27lLq9dbXb/rw7eTBDXk6G/vtW3JsLuNw+qqeeCrIw7kEl8SBT3NwH2amF U0LItgwBPiULdcu3Oc1oYE/wAKT1C99Asn0YqJk3h2a7N1i019a9e/2bFFfjmlkWb4gK 58OnS3fcdMS7h5Kzcoj4cYx2qQStzWHDlBeS1FENPRa9TD5OzVS/IviTnWRh+5nQkf09 PYuNbhjMwDnedlw9p2GJd0y5aqzhkISu3dPOmXnaYcs/x2sNnd5ZFLANOGO3Px/zb4Fc 3eiQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to; bh=B03g/WiltiofJs4YzDYMtS/V+5NQ6f2fXhsKgDryjh0=; b=445mSTTL/cRYoBuV/5bxLPggNf12hzinSJjG8t01VX0Fp58W99dYcl1RmpLyRmLorv qTuXd/UMVNYCVaO7Jf/DGIAZK0LtHVCjHgCJrq+ST+8O3kq6yPaj7e/QR2kMcYnTWXpR vcWObV3wrhHuYmgxj3qbHlWKjyZj0ZLl9ALyWPOaYSmoUUZ4IuRueQ7q01KrH+o0xID6 p2hpG5ICdNj4D63ArgLDR9KcqI+Sf7d9M1ZOTpxbU0wF+hUEusldQonxdJ2WY8rsWtcl 7bU/mPUfSGezp0UT5BpMtUtlA4rfdQMskQUGK4qPxyylw+oAiQPUfBLk+yJ6W96a4QMP bqhg== X-Gm-Message-State: AOAM533oM9Ygj9Xy2YR5aOT7vAUSCT6+JirK5d8Zih2EUJE45tmUxYof yx9kFqTZGFyFlRlWsgsnQt0tWbFp2qT1TX2L0kFoQey/NGM= X-Google-Smtp-Source: ABdhPJxk8zBv9z7dyJF7Qc7HJXME2U86K6nh9H50flBHBJvB3pv8ZKA+Fa4mfJ+/q8HeaKsIFmGNX2/0ZBKOxOf0GyA= X-Received: by 2002:a1c:4603:0:b0:381:19fe:280b with SMTP id t3-20020a1c4603000000b0038119fe280bmr2512222wma.67.1645724952250; Thu, 24 Feb 2022 09:49:12 -0800 (PST) MIME-Version: 1.0 References: <20210311220921.28899-1-suji.velupillai@broadcom.com> <9f30ff8b-4117-4c73-da05-f9acbcccad79@jkqxz.net> In-Reply-To: From: Dennis Mungai Date: Thu, 24 Feb 2022 20:48:35 +0300 Message-ID: To: FFmpeg development discussions and patches X-Content-Filtered-By: Mailman/MimeDel 2.1.29 Subject: Re: [FFmpeg-devel] [PATCH] lavu: add VKAPI hwcontext implementation X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: On Wed, 17 Mar 2021 at 00:14, Suji Velupillai wrote: > Thank you Mark for your feedback. Please see inline > > On Fri, Mar 12, 2021 at 1:14 PM Mark Thompson wrote: > > > On 11/03/2021 22:09, suji.velupillai@broadcom.com wrote: > > > From: Suji Velupillai > > > > > > Initial commit to add VKAPI hardware accelerator implementation. > > > The depedency component vkil source code can be obtained from github > > > https://github.com/Broadcom/vkil > > > > > > Signed-off-by: Suji Velupillai > > > --- > > > configure | 8 +- > > > doc/APIchanges | 4 + > > > libavutil/Makefile | 2 + > > > libavutil/hwcontext.c | 4 + > > > libavutil/hwcontext.h | 1 + > > > libavutil/hwcontext_internal.h | 1 + > > > libavutil/hwcontext_vkapi.c | 522 > +++++++++++++++++++++++++++++++++ > > > libavutil/hwcontext_vkapi.h | 104 +++++++ > > > libavutil/pixdesc.c | 4 + > > > libavutil/pixfmt.h | 6 + > > > 10 files changed, 655 insertions(+), 1 deletion(-) > > > create mode 100644 libavutil/hwcontext_vkapi.c > > > create mode 100644 libavutil/hwcontext_vkapi.h > > > > Where has the "vkapi" name come from? It seems to be consistently called > > "vkil" in that repository. > > valkyrie is the hardware name. vkil refers to VK Interface Layer. vkapi is > the name given to all ffmpeg API's. > > > > > If you are making up the name for this, please consider making it less > > confusing: > > * The standard Vulkan API already effectively owns the "vk" namespace > > prefix, and colliding with that in a related project is unhelpful. > > * Indeed, Vulkan already uses the "VKAPI" name in its headers when > > marking ABIs (see < > > > https://github.com/ARM-software/vulkan-sdk/blob/master/include/vulkan/vk_platform.h#L35 > > >). > > * Current search results for "vkapi" show it is also used by APIs for the > > VK social network. > > > > Okay I will rename all with VKAPI and VK_* reference to avoid > conflicts/confusion. > Kernel driver code consistently used "bcm_vk", also Google search points > correctly to the kernel driver for this card. > Would it be okay to switch to bcm_vk instead of vkapi and vk? > > > > > > > ... > diff --git a/libavutil/hwcontext_vkapi.h > > b/libavutil/hwcontext_vkapi.h > > > new file mode 100644 > > > index 0000000000..096602b42e > > > --- /dev/null > > > +++ b/libavutil/hwcontext_vkapi.h > > > @@ -0,0 +1,104 @@ > > > +/* > > > + * Copyright (c) 2018 Broadcom > > > + * > > > + * This file is part of FFmpeg. > > > + * > > > + * FFmpeg is free software; you can redistribute it and/or > > > + * modify it under the terms of the GNU Lesser General Public > > > + * License as published by the Free Software Foundation; either > > > + * version 2.1 of the License, or (at your option) any later version. > > > + * > > > + * FFmpeg is distributed in the hope that it will be useful, > > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > > > + * Lesser General Public License for more details. > > > + * > > > + * You should have received a copy of the GNU Lesser General Public > > > + * License along with FFmpeg; if not, write to the Free Software > > > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA > > 02110-1301 USA > > > + */ > > > + > > > +#ifndef AVUTIL_HWCONTEXT_VKAPI_H > > > +#define AVUTIL_HWCONTEXT_VKAPI_H > > > + > > > +#include > > > + > > > +#define VKAPI_METADATA_PLANE 4 > > > + > > > +/** > > > + * @file > > > + * API-specific header for AV_HWDEVICE_TYPE_VKAPI. > > > + */ > > > + > > > +/** > > > + * Allocated as AVHWDeviceContext.hwctx > > > + */ > > > +typedef struct VKAPIDeviceContext { > > > + /** > > > + * Holds pointers to hardware specific functions > > > + */ > > > + vkil_api *ilapi; > > > + /** > > > + * Internal functions definitions > > > + */ > > > + /** > > > + * Get the hwprops reference from the AVFrame:data[3] > > > + */ > > > + int (*frame_ref_hwprops)(const AVFrame *frame, void > > *phw_surface_desc); > > > + /** > > > + * Set the hwprops into AVFrame:data[3] > > > + */ > > > + int (*frame_set_hwprops)(AVFrame *frame, const vkil_buffer_surface > > *hw_surface_desc); > > > + /** > > > + * Get the hwprops from AVFrame:data[3] > > > + */ > > > + int (*frame_get_hwprops)(const AVFrame *frame, vkil_buffer_surface > > *hw_surface_desc); > > > + /** > > > + * Check if format is in an array > > > + */ > > > + int (*fmt_is_in)(int fmt, const int *fmts); > > > + /** > > > + * Convert AVPixelFormat to VKAPI equivalent pixel format > > > + */ > > > + int (*av2vk_fmt)(enum AVPixelFormat pixel_format); > > > + /** > > > + * Get no of buffer count reference in the hardware pool > > > + */ > > > + int (*get_pool_occupancy)(AVHWFramesContext *ctx); > > > +} VKAPIDeviceContext; > > > > This structure has two uses: > > > > * To be filled by the user when they already have an instance of the > > device and want to use it with libav*. > > * To provide information to the user about an instance of the device > > created inside libav*. > > > > To that end, the "ilapi" field makes sense to me (the user provides their > > vkil API handle), but they also need to provide some sort of reference to > > the actual device (a vkil_context handle, maybe?). > > > > I don't think any of the other fields make sense; it looks like you are > > trying to expose some internals in a confusing way - why would a user > > creating this structure want to set those function pointers? > > > > Agreed, Lynne also pointed this out, so I removed it and moved it within > the library that really needs the functions. > > > > > +/** > > > + * Contains color information for hardware > > > + */ > > > +typedef struct VKAPIColorContext { > > > + enum AVColorRange range; > > > + enum AVColorPrimaries primaries; > > > + enum AVColorTransferCharacteristic trc; > > > + enum AVColorSpace space; > > > + enum AVChromaLocation chroma_location; > > > +} VKAPIColorContext; > > > + > > > +/** > > > + * Allocated as AVHWFramesContext.hwctx > > > + */ > > > +typedef struct VKAPIFramesContext { > > > + /** > > > + * Handle to a hardware frame context > > > + */ > > > + uint32_t handle; > > > + /** > > > + * Hardware component port associated to the frame context > > > + */ > > > + uint32_t port_id; > > > + uint32_t extra_port_id; > > > + /** > > > + * Color information > > > + */ > > > + VKAPIColorContext color; > > > + /** > > > + * ilcontext associated to the frame context > > > + */ > > > + vkil_context *ilctx; > > > +} VKAPIFramesContext; > > > + > > > +#endif /* AVUTIL_HWCONTEXT_VKAPI_H */ > > > > The vkil_context looks like it should be part of the device. > > > > So sort of handle information for the context of the frame makes sense, > ok. > > > > The port_id fields don't seem to be used at all in your implementation, > > which strongly suggests that they should not be here. > > > > The colour information you are including here is generally represented > > per-frame, so attaching it to a frame context seems dubious. > > > Both of those fields are used in the libavcodec, but it makes sense to be > within the codecs struct itself. I'll review the code and change it. > > > > > diff --git a/libavutil/pixfmt.h b/libavutil/pixfmt.h > > > index 46ef211add..3ae607a3d6 100644 > > > --- a/libavutil/pixfmt.h > > > +++ b/libavutil/pixfmt.h > > > @@ -348,6 +348,12 @@ enum AVPixelFormat { > > > AV_PIX_FMT_NV24, ///< planar YUV 4:4:4, 24bpp, 1 plane for Y > > and 1 plane for the UV components, which are interleaved (first byte U > and > > the following byte V) > > > AV_PIX_FMT_NV42, ///< as above, but U and V bytes are > swapped > > > > > > + /** > > > + * VKAPI hardware acceleration. > > > + * data[3] contains a pointer to the vkil_buffer_surface structure > > > + */ > > > + AV_PIX_FMT_VKAPI, > > > > As already noted, please use data[0] (data[3] is unhelpfully baked into > > some older formats for historical reasons, apologies for any confusion > > there). > > > > No problem. Thank you for clarification, agreed, I'll change it to > data[0]. > > Also, you seem to have references to an additional field store in data[4], > > the meaning of which needs to be documented here as well. > > > Okay > > > > > > > To understand what is going on with this hwcontext it would be very > > helpful to see some components using it (even in prototype form). > > > > We have working encoder/decoder and scaler functionalities for this > hardware in ffmpeg framework, which I will be sending it for review. I > thought to get the hwcontext in first for feedback. It needs some clean up > and changes in code based on this patch review also. I will do that as soon > as possible. > > > > > > Thanks, > > > > - Mark > > Hello there, Any updates on this? Warm regards, Dennis. _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".