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

On Wed, Oct 16, 2013 at 06:03:42PM +0100, 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.

More issues: 720x576p @50Hz doesn't work very well, there's speckles
over the display, with the odd random line of corruption, and the
display occasionally blanks.  1280x720p at 50Hz or 60Hz is fine (apart
from the magenta line).  However, the magenta line disappears after
playing around setting various modes.

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.

In any case, here's my patch so far, which applies on top of the previous
I sent for the array overflow/encoder attachment:

 drivers/staging/imx-drm/imx-hdmi.c |   86 ++++++++++++------------------------
 1 files changed, 28 insertions(+), 58 deletions(-)

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

Commit Message

Comments

Patch