[PATCH 0/3] drm/sun4i: dsi: misc timing fixes

[Icenowy Zheng]

This patchset fixes some portion of timing calculation in sun6i_mipi_dsi
driver according to the BSP driver.

Two of the patches are reverting, one is fixing some misread of the BSP
source code, another is fixing a wrong refactor that actually breaks the
formula.

The other non-reverting patch is fixing a porch error which is usually
seen in the original driver commit. Most of porch errors are then fixed,
but this one gets ignored.

By applying these patches, several DSI panels are tested to be driven
properly by the timing provided by the vendor, including the LCD panel
of PinePhone "Don't Be Evil" DevKit, the final PinePhone panel and the
panel on PineTab. Without these patches they need dirty timing hacks to
work.

Icenowy Zheng (3):
Revert "drm/sun4i: dsi: Change the start delay calculation"
drm/sun4i: dsi: fix DRQ calculation
Revert "drm/sun4i: dsi: Rework a bit the hblk calculation"

drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 16 +++++-----------
1 file changed, 5 insertions(+), 11 deletions(-)

--
2.21.0

[Icenowy Zheng]

This reverts commit da676c6aa6413d59ab0a80c97bbc273025e640b2.

The original commit adds a start parameter to the calculation of the
start delay according to some old BSP versions from Allwinner. However,
there're two ways to add this delay -- add it in DSI controller or add
it in the TCON. Add it in both controllers won't work.

The code before this commit is picked from new versions of BSP kernel,
which has a comment for the 1 that says "put start_delay to tcon". By
checking the sun4i_tcon0_mode_set_cpu() in sun4i_tcon driver, it has
already added this delay, so we shouldn't repeat to add the delay in DSI
controller, otherwise the timing won't match.

Signed-off-by: Icenowy Zheng <ice...@aosc.io>
---
drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
index 1636344ba9ec..c86e11631ebc 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
@@ -365,8 +365,7 @@ static void sun6i_dsi_inst_init(struct sun6i_dsi *dsi,
static u16 sun6i_dsi_get_video_start_delay(struct sun6i_dsi *dsi,
struct drm_display_mode *mode)
{
- u16 start = clamp(mode->vtotal - mode->vdisplay - 10, 8, 100);
- u16 delay = mode->vtotal - (mode->vsync_end - mode->vdisplay) + start;
+ u16 delay = mode->vtotal - (mode->vsync_end - mode->vdisplay) + 1;

if (delay > mode->vtotal)
delay = delay % mode->vtotal;
--
2.21.0

[Icenowy Zheng]

According to the BSP source code, when calculating the magic DRQ value
outside burst mode, a formula of "lcd_ht - lcd_x - lcd_hbp", which is
interpreted as right margin (HFP value). However, currently the
sun6i_mipi_dsi driver code calculates it wrongly as HFP + sync length,
which lead to timing error.

Fix the DRQ calculation by change it to use HFP.

Signed-off-by: Icenowy Zheng <ice...@aosc.io>
---

drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
index c86e11631ebc..2d3e822a7739 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
@@ -436,9 +436,9 @@ static void sun6i_dsi_setup_burst(struct sun6i_dsi *dsi,
SUN6I_DSI_BURST_LINE_SYNC_POINT(SUN6I_DSI_SYNC_POINT));

val = SUN6I_DSI_TCON_DRQ_ENABLE_MODE;
- } else if ((mode->hsync_end - mode->hdisplay) > 20) {
+ } else if ((mode->hsync_start - mode->hdisplay) > 20) {
/* Maaaaaagic */
- u16 drq = (mode->hsync_end - mode->hdisplay) - 20;
+ u16 drq = (mode->hsync_start - mode->hdisplay) - 20;

drq *= mipi_dsi_pixel_format_to_bpp(device->format);
drq /= 32;
--
2.21.0

[Icenowy Zheng]

This reverts commit 62e7511a4f4dcf07f753893d3424decd9466c98b.

This commit, although claimed as a refactor, in fact changed the
formula.

By expanding the original formula, we can find that the const 10 is not
substracted, instead it's added to the value (because 10 is negative
when calculating hsa, and hsa itself is negative when calculating hblk).
This breaks the similar pattern to other formulas, so restoring the
original formula is more proper.

Signed-off-by: Icenowy Zheng <ice...@aosc.io>
---

drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
index 2d3e822a7739..cb5fd19c0d0d 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
@@ -577,14 +577,9 @@ static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi,
(mode->hsync_start - mode->hdisplay) * Bpp - HFP_PACKET_OVERHEAD);

/*
- * The blanking is set using a sync event (4 bytes)
- * and a blanking packet (4 bytes + payload + 2
- * bytes). Its minimal size is therefore 10 bytes.
+ * hblk seems to be the line + porches length.
*/
-#define HBLK_PACKET_OVERHEAD 10
- hblk = max((unsigned int)HBLK_PACKET_OVERHEAD,
- (mode->htotal - (mode->hsync_end - mode->hsync_start)) * Bpp -
- HBLK_PACKET_OVERHEAD);
+ hblk = mode->htotal * Bpp - hsa;

