[04/24] drm/i915: Update power domains only on affected crtc's.

Submitted by Maarten Lankhorst on June 1, 2015, 1:27 p.m.

Details

Message ID 1433165247-15928-5-git-send-email-maarten.lankhorst@linux.intel.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Maarten Lankhorst June 1, 2015, 1:27 p.m.
Use for_each_crtc_state to only touch affected crtc's.
In order to make sure that the initial power is still set
correctly we make sure modeset_update_crtc_power_domains is called
during the initial modeset.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c      |  3 ---
 drivers/gpu/drm/i915/intel_display.c | 41 +++++++++++++++++++++++-------------
 2 files changed, 26 insertions(+), 18 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index d3632c56fdf7..78ef0bb53c36 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -634,9 +634,6 @@  static int i915_drm_suspend(struct drm_device *dev)
 	intel_display_suspend(dev);
 	drm_modeset_unlock_all(dev);
 
-	/* suspending displays will unsets init power */
-	intel_display_set_init_power(dev_priv, true);
-
 	intel_dp_mst_suspend(dev);
 
 	intel_runtime_pm_disable_interrupts(dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8e9afc55c284..4dc07602248b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5188,42 +5188,49 @@  static unsigned long get_crtc_power_domains(struct drm_crtc *crtc)
 	return mask;
 }
 
-static void modeset_update_crtc_power_domains(struct drm_atomic_state *state)
+static void modeset_update_crtc_power_domains(struct drm_atomic_state *state, bool gr)
 {
 	struct drm_device *dev = state->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	unsigned long pipe_domains[I915_MAX_PIPES] = { 0, };
-	struct intel_crtc *crtc;
+	unsigned long pipe_domains[I915_MAX_PIPES] = { 0, }, domains;
+	struct drm_crtc_state *crtc_state;
+	struct drm_crtc *crtc;
+	int i;
 
 	/*
 	 * First get all needed power domains, then put all unneeded, to avoid
 	 * any unnecessary toggling of the power wells.
 	 */
-	for_each_intel_crtc(dev, crtc) {
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
 		enum intel_display_power_domain domain;
+		enum pipe pipe = to_intel_crtc(crtc)->pipe;
 
-		if (!crtc->base.state->enable)
+		if (!crtc->state->active)
 			continue;
 
-		pipe_domains[crtc->pipe] = get_crtc_power_domains(&crtc->base);
+		domains = pipe_domains[pipe] = get_crtc_power_domains(crtc);
+		domains &= ~to_intel_crtc(crtc)->enabled_power_domains;
 
-		for_each_power_domain(domain, pipe_domains[crtc->pipe])
+		for_each_power_domain(domain, domains)
 			intel_display_power_get(dev_priv, domain);
 	}
 
-	if (dev_priv->display.modeset_global_resources)
+	if (gr && dev_priv->display.modeset_global_resources)
 		dev_priv->display.modeset_global_resources(state);
 
-	for_each_intel_crtc(dev, crtc) {
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+		enum pipe pipe = intel_crtc->pipe;
 		enum intel_display_power_domain domain;
 
-		for_each_power_domain(domain, crtc->enabled_power_domains)
-			intel_display_power_put(dev_priv, domain);
+		domains = intel_crtc->enabled_power_domains;
+		domains &= ~pipe_domains[pipe];
 
-		crtc->enabled_power_domains = pipe_domains[crtc->pipe];
-	}
+		intel_crtc->enabled_power_domains = pipe_domains[pipe];
 
-	intel_display_set_init_power(dev_priv, false);
+		for_each_power_domain(domain, domains)
+			intel_display_power_put(dev_priv, domain);
+	}
 }
 
 void broxton_set_cdclk(struct drm_device *dev, int frequency)
@@ -12698,7 +12705,7 @@  static int __intel_set_mode(struct drm_atomic_state *state)
 	/* The state has been swaped above, so state actually contains the
 	 * old state now. */
 
-	modeset_update_crtc_power_domains(state);
+	modeset_update_crtc_power_domains(state, true);
 
 	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
