[PATCH v2 0/7] imx-drm: ipuv3-crtc: Implement mode_fixup

[PATCH v2 0/7] imx-drm: ipuv3-crtc: Implement mode_fixup

[Steve Longerbeam]

This patchset implements ->mode_fixup() in the imx ipuv3-crtc driver,
using a new support function ipu_di_adjust_videomode(). This new
function needs to be subsystem independent, so it accepts a video
mode as a 'struct videomode'. Hence ipu-crtc ->mode_fixup() needs
another support function to convert a drm_display_mode to a videomode
before passing the mode to ipu_di_adjust_videomode() for fixup.

Also some related code cleanup: 'struct ipu_di_signal_cfg' should
use 'struct videomode' for mode timings.


Jiada Wang (1):
  gpu: ipu-di: Add ipu_di_adjust_videomode()

Steve Longerbeam (6):
  gpu: ipu-di: remove some non-functional code
  drm_modes: add videomode_from_drm_display_mode
  imx-drm: ipuv3-crtc: Implement mode_fixup
  imx-drm: encoder prepare/mode_set must use adjusted mode
  gpu: ipu-v3: Use videomode in struct ipu_di_signal_cfg
  gpu: ipu-di: Switch to DIV_ROUND_CLOSEST for DI clock divider calc

 drivers/gpu/drm/drm_modes.c            |   40 +++++++++++
 drivers/gpu/drm/imx/imx-hdmi.c         |    4 +-
 drivers/gpu/drm/imx/imx-ldb.c          |    6 +-
 drivers/gpu/drm/imx/imx-tve.c          |    4 +-
 drivers/gpu/drm/imx/ipuv3-crtc.c       |   38 +++++-----
 drivers/gpu/drm/imx/parallel-display.c |    4 +-
 drivers/gpu/ipu-v3/ipu-di.c            |  121 +++++++++++++++++++-------------
 include/drm/drm_modes.h                |    2 +
 include/video/imx-ipu-v3.h             |   21 ++----
 9 files changed, 147 insertions(+), 93 deletions(-)

-- 
1.7.9.5

[PATCH v2 1/7] gpu: ipu-di: Add ipu_di_adjust_videomode()

[Steve Longerbeam]

From: Jiada Wang <jiada_wang@mentor.com>

On some monitors, high resolution modes are not working, exhibiting
pixel column truncation problems (for example, 1280x1024 displays as
1280x1022).

The function ipu_di_adjust_videomode() aims to fix these issues by
adjusting a passed videomode to IPU restrictions. The function can
be called from the drm_crtc_helper_funcs->mode_fixup() methods.

Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
Signed-off-by: Deepak Das <deepak_das@mentor.com>
Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
 drivers/gpu/ipu-v3/ipu-di.c |   29 +++++++++++++++++++++++++++++
 include/video/imx-ipu-v3.h  |    2 ++
 2 files changed, 31 insertions(+)

diff --git a/drivers/gpu/ipu-v3/ipu-di.c b/drivers/gpu/ipu-v3/ipu-di.c
index c490ba4..46f9570 100644
--- a/drivers/gpu/ipu-v3/ipu-di.c
+++ b/drivers/gpu/ipu-v3/ipu-di.c
@@ -511,6 +511,35 @@ static void ipu_di_config_clock(struct ipu_di *di,
 		clk_get_rate(di->clk_di_pixel) / (clkgen0 >> 4));
 }
 
