[PATCH v2 2/3] ARM: dts: imx6qdl-wandboard: Add HDMI support

[PATCH v2 2/3] ARM: dts: imx6qdl-wandboard: Add HDMI support

[Fabio Estevam]

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
Changes since v1:
- None

 arch/arm/boot/dts/imx6dl.dtsi            |  4 ++++
 arch/arm/boot/dts/imx6q.dtsi             |  4 ++++
 arch/arm/boot/dts/imx6qdl-wandboard.dtsi | 13 +++++++++++++
 arch/arm/boot/dts/imx6qdl.dtsi           | 10 ++++++++++
 4 files changed, 31 insertions(+)

diff --git a/arch/arm/boot/dts/imx6dl.dtsi b/arch/arm/boot/dts/imx6dl.dtsi
index 9e8ae11..65e54b4 100644
--- a/arch/arm/boot/dts/imx6dl.dtsi
+++ b/arch/arm/boot/dts/imx6dl.dtsi
@@ -88,3 +88,7 @@
 		crtcs = <&ipu1 0>, <&ipu1 1>;
 	};
 };
+
+&hdmi {
+	crtcs = <&ipu1 0>, <&ipu1 1>;
+}
diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
index f024ef2..d2467f5 100644
--- a/arch/arm/boot/dts/imx6q.dtsi
+++ b/arch/arm/boot/dts/imx6q.dtsi
@@ -159,3 +159,7 @@
 		crtcs = <&ipu1 0>, <&ipu1 1>, <&ipu2 0>, <&ipu2 1>;
 	};
 };
+
+&hdmi {
+	crtcs = <&ipu1 0>, <&ipu1 1>, <&ipu2 0>, <&ipu2 1>;
+};
diff --git a/arch/arm/boot/dts/imx6qdl-wandboard.dtsi b/arch/arm/boot/dts/imx6qdl-wandboard.dtsi
index a55113e..758c17f 100644
--- a/arch/arm/boot/dts/imx6qdl-wandboard.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-wandboard.dtsi
@@ -51,6 +51,19 @@
 	status = "okay";
 };
 
+
+&hdmi {
+	ddc = <&i2c1>;
+	status = "okay";
+};
+
+&i2c1 {
+	clock-frequency = <100000>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_i2c1_1>;
+	status = "okay";
+};
+
 &i2c2 {
 	clock-frequency = <100000>;
 	pinctrl-names = "default";
diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
index ccd55c2..b148cb3 100644
--- a/arch/arm/boot/dts/imx6qdl.dtsi
+++ b/arch/arm/boot/dts/imx6qdl.dtsi
@@ -1301,6 +1301,16 @@
 				};
 			};
 
+			hdmi: hdmi at 0120000 {
+				compatible = "fsl,imx6q-hdmi";
+				reg = <0x00120000 0x9000>;
+				interrupts = <0 115 0x04>;
+				gpr = <&gpr>;
+				clocks = <&clks 123>, <&clks 124>;
+				clock-names = "iahb", "isfr";
+				status = "disabled";
+			};
+
 			dcic1: dcic at 020e4000 {
 				reg = <0x020e4000 0x4000>;
 				interrupts = <0 124 0x04>;
-- 
1.8.1.2

[PATCH v2 2/3] ARM: dts: imx6qdl-wandboard: Add HDMI support

[Russell King - ARM Linux]

On Thu, Oct 03, 2013 at 03:51:26PM -0300, Fabio Estevam wrote:
> diff --git a/arch/arm/boot/dts/imx6dl.dtsi b/arch/arm/boot/dts/imx6dl.dtsi
> index 9e8ae11..65e54b4 100644
> --- a/arch/arm/boot/dts/imx6dl.dtsi
> +++ b/arch/arm/boot/dts/imx6dl.dtsi
> @@ -88,3 +88,7 @@
>  		crtcs = <&ipu1 0>, <&ipu1 1>;
>  	};
>  };
> +
> +&hdmi {
> +	crtcs = <&ipu1 0>, <&ipu1 1>;
> +}
> diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
> index f024ef2..d2467f5 100644
> --- a/arch/arm/boot/dts/imx6q.dtsi
> +++ b/arch/arm/boot/dts/imx6q.dtsi
> @@ -159,3 +159,7 @@
>  		crtcs = <&ipu1 0>, <&ipu1 1>, <&ipu2 0>, <&ipu2 1>;
>  	};
>  };
> +
> +&hdmi {
> +	crtcs = <&ipu1 0>, <&ipu1 1>, <&ipu2 0>, <&ipu2 1>;
> +};
> diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
> index ccd55c2..b148cb3 100644
> --- a/arch/arm/boot/dts/imx6qdl.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl.dtsi
> @@ -1301,6 +1301,16 @@
>  				};
>  			};
>  
> +			hdmi: hdmi at 0120000 {
> +				compatible = "fsl,imx6q-hdmi";
> +				reg = <0x00120000 0x9000>;
> +				interrupts = <0 115 0x04>;
> +				gpr = <&gpr>;
> +				clocks = <&clks 123>, <&clks 124>;
> +				clock-names = "iahb", "isfr";
> +				status = "disabled";
> +			};
> +
>  			dcic1: dcic at 020e4000 {
>  				reg = <0x020e4000 0x4000>;
>  				interrupts = <0 124 0x04>;

Shouldn't the above be in patch 1 (or 1.5) rather than patch 2?  Patch 2
advertises itself as adding support for the wandboard, but in actual
fact it's adding the generic bits too.

[PATCH v2 2/3] ARM: dts: imx6qdl-wandboard: Add HDMI support

[Fabio Estevam]

On Mon, Oct 14, 2013 at 2:38 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:

> Shouldn't the above be in patch 1 (or 1.5) rather than patch 2?  Patch 2
> advertises itself as adding support for the wandboard, but in actual
> fact it's adding the generic bits too.

Thanks for your review. Will address your comments in v3.

Philipp Zabel mentioned that he will move imx-drm out of staging, so
will send v3 after Philipp's patch gets into linux-next.

Regards,

Fabio Estevam

[PATCH v2 2/3] ARM: dts: imx6qdl-wandboard: Add HDMI support

[Russell King - ARM Linux]

On Mon, Oct 14, 2013 at 11:47:17PM -0300, Fabio Estevam wrote:
> On Mon, Oct 14, 2013 at 2:38 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> 
> > Shouldn't the above be in patch 1 (or 1.5) rather than patch 2?  Patch 2
> > advertises itself as adding support for the wandboard, but in actual
> > fact it's adding the generic bits too.
> 
> Thanks for your review. Will address your comments in v3.
> 
> Philipp Zabel mentioned that he will move imx-drm out of staging, so
> will send v3 after Philipp's patch gets into linux-next.

That's unfortunate, especially given the bug with the clock polarity
(which, although I can tweak the register directly, it's not obvious
that changing the clk_pol initialisation is the correct solution.)

There's also another issue here: the lack of DRM prime support.  As
you have a separate GPU and separate VPU, you really need dmabuf
support implemented so that you can hand your scanout buffers/overlays
to other subsystems.

[PATCH v2 2/3] ARM: dts: imx6qdl-wandboard: Add HDMI support

[Sascha Hauer]

On Tue, Oct 15, 2013 at 11:00:26AM +0100, Russell King - ARM Linux wrote:
> On Mon, Oct 14, 2013 at 11:47:17PM -0300, Fabio Estevam wrote:
> > On Mon, Oct 14, 2013 at 2:38 PM, Russell King - ARM Linux
> > <linux@arm.linux.org.uk> wrote:
> > 
> > > Shouldn't the above be in patch 1 (or 1.5) rather than patch 2?  Patch 2
> > > advertises itself as adding support for the wandboard, but in actual
> > > fact it's adding the generic bits too.
> > 
> > Thanks for your review. Will address your comments in v3.
> > 
> > Philipp Zabel mentioned that he will move imx-drm out of staging, so
> > will send v3 after Philipp's patch gets into linux-next.
> 
> That's unfortunate, especially given the bug with the clock polarity
> (which, although I can tweak the register directly, it's not obvious
> that changing the clk_pol initialisation is the correct solution.)
> 
> There's also another issue here: the lack of DRM prime support.  As
> you have a separate GPU and separate VPU, you really need dmabuf
> support implemented so that you can hand your scanout buffers/overlays
> to other subsystems.

See http://www.spinics.net/lists/linux-driver-devel/msg39324.html

Besides other things this adds prime support.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH v2 2/3] ARM: dts: imx6qdl-wandboard: Add HDMI support

[Russell King - ARM Linux]

Another thing that my build testing (and use on cubox-i) picked up:

On Thu, Oct 03, 2013 at 03:51:26PM -0300, Fabio Estevam wrote:
> diff --git a/arch/arm/boot/dts/imx6dl.dtsi b/arch/arm/boot/dts/imx6dl.dtsi
> index 9e8ae11..65e54b4 100644
> --- a/arch/arm/boot/dts/imx6dl.dtsi
> +++ b/arch/arm/boot/dts/imx6dl.dtsi
> @@ -88,3 +88,7 @@
>  		crtcs = <&ipu1 0>, <&ipu1 1>;
>  	};
>  };
> +
> +&hdmi {
> +	crtcs = <&ipu1 0>, <&ipu1 1>;
> +}

Missing semi-colon.

[PATCH v2 2/3] ARM: dts: imx6qdl-wandboard: Add HDMI support

[Russell King - ARM Linux]

On Mon, Oct 14, 2013 at 06:40:30PM +0100, Russell King - ARM Linux wrote:
> Another thing that my build testing (and use on cubox-i) picked up:
> 
> On Thu, Oct 03, 2013 at 03:51:26PM -0300, Fabio Estevam wrote:
> > diff --git a/arch/arm/boot/dts/imx6dl.dtsi b/arch/arm/boot/dts/imx6dl.dtsi
> > index 9e8ae11..65e54b4 100644
> > --- a/arch/arm/boot/dts/imx6dl.dtsi
> > +++ b/arch/arm/boot/dts/imx6dl.dtsi
> > @@ -88,3 +88,7 @@
> >  		crtcs = <&ipu1 0>, <&ipu1 1>;
> >  	};
> >  };
> > +
> > +&hdmi {
> > +	crtcs = <&ipu1 0>, <&ipu1 1>;
> > +}
> 
> Missing semi-colon.

Okay, next question(s).

1. How well tested is any of this imx-drm stuff?

2. What sort of testing has it been subject to?

Answers to these two questions may help stop me wasting a lot of time
chasing what is a really weird bug.