@@ -15280,12 +15287,16 @@  void intel_modeset_setup_hw_state(struct drm_device *dev,
 		ret = intel_set_mode(state);
 		if (ret) {
 			DRM_ERROR("Failed to restore previous mode\n");
+			modeset_update_crtc_power_domains(state, false);
 			drm_atomic_state_free(state);
 		}
 	} else {
+		modeset_update_crtc_power_domains(state, false);
 		drm_atomic_state_free(state);
 	}
 
+	intel_display_set_init_power(dev_priv, false);
+
 	intel_modeset_check_state(dev);
 }
 

Comments

On Mon, Jun 01, 2015 at 03:27:07PM +0200, Maarten Lankhorst wrote:
> Use for_each_crtc_state to only touch affected crtc's.
> In order to make sure that the initial power is still set
> correctly we make sure modeset_update_crtc_power_domains is called
> during the initial modeset.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c      |  3 ---
>  drivers/gpu/drm/i915/intel_display.c | 41 +++++++++++++++++++++++-------------
>  2 files changed, 26 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index d3632c56fdf7..78ef0bb53c36 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -634,9 +634,6 @@ static int i915_drm_suspend(struct drm_device *dev)
>  	intel_display_suspend(dev);
>  	drm_modeset_unlock_all(dev);
>  
> -	/* suspending displays will unsets init power */
> -	intel_display_set_init_power(dev_priv, true);
> -
>  	intel_dp_mst_suspend(dev);
>  
>  	intel_runtime_pm_disable_interrupts(dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8e9afc55c284..4dc07602248b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5188,42 +5188,49 @@ static unsigned long get_crtc_power_domains(struct drm_crtc *crtc)
>  	return mask;
>  }
>  
> -static void modeset_update_crtc_power_domains(struct drm_atomic_state *state)
> +static void modeset_update_crtc_power_domains(struct drm_atomic_state *state, bool gr)

What does 'gr' stand for and what does the parameter signify?  It seems
to just gate whether we call display.modeset_global_resources, but it's
unclear to me from the commit message above in which situations we
would/wouldn't want to do this and why.


Matt

