Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1475234 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v4 3/4] drm/i915: Use new CRC debugfs API Emil Velikov <emil.l.velikov@gmail.com> - 2016-09-02 17:20 +0200
    Re: [PATCH v4 3/4] drm/i915: Use new CRC debugfs API Tomeu Vizoso <tomeu.vizoso@collabora.com> - 2016-09-05 11:50 +0200
      Re: [PATCH v4 3/4] drm/i915: Use new CRC debugfs API Emil Velikov <emil.l.velikov@gmail.com> - 2016-09-05 14:50 +0200
        Re: [Intel-gfx] [PATCH v4 3/4] drm/i915: Use new CRC debugfs API Tomeu Vizoso <tomeu.vizoso@collabora.com> - 2016-09-05 15:20 +0200

#1475234 — Re: [PATCH v4 3/4] drm/i915: Use new CRC debugfs API

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.

On 5 August 2016 at 11:45, Tomeu Vizoso <tomeu.vizoso@collabora.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 ?


> @@ -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.


> @@ -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.


> +int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
> +                             size_t *values_cnt)
> +{

> +       ret = do_set_crc_source(crtc->dev, crtc->index, source);
> +
> +       intel_display_power_put(dev_priv, power_domain);
> +
> +       *values_cnt = 5;
> +
Please don't overwrite values_cnt if the function fails.

Regards,
Emil

[toc] | [next] | [standalone]

#1476239

On 2 September 2016 at 17:18, Emil Velikov <emil.l.velikov@gmail.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@collabora.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.

>> @@ -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.

>> +int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
>> +                             size_t *values_cnt)
>> +{
>
>> +       ret = do_set_crc_source(crtc->dev, crtc->index, source);
>> +
>> +       intel_display_power_put(dev_priv, power_domain);
>> +
>> +       *values_cnt = 5;
>> +
> Please don't overwrite values_cnt if the function fails.

Done.

Thanks,

Tomeu

> Regards,
> Emil
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

[toc] | [prev] | [next] | [standalone]

#1476384

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

Thanks
Emil

[toc] | [prev] | [next] | [standalone]

#1476405 — 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@gmail.com> wrote:
> On 5 September 2016 at 10:45, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
>> On 2 September 2016 at 17:18, Emil Velikov <emil.l.velikov@gmail.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@collabora.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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web