LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Hans Verkuil <hverkuil@xs4all.nl>
To: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>,
	Mauro Carvalho Chehab <mchehab@osg.samsung.com>,
	Hans Verkuil <hans.verkuil@cisco.com>,
	Sylwester Nawrocki <s.nawrocki@samsung.com>,
	Sakari Ailus <sakari.ailus@linux.intel.com>,
	Antti Palosaari <crope@iki.fi>,
	linux-media@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 1/2] media/v4l2-ctrls: Always run s_ctrl on volatile ctrls
Date: Mon, 23 Feb 2015 10:06:10 +0100	[thread overview]
Message-ID: <54EAED82.5040804@xs4all.nl> (raw)
In-Reply-To: <1424185706-16711-1-git-send-email-ricardo.ribalda@gmail.com>

Hi Ricardo,

On 02/17/2015 04:08 PM, Ricardo Ribalda Delgado wrote:
> Volatile controls can change their value outside the v4l-ctrl framework.
> We should ignore the cached written value of the ctrl when evaluating if
> we should run s_ctrl.

I've been thinking some more about this (also due to some comments Laurent
made on irc), and I think this should be done differently.

What you want to do here is to signal that setting this control will execute
some action that needs to happen even if the same value is set twice.

That's not really covered by VOLATILE. Interestingly, the WRITE_ONLY flag is
to be used for just that purpose, but this happens to be a R/W control, so
that can't be used either.

What is needed is the following:

1) Add a new flag: V4L2_CTRL_FLAG_ACTION.
2) Any control that sets FLAG_WRITE_ONLY should OR it with FLAG_ACTION (to
   keep the current meaning of WRITE_ONLY).
3) Any control with FLAG_ACTION set should return changed == true in
   cluster_changed.
4) Any control with FLAG_VOLATILE set should set ctrl->has_changed to false
   to prevent generating the CH_VALUE control (that's a real bug).

Your control will now set FLAG_ACTION and FLAG_VOLATILE and it will do the
right thing.

Basically what was missing was a flag to explicitly signal this 'writing
executes an action' behavior. Trying to shoehorn that into the volatile
flag or the write_only flag is just not right. It's a flag in its own right.

Regards,

	Hans

> 
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---
> v4: Hans Verkuil:
> 
> explicity set has_changed to false. and add comment
> 
>  drivers/media/v4l2-core/v4l2-ctrls.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 45c5b47..f34a689 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -1609,6 +1609,17 @@ static int cluster_changed(struct v4l2_ctrl *master)
>  
>  		if (ctrl == NULL)
>  			continue;
> +
> +		if (ctrl->flags & V4L2_CTRL_FLAG_VOLATILE) {
> +			/*
> +			 * Set has_changed to false to avoid generating
> +			 * the event V4L2_EVENT_CTRL_CH_VALUE
> +			 */
> +			ctrl->has_changed = false;
> +			changed = true;
> +			continue;
> +		}
> +
>  		for (idx = 0; !ctrl_changed && idx < ctrl->elems; idx++)
>  			ctrl_changed = !ctrl->type_ops->equal(ctrl, idx,
>  				ctrl->p_cur, ctrl->p_new);
> 


  reply	other threads:[~2015-02-23  9:06 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-02-17 15:08 Ricardo Ribalda Delgado
2015-02-23  9:06 ` Hans Verkuil [this message]
2015-02-23 23:07   ` Laurent Pinchart
2015-02-24  1:04     ` Ricardo Ribalda Delgado
2015-03-09 16:45     ` Ricardo Ribalda Delgado
2015-03-16 11:05       ` Hans Verkuil

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=54EAED82.5040804@xs4all.nl \
    --to=hverkuil@xs4all.nl \
    --cc=crope@iki.fi \
    --cc=hans.verkuil@cisco.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=mchehab@osg.samsung.com \
    --cc=ricardo.ribalda@gmail.com \
    --cc=s.nawrocki@samsung.com \
    --cc=sakari.ailus@linux.intel.com \
    --subject='Re: [PATCH v4 1/2] media/v4l2-ctrls: Always run s_ctrl on volatile ctrls' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).