diff mbox series

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

Message ID 20231105165521.3592037-6-jonas@kwiboo.se (mailing list archive)
State New, archived
Headers show
Series media: rkvdec: Add H.264 High 10 and 4:2:2 profile support | expand

Commit Message

Jonas Karlman Nov. 5, 2023, 4:55 p.m. UTC
SPS parameters is validated in try_ctrl() ops so there is no need to
re-validate when streaming starts.

Remove the unnecessary call to validate sps at streaming start.

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

Comments

Nicolas Dufresne Nov. 7, 2023, 10:01 p.m. UTC | #1
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)
Jonas Karlman Nov. 7, 2023, 10:56 p.m. UTC | #2
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)
>
Nicolas Dufresne Nov. 8, 2023, 2:39 a.m. UTC | #3
Le mardi 07 novembre 2023 à 23:56 +0100, Jonas Karlman a écrit :
> 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).

Yes, but other part of the spec prevents us from failing if the
userspace restart in the middle of the process.

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

In order to avoid having to do this, Hantro driver just ignores these
values from SPS control and do the following:

	reg = G1_REG_DEC_CTRL1_PIC_MB_WIDTH(MB_WIDTH(ctx->src_fmt.width)) |
	      G1_REG_DEC_CTRL1_PIC_MB_HEIGHT_P(MB_HEIGHT(ctx->src_fmt.height)) |
	      G1_REG_DEC_CTRL1_REF_FRAMES(sps->max_num_ref_frames);

Then all they do is reset the CAPTURE format whenever needed, matching
the bit depth that was previously set. The SPS is unfortunatly not
guarantied to be valid, but at first sight its safe in regard to
picture dimensions.

> 
> Will probably just drop this patch in v5.

That or do like in Hantro driver. What is scary though is that some of
the feature enabled by SPS may requires an auxiliary chunk of memory to
be allocated, and then this method falls appart. I think it would be
nice to fix that properly in all drivers in a future patchset.

> 
> 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)
> > 
>
Jonas Karlman Nov. 9, 2023, 6:07 p.m. UTC | #4
On 2023-11-08 03:39, Nicolas Dufresne wrote:
> Le mardi 07 novembre 2023 à 23:56 +0100, Jonas Karlman a écrit :
>> 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).
> 
> Yes, but other part of the spec prevents us from failing if the
> userspace restart in the middle of the process.
> 
>>
>> 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.
> 
> In order to avoid having to do this, Hantro driver just ignores these
> values from SPS control and do the following:
> 
> 	reg = G1_REG_DEC_CTRL1_PIC_MB_WIDTH(MB_WIDTH(ctx->src_fmt.width)) |
> 	      G1_REG_DEC_CTRL1_PIC_MB_HEIGHT_P(MB_HEIGHT(ctx->src_fmt.height)) |
> 	      G1_REG_DEC_CTRL1_REF_FRAMES(sps->max_num_ref_frames);
> 
> Then all they do is reset the CAPTURE format whenever needed, matching
> the bit depth that was previously set. The SPS is unfortunatly not
> guarantied to be valid, but at first sight its safe in regard to
> picture dimensions.

The commit 77e74be83083 ("media: rkvdec: h264: Validate and use pic
width and height in mbs") changed to use the SPS values to help fix
decoding of field encoded content, it also added this check.

Will drop this patch in v5, and should also re-add similar validation in
the HEVC series.

Regards,
Jonas

> 
>>
>> Will probably just drop this patch in v5.
> 
> That or do like in Hantro driver. What is scary though is that some of
> the feature enabled by SPS may requires an auxiliary chunk of memory to
> be allocated, and then this method falls appart. I think it would be
> nice to fix that properly in all drivers in a future patchset.
> 
>>
>> 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)
>>>
>>
>
diff mbox series

Patch

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)