[libcamera-devel] [PATCH 2/2] libcamera: pipeline: simple: Set format on sink pad during propagation

[Laurent Pinchart]

Hi Jacopo,

On Wed, Oct 21, 2020 at 12:42:58PM +0200, Jacopo Mondi wrote:
> On Wed, Oct 21, 2020 at 03:24:18AM +0300, Laurent Pinchart wrote:
> > When propagating formats through the media controller pipeline, the
> > simple pipeline handler retrieves the format on sink pads and sets it on
> > source pads. This assumes that subdevs will propagate formats
> > internally, as required by the V4L2 subdev API. However, in some cases,
> > propagation isn't properly handled by the subdev driver.
> 
> Working around a driver issue then.

Yes. In a nutshell, this subdev has multiple inputs and one outputs, and
selects which output to use based on the enabled links (only one input
link may be enabled). This makes propagation of formats more
complicated. I think it should move to the VIDIOC_SUBDEV_S_ROUTING API
(not merged yet), so the simple pipeline handler will have to be updated.

> > When can work around this issue by setting the format on source pads
> > instead of getting it. This will have the effect of trying to propgate
> > the same format through the pipeline, possibly overriding the default
> > format propagated by subdev drivers. It will however not cause the
> > pipeline configuration to be invalid, as subdevs are required to
> > constraint the format set on their sources based on the configuration of
> > the sources, and this requirement is better implemented in kernel driver
> 
> s/sources/sinks ? If you are referring to the usual "formats shall be
> propagated from sinks to sources" inside a subdev. Or are you
> referring to cross-link formats ?

You're right, I meant sinks. I'll fix it, thanks.

> > than the format propagation requirement.
> 
> I don't see any real drawback in doing this, so:

There could be some drawbacks in the future, but I don't see any now
either. I haven't made up my mind yet though, maybe I'll just fix the
driver and drop this patch :-)

> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  src/libcamera/pipeline/simple/simple.cpp | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > index 4b6f708e8fee..45ecefc59851 100644
> > --- a/src/libcamera/pipeline/simple/simple.cpp
> > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > @@ -386,7 +386,7 @@ int SimpleCameraData::setupFormats(V4L2SubdeviceFormat *format,
> >
> >  		if (source->entity() != sensor_->entity()) {
> >  			V4L2Subdevice *subdev = pipe->subdev(source->entity());
> > -			ret = subdev->getFormat(source->index(), format, whence);
> > +			ret = subdev->setFormat(source->index(), format, whence);
> >  			if (ret < 0)
> >  				return ret;
> >  		}

-- 
Regards,

Laurent Pinchart