Re: [RFCv1 API PATCH 2/4] v4l2-ctrls: add a notify callback.
On Mon October 1 2012 22:01:38 Mauro Carvalho Chehab wrote:
> Em Thu, 27 Sep 2012 08:44:25 +0200
> Hans Verkuil escreveu:
>
> > On Wed September 26 2012 12:50:11 Laurent Pinchart wrote:
>
> > > > + if (notify == NULL) {
> > > > + ctrl->call_notify = 0;
> > > > + return;
> > > > + }
> > > > + /* Only one notifier is allowed. Should we ever need to support
> > > > + multiple notifiers, then some sort of linked list of
> > > > notifiers
> > > > + should be implemented. But I don't see any real reason to
> > > > implement
> > > > + that now. If you think you need multiple notifiers, then
> > > > contact
> > > > + the linux-media mailinglist. */
>
> If only one notifier is allowed, then you should clearly state that at the
> API documentation.
Well, v4l2-controls.txt says:
"There can be only one notify function per control handler. Any attempt
to set another notify function will cause a WARN_ON."
And it is documented as well in the header, so what more do you want?
> > > > + if (WARN_ON(ctrl->handler->notify &&
> > > > + (ctrl->handler->notify != notify ||
> > > > +ctrl->handler->notify_priv != priv)))
> > > > + return;
> > >
> > > I'm not sure whether I like that. It feels a bit hackish. Wouldn't it be
> > > better to register the notifier with the handler explictly just once and
> > > then
> > > enable/disable notifications on a per-control basis ?
> >
> > I thought about that, but I prefer this method because it allows me to
> > switch
> > to per-control notifiers in the future. In addition, different controls can
> > have
> > different handlers. If you have to set the notifier for handlers, then the
> > driver needs to figure out which handlers are involved for the controls it
> > wants
> > to be notified on. It's much easier to do it like this.
>
> That also sounded hackish on my eyes. If just one notifier is allowed, the
> function should simply refuse any other call to it, as any other call to it
> is a driver's bug. So:
>
> if (WARN_ON(ctrl->handler->notify))
> return;
>
> seems to be enough.
No, it's not. Multiple controls share the same handler, so this would only work
for the first control and block any attempts to set the notifier for other
controls of the same handler.
The point is that in today's implementation the notifier is stored in the
handler
because that was the easiest implementation and because it is all we need today.
But in the future we may have to support different notifiers and also more than
one notifier per control, and in that case the notifier would move to the
control
data structure.
I want to be able to make such a change without having to change all drivers
that
use today's implementation.
The API for setting a notifier should act on a control, not on a control
handler.
The 'hack' above is just a check that ensures that you don't violate the
constraints
of the current implementation. If anyone hits that warning and contacts the ml,
then
I can see if there are good enough reasons to go the extra mile and remove this
constraint.
So I really don't want to change this patch.
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: [RFCv1 API PATCH 2/4] v4l2-ctrls: add a notify callback.
Em Thu, 27 Sep 2012 08:44:25 +0200
Hans Verkuil escreveu:
> On Wed September 26 2012 12:50:11 Laurent Pinchart wrote:
> > > + if (notify == NULL) {
> > > + ctrl->call_notify = 0;
> > > + return;
> > > + }
> > > + /* Only one notifier is allowed. Should we ever need to support
> > > +multiple notifiers, then some sort of linked list of notifiers
> > > +should be implemented. But I don't see any real reason to implement
> > > +that now. If you think you need multiple notifiers, then contact
> > > +the linux-media mailinglist. */
If only one notifier is allowed, then you should clearly state that at the
API documentation.
> > > + if (WARN_ON(ctrl->handler->notify &&
> > > + (ctrl->handler->notify != notify ||
> > > + ctrl->handler->notify_priv != priv)))
> > > + return;
> >
> > I'm not sure whether I like that. It feels a bit hackish. Wouldn't it be
> > better to register the notifier with the handler explictly just once and
> > then
> > enable/disable notifications on a per-control basis ?
>
> I thought about that, but I prefer this method because it allows me to switch
> to per-control notifiers in the future. In addition, different controls can
> have
> different handlers. If you have to set the notifier for handlers, then the
> driver needs to figure out which handlers are involved for the controls it
> wants
> to be notified on. It's much easier to do it like this.
That also sounded hackish on my eyes. If just one notifier is allowed, the
function should simply refuse any other call to it, as any other call to it
is a driver's bug. So:
if (WARN_ON(ctrl->handler->notify))
return;
seems to be enough.
Regards,
Mauro
--
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: [RFCv1 API PATCH 2/4] v4l2-ctrls: add a notify callback.
On Wed September 26 2012 12:50:11 Laurent Pinchart wrote:
> Hi Hans,
>
> Thanks for the patch.
>
> On Friday 14 September 2012 13:15:34 Hans Verkuil wrote:
> > Sometimes platform/bridge drivers need to be notified when a control from
> > a subdevice changes value. In order to support this a notify callback was
> > added.
> >
> > Signed-off-by: Hans Verkuil
> > ---
> > Documentation/video4linux/v4l2-controls.txt | 22 ++
> > drivers/media/v4l2-core/v4l2-ctrls.c| 25
> > include/media/v4l2-ctrls.h | 25
> > 3 files changed, 64 insertions(+), 8 deletions(-)
> >
> > diff --git a/Documentation/video4linux/v4l2-controls.txt
> > b/Documentation/video4linux/v4l2-controls.txt index 43da22b..cecaff8 100644
> > --- a/Documentation/video4linux/v4l2-controls.txt
> > +++ b/Documentation/video4linux/v4l2-controls.txt
> > @@ -687,14 +687,20 @@ a control of this type whenever the first control
> > belonging to a new control class is added.
> >
> >
> > -Proposals for Extensions
> > -
> > +Adding Notify Callbacks
> > +===
> > +
> > +Sometimes the platform or bridge driver needs to be notified when a control
> > +from a sub-device driver changes. You can set a notify callback by calling
> > +this function:
> >
> > -Some ideas for future extensions to the spec:
> > +void v4l2_ctrl_notify(struct v4l2_ctrl *ctrl,
> > + void (*notify)(struct v4l2_ctrl *ctrl, void *priv), void *priv);
> >
> > -1) Add a V4L2_CTRL_FLAG_HEX to have values shown as hexadecimal instead of
> > -decimal. Useful for e.g. video_mute_yuv.
> > +Whenever the give control changes value the notify callback will be called
> > +with a pointer to the control and the priv pointer that was passed with
> > +v4l2_ctrl_notify. Note that the control's handler lock is held when the
> > +notify function is called.
> >
> > -2) It is possible to mark in the controls array which controls have been
> > -successfully written and which failed by for example adding a bit to the
> > -control ID. Not sure if it is worth the effort, though.
> > +There can be only one notify function per control handler. Any attempt
> > +to set another notify function will cause a WARN_ON.
> > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
> > b/drivers/media/v4l2-core/v4l2-ctrls.c index f400035..43061e1 100644
> > --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> > @@ -1160,6 +1160,8 @@ static void new_to_cur(struct v4l2_fh *fh, struct
> > v4l2_ctrl *ctrl, send_event(fh, ctrl,
> > (changed ? V4L2_EVENT_CTRL_CH_VALUE : 0) |
> > (update_inactive ? V4L2_EVENT_CTRL_CH_FLAGS : 0));
> > + if (ctrl->call_notify && changed && ctrl->handler->notify)
> > + ctrl->handler->notify(ctrl, ctrl->handler->notify_priv);
> > }
> > }
> >
> > @@ -2628,6 +2630,29 @@ int v4l2_ctrl_s_ctrl_int64(struct v4l2_ctrl *ctrl,
> > s64 val) }
> > EXPORT_SYMBOL(v4l2_ctrl_s_ctrl_int64);
> >
> > +void v4l2_ctrl_notify(struct v4l2_ctrl *ctrl, v4l2_ctrl_notify_fnc notify,
> > void *priv)
> > +{
> > + if (ctrl == NULL)
> > + return;
>
> Isn't the caller supposed not to set ctrl to NULL ? A crash is easier to
> notice than a silent failure during development.
The reason why I do this (not only here, but in other places in the control
framework as well), is that it simplifies driver development if you have a
driver that adds controls based on the chip version. In cases like that you
only have to look at the chip version when adding the controls, but after
that you can use almost all functions without having to check the control
pointer every time. I.e., if the control wasn't added, then the control
pointer would be NULL and all these functions would be nops.
> > + if (notify == NULL) {
> > + ctrl->call_notify = 0;
> > + return;
> > + }
> > + /* Only one notifier is allowed. Should we ever need to support
> > + multiple notifiers, then some sort of linked list of notifiers
> > + should be implemented. But I don't see any real reason to implement
> > + that now. If you think you need multiple notifiers, then contact
> > + the linux-media mailinglist. */
> > + if (WARN_ON(ctrl->handler->notify &&
> > + (ctrl->handler->notify != notify ||
> > +ctrl->handler->notify_priv != priv)))
> > + return;
>
> I'm not sure whether I like that. It feels a bit hackish. Wouldn't it be
> better to register the notifier with the handler explictly just once and then
> enable/disable notifications on a per-control basis ?
I thought about that, but I prefer this method because it allows me to switch
to per-control notifiers in the future. In addition, different controls can have
different handlers. If you have to set the notifier for handlers, then the
driver needs to figure
Re: [RFCv1 API PATCH 2/4] v4l2-ctrls: add a notify callback.
Hi Hans,
Thanks for the patch.
On Friday 14 September 2012 13:15:34 Hans Verkuil wrote:
> Sometimes platform/bridge drivers need to be notified when a control from
> a subdevice changes value. In order to support this a notify callback was
> added.
>
> Signed-off-by: Hans Verkuil
> ---
> Documentation/video4linux/v4l2-controls.txt | 22 ++
> drivers/media/v4l2-core/v4l2-ctrls.c| 25
> include/media/v4l2-ctrls.h | 25
> 3 files changed, 64 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/video4linux/v4l2-controls.txt
> b/Documentation/video4linux/v4l2-controls.txt index 43da22b..cecaff8 100644
> --- a/Documentation/video4linux/v4l2-controls.txt
> +++ b/Documentation/video4linux/v4l2-controls.txt
> @@ -687,14 +687,20 @@ a control of this type whenever the first control
> belonging to a new control class is added.
>
>
> -Proposals for Extensions
> -
> +Adding Notify Callbacks
> +===
> +
> +Sometimes the platform or bridge driver needs to be notified when a control
> +from a sub-device driver changes. You can set a notify callback by calling
> +this function:
>
> -Some ideas for future extensions to the spec:
> +void v4l2_ctrl_notify(struct v4l2_ctrl *ctrl,
> + void (*notify)(struct v4l2_ctrl *ctrl, void *priv), void *priv);
>
> -1) Add a V4L2_CTRL_FLAG_HEX to have values shown as hexadecimal instead of
> -decimal. Useful for e.g. video_mute_yuv.
> +Whenever the give control changes value the notify callback will be called
> +with a pointer to the control and the priv pointer that was passed with
> +v4l2_ctrl_notify. Note that the control's handler lock is held when the
> +notify function is called.
>
> -2) It is possible to mark in the controls array which controls have been
> -successfully written and which failed by for example adding a bit to the
> -control ID. Not sure if it is worth the effort, though.
> +There can be only one notify function per control handler. Any attempt
> +to set another notify function will cause a WARN_ON.
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
> b/drivers/media/v4l2-core/v4l2-ctrls.c index f400035..43061e1 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -1160,6 +1160,8 @@ static void new_to_cur(struct v4l2_fh *fh, struct
> v4l2_ctrl *ctrl, send_event(fh, ctrl,
> (changed ? V4L2_EVENT_CTRL_CH_VALUE : 0) |
> (update_inactive ? V4L2_EVENT_CTRL_CH_FLAGS : 0));
> + if (ctrl->call_notify && changed && ctrl->handler->notify)
> + ctrl->handler->notify(ctrl, ctrl->handler->notify_priv);
> }
> }
>
> @@ -2628,6 +2630,29 @@ int v4l2_ctrl_s_ctrl_int64(struct v4l2_ctrl *ctrl,
> s64 val) }
> EXPORT_SYMBOL(v4l2_ctrl_s_ctrl_int64);
>
> +void v4l2_ctrl_notify(struct v4l2_ctrl *ctrl, v4l2_ctrl_notify_fnc notify,
> void *priv)
> +{
> + if (ctrl == NULL)
> + return;
Isn't the caller supposed not to set ctrl to NULL ? A crash is easier to
notice than a silent failure during development.
> + if (notify == NULL) {
> + ctrl->call_notify = 0;
> + return;
> + }
> + /* Only one notifier is allowed. Should we ever need to support
> +multiple notifiers, then some sort of linked list of notifiers
> +should be implemented. But I don't see any real reason to implement
> +that now. If you think you need multiple notifiers, then contact
> +the linux-media mailinglist. */
> + if (WARN_ON(ctrl->handler->notify &&
> + (ctrl->handler->notify != notify ||
> + ctrl->handler->notify_priv != priv)))
> + return;
I'm not sure whether I like that. It feels a bit hackish. Wouldn't it be
better to register the notifier with the handler explictly just once and then
enable/disable notifications on a per-control basis ?
> + ctrl->handler->notify = notify;
> + ctrl->handler->notify_priv = priv;
> + ctrl->call_notify = 1;
> +}
> +EXPORT_SYMBOL(v4l2_ctrl_notify);
> +
> static int v4l2_ctrl_add_event(struct v4l2_subscribed_event *sev, unsigned
> elems) {
> struct v4l2_ctrl *ctrl = v4l2_ctrl_find(sev->fh->ctrl_handler, sev->id);
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index 6890f5e..4484fd3 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -53,6 +53,8 @@ struct v4l2_ctrl_ops {
> int (*s_ctrl)(struct v4l2_ctrl *ctrl);
> };
>
> +typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
> +
> /** struct v4l2_ctrl - The control structure.
>* @node: The list node.
>* @ev_subs:The list of control event subscriptions.
> @@ -72,6 +74,8 @@ struct v4l2_ctrl_ops {
>* set this flag directly.
>* @has_volatiles: If set, then one or more member
[RFCv1 API PATCH 2/4] v4l2-ctrls: add a notify callback.
Sometimes platform/bridge drivers need to be notified when a control from
a subdevice changes value. In order to support this a notify callback was
added.
Signed-off-by: Hans Verkuil
---
Documentation/video4linux/v4l2-controls.txt | 22 ++
drivers/media/v4l2-core/v4l2-ctrls.c| 25 +
include/media/v4l2-ctrls.h | 25 +
3 files changed, 64 insertions(+), 8 deletions(-)
diff --git a/Documentation/video4linux/v4l2-controls.txt
b/Documentation/video4linux/v4l2-controls.txt
index 43da22b..cecaff8 100644
--- a/Documentation/video4linux/v4l2-controls.txt
+++ b/Documentation/video4linux/v4l2-controls.txt
@@ -687,14 +687,20 @@ a control of this type whenever the first control
belonging to a new control
class is added.
-Proposals for Extensions
-
+Adding Notify Callbacks
+===
+
+Sometimes the platform or bridge driver needs to be notified when a control
+from a sub-device driver changes. You can set a notify callback by calling
+this function:
-Some ideas for future extensions to the spec:
+void v4l2_ctrl_notify(struct v4l2_ctrl *ctrl,
+ void (*notify)(struct v4l2_ctrl *ctrl, void *priv), void *priv);
-1) Add a V4L2_CTRL_FLAG_HEX to have values shown as hexadecimal instead of
-decimal. Useful for e.g. video_mute_yuv.
+Whenever the give control changes value the notify callback will be called
+with a pointer to the control and the priv pointer that was passed with
+v4l2_ctrl_notify. Note that the control's handler lock is held when the
+notify function is called.
-2) It is possible to mark in the controls array which controls have been
-successfully written and which failed by for example adding a bit to the
-control ID. Not sure if it is worth the effort, though.
+There can be only one notify function per control handler. Any attempt
+to set another notify function will cause a WARN_ON.
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
b/drivers/media/v4l2-core/v4l2-ctrls.c
index f400035..43061e1 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1160,6 +1160,8 @@ static void new_to_cur(struct v4l2_fh *fh, struct
v4l2_ctrl *ctrl,
send_event(fh, ctrl,
(changed ? V4L2_EVENT_CTRL_CH_VALUE : 0) |
(update_inactive ? V4L2_EVENT_CTRL_CH_FLAGS : 0));
+ if (ctrl->call_notify && changed && ctrl->handler->notify)
+ ctrl->handler->notify(ctrl, ctrl->handler->notify_priv);
}
}
@@ -2628,6 +2630,29 @@ int v4l2_ctrl_s_ctrl_int64(struct v4l2_ctrl *ctrl, s64
val)
}
EXPORT_SYMBOL(v4l2_ctrl_s_ctrl_int64);
+void v4l2_ctrl_notify(struct v4l2_ctrl *ctrl, v4l2_ctrl_notify_fnc notify,
void *priv)
+{
+ if (ctrl == NULL)
+ return;
+ if (notify == NULL) {
+ ctrl->call_notify = 0;
+ return;
+ }
+ /* Only one notifier is allowed. Should we ever need to support
+ multiple notifiers, then some sort of linked list of notifiers
+ should be implemented. But I don't see any real reason to implement
+ that now. If you think you need multiple notifiers, then contact
+ the linux-media mailinglist. */
+ if (WARN_ON(ctrl->handler->notify &&
+ (ctrl->handler->notify != notify ||
+ctrl->handler->notify_priv != priv)))
+ return;
+ ctrl->handler->notify = notify;
+ ctrl->handler->notify_priv = priv;
+ ctrl->call_notify = 1;
+}
+EXPORT_SYMBOL(v4l2_ctrl_notify);
+
static int v4l2_ctrl_add_event(struct v4l2_subscribed_event *sev, unsigned
elems)
{
struct v4l2_ctrl *ctrl = v4l2_ctrl_find(sev->fh->ctrl_handler, sev->id);
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 6890f5e..4484fd3 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -53,6 +53,8 @@ struct v4l2_ctrl_ops {
int (*s_ctrl)(struct v4l2_ctrl *ctrl);
};
+typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
+
/** struct v4l2_ctrl - The control structure.
* @node: The list node.
* @ev_subs: The list of control event subscriptions.
@@ -72,6 +74,8 @@ struct v4l2_ctrl_ops {
*set this flag directly.
* @has_volatiles: If set, then one or more members of the cluster are
volatile.
*Drivers should never touch this flag.
+ * @call_notify: If set, then call the handler's notify function whenever the
+ *control's value changes.
* @manual_mode_value: If the is_auto flag is set, then this is the value
*of the auto control that determines if that control is in
*manual mode. So if the value of the auto control equals this
@@ -119,6 +123,7 @@ struct v4l2_ctrl {
unsigned int is_private:1;
unsigned int is_auto:1;
