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

Message ID 20260518102451.417971-15-paulk@sys-base.io (mailing list archive)
State New
Headers
Series media: sun6i-csi/isp MC-centric support and cleanups |

Commit Message

Paul Kocialkowski May 18, 2026, 10:24 a.m. UTC
Store the active format using the common V4L2 subdev active state
instead of our local copy of it.

Signed-off-by: Paul Kocialkowski <paulk@sys-base.io>
---
 .../media/sunxi/sun6i-isp/sun6i_isp_capture.c |  16 ++-
 .../media/sunxi/sun6i-isp/sun6i_isp_params.c  |  18 ++-
 .../media/sunxi/sun6i-isp/sun6i_isp_params.h  |   4 +-
 .../media/sunxi/sun6i-isp/sun6i_isp_proc.c    | 117 ++++++++----------
 .../media/sunxi/sun6i-isp/sun6i_isp_proc.h    |   7 --
 5 files changed, 82 insertions(+), 80 deletions(-)
  

Comments

sashiko-bot@kernel.org May 18, 2026, 11:44 a.m. UTC | #1
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.

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

>  
> -	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;
>  }
  
Paul Kocialkowski May 18, 2026, 11:57 a.m. UTC | #2
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
  
arash golgol May 21, 2026, 9:23 a.m. UTC | #3
Hi Paul,

