Skip to content

Commit

Permalink
drm: don't hold crtc mutexes for connector ->detect callbacks
Browse files Browse the repository at this point in the history
The coup de grace of the entire journey. No more dropped frames every
10s on my testbox!

I've tried to audit all ->detect and ->get_modes callbacks, but things
became a bit fuzzy after trying to piece together the umpteenth
implemenation. Afaict most drivers just have bog-standard output
register frobbing with a notch of i2c edid reading, nothing which
could potentially race with the newly concurrent pageflip/set_cursor
code. The big exception is load-detection code which requires a
running pipe, but radeon/nouveau seem to to this without touching any
state which can be observed from page_flip (e.g. disabled crtcs
temporarily getting enabled and so a pageflip succeeding).

The only special case I could find is the i915 load detect code. That
uses the normal modeset interface to enable the load-detect crtc, and
so userspace could try to squeeze in a pageflip on the load-detect
pipe. So we need to grab the relevant crtc mutex in there, to avoid
the temporary crtc enabling to sneak out and be visible to userspace.

Note that the sysfs files already stopped grabbing the per-crtc locks,
since I didn't want to bother with doing a interruptible
modeset_lock_all. But since there's very little in-between breakage
(essentially just the ability for userspace to pageflip on load-detect
crtcs when it shouldn't on the i915 driver) I figured I don't need to
bother.

Reviewed-by: Rob Clark <rob@ti.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
  • Loading branch information
Daniel Vetter committed Jan 20, 2013
1 parent b4d5e7d commit 7b24056
Show file tree
Hide file tree
Showing 3 changed files with 15 additions and 8 deletions.
5 changes: 3 additions & 2 deletions drivers/gpu/drm/drm_crtc.c
Original file line number Diff line number Diff line change
Expand Up @@ -1617,7 +1617,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,

DRM_DEBUG_KMS("[CONNECTOR:%d:?]\n", out_resp->connector_id);

drm_modeset_lock_all(dev);
mutex_lock(&dev->mode_config.mutex);

obj = drm_mode_object_find(dev, out_resp->connector_id,
DRM_MODE_OBJECT_CONNECTOR);
Expand Down Expand Up @@ -1714,7 +1714,8 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
out_resp->count_encoders = encoders_count;

out:
drm_modeset_unlock_all(dev);
mutex_unlock(&dev->mode_config.mutex);

return ret;
}

Expand Down
8 changes: 4 additions & 4 deletions drivers/gpu/drm/drm_crtc_helper.c
Original file line number Diff line number Diff line change
Expand Up @@ -980,7 +980,7 @@ static void output_poll_execute(struct work_struct *work)
if (!drm_kms_helper_poll)
return;

drm_modeset_lock_all(dev);
mutex_lock(&dev->mode_config.mutex);
list_for_each_entry(connector, &dev->mode_config.connector_list, head) {

/* Ignore forced connectors. */
Expand Down Expand Up @@ -1010,7 +1010,7 @@ static void output_poll_execute(struct work_struct *work)
changed = true;
}

drm_modeset_unlock_all(dev);
mutex_unlock(&dev->mode_config.mutex);

if (changed)
drm_kms_helper_hotplug_event(dev);
Expand Down Expand Up @@ -1070,7 +1070,7 @@ void drm_helper_hpd_irq_event(struct drm_device *dev)
if (!dev->mode_config.poll_enabled)
return;

drm_modeset_lock_all(dev);
mutex_lock(&dev->mode_config.mutex);
list_for_each_entry(connector, &dev->mode_config.connector_list, head) {

/* Only handle HPD capable connectors. */
Expand All @@ -1088,7 +1088,7 @@ void drm_helper_hpd_irq_event(struct drm_device *dev)
changed = true;
}

drm_modeset_unlock_all(dev);
mutex_unlock(&dev->mode_config.mutex);

if (changed)
drm_kms_helper_hotplug_event(dev);
Expand Down
10 changes: 8 additions & 2 deletions drivers/gpu/drm/i915/intel_display.c
Original file line number Diff line number Diff line change
Expand Up @@ -6700,6 +6700,8 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector,
if (encoder->crtc) {
crtc = encoder->crtc;

mutex_lock(&crtc->mutex);

old->dpms_mode = connector->dpms;
old->load_detect_temp = false;

Expand Down Expand Up @@ -6729,6 +6731,7 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector,
return false;
}

mutex_lock(&crtc->mutex);
intel_encoder->new_crtc = to_intel_crtc(crtc);
to_intel_connector(connector)->new_encoder = intel_encoder;

Expand Down Expand Up @@ -6756,13 +6759,15 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector,
DRM_DEBUG_KMS("reusing fbdev for load-detection framebuffer\n");
if (IS_ERR(fb)) {
DRM_DEBUG_KMS("failed to allocate framebuffer for load-detection\n");
mutex_unlock(&crtc->mutex);
return false;
}

if (!intel_set_mode(crtc, mode, 0, 0, fb)) {
DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n");
if (old->release_fb)
old->release_fb->funcs->destroy(old->release_fb);
mutex_unlock(&crtc->mutex);
return false;
}

Expand All @@ -6777,14 +6782,13 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
struct intel_encoder *intel_encoder =
intel_attached_encoder(connector);
struct drm_encoder *encoder = &intel_encoder->base;
struct drm_crtc *crtc = encoder->crtc;

DRM_DEBUG_KMS("[CONNECTOR:%d:%s], [ENCODER:%d:%s]\n",
connector->base.id, drm_get_connector_name(connector),
encoder->base.id, drm_get_encoder_name(encoder));

if (old->load_detect_temp) {
struct drm_crtc *crtc = encoder->crtc;

to_intel_connector(connector)->new_encoder = NULL;
intel_encoder->new_crtc = NULL;
intel_set_mode(crtc, NULL, 0, 0, NULL);
Expand All @@ -6800,6 +6804,8 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
/* Switch crtc and encoder back off if necessary */
if (old->dpms_mode != DRM_MODE_DPMS_ON)
connector->funcs->dpms(connector, old->dpms_mode);

mutex_unlock(&crtc->mutex);
}

/* Returns the clock of the currently programmed mode of the given pipe. */
Expand Down

0 comments on commit 7b24056

Please sign in to comment.