| Message ID | 1525021993-17789-13-git-send-email-akinobu.mita@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Akinobu, let me see if I got this right... On Mon, Apr 30, 2018 at 02:13:11AM +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 <jacopo+renesas@jmondi.org> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: Hans Verkuil <hans.verkuil@cisco.com> > Cc: Sakari Ailus <sakari.ailus@linux.intel.com> > Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com> > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com> > --- > * v4 > - No changes > > drivers/media/i2c/ov772x.c | 79 +++++++++++++++++++++++++++++++++++++--------- > 1 file changed, 64 insertions(+), 15 deletions(-) > > diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c > index 3e6ca98..bd37169 100644 > --- a/drivers/media/i2c/ov772x.c > +++ b/drivers/media/i2c/ov772x.c > @@ -741,19 +741,30 @@ 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->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) { If I'm not wrong this is not protected when the device is streaming. Since Hans' series frame control is called by g/s_parm (until a new ioctl is not introduced) and I've looked at the call stack and it seems to me it does not check for active streaming[1]. I -think- this is even worse when this is called on the subdev, as there is no shared notion of active streaming. Am I wrong? If you have to check for active streaming here (I see some other drivers doing that, eg ov5640), then I see two ways, or you just return -EBUSY, or you save the desired FPS for later, but then it gets messy as you won't be able to just restore paramters at power_on() time, but you would need to do that also at start streaming time. My opinion is that you should check for streaming active (as you're doing for the set_fmt() function in [13/14], do you agree? [1] This calls for a device wise 'streaming' state handler. Maybe it's there but I failed to find checks for that. > + ret = ov772x_set_frame_rate(priv, fps, priv->cfmt, priv->win); > + if (ret) > + goto error; > + } > > tpf->numerator = 1; > tpf->denominator = fps; > priv->fps = fps; > > - return 0; > +error: > + mutex_unlock(&priv->lock); > + > + return ret; > } > > static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl) > @@ -765,6 +776,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; > @@ -885,6 +906,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); > @@ -895,8 +920,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 format, the frame rate, and > + * the controls > + */ > + if (!ret) > + ret = ov772x_set_params(priv, priv->cfmt, > + priv->win); > + } else { > + ret = ov772x_power_off(priv); > + } > + } > > if (!ret) { > /* Update the power count. */ > @@ -1163,7 +1200,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 +1221,24 @@ 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->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; > > - return 0; > +error: > + mutex_unlock(&priv->lock); > + > + return ret; > } > > static int ov772x_video_probe(struct ov772x_priv *priv) > @@ -1201,7 +1248,7 @@ static int ov772x_video_probe(struct ov772x_priv *priv) > const char *devname; > int ret; > > - ret = ov772x_s_power(&priv->subdev, 1); > + ret = ov772x_power_on(priv); > if (ret < 0) > return ret; > > @@ -1241,7 +1288,7 @@ static int ov772x_video_probe(struct ov772x_priv *priv) > ret = v4l2_ctrl_handler_setup(&priv->hdl); > > done: > - ov772x_s_power(&priv->subdev, 0); > + ov772x_power_off(priv); > > return ret; > } > @@ -1340,6 +1387,8 @@ static int ov772x_probe(struct i2c_client *client, > > v4l2_i2c_subdev_init(&priv->subdev, client, &ov772x_subdev_ops); > v4l2_ctrl_handler_init(&priv->hdl, 3); > + /* Use our mutex for the controls */ > + priv->hdl.lock = &priv->lock; This is unrelated and should probably go in the previous patch (which I would move before the one where you split s_frame_interval up, for clearness) Thanks j > priv->vflip_ctrl = v4l2_ctrl_new_std(&priv->hdl, &ov772x_ctrl_ops, > V4L2_CID_VFLIP, 0, 1, 1, 0); > priv->hflip_ctrl = v4l2_ctrl_new_std(&priv->hdl, &ov772x_ctrl_ops, > -- > 2.7.4 >
2018-05-04 6:03 GMT+09:00 jacopo mondi <jacopo@jmondi.org>: > Hi Akinobu, > let me see if I got this right... > > On Mon, Apr 30, 2018 at 02:13:11AM +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 <jacopo+renesas@jmondi.org> >> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> Cc: Hans Verkuil <hans.verkuil@cisco.com> >> Cc: Sakari Ailus <sakari.ailus@linux.intel.com> >> Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com> >> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com> >> --- >> * v4 >> - No changes >> >> drivers/media/i2c/ov772x.c | 79 +++++++++++++++++++++++++++++++++++++--------- >> 1 file changed, 64 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c >> index 3e6ca98..bd37169 100644 >> --- a/drivers/media/i2c/ov772x.c >> +++ b/drivers/media/i2c/ov772x.c >> @@ -741,19 +741,30 @@ 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->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) { > > If I'm not wrong this is not protected when the device is > streaming. > > Since Hans' series frame control is called by g/s_parm (until a new > ioctl is not introduced) and I've looked at the call stack and it > seems to me it does not check for active streaming[1]. I -think- > this is even worse when this is called on the subdev, as there is > no shared notion of active streaming. Am I wrong? > > If you have to check for active streaming here (I see some other > drivers doing that, eg ov5640), then I see two ways, or you just > return -EBUSY, or you save the desired FPS for later, but then it gets > messy as you won't be able to just restore paramters at power_on() > time, but you would need to do that also at start streaming time. > > My opinion is that you should check for streaming active (as you're > doing for the set_fmt() function in [13/14], do you agree? I agree. I would like to make ov772x_s_frame_interval() return -EBUSY without saving the desired FPS for later when the device is streaming. I'm going to prepare v5 patches that address the above and other issues you found in v4 unless you prefer the incremental patch series. > [1] This calls for a device wise 'streaming' state handler. Maybe it's > there but I failed to find checks for that.
Hi Akinobu, On Fri, May 04, 2018 at 11:50:39PM +0900, Akinobu Mita wrote: > 2018-05-04 6:03 GMT+09:00 jacopo mondi <jacopo@jmondi.org>: > > Hi Akinobu, > > let me see if I got this right... > > > > On Mon, Apr 30, 2018 at 02:13:11AM +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 <jacopo+renesas@jmondi.org> > >> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> Cc: Hans Verkuil <hans.verkuil@cisco.com> > >> Cc: Sakari Ailus <sakari.ailus@linux.intel.com> > >> Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com> > >> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com> > >> --- > >> * v4 > >> - No changes > >> > >> drivers/media/i2c/ov772x.c | 79 +++++++++++++++++++++++++++++++++++++--------- > >> 1 file changed, 64 insertions(+), 15 deletions(-) > >> > >> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c > >> index 3e6ca98..bd37169 100644 > >> --- a/drivers/media/i2c/ov772x.c > >> +++ b/drivers/media/i2c/ov772x.c > >> @@ -741,19 +741,30 @@ 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->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) { > > > > If I'm not wrong this is not protected when the device is > > streaming. > > > > Since Hans' series frame control is called by g/s_parm (until a new > > ioctl is not introduced) and I've looked at the call stack and it > > seems to me it does not check for active streaming[1]. I -think- > > this is even worse when this is called on the subdev, as there is > > no shared notion of active streaming. Am I wrong? > > > > If you have to check for active streaming here (I see some other > > drivers doing that, eg ov5640), then I see two ways, or you just > > return -EBUSY, or you save the desired FPS for later, but then it gets > > messy as you won't be able to just restore paramters at power_on() > > time, but you would need to do that also at start streaming time. > > > > My opinion is that you should check for streaming active (as you're > > doing for the set_fmt() function in [13/14], do you agree? > > I agree. I would like to make ov772x_s_frame_interval() return -EBUSY > without saving the desired FPS for later when the device is streaming. > > I'm going to prepare v5 patches that address the above and other issues > you found in v4 unless you prefer the incremental patch series. Thank you. Please send v5, the incremental patches thing only applies to new developments and fixes not already part of this series. Thanks j > > > [1] This calls for a device wise 'streaming' state handler. Maybe it's > > there but I failed to find checks for that.
diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c index 3e6ca98..bd37169 100644 --- a/drivers/media/i2c/ov772x.c +++ b/drivers/media/i2c/ov772x.c @@ -741,19 +741,30 @@ 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->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; - return 0; +error: + mutex_unlock(&priv->lock); + + return ret; } static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl) @@ -765,6 +776,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; @@ -885,6 +906,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); @@ -895,8 +920,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 format, the frame rate, and + * the controls + */ + if (!ret) + ret = ov772x_set_params(priv, priv->cfmt, + priv->win); + } else { + ret = ov772x_power_off(priv); + } + } if (!ret) { /* Update the power count. */ @@ -1163,7 +1200,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 +1221,24 @@ 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->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; - return 0; +error: + mutex_unlock(&priv->lock); + + return ret; } static int ov772x_video_probe(struct ov772x_priv *priv) @@ -1201,7 +1248,7 @@ static int ov772x_video_probe(struct ov772x_priv *priv) const char *devname; int ret; - ret = ov772x_s_power(&priv->subdev, 1); + ret = ov772x_power_on(priv); if (ret < 0) return ret; @@ -1241,7 +1288,7 @@ static int ov772x_video_probe(struct ov772x_priv *priv) ret = v4l2_ctrl_handler_setup(&priv->hdl); done: - ov772x_s_power(&priv->subdev, 0); + ov772x_power_off(priv); return ret; } @@ -1340,6 +1387,8 @@ static int ov772x_probe(struct i2c_client *client, v4l2_i2c_subdev_init(&priv->subdev, client, &ov772x_subdev_ops); v4l2_ctrl_handler_init(&priv->hdl, 3); + /* Use our mutex for the controls */ + priv->hdl.lock = &priv->lock; priv->vflip_ctrl = v4l2_ctrl_new_std(&priv->hdl, &ov772x_ctrl_ops, V4L2_CID_VFLIP, 0, 1, 1, 0); priv->hflip_ctrl = v4l2_ctrl_new_std(&priv->hdl, &ov772x_ctrl_ops,
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 <jacopo+renesas@jmondi.org> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Cc: Hans Verkuil <hans.verkuil@cisco.com> Cc: Sakari Ailus <sakari.ailus@linux.intel.com> Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com> --- * v4 - No changes drivers/media/i2c/ov772x.c | 79 +++++++++++++++++++++++++++++++++++++--------- 1 file changed, 64 insertions(+), 15 deletions(-)