[20/25] media: ov5647: Program mode only if it has changed

Message ID 20200623165550.45835-1-jacopo@jmondi.org (mailing list archive)
State Superseded, archived
Delegated to: Sakari Ailus
Headers
Series media: ov5647: Support RaspberryPi Camera Module v1 |

Commit Message

Jacopo Mondi June 23, 2020, 4:55 p.m. UTC
  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.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/i2c/ov5647.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)
  

Comments

Dafna Hirschfeld June 29, 2020, 5:48 p.m. UTC | #1
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.

Thanks,
Dafna

> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>   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)
>
  
Jacopo Mondi June 30, 2020, 7:43 a.m. UTC | #2
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 ?

Thanks
   j


>
> Thanks,
> Dafna
>
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >   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)
> >
  
Dafna Hirschfeld June 30, 2020, 9:32 a.m. UTC | #3
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.

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?

Thanks,
Dafna

> 
> Thanks
>     j
> 
> 
>>
>> Thanks,
>> Dafna
>>
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>>> ---
>>>    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)
>>>
  
Jacopo Mondi June 30, 2020, 10:06 a.m. UTC | #4
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 ?

Thanks
  j
>
> Thanks,
> Dafna
>
> >
> > Thanks
> >     j
> >
> >
> > >
> > > Thanks,
> > > Dafna
> > >
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > ---
> > > >    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)
> > > >
  
Dafna Hirschfeld June 30, 2020, 10:56 a.m. UTC | #5
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
to follow the s_stream callback and call it only if the subdev is not streaming
but currently subdevs should support it themselves.


Thanks,
Dafna

> 
> Thanks
>    j
>>
>> Thanks,
>> Dafna
>>
>>>
>>> Thanks
>>>      j
>>>
>>>
>>>>
>>>> Thanks,
>>>> Dafna
>>>>
>>>>>
>>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>>>>> ---
>>>>>     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)
>>>>>
  
Jacopo Mondi June 30, 2020, 12:05 p.m. UTC | #6
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

> 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@jmondi.org>
> > > > > > ---
> > > > > >     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)
> > > > > >
  
Dafna Hirschfeld June 30, 2020, 1:01 p.m. UTC | #7
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@jmondi.org>
>>>>>>> ---
>>>>>>>      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)
>>>>>>>
  
Dave Stevenson June 30, 2020, 2:53 p.m. UTC | #8
Hi Dafna

On Tue, 30 Jun 2020 at 14:01, Dafna Hirschfeld
<dafna.hirschfeld@collabora.com> wrote:
>
> 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,

That's an odd statement as it is exactly the situation we have on the
Pi. The CSI2 receiver writes data to RAM, therefore it is a video
device.
Did you intend to say that it isn't necessarily 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.

It's true that some platforms will route the received CSI2 data
straight through the ISP, and only write the processed image(s) to
RAM. If there are multiple output video devices from the ISP then yes
they will VIDIOC_STREAMON at different points.
However is it really valid to call s_stream(1) on the sensor subdev
for each of them? Doesn't that mean you really need refcounting of the
number of s_stream(1)'s (and s_stream(0)'s) within each sensor driver,
so that it only actually stops streaming on the last s_stream(0), not
the first. A simple boolean isn't sufficient, otherwise
VIDIOC_STREAMON(NODE_A);
VIDIOC_STREAMON(NODE_B);
VIDIOC_STREAMOFF(NODE_B);
leaves you with no data from NODE_A even though it has never called
VIDIOC_STREAMOFF.

Anyway this patch was to remove excess writing of the mode registers if you did
s_fmt
s_stream(1)
s_stream(0)
s_stream(1)

The driver uses the s_power call rather than pm_runtime as that was an
acceptable approach when it was written in 2017, so the power, and
hence register settings, can be maintained between multiple s_stream
calls.

  Dave

> 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@jmondi.org>
> >>>>>>> ---
> >>>>>>>      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)
> >>>>>>>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
  
