| Message ID | 6e5f72e68f47ca0223877464bf12f0c3f3978de8.1549619502.git-series.maxime.ripard@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/sun4i: dsi: Add burst mode support | expand |
Hi Maxime, On Fri, Feb 8, 2019 at 3:23 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 front porch and sync length, > plus 1. This equals to the active vertical size plus the back porch plus 1. > > That 1 is coming in the Allwinner BSP from an variable that is set to 1. > However, if we look at the Allwinner BSP more closely, and especially in > the "legacy" code for the display (in drivers/video/sunxi/legacy/), we can > see that this variable is actually computed from the porches and the sync > minus 10, clamped between 8 and 100. > > This fixes the start delay symptom we've seen on some panels (vblank > timeouts with vertical white stripes at the bottom of the panel). > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> > --- > drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 4 +++- > 1 file changed, 3 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..9471fa695ec7 100644 > --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > @@ -357,7 +357,9 @@ 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 start = clamp(mode->vtotal - mode->vdisplay - 10, 8, 100); > + > + return mode->vtotal - (mode->vsync_end - mode->vdisplay) + start; > } As you stated in commit message about result as "active vertical size + Back porch" which indeed correct in reverse way of previous code. but the change seems incorrect to me. => mode->vtotal - (mode->vsync_end - mode->vdisplay) + start => mode->vtotal - (mode->vsync_end - mode->vdisplay) + (mode->vtotal - mode->vdisplay) # bypass 10 for now => mode->vtotal - mode->vsync_end + mode->vdisplay + mode->vtotal - mode->vdisplay => (mode->vtotal - mode->vsync_end) + mode->vtotal => Back porch + mode->vtotal but not => Back porch + mode->vdisplay Let me know if I'm wrong?
On Thu, Mar 07, 2019 at 11:24:01PM +0530, Jagan Teki wrote: > Hi Maxime, > > On Fri, Feb 8, 2019 at 3:23 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 front porch and sync length, > > plus 1. This equals to the active vertical size plus the back porch plus 1. > > > > That 1 is coming in the Allwinner BSP from an variable that is set to 1. > > However, if we look at the Allwinner BSP more closely, and especially in > > the "legacy" code for the display (in drivers/video/sunxi/legacy/), we can > > see that this variable is actually computed from the porches and the sync > > minus 10, clamped between 8 and 100. > > > > This fixes the start delay symptom we've seen on some panels (vblank > > timeouts with vertical white stripes at the bottom of the panel). > > > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> > > --- > > drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 4 +++- > > 1 file changed, 3 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..9471fa695ec7 100644 > > --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > > +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > > @@ -357,7 +357,9 @@ 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 start = clamp(mode->vtotal - mode->vdisplay - 10, 8, 100); > > + > > + return mode->vtotal - (mode->vsync_end - mode->vdisplay) + start; > > } > > As you stated in commit message about result as "active vertical size > + Back porch" which indeed correct in reverse way of previous code. > but the change seems incorrect to me. > > => mode->vtotal - (mode->vsync_end - mode->vdisplay) + start > => mode->vtotal - (mode->vsync_end - mode->vdisplay) + (mode->vtotal - > mode->vdisplay) # bypass 10 for now > => mode->vtotal - mode->vsync_end + mode->vdisplay + mode->vtotal - > mode->vdisplay > => (mode->vtotal - mode->vsync_end) + mode->vtotal > => Back porch + mode->vtotal > > but not > => Back porch + mode->vdisplay Why do you think it should be this result? Maxime
On Mon, Mar 11, 2019 at 7:42 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > On Thu, Mar 07, 2019 at 11:24:01PM +0530, Jagan Teki wrote: > > Hi Maxime, > > > > On Fri, Feb 8, 2019 at 3:23 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 front porch and sync length, > > > plus 1. This equals to the active vertical size plus the back porch plus 1. > > > > > > That 1 is coming in the Allwinner BSP from an variable that is set to 1. > > > However, if we look at the Allwinner BSP more closely, and especially in > > > the "legacy" code for the display (in drivers/video/sunxi/legacy/), we can > > > see that this variable is actually computed from the porches and the sync > > > minus 10, clamped between 8 and 100. > > > > > > This fixes the start delay symptom we've seen on some panels (vblank > > > timeouts with vertical white stripes at the bottom of the panel). > > > > > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> > > > --- > > > drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 4 +++- > > > 1 file changed, 3 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..9471fa695ec7 100644 > > > --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > > > +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > > > @@ -357,7 +357,9 @@ 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 start = clamp(mode->vtotal - mode->vdisplay - 10, 8, 100); > > > + > > > + return mode->vtotal - (mode->vsync_end - mode->vdisplay) + start; > > > } > > > > As you stated in commit message about result as "active vertical size > > + Back porch" which indeed correct in reverse way of previous code. > > but the change seems incorrect to me. > > > > => mode->vtotal - (mode->vsync_end - mode->vdisplay) + start > > => mode->vtotal - (mode->vsync_end - mode->vdisplay) + (mode->vtotal - > > mode->vdisplay) # bypass 10 for now > > => mode->vtotal - mode->vsync_end + mode->vdisplay + mode->vtotal - > > mode->vdisplay > > => (mode->vtotal - mode->vsync_end) + mode->vtotal > > => Back porch + mode->vtotal > > > > but not > > => Back porch + mode->vdisplay > > Why do you think it should be this result? This is what the commit messages mentioned. which indeed true. "The current calculation for the video start delay in the current DSI driver is that it is the total vertical size, minus the front porch and sync length, plus 1. This equals to the active vertical size plus the back porch plus 1." But the result of the logic is => Back porch + mode->vtotal
On Mon, Mar 11, 2019 at 08:07:29PM +0530, Jagan Teki wrote: > On Mon, Mar 11, 2019 at 7:42 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > > > On Thu, Mar 07, 2019 at 11:24:01PM +0530, Jagan Teki wrote: > > > Hi Maxime, > > > > > > On Fri, Feb 8, 2019 at 3:23 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 front porch and sync length, > > > > plus 1. This equals to the active vertical size plus the back porch plus 1. > > > > > > > > That 1 is coming in the Allwinner BSP from an variable that is set to 1. > > > > However, if we look at the Allwinner BSP more closely, and especially in > > > > the "legacy" code for the display (in drivers/video/sunxi/legacy/), we can > > > > see that this variable is actually computed from the porches and the sync > > > > minus 10, clamped between 8 and 100. > > > > > > > > This fixes the start delay symptom we've seen on some panels (vblank > > > > timeouts with vertical white stripes at the bottom of the panel). > > > > > > > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> > > > > --- > > > > drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 4 +++- > > > > 1 file changed, 3 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..9471fa695ec7 100644 > > > > --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > > > > +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > > > > @@ -357,7 +357,9 @@ 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 start = clamp(mode->vtotal - mode->vdisplay - 10, 8, 100); > > > > + > > > > + return mode->vtotal - (mode->vsync_end - mode->vdisplay) + start; > > > > } > > > > > > As you stated in commit message about result as "active vertical size > > > + Back porch" which indeed correct in reverse way of previous code. > > > but the change seems incorrect to me. > > > > > > => mode->vtotal - (mode->vsync_end - mode->vdisplay) + start > > > => mode->vtotal - (mode->vsync_end - mode->vdisplay) + (mode->vtotal - > > > mode->vdisplay) # bypass 10 for now > > > => mode->vtotal - mode->vsync_end + mode->vdisplay + mode->vtotal - > > > mode->vdisplay > > > => (mode->vtotal - mode->vsync_end) + mode->vtotal > > > => Back porch + mode->vtotal > > > > > > but not > > > => Back porch + mode->vdisplay > > > > Why do you think it should be this result? > > This is what the commit messages mentioned. which indeed true. > > "The current calculation for the video start delay in the current DSI driver > is that it is the total vertical size, minus the front porch and sync length, > plus 1. This equals to the active vertical size plus the back porch plus 1." > > But the result of the logic is > => Back porch + mode->vtotal Again, that commit was assuming that the back porch contained the sync period, which doesn't seem to be the case. So I'll ask again, what makes you say this isn't true? Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c index 380fc527a707..9471fa695ec7 100644 --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c @@ -357,7 +357,9 @@ 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 start = clamp(mode->vtotal - mode->vdisplay - 10, 8, 100); + + return mode->vtotal - (mode->vsync_end - mode->vdisplay) + start; } 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 front porch and sync length, plus 1. This equals to the active vertical size plus the back porch plus 1. That 1 is coming in the Allwinner BSP from an variable that is set to 1. However, if we look at the Allwinner BSP more closely, and especially in the "legacy" code for the display (in drivers/video/sunxi/legacy/), we can see that this variable is actually computed from the porches and the sync minus 10, clamped between 8 and 100. This fixes the start delay symptom we've seen on some panels (vblank timeouts with vertical white stripes at the bottom of the panel). Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> --- drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)