Re: [PATCH v7] media: add imx319 camera sensor driver
On Sun, Oct 21, 2018 at 6:53 AM Sakari Ailus 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 > > 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 > > > > 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 wrote: > > > > > > > > > > > > > > From: Bingbu Cao > > > > > > > > > > > > > > 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 > > > > > > > Cc: Sakari Ailus > > > > > > > Signed-off-by: Bingbu Cao > > > > > > > Signed-off-by: Tianshu Qiu > > > > > > > > > > > > > > --- > > > > > > > > > > > > > > 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
Re: [PATCH v7] media: add imx319 camera sensor driver
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 > 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 > > > 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 wrote: > > > > > > > > > > > > From: Bingbu Cao > > > > > > > > > > > > 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 > > > > > > Cc: Sakari Ailus > > > > > > Signed-off-by: Bingbu Cao > > > > > > Signed-off-by: Tianshu Qiu > > > > > > > > > > > > --- > > > > > > > > > > > > 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
Re: [PATCH v7] media: add imx319 camera sensor driver
On Tue, Oct 16, 2018 at 8:50 PM Sakari Ailus 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 > > 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 wrote: > > > > > > > > > > From: Bingbu Cao > > > > > > > > > > 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 > > > > > Cc: Sakari Ailus > > > > > Signed-off-by: Bingbu Cao > > > > > Signed-off-by: Tianshu Qiu > > > > > > > > > > --- > > > > > > > > > > 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
Re: [PATCH v7] media: add imx319 camera sensor driver
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 > 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 wrote: > > > > > > > > From: Bingbu Cao > > > > > > > > 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 > > > > Cc: Sakari Ailus > > > > Signed-off-by: Bingbu Cao > > > > Signed-off-by: Tianshu Qiu > > > > > > > > --- > > > > > > > > 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. > > 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. -- Kind regards, Sakari Ailus sakari.ai...@linux.intel.com
Re: [PATCH v7] media: add imx319 camera sensor driver
On Fri, Oct 12, 2018 at 4:58 PM Sakari Ailus 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 wrote: > > > > > > From: Bingbu Cao > > > > > > 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 > > > Cc: Sakari Ailus > > > Signed-off-by: Bingbu Cao > > > Signed-off-by: Tianshu Qiu > > > > > > --- > > > > > > 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. 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. Best regards, Tomasz
Re: [PATCH v7] media: add imx319 camera sensor driver
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 wrote: > > > > From: Bingbu Cao > > > > 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 > > Cc: Sakari Ailus > > Signed-off-by: Bingbu Cao > > Signed-off-by: Tianshu Qiu > > > > --- > > > > 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.). 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. > > 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. -- Kind regards, Sakari Ailus sakari.ai...@linux.intel.com
Re: [PATCH v7] media: add imx319 camera sensor driver
Hi Sakari, On Wed, Sep 26, 2018 at 11:38 AM wrote: > > From: Bingbu Cao > > 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 > Cc: Sakari Ailus > Signed-off-by: Bingbu Cao > Signed-off-by: Tianshu Qiu > > --- > > 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? 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. Best regards, Tomasz
Re: [PATCH v7] media: add imx319 camera sensor driver
On 09/26/2018 03:57 PM, Sakari Ailus wrote:
> Hi Bingbu,
>
> On Wed, Sep 26, 2018 at 10:42:18AM +0800, bingbu@intel.com wrote:
>> From: Bingbu Cao
>>
>> 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
>> Cc: Sakari Ailus
>> Signed-off-by: Bingbu Cao
>> Signed-off-by: Tianshu Qiu
>>
>> ---
>>
>> 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
> Thanks for the update!
>
> I've applied the patch with the following diff. Dividing a 64-bit number
> generally requires do_div() which was missed in the review:
Thanks!
>
> diff --git a/drivers/media/i2c/imx319.c b/drivers/media/i2c/imx319.c
> index e10d60f500dd..37c31d17ecf0 100644
> --- a/drivers/media/i2c/imx319.c
> +++ b/drivers/media/i2c/imx319.c
> @@ -2038,7 +2038,7 @@ imx319_set_pad_format(struct v4l2_subdev *sd,
> s32 vblank_def;
> s32 vblank_min;
> s64 h_blank;
> - s64 pixel_rate;
> + u64 pixel_rate;
> u32 height;
>
> mutex_lock(>mutex);
> @@ -2059,7 +2059,8 @@ imx319_set_pad_format(struct v4l2_subdev *sd,
> *framefmt = fmt->format;
> } else {
> imx319->cur_mode = mode;
> - pixel_rate = (imx319->link_def_freq * 2 * 4) / 10;
> + pixel_rate = imx319->link_def_freq * 2 * 4;
> + do_div(pixel_rate, 10);
> __v4l2_ctrl_s_ctrl_int64(imx319->pixel_rate, pixel_rate);
> /* Update limits and set FPS to default */
> height = imx319->cur_mode->height;
> @@ -2268,7 +2269,7 @@ static int imx319_init_controls(struct imx319 *imx319)
> s64 vblank_def;
> s64 vblank_min;
> s64 hblank;
> - s64 pixel_rate;
> + u64 pixel_rate;
> const struct imx319_mode *mode;
> u32 max;
> int ret;
> @@ -2287,7 +2288,8 @@ static int imx319_init_controls(struct imx319 *imx319)
> imx319->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>
> /* pixel_rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
> - pixel_rate = (imx319->link_def_freq * 2 * 4) / 10;
> + pixel_rate = imx319->link_def_freq * 2 * 4;
> + do_div(pixel_rate, 10);
> /* By default, PIXEL_RATE is read only */
> imx319->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, _ctrl_ops,
> V4L2_CID_PIXEL_RATE, pixel_rate,
>
Re: [PATCH v7] media: add imx319 camera sensor driver
Hi Bingbu,
On Wed, Sep 26, 2018 at 10:42:18AM +0800, bingbu@intel.com wrote:
> From: Bingbu Cao
>
> 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
> Cc: Sakari Ailus
> Signed-off-by: Bingbu Cao
> Signed-off-by: Tianshu Qiu
>
> ---
>
> 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
Thanks for the update!
I've applied the patch with the following diff. Dividing a 64-bit number
generally requires do_div() which was missed in the review:
diff --git a/drivers/media/i2c/imx319.c b/drivers/media/i2c/imx319.c
index e10d60f500dd..37c31d17ecf0 100644
--- a/drivers/media/i2c/imx319.c
+++ b/drivers/media/i2c/imx319.c
@@ -2038,7 +2038,7 @@ imx319_set_pad_format(struct v4l2_subdev *sd,
s32 vblank_def;
s32 vblank_min;
s64 h_blank;
- s64 pixel_rate;
+ u64 pixel_rate;
u32 height;
mutex_lock(>mutex);
@@ -2059,7 +2059,8 @@ imx319_set_pad_format(struct v4l2_subdev *sd,
*framefmt = fmt->format;
} else {
imx319->cur_mode = mode;
- pixel_rate = (imx319->link_def_freq * 2 * 4) / 10;
+ pixel_rate = imx319->link_def_freq * 2 * 4;
+ do_div(pixel_rate, 10);
__v4l2_ctrl_s_ctrl_int64(imx319->pixel_rate, pixel_rate);
/* Update limits and set FPS to default */
height = imx319->cur_mode->height;
@@ -2268,7 +2269,7 @@ static int imx319_init_controls(struct imx319 *imx319)
s64 vblank_def;
s64 vblank_min;
s64 hblank;
- s64 pixel_rate;
+ u64 pixel_rate;
const struct imx319_mode *mode;
u32 max;
int ret;
@@ -2287,7 +2288,8 @@ static int imx319_init_controls(struct imx319 *imx319)
imx319->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
/* pixel_rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
- pixel_rate = (imx319->link_def_freq * 2 * 4) / 10;
+ pixel_rate = imx319->link_def_freq * 2 * 4;
+ do_div(pixel_rate, 10);
/* By default, PIXEL_RATE is read only */
imx319->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, _ctrl_ops,
V4L2_CID_PIXEL_RATE, pixel_rate,
--
Kind regards,
Sakari Ailus
sakari.ai...@linux.intel.com
