diff mbox

[04/12] gpu: imx: fix support for interlaced modes

Message ID E1ZO6ay-0002qh-6E@rmk-PC.arm.linux.org.uk (mailing list archive)
State New, archived
Headers show

Commit Message

Russell King Aug. 8, 2015, 4:03 p.m. UTC
The support for interlaced video modes seems to be broken; we don't use
anything other than the vtotal/htotal from the timing information to
define the various sync counters.

Freescale patches for interlaced video support contain an alternative
sync counter setup, which we include here.  This setup produces the
hsync and vsync via the normal counter 2 and 3, but moves the display
enable signal from counter 5 to counter 6.  Therefore, we need to
change the display controller setup as well.

The corresponding Freescale patches for this change are:
  iMX6-HDMI-support-interlaced-display-mode.patch
  IPU-fine-tuning-the-interlace-display-timing-for-CEA.patch

This produces a working interlace format output from the IPU.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/gpu/ipu-v3/ipu-dc.c | 18 ++++++++---
 drivers/gpu/ipu-v3/ipu-di.c | 79 +++++++++++++++++++++------------------------
 2 files changed, 51 insertions(+), 46 deletions(-)

Comments

Philipp Zabel Aug. 27, 2015, 8:39 a.m. UTC | #1
Hi Russell,

Am Samstag, den 08.08.2015, 17:03 +0100 schrieb Russell King:
> The support for interlaced video modes seems to be broken; we don't use
> anything other than the vtotal/htotal from the timing information to
> define the various sync counters.

I finally made time to test this series:

Tested-by: Philipp Zabel <p.zabel@pengutronix.de>
on i.MX6 GK802 via HDMI connected to a TV (1080p60, 1080i60).

Unfortunately these timings are completely different from what Freescale
came up with for the TV Encoder on i.MX5, but the code we have currently
in mainline doesn't work for that either. I suppose I'll follow up with
a patch that adds yet another sync counter setup for the i.MX5/TVE case.

I'd like to take the two ipu-v3 patches, making a few small changes on
this one:

> Freescale patches for interlaced video support contain an alternative
> sync counter setup, which we include here.  This setup produces the
> hsync and vsync via the normal counter 2 and 3, but moves the display
> enable signal from counter 5 to counter 6.  Therefore, we need to
> change the display controller setup as well.
> 
> The corresponding Freescale patches for this change are:
>   iMX6-HDMI-support-interlaced-display-mode.patch
>   IPU-fine-tuning-the-interlace-display-timing-for-CEA.patch
> 
> This produces a working interlace format output from the IPU.

... on i.MX6 via HDMI.

> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
>  drivers/gpu/ipu-v3/ipu-dc.c | 18 ++++++++---
>  drivers/gpu/ipu-v3/ipu-di.c | 79 +++++++++++++++++++++------------------------
>  2 files changed, 51 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/gpu/ipu-v3/ipu-dc.c b/drivers/gpu/ipu-v3/ipu-dc.c
> index 9ef2e1f54ca4..aa560855c1dc 100644
> --- a/drivers/gpu/ipu-v3/ipu-dc.c
> +++ b/drivers/gpu/ipu-v3/ipu-dc.c
> @@ -183,12 +183,22 @@ int ipu_dc_init_sync(struct ipu_dc *dc, struct ipu_di *di, bool interlaced,
>  	}
>  
>  	if (interlaced) {
> -		dc_link_event(dc, DC_EVT_NL, 0, 3);
> -		dc_link_event(dc, DC_EVT_EOL, 0, 2);
> -		dc_link_event(dc, DC_EVT_NEW_DATA, 0, 1);
> +		int word, addr;
> +
> +		if (dc->di) {
> +			addr = 1;
> +			word = 1;

These two are really one and the same. The address written to the link
register for the given event has to point to the address of the
microcode instruction written to the template memory that should be
executed on this event.

I'd like to drop the word variable and use addr for both.

> +		} else {
> +			addr = 0;
> +			word = 0;
> +		}
> +
> +		dc_link_event(dc, DC_EVT_NL, addr, 3);
> +		dc_link_event(dc, DC_EVT_EOL, addr, 2);
> +		dc_link_event(dc, DC_EVT_NEW_DATA, addr, 1);
>  
>  		/* Init template microcode */
> -		dc_write_tmpl(dc, 0, WROD(0), 0, map, SYNC_WAVE, 0, 8, 1);
> +		dc_write_tmpl(dc, word, WROD(0), 0, map, SYNC_WAVE, 0, 6, 1);
>  	} else {
>  		if (dc->di) {
>  			dc_link_event(dc, DC_EVT_NL, 2, 3);
> diff --git a/drivers/gpu/ipu-v3/ipu-di.c b/drivers/gpu/ipu-v3/ipu-di.c
> index a96991c5c15f..359268e3a166 100644
> --- a/drivers/gpu/ipu-v3/ipu-di.c
> +++ b/drivers/gpu/ipu-v3/ipu-di.c
> @@ -71,6 +71,10 @@ enum di_sync_wave {
>  	DI_SYNC_HSYNC = 3,
>  	DI_SYNC_VSYNC = 4,
>  	DI_SYNC_DE = 6,
> +
> +	DI_SYNC_CNT1 = 2,	/* counter >= 2 only */
> +	DI_SYNC_CNT4 = 5,	/* counter >= 5 only */
> +	DI_SYNC_CNT5 = 6,	/* counter >= 6 only */
>  };
>  
>  #define SYNC_WAVE 0
> @@ -211,66 +215,59 @@ static void ipu_di_sync_config_interlaced(struct ipu_di *di,
>  		sig->mode.hback_porch + sig->mode.hfront_porch;
>  	u32 v_total = sig->mode.vactive + sig->mode.vsync_len +
>  		sig->mode.vback_porch + sig->mode.vfront_porch;
> -	u32 reg;
>  	struct di_sync_config cfg[] = {
>  		{
> -			.run_count = h_total / 2 - 1,
> -			.run_src = DI_SYNC_CLK,
> +			/* 1: internal VSYNC for each frame */
> +			.run_count = v_total * 2 - 1,
> +			.run_src = 3,			/* == counter 7 */

Do you know why this works? The Reference Manual v2 lists that value as
"NA" in the DI counter #1 Run Resolution field.

>  		}, {
> -			.run_count = h_total - 11,
> +			/* PIN2: HSYNC waveform */
> +			.run_count = h_total - 1,
>  			.run_src = DI_SYNC_CLK,
> -			.cnt_down = 4,
> +			.cnt_polarity_gen_en = 1,
> +			.cnt_polarity_trigger_src = DI_SYNC_CLK,
> +			.cnt_down = sig->mode.hsync_len * 2,
>  		}, {
> -			.run_count = v_total * 2 - 1,
> -			.run_src = DI_SYNC_INT_HSYNC,
> -			.offset_count = 1,
> -			.offset_src = DI_SYNC_INT_HSYNC,
> -			.cnt_down = 4,
> +			/* PIN3: VSYNC waveform */
> +			.run_count = v_total - 1,
> +			.run_src = 4,			/* == counter 7 */

Same here, ...

> +			.cnt_polarity_gen_en = 1,
> +			.cnt_polarity_trigger_src = 4,	/* == counter 7 */

... and same here, the DI counter #3 polarity Clear select field lists
the value 4 as "Reserved".

> +			.cnt_down = sig->mode.vsync_len * 2,
> +			.cnt_clr_src = DI_SYNC_CNT1,
>  		}, {
[...]
>  		}
>  	};
>  
>  	ipu_di_sync_config(di, cfg, 0, ARRAY_SIZE(cfg));
>  
> -	/* set gentime select and tag sel */
> -	reg = ipu_di_read(di, DI_SW_GEN1(9));
> -	reg &= 0x1FFFFFFF;
> -	reg |= (3 - 1) << 29 | 0x00008000;
> -	ipu_di_write(di, reg, DI_SW_GEN1(9));

As far as I understood, attaching counter #9 to counter #3 is needed to
generate the second vsync on i.MX5. Since this doesn't currently work,
I'm fine with removing it for now.

>  	ipu_di_write(di, v_total / 2 - 1, DI_SCR_CONF);
>  }
>  
> @@ -605,10 +602,8 @@ int ipu_di_init_sync_panel(struct ipu_di *di, struct ipu_di_signal_cfg *sig)
>  
>  		/* set y_sel = 1 */
>  		di_gen |= 0x10000000;
> -		di_gen |= DI_GEN_POLARITY_5;
> -		di_gen |= DI_GEN_POLARITY_8;
>  
> -		vsync_cnt = 7;
> +		vsync_cnt = 3;
>  	} else {
>  		ipu_di_sync_config_noninterlaced(di, sig, div);

regards
Philipp
Russell King - ARM Linux Aug. 27, 2015, 8:54 a.m. UTC | #2
On Thu, Aug 27, 2015 at 10:39:12AM +0200, Philipp Zabel wrote:
> Hi Russell,
> 
> Am Samstag, den 08.08.2015, 17:03 +0100 schrieb Russell King:
> > The support for interlaced video modes seems to be broken; we don't use
> > anything other than the vtotal/htotal from the timing information to
> > define the various sync counters.
> 
> I finally made time to test this series:
> 
> Tested-by: Philipp Zabel <p.zabel@pengutronix.de>
> on i.MX6 GK802 via HDMI connected to a TV (1080p60, 1080i60).
> 
> Unfortunately these timings are completely different from what Freescale
> came up with for the TV Encoder on i.MX5, but the code we have currently
> in mainline doesn't work for that either. I suppose I'll follow up with
> a patch that adds yet another sync counter setup for the i.MX5/TVE case.
> 
> I'd like to take the two ipu-v3 patches, making a few small changes on
> this one:

Please don't split the series up.  The reason it's a series is because
there's interdependencies between the patches.

> > Freescale patches for interlaced video support contain an alternative
> > sync counter setup, which we include here.  This setup produces the
> > hsync and vsync via the normal counter 2 and 3, but moves the display
> > enable signal from counter 5 to counter 6.  Therefore, we need to
> > change the display controller setup as well.
> > 
> > The corresponding Freescale patches for this change are:
> >   iMX6-HDMI-support-interlaced-display-mode.patch
> >   IPU-fine-tuning-the-interlace-display-timing-for-CEA.patch
> > 
> > This produces a working interlace format output from the IPU.
> 
> ... on i.MX6 via HDMI.

It should also be correct for any other source which wants interlaced
output.

> > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > ---
> >  drivers/gpu/ipu-v3/ipu-dc.c | 18 ++++++++---
> >  drivers/gpu/ipu-v3/ipu-di.c | 79 +++++++++++++++++++++------------------------
> >  2 files changed, 51 insertions(+), 46 deletions(-)
> > 
> > diff --git a/drivers/gpu/ipu-v3/ipu-dc.c b/drivers/gpu/ipu-v3/ipu-dc.c
> > index 9ef2e1f54ca4..aa560855c1dc 100644
> > --- a/drivers/gpu/ipu-v3/ipu-dc.c
> > +++ b/drivers/gpu/ipu-v3/ipu-dc.c
> > @@ -183,12 +183,22 @@ int ipu_dc_init_sync(struct ipu_dc *dc, struct ipu_di *di, bool interlaced,
> >  	}
> >  
> >  	if (interlaced) {
> > -		dc_link_event(dc, DC_EVT_NL, 0, 3);
> > -		dc_link_event(dc, DC_EVT_EOL, 0, 2);
> > -		dc_link_event(dc, DC_EVT_NEW_DATA, 0, 1);
> > +		int word, addr;
> > +
> > +		if (dc->di) {
> > +			addr = 1;
> > +			word = 1;
> 
> These two are really one and the same. The address written to the link
> register for the given event has to point to the address of the
> microcode instruction written to the template memory that should be
> executed on this event.
> 
> I'd like to drop the word variable and use addr for both.

Ok.  As I said in the commit message, this code comes from Freescale's
patches which I pointed to above.

> > +		} else {
> > +			addr = 0;
> > +			word = 0;
> > +		}
> > +
> > +		dc_link_event(dc, DC_EVT_NL, addr, 3);
> > +		dc_link_event(dc, DC_EVT_EOL, addr, 2);
> > +		dc_link_event(dc, DC_EVT_NEW_DATA, addr, 1);
> >  
> >  		/* Init template microcode */
> > -		dc_write_tmpl(dc, 0, WROD(0), 0, map, SYNC_WAVE, 0, 8, 1);
> > +		dc_write_tmpl(dc, word, WROD(0), 0, map, SYNC_WAVE, 0, 6, 1);
> >  	} else {
> >  		if (dc->di) {
> >  			dc_link_event(dc, DC_EVT_NL, 2, 3);
> > diff --git a/drivers/gpu/ipu-v3/ipu-di.c b/drivers/gpu/ipu-v3/ipu-di.c
> > index a96991c5c15f..359268e3a166 100644
> > --- a/drivers/gpu/ipu-v3/ipu-di.c
> > +++ b/drivers/gpu/ipu-v3/ipu-di.c
> > @@ -71,6 +71,10 @@ enum di_sync_wave {
> >  	DI_SYNC_HSYNC = 3,
> >  	DI_SYNC_VSYNC = 4,
> >  	DI_SYNC_DE = 6,
> > +
> > +	DI_SYNC_CNT1 = 2,	/* counter >= 2 only */
> > +	DI_SYNC_CNT4 = 5,	/* counter >= 5 only */
> > +	DI_SYNC_CNT5 = 6,	/* counter >= 6 only */
> >  };
> >  
> >  #define SYNC_WAVE 0
> > @@ -211,66 +215,59 @@ static void ipu_di_sync_config_interlaced(struct ipu_di *di,
> >  		sig->mode.hback_porch + sig->mode.hfront_porch;
> >  	u32 v_total = sig->mode.vactive + sig->mode.vsync_len +
> >  		sig->mode.vback_porch + sig->mode.vfront_porch;
> > -	u32 reg;
> >  	struct di_sync_config cfg[] = {
> >  		{
> > -			.run_count = h_total / 2 - 1,
> > -			.run_src = DI_SYNC_CLK,
> > +			/* 1: internal VSYNC for each frame */
> > +			.run_count = v_total * 2 - 1,
> > +			.run_src = 3,			/* == counter 7 */
> 
> Do you know why this works? The Reference Manual v2 lists that value as
> "NA" in the DI counter #1 Run Resolution field.

Yes, I noticed that Freescale were using values for the source fields
which were marked as NA in the manual.  As it works, I can only assume
that the engineer who came up with this setup has more knowledge than
is public.

> >  		}, {
> > -			.run_count = h_total - 11,
> > +			/* PIN2: HSYNC waveform */
> > +			.run_count = h_total - 1,
> >  			.run_src = DI_SYNC_CLK,
> > -			.cnt_down = 4,
> > +			.cnt_polarity_gen_en = 1,
> > +			.cnt_polarity_trigger_src = DI_SYNC_CLK,
> > +			.cnt_down = sig->mode.hsync_len * 2,
> >  		}, {
> > -			.run_count = v_total * 2 - 1,
> > -			.run_src = DI_SYNC_INT_HSYNC,
> > -			.offset_count = 1,
> > -			.offset_src = DI_SYNC_INT_HSYNC,
> > -			.cnt_down = 4,
> > +			/* PIN3: VSYNC waveform */
> > +			.run_count = v_total - 1,
> > +			.run_src = 4,			/* == counter 7 */
> 
> Same here, ...
> 
> > +			.cnt_polarity_gen_en = 1,
> > +			.cnt_polarity_trigger_src = 4,	/* == counter 7 */
> 
> ... and same here, the DI counter #3 polarity Clear select field lists
> the value 4 as "Reserved".
> 
> > +			.cnt_down = sig->mode.vsync_len * 2,
> > +			.cnt_clr_src = DI_SYNC_CNT1,
> >  		}, {
> [...]
> >  		}
> >  	};
> >  
> >  	ipu_di_sync_config(di, cfg, 0, ARRAY_SIZE(cfg));
> >  
> > -	/* set gentime select and tag sel */
> > -	reg = ipu_di_read(di, DI_SW_GEN1(9));
> > -	reg &= 0x1FFFFFFF;
> > -	reg |= (3 - 1) << 29 | 0x00008000;
> > -	ipu_di_write(di, reg, DI_SW_GEN1(9));
> 
> As far as I understood, attaching counter #9 to counter #3 is needed to
> generate the second vsync on i.MX5. Since this doesn't currently work,
> I'm fine with removing it for now.

I went through the counter setup to understand how it works, and it
seems correct provided you accept that the various source values do
work as the code claims, which includes creating the vsync at the
appropriate half-scanline position without needing this hack.

It's not easy to work back from the counter setup to get a mental
picture of what's going on, but it is possible to do so.
Philipp Zabel Aug. 27, 2015, 9:40 a.m. UTC | #3
Am Donnerstag, den 27.08.2015, 09:54 +0100 schrieb Russell King - ARM
Linux:
> On Thu, Aug 27, 2015 at 10:39:12AM +0200, Philipp Zabel wrote:
> > Hi Russell,
> > 
> > Am Samstag, den 08.08.2015, 17:03 +0100 schrieb Russell King:
> > > The support for interlaced video modes seems to be broken; we don't use
> > > anything other than the vtotal/htotal from the timing information to
> > > define the various sync counters.
> > 
> > I finally made time to test this series:
> > 
> > Tested-by: Philipp Zabel <p.zabel@pengutronix.de>
> > on i.MX6 GK802 via HDMI connected to a TV (1080p60, 1080i60).
> > 
> > Unfortunately these timings are completely different from what Freescale
> > came up with for the TV Encoder on i.MX5, but the code we have currently
> > in mainline doesn't work for that either. I suppose I'll follow up with
> > a patch that adds yet another sync counter setup for the i.MX5/TVE case.
> > 
> > I'd like to take the two ipu-v3 patches, making a few small changes on
> > this one:
> 
> Please don't split the series up.  The reason it's a series is because
> there's interdependencies between the patches.

In that case, can I take the whole series? Or would you like to respin
and have my Acked-by: Philipp Zabel <p.zabel@pengutronix.de> with the
changes below:

> > > Freescale patches for interlaced video support contain an alternative
> > > sync counter setup, which we include here.  This setup produces the
> > > hsync and vsync via the normal counter 2 and 3, but moves the display
> > > enable signal from counter 5 to counter 6.  Therefore, we need to
> > > change the display controller setup as well.
> > > 
> > > The corresponding Freescale patches for this change are:
> > >   iMX6-HDMI-support-interlaced-display-mode.patch
> > >   IPU-fine-tuning-the-interlace-display-timing-for-CEA.patch
> > > 
> > > This produces a working interlace format output from the IPU.
> > 
> > ... on i.MX6 via HDMI.
> 
> It should also be correct for any other source which wants interlaced
> output.

... on i.MX6, then. Right now I don't know what is the effect of the
undocumented settings on the i.MX5's IPUv3M.

> > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > > ---
> > >  drivers/gpu/ipu-v3/ipu-dc.c | 18 ++++++++---
> > >  drivers/gpu/ipu-v3/ipu-di.c | 79 +++++++++++++++++++++------------------------
> > >  2 files changed, 51 insertions(+), 46 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/ipu-v3/ipu-dc.c b/drivers/gpu/ipu-v3/ipu-dc.c
> > > index 9ef2e1f54ca4..aa560855c1dc 100644
> > > --- a/drivers/gpu/ipu-v3/ipu-dc.c
> > > +++ b/drivers/gpu/ipu-v3/ipu-dc.c
> > > @@ -183,12 +183,22 @@ int ipu_dc_init_sync(struct ipu_dc *dc, struct ipu_di *di, bool interlaced,
> > >  	}
> > >  
> > >  	if (interlaced) {
> > > -		dc_link_event(dc, DC_EVT_NL, 0, 3);
> > > -		dc_link_event(dc, DC_EVT_EOL, 0, 2);
> > > -		dc_link_event(dc, DC_EVT_NEW_DATA, 0, 1);
> > > +		int word, addr;
> > > +
> > > +		if (dc->di) {
> > > +			addr = 1;
> > > +			word = 1;
> > 
> > These two are really one and the same. The address written to the link
> > register for the given event has to point to the address of the
> > microcode instruction written to the template memory that should be
> > executed on this event.
> > 
> > I'd like to drop the word variable and use addr for both.
> 
> Ok.  As I said in the commit message, this code comes from Freescale's
> patches which I pointed to above.
[...]
> > > @@ -211,66 +215,59 @@ static void ipu_di_sync_config_interlaced(struct ipu_di *di,
> > >  		sig->mode.hback_porch + sig->mode.hfront_porch;
> > >  	u32 v_total = sig->mode.vactive + sig->mode.vsync_len +
> > >  		sig->mode.vback_porch + sig->mode.vfront_porch;
> > > -	u32 reg;
> > >  	struct di_sync_config cfg[] = {
> > >  		{
> > > -			.run_count = h_total / 2 - 1,
> > > -			.run_src = DI_SYNC_CLK,
> > > +			/* 1: internal VSYNC for each frame */
> > > +			.run_count = v_total * 2 - 1,
> > > +			.run_src = 3,			/* == counter 7 */
> > 
> > Do you know why this works? The Reference Manual v2 lists that value as
> > "NA" in the DI counter #1 Run Resolution field.
> 
> Yes, I noticed that Freescale were using values for the source fields
> which were marked as NA in the manual.  As it works, I can only assume
> that the engineer who came up with this setup has more knowledge than
> is public.

I'd like small a comment here that yes, we know this is marked as
NA/Reserved in the manuals.

[...]
> I went through the counter setup to understand how it works, and it
> seems correct provided you accept that the various source values do
> work as the code claims, which includes creating the vsync at the
> appropriate half-scanline position without needing this hack.
> 
> It's not easy to work back from the counter setup to get a mental
> picture of what's going on, but it is possible to do so.

Yes, being able to actually reference counters with higher numbers makes
things a lot easier to follow.

regards
Philipp
Fabio Estevam Oct. 6, 2015, 5:59 p.m. UTC | #4
On Sat, Aug 8, 2015 at 1:03 PM, Russell King
<rmk+kernel@arm.linux.org.uk> wrote:
> The support for interlaced video modes seems to be broken; we don't use
> anything other than the vtotal/htotal from the timing information to
> define the various sync counters.
>
> Freescale patches for interlaced video support contain an alternative
> sync counter setup, which we include here.  This setup produces the
> hsync and vsync via the normal counter 2 and 3, but moves the display
> enable signal from counter 5 to counter 6.  Therefore, we need to
> change the display controller setup as well.
>
> The corresponding Freescale patches for this change are:
>   iMX6-HDMI-support-interlaced-display-mode.patch
>   IPU-fine-tuning-the-interlace-display-timing-for-CEA.patch
>
> This produces a working interlace format output from the IPU.
>
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>

Reviewed-by: Fabio Estevam <fabio.estevam@freescale.com>
diff mbox

Patch

diff --git a/drivers/gpu/ipu-v3/ipu-dc.c b/drivers/gpu/ipu-v3/ipu-dc.c
index 9ef2e1f54ca4..aa560855c1dc 100644
--- a/drivers/gpu/ipu-v3/ipu-dc.c
+++ b/drivers/gpu/ipu-v3/ipu-dc.c
@@ -183,12 +183,22 @@  int ipu_dc_init_sync(struct ipu_dc *dc, struct ipu_di *di, bool interlaced,
 	}
 
 	if (interlaced) {
-		dc_link_event(dc, DC_EVT_NL, 0, 3);
-		dc_link_event(dc, DC_EVT_EOL, 0, 2);
-		dc_link_event(dc, DC_EVT_NEW_DATA, 0, 1);
+		int word, addr;
+
+		if (dc->di) {
+			addr = 1;
+			word = 1;
+		} else {
+			addr = 0;
+			word = 0;
+		}
+
+		dc_link_event(dc, DC_EVT_NL, addr, 3);
+		dc_link_event(dc, DC_EVT_EOL, addr, 2);
+		dc_link_event(dc, DC_EVT_NEW_DATA, addr, 1);
 
 		/* Init template microcode */
-		dc_write_tmpl(dc, 0, WROD(0), 0, map, SYNC_WAVE, 0, 8, 1);
+		dc_write_tmpl(dc, word, WROD(0), 0, map, SYNC_WAVE, 0, 6, 1);
 	} else {
 		if (dc->di) {
 			dc_link_event(dc, DC_EVT_NL, 2, 3);
diff --git a/drivers/gpu/ipu-v3/ipu-di.c b/drivers/gpu/ipu-v3/ipu-di.c
index a96991c5c15f..359268e3a166 100644
--- a/drivers/gpu/ipu-v3/ipu-di.c
+++ b/drivers/gpu/ipu-v3/ipu-di.c
@@ -71,6 +71,10 @@  enum di_sync_wave {
 	DI_SYNC_HSYNC = 3,
 	DI_SYNC_VSYNC = 4,
 	DI_SYNC_DE = 6,
+
+	DI_SYNC_CNT1 = 2,	/* counter >= 2 only */
+	DI_SYNC_CNT4 = 5,	/* counter >= 5 only */
+	DI_SYNC_CNT5 = 6,	/* counter >= 6 only */
 };
 
 #define SYNC_WAVE 0
@@ -211,66 +215,59 @@  static void ipu_di_sync_config_interlaced(struct ipu_di *di,
 		sig->mode.hback_porch + sig->mode.hfront_porch;
 	u32 v_total = sig->mode.vactive + sig->mode.vsync_len +
 		sig->mode.vback_porch + sig->mode.vfront_porch;
-	u32 reg;
 	struct di_sync_config cfg[] = {
 		{
-			.run_count = h_total / 2 - 1,
-			.run_src = DI_SYNC_CLK,
+			/* 1: internal VSYNC for each frame */
+			.run_count = v_total * 2 - 1,
+			.run_src = 3,			/* == counter 7 */
 		}, {
-			.run_count = h_total - 11,
+			/* PIN2: HSYNC waveform */
+			.run_count = h_total - 1,
 			.run_src = DI_SYNC_CLK,
-			.cnt_down = 4,
+			.cnt_polarity_gen_en = 1,
+			.cnt_polarity_trigger_src = DI_SYNC_CLK,
+			.cnt_down = sig->mode.hsync_len * 2,
 		}, {
-			.run_count = v_total * 2 - 1,
-			.run_src = DI_SYNC_INT_HSYNC,
-			.offset_count = 1,
-			.offset_src = DI_SYNC_INT_HSYNC,
-			.cnt_down = 4,
+			/* PIN3: VSYNC waveform */
+			.run_count = v_total - 1,
+			.run_src = 4,			/* == counter 7 */
+			.cnt_polarity_gen_en = 1,
+			.cnt_polarity_trigger_src = 4,	/* == counter 7 */
+			.cnt_down = sig->mode.vsync_len * 2,
+			.cnt_clr_src = DI_SYNC_CNT1,
 		}, {
-			.run_count = v_total / 2 - 1,
+			/* 4: Field */
+			.run_count = v_total / 2,
 			.run_src = DI_SYNC_HSYNC,
-			.offset_count = sig->mode.vback_porch,
-			.offset_src = DI_SYNC_HSYNC,
+			.offset_count = h_total / 2,
+			.offset_src = DI_SYNC_CLK,
 			.repeat_count = 2,
-			.cnt_clr_src = DI_SYNC_VSYNC,
+			.cnt_clr_src = DI_SYNC_CNT1,
 		}, {
+			/* 5: Active lines */
 			.run_src = DI_SYNC_HSYNC,
-			.repeat_count = sig->mode.vactive / 2,
-			.cnt_clr_src = 4,
-		}, {
-			.run_count = v_total - 1,
-			.run_src = DI_SYNC_HSYNC,
-		}, {
-			.run_count = v_total / 2 - 1,
-			.run_src = DI_SYNC_HSYNC,
-			.offset_count = 9,
+			.offset_count = (sig->mode.vsync_len +
+					 sig->mode.vback_porch) / 2,
 			.offset_src = DI_SYNC_HSYNC,
-			.repeat_count = 2,
-			.cnt_clr_src = DI_SYNC_VSYNC,
+			.repeat_count = sig->mode.vactive / 2,
+			.cnt_clr_src = DI_SYNC_CNT4,
 		}, {
+			/* 6: Active pixel, referenced by DC */
 			.run_src = DI_SYNC_CLK,
-			.offset_count = sig->mode.hback_porch,
+			.offset_count = sig->mode.hsync_len +
+					sig->mode.hback_porch,
 			.offset_src = DI_SYNC_CLK,
 			.repeat_count = sig->mode.hactive,
-			.cnt_clr_src = 5,
+			.cnt_clr_src = DI_SYNC_CNT5,
 		}, {
-			.run_count = v_total - 1,
-			.run_src = DI_SYNC_INT_HSYNC,
-			.offset_count = v_total / 2,
-			.offset_src = DI_SYNC_INT_HSYNC,
-			.cnt_clr_src = DI_SYNC_HSYNC,
-			.cnt_down = 4,
+			/* 7: Half line HSYNC */
+			.run_count = h_total / 2 - 1,
+			.run_src = DI_SYNC_CLK,
 		}
 	};
 
 	ipu_di_sync_config(di, cfg, 0, ARRAY_SIZE(cfg));
 
-	/* set gentime select and tag sel */
-	reg = ipu_di_read(di, DI_SW_GEN1(9));
-	reg &= 0x1FFFFFFF;
-	reg |= (3 - 1) << 29 | 0x00008000;
-	ipu_di_write(di, reg, DI_SW_GEN1(9));
-
 	ipu_di_write(di, v_total / 2 - 1, DI_SCR_CONF);
 }
 
@@ -605,10 +602,8 @@  int ipu_di_init_sync_panel(struct ipu_di *di, struct ipu_di_signal_cfg *sig)
 
 		/* set y_sel = 1 */
 		di_gen |= 0x10000000;
-		di_gen |= DI_GEN_POLARITY_5;
-		di_gen |= DI_GEN_POLARITY_8;
 
-		vsync_cnt = 7;
+		vsync_cnt = 3;
 	} else {
 		ipu_di_sync_config_noninterlaced(di, sig, div);