So, I have X up and running on the Cubox-i carrier-1, using the imx-drm
stuff (I've slightly hacked my xf86-armada X driver to get this working.)
This works fine - it detects the connectors, selects an appropriate
video mode and produces a picture of the correct shape and size.

However... I see really weird effects colouring effects - almost like
water over the image.  It's certain colour transitions in the image
which seem to trigger this.  There are also certain pixels which
"twinkle".

Text looks very strange too.  Rather than the font being crisp and clear,
it looks like there's red and green shift to it - but its not that it's
all shifted in that way.

Now, if I use the modetest utility from libdrm-2.4.43 to display a SMPTE
test pattern, this again looks right, but there are several vertical
single pixel lines of noise.  The most striking one is below the upper
red bar in the lower dark area.  I have a single pixel noisy vertical
line.

I've tried the kernel which Rabeeh supplied, which is based on BSP 4.1.0,
and this works fine as far as I can tell with the same image (though,
it's a little difficult converting the XWD dump to something that can
be directly poked into /dev/fb0..., although this results in R/B swap.)

Any ideas where to start looking?

[PATCH v2 2/3] ARM: dts: imx6qdl-wandboard: Add HDMI support

[Fabio Estevam]

On Mon, Oct 14, 2013 at 7:50 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:

> Okay, next question(s).
>
> 1. How well tested is any of this imx-drm stuff?
>
> 2. What sort of testing has it been subject to?

Most of my tests with the imx-drm driver have been through the LVDS
panel so far.

For HDMI, I haven't gone through too much of testing yet, only basic
raw framebuffer at full HD resolution.

I know Tony/Robert (and Sascha) and some folks at the Wandboard
mailing list have also tested HDMI with the previous version of the
HDMI driver that was posted by Tony.

> Answers to these two questions may help stop me wasting a lot of time
> chasing what is a really weird bug.
>
> So, I have X up and running on the Cubox-i carrier-1, using the imx-drm
> stuff (I've slightly hacked my xf86-armada X driver to get this working.)
> This works fine - it detects the connectors, selects an appropriate
> video mode and produces a picture of the correct shape and size.
>
> However... I see really weird effects colouring effects - almost like
> water over the image.  It's certain colour transitions in the image
> which seem to trigger this.  There are also certain pixels which
> "twinkle".
>
> Text looks very strange too.  Rather than the font being crisp and clear,
> it looks like there's red and green shift to it - but its not that it's
> all shifted in that way.
>
> Now, if I use the modetest utility from libdrm-2.4.43 to display a SMPTE
> test pattern, this again looks right, but there are several vertical
> single pixel lines of noise.  The most striking one is below the upper
> red bar in the lower dark area.  I have a single pixel noisy vertical
> line.

Ok, interesting. Will run a video test pattern via gstreamer
videotestsrc plugin to see how it behaves.

I am travelling tomorrow, so it will be at last in 2 weeks that I will
be able to access a mx6 hardware.

In the meantime, maybe one of the folks in Cc could share some ideas.

Regards,

Fabio Estevam

[PATCH v2 2/3] ARM: dts: imx6qdl-wandboard: Add HDMI support

[Sascha Hauer]

On Mon, Oct 14, 2013 at 11:50:29PM +0100, Russell King - ARM Linux wrote:
> Answers to these two questions may help stop me wasting a lot of time
> chasing what is a really weird bug.
> 
> So, I have X up and running on the Cubox-i carrier-1, using the imx-drm
> stuff (I've slightly hacked my xf86-armada X driver to get this working.)
> This works fine - it detects the connectors, selects an appropriate
> video mode and produces a picture of the correct shape and size.
> 
> However... I see really weird effects colouring effects - almost like
> water over the image.  It's certain colour transitions in the image
> which seem to trigger this.  There are also certain pixels which
> "twinkle".
> 
> Text looks very strange too.  Rather than the font being crisp and clear,
> it looks like there's red and green shift to it - but its not that it's
> all shifted in that way.
> 
> Now, if I use the modetest utility from libdrm-2.4.43 to display a SMPTE
> test pattern, this again looks right, but there are several vertical
> single pixel lines of noise.  The most striking one is below the upper
> red bar in the lower dark area.  I have a single pixel noisy vertical
> line.
> 
> I've tried the kernel which Rabeeh supplied, which is based on BSP 4.1.0,
> and this works fine as far as I can tell with the same image (though,
> it's a little difficult converting the XWD dump to something that can
> be directly poked into /dev/fb0..., although this results in R/B swap.)
> 
> Any ideas where to start looking?

This sounds like the wrong clock polarity. Could you try inverting
sig_cfg.clk_pol in imx-drm/ipuv3-crtc.c?

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH v2 2/3] ARM: dts: imx6qdl-wandboard: Add HDMI support

[Russell King - ARM Linux]

On Tue, Oct 15, 2013 at 09:46:18AM +0200, Sascha Hauer wrote:
> On Mon, Oct 14, 2013 at 11:50:29PM +0100, Russell King - ARM Linux wrote:
> > Answers to these two questions may help stop me wasting a lot of time
> > chasing what is a really weird bug.
> > 
> > So, I have X up and running on the Cubox-i carrier-1, using the imx-drm
> > stuff (I've slightly hacked my xf86-armada X driver to get this working.)
> > This works fine - it detects the connectors, selects an appropriate
> > video mode and produces a picture of the correct shape and size.
> > 
> > However... I see really weird effects colouring effects - almost like
> > water over the image.  It's certain colour transitions in the image
> > which seem to trigger this.  There are also certain pixels which
> > "twinkle".
> > 
> > Text looks very strange too.  Rather than the font being crisp and clear,
> > it looks like there's red and green shift to it - but its not that it's
> > all shifted in that way.
> > 
> > Now, if I use the modetest utility from libdrm-2.4.43 to display a SMPTE
> > test pattern, this again looks right, but there are several vertical
> > single pixel lines of noise.  The most striking one is below the upper
> > red bar in the lower dark area.  I have a single pixel noisy vertical
> > line.
> > 
> > I've tried the kernel which Rabeeh supplied, which is based on BSP 4.1.0,
> > and this works fine as far as I can tell with the same image (though,
> > it's a little difficult converting the XWD dump to something that can
> > be directly poked into /dev/fb0..., although this results in R/B swap.)
> > 
> > Any ideas where to start looking?
> 
> This sounds like the wrong clock polarity. Could you try inverting
> sig_cfg.clk_pol in imx-drm/ipuv3-crtc.c?

I tweaked it a different way - I used devmem2 to directly poke at the
register.  Inverting bit 17 (iow, clearing it) seems to fix the
problem.

[PATCH v2 2/3] ARM: dts: imx6qdl-wandboard: Add HDMI support

[Russell King - ARM Linux]

On Tue, Oct 15, 2013 at 10:18:30AM +0100, Russell King - ARM Linux wrote:
> On Tue, Oct 15, 2013 at 09:46:18AM +0200, Sascha Hauer wrote:
> > This sounds like the wrong clock polarity. Could you try inverting
> > sig_cfg.clk_pol in imx-drm/ipuv3-crtc.c?
> 
> I tweaked it a different way - I used devmem2 to directly poke at the
> register.  Inverting bit 17 (iow, clearing it) seems to fix the
> problem.

As a follow-up, the driver says this:

        unsigned clk_pol:1;     /* true = rising edge */

This is interpreted by the ipu-di.c driver as:

        if (!sig->clk_pol)
                di_gen |= DI_GEN_POLARITY_DISP_CLK;

note the inversion.  U-boot does something slightly different here
(apparantly it describes clk_pol in the same way though):

                if (sig.clk_pol)
                        di_gen |= DI_GEN_POL_CLK;

It's also reported that u-boot sets sig.clk_pol when
FB_SYNC_CLK_LAT_FALL bit is not set (which confirms that clk_pol
indicates rising edge active.)

Now, the confusing bit.  The docs for the imx6s/dl say that bit 17
when set is "active high" vs clear "active low".  This appears to
define a level active state.  The code seems to define an edge
instead.  What's more is that u-boot and the kernel seem to describe
'clk_pol' in the same way yet interpret it differently.

Should that inversion in the kernel be there?

[PATCH v2 2/3] ARM: dts: imx6qdl-wandboard: Add HDMI support

[Sascha Hauer]

On Tue, Oct 15, 2013 at 11:35:00AM +0100, Russell King - ARM Linux wrote:
> On Tue, Oct 15, 2013 at 10:18:30AM +0100, Russell King - ARM Linux wrote:
> > On Tue, Oct 15, 2013 at 09:46:18AM +0200, Sascha Hauer wrote:
> > > This sounds like the wrong clock polarity. Could you try inverting
> > > sig_cfg.clk_pol in imx-drm/ipuv3-crtc.c?
> > 
> > I tweaked it a different way - I used devmem2 to directly poke at the
> > register.  Inverting bit 17 (iow, clearing it) seems to fix the
> > problem.
> 
> As a follow-up, the driver says this:
> 
>         unsigned clk_pol:1;     /* true = rising edge */
> 
> This is interpreted by the ipu-di.c driver as:
> 
>         if (!sig->clk_pol)
>                 di_gen |= DI_GEN_POLARITY_DISP_CLK;
> 
> note the inversion.  U-boot does something slightly different here
> (apparantly it describes clk_pol in the same way though):
> 
>                 if (sig.clk_pol)
>                         di_gen |= DI_GEN_POL_CLK;
> 
> It's also reported that u-boot sets sig.clk_pol when
> FB_SYNC_CLK_LAT_FALL bit is not set (which confirms that clk_pol
> indicates rising edge active.)
> 
> Now, the confusing bit.  The docs for the imx6s/dl say that bit 17
> when set is "active high" vs clear "active low".  This appears to
> define a level active state.  The code seems to define an edge
> instead.  What's more is that u-boot and the kernel seem to describe
> 'clk_pol' in the same way yet interpret it differently.
> 
> Should that inversion in the kernel be there?

I could measure the pin with an oscilloscope on some board using
the parallel display output. That should how this bit really behaves
and we can cleanup the comments and/or code.

Won't have time for this before Edinburgh though.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH v2 3/3] ARM: dts: imx6qdl-sabresd: Add HDMI support

[Fabio Estevam]

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
Changes since v1:
- Put the dt entries in alphabetical order

 arch/arm/boot/dts/imx6qdl-sabresd.dtsi | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
index 39eafc2..4df4432 100644
--- a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
@@ -111,6 +111,11 @@
 	status = "okay";
 };
 
+&hdmi {
+	ddc = <&i2c2>;
+	status = "okay";
+};
+
 &i2c1 {
 	clock-frequency = <100000>;
 	pinctrl-names = "default";
@@ -140,6 +145,13 @@
        };
 };
 
+&i2c2 {
+	clock-frequency = <100000>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_i2c2_2>;
+	status = "okay";
+};
+
 &i2c3 {
 	clock-frequency = <100000>;
 	pinctrl-names = "default";
-- 
1.8.1.2

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

[Dan Carpenter]

On Thu, Oct 03, 2013 at 03:51:25PM -0300, Fabio Estevam wrote:
> This is based on the initial work done by Sascha Hauer and Tony Prisk.
> 
> Tested on imx6q-wandboard and imx6q-sabresd boards.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
> Changes since v1:
> - Rebased against 3.12-rc3 and fixed header file location
> 

It should be done against linux-next as previously mentioned but the
header still needs to be fixed in linux-next I think?

Also it has the following Smatch warnings which are all basically
correct.

drivers/staging/imx-drm/imx-hdmi.c:511 imx_hdmi_update_csc_coeffs() error: buffer overflow '(*csc_coeff)[0]' 4 <= 4
drivers/staging/imx-drm/imx-hdmi.c:520 imx_hdmi_update_csc_coeffs() error: buffer overflow '(*csc_coeff)[1]' 4 <= 4
drivers/staging/imx-drm/imx-hdmi.c:529 imx_hdmi_update_csc_coeffs() error: buffer overflow '(*csc_coeff)[2]' 4 <= 4
drivers/staging/imx-drm/imx-hdmi.c:785 hdmi_phy_i2c_write() info: ignoring unreachable code.
drivers/staging/imx-drm/imx-hdmi.c:873 hdmi_phy_configure() warn: unsigned 'hdmi->hdmi_data.video_mode.mpixelclock' is never less than zero.
drivers/staging/imx-drm/imx-hdmi.c:1537 imx_hdmi_fb_registered() warn: inconsistent returns spin_lock:&hdmi->irq_lock: locked (1514 [s32min-(-1),1-s32max]) unlocked (1508 [0], 1537 [0])
drivers/staging/imx-drm/imx-hdmi.c:1537 imx_hdmi_fb_registered() warn: inconsistent returns irqsave:flags: locked (1514 [s32min-(-1),1-s32max]) unlocked (1508 [0], 1537 [0])
drivers/staging/imx-drm/imx-hdmi.c:1837 imx_hdmi_platform_probe() info: why not propagate 'irq' from platform_get_irq() instead of (-22)?

regards,
dan carpenter

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

[Russell King - ARM Linux]

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>

diff --git a/drivers/staging/imx-drm/imx-hdmi.c b/drivers/staging/imx-drm/imx-hdmi.c
index e227eb1..ca0450b 100644
--- a/drivers/staging/imx-drm/imx-hdmi.c
+++ b/drivers/staging/imx-drm/imx-hdmi.c
@@ -507,7 +507,7 @@ static void imx_hdmi_update_csc_coeffs(struct imx_hdmi *hdmi)
 	hdmi_writeb(hdmi, ((*csc_coeff)[0][2] & 0xff), HDMI_CSC_COEF_A3_LSB);
 	hdmi_writeb(hdmi, ((*csc_coeff)[0][2] >> 8), HDMI_CSC_COEF_A3_MSB);
 	hdmi_writeb(hdmi, ((*csc_coeff)[0][3] & 0xff), HDMI_CSC_COEF_A4_LSB);
-	hdmi_writeb(hdmi, ((*csc_coeff)[0][4] >> 8), HDMI_CSC_COEF_A4_MSB);
+	hdmi_writeb(hdmi, ((*csc_coeff)[0][3] >> 8), HDMI_CSC_COEF_A4_MSB);
 
 	hdmi_writeb(hdmi, ((*csc_coeff)[1][0] & 0xff), HDMI_CSC_COEF_B1_LSB);
 	hdmi_writeb(hdmi, ((*csc_coeff)[1][0] >> 8), HDMI_CSC_COEF_B1_MSB);
@@ -516,7 +516,7 @@ static void imx_hdmi_update_csc_coeffs(struct imx_hdmi *hdmi)
 	hdmi_writeb(hdmi, ((*csc_coeff)[1][2] & 0xff), HDMI_CSC_COEF_B3_LSB);
 	hdmi_writeb(hdmi, ((*csc_coeff)[1][2] >> 8), HDMI_CSC_COEF_B3_MSB);
 	hdmi_writeb(hdmi, ((*csc_coeff)[1][3] & 0xff), HDMI_CSC_COEF_B4_LSB);
-	hdmi_writeb(hdmi, ((*csc_coeff)[1][4] >> 8), HDMI_CSC_COEF_B4_MSB);
+	hdmi_writeb(hdmi, ((*csc_coeff)[1][3] >> 8), HDMI_CSC_COEF_B4_MSB);
 
 	hdmi_writeb(hdmi, ((*csc_coeff)[2][0] & 0xff), HDMI_CSC_COEF_C1_LSB);
 	hdmi_writeb(hdmi, ((*csc_coeff)[2][0] >> 8), HDMI_CSC_COEF_C1_MSB);
@@ -525,7 +525,7 @@ static void imx_hdmi_update_csc_coeffs(struct imx_hdmi *hdmi)
 	hdmi_writeb(hdmi, ((*csc_coeff)[2][2] & 0xff), HDMI_CSC_COEF_C3_LSB);
 	hdmi_writeb(hdmi, ((*csc_coeff)[2][2] >> 8), HDMI_CSC_COEF_C3_MSB);
 	hdmi_writeb(hdmi, ((*csc_coeff)[2][3] & 0xff), HDMI_CSC_COEF_C4_LSB);
-	hdmi_writeb(hdmi, ((*csc_coeff)[2][4] >> 8), HDMI_CSC_COEF_C4_MSB);
+	hdmi_writeb(hdmi, ((*csc_coeff)[2][3] >> 8), HDMI_CSC_COEF_C4_MSB);
 
 	val = hdmi_readb(hdmi, HDMI_CSC_SCALE);
 	val &= ~HDMI_CSC_SCALE_CSCSCALE_MASK;
@@ -1774,8 +1774,6 @@ static int imx_hdmi_register(struct imx_hdmi *hdmi)
 {
 	int ret;
 
-	drm_mode_connector_attach_encoder(&hdmi->connector, &hdmi->encoder);
-
 	hdmi->connector.funcs = &imx_hdmi_connector_funcs;
 	hdmi->encoder.funcs = &imx_hdmi_encoder_funcs;
 
@@ -1803,6 +1801,8 @@ static int imx_hdmi_register(struct imx_hdmi *hdmi)
 
 	hdmi->connector.encoder = &hdmi->encoder;
 
+	drm_mode_connector_attach_encoder(&hdmi->connector, &hdmi->encoder);
+
 	return 0;
 }
 

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

[Fabio Estevam]

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.

Regards,

Fabio Estevam

[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@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.

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

[Russell King - ARM Linux]

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(-)

diff --git a/drivers/staging/imx-drm/imx-hdmi.c b/drivers/staging/imx-drm/imx-hdmi.c
index ca0450b..942dda6 100644
--- a/drivers/staging/imx-drm/imx-hdmi.c
+++ b/drivers/staging/imx-drm/imx-hdmi.c
@@ -133,7 +133,6 @@ struct imx_hdmi {
 	struct i2c_adapter *ddc;
 	void __iomem *regs;
 
-	struct fb_videomode fb_mode;
 	spinlock_t enable_lock;
 	spinlock_t lock;
 	unsigned long pixel_clk_rate;
@@ -1221,20 +1220,18 @@ static void hdmi_config_AVI(struct imx_hdmi *hdmi)
 	hdmi_writeb(hdmi, 0, HDMI_FC_AVISRB1);
 }
 
-static void hdmi_av_composer(struct imx_hdmi *hdmi)
+static void hdmi_av_composer(struct imx_hdmi *hdmi,
+	const struct drm_display_mode *mode)
 {
 	u8 inv_val;
-	struct fb_videomode *fb_mode = &hdmi->fb_mode;
 	struct hdmi_vmode *vmode = &hdmi->hdmi_data.video_mode;
-	int hblank, vblank;
+	int hblank, vblank, h_de_hs, v_de_vs, hsync_len, vsync_len;
 
-	vmode->mhsyncpolarity = ((fb_mode->sync & FB_SYNC_HOR_HIGH_ACT) != 0);
-	vmode->mvsyncpolarity = ((fb_mode->sync & FB_SYNC_VERT_HIGH_ACT) != 0);
-	vmode->minterlaced = ((fb_mode->vmode & FB_VMODE_INTERLACED) != 0);
-	vmode->mpixelclock = (fb_mode->xres + fb_mode->left_margin +
-		fb_mode->right_margin + fb_mode->hsync_len) * (fb_mode->yres +
-		fb_mode->upper_margin + fb_mode->lower_margin +
-		fb_mode->vsync_len) * fb_mode->refresh;
+	vmode->mhsyncpolarity = !!(mode->flags & DRM_MODE_FLAG_PHSYNC);
+	vmode->mvsyncpolarity = !!(mode->flags & DRM_MODE_FLAG_PVSYNC);
+	vmode->minterlaced = !!(mode->flags & DRM_MODE_FLAG_INTERLACE);
+	vmode->mpixelclock = mode->htotal * mode->vtotal *
+		drm_mode_vrefresh(mode);
 
 	dev_dbg(hdmi->dev, "final pixclk = %d\n", vmode->mpixelclock);
 
@@ -1272,38 +1269,40 @@ static void hdmi_av_composer(struct imx_hdmi *hdmi)
 
 	hdmi_writeb(hdmi, inv_val, HDMI_FC_INVIDCONF);
 
-	/* Set up horizontal active pixel region width */
-	hdmi_writeb(hdmi, fb_mode->xres >> 8, HDMI_FC_INHACTV1);
-	hdmi_writeb(hdmi, fb_mode->xres, HDMI_FC_INHACTV0);
+	/* Set up horizontal active pixel width */
+	hdmi_writeb(hdmi, mode->hdisplay >> 8, HDMI_FC_INHACTV1);
+	hdmi_writeb(hdmi, mode->hdisplay, HDMI_FC_INHACTV0);
 
-	/* Set up vertical blanking pixel region width */
-	hdmi_writeb(hdmi, fb_mode->yres >> 8, HDMI_FC_INVACTV1);
-	hdmi_writeb(hdmi, fb_mode->yres, HDMI_FC_INVACTV0);
+	/* Set up vertical active lines */
+	hdmi_writeb(hdmi, mode->vdisplay >> 8, HDMI_FC_INVACTV1);
+	hdmi_writeb(hdmi, mode->vdisplay, HDMI_FC_INVACTV0);
 
 	/* Set up horizontal blanking pixel region width */
-	hblank = fb_mode->left_margin + fb_mode->right_margin +
-		fb_mode->hsync_len;
+	hblank = mode->htotal - mode->hdisplay;
 	hdmi_writeb(hdmi, hblank >> 8, HDMI_FC_INHBLANK1);
 	hdmi_writeb(hdmi, hblank, HDMI_FC_INHBLANK0);
 
 	/* Set up vertical blanking pixel region width */
-	vblank = fb_mode->upper_margin + fb_mode->lower_margin +
-		fb_mode->vsync_len;
+	vblank = mode->vtotal - mode->vdisplay;
 	hdmi_writeb(hdmi, vblank, HDMI_FC_INVBLANK);
 
 	/* Set up HSYNC active edge delay width (in pixel clks) */
-	hdmi_writeb(hdmi, fb_mode->right_margin >> 8, HDMI_FC_HSYNCINDELAY1);
-	hdmi_writeb(hdmi, fb_mode->right_margin, HDMI_FC_HSYNCINDELAY0);
+	h_de_hs = mode->hsync_start - mode->hdisplay;
+	hdmi_writeb(hdmi, h_de_hs >> 8, HDMI_FC_HSYNCINDELAY1);
+	hdmi_writeb(hdmi, h_de_hs, HDMI_FC_HSYNCINDELAY0);
 
 	/* Set up VSYNC active edge delay (in pixel clks) */
-	hdmi_writeb(hdmi, fb_mode->lower_margin, HDMI_FC_VSYNCINDELAY);
+	v_de_vs = mode->vsync_start - mode->vdisplay;
+	hdmi_writeb(hdmi, v_de_vs, HDMI_FC_VSYNCINDELAY);
 
 	/* Set up HSYNC active pulse width (in pixel clks) */
-	hdmi_writeb(hdmi, fb_mode->hsync_len >> 8, HDMI_FC_HSYNCINWIDTH1);
-	hdmi_writeb(hdmi, fb_mode->hsync_len, HDMI_FC_HSYNCINWIDTH0);
+	hsync_len = mode->hsync_end - mode->hsync_start;
+	hdmi_writeb(hdmi, hsync_len >> 8, HDMI_FC_HSYNCINWIDTH1);
+	hdmi_writeb(hdmi, hsync_len, HDMI_FC_HSYNCINWIDTH0);
 
 	/* Set up VSYNC active edge delay (in pixel clks) */
-	hdmi_writeb(hdmi, fb_mode->vsync_len, HDMI_FC_VSYNCINWIDTH);
+	vsync_len = mode->vsync_end - mode->vsync_start;
+	hdmi_writeb(hdmi, vsync_len, HDMI_FC_VSYNCINWIDTH);
 }
 
 static void imx_hdmi_phy_disable(struct imx_hdmi *hdmi)
@@ -1383,42 +1382,13 @@ static void hdmi_disable_overflow_interrupts(struct imx_hdmi *hdmi)
 		    HDMI_IH_MUTE_FC_STAT2);
 }
 
-static inline void
-convert_to_video_timing(struct fb_videomode *timing,
-			struct drm_display_mode *mode)
-{
-	memset(timing, 0, sizeof(*timing));
-
-	timing->pixclock = mode->clock * 1000;
-	timing->refresh = drm_mode_vrefresh(mode);
-
-	timing->xres = mode->hdisplay;
-	timing->left_margin = mode->hsync_start - mode->hdisplay;
-	timing->hsync_len = mode->hsync_end - mode->hsync_start;
-	timing->right_margin = mode->htotal - mode->hsync_end;
-
-	timing->yres = mode->vdisplay;
-	timing->upper_margin = mode->vsync_start - mode->vdisplay;
-	timing->vsync_len = mode->vsync_end - mode->vsync_start;
-	timing->lower_margin = mode->vtotal - mode->vsync_end;
-
-	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
-		timing->vmode = FB_VMODE_INTERLACED;
-	else
-		timing->vmode = FB_VMODE_NONINTERLACED;
-
-	if (mode->flags & DRM_MODE_FLAG_DBLSCAN)
-		timing->vmode |= FB_VMODE_DOUBLE;
-}
-
 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;
+	hdmi->vic = drm_match_cea_mode(mode);
 
 	if (!hdmi->vic) {
 		dev_dbg(hdmi->dev, "Non-CEA mode used in HDMI\n");
@@ -1461,7 +1431,7 @@ static int imx_hdmi_setup(struct imx_hdmi *hdmi, struct drm_display_mode *mode)
 	hdmi->hdmi_data.video_mode.mdataenablepolarity = true;
 
 	/* HDMI Initialization Step B.1 */
-	hdmi_av_composer(hdmi);
+	hdmi_av_composer(hdmi, mode);
 
 	/* HDMI Initializateion Step B.2 */
 	ret = imx_hdmi_phy_init(hdmi);

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

[Greg Kroah-Hartman]

On Wed, Oct 16, 2013 at 07:07:35PM +0100, Russell King - ARM Linux wrote:
> 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.

I want it out of staging if it's working properly.  Yours is the first
report of it not working properly, and in fact, probably one of the
first users of the driver, as I haven't gotten any reports of it working
or not at all over the years.

thanks,

greg k-h

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

[Russell King - ARM Linux]

On Wed, Oct 16, 2013 at 11:31:07AM -0700, Greg Kroah-Hartman wrote:
> On Wed, Oct 16, 2013 at 07:07:35PM +0100, Russell King - ARM Linux wrote:
> > 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.
> 
> I want it out of staging if it's working properly.  Yours is the first
> report of it not working properly, and in fact, probably one of the
> first users of the driver, as I haven't gotten any reports of it working
> or not at all over the years.

Well, part of that is because I have this other thing called Armada DRM,
which is a similar thing to imx-drm, except for the Marvell Dove SoCs,
so it's been really quite easy to get a full Ubuntu 12.04 up and running
on the IMX SoC I have here.

As part of that effort, I'm now using my Armada DRM Xorg driver with
imx-drm to test this stuff out.  (Bearing in mind that IMX and Dove use
the same Vivante GPU, there's some sense in getting my Xorg Armada DRM
driver working on both.)

I'm not aware of there being any X drivers for imx-drm (google turns
up nothing), which might be a reason why it hasn't been well tested
and has also languished in drivers/staging for soo long.  Alternatively,
maybe my google searching sucks, or it is out there somewhere but
hidden from googlebot?

To be fair, so far most of the problems I'm finding are with the HDMI
addition to imx-drm rather than imx-drm itself: there's been the lockdep
problem and the clock polarity problem which the HDMI stuff discovered.

Looking at the todo list for moving it out of staging, we have:

- get DRM Maintainer review for this code
- Wait for common display framework to hit mainline and update the IPU
  driver to use it. This will most probably make changes to the devicetree
  bindings necessary.
- Factor out more code to common helper functions
- decide where to put the base driver. It is not specific to a subsystem
  and would be used by DRM/KMS and media/V4L2

(1) is quite a difficult task: I'm waiting for a review of the Armada DRM
stuff, which I'm hoping will make the next merge window.  Rob Clark is
looking at that but has been distracted recently from completing it.

(2) CDF... has been around in RFC according to LWN.net but doesn't seem to
make much in the way of progress from what I can see, despite there
allegedly being "many developers show[ing] interest in the first RFC".
CDF is over a year old now - first RFC 17 Aug 2012, second 22 Nov 2012,
third 9 Aug 2013.

(3) is an on-going effort and really isn't a reason for it to stay in
staging.

(4) is probably the biggest issue.  The problem there is that the IMX
display subsystem is very flexible: it's basically a set of DMA engines
on the front, and behind that a fair number of modules implementing
facilities like image rotation, display driving and image capture.
Display driving alone involves setting up waveforms and writing some
microcode to tell the thing what to do on certain events (like end
of frame etc.)  Hence it doesn't fit well into any particular "Linux"
subsystem.  In this regard, it's a victim of its own flexibility.

I think the biggest problem though is its complexity.  It doesn't fit
into the "single device for a card" model which DRM has.  It's made
up from several separate devices, which is all very well, but it leads
to it having its own private "framework" to glue all the different
devices together - which adds a layer of indirection and makes the code
harder to understand.  As for trying to work out how any of this stuff
works - even though I have the manual which describes all the registers,
I'm struggling with it because there seems to be bits which just aren't
documented and its not always obvious that the code can be relied upon
either.

That all said, I don't think throwing this out of the kernel will do
it any good what so ever - that will just mean that there's even less
potential eyes looking at it.  Yes, some of that is my own selfishness
here - I don't want to have to carry this hunk of code around in my
tree and then be lumbered with trying to get it into mainline myself!

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

[Sascha Hauer]

On Wed, Oct 16, 2013 at 08:02:11PM +0100, Russell King - ARM Linux wrote:
> On Wed, Oct 16, 2013 at 11:31:07AM -0700, Greg Kroah-Hartman wrote:
> > On Wed, Oct 16, 2013 at 07:07:35PM +0100, Russell King - ARM Linux wrote:
> > > 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.
> > 
> > I want it out of staging if it's working properly.  Yours is the first
> > report of it not working properly, and in fact, probably one of the
> > first users of the driver, as I haven't gotten any reports of it working
> > or not at all over the years.
> 
> Well, part of that is because I have this other thing called Armada DRM,
> which is a similar thing to imx-drm, except for the Marvell Dove SoCs,
> so it's been really quite easy to get a full Ubuntu 12.04 up and running
> on the IMX SoC I have here.
> 
> As part of that effort, I'm now using my Armada DRM Xorg driver with
> imx-drm to test this stuff out.  (Bearing in mind that IMX and Dove use
> the same Vivante GPU, there's some sense in getting my Xorg Armada DRM
> driver working on both.)
> 
> I'm not aware of there being any X drivers for imx-drm (google turns
> up nothing), which might be a reason why it hasn't been well tested
> and has also languished in drivers/staging for soo long.  Alternatively,
> maybe my google searching sucks, or it is out there somewhere but
> hidden from googlebot?

