diff mbox series

[PATCHv3] media: vicodec: add V4L2_CID_MIN_BUFFERS_FOR_* controls

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

Commit Message

Hans Verkuil Nov. 5, 2024, 7:50 a.m. UTC
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(-)

Comments

Sakari Ailus Nov. 8, 2024, 8:23 a.m. UTC | #1
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) {
Hans Verkuil Nov. 8, 2024, 8:40 a.m. UTC | #2
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) {
>
Sakari Ailus Nov. 8, 2024, 8:44 a.m. UTC | #3
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 mbox series

Patch

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) {