diff mbox series

[07/10] drm/i915: Remove crtc->config dereferences in intel_modeset_setup_hw_state

Message ID 20181011100457.8776-8-maarten.lankhorst@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: Remove low hanging crtc->config fruit, part 2. | expand

Commit Message

Maarten Lankhorst Oct. 11, 2018, 10:04 a.m. UTC
The CRTC is idle at this point, so we can dereference crtc->state safely.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Comments

Ville Syrjälä Oct. 11, 2018, 11:59 a.m. UTC | #1
On Thu, Oct 11, 2018 at 12:04:54PM +0200, Maarten Lankhorst wrote:
> The CRTC is idle at this point, so we can dereference crtc->state safely.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 19d7714df021..cbe70bc4d02d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15934,6 +15934,7 @@ intel_modeset_setup_hw_state(struct drm_device *dev,
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_crtc *crtc;
> +	struct intel_crtc_state *crtc_state;

Can be moved into narrower scope.

>  	struct intel_encoder *encoder;
>  	int i;
>  
> @@ -15952,7 +15953,7 @@ intel_modeset_setup_hw_state(struct drm_device *dev,
>  	for_each_intel_crtc(&dev_priv->drm, crtc) {
>  		drm_crtc_vblank_reset(&crtc->base);
>  
> -		if (crtc->active)
> +		if (crtc->base.state->active)
>  			drm_crtc_vblank_on(&crtc->base);
>  	}
>  
> @@ -15961,9 +15962,10 @@ intel_modeset_setup_hw_state(struct drm_device *dev,
>  	for_each_intel_encoder(dev, encoder)
>  		intel_sanitize_encoder(encoder);
>  
> -	for_each_intel_crtc(&dev_priv->drm, crtc) {
> +	for_each_intel_crtc(dev, crtc) {

I'd keep the dev_priv->drm, because we should just change the function
to take the dev_priv directly.

Apart from those
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +		crtc_state = to_intel_crtc_state(crtc->base.state);
>  		intel_sanitize_crtc(crtc, ctx);
> -		intel_dump_pipe_config(crtc, crtc->config,
> +		intel_dump_pipe_config(crtc, crtc_state,
>  				       "[setup_hw_state]");
>  	}
>  
> @@ -15997,7 +15999,8 @@ intel_modeset_setup_hw_state(struct drm_device *dev,
>  	for_each_intel_crtc(dev, crtc) {
>  		u64 put_domains;
>  
> -		put_domains = modeset_get_crtc_power_domains(&crtc->base, crtc->config);
> +		crtc_state = to_intel_crtc_state(crtc->base.state);
> +		put_domains = modeset_get_crtc_power_domains(&crtc->base, crtc_state);
>  		if (WARN_ON(put_domains))
>  			modeset_put_power_domains(dev_priv, put_domains);
>  	}
> -- 
> 2.19.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Maarten Lankhorst Oct. 16, 2018, 1:57 p.m. UTC | #2
Op 11-10-18 om 13:59 schreef Ville Syrjälä:
> On Thu, Oct 11, 2018 at 12:04:54PM +0200, Maarten Lankhorst wrote:
>> The CRTC is idle at this point, so we can dereference crtc->state safely.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 11 +++++++----
>>  1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 19d7714df021..cbe70bc4d02d 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -15934,6 +15934,7 @@ intel_modeset_setup_hw_state(struct drm_device *dev,
>>  {
>>  	struct drm_i915_private *dev_priv = to_i915(dev);
>>  	struct intel_crtc *crtc;
>> +	struct intel_crtc_state *crtc_state;
> Can be moved into narrower scope.
2 places use it, the power domains and the dump_pipe_config, because of this
I kept it as top scope. :)

Thanks, pushed patch 2-9. Hoping to get a reviewer for 1, and will resubmit 10/10 with
a patch doing the proposed intel_wait_for_vblank_if_active() changes.

>>  	struct intel_encoder *encoder;
>>  	int i;
>>  
>> @@ -15952,7 +15953,7 @@ intel_modeset_setup_hw_state(struct drm_device *dev,
>>  	for_each_intel_crtc(&dev_priv->drm, crtc) {
>>  		drm_crtc_vblank_reset(&crtc->base);
>>  
>> -		if (crtc->active)
>> +		if (crtc->base.state->active)
>>  			drm_crtc_vblank_on(&crtc->base);
>>  	}
>>  
>> @@ -15961,9 +15962,10 @@ intel_modeset_setup_hw_state(struct drm_device *dev,
>>  	for_each_intel_encoder(dev, encoder)
>>  		intel_sanitize_encoder(encoder);
>>  
>> -	for_each_intel_crtc(&dev_priv->drm, crtc) {
>> +	for_each_intel_crtc(dev, crtc) {
> I'd keep the dev_priv->drm, because we should just change the function
> to take the dev_priv directly.
>
> Apart from those
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 19d7714df021..cbe70bc4d02d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15934,6 +15934,7 @@  intel_modeset_setup_hw_state(struct drm_device *dev,
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_crtc *crtc;
+	struct intel_crtc_state *crtc_state;
 	struct intel_encoder *encoder;
 	int i;
 
@@ -15952,7 +15953,7 @@  intel_modeset_setup_hw_state(struct drm_device *dev,
 	for_each_intel_crtc(&dev_priv->drm, crtc) {
 		drm_crtc_vblank_reset(&crtc->base);
 
-		if (crtc->active)
+		if (crtc->base.state->active)
 			drm_crtc_vblank_on(&crtc->base);
 	}
 
@@ -15961,9 +15962,10 @@  intel_modeset_setup_hw_state(struct drm_device *dev,
 	for_each_intel_encoder(dev, encoder)
 		intel_sanitize_encoder(encoder);
 
-	for_each_intel_crtc(&dev_priv->drm, crtc) {
+	for_each_intel_crtc(dev, crtc) {
+		crtc_state = to_intel_crtc_state(crtc->base.state);
 		intel_sanitize_crtc(crtc, ctx);
-		intel_dump_pipe_config(crtc, crtc->config,
+		intel_dump_pipe_config(crtc, crtc_state,
 				       "[setup_hw_state]");
 	}
 
@@ -15997,7 +15999,8 @@  intel_modeset_setup_hw_state(struct drm_device *dev,
 	for_each_intel_crtc(dev, crtc) {
 		u64 put_domains;
 
-		put_domains = modeset_get_crtc_power_domains(&crtc->base, crtc->config);
+		crtc_state = to_intel_crtc_state(crtc->base.state);
+		put_domains = modeset_get_crtc_power_domains(&crtc->base, crtc_state);
 		if (WARN_ON(put_domains))
 			modeset_put_power_domains(dev_priv, put_domains);
 	}