| Message ID | 20200828062520.1110800-1-djrscally@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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 > >
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) >
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) > >
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 >
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)
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(-)