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

[David Plowman]

Hi Jacopo

On Mon, 21 Nov 2022 at 16:59, Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> Hi David,
>
> On Mon, Nov 21, 2022 at 03:44:16PM +0000, David Plowman via libcamera-devel wrote:
> > Hi again
> >
> > In v2 of this set, the first 3 patches are unchanged, so thanks for
> > some of the reviews there.
> >
> > In the light of the various discussions, I've updated the changes to
> > those other PHs (uvcvideo, simple and rkisp1). In all cases I'm being
> > more explicit that I want to preserve the previous behaviour before
> > this patch set was introduced. This may or may not (probably may not)
> > be the actual correct behaviour that people would want. So:
> >
> > uvcvideo: I've deleted the patch that updated this. These are
> > unaffected by CameraSensor changes so I think I'm better off just
> > leaving them alone.
> >
> > simple and rkisp1: Both of these, AIUI, use the CameraSensor class so
> > previously the flips were getting cleared. Therefore I've added
> > exactly this into the PHs so that the behaviour is unchanged.
> >
> > As we had discussed, I think there are concerns that the simple and
> > rkisp1 PHs implement incorrect behaviours in at least some respects,
> > but I think it's probably a separate job for someone working on those
> > platforms to investigate.
>
> Agreed, but I'm not sure it is worth to introduce the v4l2 device
> helper for this temporary solution, as I'm not sure that's the
> direction where we want to go (it would be different if the V4L2Device
> helper is used by other PH to implement Transform support).
>
> As said, my understanding is that the direction where we want to go is
> to reset flips to their driver defaults, and that should be happening
> transparently in the CameraSensor::setFormat() call path...
>
> I don't think we risk regressions if we merge the first two patches
> only, while we would solve your use case, so if others agree, I would
> just pick the first two ones..

That certainly works for me. I think the result will be that the code,
which is wrong already, will remain wrong in a slightly different way,
but I also suspect no one will notice until they start looking at
these specific cases (which would be the time to fix them).

So I'll re-post just the first two patches. And then there's still the
sensor HBLANK to sort out... :)

Thanks!
David

>
> Thanks
>   j
>
> >
> > Does that make sense?
> >
> > Thanks!
> >
> > David
> >
> > David Plowman (5):
> >   libcamera: bayer_format: Add toMbusCode method
> >   libcamera: camera_sensor: Do not clear camera flips when listing
> >     formats
> >   libcamera: v4l2_device: Add setTransform method to set a device's flip
> >     controls
> >   libcamera: pipeline: simple: Set device's flip controls as previously
> >   libcamera: pipeline: rkisp1: Set device's flip controls as previously
> >
> >  include/libcamera/internal/bayer_format.h |  1 +
> >  include/libcamera/internal/v4l2_device.h  |  3 ++
> >  src/libcamera/bayer_format.cpp            | 11 +++++
> >  src/libcamera/camera_sensor.cpp           | 49 ++++++++++++++++++-----
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp  |  3 ++
> >  src/libcamera/pipeline/simple/simple.cpp  |  3 ++
> >  src/libcamera/v4l2_device.cpp             | 37 +++++++++++++++++
> >  7 files changed, 97 insertions(+), 10 deletions(-)
> >
> > --
> > 2.30.2
> >