[PATCH 0/2] drm/tilcdc drm/i2c/tda998x workaround for sync issues on TI SoC

[PATCH 0/2] drm/tilcdc drm/i2c/tda998x workaround for sync issues on TI SoC

[Darren Etheridge]

Russell King and Sebastian Hasselbarth had proposed some very good changes
for the tda998x HDMI encoder driver.  But when those changes were tested
on BeagleBone Black against the tilcdc driver many modes would no longer
display correctly.  After analyzing the sync signals from the TI lcd contoller
to the nxp it is apparent that the hsync/vsync's are not rising at the same 
time as per the VESA spec and this is causing the HDMI encoder to get 
messed up and failing to lock correctly.

This series of patches should be applied on top of:

Russell King's
rmk's Dove DRM/TDA19988 Cubox driver series

Sebastian Hasselbarth's
drm/i2c: tda998x: fix sync generation and calculation

I have done as much of the change as I can in the tilcdc driver but there 
is a small unavoidable change in the tda998x driver.  However I have been 
careful not to break anything from the Dove drivers perspective. It 
would be great if somebody can test on Cubox and confirm that.

This patch set inverts the hsync signal coming from the tilcdc so the NXP 
is kept happy and then shifts the output to the right to compensate for the 
sync timing issues.  Display modes from the NXP have been verified using a
HDMI analyzer and are reporting correct timings at the output stage.

Hopefully this will allow the dove/tda driver changes to progress now that
were blocked as per this discussion:
http://lists.freedesktop.org/archives/dri-devel/2013-July/040900.html


Darren Etheridge (2):
  drm/i2c/tda998x prepare for tilcdc sync workaround
  drm/tilcdc fixup mode to workaound sync for tda998x

 drivers/gpu/drm/i2c/tda998x_drv.c     |   10 ++++++++++
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c  |    7 ++++++-
 drivers/gpu/drm/tilcdc/tilcdc_slave.c |   21 ++++++++++++++++++++-
 3 files changed, 36 insertions(+), 2 deletions(-)

[PATCH 1/2] drm/i2c/tda998x prepare for tilcdc sync workaround

[Darren Etheridge]

Add necessary support for flipping the hsync signal and shifting
the display to the right by a certain number of pixels.  Changes
only take effect if adjusted_mode is changed in fixup function.

Signed-off-by: Darren Etheridge <detheridge@ti.com>
---
 drivers/gpu/drm/i2c/tda998x_drv.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index e384b59..ad870e6 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -778,6 +778,16 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 	n_line       = mode->vtotal;
 
 	ref_pix      = 3 + mode->hsync_start - mode->hdisplay;
+
+	/*
+	 * handle issue on TILCDC where it is outputing
+	 * non-VESA compliant sync signals the workaround
+	 * forces us to invert the HSYNC, so need to adjust display to
+	 * the left by hskew pixels, provided by the tilcdc driver
+	 */
+	if (adjusted_mode->flags & DRM_MODE_FLAG_HSKEW)
+		ref_pix += adjusted_mode->hskew;
+
 	de_pix_s     = mode->htotal - mode->hdisplay;
 	de_pix_e     = de_pix_s + mode->hdisplay;
 	hs_pix_s     = mode->hsync_start - mode->hdisplay;
-- 
1.7.0.4

[PATCH 2/2] drm/tilcdc fixup mode to workaound sync for tda998x

[Darren Etheridge]

Add a fixup function that will flip the hsync priority and
add a hskew value that is used to shift the tda998x to the
right by a variable number of pixels depending on the mode.
This works around an issue with the sync timings that tilcdc
is outputing.

