| Message ID | 1496139572.2618.19.camel@perches.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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.
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
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 <hverkuil@xs4all.nl> 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
On Thu, Jun 8, 2017 at 12:24 PM, Hirokazu Honda <hiroh@chromium.org> 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
On Thu, 2017-06-08 at 13:39 +0900, Tomasz Figa wrote: > On Thu, Jun 8, 2017 at 12:24 PM, Hirokazu Honda <hiroh@chromium.org> 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.
On Thu, Jun 8, 2017 at 2:16 PM, Joe Perches <joe@perches.com> wrote: > On Thu, 2017-06-08 at 13:39 +0900, Tomasz Figa wrote: >> On Thu, Jun 8, 2017 at 12:24 PM, Hirokazu Honda <hiroh@chromium.org> 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
On Thu, 2017-06-08 at 14:24 +0900, Tomasz Figa wrote: > On Thu, Jun 8, 2017 at 2:16 PM, Joe Perches <joe@perches.com> 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.
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 <joe@perches.com> wrote: > On Thu, 2017-06-08 at 14:24 +0900, Tomasz Figa wrote: >> On Thu, Jun 8, 2017 at 2:16 PM, Joe Perches <joe@perches.com> 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. >
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 <joe@perches.com> wrote: >> On Thu, 2017-06-08 at 14:24 +0900, Tomasz Figa wrote: >>> On Thu, Jun 8, 2017 at 2:16 PM, Joe Perches <joe@perches.com> 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. >>
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