Re: [Freedreno] [PATCH] drm/msm/dpu: add DSC range checking during resource reservation
From: Abhinav Kumar
Date: Wed Apr 12 2023 - 13:48:42 EST
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