| Message ID | 1681247380-1607-1-git-send-email-quic_khsieh@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | drm/msm/dpu: add DSC range checking during resource reservation | expand |
On 4/12/2023 12:38 AM, Marijn Suijten wrote: > On 2023-04-11 18:50:24, Abhinav Kumar wrote: >> >> >> 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. > > These come from the catalog. Here's how it looks for sdm845: > > static struct dpu_dsc_cfg sdm845_dsc[] = { > 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), > }; > > The only way to trigger this newly introduced range check is by omitting > the DSC_x constants and manually writing e.g. an out-of-range value 10 > here, or setting DSC_NONE. This is only allowed for interfaces. > Correct, its just working on an implicit understanding that the indices in the catalog which might still be right stick to the RM limits. Thats why this is not bad to have. > As we trust the kernel, hence this config, the if introduced here (and > already present for other blocks) has pretty much no effect. > >>> 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. > > This suggested change won't make a difference to the range check > introduced here. The range-check validates that the catalog sets `id` > to a sensible value (since we do not use array indices for this, we > could even decide to do so via `[DSC_0] = (struct dpu_dsc_cfg){ ... }` > if we so desire in the future). > > It'll only get rid of the `_xxx_offset` functions looping through the > arrays in the catalog again, to find a catalog pointer with matching > `id` while we aleady have exactly that pointer here via &cat->dsc[i]. > > The only semantic difference incurred by the change is when the same > `id` value is (erroneously) used multiple times in an array: the current > implementation will always find and return the first block while the > suggestion will make sure all blocks are used. > But again, reusing an `id` is an error and shouldn't happen. > >>> 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. > > I'd rather see a separate patch removing them then, as my suggestion > won't affect the legality of the range check. > I think kuogee just added this to keep it consistent with other checks present in the RM. So I didnt see any harm with that. If he did see an issue, i will let him report that here. Otherwise, I dont want to spend more time discussing this bounds check when other blocks already have it. So, upto you guys to fight it out. > - Marijn
On 12/04/2023 21:50, Marijn Suijten wrote: > On 2023-04-12 10:48:18, Abhinav Kumar wrote: > [..] >>> The only way to trigger this newly introduced range check is by omitting >>> the DSC_x constants and manually writing e.g. an out-of-range value 10 >>> here, or setting DSC_NONE. This is only allowed for interfaces. >>> >> >> Correct, its just working on an implicit understanding that the indices >> in the catalog > > .. this sentence appears to be incomplete: what did you want to say? .. > >> which might still be right stick to the RM limits. >> >> Thats why this is not bad to have. > > What do you mean by "RM limits"? We have constants in the kernel that > both define the maximum number of blocks in these arrays and a > predefined set of ids that block can have. These are all used in > constant structs in the catalog, so there's nothing "software" or > SoC-specific limiting about this (except what is available in the > arrays). > > [..] >> I think kuogee just added this to keep it consistent with other checks >> present in the RM. So I didnt see any harm with that. > > Yep, that's the only reason > >> If he did see an issue, i will let him report that here. > > If so an out-of-bounds constant was hardcoded in dpu_hw_catalog.c. > >> Otherwise, I dont want to spend more time discussing this bounds check >> when other blocks already have it. > > I'll whip up a patch to clear out the extraneous lookup (assuming there > is no other reason/dependency for it to be there...) and can follow that > up with removing these range checks of known-good values in `const > struct` fields. Yes, please. > > - Marijn
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"); return ret; + } return ret; }
Perform DSC range checking to make sure correct DSC is requested before reserve resource for it. Fixes: c985d7bb64ff ("drm/msm/disp/dpu1: Add DSC support in RM") Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> --- drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)