Message ID | 20221209152343.180139-5-jagan@amarulasolutions.com |
---|---|
State | New |
Headers | show |
Series | drm: bridge: Add Samsung MIPI DSIM bridge | expand |
On 12/9/22 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@chromium.org> > Reported-by: Sébastien Szymanski <sebastien.szymanski@armadeus.com> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> Reviewed-by: Marek Vasut <marex@denx.de>
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@chromium.org> > Reported-by: Sébastien Szymanski <sebastien.szymanski@armadeus.com> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > --- > 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. Best regards
On Mon, Dec 12, 2022 at 2:28 PM Marek Szyprowski <m.szyprowski@samsung.com> 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@chromium.org> > > Reported-by: Sébastien Szymanski <sebastien.szymanski@armadeus.com> > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > > --- > > 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@armadeus.com/ Jagan.
On 12.12.2022 10:03, Jagan Teki wrote: > On Mon, Dec 12, 2022 at 2:28 PM Marek Szyprowski > <m.szyprowski@samsung.com> 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@chromium.org> >>> Reported-by: Sébastien Szymanski <sebastien.szymanski@armadeus.com> >>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> >>> --- >>> 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@armadeus.com/ 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; ret = s6e8aa0_parse_dt(ctx); Best regards
On Mon, Dec 12, 2022 at 7:51 PM Marek Szyprowski <m.szyprowski@samsung.com> wrote: > > On 12.12.2022 10:03, Jagan Teki wrote: > > On Mon, Dec 12, 2022 at 2:28 PM Marek Szyprowski > > <m.szyprowski@samsung.com> 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@chromium.org> > >>> Reported-by: Sébastien Szymanski <sebastien.szymanski@armadeus.com> > >>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > >>> --- > >>> 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@armadeus.com/ > > 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.
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) {