v4l: omap4iss: Remove VIDEO_OMAP4_DEBUG

PATCH In review Paul Bolle 5 participants 21 messages Feb 9, 2014 – Mar 6, 2014

Subsystems (2)

Media (V4L/DVB) Staging

lore

Share & tools

lore

Paul Bolle

author

2014-02-09 15:09 UTC #0 lore raw

PATCH 1/1 v4l: omap4iss: Remove VIDEO_OMAP4_DEBUG

Commit d632dfefd36f ("[media] v4l: omap4iss: Add support for OMAP4 camera interface - Build system") added a Kconfig entry for VIDEO_OMAP4_DEBUG. But nothing uses that symbol.

This entry was apparently copied from a similar entry for "OMAP 3 Camera debug messages". But a corresponding Makefile line is missing. Besides, the debug code also depends on a mysterious ISS_ISR_DEBUG macro. This Kconfig entry can be removed.

Someone familiar with the code might be able to say what to do with the code depending on the DEBUG and ISS_ISR_DEBUG macros.

Signed-off-by Paul Bolle <[email protected]>

---
Untested.

 drivers/staging/media/omap4iss/Kconfig | 6 ------
 1 file changed, 6 deletions(-)

drivers/staging/media/omap4iss/Kconfig +0 -6

index b9fe753..78b0fba 100644
@@ -4,9 +4,3 @@ config VIDEO_OMAP4
 	select VIDEOBUF2_DMA_CONTIG
 	---help---
 	  Driver for an OMAP 4 ISS controller.
-
-config VIDEO_OMAP4_DEBUG
-	bool "OMAP 4 Camera debug messages"
-	depends on VIDEO_OMAP4
-	---help---
-	  Enable debug messages on OMAP 4 ISS controller driver.

Signature · 1 line

1.8.5.3

Laurent Pinchart

2014-02-10 14:12 UTC #1 lore raw

Hi Paul,

Thank you for the patch.

Quoted 8 lines · On Sunday 09 February 2014 16:09:37 Paul Bolle wrote:

Commit d632dfefd36f ("[media] v4l: omap4iss: Add support for OMAP4 camera interface - Build system") added a Kconfig entry for VIDEO_OMAP4_DEBUG. But nothing uses that symbol.
This entry was apparently copied from a similar entry for "OMAP 3 Camera debug messages". But a corresponding Makefile line is missing. Besides, the debug code also depends on a mysterious ISS_ISR_DEBUG macro. This Kconfig entry can be removed.

What about adding the associated Makefile line instead to #define DEBUG when VIDEO_OMAP4_DEBUG is selected, as with the OMAP3 ISP driver ?

Quoted 2 lines

Someone familiar with the code might be able to say what to do with the code depending on the DEBUG and ISS_ISR_DEBUG macros.

ISS_ISR_DEBUG is expected to be set by manually modifying the source code, as it prints lots of messages in interrupt context.

Quoted 21 lines

Signed-off-by Paul Bolle <[email protected]>
---
Untested.
 drivers/staging/media/omap4iss/Kconfig | 6 ------
 1 file changed, 6 deletions(-)
(quoted diff) +0 -0
b/drivers/staging/media/omap4iss/Kconfig index b9fe753..78b0fba 100644
drivers/staging/media/omap4iss/Kconfig +0 -6
@@ -4,9 +4,3 @@ config VIDEO_OMAP4
 	select VIDEOBUF2_DMA_CONTIG
 	---help---
 	  Driver for an OMAP 4 ISS controller.
-
-config VIDEO_OMAP4_DEBUG
-	bool "OMAP 4 Camera debug messages"
-	depends on VIDEO_OMAP4
-	---help---
-	  Enable debug messages on OMAP 4 ISS controller driver.

Signature · 3 lines

Regards,

Laurent Pinchart

Paul Bolle

author

2014-02-10 15:13 UTC #2 lore raw

Laurent,

Quoted 10 lines · Laurent Pinchart

Quoted 8 lines · On Sunday 09 February 2014 16:09:37 Paul Bolle wrote:
Commit d632dfefd36f ("[media] v4l: omap4iss: Add support for OMAP4 camera interface - Build system") added a Kconfig entry for VIDEO_OMAP4_DEBUG. But nothing uses that symbol.
This entry was apparently copied from a similar entry for "OMAP 3 Camera debug messages". But a corresponding Makefile line is missing. Besides, the debug code also depends on a mysterious ISS_ISR_DEBUG macro. This Kconfig entry can be removed.

Quoted 8 lines

What about adding the associated Makefile line instead to #define DEBUG when VIDEO_OMAP4_DEBUG is selected, as with the OMAP3 ISP driver ?
Quoted 2 lines
Someone familiar with the code might be able to say what to do with the code depending on the DEBUG and ISS_ISR_DEBUG macros.
ISS_ISR_DEBUG is expected to be set by manually modifying the source code, as it prints lots of messages in interrupt context.

Which renders the DEBUG macro pointless. Or does the code use some dev_dbg()-like magic, which is only triggered if the DEBUG macro is set?

