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

[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

[PATCH 1/3] Revert "drm/sun4i: dsi: Change the start delay calculation"

[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 <icenowy@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

Re: [linux-sunxi] [PATCH 1/3] Revert "drm/sun4i: dsi: Change the start delay calculation"

[Jagan Teki]

On Tue, Oct 1, 2019 at 1:33 PM Icenowy Zheng <icenowy@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 <jagan@amarulasolutions.com>

Re: [linux-sunxi] [PATCH 1/3] Revert "drm/sun4i: dsi: Change the start delay calculation"

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1149 bytes --]

On Thu, Oct 03, 2019 at 12:38:43PM +0530, Jagan Teki wrote:
> On Tue, Oct 1, 2019 at 1:33 PM Icenowy Zheng <icenowy@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 <jagan@amarulasolutions.com>

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

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [linux-sunxi] [PATCH 1/3] Revert "drm/sun4i: dsi: Change the start delay calculation"

[Icenowy Zheng]

于 2019年10月3日 GMT+08:00 下午9:19:16, Maxime Ripard <mripard@kernel.org> 写到:
>On Thu, Oct 03, 2019 at 12:38:43PM +0530, Jagan Teki wrote:
>> On Tue, Oct 1, 2019 at 1:33 PM Icenowy Zheng <icenowy@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 <jagan@amarulasolutions.com>
>
>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设备。

Re: [linux-sunxi] [PATCH 1/3] Revert "drm/sun4i: dsi: Change the start delay calculation"

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1600 bytes --]

On Thu, Oct 03, 2019 at 09:21:30PM +0800, Icenowy Zheng wrote:
>
>
> 于 2019年10月3日 GMT+08:00 下午9:19:16, Maxime Ripard <mripard@kernel.org> 写到:
> >On Thu, Oct 03, 2019 at 12:38:43PM +0530, Jagan Teki wrote:
> >> On Tue, Oct 1, 2019 at 1:33 PM Icenowy Zheng <icenowy@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 <jagan@amarulasolutions.com>
> >
> >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.

Right, unfortunately, it doesn't apply anymore.

Jagan, can you send that patch rebased?

Thanks!
Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH 2/3] drm/sun4i: dsi: fix DRQ calculation

[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 <icenowy@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

Re: [linux-sunxi] [PATCH 2/3] drm/sun4i: dsi: fix DRQ calculation

[Jagan Teki]

Hi,

On Tue, Oct 1, 2019 at 1:34 PM Icenowy Zheng <icenowy@aosc.io> wrote:
>
> 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 <icenowy@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;

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/

[PATCH 3/3] Revert "drm/sun4i: dsi: Rework a bit the hblk calculation"

[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 <icenowy@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

Re: [linux-sunxi] [PATCH 3/3] Revert "drm/sun4i: dsi: Rework a bit the hblk calculation"

[Jagan Teki]

Hi Wens,

On Tue, Oct 1, 2019 at 1:34 PM Icenowy Zheng <icenowy@aosc.io> wrote:
>
> 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 <icenowy@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;

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

Re: [linux-sunxi] [PATCH 3/3] Revert "drm/sun4i: dsi: Rework a bit the hblk calculation"

[Icenowy Zheng]

在 2019-10-03四的 09:53 +0530，Jagan Teki写道：
> Hi Wens,
> 
> On Tue, Oct 1, 2019 at 1:34 PM Icenowy Zheng <icenowy@aosc.io> wrote:
> > 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 <icenowy@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;
> 
> 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.

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

Re: [linux-sunxi] [PATCH 3/3] Revert "drm/sun4i: dsi: Rework a bit the hblk calculation"

[Icenowy Zheng]

在 2019-10-06日的 22:44 +0800，Icenowy Zheng写道：
> 在 2019-10-03四的 09:53 +0530，Jagan Teki写道：
> > Hi Wens,
> > 
> > On Tue, Oct 1, 2019 at 1:34 PM Icenowy Zheng <icenowy@aosc.io>
> > wrote:
> > > 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 <icenowy@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;
> > 
> > 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.
> 
> 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).

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.

> 
> 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

Re: [linux-sunxi] [PATCH 3/3] Revert "drm/sun4i: dsi: Rework a bit the hblk calculation"

[Ondřej Jirman]

HI Icenowy,

On Sun, Oct 06, 2019 at 11:12:43PM +0800, Icenowy Zheng wrote:
> 在 2019-10-06日的 22:44 +0800，Icenowy Zheng写道：
> > 在 2019-10-03四的 09:53 +0530，Jagan Teki写道：
> > > Hi Wens,
> > > 
> > > On Tue, Oct 1, 2019 at 1:34 PM Icenowy Zheng <icenowy@aosc.io>
> > > wrote:
> > > > 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 <icenowy@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;
> > > 
> > > 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.
> > 
> > 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).
> 
> 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.

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+unsubscribe@googlegroups.com.
> To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/14da3ae768c439e387f6609553bd465e945d4a33.camel%40aosc.io.

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1054 bytes --]

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?

Ideally, having the panel drivers, and the panel datasheet would help.

Thanks!
Maxime

PS: where can we get one of those devices?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[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?