Dafna Hirschfeld June 30, 2020, 3:11 p.m. UTC | #9
On 30.06.20 16:53, Dave Stevenson wrote:
> Hi Dafna
> 
> On Tue, 30 Jun 2020 at 14:01, Dafna Hirschfeld
> <dafna.hirschfeld@collabora.com> wrote:
>>
>> 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,
> 
> That's an odd statement as it is exactly the situation we have on the
> Pi. The CSI2 receiver writes data to RAM, therefore it is a video
> device.
> Did you intend to say that it isn't necessarily connected directly to
> the sensor?
Yes, sorry, I meant, "they don't have to" (but they can).

> 
>> 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.
> 
> It's true that some platforms will route the received CSI2 data
> straight through the ISP, and only write the processed image(s) to
> RAM. If there are multiple output video devices from the ISP then yes
> they will VIDIOC_STREAMON at different points.
> However is it really valid to call s_stream(1) on the sensor subdev
> for each of them? Doesn't that mean you really need refcounting of the
> number of s_stream(1)'s (and s_stream(0)'s) within each sensor driver,
> so that it only actually stops streaming on the last s_stream(0), not
> the first. A simple boolean isn't sufficient, otherwise
> VIDIOC_STREAMON(NODE_A);
> VIDIOC_STREAMON(NODE_B);
> VIDIOC_STREAMOFF(NODE_B);
> leaves you with no data from NODE_A even though it has never called
> VIDIOC_STREAMOFF.

oh, right, there was a proposal to add functions that do it with refcounting

https://patchwork.kernel.org/project/linux-media/list/?series=271153

Dafna


> 
> Anyway this patch was to remove excess writing of the mode registers if you did
> s_fmt
> s_stream(1)
> s_stream(0)
> s_stream(1)
> 
> The driver uses the s_power call rather than pm_runtime as that was an
> acceptable approach when it was written in 2017, so the power, and
> hence register settings, can be maintained between multiple s_stream
> calls.
> 
>    Dave
> 
>> 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@jmondi.org>
>>>>>>>>> ---
>>>>>>>>>       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)
>>>>>>>>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel@lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
  
Laurent Pinchart July 1, 2020, 7:25 a.m. UTC | #10
Hi Dafna,

On Tue, Jun 30, 2020 at 03:01:21PM +0200, Dafna Hirschfeld wrote:
> On 30.06.20 14:05, Jacopo Mondi wrote:
> > On Tue, Jun 30, 2020 at 12:56:44PM +0200, Dafna Hirschfeld wrote:
> >> On 30.06.20 12:06, Jacopo Mondi wrote:
> >>> On Tue, Jun 30, 2020 at 11:32:12AM +0200, Dafna Hirschfeld wrote:
> >>>> On 30.06.20 09:43, Jacopo Mondi wrote:
> >>>>> 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.

I don't think it should be the job of the sensor driver to handle this.
A sensor can be streaming or not streaming, and a .s_stream(1) call
while already streaming shouldn't happen. It's the job of the ISP driver
(with help from core code) to ensure this won't happen. Otherwise we
would have to protect against that case in all sensor drivers,
duplicating code in many places and opening the door to bugs. Subdev
drivers should be as simple as possible.

> >> 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.
> > 
> >>>>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >>>>>>> ---
> >>>>>>>      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)
> >>>>>>>
  
Jacopo Mondi July 3, 2020, 12:21 p.m. UTC | #11
Hello,

On Wed, Jul 01, 2020 at 10:25:54AM +0300, Laurent Pinchart wrote:
> Hi Dafna,
>
> On Tue, Jun 30, 2020 at 03:01:21PM +0200, Dafna Hirschfeld wrote:
> > On 30.06.20 14:05, Jacopo Mondi wrote:
> > > On Tue, Jun 30, 2020 at 12:56:44PM +0200, Dafna Hirschfeld wrote:
> > >> On 30.06.20 12:06, Jacopo Mondi wrote:
> > >>> On Tue, Jun 30, 2020 at 11:32:12AM +0200, Dafna Hirschfeld wrote:
> > >>>> On 30.06.20 09:43, Jacopo Mondi wrote:
> > >>>>> 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.
>
> I don't think it should be the job of the sensor driver to handle this.
> A sensor can be streaming or not streaming, and a .s_stream(1) call
> while already streaming shouldn't happen. It's the job of the ISP driver
> (with help from core code) to ensure this won't happen. Otherwise we
> would have to protect against that case in all sensor drivers,
> duplicating code in many places and opening the door to bugs. Subdev
> drivers should be as simple as possible.
>

Most of the sensor driver I've briefly looked at implement a simple
check to avoid double stream(1) but they do not implement any form of
refcounting. I think that does more harm than good to be honest, as it
would hide  potential problematic start stream sequences, but would
stop the sensor at the first stream(0), leaving one of the multiple
receivers stuck.

I would prefer avoid doing this here.

However the driver already refcounting on s_power(), which if I'm not
mistaken could be avoided, as bridges should use
v4l2_pipeline_pm_get(), which already does refcounting, if I'm not
mistaken.

To me, grasping how s_stream() and s_start() work for real is still hard,
as those are the only two operation propagated along the pipeline by
briges, even for MC platforms, and it seems looking at the existing
driver, the confusion is big, as all of them handle things slightly
differently :/


> > >> 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.
> > >
> > >>>>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > >>>>>>> ---
> > >>>>>>>      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)
> > >>>>>>>
>
> --
> Regards,
>
> Laurent Pinchart
  
Laurent Pinchart July 3, 2020, 4:33 p.m. UTC | #12
Hi Jacopo,

On Fri, Jul 03, 2020 at 02:21:50PM +0200, Jacopo Mondi wrote:
> On Wed, Jul 01, 2020 at 10:25:54AM +0300, Laurent Pinchart wrote:
> > On Tue, Jun 30, 2020 at 03:01:21PM +0200, Dafna Hirschfeld wrote:
> >> On 30.06.20 14:05, Jacopo Mondi wrote:
> >>> On Tue, Jun 30, 2020 at 12:56:44PM +0200, Dafna Hirschfeld wrote:
> >>>> On 30.06.20 12:06, Jacopo Mondi wrote:
> >>>>> On Tue, Jun 30, 2020 at 11:32:12AM +0200, Dafna Hirschfeld wrote:
> >>>>>> On 30.06.20 09:43, Jacopo Mondi wrote:
> >>>>>>> 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.
> >
> > I don't think it should be the job of the sensor driver to handle this.
> > A sensor can be streaming or not streaming, and a .s_stream(1) call
> > while already streaming shouldn't happen. It's the job of the ISP driver
> > (with help from core code) to ensure this won't happen. Otherwise we
> > would have to protect against that case in all sensor drivers,
> > duplicating code in many places and opening the door to bugs. Subdev
> > drivers should be as simple as possible.
> 
> Most of the sensor driver I've briefly looked at implement a simple
> check to avoid double stream(1) but they do not implement any form of
> refcounting. I think that does more harm than good to be honest, as it
> would hide  potential problematic start stream sequences, but would
> stop the sensor at the first stream(0), leaving one of the multiple
> receivers stuck.
> 
> I would prefer avoid doing this here.
> 
> However the driver already refcounting on s_power(), which if I'm not
> mistaken could be avoided, as bridges should use
> v4l2_pipeline_pm_get(), which already does refcounting, if I'm not
> mistaken.

.s_power() can also be called when opening the subdev device node from
userspace, through sd->internal_ops->open(). In new drivers, I'd
recommend implementing .s_power() based on runtime PM, with .s_power(1)
calling pm_runtime_get_sync() and .s_power(0) calling pm_runtime_put().
.s_stream() should call the runtime PM functions too. That way all the
refcounting will be handled by runtime PM.

.s_stream() should not be refcounted, the caller should ensure that a
started sensor doesn't get started again and that a stopped sensor
doesn't receive a .s_stream(0) call.

> To me, grasping how s_stream() and s_start() work for real is still hard,
> as those are the only two operation propagated along the pipeline by
> briges, even for MC platforms, and it seems looking at the existing
> driver, the confusion is big, as all of them handle things slightly
> differently :/

None of this has ever been really documented, and APIs have evolved over
time without fixing drivers, hence today's mess.

> >>>> 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.
> >>>
> >>>>>>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >>>>>>>>> ---
> >>>>>>>>>      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)
> >>>>>>>>>
  

Patch

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)