[PATCH v2 0/3] drm/vc4: Add a load tracker

[Boris Brezillon]

On Wed, 28 Nov 2018 10:16:17 +0100
Paul Kocialkowski <paul.kocialkowski at bootlin.com> wrote:

> Hi,
> 
> On Thu, 2018-10-25 at 14:45 +0200, Boris Brezillon wrote:
> > Hello,
> > 
> > This is the 2nd version of the VC4 load tracker patch.
> > 
> > Daniel, as you suggested, I've implemented a generic infrastructure to
> > track and report underrun errors (patch 1). Not sure this is what you
> > had in mind, but it seems to do the job for my use case, and should
> > allow me to easily track regressions in the load tracking logic with a
> > bunch of IGT tests. Let me know if you want it done differently.
> > 
> > Patch 2 is implementing the generic underrun interface in the VC4
> > driver, and patch 3 is just adding the load tracking logic (hasn't
> > changed since the RFC except for the unused 'ret' var removal).  
> 
> For the whole series:
> Tested-by: Paul Kocialkowski <paul.kocialkowski at bootlin.com>
> 
> I am currently integrating this with IGT testing and have a few general
> remarks:
> 
> - I think it would make sense to have a driver-specific debugfs entry
> for enabling/disabling the rejection of commits by the load tracker.
> This would be useful for testing that there is no mismatch between the
> load tracker's behavior and hardware-detected underruns.

Yep, makes sense.

> 
> - Underrun reporting with a generic debugfs entry is a good fit for IGT
> (and userspace reporting in general), but it would be useful to have an
> intermediary state reported between applying a commit and getting the
> underrun status.
> 
> Something like returning '?' between setting a commit and the next
> vblank. This way, there is no chance that userspace reads the underrun
> status related to the previous configuration.

You will never get the result of the previous atomic-set since I reset
the underrun state to 0 before committing the changes, but you might
read the underrun file before the underrun event happened. So yes,
waiting for at least one VBLANK sounds reasonable. Not sure we want to
automate that in the driver though, as this would imply activating
vblank interrupts to update the underrun state even if the user doesn't
care. Maybe you can use the DRM_IOCTL_WAIT_VBLANK ioctl instead.