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

[Daniel Vetter]

On Thu, Jun 23, 2016 at 10:43 AM, Thierry Reding
<thierry.reding@xxxxxxxxx> wrote:
> On Thu, Jun 23, 2016 at 10:24:46AM +0200, Tomeu Vizoso wrote:
>> On 23 June 2016 at 10:21, Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote:
>> > On Wed, 22 Jun 2016, Daniel Vetter <daniel@xxxxxxxx> wrote:
>> >> On Wed, Jun 22, 2016 at 4:31 PM, Thierry Reding
>> >> <thierry.reding@xxxxxxxxx> wrote:
>> >>> 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.
>> >
>> > Side note, we should eventually do the same for sink CRCs, but I guess
>> > under the connectors. i915 currently has a special cased version for eDP
>> > (named "i915_sink_crc_eDP1"...), reading the data from DPCD.
>>
>> Was hoping we could just add one more source to this interface to expose those.
>
> I don't think that would be easy to achieve. You'd have to determine
> that a sink source is connected to the CRTC and then ajdust the list
> of possible sources dynamically.
>
> For connectors we already have separate directories in debugfs and a
> sink can easily be found from the connector it is attached to.
>
> It's also possible, though I don't know of any that do so currently,
> that eventually sinks will support several types (i.e. "sources") of
> CRC, which will further complicate to represent this in the CRTC's
> list of CRC sources.
>
> And it may also be useful to have both CRCs at the same time. The eDP
> specification for example defines a very specific polynomial that is
> used to compute the CRC, whereas not all display hardware uses a CRC
> algorithm that's documented.
>
> Finally, the data that will be checksummed by the CRTC isn't necessarily
> the same as that arriving at the sink.

We already do all that for i915. On some platforms the normal
end-of-pipe CRC doesn't work together with DP, instead you need to use
a special port (= connector/encoder) based CRC. The "auto" one
automatically walks the modeset config on those platforms to figure
out which one to pick. It would be kinda hard for userspace to
automatically figure out when the auto CRC doesn't work on the CRTC
and it would instead need to use the connector one. I think it'd
better to hide that in the kernel driver, where it's known.

Wrt specific polynomials: We could just spec that the "eDP-sink"
source is the one with CRC computed per the eDP spec. Similar for
anything else standardize. Heck you could even do the same with vendor
specific CRCs on IP blocks by using special names.

Wrt CRC measuring different data: That's also already the case on
i915. We have CRC for pre-gamma, post-gamma, post panel-fitter, on the
port, with or without audio stream included, .... And this all even
varies between platforms. The only thing that's specifified is that
"auto" should be some CRC after all the blending/color-correction has
been applied, but before any port specific scrambling happens which
might make the CRC change with each frame. E.g. on platforms where
auto aliases the DP ports we need to set a special bit to reset the
scrambler state on each vblank to ensure stable CRCs.

Given all that I think putting sink crc capture into the same
framework makes sense.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch