Messages in this thread Patch in this message | | | Date | Thu, 1 Jun 2023 11:47:35 +0200 | Subject | Re: [PATCH 6/8] mediatek: vcodec: separate struct mtk_vcodec_dev | From | AngeloGioacchino Del Regno <> |
| |
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@mediatek.com> > --- > .../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
| |