There is no X driver for imx-drm. I once tested it with the
xf86-modesetting driver which worked with a few patches back then. These
patches are now part of the modesetting driver.

We have a wrapper tool here which we use to configure the KMS part and
pass the framebuffers to QT.

> 
> To be fair, so far most of the problems I'm finding are with the HDMI
> addition to imx-drm rather than imx-drm itself: there's been the lockdep
> problem and the clock polarity problem which the HDMI stuff discovered.
> 
> Looking at the todo list for moving it out of staging, we have:
> 
> - get DRM Maintainer review for this code
> - Wait for common display framework to hit mainline and update the IPU
>   driver to use it. This will most probably make changes to the devicetree
>   bindings necessary.
> - Factor out more code to common helper functions
> - decide where to put the base driver. It is not specific to a subsystem
>   and would be used by DRM/KMS and media/V4L2
> 
> (1) is quite a difficult task: I'm waiting for a review of the Armada DRM
> stuff, which I'm hoping will make the next merge window.  Rob Clark is
> looking at that but has been distracted recently from completing it.

We have waited for quite a long time aswell before we decided to push
the imx-drm driver to staging to at least get more exposure for the
driver. The situation is, well... unsatisfying.

> 
> (2) CDF... has been around in RFC according to LWN.net but doesn't seem to
> make much in the way of progress from what I can see, despite there
> allegedly being "many developers show[ing] interest in the first RFC".
> CDF is over a year old now - first RFC 17 Aug 2012, second 22 Nov 2012,
> third 9 Aug 2013.

