[PATCH v9 0/5] drm/sun4i: sun6i_mipi_dsi: Fixes/updates

[PATCH v9 0/5] drm/sun4i: sun6i_mipi_dsi: Fixes/updates

[Jagan Teki]

Unfortunately due to various reasons[3] the previous fixes[1] and burst[2]
changes are failed to apply.

So, this series is filtered version of previous [1] and [2] changes on top
of drm-misc.

patch-1: Fix for burst mode instruction delay computation

patch-2: Fix for TCOn DRQ set bits

patch-3: Support vblk timing for 4-lane devices

patch-4: GENERIC_SHORT_WRITE_2 dsi transfer support

patch-5: Code simplification for dsi timings   

Changes for v9:
- rebase on drm-misc
- update commit messages
- add hsync_porch overflow patch
Changes for v8:
- rebase on master
- rework on commit messages
- rework video start delay
- include drq changes from previous version
Changes for v7:
- rebase on master
- collect Merlijn Wajer Tested-by credits.
Changes for v6:
- fixed all burst mode patches as per previous version comments
- rebase on master
- update proper commit message
- dropped unneeded comments
- order the patches that make review easy
Changes for v5, v4, v3, v2:
- use existing driver code construct for hblk computation
- create separate function for vblk computation 
- cleanup commit messages
- update proper commit messages
- fixed checkpatch warnings/errors
- use proper return value for tcon0 probe
- add logic to get tcon0 divider values
- simplify timings code to support burst mode
- fix drq computation return values
- rebase on master

[1] https://patchwork.kernel.org/cover/10813573/
[2] https://patchwork.kernel.org/cover/10813623/
[3] https://patchwork.kernel.org/cover/10805995/

Any inputs?
Jagan.

Jagan Teki (5):
  drm/sun4i: sun6i_mipi_dsi: Fix hsync_porch overflow
  drm/sun4i: sun6i_mipi_dsi: Fix TCON DRQ set bits
  drm/sun4i: sun6i_mipi_dsi: Support vblk timing for 4-lane devices
  drm/sun4i: sun6i_mipi_dsi: Support DSI GENERIC_SHORT_WRITE_2 transfer
  drm/sun4i: sun6i_mipi_dsi: Simplify dsi setup timings code

 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 114 +++++++++++++++----------
 1 file changed, 67 insertions(+), 47 deletions(-)

-- 
2.18.0.321.gffc6fa0e3

[PATCH v9 1/5] drm/sun4i: sun6i_mipi_dsi: Fix hsync_porch overflow

[Jagan Teki]

Loop N1 instruction delay for burst mode devices are computed
based on horizontal sync and porch timing values.

The current driver is using u16 type for computing this hsync_porch
value, which would failed to fit within the u16 type for large sync
and porch timings devices. This would result in hsync_porch overflow
and eventually computed wrong instruction delay value.

Example, timings, where it produces the overflow
{
	.hdisplay       = 1080,
	.hsync_start    = 1080 + 408,
        .hsync_end      = 1080 + 408 + 4,
        .htotal         = 1080 + 408 + 4 + 38,
}

It reproduces the desired delay value 65487 but the correct working
value should be 7.

So, Fix it by computing hsync_porch value separately with u32 type.

Fixes: 1c1a7aa3663c ("drm/sun4i: dsi: Add burst support")
Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
Tested-by: Merlijn Wajer <merlijn@wizzup.org>
---
 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