>  {
>  	struct drm_device *dev = state->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	unsigned long pipe_domains[I915_MAX_PIPES] = { 0, };
> -	struct intel_crtc *crtc;
> +	unsigned long pipe_domains[I915_MAX_PIPES] = { 0, }, domains;
> +	struct drm_crtc_state *crtc_state;
> +	struct drm_crtc *crtc;
> +	int i;
>  
>  	/*
>  	 * First get all needed power domains, then put all unneeded, to avoid
>  	 * any unnecessary toggling of the power wells.
>  	 */
> -	for_each_intel_crtc(dev, crtc) {
> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>  		enum intel_display_power_domain domain;
> +		enum pipe pipe = to_intel_crtc(crtc)->pipe;
>  
> -		if (!crtc->base.state->enable)
> +		if (!crtc->state->active)
>  			continue;
>  
> -		pipe_domains[crtc->pipe] = get_crtc_power_domains(&crtc->base);
> +		domains = pipe_domains[pipe] = get_crtc_power_domains(crtc);
> +		domains &= ~to_intel_crtc(crtc)->enabled_power_domains;
>  
> -		for_each_power_domain(domain, pipe_domains[crtc->pipe])
> +		for_each_power_domain(domain, domains)
>  			intel_display_power_get(dev_priv, domain);
>  	}
>  
> -	if (dev_priv->display.modeset_global_resources)
> +	if (gr && dev_priv->display.modeset_global_resources)
>  		dev_priv->display.modeset_global_resources(state);
>  
> -	for_each_intel_crtc(dev, crtc) {
> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +		enum pipe pipe = intel_crtc->pipe;
>  		enum intel_display_power_domain domain;
>  
> -		for_each_power_domain(domain, crtc->enabled_power_domains)
> -			intel_display_power_put(dev_priv, domain);
> +		domains = intel_crtc->enabled_power_domains;
> +		domains &= ~pipe_domains[pipe];
>  
> -		crtc->enabled_power_domains = pipe_domains[crtc->pipe];
> -	}
> +		intel_crtc->enabled_power_domains = pipe_domains[pipe];
>  
> -	intel_display_set_init_power(dev_priv, false);
> +		for_each_power_domain(domain, domains)
> +			intel_display_power_put(dev_priv, domain);
> +	}
>  }
>  
>  void broxton_set_cdclk(struct drm_device *dev, int frequency)
> @@ -12698,7 +12705,7 @@ static int __intel_set_mode(struct drm_atomic_state *state)
>  	/* The state has been swaped above, so state actually contains the
>  	 * old state now. */
>  
> -	modeset_update_crtc_power_domains(state);
> +	modeset_update_crtc_power_domains(state, true);
>  
>  	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> @@ -15280,12 +15287,16 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>  		ret = intel_set_mode(state);
>  		if (ret) {
>  			DRM_ERROR("Failed to restore previous mode\n");
> +			modeset_update_crtc_power_domains(state, false);
>  			drm_atomic_state_free(state);
>  		}
>  	} else {
> +		modeset_update_crtc_power_domains(state, false);
>  		drm_atomic_state_free(state);
>  	}
>  
> +	intel_display_set_init_power(dev_priv, false);
> +
>  	intel_modeset_check_state(dev);
>  }
>  
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 03-06-15 om 03:27 schreef Matt Roper:
> On Mon, Jun 01, 2015 at 03:27:07PM +0200, Maarten Lankhorst wrote:
>> Use for_each_crtc_state to only touch affected crtc's.
>> In order to make sure that the initial power is still set
>> correctly we make sure modeset_update_crtc_power_domains is called
>> during the initial modeset.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.c      |  3 ---
>>  drivers/gpu/drm/i915/intel_display.c | 41 +++++++++++++++++++++++-------------
>>  2 files changed, 26 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index d3632c56fdf7..78ef0bb53c36 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -634,9 +634,6 @@ static int i915_drm_suspend(struct drm_device *dev)
>>  	intel_display_suspend(dev);
>>  	drm_modeset_unlock_all(dev);
>>  
>> -	/* suspending displays will unsets init power */
>> -	intel_display_set_init_power(dev_priv, true);
>> -
>>  	intel_dp_mst_suspend(dev);
>>  
>>  	intel_runtime_pm_disable_interrupts(dev_priv);
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 8e9afc55c284..4dc07602248b 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -5188,42 +5188,49 @@ static unsigned long get_crtc_power_domains(struct drm_crtc *crtc)
>>  	return mask;
>>  }
>>  
>> -static void modeset_update_crtc_power_domains(struct drm_atomic_state *state)
>> +static void modeset_update_crtc_power_domains(struct drm_atomic_state *state, bool gr)
> What does 'gr' stand for and what does the parameter signify?  It seems
> to just gate whether we call display.modeset_global_resources, but it's
> unclear to me from the commit message above in which situations we
> would/wouldn't want to do this and why.
>
Well there's no point if no modeset is done to call display.modeset_global_resources. But I guess
calling it power_only might be better. I wish I knew why modeset_global_resources was done in the middle,
I think there's no point to do so.

But I'll split it up in 2 functions, modeset_get_crtc_power_domains and modeset_put_crtc_power_domains,
so I can fold it into the crtc modeset loop.

