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 (and one of the math fixes now
allows us to change slice_count in the panel driver, which would corrupt
previously).
Changes since v1:
- Split patch 6 into two separately backportable Fixes: patches;
- Additionally remove num_enc from msm_display_topology in favour of
num_dsc;
- Reorder patches to have all Fixes: at the beginning for easier
picking;
- Fix existing multiline comment while editing it anyway;
- Add missing Signed-off-by to patch 5.
v1: https://lore.kernel.org/linux-arm-msm/[email protected]/T/#u
Marijn Suijten (8):
drm/msm/dpu: 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 resources to be returned
drm/msm/dpu: Reject topologies for which no DSC blocks are available
drm/msm/dpu: Remove num_enc from topology struct in favour of num_dsc
drm/msm/dpu: Implement DSC binding to PP block for CTL V1
drm/msm/dpu: Add DSC configuration for SM8150 and SM8250
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 12 +++++----
.../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 1 +
.../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 1 +
.../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 | 14 ++++++++--
drivers/gpu/drm/msm/dsi/dsi_host.c | 7 ++---
drivers/gpu/drm/msm/msm_drv.h | 2 --
10 files changed, 82 insertions(+), 18 deletions(-)
--
2.39.0
Downstream calls this num_enc yet the DSC patches introduced a new
num_dsc struct member, leaving num_enc effectively unused.
Fixes: 7e9cc175b159 ("drm/msm/disp/dpu1: Add support for DSC in topology")
Signed-off-by: Marijn Suijten <[email protected]>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 9 ++++-----
drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 4 ++--
drivers/gpu/drm/msm/msm_drv.h | 2 --
3 files changed, 6 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 9c6817b5a194..a158cd502d38 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -579,19 +579,18 @@ static struct msm_display_topology dpu_encoder_get_topology(
topology.num_dspp = topology.num_lm;
}
- topology.num_enc = 0;
topology.num_intf = intf_count;
if (dpu_enc->dsc) {
- /* In case of Display Stream Compression (DSC), we would use
- * 2 encoders, 2 layer mixers and 1 interface
+ /*
+ * In case of Display Stream Compression (DSC), we would use
+ * 2 DSC encoders, 2 layer mixers and 1 interface
* this is power optimal and can drive up to (including) 4k
* screens
*/
- topology.num_enc = 2;
topology.num_dsc = 2;
- topology.num_intf = 1;
topology.num_lm = 2;
+ topology.num_intf = 1;
}
return topology;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
index dcbf03d2940a..5e7aa0f3a31c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
@@ -548,8 +548,8 @@ static int _dpu_rm_populate_requirements(
{
reqs->topology = req_topology;
- DRM_DEBUG_KMS("num_lm: %d num_enc: %d num_intf: %d\n",
- reqs->topology.num_lm, reqs->topology.num_enc,
+ DRM_DEBUG_KMS("num_lm: %d num_dsc: %d num_intf: %d\n",
+ reqs->topology.num_lm, reqs->topology.num_dsc,
reqs->topology.num_intf);
return 0;
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index d4e0ef608950..74626a271f46 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -82,14 +82,12 @@ enum msm_event_wait {
/**
* struct msm_display_topology - defines a display topology pipeline
* @num_lm: number of layer mixers used
- * @num_enc: number of compression encoder blocks used
* @num_intf: number of interfaces the panel is mounted on
* @num_dspp: number of dspp blocks used
* @num_dsc: number of Display Stream Compression (DSC) blocks used
*/
struct msm_display_topology {
u32 num_lm;
- u32 num_enc;
u32 num_intf;
u32 num_dspp;
u32 num_dsc;
--
2.39.0
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.
^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.
Fixes: bb00a452d6f7 ("drm/msm/dpu: Refactor resource manager")
Signed-off-by: Marijn Suijten <[email protected]>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 5 +++++
1 file changed, 5 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..8471d04bff50 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
@@ -660,6 +660,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.39.0
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]>
Reviewed-by: Dmitry Baryshkov <[email protected]>
Reviewed-by: Abhinav Kumar <[email protected]>
---
drivers/gpu/drm/msm/dsi/dsi_host.c | 5 +++--
1 file changed, 3 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..3409a4275d4a 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -855,11 +855,12 @@ 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.39.0
On Thu, Dec 22, 2022 at 12:19:35AM +0100, 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 (and one of the math fixes now
> allows us to change slice_count in the panel driver, which would corrupt
> previously).
>
> Changes since v1:
>
> - Split patch 6 into two separately backportable Fixes: patches;
> - Additionally remove num_enc from msm_display_topology in favour of
> num_dsc;
> - Reorder patches to have all Fixes: at the beginning for easier
> picking;
> - Fix existing multiline comment while editing it anyway;
> - Add missing Signed-off-by to patch 5.
Please note that Electric Boogaloo/Boogaloo Boys has been appropriated by
US alt-right groups, and so is really not a great thing to put into the
cover letter for your patch series. For the next round, please use a meme
that isn't tarnished like this.
Thanks, Daniel
>
> v1: https://lore.kernel.org/linux-arm-msm/[email protected]/T/#u
>
> Marijn Suijten (8):
> drm/msm/dpu: 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 resources to be returned
> drm/msm/dpu: Reject topologies for which no DSC blocks are available
> drm/msm/dpu: Remove num_enc from topology struct in favour of num_dsc
> drm/msm/dpu: Implement DSC binding to PP block for CTL V1
> drm/msm/dpu: Add DSC configuration for SM8150 and SM8250
>
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 12 +++++----
> .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 1 +
> .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 1 +
> .../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 | 14 ++++++++--
> drivers/gpu/drm/msm/dsi/dsi_host.c | 7 ++---
> drivers/gpu/drm/msm/msm_drv.h | 2 --
> 10 files changed, 82 insertions(+), 18 deletions(-)
>
> --
> 2.39.0
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On 22/12/2022 01:19, 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.
>
> ^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.
>
> Fixes: bb00a452d6f7 ("drm/msm/dpu: Refactor resource manager")
> Signed-off-by: Marijn Suijten <[email protected]>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 5 +++++
> 1 file changed, 5 insertions(+)
I think the patch is not fully correct. Please check resource
availability during allocation. I wouldn't expect an error from
get_assigned_resources because of resource exhaustion.
--
With best wishes
Dmitry
On 22/12/2022 01:19, Marijn Suijten wrote:
> Downstream calls this num_enc yet the DSC patches introduced a new
> num_dsc struct member, leaving num_enc effectively unused.
>
> Fixes: 7e9cc175b159 ("drm/msm/disp/dpu1: Add support for DSC in topology")
> Signed-off-by: Marijn Suijten <[email protected]>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 9 ++++-----
> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 4 ++--
> drivers/gpu/drm/msm/msm_drv.h | 2 --
> 3 files changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 9c6817b5a194..a158cd502d38 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -579,19 +579,18 @@ static struct msm_display_topology dpu_encoder_get_topology(
> topology.num_dspp = topology.num_lm;
> }
>
> - topology.num_enc = 0;
> topology.num_intf = intf_count;
>
> if (dpu_enc->dsc) {
> - /* In case of Display Stream Compression (DSC), we would use
> - * 2 encoders, 2 layer mixers and 1 interface
> + /*
> + * In case of Display Stream Compression (DSC), we would use
> + * 2 DSC encoders, 2 layer mixers and 1 interface
> * this is power optimal and can drive up to (including) 4k
> * screens
> */
> - topology.num_enc = 2;
> topology.num_dsc = 2;
> - topology.num_intf = 1;
> topology.num_lm = 2;
> + topology.num_intf = 1;
Unless there is a reason, please move num_intf assignment back while
preparing v3.
With that fixed:
Reviewed-by: Dmitry Baryshkov <[email protected]>
> }
>
> return topology;
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> index dcbf03d2940a..5e7aa0f3a31c 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> @@ -548,8 +548,8 @@ static int _dpu_rm_populate_requirements(
> {
> reqs->topology = req_topology;
>
> - DRM_DEBUG_KMS("num_lm: %d num_enc: %d num_intf: %d\n",
> - reqs->topology.num_lm, reqs->topology.num_enc,
> + DRM_DEBUG_KMS("num_lm: %d num_dsc: %d num_intf: %d\n",
> + reqs->topology.num_lm, reqs->topology.num_dsc,
> reqs->topology.num_intf);
>
> return 0;
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index d4e0ef608950..74626a271f46 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -82,14 +82,12 @@ enum msm_event_wait {
> /**
> * struct msm_display_topology - defines a display topology pipeline
> * @num_lm: number of layer mixers used
> - * @num_enc: number of compression encoder blocks used
> * @num_intf: number of interfaces the panel is mounted on
> * @num_dspp: number of dspp blocks used
> * @num_dsc: number of Display Stream Compression (DSC) blocks used
> */
> struct msm_display_topology {
> u32 num_lm;
> - u32 num_enc;
> u32 num_intf;
> u32 num_dspp;
> u32 num_dsc;
--
With best wishes
Dmitry
On 09/01/2023 01:28, Dmitry Baryshkov wrote:
> On 22/12/2022 01:19, 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.
>>
>> ^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.
>>
>> Fixes: bb00a452d6f7 ("drm/msm/dpu: Refactor resource manager")
>> Signed-off-by: Marijn Suijten <[email protected]>
>> ---
>> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 5 +++++
>> 1 file changed, 5 insertions(+)
>
> I think the patch is not fully correct. Please check resource
> availability during allocation. I wouldn't expect an error from
> get_assigned_resources because of resource exhaustion.
>
Another option, since allocation functions (except DSC) already have
these safety checks: check error message to mention internal
inconstency: allocated resource doesn't exist.
--
With best wishes
Dmitry
On 2023-01-05 15:49:58, Daniel Vetter wrote:
> On Thu, Dec 22, 2022 at 12:19:35AM +0100, Marijn Suijten wrote:
> > [..]
>
> Please note that Electric Boogaloo/Boogaloo Boys has been appropriated by
> US alt-right groups, and so is really not a great thing to put into the
> cover letter for your patch series. For the next round, please use a meme
> that isn't tarnished like this.
Apologies for that, I wasn't aware of this abuse as a non-US citizen and
hope this series is not offending anyone.
As far as I recall this series was set to be applied for 6.3 yet Dmitry
seems to have just posted some additional comments. May have been
confusion on my end.
Hence we do now need another cheeky title, conveying that we're already
on the second round of fixes for DSC and it is still not working on
major SoCs/boards.
- Marijn
On 2023-01-09 01:30:29, Dmitry Baryshkov wrote:
> On 09/01/2023 01:28, Dmitry Baryshkov wrote:
> > On 22/12/2022 01:19, 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.
> >>
> >> ^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.
> >>
> >> Fixes: bb00a452d6f7 ("drm/msm/dpu: Refactor resource manager")
> >> Signed-off-by: Marijn Suijten <[email protected]>
> >> ---
> >> ? drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 5 +++++
> >> ? 1 file changed, 5 insertions(+)
> >
> > I think the patch is not fully correct. Please check resource
> > availability during allocation. I wouldn't expect an error from
> > get_assigned_resources because of resource exhaustion.
Theoretically patch 5/8 should take care of this, and we should never
reach this failure condition. Emphasis on /should/, this may happen
again if/when another block type is added with sub-par resource
allocation and assignment implementation.
> Another option, since allocation functions (except DSC) already have
> these safety checks: check error message to mention internal
> inconstency: allocated resource doesn't exist.
Is this a suggestion for the wording of the error message?
- Marijn
On 2023-01-09 01:31:57, Dmitry Baryshkov wrote:
> On 22/12/2022 01:19, Marijn Suijten wrote:
> > Downstream calls this num_enc yet the DSC patches introduced a new
> > num_dsc struct member, leaving num_enc effectively unused.
> >
> > Fixes: 7e9cc175b159 ("drm/msm/disp/dpu1: Add support for DSC in topology")
> > Signed-off-by: Marijn Suijten <[email protected]>
> > ---
> > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 9 ++++-----
> > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 4 ++--
> > drivers/gpu/drm/msm/msm_drv.h | 2 --
> > 3 files changed, 6 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > index 9c6817b5a194..a158cd502d38 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > @@ -579,19 +579,18 @@ static struct msm_display_topology dpu_encoder_get_topology(
> > topology.num_dspp = topology.num_lm;
> > }
> >
> > - topology.num_enc = 0;
> > topology.num_intf = intf_count;
> >
> > if (dpu_enc->dsc) {
> > - /* In case of Display Stream Compression (DSC), we would use
> > - * 2 encoders, 2 layer mixers and 1 interface
> > + /*
> > + * In case of Display Stream Compression (DSC), we would use
> > + * 2 DSC encoders, 2 layer mixers and 1 interface
> > * this is power optimal and can drive up to (including) 4k
> > * screens
> > */
> > - topology.num_enc = 2;
> > topology.num_dsc = 2;
> > - topology.num_intf = 1;
> > topology.num_lm = 2;
> > + topology.num_intf = 1;
>
> Unless there is a reason, please move num_intf assignment back while
> preparing v3.
The assignment was reordered to match the order described in the comment
right above, such that this reads more naturally. Not sure if it's
worth sending that as a separate fix, or drop it entirely.
> With that fixed:
>
> Reviewed-by: Dmitry Baryshkov <[email protected]>
<snip>
On Mon, 9 Jan 2023 at 10:21, Marijn Suijten
<[email protected]> wrote:
>
> On 2023-01-09 01:31:57, Dmitry Baryshkov wrote:
> > On 22/12/2022 01:19, Marijn Suijten wrote:
> > > Downstream calls this num_enc yet the DSC patches introduced a new
> > > num_dsc struct member, leaving num_enc effectively unused.
> > >
> > > Fixes: 7e9cc175b159 ("drm/msm/disp/dpu1: Add support for DSC in topology")
> > > Signed-off-by: Marijn Suijten <[email protected]>
> > > ---
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 9 ++++-----
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 4 ++--
> > > drivers/gpu/drm/msm/msm_drv.h | 2 --
> > > 3 files changed, 6 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > index 9c6817b5a194..a158cd502d38 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > @@ -579,19 +579,18 @@ static struct msm_display_topology dpu_encoder_get_topology(
> > > topology.num_dspp = topology.num_lm;
> > > }
> > >
> > > - topology.num_enc = 0;
> > > topology.num_intf = intf_count;
> > >
> > > if (dpu_enc->dsc) {
> > > - /* In case of Display Stream Compression (DSC), we would use
> > > - * 2 encoders, 2 layer mixers and 1 interface
> > > + /*
> > > + * In case of Display Stream Compression (DSC), we would use
> > > + * 2 DSC encoders, 2 layer mixers and 1 interface
> > > * this is power optimal and can drive up to (including) 4k
> > > * screens
> > > */
> > > - topology.num_enc = 2;
> > > topology.num_dsc = 2;
> > > - topology.num_intf = 1;
> > > topology.num_lm = 2;
> > > + topology.num_intf = 1;
> >
> > Unless there is a reason, please move num_intf assignment back while
> > preparing v3.
>
> The assignment was reordered to match the order described in the comment
> right above, such that this reads more naturally. Not sure if it's
> worth sending that as a separate fix, or drop it entirely.
I see. Sounds logical then. Let's keep it as is.
>
> > With that fixed:
> >
> > Reviewed-by: Dmitry Baryshkov <[email protected]>
>
> <snip>
--
With best wishes
Dmitry
On Mon, 9 Jan 2023 at 10:24, Marijn Suijten
<[email protected]> wrote:
>
> On 2023-01-09 01:30:29, Dmitry Baryshkov wrote:
> > On 09/01/2023 01:28, Dmitry Baryshkov wrote:
> > > On 22/12/2022 01:19, 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.
> > >>
> > >> ^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.
> > >>
> > >> Fixes: bb00a452d6f7 ("drm/msm/dpu: Refactor resource manager")
> > >> Signed-off-by: Marijn Suijten <[email protected]>
> > >> ---
> > >> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 5 +++++
> > >> 1 file changed, 5 insertions(+)
> > >
> > > I think the patch is not fully correct. Please check resource
> > > availability during allocation. I wouldn't expect an error from
> > > get_assigned_resources because of resource exhaustion.
>
> Theoretically patch 5/8 should take care of this, and we should never
> reach this failure condition. Emphasis on /should/, this may happen
> again if/when another block type is added with sub-par resource
> allocation and assignment implementation.
Yeah. Maybe swapping 4/8 and 5/8 makes sense.
>
> > Another option, since allocation functions (except DSC) already have
> > these safety checks: check error message to mention internal
> > inconstency: allocated resource doesn't exist.
>
> Is this a suggestion for the wording of the error message?
Yes. Because the current message makes one think that it is output
during allocation / assignment to encoder, while this is a safety net.
>
> - Marijn
--
With best wishes
Dmitry
On 2023-01-09 11:06:45, Dmitry Baryshkov wrote:
> On Mon, 9 Jan 2023 at 10:24, Marijn Suijten
> <[email protected]> wrote:
> >
> > On 2023-01-09 01:30:29, Dmitry Baryshkov wrote:
> > > On 09/01/2023 01:28, Dmitry Baryshkov wrote:
> > > > On 22/12/2022 01:19, 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.
> > > >>
> > > >> ^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.
> > > >>
> > > >> Fixes: bb00a452d6f7 ("drm/msm/dpu: Refactor resource manager")
> > > >> Signed-off-by: Marijn Suijten <[email protected]>
> > > >> ---
> > > >> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 5 +++++
> > > >> 1 file changed, 5 insertions(+)
> > > >
> > > > I think the patch is not fully correct. Please check resource
> > > > availability during allocation. I wouldn't expect an error from
> > > > get_assigned_resources because of resource exhaustion.
> >
> > Theoretically patch 5/8 should take care of this, and we should never
> > reach this failure condition. Emphasis on /should/, this may happen
> > again if/when another block type is added with sub-par resource
> > allocation and assignment implementation.
>
> Yeah. Maybe swapping 4/8 and 5/8 makes sense.
Ack.
> > > Another option, since allocation functions (except DSC) already have
> > > these safety checks: check error message to mention internal
> > > inconstency: allocated resource doesn't exist.
> >
> > Is this a suggestion for the wording of the error message?
>
> Yes. Because the current message makes one think that it is output
> during allocation / assignment to encoder, while this is a safety net.
Good. So the patch is correct, just the wording is off, which I fully
agree on. This isn't allocating anything, just handing out what was
previously allocated (and is a safety net).
- Marijn
On 09/01/2023 19:12, Marijn Suijten wrote:
> On 2023-01-09 11:06:45, Dmitry Baryshkov wrote:
>> On Mon, 9 Jan 2023 at 10:24, Marijn Suijten
>> <[email protected]> wrote:
>>>
>>> On 2023-01-09 01:30:29, Dmitry Baryshkov wrote:
>>>> On 09/01/2023 01:28, Dmitry Baryshkov wrote:
>>>>> On 22/12/2022 01:19, 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.
>>>>>>
>>>>>> ^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.
>>>>>>
>>>>>> Fixes: bb00a452d6f7 ("drm/msm/dpu: Refactor resource manager")
>>>>>> Signed-off-by: Marijn Suijten <[email protected]>
>>>>>> ---
>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 5 +++++
>>>>>> 1 file changed, 5 insertions(+)
>>>>>
>>>>> I think the patch is not fully correct. Please check resource
>>>>> availability during allocation. I wouldn't expect an error from
>>>>> get_assigned_resources because of resource exhaustion.
>>>
>>> Theoretically patch 5/8 should take care of this, and we should never
>>> reach this failure condition. Emphasis on /should/, this may happen
>>> again if/when another block type is added with sub-par resource
>>> allocation and assignment implementation.
>>
>> Yeah. Maybe swapping 4/8 and 5/8 makes sense.
>
> Ack.
>
>>>> Another option, since allocation functions (except DSC) already have
>>>> these safety checks: check error message to mention internal
>>>> inconstency: allocated resource doesn't exist.
>>>
>>> Is this a suggestion for the wording of the error message?
>>
>> Yes. Because the current message makes one think that it is output
>> during allocation / assignment to encoder, while this is a safety net.
>
> Good. So the patch is correct, just the wording is off, which I fully
> agree on. This isn't allocating anything, just handing out what was
> previously allocated (and is a safety net).
Yes. Please excuse me if my original message was not 100% clear.
>
> - Marijn
--
With best wishes
Dmitry
On Thu, 22 Dec 2022 00:19:35 +0100, 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.
>
> [...]
Applied, thanks!
[1/8] drm/msm/dpu: Wire up DSC mask for active CTL configuration
https://gitlab.freedesktop.org/lumag/msm/-/commit/c2d2c62da1fc
[2/8] drm/msm/dsi: Use DSC slice(s) packet size to compute word count
https://gitlab.freedesktop.org/lumag/msm/-/commit/bbd1bccdcf4e
[3/8] drm/msm/dsi: Flip greater-than check for slice_count and slice_per_intf
https://gitlab.freedesktop.org/lumag/msm/-/commit/85b5a40991dd
[5/8] drm/msm/dpu: Reject topologies for which no DSC blocks are available
https://gitlab.freedesktop.org/lumag/msm/-/commit/f52b965c9434
[6/8] drm/msm/dpu: Remove num_enc from topology struct in favour of num_dsc
https://gitlab.freedesktop.org/lumag/msm/-/commit/9ce765395f41
[7/8] drm/msm/dpu: Implement DSC binding to PP block for CTL V1
https://gitlab.freedesktop.org/lumag/msm/-/commit/086116ae1410
[8/8] drm/msm/dpu: Add DSC configuration for SM8150 and SM8250
https://gitlab.freedesktop.org/lumag/msm/-/commit/8cc4c9de15f4
Best regards,
--
Dmitry Baryshkov <[email protected]>