diff mbox series

[3/4] media: verisilicon: Hook the (TRY_)DECODER_CMD stateless ioctls

Message ID 20231109201640.340556-3-paul.kocialkowski@bootlin.com (mailing list archive)
State New, archived
Headers show
Series [1/4] media: vicodec: Disable (TRY_)DECODER_CMD for the stateless case | expand

Commit Message

Paul Kocialkowski Nov. 9, 2023, 8:16 p.m. UTC
The (TRY_)DECODER_CMD ioctls are used to support flushing when holding capture
buffers is supported. This is the case of this driver but the ioctls were never
hooked to the ioctl ops.

Add them to correctly support flushing.

Fixes: 340ce50f75a6 ("media: hantro: Enable HOLD_CAPTURE_BUF for H.264")
Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 drivers/media/platform/verisilicon/hantro_drv.c  | 2 ++
 drivers/media/platform/verisilicon/hantro_v4l2.c | 3 +++
 2 files changed, 5 insertions(+)

Comments

Nicolas Dufresne Nov. 9, 2023, 9:38 p.m. UTC | #1
Hi Paul,


Le jeudi 09 novembre 2023 à 21:16 +0100, Paul Kocialkowski a écrit :
> The (TRY_)DECODER_CMD ioctls are used to support flushing when holding capture
> buffers is supported. This is the case of this driver but the ioctls were never
> hooked to the ioctl ops.
> 
> Add them to correctly support flushing.

While this CMD looks useful on paper, we have had no use for it in any
open source user space. So with this in mind, shall we really enable it
? How can we be sure that its actually working as intended ?

Nicolas

