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

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

[Kuogee Hsieh]

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(-)

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;
 }
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Abhinav Kumar]

On 4/11/2023 2:09 PM, Kuogee Hsieh wrote:
> 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")

I cannot find any fixes tag with this hash.

This is the right one.

Fixes: f2803ee91a41 ("drm/msm/disp/dpu1: Add DSC support in RM")

> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---

Otherwise, seems quite reasonable to me,

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

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

[Dmitry Baryshkov]

On 12/04/2023 00:09, Kuogee Hsieh wrote:
> 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")

$ git log -p -1 c985d7bb64ff
fatal: ambiguous argument 'c985d7bb64ff': unknown revision or path not 
in the working tree.

I assume this was generated against some internal tree. Please. Please. 
Don't do this again. I hoped that this was pointed out ages ago, but 
probably not. No fixes, commits, tests against internal trees, no matter 
how precious they are to you.

> 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(-)
> 
> 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;
>   }

-- 
With best wishes
Dmitry

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

[Marijn Suijten]

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.

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.

> 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(-)
> 
> 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.

Thanks!

- Marijn

>  		return ret;
> +	}
>  
>  	return ret;
>  }
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

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

[Abhinav Kumar]

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

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@quicinc.com>
>> ---
>>   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.
> 
> Thanks!
> 
> - Marijn
> 
>>   		return ret;
>> +	}
>>   
>>   	return ret;
>>   }
>> -- 
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project
>>

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

[Dmitry Baryshkov]

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.

Marijn proposed to pass struct dpu_foo_cfg directly to 
dpu_hw_foo_init(). This will allow us to drop these checks completely.

For the time being, I think it might be better to add these checks for 
DSC for the sake of uniformity.

> 
> 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@quicinc.com>
>>> ---
>>>   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
>>>

-- 
With best wishes
Dmitry

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

[Abhinav Kumar]

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@quicinc.com>
>>>> ---
>>>>   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
>>>>
> 

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

[Marijn Suijten]

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

Re: [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

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

[Marijn Suijten]

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.

- Marijn

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

[Dmitry Baryshkov]

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

-- 
With best wishes
Dmitry

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

[Abhinav Kumar]

On 4/12/2023 11:50 AM, 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? ..
> 

Its complete.

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

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

WB_MAX, DSC_MAX, LM_MAX etc are RM limits not catalog limits.

For example, LM_MAX is 8 but in the future if could have a HW which has 
10 LMs. That time if LM_MAX is not increased, its just a SW number.

Catalog on the other hand, can still list 10 LMs but with the catch that 
it uses the indices from the rm. So its just an implicit understanding 
here that catalog uses indices from RM.

Nothing prevents someone from manually adding an entry and forgetting to 
update the *_MAX in the RM.

Although, yes we will catch that in reviews.

> [..]
>> 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.
> 

Lets see what you have in mind. As I said, I am not too obsessed with 
this patch. So i dont want to spend more time convincing why it should 
be there.

> - Marijn