Linux-Media Archive on lore.kernel.org
 help / color / Atom feed
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
Cc: Jacopo Mondi <jacopo@jmondi.org>,
	mchehab@kernel.org, sakari.ailus@linux.intel.com,
	hverkuil@xs4all.nl, roman.kovalivskyi@globallogic.com,
	dave.stevenson@raspberrypi.org, naush@raspberrypi.com,
	mrodin@de.adit-jv.com, hugues.fruchet@st.com, mripard@kernel.org,
	aford173@gmail.com, sudipi@jp.adit-jv.com,
	andrew_gabbasov@mentor.com, erosca@de.adit-jv.com,
	linux-media@vger.kernel.org, libcamera-devel@lists.libcamera.org,
	dafna Hirschfeld <dafna3@gmail.com>
Subject: Re: [PATCH 20/25] media: ov5647: Program mode only if it has changed
Date: Wed, 1 Jul 2020 10:25:54 +0300
Message-ID: <20200701072554.GH5963@pendragon.ideasonboard.com> (raw)
In-Reply-To: <ae93796f-dd9d-730b-008a-13f90ff1f5cd@collabora.com>

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)
> >>>>>>>

-- 
Regards,

Laurent Pinchart

  parent reply index

Thread overview: 55+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-23 10:07 [PATCH 00/25] media: ov5647: Support RaspberryPi Camera Module v1 Jacopo Mondi
2020-06-23 10:07 ` [PATCH 01/25] dt-bindings: media: ov5647: Document pwdn-gpios Jacopo Mondi
2020-06-23 10:07 ` [PATCH 02/25] dt-bindings: media: ov5647: Document clock-noncontinuous Jacopo Mondi
2020-06-23 10:07 ` [PATCH 03/25] media: ov5647: Add support for PWDN GPIO Jacopo Mondi
2020-07-09 20:04   ` [libcamera-devel] " Dafna Hirschfeld
2020-07-14 12:45     ` Jacopo Mondi
2020-06-23 10:07 ` [PATCH 04/25] media: ov5647: Add support for non-continuous clock mode Jacopo Mondi
2020-06-23 10:07 ` [PATCH 05/25] media: ov5647: Add set_fmt and get_fmt calls Jacopo Mondi
2020-06-23 10:07 ` [PATCH 06/25] media: ov5647: Fix format initialization Jacopo Mondi
2020-06-23 10:07 ` [PATCH 07/25] media: ov5647: Fix style issues Jacopo Mondi
2020-06-23 10:07 ` [PATCH 08/25] media: ov5647: Replace license with SPDX identifier Jacopo Mondi
2020-06-23 10:07 ` [PATCH 09/25] media: ov5647: Fix return value from read/write Jacopo Mondi
2020-06-23 10:08 ` [PATCH 10/25] media: ov5647: Program mode at s_stream(1) time Jacopo Mondi
2020-06-23 16:42 ` [PATCH 11/25] media: ov5647: Implement enum_frame_size() Jacopo Mondi
2020-08-18  7:33   ` Sakari Ailus
2020-08-18  7:38     ` Sakari Ailus
2020-06-23 16:42 ` [PATCH 12/25] media: ov5647: Protect s_stream() with mutex Jacopo Mondi
2020-06-23 16:42 ` [PATCH 13/25] media: ov5647: Support gain, exposure and AWB controls Jacopo Mondi
2020-06-23 16:42 ` [PATCH 14/25] media: ov5647: Rationalize driver structure name Jacopo Mondi
2020-06-23 16:42 ` [PATCH 15/25] media: ov5647: Break out format handling Jacopo Mondi
2020-07-31 11:44   ` Sakari Ailus
2020-06-23 16:49 ` [PATCH 16/25] media: ov5647: Add support for get_selection() Jacopo Mondi
2020-07-09 19:56   ` [libcamera-devel] " Dafna Hirschfeld
2020-06-23 16:49 ` [PATCH 17/25] media: ov5647: Rename SBGGR8 VGA mode Jacopo Mondi
2020-06-23 16:49 ` [PATCH 18/25] media: ov5647: Add SGGBR10_1X10 modes Jacopo Mondi
2020-06-23 16:49 ` [PATCH 19/25] media: ov5647: Implement set_fmt pad operation Jacopo Mondi
2020-06-29 16:54   ` [libcamera-devel] " Dafna Hirschfeld
2020-06-30 10:13     ` Jacopo Mondi
2020-06-30 11:09       ` Dafna Hirschfeld
2020-06-23 16:55 ` [PATCH 20/25] media: ov5647: Program mode only if it has changed Jacopo Mondi
2020-06-29 17:48   ` Dafna Hirschfeld
2020-06-30  7:43     ` Jacopo Mondi
2020-06-30  9:32       ` Dafna Hirschfeld
2020-06-30 10:06         ` Jacopo Mondi
2020-06-30 10:56           ` Dafna Hirschfeld
2020-06-30 12:05             ` Jacopo Mondi
2020-06-30 13:01               ` Dafna Hirschfeld
2020-06-30 14:53                 ` [libcamera-devel] " Dave Stevenson
2020-06-30 15:11                   ` Dafna Hirschfeld
2020-07-01  7:25                 ` Laurent Pinchart [this message]
2020-07-03 12:21                   ` Jacopo Mondi
2020-07-03 16:33                     ` Laurent Pinchart
2020-06-23 16:55 ` [PATCH 21/25] media: ov5647: Set V4L2_SUBDEV_FL_HAS_EVENTS flag Jacopo Mondi
2020-06-23 16:55 ` [PATCH 22/25] media: ov5647: Support V4L2_CID_PIXEL_RATE Jacopo Mondi
2020-06-29 17:01   ` [libcamera-devel] " Dafna Hirschfeld
2020-06-29 21:25     ` Dave Stevenson
2020-06-30 15:13       ` Dave Stevenson
2020-07-01  7:21         ` Laurent Pinchart
2020-06-23 16:55 ` [PATCH 23/25] media: ov5647: Support V4L2_CID_HBLANK control Jacopo Mondi
2020-06-23 16:55 ` [PATCH 24/25] media: ov5647: Support V4L2_CID_VBLANK control Jacopo Mondi
2020-06-23 16:55 ` [PATCH 25/25] media: ov5647: Advertise the correct exposure range Jacopo Mondi
2020-06-29 17:33 ` [PATCH 00/25] media: ov5647: Support RaspberryPi Camera Module v1 Dafna Hirschfeld
2020-06-29 18:08   ` Dafna Hirschfeld
2020-07-10 15:59 ` Dafna Hirschfeld
2020-07-14 12:48   ` Jacopo Mondi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200701072554.GH5963@pendragon.ideasonboard.com \
    --to=laurent.pinchart@ideasonboard.com \
    --cc=aford173@gmail.com \
    --cc=andrew_gabbasov@mentor.com \
    --cc=dafna.hirschfeld@collabora.com \
    --cc=dafna3@gmail.com \
    --cc=dave.stevenson@raspberrypi.org \
    --cc=erosca@de.adit-jv.com \
    --cc=hugues.fruchet@st.com \
    --cc=hverkuil@xs4all.nl \
    --cc=jacopo@jmondi.org \
    --cc=libcamera-devel@lists.libcamera.org \
    --cc=linux-media@vger.kernel.org \
    --cc=mchehab@kernel.org \
    --cc=mripard@kernel.org \
    --cc=mrodin@de.adit-jv.com \
    --cc=naush@raspberrypi.com \
    --cc=roman.kovalivskyi@globallogic.com \
    --cc=sakari.ailus@linux.intel.com \
    --cc=sudipi@jp.adit-jv.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Media Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-media/0 linux-media/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-media linux-media/ https://lore.kernel.org/linux-media \
		linux-media@vger.kernel.org
	public-inbox-index linux-media

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-media


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git