Linux-Media Archive on lore.kernel.org
 help / color / Atom feed
From: Shuah Khan <skhan@linuxfoundation.org>
To: "Helen Koike" <helen.koike@collabora.com>,
	"André Almeida" <andrealmeid@collabora.com>,
	mchehab@kernel.org, hverkuil@xs4all.nl,
	laurent.pinchart@ideasonboard.com
Cc: linux-kernel@vger.kernel.org, linux-media@vger.kernel.org,
	kernel@collabora.com, Shuah Khan <skhan@linuxfoundation.org>
Subject: Re: [PATCH 0/3] Collapse vimc into single monolithic driver
Date: Mon, 12 Aug 2019 08:08:18 -0600
Message-ID: <2ee23903-8e99-a0a0-619a-be5bdaa71802@linuxfoundation.org> (raw)
In-Reply-To: <e21d38a5-4fcd-7b02-f5f2-e445c280f769@collabora.com>

On 8/9/19 9:51 PM, Helen Koike wrote:
> Hi Andre,
> 
> Thanks for testing this.
> 
> On 8/9/19 9:24 PM, André Almeida wrote:
>> On 8/9/19 9:17 PM, Shuah Khan wrote:
>>> Hi Andre,
>>>
>>> On 8/9/19 5:52 PM, André Almeida wrote:
>>>> Hello Shuah,
>>>>
>>>> Thanks for the patch, I did some comments below.
>>>>
>>>> On 8/9/19 6:45 PM, Shuah Khan wrote:
>>>>> vimc uses Component API to split the driver into functional components.
>>>>> The real hardware resembles a monolith structure than component and
>>>>> component structure added a level of complexity making it hard to
>>>>> maintain without adding any real benefit.
>>>>>       The sensor is one vimc component that would makes sense to be a
>>>>> separate
>>>>> module to closely align with the real hardware. It would be easier to
>>>>> collapse vimc into single monolithic driver first and then split the
>>>>> sensor off as a separate module.
>>>>>
>>>>> This patch series emoves the component API and makes minimal changes to
>>>>> the code base preserving the functional division of the code structure.
>>>>> Preserving the functional structure allows us to split the sensor off
>>>>> as a separate module in the future.
>>>>>
>>>>> Major design elements in this change are:
>>>>>       - Use existing struct vimc_ent_config and struct
>>>>> vimc_pipeline_config
>>>>>         to drive the initialization of the functional components.
>>>>>       - Make vimc_ent_config global by moving it to vimc.h
>>>>>       - Add two new hooks add and rm to initialize and register,
>>>>> unregister
>>>>>         and free subdevs.
>>>>>       - All component API is now gone and bind and unbind hooks are
>>>>> modified
>>>>>         to do "add" and "rm" with minimal changes to just add and rm
>>>>> subdevs.
>>>>>       - vimc-core's bind and unbind are now register and unregister.
>>>>>       - vimc-core invokes "add" hooks from its vimc_register_devices().
>>>>>         The "add" hooks remain the same and register subdevs. They don't
>>>>>         create platform devices of their own and use vimc's pdev.dev as
>>>>>         their reference device. The "add" hooks save their
>>>>> vimc_ent_device(s)
>>>>>         in the corresponding vimc_ent_config.
>>>>>       - vimc-core invokes "rm" hooks from its unregister to unregister
>>>>> subdevs
>>>>>         and cleanup.
>>>>>       - vimc-core invokes "add" and "rm" hooks with pointer to struct
>>>>> vimc_device
>>>>>         and the corresponding struct vimc_ent_config pointer.
>>>>>       The following configure and stream test works on all devices.
>>>>>            media-ctl -d platform:vimc -V '"Sensor
>>>>> A":0[fmt:SBGGR8_1X8/640x480]'
>>>>>       media-ctl -d platform:vimc -V '"Debayer
>>>>> A":0[fmt:SBGGR8_1X8/640x480]'
>>>>>       media-ctl -d platform:vimc -V '"Sensor
>>>>> B":0[fmt:SBGGR8_1X8/640x480]'
>>>>>       media-ctl -d platform:vimc -V '"Debayer
>>>>> B":0[fmt:SBGGR8_1X8/640x480]'
>>>>>            v4l2-ctl -z platform:vimc -d "RGB/YUV Capture" -v
>>>>> width=1920,height=1440
>>>>>       v4l2-ctl -z platform:vimc -d "Raw Capture 0" -v pixelformat=BA81
>>>>>       v4l2-ctl -z platform:vimc -d "Raw Capture 1" -v pixelformat=BA81
>>>>>            v4l2-ctl --stream-mmap --stream-count=100 -d /dev/video1
>>>>>       v4l2-ctl --stream-mmap --stream-count=100 -d /dev/video2
>>>>>       v4l2-ctl --stream-mmap --stream-count=100 -d /dev/video3
>>>>>
>>>>> The third patch in the series fixes a general protection fault found
>>>>> when rmmod is done while stream is active.
>>>>
>>>> I applied your patch on top of media_tree/master and I did some testing.
>>>> Not sure if I did something wrong, but just adding and removing the
>>>> module generated a kernel panic:
>>>
>>> Thanks for testing.
>>>
>>> Odd. I tested modprobe and rmmod both.I was working on Linux 5.3-rc2.
>>> I will apply these to media latest and work from there. I have to
>>> rebase these on top of the reverts from Lucas and Helen
>>
>> Ok, please let me know if I succeeded to reproduce.
>>
>>>>
>>>> ~# modprobe vimc
>>>> ~# rmmod vimc
>>>> [   16.452974] stack segment: 0000 [#1] SMP PTI
>>>> [   16.453688] CPU: 0 PID: 2038 Comm: rmmod Not tainted 5.3.0-rc2+ #36
>>>> [   16.454678] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
>>>> BIOS 1.12.0-20181126_142135-anatol 04/01/2014
>>>> [   16.456191] RIP: 0010:kfree+0x4d/0x240
>>>>
>>>> <registers values...>
>>>>
>>>> [   16.469188] Call Trace:
>>>> [   16.469666]  vimc_remove+0x35/0x90 [vimc]
>>>> [   16.470436]  platform_drv_remove+0x1f/0x40
>>>> [   16.471233]  device_release_driver_internal+0xd3/0x1b0
>>>> [   16.472184]  driver_detach+0x37/0x6b
>>>> [   16.472882]  bus_remove_driver+0x50/0xc1
>>>> [   16.473569]  vimc_exit+0xc/0xca0 [vimc]
>>>> [   16.474231]  __x64_sys_delete_module+0x18d/0x240
>>>> [   16.475036]  do_syscall_64+0x43/0x110
>>>> [   16.475656]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
>>>> [   16.476504] RIP: 0033:0x7fceb8dafa4b
>>>>
>>>> <registers values...>
>>>>
>>>> [   16.484853] Modules linked in: vimc(-) videobuf2_vmalloc
>>>> videobuf2_memops v4l2_tpg videobuf2_v4l2 videobuf2_common
>>>> [   16.486187] ---[ end trace 91e5e0894e254d49 ]---
>>>> [   16.486758] RIP: 0010:kfree+0x4d/0x240
>>>>
>>>> <registers values...>
>>>>
>>>> fish: “rmmod vimc” terminated by signal SIGSEGV (Address boundary error)
>>>>
>>>> I just added the module after booting, no other action was made. Here is
>>>> how my `git log --oneline` looks like:
>>>>
>>>> 897d708e922b media: vimc: Fix gpf in rmmod path when stream is active
>>>> 2e4a5ad8ad6d media: vimc: Collapse component structure into a single
>>>> monolithic driver
>>>> 7c8da1687e92 media: vimc: move private defines to a common header
>>>> 97299a303532 media: Remove dev_err() usage after platform_get_irq()
>>>> 25a3d6bac6b9 media: adv7511/cobalt: rename driver name to adv7511-v4l2
> 
> I couldn't reproduce the error, my tree looks the same:
> 
> [I] koike@floko ~/m/o/linux> git log --oneline
> e3345155c8ed (HEAD) media: vimc: Fix gpf in rmmod path when stream is active
> 43e9e2fe761f media: vimc: Collapse component structure into a single monolithic driver
> 8a6d0b9adde0 media: vimc: move private defines to a common header
> 97299a303532 (media/master) media: Remove dev_err() usage after platform_get_irq()
> 25a3d6bac6b9 media: adv7511/cobalt: rename driver name to adv7511-v4l2

Thanks Helen for trying to reproduce and sharing the result.
> 
> André, is this deterministic? Or it just happens sometimes?
> 
>>>> ...
>>>>
>>>>>
>>>>> vimc_print_dot (--print-dot) topology after this change:
>>>>> digraph board {
>>>>>      rankdir=TB
>>>>>      n00000001 [label="{{} | Sensor A\n/dev/v4l-subdev0 | {<port0>
>>>>> 0}}", shape=Mrecord, style=filled, fillcolor=green]
>>>>>      n00000001:port0 -> n00000005:port0 [style=bold]
>>>>>      n00000001:port0 -> n0000000b [style=bold]
>>>>>      n00000003 [label="{{} | Sensor B\n/dev/v4l-subdev1 | {<port0>
>>>>> 0}}", shape=Mrecord, style=filled, fillcolor=green]
>>>>>      n00000003:port0 -> n00000008:port0 [style=bold]
>>>>>      n00000003:port0 -> n0000000f [style=bold]
>>>>>      n00000005 [label="{{<port0> 0} | Debayer A\n/dev/v4l-subdev2 |
>>>>> {<port1> 1}}", shape=Mrecord, style=filled, fillcolor=green]
>>>>>      n00000005:port1 -> n00000015:port0
>>>>>      n00000008 [label="{{<port0> 0} | Debayer B\n/dev/v4l-subdev3 |
>>>>> {<port1> 1}}", shape=Mrecord, style=filled, fillcolor=green]
>>>>>      n00000008:port1 -> n00000015:port0 [style=dashed]
>>>>>      n0000000b [label="Raw Capture 0\n/dev/video1", shape=box,
>>>>> style=filled, fillcolor=yellow]
>>>>>      n0000000f [label="Raw Capture 1\n/dev/video2", shape=box,
>>>>> style=filled, fillcolor=yellow]
>>>>>      n00000013 [label="{{} | RGB/YUV Input\n/dev/v4l-subdev4 |
>>>>> {<port0> 0}}", shape=Mrecord, style=filled, fillcolor=green]
>>>>>      n00000013:port0 -> n00000015:port0 [style=dashed]
>>>>>      n00000015 [label="{{<port0> 0} | Scaler\n/dev/v4l-subdev5 |
>>>>> {<port1> 1}}", shape=Mrecord, style=filled, fillcolor=green]
>>>>>      n00000015:port1 -> n00000018 [style=bold]
>>>>>      n00000018 [label="RGB/YUV Capture\n/dev/video3", shape=box,
>>>>> style=filled, fillcolor=yellow]
>>>>> }
>>>>
>>>> Since the topology changed, it would be nice to change in the
>>>> documentation as well. The current dot file can be found at
>>>> `Documentation/media/v4l-drivers/vimc.dot` and it's rendered at this
>>>> page: https://www.kernel.org/doc/html/latest/media/v4l-drivers/vimc.html
>>>>
>>>

