On Sun, Oct 21, 2018 at 6:53 AM Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx> wrote: > > Hi Tomasz, > > On Thu, Oct 18, 2018 at 12:24:31PM +0900, Tomasz Figa wrote: > > On Tue, Oct 16, 2018 at 8:50 PM Sakari Ailus > > <sakari.ailus@xxxxxxxxxxxxxxx> wrote: > > > > > > Hi Tomasz, > > > > > > On Fri, Oct 12, 2018 at 05:06:56PM +0900, Tomasz Figa wrote: > > > > On Fri, Oct 12, 2018 at 4:58 PM Sakari Ailus > > > > <sakari.ailus@xxxxxxxxxxxxxxx> wrote: > > > > > > > > > > Hi Tomasz, > > > > > > > > > > On Fri, Oct 12, 2018 at 01:51:10PM +0900, Tomasz Figa wrote: > > > > > > Hi Sakari, > > > > > > > > > > > > On Wed, Sep 26, 2018 at 11:38 AM <bingbu.cao@xxxxxxxxx> wrote: > > > > > > > > > > > > > > From: Bingbu Cao <bingbu.cao@xxxxxxxxx> > > > > > > > > > > > > > > Add a v4l2 sub-device driver for the Sony imx319 image sensor. > > > > > > > This is a camera sensor using the i2c bus for control and the > > > > > > > csi-2 bus for data. > > > > > > > > > > > > > > This driver supports following features: > > > > > > > - manual exposure and analog/digital gain control support > > > > > > > - vblank/hblank control support > > > > > > > - 4 test patterns control support > > > > > > > - vflip/hflip control support (will impact the output bayer order) > > > > > > > - support following resolutions: > > > > > > > - 3264x2448, 3280x2464 @ 30fps > > > > > > > - 1936x1096, 1920x1080 @ 60fps > > > > > > > - 1640x1232, 1640x922, 1296x736, 1280x720 @ 120fps > > > > > > > - support 4 bayer orders output (via change v/hflip) > > > > > > > - SRGGB10(default), SGRBG10, SGBRG10, SBGGR10 > > > > > > > > > > > > > > Cc: Tomasz Figa <tfiga@xxxxxxxxxxxx> > > > > > > > Cc: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx> > > > > > > > Signed-off-by: Bingbu Cao <bingbu.cao@xxxxxxxxx> > > > > > > > Signed-off-by: Tianshu Qiu <tian.shu.qiu@xxxxxxxxx> > > > > > > > > > > > > > > --- > > > > > > > > > > > > > > This patch is based on sakari's media-tree git: > > > > > > > https://git.linuxtv.org/sailus/media_tree.git/log/?h=for-4.20-1 > > > > > > > > > > > > > > Changes from v5: > > > > > > > - add some comments for gain calculation > > > > > > > - use lock to protect the format > > > > > > > - fix some style issues > > > > > > > > > > > > > > Changes from v4 to v5: > > > > > > > - use single PLL for all internal clocks > > > > > > > - change link frequency to 482.4MHz > > > > > > > - adjust frame timing for 2x2 binning modes > > > > > > > and enlarge frame readout time > > > > > > > - get CSI-2 link frequencies and external clock > > > > > > > from firmware > > > > > > > > > > > > If I remember correctly, that was suggested by you. Why do we need to > > > > > > specify link frequency in firmware if it's fully configured by the > > > > > > driver, with the only external dependency being the external clock? > > > > > > > > > > The driver that's now in upstream supports, for now, a very limited set of > > > > > configurations from what the sensor supports. These are more or less > > > > > tailored to the particular system where it is being used right now (output > > > > > image size, external clock frequency, frame rates, link frequencies etc.). > > > > > > > > As a side note, they're tailored to exactly the system I mentioned, > > > > with different link frequency hardcoded in the firmware, coming from > > > > earlier stage of development. > > > > > > > > > If the same sensor is needed elsewhere (quite likely), the configuration > > > > > needed elsewhere is very likely to be different from what you're using now. > > > > > > > > > > The link frequency in particular is important as using a different link > > > > > frequency (which could be fine elsewhere) could cause EMI issues, e.g. > > > > > rendering your GPS receiver inoperable during the time the camera sensor is > > > > > streaming images. > > > > > > > > > > Should new configurations be added to this driver to support a different > > > > > system, the link frequencies used by those configurations may be > > > > > problematic to your system, and after a software update the driver could as > > > > > well use those new frequencies. That's a big no-no. > > > > > > > > > > > > > Okay, those are some valid points indeed, thanks for clarifying. > > > > > > > > > > > > > > > > We're having problems with firmware listing the link frequency from v4 > > > > > > and we can't easily change it anymore to report the new one. I feel > > > > > > like this dependency on the firmware here is unnecessary, as long as > > > > > > the external clock frequency matches. > > > > > > > > > > This is information you really need to know. > > > > > > > > > > A number of older drivers do not use the link frequency information from > > > > > the firmware but that comes with a risk. Really, it's better to change the > > > > > frequency now to something you can choose, rather than have it changed > > > > > later on to something someone else chose for you. > > > > > > > > I guess it means that we have to carry a local downstream patch that > > > > bypasses this check, since we cannot easily change the firmware > > > > anymore. > > > > > > Is there a possibility update the firmware, or carry an SSDT overlay as part > > > of the software? The options are laid out in > > > Documentation/acpi/ssdt-overlays.txt . Do remember to pay attention to the > > > revision field --- also in future Coreboot updates. > > > > > > > Generally we try to avoid updating the firmware in the field, but most > > of the time there is a reason to do it anyway, so that might > > eventually happen. Let me try to figure out. > > > > > > > > > > An alternative would be to make the driver try to select a frequency > > > > that matches what's in the firmware, but issue a warning and fall back > > > > to a default one if a matching is not found. It might be actually > > > > better than nothing for some early testing on new systems, since it > > > > wouldn't require firmware changes. > > > > > > You don't need firmware changes per se; see above. Allowing that will very, > > > very easily lead this being unaddressed during developement and changing > > > later on inadvertly. > > > > Still, requiring the user to create an SSDT overlay sounds like an > > overkill, for something that is not really a fatal error. We handle > > ACPI firmware bugs in many parts of the kernel by issuing a warning > > and using some reasonably safe fallback and I don't know why we > > couldn't do the same here. > > To address this in kernel only, I presume one would have to plant that > snippet in the ACPI table based on recognising the machine name and the > firmware version. I don't think this is supported right now. My point is that we normally handle such mismatches in the kernel by just printing a warning and forcing whatever the kernel needs for the device to work. For the cases where such override is not sensible by default (due to potential side-effects or so), a command line argument is often provided to force ignoring the firmware data. > > OTOH, an alternative option could be, if you use initrd for other reasons, > to bake the SSDT overlay into the initrd based on the machine > identification when creating the initrd image. That would require no kernel > changes and compared to making kernel changes, this way also there would be > no adverse effect on the size of the kernel. We don't use initrd. Generally it looks like the simplest way forward is to carry a downstream patch turning the driver error into a warning. It's probably the simplest thing to do, if we can't have it handled upstream. Best regards, Tomasz
