Hi Laurent On Wed, Nov 22, 2023 at 06:30:01AM GMT, Laurent Pinchart wrote: > It is useful for debugging purpose to dump a vsp1_pipeline to the kernel > log. Add a new function to do so, and use it when initializing the video > and DRM pipelines. > > As __vsp1_pipeline_dump() needs to construct the log message > iteratively, it uses pr_cont(...) (exact equivalent to the more verbose > "printk(KERN_CONT ..."). The function thus can't use dev_dbg() to log > the initial part of the message, for two reasons: > > - pr_cont() doesn't seem to work with dev_*(). Even if the format string > passed to dev_*() doesn't end with a '\n', pr_cont() starts a new line > in the log. This behaviour doesn't seem to be clearly documented, and > may or may not be on purpose. > > - Messages printed by dev_dbg() may be omitted if dynamic debugging is > enabled. In that case, the continuation messages will still be > printed, leading to confusing log messages. > > To still benefit from the dynamic debug infrastructure, we declare a > vsp1_pipeline_dump() macro that uses _dynamic_func_call() when dynamic > debugging is enabled. The whole vsp1_pipeline_dump() call can be > selected at runtime. The __vsp1_pipeline_dump() function then uses a > plain "printk(KERN_DEBUG ...)" to print the message header using the > debug log level, and pr_cont() to print the rest of the message on the > same line. > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx> > --- > .../media/platform/renesas/vsp1/vsp1_drm.c | 5 +++++ > .../media/platform/renesas/vsp1/vsp1_pipe.c | 22 +++++++++++++++++++ > .../media/platform/renesas/vsp1/vsp1_pipe.h | 19 ++++++++++++++++ > .../media/platform/renesas/vsp1/vsp1_video.c | 10 ++++++++- > 4 files changed, 55 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_drm.c b/drivers/media/platform/renesas/vsp1/vsp1_drm.c > index 3954c138fa7b..1aa59a74672f 100644 > --- a/drivers/media/platform/renesas/vsp1/vsp1_drm.c > +++ b/drivers/media/platform/renesas/vsp1/vsp1_drm.c > @@ -733,6 +733,8 @@ int vsp1_du_setup_lif(struct device *dev, unsigned int pipe_index, > if (ret < 0) > goto unlock; > > + vsp1_pipeline_dump(pipe, "LIF setup"); > + > /* Enable the VSP1. */ > ret = vsp1_device_get(vsp1); > if (ret < 0) > @@ -906,6 +908,9 @@ void vsp1_du_atomic_flush(struct device *dev, unsigned int pipe_index, > } > > vsp1_du_pipeline_setup_inputs(vsp1, pipe); > + > + vsp1_pipeline_dump(pipe, "atomic update"); > + > vsp1_du_pipeline_configure(pipe); > > done: > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_pipe.c b/drivers/media/platform/renesas/vsp1/vsp1_pipe.c > index 8eba3cda1e3d..edc5e9f3ba65 100644 > --- a/drivers/media/platform/renesas/vsp1/vsp1_pipe.c > +++ b/drivers/media/platform/renesas/vsp1/vsp1_pipe.c > @@ -301,6 +301,28 @@ void vsp1_pipeline_init(struct vsp1_pipeline *pipe) > pipe->state = VSP1_PIPELINE_STOPPED; > } > > +void __vsp1_pipeline_dump(struct _ddebug *, struct vsp1_pipeline *pipe, > + const char *msg) > +{ > + struct vsp1_device *vsp1 = pipe->output->entity.vsp1; > + struct vsp1_entity *entity; > + bool first = true; > + > + printk(KERN_DEBUG "%s: %s: pipe: ", dev_name(vsp1->dev), msg); > + > + list_for_each_entry(entity, &pipe->entities, list_pipe) { > + const char *name; > + > + name = strchrnul(entity->subdev.name, ' '); > + name = name ? name + 1 : entity->subdev.name; > + > + pr_cont("%s%s", first ? "" : ", ", name); > + first = false; > + } > + > + pr_cont("\n"); > +} > + > /* Must be called with the pipe irqlock held. */ > void vsp1_pipeline_run(struct vsp1_pipeline *pipe) > { > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_pipe.h b/drivers/media/platform/renesas/vsp1/vsp1_pipe.h > index c1f411227de7..46a82a9f766a 100644 > --- a/drivers/media/platform/renesas/vsp1/vsp1_pipe.h > +++ b/drivers/media/platform/renesas/vsp1/vsp1_pipe.h > @@ -9,6 +9,7 @@ > #ifndef __VSP1_PIPE_H__ > #define __VSP1_PIPE_H__ > > +#include <linux/dynamic_debug.h> > #include <linux/kref.h> > #include <linux/list.h> > #include <linux/spinlock.h> > @@ -142,6 +143,24 @@ struct vsp1_pipeline { > void vsp1_pipeline_reset(struct vsp1_pipeline *pipe); > void vsp1_pipeline_init(struct vsp1_pipeline *pipe); > > +void __vsp1_pipeline_dump(struct _ddebug *, struct vsp1_pipeline *pipe, > + const char *msg); > + > +#if defined(CONFIG_DYNAMIC_DEBUG) || \ > + (defined(CONFIG_DYNAMIC_DEBUG_CORE) && defined(DYNAMIC_DEBUG_MODULE)) > +#define vsp1_pipeline_dump(pipe, msg) \ > + _dynamic_func_call("vsp1_pipeline_dump()", __vsp1_pipeline_dump, pipe, msg) > +#elif defined(DEBUG) > +#define vsp1_pipeline_dump(pipe, msg) \ > + __vsp1_pipeline_dump(NULL, pipe, msg) > +#else > +#define vsp1_pipeline_dump(pipe, msg) \ > +({ \ > + if (0) \ > + __vsp1_pipeline_dump(NULL, pipe, msg); \ > +)} Why can't this simply be #else #define vsp1_pipeline_dump(pipe, msg) #endif ? > +#endif > + > void vsp1_pipeline_run(struct vsp1_pipeline *pipe); > bool vsp1_pipeline_stopped(struct vsp1_pipeline *pipe); > int vsp1_pipeline_stop(struct vsp1_pipeline *pipe); > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_video.c b/drivers/media/platform/renesas/vsp1/vsp1_video.c > index 6a8db541543a..84394994ccee 100644 > --- a/drivers/media/platform/renesas/vsp1/vsp1_video.c > +++ b/drivers/media/platform/renesas/vsp1/vsp1_video.c > @@ -520,11 +520,19 @@ static int vsp1_video_pipeline_build(struct vsp1_pipeline *pipe, > static int vsp1_video_pipeline_init(struct vsp1_pipeline *pipe, > struct vsp1_video *video) > { > + int ret; > + > vsp1_pipeline_init(pipe); > > pipe->frame_end = vsp1_video_pipeline_frame_end; > > - return vsp1_video_pipeline_build(pipe, video); > + ret = vsp1_video_pipeline_build(pipe, video); > + if (ret) > + return ret; > + > + vsp1_pipeline_dump(pipe, "video"); > + > + return 0; > } > > static struct vsp1_pipeline *vsp1_video_pipeline_get(struct vsp1_video *video) > -- > Regards, > > Laurent Pinchart > >