Re: [PATCH 14/16] media: sun6i-isp: Use V4L2 subdev active state

[Paul Kocialkowski <paulk@sys-base.io>]

[-- Attachment #1: Type: text/plain, Size: 3316 bytes --]

Le Mon 18 May 26, 11:44, sashiko-bot@kernel.org a écrit :
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] The format for SUN6I_ISP_PROC_PAD_SINK_PARAMS is left uninitialized (zeroed) in sun6i_isp_proc_init_state, which will cause v4l2-compliance failures.
> --
> 
> commit a1aac013cb6f860e3fef00fefa14f9e439c4e03c
> Author: Paul Kocialkowski <paulk@sys-base.io>
> 
> media: sun6i-isp: Use V4L2 subdev active state
> 
> This commit converts the driver to store the active format using the common
> V4L2 subdev active state instead of maintaining a local copy.
> 
> > diff --git a/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_proc.c b/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_proc.c
> > index 46a334b602f19..9073a7f3f8c8c 100644
> > --- a/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_proc.c
> > +++ b/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_proc.c
> 
> [ ... ]
> 
> > @@ -259,21 +262,22 @@ sun6i_isp_proc_mbus_format_prepare(struct v4l2_mbus_framefmt *mbus_format)
> >  static int sun6i_isp_proc_init_state(struct v4l2_subdev *subdev,
> >  				     struct v4l2_subdev_state *state)
> >  {
> > -	struct sun6i_isp_device *isp_dev = v4l2_get_subdevdata(subdev);
> > -	unsigned int pad = SUN6I_ISP_PROC_PAD_SINK_CSI;
> > -	struct v4l2_mbus_framefmt *mbus_format =
> > -		v4l2_subdev_state_get_format(state, pad);
> > -	struct mutex *lock = &isp_dev->proc.lock;
> > +	unsigned int pad;
> >  
> > -	mutex_lock(lock);
> > +	for (pad = 0; pad < subdev->entity.num_pads; pad++) {
> > +		struct v4l2_mbus_framefmt *mbus_format;
> >  
> > -	mbus_format->code = sun6i_isp_proc_formats[0].mbus_code;
> > -	mbus_format->width = 1280;
> > -	mbus_format->height = 720;
> > +		if (pad == SUN6I_ISP_PROC_PAD_SINK_PARAMS)
> > +			continue;
> 
> Does skipping the initialization for SUN6I_ISP_PROC_PAD_SINK_PARAMS here
> leave its format completely zeroed out?
>
> Because v4l2_subdev_init_finalize() allocates the active state memory using
> kzalloc(), skipping this pad means its format will be left with a media bus
> code of 0, as well as 0 for width and height.
> 
> With the get_fmt operation now pointing to the generic v4l2_subdev_get_fmt()
> helper, calling VIDIOC_SUBDEV_G_FMT on the parameters pad will return this
> zeroed format.

Yes, there is no mbus format definition here and everything zeroed out
is fine.

> Since a media bus code of 0 is invalid in V4L2, could this result in
> v4l2-compliance testing failures?

I think it's fine.

Paul

> 
> >  
> > -	sun6i_isp_proc_mbus_format_prepare(mbus_format);
> > +		mbus_format = v4l2_subdev_state_get_format(state, pad);
> >  
> > -	mutex_unlock(lock);
> > +		mbus_format->code = sun6i_isp_proc_formats[0].mbus_code;
> > +		mbus_format->width = 1280;
> > +		mbus_format->height = 720;
> > +
> > +		sun6i_isp_proc_mbus_format_prepare(mbus_format);
> > +	}
> >  
> >  	return 0;
> >  }
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260518102451.417971-1-paulk@sys-base.io?part=14

-- 
Paul Kocialkowski,

Independent contractor - sys-base - https://www.sys-base.io/
Free software developer - https://www.paulk.fr/

Expert in multimedia, graphics and embedded hardware support with Linux.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]