Re: [PATCH v2 1/3] imx-drm: Add mx6 hdmi transmitter support
Another problem. After performing several modesets, the IPU seems to lock up and produce no syncs or output data. I've seen this many times over the last week while testing out various aspects of imx-drm, and had put it down to problems with the clocking arrangement getting its settings wrong. Now that I've sorted all that though, and I still have the problem, there's something else going on. What I see is: - the HDMI clock is running correctly (right frequency and unmodulated) - the TMDS data lines show signs of there being some data (probably control, guard bands and data islands from the frame composer in the HDMI interface). The data lines are definitely lacking image data though. - reading the various status registers indicates that all FIFOs within the IPU are empty. - the attached TV says that there is no HDMI signal. One of my tests has been to cycle through all display resolutions from the smallest width to the largest, leaving each one set for 30 seconds. This will occasionally provoke the problem, but obviously is rather slow to do so. I tried this with a less demanding test last night as far as a change in the settings: switching between 720p at 50 and 60Hz. The clocks for these two modes are the same at 74.25MHz, and the vertical timing parameters are identical. The only timing difference is with the horizontal parameters: 1280x720 (0x41) 74.2MHz +HSync +VSync +preferred h: width 1280 start 1390 end 1430 total 1650 skew0 clock 45.0KHz v: height 720 start 725 end 730 total 750 clock 60.0Hz 1280x720 (0x4f) 74.2MHz +HSync +VSync h: width 1280 start 1720 end 1760 total 1980 skew0 clock 37.5KHz v: height 720 start 725 end 730 total 750 clock 50.0Hz This dies within a couple of minutes. I haven't gathered enough information to tell whether it always dies when switching from 50 - 60Hz or whether it's any switch. My test for this is basically: while :; do xrandr -s 1280x720 -r 50 sleep 5 xrandr -s 1280x720 -r 60 sleep 5 done ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 1/3] imx-drm: Add mx6 hdmi transmitter support
On Sun, Oct 20, 2013 at 02:00:57PM +0100, Russell King - ARM Linux wrote: As for imx-drm, there was a warning which preceded that oops. Here's the full log, below the - marker - this is from unbinding the imx-drm module, and then trying to reboot. imx-drm is really very broken in the way it tries to bend DRM to be used in DT - it doesn't consider the lifetime for anything like the CRTCs, connectors or encoders. All these have empty .destroy functions to them. If we unbind imx-drm, the top level drm_device tries to be destroyed, but it leaves behind all the CRTCs, connectors and encoders, causing the first warning because none of the framebuffers got cleaned up through that destruction (because the functions did nothing.) The second one is through trying to clean up the framebuffer, which is still in use. The third one is caused because there's still allocated memory objects against the DRM memory manager - again, because nothing has been cleaned up. Right, so how imx-drm works as far as DRM initialization is by a wing and a prayer at the moment. It works like this - the driver relies heavily upon this sequence: - imx_drm_init() - creates an imx_drm_device structure to contain references to other parts. - registers the imx-drm platform device and an associated structure. - the platform device is immediately probed, causing it to be registered with the DRM subsystem. - the DRM subsystem creates the drm_device structure, and calls the drivers -load method. - the driver initialises some basic data, places a pointer to the drm_device into the imx_drm_device and returns - imx_pd_driver_init() - registers the imx_pd_driver platform device driver for DT devices with a compatible string of fsl,imx-parallel-display - such devices will be immediately probed - these allocate an imx_parallel_display structure, which contains a drm_connector and drm_encoder structure embedded within. - these structures are registered into the core of imx_drm, and via the imx_drm_device structure, are both attached to the drm_device immediately. - imx_tve_driver_init() essentially the same as imx_pd_driver_init() - imx_ldb_driver_init() essentially the same as imx_pd_driver_init() - imx_ipu_driver_init() - registers a platform driver fot DT devices with a compatible string of fsl,imx51-ipu, fsl,imx53-ipu, or fsl,imx6q-ipu. - initialises such devices, and creates two new platform devices called imx-ipuv3-crtc, one for each display interface. - ipu_drm_driver_init() - registers a platform driver for imx-ipuv3-crtc devices. - for each device found - it allocates a ipu_crtc device structure, which embeds a drm_crtc structure. - it registers a CRTC via imx_drm_add_crtc(). - this allocates an imx_drm_crtc structure, and eventually registers the drm_crtc structure against the drm_device - imx_hdmi_driver_init similar to imx_pd_driver_init All that sequence is in init level 6. The last bit comes in init level 7 (the late_initcall): - imx_fb_helper_init() - this grabs the drm_device, and calls drm_fbdev_cma_init() on it hoping that we know the number of CRTCs at this point. This is held indefinitely. - the resulting drm_fbdev_cma is saved into the imx_drm_device. Now, if the imx-drm device is unbound from its driver, all hell breaks loose - none of these crtc/connector/encoder structures have a meaningful destroy function - their destruction is all in their individual driver remove functions. This causes some warnings to be spat out from DRM. Amongst this is the last_close callback which looks at the imx_drm_device, sees that drm_fbdev_cma is registered against it, and calls drm_fbdev_cma_restore_mode() on it. drm_fbdev_cma contains objects which store a pointer to the drm_device structure that it was registered against, which exists at this point, so everything is fine. The unload proceeds, and eventually the drm_device is freed. Now, if we rebind the imx-drm device, causing the probe actions above to be repeated. imx_drm_device still contains a pointer to the drm_fbdev_cma object that was allocated... Let's ignore the fact that none of the sub-modules have re-initialised anything against this new drm_device. The real fun comes when you try and unbind it again. This time, the drm_device which is being torn down isn't the one in drm_fbdev_cma, but we still call drm_fbdev_cma_restore_mode(). This tries to get a mutex on the _original_ drm_device-mode_config.mutex, which has been freed. The result is a kernel oops. Now, several things stand out here: piece-meal construction of a drm_device in this manner is unsafe: - it relies heavily on all devices already being present at the time that the above sequence starts, and it assumes that the drivers will probe immediately, as soon as they are registered. - the late_initcall() is really a barrier on the
Re: [PATCH v2 1/3] imx-drm: Add mx6 hdmi transmitter support
On Sun, Oct 20, 2013 at 05:31:56PM +0100, Russell King - ARM Linux wrote: On Sun, Oct 20, 2013 at 02:00:57PM +0100, Russell King - ARM Linux wrote: As for imx-drm, there was a warning which preceded that oops. Here's the full log, below the - marker - this is from unbinding the imx-drm module, and then trying to reboot. imx-drm is really very broken in the way it tries to bend DRM to be used in DT - it doesn't consider the lifetime for anything like the CRTCs, connectors or encoders. All these have empty .destroy functions to them. If we unbind imx-drm, the top level drm_device tries to be destroyed, but it leaves behind all the CRTCs, connectors and encoders, causing the first warning because none of the framebuffers got cleaned up through that destruction (because the functions did nothing.) The second one is through trying to clean up the framebuffer, which is still in use. The third one is caused because there's still allocated memory objects against the DRM memory manager - again, because nothing has been cleaned up. Right, so how imx-drm works as far as DRM initialization is by a wing and a prayer at the moment. It works like this - the driver relies heavily upon this sequence: - imx_drm_init() - creates an imx_drm_device structure to contain references to other parts. - registers the imx-drm platform device and an associated structure. - the platform device is immediately probed, causing it to be registered with the DRM subsystem. - the DRM subsystem creates the drm_device structure, and calls the drivers -load method. - the driver initialises some basic data, places a pointer to the drm_device into the imx_drm_device and returns - imx_pd_driver_init() - registers the imx_pd_driver platform device driver for DT devices with a compatible string of fsl,imx-parallel-display - such devices will be immediately probed - these allocate an imx_parallel_display structure, which contains a drm_connector and drm_encoder structure embedded within. - these structures are registered into the core of imx_drm, and via the imx_drm_device structure, are both attached to the drm_device immediately. - imx_tve_driver_init() essentially the same as imx_pd_driver_init() - imx_ldb_driver_init() essentially the same as imx_pd_driver_init() - imx_ipu_driver_init() - registers a platform driver fot DT devices with a compatible string of fsl,imx51-ipu, fsl,imx53-ipu, or fsl,imx6q-ipu. - initialises such devices, and creates two new platform devices called imx-ipuv3-crtc, one for each display interface. - ipu_drm_driver_init() - registers a platform driver for imx-ipuv3-crtc devices. - for each device found - it allocates a ipu_crtc device structure, which embeds a drm_crtc structure. - it registers a CRTC via imx_drm_add_crtc(). - this allocates an imx_drm_crtc structure, and eventually registers the drm_crtc structure against the drm_device - imx_hdmi_driver_init similar to imx_pd_driver_init All that sequence is in init level 6. The last bit comes in init level 7 (the late_initcall): - imx_fb_helper_init() - this grabs the drm_device, and calls drm_fbdev_cma_init() on it hoping that we know the number of CRTCs at this point. This is held indefinitely. - the resulting drm_fbdev_cma is saved into the imx_drm_device. Now, if the imx-drm device is unbound from its driver, all hell breaks loose - none of these crtc/connector/encoder structures have a meaningful destroy function - their destruction is all in their individual driver remove functions. This causes some warnings to be spat out from DRM. Amongst this is the last_close callback which looks at the imx_drm_device, sees that drm_fbdev_cma is registered against it, and calls drm_fbdev_cma_restore_mode() on it. drm_fbdev_cma contains objects which store a pointer to the drm_device structure that it was registered against, which exists at this point, so everything is fine. The unload proceeds, and eventually the drm_device is freed. Now, if we rebind the imx-drm device, causing the probe actions above to be repeated. imx_drm_device still contains a pointer to the drm_fbdev_cma object that was allocated... Let's ignore the fact that none of the sub-modules have re-initialised anything against this new drm_device. The real fun comes when you try and unbind it again. This time, the drm_device which is being torn down isn't the one in drm_fbdev_cma, but we still call drm_fbdev_cma_restore_mode(). This tries to get a mutex on the _original_ drm_device-mode_config.mutex, which has been freed. The result is a kernel oops. Now, several things stand out here: piece-meal construction of a drm_device in this manner is unsafe: - it relies heavily on all devices already being present at the time
Re: [PATCH v2 1/3] imx-drm: Add mx6 hdmi transmitter support
On Wed, Oct 16, 2013 at 11:31:07AM -0700, Greg Kroah-Hartman wrote: On Wed, Oct 16, 2013 at 07:07:35PM +0100, Russell King - ARM Linux wrote: Sorry, but I don't think imx-drm is driving the hardware correctly, and I know that Greg wants it moved out of drivers/staging, but frankly it seems to be far from ready for that. Certainly the HDMI parts seems to be especially problematical. I want it out of staging if it's working properly. Yours is the first report of it not working properly, and in fact, probably one of the first users of the driver, as I haven't gotten any reports of it working or not at all over the years. Next problem... unbinding, rebinding, and then unbinding the imx-drm device produces the nice oops below. I've not debugged this yet. Alignment trap: not handling instruction e1932f9f at [c00758ec] Unhandled fault: alignment exception (0x001) at 0x6e72666f Internal error: : 1 [#1] SMP ARM Modules linked in: fuse bnep rfcomm bluetooth CPU: 0 PID: 1125 Comm: bash Tainted: GW3.12.0-rc4+ #123 task: d0d6ec00 ti: d7ff2000 task.ti: d7ff2000 PC is at __lock_acquire+0x1a8/0x1e14 LR is at lock_acquire+0x68/0x7c pc : [c00758f0]lr : [c0077aa8]psr: 20070093 sp : d7ff3cc8 ip : fp : d7ff3d54 r10: db2a6334 r9 : r8 : 6e72656b r7 : c0846208 r6 : c08852cc r5 : d0d6ec00 r4 : 0002 r3 : 6e72666f r2 : db2a6334 r1 : 0001 r0 : d7ff2000 Flags: nzCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment user Control: 10c53c7d Table: 20df804a DAC: 0015 Process bash (pid: 1125, stack limit = 0xd7ff2240) Stack: (0xd7ff3cc8 to 0xd7ff4000) 3cc0: 0006 0007 c0cd22f0 0006 d7ff3d1c d7ff3ce8 3ce0: c00782f8 c0075050 0001 db801f00 d7ff2000 c0078648 d7ff2000 3d00: 0001 d7ff3e08 d7ff3e60 d7ff3dd8 d7ff3d3c d7ff3d20 d7ff3e08 3d20: d7ff3d7c d7ff3d30 c0072a58 d7ff2000 60070013 d0d6ec00 d7ff2000 3d40: c06679c8 d0d6ec00 d7ff3d8c d7ff3d58 c0077aa8 c0075754 0002 3d60: c02ff27c 0002 db2a62fc c02ff27c c08852cc 3d80: d7ff3de4 d7ff3d90 c05f80c4 c0077a4c 0002 c02ff27c c0072a20 3da0: dad64000 d7ff3e24 d7ff3df8 d7ff3e04 d7ff3e5c d0d6f000 c013f1c8 db322880 3dc0: db2a6440 db2a6000 c0872568 0008 c06679c8 db286000 d7ff3e04 d7ff3de8 3de0: c02ff27c c05f8074 db280f00 db322880 db2a6000 db29b810 d7ff3e1c d7ff3e08 3e00: c02f13ac c02ff268 d0e55000 c087265c d7ff3e2c d7ff3e20 c0464d7c c02f1398 3e20: d7ff3e54 d7ff3e30 c02f506c c0464d68 d0e55000 c087265c db29b810 c0872568 3e40: 0008 db286000 d7ff3e74 d7ff3e58 c02fa284 c02f503c c087265c c087265c 3e60: db29b810 0008 d7ff3e8c d7ff3e78 c02fb900 c02fa258 db29b810 c0872678 3e80: d7ff3e9c d7ff3e90 c0464d54 c02fb8d4 d7ff3eac d7ff3ea0 c0312bfc c0464d44 3ea0: d7ff3ec4 d7ff3eb0 c03113b0 c0312be8 db29b844 db29b810 d7ff3edc d7ff3ec8 3ec0: c0311434 c0311344 c0872678 c085b588 d7ff3efc d7ff3ee0 c03103ac c0311418 3ee0: c941b300 c941b318 d7ff3f70 db28c280 d7ff3f0c d7ff3f00 c030f934 c0310338 3f00: d7ff3f3c d7ff3f10 c013d7d0 c030f918 d7ff3f70 dbb5e600 0008 003ba408 3f20: d7ff3f70 0008 d7ff3f6c d7ff3f40 c00db77c c013d6d4 3f40: 0001 c000eb44 dbb5e600 d7ff3f70 003ba408 0008 3f60: d7ff3fa4 d7ff3f70 c00dbb58 c00db6b8 d7ff3f94 b6f7fa78 3f80: 0008 003ba408 0004 c000eb44 d7ff2000 d7ff3fa8 3fa0: c000e980 c00dbb18 b6f7fa78 0008 0001 003ba408 0008 3fc0: b6f7fa78 0008 003ba408 0004 bee5c9ec 000a6094 003f7f08 3fe0: bee5c96c b6eee747 b6f2611c 40070010 0001 0138 0020 Backtrace: [c0075748] (__lock_acquire+0x0/0x1e14) from [c0077aa8] (lock_acquire+0x68/0x7c) [c0077a40] (lock_acquire+0x0/0x7c) from [c05f80c4] (mutex_lock_nested+0x5c/0x394) r7: r6:c08852cc r5:c02ff27c r4:db2a62fc [c05f8068] (mutex_lock_nested+0x0/0x394) from [c02ff27c] (drm_modeset_lock_all+0x20/0x58) [c02ff25c] (drm_modeset_lock_all+0x0/0x58) from [c02f13ac] (drm_fbdev_cma_restore_mode+0x20/0x34) r6:db29b810 r5:db2a6000 r4:db322880 r3:db280f00 [c02f138c] (drm_fbdev_cma_restore_mode+0x0/0x34) from [c0464d7c] (imx_drm_driver_lastclose+0x20/0x24) r5:c087265c r4:d0e55000 [c0464d5c] (imx_drm_driver_lastclose+0x0/0x24) from [c02f506c] (drm_lastclose+0x3c/0x174) [c02f5030] (drm_lastclose+0x0/0x174) from [c02fa284] (drm_put_dev+0x38/0x154) [c02fa24c] (drm_put_dev+0x0/0x154) from [c02fb900] (drm_platform_exit+0x38/0x5c) r7:0008 r6:db29b810 r5:c087265c r4:c087265c [c02fb8c8] (drm_platform_exit+0x0/0x5c) from [c0464d54] (imx_drm_platform_remove+0x1c/0x24) r5:c0872678 r4:db29b810 [c0464d38] (imx_drm_platform_remove+0x0/0x24) from [c0312bfc] (platform_drv_remove+0x20/0x24) [c0312bdc] (platform_drv_remove+0x0/0x24) from [c03113b0] (__device_release_driver+0x78/0xd4) [c0311338] (__device_release_driver+0x0/0xd4) from [c0311434] (device_release_driver+0x28/0x34) r5:db29b810 r4:db29b844 [c031140c]
Re: [PATCH v2 1/3] imx-drm: Add mx6 hdmi transmitter support
Hi Russell, Am Mittwoch, den 16.10.2013, 20:02 +0100 schrieb Russell King - ARM Linux: On Wed, Oct 16, 2013 at 11:31:07AM -0700, Greg Kroah-Hartman wrote: On Wed, Oct 16, 2013 at 07:07:35PM +0100, Russell King - ARM Linux wrote: Sorry, but I don't think imx-drm is driving the hardware correctly, and I know that Greg wants it moved out of drivers/staging, but frankly it seems to be far from ready for that. Certainly the HDMI parts seems to be especially problematical. I want it out of staging if it's working properly. Yours is the first report of it not working properly, and in fact, probably one of the first users of the driver, as I haven't gotten any reports of it working or not at all over the years. Well, part of that is because I have this other thing called Armada DRM, which is a similar thing to imx-drm, except for the Marvell Dove SoCs, so it's been really quite easy to get a full Ubuntu 12.04 up and running on the IMX SoC I have here. As part of that effort, I'm now using my Armada DRM Xorg driver with imx-drm to test this stuff out. (Bearing in mind that IMX and Dove use the same Vivante GPU, there's some sense in getting my Xorg Armada DRM driver working on both.) I'm not aware of there being any X drivers for imx-drm (google turns up nothing), which might be a reason why it hasn't been well tested and has also languished in drivers/staging for soo long. Alternatively, maybe my google searching sucks, or it is out there somewhere but hidden from googlebot? X.Org isn't the center of the world anymore. For some general purpose distros this might still hold true for some time, but certainly not for the use-cases we test this driver with. For most of our embedded use-cases we currently use the FBdev emulation (phasing this out at the moment) or the apps are sitting right on top of the raw KMS APIs, rather than on top of a display server. We also did some experiments with Wayland (Weston) on top of imx-drm, which worked quite well. There might be dragons hiding in some corners for the more general purpose use-cases and we are happy to have you test the driver and provide valuable reports. To be fair, so far most of the problems I'm finding are with the HDMI addition to imx-drm rather than imx-drm itself: there's been the lockdep problem and the clock polarity problem which the HDMI stuff discovered. Looking at the todo list for moving it out of staging, we have: - get DRM Maintainer review for this code - Wait for common display framework to hit mainline and update the IPU driver to use it. This will most probably make changes to the devicetree bindings necessary. - Factor out more code to common helper functions - decide where to put the base driver. It is not specific to a subsystem and would be used by DRM/KMS and media/V4L2 (1) is quite a difficult task: I'm waiting for a review of the Armada DRM stuff, which I'm hoping will make the next merge window. Rob Clark is looking at that but has been distracted recently from completing it. (2) CDF... has been around in RFC according to LWN.net but doesn't seem to make much in the way of progress from what I can see, despite there allegedly being many developers show[ing] interest in the first RFC. CDF is over a year old now - first RFC 17 Aug 2012, second 22 Nov 2012, third 9 Aug 2013. Philipp has a prototype of CDF on imx-drm working internally and I suspect he will be able to post patches shortly after ELCE. (3) is an on-going effort and really isn't a reason for it to stay in staging. (4) is probably the biggest issue. The problem there is that the IMX display subsystem is very flexible: it's basically a set of DMA engines on the front, and behind that a fair number of modules implementing facilities like image rotation, display driving and image capture. Display driving alone involves setting up waveforms and writing some microcode to tell the thing what to do on certain events (like end of frame etc.) Hence it doesn't fit well into any particular Linux subsystem. In this regard, it's a victim of its own flexibility. We are aiming to do the same thing as the Tegra host1x driver: move the IPU core to drivers/gpu and export API for other drivers to use. This may mean we still have to keep the DRM part in staging (at least until the CDF thing is sorted, as this prevents us from setting the devicetree bindings in stone), but at least should be a step in the right direction. Regards, Lucas -- Pengutronix e.K. | Lucas Stach | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-5076 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ devel mailing list de...@linuxdriverproject.org
Re: [PATCH v2 1/3] imx-drm: Add mx6 hdmi transmitter support
Okay, next problem... As I described via the Cubox-i community last night on google+... I'm now at the point where certain resolutions and refreshes work fine (eg, 720p @ 50 or 60Hz, 1366x768, 1024x768). Others either don't display (1080p, 800x600, 848x480, 640x480), or have speckles, a line of corruption that flashes up, and blanks (576p @50 Hz, 480p @60Hz), possibly a result of loss of signal. I've been looking at the DMFC, suspecting a fifo problem, but that to me looks fine - we seem to always allocate 4 slots, with each slot having a bandwidth of 99Mpixels each. This in theory should give a maximum of 396Mpixels, which is more than plenty. The bandwidth calculation is probably wrong though - the peak bandwidth is actually the pixel clock since this is the rate at which pixels have to be supplied during a line, not the refresh * hdisplayed * vdisplayed - that's the average bandwidth over a full frame. However, if it was a DMFC problem, and as this only carries pixel data, I'd expect that not to cause loss of signal. I've checked the phy MPLL settings back to the manual for certain problem clock rates, and they also seem fine. I've polled the status register to see whether it's unstable, and it appears to remain locked. I think I should probe the HDMI signals, particularly the clock signal to see if that provides any useful clues. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 1/3] imx-drm: Add mx6 hdmi transmitter support
On Tue, Oct 15, 2013 at 10:17:07AM -0300, Fabio Estevam wrote:
On Tue, Oct 15, 2013 at 10:10 AM, Russell King - ARM Linux
li...@arm.linux.org.uk wrote:
Another point on patch 1. Sorry, I don't have patch 1 to reply to, it
seems it was deleted from linux-arm-kernel's moderation queue.
drm_mode_connector_attach_encoder() is called too early, before the
base.id field in the encoder has been initialised. This causes the
connectors encoder array to be empty, and userspace KMS to fail.
There's also bugs in the CSC setting too - it runs off the end of the
array and gcc warns about this. The code was clearly wrong.
You may wish to combine this patch with patch 1 to sort all that out.
For the patch below:
Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
Tested-by: Russell King rmk+ker...@arm.linux.org.uk
Thanks, Russell.
Will submit v3 when I am back to the office.
Okay, I still have a problem with HDMI: I have a magenta vertical line
down the left hand side of the frame, the displayed frame is shifted
right by the width of that line and the right hand side is missing some
pixels.
First off, the hsync position programmed into the HDMI registers appears
to be wrong.
I'm at a loss why imx-hdmi is obfuscated with a conversion from a
drm_display_mode structure to a fb_videomode. This adds additional
confusion and additional opportunities for bugs; this is probably
exactly why the hsync position is wrong.
In CEA-861-B for 720p @60Hz:
DE: __^^^
HS: ___^^^___
^ ^ ^
| | 220 clocks
| 40 clocks
110 clocks
The IMX6 manual says HSYINCINDELAY0 is Hsync active edge delay. Integer
number of pixel clock cycles from de non-active edge. So, this should be
110. Yet it ends up being programmed as 220, leading to a magenta vertical
bar down the left hand side of the display.
Now, if you look at Documentation/fb/framebuffer.txt, in this case, the
right margin should be 110 clocks, hsync len should be 40 and the left
margin should be 220 clocks.
However, this is not what your conversion to a fb_videomode does. It
reverses the left and right margin. A similar confusion also exists
in the conversion of the upper/lower margins too.
The DRM model is this (for 720p @60Hz):
0 1280 1390 1430 1650
|===||---|--|
^ ^^ ^ ^
start hdisplay hsync_start hsync_end htotal
The fb model is the same as the above but rotated:
left margindisplayedright margin hsync_len
|--|===||---|
So, the left margin is the bit between hsync_end and htotal, and the
right margin is the bit between hdisplay and hsync_start. Exactly
the same analysis applies to the upper/lower margins.
What I suggest is that the use of fb_videomode is entirely killed off
in this driver to remove this layer of confusion and instead the DRM
model is stuck to within this DRM driver.
Now on to the next problem. HSYNC/VSYNC polarity.
So, this is the mode which is set:
1280x720 (0x41) 74.2MHz +HSync +VSync *current +preferred
h: width 1280 start 1390 end 1430 total 1650 skew0 clock 45.0KHz
v: height 720 start 725 end 730 total 750 clock 60.0Hz
Note the positive HSync and VSync polarity - this is correct, backed
up by CEA-861-B.
The IPU DI0 is configured thusly in its general control register:
0x264 = 0x26, which is what is set when the requested mode
has DRM_MODE_FLAG_PHSYNC and DRM_MODE_FLAG_PVSYNC flags set.
However, if we look at the HDMI config: 0x121000 = 0x18. Active low
hsync/vsync. This is quite obvious when you look at the code -
convert_to_video_timing() does *nothing* at all when converting the
sync polarities, which means hdmi_av_composer() doesn't program them
appropriately. And yes, poking 0x78 into this register finally fixes
the problem.
Yet another reason why this silly conversion from one structure form
to another is a Very Bad Idea. Until I found this, I was merely going
to send a patch to sort out convert_to_video_timing(), but quite frankly
I'm going to kill this thing off right now.
Another thing:
static int imx_hdmi_setup(struct imx_hdmi *hdmi, struct drm_display_mode *mode)
{
int ret;
convert_to_video_timing(hdmi-fb_mode, mode);
hdmi_disable_overflow_interrupts(hdmi);
hdmi-vic = 6;
It's quite wrong to force every video mode set to be CEA mode 6. IIRC,
There is a function in DRM which will tell you the CEA mode number.
Again, I'll fix this in my patch rewriting this bit of the driver to
be correct.
I'm also suspicious of the HDMI Initialization Step comments, because
they make no sense:
/* HDMI Initialization Step B.4 */
static void imx_hdmi_enable_video_path(struct imx_hdmi *hdmi)
{
yet:
Re: [PATCH v2 1/3] imx-drm: Add mx6 hdmi transmitter support
On Wed, Oct 16, 2013 at 12:37:42PM -0700, Troy Kisky wrote:
On 10/16/2013 10:03 AM, Russell King - ARM Linux wrote:
On Tue, Oct 15, 2013 at 10:17:07AM -0300, Fabio Estevam wrote:
On Tue, Oct 15, 2013 at 10:10 AM, Russell King - ARM Linux
li...@arm.linux.org.uk wrote:
Another point on patch 1. Sorry, I don't have patch 1 to reply to, it
seems it was deleted from linux-arm-kernel's moderation queue.
drm_mode_connector_attach_encoder() is called too early, before the
base.id field in the encoder has been initialised. This causes the
connectors encoder array to be empty, and userspace KMS to fail.
There's also bugs in the CSC setting too - it runs off the end of the
array and gcc warns about this. The code was clearly wrong.
You may wish to combine this patch with patch 1 to sort all that out.
For the patch below:
Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
Tested-by: Russell King rmk+ker...@arm.linux.org.uk
Thanks, Russell.
Will submit v3 when I am back to the office.
Okay, I still have a problem with HDMI: I have a magenta vertical line
down the left hand side of the frame, the displayed frame is shifted
right by the width of that line and the right hand side is missing some
pixels.
First off, the hsync position programmed into the HDMI registers appears
to be wrong.
I'm at a loss why imx-hdmi is obfuscated with a conversion from a
drm_display_mode structure to a fb_videomode. This adds additional
confusion and additional opportunities for bugs; this is probably
exactly why the hsync position is wrong.
In CEA-861-B for 720p @60Hz:
DE: __^^^
HS: ___^^^___
^ ^ ^
| | 220 clocks
| 40 clocks
110 clocks
The IMX6 manual says HSYINCINDELAY0 is Hsync active edge delay. Integer
number of pixel clock cycles from de non-active edge. So, this should be
110. Yet it ends up being programmed as 220, leading to a magenta vertical
bar down the left hand side of the display.
Now, if you look at Documentation/fb/framebuffer.txt, in this case, the
right margin should be 110 clocks, hsync len should be 40 and the left
margin should be 220 clocks.
However, this is not what your conversion to a fb_videomode does. It
reverses the left and right margin. A similar confusion also exists
in the conversion of the upper/lower margins too.
The DRM model is this (for 720p @60Hz):
0 1280 1390 1430 1650
|===||---|--|
^ ^^ ^ ^
start hdisplay hsync_start hsync_end htotal
The fb model is the same as the above but rotated:
left margindisplayedright margin hsync_len
|--|===||---|
So, the left margin is the bit between hsync_end and htotal, and the
right margin is the bit between hdisplay and hsync_start. Exactly
the same analysis applies to the upper/lower margins.
What I suggest is that the use of fb_videomode is entirely killed off
in this driver to remove this layer of confusion and instead the DRM
model is stuck to within this DRM driver.
Now on to the next problem. HSYNC/VSYNC polarity.
So, this is the mode which is set:
1280x720 (0x41) 74.2MHz +HSync +VSync *current +preferred
h: width 1280 start 1390 end 1430 total 1650 skew0 clock
45.0KHz
v: height 720 start 725 end 730 total 750 clock
60.0Hz
Note the positive HSync and VSync polarity - this is correct, backed
up by CEA-861-B.
The IPU DI0 is configured thusly in its general control register:
0x264 = 0x26, which is what is set when the requested mode
has DRM_MODE_FLAG_PHSYNC and DRM_MODE_FLAG_PVSYNC flags set.
However, if we look at the HDMI config: 0x121000 = 0x18. Active low
hsync/vsync. This is quite obvious when you look at the code -
convert_to_video_timing() does *nothing* at all when converting the
sync polarities, which means hdmi_av_composer() doesn't program them
appropriately. And yes, poking 0x78 into this register finally fixes
the problem.
Yet another reason why this silly conversion from one structure form
to another is a Very Bad Idea. Until I found this, I was merely going
to send a patch to sort out convert_to_video_timing(), but quite frankly
I'm going to kill this thing off right now.
Another thing:
static int imx_hdmi_setup(struct imx_hdmi *hdmi, struct drm_display_mode
*mode)
{
int ret;
convert_to_video_timing(hdmi-fb_mode, mode);
hdmi_disable_overflow_interrupts(hdmi);
hdmi-vic = 6;
It's quite wrong to force every video mode set to be CEA mode 6. IIRC,
There is a function in DRM which will tell you the CEA mode number.
Again, I'll fix this in my patch rewriting this bit of the driver to
be correct.
I'm also
Re: [PATCH v2 1/3] imx-drm: Add mx6 hdmi transmitter support
On 10/16/2013 1:27 PM, Russell King - ARM Linux wrote:
On Wed, Oct 16, 2013 at 12:37:42PM -0700, Troy Kisky wrote:
On 10/16/2013 10:03 AM, Russell King - ARM Linux wrote:
On Tue, Oct 15, 2013 at 10:17:07AM -0300, Fabio Estevam wrote:
On Tue, Oct 15, 2013 at 10:10 AM, Russell King - ARM Linux
li...@arm.linux.org.uk wrote:
Another point on patch 1. Sorry, I don't have patch 1 to reply to, it
seems it was deleted from linux-arm-kernel's moderation queue.
drm_mode_connector_attach_encoder() is called too early, before the
base.id field in the encoder has been initialised. This causes the
connectors encoder array to be empty, and userspace KMS to fail.
There's also bugs in the CSC setting too - it runs off the end of the
array and gcc warns about this. The code was clearly wrong.
You may wish to combine this patch with patch 1 to sort all that out.
For the patch below:
Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
Tested-by: Russell King rmk+ker...@arm.linux.org.uk
Thanks, Russell.
Will submit v3 when I am back to the office.
Okay, I still have a problem with HDMI: I have a magenta vertical line
down the left hand side of the frame, the displayed frame is shifted
right by the width of that line and the right hand side is missing some
pixels.
First off, the hsync position programmed into the HDMI registers appears
to be wrong.
I'm at a loss why imx-hdmi is obfuscated with a conversion from a
drm_display_mode structure to a fb_videomode. This adds additional
confusion and additional opportunities for bugs; this is probably
exactly why the hsync position is wrong.
In CEA-861-B for 720p @60Hz:
DE: __^^^
HS: ___^^^___
^ ^ ^
| | 220 clocks
| 40 clocks
110 clocks
The IMX6 manual says HSYINCINDELAY0 is Hsync active edge delay. Integer
number of pixel clock cycles from de non-active edge. So, this should be
110. Yet it ends up being programmed as 220, leading to a magenta vertical
bar down the left hand side of the display.
Now, if you look at Documentation/fb/framebuffer.txt, in this case, the
right margin should be 110 clocks, hsync len should be 40 and the left
margin should be 220 clocks.
However, this is not what your conversion to a fb_videomode does. It
reverses the left and right margin. A similar confusion also exists
in the conversion of the upper/lower margins too.
The DRM model is this (for 720p @60Hz):
0 1280 1390 1430 1650
|===||---|--|
^ ^^ ^ ^
start hdisplay hsync_start hsync_end htotal
The fb model is the same as the above but rotated:
left margindisplayedright margin hsync_len
|--|===||---|
So, the left margin is the bit between hsync_end and htotal, and the
right margin is the bit between hdisplay and hsync_start. Exactly
the same analysis applies to the upper/lower margins.
What I suggest is that the use of fb_videomode is entirely killed off
in this driver to remove this layer of confusion and instead the DRM
model is stuck to within this DRM driver.
Now on to the next problem. HSYNC/VSYNC polarity.
So, this is the mode which is set:
1280x720 (0x41) 74.2MHz +HSync +VSync *current +preferred
h: width 1280 start 1390 end 1430 total 1650 skew0 clock
45.0KHz
v: height 720 start 725 end 730 total 750 clock 60.0Hz
Note the positive HSync and VSync polarity - this is correct, backed
up by CEA-861-B.
The IPU DI0 is configured thusly in its general control register:
0x264 = 0x26, which is what is set when the requested mode
has DRM_MODE_FLAG_PHSYNC and DRM_MODE_FLAG_PVSYNC flags set.
However, if we look at the HDMI config: 0x121000 = 0x18. Active low
hsync/vsync. This is quite obvious when you look at the code -
convert_to_video_timing() does *nothing* at all when converting the
sync polarities, which means hdmi_av_composer() doesn't program them
appropriately. And yes, poking 0x78 into this register finally fixes
the problem.
Yet another reason why this silly conversion from one structure form
to another is a Very Bad Idea. Until I found this, I was merely going
to send a patch to sort out convert_to_video_timing(), but quite frankly
I'm going to kill this thing off right now.
Another thing:
static int imx_hdmi_setup(struct imx_hdmi *hdmi, struct drm_display_mode *mode)
{
int ret;
convert_to_video_timing(hdmi-fb_mode, mode);
hdmi_disable_overflow_interrupts(hdmi);
hdmi-vic = 6;
It's quite wrong to force every video mode set to be CEA mode 6. IIRC,
There is a function in DRM which will tell you the CEA mode number.
Again, I'll fix this in my patch rewriting this bit of the driver to
be correct.
I'm also suspicious of the HDMI
Re: [PATCH v2 1/3] imx-drm: Add mx6 hdmi transmitter support
On Wed, Oct 16, 2013 at 02:03:17PM -0700, Troy Kisky wrote:
Freescale's kernel(imx_3.0.35_4.1.0) has this code
video/mxc_hdmi.c-/* Workaround to clear the overflow condition */
video/mxc_hdmi.c-static void mxc_hdmi_clear_overflow(void)
video/mxc_hdmi.c-{
video/mxc_hdmi.c- int count;
video/mxc_hdmi.c- u8 val;
video/mxc_hdmi.c-
video/mxc_hdmi.c- /* TMDS software reset */
video/mxc_hdmi.c: hdmi_writeb((u8)~HDMI_MC_SWRSTZ_TMDSSWRST_REQ,
HDMI_MC_SWRSTZ);
video/mxc_hdmi.c-
video/mxc_hdmi.c- val = hdmi_readb(HDMI_FC_INVIDCONF);
video/mxc_hdmi.c-
video/mxc_hdmi.c- if (cpu_is_mx6dl()) {
video/mxc_hdmi.c-hdmi_writeb(val, HDMI_FC_INVIDCONF);
video/mxc_hdmi.c-return;
video/mxc_hdmi.c- }
video/mxc_hdmi.c-
video/mxc_hdmi.c- for (count = 0 ; count 5 ; count++)
video/mxc_hdmi.c- hdmi_writeb(val, HDMI_FC_INVIDCONF);
video/mxc_hdmi.c-}
So, perhaps you need to move the software reset first.
Just tried that - and yes, it does work (I'm on i.MX 6Solo for which
cpu_is_mx6dl() would return true.) Well done!
Indeed yes, the workaround in the code Fabio has differs from the
procedure given in the errata.
Note that this gives a new problem: we shouldn't use cpu_is_mx6dl()
in drivers - differences like this should be specified via DT properties.
I think we need this to recognise both fsl,imx6q-hdmi and fsl,imx6dl-hdmi
so that this workaround can detect when its running on a Solo/DL SoC.
Well, I now have quite a pile of patches for the hdmi code. :(
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 1/3] imx-drm: Add mx6 hdmi transmitter support
Another point on patch 1. Sorry, I don't have patch 1 to reply to, it
seems it was deleted from linux-arm-kernel's moderation queue.
drm_mode_connector_attach_encoder() is called too early, before the
base.id field in the encoder has been initialised. This causes the
connectors encoder array to be empty, and userspace KMS to fail.
There's also bugs in the CSC setting too - it runs off the end of the
array and gcc warns about this. The code was clearly wrong.
You may wish to combine this patch with patch 1 to sort all that out.
For the patch below:
Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
Tested-by: Russell King rmk+ker...@arm.linux.org.uk
diff --git a/drivers/staging/imx-drm/imx-hdmi.c
b/drivers/staging/imx-drm/imx-hdmi.c
index e227eb1..ca0450b 100644
--- a/drivers/staging/imx-drm/imx-hdmi.c
+++ b/drivers/staging/imx-drm/imx-hdmi.c
@@ -507,7 +507,7 @@ static void imx_hdmi_update_csc_coeffs(struct imx_hdmi
*hdmi)
hdmi_writeb(hdmi, ((*csc_coeff)[0][2] 0xff), HDMI_CSC_COEF_A3_LSB);
hdmi_writeb(hdmi, ((*csc_coeff)[0][2] 8), HDMI_CSC_COEF_A3_MSB);
hdmi_writeb(hdmi, ((*csc_coeff)[0][3] 0xff), HDMI_CSC_COEF_A4_LSB);
- hdmi_writeb(hdmi, ((*csc_coeff)[0][4] 8), HDMI_CSC_COEF_A4_MSB);
+ hdmi_writeb(hdmi, ((*csc_coeff)[0][3] 8), HDMI_CSC_COEF_A4_MSB);
hdmi_writeb(hdmi, ((*csc_coeff)[1][0] 0xff), HDMI_CSC_COEF_B1_LSB);
hdmi_writeb(hdmi, ((*csc_coeff)[1][0] 8), HDMI_CSC_COEF_B1_MSB);
@@ -516,7 +516,7 @@ static void imx_hdmi_update_csc_coeffs(struct imx_hdmi
*hdmi)
hdmi_writeb(hdmi, ((*csc_coeff)[1][2] 0xff), HDMI_CSC_COEF_B3_LSB);
hdmi_writeb(hdmi, ((*csc_coeff)[1][2] 8), HDMI_CSC_COEF_B3_MSB);
hdmi_writeb(hdmi, ((*csc_coeff)[1][3] 0xff), HDMI_CSC_COEF_B4_LSB);
- hdmi_writeb(hdmi, ((*csc_coeff)[1][4] 8), HDMI_CSC_COEF_B4_MSB);
+ hdmi_writeb(hdmi, ((*csc_coeff)[1][3] 8), HDMI_CSC_COEF_B4_MSB);
hdmi_writeb(hdmi, ((*csc_coeff)[2][0] 0xff), HDMI_CSC_COEF_C1_LSB);
hdmi_writeb(hdmi, ((*csc_coeff)[2][0] 8), HDMI_CSC_COEF_C1_MSB);
@@ -525,7 +525,7 @@ static void imx_hdmi_update_csc_coeffs(struct imx_hdmi
*hdmi)
hdmi_writeb(hdmi, ((*csc_coeff)[2][2] 0xff), HDMI_CSC_COEF_C3_LSB);
hdmi_writeb(hdmi, ((*csc_coeff)[2][2] 8), HDMI_CSC_COEF_C3_MSB);
hdmi_writeb(hdmi, ((*csc_coeff)[2][3] 0xff), HDMI_CSC_COEF_C4_LSB);
- hdmi_writeb(hdmi, ((*csc_coeff)[2][4] 8), HDMI_CSC_COEF_C4_MSB);
+ hdmi_writeb(hdmi, ((*csc_coeff)[2][3] 8), HDMI_CSC_COEF_C4_MSB);
val = hdmi_readb(hdmi, HDMI_CSC_SCALE);
val = ~HDMI_CSC_SCALE_CSCSCALE_MASK;
@@ -1774,8 +1774,6 @@ static int imx_hdmi_register(struct imx_hdmi *hdmi)
{
int ret;
- drm_mode_connector_attach_encoder(hdmi-connector, hdmi-encoder);
-
hdmi-connector.funcs = imx_hdmi_connector_funcs;
hdmi-encoder.funcs = imx_hdmi_encoder_funcs;
@@ -1803,6 +1801,8 @@ static int imx_hdmi_register(struct imx_hdmi *hdmi)
hdmi-connector.encoder = hdmi-encoder;
+ drm_mode_connector_attach_encoder(hdmi-connector, hdmi-encoder);
+
return 0;
}
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 1/3] imx-drm: Add mx6 hdmi transmitter support
On Thu, Oct 03, 2013 at 03:51:25PM -0300, Fabio Estevam wrote: This is based on the initial work done by Sascha Hauer and Tony Prisk. Tested on imx6q-wandboard and imx6q-sabresd boards. Signed-off-by: Fabio Estevam fabio.este...@freescale.com --- Changes since v1: - Rebased against 3.12-rc3 and fixed header file location It should be done against linux-next as previously mentioned but the header still needs to be fixed in linux-next I think? Also it has the following Smatch warnings which are all basically correct. drivers/staging/imx-drm/imx-hdmi.c:511 imx_hdmi_update_csc_coeffs() error: buffer overflow '(*csc_coeff)[0]' 4 = 4 drivers/staging/imx-drm/imx-hdmi.c:520 imx_hdmi_update_csc_coeffs() error: buffer overflow '(*csc_coeff)[1]' 4 = 4 drivers/staging/imx-drm/imx-hdmi.c:529 imx_hdmi_update_csc_coeffs() error: buffer overflow '(*csc_coeff)[2]' 4 = 4 drivers/staging/imx-drm/imx-hdmi.c:785 hdmi_phy_i2c_write() info: ignoring unreachable code. drivers/staging/imx-drm/imx-hdmi.c:873 hdmi_phy_configure() warn: unsigned 'hdmi-hdmi_data.video_mode.mpixelclock' is never less than zero. drivers/staging/imx-drm/imx-hdmi.c:1537 imx_hdmi_fb_registered() warn: inconsistent returns spin_lock:hdmi-irq_lock: locked (1514 [s32min-(-1),1-s32max]) unlocked (1508 [0], 1537 [0]) drivers/staging/imx-drm/imx-hdmi.c:1537 imx_hdmi_fb_registered() warn: inconsistent returns irqsave:flags: locked (1514 [s32min-(-1),1-s32max]) unlocked (1508 [0], 1537 [0]) drivers/staging/imx-drm/imx-hdmi.c:1837 imx_hdmi_platform_probe() info: why not propagate 'irq' from platform_get_irq() instead of (-22)? regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