>From what I heard Laurent is still committed to mainline CDF.

> 
> (3) is an on-going effort and really isn't a reason for it to stay in
> staging.
> 
> (4) is probably the biggest issue.  The problem there is that the IMX
> display subsystem is very flexible: it's basically a set of DMA engines
> on the front, and behind that a fair number of modules implementing
> facilities like image rotation, display driving and image capture.
> Display driving alone involves setting up waveforms and writing some
> microcode to tell the thing what to do on certain events (like end
> of frame etc.)  Hence it doesn't fit well into any particular "Linux"
> subsystem.  In this regard, it's a victim of its own flexibility.
> 
> I think the biggest problem though is its complexity.  It doesn't fit
> into the "single device for a card" model which DRM has.  It's made
> up from several separate devices, which is all very well, but it leads
> to it having its own private "framework" to glue all the different
> devices together - which adds a layer of indirection and makes the code
> harder to understand.

I tried adding generic stuff to DRM to support this and failed badly due
to resistance of the maintainers. I didn't get much input what I should
do better or different (besides a general "make it a helper library").
Also nobody seemed to understand the problems I had with the multi device
nature of the i.MX IPU.

I think we made ourselves comfortable with being in the kernel, even if
it's only in staging. We should move forward instead and see how we get
the driver into the kernel.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[Lucas Stach]

Hi Russell,

Am Mittwoch, den 16.10.2013, 20:02 +0100 schrieb Russell King - ARM
Linux:
> On Wed, Oct 16, 2013 at 11:31:07AM -0700, Greg Kroah-Hartman wrote:
> > On Wed, Oct 16, 2013 at 07:07:35PM +0100, Russell King - ARM Linux wrote:
> > > 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.
> > 
> > I want it out of staging if it's working properly.  Yours is the first
> > report of it not working properly, and in fact, probably one of the
> > first users of the driver, as I haven't gotten any reports of it working
> > or not at all over the years.
> 
> Well, part of that is because I have this other thing called Armada DRM,
> which is a similar thing to imx-drm, except for the Marvell Dove SoCs,
> so it's been really quite easy to get a full Ubuntu 12.04 up and running
> on the IMX SoC I have here.
> 
> As part of that effort, I'm now using my Armada DRM Xorg driver with
> imx-drm to test this stuff out.  (Bearing in mind that IMX and Dove use
> the same Vivante GPU, there's some sense in getting my Xorg Armada DRM
> driver working on both.)
> 
> I'm not aware of there being any X drivers for imx-drm (google turns
> up nothing), which might be a reason why it hasn't been well tested
> and has also languished in drivers/staging for soo long.  Alternatively,
> maybe my google searching sucks, or it is out there somewhere but
> hidden from googlebot?
> 

X.Org isn't the center of the world anymore. For some general purpose
distros this might still hold true for some time, but certainly not for
the use-cases we test this driver with.

For most of our embedded use-cases we currently use the FBdev emulation
(phasing this out at the moment) or the apps are sitting right on top of
the raw KMS APIs, rather than on top of a display server.
We also did some experiments with Wayland (Weston) on top of imx-drm,
which worked quite well.

There might be dragons hiding in some corners for the more general
purpose use-cases and we are happy to have you test the driver and
provide valuable reports.

> To be fair, so far most of the problems I'm finding are with the HDMI
> addition to imx-drm rather than imx-drm itself: there's been the lockdep
> problem and the clock polarity problem which the HDMI stuff discovered.
> 
> Looking at the todo list for moving it out of staging, we have:
> 
> - get DRM Maintainer review for this code
> - Wait for common display framework to hit mainline and update the IPU
>   driver to use it. This will most probably make changes to the devicetree
>   bindings necessary.
> - Factor out more code to common helper functions
> - decide where to put the base driver. It is not specific to a subsystem
>   and would be used by DRM/KMS and media/V4L2
> 
> (1) is quite a difficult task: I'm waiting for a review of the Armada DRM
> stuff, which I'm hoping will make the next merge window.  Rob Clark is
> looking at that but has been distracted recently from completing it.
> 
> (2) CDF... has been around in RFC according to LWN.net but doesn't seem to
> make much in the way of progress from what I can see, despite there
> allegedly being "many developers show[ing] interest in the first RFC".
> CDF is over a year old now - first RFC 17 Aug 2012, second 22 Nov 2012,
> third 9 Aug 2013.
> 

Philipp has a prototype of CDF on imx-drm working internally and I
suspect he will be able to post patches shortly after ELCE.

> (3) is an on-going effort and really isn't a reason for it to stay in
> staging.
> 
> (4) is probably the biggest issue.  The problem there is that the IMX
> display subsystem is very flexible: it's basically a set of DMA engines
> on the front, and behind that a fair number of modules implementing
> facilities like image rotation, display driving and image capture.
> Display driving alone involves setting up waveforms and writing some
> microcode to tell the thing what to do on certain events (like end
> of frame etc.)  Hence it doesn't fit well into any particular "Linux"
> subsystem.  In this regard, it's a victim of its own flexibility.

We are aiming to do the same thing as the Tegra host1x driver: move the
IPU core to drivers/gpu and export API for other drivers to use. This
may mean we still have to keep the DRM part in staging (at least until
the CDF thing is sorted, as this prevents us from setting the devicetree
bindings in stone), but at least should be a step in the right
direction.

Regards,
Lucas

-- 
Pengutronix e.K.                           | Lucas Stach                 |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-5076 |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[Russell King - ARM Linux]

On Wed, Oct 16, 2013 at 11:31:07AM -0700, Greg Kroah-Hartman wrote:
> On Wed, Oct 16, 2013 at 07:07:35PM +0100, Russell King - ARM Linux wrote:
> > 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.
> 
> I want it out of staging if it's working properly.  Yours is the first
> report of it not working properly, and in fact, probably one of the
> first users of the driver, as I haven't gotten any reports of it working
> or not at all over the years.

Next problem... unbinding, rebinding, and then unbinding the imx-drm
device produces the nice oops below.  I've not debugged this yet.