index 6ff585055a07..465e7fc57899 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
@@ -457,8 +457,9 @@ static void sun6i_dsi_setup_inst_loop(struct sun6i_dsi *dsi,
 	u16 delay = 50 - 1;
 
 	if (device->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) {
-		delay = (mode->htotal - mode->hdisplay) * 150;
-		delay /= (mode->clock / 1000) * 8;
+		u32 hsync_porch = (mode->htotal - mode->hdisplay);
+
+		delay = ((hsync_porch * 150) / ((mode->clock / 1000) * 8));
 		delay -= 50;
 	}
 
-- 
2.18.0.321.gffc6fa0e3

Re: [PATCH v9 1/5] drm/sun4i: sun6i_mipi_dsi: Fix hsync_porch overflow

[Maxime Ripard]

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

On Sun, Mar 03, 2019 at 11:05:23PM +0530, Jagan Teki wrote:
> Loop N1 instruction delay for burst mode devices are computed
> based on horizontal sync and porch timing values.
> 
> The current driver is using u16 type for computing this hsync_porch
> value, which would failed to fit within the u16 type for large sync
> and porch timings devices. This would result in hsync_porch overflow
> and eventually computed wrong instruction delay value.
> 
> Example, timings, where it produces the overflow
> {
> 	.hdisplay       = 1080,
> 	.hsync_start    = 1080 + 408,
>         .hsync_end      = 1080 + 408 + 4,
>         .htotal         = 1080 + 408 + 4 + 38,
> }
> 
> It reproduces the desired delay value 65487 but the correct working
> value should be 7.
> 
> So, Fix it by computing hsync_porch value separately with u32 type.
> 
> Fixes: 1c1a7aa3663c ("drm/sun4i: dsi: Add burst support")
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> Tested-by: Merlijn Wajer <merlijn@wizzup.org>
> ---
>  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> index 6ff585055a07..465e7fc57899 100644
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> @@ -457,8 +457,9 @@ static void sun6i_dsi_setup_inst_loop(struct sun6i_dsi *dsi,
>  	u16 delay = 50 - 1;
>  
>  	if (device->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) {
> -		delay = (mode->htotal - mode->hdisplay) * 150;
> -		delay /= (mode->clock / 1000) * 8;
> +		u32 hsync_porch = (mode->htotal - mode->hdisplay);
> +
> +		delay = ((hsync_porch * 150) / ((mode->clock / 1000) * 8));

shouldn't we keep the multiplication by 150 in the u32 assignation?
Otherwise, we could keep a u16 for the delay

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

Re: [PATCH v9 1/5] drm/sun4i: sun6i_mipi_dsi: Fix hsync_porch overflow

[Jagan Teki]

On Mon, Mar 4, 2019 at 9:24 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> On Sun, Mar 03, 2019 at 11:05:23PM +0530, Jagan Teki wrote:
> > Loop N1 instruction delay for burst mode devices are computed
> > based on horizontal sync and porch timing values.
> >
> > The current driver is using u16 type for computing this hsync_porch
> > value, which would failed to fit within the u16 type for large sync
> > and porch timings devices. This would result in hsync_porch overflow
> > and eventually computed wrong instruction delay value.
> >
> > Example, timings, where it produces the overflow
> > {
> >       .hdisplay       = 1080,
> >       .hsync_start    = 1080 + 408,
> >         .hsync_end      = 1080 + 408 + 4,
> >         .htotal         = 1080 + 408 + 4 + 38,
> > }
> >
> > It reproduces the desired delay value 65487 but the correct working
> > value should be 7.
> >
> > So, Fix it by computing hsync_porch value separately with u32 type.
> >
> > Fixes: 1c1a7aa3663c ("drm/sun4i: dsi: Add burst support")
> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > Tested-by: Merlijn Wajer <merlijn@wizzup.org>
> > ---
> >  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > index 6ff585055a07..465e7fc57899 100644
> > --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > @@ -457,8 +457,9 @@ static void sun6i_dsi_setup_inst_loop(struct sun6i_dsi *dsi,
> >       u16 delay = 50 - 1;
> >
> >       if (device->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) {
> > -             delay = (mode->htotal - mode->hdisplay) * 150;
> > -             delay /= (mode->clock / 1000) * 8;
> > +             u32 hsync_porch = (mode->htotal - mode->hdisplay);
> > +
> > +             delay = ((hsync_porch * 150) / ((mode->clock / 1000) * 8));
>
> shouldn't we keep the multiplication by 150 in the u32 assignation?

Yes, ie true.  will move it.

[PATCH v9 2/5] drm/sun4i: sun6i_mipi_dsi: Fix TCON DRQ set bits

[Jagan Teki]

TCON DRQ for non-burst DSI mode can computed based on horizontal
front porch value, but the current driver trying to include sync
timings along with front porch resulting wrong drq.

This patch is trying to update the drq by subtracting hsync_start
with hdisplay, which is horizontal front porch.

Current code:
------------
mode->hsync_end - mode->hdisplay => horizontal front porch + sync

With this patch:
----------------
mode->hsync_start - mode->hdisplay => horizontal front porch

BSP code form BPI-M64-bsp is computing TCON DRQ set bits
for non-burts as (from linux-sunxi/
drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c)

=> panel->lcd_ht -    panel->lcd_x - panel->lcd_hbp
=> (timmings->hor_front_porch + panel->lcd_hbp + panel->lcd_x)
   - panel->lcd_x - panel->hbp
=> timmings->hor_front_porch
=> mode->hsync_start - mode->hdisplay

So, update the DRQ set bits accordingly.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
Tested-by: Merlijn Wajer <merlijn@wizzup.org>
---
 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 465e7fc57899..140e55f5ed2e 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.18.0.321.gffc6fa0e3

Re: [PATCH v9 2/5] drm/sun4i: sun6i_mipi_dsi: Fix TCON DRQ set bits

[Maxime Ripard]

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

On Sun, Mar 03, 2019 at 11:05:24PM +0530, Jagan Teki wrote:
> TCON DRQ for non-burst DSI mode can computed based on horizontal
> front porch value, but the current driver trying to include sync
> timings along with front porch resulting wrong drq.
> 
> This patch is trying to update the drq by subtracting hsync_start
> with hdisplay, which is horizontal front porch.
> 
> Current code:
> ------------
> mode->hsync_end - mode->hdisplay => horizontal front porch + sync
> 
> With this patch:
> ----------------
> mode->hsync_start - mode->hdisplay => horizontal front porch
> 
> BSP code form BPI-M64-bsp is computing TCON DRQ set bits
> for non-burts as (from linux-sunxi/
> drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c)
> 
> => panel->lcd_ht -    panel->lcd_x - panel->lcd_hbp
> => (timmings->hor_front_porch + panel->lcd_hbp + panel->lcd_x)
                                  ^ + sync length +          
>    - panel->lcd_x - panel->hbp
> => timmings->hor_front_porch
                               ^ + sync
> => mode->hsync_start - mode->hdisplay

s/hsync_start/hsync_end/

Did you encounter any panel where this was fixing something? If so,
which one, and what is the matching timings and / or datasheet?

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

Re: [PATCH v9 2/5] drm/sun4i: sun6i_mipi_dsi: Fix TCON DRQ set bits

[Maxime Ripard]

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

On Thu, Mar 07, 2019 at 05:49:07PM +0530, Jagan Teki wrote:
> On Mon, Mar 4, 2019 at 9:13 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> >
> > On Sun, Mar 03, 2019 at 11:05:24PM +0530, Jagan Teki wrote:
> > > TCON DRQ for non-burst DSI mode can computed based on horizontal
> > > front porch value, but the current driver trying to include sync
> > > timings along with front porch resulting wrong drq.
> > >
> > > This patch is trying to update the drq by subtracting hsync_start
> > > with hdisplay, which is horizontal front porch.
> > >
> > > Current code:
> > > ------------
> > > mode->hsync_end - mode->hdisplay => horizontal front porch + sync
> > >
> > > With this patch:
> > > ----------------
> > > mode->hsync_start - mode->hdisplay => horizontal front porch
> > >
> > > BSP code form BPI-M64-bsp is computing TCON DRQ set bits
> > > for non-burts as (from linux-sunxi/
> > > drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c)
> > >
> > > => panel->lcd_ht -    panel->lcd_x - panel->lcd_hbp
> > > => (timmings->hor_front_porch + panel->lcd_hbp + panel->lcd_x)
> >                                   ^ + sync length +
> > >    - panel->lcd_x - panel->hbp
> > > => timmings->hor_front_porch
> >                                ^ + sync
> > > => mode->hsync_start - mode->hdisplay
> >
> > s/hsync_start/hsync_end/
> 
> No, it should be front porch so it is hsync_start. This change is
> trying to update DRQ set to use front porch and above evaluation from
> BSP, result the same front front porch
> 
> Current driver has hsync_end - hdisplay which is not front porch
> timing (it is adding extra sync timing).

It would be if you considered that the back porch actually was the
back porch plus the sync length. I have found no such evidence, quite
the opposite actually, everything seems to point at the fact that
unlike the TCON, the DSI block uses the back porch as only the back
porch.


> I believe this is something similar like fixed patches for VBP, HBLK
> timings.
>
> > Did you encounter any panel where this was fixing something? If so,
> > which one, and what is the matching timings and / or datasheet?
> 
> W/O this change Bananapi  s070wv20 panel has issue on striped lines on
> the panel[1] and timings are
> 
> static const struct drm_display_mode s070wv20_default_mode = {
>         .clock = 30000,
>         .vrefresh = 60,
> 
>         .hdisplay = 800,
>         .hsync_start = 800 + 40,
>         .hsync_end = 800 + 40 + 48,
>         .htotal = 800 + 40 + 48 + 40,
> 
>         .vdisplay = 480,
>         .vsync_start = 480 + 13,
>         .vsync_end = 480 + 13 + 3,
>         .vtotal = 480 + 13 + 3 + 29,
> };
> 
> Which is similar like in panel-simple "bananapi,s070wv20-ct16"
> 
> Here is the DSI panel patches and sequence:
> [pixel clock is 30Mhz] https://patchwork.kernel.org/patch/10680331/
> https://github.com/yesnoandor/x300/blob/master/kernel/arch/arm/boot/dts/erobbing/x300/x300.dtsi#L81
> https://github.com/wxzed/Raspberry_5MIPI_Display/blob/master/I2C_Slave/USER/main.c#L15
> https://github.com/eliot-shao/qcom/blob/master/icn6211_cxn0102/kernel/drivers/video/msm/mdss/mdss_i2c_interface.c#L152

What are those supposed to be? It doesn't look like timings but rather initialization sequences

> matches timings for
> https://github.com/eliot-shao/qcom/blob/master/icn6211_cxn0102/kernel/arch/arm/boot/dts/qcom/dsi-mipi-2-rgb_1280p_video.dtsi#L20

That's not even the same resolution..

> https://github.com/zestroly/micromat/blob/master/test/raspberry/ICN6211.cpp#L169

And this isn't a set of timings either.

> Attached is panel datasheet.

Which is for an RGB panel... not a MIPI-DSI one.

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

Re: [PATCH v9 2/5] drm/sun4i: sun6i_mipi_dsi: Fix TCON DRQ set bits

[Jagan Teki]

On Thu, Mar 7, 2019 at 9:09 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> On Thu, Mar 07, 2019 at 05:49:07PM +0530, Jagan Teki wrote:
> > On Mon, Mar 4, 2019 at 9:13 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > >
> > > On Sun, Mar 03, 2019 at 11:05:24PM +0530, Jagan Teki wrote:
> > > > TCON DRQ for non-burst DSI mode can computed based on horizontal
> > > > front porch value, but the current driver trying to include sync
> > > > timings along with front porch resulting wrong drq.
> > > >
> > > > This patch is trying to update the drq by subtracting hsync_start
> > > > with hdisplay, which is horizontal front porch.
> > > >
> > > > Current code:
> > > > ------------
> > > > mode->hsync_end - mode->hdisplay => horizontal front porch + sync
> > > >
> > > > With this patch:
> > > > ----------------
> > > > mode->hsync_start - mode->hdisplay => horizontal front porch
> > > >
> > > > BSP code form BPI-M64-bsp is computing TCON DRQ set bits
> > > > for non-burts as (from linux-sunxi/
> > > > drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c)
> > > >
> > > > => panel->lcd_ht -    panel->lcd_x - panel->lcd_hbp
> > > > => (timmings->hor_front_porch + panel->lcd_hbp + panel->lcd_x)
> > >                                   ^ + sync length +
> > > >    - panel->lcd_x - panel->hbp
> > > > => timmings->hor_front_porch
> > >                                ^ + sync
> > > > => mode->hsync_start - mode->hdisplay
> > >
> > > s/hsync_start/hsync_end/
> >
> > No, it should be front porch so it is hsync_start. This change is
> > trying to update DRQ set to use front porch and above evaluation from
> > BSP, result the same front front porch
> >
> > Current driver has hsync_end - hdisplay which is not front porch
> > timing (it is adding extra sync timing).
>
> It would be if you considered that the back porch actually was the
> back porch plus the sync length. I have found no such evidence, quite
> the opposite actually, everything seems to point at the fact that
> unlike the TCON, the DSI block uses the back porch as only the back
> porch.

Sorry, I'm not clear about back porch here.

The current code has mode->hsync_end - mode->hdisplay which is Front
porch + sync time do you think it's not? DRQ set time is pure front
porch value (according BSP) as I didn't see any information about DRQ
set bits in manual or anywhere.

fyi: atleast if you didn't trust me, here is existing applied patch
for about equations.
https://cgit.freedesktop.org/drm/drm-misc/commit/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c?id=2cfdc24d2f8d9b14704567c065beb2a118a578fa

So, this patch fixed to remove sync time by updating hsync_start -
hdisplay which is pure front porch. and it's clear that pane is not
working without this.

>
>
> > I believe this is something similar like fixed patches for VBP, HBLK
> > timings.
> >
> > > Did you encounter any panel where this was fixing something? If so,
> > > which one, and what is the matching timings and / or datasheet?
> >
> > W/O this change Bananapi  s070wv20 panel has issue on striped lines on
> > the panel[1] and timings are
> >
> > static const struct drm_display_mode s070wv20_default_mode = {
> >         .clock = 30000,
> >         .vrefresh = 60,
> >
> >         .hdisplay = 800,
> >         .hsync_start = 800 + 40,
> >         .hsync_end = 800 + 40 + 48,
> >         .htotal = 800 + 40 + 48 + 40,
> >
> >         .vdisplay = 480,
> >         .vsync_start = 480 + 13,
> >         .vsync_end = 480 + 13 + 3,
> >         .vtotal = 480 + 13 + 3 + 29,
> > };
> >
> > Which is similar like in panel-simple "bananapi,s070wv20-ct16"
> >
> > Here is the DSI panel patches and sequence:
> > [pixel clock is 30Mhz] https://patchwork.kernel.org/patch/10680331/
> > https://github.com/yesnoandor/x300/blob/master/kernel/arch/arm/boot/dts/erobbing/x300/x300.dtsi#L81
> > https://github.com/wxzed/Raspberry_5MIPI_Display/blob/master/I2C_Slave/USER/main.c#L15
> > https://github.com/eliot-shao/qcom/blob/master/icn6211_cxn0102/kernel/drivers/video/msm/mdss/mdss_i2c_interface.c#L152
>
> What are those supposed to be? It doesn't look like timings but rather initialization sequences
>
> > matches timings for
> > https://github.com/eliot-shao/qcom/blob/master/icn6211_cxn0102/kernel/arch/arm/boot/dts/qcom/dsi-mipi-2-rgb_1280p_video.dtsi#L20
>
> That's not even the same resolution..

fyi about the sequence.

>
> > https://github.com/zestroly/micromat/blob/master/test/raspberry/ICN6211.cpp#L169
>
> And this isn't a set of timings either.
>
> > Attached is panel datasheet.
>
> Which is for an RGB panel... not a MIPI-DSI one.

Same panel timings It is a DSI ICN6211 bridge, I have attached the
patch above https://patchwork.kernel.org/patch/10680331/ for
information about the display please find previous mail attachment.

Re: [PATCH v9 2/5] drm/sun4i: sun6i_mipi_dsi: Fix TCON DRQ set bits

[Maxime Ripard]

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

On Thu, Mar 07, 2019 at 09:24:02PM +0530, Jagan Teki wrote:
> On Thu, Mar 7, 2019 at 9:09 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> >
> > On Thu, Mar 07, 2019 at 05:49:07PM +0530, Jagan Teki wrote:
> > > On Mon, Mar 4, 2019 at 9:13 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > > >
> > > > On Sun, Mar 03, 2019 at 11:05:24PM +0530, Jagan Teki wrote:
> > > > > TCON DRQ for non-burst DSI mode can computed based on horizontal
> > > > > front porch value, but the current driver trying to include sync
> > > > > timings along with front porch resulting wrong drq.
> > > > >
> > > > > This patch is trying to update the drq by subtracting hsync_start
> > > > > with hdisplay, which is horizontal front porch.
> > > > >
> > > > > Current code:
> > > > > ------------
> > > > > mode->hsync_end - mode->hdisplay => horizontal front porch + sync
> > > > >
> > > > > With this patch:
> > > > > ----------------
> > > > > mode->hsync_start - mode->hdisplay => horizontal front porch
> > > > >
> > > > > BSP code form BPI-M64-bsp is computing TCON DRQ set bits
> > > > > for non-burts as (from linux-sunxi/
> > > > > drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c)
> > > > >
> > > > > => panel->lcd_ht -    panel->lcd_x - panel->lcd_hbp
> > > > > => (timmings->hor_front_porch + panel->lcd_hbp + panel->lcd_x)
> > > >                                   ^ + sync length +
> > > > >    - panel->lcd_x - panel->hbp
> > > > > => timmings->hor_front_porch
> > > >                                ^ + sync
> > > > > => mode->hsync_start - mode->hdisplay
> > > >
> > > > s/hsync_start/hsync_end/
> > >
> > > No, it should be front porch so it is hsync_start. This change is
> > > trying to update DRQ set to use front porch and above evaluation from
> > > BSP, result the same front front porch
> > >
> > > Current driver has hsync_end - hdisplay which is not front porch
> > > timing (it is adding extra sync timing).
> >
> > It would be if you considered that the back porch actually was the
> > back porch plus the sync length. I have found no such evidence, quite
> > the opposite actually, everything seems to point at the fact that
> > unlike the TCON, the DSI block uses the back porch as only the back
> > porch.
> 
> Sorry, I'm not clear about back porch here.
> 
> The current code has mode->hsync_end - mode->hdisplay which is Front
> porch + sync time do you think it's not?

It is.

> DRQ set time is pure front porch value (according BSP) as I didn't
> see any information about DRQ set bits in manual or anywhere.

This is what I'm telling you. If you consider the back porch as only
the back porch, then the result of that BSP calculation you mentionned
earlier is the front porch and the sync length.

You imply that the back porch should actually be treated as the back
porch and the sync length in your calculation. What makes you say so?

> fyi: atleast if you didn't trust me, here is existing applied patch
> for about equations.
> https://cgit.freedesktop.org/drm/drm-misc/commit/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c?id=2cfdc24d2f8d9b14704567c065beb2a118a578fa

I'm aware of that patch, but I have no idea why do you bring it up in
that discussion.

> So, this patch fixed to remove sync time by updating hsync_start -
> hdisplay which is pure front porch. and it's clear that pane is not
> working without this.

This is the same discussion over and over again: which panel, which
datasheet, not working how?

> > > I believe this is something similar like fixed patches for VBP, HBLK
> > > timings.
> > >
> > > > Did you encounter any panel where this was fixing something? If so,
> > > > which one, and what is the matching timings and / or datasheet?
> > >
> > > W/O this change Bananapi  s070wv20 panel has issue on striped lines on
> > > the panel[1] and timings are
> > >
> > > static const struct drm_display_mode s070wv20_default_mode = {
> > >         .clock = 30000,
> > >         .vrefresh = 60,
> > >
> > >         .hdisplay = 800,
> > >         .hsync_start = 800 + 40,
> > >         .hsync_end = 800 + 40 + 48,
> > >         .htotal = 800 + 40 + 48 + 40,
> > >
> > >         .vdisplay = 480,
> > >         .vsync_start = 480 + 13,
> > >         .vsync_end = 480 + 13 + 3,
> > >         .vtotal = 480 + 13 + 3 + 29,
> > > };
> > >
> > > Which is similar like in panel-simple "bananapi,s070wv20-ct16"
> > >
> > > Here is the DSI panel patches and sequence:
> > > [pixel clock is 30Mhz] https://patchwork.kernel.org/patch/10680331/
> > > https://github.com/yesnoandor/x300/blob/master/kernel/arch/arm/boot/dts/erobbing/x300/x300.dtsi#L81
> > > https://github.com/wxzed/Raspberry_5MIPI_Display/blob/master/I2C_Slave/USER/main.c#L15
> > > https://github.com/eliot-shao/qcom/blob/master/icn6211_cxn0102/kernel/drivers/video/msm/mdss/mdss_i2c_interface.c#L152
> >
> > What are those supposed to be? It doesn't look like timings but rather initialization sequences
> >
> > > matches timings for
> > > https://github.com/eliot-shao/qcom/blob/master/icn6211_cxn0102/kernel/arch/arm/boot/dts/qcom/dsi-mipi-2-rgb_1280p_video.dtsi#L20
> >
> > That's not even the same resolution..
> 
> fyi about the sequence.

How is that relevant?

> > > https://github.com/zestroly/micromat/blob/master/test/raspberry/ICN6211.cpp#L169
> >
> > And this isn't a set of timings either.
> >
> > > Attached is panel datasheet.
> >
> > Which is for an RGB panel... not a MIPI-DSI one.
> 
> Same panel timings It is a DSI ICN6211 bridge, I have attached the
> patch above https://patchwork.kernel.org/patch/10680331/ for
> information about the display please find previous mail attachment.

The previous attachment was for a display that doesn't even have the
same bus. How is that relevant?

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

Re: [PATCH v9 2/5] drm/sun4i: sun6i_mipi_dsi: Fix TCON DRQ set bits

[Jagan Teki]

On Mon, Mar 11, 2019 at 7:39 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> On Thu, Mar 07, 2019 at 09:24:02PM +0530, Jagan Teki wrote:
> > On Thu, Mar 7, 2019 at 9:09 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > >
> > > On Thu, Mar 07, 2019 at 05:49:07PM +0530, Jagan Teki wrote:
> > > > On Mon, Mar 4, 2019 at 9:13 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > > > >
> > > > > On Sun, Mar 03, 2019 at 11:05:24PM +0530, Jagan Teki wrote:
> > > > > > TCON DRQ for non-burst DSI mode can computed based on horizontal
> > > > > > front porch value, but the current driver trying to include sync
> > > > > > timings along with front porch resulting wrong drq.
> > > > > >
> > > > > > This patch is trying to update the drq by subtracting hsync_start
> > > > > > with hdisplay, which is horizontal front porch.
> > > > > >
> > > > > > Current code:
> > > > > > ------------
> > > > > > mode->hsync_end - mode->hdisplay => horizontal front porch + sync
> > > > > >
> > > > > > With this patch:
> > > > > > ----------------
> > > > > > mode->hsync_start - mode->hdisplay => horizontal front porch
> > > > > >
> > > > > > BSP code form BPI-M64-bsp is computing TCON DRQ set bits
> > > > > > for non-burts as (from linux-sunxi/
> > > > > > drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c)
> > > > > >
> > > > > > => panel->lcd_ht -    panel->lcd_x - panel->lcd_hbp
> > > > > > => (timmings->hor_front_porch + panel->lcd_hbp + panel->lcd_x)
> > > > >                                   ^ + sync length +
> > > > > >    - panel->lcd_x - panel->hbp
> > > > > > => timmings->hor_front_porch
> > > > >                                ^ + sync
> > > > > > => mode->hsync_start - mode->hdisplay
> > > > >
> > > > > s/hsync_start/hsync_end/
> > > >
> > > > No, it should be front porch so it is hsync_start. This change is
> > > > trying to update DRQ set to use front porch and above evaluation from
> > > > BSP, result the same front front porch
> > > >
> > > > Current driver has hsync_end - hdisplay which is not front porch
> > > > timing (it is adding extra sync timing).
> > >
> > > It would be if you considered that the back porch actually was the
> > > back porch plus the sync length. I have found no such evidence, quite
> > > the opposite actually, everything seems to point at the fact that
> > > unlike the TCON, the DSI block uses the back porch as only the back
> > > porch.
> >
> > Sorry, I'm not clear about back porch here.
> >
> > The current code has mode->hsync_end - mode->hdisplay which is Front
> > porch + sync time do you think it's not?
>
> It is.
>
> > DRQ set time is pure front porch value (according BSP) as I didn't
> > see any information about DRQ set bits in manual or anywhere.
>
> This is what I'm telling you. If you consider the back porch as only
> the back porch, then the result of that BSP calculation you mentionned
> earlier is the front porch and the sync length.
>
> You imply that the back porch should actually be treated as the back
> porch and the sync length in your calculation. What makes you say so?

I'm consider the computations accordingly to drm_modes.h and which
matches similar like BSP code.

mode->hsync_end - mode->hdisplay = front porch + sync

but the actual computation is
mode->hsync_start - mode->hdisplay =>  front porch

Which is similar to what BSP is using.

=> panel->lcd_ht -    panel->lcd_x - panel->lcd_hbp
=> (timmings->hor_front_porch + panel->lcd_hbp + panel->lcd_x)
   - panel->lcd_x - panel->hbp
=> timmings->hor_front_porch
=> mode->hsync_start - mode->hdisplay

>
> > fyi: atleast if you didn't trust me, here is existing applied patch
> > for about equations.
> > https://cgit.freedesktop.org/drm/drm-misc/commit/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c?id=2cfdc24d2f8d9b14704567c065beb2a118a578fa
>
> I'm aware of that patch, but I have no idea why do you bring it up in
> that discussion.

I'm trying to give valid computation which we wrongly mentioned
before. just a reference.

>
> > So, this patch fixed to remove sync time by updating hsync_start -
> > hdisplay which is pure front porch. and it's clear that pane is not
> > working without this.
>
> This is the same discussion over and over again: which panel, which
> datasheet, not working how?

Like I said before. This bananapi,s070wv20-ct16 DSI bridge and we
don't have exact datasheet other than all  the information that I sent
in previous mail. Chen-Yu has mentioned all these references before[1]

>
> > > > I believe this is something similar like fixed patches for VBP, HBLK
> > > > timings.
> > > >
> > > > > Did you encounter any panel where this was fixing something? If so,
> > > > > which one, and what is the matching timings and / or datasheet?
> > > >
> > > > W/O this change Bananapi  s070wv20 panel has issue on striped lines on
> > > > the panel[1] and timings are
> > > >
> > > > static const struct drm_display_mode s070wv20_default_mode = {
> > > >         .clock = 30000,
> > > >         .vrefresh = 60,
> > > >
> > > >         .hdisplay = 800,
> > > >         .hsync_start = 800 + 40,
> > > >         .hsync_end = 800 + 40 + 48,
> > > >         .htotal = 800 + 40 + 48 + 40,
> > > >
> > > >         .vdisplay = 480,
> > > >         .vsync_start = 480 + 13,
> > > >         .vsync_end = 480 + 13 + 3,
> > > >         .vtotal = 480 + 13 + 3 + 29,
> > > > };
> > > >
> > > > Which is similar like in panel-simple "bananapi,s070wv20-ct16"
> > > >
> > > > Here is the DSI panel patches and sequence:
> > > > [pixel clock is 30Mhz] https://patchwork.kernel.org/patch/10680331/
> > > > https://github.com/yesnoandor/x300/blob/master/kernel/arch/arm/boot/dts/erobbing/x300/x300.dtsi#L81
> > > > https://github.com/wxzed/Raspberry_5MIPI_Display/blob/master/I2C_Slave/USER/main.c#L15
> > > > https://github.com/eliot-shao/qcom/blob/master/icn6211_cxn0102/kernel/drivers/video/msm/mdss/mdss_i2c_interface.c#L152
> > >
> > > What are those supposed to be? It doesn't look like timings but rather initialization sequences
> > >
> > > > matches timings for
> > > > https://github.com/eliot-shao/qcom/blob/master/icn6211_cxn0102/kernel/arch/arm/boot/dts/qcom/dsi-mipi-2-rgb_1280p_video.dtsi#L20
> > >
> > > That's not even the same resolution..
> >
> > fyi about the sequence.
>
> How is that relevant?
>
> > > > https://github.com/zestroly/micromat/blob/master/test/raspberry/ICN6211.cpp#L169
> > >
> > > And this isn't a set of timings either.
> > >
> > > > Attached is panel datasheet.
> > >
> > > Which is for an RGB panel... not a MIPI-DSI one.
> >
> > Same panel timings It is a DSI ICN6211 bridge, I have attached the
> > patch above https://patchwork.kernel.org/patch/10680331/ for
> > information about the display please find previous mail attachment.
>
> The previous attachment was for a display that doesn't even have the
> same bus. How is that relevant?

No, it's same datasheet for RGB, DSI and we have only of that.

[1] https://patchwork.kernel.org/patch/10618421/

Re: [PATCH v9 2/5] drm/sun4i: sun6i_mipi_dsi: Fix TCON DRQ set bits

[Maxime Ripard]

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

On Mon, Mar 11, 2019 at 08:28:22PM +0530, Jagan Teki wrote:
> On Mon, Mar 11, 2019 at 7:39 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> >
> > On Thu, Mar 07, 2019 at 09:24:02PM +0530, Jagan Teki wrote:
> > > On Thu, Mar 7, 2019 at 9:09 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > > >
> > > > On Thu, Mar 07, 2019 at 05:49:07PM +0530, Jagan Teki wrote:
> > > > > On Mon, Mar 4, 2019 at 9:13 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > > > > >
> > > > > > On Sun, Mar 03, 2019 at 11:05:24PM +0530, Jagan Teki wrote:
> > > > > > > TCON DRQ for non-burst DSI mode can computed based on horizontal
> > > > > > > front porch value, but the current driver trying to include sync
> > > > > > > timings along with front porch resulting wrong drq.
> > > > > > >
> > > > > > > This patch is trying to update the drq by subtracting hsync_start
> > > > > > > with hdisplay, which is horizontal front porch.
> > > > > > >
> > > > > > > Current code:
> > > > > > > ------------
> > > > > > > mode->hsync_end - mode->hdisplay => horizontal front porch + sync
> > > > > > >
> > > > > > > With this patch:
> > > > > > > ----------------
> > > > > > > mode->hsync_start - mode->hdisplay => horizontal front porch
> > > > > > >
> > > > > > > BSP code form BPI-M64-bsp is computing TCON DRQ set bits
> > > > > > > for non-burts as (from linux-sunxi/
> > > > > > > drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c)
> > > > > > >
> > > > > > > => panel->lcd_ht -    panel->lcd_x - panel->lcd_hbp
> > > > > > > => (timmings->hor_front_porch + panel->lcd_hbp + panel->lcd_x)
> > > > > >                                   ^ + sync length +
> > > > > > >    - panel->lcd_x - panel->hbp
> > > > > > > => timmings->hor_front_porch
> > > > > >                                ^ + sync
> > > > > > > => mode->hsync_start - mode->hdisplay
> > > > > >
> > > > > > s/hsync_start/hsync_end/
> > > > >
> > > > > No, it should be front porch so it is hsync_start. This change is
> > > > > trying to update DRQ set to use front porch and above evaluation from
> > > > > BSP, result the same front front porch
> > > > >
> > > > > Current driver has hsync_end - hdisplay which is not front porch
> > > > > timing (it is adding extra sync timing).
> > > >
> > > > It would be if you considered that the back porch actually was the
> > > > back porch plus the sync length. I have found no such evidence, quite
> > > > the opposite actually, everything seems to point at the fact that
> > > > unlike the TCON, the DSI block uses the back porch as only the back
> > > > porch.
> > >
> > > Sorry, I'm not clear about back porch here.
> > >
> > > The current code has mode->hsync_end - mode->hdisplay which is Front
> > > porch + sync time do you think it's not?
> >
> > It is.
> >
> > > DRQ set time is pure front porch value (according BSP) as I didn't
> > > see any information about DRQ set bits in manual or anywhere.
> >
> > This is what I'm telling you. If you consider the back porch as only
> > the back porch, then the result of that BSP calculation you mentionned
> > earlier is the front porch and the sync length.
> >
> > You imply that the back porch should actually be treated as the back
> > porch and the sync length in your calculation. What makes you say so?
>
> I'm consider the computations accordingly to drm_modes.h and which
> matches similar like BSP code.
>
> mode->hsync_end - mode->hdisplay = front porch + sync
>
> but the actual computation is
> mode->hsync_start - mode->hdisplay =>  front porch
>
> Which is similar to what BSP is using.
>
> => panel->lcd_ht -    panel->lcd_x - panel->lcd_hbp
> => (timmings->hor_front_porch + panel->lcd_hbp + panel->lcd_x)
>    - panel->lcd_x - panel->hbp
> => timmings->hor_front_porch
> => mode->hsync_start - mode->hdisplay

You're missing the point. What I'm asking for is why you think that
panel->lcd_ht == (timmings->hor_front_porch + panel->lcd_hbp +
panel->lcd_x) and not timmings->hor_front_porch + panel->lcd_hbp +
sync_width + panel->lcd_x

maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

Re: [PATCH v9 2/5] drm/sun4i: sun6i_mipi_dsi: Fix TCON DRQ set bits

[Jagan Teki]

On Tue, Mar 19, 2019 at 4:26 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> On Mon, Mar 11, 2019 at 08:28:22PM +0530, Jagan Teki wrote:
> > On Mon, Mar 11, 2019 at 7:39 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > >
> > > On Thu, Mar 07, 2019 at 09:24:02PM +0530, Jagan Teki wrote:
> > > > On Thu, Mar 7, 2019 at 9:09 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > > > >
> > > > > On Thu, Mar 07, 2019 at 05:49:07PM +0530, Jagan Teki wrote:
> > > > > > On Mon, Mar 4, 2019 at 9:13 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > > > > > >
> > > > > > > On Sun, Mar 03, 2019 at 11:05:24PM +0530, Jagan Teki wrote:
> > > > > > > > TCON DRQ for non-burst DSI mode can computed based on horizontal
> > > > > > > > front porch value, but the current driver trying to include sync
> > > > > > > > timings along with front porch resulting wrong drq.
> > > > > > > >
> > > > > > > > This patch is trying to update the drq by subtracting hsync_start
> > > > > > > > with hdisplay, which is horizontal front porch.
> > > > > > > >
> > > > > > > > Current code:
> > > > > > > > ------------
> > > > > > > > mode->hsync_end - mode->hdisplay => horizontal front porch + sync
> > > > > > > >
> > > > > > > > With this patch:
> > > > > > > > ----------------
> > > > > > > > mode->hsync_start - mode->hdisplay => horizontal front porch
> > > > > > > >
> > > > > > > > BSP code form BPI-M64-bsp is computing TCON DRQ set bits
> > > > > > > > for non-burts as (from linux-sunxi/
> > > > > > > > drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c)
> > > > > > > >
> > > > > > > > => panel->lcd_ht -    panel->lcd_x - panel->lcd_hbp
> > > > > > > > => (timmings->hor_front_porch + panel->lcd_hbp + panel->lcd_x)
> > > > > > >                                   ^ + sync length +
> > > > > > > >    - panel->lcd_x - panel->hbp
> > > > > > > > => timmings->hor_front_porch
> > > > > > >                                ^ + sync
> > > > > > > > => mode->hsync_start - mode->hdisplay
> > > > > > >
> > > > > > > s/hsync_start/hsync_end/
> > > > > >
> > > > > > No, it should be front porch so it is hsync_start. This change is
> > > > > > trying to update DRQ set to use front porch and above evaluation from
> > > > > > BSP, result the same front front porch
> > > > > >
> > > > > > Current driver has hsync_end - hdisplay which is not front porch
> > > > > > timing (it is adding extra sync timing).
> > > > >
> > > > > It would be if you considered that the back porch actually was the
> > > > > back porch plus the sync length. I have found no such evidence, quite
> > > > > the opposite actually, everything seems to point at the fact that
> > > > > unlike the TCON, the DSI block uses the back porch as only the back
> > > > > porch.
> > > >
> > > > Sorry, I'm not clear about back porch here.
> > > >
> > > > The current code has mode->hsync_end - mode->hdisplay which is Front
> > > > porch + sync time do you think it's not?
> > >
> > > It is.
> > >
> > > > DRQ set time is pure front porch value (according BSP) as I didn't
> > > > see any information about DRQ set bits in manual or anywhere.
> > >
> > > This is what I'm telling you. If you consider the back porch as only
> > > the back porch, then the result of that BSP calculation you mentionned
> > > earlier is the front porch and the sync length.
> > >
> > > You imply that the back porch should actually be treated as the back
> > > porch and the sync length in your calculation. What makes you say so?
> >
> > I'm consider the computations accordingly to drm_modes.h and which
> > matches similar like BSP code.
> >
> > mode->hsync_end - mode->hdisplay = front porch + sync
> >
> > but the actual computation is
> > mode->hsync_start - mode->hdisplay =>  front porch
> >
> > Which is similar to what BSP is using.
> >
> > => panel->lcd_ht -    panel->lcd_x - panel->lcd_hbp
> > => (timmings->hor_front_porch + panel->lcd_hbp + panel->lcd_x)
> >    - panel->lcd_x - panel->hbp
> > => timmings->hor_front_porch
> > => mode->hsync_start - mode->hdisplay
>
> You're missing the point. What I'm asking for is why you think that
> panel->lcd_ht == (timmings->hor_front_porch + panel->lcd_hbp +
> panel->lcd_x) and not timmings->hor_front_porch + panel->lcd_hbp +
> sync_width + panel->lcd_x

No ie not true we can't include sync_width, please see here [1]

timmings->hor_front_porch= panel_info->lcd_ht-panel_info->lcd_hbp -
panel_info->lcd_x;
panel_info->lcd_ht = timmings->hor_front_porch + panel_info->lcd_hbp +
panel_info->lcd_x;

and the result of this lcd_ht is exactly matched with Mainline timings
or any supported panel in A64.

[1] https://github.com/longsleep/linux-pine64/blob/pine64-hacks-1.2/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L1839

Re: [PATCH v9 2/5] drm/sun4i: sun6i_mipi_dsi: Fix TCON DRQ set bits

[Maxime Ripard]

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

On Thu, Mar 28, 2019 at 05:23:39PM +0530, Jagan Teki wrote:
> On Tue, Mar 19, 2019 at 4:26 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> >
> > On Mon, Mar 11, 2019 at 08:28:22PM +0530, Jagan Teki wrote:
> > > On Mon, Mar 11, 2019 at 7:39 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > > >
> > > > On Thu, Mar 07, 2019 at 09:24:02PM +0530, Jagan Teki wrote:
> > > > > On Thu, Mar 7, 2019 at 9:09 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > > > > >
> > > > > > On Thu, Mar 07, 2019 at 05:49:07PM +0530, Jagan Teki wrote:
> > > > > > > On Mon, Mar 4, 2019 at 9:13 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > > > > > > >
> > > > > > > > On Sun, Mar 03, 2019 at 11:05:24PM +0530, Jagan Teki wrote:
> > > > > > > > > TCON DRQ for non-burst DSI mode can computed based on horizontal
> > > > > > > > > front porch value, but the current driver trying to include sync
> > > > > > > > > timings along with front porch resulting wrong drq.
> > > > > > > > >
> > > > > > > > > This patch is trying to update the drq by subtracting hsync_start
> > > > > > > > > with hdisplay, which is horizontal front porch.
> > > > > > > > >
> > > > > > > > > Current code:
> > > > > > > > > ------------
> > > > > > > > > mode->hsync_end - mode->hdisplay => horizontal front porch + sync
> > > > > > > > >
> > > > > > > > > With this patch:
> > > > > > > > > ----------------
> > > > > > > > > mode->hsync_start - mode->hdisplay => horizontal front porch
> > > > > > > > >
> > > > > > > > > BSP code form BPI-M64-bsp is computing TCON DRQ set bits
> > > > > > > > > for non-burts as (from linux-sunxi/
> > > > > > > > > drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c)
> > > > > > > > >
> > > > > > > > > => panel->lcd_ht -    panel->lcd_x - panel->lcd_hbp
> > > > > > > > > => (timmings->hor_front_porch + panel->lcd_hbp + panel->lcd_x)
> > > > > > > >                                   ^ + sync length +
> > > > > > > > >    - panel->lcd_x - panel->hbp
> > > > > > > > > => timmings->hor_front_porch
> > > > > > > >                                ^ + sync
> > > > > > > > > => mode->hsync_start - mode->hdisplay
> > > > > > > >
> > > > > > > > s/hsync_start/hsync_end/
> > > > > > >
> > > > > > > No, it should be front porch so it is hsync_start. This change is
> > > > > > > trying to update DRQ set to use front porch and above evaluation from
> > > > > > > BSP, result the same front front porch
> > > > > > >
> > > > > > > Current driver has hsync_end - hdisplay which is not front porch
> > > > > > > timing (it is adding extra sync timing).
> > > > > >
> > > > > > It would be if you considered that the back porch actually was the
> > > > > > back porch plus the sync length. I have found no such evidence, quite
> > > > > > the opposite actually, everything seems to point at the fact that
> > > > > > unlike the TCON, the DSI block uses the back porch as only the back
> > > > > > porch.
> > > > >
> > > > > Sorry, I'm not clear about back porch here.
> > > > >
> > > > > The current code has mode->hsync_end - mode->hdisplay which is Front
> > > > > porch + sync time do you think it's not?
> > > >
> > > > It is.
> > > >
> > > > > DRQ set time is pure front porch value (according BSP) as I didn't
> > > > > see any information about DRQ set bits in manual or anywhere.
> > > >
> > > > This is what I'm telling you. If you consider the back porch as only
> > > > the back porch, then the result of that BSP calculation you mentionned
> > > > earlier is the front porch and the sync length.
> > > >
> > > > You imply that the back porch should actually be treated as the back
> > > > porch and the sync length in your calculation. What makes you say so?
> > >
> > > I'm consider the computations accordingly to drm_modes.h and which
> > > matches similar like BSP code.
> > >
> > > mode->hsync_end - mode->hdisplay = front porch + sync
> > >
> > > but the actual computation is
> > > mode->hsync_start - mode->hdisplay =>  front porch
> > >
> > > Which is similar to what BSP is using.
> > >
> > > => panel->lcd_ht -    panel->lcd_x - panel->lcd_hbp
> > > => (timmings->hor_front_porch + panel->lcd_hbp + panel->lcd_x)
> > >    - panel->lcd_x - panel->hbp
> > > => timmings->hor_front_porch
> > > => mode->hsync_start - mode->hdisplay
> >
> > You're missing the point. What I'm asking for is why you think that
> > panel->lcd_ht == (timmings->hor_front_porch + panel->lcd_hbp +
> > panel->lcd_x) and not timmings->hor_front_porch + panel->lcd_hbp +
> > sync_width + panel->lcd_x
>
> No ie not true we can't include sync_width, please see here [1]
>
> timmings->hor_front_porch= panel_info->lcd_ht-panel_info->lcd_hbp -
> panel_info->lcd_x;
> panel_info->lcd_ht = timmings->hor_front_porch + panel_info->lcd_hbp +
> panel_info->lcd_x;
>
> and the result of this lcd_ht is exactly matched with Mainline timings
> or any supported panel in A64.
>
> [1] https://github.com/longsleep/linux-pine64/blob/pine64-hacks-1.2/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L1839

Can we please start to reason in actual timings instead of whatever
Allwinner comes up with in their BSP?

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

[PATCH v9 3/5] drm/sun4i: sun6i_mipi_dsi: Support vblk timing for 4-lane devices

[Jagan Teki]

Like other dsi setup timings, or hblk for that matter vblk would
also require compute the timings based payload equation along with
packet overhead.

But, on the other hand vblk computation is also depends on device
lane number.
- for 4 lane devices, it is computed based on vtotal, packet overhead
  along with hblk value.
- for others devices, it is simply 0

BSP code from BPI-M64-bsp is computing vblk as for 4-lane devices
(from linux-sunxi
drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c)

tmp = (ht*dsi_pixel_bits[format]/8)*vt-(4+dsi_hblk+2);
dsi_vblk = (lane-tmp%lane);

So, update the vblk timing calculation to support all type of
devices.

Tested on 2-lane, 4-lane MIPI-DSI LCD panels.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
Tested-by: Merlijn Wajer <merlijn@wizzup.org>
---
 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 27 +++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
index 140e55f5ed2e..b38358465d87 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
@@ -527,6 +527,24 @@ static void sun6i_dsi_setup_format(struct sun6i_dsi *dsi,
 		     SUN6I_DSI_PIXEL_CTL0_FORMAT(fmt));
 }
 
+static u16 sun6i_dsi_get_timings_vblk(struct sun6i_dsi *dsi,
+				      struct drm_display_mode *mode, u16 hblk)
+{
+	struct mipi_dsi_device *device = dsi->device;
+	unsigned int Bpp = mipi_dsi_pixel_format_to_bpp(device->format) / 8;
+	int tmp;
+
+	/*
+	 * The vertical blank is set using a blanking packet (4 bytes +
+	 * payload + 2 bytes). Its minimal size is therefore 6 bytes
+	 */
+#define VBLK_PACKET_OVERHEAD	6
+	tmp = (mode->htotal * Bpp) * mode->vtotal -
+	      (hblk + VBLK_PACKET_OVERHEAD);
+
+	return (device->lanes - tmp % device->lanes);
+}
+
 static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi,
 				    struct drm_display_mode *mode)
 {
@@ -586,13 +604,8 @@ static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi,
 			   (mode->htotal - (mode->hsync_end - mode->hsync_start)) * Bpp -
 			   HBLK_PACKET_OVERHEAD);
 
-		/*
-		 * And I'm not entirely sure what vblk is about. The driver in
-		 * Allwinner BSP is using a rather convoluted calculation
-		 * there only for 4 lanes. However, using 0 (the !4 lanes
-		 * case) even with a 4 lanes screen seems to work...
-		 */
-		vblk = 0;
+		if (device->lanes == 4)
+			vblk = sun6i_dsi_get_timings_vblk(dsi, mode, hblk);
 	}
 
 	/* How many bytes do we need to send all payloads? */
-- 
2.18.0.321.gffc6fa0e3

Re: [PATCH v9 3/5] drm/sun4i: sun6i_mipi_dsi: Support vblk timing for 4-lane devices

[Maxime Ripard]

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

On Sun, Mar 03, 2019 at 11:05:25PM +0530, Jagan Teki wrote:
> Like other dsi setup timings, or hblk for that matter vblk would
> also require compute the timings based payload equation along with
> packet overhead.
> 
> But, on the other hand vblk computation is also depends on device
> lane number.
> - for 4 lane devices, it is computed based on vtotal, packet overhead
>   along with hblk value.
> - for others devices, it is simply 0
> 
> BSP code from BPI-M64-bsp is computing vblk as for 4-lane devices
> (from linux-sunxi
> drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c)
> 
> tmp = (ht*dsi_pixel_bits[format]/8)*vt-(4+dsi_hblk+2);
> dsi_vblk = (lane-tmp%lane);
> 
> So, update the vblk timing calculation to support all type of
> devices.
> 
> Tested on 2-lane, 4-lane MIPI-DSI LCD panels.

You should be explaining which issue you faced, in which setup, what
were its symptoms and how that solution is fixing it.

> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> Tested-by: Merlijn Wajer <merlijn@wizzup.org>
> ---
>  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 27 +++++++++++++++++++-------
>  1 file changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> index 140e55f5ed2e..b38358465d87 100644
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> @@ -527,6 +527,24 @@ static void sun6i_dsi_setup_format(struct sun6i_dsi *dsi,
>  		     SUN6I_DSI_PIXEL_CTL0_FORMAT(fmt));
>  }
>  
> +static u16 sun6i_dsi_get_timings_vblk(struct sun6i_dsi *dsi,
> +				      struct drm_display_mode *mode, u16 hblk)
> +{
> +	struct mipi_dsi_device *device = dsi->device;
> +	unsigned int Bpp = mipi_dsi_pixel_format_to_bpp(device->format) / 8;
> +	int tmp;
> +
> +	/*
> +	 * The vertical blank is set using a blanking packet (4 bytes +
> +	 * payload + 2 bytes). Its minimal size is therefore 6 bytes
> +	 */
> +#define VBLK_PACKET_OVERHEAD	6
> +	tmp = (mode->htotal * Bpp) * mode->vtotal -
> +	      (hblk + VBLK_PACKET_OVERHEAD);
> +
> +	return (device->lanes - tmp % device->lanes);

This should have a comment explaining why it's needed

> +}
> +
>  static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi,
>  				    struct drm_display_mode *mode)
>  {
> @@ -586,13 +604,8 @@ static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi,
>  			   (mode->htotal - (mode->hsync_end - mode->hsync_start)) * Bpp -
>  			   HBLK_PACKET_OVERHEAD);
>  
> -		/*
> -		 * And I'm not entirely sure what vblk is about. The driver in
> -		 * Allwinner BSP is using a rather convoluted calculation
> -		 * there only for 4 lanes. However, using 0 (the !4 lanes
> -		 * case) even with a 4 lanes screen seems to work...
> -		 */
> -		vblk = 0;
> +		if (device->lanes == 4)

