| Message ID | 624512bb9665e0b15244c2bdf230bcfada3a3b98.1548236066.git-series.maxime.ripard@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/sun4i: dsi: Add burst mode support | expand |
Hi, On Wed, 2019-01-23 at 16:54 +0100, Maxime Ripard wrote: > The current calculation for the video start delay in the current DSI driver > is that it is the total vertical size, minus the backporch and sync length, > plus 1. > > However, the Allwinner code has it as the active vertical size, plus the > back porch and the sync length. This doesn't make any difference on the > only panel it has been tested with so far, since in that particular case > the front porch is equal to the sum of the back porch and sync length. > > This is not the case for all panels, obviously, so we need to fix it. Since > the Allwinner code has a bunch of extra code to deal with out of bounds > values, so let's add them as well. > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> > --- > drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > index 380fc527a707..e3e4ba90c059 100644 > --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > @@ -357,7 +357,12 @@ 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) > { > - return mode->vtotal - (mode->vsync_end - mode->vdisplay) + 1; > + u16 delay = (mode->vsync_end + 1) % mode->vtotal; > + > + if (!delay) > + delay = 1; For increased clarity, you might want to change this last block to: delay = max(delay, 1); Cheers, Paul > + return delay; > } > > static void sun6i_dsi_setup_burst(struct sun6i_dsi *dsi,
On Wed, Jan 23, 2019 at 11:54 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > The current calculation for the video start delay in the current DSI driver > is that it is the total vertical size, minus the backporch and sync length, > plus 1. > > However, the Allwinner code has it as the active vertical size, plus the > back porch and the sync length. This doesn't make any difference on the > only panel it has been tested with so far, since in that particular case > the front porch is equal to the sum of the back porch and sync length. > > This is not the case for all panels, obviously, so we need to fix it. Since > the Allwinner code has a bunch of extra code to deal with out of bounds > values, so let's add them as well. > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> > --- > drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > index 380fc527a707..e3e4ba90c059 100644 > --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > @@ -357,7 +357,12 @@ 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) > { > - return mode->vtotal - (mode->vsync_end - mode->vdisplay) + 1; According to the diagram in include/drm/drm_modes.h , This is active region plus back porch plus 1, as sync_end - display = length of front porch and sync > + u16 delay = (mode->vsync_end + 1) % mode->vtotal; And this actually means (length of active region and front porch and sync pulse plus 1) % total length So I don't really understand what's happening here. And the modulus is unexplained. > + > + if (!delay) > + delay = 1; Same comment as Paul. ChenYu > + > + return delay; > } > > static void sun6i_dsi_setup_burst(struct sun6i_dsi *dsi, > -- > git-series 0.9.1 > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Wed, Jan 30, 2019 at 11:23 AM Chen-Yu Tsai <wens@csie.org> wrote: > > On Wed, Jan 23, 2019 at 11:54 PM Maxime Ripard > <maxime.ripard@bootlin.com> wrote: > > > > The current calculation for the video start delay in the current DSI driver > > is that it is the total vertical size, minus the backporch and sync length, > > plus 1. > > > > However, the Allwinner code has it as the active vertical size, plus the > > back porch and the sync length. This doesn't make any difference on the > > only panel it has been tested with so far, since in that particular case > > the front porch is equal to the sum of the back porch and sync length. > > > > This is not the case for all panels, obviously, so we need to fix it. Since > > the Allwinner code has a bunch of extra code to deal with out of bounds > > values, so let's add them as well. > > > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> > > --- > > drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > > index 380fc527a707..e3e4ba90c059 100644 > > --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > > +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > > @@ -357,7 +357,12 @@ 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) > > { > > - return mode->vtotal - (mode->vsync_end - mode->vdisplay) + 1; > > According to the diagram in include/drm/drm_modes.h , > This is active region plus back porch plus 1, as > > sync_end - display = length of front porch and sync > > > + u16 delay = (mode->vsync_end + 1) % mode->vtotal; > > And this actually means > > (length of active region and front porch and sync pulse plus 1) % > total length > > So I don't really understand what's happening here. And the modulus > is unexplained. Attempting to make sense of this. Allwinner's A64 BSP the following which uses their definitions for y, vbp, vt: s32 start_delay = panel->lcd_vt - panel->lcd_y -10; u32 vfp = panel->lcd_vt - panel->lcd_y - panel->lcd_vbp; u32 dsi_start_delay; /* put start_delay to tcon. set ready sync early to dramfreq, so set start_delay 1 */ start_delay = 1; dsi_start_delay = panel->lcd_vt - vfp + start_delay; if (dsi_start_delay > panel->lcd_vt) dsi_start_delay -= panel->lcd_vt; if (dsi_start_delay==0) dsi_start_delay = 1; This can be converted to dsi_start_delay = max(1, (mode->vtotal - (mode->vtotal - mode->vdisplay - (mode->vtotal - mode->vsync_start)) + 1) % mode->vtotal) and simplified to dsi_start_delay = max(1, (mode->vtotal + mode->vdisplay - mode->vsync_start) + 1) % mode->vtotal) and reordered to the following dsi_start_delay = max(1, (mode->vtotal - (mode->vsync_start - mode->vdisplay) + 1) % mode->vtotal) which means total length minus front porch, or length of active portion plus back porch plus sync. Based on your commit message, the last derivation is what you want. ChenYu > > + > > + if (!delay) > > + delay = 1; > > Same comment as Paul. > > ChenYu > > > + > > + return delay; > > } > > > > static void sun6i_dsi_setup_burst(struct sun6i_dsi *dsi, > > -- > > git-series 0.9.1 > > > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Chen-Yu, On Wed, Feb 06, 2019 at 12:48:21AM +0800, Chen-Yu Tsai wrote: > On Wed, Jan 30, 2019 at 11:23 AM Chen-Yu Tsai <wens@csie.org> wrote: > > > > On Wed, Jan 23, 2019 at 11:54 PM Maxime Ripard > > <maxime.ripard@bootlin.com> wrote: > > > > > > The current calculation for the video start delay in the current DSI driver > > > is that it is the total vertical size, minus the backporch and sync length, > > > plus 1. > > > > > > However, the Allwinner code has it as the active vertical size, plus the > > > back porch and the sync length. This doesn't make any difference on the > > > only panel it has been tested with so far, since in that particular case > > > the front porch is equal to the sum of the back porch and sync length. > > > > > > This is not the case for all panels, obviously, so we need to fix it. Since > > > the Allwinner code has a bunch of extra code to deal with out of bounds > > > values, so let's add them as well. > > > > > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> > > > --- > > > drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 7 ++++++- > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > > > index 380fc527a707..e3e4ba90c059 100644 > > > --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > > > +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > > > @@ -357,7 +357,12 @@ 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) > > > { > > > - return mode->vtotal - (mode->vsync_end - mode->vdisplay) + 1; > > > > According to the diagram in include/drm/drm_modes.h , > > This is active region plus back porch plus 1, as > > > > sync_end - display = length of front porch and sync > > > > > + u16 delay = (mode->vsync_end + 1) % mode->vtotal; > > > > And this actually means > > > > (length of active region and front porch and sync pulse plus 1) % > > total length > > > > So I don't really understand what's happening here. And the modulus > > is unexplained. > > Attempting to make sense of this. Sorry for dragging you into this > Allwinner's A64 BSP the following which uses their definitions for > y, vbp, vt: > > s32 start_delay = panel->lcd_vt - panel->lcd_y -10; > u32 vfp = panel->lcd_vt - panel->lcd_y - panel->lcd_vbp; > u32 dsi_start_delay; > > /* put start_delay to tcon. set ready sync early to dramfreq, so > set start_delay 1 */ > start_delay = 1; > > dsi_start_delay = panel->lcd_vt - vfp + start_delay; > if (dsi_start_delay > panel->lcd_vt) > dsi_start_delay -= panel->lcd_vt; > if (dsi_start_delay==0) > dsi_start_delay = 1; > > This can be converted to > > dsi_start_delay = max(1, (mode->vtotal - (mode->vtotal - > mode->vdisplay - (mode->vtotal - mode->vsync_start)) + 1) % > mode->vtotal) > > and simplified to > > dsi_start_delay = max(1, (mode->vtotal + mode->vdisplay - > mode->vsync_start) + 1) % mode->vtotal) > > and reordered to the following > > dsi_start_delay = max(1, (mode->vtotal - (mode->vsync_start - > mode->vdisplay) + 1) % mode->vtotal) Where did you find that Allwinner's back porch is including the sync length? If we treat the bp as vtotal - vsync_end as we should (and yet I often fail to), then we end up with (leaving out the modulo for now): dsi_start_delay = max(1, mode->vtotal - (mode->vtotal - mode->vdisplay - (mode->vtotal - mode->vsync_end)) + 1) which equals (if we remove the first mode->vtotal - mode->vtotal) dsi_start_delay = max(1, mode->vdisplay + (mode->vtotal - mode->vsync_end) + 1) Otherwise, Then yeah, I end up with the same results than you, and on the Rondo display I have, the start delay difference is of one between the formula in this patch and the one you came up with, so I think we're good. The BPI-M2M display timings would need to be adjusted as well, since the timings were created from the FEX file and the BP was assumed to be only the BP. We don't have the datasheet for this one though, so it's difficult to check. Thanks! Maxime
On Wed, Feb 6, 2019 at 10:12 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > Hi Chen-Yu, > > On Wed, Feb 06, 2019 at 12:48:21AM +0800, Chen-Yu Tsai wrote: > > On Wed, Jan 30, 2019 at 11:23 AM Chen-Yu Tsai <wens@csie.org> wrote: > > > > > > On Wed, Jan 23, 2019 at 11:54 PM Maxime Ripard > > > <maxime.ripard@bootlin.com> wrote: > > > > > > > > The current calculation for the video start delay in the current DSI driver > > > > is that it is the total vertical size, minus the backporch and sync length, > > > > plus 1. > > > > > > > > However, the Allwinner code has it as the active vertical size, plus the > > > > back porch and the sync length. This doesn't make any difference on the > > > > only panel it has been tested with so far, since in that particular case > > > > the front porch is equal to the sum of the back porch and sync length. > > > > > > > > This is not the case for all panels, obviously, so we need to fix it. Since > > > > the Allwinner code has a bunch of extra code to deal with out of bounds > > > > values, so let's add them as well. > > > > > > > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> > > > > --- > > > > drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 7 ++++++- > > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > > > > index 380fc527a707..e3e4ba90c059 100644 > > > > --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > > > > +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > > > > @@ -357,7 +357,12 @@ 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) > > > > { > > > > - return mode->vtotal - (mode->vsync_end - mode->vdisplay) + 1; > > > > > > According to the diagram in include/drm/drm_modes.h , > > > This is active region plus back porch plus 1, as > > > > > > sync_end - display = length of front porch and sync > > > > > > > + u16 delay = (mode->vsync_end + 1) % mode->vtotal; > > > > > > And this actually means > > > > > > (length of active region and front porch and sync pulse plus 1) % > > > total length > > > > > > So I don't really understand what's happening here. And the modulus > > > is unexplained. > > > > Attempting to make sense of this. > > Sorry for dragging you into this > > > Allwinner's A64 BSP the following which uses their definitions for > > y, vbp, vt: > > > > s32 start_delay = panel->lcd_vt - panel->lcd_y -10; > > u32 vfp = panel->lcd_vt - panel->lcd_y - panel->lcd_vbp; > > u32 dsi_start_delay; > > > > /* put start_delay to tcon. set ready sync early to dramfreq, so > > set start_delay 1 */ > > start_delay = 1; > > > > dsi_start_delay = panel->lcd_vt - vfp + start_delay; > > if (dsi_start_delay > panel->lcd_vt) > > dsi_start_delay -= panel->lcd_vt; > > if (dsi_start_delay==0) > > dsi_start_delay = 1; > > > > This can be converted to > > > > dsi_start_delay = max(1, (mode->vtotal - (mode->vtotal - > > mode->vdisplay - (mode->vtotal - mode->vsync_start)) + 1) % > > mode->vtotal) > > > > and simplified to > > > > dsi_start_delay = max(1, (mode->vtotal + mode->vdisplay - > > mode->vsync_start) + 1) % mode->vtotal) > > > > and reordered to the following > > > > dsi_start_delay = max(1, (mode->vtotal - (mode->vsync_start - > > mode->vdisplay) + 1) % mode->vtotal) > > Where did you find that Allwinner's back porch is including the sync > length? I believe that was the case with the TCON? And since Allwinner's code take the values from the FEX files directly for both TCON and DSI, it should be the same. There's also some dead code in the BSP that alludes to it, such as the function tcon1_cfg_ex() in drivers/video/de/lowlevel_sun50iw1/de_lcd.c from the A64 BSP. > If we treat the bp as vtotal - vsync_end as we should (and yet I often > fail to), then we end up with (leaving out the modulo for now): > > dsi_start_delay = max(1, mode->vtotal - (mode->vtotal - mode->vdisplay - > (mode->vtotal - mode->vsync_end)) + 1) > > which equals (if we remove the first mode->vtotal - mode->vtotal) > > dsi_start_delay = max(1, mode->vdisplay + (mode->vtotal - mode->vsync_end) + 1) > > Otherwise, > > Then yeah, I end up with the same results than you, and on the Rondo > display I have, the start delay difference is of one between the > formula in this patch and the one you came up with, so I think we're > good. > > The BPI-M2M display timings would need to be adjusted as well, since > the timings were created from the FEX file and the BP was assumed to > be only the BP. We don't have the datasheet for this one though, so > it's difficult to check. Let's just see if it still behaves correctly. ChenYu
diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c index 380fc527a707..e3e4ba90c059 100644 --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c @@ -357,7 +357,12 @@ 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) { - return mode->vtotal - (mode->vsync_end - mode->vdisplay) + 1; + u16 delay = (mode->vsync_end + 1) % mode->vtotal; + + if (!delay) + delay = 1; + + return delay; } static void sun6i_dsi_setup_burst(struct sun6i_dsi *dsi,
The current calculation for the video start delay in the current DSI driver is that it is the total vertical size, minus the backporch and sync length, plus 1. However, the Allwinner code has it as the active vertical size, plus the back porch and the sync length. This doesn't make any difference on the only panel it has been tested with so far, since in that particular case the front porch is equal to the sum of the back porch and sync length. This is not the case for all panels, obviously, so we need to fix it. Since the Allwinner code has a bunch of extra code to deal with out of bounds values, so let's add them as well. Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> --- drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)