Signed-off-by: Darren Etheridge <detheridge@ti.com>
---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c  |    7 ++++++-
 drivers/gpu/drm/tilcdc/tilcdc_slave.c |   21 ++++++++++++++++++++-
 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index 6118651..1c10bf1 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -377,7 +377,12 @@ static int tilcdc_crtc_mode_set(struct drm_crtc *crtc,
 	else
 		tilcdc_clear(dev, LCDC_RASTER_TIMING_2_REG, LCDC_SYNC_EDGE);
 
-	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
+	/*
+	 * use value from adjusted_mode here as this might have been
+	 * changed as part of the fixup for NXP TDA998x to solve the
+	 * issue where tilcdc timings are not VESA compliant
+	 */
+	if (adjusted_mode->flags & DRM_MODE_FLAG_NHSYNC)
 		tilcdc_set(dev, LCDC_RASTER_TIMING_2_REG, LCDC_INVERT_HSYNC);
 	else
 		tilcdc_clear(dev, LCDC_RASTER_TIMING_2_REG, LCDC_INVERT_HSYNC);
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave.c b/drivers/gpu/drm/tilcdc/tilcdc_slave.c
index dfffaf0..d5197a3 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_slave.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_slave.c
@@ -73,13 +73,32 @@ static void slave_encoder_prepare(struct drm_encoder *encoder)
 	tilcdc_crtc_set_panel_info(encoder->crtc, &slave_info);
 }
 