Thanks,

Paul Bolle

Laurent Pinchart

2014-02-10 15:28 UTC #3 lore raw

Hi Paul,

Quoted 14 lines · On Monday 10 February 2014 16:13:51 Paul Bolle wrote:

Quoted 13 lines · Laurent Pinchart
Quoted 8 lines · On Sunday 09 February 2014 16:09:37 Paul Bolle wrote:
Commit d632dfefd36f ("[media] v4l: omap4iss: Add support for OMAP4 camera interface - Build system") added a Kconfig entry for VIDEO_OMAP4_DEBUG. But nothing uses that symbol.
This entry was apparently copied from a similar entry for "OMAP 3 Camera debug messages". But a corresponding Makefile line is missing. Besides, the debug code also depends on a mysterious ISS_ISR_DEBUG macro. This Kconfig entry can be removed.
What about adding the associated Makefile line instead to #define DEBUG when VIDEO_OMAP4_DEBUG is selected, as with the OMAP3 ISP driver ?

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
Someone familiar with the code might be able to say what to do with the code depending on the DEBUG and ISS_ISR_DEBUG macros.
ISS_ISR_DEBUG is expected to be set by manually modifying the source code, as it prints lots of messages in interrupt context.
Which renders the DEBUG macro pointless. Or does the code use some dev_dbg()-like magic, which is only triggered if the DEBUG macro is set?

Yes, dev_dbg() is used.

Signature · 3 lines

Regards,

Laurent Pinchart

Paul Bolle

author

2014-02-11 11:17 UTC #4 lore raw

PATCH v2 1/1 v4l: omap4iss: Add DEBUG compiler flag

Commit d632dfefd36f ("[media] v4l: omap4iss: Add support for OMAP4 camera interface - Build system") added a Kconfig entry for VIDEO_OMAP4_DEBUG. But nothing uses that symbol.

This entry was apparently copied from a similar entry for "OMAP 3 Camera debug messages". The OMAP 3 entry is used to set the DEBUG compiler flag, which enables calls of dev_dbg().

So add a Makefile line to do that for omap4iss too.

Signed-off-by Paul Bolle <[email protected]>

---
0) v1 was called "[media] v4l: omap4iss: Remove VIDEO_OMAP4_DEBUG". This versions implements Laurent's alternative (which is much better).

1) Still untested.

 drivers/staging/media/omap4iss/Makefile | 2 ++
 1 file changed, 2 insertions(+)

drivers/staging/media/omap4iss/Makefile +2 -0

index a716ce9..f46c6bd 100644
@@ -1,5 +1,7 @@
 # Makefile for OMAP4 ISS driver
 
+ccflags-$(CONFIG_VIDEO_OMAP4_DEBUG) += -DDEBUG
+
 omap4-iss-objs += \
 	iss.o iss_csi2.o iss_csiphy.o iss_ipipeif.o iss_ipipe.o iss_resizer.o iss_video.o

Signature · 1 line

1.8.5.3

Laurent Pinchart

Acked-by on v2 1/1

2014-02-11 12:37 UTC #5 lore raw

Re: [PATCH v2] [media] v4l: omap4iss: Add DEBUG compiler flag

Hi Paul,

Thank you for the patch.

Quoted 11 lines · On Tuesday 11 February 2014 12:17:01 Paul Bolle wrote:

Commit d632dfefd36f ("[media] v4l: omap4iss: Add support for OMAP4 camera interface - Build system") added a Kconfig entry for VIDEO_OMAP4_DEBUG. But nothing uses that symbol.
This entry was apparently copied from a similar entry for "OMAP 3 Camera debug messages". The OMAP 3 entry is used to set the DEBUG compiler flag, which enables calls of dev_dbg().
So add a Makefile line to do that for omap4iss too.
Signed-off-by Paul Bolle <[email protected]>

Acked-by Laurent Pinchart <[email protected]>

and applied to my tree.

Quoted 3 lines

---
0) v1 was called "[media] v4l: omap4iss: Remove VIDEO_OMAP4_DEBUG". This versions implements Laurent's alternative (which is much better).

Thanks :-)

Quoted 17 lines

1) Still untested.
 drivers/staging/media/omap4iss/Makefile | 2 ++
 1 file changed, 2 insertions(+)
(quoted diff) +0 -0
b/drivers/staging/media/omap4iss/Makefile index a716ce9..f46c6bd 100644
drivers/staging/media/omap4iss/Makefile +2 -0
@@ -1,5 +1,7 @@
 # Makefile for OMAP4 ISS driver
 
+ccflags-$(CONFIG_VIDEO_OMAP4_DEBUG) += -DDEBUG
+
 omap4-iss-objs += \
 	iss.o iss_csi2.o iss_csiphy.o iss_ipipeif.o iss_ipipe.o iss_resizer.o
iss_video.o

Signature · 3 lines

Regards,

Laurent Pinchart

Mauro Carvalho Chehab

2014-03-05 20:10 UTC #6 lore raw

Re: [PATCH v2] [media] v4l: omap4iss: Add DEBUG compiler flag

Em Tue, 11 Feb 2014 13:38:51 +0100
Laurent Pinchart <[email protected]> escreveu:

Quoted 39 lines

Hi Paul,
Thank you for the patch.
Quoted 11 lines · On Tuesday 11 February 2014 12:17:01 Paul Bolle wrote:
Commit d632dfefd36f ("[media] v4l: omap4iss: Add support for OMAP4 camera interface - Build system") added a Kconfig entry for VIDEO_OMAP4_DEBUG. But nothing uses that symbol.
This entry was apparently copied from a similar entry for "OMAP 3 Camera debug messages". The OMAP 3 entry is used to set the DEBUG compiler flag, which enables calls of dev_dbg().
So add a Makefile line to do that for omap4iss too.
Signed-off-by Paul Bolle <[email protected]>
Acked-by Laurent Pinchart <[email protected]>
and applied to my tree.
Quoted 3 lines
---
0) v1 was called "[media] v4l: omap4iss: Remove VIDEO_OMAP4_DEBUG". This versions implements Laurent's alternative (which is much better).
Thanks :-)
Quoted 12 lines
1) Still untested.
 drivers/staging/media/omap4iss/Makefile | 2 ++
 1 file changed, 2 insertions(+)
(quoted diff) +0 -0
b/drivers/staging/media/omap4iss/Makefile index a716ce9..f46c6bd 100644
drivers/staging/media/omap4iss/Makefile +0 -0
@@ -1,5 +1,7 @@
 # Makefile for OMAP4 ISS driver
 

Quoted 2 lines

drivers/staging/media/omap4iss/Makefile line 4
Quoted 2 lines
drivers/staging/media/omap4iss/Makefile +2 -0
+ccflags-$(CONFIG_VIDEO_OMAP4_DEBUG) += -DDEBUG
+

This seems to be a very bad idea on my eyes. It is just creating an alias to an already existing Kconfig symbol with no good reason.

To be fair, there are also two other drivers doing this:

	drivers/media/platform/omap3isp/Makefile:ccflags-$(CONFIG_VIDEO_OMAP3_DEBUG) += -DDEBUG
	drivers/media/platform/ti-vpe/Makefile:ccflags-$(CONFIG_VIDEO_TI_VPE_DEBUG) += -DDEBUG

It sounds better do to a s/CONFIG_DEBUG/CONFIG_VIDEO_whatever_DEBUG/ inside each of those driver, or to just remove this, and document that one should enable CONFIG_DEBUG if they want to debug v4l2 drivers.

Quoted 4 lines

Quoted 3 lines
drivers/staging/media/omap4iss/Makefile +0 -0
 omap4-iss-objs += \
 	iss.o iss_csi2.o iss_csiphy.o iss_ipipeif.o iss_ipipe.o iss_resizer.o
iss_video.o

Signature · 2 lines

Cheers,
Mauro

Laurent Pinchart

2014-03-05 23:49 UTC #7 lore raw

Re: [PATCH v2] [media] v4l: omap4iss: Add DEBUG compiler flag

Hi Mauro,

Thank you for the review.

Quoted 37 lines · On Wednesday 05 March 2014 17:10:06 Mauro Carvalho Chehab wrote:

Em Tue, 11 Feb 2014 13:38:51 +0100 Laurent Pinchart escreveu:
Quoted 36 lines · On Tuesday 11 February 2014 12:17:01 Paul Bolle wrote:
Quoted 11 lines
Commit d632dfefd36f ("[media] v4l: omap4iss: Add support for OMAP4 camera interface - Build system") added a Kconfig entry for VIDEO_OMAP4_DEBUG. But nothing uses that symbol.
This entry was apparently copied from a similar entry for "OMAP 3 Camera debug messages". The OMAP 3 entry is used to set the DEBUG compiler flag, which enables calls of dev_dbg().
So add a Makefile line to do that for omap4iss too.
Signed-off-by Paul Bolle <[email protected]>
Acked-by Laurent Pinchart <[email protected]>
and applied to my tree.
Quoted 3 lines
---
0) v1 was called "[media] v4l: omap4iss: Remove VIDEO_OMAP4_DEBUG". This versions implements Laurent's alternative (which is much better).
Thanks :-)
Quoted 13 lines
1) Still untested.
 drivers/staging/media/omap4iss/Makefile | 2 ++
 1 file changed, 2 insertions(+)
(quoted diff) +0 -0
b/drivers/staging/media/omap4iss/Makefile index a716ce9..f46c6bd 100644
drivers/staging/media/omap4iss/Makefile +0 -0
@@ -1,5 +1,7 @@
 
 # Makefile for OMAP4 ISS driver
 

Quoted 9 lines

Quoted 2 lines
drivers/staging/media/omap4iss/Makefile line 4
Quoted 2 lines
drivers/staging/media/omap4iss/Makefile +2 -0
+ccflags-$(CONFIG_VIDEO_OMAP4_DEBUG) += -DDEBUG
+
This seems to be a very bad idea on my eyes. It is just creating an alias to an already existing Kconfig symbol with no good reason.
To be fair, there are also two other drivers doing this:

drivers/media/platform/omap3isp/Makefile:ccflags-$(CONFIG_VIDEO_OMAP3_DEBUG

Quoted 7 lines

) += -DDEBUG drivers/media/platform/ti-vpe/Makefile:ccflags-$(CONFIG_VIDEO_TI_VPE_DEBUG) += -DDEBUG
It sounds better do to a s/CONFIG_DEBUG/CONFIG_VIDEO_whatever_DEBUG/ inside each of those driver, or to just remove this, and document that one should enable CONFIG_DEBUG if they want to debug v4l2 drivers.

Please note that -DDEBUG is equivalent to '#define DEBUG', not to '#define CONFIG_DEBUG'. 'DEBUG' needs to be defined for dev_dbg() to have any effect. Furthermore, there's not CONFIG_DEBUG as far as I know.

An alternative to this would be to add

#ifdef CONFIG_VIDEO_OMAP3_DEBUG
#define DEBUG
#endif

at the beginning of each source file of the driver. That doesn't seem very practical to me though.

Quoted 5 lines

Quoted 5 lines
Quoted 5 lines
drivers/staging/media/omap4iss/Makefile +0 -0
 omap4-iss-objs += \
 
iss.o iss_csi2.o iss_csiphy.o iss_ipipeif.o iss_ipipe.o iss_resizer.o
iss_video.o

Signature · 3 lines

Regards,

Laurent Pinchart

Joe Perches

2014-03-06 00:28 UTC #8 lore raw

Re: [PATCH v2] [media] v4l: omap4iss: Add DEBUG compiler flag

Quoted 2 lines · Laurent Pinchart

Please note that -DDEBUG is equivalent to '#define DEBUG', not to '#define CONFIG_DEBUG'. 'DEBUG' needs to be defined for dev_dbg() to have any effect.

Not quite. If CONFIG_DYNAMIC_DEBUG is set, these dev_dbg statements are compiled in but not by default set to emit output. Output can be enabled by using dynamic_debug controls like:

# echo -n 'file omap4iss/* +p' > <debugfs>/dynamic_debug/control

See Documentation/dynamic-debug-howto.txt for more details.

Laurent Pinchart

2014-03-06 00:47 UTC #9 lore raw

Re: [PATCH v2] [media] v4l: omap4iss: Add DEBUG compiler flag

Hi Joe,

Quoted 5 lines · On Wednesday 05 March 2014 16:28:03 Joe Perches wrote:

Quoted 3 lines · Laurent Pinchart
Please note that -DDEBUG is equivalent to '#define DEBUG', not to '#define CONFIG_DEBUG'. 'DEBUG' needs to be defined for dev_dbg() to have any effect.

Quoted 8 lines

Not quite. If CONFIG_DYNAMIC_DEBUG is set, these dev_dbg statements are compiled in but not by default set to emit output. Output can be enabled by using dynamic_debug controls like:
# echo -n 'file omap4iss/* +p' > <debugfs>/dynamic_debug/control
See Documentation/dynamic-debug-howto.txt for more details.

Thank you for the additional information.

Would you recommend to drop driver-specific Kconfig options related to debugging and use CONFIG_DYNAMIC_DEBUG instead ?

Signature · 3 lines

Regards,

Laurent Pinchart

Joe Perches

2014-03-06 01:00 UTC #10 lore raw

Re: [PATCH v2] [media] v4l: omap4iss: Add DEBUG compiler flag

Quoted 2 lines · Laurent Pinchart

Would you recommend to drop driver-specific Kconfig options related to debugging and use CONFIG_DYNAMIC_DEBUG instead ?

For development, sure, if there's sufficient memory.

For embedded systems with limited memory, using dynamic_debug isn't always possible or effective.

Also, there are sometimes reasons to have debugging messages always enabled or emitted.

For those cases, either adding #define DEBUG or using printk(KERN_DEBUG would be fine.

Laurent Pinchart

2014-03-06 01:26 UTC #11 lore raw

Re: [PATCH v2] [media] v4l: omap4iss: Add DEBUG compiler flag

Hi Joe,

Quoted 6 lines · On Wednesday 05 March 2014 17:00:37 Joe Perches wrote:

Quoted 2 lines · Laurent Pinchart
Would you recommend to drop driver-specific Kconfig options related to debugging and use CONFIG_DYNAMIC_DEBUG instead ?
For development, sure, if there's sufficient memory.

Quoted 8 lines

For embedded systems with limited memory, using dynamic_debug isn't always possible or effective.
Also, there are sometimes reasons to have debugging messages always enabled or emitted.
For those cases, either adding #define DEBUG or using printk(KERN_DEBUG would be fine.

My goal here is to offer an easy way for users to enable debugging without requiring changes to the source code. The driver includes various dev_dbg() messages, I don't want to ask people reporting problems to turn them into printk() calls :-) Even adding #define DEBUG at the beginning of the source files is likely too error prone for users without much programming experience.

