Hi,
On 30.06.20 14:05, Jacopo Mondi wrote:
Hi Dafna,
On Tue, Jun 30, 2020 at 12:56:44PM +0200, Dafna Hirschfeld wrote:
On 30.06.20 12:06, Jacopo Mondi wrote:
Hi Dafna,
On Tue, Jun 30, 2020 at 11:32:12AM +0200, Dafna Hirschfeld wrote:
On 30.06.20 09:43, Jacopo Mondi wrote:
Hi Dafna,
On Mon, Jun 29, 2020 at 07:48:16PM +0200, Dafna Hirschfeld wrote:
On 23.06.20 18:55, Jacopo Mondi wrote:
Store in the driver structure a pointer to the currently applied mode
and program a new one at s_stream(1) time only if it has changed.
Hi,
I think this can be more readably implemented with a 'is_streaming' boolean
field.
How would you like to use an 'is_streaming' flag to decide if the
sensor mode has to be updated ?
since 'current_mode' is set to NULL upon `ov5647_stream_off`
and you return from 'ov5647_set_stream' immediately if 'mode == current_mode'
it seem very similar to returning immediately from 'ov5647_set_stream' if the
device is currently streaming.
No, the code returns immediately from ov5647_set_mode() if mode ==
current_mode. The modes comparison makes sure the sensor is not
reprogrammed if the mode hasn't changed. The remaning part of
s_stream() is executed regardless of the mode configuration. Am I
missing some part of the picture ?
But actually in this patch it seems to be possible to change the mode
while streaming, if the callbacks are executed:
s_stream(1)
s_fmt
s_stream(1)
which is maybe a bug?
The new format is stored in sensor->mode, and applied at the next
s_stream(1) operation if it differs from the already programmed one,
at least, this is how it is intended to work, have you found any
failing s_stream/set_fmt/s_stream which could be caused by a bug ?
What I meant is that there could be valid sequence of calls
s_stream(enable=1)
s_fmt
s_stream(enable=1)
For example if two video devices are connected to the sensor and they
stream simultaneously. There was a discussion about adding a code to the core
I'm not sure it is possible, given that the subdev has a single source
pad
Video devices should not be connected directly to the sensor, they can also
be connected to the sensor through an isp entity that is connected to the sensor
from one side and to two video devices from the other side.
Thanks,
D
to follow the s_stream callback and call it only if the subdev is not streaming
but currently subdevs should support it themselves.
Oh, so you're concerned about the fact userspace can call s_stream(1)
twice consecutively! it's indipendent from s_ftm if I got your point.
In this case a flag to control if the device is streaming already should
help, yes, I overlooked that indeed.
Thanks
j
Thanks,
Dafna
Thanks
j
Thanks,
Dafna
Thanks
j
Thanks,
Dafna
Signed-off-by: Jacopo Mondi <jacopo@xxxxxxxxxx>
---
drivers/media/i2c/ov5647.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
index 39e320f321bd8..ac114269e1c73 100644
--- a/drivers/media/i2c/ov5647.c
+++ b/drivers/media/i2c/ov5647.c
@@ -96,6 +96,7 @@ struct ov5647 {
bool clock_ncont;
struct v4l2_ctrl_handler ctrls;
struct ov5647_mode *mode;
+ struct ov5647_mode *current_mode;
};
static inline struct ov5647 *to_sensor(struct v4l2_subdev *sd)
@@ -750,9 +751,13 @@ static int ov5647_set_virtual_channel(struct v4l2_subdev *sd, int channel)
static int ov5647_set_mode(struct v4l2_subdev *sd)
{
struct i2c_client *client = v4l2_get_subdevdata(sd);
+ struct ov5647 *sensor = to_sensor(sd);
u8 resetval, rdval;
int ret;
+ if (sensor->mode == sensor->current_mode)
+ return 0;
+
ret = ov5647_read(sd, OV5647_SW_STANDBY, &rdval);
if (ret < 0)
return ret;
@@ -778,6 +783,8 @@ static int ov5647_set_mode(struct v4l2_subdev *sd)
return ret;
}
+ sensor->current_mode = sensor->mode;
+
return 0;
}
@@ -816,6 +823,7 @@ static int ov5647_stream_on(struct v4l2_subdev *sd)
static int ov5647_stream_off(struct v4l2_subdev *sd)
{
+ struct ov5647 *sensor = to_sensor(sd);
int ret;
ret = ov5647_write(sd, OV5647_REG_MIPI_CTRL00, MIPI_CTRL00_CLOCK_LANE_GATE |
@@ -827,7 +835,13 @@ static int ov5647_stream_off(struct v4l2_subdev *sd)
if (ret < 0)
return ret;
- return ov5647_write(sd, OV5640_REG_PAD_OUT, 0x01);
+ ret = ov5647_write(sd, OV5640_REG_PAD_OUT, 0x01);
+ if (ret < 0)
+ return ret;
+
+ sensor->current_mode = NULL;
+
+ return 0;
}
static int set_sw_standby(struct v4l2_subdev *sd, bool standby)