+static bool slave_encoder_fixup(struct drm_encoder *encoder,
+		const struct drm_display_mode *mode,
+		struct drm_display_mode *adjusted_mode)
+{
+	adjusted_mode->hskew = mode->hsync_end - mode->hsync_start;
+	adjusted_mode->flags |= DRM_MODE_FLAG_HSKEW;
+
+	if (mode->flags & DRM_MODE_FLAG_NHSYNC) {
+		adjusted_mode->flags |= DRM_MODE_FLAG_PHSYNC;
+		adjusted_mode->flags &= ~DRM_MODE_FLAG_NHSYNC;
+	} else {
+		adjusted_mode->flags |= DRM_MODE_FLAG_NHSYNC;
+		adjusted_mode->flags &= ~DRM_MODE_FLAG_PHSYNC;
+	}
+
+	return drm_i2c_encoder_mode_fixup(encoder, mode, adjusted_mode);
+}
+
+
 static const struct drm_encoder_funcs slave_encoder_funcs = {
 		.destroy        = slave_encoder_destroy,
 };
 
 static const struct drm_encoder_helper_funcs slave_encoder_helper_funcs = {
 		.dpms           = drm_i2c_encoder_dpms,
-		.mode_fixup     = drm_i2c_encoder_mode_fixup,
+		.mode_fixup     = slave_encoder_fixup,
 		.prepare        = slave_encoder_prepare,
 		.commit         = drm_i2c_encoder_commit,
 		.mode_set       = drm_i2c_encoder_mode_set,
-- 
1.7.0.4

Re: [PATCH 0/2] drm/tilcdc drm/i2c/tda998x workaround for sync issues on TI SoC

[Rob Clark]

On Thu, Jul 25, 2013 at 2:32 PM, Darren Etheridge <detheridge@ti.com> wrote:
> Russell King and Sebastian Hasselbarth had proposed some very good changes
> for the tda998x HDMI encoder driver.  But when those changes were tested
> on BeagleBone Black against the tilcdc driver many modes would no longer
> display correctly.  After analyzing the sync signals from the TI lcd contoller
> to the nxp it is apparent that the hsync/vsync's are not rising at the same
> time as per the VESA spec and this is causing the HDMI encoder to get
> messed up and failing to lock correctly.
>
> This series of patches should be applied on top of:
>
> Russell King's
> rmk's Dove DRM/TDA19988 Cubox driver series
>
> Sebastian Hasselbarth's
> drm/i2c: tda998x: fix sync generation and calculation
>
> I have done as much of the change as I can in the tilcdc driver but there
> is a small unavoidable change in the tda998x driver.  However I have been
> careful not to break anything from the Dove drivers perspective. It
> would be great if somebody can test on Cubox and confirm that.
>
> This patch set inverts the hsync signal coming from the tilcdc so the NXP
> is kept happy and then shifts the output to the right to compensate for the
> sync timing issues.  Display modes from the NXP have been verified using a
> HDMI analyzer and are reporting correct timings at the output stage.
>
> Hopefully this will allow the dove/tda driver changes to progress now that
> were blocked as per this discussion:
> http://lists.freedesktop.org/archives/dri-devel/2013-July/040900.html
>

Good find Darren!  The patches look good to me from a quick review.
It would be good to get a tested-by from someone on cubox, but it is
good that we finally found the issue so that we can unblock further
tda998x development.

For the series:

Reviewed-by: Rob Clark <robdclark@gmail.com>


> Darren Etheridge (2):
>   drm/i2c/tda998x prepare for tilcdc sync workaround
>   drm/tilcdc fixup mode to workaound sync for tda998x
>
>  drivers/gpu/drm/i2c/tda998x_drv.c     |   10 ++++++++++
>  drivers/gpu/drm/tilcdc/tilcdc_crtc.c  |    7 ++++++-
>  drivers/gpu/drm/tilcdc/tilcdc_slave.c |   21 ++++++++++++++++++++-
>  3 files changed, 36 insertions(+), 2 deletions(-)
>

Re: [PATCH 0/2] drm/tilcdc drm/i2c/tda998x workaround for sync issues on TI SoC

[Sebastian Hesselbarth]

On 07/25/2013 09:32 PM, Rob Clark wrote:
> On Thu, Jul 25, 2013 at 2:32 PM, Darren Etheridge <detheridge@ti.com> wrote:
>> Russell King and Sebastian Hasselbarth had proposed some very good changes
>> for the tda998x HDMI encoder driver.  But when those changes were tested
>> on BeagleBone Black against the tilcdc driver many modes would no longer
>> display correctly.  After analyzing the sync signals from the TI lcd contoller
>> to the nxp it is apparent that the hsync/vsync's are not rising at the same
>> time as per the VESA spec and this is causing the HDMI encoder to get
>> messed up and failing to lock correctly.
>>
>> This series of patches should be applied on top of:
>>
>> Russell King's
>> rmk's Dove DRM/TDA19988 Cubox driver series
>>
>> Sebastian Hasselbarth's
>> drm/i2c: tda998x: fix sync generation and calculation
>>
>> I have done as much of the change as I can in the tilcdc driver but there
>> is a small unavoidable change in the tda998x driver.  However I have been
>> careful not to break anything from the Dove drivers perspective. It
>> would be great if somebody can test on Cubox and confirm that.
>>
>> This patch set inverts the hsync signal coming from the tilcdc so the NXP
>> is kept happy and then shifts the output to the right to compensate for the
>> sync timing issues.  Display modes from the NXP have been verified using a
>> HDMI analyzer and are reporting correct timings at the output stage.
>>
>> Hopefully this will allow the dove/tda driver changes to progress now that
>> were blocked as per this discussion:
>> http://lists.freedesktop.org/archives/dri-devel/2013-July/040900.html
>>
>
> Good find Darren!  The patches look good to me from a quick review.
> It would be good to get a tested-by from someone on cubox, but it is
> good that we finally found the issue so that we can unblock further
> tda998x development.

Thanks to Darren, I can now test tda998x sync changes on both Cubox and
Beaglebone Black. I don't think the changes will affect Armada DRM in
any way, as it is not setting adjusted_mode.

I will put a scope on AM335x/TDA998x sync signals to fully understand
the issues tilcdc has, but I can think of a flag for TDA998x to always
accept falling input hsync/vsync and tilcdc to supply that sync. That
will maybe allow us to get rid of hskew workaround.

As far as I understand the issue, tilcdc always aligns VS with the
rising HS edge? If so, enforcing positive HS/VS on tilcdc and telling
TDA998x that it is always positive, may be a cleaner workaround. TDA998x
can invert input and output sync independently.

In any way, it will take some time to get a working setup. If you are
happy with the patches, I can still send some follow-up patches later.

Sebastian

Re: [PATCH 0/2] drm/tilcdc drm/i2c/tda998x workaround for sync issues on TI SoC

[Sebastian Hesselbarth]

On 07/25/2013 09:32 PM, Rob Clark wrote:
> On Thu, Jul 25, 2013 at 2:32 PM, Darren Etheridge <detheridge@ti.com> wrote:
[...]
>> This patch set inverts the hsync signal coming from the tilcdc so the NXP
>> is kept happy and then shifts the output to the right to compensate for the
>> sync timing issues.  Display modes from the NXP have been verified using a
>> HDMI analyzer and are reporting correct timings at the output stage.
>>
>> Hopefully this will allow the dove/tda driver changes to progress now that
>> were blocked as per this discussion:
>> http://lists.freedesktop.org/archives/dri-devel/2013-July/040900.html
>>
>
> Good find Darren!  The patches look good to me from a quick review.
> It would be good to get a tested-by from someone on cubox, but it is
> good that we finally found the issue so that we can unblock further
> tda998x development.

Darren,

I now fully understand the issues of AM335x's LCD controller and your
fix for it. I suggest to clarify the comments you added to tilcdc to
allow others to understand it more quickly.

Actually, the LCD controller always aligns vsync to the second edge
of hsync, which will never give VESA-compliant sync. The (elegant)
workaround you are proposing is to align both rising edges, so at
least TDA998x can sync on those with some hskew added. Lucky you that
it ignores hsync length but only looks for rising HS/VS edges ;)

