Hi Hans,
On Fri, Nov 08, 2024 at 09:40:30AM +0100, Hans Verkuil wrote:
> On 08/11/2024 09:23, Sakari Ailus wrote:
> > Hi Hans,
> >
> > On Tue, Nov 05, 2024 at 08:50:39AM +0100, Hans Verkuil wrote:
> >> Stateful codecs must support the V4L2_CID_MIN_BUFFERS_FOR_OUTPUT
> >> and V4L2_CID_MIN_BUFFERS_FOR_CAPTURE controls. The vicodec driver
> >> was missing support for these controls. Add them.
> >>
> >> Signed-off-by: Hans Verkuil <hverkuil@xxxxxxxxx>
> >> ---
> >> Change since v2: set min_reqbufs_allocation to the same value
> >> as used for V4L2_CID_MIN_BUFFERS_FOR_OUTPUT/CAPTURE.
> >> Change since v1: V4L2_CID_MIN_BUFFERS_FOR_OUTPUT was already
> >> supported, so that patch led to duplicated controls. That's now
> >> fixed.
> >> ---
> >> .../media/test-drivers/vicodec/vicodec-core.c | 22 ++++++++++++++-----
> >> 1 file changed, 16 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/media/test-drivers/vicodec/vicodec-core.c b/drivers/media/test-drivers/vicodec/vicodec-core.c
> >> index 00c84a06f343..556ec2a3d411 100644
> >> --- a/drivers/media/test-drivers/vicodec/vicodec-core.c
> >> +++ b/drivers/media/test-drivers/vicodec/vicodec-core.c
> >> @@ -43,6 +43,8 @@ MODULE_PARM_DESC(debug, " activates debug info");
> >> #define MIN_WIDTH 640U
> >> #define MAX_HEIGHT 2160U
> >> #define MIN_HEIGHT 360U
> >> +/* Recommended number of buffers for the stateful codecs */
> >> +#define VICODEC_REC_BUFS 2
> >>
> >> #define dprintk(dev, fmt, arg...) \
> >> v4l2_dbg(1, debug, &dev->v4l2_dev, "%s: " fmt, __func__, ## arg)
> >> @@ -1705,12 +1707,14 @@ static int queue_init(void *priv, struct vb2_queue *src_vq,
> >> src_vq->ops = &vicodec_qops;
> >> src_vq->mem_ops = &vb2_vmalloc_memops;
> >> src_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> >> - if (ctx->is_enc)
> >> + if (ctx->is_enc) {
> >> src_vq->lock = &ctx->dev->stateful_enc.mutex;
> >> - else if (ctx->is_stateless)
> >> + src_vq->min_reqbufs_allocation = VICODEC_REC_BUFS;
> >
> > Doesn't this change affect also stateless codecs?
>
> No. The patch is a bit hard to read, the "else if (ctx->is_stateless)" is
> actually moved to after this new assignment.
>
> Also note that the encoder in vicodec is stateful only. There is no
> stateless encoder.
Thanks, that explains it.
Feel free to add:
Reviewed-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
>
> >
> >> + } else if (ctx->is_stateless) {
> >> src_vq->lock = &ctx->dev->stateless_dec.mutex;
> >> - else
> >> + } else {
> >> src_vq->lock = &ctx->dev->stateful_dec.mutex;
> >> + }
> >> src_vq->supports_requests = ctx->is_stateless;
> >> src_vq->requires_requests = ctx->is_stateless;
> >> ret = vb2_queue_init(src_vq);
> >> @@ -1728,6 +1732,8 @@ static int queue_init(void *priv, struct vb2_queue *src_vq,
> >> dst_vq->mem_ops = &vb2_vmalloc_memops;
> >> dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> >> dst_vq->lock = src_vq->lock;
> >> + if (!ctx->is_stateless && !ctx->is_enc)
> >> + dst_vq->min_reqbufs_allocation = VICODEC_REC_BUFS;
> >>
> >> return vb2_queue_init(dst_vq);
> >> }
> >> @@ -1852,9 +1858,13 @@ static int vicodec_open(struct file *file)
> >> 1, 31, 1, 20);
> >> v4l2_ctrl_new_std(hdl, &vicodec_ctrl_ops, V4L2_CID_FWHT_P_FRAME_QP,
> >> 1, 31, 1, 20);
> >> - if (ctx->is_enc)
> >> - v4l2_ctrl_new_std(hdl, &vicodec_ctrl_ops,
> >> - V4L2_CID_MIN_BUFFERS_FOR_OUTPUT, 1, 1, 1, 1);
> >> + if (!ctx->is_stateless)
> >> + v4l2_ctrl_new_std(hdl, &vicodec_ctrl_ops, ctx->is_enc ?
> >> + V4L2_CID_MIN_BUFFERS_FOR_OUTPUT :
> >> + V4L2_CID_MIN_BUFFERS_FOR_CAPTURE,
> >> + VICODEC_REC_BUFS, VICODEC_REC_BUFS, 1,
> >> + VICODEC_REC_BUFS);
> >> +
> >> if (ctx->is_stateless)
> >
> > This could be replaced by an else branch.
>
> Correct, I'll do that.
--
Regards,
Sakari Ailus
