| Message ID | 1348552998-6596-1-git-send-email-cmahapatra@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2012-09-25 at 11:33 +0530, Chandrabhanu Mahapatra wrote: > The printk in DSSDBG function definition is replaced with dynamic debug enabled > pr_debug(). The use of dynamic debugging provides more flexiblity as each debug > statement can be enabled or disabled dynamically on basis of source filename, > line number, module name etc. by writing to a control file in debugfs > filesystem. For better undertsanding please refer to > Documentation/dynamic-debug-howto.txt. > > The DSSDBGF() differs from DSSDBG() by providing function name. However, > function name, line number, module name and thread ID can be printed through > dynamic debug by setting appropiate flags 'f','l','m' and 't' in the debugfs > control file. So, DSSDBGF instances are replaced with DSSDBG. Typos with: flexiblity, undertsanding, appropiate. > Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com> > --- > drivers/video/omap2/dss/apply.c | 8 ++++---- > drivers/video/omap2/dss/dsi.c | 12 ++++-------- > drivers/video/omap2/dss/dss.h | 34 ++++++++-------------------------- > 3 files changed, 16 insertions(+), 38 deletions(-) > > diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c > index 6354bb8..cb86d94 100644 > --- a/drivers/video/omap2/dss/apply.c > +++ b/drivers/video/omap2/dss/apply.c > @@ -559,7 +559,7 @@ static void dss_ovl_write_regs(struct omap_overlay *ovl) > struct mgr_priv_data *mp; > int r; > > - DSSDBGF("%d", ovl->id); > + DSSDBG("%d", ovl->id); I don't think this is good. It's true that dyn-debug can print the function name, but that's optional. The debug message should be somehow sensible independently, but in this case only a number is printed which is totally meaningless. Either the messages should be modified to give a hint what's going on, or the DSSDBGF could be kept for now. In the above case the debug message could be something like "writing ovl %d regs". However, I think it'd be easier just to keep the DSSDBGF for now, and remove it gradually. > if (!op->enabled || !op->info_dirty) > return; > @@ -594,7 +594,7 @@ static void dss_ovl_write_regs_extra(struct omap_overlay *ovl) > struct ovl_priv_data *op = get_ovl_priv(ovl); > struct mgr_priv_data *mp; > > - DSSDBGF("%d", ovl->id); > + DSSDBG("%d", ovl->id); > > if (!op->extra_info_dirty) > return; > @@ -618,7 +618,7 @@ static void dss_mgr_write_regs(struct omap_overlay_manager *mgr) > struct mgr_priv_data *mp = get_mgr_priv(mgr); > struct omap_overlay *ovl; > > - DSSDBGF("%d", mgr->id); > + DSSDBG("%d", mgr->id); > > if (!mp->enabled) > return; > @@ -644,7 +644,7 @@ static void dss_mgr_write_regs_extra(struct omap_overlay_manager *mgr) > { > struct mgr_priv_data *mp = get_mgr_priv(mgr); > > - DSSDBGF("%d", mgr->id); > + DSSDBG("%d", mgr->id); > > if (!mp->extra_info_dirty) > return; > diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c > index 8d815e3..8304cc6b 100644 > --- a/drivers/video/omap2/dss/dsi.c > +++ b/drivers/video/omap2/dss/dsi.c > @@ -1525,8 +1525,6 @@ int dsi_pll_set_clock_div(struct platform_device *dsidev, > u8 regn_start, regn_end, regm_start, regm_end; > u8 regm_dispc_start, regm_dispc_end, regm_dsi_start, regm_dsi_end; > > - DSSDBGF(); > - > dsi->current_cinfo.clkin = cinfo->clkin; > dsi->current_cinfo.fint = cinfo->fint; > dsi->current_cinfo.clkin4ddr = cinfo->clkin4ddr; > @@ -2334,8 +2332,6 @@ static int dsi_cio_init(struct omap_dss_device *dssdev) > int r; > u32 l; > > - DSSDBGF(); > - > r = dss_dsi_enable_pads(dsi->module_id, dsi_get_lane_mask(dssdev)); > if (r) > return r; > @@ -2686,7 +2682,7 @@ static void dsi_vc_initial_config(struct platform_device *dsidev, int channel) > { > u32 r; > > - DSSDBGF("%d", channel); > + DSSDBG("%d", channel); > > r = dsi_read_reg(dsidev, DSI_VC_CTRL(channel)); > > @@ -2718,7 +2714,7 @@ static int dsi_vc_config_source(struct platform_device *dsidev, int channel, > if (dsi->vc[channel].source == source) > return 0; > > - DSSDBGF("%d", channel); > + DSSDBG("%d", channel); > > dsi_sync_vc(dsidev, channel); > > @@ -3475,7 +3471,7 @@ static int dsi_enter_ulps(struct platform_device *dsidev) > int r, i; > unsigned mask; > > - DSSDBGF(); > + DSSDBG(""); This debug message is even less meaningful than the overlay number =). Again, I think either keep the DSSDBGF, or print something sensible, like "entering ULPS". > > WARN_ON(!dsi_bus_is_locked(dsidev)); > > @@ -4184,7 +4180,7 @@ int omapdss_dsi_set_clocks(struct omap_dss_device *dssdev, > unsigned long pck; > int r; > > - DSSDBGF("ddr_clk %lu, lp_clk %lu", ddr_clk, lp_clk); > + DSSDBG("ddr_clk %lu, lp_clk %lu", ddr_clk, lp_clk); > > mutex_lock(&dsi->lock); > > diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h > index 5e9fd769..3a2caab 100644 > --- a/drivers/video/omap2/dss/dss.h > +++ b/drivers/video/omap2/dss/dss.h > @@ -29,38 +29,20 @@ > > #ifdef DEBUG > extern bool dss_debug; You still left the dss_debug option here, even if it's not used by the DSSDBG anymore. What's your plan about this? Tomi
>> diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c >> index 6354bb8..cb86d94 100644 >> --- a/drivers/video/omap2/dss/apply.c >> +++ b/drivers/video/omap2/dss/apply.c >> @@ -559,7 +559,7 @@ static void dss_ovl_write_regs(struct omap_overlay *ovl) >> struct mgr_priv_data *mp; >> int r; >> >> - DSSDBGF("%d", ovl->id); >> + DSSDBG("%d", ovl->id); > > I don't think this is good. It's true that dyn-debug can print the > function name, but that's optional. The debug message should be somehow > sensible independently, but in this case only a number is printed which > is totally meaningless. > > Either the messages should be modified to give a hint what's going on, > or the DSSDBGF could be kept for now. In the above case the debug > message could be something like "writing ovl %d regs". > > However, I think it'd be easier just to keep the DSSDBGF for now, and > remove it gradually. > >> if (!op->enabled || !op->info_dirty) >> return; >> @@ -594,7 +594,7 @@ static void dss_ovl_write_regs_extra(struct omap_overlay *ovl) >> struct ovl_priv_data *op = get_ovl_priv(ovl); >> struct mgr_priv_data *mp; >> >> - DSSDBGF("%d", ovl->id); >> + DSSDBG("%d", ovl->id); >> >> if (!op->extra_info_dirty) >> return; >> @@ -618,7 +618,7 @@ static void dss_mgr_write_regs(struct omap_overlay_manager *mgr) >> struct mgr_priv_data *mp = get_mgr_priv(mgr); >> struct omap_overlay *ovl; >> >> - DSSDBGF("%d", mgr->id); >> + DSSDBG("%d", mgr->id); >> >> if (!mp->enabled) >> return; >> @@ -644,7 +644,7 @@ static void dss_mgr_write_regs_extra(struct omap_overlay_manager *mgr) >> { >> struct mgr_priv_data *mp = get_mgr_priv(mgr); >> >> - DSSDBGF("%d", mgr->id); >> + DSSDBG("%d", mgr->id); >> >> if (!mp->extra_info_dirty) >> return; >> diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c >> index 8d815e3..8304cc6b 100644 >> --- a/drivers/video/omap2/dss/dsi.c >> +++ b/drivers/video/omap2/dss/dsi.c >> @@ -1525,8 +1525,6 @@ int dsi_pll_set_clock_div(struct platform_device *dsidev, >> u8 regn_start, regn_end, regm_start, regm_end; >> u8 regm_dispc_start, regm_dispc_end, regm_dsi_start, regm_dsi_end; >> >> - DSSDBGF(); >> - >> dsi->current_cinfo.clkin = cinfo->clkin; >> dsi->current_cinfo.fint = cinfo->fint; >> dsi->current_cinfo.clkin4ddr = cinfo->clkin4ddr; >> @@ -2334,8 +2332,6 @@ static int dsi_cio_init(struct omap_dss_device *dssdev) >> int r; >> u32 l; >> >> - DSSDBGF(); >> - >> r = dss_dsi_enable_pads(dsi->module_id, dsi_get_lane_mask(dssdev)); >> if (r) >> return r; >> @@ -2686,7 +2682,7 @@ static void dsi_vc_initial_config(struct platform_device *dsidev, int channel) >> { >> u32 r; >> >> - DSSDBGF("%d", channel); >> + DSSDBG("%d", channel); >> >> r = dsi_read_reg(dsidev, DSI_VC_CTRL(channel)); >> >> @@ -2718,7 +2714,7 @@ static int dsi_vc_config_source(struct platform_device *dsidev, int channel, >> if (dsi->vc[channel].source == source) >> return 0; >> >> - DSSDBGF("%d", channel); >> + DSSDBG("%d", channel); >> >> dsi_sync_vc(dsidev, channel); >> >> @@ -3475,7 +3471,7 @@ static int dsi_enter_ulps(struct platform_device *dsidev) >> int r, i; >> unsigned mask; >> >> - DSSDBGF(); >> + DSSDBG(""); > > This debug message is even less meaningful than the overlay number =). > Again, I think either keep the DSSDBGF, or print something sensible, > like "entering ULPS". > I dont think it would be wise enough to update code for one and keep the older version for another when both DSSDBG and DSSDBGF are almost one and the same. Its better to add something meaningful to the prints as you have mentioned like "writing ovl %d regs" and "DSI entering ULPS". >> >> WARN_ON(!dsi_bus_is_locked(dsidev)); >> >> @@ -4184,7 +4180,7 @@ int omapdss_dsi_set_clocks(struct omap_dss_device *dssdev, >> unsigned long pck; >> int r; >> >> - DSSDBGF("ddr_clk %lu, lp_clk %lu", ddr_clk, lp_clk); >> + DSSDBG("ddr_clk %lu, lp_clk %lu", ddr_clk, lp_clk); >> >> mutex_lock(&dsi->lock); >> >> diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h >> index 5e9fd769..3a2caab 100644 >> --- a/drivers/video/omap2/dss/dss.h >> +++ b/drivers/video/omap2/dss/dss.h >> @@ -29,38 +29,20 @@ >> >> #ifdef DEBUG >> extern bool dss_debug; > > You still left the dss_debug option here, even if it's not used by the > DSSDBG anymore. What's your plan about this? > > Tomi > dss_debug and DEBUG need to remain here as it is being used by functions omap_dispc_irq_handler() and _dsi_print_reset_status() in dispc.c and dsi.c. I am little bit unsure of how to deal with it. There could be a single print in omap_dispc_irq_handler() but it is a bit tricky in _dsi_print_reset_status(). May be a macro like this one can be used in _dsi_print_reset_status() #define DSI_FLD_GET(fld, start, end)\ FLD_GET(dsi_read_reg(dsidev, DSI_##fld), start, end); pr_debug("PLL (%d) CIO (%d) \n PHY (%x%x%x, %d, %d, %d) \n", DSI_FLD_GET(PLL_STATUS, 0, 0), DSI_FLD_GET(COMPLEXIO_CFG1, 29, 29), DSI_FLD_GET(DSIPHY_CFG5, bo, bo), DSI_FLD_GET(DSIPHY_CFG5, b1, b1), ..................................................); This could be defined at the beginning of the function and later at its end. As you had previously mentioned a print like #define PIS(x) (status & DSI_IRQ_##x) ? (#x " ") : "" pr_debug("DSI IRQ: 0x%x: %s%s%s", status, PIS(WAKEUP), PIS(RESYNC), PIS(PLL_LOCK)); could help in print_irq_status() but I am still unsure how to deal with conditional statements in print_irq_status() like if (dss_has_feature(FEAT_MGR_LCD3)) PIS(SYNC_LOST3); Should we use approach like pr_debug("DSI IRQ: 0x%x: %s%s%s%s...", status, PIS(WAKEUP), PIS(RESYNC), PIS(PLL_LOCK) dss_has_feature(FEAT_MGR_LCD3) ? PIS(SYNC_LOST3) : "" ...................................... );
On Tue, 2012-09-25 at 15:00 +0530, Mahapatra, Chandrabhanu wrote: > > This debug message is even less meaningful than the overlay number =). > > Again, I think either keep the DSSDBGF, or print something sensible, > > like "entering ULPS". > > > > I dont think it would be wise enough to update code for one and keep > the older version for another when both DSSDBG and DSSDBGF are almost > one and the same. Its better to add something meaningful to the prints > as you have mentioned like "writing ovl %d regs" and "DSI entering > ULPS". I didn't mean to leave DSSDBGF unchanged. I meant that it should work as it works now, printing the func name, but using pr_debug. > >> > >> WARN_ON(!dsi_bus_is_locked(dsidev)); > >> > >> @@ -4184,7 +4180,7 @@ int omapdss_dsi_set_clocks(struct omap_dss_device *dssdev, > >> unsigned long pck; > >> int r; > >> > >> - DSSDBGF("ddr_clk %lu, lp_clk %lu", ddr_clk, lp_clk); > >> + DSSDBG("ddr_clk %lu, lp_clk %lu", ddr_clk, lp_clk); > >> > >> mutex_lock(&dsi->lock); > >> > >> diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h > >> index 5e9fd769..3a2caab 100644 > >> --- a/drivers/video/omap2/dss/dss.h > >> +++ b/drivers/video/omap2/dss/dss.h > >> @@ -29,38 +29,20 @@ > >> > >> #ifdef DEBUG > >> extern bool dss_debug; > > > > You still left the dss_debug option here, even if it's not used by the > > DSSDBG anymore. What's your plan about this? > > > > Tomi > > > > dss_debug and DEBUG need to remain here as it is being used by > functions omap_dispc_irq_handler() and _dsi_print_reset_status() in It would be good to clean all this in one patch series. > dispc.c and dsi.c. I am little bit unsure of how to deal with it. > There could be a single print in omap_dispc_irq_handler() but it is a > bit tricky in _dsi_print_reset_status(). > > May be a macro like this one can be used in _dsi_print_reset_status() > > #define DSI_FLD_GET(fld, start, end)\ > FLD_GET(dsi_read_reg(dsidev, DSI_##fld), start, end); > > pr_debug("PLL (%d) CIO (%d) \n PHY (%x%x%x, %d, %d, %d) \n", > DSI_FLD_GET(PLL_STATUS, 0, 0), > DSI_FLD_GET(COMPLEXIO_CFG1, 29, 29), > DSI_FLD_GET(DSIPHY_CFG5, bo, bo), > DSI_FLD_GET(DSIPHY_CFG5, b1, b1), > ..................................................); > This could be defined at the beginning of the function and later at its end. Yes, I think something like that could work. I don't see any problem with having temporary helper macros to help creating the debug message. > As you had previously mentioned a print like > > #define PIS(x) (status & DSI_IRQ_##x) ? (#x " ") : "" > > pr_debug("DSI IRQ: 0x%x: %s%s%s", > status, > PIS(WAKEUP), > PIS(RESYNC), > PIS(PLL_LOCK)); > > could help in print_irq_status() but I am still unsure how to deal > with conditional statements in print_irq_status() like > if (dss_has_feature(FEAT_MGR_LCD3)) > PIS(SYNC_LOST3); > Should we use approach like > > pr_debug("DSI IRQ: 0x%x: %s%s%s%s...", > status, > PIS(WAKEUP), > PIS(RESYNC), > PIS(PLL_LOCK) > dss_has_feature(FEAT_MGR_LCD3) ? PIS(SYNC_LOST3) : "" > ...................................... ); Yes, that looks fine to me. Tomi
Hi everyone, this patch series aims at cleaning up of DSS of printk()'s enabled with dss_debug and replace them with generic dynamic debug printing. The 1st patch * replaces printk() in DSSDBG definition with pr_debug() * removes DSSDBGF definition and replaces its instances with DSSDBG() The 2nd patch * cleans up printk()'s in omap_dispc_unregister_isr() and _dsi_print_reset_status() with pr_debug() * removes dss_debug variable Changes with respect to V1: * added debug messages to DSSDBG calls replacing DSSDBGF * added patch "OMAPDSS: Remove dss_debug variable" All your comments and suggestions are welcome. Regards, Chandrabhanu Chandrabhanu Mahapatra (2): OMAPDSS: Cleanup DSSDBG with dynamic pr_debug function OMAPDSS: Remove dss_debug variable drivers/video/omap2/dss/apply.c | 8 +++---- drivers/video/omap2/dss/core.c | 5 ---- drivers/video/omap2/dss/dispc.c | 39 +++++++++++-------------------- drivers/video/omap2/dss/dsi.c | 49 ++++++++++++++++----------------------- drivers/video/omap2/dss/dss.h | 34 +++++---------------------- 5 files changed, 44 insertions(+), 91 deletions(-)
Hi, On Wed, 2012-09-26 at 10:45 +0530, Chandrabhanu Mahapatra wrote: > Hi everyone, > this patch series aims at cleaning up of DSS of printk()'s enabled with > dss_debug and replace them with generic dynamic debug printing. > > The 1st patch > * replaces printk() in DSSDBG definition with pr_debug() > * removes DSSDBGF definition and replaces its instances with DSSDBG() > The 2nd patch > * cleans up printk()'s in omap_dispc_unregister_isr() and > _dsi_print_reset_status() with pr_debug() > * removes dss_debug variable > > Changes with respect to V1: > * added debug messages to DSSDBG calls replacing DSSDBGF > * added patch "OMAPDSS: Remove dss_debug variable" > > All your comments and suggestions are welcome. This doesn't work quite correctly. The problem is in dss.h, where we define DEBUG if CONFIG_OMAP2_DSS_DEBUG_SUPPORT is set. The thing is, DEBUG should be defined before including the kernel headers where the pr_debug etc are defined. So if you try the patches without dynamic debugging enabled, you won't get any debug outputs at all, even if CONFIG_OMAP2_DSS_DEBUG_SUPPORT is set. And for dynamic debug, the Kconfig help says: If a source file is compiled with DEBUG flag set, any pr_debug() calls in it are enabled by default, but can be disabled at runtime as below. Note that DEBUG flag is turned on by many CONFIG_*DEBUG* options. So if we have CONFIG_OMAP2_DSS_DEBUG_SUPPORT set, all the pr_debugs should be enabled by default, which is not the case, again because DEBUG is defined too late. I think setting DEBUG in dss.h should be removed, and instead DEBUG should be set in the makefile if CONFIG_OMAP2_DSS_DEBUG_SUPPORT is set. Tomi
On Wed, Sep 26, 2012 at 7:59 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote: > This doesn't work quite correctly. The problem is in dss.h, where we > define DEBUG if CONFIG_OMAP2_DSS_DEBUG_SUPPORT is set. The thing is, > DEBUG should be defined before including the kernel headers where the > pr_debug etc are defined. > > So if you try the patches without dynamic debugging enabled, you won't > get any debug outputs at all, even if CONFIG_OMAP2_DSS_DEBUG_SUPPORT is > set. > > And for dynamic debug, the Kconfig help says: > > If a source file is compiled with DEBUG flag set, any > pr_debug() calls in it are enabled by default, but can be > disabled at runtime as below. Note that DEBUG flag is > turned on by many CONFIG_*DEBUG* options. > > So if we have CONFIG_OMAP2_DSS_DEBUG_SUPPORT set, all the pr_debugs > should be enabled by default, which is not the case, again because DEBUG > is defined too late. > > I think setting DEBUG in dss.h should be removed, and instead DEBUG > should be set in the makefile if CONFIG_OMAP2_DSS_DEBUG_SUPPORT is set. > > Tomi > Well the documentation lags in describing about the DEBUG flag. I should have checked DYNAMIC_DEBUG in Kconfig and pr_debug definition in printk.h file. #if defined(CONFIG_DYNAMIC_DEBUG) /* dynamic_pr_debug() uses pr_fmt() internally so we don't need it here */ #define pr_debug(fmt, ...) \ dynamic_pr_debug(fmt, ##__VA_ARGS__) #elif defined(DEBUG) #define pr_debug(fmt, ...) \ printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__) #else #define pr_debug(fmt, ...) \ no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__) #endif As per the definition above pr_debug is dynamic with CONFIG_DYNAMIC_DEBUG set or else with DEBUG set it is just a normal kernel debug printk as you have mentioned. I still don't get how even if DEBUG is set before DSSDBG() is defined in dss.c pr_debug() fails to enable. Well anyways, how to do the same in the Makefile? I tried adding ccflags-$(CONFIG_OMAP2_DSS_DEBUG_SUPPORT) += -DEBUG to makefile in dss directory but of no use.
On Thu, 2012-09-27 at 16:20 +0530, Mahapatra, Chandrabhanu wrote: > On Wed, Sep 26, 2012 at 7:59 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote: > > > This doesn't work quite correctly. The problem is in dss.h, where we > > define DEBUG if CONFIG_OMAP2_DSS_DEBUG_SUPPORT is set. The thing is, > > DEBUG should be defined before including the kernel headers where the > > pr_debug etc are defined. > > > > So if you try the patches without dynamic debugging enabled, you won't > > get any debug outputs at all, even if CONFIG_OMAP2_DSS_DEBUG_SUPPORT is > > set. > > > > And for dynamic debug, the Kconfig help says: > > > > If a source file is compiled with DEBUG flag set, any > > pr_debug() calls in it are enabled by default, but can be > > disabled at runtime as below. Note that DEBUG flag is > > turned on by many CONFIG_*DEBUG* options. > > > > So if we have CONFIG_OMAP2_DSS_DEBUG_SUPPORT set, all the pr_debugs > > should be enabled by default, which is not the case, again because DEBUG > > is defined too late. > > > > I think setting DEBUG in dss.h should be removed, and instead DEBUG > > should be set in the makefile if CONFIG_OMAP2_DSS_DEBUG_SUPPORT is set. > > > > Tomi > > > > Well the documentation lags in describing about the DEBUG flag. I > should have checked DYNAMIC_DEBUG in Kconfig and pr_debug definition > in printk.h file. > > #if defined(CONFIG_DYNAMIC_DEBUG) > /* dynamic_pr_debug() uses pr_fmt() internally so we don't need it here */ > #define pr_debug(fmt, ...) \ > dynamic_pr_debug(fmt, ##__VA_ARGS__) > #elif defined(DEBUG) > #define pr_debug(fmt, ...) \ > printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__) > #else > #define pr_debug(fmt, ...) \ > no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__) > #endif > > As per the definition above pr_debug is dynamic with > CONFIG_DYNAMIC_DEBUG set or else with DEBUG set it is just a normal > kernel debug printk as you have mentioned. > I still don't get how even if DEBUG is set before DSSDBG() is defined > in dss.c pr_debug() fails to enable. Because printk.h is included without DEBUG, thus pr_debug is defined as no_printk. > Well anyways, how to do the same in the Makefile? I tried adding > ccflags-$(CONFIG_OMAP2_DSS_DEBUG_SUPPORT) += -DEBUG > to makefile in dss directory but of no use. -D option for the compiler is used to set defines. So it should be -DDEBUG Tomi
Hi everyone, this patch series aims at cleaning up of DSS of printk()'s enabled with dss_debug and replace them with generic dynamic debug printing. The 1st patch * moved DEBUG flag definition to Makefile The 2nd patch * replaces printk() in DSSDBG definition with pr_debug() * removes DSSDBGF definition and replaces its instances with DSSDBG() The 3rd patch * cleans up printk()'s in omap_dispc_unregister_isr() and _dsi_print_reset_status() with pr_debug() * removes dss_debug variable Changes from V1 to V2: * added debug messages to DSSDBG calls * added patch "OMAPDSS: Remove dss_debug variable" Changes from V2 to V3 * added patch "OMAPDSS: Move definition of DEBUG flag to Makefile" All your comments and suggestions are welcome. Refenence Tree: git://gitorious.org/linux-omap-dss2/chandrabhanus-linux.git dss_cleanup Regards, Chandrabhanu Chandrabhanu Mahapatra (3): OMAPDSS: Move definition of DEBUG flag to Makefile OMAPDSS: Cleanup DSSDBG with dynamic pr_debug function OMAPDSS: Remove dss_debug variable drivers/video/omap2/dss/Makefile | 1 + drivers/video/omap2/dss/apply.c | 8 +++---- drivers/video/omap2/dss/core.c | 5 ---- drivers/video/omap2/dss/dispc.c | 39 +++++++++++------------------- drivers/video/omap2/dss/dsi.c | 49 ++++++++++++++++---------------------- drivers/video/omap2/dss/dss.h | 38 +++++------------------------ 6 files changed, 45 insertions(+), 95 deletions(-)
On Fri, 2012-09-28 at 15:53 +0530, Chandrabhanu Mahapatra wrote: > Hi everyone, > this patch series aims at cleaning up of DSS of printk()'s enabled with > dss_debug and replace them with generic dynamic debug printing. > > The 1st patch > * moved DEBUG flag definition to Makefile > The 2nd patch > * replaces printk() in DSSDBG definition with pr_debug() > * removes DSSDBGF definition and replaces its instances with DSSDBG() > The 3rd patch > * cleans up printk()'s in omap_dispc_unregister_isr() and > _dsi_print_reset_status() with pr_debug() > * removes dss_debug variable > > Changes from V1 to V2: > * added debug messages to DSSDBG calls > * added patch "OMAPDSS: Remove dss_debug variable" > > Changes from V2 to V3 > * added patch "OMAPDSS: Move definition of DEBUG flag to Makefile" > > All your comments and suggestions are welcome. There's one thing that's not quite nice about omapdss's debug print behavior after this series. CONFIG_OMAP2_DSS_DEBUG_SUPPORT is marked "default y", and it's also been safe to enable earlier as we had the dss_debug variable to prevent the debug prints. But after this series, the debug prints are enabled, and will spam the kernel log quite heavily. And that happens with both dynamic debugging enabled and disabled. How things should work: For kernels with dynamic debugging disabled: by default the dss debugs are not compiled, and the user needs to explicitly enable them in the kernel config. For kernels with dynamic debugging enabled: by default the dss debugs are compiled in, but not enabled. A Kconfig option can be set to make the debugs enabled by default. In addition to those, we have the debugfs files. Those should be usable regardless of the debug prints. So I suggest the following: - Remove CONFIG_OMAP2_DSS_DEBUG_SUPPORT. We can't re-use it, because it may be enabled in user's kernel configs. - Add new Kconfig option: CONFIG_OMAP2_DSS_DEBUG. This will set DEBUG in the makefile. This is off by default. - Add new Kconfig option: CONFIG_OMAP2_DSS_DEBUGFS. This will be use to decide if debugfs functionality is compiled in or not. This is off by default. Tomi
On Fri, Sep 28, 2012 at 5:04 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote: > On Fri, 2012-09-28 at 15:53 +0530, Chandrabhanu Mahapatra wrote: >> Hi everyone, >> this patch series aims at cleaning up of DSS of printk()'s enabled with >> dss_debug and replace them with generic dynamic debug printing. >> >> The 1st patch >> * moved DEBUG flag definition to Makefile >> The 2nd patch >> * replaces printk() in DSSDBG definition with pr_debug() >> * removes DSSDBGF definition and replaces its instances with DSSDBG() >> The 3rd patch >> * cleans up printk()'s in omap_dispc_unregister_isr() and >> _dsi_print_reset_status() with pr_debug() >> * removes dss_debug variable >> >> Changes from V1 to V2: >> * added debug messages to DSSDBG calls >> * added patch "OMAPDSS: Remove dss_debug variable" >> >> Changes from V2 to V3 >> * added patch "OMAPDSS: Move definition of DEBUG flag to Makefile" >> >> All your comments and suggestions are welcome. > > There's one thing that's not quite nice about omapdss's debug print > behavior after this series. > > CONFIG_OMAP2_DSS_DEBUG_SUPPORT is marked "default y", and it's also been > safe to enable earlier as we had the dss_debug variable to prevent the > debug prints. But after this series, the debug prints are enabled, and > will spam the kernel log quite heavily. > > And that happens with both dynamic debugging enabled and disabled. Yes, I had noticed that but I thought a better way to disable debug prints is to disable both dynamic debugging (CONFIG_DYNAMIC_DEBUG) and CONFIG_OMAP2_DSS_DEBUG_SUPPORT. May be CONFIG_OMAP2_DSS_DEBUG_SUPPORT should have been false by default. > How things should work: > > For kernels with dynamic debugging disabled: by default the dss debugs > are not compiled, and the user needs to explicitly enable them in the > kernel config. > > For kernels with dynamic debugging enabled: by default the dss debugs > are compiled in, but not enabled. A Kconfig option can be set to make > the debugs enabled by default. > > In addition to those, we have the debugfs files. Those should be usable > regardless of the debug prints. > > So I suggest the following: > > - Remove CONFIG_OMAP2_DSS_DEBUG_SUPPORT. We can't re-use it, because it > may be enabled in user's kernel configs. > - Add new Kconfig option: CONFIG_OMAP2_DSS_DEBUG. This will set DEBUG in > the makefile. This is off by default. > - Add new Kconfig option: CONFIG_OMAP2_DSS_DEBUGFS. This will be use to > decide if debugfs functionality is compiled in or not. This is off by > default. > > Tomi > Well, I had a different perception. If one needs to debug then both debugfs and debug prints should be enabled. If one needs only debug prints then CONFIG_DEBUG_FS can be disabled. But above approach seems to provide more flexibilty.
Hi everyone, this patch series aims at cleaning up of DSS of printk()'s enabled with dss_debug and replace them with generic dynamic debug printing. The 1st patch * moved DEBUG flag definition to Makefile The 2nd patch * created two debug config options OMAP2_DSS_DEBUG and OMAP2_DSS_DEBUGFS The 3rd patch * replaces printk() in DSSDBG definition with pr_debug() * removes DSSDBGF definition and replaces its instances with DSSDBG() The 4th patch * cleans up printk()'s in omap_dispc_unregister_isr() and _dsi_print_reset_status() with pr_debug() The 5th patch * removes dss_debug variable Changes from V1 to V2: * added debug messages to DSSDBG calls * added patch "OMAPDSS: Remove dss_debug variable" Changes from V2 to V3 * added patch "OMAPDSS: Move definition of DEBUG flag to Makefile" Changes from V3 to V4: * added patch "OMAPDSS: Create new debug config options" * broke earlier patch "OMAPDSS: Remove dss_debug variable" into two parts as "OMAPDSS: Replace multi part debug prints with pr_debug" and "OMAPDSS: Remove dss_debug variable" All your comments and suggestions are welcome. Refenence Tree: git://gitorious.org/linux-omap-dss2/chandrabhanus-linux.git dss_cleanup Regards, Chandrabhanu Chandrabhanu Mahapatra (5): OMAPDSS: Move definition of DEBUG flag to Makefile OMAPDSS: Create new debug config options OMAPDSS: Cleanup DSSDBG with dynamic pr_debug function OMAPDSS: Replace multi part debug prints with pr_debug OMAPDSS: Remove dss_debug variable drivers/video/omap2/dss/Kconfig | 21 +++++++++++----- drivers/video/omap2/dss/Makefile | 1 + drivers/video/omap2/dss/apply.c | 8 +++---- drivers/video/omap2/dss/core.c | 11 +++------ drivers/video/omap2/dss/dispc.c | 40 ++++++++++++------------------- drivers/video/omap2/dss/dsi.c | 49 ++++++++++++++++---------------------- drivers/video/omap2/dss/dss.c | 2 +- drivers/video/omap2/dss/dss.h | 40 ++++++------------------------- 8 files changed, 66 insertions(+), 106 deletions(-)
On Sat, 2012-09-29 at 16:19 +0530, Chandrabhanu Mahapatra wrote: > Hi everyone, > this patch series aims at cleaning up of DSS of printk()'s enabled with > dss_debug and replace them with generic dynamic debug printing. Except for the missing debug print conversions in dsi.c this looks good. Do you want me to apply the current series and you can send the dsi.c patch later, or do you want to fix the dsi.c also before I apply? Tomi
Tomi, Chandrabhanu, On Friday 05 October 2012 06:16 PM, Tomi Valkeinen wrote: > On Sat, 2012-09-29 at 16:19 +0530, Chandrabhanu Mahapatra wrote: >> Hi everyone, >> this patch series aims at cleaning up of DSS of printk()'s enabled with >> dss_debug and replace them with generic dynamic debug printing. > > Except for the missing debug print conversions in dsi.c this looks good. > Do you want me to apply the current series and you can send the dsi.c > patch later, or do you want to fix the dsi.c also before I apply? With the change for DSI, please feel free to add Reviewed-by: Sumit Semwal <sumit.semwal@ti.com> BR, ~Sumit. > > Tomi > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" 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/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c index 6354bb8..cb86d94 100644 --- a/drivers/video/omap2/dss/apply.c +++ b/drivers/video/omap2/dss/apply.c @@ -559,7 +559,7 @@ static void dss_ovl_write_regs(struct omap_overlay *ovl) struct mgr_priv_data *mp; int r; - DSSDBGF("%d", ovl->id); + DSSDBG("%d", ovl->id); if (!op->enabled || !op->info_dirty) return; @@ -594,7 +594,7 @@ static void dss_ovl_write_regs_extra(struct omap_overlay *ovl) struct ovl_priv_data *op = get_ovl_priv(ovl); struct mgr_priv_data *mp; - DSSDBGF("%d", ovl->id); + DSSDBG("%d", ovl->id); if (!op->extra_info_dirty) return; @@ -618,7 +618,7 @@ static void dss_mgr_write_regs(struct omap_overlay_manager *mgr) struct mgr_priv_data *mp = get_mgr_priv(mgr); struct omap_overlay *ovl; - DSSDBGF("%d", mgr->id); + DSSDBG("%d", mgr->id); if (!mp->enabled) return; @@ -644,7 +644,7 @@ static void dss_mgr_write_regs_extra(struct omap_overlay_manager *mgr) { struct mgr_priv_data *mp = get_mgr_priv(mgr); - DSSDBGF("%d", mgr->id); + DSSDBG("%d", mgr->id); if (!mp->extra_info_dirty) return; diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c index 8d815e3..8304cc6b 100644 --- a/drivers/video/omap2/dss/dsi.c +++ b/drivers/video/omap2/dss/dsi.c @@ -1525,8 +1525,6 @@ int dsi_pll_set_clock_div(struct platform_device *dsidev, u8 regn_start, regn_end, regm_start, regm_end; u8 regm_dispc_start, regm_dispc_end, regm_dsi_start, regm_dsi_end; - DSSDBGF(); - dsi->current_cinfo.clkin = cinfo->clkin; dsi->current_cinfo.fint = cinfo->fint; dsi->current_cinfo.clkin4ddr = cinfo->clkin4ddr; @@ -2334,8 +2332,6 @@ static int dsi_cio_init(struct omap_dss_device *dssdev) int r; u32 l; - DSSDBGF(); - r = dss_dsi_enable_pads(dsi->module_id, dsi_get_lane_mask(dssdev)); if (r) return r; @@ -2686,7 +2682,7 @@ static void dsi_vc_initial_config(struct platform_device *dsidev, int channel) { u32 r; - DSSDBGF("%d", channel); + DSSDBG("%d", channel); r = dsi_read_reg(dsidev, DSI_VC_CTRL(channel)); @@ -2718,7 +2714,7 @@ static int dsi_vc_config_source(struct platform_device *dsidev, int channel, if (dsi->vc[channel].source == source) return 0; - DSSDBGF("%d", channel); + DSSDBG("%d", channel); dsi_sync_vc(dsidev, channel); @@ -3475,7 +3471,7 @@ static int dsi_enter_ulps(struct platform_device *dsidev) int r, i; unsigned mask; - DSSDBGF(); + DSSDBG(""); WARN_ON(!dsi_bus_is_locked(dsidev)); @@ -4184,7 +4180,7 @@ int omapdss_dsi_set_clocks(struct omap_dss_device *dssdev, unsigned long pck; int r; - DSSDBGF("ddr_clk %lu, lp_clk %lu", ddr_clk, lp_clk); + DSSDBG("ddr_clk %lu, lp_clk %lu", ddr_clk, lp_clk); mutex_lock(&dsi->lock); diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h index 5e9fd769..3a2caab 100644 --- a/drivers/video/omap2/dss/dss.h +++ b/drivers/video/omap2/dss/dss.h @@ -29,38 +29,20 @@ #ifdef DEBUG extern bool dss_debug; -#ifdef DSS_SUBSYS_NAME -#define DSSDBG(format, ...) \ - if (dss_debug) \ - printk(KERN_DEBUG "omapdss " DSS_SUBSYS_NAME ": " format, \ - ## __VA_ARGS__) -#else -#define DSSDBG(format, ...) \ - if (dss_debug) \ - printk(KERN_DEBUG "omapdss: " format, ## __VA_ARGS__) #endif -#ifdef DSS_SUBSYS_NAME -#define DSSDBGF(format, ...) \ - if (dss_debug) \ - printk(KERN_DEBUG "omapdss " DSS_SUBSYS_NAME \ - ": %s(" format ")\n", \ - __func__, \ - ## __VA_ARGS__) -#else -#define DSSDBGF(format, ...) \ - if (dss_debug) \ - printk(KERN_DEBUG "omapdss: " \ - ": %s(" format ")\n", \ - __func__, \ - ## __VA_ARGS__) +#ifdef pr_fmt +#undef pr_fmt #endif -#else /* DEBUG */ -#define DSSDBG(format, ...) -#define DSSDBGF(format, ...) +#ifdef DSS_SUBSYS_NAME +#define pr_fmt(fmt) DSS_SUBSYS_NAME ": " fmt +#else +#define pr_fmt(fmt) fmt #endif +#define DSSDBG(format, ...) \ + pr_debug(format, ## __VA_ARGS__) #ifdef DSS_SUBSYS_NAME #define DSSERR(format, ...) \
The printk in DSSDBG function definition is replaced with dynamic debug enabled pr_debug(). The use of dynamic debugging provides more flexiblity as each debug statement can be enabled or disabled dynamically on basis of source filename, line number, module name etc. by writing to a control file in debugfs filesystem. For better undertsanding please refer to Documentation/dynamic-debug-howto.txt. The DSSDBGF() differs from DSSDBG() by providing function name. However, function name, line number, module name and thread ID can be printed through dynamic debug by setting appropiate flags 'f','l','m' and 't' in the debugfs control file. So, DSSDBGF instances are replaced with DSSDBG. Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com> --- drivers/video/omap2/dss/apply.c | 8 ++++---- drivers/video/omap2/dss/dsi.c | 12 ++++-------- drivers/video/omap2/dss/dss.h | 34 ++++++++-------------------------- 3 files changed, 16 insertions(+), 38 deletions(-)