| Message ID | 20181130161104.16352-1-paul.kocialkowski@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/vc4: Add a debugfs entry to disable/enable the load tracker | expand |
On Fri, 30 Nov 2018 17:11:04 +0100 Paul Kocialkowski <paul.kocialkowski@bootlin.com> wrote: > In order to test whether the load tracker is working as expected, we > need the ability to compare the commit result with the underrun > indication. With the load tracker always enabled, commits that are > expected to trigger an underrun are always rejected, so userspace > cannot get the actual underrun indication from the hardware. > > Add a debugfs entry to disable/enable the load tracker, so that a DRM > commit expected to trigger an underrun can go through with the load > tracker disabled. The underrun indication is then available to > userspace and can be checked against the commit result with the load > tracker enabled. > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > --- > > Changes since v1: > * Moved all the debugfs-related functions and structure to vc4_debugfs.c. > > drivers/gpu/drm/vc4/vc4_debugfs.c | 58 +++++++++++++++++++++++++++++++ > drivers/gpu/drm/vc4/vc4_drv.c | 2 ++ > drivers/gpu/drm/vc4/vc4_drv.h | 2 ++ > drivers/gpu/drm/vc4/vc4_kms.c | 6 +++- > 4 files changed, 67 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/vc4/vc4_debugfs.c b/drivers/gpu/drm/vc4/vc4_debugfs.c > index 7a0003de71ab..8f4d7fadb226 100644 > --- a/drivers/gpu/drm/vc4/vc4_debugfs.c > +++ b/drivers/gpu/drm/vc4/vc4_debugfs.c > @@ -32,9 +32,67 @@ static const struct drm_info_list vc4_debugfs_list[] = { > > #define VC4_DEBUGFS_ENTRIES ARRAY_SIZE(vc4_debugfs_list) > > +static int vc4_debugfs_load_tracker_get(struct seq_file *m, void *data) > +{ > + struct drm_device *dev = m->private; > + struct vc4_dev *vc4 = to_vc4_dev(dev); > + > + if (vc4->load_tracker_enabled) > + seq_printf(m, "enabled\n"); > + else > + seq_printf(m, "disabled\n"); > + > + return 0; > +} > + > +static ssize_t vc4_debugfs_load_tracker_set(struct file *file, > + const char __user *ubuf, > + size_t len, loff_t *offp) > +{ > + struct seq_file *m = file->private_data; > + struct drm_device *dev = m->private; > + struct vc4_dev *vc4 = to_vc4_dev(dev); > + char buf[32] = {}; > + > + if (len >= sizeof(buf)) > + return -EINVAL; > + > + if (copy_from_user(buf, ubuf, len)) > + return -EFAULT; > + > + if (!strncasecmp(buf, "enable", 6)) > + vc4->load_tracker_enabled = true; > + else if (!strncasecmp(buf, "disable", 7)) > + vc4->load_tracker_enabled = false; > + else > + return -EINVAL; > + > + return len; > +} > + > +static int vc4_debugfs_load_tracker_open(struct inode *inode, struct file *file) > +{ > + return single_open(file, vc4_debugfs_load_tracker_get, inode->i_private); > +} > + > +static const struct file_operations vc4_debugfs_load_tracker_fops = { > + .owner = THIS_MODULE, > + .open = vc4_debugfs_load_tracker_open, > + .read = seq_read, > + .write = vc4_debugfs_load_tracker_set, > +}; > + > int > vc4_debugfs_init(struct drm_minor *minor) > { > + struct dentry *dentry; > + > + dentry = debugfs_create_file("load_tracker", S_IRUGO | S_IWUSR, > + minor->debugfs_root, minor->dev, > + &vc4_debugfs_load_tracker_fops); > + if (!dentry) > + return -ENOMEM; Hm, maybe you could use debugfs_create_bool() instead of re-implementing it. The values won't be enabled/disabled though (IIRC, it's Y or N). > + > return drm_debugfs_create_files(vc4_debugfs_list, VC4_DEBUGFS_ENTRIES, > minor->debugfs_root, minor); > } > diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c > index 7195a0bcceb3..12ac971d24d6 100644 > --- a/drivers/gpu/drm/vc4/vc4_drv.c > +++ b/drivers/gpu/drm/vc4/vc4_drv.c > @@ -290,6 +290,8 @@ static int vc4_drm_bind(struct device *dev) > > drm_fbdev_generic_setup(drm, 32); > > + vc4->load_tracker_enabled = true; > + > return 0; > > unbind_all: > diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h > index 11369da944b6..8d0f2f16a9e8 100644 > --- a/drivers/gpu/drm/vc4/vc4_drv.h > +++ b/drivers/gpu/drm/vc4/vc4_drv.h > @@ -188,6 +188,8 @@ struct vc4_dev { > > int power_refcount; > > + bool load_tracker_enabled; > + > /* Mutex controlling the power refcount. */ > struct mutex power_lock; > > diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c > index b905a19c1470..4b2509b1b8ed 100644 > --- a/drivers/gpu/drm/vc4/vc4_kms.c > +++ b/drivers/gpu/drm/vc4/vc4_kms.c > @@ -473,6 +473,7 @@ static const struct drm_private_state_funcs vc4_load_tracker_state_funcs = { > static int > vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state) > { > + struct vc4_dev *vc4 = to_vc4_dev(dev); > int ret; > > ret = vc4_ctm_atomic_check(dev, state); > @@ -483,7 +484,10 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state) > if (ret) > return ret; > > - return vc4_load_tracker_atomic_check(state); > + if (vc4->load_tracker_enabled) > + return vc4_load_tracker_atomic_check(state); > + > + return 0; > } > > static const struct drm_mode_config_funcs vc4_mode_funcs = {
Paul Kocialkowski <paul.kocialkowski@bootlin.com> writes: > In order to test whether the load tracker is working as expected, we > need the ability to compare the commit result with the underrun > indication. With the load tracker always enabled, commits that are > expected to trigger an underrun are always rejected, so userspace > cannot get the actual underrun indication from the hardware. > > Add a debugfs entry to disable/enable the load tracker, so that a DRM > commit expected to trigger an underrun can go through with the load > tracker disabled. The underrun indication is then available to > userspace and can be checked against the commit result with the load > tracker enabled. > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> Given that the load tracker is going to be conservative and say things will underrun even when they might not in practice, will this actually be useful for automated testing? Or is the intent to make it easier to tune the load tracker by disabling it so that you can experiment freely?
Hi Eric, Le vendredi 30 novembre 2018 à 12:30 -0800, Eric Anholt a écrit : > Paul Kocialkowski <paul.kocialkowski@bootlin.com> writes: > > > In order to test whether the load tracker is working as expected, we > > need the ability to compare the commit result with the underrun > > indication. With the load tracker always enabled, commits that are > > expected to trigger an underrun are always rejected, so userspace > > cannot get the actual underrun indication from the hardware. > > > > Add a debugfs entry to disable/enable the load tracker, so that a DRM > > commit expected to trigger an underrun can go through with the load > > tracker disabled. The underrun indication is then available to > > userspace and can be checked against the commit result with the load > > tracker enabled. > > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > > Given that the load tracker is going to be conservative and say things > will underrun even when they might not in practice, will this actually > be useful for automated testing? Or is the intent to make it easier to > tune the load tracker by disabling it so that you can experiment freely? The intent here is to be able to compare the load tracker's result with the underrun indication from the HVS. The idea is to have the following testing scheme: 1. enable the load tracker 2. try a commit and get the errno if it fails 3. disable the load tracker 4. do the commit 5. wait for vblank and get the underrun indication 6. check that the underrun occurs when errno == -ENOSPC This way, we can find out when the load tracker gave us a false positive and refine the load calculation. Cheers, Paul
Hi Boris, Le vendredi 30 novembre 2018 à 19:57 +0100, Boris Brezillon a écrit : > On Fri, 30 Nov 2018 17:11:04 +0100 > Paul Kocialkowski <paul.kocialkowski@bootlin.com> wrote: > > > In order to test whether the load tracker is working as expected, we > > need the ability to compare the commit result with the underrun > > indication. With the load tracker always enabled, commits that are > > expected to trigger an underrun are always rejected, so userspace > > cannot get the actual underrun indication from the hardware. > > > > Add a debugfs entry to disable/enable the load tracker, so that a DRM > > commit expected to trigger an underrun can go through with the load > > tracker disabled. The underrun indication is then available to > > userspace and can be checked against the commit result with the load > > tracker enabled. > > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > > --- > > > > Changes since v1: > > * Moved all the debugfs-related functions and structure to vc4_debugfs.c. > > > > drivers/gpu/drm/vc4/vc4_debugfs.c | 58 +++++++++++++++++++++++++++++++ > > drivers/gpu/drm/vc4/vc4_drv.c | 2 ++ > > drivers/gpu/drm/vc4/vc4_drv.h | 2 ++ > > drivers/gpu/drm/vc4/vc4_kms.c | 6 +++- > > 4 files changed, 67 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/vc4/vc4_debugfs.c b/drivers/gpu/drm/vc4/vc4_debugfs.c > > index 7a0003de71ab..8f4d7fadb226 100644 > > --- a/drivers/gpu/drm/vc4/vc4_debugfs.c > > +++ b/drivers/gpu/drm/vc4/vc4_debugfs.c > > @@ -32,9 +32,67 @@ static const struct drm_info_list vc4_debugfs_list[] = { > > > > #define VC4_DEBUGFS_ENTRIES ARRAY_SIZE(vc4_debugfs_list) > > > > +static int vc4_debugfs_load_tracker_get(struct seq_file *m, void *data) > > +{ > > + struct drm_device *dev = m->private; > > + struct vc4_dev *vc4 = to_vc4_dev(dev); > > + > > + if (vc4->load_tracker_enabled) > > + seq_printf(m, "enabled\n"); > > + else > > + seq_printf(m, "disabled\n"); > > + > > + return 0; > > +} > > + > > +static ssize_t vc4_debugfs_load_tracker_set(struct file *file, > > + const char __user *ubuf, > > + size_t len, loff_t *offp) > > +{ > > + struct seq_file *m = file->private_data; > > + struct drm_device *dev = m->private; > > + struct vc4_dev *vc4 = to_vc4_dev(dev); > > + char buf[32] = {}; > > + > > + if (len >= sizeof(buf)) > > + return -EINVAL; > > + > > + if (copy_from_user(buf, ubuf, len)) > > + return -EFAULT; > > + > > + if (!strncasecmp(buf, "enable", 6)) > > + vc4->load_tracker_enabled = true; > > + else if (!strncasecmp(buf, "disable", 7)) > > + vc4->load_tracker_enabled = false; > > + else > > + return -EINVAL; > > + > > + return len; > > +} > > + > > +static int vc4_debugfs_load_tracker_open(struct inode *inode, struct file *file) > > +{ > > + return single_open(file, vc4_debugfs_load_tracker_get, inode->i_private); > > +} > > + > > +static const struct file_operations vc4_debugfs_load_tracker_fops = { > > + .owner = THIS_MODULE, > > + .open = vc4_debugfs_load_tracker_open, > > + .read = seq_read, > > + .write = vc4_debugfs_load_tracker_set, > > +}; > > + > > int > > vc4_debugfs_init(struct drm_minor *minor) > > { > > + struct dentry *dentry; > > + > > + dentry = debugfs_create_file("load_tracker", S_IRUGO | S_IWUSR, > > + minor->debugfs_root, minor->dev, > > + &vc4_debugfs_load_tracker_fops); > > + if (!dentry) > > + return -ENOMEM; > > Hm, maybe you could use debugfs_create_bool() instead of > re-implementing it. The values won't be enabled/disabled though > (IIRC, it's Y or N). Oh, I didn't know about that! It definitely makes more sense yes. Thanks, Paul > > + > > return drm_debugfs_create_files(vc4_debugfs_list, VC4_DEBUGFS_ENTRIES, > > minor->debugfs_root, minor); > > } > > diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c > > index 7195a0bcceb3..12ac971d24d6 100644 > > --- a/drivers/gpu/drm/vc4/vc4_drv.c > > +++ b/drivers/gpu/drm/vc4/vc4_drv.c > > @@ -290,6 +290,8 @@ static int vc4_drm_bind(struct device *dev) > > > > drm_fbdev_generic_setup(drm, 32); > > > > + vc4->load_tracker_enabled = true; > > + > > return 0; > > > > unbind_all: > > diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h > > index 11369da944b6..8d0f2f16a9e8 100644 > > --- a/drivers/gpu/drm/vc4/vc4_drv.h > > +++ b/drivers/gpu/drm/vc4/vc4_drv.h > > @@ -188,6 +188,8 @@ struct vc4_dev { > > > > int power_refcount; > > > > + bool load_tracker_enabled; > > + > > /* Mutex controlling the power refcount. */ > > struct mutex power_lock; > > > > diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c > > index b905a19c1470..4b2509b1b8ed 100644 > > --- a/drivers/gpu/drm/vc4/vc4_kms.c > > +++ b/drivers/gpu/drm/vc4/vc4_kms.c > > @@ -473,6 +473,7 @@ static const struct drm_private_state_funcs vc4_load_tracker_state_funcs = { > > static int > > vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state) > > { > > + struct vc4_dev *vc4 = to_vc4_dev(dev); > > int ret; > > > > ret = vc4_ctm_atomic_check(dev, state); > > @@ -483,7 +484,10 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state) > > if (ret) > > return ret; > > > > - return vc4_load_tracker_atomic_check(state); > > + if (vc4->load_tracker_enabled) > > + return vc4_load_tracker_atomic_check(state); > > + > > + return 0; > > } > > > > static const struct drm_mode_config_funcs vc4_mode_funcs = {
On Fri, 30 Nov 2018 12:30:52 -0800 Eric Anholt <eric@anholt.net> wrote: > Paul Kocialkowski <paul.kocialkowski@bootlin.com> writes: > > > In order to test whether the load tracker is working as expected, we > > need the ability to compare the commit result with the underrun > > indication. With the load tracker always enabled, commits that are > > expected to trigger an underrun are always rejected, so userspace > > cannot get the actual underrun indication from the hardware. > > > > Add a debugfs entry to disable/enable the load tracker, so that a DRM > > commit expected to trigger an underrun can go through with the load > > tracker disabled. The underrun indication is then available to > > userspace and can be checked against the commit result with the load > > tracker enabled. > > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > > Given that the load tracker is going to be conservative and say things > will underrun even when they might not in practice, will this actually > be useful for automated testing? Or is the intent to make it easier to > tune the load tracker by disabling it so that you can experiment freely? Yes, that's one goal, though I'm not sure IGT is supposed to contain such debugging tools. But the main benefit is being able to track regressions in the load tracking algo that makes it more (too?) conservative. I think people won't like this sort of regressions. The idea would be to settle on an acceptable load tracking algo (maybe after refining the proposed one), record the results (both good and too conservative predictions) and use that as a reference for the IGT test.
Boris Brezillon <boris.brezillon@bootlin.com> writes: > On Fri, 30 Nov 2018 12:30:52 -0800 > Eric Anholt <eric@anholt.net> wrote: > >> Paul Kocialkowski <paul.kocialkowski@bootlin.com> writes: >> >> > In order to test whether the load tracker is working as expected, we >> > need the ability to compare the commit result with the underrun >> > indication. With the load tracker always enabled, commits that are >> > expected to trigger an underrun are always rejected, so userspace >> > cannot get the actual underrun indication from the hardware. >> > >> > Add a debugfs entry to disable/enable the load tracker, so that a DRM >> > commit expected to trigger an underrun can go through with the load >> > tracker disabled. The underrun indication is then available to >> > userspace and can be checked against the commit result with the load >> > tracker enabled. >> > >> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> >> >> Given that the load tracker is going to be conservative and say things >> will underrun even when they might not in practice, will this actually >> be useful for automated testing? Or is the intent to make it easier to >> tune the load tracker by disabling it so that you can experiment freely? > > Yes, that's one goal, though I'm not sure IGT is supposed to contain > such debugging tools. But the main benefit is being able to track > regressions in the load tracking algo that makes it more (too?) > conservative. I think people won't like this sort of regressions. The > idea would be to settle on an acceptable load tracking algo (maybe > after refining the proposed one), record the results (both good and too > conservative predictions) and use that as a reference for the IGT > test. Yeah, I think I'm sold on it at this point -- having a tool that isn't an automated regression test, but an automated thing that can help a developer see how accurate the estimate is, would be useful and is worth a bit of kernel code to support.
diff --git a/drivers/gpu/drm/vc4/vc4_debugfs.c b/drivers/gpu/drm/vc4/vc4_debugfs.c index 7a0003de71ab..8f4d7fadb226 100644 --- a/drivers/gpu/drm/vc4/vc4_debugfs.c +++ b/drivers/gpu/drm/vc4/vc4_debugfs.c @@ -32,9 +32,67 @@ static const struct drm_info_list vc4_debugfs_list[] = { #define VC4_DEBUGFS_ENTRIES ARRAY_SIZE(vc4_debugfs_list) +static int vc4_debugfs_load_tracker_get(struct seq_file *m, void *data) +{ + struct drm_device *dev = m->private; + struct vc4_dev *vc4 = to_vc4_dev(dev); + + if (vc4->load_tracker_enabled) + seq_printf(m, "enabled\n"); + else + seq_printf(m, "disabled\n"); + + return 0; +} + +static ssize_t vc4_debugfs_load_tracker_set(struct file *file, + const char __user *ubuf, + size_t len, loff_t *offp) +{ + struct seq_file *m = file->private_data; + struct drm_device *dev = m->private; + struct vc4_dev *vc4 = to_vc4_dev(dev); + char buf[32] = {}; + + if (len >= sizeof(buf)) + return -EINVAL; + + if (copy_from_user(buf, ubuf, len)) + return -EFAULT; + + if (!strncasecmp(buf, "enable", 6)) + vc4->load_tracker_enabled = true; + else if (!strncasecmp(buf, "disable", 7)) + vc4->load_tracker_enabled = false; + else + return -EINVAL; + + return len; +} + +static int vc4_debugfs_load_tracker_open(struct inode *inode, struct file *file) +{ + return single_open(file, vc4_debugfs_load_tracker_get, inode->i_private); +} + +static const struct file_operations vc4_debugfs_load_tracker_fops = { + .owner = THIS_MODULE, + .open = vc4_debugfs_load_tracker_open, + .read = seq_read, + .write = vc4_debugfs_load_tracker_set, +}; + int vc4_debugfs_init(struct drm_minor *minor) { + struct dentry *dentry; + + dentry = debugfs_create_file("load_tracker", S_IRUGO | S_IWUSR, + minor->debugfs_root, minor->dev, + &vc4_debugfs_load_tracker_fops); + if (!dentry) + return -ENOMEM; + return drm_debugfs_create_files(vc4_debugfs_list, VC4_DEBUGFS_ENTRIES, minor->debugfs_root, minor); } diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c index 7195a0bcceb3..12ac971d24d6 100644 --- a/drivers/gpu/drm/vc4/vc4_drv.c +++ b/drivers/gpu/drm/vc4/vc4_drv.c @@ -290,6 +290,8 @@ static int vc4_drm_bind(struct device *dev) drm_fbdev_generic_setup(drm, 32); + vc4->load_tracker_enabled = true; + return 0; unbind_all: diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h index 11369da944b6..8d0f2f16a9e8 100644 --- a/drivers/gpu/drm/vc4/vc4_drv.h +++ b/drivers/gpu/drm/vc4/vc4_drv.h @@ -188,6 +188,8 @@ struct vc4_dev { int power_refcount; + bool load_tracker_enabled; + /* Mutex controlling the power refcount. */ struct mutex power_lock; diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c index b905a19c1470..4b2509b1b8ed 100644 --- a/drivers/gpu/drm/vc4/vc4_kms.c +++ b/drivers/gpu/drm/vc4/vc4_kms.c @@ -473,6 +473,7 @@ static const struct drm_private_state_funcs vc4_load_tracker_state_funcs = { static int vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state) { + struct vc4_dev *vc4 = to_vc4_dev(dev); int ret; ret = vc4_ctm_atomic_check(dev, state); @@ -483,7 +484,10 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state) if (ret) return ret; - return vc4_load_tracker_atomic_check(state); + if (vc4->load_tracker_enabled) + return vc4_load_tracker_atomic_check(state); + + return 0; } static const struct drm_mode_config_funcs vc4_mode_funcs = {
In order to test whether the load tracker is working as expected, we need the ability to compare the commit result with the underrun indication. With the load tracker always enabled, commits that are expected to trigger an underrun are always rejected, so userspace cannot get the actual underrun indication from the hardware. Add a debugfs entry to disable/enable the load tracker, so that a DRM commit expected to trigger an underrun can go through with the load tracker disabled. The underrun indication is then available to userspace and can be checked against the commit result with the load tracker enabled. Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> --- Changes since v1: * Moved all the debugfs-related functions and structure to vc4_debugfs.c. drivers/gpu/drm/vc4/vc4_debugfs.c | 58 +++++++++++++++++++++++++++++++ drivers/gpu/drm/vc4/vc4_drv.c | 2 ++ drivers/gpu/drm/vc4/vc4_drv.h | 2 ++ drivers/gpu/drm/vc4/vc4_kms.c | 6 +++- 4 files changed, 67 insertions(+), 1 deletion(-)