[PATCH] pipeline: rpi: Don't validate configuration in generateConfiguration()

[Naushir Patuck]

Hi Kieran,

On Thu, 6 Jun 2024 at 15:57, Kieran Bingham
<kieran.bingham at ideasonboard.com> wrote:
>
> Quoting David Plowman (2024-06-06 14:26:53)
> > Hi Naush
> >
> > Thanks for the patch. I agree that this is more helpful behaviour for users.
> >
> > On Thu, 6 Jun 2024 at 12:52, Naushir Patuck <naush at raspberrypi.com> wrote:
> > >
> > > generateConfiguration() called validate() as a final step, causing the
> > > stride and frameSize fields in StreamConfiguration to be filled in based
> > > on the pixel format and width/height.
> > >
> > > If a user application did not clear the stride field when setting up a
> > > custom pixel format and width/height, the pipeline handler would respect
> > > this stride and possibly overallocate buffers with a larger stride than
> > > needed.
> > >
> > > Fix this by removing the call to validate() completely, leaving the
> > > stride and frameSize fields defaulting to 0. Removal of this call is
> > > inconsequential as we hard-code a valid configuration for Raspberry Pi
> > > platforms in generateConfiguration().
> > >
> > > Bug: https://github.com/raspberrypi/libcamera/issues/138
> > > Bug: https://github.com/raspberrypi/libcamera/issues/141
> > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> >
> > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> >
> > Thanks
> > David
> >
> > > ---
> > >  src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 2 --
> > >  1 file changed, 2 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > index 289af5165766..3041fd1ed9fd 100644
> > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > @@ -496,8 +496,6 @@ PipelineHandlerBase::generateConfiguration(Camera *camera, Span<const StreamRole
> > >                 config->addConfiguration(cfg);
> > >         }
> > >
> > > -       config->validate();
> > > -
>
> I think we expect that configurations (except a completely empty one) we
> return are 'valid'. We hardcode this so it should be fine, but I guess
> we might potentially 'forget' and re-introduce this call later?


As long as we never change the defaults (I don't see why we will), I
think we can remove the validate() call permanently.

>
>
> Does it mean we should never call validate on any pipeline that would
> cause the same issue? (or maybe this is just specific to the patch that
> went into the RPi kernel anyway).


Possibly, but the key thing going wrong is for RPi validate() will
update the stride fields to what the driver chooses.  We use this
functionality to allocate dmabufs outside of v4l2 if needed.  Not sure
if other pipeline handlers do this.

>
>
> I guess it's "impossible" for us to validate stride after it's gone to
> the application (other than making sure it is at least bigger than the
> width*bpp?) as applications might set it larger ...


Yes, this will still happen when configure() calls validate().

Regards,
Naush

>
>
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> I'll leave merging to next week incase there are any other comments /
> concerns - but I do see this already fixes user facing issues.
>
>
> > >         return config;
> > >  }
> > >
> > > --
> > > 2.34.1
> > >