| Message ID | 20211012113946.484914-1-dorota.czaplejewicz@puri.sm |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Dorota, It's great to see this progress on the Purism phone camera support. Quoting Dorota Czaplejewicz (2021-10-12 12:39:46) > The sensor is called s5k3l6xx in the kernel. > > The driver is currently out of tree and maintained by Purism. We prefer kernel drivers to be either upstream, or at least on their way upstream. Ideally, a minimum of having been posted to the linux-media mailinglist, however we know that the review process can take time, so we can integrate support to libcamera before it's fully upstream in the kernel to support the upstreaming process. Is this sensor driver something that you are intending to post to the linux-media mailinglist in the near future, or has it been posted already? > > It's the main camera sensor on the Librem 5. > > Signed-off-by: Dorota Czaplejewicz <dorota.czaplejewicz@puri.sm> > --- > src/libcamera/camera_sensor_properties.cpp | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/src/libcamera/camera_sensor_properties.cpp b/src/libcamera/camera_sensor_properties.cpp > index 48305ac4..0625a0d8 100644 > --- a/src/libcamera/camera_sensor_properties.cpp > +++ b/src/libcamera/camera_sensor_properties.cpp > @@ -134,6 +134,19 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > { controls::draft::TestPatternModeColorBars, 1 }, > }, > } }, > + { "s5k3l6xx", { > + .unitCellSize = { 1120, 1120 }, > + .testPatternModes = { > + { 0, controls::draft::TestPatternModeOff }, > + { 1, controls::draft::TestPatternModeSolidColor }, > + { 2, controls::draft::TestPatternModeColorBars }, > + { 3, controls::draft::TestPatternModeColorBarsFadeToGray }, > + { 4, controls::draft::TestPatternModeCustom1 }, /* White */ > + { 5, controls::draft::TestPatternModePn9 }, > + { 6, controls::draft::TestPatternModeCustom1 + 1 }, /* LFSR32 */ > + { 7, controls::draft::TestPatternModeCustom1 + 2 }, /* Address */ That's an interesting way to define the extra custom modes... Interstingly, I don't see any users of TestPatternModeCustom1 yet. What is LFSR32 and Address? Is 'White' just a solid colour of 'white'? I suspect these modes should be defined within src/libcamera/control_ids.yaml where possible. An all white could be TestPatternModeSolidColorWhite or TestPatternModeSolidWhite for intance. -- Kieran > + }, > + } }, > }; > > const auto it = sensorProps.find(sensor); > -- > 2.31.1 >
Hi Kieran, On Tue, 12 Oct 2021 15:07:15 +0100 Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > Quoting Dorota Czaplejewicz (2021-10-12 12:39:46) > > The sensor is called s5k3l6xx in the kernel. > > > > The driver is currently out of tree and maintained by Purism. > > We prefer kernel drivers to be either upstream, or at least on their way > upstream. Ideally, a minimum of having been posted to the linux-media > mailinglist, however we know that the review process can take time, so > we can integrate support to libcamera before it's fully upstream in the > kernel to support the upstreaming process. > > Is this sensor driver something that you are intending to post to the > linux-media mailinglist in the near future, or has it been posted > already? The driver still has some deficiencies, so I haven't posted it yet. I expect I will within a couple months. > > > > > It's the main camera sensor on the Librem 5. > > > > Signed-off-by: Dorota Czaplejewicz <dorota.czaplejewicz@puri.sm> > > --- > > src/libcamera/camera_sensor_properties.cpp | 13 +++++++++++++ > > 1 file changed, 13 insertions(+) > > > > diff --git a/src/libcamera/camera_sensor_properties.cpp b/src/libcamera/camera_sensor_properties.cpp > > index 48305ac4..0625a0d8 100644 > > --- a/src/libcamera/camera_sensor_properties.cpp > > +++ b/src/libcamera/camera_sensor_properties.cpp > > @@ -134,6 +134,19 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > > { controls::draft::TestPatternModeColorBars, 1 }, > > }, > > } }, > > + { "s5k3l6xx", { > > + .unitCellSize = { 1120, 1120 }, > > + .testPatternModes = { > > + { 0, controls::draft::TestPatternModeOff }, > > + { 1, controls::draft::TestPatternModeSolidColor }, > > + { 2, controls::draft::TestPatternModeColorBars }, > > + { 3, controls::draft::TestPatternModeColorBarsFadeToGray }, > > + { 4, controls::draft::TestPatternModeCustom1 }, /* White */ > > + { 5, controls::draft::TestPatternModePn9 }, I just noticed those two are swapped. > > + { 6, controls::draft::TestPatternModeCustom1 + 1 }, /* LFSR32 */ > > + { 7, controls::draft::TestPatternModeCustom1 + 2 }, /* Address */ > > That's an interesting way to define the extra custom modes... > Interstingly, I don't see any users of TestPatternModeCustom1 yet. I found it directly in the yaml file, where the description suggests this way of dealing with it. > > What is LFSR32 and Address? Neither is documented in the application note, but LFSR32 appears to be a pseudo-random algorithm: https://gist.github.com/edarc/2946448 > > Is 'White' just a solid colour of 'white'? > Yes, the maximum integer value is returned. > I suspect these modes should be defined within > src/libcamera/control_ids.yaml where possible. An all white could be > TestPatternModeSolidColorWhite or TestPatternModeSolidWhite for intance. > > -- > Kieran > > > > + }, > > + } }, > > }; > > > > const auto it = sensorProps.find(sensor); > > -- > > 2.31.1 > >
Hi Dorota, Quoting Dorota Czaplejewicz (2021-10-12 16:10:38) > Hi Kieran, > > On Tue, 12 Oct 2021 15:07:15 +0100 > Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Quoting Dorota Czaplejewicz (2021-10-12 12:39:46) > > > The sensor is called s5k3l6xx in the kernel. > > > > > > The driver is currently out of tree and maintained by Purism. > > > > We prefer kernel drivers to be either upstream, or at least on their way > > upstream. Ideally, a minimum of having been posted to the linux-media > > mailinglist, however we know that the review process can take time, so > > we can integrate support to libcamera before it's fully upstream in the > > kernel to support the upstreaming process. > > > > Is this sensor driver something that you are intending to post to the > > linux-media mailinglist in the near future, or has it been posted > > already? > > The driver still has some deficiencies, so I haven't posted it yet. I expect I will within a couple months. Ok, it's understandable to still have some issues to work through. I'm sure you're already aware, but if there's anything you want support on, you can post to linux-media and tag it as RFC. Highlight any known deficiencies in the coverletter or comment section, and early review will help make sure the driver is going in the right direction overall. A posted driver will make it easier to accept this patch, as we ideally need something to review it against... > > > It's the main camera sensor on the Librem 5. > > > > > > Signed-off-by: Dorota Czaplejewicz <dorota.czaplejewicz@puri.sm> > > > --- > > > src/libcamera/camera_sensor_properties.cpp | 13 +++++++++++++ > > > 1 file changed, 13 insertions(+) > > > > > > diff --git a/src/libcamera/camera_sensor_properties.cpp b/src/libcamera/camera_sensor_properties.cpp > > > index 48305ac4..0625a0d8 100644 > > > --- a/src/libcamera/camera_sensor_properties.cpp > > > +++ b/src/libcamera/camera_sensor_properties.cpp > > > @@ -134,6 +134,19 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > > > { controls::draft::TestPatternModeColorBars, 1 }, > > > }, > > > } }, > > > + { "s5k3l6xx", { > > > + .unitCellSize = { 1120, 1120 }, > > > + .testPatternModes = { > > > + { 0, controls::draft::TestPatternModeOff }, > > > + { 1, controls::draft::TestPatternModeSolidColor }, > > > + { 2, controls::draft::TestPatternModeColorBars }, > > > + { 3, controls::draft::TestPatternModeColorBarsFadeToGray }, > > > + { 4, controls::draft::TestPatternModeCustom1 }, /* White */ > > > + { 5, controls::draft::TestPatternModePn9 }, > I just noticed those two are swapped. For instance, without a driver posted, or at least referenced, I can't review these control defintions ... > > > + { 6, controls::draft::TestPatternModeCustom1 + 1 }, /* LFSR32 */ > > > + { 7, controls::draft::TestPatternModeCustom1 + 2 }, /* Address */ > > > > That's an interesting way to define the extra custom modes... > > Interstingly, I don't see any users of TestPatternModeCustom1 yet. > > I found it directly in the yaml file, where the description suggests this way of dealing with it. It does seem reasonable to 'add offsets' to TestPatternModeCustom1 initially, but given that the control is called "TestPatternModeCustom1", I'd expect TestPatternModeCustom2 and TestPatternModeCustom3 if they were really required. Otherwise, we should probably call it TestPatternModeCustom and always require a custom offset. But I would hope we would really try to avoid 'Custom' controls like this unless they are absolutley necessary. Adding a new test pattern mode here is easy. It just needs a new definition in src/libcamera/control_ids.yaml with a description of the mode. For instance, the LFSR32 could be added like the Pn9: - name: TestPatternModeLfsr32 value: 5 description: | All pixel data is replaced by a pseudo-random sequence generated from an LFSR32 sequence generator. > > > > What is LFSR32 and Address? > Neither is documented in the application note, but LFSR32 appears to be a pseudo-random algorithm: https://gist.github.com/edarc/2946448 It might be more difficult to document TestPatternModeAddress, but can you generate a raw output with that mode set yet? If so - a quick examination might make it obvious that it's a sequential number relating to the address of the pixel perhaps - or it might be something else of course. But a test capture is likely the only way for us to find out. > > > > Is 'White' just a solid colour of 'white'? > > > Yes, the maximum integer value is returned. This sounds like it is certainly worthy of it's own named mode, and could be expected to be a mode used by other sensors. A clear name will help identifying the mode too. -- Kieran > > I suspect these modes should be defined within > > src/libcamera/control_ids.yaml where possible. An all white could be > > TestPatternModeSolidColorWhite or TestPatternModeSolidWhite for intance. > > > > -- > > Kieran > > > > > > > + }, > > > + } }, > > > }; > > > > > > const auto it = sensorProps.find(sensor); > > > -- > > > 2.31.1 > > > >
diff --git a/src/libcamera/camera_sensor_properties.cpp b/src/libcamera/camera_sensor_properties.cpp index 48305ac4..0625a0d8 100644 --- a/src/libcamera/camera_sensor_properties.cpp +++ b/src/libcamera/camera_sensor_properties.cpp @@ -134,6 +134,19 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen { controls::draft::TestPatternModeColorBars, 1 }, }, } }, + { "s5k3l6xx", { + .unitCellSize = { 1120, 1120 }, + .testPatternModes = { + { 0, controls::draft::TestPatternModeOff }, + { 1, controls::draft::TestPatternModeSolidColor }, + { 2, controls::draft::TestPatternModeColorBars }, + { 3, controls::draft::TestPatternModeColorBarsFadeToGray }, + { 4, controls::draft::TestPatternModeCustom1 }, /* White */ + { 5, controls::draft::TestPatternModePn9 }, + { 6, controls::draft::TestPatternModeCustom1 + 1 }, /* LFSR32 */ + { 7, controls::draft::TestPatternModeCustom1 + 2 }, /* Address */ + }, + } }, }; const auto it = sensorProps.find(sensor);
The sensor is called s5k3l6xx in the kernel. The driver is currently out of tree and maintained by Purism. It's the main camera sensor on the Librem 5. Signed-off-by: Dorota Czaplejewicz <dorota.czaplejewicz@puri.sm> --- src/libcamera/camera_sensor_properties.cpp | 13 +++++++++++++ 1 file changed, 13 insertions(+)