Re: [PATCH v1, 07/14] media: mtk-vcodec: Add msg queue feature for lat and core architecture

[mtk12024 <yunfei.dong@mediatek.com>]

Hi Tzung-Bi,

Thanks for your detail feedback.
I add the description according to your each comments.

On Fri, 2021-07-09 at 17:39 +0800, Tzung-Bi Shih wrote:
> On Wed, Jul 7, 2021 at 2:22 PM Yunfei Dong <yunfei.dong@mediatek.com> wrote:
> > @@ -464,6 +469,11 @@ struct mtk_vcodec_enc_pdata {
> >   * comp_dev: component hardware device
> >   * component_node: component node
> >   * comp_idx: component index
> > + *
> > + * core_read: Wait queue used to signalize when core get useful lat buffer
> > + * core_queue: List of V4L2 lat_buf
> To be neat, replace "Wait" to "wait" and "List" to "list".
Will fix.
> > +int vdec_msg_queue_init(
> > +       struct mtk_vcodec_ctx *ctx,
> > +       struct vdec_msg_queue *msg_queue,
> > +       core_decode_cb_t core_decode,
> > +       int private_size)
> > +{
> > +       struct vdec_lat_buf *lat_buf;
> > +       int i, err;
> > +
> > +       init_waitqueue_head(&msg_queue->lat_read);
> > +       INIT_LIST_HEAD(&msg_queue->lat_queue);
> > +       spin_lock_init(&msg_queue->lat_lock);
> > +       msg_queue->num_lat = 0;
> > +
> > +       msg_queue->wdma_addr.size = vde_msg_queue_get_trans_size(
> > +               ctx->picinfo.buf_w, ctx->picinfo.buf_h);
> > +
> > +       err = mtk_vcodec_mem_alloc(ctx, &msg_queue->wdma_addr);
> > +       if (err) {
> > +               mtk_v4l2_err("failed to allocate wdma_addr buf");
> > +               return -ENOMEM;
> > +       }
> > +       msg_queue->wdma_rptr_addr = msg_queue->wdma_addr.dma_addr;
> > +       msg_queue->wdma_wptr_addr = msg_queue->wdma_addr.dma_addr;
> > +
> > +       for (i = 0; i < NUM_BUFFER_COUNT; i++) {
> > +               lat_buf = &msg_queue->lat_buf[i];
> > +
> > +               lat_buf->wdma_err_addr.size = VDEC_ERR_MAP_SZ_AVC;
> > +               err = mtk_vcodec_mem_alloc(ctx, &lat_buf->wdma_err_addr);
> > +               if (err) {
> > +                       mtk_v4l2_err("failed to allocate wdma_err_addr buf[%d]", i);
> > +                       return -ENOMEM;
> > +               }
> > +
> > +               lat_buf->slice_bc_addr.size = VDEC_LAT_SLICE_HEADER_SZ;
> > +               err = mtk_vcodec_mem_alloc(ctx, &lat_buf->slice_bc_addr);
> > +               if (err) {
> > +                       mtk_v4l2_err("failed to allocate wdma_addr buf[%d]", i);
> > +                       return -ENOMEM;
> > +               }
> > +
> > +               lat_buf->private_data = kzalloc(private_size, GFP_KERNEL);
> > +               if (!lat_buf->private_data) {
> > +                       mtk_v4l2_err("failed to allocate private_data[%d]", i);
> > +                       return -ENOMEM;
> > +               }
> > +
> > +               lat_buf->ctx = ctx;
> > +               lat_buf->core_decode = core_decode;
> > +               vdec_msg_queue_buf_to_lat(lat_buf);
> > +       }
> Doesn't it need to call mtk_vcodec_mem_free() and kfree() for any failure paths?
When allocate memory fail, will call deinit function auto, then free all allocated memory.
> > +struct vdec_lat_buf *vdec_msg_queue_get_core_buf(
> > +       struct mtk_vcodec_dev *dev)
> > +{
> > +       struct vdec_lat_buf *buf;
> > +       int ret;
> > +
> > +       spin_lock(&dev->core_lock);
> > +       if (list_empty(&dev->core_queue)) {
> > +               mtk_v4l2_debug(3, "core queue is NULL, num_core = %d", dev->num_core);
> > +               spin_unlock(&dev->core_lock);
> > +               ret = wait_event_freezable(dev->core_read,
> > +                       !list_empty(&dev->core_queue));
> > +               if (ret)
> > +                       return NULL;
> Should be !ret?
According the definidtion, when condition is true, return value is 0.
#define wait_event_freezable(wq_head, condition)				\
({										\
	int __ret = 0;								\
	might_sleep();								\
	if (!(condition))							\
		__ret = __wait_event_freezable(wq_head, condition);		\
	__ret;									\
}) 
> > +void vdec_msg_queue_buf_to_core(struct mtk_vcodec_dev *dev,
> > +       struct vdec_lat_buf *buf)
> > +{
> > +       spin_lock(&dev->core_lock);
> > +       list_add_tail(&buf->core_list, &dev->core_queue);
> > +       dev->num_core++;
> > +       wake_up_all(&dev->core_read);
> > +       mtk_v4l2_debug(3, "queu buf addr: (0x%p)", buf);
> Typo.
> 
> > +bool vdec_msg_queue_wait_lat_buf_full(struct vdec_msg_queue *msg_queue)
> > +{
> > +       long timeout_jiff;
> > +       int ret, i;
> > +
> > +       for (i = 0; i < NUM_BUFFER_COUNT + 2; i++) {
> > +              timeout_jiff = msecs_to_jiffies(1000);
> > +              ret = wait_event_timeout(msg_queue->lat_read,
> > +                    msg_queue->num_lat == NUM_BUFFER_COUNT, timeout_jiff);
> > +              if (ret) {
> > +                     mtk_v4l2_debug(3, "success to get lat buf: %d",
> > +                            msg_queue->num_lat);
> > +                     return true;
> > +              }
> > +       }
> Why does it need the loop?  i is unused.
Core maybe decode timeout, need to wait all core buffer process
completely.
> > +void vdec_msg_queue_deinit(
> > +       struct mtk_vcodec_ctx *ctx,
> > +       struct vdec_msg_queue *msg_queue)
> > +{
> > +       struct vdec_lat_buf *lat_buf;
> > +       struct mtk_vcodec_mem *mem;
> > +       int i;
> > +
> > +       mem = &msg_queue->wdma_addr;
> > +       if (mem->va)
> > +               mtk_vcodec_mem_free(ctx, mem);
> > +       for (i = 0; i < NUM_BUFFER_COUNT; i++) {
> > +               lat_buf = &msg_queue->lat_buf[i];
> > +
> > +               mem = &lat_buf->wdma_err_addr;
> > +               if (mem->va)
> > +                       mtk_vcodec_mem_free(ctx, mem);
> > +
> > +               mem = &lat_buf->slice_bc_addr;
> > +               if (mem->va)
> > +                       mtk_vcodec_mem_free(ctx, mem);
> > +
> > +               if (lat_buf->private_data)
> > +                       kfree(lat_buf->private_data);
> > +       }
> > +
> > +       msg_queue->init_done = false;
> Have no idea what init_done does in the code.  It is not included in
> any branch condition.
When call vdec_msg_queue_init will set this parameter to true.
> > +/**
> > + * vdec_msg_queue_init - init lat buffer information.
> > + * @ctx: v4l2 ctx
> > + * @msg_queue: used to store the lat buffer information
> > + * @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 mtk_vcodec_ctx *ctx,
> > +       struct vdec_msg_queue *msg_queue,
> > +       core_decode_cb_t core_decode,
> > +       int private_size);
> Would prefer to have *msg_queue as the first argument (also applies to
> all operators of vdec_msg_queue).
Can fix.
> > +/**
> > + * vdec_msg_queue_get_core_buf - get used core buffer for lat decode.
> > + * @dev: mtk vcodec device
> > + */
> > +struct vdec_lat_buf *vdec_msg_queue_get_core_buf(
> > +       struct mtk_vcodec_dev *dev);
> This is weird: vdec_msg_queue's operator but manipulating mtk_vcodec_dev?
vdec_msg_queue is used to share message between lat and core, for each
instance has its lat msg queue list, but all instance share one core msg
queue list. When try to get core buffer need to get it from core queue
list. Then queue it to lat queue list when core decode done.
> > +
> > +/**
> > + * vdec_msg_queue_buf_to_core - queue buf to the core for core decode.
> > + * @dev: mtk vcodec device
> > + * @buf: current lat buffer
> > + */
> > +void vdec_msg_queue_buf_to_core(struct mtk_vcodec_dev *dev,
> > +       struct vdec_lat_buf *buf);
> Also weird.
> 
> > +/**
> > + * vdec_msg_queue_buf_to_lat - queue buf to lat for lat decode.
> > + * @buf: current lat buffer
> > + */
> > +void vdec_msg_queue_buf_to_lat(struct vdec_lat_buf *buf);
> It should at least accept a struct vdec_msg_queue argument (or which
> msg queue should the buf put into?).
All buffer is struct vdec_lat_buf, used to share info between lat and core queue list.
> > +/**
> > + * vdec_msg_queue_update_ube_rptr - used to updata the ube read point.
> Typo.
> 
> > +/**
> > + * vdec_msg_queue_update_ube_wptr - used to updata the ube write point.
> Typo.
> 
> > +/**
> > + * vdec_msg_queue_deinit - deinit lat buffer information.
> > + * @ctx: v4l2 ctx
> > + * @msg_queue: used to store the lat buffer information
> > + */
> > +void vdec_msg_queue_deinit(
> > +       struct mtk_vcodec_ctx *ctx,
> > +       struct vdec_msg_queue *msg_queue);
> Would prefer to have *msg_queue as the first argument.
Yes, can fix.
> 
> The position of struct vdec_msg_queue is weird.  It looks like the msg
> queue is only for struct vdec_lat_buf.  If so, would vdec_msg_queue be
> better to call vdec_lat_queue or something similar?
> 
> It shouldn't touch the core queue in mtk_vcodec_dev anyway.  Is it
> possible to generalize the queue-related code for both lat and core
> queues?
Lat queue list is separately for each instance, but only has one core
queue list.