mbox series

[v3,00/14] staging: media: zoran: fusion in one module

Message ID 20211026193416.1176797-1-clabbe@baylibre.com (mailing list archive)
Headers show
Series staging: media: zoran: fusion in one module | expand

Message

Corentin LABBE Oct. 26, 2021, 7:34 p.m. UTC
Hello

The main change of this serie is to fusion all zoran related modules in
one.
This fixes the load order problem when everything is built-in.

Regards

Changes since v2:
- added the 4 latest patchs
- removed DEBUGFS kconfig option
- fixed Dan Carpenter's reported codec issues
- fixed kernel test robot's reported issues on vb2_dma_contig_set_max_seg_size()

Changes since v1:
- add missing debugfs cleaning
- clean some remaining module_get/put functions which made impossible to
  remove the zoran module
- added the two latest patchs

Corentin Labbe (14):
  staging: media: zoran: move module parameter checks to zoran_probe
  staging: media: zoran: use module_pci_driver
  staging: media: zoran: rename debug module parameter
  staging: media: zoran: add debugfs
  staging: media: zoran: videocode: remove procfs
  staging: media: zoran: fusion all modules
  staging: media: zoran: remove vidmem
  staging: media: zoran: move videodev alloc
  staging: media: zoran: move config select on primary kconfig
  staging: media: zoran: introduce zoran_i2c_init
  staging: media: zoran: fix usage of vb2_dma_contig_set_max_seg_size
  staging: media: zoran: clean unused code
  staging: media: zoran: fix counting buffer in reserve
  staging: media: zoran: DC30 encoder is not adv7175

 drivers/staging/media/zoran/Kconfig        |  38 +-
 drivers/staging/media/zoran/Makefile       |   8 +-
 drivers/staging/media/zoran/videocodec.c   |  68 +---
 drivers/staging/media/zoran/videocodec.h   |   4 +-
 drivers/staging/media/zoran/zoran.h        |  18 +-
 drivers/staging/media/zoran/zoran_card.c   | 400 +++++++++++++--------
 drivers/staging/media/zoran/zoran_device.h |   2 -
 drivers/staging/media/zoran/zoran_driver.c |   8 +-
 drivers/staging/media/zoran/zr36016.c      |  25 +-
 drivers/staging/media/zoran/zr36016.h      |   2 +
 drivers/staging/media/zoran/zr36050.c      |  24 +-
 drivers/staging/media/zoran/zr36050.h      |   2 +
 drivers/staging/media/zoran/zr36060.c      |  23 +-
 drivers/staging/media/zoran/zr36060.h      |   2 +
 14 files changed, 317 insertions(+), 307 deletions(-)

Comments

Hans Verkuil Nov. 3, 2021, 3:21 p.m. UTC | #1
Hi Corentin,

On 26/10/2021 21:34, Corentin Labbe wrote:
> Hello
> 
> The main change of this serie is to fusion all zoran related modules in
> one.
> This fixes the load order problem when everything is built-in.

I've been testing this series, and while the module load/unload is now working,
I'm running into a lot of other v4l2 compliance issues.

I've fixed various issues in some follow-up patches available in my tree:

https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=zoran

At least some of the worst offenders are now resolved. Note that the patch
dropping read/write support relies on this patch:

https://patchwork.linuxtv.org/project/linux-media/patch/4f89b139-13b7-eee6-9662-996626b778b0@xs4all.nl/

But there is one really major bug that makes me hesitant to merge this:

This works:

v4l2-ctl -v pixelformat=MJPG,width=768,height=576
v4l2-ctl --stream-mmap

This fails:

v4l2-ctl -v pixelformat=MJPG,width=768,height=288
v4l2-ctl --stream-mmap

It's an immediate lock up with nothing to indicate what is wrong.
As soon as the height is 288 or less, this happens.

Both with my DC30 and DC30D.

Do you see the same? Any idea what is going on? I would feel much happier
if this is fixed.

Note that the same problem is present without this patch series, so it's
been there for some time.

Regards,

	Hans

> 
> Regards
> 
> Changes since v2:
> - added the 4 latest patchs
> - removed DEBUGFS kconfig option
> - fixed Dan Carpenter's reported codec issues
> - fixed kernel test robot's reported issues on vb2_dma_contig_set_max_seg_size()
> 
> Changes since v1:
> - add missing debugfs cleaning
> - clean some remaining module_get/put functions which made impossible to
>   remove the zoran module
> - added the two latest patchs
> 
> Corentin Labbe (14):
>   staging: media: zoran: move module parameter checks to zoran_probe
>   staging: media: zoran: use module_pci_driver
>   staging: media: zoran: rename debug module parameter
>   staging: media: zoran: add debugfs
>   staging: media: zoran: videocode: remove procfs
>   staging: media: zoran: fusion all modules
>   staging: media: zoran: remove vidmem
>   staging: media: zoran: move videodev alloc
>   staging: media: zoran: move config select on primary kconfig
>   staging: media: zoran: introduce zoran_i2c_init
>   staging: media: zoran: fix usage of vb2_dma_contig_set_max_seg_size
>   staging: media: zoran: clean unused code
>   staging: media: zoran: fix counting buffer in reserve
>   staging: media: zoran: DC30 encoder is not adv7175
> 
>  drivers/staging/media/zoran/Kconfig        |  38 +-
>  drivers/staging/media/zoran/Makefile       |   8 +-
>  drivers/staging/media/zoran/videocodec.c   |  68 +---
>  drivers/staging/media/zoran/videocodec.h   |   4 +-
>  drivers/staging/media/zoran/zoran.h        |  18 +-
>  drivers/staging/media/zoran/zoran_card.c   | 400 +++++++++++++--------
>  drivers/staging/media/zoran/zoran_device.h |   2 -
>  drivers/staging/media/zoran/zoran_driver.c |   8 +-
>  drivers/staging/media/zoran/zr36016.c      |  25 +-
>  drivers/staging/media/zoran/zr36016.h      |   2 +
>  drivers/staging/media/zoran/zr36050.c      |  24 +-
>  drivers/staging/media/zoran/zr36050.h      |   2 +
>  drivers/staging/media/zoran/zr36060.c      |  23 +-
>  drivers/staging/media/zoran/zr36060.h      |   2 +
>  14 files changed, 317 insertions(+), 307 deletions(-)
>
Corentin LABBE Nov. 3, 2021, 3:57 p.m. UTC | #2
Le Wed, Nov 03, 2021 at 04:21:02PM +0100, Hans Verkuil a écrit :
> Hi Corentin,
> 
> On 26/10/2021 21:34, Corentin Labbe wrote:
> > Hello
> > 
> > The main change of this serie is to fusion all zoran related modules in
> > one.
> > This fixes the load order problem when everything is built-in.
> 
> I've been testing this series, and while the module load/unload is now working,
> I'm running into a lot of other v4l2 compliance issues.
> 
> I've fixed various issues in some follow-up patches available in my tree:
> 
> https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=zoran
> 
> At least some of the worst offenders are now resolved. Note that the patch
> dropping read/write support relies on this patch:
> 
> https://patchwork.linuxtv.org/project/linux-media/patch/4f89b139-13b7-eee6-9662-996626b778b0@xs4all.nl/

Hello

My test branch already included your "zoran: fix various V4L2 compliance errors"
I have quickly checked other patch and I am ok with them.
I will add and test with them.

> 
> But there is one really major bug that makes me hesitant to merge this:
> 
> This works:
> 
> v4l2-ctl -v pixelformat=MJPG,width=768,height=576
> v4l2-ctl --stream-mmap
> 
> This fails:
> 
> v4l2-ctl -v pixelformat=MJPG,width=768,height=288
> v4l2-ctl --stream-mmap
> 
> It's an immediate lock up with nothing to indicate what is wrong.
> As soon as the height is 288 or less, this happens.
> 
> Both with my DC30 and DC30D.

Just for curiosity, what is the difference between thoses two ?

> 
> Do you see the same? Any idea what is going on? I would feel much happier
> if this is fixed.
> 
> Note that the same problem is present without this patch series, so it's
> been there for some time.
> 

I will start on digging this problem and add thoses commands to my CI.
And I know there are a huge quantity of problem since origins.
A simple example is that just setting MJPEG as default input format does not work.

But since it is not related to my serie, can you please merge it.

Thanks
Regards
Hans Verkuil Nov. 3, 2021, 4:29 p.m. UTC | #3
On 03/11/2021 16:57, LABBE Corentin wrote:
> Le Wed, Nov 03, 2021 at 04:21:02PM +0100, Hans Verkuil a écrit :
>> Hi Corentin,
>>
>> On 26/10/2021 21:34, Corentin Labbe wrote:
>>> Hello
>>>
>>> The main change of this serie is to fusion all zoran related modules in
>>> one.
>>> This fixes the load order problem when everything is built-in.
>>
>> I've been testing this series, and while the module load/unload is now working,
>> I'm running into a lot of other v4l2 compliance issues.
>>
>> I've fixed various issues in some follow-up patches available in my tree:
>>
>> https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=zoran
>>
>> At least some of the worst offenders are now resolved. Note that the patch
>> dropping read/write support relies on this patch:
>>
>> https://patchwork.linuxtv.org/project/linux-media/patch/4f89b139-13b7-eee6-9662-996626b778b0@xs4all.nl/
> 
> Hello
> 
> My test branch already included your "zoran: fix various V4L2 compliance errors"
> I have quickly checked other patch and I am ok with them.
> I will add and test with them.
> 
>>
>> But there is one really major bug that makes me hesitant to merge this:
>>
>> This works:
>>
>> v4l2-ctl -v pixelformat=MJPG,width=768,height=576
>> v4l2-ctl --stream-mmap
>>
>> This fails:
>>
>> v4l2-ctl -v pixelformat=MJPG,width=768,height=288
>> v4l2-ctl --stream-mmap
>>
>> It's an immediate lock up with nothing to indicate what is wrong.
>> As soon as the height is 288 or less, this happens.
>>
>> Both with my DC30 and DC30D.
> 
> Just for curiosity, what is the difference between thoses two ?

It's the DC30 variant without an adv7175.

> 
>>
>> Do you see the same? Any idea what is going on? I would feel much happier
>> if this is fixed.
>>
>> Note that the same problem is present without this patch series, so it's
>> been there for some time.
>>
> 
> I will start on digging this problem and add thoses commands to my CI.
> And I know there are a huge quantity of problem since origins.
> A simple example is that just setting MJPEG as default input format does not work.
> 
> But since it is not related to my serie, can you please merge it.

Before I do that, I would really like to know a bit more about this issue:
can you reproduce it? Is it DC30 specific or a general problem with zoran?

The problem with this hard hang is that it is hard to do regression testing
with v4l2-compliance, since it will hang as soon as MJPG pixelformat is
tested.

I would feel much happier if the hang can be avoided, even if it is just
with a temporary hack. It will make it much easier going forward.

Regards,

	Hans
