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@xxxxxxxxxxxxxxxxxxxx> >> Signed-off-by: Jonas Karlman <jonas@xxxxxxxxx> >> --- >> 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) >
