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

[Daniel Vetter]

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