On Wed, Jun 22, 2016 at 4:31 PM, Thierry Reding <thierry.reding@xxxxxxxxx> wrote: > 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@xxxxxxxxx> 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. crtc-<index>/crc/(control|data) sounds great. >> 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 think it's useful for hacking together your driver support, going through tests it one more indirection. And I have a really bad attention span ;-) >> 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. Yeah, we might have room for more ... otoh for read-only debug information I prefer big files that dump everything. Easier to extend. But for tests/automation the one-value-per-file idea from sysfs, or at least going much closer to that idea is a good idea. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