Corentin LABBE Nov. 5, 2021, 2:53 p.m. UTC | #4
Le Wed, Nov 03, 2021 at 05:29:46PM +0100, Hans Verkuil a écrit :
> On 03/11/2021 16:57, LABBE Corentin wrote:
> > Le Wed, Nov 03, 2021 at 04:21:02PM +0100, Hans Verkuil a écrit :
> >> Hi Corentin,
> >>
> >> On 26/10/2021 21:34, Corentin Labbe wrote:
> >>> Hello
> >>>
> >>> The main change of this serie is to fusion all zoran related modules in
> >>> one.
> >>> This fixes the load order problem when everything is built-in.
> >>
> >> I've been testing this series, and while the module load/unload is now working,
> >> I'm running into a lot of other v4l2 compliance issues.
> >>
> >> I've fixed various issues in some follow-up patches available in my tree:
> >>
> >> https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=zoran
> >>
> >> At least some of the worst offenders are now resolved. Note that the patch
> >> dropping read/write support relies on this patch:
> >>
> >> https://patchwork.linuxtv.org/project/linux-media/patch/4f89b139-13b7-eee6-9662-996626b778b0@xs4all.nl/
> > 
> > Hello
> > 
> > My test branch already included your "zoran: fix various V4L2 compliance errors"
> > I have quickly checked other patch and I am ok with them.
> > I will add and test with them.
> > 
> >>
> >> But there is one really major bug that makes me hesitant to merge this:
> >>
> >> This works:
> >>
> >> v4l2-ctl -v pixelformat=MJPG,width=768,height=576
> >> v4l2-ctl --stream-mmap
> >>
> >> This fails:
> >>
> >> v4l2-ctl -v pixelformat=MJPG,width=768,height=288
> >> v4l2-ctl --stream-mmap
> >>
> >> It's an immediate lock up with nothing to indicate what is wrong.
> >> As soon as the height is 288 or less, this happens.
> >>
> >> Both with my DC30 and DC30D.
> > 
> > Just for curiosity, what is the difference between thoses two ?
> 
> It's the DC30 variant without an adv7175.

So my patch removing adv7175 from DC30 is wrong.
I need to add a new DC30D.

> 
> > 
> >>
> >> Do you see the same? Any idea what is going on? I would feel much happier
> >> if this is fixed.
> >>
> >> Note that the same problem is present without this patch series, so it's
> >> been there for some time.
> >>
> > 
> > I will start on digging this problem and add thoses commands to my CI.
> > And I know there are a huge quantity of problem since origins.
> > A simple example is that just setting MJPEG as default input format does not work.
> > 
> > But since it is not related to my serie, can you please merge it.
> 
> Before I do that, I would really like to know a bit more about this issue:
> can you reproduce it? Is it DC30 specific or a general problem with zoran?
> 
> The problem with this hard hang is that it is hard to do regression testing
> with v4l2-compliance, since it will hang as soon as MJPG pixelformat is
> tested.
> 
> I would feel much happier if the hang can be avoided, even if it is just
> with a temporary hack. It will make it much easier going forward.
> 

