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

[Tomeu Vizoso]

On 5 September 2016 at 14:44, Emil Velikov <emil.l.velikov@xxxxxxxxx> wrote:
> On 5 September 2016 at 10:45, Tomeu Vizoso <tomeu.vizoso@xxxxxxxxxxxxx> wrote:
>> On 2 September 2016 at 17:18, Emil Velikov <emil.l.velikov@xxxxxxxxx> 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@xxxxxxxxxxxxx> 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