[libcamera-devel] [PATCH v3 18/22] libcamera: rkisp1: Fill stride and frameSize at config validation

[Jacopo Mondi]

On Sun, Jul 05, 2020 at 12:51:03AM +0300, Laurent Pinchart wrote:
> Hi Paul,
>
> Thank you for the patch.
>
> On Sat, Jul 04, 2020 at 10:31:36PM +0900, Paul Elder wrote:
> > Fill the stride and frameSize fields of the StreamConfiguration at
> > configuration validation time instead of at camera configuration time.
> >
> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> >
> > ---
> > New in v3
> > ---
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 3c3f3f3..3ac7b3c 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -535,6 +535,10 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> >
> >  	cfg.bufferCount = RKISP1_BUFFER_COUNT;
> >
> > +	const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);
> > +	cfg.stride = info.stride(cfg.size.width, 0);
> > +	cfg.frameSize = info.frameSize(cfg.size);
>
> Same comment as for patches 16/22 and 17/22.
>
> The alternative is to use V4L2VideoDevice::tryFormat() in here. Jacopo,
> any opinion on that ?
>

I'm noticing that calculating the stride statically is becoming
cumbersome...

True that we have a few pipeline handlers where we have to reverse
those information from drivers, future pipeline handlers will be
instrumented from the beginning with those information in place, so it
will be less a concern.

The alternative, as you said, is to ask the video device, as that's
indeed the less error-prone solution, as the only way to get it wrong
is a driver bug :)

Recently I've been working to remove the assignment of streams to
videodevices from the IPU3 pipeline handler, not because I see that as
something frowned upon from an API perspective or for performances
reason, but mostly because there's no infrastructure to keep that
assignment in place between validate() and configure() and if every
pipeline handler has to implement its own method to do that, then I
would consider this approach "wrong" as it could easily become a
maintainance burden I would not like to encourage.

I'm not sure I want to go there, but from the very beginning we assume
streams gets assigned to StreamConfigurations only after
Camera::configure() has been called. I can't find in the documentation
any strong rationale for this, and we have absolutely no protection in
place at the moment to prevent application from taking a pointer from
a StreamConfiguration before it gets assigned (documentation apart).

>From a V4L2 point of view, I think there's nothing preventing us from
calling try fmt at validation time. The device is open and there's no
other requirements afaict.

Long story short: going to the video device would be best, otherwise
the same information shall be duplicated in multiple places. Even
if I agree we should try to move more knowledge of the platform to
userspace, the device requirements in terms of padding and alignement
seems to me among the less sensible information to record outside of
the kernel. At the same time, I would not like to see custom vector of
streams filled in at validate() and retrieved at configure() time.

Would calling StreamConfiguration::setStream() during validation be
that nasty in your opinion?

Thanks
   j

> > +
> >  	return status;
> >  }
> >
> > @@ -683,7 +687,6 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
> >  		return ret;
> >
> >  	cfg.setStream(&data->stream_);
> > -	cfg.stride = outputFormat.planes[0].bpl;
> >
> >  	return 0;
> >  }
>
> --
> Regards,
>
> Laurent Pinchart
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel