Re: [PATCH v2 00/23] media: ov5640: Rework the clock tree programming for MIPI

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2/21/22 10:51 AM, Jacopo Mondi wrote:
> Hi Eugen
> 
> On Thu, Feb 17, 2022 at 02:25:37PM +0000, Eugen.Hristev@xxxxxxxxxxxxx wrote:
>> On 2/14/22 8:56 PM, Jacopo Mondi wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> Hi Eugen
>>>
>>> On Mon, Feb 14, 2022 at 03:08:56PM +0000, Eugen.Hristev@xxxxxxxxxxxxx wrote:
>>>> On 2/14/22 4:38 PM, Jacopo Mondi wrote:
>>>>> Hi Eugen,
>>>>>
>>>>> On Mon, Feb 14, 2022 at 02:06:02PM +0000, Eugen.Hristev@xxxxxxxxxxxxx wrote:
>>>>>> On 2/11/22 1:25 PM, Jacopo Mondi wrote:
>>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>>>
>>>>>>> Hi Eugen
>>>>>>>
>>>>>>>             thanks very much for testing
>>>>>>>
>>>>>>> On Fri, Feb 11, 2022 at 10:09:04AM +0000, Eugen.Hristev@xxxxxxxxxxxxx wrote:
>>>>>>>> On 2/10/22 1:04 PM, Jacopo Mondi wrote:
>>>>>>>>
>>>>>>>> Hello Jacopo,
>>>>>>>>
>>>>>>>>> v1:
>>>>>>>>> https://patchwork.linuxtv.org/project/linux-media/list/?series=7249
>>>>>>>>>
>>>>>>>>> A branch for testing based on the most recent media-master is available at
>>>>>>>>> https://git.sr.ht/~jmondi_/linux #jmondi/media-master/ov5640-v2
>>>>>>>>>
>>>>>>>>> If anyone with a DVP setup could verify I have not broken their use case
>>>>>>>>> I would very much appreciate that :)
>>>>>>>>
>>>>>>>> I started testing this on my bench.
>>>>>>>> So far things look good.
>>>>>>>>
>>>>>>>
>>>>>>> \o/
>>>>>>>
>>>>>>>> To be able to test this, I have to revert this patch :
>>>>>>>> "media: i2c: ov5640: Remain in power down for DVP mode unless streaming"
>>>>>>>>
>>>>>>>> Otherwise the sensor will not power up when starting streaming.
>>>>>>>>
>>>>>>>>
>>>>>>>> I have tested several formats, as you worked more on this sensor, could
>>>>>>>> you tell me, does format YUYV_2x8 work in parallel mode at 1920x1080 or
>>>>>>>> 1024x768 ?
>>>>>>>
>>>>>>> I never tested the sensor driver with a parallel setup I'm afraid.
>>>>>>> The idea behind this series is that DVP shouldn't be affected and
>>>>>>> continue working like it did.
>>>>>>
>>>>>> Hi Jacopo,
>>>>>>
>>>>>> I was hoping that you had more information about the driver than myself.
>>>>>
>>>>> Not on DVP mode I'm sorry :(
>>>>>
>>>>>> I can tell that the parallel mode is not affected by your series from
>>>>>> what I've seen so far.
>>>>>
>>>>> That's great, thanks for testing.
>>>>
>>>>
>>>> I found one change, before your series, I could attempt to capture BGGR
>>>> @ 640x480, now it looks to be gone :
>>>>
>>>> Before:
>>>>
>>>> # v4l2-ctl -d /dev/v4l-subdev1 --list-subdev-framesizes pad=0,code=0x3001
>>>> ioctl: VIDIOC_SUBDEV_ENUM_FRAME_SIZE (pad=0)
>>>>            Size Range: 160x120 - 160x120
>>>>            Size Range: 176x144 - 176x144
>>>>            Size Range: 320x240 - 320x240
>>>>            Size Range: 640x480 - 640x480
>>>>            Size Range: 720x480 - 720x480
>>>>            Size Range: 720x576 - 720x576
>>>>            Size Range: 1024x768 - 1024x768
>>>>            Size Range: 1280x720 - 1280x720
>>>>            Size Range: 1920x1080 - 1920x1080
>>>>            Size Range: 2592x1944 - 2592x1944
>>>>
>>>>
>>>> After:
>>>>
>>>> # v4l2-ctl -d /dev/v4l-subdev1 --list-subdev-framesizes pad=0,code=0x3001
>>>> ioctl: VIDIOC_SUBDEV_ENUM_FRAME_SIZE (pad=0)
>>>>            Size Range: 1280x720 - 1280x720
>>>>            Size Range: 1920x1080 - 1920x1080
>>>>            Size Range: 2592x1944 - 2592x1944
>>>>
>>>
>>> Right... I'm limiting SRGGB formats to only resolutions > 1280x720
>>> as..
>>>
>>>>
>>>> However trying it without your patches, it doesn't work , I don't
>>>
>>> ... they do not work for me either.
>>>
>>> So if they were not working before, maybe it's right not to enumerate
>>> them ?
>>>
>>>> receive frames, and I can't even set 1280x720 at all, I get -EINVAL
>>>
>>> Be aware that DVP mode prevents you from setting a format if the
>>> currently selected framerate is said to be "not supported" for that
>>> format
>>>
>>>>
>>>> So I assume you have been reworking the frame sizes.
>>>>
>>>> With your series, I have tested RGGB at 1280x720 , 1920x1080 .
>>>> I also tested YUYV at 640x480 and RGB565_LE at 640x480.
>>>>
>>>> I can't go higher with the YUYV/RGB565, I don't receive frames anymore.
>>>
>>> Ah, I wonder if
>>> 07b49ac59fd9 media: ov5640: Fix durations to comply with FPS
>>> 82aebf4b7314 media: ov5640: Rework analog crop rectangles
>>>
>>> Might have impacted DVP...
>>>
>>> Should I keep separate fields for MIPI mode and leave the totals for
>>> DVP untouched ?
>>
>> Hi Jacopo,
>>
>> I tested again without your series, and I can capture YUYV directly
>> @1280x720 , which does not work anymore with your patches.
>>
>> The image has some bad pixels in it, but still, capture is pretty good.
>> I am not sure whether it's a timing problem on capturing pixels, I
>> uploaded it here so you can have a look :
>>
>> https://ibb.co/Yt8c0dJ
>>
> 
> This is without my series ? Or with it applied ? Sorry for the dumb
> question :)

