[libcamera-devel] [RFC PATCH 0/2] Resolve invalid attempts to set sensor flip controls

[Naushir Patuck]

Hi Jacopo,

On Thu, 3 Nov 2022 at 12:59, Jacopo Mondi <jacopo at jmondi.org> wrote:

> Hi Naush
>
> On Thu, Nov 03, 2022 at 12:05:31PM +0000, Naushir Patuck wrote:
> > Hi Jacopo,
> >
> >
> > On Thu, 3 Nov 2022 at 11:50, Jacopo Mondi via libcamera-devel <
> > libcamera-devel at lists.libcamera.org> wrote:
> >
> > > Hi David
> > >
> > > On Thu, Nov 03, 2022 at 10:40:25AM +0000, David Plowman via
> > > libcamera-devel wrote:
> > > > Hi everyone
> > > >
> > > > (Warning: may affect all pipeline handlers!)
> > > >
> > > > I wanted to start the ball rolling on this. The problem is that when
> > > > we enumerate the cameras in the system, we try to set some of the
> > > > controls (specifically the HFLIP and VFLIP ones, and also later
> > > > HBLANK). But this will fail if we don't "own" the camera, for
> example,
> > > > it is already in use elsewhere.
> > >
> > > Feels like I'm missing one point here..
> > >
> > > How do you get to CameraSensor::init() if your pipeline handler
> > > acquires the unicam media device at the very beginning of match() ?
> > >
> > > I understand we allow multiple application acquire the same PH as the
> > > ph might register multiple cameras, but why is this a problem at
> > > match() time ?
> > >
> >
> > In match(), the rpi, rkisp and ipu3 pipeline handlers call
> > CameraSensor::init() as part of the Camera object registration.  This
> call
> > is unconditional assuming we can enumerate and register the sensor.
> >
>
> Exactly, so CameraSensor::init() is called once, regardless of which
> application will later "own" the camera. Sensor's controls are
> reset indeed, is this the problem, that application do not expect to
> have the controls reset ?
>

Ah, this is where our understanding may differ.  The enumeration code -
match(), registerCamera() and CameraSensor::init() will be called for every
(concurrent or single) libcamera process.
This happens even when one of these processes has acquired and is actively
running the camera, and in this case the other processes doing the
enumeration will try to write to the sensor when they shouldn't from
CameraSensor::init().

Again, if this does not make sense or actually answer your question, please
shout :-)

Naush


>
> > Not sure if that answers your question though?
> >
> > Naush
> >
> >
> >
> > >
> > > >
> > > > These patches address just the flip bits and not the HBLANK, though I
> > > > think the latter is a more straightforward problem. The flip bits are
> > > > troublesome because they affect the reported Bayer orders.
> > > >
> > > > So here the idea is to store the formats in the sensor's native Bayer
> > > > order, as we were doing before. Only we can't reset the flip bits
> now,
> > > > we have to query them and transform any Bayer formats the sensor
> gives
> > > > us back into the native order. Two patches implement this:
> > > >
> > > > 1. The first patch adds a function to turn BayerFormats back into
> mbus
> > > > codes. I couldn't see that we had functions to do this already, but
> > > > please correct me if I missed something!
> > > >
> > > > 2. The second patch queries the flip bits and transforms any
> > > > BayerFormats back into the native order to be stored.
> > > >
> > > > As I said, this potentially affects all pipeline handlers, which is
> > > > why I left it "RFC" for the moment. I did take a look through them
> all
> > > > to see what we might need to do:
> > > >
> > > > raspberrypi.cpp
> > > >
> > > > I think this PH is OK. We assume we're given the native Bayer order
> > > > and transform it according to the final sensor transform, setting the
> > > > flip bits every time. We update the format in any raw stream to
> match.
> > > >
> > > > uvcvideo.cpp, simple.cpp
> > > >
> > > > These appear to hardcode the transform to the Identity. I think
> mostly
> > > > they should be OK because the formats here won't be raw and you can't
> > > > have raw streams (?). Probably they should clear the flip bits to
> zero
> > > > where these controls exist, perhaps someone who knows more about them
> > > > could comment.
> > > >
> > > > rkisp1.cpp:
> > > >
> > > > Seems to force the transform to the Identity as well. So I think this
> > > > is like the above ones, it should be OK though it probably needs to
> > > > clear the sensor flip bits as well.
> > > >
> > > > ipu3.cpp:
> > > >
> > > > This one does handle transforms and appears to set the flip bits, so
> > > > that's good. I couldn't see it transforming the Bayer order for raw
> > > > streams correctly, so that probably wants checking. (To be fair, this
> > > > looks suspect already and is not made worse by these changes!)
> > > >
> > > > Thoughts and opinions gratefully received, as always.
> > > >
> > > > Thanks!
> > > > David
> > > >
> > > > David Plowman (2):
> > > >   libcamera: bayer_format: Add toMbusCode method
> > > >   libcamera: camera_sensor: Do not clear camera flips when listing
> > > >     formats
> > > >
> > > >  include/libcamera/internal/bayer_format.h |  1 +
> > > >  src/libcamera/bayer_format.cpp            | 12 ++++++
> > > >  src/libcamera/camera_sensor.cpp           | 51
> +++++++++++++++++++----
> > > >  3 files changed, 56 insertions(+), 8 deletions(-)
> > > >
> > > > --
> > > > 2.30.2
> > > >
> > >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20221103/da458d95/attachment.htm>