[libcamera-devel,v3] MediaLink: Make MediaLink::setEnabled account for existing

Message ID 20200828062520.1110800-1-djrscally@gmail.com
State Accepted
Headers show
Series
  • [libcamera-devel,v3] MediaLink: Make MediaLink::setEnabled account for existing
Related show

Commit Message

Daniel Scally Aug. 28, 2020, 6:25 a.m. UTC
The setupLink function fails (ioctl returns EINVAL) when it passes the
MEDIA_LNK_FL_ENABLE flag to a link that is already flagged with
MEDIA_LNK_FL_ENABLE and MEDIA_LNK_FL_IMMUTABLE. Contrast to media-ctl's
equivalent media_setup_link() which ORs the new flags with the existing
values. This patch modifies the behaviour of setupLink() to behave the
same as media_setup_link() in media-ctl.

Signed-off-by: Dan Scally <djrscally@gmail.com>
---

Changelog:

	v3 - Moved the change to MediaLink::setEnabled()

	v2 - Simplified by removing the call to link() to fetch a link that's
		already passed as a parameter to the function.

 src/libcamera/media_object.cpp | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Daniel Scally Aug. 28, 2020, 6:32 a.m. UTC | #1
Forgot to CC the list in reply to Laurent, but I agree his proposed change
is better - and still fixes the problem I was having.

Wasn't sure if you wanted me to resubmit but I wanted to try git
send-email, hopefully this time it'll apply cleanly.

Dan

On Fri, 28 Aug 2020, 07:27 Dan Scally, <djrscally@googlemail.com> wrote:

