Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1326374 > unrolled thread

[PATCH v2 06/22] media: Media Controller non-locking __media_entity_pipeline_start/stop()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 06/22] media: Media Controller non-locking __media_entity_pipeline_start/stop() Shuah Khan <shuahkh@osg.samsung.com> - 2016-02-04 05:10 +0100
    Re: [PATCH v2 06/22] media: Media Controller non-locking  __media_entity_pipeline_start/stop() Mauro Carvalho Chehab <mchehab@osg.samsung.com> - 2016-02-04 10:20 +0100
      Re: [PATCH v2 06/22] media: Media Controller non-locking  __media_entity_pipeline_start/stop() Shuah Khan <shuahkh@osg.samsung.com> - 2016-02-04 16:10 +0100

#1326374 — [PATCH v2 06/22] media: Media Controller non-locking __media_entity_pipeline_start/stop()

Add non-locking __media_entity_pipeline_start/stop() interfaces
to be called from code paths that hold the graph_mutex. For this
change, the media_entity_pipeline_start() routine is renamed to
__media_entity_pipeline_start() minus the graph_mutex lock and
unlock. media_entity_pipeline_start() now calls the non-locking
__media_entity_pipeline_start() holding the graph_lock. The stop
interface, media_entity_pipeline_stop() routine is renamed to
__media_entity_pipeline_stop() minus the graph_mutex lock and
unlock. media_entity_pipeline_stop() now calls the non-locking
__media_entity_pipeline_stop() holding the graph_lock.

Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
---
 drivers/media/media-entity.c | 34 ++++++++++++++++++++++++----------
 include/media/media-entity.h | 12 ++++++++++++
 2 files changed, 36 insertions(+), 10 deletions(-)

diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index 9b4d712..b78e4c2 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -365,8 +365,8 @@ EXPORT_SYMBOL_GPL(media_entity_graph_walk_next);
  * Pipeline management
  */
 
-__must_check int media_entity_pipeline_start(struct media_entity *entity,
-					     struct media_pipeline *pipe)
+__must_check int __media_entity_pipeline_start(struct media_entity *entity,
+					       struct media_pipeline *pipe)
 {
 	struct media_device *mdev = entity->graph_obj.mdev;
 	struct media_entity_graph *graph = &pipe->graph;
@@ -374,8 +374,6 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity,
 	struct media_link *link;
 	int ret;
 
-	mutex_lock(&mdev->graph_mutex);
-
 	if (!pipe->streaming_count++) {
 		ret = media_entity_graph_walk_init(&pipe->graph, mdev);
 		if (ret)
@@ -456,8 +454,6 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity,
 		}
 	}
 
-	mutex_unlock(&mdev->graph_mutex);
-
 	return 0;
 
 error:
@@ -487,19 +483,28 @@ error_graph_walk_start:
 	if (!--pipe->streaming_count)
 		media_entity_graph_walk_cleanup(graph);
 
-	mutex_unlock(&mdev->graph_mutex);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(__media_entity_pipeline_start);
 
