Re: [PATCH v3] imx-drm: Add mx6 hdmi transmitter support

2013-10-30 Thread Russell King - ARM Linux
On Mon, Oct 28, 2013 at 06:26:49PM -0200, Fabio Estevam wrote:
 + /*Wait for PHY PLL lock */
 + msec = 4;
 + val = hdmi_readb(hdmi, HDMI_PHY_STAT0)  HDMI_PHY_TX_PHY_LOCK;
 + while (!val) {
 + udelay(1000);
 + if (msec-- == 0) {
 + dev_dbg(hdmi-dev, PHY PLL not locked\n);
 + return -EINVAL;
 + }
 + val = hdmi_readb(hdmi, HDMI_PHY_STAT0)  HDMI_PHY_TX_PHY_LOCK;
 + }

This loop is not always sufficient for the PLL to always lock.  It's also
buggy because it behaves like this:

msec = 4
read register
check bit - still one?
yes - wait 1ms
msec == 0?
no - msec = 3
read register
... msec eventually msec = 0
read register
check bit - still one
yes - wait 1ms
msec == 0?
yes - print debug and exit

So the last wait for 1ms performs no real function other than to delay
the inevitable exit.  If we really want to wait 5ms for the condition
we're testing to become true, then do it like this:

msec = 5;
do {
val = hdmi_readb(hdmi, HDMI_PHY_STAT0)  HDMI_PHY_TX_PHY_LOCK;
if (!val)
break;
 
if (msec == 0) {
dev_err(hdmi-dev, PHY PLL not locked\n);
return -EINVAL;
}
 
udelay(1000);
msec--;
} while (1);

This way, we check for the success condition immediately before checking
for the failure condition, and only waiting after that.

I'd also suggest making it a dev_err() so that it's obvious when it
doesn't lock - the current dev_dbg() remains hidden and causes
failures which don't seem to be visible to the user via the logs.
Hence why this issue has been overlooked...

 +static void hdmi_av_composer(struct imx_hdmi *hdmi,
 +  const struct drm_display_mode *mode)
 +{
 + u8 inv_val;
 + struct hdmi_vmode *vmode = hdmi-hdmi_data.video_mode;
 + int hblank, vblank, h_de_hs, v_de_vs, hsync_len, vsync_len;
 +
 + 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);

We can do better here:

vmode-mpixelclock = mode-clock * 1000;

as the above is not quite correct when considering all the
possibilities.  Since we're given the pixel clock in the mode structure,
we might as well make use it.

 + /* Set up VSYNC active edge delay (in pixel clks) */
 + vsync_len = mode-vsync_end - mode-vsync_start;
 + hdmi_writeb(hdmi, vsync_len, HDMI_FC_VSYNCINWIDTH);

Commentry - vsync len is in lines not pixel clocks.

Other than that, it seems to work here on the Carrier-1.  Thanks.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v3] imx-drm: Add mx6 hdmi transmitter support

2013-10-29 Thread Fabio Estevam
On Mon, Oct 28, 2013 at 7:19 PM, Russell King - ARM Linux
li...@arm.linux.org.uk wrote:
 On Mon, Oct 28, 2013 at 06:26:49PM -0200, Fabio Estevam wrote:
 From: Fabio Estevam fabio.este...@freescale.com

 This is based on the initial work done by Sascha Hauer and Tony Prisk.

 It looks like you've also taken some of the suggestions I've made too -
 like the voltage drive and symbol transmit control settings.

 It would be nice to identify the changes a bit better so that those of
 us who are testing your drivers (and have patches on top of them to make
 things work) know what's changed.

Yes, I have been out the two past weeks and tried to capture all of
your inputs concerning the HDMI driver.

Today I was able to get access to a wandboard solo and the HDMI image
does look different compared to a wandboard quad board:

- The Linux logo penguins are steady on mx6q, but on mx6solo the
contour lines look like they flicker.
- The kernel log text appears in good white colour with mx6q, but not
so white on mx6solo (tending to green).

Even the U-boot HDMI splash screen is not very steady on mx6solo.

I will try to look at these mx6 solo HDMI issues.

Regards,

Fabio Estevam
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v3] imx-drm: Add mx6 hdmi transmitter support

2013-10-29 Thread Russell King - ARM Linux
On Tue, Oct 29, 2013 at 01:45:50PM -0200, Fabio Estevam wrote:
 On Tue, Oct 29, 2013 at 1:00 PM, Fabio Estevam feste...@gmail.com wrote:
 
  Today I was able to get access to a wandboard solo and the HDMI image
  does look different compared to a wandboard quad board:
 
  - The Linux logo penguins are steady on mx6q, but on mx6solo the
  contour lines look like they flicker.
  - The kernel log text appears in good white colour with mx6q, but not
  so white on mx6solo (tending to green).
 
  Even the U-boot HDMI splash screen is not very steady on mx6solo.
 
  I will try to look at these mx6 solo HDMI issues.
 
 With the change below I am able to get a good HDMI quality image on
 mx6solo (and mx6quad as well):

Yes, I need that too on the Carrier-1 board, as suggested by Sascha.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v3] imx-drm: Add mx6 hdmi transmitter support

2013-10-29 Thread Russell King - ARM Linux
On Tue, Oct 29, 2013 at 08:01:46PM -0200, Fabio Estevam wrote:
 Hi Greg,
 
 On Mon, Oct 28, 2013 at 7:03 PM, Greg KH gre...@linuxfoundation.org wrote:
  And why is new support being added to this driver instead of working to
  get it out of staging?
 
 Philipp Zabel mentioned that he will submit the imx-drm driver out of staging:
 http://www.spinics.net/lists/dri-devel/msg46784.html

The issues surrounding DT and DRM were given an airing last week at
kernel summit in a session chaired by David Airlie.

The Kernel Summit session was to discuss issues surrounding DT and DRM,
or more specifically DT and componentised subsystems like DRM, ASoC and
V4L2.  Grant Likely has said that he will attempt to summarise some of
the discussion there; unfortunately I'm not aware of anyone making any
real minutes of what was discussed - it wasn't a formal session.

I also had discussions with David Airlie and, separately, with the
Pengutronix folk about imx-drm.

It is very clear to me that David is against imx-drm moving into
drivers/gpu/drm - the driver requires an amount of rework in various
places.  So I don't think it will move out of drivers/staging any
time soon.

I have a few other bits to sort out first, but I've volunteered to try
to resolve some of these blocking issues so that imx-drm can move
forward and hopefully out of staging.

To put it another way, moving this driver out of staging is not as
simple as moving the files; it requires real work to address the
issues the subsystem maintainer has with this code.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH v3] imx-drm: Add mx6 hdmi transmitter support

2013-10-28 Thread Russell King - ARM Linux
On Mon, Oct 28, 2013 at 06:26:49PM -0200, Fabio Estevam wrote:
 From: Fabio Estevam fabio.este...@freescale.com
 
 This is based on the initial work done by Sascha Hauer and Tony Prisk.

It looks like you've also taken some of the suggestions I've made too -
like the voltage drive and symbol transmit control settings.

It would be nice to identify the changes a bit better so that those of
us who are testing your drivers (and have patches on top of them to make
things work) know what's changed.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel