Re: [PATCH v5 25/35] omap3isp: Collect entities that are part of the pipeline

2012-03-07 Thread Laurent Pinchart
Hi Sakari,

Thanks for the patch.

On Tuesday 06 March 2012 18:33:06 Sakari Ailus wrote:
 Collect entities which are part of the pipeline into a single bit mask.
 
 Signed-off-by: Sakari Ailus sakari.ai...@iki.fi
 ---
  drivers/media/video/omap3isp/ispvideo.c |9 +
  drivers/media/video/omap3isp/ispvideo.h |1 +
  2 files changed, 10 insertions(+), 0 deletions(-)
 
 diff --git a/drivers/media/video/omap3isp/ispvideo.c
 b/drivers/media/video/omap3isp/ispvideo.c index d34f690..4bc9cca 100644
 --- a/drivers/media/video/omap3isp/ispvideo.c
 +++ b/drivers/media/video/omap3isp/ispvideo.c
 @@ -970,6 +970,8 @@ isp_video_streamon(struct file *file, void *fh, enum
 v4l2_buf_type type) {
   struct isp_video_fh *vfh = to_isp_video_fh(fh);
   struct isp_video *video = video_drvdata(file);
 + struct media_entity_graph graph;
 + struct media_entity *entity;
   enum isp_pipeline_state state;
   struct isp_pipeline *pipe;
   struct isp_video *far_end;
 @@ -992,6 +994,8 @@ isp_video_streamon(struct file *file, void *fh, enum
 v4l2_buf_type type) pipe = video-video.entity.pipe
? to_isp_pipeline(video-video.entity) : video-pipe;
 
 + pipe-entities = 0;
 +

This could be move right before the graph walk code below to keep both parts 
together. However, pipe-entities would then be invalid (instead of always 0) 
in the link validation operations. That can be considered as an issue, so I'm 
fine if you prefer leaving this assignment here.

   if (video-isp-pdata-set_constraints)
   video-isp-pdata-set_constraints(video-isp, true);
   pipe-l3_ick = clk_get_rate(video-isp-clock[ISP_CLK_L3_ICK]);
 @@ -1001,6 +1005,11 @@ isp_video_streamon(struct file *file, void *fh, enum
 v4l2_buf_type type) if (ret  0)
   goto err_pipeline_start;
 
 + entity = video-video.entity;
 + media_entity_graph_walk_start(graph, entity);
 + while ((entity = media_entity_graph_walk_next(graph)))
 + pipe-entities |= 1  entity-id;
 +

To avoid walking the graph one more time, what about moving this to 
isp_video_far_end() where we already walk the graph (and moving the 
isp_video_far_end() call earlier in this function) ? You could possible rename 
isp_video_far_end() to something a bit more in line with its new purpose then.

   /* Verify that the currently configured format matches the output of
* the connected subdev.
*/
 diff --git a/drivers/media/video/omap3isp/ispvideo.h
 b/drivers/media/video/omap3isp/ispvideo.h index d91bdb91..0423c9d 100644
 --- a/drivers/media/video/omap3isp/ispvideo.h
 +++ b/drivers/media/video/omap3isp/ispvideo.h
 @@ -96,6 +96,7 @@ struct isp_pipeline {
   enum isp_pipeline_stream_state stream_state;
   struct isp_video *input;
   struct isp_video *output;
 + u32 entities;
   unsigned long l3_ick;
   unsigned int max_rate;
   atomic_t frame_number;

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v5 25/35] omap3isp: Collect entities that are part of the pipeline

2012-03-07 Thread Laurent Pinchart
Hi Sakari,

Another comment.

On Tuesday 06 March 2012 18:33:06 Sakari Ailus wrote:
 Collect entities which are part of the pipeline into a single bit mask.
 
 Signed-off-by: Sakari Ailus sakari.ai...@iki.fi

[snip]

 diff --git a/drivers/media/video/omap3isp/ispvideo.h
 b/drivers/media/video/omap3isp/ispvideo.h index d91bdb91..0423c9d 100644
 --- a/drivers/media/video/omap3isp/ispvideo.h
 +++ b/drivers/media/video/omap3isp/ispvideo.h
 @@ -96,6 +96,7 @@ struct isp_pipeline {
   enum isp_pipeline_stream_state stream_state;
   struct isp_video *input;
   struct isp_video *output;
 + u32 entities;
   unsigned long l3_ick;
   unsigned int max_rate;
   atomic_t frame_number;

Could you please update the structure documentation ?

@entities: Bitmask of entities in the pipeline (indexed by entity ID)

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v5 25/35] omap3isp: Collect entities that are part of the pipeline

2012-03-07 Thread Sakari Ailus
Hi Laurent,

On Wed, Mar 07, 2012 at 11:50:19AM +0100, Laurent Pinchart wrote:
 Hi Sakari,
 
 Another comment.
 
 On Tuesday 06 March 2012 18:33:06 Sakari Ailus wrote:
  Collect entities which are part of the pipeline into a single bit mask.
  
  Signed-off-by: Sakari Ailus sakari.ai...@iki.fi
 
 [snip]
 
  diff --git a/drivers/media/video/omap3isp/ispvideo.h
  b/drivers/media/video/omap3isp/ispvideo.h index d91bdb91..0423c9d 100644
  --- a/drivers/media/video/omap3isp/ispvideo.h
  +++ b/drivers/media/video/omap3isp/ispvideo.h
  @@ -96,6 +96,7 @@ struct isp_pipeline {
  enum isp_pipeline_stream_state stream_state;
  struct isp_video *input;
  struct isp_video *output;
  +   u32 entities;
  unsigned long l3_ick;
  unsigned int max_rate;
  atomic_t frame_number;
 
 Could you please update the structure documentation ?
 
 @entities: Bitmask of entities in the pipeline (indexed by entity ID)

Sure. I'll take that from your patch. ;-)

-- 
Sakari Ailus
e-mail: sakari.ai...@iki.fi jabber/XMPP/Gmail: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v5 25/35] omap3isp: Collect entities that are part of the pipeline

2012-03-07 Thread Sakari Ailus
Hi Laurent,

On Wed, Mar 07, 2012 at 11:35:02AM +0100, Laurent Pinchart wrote:
 On Tuesday 06 March 2012 18:33:06 Sakari Ailus wrote:
  Collect entities which are part of the pipeline into a single bit mask.
  
  Signed-off-by: Sakari Ailus sakari.ai...@iki.fi
  ---
   drivers/media/video/omap3isp/ispvideo.c |9 +
   drivers/media/video/omap3isp/ispvideo.h |1 +
   2 files changed, 10 insertions(+), 0 deletions(-)
  
  diff --git a/drivers/media/video/omap3isp/ispvideo.c
  b/drivers/media/video/omap3isp/ispvideo.c index d34f690..4bc9cca 100644
  --- a/drivers/media/video/omap3isp/ispvideo.c
  +++ b/drivers/media/video/omap3isp/ispvideo.c
  @@ -970,6 +970,8 @@ isp_video_streamon(struct file *file, void *fh, enum
  v4l2_buf_type type) {
  struct isp_video_fh *vfh = to_isp_video_fh(fh);
  struct isp_video *video = video_drvdata(file);
  +   struct media_entity_graph graph;
  +   struct media_entity *entity;
  enum isp_pipeline_state state;
  struct isp_pipeline *pipe;
  struct isp_video *far_end;
  @@ -992,6 +994,8 @@ isp_video_streamon(struct file *file, void *fh, enum
  v4l2_buf_type type) pipe = video-video.entity.pipe
   ? to_isp_pipeline(video-video.entity) : video-pipe;
  
  +   pipe-entities = 0;
  +
 
 This could be move right before the graph walk code below to keep both parts 
 together. However, pipe-entities would then be invalid (instead of always 0) 
 in the link validation operations. That can be considered as an issue, so I'm 
 fine if you prefer leaving this assignment here.

That's what I also thought, so let's leave it there.

  if (video-isp-pdata-set_constraints)
  video-isp-pdata-set_constraints(video-isp, true);
  pipe-l3_ick = clk_get_rate(video-isp-clock[ISP_CLK_L3_ICK]);
  @@ -1001,6 +1005,11 @@ isp_video_streamon(struct file *file, void *fh, enum
  v4l2_buf_type type) if (ret  0)
  goto err_pipeline_start;
  
  +   entity = video-video.entity;
  +   media_entity_graph_walk_start(graph, entity);
  +   while ((entity = media_entity_graph_walk_next(graph)))
  +   pipe-entities |= 1  entity-id;
  +
 
 To avoid walking the graph one more time, what about moving this to 
 isp_video_far_end() where we already walk the graph (and moving the 
 isp_video_far_end() call earlier in this function) ? You could possible 
 rename 
 isp_video_far_end() to something a bit more in line with its new purpose then.

isp_video_get_graph_data()?

-- 
Sakari Ailus
e-mail: sakari.ai...@iki.fi jabber/XMPP/Gmail: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH v5 25/35] omap3isp: Collect entities that are part of the pipeline

2012-03-06 Thread Sakari Ailus
Collect entities which are part of the pipeline into a single bit mask.

Signed-off-by: Sakari Ailus sakari.ai...@iki.fi
---
 drivers/media/video/omap3isp/ispvideo.c |9 +
 drivers/media/video/omap3isp/ispvideo.h |1 +
 2 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/omap3isp/ispvideo.c 
b/drivers/media/video/omap3isp/ispvideo.c
index d34f690..4bc9cca 100644
--- a/drivers/media/video/omap3isp/ispvideo.c
+++ b/drivers/media/video/omap3isp/ispvideo.c
@@ -970,6 +970,8 @@ isp_video_streamon(struct file *file, void *fh, enum 
v4l2_buf_type type)
 {
struct isp_video_fh *vfh = to_isp_video_fh(fh);
struct isp_video *video = video_drvdata(file);
+   struct media_entity_graph graph;
+   struct media_entity *entity;
enum isp_pipeline_state state;
struct isp_pipeline *pipe;
struct isp_video *far_end;
@@ -992,6 +994,8 @@ isp_video_streamon(struct file *file, void *fh, enum 
v4l2_buf_type type)
pipe = video-video.entity.pipe
 ? to_isp_pipeline(video-video.entity) : video-pipe;
 
+   pipe-entities = 0;
+
if (video-isp-pdata-set_constraints)
video-isp-pdata-set_constraints(video-isp, true);
pipe-l3_ick = clk_get_rate(video-isp-clock[ISP_CLK_L3_ICK]);
@@ -1001,6 +1005,11 @@ isp_video_streamon(struct file *file, void *fh, enum 
v4l2_buf_type type)
if (ret  0)
goto err_pipeline_start;
 
+   entity = video-video.entity;
+   media_entity_graph_walk_start(graph, entity);
+   while ((entity = media_entity_graph_walk_next(graph)))
+   pipe-entities |= 1  entity-id;
+
/* Verify that the currently configured format matches the output of
 * the connected subdev.
 */
diff --git a/drivers/media/video/omap3isp/ispvideo.h 
b/drivers/media/video/omap3isp/ispvideo.h
index d91bdb91..0423c9d 100644
--- a/drivers/media/video/omap3isp/ispvideo.h
+++ b/drivers/media/video/omap3isp/ispvideo.h
@@ -96,6 +96,7 @@ struct isp_pipeline {
enum isp_pipeline_stream_state stream_state;
struct isp_video *input;
struct isp_video *output;
+   u32 entities;
unsigned long l3_ick;
unsigned int max_rate;
atomic_t frame_number;
-- 
1.7.2.5

--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html