On Wed, Jun 22, 2016 at 04:08:52PM +0200, Daniel Vetter wrote:
On Wed, Jun 22, 2016 at 3:32 PM, Thierry Reding thierry.reding@gmail.com wrote:
- 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.
In my opinion that makes things needlessly complicated for userspace. If you want to query the state of a specific CRTC, you have to read out the entire file and parse each line to find the correct CRTC. On the other hand, chances are that you already need to know the path to the CRTC because you want to read the CRC out of the per-CRTC CRC file. In that case it would be much easier to simply concatenate the CRTC path and the CRC (or control) filename and read a single line (actually a single word) out of it to get at the same information.
Furthermore if you have everything per-CRTC you no longer have to worry about pipe vs. index (that's always confusing because in the DRM core they're actually synonymous) because the CRTC path is canonical and will have the correct context.
Per-CRTC directory with a single duplex file, or separate control and CRC files, is much simpler than the mix proposed here. No tokenization required when parsing in userspace, and no tokenization required to parse in the kernel either.
Just jumping on this one here. I agree that if we remodel the interface making the control file per-crtc would make sense. I think separate control and read files makes sense, that's much less magic.
Agreed, separate files would be a little simpler. I must admit that my proposal is partially motivated by a desire to avoid cumbersome naming of files. If we have separate files, what do you name them? crc for reading, crc_control for writing? crc_values for reading and crc for writing?
Perhaps another way to avoid that would be to put the two files into a separate directory, as in:
/sys/kernel/debug/dri/<minor>/crtc-<pipe>/crc/ +-- control +-- data
That's slightly on the deeply nested side, but on the other hand it nicely uses the filesystem for namespacing, which is what filesystems are really good at.
And by reading the control file you can check what's the currently selected source easily.
Is that really a useful feature? If you're going to capture CRCs, you likely just want to set whatever you expect to receive irrespective of the current setting.
I'm not sure on the canonical CRTC path - right now we don't have that in debugfs. I think just using index numbers is ok, we use those all over the place already. Or maybe we could indeed add a new per-crtc subdir in debugfs for this. Either way is fine with me.
I can imagine that we'd like to expose a number of other per-CRTC properties (name, parts of the state, object ID, one day perhaps VBLANK counts, ...) this way, so a per-CRTC directory makes a lot of sense in my opinion.
Thierry