Hi AngeloGioacchino,
Thanks for your suggestion.
Whether we need to fix the code for these patches just copy from the
source file mtk_vcodec_drv.h.
Best Regards,
Yunfei Dong
On Thu, 2023-06-01 at 11:47 +0200, AngeloGioacchino Del Regno wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> Il 01/06/23 05:02, Yunfei Dong ha scritto:
> > Adding different dev struct for encoder and decoder, remove
> > 'struct mtk_vcodec_dev'.
> >
> > Signed-off-by: Yunfei Dong <yunfei.dong@xxxxxxxxxxxx>
> > ---
> > .../mediatek/vcodec/mtk_vcodec_com_drv.h | 71 +++++
> > .../mediatek/vcodec/mtk_vcodec_dbgfs.c | 51 ++-
> > .../mediatek/vcodec/mtk_vcodec_dbgfs.h | 14 +-
> > .../platform/mediatek/vcodec/mtk_vcodec_dec.c | 4 +-
> > .../mediatek/vcodec/mtk_vcodec_dec_drv.c | 19 +-
> > .../mediatek/vcodec/mtk_vcodec_dec_drv.h | 168 +++++++++-
> > .../mediatek/vcodec/mtk_vcodec_dec_hw.c | 5 +-
> > .../mediatek/vcodec/mtk_vcodec_dec_hw.h | 4 +-
> > .../mediatek/vcodec/mtk_vcodec_dec_pm.c | 10 +-
> > .../mediatek/vcodec/mtk_vcodec_dec_pm.h | 2 +-
> > .../mediatek/vcodec/mtk_vcodec_dec_stateful.c | 3 +-
> > .../vcodec/mtk_vcodec_dec_stateless.c | 5 +-
> > .../platform/mediatek/vcodec/mtk_vcodec_drv.h | 295 -----------
> -------
> > .../platform/mediatek/vcodec/mtk_vcodec_enc.c | 5 +-
> > .../mediatek/vcodec/mtk_vcodec_enc_drv.c | 15 +-
> > .../mediatek/vcodec/mtk_vcodec_enc_drv.h | 89 +++++-
> > .../mediatek/vcodec/mtk_vcodec_enc_pm.c | 4 +-
> > .../mediatek/vcodec/mtk_vcodec_enc_pm.h | 4 +-
> > .../platform/mediatek/vcodec/mtk_vcodec_fw.c | 4 +-
> > .../platform/mediatek/vcodec/mtk_vcodec_fw.h | 3 +-
> > .../mediatek/vcodec/mtk_vcodec_fw_priv.h | 3 +-
> > .../mediatek/vcodec/mtk_vcodec_fw_scp.c | 11 +-
> > .../mediatek/vcodec/mtk_vcodec_fw_vpu.c | 16 +-
> > .../mediatek/vcodec/mtk_vcodec_intr.c | 4 +-
> > .../mediatek/vcodec/mtk_vcodec_util.c | 10 +-
> > .../mediatek/vcodec/mtk_vcodec_util.h | 8 +-
> > .../vcodec/vdec/vdec_h264_req_common.h | 2 +-
> > .../vcodec/vdec/vdec_h264_req_multi_if.c | 2 +-
> > .../platform/mediatek/vcodec/vdec_drv_if.h | 1 -
> > .../platform/mediatek/vcodec/vdec_msg_queue.c | 4 +-
> > .../platform/mediatek/vcodec/vdec_msg_queue.h | 2 +-
> > .../platform/mediatek/vcodec/vdec_vpu_if.c | 3 +-
> > .../mediatek/vcodec/venc/venc_h264_if.c | 2 +-
> > .../mediatek/vcodec/venc/venc_vp8_if.c | 2 +-
> > .../platform/mediatek/vcodec/venc_drv_base.h | 2 +-
> > .../platform/mediatek/vcodec/venc_drv_if.c | 4 +-
> > .../platform/mediatek/vcodec/venc_drv_if.h | 3 +-
> > .../platform/mediatek/vcodec/venc_vpu_if.c | 2 +-
> > 38 files changed, 441 insertions(+), 415 deletions(-)
> > delete mode 100644
> drivers/media/platform/mediatek/vcodec/mtk_vcodec_drv.h
> >
>
> ..snip..
>
> > diff --git
> a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_drv.h
> b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_drv.h
> > index 94870b651919..b664d0733466 100644
> > --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_drv.h
> > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_drv.h
> > @@ -8,9 +8,49 @@
> > #define _MTK_VCODEC_DEC_DRV_H_
> >
> > #include "mtk_vcodec_com_drv.h"
> > +#include "mtk_vcodec_dbgfs.h"
> > #include "mtk_vcodec_fw_priv.h"
> > +#include "mtk_vcodec_util.h"
> > #include "vdec_msg_queue.h"
> >
> > +#define MTK_VCODEC_DEC_NAME"mtk-vcodec-dec"
> > +
> > +#define IS_VDEC_LAT_ARCH(hw_arch) ((hw_arch) >=
> MTK_VDEC_LAT_SINGLE_CORE)
> > +#define IS_VDEC_INNER_RACING(capability) ((capability) &
> MTK_VCODEC_INNER_RACING)
> > +
> > +/*
> > + * struct mtk_vdec_format_types - Structure used to get supported
>
> This is not a struct, it's an enumeration..... please fix the typo
>
> > + * format types according to decoder capability
> > + */
> > +enum mtk_vdec_format_types {
> > +MTK_VDEC_FORMAT_MM21 = 0x20,
> > +MTK_VDEC_FORMAT_MT21C = 0x40,
> > +MTK_VDEC_FORMAT_H264_SLICE = 0x100,
> > +MTK_VDEC_FORMAT_VP8_FRAME = 0x200,
> > +MTK_VDEC_FORMAT_VP9_FRAME = 0x400,
> > +MTK_VDEC_FORMAT_AV1_FRAME = 0x800,
> > +MTK_VDEC_FORMAT_HEVC_FRAME = 0x1000,
> > +MTK_VCODEC_INNER_RACING = 0x20000,
> > +};
> > +
> > +/*
> > + * enum mtk_vdec_hw_count - Supported hardware count
> > + */
> > +enum mtk_vdec_hw_count {
> > +MTK_VDEC_NO_HW = 0,
> > +MTK_VDEC_ONE_CORE,
> > +MTK_VDEC_ONE_LAT_ONE_CORE,
> > +MTK_VDEC_MAX_HW_COUNT,
> > +};
> > +
> > +/*
> > + * enum mtk_vdec_hw_arch - Used to separate different hardware
> architecture
> > + */
> > +enum mtk_vdec_hw_arch {
> > +MTK_VDEC_PURE_SINGLE_CORE,
> > +MTK_VDEC_LAT_SINGLE_CORE,
> > +};
> > +
> > /**
> > * struct vdec_pic_info - picture size information
> > * @pic_w: picture width
> > @@ -33,16 +73,46 @@ struct vdec_pic_info {
> > unsigned int reserved;
> > };
> >
> > -/*
> > - * enum mtk_vdec_hw_id - Hardware index used to separate
> > - * different hardware
> > +/**
> > + * struct mtk_vcodec_dec_pdata - compatible data for each IC
> > + * @init_vdec_params: init vdec params
> > + * @ctrls_setup: init vcodec dec ctrls
> > + * @worker: worker to start a decode job
> > + * @flush_decoder: function that flushes the decoder
> > + * @get_cap_buffer: get capture buffer from capture queue
> > + * @cap_to_disp: put capture buffer to disp list for lat and core
> arch
> > + * @vdec_vb2_ops: struct vb2_ops
> > + *
> > + * @vdec_formats: supported video decoder formats
> > + * @num_formats: count of video decoder formats
> > + * @default_out_fmt: default output buffer format
> > + * @default_cap_fmt: default capture buffer format
> > + *
> > + * @hw_arch: hardware arch is used to separate pure_sin_core and
> lat_sin_core
> > + *
> > + * @is_subdev_supported: whether support parent-node
> architecture(subdev)
> > + * @uses_stateless_api: whether the decoder uses the stateless API
> with requests
> > */
> > -enum mtk_vdec_hw_id {
> > -MTK_VDEC_CORE,
> > -MTK_VDEC_LAT0,
> > -MTK_VDEC_LAT1,
> > -MTK_VDEC_LAT_SOC,
> > -MTK_VDEC_HW_MAX,
> > +struct mtk_vcodec_dec_pdata {
> > +void (*init_vdec_params)(struct mtk_vcodec_dec_ctx *ctx);
> > +int (*ctrls_setup)(struct mtk_vcodec_dec_ctx *ctx);
> > +void (*worker)(struct work_struct *work);
> > +int (*flush_decoder)(struct mtk_vcodec_dec_ctx *ctx);
> > +struct vdec_fb *(*get_cap_buffer)(struct mtk_vcodec_dec_ctx *ctx);
> > +void (*cap_to_disp)(struct mtk_vcodec_dec_ctx *ctx, int error,
> > + struct media_request *src_buf_req);
> > +
> > +struct vb2_ops *vdec_vb2_ops;
> > +
> > +const struct mtk_video_fmt *vdec_formats;
> > +const int *num_formats;
> > +const struct mtk_video_fmt *default_out_fmt;
> > +const struct mtk_video_fmt *default_cap_fmt;
> > +
> > +enum mtk_vdec_hw_arch hw_arch;
> > +
> > +bool is_subdev_supported;
> > +bool uses_stateless_api;
> > };
> >
> > /**
> > @@ -95,7 +165,7 @@ enum mtk_vdec_hw_id {
> > */
> > struct mtk_vcodec_dec_ctx {
> > enum mtk_instance_type type;
> > -struct mtk_vcodec_dev *dev;
> > +struct mtk_vcodec_dec_dev *dev;
> > struct list_head list;
> >
> > struct v4l2_fh fh;
> > @@ -136,6 +206,84 @@ struct mtk_vcodec_dec_ctx {
> > struct vdec_msg_queue msg_queue;
> > };
> >
> > +/**
> > + * struct mtk_vcodec_dec_dev - driver data
> > + * @v4l2_dev: V4L2 device to register video devices for.
> > + * @vfd_dec: Video device for decoder
> > + * @mdev_dec: Media device for decoder
> > + *
> > + * @m2m_dev_dec: m2m device for decoder
> > + * @plat_dev: platform device
> > + * @ctx_list: list of struct mtk_vcodec_ctx
> > + * @curr_ctx: The context that is waiting for codec hardware
> > + *
> > + * @reg_base: Mapped address of MTK Vcodec registers.
> > + * @vdec_pdata: decoder IC-specific data
> > + *
> > + * @fw_handler: used to communicate with the firmware.
> > + * @id_counter: used to identify current opened instance
> > + *
> > + * @dec_mutex: decoder hardware lock
> > + * @dev_mutex: video_device lock
> > + * @decode_workqueue: decode work queue
> > + *
> > + * @irqlock: protect data access by irq handler and work thread
> > + * @dec_irq: decoder irq resource
> > + *
> > + * @pm: power management control
> > + * @dec_capability: used to identify decode capability, ex: 4k
> > + *
> > + * @core_workqueue: queue used for core hardware decode
> > + *
> > + * @subdev_dev: subdev hardware device
> > + * @subdev_prob_done: check whether all used hw device is prob
> done
> > + * @subdev_bitmap: used to record hardware is ready or not
> > + *
> > + * @dec_active_cnt: used to mark whether need to record register
> value
> > + * @vdec_racing_info: record register value
> > + * @dec_racing_info_mutex: mutex lock used for inner racing mode
> > + * @dbgfs: debug log related information
> > + */
> > +struct mtk_vcodec_dec_dev {
> > +struct v4l2_device v4l2_dev;
> > +struct video_device *vfd_dec;
> > +struct media_device mdev_dec;
> > +
> > +struct v4l2_m2m_dev *m2m_dev_dec;
> > +struct platform_device *plat_dev;
> > +struct list_head ctx_list;
> > +struct mtk_vcodec_dec_ctx *curr_ctx;
> > +
> > +void __iomem *reg_base[NUM_MAX_VCODEC_REG_BASE];
> > +const struct mtk_vcodec_dec_pdata *vdec_pdata;
> > +
> > +struct mtk_vcodec_fw *fw_handler;
> > +unsigned long id_counter;
>
> Does id_counter need to be 64 bits or 32 bits?
>
> Please change that to a fixed size type, so that we also increase the
> reliability of this driver :-)
>
> > +
> > +/* decoder hardware mutex lock */
> > +struct mutex dec_mutex[MTK_VDEC_HW_MAX];
> > +struct mutex dev_mutex;
> > +struct workqueue_struct *decode_workqueue;
> > +
> > +spinlock_t irqlock;
> > +int dec_irq;
> > +
> > +struct mtk_vcodec_pm pm;
> > +unsigned int dec_capability;
> > +
> > +struct workqueue_struct *core_workqueue;
> > +
> > +void *subdev_dev[MTK_VDEC_HW_MAX];
> > +int (*subdev_prob_done)(struct mtk_vcodec_dec_dev *vdec_dev);
> > +DECLARE_BITMAP(subdev_bitmap, MTK_VDEC_HW_MAX);
> > +
> > +atomic_t dec_active_cnt;
> > +u32 vdec_racing_info[132];
> > +/* Protects access to vdec_racing_info data */
> > +struct mutex dec_racing_info_mutex;
> > +struct mtk_vcodec_dbgfs dbgfs;
> > +};
> > +
> > static inline struct mtk_vcodec_dec_ctx *fh_to_dec_ctx(struct
> v4l2_fh *fh)
> > {
> > return container_of(fh, struct mtk_vcodec_dec_ctx, fh);
>
> ..snip..
>
> > diff --git
> a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_enc_drv.h
> b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_enc_drv.h
> > index 6aa4afe5b796..0dc1957b147e 100644
> > --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_enc_drv.h
> > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_enc_drv.h
> > @@ -8,7 +8,39 @@
> > #define _MTK_VCODEC_ENC_DRV_H_
> >
> > #include "mtk_vcodec_com_drv.h"
> > +#include "mtk_vcodec_dbgfs.h"
> > #include "mtk_vcodec_fw_priv.h"
> > +#include "mtk_vcodec_util.h"
> > +
> > +#define MTK_VCODEC_ENC_NAME"mtk-vcodec-enc"
> > +
> > +#define MTK_ENC_CTX_IS_EXT(ctx) ((ctx)->dev->venc_pdata->uses_ext)
> > +#define MTK_ENC_IOVA_IS_34BIT(ctx) ((ctx)->dev->venc_pdata-
> >uses_34bit)
> > +
> > +/**
> > + * struct mtk_vcodec_enc_pdata - compatible data for each IC
> > + *
> > + * @uses_ext: whether the encoder uses the extended firmware
> messaging format
> > + * @min_bitrate: minimum supported encoding bitrate
> > + * @max_bitrate: maximum supported encoding bitrate
> > + * @capture_formats: array of supported capture formats
> > + * @num_capture_formats: number of entries in capture_formats
> > + * @output_formats: array of supported output formats
> > + * @num_output_formats: number of entries in output_formats
> > + * @core_id: stand for h264 or vp8 encode index
> > + * @uses_34bit: whether the encoder uses 34-bit iova
> > + */
> > +struct mtk_vcodec_enc_pdata {
> > +bool uses_ext;
> > +unsigned long min_bitrate;
> > +unsigned long max_bitrate;
>
> Fixed size types for min/max bitrate please (u64?)
>
> > +const struct mtk_video_fmt *capture_formats;
> > +size_t num_capture_formats;
> > +const struct mtk_video_fmt *output_formats;
> > +size_t num_output_formats;
> > +int core_id;
>
> Besides, I think that core_id can be u8 as we'll never see more than
> 255 cores :-)
>
> > +bool uses_34bit;
> > +};
> >
> > /*
> > * enum mtk_encode_param - General encoding parameters type
> > @@ -101,7 +133,7 @@ struct mtk_enc_params {
> > */
> > struct mtk_vcodec_enc_ctx {
> > enum mtk_instance_type type;
> > -struct mtk_vcodec_dev *dev;
> > +struct mtk_vcodec_enc_dev *dev;
> > struct list_head list;
> >
> > struct v4l2_fh fh;
> > @@ -136,6 +168,61 @@ struct mtk_vcodec_enc_ctx {
> > struct mutex q_mutex;
> > };
> >
> > +/**
> > + * struct mtk_vcodec_enc_dev - driver data
> > + * @v4l2_dev: V4L2 device to register video devices for.
> > + * @vfd_enc: Video device for encoder.
> > + *
> > + * @m2m_dev_enc: m2m device for encoder.
> > + * @plat_dev: platform device
> > + * @ctx_list: list of struct mtk_vcodec_ctx
> > + * @curr_ctx: The context that is waiting for codec hardware
> > + *
> > + * @reg_base: Mapped address of MTK Vcodec registers.
> > + * @venc_pdata: encoder IC-specific data
> > + *
> > + * @fw_handler: used to communicate with the firmware.
> > + * @id_counter: used to identify current opened instance
> > + *
> > + * @enc_mutex: encoder hardware lock.
> > + * @dev_mutex: video_device lock
> > + * @encode_workqueue: encode work queue
> > + *
> > + * @enc_irq: h264 encoder irq resource
> > + * @irqlock: protect data access by irq handler and work thread
> > + *
> > + * @pm: power management control
> > + * @enc_capability: used to identify encode capability
> > + * @dbgfs: debug log related information
> > + */
> > +struct mtk_vcodec_enc_dev {
> > +struct v4l2_device v4l2_dev;
> > +struct video_device *vfd_enc;
> > +
> > +struct v4l2_m2m_dev *m2m_dev_enc;
> > +struct platform_device *plat_dev;
> > +struct list_head ctx_list;
> > +struct mtk_vcodec_enc_ctx *curr_ctx;
> > +
> > +void __iomem *reg_base[NUM_MAX_VCODEC_REG_BASE];
> > +const struct mtk_vcodec_enc_pdata *venc_pdata;
> > +
> > +struct mtk_vcodec_fw *fw_handler;
> > +unsigned long id_counter;
>
> u64 id_counter ?
>
> > +
> > +/* encoder hardware mutex lock */
> > +struct mutex enc_mutex;
> > +struct mutex dev_mutex;
> > +struct workqueue_struct *encode_workqueue;
> > +
> > +int enc_irq;
> > +spinlock_t irqlock;
> > +
> > +struct mtk_vcodec_pm pm;
> > +unsigned int enc_capability;
> > +struct mtk_vcodec_dbgfs dbgfs;
> > +};
> > +
> > static inline struct mtk_vcodec_enc_ctx *fh_to_enc_ctx(struct
> v4l2_fh *fh)
> > {
> > return container_of(fh, struct mtk_vcodec_enc_ctx, fh);
>
> ..snip..
>
> > diff --git
> a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_fw_scp.c
> b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_fw_scp.c
> > index 9a2472442c6f..eda63f4262a9 100644
> > --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_fw_scp.c
> > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_fw_scp.c
> > @@ -1,8 +1,8 @@
> > // SPDX-License-Identifier: GPL-2.0
> >
> > +#include "mtk_vcodec_dec_drv.h"
> > +#include "mtk_vcodec_enc_drv.h"
> > #include "mtk_vcodec_fw_priv.h"
> > -#include "mtk_vcodec_util.h"
> > -#include "mtk_vcodec_drv.h"
> >
> > static int mtk_vcodec_scp_load_firmware(struct mtk_vcodec_fw *fw)
> > {
> > @@ -56,11 +56,14 @@ static const struct mtk_vcodec_fw_ops
> mtk_vcodec_rproc_msg = {
> > struct mtk_vcodec_fw *mtk_vcodec_fw_scp_init(void *priv, enum
> mtk_vcodec_fw_use fw_use)
> > {
> > struct mtk_vcodec_fw *fw;
> > -struct mtk_vcodec_dev *dev = priv;
> > struct platform_device *plat_dev;
> > struct mtk_scp *scp;
> >
> > -plat_dev = dev->plat_dev;
> > +if (fw_use == DECODER)
> > +plat_dev = ((struct mtk_vcodec_dec_dev *)priv)->plat_dev;
>
> Hmm.. I don't really like this one's readability.
> Perhaps we can do...
>
> if (fw_use == DECODER) {
> struct mtk_vcodec_dec_dev *dev = priv;
> plat_dev = priv->plat_dev;
> break;
> } else if (fw_use == ENCODER) {
> struct mtk_vcodec_enc_dev *dev = priv;
> plat_dev = priv->plat_dev;
> break;
> } else {
> pr_err("Invalid fw_use %d (use a better message here)", fw_use);
> return PTR_ERR(-EINVAL);
> }
>
>
> > +else
> > +plat_dev = ((struct mtk_vcodec_enc_dev *)priv)->plat_dev;
> > +
> > scp = scp_get(plat_dev);
> > if (!scp) {
> > mtk_v4l2_err("could not get vdec scp handle");
> > diff --git
> a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_fw_vpu.c
> b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_fw_vpu.c
> > index 65562a36788f..52720a93b56a 100644
> > --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_fw_vpu.c
> > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_fw_vpu.c
> > @@ -1,8 +1,8 @@
> > // SPDX-License-Identifier: GPL-2.0
> >
> > +#include "mtk_vcodec_dec_drv.h"
> > +#include "mtk_vcodec_enc_drv.h"
> > #include "mtk_vcodec_fw_priv.h"
> > -#include "mtk_vcodec_util.h"
> > -#include "mtk_vcodec_drv.h"
> >
> > static int mtk_vcodec_vpu_load_firmware(struct mtk_vcodec_fw *fw)
> > {
> > @@ -53,7 +53,7 @@ static void mtk_vcodec_vpu_release(struct
> mtk_vcodec_fw *fw)
> >
> > static void mtk_vcodec_vpu_reset_dec_handler(void *priv)
> > {
> > -struct mtk_vcodec_dev *dev = priv;
> > +struct mtk_vcodec_dec_dev *dev = priv;
> > struct mtk_vcodec_dec_ctx *ctx;
> >
> > mtk_v4l2_err("Watchdog timeout!!");
> > @@ -69,7 +69,7 @@ static void mtk_vcodec_vpu_reset_dec_handler(void
> *priv)
> >
> > static void mtk_vcodec_vpu_reset_enc_handler(void *priv)
> > {
> > -struct mtk_vcodec_dev *dev = priv;
> > +struct mtk_vcodec_enc_dev *dev = priv;
> > struct mtk_vcodec_enc_ctx *ctx;
> >
> > mtk_v4l2_err("Watchdog timeout!!");
> > @@ -96,7 +96,6 @@ static const struct mtk_vcodec_fw_ops
> mtk_vcodec_vpu_msg = {
> > struct mtk_vcodec_fw *mtk_vcodec_fw_vpu_init(void *priv, enum
> mtk_vcodec_fw_use fw_use)
> > {
> > struct platform_device *fw_pdev;
> > -struct mtk_vcodec_dev *dev = priv;
> > struct platform_device *plat_dev;
> > struct mtk_vcodec_fw *fw;
> > enum rst_id rst_id;
> > @@ -104,14 +103,15 @@ struct mtk_vcodec_fw
> *mtk_vcodec_fw_vpu_init(void *priv, enum mtk_vcodec_fw_use
> > switch (fw_use) {
> > case ENCODER:
> > rst_id = VPU_RST_ENC;
> > +plat_dev = ((struct mtk_vcodec_enc_dev *)priv)->plat_dev;
>
> same comment from before.. :-)
>
> Regards,
> Angelo
>
