Hi Laurent On Thu, 26 Jan 2023 at 16:01, Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> wrote: > > Hi Dave, > > On Thu, Jan 26, 2023 at 03:48:04PM +0000, Dave Stevenson wrote: > > On Thu, 26 Jan 2023 at 14:52, Sakari Ailus wrote: > > > 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. > > 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. > > As long as the driver reports the correct bayer pattern, it should be > fine. It does report the correct Bayer order so I would hope so too, however I know the hard coding that can go on in client apps! > Interactions between formats and flip controls is something we still > need to clarify. I plan to have a follow-up discussion on this with > Jacopo and Sakari after sending documentation patches for the > interactions between rotation and flips. If you would like to join the > fun, please let me know. Feel free to send me an invite for future discussions, however I'm currently assigned to other tasks and have been told to leave image sensors alone :-( > Also, if you have any comment on the rotation & flip discussion notes, > and especially if there's anything that doesn't seem right to you, > feedback would be appreciated. If everything is good, you can just ack > the documentation patches when I'll post them :-) I have the luxury of being able to largely ignore the existing clients, but the proposals as currently described seem like they should work for all parties. I'll respond to your docs when they're posted. > > 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). > > Thanks for sharing the branch. No problem. I am trying to persuade management here that it is worth the effort to upstream patches, but it's tough going sometimes. I do always try to ensure that our downstream patches follow the rules and have SoB etc, so others are at liberty to lift them if they wish. Dave > > [1] https://soho-enterprise.com/ > > [2] https://github.com/raspberrypi/linux/commits/rpi-5.15.y/drivers/media/i2c/imx258.c > > -- > Regards, > > Laurent Pinchart
