Op 17-11-16 om 16:06 schreef Ville Syrjälä:
> On Tue, Nov 15, 2016 at 02:53:00PM +0100, Maarten Lankhorst wrote:
>> Op 15-11-16 om 14:41 schreef Ville Syrjälä:
>>> On Tue, Nov 15, 2016 at 11:14:29AM +0100, Maarten Lankhorst wrote:
>>>> Op 14-11-16 om 17:35 schreef ville.syrjala@xxxxxxxxxxxxxxx:
>>>>> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>>>>>
>>>>> A modeset on one pipe can update dev_priv->atomic_cdclk_freq without
>>>>> actually touching the hardware, in which case we won't force a modeset
>>>>> on all the pipes, and thus won't lock any of the other pipes either.
>>>>> That means a parallel plane update on another pipe could be looking at
>>>>> a stale dev_priv->atomic_cdcdlk_freq and thus fail to notice when the
>>>>> plane configuration is invalid, or potentially reject a valid update.
>>>>>
>>>>> To overcome this we must protect writes to atomic_cdclk_freq with
>>>>> all the crtc locks, and thus for reads any single crtc lock will
>>>>> be sufficient protection.
>>>>>
>>>>> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
>>>>> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>>>>> ---
>>>>> drivers/gpu/drm/i915/i915_drv.h | 9 +++++++-
>>>>> drivers/gpu/drm/i915/intel_display.c | 41 +++++++++++++++++++++++++++++++-----
>>>>> 2 files changed, 44 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>>>> index c0f1dfc7119e..66d2950dc657 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>>> @@ -1874,7 +1874,14 @@ struct drm_i915_private {
>>>>>
>>>>> unsigned int fsb_freq, mem_freq, is_ddr3;
>>>>> unsigned int skl_preferred_vco_freq;
>>>>> - unsigned int cdclk_freq, max_cdclk_freq, atomic_cdclk_freq;
>>>>> + unsigned int cdclk_freq, max_cdclk_freq;
>>>>> +
>>>>> + /*
>>>>> + * For reading holding any crtc lock is sufficient,
>>>>> + * for writing must hold all of them.
>>>>> + */
>>>>> + unsigned int atomic_cdclk_freq;
>>>>> +
>>>>> unsigned int max_dotclk_freq;
>>>>> unsigned int rawclk_freq;
>>>>> unsigned int hpll_freq;
>>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>>> index 70f3f0b70263..d7a4bc63b05b 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>>> @@ -13946,13 +13946,32 @@ static int haswell_mode_set_planes_workaround(struct drm_atomic_state *state)
>>>>> return 0;
>>>>> }
>>>>>
>>>>> +static int intel_lock_all_pipes(struct drm_atomic_state *state)
>>>>> +{
>>>>> + struct drm_crtc *crtc;
>>>>> +
>>>>> + /* Add all pipes to the state */
>>>>> + for_each_crtc(state->dev, crtc) {
>>>>> + struct drm_crtc_state *crtc_state;
>>>>> +
>>>>> + crtc_state = drm_atomic_get_crtc_state(state, crtc);
>>>>> + if (IS_ERR(crtc_state))
>>>>> + return PTR_ERR(crtc_state);
>>>>> + }
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> static int intel_modeset_all_pipes(struct drm_atomic_state *state)
>>>>> {
>>>>> struct drm_crtc *crtc;
>>>>> struct drm_crtc_state *crtc_state;
>>>>> int ret = 0;
>>>>>
>>>>> - /* add all active pipes to the state */
>>>>> + /*
>>>>> + * Add all pipes to the state, and force
>>>>> + * a modeset on all the active ones.
>>>>> + */
>>>>> for_each_crtc(state->dev, crtc) {
>>>>> crtc_state = drm_atomic_get_crtc_state(state, crtc);
>>>>> if (IS_ERR(crtc_state))
>>>>> @@ -14018,12 +14037,24 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
>>>>> if (ret < 0)
>>>>> return ret;
>>>>>
>>>>> + /*
>>>>> + * Writes to dev_priv->atomic_cdclk_freq must protected by
>>>>> + * holding all the crtc locks, even if we don't end up
>>>>> + * touching the hardware
>>>>> + */
>>>>> + if (intel_state->cdclk != dev_priv->atomic_cdclk_freq) {
>>>>> + ret = intel_lock_all_pipes(state);
>>>>> + if (ret < 0)
>>>>> + return ret;
>>>>> + }
>>>>> +
>>>> Would it be terrible to just use intel_modeset_all_pipes here? Since this can only be different in the all crtc's disabled case
>>>> it won't matter much.
>>> Is there any benefit in doing that? A bit confusing IMO to force a
>>> modeset when you don't have to.
>>>
>> The case where atomic cdclk changes, but dev_cdclk stays the same can only happen
>> if you configure a crtc, but all crtc's stay !active. In all other cases dev_cdclk
>> will change too.
>>
>> intel_modeset_all_pipes will only set mode_changed on active crtc's, but it will
>> add all crtc's to the atomic state regardless to make sure the cdclk stays consistent.
> I still don't see what the benefit is. IMO it's just confusing to say
> that we're going to force a modeset on a disabled pipe.
>
if (!active || needs_modeset(crtc_state)) continue; crtc_state->mode_changed= true; It's not doing a modeset on a disabled pipe, it only makes sure it's part of the state.
~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
