[v3] media: renesas: vsp1: Add underrun debug print

  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(-)
  

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.