This photo is *without* your series. *With* your series, I cannot 
capture YUYV at this resolution at all. No frames received.

> 
>> Eugen
>>
>>>
>>> Thanks again for your very useful testing
>>>
>>>
>>>> I can't go higher with the bayer to 2592x1944 . But this did not work
>>>> before your patches either.
>>>>>
>>>>>>
>>>>>>>
>>>>>>>> I managed to get it working fine at 640x480 .
>>>>>>>>
>>>>>>>> The sensor looks to report valid framesizes for this mbus code :
>>>>>>>>
>>>>>>>> # v4l2-ctl -d /dev/v4l-subdev1 --list-subdev-mbus-codes
>>>>>>>> \ioctl: VIDIOC_SUBDEV_ENUM_MBUS_CODE (pad=0)
>>>>>>>>              0x4001: MEDIA_BUS_FMT_JPEG_1X8
>>>>>>>>              0x2006: MEDIA_BUS_FMT_UYVY8_2X8
>>>>>>>>              0x200f: MEDIA_BUS_FMT_UYVY8_1X16
>>>>>>>>              0x2008: MEDIA_BUS_FMT_YUYV8_2X8
>>>>>>>>              0x2011: MEDIA_BUS_FMT_YUYV8_1X16
>>>>>>>>              0x1008: MEDIA_BUS_FMT_RGB565_2X8_LE
>>>>>>>>              0x1007: MEDIA_BUS_FMT_RGB565_2X8_BE
>>>>>>>>              0x1017: MEDIA_BUS_FMT_RGB565_1X16
>>>>>>>>              0x100a: MEDIA_BUS_FMT_RGB888_1X24
>>>>>>>>              0x1013: MEDIA_BUS_FMT_BGR888_1X24
>>>>>>>>              0x3001: MEDIA_BUS_FMT_SBGGR8_1X8
>>>>>>>>              0x3013: MEDIA_BUS_FMT_SGBRG8_1X8
>>>>>>>>              0x3002: MEDIA_BUS_FMT_SGRBG8_1X8
>>>>>>>>              0x3014: MEDIA_BUS_FMT_SRGGB8_1X8
>>>>>>>> # v4l2-ctl -d /dev/v4l-subdev1 --list-subdev-framesizes pad=0,code=0x2008
>>>>>>>> ioctl: VIDIOC_SUBDEV_ENUM_FRAME_SIZE (pad=0)
>>>>>>>>              Size Range: 160x120 - 160x120
>>>>>>>>              Size Range: 176x144 - 176x144
>>>>>>>>              Size Range: 320x240 - 320x240
>>>>>>>>              Size Range: 640x480 - 640x480
>>>>>>>>              Size Range: 720x480 - 720x480
>>>>>>>>              Size Range: 720x576 - 720x576
>>>>>>>>              Size Range: 1024x768 - 1024x768
>>>>>>>>              Size Range: 1280x720 - 1280x720
>>>>>>>>              Size Range: 1920x1080 - 1920x1080
>>>>>>>>              Size Range: 2592x1944 - 2592x1944
>>>>>>>> #
>>>>>>>>
>>>>>>>> but the ISC does not receive any frames at 1024x768 and 1920x1080.
>>>>>>>
>>>>>>> Are 1080p and 1024x768 working without this series applied on your
>>>>>>> setup ?
>>>>>>
>>>>>> I remember they weren't working before either.
>>>>>>
>>>>>> I don't know exactly to which patches to add my Tested-by , as I have
>>>>>> not tested the explicit patch behavior for each patch (e.g. where you
>>>>>> add HBLANK control, I have not tested that ).
>>>>>>
>>>>>
>>>>> I think it's good enough making sure I have not broke DVP.
>>>>>
>>>>> As you can see the driver now behaves in two different ways in DVP and
>>>>> MIPI mode. The DVP works as it used to, and the framerate is
>>>>> controlled by s_frame_interval, the frame size is fixed and the PCLK
>>>>> is computed dyanically to accomodate the required FPS. In MIPI mode the
>>>>> framerate is controlled by changing the vertical blankings. To each
>>>>> mode a pixel rate is assigned and userspace changes the frame duration
>>>>> by changing blankings. The most obvious gain is that the frame rate is
>>>>> controllable in a continuous interval instead of being limited to a
>>>>> few fixed values. It is my understanding that all drivers should be
>>>>> moved to this model and s_frame_intervall should be dropped, but
>>>>> unfortunately some bridge drivers in mainline still deman that.
>>>>>
>>>>> If you are interested, I think the DVP mode should be moved to use the
>>>>> same mecahnism as MIPI mode. I can't test so I haven't done so in this
>>>>> series.
>>>>>
>>>>> For now I think it's enough to make sure there are no regressions in
>>>>> DVP mode.
>>>>>
>>>>> For the tag, I think it would be appropriate to add it to the
>>>>> following one:
>>>>>
>>>>> 91ae667b0d47 media: ov5640: Limit frame_interval to DVP mode only
>>>>>
>>>>>> Is there something particular you would like me to try , except my usual
>>>>>> image captures ?
>>>>>
>>>>> RGB888 is weird on both the platforms I've tested on and I cannot get
>>>>> colors right. Does your platform support RGB888 ?
>>>>
>>>> I can't test this one unfortunately. It's a 1X24 . I have only 8 bits
>>>> connected, so unless you can make this a 3x8 , there isn't anything I
>>>> can do.
>>>>
>>>>>
>>>>> Thanks
>>>>>      j
>>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>> Eugen
>>>>>>
>>>>>>>
>>>>>>> Thanks again for testin!
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> What I can say is that the raw bayer format works at 1920x1080 , frames
>>>>>>>> are received correctly.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Eugen
>>>>>>>>
>>>>>>>>>
>>>>>>>>> v1 -> v2:
>>>>>>>>> - rework the modes definition to process the full pixel array
>>>>>>>>> - rework get_selection to report the correct BOUND and DEFAULT targets
>>>>>>>>> - implement init_cfg
>>>>>>>>> - minor style changes as suggested by Laurent
>>>>>>>>> - test with 1 data lane
>>>>>>>>>
>>>>>>>>> Thanks
>>>>>>>>>         j
>>>>>>>>>
>>>>>>>>> Jacopo Mondi (23):
>>>>>>>>>        media: ov5640: Add pixel rate to modes
>>>>>>>>>        media: ov5604: Re-arrange modes definition
>>>>>>>>>        media: ov5640: Add ov5640_is_csi2() function
>>>>>>>>>        media: ov5640: Associate bpp with formats
>>>>>>>>>        media: ov5640: Add LINK_FREQ control
>>>>>>>>>        media: ov5640: Update pixel_rate and link_freq
>>>>>>>>>        media: ov5640: Rework CSI-2 clock tree
>>>>>>>>>        media: ov5640: Rework timings programming
>>>>>>>>>        media: ov5640: Fix 720x480 in RGB888 mode
>>>>>>>>>        media: ov5640: Rework analog crop rectangles
>>>>>>>>>        media: ov5640: Re-sort per-mode register tables
>>>>>>>>>        media: ov5640: Remove ov5640_mode_init_data
>>>>>>>>>        media: ov5640: Add HBLANK control
>>>>>>>>>        media: ov5640: Add VBLANK control
>>>>>>>>>        media: ov5640: Fix durations to comply with FPS
>>>>>>>>>        media: ov5640: Implement init_cfg
>>>>>>>>>        media: ov5640: Implement get_selection
>>>>>>>>>        media: ov5640: Limit frame_interval to DVP mode only
>>>>>>>>>        media: ov5640: Register device properties
>>>>>>>>>        media: ov5640: Add RGB565_1X16 format
>>>>>>>>>        media: ov5640: Add RGB888/BGR888 formats
>>>>>>>>>        media: ov5640: Restrict sizes to mbus code
>>>>>>>>>        media: ov5640: Adjust format to bpp in s_fmt
>>>>>>>>>
>>>>>>>>>       drivers/media/i2c/ov5640.c | 1143 ++++++++++++++++++++++++++----------
>>>>>>>>>       1 file changed, 830 insertions(+), 313 deletions(-)
>>>>>>>>>
>>>>>>>>> --
>>>>>>>>> 2.35.0
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>





[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux