linux-kernel - Re: [Intel-gfx] [PATCH v6 3/4] drm/i915: Use new CRC debugfs API

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACvgo52p3J0mPNLhO7DJY14WnWCK0nzQTFm1eqAgEDMGi_CzoQ@mail.gmail.com>
Date:   Thu, 8 Sep 2016 14:35:01 +0100
From:   Emil Velikov <emil.l.velikov@...il.com>
To:     Tomeu Vizoso <tomeu.vizoso@...labora.com>
Cc:     "Linux-Kernel@...r. Kernel. Org" <linux-kernel@...r.kernel.org>,
        David Airlie <airlied@...ux.ie>,
        "intel-gfx@...ts.freedesktop.org" <intel-gfx@...ts.freedesktop.org>,
        Thierry Reding <thierry.reding@...il.com>,
        ML dri-devel <dri-devel@...ts.freedesktop.org>,
        Daniel Vetter <daniel.vetter@...el.com>,
        Emil Velikov <emil.velikov@...labora.com>
Subject: Re: [Intel-gfx] [PATCH v6 3/4] drm/i915: Use new CRC debugfs API

Hi Tomeu,

Just a couple of nitpicks. Nothing that has to be fixed or (if you
agree) cannot be done on top/later on.

On 7 September 2016 at 11:27, Tomeu Vizoso <tomeu.vizoso@...labora.com> wrote:
> The core provides now an ABI to userspace for generation of frame CRCs,
> so implement the ->set_crc_source() callback and reuse as much code as
> possible with the previous ABI implementation.
>
> v2:
>     - Leave the legacy implementation in place as the ABI implementation
>       in the core is incompatible with it.
> v3:
>     - Use the "cooked" vblank counter so we have a whole 32 bits.
>     - Make sure we don't mess with the state of the legacy CRC capture
>       ABI implementation.
> v4:
>     - Keep use of get_vblank_counter as in the legacy code, will be
>       changed in a followup commit.
>
> v5:
>     - Skip first frame or two as it's known that they contain wrong
>       data.
Even if the frames are only skipped in the new code, it doesn't
explain why one'd need it in the first place and/or how it isn't
required with the current code. Might be worth poking the original
authors and/or adding a big WARNING/NOTE/XXX/HACK to make things more
prominent.


>     - A few fixes suggested by Emil Velikov.
>
> v6:
>     - Rework programming of the HW registers to preserve previous
>       behavior.
>
Huge thanks for this.


> @@ -791,7 +797,7 @@ display_crc_ctl_parse_object(const char *buf, enum intel_pipe_crc_object *o)
>                 if (!strcmp(buf, pipe_crc_objects[i])) {
>                         *o = i;
>                         return 0;
> -                   }
> +               }
>
Looks like newly introduced whitespace changes, should have been part of 1/4 ?

>         return -EINVAL;
>  }
> @@ -813,11 +819,16 @@ display_crc_ctl_parse_source(const char *buf, enum intel_pipe_crc_source *s)
>  {
>         int i;

>                 if (!strcmp(buf, pipe_crc_sources[i])) {
>                         *s = i;
>                         return 0;
> -                   }
> +               }
>
Ditto ?

Thanks
Emil