[PATCH] drm/imx: imx-ldb: do not try to dereference crtc->state->state in encoder mode_set
On Mon, Jul 25, 2016 at 12:42:50PM +0200, Philipp Zabel wrote: > Am Freitag, den 22.07.2016, 18:49 +0200 schrieb Daniel Vetter: > > On Fri, Jul 22, 2016 at 12:57:15PM +0200, Philipp Zabel wrote: > > > Am Freitag, den 22.07.2016, 11:35 +0200 schrieb Daniel Vetter: > > > [...] > > > > Proper fix would be to roll out atomic_ versions of all teh encoder > > > > callbacks where we additionally pass both the crtc state and the > > > > connector > > > > state. Then there's no need for walking connector lists like that. And > > > > in > > > > the atomic helpers those two states are always readily available, and > > > > passing them down to callbacks is also what we will do in i915. I'd be > > > > happy to merge such a patch. > > > > > > Thanks, that is a good idea. Which encoder callbacks besides mode_set -> > > > atomic_mode_set are you thinking of, though? enable/disable? > > > > Yes. The only other one is mode_fixup, and we already have atomic_check as > > the fancy replacement for that. Please align the parameter ordering with > > encoder->atomic_check for consistency. > > Which crtc state should be passed to encoder atomic_disable? > old_crtc_state? Yes. > > Also when you do that patch, pls don't forget the vtable kerneldoc > > comments, and make sure there's no warnings/issues and it renders nicely > > using > > > > $ make htmldocs > > > > Since 4.8 will feature a new sphinx-based toolchain, pls run that command > > on top of linux-next (for the latest sphinx fixes from docs-next). > > -Daniel > > Ok, I'll have a look. Otoh, if there's not yet a need then maybe it doesn't yet make sense to add the full set of hooks. Might be worth it to skim driver's encoder callbacks for connector loops and similar things. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
[PATCH] drm/imx: imx-ldb: do not try to dereference crtc->state->state in encoder mode_set
Am Freitag, den 22.07.2016, 18:49 +0200 schrieb Daniel Vetter: > On Fri, Jul 22, 2016 at 12:57:15PM +0200, Philipp Zabel wrote: > > Am Freitag, den 22.07.2016, 11:35 +0200 schrieb Daniel Vetter: > > [...] > > > Proper fix would be to roll out atomic_ versions of all teh encoder > > > callbacks where we additionally pass both the crtc state and the connector > > > state. Then there's no need for walking connector lists like that. And in > > > the atomic helpers those two states are always readily available, and > > > passing them down to callbacks is also what we will do in i915. I'd be > > > happy to merge such a patch. > > > > Thanks, that is a good idea. Which encoder callbacks besides mode_set -> > > atomic_mode_set are you thinking of, though? enable/disable? > > Yes. The only other one is mode_fixup, and we already have atomic_check as > the fancy replacement for that. Please align the parameter ordering with > encoder->atomic_check for consistency. Which crtc state should be passed to encoder atomic_disable? old_crtc_state? > Also when you do that patch, pls don't forget the vtable kerneldoc > comments, and make sure there's no warnings/issues and it renders nicely > using > > $ make htmldocs > > Since 4.8 will feature a new sphinx-based toolchain, pls run that command > on top of linux-next (for the latest sphinx fixes from docs-next). > -Daniel Ok, I'll have a look. thanks Philipp
[PATCH] drm/imx: imx-ldb: do not try to dereference crtc->state->state in encoder mode_set
On Fri, Jul 22, 2016 at 12:57:15PM +0200, Philipp Zabel wrote: > Am Freitag, den 22.07.2016, 11:35 +0200 schrieb Daniel Vetter: > [...] > > Proper fix would be to roll out atomic_ versions of all teh encoder > > callbacks where we additionally pass both the crtc state and the connector > > state. Then there's no need for walking connector lists like that. And in > > the atomic helpers those two states are always readily available, and > > passing them down to callbacks is also what we will do in i915. I'd be > > happy to merge such a patch. > > Thanks, that is a good idea. Which encoder callbacks besides mode_set -> > atomic_mode_set are you thinking of, though? enable/disable? Yes. The only other one is mode_fixup, and we already have atomic_check as the fancy replacement for that. Please align the parameter ordering with encoder->atomic_check for consistency. Also when you do that patch, pls don't forget the vtable kerneldoc comments, and make sure there's no warnings/issues and it renders nicely using $ make htmldocs Since 4.8 will feature a new sphinx-based toolchain, pls run that command on top of linux-next (for the latest sphinx fixes from docs-next). -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
[PATCH] drm/imx: imx-ldb: do not try to dereference crtc->state->state in encoder mode_set
Am Freitag, den 22.07.2016, 11:35 +0200 schrieb Daniel Vetter: [...] > Proper fix would be to roll out atomic_ versions of all teh encoder > callbacks where we additionally pass both the crtc state and the connector > state. Then there's no need for walking connector lists like that. And in > the atomic helpers those two states are always readily available, and > passing them down to callbacks is also what we will do in i915. I'd be > happy to merge such a patch. Thanks, that is a good idea. Which encoder callbacks besides mode_set -> atomic_mode_set are you thinking of, though? enable/disable? regards Philipp
[PATCH] drm/imx: imx-ldb: do not try to dereference crtc->state->state in encoder mode_set
On Fri, Jul 22, 2016 at 10:59:51AM +0200, Philipp Zabel wrote: > The code in imx_ldb_encoder_mode_set crashes with a NULL pointer > dereference trying to access crtc->state->state, which was previously > cleared by drm_atomic_helper_swap_state: > > Unable to handle kernel NULL pointer dereference at virtual address > 0010 > pgd = ae08c000 > [0010] *pgd=3e00e831, *pte=, *ppte= > Internal error: Oops: 17 [#1] PREEMPT SMP ARM > Modules linked in: > CPU: 1 PID: 102 Comm: kmsfb-manage Not tainted 4.7.0-rc5+ #232 > Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree) > task: ae058c40 ti: ae04e000 task.ti: ae04e000 > PC is at imx_ldb_encoder_mode_set+0x138/0x2f8 > LR is at 0xae881818 > pc : [<8051a8c8>]lr : []psr: 600f0013 > sp : ae04fc70 ip : ae04fbb0 fp : ae04fcbc > r10: ae8ea018 r9 : r8 : ae246418 > r7 : ae8ea010 r6 : ae8ea308 r5 : r4 : > r3 : r2 : r1 : 0110 r0 : > Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none > Control: 10c5387d Table: 3e08c04a DAC: 0051 > Process kmsfb-manage (pid: 102, stack limit = 0xae04e210) > Stack: (0xae04fc70 to 0xae05) > fc60: 043ce660 0001 009e > 043ce660 > fc80: 0002 af75cf50 1009 ae23f440 0001 > ae246418 > fca0: 8155a210 ae8ea308 8093c364 ae2464e0 ae04fcec ae04fcc0 804ef350 > 8051a79c > fcc0: 0004 0004 ae23f440 af3f9000 ae881818 8155a210 af1af200 > ae8ea020 > fce0: ae04fd1c ae04fcf0 80519124 804ef060 ae04fd34 > > fd00: ae881818 ae23f440 80d4ec8c ae04fd34 ae04fd20 804f00b4 > 80518fac > fd20: ae23f440 ae04fd54 ae04fd38 804f2190 804f0074 ae23f440 > af3f9000 > fd40: ae04fdd4 ae881818 ae04fd6c ae04fd58 80516390 804f20f4 ae23f440 > > fd60: ae04fd8c ae04fd70 804f26f4 80516348 ae23a000 ae881818 0001 > af3f9000 > fd80: ae04fdac ae04fd90 80502c58 804f2678 ae04fe50 ae23f400 0001 > af3f9000 > fda0: ae04fe1c ae04fdb0 80507a1c 80502bf8 ae23a000 ae058c40 af1af200 > ae23f400 > fdc0: ae23a000 af3f9000 ae881818 ae23a00c 80176c7c ae23a000 ae881818 > af1af200 > fde0: ae23f400 0001 ae04fe1c 0051 ae04fe50 > 8155a210 > fe00: 80932060 c06864a2 af3f9000 ae246200 ae04fefc ae04fe20 804f9718 > 805074e8 > fe20: ae04feac ae04fe30 80177360 8017631c 805074dc 0068 0068 > 0062 > fe40: 0068 00a2 ae04fe50 7ef29688 7ef29c40 0001 > 0018 > fe60: 0026 0001 000115bc 05010500 > 05a0059f > fe80: 0320 03360321 0337 003c 0040 30383231 > 30303878 > fea0: 4000 > aea6a140 > fec0: 80d77b71 80283110 600f0013 7ef29688 af342bb0 > ae250b40 > fee0: 80275440 0003 ae04e000 ae04ff7c ae04ff00 80274ac8 > 804f957c > ff00: 80283128 80179030 80282fd8 ae1e 003d > aea6a1d0 > ff20: 0002 0003 4000 007f8c60 c06864a2 7ef29688 ae04e000 > > ff40: ae04ff6c ae04ff50 80283260 80282fe4 00017050 ae250b41 0003 > ae250b40 > ff60: c06864a2 7ef29688 ae04e000 ae04ffa4 ae04ff80 80275440 > 80274a20 > ff80: 00017050 0001 007f8c60 0036 801088a4 ae04e000 > ae04ffa8 > ffa0: 80108700 80275408 00017050 0001 0003 c06864a2 7ef29688 > 000115bc > ffc0: 00017050 0001 007f8c60 0036 0003 0026 > 0018 > ffe0: 00016f28 7ef29684 b7d9 76e4a1e6 400f0030 0003 3ff7e861 > 3ff7ec61 > Backtrace: > [<8051a790>] (imx_ldb_encoder_mode_set) from [<804ef350>] > (drm_atomic_helper_commit_modeset_disables+0x2fc/0x3f0) > r10:ae2464e0 r9:8093c364 r8:ae8ea308 r7:8155a210 r6:ae246418 r5:0001 > r4:ae23f440 > [<804ef054>] (drm_atomic_helper_commit_modeset_disables) from > [<80519124>] (imx_drm_atomic_commit_tail+0x184/0x1e0) > r10:ae8ea020 r9:af1af200 r8:8155a210 r7:ae881818 r6:af3f9000 r5:ae23f440 > r4:0004 r3:0004 > [<80518fa0>] (imx_drm_atomic_commit_tail) from [<804f00b4>] > (commit_tail+0x4c/0x68) > r6: r5:80d4ec8c r4:ae23f440 > [<804f0068>] (commit_tail) from [<804f2190>] > (drm_atomic_helper_commit+0xa8/0xd4) > r5: r4:ae23f440 > [<804f20e8>] (drm_atomic_helper_commit) from [<80516390>] > (drm_atomic_commit+0x54/0x74) > r7:ae881818 r6:ae04fdd4 r5:af3f9000 r4:ae23f440 > [<8051633c>] (drm_atomic_commit) from [<804f26f4>] > (drm_atomic_helper_set_config+0x88/0xac) > r5: r4:ae23f440 > [<804f266c>] (drm_atomic_helper_set_config) from [<80502c58>] > (drm_mode_set_config_internal+0x6c/0xf4) > r7:af3f9000 r6:0001 r5:ae881818 r4:ae23a000 > [<80502bec>]
[PATCH] drm/imx: imx-ldb: do not try to dereference crtc->state->state in encoder mode_set
Am Freitag, den 22.07.2016, 10:59 +0200 schrieb Philipp Zabel: > + struct drm_crtc_state *crtc_state = encoder->crtc->state; Sorry, this is superfluous. I'll drop it.
[PATCH] drm/imx: imx-ldb: do not try to dereference crtc->state->state in encoder mode_set
The code in imx_ldb_encoder_mode_set crashes with a NULL pointer dereference trying to access crtc->state->state, which was previously cleared by drm_atomic_helper_swap_state: Unable to handle kernel NULL pointer dereference at virtual address 0010 pgd = ae08c000 [0010] *pgd=3e00e831, *pte=, *ppte= Internal error: Oops: 17 [#1] PREEMPT SMP ARM Modules linked in: CPU: 1 PID: 102 Comm: kmsfb-manage Not tainted 4.7.0-rc5+ #232 Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree) task: ae058c40 ti: ae04e000 task.ti: ae04e000 PC is at imx_ldb_encoder_mode_set+0x138/0x2f8 LR is at 0xae881818 pc : [<8051a8c8>]lr : []psr: 600f0013 sp : ae04fc70 ip : ae04fbb0 fp : ae04fcbc r10: ae8ea018 r9 : r8 : ae246418 r7 : ae8ea010 r6 : ae8ea308 r5 : r4 : r3 : r2 : r1 : 0110 r0 : Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none Control: 10c5387d Table: 3e08c04a DAC: 0051 Process kmsfb-manage (pid: 102, stack limit = 0xae04e210) Stack: (0xae04fc70 to 0xae05) fc60: 043ce660 0001 009e 043ce660 fc80: 0002 af75cf50 1009 ae23f440 0001 ae246418 fca0: 8155a210 ae8ea308 8093c364 ae2464e0 ae04fcec ae04fcc0 804ef350 8051a79c fcc0: 0004 0004 ae23f440 af3f9000 ae881818 8155a210 af1af200 ae8ea020 fce0: ae04fd1c ae04fcf0 80519124 804ef060 ae04fd34 fd00: ae881818 ae23f440 80d4ec8c ae04fd34 ae04fd20 804f00b4 80518fac fd20: ae23f440 ae04fd54 ae04fd38 804f2190 804f0074 ae23f440 af3f9000 fd40: ae04fdd4 ae881818 ae04fd6c ae04fd58 80516390 804f20f4 ae23f440 fd60: ae04fd8c ae04fd70 804f26f4 80516348 ae23a000 ae881818 0001 af3f9000 fd80: ae04fdac ae04fd90 80502c58 804f2678 ae04fe50 ae23f400 0001 af3f9000 fda0: ae04fe1c ae04fdb0 80507a1c 80502bf8 ae23a000 ae058c40 af1af200 ae23f400 fdc0: ae23a000 af3f9000 ae881818 ae23a00c 80176c7c ae23a000 ae881818 af1af200 fde0: ae23f400 0001 ae04fe1c 0051 ae04fe50 8155a210 fe00: 80932060 c06864a2 af3f9000 ae246200 ae04fefc ae04fe20 804f9718 805074e8 fe20: ae04feac ae04fe30 80177360 8017631c 805074dc 0068 0068 0062 fe40: 0068 00a2 ae04fe50 7ef29688 7ef29c40 0001 0018 fe60: 0026 0001 000115bc 05010500 05a0059f fe80: 0320 03360321 0337 003c 0040 30383231 30303878 fea0: 4000 aea6a140 fec0: 80d77b71 80283110 600f0013 7ef29688 af342bb0 ae250b40 fee0: 80275440 0003 ae04e000 ae04ff7c ae04ff00 80274ac8 804f957c ff00: 80283128 80179030 80282fd8 ae1e 003d aea6a1d0 ff20: 0002 0003 4000 007f8c60 c06864a2 7ef29688 ae04e000 ff40: ae04ff6c ae04ff50 80283260 80282fe4 00017050 ae250b41 0003 ae250b40 ff60: c06864a2 7ef29688 ae04e000 ae04ffa4 ae04ff80 80275440 80274a20 ff80: 00017050 0001 007f8c60 0036 801088a4 ae04e000 ae04ffa8 ffa0: 80108700 80275408 00017050 0001 0003 c06864a2 7ef29688 000115bc ffc0: 00017050 0001 007f8c60 0036 0003 0026 0018 ffe0: 00016f28 7ef29684 b7d9 76e4a1e6 400f0030 0003 3ff7e861 3ff7ec61 Backtrace: [<8051a790>] (imx_ldb_encoder_mode_set) from [<804ef350>] (drm_atomic_helper_commit_modeset_disables+0x2fc/0x3f0) r10:ae2464e0 r9:8093c364 r8:ae8ea308 r7:8155a210 r6:ae246418 r5:0001 r4:ae23f440 [<804ef054>] (drm_atomic_helper_commit_modeset_disables) from [<80519124>] (imx_drm_atomic_commit_tail+0x184/0x1e0) r10:ae8ea020 r9:af1af200 r8:8155a210 r7:ae881818 r6:af3f9000 r5:ae23f440 r4:0004 r3:0004 [<80518fa0>] (imx_drm_atomic_commit_tail) from [<804f00b4>] (commit_tail+0x4c/0x68) r6: r5:80d4ec8c r4:ae23f440 [<804f0068>] (commit_tail) from [<804f2190>] (drm_atomic_helper_commit+0xa8/0xd4) r5: r4:ae23f440 [<804f20e8>] (drm_atomic_helper_commit) from [<80516390>] (drm_atomic_commit+0x54/0x74) r7:ae881818 r6:ae04fdd4 r5:af3f9000 r4:ae23f440 [<8051633c>] (drm_atomic_commit) from [<804f26f4>] (drm_atomic_helper_set_config+0x88/0xac) r5: r4:ae23f440 [<804f266c>] (drm_atomic_helper_set_config) from [<80502c58>] (drm_mode_set_config_internal+0x6c/0xf4) r7:af3f9000 r6:0001 r5:ae881818 r4:ae23a000 [<80502bec>] (drm_mode_set_config_internal) from [<80507a1c>] (drm_mode_setcrtc+0x540/0x5b8) r7:af3f9000 r6:0001 r5:ae23f400 r4:ae04fe50 [<805074dc>] (drm_mode_setcrtc) from [<804f9718>] (drm_ioctl+0x1a8/0x46c) r10:ae246200 r9:af3f9000 r8:c06864a2 r7:80932060 r6:8155a210
