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. > 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
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