Should we prepare a new patch set comprising the following patches?

Russell King:
drm/i2c: nxp-tda998x: fix EDID reading on TDA19988 devices
drm/i2c: nxp-tda998x: ensure VIP output mux is properly set
drm/i2c: nxp-tda998x: fix npix/nline programming
drm/i2c: nxp-tda998x: prepare for video input configuration
drm/i2c: nxp-tda998x: add video and audio input configuration

Sebastian Hesselbarth:
drm/i2c: tda998x: fix sync generation and calculation

Darren Etheridge:
drm/i2c/tda998x prepare for tilcdc sync workaround
drm/tilcdc fixup mode to workaound sync for tda998x

Or do we keep them separated and possibly resend them if David cannot
find them anymore?

Sebastian

Re: [PATCH 0/2] drm/tilcdc drm/i2c/tda998x workaround for sync issues on TI SoC

[Russell King - ARM Linux]

On Wed, Jul 31, 2013 at 10:21:20PM +0200, Sebastian Hesselbarth wrote:
> Should we prepare a new patch set comprising the following patches?
>
> Russell King:
> drm/i2c: nxp-tda998x: fix EDID reading on TDA19988 devices
> drm/i2c: nxp-tda998x: ensure VIP output mux is properly set
> drm/i2c: nxp-tda998x: fix npix/nline programming
> drm/i2c: nxp-tda998x: prepare for video input configuration
> drm/i2c: nxp-tda998x: add video and audio input configuration
>
> Sebastian Hesselbarth:
> drm/i2c: tda998x: fix sync generation and calculation

Unless you've changed this one since I tested it, it needs fixes which
basically revert the vsync changes back to the original code.

Re: [PATCH 0/2] drm/tilcdc drm/i2c/tda998x workaround for sync issues on TI SoC

[Darren Etheridge]

Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> wrote on Wed [2013-Jul-31 22:21:20 +0200]:
> Darren,
> 
> I now fully understand the issues of AM335x's LCD controller and your
> fix for it. I suggest to clarify the comments you added to tilcdc to
> allow others to understand it more quickly.
> 
Sebastian,
Thanks for looking at my proposed changes, you understand this sync
stuff very well so I appreciate your input that this is actually an
acceptable workaround.