I hit the same problem with my DC10+.
I got the following trace:
[   97.022391] BUG: kernel NULL pointer dereference, address: 0000000000000018
[   97.029357] #PF: supervisor write access in kernel mode
[   97.034579] #PF: error_code(0x0002) - not-present page
[   97.039712] PGD 100e30067 P4D 100e30067 PUD 11c958067 PMD 0 
[   97.045370] Oops: 0002 [#1] PREEMPT SMP NOPTI
[   97.049723] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G         C        5.15.0-next-20211105+ #126
[   97.058500] Hardware name: To Be Filled By O.E.M. To Be Filled By O.E.M./K10N78, BIOS P2.00 07/01/2010
[   97.067791] RIP: 0010:zoran_irq+0x178/0x2e0 [zr36067]
[   97.072845] Code: 01 8d 5c 00 01 48 8b 85 90 0c 00 00 48 63 db 44 8b 2c 98 41 f6 c5 01 0f 84 64 01 00 00 4c 8b bc dd 38 0d 00 00 e8 98 72 a1 fa <49> 89 47 18 83 bd 90 0b 00 00 01 0f 84 da 00 00 00 48 8b 85 68 0c
[   97.091590] RSP: 0018:ffffa57040003f00 EFLAGS: 00010016
[   97.096807] RAX: 000000169273a7d2 RBX: 0000000000000001 RCX: 0000000000000018
[   97.103932] RDX: 000000830c927d90 RSI: 000000000000d33a RDI: 00041965ba87a734
[   97.111063] RBP: ffff9d845cce1028 R08: 00000000005b6db7 R09: 0000000000000000
[   97.118188] R10: 0000000000000000 R11: ffffa57040003ff8 R12: 0000000000000065
[   97.125312] R13: 0000000004027541 R14: ffff9d845cce1d58 R15: 0000000000000000
[   97.132434] FS:  0000000000000000(0000) GS:ffff9d845fc00000(0000) knlGS:0000000000000000
[   97.140513] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   97.146249] CR2: 0000000000000018 CR3: 000000011c956000 CR4: 00000000000006f0
[   97.153374] Call Trace:
[   97.155822]  <IRQ>
[   97.157840]  __handle_irq_event_percpu+0x35/0x180
[   97.162546]  handle_irq_event+0x50/0xb0
[   97.166384]  handle_fasteoi_irq+0x8b/0x1e0
[   97.170483]  __common_interrupt+0x64/0x100
[   97.174581]  common_interrupt+0x9f/0xc0
[   97.178414]  </IRQ>
[   97.180510]  <TASK>
[   97.182609]  asm_common_interrupt+0x1e/0x40
[   97.186793] RIP: 0010:acpi_idle_do_entry+0x47/0x50
[   97.191585] Code: 08 48 8b 15 1f db 5d 01 ed c3 e9 64 fd ff ff 65 48 8b 04 25 00 ad 01 00 48 8b 00 a8 08 75 ea eb 07 0f 00 2d 0b 80 5b 00 fb f4 <fa> c3 cc cc cc cc cc cc cc 41 56 49 89 f6 41 55 41 89 d5 41 54 55
[   97.210323] RSP: 0018:ffffffffbc603e30 EFLAGS: 00000246
[   97.215539] RAX: 0000000000004000 RBX: ffff9d84413f1c00 RCX: 000000000000001f
[   97.222666] RDX: ffff9d845fc00000 RSI: ffff9d8440165000 RDI: ffff9d8440165064
[   97.229798] RBP: ffff9d8440165064 R08: 000000000001184d R09: 0000000000000018
[   97.236921] R10: 00000000000029cc R11: 000000000000318c R12: 0000000000000001
[   97.244046] R13: ffffffffbc7c3e20 R14: 0000000000000001 R15: 0000000000000000
[   97.251178]  acpi_idle_enter+0x99/0xe0
[   97.254931]  cpuidle_enter_state+0x84/0x360
[   97.259118]  cpuidle_enter+0x24/0x40
[   97.262698]  do_idle+0x1d0/0x250
[   97.265928]  cpu_startup_entry+0x14/0x20
[   97.269846]  start_kernel+0x63a/0x65f
[   97.273514]  secondary_startup_64_no_verify+0xc2/0xcb
[   97.278565]  </TASK>
[   97.280748] Modules linked in: adv7175 saa7110 zr36067(C) videobuf2_dma_contig
[   97.287970] CR2: 0000000000000018
[   97.291279] ---[ end trace 0ee22c5269015e89 ]---
[   97.295888] RIP: 0010:zoran_irq+0x178/0x2e0 [zr36067]
[   97.300941] Code: 01 8d 5c 00 01 48 8b 85 90 0c 00 00 48 63 db 44 8b 2c 98 41 f6 c5 01 0f 84 64 01 00 00 4c 8b bc dd 38 0d 00 00 e8 98 72 a1 fa <49> 89 47 18 83 bd 90 0b 00 00 01 0f 84 da 00 00 00 48 8b 85 68 0c
[   97.319679] RSP: 0018:ffffa57040003f00 EFLAGS: 00010016
[   97.324896] RAX: 000000169273a7d2 RBX: 0000000000000001 RCX: 0000000000000018
[   97.332019] RDX: 000000830c927d90 RSI: 000000000000d33a RDI: 00041965ba87a734
[   97.339144] RBP: ffff9d845cce1028 R08: 00000000005b6db7 R09: 0000000000000000
[   97.346276] R10: 0000000000000000 R11: ffffa57040003ff8 R12: 0000000000000065
[   97.353401] R13: 0000000004027541 R14: ffff9d845cce1d58 R15: 0000000000000000
[   97.360525] FS:  0000000000000000(0000) GS:ffff9d845fc00000(0000) knlGS:0000000000000000
[   97.368603] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   97.374340] CR2: 0000000000000018 CR3: 000000011c956000 CR4: 00000000000006f0
[   97.381464] Kernel panic - not syncing: Fatal exception in interrupt
[   97.387810] Kernel Offset: 0x39c00000 from 0xffffffff81000000 (relocation range: 0xffffffff80000000-0xffffffffbfffffff)
[   97.398580] ---[ end Kernel panic - not syncing: Fatal exception in interrupt ]---

