[Freedreno] [PATCH] drm/msm/dpu: add DSC range checking during resource reservation

[Abhinav Kumar]

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