And that can be done in the function itself.

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

Re: [PATCH v9 3/5] drm/sun4i: sun6i_mipi_dsi: Support vblk timing for 4-lane devices

[Jagan Teki]

On Mon, Mar 4, 2019 at 9:19 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> On Sun, Mar 03, 2019 at 11:05:25PM +0530, Jagan Teki wrote:
> > Like other dsi setup timings, or hblk for that matter vblk would
> > also require compute the timings based payload equation along with
> > packet overhead.
> >
> > But, on the other hand vblk computation is also depends on device
> > lane number.
> > - for 4 lane devices, it is computed based on vtotal, packet overhead
> >   along with hblk value.
> > - for others devices, it is simply 0
> >
> > BSP code from BPI-M64-bsp is computing vblk as for 4-lane devices
> > (from linux-sunxi
> > drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c)
> >
> > tmp = (ht*dsi_pixel_bits[format]/8)*vt-(4+dsi_hblk+2);
> > dsi_vblk = (lane-tmp%lane);
> >
> > So, update the vblk timing calculation to support all type of
> > devices.
> >
> > Tested on 2-lane, 4-lane MIPI-DSI LCD panels.
>
> You should be explaining which issue you faced, in which setup, what
> were its symptoms and how that solution is fixing it.

No, it is not a fix, didn't specify either. it is vblk timings support
like others dsi timings.

>
> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > Tested-by: Merlijn Wajer <merlijn@wizzup.org>
> > ---
> >  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 27 +++++++++++++++++++-------
> >  1 file changed, 20 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > index 140e55f5ed2e..b38358465d87 100644
> > --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > @@ -527,6 +527,24 @@ static void sun6i_dsi_setup_format(struct sun6i_dsi *dsi,
> >                    SUN6I_DSI_PIXEL_CTL0_FORMAT(fmt));
> >  }
> >
> > +static u16 sun6i_dsi_get_timings_vblk(struct sun6i_dsi *dsi,
> > +                                   struct drm_display_mode *mode, u16 hblk)
> > +{
> > +     struct mipi_dsi_device *device = dsi->device;
> > +     unsigned int Bpp = mipi_dsi_pixel_format_to_bpp(device->format) / 8;
> > +     int tmp;
> > +
> > +     /*
> > +      * The vertical blank is set using a blanking packet (4 bytes +
> > +      * payload + 2 bytes). Its minimal size is therefore 6 bytes
> > +      */
> > +#define VBLK_PACKET_OVERHEAD 6
> > +     tmp = (mode->htotal * Bpp) * mode->vtotal -
> > +           (hblk + VBLK_PACKET_OVERHEAD);
> > +
> > +     return (device->lanes - tmp % device->lanes);
>
> This should have a comment explaining why it's needed

Grabbed from BSP as I mentioned in the commit message, to satisfy the
proper dsi timing initialization.

>
> > +}
> > +
> >  static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi,
> >                                   struct drm_display_mode *mode)
> >  {
> > @@ -586,13 +604,8 @@ static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi,
> >                          (mode->htotal - (mode->hsync_end - mode->hsync_start)) * Bpp -
> >                          HBLK_PACKET_OVERHEAD);
> >
> > -             /*
> > -              * And I'm not entirely sure what vblk is about. The driver in
> > -              * Allwinner BSP is using a rather convoluted calculation
> > -              * there only for 4 lanes. However, using 0 (the !4 lanes
> > -              * case) even with a 4 lanes screen seems to work...
> > -              */
> > -             vblk = 0;
> > +             if (device->lanes == 4)
>
> And that can be done in the function itself.

