| Message ID | 20181025124546.22145-1-boris.brezillon@bootlin.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm/vc4: Add a load tracker | expand |
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@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. - 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. What do you think? Cheers, Paul > Regards, > > Boris > > Boris Brezillon (3): > drm/atomic: Add a generic infrastructure to track underrun errors > drm/vc4: Report underrun errors > drm/vc4: Add a load tracker to prevent HVS underflow errors > > drivers/gpu/drm/drm_atomic.c | 12 +++ > drivers/gpu/drm/drm_atomic_helper.c | 4 + > drivers/gpu/drm/vc4/vc4_drv.c | 1 + > drivers/gpu/drm/vc4/vc4_drv.h | 14 ++++ > drivers/gpu/drm/vc4/vc4_hvs.c | 71 ++++++++++++++++++ > drivers/gpu/drm/vc4/vc4_kms.c | 110 +++++++++++++++++++++++++++- > drivers/gpu/drm/vc4/vc4_plane.c | 60 +++++++++++++++ > drivers/gpu/drm/vc4/vc4_regs.h | 46 +++--------- > include/drm/drm_atomic_helper.h | 53 ++++++++++++++ > include/drm/drm_device.h | 15 ++++ > include/drm/drm_mode_config.h | 26 +++++++ > 11 files changed, 376 insertions(+), 36 deletions(-) >
On Wed, 28 Nov 2018 10:16:17 +0100 Paul Kocialkowski <paul.kocialkowski@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@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.
Hi, On Wed, 2018-11-28 at 10:29 +0100, Boris Brezillon wrote: > On Wed, 28 Nov 2018 10:16:17 +0100 > Paul Kocialkowski <paul.kocialkowski@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@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. Right, that works just fine on VC4 and I guess it's fair to expect that future hardware that uses an underrun indication will have received the underrun indication by the time vblank happens.