+__must_check int media_entity_pipeline_start(struct media_entity *entity,
+					     struct media_pipeline *pipe)
+{
+	struct media_device *mdev = entity->graph_obj.mdev;
+	int ret;
+
+	mutex_lock(&mdev->graph_mutex);
+	ret = __media_entity_pipeline_start(entity, pipe);
+	mutex_unlock(&mdev->graph_mutex);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(media_entity_pipeline_start);
 
-void media_entity_pipeline_stop(struct media_entity *entity)
+void __media_entity_pipeline_stop(struct media_entity *entity)
 {
-	struct media_device *mdev = entity->graph_obj.mdev;
 	struct media_entity_graph *graph = &entity->pipe->graph;
 	struct media_pipeline *pipe = entity->pipe;
 
-	mutex_lock(&mdev->graph_mutex);
 
 	WARN_ON(!pipe->streaming_count);
 	media_entity_graph_walk_start(graph, entity);
@@ -516,6 +521,15 @@ void media_entity_pipeline_stop(struct media_entity *entity)
 	if (!--pipe->streaming_count)
 		media_entity_graph_walk_cleanup(graph);
 
+}
+EXPORT_SYMBOL_GPL(__media_entity_pipeline_stop);
+
+void media_entity_pipeline_stop(struct media_entity *entity)
+{
+	struct media_device *mdev = entity->graph_obj.mdev;
+
+	mutex_lock(&mdev->graph_mutex);
+	__media_entity_pipeline_stop(entity);
 	mutex_unlock(&mdev->graph_mutex);
 }
 EXPORT_SYMBOL_GPL(media_entity_pipeline_stop);
diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index fe485d3..c7583db 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -832,6 +832,12 @@ media_entity_graph_walk_next(struct media_entity_graph *graph);
  */
 __must_check int media_entity_pipeline_start(struct media_entity *entity,
 					     struct media_pipeline *pipe);
+/**
+ * non-locking __media_entity_pipeline_start() can be called from
+ * code paths that hold the graph_mutex
+ */
+__must_check int __media_entity_pipeline_start(struct media_entity *entity,
+					       struct media_pipeline *pipe);
 
 /**
  * media_entity_pipeline_stop - Mark a pipeline as not streaming
@@ -848,6 +854,12 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity,
 void media_entity_pipeline_stop(struct media_entity *entity);
 
 /**
+ * non-locking __media_entity_pipeline_stop() can be called from
+ * code paths that hold the graph_mutex
+ */
+void __media_entity_pipeline_stop(struct media_entity *entity);
+
+/**
  * media_devnode_create() - creates and initializes a device node interface
  *
  * @mdev:	pointer to struct &media_device
-- 
2.5.0

[toc] | [next] | [standalone]

#1326502 — Re: [PATCH v2 06/22] media: Media Controller non-locking __media_entity_pipeline_start/stop()

Em Wed, 03 Feb 2016 21:03:38 -0700
Shuah Khan <shuahkh@osg.samsung.com> escreveu:

> Add non-locking __media_entity_pipeline_start/stop() interfaces
> to be called from code paths that hold the graph_mutex. For this
> change, the media_entity_pipeline_start() routine is renamed to
> __media_entity_pipeline_start() minus the graph_mutex lock and
> unlock. media_entity_pipeline_start() now calls the non-locking
> __media_entity_pipeline_start() holding the graph_lock. The stop
> interface, media_entity_pipeline_stop() routine is renamed to
> __media_entity_pipeline_stop() minus the graph_mutex lock and
> unlock. media_entity_pipeline_stop() now calls the non-locking
> __media_entity_pipeline_stop() holding the graph_lock.

Please write a better patch descriptions. The above could easily
be split into 3 or 4 paragraphs, with would make it easier to
read.

Also, too much details, written on a spaghetti style;)

It is obvious by anyone that looked at the patch that
"media_entity_pipeline_start() routine is renamed to 
__media_entity_pipeline_start() minus the graph_mutex lock and
unlock."

I read exactly the same thing at the C code, *and* at the
subject of the e-mail. Patch subject/description should tell
what, why and how (but you should not repeat what can easily
be inferred from looking at the diff).

I would describe it as:

   Currently, media pipeline graph traversal routines take
   the graph_mutex lock.

   While this works for the current code, we'll need 
   unlocked versions of them because
   (some explanation *why* you need it, with is the main
    thing that should be at the description).

   So, add non-locking media_entity_pipeline_start/stop()
   variations.

> 
> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
> ---
>  drivers/media/media-entity.c | 34 ++++++++++++++++++++++++----------
>  include/media/media-entity.h | 12 ++++++++++++
>  2 files changed, 36 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index 9b4d712..b78e4c2 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -365,8 +365,8 @@ EXPORT_SYMBOL_GPL(media_entity_graph_walk_next);
>   * Pipeline management
>   */
>  
> -__must_check int media_entity_pipeline_start(struct media_entity *entity,
> -					     struct media_pipeline *pipe)
> +__must_check int __media_entity_pipeline_start(struct media_entity *entity,
> +					       struct media_pipeline *pipe)
>  {
>  	struct media_device *mdev = entity->graph_obj.mdev;
>  	struct media_entity_graph *graph = &pipe->graph;
> @@ -374,8 +374,6 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity,
>  	struct media_link *link;
>  	int ret;
>  
> -	mutex_lock(&mdev->graph_mutex);
> -
>  	if (!pipe->streaming_count++) {
>  		ret = media_entity_graph_walk_init(&pipe->graph, mdev);
>  		if (ret)
> @@ -456,8 +454,6 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity,
>  		}
>  	}
>  
> -	mutex_unlock(&mdev->graph_mutex);
> -
>  	return 0;
>  
>  error:
> @@ -487,19 +483,28 @@ error_graph_walk_start:
>  	if (!--pipe->streaming_count)
>  		media_entity_graph_walk_cleanup(graph);
>  
> -	mutex_unlock(&mdev->graph_mutex);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(__media_entity_pipeline_start);
>  
> +__must_check int media_entity_pipeline_start(struct media_entity *entity,
> +					     struct media_pipeline *pipe)
> +{
> +	struct media_device *mdev = entity->graph_obj.mdev;
> +	int ret;
> +
> +	mutex_lock(&mdev->graph_mutex);
> +	ret = __media_entity_pipeline_start(entity, pipe);
> +	mutex_unlock(&mdev->graph_mutex);
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(media_entity_pipeline_start);
>  
> -void media_entity_pipeline_stop(struct media_entity *entity)
> +void __media_entity_pipeline_stop(struct media_entity *entity)
>  {
> -	struct media_device *mdev = entity->graph_obj.mdev;
>  	struct media_entity_graph *graph = &entity->pipe->graph;
>  	struct media_pipeline *pipe = entity->pipe;
>  
> -	mutex_lock(&mdev->graph_mutex);
>  
>  	WARN_ON(!pipe->streaming_count);
>  	media_entity_graph_walk_start(graph, entity);
> @@ -516,6 +521,15 @@ void media_entity_pipeline_stop(struct media_entity *entity)
>  	if (!--pipe->streaming_count)
>  		media_entity_graph_walk_cleanup(graph);
>  
> +}
> +EXPORT_SYMBOL_GPL(__media_entity_pipeline_stop);
> +
> +void media_entity_pipeline_stop(struct media_entity *entity)
> +{
> +	struct media_device *mdev = entity->graph_obj.mdev;
> +
> +	mutex_lock(&mdev->graph_mutex);
> +	__media_entity_pipeline_stop(entity);
>  	mutex_unlock(&mdev->graph_mutex);
>  }
>  EXPORT_SYMBOL_GPL(media_entity_pipeline_stop);
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index fe485d3..c7583db 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -832,6 +832,12 @@ media_entity_graph_walk_next(struct media_entity_graph *graph);
>   */
>  __must_check int media_entity_pipeline_start(struct media_entity *entity,
>  					     struct media_pipeline *pipe);
> +/**
> + * non-locking __media_entity_pipeline_start() can be called from
> + * code paths that hold the graph_mutex
> + */
> +__must_check int __media_entity_pipeline_start(struct media_entity *entity,
> +					       struct media_pipeline *pipe);
>  
>  /**
>   * media_entity_pipeline_stop - Mark a pipeline as not streaming
> @@ -848,6 +854,12 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity,
>  void media_entity_pipeline_stop(struct media_entity *entity);
>  
>  /**
> + * non-locking __media_entity_pipeline_stop() can be called from
> + * code paths that hold the graph_mutex
> + */
> +void __media_entity_pipeline_stop(struct media_entity *entity);
> +
> +/**
>   * media_devnode_create() - creates and initializes a device node interface
>   *
>   * @mdev:	pointer to struct &media_device

[toc] | [prev] | [next] | [standalone]

#1326902 — Re: [PATCH v2 06/22] media: Media Controller non-locking __media_entity_pipeline_start/stop()

On 02/04/2016 02:18 AM, Mauro Carvalho Chehab wrote:
> Em Wed, 03 Feb 2016 21:03:38 -0700
> Shuah Khan <shuahkh@osg.samsung.com> escreveu:
> 
>> Add non-locking __media_entity_pipeline_start/stop() interfaces
>> to be called from code paths that hold the graph_mutex. For this
>> change, the media_entity_pipeline_start() routine is renamed to
>> __media_entity_pipeline_start() minus the graph_mutex lock and
>> unlock. media_entity_pipeline_start() now calls the non-locking
>> __media_entity_pipeline_start() holding the graph_lock. The stop
>> interface, media_entity_pipeline_stop() routine is renamed to
>> __media_entity_pipeline_stop() minus the graph_mutex lock and
>> unlock. media_entity_pipeline_stop() now calls the non-locking
>> __media_entity_pipeline_stop() holding the graph_lock.
> 
> Please write a better patch descriptions. The above could easily
> be split into 3 or 4 paragraphs, with would make it easier to
> read.
> 

Removed linux-api from cc list


ok - I updated several changelogs based on your earlier
comments and this one slipped by.

-- Shuah
> Also, too much details, written on a spaghetti style;)
> 
> It is obvious by anyone that looked at the patch that
> "media_entity_pipeline_start() routine is renamed to 
> __media_entity_pipeline_start() minus the graph_mutex lock and
> unlock."
> 
> I read exactly the same thing at the C code, *and* at the
> subject of the e-mail. Patch subject/description should tell
> what, why and how (but you should not repeat what can easily
> be inferred from looking at the diff).
> 
> I would describe it as:
> 
>    Currently, media pipeline graph traversal routines take
>    the graph_mutex lock.
> 
>    While this works for the current code, we'll need 
>    unlocked versions of them because
>    (some explanation *why* you need it, with is the main
>     thing that should be at the description).
> 
>    So, add non-locking media_entity_pipeline_start/stop()
>    variations.
> 
>>
>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
>> ---
>>  drivers/media/media-entity.c | 34 ++++++++++++++++++++++++----------
>>  include/media/media-entity.h | 12 ++++++++++++
>>  2 files changed, 36 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
>> index 9b4d712..b78e4c2 100644
>> --- a/drivers/media/media-entity.c
>> +++ b/drivers/media/media-entity.c
>> @@ -365,8 +365,8 @@ EXPORT_SYMBOL_GPL(media_entity_graph_walk_next);
>>   * Pipeline management
>>   */
>>  
>> -__must_check int media_entity_pipeline_start(struct media_entity *entity,
>> -					     struct media_pipeline *pipe)
>> +__must_check int __media_entity_pipeline_start(struct media_entity *entity,
>> +					       struct media_pipeline *pipe)
>>  {
>>  	struct media_device *mdev = entity->graph_obj.mdev;
>>  	struct media_entity_graph *graph = &pipe->graph;
>> @@ -374,8 +374,6 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity,
>>  	struct media_link *link;
>>  	int ret;
>>  
>> -	mutex_lock(&mdev->graph_mutex);
>> -
>>  	if (!pipe->streaming_count++) {
>>  		ret = media_entity_graph_walk_init(&pipe->graph, mdev);
>>  		if (ret)
>> @@ -456,8 +454,6 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity,
>>  		}
>>  	}
>>  
>> -	mutex_unlock(&mdev->graph_mutex);
>> -
>>  	return 0;
>>  
>>  error:
>> @@ -487,19 +483,28 @@ error_graph_walk_start:
>>  	if (!--pipe->streaming_count)
>>  		media_entity_graph_walk_cleanup(graph);
>>  
>> -	mutex_unlock(&mdev->graph_mutex);
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(__media_entity_pipeline_start);
>>  
>> +__must_check int media_entity_pipeline_start(struct media_entity *entity,
>> +					     struct media_pipeline *pipe)
>> +{
>> +	struct media_device *mdev = entity->graph_obj.mdev;
>> +	int ret;
>> +
>> +	mutex_lock(&mdev->graph_mutex);
>> +	ret = __media_entity_pipeline_start(entity, pipe);
>> +	mutex_unlock(&mdev->graph_mutex);
>>  	return ret;
>>  }
>>  EXPORT_SYMBOL_GPL(media_entity_pipeline_start);
>>  
>> -void media_entity_pipeline_stop(struct media_entity *entity)
>> +void __media_entity_pipeline_stop(struct media_entity *entity)
>>  {
>> -	struct media_device *mdev = entity->graph_obj.mdev;
>>  	struct media_entity_graph *graph = &entity->pipe->graph;
>>  	struct media_pipeline *pipe = entity->pipe;
>>  
>> -	mutex_lock(&mdev->graph_mutex);
>>  
>>  	WARN_ON(!pipe->streaming_count);
>>  	media_entity_graph_walk_start(graph, entity);
>> @@ -516,6 +521,15 @@ void media_entity_pipeline_stop(struct media_entity *entity)
>>  	if (!--pipe->streaming_count)
>>  		media_entity_graph_walk_cleanup(graph);
>>  
>> +}
>> +EXPORT_SYMBOL_GPL(__media_entity_pipeline_stop);
>> +
>> +void media_entity_pipeline_stop(struct media_entity *entity)
>> +{
>> +	struct media_device *mdev = entity->graph_obj.mdev;
>> +
>> +	mutex_lock(&mdev->graph_mutex);
>> +	__media_entity_pipeline_stop(entity);
>>  	mutex_unlock(&mdev->graph_mutex);
>>  }
>>  EXPORT_SYMBOL_GPL(media_entity_pipeline_stop);
>> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
>> index fe485d3..c7583db 100644
>> --- a/include/media/media-entity.h
>> +++ b/include/media/media-entity.h
>> @@ -832,6 +832,12 @@ media_entity_graph_walk_next(struct media_entity_graph *graph);
>>   */
>>  __must_check int media_entity_pipeline_start(struct media_entity *entity,
>>  					     struct media_pipeline *pipe);
>> +/**
>> + * non-locking __media_entity_pipeline_start() can be called from
>> + * code paths that hold the graph_mutex
>> + */
>> +__must_check int __media_entity_pipeline_start(struct media_entity *entity,
>> +					       struct media_pipeline *pipe);
>>  
>>  /**
>>   * media_entity_pipeline_stop - Mark a pipeline as not streaming
>> @@ -848,6 +854,12 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity,
>>  void media_entity_pipeline_stop(struct media_entity *entity);
>>  
>>  /**
>> + * non-locking __media_entity_pipeline_stop() can be called from
>> + * code paths that hold the graph_mutex
>> + */
>> +void __media_entity_pipeline_stop(struct media_entity *entity);
>> +
>> +/**
>>   * media_devnode_create() - creates and initializes a device node interface
>>   *
>>   * @mdev:	pointer to struct &media_device


-- 
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America (Silicon Valley)
shuahkh@osg.samsung.com | (970) 217-8978

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web