Re: [RESEND PATCH 1/1] v4l2-subdev: Try formats are only available if subdev API is enabled

[Janusz Krzysztofik <jmkrzyszt@gmail.com>]

Hi Sakari,

On Friday, May 31, 2019 1:54:26 PM CEST Sakari Ailus wrote:
> 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. 

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.

> 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


> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> 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(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/
v4l2-subdev.c
> index 34219e489be27..88b4b9d7c41be 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -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;
>  
>