+/*
+ * This function is called to adjust a video mode to IPU restrictions.
+ * It is meant to be called from drm crtc mode_fixup() methods.
+ */
+int ipu_di_adjust_videomode(struct ipu_di *di, struct videomode *mode)
+{
+	u32 diff;
+
+	if (mode->vfront_porch >= 2)
+		return 0;
+
+	diff = 2 - mode->vfront_porch;
+
+	if (mode->vback_porch >= diff) {
+		mode->vfront_porch = 2;
+		mode->vback_porch -= diff;
+	} else if (mode->vsync_len > diff) {
+		mode->vfront_porch = 2;
+		mode->vsync_len = mode->vsync_len - diff;
+	} else {
+		dev_warn(di->ipu->dev, "failed to adjust videomode\n");
+		return -EINVAL;
+	}
+
+	dev_warn(di->ipu->dev, "videomode adapted for IPU restrictions\n");
+	return 0;
+}
+EXPORT_SYMBOL_GPL(ipu_di_adjust_videomode);
+
 int ipu_di_init_sync_panel(struct ipu_di *di, struct ipu_di_signal_cfg *sig)
 {
 	u32 reg;
diff --git a/include/video/imx-ipu-v3.h b/include/video/imx-ipu-v3.h
index c74bf4a..d333d54 100644
--- a/include/video/imx-ipu-v3.h
+++ b/include/video/imx-ipu-v3.h
@@ -17,6 +17,7 @@
 #include <linux/bitmap.h>
 #include <linux/fb.h>
 #include <media/v4l2-mediabus.h>
+#include <video/videomode.h>
 
 struct ipu_soc;
 
@@ -236,6 +237,7 @@ void ipu_di_put(struct ipu_di *);
 int ipu_di_disable(struct ipu_di *);
 int ipu_di_enable(struct ipu_di *);
 int ipu_di_get_num(struct ipu_di *);
+int ipu_di_adjust_videomode(struct ipu_di *di, struct videomode *mode);
 int ipu_di_init_sync_panel(struct ipu_di *, struct ipu_di_signal_cfg *sig);
 
 /*
-- 
1.7.9.5

Re: [PATCH v2 1/7] gpu: ipu-di: Add ipu_di_adjust_videomode()

[Philipp Zabel]

Hi Steve,

Am Donnerstag, den 18.12.2014, 18:00 -0800 schrieb Steve Longerbeam:
> From: Jiada Wang <jiada_wang@mentor.com>
> 
> On some monitors, high resolution modes are not working, exhibiting
> pixel column truncation problems (for example, 1280x1024 displays as
> 1280x1022).
> 
> The function ipu_di_adjust_videomode() aims to fix these issues by
> adjusting a passed videomode to IPU restrictions. The function can
> be called from the drm_crtc_helper_funcs->mode_fixup() methods.
> 
> Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
> Signed-off-by: Deepak Das <deepak_das@mentor.com>
> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> ---
>  drivers/gpu/ipu-v3/ipu-di.c |   29 +++++++++++++++++++++++++++++
>  include/video/imx-ipu-v3.h  |    2 ++
>  2 files changed, 31 insertions(+)
> 
> diff --git a/drivers/gpu/ipu-v3/ipu-di.c b/drivers/gpu/ipu-v3/ipu-di.c
> index c490ba4..46f9570 100644
> --- a/drivers/gpu/ipu-v3/ipu-di.c
> +++ b/drivers/gpu/ipu-v3/ipu-di.c
> @@ -511,6 +511,35 @@ static void ipu_di_config_clock(struct ipu_di *di,
>  		clk_get_rate(di->clk_di_pixel) / (clkgen0 >> 4));
>  }
>  
> +/*
> + * This function is called to adjust a video mode to IPU restrictions.
> + * It is meant to be called from drm crtc mode_fixup() methods.
> + */
> +int ipu_di_adjust_videomode(struct ipu_di *di, struct videomode *mode)
> +{
> +	u32 diff;
> +
> +	if (mode->vfront_porch >= 2)
> +		return 0;
> +
> +	diff = 2 - mode->vfront_porch;
> +
> +	if (mode->vback_porch >= diff) {
> +		mode->vfront_porch = 2;
> +		mode->vback_porch -= diff;

Should we also add

	else if (mode->vback_porch >= 1 && mode->vsync_len > 1) {
		mode->vback_porch--;
		mode->vsync_len--;

here and maybe move the two "mode->vback_porch = 2" to a single place
below the conditional statement?

> +	} else if (mode->vsync_len > diff) {
> +		mode->vfront_porch = 2;
> +		mode->vsync_len = mode->vsync_len - diff;

Why use "vback_porch -= diff" above, but not "vsync_len -= diff" here?

> +	} else {
> +		dev_warn(di->ipu->dev, "failed to adjust videomode\n");
> +		return -EINVAL;
> +	}
> +
> +	dev_warn(di->ipu->dev, "videomode adapted for IPU restrictions\n");

Since we can return the adjusted mode to userspace now, I think this
should be dev_dbg.

regards
Philipp

[PATCH v2 2/7] gpu: ipu-di: remove some non-functional code

[Steve Longerbeam]

h_total and v_total were calculated in ipu_di_init_sync_panel()
but never actually used. Remove.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
 drivers/gpu/ipu-v3/ipu-di.c |    6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/gpu/ipu-v3/ipu-di.c b/drivers/gpu/ipu-v3/ipu-di.c
index 46f9570..41df8d7 100644
--- a/drivers/gpu/ipu-v3/ipu-di.c
+++ b/drivers/gpu/ipu-v3/ipu-di.c
@@ -545,7 +545,6 @@ int ipu_di_init_sync_panel(struct ipu_di *di, struct ipu_di_signal_cfg *sig)
 	u32 reg;
 	u32 di_gen, vsync_cnt;
 	u32 div;
-	u32 h_total, v_total;
 
 	dev_dbg(di->ipu->dev, "disp %d: panel size = %d x %d\n",
 		di->id, sig->width, sig->height);
@@ -553,11 +552,6 @@ int ipu_di_init_sync_panel(struct ipu_di *di, struct ipu_di_signal_cfg *sig)
 	if ((sig->v_sync_width = 0) || (sig->h_sync_width = 0))
 		return -EINVAL;
 
-	h_total = sig->width + sig->h_sync_width + sig->h_start_width +
-		sig->h_end_width;
-	v_total = sig->height + sig->v_sync_width + sig->v_start_width +
-		sig->v_end_width;
-
 	dev_dbg(di->ipu->dev, "Clocks: IPU %luHz DI %luHz Needed %luHz\n",
 		clk_get_rate(di->clk_ipu),
 		clk_get_rate(di->clk_di),
-- 
1.7.9.5

[PATCH v2 3/7] drm_modes: add videomode_from_drm_display_mode

[Steve Longerbeam]

Add conversion from drm_display_mode to videomode.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
 drivers/gpu/drm/drm_modes.c |   40 ++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_modes.h     |    2 ++
 2 files changed, 42 insertions(+)

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 6d8b941..583a391 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -615,6 +615,46 @@ void drm_display_mode_from_videomode(const struct videomode *vm,
 }
 EXPORT_SYMBOL_GPL(drm_display_mode_from_videomode);
 
+/**
+ * videomode_from_drm_display_mode - fill in @vm using @dmode,
+ * @dmode: drm_display_mode structure to use as source
+ * @vm: videomode structure to use as destination
+ *
+ * Fills out @vm using the display mode specified in @dmode.
+ */
+void videomode_from_drm_display_mode(const struct drm_display_mode *dmode,
+				     struct videomode *vm)
+{
+	vm->hactive = dmode->hdisplay;
+	vm->hfront_porch = dmode->hsync_start - dmode->hdisplay;
+	vm->hsync_len = dmode->hsync_end - dmode->hsync_start;
+	vm->hback_porch = dmode->htotal - dmode->hsync_end;
+
+	vm->vactive = dmode->vdisplay;
+	vm->vfront_porch = dmode->vsync_start - dmode->vdisplay;
+	vm->vsync_len = dmode->vsync_end - dmode->vsync_start;
+	vm->vback_porch = dmode->vtotal - dmode->vsync_end;
+
+	vm->pixelclock = dmode->clock * 1000;
+
+	vm->flags = 0;
+	if (dmode->flags & DRM_MODE_FLAG_PHSYNC)
+		vm->flags |= DISPLAY_FLAGS_HSYNC_HIGH;
+	else if (dmode->flags & DRM_MODE_FLAG_NHSYNC)
+		vm->flags |= DISPLAY_FLAGS_HSYNC_LOW;
+	if (dmode->flags & DRM_MODE_FLAG_PVSYNC)
+		vm->flags |= DISPLAY_FLAGS_VSYNC_HIGH;
+	else if (dmode->flags & DRM_MODE_FLAG_NVSYNC)
+		vm->flags |= DISPLAY_FLAGS_VSYNC_LOW;
+	if (dmode->flags & DRM_MODE_FLAG_INTERLACE)
+		vm->flags |= DISPLAY_FLAGS_INTERLACED;
+	if (dmode->flags & DRM_MODE_FLAG_DBLSCAN)
+		vm->flags |= DISPLAY_FLAGS_DOUBLESCAN;
+	if (dmode->flags & DRM_MODE_FLAG_DBLCLK)
+		vm->flags |= DISPLAY_FLAGS_DOUBLECLK;
+}
+EXPORT_SYMBOL_GPL(videomode_from_drm_display_mode);
+
 #ifdef CONFIG_OF
 /**
  * of_get_drm_display_mode - get a drm_display_mode from devicetree
diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
index 91d0582..60c0144 100644
--- a/include/drm/drm_modes.h
+++ b/include/drm/drm_modes.h
@@ -197,6 +197,8 @@ struct drm_display_mode *drm_gtf_mode_complex(struct drm_device *dev,
 					      int GTF_K, int GTF_2J);
 void drm_display_mode_from_videomode(const struct videomode *vm,
 				     struct drm_display_mode *dmode);
+void videomode_from_drm_display_mode(const struct drm_display_mode *dmode,
+				     struct videomode *vm);
 int of_get_drm_display_mode(struct device_node *np,
 			    struct drm_display_mode *dmode,
 			    int index);
-- 
1.7.9.5

Re: [PATCH v2 3/7] drm_modes: add videomode_from_drm_display_mode

[Philipp Zabel]

Am Donnerstag, den 18.12.2014, 18:00 -0800 schrieb Steve Longerbeam:
> Add conversion from drm_display_mode to videomode.
> 
> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> ---
>  drivers/gpu/drm/drm_modes.c |   40 ++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_modes.h     |    2 ++
>  2 files changed, 42 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index 6d8b941..583a391 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -615,6 +615,46 @@ void drm_display_mode_from_videomode(const struct videomode *vm,
>  }
>  EXPORT_SYMBOL_GPL(drm_display_mode_from_videomode);
>  
> +/**
> + * videomode_from_drm_display_mode - fill in @vm using @dmode,
> + * @dmode: drm_display_mode structure to use as source
> + * @vm: videomode structure to use as destination
> + *
> + * Fills out @vm using the display mode specified in @dmode.
> + */
> +void videomode_from_drm_display_mode(const struct drm_display_mode *dmode,
> +				     struct videomode *vm)
> +{
> +	vm->hactive = dmode->hdisplay;
> +	vm->hfront_porch = dmode->hsync_start - dmode->hdisplay;
> +	vm->hsync_len = dmode->hsync_end - dmode->hsync_start;
> +	vm->hback_porch = dmode->htotal - dmode->hsync_end;
> +
> +	vm->vactive = dmode->vdisplay;
> +	vm->vfront_porch = dmode->vsync_start - dmode->vdisplay;
> +	vm->vsync_len = dmode->vsync_end - dmode->vsync_start;
> +	vm->vback_porch = dmode->vtotal - dmode->vsync_end;
> +
> +	vm->pixelclock = dmode->clock * 1000;
> +
> +	vm->flags = 0;
> +	if (dmode->flags & DRM_MODE_FLAG_PHSYNC)
> +		vm->flags |= DISPLAY_FLAGS_HSYNC_HIGH;
> +	else if (dmode->flags & DRM_MODE_FLAG_NHSYNC)
> +		vm->flags |= DISPLAY_FLAGS_HSYNC_LOW;
> +	if (dmode->flags & DRM_MODE_FLAG_PVSYNC)
> +		vm->flags |= DISPLAY_FLAGS_VSYNC_HIGH;
> +	else if (dmode->flags & DRM_MODE_FLAG_NVSYNC)
> +		vm->flags |= DISPLAY_FLAGS_VSYNC_LOW;
> +	if (dmode->flags & DRM_MODE_FLAG_INTERLACE)
> +		vm->flags |= DISPLAY_FLAGS_INTERLACED;
> +	if (dmode->flags & DRM_MODE_FLAG_DBLSCAN)
> +		vm->flags |= DISPLAY_FLAGS_DOUBLESCAN;
> +	if (dmode->flags & DRM_MODE_FLAG_DBLCLK)
> +		vm->flags |= DISPLAY_FLAGS_DOUBLECLK;
> +}
> +EXPORT_SYMBOL_GPL(videomode_from_drm_display_mode);

Is it ok for drm_modes to export a function that doesn't start with
drm_ ? We could just rename this to drm_display_mode_to_videomode if
necessary. I can fix it up as I apply it, but I'd like to know which is
preferred.

regards
Philipp

Re: [PATCH v2 3/7] drm_modes: add videomode_from_drm_display_mode

[Emil Velikov]

On 19/12/14 11:03, Philipp Zabel wrote:
> Am Donnerstag, den 18.12.2014, 18:00 -0800 schrieb Steve Longerbeam:
>> Add conversion from drm_display_mode to videomode.
>>
>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>> ---
>>  drivers/gpu/drm/drm_modes.c |   40 ++++++++++++++++++++++++++++++++++++++++
>>  include/drm/drm_modes.h     |    2 ++
>>  2 files changed, 42 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
>> index 6d8b941..583a391 100644
>> --- a/drivers/gpu/drm/drm_modes.c
>> +++ b/drivers/gpu/drm/drm_modes.c
>> @@ -615,6 +615,46 @@ void drm_display_mode_from_videomode(const struct videomode *vm,
>>  }
>>  EXPORT_SYMBOL_GPL(drm_display_mode_from_videomode);
>>  
>> +/**
>> + * videomode_from_drm_display_mode - fill in @vm using @dmode,
>> + * @dmode: drm_display_mode structure to use as source
>> + * @vm: videomode structure to use as destination
>> + *
>> + * Fills out @vm using the display mode specified in @dmode.
>> + */
>> +void videomode_from_drm_display_mode(const struct drm_display_mode *dmode,
>> +				     struct videomode *vm)
>> +{
>> +	vm->hactive = dmode->hdisplay;
>> +	vm->hfront_porch = dmode->hsync_start - dmode->hdisplay;
>> +	vm->hsync_len = dmode->hsync_end - dmode->hsync_start;
>> +	vm->hback_porch = dmode->htotal - dmode->hsync_end;
>> +
>> +	vm->vactive = dmode->vdisplay;
>> +	vm->vfront_porch = dmode->vsync_start - dmode->vdisplay;
>> +	vm->vsync_len = dmode->vsync_end - dmode->vsync_start;
>> +	vm->vback_porch = dmode->vtotal - dmode->vsync_end;
>> +
>> +	vm->pixelclock = dmode->clock * 1000;
>> +
>> +	vm->flags = 0;
>> +	if (dmode->flags & DRM_MODE_FLAG_PHSYNC)
>> +		vm->flags |= DISPLAY_FLAGS_HSYNC_HIGH;
>> +	else if (dmode->flags & DRM_MODE_FLAG_NHSYNC)
>> +		vm->flags |= DISPLAY_FLAGS_HSYNC_LOW;
>> +	if (dmode->flags & DRM_MODE_FLAG_PVSYNC)
>> +		vm->flags |= DISPLAY_FLAGS_VSYNC_HIGH;
>> +	else if (dmode->flags & DRM_MODE_FLAG_NVSYNC)
>> +		vm->flags |= DISPLAY_FLAGS_VSYNC_LOW;
>> +	if (dmode->flags & DRM_MODE_FLAG_INTERLACE)
>> +		vm->flags |= DISPLAY_FLAGS_INTERLACED;
>> +	if (dmode->flags & DRM_MODE_FLAG_DBLSCAN)
>> +		vm->flags |= DISPLAY_FLAGS_DOUBLESCAN;
>> +	if (dmode->flags & DRM_MODE_FLAG_DBLCLK)
>> +		vm->flags |= DISPLAY_FLAGS_DOUBLECLK;
>> +}
>> +EXPORT_SYMBOL_GPL(videomode_from_drm_display_mode);
> 
> Is it ok for drm_modes to export a function that doesn't start with
> drm_ ? We could just rename this to drm_display_mode_to_videomode if
> necessary. I can fix it up as I apply it, but I'd like to know which is
> preferred.
> 
Non-native English speaker here, and drm_display_mode_to_videomode
sounds better. Plus I think that *to* functions are more common than
*from* ones.

I'm not a drm dev, so just my 2c :)

Cheers,
Emil

Re: [PATCH v2 3/7] drm_modes: add videomode_from_drm_display_mode

[Steve Longerbeam]

On 12/19/2014 03:03 AM, Philipp Zabel wrote:
>
> +EXPORT_SYMBOL_GPL(videomode_from_drm_display_mode);
> Is it ok for drm_modes to export a function that doesn't start with
> drm_ ? We could just rename this to drm_display_mode_to_videomode if
> necessary. I can fix it up as I apply it, but I'd like to know which is
> preferred.

Yeah, drm_display_mode_to_videomode() is probably better,
makes it more clear it's part of the DRM kernel interfaces.

Steve

[PATCH v2 4/7] imx-drm: ipuv3-crtc: Implement mode_fixup

[Steve Longerbeam]

Ask the IPU display interface, via ipu_di_adjust_videomode(), to
adjust a video mode to meet any DI restrictions. The function takes
a subsystem independent videomode, so the drm_display_mode must be
converted to videomode first, and then the adjusted mode converted
back to a drm_display_mode.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
 drivers/gpu/drm/imx/ipuv3-crtc.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
index 11e84a2..fb16026 100644
--- a/drivers/gpu/drm/imx/ipuv3-crtc.c
+++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
@@ -242,6 +242,18 @@ static bool ipu_crtc_mode_fixup(struct drm_crtc *crtc,
 				  const struct drm_display_mode *mode,
 				  struct drm_display_mode *adjusted_mode)
 {
+	struct ipu_crtc *ipu_crtc = to_ipu_crtc(crtc);
+	struct videomode vm;
+	int ret;
+
+	videomode_from_drm_display_mode(adjusted_mode, &vm);
+
+	ret = ipu_di_adjust_videomode(ipu_crtc->di, &vm);
+	if (ret)
+		return false;
+
+	drm_display_mode_from_videomode(&vm, adjusted_mode);
+
 	return true;
 }
 
-- 
1.7.9.5

Re: [PATCH v2 4/7] imx-drm: ipuv3-crtc: Implement mode_fixup

[Russell King - ARM Linux]

On Thu, Dec 18, 2014 at 06:00:23PM -0800, Steve Longerbeam wrote:
> Ask the IPU display interface, via ipu_di_adjust_videomode(), to
> adjust a video mode to meet any DI restrictions. The function takes
> a subsystem independent videomode, so the drm_display_mode must be
> converted to videomode first, and then the adjusted mode converted
> back to a drm_display_mode.

What is the reason to use videomode (apart from it being subsystem
independent)?  Do we forsee implementation of other output subsystems
for the IPU?

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v2 4/7] imx-drm: ipuv3-crtc: Implement mode_fixup

[Philipp Zabel]

Hi Russell,

On Sat, Dec 20, 2014 at 03:52:54PM +0000, Russell King - ARM Linux wrote:
> On Thu, Dec 18, 2014 at 06:00:23PM -0800, Steve Longerbeam wrote:
> > Ask the IPU display interface, via ipu_di_adjust_videomode(), to
> > adjust a video mode to meet any DI restrictions. The function takes
> > a subsystem independent videomode, so the drm_display_mode must be
> > converted to videomode first, and then the adjusted mode converted
> > back to a drm_display_mode.
> 
> What is the reason to use videomode (apart from it being subsystem
> independent)?  Do we forsee implementation of other output subsystems
> for the IPU?

There might be the issue of possible CSI -> IC -> DC -> DI passthrough,
where the DI timing must be synchronized to the CSI input signal.
I haven't really thought about how that should be integrated with the
DRM driver mostly because of the 1024 pixel output width maximum in the
IC, which limits the usefulness somewhat.
I like the use of struct videomode here for the symmetry with patch 6.
But currently, we could make ipu_di_adjust_videomode take a drm_display_mode
just as well.

regards
Philipp

[PATCH v2 5/7] imx-drm: encoder prepare/mode_set must use adjusted mode

[Steve Longerbeam]

The encoder ->prepare() and ->mode_set() methods need to use the
hw adjusted mode, not the original mode.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
 drivers/gpu/drm/imx/imx-hdmi.c         |    4 ++--
 drivers/gpu/drm/imx/imx-ldb.c          |    6 +++---
 drivers/gpu/drm/imx/imx-tve.c          |    4 ++--
 drivers/gpu/drm/imx/parallel-display.c |    4 ++--
 4 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/imx/imx-hdmi.c b/drivers/gpu/drm/imx/imx-hdmi.c
index aaec6b2..32116cc 100644
--- a/drivers/gpu/drm/imx/imx-hdmi.c
+++ b/drivers/gpu/drm/imx/imx-hdmi.c
@@ -1417,8 +1417,8 @@ static struct drm_encoder *imx_hdmi_connector_best_encoder(struct drm_connector
 }
 
 static void imx_hdmi_encoder_mode_set(struct drm_encoder *encoder,
-			struct drm_display_mode *mode,
-			struct drm_display_mode *adjusted_mode)
+			struct drm_display_mode *orig_mode,
+			struct drm_display_mode *mode)
 {
 	struct imx_hdmi *hdmi = container_of(encoder, struct imx_hdmi, encoder);
 
diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
index 4662e00..5b9c875 100644
--- a/drivers/gpu/drm/imx/imx-ldb.c
+++ b/drivers/gpu/drm/imx/imx-ldb.c
@@ -168,7 +168,7 @@ static void imx_ldb_encoder_prepare(struct drm_encoder *encoder)
 {
 	struct imx_ldb_channel *imx_ldb_ch = enc_to_imx_ldb_ch(encoder);
 	struct imx_ldb *ldb = imx_ldb_ch->ldb;
-	struct drm_display_mode *mode = &encoder->crtc->mode;
+	struct drm_display_mode *mode = &encoder->crtc->hwmode;
 	u32 pixel_fmt;
 	unsigned long serial_clk;
 	unsigned long di_clk = mode->clock * 1000;
@@ -246,8 +246,8 @@ static void imx_ldb_encoder_commit(struct drm_encoder *encoder)
 }
 
 static void imx_ldb_encoder_mode_set(struct drm_encoder *encoder,
-			 struct drm_display_mode *mode,
-			 struct drm_display_mode *adjusted_mode)
+			 struct drm_display_mode *orig_mode,
+			 struct drm_display_mode *mode)
 {
 	struct imx_ldb_channel *imx_ldb_ch = enc_to_imx_ldb_ch(encoder);
 	struct imx_ldb *ldb = imx_ldb_ch->ldb;
diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c
index 42c651b..9709bf9a 100644
--- a/drivers/gpu/drm/imx/imx-tve.c
+++ b/drivers/gpu/drm/imx/imx-tve.c
@@ -312,8 +312,8 @@ static void imx_tve_encoder_prepare(struct drm_encoder *encoder)
 }
 
 static void imx_tve_encoder_mode_set(struct drm_encoder *encoder,
-				     struct drm_display_mode *mode,
-				     struct drm_display_mode *adjusted_mode)
+				     struct drm_display_mode *orig_mode,
+				     struct drm_display_mode *mode)
 {
 	struct imx_tve *tve = enc_to_tve(encoder);
 	unsigned long rounded_rate;
diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
index 015a454..d0842a4 100644
--- a/drivers/gpu/drm/imx/parallel-display.c
+++ b/drivers/gpu/drm/imx/parallel-display.c
@@ -131,8 +131,8 @@ static void imx_pd_encoder_commit(struct drm_encoder *encoder)
 }
 
 static void imx_pd_encoder_mode_set(struct drm_encoder *encoder,
-			 struct drm_display_mode *mode,
-			 struct drm_display_mode *adjusted_mode)
+			 struct drm_display_mode *orig_mode,
+			 struct drm_display_mode *mode)
 {
 }
 
