Re: [PATCH 2/3] media: ov5640: add PIXEL_RATE and LINK_FREQ controls
On Tue, Apr 24, 2018 at 02:03:22PM +0200, Daniel Mack wrote: > Hi, > > On Tuesday, April 24, 2018 12:22 PM, Sakari Ailus wrote: > > On Fri, Apr 20, 2018 at 11:44:18AM +0200, Daniel Mack wrote: > >> Add v4l2 controls to report the pixel and MIPI link rates of each mode. > >> The camss camera subsystem needs them to set up the correct hardware > >> clocks. > >> > >> Tested on msm8016 based hardware. > >> > >> Signed-off-by: Daniel Mack > > > > Maxime has written a number of patches against the driver that seem very > > much related; could you rebase these on his set (v2)? > > > > https://patchwork.linuxtv.org/project/linux-media/list/?submitter=Maxime+Ripard&state=*&q=ov5640> > > I didn't know about the ongoing work in this area, so I think both this > and 3/3 are not needed. If you want, you can still pick the 1st patch in > this series, but that's just a cosmetic cleanup. That patch, too, would effectively need a rebase. I'd also suggest adding a macro that constructs the entries in the array --- much of what changes from entry to entry are fps, width, height and whether subsampling or scaling has been used. That information would likely fit nicely on a single line. The resolution names are also redundant as the size is explicitly part of the mode list variable names. -- Sakari Ailus e-mail: sakari.ai...@iki.fi
Re: [PATCH 2/3] media: ov5640: add PIXEL_RATE and LINK_FREQ controls
Hi, On Tuesday, April 24, 2018 12:22 PM, Sakari Ailus wrote: > On Fri, Apr 20, 2018 at 11:44:18AM +0200, Daniel Mack wrote: >> Add v4l2 controls to report the pixel and MIPI link rates of each mode. >> The camss camera subsystem needs them to set up the correct hardware >> clocks. >> >> Tested on msm8016 based hardware. >> >> Signed-off-by: Daniel Mack > > Maxime has written a number of patches against the driver that seem very > much related; could you rebase these on his set (v2)? > > https://patchwork.linuxtv.org/project/linux-media/list/?submitter=Maxime+Ripard&state=*&q=ov5640> I didn't know about the ongoing work in this area, so I think both this and 3/3 are not needed. If you want, you can still pick the 1st patch in this series, but that's just a cosmetic cleanup. Thanks, Daniel
Re: [PATCH 2/3] media: ov5640: add PIXEL_RATE and LINK_FREQ controls
Hi Daniel,
On Fri, Apr 20, 2018 at 11:44:18AM +0200, Daniel Mack wrote:
> Add v4l2 controls to report the pixel and MIPI link rates of each mode.
> The camss camera subsystem needs them to set up the correct hardware
> clocks.
>
> Tested on msm8016 based hardware.
>
> Signed-off-by: Daniel Mack
Maxime has written a number of patches against the driver that seem very
much related; could you rebase these on his set (v2)?
https://patchwork.linuxtv.org/project/linux-media/list/?submitter=Maxime+Ripard&state=*&q=ov5640>
> ---
> drivers/media/i2c/ov5640.c | 77
> ++
> 1 file changed, 77 insertions(+)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 96f1564abdf5..78669ed386cd 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -91,6 +91,20 @@
> #define OV5640_REG_SDE_CTRL5 0x5585
> #define OV5640_REG_AVG_READOUT 0x56a1
>
> +#define OV5640_LINK_FREQ_111 0
> +#define OV5640_LINK_FREQ_166 1
> +#define OV5640_LINK_FREQ_222 2
> +#define OV5640_LINK_FREQ_333 3
> +#define OV5640_LINK_FREQ_666 4
> +
> +static const s64 link_freq_menu_items[] = {
> + 11106,
> + 16660,
> + 22213,
> + 33220,
> + 66640,
> +};
> +
> enum ov5640_mode_id {
> OV5640_MODE_QCIF_176_144 = 0,
> OV5640_MODE_QVGA_320_240,
> @@ -167,12 +181,18 @@ struct ov5640_mode_info {
> enum ov5640_downsize_mode dn_mode;
> u32 width;
> u32 height;
> + u32 pixel_rate;
> + u32 link_freq_idx;
> const struct reg_value *reg_data;
> u32 reg_data_size;
> };
>
> struct ov5640_ctrls {
> struct v4l2_ctrl_handler handler;
> + struct {
> + struct v4l2_ctrl *link_freq;
> + struct v4l2_ctrl *pixel_rate;
> + };
> struct {
> struct v4l2_ctrl *auto_exp;
> struct v4l2_ctrl *exposure;
> @@ -732,6 +752,8 @@ static const struct ov5640_mode_info
> ov5640_mode_init_data = {
> .dn_mode= SUBSAMPLING,
> .width = 640,
> .height = 480,
> + .pixel_rate = 2776,
> + .link_freq_idx = OV5640_LINK_FREQ_111,
> .reg_data = ov5640_init_setting_30fps_VGA,
> .reg_data_size = ARRAY_SIZE(ov5640_init_setting_30fps_VGA),
> };
> @@ -744,6 +766,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES]
> = {
> .dn_mode= SUBSAMPLING,
> .width = 176,
> .height = 144,
> + .pixel_rate = 2776,
> + .link_freq_idx = OV5640_LINK_FREQ_111,
> .reg_data = ov5640_setting_15fps_QCIF_176_144,
> .reg_data_size = ARRAY_SIZE(ov5640_setting_15fps_QCIF_176_144),
> },
> @@ -752,6 +776,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES]
> = {
> .dn_mode= SUBSAMPLING,
> .width = 320,
> .height = 240,
> + .pixel_rate = 2776,
> + .link_freq_idx = OV5640_LINK_FREQ_111,
> .reg_data = ov5640_setting_15fps_QVGA_320_240,
> .reg_data_size = ARRAY_SIZE(ov5640_setting_15fps_QVGA_320_240),
> },
> @@ -760,6 +786,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES]
> = {
> .dn_mode= SUBSAMPLING,
> .width = 640,
> .height = 480,
> + .pixel_rate = 2776,
> + .link_freq_idx = OV5640_LINK_FREQ_111,
> .reg_data = ov5640_setting_15fps_VGA_640_480,
> .reg_data_size = ARRAY_SIZE(ov5640_setting_15fps_VGA_640_480)
> },
> @@ -768,6 +796,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES]
> = {
> .dn_mode= SUBSAMPLING,
> .width = 720,
> .height = 480,
> + .pixel_rate = 2776,
> + .link_freq_idx = OV5640_LINK_FREQ_111,
> .reg_data = ov5640_setting_15fps_NTSC_720_480,
> .reg_data_size = ARRAY_SIZE(ov5640_setting_15fps_NTSC_720_480),
> },
> @@ -776,6 +806,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES]
> = {
> .dn_mode= SUBSAMPLING,
> .width = 720,
> .height = 576,
> + .pixel_rate = 2776,
> + .link_freq_idx = OV5640_LINK_FREQ_111,
> .reg_data = ov5640_setting_15fps_PAL_720_576,
> .reg_data_size = ARRAY_SIZE(ov5640_setting_15fps_PAL_720_576),
> },
> @@ -784,6 +816,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES]
> = {
> .dn_mode= SUBSAMPLING,
> .width = 1024,
> .height = 768,
> + .pixel_rate = 2776,
> + .link_freq_idx =
Re: [PATCH 2/3] media: ov5640: add PIXEL_RATE and LINK_FREQ controls
On Fri, Apr 20, 2018 at 04:29:42PM +0200, Daniel Mack wrote:
> Hi,
>
> On Friday, April 20, 2018 04:15 PM, Maxime Ripard wrote:
> > On Fri, Apr 20, 2018 at 11:44:18AM +0200, Daniel Mack wrote:
>
> >> struct ov5640_ctrls {
> >>struct v4l2_ctrl_handler handler;
> >> + struct {
> >> + struct v4l2_ctrl *link_freq;
> >> + struct v4l2_ctrl *pixel_rate;
> >> + };
> >>struct {
> >>struct v4l2_ctrl *auto_exp;
> >>struct v4l2_ctrl *exposure;
> >> @@ -732,6 +752,8 @@ static const struct ov5640_mode_info
> >> ov5640_mode_init_data = {
> >>.dn_mode= SUBSAMPLING,
> >>.width = 640,
> >>.height = 480,
> >> + .pixel_rate = 2776,
> >> + .link_freq_idx = OV5640_LINK_FREQ_111,
> >
> > I'm not sure where this is coming from, but on a parallel sensor I
> > have a quite different pixel rate.
>
> Ah, interesting. What exactly do you mean by 'parallel'? What kind of
> module is that, and what are your pixel rates?
An RGB bus, or MIPI-DPI, or basically a pixel clock + 1 line for each
bit. The sensor can operate using both that bus and a MIPI-CSI2 one.
You have the list of pixel rates in the patch I've linked before:
https://patchwork.linuxtv.org/patch/48710/
There's one pixel sent per clock cycle, so the pixel rate is the same
than the clock rate.
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
Re: [PATCH 2/3] media: ov5640: add PIXEL_RATE and LINK_FREQ controls
Hi,
On Friday, April 20, 2018 04:15 PM, Maxime Ripard wrote:
> On Fri, Apr 20, 2018 at 11:44:18AM +0200, Daniel Mack wrote:
>> struct ov5640_ctrls {
>> struct v4l2_ctrl_handler handler;
>> +struct {
>> +struct v4l2_ctrl *link_freq;
>> +struct v4l2_ctrl *pixel_rate;
>> +};
>> struct {
>> struct v4l2_ctrl *auto_exp;
>> struct v4l2_ctrl *exposure;
>> @@ -732,6 +752,8 @@ static const struct ov5640_mode_info
>> ov5640_mode_init_data = {
>> .dn_mode= SUBSAMPLING,
>> .width = 640,
>> .height = 480,
>> +.pixel_rate = 2776,
>> +.link_freq_idx = OV5640_LINK_FREQ_111,
>
> I'm not sure where this is coming from, but on a parallel sensor I
> have a quite different pixel rate.
Ah, interesting. What exactly do you mean by 'parallel'? What kind of
module is that, and what are your pixel rates?
> I have a serie ongoing that tries to deal with this, hopefully in
> order to get rid of all the clock setup done in the initialiasation
> array.
>
> See https://patchwork.linuxtv.org/patch/48710/ for the patch and
> https://www.spinics.net/lists/linux-media/msg132201.html for a
> discussion on what the clock tree might look like on a MIPI-CSI bus.
Okay, nice. Even better if this patch isn't needed in the end.
Thanks!
Daniel
Re: [PATCH 2/3] media: ov5640: add PIXEL_RATE and LINK_FREQ controls
Hi,
On Fri, Apr 20, 2018 at 11:44:18AM +0200, Daniel Mack wrote:
> Add v4l2 controls to report the pixel and MIPI link rates of each mode.
> The camss camera subsystem needs them to set up the correct hardware
> clocks.
>
> Tested on msm8016 based hardware.
>
> Signed-off-by: Daniel Mack
> ---
> drivers/media/i2c/ov5640.c | 77
> ++
> 1 file changed, 77 insertions(+)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 96f1564abdf5..78669ed386cd 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -91,6 +91,20 @@
> #define OV5640_REG_SDE_CTRL5 0x5585
> #define OV5640_REG_AVG_READOUT 0x56a1
>
> +#define OV5640_LINK_FREQ_111 0
> +#define OV5640_LINK_FREQ_166 1
> +#define OV5640_LINK_FREQ_222 2
> +#define OV5640_LINK_FREQ_333 3
> +#define OV5640_LINK_FREQ_666 4
> +
> +static const s64 link_freq_menu_items[] = {
> + 11106,
> + 16660,
> + 22213,
> + 33220,
> + 66640,
> +};
> +
> enum ov5640_mode_id {
> OV5640_MODE_QCIF_176_144 = 0,
> OV5640_MODE_QVGA_320_240,
> @@ -167,12 +181,18 @@ struct ov5640_mode_info {
> enum ov5640_downsize_mode dn_mode;
> u32 width;
> u32 height;
> + u32 pixel_rate;
> + u32 link_freq_idx;
> const struct reg_value *reg_data;
> u32 reg_data_size;
> };
>
> struct ov5640_ctrls {
> struct v4l2_ctrl_handler handler;
> + struct {
> + struct v4l2_ctrl *link_freq;
> + struct v4l2_ctrl *pixel_rate;
> + };
> struct {
> struct v4l2_ctrl *auto_exp;
> struct v4l2_ctrl *exposure;
> @@ -732,6 +752,8 @@ static const struct ov5640_mode_info
> ov5640_mode_init_data = {
> .dn_mode= SUBSAMPLING,
> .width = 640,
> .height = 480,
> + .pixel_rate = 2776,
> + .link_freq_idx = OV5640_LINK_FREQ_111,
I'm not sure where this is coming from, but on a parallel sensor I
have a quite different pixel rate.
I have a serie ongoing that tries to deal with this, hopefully in
order to get rid of all the clock setup done in the initialiasation
array.
See https://patchwork.linuxtv.org/patch/48710/ for the patch and
https://www.spinics.net/lists/linux-media/msg132201.html for a
discussion on what the clock tree might look like on a MIPI-CSI bus.
Feel free to step in the discussion.
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
signature.asc
Description: PGP signature
[PATCH 2/3] media: ov5640: add PIXEL_RATE and LINK_FREQ controls
Add v4l2 controls to report the pixel and MIPI link rates of each mode.
The camss camera subsystem needs them to set up the correct hardware
clocks.
Tested on msm8016 based hardware.
Signed-off-by: Daniel Mack
---
drivers/media/i2c/ov5640.c | 77 ++
1 file changed, 77 insertions(+)
diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 96f1564abdf5..78669ed386cd 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -91,6 +91,20 @@
#define OV5640_REG_SDE_CTRL5 0x5585
#define OV5640_REG_AVG_READOUT 0x56a1
+#define OV5640_LINK_FREQ_111 0
+#define OV5640_LINK_FREQ_166 1
+#define OV5640_LINK_FREQ_222 2
+#define OV5640_LINK_FREQ_333 3
+#define OV5640_LINK_FREQ_666 4
+
+static const s64 link_freq_menu_items[] = {
+ 11106,
+ 16660,
+ 22213,
+ 33220,
+ 66640,
+};
+
enum ov5640_mode_id {
OV5640_MODE_QCIF_176_144 = 0,
OV5640_MODE_QVGA_320_240,
@@ -167,12 +181,18 @@ struct ov5640_mode_info {
enum ov5640_downsize_mode dn_mode;
u32 width;
u32 height;
+ u32 pixel_rate;
+ u32 link_freq_idx;
const struct reg_value *reg_data;
u32 reg_data_size;
};
struct ov5640_ctrls {
struct v4l2_ctrl_handler handler;
+ struct {
+ struct v4l2_ctrl *link_freq;
+ struct v4l2_ctrl *pixel_rate;
+ };
struct {
struct v4l2_ctrl *auto_exp;
struct v4l2_ctrl *exposure;
@@ -732,6 +752,8 @@ static const struct ov5640_mode_info ov5640_mode_init_data
= {
.dn_mode= SUBSAMPLING,
.width = 640,
.height = 480,
+ .pixel_rate = 2776,
+ .link_freq_idx = OV5640_LINK_FREQ_111,
.reg_data = ov5640_init_setting_30fps_VGA,
.reg_data_size = ARRAY_SIZE(ov5640_init_setting_30fps_VGA),
};
@@ -744,6 +766,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] =
{
.dn_mode= SUBSAMPLING,
.width = 176,
.height = 144,
+ .pixel_rate = 2776,
+ .link_freq_idx = OV5640_LINK_FREQ_111,
.reg_data = ov5640_setting_15fps_QCIF_176_144,
.reg_data_size = ARRAY_SIZE(ov5640_setting_15fps_QCIF_176_144),
},
@@ -752,6 +776,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] =
{
.dn_mode= SUBSAMPLING,
.width = 320,
.height = 240,
+ .pixel_rate = 2776,
+ .link_freq_idx = OV5640_LINK_FREQ_111,
.reg_data = ov5640_setting_15fps_QVGA_320_240,
.reg_data_size = ARRAY_SIZE(ov5640_setting_15fps_QVGA_320_240),
},
@@ -760,6 +786,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] =
{
.dn_mode= SUBSAMPLING,
.width = 640,
.height = 480,
+ .pixel_rate = 2776,
+ .link_freq_idx = OV5640_LINK_FREQ_111,
.reg_data = ov5640_setting_15fps_VGA_640_480,
.reg_data_size = ARRAY_SIZE(ov5640_setting_15fps_VGA_640_480)
},
@@ -768,6 +796,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] =
{
.dn_mode= SUBSAMPLING,
.width = 720,
.height = 480,
+ .pixel_rate = 2776,
+ .link_freq_idx = OV5640_LINK_FREQ_111,
.reg_data = ov5640_setting_15fps_NTSC_720_480,
.reg_data_size = ARRAY_SIZE(ov5640_setting_15fps_NTSC_720_480),
},
@@ -776,6 +806,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] =
{
.dn_mode= SUBSAMPLING,
.width = 720,
.height = 576,
+ .pixel_rate = 2776,
+ .link_freq_idx = OV5640_LINK_FREQ_111,
.reg_data = ov5640_setting_15fps_PAL_720_576,
.reg_data_size = ARRAY_SIZE(ov5640_setting_15fps_PAL_720_576),
},
@@ -784,6 +816,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] =
{
.dn_mode= SUBSAMPLING,
.width = 1024,
.height = 768,
+ .pixel_rate = 2776,
+ .link_freq_idx = OV5640_LINK_FREQ_111,
.reg_data = ov5640_setting_15fps_XGA_1024_768,
.reg_data_size = ARRAY_SIZE(ov5640_setting_15fps_XGA_1024_768),
},
@@ -792,6 +826,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] =
{
.dn_mode= SUBSAMPLING,
.width = 1280,
.height = 720,
+
