Re: [PATCH 2/4] drm/sun4i: dsi: Change the start delay calculation - Chen-Yu Tsai

From: Chen-Yu Tsai <wens@csie.org>
To: Maxime Ripard <maxime.ripard@bootlin.com>
Cc: bbrezillon@kernel.org,
	Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	Paul Kocialkowski <paul.kocialkowski@bootlin.com>,
	Sean Paul <seanpaul@chromium.org>,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	Jagan Teki <jagan@openedev.com>,
	linux-arm-kernel <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH 2/4] drm/sun4i: dsi: Change the start delay calculation
Date: Wed, 30 Jan 2019 11:23:23 +0800	[thread overview]
Message-ID: <CAGb2v6702pRt-=SrOLqfxO5CBQT+bxFgQ6fugA=4SP_vWdqcBw@mail.gmail.com> (raw)
In-Reply-To: <624512bb9665e0b15244c2bdf230bcfada3a3b98.1548236066.git-series.maxime.ripard@bootlin.com>

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel