| Message ID | 1392117421.5686.4.camel@x220 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 >
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
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. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 ?
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. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 "...").
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 :-)
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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(+)