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

[Tzung-Bi Shih <tzungbi@google.com>]

On Mon, Jul 12, 2021 at 3:28 PM mtk12024 <yunfei.dong@mediatek.com> wrote:
> 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:
> > 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.
I guess you mean: if vdec_msg_queue_init() fails,
vdec_msg_queue_deinit() should be called?

If so:
- It is not "auto".  It depends on callers to invoke _deinit() if _init() fails.
- The API usage would be a bit weird: if the object hasn't been
initialized, shall we de-initialize it?

> > > +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.
Yeah, you're right.  I was confused a bit with wait_event_timeout().

> > > +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.
The point is: the i is unused.  If it needs more time to complete,
could it just wait for (NUM_BUFFER_COUNT + 2) * 1000 msecs?

> > > +       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.
The point is: if init_done doesn't change any code branch but just a
flag, does it really need the flag?

For example usages:
- If see the msg_queue->init_done has already been set to true in
vdec_msg_queue_init(), return errors.
- If see the msg_queue->init_done has already been set to false in
vdec_msg_queue_deinit(), return errors.

In the cases, I believe it brings very limited benefit (i.e. the
msg_queue is likely to _init and _deinit only once).

> > > +/**
> > > + * 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.
I guess you mean: during runtime, it has n lat queues and 1 core queue.

If so, would it be intuitive and simple by:

msg_queue *core_q;
msg_queue *lat_q[LAT_N];

vdec_msg_queue_dequeue(core_q) if it wants to get from core queue.
vdec_msg_queue_enqueue(lat_q[X], data) if it wants to put data to lat queue X.

> > > +/**
> > > + * 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.
The API semantic needs to provide a way to specify which msg_queue the
buf would put into.

> > 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.
Suggested to generalize the vdec_msg_queue to handle both lat and core
(and maybe furthermore).  See comment above.