x86_64-pc-linux-gnu-addr2line -e vmlinux zoran_irq+0x16f/0x2e0
/usr/src/linux-next/arch/x86/include/asm/processor.h:443

I have no more clue for the moment.
Corentin LABBE Nov. 7, 2021, 4:35 p.m. UTC | #5
Le Wed, Nov 03, 2021 at 05:29:46PM +0100, Hans Verkuil a écrit :
> On 03/11/2021 16:57, LABBE Corentin wrote:
> > Le Wed, Nov 03, 2021 at 04:21:02PM +0100, Hans Verkuil a écrit :
> >> Hi Corentin,
> >>
> >> On 26/10/2021 21:34, Corentin Labbe wrote:
> >>> Hello
> >>>
> >>> The main change of this serie is to fusion all zoran related modules in
> >>> one.
> >>> This fixes the load order problem when everything is built-in.
> >>
> >> I've been testing this series, and while the module load/unload is now working,
> >> I'm running into a lot of other v4l2 compliance issues.
> >>
> >> I've fixed various issues in some follow-up patches available in my tree:
> >>
> >> https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=zoran
> >>
> >> At least some of the worst offenders are now resolved. Note that the patch
> >> dropping read/write support relies on this patch:
> >>
> >> https://patchwork.linuxtv.org/project/linux-media/patch/4f89b139-13b7-eee6-9662-996626b778b0@xs4all.nl/
> > 
> > Hello
> > 
> > My test branch already included your "zoran: fix various V4L2 compliance errors"
> > I have quickly checked other patch and I am ok with them.
> > I will add and test with them.
> > 
> >>
> >> But there is one really major bug that makes me hesitant to merge this:
> >>
> >> This works:
> >>
> >> v4l2-ctl -v pixelformat=MJPG,width=768,height=576
> >> v4l2-ctl --stream-mmap
> >>
> >> This fails:
> >>
> >> v4l2-ctl -v pixelformat=MJPG,width=768,height=288
> >> v4l2-ctl --stream-mmap
> >>
> >> It's an immediate lock up with nothing to indicate what is wrong.
> >> As soon as the height is 288 or less, this happens.
> >>
> >> Both with my DC30 and DC30D.
> > 
> > Just for curiosity, what is the difference between thoses two ?
> 
> It's the DC30 variant without an adv7175.
> 
> > 
> >>
> >> Do you see the same? Any idea what is going on? I would feel much happier
> >> if this is fixed.
> >>
> >> Note that the same problem is present without this patch series, so it's
> >> been there for some time.
> >>
> > 
> > I will start on digging this problem and add thoses commands to my CI.
> > And I know there are a huge quantity of problem since origins.
> > A simple example is that just setting MJPEG as default input format does not work.
> > 
> > But since it is not related to my serie, can you please merge it.
> 
> Before I do that, I would really like to know a bit more about this issue:
> can you reproduce it? Is it DC30 specific or a general problem with zoran?
> 
> The problem with this hard hang is that it is hard to do regression testing
> with v4l2-compliance, since it will hang as soon as MJPG pixelformat is
> tested.
> 
> I would feel much happier if the hang can be avoided, even if it is just
> with a temporary hack. It will make it much easier going forward.
> 

I found the bug

The null pointer deref was in zoran_reap_stat_com() due to 
buf = zr->inuse[i];
...
buf->vbuf.vb2_buf.timestamp = ktime_get_ns();
with buf = NULL;

It is due to miscalculation of "i".

I will resend my serie with the fix for that.

Regards
Hans Verkuil Nov. 8, 2021, 8:21 a.m. UTC | #6
On 07/11/2021 17:35, LABBE Corentin wrote:
> Le Wed, Nov 03, 2021 at 05:29:46PM +0100, Hans Verkuil a écrit :
>> On 03/11/2021 16:57, LABBE Corentin wrote:
>>> Le Wed, Nov 03, 2021 at 04:21:02PM +0100, Hans Verkuil a écrit :
>>>> Hi Corentin,
>>>>
>>>> On 26/10/2021 21:34, Corentin Labbe wrote:
>>>>> Hello
>>>>>
>>>>> The main change of this serie is to fusion all zoran related modules in
>>>>> one.
>>>>> This fixes the load order problem when everything is built-in.
>>>>
>>>> I've been testing this series, and while the module load/unload is now working,
>>>> I'm running into a lot of other v4l2 compliance issues.
>>>>
>>>> I've fixed various issues in some follow-up patches available in my tree:
>>>>
>>>> https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=zoran
>>>>
>>>> At least some of the worst offenders are now resolved. Note that the patch
>>>> dropping read/write support relies on this patch:
>>>>
>>>> https://patchwork.linuxtv.org/project/linux-media/patch/4f89b139-13b7-eee6-9662-996626b778b0@xs4all.nl/
>>>
>>> Hello
>>>
>>> My test branch already included your "zoran: fix various V4L2 compliance errors"
>>> I have quickly checked other patch and I am ok with them.
>>> I will add and test with them.
>>>
>>>>
>>>> But there is one really major bug that makes me hesitant to merge this:
>>>>
>>>> This works:
>>>>
>>>> v4l2-ctl -v pixelformat=MJPG,width=768,height=576
>>>> v4l2-ctl --stream-mmap
>>>>
>>>> This fails:
>>>>
>>>> v4l2-ctl -v pixelformat=MJPG,width=768,height=288
>>>> v4l2-ctl --stream-mmap
>>>>
>>>> It's an immediate lock up with nothing to indicate what is wrong.
>>>> As soon as the height is 288 or less, this happens.
>>>>
>>>> Both with my DC30 and DC30D.
>>>
>>> Just for curiosity, what is the difference between thoses two ?
>>
>> It's the DC30 variant without an adv7175.
>>
>>>
>>>>
>>>> Do you see the same? Any idea what is going on? I would feel much happier
>>>> if this is fixed.
>>>>
>>>> Note that the same problem is present without this patch series, so it's
>>>> been there for some time.
>>>>
>>>
>>> I will start on digging this problem and add thoses commands to my CI.
>>> And I know there are a huge quantity of problem since origins.
>>> A simple example is that just setting MJPEG as default input format does not work.
>>>
>>> But since it is not related to my serie, can you please merge it.
>>
>> Before I do that, I would really like to know a bit more about this issue:
>> can you reproduce it? Is it DC30 specific or a general problem with zoran?
>>
>> The problem with this hard hang is that it is hard to do regression testing
>> with v4l2-compliance, since it will hang as soon as MJPG pixelformat is
>> tested.
>>
>> I would feel much happier if the hang can be avoided, even if it is just
>> with a temporary hack. It will make it much easier going forward.
>>
> 
> I found the bug
> 
> The null pointer deref was in zoran_reap_stat_com() due to 
> buf = zr->inuse[i];
> ...
> buf->vbuf.vb2_buf.timestamp = ktime_get_ns();
> with buf = NULL;
> 
> It is due to miscalculation of "i".
> 
> I will resend my serie with the fix for that.

Excellent news! Thank you for tracking this one down.

When you post your series, can you include my patches from
https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=zoran as well?

Regards,

	Hans
