On 03/06/2013 12:53 PM, Shaik Ameer Basha wrote:
This patch modifies the current fimc_pipeline to exynos_pipeline,
I think we could leave it as fimc_pipeline, exynos_pipeline seems too generic to me.
which can be used across multiple media device drivers. Signed-off-by: Shaik Ameer Basha<shaik.am...@samsung.com> --- drivers/media/platform/s5p-fimc/fimc-capture.c | 96 +++++++----- drivers/media/platform/s5p-fimc/fimc-core.h | 4 +- drivers/media/platform/s5p-fimc/fimc-lite.c | 73 ++++------ drivers/media/platform/s5p-fimc/fimc-lite.h | 4 +- drivers/media/platform/s5p-fimc/fimc-mdevice.c | 186 ++++++++++++++++++++++-- drivers/media/platform/s5p-fimc/fimc-mdevice.h | 41 +++--- include/media/s5p_fimc.h | 66 ++++++--- 7 files changed, 326 insertions(+), 144 deletions(-) diff --git a/drivers/media/platform/s5p-fimc/fimc-capture.c b/drivers/media/platform/s5p-fimc/fimc-capture.c index 4cbaf46..106466e 100644 --- a/drivers/media/platform/s5p-fimc/fimc-capture.c +++ b/drivers/media/platform/s5p-fimc/fimc-capture.c @@ -27,24 +27,26 @@ #include<media/videobuf2-core.h> #include<media/videobuf2-dma-contig.h> -#include "fimc-mdevice.h" #include "fimc-core.h" #include "fimc-reg.h" static int fimc_capture_hw_init(struct fimc_dev *fimc) { struct fimc_ctx *ctx = fimc->vid_cap.ctx; - struct fimc_pipeline *p =&fimc->pipeline; + struct exynos_pipeline *p =&fimc->pipeline; struct fimc_sensor_info *sensor; unsigned long flags; + struct v4l2_subdev *sd; int ret = 0; - if (p->subdevs[IDX_SENSOR] == NULL || ctx == NULL) + sd = exynos_pipeline_get_subdev(fimc->pipeline_ops, + get_subdev_sensor, p);
Hmm, it feels it is going wrong path this way. I would keep changes to the s5p-fimc driver as small as possible. And the modules that are shared across the exynos4 and exynos5 driver should use generic media graph walking routines where possible.
+ if (sd == NULL || ctx == NULL) return -ENXIO; if (ctx->s_frame.fmt == NULL) return -EINVAL; - sensor = v4l2_get_subdev_hostdata(p->subdevs[IDX_SENSOR]); + sensor = v4l2_get_subdev_hostdata(sd); spin_lock_irqsave(&fimc->slock, flags); fimc_prepare_dma_offset(ctx,&ctx->d_frame);
...
@@ -486,9 +491,12 @@ static struct vb2_ops fimc_capture_qops = { int fimc_capture_ctrls_create(struct fimc_dev *fimc) { struct fimc_vid_cap *vid_cap =&fimc->vid_cap; - struct v4l2_subdev *sensor = fimc->pipeline.subdevs[IDX_SENSOR]; + struct v4l2_subdev *sensor; int ret; + sensor = exynos_pipeline_get_subdev(fimc->pipeline_ops, + get_subdev_sensor,&fimc->pipeline); + if (WARN_ON(vid_cap->ctx == NULL)) return -ENXIO; if (vid_cap->ctx->ctrls.ready) @@ -513,7 +521,7 @@ static int fimc_capture_open(struct file *file) dbg("pid: %d, state: 0x%lx", task_pid_nr(current), fimc->state); - fimc_md_graph_lock(fimc); + exynos_pipeline_graph_lock(fimc->pipeline_ops,&fimc->pipeline);
Hmm, this look pretty scary to me. But I suspect this change is not needed at all. The graph lock is _not_ the pipeline lock. It protects all media entities registered to the media device, and links between them. Not only entities linked into specific video processing pipeline at a moment.
mutex_lock(&fimc->lock); if (fimc_m2m_active(fimc)) @@ -531,7 +539,7 @@ static int fimc_capture_open(struct file *file) } if (++fimc->vid_cap.refcnt == 1) { - ret = fimc_pipeline_call(fimc, open,&fimc->pipeline, + ret = exynos_pipeline_call(fimc, open,&fimc->pipeline, &fimc->vid_cap.vfd.entity, true); if (!ret&& !fimc->vid_cap.user_subdev_api) @@ -549,7 +557,7 @@ static int fimc_capture_open(struct file *file) } unlock: mutex_unlock(&fimc->lock); - fimc_md_graph_unlock(fimc); + exynos_pipeline_graph_unlock(fimc->pipeline_ops,&fimc->pipeline); return ret; }
...
/** diff --git a/drivers/media/platform/s5p-fimc/fimc-lite.c b/drivers/media/platform/s5p-fimc/fimc-lite.c index 3266c3f..122cf95 100644 --- a/drivers/media/platform/s5p-fimc/fimc-lite.c +++ b/drivers/media/platform/s5p-fimc/fimc-lite.c
...
-/* Called with the media graph mutex held */ -static struct v4l2_subdev *__find_remote_sensor(struct media_entity *me) -{ - struct media_pad *pad =&me->pads[0]; - struct v4l2_subdev *sd; - - while (pad->flags& MEDIA_PAD_FL_SINK) { - /* source pad */ - pad = media_entity_remote_source(pad); - if (pad == NULL || - media_entity_type(pad->entity) != MEDIA_ENT_T_V4L2_SUBDEV) - break; - - sd = media_entity_to_v4l2_subdev(pad->entity); - - if (sd->grp_id == GRP_ID_FIMC_IS_SENSOR) - return sd; - /* sink pad */ - pad =&sd->entity.pads[0]; - } - return NULL; -}
What's wrong with this function ? Why doesn't it work for you ? I think we should drop direct usage of fimc->pipeline->subdevs[IDX_SENSOR] instead. The sensor group IDs should probably be moved to some common header file.
diff --git a/drivers/media/platform/s5p-fimc/fimc-mdevice.c b/drivers/media/platform/s5p-fimc/fimc-mdevice.c index fc93fad..938cc56 100644 --- a/drivers/media/platform/s5p-fimc/fimc-mdevice.c +++ b/drivers/media/platform/s5p-fimc/fimc-mdevice.c @@ -36,6 +36,8 @@ #include "fimc-mdevice.h" #include "mipi-csis.h" +static struct fimc_md *g_fimc_mdev;
Hm, this is pretty ugly. Can you explain why it is needed ?
static int __fimc_md_set_camclk(struct fimc_md *fmd, struct fimc_sensor_info *s_info, bool on); @@ -143,6 +145,73 @@ static int fimc_pipeline_s_power(struct fimc_pipeline *p, bool state) } /** + * __fimc_pipeline_init + * allocate the fimc_pipeline structure and do the basic initialization
This is not a proper kernel-doc style.
+ */ +static int __fimc_pipeline_init(struct exynos_pipeline *ep) +{ + struct fimc_pipeline *p; + + p = kzalloc(sizeof(*p), GFP_KERNEL); + if (!p) + return -ENOMEM;
Why this needs to be allocated dynamically ?
+ p->is_init = true; + p->fmd = g_fimc_mdev; + ep->priv = (void *)p; + return 0; +} + +/** + * __fimc_pipeline_deinit + * free the allocated resources for fimc_pipeline + */ +static int __fimc_pipeline_deinit(struct exynos_pipeline *ep) +{ + struct fimc_pipeline *p = (struct fimc_pipeline *)ep->priv; + + if (!p || !p->is_init) + return -EINVAL; + + p->is_init = false; + kfree(p); + + return 0; +} + +/** + * __fimc_pipeline_get_subdev_sensor + * if valid pipeline, returns the sensor subdev pointer + * else returns NULL + */ +static struct v4l2_subdev *__fimc_pipeline_get_subdev_sensor( + struct exynos_pipeline *ep) +{ + struct fimc_pipeline *p = (struct fimc_pipeline *)ep->priv; + + if (!p || !p->is_init) + return NULL; + + return p->subdevs[IDX_SENSOR]; +} + +/** + * __fimc_pipeline_get_subdev_csis + * if valid pipeline, returns the csis subdev pointer + * else returns NULL + */ +static struct v4l2_subdev *__fimc_pipeline_get_subdev_csis( + struct exynos_pipeline *ep) +{ + struct fimc_pipeline *p = (struct fimc_pipeline *)ep->priv; + + if (!p || !p->is_init) + return NULL; + + return p->subdevs[IDX_CSIS]; +} + +/** * __fimc_pipeline_open - update the pipeline information, enable power * of all pipeline subdevs and the sensor clock * @me: media entity to start graph walk with @@ -150,11 +219,15 @@ static int fimc_pipeline_s_power(struct fimc_pipeline *p, bool state) * * Called with the graph mutex held. */ -static int __fimc_pipeline_open(struct fimc_pipeline *p, +static int __fimc_pipeline_open(struct exynos_pipeline *ep, struct media_entity *me, bool prep) { + struct fimc_pipeline *p = (struct fimc_pipeline *)ep->priv; int ret; + if (!p || !p->is_init) + return -EINVAL; + if (prep) fimc_pipeline_prepare(p, me); @@ -174,17 +247,20 @@ static int __fimc_pipeline_open(struct fimc_pipeline *p, * * Disable power of all subdevs and turn the external sensor clock off. */ -static int __fimc_pipeline_close(struct fimc_pipeline *p) +static int __fimc_pipeline_close(struct exynos_pipeline *ep) { + struct fimc_pipeline *p = (struct fimc_pipeline *)ep->priv; int ret = 0; - if (!p || !p->subdevs[IDX_SENSOR]) + if (!p || !p->is_init) return -EINVAL; - if (p->subdevs[IDX_SENSOR]) { - ret = fimc_pipeline_s_power(p, 0); - fimc_md_set_camclk(p->subdevs[IDX_SENSOR], false); - } + if (!p->subdevs[IDX_SENSOR]) + return -EINVAL; + + ret = fimc_pipeline_s_power(p, 0); + fimc_md_set_camclk(p->subdevs[IDX_SENSOR], false); + return ret == -ENXIO ? 0 : ret; } @@ -193,10 +269,14 @@ static int __fimc_pipeline_close(struct fimc_pipeline *p) * @pipeline: video pipeline structure * @on: passed as the s_stream call argument */ -static int __fimc_pipeline_s_stream(struct fimc_pipeline *p, bool on) +static int __fimc_pipeline_s_stream(struct exynos_pipeline *ep, bool on) { + struct fimc_pipeline *p = (struct fimc_pipeline *)ep->priv; int i, ret; + if (!p || !p->is_init) + return -EINVAL; + if (p->subdevs[IDX_SENSOR] == NULL) return -ENODEV; @@ -213,11 +293,47 @@ static int __fimc_pipeline_s_stream(struct fimc_pipeline *p, bool on) } +static void __fimc_pipeline_graph_lock(struct exynos_pipeline *ep) +{ + struct fimc_pipeline *p = (struct fimc_pipeline *)ep->priv; + struct fimc_md *fmd = p->fmd; + + mutex_lock(&fmd->media_dev.graph_mutex); +} + +static void __fimc_pipeline_graph_unlock(struct exynos_pipeline *ep) +{ + struct fimc_pipeline *p = (struct fimc_pipeline *)ep->priv; + struct fimc_md *fmd = p->fmd; + + mutex_unlock(&fmd->media_dev.graph_mutex); +}
This seems really wrong to me. You can reference the media graph mutex from a subdev or video device through its 'entity' member, e.g. sd->entity.parent->graph_mutex. ...
static int fimc_md_probe(struct platform_device *pdev) { struct device *dev =&pdev->dev; @@ -1175,7 +1327,7 @@ static int fimc_md_probe(struct platform_device *pdev) v4l2_dev =&fmd->v4l2_dev; v4l2_dev->mdev =&fmd->media_dev; - v4l2_dev->notify = fimc_sensor_notify; + v4l2_dev->notify = fimc_md_sensor_notify; strlcpy(v4l2_dev->name, "s5p-fimc-md", sizeof(v4l2_dev->name)); @@ -1194,6 +1346,7 @@ static int fimc_md_probe(struct platform_device *pdev) goto err_clk; fmd->user_subdev_api = (dev->of_node != NULL); + g_fimc_mdev = fmd; /* Protect the media graph while we're registering entities */ mutex_lock(&fmd->media_dev.graph_mutex); @@ -1252,6 +1405,7 @@ static int fimc_md_remove(struct platform_device *pdev) if (!fmd) return 0; + g_fimc_mdev = NULL; device_remove_file(&pdev->dev,&dev_attr_subdev_conf_mode); fimc_md_unregister_entities(fmd); media_device_unregister(&fmd->media_dev); diff --git a/drivers/media/platform/s5p-fimc/fimc-mdevice.h b/drivers/media/platform/s5p-fimc/fimc-mdevice.h index f3e0251..1ea7acf 100644 --- a/drivers/media/platform/s5p-fimc/fimc-mdevice.h +++ b/drivers/media/platform/s5p-fimc/fimc-mdevice.h @@ -37,6 +37,15 @@ #define FIMC_MAX_SENSORS 8 #define FIMC_MAX_CAMCLKS 2 +enum fimc_subdev_index { + IDX_SENSOR, + IDX_CSIS, + IDX_FLITE, + IDX_IS_ISP, + IDX_FIMC, + IDX_MAX, +}; + struct fimc_csis_info { struct v4l2_subdev *sd; int id; @@ -49,20 +58,6 @@ struct fimc_camclk_info { }; /** - * struct fimc_sensor_info - image data source subdev information - * @pdata: sensor's atrributes passed as media device's platform data - * @subdev: image sensor v4l2 subdev - * @host: fimc device the sensor is currently linked to - * - * This data structure applies to image sensor and the writeback subdevs. - */ -struct fimc_sensor_info { - struct fimc_source_info pdata; - struct v4l2_subdev *subdev; - struct fimc_dev *host; -}; - -/** * struct fimc_md - fimc media device information * @csis: MIPI CSIS subdevs data * @sensor: array of registered sensor subdevs @@ -89,6 +84,14 @@ struct fimc_md { spinlock_t slock; }; +struct fimc_pipeline { + int is_init; + struct fimc_md *fmd; + struct v4l2_subdev *subdevs[IDX_MAX]; + void (*sensor_notify)(struct v4l2_subdev *sd, + unsigned int notification, void *arg); +};
This is supposed to be the s5p-fimc driver specific only data structure, right ?
#define is_subdev_pad(pad) (pad == NULL || \ media_entity_type(pad->entity) == MEDIA_ENT_T_V4L2_SUBDEV) @@ -103,16 +106,6 @@ static inline struct fimc_md *entity_to_fimc_mdev(struct media_entity *me) container_of(me->parent, struct fimc_md, media_dev); } -static inline void fimc_md_graph_lock(struct fimc_dev *fimc) -{ - mutex_lock(&fimc->vid_cap.vfd.entity.parent->graph_mutex); -} - -static inline void fimc_md_graph_unlock(struct fimc_dev *fimc) -{ - mutex_unlock(&fimc->vid_cap.vfd.entity.parent->graph_mutex); -}
Why to remove these s5p-fimc driver private functions ?
int fimc_md_set_camclk(struct v4l2_subdev *sd, bool on); #endif diff --git a/include/media/s5p_fimc.h b/include/media/s5p_fimc.h index e2434bb..007e998 100644 --- a/include/media/s5p_fimc.h +++ b/include/media/s5p_fimc.h @@ -13,6 +13,7 @@ #define S5P_FIMC_H_ #include<media/media-entity.h> +#include<media/v4l2-subdev.h> /* * Enumeration of data inputs to the camera subsystem. @@ -75,6 +76,20 @@ struct fimc_source_info { }; /** + * struct fimc_sensor_info - image data source subdev information + * @pdata: sensor's atrributes passed as media device's platform data + * @subdev: image sensor v4l2 subdev + * @host: capture device the sensor is currently linked to + * + * This data structure applies to image sensor and the writeback subdevs. + */ +struct fimc_sensor_info { + struct fimc_source_info pdata; + struct v4l2_subdev *subdev; + void *host; +}; + +/** * struct s5p_platform_fimc - camera host interface platform data * * @source_info: properties of an image source for the host interface setup @@ -93,21 +108,10 @@ struct s5p_platform_fimc { */ #define S5P_FIMC_TX_END_NOTIFY _IO('e', 0) -enum fimc_subdev_index { - IDX_SENSOR, - IDX_CSIS, - IDX_FLITE, - IDX_IS_ISP, - IDX_FIMC, - IDX_MAX, -}; - -struct media_pipeline; -struct v4l2_subdev; -struct fimc_pipeline { - struct v4l2_subdev *subdevs[IDX_MAX]; - struct media_pipeline *m_pipeline; +struct exynos_pipeline { + struct media_pipeline m_pipeline; + void *priv; }; /* @@ -115,15 +119,39 @@ struct fimc_pipeline { * video node when it is the last entity of the pipeline. Implemented * by corresponding media device driver. */ -struct fimc_pipeline_ops { - int (*open)(struct fimc_pipeline *p, struct media_entity *me, +struct exynos_pipeline_ops { + int (*init) (struct exynos_pipeline *p); + int (*deinit) (struct exynos_pipeline *p);
How about naming it 'free' instead ?
+ int (*open)(struct exynos_pipeline *p, struct media_entity *me, bool resume); - int (*close)(struct fimc_pipeline *p); - int (*set_stream)(struct fimc_pipeline *p, bool state); + int (*close)(struct exynos_pipeline *p); + int (*set_stream)(struct exynos_pipeline *p, bool state); + void (*graph_lock)(struct exynos_pipeline *p); + void (*graph_unlock)(struct exynos_pipeline *p);
Why do you think these graph callbacks are needed here ?
+ struct v4l2_subdev *(*get_subdev_sensor)(struct exynos_pipeline *p); + struct v4l2_subdev *(*get_subdev_csis)(struct exynos_pipeline *p);
No, we should instead use generic media graph walking routines in the shared drivers (FIMC-LITE, MIPI-CSIS). FIMC driver could be making certain assumptions about it's related media device driver. These drivers are contained in a single module anyway.
+ void (*register_notify_cb)(struct exynos_pipeline *p, + void (*cb)(struct v4l2_subdev *sd, + unsigned int notification, void *arg));
Hmm, I need to think more about this. AFAIR this would be only needed for S5P/Exynos4 FIMC. -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html