> 
> Fixes: 340ce50f75a6 ("media: hantro: Enable HOLD_CAPTURE_BUF for H.264")
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> ---
>  drivers/media/platform/verisilicon/hantro_drv.c  | 2 ++
>  drivers/media/platform/verisilicon/hantro_v4l2.c | 3 +++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
> index a9fa05ac56a9..3a2a0f28cbfe 100644
> --- a/drivers/media/platform/verisilicon/hantro_drv.c
> +++ b/drivers/media/platform/verisilicon/hantro_drv.c
> @@ -905,6 +905,8 @@ static int hantro_add_func(struct hantro_dev *vpu, unsigned int funcid)
>  
>  	if (funcid == MEDIA_ENT_F_PROC_VIDEO_ENCODER) {
>  		vpu->encoder = func;
> +		v4l2_disable_ioctl(vfd, VIDIOC_TRY_DECODER_CMD);
> +		v4l2_disable_ioctl(vfd, VIDIOC_DECODER_CMD);
>  	} else {
>  		vpu->decoder = func;
>  		v4l2_disable_ioctl(vfd, VIDIOC_TRY_ENCODER_CMD);
> diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c
> index b3ae037a50f6..db145519fc5d 100644
> --- a/drivers/media/platform/verisilicon/hantro_v4l2.c
> +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c
> @@ -785,6 +785,9 @@ const struct v4l2_ioctl_ops hantro_ioctl_ops = {
>  	.vidioc_g_selection = vidioc_g_selection,
>  	.vidioc_s_selection = vidioc_s_selection,
>  
> +	.vidioc_decoder_cmd = v4l2_m2m_ioctl_stateless_decoder_cmd,
> +	.vidioc_try_decoder_cmd = v4l2_m2m_ioctl_stateless_try_decoder_cmd,
> +
>  	.vidioc_try_encoder_cmd = v4l2_m2m_ioctl_try_encoder_cmd,
>  	.vidioc_encoder_cmd = vidioc_encoder_cmd,
>  };
Paul Kocialkowski Nov. 10, 2023, 8:33 a.m. UTC | #2
Hi Nicolas,

On Thu 09 Nov 23, 16:38, Nicolas Dufresne wrote:
> Le jeudi 09 novembre 2023 à 21:16 +0100, Paul Kocialkowski a écrit :
> > The (TRY_)DECODER_CMD ioctls are used to support flushing when holding capture
> > buffers is supported. This is the case of this driver but the ioctls were never
> > hooked to the ioctl ops.
> > 
> > Add them to correctly support flushing.
> 
> While this CMD looks useful on paper, we have had no use for it in any
> open source user space. So with this in mind, shall we really enable it
> ? How can we be sure that its actually working as intended ?

Well I think it's part of the spec that it needs to be supportd.

I must admit I have never used it in a real use case either, but it can be fixed
later (without changing the uAPI) if the implementation turns out to be
malfunctioning.

Cheers,

Paul

> > Fixes: 340ce50f75a6 ("media: hantro: Enable HOLD_CAPTURE_BUF for H.264")
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > ---
> >  drivers/media/platform/verisilicon/hantro_drv.c  | 2 ++
> >  drivers/media/platform/verisilicon/hantro_v4l2.c | 3 +++
> >  2 files changed, 5 insertions(+)
> > 
> > diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
> > index a9fa05ac56a9..3a2a0f28cbfe 100644
> > --- a/drivers/media/platform/verisilicon/hantro_drv.c
> > +++ b/drivers/media/platform/verisilicon/hantro_drv.c
> > @@ -905,6 +905,8 @@ static int hantro_add_func(struct hantro_dev *vpu, unsigned int funcid)
> >  
> >  	if (funcid == MEDIA_ENT_F_PROC_VIDEO_ENCODER) {
> >  		vpu->encoder = func;
> > +		v4l2_disable_ioctl(vfd, VIDIOC_TRY_DECODER_CMD);
> > +		v4l2_disable_ioctl(vfd, VIDIOC_DECODER_CMD);
> >  	} else {
> >  		vpu->decoder = func;
> >  		v4l2_disable_ioctl(vfd, VIDIOC_TRY_ENCODER_CMD);
> > diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c
> > index b3ae037a50f6..db145519fc5d 100644
> > --- a/drivers/media/platform/verisilicon/hantro_v4l2.c
> > +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c
> > @@ -785,6 +785,9 @@ const struct v4l2_ioctl_ops hantro_ioctl_ops = {
> >  	.vidioc_g_selection = vidioc_g_selection,
> >  	.vidioc_s_selection = vidioc_s_selection,
> >  
> > +	.vidioc_decoder_cmd = v4l2_m2m_ioctl_stateless_decoder_cmd,
> > +	.vidioc_try_decoder_cmd = v4l2_m2m_ioctl_stateless_try_decoder_cmd,
> > +
> >  	.vidioc_try_encoder_cmd = v4l2_m2m_ioctl_try_encoder_cmd,
> >  	.vidioc_encoder_cmd = vidioc_encoder_cmd,
> >  };
>
Daniel Almeida Nov. 10, 2023, 9:07 p.m. UTC | #3
On Thursday, November 09, 2023 17:16 -03, Paul Kocialkowski <paul.kocialkowski@bootlin.com> wrote:

> The (TRY_)DECODER_CMD ioctls are used to support flushing when holding capture
> buffers is supported. This is the case of this driver but the ioctls were never
> hooked to the ioctl ops.
> 
> Add them to correctly support flushing.
> 
> Fixes: 340ce50f75a6 ("media: hantro: Enable HOLD_CAPTURE_BUF for H.264")
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> ---
>  drivers/media/platform/verisilicon/hantro_drv.c  | 2 ++
>  drivers/media/platform/verisilicon/hantro_v4l2.c | 3 +++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
> index a9fa05ac56a9..3a2a0f28cbfe 100644
> --- a/drivers/media/platform/verisilicon/hantro_drv.c
> +++ b/drivers/media/platform/verisilicon/hantro_drv.c
> @@ -905,6 +905,8 @@ static int hantro_add_func(struct hantro_dev *vpu, unsigned int funcid)
>  
>  	if (funcid == MEDIA_ENT_F_PROC_VIDEO_ENCODER) {
>  		vpu->encoder = func;
> +		v4l2_disable_ioctl(vfd, VIDIOC_TRY_DECODER_CMD);
> +		v4l2_disable_ioctl(vfd, VIDIOC_DECODER_CMD);
>  	} else {
>  		vpu->decoder = func;
>  		v4l2_disable_ioctl(vfd, VIDIOC_TRY_ENCODER_CMD);
> diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c
> index b3ae037a50f6..db145519fc5d 100644
> --- a/drivers/media/platform/verisilicon/hantro_v4l2.c
> +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c
> @@ -785,6 +785,9 @@ const struct v4l2_ioctl_ops hantro_ioctl_ops = {
>  	.vidioc_g_selection = vidioc_g_selection,
>  	.vidioc_s_selection = vidioc_s_selection,
>  
> +	.vidioc_decoder_cmd = v4l2_m2m_ioctl_stateless_decoder_cmd,
> +	.vidioc_try_decoder_cmd = v4l2_m2m_ioctl_stateless_try_decoder_cmd,
> +
>  	.vidioc_try_encoder_cmd = v4l2_m2m_ioctl_try_encoder_cmd,
>  	.vidioc_encoder_cmd = vidioc_encoder_cmd,
>  };
> -- 
> 2.42.1
> 
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>
diff mbox series

Patch

diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
index a9fa05ac56a9..3a2a0f28cbfe 100644
--- a/drivers/media/platform/verisilicon/hantro_drv.c
+++ b/drivers/media/platform/verisilicon/hantro_drv.c
@@ -905,6 +905,8 @@  static int hantro_add_func(struct hantro_dev *vpu, unsigned int funcid)
 
 	if (funcid == MEDIA_ENT_F_PROC_VIDEO_ENCODER) {
 		vpu->encoder = func;
+		v4l2_disable_ioctl(vfd, VIDIOC_TRY_DECODER_CMD);
+		v4l2_disable_ioctl(vfd, VIDIOC_DECODER_CMD);
 	} else {
 		vpu->decoder = func;
 		v4l2_disable_ioctl(vfd, VIDIOC_TRY_ENCODER_CMD);
diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c
index b3ae037a50f6..db145519fc5d 100644
--- a/drivers/media/platform/verisilicon/hantro_v4l2.c
+++ b/drivers/media/platform/verisilicon/hantro_v4l2.c
@@ -785,6 +785,9 @@  const struct v4l2_ioctl_ops hantro_ioctl_ops = {
 	.vidioc_g_selection = vidioc_g_selection,
 	.vidioc_s_selection = vidioc_s_selection,
 
+	.vidioc_decoder_cmd = v4l2_m2m_ioctl_stateless_decoder_cmd,
+	.vidioc_try_decoder_cmd = v4l2_m2m_ioctl_stateless_try_decoder_cmd,
+
 	.vidioc_try_encoder_cmd = v4l2_m2m_ioctl_try_encoder_cmd,
 	.vidioc_encoder_cmd = vidioc_encoder_cmd,
 };