Hi Jacopo On Thu, 26 Jan 2023 at 20:03, Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx> wrote: > > Hi David > > On Thu, Jan 26, 2023 at 05:58:30PM +0000, Dave Stevenson wrote: > > Hi Jacopo > > > > On Thu, 26 Jan 2023 at 17:31, Jacopo Mondi > > <jacopo.mondi@xxxxxxxxxxxxxxxx> wrote: > > > > > > Hello > > > > > > On Thu, Jan 26, 2023 at 06:02:07PM +0200, Sakari Ailus wrote: > > > > Hi Dave, > > > > > > > > On Thu, Jan 26, 2023 at 03:48:04PM +0000, Dave Stevenson wrote: > > > > > Hi Jacopo and Sakari > > > > > > > > > > On Thu, 26 Jan 2023 at 14:52, Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx> wrote: > > > > > > > > > > > > Hi Jacopo, > > > > > > > > > > > > On Tue, Jan 17, 2023 at 11:06:00AM +0100, Jacopo Mondi wrote: > > > > > > > Currently the imx258 driver requires to have the 'rotation' device node > > > > > > > property specified in DTS with a fixed value of 180 degrees. > > > > > > > > > > > > > > The "rotation" fwnode device property is intended to allow specify the > > > > > > > sensor's physical mounting rotation, so that it can be exposed through > > > > > > > the read-only V4L2_CID_CAMERA_SENSOR_ROTATION control and applications > > > > > > > can decide how to compensate for that. > > > > > > > > > > > > > > The imx258 driver has read-only VFLIP and HFLIP enabled, resulting in > > > > > > > a 180 degrees image rotation being produced by the sensor. But this > > > > > > > doesn't imply that the physical mounting rotation should match the > > > > > > > driver's implementation. > > > > > > > > > > > > > > I took into the series Robert's patch that register device node properties and > > > > > > > on top of that register flips controls, in order to remove the hard requirement > > > > > > > of the 180 degrees rotation property presence. > > > > > > > > > > > > Reconsidering these patches after the flipping vs. rotation discussion, > > > > > > they seem fine. The only thing I'd like to see, after removing the rotation > > > > > > property check, would be to add support for the actual flipping controls. > > > > > > I'm pretty sure they can be found in the same registers as on CCS --- the > > > > > > rest of the registers look very much like that. Would you like to send a > > > > > > patch? :-) > > > > > > > > > > Yes it is register 0x0101, bits 0 (H) & 1 (V). > > > > > > > > > > Do watch out as there are register errors in the driver. Currently > > > > > Y_ADD_STA is set to 0, and Y_ADD_END to 3118, giving 3119 lines total. > > > > > > > > Yes, this is the problem with register list based drivers. Well spotted. > > > > > > > > I remember one driver for a Toshiba sensor using value of 5 for a register > > > > the range of which was 2--10, but only even values were allowed. It worked > > > > nonetheless... oh well. > > > > > > > > I wonder if this sensor would work better with the CCS driver > > > > > > > > > That means that when you initially implement flips the Bayer order > > > > > won't change, but you change the field of view by one line. > > > > > The start and end values also break the requirements listed in the > > > > > datasheets for STA/END values being multiples of X (table 4-2 of the > > > > > datasheet). Correcting that will change the Bayer order when inverted. > > > > > Does that count as a regression to userspace? I hope not. Memory says > > > > > that if you don't correct Y_ADD_END then some of the binned modes > > > > > misbehave. > > > > > > > > Most sensors also require even values for the ?_ADDR_START registers (and > > > > odd for the _ADDR_END registers). Using an invalid value sometimes might > > > > work, too, but only testing will tell. > > > > > > > > > > > > > > I have been through this loop before as Soho Enterprise [1] make an > > > > > IMX258 board for the Pi. I haven't upstreamed the patches [2] though > > > > > (sorry). > > > > > > > > It'd be nice if both worked with the same driver. > > > > > > > > > > There are a lot of interesting changes in here that would be worth > > > upstreaming > > > https://github.com/raspberrypi/linux/commits/rpi-5.15.y/drivers/media/i2c/imx258.c > > > > I do feel a little guilty for not upstreaming these patches myself, > > but I'm currently not being given any time to do so. Perhaps I'll take > > an afternoon off and blitz a load of patches (imx258, imx290, and > > ov7251 for a start). > > > > Obviously that branch is based on 5.15. I was going to say that the > > rpi-6.2.y branch may be a better source, however it looks like all the > > commits got squashed :-( > > > > > I would prefer if we can get these three easy patches of minr in and > > > then start shoveling the good stuff from the rpi repo ? > > > > > > Otherwise I can plumb flip support in with the current wrong totals > > > which, if I understand you right, doesn't require changing the bayer > > > patter order ? > > > > Memory says VFLIP won't currently change Bayer order, but I think HFLIP will. > > If it is permitted to merge the current read-only FLIP patches, then > > I'd suggest doing that initially. > > > > Out of interest, do you have a user of imx258, or is this just as clean ups? > > It may be possible to get a sample from Soho Enterprises if you > > explain your involvement in libcamera. They're generally very > > approachable. > > > > Thanks for the hint. > > I'm using the imx258 on the PinephonePro, so I have a testing device. > > I don't see a tuning file for the sensor in libcamera, does Soho > Enterprise ever published one ? I don't recall having seen a final tuning file for imx258, but it would be for the Pi IPA anyway which would make it less applicable for the PinephonePro. They've been making a number of modules, and I need to encourage them to upstream their drivers and tunings. Their main guy is ex-Sony, so he's fairly hot on image quality and tuning, but has turned to me for a moderate amount of the kernel side detail. It looks like https://forums.raspberrypi.com/viewtopic.php?t=331819 was our forum thread discussing imx258. The incorrect register settings were giving white lines on the edges of the images, particularly on the binned modes. I have no information on HDR or PDAF for imx258, although I do know both are different from IMX708. Also note that PDAF is optional in the module, so shielded pixel correction needs to be correctly set for the module. Feel free to shout if you need me to test any driver changes, although I'll keep an eye out for any imx258 patches anyway. Dave > > Dave > > > > > > > > > -- > > > > Kind regards, > > > > > > > > Sakari Ailus
