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

[Russell King - ARM Linux]

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.