> Actually, the LCD controller always aligns vsync to the second edge
> of hsync, which will never give VESA-compliant sync. The (elegant)
> workaround you are proposing is to align both rising edges, so at
> least TDA998x can sync on those with some hskew added. Lucky you that
> it ignores hsync length but only looks for rising HS/VS edges ;)
Yes we definitely got lucky with this one, good thing the NXP
supported that reference pixel position, as I was out of options from
the lcd controller side of things to adjust the horizontal position. 

> 
> Should we prepare a new patch set comprising the following patches?
> 
> Russell King:
> drm/i2c: nxp-tda998x: fix EDID reading on TDA19988 devices
> drm/i2c: nxp-tda998x: ensure VIP output mux is properly set
> drm/i2c: nxp-tda998x: fix npix/nline programming
> drm/i2c: nxp-tda998x: prepare for video input configuration
> drm/i2c: nxp-tda998x: add video and audio input configuration
> 
> Sebastian Hesselbarth:
> drm/i2c: tda998x: fix sync generation and calculation
> 
> Darren Etheridge:
> drm/i2c/tda998x prepare for tilcdc sync workaround
> drm/tilcdc fixup mode to workaound sync for tda998x
> 
> Or do we keep them separated and possibly resend them if David cannot
> find them anymore?
I vote we submit a complete series that we can all test, there were quite
a lot of versions of things in flight at the same time so I am sure
David would appreciate a consolidated version.  
The only thing I have not tested is audio support, but as the original 
driver did not have that anyway I don't consider it blocking if it is 
working for CuBox.

Darren

Re: [PATCH 0/2] drm/tilcdc drm/i2c/tda998x workaround for sync issues on TI SoC

[Rob Clark]

On Thu, Aug 1, 2013 at 10:29 AM, Darren Etheridge <detheridge@ti.com> wrote:
> Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> wrote on Wed [2013-Jul-31 22:21:20 +0200]:
>> Darren,
>>
>> I now fully understand the issues of AM335x's LCD controller and your
>> fix for it. I suggest to clarify the comments you added to tilcdc to
>> allow others to understand it more quickly.
>>
> Sebastian,
> Thanks for looking at my proposed changes, you understand this sync
> stuff very well so I appreciate your input that this is actually an
> acceptable workaround.
>
>> Actually, the LCD controller always aligns vsync to the second edge
>> of hsync, which will never give VESA-compliant sync. The (elegant)
>> workaround you are proposing is to align both rising edges, so at
>> least TDA998x can sync on those with some hskew added. Lucky you that
>> it ignores hsync length but only looks for rising HS/VS edges ;)
> Yes we definitely got lucky with this one, good thing the NXP
> supported that reference pixel position, as I was out of options from
> the lcd controller side of things to adjust the horizontal position.
>
>>
>> Should we prepare a new patch set comprising the following patches?
>>
>> Russell King:
>> drm/i2c: nxp-tda998x: fix EDID reading on TDA19988 devices
>> drm/i2c: nxp-tda998x: ensure VIP output mux is properly set
>> drm/i2c: nxp-tda998x: fix npix/nline programming
>> drm/i2c: nxp-tda998x: prepare for video input configuration
>> drm/i2c: nxp-tda998x: add video and audio input configuration
>>
>> Sebastian Hesselbarth:
>> drm/i2c: tda998x: fix sync generation and calculation
>>
>> Darren Etheridge:
>> drm/i2c/tda998x prepare for tilcdc sync workaround
>> drm/tilcdc fixup mode to workaound sync for tda998x
>>
>> Or do we keep them separated and possibly resend them if David cannot
>> find them anymore?
> I vote we submit a complete series that we can all test, there were quite
> a lot of versions of things in flight at the same time so I am sure
> David would appreciate a consolidated version.

I'm sure Dave would appreciate it if someone set up a tree w/ all the
patches consolidated and sent a pull req.

I can help with that if needed, but it probably makes more sense for
someone who is using a beagle-bone and/or CuBox on a more regular
basis to do this

BR,
-R

> The only thing I have not tested is audio support, but as the original
> driver did not have that anyway I don't consider it blocking if it is
> working for CuBox.
>
> Darren