Message ID | 20200224232126.3385250-1-sebastian.reichel@collabora.com (mailing list archive) |
---|---|
Headers | show |
Series | drm/omap: Convert DSI code to use drm_mipi_dsi and drm_panel | expand |
* Sebastian Reichel <sebastian.reichel@collabora.com> [200224 23:22]: > This updates the existing omapdrm DSI code, so that it uses > common drm_mipi_dsi API and drm_panel. > > The patchset has been tested with Droid 4 using Linux console, X.org and > Weston. The patchset is based on Laurent Pinchartl's patch series [0] > and removes the last custom panel driver, so quite a few cleanups on the > omapdrm codebase were possible. > > [0] [PATCH v7 00/54] drm/omap: Replace custom display drivers with drm_bridge and drm_panel > https://lore.kernel.org/dri-devel/20200222150106.22919-1-laurent.pinchart@ideasonboard.com/ > git://linuxtv.org/pinchartl/media.git omapdrm/bridge/devel > > I pushed this patchset into the following branch: > > git://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-n900.git omapdrm/bridge/devel-with-dsi I gave your omapdrm/bridge/devel-with-dsi branch a quirk test on droid4, but get the following oops with mostly modular omap2_defconfig. BTW, I think there's also some refcount issue in general where the omapdrm related modules cannot be unloaded any longer? Regards, Tony 8< ------------------------- Internal error: Oops: 805 [#1] PREEMPT SMP ARM Modules linked in: omapdss(+) omapdss_base drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops drm drm_panel_ors CPU: 0 PID: 811 Comm: modprobe Not tainted 5.6.0-rc2-00376-g163017c12d62a #1926 Hardware name: Generic OMAP4 (Flattened Device Tree) PC is at drm_bridge_remove+0x24/0x40 [drm] LR is at drm_bridge_remove+0x14/0x40 [drm] pc : [<bf2a0cb4>] lr : [<bf2a0ca4>] psr: 60000013 sp : edc6fc88 ip : eda3a100 fp : bf38a5b4 r10: bf38c7d0 r9 : edc164b8 r8 : edc1606c r7 : edc1647c r6 : edc93010 r5 : bf2dc428 r4 : edc164b8 r3 : edc164fc r2 : 00000000 r1 : 00000000 r0 : bf2dc428 Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none Control: 10c5387d Table: adc7404a DAC: 00000051 Process modprobe (pid: 811, stack limit = 0x(ptrval)) Stack: (0xedc6fc88 to 0xedc70000) fc80: edc16040 fffffdfb edc93010 bf37dfd0 00000080 edc2eb00 fca0: edc16040 edc93010 edc93010 00000000 bf392178 00000000 00000000 bf392178 fcc0: 0000001e c0624424 edc93010 c0efafa0 c0efafa4 00000000 00000000 c06222e4 fce0: edc93010 bf392178 bf392178 c0622880 c06243a0 c06243dc c0e93210 c06225cc fd00: bf38c450 a0000013 bf38a5b4 edc93010 00000000 bf392178 c0622880 c06243a0 fd20: c06243dc c0622878 00000000 bf392178 edc93010 c06228d8 edc94fb4 edc6e000 fd40: bf392178 c062069c edc50eb8 ee8dae58 edc94fb4 d759a07d ee8dae6c bf392178 fd60: edc50e80 c0e93210 00000000 c0621618 bf38f0c0 bf3922c0 00000006 bf392178 fd80: bf3922c0 00000006 c0623db0 c062344c 00000002 bf3922c0 00000006 c06244fc fda0: bf38a5b0 bf38a5a8 000be666 c0ec0540 bf39a000 00000000 bf392308 00000002 fdc0: edc2c300 c01dc694 edc6ff30 c0102fec c0ebbcf4 00000000 00000cc0 c0d75a70 fde0: edc2c2c0 c02b4538 c0d75a70 a0000013 a0000013 00000008 c01de5bc 00000002 fe00: 00000001 c01dc694 edc6ff30 d759a07d edbe7400 bf3922c0 00000002 d759a07d fe20: bf392308 bf3922c0 edbe7400 edc2c2c0 bf392308 c01de5f8 00000002 edc2c2c0 fe40: bf3922c0 00000002 edc2c2c0 c01e0b1c bf3922cc 00007fff bf3922c0 c01dd34c fe60: bf3a44ee 004c1a8f bf3922c0 bf3923d4 bf3924b8 c0a05fa8 00000000 bf38d075 fe80: 00000001 00000000 c0bddd78 c0bc76d8 bf38a03c 00000002 00000000 00000000 fea0: 00000000 00000000 6e72656b 00006c65 00000000 00000000 00000000 00000000 fec0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 fee0: 00000000 d759a07d 7fffffff edc6e000 00000000 00000003 004c1a8f 7fffffff ff00: 00000000 0000017b 004d56a0 c01e1050 7fffffff 00000000 00000003 c093c724 ff20: 00000002 f1370000 000e3464 00000000 f138b0c6 f138f3c0 f1370000 000e3464 ff40: f1452a3c f14527a4 f1425e58 00022000 00025680 0000a084 00027a0b 00000000 ff60: 00000000 00000000 0000a074 0000003e 0000003f 00000021 00000025 00000018 ff80: 00000000 d759a07d 004dcb80 64a54c46 b6f6a510 0000017b 0000017b c0101204 ffa0: edc6e000 c0101000 64a54c46 b6f6a510 00000003 004c1a8f 00000000 004dcb80 ffc0: 64a54c46 b6f6a510 0000017b 0000017b 004dcb80 00000001 00000000 004d56a0 ffe0: 004dcb80 bea7ba90 0043d1a9 b6f1d0b8 60000030 00000003 00000000 00000000 [<bf2a0cb4>] (drm_bridge_remove [drm]) from [<bf37dfd0>] (dsi_probe+0x3ac/0x55c [omapdss]) [<bf37dfd0>] (dsi_probe [omapdss]) from [<c0624424>] (platform_drv_probe+0x48/0x98) [<c0624424>] (platform_drv_probe) from [<c06222e4>] (really_probe+0x1dc/0x340) [<c06222e4>] (really_probe) from [<c06225cc>] (driver_probe_device+0x5c/0x160) [<c06225cc>] (driver_probe_device) from [<c0622878>] (device_driver_attach+0x58/0x60) [<c0622878>] (device_driver_attach) from [<c06228d8>] (__driver_attach+0x58/0xcc) [<c06228d8>] (__driver_attach) from [<c062069c>] (bus_for_each_dev+0x78/0xb8) [<c062069c>] (bus_for_each_dev) from [<c0621618>] (bus_add_driver+0xf0/0x1d4) [<c0621618>] (bus_add_driver) from [<c062344c>] (driver_register+0x74/0x108) [<c062344c>] (driver_register) from [<c06244fc>] (__platform_register_drivers+0x54/0xd0) [<c06244fc>] (__platform_register_drivers) from [<c0102fec>] (do_one_initcall+0x44/0x2a8) [<c0102fec>] (do_one_initcall) from [<c01de5f8>] (do_init_module+0x5c/0x234) [<c01de5f8>] (do_init_module) from [<c01e0b1c>] (load_module+0x22c4/0x2580) [<c01e0b1c>] (load_module) from [<c01e1050>] (sys_finit_module+0xc4/0xe0) [<c01e1050>] (sys_finit_module) from [<c0101000>] (ret_fast_syscall+0x0/0x54) Exception stack(0xedc6ffa8 to 0xedc6fff0) ffa0: 64a54c46 b6f6a510 00000003 004c1a8f 00000000 004dcb80 ffc0: 64a54c46 b6f6a510 0000017b 0000017b 004dcb80 00000001 00000000 004d56a0 ffe0: 004dcb80 bea7ba90 0043d1a9 b6f1d0b8 Code: e5942048 e5941044 e2843044 e1a00005 (e5812004) ---[ end trace 6213ab3cbabf9f59 ]---
Hi, On Mon, Feb 24, 2020 at 04:10:11PM -0800, Tony Lindgren wrote: > * Sebastian Reichel <sebastian.reichel@collabora.com> [200224 23:22]: > > This updates the existing omapdrm DSI code, so that it uses > > common drm_mipi_dsi API and drm_panel. > > > > The patchset has been tested with Droid 4 using Linux console, X.org and > > Weston. The patchset is based on Laurent Pinchartl's patch series [0] > > and removes the last custom panel driver, so quite a few cleanups on the > > omapdrm codebase were possible. > > > > [0] [PATCH v7 00/54] drm/omap: Replace custom display drivers with drm_bridge and drm_panel > > https://lore.kernel.org/dri-devel/20200222150106.22919-1-laurent.pinchart@ideasonboard.com/ > > git://linuxtv.org/pinchartl/media.git omapdrm/bridge/devel > > > > I pushed this patchset into the following branch: > > > > git://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-n900.git omapdrm/bridge/devel-with-dsi > > I gave your omapdrm/bridge/devel-with-dsi branch a quirk test > on droid4, but get the following oops with mostly modular > omap2_defconfig. :( I was testing with a non-modular kernel, since that gives me a much easier testing workflow. Guess I need to spend some time to get a good setup for testing modular kernel with Droid4. > BTW, I think there's also some refcount issue in general where > the omapdrm related modules cannot be unloaded any longer? I wouldn't be too surprised. The dependencies are quite interesting at the moment with omapdss registering omapdrm and then omapdrm registers the drm_device, which references the encoders from omapdss. I think this is something to look at once Laurent's and my branch have been merged to avoid increasing the complexity. Technically it should be possible to link everything into one module. > 8< ------------------------- > Internal error: Oops: 805 [#1] PREEMPT SMP ARM > Modules linked in: omapdss(+) omapdss_base drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops drm drm_panel_ors > CPU: 0 PID: 811 Comm: modprobe Not tainted 5.6.0-rc2-00376-g163017c12d62a #1926 > Hardware name: Generic OMAP4 (Flattened Device Tree) > PC is at drm_bridge_remove+0x24/0x40 [drm] > LR is at drm_bridge_remove+0x14/0x40 [drm] > pc : [<bf2a0cb4>] lr : [<bf2a0ca4>] psr: 60000013 > sp : edc6fc88 ip : eda3a100 fp : bf38a5b4 > r10: bf38c7d0 r9 : edc164b8 r8 : edc1606c > r7 : edc1647c r6 : edc93010 r5 : bf2dc428 r4 : edc164b8 > r3 : edc164fc r2 : 00000000 r1 : 00000000 r0 : bf2dc428 > Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none > Control: 10c5387d Table: adc7404a DAC: 00000051 > Process modprobe (pid: 811, stack limit = 0x(ptrval)) > Stack: (0xedc6fc88 to 0xedc70000) > fc80: edc16040 fffffdfb edc93010 bf37dfd0 00000080 edc2eb00 > fca0: edc16040 edc93010 edc93010 00000000 bf392178 00000000 00000000 bf392178 > fcc0: 0000001e c0624424 edc93010 c0efafa0 c0efafa4 00000000 00000000 c06222e4 > fce0: edc93010 bf392178 bf392178 c0622880 c06243a0 c06243dc c0e93210 c06225cc > fd00: bf38c450 a0000013 bf38a5b4 edc93010 00000000 bf392178 c0622880 c06243a0 > fd20: c06243dc c0622878 00000000 bf392178 edc93010 c06228d8 edc94fb4 edc6e000 > fd40: bf392178 c062069c edc50eb8 ee8dae58 edc94fb4 d759a07d ee8dae6c bf392178 > fd60: edc50e80 c0e93210 00000000 c0621618 bf38f0c0 bf3922c0 00000006 bf392178 > fd80: bf3922c0 00000006 c0623db0 c062344c 00000002 bf3922c0 00000006 c06244fc > fda0: bf38a5b0 bf38a5a8 000be666 c0ec0540 bf39a000 00000000 bf392308 00000002 > fdc0: edc2c300 c01dc694 edc6ff30 c0102fec c0ebbcf4 00000000 00000cc0 c0d75a70 > fde0: edc2c2c0 c02b4538 c0d75a70 a0000013 a0000013 00000008 c01de5bc 00000002 > fe00: 00000001 c01dc694 edc6ff30 d759a07d edbe7400 bf3922c0 00000002 d759a07d > fe20: bf392308 bf3922c0 edbe7400 edc2c2c0 bf392308 c01de5f8 00000002 edc2c2c0 > fe40: bf3922c0 00000002 edc2c2c0 c01e0b1c bf3922cc 00007fff bf3922c0 c01dd34c > fe60: bf3a44ee 004c1a8f bf3922c0 bf3923d4 bf3924b8 c0a05fa8 00000000 bf38d075 > fe80: 00000001 00000000 c0bddd78 c0bc76d8 bf38a03c 00000002 00000000 00000000 > fea0: 00000000 00000000 6e72656b 00006c65 00000000 00000000 00000000 00000000 > fec0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 > fee0: 00000000 d759a07d 7fffffff edc6e000 00000000 00000003 004c1a8f 7fffffff > ff00: 00000000 0000017b 004d56a0 c01e1050 7fffffff 00000000 00000003 c093c724 > ff20: 00000002 f1370000 000e3464 00000000 f138b0c6 f138f3c0 f1370000 000e3464 > ff40: f1452a3c f14527a4 f1425e58 00022000 00025680 0000a084 00027a0b 00000000 > ff60: 00000000 00000000 0000a074 0000003e 0000003f 00000021 00000025 00000018 > ff80: 00000000 d759a07d 004dcb80 64a54c46 b6f6a510 0000017b 0000017b c0101204 > ffa0: edc6e000 c0101000 64a54c46 b6f6a510 00000003 004c1a8f 00000000 004dcb80 > ffc0: 64a54c46 b6f6a510 0000017b 0000017b 004dcb80 00000001 00000000 004d56a0 > ffe0: 004dcb80 bea7ba90 0043d1a9 b6f1d0b8 60000030 00000003 00000000 00000000 > [<bf2a0cb4>] (drm_bridge_remove [drm]) from [<bf37dfd0>] (dsi_probe+0x3ac/0x55c [omapdss]) > [<bf37dfd0>] (dsi_probe [omapdss]) from [<c0624424>] (platform_drv_probe+0x48/0x98) > [<c0624424>] (platform_drv_probe) from [<c06222e4>] (really_probe+0x1dc/0x340) > [<c06222e4>] (really_probe) from [<c06225cc>] (driver_probe_device+0x5c/0x160) > [<c06225cc>] (driver_probe_device) from [<c0622878>] (device_driver_attach+0x58/0x60) > [<c0622878>] (device_driver_attach) from [<c06228d8>] (__driver_attach+0x58/0xcc) > [<c06228d8>] (__driver_attach) from [<c062069c>] (bus_for_each_dev+0x78/0xb8) > [<c062069c>] (bus_for_each_dev) from [<c0621618>] (bus_add_driver+0xf0/0x1d4) > [<c0621618>] (bus_add_driver) from [<c062344c>] (driver_register+0x74/0x108) > [<c062344c>] (driver_register) from [<c06244fc>] (__platform_register_drivers+0x54/0xd0) > [<c06244fc>] (__platform_register_drivers) from [<c0102fec>] (do_one_initcall+0x44/0x2a8) > [<c0102fec>] (do_one_initcall) from [<c01de5f8>] (do_init_module+0x5c/0x234) > [<c01de5f8>] (do_init_module) from [<c01e0b1c>] (load_module+0x22c4/0x2580) > [<c01e0b1c>] (load_module) from [<c01e1050>] (sys_finit_module+0xc4/0xe0) > [<c01e1050>] (sys_finit_module) from [<c0101000>] (ret_fast_syscall+0x0/0x54) > Exception stack(0xedc6ffa8 to 0xedc6fff0) > ffa0: 64a54c46 b6f6a510 00000003 004c1a8f 00000000 004dcb80 > ffc0: 64a54c46 b6f6a510 0000017b 0000017b 004dcb80 00000001 00000000 004d56a0 > ffe0: 004dcb80 bea7ba90 0043d1a9 b6f1d0b8 > Code: e5942048 e5941044 e2843044 e1a00005 (e5812004) > ---[ end trace 6213ab3cbabf9f59 ]--- oops, looks like dsi_bridge_init() is missing "drm_bridge_add(&dsi->bridge);" before returning. Now I wonder why it is working for me without the bridge being added :) I will send out an updated patch for this tomorrow. -- Sebastian
* Sebastian Reichel <sre@kernel.org> [200225 02:29]: > On Mon, Feb 24, 2020 at 04:10:11PM -0800, Tony Lindgren wrote: > > BTW, I think there's also some refcount issue in general where > > the omapdrm related modules cannot be unloaded any longer? > > I wouldn't be too surprised. The dependencies are quite interesting > at the moment with omapdss registering omapdrm and then omapdrm > registers the drm_device, which references the encoders from > omapdss. I think this is something to look at once Laurent's and > my branch have been merged to avoid increasing the complexity. > Technically it should be possible to link everything into one > module. Well the DSS is really DOSSI for Display Output SubSystem Interconnect :) The devices on the interconnect are mostly independent and ideally the toplevel dss driver would just provide Linux generic resources to dispc and various output drivers. So probably not a good idea to try to build it all into a single module. Regards, Tony
Hi, On Tue, Feb 25, 2020 at 07:42:37AM -0800, Tony Lindgren wrote: > * Sebastian Reichel <sre@kernel.org> [200225 02:29]: > > On Mon, Feb 24, 2020 at 04:10:11PM -0800, Tony Lindgren wrote: > > > BTW, I think there's also some refcount issue in general where > > > the omapdrm related modules cannot be unloaded any longer? > > > > I wouldn't be too surprised. The dependencies are quite interesting > > at the moment with omapdss registering omapdrm and then omapdrm > > registers the drm_device, which references the encoders from > > omapdss. I think this is something to look at once Laurent's and > > my branch have been merged to avoid increasing the complexity. > > Technically it should be possible to link everything into one > > module. > > Well the DSS is really DOSSI for Display Output SubSystem > Interconnect :) The devices on the interconnect are mostly > independent and ideally the toplevel dss driver would just > provide Linux generic resources to dispc and various output > drivers. So probably not a good idea to try to build it all > into a single module. All the output drivers and dispc are already in a single module: omapdss.ko. There is omapdss-base.ko, omapdss.ko and omapdrm.ko module. omapdss-base.ko contains a few helpers, omapdss.ko contains dispc and all output encoders, omapdrm has the tiler code and wraps some of the custom DSS APIs to DRM APIs. I think the best way forward is to eliminate the custom API and use common DRM APIs directly. Then merge all 3 modules into one module. In theory one could add modules for each encoder, but practically this only increases complexity. DRM cannnot hotplug encoders, so removing any module results in complete loss of DRM. Also during probe we need to load all modules, since something might be connected. So having extra modules is not really useful? -- Sebastian
Hi Sebastian, On Wed, Feb 26, 2020 at 12:01:24AM +0100, Sebastian Reichel wrote: > On Tue, Feb 25, 2020 at 07:42:37AM -0800, Tony Lindgren wrote: > > * Sebastian Reichel <sre@kernel.org> [200225 02:29]: > > > On Mon, Feb 24, 2020 at 04:10:11PM -0800, Tony Lindgren wrote: > > > > BTW, I think there's also some refcount issue in general where > > > > the omapdrm related modules cannot be unloaded any longer? > > > > > > I wouldn't be too surprised. The dependencies are quite interesting > > > at the moment with omapdss registering omapdrm and then omapdrm > > > registers the drm_device, which references the encoders from > > > omapdss. I think this is something to look at once Laurent's and > > > my branch have been merged to avoid increasing the complexity. > > > Technically it should be possible to link everything into one > > > module. > > > > Well the DSS is really DOSSI for Display Output SubSystem > > Interconnect :) The devices on the interconnect are mostly > > independent and ideally the toplevel dss driver would just > > provide Linux generic resources to dispc and various output > > drivers. So probably not a good idea to try to build it all > > into a single module. > > All the output drivers and dispc are already in a single module: > omapdss.ko. There is omapdss-base.ko, omapdss.ko and omapdrm.ko > module. omapdss-base.ko contains a few helpers, omapdss.ko contains > dispc and all output encoders, omapdrm has the tiler code and > wraps some of the custom DSS APIs to DRM APIs. I think the best > way forward is to eliminate the custom API and use common DRM > APIs directly. Then merge all 3 modules into one module. > > In theory one could add modules for each encoder, but practically > this only increases complexity. DRM cannnot hotplug encoders, so > removing any module results in complete loss of DRM. Also during > probe we need to load all modules, since something might be > connected. So having extra modules is not really useful? This was considered previously, but blocked (IIRC) because it introduced circular dependencies with the omapdrm-specific display drivers in drivers/gpu/drm/omapdrm/displays/. Once panel-dsi-cm goes away, I think we can give it another try.
* Sebastian Reichel <sre@kernel.org> [200225 23:03]: > Hi, > > On Tue, Feb 25, 2020 at 07:42:37AM -0800, Tony Lindgren wrote: > > * Sebastian Reichel <sre@kernel.org> [200225 02:29]: > > > On Mon, Feb 24, 2020 at 04:10:11PM -0800, Tony Lindgren wrote: > > > > BTW, I think there's also some refcount issue in general where > > > > the omapdrm related modules cannot be unloaded any longer? > > > > > > I wouldn't be too surprised. The dependencies are quite interesting > > > at the moment with omapdss registering omapdrm and then omapdrm > > > registers the drm_device, which references the encoders from > > > omapdss. I think this is something to look at once Laurent's and > > > my branch have been merged to avoid increasing the complexity. > > > Technically it should be possible to link everything into one > > > module. > > > > Well the DSS is really DOSSI for Display Output SubSystem > > Interconnect :) The devices on the interconnect are mostly > > independent and ideally the toplevel dss driver would just > > provide Linux generic resources to dispc and various output > > drivers. So probably not a good idea to try to build it all > > into a single module. > > All the output drivers and dispc are already in a single module: > omapdss.ko. There is omapdss-base.ko, omapdss.ko and omapdrm.ko > module. omapdss-base.ko contains a few helpers, omapdss.ko contains > dispc and all output encoders, omapdrm has the tiler code and > wraps some of the custom DSS APIs to DRM APIs. I think the best > way forward is to eliminate the custom API and use common DRM > APIs directly. Then merge all 3 modules into one module. > > In theory one could add modules for each encoder, but practically > this only increases complexity. DRM cannnot hotplug encoders, so > removing any module results in complete loss of DRM. Also during > probe we need to load all modules, since something might be > connected. So having extra modules is not really useful? Well my main concern here is that we should use generic Linux frameworks between the devices within DSS where possible. I can see a single driver pile of code quickly turn into a spaghetti of internal calls instead.. Also each devices on the DSS interconnect needs to do pm_runtime_get for it's struct device naturally. If you can avoid the issues above, then I have no objections of just having one module. Regards, Tony
Hi, On Tue, Feb 25, 2020 at 03:09:37PM -0800, Tony Lindgren wrote: > * Sebastian Reichel <sre@kernel.org> [200225 23:03]: > > Hi, > > > > On Tue, Feb 25, 2020 at 07:42:37AM -0800, Tony Lindgren wrote: > > > * Sebastian Reichel <sre@kernel.org> [200225 02:29]: > > > > On Mon, Feb 24, 2020 at 04:10:11PM -0800, Tony Lindgren wrote: > > > > > BTW, I think there's also some refcount issue in general where > > > > > the omapdrm related modules cannot be unloaded any longer? > > > > > > > > I wouldn't be too surprised. The dependencies are quite interesting > > > > at the moment with omapdss registering omapdrm and then omapdrm > > > > registers the drm_device, which references the encoders from > > > > omapdss. I think this is something to look at once Laurent's and > > > > my branch have been merged to avoid increasing the complexity. > > > > Technically it should be possible to link everything into one > > > > module. > > > > > > Well the DSS is really DOSSI for Display Output SubSystem > > > Interconnect :) The devices on the interconnect are mostly > > > independent and ideally the toplevel dss driver would just > > > provide Linux generic resources to dispc and various output > > > drivers. So probably not a good idea to try to build it all > > > into a single module. > > > > All the output drivers and dispc are already in a single module: > > omapdss.ko. There is omapdss-base.ko, omapdss.ko and omapdrm.ko > > module. omapdss-base.ko contains a few helpers, omapdss.ko contains > > dispc and all output encoders, omapdrm has the tiler code and > > wraps some of the custom DSS APIs to DRM APIs. I think the best > > way forward is to eliminate the custom API and use common DRM > > APIs directly. Then merge all 3 modules into one module. > > > > In theory one could add modules for each encoder, but practically > > this only increases complexity. DRM cannnot hotplug encoders, so > > removing any module results in complete loss of DRM. Also during > > probe we need to load all modules, since something might be > > connected. So having extra modules is not really useful? > > Well my main concern here is that we should use generic > Linux frameworks between the devices within DSS where possible. Ack. > I can see a single driver pile of code quickly turn into a > spaghetti of internal calls instead. I was only talking about generating a single module, it contains multiple drivers. And we already have the spaghetti of internal calls between omapdrm and omapdss. With the modules it only means, that functions are either exported or accessed via callbacks. > Also each devices on the DSS interconnect needs to do > pm_runtime_get for it's struct device naturally. Sure, but you are talking about things that are already part of a single module (omapdss.ko). omapdss-base.ko and omapdrm.ko do not directly access hardware. > If you can avoid the issues above, then I have no objections > of just having one module. Well for now let's get Laurent's and my series forward. I think the next step would be to get rid of omap_encoder by moving the encoder init into the DSS output code. Technically we are already merging omapdrm and omapdss, e.g. omap_connector is gone from omapdrm after the series. -- Sebastian
On 26/02/2020 01:52, Sebastian Reichel wrote: > Well for now let's get Laurent's and my series forward. I think > the next step would be to get rid of omap_encoder by moving the > encoder init into the DSS output code. Technically we are already > merging omapdrm and omapdss, e.g. omap_connector is gone from > omapdrm after the series. After Laurent's series (now pushed) and this one, we can turn omapdrm into a single module, as it's supposed to be. I don't see a point in trying to change DSS internal encoders into separate modules, as they're all under DSS, and at times connected in ways which are not easily represented with generic APIs. In theory it should be possible, but I don't see any real benefit in such work. Tomi
On 25/02/2020 01:20, Sebastian Reichel wrote: > This updates the existing omapdrm DSI code, so that it uses > common drm_mipi_dsi API and drm_panel. > > The patchset has been tested with Droid 4 using Linux console, X.org and > Weston. The patchset is based on Laurent Pinchartl's patch series [0] > and removes the last custom panel driver, so quite a few cleanups on the > omapdrm codebase were possible. I haven't done any reviews yet, but applied these (and the one v2.1 patch) on top of Laurent's. I booted up AM5 EVM and loaded the modules: [ 17.261560] WARNING: CPU: 0 PID: 419 at drivers/base/component.c:636 component_bind_all+0x1f4/0x258 [ 17.270811] Modules linked in: omapdrm(+) omapdss omapdss_base panel_osd_osd101t2587_53ts panel_simple simple_bridge ti_tpd12s015 display_connec tor tc358767 tc358768 sii902x ti_tfp410 drm_kms_helper drm drm_panel_orientation_quirks cfbfillrect cfbimgblt cfbcopyarea cec [ 17.295521] CPU: 0 PID: 419 Comm: insmod Not tainted 5.6.0-rc2-00375-g66c4203ed9d4 #2 [ 17.303387] Hardware name: Generic DRA74X (Flattened Device Tree) [ 17.309520] [<c011431c>] (unwind_backtrace) from [<c010dc20>] (show_stack+0x10/0x14) [ 17.317306] [<c010dc20>] (show_stack) from [<c09acca4>] (dump_stack+0xb4/0xd0) [ 17.324567] [<c09acca4>] (dump_stack) from [<c013a33c>] (__warn+0xc0/0xf8) [ 17.331477] [<c013a33c>] (__warn) from [<c013a700>] (warn_slowpath_fmt+0x58/0xb8) [ 17.338998] [<c013a700>] (warn_slowpath_fmt) from [<c0630698>] (component_bind_all+0x1f4/0x258) [ 17.347768] [<c0630698>] (component_bind_all) from [<bf11b52c>] (pdev_probe+0xe0/0x7a0 [omapdrm]) [ 17.356709] [<bf11b52c>] (pdev_probe [omapdrm]) from [<c063a198>] (platform_drv_probe+0x48/0x98) [ 17.365537] [<c063a198>] (platform_drv_probe) from [<c0637c1c>] (really_probe+0x200/0x478) [ 17.373841] [<c0637c1c>] (really_probe) from [<c0638060>] (driver_probe_device+0x6c/0x1b4) [ 17.382145] [<c0638060>] (driver_probe_device) from [<c06383f0>] (device_driver_attach+0x58/0x60) [ 17.391060] [<c06383f0>] (device_driver_attach) from [<c063849c>] (__driver_attach+0xa4/0x148) [ 17.399713] [<c063849c>] (__driver_attach) from [<c0635af8>] (bus_for_each_dev+0x70/0xb4) [ 17.407930] [<c0635af8>] (bus_for_each_dev) from [<c0636d60>] (bus_add_driver+0x100/0x204) [ 17.416233] [<c0636d60>] (bus_add_driver) from [<c0639078>] (driver_register+0x74/0x108) [ 17.424361] [<c0639078>] (driver_register) from [<c063a2a8>] (__platform_register_drivers+0x58/0x150) [ 17.433628] [<c063a2a8>] (__platform_register_drivers) from [<c0102ff8>] (do_one_initcall+0x48/0x2a0) [ 17.442893] [<c0102ff8>] (do_one_initcall) from [<c01e2efc>] (do_init_module+0x5c/0x234) [ 17.451022] [<c01e2efc>] (do_init_module) from [<c01e5668>] (load_module+0x250c/0x28a4) [ 17.459064] [<c01e5668>] (load_module) from [<c01e5cb8>] (sys_finit_module+0xcc/0x110) [ 17.467020] [<c01e5cb8>] (sys_finit_module) from [<c0101000>] (ret_fast_syscall+0x0/0x54) [ 17.475233] Exception stack(0xea84bfa8 to 0xea84bff0) [ 17.480307] bfa0: 00000002 00000000 00000003 0002a894 00000000 bebd2d74 [ 17.488524] bfc0: 00000002 00000000 00028424 0000017b 0003eeb0 00000002 b6ffe000 00000000 [ 17.496738] bfe0: bebd2bb8 bebd2ba8 00020d7c b6edb0b0 [ 17.502365] ---[ end trace c20802296d6b2775 ]--- [ 17.508567] omapdss_dss 58000000.dss: bound 58001000.dispc (ops dsi_framedone_timeout_work_callback [omapdss]) [ 17.520358] omapdss_dss 58000000.dss: bound 58040000.encoder (ops dsi_framedone_timeout_work_callback [omapdss]) [ 17.559728] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). [ 17.574544] [drm] Enabling DMM ywrap scrolling [ 17.580606] omapdrm omapdrm.0: fb0: omapdrmdrmfb frame buffer device [ 17.625099] [drm] Initialized omapdrm 1.0.0 20110917 for omapdrm.0 on minor 0 Similar warning on module unload. The display works, though. Tomi
Hi, On Wed, Feb 26, 2020 at 02:28:23PM +0200, Tomi Valkeinen wrote: > On 25/02/2020 01:20, Sebastian Reichel wrote: > > This updates the existing omapdrm DSI code, so that it uses > > common drm_mipi_dsi API and drm_panel. > > > > The patchset has been tested with Droid 4 using Linux console, X.org and > > Weston. The patchset is based on Laurent Pinchartl's patch series [0] > > and removes the last custom panel driver, so quite a few cleanups on the > > omapdrm codebase were possible. > > I haven't done any reviews yet, but applied these (and the one > v2.1 patch) on top of Laurent's. I booted up AM5 EVM and loaded > the modules: > > [ 17.261560] WARNING: CPU: 0 PID: 419 at drivers/base/component.c:636 component_bind_all+0x1f4/0x258 I did not see that warning for some reason. I guess I was always lucky, since its a race condition introduced by "drm/omap: bind components with drm_device argument". That patch moves component_bind_all from dss_bind() to omapdrm's probe() to get access to drm_dev. Currently omapdrm is the only DRM driver not supplying that as parameter and its required to move encoder init into the DSS output drivers. I assumed that it would be ok, since it just postpones the call a little bit. Unfortunately it also moves it out of the lock protection from dss_bind(). The only fix, that I see is to register the drm_dev is dss_bind and then supply it to omapdrm via pdata. But I think we could actually get rid of the omapdrm platform device completly and just use its parent "omapdss_dss". I suppose a follow-up patchset could merge the code into one module and do this merge. Probably it's the best to drop that patch for now. It's not yet used in the patches, that I sent. > [ 17.270811] Modules linked in: omapdrm(+) omapdss omapdss_base panel_osd_osd101t2587_53ts panel_simple simple_bridge ti_tpd12s015 display_connec > tor tc358767 tc358768 sii902x ti_tfp410 drm_kms_helper drm drm_panel_orientation_quirks cfbfillrect cfbimgblt cfbcopyarea cec > [ 17.295521] CPU: 0 PID: 419 Comm: insmod Not tainted 5.6.0-rc2-00375-g66c4203ed9d4 #2 > [ 17.303387] Hardware name: Generic DRA74X (Flattened Device Tree) > [ 17.309520] [<c011431c>] (unwind_backtrace) from [<c010dc20>] (show_stack+0x10/0x14) > [ 17.317306] [<c010dc20>] (show_stack) from [<c09acca4>] (dump_stack+0xb4/0xd0) > [ 17.324567] [<c09acca4>] (dump_stack) from [<c013a33c>] (__warn+0xc0/0xf8) > [ 17.331477] [<c013a33c>] (__warn) from [<c013a700>] (warn_slowpath_fmt+0x58/0xb8) > [ 17.338998] [<c013a700>] (warn_slowpath_fmt) from [<c0630698>] (component_bind_all+0x1f4/0x258) > [ 17.347768] [<c0630698>] (component_bind_all) from [<bf11b52c>] (pdev_probe+0xe0/0x7a0 [omapdrm]) > [ 17.356709] [<bf11b52c>] (pdev_probe [omapdrm]) from [<c063a198>] (platform_drv_probe+0x48/0x98) > [ 17.365537] [<c063a198>] (platform_drv_probe) from [<c0637c1c>] (really_probe+0x200/0x478) > [ 17.373841] [<c0637c1c>] (really_probe) from [<c0638060>] (driver_probe_device+0x6c/0x1b4) > [ 17.382145] [<c0638060>] (driver_probe_device) from [<c06383f0>] (device_driver_attach+0x58/0x60) > [ 17.391060] [<c06383f0>] (device_driver_attach) from [<c063849c>] (__driver_attach+0xa4/0x148) > [ 17.399713] [<c063849c>] (__driver_attach) from [<c0635af8>] (bus_for_each_dev+0x70/0xb4) > [ 17.407930] [<c0635af8>] (bus_for_each_dev) from [<c0636d60>] (bus_add_driver+0x100/0x204) > [ 17.416233] [<c0636d60>] (bus_add_driver) from [<c0639078>] (driver_register+0x74/0x108) > [ 17.424361] [<c0639078>] (driver_register) from [<c063a2a8>] (__platform_register_drivers+0x58/0x150) > [ 17.433628] [<c063a2a8>] (__platform_register_drivers) from [<c0102ff8>] (do_one_initcall+0x48/0x2a0) > [ 17.442893] [<c0102ff8>] (do_one_initcall) from [<c01e2efc>] (do_init_module+0x5c/0x234) > [ 17.451022] [<c01e2efc>] (do_init_module) from [<c01e5668>] (load_module+0x250c/0x28a4) > [ 17.459064] [<c01e5668>] (load_module) from [<c01e5cb8>] (sys_finit_module+0xcc/0x110) > [ 17.467020] [<c01e5cb8>] (sys_finit_module) from [<c0101000>] (ret_fast_syscall+0x0/0x54) > [ 17.475233] Exception stack(0xea84bfa8 to 0xea84bff0) > [ 17.480307] bfa0: 00000002 00000000 00000003 0002a894 00000000 bebd2d74 > [ 17.488524] bfc0: 00000002 00000000 00028424 0000017b 0003eeb0 00000002 b6ffe000 00000000 > [ 17.496738] bfe0: bebd2bb8 bebd2ba8 00020d7c b6edb0b0 > [ 17.502365] ---[ end trace c20802296d6b2775 ]--- > [ 17.508567] omapdss_dss 58000000.dss: bound 58001000.dispc (ops dsi_framedone_timeout_work_callback [omapdss]) > [ 17.520358] omapdss_dss 58000000.dss: bound 58040000.encoder (ops dsi_framedone_timeout_work_callback [omapdss]) > [ 17.559728] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). > [ 17.574544] [drm] Enabling DMM ywrap scrolling > [ 17.580606] omapdrm omapdrm.0: fb0: omapdrmdrmfb frame buffer device > [ 17.625099] [drm] Initialized omapdrm 1.0.0 20110917 for omapdrm.0 on minor 0 > > Similar warning on module unload. The display works, though. same problem, with dss_unbind() and omapdrm's remove function. -- Sebastian
On 25/02/2020 01:20, Sebastian Reichel wrote: > This updates the existing omapdrm DSI code, so that it uses > common drm_mipi_dsi API and drm_panel. > > The patchset has been tested with Droid 4 using Linux console, X.org and > Weston. The patchset is based on Laurent Pinchartl's patch series [0] > and removes the last custom panel driver, so quite a few cleanups on the > omapdrm codebase were possible. This is a big series, and I suggest to keep the cleanups to minimum. Things can be cleaned up afterwards after the functional parts of this series have been merged. Tomi
Hi Tomi, On Wed, Mar 25, 2020 at 02:47:48PM +0200, Tomi Valkeinen wrote: > On 25/02/2020 01:20, Sebastian Reichel wrote: > > This updates the existing omapdrm DSI code, so that it uses > > common drm_mipi_dsi API and drm_panel. > > > > The patchset has been tested with Droid 4 using Linux console, X.org and > > Weston. The patchset is based on Laurent Pinchartl's patch series [0] > > and removes the last custom panel driver, so quite a few cleanups on the > > omapdrm codebase were possible. > > This is a big series, and I suggest to keep the cleanups to minimum. > Things can be cleaned up afterwards after the functional parts of this > series have been merged. There are a few cleanups at the bottom of the series that could be merged without waiting for the rest though :-)
On 25/03/2020 15:03, Laurent Pinchart wrote: > Hi Tomi, > > On Wed, Mar 25, 2020 at 02:47:48PM +0200, Tomi Valkeinen wrote: >> On 25/02/2020 01:20, Sebastian Reichel wrote: >>> This updates the existing omapdrm DSI code, so that it uses >>> common drm_mipi_dsi API and drm_panel. >>> >>> The patchset has been tested with Droid 4 using Linux console, X.org and >>> Weston. The patchset is based on Laurent Pinchartl's patch series [0] >>> and removes the last custom panel driver, so quite a few cleanups on the >>> omapdrm codebase were possible. >> >> This is a big series, and I suggest to keep the cleanups to minimum. >> Things can be cleaned up afterwards after the functional parts of this >> series have been merged. > > There are a few cleanups at the bottom of the series that could be > merged without waiting for the rest though :-) Yep. So, Sebastian, if there are patches that can be applied independently of this series, can you send those separately or move them to the beginning of the series. There was also the change to include/drm/drm_mipi_dsi.h which is outside the OMAP context. Maybe send that separately to the relevant maintainers. Or at least cc them, as now it's kind of hidden between all the omap changes. And I need to try to wake up one of my boards with a DSI video mode display for testing... Tomi
Hi Sebastian, > Am 25.02.2020 um 00:20 schrieb Sebastian Reichel <sebastian.reichel@collabora.com>: > > This updates the existing omapdrm DSI code, so that it uses > common drm_mipi_dsi API and drm_panel. > > The patchset has been tested with Droid 4 using Linux console, X.org and > Weston. The patchset is based on Laurent Pinchartl's patch series [0] > and removes the last custom panel driver, so quite a few cleanups on the > omapdrm codebase were possible. > > [0] [PATCH v7 00/54] drm/omap: Replace custom display drivers with drm_bridge and drm_panel > https://lore.kernel.org/dri-devel/20200222150106.22919-1-laurent.pinchart@ideasonboard.com/ > git://linuxtv.org/pinchartl/media.git omapdrm/bridge/devel > > I pushed this patchset into the following branch: > > git://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-n900.git omapdrm/bridge/devel-with-dsi > > The previous version of this patchset has been sent quite some time ago. > This version has been rebased and required cleaning up most of the > hacks. I do not have a detailed changelog, but quite a few things > changed. I decided against doing anything special for the DT change > (adding DSI channel number), since only 3 devices are affected. It is > quite likely, that all developers of those devices update DT together > with kernel for those devices. My suggestion is to merge the first two > patches ASAP and backport to stable, since it does not affect old > kernels and the change is rather small. > > RFCv1: https://lore.kernel.org/dri-devel/20191117023946.VjCC3yE08DMx7JIKxNagPoT5et7WTnKGVV6MtOtB9Ro@z/ is there something new for this series? Is there a repo where it has been rebased to v5.8-rc1 (and tested)? Our Pyra MIPI/DSI omap5 panel setup is broken by upstream changes since v5.7-rc1 and I am not sure if it is better to search in the old omapdrm code what is missing now. Or to wait for your DSI code and just update/fix our panel driver (and have a chance to get that one upstream). BR and thanks, Nikolaus > > -- Sebastian > > Sebastian Reichel (56): > ARM: dts: omap: add channel to DSI panels > ARM: dts: omap4-droid4: add panel compatible > Revert "drm/omap: dss: Remove unused omap_dss_device operations" > omap/drm: drop unused dsi.configure_pins > drm/omap: dsi: use MIPI_DSI_FMT_* instead of OMAP_DSS_DSI_FMT_* > drm/omap: constify write buffers > drm/omap: dsi: add generic transfer function > drm/omap: panel-dsi-cm: convert to transfer API > drm/omap: dsi: unexport specific data transfer functions > drm/omap: dsi: drop virtual channel logic > drm/omap: dsi: simplify write function > drm/omap: dsi: simplify read functions > drm/omap: dsi: switch dsi_vc_send_long/short to mipi_dsi_msg > drm/omap: dsi: introduce mipi_dsi_host > drm/omap: panel-dsi-cm: use DSI helpers > drm/omap: dsi: request VC via mipi_dsi_attach > drm/omap: panel-dsi-cm: drop hardcoded VC > drm/omap: panel-dsi-cm: use common MIPI DCS 1.3 defines > drm/omap: dsi: drop unused memory_read() > drm/omap: dsi: drop unused get_te() > drm/omap: dsi: drop unused enable_te() > drm/omap: dsi: drop useless sync() > drm/omap: dsi: use pixel-format and mode from attach > drm/omap: panel-dsi-cm: use bulk regulator API > drm/omap: dsi: lp/hs switching support for transfer() > drm/omap: dsi: move TE GPIO handling into core > drm/omap: dsi: drop custom enable_te() API > drm/omap: dsi: do bus locking in host driver > drm/omap: dsi: untangle ulps ops from enable/disable > drm/dsi: add MIPI_DSI_MODE_ULPS_IDLE > drm/omap: dsi: do ULPS in host driver > drm/omap: dsi: move panel refresh function to host > drm/omap: dsi: Reverse direction of the DSS device enable/disable > operations > drm/omap: dsi: drop custom panel capability support > drm/omap: dsi: convert to drm_panel > drm/omap: drop omapdss-boot-init > drm/omap: dsi: implement check timings > drm/omap: panel-dsi-cm: use DEVICE_ATTR_RO > drm/omap: panel-dsi-cm: support unbinding > drm/omap: panel-dsi-cm: fix remove() > drm/omap: dsi: return proper error code from dsi_update_all() > drm/omap: remove global dss_device variable > drm/omap: bind components with drm_device argument > drm/panel: Move OMAP's DSI command mode panel driver > drm/omap: dsi: Register a drm_bridge > drm/omap: remove legacy DSS device operations > drm/omap: remove unused omap_connector > drm/omap: simplify omap_display_id > drm/omap: drop unused DSS next pointer > drm/omap: drop empty omap_encoder helper functions > drm/omap: drop DSS ops_flags > drm/omap: drop dssdev display field > drm/omap: simplify DSI manual update code > ARM: omap2plus_defconfig: Update for moved DSI command mode panel > drm/panel/panel-dsi-cm: support rotation property > ARM: dts: omap4-droid4: add panel orientation > > .../bindings/display/panel/panel-dsi-cm.txt | 4 +- > .../boot/dts/motorola-mapphone-common.dtsi | 6 +- > arch/arm/boot/dts/omap3-n950.dts | 3 +- > arch/arm/boot/dts/omap3.dtsi | 3 + > arch/arm/boot/dts/omap4-sdp.dts | 6 +- > arch/arm/boot/dts/omap4.dtsi | 6 + > arch/arm/boot/dts/omap5.dtsi | 6 + > arch/arm/configs/omap2plus_defconfig | 2 +- > drivers/gpu/drm/omapdrm/Kconfig | 1 - > drivers/gpu/drm/omapdrm/Makefile | 2 - > drivers/gpu/drm/omapdrm/displays/Kconfig | 10 - > drivers/gpu/drm/omapdrm/displays/Makefile | 2 - > .../gpu/drm/omapdrm/displays/panel-dsi-cm.c | 1387 ----------------- > drivers/gpu/drm/omapdrm/dss/Kconfig | 4 +- > drivers/gpu/drm/omapdrm/dss/Makefile | 2 - > drivers/gpu/drm/omapdrm/dss/base.c | 58 +- > drivers/gpu/drm/omapdrm/dss/dsi.c | 1034 +++++++----- > drivers/gpu/drm/omapdrm/dss/dss.c | 44 +- > .../gpu/drm/omapdrm/dss/omapdss-boot-init.c | 220 --- > drivers/gpu/drm/omapdrm/dss/omapdss.h | 141 +- > drivers/gpu/drm/omapdrm/dss/output.c | 13 +- > drivers/gpu/drm/omapdrm/dss/venc.c | 1 - > drivers/gpu/drm/omapdrm/omap_connector.c | 157 -- > drivers/gpu/drm/omapdrm/omap_connector.h | 28 - > drivers/gpu/drm/omapdrm/omap_crtc.c | 35 +- > drivers/gpu/drm/omapdrm/omap_drv.c | 49 +- > drivers/gpu/drm/omapdrm/omap_drv.h | 1 - > drivers/gpu/drm/omapdrm/omap_encoder.c | 59 +- > drivers/gpu/drm/panel/Kconfig | 9 + > drivers/gpu/drm/panel/Makefile | 1 + > drivers/gpu/drm/panel/panel-dsi-cm.c | 674 ++++++++ > include/drm/drm_mipi_dsi.h | 2 + > 32 files changed, 1465 insertions(+), 2505 deletions(-) > delete mode 100644 drivers/gpu/drm/omapdrm/displays/Kconfig > delete mode 100644 drivers/gpu/drm/omapdrm/displays/Makefile > delete mode 100644 drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c > delete mode 100644 drivers/gpu/drm/omapdrm/dss/omapdss-boot-init.c > delete mode 100644 drivers/gpu/drm/omapdrm/omap_connector.c > delete mode 100644 drivers/gpu/drm/omapdrm/omap_connector.h > create mode 100644 drivers/gpu/drm/panel/panel-dsi-cm.c > > > base-commit: 54ba965bb3873eca6098ddf04e3a8d7bba1b5557 > -- > 2.25.0 >