Re: [PATCH] staging: media: imx: ic-prpencvf: add FORMAT_TRY frame interval support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] staging: media: imx: unpropagated TRY interval in
  prp_set_frame_interval()
--

commit 39c074ccbb9c23c1bfb8a1a38f0cced39b190229
Author: DelaySystem <DelaySystem@tubot.lat>

staging: media: imx: ic-prpencvf: add FORMAT_TRY frame interval support

This commit adds V4L2_SUBDEV_FORMAT_TRY support to the frame interval
callbacks in imx-ic-prpencvf by storing TRY intervals in the subdev state
and initializing them from the default active interval in a new init_state
callback.

> diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c
> index 3430ddeac3569..fd70c9a36f7ee 100644
> --- a/drivers/staging/media/imx/imx-ic-prpencvf.c
> +++ b/drivers/staging/media/imx/imx-ic-prpencvf.c
> @@ -795,6 +795,18 @@ __prp_get_fmt(struct prp_priv *priv, struct v4l2_subdev_state *sd_state,
>  		return &priv->format_mbus[pad];
>  }
>  
> +static struct v4l2_fract *
> +__prp_get_frame_interval(struct prp_priv *priv,
> +			 struct v4l2_subdev_state *sd_state,
> +			 unsigned int pad,
> +			 enum v4l2_subdev_format_whence which)
> +{
> +	if (which == V4L2_SUBDEV_FORMAT_TRY)
> +		return v4l2_subdev_state_get_interval(sd_state, pad);
> +
> +	return &priv->frame_interval;
> +}
> +

[ ... ]

> @@ -1231,24 +1246,50 @@ static int prp_set_frame_interval(struct v4l2_subdev *sd,
>  				  struct v4l2_subdev_frame_interval *fi)
>  {
>  	struct prp_priv *priv = sd_to_priv(sd);
> -
> -	/*
> -	 * FIXME: Implement support for V4L2_SUBDEV_FORMAT_TRY, using the V4L2
> -	 * subdev active state API.
> -	 */
> -	if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> -		return -EINVAL;
> +	struct v4l2_fract *interval;
> +	int ret = 0;
>  
>  	if (fi->pad >= PRPENCVF_NUM_PADS)
>  		return -EINVAL;
>  
>  	mutex_lock(&priv->lock);
> +	interval = __prp_get_frame_interval(priv, sd_state, fi->pad,
> +					    fi->which);
> +	if (!interval) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
>  
>  	/* No limits on valid frame intervals */
>  	if (fi->interval.numerator == 0 || fi->interval.denominator == 0)
> -		fi->interval = priv->frame_interval;
> +		fi->interval = *interval;
>  	else
> -		priv->frame_interval = fi->interval;
> +		*interval = fi->interval;
> +out:
>  	mutex_unlock(&priv->lock);
>  
> -	return 0;
> +	return ret;
> +}

[Severity: Medium]
Should prp_set_frame_interval() keep the sink and source pad intervals in
sync when fi->which is V4L2_SUBDEV_FORMAT_TRY?

For V4L2_SUBDEV_FORMAT_ACTIVE, __prp_get_frame_interval() ignores pad and
returns the single shared &priv->frame_interval because the IC preprocess
task does not change the frame rate between PRPENCVF_SINK_PAD and
PRPENCVF_SRC_PAD.  For V4L2_SUBDEV_FORMAT_TRY, v4l2_subdev_state_get_interval()
returns a separate slot per pad, and prp_set_frame_interval() only updates
fi->pad without propagating the interval to the other pad (unlike
prp_set_fmt(), which propagates sink pad changes to PRPENCVF_SRC_PAD).
Userspace negotiating TRY frame intervals on PRPENCVF_SINK_PAD will read a
stale interval on PRPENCVF_SRC_PAD, or configure different TRY intervals on
the two pads that the hardware and ACTIVE state cannot represent.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107-batch1-m2-prpencvf-try-v1-1-76bd9f505710@tubot.lat?part=1