On 21 June 2016 at 17:07, Thierry Reding thierry.reding@gmail.com wrote:
On Tue, Jun 21, 2016 at 01:06:41PM +0200, Tomeu Vizoso wrote: [...]
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
[...]
+static int crc_control_show(struct seq_file *m, void *data) +{
struct drm_device *dev = m->private;struct drm_crtc *crtc;drm_for_each_crtc(crtc, dev)seq_printf(m, "crtc %d %s\n", crtc->index,crtc->crc.source ? crtc->crc.source : "none");return 0;+}
Why are these control files not per-CRTC? I'd imagine you could do something like control the CRC generation on writes and provide the sampled CRCs on reads.
We just thought there wasn't a strong point for breaking the existing API in a fundamental way. The current proposal allows us to reuse more code between the new and legacy CRC implementations in i915 and in IGT.
source = NULL;if (!crc->source && !source)return 0;if (crc->source && source && !strcmp(crc->source, source))return 0;/* Forbid changing the source without going back to "none". */if (crc->source && source)return -EINVAL;Why? It seems to me that if a driver doesn't support switching from one source to another directly, then it should internally handle that. After all the source parameter is already driver-specific, so it seems odd to impose this kind of policy on it at this level.
Hmm, I don't see when that would make sense for userspace. If userspace has a source configured and changes directly to another, how does it know what's the last CRC for the old source? I think that if userspace does that it's shooting in its foot and it's good to give an error.
drm_for_each_crtc(crtc, dev)if (i++ == index)return crtc;return NULL;+}
This looks like a candidate for the core. I know that at least Tegra implements a variant of this, and I think i915 does, too.
And a few others. I would go this way but when I pinged danvet on irc he didn't reply so I just went with the safe option.
+/*
- Parse CRC command strings:
- command: wsp* object wsp+ (crtc | pipe) wsp+ source wsp*
Should the "(crtc | pipe)" in the above be "object"?
In one case they are literals and in the other symbols.
- object: ('crtc' | 'pipe')
Because you define that here?
- crtc: (0 | 1 | 2 | ...)
- pipe: (A | B | C)
- source: (none | plane1 | plane2 | ...)
I wouldn't provide "plane1 | plane2 |" here, since the parameter is passed as-is to drivers, which may or may not support plane1 or plane2.
Agreed, feels more confusing than clarifying.
- wsp: (#0x20 | #0x9 | #0xA)+
- eg.:
- "crtc 0 plane1" -> Start CRC computations on plane1 of first CRTC
- "crtc 0 none" -> Stop CRC
I've said this above, but again, it seems odd to me that you'd have to configure the CRC per-CRTC in one per-device file and read out the CRC from per-CRTC files.
Not sure, I like that the per-crtc files just provide CRC data, and that there's a separate control file that can be queried for the current state.
entry->frame, entry->crc[0],entry->crc[1], entry->crc[2],entry->crc[3], entry->crc[4]);What about drivers that only support one uint32_t for the CRC? Do they also need to output all unused uint32_t:s?
Yeah, do you think that could be a problem?
ret = copy_to_user(user_buf, buf, CRC_LINE_LEN);if (ret == CRC_LINE_LEN)return -EFAULT;user_buf += CRC_LINE_LEN;n_entries--;spin_lock_irq(&crc->lock);}spin_unlock_irq(&crc->lock);return bytes_read;+}
+const struct file_operations drm_crtc_crc_fops = {
.owner = THIS_MODULE,.open = crtc_crc_open,.read = crtc_crc_read,.release = crtc_crc_release,+};
Do we want to support poll?
Don't see the point of it, TBH.
+static int drm_debugfs_crtc_add_for_minor(struct drm_crtc *crtc,
struct drm_minor *minor)+{
struct dentry *ent;char *name;if (!minor->debugfs_root)return -1;Can this ever happen? Perhaps turn this into a symbolic name if you really need it.
Sorry, can you explain what you mean by that?
name = kasprintf(GFP_KERNEL, "drm_crtc_%d_crc", crtc->index);if (!name)return -ENOMEM;I think it might be preferable to move this all into per-CRTC debugfs directories, perhaps even collapse the "crc" and "control" files. But in any case I think the drm_ prefix is redundant here and should be dropped.
Yeah, I kind of like the redundancy as in the client code you will only sometimes find the file name next to the directory name, but I don't particularly care myself.
+int drm_debugfs_crtc_add(struct drm_crtc *crtc) +{
int ret;ret = drm_debugfs_crtc_add_for_minor(crtc, crtc->dev->control);if (ret)return ret;ret = drm_debugfs_crtc_add_for_minor(crtc, crtc->dev->primary);if (ret)return ret;ret = drm_debugfs_crtc_add_for_minor(crtc, crtc->dev->render);if (ret)return ret;return 0;+}
Do we really want these for all minors? Is ->primary not enough? It certainly seems completely misplaced in ->render, and I don't think anything really uses ->control anymore.
Agreed.
+void drm_crtc_add_crc_entry(struct drm_crtc *crtc, uint32_t frame,
uint32_t crc0, uint32_t crc1, uint32_t crc2,uint32_t crc3, uint32_t crc4)Perhaps allow passing the CRC as an array with a count parameter? I can imagine that a lot of hardware will only give you a single uint32_t for the CRC, in which case you could do:
drm_crtc_add_crc_entry(crtc, frame, &crc, 1);instead of:
drm_crtc_add_crc_entry(crtc, frame, crc, 0, 0, 0, 0);It would probably save poor users of the interface, such as myself, a lot of headaches because they can't remember how many uint32_t:s the function needs.
Sounds good to me, I don't really know how common multiple sources of complex CRC data will be in the future.
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h index 38401d406532..e5b124d937f5 100644 --- a/drivers/gpu/drm/drm_internal.h +++ b/drivers/gpu/drm/drm_internal.h @@ -100,6 +100,8 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id, int drm_debugfs_cleanup(struct drm_minor *minor); int drm_debugfs_connector_add(struct drm_connector *connector); void drm_debugfs_connector_remove(struct drm_connector *connector); +int drm_debugfs_crtc_add(struct drm_crtc *crtc); +void drm_debugfs_crtc_remove(struct drm_crtc *crtc);
Oh... this isn't something that drivers are supposed to call?
Right, it's analogous to drm_debugfs_connector_add.
** When CRC generation is enabled, the driver should call* drm_crtc_add_crc_entry() at each frame, providing any information* that characterizes the frame contents in the crcN arguments, as* provided from the configured source. Drivers should accept a "auto"* source name that will select a default source for this CRTC.Would it be useful to provide some more aliases? "enable" and "on" for "auto", "disable" and "off" for "none"?
Not sure, TBH, i like to keep my interfaces simple. Do you think this could be helpful?
Thanks for the great review!
Tomeu