| Message ID | 20231023123308.782592-2-sakari.ailus@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Unify sub-device state access functions | expand |
Hi Sakari, Thank you for the patch. On Mon, Oct 23, 2023 at 03:33:03PM +0300, Sakari Ailus wrote: > There are two sets of functions that return information from sub-device > state, one for stream-unaware users and another for stream-aware users. > Add support for stream-aware functions to return format, crop and compose > information from pad-based array that are functionally equivalent to the > old, stream-unaware ones. > > Also check state is non-NULL, in order to guard against old drivers > potentially calling this with NULL state for active formats or selection > rectangles. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/v4l2-core/v4l2-subdev.c | 39 +++++++++++++++++++++++++++ > 1 file changed, 39 insertions(+) > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > index ee4fe8f33a41..955ee9a6c91f 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -1684,6 +1684,19 @@ v4l2_subdev_state_get_stream_format(struct v4l2_subdev_state *state, > struct v4l2_subdev_stream_configs *stream_configs; > unsigned int i; > > + if (WARN_ON(!state)) > + return NULL; > + > + if (state->pads) { > + if (stream) > + return NULL; > + > + if (WARN_ON(pad >= state->sd->entity.num_pads)) > + pad = 0; > + > + return &state->pads[pad].try_fmt; > + } > + > lockdep_assert_held(state->lock); Can we move towards proper locking for all callers ? > > stream_configs = &state->stream_configs; > @@ -1705,6 +1718,19 @@ v4l2_subdev_state_get_stream_crop(struct v4l2_subdev_state *state, > struct v4l2_subdev_stream_configs *stream_configs; > unsigned int i; > > + if (WARN_ON(!state)) > + return NULL; > + > + if (state->pads) { > + if (stream) > + return NULL; > + > + if (WARN_ON(pad >= state->sd->entity.num_pads)) > + pad = 0; > + > + return &state->pads[pad].try_crop; > + } > + > lockdep_assert_held(state->lock); > > stream_configs = &state->stream_configs; > @@ -1726,6 +1752,19 @@ v4l2_subdev_state_get_stream_compose(struct v4l2_subdev_state *state, > struct v4l2_subdev_stream_configs *stream_configs; > unsigned int i; > > + if (WARN_ON(!state)) > + return NULL; > + > + if (state->pads) { > + if (stream) > + return NULL; > + > + if (WARN_ON(pad >= state->sd->entity.num_pads)) > + pad = 0; > + > + return &state->pads[pad].try_compose; > + } > + > lockdep_assert_held(state->lock); > > stream_configs = &state->stream_configs; > -- > 2.39.2 >
On Mon, Oct 23, 2023 at 04:29:04PM +0300, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. > > On Mon, Oct 23, 2023 at 03:33:03PM +0300, Sakari Ailus wrote: > > There are two sets of functions that return information from sub-device > > state, one for stream-unaware users and another for stream-aware users. > > Add support for stream-aware functions to return format, crop and compose > > information from pad-based array that are functionally equivalent to the > > old, stream-unaware ones. > > > > Also check state is non-NULL, in order to guard against old drivers > > potentially calling this with NULL state for active formats or selection > > rectangles. > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > drivers/media/v4l2-core/v4l2-subdev.c | 39 +++++++++++++++++++++++++++ > > 1 file changed, 39 insertions(+) > > > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > > index ee4fe8f33a41..955ee9a6c91f 100644 > > --- a/drivers/media/v4l2-core/v4l2-subdev.c > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > > @@ -1684,6 +1684,19 @@ v4l2_subdev_state_get_stream_format(struct v4l2_subdev_state *state, > > struct v4l2_subdev_stream_configs *stream_configs; > > unsigned int i; > > > > + if (WARN_ON(!state)) > > + return NULL; > > + > > + if (state->pads) { > > + if (stream) > > + return NULL; > > + > > + if (WARN_ON(pad >= state->sd->entity.num_pads)) There's no sd field in struct v4l2_subdev_state in the linux media master branch, no mention of dependencies in the cover letter, and no specified base. Please generate patch series with --base. > > + pad = 0; > > + > > + return &state->pads[pad].try_fmt; > > + } > > + > > lockdep_assert_held(state->lock); > > Can we move towards proper locking for all callers ? > > > > > stream_configs = &state->stream_configs; > > @@ -1705,6 +1718,19 @@ v4l2_subdev_state_get_stream_crop(struct v4l2_subdev_state *state, > > struct v4l2_subdev_stream_configs *stream_configs; > > unsigned int i; > > > > + if (WARN_ON(!state)) > > + return NULL; > > + > > + if (state->pads) { > > + if (stream) > > + return NULL; > > + > > + if (WARN_ON(pad >= state->sd->entity.num_pads)) > > + pad = 0; > > + > > + return &state->pads[pad].try_crop; > > + } > > + > > lockdep_assert_held(state->lock); > > > > stream_configs = &state->stream_configs; > > @@ -1726,6 +1752,19 @@ v4l2_subdev_state_get_stream_compose(struct v4l2_subdev_state *state, > > struct v4l2_subdev_stream_configs *stream_configs; > > unsigned int i; > > > > + if (WARN_ON(!state)) > > + return NULL; > > + > > + if (state->pads) { > > + if (stream) > > + return NULL; > > + > > + if (WARN_ON(pad >= state->sd->entity.num_pads)) > > + pad = 0; > > + > > + return &state->pads[pad].try_compose; > > + } > > + > > lockdep_assert_held(state->lock); > > > > stream_configs = &state->stream_configs;
Hi Sakari,
kernel test robot noticed the following build errors:
[auto build test ERROR on media-tree/master]
[also build test ERROR on rockchip/for-next sailus-media-tree/streams linus/master v6.6-rc7 next-20231023]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Sakari-Ailus/media-v4l-subdev-Also-return-pads-array-information-on-stream-functions/20231023-203626
base: git://linuxtv.org/media_tree.git master
patch link: https://lore.kernel.org/r/20231023123308.782592-2-sakari.ailus%40linux.intel.com
patch subject: [PATCH v2 1/6] media: v4l: subdev: Also return pads array information on stream functions
config: csky-randconfig-002-20231023 (https://download.01.org/0day-ci/archive/20231023/202310232249.oyXxfG4x-lkp@intel.com/config)
compiler: csky-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231023/202310232249.oyXxfG4x-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310232249.oyXxfG4x-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from arch/csky/include/asm/bug.h:18,
from include/linux/bug.h:5,
from include/linux/thread_info.h:13,
from include/asm-generic/current.h:6,
from ./arch/csky/include/generated/asm/current.h:1,
from include/linux/sched.h:12,
from include/linux/ratelimit.h:6,
from include/linux/dev_printk.h:16,
from include/linux/device.h:15,
from include/linux/leds.h:12,
from drivers/media/v4l2-core/v4l2-subdev.c:13:
drivers/media/v4l2-core/v4l2-subdev.c: In function 'v4l2_subdev_state_get_stream_format':
>> drivers/media/v4l2-core/v4l2-subdev.c:1692:41: error: 'struct v4l2_subdev_state' has no member named 'sd'
1692 | if (WARN_ON(pad >= state->sd->entity.num_pads))
| ^~
include/asm-generic/bug.h:123:32: note: in definition of macro 'WARN_ON'
123 | int __ret_warn_on = !!(condition); \
| ^~~~~~~~~
drivers/media/v4l2-core/v4l2-subdev.c: In function 'v4l2_subdev_state_get_stream_crop':
drivers/media/v4l2-core/v4l2-subdev.c:1726:41: error: 'struct v4l2_subdev_state' has no member named 'sd'
1726 | if (WARN_ON(pad >= state->sd->entity.num_pads))
| ^~
include/asm-generic/bug.h:123:32: note: in definition of macro 'WARN_ON'
123 | int __ret_warn_on = !!(condition); \
| ^~~~~~~~~
drivers/media/v4l2-core/v4l2-subdev.c: In function 'v4l2_subdev_state_get_stream_compose':
drivers/media/v4l2-core/v4l2-subdev.c:1760:41: error: 'struct v4l2_subdev_state' has no member named 'sd'
1760 | if (WARN_ON(pad >= state->sd->entity.num_pads))
| ^~
include/asm-generic/bug.h:123:32: note: in definition of macro 'WARN_ON'
123 | int __ret_warn_on = !!(condition); \
| ^~~~~~~~~
vim +1692 drivers/media/v4l2-core/v4l2-subdev.c
1677
1678 struct v4l2_mbus_framefmt *
1679 v4l2_subdev_state_get_stream_format(struct v4l2_subdev_state *state,
1680 unsigned int pad, u32 stream)
1681 {
1682 struct v4l2_subdev_stream_configs *stream_configs;
1683 unsigned int i;
1684
1685 if (WARN_ON(!state))
1686 return NULL;
1687
1688 if (state->pads) {
1689 if (stream)
1690 return NULL;
1691
> 1692 if (WARN_ON(pad >= state->sd->entity.num_pads))
1693 pad = 0;
1694
1695 return &state->pads[pad].try_fmt;
1696 }
1697
1698 lockdep_assert_held(state->lock);
1699
1700 stream_configs = &state->stream_configs;
1701
1702 for (i = 0; i < stream_configs->num_configs; ++i) {
1703 if (stream_configs->configs[i].pad == pad &&
1704 stream_configs->configs[i].stream == stream)
1705 return &stream_configs->configs[i].fmt;
1706 }
1707
1708 return NULL;
1709 }
1710 EXPORT_SYMBOL_GPL(v4l2_subdev_state_get_stream_format);
1711
Hi Laurent, On Mon, Oct 23, 2023 at 05:08:45PM +0300, Laurent Pinchart wrote: > > > + if (WARN_ON(pad >= state->sd->entity.num_pads)) > > There's no sd field in struct v4l2_subdev_state in the linux media > master branch, no mention of dependencies in the cover letter, and no > specified base. > > Please generate patch series with --base. That wouldn't help. But I've realised what the problem is. I forgot to include the first patch. There were six patches, but one added made that seven... I'll send v3, addressing comments found in v2.
Hi Laurent, On Mon, Oct 23, 2023 at 04:29:02PM +0300, Laurent Pinchart wrote: > > + return NULL; > > + > > + if (state->pads) { > > + if (stream) > > + return NULL; > > + > > + if (WARN_ON(pad >= state->sd->entity.num_pads)) > > + pad = 0; > > + > > + return &state->pads[pad].try_fmt; > > + } > > + > > lockdep_assert_held(state->lock); > > Can we move towards proper locking for all callers ? This was never done in the existing pad ops. That's certainly a good idea, but it belongs to a new patch. I'll add one.
On Mon, Oct 23, 2023 at 05:01:48PM +0000, Sakari Ailus wrote: > Hi Laurent, > > On Mon, Oct 23, 2023 at 05:08:45PM +0300, Laurent Pinchart wrote: > > > > + if (WARN_ON(pad >= state->sd->entity.num_pads)) > > > > There's no sd field in struct v4l2_subdev_state in the linux media > > master branch, no mention of dependencies in the cover letter, and no > > specified base. > > > > Please generate patch series with --base. > > That wouldn't help. Why not ? > But I've realised what the problem is. I forgot to include the first patch. > There were six patches, but one added made that seven... > > I'll send v3, addressing comments found in v2.
On Mon, Oct 23, 2023 at 08:09:53PM +0300, Laurent Pinchart wrote: > On Mon, Oct 23, 2023 at 05:01:48PM +0000, Sakari Ailus wrote: > > Hi Laurent, > > > > On Mon, Oct 23, 2023 at 05:08:45PM +0300, Laurent Pinchart wrote: > > > > > + if (WARN_ON(pad >= state->sd->entity.num_pads)) > > > > > > There's no sd field in struct v4l2_subdev_state in the linux media > > > master branch, no mention of dependencies in the cover letter, and no > > > specified base. > > > > > > Please generate patch series with --base. > > > > That wouldn't help. > > Why not ? Because the patch the rest of the set depends on is missing. The base for the patches is indeed the media stage tree (or close to that). > > > But I've realised what the problem is. I forgot to include the first patch. > > There were six patches, but one added made that seven... > > > > I'll send v3, addressing comments found in v2. > > -- > Regards, > > Laurent Pinchart
On Mon, Oct 23, 2023 at 05:18:35PM +0000, Sakari Ailus wrote: > On Mon, Oct 23, 2023 at 08:09:53PM +0300, Laurent Pinchart wrote: > > On Mon, Oct 23, 2023 at 05:01:48PM +0000, Sakari Ailus wrote: > > > Hi Laurent, > > > > > > On Mon, Oct 23, 2023 at 05:08:45PM +0300, Laurent Pinchart wrote: > > > > > > + if (WARN_ON(pad >= state->sd->entity.num_pads)) > > > > > > > > There's no sd field in struct v4l2_subdev_state in the linux media > > > > master branch, no mention of dependencies in the cover letter, and no > > > > specified base. > > > > > > > > Please generate patch series with --base. > > > > > > That wouldn't help. > > > > Why not ? > > Because the patch the rest of the set depends on is missing. The base for > the patches is indeed the media stage tree (or close to that). The cover letter will then mention that. Try it out :-) > > > But I've realised what the problem is. I forgot to include the first patch. > > > There were six patches, but one added made that seven... > > > > > > I'll send v3, addressing comments found in v2.
On Mon, Oct 23, 2023 at 08:29:38PM +0300, Laurent Pinchart wrote: > On Mon, Oct 23, 2023 at 05:18:35PM +0000, Sakari Ailus wrote: > > On Mon, Oct 23, 2023 at 08:09:53PM +0300, Laurent Pinchart wrote: > > > On Mon, Oct 23, 2023 at 05:01:48PM +0000, Sakari Ailus wrote: > > > > Hi Laurent, > > > > > > > > On Mon, Oct 23, 2023 at 05:08:45PM +0300, Laurent Pinchart wrote: > > > > > > > + if (WARN_ON(pad >= state->sd->entity.num_pads)) > > > > > > > > > > There's no sd field in struct v4l2_subdev_state in the linux media > > > > > master branch, no mention of dependencies in the cover letter, and no > > > > > specified base. > > > > > > > > > > Please generate patch series with --base. > > > > > > > > That wouldn't help. > > > > > > Why not ? > > > > Because the patch the rest of the set depends on is missing. The base for > > the patches is indeed the media stage tree (or close to that). > > The cover letter will then mention that. Try it out :-) I can use it in v3 but that alone will not solve the problem. :-)
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c index ee4fe8f33a41..955ee9a6c91f 100644 --- a/drivers/media/v4l2-core/v4l2-subdev.c +++ b/drivers/media/v4l2-core/v4l2-subdev.c @@ -1684,6 +1684,19 @@ v4l2_subdev_state_get_stream_format(struct v4l2_subdev_state *state, struct v4l2_subdev_stream_configs *stream_configs; unsigned int i; + if (WARN_ON(!state)) + return NULL; + + if (state->pads) { + if (stream) + return NULL; + + if (WARN_ON(pad >= state->sd->entity.num_pads)) + pad = 0; + + return &state->pads[pad].try_fmt; + } + lockdep_assert_held(state->lock); stream_configs = &state->stream_configs; @@ -1705,6 +1718,19 @@ v4l2_subdev_state_get_stream_crop(struct v4l2_subdev_state *state, struct v4l2_subdev_stream_configs *stream_configs; unsigned int i; + if (WARN_ON(!state)) + return NULL; + + if (state->pads) { + if (stream) + return NULL; + + if (WARN_ON(pad >= state->sd->entity.num_pads)) + pad = 0; + + return &state->pads[pad].try_crop; + } + lockdep_assert_held(state->lock); stream_configs = &state->stream_configs; @@ -1726,6 +1752,19 @@ v4l2_subdev_state_get_stream_compose(struct v4l2_subdev_state *state, struct v4l2_subdev_stream_configs *stream_configs; unsigned int i; + if (WARN_ON(!state)) + return NULL; + + if (state->pads) { + if (stream) + return NULL; + + if (WARN_ON(pad >= state->sd->entity.num_pads)) + pad = 0; + + return &state->pads[pad].try_compose; + } + lockdep_assert_held(state->lock); stream_configs = &state->stream_configs;
There are two sets of functions that return information from sub-device state, one for stream-unaware users and another for stream-aware users. Add support for stream-aware functions to return format, crop and compose information from pad-based array that are functionally equivalent to the old, stream-unaware ones. Also check state is non-NULL, in order to guard against old drivers potentially calling this with NULL state for active formats or selection rectangles. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- drivers/media/v4l2-core/v4l2-subdev.c | 39 +++++++++++++++++++++++++++ 1 file changed, 39 insertions(+)