* [Patch v2 0/3] media: ov5640: updates
@ 2019-10-02 13:51 Benoit Parrot
2019-10-02 13:51 ` [Patch v2 1/3] media: ov5640: add PIXEL_RATE control Benoit Parrot
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Benoit Parrot @ 2019-10-02 13:51 UTC (permalink / raw)
To: Hans Verkuil, Sakari Ailus
Cc: linux-media, devicetree, linux-kernel, Benoit Parrot
This patch series is a collection of patches we have been carrying for a
while.
First, it adds support for PIXEL_RATE control which is used by some
CSI2 receiver driver to properly set-up their DPHY.
Then we fix an issue related to having extra sensor enable/disable in
the register array for the 1920x1080 mode.
Finally we restrict the largest resolution which should only be
available at the lowest FPS.
Changes since v1:
- Addressed comment from Sakari.
added a function to calculate the pixel rate and remove the need to
cache its value
Benoit Parrot (3):
media: ov5640: add PIXEL_RATE control
media: ov5640: Fix 1920x1080 mode to remove extra enable/disable
media: ov5640: Make 2592x1944 mode only available at 15 fps
drivers/media/i2c/ov5640.c | 33 +++++++++++++++++++++++++++++++--
1 file changed, 31 insertions(+), 2 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Patch v2 1/3] media: ov5640: add PIXEL_RATE control
2019-10-02 13:51 [Patch v2 0/3] media: ov5640: updates Benoit Parrot
@ 2019-10-02 13:51 ` Benoit Parrot
2019-10-03 7:17 ` Jacopo Mondi
2019-10-02 13:51 ` [Patch v2 2/3] media: ov5640: Fix 1920x1080 mode to remove extra enable/disable Benoit Parrot
2019-10-02 13:51 ` [Patch v2 3/3] media: ov5640: Make 2592x1944 mode only available at 15 fps Benoit Parrot
2 siblings, 1 reply; 12+ messages in thread
From: Benoit Parrot @ 2019-10-02 13:51 UTC (permalink / raw)
To: Hans Verkuil, Sakari Ailus
Cc: linux-media, devicetree, linux-kernel, Benoit Parrot
Add v4l2 controls to report the pixel rates of each mode. This is
needed by some CSI2 receiver in order to perform proper DPHY
configuration.
Signed-off-by: Benoit Parrot <bparrot@ti.com>
---
drivers/media/i2c/ov5640.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 500d9bbff10b..5198dc887400 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -193,6 +193,9 @@ struct ov5640_mode_info {
struct ov5640_ctrls {
struct v4l2_ctrl_handler handler;
+ struct {
+ struct v4l2_ctrl *pixel_rate;
+ };
struct {
struct v4l2_ctrl *auto_exp;
struct v4l2_ctrl *exposure;
@@ -2194,6 +2197,16 @@ static int ov5640_try_fmt_internal(struct v4l2_subdev *sd,
return 0;
}
+static u64 ov5640_calc_pixel_rate(struct ov5640_dev *sensor)
+{
+ u64 rate;
+
+ rate = sensor->current_mode->vtot * sensor->current_mode->htot;
+ rate *= ov5640_framerates[sensor->current_fr];
+
+ return rate;
+}
+
static int ov5640_set_fmt(struct v4l2_subdev *sd,
struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_format *format)
@@ -2233,6 +2246,8 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd,
if (mbus_fmt->code != sensor->fmt.code)
sensor->pending_fmt_change = true;
+ __v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixel_rate,
+ ov5640_calc_pixel_rate(sensor));
out:
mutex_unlock(&sensor->lock);
return ret;
@@ -2657,6 +2672,13 @@ static int ov5640_init_controls(struct ov5640_dev *sensor)
/* we can use our own mutex for the ctrl lock */
hdl->lock = &sensor->lock;
+ /* Clock related controls */
+ ctrls->pixel_rate =
+ v4l2_ctrl_new_std(hdl, ops,
+ V4L2_CID_PIXEL_RATE, 0, INT_MAX, 1,
+ ov5640_calc_pixel_rate(sensor));
+ ctrls->pixel_rate->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
/* Auto/manual white balance */
ctrls->auto_wb = v4l2_ctrl_new_std(hdl, ops,
V4L2_CID_AUTO_WHITE_BALANCE,
@@ -2816,6 +2838,9 @@ static int ov5640_s_frame_interval(struct v4l2_subdev *sd,
sensor->frame_interval = fi->interval;
sensor->current_mode = mode;
sensor->pending_mode_change = true;
+
+ __v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixel_rate,
+ ov5640_calc_pixel_rate(sensor));
}
out:
mutex_unlock(&sensor->lock);
--
2.17.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [Patch v2 2/3] media: ov5640: Fix 1920x1080 mode to remove extra enable/disable
2019-10-02 13:51 [Patch v2 0/3] media: ov5640: updates Benoit Parrot
2019-10-02 13:51 ` [Patch v2 1/3] media: ov5640: add PIXEL_RATE control Benoit Parrot
@ 2019-10-02 13:51 ` Benoit Parrot
2019-10-03 7:26 ` Jacopo Mondi
2019-10-02 13:51 ` [Patch v2 3/3] media: ov5640: Make 2592x1944 mode only available at 15 fps Benoit Parrot
2 siblings, 1 reply; 12+ messages in thread
From: Benoit Parrot @ 2019-10-02 13:51 UTC (permalink / raw)
To: Hans Verkuil, Sakari Ailus
Cc: linux-media, devicetree, linux-kernel, Benoit Parrot
In the 1920x1080 register array an extra pair of reset ctrl disable
re-enable was causing unwanted init delays.
Signed-off-by: Benoit Parrot <bparrot@ti.com>
---
drivers/media/i2c/ov5640.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 5198dc887400..103a4e8f88e1 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -492,7 +492,6 @@ static const struct reg_value ov5640_setting_720P_1280_720[] = {
};
static const struct reg_value ov5640_setting_1080P_1920_1080[] = {
- {0x3008, 0x42, 0, 0},
{0x3c07, 0x08, 0, 0},
{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
{0x3814, 0x11, 0, 0},
@@ -520,7 +519,7 @@ static const struct reg_value ov5640_setting_1080P_1920_1080[] = {
{0x3a0e, 0x03, 0, 0}, {0x3a0d, 0x04, 0, 0}, {0x3a14, 0x04, 0, 0},
{0x3a15, 0x60, 0, 0}, {0x4407, 0x04, 0, 0},
{0x460b, 0x37, 0, 0}, {0x460c, 0x20, 0, 0}, {0x3824, 0x04, 0, 0},
- {0x4005, 0x1a, 0, 0}, {0x3008, 0x02, 0, 0},
+ {0x4005, 0x1a, 0, 0},
};
static const struct reg_value ov5640_setting_QSXGA_2592_1944[] = {
--
2.17.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [Patch v2 3/3] media: ov5640: Make 2592x1944 mode only available at 15 fps
2019-10-02 13:51 [Patch v2 0/3] media: ov5640: updates Benoit Parrot
2019-10-02 13:51 ` [Patch v2 1/3] media: ov5640: add PIXEL_RATE control Benoit Parrot
2019-10-02 13:51 ` [Patch v2 2/3] media: ov5640: Fix 1920x1080 mode to remove extra enable/disable Benoit Parrot
@ 2019-10-02 13:51 ` Benoit Parrot
2019-10-03 7:31 ` Jacopo Mondi
2 siblings, 1 reply; 12+ messages in thread
From: Benoit Parrot @ 2019-10-02 13:51 UTC (permalink / raw)
To: Hans Verkuil, Sakari Ailus
Cc: linux-media, devicetree, linux-kernel, Benoit Parrot
The sensor data sheet clearly state that 2592x1944 only works at 15 fps
make sure we don't try to miss configure the pll out of acceptable
range.
Signed-off-by: Benoit Parrot <bparrot@ti.com>
---
drivers/media/i2c/ov5640.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 103a4e8f88e1..d5b0be2c7a0a 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -1613,6 +1613,11 @@ ov5640_find_mode(struct ov5640_dev *sensor, enum ov5640_frame_rate fr,
!(mode->hact == 640 && mode->vact == 480))
return NULL;
+ /* 2592x1944 only works at 15fps */
+ if (fr != OV5640_15_FPS &&
+ (mode->hact == 2592 && mode->vact == 1944))
+ return NULL;
+
return mode;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [Patch v2 1/3] media: ov5640: add PIXEL_RATE control
2019-10-02 13:51 ` [Patch v2 1/3] media: ov5640: add PIXEL_RATE control Benoit Parrot
@ 2019-10-03 7:17 ` Jacopo Mondi
2019-10-03 7:22 ` Sakari Ailus
2019-10-03 11:59 ` Benoit Parrot
0 siblings, 2 replies; 12+ messages in thread
From: Jacopo Mondi @ 2019-10-03 7:17 UTC (permalink / raw)
To: Benoit Parrot
Cc: Hans Verkuil, Sakari Ailus, linux-media, devicetree, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 3218 bytes --]
Hi Benoit,
On Wed, Oct 02, 2019 at 08:51:32AM -0500, Benoit Parrot wrote:
> Add v4l2 controls to report the pixel rates of each mode. This is
> needed by some CSI2 receiver in order to perform proper DPHY
> configuration.
>
> Signed-off-by: Benoit Parrot <bparrot@ti.com>
> ---
> drivers/media/i2c/ov5640.c | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 500d9bbff10b..5198dc887400 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -193,6 +193,9 @@ struct ov5640_mode_info {
>
> struct ov5640_ctrls {
> struct v4l2_ctrl_handler handler;
> + struct {
> + struct v4l2_ctrl *pixel_rate;
> + };
Do you need to wrap this v4l2_ctrl in it's own unnamed struct? Other
controls here declared in this way are clustered and, if I'm not
mistaken, using unnamed struct to wrap them is just a typographically
nice way to convey that. I think your new control could be declared
without a wrapping struct { }.
> struct {
> struct v4l2_ctrl *auto_exp;
> struct v4l2_ctrl *exposure;
> @@ -2194,6 +2197,16 @@ static int ov5640_try_fmt_internal(struct v4l2_subdev *sd,
> return 0;
> }
>
> +static u64 ov5640_calc_pixel_rate(struct ov5640_dev *sensor)
> +{
> + u64 rate;
> +
> + rate = sensor->current_mode->vtot * sensor->current_mode->htot;
> + rate *= ov5640_framerates[sensor->current_fr];
> +
> + return rate;
> +}
> +
Just to point out this is the -theoretical- pixel rate, and might be
quite different from the one calculated by the clock tree tuning
procedure (which should be updated to match Hugues' latest findings).
> static int ov5640_set_fmt(struct v4l2_subdev *sd,
> struct v4l2_subdev_pad_config *cfg,
> struct v4l2_subdev_format *format)
> @@ -2233,6 +2246,8 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd,
> if (mbus_fmt->code != sensor->fmt.code)
> sensor->pending_fmt_change = true;
>
> + __v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixel_rate,
> + ov5640_calc_pixel_rate(sensor));
> out:
> mutex_unlock(&sensor->lock);
> return ret;
> @@ -2657,6 +2672,13 @@ static int ov5640_init_controls(struct ov5640_dev *sensor)
> /* we can use our own mutex for the ctrl lock */
> hdl->lock = &sensor->lock;
>
> + /* Clock related controls */
> + ctrls->pixel_rate =
> + v4l2_ctrl_new_std(hdl, ops,
If you like it better, this could fit in 1 line
ctrls->pixel_rate = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_PIXEL_RATE,
0, INT_MAX, 1,
ov5640_calc_pixel_rate(sensor)
Thanks
j
> + V4L2_CID_PIXEL_RATE, 0, INT_MAX, 1,
> + ov5640_calc_pixel_rate(sensor));
> + ctrls->pixel_rate->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
> /* Auto/manual white balance */
> ctrls->auto_wb = v4l2_ctrl_new_std(hdl, ops,
> V4L2_CID_AUTO_WHITE_BALANCE,
> @@ -2816,6 +2838,9 @@ static int ov5640_s_frame_interval(struct v4l2_subdev *sd,
> sensor->frame_interval = fi->interval;
> sensor->current_mode = mode;
> sensor->pending_mode_change = true;
> +
> + __v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixel_rate,
> + ov5640_calc_pixel_rate(sensor));
> }
> out:
> mutex_unlock(&sensor->lock);
> --
> 2.17.1
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch v2 1/3] media: ov5640: add PIXEL_RATE control
2019-10-03 7:17 ` Jacopo Mondi
@ 2019-10-03 7:22 ` Sakari Ailus
2019-10-03 12:07 ` Benoit Parrot
2019-10-03 11:59 ` Benoit Parrot
1 sibling, 1 reply; 12+ messages in thread
From: Sakari Ailus @ 2019-10-03 7:22 UTC (permalink / raw)
To: Jacopo Mondi
Cc: Benoit Parrot, Hans Verkuil, linux-media, devicetree, linux-kernel
Hi Jacopo, Benoit,
On Thu, Oct 03, 2019 at 09:17:14AM +0200, Jacopo Mondi wrote:
> Hi Benoit,
>
> On Wed, Oct 02, 2019 at 08:51:32AM -0500, Benoit Parrot wrote:
> > Add v4l2 controls to report the pixel rates of each mode. This is
> > needed by some CSI2 receiver in order to perform proper DPHY
> > configuration.
> >
> > Signed-off-by: Benoit Parrot <bparrot@ti.com>
> > ---
> > drivers/media/i2c/ov5640.c | 25 +++++++++++++++++++++++++
> > 1 file changed, 25 insertions(+)
> >
> > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > index 500d9bbff10b..5198dc887400 100644
> > --- a/drivers/media/i2c/ov5640.c
> > +++ b/drivers/media/i2c/ov5640.c
> > @@ -193,6 +193,9 @@ struct ov5640_mode_info {
> >
> > struct ov5640_ctrls {
> > struct v4l2_ctrl_handler handler;
> > + struct {
> > + struct v4l2_ctrl *pixel_rate;
> > + };
>
> Do you need to wrap this v4l2_ctrl in it's own unnamed struct? Other
> controls here declared in this way are clustered and, if I'm not
> mistaken, using unnamed struct to wrap them is just a typographically
> nice way to convey that. I think your new control could be declared
> without a wrapping struct { }.
>
> > struct {
> > struct v4l2_ctrl *auto_exp;
> > struct v4l2_ctrl *exposure;
> > @@ -2194,6 +2197,16 @@ static int ov5640_try_fmt_internal(struct v4l2_subdev *sd,
> > return 0;
> > }
> >
> > +static u64 ov5640_calc_pixel_rate(struct ov5640_dev *sensor)
> > +{
> > + u64 rate;
> > +
> > + rate = sensor->current_mode->vtot * sensor->current_mode->htot;
> > + rate *= ov5640_framerates[sensor->current_fr];
> > +
> > + return rate;
> > +}
> > +
>
> Just to point out this is the -theoretical- pixel rate, and might be
> quite different from the one calculated by the clock tree tuning
> procedure (which should be updated to match Hugues' latest findings).
Hmm. Considering the xclk rate may be pretty much anything, I'd suppose
the value above would only be correct for a given xclk rate.
Could this be simply calculated from the clock tree configuration, to get
the right value in all cases?
>
> > static int ov5640_set_fmt(struct v4l2_subdev *sd,
> > struct v4l2_subdev_pad_config *cfg,
> > struct v4l2_subdev_format *format)
> > @@ -2233,6 +2246,8 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd,
> > if (mbus_fmt->code != sensor->fmt.code)
> > sensor->pending_fmt_change = true;
> >
> > + __v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixel_rate,
> > + ov5640_calc_pixel_rate(sensor));
> > out:
> > mutex_unlock(&sensor->lock);
> > return ret;
> > @@ -2657,6 +2672,13 @@ static int ov5640_init_controls(struct ov5640_dev *sensor)
> > /* we can use our own mutex for the ctrl lock */
> > hdl->lock = &sensor->lock;
> >
> > + /* Clock related controls */
> > + ctrls->pixel_rate =
> > + v4l2_ctrl_new_std(hdl, ops,
>
> If you like it better, this could fit in 1 line
>
> ctrls->pixel_rate = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_PIXEL_RATE,
> 0, INT_MAX, 1,
> ov5640_calc_pixel_rate(sensor)
>
> Thanks
> j
>
> > + V4L2_CID_PIXEL_RATE, 0, INT_MAX, 1,
> > + ov5640_calc_pixel_rate(sensor));
>
>
> > + ctrls->pixel_rate->flags |= V4L2_CTRL_FLAG_READ_ONLY;
Note that ctrls->pixel_rate is NULL if e.g. memory allocation fails when
creating the control.
> > +
> > /* Auto/manual white balance */
> > ctrls->auto_wb = v4l2_ctrl_new_std(hdl, ops,
> > V4L2_CID_AUTO_WHITE_BALANCE,
> > @@ -2816,6 +2838,9 @@ static int ov5640_s_frame_interval(struct v4l2_subdev *sd,
> > sensor->frame_interval = fi->interval;
> > sensor->current_mode = mode;
> > sensor->pending_mode_change = true;
> > +
> > + __v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixel_rate,
> > + ov5640_calc_pixel_rate(sensor));
> > }
> > out:
> > mutex_unlock(&sensor->lock);
--
Regards,
Sakari Ailus
sakari.ailus@linux.intel.com
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch v2 2/3] media: ov5640: Fix 1920x1080 mode to remove extra enable/disable
2019-10-02 13:51 ` [Patch v2 2/3] media: ov5640: Fix 1920x1080 mode to remove extra enable/disable Benoit Parrot
@ 2019-10-03 7:26 ` Jacopo Mondi
0 siblings, 0 replies; 12+ messages in thread
From: Jacopo Mondi @ 2019-10-03 7:26 UTC (permalink / raw)
To: Benoit Parrot
Cc: Hans Verkuil, Sakari Ailus, linux-media, devicetree, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1696 bytes --]
Hi Benoit,
good catch. I wonder why those power down/up sequences where only
set for this mode...
I also wonder which kind of power down mode do we enter, if the chip
is set in 'software power down mode' with 0x3008=0x42 at the beginning
of the register blob write sequence, and we still can successfully
program registers..
In any case, assuming 720p still works:
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
Thanks
j
On Wed, Oct 02, 2019 at 08:51:33AM -0500, Benoit Parrot wrote:
> In the 1920x1080 register array an extra pair of reset ctrl disable
> re-enable was causing unwanted init delays.
>
> Signed-off-by: Benoit Parrot <bparrot@ti.com>
> ---
> drivers/media/i2c/ov5640.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 5198dc887400..103a4e8f88e1 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -492,7 +492,6 @@ static const struct reg_value ov5640_setting_720P_1280_720[] = {
> };
>
> static const struct reg_value ov5640_setting_1080P_1920_1080[] = {
> - {0x3008, 0x42, 0, 0},
> {0x3c07, 0x08, 0, 0},
> {0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
> {0x3814, 0x11, 0, 0},
> @@ -520,7 +519,7 @@ static const struct reg_value ov5640_setting_1080P_1920_1080[] = {
> {0x3a0e, 0x03, 0, 0}, {0x3a0d, 0x04, 0, 0}, {0x3a14, 0x04, 0, 0},
> {0x3a15, 0x60, 0, 0}, {0x4407, 0x04, 0, 0},
> {0x460b, 0x37, 0, 0}, {0x460c, 0x20, 0, 0}, {0x3824, 0x04, 0, 0},
> - {0x4005, 0x1a, 0, 0}, {0x3008, 0x02, 0, 0},
> + {0x4005, 0x1a, 0, 0},
> };
>
> static const struct reg_value ov5640_setting_QSXGA_2592_1944[] = {
> --
> 2.17.1
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch v2 3/3] media: ov5640: Make 2592x1944 mode only available at 15 fps
2019-10-02 13:51 ` [Patch v2 3/3] media: ov5640: Make 2592x1944 mode only available at 15 fps Benoit Parrot
@ 2019-10-03 7:31 ` Jacopo Mondi
2019-10-03 12:09 ` Benoit Parrot
0 siblings, 1 reply; 12+ messages in thread
From: Jacopo Mondi @ 2019-10-03 7:31 UTC (permalink / raw)
To: Benoit Parrot
Cc: Hans Verkuil, Sakari Ailus, linux-media, devicetree, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1516 bytes --]
Hi Benoit,
On Wed, Oct 02, 2019 at 08:51:34AM -0500, Benoit Parrot wrote:
> The sensor data sheet clearly state that 2592x1944 only works at 15 fps
> make sure we don't try to miss configure the pll out of acceptable
> range.
The datasheet clearly indicates that 15 fps is the maximum achievable
rate with that resolution, so I guess preventing it from being set
to anything faster than that it's acceptable.
>
> Signed-off-by: Benoit Parrot <bparrot@ti.com>
> ---
> drivers/media/i2c/ov5640.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 103a4e8f88e1..d5b0be2c7a0a 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -1613,6 +1613,11 @@ ov5640_find_mode(struct ov5640_dev *sensor, enum ov5640_frame_rate fr,
> !(mode->hact == 640 && mode->vact == 480))
> return NULL;
>
> + /* 2592x1944 only works at 15fps */
> + if (fr != OV5640_15_FPS &&
As long as 15 fps is the lower framerate declared in
ov5640_framerates[] this is ok, but I would make this condition a
check for "fr > OV5640_15_FPS" so that it's safe for future
extensions.
(And I would check for the resolution first then FPS, so you check
the most unlikely condition first, but that's really a minor
optimization).
With the above small details addressed
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
Thanks
j
> + (mode->hact == 2592 && mode->vact == 1944))
> + return NULL;
> +
> return mode;
> }
>
> --
> 2.17.1
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch v2 1/3] media: ov5640: add PIXEL_RATE control
2019-10-03 7:17 ` Jacopo Mondi
2019-10-03 7:22 ` Sakari Ailus
@ 2019-10-03 11:59 ` Benoit Parrot
1 sibling, 0 replies; 12+ messages in thread
From: Benoit Parrot @ 2019-10-03 11:59 UTC (permalink / raw)
To: Jacopo Mondi
Cc: Hans Verkuil, Sakari Ailus, linux-media, devicetree, linux-kernel
Jacopo Mondi <jacopo@jmondi.org> wrote on Thu [2019-Oct-03 09:17:14 +0200]:
> Hi Benoit,
>
> On Wed, Oct 02, 2019 at 08:51:32AM -0500, Benoit Parrot wrote:
> > Add v4l2 controls to report the pixel rates of each mode. This is
> > needed by some CSI2 receiver in order to perform proper DPHY
> > configuration.
> >
> > Signed-off-by: Benoit Parrot <bparrot@ti.com>
> > ---
> > drivers/media/i2c/ov5640.c | 25 +++++++++++++++++++++++++
> > 1 file changed, 25 insertions(+)
> >
> > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > index 500d9bbff10b..5198dc887400 100644
> > --- a/drivers/media/i2c/ov5640.c
> > +++ b/drivers/media/i2c/ov5640.c
> > @@ -193,6 +193,9 @@ struct ov5640_mode_info {
> >
> > struct ov5640_ctrls {
> > struct v4l2_ctrl_handler handler;
> > + struct {
> > + struct v4l2_ctrl *pixel_rate;
> > + };
>
> Do you need to wrap this v4l2_ctrl in it's own unnamed struct? Other
> controls here declared in this way are clustered and, if I'm not
> mistaken, using unnamed struct to wrap them is just a typographically
> nice way to convey that. I think your new control could be declared
> without a wrapping struct { }.
Probably not, just tried to be consistent with the rest of code here.
>
> > struct {
> > struct v4l2_ctrl *auto_exp;
> > struct v4l2_ctrl *exposure;
> > @@ -2194,6 +2197,16 @@ static int ov5640_try_fmt_internal(struct v4l2_subdev *sd,
> > return 0;
> > }
> >
> > +static u64 ov5640_calc_pixel_rate(struct ov5640_dev *sensor)
> > +{
> > + u64 rate;
> > +
> > + rate = sensor->current_mode->vtot * sensor->current_mode->htot;
> > + rate *= ov5640_framerates[sensor->current_fr];
> > +
> > + return rate;
> > +}
> > +
>
> Just to point out this is the -theoretical- pixel rate, and might be
> quite different from the one calculated by the clock tree tuning
> procedure (which should be updated to match Hugues' latest findings).
True, and to my surprise my receiver worked with all of those value even if
some actual value maybe off, I guess in my case they were close enough.
>
> > static int ov5640_set_fmt(struct v4l2_subdev *sd,
> > struct v4l2_subdev_pad_config *cfg,
> > struct v4l2_subdev_format *format)
> > @@ -2233,6 +2246,8 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd,
> > if (mbus_fmt->code != sensor->fmt.code)
> > sensor->pending_fmt_change = true;
> >
> > + __v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixel_rate,
> > + ov5640_calc_pixel_rate(sensor));
> > out:
> > mutex_unlock(&sensor->lock);
> > return ret;
> > @@ -2657,6 +2672,13 @@ static int ov5640_init_controls(struct ov5640_dev *sensor)
> > /* we can use our own mutex for the ctrl lock */
> > hdl->lock = &sensor->lock;
> >
> > + /* Clock related controls */
> > + ctrls->pixel_rate =
> > + v4l2_ctrl_new_std(hdl, ops,
>
> If you like it better, this could fit in 1 line
>
> ctrls->pixel_rate = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_PIXEL_RATE,
> 0, INT_MAX, 1,
> ov5640_calc_pixel_rate(sensor)
>
Either way works for me.
Benoit
> Thanks
> j
>
> > + V4L2_CID_PIXEL_RATE, 0, INT_MAX, 1,
> > + ov5640_calc_pixel_rate(sensor));
>
>
> > + ctrls->pixel_rate->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > +
> > /* Auto/manual white balance */
> > ctrls->auto_wb = v4l2_ctrl_new_std(hdl, ops,
> > V4L2_CID_AUTO_WHITE_BALANCE,
> > @@ -2816,6 +2838,9 @@ static int ov5640_s_frame_interval(struct v4l2_subdev *sd,
> > sensor->frame_interval = fi->interval;
> > sensor->current_mode = mode;
> > sensor->pending_mode_change = true;
> > +
> > + __v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixel_rate,
> > + ov5640_calc_pixel_rate(sensor));
> > }
> > out:
> > mutex_unlock(&sensor->lock);
> > --
> > 2.17.1
> >
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch v2 1/3] media: ov5640: add PIXEL_RATE control
2019-10-03 7:22 ` Sakari Ailus
@ 2019-10-03 12:07 ` Benoit Parrot
2019-10-03 14:32 ` Sakari Ailus
0 siblings, 1 reply; 12+ messages in thread
From: Benoit Parrot @ 2019-10-03 12:07 UTC (permalink / raw)
To: Sakari Ailus
Cc: Jacopo Mondi, Hans Verkuil, linux-media, devicetree, linux-kernel
Sakari Ailus <sakari.ailus@linux.intel.com> wrote on Thu [2019-Oct-03 10:22:51 +0300]:
> Hi Jacopo, Benoit,
>
> On Thu, Oct 03, 2019 at 09:17:14AM +0200, Jacopo Mondi wrote:
> > Hi Benoit,
> >
> > On Wed, Oct 02, 2019 at 08:51:32AM -0500, Benoit Parrot wrote:
> > > Add v4l2 controls to report the pixel rates of each mode. This is
> > > needed by some CSI2 receiver in order to perform proper DPHY
> > > configuration.
> > >
> > > Signed-off-by: Benoit Parrot <bparrot@ti.com>
> > > ---
> > > drivers/media/i2c/ov5640.c | 25 +++++++++++++++++++++++++
> > > 1 file changed, 25 insertions(+)
> > >
> > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > > index 500d9bbff10b..5198dc887400 100644
> > > --- a/drivers/media/i2c/ov5640.c
> > > +++ b/drivers/media/i2c/ov5640.c
> > > @@ -193,6 +193,9 @@ struct ov5640_mode_info {
> > >
> > > struct ov5640_ctrls {
> > > struct v4l2_ctrl_handler handler;
> > > + struct {
> > > + struct v4l2_ctrl *pixel_rate;
> > > + };
> >
> > Do you need to wrap this v4l2_ctrl in it's own unnamed struct? Other
> > controls here declared in this way are clustered and, if I'm not
> > mistaken, using unnamed struct to wrap them is just a typographically
> > nice way to convey that. I think your new control could be declared
> > without a wrapping struct { }.
> >
> > > struct {
> > > struct v4l2_ctrl *auto_exp;
> > > struct v4l2_ctrl *exposure;
> > > @@ -2194,6 +2197,16 @@ static int ov5640_try_fmt_internal(struct v4l2_subdev *sd,
> > > return 0;
> > > }
> > >
> > > +static u64 ov5640_calc_pixel_rate(struct ov5640_dev *sensor)
> > > +{
> > > + u64 rate;
> > > +
> > > + rate = sensor->current_mode->vtot * sensor->current_mode->htot;
> > > + rate *= ov5640_framerates[sensor->current_fr];
> > > +
> > > + return rate;
> > > +}
> > > +
> >
> > Just to point out this is the -theoretical- pixel rate, and might be
> > quite different from the one calculated by the clock tree tuning
> > procedure (which should be updated to match Hugues' latest findings).
>
> Hmm. Considering the xclk rate may be pretty much anything, I'd suppose
> the value above would only be correct for a given xclk rate.
I am not sure about that, different xclk rate might yield slightly
different byte clock, but all in all the resolution and framerate pretty
much dictate the end result, no?
>
> Could this be simply calculated from the clock tree configuration, to get
> the right value in all cases?
It probably could, and as I said earlier I gave it a try and failed, since
the theoretical value worked for me that's what I went with. Those are the
same values that Maxime's patch referred to. (dfbfb7aa832cdb media: ov5640:
Compute the clock rate at runtime).
Here I am just "publishing it".
Benoit
>
> >
> > > static int ov5640_set_fmt(struct v4l2_subdev *sd,
> > > struct v4l2_subdev_pad_config *cfg,
> > > struct v4l2_subdev_format *format)
> > > @@ -2233,6 +2246,8 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd,
> > > if (mbus_fmt->code != sensor->fmt.code)
> > > sensor->pending_fmt_change = true;
> > >
> > > + __v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixel_rate,
> > > + ov5640_calc_pixel_rate(sensor));
> > > out:
> > > mutex_unlock(&sensor->lock);
> > > return ret;
> > > @@ -2657,6 +2672,13 @@ static int ov5640_init_controls(struct ov5640_dev *sensor)
> > > /* we can use our own mutex for the ctrl lock */
> > > hdl->lock = &sensor->lock;
> > >
> > > + /* Clock related controls */
> > > + ctrls->pixel_rate =
> > > + v4l2_ctrl_new_std(hdl, ops,
> >
> > If you like it better, this could fit in 1 line
> >
> > ctrls->pixel_rate = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_PIXEL_RATE,
> > 0, INT_MAX, 1,
> > ov5640_calc_pixel_rate(sensor)
> >
> > Thanks
> > j
> >
> > > + V4L2_CID_PIXEL_RATE, 0, INT_MAX, 1,
> > > + ov5640_calc_pixel_rate(sensor));
> >
> >
> > > + ctrls->pixel_rate->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>
> Note that ctrls->pixel_rate is NULL if e.g. memory allocation fails when
> creating the control.
>
> > > +
> > > /* Auto/manual white balance */
> > > ctrls->auto_wb = v4l2_ctrl_new_std(hdl, ops,
> > > V4L2_CID_AUTO_WHITE_BALANCE,
> > > @@ -2816,6 +2838,9 @@ static int ov5640_s_frame_interval(struct v4l2_subdev *sd,
> > > sensor->frame_interval = fi->interval;
> > > sensor->current_mode = mode;
> > > sensor->pending_mode_change = true;
> > > +
> > > + __v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixel_rate,
> > > + ov5640_calc_pixel_rate(sensor));
> > > }
> > > out:
> > > mutex_unlock(&sensor->lock);
>
> --
> Regards,
>
> Sakari Ailus
> sakari.ailus@linux.intel.com
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch v2 3/3] media: ov5640: Make 2592x1944 mode only available at 15 fps
2019-10-03 7:31 ` Jacopo Mondi
@ 2019-10-03 12:09 ` Benoit Parrot
0 siblings, 0 replies; 12+ messages in thread
From: Benoit Parrot @ 2019-10-03 12:09 UTC (permalink / raw)
To: Jacopo Mondi
Cc: Hans Verkuil, Sakari Ailus, linux-media, devicetree, linux-kernel
Jacopo Mondi <jacopo@jmondi.org> wrote on Thu [2019-Oct-03 09:31:55 +0200]:
> Hi Benoit,
>
> On Wed, Oct 02, 2019 at 08:51:34AM -0500, Benoit Parrot wrote:
> > The sensor data sheet clearly state that 2592x1944 only works at 15 fps
> > make sure we don't try to miss configure the pll out of acceptable
> > range.
>
> The datasheet clearly indicates that 15 fps is the maximum achievable
> rate with that resolution, so I guess preventing it from being set
> to anything faster than that it's acceptable.
> >
> > Signed-off-by: Benoit Parrot <bparrot@ti.com>
> > ---
> > drivers/media/i2c/ov5640.c | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > index 103a4e8f88e1..d5b0be2c7a0a 100644
> > --- a/drivers/media/i2c/ov5640.c
> > +++ b/drivers/media/i2c/ov5640.c
> > @@ -1613,6 +1613,11 @@ ov5640_find_mode(struct ov5640_dev *sensor, enum ov5640_frame_rate fr,
> > !(mode->hact == 640 && mode->vact == 480))
> > return NULL;
> >
> > + /* 2592x1944 only works at 15fps */
> > + if (fr != OV5640_15_FPS &&
>
> As long as 15 fps is the lower framerate declared in
> ov5640_framerates[] this is ok, but I would make this condition a
> check for "fr > OV5640_15_FPS" so that it's safe for future
> extensions.
>
> (And I would check for the resolution first then FPS, so you check
> the most unlikely condition first, but that's really a minor
> optimization).
Ah, very good I'll change that.
Benoit
>
> With the above small details addressed
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>
> Thanks
> j
>
> > + (mode->hact == 2592 && mode->vact == 1944))
> > + return NULL;
> > +
> > return mode;
> > }
> >
> > --
> > 2.17.1
> >
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch v2 1/3] media: ov5640: add PIXEL_RATE control
2019-10-03 12:07 ` Benoit Parrot
@ 2019-10-03 14:32 ` Sakari Ailus
0 siblings, 0 replies; 12+ messages in thread
From: Sakari Ailus @ 2019-10-03 14:32 UTC (permalink / raw)
To: Benoit Parrot
Cc: Jacopo Mondi, Hans Verkuil, linux-media, devicetree, linux-kernel
Hi Benoit,
On Thu, Oct 03, 2019 at 07:07:41AM -0500, Benoit Parrot wrote:
> Sakari Ailus <sakari.ailus@linux.intel.com> wrote on Thu [2019-Oct-03 10:22:51 +0300]:
> > Hi Jacopo, Benoit,
> >
> > On Thu, Oct 03, 2019 at 09:17:14AM +0200, Jacopo Mondi wrote:
> > > Hi Benoit,
> > >
> > > On Wed, Oct 02, 2019 at 08:51:32AM -0500, Benoit Parrot wrote:
> > > > Add v4l2 controls to report the pixel rates of each mode. This is
> > > > needed by some CSI2 receiver in order to perform proper DPHY
> > > > configuration.
> > > >
> > > > Signed-off-by: Benoit Parrot <bparrot@ti.com>
> > > > ---
> > > > drivers/media/i2c/ov5640.c | 25 +++++++++++++++++++++++++
> > > > 1 file changed, 25 insertions(+)
> > > >
> > > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > > > index 500d9bbff10b..5198dc887400 100644
> > > > --- a/drivers/media/i2c/ov5640.c
> > > > +++ b/drivers/media/i2c/ov5640.c
> > > > @@ -193,6 +193,9 @@ struct ov5640_mode_info {
> > > >
> > > > struct ov5640_ctrls {
> > > > struct v4l2_ctrl_handler handler;
> > > > + struct {
> > > > + struct v4l2_ctrl *pixel_rate;
> > > > + };
> > >
> > > Do you need to wrap this v4l2_ctrl in it's own unnamed struct? Other
> > > controls here declared in this way are clustered and, if I'm not
> > > mistaken, using unnamed struct to wrap them is just a typographically
> > > nice way to convey that. I think your new control could be declared
> > > without a wrapping struct { }.
> > >
> > > > struct {
> > > > struct v4l2_ctrl *auto_exp;
> > > > struct v4l2_ctrl *exposure;
> > > > @@ -2194,6 +2197,16 @@ static int ov5640_try_fmt_internal(struct v4l2_subdev *sd,
> > > > return 0;
> > > > }
> > > >
> > > > +static u64 ov5640_calc_pixel_rate(struct ov5640_dev *sensor)
> > > > +{
> > > > + u64 rate;
> > > > +
> > > > + rate = sensor->current_mode->vtot * sensor->current_mode->htot;
> > > > + rate *= ov5640_framerates[sensor->current_fr];
> > > > +
> > > > + return rate;
> > > > +}
> > > > +
> > >
> > > Just to point out this is the -theoretical- pixel rate, and might be
> > > quite different from the one calculated by the clock tree tuning
> > > procedure (which should be updated to match Hugues' latest findings).
> >
> > Hmm. Considering the xclk rate may be pretty much anything, I'd suppose
> > the value above would only be correct for a given xclk rate.
>
> I am not sure about that, different xclk rate might yield slightly
> different byte clock, but all in all the resolution and framerate pretty
> much dictate the end result, no?
Interestingly, the driver determines the PLL configuration based on the
pixels per line and lines per frame (including blanking) and the frames per
seconds. I guess it's always been like that in this driver.
So I agree the target frame rate can be used for this.
You could change ov5640_set_mode() to use this function as well to avoid
doing the same calculation twice in different places in the driver. Up to
you.
>
> >
> > Could this be simply calculated from the clock tree configuration, to get
> > the right value in all cases?
>
> It probably could, and as I said earlier I gave it a try and failed, since
> the theoretical value worked for me that's what I went with. Those are the
> same values that Maxime's patch referred to. (dfbfb7aa832cdb media: ov5640:
> Compute the clock rate at runtime).
>
> Here I am just "publishing it".
>
> Benoit
>
> >
> > >
> > > > static int ov5640_set_fmt(struct v4l2_subdev *sd,
> > > > struct v4l2_subdev_pad_config *cfg,
> > > > struct v4l2_subdev_format *format)
> > > > @@ -2233,6 +2246,8 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd,
> > > > if (mbus_fmt->code != sensor->fmt.code)
> > > > sensor->pending_fmt_change = true;
> > > >
> > > > + __v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixel_rate,
> > > > + ov5640_calc_pixel_rate(sensor));
> > > > out:
> > > > mutex_unlock(&sensor->lock);
> > > > return ret;
> > > > @@ -2657,6 +2672,13 @@ static int ov5640_init_controls(struct ov5640_dev *sensor)
> > > > /* we can use our own mutex for the ctrl lock */
> > > > hdl->lock = &sensor->lock;
> > > >
> > > > + /* Clock related controls */
> > > > + ctrls->pixel_rate =
> > > > + v4l2_ctrl_new_std(hdl, ops,
> > >
> > > If you like it better, this could fit in 1 line
> > >
> > > ctrls->pixel_rate = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_PIXEL_RATE,
> > > 0, INT_MAX, 1,
> > > ov5640_calc_pixel_rate(sensor)
> > >
> > > Thanks
> > > j
> > >
> > > > + V4L2_CID_PIXEL_RATE, 0, INT_MAX, 1,
> > > > + ov5640_calc_pixel_rate(sensor));
> > >
> > >
> > > > + ctrls->pixel_rate->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> >
> > Note that ctrls->pixel_rate is NULL if e.g. memory allocation fails when
> > creating the control.
> >
> > > > +
> > > > /* Auto/manual white balance */
> > > > ctrls->auto_wb = v4l2_ctrl_new_std(hdl, ops,
> > > > V4L2_CID_AUTO_WHITE_BALANCE,
> > > > @@ -2816,6 +2838,9 @@ static int ov5640_s_frame_interval(struct v4l2_subdev *sd,
> > > > sensor->frame_interval = fi->interval;
> > > > sensor->current_mode = mode;
> > > > sensor->pending_mode_change = true;
> > > > +
> > > > + __v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixel_rate,
> > > > + ov5640_calc_pixel_rate(sensor));
> > > > }
> > > > out:
> > > > mutex_unlock(&sensor->lock);
> >
> > --
> > Regards,
> >
> > Sakari Ailus
> > sakari.ailus@linux.intel.com
--
Sakari Ailus
sakari.ailus@linux.intel.com
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2019-10-03 14:32 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-02 13:51 [Patch v2 0/3] media: ov5640: updates Benoit Parrot
2019-10-02 13:51 ` [Patch v2 1/3] media: ov5640: add PIXEL_RATE control Benoit Parrot
2019-10-03 7:17 ` Jacopo Mondi
2019-10-03 7:22 ` Sakari Ailus
2019-10-03 12:07 ` Benoit Parrot
2019-10-03 14:32 ` Sakari Ailus
2019-10-03 11:59 ` Benoit Parrot
2019-10-02 13:51 ` [Patch v2 2/3] media: ov5640: Fix 1920x1080 mode to remove extra enable/disable Benoit Parrot
2019-10-03 7:26 ` Jacopo Mondi
2019-10-02 13:51 ` [Patch v2 3/3] media: ov5640: Make 2592x1944 mode only available at 15 fps Benoit Parrot
2019-10-03 7:31 ` Jacopo Mondi
2019-10-03 12:09 ` Benoit Parrot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).