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<[email protected]>
---
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.
_______________________________________________
devicetree-discuss mailing list
[email protected]
https://lists.ozlabs.org/listinfo/devicetree-discuss
