[libcamera-devel] [PATCH 1/2] libcamera: pipeline: simple: make sure the formats at the link's pads match

[Laurent Pinchart]

Hi Andrey,

Thank you for the patch.

On Tue, Apr 21, 2020 at 11:39:53PM +0300, Andrey Konovalov wrote:
> Change SimpleCameraData::setupFormats() to return -EINVAL if the sink
> pad of the link doesn't support the format set on the source pad of this
> link.

Note that there's at least one driver (omap3isp) that supports different
formats on the two ends of a link (it's used at the input of the CCDC
block to model bits being dropped on the bus, for instance when
connecting D[9:2] of a 10-bit sensor to D[7:0] of the CCDC input). This
is a bit of a hack and shouldn't be encouraged. The OMAP3 ISP would also
need its own pipeline handler anyway. I think your patch is thus fine.

> Signed-off-by: Andrey Konovalov <andrey.konovalov at linaro.org>
> ---
>  src/libcamera/pipeline/simple/simple.cpp | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index b5f9177..8212bd9 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -372,6 +372,7 @@ int SimpleCameraData::setupFormats(V4L2SubdeviceFormat *format,
>  		MediaLink *link = e.link;
>  		MediaPad *source = link->source();
>  		MediaPad *sink = link->sink();
> +		V4L2SubdeviceFormat source_format;

s/source_format/sourceFormat/

>  
>  		if (source->entity() != sensor_->entity()) {
>  			V4L2Subdevice *subdev = pipe->subdev(source->entity());
> @@ -380,11 +381,22 @@ int SimpleCameraData::setupFormats(V4L2SubdeviceFormat *format,
>  				return ret;
>  		}
>  
> +		source_format = *format;

You can move this and the variable declaration within the if () below.

>  		if (sink->entity()->function() != MEDIA_ENT_F_IO_V4L) {
>  			V4L2Subdevice *subdev = pipe->subdev(sink->entity());
>  			ret = subdev->setFormat(sink->index(), format, whence);
>  			if (ret < 0)
>  				return ret;
> +
> +			if (format->mbus_code != source_format.mbus_code
> +			    || format->size != source_format.size) {

We usually put the || at the end of the line.

> +				LOG(SimplePipeline, Debug)
> +					<< "Source pad format isn't supported "
> +					<< "by the sink pad of the link: "
> +					<< "Source: " << source_format.toString()
> +					<< "Sink: " << format->toString();

It could be useful to tell which link this is related to. How about

					<< "Source '" << source->entity()->name()
					<< "':" << source->index()
					<< " produces " << sourceFormat.toString()
					<< ", sink '" << sink->entity()->name()
					<< "':" << sink->index()
					<< " requires " << format->toString();

With those changes,

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

I can fix when applying if you're fine with this.

> +				return -EINVAL;
> +			}
>  		}
>  
>  		LOG(SimplePipeline, Debug)

-- 
Regards,

Laurent Pinchart