[PATCH] [media] v4l: omap4iss: Remove VIDEO_OMAP4_DEBUG

[PATCH] [media] v4l: omap4iss: Remove VIDEO_OMAP4_DEBUG

[Paul Bolle]

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 <pebolle@tiscali.nl>
---
Untested.

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

diff --git a/drivers/staging/media/omap4iss/Kconfig b/drivers/staging/media/omap4iss/Kconfig
index b9fe753..78b0fba 100644
--- a/drivers/staging/media/omap4iss/Kconfig
+++ b/drivers/staging/media/omap4iss/Kconfig
@@ -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.
-- 
1.8.5.3

Re: [PATCH] [media] v4l: omap4iss: Remove VIDEO_OMAP4_DEBUG

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

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 ?
 
> 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.

> Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
> ---
> Untested.
> 
>  drivers/staging/media/omap4iss/Kconfig | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/staging/media/omap4iss/Kconfig
> b/drivers/staging/media/omap4iss/Kconfig index b9fe753..78b0fba 100644
> --- a/drivers/staging/media/omap4iss/Kconfig
> +++ b/drivers/staging/media/omap4iss/Kconfig
> @@ -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.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] [media] v4l: omap4iss: Remove VIDEO_OMAP4_DEBUG

[Paul Bolle]

Laurent,

On Mon, 2014-02-10 at 15:13 +0100, Laurent Pinchart wrote:
> 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 ?
>  
> > 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

Re: [PATCH] [media] v4l: omap4iss: Remove VIDEO_OMAP4_DEBUG

[Laurent Pinchart]

Hi Paul,

On Monday 10 February 2014 16:13:51 Paul Bolle wrote:
> On Mon, 2014-02-10 at 15:13 +0100, Laurent Pinchart wrote:
> > 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 ?
> > 
> > > 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.

-- 
Regards,

Laurent Pinchart

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

[Paul Bolle]

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 <pebolle@tiscali.nl>
---
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(+)

diff --git a/drivers/staging/media/omap4iss/Makefile b/drivers/staging/media/omap4iss/Makefile
index a716ce9..f46c6bd 100644
--- a/drivers/staging/media/omap4iss/Makefile
+++ b/drivers/staging/media/omap4iss/Makefile
@@ -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
 
-- 
1.8.5.3

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

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

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 <pebolle@tiscali.nl>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

and applied to my tree.

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

Thanks :-)

> 1) Still untested.
> 
>  drivers/staging/media/omap4iss/Makefile | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/staging/media/omap4iss/Makefile
> b/drivers/staging/media/omap4iss/Makefile index a716ce9..f46c6bd 100644
> --- a/drivers/staging/media/omap4iss/Makefile
> +++ b/drivers/staging/media/omap4iss/Makefile
> @@ -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

-- 
Regards,

Laurent Pinchart

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

[Mauro Carvalho Chehab]

Em Tue, 11 Feb 2014 13:38:51 +0100
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:

> Hi Paul,
> 
> Thank you for the patch.
> 
> 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 <pebolle@tiscali.nl>
> 
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> and applied to my tree.
> 
> > ---
> > 0) v1 was called "[media] v4l: omap4iss: Remove VIDEO_OMAP4_DEBUG". This
> > versions implements Laurent's alternative (which is much better).
> 
> Thanks :-)
> 
> > 1) Still untested.
> > 
> >  drivers/staging/media/omap4iss/Makefile | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/staging/media/omap4iss/Makefile
> > b/drivers/staging/media/omap4iss/Makefile index a716ce9..f46c6bd 100644
> > --- a/drivers/staging/media/omap4iss/Makefile
> > +++ b/drivers/staging/media/omap4iss/Makefile
> > @@ -1,5 +1,7 @@
> >  # Makefile for OMAP4 ISS driver
> > 
> > +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.

> >  omap4-iss-objs += \
> >  	iss.o iss_csi2.o iss_csiphy.o iss_ipipeif.o iss_ipipe.o iss_resizer.o
> > iss_video.o
> 


-- 

Cheers,
Mauro

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

[Laurent Pinchart]

Hi Mauro,

Thank you for the review.

On Wednesday 05 March 2014 17:10:06 Mauro Carvalho Chehab wrote:
> Em Tue, 11 Feb 2014 13:38:51 +0100 Laurent Pinchart escreveu:
> > 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 <pebolle@tiscali.nl>
> > 
> > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > and applied to my tree.
> > 
> > > ---
> > > 0) v1 was called "[media] v4l: omap4iss: Remove VIDEO_OMAP4_DEBUG". This
> > > versions implements Laurent's alternative (which is much better).
> > 
> > Thanks :-)
> > 
> > > 1) Still untested.
> > > 
> > >  drivers/staging/media/omap4iss/Makefile | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/staging/media/omap4iss/Makefile
> > > b/drivers/staging/media/omap4iss/Makefile index a716ce9..f46c6bd 100644
> > > --- a/drivers/staging/media/omap4iss/Makefile
> > > +++ b/drivers/staging/media/omap4iss/Makefile
> > > @@ -1,5 +1,7 @@
> > > 
> > >  # Makefile for OMAP4 ISS driver
> > > 
> > > +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.

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.

