linux-kernel - Re: [Intel-gfx] [PATCH v4 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] [day] [month] [year] [list]

Message-ID: <CAAObsKBVC4UcsN1-5R9apk46WLz30A5yuGEYLSOa6g+dGoD51w@mail.gmail.com>
Date:   Mon, 5 Sep 2016 15:12:40 +0200
From:   Tomeu Vizoso <tomeu.vizoso@...labora.com>
To:     Emil Velikov <emil.l.velikov@...il.com>
Cc:     Daniel Vetter <daniel.vetter@...el.com>,
        "intel-gfx@...ts.freedesktop.org" <intel-gfx@...ts.freedesktop.org>,
        "Linux-Kernel@...r. Kernel. Org" <linux-kernel@...r.kernel.org>,
        ML dri-devel <dri-devel@...ts.freedesktop.org>,
        Emil Velikov <emil.velikov@...labora.com>
Subject: Re: [Intel-gfx] [PATCH v4 3/4] drm/i915: Use new CRC debugfs API

On 5 September 2016 at 14:44, Emil Velikov <emil.l.velikov@...il.com> wrote:
> On 5 September 2016 at 10:45, Tomeu Vizoso <tomeu.vizoso@...labora.com> wrote:
>> On 2 September 2016 at 17:18, Emil Velikov <emil.l.velikov@...il.com> wrote:
>>> Hi Tomeu,
>>>
>>> IMHO it would be better to split out the refactoring into preparatory
>>> patch. It brings a minor change which (not 100% sure on that) should
>>> not cause issues but is worth pointing out.
>>
>> I think at this point it would make sense to change the series
>> structure only if there was a strong reason, as a few people have
>> already looked at the patches already.
>>
>>> On 5 August 2016 at 11:45, Tomeu Vizoso <tomeu.vizoso@...labora.com> wrote:
>>>
>>>> +static int do_set_crc_source(struct drm_device *dev, enum pipe pipe,
>>>> +                            enum intel_pipe_crc_source source)
>>>> +{
>>>
>>>> +       if (source == INTEL_PIPE_CRC_SOURCE_NONE) {
>>> Nit: use !source here or sourse != INTEL_PIPE_CRC_SOURCE_NONE
>>> elsewhere in the code ?
>>
>> Agreed.
>>
>>>
>>>> @@ -693,10 +718,11 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
>>>>                 spin_unlock_irq(&pipe_crc->lock);
>>>>         }
>>>>
>>>> -       pipe_crc->source = source;
>>>> +       ret = do_set_crc_source(dev, pipe, source);
>>>> +       if (ret)
>>>> +               goto out;
>>>>
>>> We seem to have modified pipe_crc even if the new function fails.
>>> Haven't check if it matters, but definatelly not ideal.
>>
>> If we had modified pipe_crc that's because we were trying to start CRC
>> capture and we initialized the entry storage. As CRC generation is
>> disabled, those changes have no effects. When CRC capture is attempted
>> again, they will be initialized again.
>>
>> To avoid this we would need to split the HW programming in two
>> functions and I'm not sure it would be worth it.
>>
> A simple way out will be to keep the "can fail" hunk at the top
> separate from the rest. This way even if things get reinitialised
> correctly currently, they won't break if someone applies the
> (perfectly reasonable imho) assumption "function does not modify any
> data when it fails".
> </preach mode>
>
>>>> @@ -720,15 +746,6 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
>>>>                 spin_unlock_irq(&pipe_crc->lock);
>>>>
>>>>                 kfree(entries);
>>>> -
>>>> -               if (IS_G4X(dev))
>>>> -                       g4x_undo_pipe_scramble_reset(dev, pipe);
>>>> -               else if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev))
>>>> -                       vlv_undo_pipe_scramble_reset(dev, pipe);
>>>> -               else if (IS_HASWELL(dev) && pipe == PIPE_A)
>>>> -                       hsw_trans_edp_pipe_A_crc_wa(dev, false);
>>>> -
>>>> -               hsw_enable_ips(crtc);
>>> The above is the piece I have in mind:
>>> With the introduction of do_set_crc_source() the above are executed
>>> prior to the intel_wait_for_vblank() call.
>>>
>>> Afaics this will not cause any functional change, then again I'm not
>>> that familiar with the i915 vblank code.
>>
>> Yeah, not sure either of when do those changes take effect.
>>
> With this said, it would be way better to keep it separate (with a big
> fat warning in the commit summary).
>
> Speaking of which - why did you fold the separate bugfix/workaround
> "skip the first one or two frames" in v5 ? Shouldn't it be separate so
> that people can pick it for -fixes/-stable ?

Oh, that's only in the new code paths. For the old i915 ABI, the
frames are skipped in userspace, but as it's something highly
HW-dependent, implementors of the new API need to do it within their
drivers.

Regards,

Tomeu