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@xxxxxxxxxxxxx> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