Signature · 3 lines

Regards,

Laurent Pinchart

Joe Perches

2014-03-06 01:35 UTC #12 lore raw

Re: [PATCH v2] [media] v4l: omap4iss: Add DEBUG compiler flag

Quoted 11 lines · Laurent Pinchart

Quoted 10 lines · On Wednesday 05 March 2014 17:00:37 Joe Perches wrote:
Quoted 2 lines · Laurent Pinchart
Would you recommend to drop driver-specific Kconfig options related to debugging and use CONFIG_DYNAMIC_DEBUG instead ?
For development, sure, if there's sufficient memory.
For embedded systems with limited memory, using dynamic_debug isn't always possible or effective.
Also, there are sometimes reasons to have debugging messages always enabled or emitted.
For those cases, either adding #define DEBUG or using printk(KERN_DEBUG would be fine.

Quoted 2 lines

My goal here is to offer an easy way for users to enable debugging without requiring changes to the source code.

Dynamic debugging works, but various distributions don't have it enabled.

Quoted 4 lines

The driver includes various dev_dbg()
messages, I don't want to ask people reporting problems to turn them into printk() calls :-) Even adding #define DEBUG at the beginning of the source files is likely too error prone for users without much programming experience.

I think your best bet is to add #define DEBUG to a common header file like iss.h or omap4iss.h

Laurent Pinchart

2014-03-06 01:50 UTC #13 lore raw

Re: [PATCH v2] [media] v4l: omap4iss: Add DEBUG compiler flag

Hi Joe,

Quoted 20 lines · On Wednesday 05 March 2014 17:35:42 Joe Perches wrote:

Quoted 13 lines · Laurent Pinchart
Quoted 11 lines · On Wednesday 05 March 2014 17:00:37 Joe Perches wrote:
Quoted 2 lines · Laurent Pinchart
Would you recommend to drop driver-specific Kconfig options related to debugging and use CONFIG_DYNAMIC_DEBUG instead ?
For development, sure, if there's sufficient memory.
For embedded systems with limited memory, using dynamic_debug isn't always possible or effective.
Also, there are sometimes reasons to have debugging messages always enabled or emitted.
For those cases, either adding #define DEBUG or using printk(KERN_DEBUG would be fine.
Quoted 2 lines
My goal here is to offer an easy way for users to enable debugging without requiring changes to the source code.
Dynamic debugging works, but various distributions don't have it enabled.

Quoted 7 lines

Quoted 4 lines
The driver includes various dev_dbg() messages, I don't want to ask people reporting problems to turn them into printk() calls :-) Even adding #define DEBUG at the beginning of the source files is likely too error prone for users without much programming experience.
I think your best bet is to add #define DEBUG to a common header file like iss.h or omap4iss.h

I've thought about that, but it would require iss.h to be included before all other headers. I've also thought about creating an iss-debug.h header to be included first just to #define DEBUG, but decided to go for handling the OMAP4 ISS debug option in the Makefile instead. If that's ugly and discouraged as reported by Mauro I can try to come up with something else.

Signature · 3 lines

Regards,

Laurent Pinchart

Joe Perches

2014-03-06 03:25 UTC #14 lore raw

Re: [PATCH v2] [media] v4l: omap4iss: Add DEBUG compiler flag

On Thu, 2014-03-06 at 02:52 +0100, Laurent Pinchart wrote:

Hi again Laurent

Quoted 5 lines

I've thought about that, but it would require iss.h to be included before all other headers. I've also thought about creating an iss-debug.h header to be included first just to #define DEBUG, but decided to go for handling the OMAP4 ISS debug option in the Makefile instead. If that's ugly and discouraged as reported by Mauro I can try to come up with something else.

Unless debugging logging statements are in system level static inlines, adding #define DEBUG to iss.h should otherwise produce the same output as -DDEBUG in a Makefile.

Laurent Pinchart

2014-03-06 10:54 UTC #15 lore raw

Re: [PATCH v2] [media] v4l: omap4iss: Add DEBUG compiler flag

Hi Joe,

Quoted 11 lines · On Wednesday 05 March 2014 19:25:47 Joe Perches wrote:

Quoted 6 lines · Laurent Pinchart
I've thought about that, but it would require iss.h to be included before all other headers. I've also thought about creating an iss-debug.h header to be included first just to #define DEBUG, but decided to go for handling the OMAP4 ISS debug option in the Makefile instead. If that's ugly and discouraged as reported by Mauro I can try to come up with something else.
Unless debugging logging statements are in system level static inlines, adding #define DEBUG to iss.h should otherwise produce the same output as -DDEBUG in a Makefile.

dev_dbg() is defined in include/linux/device.h as

#if defined(CONFIG_DYNAMIC_DEBUG)
#define dev_dbg(dev, format, ...) \

