[PATCH] staging: imx-drm: Fix pixel clock polarity

[David Jander]

Dear Philipp,

On Fri, 26 Jun 2015 12:27:03 +0200
Philipp Zabel <p.zabel at pengutronix.de> wrote:

> Hi David,
> 
> Am Freitag, den 26.06.2015, 09:51 +0200 schrieb David Jander:
> > clk_pol should almost always be 1 (active-high pixel clock). When using the
> > LDB, it must be 1 and for externally connected displays it should most
> > probably also be 1. Original Freescale code sets this bit always, except
> > when the FB_SYNC_CLK_LAT_FALL flag is set, which is a proprietary extension
> > from Freescale that is not normally set.
> > 
> > Signed-off-by: David Jander <david at protonic.nl>
> > ---
> >  drivers/gpu/drm/imx/ipuv3-crtc.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c
> > b/drivers/gpu/drm/imx/ipuv3-crtc.c index 7425fcc..97799ff 100644
> > --- a/drivers/gpu/drm/imx/ipuv3-crtc.c
> > +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
> > @@ -173,7 +173,7 @@ static int ipu_crtc_mode_set(struct drm_crtc *crtc,
> >  		sig_cfg.clkflags = 0;
> >  
> >  	sig_cfg.enable_pol = 1;
> > -	sig_cfg.clk_pol = 0;
> > +	sig_cfg.clk_pol = 1;
> >  	sig_cfg.bus_format = ipu_crtc->bus_format;
> >  	sig_cfg.v_to_h_sync = 0;
> >  	sig_cfg.hsync_pin = ipu_crtc->di_hsync_pin;
> 
> have a look at 85de9d17c485 ("imx-drm: match ipu_di_signal_cfg's clk_pol
> with its description."), which inverts the meaning of clk_pol.
> In the freescale code I see in drivers/video/mxc/mxc_ipuv3_fb.c:
> 
>                 memset(&sig_cfg, 0, sizeof(sig_cfg));
> 		/* ... */
>                 if (!(fbi->var.sync & FB_SYNC_CLK_LAT_FALL))
>                         sig_cfg.clk_pol = true;
> 
> And in drivers/mxc/ipu3/ipu_disp.c:
> 
>         if (!sig.clk_pol)
>                 di_gen |= DI_GEN_POLARITY_DISP_CLK;
>         ipu_di_write(ipu, disp, di_gen, DI_GENERAL);
> 
> So if FB_SYNC_CLK_LAT_FALL is not set, clk_pol defaults to true, but
> this causes the POLARITY_DISP_CLK bit in DI_GENERAL to be cleared (which
> is documented as active low). We do the same, just the other way around.
> drivers/gpu/drm/imx/ipuv3-crtc.c:
> 
>         sig_cfg.clk_pol = 0;
> 
> And in drivers/gpu/ipu-v3/ipu-di.c:
> 
>         if (sig->clk_pol)
>                 di_gen |= DI_GEN_POLARITY_DISP_CLK;
>         ipu_di_write(di, di_gen, DI_GENERAL);
> 
> At least with the HDMI output I have verified that the picture is
> currently correct but gets shifted by a pixel with your patch.

This is odd. We started investigating because one of our customers (using an
LVDS connected panel) reported some boards causing color errors in gradients.
If you displayed for example a horizontal red gradient from 0 to 255, you would
see 3 stripes through the gradient (dividing it into 4 parts) of pixels that
are too bright. Not all the boards had this issue, and in many of them itwas
an unstable symptom, which immediately pointed at a problem related to process
variations. The symptoms can be explained by a latch in the LDB (LVDS bridge)
that latches the individual bits in the shift register on the same clock-edge
that those signals change. So the internal signal delay can cause either the
old or the new value of the pixel-bit getting latched. The color stripes are
then caused by the roll-over from xx0111 to xx1000, where bit 3 gets latched
at it's new value, while all lower bits get latched at their previous value.
Most of the time this situation was unstable. I even have scope images showing
the anomaly on the LVDS signal. This patch fixes the issue for all boards.

If you are confident that your code is correct, then maybe the LDB expects an
inverted clock signal?

I must admit that this patch is rather old. I tested it thoroughly on 3.17,
but AFAICS, the patch you mention is also included in 3.17, so I suppose the
situation has not changed.

Best regards,

-- 
David Jander
Protonic Holland.