! Maarten
On Wed, Jun 03, 2015 at 08:52:52AM +0200, Maarten Lankhorst wrote:
> Op 03-06-15 om 03:27 schreef Matt Roper:
> > On Mon, Jun 01, 2015 at 03:27:07PM +0200, Maarten Lankhorst wrote:
> >> Use for_each_crtc_state to only touch affected crtc's.
> >> In order to make sure that the initial power is still set
> >> correctly we make sure modeset_update_crtc_power_domains is called
> >> during the initial modeset.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_drv.c      |  3 ---
> >>  drivers/gpu/drm/i915/intel_display.c | 41 +++++++++++++++++++++++-------------
> >>  2 files changed, 26 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> >> index d3632c56fdf7..78ef0bb53c36 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.c
> >> +++ b/drivers/gpu/drm/i915/i915_drv.c
> >> @@ -634,9 +634,6 @@ static int i915_drm_suspend(struct drm_device *dev)
> >>  	intel_display_suspend(dev);
> >>  	drm_modeset_unlock_all(dev);
> >>  
> >> -	/* suspending displays will unsets init power */
> >> -	intel_display_set_init_power(dev_priv, true);
> >> -
> >>  	intel_dp_mst_suspend(dev);
> >>  
> >>  	intel_runtime_pm_disable_interrupts(dev_priv);
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index 8e9afc55c284..4dc07602248b 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -5188,42 +5188,49 @@ static unsigned long get_crtc_power_domains(struct drm_crtc *crtc)
> >>  	return mask;
> >>  }
> >>  
> >> -static void modeset_update_crtc_power_domains(struct drm_atomic_state *state)
> >> +static void modeset_update_crtc_power_domains(struct drm_atomic_state *state, bool gr)
> > What does 'gr' stand for and what does the parameter signify?  It seems
> > to just gate whether we call display.modeset_global_resources, but it's
> > unclear to me from the commit message above in which situations we
> > would/wouldn't want to do this and why.
> >
> Well there's no point if no modeset is done to call display.modeset_global_resources. But I guess
> calling it power_only might be better. I wish I knew why modeset_global_resources was done in the middle,
> I think there's no point to do so.

When doing global changes like updating the cdclk we better do that only
when the hardware is guaranteed to be on. Since we could update global
things both when enabling and when disabling, the only place where the hw
is on in either case is in the middle.
-Daniel
Op 15-06-15 om 13:43 schreef Daniel Vetter:
> On Wed, Jun 03, 2015 at 08:52:52AM +0200, Maarten Lankhorst wrote:
>> Op 03-06-15 om 03:27 schreef Matt Roper:
>>> On Mon, Jun 01, 2015 at 03:27:07PM +0200, Maarten Lankhorst wrote:
>>>> Use for_each_crtc_state to only touch affected crtc's.
>>>> In order to make sure that the initial power is still set
>>>> correctly we make sure modeset_update_crtc_power_domains is called
>>>> during the initial modeset.
>>>>
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> ---
>>>>  drivers/gpu/drm/i915/i915_drv.c      |  3 ---
>>>>  drivers/gpu/drm/i915/intel_display.c | 41 +++++++++++++++++++++++-------------
>>>>  2 files changed, 26 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>>>> index d3632c56fdf7..78ef0bb53c36 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.c
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>>>> @@ -634,9 +634,6 @@ static int i915_drm_suspend(struct drm_device *dev)
>>>>  	intel_display_suspend(dev);
>>>>  	drm_modeset_unlock_all(dev);
>>>>  
>>>> -	/* suspending displays will unsets init power */
>>>> -	intel_display_set_init_power(dev_priv, true);
>>>> -
>>>>  	intel_dp_mst_suspend(dev);
>>>>  
>>>>  	intel_runtime_pm_disable_interrupts(dev_priv);
>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>> index 8e9afc55c284..4dc07602248b 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -5188,42 +5188,49 @@ static unsigned long get_crtc_power_domains(struct drm_crtc *crtc)
>>>>  	return mask;
>>>>  }
>>>>  
>>>> -static void modeset_update_crtc_power_domains(struct drm_atomic_state *state)
>>>> +static void modeset_update_crtc_power_domains(struct drm_atomic_state *state, bool gr)
>>> What does 'gr' stand for and what does the parameter signify?  It seems
>>> to just gate whether we call display.modeset_global_resources, but it's
>>> unclear to me from the commit message above in which situations we
>>> would/wouldn't want to do this and why.
>>>
>> Well there's no point if no modeset is done to call display.modeset_global_resources. But I guess
>> calling it power_only might be better. I wish I knew why modeset_global_resources was done in the middle,
>> I think there's no point to do so.
> When doing global changes like updating the cdclk we better do that only
> when the hardware is guaranteed to be on. Since we could update global
> things both when enabling and when disabling, the only place where the hw
> is on in either case is in the middle.
>
Ok.