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

From: Daniel Vetter <daniel@ffwll.ch>
To: Thierry Reding <thierry.reding@gmail.com>
Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>,
	Daniel Vetter <daniel.vetter@intel.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	Emil Velikov <emil.velikov@collabora.com>
Subject: Re: [PATCH v1 2/3] drm: Add API for capturing frame CRCs
Date: Wed, 22 Jun 2016 16:08:52 +0200	[thread overview]
Message-ID: <CAKMK7uHu3+BM=cn2GiniTCd_ML9QmpvC5Gj9MKn9LvMK69b0YQ@mail.gmail.com> (raw)
In-Reply-To: <20160622133216.GL26943@ulmo.ba.sec>

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.
And by reading the control file you can check what's the currently
selected source easily.

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.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch