On Wed, 17 Mar 2021 at 00:14, Suji Velupillai <suji.velupil...@broadcom.com> wrote:
> Thank you Mark for your feedback. Please see inline > > On Fri, Mar 12, 2021 at 1:14 PM Mark Thompson <s...@jkqxz.net> wrote: > > > On 11/03/2021 22:09, suji.velupil...@broadcom.com wrote: > > > From: Suji Velupillai <suji.velupil...@broadcom.com> > > > > > > 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 <suji.velupil...@broadcom.com> > > > --- > > > 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 <vkil_api.h> > > > + > > > +#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-requ...@ffmpeg.org with subject "unsubscribe".
