[PATCH] hantro_v4l2: ignore enum_framesizes for non-coded formats without postproc

[PATCH] hantro_v4l2: ignore enum_framesizes for non-coded formats without postproc

[Michael Grzeschik]

When codec_mode is HANTRO_MODE_NONE, then vidioc_enum_framesizes should
return with -EINVAL. At least when hantro_needs_postproc returns false.
Which is the case for encoders. But with the latest postproc scaling
patch this is not the case anymore. This patch is fixing this.

Fixes: 79c987de8b35 ("media: hantro: Use post processor scaling capacities")
Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 drivers/staging/media/hantro/hantro_v4l2.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/hantro/hantro_v4l2.c b/drivers/staging/media/hantro/hantro_v4l2.c
index 22ad182ee972ca..0eb0873d383678 100644
--- a/drivers/staging/media/hantro/hantro_v4l2.c
+++ b/drivers/staging/media/hantro/hantro_v4l2.c
@@ -124,8 +124,11 @@ static int vidioc_enum_framesizes(struct file *file, void *priv,
 	}
 
 	/* For non-coded formats check if postprocessing scaling is possible */
-	if (fmt->codec_mode == HANTRO_MODE_NONE && hantro_needs_postproc(ctx, fmt)) {
-		return hanto_postproc_enum_framesizes(ctx, fsize);
+	if (fmt->codec_mode == HANTRO_MODE_NONE) {
+		if (hantro_needs_postproc(ctx, fmt))
+			return hanto_postproc_enum_framesizes(ctx, fsize);
+		else
+			return -EINVAL;
 	} else if (fsize->index != 0) {
 		vpu_debug(0, "invalid frame size index (expected 0, got %d)\n",
 			  fsize->index);
-- 
2.30.2

Re: [PATCH] hantro_v4l2: ignore enum_framesizes for non-coded formats without postproc

[Ezequiel Garcia]

Hi Michael,

On Tue, Jun 21, 2022 at 12:31:50AM +0200, Michael Grzeschik wrote:
> When codec_mode is HANTRO_MODE_NONE, then vidioc_enum_framesizes should
> return with -EINVAL. At least when hantro_needs_postproc returns false.
> Which is the case for encoders. But with the latest postproc scaling
> patch this is not the case anymore. This patch is fixing this.
> 

The fix looks correct, but the commit description looks a bit confusing,
do you think you could reword it and make it more readable?

For commits that fix regressions, it's often handy to include the word "fix"
in the commit subject. How about:

  hantro: fix VIDIOC_ENUM_FRAMESIZES for non-coded formats without postproc

> Fixes: 79c987de8b35 ("media: hantro: Use post processor scaling capacities")
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>

Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>

Thanks,
Ezequiel

> ---
>  drivers/staging/media/hantro/hantro_v4l2.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/hantro/hantro_v4l2.c b/drivers/staging/media/hantro/hantro_v4l2.c
> index 22ad182ee972ca..0eb0873d383678 100644
> --- a/drivers/staging/media/hantro/hantro_v4l2.c
> +++ b/drivers/staging/media/hantro/hantro_v4l2.c
> @@ -124,8 +124,11 @@ static int vidioc_enum_framesizes(struct file *file, void *priv,
>  	}
>  
>  	/* For non-coded formats check if postprocessing scaling is possible */
> -	if (fmt->codec_mode == HANTRO_MODE_NONE && hantro_needs_postproc(ctx, fmt)) {
> -		return hanto_postproc_enum_framesizes(ctx, fsize);
> +	if (fmt->codec_mode == HANTRO_MODE_NONE) {
> +		if (hantro_needs_postproc(ctx, fmt))
> +			return hanto_postproc_enum_framesizes(ctx, fsize);
> +		else
> +			return -EINVAL;
>  	} else if (fsize->index != 0) {
>  		vpu_debug(0, "invalid frame size index (expected 0, got %d)\n",
>  			  fsize->index);
> -- 
> 2.30.2
> 

Re: [PATCH] hantro_v4l2: ignore enum_framesizes for non-coded formats without postproc

[Nicolas Dufresne]

Le mardi 21 juin 2022 à 00:31 +0200, Michael Grzeschik a écrit :
> When codec_mode is HANTRO_MODE_NONE, then vidioc_enum_framesizes should
> return with -EINVAL. At least when hantro_needs_postproc returns false.
> Which is the case for encoders. But with the latest postproc scaling
> patch this is not the case anymore. This patch is fixing this.
> 
> Fixes: 79c987de8b35 ("media: hantro: Use post processor scaling capacities")
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
>  drivers/staging/media/hantro/hantro_v4l2.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/hantro/hantro_v4l2.c b/drivers/staging/media/hantro/hantro_v4l2.c
> index 22ad182ee972ca..0eb0873d383678 100644
> --- a/drivers/staging/media/hantro/hantro_v4l2.c
> +++ b/drivers/staging/media/hantro/hantro_v4l2.c
> @@ -124,8 +124,11 @@ static int vidioc_enum_framesizes(struct file *file, void *priv,
>  	}
>  
>  	/* For non-coded formats check if postprocessing scaling is possible */
> -	if (fmt->codec_mode == HANTRO_MODE_NONE && hantro_needs_postproc(ctx, fmt)) {
> -		return hanto_postproc_enum_framesizes(ctx, fsize);
> +	if (fmt->codec_mode == HANTRO_MODE_NONE) {
> +		if (hantro_needs_postproc(ctx, fmt))
> +			return hanto_postproc_enum_framesizes(ctx, fsize);
> +		else
> +			return -EINVAL;

Was already broken before (because no one uses that yet), but this EINVAL is not
strictly correct. If there is no postproc, enum should return a single frame
size or ENOTTY. Returning EINVAL means the driver supports not output format,
which is a lie. I personally think its saner to have a single framesize being
returned, one that matches what G_FMT returns already.

>  	} else if (fsize->index != 0) {
>  		vpu_debug(0, "invalid frame size index (expected 0, got %d)\n",
>  			  fsize->index);