Since vblk is initialized to 0 starting of the function, I'm calling
directly this for 4-lane, because for rest it's 0 on non-burst mode.

Re: [PATCH v9 3/5] drm/sun4i: sun6i_mipi_dsi: Support vblk timing for 4-lane devices

[Maxime Ripard]

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

On Thu, Mar 07, 2019 at 09:33:38PM +0530, Jagan Teki wrote:
> On Mon, Mar 4, 2019 at 9:19 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> >
> > On Sun, Mar 03, 2019 at 11:05:25PM +0530, Jagan Teki wrote:
> > > Like other dsi setup timings, or hblk for that matter vblk would
> > > also require compute the timings based payload equation along with
> > > packet overhead.
> > >
> > > But, on the other hand vblk computation is also depends on device
> > > lane number.
> > > - for 4 lane devices, it is computed based on vtotal, packet overhead
> > >   along with hblk value.
> > > - for others devices, it is simply 0
> > >
> > > BSP code from BPI-M64-bsp is computing vblk as for 4-lane devices
> > > (from linux-sunxi
> > > drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c)
> > >
> > > tmp = (ht*dsi_pixel_bits[format]/8)*vt-(4+dsi_hblk+2);
> > > dsi_vblk = (lane-tmp%lane);
> > >
> > > So, update the vblk timing calculation to support all type of
> > > devices.
> > >
> > > Tested on 2-lane, 4-lane MIPI-DSI LCD panels.
> >
> > You should be explaining which issue you faced, in which setup, what
> > were its symptoms and how that solution is fixing it.
> 
> No, it is not a fix, didn't specify either. it is vblk timings support
> like others dsi timings.

So it's not fixing anything, and this isn't a new feature
either. What's the point then?

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

Re: [PATCH v9 3/5] drm/sun4i: sun6i_mipi_dsi: Support vblk timing for 4-lane devices

[Jagan Teki]

On Mon, Mar 11, 2019 at 7:34 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> On Thu, Mar 07, 2019 at 09:33:38PM +0530, Jagan Teki wrote:
> > On Mon, Mar 4, 2019 at 9:19 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > >
> > > On Sun, Mar 03, 2019 at 11:05:25PM +0530, Jagan Teki wrote:
> > > > Like other dsi setup timings, or hblk for that matter vblk would
> > > > also require compute the timings based payload equation along with
> > > > packet overhead.
> > > >
> > > > But, on the other hand vblk computation is also depends on device
> > > > lane number.
> > > > - for 4 lane devices, it is computed based on vtotal, packet overhead
> > > >   along with hblk value.
> > > > - for others devices, it is simply 0
> > > >
> > > > BSP code from BPI-M64-bsp is computing vblk as for 4-lane devices
> > > > (from linux-sunxi
> > > > drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c)
> > > >
> > > > tmp = (ht*dsi_pixel_bits[format]/8)*vt-(4+dsi_hblk+2);
> > > > dsi_vblk = (lane-tmp%lane);
> > > >
> > > > So, update the vblk timing calculation to support all type of
> > > > devices.
> > > >
> > > > Tested on 2-lane, 4-lane MIPI-DSI LCD panels.
> > >
> > > You should be explaining which issue you faced, in which setup, what
> > > were its symptoms and how that solution is fixing it.
> >
> > No, it is not a fix, didn't specify either. it is vblk timings support
> > like others dsi timings.
>
> So it's not fixing anything, and this isn't a new feature
> either. What's the point then?

