Re: [PATCH v3 07/11] media: ov772x: handle nested s_power() calls

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



2018-04-23 17:35 GMT+09:00 jacopo mondi <jacopo@xxxxxxxxxx>:
> Hi Akinobu,
>    thanks for v3
>
> On Mon, Apr 23, 2018 at 12:56:13AM +0900, Akinobu Mita wrote:
>> Depending on the v4l2 driver, calling s_power() could be nested.  So the
>> actual transitions between power saving mode and normal operation mode
>> should only happen at the first power on and the last power off.
>>
>> This adds an s_power() nesting counter and updates the power state if the
>> counter is modified from 0 to != 0 or from != 0 to 0.
>>
>> Cc: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>
>> Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>> Cc: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>> Cc: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
>> Cc: Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxxxxx>
>> Signed-off-by: Akinobu Mita <akinobu.mita@xxxxxxxxx>
>> ---
>> * v3
>> - Rename mutex name from power_lock to lock
>> - Add warning for duplicated s_power call
>>
>>  drivers/media/i2c/ov772x.c | 34 ++++++++++++++++++++++++++++++----
>>  1 file changed, 30 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
>> index 95c1c95d..8c0b850 100644
>> --- a/drivers/media/i2c/ov772x.c
>> +++ b/drivers/media/i2c/ov772x.c
>> @@ -424,6 +424,9 @@ struct ov772x_priv {
>>       /* band_filter = COM8[5] ? 256 - BDBASE : 0 */
>>       unsigned short                    band_filter;
>>       unsigned int                      fps;
>> +     /* lock to protect power_count */
>> +     struct mutex                      lock;
>> +     int                               power_count;
>>  #ifdef CONFIG_MEDIA_CONTROLLER
>>       struct media_pad pad;
>>  #endif
>> @@ -871,9 +874,26 @@ static int ov772x_power_off(struct ov772x_priv *priv)
>>  static int ov772x_s_power(struct v4l2_subdev *sd, int on)
>>  {
>>       struct ov772x_priv *priv = to_ov772x(sd);
>> +     int ret = 0;
>> +
>> +     mutex_lock(&priv->lock);
>> +
>> +     /* 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 (!ret) {
>> +             /* Update the power count. */
>> +             priv->power_count += on ? 1 : -1;
>> +             WARN(priv->power_count < 0, "Unbalanced power count\n");
>> +             WARN(priv->power_count > 1, "Duplicated s_power call\n");
>
>
> The first time you receive a power on (on == 1, power_count == 0)
> you'll have:
>
>         if (0 == !1)
>                 ret = ov772x_power_on()
>         if (!0)
>                 power_count += 1;
>
> If power management is 'nested' you could receive a second,
> unexpected, power on (on == 1, power_count == 1)
>         if (1 == !1)
>         if (!0)
>                 power_count += 1;
>
> Now power_count == 2, if you now receive a power off, you're going to
> ignore it:
>
>         if (2 == !0)
>                 ret = ov772x_power_off();
>         if (!0)
>                 power_count += -1;
>
> Again you now receive a new power off, and you're going to issue that
> one.
>
>         if (1 == !0)
>                 ret = ov772x_power_off();
>         if (!0)
>                 power_count += -1;
>
> So this seems correct to me!
>
> If I were you, I would simplify this as:
>
>         power_count += on ? 1 : -1;
>         if (power_count == !!on)
>                 return on ? ov772x_power_on(priv) :
>                             ov772x_power_off(priv);
>         WARN_ON(power_count < 0 || power_count > 1);
>
>         return 0;

The only drawback is that the power_count is updated even when the
ov772x_power_on/off failed.

> which, if I'm not wrong expands to:
>
> power_count = 0 ; on = 1
>         power_count += 1;
>         if (1 == !!1)
>                 return ov772x_power_on(priv);
>
> power_count = 1 ; on = 1
>         power_count += 1;
>         if (2 == !!1)
>         WARN_ON(2 > 1)
>
> power_count = 2 ; on = 0
>         power_count -= 1;
>         if (1 == !!0)
>         if (1 > 1 || 1 < 0)
>
> power_count = 1; on = 0
>         power_count -= 1;
>         if (0 == !!0)
>                 return ov772_power_off(priv);
>
> power_count = 0 ; on = 0
>         power_count -= 1;
>         if (-1 == !!0)
>         WARN_ON(-1 < 0)
>
> But if you have seen that pattern in other drivers, and maybe already
> tested your one, I'm fine with either of them.

For now, I leave this patch unchanged. If we can guarantee that s_power()
is not nested, we can simplify it by removing power_count or moving to
common field.

> Reviewed-by: Jacopo Mondi <jacopo@xxxxxxxxxx>
>
> Thanks
>    j
>
>> +     }
>> +
>> +     mutex_unlock(&priv->lock);
>>
>> -     return on ? ov772x_power_on(priv) :
>> -                 ov772x_power_off(priv);
>> +     return ret;
>>  }
>>
>>  static const struct ov772x_win_size *ov772x_select_win(u32 width, u32 height)
>> @@ -1303,6 +1323,7 @@ static int ov772x_probe(struct i2c_client *client,
>>               return -ENOMEM;
>>
>>       priv->info = client->dev.platform_data;
>> +     mutex_init(&priv->lock);
>>
>>       v4l2_i2c_subdev_init(&priv->subdev, client, &ov772x_subdev_ops);
>>       v4l2_ctrl_handler_init(&priv->hdl, 3);
>> @@ -1313,8 +1334,10 @@ static int ov772x_probe(struct i2c_client *client,
>>       v4l2_ctrl_new_std(&priv->hdl, &ov772x_ctrl_ops,
>>                         V4L2_CID_BAND_STOP_FILTER, 0, 256, 1, 0);
>>       priv->subdev.ctrl_handler = &priv->hdl;
>> -     if (priv->hdl.error)
>> -             return priv->hdl.error;
>> +     if (priv->hdl.error) {
>> +             ret = priv->hdl.error;
>> +             goto error_mutex_destroy;
>> +     }
>>
>>       priv->clk = clk_get(&client->dev, "xclk");
>>       if (IS_ERR(priv->clk)) {
>> @@ -1362,6 +1385,8 @@ static int ov772x_probe(struct i2c_client *client,
>>       clk_put(priv->clk);
>>  error_ctrl_free:
>>       v4l2_ctrl_handler_free(&priv->hdl);
>> +error_mutex_destroy:
>> +     mutex_destroy(&priv->lock);
>>
>>       return ret;
>>  }
>> @@ -1376,6 +1401,7 @@ static int ov772x_remove(struct i2c_client *client)
>>               gpiod_put(priv->pwdn_gpio);
>>       v4l2_async_unregister_subdev(&priv->subdev);
>>       v4l2_ctrl_handler_free(&priv->hdl);
>> +     mutex_destroy(&priv->lock);
>>
>>       return 0;
>>  }
>> --
>> 2.7.4
>>



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux