v4l2-subdev: Try formats are only available if subdev API is enabled
PATCH RESEND 1/1 v4l2-subdev: Try formats are only available if subdev API is enabled
Return an error for which == V4L2_SUBDEV_FORMAT_TRY if CONFIG_VIDEO_V4L2_SUBDEV_API is not enabled. This is because the try formats are not available in that case. Do the check here so that drivers don't need to.
Depends-on: ("media: v4l2-subdev: Verify v4l2_subdev_call() pad config argument")
--- drivers/media/v4l2-core/v4l2-subdev.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/media/v4l2-core/v4l2-subdev.c +4 -1
@@ -122,7 +122,10 @@ static int subdev_close(struct file *file)static inline int check_which (__u32 which) { -if (which !=V4L2_SUBDEV_FORMAT_TRY && + if ( +#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API + which !=V4L2_SUBDEV_FORMAT_TRY && +#endif which != V4L2_SUBDEV_FORMAT_ACTIVE) return -EINVAL;
Signature · 1 line
2.11.0
Hi Sakari,
Quoted 3 lines · CEST Sakari Ailus
Return an error for which ==
V4L2_SUBDEV_FORMAT_TRYifCONFIG_VIDEO_V4L2_SUBDEV_APIis not enabled. This is because the try formats are not available in that case.
As far as I can see, there are 21 V4L2 subdevice drivers which don't support V4L2 subdev API but implement V4L2_SUBDEV_FORMAT_TRY operation mode somehow. Someone already took that decision in the past and replaced video operation callbacks with pad operation callbacks supporting V4L2_SUBDEV_FORMAT_TRY without the requirement for V4L2 subdev API support.
With this change in place, those drivers will loose that functionality if built without CONFIG_VIDEO_V4L2_SUBDEV_API, but will happily work as before otherwise. That's probably not exactly what you intended.
Quoted 2 lines
Do the check here so that drivers
don't need to.
So only beneficiaries of this change will be V4L2 subdevice drivers which don't support V4L2 subdev API and don't support V4L2_SUBDEV_FORMAT_TRY. Existing drivers of that kind already have that check in place and I can hardly imagine someone is going to optimize them by removing it. Any new drivers will probably support V4L2 subdev API from the beginning so they'll not benefit from this patch at all as they won't build without CONFIG_VIDEO_V4L2_SUBDEV_API.
I think that if a driver doesn't support V4L2_SUBDEV_FORMAT_TRY, it should simply check for that value itself and respond accordingly, no matter if it supports V4L2 subdev API or not. As time passes, old drivers will be either depreciated or refreshed to handle V4L2_SUBDEV_FORMAT_TRY the V4L2 subdev API way, as we would like them to.
Thanks,
Janusz
Quoted 3 lines
Signed-off-by Sakari Ailus <[email protected]>Depends-on: ("media: v4l2-subdev: Verify
v4l2_subdev_call()pad config
argument")
drivers/media/v4l2-core/
Quoted 5 lines
--- drivers/media/v4l2-core/v4l2-subdev.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)drivers/media/v4l2-core/ renamed from drivers/media/v4l2-core/v4l2-subdev.c +0
-0
v4l2-subdev.c
Quoted 16 lines
index
34219e489be27..88b4b9d7c41be100644drivers/media/v4l2-core/v4l2-subdev.c +4
-1@@ -122,7 +122,10 @@ static int subdev_close(struct file *file)static inline int check_which (__u32 which) { -if (which !=V4L2_SUBDEV_FORMAT_TRY && + if ( +#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API + which !=V4L2_SUBDEV_FORMAT_TRY && +#endif which != V4L2_SUBDEV_FORMAT_ACTIVE) return -EINVAL;
Hi Janusz,
Quoted 13 lines · Janusz Krzysztofik
Hi Sakari,
Quoted 3 lines · CEST Sakari Ailus
Return an error for which ==
V4L2_SUBDEV_FORMAT_TRYifCONFIG_VIDEO_V4L2_SUBDEV_APIis not enabled. This is because the try formats are not available in that case.As far as I can see, there are 21 V4L2 subdevice drivers which don't support V4L2 subdev API but implement
V4L2_SUBDEV_FORMAT_TRYoperation mode somehow. Someone already took that decision in the past and replaced video operation callbacks with pad operation callbacks supportingV4L2_SUBDEV_FORMAT_TRYwithout the requirement for V4L2 subdev API support.
Quoted 3 lines
With this change in place, those drivers will loose that functionality if built without
CONFIG_VIDEO_V4L2_SUBDEV_API, but will happily work as before otherwise. That's probably not exactly what you intended.
Right. Indeed. The old format video ops were converted to use the pad ops, even for drivers that knew nothing about pads. When writing this patch, I missed the try_fmt was actually still supported the same way. Not many drivers use it, but it's still there.
I believe your earlier patchset almost covers the thinkable pitfalls that could arise from supporting both legacy subdev format and subder pad format semantics. I'll submit another patch, let's see if people can poke holes into that one. :-)
Quoted 51 lines
Quoted 3 lines
Do the check here so that drivers
don't need to.So only beneficiaries of this change will be V4L2 subdevice drivers which don't support V4L2 subdev API and don't support
V4L2_SUBDEV_FORMAT_TRY. Existing drivers of that kind already have that check in place and I can hardly imagine someone is going to optimize them by removing it. Any new drivers will probably support V4L2 subdev API from the beginning so they'll not benefit from this patch at all as they won't build withoutCONFIG_VIDEO_V4L2_SUBDEV_API.I think that if a driver doesn't support
V4L2_SUBDEV_FORMAT_TRY, it should simply check for that value itself and respond accordingly, no matter if it supports V4L2 subdev API or not. As time passes, old drivers will be either depreciated or refreshed to handleV4L2_SUBDEV_FORMAT_TRYthe V4L2 subdev API way, as we would like them to.Thanks,
JanuszQuoted 3 lines
Signed-off-by Sakari Ailus <[email protected]>Depends-on: ("media: v4l2-subdev: Verify
v4l2_subdev_call()pad configargument")
drivers/media/v4l2-core/
Quoted 5 lines
--- drivers/media/v4l2-core/v4l2-subdev.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)drivers/media/v4l2-core/ renamed from drivers/media/v4l2-core/v4l2-subdev.c +0
-0
v4l2-subdev.cQuoted 16 lines
index
34219e489be27..88b4b9d7c41be100644drivers/media/v4l2-core/v4l2-subdev.c +4
-1@@ -122,7 +122,10 @@ static int subdev_close(struct file *file)static inline int check_which (__u32 which) { -if (which !=V4L2_SUBDEV_FORMAT_TRY && + if ( +#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API + which !=V4L2_SUBDEV_FORMAT_TRY && +#endif which != V4L2_SUBDEV_FORMAT_ACTIVE) return -EINVAL;
Signature · 4 lines
Regards, Sakari Ailus [email protected]