Corentin LABBE Nov. 16, 2021, 2:12 p.m. UTC | #7
Le Mon, Nov 08, 2021 at 09:21:22AM +0100, Hans Verkuil a écrit :
> On 07/11/2021 17:35, LABBE Corentin wrote:
> > Le Wed, Nov 03, 2021 at 05:29:46PM +0100, Hans Verkuil a écrit :
> >> On 03/11/2021 16:57, LABBE Corentin wrote:
> >>> Le Wed, Nov 03, 2021 at 04:21:02PM +0100, Hans Verkuil a écrit :
> >>>> Hi Corentin,
> >>>>
> >>>> On 26/10/2021 21:34, Corentin Labbe wrote:
> >>>>> Hello
> >>>>>
> >>>>> The main change of this serie is to fusion all zoran related modules in
> >>>>> one.
> >>>>> This fixes the load order problem when everything is built-in.
> >>>>
> >>>> I've been testing this series, and while the module load/unload is now working,
> >>>> I'm running into a lot of other v4l2 compliance issues.
> >>>>
> >>>> I've fixed various issues in some follow-up patches available in my tree:
> >>>>
> >>>> https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=zoran
> >>>>
> >>>> At least some of the worst offenders are now resolved. Note that the patch
> >>>> dropping read/write support relies on this patch:
> >>>>
> >>>> https://patchwork.linuxtv.org/project/linux-media/patch/4f89b139-13b7-eee6-9662-996626b778b0@xs4all.nl/
> >>>
> >>> Hello
> >>>
> >>> My test branch already included your "zoran: fix various V4L2 compliance errors"
> >>> I have quickly checked other patch and I am ok with them.
> >>> I will add and test with them.
> >>>
> >>>>
> >>>> But there is one really major bug that makes me hesitant to merge this:
> >>>>
> >>>> This works:
> >>>>
> >>>> v4l2-ctl -v pixelformat=MJPG,width=768,height=576
> >>>> v4l2-ctl --stream-mmap
> >>>>
> >>>> This fails:
> >>>>
> >>>> v4l2-ctl -v pixelformat=MJPG,width=768,height=288
> >>>> v4l2-ctl --stream-mmap
> >>>>
> >>>> It's an immediate lock up with nothing to indicate what is wrong.
> >>>> As soon as the height is 288 or less, this happens.
> >>>>
> >>>> Both with my DC30 and DC30D.
> >>>
> >>> Just for curiosity, what is the difference between thoses two ?
> >>
> >> It's the DC30 variant without an adv7175.
> >>
> >>>
> >>>>
> >>>> Do you see the same? Any idea what is going on? I would feel much happier
> >>>> if this is fixed.
> >>>>
> >>>> Note that the same problem is present without this patch series, so it's
> >>>> been there for some time.
> >>>>
> >>>
> >>> I will start on digging this problem and add thoses commands to my CI.
> >>> And I know there are a huge quantity of problem since origins.
> >>> A simple example is that just setting MJPEG as default input format does not work.
> >>>
> >>> But since it is not related to my serie, can you please merge it.
> >>
> >> Before I do that, I would really like to know a bit more about this issue:
> >> can you reproduce it? Is it DC30 specific or a general problem with zoran?
> >>
> >> The problem with this hard hang is that it is hard to do regression testing
> >> with v4l2-compliance, since it will hang as soon as MJPG pixelformat is
> >> tested.
> >>
> >> I would feel much happier if the hang can be avoided, even if it is just
> >> with a temporary hack. It will make it much easier going forward.
> >>
> > 
> > I found the bug
> > 
> > The null pointer deref was in zoran_reap_stat_com() due to 
> > buf = zr->inuse[i];
> > ...
> > buf->vbuf.vb2_buf.timestamp = ktime_get_ns();
> > with buf = NULL;
> > 
> > It is due to miscalculation of "i".
> > 
> > I will resend my serie with the fix for that.
> 
> Excellent news! Thank you for tracking this one down.
> 
> When you post your series, can you include my patches from
> https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=zoran as well?
> 

Hello

Yes, I will include them.

Regards