do {                                                 \
        dynamic_dev_dbg(dev, format, ##__VA_ARGS__); \
} while (0)

#elif defined(DEBUG)
#define dev_dbg(dev, format, arg...) \
dev_printk(KERN_DEBUG, dev, format, ##arg)
#else

#define dev_dbg(dev, format, arg...)                            \
({                                                              \
        if (0)                                                  \
                dev_printk(KERN_DEBUG, dev, format, ##arg);     \
        0;                                                      \

})
#endif

We thus need the #define DEBUG it appear before the first time device.h is included, either directly or indirectly. Adding #define DEBUG to iss.h won't work now as iss.h is included after all system includes (which is the usual practice, #include <...> come before #include "...").

Signature · 3 lines

Regards,

Laurent Pinchart

Joe Perches

2014-03-06 16:48 UTC #16 lore raw

Re: [PATCH v2] [media] v4l: omap4iss: Add DEBUG compiler flag

Quoted 4 lines · Laurent Pinchart

We thus need the #define DEBUG it appear before the first time device.h is included, either directly or indirectly. Adding #define DEBUG to iss.h won't work now as iss.h is included after all system includes (which is the usual practice, #include <...> come before #include "...").

Ahh, right, good point.
I'd forgotten about that include order dependency.

cheers, Joe

Greg Kroah-Hartman

maintainer

2014-03-06 04:45 UTC #17 lore raw

Re: [PATCH v2] [media] v4l: omap4iss: Add DEBUG compiler flag

In reply to Laurent Pinchart

Quoted 13 lines · Laurent Pinchart

Hi Joe,
Quoted 10 lines · On Wednesday 05 March 2014 16:28:03 Joe Perches wrote:
Quoted 3 lines · Laurent Pinchart
Please note that -DDEBUG is equivalent to '#define DEBUG', not to '#define CONFIG_DEBUG'. 'DEBUG' needs to be defined for dev_dbg() to have any effect.
Not quite. If CONFIG_DYNAMIC_DEBUG is set, these dev_dbg statements are compiled in but not by default set to emit output. Output can be enabled by using dynamic_debug controls like:

Quoted 8 lines

Quoted 3 lines
# echo -n 'file omap4iss/* +p' > <debugfs>/dynamic_debug/control
See Documentation/dynamic-debug-howto.txt for more details.
Thank you for the additional information.
Would you recommend to drop driver-specific Kconfig options related to debugging and use CONFIG_DYNAMIC_DEBUG instead ?

Yes, please do that, no one wants to rebuild drivers and subsystems with different options just for debugging.

thanks,

greg k-h

Paul Bolle

author

2014-03-06 10:23 UTC #18 lore raw

Re: [PATCH v2] [media] v4l: omap4iss: Add DEBUG compiler flag

Quoted 6 lines · Greg Kroah-Hartman

Quoted 2 lines · Laurent Pinchart
Would you recommend to drop driver-specific Kconfig options related to debugging and use CONFIG_DYNAMIC_DEBUG instead ?
Yes, please do that, no one wants to rebuild drivers and subsystems with different options just for debugging.

There are 50+ cases of Kconfig options setting the DEBUG flag, so there might be room for a series to remove some of those. (Note that Joe says there are valid reasons to use a Kconfig option to set this flag, if I'm not misunderstanding Joe.) For what it's worth, I've added the list of these (for v3.14-rc5) below.

Paul Bolle

v3.14-rc5:arch/powerpc/platforms/pseries/Makefile:ccflags-$(CONFIG_PPC_PSERIES_DEBUG) += -DDEBUG
v3.14-rc5:drivers/base/Makefile:ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG v3.14-rc5:drivers/base/power/Makefile:ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG
v3.14-rc5:drivers/bcma/Makefile:ccflags-$(CONFIG_BCMA_DEBUG) := -DDEBUG
v3.14-rc5:drivers/dma/Makefile:ccflags-$(CONFIG_DMADEVICES_DEBUG) := -DDEBUG
v3.14-rc5:drivers/gpio/Makefile:ccflags-$(CONFIG_DEBUG_GPIO) += -DDEBUG
v3.14-rc5:drivers/gpu/drm/tegra/Makefile:ccflags-$(CONFIG_DRM_TEGRA_DEBUG) += -DDEBUG v3.14-rc5:drivers/hwmon/Makefile:ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG v3.14-rc5:drivers/i2c/Makefile:ccflags-$(CONFIG_I2C_DEBUG_CORE) := -DDEBUG v3.14-rc5:drivers/i2c/algos/Makefile:ccflags-$(CONFIG_I2C_DEBUG_ALGO) := -DDEBUG v3.14-rc5:drivers/i2c/busses/Makefile:ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG v3.14-rc5:drivers/i2c/muxes/Makefile:ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG v3.14-rc5:drivers/infiniband/hw/amso1100/Kbuild:ccflags-$(CONFIG_INFINIBAND_AMSO1100_DEBUG) := -DDEBUG v3.14-rc5:drivers/infiniband/hw/cxgb3/Makefile:ccflags-$(CONFIG_INFINIBAND_CXGB3_DEBUG) += -DDEBUG v3.14-rc5:drivers/media/platform/omap3isp/Makefile:ccflags-$(CONFIG_VIDEO_OMAP3_DEBUG) += -DDEBUG v3.14-rc5:drivers/media/platform/ti-vpe/Makefile:ccflags-$(CONFIG_VIDEO_TI_VPE_DEBUG) += -DDEBUG v3.14-rc5:drivers/memstick/Makefile:subdir-ccflags-$(CONFIG_MEMSTICK_DEBUG) := -DDEBUG
v3.14-rc5:drivers/misc/cb710/Makefile:ccflags-$(CONFIG_CB710_DEBUG) := -DDEBUG
v3.14-rc5:drivers/misc/sgi-gru/Makefile:ccflags-$(CONFIG_SGI_GRU_DEBUG) := -DDEBUG
v3.14-rc5:drivers/mmc/Makefile:subdir-ccflags-$(CONFIG_MMC_DEBUG) := -DDEBUG v3.14-rc5:drivers/net/caif/Makefile:ccflags-$(CONFIG_CAIF_DEBUG) := -DDEBUG v3.14-rc5:drivers/net/can/Makefile:ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG v3.14-rc5:drivers/net/can/c_can/Makefile:ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG v3.14-rc5:drivers/net/can/cc770/Makefile:ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG v3.14-rc5:drivers/net/can/mscan/Makefile:ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG v3.14-rc5:drivers/net/can/sja1000/Makefile:ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG v3.14-rc5:drivers/net/can/softing/Makefile:ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG v3.14-rc5:drivers/net/can/usb/Makefile:ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
v3.14-rc5:drivers/net/ethernet/dec/tulip/Makefile:ccflags-$(CONFIG_NET_TULIP) := -DDEBUG
v3.14-rc5:drivers/net/wireless/brcm80211/Makefile:subdir-ccflags-$(CONFIG_BRCMDBG) += -DDEBUG
v3.14-rc5:drivers/net/wireless/zd1211rw/Makefile:ccflags-$(CONFIG_ZD1211RW_DEBUG) := -DDEBUG v3.14-rc5:drivers/nfc/Makefile:ccflags-$(CONFIG_NFC_DEBUG) := -DDEBUG v3.14-rc5:drivers/pci/Makefile:ccflags-$(CONFIG_PCI_DEBUG) := -DDEBUG
v3.14-rc5:drivers/pinctrl/Makefile:ccflags-$(CONFIG_DEBUG_PINCTRL) += -DDEBUG
v3.14-rc5:drivers/power/Makefile:ccflags-$(CONFIG_POWER_SUPPLY_DEBUG) := -DDEBUG v3.14-rc5:drivers/pps/Makefile:ccflags-$(CONFIG_PPS_DEBUG) := -DDEBUG v3.14-rc5:drivers/pps/clients/Makefile:ccflags-$(CONFIG_PPS_DEBUG) := -DDEBUG v3.14-rc5:drivers/rapidio/Makefile:subdir-ccflags-$(CONFIG_RAPIDIO_DEBUG) := -DDEBUG v3.14-rc5:drivers/regulator/Makefile:ccflags-$(CONFIG_REGULATOR_DEBUG) += -DDEBUG
v3.14-rc5:drivers/rtc/Makefile:ccflags-$(CONFIG_RTC_DEBUG) := -DDEBUG
v3.14-rc5:drivers/spi/Makefile:ccflags-$(CONFIG_SPI_DEBUG) := -DDEBUG
v3.14-rc5:drivers/staging/comedi/Makefile:ccflags-$(CONFIG_COMEDI_DEBUG) := -DDEBUG
v3.14-rc5:drivers/staging/comedi/drivers/Makefile:ccflags-$(CONFIG_COMEDI_DEBUG) := -DDEBUG
v3.14-rc5:drivers/staging/comedi/kcomedilib/Makefile:ccflags-$(CONFIG_COMEDI_DEBUG) := -DDEBUG
v3.14-rc5:drivers/staging/usbip/Makefile:ccflags-$(CONFIG_USBIP_DEBUG) := -DDEBUG v3.14-rc5:drivers/usb/chipidea/Makefile:ccflags-$(CONFIG_USB_CHIPIDEA_DEBUG) := -DDEBUG
v3.14-rc5:drivers/usb/dwc2/Makefile:ccflags-$(CONFIG_USB_DWC2_DEBUG) += -DDEBUG
v3.14-rc5:drivers/usb/dwc3/Makefile:ccflags-$(CONFIG_USB_DWC3_DEBUG) := -DDEBUG
v3.14-rc5:drivers/usb/gadget/Makefile:ccflags-$(CONFIG_USB_GADGET_DEBUG) := -DDEBUG
v3.14-rc5:drivers/usb/wusbcore/Makefile:ccflags-$(CONFIG_USB_WUSB_CBAF_DEBUG) := -DDEBUG v3.14-rc5:drivers/video/intelfb/Makefile:ccflags-$(CONFIG_FB_INTEL_DEBUG) := -DDEBUG -DREGDUMP v3.14-rc5:drivers/video/omap2/dss/Makefile:ccflags-$(CONFIG_OMAP2_DSS_DEBUG) += -DDEBUG
v3.14-rc5:fs/ntfs/Makefile:ccflags-$(CONFIG_NTFS_DEBUG) += -DDEBUG
v3.14-rc5:kernel/power/Makefile:ccflags-$(CONFIG_PM_DEBUG) := -DDEBUG
v3.14-rc5:net/caif/Makefile:ccflags-$(CONFIG_CAIF_DEBUG) := -DDEBUG
v3.14-rc5:net/rds/Makefile:ccflags-$(CONFIG_RDS_DEBUG) := -DDEBUG

Laurent Pinchart

2014-03-06 10:59 UTC #19 lore raw

Re: [PATCH v2] [media] v4l: omap4iss: Add DEBUG compiler flag

In reply to Greg Kroah-Hartman

Hi Greg,

Quoted 16 lines · On Wednesday 05 March 2014 20:45:29 Greg Kroah-Hartman wrote:

Quoted 15 lines · Laurent Pinchart
Quoted 13 lines · On Wednesday 05 March 2014 16:28:03 Joe Perches wrote:
Quoted 3 lines · Laurent Pinchart
Please note that -DDEBUG is equivalent to '#define DEBUG', not to '#define CONFIG_DEBUG'. 'DEBUG' needs to be defined for dev_dbg() to have any effect.
Not quite. If CONFIG_DYNAMIC_DEBUG is set, these dev_dbg statements are compiled in but not by default set to emit output. Output can be enabled by using dynamic_debug controls like:
# echo -n 'file omap4iss/* +p' > <debugfs>/dynamic_debug/control
See Documentation/dynamic-debug-howto.txt for more details.

Quoted 7 lines

Quoted 4 lines
Thank you for the additional information.
Would you recommend to drop driver-specific Kconfig options related to debugging and use CONFIG_DYNAMIC_DEBUG instead ?
Yes, please do that, no one wants to rebuild drivers and subsystems with different options just for debugging.

Is CONFIG_DYNAMIC_DEBUG lean enough to be used on embedded systems ? Note that people would still have to rebuild their kernel to enable CONFIG_DYNAMIC_DEBUG anyway :-)

Signature · 3 lines

Regards,

Laurent Pinchart

Mauro Carvalho Chehab

2014-03-06 11:59 UTC #20 lore raw

Re: [PATCH v2] [media] v4l: omap4iss: Add DEBUG compiler flag

Em Thu, 06 Mar 2014 12:00:30 +0100
Laurent Pinchart <[email protected]> escreveu:

Quoted 26 lines

Hi Greg,
Quoted 23 lines · On Wednesday 05 March 2014 20:45:29 Greg Kroah-Hartman wrote:
Quoted 11 lines · Laurent Pinchart
Quoted 10 lines · On Wednesday 05 March 2014 16:28:03 Joe Perches wrote:
Quoted 3 lines · Laurent Pinchart
Please note that -DDEBUG is equivalent to '#define DEBUG', not to '#define CONFIG_DEBUG'. 'DEBUG' needs to be defined for dev_dbg() to have any effect.
Not quite. If CONFIG_DYNAMIC_DEBUG is set, these dev_dbg statements are compiled in but not by default set to emit output. Output can be enabled by using dynamic_debug controls like:
Quoted 8 lines
Quoted 3 lines
# echo -n 'file omap4iss/* +p' > <debugfs>/dynamic_debug/control
See Documentation/dynamic-debug-howto.txt for more details.
Thank you for the additional information.
Would you recommend to drop driver-specific Kconfig options related to debugging and use CONFIG_DYNAMIC_DEBUG instead ?
Yes, please do that, no one wants to rebuild drivers and subsystems with different options just for debugging.

I agree that this is the best solution.

Quoted 4 lines

Is CONFIG_DYNAMIC_DEBUG lean enough to be used on embedded systems ? Note that people would still have to rebuild their kernel to enable CONFIG_DYNAMIC_DEBUG anyway :-)

Some distros, like Fedora, ships two different kernels: one compiled with most of those DEBUG macros disabled, and another one with them enabled:

kernel.x86_64 : The Linux kernel
kernel-debug.x86_64 : The Linux kernel compiled with extra debugging enabled

That helps to have a "production" kernel using less memory, yet allowing one to boot with the debug Kernel, if he needs to debug some driver(s).

PS.: In Fedora, in the specific case of CONFIG_DYNAMIC_DEBUG, it has it enabled since, at least, 2010 (when they changed the SCM to git) at the "production" kernel even for ARM. So, I suspect that the extra amount of memory required for it is not much, but I never actually bothered to check.

On my view, except on embedded systems with very very limited memory constraints, it makes sense to keep CONFIG_DYNAMIC_DEBUG always enabled.

Btw, I would expect a reasonable amount of RAM on any embedded system that supports v4l, because video buffers require a lot of memory. Comparing to the size of those buffers, I suspect that the extra amount of memory for the debug strings and code is negligible.

Signature · 2 lines

Cheers,
Mauro