Thanks Andre! Yes this is the one I am using as a reference.

>>> Topology shouldn't have changed. No changes to links or pads etc.
>>> I will take a look to be sure. I agree that if topology changes
>>> document should be updated.
>>
>> If you "diff" the current dot with the dot you generated, you will see
>> some differences. The main difference is that "RGB/YUV Input" was a
>> device "/dev/video2/", and now it a subdevice "/dev/v4l-subdev4".
> 
> hmm, I just generated the topology for media/master, and it is
> /dev/v4l-subdev4. As we don't have an implementation of the output device
> yet, we used the sensor as a place holder and that is why it appears as
> "/dev/v4l-subdev4", what is in the docs is the ideal version after we get
> the output merged. We should update the docs in any case.
> 

Helen! Does the master match what I generated. In any case, I will do
the diff of before and after my patches for sure.

I can update the document with what it should be.

thanks,
-- Shuah


  reply index

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-09 21:45 Shuah Khan
2019-08-09 21:45 ` [PATCH 1/3] media: vimc: move private defines to a common header Shuah Khan
2019-08-10  3:15   ` Helen Koike
2019-08-12 14:03     ` Shuah Khan
2019-08-10 14:14   ` Laurent Pinchart
2019-08-12 14:19     ` Shuah Khan
2019-08-12 14:24       ` Laurent Pinchart
2019-08-12 14:27         ` Shuah Khan
2019-08-09 21:45 ` [PATCH 2/3] media: vimc: Collapse component structure into a single monolithic driver Shuah Khan
2019-08-10  4:12   ` Helen Koike
2019-08-12 14:12     ` Shuah Khan
2019-08-09 21:45 ` [PATCH 3/3] media: vimc: Fix gpf in rmmod path when stream is active Shuah Khan
2019-08-09 23:52 ` [PATCH 0/3] Collapse vimc into single monolithic driver André Almeida
2019-08-10  0:17   ` Shuah Khan
2019-08-10  0:24     ` André Almeida
2019-08-10  0:48       ` Shuah Khan
2019-08-10  3:51       ` Helen Koike
2019-08-12 14:08         ` Shuah Khan [this message]
2019-08-12 18:52           ` André Almeida
2019-08-12 19:10             ` Shuah Khan
2019-08-12 22:14               ` Shuah Khan
2019-08-12 23:41                 ` Helen Koike
2019-08-13  0:58                   ` Shuah Khan
2019-08-13  9:56                   ` Laurent Pinchart
2019-08-13 12:25                     ` Helen Koike
2019-08-13 12:36                       ` Laurent Pinchart
2019-08-13 23:22                         ` Shuah Khan

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=2ee23903-8e99-a0a0-619a-be5bdaa71802@linuxfoundation.org \
    --to=skhan@linuxfoundation.org \
    --cc=andrealmeid@collabora.com \
    --cc=helen.koike@collabora.com \
    --cc=hverkuil@xs4all.nl \
    --cc=kernel@collabora.com \
    --cc=laurent.pinchart@ideasonboard.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=mchehab@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Media Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-media/0 linux-media/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-media linux-media/ https://lore.kernel.org/linux-media \
		linux-media@vger.kernel.org linux-media@archiver.kernel.org
	public-inbox-index linux-media


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-media


AGPL code for this site: git clone https://public-inbox.org/ public-inbox