From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751880AbcFVOqP (ORCPT ); Wed, 22 Jun 2016 10:46:15 -0400 Received: from mail-pa0-f65.google.com ([209.85.220.65]:32794 "EHLO mail-pa0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750957AbcFVOqO (ORCPT ); Wed, 22 Jun 2016 10:46:14 -0400 Date: Wed, 22 Jun 2016 16:31:44 +0200 From: Thierry Reding To: Daniel Vetter Cc: Tomeu Vizoso , Daniel Vetter , "linux-kernel@vger.kernel.org" , "dri-devel@lists.freedesktop.org" , Emil Velikov Subject: Re: [PATCH v1 2/3] drm: Add API for capturing frame CRCs Message-ID: <20160622143144.GN26943@ulmo.ba.sec> References: <1466507202-23222-1-git-send-email-tomeu.vizoso@collabora.com> <1466507202-23222-3-git-send-email-tomeu.vizoso@collabora.com> <20160621150758.GB21079@ulmo.ba.sec> <20160622133216.GL26943@ulmo.ba.sec> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="4Y142/9l9nQlBiaj" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.6.1 (2016-04-27) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --4Y142/9l9nQlBiaj Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Jun 22, 2016 at 04:08:52PM +0200, Daniel Vetter wrote: > On Wed, Jun 22, 2016 at 3:32 PM, Thierry Reding > 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 C= RC > >> > 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. >=20 > 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//crtc-/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 --4Y142/9l9nQlBiaj Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAABCAAGBQJXaqFMAAoJEN0jrNd/PrOh1G8P+wblFaL/JYL5fbdQJ1NaoeVj JTDSTuDzMcASX/tAlC9vanHObsyP3zPBhNRuWlnQZzxKerM0viukZk9C5DF1JSQf VKSBPG26u47rDRkR+hiVfcSMx7zV8bqjg2yhbQN0uc0Xm/eAcQ5adXCGoFfj0wZP gn6w2t3bRtdJ5R9UwPG3Zi1EH3xL3ImaAPtOUvMciFpf94/M9mSXWHZXuMJebyz/ kcBJtiEVGA5PDe48hOyJ7N4+k4NlFkPJqaRI4pO/M/tXZ2qiHkKhqWJYkMlZhssW nzZCiSsWqe6lAaQ6axTiNS8StfXfeQeYlj5ag/x1STK75vBQY2vhcZVkTCvWPjGg aMQsO+GMIPQo7GgLk6wRXG/+adHdEl9Yu5uFkvtKdXeGZj27O9Y4bbPnvSJDDv9e cdYGK8vTeIXH5uU6jxAozFcWLt5uAYC7Na+z4ZyilN/1A7jMxRq4COpSdr2jcF78 p6wqZRnuseXQgsv4svGKlO9aGOXkiY/yL9fZl0UXMAl7o1lnjP5i2iFnRGs7YJKQ 44Me7vvhyBTh6/pboyDTwPHTDo0NgZjIPvjl3eRhrX1EXZFVaEROrlXp/jUV5hzK CiTJQ3N80L/T5vqpEH8OJKk5cZSInSbSCmgPC/TseesClJ+b9Bq/tkybjphE9jqS xIplzA1YXoG6pyZofO2q =Ko6d -----END PGP SIGNATURE----- --4Y142/9l9nQlBiaj--