* [PATCH] media: verisilicon: Explicitly disable all encoder ioctls for decoders @ 2025-08-26 19:04 ` Paul Kocialkowski 0 siblings, 0 replies; 8+ messages in thread From: Paul Kocialkowski @ 2025-08-26 19:04 UTC (permalink / raw) To: linux-media, linux-rockchip, linux-kernel Cc: Nicolas Dufresne, Benjamin Gaignard, Philipp Zabel, Mauro Carvalho Chehab, Paul Kocialkowski Call the dedicated v4l2_disable_ioctl helper instead of manually checking whether the current context is an encoder for the selection ioctls. Signed-off-by: Paul Kocialkowski <paulk@sys-base.io> --- drivers/media/platform/verisilicon/hantro_drv.c | 2 ++ drivers/media/platform/verisilicon/hantro_v4l2.c | 6 ++---- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c index 4cc9d00fd293..6fb28a6293e7 100644 --- a/drivers/media/platform/verisilicon/hantro_drv.c +++ b/drivers/media/platform/verisilicon/hantro_drv.c @@ -916,6 +916,8 @@ static int hantro_add_func(struct hantro_dev *vpu, unsigned int funcid) vpu->decoder = func; v4l2_disable_ioctl(vfd, VIDIOC_TRY_ENCODER_CMD); v4l2_disable_ioctl(vfd, VIDIOC_ENCODER_CMD); + v4l2_disable_ioctl(vfd, VIDIOC_G_SELECTION); + v4l2_disable_ioctl(vfd, VIDIOC_S_SELECTION); } video_set_drvdata(vfd, vpu); diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c index 6bcd892e7bb4..fcf3bd9bcda2 100644 --- a/drivers/media/platform/verisilicon/hantro_v4l2.c +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c @@ -663,8 +663,7 @@ static int vidioc_g_selection(struct file *file, void *priv, struct hantro_ctx *ctx = file_to_ctx(file); /* Crop only supported on source. */ - if (!ctx->is_encoder || - sel->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) + if (sel->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) return -EINVAL; switch (sel->target) { @@ -696,8 +695,7 @@ static int vidioc_s_selection(struct file *file, void *priv, struct vb2_queue *vq; /* Crop only supported on source. */ - if (!ctx->is_encoder || - sel->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) + if (sel->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) return -EINVAL; /* Change not allowed if the queue is streaming. */ -- 2.50.1 _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH] media: verisilicon: Explicitly disable all encoder ioctls for decoders @ 2025-08-26 19:04 ` Paul Kocialkowski 0 siblings, 0 replies; 8+ messages in thread From: Paul Kocialkowski @ 2025-08-26 19:04 UTC (permalink / raw) To: linux-media, linux-rockchip, linux-kernel Cc: Nicolas Dufresne, Benjamin Gaignard, Philipp Zabel, Mauro Carvalho Chehab, Paul Kocialkowski Call the dedicated v4l2_disable_ioctl helper instead of manually checking whether the current context is an encoder for the selection ioctls. Signed-off-by: Paul Kocialkowski <paulk@sys-base.io> --- drivers/media/platform/verisilicon/hantro_drv.c | 2 ++ drivers/media/platform/verisilicon/hantro_v4l2.c | 6 ++---- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c index 4cc9d00fd293..6fb28a6293e7 100644 --- a/drivers/media/platform/verisilicon/hantro_drv.c +++ b/drivers/media/platform/verisilicon/hantro_drv.c @@ -916,6 +916,8 @@ static int hantro_add_func(struct hantro_dev *vpu, unsigned int funcid) vpu->decoder = func; v4l2_disable_ioctl(vfd, VIDIOC_TRY_ENCODER_CMD); v4l2_disable_ioctl(vfd, VIDIOC_ENCODER_CMD); + v4l2_disable_ioctl(vfd, VIDIOC_G_SELECTION); + v4l2_disable_ioctl(vfd, VIDIOC_S_SELECTION); } video_set_drvdata(vfd, vpu); diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c index 6bcd892e7bb4..fcf3bd9bcda2 100644 --- a/drivers/media/platform/verisilicon/hantro_v4l2.c +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c @@ -663,8 +663,7 @@ static int vidioc_g_selection(struct file *file, void *priv, struct hantro_ctx *ctx = file_to_ctx(file); /* Crop only supported on source. */ - if (!ctx->is_encoder || - sel->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) + if (sel->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) return -EINVAL; switch (sel->target) { @@ -696,8 +695,7 @@ static int vidioc_s_selection(struct file *file, void *priv, struct vb2_queue *vq; /* Crop only supported on source. */ - if (!ctx->is_encoder || - sel->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) + if (sel->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) return -EINVAL; /* Change not allowed if the queue is streaming. */ -- 2.50.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] media: verisilicon: Explicitly disable all encoder ioctls for decoders 2025-08-26 19:04 ` Paul Kocialkowski @ 2025-08-27 14:30 ` Nicolas Dufresne -1 siblings, 0 replies; 8+ messages in thread From: Nicolas Dufresne @ 2025-08-27 14:30 UTC (permalink / raw) To: Paul Kocialkowski, linux-media, linux-rockchip, linux-kernel Cc: Benjamin Gaignard, Philipp Zabel, Mauro Carvalho Chehab [-- Attachment #1.1: Type: text/plain, Size: 2569 bytes --] Hi Paul, Le mardi 26 août 2025 à 21:04 +0200, Paul Kocialkowski a écrit : > Call the dedicated v4l2_disable_ioctl helper instead of manually > checking whether the current context is an encoder for the selection > ioctls. > > Signed-off-by: Paul Kocialkowski <paulk@sys-base.io> > --- > drivers/media/platform/verisilicon/hantro_drv.c | 2 ++ > drivers/media/platform/verisilicon/hantro_v4l2.c | 6 ++---- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/platform/verisilicon/hantro_drv.c > b/drivers/media/platform/verisilicon/hantro_drv.c > index 4cc9d00fd293..6fb28a6293e7 100644 > --- a/drivers/media/platform/verisilicon/hantro_drv.c > +++ b/drivers/media/platform/verisilicon/hantro_drv.c > @@ -916,6 +916,8 @@ static int hantro_add_func(struct hantro_dev *vpu, > unsigned int funcid) > vpu->decoder = func; > v4l2_disable_ioctl(vfd, VIDIOC_TRY_ENCODER_CMD); > v4l2_disable_ioctl(vfd, VIDIOC_ENCODER_CMD); > + v4l2_disable_ioctl(vfd, VIDIOC_G_SELECTION); > + v4l2_disable_ioctl(vfd, VIDIOC_S_SELECTION); Disabling this IOCTL for JPEG is fine, but for VP8, H.264, HEVC, VP9 and AV1, it is pretty much mandatory. Otherwise your stream will advertise the padded dimentions and there would be no way to tell it that what is the cropping window for bitstream generation purpose. Considering you are looking forward adding H.264 encoding, do you really want to apply this fix ? Nicolas > } > > video_set_drvdata(vfd, vpu); > diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c > b/drivers/media/platform/verisilicon/hantro_v4l2.c > index 6bcd892e7bb4..fcf3bd9bcda2 100644 > --- a/drivers/media/platform/verisilicon/hantro_v4l2.c > +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c > @@ -663,8 +663,7 @@ static int vidioc_g_selection(struct file *file, void > *priv, > struct hantro_ctx *ctx = file_to_ctx(file); > > /* Crop only supported on source. */ > - if (!ctx->is_encoder || > - sel->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) > + if (sel->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) > return -EINVAL; > > switch (sel->target) { > @@ -696,8 +695,7 @@ static int vidioc_s_selection(struct file *file, void > *priv, > struct vb2_queue *vq; > > /* Crop only supported on source. */ > - if (!ctx->is_encoder || > - sel->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) > + if (sel->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) > return -EINVAL; > > /* Change not allowed if the queue is streaming. */ [-- Attachment #1.2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 228 bytes --] [-- Attachment #2: Type: text/plain, Size: 170 bytes --] _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] media: verisilicon: Explicitly disable all encoder ioctls for decoders @ 2025-08-27 14:30 ` Nicolas Dufresne 0 siblings, 0 replies; 8+ messages in thread From: Nicolas Dufresne @ 2025-08-27 14:30 UTC (permalink / raw) To: Paul Kocialkowski, linux-media, linux-rockchip, linux-kernel Cc: Benjamin Gaignard, Philipp Zabel, Mauro Carvalho Chehab [-- Attachment #1: Type: text/plain, Size: 2569 bytes --] Hi Paul, Le mardi 26 août 2025 à 21:04 +0200, Paul Kocialkowski a écrit : > Call the dedicated v4l2_disable_ioctl helper instead of manually > checking whether the current context is an encoder for the selection > ioctls. > > Signed-off-by: Paul Kocialkowski <paulk@sys-base.io> > --- > drivers/media/platform/verisilicon/hantro_drv.c | 2 ++ > drivers/media/platform/verisilicon/hantro_v4l2.c | 6 ++---- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/platform/verisilicon/hantro_drv.c > b/drivers/media/platform/verisilicon/hantro_drv.c > index 4cc9d00fd293..6fb28a6293e7 100644 > --- a/drivers/media/platform/verisilicon/hantro_drv.c > +++ b/drivers/media/platform/verisilicon/hantro_drv.c > @@ -916,6 +916,8 @@ static int hantro_add_func(struct hantro_dev *vpu, > unsigned int funcid) > vpu->decoder = func; > v4l2_disable_ioctl(vfd, VIDIOC_TRY_ENCODER_CMD); > v4l2_disable_ioctl(vfd, VIDIOC_ENCODER_CMD); > + v4l2_disable_ioctl(vfd, VIDIOC_G_SELECTION); > + v4l2_disable_ioctl(vfd, VIDIOC_S_SELECTION); Disabling this IOCTL for JPEG is fine, but for VP8, H.264, HEVC, VP9 and AV1, it is pretty much mandatory. Otherwise your stream will advertise the padded dimentions and there would be no way to tell it that what is the cropping window for bitstream generation purpose. Considering you are looking forward adding H.264 encoding, do you really want to apply this fix ? Nicolas > } > > video_set_drvdata(vfd, vpu); > diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c > b/drivers/media/platform/verisilicon/hantro_v4l2.c > index 6bcd892e7bb4..fcf3bd9bcda2 100644 > --- a/drivers/media/platform/verisilicon/hantro_v4l2.c > +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c > @@ -663,8 +663,7 @@ static int vidioc_g_selection(struct file *file, void > *priv, > struct hantro_ctx *ctx = file_to_ctx(file); > > /* Crop only supported on source. */ > - if (!ctx->is_encoder || > - sel->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) > + if (sel->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) > return -EINVAL; > > switch (sel->target) { > @@ -696,8 +695,7 @@ static int vidioc_s_selection(struct file *file, void > *priv, > struct vb2_queue *vq; > > /* Crop only supported on source. */ > - if (!ctx->is_encoder || > - sel->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) > + if (sel->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) > return -EINVAL; > > /* Change not allowed if the queue is streaming. */ [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] media: verisilicon: Explicitly disable all encoder ioctls for decoders 2025-08-27 14:30 ` Nicolas Dufresne @ 2025-08-27 16:11 ` Paul Kocialkowski -1 siblings, 0 replies; 8+ messages in thread From: Paul Kocialkowski @ 2025-08-27 16:11 UTC (permalink / raw) To: Nicolas Dufresne Cc: linux-media, linux-rockchip, linux-kernel, Benjamin Gaignard, Philipp Zabel, Mauro Carvalho Chehab [-- Attachment #1.1: Type: text/plain, Size: 3382 bytes --] Hi Nicolas, On Wed 27 Aug 25, 10:30, Nicolas Dufresne wrote: > Hi Paul, > > Le mardi 26 août 2025 à 21:04 +0200, Paul Kocialkowski a écrit : > > Call the dedicated v4l2_disable_ioctl helper instead of manually > > checking whether the current context is an encoder for the selection > > ioctls. > > > > Signed-off-by: Paul Kocialkowski <paulk@sys-base.io> > > --- > > drivers/media/platform/verisilicon/hantro_drv.c | 2 ++ > > drivers/media/platform/verisilicon/hantro_v4l2.c | 6 ++---- > > 2 files changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/media/platform/verisilicon/hantro_drv.c > > b/drivers/media/platform/verisilicon/hantro_drv.c > > index 4cc9d00fd293..6fb28a6293e7 100644 > > --- a/drivers/media/platform/verisilicon/hantro_drv.c > > +++ b/drivers/media/platform/verisilicon/hantro_drv.c > > @@ -916,6 +916,8 @@ static int hantro_add_func(struct hantro_dev *vpu, > > unsigned int funcid) > > vpu->decoder = func; > > v4l2_disable_ioctl(vfd, VIDIOC_TRY_ENCODER_CMD); > > v4l2_disable_ioctl(vfd, VIDIOC_ENCODER_CMD); > > + v4l2_disable_ioctl(vfd, VIDIOC_G_SELECTION); > > + v4l2_disable_ioctl(vfd, VIDIOC_S_SELECTION); > > Disabling this IOCTL for JPEG is fine, but for VP8, H.264, HEVC, VP9 and AV1, it > is pretty much mandatory. Otherwise your stream will advertise the padded > dimentions and there would be no way to tell it that what is the cropping window > for bitstream generation purpose. Maybe the lack of context around the patch doesn't make it clear, but this is to disable the ioctls for decoders (not encoders), which don't need to use the selection API. This keeps the ioctls enabled and available for all encoders. > Considering you are looking forward adding H.264 encoding, do you really want > to apply this fix ? I am well aware that this is required to setup the crop in the coded bitstream and I am definitely using it in my encoding work :) Cheers, Paul > > Nicolas > > > } > > > > video_set_drvdata(vfd, vpu); > > diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c > > b/drivers/media/platform/verisilicon/hantro_v4l2.c > > index 6bcd892e7bb4..fcf3bd9bcda2 100644 > > --- a/drivers/media/platform/verisilicon/hantro_v4l2.c > > +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c > > @@ -663,8 +663,7 @@ static int vidioc_g_selection(struct file *file, void > > *priv, > > struct hantro_ctx *ctx = file_to_ctx(file); > > > > /* Crop only supported on source. */ > > - if (!ctx->is_encoder || > > - sel->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) > > + if (sel->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) > > return -EINVAL; > > > > switch (sel->target) { > > @@ -696,8 +695,7 @@ static int vidioc_s_selection(struct file *file, void > > *priv, > > struct vb2_queue *vq; > > > > /* Crop only supported on source. */ > > - if (!ctx->is_encoder || > > - sel->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) > > + if (sel->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) > > return -EINVAL; > > > > /* Change not allowed if the queue is streaming. */ -- Paul Kocialkowski, Independent contractor - sys-base - https://www.sys-base.io/ Free software developer - https://www.paulk.fr/ Expert in multimedia, graphics and embedded hardware support with Linux. [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 170 bytes --] _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] media: verisilicon: Explicitly disable all encoder ioctls for decoders @ 2025-08-27 16:11 ` Paul Kocialkowski 0 siblings, 0 replies; 8+ messages in thread From: Paul Kocialkowski @ 2025-08-27 16:11 UTC (permalink / raw) To: Nicolas Dufresne Cc: linux-media, linux-rockchip, linux-kernel, Benjamin Gaignard, Philipp Zabel, Mauro Carvalho Chehab [-- Attachment #1: Type: text/plain, Size: 3382 bytes --] Hi Nicolas, On Wed 27 Aug 25, 10:30, Nicolas Dufresne wrote: > Hi Paul, > > Le mardi 26 août 2025 à 21:04 +0200, Paul Kocialkowski a écrit : > > Call the dedicated v4l2_disable_ioctl helper instead of manually > > checking whether the current context is an encoder for the selection > > ioctls. > > > > Signed-off-by: Paul Kocialkowski <paulk@sys-base.io> > > --- > > drivers/media/platform/verisilicon/hantro_drv.c | 2 ++ > > drivers/media/platform/verisilicon/hantro_v4l2.c | 6 ++---- > > 2 files changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/media/platform/verisilicon/hantro_drv.c > > b/drivers/media/platform/verisilicon/hantro_drv.c > > index 4cc9d00fd293..6fb28a6293e7 100644 > > --- a/drivers/media/platform/verisilicon/hantro_drv.c > > +++ b/drivers/media/platform/verisilicon/hantro_drv.c > > @@ -916,6 +916,8 @@ static int hantro_add_func(struct hantro_dev *vpu, > > unsigned int funcid) > > vpu->decoder = func; > > v4l2_disable_ioctl(vfd, VIDIOC_TRY_ENCODER_CMD); > > v4l2_disable_ioctl(vfd, VIDIOC_ENCODER_CMD); > > + v4l2_disable_ioctl(vfd, VIDIOC_G_SELECTION); > > + v4l2_disable_ioctl(vfd, VIDIOC_S_SELECTION); > > Disabling this IOCTL for JPEG is fine, but for VP8, H.264, HEVC, VP9 and AV1, it > is pretty much mandatory. Otherwise your stream will advertise the padded > dimentions and there would be no way to tell it that what is the cropping window > for bitstream generation purpose. Maybe the lack of context around the patch doesn't make it clear, but this is to disable the ioctls for decoders (not encoders), which don't need to use the selection API. This keeps the ioctls enabled and available for all encoders. > Considering you are looking forward adding H.264 encoding, do you really want > to apply this fix ? I am well aware that this is required to setup the crop in the coded bitstream and I am definitely using it in my encoding work :) Cheers, Paul > > Nicolas > > > } > > > > video_set_drvdata(vfd, vpu); > > diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c > > b/drivers/media/platform/verisilicon/hantro_v4l2.c > > index 6bcd892e7bb4..fcf3bd9bcda2 100644 > > --- a/drivers/media/platform/verisilicon/hantro_v4l2.c > > +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c > > @@ -663,8 +663,7 @@ static int vidioc_g_selection(struct file *file, void > > *priv, > > struct hantro_ctx *ctx = file_to_ctx(file); > > > > /* Crop only supported on source. */ > > - if (!ctx->is_encoder || > > - sel->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) > > + if (sel->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) > > return -EINVAL; > > > > switch (sel->target) { > > @@ -696,8 +695,7 @@ static int vidioc_s_selection(struct file *file, void > > *priv, > > struct vb2_queue *vq; > > > > /* Crop only supported on source. */ > > - if (!ctx->is_encoder || > > - sel->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) > > + if (sel->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) > > return -EINVAL; > > > > /* Change not allowed if the queue is streaming. */ -- Paul Kocialkowski, Independent contractor - sys-base - https://www.sys-base.io/ Free software developer - https://www.paulk.fr/ Expert in multimedia, graphics and embedded hardware support with Linux. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] media: verisilicon: Explicitly disable all encoder ioctls for decoders 2025-08-27 16:11 ` Paul Kocialkowski @ 2025-08-27 17:47 ` Nicolas Dufresne -1 siblings, 0 replies; 8+ messages in thread From: Nicolas Dufresne @ 2025-08-27 17:47 UTC (permalink / raw) To: Paul Kocialkowski Cc: linux-media, linux-rockchip, linux-kernel, Benjamin Gaignard, Philipp Zabel, Mauro Carvalho Chehab [-- Attachment #1.1: Type: text/plain, Size: 3705 bytes --] Le mercredi 27 août 2025 à 18:11 +0200, Paul Kocialkowski a écrit : > Hi Nicolas, > > On Wed 27 Aug 25, 10:30, Nicolas Dufresne wrote: > > Hi Paul, > > > > Le mardi 26 août 2025 à 21:04 +0200, Paul Kocialkowski a écrit : > > > Call the dedicated v4l2_disable_ioctl helper instead of manually > > > checking whether the current context is an encoder for the selection > > > ioctls. > > > > > > Signed-off-by: Paul Kocialkowski <paulk@sys-base.io> > > > --- > > > drivers/media/platform/verisilicon/hantro_drv.c | 2 ++ > > > drivers/media/platform/verisilicon/hantro_v4l2.c | 6 ++---- > > > 2 files changed, 4 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/media/platform/verisilicon/hantro_drv.c > > > b/drivers/media/platform/verisilicon/hantro_drv.c > > > index 4cc9d00fd293..6fb28a6293e7 100644 > > > --- a/drivers/media/platform/verisilicon/hantro_drv.c > > > +++ b/drivers/media/platform/verisilicon/hantro_drv.c > > > @@ -916,6 +916,8 @@ static int hantro_add_func(struct hantro_dev *vpu, > > > unsigned int funcid) > > > vpu->decoder = func; > > > v4l2_disable_ioctl(vfd, VIDIOC_TRY_ENCODER_CMD); > > > v4l2_disable_ioctl(vfd, VIDIOC_ENCODER_CMD); > > > + v4l2_disable_ioctl(vfd, VIDIOC_G_SELECTION); > > > + v4l2_disable_ioctl(vfd, VIDIOC_S_SELECTION); > > > > Disabling this IOCTL for JPEG is fine, but for VP8, H.264, HEVC, VP9 and > > AV1, it > > is pretty much mandatory. Otherwise your stream will advertise the padded > > dimentions and there would be no way to tell it that what is the cropping > > window > > for bitstream generation purpose. > > Maybe the lack of context around the patch doesn't make it clear, but this is > to disable the ioctls for decoders (not encoders), which don't need to use the > selection API. This keeps the ioctls enabled and available for all encoders. My bad, miss-read. I think you could reduce the confusion with a subject line that is narrower: media: verisilicon: Explicitly disable SELECTION API ioctl for decoders And you can add: Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> cheers, Nicolas > > > Considering you are looking forward adding H.264 encoding, do you really > > want > > to apply this fix ? > > I am well aware that this is required to setup the crop in the coded bitstream > and I am definitely using it in my encoding work :) > > Cheers, > > Paul > > > > > Nicolas > > > > > } > > > > > > video_set_drvdata(vfd, vpu); > > > diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c > > > b/drivers/media/platform/verisilicon/hantro_v4l2.c > > > index 6bcd892e7bb4..fcf3bd9bcda2 100644 > > > --- a/drivers/media/platform/verisilicon/hantro_v4l2.c > > > +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c > > > @@ -663,8 +663,7 @@ static int vidioc_g_selection(struct file *file, void > > > *priv, > > > struct hantro_ctx *ctx = file_to_ctx(file); > > > > > > /* Crop only supported on source. */ > > > - if (!ctx->is_encoder || > > > - sel->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) > > > + if (sel->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) > > > return -EINVAL; > > > > > > switch (sel->target) { > > > @@ -696,8 +695,7 @@ static int vidioc_s_selection(struct file *file, void > > > *priv, > > > struct vb2_queue *vq; > > > > > > /* Crop only supported on source. */ > > > - if (!ctx->is_encoder || > > > - sel->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) > > > + if (sel->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) > > > return -EINVAL; > > > > > > /* Change not allowed if the queue is streaming. */ > > [-- Attachment #1.2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 228 bytes --] [-- Attachment #2: Type: text/plain, Size: 170 bytes --] _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] media: verisilicon: Explicitly disable all encoder ioctls for decoders @ 2025-08-27 17:47 ` Nicolas Dufresne 0 siblings, 0 replies; 8+ messages in thread From: Nicolas Dufresne @ 2025-08-27 17:47 UTC (permalink / raw) To: Paul Kocialkowski Cc: linux-media, linux-rockchip, linux-kernel, Benjamin Gaignard, Philipp Zabel, Mauro Carvalho Chehab [-- Attachment #1: Type: text/plain, Size: 3705 bytes --] Le mercredi 27 août 2025 à 18:11 +0200, Paul Kocialkowski a écrit : > Hi Nicolas, > > On Wed 27 Aug 25, 10:30, Nicolas Dufresne wrote: > > Hi Paul, > > > > Le mardi 26 août 2025 à 21:04 +0200, Paul Kocialkowski a écrit : > > > Call the dedicated v4l2_disable_ioctl helper instead of manually > > > checking whether the current context is an encoder for the selection > > > ioctls. > > > > > > Signed-off-by: Paul Kocialkowski <paulk@sys-base.io> > > > --- > > > drivers/media/platform/verisilicon/hantro_drv.c | 2 ++ > > > drivers/media/platform/verisilicon/hantro_v4l2.c | 6 ++---- > > > 2 files changed, 4 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/media/platform/verisilicon/hantro_drv.c > > > b/drivers/media/platform/verisilicon/hantro_drv.c > > > index 4cc9d00fd293..6fb28a6293e7 100644 > > > --- a/drivers/media/platform/verisilicon/hantro_drv.c > > > +++ b/drivers/media/platform/verisilicon/hantro_drv.c > > > @@ -916,6 +916,8 @@ static int hantro_add_func(struct hantro_dev *vpu, > > > unsigned int funcid) > > > vpu->decoder = func; > > > v4l2_disable_ioctl(vfd, VIDIOC_TRY_ENCODER_CMD); > > > v4l2_disable_ioctl(vfd, VIDIOC_ENCODER_CMD); > > > + v4l2_disable_ioctl(vfd, VIDIOC_G_SELECTION); > > > + v4l2_disable_ioctl(vfd, VIDIOC_S_SELECTION); > > > > Disabling this IOCTL for JPEG is fine, but for VP8, H.264, HEVC, VP9 and > > AV1, it > > is pretty much mandatory. Otherwise your stream will advertise the padded > > dimentions and there would be no way to tell it that what is the cropping > > window > > for bitstream generation purpose. > > Maybe the lack of context around the patch doesn't make it clear, but this is > to disable the ioctls for decoders (not encoders), which don't need to use the > selection API. This keeps the ioctls enabled and available for all encoders. My bad, miss-read. I think you could reduce the confusion with a subject line that is narrower: media: verisilicon: Explicitly disable SELECTION API ioctl for decoders And you can add: Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> cheers, Nicolas > > > Considering you are looking forward adding H.264 encoding, do you really > > want > > to apply this fix ? > > I am well aware that this is required to setup the crop in the coded bitstream > and I am definitely using it in my encoding work :) > > Cheers, > > Paul > > > > > Nicolas > > > > > } > > > > > > video_set_drvdata(vfd, vpu); > > > diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c > > > b/drivers/media/platform/verisilicon/hantro_v4l2.c > > > index 6bcd892e7bb4..fcf3bd9bcda2 100644 > > > --- a/drivers/media/platform/verisilicon/hantro_v4l2.c > > > +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c > > > @@ -663,8 +663,7 @@ static int vidioc_g_selection(struct file *file, void > > > *priv, > > > struct hantro_ctx *ctx = file_to_ctx(file); > > > > > > /* Crop only supported on source. */ > > > - if (!ctx->is_encoder || > > > - sel->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) > > > + if (sel->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) > > > return -EINVAL; > > > > > > switch (sel->target) { > > > @@ -696,8 +695,7 @@ static int vidioc_s_selection(struct file *file, void > > > *priv, > > > struct vb2_queue *vq; > > > > > > /* Crop only supported on source. */ > > > - if (!ctx->is_encoder || > > > - sel->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) > > > + if (sel->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) > > > return -EINVAL; > > > > > > /* Change not allowed if the queue is streaming. */ > > [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2025-08-27 19:37 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-08-26 19:04 [PATCH] media: verisilicon: Explicitly disable all encoder ioctls for decoders Paul Kocialkowski 2025-08-26 19:04 ` Paul Kocialkowski 2025-08-27 14:30 ` Nicolas Dufresne 2025-08-27 14:30 ` Nicolas Dufresne 2025-08-27 16:11 ` Paul Kocialkowski 2025-08-27 16:11 ` Paul Kocialkowski 2025-08-27 17:47 ` Nicolas Dufresne 2025-08-27 17:47 ` Nicolas Dufresne
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.