Re: [REVIEW PATCH 3/3] smiapp: Use __v4l2_ctrl_grab() to grab controls
On 02/25/15 15:22, Sakari Ailus wrote:
> Hi Hans,
>
> On Wed, Feb 25, 2015 at 03:14:16PM +0100, Hans Verkuil wrote:
>> On 02/25/15 14:57, Sakari Ailus wrote:
>>> Hi Hans,
>>>
>>> On Wed, Feb 25, 2015 at 02:45:58PM +0100, Hans Verkuil wrote:
>>> ...
> @@ -1535,15 +1529,15 @@ static int smiapp_set_stream(struct v4l2_subdev
> *subdev, int enable)
> if (sensor->streaming == enable)
> goto out;
>
> - if (enable) {
> - sensor->streaming = true;
> + if (enable)
> rval = smiapp_start_streaming(sensor);
> - if (rval < 0)
> - sensor->streaming = false;
> - } else {
> + else
> rval = smiapp_stop_streaming(sensor);
> - sensor->streaming = false;
> - }
> +
> + sensor->streaming = enable;
> + __v4l2_ctrl_grab(sensor->hflip, enable);
> + __v4l2_ctrl_grab(sensor->vflip, enable);
> + __v4l2_ctrl_grab(sensor->link_freq, enable);
Just checking: is it really not possible to change these controls
while streaming? Most devices I know of allow changing this on the fly.
If it is really not possible, then you can add my Ack for this series:
>>>
>>> I'm not sure what the sensors would do in practice, but the problem is that
>>> changing the values of these control affect the pixel order. That's why
>>> changing them has been prevented while streaming.
>>
>> Ah, OK.
>>
>> Can you add a comment explaining why this is done?
>>
>> BTW, I understand that HFLIP will cause changes in the pixel order,
>> but VFLIP and link_freq should be OK, I would expect.
>
> Sure I can add a comment. Same for vflip, it will change the pixel order.
> The flip controls will change the readout direction. For example, a 4x4
> bayer sensor:
>
> GRGR
> BGBG
> GRGR
> BGBG
>
> Without flipping, the readout of the first line will be GRGR while the
> second is BGBG. With vertical flipping, the first line read out from the
> sensor will be BGBG and the second GRGR.
Ah, of course. A comment would be useful indeed as this is not immediately
obvious (well, not to me at least!).
>
> The link frequency cannot be changed since this would change the sensor PLL
> configuration and the CSI-2 bus frequency, neither of which are changeable
> while streaming.
>
Sorry, my mistake. I confused the link frequency with the powerline frequency
control (50/60 Hz). Of course the link frequency can't be changed while
streaming.
Regards,
Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [REVIEW PATCH 3/3] smiapp: Use __v4l2_ctrl_grab() to grab controls
Hi Hans,
On Wed, Feb 25, 2015 at 03:14:16PM +0100, Hans Verkuil wrote:
> On 02/25/15 14:57, Sakari Ailus wrote:
> > Hi Hans,
> >
> > On Wed, Feb 25, 2015 at 02:45:58PM +0100, Hans Verkuil wrote:
> > ...
> >>> @@ -1535,15 +1529,15 @@ static int smiapp_set_stream(struct v4l2_subdev
> >>> *subdev, int enable)
> >>> if (sensor->streaming == enable)
> >>> goto out;
> >>>
> >>> - if (enable) {
> >>> - sensor->streaming = true;
> >>> + if (enable)
> >>> rval = smiapp_start_streaming(sensor);
> >>> - if (rval < 0)
> >>> - sensor->streaming = false;
> >>> - } else {
> >>> + else
> >>> rval = smiapp_stop_streaming(sensor);
> >>> - sensor->streaming = false;
> >>> - }
> >>> +
> >>> + sensor->streaming = enable;
> >>> + __v4l2_ctrl_grab(sensor->hflip, enable);
> >>> + __v4l2_ctrl_grab(sensor->vflip, enable);
> >>> + __v4l2_ctrl_grab(sensor->link_freq, enable);
> >>
> >> Just checking: is it really not possible to change these controls
> >> while streaming? Most devices I know of allow changing this on the fly.
> >>
> >> If it is really not possible, then you can add my Ack for this series:
> >
> > I'm not sure what the sensors would do in practice, but the problem is that
> > changing the values of these control affect the pixel order. That's why
> > changing them has been prevented while streaming.
>
> Ah, OK.
>
> Can you add a comment explaining why this is done?
>
> BTW, I understand that HFLIP will cause changes in the pixel order,
> but VFLIP and link_freq should be OK, I would expect.
Sure I can add a comment. Same for vflip, it will change the pixel order.
The flip controls will change the readout direction. For example, a 4x4
bayer sensor:
GRGR
BGBG
GRGR
BGBG
Without flipping, the readout of the first line will be GRGR while the
second is BGBG. With vertical flipping, the first line read out from the
sensor will be BGBG and the second GRGR.
The link frequency cannot be changed since this would change the sensor PLL
configuration and the CSI-2 bus frequency, neither of which are changeable
while streaming.
--
Regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [REVIEW PATCH 3/3] smiapp: Use __v4l2_ctrl_grab() to grab controls
On 02/25/15 14:57, Sakari Ailus wrote:
> Hi Hans,
>
> On Wed, Feb 25, 2015 at 02:45:58PM +0100, Hans Verkuil wrote:
> ...
>>> @@ -1535,15 +1529,15 @@ static int smiapp_set_stream(struct v4l2_subdev
>>> *subdev, int enable)
>>> if (sensor->streaming == enable)
>>> goto out;
>>>
>>> - if (enable) {
>>> - sensor->streaming = true;
>>> + if (enable)
>>> rval = smiapp_start_streaming(sensor);
>>> - if (rval < 0)
>>> - sensor->streaming = false;
>>> - } else {
>>> + else
>>> rval = smiapp_stop_streaming(sensor);
>>> - sensor->streaming = false;
>>> - }
>>> +
>>> + sensor->streaming = enable;
>>> + __v4l2_ctrl_grab(sensor->hflip, enable);
>>> + __v4l2_ctrl_grab(sensor->vflip, enable);
>>> + __v4l2_ctrl_grab(sensor->link_freq, enable);
>>
>> Just checking: is it really not possible to change these controls
>> while streaming? Most devices I know of allow changing this on the fly.
>>
>> If it is really not possible, then you can add my Ack for this series:
>
> I'm not sure what the sensors would do in practice, but the problem is that
> changing the values of these control affect the pixel order. That's why
> changing them has been prevented while streaming.
Ah, OK.
Can you add a comment explaining why this is done?
BTW, I understand that HFLIP will cause changes in the pixel order,
but VFLIP and link_freq should be OK, I would expect.
Regards,
Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [REVIEW PATCH 3/3] smiapp: Use __v4l2_ctrl_grab() to grab controls
Hi Hans,
On Wed, Feb 25, 2015 at 02:45:58PM +0100, Hans Verkuil wrote:
...
> > @@ -1535,15 +1529,15 @@ static int smiapp_set_stream(struct v4l2_subdev
> > *subdev, int enable)
> > if (sensor->streaming == enable)
> > goto out;
> >
> > - if (enable) {
> > - sensor->streaming = true;
> > + if (enable)
> > rval = smiapp_start_streaming(sensor);
> > - if (rval < 0)
> > - sensor->streaming = false;
> > - } else {
> > + else
> > rval = smiapp_stop_streaming(sensor);
> > - sensor->streaming = false;
> > - }
> > +
> > + sensor->streaming = enable;
> > + __v4l2_ctrl_grab(sensor->hflip, enable);
> > + __v4l2_ctrl_grab(sensor->vflip, enable);
> > + __v4l2_ctrl_grab(sensor->link_freq, enable);
>
> Just checking: is it really not possible to change these controls
> while streaming? Most devices I know of allow changing this on the fly.
>
> If it is really not possible, then you can add my Ack for this series:
I'm not sure what the sensors would do in practice, but the problem is that
changing the values of these control affect the pixel order. That's why
changing them has been prevented while streaming.
Thanks for the ack!
--
Regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [REVIEW PATCH 3/3] smiapp: Use __v4l2_ctrl_grab() to grab controls
Hi Sakari,
Thanks for the patch series, but I have one question, see below.
On 02/25/15 13:33, Sakari Ailus wrote:
> From: Sakari Ailus
>
> Instead of returning -EBUSY in s_ctrl(), use __v4l2_ctrl_grab() to mark the
> controls grabbed.
>
> Signed-off-by: Sakari Ailus
> ---
> drivers/media/i2c/smiapp/smiapp-core.c | 20 +++-
> 1 file changed, 7 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/media/i2c/smiapp/smiapp-core.c
> b/drivers/media/i2c/smiapp/smiapp-core.c
> index e534f1b..6658f61 100644
> --- a/drivers/media/i2c/smiapp/smiapp-core.c
> +++ b/drivers/media/i2c/smiapp/smiapp-core.c
> @@ -423,9 +423,6 @@ static int smiapp_set_ctrl(struct v4l2_ctrl *ctrl)
>
> case V4L2_CID_HFLIP:
> case V4L2_CID_VFLIP:
> - if (sensor->streaming)
> - return -EBUSY;
> -
> if (sensor->hflip->val)
> orient |= SMIAPP_IMAGE_ORIENTATION_HFLIP;
>
> @@ -469,9 +466,6 @@ static int smiapp_set_ctrl(struct v4l2_ctrl *ctrl)
> + ctrl->val);
>
> case V4L2_CID_LINK_FREQ:
> - if (sensor->streaming)
> - return -EBUSY;
> -
> return smiapp_pll_update(sensor);
>
> case V4L2_CID_TEST_PATTERN: {
> @@ -1535,15 +1529,15 @@ static int smiapp_set_stream(struct v4l2_subdev
> *subdev, int enable)
> if (sensor->streaming == enable)
> goto out;
>
> - if (enable) {
> - sensor->streaming = true;
> + if (enable)
> rval = smiapp_start_streaming(sensor);
> - if (rval < 0)
> - sensor->streaming = false;
> - } else {
> + else
> rval = smiapp_stop_streaming(sensor);
> - sensor->streaming = false;
> - }
> +
> + sensor->streaming = enable;
> + __v4l2_ctrl_grab(sensor->hflip, enable);
> + __v4l2_ctrl_grab(sensor->vflip, enable);
> + __v4l2_ctrl_grab(sensor->link_freq, enable);
Just checking: is it really not possible to change these controls
while streaming? Most devices I know of allow changing this on the fly.
If it is really not possible, then you can add my Ack for this series:
Acked-by: Hans Verkuil
Regards,
Hans
>
> out:
> mutex_unlock(&sensor->mutex);
>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[REVIEW PATCH 3/3] smiapp: Use __v4l2_ctrl_grab() to grab controls
From: Sakari Ailus
Instead of returning -EBUSY in s_ctrl(), use __v4l2_ctrl_grab() to mark the
controls grabbed.
Signed-off-by: Sakari Ailus
---
drivers/media/i2c/smiapp/smiapp-core.c | 20 +++-
1 file changed, 7 insertions(+), 13 deletions(-)
diff --git a/drivers/media/i2c/smiapp/smiapp-core.c
b/drivers/media/i2c/smiapp/smiapp-core.c
index e534f1b..6658f61 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -423,9 +423,6 @@ static int smiapp_set_ctrl(struct v4l2_ctrl *ctrl)
case V4L2_CID_HFLIP:
case V4L2_CID_VFLIP:
- if (sensor->streaming)
- return -EBUSY;
-
if (sensor->hflip->val)
orient |= SMIAPP_IMAGE_ORIENTATION_HFLIP;
@@ -469,9 +466,6 @@ static int smiapp_set_ctrl(struct v4l2_ctrl *ctrl)
+ ctrl->val);
case V4L2_CID_LINK_FREQ:
- if (sensor->streaming)
- return -EBUSY;
-
return smiapp_pll_update(sensor);
case V4L2_CID_TEST_PATTERN: {
@@ -1535,15 +1529,15 @@ static int smiapp_set_stream(struct v4l2_subdev
*subdev, int enable)
if (sensor->streaming == enable)
goto out;
- if (enable) {
- sensor->streaming = true;
+ if (enable)
rval = smiapp_start_streaming(sensor);
- if (rval < 0)
- sensor->streaming = false;
- } else {
+ else
rval = smiapp_stop_streaming(sensor);
- sensor->streaming = false;
- }
+
+ sensor->streaming = enable;
+ __v4l2_ctrl_grab(sensor->hflip, enable);
+ __v4l2_ctrl_grab(sensor->vflip, enable);
+ __v4l2_ctrl_grab(sensor->link_freq, enable);
out:
mutex_unlock(&sensor->mutex);
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
