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

[troy.kisky@boundarydevices.com (Troy Kisky)]

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
>>>> <linux@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@arm.linux.org.uk>
>>>>> Tested-by: Russell King <rmk+kernel@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"
> Hi Troy,
>
> I think it's implementing that - we have this code:
>
> /* Workaround to clear the overflow condition */
> static void imx_hdmi_clear_overflow(struct imx_hdmi *hdmi)
> {
>          int count;
>          u8 val;
>           
>          val = hdmi_readb(hdmi, HDMI_FC_INVIDCONF);
>          
>          for (count = 0; count < 5; count++)
>                  hdmi_writeb(hdmi, val, HDMI_FC_INVIDCONF);
>           
>          /* TMDS software reset */
>          hdmi_writeb(hdmi, (u8)~HDMI_MC_SWRSTZ_TMDSSWRST_REQ, HDMI_MC_SWRSTZ);
> }
>
>
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.

I don't know of any other documentation.

Troy