/*
* And I'm not entirely sure what vblk is about. The driver in
--
2.21.0

[Icenowy Zheng]

在 2019-10-02三的 12:36 +0200，Maxime Ripard写道：
> Hi,

>
> On Tue, Oct 01, 2019 at 04:02:50PM +0800, Icenowy Zheng wrote:
> > This patchset fixes some portion of timing calculation in
> > sun6i_mipi_dsi
> > driver according to the BSP driver.
> >
> > Two of the patches are reverting, one is fixing some misread of the
> > BSP
> > source code, another is fixing a wrong refactor that actually
> > breaks the
> > formula.
> >
> > The other non-reverting patch is fixing a porch error which is
> > usually
> > seen in the original driver commit. Most of porch errors are then
> > fixed,
> > but this one gets ignored.
> >
> > By applying these patches, several DSI panels are tested to be
> > driven
> > properly by the timing provided by the vendor, including the LCD
> > panel
> > of PinePhone "Don't Be Evil" DevKit, the final PinePhone panel and
> > the
> > panel on PineTab. Without these patches they need dirty timing
> > hacks to
> > work.
>

> Thanks for going after that issue. Can you provide references to the
> BSP on the various patches?

For patch 1: [1] for setting delay 1 in DSI controller, [2] for setting
real delay in TCON controller.

For patch 2: [3]

Patch 3 is reverting a breaking change, so I didn't check it in the
BSP. It can be verified by mathmatical calculation.

[1]
https://github.com/ayufan-pine64/linux-pine64/blob/my-hacks-1.2-with-drm/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c#L730

[2]
https://github.com/ayufan-pine64/linux-pine64/blob/my-hacks-1.2-with-drm/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_lcd.c#L369

[3]
https://github.com/ayufan-pine64/linux-pine64/blob/my-hacks-1.2-with-drm/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c#L780

>
> Ideally, having the panel drivers, and the panel datasheet would
> help.
>
> Thanks!
> Maxime
>
> PS: where can we get one of those devices?

[Jagan Teki]

Hi Wens,

The original formula is correct according to BSP [1] and work with my
panels which I have tested before. May be the horizontal timings on
panels you have leads to negative value.

[1] https://github.com/ayufan-pine64/linux-pine64/blob/my-hacks-1.2-with-drm/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c#L919

[Jagan Teki]

Hi,

On Tue, Oct 1, 2019 at 1:34 PM Icenowy Zheng <ice...@aosc.io> wrote:
>

I have similar patch in the ML, which required commit details
commented by Chen-Yu [1]. So, I'm trying to send it accordingly, let
me know if you have issues.

[1] https://patchwork.freedesktop.org/patch/305920/

[Jagan Teki]

On Tue, Oct 1, 2019 at 1:33 PM Icenowy Zheng <ice...@aosc.io> wrote:
>
> This reverts commit da676c6aa6413d59ab0a80c97bbc273025e640b2.
>
> The original commit adds a start parameter to the calculation of the
> start delay according to some old BSP versions from Allwinner. However,
> there're two ways to add this delay -- add it in DSI controller or add
> it in the TCON. Add it in both controllers won't work.
>
> The code before this commit is picked from new versions of BSP kernel,
> which has a comment for the 1 that says "put start_delay to tcon". By
> checking the sun4i_tcon0_mode_set_cpu() in sun4i_tcon driver, it has
> already added this delay, so we shouldn't repeat to add the delay in DSI
> controller, otherwise the timing won't match.

Thanks for this change. look like this is proper reason for adding +
1. also adding bsp code links here might help for future reference.

Otherwise,

Reviewed-by: Jagan Teki <ja...@amarulasolutions.com>

[Icenowy Zheng]

于 2019年10月3日 GMT+08:00 下午9:19:16, Maxime Ripard <mri...@kernel.org> 写到:

>The commit log was better in this one. I ended up merging this one,
>with your R-b.

Please note that Jagan's v11 3/7 is still needed.

>
>Maxime

--
使用 K-9 Mail 发送自我的Android设备。

[Icenowy Zheng]

在 2019-10-03四的 09:53 +0530，Jagan Teki写道：

Do you tested the same timing with BSP kernel?

It's quite difficult to get a negative value here, because the value is
quite big (includes mode->hdisplay * Bpp).

Strangely, only change the formula here back makes the timing
translated from FEX file works (tested on PineTab and PinePhone
production ver). The translation rule is from [1].

So I still insist on the patch because it's needed by experiment.

[1] http://linux-sunxi.org/LCD

>
> [1]
> https://github.com/ayufan-pine64/linux-pine64/blob/my-hacks-1.2-with-drm/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c#L919

[Icenowy Zheng]

在 2019-10-06日的 22:44 +0800，Icenowy Zheng写道：

By re-checking with the BSP source code, I found that the constant in
the HFP formula is indeed wrong -- it should be 16, not 6.

[Ondřej Jirman]

HI Icenowy,

I'm not sure if it's relevant to the discussion, but I've recently found
a LCD configuration manual for A10, that may contain some useful info:

See this: https://github.com/pocketbook/Platform_A13/blob/master/Kernel/drivers/video/sun5i/lcd/a10_lcd_config_nanual_v1.0.pdf

regards,
o.

> >
> > Strangely, only change the formula here back makes the timing
> > translated from FEX file works (tested on PineTab and PinePhone
> > production ver). The translation rule is from [1].
> >
> > So I still insist on the patch because it's needed by experiment.
> >
> > [1] http://linux-sunxi.org/LCD
> >
> > > [1]
> > > https://github.com/ayufan-pine64/linux-pine64/blob/my-hacks-1.2-with-drm/drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c#L919
>

> --
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi...@googlegroups.com.
> To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/14da3ae768c439e387f6609553bd465e945d4a33.camel%40aosc.io.