[libcamera-devel] [PATCH V2] libcamera: MediaDevice: Make MediaDevice::setupLink account for existing link flags

[Laurent Pinchart]

Hi Dan,

Thank you for the patch.

On Wed, Aug 26, 2020 at 04:05:50PM +0000, 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.
> 
> ref https://git.linuxtv.org/v4l-utils.git/tree/utils/media-ctl/libmediactl.c#n210
> 
> Changelog:
> 
>     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_device.cpp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp
> index de18d57..007a45b 100644
> --- a/src/libcamera/media_device.cpp
> +++ b/src/libcamera/media_device.cpp
> @@ -794,7 +794,7 @@ int MediaDevice::setupLink(const MediaLink *link, unsigned int flags)
>  	linkDesc.sink.index = sink->index();
>  	linkDesc.sink.flags = MEDIA_PAD_FL_SINK;
>  
> 
> -	linkDesc.flags = flags;
> +	linkDesc.flags = flags | (link->flags() & MEDIA_LNK_FL_IMMUTABLE);

I wonder if this shouldn't be done in MediaLink::setEnabled() instead.
The function reads as

int MediaLink::setEnabled(bool enable)
{
	unsigned int flags = enable ? MEDIA_LNK_FL_ENABLED : 0;

	int ret = dev_->setupLink(this, flags);
	if (ret)
		return ret;

	flags_ = flags;

	return 0;
}

and flags_ will lose MEDIA_LNK_FL_IMMUTABLE when setEnabled() is called.
How about

-	unsigned int flags = enable ? MEDIA_LNK_FL_ENABLED : 0;
+	unsigned int flags = (flags_ & ~MEDIA_LNK_FL_ENABLED)
+			   | (enable ? MEDIA_LNK_FL_ENABLED : 0);

?

On a side note, I recommend using git-send-email to send patches, to
avoid them getting mangled by the mail user agent. For a patch as simple
as this one Kieran fixed the issues by hand, but for more complex
patches that wouldn't be an option.

>  
> 
>  	int ret = ioctl(fd_, MEDIA_IOC_SETUP_LINK, &linkDesc);
>  	if (ret) {

-- 
Regards,

Laurent Pinchart