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.
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.
- Marijn
