Re: [PATCH 2/3] tcm825x: Remove driver
On Mon, Nov 12, 2012 at 2:47 PM, Sakari Ailus sakari.ai...@iki.fi wrote:
Remove tcm825x driver. It uses the obsolete V4L2 int device framework, and
can only work with omap24xxcam driver.
Signed-off-by: Sakari Ailus sakari.ai...@iki.fi
I wonder if you will ever fix and send it again :)
Acked-by: David Cohen daco...@gmail.com
---
drivers/media/i2c/Kconfig |8 -
drivers/media/i2c/Makefile |1 -
drivers/media/i2c/tcm825x.c | 937
---
drivers/media/i2c/tcm825x.h | 200 -
4 files changed, 0 insertions(+), 1146 deletions(-)
delete mode 100644 drivers/media/i2c/tcm825x.c
delete mode 100644 drivers/media/i2c/tcm825x.h
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 24d78e2..aa8f18c 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -475,14 +475,6 @@ config VIDEO_MT9V032
This is a Video4Linux2 sensor-level driver for the Micron
MT9V032 752x480 CMOS sensor.
-config VIDEO_TCM825X
- tristate TCM825x camera sensor support
- depends on I2C VIDEO_V4L2
- depends on MEDIA_CAMERA_SUPPORT
- ---help---
- This is a driver for the Toshiba TCM825x VGA camera sensor.
- It is used for example in Nokia N800.
-
config VIDEO_SR030PC30
tristate Siliconfile SR030PC30 sensor support
depends on I2C VIDEO_V4L2
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index b1d62df..eb1757f 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -47,7 +47,6 @@ obj-$(CONFIG_VIDEO_VP27SMPX) += vp27smpx.o
obj-$(CONFIG_VIDEO_UPD64031A) += upd64031a.o
obj-$(CONFIG_VIDEO_UPD64083) += upd64083.o
obj-$(CONFIG_VIDEO_OV7670) += ov7670.o
-obj-$(CONFIG_VIDEO_TCM825X) += tcm825x.o
obj-$(CONFIG_VIDEO_TVEEPROM) += tveeprom.o
obj-$(CONFIG_VIDEO_MT9M032) += mt9m032.o
obj-$(CONFIG_VIDEO_MT9P031) += mt9p031.o
diff --git a/drivers/media/i2c/tcm825x.c b/drivers/media/i2c/tcm825x.c
deleted file mode 100644
index 9252529..000
--- a/drivers/media/i2c/tcm825x.c
+++ /dev/null
@@ -1,937 +0,0 @@
-/*
- * drivers/media/i2c/tcm825x.c
- *
- * TCM825X camera sensor driver.
- *
- * Copyright (C) 2007 Nokia Corporation.
- *
- * Contact: Sakari Ailus sakari.ai...@nokia.com
- *
- * Based on code from David Cohen david.co...@indt.org.br
- *
- * This driver was based on ov9640 sensor driver from MontaVista
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License
- * version 2 as published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful, but
- * WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
- * General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
- * 02110-1301 USA
- */
-
-#include linux/i2c.h
-#include linux/module.h
-#include media/v4l2-int-device.h
-
-#include tcm825x.h
-
-/*
- * The sensor has two fps modes: the lower one just gives half the fps
- * at the same xclk than the high one.
- */
-#define MAX_FPS 30
-#define MIN_FPS 8
-#define MAX_HALF_FPS (MAX_FPS / 2)
-#define HIGH_FPS_MODE_LOWER_LIMIT 14
-#define DEFAULT_FPS MAX_HALF_FPS
-
-struct tcm825x_sensor {
- const struct tcm825x_platform_data *platform_data;
- struct v4l2_int_device *v4l2_int_device;
- struct i2c_client *i2c_client;
- struct v4l2_pix_format pix;
- struct v4l2_fract timeperframe;
-};
-
-/* list of image formats supported by TCM825X sensor */
-static const struct v4l2_fmtdesc tcm825x_formats[] = {
- {
- .description = YUYV (YUV 4:2:2), packed,
- .pixelformat = V4L2_PIX_FMT_UYVY,
- }, {
- /* Note: V4L2 defines RGB565 as:
-*
-* Byte 0Byte 1
-* g2 g1 g0 r4 r3 r2 r1 r0 b4 b3 b2 b1 b0 g5 g4 g3
-*
-* We interpret RGB565 as:
-*
-* Byte 0Byte 1
-* g2 g1 g0 b4 b3 b2 b1 b0 r4 r3 r2 r1 r0 g5 g4 g3
-*/
- .description = RGB565, le,
- .pixelformat = V4L2_PIX_FMT_RGB565,
- },
-};
-
-#define TCM825X_NUM_CAPTURE_FORMATSARRAY_SIZE(tcm825x_formats)
-
-/*
- * TCM825X register configuration for all combinations of pixel format and
- * image size
- */
-static const struct tcm825x_reg subqcif= { 0x20,
TCM825X_PICSIZ };
-static const struct tcm825x_reg qcif = { 0x18, TCM825X_PICSIZ };
-static const struct tcm825x_reg
Re: [PATCH v2 8/9] soc-camera: Add and use soc_camera_power_[on|off]() helper functions
Hi Laurent,
On 07/17/2012 04:24 AM, Laurent Pinchart wrote:
Hi David,
Thanks for the review.
You're welcome.
On Monday 16 July 2012 02:17:43 David Cohen wrote:
On 07/05/2012 11:38 PM, Laurent Pinchart wrote:
Instead of forcing all soc-camera drivers to go through the mid-layer to
handle power management, create soc_camera_power_[on|off]() functions
that can be called from the subdev .s_power() operation to manage
regulators and platform-specific power handling. This allows non
soc-camera hosts to use soc-camera-aware clients.
Signed-off-by: Laurent Pinchart laurent.pinch...@ideasonboard.com
---
drivers/media/video/imx074.c |9 +++
drivers/media/video/mt9m001.c |9 +++
drivers/media/video/mt9m111.c | 52 +-
drivers/media/video/mt9t031.c | 11 +++-
drivers/media/video/mt9t112.c |9 +++
drivers/media/video/mt9v022.c |9 +++
drivers/media/video/ov2640.c |9 +++
drivers/media/video/ov5642.c | 10 +++-
drivers/media/video/ov6650.c |9 +++
drivers/media/video/ov772x.c |9 +++
drivers/media/video/ov9640.c | 10 +++-
drivers/media/video/ov9740.c | 15 +-
drivers/media/video/rj54n1cb0c.c |9 +++
drivers/media/video/soc_camera.c | 83
drivers/media/video/soc_camera_platform.c | 11 -
drivers/media/video/tw9910.c |9 +++
include/media/soc_camera.h| 10
17 files changed, 225 insertions(+), 58 deletions(-)
[snip]
diff --git a/drivers/media/video/ov9740.c b/drivers/media/video/ov9740.c
index 3eb07c2..effd0f1 100644
--- a/drivers/media/video/ov9740.c
+++ b/drivers/media/video/ov9740.c
@@ -786,16 +786,29 @@ static int ov9740_g_chip_ident(struct v4l2_subdev
*sd,
static int ov9740_s_power(struct v4l2_subdev *sd, int on)
{
+ struct i2c_client *client = v4l2_get_subdevdata(sd);
+ struct soc_camera_link *icl = soc_camera_i2c_to_link(client);
struct ov9740_priv *priv = to_ov9740(sd);
+ int ret;
- if (!priv-current_enable)
+ if (on) {
+ ret = soc_camera_power_on(client-dev, icl);
+ if (ret 0)
+ return ret;
+ }
+
+ if (!priv-current_enable) {
+ if (!on)
+ soc_camera_power_off(client-dev, icl);
After your changes, this function has 3 if's (one nested) where all of
them checks on variable due to you need to mix on and
priv-current_enable checks. However, code's traceability is not so
trivial.
How about if you nest priv-current_enable into last if and keep
only that one?
See an incomplete code below:
return 0;
+ }
if (on) {
soc_camera_power_on();
if (!priv-current_enable)
return;
ov9740_s_fmt(sd, priv-current_mf);
ov9740_s_stream(sd, priv-current_enable);
} else {
ov9740_s_stream(sd, 0);
Execute ov9740_s_stream() conditionally:
if (priv-current_enable) {
ov9740_s_stream();
priv-current_enable = true;
}
+ soc_camera_power_off(client-dev, icl);
priv-current_enable = true;
priv-current_enable is set to false when ov9740_s_stream(0) is called
then this function sets it back to true afterwards. So, in case you want
to have no functional change, it seems to me you should call
soc_camera_power_off() after that variable has its original value set
back.
In this case, even if you don't like my suggestion, you still need to
swap those 2 lines above. :)
What do you think of
Sounds good to me :)
Br,
David Cohen
diff --git a/drivers/media/video/ov9740.c b/drivers/media/video/ov9740.c
index 3eb07c2..10c0ba9 100644
--- a/drivers/media/video/ov9740.c
+++ b/drivers/media/video/ov9740.c
@@ -786,17 +786,27 @@ static int ov9740_g_chip_ident(struct v4l2_subdev *sd,
static int ov9740_s_power(struct v4l2_subdev *sd, int on)
{
+ struct i2c_client *client = v4l2_get_subdevdata(sd);
+ struct soc_camera_link *icl = soc_camera_i2c_to_link(client);
struct ov9740_priv *priv = to_ov9740(sd);
-
- if (!priv-current_enable)
- return 0;
+ int ret;
if (on) {
- ov9740_s_fmt(sd, priv-current_mf);
- ov9740_s_stream(sd, priv-current_enable);
+ ret = soc_camera_power_on(client-dev, icl);
+ if (ret 0)
+ return ret;
+
+ if (priv-current_enable) {
+ ov9740_s_fmt(sd, priv-current_mf);
+ ov9740_s_stream(sd, 1);
+ }
} else {
- ov9740_s_stream(sd, 0);
- priv-current_enable = true;
+ if (priv-current_enable) {
+ ov9740_s_stream
Re: [PATCH v2 7/9] soc-camera: Continue the power off sequence if one of the steps fails
Hi Laurent,
On 07/17/2012 02:45 AM, Laurent Pinchart wrote:
Hi David,
Thank you for the review.
You're welcome.
On Monday 16 July 2012 01:24:37 David Cohen wrote:
On 07/05/2012 11:38 PM, Laurent Pinchart wrote:
Powering off a device is a best effort task: failure to execute one of
the steps should not prevent the next steps to be executed. For
instance, an I2C communication error when putting the chip in stand-by
mode should not prevent the more agressive next step of turning the
chip's power supply off.
Signed-off-by: Laurent Pinchart laurent.pinch...@ideasonboard.com
---
drivers/media/video/soc_camera.c |9 +++--
1 files changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/media/video/soc_camera.c
b/drivers/media/video/soc_camera.c index 55b981f..bbd518f 100644
--- a/drivers/media/video/soc_camera.c
+++ b/drivers/media/video/soc_camera.c
@@ -89,18 +89,15 @@ static int soc_camera_power_off(struct
soc_camera_device *icd,
struct soc_camera_link *icl)
{
struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
- int ret = v4l2_subdev_call(sd, core, s_power, 0);
+ int ret;
- if (ret 0 ret != -ENOIOCTLCMD ret != -ENODEV)
- return ret;
+ v4l2_subdev_call(sd, core, s_power, 0);
Fair enough. I agree we should not prevent power off because of failure
in this step. But IMO we should not silently bypass it too. How about
an error message?
I'll add that.
if (icl-power) {
ret = icl-power(icd-control, 0);
- if (ret 0) {
+ if (ret 0)
dev_err(icd-pdev,
Platform failed to power-off the camera.\n);
- return ret;
- }
}
ret = regulator_bulk_disable(icl-num_regulators,
One more comment. Should this function's return value being based fully
on last action? If any earlier error happened but this last step is
fine, IMO we should not return 0.
Good point. What about this (on top of the current patch) ?
That sounds nice to me :)
Regards,
David Cohen
diff --git a/drivers/media/video/soc_camera.c b/drivers/media/video/soc_camera.c
index bbd518f..7bf21da 100644
--- a/drivers/media/video/soc_camera.c
+++ b/drivers/media/video/soc_camera.c
@@ -89,21 +89,30 @@ static int soc_camera_power_off(struct soc_camera_device
*icd,
struct soc_camera_link *icl)
{
struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
- int ret;
+ int ret = 0;
+ int err;
- v4l2_subdev_call(sd, core, s_power, 0);
+ err = v4l2_subdev_call(sd, core, s_power, 0);
+ if (err 0 err != -ENOIOCTLCMD err != -ENODEV) {
+ dev_err(icd-pdev, Subdev failed to power-off the camera.\n);
+ ret = err;
+ }
if (icl-power) {
- ret = icl-power(icd-control, 0);
- if (ret 0)
+ err = icl-power(icd-control, 0);
+ if (err 0) {
dev_err(icd-pdev,
Platform failed to power-off the camera.\n);
+ ret = ret ? : err;
+ }
}
- ret = regulator_bulk_disable(icl-num_regulators,
+ err = regulator_bulk_disable(icl-num_regulators,
icl-regulators);
- if (ret 0)
+ if (err 0) {
dev_err(icd-pdev, Cannot disable regulators\n);
+ ret = ret ? : err;
+ }
return ret;
}
--
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 2/2] v4l: Add v4l2 subdev driver for S5K4ECGX sensor
v4l2_subdev_core_ops s5k4ecgx_core_ops = {
+ .s_power = s5k4ecgx_s_power,
+ .log_status = s5k4ecgx_log_status,
+};
+
+static int __s5k4ecgx_s_stream(struct v4l2_subdev *sd, int on)
+{
+ struct s5k4ecgx *priv = to_s5k4ecgx(sd);
+ int err = 0;
+
+ if (on)
+ err = s5k4ecgx_write_array(sd, pview_size[priv-p_now-idx]);
+
+ return err;
+}
+
+static int s5k4ecgx_s_stream(struct v4l2_subdev *sd, int on)
+{
+ struct s5k4ecgx *priv = to_s5k4ecgx(sd);
+ int ret = 0;
+
+ v4l2_dbg(1, debug, sd, Turn streaming %s\n, on ? on : off);
+ mutex_lock(priv-lock);
+ if (on !priv-streaming)
+ ret = __s5k4ecgx_s_stream(sd, on);
+ else
+ priv-streaming = 0;
Is s_stream(1) is called twice, either you ignore it or return error.
But turning it to s_stream(0) isn't correct to me.
+ mutex_unlock(priv-lock);
+
+ return ret;
+}
+
+static const struct v4l2_subdev_video_ops s5k4ecgx_video_ops = {
+ .s_stream = s5k4ecgx_s_stream,
+};
+
+static const struct v4l2_subdev_ops s5k4ecgx_ops = {
+ .core = s5k4ecgx_core_ops,
+ .pad = s5k4ecgx_pad_ops,
+ .video = s5k4ecgx_video_ops,
+};
+
+static int s5k4ecgx_initialize_ctrls(struct s5k4ecgx *priv)
+{
+ const struct v4l2_ctrl_ops *ops = s5k4ecgx_ctrl_ops;
+ struct v4l2_ctrl_handler *hdl = priv-handler;
+ int ret;
+
+ ret = v4l2_ctrl_handler_init(hdl, 16);
+ if (ret)
+ return ret;
+
+ v4l2_ctrl_new_std(hdl, ops, V4L2_CID_BRIGHTNESS, -208, 127, 1, 0);
+ v4l2_ctrl_new_std(hdl, ops, V4L2_CID_CONTRAST, -127, 127, 1, 0);
+ v4l2_ctrl_new_std(hdl, ops, V4L2_CID_SATURATION, -127, 127, 1, 0);
+
+ /* For sharpness, 0x6024 is default value */
+ v4l2_ctrl_new_std(hdl, ops, V4L2_CID_SHARPNESS, -32704, 24612, 8208,
+ 24612);
+ if (hdl-error) {
+ ret = hdl-error;
+ v4l2_ctrl_handler_free(hdl);
+ return ret;
+ }
+ priv-sd.ctrl_handler = hdl;
+
+ return 0;
+};
+
+/*
+ * Set initial values for all preview presets
+ */
+static void s5k4ecgx_presets_data_init(struct s5k4ecgx *priv)
+{
+ struct s5k4ecgx_preset *preset = priv-presets[0];
+ int i;
+
+ for (i = 0; i S5K4ECGX_MAX_PRESETS; i++) {
+ preset-mbus_fmt.width = S5K4ECGX_OUT_WIDTH_DEF;
+ preset-mbus_fmt.height = S5K4ECGX_OUT_HEIGHT_DEF;
+ preset-mbus_fmt.code= s5k4ecgx_formats[0].code;
+ preset-index= i;
+ preset-clk_id = 0;
+ preset++;
+ }
+ priv-preset = priv-presets[0];
+}
+
+/*
+ * Fetching platform data is being done with s_config subdev call.
+ * In probe routine, we just register subdev device
+ */
+static int s5k4ecgx_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct v4l2_subdev *sd;
+ struct s5k4ecgx *priv;
+ struct s5k4ecgx_platform_data *pdata = client-dev.platform_data;
+ int ret;
+
+ if (pdata == NULL) {
+ dev_err(client-dev, platform data is missing!\n);
+ return -EINVAL;
+ }
+ priv = kzalloc(sizeof(struct s5k4ecgx), GFP_KERNEL);
+
+ if (!priv)
+ return -ENOMEM;
+
+ mutex_init(priv-lock);
+
+ priv-set_power = pdata-set_power;
+ priv-mdelay = pdata-mdelay;
+
+ sd = priv-sd;
+ /* Registering subdev */
+ v4l2_i2c_subdev_init(sd, client, s5k4ecgx_ops);
+ strlcpy(sd-name, S5K4ECGX_DRIVER_NAME, sizeof(sd-name));
+
+ sd-internal_ops = s5k4ecgx_subdev_internal_ops;
+ /* Support v4l2 sub-device userspace API */
+ sd-flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+
+ priv-pad.flags = MEDIA_PAD_FL_SOURCE;
+ sd-entity.type = MEDIA_ENT_T_V4L2_SUBDEV_SENSOR;
+ ret = media_entity_init(sd-entity, 1, priv-pad, 0);
+ if (ret)
+ goto out_err;
+
+ ret = s5k4ecgx_initialize_ctrls(priv);
+ s5k4ecgx_presets_data_init(priv);
+
+ if (ret)
+ goto out_err;
+ else
This else could be removed.
+ return 0;
+
+ out_err:
+ media_entity_cleanup(priv-sd.entity);
+ kfree(priv);
+
+ return ret;
+}
+
+static int s5k4ecgx_remove(struct i2c_client *client)
+{
+ struct v4l2_subdev *sd = i2c_get_clientdata(client);
+ struct s5k4ecgx *priv = to_s5k4ecgx(sd);
+
+ v4l2_device_unregister_subdev(sd);
+ v4l2_ctrl_handler_free(priv-handler);
+ media_entity_cleanup(sd-entity);
+ mutex_destroy(priv-lock);
For debugging purpose, maybe mutex_destroy() could be first one.
Kind regards.
David Cohen
+ kfree(priv);
+
+ return 0;
+}
+
+static const struct i2c_device_id s5k4ecgx_id[] = {
+ { S5K4ECGX_DRIVER_NAME, 0 },
+ {}
+};
+MODULE_DEVICE_TABLE(i2c, s5k4ecgx_id);
+
+static
Re: [PATCH v2 7/9] soc-camera: Continue the power off sequence if one of the steps fails
Hi Laurent,
Few comments below.
On 07/05/2012 11:38 PM, Laurent Pinchart wrote:
Powering off a device is a best effort task: failure to execute one of
the steps should not prevent the next steps to be executed. For
instance, an I2C communication error when putting the chip in stand-by
mode should not prevent the more agressive next step of turning the
chip's power supply off.
Signed-off-by: Laurent Pinchart laurent.pinch...@ideasonboard.com
---
drivers/media/video/soc_camera.c |9 +++--
1 files changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/media/video/soc_camera.c b/drivers/media/video/soc_camera.c
index 55b981f..bbd518f 100644
--- a/drivers/media/video/soc_camera.c
+++ b/drivers/media/video/soc_camera.c
@@ -89,18 +89,15 @@ static int soc_camera_power_off(struct soc_camera_device
*icd,
struct soc_camera_link *icl)
{
struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
- int ret = v4l2_subdev_call(sd, core, s_power, 0);
+ int ret;
- if (ret 0 ret != -ENOIOCTLCMD ret != -ENODEV)
- return ret;
+ v4l2_subdev_call(sd, core, s_power, 0);
Fair enough. I agree we should not prevent power off because of failure
in this step. But IMO we should not silently bypass it too. How about
an error message?
if (icl-power) {
ret = icl-power(icd-control, 0);
- if (ret 0) {
+ if (ret 0)
dev_err(icd-pdev,
Platform failed to power-off the camera.\n);
- return ret;
- }
}
ret = regulator_bulk_disable(icl-num_regulators,
One more comment. Should this function's return value being based fully
on last action? If any earlier error happened but this last step is
fine, IMO we should not return 0.
Kind regards,
David Cohen
--
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 v2 8/9] soc-camera: Add and use soc_camera_power_[on|off]() helper functions
Hi Laurent,
Few more comments below :)
On 07/05/2012 11:38 PM, Laurent Pinchart wrote:
Instead of forcing all soc-camera drivers to go through the mid-layer to
handle power management, create soc_camera_power_[on|off]() functions
that can be called from the subdev .s_power() operation to manage
regulators and platform-specific power handling. This allows non
soc-camera hosts to use soc-camera-aware clients.
Signed-off-by: Laurent Pinchart laurent.pinch...@ideasonboard.com
---
drivers/media/video/imx074.c |9 +++
drivers/media/video/mt9m001.c |9 +++
drivers/media/video/mt9m111.c | 52 +-
drivers/media/video/mt9t031.c | 11 +++-
drivers/media/video/mt9t112.c |9 +++
drivers/media/video/mt9v022.c |9 +++
drivers/media/video/ov2640.c |9 +++
drivers/media/video/ov5642.c | 10 +++-
drivers/media/video/ov6650.c |9 +++
drivers/media/video/ov772x.c |9 +++
drivers/media/video/ov9640.c | 10 +++-
drivers/media/video/ov9740.c | 15 +-
drivers/media/video/rj54n1cb0c.c |9 +++
drivers/media/video/soc_camera.c | 83 +
drivers/media/video/soc_camera_platform.c | 11 -
drivers/media/video/tw9910.c |9 +++
include/media/soc_camera.h| 10
17 files changed, 225 insertions(+), 58 deletions(-)
[snip]
diff --git a/drivers/media/video/ov9740.c b/drivers/media/video/ov9740.c
index 3eb07c2..effd0f1 100644
--- a/drivers/media/video/ov9740.c
+++ b/drivers/media/video/ov9740.c
@@ -786,16 +786,29 @@ static int ov9740_g_chip_ident(struct v4l2_subdev *sd,
static int ov9740_s_power(struct v4l2_subdev *sd, int on)
{
+ struct i2c_client *client = v4l2_get_subdevdata(sd);
+ struct soc_camera_link *icl = soc_camera_i2c_to_link(client);
struct ov9740_priv *priv = to_ov9740(sd);
+ int ret;
- if (!priv-current_enable)
+ if (on) {
+ ret = soc_camera_power_on(client-dev, icl);
+ if (ret 0)
+ return ret;
+ }
+
+ if (!priv-current_enable) {
+ if (!on)
+ soc_camera_power_off(client-dev, icl);
After your changes, this function has 3 if's (one nested) where all of
them checks on variable due to you need to mix on and
priv-current_enable checks. However, code's traceability is not so
trivial.
How about if you nest priv-current_enable into last if and keep
only that one?
See an incomplete code below:
return 0;
+ }
if (on) {
soc_camera_power_on();
if (!priv-current_enable)
return;
ov9740_s_fmt(sd, priv-current_mf);
ov9740_s_stream(sd, priv-current_enable);
} else {
ov9740_s_stream(sd, 0);
Execute ov9740_s_stream() conditionally:
if (priv-current_enable) {
ov9740_s_stream();
priv-current_enable = true;
}
+ soc_camera_power_off(client-dev, icl);
priv-current_enable = true;
priv-current_enable is set to false when ov9740_s_stream(0) is called
then this function sets it back to true afterwards. So, in case you want
to have no functional change, it seems to me you should call
soc_camera_power_off() after that variable has its original value set
back.
In this case, even if you don't like my suggestion, you still need to
swap those 2 lines above. :)
Br,
David Cohen
}
diff --git a/drivers/media/video/rj54n1cb0c.c b/drivers/media/video/rj54n1cb0c.c
index f6419b2..ca1cee7 100644
--- a/drivers/media/video/rj54n1cb0c.c
+++ b/drivers/media/video/rj54n1cb0c.c
@@ -1180,6 +1180,14 @@ static int rj54n1_s_register(struct v4l2_subdev *sd,
}
#endif
+static int rj54n1_s_power(struct v4l2_subdev *sd, int on)
+{
+ struct i2c_client *client = v4l2_get_subdevdata(sd);
+ struct soc_camera_link *icl = soc_camera_i2c_to_link(client);
+
+ return soc_camera_set_power(client-dev, icl, on);
+}
+
static int rj54n1_s_ctrl(struct v4l2_ctrl *ctrl)
{
struct rj54n1 *rj54n1 = container_of(ctrl-handler, struct rj54n1, hdl);
@@ -1230,6 +1238,7 @@ static struct v4l2_subdev_core_ops rj54n1_subdev_core_ops
= {
.g_register = rj54n1_g_register,
.s_register = rj54n1_s_register,
#endif
+ .s_power= rj54n1_s_power,
};
static int rj54n1_g_mbus_config(struct v4l2_subdev *sd,
diff --git a/drivers/media/video/soc_camera.c b/drivers/media/video/soc_camera.c
index bbd518f..576146e 100644
--- a/drivers/media/video/soc_camera.c
+++ b/drivers/media/video/soc_camera.c
@@ -50,63 +50,72 @@ static LIST_HEAD(hosts);
static LIST_HEAD(devices);
static DEFINE_MUTEX(list_lock); /* Protects the list of hosts */
-static int soc_camera_power_on(struct soc_camera_device *icd
Re: [PATCH] af9035: add several new USB IDs
Hi Antti,
On 04/04/2012 02:59 PM, Antti Palosaari wrote:
On 04.04.2012 14:47, Gianluca Gennari wrote:
Add several new USB IDs extracted from the Windows and Linux drivers
published
by the manufacturers (Terratec and AVerMedia).
+ [AF9035_07CA_0867] = {
+ USB_DEVICE(USB_VID_AVERMEDIA, USB_PID_AVERMEDIA_0867)},
[AF9035_07CA_1867] = {
USB_DEVICE(USB_VID_AVERMEDIA, USB_PID_AVERMEDIA_1867)},
+ [AF9035_07CA_3867] = {
+ USB_DEVICE(USB_VID_AVERMEDIA, USB_PID_AVERMEDIA_3867)},
[AF9035_07CA_A867] = {
USB_DEVICE(USB_VID_AVERMEDIA, USB_PID_AVERMEDIA_A867)},
+ [AF9035_07CA_B867] = {
+ USB_DEVICE(USB_VID_AVERMEDIA, USB_PID_AVERMEDIA_B867)},
It have been common practise to use product names for USB PID
definitions instead of USB ID numbers. I vote to continue that practise.
Also, I am not very sure if it is wise to add new IDs without any
testing. Likely those are just reference design and will work, but
sometimes there is also some changes done for schematic wiring.
Especially for Avermedia, see hacks needed some AF9015 Avermedia
devices. They have put invalid data to eeprom and thus hacks are needed
for overriding tuner IDs etc.
Not to mention, driver supports also dynamic IDs and even device ID is
missing user can load driver using dynamic ID and report it working or
non-working.
Anyone else any thoughts about adding IDs without testing ?
In my experience, it's not always workable out-of-the-box (for the
reasons you mentioned).
IMO it would be better either them adding as long as they're been
tested, or at least to add comments when untested but likely to work.
Br,
David
regards
Antti
--
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] fc0011: Reduce number of retries
Hi,
On 04/03/2012 12:05 PM, Michael Büsch wrote:
Now that i2c transfers are fixed, 3 retries are enough.
Signed-off-by: Michael Bueschm...@bues.ch
---
Index: linux/drivers/media/common/tuners/fc0011.c
===
--- linux.orig/drivers/media/common/tuners/fc0011.c 2012-04-03
08:48:39.0 +0200
+++ linux/drivers/media/common/tuners/fc0011.c 2012-04-03 10:44:07.243418827
+0200
@@ -314,7 +314,7 @@
if (err)
return err;
vco_retries = 0;
- while (!(vco_cal FC11_VCOCAL_OK) vco_retries 6) {
+ while (!(vco_cal FC11_VCOCAL_OK) vco_retries 3) {
Do we need to retry at all?
I2C core layer is responsible to retry is xfer() fails.
If failure is propagated to driver I'd assume:
- I2C is still buggy by not return -EAGAIN on arbitrary error
- I2C xfer failed for real.
Look this piece of code from i2c-core.c:
int i2c_transfer()
{
...
/* Retry automatically on arbitration loss */
orig_jiffies = jiffies;
for (ret = 0, try = 0; try = adap-retries; try++) {
ret = adap-algo-master_xfer(adap, msgs, num);
if (ret != -EAGAIN)
break;
if (time_after(jiffies, orig_jiffies +
adap-timeout))
break;
}
...
}
BR,
David
/* Reset the tuner and try again */
err = fe-callback(priv-i2c, DVB_FRONTEND_COMPONENT_TUNER,
FC0011_FE_CALLBACK_RESET, priv-addr);
--
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] fc0011: Reduce number of retries
On 04/03/2012 01:07 PM, Michael Büsch wrote:
On Tue, 03 Apr 2012 12:58:16 +0300
David Cohendavid.a.co...@linux.intel.com wrote:
- while (!(vco_cal FC11_VCOCAL_OK) vco_retries 6) {
+ while (!(vco_cal FC11_VCOCAL_OK) vco_retries 3) {
Do we need to retry at all?
It is not an i2c retry. It retries the whole device configuration operation
after resetting it.
I shouldn't have mentioned i2c in the commit log, because this really only was
a side effect.
Hm, got it. :)
--
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: RFC: Proposal to change the way pending events are handled
Hi Hans, On Tue, Jun 7, 2011 at 2:29 PM, Hans Verkuil hverk...@xs4all.nl wrote: While working on the control events I realized that the way we handle pending events is rather complicated. What currently happens internally is that you have to allocate a fixed sized list of events. New events are queued on the 'available' list and when they are processed by the application they are queued on the 'free' list. If the 'free' list is empty, then no new events can be queued and you will drop events. Dropping events can be nasty and in the case of control events can cause a control panel to contain stale control values if it missed a value change event. I remember it was a topic I discussed with Sakari. One option is to allocate enough events, but what is 'enough' events? That depends on many factors. And allocating more events than is necessary wastes memory. Cases where events are lost are exception and IMO enough events would be almost always waste of memory. But what might be a better option is this: for each event a filehandle subscribes to there is only one internal v4l2_kevent allocated. This struct is either marked empty (no event was raised) or contains the latest state of this event. When the event is dequeued by the application the struct is marked empty again. So you never get duplicate events, instead, if a 'duplicate' event is raised it will just overwrite the 'old' event and move it to the end of the list of pending events. In other words, the old event is removed and the new event is inserted instead. That's an interesting proposal. Currently it will have impact at least on statistics collection OMAP3ISP driver. It brings to my mind 2 points: - OMAP3ISP triggers one event for each statistic buffers produced. If we avoid events duplication, userapp will miss a statistic buffer. It's possible to bypass this problem, but the OMAP3 ISP statistics' private interface should be updated as well. - To define a standard for statistics collection is something we need to do to avoid new ISP's to always create custom interfaces. The nice thing about this is that for each subscribed event type you will never lose a raised event completely. You may lose intermediate events, but the latest event for that type will always be available. I may have a suggestion. If some event is affected by the number of times it was triggered (like the statistic ones mentioned above), instead of a bool empty flag, it may contain a counter. Then a duplicated event will be raised and will still inform how many intermediate events were lost. After event is dequeued once, the counter could be reset. Regards, David Cohen E.g. supposed you subscribed to a control containing the status of the HDMI hotplug. Connecting an HDMI cable can cause a bounce condition where the HDMI hotplug toggles many times in quick succession. This could currently flood the event queue and you may lose the last event. With the proposed change the last event will always arrive, although the intermediate events will be lost. Comments? Regards, Hans -- 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 -- 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: RFC: Proposal to change the way pending events are handled
Hi Hans, Laurent, On Tue, Jun 7, 2011 at 3:20 PM, Hans Verkuil hverk...@xs4all.nl wrote: On Tuesday, June 07, 2011 13:51:38 David Cohen wrote: Hi Hans, On Tue, Jun 7, 2011 at 2:29 PM, Hans Verkuil hverk...@xs4all.nl wrote: While working on the control events I realized that the way we handle pending events is rather complicated. What currently happens internally is that you have to allocate a fixed sized list of events. New events are queued on the 'available' list and when they are processed by the application they are queued on the 'free' list. If the 'free' list is empty, then no new events can be queued and you will drop events. Dropping events can be nasty and in the case of control events can cause a control panel to contain stale control values if it missed a value change event. I remember it was a topic I discussed with Sakari. One option is to allocate enough events, but what is 'enough' events? That depends on many factors. And allocating more events than is necessary wastes memory. Cases where events are lost are exception and IMO enough events would be almost always waste of memory. The problem with exceptions is that they *do* happen and you need a way to deal with them sensibly. One way of doing that is to ensure that for each subscribed event you can get at least the latest raised event of that type. But what might be a better option is this: for each event a filehandle subscribes to there is only one internal v4l2_kevent allocated. This struct is either marked empty (no event was raised) or contains the latest state of this event. When the event is dequeued by the application the struct is marked empty again. So you never get duplicate events, instead, if a 'duplicate' event is raised it will just overwrite the 'old' event and move it to the end of the list of pending events. In other words, the old event is removed and the new event is inserted instead. That's an interesting proposal. Currently it will have impact at least on statistics collection OMAP3ISP driver. It brings to my mind 2 points: - OMAP3ISP triggers one event for each statistic buffers produced. If we avoid events duplication, userapp will miss a statistic buffer. It's possible to bypass this problem, but the OMAP3 ISP statistics' private interface should be updated as well. Two thoughts: first of all given the current internal event architecture there are no guarantees that the event can even be raised (e.g. if another event flooded the system with events). Secondly, perhaps we should allocate events per event type. So for the statistics collection event would might want to reserve X events. And if the app doesn't read events fast enough, then the oldest event would be lost. Yes, the oldest event will be lost, but the driver stores N buffers where N 1. Currently statistics' drivers depend on event duplication. But it's not a hard dependence. It's possible and easy to change it. BTW, isn't statistics associated with a frame? So if you read too slow, then does it matter that you lose the older statistics? They are related but dequeued independently in userspace. - To define a standard for statistics collection is something we need to do to avoid new ISP's to always create custom interfaces. The nice thing about this is that for each subscribed event type you will never lose a raised event completely. You may lose intermediate events, but the latest event for that type will always be available. I may have a suggestion. If some event is affected by the number of times it was triggered (like the statistic ones mentioned above), instead of a bool empty flag, it may contain a counter. Then a duplicated event will be raised and will still inform how many intermediate events were lost. After event is dequeued once, the counter could be reset. I'm not sure what you mean. If you mean that we can report the number of lost events in struct v4l2_event, then I agree with that. Yes, I mean the number of lost events. But Laurent gave a good point. So I can identify 3 different types regarding to event duplication: 1 - Event doesn't care about duplication. 2 - Event care about duplication but doesn't care about detailed information. 3 - Event care about duplication and detailed information. For 1, to discard all older events is enough. For 2, to discard older events but keep a counter is enough. For 3, it can't be infinite because the driver will have a certain limit to store old data associated to those events, so to define a max number of queued events and discard any older one is enough. All cases should be possible to handle without any loss of information. Regards, Hans Regards, David Cohen E.g. supposed you subscribed to a control containing the status of the HDMI hotplug. Connecting an HDMI cable can cause a bounce condition where the HDMI hotplug toggles many times
Re: RFC: Proposal to change the way pending events are handled
On Tue, Jun 7, 2011 at 3:46 PM, Hans Verkuil hverk...@xs4all.nl wrote: On Tuesday, June 07, 2011 14:30:38 Sakari Ailus wrote: Laurent Pinchart wrote: Hi Hans and David, On Tuesday 07 June 2011 13:51:38 David Cohen wrote: On Tue, Jun 7, 2011 at 2:29 PM, Hans Verkuil hverk...@xs4all.nl wrote: While working on the control events I realized that the way we handle pending events is rather complicated. What currently happens internally is that you have to allocate a fixed sized list of events. New events are queued on the 'available' list and when they are processed by the application they are queued on the 'free' list. If the 'free' list is empty, then no new events can be queued and you will drop events. Dropping events can be nasty and in the case of control events can cause a control panel to contain stale control values if it missed a value change event. I remember it was a topic I discussed with Sakari. One option is to allocate enough events, but what is 'enough' events? That depends on many factors. And allocating more events than is necessary wastes memory. Cases where events are lost are exception and IMO enough events would be almost always waste of memory. But what might be a better option is this: for each event a filehandle subscribes to there is only one internal v4l2_kevent allocated. This struct is either marked empty (no event was raised) or contains the latest state of this event. When the event is dequeued by the application the struct is marked empty again. So you never get duplicate events, instead, if a 'duplicate' event is raised it will just overwrite the 'old' event and move it to the end of the list of pending events. In other words, the old event is removed and the new event is inserted instead. That's an interesting proposal. Currently it will have impact at least on statistics collection OMAP3ISP driver. It brings to my mind 2 points: - OMAP3ISP triggers one event for each statistic buffers produced. If we avoid events duplication, userapp will miss a statistic buffer. It's possible to bypass this problem, but the OMAP3 ISP statistics' private interface should be updated as well. - To define a standard for statistics collection is something we need to do to avoid new ISP's to always create custom interfaces. The nice thing about this is that for each subscribed event type you will never lose a raised event completely. You may lose intermediate events, but the latest event for that type will always be available. I may have a suggestion. If some event is affected by the number of times it was triggered (like the statistic ones mentioned above), instead of a bool empty flag, it may contain a counter. Then a duplicated event will be raised and will still inform how many intermediate events were lost. After event is dequeued once, the counter could be reset. A counter would help mitigate events loss issues, when an application is not only interested in the last event state (like for HDMI hotplug for instance), but also on the intermediate events. This isn't a perfect solution though, applications can still make use of detailed event informations (such as timestamps, and event-specific data) even if they arrive late. It really depends on the event type. I agree with Laurent. When the interface was originally defined, the assumption was that any kind of event loss would be a major nuisance and should ideally never happen. It's a good question what is best when there's not enough room for new events. Definitely your proposal does have its advantages, but also causes loss of information such as timestamps much more easily in cases such as the OMAP 3 ISP driver where many events are generated per frame, usually one per type. On the other hand, the importance of timestamps generally decreases when as they get older. I'm uncertain whether such a change would actually break something, at least I can't say it wouldn't right now. I wonder if it would be too complicated to pre-allocate n events per event type, n being be a small natural number. This might be wasteful, however. I think this is actually the best approach. It is even possible to let the application select 'N' when subscribing an event. This ensures that you never completely miss an event: e.g. if X events of type T are raised, then you will at least get min(N, X) events of that type. It's much nicer that way since it gives you useful guarantees that you don't have today. And the allocation can be done when subscribing events instead of having to guess some global maximum for the total number of events. I agree with this proposal. I won't affect OMAP3ISP statistic drivers either. Br, David The current approach is definitely problematic since there are no guarantees whatsoever. Regards, Hans Or allow at least one event per event type
Re: [PATCH 0/3] V4L2 API for flash devices and the adp1653 flash controller driver
Hi Laurent, On Thu, May 19, 2011 at 3:14 PM, Laurent Pinchart laurent.pinch...@ideasonboard.com wrote: Hi Sakari, On Thursday 19 May 2011 12:41:14 Sakari Ailus wrote: Hi, This patchset implements RFC v4 of V4L2 API for flash devices [1], with minor modifications, and adds the adp1653 flash controller driver. As already answered in private: Acked-by: Sakari Ailus sakari.ai...@maxwell.research.nokia.com Sorry if I missed something, but shouldn't be your ack here? :) The patches are from Sakari already. Br, David This patchset depends on the bitmask controls patch [4]. Changes since v2 [3] of the RFC patchset: - Improved flash control documentation in general. - Faults may change the LED mode to none. This is now documented. - adp1653 is returned to sane after faults are detected. - Proper error handling for adp1653_get_fault() in adp1653_strobe(). - Remove useless function: adp1653_registered() and the corresponding callback. Controls are now initialised in adp1653_probe(). - Improved fault handling in adp1653_init_device(). Changes since v1 [2] of the RFC patchset: - Faults on the flash LED are allowed to make the LED unusable before the faults are read. This is implemented in the adp1653 driver. - Intensities are using standard units; mA for flash / torch and uA for the indicator. Thanks to those who have given their feedback so far in the process! [1] http://www.spinics.net/lists/linux-media/msg32030.html [2] http://www.spinics.net/lists/linux-media/msg32396.html [3] http://www.spinics.net/lists/linux-media/msg32436.html [4] http://www.spinics.net/lists/linux-media/msg31001.html -- 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: OMAP3 ISP deadlocks on my new arm
On Mon, Apr 18, 2011 at 1:43 PM, Bastian Hecht hec...@googlemail.com wrote:
2011/4/16 David Cohen daco...@gmail.com:
Hi Bastian,
On Thu, Apr 14, 2011 at 1:36 PM, Bastian Hecht hec...@googlemail.com wrote:
Yeah!
S... when I initialized the the camera (loading a 108 bytes
register listing) I just let run the camera and sent images. So I
first realized a counter overflow if (count++ 10) after a few
seconds. But this seemed to be handled correctly (ignore and delete
HS_VS_IRQ flag) while after 2 or more yavta calls it made the driver
hang.
I modified my register listing so that the chip gets booted silently.
In
static struct v4l2_subdev_video_ops framix_subdev_video_ops = {
.s_stream = framix_s_stream, ===
};
I correctly check the stream status now and enable/disable the camera
signals.
I am unsure whether the isp should be able to handle an ongoing data
stream independently of ISP_PIPELINE_STREAM_STOPPED.
streamoff should finish synchronously with last ongoing data. So, it
should have no ongoing data afterwards. Was that your question?
I formulated my reply a bit strange. I meant that that the ongoing
datastream from my camera module (even when the isp-stack is in
stream_stopped state) produces my problem. The question was if it
should be allowed for the camera to send data all time long or only
when it is told to do so by s_stream.
I may assume you are mentioning a pipeline which includes camera
sensor + ISP. In this case there should be no data.
Regards,
David
best regards,
Bastian Hecht
Br,
David Cohen
If you decide it should, I will gladly help you find out more, just
ask. It worked on an OMAP3530 before, but I do not know if it is the
hardware or isp software that changed. Unfortunately I can't get an
3530 anymore (I trashed mine...).
You helped me so much! Big thanks.
Bastian Hecht
2011/4/14 Laurent Pinchart laurent.pinch...@ideasonboard.com:
Hi Bastian,
On Thursday 14 April 2011 10:33:12 Bastian Hecht wrote:
2011/4/13 Sakari Ailus sakari.ai...@maxwell.research.nokia.com:
Bastian Hecht wrote:
Hello people,
Hi Bastian,
I'm cc'ing Laurent.
I switched to the new DM3730 from IGEP and while it's supposed to be
(almost) the same as the 3530 Version the isp deadlocks
deterministically after I start capturing the second time with yavta.
Does the capture work the first time w/o issues?
Yes, I can always run yavta once capturing 4 frames (3 skipped, last
saved). It usually deadlocks when running yavta the second time but I had
1 successful 2nd try (out of 20 maybe).
All extra locking debug output is enabled in the kernel .config.
I'm not fully certain on what this exactly is that you have below, but
it looks like your system is staying in interrupt context all the time.
My guess is that the ISP is producing interrupts that the driver is not
handling properly, causing the interrupt handler to be called again
immediately.
Nice! OK, I'd like to fully understand the panic output, maybe you can
help there:
After
[ 376.016906] [c02e3dc4] (_raw_spin_unlock_irqrestore+0x40/0x44)
from [bf01f678] (omap3isp_video_queue_streamon+0x80/0x90
the IRQs get enabled again. Immediately our offending irq wants to get
served but noone is clearing it.
At some time, the timer interrupt triggers the watchdog for a kernel
panic.
So the last exception block is the process context that is currently
active. But why are there 2 irq routines displayed? Is the middle one the
hardware handling that causes a software irq to be triggered (upper
block)?
So my next step could be to find the unhandled irq number?
If the problem is caused by an interrupt storm, the following patch will
make
your system responsive again after a couple of seconds (but will kill the
ISP
driver :-)). If it doesn't, the problem is likely caused by something else.
diff --git a/drivers/media/video/omap3isp/isp.c
b/drivers/media/video/omap3isp/isp.c
index de2dec5..6497300 100644
--- a/drivers/media/video/omap3isp/isp.c
+++ b/drivers/media/video/omap3isp/isp.c
@@ -462,6 +464,7 @@ static irqreturn_t isp_isr(int irq, void *_isp)
IRQ0STATUS_CCDC_VD0_IRQ |
IRQ0STATUS_CCDC_VD1_IRQ |
IRQ0STATUS_HS_VS_IRQ;
+ static unsigned int count = 0;
struct isp_device *isp = _isp;
u32 irqstatus;
int ret;
@@ -469,6 +472,11 @@ static irqreturn_t isp_isr(int irq, void *_isp)
irqstatus = isp_reg_readl(isp, OMAP3_ISP_IOMEM_MAIN,
ISP_IRQ0STATUS);
isp_reg_writel(isp, irqstatus, OMAP3_ISP_IOMEM_MAIN,
ISP_IRQ0STATUS);
+ if (count++ 10) {
+ isp_disable_interrupts(isp);
+ return IRQ_HANDLED;
+ }
+
isp_isr_sbl(isp);
if (irqstatus IRQ0STATUS_CSIA_IRQ) {
Do you have the same sensor working on OMAP
Re: OMAP3 ISP deadlocks on my new arm
Hi Bastian,
On Thu, Apr 14, 2011 at 1:36 PM, Bastian Hecht hec...@googlemail.com wrote:
Yeah!
S... when I initialized the the camera (loading a 108 bytes
register listing) I just let run the camera and sent images. So I
first realized a counter overflow if (count++ 10) after a few
seconds. But this seemed to be handled correctly (ignore and delete
HS_VS_IRQ flag) while after 2 or more yavta calls it made the driver
hang.
I modified my register listing so that the chip gets booted silently.
In
static struct v4l2_subdev_video_ops framix_subdev_video_ops = {
.s_stream = framix_s_stream, ===
};
I correctly check the stream status now and enable/disable the camera signals.
I am unsure whether the isp should be able to handle an ongoing data
stream independently of ISP_PIPELINE_STREAM_STOPPED.
streamoff should finish synchronously with last ongoing data. So, it
should have no ongoing data afterwards. Was that your question?
Br,
David Cohen
If you decide it should, I will gladly help you find out more, just
ask. It worked on an OMAP3530 before, but I do not know if it is the
hardware or isp software that changed. Unfortunately I can't get an
3530 anymore (I trashed mine...).
You helped me so much! Big thanks.
Bastian Hecht
2011/4/14 Laurent Pinchart laurent.pinch...@ideasonboard.com:
Hi Bastian,
On Thursday 14 April 2011 10:33:12 Bastian Hecht wrote:
2011/4/13 Sakari Ailus sakari.ai...@maxwell.research.nokia.com:
Bastian Hecht wrote:
Hello people,
Hi Bastian,
I'm cc'ing Laurent.
I switched to the new DM3730 from IGEP and while it's supposed to be
(almost) the same as the 3530 Version the isp deadlocks
deterministically after I start capturing the second time with yavta.
Does the capture work the first time w/o issues?
Yes, I can always run yavta once capturing 4 frames (3 skipped, last
saved). It usually deadlocks when running yavta the second time but I had
1 successful 2nd try (out of 20 maybe).
All extra locking debug output is enabled in the kernel .config.
I'm not fully certain on what this exactly is that you have below, but
it looks like your system is staying in interrupt context all the time.
My guess is that the ISP is producing interrupts that the driver is not
handling properly, causing the interrupt handler to be called again
immediately.
Nice! OK, I'd like to fully understand the panic output, maybe you can
help there:
After
[ 376.016906] [c02e3dc4] (_raw_spin_unlock_irqrestore+0x40/0x44)
from [bf01f678] (omap3isp_video_queue_streamon+0x80/0x90
the IRQs get enabled again. Immediately our offending irq wants to get
served but noone is clearing it.
At some time, the timer interrupt triggers the watchdog for a kernel panic.
So the last exception block is the process context that is currently
active. But why are there 2 irq routines displayed? Is the middle one the
hardware handling that causes a software irq to be triggered (upper
block)?
So my next step could be to find the unhandled irq number?
If the problem is caused by an interrupt storm, the following patch will make
your system responsive again after a couple of seconds (but will kill the ISP
driver :-)). If it doesn't, the problem is likely caused by something else.
diff --git a/drivers/media/video/omap3isp/isp.c
b/drivers/media/video/omap3isp/isp.c
index de2dec5..6497300 100644
--- a/drivers/media/video/omap3isp/isp.c
+++ b/drivers/media/video/omap3isp/isp.c
@@ -462,6 +464,7 @@ static irqreturn_t isp_isr(int irq, void *_isp)
IRQ0STATUS_CCDC_VD0_IRQ |
IRQ0STATUS_CCDC_VD1_IRQ |
IRQ0STATUS_HS_VS_IRQ;
+ static unsigned int count = 0;
struct isp_device *isp = _isp;
u32 irqstatus;
int ret;
@@ -469,6 +472,11 @@ static irqreturn_t isp_isr(int irq, void *_isp)
irqstatus = isp_reg_readl(isp, OMAP3_ISP_IOMEM_MAIN, ISP_IRQ0STATUS);
isp_reg_writel(isp, irqstatus, OMAP3_ISP_IOMEM_MAIN, ISP_IRQ0STATUS);
+ if (count++ 10) {
+ isp_disable_interrupts(isp);
+ return IRQ_HANDLED;
+ }
+
isp_isr_sbl(isp);
if (irqstatus IRQ0STATUS_CSIA_IRQ) {
Do you have the same sensor working on OMAP 3530?
I never had this problem on an OMAP 3530, although I better test it
again with the current setup. I try to get my hands on an 3530 today.
I am unsure if this is an ISP thing or a problem in the
interrupthandling software.
This has probably something to do with the ISP driver. :-)
The first block is the watchdog that detects the deadlock. The last
block is in the isp-code but how can it hang when trying to UNlock a
spinlock? I am unsure about the 2nd block.
The assembler code of __irq_svc is located in
arch/arm/kernel/entry-armv.S Maybe I should try on
linux
Re: [RFC] V4L2 API for flash devices
On Mon, Mar 28, 2011 at 3:55 PM, Sakari Ailus sakari.ai...@maxwell.research.nokia.com wrote: Hi, Hi Sakari, [snip] This is a bitmask containing the fault information for the flash. This assumes the proposed V4L2 bit mask controls [5]; otherwise this would likely need to be a set of controls. #define V4L2_FLASH_FAULT_OVER_VOLTAGE 0x0001 #define V4L2_FLASH_FAULT_TIMEOUT 0x0002 #define V4L2_FLASH_FAULT_OVER_TEMPERATURE 0x0004 #define V4L2_FLASH_FAULT_SHORT_CIRCUIT 0x0008 Sorry for bringing this a bit late. As we already talked directly, IMO (1 0), (1 1), ... could have a better readability to expose how you want to define an expand these macros. Br, David Cohen -- 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: [RFC] V4L2 API for flash devices
On Wed, Mar 30, 2011 at 5:18 PM, Sakari Ailus
sakari.ai...@maxwell.research.nokia.com wrote:
Laurent Pinchart wrote:
Hi Sakari,
Heippa,
Hello,
My 2 cents below.
On Wednesday 30 March 2011 14:44:25 Sakari Ailus wrote:
Laurent Pinchart wrote:
On Tuesday 29 March 2011 11:35:19 Sakari Ailus wrote:
Hans Verkuil wrote:
On Monday, March 28, 2011 14:55:40 Sakari Ailus wrote:
[snip]
V4L2_CID_FLASH_TIMEOUT (integer; LED)
The flash controller provides timeout functionality to shut down the
led in case the host fails to do that. For hardware strobe, this is
the maximum amount of time the flash should stay on, and the purpose
of the setting is to prevent the LED from catching fire.
For software strobe, the setting may be used to limit the length of
the strobe in case a driver does not implement it itself. The
granularity of the timeout in [1, 2, 3] is very coarse. However, the
length of a driver-implemented LED strobe shutoff is very dependent
on host. Possibly V4L2_CID_FLASH_DURATION should be added, and
V4L2_CID_FLASH_TIMEOUT would be read-only so that the user would be
able to obtain the actual hardware implemented safety timeout.
Likely a standard unit such as ms or 盜 should be used.
It seems to me that this control should always be read-only. A setting
like this is very much hardware specific and you don't want an attacker
changing the timeout to the max value that might cause a LED catching
fire.
I'm not sure about that.
The driver already must take care of protecting the hardware in my
opinion. Besides, at least one control is required to select the
duration for the flash if there's no hardware synchronisation.
What about this:
V4L2_CID_FLASH_TIMEOUT
Hardware timeout, read-only. Programmed to the maximum value allowed by
the hardware for the external strobe, greater or equal to
V4L2_CID_FLASH_DURATION for software strobe.
V4L2_CID_FLASH_DURATION
Software implemented timeout when V4L2_CID_FLASH_STROBE_MODE ==
V4L2_FLASH_STROBE_MODE_SOFTWARE.
Why would we need two controls here ? My understanding is that the
maximum strobe duration length can be limited by
- the flash controller itself
- platform-specific constraints to avoid over-heating the flash
The platform-specific constraints come from board code, and the flash
driver needs to ensure that the flash is never strobed for a duration
longer than the limit. This requires implementing a software timer if
the hardware has no timeout control, and programming the hardware with
the correct timeout value otherwise. The limit can be queried with
QUERYCTRL on the duration control.
That's true.
The alternative would be software timeout since the hardware timeout is
rather coarse. Its intention is to protect the hardware from catching
fire mostly.
A software timeout can always be implemented in the driver in addition to the
hardware timeout. I think this should be transparent for applications.
But as I commented in the other e-mail, there likely isn't a need to be
able to control this very precisely. The user just shuts down the flash
whenever (s)he no longer needs it rather than knows beforehand how long
it needs to stay on.
What about hardware that needs to be pre-programmed with a duration ?
Same control?
I wonder if I could say we agree to have one timeout control which is
used to control the hardware timeout directly, or to implement a timeout
in software? :-)
Correct if I'm wrong, but I guess we might be talking about 2 kind of timeouts:
- One for the duration itself
- Another one to act like watchdog in addition to the hw timeout
IMO they should be different controls. We could even specify on the
control name when it's a watchdog case to make it more clear.
I have to say I'm not entirely sure the duration control is required.
The timeout could be writable for software strobe in the case drivers do
not implement software timeout. The granularity isn't _that_ much
anyway. Also, a timeout fault should be produced whenever the duration
would expire.
Perhaps it would be best to just leave that out for now.
V4L2_CID_FLASH_LED_MODE (menu; LED)
enum v4l2_flash_led_mode {
V4L2_FLASH_LED_MODE_FLASH = 1,
V4L2_FLASH_LED_MODE_TORCH,
torch mode can also be used for video, should we rename TORCH to
something more generic ? Maybe a manual mode ?
The controllers recognise a torch mode and I think it describes the
functionality quite well. Some appear to make a difference between torch
and video light --- but I can't imagine a purpose in which this could be
useful.
Torch mode is indeed a common name, but it sounds a bit specific to me.
Torch suggests it can be used over extended periods of time, unlike
manual which doesn't really say much. I'd keep it torch since what it
suggests is that it can stay on for long. No references outside the
flash controller itself.
I'd keep with torch also as it seems to be more clear.
Regards,
David
--
Sakari
Re: [PATCH v2 2/3] omap3: change ISP's IOMMU da_start address
On Wed, Mar 9, 2011 at 9:43 AM, Sakari Ailus
sakari.ai...@maxwell.research.nokia.com wrote:
David Cohen wrote:
ISP doesn't consider 0x0 as a valid address, so it should explicitly
exclude first page from allowed 'da' range.
Signed-off-by: David Cohen daco...@gmail.com
---
arch/arm/mach-omap2/omap-iommu.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/arm/mach-omap2/omap-iommu.c
b/arch/arm/mach-omap2/omap-iommu.c
index 3fc5dc7..3bea489 100644
--- a/arch/arm/mach-omap2/omap-iommu.c
+++ b/arch/arm/mach-omap2/omap-iommu.c
@@ -33,7 +33,7 @@ static struct iommu_device omap3_devices[] = {
.name = isp,
.nr_tlb_entries = 8,
.clk_name = cam_ick,
- .da_start = 0x0,
+ .da_start = 0x1000,
.da_end = 0xF000,
},
},
Hi David!
Hi Sakari,
Thanks for the patch.
And thanks for the comments. :)
My question is once again: is this necessary? My understanding is that
the IOMMU allows mapping the NULL address if the user wishes to map it
explicitly. da_end specifies the real hardware limit for the mapped top
address, da_start should do the same for bottom.
Hm. da_start/da_end in this case belong to OMAP3 IOMMU ISP VM. It
means they're related to OMAP3 ISP only. But they do not reflect the
hw limit, as the range limit is anything which fits in u32. They say
what's the range OMAP3 ISP is expecting to have mapped. And the answer
to this question is the first page is not expected. Then, why say the
opposite in da_start?
I think that the IOMMU users should be either able to rely that they get
no NULL allocated automatically for them. Do we want or not want it to
be part of the API? I don't think the ISP driver is a special case of
all the possible drivers using the IOMMU.
My understanding after this discussion is address 0x0 should be
allowed (what is done by patch 3/3 of this set). But as a safer
choice, it won't be returned without explicitly request (what is done
in path 1/3). Because of that, I'm OK in drop this patch 2/3. But then
there's one question which is the motivation for this change:
If the current OMAP3 ISP driver doesn't want the first page, (1)
should we pass a generic information and expect IOVMM driver to
correct it to ISP need or (2) should we pass the information which
reflects the real ISP driver's need?
My understanding is (2). But I'm fine with (1) as it will lead to the
same result.
On the other hand, probably there will be an API change at some point
for the IOMMU since as far as I remember, there are somewhat
established APIs for IOMMUs in existence.
At some point we need to find a standard for many IOMMU drivers. But
for now we need to stick with what we have. :)
Regards,
David Cohen
--
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 v3 0/2] omap: iovmm: Fix IOVMM check for fixed 'da'
Hi, Previous patch 2/3 was dropped in this new version. Patch 1 was updated according to a comment it got. --- IOVMM driver checks input 'da == 0' when mapping address to determine whether user wants fixed 'da' or not. At the same time, it doesn't disallow address 0x0 to be used, what creates an ambiguous situation. This patch set moves fixed 'da' check to the input flags. Br, David Cohen --- David Cohen (1): omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED flag Michael Jones (1): omap: iovmm: disallow mapping NULL address when IOVMF_DA_ANON is set arch/arm/plat-omap/include/plat/iovmm.h |2 -- arch/arm/plat-omap/iovmm.c | 27 --- 2 files changed, 12 insertions(+), 17 deletions(-) -- 1.7.4.1 -- 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 v3 1/2] omap: iovmm: disallow mapping NULL address when IOVMF_DA_ANON is set
From: Michael Jones michael.jo...@matrix-vision.de
commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb allowed mapping the NULL
address if da_start==0, which would then not get unmapped. Disallow
this again if IOVMF_DA_ANON is set. And spell variable 'alignment'
correctly.
Signed-off-by: Michael Jones michael.jo...@matrix-vision.de
---
arch/arm/plat-omap/iovmm.c | 13 +++--
1 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
index 6dc1296..ea7eab9 100644
--- a/arch/arm/plat-omap/iovmm.c
+++ b/arch/arm/plat-omap/iovmm.c
@@ -271,20 +271,21 @@ static struct iovm_struct *alloc_iovm_area(struct iommu
*obj, u32 da,
size_t bytes, u32 flags)
{
struct iovm_struct *new, *tmp;
- u32 start, prev_end, alignement;
+ u32 start, prev_end, alignment;
if (!obj || !bytes)
return ERR_PTR(-EINVAL);
start = da;
- alignement = PAGE_SIZE;
+ alignment = PAGE_SIZE;
if (flags IOVMF_DA_ANON) {
- start = obj-da_start;
+ /* Don't map address 0 */
+ start = obj-da_start ? obj-da_start : alignment;
if (flags IOVMF_LINEAR)
- alignement = iopgsz_max(bytes);
- start = roundup(start, alignement);
+ alignment = iopgsz_max(bytes);
+ start = roundup(start, alignment);
} else if (start obj-da_start || start obj-da_end ||
obj-da_end - start bytes) {
return ERR_PTR(-EINVAL);
@@ -304,7 +305,7 @@ static struct iovm_struct *alloc_iovm_area(struct iommu
*obj, u32 da,
goto found;
if (tmp-da_end = start flags IOVMF_DA_ANON)
- start = roundup(tmp-da_end + 1, alignement);
+ start = roundup(tmp-da_end + 1, alignment);
prev_end = tmp-da_end;
}
--
1.7.4.1
--
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 v3 2/2] omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED flag
Currently IOVMM driver sets IOVMF_DA_FIXED/IOVMF_DA_ANON flags according
to input 'da' address when mapping memory:
da == 0: IOVMF_DA_ANON
da != 0: IOVMF_DA_FIXED
It prevents IOMMU to map first page with fixed 'da'. To avoid such
issue, IOVMM will not automatically set IOVMF_DA_FIXED. It should now
come from the user throught 'flags' parameter when mapping memory.
As IOVMF_DA_ANON and IOVMF_DA_FIXED are mutually exclusive, IOVMF_DA_ANON
can be removed. The driver will now check internally if IOVMF_DA_FIXED
is set or not.
Signed-off-by: David Cohen daco...@gmail.com
---
arch/arm/plat-omap/include/plat/iovmm.h |2 --
arch/arm/plat-omap/iovmm.c | 14 +-
2 files changed, 5 insertions(+), 11 deletions(-)
diff --git a/arch/arm/plat-omap/include/plat/iovmm.h
b/arch/arm/plat-omap/include/plat/iovmm.h
index bdc7ce5..32a2f6c 100644
--- a/arch/arm/plat-omap/include/plat/iovmm.h
+++ b/arch/arm/plat-omap/include/plat/iovmm.h
@@ -71,8 +71,6 @@ struct iovm_struct {
#define IOVMF_LINEAR_MASK (3 (2 + IOVMF_SW_SHIFT))
#define IOVMF_DA_FIXED (1 (4 + IOVMF_SW_SHIFT))
-#define IOVMF_DA_ANON (2 (4 + IOVMF_SW_SHIFT))
-#define IOVMF_DA_MASK (3 (4 + IOVMF_SW_SHIFT))
extern struct iovm_struct *find_iovm_area(struct iommu *obj, u32 da);
diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
index ea7eab9..51ef43e 100644
--- a/arch/arm/plat-omap/iovmm.c
+++ b/arch/arm/plat-omap/iovmm.c
@@ -279,7 +279,7 @@ static struct iovm_struct *alloc_iovm_area(struct iommu
*obj, u32 da,
start = da;
alignment = PAGE_SIZE;
- if (flags IOVMF_DA_ANON) {
+ if (~flags IOVMF_DA_FIXED) {
/* Don't map address 0 */
start = obj-da_start ? obj-da_start : alignment;
@@ -304,7 +304,7 @@ static struct iovm_struct *alloc_iovm_area(struct iommu
*obj, u32 da,
if (tmp-da_start start (tmp-da_start - start) = bytes)
goto found;
- if (tmp-da_end = start flags IOVMF_DA_ANON)
+ if (tmp-da_end = start ~flags IOVMF_DA_FIXED)
start = roundup(tmp-da_end + 1, alignment);
prev_end = tmp-da_end;
@@ -651,7 +651,6 @@ u32 iommu_vmap(struct iommu *obj, u32 da, const struct
sg_table *sgt,
flags = IOVMF_HW_MASK;
flags |= IOVMF_DISCONT;
flags |= IOVMF_MMIO;
- flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
da = __iommu_vmap(obj, da, sgt, va, bytes, flags);
if (IS_ERR_VALUE(da))
@@ -691,7 +690,7 @@ EXPORT_SYMBOL_GPL(iommu_vunmap);
* @flags: iovma and page property
*
* Allocate @bytes linearly and creates 1-n-1 mapping and returns
- * @da again, which might be adjusted if 'IOVMF_DA_ANON' is set.
+ * @da again, which might be adjusted if 'IOVMF_DA_FIXED' is not set.
*/
u32 iommu_vmalloc(struct iommu *obj, u32 da, size_t bytes, u32 flags)
{
@@ -710,7 +709,6 @@ u32 iommu_vmalloc(struct iommu *obj, u32 da, size_t bytes,
u32 flags)
flags = IOVMF_HW_MASK;
flags |= IOVMF_DISCONT;
flags |= IOVMF_ALLOC;
- flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
sgt = sgtable_alloc(bytes, flags, da, 0);
if (IS_ERR(sgt)) {
@@ -781,7 +779,7 @@ static u32 __iommu_kmap(struct iommu *obj, u32 da, u32 pa,
void *va,
* @flags: iovma and page property
*
* Creates 1-1-1 mapping and returns @da again, which can be
- * adjusted if 'IOVMF_DA_ANON' is set.
+ * adjusted if 'IOVMF_DA_FIXED' is not set.
*/
u32 iommu_kmap(struct iommu *obj, u32 da, u32 pa, size_t bytes,
u32 flags)
@@ -800,7 +798,6 @@ u32 iommu_kmap(struct iommu *obj, u32 da, u32 pa, size_t
bytes,
flags = IOVMF_HW_MASK;
flags |= IOVMF_LINEAR;
flags |= IOVMF_MMIO;
- flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
da = __iommu_kmap(obj, da, pa, va, bytes, flags);
if (IS_ERR_VALUE(da))
@@ -839,7 +836,7 @@ EXPORT_SYMBOL_GPL(iommu_kunmap);
* @flags: iovma and page property
*
* Allocate @bytes linearly and creates 1-1-1 mapping and returns
- * @da again, which might be adjusted if 'IOVMF_DA_ANON' is set.
+ * @da again, which might be adjusted if 'IOVMF_DA_FIXED' is not set.
*/
u32 iommu_kmalloc(struct iommu *obj, u32 da, size_t bytes, u32 flags)
{
@@ -859,7 +856,6 @@ u32 iommu_kmalloc(struct iommu *obj, u32 da, size_t bytes,
u32 flags)
flags = IOVMF_HW_MASK;
flags |= IOVMF_LINEAR;
flags |= IOVMF_ALLOC;
- flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
da = __iommu_kmap(obj, da, pa, va, bytes, flags);
if (IS_ERR_VALUE(da))
--
1.7.4.1
--
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] omap: iommu: disallow mapping NULL address
Hi Fernando, On Tue, Mar 8, 2011 at 11:13 AM, Sakari Ailus sakari.ai...@maxwell.research.nokia.com wrote: Guzman Lugo, Fernando wrote: On Mon, Mar 7, 2011 at 1:19 PM, David Cohen daco...@gmail.com wrote: On Mon, Mar 7, 2011 at 9:17 PM, Guzman Lugo, Fernando fernando.l...@ti.com wrote: On Mon, Mar 7, 2011 at 7:10 AM, Michael Jones michael.jo...@matrix-vision.de wrote: From e7dbe4c4b64eb114f9b0804d6af3a3ca0e78acc8 Mon Sep 17 00:00:00 2001 From: Michael Jones michael.jo...@matrix-vision.de Date: Mon, 7 Mar 2011 13:36:15 +0100 Subject: [PATCH] omap: iommu: disallow mapping NULL address commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb allowed mapping the NULL address if da_start==0. Force da_start to exclude the first page. what about devices that uses page 0? ipu after reset always starts from 0x how could we map that address?? from 0x0? The driver sees da == 0 as error. May I ask you why do you want it? unlike DSP that you can load a register with the addres the DSP will boot, IPU core always starts from address 0x, so if you take IPU out of reset it will try to access address 0x0 if not map it, there will be a mmu fault. I think the driver for IPU (what is it, btw.?) must map the NULL address explicitly. It cannot rely on automatic allocation of the NULL address by the iommu even if it was the first allocation. That's an interesting question. My first thought was it's not automatic allocation, because it seems you know the specific 'da' IPU needs. But then, looking into the driver's API, the automatic allocation is defined whether the argument da == 0 (automatic allocation) or da != 0 (fixed da). So, by default, the IOMMU driver does not see da == 0 as valid address for fixed da. Then, why only automatic allocation should use such address? My second point is: if you're using automatic allocation, you *cannot* rely on specific da to be used, as it would be up to IOMMU driver to choose. So, doesn't matter the option, your driver seems to be wrong, unless I'm missing something. If you were using fixed da passing da = 0, you were just being lucky that it was the first request and automatic allocation returned da == 0. IMO either first page is not allowed at all or OMAP's IOMMU API should change the way it checks if it's fixed da or not. Kind regards, David -- Sakari Ailus sakari.ai...@maxwell.research.nokia.com -- 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 0/3] omap: iovmm: Fix IOVMM check for fixed 'da'
IOVMM driver checks input 'da == 0' when mapping address to determine whether user wants fixed 'da' or not. At the same time, it doesn't disallow address 0x0 to be used, what creates an ambiguous situation. This patch set moves fixed 'da' check to the input flags. It also fixes da_start value for ISP IOMMU instance. David Cohen (2): omap3: change ISP's IOMMU da_start address omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED/IOVMF_DA_ANON flags Michael Jones (1): omap: iovmm: disallow mapping NULL address arch/arm/mach-omap2/omap-iommu.c |2 +- arch/arm/plat-omap/iovmm.c | 28 ++-- 2 files changed, 19 insertions(+), 11 deletions(-) -- 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 2/3] omap3: change ISP's IOMMU da_start address
ISP doesn't consider 0x0 as a valid address, so it should explicitly
exclude first page from allowed 'da' range.
Signed-off-by: David Cohen daco...@gmail.com
---
arch/arm/mach-omap2/omap-iommu.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/arm/mach-omap2/omap-iommu.c b/arch/arm/mach-omap2/omap-iommu.c
index 3fc5dc7..3bea489 100644
--- a/arch/arm/mach-omap2/omap-iommu.c
+++ b/arch/arm/mach-omap2/omap-iommu.c
@@ -33,7 +33,7 @@ static struct iommu_device omap3_devices[] = {
.name = isp,
.nr_tlb_entries = 8,
.clk_name = cam_ick,
- .da_start = 0x0,
+ .da_start = 0x1000,
.da_end = 0xF000,
},
},
--
1.7.0.4
--
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 1/3] omap: iovmm: disallow mapping NULL address
From: Michael Jones michael.jo...@matrix-vision.de
commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb allowed mapping
the NULL address if da_start==0, which would then not get unmapped.
Disallow this again. And spell variable 'alignment' correctly.
Signed-off-by: Michael Jones michael.jo...@matrix-vision.de
---
arch/arm/plat-omap/iovmm.c | 16 ++--
1 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
index 6dc1296..11c9b76 100644
--- a/arch/arm/plat-omap/iovmm.c
+++ b/arch/arm/plat-omap/iovmm.c
@@ -271,20 +271,24 @@ static struct iovm_struct *alloc_iovm_area(struct iommu
*obj, u32 da,
size_t bytes, u32 flags)
{
struct iovm_struct *new, *tmp;
- u32 start, prev_end, alignement;
+ u32 start, prev_end, alignment;
if (!obj || !bytes)
return ERR_PTR(-EINVAL);
start = da;
- alignement = PAGE_SIZE;
+ alignment = PAGE_SIZE;
if (flags IOVMF_DA_ANON) {
- start = obj-da_start;
+ /* Don't map address 0 */
+ if (obj-da_start)
+ start = obj-da_start;
+ else
+ start = obj-da_start + alignment;
if (flags IOVMF_LINEAR)
- alignement = iopgsz_max(bytes);
- start = roundup(start, alignement);
+ alignment = iopgsz_max(bytes);
+ start = roundup(start, alignment);
} else if (start obj-da_start || start obj-da_end ||
obj-da_end - start bytes) {
return ERR_PTR(-EINVAL);
@@ -304,7 +308,7 @@ static struct iovm_struct *alloc_iovm_area(struct iommu
*obj, u32 da,
goto found;
if (tmp-da_end = start flags IOVMF_DA_ANON)
- start = roundup(tmp-da_end + 1, alignement);
+ start = roundup(tmp-da_end + 1, alignment);
prev_end = tmp-da_end;
}
--
1.7.0.4
--
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 3/3] omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED/IOVMF_DA_ANON flags
Currently IOVMM driver sets IOVMF_DA_FIXED/IOVMF_DA_ANON flags according
to input 'da' address when mapping memory:
da == 0: IOVMF_DA_ANON
da != 0: IOVMF_DA_FIXED
It prevents IOMMU to map first page with fixed 'da'. To avoid such
issue, IOVMM will not automatically set IOVMF_DA_FIXED. It should now
come from the user. IOVMF_DA_ANON will be automatically set if
IOVMF_DA_FIXED isn't set.
Signed-off-by: David Cohen daco...@gmail.com
---
arch/arm/plat-omap/iovmm.c | 12
1 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
index 11c9b76..dde9cb0 100644
--- a/arch/arm/plat-omap/iovmm.c
+++ b/arch/arm/plat-omap/iovmm.c
@@ -654,7 +654,8 @@ u32 iommu_vmap(struct iommu *obj, u32 da, const struct
sg_table *sgt,
flags = IOVMF_HW_MASK;
flags |= IOVMF_DISCONT;
flags |= IOVMF_MMIO;
- flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
+ if (~flags IOVMF_DA_FIXED)
+ flags |= IOVMF_DA_ANON;
da = __iommu_vmap(obj, da, sgt, va, bytes, flags);
if (IS_ERR_VALUE(da))
@@ -713,7 +714,8 @@ u32 iommu_vmalloc(struct iommu *obj, u32 da, size_t bytes,
u32 flags)
flags = IOVMF_HW_MASK;
flags |= IOVMF_DISCONT;
flags |= IOVMF_ALLOC;
- flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
+ if (~flags IOVMF_DA_FIXED)
+ flags |= IOVMF_DA_ANON;
sgt = sgtable_alloc(bytes, flags, da, 0);
if (IS_ERR(sgt)) {
@@ -803,7 +805,8 @@ u32 iommu_kmap(struct iommu *obj, u32 da, u32 pa, size_t
bytes,
flags = IOVMF_HW_MASK;
flags |= IOVMF_LINEAR;
flags |= IOVMF_MMIO;
- flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
+ if (~flags IOVMF_DA_FIXED)
+ flags |= IOVMF_DA_ANON;
da = __iommu_kmap(obj, da, pa, va, bytes, flags);
if (IS_ERR_VALUE(da))
@@ -862,7 +865,8 @@ u32 iommu_kmalloc(struct iommu *obj, u32 da, size_t bytes,
u32 flags)
flags = IOVMF_HW_MASK;
flags |= IOVMF_LINEAR;
flags |= IOVMF_ALLOC;
- flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
+ if (~flags IOVMF_DA_FIXED)
+ flags |= IOVMF_DA_ANON;
da = __iommu_kmap(obj, da, pa, va, bytes, flags);
if (IS_ERR_VALUE(da))
--
1.7.0.4
--
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 2/3] omap3: change ISP's IOMMU da_start address
Hi Sakari,
On Tue, Mar 8, 2011 at 4:06 PM, Sakari Ailus
sakari.ai...@maxwell.research.nokia.com wrote:
David Cohen wrote:
ISP doesn't consider 0x0 as a valid address, so it should explicitly
exclude first page from allowed 'da' range.
Signed-off-by: David Cohen daco...@gmail.com
---
arch/arm/mach-omap2/omap-iommu.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/arm/mach-omap2/omap-iommu.c
b/arch/arm/mach-omap2/omap-iommu.c
index 3fc5dc7..3bea489 100644
--- a/arch/arm/mach-omap2/omap-iommu.c
+++ b/arch/arm/mach-omap2/omap-iommu.c
@@ -33,7 +33,7 @@ static struct iommu_device omap3_devices[] = {
.name = isp,
.nr_tlb_entries = 8,
.clk_name = cam_ick,
- .da_start = 0x0,
+ .da_start = 0x1000,
The NULL address is still valid for the MMU. Can the IOVMF_DA_ANON
mapping be specified by the API to be always non-NULL?
The patch 1/3 does that :)
This way it would be possible to combine IOVMF_DA_FIXED and
IOVMF_DA_ANON mappings in the same IOMMU while still being able to rely
that IOVMF_DA_ANON mappings would always be non-NULL.
Indeed the IOMMU driver supports it. The flag is set by iovm area and
each IOMMU instance supports a mix of mappings with fixed da or not.
By changing the OMAP3 isp IOMMU pdata's da_start to 0x1000, we're
explicitly saying that iovm areas with fixed 'da' or not will never
map first page to OMAP3 ISP driver, what is the behavior it expects.
Without this patch, thanks to patch 1/3 the IOMMU driver will not map
address 0x0 to OMAP3 ISP anyway. But then ISP will be passing wrong
information that it would be fine to map first page. :)
Kind regards,
David Cohen
Regards,
--
Sakari Ailus
sakari.ai...@maxwell.research.nokia.com
--
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 3/3] omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED/IOVMF_DA_ANON flags
Hi Hiroshi, Fernando, On Tue, Mar 8, 2011 at 8:53 PM, Guzman Lugo, Fernando fernando.l...@ti.com wrote: On Tue, Mar 8, 2011 at 12:09 PM, Hiroshi DOYU hiroshi.d...@nokia.com wrote: From: ext Guzman Lugo, Fernando fernando.l...@ti.com Subject: Re: [PATCH 3/3] omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED/IOVMF_DA_ANON flags Date: Tue, 8 Mar 2011 11:59:43 -0600 On Tue, Mar 8, 2011 at 6:46 AM, David Cohen daco...@gmail.com wrote: Currently IOVMM driver sets IOVMF_DA_FIXED/IOVMF_DA_ANON flags according to input 'da' address when mapping memory: da == 0: IOVMF_DA_ANON da != 0: IOVMF_DA_FIXED It prevents IOMMU to map first page with fixed 'da'. To avoid such issue, IOVMM will not automatically set IOVMF_DA_FIXED. It should now come from the user. IOVMF_DA_ANON will be automatically set if IOVMF_DA_FIXED isn't set. Signed-off-by: David Cohen daco...@gmail.com --- arch/arm/plat-omap/iovmm.c | 12 1 files changed, 8 insertions(+), 4 deletions(-) diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c index 11c9b76..dde9cb0 100644 --- a/arch/arm/plat-omap/iovmm.c +++ b/arch/arm/plat-omap/iovmm.c @@ -654,7 +654,8 @@ u32 iommu_vmap(struct iommu *obj, u32 da, const struct sg_table *sgt, flags = IOVMF_HW_MASK; flags |= IOVMF_DISCONT; flags |= IOVMF_MMIO; - flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON); + if (~flags IOVMF_DA_FIXED) + flags |= IOVMF_DA_ANON; could we use only one? both are mutual exclusive, what happen if flag is IOVMF_DA_FIXED | IOVMF_DA_ANON? so, I suggest to get rid of IOVMF_DA_ANON. Then, what about introducing some MACRO? Better names? #define set_iovmf_da_anon(flags) #define set_iovmf_da_fix(flags) #define set_iovmf_mmio(flags) will they be used by the users? I think people are more used to use iommu_vmap(obj, da, sgt, IOVMF_MMIO | IOVMF_DA_ANON); I'd be happier with this approach, instead of the macros. :) It's intuitive and very common on kernel. than set_iovmf_da_anon(flags) set_iovmf_mmio(flags) iommu_vmap(obj, da, sgt, flags); I don't have problem with the change, but I think how it is now is ok, just that we don't we two bits to handle anon/fixed da, it can be managed it only 1 bit (one flag), or is there a issue? We can exclude IOVMF_DA_ANON and stick with IOVMF_DA_FIXED only. I can resend my patch if we agree it's OK. Regards, David Regards, Fernando. .. -- 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 1/3] omap: iovmm: disallow mapping NULL address
On Tue, Mar 8, 2011 at 8:06 PM, Guzman Lugo, Fernando
fernando.l...@ti.com wrote:
On Tue, Mar 8, 2011 at 6:46 AM, David Cohen daco...@gmail.com wrote:
From: Michael Jones michael.jo...@matrix-vision.de
commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb allowed mapping
the NULL address if da_start==0, which would then not get unmapped.
Disallow this again. And spell variable 'alignment' correctly.
Signed-off-by: Michael Jones michael.jo...@matrix-vision.de
---
arch/arm/plat-omap/iovmm.c | 16 ++--
1 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
index 6dc1296..11c9b76 100644
--- a/arch/arm/plat-omap/iovmm.c
+++ b/arch/arm/plat-omap/iovmm.c
@@ -271,20 +271,24 @@ static struct iovm_struct *alloc_iovm_area(struct
iommu *obj, u32 da,
size_t bytes, u32 flags)
{
struct iovm_struct *new, *tmp;
- u32 start, prev_end, alignement;
+ u32 start, prev_end, alignment;
if (!obj || !bytes)
return ERR_PTR(-EINVAL);
start = da;
- alignement = PAGE_SIZE;
+ alignment = PAGE_SIZE;
if (flags IOVMF_DA_ANON) {
- start = obj-da_start;
+ /* Don't map address 0 */
+ if (obj-da_start)
+ start = obj-da_start;
+ else
+ start = obj-da_start + alignment;
else part obj-da_star is always 0, so why not?
start = alignment;
The patch is not from me, but I can fix it if Michael agrees.
so, why I understand, it now it is possible mapp address 0x0 only if
IOVMF_DA_ANON is not set, right? maybe that would be mention in the
patch.
Indeed address 0x0 was never meant to be mapped. The mentioned commit
on the patch's description did that without noticing. But the new
change is documented in the code by the comment Don't map address 0
and it's also mentioned on the patch description. Is it OK for you?
Regards,
David Cohen
Regards,
Fernando.
if (flags IOVMF_LINEAR)
- alignement = iopgsz_max(bytes);
- start = roundup(start, alignement);
+ alignment = iopgsz_max(bytes);
+ start = roundup(start, alignment);
} else if (start obj-da_start || start obj-da_end ||
obj-da_end - start bytes) {
return ERR_PTR(-EINVAL);
@@ -304,7 +308,7 @@ static struct iovm_struct *alloc_iovm_area(struct iommu
*obj, u32 da,
goto found;
if (tmp-da_end = start flags IOVMF_DA_ANON)
- start = roundup(tmp-da_end + 1, alignement);
+ start = roundup(tmp-da_end + 1, alignment);
prev_end = tmp-da_end;
}
--
1.7.0.4
--
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 3/3] omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED/IOVMF_DA_ANON flags
[snip] - flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON); + if (~flags IOVMF_DA_FIXED) + flags |= IOVMF_DA_ANON; could we use only one? both are mutual exclusive, what happen if flag is IOVMF_DA_FIXED | IOVMF_DA_ANON? so, I suggest to get rid of IOVMF_DA_ANON. Then, what about introducing some MACRO? Better names? #define set_iovmf_da_anon(flags) #define set_iovmf_da_fix(flags) #define set_iovmf_mmio(flags) will they be used by the users? I think people are more used to use iommu_vmap(obj, da, sgt, IOVMF_MMIO | IOVMF_DA_ANON); I'd be happier with this approach, instead of the macros. :) It's intuitive and very common on kernel. than set_iovmf_da_anon(flags) set_iovmf_mmio(flags) iommu_vmap(obj, da, sgt, flags); I don't have problem with the change, but I think how it is now is ok, just that we don't we two bits to handle anon/fixed da, it can be managed it only 1 bit (one flag), or is there a issue? We can exclude IOVMF_DA_ANON and stick with IOVMF_DA_FIXED only. I can resend my patch if we agree it's OK. sounds perfect to me. Not sure indeed if this change fits to this same patch. Looks like a 4th patch sounds better. Br, David Cohen -- 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 3/3] omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED/IOVMF_DA_ANON flags
On Tue, Mar 8, 2011 at 9:46 PM, David Cohen daco...@gmail.com wrote: [snip] - flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON); + if (~flags IOVMF_DA_FIXED) + flags |= IOVMF_DA_ANON; could we use only one? both are mutual exclusive, what happen if flag is IOVMF_DA_FIXED | IOVMF_DA_ANON? so, I suggest to get rid of IOVMF_DA_ANON. Then, what about introducing some MACRO? Better names? #define set_iovmf_da_anon(flags) #define set_iovmf_da_fix(flags) #define set_iovmf_mmio(flags) will they be used by the users? I think people are more used to use iommu_vmap(obj, da, sgt, IOVMF_MMIO | IOVMF_DA_ANON); I'd be happier with this approach, instead of the macros. :) It's intuitive and very common on kernel. than set_iovmf_da_anon(flags) set_iovmf_mmio(flags) iommu_vmap(obj, da, sgt, flags); I don't have problem with the change, but I think how it is now is ok, just that we don't we two bits to handle anon/fixed da, it can be managed it only 1 bit (one flag), or is there a issue? We can exclude IOVMF_DA_ANON and stick with IOVMF_DA_FIXED only. I can resend my patch if we agree it's OK. sounds perfect to me. Not sure indeed if this change fits to this same patch. Looks like a 4th patch sounds better. Indeed not. :) A new set is coming soon. Br, David Br, David Cohen -- 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 1/3] omap: iovmm: disallow mapping NULL address
On Tue, Mar 8, 2011 at 9:53 PM, Guzman Lugo, Fernando
fernando.l...@ti.com wrote:
On Tue, Mar 8, 2011 at 1:06 PM, David Cohen daco...@gmail.com wrote:
On Tue, Mar 8, 2011 at 8:06 PM, Guzman Lugo, Fernando
fernando.l...@ti.com wrote:
On Tue, Mar 8, 2011 at 6:46 AM, David Cohen daco...@gmail.com wrote:
From: Michael Jones michael.jo...@matrix-vision.de
commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb allowed mapping
the NULL address if da_start==0, which would then not get unmapped.
Disallow this again. And spell variable 'alignment' correctly.
Signed-off-by: Michael Jones michael.jo...@matrix-vision.de
---
arch/arm/plat-omap/iovmm.c | 16 ++--
1 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
index 6dc1296..11c9b76 100644
--- a/arch/arm/plat-omap/iovmm.c
+++ b/arch/arm/plat-omap/iovmm.c
@@ -271,20 +271,24 @@ static struct iovm_struct *alloc_iovm_area(struct
iommu *obj, u32 da,
size_t bytes, u32 flags)
{
struct iovm_struct *new, *tmp;
- u32 start, prev_end, alignement;
+ u32 start, prev_end, alignment;
if (!obj || !bytes)
return ERR_PTR(-EINVAL);
start = da;
- alignement = PAGE_SIZE;
+ alignment = PAGE_SIZE;
if (flags IOVMF_DA_ANON) {
- start = obj-da_start;
+ /* Don't map address 0 */
+ if (obj-da_start)
+ start = obj-da_start;
+ else
+ start = obj-da_start + alignment;
else part obj-da_star is always 0, so why not?
start = alignment;
The patch is not from me, but I can fix it if Michael agrees.
so, why I understand, it now it is possible mapp address 0x0 only if
IOVMF_DA_ANON is not set, right? maybe that would be mention in the
patch.
Indeed address 0x0 was never meant to be mapped. The mentioned commit
on the patch's description did that without noticing. But the new
change is documented in the code by the comment Don't map address 0
yeah, but it only applies when flags IOVMF_DA_ANON, So if I use
IOVMF_DA_FIXED and da_start == 0, I will be able to map some area
which starts from address 0x0, right? if so, that looks good to me
and the description should mention that if is disallowing mapping
address 0x0 when IOVMF_DA_ANON is used.
Yes, that's the case. I will update the comment. I hope Michael does
not complain about the changes. :)
Br,
David
Regards,
Fernando.
and it's also mentioned on the patch description. Is it OK for you?
Regards,
David Cohen
Regards,
Fernando.
if (flags IOVMF_LINEAR)
- alignement = iopgsz_max(bytes);
- start = roundup(start, alignement);
+ alignment = iopgsz_max(bytes);
+ start = roundup(start, alignment);
} else if (start obj-da_start || start obj-da_end ||
obj-da_end - start bytes) {
return ERR_PTR(-EINVAL);
@@ -304,7 +308,7 @@ static struct iovm_struct *alloc_iovm_area(struct
iommu *obj, u32 da,
goto found;
if (tmp-da_end = start flags IOVMF_DA_ANON)
- start = roundup(tmp-da_end + 1, alignement);
+ start = roundup(tmp-da_end + 1, alignment);
prev_end = tmp-da_end;
}
--
1.7.0.4
--
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 v2 0/3] *** SUBJECT HERE ***
On Tue, Mar 8, 2011 at 10:04 PM, David Cohen daco...@gmail.com wrote: *** BLURB HERE *** Sorry for this garbage :/ Br, David David Cohen (2): omap3: change ISP's IOMMU da_start address omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED flag Michael Jones (1): omap: iovmm: disallow mapping NULL address arch/arm/mach-omap2/omap-iommu.c | 2 +- arch/arm/plat-omap/include/plat/iovmm.h | 2 -- arch/arm/plat-omap/iovmm.c | 30 +++--- 3 files changed, 16 insertions(+), 18 deletions(-) -- 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 v2 0/3] omap: iovmm: Fix IOVMM check for fixed 'da'
IOVMM driver checks input 'da == 0' when mapping address to determine whether user wants fixed 'da' or not. At the same time, it doesn't disallow address 0x0 to be used, what creates an ambiguous situation. This patch set moves fixed 'da' check to the input flags. It also fixes da_start value for ISP IOMMU instance. Br, David Cohen --- David Cohen (2): omap3: change ISP's IOMMU da_start address omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED flag Michael Jones (1): omap: iovmm: disallow mapping NULL address when IOVMF_DA_ANON is set arch/arm/mach-omap2/omap-iommu.c|2 +- arch/arm/plat-omap/include/plat/iovmm.h |2 -- arch/arm/plat-omap/iovmm.c | 30 +++--- 3 files changed, 16 insertions(+), 18 deletions(-) -- 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 v2 1/3] omap: iovmm: disallow mapping NULL address when IOVMF_DA_ANON is set
From: Michael Jones michael.jo...@matrix-vision.de
commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb allowed mapping the NULL
address if da_start==0, which would then not get unmapped. Disallow
this again if IOVMF_DA_ANON is set. And spell variable 'alignment'
correctly.
Signed-off-by: Michael Jones michael.jo...@matrix-vision.de
---
arch/arm/plat-omap/iovmm.c | 16 ++--
1 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
index 6dc1296..e5f8341 100644
--- a/arch/arm/plat-omap/iovmm.c
+++ b/arch/arm/plat-omap/iovmm.c
@@ -271,20 +271,24 @@ static struct iovm_struct *alloc_iovm_area(struct iommu
*obj, u32 da,
size_t bytes, u32 flags)
{
struct iovm_struct *new, *tmp;
- u32 start, prev_end, alignement;
+ u32 start, prev_end, alignment;
if (!obj || !bytes)
return ERR_PTR(-EINVAL);
start = da;
- alignement = PAGE_SIZE;
+ alignment = PAGE_SIZE;
if (flags IOVMF_DA_ANON) {
- start = obj-da_start;
+ /* Don't map address 0 */
+ if (obj-da_start)
+ start = obj-da_start;
+ else
+ start = alignment;
if (flags IOVMF_LINEAR)
- alignement = iopgsz_max(bytes);
- start = roundup(start, alignement);
+ alignment = iopgsz_max(bytes);
+ start = roundup(start, alignment);
} else if (start obj-da_start || start obj-da_end ||
obj-da_end - start bytes) {
return ERR_PTR(-EINVAL);
@@ -304,7 +308,7 @@ static struct iovm_struct *alloc_iovm_area(struct iommu
*obj, u32 da,
goto found;
if (tmp-da_end = start flags IOVMF_DA_ANON)
- start = roundup(tmp-da_end + 1, alignement);
+ start = roundup(tmp-da_end + 1, alignment);
prev_end = tmp-da_end;
}
--
1.7.0.4
--
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 v2 2/3] omap3: change ISP's IOMMU da_start address
ISP doesn't consider 0x0 as a valid address, so it should explicitly
exclude first page from allowed 'da' range.
Signed-off-by: David Cohen daco...@gmail.com
---
arch/arm/mach-omap2/omap-iommu.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/arm/mach-omap2/omap-iommu.c b/arch/arm/mach-omap2/omap-iommu.c
index 3fc5dc7..3bea489 100644
--- a/arch/arm/mach-omap2/omap-iommu.c
+++ b/arch/arm/mach-omap2/omap-iommu.c
@@ -33,7 +33,7 @@ static struct iommu_device omap3_devices[] = {
.name = isp,
.nr_tlb_entries = 8,
.clk_name = cam_ick,
- .da_start = 0x0,
+ .da_start = 0x1000,
.da_end = 0xF000,
},
},
--
1.7.0.4
--
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 v2 3/3] omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED flag
Currently IOVMM driver sets IOVMF_DA_FIXED/IOVMF_DA_ANON flags according
to input 'da' address when mapping memory:
da == 0: IOVMF_DA_ANON
da != 0: IOVMF_DA_FIXED
It prevents IOMMU to map first page with fixed 'da'. To avoid such
issue, IOVMM will not automatically set IOVMF_DA_FIXED. It should now
come from the user throught 'flags' parameter when mapping memory.
As IOVMF_DA_ANON and IOVMF_DA_FIXED are mutually exclusive, IOVMF_DA_ANON
can be removed. The driver will now check internally if IOVMF_DA_FIXED
is set or not.
Signed-off-by: David Cohen daco...@gmail.com
---
arch/arm/plat-omap/include/plat/iovmm.h |2 --
arch/arm/plat-omap/iovmm.c | 14 +-
2 files changed, 5 insertions(+), 11 deletions(-)
diff --git a/arch/arm/plat-omap/include/plat/iovmm.h
b/arch/arm/plat-omap/include/plat/iovmm.h
index bdc7ce5..32a2f6c 100644
--- a/arch/arm/plat-omap/include/plat/iovmm.h
+++ b/arch/arm/plat-omap/include/plat/iovmm.h
@@ -71,8 +71,6 @@ struct iovm_struct {
#define IOVMF_LINEAR_MASK (3 (2 + IOVMF_SW_SHIFT))
#define IOVMF_DA_FIXED (1 (4 + IOVMF_SW_SHIFT))
-#define IOVMF_DA_ANON (2 (4 + IOVMF_SW_SHIFT))
-#define IOVMF_DA_MASK (3 (4 + IOVMF_SW_SHIFT))
extern struct iovm_struct *find_iovm_area(struct iommu *obj, u32 da);
diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
index e5f8341..894489c 100644
--- a/arch/arm/plat-omap/iovmm.c
+++ b/arch/arm/plat-omap/iovmm.c
@@ -279,7 +279,7 @@ static struct iovm_struct *alloc_iovm_area(struct iommu
*obj, u32 da,
start = da;
alignment = PAGE_SIZE;
- if (flags IOVMF_DA_ANON) {
+ if (~flags IOVMF_DA_FIXED) {
/* Don't map address 0 */
if (obj-da_start)
start = obj-da_start;
@@ -307,7 +307,7 @@ static struct iovm_struct *alloc_iovm_area(struct iommu
*obj, u32 da,
if (tmp-da_start start (tmp-da_start - start) = bytes)
goto found;
- if (tmp-da_end = start flags IOVMF_DA_ANON)
+ if (tmp-da_end = start ~flags IOVMF_DA_FIXED)
start = roundup(tmp-da_end + 1, alignment);
prev_end = tmp-da_end;
@@ -654,7 +654,6 @@ u32 iommu_vmap(struct iommu *obj, u32 da, const struct
sg_table *sgt,
flags = IOVMF_HW_MASK;
flags |= IOVMF_DISCONT;
flags |= IOVMF_MMIO;
- flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
da = __iommu_vmap(obj, da, sgt, va, bytes, flags);
if (IS_ERR_VALUE(da))
@@ -694,7 +693,7 @@ EXPORT_SYMBOL_GPL(iommu_vunmap);
* @flags: iovma and page property
*
* Allocate @bytes linearly and creates 1-n-1 mapping and returns
- * @da again, which might be adjusted if 'IOVMF_DA_ANON' is set.
+ * @da again, which might be adjusted if 'IOVMF_DA_FIXED' is not set.
*/
u32 iommu_vmalloc(struct iommu *obj, u32 da, size_t bytes, u32 flags)
{
@@ -713,7 +712,6 @@ u32 iommu_vmalloc(struct iommu *obj, u32 da, size_t bytes,
u32 flags)
flags = IOVMF_HW_MASK;
flags |= IOVMF_DISCONT;
flags |= IOVMF_ALLOC;
- flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
sgt = sgtable_alloc(bytes, flags, da, 0);
if (IS_ERR(sgt)) {
@@ -784,7 +782,7 @@ static u32 __iommu_kmap(struct iommu *obj, u32 da, u32 pa,
void *va,
* @flags: iovma and page property
*
* Creates 1-1-1 mapping and returns @da again, which can be
- * adjusted if 'IOVMF_DA_ANON' is set.
+ * adjusted if 'IOVMF_DA_FIXED' is not set.
*/
u32 iommu_kmap(struct iommu *obj, u32 da, u32 pa, size_t bytes,
u32 flags)
@@ -803,7 +801,6 @@ u32 iommu_kmap(struct iommu *obj, u32 da, u32 pa, size_t
bytes,
flags = IOVMF_HW_MASK;
flags |= IOVMF_LINEAR;
flags |= IOVMF_MMIO;
- flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
da = __iommu_kmap(obj, da, pa, va, bytes, flags);
if (IS_ERR_VALUE(da))
@@ -842,7 +839,7 @@ EXPORT_SYMBOL_GPL(iommu_kunmap);
* @flags: iovma and page property
*
* Allocate @bytes linearly and creates 1-1-1 mapping and returns
- * @da again, which might be adjusted if 'IOVMF_DA_ANON' is set.
+ * @da again, which might be adjusted if 'IOVMF_DA_FIXED' is not set.
*/
u32 iommu_kmalloc(struct iommu *obj, u32 da, size_t bytes, u32 flags)
{
@@ -862,7 +859,6 @@ u32 iommu_kmalloc(struct iommu *obj, u32 da, size_t bytes,
u32 flags)
flags = IOVMF_HW_MASK;
flags |= IOVMF_LINEAR;
flags |= IOVMF_ALLOC;
- flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
da = __iommu_kmap(obj, da, pa, va, bytes, flags);
if (IS_ERR_VALUE(da))
--
1.7.0.4
--
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] omap: iommu: disallow mapping NULL address
On Tue, Mar 8, 2011 at 10:31 PM, Laurent Pinchart laurent.pinch...@ideasonboard.com wrote: Hi David, On Monday 07 March 2011 22:35:31 David Cohen wrote: On Mon, Mar 7, 2011 at 11:19 PM, Laurent Pinchart wrote: On Monday 07 March 2011 20:41:21 David Cohen wrote: On Mon, Mar 7, 2011 at 9:25 PM, Guzman Lugo, Fernando wrote: On Mon, Mar 7, 2011 at 1:19 PM, David Cohen wrote: On Mon, Mar 7, 2011 at 9:17 PM, Guzman Lugo, Fernando wrote: On Mon, Mar 7, 2011 at 7:10 AM, Michael Jones wrote: From e7dbe4c4b64eb114f9b0804d6af3a3ca0e78acc8 Mon Sep 17 00:00:00 2001 From: Michael Jones michael.jo...@matrix-vision.de Date: Mon, 7 Mar 2011 13:36:15 +0100 Subject: [PATCH] omap: iommu: disallow mapping NULL address commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb allowed mapping the NULL address if da_start==0. Force da_start to exclude the first page. what about devices that uses page 0? ipu after reset always starts from 0x how could we map that address?? from 0x0? The driver sees da == 0 as error. May I ask you why do you want it? unlike DSP that you can load a register with the addres the DSP will boot, IPU core always starts from address 0x, so if you take IPU out of reset it will try to access address 0x0 if not map it, there will be a mmu fault. Hm. Looks like the iommu should not restrict any da. The valid da range should rely only on pdata. Michael, what about just update ISP's da_start on omap-iommu.c file? Set it to 0x1000. What about patching the OMAP3 ISP driver to use a non-zero value (maybe -1) as an invalid/freed pointer ? I wouldn't be comfortable to use 0 (or NULL) value as valid address on ISP driver. Why not ? The IOMMUs can use 0x as a valid address. Whether we allow it or not is a software architecture decision, not influenced by the IOMMU hardware. As some peripherals (namely IPU) require mapping memory to 0x, the IOMMU layer must support it and not treat 0x specially. All da == 0 checks to aim at catching invalid address values must be removed, both from the IOMMU API and the IOMMU internals. Yes, it can use and IOMMU should not treat is specially. That's the aim of my patch: [PATCH v2 3/3] omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED flag I'm not advocating to not allow 0x0, but to not use it when user is not requesting fixed da. In many sw architecture decisions 0x0 address is a special case. To avoid any misuse, IOMMU will not use it unless it's requested. If user is not requesting fixed 'da', it's not a problem to not give 0x0 anyway. IMO that's the safer option for all cases. The 'da' range (da_start and da_end) is defined per VM and specified as platform data. IMO, to set da_start = 0x1000 seems to be a correct approach for ISP as it's the only client for its IOMMU instance. We can do that, and then use 0 as an invalid pointer in the ISP driver. As the IOMMU API will use another value (what about 0x, as for the userspace mmap() call ?) to mean invalid pointer, it might be better to use the same value in the ISP driver. That can be done, of course. But the main point is in OMAP3 ISP all initial register values to read/write from/to memory are 0x0. It means sometimes we can catch bugs more easily by not mapping that address. So, IMO, OMAP3 ISP should not allow to map first page. But that's a special case for this driver only. Br, David -- 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] omap: iommu: disallow mapping NULL address
On Mon, Mar 7, 2011 at 9:17 PM, Guzman Lugo, Fernando fernando.l...@ti.com wrote: On Mon, Mar 7, 2011 at 7:10 AM, Michael Jones michael.jo...@matrix-vision.de wrote: From e7dbe4c4b64eb114f9b0804d6af3a3ca0e78acc8 Mon Sep 17 00:00:00 2001 From: Michael Jones michael.jo...@matrix-vision.de Date: Mon, 7 Mar 2011 13:36:15 +0100 Subject: [PATCH] omap: iommu: disallow mapping NULL address commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb allowed mapping the NULL address if da_start==0. Force da_start to exclude the first page. what about devices that uses page 0? ipu after reset always starts from 0x how could we map that address?? from 0x0? The driver sees da == 0 as error. May I ask you why do you want it? Br, David Regards, Fernando. Signed-off-by: Michael Jones michael.jo...@matrix-vision.de --- arch/arm/plat-omap/iommu.c | 6 -- 1 files changed, 4 insertions(+), 2 deletions(-) diff --git a/arch/arm/plat-omap/iommu.c b/arch/arm/plat-omap/iommu.c index 5990ea6..dcb5513 100644 --- a/arch/arm/plat-omap/iommu.c +++ b/arch/arm/plat-omap/iommu.c @@ -850,7 +850,7 @@ int iommu_set_da_range(struct iommu *obj, u32 start, u32 end) if (end start || !PAGE_ALIGN(start | end)) return -EINVAL; - obj-da_start = start; + obj-da_start = max(start, (u32)PAGE_SIZE); obj-da_end = end; return 0; @@ -950,7 +950,9 @@ static int __devinit omap_iommu_probe(struct platform_device *pdev) obj-name = pdata-name; obj-dev = pdev-dev; obj-ctx = (void *)obj + sizeof(*obj); - obj-da_start = pdata-da_start; + + /* reserve the first page for NULL */ + obj-da_start = max(pdata-da_start, (u32)PAGE_SIZE); obj-da_end = pdata-da_end; mutex_init(obj-iommu_lock); -- 1.7.4.1 MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler Registergericht: Amtsgericht Stuttgart, HRB 271090 Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner -- 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] omap: iommu: disallow mapping NULL address
On Mon, Mar 7, 2011 at 9:25 PM, Guzman Lugo, Fernando fernando.l...@ti.com wrote: On Mon, Mar 7, 2011 at 1:19 PM, David Cohen daco...@gmail.com wrote: On Mon, Mar 7, 2011 at 9:17 PM, Guzman Lugo, Fernando fernando.l...@ti.com wrote: On Mon, Mar 7, 2011 at 7:10 AM, Michael Jones michael.jo...@matrix-vision.de wrote: From e7dbe4c4b64eb114f9b0804d6af3a3ca0e78acc8 Mon Sep 17 00:00:00 2001 From: Michael Jones michael.jo...@matrix-vision.de Date: Mon, 7 Mar 2011 13:36:15 +0100 Subject: [PATCH] omap: iommu: disallow mapping NULL address commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb allowed mapping the NULL address if da_start==0. Force da_start to exclude the first page. what about devices that uses page 0? ipu after reset always starts from 0x how could we map that address?? from 0x0? The driver sees da == 0 as error. May I ask you why do you want it? unlike DSP that you can load a register with the addres the DSP will boot, IPU core always starts from address 0x, so if you take IPU out of reset it will try to access address 0x0 if not map it, there will be a mmu fault. Hm. Looks like the iommu should not restrict any da. The valid da range should rely only on pdata. Michael, what about just update ISP's da_start on omap-iommu.c file? Set it to 0x1000. Hiroshi, any opinion? Br, David Regards, Fernando. Br, David Regards, Fernando. Signed-off-by: Michael Jones michael.jo...@matrix-vision.de --- arch/arm/plat-omap/iommu.c | 6 -- 1 files changed, 4 insertions(+), 2 deletions(-) diff --git a/arch/arm/plat-omap/iommu.c b/arch/arm/plat-omap/iommu.c index 5990ea6..dcb5513 100644 --- a/arch/arm/plat-omap/iommu.c +++ b/arch/arm/plat-omap/iommu.c @@ -850,7 +850,7 @@ int iommu_set_da_range(struct iommu *obj, u32 start, u32 end) if (end start || !PAGE_ALIGN(start | end)) return -EINVAL; - obj-da_start = start; + obj-da_start = max(start, (u32)PAGE_SIZE); obj-da_end = end; return 0; @@ -950,7 +950,9 @@ static int __devinit omap_iommu_probe(struct platform_device *pdev) obj-name = pdata-name; obj-dev = pdev-dev; obj-ctx = (void *)obj + sizeof(*obj); - obj-da_start = pdata-da_start; + + /* reserve the first page for NULL */ + obj-da_start = max(pdata-da_start, (u32)PAGE_SIZE); obj-da_end = pdata-da_end; mutex_init(obj-iommu_lock); -- 1.7.4.1 MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler Registergericht: Amtsgericht Stuttgart, HRB 271090 Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner -- 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] omap: iommu: disallow mapping NULL address
On Mon, Mar 7, 2011 at 11:19 PM, Laurent Pinchart laurent.pinch...@ideasonboard.com wrote: Hi David, Hi Laurent, On Monday 07 March 2011 20:41:21 David Cohen wrote: On Mon, Mar 7, 2011 at 9:25 PM, Guzman Lugo, Fernando wrote: On Mon, Mar 7, 2011 at 1:19 PM, David Cohen wrote: On Mon, Mar 7, 2011 at 9:17 PM, Guzman Lugo, Fernando wrote: On Mon, Mar 7, 2011 at 7:10 AM, Michael Jones wrote: From e7dbe4c4b64eb114f9b0804d6af3a3ca0e78acc8 Mon Sep 17 00:00:00 2001 From: Michael Jones michael.jo...@matrix-vision.de Date: Mon, 7 Mar 2011 13:36:15 +0100 Subject: [PATCH] omap: iommu: disallow mapping NULL address commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb allowed mapping the NULL address if da_start==0. Force da_start to exclude the first page. what about devices that uses page 0? ipu after reset always starts from 0x how could we map that address?? from 0x0? The driver sees da == 0 as error. May I ask you why do you want it? unlike DSP that you can load a register with the addres the DSP will boot, IPU core always starts from address 0x, so if you take IPU out of reset it will try to access address 0x0 if not map it, there will be a mmu fault. Hm. Looks like the iommu should not restrict any da. The valid da range should rely only on pdata. Michael, what about just update ISP's da_start on omap-iommu.c file? Set it to 0x1000. What about patching the OMAP3 ISP driver to use a non-zero value (maybe -1) as an invalid/freed pointer ? I wouldn't be comfortable to use 0 (or NULL) value as valid address on ISP driver. The 'da' range (da_start and da_end) is defined per VM and specified as platform data. IMO, to set da_start = 0x1000 seems to be a correct approach for ISP as it's the only client for its IOMMU instance. Regards, David -- 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: [GIT PULL FOR 2.6.39] Media controller and OMAP3 ISP driver
Hi Hans, On Sat, Mar 5, 2011 at 1:52 PM, Hans Verkuil hverk...@xs4all.nl wrote: On Friday, March 04, 2011 21:10:05 Mauro Carvalho Chehab wrote: Em 03-03-2011 07:25, Laurent Pinchart escreveu: Hi Mauro, The following changes since commit 88a763df226facb74fdb254563e30e9efb64275c: [media] dw2102: prof 1100 corrected (2011-03-02 16:56:54 -0300) are available in the git repository at: git://linuxtv.org/pinchartl/media.git media-2.6.39-0005-omap3isp The branch has been rebased on top of the latest for_v2.6.39 branch, with the v4l2-ioctl.c conflict resolved. Antti Koskipaa (1): v4l: v4l2_subdev userspace crop API David Cohen (1): omap3isp: Statistics Laurent Pinchart (36): v4l: Share code between video_usercopy and video_ioctl2 v4l: subdev: Don't require core operations v4l: subdev: Add device node support v4l: subdev: Uninline the v4l2_subdev_init function v4l: subdev: Control ioctls support media: Media device node support media: Media device media: Entities, pads and links media: Entity use count media: Media device information query media: Entities, pads and links enumeration media: Links setup media: Pipelines and media streams v4l: Add a media_device pointer to the v4l2_device structure v4l: Make video_device inherit from media_entity v4l: Make v4l2_subdev inherit from media_entity v4l: Move the media/v4l2-mediabus.h header to include/linux v4l: Replace enums with fixed-sized fields in public structure v4l: Rename V4L2_MBUS_FMT_GREY8_1X8 to V4L2_MBUS_FMT_Y8_1X8 v4l: Group media bus pixel codes by types and sort them alphabetically The presence of those mediabus names against the traditional fourcc codes at the API adds some mess to the media controller. Not sure how to solve, but maybe the best way is to add a table at the V4L2 API associating each media bus format to the corresponding V4L2 fourcc codes. You can't do that in general. Only for specific hardware platforms. If you could do it, then we would have never bothered creating these mediabus fourccs. How a mediabus fourcc translates to a pixelcode (== memory format) depends entirely on the hardware capabilities (mostly that of the DMA engine). May I ask you one question here? (not entirely related to this patch set). Why pixelcode != mediabus fourcc? e.g. OMAP2 camera driver talks to sensor through subdev interface and sets its own output pixelformat depending on sensor's mediabus fourcc. So it needs a translation table mbus_pixelcode - pixelformat. Why can't it be pixelformat - pixelformat ? Regards, David A generic V4L2 application will never use mediabus fourcc codes. It's only used by drivers and applications written specifically for that hardware and using /dev/v4l-subdevX devices. Regards, Hans -- Hans Verkuil - video4linux developer - sponsored by Cisco -- 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 -- 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: omap3isp cache error when unloading
Hi,
[snip]
Sorry, I should've mentioned: I'm using your media-0005-omap3isp branch
based on 2.6.38-rc5. I didn't have the problem with 2.6.37, either.
It's actually not related to mis-configuring the ISP pipeline like I
thought at first- it also happens after I have successfully captured images.
I've since tracked down the problem, although I don't understand the
cache management well enough to be sure it's a proper fix, so hopefully
some new recipients on this can make suggestions/comments.
The patch below solves the problem, which modifies a commit by Fernando
Guzman Lugo from December.
-Michael
From db35fb8edca2a4f8fd37197d77fd58676cb1dcac Mon Sep 17 00:00:00 2001
From: Michael Jones michael.jo...@matrix-vision.de
Date: Thu, 3 Mar 2011 16:50:39 +0100
Subject: [PATCH] fix iovmm slab cache error on module unload
modify OMAP: iommu: create new api to set valid da range
This modifies commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb.
---
arch/arm/plat-omap/iovmm.c | 5 -
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
index 6dc1296..2fba6f1 100644
--- a/arch/arm/plat-omap/iovmm.c
+++ b/arch/arm/plat-omap/iovmm.c
@@ -280,7 +280,10 @@ static struct iovm_struct *alloc_iovm_area(struct iommu
*obj, u32 da,
alignement = PAGE_SIZE;
if (flags IOVMF_DA_ANON) {
- start = obj-da_start;
+ /*
+ * Reserve the first page for NULL
+ */
+ start = obj-da_start + PAGE_SIZE;
IMO if obj-da_start != 0, no need to add PAGE_SIZE. Otherwise, it
does make sense to correct wrong obj-da_start == 0. Another thing is
this piece of code is using alignement (alignment) variable instead of
PAGE_SIZE (which is the same value).
Br,
David
--
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: omap3isp cache error when unloading
[snip]
From 2712f2fd087ca782e964c912c7f1973e7d84f2b7 Mon Sep 17 00:00:00 2001
From: Michael Jones michael.jo...@matrix-vision.de
Date: Fri, 4 Mar 2011 15:09:48 +0100
Subject: [PATCH] omap: iovmm: disallow mapping NULL address
commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb allowed mapping
the NULL address if da_start==0, which would then not get unmapped.
Disallow this again. And spell variable 'alignment' correctly.
Signed-off-by: Michael Jones michael.jo...@matrix-vision.de
---
arch/arm/plat-omap/iovmm.c | 16 ++--
1 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
index 6dc1296..11c9b76 100644
--- a/arch/arm/plat-omap/iovmm.c
+++ b/arch/arm/plat-omap/iovmm.c
@@ -271,20 +271,24 @@ static struct iovm_struct *alloc_iovm_area(struct
iommu *obj, u32 da,
size_t bytes, u32 flags)
{
struct iovm_struct *new, *tmp;
- u32 start, prev_end, alignement;
+ u32 start, prev_end, alignment;
if (!obj || !bytes)
return ERR_PTR(-EINVAL);
start = da;
- alignement = PAGE_SIZE;
+ alignment = PAGE_SIZE;
if (flags IOVMF_DA_ANON) {
- start = obj-da_start;
+ /* Don't map address 0 */
+ if (obj-da_start)
+ start = obj-da_start;
+ else
+ start = obj-da_start + alignment;
It seems to be fine for me now. Let's see what Hiroshi says.
Sorry, I'm afraid I changed my mind after take a look into the driver. :)
Try to correct obj-da_start in the functions iommu_set_da_range() and
omap_iommu_probe(). That should be the correct way. Your patch doesn't
fix this situation when IOVMF_DA_ANON isn't set.
After obj-da_start is correctly set, your current patch is non longer required.
Regards,
David
Regards,
David
if (flags IOVMF_LINEAR)
- alignement = iopgsz_max(bytes);
- start = roundup(start, alignement);
+ alignment = iopgsz_max(bytes);
+ start = roundup(start, alignment);
} else if (start obj-da_start || start obj-da_end ||
obj-da_end - start bytes) {
return ERR_PTR(-EINVAL);
@@ -304,7 +308,7 @@ static struct iovm_struct *alloc_iovm_area(struct iommu
*obj, u32 da,
goto found;
if (tmp-da_end = start flags IOVMF_DA_ANON)
- start = roundup(tmp-da_end + 1, alignement);
+ start = roundup(tmp-da_end + 1, alignment);
prev_end = tmp-da_end;
}
--
1.7.4.1
MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner
--
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: [GIT PULL FOR 2.6.39] Media controller and OMAP3 ISP driver
Hi Mauro, On Fri, Mar 4, 2011 at 10:10 PM, Mauro Carvalho Chehab mche...@redhat.com wrote: Em 03-03-2011 07:25, Laurent Pinchart escreveu: Hi Mauro, The following changes since commit 88a763df226facb74fdb254563e30e9efb64275c: [media] dw2102: prof 1100 corrected (2011-03-02 16:56:54 -0300) are available in the git repository at: git://linuxtv.org/pinchartl/media.git media-2.6.39-0005-omap3isp The branch has been rebased on top of the latest for_v2.6.39 branch, with the v4l2-ioctl.c conflict resolved. Antti Koskipaa (1): v4l: v4l2_subdev userspace crop API David Cohen (1): omap3isp: Statistics Laurent Pinchart (36): v4l: Share code between video_usercopy and video_ioctl2 v4l: subdev: Don't require core operations v4l: subdev: Add device node support v4l: subdev: Uninline the v4l2_subdev_init function v4l: subdev: Control ioctls support media: Media device node support media: Media device media: Entities, pads and links media: Entity use count media: Media device information query media: Entities, pads and links enumeration media: Links setup media: Pipelines and media streams v4l: Add a media_device pointer to the v4l2_device structure v4l: Make video_device inherit from media_entity v4l: Make v4l2_subdev inherit from media_entity v4l: Move the media/v4l2-mediabus.h header to include/linux v4l: Replace enums with fixed-sized fields in public structure v4l: Rename V4L2_MBUS_FMT_GREY8_1X8 to V4L2_MBUS_FMT_Y8_1X8 v4l: Group media bus pixel codes by types and sort them alphabetically The presence of those mediabus names against the traditional fourcc codes at the API adds some mess to the media controller. Not sure how to solve, but maybe the best way is to add a table at the V4L2 API associating each media bus format to the corresponding V4L2 fourcc codes. That would be good. OMAP2 camera driver also needs such table. Br, David Cheers, Mauro -- 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 -- 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: [GIT PULL FOR 2.6.39] Media controller and OMAP3 ISP driver
Hi Mauro, On Sat, Mar 5, 2011 at 12:16 AM, Mauro Carvalho Chehab mche...@redhat.com wrote: Hi Laurent, Em 17-02-2011 13:06, Laurent Pinchart escreveu: Hi Mauro, The following changes since commit 85e2efbb1db9a18d218006706d6e4fbeb0216213: Linux 2.6.38-rc5 (2011-02-15 19:23:45 -0800) are available in the git repository at: git://linuxtv.org/pinchartl/media.git media-0005-omap3isp I've added the patches that looked ok on my eyes at: http://git.linuxtv.org/mchehab/experimental.git?a=shortlog;h=refs/heads/media_controller There are just a few small adjustments on a few of them, as I've commented. I prefer if you do them on separate patches, to save my work of not needing to review the entire series again. The ones still pending on my quilt tree are: 0030-v4l-subdev-Generic-ioctl-support.patch 0040-omap3isp-OMAP3-ISP-core.patch 0041-omap3isp-Video-devices-and-buffers-queue.patch 0042-omap3isp-CCP2-CSI2-receivers.patch 0043-omap3isp-CCDC-preview-engine-and-resizer.patch 0044-omap3isp-Statistics.patch 0045-omap3isp-Kconfig-and-Makefile.patch 0046-omap3isp-Add-set-performance-callback-in-isp-platfor.patch with the following diffstat: Documentation/video4linux/v4l2-framework.txt | 5 + MAINTAINERS | 6 + drivers/media/video/Kconfig | 13 + drivers/media/video/Makefile | 2 + drivers/media/video/omap3-isp/Makefile | 13 + drivers/media/video/omap3-isp/cfa_coef_table.h | 61 + drivers/media/video/omap3-isp/gamma_table.h | 90 + drivers/media/video/omap3-isp/isp.c | 2220 +++ drivers/media/video/omap3-isp/isp.h | 428 drivers/media/video/omap3-isp/ispccdc.c | 2268 drivers/media/video/omap3-isp/ispccdc.h | 219 ++ drivers/media/video/omap3-isp/ispccp2.c | 1173 ++ drivers/media/video/omap3-isp/ispccp2.h | 98 + drivers/media/video/omap3-isp/ispcsi2.c | 1317 drivers/media/video/omap3-isp/ispcsi2.h | 166 ++ drivers/media/video/omap3-isp/ispcsiphy.c | 247 +++ drivers/media/video/omap3-isp/ispcsiphy.h | 74 + drivers/media/video/omap3-isp/isph3a.h | 117 + drivers/media/video/omap3-isp/isph3a_aewb.c | 374 drivers/media/video/omap3-isp/isph3a_af.c | 429 drivers/media/video/omap3-isp/isphist.c | 520 + drivers/media/video/omap3-isp/isphist.h | 40 + drivers/media/video/omap3-isp/isppreview.c | 2113 ++ drivers/media/video/omap3-isp/isppreview.h | 214 ++ drivers/media/video/omap3-isp/ispqueue.c | 1153 ++ drivers/media/video/omap3-isp/ispqueue.h | 187 ++ drivers/media/video/omap3-isp/ispreg.h | 1589 ++ drivers/media/video/omap3-isp/ispresizer.c | 1693 +++ drivers/media/video/omap3-isp/ispresizer.h | 147 ++ drivers/media/video/omap3-isp/ispstat.c | 1092 ++ drivers/media/video/omap3-isp/ispstat.h | 169 ++ drivers/media/video/omap3-isp/ispvideo.c | 1255 +++ drivers/media/video/omap3-isp/ispvideo.h | 202 ++ drivers/media/video/omap3-isp/luma_enhance_table.h | 42 + drivers/media/video/omap3-isp/noise_filter_table.h | 30 + drivers/media/video/v4l2-subdev.c | 2 +- drivers/media/video/videobuf-dma-contig.c | 2 +- include/linux/Kbuild | 1 + 38 files changed, 19769 insertions(+), 2 deletions(-) I used quilt for all patches, except for the one patch with some gifs, where I did a git cherry-pick. So, the imported patches should be ok. Of course, it doesn't hurt do double check. The main issue with the omap3isp is due to the presence of private ioctl's that I don't have a clear idea about what they are really doing. I couldn't see any documentation about them on a very quick look. While I suspect that they are used only for 3A, I have no means of being sure about that. Also, as I've said several times, while I don't like, I have nothing against having some ioctls that would be used by a vendor to implement their own 3A software algorithms that he may need to hide for some reason or have any patents applied to the algorithm, but only if: 1) such algorithms are implemented on userspace; Yes. 2) the userspace API used by them is fully documented, in order to allow that someone else with enough motivation and spare time may want to implement his own algorithm (including an open-source one); The API is pretty close to what is found on public OMAP3 TRM. I'd say it's almost to fill registers through a userspace API. 3) there are no patents denying or charging for the usage
Re: [GIT PULL FOR 2.6.39] Media controller and OMAP3 ISP driver
On Sat, Mar 5, 2011 at 12:43 AM, Mauro Carvalho Chehab mche...@redhat.com wrote: Em 04-03-2011 19:33, David Cohen escreveu: Hi Mauro, On Sat, Mar 5, 2011 at 12:16 AM, Mauro Carvalho Chehab mche...@redhat.com wrote: Hi Laurent, Em 17-02-2011 13:06, Laurent Pinchart escreveu: Hi Mauro, The following changes since commit 85e2efbb1db9a18d218006706d6e4fbeb0216213: Linux 2.6.38-rc5 (2011-02-15 19:23:45 -0800) are available in the git repository at: git://linuxtv.org/pinchartl/media.git media-0005-omap3isp I've added the patches that looked ok on my eyes at: http://git.linuxtv.org/mchehab/experimental.git?a=shortlog;h=refs/heads/media_controller There are just a few small adjustments on a few of them, as I've commented. I prefer if you do them on separate patches, to save my work of not needing to review the entire series again. The ones still pending on my quilt tree are: 0030-v4l-subdev-Generic-ioctl-support.patch 0040-omap3isp-OMAP3-ISP-core.patch 0041-omap3isp-Video-devices-and-buffers-queue.patch 0042-omap3isp-CCP2-CSI2-receivers.patch 0043-omap3isp-CCDC-preview-engine-and-resizer.patch 0044-omap3isp-Statistics.patch 0045-omap3isp-Kconfig-and-Makefile.patch 0046-omap3isp-Add-set-performance-callback-in-isp-platfor.patch with the following diffstat: Documentation/video4linux/v4l2-framework.txt | 5 + MAINTAINERS | 6 + drivers/media/video/Kconfig | 13 + drivers/media/video/Makefile | 2 + drivers/media/video/omap3-isp/Makefile | 13 + drivers/media/video/omap3-isp/cfa_coef_table.h | 61 + drivers/media/video/omap3-isp/gamma_table.h | 90 + drivers/media/video/omap3-isp/isp.c | 2220 +++ drivers/media/video/omap3-isp/isp.h | 428 drivers/media/video/omap3-isp/ispccdc.c | 2268 drivers/media/video/omap3-isp/ispccdc.h | 219 ++ drivers/media/video/omap3-isp/ispccp2.c | 1173 ++ drivers/media/video/omap3-isp/ispccp2.h | 98 + drivers/media/video/omap3-isp/ispcsi2.c | 1317 drivers/media/video/omap3-isp/ispcsi2.h | 166 ++ drivers/media/video/omap3-isp/ispcsiphy.c | 247 +++ drivers/media/video/omap3-isp/ispcsiphy.h | 74 + drivers/media/video/omap3-isp/isph3a.h | 117 + drivers/media/video/omap3-isp/isph3a_aewb.c | 374 drivers/media/video/omap3-isp/isph3a_af.c | 429 drivers/media/video/omap3-isp/isphist.c | 520 + drivers/media/video/omap3-isp/isphist.h | 40 + drivers/media/video/omap3-isp/isppreview.c | 2113 ++ drivers/media/video/omap3-isp/isppreview.h | 214 ++ drivers/media/video/omap3-isp/ispqueue.c | 1153 ++ drivers/media/video/omap3-isp/ispqueue.h | 187 ++ drivers/media/video/omap3-isp/ispreg.h | 1589 ++ drivers/media/video/omap3-isp/ispresizer.c | 1693 +++ drivers/media/video/omap3-isp/ispresizer.h | 147 ++ drivers/media/video/omap3-isp/ispstat.c | 1092 ++ drivers/media/video/omap3-isp/ispstat.h | 169 ++ drivers/media/video/omap3-isp/ispvideo.c | 1255 +++ drivers/media/video/omap3-isp/ispvideo.h | 202 ++ drivers/media/video/omap3-isp/luma_enhance_table.h | 42 + drivers/media/video/omap3-isp/noise_filter_table.h | 30 + drivers/media/video/v4l2-subdev.c | 2 +- drivers/media/video/videobuf-dma-contig.c | 2 +- include/linux/Kbuild | 1 + 38 files changed, 19769 insertions(+), 2 deletions(-) I used quilt for all patches, except for the one patch with some gifs, where I did a git cherry-pick. So, the imported patches should be ok. Of course, it doesn't hurt do double check. The main issue with the omap3isp is due to the presence of private ioctl's that I don't have a clear idea about what they are really doing. I couldn't see any documentation about them on a very quick look. While I suspect that they are used only for 3A, I have no means of being sure about that. Also, as I've said several times, while I don't like, I have nothing against having some ioctls that would be used by a vendor to implement their own 3A software algorithms that he may need to hide for some reason or have any patents applied to the algorithm, but only if: 1) such algorithms are implemented on userspace; Yes. 2) the userspace API used by them is fully documented, in order to allow that someone else with enough motivation and spare time may want to implement his own algorithm (including an open-source one); The API is pretty close to what is found on public OMAP3
Re: [GIT PULL FOR 2.6.39] Media controller and OMAP3 ISP driver
On Sat, Mar 5, 2011 at 1:49 AM, Mauro Carvalho Chehab mche...@redhat.com wrote: Em 04-03-2011 19:49, David Cohen escreveu: On Sat, Mar 5, 2011 at 12:43 AM, Mauro Carvalho Chehab mche...@redhat.com wrote: Em 04-03-2011 19:33, David Cohen escreveu: Hi Mauro, On Sat, Mar 5, 2011 at 12:16 AM, Mauro Carvalho Chehab mche...@redhat.com wrote: Hi Laurent, Em 17-02-2011 13:06, Laurent Pinchart escreveu: Hi Mauro, The following changes since commit 85e2efbb1db9a18d218006706d6e4fbeb0216213: Linux 2.6.38-rc5 (2011-02-15 19:23:45 -0800) are available in the git repository at: git://linuxtv.org/pinchartl/media.git media-0005-omap3isp I've added the patches that looked ok on my eyes at: http://git.linuxtv.org/mchehab/experimental.git?a=shortlog;h=refs/heads/media_controller There are just a few small adjustments on a few of them, as I've commented. I prefer if you do them on separate patches, to save my work of not needing to review the entire series again. The ones still pending on my quilt tree are: 0030-v4l-subdev-Generic-ioctl-support.patch 0040-omap3isp-OMAP3-ISP-core.patch 0041-omap3isp-Video-devices-and-buffers-queue.patch 0042-omap3isp-CCP2-CSI2-receivers.patch 0043-omap3isp-CCDC-preview-engine-and-resizer.patch 0044-omap3isp-Statistics.patch 0045-omap3isp-Kconfig-and-Makefile.patch 0046-omap3isp-Add-set-performance-callback-in-isp-platfor.patch with the following diffstat: Documentation/video4linux/v4l2-framework.txt | 5 + MAINTAINERS | 6 + drivers/media/video/Kconfig | 13 + drivers/media/video/Makefile | 2 + drivers/media/video/omap3-isp/Makefile | 13 + drivers/media/video/omap3-isp/cfa_coef_table.h | 61 + drivers/media/video/omap3-isp/gamma_table.h | 90 + drivers/media/video/omap3-isp/isp.c | 2220 +++ drivers/media/video/omap3-isp/isp.h | 428 drivers/media/video/omap3-isp/ispccdc.c | 2268 drivers/media/video/omap3-isp/ispccdc.h | 219 ++ drivers/media/video/omap3-isp/ispccp2.c | 1173 ++ drivers/media/video/omap3-isp/ispccp2.h | 98 + drivers/media/video/omap3-isp/ispcsi2.c | 1317 drivers/media/video/omap3-isp/ispcsi2.h | 166 ++ drivers/media/video/omap3-isp/ispcsiphy.c | 247 +++ drivers/media/video/omap3-isp/ispcsiphy.h | 74 + drivers/media/video/omap3-isp/isph3a.h | 117 + drivers/media/video/omap3-isp/isph3a_aewb.c | 374 drivers/media/video/omap3-isp/isph3a_af.c | 429 drivers/media/video/omap3-isp/isphist.c | 520 + drivers/media/video/omap3-isp/isphist.h | 40 + drivers/media/video/omap3-isp/isppreview.c | 2113 ++ drivers/media/video/omap3-isp/isppreview.h | 214 ++ drivers/media/video/omap3-isp/ispqueue.c | 1153 ++ drivers/media/video/omap3-isp/ispqueue.h | 187 ++ drivers/media/video/omap3-isp/ispreg.h | 1589 ++ drivers/media/video/omap3-isp/ispresizer.c | 1693 +++ drivers/media/video/omap3-isp/ispresizer.h | 147 ++ drivers/media/video/omap3-isp/ispstat.c | 1092 ++ drivers/media/video/omap3-isp/ispstat.h | 169 ++ drivers/media/video/omap3-isp/ispvideo.c | 1255 +++ drivers/media/video/omap3-isp/ispvideo.h | 202 ++ drivers/media/video/omap3-isp/luma_enhance_table.h | 42 + drivers/media/video/omap3-isp/noise_filter_table.h | 30 + drivers/media/video/v4l2-subdev.c | 2 +- drivers/media/video/videobuf-dma-contig.c | 2 +- include/linux/Kbuild | 1 + 38 files changed, 19769 insertions(+), 2 deletions(-) I used quilt for all patches, except for the one patch with some gifs, where I did a git cherry-pick. So, the imported patches should be ok. Of course, it doesn't hurt do double check. The main issue with the omap3isp is due to the presence of private ioctl's that I don't have a clear idea about what they are really doing. I couldn't see any documentation about them on a very quick look. While I suspect that they are used only for 3A, I have no means of being sure about that. Also, as I've said several times, while I don't like, I have nothing against having some ioctls that would be used by a vendor to implement their own 3A software algorithms that he may need to hide for some reason or have any patents applied to the algorithm, but only if: 1) such algorithms are implemented on userspace; Yes. 2) the userspace API used by them is fully documented, in order to allow that someone else with enough motivation and spare time
Re: [PATCH v2 1/1] headers: fix circular dependency between linux/sched.h and linux/wait.h
On Mon, Feb 21, 2011 at 7:27 PM, Oleg Nesterov o...@redhat.com wrote: On 02/21, Oleg Nesterov wrote: On 02/21, Peter Zijlstra wrote: afaict its needed because struct signal_struct and struct sighand_struct include a wait_queue_head_t. The inclusion seems to come through completion.h, but afaict we don't actually need to include completion.h because all we have is a pointer to a completion, which is perfectly fine with an incomplete type. This all would suggest we move the signal bits into their own header (include/linux/signal.h already exists and seems inviting). Agreed, sched.h contatins a lot of garbage, including the signal bits. As for signal_struct in particular I am not really sure, it is just misnamed. It is in fact struct process or struct thread_group. But dequeue_signal/etc should go into signal.h. The only problem, it is not clear how to test such a change. Ah. sched.h includes signal.h, the testing is not the problem. If sched.h includes signal.h and we move wait_queue_head_t users to signal.h, it means signal.h should include wait.h and then it is a problem to include sched.h in wait.h. So, we can (at least) safely move some declarations. Safely, yes, but it won't solve the issue for TASK_* in wait.h. Br, David Oleg. -- 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 1/1] headers: fix circular dependency between linux/sched.h and linux/wait.h
Currently sched.h and wait.h have circular dependency between both.
wait.h defines macros wake_up*() which use macros TASK_* defined by
sched.h. But as sched.h indirectly includes wait.h, such wait.h header
file can't include sched.h too. The side effect is when some file
includes wait.h and tries to use its wake_up*() macros, it's necessary
to include sched.h also.
This patch moves all TASK_* macros from linux/sched.h to a new header
file linux/task_sched.h. This way, both sched.h and wait.h can include
task_sched.h and fix the circular dependency. No need to include sched.h
anymore when wake_up*() macros are used.
Signed-off-by: David Cohen daco...@gmail.com
---
include/linux/sched.h | 61 +-
include/linux/task_sched.h | 64
include/linux/wait.h |1 +
3 files changed, 66 insertions(+), 60 deletions(-)
create mode 100644 include/linux/task_sched.h
diff --git a/include/linux/sched.h b/include/linux/sched.h
index d747f94..c60dcee 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -90,6 +90,7 @@ struct sched_param {
#include linux/task_io_accounting.h
#include linux/latencytop.h
#include linux/cred.h
+#include linux/task_sched.h
#include asm/processor.h
@@ -169,63 +170,6 @@ print_cfs_rq(struct seq_file *m, int cpu, struct cfs_rq
*cfs_rq)
#endif
/*
- * Task state bitmask. NOTE! These bits are also
- * encoded in fs/proc/array.c: get_task_state().
- *
- * We have two separate sets of flags: task-state
- * is about runnability, while task-exit_state are
- * about the task exiting. Confusing, but this way
- * modifying one set can't modify the other one by
- * mistake.
- */
-#define TASK_RUNNING 0
-#define TASK_INTERRUPTIBLE 1
-#define TASK_UNINTERRUPTIBLE 2
-#define __TASK_STOPPED 4
-#define __TASK_TRACED 8
-/* in tsk-exit_state */
-#define EXIT_ZOMBIE16
-#define EXIT_DEAD 32
-/* in tsk-state again */
-#define TASK_DEAD 64
-#define TASK_WAKEKILL 128
-#define TASK_WAKING256
-#define TASK_STATE_MAX 512
-
-#define TASK_STATE_TO_CHAR_STR RSDTtZXxKW
-
-extern char ___assert_task_state[1 - 2*!!(
- sizeof(TASK_STATE_TO_CHAR_STR)-1 != ilog2(TASK_STATE_MAX)+1)];
-
-/* Convenience macros for the sake of set_task_state */
-#define TASK_KILLABLE (TASK_WAKEKILL | TASK_UNINTERRUPTIBLE)
-#define TASK_STOPPED (TASK_WAKEKILL | __TASK_STOPPED)
-#define TASK_TRACED(TASK_WAKEKILL | __TASK_TRACED)
-
-/* Convenience macros for the sake of wake_up */
-#define TASK_NORMAL(TASK_INTERRUPTIBLE | TASK_UNINTERRUPTIBLE)
-#define TASK_ALL (TASK_NORMAL | __TASK_STOPPED | __TASK_TRACED)
-
-/* get_task_state() */
-#define TASK_REPORT(TASK_RUNNING | TASK_INTERRUPTIBLE | \
-TASK_UNINTERRUPTIBLE | __TASK_STOPPED | \
-__TASK_TRACED)
-
-#define task_is_traced(task) ((task-state __TASK_TRACED) != 0)
-#define task_is_stopped(task) ((task-state __TASK_STOPPED) != 0)
-#define task_is_dead(task) ((task)-exit_state != 0)
-#define task_is_stopped_or_traced(task)\
- ((task-state (__TASK_STOPPED | __TASK_TRACED)) != 0)
-#define task_contributes_to_load(task) \
- ((task-state TASK_UNINTERRUPTIBLE) != 0 \
-(task-flags PF_FREEZING) == 0)
-
-#define __set_task_state(tsk, state_value) \
- do { (tsk)-state = (state_value); } while (0)
-#define set_task_state(tsk, state_value) \
- set_mb((tsk)-state, (state_value))
-
-/*
* set_current_state() includes a barrier so that the write of current-state
* is correctly serialised wrt the caller's subsequent test of whether to
* actually sleep:
@@ -241,9 +185,6 @@ extern char ___assert_task_state[1 - 2*!!(
#define set_current_state(state_value) \
set_mb(current-state, (state_value))
-/* Task command name length */
-#define TASK_COMM_LEN 16
-
#include linux/spinlock.h
/*
diff --git a/include/linux/task_sched.h b/include/linux/task_sched.h
new file mode 100644
index 000..3787387
--- /dev/null
+++ b/include/linux/task_sched.h
@@ -0,0 +1,64 @@
+#ifndef _LINUX_TASK_H
+#define _LINUX_TASK_H
+
+/*
+ * Task state bitmask. NOTE! These bits are also
+ * encoded in fs/proc/array.c: get_task_state().
+ *
+ * We have two separate sets of flags: task-state
+ * is about runnability, while task-exit_state are
+ * about the task exiting. Confusing, but this way
+ * modifying one set can't modify the other one by
+ * mistake.
+ */
+#define TASK_RUNNING 0
+#define TASK_INTERRUPTIBLE 1
+#define TASK_UNINTERRUPTIBLE 2
+#define __TASK_STOPPED 4
+#define __TASK_TRACED 8
+/* in tsk-exit_state */
+#define EXIT_ZOMBIE16
+#define EXIT_DEAD 32
[PATCH 0/1] Fix linux/wait.h header file
Hi, OMAP2 camera driver compilation is broken due to problems on linux/wait.h header file: drivers/media/video/omap24xxcam.c: In function 'omap24xxcam_vbq_complete': drivers/media/video/omap24xxcam.c:414: error: 'TASK_NORMAL' undeclared (first use in this function) drivers/media/video/omap24xxcam.c:414: error: (Each undeclared identifier is reported only once drivers/media/video/omap24xxcam.c:414: error: for each function it appears in.) make[3]: *** [drivers/media/video/omap24xxcam.o] Error 1 make[2]: *** [drivers/media/video] Error 2 make[1]: *** [drivers/media] Error 2 make: *** [drivers] Error 2 This file defines macros wake_up*() which use TASK_* defined on linux/sched.h. But sched.h cannot be included on wait.h due to a circular dependency between both files. This patch fixes such compilation and the circular dependency problem. Br, David --- David Cohen (1): headers: fix circular dependency between linux/sched.h and linux/wait.h include/linux/sched.h | 61 +- include/linux/task_sched.h | 64 include/linux/wait.h |1 + 3 files changed, 66 insertions(+), 60 deletions(-) create mode 100644 include/linux/task_sched.h -- 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 resend] video: omap24xxcam: Fix compilation
On Mon, Feb 21, 2011 at 9:36 AM, Felipe Balbi ba...@ti.com wrote: Hi, On Sat, Feb 19, 2011 at 06:04:58PM +0200, David Cohen wrote: I have to disagree. The fundamental problem is the circular dependency between those two files: sched.h uses wait_queue_head_t defined in wait.h wait.h uses TASK_* defined in sched.h So, IMO the real fix would be clear out the circular dependency. Maybe introducing linux/task.h to define those TASK_* symbols and include that on sched.h and wait.h Just dig a quick and dirty to try it out and works like a charm We have 2 problems: - omap24xxcam compilation broken - circular dependency between sched.h and wait.h To fix the broken compilation we can do what the rest of the kernel is doing, which is to include sched.h. Then, the circular dependency is fixed by some different approach which would probably change *all* current usage of TASK_*. considering that 1 is caused by 2 I would fix 2. IMO, there's no need to create a dependency between those issues. There's no dependency between them, it's just that the root cause for this problem is a circular dependency between wait.h and sched.h I did a try to fix this circular dependency and the comment I got was to include sched.h in omap24xxcam.c file: http://marc.info/?l=linux-omapm=129828637120270w=2 I'm working to remove v4l2 internal device interface from omap24xxcam and then I need this driver's compilation fixed. The whole kernel is including sched.h when wake_up*() macro is used, so this should be our first solution IMO. As I said earlier, no need to make this compilation fix be dependent of wait.h fix (if it's really going to be changed). I think we should proceed with this patch. Br, David -- balbi -- 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 1/1] headers: fix circular dependency between linux/sched.h and linux/wait.h
On Mon, Feb 21, 2011 at 1:05 PM, Alexey Dobriyan adobri...@gmail.com wrote: On Mon, Feb 21, 2011 at 12:20 PM, David Cohen daco...@gmail.com wrote: Currently sched.h and wait.h have circular dependency between both. wait.h defines macros wake_up*() which use macros TASK_* defined by sched.h. But as sched.h indirectly includes wait.h, such wait.h header file can't include sched.h too. The side effect is when some file includes wait.h and tries to use its wake_up*() macros, it's necessary to include sched.h also. This patch moves all TASK_* macros from linux/sched.h to a new header file linux/task_sched.h. This way, both sched.h and wait.h can include task_sched.h and fix the circular dependency. No need to include sched.h anymore when wake_up*() macros are used. Just include linux/sched.h in your driver. Sounds a reasonable solution for me. This include splitting in small pieces is troublesome as well. But I disagree it's troublesome. It's transparent to everyone else. The only side effect is to not have to include sched.h when using a macro define on wait.h. Why are you moving TASK_COMM_LEN? This one can be moved back to sched.h. Br, David include/linux/sched.h | 61 +- include/linux/task_sched.h | 64 include/linux/wait.h | 1 + 3 files changed, 66 insertions(+), 60 deletions(-) create mode 100644 include/linux/task_sched.h --- a/include/linux/sched.h +++ b/include/linux/sched.h +#include linux/task_sched.h -- 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 1/1] headers: fix circular dependency between linux/sched.h and linux/wait.h
On Mon, Feb 21, 2011 at 3:57 PM, Felipe Balbi ba...@ti.com wrote: Hi, On Mon, Feb 21, 2011 at 03:51:25PM +0200, Alexey Dobriyan wrote: I rather have the split done and kill the circular dependency. It's not circular for starters. how come ? wait.h depends on sched and sched.h depends on wait.h The tricky thing is wait.h doesn't depend on sched.h, but the file which uses wake_up*() macro defined on wait.h will depend on sched.h (what is still bad). wait.h should provide all dependencies to use a macro it defines. I'll send a new version for this patch following the comments I got. Let's see how it looks like. Br, David -- balbi -- 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 v2 0/1] Fix linux/wait.h header file
Hi, OMAP2 camera driver compilation is broken due to problems on linux/wait.h header file: drivers/media/video/omap24xxcam.c: In function 'omap24xxcam_vbq_complete': drivers/media/video/omap24xxcam.c:414: error: 'TASK_NORMAL' undeclared (first use in this function) drivers/media/video/omap24xxcam.c:414: error: (Each undeclared identifier is reported only once drivers/media/video/omap24xxcam.c:414: error: for each function it appears in.) make[3]: *** [drivers/media/video/omap24xxcam.o] Error 1 make[2]: *** [drivers/media/video] Error 2 make[1]: *** [drivers/media] Error 2 make: *** [drivers] Error 2 This file defines macros wake_up*() which use TASK_* defined on linux/sched.h. But sched.h cannot be included on wait.h due to a circular dependency between both files. This patch fixes such compilation and the circular dependency problem. Br, David --- David Cohen (1): headers: fix circular dependency between linux/sched.h and linux/wait.h include/linux/sched.h | 58 +- include/linux/task_state.h | 61 include/linux/wait.h |1 + 3 files changed, 63 insertions(+), 57 deletions(-) create mode 100644 include/linux/task_state.h -- 1.7.2.3 -- 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 v2 1/1] headers: fix circular dependency between linux/sched.h and linux/wait.h
Currently sched.h and wait.h have circular dependency between both.
wait.h defines macros wake_up*() which use macros TASK_* defined by
sched.h. But as sched.h indirectly includes wait.h, such wait.h header
file can't include sched.h too. The side effect is when some file
includes wait.h and tries to use its wake_up*() macros, it's necessary
to include sched.h also.
This patch moves all TASK_* macros from linux/sched.h to a new header
file linux/task_state.h. This way, both sched.h and wait.h can include
task_state.h and fix the circular dependency. No need to include sched.h
anymore when wake_up*() macros are used.
Signed-off-by: David Cohen daco...@gmail.com
---
include/linux/sched.h | 58 +-
include/linux/task_state.h | 61
include/linux/wait.h |1 +
3 files changed, 63 insertions(+), 57 deletions(-)
create mode 100644 include/linux/task_state.h
diff --git a/include/linux/sched.h b/include/linux/sched.h
index d747f94..a75b5ba 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -90,6 +90,7 @@ struct sched_param {
#include linux/task_io_accounting.h
#include linux/latencytop.h
#include linux/cred.h
+#include linux/task_state.h
#include asm/processor.h
@@ -169,63 +170,6 @@ print_cfs_rq(struct seq_file *m, int cpu, struct cfs_rq
*cfs_rq)
#endif
/*
- * Task state bitmask. NOTE! These bits are also
- * encoded in fs/proc/array.c: get_task_state().
- *
- * We have two separate sets of flags: task-state
- * is about runnability, while task-exit_state are
- * about the task exiting. Confusing, but this way
- * modifying one set can't modify the other one by
- * mistake.
- */
-#define TASK_RUNNING 0
-#define TASK_INTERRUPTIBLE 1
-#define TASK_UNINTERRUPTIBLE 2
-#define __TASK_STOPPED 4
-#define __TASK_TRACED 8
-/* in tsk-exit_state */
-#define EXIT_ZOMBIE16
-#define EXIT_DEAD 32
-/* in tsk-state again */
-#define TASK_DEAD 64
-#define TASK_WAKEKILL 128
-#define TASK_WAKING256
-#define TASK_STATE_MAX 512
-
-#define TASK_STATE_TO_CHAR_STR RSDTtZXxKW
-
-extern char ___assert_task_state[1 - 2*!!(
- sizeof(TASK_STATE_TO_CHAR_STR)-1 != ilog2(TASK_STATE_MAX)+1)];
-
-/* Convenience macros for the sake of set_task_state */
-#define TASK_KILLABLE (TASK_WAKEKILL | TASK_UNINTERRUPTIBLE)
-#define TASK_STOPPED (TASK_WAKEKILL | __TASK_STOPPED)
-#define TASK_TRACED(TASK_WAKEKILL | __TASK_TRACED)
-
-/* Convenience macros for the sake of wake_up */
-#define TASK_NORMAL(TASK_INTERRUPTIBLE | TASK_UNINTERRUPTIBLE)
-#define TASK_ALL (TASK_NORMAL | __TASK_STOPPED | __TASK_TRACED)
-
-/* get_task_state() */
-#define TASK_REPORT(TASK_RUNNING | TASK_INTERRUPTIBLE | \
-TASK_UNINTERRUPTIBLE | __TASK_STOPPED | \
-__TASK_TRACED)
-
-#define task_is_traced(task) ((task-state __TASK_TRACED) != 0)
-#define task_is_stopped(task) ((task-state __TASK_STOPPED) != 0)
-#define task_is_dead(task) ((task)-exit_state != 0)
-#define task_is_stopped_or_traced(task)\
- ((task-state (__TASK_STOPPED | __TASK_TRACED)) != 0)
-#define task_contributes_to_load(task) \
- ((task-state TASK_UNINTERRUPTIBLE) != 0 \
-(task-flags PF_FREEZING) == 0)
-
-#define __set_task_state(tsk, state_value) \
- do { (tsk)-state = (state_value); } while (0)
-#define set_task_state(tsk, state_value) \
- set_mb((tsk)-state, (state_value))
-
-/*
* set_current_state() includes a barrier so that the write of current-state
* is correctly serialised wrt the caller's subsequent test of whether to
* actually sleep:
diff --git a/include/linux/task_state.h b/include/linux/task_state.h
new file mode 100644
index 000..36a8db8
--- /dev/null
+++ b/include/linux/task_state.h
@@ -0,0 +1,61 @@
+#ifndef _LINUX_TASK_H
+#define _LINUX_TASK_H
+
+/*
+ * Task state bitmask. NOTE! These bits are also
+ * encoded in fs/proc/array.c: get_task_state().
+ *
+ * We have two separate sets of flags: task-state
+ * is about runnability, while task-exit_state are
+ * about the task exiting. Confusing, but this way
+ * modifying one set can't modify the other one by
+ * mistake.
+ */
+#define TASK_RUNNING 0
+#define TASK_INTERRUPTIBLE 1
+#define TASK_UNINTERRUPTIBLE 2
+#define __TASK_STOPPED 4
+#define __TASK_TRACED 8
+/* in tsk-exit_state */
+#define EXIT_ZOMBIE16
+#define EXIT_DEAD 32
+/* in tsk-state again */
+#define TASK_DEAD 64
+#define TASK_WAKEKILL 128
+#define TASK_WAKING256
+#define TASK_STATE_MAX 512
+
+#define TASK_STATE_TO_CHAR_STR RSDTtZXxKW
+
+extern char ___assert_task_state[1 - 2
Re: [PATCH v2 1/1] headers: fix circular dependency between linux/sched.h and linux/wait.h
On Mon, Feb 21, 2011 at 5:54 PM, Peter Zijlstra pet...@infradead.org wrote: On Mon, 2011-02-21 at 16:38 +0200, David Cohen wrote: Currently sched.h and wait.h have circular dependency between both. wait.h defines macros wake_up*() which use macros TASK_* defined by sched.h. But as sched.h indirectly includes wait.h, such wait.h header file can't include sched.h too. The side effect is when some file includes wait.h and tries to use its wake_up*() macros, it's necessary to include sched.h also. This patch moves all TASK_* macros from linux/sched.h to a new header file linux/task_state.h. This way, both sched.h and wait.h can include task_state.h and fix the circular dependency. No need to include sched.h anymore when wake_up*() macros are used. I think Alexey already told you what you done wrong. Also, I really don't like the task_state.h header, it assumes a lot of things it doesn't include itself and only works because its using macros and not inlines at it probably should. Like wait.h I'd say. The main issue is wait.h uses TASK_* macros but cannot properly include sched.h as it would create a circular dependency. So a file including wait.h is able to compile because the dependency of sched.h relies on wake_up*() macros and it's not always used. We can still drop everything else from task_state.h but the TASK_* macros and then the problem you just pointed out won't exist anymore. What do you think about it? Br, David -- 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: [RFC/PATCH 0/1] Get rid of V4L2 internal device interface usage
On Sun, Feb 20, 2011 at 10:47 AM, Hans Verkuil hverk...@xs4all.nl wrote: Hi David, Hi Hans, On Saturday, February 19, 2011 17:35:46 David Cohen wrote: Hi, This is the first patch (set) version to remove V4L2 internal device interface. I have converted tcm825x VGA sensor to V4L2 sub device interface. I removed also some workarounds in the driver which doesn't fit anymore in its new interface. Very nice! It looks good. I noticed that you didn't convert it to the control framework yet, but after looking at the controls I think that it is probably better if I do that anyway. There are several private controls in this driver, and I will need to take a good look at those. Yes, to port to control fw is not part of this task yet. IMO there are plenty of missing tasks to let the driver in a good shape and it may need a very good rework. For now I'm focusing in remove v4l2 internal interface. TODO: - Remove V4L2 int device interface from omap24xxcam driver. - Define a new interface to handle xclk. OMAP3 ISP could be used as base. - Use some base platform (probably N8X0) to add board code and test them. - Remove V4L2 int device. :) It would be so nice to have that API removed :-) Yes. :) Br, David Regards, Hans Br, David --- David Cohen (1): tcm825x: convert driver to V4L2 sub device interface drivers/media/video/tcm825x.c | 369 - drivers/media/video/tcm825x.h | 6 +- 2 files changed, 109 insertions(+), 266 deletions(-) -- 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 -- Hans Verkuil - video4linux developer - sponsored by Cisco -- 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 resend] video: omap24xxcam: Fix compilation
Hi Sakari and Felipe, On Tue, Feb 15, 2011 at 2:17 PM, Sakari Ailus sakari.ai...@maxwell.research.nokia.com wrote: Felipe Balbi wrote: Hi, On Tue, Feb 15, 2011 at 12:50:12PM +0100, Thomas Weber wrote: Hello Felipe, in include/linux/wait.h #define wake_up(x) __wake_up(x, TASK_NORMAL, 1, NULL) aha, now I get it, so shouldn't the real fix be including linux/sched.h on linux/wait.h, I mean, it's linuux/wait.h who uses a symbol defined in linux/sched.h, right ? That's a tricky situation. linux/sched.h includes indirectly linux/completion.h which includes linux/wait.h. By including sched.h in wait.h, the side effect is completion.h will then include a blank wait.h file and trigger a compilation error every time wait.h is included by any file. Surprisingly many other files still don't seem to be affected. But this is actually a better solution (to include sched.h in wait.h). It does not affect all files include wait.h because TASK_* macros are used with #define statements only. So it has no effect unless some file tries to use a macro which used TASK_*. It seems the usual on kernel is to include both wait.h and sched.h when necessary. IMO your patch is fine. Br, David -- Sakari Ailus sakari.ai...@maxwell.research.nokia.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ -- 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 resend] video: omap24xxcam: Fix compilation
On Sat, Feb 19, 2011 at 5:00 PM, Felipe Balbi ba...@ti.com wrote: Hi, On Sat, Feb 19, 2011 at 01:35:09PM +0200, David Cohen wrote: aha, now I get it, so shouldn't the real fix be including linux/sched.h on linux/wait.h, I mean, it's linuux/wait.h who uses a symbol defined in linux/sched.h, right ? That's a tricky situation. linux/sched.h includes indirectly linux/completion.h which includes linux/wait.h. Ok, so the real problem is that there is circular dependency between linux/sched.h and linux/wait.h By including sched.h in wait.h, the side effect is completion.h will then include a blank wait.h file and trigger a compilation error every time wait.h is included by any file. true, but the real problem is the circular dependency between those files. Surprisingly many other files still don't seem to be affected. But this is actually a better solution (to include sched.h in wait.h). It does not affect all files include wait.h because TASK_* macros are used with #define statements only. So it has no effect unless some file tries to use a macro which used TASK_*. It seems the usual on kernel is to include both wait.h and sched.h when necessary. IMO your patch is fine. I have to disagree. The fundamental problem is the circular dependency between those two files: sched.h uses wait_queue_head_t defined in wait.h wait.h uses TASK_* defined in sched.h So, IMO the real fix would be clear out the circular dependency. Maybe introducing linux/task.h to define those TASK_* symbols and include that on sched.h and wait.h Just dig a quick and dirty to try it out and works like a charm We have 2 problems: - omap24xxcam compilation broken - circular dependency between sched.h and wait.h To fix the broken compilation we can do what the rest of the kernel is doing, which is to include sched.h. Then, the circular dependency is fixed by some different approach which would probably change *all* current usage of TASK_*. IMO, there's no need to create a dependency between those issues. Br, David -- balbi -- 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
[RFC/PATCH 0/1] Get rid of V4L2 internal device interface usage
Hi, This is the first patch (set) version to remove V4L2 internal device interface. I have converted tcm825x VGA sensor to V4L2 sub device interface. I removed also some workarounds in the driver which doesn't fit anymore in its new interface. TODO: - Remove V4L2 int device interface from omap24xxcam driver. - Define a new interface to handle xclk. OMAP3 ISP could be used as base. - Use some base platform (probably N8X0) to add board code and test them. - Remove V4L2 int device. :) Br, David --- David Cohen (1): tcm825x: convert driver to V4L2 sub device interface drivers/media/video/tcm825x.c | 369 - drivers/media/video/tcm825x.h |6 +- 2 files changed, 109 insertions(+), 266 deletions(-) -- 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
[RFC/PATCH 1/1] tcm825x: convert driver to V4L2 sub device interface
This patch converts Toshiba TCM825X VGA camera sersor driver to V4L2 sub
device interface.
Signed-off-by: David Cohen daco...@gmail.com
---
drivers/media/video/tcm825x.c | 369 -
drivers/media/video/tcm825x.h |6 +-
2 files changed, 109 insertions(+), 266 deletions(-)
diff --git a/drivers/media/video/tcm825x.c b/drivers/media/video/tcm825x.c
index 54681a5..36080f1 100644
--- a/drivers/media/video/tcm825x.c
+++ b/drivers/media/video/tcm825x.c
@@ -7,7 +7,7 @@
*
* Contact: Sakari Ailus sakari.ai...@nokia.com
*
- * Based on code from David Cohen david.co...@indt.org.br
+ * Based on code from David Cohen daco...@gmail.com
*
* This driver was based on ov9640 sensor driver from MontaVista
*
@@ -27,7 +27,8 @@
*/
#include linux/i2c.h
-#include media/v4l2-int-device.h
+#include media/v4l2-device.h
+#include media/v4l2-mediabus.h
#include tcm825x.h
@@ -41,37 +42,21 @@
#define HIGH_FPS_MODE_LOWER_LIMIT 14
#define DEFAULT_FPS MAX_HALF_FPS
+#define to_tcm825x_sensor(sd) container_of(sd, struct tcm825x_sensor, subdev)
+
struct tcm825x_sensor {
const struct tcm825x_platform_data *platform_data;
- struct v4l2_int_device *v4l2_int_device;
- struct i2c_client *i2c_client;
- struct v4l2_pix_format pix;
+ struct v4l2_subdev subdev;
+ struct v4l2_mbus_framefmt mf;
struct v4l2_fract timeperframe;
};
/* list of image formats supported by TCM825X sensor */
-static const struct v4l2_fmtdesc tcm825x_formats[] = {
- {
- .description = YUYV (YUV 4:2:2), packed,
- .pixelformat = V4L2_PIX_FMT_UYVY,
- }, {
- /* Note: V4L2 defines RGB565 as:
-*
-* Byte 0Byte 1
-* g2 g1 g0 r4 r3 r2 r1 r0 b4 b3 b2 b1 b0 g5 g4 g3
-*
-* We interpret RGB565 as:
-*
-* Byte 0Byte 1
-* g2 g1 g0 b4 b3 b2 b1 b0 r4 r3 r2 r1 r0 g5 g4 g3
-*/
- .description = RGB565, le,
- .pixelformat = V4L2_PIX_FMT_RGB565,
- },
+static const enum v4l2_mbus_pixelcode tcm825x_codes[] = {
+ V4L2_MBUS_FMT_UYVY8_2X8,
+ V4L2_MBUS_FMT_RGB565_2X8_LE,
};
-#define TCM825X_NUM_CAPTURE_FORMATSARRAY_SIZE(tcm825x_formats)
-
/*
* TCM825X register configuration for all combinations of pixel format and
* image size
@@ -382,24 +367,24 @@ static struct vcontrol *find_vctrl(int id)
* as the requested size, or the smallest image size if the requested size
* has fewer pixels than the smallest image.
*/
-static enum image_size tcm825x_find_size(struct v4l2_int_device *s,
+static enum image_size tcm825x_find_size(struct v4l2_subdev *sd,
unsigned int width,
unsigned int height)
{
+ struct i2c_client *client = v4l2_get_subdevdata(sd);
enum image_size isize;
unsigned long pixels = width * height;
- struct tcm825x_sensor *sensor = s-priv;
for (isize = subQCIF; isize VGA; isize++) {
if (tcm825x_sizes[isize + 1].height
* tcm825x_sizes[isize + 1].width pixels) {
- dev_dbg(sensor-i2c_client-dev, size %d\n, isize);
+ dev_dbg(client-dev, size %d\n, isize);
return isize;
}
}
- dev_dbg(sensor-i2c_client-dev, format default VGA\n);
+ dev_dbg(client-dev, format default VGA\n);
return VGA;
}
@@ -410,11 +395,12 @@ static enum image_size tcm825x_find_size(struct
v4l2_int_device *s,
* fraction. Returns zero if successful, or non-zero otherwise. The
* actual frame period is returned in fper.
*/
-static int tcm825x_configure(struct v4l2_int_device *s)
+static int tcm825x_configure(struct v4l2_subdev *sd)
{
- struct tcm825x_sensor *sensor = s-priv;
- struct v4l2_pix_format *pix = sensor-pix;
- enum image_size isize = tcm825x_find_size(s, pix-width, pix-height);
+ struct i2c_client *client = v4l2_get_subdevdata(sd);
+ struct tcm825x_sensor *sensor = to_tcm825x_sensor(sd);
+ struct v4l2_mbus_framefmt *mf = sensor-mf;
+ enum image_size isize = tcm825x_find_size(sd, mf-width, mf-height);
struct v4l2_fract *fper = sensor-timeperframe;
enum pixel_format pfmt;
int err;
@@ -422,37 +408,32 @@ static int tcm825x_configure(struct v4l2_int_device *s)
u8 val;
/* common register initialization */
- err = tcm825x_write_default_regs(
- sensor-i2c_client, sensor-platform_data-default_regs());
+ err = tcm825x_write_default_regs(client,
+sensor-platform_data-default_regs());
if (err)
return err;
/* configure image size */
val
Re: [PATCH v6 04/10] omap2: Fix camera resources for multiomap
On Mon, Feb 14, 2011 at 3:50 PM, Laurent Pinchart
laurent.pinch...@ideasonboard.com wrote:
Hi Felipe,
Hello,
On Monday 14 February 2011 14:41:16 Felipe Balbi wrote:
On Mon, Feb 14, 2011 at 02:19:24PM +0100, Laurent Pinchart wrote:
On Monday 14 February 2011 13:35:59 Felipe Balbi wrote:
On Mon, Feb 14, 2011 at 01:21:31PM +0100, Laurent Pinchart wrote:
diff --git a/arch/arm/mach-omap2/devices.c
b/arch/arm/mach-omap2/devices.c index 4cf48ea..5d844bd 100644
--- a/arch/arm/mach-omap2/devices.c
+++ b/arch/arm/mach-omap2/devices.c
@@ -38,7 +38,7 @@
#if defined(CONFIG_VIDEO_OMAP2) ||
defined(CONFIG_VIDEO_OMAP2_MODULE)
-static struct resource cam_resources[] = {
+static struct resource omap2cam_resources[] = {
should this be __initdata ??
The resources will be used when the OMAP3 ISP module is loaded. Won't
they be discared if marked as __initdata ?
I believe driver core makes a copy of those, no ? not sure.
Not that I know of, but I may be wrong.
I don't think omap2cam_resources would be used at all.
AFAIK, it belongs to omap2xxcam, isn't it? :)
Br,
David
--
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 v6 03/10] omap3: Add function to register omap3isp platform device structure
On Mon, Feb 14, 2011 at 3:18 PM, Felipe Balbi ba...@ti.com wrote: Hi, Hi On Mon, Feb 14, 2011 at 02:07:08PM +0100, Laurent Pinchart wrote: diff --git a/arch/arm/mach-omap2/devices.h b/arch/arm/mach-omap2/devices.h new file mode 100644 index 000..12ddb8a --- /dev/null +++ b/arch/arm/mach-omap2/devices.h @@ -0,0 +1,17 @@ +/* + * arch/arm/mach-omap2/devices.h + * + * OMAP2 platform device setup/initialization + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + */ + +#ifndef __ARCH_ARM_MACH_OMAP_DEVICES_H +#define __ARCH_ARM_MACH_OMAP_DEVICES_H + +int omap3_init_camera(void *pdata); missing extern ? Is that mandatory ? Many (most ?) headers in the kernel don't use the extern keyword when declaring functions. maybe not mandatory, worth checking what sparse would say though :-p sparse is not complaining. Br, David -- balbi -- -- 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 v3 0/2] OmniVision OV9640 sensor driver cleanup and fix
Hi, These patches apply some cleanups and fix OV9640 driver's priv data retrieving on I2C remove and video_probe functions. I included in this new version one missing fix pointed out by Guennadi Liakhovetski. Despite the patches are simple, they haven't been tested yet due to lack of hw. These patches are intended to 2.6.38. Br, David Cohen --- David Cohen (2): ov9640: use macro to request OmniVision OV9640 sensor private data ov9640: fix OmniVision OV9640 sensor driver's priv data retrieving drivers/media/video/ov9640.c | 19 +-- 1 files changed, 9 insertions(+), 10 deletions(-) -- 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 v3 1/2] ov9640: use macro to request OmniVision OV9640 sensor private data
This cleanup patch creates macro to request OmniVision OV9640 private
data, which increases readability.
Signed-off-by: David Cohen daco...@gmail.com
---
drivers/media/video/ov9640.c | 13 +
1 files changed, 5 insertions(+), 8 deletions(-)
diff --git a/drivers/media/video/ov9640.c b/drivers/media/video/ov9640.c
index 99e9e1d..c6d8e8a 100644
--- a/drivers/media/video/ov9640.c
+++ b/drivers/media/video/ov9640.c
@@ -31,6 +31,8 @@
#include ov9640.h
+#define to_ov9640_sensor(sd) container_of(sd, struct ov9640_priv, subdev)
+
/* default register setup */
static const struct ov9640_reg ov9640_regs_dflt[] = {
{ OV9640_COM5, OV9640_COM5_SYSCLK | OV9640_COM5_LONGEXP },
@@ -308,9 +310,7 @@ static unsigned long ov9640_query_bus_param(struct
soc_camera_device *icd)
/* Get status of additional camera capabilities */
static int ov9640_g_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
{
- struct i2c_client *client = v4l2_get_subdevdata(sd);
- struct ov9640_priv *priv = container_of(i2c_get_clientdata(client),
- struct ov9640_priv, subdev);
+ struct ov9640_priv *priv = to_ov9640_sensor(sd);
switch (ctrl-id) {
case V4L2_CID_VFLIP:
@@ -327,8 +327,7 @@ static int ov9640_g_ctrl(struct v4l2_subdev *sd, struct
v4l2_control *ctrl)
static int ov9640_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
{
struct i2c_client *client = v4l2_get_subdevdata(sd);
- struct ov9640_priv *priv = container_of(i2c_get_clientdata(client),
- struct ov9640_priv, subdev);
+ struct ov9640_priv *priv = to_ov9640_sensor(sd);
int ret = 0;
@@ -360,9 +359,7 @@ static int ov9640_s_ctrl(struct v4l2_subdev *sd, struct
v4l2_control *ctrl)
static int ov9640_g_chip_ident(struct v4l2_subdev *sd,
struct v4l2_dbg_chip_ident *id)
{
- struct i2c_client *client = v4l2_get_subdevdata(sd);
- struct ov9640_priv *priv = container_of(i2c_get_clientdata(client),
- struct ov9640_priv, subdev);
+ struct ov9640_priv *priv = to_ov9640_sensor(sd);
id-ident = priv-model;
id-revision= priv-revision;
--
1.7.1
--
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 v3 2/2] ov9640: fix OmniVision OV9640 sensor driver's priv data retrieving
OmniVision OV9640 driver wasn't requesting properly its private data
on I2C remove and video_probe functions. It was retrieving the V4L2
subdev struct address instead of priv struct's one. This patch fixes
such problem.
Signed-off-by: David Cohen daco...@gmail.com
---
drivers/media/video/ov9640.c |6 --
1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/media/video/ov9640.c b/drivers/media/video/ov9640.c
index c6d8e8a..53d88a2 100644
--- a/drivers/media/video/ov9640.c
+++ b/drivers/media/video/ov9640.c
@@ -651,7 +651,8 @@ static int ov9640_cropcap(struct v4l2_subdev *sd, struct
v4l2_cropcap *a)
static int ov9640_video_probe(struct soc_camera_device *icd,
struct i2c_client *client)
{
- struct ov9640_priv *priv = i2c_get_clientdata(client);
+ struct v4l2_subdev *sd = i2c_get_clientdata(client);
+ struct ov9640_priv *priv = to_ov9640_sensor(sd);
u8 pid, ver, midh, midl;
const char *devname;
int ret = 0;
@@ -788,7 +789,8 @@ static int ov9640_probe(struct i2c_client *client,
static int ov9640_remove(struct i2c_client *client)
{
- struct ov9640_priv *priv = i2c_get_clientdata(client);
+ struct v4l2_subdev *sd = i2c_get_clientdata(client);
+ struct ov9640_priv *priv = to_ov9640_sensor(sd);
kfree(priv);
return 0;
--
1.7.1
--
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 2/2] ov9640: fix OmniVision OV9640 sensor driver's I2C remove function
Hi,
On Tue, Dec 7, 2010 at 11:52 PM, Guennadi Liakhovetski
g.liakhovet...@gmx.de wrote:
Hi David
On Fri, 3 Dec 2010, David Cohen wrote:
OmniVision OV9640 driver wasn't deallocating properly its private data
as it was requesting the V4L2 subdev struct address instead of the priv
struct's one. This patch fixes such problem.
This is a bug-fix, that should go into 2.6.37, but it depends on your
previous patch, adding to_ov9640_sensor(), which isn't a fix, and so has
to wait until 2.6.38. Could you please respin this patch without that
macro and resend?
Sure. I'll resend it.
Regards,
David Cohen
Thanks
Guennadi
Signed-off-by: David Cohen daco...@gmail.com
---
drivers/media/video/ov9640.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/drivers/media/video/ov9640.c b/drivers/media/video/ov9640.c
index c6d8e8a..7628754 100644
--- a/drivers/media/video/ov9640.c
+++ b/drivers/media/video/ov9640.c
@@ -788,7 +788,8 @@ static int ov9640_probe(struct i2c_client *client,
static int ov9640_remove(struct i2c_client *client)
{
- struct ov9640_priv *priv = i2c_get_clientdata(client);
+ struct v4l2_subdev *sd = i2c_get_clientdata(client);
+ struct ov9640_priv *priv = to_ov9640_sensor(sd);
kfree(priv);
return 0;
--
1.7.1
--
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
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
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 v2] ov9640: fix OmniVision OV9640 sensor driver's I2C remove function
OmniVision OV9640 driver wasn't deallocating properly its private data
as it was requesting the V4L2 subdev struct address instead of the priv
struct's one. This patch fixes such problem.
Signed-off-by: David Cohen daco...@gmail.com
---
drivers/media/video/ov9640.c |3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/drivers/media/video/ov9640.c b/drivers/media/video/ov9640.c
index 99e9e1d..1315696 100644
--- a/drivers/media/video/ov9640.c
+++ b/drivers/media/video/ov9640.c
@@ -791,7 +791,8 @@ static int ov9640_probe(struct i2c_client *client,
static int ov9640_remove(struct i2c_client *client)
{
- struct ov9640_priv *priv = i2c_get_clientdata(client);
+ struct v4l2_subdev *sd = i2c_get_clientdata(client);
+ struct ov9640_priv *priv = container_of(sd, struct ov9640_priv, subdev);
kfree(priv);
return 0;
--
1.7.1
--
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 v2] ov9640: fix OmniVision OV9640 sensor driver's I2C remove function
On Wed, Dec 8, 2010 at 1:15 AM, Guennadi Liakhovetski
g.liakhovet...@gmx.de wrote:
On Wed, 8 Dec 2010, David Cohen wrote:
OmniVision OV9640 driver wasn't deallocating properly its private data
as it was requesting the V4L2 subdev struct address instead of the priv
struct's one. This patch fixes such problem.
Signed-off-by: David Cohen daco...@gmail.com
---
drivers/media/video/ov9640.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/drivers/media/video/ov9640.c b/drivers/media/video/ov9640.c
index 99e9e1d..1315696 100644
--- a/drivers/media/video/ov9640.c
+++ b/drivers/media/video/ov9640.c
@@ -791,7 +791,8 @@ static int ov9640_probe(struct i2c_client *client,
static int ov9640_remove(struct i2c_client *client)
{
- struct ov9640_priv *priv = i2c_get_clientdata(client);
+ struct v4l2_subdev *sd = i2c_get_clientdata(client);
+ struct ov9640_priv *priv = container_of(sd, struct ov9640_priv,
subdev);
Ok, this doesn't crash, because subdev is currently the first member of
struct ov9640_priv, so, this patch only fixes an academic problem. But
even then it solves it incompletely:
static int ov9640_video_probe(struct soc_camera_device *icd,
struct i2c_client *client)
{
struct ov9640_priv *priv = i2c_get_clientdata(client);
True. I missed that one.
So, sorry for changing my mind so quickly... We either need a complete
patch, fixing both these locations, or we leave it as is until 2.6.38.
TBH, I'd prefer the latter, what do you think? Interestingly, you also
don't fix this location in v1 of your patches. So, I suggest, we make a
proper fix and schedule it for 2.6.38.
IMO we can schedule it for 2.6.38 and release a proper fix. The first
patch is also very welcome as the driver is recovering priv data
through something like this:
static int ov9640_g_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
{
struct i2c_client *client = v4l2_get_subdevdata(sd);
struct ov9640_priv *priv = container_of(i2c_get_clientdata(client),
struct ov9640_priv, subdev);
but i2c_get_clientdata(client) == sd, which is already an argument.
Br,
David
kfree(priv);
return 0;
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
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 0/2] OmniVision OV9640 sensor driver cleanup and fix
Hi, These patches fix OV9640 driver's I2C remove function. Despite the patches are simple, they haven't been tested yet as I don't have the hw. Br, David Cohen --- David Cohen (2): ov9640: use macro to request OmniVision OV9640 sensor private data ov9640: fix OmniVision OV9640 sensor driver's I2C remove function drivers/media/video/ov9640.c | 16 +++- 1 files changed, 7 insertions(+), 9 deletions(-) -- 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 1/2] ov9640: use macro to request OmniVision OV9640 sensor private data
This cleanup patch creates macro to request OmniVision OV9640 private
data, which increases readability.
Signed-off-by: David Cohen daco...@gmail.com
---
drivers/media/video/ov9640.c | 13 +
1 files changed, 5 insertions(+), 8 deletions(-)
diff --git a/drivers/media/video/ov9640.c b/drivers/media/video/ov9640.c
index 99e9e1d..c6d8e8a 100644
--- a/drivers/media/video/ov9640.c
+++ b/drivers/media/video/ov9640.c
@@ -31,6 +31,8 @@
#include ov9640.h
+#define to_ov9640_sensor(sd) container_of(sd, struct ov9640_priv, subdev)
+
/* default register setup */
static const struct ov9640_reg ov9640_regs_dflt[] = {
{ OV9640_COM5, OV9640_COM5_SYSCLK | OV9640_COM5_LONGEXP },
@@ -308,9 +310,7 @@ static unsigned long ov9640_query_bus_param(struct
soc_camera_device *icd)
/* Get status of additional camera capabilities */
static int ov9640_g_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
{
- struct i2c_client *client = v4l2_get_subdevdata(sd);
- struct ov9640_priv *priv = container_of(i2c_get_clientdata(client),
- struct ov9640_priv, subdev);
+ struct ov9640_priv *priv = to_ov9640_sensor(sd);
switch (ctrl-id) {
case V4L2_CID_VFLIP:
@@ -327,8 +327,7 @@ static int ov9640_g_ctrl(struct v4l2_subdev *sd, struct
v4l2_control *ctrl)
static int ov9640_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
{
struct i2c_client *client = v4l2_get_subdevdata(sd);
- struct ov9640_priv *priv = container_of(i2c_get_clientdata(client),
- struct ov9640_priv, subdev);
+ struct ov9640_priv *priv = to_ov9640_sensor(sd);
int ret = 0;
@@ -360,9 +359,7 @@ static int ov9640_s_ctrl(struct v4l2_subdev *sd, struct
v4l2_control *ctrl)
static int ov9640_g_chip_ident(struct v4l2_subdev *sd,
struct v4l2_dbg_chip_ident *id)
{
- struct i2c_client *client = v4l2_get_subdevdata(sd);
- struct ov9640_priv *priv = container_of(i2c_get_clientdata(client),
- struct ov9640_priv, subdev);
+ struct ov9640_priv *priv = to_ov9640_sensor(sd);
id-ident = priv-model;
id-revision= priv-revision;
--
1.7.1
--
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 2/2] ov9640: fix OmniVision OV9640 sensor driver's I2C remove function
OmniVision OV9640 driver wasn't deallocating properly its private data
as it was requesting the V4L2 subdev struct address instead of the priv
struct's one. This patch fixes such problem.
Signed-off-by: David Cohen daco...@gmail.com
---
drivers/media/video/ov9640.c |3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/drivers/media/video/ov9640.c b/drivers/media/video/ov9640.c
index c6d8e8a..7628754 100644
--- a/drivers/media/video/ov9640.c
+++ b/drivers/media/video/ov9640.c
@@ -788,7 +788,8 @@ static int ov9640_probe(struct i2c_client *client,
static int ov9640_remove(struct i2c_client *client)
{
- struct ov9640_priv *priv = i2c_get_clientdata(client);
+ struct v4l2_subdev *sd = i2c_get_clientdata(client);
+ struct ov9640_priv *priv = to_ov9640_sensor(sd);
kfree(priv);
return 0;
--
1.7.1
--
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: [omap3isp RFC][RESEND PATCH v2 0/4] Improve inter subdev interaction
Hi Sergio, On Mon, Nov 22, 2010 at 08:53:53PM +0100, ext Sergio Aguirre wrote: (Resending...) Hi, These are some patches to make these operations more generic: - Clock control is being controlled in a very crude manner by subdevices, it should be centralized in isp.c. - LSC prefetch wait check is reading a main ISP register, so move it to isp.c - Abstract SBL busy check: we don't want a submodule thinkering with main ISP registers. That should be done in the main isp.c Also, remove main ISP register dump from CSI2 specific dump. We should be using isp_print_status if we'll like to know main ISP regdump. Comments are welcome. More cleanups for better subdevice isolation are on the way. Regards, Sergio Changelog: v2: - Improved logic in isp subdevs clock control (Thanks David Cohen) - Renamed ispccdc_lsc_wait_prefetch - isp_ccdc_* to be clear on function declaration new location (isp.c) (Thanks David and Laurent) This version looks good for me. Regards, David Cohen v1: - Initial version Sergio Aguirre (4): omap3isp: Abstract isp subdevs clock control omap3isp: Move CCDC LSC prefetch wait to main isp code omap3isp: sbl: Abstract SBL busy check omap3isp: csi2: Don't dump ISP main registers drivers/media/video/isp/isp.c| 87 ++ drivers/media/video/isp/isp.h| 16 ++ drivers/media/video/isp/ispccdc.c| 42 ++--- drivers/media/video/isp/ispcsi2.c|7 --- drivers/media/video/isp/isppreview.c |6 +-- drivers/media/video/isp/ispresizer.c |6 +-- 6 files changed, 111 insertions(+), 53 deletions(-) -- 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: [omap3isp][PATCH v2 8/9] omap3isp: ccp2: Make SYSCONFIG fields consistent
On Mon, Nov 22, 2010 at 04:19:42PM +0100, ext Aguirre, Sergio wrote: Hi, -Original Message- From: David Cohen [mailto:david.co...@nokia.com] Sent: Saturday, November 20, 2010 4:49 AM To: ext Laurent Pinchart Cc: Aguirre, Sergio; linux-media@vger.kernel.org Subject: Re: [omap3isp][PATCH v2 8/9] omap3isp: ccp2: Make SYSCONFIG fields consistent On Sat, Nov 20, 2010 at 11:34:04AM +0100, ext Laurent Pinchart wrote: Hi, On Saturday 20 November 2010 11:00:30 David Cohen wrote: On Sat, Nov 20, 2010 at 12:10:11AM +0100, ext Aguirre, Sergio wrote: On Friday, November 19, 2010 9:44 AM Aguirre, Sergio wrote: On Friday, November 19, 2010 4:06 AM David Cohen wrote: On Mon, Nov 15, 2010 at 03:30:00PM +0100, ext Sergio Aguirre wrote: Signed-off-by: Sergio Aguirre saagui...@ti.com --- drivers/media/video/isp/ispccp2.c |3 +-- drivers/media/video/isp/ispreg.h | 14 -- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/drivers/media/video/isp/ispccp2.c b/drivers/media/video/isp/ispccp2.c index fa23394..3127a74 100644 --- a/drivers/media/video/isp/ispccp2.c +++ b/drivers/media/video/isp/ispccp2.c @@ -419,8 +419,7 @@ static void ispccp2_mem_configure(struct isp_ccp2_device *ccp2, config-src_ofst = 0; } - isp_reg_writel(isp, (ISPCSI1_MIDLEMODE_SMARTSTANDBY - ISPCSI1_MIDLEMODE_SHIFT), + isp_reg_writel(isp, ISPCCP2_SYSCONFIG_MSTANDBY_MODE_SMART, OMAP3_ISP_IOMEM_CCP2, ISPCCP2_SYSCONFIG); To make your cleanup even better, you could change isp_reg_clr_set() instead. If CCP2 MSTANDY_MODE is set to NOSTANDY, the result is going to be an invalid 0x3 value. Despite it cannot happen with the current code, it's still better to avoid such situations for future versions. :) Hmm you're right, I guess I didn't do any real functional changes, just defines renaming. I can repost an updated patch with this suggestion. No problem. David, Geez I guess I wasn't paying much attention to this either. Sorry. The case you mention about it potentially become 0x3 is not possible, because, I'm basically overwriting the whole register with (0x2 12) Notice the isp_reg_write, there's no OR operation... That's correct. My mistake. IMO ISP power settings still need a better way to be setup, but it definitively does not belong to your cleanup patch. I'm fine with your changes. Now, this means that we have been implicitly setting other fields as Zeroes (SOFT_RESET = 0, and AUTO_IDLE = 0) aswell. Has AUTOIDLE in CCP2 been tried in N900? I don't have a CCP2 sensor to test with :(. I have no idea. Indeed, AUTOIDLE in ISP doesn't seem to be much reliable so far. It should be set to 0 until somebody proves it can be set to 1 :) To summarize things, we're only setting the CCP2 AUTOIDLE bit on ES 1.0 devices, and we're clearing it anyway ispccp2_mem_configure(). For the sake of correctness we should replace isp_reg_writel() by isp_reg_clr_set() in ispccp2_mem_configure(), which will only make a difference on ES 1.0 devices that have basically no users. Is that OK with both of you ? Yes, It's fine with me. I'll say that if this doesn't change things for N900, which appears to be the only community available device (I know of) that uses CCP2 cameras, then we shouldn't care. If somebody comes up with an ES1.0 in the future, and he does see problems, then we will address that. But that would be a very weird case. In fact, it will be most probably someone from TI with a hidden board buried in some box of scraps, which happens to have hacked the board for a CCP2 camera... The more I think about it, the more I'm getting convinced this shouldn't be a worry. :) You're true. Maybe we could rework in future part of the driver's PM. I'm fine with this patch. :) I only suggested something to make it a little better. But as AUTOIDLE is not being set anyway, the result is the same. Br, David Regards, Sergio I'm fine with both solutions. IMO power settings could be improved and not be hardcoded. It could come from platform data. But that's another case. :) Br, David -- 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 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord
Re: [omap3isp RFC][PATCH 0/4] Improve inter subdev interaction
Hi Laurent and Sergio, On Mon, Nov 22, 2010 at 04:31:09PM +0100, ext Aguirre, Sergio wrote: -Original Message- From: Laurent Pinchart [mailto:laurent.pinch...@ideasonboard.com] Sent: Saturday, November 20, 2010 11:31 AM To: David Cohen Cc: Aguirre, Sergio; linux-media@vger.kernel.org Subject: Re: [omap3isp RFC][PATCH 0/4] Improve inter subdev interaction Hi David, On Saturday 20 November 2010 12:44:27 David Cohen wrote: On Sat, Nov 20, 2010 at 12:23:47AM +0100, ext Sergio Aguirre wrote: Hi, These are some patches to make these operations more generic: - Clock control is being controlled in a very crude manner by subdevices, it should be centralized in isp.c. - LSC prefetch wait check is reading a main ISP register, so move it to isp.c - Abstract SBL busy check: we don't want a submodule thinkering with main ISP registers. That should be done in the main isp.c Also, remove main ISP register dump from CSI2 specific dump. We should be using isp_print_status if we'll like to know main ISP regdump. Comments are welcome. More cleanups for better subdevice isolation are on the way. Your patches are fine for me. I sent you some comments, but they are opitional and it's up to you to decide what to do. :) You can copy linux-omap@ as well in future patches. David and Laurent, I appreciate a lot your review time and comments. I will try to submit the next version of the omap3isp driver for upstream review, either at the end of the weekend or on Monday. I will cross-post the driver to linux-media, linux-omap and LKML. Let's be ready to defend the media controller and the omap3isp driver :-) Yeah! :) Until then let's not spam linux-omap with patches for a driver they don't know about. Agreed. Me too. :) Br, David Cohen Regards, Sergio -- 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: [omap3isp][PATCH v2 8/9] omap3isp: ccp2: Make SYSCONFIG fields consistent
On Sat, Nov 20, 2010 at 12:10:11AM +0100, ext Aguirre, Sergio wrote: -Original Message- From: Aguirre, Sergio Sent: Friday, November 19, 2010 9:44 AM To: 'David Cohen' Cc: Laurent Pinchart; linux-media@vger.kernel.org Subject: RE: [omap3isp][PATCH v2 8/9] omap3isp: ccp2: Make SYSCONFIG fields consistent -Original Message- From: David Cohen [mailto:david.co...@nokia.com] Sent: Friday, November 19, 2010 4:06 AM To: Aguirre, Sergio Cc: Laurent Pinchart; linux-media@vger.kernel.org Subject: Re: [omap3isp][PATCH v2 8/9] omap3isp: ccp2: Make SYSCONFIG fields consistent Hi Sergio, Hi David, Thanks for the review. I've few comments below. On Mon, Nov 15, 2010 at 03:30:00PM +0100, ext Sergio Aguirre wrote: Signed-off-by: Sergio Aguirre saagui...@ti.com --- drivers/media/video/isp/ispccp2.c |3 +-- drivers/media/video/isp/ispreg.h | 14 -- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/drivers/media/video/isp/ispccp2.c b/drivers/media/video/isp/ispccp2.c index fa23394..3127a74 100644 --- a/drivers/media/video/isp/ispccp2.c +++ b/drivers/media/video/isp/ispccp2.c @@ -419,8 +419,7 @@ static void ispccp2_mem_configure(struct isp_ccp2_device *ccp2, config-src_ofst = 0; } - isp_reg_writel(isp, (ISPCSI1_MIDLEMODE_SMARTSTANDBY - ISPCSI1_MIDLEMODE_SHIFT), + isp_reg_writel(isp, ISPCCP2_SYSCONFIG_MSTANDBY_MODE_SMART, OMAP3_ISP_IOMEM_CCP2, ISPCCP2_SYSCONFIG); To make your cleanup even better, you could change isp_reg_clr_set() instead. If CCP2 MSTANDY_MODE is set to NOSTANDY, the result is going to be an invalid 0x3 value. Despite it cannot happen with the current code, it's still better to avoid such situations for future versions. :) Hmm you're right, I guess I didn't do any real functional changes, just defines renaming. I can repost an updated patch with this suggestion. No problem. David, Geez I guess I wasn't paying much attention to this either. Sorry. The case you mention about it potentially become 0x3 is not possible, because, I'm basically overwriting the whole register with (0x2 12) Notice the isp_reg_write, there's no OR operation... That's correct. My mistake. IMO ISP power settings still need a better way to be setup, but it definitively does not belong to your cleanup patch. I'm fine with your changes. Now, this means that we have been implicitly setting other fields as Zeroes (SOFT_RESET = 0, and AUTO_IDLE = 0) aswell. Has AUTOIDLE in CCP2 been tried in N900? I don't have a CCP2 sensor to test with :(. I have no idea. Indeed, AUTOIDLE in ISP doesn't seem to be much reliable so far. It should be set to 0 until somebody proves it can be set to 1 :) Br, David Regards, Sergio /* Hsize, Skip */ diff --git a/drivers/media/video/isp/ispreg.h b/drivers/media/video/isp/ispreg.h index d885541..9b0d3ad 100644 --- a/drivers/media/video/isp/ispreg.h +++ b/drivers/media/video/isp/ispreg.h @@ -141,6 +141,14 @@ #define ISPCCP2_REVISION (0x000) #define ISPCCP2_SYSCONFIG (0x004) #define ISPCCP2_SYSCONFIG_SOFT_RESET (1 1) +#define ISPCCP2_SYSCONFIG_AUTO_IDLE0x1 +#define ISPCCP2_SYSCONFIG_MSTANDBY_MODE_SHIFT 12 +#define ISPCCP2_SYSCONFIG_MSTANDBY_MODE_FORCE \ + (0x0 ISPCCP2_SYSCONFIG_MSTANDBY_MODE_SHIFT) +#define ISPCCP2_SYSCONFIG_MSTANDBY_MODE_NO \ + (0x1 ISPCCP2_SYSCONFIG_MSTANDBY_MODE_SHIFT) +#define ISPCCP2_SYSCONFIG_MSTANDBY_MODE_SMART \ + (0x2 ISPCCP2_SYSCONFIG_MSTANDBY_MODE_SHIFT) You can add some ISPCCP2_SYSCONFIG_MSTANDY_MODE_MASK, as 2 bits must be always set together. Sure, will do. Thanks and Regards, Sergio Regards, David Cohen #define ISPCCP2_SYSSTATUS (0x008) #define ISPCCP2_SYSSTATUS_RESET_DONE (1 0) #define ISPCCP2_LC01_IRQENABLE (0x00C) @@ -1309,12 +1317,6 @@ #define ISPMMU_SIDLEMODE_SMARTIDLE 2 #define ISPMMU_SIDLEMODE_SHIFT 3 -#define ISPCSI1_AUTOIDLE 0x1 -#define ISPCSI1_MIDLEMODE_SHIFT12 -#define ISPCSI1_MIDLEMODE_FORCESTANDBY 0x0 -#define ISPCSI1_MIDLEMODE_NOSTANDBY0x1 -#define ISPCSI1_MIDLEMODE_SMARTSTANDBY 0x2 - /* -- -- - * CSI2 receiver registers (ES2.0) */ -- 1.7.0.4 -- 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: [omap3isp][PATCH v2 7/9] omap3isp: Cleanup isp_power_settings
On Fri, Nov 19, 2010 at 11:58:32PM +0100, ext Aguirre, Sergio wrote:
-Original Message-
From: Aguirre, Sergio
Sent: Friday, November 19, 2010 9:46 AM
To: 'David Cohen'
Cc: Laurent Pinchart; linux-media@vger.kernel.org
Subject: RE: [omap3isp][PATCH v2 7/9] omap3isp: Cleanup isp_power_settings
-Original Message-
From: David Cohen [mailto:david.co...@nokia.com]
Sent: Friday, November 19, 2010 4:18 AM
To: Aguirre, Sergio
Cc: Laurent Pinchart; linux-media@vger.kernel.org
Subject: Re: [omap3isp][PATCH v2 7/9] omap3isp: Cleanup
isp_power_settings
Hi Sergio,
Thanks for the patch.
Hi David,
Thanks for the comments.
On Mon, Nov 15, 2010 at 03:29:59PM +0100, ext Sergio Aguirre wrote:
1. Get rid of CSI2 / CCP2 power settings, as they are controlled
in the receivers code anyways.
CCP2 is not correctly handling this. It's setting SMART STANDBY mode one
when reading from memory. You should fix it before remove such code from
ISP core driver.
Ok, agreed.
I'll generate a new patch before this to compensate that. Not a problem.
Is N900 seeing any functional difference w/ this patch?
Actually, I reanalyzed the patch, and this code should be unexecuted, since
it is conditioned to 3430 ES1.0 chip (omap_rev() == OMAP3430_REV_ES1_0),
which I don't think much people has access. And not definitely any
production quality device.
Even the N900 is using ES3.1 or something like that AFAIK.
So, It should not make any functional difference, unless you have an ES1.0.
Your patch is correct once the code you're removing does not belong to
the ISP core driver, but you're not only getting rid of duplicated code.
You're also removing code for an old version.
I'm not much confortable with this, but I won't disagree if you decide
to keep this patch, once you're not breaking any compatibility. But then
I need to revisit it in future and implement a better way to add the
missing code again.
Br,
David
Regards,
Sergio
2. Avoid code duplication.
Agree. But only after considering the comment above.
Ok.
Thanks and Regards,
Sergio
Regards,
David
Signed-off-by: Sergio Aguirre saagui...@ti.com
---
drivers/media/video/isp/isp.c | 49 ++--
--
---
1 files changed, 7 insertions(+), 42 deletions(-)
diff --git a/drivers/media/video/isp/isp.c
b/drivers/media/video/isp/isp.c
index de9352b..30bdc48 100644
--- a/drivers/media/video/isp/isp.c
+++ b/drivers/media/video/isp/isp.c
@@ -254,48 +254,13 @@ EXPORT_SYMBOL(isp_set_xclk);
*/
static void isp_power_settings(struct isp_device *isp, int idle)
{
- if (idle) {
- isp_reg_writel(isp,
- (ISP_SYSCONFIG_MIDLEMODE_SMARTSTANDBY
- ISP_SYSCONFIG_MIDLEMODE_SHIFT),
- OMAP3_ISP_IOMEM_MAIN, ISP_SYSCONFIG);
- if (omap_rev() == OMAP3430_REV_ES1_0) {
- isp_reg_writel(isp, ISPCSI1_AUTOIDLE |
- (ISPCSI1_MIDLEMODE_SMARTSTANDBY
- ISPCSI1_MIDLEMODE_SHIFT),
- OMAP3_ISP_IOMEM_CSI2A_REGS1,
- ISPCSI2_SYSCONFIG);
- isp_reg_writel(isp, ISPCSI1_AUTOIDLE |
- (ISPCSI1_MIDLEMODE_SMARTSTANDBY
- ISPCSI1_MIDLEMODE_SHIFT),
- OMAP3_ISP_IOMEM_CCP2,
- ISPCCP2_SYSCONFIG);
- }
- isp_reg_writel(isp, ISPCTRL_SBL_AUTOIDLE,
OMAP3_ISP_IOMEM_MAIN,
- ISP_CTRL);
-
- } else {
- isp_reg_writel(isp,
- (ISP_SYSCONFIG_MIDLEMODE_FORCESTANDBY
- ISP_SYSCONFIG_MIDLEMODE_SHIFT),
- OMAP3_ISP_IOMEM_MAIN, ISP_SYSCONFIG);
- if (omap_rev() == OMAP3430_REV_ES1_0) {
- isp_reg_writel(isp, ISPCSI1_AUTOIDLE |
- (ISPCSI1_MIDLEMODE_FORCESTANDBY
- ISPCSI1_MIDLEMODE_SHIFT),
- OMAP3_ISP_IOMEM_CSI2A_REGS1,
- ISPCSI2_SYSCONFIG);
-
- isp_reg_writel(isp, ISPCSI1_AUTOIDLE |
- (ISPCSI1_MIDLEMODE_FORCESTANDBY
- ISPCSI1_MIDLEMODE_SHIFT),
- OMAP3_ISP_IOMEM_CCP2
Re: [omap3isp][PATCH v2 8/9] omap3isp: ccp2: Make SYSCONFIG fields consistent
On Sat, Nov 20, 2010 at 11:34:04AM +0100, ext Laurent Pinchart wrote: Hi, On Saturday 20 November 2010 11:00:30 David Cohen wrote: On Sat, Nov 20, 2010 at 12:10:11AM +0100, ext Aguirre, Sergio wrote: On Friday, November 19, 2010 9:44 AM Aguirre, Sergio wrote: On Friday, November 19, 2010 4:06 AM David Cohen wrote: On Mon, Nov 15, 2010 at 03:30:00PM +0100, ext Sergio Aguirre wrote: Signed-off-by: Sergio Aguirre saagui...@ti.com --- drivers/media/video/isp/ispccp2.c |3 +-- drivers/media/video/isp/ispreg.h | 14 -- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/drivers/media/video/isp/ispccp2.c b/drivers/media/video/isp/ispccp2.c index fa23394..3127a74 100644 --- a/drivers/media/video/isp/ispccp2.c +++ b/drivers/media/video/isp/ispccp2.c @@ -419,8 +419,7 @@ static void ispccp2_mem_configure(struct isp_ccp2_device *ccp2, config-src_ofst = 0; } - isp_reg_writel(isp, (ISPCSI1_MIDLEMODE_SMARTSTANDBY - ISPCSI1_MIDLEMODE_SHIFT), + isp_reg_writel(isp, ISPCCP2_SYSCONFIG_MSTANDBY_MODE_SMART, OMAP3_ISP_IOMEM_CCP2, ISPCCP2_SYSCONFIG); To make your cleanup even better, you could change isp_reg_clr_set() instead. If CCP2 MSTANDY_MODE is set to NOSTANDY, the result is going to be an invalid 0x3 value. Despite it cannot happen with the current code, it's still better to avoid such situations for future versions. :) Hmm you're right, I guess I didn't do any real functional changes, just defines renaming. I can repost an updated patch with this suggestion. No problem. David, Geez I guess I wasn't paying much attention to this either. Sorry. The case you mention about it potentially become 0x3 is not possible, because, I'm basically overwriting the whole register with (0x2 12) Notice the isp_reg_write, there's no OR operation... That's correct. My mistake. IMO ISP power settings still need a better way to be setup, but it definitively does not belong to your cleanup patch. I'm fine with your changes. Now, this means that we have been implicitly setting other fields as Zeroes (SOFT_RESET = 0, and AUTO_IDLE = 0) aswell. Has AUTOIDLE in CCP2 been tried in N900? I don't have a CCP2 sensor to test with :(. I have no idea. Indeed, AUTOIDLE in ISP doesn't seem to be much reliable so far. It should be set to 0 until somebody proves it can be set to 1 :) To summarize things, we're only setting the CCP2 AUTOIDLE bit on ES 1.0 devices, and we're clearing it anyway ispccp2_mem_configure(). For the sake of correctness we should replace isp_reg_writel() by isp_reg_clr_set() in ispccp2_mem_configure(), which will only make a difference on ES 1.0 devices that have basically no users. Is that OK with both of you ? I'm fine with both solutions. IMO power settings could be improved and not be hardcoded. It could come from platform data. But that's another case. :) Br, David -- 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 -- 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: [omap3isp][PATCH v2 7/9] omap3isp: Cleanup isp_power_settings
On Sat, Nov 20, 2010 at 11:45:21AM +0100, Cohen David (Nokia-MS/Helsinki) wrote:
On Fri, Nov 19, 2010 at 11:58:32PM +0100, ext Aguirre, Sergio wrote:
-Original Message-
From: Aguirre, Sergio
Sent: Friday, November 19, 2010 9:46 AM
To: 'David Cohen'
Cc: Laurent Pinchart; linux-media@vger.kernel.org
Subject: RE: [omap3isp][PATCH v2 7/9] omap3isp: Cleanup isp_power_settings
-Original Message-
From: David Cohen [mailto:david.co...@nokia.com]
Sent: Friday, November 19, 2010 4:18 AM
To: Aguirre, Sergio
Cc: Laurent Pinchart; linux-media@vger.kernel.org
Subject: Re: [omap3isp][PATCH v2 7/9] omap3isp: Cleanup
isp_power_settings
Hi Sergio,
Thanks for the patch.
Hi David,
Thanks for the comments.
On Mon, Nov 15, 2010 at 03:29:59PM +0100, ext Sergio Aguirre wrote:
1. Get rid of CSI2 / CCP2 power settings, as they are controlled
in the receivers code anyways.
CCP2 is not correctly handling this. It's setting SMART STANDBY mode one
when reading from memory. You should fix it before remove such code from
ISP core driver.
Ok, agreed.
I'll generate a new patch before this to compensate that. Not a problem.
Is N900 seeing any functional difference w/ this patch?
Actually, I reanalyzed the patch, and this code should be unexecuted, since
it is conditioned to 3430 ES1.0 chip (omap_rev() == OMAP3430_REV_ES1_0),
which I don't think much people has access. And not definitely any
production quality device.
Even the N900 is using ES3.1 or something like that AFAIK.
So, It should not make any functional difference, unless you have an ES1.0.
Your patch is correct once the code you're removing does not belong to
the ISP core driver, but you're not only getting rid of duplicated code.
You're also removing code for an old version.
I'm not much confortable with this, but I won't disagree if you decide
to keep this patch, once you're not breaking any compatibility. But then
I need to revisit it in future and implement a better way to add the
s/I need/we need/ :)
David
missing code again.
Br,
David
Regards,
Sergio
2. Avoid code duplication.
Agree. But only after considering the comment above.
Ok.
Thanks and Regards,
Sergio
Regards,
David
Signed-off-by: Sergio Aguirre saagui...@ti.com
---
drivers/media/video/isp/isp.c | 49 ++--
--
---
1 files changed, 7 insertions(+), 42 deletions(-)
diff --git a/drivers/media/video/isp/isp.c
b/drivers/media/video/isp/isp.c
index de9352b..30bdc48 100644
--- a/drivers/media/video/isp/isp.c
+++ b/drivers/media/video/isp/isp.c
@@ -254,48 +254,13 @@ EXPORT_SYMBOL(isp_set_xclk);
*/
static void isp_power_settings(struct isp_device *isp, int idle)
{
- if (idle) {
- isp_reg_writel(isp,
-(ISP_SYSCONFIG_MIDLEMODE_SMARTSTANDBY
- ISP_SYSCONFIG_MIDLEMODE_SHIFT),
-OMAP3_ISP_IOMEM_MAIN, ISP_SYSCONFIG);
- if (omap_rev() == OMAP3430_REV_ES1_0) {
- isp_reg_writel(isp, ISPCSI1_AUTOIDLE |
-(ISPCSI1_MIDLEMODE_SMARTSTANDBY
- ISPCSI1_MIDLEMODE_SHIFT),
-OMAP3_ISP_IOMEM_CSI2A_REGS1,
-ISPCSI2_SYSCONFIG);
- isp_reg_writel(isp, ISPCSI1_AUTOIDLE |
-(ISPCSI1_MIDLEMODE_SMARTSTANDBY
- ISPCSI1_MIDLEMODE_SHIFT),
-OMAP3_ISP_IOMEM_CCP2,
-ISPCCP2_SYSCONFIG);
- }
- isp_reg_writel(isp, ISPCTRL_SBL_AUTOIDLE,
OMAP3_ISP_IOMEM_MAIN,
-ISP_CTRL);
-
- } else {
- isp_reg_writel(isp,
-(ISP_SYSCONFIG_MIDLEMODE_FORCESTANDBY
- ISP_SYSCONFIG_MIDLEMODE_SHIFT),
-OMAP3_ISP_IOMEM_MAIN, ISP_SYSCONFIG);
- if (omap_rev() == OMAP3430_REV_ES1_0) {
- isp_reg_writel(isp, ISPCSI1_AUTOIDLE |
-(ISPCSI1_MIDLEMODE_FORCESTANDBY
- ISPCSI1_MIDLEMODE_SHIFT),
-OMAP3_ISP_IOMEM_CSI2A_REGS1,
-ISPCSI2_SYSCONFIG);
-
- isp_reg_writel(isp, ISPCSI1_AUTOIDLE |
-(ISPCSI1_MIDLEMODE_FORCESTANDBY
Re: [omap3isp RFC][PATCH 1/4] omap3isp: Abstract isp subdevs clock control
Hi Sergio,
Thanks for the patch. :)
On Sat, Nov 20, 2010 at 12:23:48AM +0100, ext Sergio Aguirre wrote:
Submodules shouldn't be aware of global register bit structure,
specially if the submodules are shared in the future with
other TI architectures (Davinci, future OMAPs, etc)
Signed-off-by: Sergio Aguirre saagui...@ti.com
---
drivers/media/video/isp/isp.c| 54
++
drivers/media/video/isp/isp.h| 12 +++
drivers/media/video/isp/ispccdc.c|6 +--
drivers/media/video/isp/isppreview.c |6 +--
drivers/media/video/isp/ispresizer.c |6 +--
5 files changed, 72 insertions(+), 12 deletions(-)
diff --git a/drivers/media/video/isp/isp.c b/drivers/media/video/isp/isp.c
index 30bdc48..2e5030f 100644
--- a/drivers/media/video/isp/isp.c
+++ b/drivers/media/video/isp/isp.c
@@ -991,6 +991,60 @@ void isp_sbl_disable(struct isp_device *isp, enum
isp_sbl_resource res)
* Clock management
*/
+void isp_subclk_enable(struct isp_device *isp, enum isp_subclk_resource res)
+{
+ u32 clk = 0;
+
+ isp-subclk_resources |= res;
+
+ if (isp-subclk_resources OMAP3_ISP_SUBCLK_H3A)
+ clk |= ISPCTRL_H3A_CLK_EN;
+
+ if (isp-subclk_resources OMAP3_ISP_SUBCLK_HIST)
+ clk |= ISPCTRL_HIST_CLK_EN;
+
+ if (isp-subclk_resources OMAP3_ISP_SUBCLK_RESIZER)
+ clk |= ISPCTRL_RSZ_CLK_EN;
+
+ /* NOTE: For CCDC Preview submodules, we need to affect internal
+ * RAM aswell.
+ */
+ if (isp-subclk_resources OMAP3_ISP_SUBCLK_CCDC)
+ clk |= ISPCTRL_CCDC_CLK_EN | ISPCTRL_CCDC_RAM_EN;
+
+ if (isp-subclk_resources OMAP3_ISP_SUBCLK_PREVIEW)
+ clk |= ISPCTRL_PREV_CLK_EN | ISPCTRL_PREV_RAM_EN;
+
+ isp_reg_set(isp, OMAP3_ISP_IOMEM_MAIN, ISP_CTRL, clk);
+}
To decrease the lines of code, you can create a new static function
called e.g. isp_subclk_update(). It can be a copy of isp_subclk_enable()
but removing the line where it changes 'isp-subclk_resources' and
replacing 'isp_reg_set(isp, OMAP3_ISP_IOMEM_MAIN, ISP_CTRL, clk)' by
'isp_reg_clr_set(isp, OMAP3_ISP_IOMEM_MAIN, ISP_CTRL, clocks mask, clk)'.
Then, isp_subclk_enable() can update isp-subclk_resources, call
isp_subclk_update() and finish.
+
+void isp_subclk_disable(struct isp_device *isp, enum isp_subclk_resource res)
+{
+ u32 clk = 0;
+
+ isp-subclk_resources = ~res;
Same here. This function can call isp_subclk_update() and end now.
+
+ if (!(isp-subclk_resources OMAP3_ISP_SUBCLK_H3A))
+ clk |= ISPCTRL_H3A_CLK_EN;
+
+ if (!(isp-subclk_resources OMAP3_ISP_SUBCLK_HIST))
+ clk |= ISPCTRL_HIST_CLK_EN;
+
+ if (!(isp-subclk_resources OMAP3_ISP_SUBCLK_RESIZER))
+ clk |= ISPCTRL_RSZ_CLK_EN;
+
+ /* NOTE: For CCDC Preview submodules, we need to affect internal
+ * RAM aswell.
+ */
+ if (!(isp-subclk_resources OMAP3_ISP_SUBCLK_CCDC))
+ clk |= ISPCTRL_CCDC_CLK_EN | ISPCTRL_CCDC_RAM_EN;
+
+ if (!(isp-subclk_resources OMAP3_ISP_SUBCLK_PREVIEW))
+ clk |= ISPCTRL_PREV_CLK_EN | ISPCTRL_PREV_RAM_EN;
+
+ isp_reg_clr(isp, OMAP3_ISP_IOMEM_MAIN, ISP_CTRL, clk);
+}
+
/*
* isp_enable_clocks - Enable ISP clocks
* @isp: OMAP3 ISP device
diff --git a/drivers/media/video/isp/isp.h b/drivers/media/video/isp/isp.h
index b8f63e2..1260e9f 100644
--- a/drivers/media/video/isp/isp.h
+++ b/drivers/media/video/isp/isp.h
@@ -85,6 +85,14 @@ enum isp_sbl_resource {
OMAP3_ISP_SBL_RESIZER_WRITE = 0x200,
};
+enum isp_subclk_resource {
+ OMAP3_ISP_SUBCLK_CCDC = 0x1,
+ OMAP3_ISP_SUBCLK_H3A= 0x2,
+ OMAP3_ISP_SUBCLK_HIST = 0x4,
+ OMAP3_ISP_SUBCLK_PREVIEW= 0x8,
+ OMAP3_ISP_SUBCLK_RESIZER= 0x10,
I'm fine with this way, but (1 0), (1 1), ... has a better readability.
Br,
David Cohen
+};
+
enum isp_interface_type {
ISP_INTERFACE_PARALLEL,
ISP_INTERFACE_CSI2A_PHY2,
@@ -262,6 +270,7 @@ struct isp_device {
struct isp_csiphy isp_csiphy2;
unsigned int sbl_resources;
+ unsigned int subclk_resources;
struct iommu *iommu;
};
@@ -294,6 +303,9 @@ void isp_print_status(struct isp_device *isp);
void isp_sbl_enable(struct isp_device *isp, enum isp_sbl_resource res);
void isp_sbl_disable(struct isp_device *isp, enum isp_sbl_resource res);
+void isp_subclk_enable(struct isp_device *isp, enum isp_subclk_resource res);
+void isp_subclk_disable(struct isp_device *isp, enum isp_subclk_resource
res);
+
int omap3isp_register_entities(struct platform_device *pdev,
struct v4l2_device *v4l2_dev);
void omap3isp_unregister_entities(struct platform_device *pdev);
diff --git a/drivers/media/video/isp/ispccdc.c
b/drivers/media/video/isp/ispccdc.c
index c3d1d7a..4244edf 100644
--- a/drivers
Re: [omap3isp RFC][PATCH 2/4] omap3isp: Move CCDC LSC prefetch wait to main isp code
Hi Sergio,
On Sat, Nov 20, 2010 at 12:23:49AM +0100, ext Sergio Aguirre wrote:
Since this sequence strictly touches ISP global registers, it's
not really part of the same register address space than the CCDC.
Do this check in main isp code instead.
Signed-off-by: Sergio Aguirre saagui...@ti.com
---
drivers/media/video/isp/isp.c | 24
drivers/media/video/isp/isp.h |2 ++
drivers/media/video/isp/ispccdc.c | 26 +-
3 files changed, 27 insertions(+), 25 deletions(-)
diff --git a/drivers/media/video/isp/isp.c b/drivers/media/video/isp/isp.c
index 2e5030f..ee45eb6 100644
--- a/drivers/media/video/isp/isp.c
+++ b/drivers/media/video/isp/isp.c
@@ -339,6 +339,30 @@ void isphist_dma_done(struct isp_device *isp)
}
}
+int ispccdc_lsc_wait_prefetch(struct isp_device *isp)
This is up to you, but to ensure this function now belongs to ISP core
and not CCDC anymore, I would change the function name to something like
isp_ccdc_lsc_wait_prefetch().
isp_* is prefix for ISP core and ispccdc_* is prefix for CCDC driver.
I know we have the isphist_dma_done() inside ISP core, but changing it
to isp_hist_dma_done could be a good cleanup as well.
But this is my opinion only. :)
Br,
David Cohen
+{
+ unsigned int wait;
+
+ isp_reg_writel(isp, IRQ0STATUS_CCDC_LSC_PREF_COMP_IRQ,
+OMAP3_ISP_IOMEM_MAIN, ISP_IRQ0STATUS);
+
+ /* timeout 1 ms */
+ for (wait = 0; wait 1000; wait++) {
+ if (isp_reg_readl(isp, OMAP3_ISP_IOMEM_MAIN, ISP_IRQ0STATUS)
+ IRQ0STATUS_CCDC_LSC_PREF_COMP_IRQ) {
+ isp_reg_writel(isp, IRQ0STATUS_CCDC_LSC_PREF_COMP_IRQ,
+OMAP3_ISP_IOMEM_MAIN, ISP_IRQ0STATUS);
+ return 0;
+ }
+
+ rmb();
+ udelay(1);
+ }
+
+ return -ETIMEDOUT;
+}
+
+
static inline void isp_isr_dbg(struct isp_device *isp, u32 irqstatus)
{
static const char *name[] = {
diff --git a/drivers/media/video/isp/isp.h b/drivers/media/video/isp/isp.h
index 1260e9f..d0b7b0f 100644
--- a/drivers/media/video/isp/isp.h
+++ b/drivers/media/video/isp/isp.h
@@ -280,6 +280,8 @@ struct isp_device {
void isphist_dma_done(struct isp_device *isp);
+int ispccdc_lsc_wait_prefetch(struct isp_device *isp);
+
void isp_flush(struct isp_device *isp);
int isp_pipeline_set_stream(struct isp_pipeline *pipe,
diff --git a/drivers/media/video/isp/ispccdc.c
b/drivers/media/video/isp/ispccdc.c
index 4244edf..b039bce 100644
--- a/drivers/media/video/isp/ispccdc.c
+++ b/drivers/media/video/isp/ispccdc.c
@@ -223,30 +223,6 @@ static void ispccdc_lsc_setup_regs(struct
isp_ccdc_device *ccdc,
ISPCCDC_LSC_INITIAL);
}
-static int ispccdc_lsc_wait_prefetch(struct isp_ccdc_device *ccdc)
-{
- struct isp_device *isp = to_isp_device(ccdc);
- unsigned int wait;
-
- isp_reg_writel(isp, IRQ0STATUS_CCDC_LSC_PREF_COMP_IRQ,
-OMAP3_ISP_IOMEM_MAIN, ISP_IRQ0STATUS);
-
- /* timeout 1 ms */
- for (wait = 0; wait 1000; wait++) {
- if (isp_reg_readl(isp, OMAP3_ISP_IOMEM_MAIN, ISP_IRQ0STATUS)
- IRQ0STATUS_CCDC_LSC_PREF_COMP_IRQ) {
- isp_reg_writel(isp, IRQ0STATUS_CCDC_LSC_PREF_COMP_IRQ,
-OMAP3_ISP_IOMEM_MAIN, ISP_IRQ0STATUS);
- return 0;
- }
-
- rmb();
- udelay(1);
- }
-
- return -ETIMEDOUT;
-}
-
/*
* __ispccdc_lsc_enable - Enables/Disables the Lens Shading Compensation
module.
* @ccdc: Pointer to ISP CCDC device.
@@ -272,7 +248,7 @@ static int __ispccdc_lsc_enable(struct isp_ccdc_device
*ccdc, int enable)
ISPCCDC_LSC_ENABLE, enable ? ISPCCDC_LSC_ENABLE : 0);
if (enable) {
- if (ispccdc_lsc_wait_prefetch(ccdc) 0) {
+ if (ispccdc_lsc_wait_prefetch(isp) 0) {
isp_reg_clr(isp, OMAP3_ISP_IOMEM_CCDC,
ISPCCDC_LSC_CONFIG, ISPCCDC_LSC_ENABLE);
ccdc-lsc.state = LSC_STATE_STOPPED;
--
1.7.0.4
--
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
--
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: [omap3isp RFC][PATCH 0/4] Improve inter subdev interaction
Hi Sergio, On Sat, Nov 20, 2010 at 12:23:47AM +0100, ext Sergio Aguirre wrote: Hi, These are some patches to make these operations more generic: - Clock control is being controlled in a very crude manner by subdevices, it should be centralized in isp.c. - LSC prefetch wait check is reading a main ISP register, so move it to isp.c - Abstract SBL busy check: we don't want a submodule thinkering with main ISP registers. That should be done in the main isp.c Also, remove main ISP register dump from CSI2 specific dump. We should be using isp_print_status if we'll like to know main ISP regdump. Comments are welcome. More cleanups for better subdevice isolation are on the way. Your patches are fine for me. I sent you some comments, but they are opitional and it's up to you to decide what to do. :) You can copy linux-omap@ as well in future patches. Regards, David Regards, Sergio Sergio Aguirre (4): omap3isp: Abstract isp subdevs clock control omap3isp: Move CCDC LSC prefetch wait to main isp code omap3isp: sbl: Abstract SBL busy check omap3isp: csi2: Don't dump ISP main registers drivers/media/video/isp/isp.c| 95 ++ drivers/media/video/isp/isp.h| 16 ++ drivers/media/video/isp/ispccdc.c| 42 ++- drivers/media/video/isp/ispcsi2.c|7 --- drivers/media/video/isp/isppreview.c |6 +-- drivers/media/video/isp/ispresizer.c |6 +-- 6 files changed, 119 insertions(+), 53 deletions(-) -- 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 -- 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
[omap3isp][PATCH v2] omap3isp: does not allow pipeline with multiple video outputs yet
From: Cohen David (Nokia-MS/Helsinki) david.co...@nokia.com
The ISP core doesn't support pipelines with multiple video outputs. Revisit
this when it will be implemented, and return -EBUSY for now.
Signed-off-by: David Cohen david.co...@nokia.com
---
drivers/media/video/isp/ispccdc.c| 26 --
drivers/media/video/isp/ispcsi2.c| 19 +++
drivers/media/video/isp/isppreview.c | 19 +++
3 files changed, 50 insertions(+), 14 deletions(-)
diff --git a/drivers/media/video/isp/ispccdc.c
b/drivers/media/video/isp/ispccdc.c
index 2541c92..6cca5ce 100644
--- a/drivers/media/video/isp/ispccdc.c
+++ b/drivers/media/video/isp/ispccdc.c
@@ -2018,30 +2018,44 @@ static int ccdc_link_setup(struct media_entity *entity,
break;
+ /*
+* This driver currently does not support pipeline with multiples
+* video outputs. It must return -EBUSY while it's not implemented.
+*/
+
case CCDC_PAD_SOURCE_VP | (MEDIA_ENTITY_TYPE_SUBDEV 16):
/* Write to preview engine, histogram and H3A. When none of
* those links are active, the video port can be disabled.
*/
- if (flags MEDIA_LINK_FLAG_ACTIVE)
+ if (flags MEDIA_LINK_FLAG_ACTIVE) {
+ if (ccdc-output ~CCDC_OUTPUT_PREVIEW)
+ return -EBUSY;
ccdc-output |= CCDC_OUTPUT_PREVIEW;
- else
+ } else {
ccdc-output = ~CCDC_OUTPUT_PREVIEW;
+ }
break;
case CCDC_PAD_SOURCE_OF | (MEDIA_ENTITY_TYPE_NODE 16):
/* Write to memory */
- if (flags MEDIA_LINK_FLAG_ACTIVE)
+ if (flags MEDIA_LINK_FLAG_ACTIVE) {
+ if (ccdc-output ~CCDC_OUTPUT_MEMORY)
+ return -EBUSY;
ccdc-output |= CCDC_OUTPUT_MEMORY;
- else
+ } else {
ccdc-output = ~CCDC_OUTPUT_MEMORY;
+ }
break;
case CCDC_PAD_SOURCE_OF | (MEDIA_ENTITY_TYPE_SUBDEV 16):
/* Write to resizer */
- if (flags MEDIA_LINK_FLAG_ACTIVE)
+ if (flags MEDIA_LINK_FLAG_ACTIVE) {
+ if (ccdc-output ~CCDC_OUTPUT_RESIZER)
+ return -EBUSY;
ccdc-output |= CCDC_OUTPUT_RESIZER;
- else
+ } else {
ccdc-output = ~CCDC_OUTPUT_RESIZER;
+ }
break;
default:
diff --git a/drivers/media/video/isp/ispcsi2.c
b/drivers/media/video/isp/ispcsi2.c
index a94c1ec..ddf485b 100644
--- a/drivers/media/video/isp/ispcsi2.c
+++ b/drivers/media/video/isp/ispcsi2.c
@@ -1046,19 +1046,30 @@ static int csi2_link_setup(struct media_entity *entity,
struct isp_csi2_device *csi2 = v4l2_get_subdevdata(sd);
struct isp_csi2_ctrl_cfg *ctrl = csi2-ctrl;
+ /*
+* This driver currently does not support pipeline with multiples
+* video outputs. It must return -EBUSY while it's not implemented.
+*/
+
switch (local-index | (remote-entity-type 16)) {
case CSI2_PAD_SOURCE | (MEDIA_ENTITY_TYPE_NODE 16):
- if (flags MEDIA_LINK_FLAG_ACTIVE)
+ if (flags MEDIA_LINK_FLAG_ACTIVE) {
+ if (csi2-output ~CSI2_OUTPUT_MEMORY)
+ return -EBUSY;
csi2-output |= CSI2_OUTPUT_MEMORY;
- else
+ } else {
csi2-output = ~CSI2_OUTPUT_MEMORY;
+ }
break;
case CSI2_PAD_SOURCE | (MEDIA_ENTITY_TYPE_SUBDEV 16):
- if (flags MEDIA_LINK_FLAG_ACTIVE)
+ if (flags MEDIA_LINK_FLAG_ACTIVE) {
+ if (csi2-output ~CSI2_OUTPUT_CCDC)
+ return -EBUSY;
csi2-output |= CSI2_OUTPUT_CCDC;
- else
+ } else {
csi2-output = ~CSI2_OUTPUT_CCDC;
+ }
break;
default:
diff --git a/drivers/media/video/isp/isppreview.c
b/drivers/media/video/isp/isppreview.c
index 8da0de1..55f2bf4 100644
--- a/drivers/media/video/isp/isppreview.c
+++ b/drivers/media/video/isp/isppreview.c
@@ -2013,20 +2013,31 @@ static int preview_link_setup(struct media_entity
*entity,
}
break;
+ /*
+* This driver currently does not support pipeline with multiples
+* video outputs. It must return -EBUSY while it's not implemented.
+*/
+
case PREV_PAD_SOURCE | (MEDIA_ENTITY_TYPE_NODE 16):
/* write to memory */
- if (flags MEDIA_LINK_FLAG_ACTIVE)
+ if (flags
Re: [omap3isp][PATCH v2 8/9] omap3isp: ccp2: Make SYSCONFIG fields consistent
Hi Sergio, I've few comments below. On Mon, Nov 15, 2010 at 03:30:00PM +0100, ext Sergio Aguirre wrote: Signed-off-by: Sergio Aguirre saagui...@ti.com --- drivers/media/video/isp/ispccp2.c |3 +-- drivers/media/video/isp/ispreg.h | 14 -- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/drivers/media/video/isp/ispccp2.c b/drivers/media/video/isp/ispccp2.c index fa23394..3127a74 100644 --- a/drivers/media/video/isp/ispccp2.c +++ b/drivers/media/video/isp/ispccp2.c @@ -419,8 +419,7 @@ static void ispccp2_mem_configure(struct isp_ccp2_device *ccp2, config-src_ofst = 0; } - isp_reg_writel(isp, (ISPCSI1_MIDLEMODE_SMARTSTANDBY -ISPCSI1_MIDLEMODE_SHIFT), + isp_reg_writel(isp, ISPCCP2_SYSCONFIG_MSTANDBY_MODE_SMART, OMAP3_ISP_IOMEM_CCP2, ISPCCP2_SYSCONFIG); To make your cleanup even better, you could change isp_reg_clr_set() instead. If CCP2 MSTANDY_MODE is set to NOSTANDY, the result is going to be an invalid 0x3 value. Despite it cannot happen with the current code, it's still better to avoid such situations for future versions. :) /* Hsize, Skip */ diff --git a/drivers/media/video/isp/ispreg.h b/drivers/media/video/isp/ispreg.h index d885541..9b0d3ad 100644 --- a/drivers/media/video/isp/ispreg.h +++ b/drivers/media/video/isp/ispreg.h @@ -141,6 +141,14 @@ #define ISPCCP2_REVISION (0x000) #define ISPCCP2_SYSCONFIG(0x004) #define ISPCCP2_SYSCONFIG_SOFT_RESET (1 1) +#define ISPCCP2_SYSCONFIG_AUTO_IDLE 0x1 +#define ISPCCP2_SYSCONFIG_MSTANDBY_MODE_SHIFT12 +#define ISPCCP2_SYSCONFIG_MSTANDBY_MODE_FORCE\ + (0x0 ISPCCP2_SYSCONFIG_MSTANDBY_MODE_SHIFT) +#define ISPCCP2_SYSCONFIG_MSTANDBY_MODE_NO \ + (0x1 ISPCCP2_SYSCONFIG_MSTANDBY_MODE_SHIFT) +#define ISPCCP2_SYSCONFIG_MSTANDBY_MODE_SMART\ + (0x2 ISPCCP2_SYSCONFIG_MSTANDBY_MODE_SHIFT) You can add some ISPCCP2_SYSCONFIG_MSTANDY_MODE_MASK, as 2 bits must be always set together. Regards, David Cohen #define ISPCCP2_SYSSTATUS(0x008) #define ISPCCP2_SYSSTATUS_RESET_DONE (1 0) #define ISPCCP2_LC01_IRQENABLE (0x00C) @@ -1309,12 +1317,6 @@ #define ISPMMU_SIDLEMODE_SMARTIDLE 2 #define ISPMMU_SIDLEMODE_SHIFT 3 -#define ISPCSI1_AUTOIDLE 0x1 -#define ISPCSI1_MIDLEMODE_SHIFT 12 -#define ISPCSI1_MIDLEMODE_FORCESTANDBY 0x0 -#define ISPCSI1_MIDLEMODE_NOSTANDBY 0x1 -#define ISPCSI1_MIDLEMODE_SMARTSTANDBY 0x2 - /* - * CSI2 receiver registers (ES2.0) */ -- 1.7.0.4 -- 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 -- 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: [omap3isp][PATCH v2 7/9] omap3isp: Cleanup isp_power_settings
Hi Sergio,
Thanks for the patch.
On Mon, Nov 15, 2010 at 03:29:59PM +0100, ext Sergio Aguirre wrote:
1. Get rid of CSI2 / CCP2 power settings, as they are controlled
in the receivers code anyways.
CCP2 is not correctly handling this. It's setting SMART STANDBY mode one
when reading from memory. You should fix it before remove such code from
ISP core driver.
2. Avoid code duplication.
Agree. But only after considering the comment above.
Regards,
David
Signed-off-by: Sergio Aguirre saagui...@ti.com
---
drivers/media/video/isp/isp.c | 49
++---
1 files changed, 7 insertions(+), 42 deletions(-)
diff --git a/drivers/media/video/isp/isp.c b/drivers/media/video/isp/isp.c
index de9352b..30bdc48 100644
--- a/drivers/media/video/isp/isp.c
+++ b/drivers/media/video/isp/isp.c
@@ -254,48 +254,13 @@ EXPORT_SYMBOL(isp_set_xclk);
*/
static void isp_power_settings(struct isp_device *isp, int idle)
{
- if (idle) {
- isp_reg_writel(isp,
-(ISP_SYSCONFIG_MIDLEMODE_SMARTSTANDBY
- ISP_SYSCONFIG_MIDLEMODE_SHIFT),
-OMAP3_ISP_IOMEM_MAIN, ISP_SYSCONFIG);
- if (omap_rev() == OMAP3430_REV_ES1_0) {
- isp_reg_writel(isp, ISPCSI1_AUTOIDLE |
-(ISPCSI1_MIDLEMODE_SMARTSTANDBY
- ISPCSI1_MIDLEMODE_SHIFT),
-OMAP3_ISP_IOMEM_CSI2A_REGS1,
-ISPCSI2_SYSCONFIG);
- isp_reg_writel(isp, ISPCSI1_AUTOIDLE |
-(ISPCSI1_MIDLEMODE_SMARTSTANDBY
- ISPCSI1_MIDLEMODE_SHIFT),
-OMAP3_ISP_IOMEM_CCP2,
-ISPCCP2_SYSCONFIG);
- }
- isp_reg_writel(isp, ISPCTRL_SBL_AUTOIDLE, OMAP3_ISP_IOMEM_MAIN,
-ISP_CTRL);
-
- } else {
- isp_reg_writel(isp,
-(ISP_SYSCONFIG_MIDLEMODE_FORCESTANDBY
- ISP_SYSCONFIG_MIDLEMODE_SHIFT),
-OMAP3_ISP_IOMEM_MAIN, ISP_SYSCONFIG);
- if (omap_rev() == OMAP3430_REV_ES1_0) {
- isp_reg_writel(isp, ISPCSI1_AUTOIDLE |
-(ISPCSI1_MIDLEMODE_FORCESTANDBY
- ISPCSI1_MIDLEMODE_SHIFT),
-OMAP3_ISP_IOMEM_CSI2A_REGS1,
-ISPCSI2_SYSCONFIG);
-
- isp_reg_writel(isp, ISPCSI1_AUTOIDLE |
-(ISPCSI1_MIDLEMODE_FORCESTANDBY
- ISPCSI1_MIDLEMODE_SHIFT),
-OMAP3_ISP_IOMEM_CCP2,
-ISPCCP2_SYSCONFIG);
- }
-
- isp_reg_writel(isp, ISPCTRL_SBL_AUTOIDLE, OMAP3_ISP_IOMEM_MAIN,
-ISP_CTRL);
- }
+ isp_reg_writel(isp,
+((idle ? ISP_SYSCONFIG_MIDLEMODE_SMARTSTANDBY :
+ ISP_SYSCONFIG_MIDLEMODE_FORCESTANDBY)
+ ISP_SYSCONFIG_MIDLEMODE_SHIFT),
+OMAP3_ISP_IOMEM_MAIN, ISP_SYSCONFIG);
+ isp_reg_writel(isp, ISPCTRL_SBL_AUTOIDLE, OMAP3_ISP_IOMEM_MAIN,
+ISP_CTRL);
}
/*
--
1.7.0.4
--
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
--
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: [omap3isp][PATCH v2 5/9] omap3isp: Remove unused CBUFF register access
Hi Sergio,
I have one comment below.
On Mon, Nov 15, 2010 at 03:29:57PM +0100, ext Sergio Aguirre wrote:
Signed-off-by: Sergio Aguirre saagui...@ti.com
---
drivers/media/video/isp/isp.c|2 --
drivers/media/video/isp/isp.h|1 -
drivers/media/video/isp/ispreg.h | 25 -
3 files changed, 0 insertions(+), 28 deletions(-)
diff --git a/drivers/media/video/isp/isp.c b/drivers/media/video/isp/isp.c
index a5c02ba..f266e7c 100644
--- a/drivers/media/video/isp/isp.c
+++ b/drivers/media/video/isp/isp.c
@@ -86,7 +86,6 @@ static const struct isp_res_mapping isp_res_maps[] = {
{
.isp_rev = ISP_REVISION_2_0,
.map = 1 OMAP3_ISP_IOMEM_MAIN |
-1 OMAP3_ISP_IOMEM_CBUFF |
1 OMAP3_ISP_IOMEM_CCP2 |
1 OMAP3_ISP_IOMEM_CCDC |
1 OMAP3_ISP_IOMEM_HIST |
@@ -100,7 +99,6 @@ static const struct isp_res_mapping isp_res_maps[] = {
{
.isp_rev = ISP_REVISION_15_0,
.map = 1 OMAP3_ISP_IOMEM_MAIN |
-1 OMAP3_ISP_IOMEM_CBUFF |
1 OMAP3_ISP_IOMEM_CCP2 |
1 OMAP3_ISP_IOMEM_CCDC |
1 OMAP3_ISP_IOMEM_HIST |
diff --git a/drivers/media/video/isp/isp.h b/drivers/media/video/isp/isp.h
index edc029c..b8f63e2 100644
--- a/drivers/media/video/isp/isp.h
+++ b/drivers/media/video/isp/isp.h
@@ -56,7 +56,6 @@
enum isp_mem_resources {
OMAP3_ISP_IOMEM_MAIN,
- OMAP3_ISP_IOMEM_CBUFF,
OMAP3_ISP_IOMEM_CCP2,
OMAP3_ISP_IOMEM_CCDC,
OMAP3_ISP_IOMEM_HIST,
diff --git a/drivers/media/video/isp/ispreg.h
b/drivers/media/video/isp/ispreg.h
index 8e4324f..c080980 100644
--- a/drivers/media/video/isp/ispreg.h
+++ b/drivers/media/video/isp/ispreg.h
@@ -37,11 +37,6 @@
#define OMAP3ISP_REG_BASEOMAP3430_ISP_BASE
#define OMAP3ISP_REG(offset) (OMAP3ISP_REG_BASE + (offset))
-#define OMAP3ISP_CBUFF_REG_OFFSET0x0100
-#define OMAP3ISP_CBUFF_REG_BASE (OMAP3ISP_REG_BASE +
\
- OMAP3ISP_CBUFF_REG_OFFSET)
-#define OMAP3ISP_CBUFF_REG(offset) (OMAP3ISP_CBUFF_REG_BASE + (offset))
-
#define OMAP3ISP_CCP2_REG_OFFSET 0x0400
#define OMAP3ISP_CCP2_REG_BASE (OMAP3ISP_REG_BASE +
\
OMAP3ISP_CCP2_REG_OFFSET)
@@ -244,26 +239,6 @@
#define ISP_CSIB_SYSCONFIG ISPCCP2_SYSCONFIG
#define ISP_CSIA_SYSCONFIG ISPCSI2_SYSCONFIG
-/* ISP_CBUFF Registers */
-
-#define ISP_CBUFF_SYSCONFIG (0x010)
-#define ISP_CBUFF_IRQENABLE (0x01C)
-
-#define ISP_CBUFF0_CTRL (0x020)
-#define ISP_CBUFF1_CTRL (0x024)
-
-#define ISP_CBUFF0_START (0x040)
-#define ISP_CBUFF1_START (0x044)
-
-#define ISP_CBUFF0_END (0x050)
-#define ISP_CBUFF1_END (0x054)
-
-#define ISP_CBUFF0_WINDOWSIZE(0x060)
-#define ISP_CBUFF1_WINDOWSIZE(0x064)
-
-#define ISP_CBUFF0_THRESHOLD (0x070)
-#define ISP_CBUFF1_THRESHOLD (0x074)
-
No need to remove the registers from header file. We're not using them
on current version, but it doesn't mean we won't use ever. :)
Br,
David Cohen
/* CCDC module register offset */
#define ISPCCDC_PID (0x000)
--
1.7.0.4
--
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
--
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
[omap3isp][PATCH] omap3isp: does not allow pipeline with multiple video outputs yet
OMAP3 ISP driver does not support pipeline with multiple video outputs yet.
Driver must return -EBUSY in this case.
Signed-off-by: David Cohen david.co...@nokia.com
---
drivers/media/video/isp/ispccdc.c| 26 --
drivers/media/video/isp/ispcsi2.c| 19 +++
drivers/media/video/isp/isppreview.c | 19 +++
3 files changed, 50 insertions(+), 14 deletions(-)
diff --git a/drivers/media/video/isp/ispccdc.c
b/drivers/media/video/isp/ispccdc.c
index a4c6444..a19795c 100644
--- a/drivers/media/video/isp/ispccdc.c
+++ b/drivers/media/video/isp/ispccdc.c
@@ -2016,30 +2016,44 @@ static int ccdc_link_setup(struct media_entity *entity,
break;
+ /*
+* This driver currently does not support pipeline with multiples
+* video outputs. It must return -EBUSY while it's not implemented.
+*/
+
case CCDC_PAD_SOURCE_VP | (MEDIA_ENTITY_TYPE_SUBDEV 16):
/* Write to preview engine, histogram and H3A. When none of
* those links are active, the video port can be disabled.
*/
- if (flags MEDIA_LINK_FLAG_ACTIVE)
+ if (flags MEDIA_LINK_FLAG_ACTIVE) {
+ if (ccdc-output ~CCDC_OUTPUT_PREVIEW)
+ return -EBUSY;
ccdc-output |= CCDC_OUTPUT_PREVIEW;
- else
+ } else {
ccdc-output = ~CCDC_OUTPUT_PREVIEW;
+ }
break;
case CCDC_PAD_SOURCE_OF | (MEDIA_ENTITY_TYPE_NODE 16):
/* Write to memory */
- if (flags MEDIA_LINK_FLAG_ACTIVE)
+ if (flags MEDIA_LINK_FLAG_ACTIVE) {
+ if (ccdc-output ~CCDC_OUTPUT_MEMORY)
+ return -EBUSY;
ccdc-output |= CCDC_OUTPUT_MEMORY;
- else
+ } else {
ccdc-output = ~CCDC_OUTPUT_MEMORY;
+ }
break;
case CCDC_PAD_SOURCE_OF | (MEDIA_ENTITY_TYPE_SUBDEV 16):
/* Write to resizer */
- if (flags MEDIA_LINK_FLAG_ACTIVE)
+ if (flags MEDIA_LINK_FLAG_ACTIVE) {
+ if (ccdc-output ~CCDC_OUTPUT_RESIZER)
+ return -EBUSY;
ccdc-output |= CCDC_OUTPUT_RESIZER;
- else
+ } else {
ccdc-output = ~CCDC_OUTPUT_RESIZER;
+ }
break;
default:
diff --git a/drivers/media/video/isp/ispcsi2.c
b/drivers/media/video/isp/ispcsi2.c
index 65c777a..8052b38 100644
--- a/drivers/media/video/isp/ispcsi2.c
+++ b/drivers/media/video/isp/ispcsi2.c
@@ -1088,19 +1088,30 @@ static int csi2_link_setup(struct media_entity *entity,
struct isp_csi2_device *csi2 = v4l2_get_subdevdata(sd);
struct isp_csi2_ctrl_cfg *ctrl = csi2-ctrl;
+ /*
+* This driver currently does not support pipeline with multiples
+* video outputs. It must return -EBUSY while it's not implemented.
+*/
+
switch (local-index | (remote-entity-type 16)) {
case CSI2_PAD_SOURCE | (MEDIA_ENTITY_TYPE_NODE 16):
- if (flags MEDIA_LINK_FLAG_ACTIVE)
+ if (flags MEDIA_LINK_FLAG_ACTIVE) {
+ if (csi2-output ~CSI2_OUTPUT_MEMORY)
+ return -EBUSY;
csi2-output |= CSI2_OUTPUT_MEMORY;
- else
+ } else {
csi2-output = ~CSI2_OUTPUT_MEMORY;
+ }
break;
case CSI2_PAD_SOURCE | (MEDIA_ENTITY_TYPE_SUBDEV 16):
- if (flags MEDIA_LINK_FLAG_ACTIVE)
+ if (flags MEDIA_LINK_FLAG_ACTIVE) {
+ if (csi2-output ~CSI2_OUTPUT_MEMORY)
+ return -EBUSY;
csi2-output |= CSI2_OUTPUT_CCDC;
- else
+ } else {
csi2-output = ~CSI2_OUTPUT_CCDC;
+ }
break;
default:
diff --git a/drivers/media/video/isp/isppreview.c
b/drivers/media/video/isp/isppreview.c
index 6274b44..39d4da4 100644
--- a/drivers/media/video/isp/isppreview.c
+++ b/drivers/media/video/isp/isppreview.c
@@ -2026,20 +2026,31 @@ static int preview_link_setup(struct media_entity
*entity,
}
break;
+ /*
+* This driver currently does not support pipeline with multiples
+* video outputs. It must return -EBUSY while it's not implemented.
+*/
+
case PREV_PAD_SOURCE | (MEDIA_ENTITY_TYPE_NODE 16):
/* write to memory */
- if (flags MEDIA_LINK_FLAG_ACTIVE)
+ if (flags MEDIA_LINK_FLAG_ACTIVE) {
+ if (prev-output
Re: Translation faults with OMAP ISP
Hi Lane, On Thu, Nov 18, 2010 at 12:17:21AM +0100, ext Lane Brooks wrote: On 11/17/2010 04:09 PM, Laurent Pinchart wrote: Hi Lane, On Wednesday 17 November 2010 00:46:27 Lane Brooks wrote: Laurent, I am getting iommu translation errors when I try to use the CCDC output after using the Resizer output. If I use the CCDC output to stream some video, then close it down, switch to the Resizer output and open it up and try to stream, I get the following errors spewing out: omap-iommu omap-iommu.0: omap2_iommu_fault_isr: da:00d0ef00 translation fault omap-iommu omap-iommu.0: iommu_fault_handler: da:00d0ef00 pgd:ce664034 *pgd: and the select times out. From a fresh boot, I can stream just fine from the Resizer and then switch to the CCDC output just fine. It is only when I go from the CCDC to the Resizer that I get this problem. Furthermore, when it gets into this state, then anything dev node I try to use has the translation errors and the only way to recover is to reboot. Any ideas on the problem? Ouch. First of all, could you please make sure you run the latest code ? Many bugs have been fixed in the last few months I had a pretty good idea that this would be your response, but I was hoping otherwise as merging has become more and more difficult to keep up with. Anyway, until I have a chance to merge in everything, I just found a work around for our usage needs, and that is to always use the resizer output and just change the resizer format between full resolution and preview resolution. This has turned out to be much more stable than switching between the CCDC and RESIZER dev nodes. I'm not sure if it's your case, but OMAP3 ISP driver does not support pipeline with multiples outputs yet. We have to return error from the driver in this case. If you configured CCDC to write to memory and then to write to preview/resizer afterwards without deactivating the link to write to memory, you may face a similar problem you described. Can you please try a patch I've sent to you (CC'ing linux-media) with subject: [omap3isp][PATCH] omap3isp: does not allow pipeline with multiple video outputs yet? Regards, David Thanks again for your feedback. Lane -- 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 -- 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: [omap3isp][PATCH] omap3isp: does not allow pipeline with multiple video outputs yet
On Fri, Nov 19, 2010 at 02:23:54PM +0100, Cohen David (Nokia-MS/Helsinki) wrote:
OMAP3 ISP driver does not support pipeline with multiple video outputs yet.
Driver must return -EBUSY in this case.
Signed-off-by: David Cohen david.co...@nokia.com
---
drivers/media/video/isp/ispccdc.c| 26 --
drivers/media/video/isp/ispcsi2.c| 19 +++
drivers/media/video/isp/isppreview.c | 19 +++
3 files changed, 50 insertions(+), 14 deletions(-)
[snip]
diff --git a/drivers/media/video/isp/ispcsi2.c
b/drivers/media/video/isp/ispcsi2.c
index 65c777a..8052b38 100644
--- a/drivers/media/video/isp/ispcsi2.c
+++ b/drivers/media/video/isp/ispcsi2.c
@@ -1088,19 +1088,30 @@ static int csi2_link_setup(struct media_entity
*entity,
struct isp_csi2_device *csi2 = v4l2_get_subdevdata(sd);
struct isp_csi2_ctrl_cfg *ctrl = csi2-ctrl;
+ /*
+ * This driver currently does not support pipeline with multiples
+ * video outputs. It must return -EBUSY while it's not implemented.
+ */
+
switch (local-index | (remote-entity-type 16)) {
case CSI2_PAD_SOURCE | (MEDIA_ENTITY_TYPE_NODE 16):
- if (flags MEDIA_LINK_FLAG_ACTIVE)
+ if (flags MEDIA_LINK_FLAG_ACTIVE) {
+ if (csi2-output ~CSI2_OUTPUT_MEMORY)
+ return -EBUSY;
csi2-output |= CSI2_OUTPUT_MEMORY;
- else
+ } else {
csi2-output = ~CSI2_OUTPUT_MEMORY;
+ }
break;
case CSI2_PAD_SOURCE | (MEDIA_ENTITY_TYPE_SUBDEV 16):
- if (flags MEDIA_LINK_FLAG_ACTIVE)
+ if (flags MEDIA_LINK_FLAG_ACTIVE) {
+ if (csi2-output ~CSI2_OUTPUT_MEMORY)
There is a typo here. Please, change ~CSI2_OUTPUT_MEMORY to ~CSI2_OUTPUT_CCDC
here before test it. I'll send a new version after get comments.
Br,
David Cohen
+ return -EBUSY;
csi2-output |= CSI2_OUTPUT_CCDC;
- else
+ } else {
csi2-output = ~CSI2_OUTPUT_CCDC;
+ }
break;
default:
diff --git a/drivers/media/video/isp/isppreview.c
b/drivers/media/video/isp/isppreview.c
index 6274b44..39d4da4 100644
--- a/drivers/media/video/isp/isppreview.c
+++ b/drivers/media/video/isp/isppreview.c
@@ -2026,20 +2026,31 @@ static int preview_link_setup(struct media_entity
*entity,
}
break;
+ /*
+ * This driver currently does not support pipeline with multiples
+ * video outputs. It must return -EBUSY while it's not implemented.
+ */
+
case PREV_PAD_SOURCE | (MEDIA_ENTITY_TYPE_NODE 16):
/* write to memory */
- if (flags MEDIA_LINK_FLAG_ACTIVE)
+ if (flags MEDIA_LINK_FLAG_ACTIVE) {
+ if (prev-output ~PREVIEW_OUTPUT_MEMORY)
+ return -EBUSY;
prev-output |= PREVIEW_OUTPUT_MEMORY;
- else
+ } else {
prev-output = ~PREVIEW_OUTPUT_MEMORY;
+ }
break;
case PREV_PAD_SOURCE | (MEDIA_ENTITY_TYPE_SUBDEV 16):
/* write to resizer */
- if (flags MEDIA_LINK_FLAG_ACTIVE)
+ if (flags MEDIA_LINK_FLAG_ACTIVE) {
+ if (prev-output ~PREVIEW_OUTPUT_RESIZER)
+ return -EBUSY;
prev-output |= PREVIEW_OUTPUT_RESIZER;
- else
+ } else {
prev-output = ~PREVIEW_OUTPUT_RESIZER;
+ }
break;
default:
--
1.7.2.3
--
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: Translation faults with OMAP ISP
On Fri, Nov 19, 2010 at 03:16:41PM +0100, ext Lane Brooks wrote: On 11/19/2010 07:13 AM, Laurent Pinchart wrote: On Friday 19 November 2010 15:08:38 Lane Brooks wrote: On 11/19/2010 06:29 AM, David Cohen wrote: On Thu, Nov 18, 2010 at 12:17:21AM +0100, ext Lane Brooks wrote: On Wednesday 17 November 2010 00:46:27 Lane Brooks wrote: Laurent, I am getting iommu translation errors when I try to use the CCDC output after using the Resizer output. If I use the CCDC output to stream some video, then close it down, switch to the Resizer output and open it up and try to stream, I get the following errors spewing out: omap-iommu omap-iommu.0: omap2_iommu_fault_isr: da:00d0ef00 translation fault omap-iommu omap-iommu.0: iommu_fault_handler: da:00d0ef00 pgd:ce664034 *pgd: and the select times out. From a fresh boot, I can stream just fine from the Resizer and then switch to the CCDC output just fine. It is only when I go from the CCDC to the Resizer that I get this problem. Furthermore, when it gets into this state, then anything dev node I try to use has the translation errors and the only way to recover is to reboot. Any ideas on the problem? I'm not sure if it's your case, but OMAP3 ISP driver does not support pipeline with multiples outputs yet. We have to return error from the driver in this case. If you configured CCDC to write to memory and then to write to preview/resizer afterwards without deactivating the link to write to memory, you may face a similar problem you described. Can you please try a patch I've sent to you (CC'ing linux-media) with subject: [omap3isp][PATCH] omap3isp: does not allow pipeline with multiple video outputs yet? Regards, David David, I am not trying to use multiple outputs simultaneously. I get the translation error with the following sequence: - Open resizer output and setup media links. - Stream some images. - Close resizer. - Reset all media links. - Open CCDC and setup media links. - Try to stream some images but get translation faults. You're describing some different steps from your previous e-mail, as here the iommu faults come while CCDC outputting to memory and in your comment above it was happening while Resizer outputting to memory. Which one should I consider as the correct? :) It would be nice if you could print the values of CCDC_SDR_ADDR and RSZ_SDR_OUTADD just before the bug. Are you also enabling CCDC's LSC? Is your patch going to help with this problem? If you reset all links before setting them up for the CCDC output, probably not (unless you have a bug in your CCDC links setup, but I doubt that). As Laurent said, probably not. But if you want to go ahead to test this patch, that's fine. It's very unlikely we have a bug on CCDC or Resizer link setup, but not completely impossible. :) A new version of this patch fixing the typo I mentioned there is going to be locally applied anyway. I can stream just fine from the CCDC output if I do not use the resizer prior, so I am pretty sure I am setting up the CCDC links correctly. Well, iommu faults mean bug on kernel side. If you're still doing something wrong, the driver must be able to return and error to userland. Regards, David -- 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 -- 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: Translation faults with OMAP ISP
On Fri, Nov 19, 2010 at 04:07:27PM +0100, ext Laurent Pinchart wrote: Hi David, On Friday 19 November 2010 16:06:21 David Cohen wrote: On Fri, Nov 19, 2010 at 03:16:41PM +0100, ext Lane Brooks wrote: On 11/19/2010 07:13 AM, Laurent Pinchart wrote: On Friday 19 November 2010 15:08:38 Lane Brooks wrote: On 11/19/2010 06:29 AM, David Cohen wrote: On Thu, Nov 18, 2010 at 12:17:21AM +0100, ext Lane Brooks wrote: On Wednesday 17 November 2010 00:46:27 Lane Brooks wrote: Laurent, I am getting iommu translation errors when I try to use the CCDC output after using the Resizer output. If I use the CCDC output to stream some video, then close it down, switch to the Resizer output and open it up and try to stream, I get the following errors spewing out: omap-iommu omap-iommu.0: omap2_iommu_fault_isr: da:00d0ef00 translation fault omap-iommu omap-iommu.0: iommu_fault_handler: da:00d0ef00 pgd:ce664034 *pgd: and the select times out. From a fresh boot, I can stream just fine from the Resizer and then switch to the CCDC output just fine. It is only when I go from the CCDC to the Resizer that I get this problem. Furthermore, when it gets into this state, then anything dev node I try to use has the translation errors and the only way to recover is to reboot. Any ideas on the problem? I'm not sure if it's your case, but OMAP3 ISP driver does not support pipeline with multiples outputs yet. We have to return error from the driver in this case. If you configured CCDC to write to memory and then to write to preview/resizer afterwards without deactivating the link to write to memory, you may face a similar problem you described. Can you please try a patch I've sent to you (CC'ing linux-media) with subject: [omap3isp][PATCH] omap3isp: does not allow pipeline with multiple video outputs yet? Regards, David David, I am not trying to use multiple outputs simultaneously. I get the translation error with the following sequence: - Open resizer output and setup media links. - Stream some images. - Close resizer. - Reset all media links. - Open CCDC and setup media links. - Try to stream some images but get translation faults. You're describing some different steps from your previous e-mail, as here the iommu faults come while CCDC outputting to memory and in your comment above it was happening while Resizer outputting to memory. Which one should I consider as the correct? :) It would be nice if you could print the values of CCDC_SDR_ADDR and RSZ_SDR_OUTADD just before the bug. Are you also enabling CCDC's LSC? Is your patch going to help with this problem? If you reset all links before setting them up for the CCDC output, probably not (unless you have a bug in your CCDC links setup, but I doubt that). As Laurent said, probably not. But if you want to go ahead to test this patch, that's fine. It's very unlikely we have a bug on CCDC or Resizer link setup, but not completely impossible. :) A new version of this patch fixing the typo I mentioned there is going to be locally applied anyway. I can stream just fine from the CCDC output if I do not use the resizer prior, so I am pretty sure I am setting up the CCDC links correctly. Well, iommu faults mean bug on kernel side. If you're still doing something wrong, the driver must be able to return and error to userland. Don't forget that Lane is using an older version of the OMAP3 ISP driver. The bug might have been fixed in the latest code. Hm. We did fix some iommu faults. Maybe it's better to test a newer version instead. If you still see that bug using an up-to-date version, please report it and I can try to help you. :) Regards, David -- 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: [omap3isp][PATCH v2 5/9] omap3isp: Remove unused CBUFF register access
On Fri, Nov 19, 2010 at 04:54:01PM +0100, ext Aguirre, Sergio wrote: Hi Laurent and David, Hi Sergio, -Original Message- From: Laurent Pinchart [mailto:laurent.pinch...@ideasonboard.com] Sent: Friday, November 19, 2010 4:33 AM To: David Cohen Cc: Aguirre, Sergio; linux-media@vger.kernel.org Subject: Re: [omap3isp][PATCH v2 5/9] omap3isp: Remove unused CBUFF register access Hi David, On Friday 19 November 2010 11:19:44 David Cohen wrote: On Mon, Nov 15, 2010 at 03:29:57PM +0100, ext Sergio Aguirre wrote: [snip] @@ -244,26 +239,6 @@ #define ISP_CSIB_SYSCONFIG ISPCCP2_SYSCONFIG #define ISP_CSIA_SYSCONFIG ISPCSI2_SYSCONFIG -/* ISP_CBUFF Registers */ - -#define ISP_CBUFF_SYSCONFIG(0x010) -#define ISP_CBUFF_IRQENABLE(0x01C) - -#define ISP_CBUFF0_CTRL(0x020) -#define ISP_CBUFF1_CTRL(0x024) - -#define ISP_CBUFF0_START (0x040) -#define ISP_CBUFF1_START (0x044) - -#define ISP_CBUFF0_END (0x050) -#define ISP_CBUFF1_END (0x054) - -#define ISP_CBUFF0_WINDOWSIZE (0x060) -#define ISP_CBUFF1_WINDOWSIZE (0x064) - -#define ISP_CBUFF0_THRESHOLD (0x070) -#define ISP_CBUFF1_THRESHOLD (0x074) - No need to remove the registers from header file. We're not using them on current version, but it doesn't mean we won't use ever. :) I would have made the same comment for other registers, but we're not using the CBUFF module at all here, with no plans to use it in the future. It might not be worth it keeping the register definitions. I have no strong feeling about it, I'm fine with both choices. David, IMO, we should not introduce dead code/unusued defines in the first omap3isp upstream version. I think it's already quite hard to review for somebody outside the omap3isp development team. Having all this just in case will most probably end up in bulk, as we might never implement the CBUFF HW block, as Laurent mentions. That's a good point. I see no problem in removing it in that case. Br, David I'll be more biased on all this being re-added if we end up implementing a ispcbuff submodule. Regards, Sergio -- 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 -- 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] v4l2: change kmalloc to vmalloc for sglist allocation in videobuf_dma_map/unmap
On Thu, May 14, 2009 at 09:04:48AM +0200, Cohen David.A (Nokia-D/Helsinki)
wrote:
From: Cohen David.A (Nokia-D/Helsinki) david.co...@nokia.com
Change kmalloc()/kfree() to vmalloc()/vfree() for sglist allocation
during videobuf_dma_map() and videobuf_dma_unmap()
High resolution sensors might require too many contiguous pages
to be allocated for sglist by kmalloc() during videobuf_dma_map()
(i.e. 256Kib for 8MP sensor).
In such situations, kmalloc() could face some problem to find the
required free memory. vmalloc() is a safer solution instead, as the
allocated memory does not need to be contiguous.
Signed-off-by: David Cohen david.co...@nokia.com
---
drivers/media/video/videobuf-dma-sg.c | 17 +
1 files changed, 9 insertions(+), 8 deletions(-)
Hi Mauro,
Any comments?
Br,
David Cohen
diff --git a/drivers/media/video/videobuf-dma-sg.c
b/drivers/media/video/videobuf-dma-sg.c
index da1790e..c9a5d7e 100644
--- a/drivers/media/video/videobuf-dma-sg.c
+++ b/drivers/media/video/videobuf-dma-sg.c
@@ -58,9 +58,10 @@ videobuf_vmalloc_to_sg(unsigned char *virt, int nr_pages)
struct page *pg;
int i;
- sglist = kcalloc(nr_pages, sizeof(struct scatterlist), GFP_KERNEL);
+ sglist = vmalloc(nr_pages * sizeof(*sglist));
if (NULL == sglist)
return NULL;
+ memset(sglist, 0, nr_pages * sizeof(*sglist));
sg_init_table(sglist, nr_pages);
for (i = 0; i nr_pages; i++, virt += PAGE_SIZE) {
pg = vmalloc_to_page(virt);
@@ -72,7 +73,7 @@ videobuf_vmalloc_to_sg(unsigned char *virt, int nr_pages)
return sglist;
err:
- kfree(sglist);
+ vfree(sglist);
return NULL;
}
@@ -84,7 +85,7 @@ videobuf_pages_to_sg(struct page **pages, int nr_pages, int
offset)
if (NULL == pages[0])
return NULL;
- sglist = kmalloc(nr_pages * sizeof(*sglist), GFP_KERNEL);
+ sglist = vmalloc(nr_pages * sizeof(*sglist));
if (NULL == sglist)
return NULL;
sg_init_table(sglist, nr_pages);
@@ -104,12 +105,12 @@ videobuf_pages_to_sg(struct page **pages, int nr_pages,
int offset)
nopage:
dprintk(2,sgl: oops - no page\n);
- kfree(sglist);
+ vfree(sglist);
return NULL;
highmem:
dprintk(2,sgl: oops - highmem page\n);
- kfree(sglist);
+ vfree(sglist);
return NULL;
}
@@ -230,7 +231,7 @@ int videobuf_dma_map(struct videobuf_queue* q, struct
videobuf_dmabuf *dma)
(dma-vmalloc,dma-nr_pages);
}
if (dma-bus_addr) {
- dma-sglist = kmalloc(sizeof(struct scatterlist), GFP_KERNEL);
+ dma-sglist = vmalloc(sizeof(*dma-sglist));
if (NULL != dma-sglist) {
dma-sglen = 1;
sg_dma_address(dma-sglist[0]) = dma-bus_addr
PAGE_MASK;
@@ -248,7 +249,7 @@ int videobuf_dma_map(struct videobuf_queue* q, struct
videobuf_dmabuf *dma)
if (0 == dma-sglen) {
printk(KERN_WARNING
%s: videobuf_map_sg failed\n,__func__);
- kfree(dma-sglist);
+ vfree(dma-sglist);
dma-sglist = NULL;
dma-sglen = 0;
return -EIO;
@@ -274,7 +275,7 @@ int videobuf_dma_unmap(struct videobuf_queue* q,struct
videobuf_dmabuf *dma)
dma_unmap_sg(q-dev, dma-sglist, dma-nr_pages, dma-direction);
- kfree(dma-sglist);
+ vfree(dma-sglist);
dma-sglist = NULL;
dma-sglen = 0;
return 0;
--
1.6.1
--
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] v4l2: change kmalloc to vmalloc for sglist allocation in videobuf_dma_map/unmap
From: Cohen David.A (Nokia-D/Helsinki) david.co...@nokia.com
Change kmalloc()/kfree() to vmalloc()/vfree() for sglist allocation
during videobuf_dma_map() and videobuf_dma_unmap()
High resolution sensors might require too many contiguous pages
to be allocated for sglist by kmalloc() during videobuf_dma_map()
(i.e. 256Kib for 8MP sensor).
In such situations, kmalloc() could face some problem to find the
required free memory. vmalloc() is a safer solution instead, as the
allocated memory does not need to be contiguous.
Signed-off-by: David Cohen david.co...@nokia.com
---
drivers/media/video/videobuf-dma-sg.c | 17 +
1 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/media/video/videobuf-dma-sg.c
b/drivers/media/video/videobuf-dma-sg.c
index da1790e..c9a5d7e 100644
--- a/drivers/media/video/videobuf-dma-sg.c
+++ b/drivers/media/video/videobuf-dma-sg.c
@@ -58,9 +58,10 @@ videobuf_vmalloc_to_sg(unsigned char *virt, int nr_pages)
struct page *pg;
int i;
- sglist = kcalloc(nr_pages, sizeof(struct scatterlist), GFP_KERNEL);
+ sglist = vmalloc(nr_pages * sizeof(*sglist));
if (NULL == sglist)
return NULL;
+ memset(sglist, 0, nr_pages * sizeof(*sglist));
sg_init_table(sglist, nr_pages);
for (i = 0; i nr_pages; i++, virt += PAGE_SIZE) {
pg = vmalloc_to_page(virt);
@@ -72,7 +73,7 @@ videobuf_vmalloc_to_sg(unsigned char *virt, int nr_pages)
return sglist;
err:
- kfree(sglist);
+ vfree(sglist);
return NULL;
}
@@ -84,7 +85,7 @@ videobuf_pages_to_sg(struct page **pages, int nr_pages, int
offset)
if (NULL == pages[0])
return NULL;
- sglist = kmalloc(nr_pages * sizeof(*sglist), GFP_KERNEL);
+ sglist = vmalloc(nr_pages * sizeof(*sglist));
if (NULL == sglist)
return NULL;
sg_init_table(sglist, nr_pages);
@@ -104,12 +105,12 @@ videobuf_pages_to_sg(struct page **pages, int nr_pages,
int offset)
nopage:
dprintk(2,sgl: oops - no page\n);
- kfree(sglist);
+ vfree(sglist);
return NULL;
highmem:
dprintk(2,sgl: oops - highmem page\n);
- kfree(sglist);
+ vfree(sglist);
return NULL;
}
@@ -230,7 +231,7 @@ int videobuf_dma_map(struct videobuf_queue* q, struct
videobuf_dmabuf *dma)
(dma-vmalloc,dma-nr_pages);
}
if (dma-bus_addr) {
- dma-sglist = kmalloc(sizeof(struct scatterlist), GFP_KERNEL);
+ dma-sglist = vmalloc(sizeof(*dma-sglist));
if (NULL != dma-sglist) {
dma-sglen = 1;
sg_dma_address(dma-sglist[0]) = dma-bus_addr
PAGE_MASK;
@@ -248,7 +249,7 @@ int videobuf_dma_map(struct videobuf_queue* q, struct
videobuf_dmabuf *dma)
if (0 == dma-sglen) {
printk(KERN_WARNING
%s: videobuf_map_sg failed\n,__func__);
- kfree(dma-sglist);
+ vfree(dma-sglist);
dma-sglist = NULL;
dma-sglen = 0;
return -EIO;
@@ -274,7 +275,7 @@ int videobuf_dma_unmap(struct videobuf_queue* q,struct
videobuf_dmabuf *dma)
dma_unmap_sg(q-dev, dma-sglist, dma-nr_pages, dma-direction);
- kfree(dma-sglist);
+ vfree(dma-sglist);
dma-sglist = NULL;
dma-sglen = 0;
return 0;
--
1.6.1
--
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
