[PATCH] media: microchip-isc: Remove dead code in pipeline validation

[PATCH] media: microchip-isc: Remove dead code in pipeline validation

[Laurent Pinchart]

The isc_try_fse() function, called from isc_validate(), takes two
parameters, an isc_device pointer, and a v4l2_subdev_state pointer. The
isc_device is accessed but not modified by the function. The state is
modified, including the struct v4l2_subdev_pad_config array it points
to, but they are then never used by the caller. Furthermore, the V4L2
subdev operation called by isc_try_fse() doesn't modify the subdev it is
called on. The isc_try_fse() function has thus no effect, and can just
be dropped.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
This patch has been compile-tested only. Eugen, would you be able to
test it on hardware ?
---
 .../platform/microchip/microchip-isc-base.c   | 39 -------------------
 1 file changed, 39 deletions(-)

diff --git a/drivers/media/platform/microchip/microchip-isc-base.c b/drivers/media/platform/microchip/microchip-isc-base.c
index 1f8528844497..540cb1378289 100644
--- a/drivers/media/platform/microchip/microchip-isc-base.c
+++ b/drivers/media/platform/microchip/microchip-isc-base.c
@@ -851,38 +851,6 @@ static int isc_try_configure_pipeline(struct isc_device *isc)
 	return 0;
 }
 
-static void isc_try_fse(struct isc_device *isc,
-			struct v4l2_subdev_state *sd_state)
-{
-	struct v4l2_subdev_frame_size_enum fse = {
-		.which = V4L2_SUBDEV_FORMAT_TRY,
-	};
-	int ret;
-
-	/*
-	 * If we do not know yet which format the subdev is using, we cannot
-	 * do anything.
-	 */
-	if (!isc->config.sd_format)
-		return;
-
-	fse.code = isc->try_config.sd_format->mbus_code;
-
-	ret = v4l2_subdev_call(isc->current_subdev->sd, pad, enum_frame_size,
-			       sd_state, &fse);
-	/*
-	 * Attempt to obtain format size from subdev. If not available,
-	 * just use the maximum ISC can receive.
-	 */
-	if (ret) {
-		sd_state->pads->try_crop.width = isc->max_width;
-		sd_state->pads->try_crop.height = isc->max_height;
-	} else {
-		sd_state->pads->try_crop.width = fse.max_width;
-		sd_state->pads->try_crop.height = fse.max_height;
-	}
-}
-
 static int isc_try_fmt(struct isc_device *isc, struct v4l2_format *f)
 {
 	struct v4l2_pix_format *pixfmt = &f->fmt.pix;
@@ -944,10 +912,6 @@ static int isc_validate(struct isc_device *isc)
 		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
 		.pad = isc->remote_pad,
 	};
