[v1] soc: mediatek: mtk-cmdq-helper: Various cleanups

[PATCH v1 2/3] soc: mediatek: mtk-cmdq: Mark very unlikely branches as such

Posted by AngeloGioacchino Del Regno 2 months, 1 week ago

Calling cmdq packet builders with an unsupported event number,
or without left/right operands (in the case of logic commands)
means that the caller (another driver) wants to perform an
unsupported operation, so this means that the caller must be
fixed instead.

Anyway, such checks are here for safety and, unless any driver
bug or any kind of misconfiguration is present, will always be
false so add a very unlikely hint for those.

Knowing that CPUs' branch predictors (and compilers, anyway) are
indeed smart about these cases, this is done mainly for human
readability purposes.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/soc/mediatek/mtk-cmdq-helper.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
index 620c371fd1fc..4ffd1a35df87 100644
--- a/drivers/soc/mediatek/mtk-cmdq-helper.c
+++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
@@ -336,7 +336,7 @@ int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event, bool clear)
 	struct cmdq_instruction inst = { {0} };
 	u32 clear_option = clear ? CMDQ_WFE_UPDATE : 0;
 
-	if (event >= CMDQ_MAX_EVENT)
+	if (unlikely(event >= CMDQ_MAX_EVENT))
 		return -EINVAL;
 
 	inst.op = CMDQ_CODE_WFE;
