Re: [PATCH v1 2/3] drm: Add API for capturing frame CRCs

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3469 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]