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@xxxxxxxxxxxxxxxx>
> > ---
> > 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
