On 4/11/2023 6:06 PM, Dmitry Baryshkov wrote:
On 12/04/2023 01:32, Abhinav Kumar wrote:
Hi Marijn
On 4/11/2023 3:24 PM, Marijn Suijten wrote:
Again, don't forget to include previous reviewers in cc, please :)
On 2023-04-11 14:09:40, Kuogee Hsieh wrote:
Perform DSC range checking to make sure correct DSC is requested before
reserve resource for it.
nit: reserving
This isn't performing any range checking for resource reservations /
requests: this is only validating the constants written in our catalog
and seems rather useless. It isn't fixing any real bug either, so the
Fixes: tag below seems extraneous.
Given prior comments from Abhinav that "the kernel should be trusted",
we should remove this validation for all the other blocks instead.
The purpose of this check is that today all our blocks in RM use the
DSC_* enum as the size.
struct dpu_hw_blk *dsc_blks[DSC_MAX - DSC_0];
If the device tree ends up with more DSC blocks than the DSC_* enum,
how can we avoid this issue today? Not because its a bug in device
tree but how many static number of DSCs are hard-coded in RM.
We don't have these blocks in device tree. And dpu_hw_catalog shouldn't
use indices outside of enum dpu_dsc.
ah, my bad, i should have said catalog here. Okay so the expectation is
that dpu_hw_catalog.c will program the indices to match the RM limits.
I still stand by the fact that the hardware capabilities coming from
catalog should be trusted but this is just the SW index.
Marijn proposed to pass struct dpu_foo_cfg directly to
dpu_hw_foo_init(). This will allow us to drop these checks completely.
Ah okay, sure, would like to see that then uniformly get rid of these
checks.
For the time being, I think it might be better to add these checks for
DSC for the sake of uniformity.
Yes, i think so too.
And like you said, this is not specific to DSC. Such checks are
present for other blocks too.
Fixes: c985d7bb64ff ("drm/msm/disp/dpu1: Add DSC support in RM")
Signed-off-by: Kuogee Hsieh <quic_khsieh@xxxxxxxxxxx>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
index f4dda88..95e58f1 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
@@ -1,6 +1,7 @@
// SPDX-License-Identifier: GPL-2.0-only
/*
* Copyright (c) 2016-2018, The Linux Foundation. All rights
reserved.
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights
reserved.
*/
#define pr_fmt(fmt) "[drm:%s] " fmt, __func__
@@ -250,6 +251,11 @@ int dpu_rm_init(struct dpu_rm *rm,
struct dpu_hw_dsc *hw;
const struct dpu_dsc_cfg *dsc = &cat->dsc[i];
+ if (dsc->id < DSC_0 || dsc->id >= DSC_MAX) {
+ DPU_ERROR("skip dsc %d with invalid id\n", dsc->id);
+ continue;
+ }
+
hw = dpu_hw_dsc_init(dsc->id, mmio, cat);
if (IS_ERR_OR_NULL(hw)) {
rc = PTR_ERR(hw);
@@ -557,8 +563,10 @@ static int _dpu_rm_make_reservation(
}
ret = _dpu_rm_reserve_dsc(rm, global_state, enc,
&reqs->topology);
- if (ret)
+ if (ret) {
+ DPU_ERROR("unable to find appropriate DSC\n");
This, while a nice addition, should go in a different patch.
I'd agree here, a separate patch.
Thanks!
- Marijn
return ret;
+ }
return ret;
}
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum,
a Linux Foundation Collaborative Project