On Mon, May 18, 2026 at 2:02 PM Paul Kocialkowski <paulk@sys-base.io> wrote:
>
> Store the active format using the common V4L2 subdev active state
> instead of our local copy of it.
>
> Signed-off-by: Paul Kocialkowski <paulk@sys-base.io>
> ---
>  .../media/sunxi/sun6i-isp/sun6i_isp_capture.c |  16 ++-
>  .../media/sunxi/sun6i-isp/sun6i_isp_params.c  |  18 ++-
>  .../media/sunxi/sun6i-isp/sun6i_isp_params.h  |   4 +-
>  .../media/sunxi/sun6i-isp/sun6i_isp_proc.c    | 117 ++++++++----------
>  .../media/sunxi/sun6i-isp/sun6i_isp_proc.h    |   7 --
>  5 files changed, 82 insertions(+), 80 deletions(-)
>
> diff --git a/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_capture.c b/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_capture.c
> index e7b99cee63d6..24e731bcabe9 100644
> --- a/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_capture.c
> +++ b/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_capture.c
> @@ -595,11 +595,25 @@ static int sun6i_isp_capture_link_validate(struct media_link *link)
>                 media_entity_to_video_device(link->sink->entity);
>         struct sun6i_isp_device *isp_dev = video_get_drvdata(video_dev);
>         struct v4l2_device *v4l2_dev = &isp_dev->v4l2.v4l2_dev;
> +       struct v4l2_subdev *proc_subdev =
> +               media_entity_to_v4l2_subdev(link->source->entity);
>         unsigned int capture_width, capture_height;
>         unsigned int proc_width, proc_height;
> +       struct v4l2_subdev_format proc_subdev_format = {
> +               .which  = V4L2_SUBDEV_FORMAT_ACTIVE,
> +               .pad    = link->source->index,
> +       };
> +       int ret;
>
>         sun6i_isp_capture_dimensions(isp_dev, &capture_width, &capture_height);
> -       sun6i_isp_proc_dimensions(isp_dev, &proc_width, &proc_height);
> +
> +       ret = v4l2_subdev_call(proc_subdev, pad, get_fmt, NULL,
> +                              &proc_subdev_format);
> +       if (ret)
> +               return ret;
> +
> +       proc_width = proc_subdev_format.format.width;
> +       proc_height = proc_subdev_format.format.height;
>
>         /* No cropping/scaling is supported (yet). */
>         if (capture_width != proc_width || capture_height != proc_height) {
> diff --git a/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_params.c b/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_params.c
> index b7ef33fa2b13..0cc48e2bc8c6 100644
> --- a/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_params.c
> +++ b/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_params.c
> @@ -43,11 +43,14 @@ static const struct sun6i_isp_params_config sun6i_isp_params_config_default = {
>         },
>  };
>
> -static void sun6i_isp_params_configure_ob(struct sun6i_isp_device *isp_dev)
> +static void
> +sun6i_isp_params_configure_ob(struct sun6i_isp_device *isp_dev,
> +                             const struct v4l2_mbus_framefmt *mbus_format)
>  {
>         unsigned int width, height;
>
> -       sun6i_isp_proc_dimensions(isp_dev, &width, &height);
> +       width = mbus_format->width;
> +       height = mbus_format->height;
>
>         sun6i_isp_load_write(isp_dev, SUN6I_ISP_OB_SIZE_REG,
>                              SUN6I_ISP_OB_SIZE_WIDTH(width) |
> @@ -112,10 +115,12 @@ static void sun6i_isp_params_configure_wb(struct sun6i_isp_device *isp_dev)
>                              SUN6I_ISP_WB_CFG_CLIP(0xfff));
>  }
>
> -static void sun6i_isp_params_configure_base(struct sun6i_isp_device *isp_dev)
> +static void
> +sun6i_isp_params_configure_base(struct sun6i_isp_device *isp_dev,
> +                               const struct v4l2_mbus_framefmt *mbus_format)
>  {
>         sun6i_isp_params_configure_ae(isp_dev);
> -       sun6i_isp_params_configure_ob(isp_dev);
> +       sun6i_isp_params_configure_ob(isp_dev, mbus_format);
>         sun6i_isp_params_configure_wb(isp_dev);
>  }
>
> @@ -170,14 +175,15 @@ sun6i_isp_params_configure_modules(struct sun6i_isp_device *isp_dev,
>         sun6i_isp_load_write(isp_dev, SUN6I_ISP_MODULE_EN_REG, value);
>  }
>
> -void sun6i_isp_params_configure(struct sun6i_isp_device *isp_dev)
> +void sun6i_isp_params_configure(struct sun6i_isp_device *isp_dev,
> +                               const struct v4l2_mbus_framefmt *mbus_format)
>  {
>         struct sun6i_isp_params_state *state = &isp_dev->params.state;
>         unsigned long flags;
>
>         spin_lock_irqsave(&state->lock, flags);
>
> -       sun6i_isp_params_configure_base(isp_dev);
> +       sun6i_isp_params_configure_base(isp_dev, mbus_format);
>
>         /* Default config is only applied at the very first stream start. */
>         if (state->configured)
> diff --git a/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_params.h b/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_params.h
> index 50f10f879c42..c0d6cff95d54 100644
> --- a/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_params.h
> +++ b/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_params.h
> @@ -36,8 +36,8 @@ struct sun6i_isp_params {
>
>  /* Params */
>
> -void sun6i_isp_params_configure(struct sun6i_isp_device *isp_dev);
> -
> +void sun6i_isp_params_configure(struct sun6i_isp_device *isp_dev,
> +                               const struct v4l2_mbus_framefmt *mbus_format);
>  /* State */
>
>  void sun6i_isp_params_state_update(struct sun6i_isp_device *isp_dev,
> 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 46a334b602f1..9073a7f3f8c8 100644
> --- a/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_proc.c
> +++ b/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_proc.c
> @@ -15,17 +15,6 @@
>  #include "sun6i_isp_proc.h"
>  #include "sun6i_isp_reg.h"
>
> -/* Helpers */
> -
> -void sun6i_isp_proc_dimensions(struct sun6i_isp_device *isp_dev,
> -                              unsigned int *width, unsigned int *height)
> -{
> -       if (width)
> -               *width = isp_dev->proc.mbus_format.width;
> -       if (height)
> -               *height = isp_dev->proc.mbus_format.height;
> -}
> -
>  /* Format */
>
>  static const struct sun6i_isp_proc_format sun6i_isp_proc_formats[] = {
> @@ -137,9 +126,10 @@ static void sun6i_isp_proc_disable(struct sun6i_isp_device *isp_dev)
>         regmap_write(regmap, SUN6I_ISP_FE_CFG_REG, 0);
>  }
>
> -static void sun6i_isp_proc_configure(struct sun6i_isp_device *isp_dev)
> +static void
> +sun6i_isp_proc_configure(struct sun6i_isp_device *isp_dev,
> +                        const struct v4l2_mbus_framefmt *mbus_format)
>  {
> -       struct v4l2_mbus_framefmt *mbus_format = &isp_dev->proc.mbus_format;
>         const struct sun6i_isp_proc_format *format;
>         u32 value;
>
> @@ -173,6 +163,8 @@ static int sun6i_isp_proc_s_stream(struct v4l2_subdev *subdev, int on)
>         struct sun6i_isp_proc_source *source;
>         struct v4l2_subdev *source_subdev;
>         struct media_pad *remote_pad;
> +       struct v4l2_subdev_state *state;
> +       const struct v4l2_mbus_framefmt *mbus_format;
>         int ret;
>
>         /* Source */
> @@ -191,6 +183,10 @@ static int sun6i_isp_proc_s_stream(struct v4l2_subdev *subdev, int on)
>         else
>                 source = &proc->source_csi1;
>
> +       /* Active State */
> +
> +       state = v4l2_subdev_lock_and_get_active_state(subdev);
> +
>         if (!on) {
>                 sun6i_isp_proc_irq_disable(isp_dev);
>                 v4l2_subdev_call(source_subdev, video, s_stream, 0);
> @@ -202,7 +198,7 @@ static int sun6i_isp_proc_s_stream(struct v4l2_subdev *subdev, int on)
>
>         ret = pm_runtime_resume_and_get(dev);
>         if (ret < 0)
> -               return ret;
> +               goto unlock;
>
>         /* Clear */
>
> @@ -210,9 +206,12 @@ static int sun6i_isp_proc_s_stream(struct v4l2_subdev *subdev, int on)
>
>         /* Configure */
>
> +       mbus_format = v4l2_subdev_state_get_format(state,
> +                                                  SUN6I_ISP_PROC_PAD_SINK_CSI);
> +
>         sun6i_isp_tables_configure(isp_dev);
> -       sun6i_isp_params_configure(isp_dev);
> -       sun6i_isp_proc_configure(isp_dev);
> +       sun6i_isp_params_configure(isp_dev, mbus_format);
> +       sun6i_isp_proc_configure(isp_dev, mbus_format);
>         sun6i_isp_capture_configure(isp_dev);
>
>         /* State Update */
> @@ -230,13 +229,17 @@ static int sun6i_isp_proc_s_stream(struct v4l2_subdev *subdev, int on)
>                 goto disable;
>         }
>
> -       return 0;
> +       ret = 0;
> +       goto unlock;
>
>  disable:
>         sun6i_isp_proc_disable(isp_dev);
>
>         pm_runtime_put(dev);
>
> +unlock:
> +       v4l2_subdev_unlock_state(state);
> +
>         return ret;
>  }
>
> @@ -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;
>
> -       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;
>  }
> @@ -291,53 +295,31 @@ sun6i_isp_proc_enum_mbus_code(struct v4l2_subdev *subdev,
>         return 0;
>  }
>
> -static int sun6i_isp_proc_get_fmt(struct v4l2_subdev *subdev,
> -                                 struct v4l2_subdev_state *state,
> -                                 struct v4l2_subdev_format *format)
> -{
> -       struct sun6i_isp_device *isp_dev = v4l2_get_subdevdata(subdev);
> -       struct v4l2_mbus_framefmt *mbus_format = &format->format;
> -       struct mutex *lock = &isp_dev->proc.lock;
> -
> -       mutex_lock(lock);
> -
> -       if (format->which == V4L2_SUBDEV_FORMAT_TRY)
> -               *mbus_format = *v4l2_subdev_state_get_format(state,
> -                                                            format->pad);
> -       else
> -               *mbus_format = isp_dev->proc.mbus_format;
> -
> -       mutex_unlock(lock);
> -
> -       return 0;
> -}
> -
>  static int sun6i_isp_proc_set_fmt(struct v4l2_subdev *subdev,
>                                   struct v4l2_subdev_state *state,
>                                   struct v4l2_subdev_format *format)
>  {
> -       struct sun6i_isp_device *isp_dev = v4l2_get_subdevdata(subdev);
> -       struct v4l2_mbus_framefmt *mbus_format = &format->format;
> -       struct mutex *lock = &isp_dev->proc.lock;
> +       struct v4l2_mbus_framefmt *mbus_format;
>
> -       mutex_lock(lock);
> +       if (format->pad != SUN6I_ISP_PROC_PAD_SINK_CSI)
> +               return v4l2_subdev_get_fmt(subdev, state, format);
>
> -       sun6i_isp_proc_mbus_format_prepare(mbus_format);
> +       sun6i_isp_proc_mbus_format_prepare(&format->format);
>
> -       if (format->which == V4L2_SUBDEV_FORMAT_TRY)
> -               *v4l2_subdev_state_get_format(state, format->pad) =
> -                       *mbus_format;
> -       else
> -               isp_dev->proc.mbus_format = *mbus_format;
> +       mbus_format = v4l2_subdev_state_get_format(state, format->pad);
> +       *mbus_format = format->format;
>
> -       mutex_unlock(lock);
> +       /* Propagate the format to the source pad. */
> +       mbus_format = v4l2_subdev_state_get_format(state,
> +                                                  SUN6I_ISP_PROC_PAD_SOURCE);
> +       *mbus_format = format->format;
>
>         return 0;
>  }
>
>  static const struct v4l2_subdev_pad_ops sun6i_isp_proc_pad_ops = {
>         .enum_mbus_code = sun6i_isp_proc_enum_mbus_code,
> -       .get_fmt        = sun6i_isp_proc_get_fmt,
> +       .get_fmt        = v4l2_subdev_get_fmt,
>         .set_fmt        = sun6i_isp_proc_set_fmt,
>  };
>
> @@ -499,8 +481,6 @@ int sun6i_isp_proc_setup(struct sun6i_isp_device *isp_dev)
>         struct media_pad *pads = proc->pads;
>         int ret;
>
> -       mutex_init(&proc->lock);
> -
>         /* V4L2 Subdev */
>
>         v4l2_subdev_init(subdev, &sun6i_isp_proc_subdev_ops);
> @@ -532,10 +512,14 @@ int sun6i_isp_proc_setup(struct sun6i_isp_device *isp_dev)
>
>         /* V4L2 Subdev */
>
> +       ret = v4l2_subdev_init_finalize(subdev);
> +       if (ret < 0)
> +               goto error_media_entity;
> +
>         ret = v4l2_device_register_subdev(v4l2_dev, subdev);
>         if (ret < 0) {
>                 v4l2_err(v4l2_dev, "failed to register v4l2 subdev: %d\n", ret);
> -               goto error_media_entity;
> +               goto error_subdev_finalize;
>         }
>
>         /* V4L2 Async */
> @@ -562,6 +546,9 @@ int sun6i_isp_proc_setup(struct sun6i_isp_device *isp_dev)
>
>         v4l2_device_unregister_subdev(subdev);
>
> +error_subdev_finalize:
> +       v4l2_subdev_cleanup(subdev);
> +
>  error_media_entity:
>         media_entity_cleanup(&subdev->entity);
>
> @@ -577,5 +564,7 @@ void sun6i_isp_proc_cleanup(struct sun6i_isp_device *isp_dev)
>         v4l2_async_nf_cleanup(notifier);
>
>         v4l2_device_unregister_subdev(subdev);
> +       v4l2_subdev_cleanup(subdev);
> +
>         media_entity_cleanup(&subdev->entity);
>  }
> diff --git a/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_proc.h b/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_proc.h
> index db6738a39147..26c4327c5ed7 100644
> --- a/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_proc.h
> +++ b/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_proc.h
> @@ -42,18 +42,11 @@ struct sun6i_isp_proc {
>         struct v4l2_subdev              subdev;
>         struct media_pad                pads[3];
>         struct v4l2_async_notifier      notifier;
> -       struct v4l2_mbus_framefmt       mbus_format;
> -       struct mutex                    lock; /* Mbus format lock. */
>
>         struct sun6i_isp_proc_source    source_csi0;
>         struct sun6i_isp_proc_source    source_csi1;
>  };
>
> -/* Helpers */
> -
> -void sun6i_isp_proc_dimensions(struct sun6i_isp_device *isp_dev,
> -                              unsigned int *width, unsigned int *height);
> -
>  /* Format */
>
>  const struct sun6i_isp_proc_format *sun6i_isp_proc_format_find(u32 mbus_code);
> --
> 2.54.0
>

I used LicheePi Zero Dock (V3s) with the following pipeline as test setup.

ov5647 -> sun6i-mipi-csi2 -> sun6i-csi-bridge -> sun6i-isp-proc ->
sun6i-isp-capture

I verified TRY and ACTIVE state handling, including changing TRY
formats without affecting ACTIVE state. Format propagation from the
sink (csi) pad to the source pad was also tested.

I also tested streaming with the sensor test pattern enabled and
verified the captured output was correct.

Tested-by: Arash Golgol <arash.golgol@gmail.com>
  
Paul Kocialkowski May 22, 2026, 11:15 a.m. UTC | #4
Hi Arash,

Le Thu 21 May 26, 12:53, arash golgol a écrit :
> I used LicheePi Zero Dock (V3s) with the following pipeline as test setup.
> 
> ov5647 -> sun6i-mipi-csi2 -> sun6i-csi-bridge -> sun6i-isp-proc ->
> sun6i-isp-capture
> 
> I verified TRY and ACTIVE state handling, including changing TRY
> formats without affecting ACTIVE state. Format propagation from the
> sink (csi) pad to the source pad was also tested.
> 
> I also tested streaming with the sensor test pattern enabled and
> verified the captured output was correct.
> 
> Tested-by: Arash Golgol <arash.golgol@gmail.com>

Thanks a lot for testing this! Did you test just this patch or the other
ones (especially sun6i-csi format enumeration) as well?

Also if you have an opinion on the code itself feel free to drop a
Reviewed-by tag, a lot of this work is very similar to what you did for
sun6i-csi and other drivers.

All the best,

Paul
  
arash golgol May 23, 2026, 3:15 a.m. UTC | #5
Hi Paul,

On Fri, May 22, 2026 at 2:45 PM Paul Kocialkowski <paulk@sys-base.io> wrote:
>
> Hi Arash,
>
> Le Thu 21 May 26, 12:53, arash golgol a écrit :
> > I used LicheePi Zero Dock (V3s) with the following pipeline as test setup.
> >
> > ov5647 -> sun6i-mipi-csi2 -> sun6i-csi-bridge -> sun6i-isp-proc ->
> > sun6i-isp-capture
> >
> > I verified TRY and ACTIVE state handling, including changing TRY
> > formats without affecting ACTIVE state. Format propagation from the
> > sink (csi) pad to the source pad was also tested.
> >
> > I also tested streaming with the sensor test pattern enabled and
> > verified the captured output was correct.
> >
> > Tested-by: Arash Golgol <arash.golgol@gmail.com>
>
> Thanks a lot for testing this! Did you test just this patch or the other
> ones (especially sun6i-csi format enumeration) as well?
>

Happy to help.
I only tested patch 14 for now, but I plan to test the other patches
as well when I get some more time, especially the sun6i-csi changes.

> Also if you have an opinion on the code itself feel free to drop a
> Reviewed-by tag, a lot of this work is very similar to what you did for
> sun6i-csi and other drivers.
>

>From a code perspective, the changes look good to me.
Reviewed-by: Arash Golgol <arash.golgol@gmail.com>
  

Patch

diff --git a/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_capture.c b/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_capture.c
index e7b99cee63d6..24e731bcabe9 100644
--- a/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_capture.c
+++ b/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_capture.c
@@ -595,11 +595,25 @@  static int sun6i_isp_capture_link_validate(struct media_link *link)
 		media_entity_to_video_device(link->sink->entity);
 	struct sun6i_isp_device *isp_dev = video_get_drvdata(video_dev);
 	struct v4l2_device *v4l2_dev = &isp_dev->v4l2.v4l2_dev;
+	struct v4l2_subdev *proc_subdev =
+		media_entity_to_v4l2_subdev(link->source->entity);
 	unsigned int capture_width, capture_height;
 	unsigned int proc_width, proc_height;
+	struct v4l2_subdev_format proc_subdev_format = {
+		.which	= V4L2_SUBDEV_FORMAT_ACTIVE,
+		.pad	= link->source->index,
+	};
+	int ret;
 
 	sun6i_isp_capture_dimensions(isp_dev, &capture_width, &capture_height);
-	sun6i_isp_proc_dimensions(isp_dev, &proc_width, &proc_height);
+
+	ret = v4l2_subdev_call(proc_subdev, pad, get_fmt, NULL,
+			       &proc_subdev_format);
+	if (ret)
+		return ret;
+
+	proc_width = proc_subdev_format.format.width;
+	proc_height = proc_subdev_format.format.height;
 
 	/* No cropping/scaling is supported (yet). */
 	if (capture_width != proc_width || capture_height != proc_height) {
diff --git a/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_params.c b/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_params.c
index b7ef33fa2b13..0cc48e2bc8c6 100644
--- a/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_params.c
+++ b/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_params.c
@@ -43,11 +43,14 @@  static const struct sun6i_isp_params_config sun6i_isp_params_config_default = {
 	},
 };
 
-static void sun6i_isp_params_configure_ob(struct sun6i_isp_device *isp_dev)
+static void
+sun6i_isp_params_configure_ob(struct sun6i_isp_device *isp_dev,
+			      const struct v4l2_mbus_framefmt *mbus_format)
 {
 	unsigned int width, height;
 
-	sun6i_isp_proc_dimensions(isp_dev, &width, &height);
+	width = mbus_format->width;
+	height = mbus_format->height;
 
 	sun6i_isp_load_write(isp_dev, SUN6I_ISP_OB_SIZE_REG,
 			     SUN6I_ISP_OB_SIZE_WIDTH(width) |
@@ -112,10 +115,12 @@  static void sun6i_isp_params_configure_wb(struct sun6i_isp_device *isp_dev)
 			     SUN6I_ISP_WB_CFG_CLIP(0xfff));
 }
 
-static void sun6i_isp_params_configure_base(struct sun6i_isp_device *isp_dev)
+static void
+sun6i_isp_params_configure_base(struct sun6i_isp_device *isp_dev,
+				const struct v4l2_mbus_framefmt *mbus_format)
 {
 	sun6i_isp_params_configure_ae(isp_dev);
-	sun6i_isp_params_configure_ob(isp_dev);
+	sun6i_isp_params_configure_ob(isp_dev, mbus_format);
 	sun6i_isp_params_configure_wb(isp_dev);
 }
 
@@ -170,14 +175,15 @@  sun6i_isp_params_configure_modules(struct sun6i_isp_device *isp_dev,
 	sun6i_isp_load_write(isp_dev, SUN6I_ISP_MODULE_EN_REG, value);
 }
 
-void sun6i_isp_params_configure(struct sun6i_isp_device *isp_dev)
+void sun6i_isp_params_configure(struct sun6i_isp_device *isp_dev,
+				const struct v4l2_mbus_framefmt *mbus_format)
 {
 	struct sun6i_isp_params_state *state = &isp_dev->params.state;
 	unsigned long flags;
 
 	spin_lock_irqsave(&state->lock, flags);
 
-	sun6i_isp_params_configure_base(isp_dev);
+	sun6i_isp_params_configure_base(isp_dev, mbus_format);
 
 	/* Default config is only applied at the very first stream start. */
 	if (state->configured)
diff --git a/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_params.h b/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_params.h
index 50f10f879c42..c0d6cff95d54 100644
--- a/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_params.h
+++ b/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_params.h
@@ -36,8 +36,8 @@  struct sun6i_isp_params {
 
 /* Params */
 
-void sun6i_isp_params_configure(struct sun6i_isp_device *isp_dev);
-
+void sun6i_isp_params_configure(struct sun6i_isp_device *isp_dev,
+				const struct v4l2_mbus_framefmt *mbus_format);
 /* State */
 
 void sun6i_isp_params_state_update(struct sun6i_isp_device *isp_dev,
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 46a334b602f1..9073a7f3f8c8 100644
--- a/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_proc.c
+++ b/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_proc.c
@@ -15,17 +15,6 @@ 
 #include "sun6i_isp_proc.h"
 #include "sun6i_isp_reg.h"
 
-/* Helpers */
-
-void sun6i_isp_proc_dimensions(struct sun6i_isp_device *isp_dev,
-			       unsigned int *width, unsigned int *height)
-{
-	if (width)
-		*width = isp_dev->proc.mbus_format.width;
-	if (height)
-		*height = isp_dev->proc.mbus_format.height;
-}
-
 /* Format */
 
 static const struct sun6i_isp_proc_format sun6i_isp_proc_formats[] = {
@@ -137,9 +126,10 @@  static void sun6i_isp_proc_disable(struct sun6i_isp_device *isp_dev)
 	regmap_write(regmap, SUN6I_ISP_FE_CFG_REG, 0);
 }
 
-static void sun6i_isp_proc_configure(struct sun6i_isp_device *isp_dev)
+static void
+sun6i_isp_proc_configure(struct sun6i_isp_device *isp_dev,
+			 const struct v4l2_mbus_framefmt *mbus_format)
 {
-	struct v4l2_mbus_framefmt *mbus_format = &isp_dev->proc.mbus_format;
 	const struct sun6i_isp_proc_format *format;
 	u32 value;
 
@@ -173,6 +163,8 @@  static int sun6i_isp_proc_s_stream(struct v4l2_subdev *subdev, int on)
 	struct sun6i_isp_proc_source *source;
 	struct v4l2_subdev *source_subdev;
 	struct media_pad *remote_pad;
+	struct v4l2_subdev_state *state;
+	const struct v4l2_mbus_framefmt *mbus_format;
 	int ret;
 
 	/* Source */
@@ -191,6 +183,10 @@  static int sun6i_isp_proc_s_stream(struct v4l2_subdev *subdev, int on)
 	else
 		source = &proc->source_csi1;
 
+	/* Active State */
+
+	state = v4l2_subdev_lock_and_get_active_state(subdev);
+
 	if (!on) {
 		sun6i_isp_proc_irq_disable(isp_dev);
 		v4l2_subdev_call(source_subdev, video, s_stream, 0);
@@ -202,7 +198,7 @@  static int sun6i_isp_proc_s_stream(struct v4l2_subdev *subdev, int on)
 
 	ret = pm_runtime_resume_and_get(dev);
 	if (ret < 0)
-		return ret;
+		goto unlock;
 
 	/* Clear */
 
@@ -210,9 +206,12 @@  static int sun6i_isp_proc_s_stream(struct v4l2_subdev *subdev, int on)
 
 	/* Configure */
 
+	mbus_format = v4l2_subdev_state_get_format(state,
+						   SUN6I_ISP_PROC_PAD_SINK_CSI);
+
 	sun6i_isp_tables_configure(isp_dev);
-	sun6i_isp_params_configure(isp_dev);
-	sun6i_isp_proc_configure(isp_dev);
+	sun6i_isp_params_configure(isp_dev, mbus_format);
+	sun6i_isp_proc_configure(isp_dev, mbus_format);
 	sun6i_isp_capture_configure(isp_dev);
 
 	/* State Update */
@@ -230,13 +229,17 @@  static int sun6i_isp_proc_s_stream(struct v4l2_subdev *subdev, int on)
 		goto disable;
 	}
 
-	return 0;
+	ret = 0;
+	goto unlock;
 
 disable:
 	sun6i_isp_proc_disable(isp_dev);
 
 	pm_runtime_put(dev);
 
+unlock:
+	v4l2_subdev_unlock_state(state);
+
 	return ret;
 }
 
@@ -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;
 
-	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;
 }
@@ -291,53 +295,31 @@  sun6i_isp_proc_enum_mbus_code(struct v4l2_subdev *subdev,
 	return 0;
 }
 
-static int sun6i_isp_proc_get_fmt(struct v4l2_subdev *subdev,
-				  struct v4l2_subdev_state *state,
-				  struct v4l2_subdev_format *format)
-{
-	struct sun6i_isp_device *isp_dev = v4l2_get_subdevdata(subdev);
-	struct v4l2_mbus_framefmt *mbus_format = &format->format;
-	struct mutex *lock = &isp_dev->proc.lock;
-
-	mutex_lock(lock);
-
-	if (format->which == V4L2_SUBDEV_FORMAT_TRY)
-		*mbus_format = *v4l2_subdev_state_get_format(state,
-							     format->pad);
-	else
-		*mbus_format = isp_dev->proc.mbus_format;
-
-	mutex_unlock(lock);
-
-	return 0;
-}
-
 static int sun6i_isp_proc_set_fmt(struct v4l2_subdev *subdev,
 				  struct v4l2_subdev_state *state,
 				  struct v4l2_subdev_format *format)
 {
-	struct sun6i_isp_device *isp_dev = v4l2_get_subdevdata(subdev);
-	struct v4l2_mbus_framefmt *mbus_format = &format->format;
-	struct mutex *lock = &isp_dev->proc.lock;
+	struct v4l2_mbus_framefmt *mbus_format;
 
-	mutex_lock(lock);
+	if (format->pad != SUN6I_ISP_PROC_PAD_SINK_CSI)
+		return v4l2_subdev_get_fmt(subdev, state, format);
 
-	sun6i_isp_proc_mbus_format_prepare(mbus_format);
+	sun6i_isp_proc_mbus_format_prepare(&format->format);
 
-	if (format->which == V4L2_SUBDEV_FORMAT_TRY)
-		*v4l2_subdev_state_get_format(state, format->pad) =
-			*mbus_format;
-	else
-		isp_dev->proc.mbus_format = *mbus_format;
+	mbus_format = v4l2_subdev_state_get_format(state, format->pad);
+	*mbus_format = format->format;
 
-	mutex_unlock(lock);
+	/* Propagate the format to the source pad. */
+	mbus_format = v4l2_subdev_state_get_format(state,
+						   SUN6I_ISP_PROC_PAD_SOURCE);
+	*mbus_format = format->format;
 
 	return 0;
 }
 
 static const struct v4l2_subdev_pad_ops sun6i_isp_proc_pad_ops = {
 	.enum_mbus_code	= sun6i_isp_proc_enum_mbus_code,
-	.get_fmt	= sun6i_isp_proc_get_fmt,
+	.get_fmt	= v4l2_subdev_get_fmt,
 	.set_fmt	= sun6i_isp_proc_set_fmt,
 };
 
@@ -499,8 +481,6 @@  int sun6i_isp_proc_setup(struct sun6i_isp_device *isp_dev)
 	struct media_pad *pads = proc->pads;
 	int ret;
 
-	mutex_init(&proc->lock);
-
 	/* V4L2 Subdev */
 
 	v4l2_subdev_init(subdev, &sun6i_isp_proc_subdev_ops);
@@ -532,10 +512,14 @@  int sun6i_isp_proc_setup(struct sun6i_isp_device *isp_dev)
 
 	/* V4L2 Subdev */
 
+	ret = v4l2_subdev_init_finalize(subdev);
+	if (ret < 0)
+		goto error_media_entity;
+
 	ret = v4l2_device_register_subdev(v4l2_dev, subdev);
 	if (ret < 0) {
 		v4l2_err(v4l2_dev, "failed to register v4l2 subdev: %d\n", ret);
-		goto error_media_entity;
+		goto error_subdev_finalize;
 	}
 
 	/* V4L2 Async */
@@ -562,6 +546,9 @@  int sun6i_isp_proc_setup(struct sun6i_isp_device *isp_dev)
 
 	v4l2_device_unregister_subdev(subdev);
 
+error_subdev_finalize:
+	v4l2_subdev_cleanup(subdev);
+
 error_media_entity:
 	media_entity_cleanup(&subdev->entity);
 
@@ -577,5 +564,7 @@  void sun6i_isp_proc_cleanup(struct sun6i_isp_device *isp_dev)
 	v4l2_async_nf_cleanup(notifier);
 
 	v4l2_device_unregister_subdev(subdev);
+	v4l2_subdev_cleanup(subdev);
+
 	media_entity_cleanup(&subdev->entity);
 }
diff --git a/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_proc.h b/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_proc.h
index db6738a39147..26c4327c5ed7 100644
--- a/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_proc.h
+++ b/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_proc.h
@@ -42,18 +42,11 @@  struct sun6i_isp_proc {
 	struct v4l2_subdev		subdev;
 	struct media_pad		pads[3];
 	struct v4l2_async_notifier	notifier;
-	struct v4l2_mbus_framefmt	mbus_format;
-	struct mutex			lock; /* Mbus format lock. */
 
 	struct sun6i_isp_proc_source	source_csi0;
 	struct sun6i_isp_proc_source	source_csi1;
 };
 
-/* Helpers */
-
-void sun6i_isp_proc_dimensions(struct sun6i_isp_device *isp_dev,
-			       unsigned int *width, unsigned int *height);
-
 /* Format */
 
 const struct sun6i_isp_proc_format *sun6i_isp_proc_format_find(u32 mbus_code);