| Message ID | 20230214164223.184920-1-tomi.valkeinen+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | [v3] media: renesas: vsp1: Add underrun debug print | expand |
Hi Tomi, Thank you for the patch. On Tue, Feb 14, 2023 at 06:42:23PM +0200, Tomi Valkeinen wrote: > Print underrun interrupts with ratelimited print. > > Note that we don't enable the underrun interrupt. If we have underruns, > we don't want to get flooded with interrupts about them. It's enough to > see that an underrun happened at the end of a frame. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> > --- > > Changes in v3: > - Reset underrun counter when enabling VSP > > I have to say I'm not familiar enough with the VSP driver to say if > these are the correct places where to reset the counters. It's fine. We could factor it out to a clear function, but it's not worth it if there's nothing else to factor out. It could be done later. > There's also a > possibility of a race, but my assumption is that we cannot get underrun > interrupts for the WPF we are currently enabling. It should be fine. > Also, I realized the underrun counter could be moved to struct > vsp1_rwpf, but as that's used also for RPF, I didn't do that change. Another option would be to store it in the pipeline structure, as a pipeline has one and only one WPF. What do you think ? > drivers/media/platform/renesas/vsp1/vsp1.h | 2 ++ > drivers/media/platform/renesas/vsp1/vsp1_drm.c | 3 +++ > drivers/media/platform/renesas/vsp1/vsp1_drv.c | 11 ++++++++++- > drivers/media/platform/renesas/vsp1/vsp1_regs.h | 2 ++ > drivers/media/platform/renesas/vsp1/vsp1_video.c | 3 +++ > 5 files changed, 20 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/platform/renesas/vsp1/vsp1.h b/drivers/media/platform/renesas/vsp1/vsp1.h > index 2f6f0c6ae555..9eb023f4fee8 100644 > --- a/drivers/media/platform/renesas/vsp1/vsp1.h > +++ b/drivers/media/platform/renesas/vsp1/vsp1.h > @@ -107,6 +107,8 @@ struct vsp1_device { > struct media_entity_operations media_ops; > > struct vsp1_drm *drm; > + > + u32 underrun_count[VSP1_MAX_WPF]; > }; > > int vsp1_device_get(struct vsp1_device *vsp1); > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_drm.c b/drivers/media/platform/renesas/vsp1/vsp1_drm.c > index c6f25200982c..e3b4e993787c 100644 > --- a/drivers/media/platform/renesas/vsp1/vsp1_drm.c > +++ b/drivers/media/platform/renesas/vsp1/vsp1_drm.c > @@ -710,6 +710,9 @@ int vsp1_du_setup_lif(struct device *dev, unsigned int pipe_index, > return 0; > } > > + /* Reset the underrun counter */ > + vsp1->underrun_count[pipe->output->entity.index] = 0; > + > drm_pipe->width = cfg->width; > drm_pipe->height = cfg->height; > pipe->interlaced = cfg->interlaced; > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_drv.c b/drivers/media/platform/renesas/vsp1/vsp1_drv.c > index 5710152d6511..f9be0fda1659 100644 > --- a/drivers/media/platform/renesas/vsp1/vsp1_drv.c > +++ b/drivers/media/platform/renesas/vsp1/vsp1_drv.c > @@ -45,7 +45,8 @@ > > static irqreturn_t vsp1_irq_handler(int irq, void *data) > { > - u32 mask = VI6_WPF_IRQ_STA_DFE | VI6_WPF_IRQ_STA_FRE; > + u32 mask = VI6_WPF_IRQ_STA_DFE | VI6_WPF_IRQ_STA_FRE | > + VI6_WPF_IRQ_STA_UND; > struct vsp1_device *vsp1 = data; > irqreturn_t ret = IRQ_NONE; > unsigned int i; > @@ -60,6 +61,14 @@ static irqreturn_t vsp1_irq_handler(int irq, void *data) > status = vsp1_read(vsp1, VI6_WPF_IRQ_STA(i)); > vsp1_write(vsp1, VI6_WPF_IRQ_STA(i), ~status & mask); > > + if (status & VI6_WPF_IRQ_STA_UND) { > + vsp1->underrun_count[i]++; > + > + dev_warn_ratelimited(vsp1->dev, > + "Underrun occurred at WPF%u (total underruns %u)\n", > + i, vsp1->underrun_count[i]); > + } > + > if (status & VI6_WPF_IRQ_STA_DFE) { > vsp1_pipeline_frame_end(wpf->entity.pipe); > ret = IRQ_HANDLED; > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_regs.h b/drivers/media/platform/renesas/vsp1/vsp1_regs.h > index d94343ae57a1..7eca82e0ba7e 100644 > --- a/drivers/media/platform/renesas/vsp1/vsp1_regs.h > +++ b/drivers/media/platform/renesas/vsp1/vsp1_regs.h > @@ -32,10 +32,12 @@ > #define VI6_STATUS_SYS_ACT(n) BIT((n) + 8) > > #define VI6_WPF_IRQ_ENB(n) (0x0048 + (n) * 12) > +#define VI6_WPF_IRQ_ENB_UNDE BIT(16) > #define VI6_WPF_IRQ_ENB_DFEE BIT(1) > #define VI6_WPF_IRQ_ENB_FREE BIT(0) > > #define VI6_WPF_IRQ_STA(n) (0x004c + (n) * 12) > +#define VI6_WPF_IRQ_STA_UND BIT(16) > #define VI6_WPF_IRQ_STA_DFE BIT(1) > #define VI6_WPF_IRQ_STA_FRE BIT(0) > > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_video.c b/drivers/media/platform/renesas/vsp1/vsp1_video.c > index 544012fd1fe9..6eca2b9c8dee 100644 > --- a/drivers/media/platform/renesas/vsp1/vsp1_video.c > +++ b/drivers/media/platform/renesas/vsp1/vsp1_video.c > @@ -1062,6 +1062,9 @@ vsp1_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type) > if (ret < 0) > goto err_stop; > > + /* Reset the underrun counter */ > + video->vsp1->underrun_count[pipe->output->entity.index] = 0; > + > /* Start the queue. */ > ret = vb2_streamon(&video->queue, type); > if (ret < 0)
On 15/02/2023 00:15, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Tue, Feb 14, 2023 at 06:42:23PM +0200, Tomi Valkeinen wrote: >> Print underrun interrupts with ratelimited print. >> >> Note that we don't enable the underrun interrupt. If we have underruns, >> we don't want to get flooded with interrupts about them. It's enough to >> see that an underrun happened at the end of a frame. >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> >> --- >> >> Changes in v3: >> - Reset underrun counter when enabling VSP >> >> I have to say I'm not familiar enough with the VSP driver to say if >> these are the correct places where to reset the counters. > > It's fine. We could factor it out to a clear function, but it's not > worth it if there's nothing else to factor out. It could be done later. > >> There's also a >> possibility of a race, but my assumption is that we cannot get underrun >> interrupts for the WPF we are currently enabling. > > It should be fine. > >> Also, I realized the underrun counter could be moved to struct >> vsp1_rwpf, but as that's used also for RPF, I didn't do that change. > > Another option would be to store it in the pipeline structure, as a > pipeline has one and only one WPF. What do you think ? Hmm, the pipe is allocated and assigned as needed, isn't it? So in the irq handler we might get an underflow with !pipe. We could skip the print in that case, of course. Is a pipe allocated every time VSP is started? Or does the allocation normally happen only once? If the former, then if the counter was stored in the pipe, that would handle clearing the counter automatically. Tomi
Hi Tomi, On Wed, Feb 15, 2023 at 08:52:02AM +0200, Tomi Valkeinen wrote: > On 15/02/2023 00:15, Laurent Pinchart wrote: > > On Tue, Feb 14, 2023 at 06:42:23PM +0200, Tomi Valkeinen wrote: > >> Print underrun interrupts with ratelimited print. > >> > >> Note that we don't enable the underrun interrupt. If we have underruns, > >> we don't want to get flooded with interrupts about them. It's enough to > >> see that an underrun happened at the end of a frame. > >> > >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> > >> --- > >> > >> Changes in v3: > >> - Reset underrun counter when enabling VSP > >> > >> I have to say I'm not familiar enough with the VSP driver to say if > >> these are the correct places where to reset the counters. > > > > It's fine. We could factor it out to a clear function, but it's not > > worth it if there's nothing else to factor out. It could be done later. > > > >> There's also a > >> possibility of a race, but my assumption is that we cannot get underrun > >> interrupts for the WPF we are currently enabling. > > > > It should be fine. > > > >> Also, I realized the underrun counter could be moved to struct > >> vsp1_rwpf, but as that's used also for RPF, I didn't do that change. > > > > Another option would be to store it in the pipeline structure, as a > > pipeline has one and only one WPF. What do you think ? > > Hmm, the pipe is allocated and assigned as needed, isn't it? So in the > irq handler we might get an underflow with !pipe. We could skip the > print in that case, of course. For display pipelines, the pipeline is embedded in the vsp1_drm_pipeline structure, itself embedded in the vsp1_drm structure, so it won't come and go. The WPF's pipe pointer (in the vsp1_entity structure) is set in vsp1_drm_init() and never reset. For memory-to-memory pipelines, yes, the pipeline is allocated dynamically. Interrupts are not supposed to occur when the pipeline is stopped, but of course there's real life and race conditions. Underrun notifications are likely useless in that case so we could indeed skip them. > Is a pipe allocated every time VSP is started? Or does the allocation > normally happen only once? If the former, then if the counter was stored > in the pipe, that would handle clearing the counter automatically. For memory-to-memory pipelines, the allocation happens when the user calls VIDIOC_STREAMON the first time on any video node in the pipeline (a pipeline has one or more RPFs and exactly one WPF), and the pipeline is freed when the last video node is stopped with VIDIOC_STREAMOFF. It is thus possible to stop and restart the pipeline without the vsp1_pipeline structure being freed, but in that case, I'd say we don't have to actually reset the counter. If the user keeps some video nodes streaming, I think it's acceptable to keep the underrun counter going. For display pipelines, you'll have to reset the counter manually in vsp1_du_setup_lif(). It could be done at stop time instead of start time if desired.
diff --git a/drivers/media/platform/renesas/vsp1/vsp1.h b/drivers/media/platform/renesas/vsp1/vsp1.h index 2f6f0c6ae555..9eb023f4fee8 100644 --- a/drivers/media/platform/renesas/vsp1/vsp1.h +++ b/drivers/media/platform/renesas/vsp1/vsp1.h @@ -107,6 +107,8 @@ struct vsp1_device { struct media_entity_operations media_ops; struct vsp1_drm *drm; + + u32 underrun_count[VSP1_MAX_WPF]; }; int vsp1_device_get(struct vsp1_device *vsp1); diff --git a/drivers/media/platform/renesas/vsp1/vsp1_drm.c b/drivers/media/platform/renesas/vsp1/vsp1_drm.c index c6f25200982c..e3b4e993787c 100644 --- a/drivers/media/platform/renesas/vsp1/vsp1_drm.c +++ b/drivers/media/platform/renesas/vsp1/vsp1_drm.c @@ -710,6 +710,9 @@ int vsp1_du_setup_lif(struct device *dev, unsigned int pipe_index, return 0; } + /* Reset the underrun counter */ + vsp1->underrun_count[pipe->output->entity.index] = 0; + drm_pipe->width = cfg->width; drm_pipe->height = cfg->height; pipe->interlaced = cfg->interlaced; diff --git a/drivers/media/platform/renesas/vsp1/vsp1_drv.c b/drivers/media/platform/renesas/vsp1/vsp1_drv.c index 5710152d6511..f9be0fda1659 100644 --- a/drivers/media/platform/renesas/vsp1/vsp1_drv.c +++ b/drivers/media/platform/renesas/vsp1/vsp1_drv.c @@ -45,7 +45,8 @@ static irqreturn_t vsp1_irq_handler(int irq, void *data) { - u32 mask = VI6_WPF_IRQ_STA_DFE | VI6_WPF_IRQ_STA_FRE; + u32 mask = VI6_WPF_IRQ_STA_DFE | VI6_WPF_IRQ_STA_FRE | + VI6_WPF_IRQ_STA_UND; struct vsp1_device *vsp1 = data; irqreturn_t ret = IRQ_NONE; unsigned int i; @@ -60,6 +61,14 @@ static irqreturn_t vsp1_irq_handler(int irq, void *data) status = vsp1_read(vsp1, VI6_WPF_IRQ_STA(i)); vsp1_write(vsp1, VI6_WPF_IRQ_STA(i), ~status & mask); + if (status & VI6_WPF_IRQ_STA_UND) { + vsp1->underrun_count[i]++; + + dev_warn_ratelimited(vsp1->dev, + "Underrun occurred at WPF%u (total underruns %u)\n", + i, vsp1->underrun_count[i]); + } + if (status & VI6_WPF_IRQ_STA_DFE) { vsp1_pipeline_frame_end(wpf->entity.pipe); ret = IRQ_HANDLED; diff --git a/drivers/media/platform/renesas/vsp1/vsp1_regs.h b/drivers/media/platform/renesas/vsp1/vsp1_regs.h index d94343ae57a1..7eca82e0ba7e 100644 --- a/drivers/media/platform/renesas/vsp1/vsp1_regs.h +++ b/drivers/media/platform/renesas/vsp1/vsp1_regs.h @@ -32,10 +32,12 @@ #define VI6_STATUS_SYS_ACT(n) BIT((n) + 8) #define VI6_WPF_IRQ_ENB(n) (0x0048 + (n) * 12) +#define VI6_WPF_IRQ_ENB_UNDE BIT(16) #define VI6_WPF_IRQ_ENB_DFEE BIT(1) #define VI6_WPF_IRQ_ENB_FREE BIT(0) #define VI6_WPF_IRQ_STA(n) (0x004c + (n) * 12) +#define VI6_WPF_IRQ_STA_UND BIT(16) #define VI6_WPF_IRQ_STA_DFE BIT(1) #define VI6_WPF_IRQ_STA_FRE BIT(0) diff --git a/drivers/media/platform/renesas/vsp1/vsp1_video.c b/drivers/media/platform/renesas/vsp1/vsp1_video.c index 544012fd1fe9..6eca2b9c8dee 100644 --- a/drivers/media/platform/renesas/vsp1/vsp1_video.c +++ b/drivers/media/platform/renesas/vsp1/vsp1_video.c @@ -1062,6 +1062,9 @@ vsp1_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type) if (ret < 0) goto err_stop; + /* Reset the underrun counter */ + video->vsp1->underrun_count[pipe->output->entity.index] = 0; + /* Start the queue. */ ret = vb2_streamon(&video->queue, type); if (ret < 0)
Print underrun interrupts with ratelimited print. Note that we don't enable the underrun interrupt. If we have underruns, we don't want to get flooded with interrupts about them. It's enough to see that an underrun happened at the end of a frame. Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> --- Changes in v3: - Reset underrun counter when enabling VSP I have to say I'm not familiar enough with the VSP driver to say if these are the correct places where to reset the counters. There's also a possibility of a race, but my assumption is that we cannot get underrun interrupts for the WPF we are currently enabling. Also, I realized the underrun counter could be moved to struct vsp1_rwpf, but as that's used also for RPF, I didn't do that change. drivers/media/platform/renesas/vsp1/vsp1.h | 2 ++ drivers/media/platform/renesas/vsp1/vsp1_drm.c | 3 +++ drivers/media/platform/renesas/vsp1/vsp1_drv.c | 11 ++++++++++- drivers/media/platform/renesas/vsp1/vsp1_regs.h | 2 ++ drivers/media/platform/renesas/vsp1/vsp1_video.c | 3 +++ 5 files changed, 20 insertions(+), 1 deletion(-)