| Message ID | 20240918100620.103536-3-angelogioacchino.delregno@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soc: mediatek: mtk-cmdq-helper: Various cleanups | expand |
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;
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;
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; >
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; >>
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;
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(-)