You can consider a feature, as comments says it's vblk support for
4-lane devices which doesn't be 0. don't know why you are asking like
a first time patch since it's commented before[1]

[1] https://patchwork.kernel.org/patch/10657543/

[PATCH v9 4/5] drm/sun4i: sun6i_mipi_dsi: Support DSI GENERIC_SHORT_WRITE_2 transfer

[Jagan Teki]

Some DSI panels do use GENERIC_SHORT_WRITE_2 transfer protocol to host
DSI driver and which is similar to GENERIC_SHORT_WRITE.

Add support for the same transfer, so-that so-that the panels which are
requesting similar transfer type will process properly.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
Tested-by: Merlijn Wajer <merlijn@wizzup.org>
---
 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
index b38358465d87..31ec4048804d 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
@@ -993,6 +993,7 @@ static ssize_t sun6i_dsi_transfer(struct mipi_dsi_host *host,
 	switch (msg->type) {
 	case MIPI_DSI_DCS_SHORT_WRITE:
 	case MIPI_DSI_DCS_SHORT_WRITE_PARAM:
+	case MIPI_DSI_GENERIC_SHORT_WRITE_2_PARAM:
 		ret = sun6i_dsi_dcs_write_short(dsi, msg);
 		break;
 
-- 
2.18.0.321.gffc6fa0e3

[PATCH v9 5/5] drm/sun4i: sun6i_mipi_dsi: Simplify dsi setup timings code

[Jagan Teki]

DSI timings are varies between burst/non-burst devices and
current driver is handling this support via if, else statements
which would difficult to read.

Simplify it by using goto to make code more readable.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
Tested-by: Merlijn Wajer <merlijn@wizzup.org>
---
Note: This change is created based on previous version burst
changes [1] by addressing comment from [2] by Maxime to make
code readable. 

[1] https://patchwork.kernel.org/cover/10813623/
[2] https://patchwork.kernel.org/patch/10666607/

 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 79 ++++++++++++++------------
 1 file changed, 42 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
index 31ec4048804d..898f32319c2d 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
@@ -550,7 +550,8 @@ static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi,
 {
 	struct mipi_dsi_device *device = dsi->device;
 	unsigned int Bpp = mipi_dsi_pixel_format_to_bpp(device->format) / 8;
-	u16 hbp = 0, hfp = 0, hsa = 0, hblk = 0, vblk = 0;
+	u16 hbp, hfp, hsa, hblk;
+	u16 vblk = 0;
 	u32 basic_ctl = 0;
 	size_t bytes;
 	u8 *buffer;
@@ -558,6 +559,7 @@ static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi,
 	/* Do all timing calculations up front to allocate buffer space */
 
 	if (device->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) {
+		hbp = hfp = hsa = 0;
 		hblk = mode->hdisplay * Bpp;
 		basic_ctl = SUN6I_DSI_BASIC_CTL_VIDEO_BURST |
 			    SUN6I_DSI_BASIC_CTL_HSA_HSE_DIS |
@@ -566,48 +568,51 @@ static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi,
 		if (device->lanes == 4)
 			basic_ctl |= SUN6I_DSI_BASIC_CTL_TRAIL_FILL |
 				     SUN6I_DSI_BASIC_CTL_TRAIL_INV(0xc);
-	} else {
-		/*
-		 * A sync period is composed of a blanking packet (4
-		 * bytes + payload + 2 bytes) and a sync event packet
-		 * (4 bytes). Its minimal size is therefore 10 bytes
-		 */
+
+		goto alloc_buf;
+	}
+
+	/*
+	 * A sync period is composed of a blanking packet (4
+	 * bytes + payload + 2 bytes) and a sync event packet
+	 * (4 bytes). Its minimal size is therefore 10 bytes
+	 */
 #define HSA_PACKET_OVERHEAD	10
-		hsa = max((unsigned int)HSA_PACKET_OVERHEAD,
-			  (mode->hsync_end - mode->hsync_start) * Bpp - HSA_PACKET_OVERHEAD);
-
-		/*
-		 * The backporch is set using a blanking packet (4
-		 * bytes + payload + 2 bytes). Its minimal size is
-		 * therefore 6 bytes
-		 */
+	hsa = max((unsigned int)HSA_PACKET_OVERHEAD,
+		  (mode->hsync_end - mode->hsync_start) * Bpp - HSA_PACKET_OVERHEAD);
+
+	/*
+	 * The backporch is set using a blanking packet (4
+	 * bytes + payload + 2 bytes). Its minimal size is
+	 * therefore 6 bytes
+	 */
 #define HBP_PACKET_OVERHEAD	6
-		hbp = max((unsigned int)HBP_PACKET_OVERHEAD,
-			  (mode->htotal - mode->hsync_end) * Bpp - HBP_PACKET_OVERHEAD);
-
-		/*
-		 * The frontporch is set using a blanking packet (4
-		 * bytes + payload + 2 bytes). Its minimal size is
-		 * therefore 6 bytes
-		 */
+	hbp = max((unsigned int)HBP_PACKET_OVERHEAD,
+		  (mode->htotal - mode->hsync_end) * Bpp - HBP_PACKET_OVERHEAD);
+
+	/*
+	 * The frontporch is set using a blanking packet (4
+	 * bytes + payload + 2 bytes). Its minimal size is
+	 * therefore 6 bytes
+	 */
 #define HFP_PACKET_OVERHEAD	6
-		hfp = max((unsigned int)HFP_PACKET_OVERHEAD,
-			  (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.
-		 */
+	hfp = max((unsigned int)HFP_PACKET_OVERHEAD,
+		  (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.
+	 */
 #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 = max((unsigned int)HBLK_PACKET_OVERHEAD,
+		   (mode->htotal - (mode->hsync_end - mode->hsync_start)) * Bpp -
+		   HBLK_PACKET_OVERHEAD);
 
-		if (device->lanes == 4)
-			vblk = sun6i_dsi_get_timings_vblk(dsi, mode, hblk);
-	}
+	if (device->lanes == 4)
+		vblk = sun6i_dsi_get_timings_vblk(dsi, mode, hblk);
 
+alloc_buf:
 	/* How many bytes do we need to send all payloads? */
 	bytes = max_t(size_t, max(max(hfp, hblk), max(hsa, hbp)), vblk);
 	buffer = kmalloc(bytes, GFP_KERNEL);
-- 
2.18.0.321.gffc6fa0e3

Re: [PATCH v9 5/5] drm/sun4i: sun6i_mipi_dsi: Simplify dsi setup timings code

[Maxime Ripard]

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

On Sun, Mar 03, 2019 at 11:05:27PM +0530, Jagan Teki wrote:
> DSI timings are varies between burst/non-burst devices and
> current driver is handling this support via if, else statements
> which would difficult to read.
> 
> Simplify it by using goto to make code more readable.
> 
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> Tested-by: Merlijn Wajer <merlijn@wizzup.org>
> ---
> Note: This change is created based on previous version burst
> changes [1] by addressing comment from [2] by Maxime to make
> code readable. 
> 
> [1] https://patchwork.kernel.org/cover/10813623/
> [2] https://patchwork.kernel.org/patch/10666607/
> 
>  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 79 ++++++++++++++------------
>  1 file changed, 42 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> index 31ec4048804d..898f32319c2d 100644
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> @@ -550,7 +550,8 @@ static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi,
>  {
>  	struct mipi_dsi_device *device = dsi->device;
>  	unsigned int Bpp = mipi_dsi_pixel_format_to_bpp(device->format) / 8;
> -	u16 hbp = 0, hfp = 0, hsa = 0, hblk = 0, vblk = 0;
> +	u16 hbp, hfp, hsa, hblk;
> +	u16 vblk = 0;
>  	u32 basic_ctl = 0;
>  	size_t bytes;
>  	u8 *buffer;
> @@ -558,6 +559,7 @@ static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi,
>  	/* Do all timing calculations up front to allocate buffer space */
>  
>  	if (device->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) {
> +		hbp = hfp = hsa = 0;

This raises a checkpatch warning and isn't necessary

>  		hblk = mode->hdisplay * Bpp;
>  		basic_ctl = SUN6I_DSI_BASIC_CTL_VIDEO_BURST |
>  			    SUN6I_DSI_BASIC_CTL_HSA_HSE_DIS |
> @@ -566,48 +568,51 @@ static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi,
>  		if (device->lanes == 4)
>  			basic_ctl |= SUN6I_DSI_BASIC_CTL_TRAIL_FILL |
>  				     SUN6I_DSI_BASIC_CTL_TRAIL_INV(0xc);
> -	} else {
> -		/*
> -		 * A sync period is composed of a blanking packet (4
> -		 * bytes + payload + 2 bytes) and a sync event packet
> -		 * (4 bytes). Its minimal size is therefore 10 bytes
> -		 */
> +
> +		goto alloc_buf;

And I'd rather not have a goto in the middle of the code for no
particular reason.

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

Re: [PATCH v9 5/5] drm/sun4i: sun6i_mipi_dsi: Simplify dsi setup timings code

[Jagan Teki]

On Mon, Mar 4, 2019 at 9:20 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> On Sun, Mar 03, 2019 at 11:05:27PM +0530, Jagan Teki wrote:
> > DSI timings are varies between burst/non-burst devices and
> > current driver is handling this support via if, else statements
> > which would difficult to read.
> >
> > Simplify it by using goto to make code more readable.
> >
> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > Tested-by: Merlijn Wajer <merlijn@wizzup.org>
> > ---
> > Note: This change is created based on previous version burst
> > changes [1] by addressing comment from [2] by Maxime to make
> > code readable.
> >
> > [1] https://patchwork.kernel.org/cover/10813623/
> > [2] https://patchwork.kernel.org/patch/10666607/
> >
> >  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 79 ++++++++++++++------------
> >  1 file changed, 42 insertions(+), 37 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > index 31ec4048804d..898f32319c2d 100644
> > --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > @@ -550,7 +550,8 @@ static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi,
> >  {
> >       struct mipi_dsi_device *device = dsi->device;
> >       unsigned int Bpp = mipi_dsi_pixel_format_to_bpp(device->format) / 8;
> > -     u16 hbp = 0, hfp = 0, hsa = 0, hblk = 0, vblk = 0;
> > +     u16 hbp, hfp, hsa, hblk;
> > +     u16 vblk = 0;
> >       u32 basic_ctl = 0;
> >       size_t bytes;
> >       u8 *buffer;
> > @@ -558,6 +559,7 @@ static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi,
> >       /* Do all timing calculations up front to allocate buffer space */
> >
> >       if (device->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) {
> > +             hbp = hfp = hsa = 0;
>
> This raises a checkpatch warning and isn't necessary
>
> >               hblk = mode->hdisplay * Bpp;
> >               basic_ctl = SUN6I_DSI_BASIC_CTL_VIDEO_BURST |
> >                           SUN6I_DSI_BASIC_CTL_HSA_HSE_DIS |
> > @@ -566,48 +568,51 @@ static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi,
> >               if (device->lanes == 4)
> >                       basic_ctl |= SUN6I_DSI_BASIC_CTL_TRAIL_FILL |
> >                                    SUN6I_DSI_BASIC_CTL_TRAIL_INV(0xc);
> > -     } else {
> > -             /*
> > -              * A sync period is composed of a blanking packet (4
> > -              * bytes + payload + 2 bytes) and a sync event packet
> > -              * (4 bytes). Its minimal size is therefore 10 bytes
> > -              */
> > +
> > +             goto alloc_buf;
>
> And I'd rather not have a goto in the middle of the code for no
> particular reason.

OK, will try to think for better simplification otherwise will drop it
in next version.

Re: [PATCH v9 0/5] drm/sun4i: sun6i_mipi_dsi: Fixes/updates

[Jagan Teki]

Hi,

On Sun, Mar 3, 2019 at 11:06 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> Unfortunately due to various reasons[3] the previous fixes[1] and burst[2]
> changes are failed to apply.
>
> So, this series is filtered version of previous [1] and [2] changes on top
> of drm-misc.
>
> patch-1: Fix for burst mode instruction delay computation
>
> patch-2: Fix for TCOn DRQ set bits
>
> patch-3: Support vblk timing for 4-lane devices
>
> patch-4: GENERIC_SHORT_WRITE_2 dsi transfer support
>
> patch-5: Code simplification for dsi timings
>
> Changes for v9:
> - rebase on drm-misc
> - update commit messages
> - add hsync_porch overflow patch
> Changes for v8:
> - rebase on master
> - rework on commit messages
> - rework video start delay
> - include drq changes from previous version
> Changes for v7:
> - rebase on master
> - collect Merlijn Wajer Tested-by credits.
> Changes for v6:
> - fixed all burst mode patches as per previous version comments
> - rebase on master
> - update proper commit message
> - dropped unneeded comments
> - order the patches that make review easy
> Changes for v5, v4, v3, v2:
> - use existing driver code construct for hblk computation
> - create separate function for vblk computation
> - cleanup commit messages
> - update proper commit messages
> - fixed checkpatch warnings/errors
> - use proper return value for tcon0 probe
> - add logic to get tcon0 divider values
> - simplify timings code to support burst mode
> - fix drq computation return values
> - rebase on master
>
> [1] https://patchwork.kernel.org/cover/10813573/
> [2] https://patchwork.kernel.org/cover/10813623/
> [3] https://patchwork.kernel.org/cover/10805995/
>
> Any inputs?
> Jagan.

Any further comments on this series? Can I send next version with some changes?

Re: [PATCH v9 0/5] drm/sun4i: sun6i_mipi_dsi: Fixes/updates

[Maxime Ripard]

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

On Mon, Mar 18, 2019 at 11:58:39PM +0530, Jagan Teki wrote:
> Hi,
>
> On Sun, Mar 3, 2019 at 11:06 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
> >
> > Unfortunately due to various reasons[3] the previous fixes[1] and burst[2]
> > changes are failed to apply.
> >
> > So, this series is filtered version of previous [1] and [2] changes on top
> > of drm-misc.
> >
> > patch-1: Fix for burst mode instruction delay computation
> >
> > patch-2: Fix for TCOn DRQ set bits
> >
> > patch-3: Support vblk timing for 4-lane devices
> >
> > patch-4: GENERIC_SHORT_WRITE_2 dsi transfer support
> >
> > patch-5: Code simplification for dsi timings
> >
> > Changes for v9:
> > - rebase on drm-misc
> > - update commit messages
> > - add hsync_porch overflow patch
> > Changes for v8:
> > - rebase on master
> > - rework on commit messages
> > - rework video start delay
> > - include drq changes from previous version
> > Changes for v7:
> > - rebase on master
> > - collect Merlijn Wajer Tested-by credits.
> > Changes for v6:
> > - fixed all burst mode patches as per previous version comments
> > - rebase on master
> > - update proper commit message
> > - dropped unneeded comments
> > - order the patches that make review easy
> > Changes for v5, v4, v3, v2:
> > - use existing driver code construct for hblk computation
> > - create separate function for vblk computation
> > - cleanup commit messages
> > - update proper commit messages
> > - fixed checkpatch warnings/errors
> > - use proper return value for tcon0 probe
> > - add logic to get tcon0 divider values
> > - simplify timings code to support burst mode
> > - fix drq computation return values
> > - rebase on master
> >
> > [1] https://patchwork.kernel.org/cover/10813573/
> > [2] https://patchwork.kernel.org/cover/10813623/
> > [3] https://patchwork.kernel.org/cover/10805995/
> >
> > Any inputs?
> > Jagan.
>
> Any further comments on this series? Can I send next version with some changes?

Unless you address what we've asked for the very beginning and provide
some accurate description of your problem, and references to why you
think your changes are the right thing to do, it's not worth sending a
new version, the discussion cannot make further progress.

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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