-- 
1.7.9.5

[PATCH v2 6/7] gpu: ipu-v3: Use videomode in struct ipu_di_signal_cfg

[Steve Longerbeam]

This patch changes struct ipu_di_signal_cfg to use struct videomode
to define video timings and flags.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
 drivers/gpu/drm/imx/ipuv3-crtc.c |   26 +++--------
 drivers/gpu/ipu-v3/ipu-di.c      |   89 ++++++++++++++++++++------------------
 include/video/imx-ipu-v3.h       |   19 ++------
 3 files changed, 56 insertions(+), 78 deletions(-)

diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
index fb16026..1ca6492 100644
--- a/drivers/gpu/drm/imx/ipuv3-crtc.c
+++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
@@ -158,35 +158,19 @@ static int ipu_crtc_mode_set(struct drm_crtc *crtc,
 
 	out_pixel_fmt = ipu_crtc->interface_pix_fmt;
 
-	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
-		sig_cfg.interlaced = 1;
-	if (mode->flags & DRM_MODE_FLAG_PHSYNC)
-		sig_cfg.Hsync_pol = 1;
-	if (mode->flags & DRM_MODE_FLAG_PVSYNC)
-		sig_cfg.Vsync_pol = 1;
-
 	sig_cfg.enable_pol = 1;
 	sig_cfg.clk_pol = 0;
-	sig_cfg.width = mode->hdisplay;
-	sig_cfg.height = mode->vdisplay;
 	sig_cfg.pixel_fmt = out_pixel_fmt;
-	sig_cfg.h_start_width = mode->htotal - mode->hsync_end;
-	sig_cfg.h_sync_width = mode->hsync_end - mode->hsync_start;
-	sig_cfg.h_end_width = mode->hsync_start - mode->hdisplay;
-
-	sig_cfg.v_start_width = mode->vtotal - mode->vsync_end;
-	sig_cfg.v_sync_width = mode->vsync_end - mode->vsync_start;
-	sig_cfg.v_end_width = mode->vsync_start - mode->vdisplay;
-	sig_cfg.pixelclock = mode->clock * 1000;
 	sig_cfg.clkflags = ipu_crtc->di_clkflags;
-
 	sig_cfg.v_to_h_sync = 0;
-
 	sig_cfg.hsync_pin = ipu_crtc->di_hsync_pin;
 	sig_cfg.vsync_pin = ipu_crtc->di_vsync_pin;
 
-	ret = ipu_dc_init_sync(ipu_crtc->dc, ipu_crtc->di, sig_cfg.interlaced,
-			out_pixel_fmt, mode->hdisplay);
+	videomode_from_drm_display_mode(mode, &sig_cfg.mode);
+
+	ret = ipu_dc_init_sync(ipu_crtc->dc, ipu_crtc->di,
+			       mode->flags & DRM_MODE_FLAG_INTERLACE,
+			       out_pixel_fmt, mode->hdisplay);
 	if (ret) {
 		dev_err(ipu_crtc->dev,
 				"initializing display controller failed with %d\n",
diff --git a/drivers/gpu/ipu-v3/ipu-di.c b/drivers/gpu/ipu-v3/ipu-di.c
index 41df8d7..d95fbd0 100644
--- a/drivers/gpu/ipu-v3/ipu-di.c
+++ b/drivers/gpu/ipu-v3/ipu-di.c
@@ -207,10 +207,10 @@ static void ipu_di_sync_config(struct ipu_di *di, struct di_sync_config *config,
 static void ipu_di_sync_config_interlaced(struct ipu_di *di,
 		struct ipu_di_signal_cfg *sig)
 {
-	u32 h_total = sig->width + sig->h_sync_width +
-		sig->h_start_width + sig->h_end_width;
-	u32 v_total = sig->height + sig->v_sync_width +
-		sig->v_start_width + sig->v_end_width;
+	u32 h_total = sig->mode.hactive + sig->mode.hsync_len +
+		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[] = {
 		{
@@ -229,13 +229,13 @@ static void ipu_di_sync_config_interlaced(struct ipu_di *di,
 		}, {
 			.run_count = v_total / 2 - 1,
 			.run_src = DI_SYNC_HSYNC,
-			.offset_count = sig->v_start_width,
+			.offset_count = sig->mode.vback_porch,
 			.offset_src = DI_SYNC_HSYNC,
 			.repeat_count = 2,
 			.cnt_clr_src = DI_SYNC_VSYNC,
 		}, {
 			.run_src = DI_SYNC_HSYNC,
-			.repeat_count = sig->height / 2,
+			.repeat_count = sig->mode.vactive / 2,
 			.cnt_clr_src = 4,
 		}, {
 			.run_count = v_total - 1,
@@ -249,9 +249,9 @@ static void ipu_di_sync_config_interlaced(struct ipu_di *di,
 			.cnt_clr_src = DI_SYNC_VSYNC,
 		}, {
 			.run_src = DI_SYNC_CLK,
-			.offset_count = sig->h_start_width,
+			.offset_count = sig->mode.hback_porch,
 			.offset_src = DI_SYNC_CLK,
-			.repeat_count = sig->width,
+			.repeat_count = sig->mode.hactive,
 			.cnt_clr_src = 5,
 		}, {
 			.run_count = v_total - 1,
@@ -277,10 +277,10 @@ static void ipu_di_sync_config_interlaced(struct ipu_di *di,
 static void ipu_di_sync_config_noninterlaced(struct ipu_di *di,
 		struct ipu_di_signal_cfg *sig, int div)
 {
-	u32 h_total = sig->width + sig->h_sync_width + sig->h_start_width +
-		sig->h_end_width;
-	u32 v_total = sig->height + sig->v_sync_width + sig->v_start_width +
-		sig->v_end_width;
+	u32 h_total = sig->mode.hactive + sig->mode.hsync_len +
+		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;
 	struct di_sync_config cfg[] = {
 		{
 			/* 1: INT_HSYNC */
@@ -294,27 +294,29 @@ static void ipu_di_sync_config_noninterlaced(struct ipu_di *di,
 			.offset_src = DI_SYNC_CLK,
 			.cnt_polarity_gen_en = 1,
 			.cnt_polarity_trigger_src = DI_SYNC_CLK,
-			.cnt_down = sig->h_sync_width * 2,
+			.cnt_down = sig->mode.hsync_len * 2,
 		} , {
 			/* PIN3: VSYNC */
 			.run_count = v_total - 1,
 			.run_src = DI_SYNC_INT_HSYNC,
 			.cnt_polarity_gen_en = 1,
 			.cnt_polarity_trigger_src = DI_SYNC_INT_HSYNC,
-			.cnt_down = sig->v_sync_width * 2,
+			.cnt_down = sig->mode.vsync_len * 2,
 		} , {
 			/* 4: Line Active */
 			.run_src = DI_SYNC_HSYNC,
-			.offset_count = sig->v_sync_width + sig->v_start_width,
+			.offset_count = sig->mode.vsync_len +
+					sig->mode.vback_porch,
 			.offset_src = DI_SYNC_HSYNC,
-			.repeat_count = sig->height,
+			.repeat_count = sig->mode.vactive,
 			.cnt_clr_src = DI_SYNC_VSYNC,
 		} , {
 			/* 5: Pixel Active, referenced by DC */
 			.run_src = DI_SYNC_CLK,
-			.offset_count = sig->h_sync_width + sig->h_start_width,
+			.offset_count = sig->mode.hsync_len +
+					sig->mode.hback_porch,
 			.offset_src = DI_SYNC_CLK,
-			.repeat_count = sig->width,
+			.repeat_count = sig->mode.hactive,
 			.cnt_clr_src = 5, /* Line Active */
 		} , {
 			/* unused */
@@ -339,9 +341,10 @@ static void ipu_di_sync_config_noninterlaced(struct ipu_di *di,
 		} , {
 			/* 3: Line Active */
 			.run_src = DI_SYNC_INT_HSYNC,
-			.offset_count = sig->v_sync_width + sig->v_start_width,
+			.offset_count = sig->mode.vsync_len +
+					sig->mode.vback_porch,
 			.offset_src = DI_SYNC_INT_HSYNC,
-			.repeat_count = sig->height,
+			.repeat_count = sig->mode.vactive,
 			.cnt_clr_src = 3 /* VSYNC */,
 		} , {
 			/* PIN4: HSYNC for VGA via TVEv2 on TQ MBa53 */
@@ -351,13 +354,14 @@ static void ipu_di_sync_config_noninterlaced(struct ipu_di *di,
 			.offset_src = DI_SYNC_CLK,
 			.cnt_polarity_gen_en = 1,
 			.cnt_polarity_trigger_src = DI_SYNC_CLK,
-			.cnt_down = sig->h_sync_width * 2,
+			.cnt_down = sig->mode.hsync_len * 2,
 		} , {
 			/* 5: Pixel Active signal to DC */
 			.run_src = DI_SYNC_CLK,
-			.offset_count = sig->h_sync_width + sig->h_start_width,
+			.offset_count = sig->mode.hsync_len +
+					sig->mode.hback_porch,
 			.offset_src = DI_SYNC_CLK,
-			.repeat_count = sig->width,
+			.repeat_count = sig->mode.hactive,
 			.cnt_clr_src = 4, /* Line Active */
 		} , {
 			/* PIN6: VSYNC for VGA via TVEv2 on TQ MBa53 */
@@ -367,7 +371,7 @@ static void ipu_di_sync_config_noninterlaced(struct ipu_di *di,
 			.offset_src = DI_SYNC_INT_HSYNC,
 			.cnt_polarity_gen_en = 1,
 			.cnt_polarity_trigger_src = DI_SYNC_INT_HSYNC,
-			.cnt_down = sig->v_sync_width * 2,
+			.cnt_down = sig->mode.vsync_len * 2,
 		} , {
 			/* PIN4: HSYNC for VGA via TVEv2 on i.MX53-QSB */
 			.run_count = h_total - 1,
@@ -376,7 +380,7 @@ static void ipu_di_sync_config_noninterlaced(struct ipu_di *di,
 			.offset_src = DI_SYNC_CLK,
 			.cnt_polarity_gen_en = 1,
 			.cnt_polarity_trigger_src = DI_SYNC_CLK,
-			.cnt_down = sig->h_sync_width * 2,
+			.cnt_down = sig->mode.hsync_len * 2,
 		} , {
 			/* PIN6: VSYNC for VGA via TVEv2 on i.MX53-QSB */
 			.run_count = v_total - 1,
@@ -385,7 +389,7 @@ static void ipu_di_sync_config_noninterlaced(struct ipu_di *di,
 			.offset_src = DI_SYNC_INT_HSYNC,
 			.cnt_polarity_gen_en = 1,
 			.cnt_polarity_trigger_src = DI_SYNC_INT_HSYNC,
-			.cnt_down = sig->v_sync_width * 2,
+			.cnt_down = sig->mode.vsync_len * 2,
 		} , {
 			/* unused */
 		},
@@ -433,10 +437,11 @@ static void ipu_di_config_clock(struct ipu_di *di,
 			unsigned long in_rate;
 			unsigned div;
 
-			clk_set_rate(clk, sig->pixelclock);
+			clk_set_rate(clk, sig->mode.pixelclock);
 
 			in_rate = clk_get_rate(clk);
-			div = (in_rate + sig->pixelclock / 2) / sig->pixelclock;
+			div = (in_rate + sig->mode.pixelclock / 2) /
+				sig->mode.pixelclock;
 			if (div = 0)
 				div = 1;
 
@@ -454,10 +459,11 @@ static void ipu_di_config_clock(struct ipu_di *di,
 		unsigned div, error;
 
 		clkrate = clk_get_rate(di->clk_ipu);
-		div = (clkrate + sig->pixelclock / 2) / sig->pixelclock;
+		div = (clkrate + sig->mode.pixelclock / 2) /
+			sig->mode.pixelclock;
 		rate = clkrate / div;
 
-		error = rate / (sig->pixelclock / 1000);
+		error = rate / (sig->mode.pixelclock / 1000);
 
 		dev_dbg(di->ipu->dev, "  IPU clock can give %lu with divider %u, error %d.%u%%\n",
 			rate, div, (signed)(error - 1000) / 10, error % 10);
@@ -473,10 +479,11 @@ static void ipu_di_config_clock(struct ipu_di *di,
 
 			clk = di->clk_di;
 
-			clk_set_rate(clk, sig->pixelclock);
+			clk_set_rate(clk, sig->mode.pixelclock);
 
 			in_rate = clk_get_rate(clk);
-			div = (in_rate + sig->pixelclock / 2) / sig->pixelclock;
+			div = (in_rate + sig->mode.pixelclock / 2) /
+				sig->mode.pixelclock;
 			if (div = 0)
 				div = 1;
 
@@ -504,7 +511,7 @@ static void ipu_di_config_clock(struct ipu_di *di,
 	ipu_di_write(di, val, DI_GENERAL);
 
 	dev_dbg(di->ipu->dev, "Want %luHz IPU %luHz DI %luHz using %s, %luHz\n",
-		sig->pixelclock,
+		sig->mode.pixelclock,
 		clk_get_rate(di->clk_ipu),
 		clk_get_rate(di->clk_di),
 		clk = di->clk_di ? "DI" : "IPU",
@@ -547,15 +554,15 @@ int ipu_di_init_sync_panel(struct ipu_di *di, struct ipu_di_signal_cfg *sig)
 	u32 div;
 
 	dev_dbg(di->ipu->dev, "disp %d: panel size = %d x %d\n",
-		di->id, sig->width, sig->height);
+		di->id, sig->mode.hactive, sig->mode.vactive);
 
-	if ((sig->v_sync_width = 0) || (sig->h_sync_width = 0))
+	if ((sig->mode.vsync_len = 0) || (sig->mode.hsync_len = 0))
 		return -EINVAL;
 
 	dev_dbg(di->ipu->dev, "Clocks: IPU %luHz DI %luHz Needed %luHz\n",
 		clk_get_rate(di->clk_ipu),
 		clk_get_rate(di->clk_di),
-		sig->pixelclock);
+		sig->mode.pixelclock);
 
 	mutex_lock(&di_mutex);
 
@@ -574,7 +581,7 @@ int ipu_di_init_sync_panel(struct ipu_di *di, struct ipu_di_signal_cfg *sig)
 	di_gen = ipu_di_read(di, DI_GENERAL) & DI_GEN_DI_CLK_EXT;
 	di_gen |= DI_GEN_DI_VSYNC_EXT;
 
-	if (sig->interlaced) {
+	if (sig->mode.flags & DISPLAY_FLAGS_INTERLACED) {
 		ipu_di_sync_config_interlaced(di, sig);
 
 		/* set y_sel = 1 */
@@ -584,9 +591,9 @@ int ipu_di_init_sync_panel(struct ipu_di *di, struct ipu_di_signal_cfg *sig)
 
 		vsync_cnt = 7;
 
-		if (sig->Hsync_pol)
+		if (sig->mode.flags & DISPLAY_FLAGS_HSYNC_HIGH)
 			di_gen |= DI_GEN_POLARITY_3;
-		if (sig->Vsync_pol)
+		if (sig->mode.flags & DISPLAY_FLAGS_VSYNC_HIGH)
 			di_gen |= DI_GEN_POLARITY_2;
 	} else {
 		ipu_di_sync_config_noninterlaced(di, sig, div);
@@ -600,7 +607,7 @@ int ipu_di_init_sync_panel(struct ipu_di *di, struct ipu_di_signal_cfg *sig)
 			if (!(sig->hsync_pin = 2 && sig->vsync_pin = 3))
 				vsync_cnt = 6;
 
-		if (sig->Hsync_pol) {
+		if (sig->mode.flags & DISPLAY_FLAGS_HSYNC_HIGH) {
 			if (sig->hsync_pin = 2)
 				di_gen |= DI_GEN_POLARITY_2;
 			else if (sig->hsync_pin = 4)
@@ -608,7 +615,7 @@ int ipu_di_init_sync_panel(struct ipu_di *di, struct ipu_di_signal_cfg *sig)
 			else if (sig->hsync_pin = 7)
 				di_gen |= DI_GEN_POLARITY_7;
 		}
-		if (sig->Vsync_pol) {
+		if (sig->mode.flags & DISPLAY_FLAGS_VSYNC_HIGH) {
 			if (sig->vsync_pin = 3)
 				di_gen |= DI_GEN_POLARITY_3;
 			else if (sig->vsync_pin = 6)
diff --git a/include/video/imx-ipu-v3.h b/include/video/imx-ipu-v3.h
index d333d54..73390c1 100644
--- a/include/video/imx-ipu-v3.h
+++ b/include/video/imx-ipu-v3.h
@@ -33,28 +33,15 @@ enum ipuv3_type {
  * Bitfield of Display Interface signal polarities.
  */
 struct ipu_di_signal_cfg {
-	unsigned datamask_en:1;
-	unsigned interlaced:1;
-	unsigned odd_field_first:1;
-	unsigned clksel_en:1;
-	unsigned clkidle_en:1;
 	unsigned data_pol:1;	/* true = inverted */
 	unsigned clk_pol:1;	/* true = rising edge */
 	unsigned enable_pol:1;
-	unsigned Hsync_pol:1;	/* true = active high */
-	unsigned Vsync_pol:1;
 
-	u16 width;
-	u16 height;
+	struct videomode mode;
+
 	u32 pixel_fmt;
-	u16 h_start_width;
-	u16 h_sync_width;
-	u16 h_end_width;
-	u16 v_start_width;
-	u16 v_sync_width;
-	u16 v_end_width;
 	u32 v_to_h_sync;
-	unsigned long pixelclock;
+
 #define IPU_DI_CLKMODE_SYNC	(1 << 0)
 #define IPU_DI_CLKMODE_EXT	(1 << 1)
 	unsigned long clkflags;
-- 
1.7.9.5

[PATCH v2 7/7] gpu: ipu-di: Switch to DIV_ROUND_CLOSEST for DI clock divider calc

[Steve Longerbeam]

We can use the DIV_ROUND_CLOSEST() macro when calculating the DI
clock divider, rounded to nearest int.

Suggested-by: Philipp Zabel <p.zabel@pengutronix.de>
Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
 drivers/gpu/ipu-v3/ipu-di.c |    9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/ipu-v3/ipu-di.c b/drivers/gpu/ipu-v3/ipu-di.c
index d95fbd0..b61d6be 100644
--- a/drivers/gpu/ipu-v3/ipu-di.c
+++ b/drivers/gpu/ipu-v3/ipu-di.c
@@ -440,8 +440,7 @@ static void ipu_di_config_clock(struct ipu_di *di,
 			clk_set_rate(clk, sig->mode.pixelclock);
 
 			in_rate = clk_get_rate(clk);
-			div = (in_rate + sig->mode.pixelclock / 2) /
-				sig->mode.pixelclock;
+			div = DIV_ROUND_CLOSEST(in_rate, sig->mode.pixelclock);
 			if (div = 0)
 				div = 1;
 
@@ -459,8 +458,7 @@ static void ipu_di_config_clock(struct ipu_di *di,
 		unsigned div, error;
 
 		clkrate = clk_get_rate(di->clk_ipu);
-		div = (clkrate + sig->mode.pixelclock / 2) /
-			sig->mode.pixelclock;
+		div = DIV_ROUND_CLOSEST(clkrate, sig->mode.pixelclock);
 		rate = clkrate / div;
 
 		error = rate / (sig->mode.pixelclock / 1000);
@@ -482,8 +480,7 @@ static void ipu_di_config_clock(struct ipu_di *di,
 			clk_set_rate(clk, sig->mode.pixelclock);
 
 			in_rate = clk_get_rate(clk);
-			div = (in_rate + sig->mode.pixelclock / 2) /
-				sig->mode.pixelclock;
+			div = DIV_ROUND_CLOSEST(in_rate, sig->mode.pixelclock);
 			if (div = 0)
 				div = 1;
 
-- 
1.7.9.5

Re: [PATCH v2 0/7] imx-drm: ipuv3-crtc: Implement mode_fixup

[Philipp Zabel]

Am Donnerstag, den 18.12.2014, 18:00 -0800 schrieb Steve Longerbeam:
> This patchset implements ->mode_fixup() in the imx ipuv3-crtc driver,
> using a new support function ipu_di_adjust_videomode(). This new
> function needs to be subsystem independent, so it accepts a video
> mode as a 'struct videomode'. Hence ipu-crtc ->mode_fixup() needs
> another support function to convert a drm_display_mode to a videomode
> before passing the mode to ipu_di_adjust_videomode() for fixup.
> 
> Also some related code cleanup: 'struct ipu_di_signal_cfg' should
> use 'struct videomode' for mode timings.

Alright, I have applied the series with
s/videomode_from_drm_display_mode/drm_display_mode_to_videomode/

thanks
Philipp

Re: [PATCH v2 0/7] imx-drm: ipuv3-crtc: Implement mode_fixup

[Liu Ying]

Hi,

It looks that the below commit makes my Hannstar XGA LVDS panel stop working
on the i.MX6DL SabreSD board.  Any idea?

commit eb10d6355532def3a74aaabd115e2373cca70b9d
Author: Steve Longerbeam <slongerbeam@gmail.com>
Date:   Thu Dec 18 18:00:24 2014 -0800

imx-drm: encoder prepare/mode_set must use adjusted mode

The encoder ->prepare() and ->mode_set() methods need to use the
hw adjusted mode, not the original mode.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>

Regards,
Liu Ying

On Wed, Jan 07, 2015 at 07:27:28PM +0100, Philipp Zabel wrote:
> Am Donnerstag, den 18.12.2014, 18:00 -0800 schrieb Steve Longerbeam:
> > This patchset implements ->mode_fixup() in the imx ipuv3-crtc driver,
> > using a new support function ipu_di_adjust_videomode(). This new
> > function needs to be subsystem independent, so it accepts a video
> > mode as a 'struct videomode'. Hence ipu-crtc ->mode_fixup() needs
> > another support function to convert a drm_display_mode to a videomode
> > before passing the mode to ipu_di_adjust_videomode() for fixup.
> > 
> > Also some related code cleanup: 'struct ipu_di_signal_cfg' should
> > use 'struct videomode' for mode timings.
> 
> Alright, I have applied the series with
> s/videomode_from_drm_display_mode/drm_display_mode_to_videomode/
> 
> thanks
> Philipp
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/7] imx-drm: ipuv3-crtc: Implement mode_fixup

[Philipp Zabel]

Hi Liu,

Am Freitag, den 23.01.2015, 10:56 +0800 schrieb Liu Ying:
> Hi,
> 
> It looks that the below commit makes my Hannstar XGA LVDS panel stop working
> on the i.MX6DL SabreSD board.  Any idea?
> 
> commit eb10d6355532def3a74aaabd115e2373cca70b9d
> Author: Steve Longerbeam <slongerbeam@gmail.com>
> Date:   Thu Dec 18 18:00:24 2014 -0800
> 
> imx-drm: encoder prepare/mode_set must use adjusted mode
> 
> The encoder ->prepare() and ->mode_set() methods need to use the
> hw adjusted mode, not the original mode.
> 
> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> 
> Regards,
> Liu Ying

What are this panel timings? The adjustment should increase the vertical
back porch by up to two lines (so it is at least two lines), reducing
the front porch or vsync length by the same amount. Does this panel use
the HSYNC/VSYNC signals embedded in the LVDS stream?

regards
Philipp

Re: [PATCH v2 0/7] imx-drm: ipuv3-crtc: Implement mode_fixup

[Fabio Estevam]

Hi Philipp,

On Fri, Jan 23, 2015 at 8:50 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:

> What are this panel timings? The adjustment should increase the vertical
> back porch by up to two lines (so it is at least two lines), reducing
> the front porch or vsync length by the same amount. Does this panel use
> the HSYNC/VSYNC signals embedded in the LVDS stream?

The panel on this board is a Hannstar HSD100PXN1. The timings we use
are described in imx6qdl-sabresd.dtsi:

        display-timings {
            native-mode = <&timing0>;
            timing0: hsd100pxn1 {
                clock-frequency = <65000000>;
                hactive = <1024>;
                vactive = <768>;
                hback-porch = <220>;
                hfront-porch = <40>;
                vback-porch = <21>;
                vfront-porch = <7>;
                hsync-len = <60>;
                vsync-len = <10>;
            };

Re: [PATCH v2 0/7] imx-drm: ipuv3-crtc: Implement mode_fixup

[Fabio Estevam]

On Fri, Jan 23, 2015 at 12:56 AM, Liu Ying <Ying.Liu@freescale.com> wrote:
> Hi,
>
> It looks that the below commit makes my Hannstar XGA LVDS panel stop working
> on the i.MX6DL SabreSD board.  Any idea?

Yes, with eb10d6355532def3a ("mx-drm: encoder prepare/mode_set must
use adjusted mode") applied
the DI clock is 0:

--- a/drivers/gpu/drm/imx/imx-ldb.c
+++ b/drivers/gpu/drm/imx/imx-ldb.c
@@ -169,6 +169,8 @@ static void imx_ldb_encoder_prepare(struct
drm_encoder *encoder)
        unsigned long di_clk = mode->clock * 1000;
        int mux = imx_drm_encoder_get_mux_id(imx_ldb_ch->child, encoder);

+       pr_err("********* DI clock is %ld\n", di_clk);
+
        if (ldb->ldb_ctrl & LDB_SPLIT_MODE_EN) {

With eb10d6355532def3a applied:

[    1.493745] ********* DI clock is 0

With eb10d6355532def3a reverted:

[    1.493639] ********* DI clock is 65000000

Should we just go back to the previous usage?

--- a/drivers/gpu/drm/imx/imx-ldb.c
+++ b/drivers/gpu/drm/imx/imx-ldb.c
@@ -163,12 +163,14 @@ static void imx_ldb_encoder_prepare(struct
drm_encoder *encoder)
 {
        struct imx_ldb_channel *imx_ldb_ch = enc_to_imx_ldb_ch(encoder);
        struct imx_ldb *ldb = imx_ldb_ch->ldb;
-       struct drm_display_mode *mode = &encoder->crtc->hwmode;
+       struct drm_display_mode *mode = &encoder->crtc->mode;
        u32 pixel_fmt;

Re: [PATCH v2 0/7] imx-drm: ipuv3-crtc: Implement mode_fixup

[Philipp Zabel]

Am Freitag, den 23.01.2015, 13:06 -0200 schrieb Fabio Estevam:
> On Fri, Jan 23, 2015 at 12:56 AM, Liu Ying <Ying.Liu@freescale.com> wrote:
> > Hi,
> >
> > It looks that the below commit makes my Hannstar XGA LVDS panel stop working
> > on the i.MX6DL SabreSD board.  Any idea?
> 
> Yes, with eb10d6355532def3a ("mx-drm: encoder prepare/mode_set must
> use adjusted mode") applied
> the DI clock is 0:

My bad, the problem is we are misusing encoder_prepare to enable the
display interface clock needed for the following crtc mode set.

What we really want is to use is adjusted_mode given to
encoder_funcs->mode_set, before the clock is enabled by
crtc_funcs->commit.

How about this patch:

-----8<-----
diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
index abceb3d..99fe4bb 100644
--- a/drivers/gpu/drm/imx/imx-ldb.c
+++ b/drivers/gpu/drm/imx/imx-ldb.c
@@ -193,22 +193,8 @@ static void imx_ldb_encoder_prepare(struct drm_encoder *encoder)
 {
 	struct imx_ldb_channel *imx_ldb_ch = enc_to_imx_ldb_ch(encoder);
 	struct imx_ldb *ldb = imx_ldb_ch->ldb;
-	struct drm_display_mode *mode = &encoder->crtc->hwmode;
 	u32 pixel_fmt;
-	unsigned long serial_clk;
-	unsigned long di_clk = mode->clock * 1000;
-	int mux = imx_ldb_get_mux_id(imx_ldb_ch);
 
-	if (ldb->ldb_ctrl & LDB_SPLIT_MODE_EN) {
-		/* dual channel LVDS mode */
-		serial_clk = 3500UL * mode->clock;
-		imx_ldb_set_clock(ldb, mux, 0, serial_clk, di_clk);
-		imx_ldb_set_clock(ldb, mux, 1, serial_clk, di_clk);
-	} else {
-		serial_clk = 7000UL * mode->clock;
-		imx_ldb_set_clock(ldb, mux, imx_ldb_ch->chno, serial_clk,
-				di_clk);
-	}
 
 	switch (imx_ldb_ch->chno) {
 	case 0:
@@ -281,6 +267,9 @@ static void imx_ldb_encoder_mode_set(struct drm_encoder *encoder,
 	struct imx_ldb_channel *imx_ldb_ch = enc_to_imx_ldb_ch(encoder);
 	struct imx_ldb *ldb = imx_ldb_ch->ldb;
 	int dual = ldb->ldb_ctrl & LDB_SPLIT_MODE_EN;
+	unsigned long serial_clk;
+	unsigned long di_clk = mode->clock * 1000;
+	int mux = imx_ldb_get_mux_id(imx_ldb_ch);
 
 	if (mode->clock > 170000) {
 		dev_warn(ldb->dev,
@@ -291,6 +280,16 @@ static void imx_ldb_encoder_mode_set(struct drm_encoder *encoder,
 			 "%s: mode exceeds 85 MHz pixel clock\n", __func__);
 	}
 
+	if (dual) {
+		serial_clk = 3500UL * mode->clock;
+		imx_ldb_set_clock(ldb, mux, 0, serial_clk, di_clk);
+		imx_ldb_set_clock(ldb, mux, 1, serial_clk, di_clk);
+	} else {
+		serial_clk = 7000UL * mode->clock;
+		imx_ldb_set_clock(ldb, mux, imx_ldb_ch->chno, serial_clk,
+				  di_clk);
+	}
+
 	/* FIXME - assumes straight connections DI0 --> CH0, DI1 --> CH1 */
 	if (imx_ldb_ch = &ldb->channel[0]) {
 		if (mode->flags & DRM_MODE_FLAG_NVSYNC)
-- 
2.1.4
----->8-----

regards
Philipp

Re: [PATCH v2 0/7] imx-drm: ipuv3-crtc: Implement mode_fixup

[Fabio Estevam]

Hi Philipp,

On Fri, Jan 23, 2015 at 2:18 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> @@ -281,6 +267,9 @@ static void imx_ldb_encoder_mode_set(struct drm_encoder *encoder,
>         struct imx_ldb_channel *imx_ldb_ch = enc_to_imx_ldb_ch(encoder);
>         struct imx_ldb *ldb = imx_ldb_ch->ldb;
>         int dual = ldb->ldb_ctrl & LDB_SPLIT_MODE_EN;
> +       unsigned long serial_clk;
> +       unsigned long di_clk = mode->clock * 1000;
> +       int mux = imx_ldb_get_mux_id(imx_ldb_ch);

I can't find imx_ldb_get_mux_id() on linux-next.

Re: [PATCH v2 0/7] imx-drm: ipuv3-crtc: Implement mode_fixup

[Philipp Zabel]

Am Freitag, den 23.01.2015, 14:27 -0200 schrieb Fabio Estevam:
> Hi Philipp,
> 
> On Fri, Jan 23, 2015 at 2:18 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > @@ -281,6 +267,9 @@ static void imx_ldb_encoder_mode_set(struct drm_encoder *encoder,
> >         struct imx_ldb_channel *imx_ldb_ch = enc_to_imx_ldb_ch(encoder);
> >         struct imx_ldb *ldb = imx_ldb_ch->ldb;
> >         int dual = ldb->ldb_ctrl & LDB_SPLIT_MODE_EN;
> > +       unsigned long serial_clk;
> > +       unsigned long di_clk = mode->clock * 1000;
> > +       int mux = imx_ldb_get_mux_id(imx_ldb_ch);
> 
> I can't find imx_ldb_get_mux_id() on linux-next.

Sorry, that should be

	int mux = imx_drm_encoder_get_mux_id(imx_ldb_ch->child, encoder);

regards
Philipp

Re: [PATCH v2 0/7] imx-drm: ipuv3-crtc: Implement mode_fixup

[Fabio Estevam]

On Fri, Jan 23, 2015 at 2:39 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> Am Freitag, den 23.01.2015, 14:27 -0200 schrieb Fabio Estevam:
>> Hi Philipp,
>>
>> On Fri, Jan 23, 2015 at 2:18 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
>> > @@ -281,6 +267,9 @@ static void imx_ldb_encoder_mode_set(struct drm_encoder *encoder,
>> >         struct imx_ldb_channel *imx_ldb_ch = enc_to_imx_ldb_ch(encoder);
>> >         struct imx_ldb *ldb = imx_ldb_ch->ldb;
>> >         int dual = ldb->ldb_ctrl & LDB_SPLIT_MODE_EN;
>> > +       unsigned long serial_clk;
>> > +       unsigned long di_clk = mode->clock * 1000;
>> > +       int mux = imx_ldb_get_mux_id(imx_ldb_ch);
>>
>> I can't find imx_ldb_get_mux_id() on linux-next.
>
> Sorry, that should be
>
>         int mux = imx_drm_encoder_get_mux_id(imx_ldb_ch->child, encoder);

It works fine now, thanks:

Tested-by: Fabio Estevam <fabio.estevam@freescale.com>