> The setupLink function fails (ioctl returns EINVAL) when it passes the
> MEDIA_LNK_FL_ENABLE flag to a link that is already flagged with
> MEDIA_LNK_FL_ENABLE and MEDIA_LNK_FL_IMMUTABLE. Contrast to media-ctl's
> equivalent media_setup_link() which ORs the new flags with the existing
> values. This patch modifies the behaviour of setupLink() to behave the
> same as media_setup_link() in media-ctl.
>
> Signed-off-by: Dan Scally <djrscally@gmail.com>
> ---
>
> Changelog:
>
>         v3 - Moved the change to MediaLink::setEnabled()
>
>         v2 - Simplified by removing the call to link() to fetch a link
> that's
>                 already passed as a parameter to the function.
>
>  src/libcamera/media_object.cpp | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/src/libcamera/media_object.cpp
> b/src/libcamera/media_object.cpp
> index ce77a72..a2e6a0d 100644
> --- a/src/libcamera/media_object.cpp
> +++ b/src/libcamera/media_object.cpp
> @@ -115,7 +115,8 @@ LOG_DECLARE_CATEGORY(MediaDevice)
>   */
>  int MediaLink::setEnabled(bool enable)
>  {
> -       unsigned int flags = enable ? MEDIA_LNK_FL_ENABLED : 0;
> +       unsigned int flags = (flags_ & ~MEDIA_LNK_FL_ENABLED)
> +                                               | (enable ?
> MEDIA_LNK_FL_ENABLED : 0);
>
>         int ret = dev_->setupLink(this, flags);
>         if (ret)
> --
> 2.25.1
>
>
Kieran Bingham Aug. 28, 2020, 9:40 a.m. UTC | #2
Hi Dan,

Thankyou, and yes - this git-send-mail version applies perfectly well
with git-am.


On 28/08/2020 07:25, Dan Scally wrote:
> The setupLink function fails (ioctl returns EINVAL) when it passes the
> MEDIA_LNK_FL_ENABLE flag to a link that is already flagged with
> MEDIA_LNK_FL_ENABLE and MEDIA_LNK_FL_IMMUTABLE. Contrast to media-ctl's
> equivalent media_setup_link() which ORs the new flags with the existing
> values. This patch modifies the behaviour of setupLink() to behave the
> same as media_setup_link() in media-ctl.
> 
> Signed-off-by: Dan Scally <djrscally@gmail.com>
> ---
> 
> Changelog:
> 
> 	v3 - Moved the change to MediaLink::setEnabled()
> 
> 	v2 - Simplified by removing the call to link() to fetch a link that's
> 		already passed as a parameter to the function.
> 
>  src/libcamera/media_object.cpp | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/media_object.cpp b/src/libcamera/media_object.cpp
> index ce77a72..a2e6a0d 100644
> --- a/src/libcamera/media_object.cpp
> +++ b/src/libcamera/media_object.cpp
> @@ -115,7 +115,8 @@ LOG_DECLARE_CATEGORY(MediaDevice)
>   */
>  int MediaLink::setEnabled(bool enable)
>  {
> -	unsigned int flags = enable ? MEDIA_LNK_FL_ENABLED : 0;
> +	unsigned int flags = (flags_ & ~MEDIA_LNK_FL_ENABLED)
> +			   			| (enable ? MEDIA_LNK_FL_ENABLED : 0);

To fit our coding style, that second line should be aligned so that the
| operator is below the = operator I think ... but that's really minor
and is easily handled when applying.

Other than that, this sounds like a better approach that v2 indeed,
especially as now the flags_ is going to be kept consistent.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

>  
>  	int ret = dev_->setupLink(this, flags);
>  	if (ret)
>
Laurent Pinchart Aug. 28, 2020, 4:52 p.m. UTC | #3
On Fri, Aug 28, 2020 at 10:40:45AM +0100, Kieran Bingham wrote:
> Hi Dan,
> 
> Thankyou, and yes - this git-send-mail version applies perfectly well
> with git-am.
> 
> On 28/08/2020 07:25, Dan Scally wrote:
> > The setupLink function fails (ioctl returns EINVAL) when it passes the
> > MEDIA_LNK_FL_ENABLE flag to a link that is already flagged with
> > MEDIA_LNK_FL_ENABLE and MEDIA_LNK_FL_IMMUTABLE. Contrast to media-ctl's
> > equivalent media_setup_link() which ORs the new flags with the existing
> > values. This patch modifies the behaviour of setupLink() to behave the
> > same as media_setup_link() in media-ctl.

The commit message is now out of date I'm afraid, as the patch doesn't
modify setupLink() anymore. No worries, I'll update this to

The MediaDevice::setupLink() function fails (ioctl returns EINVAL) when
it passes only the MEDIA_LNK_FL_ENABLE flag to a link that is already
flagged with MEDIA_LNK_FL_ENABLE and MEDIA_LNK_FL_IMMUTABLE. Contrast to
media-ctl's equivalent media_setup_link() which ORs the new flags with
the existing values. Fix this by preserving all flags but
MEDIA_LNK_FL_ENABLED in MediaLink::setEnabled().

The subject line seems to also have been truncated, I think it misses
"flags" at the end. It also should start with "libcamera: media_object:"
to match the usual style of commit messages.

Please let me know if you have any objection.

> > 
> > Signed-off-by: Dan Scally <djrscally@gmail.com>
> > ---
> > 
> > Changelog:
> > 
> > 	v3 - Moved the change to MediaLink::setEnabled()
> > 
> > 	v2 - Simplified by removing the call to link() to fetch a link that's
> > 		already passed as a parameter to the function.
> > 
> >  src/libcamera/media_object.cpp | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/libcamera/media_object.cpp b/src/libcamera/media_object.cpp
> > index ce77a72..a2e6a0d 100644
> > --- a/src/libcamera/media_object.cpp
> > +++ b/src/libcamera/media_object.cpp
> > @@ -115,7 +115,8 @@ LOG_DECLARE_CATEGORY(MediaDevice)
> >   */
> >  int MediaLink::setEnabled(bool enable)
> >  {
> > -	unsigned int flags = enable ? MEDIA_LNK_FL_ENABLED : 0;
> > +	unsigned int flags = (flags_ & ~MEDIA_LNK_FL_ENABLED)
> > +			   			| (enable ? MEDIA_LNK_FL_ENABLED : 0);
> 
> To fit our coding style, that second line should be aligned so that the
> | operator is below the = operator I think ... but that's really minor
> and is easily handled when applying.
> 
> Other than that, this sounds like a better approach that v2 indeed,
> especially as now the flags_ is going to be kept consistent.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

With this addressed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> >  
> >  	int ret = dev_->setupLink(this, flags);
> >  	if (ret)
> >
Daniel Scally Aug. 28, 2020, 9:52 p.m. UTC | #4
Hi Laurent,

No objections from me

Dan

On Fri, 28 Aug 2020, 17:53 Laurent Pinchart, <
laurent.pinchart@ideasonboard.com> wrote:

> On Fri, Aug 28, 2020 at 10:40:45AM +0100, Kieran Bingham wrote:
> > Hi Dan,
> >
> > Thankyou, and yes - this git-send-mail version applies perfectly well
> > with git-am.
> >
> > On 28/08/2020 07:25, Dan Scally wrote:
> > > The setupLink function fails (ioctl returns EINVAL) when it passes the
> > > MEDIA_LNK_FL_ENABLE flag to a link that is already flagged with
> > > MEDIA_LNK_FL_ENABLE and MEDIA_LNK_FL_IMMUTABLE. Contrast to media-ctl's
> > > equivalent media_setup_link() which ORs the new flags with the existing
> > > values. This patch modifies the behaviour of setupLink() to behave the
> > > same as media_setup_link() in media-ctl.
>
> The commit message is now out of date I'm afraid, as the patch doesn't
> modify setupLink() anymore. No worries, I'll update this to
>
> The MediaDevice::setupLink() function fails (ioctl returns EINVAL) when
> it passes only the MEDIA_LNK_FL_ENABLE flag to a link that is already
> flagged with MEDIA_LNK_FL_ENABLE and MEDIA_LNK_FL_IMMUTABLE. Contrast to
> media-ctl's equivalent media_setup_link() which ORs the new flags with
> the existing values. Fix this by preserving all flags but
> MEDIA_LNK_FL_ENABLED in MediaLink::setEnabled().
>
> The subject line seems to also have been truncated, I think it misses
> "flags" at the end. It also should start with "libcamera: media_object:"
> to match the usual style of commit messages.
>
> Please let me know if you have any objection.
>
> > >
> > > Signed-off-by: Dan Scally <djrscally@gmail.com>
> > > ---
> > >
> > > Changelog:
> > >
> > >     v3 - Moved the change to MediaLink::setEnabled()
> > >
> > >     v2 - Simplified by removing the call to link() to fetch a link
> that's
> > >             already passed as a parameter to the function.
> > >
> > >  src/libcamera/media_object.cpp | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/libcamera/media_object.cpp
> b/src/libcamera/media_object.cpp
> > > index ce77a72..a2e6a0d 100644
> > > --- a/src/libcamera/media_object.cpp
> > > +++ b/src/libcamera/media_object.cpp
> > > @@ -115,7 +115,8 @@ LOG_DECLARE_CATEGORY(MediaDevice)
> > >   */
> > >  int MediaLink::setEnabled(bool enable)
> > >  {
> > > -   unsigned int flags = enable ? MEDIA_LNK_FL_ENABLED : 0;
> > > +   unsigned int flags = (flags_ & ~MEDIA_LNK_FL_ENABLED)
> > > +                                           | (enable ?
> MEDIA_LNK_FL_ENABLED : 0);
> >
> > To fit our coding style, that second line should be aligned so that the
> > | operator is below the = operator I think ... but that's really minor
> > and is easily handled when applying.
> >
> > Other than that, this sounds like a better approach that v2 indeed,
> > especially as now the flags_ is going to be kept consistent.
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
> With this addressed,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> > >
> > >     int ret = dev_->setupLink(this, flags);
> > >     if (ret)
> > >
>
> --
> Regards,
>
> Laurent Pinchart
>

Patch

diff --git a/src/libcamera/media_object.cpp b/src/libcamera/media_object.cpp
index ce77a72..a2e6a0d 100644
--- a/src/libcamera/media_object.cpp
+++ b/src/libcamera/media_object.cpp
@@ -115,7 +115,8 @@  LOG_DECLARE_CATEGORY(MediaDevice)
  */
 int MediaLink::setEnabled(bool enable)
 {
-	unsigned int flags = enable ? MEDIA_LNK_FL_ENABLED : 0;
+	unsigned int flags = (flags_ & ~MEDIA_LNK_FL_ENABLED)
+			   			| (enable ? MEDIA_LNK_FL_ENABLED : 0);
 
 	int ret = dev_->setupLink(this, flags);
 	if (ret)