This preliminary Display Stream Compression support package for
(initially tested on) sm8[12]50 is based on comparing DSC behaviour
between downstream and mainline. Some new callbacks are added (for
binding blocks on active CTLs), logic bugs are corrected, zeroed struct
members are now assigned proper values, and RM allocation and hw block
retrieval now hand out (or not) DSC blocks without causing null-pointer
dereferences.
Unfortunately it is not yet enough to get rid of completely corrupted
display output on the boards I tested here:
- Sony Xperia 1 (sm8150), 1644x3840 or 1096x2560 pixels;
- Sony Xperia 5II (sm8250), 1080x2520, at 60 or 120Hz;
- (can include more Xperia boards if desired)
Both devices use the DUALPIPE_DSCMERGE topology downstream: dual LM, PP
and DSC, but only a single INTF/encoder/DSI-link.
Hopefully this spawns some community/upstream interest to help rootcause
our corruption issues (after we open a drm/msm report on GitLab for more
appropriate tracking).
The Sony Xperia XZ3 (sdm845) was fully tested and validated with this
series to not cause any regressions (an one of the math fixes now allows
us to change slice_count in the panel driver, which would corrupt
previously).
Marijn Suijten (6):
drm/msm/dpu1: Implement DSC binding to PP block for CTL V1
drm/msm/dpu1: Add DSC config for sm8150 and sm8250
drm/msm/dpu1: Wire up DSC mask for active CTL configuration
drm/msm/dsi: Use DSC slice(s) packet size to compute word count
drm/msm/dsi: Flip greater-than check for slice_count and
slice_per_intf
drm/msm/dpu: Disallow unallocated (DSC) resources to be returned
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 3 +++
.../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 1 +
.../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 1 +
.../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 2 ++
.../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 23 +++++++++++-----
.../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 9 +++++++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c | 27 +++++++++++++++++++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h | 4 +++
drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 10 +++++++
drivers/gpu/drm/msm/dsi/dsi_host.c | 6 ++---
10 files changed, 77 insertions(+), 9 deletions(-)
--
2.38.1
Active CTLs have to configure what DSC block(s) have to be enabled, and
what DSC block(s) have to be flushed; this value was initialized to zero
resulting in the necessary register writes to never happen (or would
write zero otherwise). This seems to have gotten lost in the DSC v4->v5
series while refactoring how the combination with merge_3d was handled.
Fixes: 58dca9810749 ("drm/msm/disp/dpu1: Add support for DSC in encoder")
Signed-off-by: Marijn Suijten <[email protected]>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 1 +
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 1 +
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 2 ++
3 files changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
index ae28b2b93e69..35791f93c33d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
@@ -61,6 +61,7 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
intf_cfg.intf_mode_sel = DPU_CTL_MODE_SEL_CMD;
intf_cfg.stream_sel = cmd_enc->stream_sel;
intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc);
+ intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
ctl->ops.setup_intf_cfg(ctl, &intf_cfg);
/* setup which pp blk will connect to this intf */
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
index 0f71e8fe7be7..9ee3a7306a5f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
@@ -274,6 +274,7 @@ static void dpu_encoder_phys_vid_setup_timing_engine(
intf_cfg.intf_mode_sel = DPU_CTL_MODE_SEL_VID;
intf_cfg.stream_sel = 0; /* Don't care value for video mode */
intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc);
+ intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
if (phys_enc->hw_pp->merge_3d)
intf_cfg.merge_3d = phys_enc->hw_pp->merge_3d->idx;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
index 7cbcef6efe17..92ddf9995b37 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
@@ -209,6 +209,7 @@ static void dpu_encoder_phys_wb_setup_cdp(struct dpu_encoder_phys *phys_enc)
intf_cfg.intf = DPU_NONE;
intf_cfg.wb = hw_wb->idx;
+ intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
if (mode_3d && hw_pp && hw_pp->merge_3d)
intf_cfg.merge_3d = hw_pp->merge_3d->idx;
@@ -230,6 +231,7 @@ static void dpu_encoder_phys_wb_setup_cdp(struct dpu_encoder_phys *phys_enc)
intf_cfg.wb = hw_wb->idx;
intf_cfg.mode_3d =
dpu_encoder_helper_get_3d_blend_mode(phys_enc);
+ intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
phys_enc->hw_ctl->ops.setup_intf_cfg(phys_enc->hw_ctl, &intf_cfg);
}
}
--
2.38.1
According to downstream the value to use for WORD_COUNT is
bytes_per_pkt, which denotes the number of bytes in a packet based on
how many slices have been configured by the panel driver times the
width of a slice times the number of bytes per pixel.
The DSC panels seen thus far use one byte per pixel, only one slice
per packet, and a slice width of half the panel width leading to the
desired bytes_per_pkt+1 value to be equal to hdisplay/2+1. This however
isn't the case anymore for panels that configure two slices per packet,
where the value should now be hdisplay+1.
Note that the aforementioned panel (on a Sony Xperia XZ3, sdm845) with
slice_count=1 has also been tested to successfully accept slice_count=2,
which would have shown corrupted output previously.
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 | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index b83cf70b1adb..0686c35a6fd4 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -989,7 +989,7 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
if (!msm_host->dsc)
wc = hdisplay * dsi_get_bpp(msm_host->format) / 8 + 1;
else
- wc = mode->hdisplay / 2 + 1;
+ wc = msm_host->dsc->slice_chunk_size * msm_host->dsc->slice_count + 1;
dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_CTRL,
DSI_CMD_MDP_STREAM0_CTRL_WORD_COUNT(wc) |
--
2.38.1
According to downstream /and the comment copied from it/ this comparison
should be the other way around. In other words, when the panel driver
requests to use more slices per packet than what could be sent over this
interface, it is bumped down to only use a single slice per packet (and
strangely not the number of slices that could fit on the interface).
Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
---
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 0686c35a6fd4..9bdfa0864cdf 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -855,11 +855,11 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
*/
slice_per_intf = DIV_ROUND_UP(hdisplay, dsc->slice_width);
- /* If slice_per_pkt is greater than slice_per_intf
+ /* If slice_count is greater than slice_per_intf
* then default to 1. This can happen during partial
* update.
*/
- if (slice_per_intf > dsc->slice_count)
+ if (dsc->slice_count > slice_per_intf)
dsc->slice_count = 1;
total_bytes_per_intf = dsc->slice_chunk_size * slice_per_intf;
--
2.38.1
These blocks on CTL V1 support setting a PINGPONG idx to send pixel
output to.
Signed-off-by: Marijn Suijten <[email protected]>
---
.../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 23 ++++++++++++++-----
1 file changed, 17 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
index 318f0b4dbf6e..114ad8ca4554 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -1566,18 +1566,25 @@ static const struct dpu_merge_3d_cfg sm8150_merge_3d[] = {
/*************************************************************
* DSC sub blocks config
*************************************************************/
-#define DSC_BLK(_name, _id, _base) \
+#define DSC_BLK(_name, _id, _base, _features) \
{\
.name = _name, .id = _id, \
.base = _base, .len = 0x140, \
- .features = 0, \
+ .features = _features, \
}
static struct dpu_dsc_cfg sdm845_dsc[] = {
- DSC_BLK("dsc_0", DSC_0, 0x80000),
- DSC_BLK("dsc_1", DSC_1, 0x80400),
- DSC_BLK("dsc_2", DSC_2, 0x80800),
- DSC_BLK("dsc_3", DSC_3, 0x80c00),
+ DSC_BLK("dsc_0", DSC_0, 0x80000, 0),
+ DSC_BLK("dsc_1", DSC_1, 0x80400, 0),
+ DSC_BLK("dsc_2", DSC_2, 0x80800, 0),
+ DSC_BLK("dsc_3", DSC_3, 0x80c00, 0),
+};
+
+static struct dpu_dsc_cfg sm8150_dsc[] = {
+ DSC_BLK("dsc_0", DSC_0, 0x80000, BIT(DPU_DSC_OUTPUT_CTRL)),
+ DSC_BLK("dsc_1", DSC_1, 0x80400, BIT(DPU_DSC_OUTPUT_CTRL)),
+ DSC_BLK("dsc_2", DSC_2, 0x80800, BIT(DPU_DSC_OUTPUT_CTRL)),
+ DSC_BLK("dsc_3", DSC_3, 0x80c00, BIT(DPU_DSC_OUTPUT_CTRL)),
};
/*************************************************************
@@ -2474,6 +2481,8 @@ static const struct dpu_mdss_cfg sm8150_dpu_cfg = {
.mixer = sm8150_lm,
.dspp_count = ARRAY_SIZE(sm8150_dspp),
.dspp = sm8150_dspp,
+ .dsc_count = ARRAY_SIZE(sm8150_dsc),
+ .dsc = sm8150_dsc,
.pingpong_count = ARRAY_SIZE(sm8150_pp),
.pingpong = sm8150_pp,
.merge_3d_count = ARRAY_SIZE(sm8150_merge_3d),
@@ -2524,6 +2533,8 @@ static const struct dpu_mdss_cfg sm8250_dpu_cfg = {
.mixer = sm8150_lm,
.dspp_count = ARRAY_SIZE(sm8150_dspp),
.dspp = sm8150_dspp,
+ .dsc_count = ARRAY_SIZE(sm8150_dsc),
+ .dsc = sm8150_dsc,
.pingpong_count = ARRAY_SIZE(sm8150_pp),
.pingpong = sm8150_pp,
.merge_3d_count = ARRAY_SIZE(sm8150_merge_3d),
--
2.38.1
All V1 CTL blocks (active CTLs) explicitly bind the pixel output from a
DSC block to a PINGPONG block by setting the PINGPONG idx in a DSC
hardware register.
Signed-off-by: Marijn Suijten <[email protected]>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 3 +++
.../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 9 +++++++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c | 27 +++++++++++++++++++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h | 4 +++
4 files changed, 43 insertions(+)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 9c6817b5a194..c17ac85eb447 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1830,6 +1830,9 @@ static void dpu_encoder_dsc_pipe_cfg(struct dpu_hw_dsc *hw_dsc,
if (hw_pp->ops.setup_dsc)
hw_pp->ops.setup_dsc(hw_pp);
+ if (hw_dsc->ops.dsc_bind_pingpong_blk)
+ hw_dsc->ops.dsc_bind_pingpong_blk(hw_dsc, true, hw_pp->idx);
+
if (hw_pp->ops.enable_dsc)
hw_pp->ops.enable_dsc(hw_pp);
}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
index c160dae95a69..96f849907aa2 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
@@ -268,6 +268,15 @@ enum {
DPU_VBIF_MAX
};
+/**
+ * DSC features
+ * @DPU_DSC_OUTPUT_CTRL Configure which PINGPONG block gets
+ * the pixel output from this DSC.
+ */
+enum {
+ DPU_DSC_OUTPUT_CTRL = 0x1,
+};
+
/**
* MACRO DPU_HW_BLK_INFO - information of HW blocks inside DPU
* @name: string name for debug purposes
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
index 3662df698dae..619926da1441 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
@@ -29,6 +29,8 @@
#define DSC_RANGE_MAX_QP 0x0B0
#define DSC_RANGE_BPG_OFFSET 0x0EC
+#define DSC_CTL(m) (0x1800 - 0x3FC * (m - DSC_0))
+
static void dpu_hw_dsc_disable(struct dpu_hw_dsc *dsc)
{
struct dpu_hw_blk_reg_map *c = &dsc->hw;
@@ -150,6 +152,29 @@ static void dpu_hw_dsc_config_thresh(struct dpu_hw_dsc *hw_dsc,
}
}
+static void dpu_hw_dsc_bind_pingpong_blk(
+ struct dpu_hw_dsc *hw_dsc,
+ bool enable,
+ const enum dpu_pingpong pp)
+{
+ struct dpu_hw_blk_reg_map *c = &hw_dsc->hw;
+ int mux_cfg = 0xF;
+ u32 dsc_ctl_offset;
+
+ dsc_ctl_offset = DSC_CTL(hw_dsc->idx);
+
+ if (enable)
+ mux_cfg = (pp - PINGPONG_0) & 0x7;
+
+ DRM_DEBUG_KMS("%s dsc:%d %s pp:%d\n",
+ enable ? "Binding" : "Unbinding",
+ hw_dsc->idx - DSC_0,
+ enable ? "to" : "from",
+ pp - PINGPONG_0);
+
+ DPU_REG_WRITE(c, dsc_ctl_offset, mux_cfg);
+}
+
static struct dpu_dsc_cfg *_dsc_offset(enum dpu_dsc dsc,
const struct dpu_mdss_cfg *m,
void __iomem *addr,
@@ -174,6 +199,8 @@ static void _setup_dsc_ops(struct dpu_hw_dsc_ops *ops,
ops->dsc_disable = dpu_hw_dsc_disable;
ops->dsc_config = dpu_hw_dsc_config;
ops->dsc_config_thresh = dpu_hw_dsc_config_thresh;
+ if (cap & BIT(DPU_DSC_OUTPUT_CTRL))
+ ops->dsc_bind_pingpong_blk = dpu_hw_dsc_bind_pingpong_blk;
};
struct dpu_hw_dsc *dpu_hw_dsc_init(enum dpu_dsc idx, void __iomem *addr,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h
index c0b77fe1a696..ae9b5db53d7f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h
@@ -42,6 +42,10 @@ struct dpu_hw_dsc_ops {
*/
void (*dsc_config_thresh)(struct dpu_hw_dsc *hw_dsc,
struct drm_dsc_config *dsc);
+
+ void (*dsc_bind_pingpong_blk)(struct dpu_hw_dsc *hw_dsc,
+ bool enable,
+ enum dpu_pingpong pp);
};
struct dpu_hw_dsc {
--
2.38.1
In the event that the topology requests resources that have not been
created by the system (because they are typically not represented in
dpu_mdss_cfg ^1), the resource(s) in global_state (in this case DSC
blocks) remain NULL but will still be returned out of
dpu_rm_get_assigned_resources, where the caller expects to get an array
containing num_blks valid pointers (but instead gets these NULLs).
To prevent this from happening, where null-pointer dereferences
typically result in a hard-to-debug platform lockup, num_blks shouldn't
increase past NULL blocks and will print an error and break instead.
After all, max_blks represents the static size of the maximum number of
blocks whereas the actual amount varies per platform.
In the specific case of DSC initial resource allocation should behave
more like LMs and CTLs where NULL resources are skipped. The current
hardcoded mapping of DSC blocks should be loosened separately as DPU
5.0.0 introduced a crossbar where DSC blocks can be "somewhat" freely
bound to any PP and CTL, but that hardcoding currently means that we
will return an error when the topology reserves a DSC that isn't
available, instead of looking for the next free one.
^1: which can happen after a git rebase ended up moving additions to
_dpu_cfg to a different struct which has the same patch context.
Signed-off-by: Marijn Suijten <[email protected]>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
index 73b3442e7467..dcbf03d2940a 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
@@ -496,6 +496,11 @@ static int _dpu_rm_reserve_dsc(struct dpu_rm *rm,
/* check if DSC required are allocated or not */
for (i = 0; i < num_dsc; i++) {
+ if (!rm->dsc_blks[i]) {
+ DPU_ERROR("DSC %d does not exist\n", i);
+ return -EIO;
+ }
+
if (global_state->dsc_to_enc_id[i]) {
DPU_ERROR("DSC %d is already allocated\n", i);
return -EIO;
@@ -660,6 +665,11 @@ int dpu_rm_get_assigned_resources(struct dpu_rm *rm,
blks_size, enc_id);
break;
}
+ if (!hw_blks[i]) {
+ DPU_ERROR("No more resource %d available to assign to enc %d\n",
+ type, enc_id);
+ break;
+ }
blks[num_blks++] = hw_blks[i];
}
--
2.38.1
On 14.12.2022 00:22, Marijn Suijten wrote:
> According to downstream /and the comment copied from it/ this comparison
> should be the other way around. In other words, when the panel driver
> requests to use more slices per packet than what could be sent over this
> interface, it is bumped down to only use a single slice per packet (and
> strangely not the number of slices that could fit on the interface).
>
> Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
> ---
Missing s-o-b
> 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 0686c35a6fd4..9bdfa0864cdf 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -855,11 +855,11 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
> */
> slice_per_intf = DIV_ROUND_UP(hdisplay, dsc->slice_width);
>
> - /* If slice_per_pkt is greater than slice_per_intf
> + /* If slice_count is greater than slice_per_intf
> * then default to 1. This can happen during partial
> * update.
> */
> - if (slice_per_intf > dsc->slice_count)
> + if (dsc->slice_count > slice_per_intf)
> dsc->slice_count = 1;
>
> total_bytes_per_intf = dsc->slice_chunk_size * slice_per_intf;
On 2022-12-14 01:02:14, Konrad Dybcio wrote:
>
>
> On 14.12.2022 00:22, Marijn Suijten wrote:
> > According to downstream /and the comment copied from it/ this comparison
> > should be the other way around. In other words, when the panel driver
> > requests to use more slices per packet than what could be sent over this
> > interface, it is bumped down to only use a single slice per packet (and
> > strangely not the number of slices that could fit on the interface).
> >
> > Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
Signed-off-by: Marijn Suijten <[email protected]>
> > ---
> Missing s-o-b
Thanks for catching, checkpatch would've pointed this out (in addition
to a typo in the cover letter) if I had ran it.
> > 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 0686c35a6fd4..9bdfa0864cdf 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > @@ -855,11 +855,11 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
> > */
> > slice_per_intf = DIV_ROUND_UP(hdisplay, dsc->slice_width);
> >
> > - /* If slice_per_pkt is greater than slice_per_intf
> > + /* If slice_count is greater than slice_per_intf
> > * then default to 1. This can happen during partial
> > * update.
> > */
> > - if (slice_per_intf > dsc->slice_count)
> > + if (dsc->slice_count > slice_per_intf)
> > dsc->slice_count = 1;
> >
> > total_bytes_per_intf = dsc->slice_chunk_size * slice_per_intf;
On 14/12/2022 01:22, Marijn Suijten wrote:
> All V1 CTL blocks (active CTLs) explicitly bind the pixel output from a
> DSC block to a PINGPONG block by setting the PINGPONG idx in a DSC
> hardware register.
>
> Signed-off-by: Marijn Suijten <[email protected]>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 3 +++
> .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 9 +++++++
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c | 27 +++++++++++++++++++
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h | 4 +++
> 4 files changed, 43 insertions(+)
Reviewed-by: Dmitry Baryshkov <[email protected]>
--
With best wishes
Dmitry
On 14/12/2022 01:22, Marijn Suijten wrote:
> Active CTLs have to configure what DSC block(s) have to be enabled, and
> what DSC block(s) have to be flushed; this value was initialized to zero
> resulting in the necessary register writes to never happen (or would
> write zero otherwise). This seems to have gotten lost in the DSC v4->v5
> series while refactoring how the combination with merge_3d was handled.
>
> Fixes: 58dca9810749 ("drm/msm/disp/dpu1: Add support for DSC in encoder")
> Signed-off-by: Marijn Suijten <[email protected]>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 1 +
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 1 +
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 2 ++
> 3 files changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> index ae28b2b93e69..35791f93c33d 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> @@ -61,6 +61,7 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
> intf_cfg.intf_mode_sel = DPU_CTL_MODE_SEL_CMD;
> intf_cfg.stream_sel = cmd_enc->stream_sel;
> intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc);
> + intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
> ctl->ops.setup_intf_cfg(ctl, &intf_cfg);
>
> /* setup which pp blk will connect to this intf */
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> index 0f71e8fe7be7..9ee3a7306a5f 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> @@ -274,6 +274,7 @@ static void dpu_encoder_phys_vid_setup_timing_engine(
> intf_cfg.intf_mode_sel = DPU_CTL_MODE_SEL_VID;
> intf_cfg.stream_sel = 0; /* Don't care value for video mode */
> intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc);
> + intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
> if (phys_enc->hw_pp->merge_3d)
> intf_cfg.merge_3d = phys_enc->hw_pp->merge_3d->idx;
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> index 7cbcef6efe17..92ddf9995b37 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> @@ -209,6 +209,7 @@ static void dpu_encoder_phys_wb_setup_cdp(struct dpu_encoder_phys *phys_enc)
>
> intf_cfg.intf = DPU_NONE;
> intf_cfg.wb = hw_wb->idx;
> + intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
We usually don't have DSC with the writeback, don't we?
> if (mode_3d && hw_pp && hw_pp->merge_3d)
> intf_cfg.merge_3d = hw_pp->merge_3d->idx;
> @@ -230,6 +231,7 @@ static void dpu_encoder_phys_wb_setup_cdp(struct dpu_encoder_phys *phys_enc)
> intf_cfg.wb = hw_wb->idx;
> intf_cfg.mode_3d =
> dpu_encoder_helper_get_3d_blend_mode(phys_enc);
> + intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
> phys_enc->hw_ctl->ops.setup_intf_cfg(phys_enc->hw_ctl, &intf_cfg);
> }
> }
--
With best wishes
Dmitry
On 14/12/2022 01:22, Marijn Suijten wrote:
> According to downstream the value to use for WORD_COUNT is
> bytes_per_pkt, which denotes the number of bytes in a packet based on
> how many slices have been configured by the panel driver times the
> width of a slice times the number of bytes per pixel.
>
> The DSC panels seen thus far use one byte per pixel, only one slice
> per packet, and a slice width of half the panel width leading to the
> desired bytes_per_pkt+1 value to be equal to hdisplay/2+1. This however
> isn't the case anymore for panels that configure two slices per packet,
> where the value should now be hdisplay+1.
>
> Note that the aforementioned panel (on a Sony Xperia XZ3, sdm845) with
> slice_count=1 has also been tested to successfully accept slice_count=2,
> which would have shown corrupted output previously.
>
> 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 | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Dmitry Baryshkov <[email protected]>
--
With best wishes
Dmitry
On 14/12/2022 01:22, Marijn Suijten wrote:
> This preliminary Display Stream Compression support package for
> (initially tested on) sm8[12]50 is based on comparing DSC behaviour
> between downstream and mainline. Some new callbacks are added (for
> binding blocks on active CTLs), logic bugs are corrected, zeroed struct
> members are now assigned proper values, and RM allocation and hw block
> retrieval now hand out (or not) DSC blocks without causing null-pointer
> dereferences.
>
> Unfortunately it is not yet enough to get rid of completely corrupted
> display output on the boards I tested here:
> - Sony Xperia 1 (sm8150), 1644x3840 or 1096x2560 pixels;
> - Sony Xperia 5II (sm8250), 1080x2520, at 60 or 120Hz;
> - (can include more Xperia boards if desired)
>
> Both devices use the DUALPIPE_DSCMERGE topology downstream: dual LM, PP
> and DSC, but only a single INTF/encoder/DSI-link.
>
> Hopefully this spawns some community/upstream interest to help rootcause
> our corruption issues (after we open a drm/msm report on GitLab for more
> appropriate tracking).
>
> The Sony Xperia XZ3 (sdm845) was fully tested and validated with this
> series to not cause any regressions (an one of the math fixes now allows
> us to change slice_count in the panel driver, which would corrupt
> previously).
>
> Marijn Suijten (6):
> drm/msm/dpu1: Implement DSC binding to PP block for CTL V1
> drm/msm/dpu1: Add DSC config for sm8150 and sm8250
> drm/msm/dpu1: Wire up DSC mask for active CTL configuration
> drm/msm/dsi: Use DSC slice(s) packet size to compute word count
> drm/msm/dsi: Flip greater-than check for slice_count and
> slice_per_intf
> drm/msm/dpu: Disallow unallocated (DSC) resources to be returned
General comment: patches with Fixes ideally should come first. Usually
they are picked into -fixes and/or stable kernels. If the Fixes patches
are in the middle of the series, one can not be sure that they do not
have dependencies on previous patches. If there is one, it should
probably be stated clearly to ease work on backporting them.
>
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 3 +++
> .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 1 +
> .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 1 +
> .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 2 ++
> .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 23 +++++++++++-----
> .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 9 +++++++
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c | 27 +++++++++++++++++++
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h | 4 +++
> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 10 +++++++
> drivers/gpu/drm/msm/dsi/dsi_host.c | 6 ++---
> 10 files changed, 77 insertions(+), 9 deletions(-)
>
> --
> 2.38.1
>
--
With best wishes
Dmitry
On 14/12/2022 01:22, Marijn Suijten wrote:
> According to downstream /and the comment copied from it/ this comparison
> should be the other way around. In other words, when the panel driver
> requests to use more slices per packet than what could be sent over this
> interface, it is bumped down to only use a single slice per packet (and
> strangely not the number of slices that could fit on the interface).
>
> Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
> ---
> drivers/gpu/drm/msm/dsi/dsi_host.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
With SoB in place:
Reviewed-by: Dmitry Baryshkov <[email protected]>
--
With best wishes
Dmitry
On 14/12/2022 01:22, Marijn Suijten wrote:
> In the event that the topology requests resources that have not been
> created by the system (because they are typically not represented in
> dpu_mdss_cfg ^1), the resource(s) in global_state (in this case DSC
> blocks) remain NULL but will still be returned out of
> dpu_rm_get_assigned_resources, where the caller expects to get an array
> containing num_blks valid pointers (but instead gets these NULLs).
>
> To prevent this from happening, where null-pointer dereferences
> typically result in a hard-to-debug platform lockup, num_blks shouldn't
> increase past NULL blocks and will print an error and break instead.
> After all, max_blks represents the static size of the maximum number of
> blocks whereas the actual amount varies per platform.
>
> In the specific case of DSC initial resource allocation should behave
> more like LMs and CTLs where NULL resources are skipped. The current
> hardcoded mapping of DSC blocks should be loosened separately as DPU
> 5.0.0 introduced a crossbar where DSC blocks can be "somewhat" freely
> bound to any PP and CTL, but that hardcoding currently means that we
> will return an error when the topology reserves a DSC that isn't
> available, instead of looking for the next free one.
>
> ^1: which can happen after a git rebase ended up moving additions to
> _dpu_cfg to a different struct which has the same patch context.
>
> Signed-off-by: Marijn Suijten <[email protected]>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> index 73b3442e7467..dcbf03d2940a 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> @@ -496,6 +496,11 @@ static int _dpu_rm_reserve_dsc(struct dpu_rm *rm,
>
> /* check if DSC required are allocated or not */
> for (i = 0; i < num_dsc; i++) {
> + if (!rm->dsc_blks[i]) {
> + DPU_ERROR("DSC %d does not exist\n", i);
> + return -EIO;
> + }
> +
> if (global_state->dsc_to_enc_id[i]) {
> DPU_ERROR("DSC %d is already allocated\n", i);
> return -EIO;
> @@ -660,6 +665,11 @@ int dpu_rm_get_assigned_resources(struct dpu_rm *rm,
> blks_size, enc_id);
> break;
> }
> + if (!hw_blks[i]) {
> + DPU_ERROR("No more resource %d available to assign to enc %d\n",
> + type, enc_id);
> + break;
> + }
> blks[num_blks++] = hw_blks[i];
> }
>
These two chunks should come as two separate patches, each having it's
own Fixes tag.
--
With best wishes
Dmitry
On 14/12/2022 01:22, Marijn Suijten wrote:
> These blocks on CTL V1 support setting a PINGPONG idx to send pixel
> output to.
>
> Signed-off-by: Marijn Suijten <[email protected]>
> ---
> .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 23 ++++++++++++++-----
> 1 file changed, 17 insertions(+), 6 deletions(-)
Reviewed-by: Dmitry Baryshkov <[email protected]>
--
With best wishes
Dmitry
On 2022-12-14 20:43:29, Dmitry Baryshkov wrote:
> On 14/12/2022 01:22, Marijn Suijten wrote:
> > Active CTLs have to configure what DSC block(s) have to be enabled, and
> > what DSC block(s) have to be flushed; this value was initialized to zero
> > resulting in the necessary register writes to never happen (or would
> > write zero otherwise). This seems to have gotten lost in the DSC v4->v5
> > series while refactoring how the combination with merge_3d was handled.
> >
> > Fixes: 58dca9810749 ("drm/msm/disp/dpu1: Add support for DSC in encoder")
> > Signed-off-by: Marijn Suijten <[email protected]>
> > ---
> > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 1 +
> > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 1 +
> > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 2 ++
> > 3 files changed, 4 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> > index ae28b2b93e69..35791f93c33d 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> > @@ -61,6 +61,7 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
> > intf_cfg.intf_mode_sel = DPU_CTL_MODE_SEL_CMD;
> > intf_cfg.stream_sel = cmd_enc->stream_sel;
> > intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc);
> > + intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
> > ctl->ops.setup_intf_cfg(ctl, &intf_cfg);
> >
> > /* setup which pp blk will connect to this intf */
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> > index 0f71e8fe7be7..9ee3a7306a5f 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> > @@ -274,6 +274,7 @@ static void dpu_encoder_phys_vid_setup_timing_engine(
> > intf_cfg.intf_mode_sel = DPU_CTL_MODE_SEL_VID;
> > intf_cfg.stream_sel = 0; /* Don't care value for video mode */
> > intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc);
> > + intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
> > if (phys_enc->hw_pp->merge_3d)
> > intf_cfg.merge_3d = phys_enc->hw_pp->merge_3d->idx;
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> > index 7cbcef6efe17..92ddf9995b37 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> > @@ -209,6 +209,7 @@ static void dpu_encoder_phys_wb_setup_cdp(struct dpu_encoder_phys *phys_enc)
> >
> > intf_cfg.intf = DPU_NONE;
> > intf_cfg.wb = hw_wb->idx;
> > + intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
>
> We usually don't have DSC with the writeback, don't we?
I am unsure so ended up adding them in writeback regardless. Downstream
uses a separate callback to process intf_cfg.dsc instead of going
through setup_intf_cfg().
To prevent these from being missed again (in the case of copy&paste),
how about instead having some function that sets up intf_cfg with these
default values from a phys_enc? That way most of this remains oblivious
to the caller.
On the same note, that callback on non-DPU_CTL_ACTIVE_CFG hardware
doesn't use the intf_cfg.dsc member anyway, but it was again added to
keep the blocks somewhat consistent (in case it ever becomes used?).
> > if (mode_3d && hw_pp && hw_pp->merge_3d)
> > intf_cfg.merge_3d = hw_pp->merge_3d->idx;
> > @@ -230,6 +231,7 @@ static void dpu_encoder_phys_wb_setup_cdp(struct dpu_encoder_phys *phys_enc)
> > intf_cfg.wb = hw_wb->idx;
> > intf_cfg.mode_3d =
> > dpu_encoder_helper_get_3d_blend_mode(phys_enc);
> > + intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
> > phys_enc->hw_ctl->ops.setup_intf_cfg(phys_enc->hw_ctl, &intf_cfg);
> > }
> > }
- Marijn
On 2022-12-14 20:40:06, Dmitry Baryshkov wrote:
> On 14/12/2022 01:22, Marijn Suijten wrote:
> > This preliminary Display Stream Compression support package for
> > (initially tested on) sm8[12]50 is based on comparing DSC behaviour
> > between downstream and mainline. Some new callbacks are added (for
> > binding blocks on active CTLs), logic bugs are corrected, zeroed struct
> > members are now assigned proper values, and RM allocation and hw block
> > retrieval now hand out (or not) DSC blocks without causing null-pointer
> > dereferences.
> >
> > Unfortunately it is not yet enough to get rid of completely corrupted
> > display output on the boards I tested here:
> > - Sony Xperia 1 (sm8150), 1644x3840 or 1096x2560 pixels;
> > - Sony Xperia 5II (sm8250), 1080x2520, at 60 or 120Hz;
> > - (can include more Xperia boards if desired)
> >
> > Both devices use the DUALPIPE_DSCMERGE topology downstream: dual LM, PP
> > and DSC, but only a single INTF/encoder/DSI-link.
> >
> > Hopefully this spawns some community/upstream interest to help rootcause
> > our corruption issues (after we open a drm/msm report on GitLab for more
> > appropriate tracking).
> >
> > The Sony Xperia XZ3 (sdm845) was fully tested and validated with this
> > series to not cause any regressions (an one of the math fixes now allows
> > us to change slice_count in the panel driver, which would corrupt
> > previously).
> >
> > Marijn Suijten (6):
> > drm/msm/dpu1: Implement DSC binding to PP block for CTL V1
> > drm/msm/dpu1: Add DSC config for sm8150 and sm8250
> > drm/msm/dpu1: Wire up DSC mask for active CTL configuration
> > drm/msm/dsi: Use DSC slice(s) packet size to compute word count
> > drm/msm/dsi: Flip greater-than check for slice_count and
> > slice_per_intf
> > drm/msm/dpu: Disallow unallocated (DSC) resources to be returned
>
> General comment: patches with Fixes ideally should come first. Usually
> they are picked into -fixes and/or stable kernels. If the Fixes patches
> are in the middle of the series, one can not be sure that they do not
> have dependencies on previous patches. If there is one, it should
> probably be stated clearly to ease work on backporting them.
Ack, I may have rushed these RFC patches straight off my branches onto
the lists in hopes of sparking some suggestions on what may still be
broken or missing to get DSC working on sm[12]50, but will keep this in
mind for v2 after receiving some more review.
That said, any suggestions?
- Marijn
On 2022-12-14 20:56:30, Dmitry Baryshkov wrote:
> On 14/12/2022 01:22, Marijn Suijten wrote:
> > In the event that the topology requests resources that have not been
> > created by the system (because they are typically not represented in
> > dpu_mdss_cfg ^1), the resource(s) in global_state (in this case DSC
> > blocks) remain NULL but will still be returned out of
> > dpu_rm_get_assigned_resources, where the caller expects to get an array
> > containing num_blks valid pointers (but instead gets these NULLs).
> >
> > To prevent this from happening, where null-pointer dereferences
> > typically result in a hard-to-debug platform lockup, num_blks shouldn't
> > increase past NULL blocks and will print an error and break instead.
> > After all, max_blks represents the static size of the maximum number of
> > blocks whereas the actual amount varies per platform.
> >
> > In the specific case of DSC initial resource allocation should behave
> > more like LMs and CTLs where NULL resources are skipped. The current
> > hardcoded mapping of DSC blocks should be loosened separately as DPU
> > 5.0.0 introduced a crossbar where DSC blocks can be "somewhat" freely
> > bound to any PP and CTL, but that hardcoding currently means that we
> > will return an error when the topology reserves a DSC that isn't
> > available, instead of looking for the next free one.
> >
> > ^1: which can happen after a git rebase ended up moving additions to
> > _dpu_cfg to a different struct which has the same patch context.
> >
> > Signed-off-by: Marijn Suijten <[email protected]>
> > ---
> > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 10 ++++++++++
> > 1 file changed, 10 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> > index 73b3442e7467..dcbf03d2940a 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> > @@ -496,6 +496,11 @@ static int _dpu_rm_reserve_dsc(struct dpu_rm *rm,
> >
> > /* check if DSC required are allocated or not */
> > for (i = 0; i < num_dsc; i++) {
> > + if (!rm->dsc_blks[i]) {
> > + DPU_ERROR("DSC %d does not exist\n", i);
> > + return -EIO;
> > + }
> > +
> > if (global_state->dsc_to_enc_id[i]) {
> > DPU_ERROR("DSC %d is already allocated\n", i);
> > return -EIO;
> > @@ -660,6 +665,11 @@ int dpu_rm_get_assigned_resources(struct dpu_rm *rm,
> > blks_size, enc_id);
> > break;
> > }
> > + if (!hw_blks[i]) {
> > + DPU_ERROR("No more resource %d available to assign to enc %d\n",
> > + type, enc_id);
> > + break;
> > + }
> > blks[num_blks++] = hw_blks[i];
> > }
> >
>
> These two chunks should come as two separate patches, each having it's
> own Fixes tag.
Ack. They are indeed addressing different issues (with the same
outcome) with differing "backportability". Will address in v2, thanks
for pointing it out (and missing a Fixes: in the first place, of which
we already have so many...).
- Marijn
On 14/12/2022 21:23, Marijn Suijten wrote:
> On 2022-12-14 20:40:06, Dmitry Baryshkov wrote:
>> On 14/12/2022 01:22, Marijn Suijten wrote:
>>> This preliminary Display Stream Compression support package for
>>> (initially tested on) sm8[12]50 is based on comparing DSC behaviour
>>> between downstream and mainline. Some new callbacks are added (for
>>> binding blocks on active CTLs), logic bugs are corrected, zeroed struct
>>> members are now assigned proper values, and RM allocation and hw block
>>> retrieval now hand out (or not) DSC blocks without causing null-pointer
>>> dereferences.
>>>
>>> Unfortunately it is not yet enough to get rid of completely corrupted
>>> display output on the boards I tested here:
>>> - Sony Xperia 1 (sm8150), 1644x3840 or 1096x2560 pixels;
>>> - Sony Xperia 5II (sm8250), 1080x2520, at 60 or 120Hz;
>>> - (can include more Xperia boards if desired)
>>>
>>> Both devices use the DUALPIPE_DSCMERGE topology downstream: dual LM, PP
>>> and DSC, but only a single INTF/encoder/DSI-link.
>>>
>>> Hopefully this spawns some community/upstream interest to help rootcause
>>> our corruption issues (after we open a drm/msm report on GitLab for more
>>> appropriate tracking).
>>>
>>> The Sony Xperia XZ3 (sdm845) was fully tested and validated with this
>>> series to not cause any regressions (an one of the math fixes now allows
>>> us to change slice_count in the panel driver, which would corrupt
>>> previously).
>>>
>>> Marijn Suijten (6):
>>> drm/msm/dpu1: Implement DSC binding to PP block for CTL V1
>>> drm/msm/dpu1: Add DSC config for sm8150 and sm8250
>>> drm/msm/dpu1: Wire up DSC mask for active CTL configuration
>>> drm/msm/dsi: Use DSC slice(s) packet size to compute word count
>>> drm/msm/dsi: Flip greater-than check for slice_count and
>>> slice_per_intf
>>> drm/msm/dpu: Disallow unallocated (DSC) resources to be returned
>>
>> General comment: patches with Fixes ideally should come first. Usually
>> they are picked into -fixes and/or stable kernels. If the Fixes patches
>> are in the middle of the series, one can not be sure that they do not
>> have dependencies on previous patches. If there is one, it should
>> probably be stated clearly to ease work on backporting them.
>
> Ack, I may have rushed these RFC patches straight off my branches onto
> the lists in hopes of sparking some suggestions on what may still be
> broken or missing to get DSC working on sm[12]50, but will keep this in
> mind for v2 after receiving some more review.
>
> That said, any suggestions?
From what I've noticed lately:
- set dsc_version_major/dsc_version_minor
- try using dsc params from 1.2 rater than 1.1 version spec (there is
small difference there)
--
With best wishes
Dmitry
On 14/12/2022 21:30, Marijn Suijten wrote:
> On 2022-12-14 20:43:29, Dmitry Baryshkov wrote:
>> On 14/12/2022 01:22, Marijn Suijten wrote:
>>> Active CTLs have to configure what DSC block(s) have to be enabled, and
>>> what DSC block(s) have to be flushed; this value was initialized to zero
>>> resulting in the necessary register writes to never happen (or would
>>> write zero otherwise). This seems to have gotten lost in the DSC v4->v5
>>> series while refactoring how the combination with merge_3d was handled.
>>>
>>> Fixes: 58dca9810749 ("drm/msm/disp/dpu1: Add support for DSC in encoder")
>>> Signed-off-by: Marijn Suijten <[email protected]>
>>> ---
>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 1 +
>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 1 +
>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 2 ++
>>> 3 files changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>> index ae28b2b93e69..35791f93c33d 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>> @@ -61,6 +61,7 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
>>> intf_cfg.intf_mode_sel = DPU_CTL_MODE_SEL_CMD;
>>> intf_cfg.stream_sel = cmd_enc->stream_sel;
>>> intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc);
>>> + intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
>>> ctl->ops.setup_intf_cfg(ctl, &intf_cfg);
>>>
>>> /* setup which pp blk will connect to this intf */
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>>> index 0f71e8fe7be7..9ee3a7306a5f 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>>> @@ -274,6 +274,7 @@ static void dpu_encoder_phys_vid_setup_timing_engine(
>>> intf_cfg.intf_mode_sel = DPU_CTL_MODE_SEL_VID;
>>> intf_cfg.stream_sel = 0; /* Don't care value for video mode */
>>> intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc);
>>> + intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
>>> if (phys_enc->hw_pp->merge_3d)
>>> intf_cfg.merge_3d = phys_enc->hw_pp->merge_3d->idx;
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
>>> index 7cbcef6efe17..92ddf9995b37 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
>>> @@ -209,6 +209,7 @@ static void dpu_encoder_phys_wb_setup_cdp(struct dpu_encoder_phys *phys_enc)
>>>
>>> intf_cfg.intf = DPU_NONE;
>>> intf_cfg.wb = hw_wb->idx;
>>> + intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
>>
>> We usually don't have DSC with the writeback, don't we?
>
> I am unsure so ended up adding them in writeback regardless. Downstream
> uses a separate callback to process intf_cfg.dsc instead of going
> through setup_intf_cfg().
>
> To prevent these from being missed again (in the case of copy&paste),
> how about instead having some function that sets up intf_cfg with these
> default values from a phys_enc? That way most of this remains oblivious
> to the caller.
I'm not sure this is possible. E.g. intf_cfg.dsc should not be set for
the WB.
>
> On the same note, that callback on non-DPU_CTL_ACTIVE_CFG hardware
> doesn't use the intf_cfg.dsc member anyway, but it was again added to
> keep the blocks somewhat consistent (in case it ever becomes used?).
>
>>> if (mode_3d && hw_pp && hw_pp->merge_3d)
>>> intf_cfg.merge_3d = hw_pp->merge_3d->idx;
>>> @@ -230,6 +231,7 @@ static void dpu_encoder_phys_wb_setup_cdp(struct dpu_encoder_phys *phys_enc)
>>> intf_cfg.wb = hw_wb->idx;
>>> intf_cfg.mode_3d =
>>> dpu_encoder_helper_get_3d_blend_mode(phys_enc);
>>> + intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
>>> phys_enc->hw_ctl->ops.setup_intf_cfg(phys_enc->hw_ctl, &intf_cfg);
>>> }
>>> }
>
> - Marijn
--
With best wishes
Dmitry
On 12/13/2022 3:22 PM, Marijn Suijten wrote:
> All V1 CTL blocks (active CTLs) explicitly bind the pixel output from a
> DSC block to a PINGPONG block by setting the PINGPONG idx in a DSC
> hardware register.
>
> Signed-off-by: Marijn Suijten <[email protected]>
Reviewed-by: Abhinav Kumar <[email protected]>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 3 +++
> .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 9 +++++++
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c | 27 +++++++++++++++++++
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h | 4 +++
> 4 files changed, 43 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 9c6817b5a194..c17ac85eb447 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -1830,6 +1830,9 @@ static void dpu_encoder_dsc_pipe_cfg(struct dpu_hw_dsc *hw_dsc,
> if (hw_pp->ops.setup_dsc)
> hw_pp->ops.setup_dsc(hw_pp);
>
> + if (hw_dsc->ops.dsc_bind_pingpong_blk)
> + hw_dsc->ops.dsc_bind_pingpong_blk(hw_dsc, true, hw_pp->idx);
> +
> if (hw_pp->ops.enable_dsc)
> hw_pp->ops.enable_dsc(hw_pp);
> }
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> index c160dae95a69..96f849907aa2 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> @@ -268,6 +268,15 @@ enum {
> DPU_VBIF_MAX
> };
>
> +/**
> + * DSC features
> + * @DPU_DSC_OUTPUT_CTRL Configure which PINGPONG block gets
> + * the pixel output from this DSC.
> + */
> +enum {
> + DPU_DSC_OUTPUT_CTRL = 0x1,
> +};
> +
> /**
> * MACRO DPU_HW_BLK_INFO - information of HW blocks inside DPU
> * @name: string name for debug purposes
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
> index 3662df698dae..619926da1441 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
> @@ -29,6 +29,8 @@
> #define DSC_RANGE_MAX_QP 0x0B0
> #define DSC_RANGE_BPG_OFFSET 0x0EC
>
> +#define DSC_CTL(m) (0x1800 - 0x3FC * (m - DSC_0))
> +
> static void dpu_hw_dsc_disable(struct dpu_hw_dsc *dsc)
> {
> struct dpu_hw_blk_reg_map *c = &dsc->hw;
> @@ -150,6 +152,29 @@ static void dpu_hw_dsc_config_thresh(struct dpu_hw_dsc *hw_dsc,
> }
> }
>
> +static void dpu_hw_dsc_bind_pingpong_blk(
> + struct dpu_hw_dsc *hw_dsc,
> + bool enable,
> + const enum dpu_pingpong pp)
> +{
> + struct dpu_hw_blk_reg_map *c = &hw_dsc->hw;
> + int mux_cfg = 0xF;
> + u32 dsc_ctl_offset;
> +
> + dsc_ctl_offset = DSC_CTL(hw_dsc->idx);
> +
> + if (enable)
> + mux_cfg = (pp - PINGPONG_0) & 0x7;
> +
> + DRM_DEBUG_KMS("%s dsc:%d %s pp:%d\n",
> + enable ? "Binding" : "Unbinding",
> + hw_dsc->idx - DSC_0,
> + enable ? "to" : "from",
> + pp - PINGPONG_0);
> +
> + DPU_REG_WRITE(c, dsc_ctl_offset, mux_cfg);
> +}
> +
> static struct dpu_dsc_cfg *_dsc_offset(enum dpu_dsc dsc,
> const struct dpu_mdss_cfg *m,
> void __iomem *addr,
> @@ -174,6 +199,8 @@ static void _setup_dsc_ops(struct dpu_hw_dsc_ops *ops,
> ops->dsc_disable = dpu_hw_dsc_disable;
> ops->dsc_config = dpu_hw_dsc_config;
> ops->dsc_config_thresh = dpu_hw_dsc_config_thresh;
> + if (cap & BIT(DPU_DSC_OUTPUT_CTRL))
> + ops->dsc_bind_pingpong_blk = dpu_hw_dsc_bind_pingpong_blk;
> };
>
> struct dpu_hw_dsc *dpu_hw_dsc_init(enum dpu_dsc idx, void __iomem *addr,
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h
> index c0b77fe1a696..ae9b5db53d7f 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h
> @@ -42,6 +42,10 @@ struct dpu_hw_dsc_ops {
> */
> void (*dsc_config_thresh)(struct dpu_hw_dsc *hw_dsc,
> struct drm_dsc_config *dsc);
> +
> + void (*dsc_bind_pingpong_blk)(struct dpu_hw_dsc *hw_dsc,
> + bool enable,
> + enum dpu_pingpong pp);
> };
>
> struct dpu_hw_dsc {
On 12/13/2022 3:22 PM, Marijn Suijten wrote:
> These blocks on CTL V1 support setting a PINGPONG idx to send pixel
> output to.
>
> Signed-off-by: Marijn Suijten <[email protected]>
Reviewed-by: Abhinav Kumar <[email protected]>
> ---
> .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 23 ++++++++++++++-----
> 1 file changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> index 318f0b4dbf6e..114ad8ca4554 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> @@ -1566,18 +1566,25 @@ static const struct dpu_merge_3d_cfg sm8150_merge_3d[] = {
> /*************************************************************
> * DSC sub blocks config
> *************************************************************/
> -#define DSC_BLK(_name, _id, _base) \
> +#define DSC_BLK(_name, _id, _base, _features) \
> {\
> .name = _name, .id = _id, \
> .base = _base, .len = 0x140, \
> - .features = 0, \
> + .features = _features, \
> }
>
> static struct dpu_dsc_cfg sdm845_dsc[] = {
> - DSC_BLK("dsc_0", DSC_0, 0x80000),
> - DSC_BLK("dsc_1", DSC_1, 0x80400),
> - DSC_BLK("dsc_2", DSC_2, 0x80800),
> - DSC_BLK("dsc_3", DSC_3, 0x80c00),
> + DSC_BLK("dsc_0", DSC_0, 0x80000, 0),
> + DSC_BLK("dsc_1", DSC_1, 0x80400, 0),
> + DSC_BLK("dsc_2", DSC_2, 0x80800, 0),
> + DSC_BLK("dsc_3", DSC_3, 0x80c00, 0),
> +};
> +
> +static struct dpu_dsc_cfg sm8150_dsc[] = {
> + DSC_BLK("dsc_0", DSC_0, 0x80000, BIT(DPU_DSC_OUTPUT_CTRL)),
> + DSC_BLK("dsc_1", DSC_1, 0x80400, BIT(DPU_DSC_OUTPUT_CTRL)),
> + DSC_BLK("dsc_2", DSC_2, 0x80800, BIT(DPU_DSC_OUTPUT_CTRL)),
> + DSC_BLK("dsc_3", DSC_3, 0x80c00, BIT(DPU_DSC_OUTPUT_CTRL)),
> };
>
> /*************************************************************
> @@ -2474,6 +2481,8 @@ static const struct dpu_mdss_cfg sm8150_dpu_cfg = {
> .mixer = sm8150_lm,
> .dspp_count = ARRAY_SIZE(sm8150_dspp),
> .dspp = sm8150_dspp,
> + .dsc_count = ARRAY_SIZE(sm8150_dsc),
> + .dsc = sm8150_dsc,
> .pingpong_count = ARRAY_SIZE(sm8150_pp),
> .pingpong = sm8150_pp,
> .merge_3d_count = ARRAY_SIZE(sm8150_merge_3d),
> @@ -2524,6 +2533,8 @@ static const struct dpu_mdss_cfg sm8250_dpu_cfg = {
> .mixer = sm8150_lm,
> .dspp_count = ARRAY_SIZE(sm8150_dspp),
> .dspp = sm8150_dspp,
> + .dsc_count = ARRAY_SIZE(sm8150_dsc),
> + .dsc = sm8150_dsc,
> .pingpong_count = ARRAY_SIZE(sm8150_pp),
> .pingpong = sm8150_pp,
> .merge_3d_count = ARRAY_SIZE(sm8150_merge_3d),
On 12/14/2022 5:08 PM, Dmitry Baryshkov wrote:
> On 14/12/2022 21:30, Marijn Suijten wrote:
>> On 2022-12-14 20:43:29, Dmitry Baryshkov wrote:
>>> On 14/12/2022 01:22, Marijn Suijten wrote:
>>>> Active CTLs have to configure what DSC block(s) have to be enabled, and
>>>> what DSC block(s) have to be flushed; this value was initialized to
>>>> zero
>>>> resulting in the necessary register writes to never happen (or would
>>>> write zero otherwise). This seems to have gotten lost in the DSC
>>>> v4->v5
>>>> series while refactoring how the combination with merge_3d was handled.
>>>>
>>>> Fixes: 58dca9810749 ("drm/msm/disp/dpu1: Add support for DSC in
>>>> encoder")
>>>> Signed-off-by: Marijn Suijten <[email protected]>
>>>> ---
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 1 +
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 1 +
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 2 ++
>>>> 3 files changed, 4 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>>> index ae28b2b93e69..35791f93c33d 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>>> @@ -61,6 +61,7 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
>>>> intf_cfg.intf_mode_sel = DPU_CTL_MODE_SEL_CMD;
>>>> intf_cfg.stream_sel = cmd_enc->stream_sel;
>>>> intf_cfg.mode_3d =
>>>> dpu_encoder_helper_get_3d_blend_mode(phys_enc);
>>>> + intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
>>>> ctl->ops.setup_intf_cfg(ctl, &intf_cfg);
>>>> /* setup which pp blk will connect to this intf */
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>>>> index 0f71e8fe7be7..9ee3a7306a5f 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>>>> @@ -274,6 +274,7 @@ static void
>>>> dpu_encoder_phys_vid_setup_timing_engine(
>>>> intf_cfg.intf_mode_sel = DPU_CTL_MODE_SEL_VID;
>>>> intf_cfg.stream_sel = 0; /* Don't care value for video mode */
>>>> intf_cfg.mode_3d =
>>>> dpu_encoder_helper_get_3d_blend_mode(phys_enc);
>>>> + intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
>>>> if (phys_enc->hw_pp->merge_3d)
>>>> intf_cfg.merge_3d = phys_enc->hw_pp->merge_3d->idx;
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
>>>> index 7cbcef6efe17..92ddf9995b37 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
>>>> @@ -209,6 +209,7 @@ static void dpu_encoder_phys_wb_setup_cdp(struct
>>>> dpu_encoder_phys *phys_enc)
>>>> intf_cfg.intf = DPU_NONE;
>>>> intf_cfg.wb = hw_wb->idx;
>>>> + intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
>>>
>>> We usually don't have DSC with the writeback, don't we?
>>
>> I am unsure so ended up adding them in writeback regardless. Downstream
>> uses a separate callback to process intf_cfg.dsc instead of going
>> through setup_intf_cfg().
>>
>> To prevent these from being missed again (in the case of copy&paste),
>> how about instead having some function that sets up intf_cfg with these
>> default values from a phys_enc? That way most of this remains oblivious
>> to the caller.
>
> I'm not sure this is possible. E.g. intf_cfg.dsc should not be set for
> the WB.
>
Although this change is harmless because
dpu_encoder_helper_get_dsc(phys_enc) will not return a valid DSC mask
for the WB encoder, hence the setup_intf_cfg will just skip the DSC
programming, I also agree that we can skip setting the intf_cfg.dsc for
the writeback encoder in this patch.
>>
>> On the same note, that callback on non-DPU_CTL_ACTIVE_CFG hardware
>> doesn't use the intf_cfg.dsc member anyway, but it was again added to
>> keep the blocks somewhat consistent (in case it ever becomes used?).
>>
>>>> if (mode_3d && hw_pp && hw_pp->merge_3d)
>>>> intf_cfg.merge_3d = hw_pp->merge_3d->idx;
>>>> @@ -230,6 +231,7 @@ static void dpu_encoder_phys_wb_setup_cdp(struct
>>>> dpu_encoder_phys *phys_enc)
>>>> intf_cfg.wb = hw_wb->idx;
>>>> intf_cfg.mode_3d =
>>>> dpu_encoder_helper_get_3d_blend_mode(phys_enc);
>>>> + intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
>>>> phys_enc->hw_ctl->ops.setup_intf_cfg(phys_enc->hw_ctl,
>>>> &intf_cfg);
>>>> }
>>>> }
>>
>> - Marijn
>
On 12/13/2022 3:22 PM, Marijn Suijten wrote:
> According to downstream the value to use for WORD_COUNT is
> bytes_per_pkt, which denotes the number of bytes in a packet based on
> how many slices have been configured by the panel driver times the
> width of a slice times the number of bytes per pixel.
>
> The DSC panels seen thus far use one byte per pixel, only one slice
> per packet, and a slice width of half the panel width leading to the
> desired bytes_per_pkt+1 value to be equal to hdisplay/2+1. This however
> isn't the case anymore for panels that configure two slices per packet,
> where the value should now be hdisplay+1.
>
> Note that the aforementioned panel (on a Sony Xperia XZ3, sdm845) with
> slice_count=1 has also been tested to successfully accept slice_count=2,
> which would have shown corrupted output previously.
>
> Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
> Signed-off-by: Marijn Suijten <[email protected]>
Reviewed-by: Abhinav Kumar <[email protected]>
> ---
> drivers/gpu/drm/msm/dsi/dsi_host.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index b83cf70b1adb..0686c35a6fd4 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -989,7 +989,7 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> if (!msm_host->dsc)
> wc = hdisplay * dsi_get_bpp(msm_host->format) / 8 + 1;
> else
> - wc = mode->hdisplay / 2 + 1;
> + wc = msm_host->dsc->slice_chunk_size * msm_host->dsc->slice_count + 1;
>
> dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_CTRL,
> DSI_CMD_MDP_STREAM0_CTRL_WORD_COUNT(wc) |
On 12/13/2022 3:22 PM, Marijn Suijten wrote:
> According to downstream /and the comment copied from it/ this comparison
> should be the other way around. In other words, when the panel driver
> requests to use more slices per packet than what could be sent over this
> interface, it is bumped down to only use a single slice per packet (and
> strangely not the number of slices that could fit on the interface).
>
> Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
Like others have said, with SOB,
Reviewed-by: Abhinav Kumar <[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 0686c35a6fd4..9bdfa0864cdf 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -855,11 +855,11 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
> */
> slice_per_intf = DIV_ROUND_UP(hdisplay, dsc->slice_width);
>
> - /* If slice_per_pkt is greater than slice_per_intf
> + /* If slice_count is greater than slice_per_intf
> * then default to 1. This can happen during partial
> * update.
> */
> - if (slice_per_intf > dsc->slice_count)
> + if (dsc->slice_count > slice_per_intf)
> dsc->slice_count = 1;
>
> total_bytes_per_intf = dsc->slice_chunk_size * slice_per_intf;
On 2022-12-15 02:52:01, Dmitry Baryshkov wrote:
> On 14/12/2022 21:23, Marijn Suijten wrote:
> > On 2022-12-14 20:40:06, Dmitry Baryshkov wrote:
> >> On 14/12/2022 01:22, Marijn Suijten wrote:
> >>> This preliminary Display Stream Compression support package for
> >>> (initially tested on) sm8[12]50 is based on comparing DSC behaviour
> >>> between downstream and mainline. Some new callbacks are added (for
> >>> binding blocks on active CTLs), logic bugs are corrected, zeroed struct
> >>> members are now assigned proper values, and RM allocation and hw block
> >>> retrieval now hand out (or not) DSC blocks without causing null-pointer
> >>> dereferences.
> >>>
> >>> Unfortunately it is not yet enough to get rid of completely corrupted
> >>> display output on the boards I tested here:
> >>> - Sony Xperia 1 (sm8150), 1644x3840 or 1096x2560 pixels;
> >>> - Sony Xperia 5II (sm8250), 1080x2520, at 60 or 120Hz;
> >>> - (can include more Xperia boards if desired)
> >>>
> >>> Both devices use the DUALPIPE_DSCMERGE topology downstream: dual LM, PP
> >>> and DSC, but only a single INTF/encoder/DSI-link.
> >>>
> >>> Hopefully this spawns some community/upstream interest to help rootcause
> >>> our corruption issues (after we open a drm/msm report on GitLab for more
> >>> appropriate tracking).
> >>>
> >>> The Sony Xperia XZ3 (sdm845) was fully tested and validated with this
> >>> series to not cause any regressions (an one of the math fixes now allows
> >>> us to change slice_count in the panel driver, which would corrupt
> >>> previously).
> >>>
> >>> Marijn Suijten (6):
> >>> drm/msm/dpu1: Implement DSC binding to PP block for CTL V1
> >>> drm/msm/dpu1: Add DSC config for sm8150 and sm8250
> >>> drm/msm/dpu1: Wire up DSC mask for active CTL configuration
> >>> drm/msm/dsi: Use DSC slice(s) packet size to compute word count
> >>> drm/msm/dsi: Flip greater-than check for slice_count and
> >>> slice_per_intf
> >>> drm/msm/dpu: Disallow unallocated (DSC) resources to be returned
> >>
> >> General comment: patches with Fixes ideally should come first. Usually
> >> they are picked into -fixes and/or stable kernels. If the Fixes patches
> >> are in the middle of the series, one can not be sure that they do not
> >> have dependencies on previous patches. If there is one, it should
> >> probably be stated clearly to ease work on backporting them.
> >
> > Ack, I may have rushed these RFC patches straight off my branches onto
> > the lists in hopes of sparking some suggestions on what may still be
> > broken or missing to get DSC working on sm[12]50, but will keep this in
> > mind for v2 after receiving some more review.
> >
> > That said, any suggestions?
>
>
> From what I've noticed lately:
Apologies for the late reply, I wanted to double-check this but now
ended up basing my
> - set dsc_version_major/dsc_version_minor
We always set these in our panel drivers (all the way from back when our
initial panel driver changes were based on what Vinod did for Pixel 3),
both to 1. As expected this results in 0x11 in the first byte of the
Pixel Parameter Set sent to the DrIC over DSI.
> - try using dsc params from 1.2 rater than 1.1 version spec (there is
> small difference there)
Didn't have any effect and this is not what downstream sets/sends
regardless, all our panels (on these sm8[12]50 SoCs) are hardcoded to
DSC 1.1 downstream.
Should I test this again, but also setting the version in the
compression_mode command?
- Marijn
On 2022-12-16 14:20:52, Abhinav Kumar wrote:
>
>
> On 12/14/2022 5:08 PM, Dmitry Baryshkov wrote:
> > On 14/12/2022 21:30, Marijn Suijten wrote:
> >> On 2022-12-14 20:43:29, Dmitry Baryshkov wrote:
> >>> On 14/12/2022 01:22, Marijn Suijten wrote:
> >>>> [..]
> >>> We usually don't have DSC with the writeback, don't we?
> >>
> >> I am unsure so ended up adding them in writeback regardless.? Downstream
> >> uses a separate callback to process intf_cfg.dsc instead of going
> >> through setup_intf_cfg().
> >>
> >> To prevent these from being missed again (in the case of copy&paste),
> >> how about instead having some function that sets up intf_cfg with these
> >> default values from a phys_enc?? That way most of this remains oblivious
> >> to the caller.
> >
> > I'm not sure this is possible. E.g. intf_cfg.dsc should not be set for
> > the WB.
> >
>
> Although this change is harmless because
> dpu_encoder_helper_get_dsc(phys_enc) will not return a valid DSC mask
> for the WB encoder, hence the setup_intf_cfg will just skip the DSC
> programming, I also agree that we can skip setting the intf_cfg.dsc for
> the writeback encoder in this patch.
Since both of you agree that it is useless I'll drop this in V2. Have
to confess that I know nothing about the writeback interface and haven't
even read the code; does it run in parallel to a "physical" (e.g.
DP/DSI) interface to capture screenshots (or even video) of what is
currently being shown on the screen? By that logic the WB may have
needed to know what is going on in the HW, but it wouldn't have made any
sense regardless if the presented planes first pass through DSC before
being captured. Something for me to read up on :)
- Marijn