Re: [PATCH v4 05/11] media: rkvdec: h264: Remove SPS validation at streaming start

From: Nicolas  Dufresne <nicolas.dufresne@collabora.com>
To: Jonas Karlman <jonas@kwiboo.se>,
	Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	 Hans Verkuil <hverkuil-cisco@xs4all.nl>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Alex Bee <knaerzche@gmail.com>,
	Benjamin Gaignard <benjamin.gaignard@collabora.com>,
	Sebastian Fricke <sebastian.fricke@collabora.com>,
	Christopher Obbard <chris.obbard@collabora.com>,
	linux-media@vger.kernel.org,  linux-rockchip@lists.infradead.org,
	linux-staging@lists.linux.dev,  linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 05/11] media: rkvdec: h264: Remove SPS validation at streaming start
Date: Tue, 07 Nov 2023 17:01:02 -0500	[thread overview]
Message-ID: <c75c894a09292775773ad338121ee81924337cf0.camel@collabora.com> (raw)
In-Reply-To: <20231105165521.3592037-6-jonas@kwiboo.se>

Le dimanche 05 novembre 2023 à 16:55 +0000, Jonas Karlman a écrit :
> SPS parameters is validated in try_ctrl() ops so there is no need to

                 are

> re-validate when streaming starts.
> 
> Remove the unnecessary call to validate sps at streaming start.

This patch is not working since user may change the format after the
control have been set. The proper fix should actually reset the SPS
(well all header controls) to match the the newly set format. Then this
validation won't be needed anymore.

The sequence that is problematic after this patch is:

S_FMT (OUTPUT, width, height);
S_CTRL (SPS) // valid
S_FMT(OUTPUT, width', height'); // SPS is no longer valid

One suggestion I may make is to add a ops so that each codec
implementation can reset their header controls to make it valid against
the new resolution. With that in place you'll be able drop the check.

Nicolas

p.s. you can also just drop this patch from the series and revisit it
later, though I think its worth fixing.

> 
> Suggested-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
> v4:
> - No change
> 
> v3:
> - New patch
> 
>  drivers/staging/media/rkvdec/rkvdec-h264.c | 19 ++-----------------
>  1 file changed, 2 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
> index 8bce8902b8dd..815d5359ddd5 100644
> --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
> +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
> @@ -1070,17 +1070,6 @@ static int rkvdec_h264_start(struct rkvdec_ctx *ctx)
>  	struct rkvdec_dev *rkvdec = ctx->dev;
>  	struct rkvdec_h264_priv_tbl *priv_tbl;
>  	struct rkvdec_h264_ctx *h264_ctx;
> -	struct v4l2_ctrl *ctrl;
> -	int ret;
> -
> -	ctrl = v4l2_ctrl_find(&ctx->ctrl_hdl,
> -			      V4L2_CID_STATELESS_H264_SPS);
> -	if (!ctrl)
> -		return -EINVAL;
> -
> -	ret = rkvdec_h264_validate_sps(ctx, ctrl->p_new.p_h264_sps);
> -	if (ret)
> -		return ret;
>  
>  	h264_ctx = kzalloc(sizeof(*h264_ctx), GFP_KERNEL);
>  	if (!h264_ctx)
> @@ -1089,8 +1078,8 @@ static int rkvdec_h264_start(struct rkvdec_ctx *ctx)
>  	priv_tbl = dma_alloc_coherent(rkvdec->dev, sizeof(*priv_tbl),
>  				      &h264_ctx->priv_tbl.dma, GFP_KERNEL);
>  	if (!priv_tbl) {
> -		ret = -ENOMEM;
> -		goto err_free_ctx;
> +		kfree(h264_ctx);
> +		return -ENOMEM;
>  	}
>  
>  	h264_ctx->priv_tbl.size = sizeof(*priv_tbl);
> @@ -1100,10 +1089,6 @@ static int rkvdec_h264_start(struct rkvdec_ctx *ctx)
>  
>  	ctx->priv = h264_ctx;
>  	return 0;
> -
> -err_free_ctx:
> -	kfree(h264_ctx);
> -	return ret;
>  }
>  
>  static void rkvdec_h264_stop(struct rkvdec_ctx *ctx)

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip