Message ID | 3c0362e5-aa47-4545-a81e-e696b0e01440@xs4all.nl |
---|---|
State | Superseded |
Headers | show |
Series | [PATCHv3] media: vicodec: add V4L2_CID_MIN_BUFFERS_FOR_* controls | expand |
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@xs4all.nl> > --- > 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? > + } 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. > v4l2_ctrl_new_custom(hdl, &vicodec_ctrl_stateless_state, NULL); > if (hdl->error) {
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@xs4all.nl> >> --- >> 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. > >> + } 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, Hans > >> v4l2_ctrl_new_custom(hdl, &vicodec_ctrl_stateless_state, NULL); >> if (hdl->error) { >
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@xs4all.nl> > >> --- > >> 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@linux.intel.com> > > > > >> + } 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.
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; + } 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) v4l2_ctrl_new_custom(hdl, &vicodec_ctrl_stateless_state, NULL); if (hdl->error) {
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@xs4all.nl> --- 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(-)