[v2,2/5] media: Always keep a graph walk large enough around
diff mbox

Message ID 1453906078-29087-3-git-send-email-sakari.ailus@iki.fi
State New
Headers show

Commit Message

Sakari Ailus Jan. 27, 2016, 2:47 p.m. UTC
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(+)

Comments

kbuild test robot Jan. 27, 2016, 3:44 p.m. UTC | #1
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
Mauro Carvalho Chehab Feb. 19, 2016, 2:03 p.m. UTC | #2
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);
Sakari Ailus Feb. 19, 2016, 2:40 p.m. UTC | #3
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.
Mauro Carvalho Chehab Feb. 19, 2016, 4:14 p.m. UTC | #4
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
Sakari Ailus Feb. 20, 2016, 10:59 p.m. UTC | #5
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.
Sakari Ailus Feb. 21, 2016, 4 p.m. UTC | #6
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.

Patch
diff mbox

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);