Re: [PATCH v3.6 16/22] drm/i915: Implement intel_crtc_control using atomic state, v4.

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 29-05-15 om 02:57 schreef Matt Roper:
> On Tue, May 26, 2015 at 10:35:22AM +0200, Maarten Lankhorst wrote:
>> Assume the callers lock everything with drm_modeset_lock_all.
>>
>> This change had to be done after converting suspend/resume to
>> use atomic_state so the atomic state is preserved, otherwise
>> all transitional state is erased.
>>
>> Now all callers of .crtc_enable and .crtc_disable go through
>> atomic modeset! :-D
>>
>> Changes since v1:
>> - Only check for crtc_state->active in valleyview_modeset_global_pipes.
>> - Only check for crtc_state->active in modeset_update_crtc_power_domains.
>> Changes since v2:
>> - Rework on top of the changed patch order.
>> Changes since v3:
>> - Rename intel_crtc_toggle in description to *_control
>> - Change return value to int.
>> - Do not add plane state, should be done implicitly already.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 53 ++++++++++++++++++++++--------------
>>  drivers/gpu/drm/i915/intel_drv.h     |  2 +-
>>  2 files changed, 33 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index e9680eb85bd0..32bab28432f7 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -5973,38 +5973,49 @@ void intel_display_suspend(struct drm_device *dev)
>>  }
>>  
>>  /* Master function to enable/disable CRTC and corresponding power wells */
>> -void intel_crtc_control(struct drm_crtc *crtc, bool enable)
>> +int intel_crtc_control(struct drm_crtc *crtc, bool enable)
> We change the return value to 'int' here, but we never check the error
> code anywhere.  Maybe we should hold this off for another patch and then
> also propagate the return value back up the call chain?  It seems like
> at least some of the connector-specific DPMS functions should recognize
> the failure of intel_crtc_update_dpms() before trying to continue with
> other operations.
Yeah, but I wasn't sure which ones, and handling is a lot harder. So I wanted to
mark this as 'int' to warn future users that this function may fail so they can check.
>>  {
>>  	struct drm_device *dev = crtc->dev;
>> -	struct drm_i915_private *dev_priv = dev->dev_private;
>> +	struct drm_mode_config *config = &dev->mode_config;
>> +	struct drm_modeset_acquire_ctx *ctx = config->acquire_ctx;
>>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> -	enum intel_display_power_domain domain;
>> -	unsigned long domains;
>> +	struct intel_crtc_state *pipe_config;
>> +	struct drm_atomic_state *state;
>> +	int ret;
>>  
>>  	if (enable == intel_crtc->active)
>> -		return;
>> +		return 0;
>>  
>>  	if (enable && !crtc->state->enable)
>> -		return;
>> +		return 0;
>>  
>> -	crtc->state->active = enable;
>> -	if (enable) {
>> -		domains = get_crtc_power_domains(crtc);
>> -		for_each_power_domain(domain, domains)
>> -			intel_display_power_get(dev_priv, domain);
>> -		intel_crtc->enabled_power_domains = domains;
>> +	/* this function should be called with drm_modeset_lock_all for now */
>> +	if (WARN_ON(!ctx))
>> +		return -EIO;
> EIO seems like an unusual choice here (but I'm not really sure what the
> best option would be).
No idea, I just wanted to make sure that it was recognized as programming error.
-EINVAL Is used everywhere in the atomic path, -ENODEV didn't seem appropriate.
>> +	lockdep_assert_held(&ctx->ww_ctx);
>>  
>> -		dev_priv->display.crtc_enable(crtc);
>> -		intel_crtc_enable_planes(crtc);
>> -	} else {
>> -		intel_crtc_disable_planes(crtc);
>> -		dev_priv->display.crtc_disable(crtc);
>> +	state = drm_atomic_state_alloc(dev);
>> +	if (WARN_ON(!state))
>> +		return -ENOMEM;
>>  
>> -		domains = intel_crtc->enabled_power_domains;
>> -		for_each_power_domain(domain, domains)
>> -			intel_display_power_put(dev_priv, domain);
>> -		intel_crtc->enabled_power_domains = 0;
>> +	state->acquire_ctx = ctx;
>> +	state->allow_modeset = true;
>> +
>> +	pipe_config = intel_atomic_get_crtc_state(state, intel_crtc);
>> +	if (IS_ERR(pipe_config)) {
>> +		ret = PTR_ERR(pipe_config);
>> +		goto err;
>>  	}
>> +	pipe_config->base.active = enable;
>> +
>> +	ret = intel_set_mode(state);
>> +	if (!ret)
>> +		return ret;
>> +
>> +err:
>> +	DRM_ERROR("Updating crtc active failed with %i\n", ret);
>> +	drm_atomic_state_free(state);
>> +	return ret;
>>  }
>>  
>>  /**
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 60a29ff65f1f..c113f187090f 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -989,7 +989,7 @@ void intel_mark_busy(struct drm_device *dev);
>>  void intel_mark_idle(struct drm_device *dev);
>>  void intel_crtc_restore_mode(struct drm_crtc *crtc);
>>  void intel_display_suspend(struct drm_device *dev);
>> -void intel_crtc_control(struct drm_crtc *crtc, bool enable);
>> +int intel_crtc_control(struct drm_crtc *crtc, bool enable);
>>  void intel_crtc_update_dpms(struct drm_crtc *crtc);
>>  void intel_encoder_destroy(struct drm_encoder *encoder);
>>  int intel_connector_init(struct intel_connector *);
>> -- 
>> 2.1.0
>>
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx