Re: [PATCH v2] [media] vb2: core: Lower the log level of debug outputs

[Hans Verkuil]

On 06/28/2017 12:57 AM, Hirokazu Honda wrote:
> Hi,
> 
> According to patch work, this patch are not approved yet and its
> status are "Changes Requested."
> What changes are necessary actually?
> If there is no necessary change, can you approve this patch?

I was considering to have more fine grained control by changing
the debug parameter to a bitmask. But after thinking about it a
bit more I decided that this patch is OK after all.

I'll pick it up the next time I prepare a pull request.

Regards,

Hans

> 
> Best,
> Hirokazu Honda
> 
> On Thu, Jun 8, 2017 at 2:33 PM, Joe Perches  wrote:
>> On Thu, 2017-06-08 at 14:24 +0900, Tomasz Figa wrote:
>>> On Thu, Jun 8, 2017 at 2:16 PM, Joe Perches  wrote:
>> []
 If there automated systems that rely on specific levels, then
 changing the levels of individual messages could also cause
 those automated systems to fail.
>>>
>>> Well, that might be true for some of them indeed. I was thinking about
>>> our use case, which relies on particular numbers to get expected
>>> verbosity levels not caring about particular messages. I guess the
>>> break all or none rule is going to apply here, so we should do the
>>> bitmap conversion indeed. :)
>>>
>>> On the other hand, I think it would be still preferable to do the
>>> conversion in a separate patch.
>>
>> Right.  No worries.
>>

Re: [PATCH v2] [media] vb2: core: Lower the log level of debug outputs

[Hirokazu Honda]

Hi,

According to patch work, this patch are not approved yet and its
status are "Changes Requested."
What changes are necessary actually?
If there is no necessary change, can you approve this patch?

Best,
Hirokazu Honda

On Thu, Jun 8, 2017 at 2:33 PM, Joe Perches  wrote:
> On Thu, 2017-06-08 at 14:24 +0900, Tomasz Figa wrote:
>> On Thu, Jun 8, 2017 at 2:16 PM, Joe Perches  wrote:
> []
>> > If there automated systems that rely on specific levels, then
>> > changing the levels of individual messages could also cause
>> > those automated systems to fail.
>>
>> Well, that might be true for some of them indeed. I was thinking about
>> our use case, which relies on particular numbers to get expected
>> verbosity levels not caring about particular messages. I guess the
>> break all or none rule is going to apply here, so we should do the
>> bitmap conversion indeed. :)
>>
>> On the other hand, I think it would be still preferable to do the
>> conversion in a separate patch.
>
> Right.  No worries.
>

Re: [PATCH v2] [media] vb2: core: Lower the log level of debug outputs

[Joe Perches]

On Thu, 2017-06-08 at 14:24 +0900, Tomasz Figa wrote:
> On Thu, Jun 8, 2017 at 2:16 PM, Joe Perches  wrote:
[]
> > If there automated systems that rely on specific levels, then
> > changing the levels of individual messages could also cause
> > those automated systems to fail.
> 
> Well, that might be true for some of them indeed. I was thinking about
> our use case, which relies on particular numbers to get expected
> verbosity levels not caring about particular messages. I guess the
> break all or none rule is going to apply here, so we should do the
> bitmap conversion indeed. :)
> 
> On the other hand, I think it would be still preferable to do the
> conversion in a separate patch.

Right.  No worries.

Re: [PATCH v2] [media] vb2: core: Lower the log level of debug outputs

[Tomasz Figa]

On Thu, Jun 8, 2017 at 2:16 PM, Joe Perches  wrote:
> On Thu, 2017-06-08 at 13:39 +0900, Tomasz Figa wrote:
>> On Thu, Jun 8, 2017 at 12:24 PM, Hirokazu Honda  wrote:
>> > Hi,
>> >
>> > I completely understand bitmask method now.
>> > I agree to the idea, but it is necessary to change the specification of
>> > a debug parameter.
>> >  (We probably need to change a document about that?)
>> > For example, there is maybe a user who set a debug parameter 3.
>> > The user assume that logs whose levels are less than 4 are shown.
>> > However, after the bitmask method is adopted, someday the logs whose
>> > level is 1 or 2 are only shown, not 3 level logs are not shown.
>> > This will be confusing to users.
>>
>> I think I have to agree with Hirokazu here. Even though it's only
>> about debugging, there might be some automatic testing systems that
>> actually rely on certain values here.
>
> I think it's a non-argument.
>
> If there automated systems that rely on specific levels, then
> changing the levels of individual messages could also cause
> those automated systems to fail.

Well, that might be true for some of them indeed. I was thinking about
our use case, which relies on particular numbers to get expected
verbosity levels not caring about particular messages. I guess the
break all or none rule is going to apply here, so we should do the
bitmap conversion indeed. :)

On the other hand, I think it would be still preferable to do the
conversion in a separate patch.

Best regards,
Tomasz

Re: [PATCH v2] [media] vb2: core: Lower the log level of debug outputs

[Joe Perches]

On Thu, 2017-06-08 at 13:39 +0900, Tomasz Figa wrote:
> On Thu, Jun 8, 2017 at 12:24 PM, Hirokazu Honda  wrote:
> > Hi,
> > 
> > I completely understand bitmask method now.
> > I agree to the idea, but it is necessary to change the specification of
> > a debug parameter.
> >  (We probably need to change a document about that?)
> > For example, there is maybe a user who set a debug parameter 3.
> > The user assume that logs whose levels are less than 4 are shown.
> > However, after the bitmask method is adopted, someday the logs whose
> > level is 1 or 2 are only shown, not 3 level logs are not shown.
> > This will be confusing to users.
> 
> I think I have to agree with Hirokazu here. Even though it's only
> about debugging, there might be some automatic testing systems that
> actually rely on certain values here.

I think it's a non-argument.

If there automated systems that rely on specific levels, then
changing the levels of individual messages could also cause
those automated systems to fail.

Re: [PATCH v2] [media] vb2: core: Lower the log level of debug outputs

[Tomasz Figa]

On Thu, Jun 8, 2017 at 12:24 PM, Hirokazu Honda  wrote:
> Hi,
>
> I completely understand bitmask method now.
> I agree to the idea, but it is necessary to change the specification of
> a debug parameter.
>  (We probably need to change a document about that?)
> For example, there is maybe a user who set a debug parameter 3.
> The user assume that logs whose levels are less than 4 are shown.
> However, after the bitmask method is adopted, someday the logs whose
> level is 1 or 2 are only shown, not 3 level logs are not shown.
> This will be confusing to users.

I think I have to agree with Hirokazu here. Even though it's only
about debugging, there might be some automatic testing systems that
actually rely on certain values here. It probably shouldn't be
considered hard ABI, but that still could be a significant annoyance
for everyone.

However, one could add this in an incremental way, i.e. add a new
debug_mask parameter that would be used by dprinkt(), while making the
original debug parameter simply update the debug_mask whenever it's
changed.

I still think that it should be made with a separate patch, though, as
adjusting the levels and changing the filtering method are orthogonal.

Best regards,
Tomasz

Re: [PATCH v2] [media] vb2: core: Lower the log level of debug outputs

[Hirokazu Honda]

Hi,

I completely understand bitmask method now.
I agree to the idea, but it is necessary to change the specification of
a debug parameter.
 (We probably need to change a document about that?)
For example, there is maybe a user who set a debug parameter 3.
The user assume that logs whose levels are less than 4 are shown.
However, after the bitmask method is adopted, someday the logs whose
level is 1 or 2 are only shown, not 3 level logs are not shown.
This will be confusing to users.
The function that users can select a log method is necessary (e.g.
implement dprintk_bitmask and dprintk_level)

The main purpose of my patch is to not output much log messages.
I think the current patch is enough to accomplish the purpose.

Changing the method is a related but different task from my patch.
Therefore, I think it should be done in another patch.

Best,
Hirokazu Honda

On Wed, Jun 7, 2017 at 6:01 PM, Hans Verkuil  wrote:
> On 31/05/17 06:06, Joe Perches wrote:
>> On Wed, 2017-05-31 at 12:28 +0900, Hirokazu Honda wrote:
>>> If I understand a bitmap correctly, it is necessary to change the log level
>>> for each message.
>>> I didn't mean a bitmap will take a long CPU time.
>>> I mean the work to change so takes a long time.
>>
>> No, none of the messages or levels need change,
>> only the >= test changes to & so that for instance,
>> level 1 and level 3 messages could be emitted
>> without also emitting level 2 messages.
>>
>> The patch suggested is all that would be required.
>>
>
> I prefer the solution that Joe proposed as well.
>
> It's more useful, esp. with a complex beast like vb2.
>
> Regards,
>
> Hans

Re: [PATCH v2] [media] vb2: core: Lower the log level of debug outputs

[Hans Verkuil]

On 31/05/17 06:06, Joe Perches wrote:
> On Wed, 2017-05-31 at 12:28 +0900, Hirokazu Honda wrote:
>> If I understand a bitmap correctly, it is necessary to change the log level
>> for each message.
>> I didn't mean a bitmap will take a long CPU time.
>> I mean the work to change so takes a long time.
> 
> No, none of the messages or levels need change,
> only the >= test changes to & so that for instance,
> level 1 and level 3 messages could be emitted
> without also emitting level 2 messages.
> 
> The patch suggested is all that would be required.
> 

I prefer the solution that Joe proposed as well.

It's more useful, esp. with a complex beast like vb2.

Regards,

Hans

Re: [PATCH v2] [media] vb2: core: Lower the log level of debug outputs

[Joe Perches]

On Wed, 2017-05-31 at 12:28 +0900, Hirokazu Honda wrote:
> If I understand a bitmap correctly, it is necessary to change the log level
> for each message.
> I didn't mean a bitmap will take a long CPU time.
> I mean the work to change so takes a long time.

No, none of the messages or levels need change,
only the >= test changes to & so that for instance,
level 1 and level 3 messages could be emitted
without also emitting level 2 messages.

The patch suggested is all that would be required.

Re: [PATCH v2] [media] vb2: core: Lower the log level of debug outputs

[Joe Perches]

On Wed, 2017-05-31 at 11:05 +0900, Hirokazu Honda wrote:
> Although bitmap is useful, there is need to change the log level for each
> log.
> Because it will take a longer time, it should be done in another patch.

I have no idea what you mean.

A bit & comparison is typically an identical instruction
cycle count to a >= comparison.

Re: [PATCH v2] [media] vb2: core: Lower the log level of debug outputs

[Joe Perches]

On Tue, 2017-05-30 at 18:49 +0900, Hirokazu Honda wrote:
> Some debug output whose log level is set 1 flooded the log.
> Their log level is lowered to find the important log easily.

Maybe use pr_debug instead?

Perhaps it would be better to change the level to a bitmap
so these can be more individually controlled.

Maybe add MODULE_PARM_DESC too.

Perhaps something like below (without the pr_debug conversion)

---
 drivers/media/v4l2-core/videobuf2-core.c | 11 ++-
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c 
b/drivers/media/v4l2-core/videobuf2-core.c
index 94afbbf92807..88ae2b238115 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -31,12 +31,13 @@
 
 static int debug;
 module_param(debug, int, 0644);
+MODULE_PARM_DESC(debug, "debugging output control bitmap (values from 0-31)")
 
-#define dprintk(level, fmt, arg...)  \
-   do {  \
-   if (debug >= level)   \
-   pr_info("vb2-core: %s: " fmt, __func__, ## arg); \
-   } while (0)
+#define dprintk(level, fmt, ...)   \
+do {   \
+   if (debug & BIT(level)) \
+   pr_info("vb2-core: %s: " fmt, __func__, ##__VA_ARGS__); \
+} while (0)
 
 #ifdef CONFIG_VIDEO_ADV_DEBUG