Reviewed-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
(resent with correct smtp)
On Fri, Nov 29, 2013 at 10:38 AM, Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> wrote:
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
>
> On Wed, Nov 27, 2013 at 06:01:19PM -0200, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
>>
>> Currently, PC8 is enabled at modeset_global_resources, which is called
>> after intel_modeset_update_state. Due to this, there's a small race
>> condition on the case where we start enabling PC8, then do a modeset
>> while PC8 is still being enabled. The racing condition triggers a WARN
>> because intel_modeset_update_state will mark the CRTC as enabled, then
>> the thread that's still enabling PC8 might look at the data structure
>> and think that PC8 is being enabled while a pipe is enabled. Despite
>> the WARN, this is not really a bug since we'll wait for the
>> PC8-enabling thread to finish when we call modeset_global_resources.
>>
>> So this patch makes sure we get/put PC8 before we update
>> drm_crtc->enabled, because if a get() call triggers a PC8 disable,
>> we'll call cancel_delayed_work_sync(), which will wait for the thread
>> that's enabling PC8, then, after this, we'll disable PC8.
>>
>> The side-effect benefit of this patch is that we have a nice place to
>> track enabled/disabled CRTCs, so we may want to move some code from
>> modeset_global_resources to intel_crtc_set_enabled in the future.
>>
>> The problem fixed by this patch can be reproduced by the
>> modeset-lpsp-stress-no-wait subtest from the pc8 test of
>> intel-gpu-tools.
>>
>> v2: - No need for pc8.lock since we already have
>> cancel_delayed_work_sync().
>> v3: - Rename to intel_crtc_set_enabled
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
>> ---
>> drivers/gpu/drm/i915/intel_display.c | 27 +++++++++++++++++++++++----
>> 1 file changed, 23 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 0eb7053..5adf540 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -9129,6 +9129,24 @@ static bool intel_crtc_in_use(struct drm_crtc *crtc)
>> return false;
>> }
>>
>> +/* Sets crtc->base.enabled and gets/puts whatever resources are needed by the
>> + * CRTC. */
>> +static void intel_crtc_set_enabled(struct intel_crtc *crtc, bool enabled)
>> +{
>> + struct drm_device *dev = crtc->base.dev;
>> + struct drm_i915_private *dev_priv = dev->dev_private;
>> +
>> + if (enabled == crtc->base.enabled)
>> + return;
>> +
>> + if (enabled)
>> + hsw_disable_package_c8(dev_priv);
>> + else
>> + hsw_enable_package_c8(dev_priv);
>> +
>> + crtc->base.enabled = enabled;
>> +}
>> +
>> static void
>> intel_modeset_update_state(struct drm_device *dev, unsigned prepare_pipes)
>> {
>> @@ -9152,7 +9170,8 @@ intel_modeset_update_state(struct drm_device *dev, unsigned prepare_pipes)
>> /* Update computed state. */
>> list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list,
>> base.head) {
>> - intel_crtc->base.enabled = intel_crtc_in_use(&intel_crtc->base);
>> + intel_crtc_set_enabled(intel_crtc,
>> + intel_crtc_in_use(&intel_crtc->base));
>> }
>>
>> list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
>> @@ -10961,7 +10980,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>> }
>>
>> WARN_ON(crtc->active);
>> - crtc->base.enabled = false;
>> + intel_crtc_set_enabled(crtc, false);
>> }
>>
>> if (dev_priv->quirks & QUIRK_PIPEA_FORCE &&
>> @@ -10988,7 +11007,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>> crtc->base.enabled ? "enabled" : "disabled",
>> crtc->active ? "enabled" : "disabled");
>>
>> - crtc->base.enabled = crtc->active;
>> + intel_crtc_set_enabled(crtc, crtc->active);
>>
>> /* Because we only establish the connector -> encoder ->
>> * crtc links if something is active, this means the
>> @@ -11085,7 +11104,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>> crtc->active = dev_priv->display.get_pipe_config(crtc,
>> &crtc->config);
>>
>> - crtc->base.enabled = crtc->active;
>> + intel_crtc_set_enabled(crtc, crtc->active);
>> crtc->primary_enabled = crtc->active;
>>
>> DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n",
>> --
>> 1.8.3.1
>>
>> _______________________________________________
>> 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
--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