Alignment trap: not handling instruction e1932f9f at [<c00758ec>]
Unhandled fault: alignment exception (0x001) at 0x6e72666f
Internal error: : 1 [#1] SMP ARM
Modules linked in: fuse bnep rfcomm bluetooth
CPU: 0 PID: 1125 Comm: bash Tainted: G        W    3.12.0-rc4+ #123
task: d0d6ec00 ti: d7ff2000 task.ti: d7ff2000
PC is at __lock_acquire+0x1a8/0x1e14
LR is at lock_acquire+0x68/0x7c
pc : [<c00758f0>]    lr : [<c0077aa8>]    psr: 20070093
sp : d7ff3cc8  ip : 00000000  fp : d7ff3d54
r10: db2a6334  r9 : 00000000  r8 : 6e72656b
r7 : c0846208  r6 : c08852cc  r5 : d0d6ec00  r4 : 00000002
r3 : 6e72666f  r2 : db2a6334  r1 : 00000001  r0 : d7ff2000
Flags: nzCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment user
Control: 10c53c7d  Table: 20df804a  DAC: 00000015
Process bash (pid: 1125, stack limit = 0xd7ff2240)
Stack: (0xd7ff3cc8 to 0xd7ff4000)
3cc0:                   00000006 00000007 c0cd22f0 00000006 d7ff3d1c d7ff3ce8
3ce0: c00782f8 c0075050 00000001 00000000 db801f00 d7ff2000 c0078648 d7ff2000
3d00: 00000001 d7ff3e08 d7ff3e60 d7ff3dd8 d7ff3d3c d7ff3d20 00000000 d7ff3e08
3d20: d7ff3d7c d7ff3d30 c0072a58 00000000 d7ff2000 60070013 d0d6ec00 d7ff2000
3d40: c06679c8 d0d6ec00 d7ff3d8c d7ff3d58 c0077aa8 c0075754 00000002 00000000
3d60: 00000000 c02ff27c 00000000 00000002 db2a62fc c02ff27c c08852cc 00000000
3d80: d7ff3de4 d7ff3d90 c05f80c4 c0077a4c 00000002 00000000 c02ff27c c0072a20
3da0: dad64000 d7ff3e24 d7ff3df8 d7ff3e04 d7ff3e5c d0d6f000 c013f1c8 db322880
3dc0: db2a6440 db2a6000 c0872568 00000008 c06679c8 db286000 d7ff3e04 d7ff3de8
3de0: c02ff27c c05f8074 db280f00 db322880 db2a6000 db29b810 d7ff3e1c d7ff3e08
3e00: c02f13ac c02ff268 d0e55000 c087265c d7ff3e2c d7ff3e20 c0464d7c c02f1398
3e20: d7ff3e54 d7ff3e30 c02f506c c0464d68 d0e55000 c087265c db29b810 c0872568
3e40: 00000008 db286000 d7ff3e74 d7ff3e58 c02fa284 c02f503c c087265c c087265c
3e60: db29b810 00000008 d7ff3e8c d7ff3e78 c02fb900 c02fa258 db29b810 c0872678
3e80: d7ff3e9c d7ff3e90 c0464d54 c02fb8d4 d7ff3eac d7ff3ea0 c0312bfc c0464d44
3ea0: d7ff3ec4 d7ff3eb0 c03113b0 c0312be8 db29b844 db29b810 d7ff3edc d7ff3ec8
3ec0: c0311434 c0311344 c0872678 c085b588 d7ff3efc d7ff3ee0 c03103ac c0311418
3ee0: c941b300 c941b318 d7ff3f70 db28c280 d7ff3f0c d7ff3f00 c030f934 c0310338
3f00: d7ff3f3c d7ff3f10 c013d7d0 c030f918 d7ff3f70 dbb5e600 00000008 003ba408
3f20: d7ff3f70 00000000 00000000 00000008 d7ff3f6c d7ff3f40 c00db77c c013d6d4
3f40: 00000001 c000eb44 dbb5e600 00000000 d7ff3f70 003ba408 00000000 00000008
3f60: d7ff3fa4 d7ff3f70 c00dbb58 c00db6b8 00000000 00000000 d7ff3f94 b6f7fa78
3f80: 00000008 003ba408 00000004 c000eb44 d7ff2000 00000000 00000000 d7ff3fa8
3fa0: c000e980 c00dbb18 b6f7fa78 00000008 00000001 003ba408 00000008 00000000
3fc0: b6f7fa78 00000008 003ba408 00000004 bee5c9ec 000a6094 00000000 003f7f08
3fe0: 00000000 bee5c96c b6eee747 b6f2611c 40070010 00000001 00000138 00000020
Backtrace: 
[<c0075748>] (__lock_acquire+0x0/0x1e14) from [<c0077aa8>] (lock_acquire+0x68/0x7c)
[<c0077a40>] (lock_acquire+0x0/0x7c) from [<c05f80c4>] (mutex_lock_nested+0x5c/0x394)
 r7:00000000 r6:c08852cc r5:c02ff27c r4:db2a62fc
[<c05f8068>] (mutex_lock_nested+0x0/0x394) from [<c02ff27c>] (drm_modeset_lock_all+0x20/0x58)
[<c02ff25c>] (drm_modeset_lock_all+0x0/0x58) from [<c02f13ac>] (drm_fbdev_cma_restore_mode+0x20/0x34)
 r6:db29b810 r5:db2a6000 r4:db322880 r3:db280f00
[<c02f138c>] (drm_fbdev_cma_restore_mode+0x0/0x34) from [<c0464d7c>] (imx_drm_driver_lastclose+0x20/0x24)
 r5:c087265c r4:d0e55000
[<c0464d5c>] (imx_drm_driver_lastclose+0x0/0x24) from [<c02f506c>] (drm_lastclose+0x3c/0x174)
[<c02f5030>] (drm_lastclose+0x0/0x174) from [<c02fa284>] (drm_put_dev+0x38/0x154)
[<c02fa24c>] (drm_put_dev+0x0/0x154) from [<c02fb900>] (drm_platform_exit+0x38/0x5c)
 r7:00000008 r6:db29b810 r5:c087265c r4:c087265c
[<c02fb8c8>] (drm_platform_exit+0x0/0x5c) from [<c0464d54>] (imx_drm_platform_remove+0x1c/0x24)
 r5:c0872678 r4:db29b810
[<c0464d38>] (imx_drm_platform_remove+0x0/0x24) from [<c0312bfc>] (platform_drv_remove+0x20/0x24)
[<c0312bdc>] (platform_drv_remove+0x0/0x24) from [<c03113b0>] (__device_release_driver+0x78/0xd4)
[<c0311338>] (__device_release_driver+0x0/0xd4) from [<c0311434>] (device_release_driver+0x28/0x34)
 r5:db29b810 r4:db29b844
[<c031140c>] (device_release_driver+0x0/0x34) from [<c03103ac>] (unbind_store+0x80/0x98)
 r5:c085b588 r4:c0872678
[<c031032c>] (unbind_store+0x0/0x98) from [<c030f934>] (drv_attr_store+0x28/0x34)
 r7:db28c280 r6:d7ff3f70 r5:c941b318 r4:c941b300
[<c030f90c>] (drv_attr_store+0x0/0x34) from [<c013d7d0>] (sysfs_write_file+0x108/0x188)
[<c013d6c8>] (sysfs_write_file+0x0/0x188) from [<c00db77c>] (vfs_write+0xd0/0x19c)
[<c00db6ac>] (vfs_write+0x0/0x19c) from [<c00dbb58>] (SyS_write+0x4c/0x78)
[<c00dbb0c>] (SyS_write+0x0/0x78) from [<c000e980>] (ret_fast_syscall+0x0/0x48)
Code: 0affffbd e2883f41 f593f000 e1932f9f (e2822001) 
---[ end trace f2924517b64a28f4 ]---

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

[Russell King - ARM Linux]

On Sun, Oct 20, 2013 at 01:04:34AM +0100, Russell King - ARM Linux wrote:
> On Wed, Oct 16, 2013 at 11:31:07AM -0700, Greg Kroah-Hartman wrote:
> > On Wed, Oct 16, 2013 at 07:07:35PM +0100, Russell King - ARM Linux wrote:
> > > 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.
> > 
> > I want it out of staging if it's working properly.  Yours is the first
> > report of it not working properly, and in fact, probably one of the
> > first users of the driver, as I haven't gotten any reports of it working
> > or not at all over the years.
> 
> Next problem... unbinding, rebinding, and then unbinding the imx-drm
> device produces the nice oops below.  I've not debugged this yet.

I've been tweaking the ARM oops format slightly (I hope this isn't going
to end up breaking any tools... it's only a minor change.)  I've been
using this as a way of testing it.  The change here is using %ps instead
of %pS for the first symbolic print in each entry (it's always the start
of the function).  The other change is fixing the saved register set so
saves including r10 get printed - and its better formatted.


As for imx-drm, there was a warning which preceded that oops.  Here's the
full log, below the "---------" marker - this is from unbinding the imx-drm
module, and then trying to reboot.

imx-drm is really very broken in the way it tries to bend DRM to be
used in DT - it doesn't consider the lifetime for anything like the
CRTCs, connectors or encoders.  All these have empty .destroy functions
to them. If we unbind imx-drm, the top level drm_device tries to be
destroyed, but it leaves behind all the CRTCs, connectors and encoders,
causing the first warning because none of the framebuffers got cleaned
up through that destruction (because the functions did nothing.)

The second one is through trying to clean up the framebuffer, which is
still in use.

The third one is caused because there's still allocated memory objects
against the DRM memory manager - again, because nothing has been cleaned
up.

Finally, the oops at the end.  I thought that's a candidate for
enabling kobject debugging and release debugging, but as soon as I do,
it perversely goes away!  Eventually I managed to reproduce it, and
it's the same bug which was investigated recently wrt sysfs lifetimes.
I would test David's fix for this, but it *isn't* generated against an
-rc kernel, it rejects fairly horribly.

Since it happens without kobject debugging enabled, it would be good
to get the fix in for 3.12-rc.  David?

kobject: 'card0' (db2bfc18): kobject_add_internal: parent: 'drm', set: 'devices'
kobject: 'card0' (db2bfc18): kobject_uevent_env
kobject: 'card0' (db2bfc18): fill_kobj_path: path = '/devices/platform/imx-drm/drm/card0'
[drm] Supports vblank timestamp caching Rev 1 (10.10.2010).
[drm] No driver support for vblank timestamp query.
...
kobject: 'card0' (db2bfc18): kobject_uevent_env
kobject: 'card0' (db2bfc18): fill_kobj_path: path = '/devices/platform/imx-drm/drm/card0'
kobject: 'imx-hdmi' (db293840): kobject_uevent_env
kobject: 'imx-hdmi' (db293840): fill_kobj_path: path = '/bus/platform/drivers/imx-hdmi'
...
kobject: 'card0' (db2bfc18): kobject_uevent_env
kobject: 'card0' (db2bfc18): fill_kobj_path: path = '/devices/platform/imx-drm/drm/card0'
kobject: 'card0-HDMI-A-1' (db837020): kobject_uevent_env
kobject: 'card0-HDMI-A-1' (db837020): fill_kobj_path: path = '/devices/platform/imx-drm/drm/card0/card0-HDMI-A-1'
...
kobject: 'card0' (db2bfc18): kobject_uevent_env
kobject: 'card0' (db2bfc18): fill_kobj_path: path = '/devices/platform/imx-drm/drm/card0'
kobject: 'card0-HDMI-A-1' (db837020): kobject_uevent_env
kobject: 'card0-HDMI-A-1' (db837020): fill_kobj_path: path = '/devices/platform/imx-drm/drm/card0/card0-HDMI-A-1'
...
kobject: 'card0' (db2bfc18): kobject_uevent_env
kobject: 'card0' (db2bfc18): fill_kobj_path: path = '/devices/platform/imx-drm/drm/card0'
kobject: 'drm' (db28fb00): kobject_release, parent db2bf418 (delayed)
[drm] Module unloaded
kobject: 'controlD64' (db2bf818): kobject_cleanup, parent   (null)
kobject: 'controlD64' (db2bf818): calling ktype release
kobject: 'controlD64': free name
kobject: 'drm' (db28fb00): kobject_cleanup, parent db2bf418
kobject: 'drm' (db28fb00): auto cleanup kobject_del
kobject: 'drm' (db28fb00): calling ktype release
kobject: 'drm': free name
...
WARNING: CPU: 0 PID: 1027 at /home/rmk/git/linux-rmk/include/linux/kref.h:47 kobject_get+0x60/0x74()
Modules linked in: fuse rfcomm bnep bluetooth
CPU: 0 PID: 1027 Comm: reboot Tainted: G        W    3.12.0-rc4+ #128
Backtrace:
[<c001218c>] (dump_backtrace) from [<c0012328>] (show_stack+0x18/0x1c)
 r6:c0646298 r5:0000002f r4:00000000 r3:00000000
[<c0012310>] (show_stack) from [<c05f6a78>] (dump_stack+0x70/0x90)
[<c05f6a08>] (dump_stack) from [<c00233a0>] (warn_slowpath_common+0x6c/0x8c)
 r4:00000000 r3:db142400
[<c0023334>] (warn_slowpath_common) from [<c00233e4>] (warn_slowpath_null+0x24/0x2c)
 r8:c088361c r7:db837024 r6:c0dc7fa4 r5:c08834f2 r4:db2bfc18
                                                    ^^^^^^^^ kobject address
[<c00233c0>] (warn_slowpath_null) from [<c027fbc8>] (kobject_get+0x60/0x74)
[<c027fb68>] (kobject_get) from [<c030dbfc>] (get_device+0x1c/0x24)
 r5:00000000 r4:db837018
[<c030dbe0>] (get_device) from [<c030fb0c>] (device_shutdown+0x90/0x19c)

-------------------
WARNING: CPU: 0 PID: 997 at /home/rmk/git/linux-rmk/drivers/gpu/drm/drm_crtc.c:4036 drm_mode_config_cleanup+0x250/0x25c()
Modules linked in: fuse bnep rfcomm bluetooth
CPU: 0 PID: 997 Comm: bash Not tainted 3.12.0-rc4+ #127
Backtrace: 
[<c001218c>] (dump_backtrace) from [<c0012328>] (show_stack+0x18/0x1c)
 r6:c065dc74 r5:00000fc4 r4:00000000 r3:00000000
[<c0012310>] (show_stack) from [<c05f5f90>] (dump_stack+0x70/0x90)
[<c05f5f20>] (dump_stack) from [<c00233a0>] (warn_slowpath_common+0x6c/0x8c)
 r4:00000000 r3:d7c75a00
[<c0023334>] (warn_slowpath_common) from [<c00233e4>] (warn_slowpath_null+0x24/0x2c)
 r8:db2a64cc r7:db2a6410 r6:db2a64c0 r5:db2a6000 r4:db2a64c0
[<c00233c0>] (warn_slowpath_null) from [<c03008b0>] (drm_mode_config_cleanup+0x250/0x25c)
[<c0300660>] (drm_mode_config_cleanup) from [<c0465494>] (imx_drm_driver_unload+0x1c/0x2c)
 r10:db286000 r9:c06679c8 r8:00000008 r7:c0872568 r6:db29b810 r5:c087265c
 r4:db280f00 r3:d7c75a00
[<c0465478>] (imx_drm_driver_unload) from [<c02fa29c>] (drm_put_dev+0x50/0x154)
 r4:db2a6000 r3:c0465478
[<c02fa24c>] (drm_put_dev) from [<c02fb900>] (drm_platform_exit+0x38/0x5c)
 r7:00000008 r6:db29b810 r5:c087265c r4:c087265c
[<c02fb8c8>] (drm_platform_exit) from [<c0464d54>] (imx_drm_platform_remove+0x1c/0x24)
 r5:c0872678 r4:db29b810
[<c0464d38>] (imx_drm_platform_remove) from [<c0312bfc>] (platform_drv_remove+0x20/0x24)
[<c0312bdc>] (platform_drv_remove) from [<c03113b0>] (__device_release_driver+0x78/0xd4)
[<c0311338>] (__device_release_driver) from [<c0311434>] (device_release_driver+0x28/0x34)
 r5:db29b810 r4:db29b844
[<c031140c>] (device_release_driver) from [<c03103ac>] (unbind_store+0x80/0x98)
 r5:c085b588 r4:c0872678
[<c031032c>] (unbind_store) from [<c030f934>] (drv_attr_store+0x28/0x34)
 r7:db28c280 r6:d7d9ff70 r5:d7d9a898 r4:d7d9a880
[<c030f90c>] (drv_attr_store) from [<c013d7d0>] (sysfs_write_file+0x108/0x188)
[<c013d6c8>] (sysfs_write_file) from [<c00db77c>] (vfs_write+0xd0/0x19c)
 r10:00000008 r9:00000000 r8:00000000 r7:d7d9ff70 r6:0132f408 r5:00000008
 r4:d7df5d00 r3:d7d9ff70
[<c00db6ac>] (vfs_write) from [<c00dbb58>] (SyS_write+0x4c/0x78)
 r10:00000008 r8:00000000 r7:0132f408 r6:d7d9ff70 r5:00000000 r4:d7df5d00
[<c00dbb0c>] (SyS_write) from [<c000e980>] (ret_fast_syscall+0x0/0x48)
 r10:00000000 r9:d7d9e000 r8:c000eb44 r7:00000004 r6:0132f408 r5:00000008
 r4:b6f72a78
---[ end trace 44f221ca42d7ae54 ]---
------------[ cut here ]------------
WARNING: CPU: 0 PID: 997 at /home/rmk/git/linux-rmk/drivers/gpu/drm/drm_crtc.c:571 drm_framebuffer_remove+0x10c/0x118()
Modules linked in: fuse bnep rfcomm bluetooth
CPU: 0 PID: 997 Comm: bash Tainted: G        W    3.12.0-rc4+ #127
Backtrace: 
[<c001218c>] (dump_backtrace) from [<c0012328>] (show_stack+0x18/0x1c)
 r6:c065dc74 r5:0000023b r4:00000000 r3:00000000
[<c0012310>] (show_stack) from [<c05f5f90>] (dump_stack+0x70/0x90)
[<c05f5f20>] (dump_stack) from [<c00233a0>] (warn_slowpath_common+0x6c/0x8c)
 r4:00000000 r3:d7c75a00
[<c0023334>] (warn_slowpath_common) from [<c00233e4>] (warn_slowpath_null+0x24/0x2c)
 r8:db2a6000 r7:db2a6410 r6:db2a64c0 r5:dae7b280 r4:db32ea00
[<c00233c0>] (warn_slowpath_null) from [<c0300654>] (drm_framebuffer_remove+0x10c/0x118)
[<c0300548>] (drm_framebuffer_remove) from [<c0300800>] (drm_mode_config_cleanup+0x1a0/0x25c)
 r10:00200200 r9:00100100 r8:db2a64cc r7:db2a6410 r6:db2a64c0 r5:db2a6000
 r4:db32ea00
[<c0300660>] (drm_mode_config_cleanup) from [<c0465494>] (imx_drm_driver_unload+0x1c/0x2c)
 r10:db286000 r9:c06679c8 r8:00000008 r7:c0872568 r6:db29b810 r5:c087265c
 r4:db280f00 r3:d7c75a00
[<c0465478>] (imx_drm_driver_unload) from [<c02fa29c>] (drm_put_dev+0x50/0x154)
 r4:db2a6000 r3:c0465478
[<c02fa24c>] (drm_put_dev) from [<c02fb900>] (drm_platform_exit+0x38/0x5c)
 r7:00000008 r6:db29b810 r5:c087265c r4:c087265c
[<c02fb8c8>] (drm_platform_exit) from [<c0464d54>] (imx_drm_platform_remove+0x1c/0x24)
 r5:c0872678 r4:db29b810
[<c0464d38>] (imx_drm_platform_remove) from [<c0312bfc>] (platform_drv_remove+0x20/0x24)
[<c0312bdc>] (platform_drv_remove) from [<c03113b0>] (__device_release_driver+0x78/0xd4)
[<c0311338>] (__device_release_driver) from [<c0311434>] (device_release_driver+0x28/0x34)
 r5:db29b810 r4:db29b844
[<c031140c>] (device_release_driver) from [<c03103ac>] (unbind_store+0x80/0x98)
 r5:c085b588 r4:c0872678
[<c031032c>] (unbind_store) from [<c030f934>] (drv_attr_store+0x28/0x34)
 r7:db28c280 r6:d7d9ff70 r5:d7d9a898 r4:d7d9a880
[<c030f90c>] (drv_attr_store) from [<c013d7d0>] (sysfs_write_file+0x108/0x188)
[<c013d6c8>] (sysfs_write_file) from [<c00db77c>] (vfs_write+0xd0/0x19c)
 r10:00000008 r9:00000000 r8:00000000 r7:d7d9ff70 r6:0132f408 r5:00000008
 r4:d7df5d00 r3:d7d9ff70
[<c00db6ac>] (vfs_write) from [<c00dbb58>] (SyS_write+0x4c/0x78)
 r10:00000008 r8:00000000 r7:0132f408 r6:d7d9ff70 r5:00000000 r4:d7df5d00
[<c00dbb0c>] (SyS_write) from [<c000e980>] (ret_fast_syscall+0x0/0x48)
 r10:00000000 r9:d7d9e000 r8:c000eb44 r7:00000004 r6:0132f408 r5:00000008
 r4:b6f72a78
---[ end trace 44f221ca42d7ae55 ]---
------------[ cut here ]------------
WARNING: CPU: 0 PID: 997 at /home/rmk/git/linux-rmk/drivers/gpu/drm/drm_mm.c:578 drm_mm_takedown+0x2c/0x34()
Memory manager not clean during takedown.
Modules linked in: fuse bnep rfcomm bluetooth
CPU: 0 PID: 997 Comm: bash Tainted: G        W    3.12.0-rc4+ #127
Backtrace: 
[<c001218c>] (dump_backtrace) from [<c0012328>] (show_stack+0x18/0x1c)
 r6:c065dbc8 r5:00000242 r4:00000000 r3:00000000
[<c0012310>] (show_stack) from [<c05f5f90>] (dump_stack+0x70/0x90)
[<c05f5f20>] (dump_stack) from [<c00233a0>] (warn_slowpath_common+0x6c/0x8c)
 r4:d7d9fdf8 r3:d7c75a00
[<c0023334>] (warn_slowpath_common) from [<c0023464>] (warn_slowpath_fmt+0x38/0x40)
 r8:00000008 r7:c0872568 r6:db2a6110 r5:db286080 r4:db286080
[<c0023430>] (warn_slowpath_fmt) from [<c02fd5f0>] (drm_mm_takedown+0x2c/0x34)
 r3:db2860ac r2:c065dbfc
[<c02fd5c4>] (drm_mm_takedown) from [<c030b3d0>] (drm_vma_offset_manager_destroy+0x1c/0x28)
[<c030b3b4>] (drm_vma_offset_manager_destroy) from [<c02f6d94>] (drm_gem_destroy+0x1c/0x30)
 r4:db2a6000 r3:00003000
[<c02f6d78>] (drm_gem_destroy) from [<c02fa384>] (drm_put_dev+0x138/0x154)
 r5:db2a6110 r4:db2a6000
[<c02fa24c>] (drm_put_dev) from [<c02fb900>] (drm_platform_exit+0x38/0x5c)
 r7:00000008 r6:db29b810 r5:c087265c r4:c087265c
[<c02fb8c8>] (drm_platform_exit) from [<c0464d54>] (imx_drm_platform_remove+0x1c/0x24)
 r5:c0872678 r4:db29b810
[<c0464d38>] (imx_drm_platform_remove) from [<c0312bfc>] (platform_drv_remove+0x20/0x24)
[<c0312bdc>] (platform_drv_remove) from [<c03113b0>] (__device_release_driver+0x78/0xd4)
[<c0311338>] (__device_release_driver) from [<c0311434>] (device_release_driver+0x28/0x34)
 r5:db29b810 r4:db29b844
[<c031140c>] (device_release_driver) from [<c03103ac>] (unbind_store+0x80/0x98)
 r5:c085b588 r4:c0872678
[<c031032c>] (unbind_store) from [<c030f934>] (drv_attr_store+0x28/0x34)
 r7:db28c280 r6:d7d9ff70 r5:d7d9a898 r4:d7d9a880
[<c030f90c>] (drv_attr_store) from [<c013d7d0>] (sysfs_write_file+0x108/0x188)
[<c013d6c8>] (sysfs_write_file) from [<c00db77c>] (vfs_write+0xd0/0x19c)
 r10:00000008 r9:00000000 r8:00000000 r7:d7d9ff70 r6:0132f408 r5:00000008
 r4:d7df5d00 r3:d7d9ff70
[<c00db6ac>] (vfs_write) from [<c00dbb58>] (SyS_write+0x4c/0x78)
 r10:00000008 r8:00000000 r7:0132f408 r6:d7d9ff70 r5:00000000 r4:d7df5d00
[<c00dbb0c>] (SyS_write) from [<c000e980>] (ret_fast_syscall+0x0/0x48)
 r10:00000000 r9:d7d9e000 r8:c000eb44 r7:00000004 r6:0132f408 r5:00000008
 r4:b6f72a78
---[ end trace 44f221ca42d7ae56 ]---
[drm] Module unloaded
Alignment trap: not handling instruction e1932f9f at [<c00758ec>]
Unhandled fault: alignment exception (0x001) at 0x63700b11
Internal error: : 1 [#1] SMP ARM
Modules linked in: fuse bnep rfcomm bluetooth
CPU: 0 PID: 1050 Comm: reboot Tainted: G        W    3.12.0-rc4+ #127
task: d7f68000 ti: d7f4a000 task.ti: d7f4a000
PC is at __lock_acquire+0x1a8/0x1e14
LR is at lock_acquire+0x68/0x7c
pc : [<c00758f0>]    lr : [<c0077aa8>]    psr: 200f0093
sp : d7f4bd18  ip : 00000000  fp : d7f4bda4
r10: db2ac07c  r9 : 00000000  r8 : 63700a0d
r7 : c0846208  r6 : c08852cc  r5 : d7f68000  r4 : 00000002
r3 : 63700b11  r2 : db2ac07c  r1 : 00000001  r0 : d7f4a000
Flags: nzCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment user
Control: 10c53c7d  Table: 27f0404a  DAC: 00000015
Process reboot (pid: 1050, stack limit = 0xd7f4a240)
Stack: (0xd7f4bd18 to 0xd7f4c000)
bd00:                                                       d7f4bdb4 00000002
bd20: d7f68000 c08852cc 00000001 00000074 00000002 db83ac18 d7f4bdd4 00000000
bd40: c0075a50 c0075050 c00782f8 d7f4a000 8c10a086 0000000a 00000000 d7f4a000
bd60: c0cd22f0 d7f683a0 d7f4bdac 00000000 00546074 00000000 c098f118 00000000
bd80: d7f4a000 600f0013 d7f68000 d7f4a000 d7f4a000 d7f68000 d7f4bddc d7f4bda8
bda0: c0077aa8 c0075754 00000002 00000000 00000000 c030f814 00000000 c088131c
bdc0: db2ac044 c030f814 c08852cc 00000000 d7f4be34 d7f4bde0 c05f80c4 c0077a4c
bde0: 00000002 00000000 c030f814 c0dc5b64 db2acc1c db83ac08 db2ac010 c0dc5b64
be00: db2b0024 c088131c db83ac08 db2b0018 db2ac010 c0dc5b64 db2b0024 c088131c
be20: d7f4a000 00000000 d7f4be5c d7f4be38 c030f814 c05f8074 c0883788 00000000
be40: c0844cc8 00000000 00000003 c000eb44 d7f4be6c d7f4be60 c0048988 c030f74c
be60: d7f4be84 d7f4be70 c00489a0 c0048958 01234567 4321fedc d7f4bfa4 d7f4be88
be80: c0048af4 c0048998 000000a8 db3bee50 00076046 00000000 0000000e d7f4a000
bea0: d7f4bea0 db42310c d7f4bea8 00000000 00000000 00000000 db265970 00000001
bec0: d7f4bedc d7f4bed0 c00a9050 00000000 d7f4a000 600f0013 d7f68000 00000007
bee0: c084192c b6f04714 d7f4bfb0 00000000 00000000 b6f52f3c d7f4bfac d7f4bf08
bf00: c00084b0 c0019e60 00000002 db422ff0 db85d088 c0836300 c083c934 db422ff0
bf20: db422ff0 db423040 d7f4bf54 d7f4bf38 c00f47f8 d7f4a000 c000e9a4 d7f4a000
bf40: c000ea10 d7f68000 00000001 00000000 d7f4a000 b6f52f3c d7f4bf84 d7f4bf68
bf60: c00784cc c0078294 00000000 00000000 bebff7b4 00000058 d7f4bf94 d7f4bf88
bf80: c00785a4 c00783e4 00000000 00000000 bebff7b4 00000058 00000000 d7f4bfa8
bfa0: c000e980 c0048a18 00000000 00000000 fee1dead 28121969 01234567 00000003
bfc0: 00000000 00000000 bebff7b4 00000058 00000000 00000000 b6f52f3c 00000000
bfe0: 00000058 bebff624 b6ea0a1d b6e2b276 20000030 fee1dead 00222c40 002d394c
Backtrace: 
[<c0075748>] (__lock_acquire) from [<c0077aa8>] (lock_acquire+0x68/0x7c)
 r10:d7f68000 r9:d7f4a000 r8:d7f4a000 r7:d7f68000 r6:600f0013 r5:d7f4a000
 r4:00000000
[<c0077a40>] (lock_acquire) from [<c05f80c4>] (mutex_lock_nested+0x5c/0x394)
 r7:00000000 r6:c08852cc r5:c030f814 r4:db2ac044
[<c05f8068>] (mutex_lock_nested) from [<c030f814>] (device_shutdown+0xd4/0x19c)
 r10:00000000 r9:d7f4a000 r8:c088131c r7:db2b0024 r6:c0dc5b64 r5:db2ac010
 r4:db2b0018
[<c030f740>] (device_shutdown) from [<c0048988>] (kernel_restart_prepare+0x3c/0x40)
 r8:c000eb44 r7:00000003 r6:00000000 r5:c0844cc8 r4:00000000 r3:c0883788
[<c004894c>] (kernel_restart_prepare) from [<c00489a0>] (kernel_restart+0x14/0x68)
[<c004898c>] (kernel_restart) from [<c0048af4>] (SyS_reboot+0xe8/0x1cc)
 r4:4321fedc r3:01234567
[<c0048a0c>] (SyS_reboot) from [<c000e980>] (ret_fast_syscall+0x0/0x48)
 r7:00000058 r6:bebff7b4 r5:00000000 r4:00000000
Code: 0affffbd e2883f41 f593f000 e1932f9f (e2822001) 
---[ end trace 44f221ca42d7ae57 ]---

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

[Russell King - ARM Linux]

On Sun, Oct 20, 2013 at 02:00:57PM +0100, Russell King - ARM Linux wrote:
> As for imx-drm, there was a warning which preceded that oops.  Here's the
> full log, below the "---------" marker - this is from unbinding the imx-drm
> module, and then trying to reboot.
> 
> imx-drm is really very broken in the way it tries to bend DRM to be
> used in DT - it doesn't consider the lifetime for anything like the
> CRTCs, connectors or encoders.  All these have empty .destroy functions
> to them. If we unbind imx-drm, the top level drm_device tries to be
> destroyed, but it leaves behind all the CRTCs, connectors and encoders,
> causing the first warning because none of the framebuffers got cleaned
> up through that destruction (because the functions did nothing.)
> 
> The second one is through trying to clean up the framebuffer, which is
> still in use.
> 
> The third one is caused because there's still allocated memory objects
> against the DRM memory manager - again, because nothing has been cleaned
> up.

Right, so how imx-drm "works" as far as DRM initialization is by a wing
and a prayer at the moment.

It works like this - the driver relies heavily upon this sequence:

- imx_drm_init()
  - creates an imx_drm_device structure to contain references to other
    parts.
  - registers the imx-drm platform device and an associated structure.
    - the platform device is immediately probed, causing it to be registered
      with the DRM subsystem.
    - the DRM subsystem creates the drm_device structure, and calls the
      drivers ->load method.
    - the driver initialises some basic data, places a pointer to the
      drm_device into the imx_drm_device and returns
- imx_pd_driver_init()
  - registers the imx_pd_driver platform device driver for DT devices
    with a compatible string of "fsl,imx-parallel-display"
  - such devices will be immediately probed
    - these allocate an imx_parallel_display structure, which contains
      a drm_connector and drm_encoder structure embedded within.
    - these structures are registered into the core of imx_drm, and
      via the imx_drm_device structure, are both attached to the
      drm_device immediately.
- imx_tve_driver_init()
  essentially the same as imx_pd_driver_init()
- imx_ldb_driver_init()
  essentially the same as imx_pd_driver_init()
- imx_ipu_driver_init()
  - registers a platform driver fot DT devices with a compatible string
    of "fsl,imx51-ipu", "fsl,imx53-ipu", or "fsl,imx6q-ipu".
  - initialises such devices, and creates two new platform devices
    called "imx-ipuv3-crtc", one for each display interface.
- ipu_drm_driver_init()
  - registers a platform driver for "imx-ipuv3-crtc" devices.
    - for each device found
      - it allocates a ipu_crtc device structure, which embeds a drm_crtc
        structure.
      - it registers a CRTC via imx_drm_add_crtc().
        - this allocates an imx_drm_crtc structure, and eventually registers
          the drm_crtc structure against the drm_device
- imx_hdmi_driver_init
  similar to imx_pd_driver_init

All that sequence is in init level 6.  The last bit comes in init level 7
(the late_initcall):

- imx_fb_helper_init()
  - this grabs the drm_device, and calls drm_fbdev_cma_init() on it
    hoping that we know the number of CRTCs at this point.  This is
    held indefinitely.
  - the resulting drm_fbdev_cma is saved into the imx_drm_device.

Now, if the imx-drm device is unbound from its driver, all hell breaks
loose - none of these crtc/connector/encoder structures have a meaningful
destroy function - their destruction is all in their individual driver
"remove" functions.  This causes some warnings to be spat out from DRM.

Amongst this is the "last_close" callback which looks at the imx_drm_device,
sees that drm_fbdev_cma is registered against it, and calls
drm_fbdev_cma_restore_mode() on it.  drm_fbdev_cma contains objects which
store a pointer to the drm_device structure that it was registered against,
which exists at this point, so everything is fine.

The unload proceeds, and eventually the drm_device is freed.

Now, if we rebind the imx-drm device, causing the probe actions above to
be repeated.  imx_drm_device still contains a pointer to the drm_fbdev_cma
object that was allocated...  Let's ignore the fact that none of the
sub-modules have re-initialised anything against this new drm_device.

The real fun comes when you try and unbind it again.  This time, the
drm_device which is being torn down isn't the one in drm_fbdev_cma,
but we still call drm_fbdev_cma_restore_mode().  This tries to get a
mutex on the _original_ drm_device->mode_config.mutex, which has been
freed.  The result is a kernel oops.

Now, several things stand out here: piece-meal construction of a
drm_device in this manner is unsafe:
- it relies heavily on all devices already being present at the time that
  the above sequence starts, and it assumes that the drivers will probe
  immediately, as soon as they are registered.
- the late_initcall() is really a "barrier" on the initialisation sequence:
  no CRTCs can be registered after that point.
- it is impossible to tear down and re-create the subsystem when the device
  goes wrong as you can never clear out that CMA fbdev helper, even if you
  unbind every other device in that setup in the right order.
- each of these drivers is itself a separate loadable module, which means
  when built in a modular state and loaded under Ubuntu 12.04, which does
  multi-threaded module loading, there is no guarantee of any initialisation
  order.

Really, this kind of hack needs to be outlawed.  Trying to bend a subsystem
which has a fairly solid model around a single "device" into this kind of
multi-device for the sake of DT is really buggered.

This is not an imx-drm specific problem: this the same problem which the
Armada DRM driver would have with DT: DT is based around describing
individual components of hardware separately rather than describing
something at high level.

I've been looking at what can be done with imx-drm.  One thing that
desperately needs sorting is that drm_fbdev_cma thing, which has to be
registered after all CRTCs.  The problem is that there is _no way_ to
positively know when all CRTCs have been registered, because they're
separate devices entirely.

For other stuff, it would require quite an amount of restructuring, so
that sub-modules don't "probe" themselves until the main drm_device is
known to be in place, and clean themselves up in the ->destroy callbacks.
... maybe.  Sorting out the connectors/encoders might actually be the
easier bit of the task, something which I'm currently looking into at
the moment.

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

[Russell King - ARM Linux]

On Sun, Oct 20, 2013 at 05:31:56PM +0100, Russell King - ARM Linux wrote:
> On Sun, Oct 20, 2013 at 02:00:57PM +0100, Russell King - ARM Linux wrote:
> > As for imx-drm, there was a warning which preceded that oops.  Here's the
> > full log, below the "---------" marker - this is from unbinding the imx-drm
> > module, and then trying to reboot.
> > 
> > imx-drm is really very broken in the way it tries to bend DRM to be
> > used in DT - it doesn't consider the lifetime for anything like the
> > CRTCs, connectors or encoders.  All these have empty .destroy functions
> > to them. If we unbind imx-drm, the top level drm_device tries to be
> > destroyed, but it leaves behind all the CRTCs, connectors and encoders,
> > causing the first warning because none of the framebuffers got cleaned
> > up through that destruction (because the functions did nothing.)
> > 
> > The second one is through trying to clean up the framebuffer, which is
> > still in use.
> > 
> > The third one is caused because there's still allocated memory objects
> > against the DRM memory manager - again, because nothing has been cleaned
> > up.
> 
> Right, so how imx-drm "works" as far as DRM initialization is by a wing
> and a prayer at the moment.
> 
> It works like this - the driver relies heavily upon this sequence:
> 
> - imx_drm_init()
>   - creates an imx_drm_device structure to contain references to other
>     parts.
>   - registers the imx-drm platform device and an associated structure.
>     - the platform device is immediately probed, causing it to be registered
>       with the DRM subsystem.
>     - the DRM subsystem creates the drm_device structure, and calls the
>       drivers ->load method.
>     - the driver initialises some basic data, places a pointer to the
>       drm_device into the imx_drm_device and returns
> - imx_pd_driver_init()
>   - registers the imx_pd_driver platform device driver for DT devices
>     with a compatible string of "fsl,imx-parallel-display"
>   - such devices will be immediately probed
>     - these allocate an imx_parallel_display structure, which contains
>       a drm_connector and drm_encoder structure embedded within.
>     - these structures are registered into the core of imx_drm, and
>       via the imx_drm_device structure, are both attached to the
>       drm_device immediately.
> - imx_tve_driver_init()
>   essentially the same as imx_pd_driver_init()
> - imx_ldb_driver_init()
>   essentially the same as imx_pd_driver_init()
> - imx_ipu_driver_init()
>   - registers a platform driver fot DT devices with a compatible string
>     of "fsl,imx51-ipu", "fsl,imx53-ipu", or "fsl,imx6q-ipu".
>   - initialises such devices, and creates two new platform devices
>     called "imx-ipuv3-crtc", one for each display interface.
> - ipu_drm_driver_init()
>   - registers a platform driver for "imx-ipuv3-crtc" devices.
>     - for each device found
>       - it allocates a ipu_crtc device structure, which embeds a drm_crtc
>         structure.
>       - it registers a CRTC via imx_drm_add_crtc().
>         - this allocates an imx_drm_crtc structure, and eventually registers
>           the drm_crtc structure against the drm_device
> - imx_hdmi_driver_init
>   similar to imx_pd_driver_init
> 
> All that sequence is in init level 6.  The last bit comes in init level 7
> (the late_initcall):
> 
> - imx_fb_helper_init()
>   - this grabs the drm_device, and calls drm_fbdev_cma_init() on it
>     hoping that we know the number of CRTCs at this point.  This is
>     held indefinitely.
>   - the resulting drm_fbdev_cma is saved into the imx_drm_device.
> 
> Now, if the imx-drm device is unbound from its driver, all hell breaks
> loose - none of these crtc/connector/encoder structures have a meaningful
> destroy function - their destruction is all in their individual driver
> "remove" functions.  This causes some warnings to be spat out from DRM.
> 
> Amongst this is the "last_close" callback which looks at the imx_drm_device,
> sees that drm_fbdev_cma is registered against it, and calls
> drm_fbdev_cma_restore_mode() on it.  drm_fbdev_cma contains objects which
> store a pointer to the drm_device structure that it was registered against,
> which exists at this point, so everything is fine.
> 
> The unload proceeds, and eventually the drm_device is freed.
> 
> Now, if we rebind the imx-drm device, causing the probe actions above to
> be repeated.  imx_drm_device still contains a pointer to the drm_fbdev_cma
> object that was allocated...  Let's ignore the fact that none of the
> sub-modules have re-initialised anything against this new drm_device.
> 
> The real fun comes when you try and unbind it again.  This time, the
> drm_device which is being torn down isn't the one in drm_fbdev_cma,
> but we still call drm_fbdev_cma_restore_mode().  This tries to get a
> mutex on the _original_ drm_device->mode_config.mutex, which has been
> freed.  The result is a kernel oops.
> 
> Now, several things stand out here: piece-meal construction of a
> drm_device in this manner is unsafe:
> - it relies heavily on all devices already being present at the time that
>   the above sequence starts, and it assumes that the drivers will probe
>   immediately, as soon as they are registered.
> - the late_initcall() is really a "barrier" on the initialisation sequence:
>   no CRTCs can be registered after that point.
> - it is impossible to tear down and re-create the subsystem when the device
>   goes wrong as you can never clear out that CMA fbdev helper, even if you
>   unbind every other device in that setup in the right order.
> - each of these drivers is itself a separate loadable module, which means
>   when built in a modular state and loaded under Ubuntu 12.04, which does
>   multi-threaded module loading, there is no guarantee of any initialisation
>   order.
> 
> Really, this kind of hack needs to be outlawed.  Trying to bend a subsystem
> which has a fairly solid model around a single "device" into this kind of
> multi-device for the sake of DT is really buggered.
> 
> This is not an imx-drm specific problem: this the same problem which the
> Armada DRM driver would have with DT: DT is based around describing
> individual components of hardware separately rather than describing
> something at high level.
> 
> I've been looking at what can be done with imx-drm.  One thing that
> desperately needs sorting is that drm_fbdev_cma thing, which has to be
> registered after all CRTCs.  The problem is that there is _no way_ to
> positively know when all CRTCs have been registered, because they're
> separate devices entirely.
> 
> For other stuff, it would require quite an amount of restructuring, so
> that sub-modules don't "probe" themselves until the main drm_device is
> known to be in place, and clean themselves up in the ->destroy callbacks.
> ... maybe.  Sorting out the connectors/encoders might actually be the
> easier bit of the task, something which I'm currently looking into at
> the moment.

Further comments as a result of this: I have a much better solution
in place for encoders and connectors.  I'm now able to remove and
re-add back connectors.  This all sounds good, but it doesn't quite
work.

The reason is that the fb_helper layer caches pointers to the
connectors, and number of connectors.  This doesn't get updated when
we remove and add connectors - which causes another set of problems.

A similar problem exists if we add a connector after the fb helper
has been initialised.

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

[Russell King - ARM Linux]

Another problem.

After performing several modesets, the IPU seems to lock up and produce
no syncs or output data.

I've seen this many times over the last week while testing out various
aspects of imx-drm, and had put it down to problems with the clocking
arrangement getting its settings wrong.  Now that I've sorted all that
though, and I still have the problem, there's something else going on.

What I see is:
- the HDMI clock is running correctly (right frequency and unmodulated)
- the TMDS data lines show signs of there being some data (probably
  control, guard bands and data islands from the frame composer in the
  HDMI interface).  The data lines are definitely lacking image data though.
- reading the various status registers indicates that all FIFOs within
  the IPU are empty.
- the attached TV says that there is no HDMI signal.

One of my tests has been to cycle through all display resolutions from the
smallest width to the largest, leaving each one set for 30 seconds.  This
will occasionally provoke the problem, but obviously is rather slow to do
so.

I tried this with a less demanding test last night as far as a change in
the settings: switching between 720p at 50 and 60Hz.  The clocks for these
two modes are the same at 74.25MHz, and the vertical timing parameters are
identical.  The only timing difference is with the horizontal parameters:

  1280x720 (0x41)   74.2MHz +HSync +VSync +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
  1280x720 (0x4f)   74.2MHz +HSync +VSync
        h: width  1280 start 1720 end 1760 total 1980 skew    0 clock   37.5KHz
        v: height  720 start  725 end  730 total  750           clock   50.0Hz

This dies within a couple of minutes.  I haven't gathered enough
information to tell whether it always dies when switching from 50 -> 60Hz
or whether it's any switch.

My test for this is basically:

while :; do
  xrandr -s 1280x720 -r 50
  sleep 5
  xrandr -s 1280x720 -r 60
  sleep 5
done

[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@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"


Troy

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

[Russell King - ARM Linux]

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);
}

However, there's no detection of whether a FC packet queue overflow
occurs: yes, sure, the interrupts are unmasked, but the irq handler
does nothing with it.

However, there is something of interest here.  Is there any ordering
requirement between the setup of the IPU and HDMI?

At present, the DRM core does this order:

- encoders: prepare callback
- crtc: prepare callback
- crtc: mode_set
- encoders: mode_set
- crtc: commit callback
- encoders: commit callback

This translates to:

- disable HDMI
- disable IPU
- configure IPU
- configure *and* enable HDMI output
- enable IPU
- reconfigure and enable HDMI output

Now, what I've just discovered is that killing off the 2nd reconfiguration
and enable in the HDMI commit callback gets rid of the magenta line, but
now I'm left with a picture which occasionally blanks.  I've tried using
devmem2 to write the INVIDCONF register three times and then hit the
reset bit (./devmem2 0x124002 b 0xfd) but that doesn't cure this.

However, after X goes into DPMS and wakes up, it's fine.

If I comment out the configure and enable in the mode_set, and leave
the one in the commit callback, I get the magenta line back.

I think there's a clue in the errata document:

  Each time one writes to some FC registers, and depending on the clock
  relation of sfr clk and tmds clk ...

That seems to imply that there's supposed to be various clocks running
here while these registers are accessed.  If the IPU is all shut down,
and the HDMI itself is disabled, there's no TMDS clock.  That could
explain why the video timings appear to be wrong.

Oh my, and while reading through this, I find:

        spin_lock_irqsave(&hdmi->irq_lock, flags);
                
        ret = clk_prepare_enable(hdmi->iahb_clk);
        if (ret)
                return ret;

That is sooo wrong.  Not only is that returning leaving a spin lock in
the locked state, but clk_prepare_enable() _can_ sleep.  And then I've
turned my attention to finding out what exactly irq_lock protects?
Would it be preventing the IRQ handler from doing stuff?  Well, the
IRQ handler doesn't take the lock at all, so that can't be it.  The
only function which takes that lock is imx_hdmi_fb_registered(), which
is only called at driver probe time.  What use is this lock?  What's
it protecting?  As far as I can see, it's doing nothing apart from
disabling interrupts.  And that doesn't work if you're running on a
dual-CPU or quad-CPU IMX6.

This driver just seems to be wrong in soo many ways, it's really not
funny.

So, here's the thing: is there any documentation around (apart from
the applications processor reference manual) which gives guidance as
to how the IPU and HDMI are supposed to be configured on the IMX6, or
is that all secret NDA stuff which only Freescale people are allowed
to know?  Why?  At this point I'm beginning to think that rewriting
at least the HDMI backend from scratch is probably the best solution.

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

