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.syrj...@linux.intel.com:
>>> From: Ville Syrjälä <ville.syrj...@linux.intel.com>
>>>
>>> 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.lankho...@linux.intel.com>
>>> Signed-off-by: Ville Syrjälä <ville.syrj...@linux.intel.com>
>>> ---
>>> 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.
~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
