* [Patch 0/3] media: ov5640: updates
@ 2019-09-25 15:22 Benoit Parrot
2019-09-25 15:22 ` [Patch 1/3] media: ov5640: add PIXEL_RATE control Benoit Parrot
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Benoit Parrot @ 2019-09-25 15:22 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.
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] 13+ messages in thread
* [Patch 1/3] media: ov5640: add PIXEL_RATE control
2019-09-25 15:22 [Patch 0/3] media: ov5640: updates Benoit Parrot
@ 2019-09-25 15:22 ` Benoit Parrot
2019-10-01 7:57 ` Sakari Ailus
2019-09-25 15:23 ` [Patch 2/3] media: ov5640: Fix 1920x1080 mode to remove extra enable/disable Benoit Parrot
2019-09-25 15:23 ` [Patch 3/3] media: ov5640: Make 2592x1944 mode only available at 15 fps Benoit Parrot
2 siblings, 1 reply; 13+ messages in thread
From: Benoit Parrot @ 2019-09-25 15:22 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..c2a44f30d56e 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;
@@ -241,6 +244,7 @@ struct ov5640_dev {
const struct ov5640_mode_info *last_mode;
enum ov5640_frame_rate current_fr;
struct v4l2_fract frame_interval;
+ u64 pixel_rate;
struct ov5640_ctrls ctrls;
@@ -2202,6 +2206,7 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd,
const struct ov5640_mode_info *new_mode;
struct v4l2_mbus_framefmt *mbus_fmt = &format->format;
struct v4l2_mbus_framefmt *fmt;
+ u64 rate;
int ret;
if (format->pad != 0)
@@ -2233,6 +2238,12 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd,
if (mbus_fmt->code != sensor->fmt.code)
sensor->pending_fmt_change = true;
+ rate = sensor->current_mode->vtot * sensor->current_mode->htot;
+ rate *= ov5640_framerates[sensor->current_fr];
+ sensor->pixel_rate = rate;
+
+ __v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixel_rate,
+ sensor->pixel_rate);
out:
mutex_unlock(&sensor->lock);
return ret;
@@ -2657,6 +2668,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,
+ 55969920);
+ 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,
@@ -2782,6 +2800,7 @@ static int ov5640_s_frame_interval(struct v4l2_subdev *sd,
struct ov5640_dev *sensor = to_ov5640_dev(sd);
const struct ov5640_mode_info *mode;
int frame_rate, ret = 0;
+ u64 rate;
if (fi->pad != 0)
return -EINVAL;
@@ -2816,6 +2835,12 @@ static int ov5640_s_frame_interval(struct v4l2_subdev *sd,
sensor->frame_interval = fi->interval;
sensor->current_mode = mode;
sensor->pending_mode_change = true;
+
+ rate = sensor->current_mode->vtot * sensor->current_mode->htot;
+ rate *= ov5640_framerates[sensor->current_fr];
+ sensor->pixel_rate = rate;
+ __v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixel_rate,
+ sensor->pixel_rate);
}
out:
mutex_unlock(&sensor->lock);
--
2.17.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [Patch 2/3] media: ov5640: Fix 1920x1080 mode to remove extra enable/disable
2019-09-25 15:22 [Patch 0/3] media: ov5640: updates Benoit Parrot
2019-09-25 15:22 ` [Patch 1/3] media: ov5640: add PIXEL_RATE control Benoit Parrot
@ 2019-09-25 15:23 ` Benoit Parrot
2019-09-25 15:23 ` [Patch 3/3] media: ov5640: Make 2592x1944 mode only available at 15 fps Benoit Parrot
2 siblings, 0 replies; 13+ messages in thread
From: Benoit Parrot @ 2019-09-25 15:23 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 c2a44f30d56e..aa56a5a93283 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -493,7 +493,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},
@@ -521,7 +520,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] 13+ messages in thread
* [Patch 3/3] media: ov5640: Make 2592x1944 mode only available at 15 fps
2019-09-25 15:22 [Patch 0/3] media: ov5640: updates Benoit Parrot
2019-09-25 15:22 ` [Patch 1/3] media: ov5640: add PIXEL_RATE control Benoit Parrot
2019-09-25 15:23 ` [Patch 2/3] media: ov5640: Fix 1920x1080 mode to remove extra enable/disable Benoit Parrot
@ 2019-09-25 15:23 ` Benoit Parrot
2 siblings, 0 replies; 13+ messages in thread
From: Benoit Parrot @ 2019-09-25 15:23 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 aa56a5a93283..3c9e61f948a5 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -1614,6 +1614,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] 13+ messages in thread
* Re: [Patch 1/3] media: ov5640: add PIXEL_RATE control
2019-09-25 15:22 ` [Patch 1/3] media: ov5640: add PIXEL_RATE control Benoit Parrot
@ 2019-10-01 7:57 ` Sakari Ailus
2019-10-01 16:23 ` Benoit Parrot
0 siblings, 1 reply; 13+ messages in thread
From: Sakari Ailus @ 2019-10-01 7:57 UTC (permalink / raw)
To: Benoit Parrot; +Cc: Hans Verkuil, linux-media, devicetree, linux-kernel
Hi Benoit,
On Wed, Sep 25, 2019 at 10:22:59AM -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..c2a44f30d56e 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;
> @@ -241,6 +244,7 @@ struct ov5640_dev {
> const struct ov5640_mode_info *last_mode;
> enum ov5640_frame_rate current_fr;
> struct v4l2_fract frame_interval;
> + u64 pixel_rate;
>
> struct ov5640_ctrls ctrls;
>
> @@ -2202,6 +2206,7 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd,
> const struct ov5640_mode_info *new_mode;
> struct v4l2_mbus_framefmt *mbus_fmt = &format->format;
> struct v4l2_mbus_framefmt *fmt;
> + u64 rate;
> int ret;
>
> if (format->pad != 0)
> @@ -2233,6 +2238,12 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd,
> if (mbus_fmt->code != sensor->fmt.code)
> sensor->pending_fmt_change = true;
>
> + rate = sensor->current_mode->vtot * sensor->current_mode->htot;
> + rate *= ov5640_framerates[sensor->current_fr];
> + sensor->pixel_rate = rate;
> +
> + __v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixel_rate,
> + sensor->pixel_rate);
> out:
> mutex_unlock(&sensor->lock);
> return ret;
> @@ -2657,6 +2668,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,
> + 55969920);
Could you calculate this value instead of using a seemingly random number?
> + 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,
> @@ -2782,6 +2800,7 @@ static int ov5640_s_frame_interval(struct v4l2_subdev *sd,
> struct ov5640_dev *sensor = to_ov5640_dev(sd);
> const struct ov5640_mode_info *mode;
> int frame_rate, ret = 0;
> + u64 rate;
>
> if (fi->pad != 0)
> return -EINVAL;
> @@ -2816,6 +2835,12 @@ static int ov5640_s_frame_interval(struct v4l2_subdev *sd,
> sensor->frame_interval = fi->interval;
> sensor->current_mode = mode;
> sensor->pending_mode_change = true;
> +
> + rate = sensor->current_mode->vtot * sensor->current_mode->htot;
> + rate *= ov5640_framerates[sensor->current_fr];
> + sensor->pixel_rate = rate;
I think it'd be better to have a function to calculate the value instead of
duplicating the code here.
> + __v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixel_rate,
> + sensor->pixel_rate);
> }
> out:
> mutex_unlock(&sensor->lock);
--
Kind regards,
Sakari Ailus
sakari.ailus@linux.intel.com
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Patch 1/3] media: ov5640: add PIXEL_RATE control
2019-10-01 7:57 ` Sakari Ailus
@ 2019-10-01 16:23 ` Benoit Parrot
2019-10-02 7:59 ` Jacopo Mondi
0 siblings, 1 reply; 13+ messages in thread
From: Benoit Parrot @ 2019-10-01 16:23 UTC (permalink / raw)
To: Sakari Ailus; +Cc: Hans Verkuil, linux-media, devicetree, linux-kernel
Hi Sakari,
Thanks for the review.
Sakari Ailus <sakari.ailus@linux.intel.com> wrote on Tue [2019-Oct-01 10:57:04 +0300]:
> Hi Benoit,
>
> On Wed, Sep 25, 2019 at 10:22:59AM -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..c2a44f30d56e 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;
> > @@ -241,6 +244,7 @@ struct ov5640_dev {
> > const struct ov5640_mode_info *last_mode;
> > enum ov5640_frame_rate current_fr;
> > struct v4l2_fract frame_interval;
> > + u64 pixel_rate;
> >
> > struct ov5640_ctrls ctrls;
> >
> > @@ -2202,6 +2206,7 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd,
> > const struct ov5640_mode_info *new_mode;
> > struct v4l2_mbus_framefmt *mbus_fmt = &format->format;
> > struct v4l2_mbus_framefmt *fmt;
> > + u64 rate;
> > int ret;
> >
> > if (format->pad != 0)
> > @@ -2233,6 +2238,12 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd,
> > if (mbus_fmt->code != sensor->fmt.code)
> > sensor->pending_fmt_change = true;
> >
> > + rate = sensor->current_mode->vtot * sensor->current_mode->htot;
> > + rate *= ov5640_framerates[sensor->current_fr];
> > + sensor->pixel_rate = rate;
> > +
> > + __v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixel_rate,
> > + sensor->pixel_rate);
> > out:
> > mutex_unlock(&sensor->lock);
> > return ret;
> > @@ -2657,6 +2668,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,
> > + 55969920);
>
> Could you calculate this value instead of using a seemingly random number?
Yes I guess I could, especially if I make a helper function for it :).
>
> > + 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,
> > @@ -2782,6 +2800,7 @@ static int ov5640_s_frame_interval(struct v4l2_subdev *sd,
> > struct ov5640_dev *sensor = to_ov5640_dev(sd);
> > const struct ov5640_mode_info *mode;
> > int frame_rate, ret = 0;
> > + u64 rate;
> >
> > if (fi->pad != 0)
> > return -EINVAL;
> > @@ -2816,6 +2835,12 @@ static int ov5640_s_frame_interval(struct v4l2_subdev *sd,
> > sensor->frame_interval = fi->interval;
> > sensor->current_mode = mode;
> > sensor->pending_mode_change = true;
> > +
> > + rate = sensor->current_mode->vtot * sensor->current_mode->htot;
> > + rate *= ov5640_framerates[sensor->current_fr];
> > + sensor->pixel_rate = rate;
>
> I think it'd be better to have a function to calculate the value instead of
> duplicating the code here.
Yes I'll create a helper function.
Benoit
>
> > + __v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixel_rate,
> > + sensor->pixel_rate);
> > }
> > out:
> > mutex_unlock(&sensor->lock);
>
> --
> Kind regards,
>
> Sakari Ailus
> sakari.ailus@linux.intel.com
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Patch 1/3] media: ov5640: add PIXEL_RATE control
2019-10-01 16:23 ` Benoit Parrot
@ 2019-10-02 7:59 ` Jacopo Mondi
2019-10-02 12:14 ` Benoit Parrot
0 siblings, 1 reply; 13+ messages in thread
From: Jacopo Mondi @ 2019-10-02 7:59 UTC (permalink / raw)
To: Benoit Parrot
Cc: Sakari Ailus, Hans Verkuil, linux-media, devicetree,
linux-kernel, hugues.fruchet
[-- Attachment #1: Type: text/plain, Size: 4142 bytes --]
Hi Benoit,
+Hugues
If you're considering an helper, this thread might be useful to you:
https://patchwork.kernel.org/patch/11019673/
Thanks
j
On Tue, Oct 01, 2019 at 11:23:41AM -0500, Benoit Parrot wrote:
> Hi Sakari,
>
> Thanks for the review.
>
> Sakari Ailus <sakari.ailus@linux.intel.com> wrote on Tue [2019-Oct-01 10:57:04 +0300]:
> > Hi Benoit,
> >
> > On Wed, Sep 25, 2019 at 10:22:59AM -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..c2a44f30d56e 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;
> > > @@ -241,6 +244,7 @@ struct ov5640_dev {
> > > const struct ov5640_mode_info *last_mode;
> > > enum ov5640_frame_rate current_fr;
> > > struct v4l2_fract frame_interval;
> > > + u64 pixel_rate;
> > >
> > > struct ov5640_ctrls ctrls;
> > >
> > > @@ -2202,6 +2206,7 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd,
> > > const struct ov5640_mode_info *new_mode;
> > > struct v4l2_mbus_framefmt *mbus_fmt = &format->format;
> > > struct v4l2_mbus_framefmt *fmt;
> > > + u64 rate;
> > > int ret;
> > >
> > > if (format->pad != 0)
> > > @@ -2233,6 +2238,12 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd,
> > > if (mbus_fmt->code != sensor->fmt.code)
> > > sensor->pending_fmt_change = true;
> > >
> > > + rate = sensor->current_mode->vtot * sensor->current_mode->htot;
> > > + rate *= ov5640_framerates[sensor->current_fr];
> > > + sensor->pixel_rate = rate;
> > > +
> > > + __v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixel_rate,
> > > + sensor->pixel_rate);
> > > out:
> > > mutex_unlock(&sensor->lock);
> > > return ret;
> > > @@ -2657,6 +2668,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,
> > > + 55969920);
> >
> > Could you calculate this value instead of using a seemingly random number?
>
> Yes I guess I could, especially if I make a helper function for it :).
>
> >
> > > + 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,
> > > @@ -2782,6 +2800,7 @@ static int ov5640_s_frame_interval(struct v4l2_subdev *sd,
> > > struct ov5640_dev *sensor = to_ov5640_dev(sd);
> > > const struct ov5640_mode_info *mode;
> > > int frame_rate, ret = 0;
> > > + u64 rate;
> > >
> > > if (fi->pad != 0)
> > > return -EINVAL;
> > > @@ -2816,6 +2835,12 @@ static int ov5640_s_frame_interval(struct v4l2_subdev *sd,
> > > sensor->frame_interval = fi->interval;
> > > sensor->current_mode = mode;
> > > sensor->pending_mode_change = true;
> > > +
> > > + rate = sensor->current_mode->vtot * sensor->current_mode->htot;
> > > + rate *= ov5640_framerates[sensor->current_fr];
> > > + sensor->pixel_rate = rate;
> >
> > I think it'd be better to have a function to calculate the value instead of
> > duplicating the code here.
>
> Yes I'll create a helper function.
>
> Benoit
>
> >
> > > + __v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixel_rate,
> > > + sensor->pixel_rate);
> > > }
> > > out:
> > > mutex_unlock(&sensor->lock);
> >
> > --
> > Kind regards,
> >
> > Sakari Ailus
> > sakari.ailus@linux.intel.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Patch 1/3] media: ov5640: add PIXEL_RATE control
2019-10-02 7:59 ` Jacopo Mondi
@ 2019-10-02 12:14 ` Benoit Parrot
2019-10-02 12:35 ` Sakari Ailus
2019-10-02 14:32 ` Jacopo Mondi
0 siblings, 2 replies; 13+ messages in thread
From: Benoit Parrot @ 2019-10-02 12:14 UTC (permalink / raw)
To: Jacopo Mondi
Cc: Sakari Ailus, Hans Verkuil, linux-media, devicetree,
linux-kernel, hugues.fruchet
Hi Jacopo,
Maybe, I miss spoke when I mentioned a helper I did not intent a framework
level generic function. Just a function to help in this case :)
That being said, I re-read the thread you mentioned. And as Hughes pointed
out dynamically generating a "working" link frequency value which can be
used by a CSI2 receiver to properly configure its PHY is not trivial.
When I created this patch, I also had another to add V4L2_CID_LINK_FREQ
support. I am testing this against the TI CAL CSI2 receiver, which already
uses the V4L2_CID_PIXEL_RATE value for that purpose, so I also had a patch
to add support for V4L2_CID_LINK_FREQ to that driver as well.
Unfortunately, similar to Hughes' findings I was not able to make it "work"
with all supported resolution/framerate.
Unlike my V4L2_CID_PIXEL_RATE solution which now works in all mode with the
same receiver.
So long story short I dropped the V4L2_CID_LINK_FREQ patch and focused on
V4L2_CID_PIXEL_RATE instead.
Regard,
Benoit
Jacopo Mondi <jacopo@jmondi.org> wrote on Wed [2019-Oct-02 09:59:51 +0200]:
> Hi Benoit,
> +Hugues
>
> If you're considering an helper, this thread might be useful to you:
> https://patchwork.kernel.org/patch/11019673/
>
> Thanks
> j
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Patch 1/3] media: ov5640: add PIXEL_RATE control
2019-10-02 12:14 ` Benoit Parrot
@ 2019-10-02 12:35 ` Sakari Ailus
2019-10-02 12:40 ` Benoit Parrot
2019-10-02 14:32 ` Jacopo Mondi
1 sibling, 1 reply; 13+ messages in thread
From: Sakari Ailus @ 2019-10-02 12:35 UTC (permalink / raw)
To: Benoit Parrot
Cc: Jacopo Mondi, Hans Verkuil, linux-media, devicetree,
linux-kernel, hugues.fruchet
Hi Benoit,
On Wed, Oct 02, 2019 at 07:14:38AM -0500, Benoit Parrot wrote:
> Hi Jacopo,
>
> Maybe, I miss spoke when I mentioned a helper I did not intent a framework
> level generic function. Just a function to help in this case :)
>
> That being said, I re-read the thread you mentioned. And as Hughes pointed
> out dynamically generating a "working" link frequency value which can be
> used by a CSI2 receiver to properly configure its PHY is not trivial.
>
> When I created this patch, I also had another to add V4L2_CID_LINK_FREQ
> support. I am testing this against the TI CAL CSI2 receiver, which already
> uses the V4L2_CID_PIXEL_RATE value for that purpose, so I also had a patch
> to add support for V4L2_CID_LINK_FREQ to that driver as well.
>
> Unfortunately, similar to Hughes' findings I was not able to make it "work"
> with all supported resolution/framerate.
>
> Unlike my V4L2_CID_PIXEL_RATE solution which now works in all mode with the
> same receiver.
>
> So long story short I dropped the V4L2_CID_LINK_FREQ patch and focused on
> V4L2_CID_PIXEL_RATE instead.
It shouldn't make a difference which one you use; if you know the bus type
(and if it's CSI-2 with D-PHY, number of lanes and how many bits per pixel
the media bus format has), you can convert fairly trivially between the
two.
--
Sakari Ailus
sakari.ailus@linux.intel.com
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Patch 1/3] media: ov5640: add PIXEL_RATE control
2019-10-02 12:35 ` Sakari Ailus
@ 2019-10-02 12:40 ` Benoit Parrot
0 siblings, 0 replies; 13+ messages in thread
From: Benoit Parrot @ 2019-10-02 12:40 UTC (permalink / raw)
To: Sakari Ailus
Cc: Jacopo Mondi, Hans Verkuil, linux-media, devicetree,
linux-kernel, hugues.fruchet
Sakari Ailus <sakari.ailus@linux.intel.com> wrote on Wed [2019-Oct-02 15:35:13 +0300]:
> Hi Benoit,
>
> On Wed, Oct 02, 2019 at 07:14:38AM -0500, Benoit Parrot wrote:
> > Hi Jacopo,
> >
> > Maybe, I miss spoke when I mentioned a helper I did not intent a framework
> > level generic function. Just a function to help in this case :)
> >
> > That being said, I re-read the thread you mentioned. And as Hughes pointed
> > out dynamically generating a "working" link frequency value which can be
> > used by a CSI2 receiver to properly configure its PHY is not trivial.
> >
> > When I created this patch, I also had another to add V4L2_CID_LINK_FREQ
> > support. I am testing this against the TI CAL CSI2 receiver, which already
> > uses the V4L2_CID_PIXEL_RATE value for that purpose, so I also had a patch
> > to add support for V4L2_CID_LINK_FREQ to that driver as well.
> >
> > Unfortunately, similar to Hughes' findings I was not able to make it "work"
> > with all supported resolution/framerate.
> >
> > Unlike my V4L2_CID_PIXEL_RATE solution which now works in all mode with the
> > same receiver.
> >
> > So long story short I dropped the V4L2_CID_LINK_FREQ patch and focused on
> > V4L2_CID_PIXEL_RATE instead.
>
> It shouldn't make a difference which one you use; if you know the bus type
> (and if it's CSI-2 with D-PHY, number of lanes and how many bits per pixel
> the media bus format has), you can convert fairly trivially between the
> two.
Sakari,
Yes I get that, but at this point as they say I will leave that as an
exercise to the reader...
Benoit
>
> --
> Sakari Ailus
> sakari.ailus@linux.intel.com
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Patch 1/3] media: ov5640: add PIXEL_RATE control
2019-10-02 12:14 ` Benoit Parrot
2019-10-02 12:35 ` Sakari Ailus
@ 2019-10-02 14:32 ` Jacopo Mondi
2019-10-02 15:06 ` Benoit Parrot
1 sibling, 1 reply; 13+ messages in thread
From: Jacopo Mondi @ 2019-10-02 14:32 UTC (permalink / raw)
To: Benoit Parrot
Cc: Sakari Ailus, Hans Verkuil, linux-media, devicetree,
linux-kernel, hugues.fruchet
[-- Attachment #1: Type: text/plain, Size: 2073 bytes --]
Hi Benoit,
On Wed, Oct 02, 2019 at 07:14:38AM -0500, Benoit Parrot wrote:
> Hi Jacopo,
>
> Maybe, I miss spoke when I mentioned a helper I did not intent a framework
> level generic function. Just a function to help in this case :)
Yes indeed, the discussion thread I linked here was mostly interesting
because Hugues tried to do the same for LINK_FREQ iirc, and there
where some usefult pointers.
>
> That being said, I re-read the thread you mentioned. And as Hughes pointed
> out dynamically generating a "working" link frequency value which can be
> used by a CSI2 receiver to properly configure its PHY is not trivial.
>
> When I created this patch, I also had another to add V4L2_CID_LINK_FREQ
> support. I am testing this against the TI CAL CSI2 receiver, which already
> uses the V4L2_CID_PIXEL_RATE value for that purpose, so I also had a patch
> to add support for V4L2_CID_LINK_FREQ to that driver as well.
>
> Unfortunately, similar to Hughes' findings I was not able to make it "work"
> with all supported resolution/framerate.
As reported by Hugues findings, the PLL calculation procedure might be
faulty, and the actuall frequencies on the bus are different from the
calculated ones.
I wish I had more time to re-look at that, as they worked for my and
Sam's use case, but deserve some rework.
>
> Unlike my V4L2_CID_PIXEL_RATE solution which now works in all mode with the
> same receiver.
>
It seems to me you're reporting a fixed rate. It might make your
receiver happy, but does not report what is acutally put on the bus.
Am I missing something ?
> So long story short I dropped the V4L2_CID_LINK_FREQ patch and focused on
> V4L2_CID_PIXEL_RATE instead.
>
As Sakari pointed out, going from one to the other is trivial and
could be done on top.
Thanks
j
> Regard,
> Benoit
>
> Jacopo Mondi <jacopo@jmondi.org> wrote on Wed [2019-Oct-02 09:59:51 +0200]:
> > Hi Benoit,
> > +Hugues
> >
> > If you're considering an helper, this thread might be useful to you:
> > https://patchwork.kernel.org/patch/11019673/
> >
> > Thanks
> > j
> >
>
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Patch 1/3] media: ov5640: add PIXEL_RATE control
2019-10-02 14:32 ` Jacopo Mondi
@ 2019-10-02 15:06 ` Benoit Parrot
2019-10-03 6:55 ` Jacopo Mondi
0 siblings, 1 reply; 13+ messages in thread
From: Benoit Parrot @ 2019-10-02 15:06 UTC (permalink / raw)
To: Jacopo Mondi
Cc: Sakari Ailus, Hans Verkuil, linux-media, devicetree,
linux-kernel, hugues.fruchet
Jacopo Mondi <jacopo@jmondi.org> wrote on Wed [2019-Oct-02 16:32:26 +0200]:
> Hi Benoit,
>
> On Wed, Oct 02, 2019 at 07:14:38AM -0500, Benoit Parrot wrote:
> > Hi Jacopo,
> >
> > Maybe, I miss spoke when I mentioned a helper I did not intent a framework
> > level generic function. Just a function to help in this case :)
>
> Yes indeed, the discussion thread I linked here was mostly interesting
> because Hugues tried to do the same for LINK_FREQ iirc, and there
> where some usefult pointers.
>
> >
> > That being said, I re-read the thread you mentioned. And as Hughes pointed
> > out dynamically generating a "working" link frequency value which can be
> > used by a CSI2 receiver to properly configure its PHY is not trivial.
> >
> > When I created this patch, I also had another to add V4L2_CID_LINK_FREQ
> > support. I am testing this against the TI CAL CSI2 receiver, which already
> > uses the V4L2_CID_PIXEL_RATE value for that purpose, so I also had a patch
> > to add support for V4L2_CID_LINK_FREQ to that driver as well.
> >
> > Unfortunately, similar to Hughes' findings I was not able to make it "work"
> > with all supported resolution/framerate.
>
> As reported by Hugues findings, the PLL calculation procedure might be
> faulty, and the actuall frequencies on the bus are different from the
> calculated ones.
>
> I wish I had more time to re-look at that, as they worked for my and
> Sam's use case, but deserve some rework.
>
> >
> > Unlike my V4L2_CID_PIXEL_RATE solution which now works in all mode with the
> > same receiver.
> >
>
> It seems to me you're reporting a fixed rate. It might make your
> receiver happy, but does not report what is acutally put on the bus.
> Am I missing something ?
No it is not fixed, the only fixed value was the initial value (which
representative of the initial/default resolution and framerate), I
fixed this in v2. The reported PIXEL_RATE is re-calculated every time there
is a s_fmt and/or framerate change and the V4L2_CID_PIXEL_RATE control
value is updated accordingly.
>
> > So long story short I dropped the V4L2_CID_LINK_FREQ patch and focused on
> > V4L2_CID_PIXEL_RATE instead.
> >
>
> As Sakari pointed out, going from one to the other is trivial and
> could be done on top.
As you said it could be done on top. :)
Benoit
>
> Thanks
> j
>
> > Regard,
> > Benoit
> >
> > Jacopo Mondi <jacopo@jmondi.org> wrote on Wed [2019-Oct-02 09:59:51 +0200]:
> > > Hi Benoit,
> > > +Hugues
> > >
> > > If you're considering an helper, this thread might be useful to you:
> > > https://patchwork.kernel.org/patch/11019673/
> > >
> > > Thanks
> > > j
> > >
> >
> >
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Patch 1/3] media: ov5640: add PIXEL_RATE control
2019-10-02 15:06 ` Benoit Parrot
@ 2019-10-03 6:55 ` Jacopo Mondi
0 siblings, 0 replies; 13+ messages in thread
From: Jacopo Mondi @ 2019-10-03 6:55 UTC (permalink / raw)
To: Benoit Parrot
Cc: Sakari Ailus, Hans Verkuil, linux-media, devicetree,
linux-kernel, hugues.fruchet
[-- Attachment #1: Type: text/plain, Size: 2938 bytes --]
Hi Benoit,
On Wed, Oct 02, 2019 at 10:06:15AM -0500, Benoit Parrot wrote:
> Jacopo Mondi <jacopo@jmondi.org> wrote on Wed [2019-Oct-02 16:32:26 +0200]:
> > Hi Benoit,
> >
> > On Wed, Oct 02, 2019 at 07:14:38AM -0500, Benoit Parrot wrote:
> > > Hi Jacopo,
> > >
> > > Maybe, I miss spoke when I mentioned a helper I did not intent a framework
> > > level generic function. Just a function to help in this case :)
> >
> > Yes indeed, the discussion thread I linked here was mostly interesting
> > because Hugues tried to do the same for LINK_FREQ iirc, and there
> > where some usefult pointers.
> >
> > >
> > > That being said, I re-read the thread you mentioned. And as Hughes pointed
> > > out dynamically generating a "working" link frequency value which can be
> > > used by a CSI2 receiver to properly configure its PHY is not trivial.
> > >
> > > When I created this patch, I also had another to add V4L2_CID_LINK_FREQ
> > > support. I am testing this against the TI CAL CSI2 receiver, which already
> > > uses the V4L2_CID_PIXEL_RATE value for that purpose, so I also had a patch
> > > to add support for V4L2_CID_LINK_FREQ to that driver as well.
> > >
> > > Unfortunately, similar to Hughes' findings I was not able to make it "work"
> > > with all supported resolution/framerate.
> >
> > As reported by Hugues findings, the PLL calculation procedure might be
> > faulty, and the actuall frequencies on the bus are different from the
> > calculated ones.
> >
> > I wish I had more time to re-look at that, as they worked for my and
> > Sam's use case, but deserve some rework.
> >
> > >
> > > Unlike my V4L2_CID_PIXEL_RATE solution which now works in all mode with the
> > > same receiver.
> > >
> >
> > It seems to me you're reporting a fixed rate. It might make your
> > receiver happy, but does not report what is acutally put on the bus.
> > Am I missing something ?
>
> No it is not fixed, the only fixed value was the initial value (which
> representative of the initial/default resolution and framerate), I
> fixed this in v2. The reported PIXEL_RATE is re-calculated every time there
> is a s_fmt and/or framerate change and the V4L2_CID_PIXEL_RATE control
> value is updated accordingly.
Oh I missed v2! I only seen this one.
I'll reply there.
Thanks
j
>
> >
> > > So long story short I dropped the V4L2_CID_LINK_FREQ patch and focused on
> > > V4L2_CID_PIXEL_RATE instead.
> > >
> >
> > As Sakari pointed out, going from one to the other is trivial and
> > could be done on top.
>
> As you said it could be done on top. :)
>
> Benoit
>
> >
> > Thanks
> > j
> >
> > > Regard,
> > > Benoit
> > >
> > > Jacopo Mondi <jacopo@jmondi.org> wrote on Wed [2019-Oct-02 09:59:51 +0200]:
> > > > Hi Benoit,
> > > > +Hugues
> > > >
> > > > If you're considering an helper, this thread might be useful to you:
> > > > https://patchwork.kernel.org/patch/11019673/
> > > >
> > > > Thanks
> > > > j
> > > >
> > >
> > >
>
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2019-10-03 6:53 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-25 15:22 [Patch 0/3] media: ov5640: updates Benoit Parrot
2019-09-25 15:22 ` [Patch 1/3] media: ov5640: add PIXEL_RATE control Benoit Parrot
2019-10-01 7:57 ` Sakari Ailus
2019-10-01 16:23 ` Benoit Parrot
2019-10-02 7:59 ` Jacopo Mondi
2019-10-02 12:14 ` Benoit Parrot
2019-10-02 12:35 ` Sakari Ailus
2019-10-02 12:40 ` Benoit Parrot
2019-10-02 14:32 ` Jacopo Mondi
2019-10-02 15:06 ` Benoit Parrot
2019-10-03 6:55 ` Jacopo Mondi
2019-09-25 15:23 ` [Patch 2/3] media: ov5640: Fix 1920x1080 mode to remove extra enable/disable Benoit Parrot
2019-09-25 15:23 ` [Patch 3/3] media: ov5640: Make 2592x1944 mode only available at 15 fps 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).