-	struct v4l2_subdev_pad_config pad_cfg = {};
-	struct v4l2_subdev_state pad_state = {
-		.pads = &pad_cfg,
-	};
 
 	/* Get current format from subdev */
 	ret = v4l2_subdev_call(isc->current_subdev->sd, pad, get_fmt, NULL,
@@ -1008,9 +972,6 @@ static int isc_validate(struct isc_device *isc)
 	if (ret)
 		return ret;
 
-	/* Obtain frame sizes if possible to have crop requirements ready */
-	isc_try_fse(isc, &pad_state);
-
 	/* Configure ISC pipeline for the config */
 	ret = isc_try_configure_pipeline(isc);
 	if (ret)
-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: microchip-isc: Remove dead code in pipeline validation

[Eugen Hristev]

On 10/25/23 02:34, Laurent Pinchart wrote:
> The isc_try_fse() function, called from isc_validate(), takes two
> parameters, an isc_device pointer, and a v4l2_subdev_state pointer. The
> isc_device is accessed but not modified by the function. The state is
> modified, including the struct v4l2_subdev_pad_config array it points
> to, but they are then never used by the caller. Furthermore, the V4L2
> subdev operation called by isc_try_fse() doesn't modify the subdev it is
> called on. The isc_try_fse() function has thus no effect, and can just
> be dropped.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> This patch has been compile-tested only. Eugen, would you be able to
> test it on hardware ?

Hello Laurent,

Thank you for the patch.
When I initially wrote this code, I definitely had something in mind. I 
think it was the fact that the ISC should adjust it's crop area 
according to what the subdev is emitting, to avoid other problems (e.g. 
reporting bad frame size and v4l2-compliance crying )
Somehow , maybe, that intention is lost somewhere. I agree with you that 
this code appears useless.
I will test it out and let you know . (I cannot promise exactly when I 
have some time to do it, so bear with me please...)

Eugen

> ---
>   .../platform/microchip/microchip-isc-base.c   | 39 -------------------
>   1 file changed, 39 deletions(-)
> 
> diff --git a/drivers/media/platform/microchip/microchip-isc-base.c b/drivers/media/platform/microchip/microchip-isc-base.c
> index 1f8528844497..540cb1378289 100644
> --- a/drivers/media/platform/microchip/microchip-isc-base.c
> +++ b/drivers/media/platform/microchip/microchip-isc-base.c
> @@ -851,38 +851,6 @@ static int isc_try_configure_pipeline(struct isc_device *isc)
>   	return 0;
>   }
>   
> -static void isc_try_fse(struct isc_device *isc,
> -			struct v4l2_subdev_state *sd_state)
> -{
> -	struct v4l2_subdev_frame_size_enum fse = {
> -		.which = V4L2_SUBDEV_FORMAT_TRY,
> -	};
> -	int ret;
> -
> -	/*
> -	 * If we do not know yet which format the subdev is using, we cannot
> -	 * do anything.
> -	 */
> -	if (!isc->config.sd_format)
> -		return;
> -
> -	fse.code = isc->try_config.sd_format->mbus_code;
> -
> -	ret = v4l2_subdev_call(isc->current_subdev->sd, pad, enum_frame_size,
> -			       sd_state, &fse);
> -	/*
> -	 * Attempt to obtain format size from subdev. If not available,
> -	 * just use the maximum ISC can receive.
> -	 */
> -	if (ret) {
> -		sd_state->pads->try_crop.width = isc->max_width;
> -		sd_state->pads->try_crop.height = isc->max_height;
> -	} else {
> -		sd_state->pads->try_crop.width = fse.max_width;
> -		sd_state->pads->try_crop.height = fse.max_height;
> -	}
> -}
> -
>   static int isc_try_fmt(struct isc_device *isc, struct v4l2_format *f)
>   {
>   	struct v4l2_pix_format *pixfmt = &f->fmt.pix;
> @@ -944,10 +912,6 @@ static int isc_validate(struct isc_device *isc)
>   		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
>   		.pad = isc->remote_pad,
>   	};
> -	struct v4l2_subdev_pad_config pad_cfg = {};
> -	struct v4l2_subdev_state pad_state = {
> -		.pads = &pad_cfg,
> -	};
>   
>   	/* Get current format from subdev */
>   	ret = v4l2_subdev_call(isc->current_subdev->sd, pad, get_fmt, NULL,
> @@ -1008,9 +972,6 @@ static int isc_validate(struct isc_device *isc)
>   	if (ret)
>   		return ret;
>   
> -	/* Obtain frame sizes if possible to have crop requirements ready */
> -	isc_try_fse(isc, &pad_state);
> -
>   	/* Configure ISC pipeline for the config */
>   	ret = isc_try_configure_pipeline(isc);
>   	if (ret)

Re: [PATCH] media: microchip-isc: Remove dead code in pipeline validation

[Laurent Pinchart]

Hi Eugen,

On Wed, Oct 25, 2023 at 11:31:20AM +0300, Eugen Hristev wrote:
> On 10/25/23 02:34, Laurent Pinchart wrote:
> > The isc_try_fse() function, called from isc_validate(), takes two
> > parameters, an isc_device pointer, and a v4l2_subdev_state pointer. The
> > isc_device is accessed but not modified by the function. The state is
> > modified, including the struct v4l2_subdev_pad_config array it points
> > to, but they are then never used by the caller. Furthermore, the V4L2
> > subdev operation called by isc_try_fse() doesn't modify the subdev it is
> > called on. The isc_try_fse() function has thus no effect, and can just
> > be dropped.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > This patch has been compile-tested only. Eugen, would you be able to
> > test it on hardware ?
> 
> Hello Laurent,
> 
> Thank you for the patch.
> When I initially wrote this code, I definitely had something in mind. I 
> think it was the fact that the ISC should adjust it's crop area 
> according to what the subdev is emitting, to avoid other problems (e.g. 
> reporting bad frame size and v4l2-compliance crying )
> Somehow , maybe, that intention is lost somewhere. I agree with you that 
> this code appears useless.

It seems to have been copied from the previous driver, which used the
try_crop field after fetching it, but that last part was dropping as
part of the conversion to MC.

> I will test it out and let you know . (I cannot promise exactly when I 
> have some time to do it, so bear with me please...)

Thank you.

> > ---
> >   .../platform/microchip/microchip-isc-base.c   | 39 -------------------
> >   1 file changed, 39 deletions(-)
> > 
> > diff --git a/drivers/media/platform/microchip/microchip-isc-base.c b/drivers/media/platform/microchip/microchip-isc-base.c
> > index 1f8528844497..540cb1378289 100644
> > --- a/drivers/media/platform/microchip/microchip-isc-base.c
> > +++ b/drivers/media/platform/microchip/microchip-isc-base.c
> > @@ -851,38 +851,6 @@ static int isc_try_configure_pipeline(struct isc_device *isc)
> >   	return 0;
> >   }
> >   
> > -static void isc_try_fse(struct isc_device *isc,
> > -			struct v4l2_subdev_state *sd_state)
> > -{
> > -	struct v4l2_subdev_frame_size_enum fse = {
> > -		.which = V4L2_SUBDEV_FORMAT_TRY,
> > -	};
> > -	int ret;
> > -
> > -	/*
> > -	 * If we do not know yet which format the subdev is using, we cannot
> > -	 * do anything.
> > -	 */
> > -	if (!isc->config.sd_format)
> > -		return;
> > -
> > -	fse.code = isc->try_config.sd_format->mbus_code;
> > -
> > -	ret = v4l2_subdev_call(isc->current_subdev->sd, pad, enum_frame_size,
> > -			       sd_state, &fse);
> > -	/*
> > -	 * Attempt to obtain format size from subdev. If not available,
> > -	 * just use the maximum ISC can receive.
> > -	 */
> > -	if (ret) {
> > -		sd_state->pads->try_crop.width = isc->max_width;
> > -		sd_state->pads->try_crop.height = isc->max_height;
> > -	} else {
> > -		sd_state->pads->try_crop.width = fse.max_width;
> > -		sd_state->pads->try_crop.height = fse.max_height;
> > -	}
> > -}
> > -
> >   static int isc_try_fmt(struct isc_device *isc, struct v4l2_format *f)
> >   {
> >   	struct v4l2_pix_format *pixfmt = &f->fmt.pix;
> > @@ -944,10 +912,6 @@ static int isc_validate(struct isc_device *isc)
> >   		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> >   		.pad = isc->remote_pad,
> >   	};
> > -	struct v4l2_subdev_pad_config pad_cfg = {};
> > -	struct v4l2_subdev_state pad_state = {
> > -		.pads = &pad_cfg,
> > -	};
> >   
> >   	/* Get current format from subdev */
> >   	ret = v4l2_subdev_call(isc->current_subdev->sd, pad, get_fmt, NULL,
> > @@ -1008,9 +972,6 @@ static int isc_validate(struct isc_device *isc)
> >   	if (ret)
> >   		return ret;
> >   
> > -	/* Obtain frame sizes if possible to have crop requirements ready */
> > -	isc_try_fse(isc, &pad_state);
> > -
> >   	/* Configure ISC pipeline for the config */
> >   	ret = isc_try_configure_pipeline(isc);
> >   	if (ret)
> 

-- 
Regards,

Laurent Pinchart