| Message ID | 20200403213312.1863876-2-helen.koike@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: add v4l2_pipeline_stream_{enable,disable} helpers | expand |
Hi Helen,
I love your patch! Yet something to improve:
[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on v5.6 next-20200404]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Helen-Koike/media-add-v4l2_pipeline_stream_-enable-disable-helpers/20200405-050004
base: git://linuxtv.org/media_tree.git master
config: s390-randconfig-a001-20200405 (attached as .config)
compiler: s390-linux-gcc (GCC) 9.3.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=9.3.0 make.cross ARCH=s390
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>
All error/warnings (new ones prefixed by >>):
drivers/media/v4l2-core/v4l2-common.c: In function 'v4l2_pipeline_subdevs_get':
>> drivers/media/v4l2-core/v4l2-common.c:463:37: error: 'struct video_device' has no member named 'entity'
463 | struct media_entity *entity = &vdev->entity;
| ^~
In file included from include/linux/kernel.h:11,
from include/linux/list.h:9,
from include/linux/module.h:12,
from drivers/media/v4l2-core/v4l2-common.c:37:
>> include/linux/kernel.h:987:51: error: 'struct v4l2_subdev' has no member named 'entity'
987 | BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
| ^~
include/linux/compiler.h:330:9: note: in definition of macro '__compiletime_assert'
330 | if (!(condition)) \
| ^~~~~~~~~
include/linux/compiler.h:350:2: note: in expansion of macro '_compiletime_assert'
350 | _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
| ^~~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:39:37: note: in expansion of macro 'compiletime_assert'
39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
| ^~~~~~~~~~~~~~~~~~
include/linux/kernel.h:987:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
987 | BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
| ^~~~~~~~~~~~~~~~
include/linux/kernel.h:987:20: note: in expansion of macro '__same_type'
987 | BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
| ^~~~~~~~~~~
>> include/media/v4l2-subdev.h:888:3: note: in expansion of macro 'container_of'
888 | container_of(__me_sd_ent, struct v4l2_subdev, entity) : \
| ^~~~~~~~~~~~
>> drivers/media/v4l2-core/v4l2-common.c:491:20: note: in expansion of macro 'media_entity_to_v4l2_subdev'
491 | subdevs[idx++] = media_entity_to_v4l2_subdev(entity);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from <command-line>:
>> include/linux/compiler_types.h:129:35: error: 'struct v4l2_subdev' has no member named 'entity'
129 | #define __compiler_offsetof(a, b) __builtin_offsetof(a, b)
| ^~~~~~~~~~~~~~~~~~
include/linux/stddef.h:17:32: note: in expansion of macro '__compiler_offsetof'
17 | #define offsetof(TYPE, MEMBER) __compiler_offsetof(TYPE, MEMBER)
| ^~~~~~~~~~~~~~~~~~~
include/linux/kernel.h:990:21: note: in expansion of macro 'offsetof'
990 | ((type *)(__mptr - offsetof(type, member))); })
| ^~~~~~~~
>> include/media/v4l2-subdev.h:888:3: note: in expansion of macro 'container_of'
888 | container_of(__me_sd_ent, struct v4l2_subdev, entity) : \
| ^~~~~~~~~~~~
>> drivers/media/v4l2-core/v4l2-common.c:491:20: note: in expansion of macro 'media_entity_to_v4l2_subdev'
491 | subdevs[idx++] = media_entity_to_v4l2_subdev(entity);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/media/v4l2-core/v4l2-common.c: In function 'v4l2_pipeline_stream_enable':
drivers/media/v4l2-core/v4l2-common.c:499:34: error: 'struct video_device' has no member named 'entity'
499 | struct media_device *mdev = vdev->entity.graph_obj.mdev;
| ^~
In file included from include/linux/device.h:15,
from include/media/v4l2-dev.h:15,
from include/media/v4l2-common.h:18,
from drivers/media/v4l2-core/v4l2-common.c:47:
>> drivers/media/v4l2-core/v4l2-common.c:514:36: error: 'struct v4l2_subdev' has no member named 'entity'
514 | "enabling stream for '%s'\n", sd->entity.name);
| ^~
include/linux/dev_printk.h:122:47: note: in definition of macro 'dev_dbg'
122 | dev_printk(KERN_DEBUG, dev, dev_fmt(fmt), ##__VA_ARGS__); \
| ^~~~~~~~~~~
drivers/media/v4l2-core/v4l2-common.c:529:37: error: 'struct v4l2_subdev' has no member named 'entity'
529 | "disabling stream for '%s'\n", sd->entity.name);
| ^~
include/linux/dev_printk.h:122:47: note: in definition of macro 'dev_dbg'
122 | dev_printk(KERN_DEBUG, dev, dev_fmt(fmt), ##__VA_ARGS__); \
| ^~~~~~~~~~~
drivers/media/v4l2-core/v4l2-common.c: In function 'v4l2_pipeline_stream_disable':
drivers/media/v4l2-core/v4l2-common.c:540:34: error: 'struct video_device' has no member named 'entity'
540 | struct media_device *mdev = vdev->entity.graph_obj.mdev;
| ^~
In file included from include/linux/device.h:15,
from include/media/v4l2-dev.h:15,
from include/media/v4l2-common.h:18,
from drivers/media/v4l2-core/v4l2-common.c:47:
drivers/media/v4l2-core/v4l2-common.c:554:37: error: 'struct v4l2_subdev' has no member named 'entity'
554 | "disabling stream for '%s'\n", sd->entity.name);
| ^~
include/linux/dev_printk.h:122:47: note: in definition of macro 'dev_dbg'
122 | dev_printk(KERN_DEBUG, dev, dev_fmt(fmt), ##__VA_ARGS__); \
| ^~~~~~~~~~~
vim +463 drivers/media/v4l2-core/v4l2-common.c
444
445 /*
446 * v4l2_pipeline_subdevs_get - Assemble a list of subdevices in a pipeline
447 * @subdevs: the array to be filled
448 * @size: the array size
449 *
450 * Walk from a video node, following link from sink to source and fill the
451 * array with subdevices in the path.
452 *
453 * Note: this function follows the first enabled link in a sink pad found in a
454 * given entity. Thus it won't work if there are entities with multiple enabled
455 * links to its sink pads in the topology.
456 *
457 * Return the number of subdevices filled in the array.
458 */
459 static unsigned int v4l2_pipeline_subdevs_get(struct video_device *vdev,
460 struct v4l2_subdev **subdevs,
461 unsigned int size)
462 {
> 463 struct media_entity *entity = &vdev->entity;
464 unsigned int idx = 0;
465
466 while (1) {
467 struct media_pad *src_pad = NULL;
468 unsigned int i;
469
470 /* Find remote source pad */
471 for (i = 0; i < entity->num_pads; i++) {
472 struct media_pad *sink_pad = &entity->pads[i];
473
474 if (!(sink_pad->flags & MEDIA_PAD_FL_SINK))
475 continue;
476
477 src_pad = media_entity_remote_pad(sink_pad);
478 if (src_pad &&
479 is_media_entity_v4l2_subdev(src_pad->entity))
480 break;
481 }
482 if (i == entity->num_pads)
483 break;
484
485 if (idx >= size) {
486 WARN_ON(1);
487 return 0;
488 }
489
490 entity = src_pad->entity;
> 491 subdevs[idx++] = media_entity_to_v4l2_subdev(entity);
492 }
493
494 return idx;
495 }
496
497 __must_check int v4l2_pipeline_stream_enable(struct video_device *vdev)
498 {
> 499 struct media_device *mdev = vdev->entity.graph_obj.mdev;
500 struct v4l2_subdev *subdevs[MEDIA_ENTITY_ENUM_MAX_DEPTH];
501 struct v4l2_subdev *sd;
502 unsigned int i, size;
503 int ret;
504
505 mutex_lock(&mdev->graph_mutex);
506
507 size = v4l2_pipeline_subdevs_get(vdev, subdevs, ARRAY_SIZE(subdevs));
508
509 for (i = 0; i < size; i++) {
510 sd = subdevs[i];
511 if (sd->stream_count++)
512 continue;
513 dev_dbg(mdev->dev,
> 514 "enabling stream for '%s'\n", sd->entity.name);
515 ret = v4l2_subdev_call(sd, video, s_stream, true);
516 if (ret && ret != -ENOIOCTLCMD)
517 goto err_stream_disable;
518 }
519
520 mutex_unlock(&mdev->graph_mutex);
521 return 0;
522
523 err_stream_disable:
524 do {
525 sd = subdevs[i];
526 if (--sd->stream_count)
527 continue;
528 dev_dbg(mdev->dev,
529 "disabling stream for '%s'\n", sd->entity.name);
530 v4l2_subdev_call(sd, video, s_stream, false);
531 } while (i--);
532
533 mutex_unlock(&mdev->graph_mutex);
534 return ret;
535 }
536 EXPORT_SYMBOL_GPL(v4l2_pipeline_stream_enable);
537
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Helen,
I love your patch! Perhaps something to improve:
[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v5.6 next-20200404]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Helen-Koike/media-add-v4l2_pipeline_stream_-enable-disable-helpers/20200405-050004
base: git://linuxtv.org/media_tree.git master
config: x86_64-randconfig-g003-20200405 (attached as .config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/media/v4l2-core/v4l2-common.c: In function 'v4l2_pipeline_subdevs_get':
drivers/media/v4l2-core/v4l2-common.c:463:37: error: 'struct video_device' has no member named 'entity'
struct media_entity *entity = &vdev->entity;
^~
In file included from include/linux/export.h:43:0,
from include/linux/linkage.h:7,
from include/linux/kernel.h:8,
from include/linux/list.h:9,
from include/linux/module.h:12,
from drivers/media/v4l2-core/v4l2-common.c:37:
include/linux/kernel.h:987:51: error: 'struct v4l2_subdev' has no member named 'entity'
BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
^
include/linux/compiler.h:330:9: note: in definition of macro '__compiletime_assert'
if (!(condition)) \
^~~~~~~~~
include/linux/compiler.h:350:2: note: in expansion of macro '_compiletime_assert'
_compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
^~~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:39:37: note: in expansion of macro 'compiletime_assert'
#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
^~~~~~~~~~~~~~~~~~
include/linux/kernel.h:987:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
^~~~~~~~~~~~~~~~
include/linux/kernel.h:987:20: note: in expansion of macro '__same_type'
BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
^~~~~~~~~~~
include/media/v4l2-subdev.h:888:3: note: in expansion of macro 'container_of'
container_of(__me_sd_ent, struct v4l2_subdev, entity) : \
^~~~~~~~~~~~
drivers/media/v4l2-core/v4l2-common.c:491:20: note: in expansion of macro 'media_entity_to_v4l2_subdev'
subdevs[idx++] = media_entity_to_v4l2_subdev(entity);
^~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from <command-line>:0:0:
include/linux/compiler_types.h:129:35: error: 'struct v4l2_subdev' has no member named 'entity'
#define __compiler_offsetof(a, b) __builtin_offsetof(a, b)
^
include/linux/stddef.h:17:32: note: in expansion of macro '__compiler_offsetof'
#define offsetof(TYPE, MEMBER) __compiler_offsetof(TYPE, MEMBER)
^~~~~~~~~~~~~~~~~~~
include/linux/kernel.h:990:21: note: in expansion of macro 'offsetof'
((type *)(__mptr - offsetof(type, member))); })
^~~~~~~~
include/media/v4l2-subdev.h:888:3: note: in expansion of macro 'container_of'
container_of(__me_sd_ent, struct v4l2_subdev, entity) : \
^~~~~~~~~~~~
drivers/media/v4l2-core/v4l2-common.c:491:20: note: in expansion of macro 'media_entity_to_v4l2_subdev'
subdevs[idx++] = media_entity_to_v4l2_subdev(entity);
^~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/media/v4l2-core/v4l2-common.c: In function 'v4l2_pipeline_stream_enable':
drivers/media/v4l2-core/v4l2-common.c:499:34: error: 'struct video_device' has no member named 'entity'
struct media_device *mdev = vdev->entity.graph_obj.mdev;
^~
In file included from include/linux/printk.h:331:0,
from include/linux/kernel.h:15,
from include/linux/list.h:9,
from include/linux/module.h:12,
from drivers/media/v4l2-core/v4l2-common.c:37:
drivers/media/v4l2-core/v4l2-common.c:514:36: error: 'struct v4l2_subdev' has no member named 'entity'
"enabling stream for '%s'\n", sd->entity.name);
^
include/linux/dynamic_debug.h:125:15: note: in definition of macro '__dynamic_func_call'
func(&id, ##__VA_ARGS__); \
^~~~~~~~~~~
include/linux/dynamic_debug.h:157:2: note: in expansion of macro '_dynamic_func_call'
_dynamic_func_call(fmt,__dynamic_dev_dbg, \
^~~~~~~~~~~~~~~~~~
include/linux/dev_printk.h:114:2: note: in expansion of macro 'dynamic_dev_dbg'
dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
^~~~~~~~~~~~~~~
>> drivers/media/v4l2-core/v4l2-common.c:513:3: note: in expansion of macro 'dev_dbg'
dev_dbg(mdev->dev,
^~~~~~~
drivers/media/v4l2-core/v4l2-common.c:529:37: error: 'struct v4l2_subdev' has no member named 'entity'
"disabling stream for '%s'\n", sd->entity.name);
^
include/linux/dynamic_debug.h:125:15: note: in definition of macro '__dynamic_func_call'
func(&id, ##__VA_ARGS__); \
^~~~~~~~~~~
include/linux/dynamic_debug.h:157:2: note: in expansion of macro '_dynamic_func_call'
_dynamic_func_call(fmt,__dynamic_dev_dbg, \
^~~~~~~~~~~~~~~~~~
include/linux/dev_printk.h:114:2: note: in expansion of macro 'dynamic_dev_dbg'
dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
^~~~~~~~~~~~~~~
drivers/media/v4l2-core/v4l2-common.c:528:3: note: in expansion of macro 'dev_dbg'
dev_dbg(mdev->dev,
^~~~~~~
drivers/media/v4l2-core/v4l2-common.c: In function 'v4l2_pipeline_stream_disable':
drivers/media/v4l2-core/v4l2-common.c:540:34: error: 'struct video_device' has no member named 'entity'
struct media_device *mdev = vdev->entity.graph_obj.mdev;
^~
In file included from include/linux/printk.h:331:0,
from include/linux/kernel.h:15,
from include/linux/list.h:9,
from include/linux/module.h:12,
from drivers/media/v4l2-core/v4l2-common.c:37:
drivers/media/v4l2-core/v4l2-common.c:554:37: error: 'struct v4l2_subdev' has no member named 'entity'
"disabling stream for '%s'\n", sd->entity.name);
^
include/linux/dynamic_debug.h:125:15: note: in definition of macro '__dynamic_func_call'
func(&id, ##__VA_ARGS__); \
^~~~~~~~~~~
include/linux/dynamic_debug.h:157:2: note: in expansion of macro '_dynamic_func_call'
_dynamic_func_call(fmt,__dynamic_dev_dbg, \
^~~~~~~~~~~~~~~~~~
include/linux/dev_printk.h:114:2: note: in expansion of macro 'dynamic_dev_dbg'
dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
^~~~~~~~~~~~~~~
drivers/media/v4l2-core/v4l2-common.c:553:3: note: in expansion of macro 'dev_dbg'
dev_dbg(mdev->dev,
^~~~~~~
vim +/dev_dbg +513 drivers/media/v4l2-core/v4l2-common.c
496
497 __must_check int v4l2_pipeline_stream_enable(struct video_device *vdev)
498 {
499 struct media_device *mdev = vdev->entity.graph_obj.mdev;
500 struct v4l2_subdev *subdevs[MEDIA_ENTITY_ENUM_MAX_DEPTH];
501 struct v4l2_subdev *sd;
502 unsigned int i, size;
503 int ret;
504
505 mutex_lock(&mdev->graph_mutex);
506
507 size = v4l2_pipeline_subdevs_get(vdev, subdevs, ARRAY_SIZE(subdevs));
508
509 for (i = 0; i < size; i++) {
510 sd = subdevs[i];
511 if (sd->stream_count++)
512 continue;
> 513 dev_dbg(mdev->dev,
514 "enabling stream for '%s'\n", sd->entity.name);
515 ret = v4l2_subdev_call(sd, video, s_stream, true);
516 if (ret && ret != -ENOIOCTLCMD)
517 goto err_stream_disable;
518 }
519
520 mutex_unlock(&mdev->graph_mutex);
521 return 0;
522
523 err_stream_disable:
524 do {
525 sd = subdevs[i];
526 if (--sd->stream_count)
527 continue;
528 dev_dbg(mdev->dev,
529 "disabling stream for '%s'\n", sd->entity.name);
530 v4l2_subdev_call(sd, video, s_stream, false);
531 } while (i--);
532
533 mutex_unlock(&mdev->graph_mutex);
534 return ret;
535 }
536 EXPORT_SYMBOL_GPL(v4l2_pipeline_stream_enable);
537
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Helen, Thanks for your work. On 2020-04-03 18:33:10 -0300, Helen Koike wrote: > Add v4l2_pipeline_stream_{enable,disable} helper functions to iterate > through the subdevices in a given stream (i.e following links from sink > to source) and call .s_stream() callback. > > Add stream_count on the subdevice object for simultaneous streaming from > different video devices which shares subdevices. > > Prevent calling .s_stream(true) if it was already called previously by > another stream. > > Prevent calling .s_stream(false) from one stream when there are still > others active. > > If .s_stream(true) fails, call .s_stream(false) in the reverse order. > > Signed-off-by: Helen Koike <helen.koike@collabora.com> > > --- > > Changes in v2: > - re-write helpers to not use media walkers > > drivers/media/v4l2-core/v4l2-common.c | 117 ++++++++++++++++++++++++++ > include/media/v4l2-common.h | 28 ++++++ > include/media/v4l2-subdev.h | 2 + > 3 files changed, 147 insertions(+) > > diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c > index d0e5ebc736f9f..379d4bf4f8128 100644 > --- a/drivers/media/v4l2-core/v4l2-common.c > +++ b/drivers/media/v4l2-core/v4l2-common.c > @@ -441,3 +441,120 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat, > return 0; > } > EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt); > + > +/* > + * v4l2_pipeline_subdevs_get - Assemble a list of subdevices in a pipeline > + * @subdevs: the array to be filled > + * @size: the array size Should this be documented as the maximum number of elements that can fit in the subdevs array? > + * > + * Walk from a video node, following link from sink to source and fill the > + * array with subdevices in the path. > + * > + * Note: this function follows the first enabled link in a sink pad found in a > + * given entity. Thus it won't work if there are entities with multiple enabled > + * links to its sink pads in the topology. I wonder if it would be more useful to make this function return all subdevs in the pipeline that have an enabled link going from sink to source while walking from the video device? When reading the commit messages I thought this could be useful for the rcar-vin driver. By not finding all subdevices in the pipeline this would not be possible as there on some platforms are a CSI-2 bus where the CSI-2 transmitter have 4 sink pads and one source pads so the whole pipeline would not be started. > + * > + * Return the number of subdevices filled in the array. > + */ > +static unsigned int v4l2_pipeline_subdevs_get(struct video_device *vdev, > + struct v4l2_subdev **subdevs, > + unsigned int size) > +{ > + struct media_entity *entity = &vdev->entity; > + unsigned int idx = 0; > + > + while (1) { > + struct media_pad *src_pad = NULL; > + unsigned int i; > + > + /* Find remote source pad */ > + for (i = 0; i < entity->num_pads; i++) { > + struct media_pad *sink_pad = &entity->pads[i]; > + > + if (!(sink_pad->flags & MEDIA_PAD_FL_SINK)) > + continue; > + > + src_pad = media_entity_remote_pad(sink_pad); > + if (src_pad && > + is_media_entity_v4l2_subdev(src_pad->entity)) > + break; > + } > + if (i == entity->num_pads) > + break; > + > + if (idx >= size) { > + WARN_ON(1); > + return 0; Would it make sens to have this function return int and a negative error code here? Is this now how other areas of V4L2 deal with when a provided array is too small ? I'm thinking about if this function could be exported for use by drivers in the future. > + } > + > + entity = src_pad->entity; > + subdevs[idx++] = media_entity_to_v4l2_subdev(entity); > + } > + > + return idx; > +} > + > +__must_check int v4l2_pipeline_stream_enable(struct video_device *vdev) > +{ > + struct media_device *mdev = vdev->entity.graph_obj.mdev; > + struct v4l2_subdev *subdevs[MEDIA_ENTITY_ENUM_MAX_DEPTH]; > + struct v4l2_subdev *sd; > + unsigned int i, size; > + int ret; > + > + mutex_lock(&mdev->graph_mutex); > + > + size = v4l2_pipeline_subdevs_get(vdev, subdevs, ARRAY_SIZE(subdevs)); > + > + for (i = 0; i < size; i++) { > + sd = subdevs[i]; > + if (sd->stream_count++) > + continue; > + dev_dbg(mdev->dev, > + "enabling stream for '%s'\n", sd->entity.name); Small nit, would it make sens to print the sd->stream_count also in this debug statement (and the similar ones bellow) ? > + ret = v4l2_subdev_call(sd, video, s_stream, true); > + if (ret && ret != -ENOIOCTLCMD) > + goto err_stream_disable; > + } > + > + mutex_unlock(&mdev->graph_mutex); > + return 0; > + > +err_stream_disable: > + do { > + sd = subdevs[i]; > + if (--sd->stream_count) > + continue; > + dev_dbg(mdev->dev, > + "disabling stream for '%s'\n", sd->entity.name); > + v4l2_subdev_call(sd, video, s_stream, false); > + } while (i--); > + > + mutex_unlock(&mdev->graph_mutex); > + return ret; > +} > +EXPORT_SYMBOL_GPL(v4l2_pipeline_stream_enable); > + > +void v4l2_pipeline_stream_disable(struct video_device *vdev) > +{ > + struct media_device *mdev = vdev->entity.graph_obj.mdev; > + struct v4l2_subdev *subdevs[MEDIA_ENTITY_ENUM_MAX_DEPTH]; > + unsigned int size; > + > + mutex_lock(&mdev->graph_mutex); > + > + size = v4l2_pipeline_subdevs_get(vdev, subdevs, ARRAY_SIZE(subdevs)); > + > + while (size--) { > + struct v4l2_subdev *sd = subdevs[size]; > + > + if (--sd->stream_count) > + continue; > + dev_dbg(mdev->dev, > + "disabling stream for '%s'\n", sd->entity.name); > + v4l2_subdev_call(sd, video, s_stream, false); > + } > + > + mutex_unlock(&mdev->graph_mutex); > +} > +EXPORT_SYMBOL_GPL(v4l2_pipeline_stream_disable); > diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h > index 150ee16ebd811..e833610b0f66d 100644 > --- a/include/media/v4l2-common.h > +++ b/include/media/v4l2-common.h > @@ -519,6 +519,34 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat, > int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, u32 pixelformat, > u32 width, u32 height); > > +/** > + * v4l2_pipeline_stream_enable - Call .s_stream(true) callbacks in the stream > + * @vdev: Starting video device > + * > + * Call .s_stream(true) callback in all the subdevices participating in the > + * stream, i.e. following links from sink to source. > + * > + * Calls to this function can be nested, in which case the same number of > + * v4l2_pipeline_stream_disable() calls will be required to stop streaming. > + * The pipeline pointer must be identical for all nested calls to > + * v4l2_pipeline_stream_enable(). > + */ > +__must_check int v4l2_pipeline_stream_enable(struct video_device *vdev); > + > +/** > + * v4l2_pipeline_stream_disable - Call .s_stream(false) callbacks in the stream > + * @vdev: Starting video device > + * > + * Call .s_stream(true) callback in all the subdevices participating in the > + * stream, i.e. following links from sink to source. > + * > + * Calls to this function can be nested, in which case the same number of > + * v4l2_pipeline_stream_disable() calls will be required to stop streaming. > + * The pipeline pointer must be identical for all nested calls to > + * v4l2_pipeline_stream_enable(). > + */ > +void v4l2_pipeline_stream_disable(struct video_device *vdev); > + > static inline u64 v4l2_buffer_get_timestamp(const struct v4l2_buffer *buf) > { > /* > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > index a4848de598521..20f913a9f70c5 100644 > --- a/include/media/v4l2-subdev.h > +++ b/include/media/v4l2-subdev.h > @@ -838,6 +838,7 @@ struct v4l2_subdev_platform_data { > * @subdev_notifier: A sub-device notifier implicitly registered for the sub- > * device using v4l2_device_register_sensor_subdev(). > * @pdata: common part of subdevice platform data > + * @stream_count: Stream count for the subdevice. > * > * Each instance of a subdev driver should create this struct, either > * stand-alone or embedded in a larger struct. > @@ -869,6 +870,7 @@ struct v4l2_subdev { > struct v4l2_async_notifier *notifier; > struct v4l2_async_notifier *subdev_notifier; > struct v4l2_subdev_platform_data *pdata; > + unsigned int stream_count; > }; > > > -- > 2.26.0 >
Hi Niklas, Thanks for your review. On 4/7/20 4:24 PM, Niklas Söderlund wrote: > Hi Helen, > > Thanks for your work. > > On 2020-04-03 18:33:10 -0300, Helen Koike wrote: >> Add v4l2_pipeline_stream_{enable,disable} helper functions to iterate >> through the subdevices in a given stream (i.e following links from sink >> to source) and call .s_stream() callback. >> >> Add stream_count on the subdevice object for simultaneous streaming from >> different video devices which shares subdevices. >> >> Prevent calling .s_stream(true) if it was already called previously by >> another stream. >> >> Prevent calling .s_stream(false) from one stream when there are still >> others active. >> >> If .s_stream(true) fails, call .s_stream(false) in the reverse order. >> >> Signed-off-by: Helen Koike <helen.koike@collabora.com> >> >> --- >> >> Changes in v2: >> - re-write helpers to not use media walkers >> >> drivers/media/v4l2-core/v4l2-common.c | 117 ++++++++++++++++++++++++++ >> include/media/v4l2-common.h | 28 ++++++ >> include/media/v4l2-subdev.h | 2 + >> 3 files changed, 147 insertions(+) >> >> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c >> index d0e5ebc736f9f..379d4bf4f8128 100644 >> --- a/drivers/media/v4l2-core/v4l2-common.c >> +++ b/drivers/media/v4l2-core/v4l2-common.c >> @@ -441,3 +441,120 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat, >> return 0; >> } >> EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt); >> + >> +/* >> + * v4l2_pipeline_subdevs_get - Assemble a list of subdevices in a pipeline >> + * @subdevs: the array to be filled >> + * @size: the array size > > Should this be documented as the maximum number of elements that can fit > in the subdevs array? sure, I'll update it. > >> + * >> + * Walk from a video node, following link from sink to source and fill the >> + * array with subdevices in the path. >> + * >> + * Note: this function follows the first enabled link in a sink pad found in a >> + * given entity. Thus it won't work if there are entities with multiple enabled >> + * links to its sink pads in the topology. > > I wonder if it would be more useful to make this function return all > subdevs in the pipeline that have an enabled link going from sink to > source while walking from the video device? > > When reading the commit messages I thought this could be useful for the > rcar-vin driver. By not finding all subdevices in the pipeline this > would not be possible as there on some platforms are a CSI-2 bus where > the CSI-2 transmitter have 4 sink pads and one source pads so the whole > pipeline would not be started. right, I'm not familiar with rcar-vin, I didn't know we had drivers using more then a sink link to subdevices that could be enabled at the same time. v1 of this patchset was returning all the nodes with enabled links going from sink to source, but in reverse order. I guess I can re-work v1, and add a flag to return in reverse order, I'll check. > >> + * >> + * Return the number of subdevices filled in the array. >> + */ >> +static unsigned int v4l2_pipeline_subdevs_get(struct video_device *vdev, >> + struct v4l2_subdev **subdevs, >> + unsigned int size) >> +{ >> + struct media_entity *entity = &vdev->entity; >> + unsigned int idx = 0; >> + >> + while (1) { >> + struct media_pad *src_pad = NULL; >> + unsigned int i; >> + >> + /* Find remote source pad */ >> + for (i = 0; i < entity->num_pads; i++) { >> + struct media_pad *sink_pad = &entity->pads[i]; >> + >> + if (!(sink_pad->flags & MEDIA_PAD_FL_SINK)) >> + continue; >> + >> + src_pad = media_entity_remote_pad(sink_pad); >> + if (src_pad && >> + is_media_entity_v4l2_subdev(src_pad->entity)) >> + break; >> + } >> + if (i == entity->num_pads) >> + break; >> + >> + if (idx >= size) { >> + WARN_ON(1); >> + return 0; > > Would it make sens to have this function return int and a negative error > code here? Is this now how other areas of V4L2 deal with when a provided > array is too small ? I'm thinking about if this function could be > exported for use by drivers in the future. we can do that, sure. > >> + } >> + >> + entity = src_pad->entity; >> + subdevs[idx++] = media_entity_to_v4l2_subdev(entity); >> + } >> + >> + return idx; >> +} >> + >> +__must_check int v4l2_pipeline_stream_enable(struct video_device *vdev) >> +{ >> + struct media_device *mdev = vdev->entity.graph_obj.mdev; >> + struct v4l2_subdev *subdevs[MEDIA_ENTITY_ENUM_MAX_DEPTH]; >> + struct v4l2_subdev *sd; >> + unsigned int i, size; >> + int ret; >> + >> + mutex_lock(&mdev->graph_mutex); >> + >> + size = v4l2_pipeline_subdevs_get(vdev, subdevs, ARRAY_SIZE(subdevs)); >> + >> + for (i = 0; i < size; i++) { >> + sd = subdevs[i]; >> + if (sd->stream_count++) >> + continue; >> + dev_dbg(mdev->dev, >> + "enabling stream for '%s'\n", sd->entity.name); > > Small nit, would it make sens to print the sd->stream_count also in this > debug statement (and the similar ones bellow) ? Only if we move this print before the "continue" above, otherwise it will always print 1. Thanks, Helen > >> + ret = v4l2_subdev_call(sd, video, s_stream, true); >> + if (ret && ret != -ENOIOCTLCMD) >> + goto err_stream_disable; >> + } >> + >> + mutex_unlock(&mdev->graph_mutex); >> + return 0; >> + >> +err_stream_disable: >> + do { >> + sd = subdevs[i]; >> + if (--sd->stream_count) >> + continue; >> + dev_dbg(mdev->dev, >> + "disabling stream for '%s'\n", sd->entity.name); >> + v4l2_subdev_call(sd, video, s_stream, false); >> + } while (i--); >> + >> + mutex_unlock(&mdev->graph_mutex); >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(v4l2_pipeline_stream_enable); >> + >> +void v4l2_pipeline_stream_disable(struct video_device *vdev) >> +{ >> + struct media_device *mdev = vdev->entity.graph_obj.mdev; >> + struct v4l2_subdev *subdevs[MEDIA_ENTITY_ENUM_MAX_DEPTH]; >> + unsigned int size; >> + >> + mutex_lock(&mdev->graph_mutex); >> + >> + size = v4l2_pipeline_subdevs_get(vdev, subdevs, ARRAY_SIZE(subdevs)); >> + >> + while (size--) { >> + struct v4l2_subdev *sd = subdevs[size]; >> + >> + if (--sd->stream_count) >> + continue; >> + dev_dbg(mdev->dev, >> + "disabling stream for '%s'\n", sd->entity.name); >> + v4l2_subdev_call(sd, video, s_stream, false); >> + } >> + >> + mutex_unlock(&mdev->graph_mutex); >> +} >> +EXPORT_SYMBOL_GPL(v4l2_pipeline_stream_disable); >> diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h >> index 150ee16ebd811..e833610b0f66d 100644 >> --- a/include/media/v4l2-common.h >> +++ b/include/media/v4l2-common.h >> @@ -519,6 +519,34 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat, >> int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, u32 pixelformat, >> u32 width, u32 height); >> >> +/** >> + * v4l2_pipeline_stream_enable - Call .s_stream(true) callbacks in the stream >> + * @vdev: Starting video device >> + * >> + * Call .s_stream(true) callback in all the subdevices participating in the >> + * stream, i.e. following links from sink to source. >> + * >> + * Calls to this function can be nested, in which case the same number of >> + * v4l2_pipeline_stream_disable() calls will be required to stop streaming. >> + * The pipeline pointer must be identical for all nested calls to >> + * v4l2_pipeline_stream_enable(). >> + */ >> +__must_check int v4l2_pipeline_stream_enable(struct video_device *vdev); >> + >> +/** >> + * v4l2_pipeline_stream_disable - Call .s_stream(false) callbacks in the stream >> + * @vdev: Starting video device >> + * >> + * Call .s_stream(true) callback in all the subdevices participating in the >> + * stream, i.e. following links from sink to source. >> + * >> + * Calls to this function can be nested, in which case the same number of >> + * v4l2_pipeline_stream_disable() calls will be required to stop streaming. >> + * The pipeline pointer must be identical for all nested calls to >> + * v4l2_pipeline_stream_enable(). >> + */ >> +void v4l2_pipeline_stream_disable(struct video_device *vdev); >> + >> static inline u64 v4l2_buffer_get_timestamp(const struct v4l2_buffer *buf) >> { >> /* >> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h >> index a4848de598521..20f913a9f70c5 100644 >> --- a/include/media/v4l2-subdev.h >> +++ b/include/media/v4l2-subdev.h >> @@ -838,6 +838,7 @@ struct v4l2_subdev_platform_data { >> * @subdev_notifier: A sub-device notifier implicitly registered for the sub- >> * device using v4l2_device_register_sensor_subdev(). >> * @pdata: common part of subdevice platform data >> + * @stream_count: Stream count for the subdevice. >> * >> * Each instance of a subdev driver should create this struct, either >> * stand-alone or embedded in a larger struct. >> @@ -869,6 +870,7 @@ struct v4l2_subdev { >> struct v4l2_async_notifier *notifier; >> struct v4l2_async_notifier *subdev_notifier; >> struct v4l2_subdev_platform_data *pdata; >> + unsigned int stream_count; >> }; >> >> >> -- >> 2.26.0 >> >
diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c index d0e5ebc736f9f..379d4bf4f8128 100644 --- a/drivers/media/v4l2-core/v4l2-common.c +++ b/drivers/media/v4l2-core/v4l2-common.c @@ -441,3 +441,120 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat, return 0; } EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt); + +/* + * v4l2_pipeline_subdevs_get - Assemble a list of subdevices in a pipeline + * @subdevs: the array to be filled + * @size: the array size + * + * Walk from a video node, following link from sink to source and fill the + * array with subdevices in the path. + * + * Note: this function follows the first enabled link in a sink pad found in a + * given entity. Thus it won't work if there are entities with multiple enabled + * links to its sink pads in the topology. + * + * Return the number of subdevices filled in the array. + */ +static unsigned int v4l2_pipeline_subdevs_get(struct video_device *vdev, + struct v4l2_subdev **subdevs, + unsigned int size) +{ + struct media_entity *entity = &vdev->entity; + unsigned int idx = 0; + + while (1) { + struct media_pad *src_pad = NULL; + unsigned int i; + + /* Find remote source pad */ + for (i = 0; i < entity->num_pads; i++) { + struct media_pad *sink_pad = &entity->pads[i]; + + if (!(sink_pad->flags & MEDIA_PAD_FL_SINK)) + continue; + + src_pad = media_entity_remote_pad(sink_pad); + if (src_pad && + is_media_entity_v4l2_subdev(src_pad->entity)) + break; + } + if (i == entity->num_pads) + break; + + if (idx >= size) { + WARN_ON(1); + return 0; + } + + entity = src_pad->entity; + subdevs[idx++] = media_entity_to_v4l2_subdev(entity); + } + + return idx; +} + +__must_check int v4l2_pipeline_stream_enable(struct video_device *vdev) +{ + struct media_device *mdev = vdev->entity.graph_obj.mdev; + struct v4l2_subdev *subdevs[MEDIA_ENTITY_ENUM_MAX_DEPTH]; + struct v4l2_subdev *sd; + unsigned int i, size; + int ret; + + mutex_lock(&mdev->graph_mutex); + + size = v4l2_pipeline_subdevs_get(vdev, subdevs, ARRAY_SIZE(subdevs)); + + for (i = 0; i < size; i++) { + sd = subdevs[i]; + if (sd->stream_count++) + continue; + dev_dbg(mdev->dev, + "enabling stream for '%s'\n", sd->entity.name); + ret = v4l2_subdev_call(sd, video, s_stream, true); + if (ret && ret != -ENOIOCTLCMD) + goto err_stream_disable; + } + + mutex_unlock(&mdev->graph_mutex); + return 0; + +err_stream_disable: + do { + sd = subdevs[i]; + if (--sd->stream_count) + continue; + dev_dbg(mdev->dev, + "disabling stream for '%s'\n", sd->entity.name); + v4l2_subdev_call(sd, video, s_stream, false); + } while (i--); + + mutex_unlock(&mdev->graph_mutex); + return ret; +} +EXPORT_SYMBOL_GPL(v4l2_pipeline_stream_enable); + +void v4l2_pipeline_stream_disable(struct video_device *vdev) +{ + struct media_device *mdev = vdev->entity.graph_obj.mdev; + struct v4l2_subdev *subdevs[MEDIA_ENTITY_ENUM_MAX_DEPTH]; + unsigned int size; + + mutex_lock(&mdev->graph_mutex); + + size = v4l2_pipeline_subdevs_get(vdev, subdevs, ARRAY_SIZE(subdevs)); + + while (size--) { + struct v4l2_subdev *sd = subdevs[size]; + + if (--sd->stream_count) + continue; + dev_dbg(mdev->dev, + "disabling stream for '%s'\n", sd->entity.name); + v4l2_subdev_call(sd, video, s_stream, false); + } + + mutex_unlock(&mdev->graph_mutex); +} +EXPORT_SYMBOL_GPL(v4l2_pipeline_stream_disable); diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h index 150ee16ebd811..e833610b0f66d 100644 --- a/include/media/v4l2-common.h +++ b/include/media/v4l2-common.h @@ -519,6 +519,34 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat, int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, u32 pixelformat, u32 width, u32 height); +/** + * v4l2_pipeline_stream_enable - Call .s_stream(true) callbacks in the stream + * @vdev: Starting video device + * + * Call .s_stream(true) callback in all the subdevices participating in the + * stream, i.e. following links from sink to source. + * + * Calls to this function can be nested, in which case the same number of + * v4l2_pipeline_stream_disable() calls will be required to stop streaming. + * The pipeline pointer must be identical for all nested calls to + * v4l2_pipeline_stream_enable(). + */ +__must_check int v4l2_pipeline_stream_enable(struct video_device *vdev); + +/** + * v4l2_pipeline_stream_disable - Call .s_stream(false) callbacks in the stream + * @vdev: Starting video device + * + * Call .s_stream(true) callback in all the subdevices participating in the + * stream, i.e. following links from sink to source. + * + * Calls to this function can be nested, in which case the same number of + * v4l2_pipeline_stream_disable() calls will be required to stop streaming. + * The pipeline pointer must be identical for all nested calls to + * v4l2_pipeline_stream_enable(). + */ +void v4l2_pipeline_stream_disable(struct video_device *vdev); + static inline u64 v4l2_buffer_get_timestamp(const struct v4l2_buffer *buf) { /* diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h index a4848de598521..20f913a9f70c5 100644 --- a/include/media/v4l2-subdev.h +++ b/include/media/v4l2-subdev.h @@ -838,6 +838,7 @@ struct v4l2_subdev_platform_data { * @subdev_notifier: A sub-device notifier implicitly registered for the sub- * device using v4l2_device_register_sensor_subdev(). * @pdata: common part of subdevice platform data + * @stream_count: Stream count for the subdevice. * * Each instance of a subdev driver should create this struct, either * stand-alone or embedded in a larger struct. @@ -869,6 +870,7 @@ struct v4l2_subdev { struct v4l2_async_notifier *notifier; struct v4l2_async_notifier *subdev_notifier; struct v4l2_subdev_platform_data *pdata; + unsigned int stream_count; };
Add v4l2_pipeline_stream_{enable,disable} helper functions to iterate through the subdevices in a given stream (i.e following links from sink to source) and call .s_stream() callback. Add stream_count on the subdevice object for simultaneous streaming from different video devices which shares subdevices. Prevent calling .s_stream(true) if it was already called previously by another stream. Prevent calling .s_stream(false) from one stream when there are still others active. If .s_stream(true) fails, call .s_stream(false) in the reverse order. Signed-off-by: Helen Koike <helen.koike@collabora.com> --- Changes in v2: - re-write helpers to not use media walkers drivers/media/v4l2-core/v4l2-common.c | 117 ++++++++++++++++++++++++++ include/media/v4l2-common.h | 28 ++++++ include/media/v4l2-subdev.h | 2 + 3 files changed, 147 insertions(+)