Add work queue to process core hardware information.
First, get lat_buf from message queue, then call core
hardware of each codec(H264/VP9/AV1) to decode, finally
puts lat_buf back to the message.
Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com>
---
.../platform/mtk-vcodec/mtk_vcodec_dec_drv.c | 16 +++++++-
.../platform/mtk-vcodec/mtk_vcodec_drv.h | 3 ++
.../platform/mtk-vcodec/vdec_msg_queue.c | 41 ++++++++++++++++---
.../platform/mtk-vcodec/vdec_msg_queue.h | 8 ++--
4 files changed, 57 insertions(+), 11 deletions(-)
diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
index d460703f335d..4fbff61d2334 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
@@ -341,6 +341,17 @@ static int mtk_vcodec_probe(struct platform_device *pdev)
goto err_dec_pm;
}
+ if (IS_VDEC_LAT_ARCH(dev->vdec_pdata->hw_arch)) {
+ vdec_msg_queue_init_ctx(&dev->msg_queue_core_ctx, MTK_VDEC_CORE);
+ dev->core_workqueue = alloc_ordered_workqueue("core-decoder",
+ WQ_MEM_RECLAIM | WQ_FREEZABLE);
+ if (!dev->core_workqueue) {
+ mtk_v4l2_err("Failed to create core workqueue");
+ ret = -EINVAL;
+ goto err_res;
+ }
+ }
+
for (i = 0; i < MTK_VDEC_HW_MAX; i++)
mutex_init(&dev->dec_mutex[i]);
spin_lock_init(&dev->irqlock);
@@ -351,7 +362,7 @@ static int mtk_vcodec_probe(struct platform_device *pdev)
ret = v4l2_device_register(&pdev->dev, &dev->v4l2_dev);
if (ret) {
mtk_v4l2_err("v4l2_device_register err=%d", ret);
- goto err_res;
+ goto err_core_workq;
}
init_waitqueue_head(&dev->queue);
@@ -450,6 +461,9 @@ static int mtk_vcodec_probe(struct platform_device *pdev)
video_unregister_device(vfd_dec);
err_dec_alloc:
v4l2_device_unregister(&dev->v4l2_dev);
+err_core_workq:
+ if (IS_VDEC_LAT_ARCH(dev->vdec_pdata->hw_arch))
+ destroy_workqueue(dev->core_workqueue);
err_res:
mtk_vcodec_release_dec_pm(&dev->pm);
err_dec_pm:
diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
index cbaed96dcfa2..a558cc16026d 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
@@ -27,6 +27,7 @@
#define MTK_VCODEC_MAX_PLANES 3
#define MTK_V4L2_BENCHMARK 0
#define WAIT_INTR_TIMEOUT_MS 1000
+#define IS_VDEC_LAT_ARCH(hw_arch) ((hw_arch) >= MTK_VDEC_LAT_SINGLE_CORE)
/*
* enum mtk_hw_reg_idx - MTK hw register base index
@@ -464,6 +465,7 @@ struct mtk_vcodec_enc_pdata {
* @dec_capability: used to identify decode capability, ex: 4k
* @enc_capability: used to identify encode capability
*
+ * @core_workqueue: queue used for core hardware decode
* @msg_queue_core_ctx: msg queue context used for core workqueue
*
* @subdev_dev: subdev hardware device
@@ -506,6 +508,7 @@ struct mtk_vcodec_dev {
unsigned int dec_capability;
unsigned int enc_capability;
+ struct workqueue_struct *core_workqueue;
struct vdec_msg_queue_ctx msg_queue_core_ctx;
void *subdev_dev[MTK_VDEC_HW_MAX];
diff --git a/drivers/media/platform/mtk-vcodec/vdec_msg_queue.c b/drivers/media/platform/mtk-vcodec/vdec_msg_queue.c
index 913aefa67618..24f1d03df9f1 100644
--- a/drivers/media/platform/mtk-vcodec/vdec_msg_queue.c
+++ b/drivers/media/platform/mtk-vcodec/vdec_msg_queue.c
@@ -68,6 +68,9 @@ int vdec_msg_queue_qbuf(struct vdec_msg_queue_ctx *msg_ctx, struct vdec_lat_buf
if (msg_ctx->hardware_index != MTK_VDEC_CORE)
wake_up_all(&msg_ctx->ready_to_use);
+ else
+ queue_work(buf->ctx->dev->core_workqueue,
+ &buf->ctx->msg_queue.core_work);
mtk_v4l2_debug(3, "enqueue buf type: %d addr: 0x%p num: %d",
msg_ctx->hardware_index, buf, msg_ctx->ready_num);
@@ -169,8 +172,7 @@ bool vdec_msg_queue_wait_lat_buf_full(struct vdec_msg_queue *msg_queue)
return false;
}
-void vdec_msg_queue_deinit(
- struct vdec_msg_queue *msg_queue,
+void vdec_msg_queue_deinit(struct vdec_msg_queue *msg_queue,
struct mtk_vcodec_ctx *ctx)
{
struct vdec_lat_buf *lat_buf;
@@ -196,10 +198,36 @@ void vdec_msg_queue_deinit(
}
}
-int vdec_msg_queue_init(
- struct vdec_msg_queue *msg_queue,
- struct mtk_vcodec_ctx *ctx,
- core_decode_cb_t core_decode,
+static void vdec_msg_queue_core_work(struct work_struct *work)
+{
+ struct vdec_msg_queue *msg_queue =
+ container_of(work, struct vdec_msg_queue, core_work);
+ struct mtk_vcodec_ctx *ctx =
+ container_of(msg_queue, struct mtk_vcodec_ctx, msg_queue);
+ struct mtk_vcodec_dev *dev = ctx->dev;
+ struct vdec_lat_buf *lat_buf;
+
+ lat_buf = vdec_msg_queue_dqbuf(&dev->msg_queue_core_ctx);
+ if (!lat_buf)
+ return;
+
+ ctx = lat_buf->ctx;
+ mtk_vcodec_set_curr_ctx(dev, ctx, MTK_VDEC_CORE);
+
+ lat_buf->core_decode(lat_buf);
+
+ mtk_vcodec_set_curr_ctx(dev, NULL, MTK_VDEC_CORE);
+ vdec_msg_queue_qbuf(&ctx->msg_queue.lat_ctx, lat_buf);
+
+ if (!list_empty(&ctx->msg_queue.lat_ctx.ready_queue)) {
+ mtk_v4l2_debug(3, "re-schedule to decode for core",
+ dev->msg_queue_core_ctx.ready_num);
+ queue_work(dev->core_workqueue, &msg_queue->core_work);
+ }
+}
+
+int vdec_msg_queue_init(struct vdec_msg_queue *msg_queue,
+ struct mtk_vcodec_ctx *ctx, core_decode_cb_t core_decode,
int private_size)
{
struct vdec_lat_buf *lat_buf;
@@ -210,6 +238,7 @@ int vdec_msg_queue_init(
return 0;
vdec_msg_queue_init_ctx(&msg_queue->lat_ctx, MTK_VDEC_LAT0);
+ INIT_WORK(&msg_queue->core_work, vdec_msg_queue_core_work);
msg_queue->wdma_addr.size = vde_msg_queue_get_trans_size(
ctx->picinfo.buf_w, ctx->picinfo.buf_h);
diff --git a/drivers/media/platform/mtk-vcodec/vdec_msg_queue.h b/drivers/media/platform/mtk-vcodec/vdec_msg_queue.h
index 21a9c0aeb1b4..43eae638a2a8 100644
--- a/drivers/media/platform/mtk-vcodec/vdec_msg_queue.h
+++ b/drivers/media/platform/mtk-vcodec/vdec_msg_queue.h
@@ -67,6 +67,7 @@ struct vdec_lat_buf {
* @wdma_addr: wdma address used for ube
* @wdma_rptr_addr: ube read point
* @wdma_wptr_addr: ube write point
+ * @core_work: core hardware work
* @lat_ctx: used to store lat buffer list
*/
struct vdec_msg_queue {
@@ -76,6 +77,7 @@ struct vdec_msg_queue {
uint64_t wdma_rptr_addr;
uint64_t wdma_wptr_addr;
+ struct work_struct core_work;
struct vdec_msg_queue_ctx lat_ctx;
};
@@ -86,10 +88,8 @@ struct vdec_msg_queue {
* @core_decode: core decode callback for each codec
* @private_size: the private data size used to share with core
*/
-int vdec_msg_queue_init(
- struct vdec_msg_queue *msg_queue,
- struct mtk_vcodec_ctx *ctx,
- core_decode_cb_t core_decode,
+int vdec_msg_queue_init(struct vdec_msg_queue *msg_queue,
+ struct mtk_vcodec_ctx *ctx, core_decode_cb_t core_decode,
int private_size);
/**
--
2.25.1
On Wed, Dec 1, 2021 at 7:46 PM Yunfei Dong <yunfei.dong@mediatek.com> wrote: > > Add work queue to process core hardware information. > First, get lat_buf from message queue, then call core > hardware of each codec(H264/VP9/AV1) to decode, finally > puts lat_buf back to the message. > > Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com> > --- > .../platform/mtk-vcodec/mtk_vcodec_dec_drv.c | 16 +++++++- > .../platform/mtk-vcodec/mtk_vcodec_drv.h | 3 ++ > .../platform/mtk-vcodec/vdec_msg_queue.c | 41 ++++++++++++++++--- > .../platform/mtk-vcodec/vdec_msg_queue.h | 8 ++-- > 4 files changed, 57 insertions(+), 11 deletions(-) > > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c > index d460703f335d..4fbff61d2334 100644 > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c > @@ -341,6 +341,17 @@ static int mtk_vcodec_probe(struct platform_device *pdev) > goto err_dec_pm; > } > > + if (IS_VDEC_LAT_ARCH(dev->vdec_pdata->hw_arch)) { > + vdec_msg_queue_init_ctx(&dev->msg_queue_core_ctx, MTK_VDEC_CORE); > + dev->core_workqueue = alloc_ordered_workqueue("core-decoder", > + WQ_MEM_RECLAIM | WQ_FREEZABLE); > + if (!dev->core_workqueue) { > + mtk_v4l2_err("Failed to create core workqueue"); > + ret = -EINVAL; > + goto err_res; > + } > + } > + > for (i = 0; i < MTK_VDEC_HW_MAX; i++) > mutex_init(&dev->dec_mutex[i]); > spin_lock_init(&dev->irqlock); > @@ -351,7 +362,7 @@ static int mtk_vcodec_probe(struct platform_device *pdev) > ret = v4l2_device_register(&pdev->dev, &dev->v4l2_dev); > if (ret) { > mtk_v4l2_err("v4l2_device_register err=%d", ret); > - goto err_res; > + goto err_core_workq; > } > > init_waitqueue_head(&dev->queue); > @@ -450,6 +461,9 @@ static int mtk_vcodec_probe(struct platform_device *pdev) > video_unregister_device(vfd_dec); > err_dec_alloc: > v4l2_device_unregister(&dev->v4l2_dev); > +err_core_workq: > + if (IS_VDEC_LAT_ARCH(dev->vdec_pdata->hw_arch)) > + destroy_workqueue(dev->core_workqueue); > err_res: > mtk_vcodec_release_dec_pm(&dev->pm); > err_dec_pm: > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h > index cbaed96dcfa2..a558cc16026d 100644 > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h > @@ -27,6 +27,7 @@ > #define MTK_VCODEC_MAX_PLANES 3 > #define MTK_V4L2_BENCHMARK 0 > #define WAIT_INTR_TIMEOUT_MS 1000 > +#define IS_VDEC_LAT_ARCH(hw_arch) ((hw_arch) >= MTK_VDEC_LAT_SINGLE_CORE) Basic question: What is practical meaning of this? What architectures are supported? > > /* > * enum mtk_hw_reg_idx - MTK hw register base index > @@ -464,6 +465,7 @@ struct mtk_vcodec_enc_pdata { > * @dec_capability: used to identify decode capability, ex: 4k > * @enc_capability: used to identify encode capability > * > + * @core_workqueue: queue used for core hardware decode > * @msg_queue_core_ctx: msg queue context used for core workqueue > * > * @subdev_dev: subdev hardware device > @@ -506,6 +508,7 @@ struct mtk_vcodec_dev { > unsigned int dec_capability; > unsigned int enc_capability; > > + struct workqueue_struct *core_workqueue; > struct vdec_msg_queue_ctx msg_queue_core_ctx; > > void *subdev_dev[MTK_VDEC_HW_MAX]; > diff --git a/drivers/media/platform/mtk-vcodec/vdec_msg_queue.c b/drivers/media/platform/mtk-vcodec/vdec_msg_queue.c > index 913aefa67618..24f1d03df9f1 100644 > --- a/drivers/media/platform/mtk-vcodec/vdec_msg_queue.c > +++ b/drivers/media/platform/mtk-vcodec/vdec_msg_queue.c > @@ -68,6 +68,9 @@ int vdec_msg_queue_qbuf(struct vdec_msg_queue_ctx *msg_ctx, struct vdec_lat_buf > > if (msg_ctx->hardware_index != MTK_VDEC_CORE) > wake_up_all(&msg_ctx->ready_to_use); > + else > + queue_work(buf->ctx->dev->core_workqueue, > + &buf->ctx->msg_queue.core_work); need {} for else here? > > mtk_v4l2_debug(3, "enqueue buf type: %d addr: 0x%p num: %d", > msg_ctx->hardware_index, buf, msg_ctx->ready_num); > @@ -169,8 +172,7 @@ bool vdec_msg_queue_wait_lat_buf_full(struct vdec_msg_queue *msg_queue) > return false; > } > > -void vdec_msg_queue_deinit( > - struct vdec_msg_queue *msg_queue, > +void vdec_msg_queue_deinit(struct vdec_msg_queue *msg_queue, > struct mtk_vcodec_ctx *ctx) > { > struct vdec_lat_buf *lat_buf; > @@ -196,10 +198,36 @@ void vdec_msg_queue_deinit( > } > } > > -int vdec_msg_queue_init( > - struct vdec_msg_queue *msg_queue, > - struct mtk_vcodec_ctx *ctx, > - core_decode_cb_t core_decode, > +static void vdec_msg_queue_core_work(struct work_struct *work) > +{ > + struct vdec_msg_queue *msg_queue = > + container_of(work, struct vdec_msg_queue, core_work); > + struct mtk_vcodec_ctx *ctx = > + container_of(msg_queue, struct mtk_vcodec_ctx, msg_queue); > + struct mtk_vcodec_dev *dev = ctx->dev; > + struct vdec_lat_buf *lat_buf; > + > + lat_buf = vdec_msg_queue_dqbuf(&dev->msg_queue_core_ctx); > + if (!lat_buf) > + return; If we were to return in this error condition, isn't it better to also differentiate this error with return code and change void return type? > + > + ctx = lat_buf->ctx; > + mtk_vcodec_set_curr_ctx(dev, ctx, MTK_VDEC_CORE); > + > + lat_buf->core_decode(lat_buf); > + > + mtk_vcodec_set_curr_ctx(dev, NULL, MTK_VDEC_CORE); > + vdec_msg_queue_qbuf(&ctx->msg_queue.lat_ctx, lat_buf); > + > + if (!list_empty(&ctx->msg_queue.lat_ctx.ready_queue)) { > + mtk_v4l2_debug(3, "re-schedule to decode for core", > + dev->msg_queue_core_ctx.ready_num); > + queue_work(dev->core_workqueue, &msg_queue->core_work); > + } > +} > + > +int vdec_msg_queue_init(struct vdec_msg_queue *msg_queue, > + struct mtk_vcodec_ctx *ctx, core_decode_cb_t core_decode, > int private_size) > { > struct vdec_lat_buf *lat_buf; > @@ -210,6 +238,7 @@ int vdec_msg_queue_init( > return 0; > > vdec_msg_queue_init_ctx(&msg_queue->lat_ctx, MTK_VDEC_LAT0); > + INIT_WORK(&msg_queue->core_work, vdec_msg_queue_core_work); > msg_queue->wdma_addr.size = vde_msg_queue_get_trans_size( > ctx->picinfo.buf_w, ctx->picinfo.buf_h); > > diff --git a/drivers/media/platform/mtk-vcodec/vdec_msg_queue.h b/drivers/media/platform/mtk-vcodec/vdec_msg_queue.h > index 21a9c0aeb1b4..43eae638a2a8 100644 > --- a/drivers/media/platform/mtk-vcodec/vdec_msg_queue.h > +++ b/drivers/media/platform/mtk-vcodec/vdec_msg_queue.h > @@ -67,6 +67,7 @@ struct vdec_lat_buf { > * @wdma_addr: wdma address used for ube > * @wdma_rptr_addr: ube read point > * @wdma_wptr_addr: ube write point > + * @core_work: core hardware work > * @lat_ctx: used to store lat buffer list > */ > struct vdec_msg_queue { > @@ -76,6 +77,7 @@ struct vdec_msg_queue { > uint64_t wdma_rptr_addr; > uint64_t wdma_wptr_addr; > > + struct work_struct core_work; > struct vdec_msg_queue_ctx lat_ctx; > }; > > @@ -86,10 +88,8 @@ struct vdec_msg_queue { > * @core_decode: core decode callback for each codec > * @private_size: the private data size used to share with core > */ > -int vdec_msg_queue_init( > - struct vdec_msg_queue *msg_queue, > - struct mtk_vcodec_ctx *ctx, > - core_decode_cb_t core_decode, > +int vdec_msg_queue_init(struct vdec_msg_queue *msg_queue, > + struct mtk_vcodec_ctx *ctx, core_decode_cb_t core_decode, Not sure about the formatting rule, but is it supposed to be one param per line? If so, this comment also applied to function definition part. > int private_size); > > /** > -- > 2.25.1 >
Hi steve, Thanks for your suggestion. On Thu, 2021-12-09 at 15:44 -0800, Steve Cho wrote: > On Wed, Dec 1, 2021 at 7:46 PM Yunfei Dong <yunfei.dong@mediatek.com> > wrote: > > > > Add work queue to process core hardware information. > > First, get lat_buf from message queue, then call core > > hardware of each codec(H264/VP9/AV1) to decode, finally > > puts lat_buf back to the message. > > > > Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com> > > --- > > .../platform/mtk-vcodec/mtk_vcodec_dec_drv.c | 16 +++++++- > > .../platform/mtk-vcodec/mtk_vcodec_drv.h | 3 ++ > > .../platform/mtk-vcodec/vdec_msg_queue.c | 41 > > ++++++++++++++++--- > > .../platform/mtk-vcodec/vdec_msg_queue.h | 8 ++-- > > 4 files changed, 57 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c > > b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c > > index d460703f335d..4fbff61d2334 100644 > > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c > > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c > > @@ -341,6 +341,17 @@ static int mtk_vcodec_probe(struct > > platform_device *pdev) > > goto err_dec_pm; > > } > > > > + if (IS_VDEC_LAT_ARCH(dev->vdec_pdata->hw_arch)) { > > + vdec_msg_queue_init_ctx(&dev->msg_queue_core_ctx, > > MTK_VDEC_CORE); > > + dev->core_workqueue = > > alloc_ordered_workqueue("core-decoder", > > + WQ_MEM_RECLAIM | WQ_FREEZABLE); > > + if (!dev->core_workqueue) { > > + mtk_v4l2_err("Failed to create core > > workqueue"); > > + ret = -EINVAL; > > + goto err_res; > > + } > > + } > > + > > for (i = 0; i < MTK_VDEC_HW_MAX; i++) > > mutex_init(&dev->dec_mutex[i]); > > spin_lock_init(&dev->irqlock); > > @@ -351,7 +362,7 @@ static int mtk_vcodec_probe(struct > > platform_device *pdev) > > ret = v4l2_device_register(&pdev->dev, &dev->v4l2_dev); > > if (ret) { > > mtk_v4l2_err("v4l2_device_register err=%d", ret); > > - goto err_res; > > + goto err_core_workq; > > } > > > > init_waitqueue_head(&dev->queue); > > @@ -450,6 +461,9 @@ static int mtk_vcodec_probe(struct > > platform_device *pdev) > > video_unregister_device(vfd_dec); > > err_dec_alloc: > > v4l2_device_unregister(&dev->v4l2_dev); > > +err_core_workq: > > + if (IS_VDEC_LAT_ARCH(dev->vdec_pdata->hw_arch)) > > + destroy_workqueue(dev->core_workqueue); > > err_res: > > mtk_vcodec_release_dec_pm(&dev->pm); > > err_dec_pm: > > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h > > b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h > > index cbaed96dcfa2..a558cc16026d 100644 > > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h > > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h > > @@ -27,6 +27,7 @@ > > #define MTK_VCODEC_MAX_PLANES 3 > > #define MTK_V4L2_BENCHMARK 0 > > #define WAIT_INTR_TIMEOUT_MS 1000 > > +#define IS_VDEC_LAT_ARCH(hw_arch) ((hw_arch) >= > > MTK_VDEC_LAT_SINGLE_CORE) > > Basic question: What is practical meaning of this? What architectures > are supported? > This definition is used to separate different architectures. Pure single core/lat single core at current period is supported. > > > > /* > > * enum mtk_hw_reg_idx - MTK hw register base index > > @@ -464,6 +465,7 @@ struct mtk_vcodec_enc_pdata { > > * @dec_capability: used to identify decode capability, ex: 4k > > * @enc_capability: used to identify encode capability > > * > > + * @core_workqueue: queue used for core hardware decode > > * @msg_queue_core_ctx: msg queue context used for core workqueue > > * > > * @subdev_dev: subdev hardware device > > @@ -506,6 +508,7 @@ struct mtk_vcodec_dev { > > unsigned int dec_capability; > > unsigned int enc_capability; > > > > + struct workqueue_struct *core_workqueue; > > struct vdec_msg_queue_ctx msg_queue_core_ctx; > > > > void *subdev_dev[MTK_VDEC_HW_MAX]; > > diff --git a/drivers/media/platform/mtk-vcodec/vdec_msg_queue.c > > b/drivers/media/platform/mtk-vcodec/vdec_msg_queue.c > > index 913aefa67618..24f1d03df9f1 100644 > > --- a/drivers/media/platform/mtk-vcodec/vdec_msg_queue.c > > +++ b/drivers/media/platform/mtk-vcodec/vdec_msg_queue.c > > @@ -68,6 +68,9 @@ int vdec_msg_queue_qbuf(struct vdec_msg_queue_ctx > > *msg_ctx, struct vdec_lat_buf > > > > if (msg_ctx->hardware_index != MTK_VDEC_CORE) > > wake_up_all(&msg_ctx->ready_to_use); > > + else > > + queue_work(buf->ctx->dev->core_workqueue, > > + &buf->ctx->msg_queue.core_work); > > need {} for else here? > If condition not add "{}", else need not to add "{}" ? > > > > mtk_v4l2_debug(3, "enqueue buf type: %d addr: 0x%p num: > > %d", > > msg_ctx->hardware_index, buf, msg_ctx->ready_num); > > @@ -169,8 +172,7 @@ bool vdec_msg_queue_wait_lat_buf_full(struct > > vdec_msg_queue *msg_queue) > > return false; > > } > > > > -void vdec_msg_queue_deinit( > > - struct vdec_msg_queue *msg_queue, > > +void vdec_msg_queue_deinit(struct vdec_msg_queue *msg_queue, > > struct mtk_vcodec_ctx *ctx) > > { > > struct vdec_lat_buf *lat_buf; > > @@ -196,10 +198,36 @@ void vdec_msg_queue_deinit( > > } > > } > > > > -int vdec_msg_queue_init( > > - struct vdec_msg_queue *msg_queue, > > - struct mtk_vcodec_ctx *ctx, > > - core_decode_cb_t core_decode, > > +static void vdec_msg_queue_core_work(struct work_struct *work) > > +{ > > + struct vdec_msg_queue *msg_queue = > > + container_of(work, struct vdec_msg_queue, > > core_work); > > + struct mtk_vcodec_ctx *ctx = > > + container_of(msg_queue, struct mtk_vcodec_ctx, > > msg_queue); > > + struct mtk_vcodec_dev *dev = ctx->dev; > > + struct vdec_lat_buf *lat_buf; > > + > > + lat_buf = vdec_msg_queue_dqbuf(&dev->msg_queue_core_ctx); > > + if (!lat_buf) > > + return; > > If we were to return in this error condition, > isn't it better to also differentiate this error with return code and > change void return type? > vdec_msg_queue_core_work function is callback for param "func" in struct work_struct, need not to add return value. > > + > > + ctx = lat_buf->ctx; > > + mtk_vcodec_set_curr_ctx(dev, ctx, MTK_VDEC_CORE); > > + > > + lat_buf->core_decode(lat_buf); > > + > > + mtk_vcodec_set_curr_ctx(dev, NULL, MTK_VDEC_CORE); > > + vdec_msg_queue_qbuf(&ctx->msg_queue.lat_ctx, lat_buf); > > + > > + if (!list_empty(&ctx->msg_queue.lat_ctx.ready_queue)) { > > + mtk_v4l2_debug(3, "re-schedule to decode for core", > > + dev->msg_queue_core_ctx.ready_num); > > + queue_work(dev->core_workqueue, &msg_queue- > > >core_work); > > + } > > +} > > + > > +int vdec_msg_queue_init(struct vdec_msg_queue *msg_queue, > > + struct mtk_vcodec_ctx *ctx, core_decode_cb_t > > core_decode, > > int private_size) > > { > > struct vdec_lat_buf *lat_buf; > > @@ -210,6 +238,7 @@ int vdec_msg_queue_init( > > return 0; > > > > vdec_msg_queue_init_ctx(&msg_queue->lat_ctx, > > MTK_VDEC_LAT0); > > + INIT_WORK(&msg_queue->core_work, vdec_msg_queue_core_work); > > msg_queue->wdma_addr.size = vde_msg_queue_get_trans_size( > > ctx->picinfo.buf_w, ctx->picinfo.buf_h); > > > > diff --git a/drivers/media/platform/mtk-vcodec/vdec_msg_queue.h > > b/drivers/media/platform/mtk-vcodec/vdec_msg_queue.h > > index 21a9c0aeb1b4..43eae638a2a8 100644 > > --- a/drivers/media/platform/mtk-vcodec/vdec_msg_queue.h > > +++ b/drivers/media/platform/mtk-vcodec/vdec_msg_queue.h > > @@ -67,6 +67,7 @@ struct vdec_lat_buf { > > * @wdma_addr: wdma address used for ube > > * @wdma_rptr_addr: ube read point > > * @wdma_wptr_addr: ube write point > > + * @core_work: core hardware work > > * @lat_ctx: used to store lat buffer list > > */ > > struct vdec_msg_queue { > > @@ -76,6 +77,7 @@ struct vdec_msg_queue { > > uint64_t wdma_rptr_addr; > > uint64_t wdma_wptr_addr; > > > > + struct work_struct core_work; > > struct vdec_msg_queue_ctx lat_ctx; > > }; > > > > @@ -86,10 +88,8 @@ struct vdec_msg_queue { > > * @core_decode: core decode callback for each codec > > * @private_size: the private data size used to share with core > > */ > > -int vdec_msg_queue_init( > > - struct vdec_msg_queue *msg_queue, > > - struct mtk_vcodec_ctx *ctx, > > - core_decode_cb_t core_decode, > > +int vdec_msg_queue_init(struct vdec_msg_queue *msg_queue, > > + struct mtk_vcodec_ctx *ctx, core_decode_cb_t > > core_decode, > > Not sure about the formatting rule, but is it supposed to be one > param per line? > If so, this comment also applied to function definition part. > > > int private_size); > > I try to review other files, it looks that two or more parameters per line is ok. Whether you mean that we'd better to write one parameter per line? int vdec_msg_queue_init(struct vdec_msg_queue *msg_queue, struct mtk_vcodec_ctx *ctx, core_decode_cb_t Thanks, Yunfei Dong > > /** > > -- > > 2.25.1 > >
Hi Yunfei, Thank you for addressing my questions and comments. Note that my comments for formatting are usually based on standards from chromium & chrome os. So feel free to reflect only if you think it is relevant. Reviewed-by: Steve Cho <stevecho@chromium.org> On Mon, Dec 13, 2021 at 12:52 AM yunfei.dong@mediatek.com <yunfei.dong@mediatek.com> wrote: > > Hi steve, > > Thanks for your suggestion. > > On Thu, 2021-12-09 at 15:44 -0800, Steve Cho wrote: > > On Wed, Dec 1, 2021 at 7:46 PM Yunfei Dong <yunfei.dong@mediatek.com> > > wrote: > > > > > > Add work queue to process core hardware information. > > > First, get lat_buf from message queue, then call core > > > hardware of each codec(H264/VP9/AV1) to decode, finally > > > puts lat_buf back to the message. > > > > > > Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com> > > > --- > > > .../platform/mtk-vcodec/mtk_vcodec_dec_drv.c | 16 +++++++- > > > .../platform/mtk-vcodec/mtk_vcodec_drv.h | 3 ++ > > > .../platform/mtk-vcodec/vdec_msg_queue.c | 41 > > > ++++++++++++++++--- > > > .../platform/mtk-vcodec/vdec_msg_queue.h | 8 ++-- > > > 4 files changed, 57 insertions(+), 11 deletions(-) > > > > > > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c > > > b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c > > > index d460703f335d..4fbff61d2334 100644 > > > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c > > > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c > > > @@ -341,6 +341,17 @@ static int mtk_vcodec_probe(struct > > > platform_device *pdev) > > > goto err_dec_pm; > > > } > > > > > > + if (IS_VDEC_LAT_ARCH(dev->vdec_pdata->hw_arch)) { > > > + vdec_msg_queue_init_ctx(&dev->msg_queue_core_ctx, > > > MTK_VDEC_CORE); > > > + dev->core_workqueue = > > > alloc_ordered_workqueue("core-decoder", > > > + WQ_MEM_RECLAIM | WQ_FREEZABLE); > > > + if (!dev->core_workqueue) { > > > + mtk_v4l2_err("Failed to create core > > > workqueue"); > > > + ret = -EINVAL; > > > + goto err_res; > > > + } > > > + } > > > + > > > for (i = 0; i < MTK_VDEC_HW_MAX; i++) > > > mutex_init(&dev->dec_mutex[i]); > > > spin_lock_init(&dev->irqlock); > > > @@ -351,7 +362,7 @@ static int mtk_vcodec_probe(struct > > > platform_device *pdev) > > > ret = v4l2_device_register(&pdev->dev, &dev->v4l2_dev); > > > if (ret) { > > > mtk_v4l2_err("v4l2_device_register err=%d", ret); > > > - goto err_res; > > > + goto err_core_workq; > > > } > > > > > > init_waitqueue_head(&dev->queue); > > > @@ -450,6 +461,9 @@ static int mtk_vcodec_probe(struct > > > platform_device *pdev) > > > video_unregister_device(vfd_dec); > > > err_dec_alloc: > > > v4l2_device_unregister(&dev->v4l2_dev); > > > +err_core_workq: > > > + if (IS_VDEC_LAT_ARCH(dev->vdec_pdata->hw_arch)) > > > + destroy_workqueue(dev->core_workqueue); > > > err_res: > > > mtk_vcodec_release_dec_pm(&dev->pm); > > > err_dec_pm: > > > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h > > > b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h > > > index cbaed96dcfa2..a558cc16026d 100644 > > > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h > > > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h > > > @@ -27,6 +27,7 @@ > > > #define MTK_VCODEC_MAX_PLANES 3 > > > #define MTK_V4L2_BENCHMARK 0 > > > #define WAIT_INTR_TIMEOUT_MS 1000 > > > +#define IS_VDEC_LAT_ARCH(hw_arch) ((hw_arch) >= > > > MTK_VDEC_LAT_SINGLE_CORE) > > > > Basic question: What is practical meaning of this? What architectures > > are supported? > > > This definition is used to separate different architectures. > Pure single core/lat single core at current period is supported. > > > > > > /* > > > * enum mtk_hw_reg_idx - MTK hw register base index > > > @@ -464,6 +465,7 @@ struct mtk_vcodec_enc_pdata { > > > * @dec_capability: used to identify decode capability, ex: 4k > > > * @enc_capability: used to identify encode capability > > > * > > > + * @core_workqueue: queue used for core hardware decode > > > * @msg_queue_core_ctx: msg queue context used for core workqueue > > > * > > > * @subdev_dev: subdev hardware device > > > @@ -506,6 +508,7 @@ struct mtk_vcodec_dev { > > > unsigned int dec_capability; > > > unsigned int enc_capability; > > > > > > + struct workqueue_struct *core_workqueue; > > > struct vdec_msg_queue_ctx msg_queue_core_ctx; > > > > > > void *subdev_dev[MTK_VDEC_HW_MAX]; > > > diff --git a/drivers/media/platform/mtk-vcodec/vdec_msg_queue.c > > > b/drivers/media/platform/mtk-vcodec/vdec_msg_queue.c > > > index 913aefa67618..24f1d03df9f1 100644 > > > --- a/drivers/media/platform/mtk-vcodec/vdec_msg_queue.c > > > +++ b/drivers/media/platform/mtk-vcodec/vdec_msg_queue.c > > > @@ -68,6 +68,9 @@ int vdec_msg_queue_qbuf(struct vdec_msg_queue_ctx > > > *msg_ctx, struct vdec_lat_buf > > > > > > if (msg_ctx->hardware_index != MTK_VDEC_CORE) > > > wake_up_all(&msg_ctx->ready_to_use); > > > + else > > > + queue_work(buf->ctx->dev->core_workqueue, > > > + &buf->ctx->msg_queue.core_work); > > > > need {} for else here? > > > If condition not add "{}", else need not to add "{}" ? I was suggesting as it had 2 lines. > > > > > > mtk_v4l2_debug(3, "enqueue buf type: %d addr: 0x%p num: > > > %d", > > > msg_ctx->hardware_index, buf, msg_ctx->ready_num); > > > @@ -169,8 +172,7 @@ bool vdec_msg_queue_wait_lat_buf_full(struct > > > vdec_msg_queue *msg_queue) > > > return false; > > > } > > > > > > -void vdec_msg_queue_deinit( > > > - struct vdec_msg_queue *msg_queue, > > > +void vdec_msg_queue_deinit(struct vdec_msg_queue *msg_queue, > > > struct mtk_vcodec_ctx *ctx) > > > { > > > struct vdec_lat_buf *lat_buf; > > > @@ -196,10 +198,36 @@ void vdec_msg_queue_deinit( > > > } > > > } > > > > > > -int vdec_msg_queue_init( > > > - struct vdec_msg_queue *msg_queue, > > > - struct mtk_vcodec_ctx *ctx, > > > - core_decode_cb_t core_decode, > > > +static void vdec_msg_queue_core_work(struct work_struct *work) > > > +{ > > > + struct vdec_msg_queue *msg_queue = > > > + container_of(work, struct vdec_msg_queue, > > > core_work); > > > + struct mtk_vcodec_ctx *ctx = > > > + container_of(msg_queue, struct mtk_vcodec_ctx, > > > msg_queue); > > > + struct mtk_vcodec_dev *dev = ctx->dev; > > > + struct vdec_lat_buf *lat_buf; > > > + > > > + lat_buf = vdec_msg_queue_dqbuf(&dev->msg_queue_core_ctx); > > > + if (!lat_buf) > > > + return; > > > > If we were to return in this error condition, > > isn't it better to also differentiate this error with return code and > > change void return type? > > > vdec_msg_queue_core_work function is callback for param "func" in > struct work_struct, need not to add return value. > > > > + > > > + ctx = lat_buf->ctx; > > > + mtk_vcodec_set_curr_ctx(dev, ctx, MTK_VDEC_CORE); > > > + > > > + lat_buf->core_decode(lat_buf); > > > + > > > + mtk_vcodec_set_curr_ctx(dev, NULL, MTK_VDEC_CORE); > > > + vdec_msg_queue_qbuf(&ctx->msg_queue.lat_ctx, lat_buf); > > > + > > > + if (!list_empty(&ctx->msg_queue.lat_ctx.ready_queue)) { > > > + mtk_v4l2_debug(3, "re-schedule to decode for core", > > > + dev->msg_queue_core_ctx.ready_num); > > > + queue_work(dev->core_workqueue, &msg_queue- > > > >core_work); > > > + } > > > +} > > > + > > > +int vdec_msg_queue_init(struct vdec_msg_queue *msg_queue, > > > + struct mtk_vcodec_ctx *ctx, core_decode_cb_t > > > core_decode, > > > int private_size) > > > { > > > struct vdec_lat_buf *lat_buf; > > > @@ -210,6 +238,7 @@ int vdec_msg_queue_init( > > > return 0; > > > > > > vdec_msg_queue_init_ctx(&msg_queue->lat_ctx, > > > MTK_VDEC_LAT0); > > > + INIT_WORK(&msg_queue->core_work, vdec_msg_queue_core_work); > > > msg_queue->wdma_addr.size = vde_msg_queue_get_trans_size( > > > ctx->picinfo.buf_w, ctx->picinfo.buf_h); > > > > > > diff --git a/drivers/media/platform/mtk-vcodec/vdec_msg_queue.h > > > b/drivers/media/platform/mtk-vcodec/vdec_msg_queue.h > > > index 21a9c0aeb1b4..43eae638a2a8 100644 > > > --- a/drivers/media/platform/mtk-vcodec/vdec_msg_queue.h > > > +++ b/drivers/media/platform/mtk-vcodec/vdec_msg_queue.h > > > @@ -67,6 +67,7 @@ struct vdec_lat_buf { > > > * @wdma_addr: wdma address used for ube > > > * @wdma_rptr_addr: ube read point > > > * @wdma_wptr_addr: ube write point > > > + * @core_work: core hardware work > > > * @lat_ctx: used to store lat buffer list > > > */ > > > struct vdec_msg_queue { > > > @@ -76,6 +77,7 @@ struct vdec_msg_queue { > > > uint64_t wdma_rptr_addr; > > > uint64_t wdma_wptr_addr; > > > > > > + struct work_struct core_work; > > > struct vdec_msg_queue_ctx lat_ctx; > > > }; > > > > > > @@ -86,10 +88,8 @@ struct vdec_msg_queue { > > > * @core_decode: core decode callback for each codec > > > * @private_size: the private data size used to share with core > > > */ > > > -int vdec_msg_queue_init( > > > - struct vdec_msg_queue *msg_queue, > > > - struct mtk_vcodec_ctx *ctx, > > > - core_decode_cb_t core_decode, > > > +int vdec_msg_queue_init(struct vdec_msg_queue *msg_queue, > > > + struct mtk_vcodec_ctx *ctx, core_decode_cb_t > > > core_decode, > > > > Not sure about the formatting rule, but is it supposed to be one > > param per line? > > If so, this comment also applied to function definition part. > > > > > int private_size); > > > > I try to review other files, it looks that two or more parameters per > line is ok. Whether you mean that we'd better to write one parameter > per line? > int vdec_msg_queue_init(struct vdec_msg_queue *msg_queue, > struct mtk_vcodec_ctx *ctx, > core_decode_cb_t Yes, that's what I meant, but you are welcome to keep current change. > > Thanks, > Yunfei Dong > > > /** > > > -- > > > 2.25.1 > > >
© 2016 - 2024 Red Hat, Inc.