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@xxxxxxxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxxxxxxxx
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