@@ -351,7 +351,7 @@ int cmdq_pkt_acquire_event(struct cmdq_pkt *pkt, u16 event)
 {
 	struct cmdq_instruction inst = {};
 
-	if (event >= CMDQ_MAX_EVENT)
+	if (unlikely(event >= CMDQ_MAX_EVENT))
 		return -EINVAL;
 
 	inst.op = CMDQ_CODE_WFE;
@@ -366,7 +366,7 @@ int cmdq_pkt_clear_event(struct cmdq_pkt *pkt, u16 event)
 {
 	struct cmdq_instruction inst = { {0} };
 
-	if (event >= CMDQ_MAX_EVENT)
+	if (unlikely(event >= CMDQ_MAX_EVENT))
 		return -EINVAL;
 
 	inst.op = CMDQ_CODE_WFE;
@@ -381,7 +381,7 @@ int cmdq_pkt_set_event(struct cmdq_pkt *pkt, u16 event)
 {
 	struct cmdq_instruction inst = {};
 
-	if (event >= CMDQ_MAX_EVENT)
+	if (unlikely(event >= CMDQ_MAX_EVENT))
 		return -EINVAL;
 
 	inst.op = CMDQ_CODE_WFE;
@@ -476,7 +476,7 @@ int cmdq_pkt_logic_command(struct cmdq_pkt *pkt, u16 result_reg_idx,
 {
 	struct cmdq_instruction inst = { {0} };
 
-	if (!left_operand || !right_operand || s_op >= CMDQ_LOGIC_MAX)
+	if (unlikely(!left_operand || !right_operand || s_op >= CMDQ_LOGIC_MAX))
 		return -EINVAL;
 
 	inst.op = CMDQ_CODE_LOGIC;
-- 
2.46.0

Re: [PATCH v1 2/3] soc: mediatek: mtk-cmdq: Mark very unlikely branches as such

Posted by Matthias Brugger 1 month, 4 weeks ago


On 18/09/2024 12:06, AngeloGioacchino Del Regno wrote:
> Calling cmdq packet builders with an unsupported event number,
> or without left/right operands (in the case of logic commands)
> means that the caller (another driver) wants to perform an
> unsupported operation, so this means that the caller must be
> fixed instead.
> 
> Anyway, such checks are here for safety and, unless any driver
> bug or any kind of misconfiguration is present, will always be
> false so add a very unlikely hint for those.
> 
> Knowing that CPUs' branch predictors (and compilers, anyway) are
> indeed smart about these cases, this is done mainly for human
> readability purposes.
> 

Are you really sure we need that? As you mentioned the unlikely() is probably 
useless as compiler and branch predictions will do the job. I don't see the 
complexity in the code to have this annotations for human readability.

I would argue against using unlikely() here as, in general, it is discouraged to 
use it. We will just create a data point of doing things that should only be 
done with very good reason. I don't see the reason here, it will only confuse 
other developers about the use of likely() and unlikely().

Regards,
Matthias

> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>   drivers/soc/mediatek/mtk-cmdq-helper.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> index 620c371fd1fc..4ffd1a35df87 100644
> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> @@ -336,7 +336,7 @@ int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event, bool clear)
>   	struct cmdq_instruction inst = { {0} };
>   	u32 clear_option = clear ? CMDQ_WFE_UPDATE : 0;
>   
> -	if (event >= CMDQ_MAX_EVENT)
> +	if (unlikely(event >= CMDQ_MAX_EVENT))
>   		return -EINVAL;
>   
>   	inst.op = CMDQ_CODE_WFE;
> @@ -351,7 +351,7 @@ int cmdq_pkt_acquire_event(struct cmdq_pkt *pkt, u16 event)
>   {
>   	struct cmdq_instruction inst = {};
>   
> -	if (event >= CMDQ_MAX_EVENT)
> +	if (unlikely(event >= CMDQ_MAX_EVENT))
>   		return -EINVAL;
>   
>   	inst.op = CMDQ_CODE_WFE;
> @@ -366,7 +366,7 @@ int cmdq_pkt_clear_event(struct cmdq_pkt *pkt, u16 event)
>   {
>   	struct cmdq_instruction inst = { {0} };
>   
> -	if (event >= CMDQ_MAX_EVENT)
> +	if (unlikely(event >= CMDQ_MAX_EVENT))
>   		return -EINVAL;
>   
>   	inst.op = CMDQ_CODE_WFE;
> @@ -381,7 +381,7 @@ int cmdq_pkt_set_event(struct cmdq_pkt *pkt, u16 event)
>   {
>   	struct cmdq_instruction inst = {};
>   
> -	if (event >= CMDQ_MAX_EVENT)
> +	if (unlikely(event >= CMDQ_MAX_EVENT))
>   		return -EINVAL;
>   
>   	inst.op = CMDQ_CODE_WFE;
> @@ -476,7 +476,7 @@ int cmdq_pkt_logic_command(struct cmdq_pkt *pkt, u16 result_reg_idx,
>   {
>   	struct cmdq_instruction inst = { {0} };
>   
> -	if (!left_operand || !right_operand || s_op >= CMDQ_LOGIC_MAX)
> +	if (unlikely(!left_operand || !right_operand || s_op >= CMDQ_LOGIC_MAX))
>   		return -EINVAL;
>   
>   	inst.op = CMDQ_CODE_LOGIC;

Re: [PATCH v1 2/3] soc: mediatek: mtk-cmdq: Mark very unlikely branches as such

Posted by AngeloGioacchino Del Regno 1 month, 4 weeks ago

Il 02/10/24 14:41, Matthias Brugger ha scritto:
> 
> 
> On 18/09/2024 12:06, AngeloGioacchino Del Regno wrote:
>> Calling cmdq packet builders with an unsupported event number,
>> or without left/right operands (in the case of logic commands)
>> means that the caller (another driver) wants to perform an
>> unsupported operation, so this means that the caller must be
>> fixed instead.
>>
>> Anyway, such checks are here for safety and, unless any driver
>> bug or any kind of misconfiguration is present, will always be
>> false so add a very unlikely hint for those.
>>
>> Knowing that CPUs' branch predictors (and compilers, anyway) are
>> indeed smart about these cases, this is done mainly for human
>> readability purposes.
>>
> 
> Are you really sure we need that? As you mentioned the unlikely() is probably 
> useless as compiler and branch predictions will do the job. I don't see the 
> complexity in the code to have this annotations for human readability.
> 
> I would argue against using unlikely() here as, in general, it is discouraged to 
> use it. We will just create a data point of doing things that should only be done 
> with very good reason. I don't see the reason here, it will only confuse other 
> developers about the use of likely() and unlikely().
> 

If you have strong opinions I have no problem dropping this.
Perhaps I can add a comment stating "this is very unlikely to happen and should
be dropped after thorough cleanup", if that's better?

Cheers!
Angelo

> Regards,
> Matthias
> 
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> ---
>>   drivers/soc/mediatek/mtk-cmdq-helper.c | 10 +++++-----
>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c 
>> b/drivers/soc/mediatek/mtk-cmdq-helper.c
>> index 620c371fd1fc..4ffd1a35df87 100644
>> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
>> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
>> @@ -336,7 +336,7 @@ int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event, bool clear)
>>       struct cmdq_instruction inst = { {0} };
>>       u32 clear_option = clear ? CMDQ_WFE_UPDATE : 0;
>> -    if (event >= CMDQ_MAX_EVENT)
>> +    if (unlikely(event >= CMDQ_MAX_EVENT))
>>           return -EINVAL;
>>       inst.op = CMDQ_CODE_WFE;
>> @@ -351,7 +351,7 @@ int cmdq_pkt_acquire_event(struct cmdq_pkt *pkt, u16 event)
>>   {
>>       struct cmdq_instruction inst = {};
>> -    if (event >= CMDQ_MAX_EVENT)
>> +    if (unlikely(event >= CMDQ_MAX_EVENT))
>>           return -EINVAL;
>>       inst.op = CMDQ_CODE_WFE;
>> @@ -366,7 +366,7 @@ int cmdq_pkt_clear_event(struct cmdq_pkt *pkt, u16 event)
>>   {
>>       struct cmdq_instruction inst = { {0} };
>> -    if (event >= CMDQ_MAX_EVENT)
>> +    if (unlikely(event >= CMDQ_MAX_EVENT))
>>           return -EINVAL;
>>       inst.op = CMDQ_CODE_WFE;
>> @@ -381,7 +381,7 @@ int cmdq_pkt_set_event(struct cmdq_pkt *pkt, u16 event)
>>   {
>>       struct cmdq_instruction inst = {};
>> -    if (event >= CMDQ_MAX_EVENT)
>> +    if (unlikely(event >= CMDQ_MAX_EVENT))
>>           return -EINVAL;
>>       inst.op = CMDQ_CODE_WFE;
>> @@ -476,7 +476,7 @@ int cmdq_pkt_logic_command(struct cmdq_pkt *pkt, u16 
>> result_reg_idx,
>>   {
>>       struct cmdq_instruction inst = { {0} };
>> -    if (!left_operand || !right_operand || s_op >= CMDQ_LOGIC_MAX)
>> +    if (unlikely(!left_operand || !right_operand || s_op >= CMDQ_LOGIC_MAX))
>>           return -EINVAL;
>>       inst.op = CMDQ_CODE_LOGIC;

-- 
AngeloGioacchino Del Regno
Senior Software Engineer

Collabora Ltd.
Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK
Registered in England & Wales, no. 5513718

Re: [PATCH v1 2/3] soc: mediatek: mtk-cmdq: Mark very unlikely branches as such

Posted by Matthias Brugger 1 month, 4 weeks ago


On 02/10/2024 14:43, AngeloGioacchino Del Regno wrote:
> Il 02/10/24 14:41, Matthias Brugger ha scritto:
>>
>>
>> On 18/09/2024 12:06, AngeloGioacchino Del Regno wrote:
>>> Calling cmdq packet builders with an unsupported event number,
>>> or without left/right operands (in the case of logic commands)
>>> means that the caller (another driver) wants to perform an
>>> unsupported operation, so this means that the caller must be
>>> fixed instead.
>>>
>>> Anyway, such checks are here for safety and, unless any driver
>>> bug or any kind of misconfiguration is present, will always be
>>> false so add a very unlikely hint for those.
>>>
>>> Knowing that CPUs' branch predictors (and compilers, anyway) are
>>> indeed smart about these cases, this is done mainly for human
>>> readability purposes.
>>>
>>
>> Are you really sure we need that? As you mentioned the unlikely() is probably 
>> useless as compiler and branch predictions will do the job. I don't see the 
>> complexity in the code to have this annotations for human readability.
>>
>> I would argue against using unlikely() here as, in general, it is discouraged 
>> to use it. We will just create a data point of doing things that should only 
>> be done with very good reason. I don't see the reason here, it will only 
>> confuse other developers about the use of likely() and unlikely().
>>
> 
> If you have strong opinions I have no problem dropping this.

My take would be to drop it.

> Perhaps I can add a comment stating "this is very unlikely to happen and should
> be dropped after thorough cleanup", if that's better?
> 

As these are exported functions they could be used by out-of-tree modules, so it 
could make sense to check the input parameter. Maybe transform it to 
WARN_ON(event >= CMDQ_MAX_EVENT)?

Regards,
Matthias

> Cheers!
> Angelo
> 
>> Regards,
>> Matthias
>>
>>> Signed-off-by: AngeloGioacchino Del Regno 
>>> <angelogioacchino.delregno@collabora.com>
>>> ---
>>>   drivers/soc/mediatek/mtk-cmdq-helper.c | 10 +++++-----
>>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c 
>>> b/drivers/soc/mediatek/mtk-cmdq-helper.c
>>> index 620c371fd1fc..4ffd1a35df87 100644
>>> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
>>> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
>>> @@ -336,7 +336,7 @@ int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event, bool 
>>> clear)
>>>       struct cmdq_instruction inst = { {0} };
>>>       u32 clear_option = clear ? CMDQ_WFE_UPDATE : 0;
>>> -    if (event >= CMDQ_MAX_EVENT)
>>> +    if (unlikely(event >= CMDQ_MAX_EVENT))
>>>           return -EINVAL;
>>>       inst.op = CMDQ_CODE_WFE;
>>> @@ -351,7 +351,7 @@ int cmdq_pkt_acquire_event(struct cmdq_pkt *pkt, u16 event)
>>>   {
>>>       struct cmdq_instruction inst = {};
>>> -    if (event >= CMDQ_MAX_EVENT)
>>> +    if (unlikely(event >= CMDQ_MAX_EVENT))
>>>           return -EINVAL;
>>>       inst.op = CMDQ_CODE_WFE;
>>> @@ -366,7 +366,7 @@ int cmdq_pkt_clear_event(struct cmdq_pkt *pkt, u16 event)
>>>   {
>>>       struct cmdq_instruction inst = { {0} };
>>> -    if (event >= CMDQ_MAX_EVENT)
>>> +    if (unlikely(event >= CMDQ_MAX_EVENT))
>>>           return -EINVAL;
>>>       inst.op = CMDQ_CODE_WFE;
>>> @@ -381,7 +381,7 @@ int cmdq_pkt_set_event(struct cmdq_pkt *pkt, u16 event)
>>>   {
>>>       struct cmdq_instruction inst = {};
>>> -    if (event >= CMDQ_MAX_EVENT)
>>> +    if (unlikely(event >= CMDQ_MAX_EVENT))
>>>           return -EINVAL;
>>>       inst.op = CMDQ_CODE_WFE;
>>> @@ -476,7 +476,7 @@ int cmdq_pkt_logic_command(struct cmdq_pkt *pkt, u16 
>>> result_reg_idx,
>>>   {
>>>       struct cmdq_instruction inst = { {0} };
>>> -    if (!left_operand || !right_operand || s_op >= CMDQ_LOGIC_MAX)
>>> +    if (unlikely(!left_operand || !right_operand || s_op >= CMDQ_LOGIC_MAX))
>>>           return -EINVAL;
>>>       inst.op = CMDQ_CODE_LOGIC;
>

Re: [PATCH v1 2/3] soc: mediatek: mtk-cmdq: Mark very unlikely branches as such

Posted by AngeloGioacchino Del Regno 1 month, 4 weeks ago

Il 02/10/24 14:58, Matthias Brugger ha scritto:
> 
> 
> On 02/10/2024 14:43, AngeloGioacchino Del Regno wrote:
>> Il 02/10/24 14:41, Matthias Brugger ha scritto:
>>>
>>>
>>> On 18/09/2024 12:06, AngeloGioacchino Del Regno wrote:
>>>> Calling cmdq packet builders with an unsupported event number,
>>>> or without left/right operands (in the case of logic commands)
>>>> means that the caller (another driver) wants to perform an
>>>> unsupported operation, so this means that the caller must be
>>>> fixed instead.
>>>>
>>>> Anyway, such checks are here for safety and, unless any driver
>>>> bug or any kind of misconfiguration is present, will always be
>>>> false so add a very unlikely hint for those.
>>>>
>>>> Knowing that CPUs' branch predictors (and compilers, anyway) are
>>>> indeed smart about these cases, this is done mainly for human
>>>> readability purposes.
>>>>
>>>
>>> Are you really sure we need that? As you mentioned the unlikely() is probably 
>>> useless as compiler and branch predictions will do the job. I don't see the 
>>> complexity in the code to have this annotations for human readability.
>>>
>>> I would argue against using unlikely() here as, in general, it is discouraged to 
>>> use it. We will just create a data point of doing things that should only be 
>>> done with very good reason. I don't see the reason here, it will only confuse 
>>> other developers about the use of likely() and unlikely().
>>>
>>
>> If you have strong opinions I have no problem dropping this.
> 
> My take would be to drop it.
> 

Let's drop it then. :-)

>> Perhaps I can add a comment stating "this is very unlikely to happen and should
>> be dropped after thorough cleanup", if that's better?
>>
> 
> As these are exported functions they could be used by out-of-tree modules, so it 
> could make sense to check the input parameter. Maybe transform it to WARN_ON(event 
>  >= CMDQ_MAX_EVENT)?
> 

The reasoning behind all of this is that those functions are used in drivers'
performance paths, including display and camera.... so a WARN_ON would be even
more against what I'm trying to do.

At this point we can just leave this as it is....

> Regards,
> Matthias
> 
>> Cheers!
>> Angelo
>>
>>> Regards,
>>> Matthias
>>>
>>>> Signed-off-by: AngeloGioacchino Del Regno 
>>>> <angelogioacchino.delregno@collabora.com>
>>>> ---
>>>>   drivers/soc/mediatek/mtk-cmdq-helper.c | 10 +++++-----
>>>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c 
>>>> b/drivers/soc/mediatek/mtk-cmdq-helper.c
>>>> index 620c371fd1fc..4ffd1a35df87 100644
>>>> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
>>>> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
>>>> @@ -336,7 +336,7 @@ int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event, bool clear)
>>>>       struct cmdq_instruction inst = { {0} };
>>>>       u32 clear_option = clear ? CMDQ_WFE_UPDATE : 0;
>>>> -    if (event >= CMDQ_MAX_EVENT)
>>>> +    if (unlikely(event >= CMDQ_MAX_EVENT))
>>>>           return -EINVAL;
>>>>       inst.op = CMDQ_CODE_WFE;
>>>> @@ -351,7 +351,7 @@ int cmdq_pkt_acquire_event(struct cmdq_pkt *pkt, u16 event)
>>>>   {
>>>>       struct cmdq_instruction inst = {};
>>>> -    if (event >= CMDQ_MAX_EVENT)
>>>> +    if (unlikely(event >= CMDQ_MAX_EVENT))
>>>>           return -EINVAL;
>>>>       inst.op = CMDQ_CODE_WFE;
>>>> @@ -366,7 +366,7 @@ int cmdq_pkt_clear_event(struct cmdq_pkt *pkt, u16 event)
>>>>   {
>>>>       struct cmdq_instruction inst = { {0} };
>>>> -    if (event >= CMDQ_MAX_EVENT)
>>>> +    if (unlikely(event >= CMDQ_MAX_EVENT))
>>>>           return -EINVAL;
>>>>       inst.op = CMDQ_CODE_WFE;
>>>> @@ -381,7 +381,7 @@ int cmdq_pkt_set_event(struct cmdq_pkt *pkt, u16 event)
>>>>   {
>>>>       struct cmdq_instruction inst = {};
>>>> -    if (event >= CMDQ_MAX_EVENT)
>>>> +    if (unlikely(event >= CMDQ_MAX_EVENT))
>>>>           return -EINVAL;
>>>>       inst.op = CMDQ_CODE_WFE;
>>>> @@ -476,7 +476,7 @@ int cmdq_pkt_logic_command(struct cmdq_pkt *pkt, u16 
>>>> result_reg_idx,
>>>>   {
>>>>       struct cmdq_instruction inst = { {0} };
>>>> -    if (!left_operand || !right_operand || s_op >= CMDQ_LOGIC_MAX)
>>>> +    if (unlikely(!left_operand || !right_operand || s_op >= CMDQ_LOGIC_MAX))
>>>>           return -EINVAL;
>>>>       inst.op = CMDQ_CODE_LOGIC;
>>

[PATCH v1 1/3] soc: mediatek: mtk-cmdq: Move mask build and append to function
[PATCH v1 2/3] soc: mediatek: mtk-cmdq: Mark very unlikely branches as such
[PATCH v1 3/3] soc: mediatek: mtk-cmdq: Move cmdq_instruction init to declaration