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@xxxxxxxxxxxxxxxx> > >> --- > >> > >> 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. -- Regards, Laurent Pinchart
