Re: [PATCH v2 10/10] media: ov772x: avoid accessing registers under power saving mode
2018-04-18 21:55 GMT+09:00 jacopo mondi :
>> @@ -898,8 +922,20 @@ static int ov772x_s_power(struct v4l2_subdev *sd, int
>> on)
>> /* If the power count is modified from 0 to != 0 or from != 0 to 0,
>>* update the power state.
>>*/
>> - if (priv->power_count == !on)
>> - ret = on ? ov772x_power_on(priv) : ov772x_power_off(priv);
>> + if (priv->power_count == !on) {
>> + if (on) {
>> + ret = ov772x_power_on(priv);
>> + /* Restore the controls */
>> + if (!ret)
>> + ret = ov772x_set_params(priv, priv->cfmt,
>> + priv->win);
>> + /* Restore the format and the frame rate */
>> + if (!ret)
>> + ret = __v4l2_ctrl_handler_setup(&priv->hdl);
>
> frame interval is not listed in the sensor control list, it won't be
> restored if I'm not wrong...
The above two comments were swapped wrongly. The ov772x_set_params()
actually restores the format, the frame rate. It restores COM3,
COM8, and BDBASE registers, too. So calling __v4l2_ctrl_handler_setup()
here is not needed.
Re: [PATCH v2 10/10] media: ov772x: avoid accessing registers under power saving mode
Hi Sakari, On Wed, Apr 18, 2018 at 04:17:02PM +0300, Sakari Ailus wrote: > On Wed, Apr 18, 2018 at 02:55:36PM +0200, jacopo mondi wrote: > > Hi Akinobu, > > > > On Mon, Apr 16, 2018 at 11:51:51AM +0900, Akinobu Mita wrote: > > > The set_fmt() in subdev pad ops, the s_ctrl() for subdev control handler, > > > and the s_frame_interval() in subdev video ops could be called when the > > > device is under power saving mode. These callbacks for ov772x driver > > > cause updating H/W registers that will fail under power saving mode. > > > > > > > I might be wrong, but if the sensor is powered off, you should not > > receive any subdev_pad_ops function call if sensor is powered off. > > This happens (now that the driver supports sub-device uAPI) if the user > opens a sub-device node but the main driver has not powered the sensor on. Indeed. Sorry for the noise. Could you please check my reply to [08/10] as well? Thanks j > > -- > Sakari Ailus > sakari.ai...@linux.intel.com signature.asc Description: PGP signature
Re: [PATCH v2 10/10] media: ov772x: avoid accessing registers under power saving mode
On Wed, Apr 18, 2018 at 02:55:36PM +0200, jacopo mondi wrote: > Hi Akinobu, > > On Mon, Apr 16, 2018 at 11:51:51AM +0900, Akinobu Mita wrote: > > The set_fmt() in subdev pad ops, the s_ctrl() for subdev control handler, > > and the s_frame_interval() in subdev video ops could be called when the > > device is under power saving mode. These callbacks for ov772x driver > > cause updating H/W registers that will fail under power saving mode. > > > > I might be wrong, but if the sensor is powered off, you should not > receive any subdev_pad_ops function call if sensor is powered off. This happens (now that the driver supports sub-device uAPI) if the user opens a sub-device node but the main driver has not powered the sensor on. -- Sakari Ailus sakari.ai...@linux.intel.com
Re: [PATCH v2 10/10] media: ov772x: avoid accessing registers under power saving mode
Hi Akinobu,
On Mon, Apr 16, 2018 at 11:51:51AM +0900, Akinobu Mita wrote:
> The set_fmt() in subdev pad ops, the s_ctrl() for subdev control handler,
> and the s_frame_interval() in subdev video ops could be called when the
> device is under power saving mode. These callbacks for ov772x driver
> cause updating H/W registers that will fail under power saving mode.
>
I might be wrong, but if the sensor is powered off, you should not
receive any subdev_pad_ops function call if sensor is powered off.
For this driver that's up to the platform driver to handle this
correctly, have you found any case where it is necessary to handle
this in the sensor driver? Have I mis-interpreted the use case of this
patch?
> This avoids it by not apply any changes to H/W if the device is not powered
> up. Instead the changes will be restored right after power-up.
>
> Cc: Jacopo Mondi
> Cc: Laurent Pinchart
> Cc: Hans Verkuil
> Cc: Sakari Ailus
> Cc: Mauro Carvalho Chehab
> Signed-off-by: Akinobu Mita
> ---
> * v2
> - New patch
>
> drivers/media/i2c/ov772x.c | 77
> +-
> 1 file changed, 62 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> index 1297a21..c44728f 100644
> --- a/drivers/media/i2c/ov772x.c
> +++ b/drivers/media/i2c/ov772x.c
> @@ -741,19 +741,29 @@ static int ov772x_s_frame_interval(struct v4l2_subdev
> *sd,
> struct ov772x_priv *priv = to_ov772x(sd);
> struct v4l2_fract *tpf = &ival->interval;
> unsigned int fps;
> - int ret;
> + int ret = 0;
>
> fps = ov772x_select_fps(priv, tpf);
>
> - ret = ov772x_set_frame_rate(priv, fps, priv->cfmt, priv->win);
> - if (ret)
> - return ret;
> + mutex_lock(&priv->power_lock);
> + /*
> + * If the device is not powered up by the host driver do
> + * not apply any changes to H/W at this time. Instead
> + * the frame rate will be restored right after power-up.
> + */
> + if (priv->power_count > 0) {
> + ret = ov772x_set_frame_rate(priv, fps, priv->cfmt, priv->win);
> + if (ret)
> + goto error;
> + }
>
> tpf->numerator = 1;
> tpf->denominator = fps;
> priv->fps = fps;
> +error:
> + mutex_unlock(&priv->power_lock);
>
> - return 0;
> + return ret;
> }
>
> static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl)
> @@ -765,6 +775,16 @@ static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl)
> int ret = 0;
> u8 val;
>
> + /* v4l2_ctrl_lock() locks our own mutex */
> +
> + /*
> + * If the device is not powered up by the host driver do
> + * not apply any controls to H/W at this time. Instead
> + * the controls will be restored right after power-up.
> + */
> + if (priv->power_count == 0)
> + return 0;
> +
> switch (ctrl->id) {
> case V4L2_CID_VFLIP:
> val = ctrl->val ? VFLIP_IMG : 0x00;
> @@ -888,6 +908,10 @@ static int ov772x_power_off(struct ov772x_priv *priv)
> return 0;
> }
>
> +static int ov772x_set_params(struct ov772x_priv *priv,
> + const struct ov772x_color_format *cfmt,
> + const struct ov772x_win_size *win);
> +
> static int ov772x_s_power(struct v4l2_subdev *sd, int on)
> {
> struct ov772x_priv *priv = to_ov772x(sd);
> @@ -898,8 +922,20 @@ static int ov772x_s_power(struct v4l2_subdev *sd, int on)
> /* If the power count is modified from 0 to != 0 or from != 0 to 0,
>* update the power state.
>*/
> - if (priv->power_count == !on)
> - ret = on ? ov772x_power_on(priv) : ov772x_power_off(priv);
> + if (priv->power_count == !on) {
> + if (on) {
> + ret = ov772x_power_on(priv);
> + /* Restore the controls */
> + if (!ret)
> + ret = ov772x_set_params(priv, priv->cfmt,
> + priv->win);
> + /* Restore the format and the frame rate */
> + if (!ret)
> + ret = __v4l2_ctrl_handler_setup(&priv->hdl);
frame interval is not listed in the sensor control list, it won't be
restored if I'm not wrong...
Thanks
j
> + } else {
> + ret = ov772x_power_off(priv);
> + }
> + }
>
> if (!ret) {
> /* Update the power count. */
> @@ -1163,7 +1199,7 @@ static int ov772x_set_fmt(struct v4l2_subdev *sd,
> struct v4l2_mbus_framefmt *mf = &format->format;
> const struct ov772x_color_format *cfmt;
> const struct ov772x_win_size *win;
> - int ret;
> + int ret = 0;
>
> if (format->pad)
> return -EINVAL;
> @@ -1184,14 +1220,23 @@ static int ov772x_set_fmt(struct v4l2_subdev *sd,
> return 0;
> }
>
> - ret = ov772x_set_p
Re: [PATCH v2 10/10] media: ov772x: avoid accessing registers under power saving mode
2018-04-16 19:55 GMT+09:00 Sakari Ailus :
> Hi Akinobu,
>
> As the driver now offers a V4L2 sub-device uAPI, it needs to serialise
> access to its internal data structures. This appears to be fine, but there
> are additional requirements; for instance ov772x_select_params() should
> likely fail if you're streaming.
OK. I can find many subdev drivers that have 'streaming' flag in the
private data to keep track of the streaming state and make set_fmt()
return -EBUSY if streaming is on.
I'll prepare another patch that does the same thing.
> On Mon, Apr 16, 2018 at 11:51:51AM +0900, Akinobu Mita wrote:
>> The set_fmt() in subdev pad ops, the s_ctrl() for subdev control handler,
>> and the s_frame_interval() in subdev video ops could be called when the
>> device is under power saving mode. These callbacks for ov772x driver
>> cause updating H/W registers that will fail under power saving mode.
>>
>> This avoids it by not apply any changes to H/W if the device is not powered
>> up. Instead the changes will be restored right after power-up.
>>
>> Cc: Jacopo Mondi
>> Cc: Laurent Pinchart
>> Cc: Hans Verkuil
>> Cc: Sakari Ailus
>> Cc: Mauro Carvalho Chehab
>> Signed-off-by: Akinobu Mita
>> ---
>> * v2
>> - New patch
>>
>> drivers/media/i2c/ov772x.c | 77
>> +-
>> 1 file changed, 62 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
>> index 1297a21..c44728f 100644
>> --- a/drivers/media/i2c/ov772x.c
>> +++ b/drivers/media/i2c/ov772x.c
>> @@ -741,19 +741,29 @@ static int ov772x_s_frame_interval(struct v4l2_subdev
>> *sd,
>> struct ov772x_priv *priv = to_ov772x(sd);
>> struct v4l2_fract *tpf = &ival->interval;
>> unsigned int fps;
>> - int ret;
>> + int ret = 0;
>>
>> fps = ov772x_select_fps(priv, tpf);
>>
>> - ret = ov772x_set_frame_rate(priv, fps, priv->cfmt, priv->win);
>> - if (ret)
>> - return ret;
>> + mutex_lock(&priv->power_lock);
>> + /*
>> + * If the device is not powered up by the host driver do
>> + * not apply any changes to H/W at this time. Instead
>> + * the frame rate will be restored right after power-up.
>> + */
>> + if (priv->power_count > 0) {
>> + ret = ov772x_set_frame_rate(priv, fps, priv->cfmt, priv->win);
>> + if (ret)
>> + goto error;
>> + }
>>
>> tpf->numerator = 1;
>> tpf->denominator = fps;
>> priv->fps = fps;
>
> Newline before a label would be nice.
I see.
Re: [PATCH v2 10/10] media: ov772x: avoid accessing registers under power saving mode
Hi Akinobu,
As the driver now offers a V4L2 sub-device uAPI, it needs to serialise
access to its internal data structures. This appears to be fine, but there
are additional requirements; for instance ov772x_select_params() should
likely fail if you're streaming.
On Mon, Apr 16, 2018 at 11:51:51AM +0900, Akinobu Mita wrote:
> The set_fmt() in subdev pad ops, the s_ctrl() for subdev control handler,
> and the s_frame_interval() in subdev video ops could be called when the
> device is under power saving mode. These callbacks for ov772x driver
> cause updating H/W registers that will fail under power saving mode.
>
> This avoids it by not apply any changes to H/W if the device is not powered
> up. Instead the changes will be restored right after power-up.
>
> Cc: Jacopo Mondi
> Cc: Laurent Pinchart
> Cc: Hans Verkuil
> Cc: Sakari Ailus
> Cc: Mauro Carvalho Chehab
> Signed-off-by: Akinobu Mita
> ---
> * v2
> - New patch
>
> drivers/media/i2c/ov772x.c | 77
> +-
> 1 file changed, 62 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> index 1297a21..c44728f 100644
> --- a/drivers/media/i2c/ov772x.c
> +++ b/drivers/media/i2c/ov772x.c
> @@ -741,19 +741,29 @@ static int ov772x_s_frame_interval(struct v4l2_subdev
> *sd,
> struct ov772x_priv *priv = to_ov772x(sd);
> struct v4l2_fract *tpf = &ival->interval;
> unsigned int fps;
> - int ret;
> + int ret = 0;
>
> fps = ov772x_select_fps(priv, tpf);
>
> - ret = ov772x_set_frame_rate(priv, fps, priv->cfmt, priv->win);
> - if (ret)
> - return ret;
> + mutex_lock(&priv->power_lock);
> + /*
> + * If the device is not powered up by the host driver do
> + * not apply any changes to H/W at this time. Instead
> + * the frame rate will be restored right after power-up.
> + */
> + if (priv->power_count > 0) {
> + ret = ov772x_set_frame_rate(priv, fps, priv->cfmt, priv->win);
> + if (ret)
> + goto error;
> + }
>
> tpf->numerator = 1;
> tpf->denominator = fps;
> priv->fps = fps;
Newline before a label would be nice.
> +error:
> + mutex_unlock(&priv->power_lock);
>
> - return 0;
> + return ret;
> }
>
> static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl)
> @@ -765,6 +775,16 @@ static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl)
> int ret = 0;
> u8 val;
>
> + /* v4l2_ctrl_lock() locks our own mutex */
> +
> + /*
> + * If the device is not powered up by the host driver do
> + * not apply any controls to H/W at this time. Instead
> + * the controls will be restored right after power-up.
> + */
> + if (priv->power_count == 0)
> + return 0;
> +
> switch (ctrl->id) {
> case V4L2_CID_VFLIP:
> val = ctrl->val ? VFLIP_IMG : 0x00;
> @@ -888,6 +908,10 @@ static int ov772x_power_off(struct ov772x_priv *priv)
> return 0;
> }
>
> +static int ov772x_set_params(struct ov772x_priv *priv,
> + const struct ov772x_color_format *cfmt,
> + const struct ov772x_win_size *win);
> +
> static int ov772x_s_power(struct v4l2_subdev *sd, int on)
> {
> struct ov772x_priv *priv = to_ov772x(sd);
> @@ -898,8 +922,20 @@ static int ov772x_s_power(struct v4l2_subdev *sd, int on)
> /* If the power count is modified from 0 to != 0 or from != 0 to 0,
>* update the power state.
>*/
> - if (priv->power_count == !on)
> - ret = on ? ov772x_power_on(priv) : ov772x_power_off(priv);
> + if (priv->power_count == !on) {
> + if (on) {
> + ret = ov772x_power_on(priv);
> + /* Restore the controls */
> + if (!ret)
> + ret = ov772x_set_params(priv, priv->cfmt,
> + priv->win);
> + /* Restore the format and the frame rate */
> + if (!ret)
> + ret = __v4l2_ctrl_handler_setup(&priv->hdl);
> + } else {
> + ret = ov772x_power_off(priv);
> + }
> + }
>
> if (!ret) {
> /* Update the power count. */
> @@ -1163,7 +1199,7 @@ static int ov772x_set_fmt(struct v4l2_subdev *sd,
> struct v4l2_mbus_framefmt *mf = &format->format;
> const struct ov772x_color_format *cfmt;
> const struct ov772x_win_size *win;
> - int ret;
> + int ret = 0;
>
> if (format->pad)
> return -EINVAL;
> @@ -1184,14 +1220,23 @@ static int ov772x_set_fmt(struct v4l2_subdev *sd,
> return 0;
> }
>
> - ret = ov772x_set_params(priv, cfmt, win);
> - if (ret < 0)
> - return ret;
> -
> + mutex_lock(&priv->power_lock);
> + /*
> + * If the
[PATCH v2 10/10] media: ov772x: avoid accessing registers under power saving mode
The set_fmt() in subdev pad ops, the s_ctrl() for subdev control handler,
and the s_frame_interval() in subdev video ops could be called when the
device is under power saving mode. These callbacks for ov772x driver
cause updating H/W registers that will fail under power saving mode.
This avoids it by not apply any changes to H/W if the device is not powered
up. Instead the changes will be restored right after power-up.
Cc: Jacopo Mondi
Cc: Laurent Pinchart
Cc: Hans Verkuil
Cc: Sakari Ailus
Cc: Mauro Carvalho Chehab
Signed-off-by: Akinobu Mita
---
* v2
- New patch
drivers/media/i2c/ov772x.c | 77 +-
1 file changed, 62 insertions(+), 15 deletions(-)
diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index 1297a21..c44728f 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -741,19 +741,29 @@ static int ov772x_s_frame_interval(struct v4l2_subdev *sd,
struct ov772x_priv *priv = to_ov772x(sd);
struct v4l2_fract *tpf = &ival->interval;
unsigned int fps;
- int ret;
+ int ret = 0;
fps = ov772x_select_fps(priv, tpf);
- ret = ov772x_set_frame_rate(priv, fps, priv->cfmt, priv->win);
- if (ret)
- return ret;
+ mutex_lock(&priv->power_lock);
+ /*
+* If the device is not powered up by the host driver do
+* not apply any changes to H/W at this time. Instead
+* the frame rate will be restored right after power-up.
+*/
+ if (priv->power_count > 0) {
+ ret = ov772x_set_frame_rate(priv, fps, priv->cfmt, priv->win);
+ if (ret)
+ goto error;
+ }
tpf->numerator = 1;
tpf->denominator = fps;
priv->fps = fps;
+error:
+ mutex_unlock(&priv->power_lock);
- return 0;
+ return ret;
}
static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl)
@@ -765,6 +775,16 @@ static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl)
int ret = 0;
u8 val;
+ /* v4l2_ctrl_lock() locks our own mutex */
+
+ /*
+* If the device is not powered up by the host driver do
+* not apply any controls to H/W at this time. Instead
+* the controls will be restored right after power-up.
+*/
+ if (priv->power_count == 0)
+ return 0;
+
switch (ctrl->id) {
case V4L2_CID_VFLIP:
val = ctrl->val ? VFLIP_IMG : 0x00;
@@ -888,6 +908,10 @@ static int ov772x_power_off(struct ov772x_priv *priv)
return 0;
}
+static int ov772x_set_params(struct ov772x_priv *priv,
+const struct ov772x_color_format *cfmt,
+const struct ov772x_win_size *win);
+
static int ov772x_s_power(struct v4l2_subdev *sd, int on)
{
struct ov772x_priv *priv = to_ov772x(sd);
@@ -898,8 +922,20 @@ static int ov772x_s_power(struct v4l2_subdev *sd, int on)
/* If the power count is modified from 0 to != 0 or from != 0 to 0,
* update the power state.
*/
- if (priv->power_count == !on)
- ret = on ? ov772x_power_on(priv) : ov772x_power_off(priv);
+ if (priv->power_count == !on) {
+ if (on) {
+ ret = ov772x_power_on(priv);
+ /* Restore the controls */
+ if (!ret)
+ ret = ov772x_set_params(priv, priv->cfmt,
+ priv->win);
+ /* Restore the format and the frame rate */
+ if (!ret)
+ ret = __v4l2_ctrl_handler_setup(&priv->hdl);
+ } else {
+ ret = ov772x_power_off(priv);
+ }
+ }
if (!ret) {
/* Update the power count. */
@@ -1163,7 +1199,7 @@ static int ov772x_set_fmt(struct v4l2_subdev *sd,
struct v4l2_mbus_framefmt *mf = &format->format;
const struct ov772x_color_format *cfmt;
const struct ov772x_win_size *win;
- int ret;
+ int ret = 0;
if (format->pad)
return -EINVAL;
@@ -1184,14 +1220,23 @@ static int ov772x_set_fmt(struct v4l2_subdev *sd,
return 0;
}
- ret = ov772x_set_params(priv, cfmt, win);
- if (ret < 0)
- return ret;
-
+ mutex_lock(&priv->power_lock);
+ /*
+* If the device is not powered up by the host driver do
+* not apply any changes to H/W at this time. Instead
+* the format will be restored right after power-up.
+*/
+ if (priv->power_count > 0) {
+ ret = ov772x_set_params(priv, cfmt, win);
+ if (ret < 0)
+ goto error;
+ }
priv->win = win;
priv->cfmt = cfmt;
+error:
+ mutex_unlock(&priv->power_lock);
- return 0;
+
