* [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
* 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
* [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
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).