Re: [PATCH v4] venus: venc: Fix enum frameintervals
Hi Stanimir, On Thu, 17 Oct 2019 at 17:47, Stanimir Varbanov wrote: > > Hi Loic, > > On 10/17/19 6:08 PM, Loic Poulain wrote: > > Hi Stanimir, > > > > On Thu, 3 Oct 2019 at 12:15, Stanimir Varbanov > > wrote: > >> > >> I have tested this on db410c with following gst pipeline: > >> > >> gst-launch-1.0 -v videotestsrc ! > >> video/x-raw,format=NV12,width=1280,height=960,framerate=24/1 ! > >> v4l2h264enc > >> extra-controls="controls,h264_profile=4,h264_level="5",video_bitrate=1000;" > >> ! filesink location=gstenc.h264 > >> > >> Loic, could you give it a try on db820c too? > >> > >> Here is the info on the bug which I try to fix with current patch: > >> > >> https://bugs.96boards.org/show_bug.cgi?id=513 > >> > >> On 10/3/19 1:10 PM, Stanimir Varbanov wrote: > >>> This fixes an issue when setting the encoder framerate because of > >>> missing precision. Now the frameinterval type is changed to > >>> TYPE_CONTINUOUS and step = 1. Also the math is changed when > >>> framerate property is called - the firmware side expects the > >>> framerate in Q16 values. > >>> > >>> Signed-off-by: Stanimir Varbanov > >>> --- > >>> > >>> Changes since v3: > >>> Keep min/max numerator one, and divide frate(max/min) to frame > >>> factor (returned framerate max/min capabilities are in range > >>> 1 to 120fps but in Q16 i.e. 65536 to 7864320). > >>> > >>> drivers/media/platform/qcom/venus/venc.c | 17 - > >>> 1 file changed, 12 insertions(+), 5 deletions(-) > >>> > >>> diff --git a/drivers/media/platform/qcom/venus/venc.c > >>> b/drivers/media/platform/qcom/venus/venc.c > >>> index 1b7fb2d5887c..133ff7eceb83 100644 > >>> --- a/drivers/media/platform/qcom/venus/venc.c > >>> +++ b/drivers/media/platform/qcom/venus/venc.c > >>> @@ -22,6 +22,7 @@ > >>> #include "venc.h" > >>> > >>> #define NUM_B_FRAMES_MAX 4 > >>> +#define FRAMERATE_FACTOR BIT(16) > >>> > >>> /* > >>> * Three resons to keep MPLANE formats (despite that the number of planes > >>> @@ -576,7 +577,7 @@ static int venc_enum_frameintervals(struct file > >>> *file, void *fh, > >>> struct venus_inst *inst = to_inst(file); > >>> const struct venus_format *fmt; > >>> > >>> - fival->type = V4L2_FRMIVAL_TYPE_STEPWISE; > >>> + fival->type = V4L2_FRMIVAL_TYPE_CONTINUOUS; > >>> > >>> fmt = find_format(inst, fival->pixel_format, > >>> V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE); > >>> @@ -600,11 +601,11 @@ static int venc_enum_frameintervals(struct file > >>> *file, void *fh, > >>> return -EINVAL; > >>> > >>> fival->stepwise.min.numerator = 1; > >>> - fival->stepwise.min.denominator = frate_max(inst); > >>> + fival->stepwise.min.denominator = frate_max(inst) / > >>> FRAMERATE_FACTOR; > > > > On 820c frate_max() returns 120 set denominator to 0, and causes > > gstreamer failure. > > OK, thanks! > > We have two options > - unify frate_min/max() to return in Q16 depending on the hfi version > - or move frame_factor in frate_min/max() and return the framerate (1..120) No strong preference... Regards, Loic
Re: [PATCH v4] venus: venc: Fix enum frameintervals
Hi Stanimir, On Thu, 3 Oct 2019 at 12:15, Stanimir Varbanov wrote: > > I have tested this on db410c with following gst pipeline: > > gst-launch-1.0 -v videotestsrc ! > video/x-raw,format=NV12,width=1280,height=960,framerate=24/1 ! > v4l2h264enc > extra-controls="controls,h264_profile=4,h264_level="5",video_bitrate=1000;" > ! filesink location=gstenc.h264 > > Loic, could you give it a try on db820c too? > > Here is the info on the bug which I try to fix with current patch: > > https://bugs.96boards.org/show_bug.cgi?id=513 > > On 10/3/19 1:10 PM, Stanimir Varbanov wrote: > > This fixes an issue when setting the encoder framerate because of > > missing precision. Now the frameinterval type is changed to > > TYPE_CONTINUOUS and step = 1. Also the math is changed when > > framerate property is called - the firmware side expects the > > framerate in Q16 values. > > > > Signed-off-by: Stanimir Varbanov > > --- > > > > Changes since v3: > > Keep min/max numerator one, and divide frate(max/min) to frame > > factor (returned framerate max/min capabilities are in range > > 1 to 120fps but in Q16 i.e. 65536 to 7864320). > > > > drivers/media/platform/qcom/venus/venc.c | 17 - > > 1 file changed, 12 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/media/platform/qcom/venus/venc.c > > b/drivers/media/platform/qcom/venus/venc.c > > index 1b7fb2d5887c..133ff7eceb83 100644 > > --- a/drivers/media/platform/qcom/venus/venc.c > > +++ b/drivers/media/platform/qcom/venus/venc.c > > @@ -22,6 +22,7 @@ > > #include "venc.h" > > > > #define NUM_B_FRAMES_MAX 4 > > +#define FRAMERATE_FACTOR BIT(16) > > > > /* > > * Three resons to keep MPLANE formats (despite that the number of planes > > @@ -576,7 +577,7 @@ static int venc_enum_frameintervals(struct file *file, > > void *fh, > > struct venus_inst *inst = to_inst(file); > > const struct venus_format *fmt; > > > > - fival->type = V4L2_FRMIVAL_TYPE_STEPWISE; > > + fival->type = V4L2_FRMIVAL_TYPE_CONTINUOUS; > > > > fmt = find_format(inst, fival->pixel_format, > > V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE); > > @@ -600,11 +601,11 @@ static int venc_enum_frameintervals(struct file > > *file, void *fh, > > return -EINVAL; > > > > fival->stepwise.min.numerator = 1; > > - fival->stepwise.min.denominator = frate_max(inst); > > + fival->stepwise.min.denominator = frate_max(inst) / FRAMERATE_FACTOR; On 820c frate_max() returns 120 set denominator to 0, and causes gstreamer failure. Regards, Loic
[PATCH v2] media: venus: core: Fix msm8996 frequency table
In downstream driver, there are two frequency tables defined,
one for the encoder and one for the decoder:
/* Encoders /
<972000 49000 0x>, / 4k UHD @ 30 /
<489600 32000 0x>, / 1080p @ 60 /
<244800 15000 0x>, / 1080p @ 30 /
<108000 7500 0x>, / 720p @ 30 */
/* Decoders /
<1944000 49000 0x>, / 4k UHD @ 60 /
< 972000 32000 0x>, / 4k UHD @ 30 /
< 489600 15000 0x>, / 1080p @ 60 /
< 244800 7500 0x>; / 1080p @ 30 */
It shows that encoder always needs a higher clock than decoder.
In current venus driver, the unified frequency table is aligned
with the downstream decoder table which causes performance issues
in encoding scenarios. Fix that by aligning frequency table on
worst case (encoding).
Signed-off-by: Loic Poulain
---
drivers/media/platform/qcom/venus/core.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/core.c
b/drivers/media/platform/qcom/venus/core.c
index 0acc757..e0d5a10 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -427,10 +427,11 @@ static const struct venus_resources msm8916_res = {
};
static const struct freq_tbl msm8996_freq_table[] = {
- { 1944000, 49000 }, /* 4k UHD @ 60 */
- { 972000, 32000 }, /* 4k UHD @ 30 */
- { 489600, 15000 }, /* 1080p @ 60 */
- { 244800, 7500 }, /* 1080p @ 30 */
+ { 1944000, 52000 }, /* 4k UHD @ 60 (decode only) */
+ { 972000, 52000 }, /* 4k UHD @ 30 */
+ { 489600, 34667 }, /* 1080p @ 60 */
+ { 244800, 15000 }, /* 1080p @ 30 */
+ { 108000, 7500 }, /* 720p @ 30 */
};
static const struct reg_val msm8996_reg_preset[] = {
--
2.7.4
[PATCH] media: venus: core: Fix msm8996 frequency table
In downstream driver, there are two frequency tables defined,
one for the encoder and one for the decoder:
/* Encoders /
<972000 49000 0x>, / 4k UHD @ 30 /
<489600 32000 0x>, / 1080p @ 60 /
<244800 15000 0x>, / 1080p @ 30 /
<108000 7500 0x>, / 720p @ 30 */
/* Decoders /
<1944000 49000 0x>, / 4k UHD @ 60 /
< 972000 32000 0x>, / 4k UHD @ 30 /
< 489600 15000 0x>, / 1080p @ 60 /
< 244800 7500 0x>; / 1080p @ 30 */
It shows that encoder always needs a higher clock than decoder.
In current venus driver, the unified frequency table is aligned
with the downstream decoder table which causes performance issues
in decoding scenarios. Fix that by aligning frequency table on
worst case (encoding).
Signed-off-by: Loic Poulain
---
drivers/media/platform/qcom/venus/core.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/core.c
b/drivers/media/platform/qcom/venus/core.c
index 0acc757..1e80689 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -427,10 +427,11 @@ static const struct venus_resources msm8916_res = {
};
static const struct freq_tbl msm8996_freq_table[] = {
- { 1944000, 49000 }, /* 4k UHD @ 60 */
- { 972000, 32000 }, /* 4k UHD @ 30 */
- { 489600, 15000 }, /* 1080p @ 60 */
- { 244800, 7500 }, /* 1080p @ 30 */
+ { 1944000, 49000 }, /* 4k UHD @ 60 (decode only) */
+ { 972000, 49000 }, /* 4k UHD @ 30 */
+ { 489600, 32000 }, /* 1080p @ 60 */
+ { 244800, 15000 }, /* 1080p @ 30 */
+ { 108000, 7500 }, /* 720p @ 30 */
};
static const struct reg_val msm8996_reg_preset[] = {
--
2.7.4
Re: Issues with ov5640 sensor
Hi Eugen, On Wed, 13 Feb 2019 at 09:02, wrote: > > Hello Loic, > > I am trying to make sensor Omnivision ov5640 work with our Atmel-isc > controller, I saw you implemented RAW mode for this sensor in the > driver, so I was hoping I can ask you some things: > > I cannot make the RAW bayer format work, BA81 / mbus > MEDIA_BUS_FMT_SBGGR8_1X8 makes the photo look like a maze of colors... > > The sensor works for me in YUYV and RGB565 mode, so I assume the wiring > is done correctly for my setup > > Anything special I need to do for this format to work ? I definitely need to check with the latest driver version, many changes have been integrated recently, including clock autoconfiguration. Moreover, AFAIU, you are connected via the parallel interface, I only tested with MIPI/CSI. I would suggest you adding debug in the ov5640 driver to retrieve calculated pclk, sysclk, etc... Also the following lines does'nt look correct anymore: /* * All the formats we support have 16 bits per pixel, seems to require * the same rate than YUV, so we can just use 16 bpp all the time. */ rate = mode->vtot * mode->htot * 16; rate *= ov5640_framerates[sensor->current_fr]; With RAW8, we have 8 bits per pixel, maybe it would worth for testing purpose to change 16 to 8 and see what happens. > > The same RAW BAYER configuration works for me on ov7670 for example... > > Unrelated: are you familiar with ov7740 ? This sensor looks to have > stopped working in latest mediatree : failed to enable streaming. > (worked perfectly in last stable for me - 4.14...) No sorry. Regards, Loic
[PATCH] media: i2c: ov5640: Fix post-reset delay
According to the ov5640 specification (2.7 power up sequence), host can access the sensor's registers 20ms after reset. Trying to access them before leads to undefined behavior and result in sporadic initialization errors. Signed-off-by: Loic Poulain --- drivers/media/i2c/ov5640.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c index 5a909ab..6415231 100644 --- a/drivers/media/i2c/ov5640.c +++ b/drivers/media/i2c/ov5640.c @@ -1906,7 +1906,7 @@ static void ov5640_reset(struct ov5640_dev *sensor) usleep_range(1000, 2000); gpiod_set_value_cansleep(sensor->reset_gpio, 0); - usleep_range(5000, 1); + usleep_range(2, 25000); } static int ov5640_set_power_on(struct ov5640_dev *sensor) -- 2.7.4
[PATCH] media: ov5640: Add RAW bayer format support
OV5640 sensor supports raw image output (bayer).
Configure ISP mux/format registers accordingly.
Signed-off-by: Loic Poulain
---
drivers/media/i2c/ov5640.c | 58 --
1 file changed, 46 insertions(+), 12 deletions(-)
diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 071f4bc..e38e05e 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -116,6 +116,15 @@ enum ov5640_frame_rate {
OV5640_NUM_FRAMERATES,
};
+enum ov5640_format_mux {
+ OV5640_FMT_MUX_YUV422 = 0,
+ OV5640_FMT_MUX_RGB,
+ OV5640_FMT_MUX_DITHER,
+ OV5640_FMT_MUX_RAW_DPC,
+ OV5640_FMT_MUX_SNR_RAW,
+ OV5640_FMT_MUX_RAW_CIP,
+};
+
struct ov5640_pixfmt {
u32 code;
u32 colorspace;
@@ -127,6 +136,10 @@ static const struct ov5640_pixfmt ov5640_formats[] = {
{ MEDIA_BUS_FMT_YUYV8_2X8, V4L2_COLORSPACE_SRGB, },
{ MEDIA_BUS_FMT_RGB565_2X8_LE, V4L2_COLORSPACE_SRGB, },
{ MEDIA_BUS_FMT_RGB565_2X8_BE, V4L2_COLORSPACE_SRGB, },
+ { MEDIA_BUS_FMT_SBGGR8_1X8, V4L2_COLORSPACE_SRGB, },
+ { MEDIA_BUS_FMT_SGBRG8_1X8, V4L2_COLORSPACE_SRGB, },
+ { MEDIA_BUS_FMT_SGRBG8_1X8, V4L2_COLORSPACE_SRGB, },
+ { MEDIA_BUS_FMT_SRGGB8_1X8, V4L2_COLORSPACE_SRGB, },
};
/*
@@ -1980,46 +1993,67 @@ static int ov5640_set_framefmt(struct ov5640_dev
*sensor,
struct v4l2_mbus_framefmt *format)
{
int ret = 0;
- bool is_rgb = false;
bool is_jpeg = false;
- u8 val;
+ u8 fmt, mux;
switch (format->code) {
case MEDIA_BUS_FMT_UYVY8_2X8:
/* YUV422, UYVY */
- val = 0x3f;
+ fmt = 0x3f;
+ mux = OV5640_FMT_MUX_YUV422;
break;
case MEDIA_BUS_FMT_YUYV8_2X8:
/* YUV422, YUYV */
- val = 0x30;
+ fmt = 0x30;
+ mux = OV5640_FMT_MUX_YUV422;
break;
case MEDIA_BUS_FMT_RGB565_2X8_LE:
/* RGB565 {g[2:0],b[4:0]},{r[4:0],g[5:3]} */
- val = 0x6F;
- is_rgb = true;
+ fmt = 0x6F;
+ mux = OV5640_FMT_MUX_RGB;
break;
case MEDIA_BUS_FMT_RGB565_2X8_BE:
/* RGB565 {r[4:0],g[5:3]},{g[2:0],b[4:0]} */
- val = 0x61;
- is_rgb = true;
+ fmt = 0x61;
+ mux = OV5640_FMT_MUX_RGB;
break;
case MEDIA_BUS_FMT_JPEG_1X8:
/* YUV422, YUYV */
- val = 0x30;
+ fmt = 0x30;
+ mux = OV5640_FMT_MUX_YUV422;
is_jpeg = true;
break;
+ case MEDIA_BUS_FMT_SBGGR8_1X8:
+ /* Raw, BGBG... / GRGR... */
+ fmt = 0x00;
+ mux = OV5640_FMT_MUX_RAW_DPC;
+ break;
+ case MEDIA_BUS_FMT_SGBRG8_1X8:
+ /* Raw bayer, GBGB... / RGRG... */
+ fmt = 0x01;
+ mux = OV5640_FMT_MUX_RAW_DPC;
+ break;
+ case MEDIA_BUS_FMT_SGRBG8_1X8:
+ /* Raw bayer, GRGR... / BGBG... */
+ fmt = 0x02;
+ mux = OV5640_FMT_MUX_RAW_DPC;
+ break;
+ case MEDIA_BUS_FMT_SRGGB8_1X8:
+ /* Raw bayer, RGRG... / GBGB... */
+ fmt = 0x03;
+ mux = OV5640_FMT_MUX_RAW_DPC;
+ break;
default:
return -EINVAL;
}
/* FORMAT CONTROL00: YUV and RGB formatting */
- ret = ov5640_write_reg(sensor, OV5640_REG_FORMAT_CONTROL00, val);
+ ret = ov5640_write_reg(sensor, OV5640_REG_FORMAT_CONTROL00, fmt);
if (ret)
return ret;
/* FORMAT MUX CONTROL: ISP YUV or RGB */
- ret = ov5640_write_reg(sensor, OV5640_REG_ISP_FORMAT_MUX_CTRL,
- is_rgb ? 0x01 : 0x00);
+ ret = ov5640_write_reg(sensor, OV5640_REG_ISP_FORMAT_MUX_CTRL, mux);
if (ret)
return ret;
--
2.7.4
Re: [PATCH v3 1/2] media: ov5640: Re-work MIPI startup sequence
On 6 September 2018 at 10:48, jacopo mondi wrote:
> Hello Loic,
>
> On Thu, Sep 06, 2018 at 10:13:53AM +0200, Loic Poulain wrote:
>> On 6 September 2018 at 09:48, jacopo mondi wrote:
>> > Hello Loic,
>> >thanks for looking into this
>> >
>> > On Tue, Sep 04, 2018 at 07:22:50PM +0200, Loic Poulain wrote:
>> >> Hi Jacopo,
>> >>
>> >> > - ret = ov5640_mod_reg(sensor, OV5640_REG_MIPI_CTRL00, BIT(5),
>> >> > -on ? 0 : BIT(5));
>> >> > - if (ret)
>> >> > - return ret;
>> >> > - ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00,
>> >> > - on ? 0x00 : 0x70);
>> >> > + /*
>> >> > +* Enable/disable the MIPI interface
>> >> > +*
>> >> > +* 0x300e = on ? 0x45 : 0x40
>> >> > +* [7:5] = 001 : 2 data lanes mode
>> >>
>> >> Does 2-Lanes work with this config?
>> >> AFAIU, if 2-Lanes is bit 5, value should be 0x25 and 0x20.
>> >>
>> >
>> > Yes, confusing.
>> >
>> > The sensor manual reports
>> > 0x300e[7:5] = 000 one lane mode
>> > 0x300e[7:5] = 001 two lanes mode
>> >
>> > Although this configuration works with 2 lanes, and the application
>> > note I have, with the suggested settings for MIPI CSI-2 2 lanes
>> > reports 0x40 to be the 2 lanes mode...
>> >
>> > I used that one, also because the removed entry from the settings blob
>> > is:
>> > - {0x300e, 0x45, 0, 0}, {0x302e, 0x08, 0, 0}, {0x4300, 0x3f, 0, 0},
>> > + {0x302e, 0x08, 0, 0}, {0x4300, 0x3f, 0, 0},
>> >
>> > So it was using BIT(6) already.
>>
>> Yes, it was setting BIT(6) from static config and BIT(5) from the
>> ov5640_set_stream_mipi function. In your patch you don't set
>> BIT(5) anymore.
>>
>> So it's not clear to me why it is still working, and the datasheet does
>> not help a lot on this (BIT(6) is for debug modes).
>> FYI I tried with BIT(5) only but it does not work (though I did not
>> investigate a lot).
>
> Thanks. Is your setup using 1 or 2 lanes? (I assume 2...)
>
> Another question, unrelated to this specific issue: was the ov5640
> working with dragonboard before this patch? I'm asking as I've seen
> different behaviors between different platforms, and knowing this
> fixes a widespread one like dragonboard is, would help getting this
> patches in faster :)
I did not test without the patch, will do.
Re: [PATCH v3 1/2] media: ov5640: Re-work MIPI startup sequence
On 6 September 2018 at 09:48, jacopo mondi wrote:
> Hello Loic,
>thanks for looking into this
>
> On Tue, Sep 04, 2018 at 07:22:50PM +0200, Loic Poulain wrote:
>> Hi Jacopo,
>>
>> > - ret = ov5640_mod_reg(sensor, OV5640_REG_MIPI_CTRL00, BIT(5),
>> > -on ? 0 : BIT(5));
>> > - if (ret)
>> > - return ret;
>> > - ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00,
>> > - on ? 0x00 : 0x70);
>> > + /*
>> > +* Enable/disable the MIPI interface
>> > +*
>> > +* 0x300e = on ? 0x45 : 0x40
>> > +* [7:5] = 001 : 2 data lanes mode
>>
>> Does 2-Lanes work with this config?
>> AFAIU, if 2-Lanes is bit 5, value should be 0x25 and 0x20.
>>
>
> Yes, confusing.
>
> The sensor manual reports
> 0x300e[7:5] = 000 one lane mode
> 0x300e[7:5] = 001 two lanes mode
>
> Although this configuration works with 2 lanes, and the application
> note I have, with the suggested settings for MIPI CSI-2 2 lanes
> reports 0x40 to be the 2 lanes mode...
>
> I used that one, also because the removed entry from the settings blob
> is:
> - {0x300e, 0x45, 0, 0}, {0x302e, 0x08, 0, 0}, {0x4300, 0x3f, 0, 0},
> + {0x302e, 0x08, 0, 0}, {0x4300, 0x3f, 0, 0},
>
> So it was using BIT(6) already.
Yes, it was setting BIT(6) from static config and BIT(5) from the
ov5640_set_stream_mipi function. In your patch you don't set
BIT(5) anymore.
So it's not clear to me why it is still working, and the datasheet does
not help a lot on this (BIT(6) is for debug modes).
FYI I tried with BIT(5) only but it does not work (though I did not
investigate a lot).
> Anyway, it works for me with 2 lanes (and I assume Steve), you have tested
> too, with how many lanes are you working with?
>
> Anyway, a comment there might be nice to have... Will add in next
> version
Definitely.
Regards,
Loic
Re: [PATCH v3 1/2] media: ov5640: Re-work MIPI startup sequence
Hi Jacopo, > - ret = ov5640_mod_reg(sensor, OV5640_REG_MIPI_CTRL00, BIT(5), > -on ? 0 : BIT(5)); > - if (ret) > - return ret; > - ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00, > - on ? 0x00 : 0x70); > + /* > +* Enable/disable the MIPI interface > +* > +* 0x300e = on ? 0x45 : 0x40 > +* [7:5] = 001 : 2 data lanes mode Does 2-Lanes work with this config? AFAIU, if 2-Lanes is bit 5, value should be 0x25 and 0x20. > +* [4] = 0 : Power up MIPI HS Tx > +* [3] = 0 : Power up MIPI LS Rx > +* [2] = 1/0: MIPI interface enable/disable > +* [1:0] = 01/00: FIXME: 'debug' > +*/ > + ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, > + on ? 0x45 : 0x40); Regards, Loic
Re: [PATCH v3 0/2] media: i2c: ov5640: Re-work MIPI startup sequence
On 15 August 2018 at 12:28, Jacopo Mondi wrote: > Hello ov5640 people, >this driver has received a lot of attention recently, and this series aims > to fix the CSI-2 interface startup on i.Mx6Q platforms. > > Please refer to the v2 cover letters for more background informations: > https://www.mail-archive.com/linux-media@vger.kernel.org/msg133420.html > > This two patches alone allows the MIPI interface to startup properly, but in > order to capture good images (good as in 'not completely black') exposure and > gain handling should be fixed too. > Hugues Fruchet has a series in review that fixes that issues: > [PATCH v2 0/5] Fix OV5640 exposure & gain > > And I have re-based it on top of this two fixes here: > git://jmondi.org/linux ov5640/timings_exposure > > Steve Longerbeam tested that branch on his I.MX6q SabreSD board and confirms > he > can now capture frames (I added his Tested-by tag to this patches). I have > verified the same on Engicam iCore I.MX6q and an Intel Atom based board. > > Ideally I would like to have these two fixes merged, and Hugues' ones then > applied on top. Of course, more testing on other platforms using CSI-2 is very > welcome. > > Thanks >j > > v2 -> v3: > - patch [2/2] was originally sent in a different series, compared to v2 it > removes entries from the blob array instead of adding more. > > Jacopo Mondi (2): > media: ov5640: Re-work MIPI startup sequence > media: ov5640: Fix timings setup code > > drivers/media/i2c/ov5640.c | 141 > + > 1 file changed, 92 insertions(+), 49 deletions(-) > > -- > 2.7.4 > Thanks for this work. I've just tested this with a dragonboard-410c (MICPI/CSI) + OV5640 sensor. It works on my side for 1280*720, 1920*1080 and 2592*1944 formats. Tested-by: Loic Poulain
Re: [PATCH v2 00/12] media: ov5640: Misc cleanup and improvements
Hi, On 3 May 2018 at 17:16, Maxime Ripard wrote: > Hi, > > On Wed, May 02, 2018 at 11:11:55AM -0700, Sam Bobrowicz wrote: >> > On Wednesday, 25 April 2018 01:11:19 EEST Sam Bobrowicz wrote: >> >> FYI, still hard at work on this. Did some more experiments last week >> >> that seemed to corroborate the clock tree in the spreadsheet. It also >> >> seems that the output of the P divider cell, SCLK cell and MIPI Rate >> >> cell in the spreadsheet must have a ratio of 2x:1x:8x (respectively) >> >> in order for the sensor to work properly on my platform, and that the >> >> SCLK value must be close to the "rate" variable that you calculate and >> >> pass to set_mipi_pclk. Unfortunately, I've only got the sensor working >> >> well for 1080p@15Hz and 720p@30Hz, both with a SCLK of 42MHz (aka >> >> 84:42:336). I'm running experiments now trying to adjust the htot and >> >> vtot values to create different required rates, and also to try to get >> >> faster Mipi rates working. Any information you have on the >> >> requirements of the htot and vtot values with respect to vact and hact >> >> values would likely be helpful. >> >> >> >> I'm also keeping an eye on the scaler clock, which I think may be >> >> affecting certain resolutions, but haven't been able to see it make a >> >> difference yet (see register 0x3824 and 0x460c) >> >> >> >> I plan on pushing a set of patches once I get this figured out, we can >> >> discuss what I should base them on when I get closer to that point. >> >> I'm new to this process :) >> > >> > I'm also interested in getting the ov5640 driver working with MIPI CSI-2. >> > Studying the datasheet and the driver code, I found the stream on sequence >> > to >> > be a bit weird. In particular the configuration of >> > OV5640_REG_IO_MIPI_CTRL00, >> > OV5640_REG_PAD_OUTPUT00 and OV5640_REG_MIPI_CTRL00 caught my attention. >> > >> > OV5640_REG_IO_MIPI_CTRL00 (0x300e) is set to 0x45 in the large array of >> > init >> > mode data and never touched for MIPI CSI-2 (the register is only touched in >> > ov5640_set_stream_dvp). The value means >> > >> > - mipi_lane_mode: 010 is documented as "debug mode", I would have expected >> > 000 >> > for one lane or 001 for two lanes. >> >> I noticed this too, but it seems that 010 is the correct value for two >> lane mode. I think this is a typo in the datasheet. >> >> > >> > - MIPI TX PHY power down: 0 is documented as "debug mode" and 1 as "Power >> > down >> > PHY HS TX", so I suppose 0 is correct. >> > >> > - MIPI RX PHY power down: 0 is documented as "debug mode" and 1 as "Power >> > down >> > PHY LP RX module", so I suppose 0 is correct. I however wonder why there's >> > a >> > RX PHY, it could be a typo. >> > >> > - mipi_en: 1 means MIPI enable, which should be correct. >> > >> > - BIT(0) is undocumented. >> > >> > OV5640_REG_PAD_OUTPUT00 (0x3019) isn't initialized explicitly and thus >> > retains >> > its default value of 0x00, and is controlled when starting and stopping the >> > stream where it's set to 0x00 and 0x70 respectively. Bits 6:4 control the >> > "sleep mode" state of lane 2, lane 1 and clock lane respectively, and >> > should >> > be LP11 in my opinion (that's the PHY stop state). However, setting them to >> > 0x00 when starting the stream mean that LP00 is selected as the sleep >> > state at >> > stream start, and LP11 when stopping the stream. Maybe "sleep mode" means >> > LPDT, but I would expect that to be controlled by the idle status bit in >> > register 0x4800. >> > >> >> I did not need to mess with the accesses to 0x3019 in order to get >> things working on my system. I'm not sure of this registers actual >> behavior, but it might affect idling while not streaming (after power >> on). My pipeline currently only powers the sensor while streaming, so >> I might be missing some ramifications of this register. >> >> > OV5640_REG_MIPI_CTRL00 (0x4800) is set to 0x04 in the large array of init >> > mode >> > data, and BIT(5) is then cleared at stream on time and set at stream off >> > time. >> > This means: >> > >> > - Clock lane gate enable: Clock lane is free running >> > - Line sync enable: Do not send line short packets for each line (I assume >> > that's LS/LE) >> > - Lane select: Use lane1 as default data lane. >> > - Idle status: MIPI bus will be LP11 when no packet to transmit. I would >> > have >> > expected the idle status to correspond to LPDT, and thus be LP00 (as >> > opposed >> > to the stop state that should be LP11, which I believe is named "sleep >> > mode" >> > in the datasheet and controlled in register 0x3019). >> > >> > BIT(5) is the clock lane gate enable, so at stream on time the clock is >> > set to >> > free running, and at stream off time to "Gate clock lane when no packet to >> > transmit". Couldn't we always enable clock gating ? >> >> Good question, it might be worth testing. Same as above, I didn't need >> to mess with this reg either. >> >> > Do you have any insight on this ? Have you modified th
Re: [PATCH v2 00/12] media: ov5640: Misc cleanup and improvements
Hi, > Good news, MIPI is now working on my platform. I had to make several > changes to how the mipi clocking is calculated in order to get things > stable, but I think I got it figured out. Maxime's changes were really > helpful. Great, I also try to make it work with MIPI-CSI2, If you have found the magic formula to configure the registers, I would be pleased to test it on my side. > > I will try to get some patches out today or tomorrow that should get > you up and running. > > Maxime, I'd prefer to create the patches myself for the experience. > I've read all of the submission guidelines and I understand the > general process, but how should I submit them to the mailing list > since they are based to your patches, which are still under review? > Should I send the patch series to the mailing list myself and just > mention this patch series, maybe with the In-Reply-To header? Or > should I just post a link to them here so you can review them and add > them to a new version of your patch series? Yes, I think your patch(es) should be integrated in the Maxime's series. Regards, Loic
Re: [RESEND PATCH] media: i2c: ov5640: Add pixel clock support
Hi Sakari, >> Any comments on this change? > > https://patchwork.linuxtv.org/project/linux-media/list/?submitter=Maxime+Ripard&state=*&q=ov5640> > > There's also another set that adds PIXEL_CLOCK (as well as LINK_FREQ) > support to the driver, that seems more complete than this patch but > requires a rebase on Maxime's patches: > > https://patchwork.linuxtv.org/project/linux-media/list/?submitter=7218&state=*&q=ov5640> Thanks, I've just see this patch series. Indeed, patch will need a rework/rebase. Regards, Loic
Re: [RESEND PATCH] media: i2c: ov5640: Add pixel clock support
On 29 March 2018 at 16:55, Manivannan Sadhasivam wrote: > Some of the camera subsystems like camss in Qualcommm MSM chipsets > require pixel clock support in camera sensor drivers. So, this commit > adds a default pixel clock rate of 96MHz to OV5640 camera sensor driver. > > According to the datasheet, 96MHz can be used as a pixel clock rate for > most of the modes. > > Signed-off-by: Manivannan Sadhasivam Tested-by: Loic Poulain It works for me on Dragonboard 410c + D3 camera mezzanine (ov5640) . Any comments on this change? Regards, Loic
[PATCH 1/2] media: venus: venc: configure entropy mode
H264 entropy mode can be selected via V4L2 API but is eventually not
applied. Configure encoder with selected mode, CALVC (def) or CABAC.
Note that hw/firmware also expects a CABAC model configuration which
currently doesn't have existing V4L2 API control. For now, use model_0
which seems always supported and so the default one.
Signed-off-by: Loic Poulain
---
drivers/media/platform/qcom/venus/venc.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/drivers/media/platform/qcom/venus/venc.c
b/drivers/media/platform/qcom/venus/venc.c
index 6f123a3..d5d824e 100644
--- a/drivers/media/platform/qcom/venus/venc.c
+++ b/drivers/media/platform/qcom/venus/venc.c
@@ -641,6 +641,7 @@ static int venc_set_properties(struct venus_inst *inst)
if (inst->fmt_cap->pixfmt == V4L2_PIX_FMT_H264) {
struct hfi_h264_vui_timing_info info;
+ struct hfi_h264_entropy_control entropy;
ptype = HFI_PROPERTY_PARAM_VENC_H264_VUI_TIMING_INFO;
info.enable = 1;
@@ -650,6 +651,16 @@ static int venc_set_properties(struct venus_inst *inst)
ret = hfi_session_set_property(inst, ptype, &info);
if (ret)
return ret;
+
+ ptype = HFI_PROPERTY_PARAM_VENC_H264_ENTROPY_CONTROL;
+ entropy.entropy_mode = venc_v4l2_to_hfi(
+ V4L2_CID_MPEG_VIDEO_H264_ENTROPY_MODE,
+ ctr->h264_entropy_mode);
+ entropy.cabac_model = HFI_H264_CABAC_MODEL_0;
+
+ ret = hfi_session_set_property(inst, ptype, &entropy);
+ if (ret)
+ return ret;
}
ptype = HFI_PROPERTY_CONFIG_VENC_IDR_PERIOD;
--
2.7.4
[PATCH 2/2] media: venus: venc: Apply inloop deblocking filter
Deblocking filter allows to reduce blocking artifacts and improve
visual quality. This is configurable via the V4L2 API but eventually
not applied to the encoder.
Note that alpha and beta deblocking values are 32-bit signed (-6;+6).
Signed-off-by: Loic Poulain
---
drivers/media/platform/qcom/venus/core.h | 4 ++--
drivers/media/platform/qcom/venus/hfi_helper.h | 4 ++--
drivers/media/platform/qcom/venus/venc.c | 22 ++
3 files changed, 26 insertions(+), 4 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/core.h
b/drivers/media/platform/qcom/venus/core.h
index cba092b..4833af7 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -144,8 +144,8 @@ struct venc_controls {
u32 h264_min_qp;
u32 h264_max_qp;
u32 h264_loop_filter_mode;
- u32 h264_loop_filter_alpha;
- u32 h264_loop_filter_beta;
+ s32 h264_loop_filter_alpha;
+ s32 h264_loop_filter_beta;
u32 vp8_min_qp;
u32 vp8_max_qp;
diff --git a/drivers/media/platform/qcom/venus/hfi_helper.h
b/drivers/media/platform/qcom/venus/hfi_helper.h
index 8d282db..55d8eb2 100644
--- a/drivers/media/platform/qcom/venus/hfi_helper.h
+++ b/drivers/media/platform/qcom/venus/hfi_helper.h
@@ -585,8 +585,8 @@ struct hfi_enable {
struct hfi_h264_db_control {
u32 mode;
- u32 slice_alpha_offset;
- u32 slice_beta_offset;
+ s32 slice_alpha_offset;
+ s32 slice_beta_offset;
};
#define HFI_H264_ENTROPY_CAVLC 0x1
diff --git a/drivers/media/platform/qcom/venus/venc.c
b/drivers/media/platform/qcom/venus/venc.c
index d5d824e..1e79f49 100644
--- a/drivers/media/platform/qcom/venus/venc.c
+++ b/drivers/media/platform/qcom/venus/venc.c
@@ -234,6 +234,16 @@ static int venc_v4l2_to_hfi(int id, int value)
case 3:
return HFI_VPX_PROFILE_VERSION_3;
}
+ case V4L2_CID_MPEG_VIDEO_H264_LOOP_FILTER_MODE:
+ switch (value) {
+ case V4L2_MPEG_VIDEO_H264_LOOP_FILTER_MODE_ENABLED:
+ default:
+ return HFI_H264_DB_MODE_ALL_BOUNDARY;
+ case V4L2_MPEG_VIDEO_H264_LOOP_FILTER_MODE_DISABLED:
+ return HFI_H264_DB_MODE_DISABLE;
+ case
V4L2_MPEG_VIDEO_H264_LOOP_FILTER_MODE_DISABLED_AT_SLICE_BOUNDARY:
+ return HFI_H264_DB_MODE_SKIP_SLICE_BOUNDARY;
+ }
}
return 0;
@@ -642,6 +652,7 @@ static int venc_set_properties(struct venus_inst *inst)
if (inst->fmt_cap->pixfmt == V4L2_PIX_FMT_H264) {
struct hfi_h264_vui_timing_info info;
struct hfi_h264_entropy_control entropy;
+ struct hfi_h264_db_control deblock;
ptype = HFI_PROPERTY_PARAM_VENC_H264_VUI_TIMING_INFO;
info.enable = 1;
@@ -661,6 +672,17 @@ static int venc_set_properties(struct venus_inst *inst)
ret = hfi_session_set_property(inst, ptype, &entropy);
if (ret)
return ret;
+
+ ptype = HFI_PROPERTY_PARAM_VENC_H264_DEBLOCK_CONTROL;
+ deblock.mode = venc_v4l2_to_hfi(
+ V4L2_CID_MPEG_VIDEO_H264_LOOP_FILTER_MODE,
+ ctr->h264_loop_filter_mode);
+ deblock.slice_alpha_offset = ctr->h264_loop_filter_alpha;
+ deblock.slice_beta_offset = ctr->h264_loop_filter_beta;
+
+ ret = hfi_session_set_property(inst, ptype, &deblock);
+ if (ret)
+ return ret;
}
ptype = HFI_PROPERTY_CONFIG_VENC_IDR_PERIOD;
--
2.7.4
