| Message ID | 20201021002418.21764-3-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Rejected |
| Delegated to: | Laurent Pinchart |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, 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. > 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 ? > than the format propagation requirement. > I don't see any real drawback in doing this, so: Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > Signed-off-by: Laurent Pinchart <laurent.pinchart@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 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Laurent, Thanks for your work. On 2020-10-21 03:24:18 +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. > > 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 > than the format propagation requirement. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > 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 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@jmondi.org> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@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; > > }
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; }
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. 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 than the format propagation requirement. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/libcamera/pipeline/simple/simple.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)