Re: [RFC] i.MX DRM devicetree binding
nel0-edid = [edid-data];
> + channel1-ddc = <&i2c0>;
> + channel0-crtcs = <&ipu0 0>, <&ipu0 1>, <&ipu1 0>, <&ipu1 1>;
> + channel1-crtcs = <&ipu0 0>, <&ipu0 1>, <&ipu1 0>, <&ipu1 1>;
> +};
> +
> +Specifying CRTCs connected to display output devices
> +
> +
> +Display output device nodes should specify which CRTCs they can be
> +connected to in their crtcs property, containing a 'crtc-list':
> +
> + crtc-list ::= [crtc-list]
> + single-crtc ::=
> + crtc-phandle : phandle to device node providing the crtc
> + crtc-specifier : Array of #crtc-cells specifying specific crtc
> + (controller specific, 1 on imx-ipuv3)
> +
> +In the following example, two image processing units ipu0 and ipu1
> +provide two CRTCs each, indexed with a single cell. The hdmi connector
> +can be connected to either CRTC of ipu1, and the lvds connector is
> +fixed to the second CRTC of ipu0.
> +
> + ipu0: ipu0 {
> + #crtc-cells = <1>;
> + };
> + ipu1: ipu1 {
> + #crtc-cells = <1>;
> + };
> + [...]
> + hdmi: hdmi {
> + ddc = <&i2c2>;
> + crtcs = <&ipu1 0 &ipu1 1>;
> + };
> + lvds: lvds {
> + edid = [edid-data];
> + crtcs = <&ipu0 1>;
> + };
> +
> +Note that the crtc-specifier length is controller dependent. A simple
> +lcdc controller with a single CRTC should use #crtc-cells = <0>:
> +
> + lcdc: lcdc {
> + #crtc-cells = <0>;
> + };
> + [...]
> + parallel-display {
> + crtcs = <&lcdc>
> + };
> +
Best regards,
--
David Jander
Protonic Holland.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC] i.MX DRM devicetree binding
On Thu, 14 Jun 2012 16:45:33 +0200 Sascha Hauer wrote: > On Thu, Jun 14, 2012 at 04:10:16PM +0200, David Jander wrote: > > On Thu, 14 Jun 2012 15:07:56 +0200 > > Sascha Hauer wrote: > > > > > + > > > +Required properties: > > > +- compatible: Should be "fsl,imx-parallel-display" > > > +- crtc: the crtc this display is connected to, see below > > > +Optional properties: > > > +- interface_pix_fmt: How this display is connected to the > > > + crtc. Currently supported types: "rgb24", "rgb565" > > > +- edid: verbatim EDID data block describing attached display. > > > > I never really understood why one should put EDID data in a device-tree. It > > carries a lot of irrelevant information and is in a quite hostile format. > > The > > only reason it exists actually is historically grown "intelligency" built > > into > > VESA compatible CRT monitors for PC's what do _we_ have to do with that? > > There isn't even a decent tool to generate this data on linux. > > On top of that, the examples I have seen of EDID blobs in device-trees so > > far > > are just plain wrong and even contain device id's from "Samsung SyncMaster" > > and other such stuff that IMHO has no place here. > > > > But we need an alternative way of communicating timing parameters. Can't we > > use something more ad-hoc, like the data one would give to DRM_MODE() in > > drm_crtc.h? > > Generally +1 for this. Thanks ;-) > I Just don't want to open up another front for now, but I am willing to > support a generic (not i.MX specific) format to describe display timings > in devicetree. I understand. > The only discussion I know about was here: > > https://lists.ozlabs.org/pipermail/linuxppc-dev/2010-February/080683.html Oh yeah. I remember vaguely having dealt with that one. I think we actually use the code being discussed there on another boards, and also remember feeling acute pain from tying to figure out how to cough up some useful EDID data blob for our LCD panels. It felt wrong on all ends, and I think I ended up hacking something around this in order to avoid having to deal with EDID. > The outcome was that the suggested format was not entirely generic and > that it would be better to use some existing format rather than > inventing something new. I disagree here. The thing called modeline, > drm_display_mode or fb_videomode is well established and it is known > which parameters (most) displays need, so it should be possible to have > a generic display description in the devicetree. Right! It really can't be that complicated. > However, we need EDID data parsing anyway in the kernel and being able Why? This also feels almost entirely wrong, specially on an embedded system. IMHO, encoders that actually receive EDID data via legacy DDC interfaces, should convert this data immediately to generic timing data, and the kernel should use that instead. But I agree that this is a different discussion and should be held elsewhere. > to overwrite the (maybe broken monitor supplied) EDID data in the > devicetree might become handy. So having this way of supplying EDID data > is good to have even if there is a generic display description. I don't see why it would be good to have, but I agree that since it is optional, it can perfectly well be ignored and we can use something else instead. In fact, once there is "something else", I doubt anyone would want to use EDID in a device-tree. EVK's like Babbage and LOCO have DDC interfaces, and all other real embedded systems I can think of would rather benefit from more straight-forward timing information, which can probably be copied almost verbatim from the datasheet of the LCD panel. Best regards, -- David Jander Protonic Holland. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[RFC] i.MX DRM devicetree binding
t; + channel0-edid = [edid-data];
> + channel1-ddc = <&i2c0>;
> + channel0-crtcs = <&ipu0 0>, <&ipu0 1>, <&ipu1 0>, <&ipu1 1>;
> + channel1-crtcs = <&ipu0 0>, <&ipu0 1>, <&ipu1 0>, <&ipu1 1>;
> +};
> +
> +Specifying CRTCs connected to display output devices
> +
> +
> +Display output device nodes should specify which CRTCs they can be
> +connected to in their crtcs property, containing a 'crtc-list':
> +
> + crtc-list ::= [crtc-list]
> + single-crtc ::=
> + crtc-phandle : phandle to device node providing the crtc
> + crtc-specifier : Array of #crtc-cells specifying specific crtc
> + (controller specific, 1 on imx-ipuv3)
> +
> +In the following example, two image processing units ipu0 and ipu1
> +provide two CRTCs each, indexed with a single cell. The hdmi connector
> +can be connected to either CRTC of ipu1, and the lvds connector is
> +fixed to the second CRTC of ipu0.
> +
> + ipu0: ipu0 {
> + #crtc-cells = <1>;
> + };
> + ipu1: ipu1 {
> + #crtc-cells = <1>;
> + };
> + [...]
> + hdmi: hdmi {
> + ddc = <&i2c2>;
> + crtcs = <&ipu1 0 &ipu1 1>;
> + };
> + lvds: lvds {
> + edid = [edid-data];
> + crtcs = <&ipu0 1>;
> + };
> +
> +Note that the crtc-specifier length is controller dependent. A simple
> +lcdc controller with a single CRTC should use #crtc-cells = <0>:
> +
> + lcdc: lcdc {
> + #crtc-cells = <0>;
> + };
> + [...]
> + parallel-display {
> + crtcs = <&lcdc>
> + };
> +
Best regards,
--
David Jander
Protonic Holland.
[RFC] i.MX DRM devicetree binding
On Thu, 14 Jun 2012 16:45:33 +0200 Sascha Hauer wrote: > On Thu, Jun 14, 2012 at 04:10:16PM +0200, David Jander wrote: > > On Thu, 14 Jun 2012 15:07:56 +0200 > > Sascha Hauer wrote: > > > > > + > > > +Required properties: > > > +- compatible: Should be "fsl,imx-parallel-display" > > > +- crtc: the crtc this display is connected to, see below > > > +Optional properties: > > > +- interface_pix_fmt: How this display is connected to the > > > + crtc. Currently supported types: "rgb24", "rgb565" > > > +- edid: verbatim EDID data block describing attached display. > > > > I never really understood why one should put EDID data in a device-tree. It > > carries a lot of irrelevant information and is in a quite hostile format. > > The > > only reason it exists actually is historically grown "intelligency" built > > into > > VESA compatible CRT monitors for PC's what do _we_ have to do with that? > > There isn't even a decent tool to generate this data on linux. > > On top of that, the examples I have seen of EDID blobs in device-trees so > > far > > are just plain wrong and even contain device id's from "Samsung SyncMaster" > > and other such stuff that IMHO has no place here. > > > > But we need an alternative way of communicating timing parameters. Can't we > > use something more ad-hoc, like the data one would give to DRM_MODE() in > > drm_crtc.h? > > Generally +1 for this. Thanks ;-) > I Just don't want to open up another front for now, but I am willing to > support a generic (not i.MX specific) format to describe display timings > in devicetree. I understand. > The only discussion I know about was here: > > https://lists.ozlabs.org/pipermail/linuxppc-dev/2010-February/080683.html Oh yeah. I remember vaguely having dealt with that one. I think we actually use the code being discussed there on another boards, and also remember feeling acute pain from tying to figure out how to cough up some useful EDID data blob for our LCD panels. It felt wrong on all ends, and I think I ended up hacking something around this in order to avoid having to deal with EDID. > The outcome was that the suggested format was not entirely generic and > that it would be better to use some existing format rather than > inventing something new. I disagree here. The thing called modeline, > drm_display_mode or fb_videomode is well established and it is known > which parameters (most) displays need, so it should be possible to have > a generic display description in the devicetree. Right! It really can't be that complicated. > However, we need EDID data parsing anyway in the kernel and being able Why? This also feels almost entirely wrong, specially on an embedded system. IMHO, encoders that actually receive EDID data via legacy DDC interfaces, should convert this data immediately to generic timing data, and the kernel should use that instead. But I agree that this is a different discussion and should be held elsewhere. > to overwrite the (maybe broken monitor supplied) EDID data in the > devicetree might become handy. So having this way of supplying EDID data > is good to have even if there is a generic display description. I don't see why it would be good to have, but I agree that since it is optional, it can perfectly well be ignored and we can use something else instead. In fact, once there is "something else", I doubt anyone would want to use EDID in a device-tree. EVK's like Babbage and LOCO have DDC interfaces, and all other real embedded systems I can think of would rather benefit from more straight-forward timing information, which can probably be copied almost verbatim from the datasheet of the LCD panel. Best regards, -- David Jander Protonic Holland.
[PATCH] staging: imx-drm: Fix pixel clock polarity
clk_pol should almost always be 1 (active-high pixel clock). When using the LDB, it must be 1 and for externally connected displays it should most probably also be 1. Original Freescale code sets this bit always, except when the FB_SYNC_CLK_LAT_FALL flag is set, which is a proprietary extension from Freescale that is not normally set. Signed-off-by: David Jander --- drivers/gpu/drm/imx/ipuv3-crtc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c index 7425fcc..97799ff 100644 --- a/drivers/gpu/drm/imx/ipuv3-crtc.c +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c @@ -173,7 +173,7 @@ static int ipu_crtc_mode_set(struct drm_crtc *crtc, sig_cfg.clkflags = 0; sig_cfg.enable_pol = 1; - sig_cfg.clk_pol = 0; + sig_cfg.clk_pol = 1; sig_cfg.bus_format = ipu_crtc->bus_format; sig_cfg.v_to_h_sync = 0; sig_cfg.hsync_pin = ipu_crtc->di_hsync_pin; -- 2.1.4
[PATCH] staging: imx-drm: Fix pixel clock polarity
Dear Philipp,
On Fri, 26 Jun 2015 12:27:03 +0200
Philipp Zabel wrote:
> Hi David,
>
> Am Freitag, den 26.06.2015, 09:51 +0200 schrieb David Jander:
> > clk_pol should almost always be 1 (active-high pixel clock). When using the
> > LDB, it must be 1 and for externally connected displays it should most
> > probably also be 1. Original Freescale code sets this bit always, except
> > when the FB_SYNC_CLK_LAT_FALL flag is set, which is a proprietary extension
> > from Freescale that is not normally set.
> >
> > Signed-off-by: David Jander
> > ---
> > drivers/gpu/drm/imx/ipuv3-crtc.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c
> > b/drivers/gpu/drm/imx/ipuv3-crtc.c index 7425fcc..97799ff 100644
> > --- a/drivers/gpu/drm/imx/ipuv3-crtc.c
> > +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
> > @@ -173,7 +173,7 @@ static int ipu_crtc_mode_set(struct drm_crtc *crtc,
> > sig_cfg.clkflags = 0;
> >
> > sig_cfg.enable_pol = 1;
> > - sig_cfg.clk_pol = 0;
> > + sig_cfg.clk_pol = 1;
> > sig_cfg.bus_format = ipu_crtc->bus_format;
> > sig_cfg.v_to_h_sync = 0;
> > sig_cfg.hsync_pin = ipu_crtc->di_hsync_pin;
>
> have a look at 85de9d17c485 ("imx-drm: match ipu_di_signal_cfg's clk_pol
> with its description."), which inverts the meaning of clk_pol.
> In the freescale code I see in drivers/video/mxc/mxc_ipuv3_fb.c:
>
> memset(&sig_cfg, 0, sizeof(sig_cfg));
> /* ... */
> if (!(fbi->var.sync & FB_SYNC_CLK_LAT_FALL))
> sig_cfg.clk_pol = true;
>
> And in drivers/mxc/ipu3/ipu_disp.c:
>
> if (!sig.clk_pol)
> di_gen |= DI_GEN_POLARITY_DISP_CLK;
> ipu_di_write(ipu, disp, di_gen, DI_GENERAL);
>
> So if FB_SYNC_CLK_LAT_FALL is not set, clk_pol defaults to true, but
> this causes the POLARITY_DISP_CLK bit in DI_GENERAL to be cleared (which
> is documented as active low). We do the same, just the other way around.
> drivers/gpu/drm/imx/ipuv3-crtc.c:
>
> sig_cfg.clk_pol = 0;
>
> And in drivers/gpu/ipu-v3/ipu-di.c:
>
> if (sig->clk_pol)
> di_gen |= DI_GEN_POLARITY_DISP_CLK;
> ipu_di_write(di, di_gen, DI_GENERAL);
>
> At least with the HDMI output I have verified that the picture is
> currently correct but gets shifted by a pixel with your patch.
This is odd. We started investigating because one of our customers (using an
LVDS connected panel) reported some boards causing color errors in gradients.
If you displayed for example a horizontal red gradient from 0 to 255, you would
see 3 stripes through the gradient (dividing it into 4 parts) of pixels that
are too bright. Not all the boards had this issue, and in many of them itwas
an unstable symptom, which immediately pointed at a problem related to process
variations. The symptoms can be explained by a latch in the LDB (LVDS bridge)
that latches the individual bits in the shift register on the same clock-edge
that those signals change. So the internal signal delay can cause either the
old or the new value of the pixel-bit getting latched. The color stripes are
then caused by the roll-over from xx0111 to xx1000, where bit 3 gets latched
at it's new value, while all lower bits get latched at their previous value.
Most of the time this situation was unstable. I even have scope images showing
the anomaly on the LVDS signal. This patch fixes the issue for all boards.
If you are confident that your code is correct, then maybe the LDB expects an
inverted clock signal?
I must admit that this patch is rather old. I tested it thoroughly on 3.17,
but AFAICS, the patch you mention is also included in 3.17, so I suppose the
situation has not changed.
Best regards,
--
David Jander
Protonic Holland.
[PATCH] staging: imx-drm: Fix pixel clock polarity
Hi Philipp, On Fri, 26 Jun 2015 17:31:18 +0200 Philipp Zabel wrote: > Hi David, > > Am Freitag, den 26.06.2015, 13:26 +0200 schrieb David Jander: > > This is odd. We started investigating because one of our customers (using > > an LVDS connected panel) reported some boards causing color errors in > > gradients. If you displayed for example a horizontal red gradient from 0 > > to 255, you would see 3 stripes through the gradient (dividing it into 4 > > parts) of pixels that are too bright. Not all the boards had this issue, > > and in many of them itwas an unstable symptom, which immediately pointed > > at a problem related to process variations. The symptoms can be explained > > by a latch in the LDB (LVDS bridge) that latches the individual bits in > > the shift register on the same clock-edge that those signals change. So > > the internal signal delay can cause either the old or the new value of the > > pixel-bit getting latched. The color stripes are then caused by the > > roll-over from xx0111 to xx1000, where bit 3 gets latched at it's new > > value, while all lower bits get latched at their previous value. Most of > > the time this situation was unstable. I even have scope images showing the > > anomaly on the LVDS signal. This patch fixes the issue for all boards. > > I have seen similar artifacts in the red channel of LVDS0 on an i.MX6S > once where changing clock polarity didn't have any effect, but switching > from DI0 to DI1 helped. That's weird, but I assume unrelated to this issue. > > If you are confident that your code is correct, then maybe the LDB expects > > an inverted clock signal? > > There are parallel panels that sample on the rising pixel clock edge, > for those this information should be part of the panel descriptor. Yes, I have seen those. > For > LVDS the clock waveform is standardized, so this should be an issue in > the LDB. On the other hand, the serializer in the LDB shouldn't care > about the pixel clock polarity if it uses the 7x serializer clock and an > internal counter compared to the counter_reset_val field to determine at > which point to load the shift registers. But that's my understanding I assume the 7x serializer clock is synchronized with the pixel clock. On external LVDS transceivers, there's a PLL to generate the 7x clock derived from the pixel-clock, but since this is an internal LVDS bridge, it uses a generated clock for the serializer that's set to 7x the pixel clock. I think that the clock _output_ of the LVDS interface is a synthesized signal that is generated from the serializer clock. This is easier to design. That makes the whole LDB block synchronous to the pixel-clock and it will always generate a standards-compliant clock signal to the panel, BUT... if the input pixel clock is out of phase, strange things can happen. I am pretty sure that the LDB latches the pixel data to the shift registers on the wrong edge. The scope images are clear as water. I don't know whether this is a silicon-bug or a driver bug, but definitely inverting the pixel clock fixes the problem. Just to make this entirely clear: The LVDS signal that comes out of the LVDS0 channel is clearly broken (on a small percentage of boards). It is NOT the display panel that is misinterpreting an otherwise valid LVDS signal. > from the reference manual, which doesn't necessarily have to be the same > as reality. Maybe the pixel clock latches the input bus signal onto an > internal register. Yes, I am pretty sure it does. Problem is that this latch is supposed to work on the other edge of the clock. > I have just tried three different devices with LVDS displays, that show > no reaction to changes of the clk_pol setting. I know. It appears on one out of ten boards maybe, and sometimes it takes a while for the problem to appear... > > I must admit that this patch is rather old. I tested it thoroughly on 3.17, > > but AFAICS, the patch you mention is also included in 3.17, so I suppose > > the situation has not changed. > > Do you still have devices that show this issue available to test? I'd be > very interested to know whether switching DIs or using the LDB_DI parent > selection procedure described in > http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/313618.html > makes any difference. We can try this out. I'll try to find a board that has this issue. It was on a i.MX6U (Dual-light). Unfortunately I don't think I manage to do it this week, and from next week on I'll be on vacation, so it can take a while... Again, all this testing was on mainline 3.17, so it will be good to check that we still can reproduce this problem with 4.1. Best regards, -- David Jander Protonic Holland.
