| Message ID | 8E0971CCB6EA9D41AF58191A2D3978B61D4E49E8@PGSMSX111.gar.corp.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Andy, On Fri, Jan 19, 2018 at 07:29:46AM +0000, Yeh, Andy wrote: > Thanks Tomasz, > > Agree with your point, if so, we could just change as below with a simple check of streaming flag. > And for Sakari, do you agree with Tomasz's comment? > > Kindly review and I would send v5 with the change. > > diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c > index a7e58bd2..cf1c5ee 100644 > --- a/drivers/media/i2c/imx258.c > +++ b/drivers/media/i2c/imx258.c > @@ -561,10 +561,13 @@ static int imx258_set_ctrl(struct v4l2_ctrl *ctrl) > > /* > * Applying V4L2 control value only happens > - * when power is up for qstreaming > + * when streaming flag is on > */ > - if (pm_runtime_get_if_in_use(&client->dev) <= 0) > + if (imx258->streaming == 0) This doesn't address the problem yet. I think we'll need one more field in the device specific struct to convey this to the driver. Please see the smiapp driver, and its use of "active" field. It's a little different implementation, you could well put the check here rather than the function performing the writes. This isn't a severe issue though, in practice it'll be unlikely to be noticed as it hasn't been noticed in some other drivers that use the same pattern. IMO this could be addressed later on, possibly together with other drivers with similar issues. > return 0; > > switch (ctrl->id) { > case V4L2_CID_ANALOGUE_GAIN: > @@ -590,8 +593,6 @@ static int imx258_set_ctrl(struct v4l2_ctrl *ctrl) > break; > } > > - pm_runtime_put(&client->dev); > - > return ret; > }
Hi Sakari, Tomasz, As below discussion that other drivers are with this pattern, I would prefer to defer to address the concern in later discussion with you and owners of other sensors. Thanks a lot. Regards, Andy -----Original Message----- From: Sakari Ailus [mailto:sakari.ailus@linux.intel.com] Sent: Friday, January 19, 2018 5:18 PM To: Yeh, Andy <andy.yeh@intel.com> Cc: Tomasz Figa <tfiga@chromium.org>; Linux Media Mailing List <linux-media@vger.kernel.org> Subject: Re: [PATCH v4] media: imx258: Add imx258 camera sensor driver Hi Andy, On Fri, Jan 19, 2018 at 07:29:46AM +0000, Yeh, Andy wrote: > Thanks Tomasz, > > Agree with your point, if so, we could just change as below with a simple check of streaming flag. > And for Sakari, do you agree with Tomasz's comment? > > Kindly review and I would send v5 with the change. > > diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c > index a7e58bd2..cf1c5ee 100644 > --- a/drivers/media/i2c/imx258.c > +++ b/drivers/media/i2c/imx258.c > @@ -561,10 +561,13 @@ static int imx258_set_ctrl(struct v4l2_ctrl > *ctrl) > > /* > * Applying V4L2 control value only happens > - * when power is up for qstreaming > + * when streaming flag is on > */ > - if (pm_runtime_get_if_in_use(&client->dev) <= 0) > + if (imx258->streaming == 0) This doesn't address the problem yet. I think we'll need one more field in the device specific struct to convey this to the driver. Please see the smiapp driver, and its use of "active" field. It's a little different implementation, you could well put the check here rather than the function performing the writes. This isn't a severe issue though, in practice it'll be unlikely to be noticed as it hasn't been noticed in some other drivers that use the same pattern. IMO this could be addressed later on, possibly together with other drivers with similar issues. > return 0; > > switch (ctrl->id) { > case V4L2_CID_ANALOGUE_GAIN: > @@ -590,8 +593,6 @@ static int imx258_set_ctrl(struct v4l2_ctrl *ctrl) > break; > } > > - pm_runtime_put(&client->dev); > - > return ret; > } -- Kind regards, Sakari Ailus sakari.ailus@linux.intel.com
On Mon, Jan 22, 2018 at 08:03:23AM +0000, Yeh, Andy wrote: > Hi Sakari, Tomasz, > > As below discussion that other drivers are with this pattern, I would > prefer to defer to address the concern in later discussion with you and > owners of other sensors. > > Thanks a lot. Works for me.
Hi Andy, On Mon, Jan 22, 2018 at 08:03:23AM +0000, Yeh, Andy wrote: > Hi Sakari, Tomasz, > > As below discussion that other drivers are with this pattern, I would prefer to defer to address the concern in later discussion with you and owners of other sensors. > > Thanks a lot. I thought of taking a look into the problem area and one sensor driver which doesn't appear to have the problem in this respect is imx258. This is because the v4l2_ctrl_handler_setup() isn't called in a runtime PM callback, but through V4L2 sub-dev s_stream callback instead. The runtime PM transition has already taken place by then. This isn't entirely optimal but works. Other sensor drivers will still need to be fixed.
On Mon, Jan 22, 2018 at 10:37:43AM +0200, Sakari Ailus wrote: > Hi Andy, > > On Mon, Jan 22, 2018 at 08:03:23AM +0000, Yeh, Andy wrote: > > Hi Sakari, Tomasz, > > > > As below discussion that other drivers are with this pattern, I would prefer to defer to address the concern in later discussion with you and owners of other sensors. > > > > Thanks a lot. > > I thought of taking a look into the problem area and one sensor driver > which doesn't appear to have the problem in this respect is imx258. This is > because the v4l2_ctrl_handler_setup() isn't called in a runtime PM > callback, but through V4L2 sub-dev s_stream callback instead. The runtime > PM transition has already taken place by then. This isn't entirely optimal > but works. Other sensor drivers will still need to be fixed. And by a quick check we seem to have no sensor drivers doing this after all... the v4l2_ctrl_handler_setup is called in the s_stream callback.
diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c index a7e58bd2..cf1c5ee 100644 --- a/drivers/media/i2c/imx258.c +++ b/drivers/media/i2c/imx258.c @@ -561,10 +561,13 @@ static int imx258_set_ctrl(struct v4l2_ctrl *ctrl) /* * Applying V4L2 control value only happens - * when power is up for qstreaming + * when streaming flag is on */ - if (pm_runtime_get_if_in_use(&client->dev) <= 0) + if (imx258->streaming == 0) return 0; switch (ctrl->id) { case V4L2_CID_ANALOGUE_GAIN: @@ -590,8 +593,6 @@ static int imx258_set_ctrl(struct v4l2_ctrl *ctrl) break; }
Thanks Tomasz, Agree with your point, if so, we could just change as below with a simple check of streaming flag. And for Sakari, do you agree with Tomasz's comment? Kindly review and I would send v5 with the change. - pm_runtime_put(&client->dev); - return ret; } Regards, Andy -----Original Message----- From: Tomasz Figa [mailto:tfiga@chromium.org] Sent: Friday, January 19, 2018 12:18 PM To: Yeh, Andy <andy.yeh@intel.com> Cc: Linux Media Mailing List <linux-media@vger.kernel.org>; Sakari Ailus <sakari.ailus@linux.intel.com> Subject: Re: [PATCH v4] media: imx258: Add imx258 camera sensor driver Hi Andy, Thanks for the patch. Please see my comments inline. On Fri, Jan 19, 2018 at 12:37 PM, Andy Yeh <andy.yeh@intel.com> wrote: > Add a V4L2 sub-device driver for the Sony IMX258 image sensor. > This is a camera sensor using the I2C bus for control and the > CSI-2 bus for data. > > Signed-off-by: Andy Yeh <andy.yeh@intel.com> > --- > - v2->v3 > -- Update the streaming function to remove SW_STANDBY in the beginning. > -- Adjust the delay time from 1ms to 12ms before set stream-on register. > - v3->v4 > -- fix the sd.entity to make code be compiled on the mainline kernel. > -- " - imx258->sd.entity.type = MEDIA_ENT_T_V4L2_SUBDEV_SENSOR;" > -- " + imx258->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;" > > MAINTAINERS | 7 + > drivers/media/i2c/Kconfig | 11 + > drivers/media/i2c/Makefile | 1 + > drivers/media/i2c/imx258.c | 1148 > ++++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 1167 insertions(+) > create mode 100644 drivers/media/i2c/imx258.c [snip] > +static int imx258_set_ctrl(struct v4l2_ctrl *ctrl) { > + struct imx258 *imx258 = > + container_of(ctrl->handler, struct imx258, ctrl_handler); > + struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd); > + s64 max; > + int ret = 0; > + > + /* Propagate change of current control to all related controls */ > + if (ctrl->id == V4L2_CID_VBLANK) { > + /* Update max exposure while meeting expected vblanking */ > + max = imx258->cur_mode->height + ctrl->val - 8; > + __v4l2_ctrl_modify_range(imx258->exposure, > + imx258->exposure->minimum, > + max, imx258->exposure->step, max); > + } > + > + /* > + * Applying V4L2 control value only happens > + * when power is up for streaming > + */ > + if (pm_runtime_get_if_in_use(&client->dev) <= 0) > + return 0; This won't work if runtime PM is not compiled in or is disabled at runtime, i.e. pm_runtime_get_if_in_use() returns -EINVAL. But actually, do we need to care about runtime PM here? Could we just return early if we're not streaming? Then the controls would be handled when streaming starts, since we call __v4l2_ctrl_handler_setup() from _start_streaming(). Best regards, Tomasz