When configuring the timing of DSI hosts (interfaces) in
dsi_timing_setup() all values written to registers are taking bonded
DSI into account by dividing the original mode width by 2 (half the
data is sent over each of the two DSI hosts), but the full width
instead of the interface width is passed as hdisplay parameter to
dsi_update_dsc_timing().
Currently only msm_dsc_get_slices_per_intf() is called within
dsi_update_dsc_timing() with the `hdisplay` argument which clearly
documents that it wants the width of a single interface (which, again,
in bonded DSI mode is half the total width of the mode). Thus pass the
bonded-mode-adjusted hdisplay parameter into dsi_update_dsc_timing()
otherwise all values written to registers by this function (i.e. the
number of slices per interface or packet, and derived from this the EOL
byte number) are twice too large.
Inversely the panel driver is expected to only set the slice width and
number of slices for half the panel, i.e. what will be sent by each
host individually, rather than fixing that up like hdisplay here.
Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
Signed-off-by: Marijn Suijten <[email protected]>
---
drivers/gpu/drm/msm/dsi/dsi_host.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index c80be74cf10b..9d0c940dcb28 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -987,7 +987,7 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO) {
if (msm_host->dsc)
- dsi_update_dsc_timing(msm_host, false, mode->hdisplay);
+ dsi_update_dsc_timing(msm_host, false, hdisplay);
dsi_write(msm_host, REG_DSI_ACTIVE_H,
DSI_ACTIVE_H_START(ha_start) |
@@ -1008,7 +1008,7 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
DSI_ACTIVE_VSYNC_VPOS_END(vs_end));
} else { /* command mode */
if (msm_host->dsc)
- dsi_update_dsc_timing(msm_host, true, mode->hdisplay);
+ dsi_update_dsc_timing(msm_host, true, hdisplay);
/* image data and 1 byte write_memory_start cmd */
if (!msm_host->dsc)
--
2.44.0
On Wed, 17 Apr 2024 at 02:57, Marijn Suijten
<[email protected]> wrote:
>
> When configuring the timing of DSI hosts (interfaces) in
> dsi_timing_setup() all values written to registers are taking bonded
> DSI into account by dividing the original mode width by 2 (half the
> data is sent over each of the two DSI hosts), but the full width
> instead of the interface width is passed as hdisplay parameter to
> dsi_update_dsc_timing().
>
> Currently only msm_dsc_get_slices_per_intf() is called within
> dsi_update_dsc_timing() with the `hdisplay` argument which clearly
> documents that it wants the width of a single interface (which, again,
> in bonded DSI mode is half the total width of the mode). Thus pass the
> bonded-mode-adjusted hdisplay parameter into dsi_update_dsc_timing()
> otherwise all values written to registers by this function (i.e. the
> number of slices per interface or packet, and derived from this the EOL
> byte number) are twice too large.
>
> Inversely the panel driver is expected to only set the slice width and
> number of slices for half the panel, i.e. what will be sent by each
> host individually, rather than fixing that up like hdisplay here.
>
> Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
> Signed-off-by: Marijn Suijten <[email protected]>
> ---
> drivers/gpu/drm/msm/dsi/dsi_host.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Dmitry Baryshkov <[email protected]>
--
With best wishes
Dmitry
On 2024-04-17 14:58:25, Dmitry Baryshkov wrote:
> On Wed, 17 Apr 2024 at 02:57, Marijn Suijten
> <[email protected]> wrote:
> >
> > When configuring the timing of DSI hosts (interfaces) in
> > dsi_timing_setup() all values written to registers are taking bonded
> > DSI into account by dividing the original mode width by 2 (half the
> > data is sent over each of the two DSI hosts), but the full width
> > instead of the interface width is passed as hdisplay parameter to
> > dsi_update_dsc_timing().
> >
> > Currently only msm_dsc_get_slices_per_intf() is called within
> > dsi_update_dsc_timing() with the `hdisplay` argument which clearly
> > documents that it wants the width of a single interface (which, again,
> > in bonded DSI mode is half the total width of the mode). Thus pass the
> > bonded-mode-adjusted hdisplay parameter into dsi_update_dsc_timing()
> > otherwise all values written to registers by this function (i.e. the
> > number of slices per interface or packet, and derived from this the EOL
> > byte number) are twice too large.
> >
> > Inversely the panel driver is expected to only set the slice width and
> > number of slices for half the panel, i.e. what will be sent by each
> > host individually, rather than fixing that up like hdisplay here.
> >
> > Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
> > Signed-off-by: Marijn Suijten <[email protected]>
> > ---
> > drivers/gpu/drm/msm/dsi/dsi_host.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
>
> Reviewed-by: Dmitry Baryshkov <[email protected]>
Thanks, it seems this patch has already been picked up for 6.10 [1] to test at
least, but I'd advise you to drop it until I resend it in v2, as it no longer
performs as written in the title.
When I wrote this patch in in June 2023, commit efcbd6f9cdeb ("drm/msm/
dsi: Enable widebus for DSI") from August 2023 wasn't there yet. That patch
updates hdisplay (because it is unused after that point) with the number
of compressed bytes to be sent over each interface, which is effectively
hdisplay (based on slice_count * slice_width, so as explained in the commit
message that corresponds to half the panel width), divided by a compression
ratio of 3 or 6 depending on widebus, thus passing a way too low value into
dsi_update_dsc_timing().
As a result this patch regresses the DSC panel on my SM8150 Sony Xperia 1, and
likely also explains why it was quite hard to get the porches "just right" on
the Xperia 1 III with its dual-DSI dual-DSC 4k@120Hz panel (that these patches
are specifically for).
I'm still thinking of how to best fix that: probably introducing a new separate
local variable, though dsi_update_dsc_timing() only uses it to calculate
the number of slices per interface, which again as written in the commit
description, is currently required to already be for one interface (in other
words, the Xperia 1 with only a single intf sets slice_count=2, but the Xperia 1
III with 2 bonded DSI interfaces sets slice_count=1). Which means that this is
always equivalent to slice_per_intf = dsc->slice_count.
Let me know which approach is preferred.
- Marijn
[1]: https://gitlab.freedesktop.org/drm/msm/-/merge_requests/110
On Sat, Apr 20, 2024 at 12:18:39AM +0200, Marijn Suijten wrote:
> On 2024-04-17 14:58:25, Dmitry Baryshkov wrote:
> > On Wed, 17 Apr 2024 at 02:57, Marijn Suijten
> > <[email protected]> wrote:
> > >
> > > When configuring the timing of DSI hosts (interfaces) in
> > > dsi_timing_setup() all values written to registers are taking bonded
> > > DSI into account by dividing the original mode width by 2 (half the
> > > data is sent over each of the two DSI hosts), but the full width
> > > instead of the interface width is passed as hdisplay parameter to
> > > dsi_update_dsc_timing().
> > >
> > > Currently only msm_dsc_get_slices_per_intf() is called within
> > > dsi_update_dsc_timing() with the `hdisplay` argument which clearly
> > > documents that it wants the width of a single interface (which, again,
> > > in bonded DSI mode is half the total width of the mode). Thus pass the
> > > bonded-mode-adjusted hdisplay parameter into dsi_update_dsc_timing()
> > > otherwise all values written to registers by this function (i.e. the
> > > number of slices per interface or packet, and derived from this the EOL
> > > byte number) are twice too large.
> > >
> > > Inversely the panel driver is expected to only set the slice width and
> > > number of slices for half the panel, i.e. what will be sent by each
> > > host individually, rather than fixing that up like hdisplay here.
> > >
> > > Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
> > > Signed-off-by: Marijn Suijten <[email protected]>
> > > ---
> > > drivers/gpu/drm/msm/dsi/dsi_host.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > Reviewed-by: Dmitry Baryshkov <[email protected]>
>
> Thanks, it seems this patch has already been picked up for 6.10 [1] to test at
> least, but I'd advise you to drop it until I resend it in v2, as it no longer
> performs as written in the title.
Ok, dropping.
>
> When I wrote this patch in in June 2023, commit efcbd6f9cdeb ("drm/msm/
> dsi: Enable widebus for DSI") from August 2023 wasn't there yet. That patch
> updates hdisplay (because it is unused after that point) with the number
> of compressed bytes to be sent over each interface, which is effectively
> hdisplay (based on slice_count * slice_width, so as explained in the commit
> message that corresponds to half the panel width), divided by a compression
> ratio of 3 or 6 depending on widebus, thus passing a way too low value into
> dsi_update_dsc_timing().
>
> As a result this patch regresses the DSC panel on my SM8150 Sony Xperia 1, and
> likely also explains why it was quite hard to get the porches "just right" on
> the Xperia 1 III with its dual-DSI dual-DSC 4k@120Hz panel (that these patches
> are specifically for).
>
> I'm still thinking of how to best fix that: probably introducing a new separate
> local variable, though dsi_update_dsc_timing() only uses it to calculate
> the number of slices per interface, which again as written in the commit
> description, is currently required to already be for one interface (in other
> words, the Xperia 1 with only a single intf sets slice_count=2, but the Xperia 1
> III with 2 bonded DSI interfaces sets slice_count=1). Which means that this is
> always equivalent to slice_per_intf = dsc->slice_count.
>
> Let me know which approach is preferred.
>
> - Marijn
>
> [1]: https://gitlab.freedesktop.org/drm/msm/-/merge_requests/110
--
With best wishes
Dmitry