> > >  omap4-iss-objs += \
> > >  
> > >  	iss.o iss_csi2.o iss_csiphy.o iss_ipipeif.o iss_ipipe.o iss_resizer.o
> > > 
> > > iss_video.o

-- 
Regards,

Laurent Pinchart

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

[Joe Perches]

On Thu, 2014-03-06 at 00:50 +0100, Laurent Pinchart wrote:

> 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.

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

[Laurent Pinchart]

Hi Joe,

On Wednesday 05 March 2014 16:28:03 Joe Perches wrote:
> On Thu, 2014-03-06 at 00:50 +0100, Laurent Pinchart wrote:
> > 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.

Thank you for the additional information.

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

-- 
Regards,

Laurent Pinchart

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

[Joe Perches]

On Thu, 2014-03-06 at 01:48 +0100, Laurent Pinchart wrote:
> 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.

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

[Laurent Pinchart]

Hi Joe,

On Wednesday 05 March 2014 17:00:37 Joe Perches wrote:
> On Thu, 2014-03-06 at 01:48 +0100, Laurent Pinchart wrote:
> > 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.

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.

-- 
Regards,

Laurent Pinchart

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

[Joe Perches]

On Thu, 2014-03-06 at 02:27 +0100, Laurent Pinchart wrote:
> On Wednesday 05 March 2014 17:00:37 Joe Perches wrote:
> > On Thu, 2014-03-06 at 01:48 +0100, Laurent Pinchart wrote:
> > > 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.
> 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.

> 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

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

[Laurent Pinchart]

Hi Joe,

On Wednesday 05 March 2014 17:35:42 Joe Perches wrote:
> On Thu, 2014-03-06 at 02:27 +0100, Laurent Pinchart wrote:
> > On Wednesday 05 March 2014 17:00:37 Joe Perches wrote:
> > > On Thu, 2014-03-06 at 01:48 +0100, Laurent Pinchart wrote:
> > > > 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.
> > 
> > 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.
> 
> > 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.

-- 
Regards,

Laurent Pinchart

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

[Joe Perches]

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

Hi again Laurent

> 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.

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

[Greg Kroah-Hartman]

On Thu, Mar 06, 2014 at 01:48:29AM +0100, Laurent Pinchart wrote:
> Hi Joe,
> 
> On Wednesday 05 March 2014 16:28:03 Joe Perches wrote:
> > On Thu, 2014-03-06 at 00:50 +0100, Laurent Pinchart wrote:
> > > 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.
> 
> 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

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

[Paul Bolle]

On Wed, 2014-03-05 at 20:45 -0800, Greg Kroah-Hartman wrote:
> On Thu, Mar 06, 2014 at 01:48:29AM +0100, Laurent Pinchart wrote:
> > 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

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

[Laurent Pinchart]

Hi Joe,

On Wednesday 05 March 2014 19:25:47 Joe Perches wrote:
> On Thu, 2014-03-06 at 02:52 +0100, Laurent Pinchart wrote:
> > 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 "...").

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

Hi Greg,

On Wednesday 05 March 2014 20:45:29 Greg Kroah-Hartman wrote:
> On Thu, Mar 06, 2014 at 01:48:29AM +0100, Laurent Pinchart wrote:
> > On Wednesday 05 March 2014 16:28:03 Joe Perches wrote:
> > > On Thu, 2014-03-06 at 00:50 +0100, Laurent Pinchart wrote:
> > > > 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.
> > 
> > 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 :-)

-- 
Regards,

Laurent Pinchart

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

[Mauro Carvalho Chehab]

Em Thu, 06 Mar 2014 12:00:30 +0100
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:

> Hi Greg,
> 
> On Wednesday 05 March 2014 20:45:29 Greg Kroah-Hartman wrote:
> > On Thu, Mar 06, 2014 at 01:48:29AM +0100, Laurent Pinchart wrote:
> > > On Wednesday 05 March 2014 16:28:03 Joe Perches wrote:
> > > > On Thu, 2014-03-06 at 00:50 +0100, Laurent Pinchart wrote:
> > > > > 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.
> > > 
> > > 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.

> 
> 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.

-- 

Cheers,
Mauro

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

[Joe Perches]

On Thu, 2014-03-06 at 11:55 +0100, Laurent Pinchart wrote:
> 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