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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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