Re: [Intel-gfx] [PATCH 2/3] drm/i915: Protect dev_priv->atomic_cdclk_freq with all the crtc locks

2016-11-18 Thread Ville Syrjälä 
On Fri, Nov 18, 2016 at 11:26:18AM +0100, Maarten Lankhorst wrote:
> 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.syrj...@linux.intel.com:
> > From: Ville Syrjälä 
> >
> > 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 
> > Signed-off-by: Ville Syrjälä 
> > ---
> >  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.

Re: [Intel-gfx] [PATCH 2/3] drm/i915: Protect dev_priv->atomic_cdclk_freq with all the crtc locks

2016-11-18 Thread Maarten Lankhorst 
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.syrj...@linux.intel.com:
> From: Ville Syrjälä 
>
> 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 
> Signed-off-by: Ville Syrjälä 
> ---
>  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))

Re: [Intel-gfx] [PATCH 2/3] drm/i915: Protect dev_priv->atomic_cdclk_freq with all the crtc locks

2016-11-17 Thread 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.syrj...@linux.intel.com:
> >>> From: Ville Syrjälä 
> >>>
> >>> 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 
> >>> Signed-off-by: Ville Syrjälä 
> >>> ---
> >>>  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.

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list

Re: [Intel-gfx] [PATCH 2/3] drm/i915: Protect dev_priv->atomic_cdclk_freq with all the crtc locks

2016-11-15 Thread Maarten Lankhorst 
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ä 
>>>
>>> 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 
>>> Signed-off-by: Ville Syrjälä 
>>> ---
>>>  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

Re: [Intel-gfx] [PATCH 2/3] drm/i915: Protect dev_priv->atomic_cdclk_freq with all the crtc locks

2016-11-15 Thread 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ä 
> >
> > 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 
> > Signed-off-by: Ville Syrjälä 
> > ---
> >  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.

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/3] drm/i915: Protect dev_priv->atomic_cdclk_freq with all the crtc locks

2016-11-15 Thread Maarten Lankhorst 
Op 14-11-16 om 17:35 schreef ville.syrj...@linux.intel.com:
> From: Ville Syrjälä 
>
> 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 
> Signed-off-by: Ville Syrjälä 
> ---
>  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.

~Maarten
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/3] drm/i915: Protect dev_priv->atomic_cdclk_freq with all the crtc locks

2016-11-15 Thread Ville Syrjälä 
On Tue, Nov 15, 2016 at 10:08:10AM +0100, Daniel Vetter wrote:
> On Mon, Nov 14, 2016 at 06:35:10PM +0200, ville.syrj...@linux.intel.com wrote:
> > From: Ville Syrjälä 
> > 
> > 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.
> 
> It's a bit a bikeshed, but everywhere else in atomic those kinds of data
> are called ->state or something like that. Diverging and giving it an
> atomic_ prefix seems a bit funky.
> 
> Maybe we need a dev_priv->state substruct where we could put all these
> global atomic bits into?

Yeah, I've been complaining about this stuff for quite a while now.
But so far I wasn't able to bait Maarten into doing the work ;)

> -Daniel
> 
> > 
> > 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 
> > Signed-off-by: Ville Syrjälä 
> > ---
> >  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;
> > +   }
> > +
> > +   /* All pipes must be switched off while we change the cdclk. */
> > if (intel_state->dev_cdclk != dev_priv->cdclk_freq ||
> > -   intel_state->cdclk_pll_vco != dev_priv->cdclk_pll.vco)
> > +   intel_state->cdclk_pll_vco != dev_priv->cdclk_pll.vco) {
> > ret = intel_modeset_all_pipes(state);
> > -
> > -   if (ret < 0)
> > -   return ret;
> > +   if (ret < 0)
> > +   return ret;
> > +   }
> >  
> > DRM_DEBUG_KMS("New cdclk calculated to be atomic %u, actual 
> > %u\n",
> >

Re: [Intel-gfx] [PATCH 2/3] drm/i915: Protect dev_priv->atomic_cdclk_freq with all the crtc locks

2016-11-15 Thread Daniel Vetter 
On Tue, Nov 15, 2016 at 10:08:10AM +0100, Daniel Vetter wrote:
> On Mon, Nov 14, 2016 at 06:35:10PM +0200, ville.syrj...@linux.intel.com wrote:
> > From: Ville Syrjälä 
> > 
> > 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.
> 
> It's a bit a bikeshed, but everywhere else in atomic those kinds of data
> are called ->state or something like that. Diverging and giving it an
> atomic_ prefix seems a bit funky.
> 
> Maybe we need a dev_priv->state substruct where we could put all these
> global atomic bits into?

Even more bikesheds in this area: Having different names for the same
stuff in intel_state and dev_priv is imo also bad.

Another one: We unconditionally put the values from intel_state into
dev_priv when intel_state->modeset is set. I think going more full-on with
state structs (intel_global_modeset_state) and pointers would streamline
the code a lot. With a full state structure and a real pointer in
intel_state we could also use the full get_state/destroy_state pattern,
and hide the locking in there. Everyone who just needs read access could
then either look at dev_priv->modeset_state->cdclk_freq, or we could have
a copy in intel_pipe_config.

Oh well, I guess cleaning that up is something for another day. Patch
itself looks correct.

Reviewed-by: Daniel Vetter 

> -Daniel
> 
> > 
> > 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 
> > Signed-off-by: Ville Syrjälä 
> > ---
> >  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;
> > +

Re: [Intel-gfx] [PATCH 2/3] drm/i915: Protect dev_priv->atomic_cdclk_freq with all the crtc locks

2016-11-15 Thread Daniel Vetter 
On Mon, Nov 14, 2016 at 06:35:10PM +0200, ville.syrj...@linux.intel.com wrote:
> From: Ville Syrjälä 
> 
> 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.

It's a bit a bikeshed, but everywhere else in atomic those kinds of data
are called ->state or something like that. Diverging and giving it an
atomic_ prefix seems a bit funky.

Maybe we need a dev_priv->state substruct where we could put all these
global atomic bits into?
-Daniel

> 
> 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 
> Signed-off-by: Ville Syrjälä 
> ---
>  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;
> + }
> +
> + /* All pipes must be switched off while we change the cdclk. */
>   if (intel_state->dev_cdclk != dev_priv->cdclk_freq ||
> - intel_state->cdclk_pll_vco != dev_priv->cdclk_pll.vco)
> + intel_state->cdclk_pll_vco != dev_priv->cdclk_pll.vco) {
>   ret = intel_modeset_all_pipes(state);
> -
> - if (ret < 0)
> - return ret;
> + if (ret < 0)
> + return ret;
> + }
>  
>   DRM_DEBUG_KMS("New cdclk calculated to be atomic %u, actual 
> %u\n",
> intel_state->cdclk, intel_state->dev_cdclk);
> -- 
> 2.7.4
> 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[Intel-gfx] [PATCH 2/3] drm/i915: Protect dev_priv->atomic_cdclk_freq with all the crtc locks

2016-11-14 Thread ville . syrjala 
From: Ville Syrjälä 

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 
Signed-off-by: Ville Syrjälä 
---
 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;
+   }
+
+   /* All pipes must be switched off while we change the cdclk. */
if (intel_state->dev_cdclk != dev_priv->cdclk_freq ||
-   intel_state->cdclk_pll_vco != dev_priv->cdclk_pll.vco)
+   intel_state->cdclk_pll_vco != dev_priv->cdclk_pll.vco) {
ret = intel_modeset_all_pipes(state);
-
-   if (ret < 0)
-   return ret;
+   if (ret < 0)
+   return ret;
+   }
 
DRM_DEBUG_KMS("New cdclk calculated to be atomic %u, actual 
%u\n",
  intel_state->cdclk, intel_state->dev_cdclk);
-- 
2.7.4

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx