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

[Tomeu Vizoso <tomeu.vizoso@collabora.com>]

On 8 September 2016 at 15:35, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> 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@collabora.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.

Have added a note to the commit message, as once the legacy codepath
is removed, it could be confusing.

>
>>     - 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 ?

Oops, both instances fixed in v7.

Thanks,

Tomeu

>>         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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx