[libcamera-devel] [PATCH] pipeline: rpi: Respect provided stride

[Laurent Pinchart]

Hi Naush,

On Mon, Dec 18, 2023 at 08:52:33AM +0000, Naushir Patuck wrote:
> On Mon, 18 Dec 2023 at 03:50, Laurent Pinchart via libcamera-devel wrote:
> > On Tue, Dec 12, 2023 at 01:24:52PM +0000, William Vinnicombe wrote:
> > > On Tue, 12 Dec 2023 at 10:08, Laurent Pinchart wrote:
> > > > On Tue, Dec 12, 2023 at 09:56:22AM +0000, David Plowman via libcamera-devel wrote:
> > > > > On Mon, 11 Dec 2023 at 17:14, William Vinnicombe via libcamera-devel wrote:
> > > > > >
> > > > > > When converting from StreamConfiguration to V4L2DeviceFormat, the stride
> > > > > > was being dropped.
> > > > >
> > > > > I might add a "with the result that users could not request a custom
> > > > > stride" (a bit like playing consequences!) just for the extra
> > > > > clarification... but with or without that it looks good to me:
> > > >
> > > > That looks like a nice addition, I can update the commit message when
> > > > pushing if a v2 is not needed.
> > >
> > > That sounds good, thanks
> > >
> > > > > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> > > > >
> > > > > > Set the stride in the V4L2DeviceFormat to prevent this happening.
> > > > > >
> > > > > > Signed-off-by: William Vinnicombe <william.vinnicombe at raspberrypi.com>
> > > > > > ---
> > > > > >  src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 1 +
> > > > > >  1 file changed, 1 insertion(+)
> > > > > >
> > > > > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > > > > index 9f788c9d..5afa8dbb 100644
> > > > > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > > > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > > > > @@ -367,6 +367,7 @@ V4L2DeviceFormat PipelineHandlerBase::toV4L2DeviceFormat(const V4L2VideoDevice *
> > > > > >         deviceFormat.planesCount = info.numPlanes();
> > > > > >         deviceFormat.fourcc = dev->toV4L2PixelFormat(stream->pixelFormat);
> > > > > >         deviceFormat.size = stream->size;
> > > > > > +       deviceFormat.planes[0].bpl = stream->stride;
> > > >
> > > > Shouldn't the stride be set for all planes ?
> > >
> > > As there isn't any per-plane stride information in the stream
> > > configuration, and our pipeline handler only reads the stride from the
> > > first plane, I think it's safer to only set the stride for the first
> > > plane in this configuration.
> >
> > From a libcamera API point of view, we've opted for a single stride
> > value related to the first plane, with the stride of the other planes
> > derived from the first plane. The rationale is that not all hardware
> > platforms would allow configuration of the stride per-plane, and we
> > haven't come across use cases for per-plane stride values.
> >
> > From a V4L2 point of view, however, the stride is defined per plane.
> > Unless I'm mistaken, Pi 4 supports the single planar API only (formats
> > with multiple planes, such as NV12, use a single V4L2 plane to store all
> > the planes), but Pi 5 uses the V4L2 multi planar API. As this patch is
> > for common code, I think it would be best to initialize all the planes
> > correctly instead of relying on the kernel driver to fix the values.
> 
> Pi 5 supports the multiplanar API, but we still only use single-plane
> formats within it.  Lots of things would need to be fixed up if we
> switched to use true multiplanar buffers.

I may be missing something, but aren't these formats multi-planar
https://github.com/raspberrypi/linux/blob/rpi-6.1.y/drivers/media/platform/raspberrypi/pisp_be/pisp_be_formats.h#L127
?

> > > > > >         deviceFormat.colorSpace = stream->colorSpace;
> > > > > >
> > > > > >         return deviceFormat;

-- 
Regards,

Laurent Pinchart