[libcamera-devel] [PATCH] src/libcamera/media_device.cpp: Make MediaDevice::setupLink account for existing link flags

[Kieran Bingham]

Hi Dan,

Welcome to libcamera, and thanks for contributing!

On 24/08/2020 21:01, Dan Scally wrote:

Our normal style for patches is to have short prefixes on the commit
title to describe the component being modified, so I think we can
simplify the $SUBJECT:

"libcamera: media_device: Account for existing flags with setupLink"

> 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.

Oh, interesting catch, what platform did you hit this on?

> ref https://git.linuxtv.org/v4l-utils.git/tree/utils/media-ctl/libmediactl.c#n210
> 
> Signed-off-by: Daniel Scally <djrscally at protonmail.com>
> ---
>  src/libcamera/media_device.cpp | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp
> index de18d57..cd44902 100644
> --- a/src/libcamera/media_device.cpp
> +++ b/src/libcamera/media_device.cpp
> @@ -780,11 +780,20 @@ void MediaDevice::fixupEntityFlags(struct media_v2_entity *entity)
>   *
>   * \return 0 on success or a negative error code otherwise
>   */
> -int MediaDevice::setupLink(const MediaLink *link, unsigned int flags)
> +int MediaDevice::setupLink(const MediaLink *nlink, unsigned int flags)
>  {
>  	struct media_link_desc linkDesc = {};
> -	MediaPad *source = link->source();
> -	MediaPad *sink = link->sink();
> +	MediaPad *source = nlink->source();
> +	MediaPad *sink = nlink->sink();
> +	MediaLink *elink;	// existing link
> +
> +	elink = link(source, sink);

Isn't this the same link as the passed in (link,nlink) ?

I don't think we need to refetch it do we?
(Perhaps I missed something reading the code?)


> +
> +	if (elink == NULL) {
> +		LOG(MediaDevice, Error)
> +			<< __func__ << ": Link not found\n";
> +		return -ENOENT;
> +	}
>  
> 
>  	linkDesc.source.entity = source->entity()->id();
>  	linkDesc.source.index = source->index();
> @@ -794,7 +803,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 | (elink->flags() & MEDIA_LNK_FL_IMMUTABLE);

If we can re-use the const MediaLink *link, we could then simply take
the link->flags() ... but I'd then be wondering if we need to update the
flags when they've been set ... but I don't think you need to worry
about that for this patch.

Could you test to see if setting this as simply :

 linkDesc.flags = flags | (link->flags() & MEDIA_LNK_FL_IMMUTABLE);

without changing the rest of the function solves your issue please?

--
Regards

Kieran


>  
> 
>  	int ret = ioctl(fd_, MEDIA_IOC_SETUP_LINK, &linkDesc);
>  	if (ret) {
> 
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
> 

-- 
Regards
--
Kieran