Re: [PATCH v9 04/18] drm: exynos: dsi: Fix MIPI_DSI*_NO_* mode flags

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Dec 12, 2022 at 7:51 PM Marek Szyprowski
<m.szyprowski@xxxxxxxxxxx> wrote:
>
> On 12.12.2022 10:03, Jagan Teki wrote:
> >   On Mon, Dec 12, 2022 at 2:28 PM Marek Szyprowski
> > <m.szyprowski@xxxxxxxxxxx> wrote:
> >> Hi Jagan,
> >>
> >> On 09.12.2022 16:23, Jagan Teki wrote:
> >>> HFP/HBP/HSA/EOT_PACKET modes in Exynos DSI host specifies
> >>> 0 = Enable and 1 = Disable.
> >>>
> >>> The logic for checking these mode flags was correct before
> >>> the MIPI_DSI*_NO_* mode flag conversion.
> >>>
> >>> Fix the MIPI_DSI*_NO_* mode flags handling.
> >>>
> >>> Fixes: <0f3b68b66a6d> ("drm/dsi: Add _NO_ to MIPI_DSI_* flags disabling
> >>> features")
> >>> Reviewed-by: Nicolas Boichat <drinkcat@xxxxxxxxxxxx>
> >>> Reported-by: Sébastien Szymanski <sebastien.szymanski@xxxxxxxxxxxx>
> >>> Signed-off-by: Jagan Teki <jagan@xxxxxxxxxxxxxxxxxxxx>
> >>> ---
> >>> Changes for v9:
> >>> - none
> >>>
> >>>    drivers/gpu/drm/exynos/exynos_drm_dsi.c | 8 ++++----
> >>>    1 file changed, 4 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> >>> index e5b1540c4ae4..50a2a9ca88a9 100644
> >>> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> >>> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> >>> @@ -805,15 +805,15 @@ static int exynos_dsi_init_link(struct exynos_dsi *dsi)
> >>>                        reg |= DSIM_AUTO_MODE;
> >>>                if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_HSE)
> >>>                        reg |= DSIM_HSE_MODE;
> >>> -             if (!(dsi->mode_flags & MIPI_DSI_MODE_VIDEO_NO_HFP))
> >>> +             if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_NO_HFP)
> >>>                        reg |= DSIM_HFP_MODE;
> >>> -             if (!(dsi->mode_flags & MIPI_DSI_MODE_VIDEO_NO_HBP))
> >>> +             if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_NO_HBP)
> >>>                        reg |= DSIM_HBP_MODE;
> >>> -             if (!(dsi->mode_flags & MIPI_DSI_MODE_VIDEO_NO_HSA))
> >>> +             if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_NO_HSA)
> >>>                        reg |= DSIM_HSA_MODE;
> >>>        }
> >>>
> >>> -     if (!(dsi->mode_flags & MIPI_DSI_MODE_NO_EOT_PACKET))
> >>> +     if (dsi->mode_flags & MIPI_DSI_MODE_NO_EOT_PACKET)
> >>>                reg |= DSIM_EOT_DISABLE;
> >>>
> >>>        switch (dsi->format) {
> >>
> >> Huh, this changes the logic in the driver! I've spent another half of
> >> the night trying to figure out why v8 and v9 doesn't work on all my
> >> Exynos boards with DSI panels again...
> >>
> >> Please drop this patch from this series. If you want to get the Exynos
> >> DSI -> Samsung DSIM conversion merged, please focus on the core patches
> >> and don't add more random 'fixes' to each new version.
> >>
> >> This change has to be discussed separately. The values written by the
> >> Exynos DSI driver to the registers ARE CORRECT and DSI panels work fine
> >> with such configuration. So either everything is correct, or these flags
> >> are reversed both in the Exynos DSI driver AND at least tested DSI
> >> panels (s6e8aa0, s6e3ha2, s6e63j0x03). I would need to check this in
> >> panel datasheets if I manage to get them.
> > This issue was reproduced as part of the series in v7 in i.MX8m
> > platform and reported by Sébastien Szymanski [1] and TRM matches the
> > fix as well.
> >
> > [1] https://lore.kernel.org/all/4c9475d0-f76f-0c59-1208-6e5395496c9e@xxxxxxxxxxxx/
>
> Then please append the following changes to this patch to keep current
> code working:
>
> diff --git a/drivers/gpu/drm/panel/panel-samsung-s6e3ha2.c
> b/drivers/gpu/drm/panel/panel-samsung-s6e3ha2.c
> index 1355b2c27932..2a33602372d9 100644
> --- a/drivers/gpu/drm/panel/panel-samsung-s6e3ha2.c
> +++ b/drivers/gpu/drm/panel/panel-samsung-s6e3ha2.c
> @@ -692,7 +692,9 @@ static int s6e3ha2_probe(struct mipi_dsi_device *dsi)
>
>          dsi->lanes = 4;
>          dsi->format = MIPI_DSI_FMT_RGB888;
> -       dsi->mode_flags = MIPI_DSI_CLOCK_NON_CONTINUOUS;
> +       dsi->mode_flags = MIPI_DSI_CLOCK_NON_CONTINUOUS
> +               | MIPI_DSI_MODE_VIDEO_NO_HFP | MIPI_DSI_MODE_VIDEO_NO_HBP
> +               | MIPI_DSI_MODE_VIDEO_NO_HSA | MIPI_DSI_MODE_NO_EOT_PACKET;
>
>          ctx->supplies[0].supply = "vdd3";
>          ctx->supplies[1].supply = "vci";
> diff --git a/drivers/gpu/drm/panel/panel-samsung-s6e63j0x03.c
> b/drivers/gpu/drm/panel/panel-samsung-s6e63j0x03.c
> index 3223a9d06a50..bb47dbfdd7ee 100644
> --- a/drivers/gpu/drm/panel/panel-samsung-s6e63j0x03.c
> +++ b/drivers/gpu/drm/panel/panel-samsung-s6e63j0x03.c
> @@ -446,7 +446,8 @@ static int s6e63j0x03_probe(struct mipi_dsi_device *dsi)
>
>          dsi->lanes = 1;
>          dsi->format = MIPI_DSI_FMT_RGB888;
> -       dsi->mode_flags = MIPI_DSI_MODE_NO_EOT_PACKET;
> +       dsi->mode_flags = MIPI_DSI_MODE_VIDEO_NO_HFP
> +               | MIPI_DSI_MODE_VIDEO_NO_HBP | MIPI_DSI_MODE_VIDEO_NO_HSA;
>
>          ctx->supplies[0].supply = "vdd3";
>          ctx->supplies[1].supply = "vci";
> diff --git a/drivers/gpu/drm/panel/panel-samsung-s6e8aa0.c
> b/drivers/gpu/drm/panel/panel-samsung-s6e8aa0.c
> index 362eb10f10ce..c51d07ec1529 100644
> --- a/drivers/gpu/drm/panel/panel-samsung-s6e8aa0.c
> +++ b/drivers/gpu/drm/panel/panel-samsung-s6e8aa0.c
> @@ -990,8 +990,6 @@ static int s6e8aa0_probe(struct mipi_dsi_device *dsi)
>          dsi->lanes = 4;
>          dsi->format = MIPI_DSI_FMT_RGB888;
>          dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST
> -               | MIPI_DSI_MODE_VIDEO_NO_HFP | MIPI_DSI_MODE_VIDEO_NO_HBP
> -               | MIPI_DSI_MODE_VIDEO_NO_HSA | MIPI_DSI_MODE_NO_EOT_PACKET
>                  | MIPI_DSI_MODE_VSYNC_FLUSH |
> MIPI_DSI_MODE_VIDEO_AUTO_VERT;

Okay. I think I can send the first 5 patches separately. and then send
the DSIM. Do you think it makes sense? and any chance to apply these
soon?

Jagan.




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux