Re: [PATCH v5 06/23] staging: media: tegra-video: vi: adjust get_selection op check

[Hans Verkuil <hverkuil+cisco@kernel.org>]

Hi Svyatoslav,

On 22/10/2025 16:20, Svyatoslav Ryhel wrote:
> Get_selection operation may be implemented only for sink pad and may
> return error code. Set try_crop to 0 instead of returning error.

Can you mention why try_crop is set to 0 instead of returning an error?

That would be good to have in the commit log. And in fact, it's not
clear to me either why you want this.

> 
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> ---
>  drivers/staging/media/tegra-video/vi.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/media/tegra-video/vi.c b/drivers/staging/media/tegra-video/vi.c
> index 7c44a3448588..856b7c18b551 100644
> --- a/drivers/staging/media/tegra-video/vi.c
> +++ b/drivers/staging/media/tegra-video/vi.c
> @@ -476,15 +476,11 @@ static int __tegra_channel_try_format(struct tegra_vi_channel *chan,
>  	fse.code = fmtinfo->code;
>  	ret = v4l2_subdev_call(subdev, pad, enum_frame_size, sd_state, &fse);
>  	if (ret) {
> -		if (!v4l2_subdev_has_op(subdev, pad, get_selection)) {
> +		if (!v4l2_subdev_has_op(subdev, pad, get_selection) ||
> +		    v4l2_subdev_call(subdev, pad, get_selection, NULL, &sdsel)) {
>  			try_crop->width = 0;
>  			try_crop->height = 0;

This looks all a bit magical. Which subdev is queried here? I.e. what is the corresponding
subdev driver that implements get_selection?

>  		} else {
> -			ret = v4l2_subdev_call(subdev, pad, get_selection,
> -					       NULL, &sdsel);
> -			if (ret)
> -				return -EINVAL;
> -
>  			try_crop->width = sdsel.r.width;
>  			try_crop->height = sdsel.r.height;
>  		}

It looks odd (esp. setting try_crop to 0), and I wonder if this code path has been tested.

Regards,

	Hans