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

Jonas Karlman
2023-11-07T22:57:16Z

On 2023-11-07 23:01, Nicolas Dufresne wrote:
> 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.

According to the Initialization section of the V4L2 stateless
documentation a client is supposed to S_CTRL(SPS) after S_FMT(OUTPUT).

https://docs.kernel.org/userspace-api/media/v4l/dev-stateless-decoder.html#initialization

I guess that all stateless decoders probably should reset all ctrls to
default value on S_FMT(OUTPUT) or decoders may end up in an unexpected
state?

Is resetting a ctrl value back to default something that is supported by
v4l2 ctrl core? Did not find any obvious way to reset a ctrl value.

Will probably just drop this patch in v5.

Regards,
Jonas

>
> 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)
>