[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

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

[Russell King - ARM Linux]

On Wed, Oct 16, 2013 at 02:03:17PM -0700, Troy Kisky wrote:
> 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.

Just tried that - and yes, it does work (I'm on i.MX 6Solo for which
cpu_is_mx6dl() would return true.)  Well done!

Indeed yes, the workaround in the code Fabio has differs from the
procedure given in the errata.

Note that this gives a new problem: we shouldn't use cpu_is_mx6dl()
in drivers - differences like this should be specified via DT properties.
I think we need this to recognise both fsl,imx6q-hdmi and fsl,imx6dl-hdmi
so that this workaround can detect when its running on a Solo/DL SoC.

Well, I now have quite a pile of patches for the hdmi code. :(

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

[Russell King - ARM Linux]

Okay, next problem...

As I described via the Cubox-i community last night on google+...

I'm now at the point where certain resolutions and refreshes work fine
(eg, 720p @ 50 or 60Hz, 1366x768, 1024x768).

Others either don't display (1080p, 800x600, 848x480, 640x480), or have
speckles, a line of corruption that flashes up, and blanks (576p @50 Hz,
480p @60Hz), possibly a result of loss of signal.

I've been looking at the DMFC, suspecting a fifo problem, but that to me
looks fine - we seem to always allocate 4 slots, with each slot having a
bandwidth of 99Mpixels each.  This in theory should give a maximum of
396Mpixels, which is more than plenty.  The bandwidth calculation is
probably wrong though - the peak bandwidth is actually the pixel clock
since this is the rate at which pixels have to be supplied during a line,
not the refresh * hdisplayed * vdisplayed - that's the average bandwidth
over a full frame.

However, if it was a DMFC problem, and as this only carries pixel data,
I'd expect that not to cause loss of signal.

I've checked the phy MPLL settings back to the manual for certain problem
clock rates, and they also seem fine.  I've polled the status register to
see whether it's unstable, and it appears to remain locked.

I think I should probe the HDMI signals, particularly the clock signal to
see if that provides any useful clues.

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

[Greg KH]

On Thu, Oct 03, 2013 at 03:51:25PM -0300, Fabio Estevam wrote:
> This is based on the initial work done by Sascha Hauer and Tony Prisk.
> 
> Tested on imx6q-wandboard and imx6q-sabresd boards.

Why is _any_ new work going on here instead of working to get this code
out of staging?

I really don't want to accept this at all, as I'm not seeing any effort
to get this out of staging and into the proper place in the drm tree,
sorry.

And I shouldn't have to be saying this at all, you all know better than
to try to add new hardware support for staging drivers...

greg k-h