| Message ID | 1453906078-29087-3-git-send-email-sakari.ailus@iki.fi (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Sakari, [auto build test WARNING on linuxtv-media/master] [also build test WARNING on v4.5-rc1 next-20160127] [if your patch is applied to the wrong git tree, please drop us a note to help improving the system] url: https://github.com/0day-ci/linux/commits/Sakari-Ailus/Unify-MC-graph-power-management-code/20160127-215417 base: git://linuxtv.org/media_tree.git master reproduce: make htmldocs All warnings (new ones prefixed by >>): include/linux/init.h:1: warning: no structured comments found kernel/sys.c:1: warning: no structured comments found drivers/dma-buf/seqno-fence.c:1: warning: no structured comments found drivers/dma-buf/reservation.c:1: warning: no structured comments found include/linux/reservation.h:1: warning: no structured comments found >> include/media/media-device.h:334: warning: No description found for parameter 'pm_count_walk' >> include/media/media-device.h:334: warning: No description found for parameter 'pm_count_walk' include/linux/spi/spi.h:540: warning: No description found for parameter 'max_transfer_size' vim +/pm_count_walk +334 include/media/media-device.h 57cf79b7 Mauro Carvalho Chehab 2015-08-21 318 struct list_head interfaces; 9155d859 Mauro Carvalho Chehab 2015-08-23 319 struct list_head pads; 9155d859 Mauro Carvalho Chehab 2015-08-23 320 struct list_head links; 53e269c1 Laurent Pinchart 2009-12-09 321 a08fad1e Mauro Carvalho Chehab 2015-12-09 322 /* Protects the graph objects creation/removal */ 53e269c1 Laurent Pinchart 2009-12-09 323 spinlock_t lock; 503c3d82 Laurent Pinchart 2010-03-07 324 /* Serializes graph operations. */ 503c3d82 Laurent Pinchart 2010-03-07 325 struct mutex graph_mutex; 12c5791e Sakari Ailus 2016-01-27 326 /* 12c5791e Sakari Ailus 2016-01-27 327 * Graph walk for power state walk. Access serialised using 12c5791e Sakari Ailus 2016-01-27 328 * graph_mutex. 12c5791e Sakari Ailus 2016-01-27 329 */ 12c5791e Sakari Ailus 2016-01-27 330 struct media_entity_graph pm_count_walk; 97548ed4 Laurent Pinchart 2009-12-09 331 813f5c0a Sylwester Nawrocki 2013-05-31 332 int (*link_notify)(struct media_link *link, u32 flags, 813f5c0a Sylwester Nawrocki 2013-05-31 333 unsigned int notification); 176fb0d1 Laurent Pinchart 2009-12-09 @334 }; 176fb0d1 Laurent Pinchart 2009-12-09 335 e576d60b Shuah Khan 2015-06-05 336 #ifdef CONFIG_MEDIA_CONTROLLER e576d60b Shuah Khan 2015-06-05 337 813f5c0a Sylwester Nawrocki 2013-05-31 338 /* Supported link_notify @notification values. */ 813f5c0a Sylwester Nawrocki 2013-05-31 339 #define MEDIA_DEV_NOTIFY_PRE_LINK_CH 0 813f5c0a Sylwester Nawrocki 2013-05-31 340 #define MEDIA_DEV_NOTIFY_POST_LINK_CH 1 813f5c0a Sylwester Nawrocki 2013-05-31 341 176fb0d1 Laurent Pinchart 2009-12-09 342 /* media_devnode to media_device */ :::::: The code at line 334 was first introduced by commit :::::: 176fb0d108f7495ccf9aa127e1342a1a0d87e004 [media] media: Media device :::::: TO: Laurent Pinchart <laurent.pinchart@ideasonboard.com> :::::: CC: Mauro Carvalho Chehab <mchehab@redhat.com> --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Sakari, Em Wed, 27 Jan 2016 16:47:55 +0200 Sakari Ailus <sakari.ailus@iki.fi> escreveu: > Re-create the graph walk object as needed in order to have one large enough > available for all entities in the graph. > > This enumeration is used for pipeline power management in the future. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/media-device.c | 21 +++++++++++++++++++++ > include/media/media-device.h | 5 +++++ > 2 files changed, 26 insertions(+) > > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c > index 4d1c13d..52d7809 100644 > --- a/drivers/media/media-device.c > +++ b/drivers/media/media-device.c > @@ -577,6 +577,26 @@ int __must_check media_device_register_entity(struct media_device *mdev, > > spin_unlock(&mdev->lock); > > + mutex_lock(&mdev->graph_mutex); > + if (mdev->entity_internal_idx_max > + >= mdev->pm_count_walk.ent_enum.idx_max) { > + struct media_entity_graph new = { 0 }; > + > + /* > + * Initialise the new graph walk before cleaning up > + * the old one in order not to spoil the graph walk > + * object of the media device if graph walk init fails. > + */ > + ret = media_entity_graph_walk_init(&new, mdev); > + if (ret) { > + mutex_unlock(&mdev->graph_mutex); > + return ret; > + } > + media_entity_graph_walk_cleanup(&mdev->pm_count_walk); > + mdev->pm_count_walk = new; > + } > + mutex_unlock(&mdev->graph_mutex); > + I don't like the idea of creating a new graph init and destroying the previous one every time. In principle, this needs to be done only when trying to start the graph - or just before registering the MC devnode, if the driver needs/wants to optimize it. As kbuildtest also didn't like this patch, I'm not applying it for now. > return 0; > } > EXPORT_SYMBOL_GPL(media_device_register_entity); > @@ -652,6 +672,7 @@ void media_device_cleanup(struct media_device *mdev) > { > ida_destroy(&mdev->entity_internal_idx); > mdev->entity_internal_idx_max = 0; > + media_entity_graph_walk_cleanup(&mdev->pm_count_walk); > mutex_destroy(&mdev->graph_mutex); > } > EXPORT_SYMBOL_GPL(media_device_cleanup); > diff --git a/include/media/media-device.h b/include/media/media-device.h > index d385589..dba3986 100644 > --- a/include/media/media-device.h > +++ b/include/media/media-device.h > @@ -323,6 +323,11 @@ struct media_device { > spinlock_t lock; > /* Serializes graph operations. */ > struct mutex graph_mutex; > + /* > + * Graph walk for power state walk. Access serialised using > + * graph_mutex. > + */ > + struct media_entity_graph pm_count_walk; > > int (*link_notify)(struct media_link *link, u32 flags, > unsigned int notification);
Hi Mauro, On Fri, Feb 19, 2016 at 12:03:41PM -0200, Mauro Carvalho Chehab wrote: > Hi Sakari, > > Em Wed, 27 Jan 2016 16:47:55 +0200 > Sakari Ailus <sakari.ailus@iki.fi> escreveu: > > > Re-create the graph walk object as needed in order to have one large enough > > available for all entities in the graph. > > > > This enumeration is used for pipeline power management in the future. > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > drivers/media/media-device.c | 21 +++++++++++++++++++++ > > include/media/media-device.h | 5 +++++ > > 2 files changed, 26 insertions(+) > > > > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c > > index 4d1c13d..52d7809 100644 > > --- a/drivers/media/media-device.c > > +++ b/drivers/media/media-device.c > > @@ -577,6 +577,26 @@ int __must_check media_device_register_entity(struct media_device *mdev, > > > > spin_unlock(&mdev->lock); > > > > + mutex_lock(&mdev->graph_mutex); > > + if (mdev->entity_internal_idx_max > > + >= mdev->pm_count_walk.ent_enum.idx_max) { > > + struct media_entity_graph new = { 0 }; > > + > > + /* > > + * Initialise the new graph walk before cleaning up > > + * the old one in order not to spoil the graph walk > > + * object of the media device if graph walk init fails. > > + */ > > + ret = media_entity_graph_walk_init(&new, mdev); > > + if (ret) { > > + mutex_unlock(&mdev->graph_mutex); > > + return ret; > > + } > > + media_entity_graph_walk_cleanup(&mdev->pm_count_walk); > > + mdev->pm_count_walk = new; > > + } > > + mutex_unlock(&mdev->graph_mutex); > > + > > I don't like the idea of creating a new graph init and destroying the > previous one every time. In principle, this needs to be done only > when trying to start the graph - or just before registering the > MC devnode, if the driver needs/wants to optimize it. It's not every time --- with the previous patch, that's every 32 or 64 additional entity, depending on how many bits the unsigned long is. > > As kbuildtest also didn't like this patch, I'm not applying it > for now. For missing KernelDoc documentation for a struct field. Other fields in the struct don't have KernelDoc documentation either, and I didn't feel it'd fit well for this patch. I can add a patch to change the field documentation to the set if you like.
Em Fri, 19 Feb 2016 16:40:46 +0200 Sakari Ailus <sakari.ailus@iki.fi> escreveu: > Hi Mauro, > > On Fri, Feb 19, 2016 at 12:03:41PM -0200, Mauro Carvalho Chehab wrote: > > Hi Sakari, > > > > Em Wed, 27 Jan 2016 16:47:55 +0200 > > Sakari Ailus <sakari.ailus@iki.fi> escreveu: > > > > > Re-create the graph walk object as needed in order to have one large enough > > > available for all entities in the graph. > > > > > > This enumeration is used for pipeline power management in the future. > > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > --- > > > drivers/media/media-device.c | 21 +++++++++++++++++++++ > > > include/media/media-device.h | 5 +++++ > > > 2 files changed, 26 insertions(+) > > > > > > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c > > > index 4d1c13d..52d7809 100644 > > > --- a/drivers/media/media-device.c > > > +++ b/drivers/media/media-device.c > > > @@ -577,6 +577,26 @@ int __must_check media_device_register_entity(struct media_device *mdev, > > > > > > spin_unlock(&mdev->lock); > > > > > > + mutex_lock(&mdev->graph_mutex); > > > + if (mdev->entity_internal_idx_max > > > + >= mdev->pm_count_walk.ent_enum.idx_max) { > > > + struct media_entity_graph new = { 0 }; > > > + > > > + /* > > > + * Initialise the new graph walk before cleaning up > > > + * the old one in order not to spoil the graph walk > > > + * object of the media device if graph walk init fails. > > > + */ > > > + ret = media_entity_graph_walk_init(&new, mdev); > > > + if (ret) { > > > + mutex_unlock(&mdev->graph_mutex); > > > + return ret; > > > + } > > > + media_entity_graph_walk_cleanup(&mdev->pm_count_walk); > > > + mdev->pm_count_walk = new; > > > + } > > > + mutex_unlock(&mdev->graph_mutex); > > > + > > > > I don't like the idea of creating a new graph init and destroying the > > previous one every time. In principle, this needs to be done only > > when trying to start the graph - or just before registering the > > MC devnode, if the driver needs/wants to optimize it. > > It's not every time --- with the previous patch, that's every 32 or 64 > additional entity, depending on how many bits the unsigned long is. Still it will be called a lot of times for DVB devices. Why doing that, if such alloc can be done only after finishing registering all devices? > > > > > As kbuildtest also didn't like this patch, I'm not applying it > > for now. > > For missing KernelDoc documentation for a struct field. > > Other fields in the struct don't have KernelDoc documentation either, and I > didn't feel it'd fit well for this patch. I can add a patch to change the > field documentation to the set if you like. Ok, it could be done on a separate patch. Feel free to submit it. Thanks! Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mauro, On Fri, Feb 19, 2016 at 02:14:23PM -0200, Mauro Carvalho Chehab wrote: > Em Fri, 19 Feb 2016 16:40:46 +0200 > Sakari Ailus <sakari.ailus@iki.fi> escreveu: > > > Hi Mauro, > > > > On Fri, Feb 19, 2016 at 12:03:41PM -0200, Mauro Carvalho Chehab wrote: > > > Hi Sakari, > > > > > > Em Wed, 27 Jan 2016 16:47:55 +0200 > > > Sakari Ailus <sakari.ailus@iki.fi> escreveu: > > > > > > > Re-create the graph walk object as needed in order to have one large enough > > > > available for all entities in the graph. > > > > > > > > This enumeration is used for pipeline power management in the future. > > > > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > > --- > > > > drivers/media/media-device.c | 21 +++++++++++++++++++++ > > > > include/media/media-device.h | 5 +++++ > > > > 2 files changed, 26 insertions(+) > > > > > > > > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c > > > > index 4d1c13d..52d7809 100644 > > > > --- a/drivers/media/media-device.c > > > > +++ b/drivers/media/media-device.c > > > > @@ -577,6 +577,26 @@ int __must_check media_device_register_entity(struct media_device *mdev, > > > > > > > > spin_unlock(&mdev->lock); > > > > > > > > + mutex_lock(&mdev->graph_mutex); > > > > + if (mdev->entity_internal_idx_max > > > > + >= mdev->pm_count_walk.ent_enum.idx_max) { > > > > + struct media_entity_graph new = { 0 }; > > > > + > > > > + /* > > > > + * Initialise the new graph walk before cleaning up > > > > + * the old one in order not to spoil the graph walk > > > > + * object of the media device if graph walk init fails. > > > > + */ > > > > + ret = media_entity_graph_walk_init(&new, mdev); > > > > + if (ret) { > > > > + mutex_unlock(&mdev->graph_mutex); > > > > + return ret; > > > > + } > > > > + media_entity_graph_walk_cleanup(&mdev->pm_count_walk); > > > > + mdev->pm_count_walk = new; > > > > + } > > > > + mutex_unlock(&mdev->graph_mutex); > > > > + > > > > > > I don't like the idea of creating a new graph init and destroying the > > > previous one every time. In principle, this needs to be done only > > > when trying to start the graph - or just before registering the > > > MC devnode, if the driver needs/wants to optimize it. > > > > It's not every time --- with the previous patch, that's every 32 or 64 > > additional entity, depending on how many bits the unsigned long is. > > Still it will be called a lot of times for DVB devices. Why doing that, > if such alloc can be done only after finishing registering all devices? The intent is to prepare for being able to dynamically allocate and remove entities, and still not allocate excessive amounts of memory for the graph. With this set, there's a guarantee that the graph walk will always be successful even if entities were added to or removed from the graph. That's important as there are operations that may not fail such as decrementing the use count of an entity (and possibly other entities as well as a result; video nodes in practice). I'd still like to claim that this will not have noticeable adverse effect on the time it takes to register the necessary entities.
On Fri, Feb 19, 2016 at 02:14:23PM -0200, Mauro Carvalho Chehab wrote: ... > > > As kbuildtest also didn't like this patch, I'm not applying it > > > for now. > > > > For missing KernelDoc documentation for a struct field. > > > > Other fields in the struct don't have KernelDoc documentation either, and I > > didn't feel it'd fit well for this patch. I can add a patch to change the > > field documentation to the set if you like. > > Ok, it could be done on a separate patch. Feel free to submit it. I noticed the struct had KernelDoc comments but I missed them. I'll update the patch accordingly. If you still think it'd be a good idea to move the graph initialisation elsewhere, let me know. In the meantime I'll send v3.
diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c index 4d1c13d..52d7809 100644 --- a/drivers/media/media-device.c +++ b/drivers/media/media-device.c @@ -577,6 +577,26 @@ int __must_check media_device_register_entity(struct media_device *mdev, spin_unlock(&mdev->lock); + mutex_lock(&mdev->graph_mutex); + if (mdev->entity_internal_idx_max + >= mdev->pm_count_walk.ent_enum.idx_max) { + struct media_entity_graph new = { 0 }; + + /* + * Initialise the new graph walk before cleaning up + * the old one in order not to spoil the graph walk + * object of the media device if graph walk init fails. + */ + ret = media_entity_graph_walk_init(&new, mdev); + if (ret) { + mutex_unlock(&mdev->graph_mutex); + return ret; + } + media_entity_graph_walk_cleanup(&mdev->pm_count_walk); + mdev->pm_count_walk = new; + } + mutex_unlock(&mdev->graph_mutex); + return 0; } EXPORT_SYMBOL_GPL(media_device_register_entity); @@ -652,6 +672,7 @@ void media_device_cleanup(struct media_device *mdev) { ida_destroy(&mdev->entity_internal_idx); mdev->entity_internal_idx_max = 0; + media_entity_graph_walk_cleanup(&mdev->pm_count_walk); mutex_destroy(&mdev->graph_mutex); } EXPORT_SYMBOL_GPL(media_device_cleanup); diff --git a/include/media/media-device.h b/include/media/media-device.h index d385589..dba3986 100644 --- a/include/media/media-device.h +++ b/include/media/media-device.h @@ -323,6 +323,11 @@ struct media_device { spinlock_t lock; /* Serializes graph operations. */ struct mutex graph_mutex; + /* + * Graph walk for power state walk. Access serialised using + * graph_mutex. + */ + struct media_entity_graph pm_count_walk; int (*link_notify)(struct media_link *link, u32 flags, unsigned int notification);
Re-create the graph walk object as needed in order to have one large enough available for all entities in the graph. This enumeration is used for pipeline power management in the future. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- drivers/media/media-device.c | 21 +++++++++++++++++++++ include/media/media-device.h | 5 +++++ 2 files changed, 26 insertions(+)