[PATCH v2 1/3] imx-drm: Add mx6 hdmi transmitter support

[Troy Kisky]

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
>> <linux at 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+kernel at arm.linux.org.uk>
>>> Tested-by: Russell King <rmk+kernel at 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 margin            displayed            right 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 skew    0 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:
> 0x2640000 = 0x200006, 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:
>
>          /* HDMI Initialization Step B.3 */
>          imx_hdmi_enable_video_path(hdmi);
>
> One's left wondering whether Step B.3 really is to just call a function
> with a particular name, but B.4 is to actually do something with the
> hardware.  I'm quite sure that if this is a documented procedure, that
> it doesn't say that and these comments are wrong (and probably the code
> too.)
>
> Even after all this, I still haven't got rid of that magenta line - in
> as far as I can tell, nothing has changed as a result of any of these
> (although reading back the register values, they're now much better.)
> What I do find is if I poke 0x78 back into the INVIDCONF register
> (which already contains 0x78) the magenta line disappears.
>
> I'm beginning to suspect that there's some ordering dependency that
> isn't being satisfied - but as the i.MX6Solo/DualLite manual doesn't
> seem to contain any of these details, I'm running out of ideas.
>
> _______________________________________________
>
It sound to me like you hit errata "ERR004308 HDMI: 8000504668—The 
arithmetic unit may get wrong video
timing values although the FC_* registers hold correct values"


Also, checkout ERR005173, it says
"Workarounds:
The programming flow should be:
1. Program all the controller registers including the frame composer 
registers.
2. Assert software resets
3. Write (3 or 4 times) in FC_INVIDCONF the final value (this will make 
sure that the update
pulse for the fc_arithlogicunit_div that will update the 
fc_arithlogicunit_div units is generated)
4. If frame composer packet queue overflow still occurs, then repeat 
steps 2 and 3"


Troy