| Message ID | 20161213230814.19598-10-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Dec 13, 2016 at 6:08 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > - Modeset state needs mode_config->connection mutex, that covers > figuring out the encoder, and reading properties (since in the > atomic case those need to look at connector->state). > > - Don't hold any locks for stuff that's invariant (i.e. possible > connectors). > > - Same for connector lookup and unref, those don't need any locks. > > - And finally the probe stuff is only protected by mode_config->mutex. > > While at it updated the kerneldoc for these fields in drm_connector > and add docs explaining what's protected by which locks. > Reviewed-by: Sean Paul <seanpaul@chromium.org> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/gpu/drm/drm_connector.c | 92 ++++++++++++++++++++--------------------- > include/drm/drm_connector.h | 23 +++++++++-- > 2 files changed, 63 insertions(+), 52 deletions(-) > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > index 0d4728704099..44b556d5d40c 100644 > --- a/drivers/gpu/drm/drm_connector.c > +++ b/drivers/gpu/drm/drm_connector.c > @@ -1160,43 +1160,65 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, > > memset(&u_mode, 0, sizeof(struct drm_mode_modeinfo)); > > - mutex_lock(&dev->mode_config.mutex); > - > connector = drm_connector_lookup(dev, out_resp->connector_id); > - if (!connector) { > - ret = -ENOENT; > - goto out_unlock; > - } > + if (!connector) > + return -ENOENT; > + > + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > + encoder = drm_connector_get_encoder(connector); > + if (encoder) > + out_resp->encoder_id = encoder->base.id; > + else > + out_resp->encoder_id = 0; > + > + ret = drm_mode_object_get_properties(&connector->base, file_priv->atomic, > + (uint32_t __user *)(unsigned long)(out_resp->props_ptr), > + (uint64_t __user *)(unsigned long)(out_resp->prop_values_ptr), > + &out_resp->count_props); > + drm_modeset_unlock(&dev->mode_config.connection_mutex); > + if (ret) > + goto out_unref; > > for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) > if (connector->encoder_ids[i] != 0) > encoders_count++; > > + if ((out_resp->count_encoders >= encoders_count) && encoders_count) { > + copied = 0; > + encoder_ptr = (uint32_t __user *)(unsigned long)(out_resp->encoders_ptr); > + for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) { > + if (connector->encoder_ids[i] != 0) { > + if (put_user(connector->encoder_ids[i], > + encoder_ptr + copied)) { > + ret = -EFAULT; > + goto out_unref; > + } > + copied++; > + } > + } > + } > + out_resp->count_encoders = encoders_count; > + > + out_resp->connector_id = connector->base.id; > + out_resp->connector_type = connector->connector_type; > + out_resp->connector_type_id = connector->connector_type_id; > + > + mutex_lock(&dev->mode_config.mutex); > if (out_resp->count_modes == 0) { > connector->funcs->fill_modes(connector, > dev->mode_config.max_width, > dev->mode_config.max_height); > } > > - /* delayed so we get modes regardless of pre-fill_modes state */ > - list_for_each_entry(mode, &connector->modes, head) > - if (drm_mode_expose_to_userspace(mode, file_priv)) > - mode_count++; > - > - out_resp->connector_id = connector->base.id; > - out_resp->connector_type = connector->connector_type; > - out_resp->connector_type_id = connector->connector_type_id; > out_resp->mm_width = connector->display_info.width_mm; > out_resp->mm_height = connector->display_info.height_mm; > out_resp->subpixel = connector->display_info.subpixel_order; > out_resp->connection = connector->status; > > - drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > - encoder = drm_connector_get_encoder(connector); > - if (encoder) > - out_resp->encoder_id = encoder->base.id; > - else > - out_resp->encoder_id = 0; > + /* delayed so we get modes regardless of pre-fill_modes state */ > + list_for_each_entry(mode, &connector->modes, head) > + if (drm_mode_expose_to_userspace(mode, file_priv)) > + mode_count++; > > /* > * This ioctl is called twice, once to determine how much space is > @@ -1219,36 +1241,10 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, > } > } > out_resp->count_modes = mode_count; > - > - ret = drm_mode_object_get_properties(&connector->base, file_priv->atomic, > - (uint32_t __user *)(unsigned long)(out_resp->props_ptr), > - (uint64_t __user *)(unsigned long)(out_resp->prop_values_ptr), > - &out_resp->count_props); > - if (ret) > - goto out; > - > - if ((out_resp->count_encoders >= encoders_count) && encoders_count) { > - copied = 0; > - encoder_ptr = (uint32_t __user *)(unsigned long)(out_resp->encoders_ptr); > - for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) { > - if (connector->encoder_ids[i] != 0) { > - if (put_user(connector->encoder_ids[i], > - encoder_ptr + copied)) { > - ret = -EFAULT; > - goto out; > - } > - copied++; > - } > - } > - } > - out_resp->count_encoders = encoders_count; > - > out: > - drm_modeset_unlock(&dev->mode_config.connection_mutex); > - > - drm_connector_unreference(connector); > -out_unlock: > mutex_unlock(&dev->mode_config.mutex); > +out_unref: > + drm_connector_unreference(connector); > > return ret; > } > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > index a24559ef8bb7..9af4141165d3 100644 > --- a/include/drm/drm_connector.h > +++ b/include/drm/drm_connector.h > @@ -559,9 +559,6 @@ struct drm_cmdline_mode { > * @interlace_allowed: can this connector handle interlaced modes? > * @doublescan_allowed: can this connector handle doublescan? > * @stereo_allowed: can this connector handle stereo modes? > - * @modes: modes available on this connector (from fill_modes() + user) > - * @status: one of the drm_connector_status enums (connected, not, or unknown) > - * @probed_modes: list of modes derived directly from the display > * @funcs: connector control functions > * @edid_blob_ptr: DRM property containing EDID if present > * @properties: property tracking for this connector > @@ -631,11 +628,27 @@ struct drm_connector { > * Protected by @mutex. > */ > bool registered; > + > + /** > + * @modes: > + * Modes available on this connector (from fill_modes() + user). > + * Protected by dev->mode_config.mutex. > + */ > struct list_head modes; /* list of modes on this connector */ > > + /** > + * @status: > + * One of the drm_connector_status enums (connected, not, or unknown). > + * Protected by dev->mode_config.mutex. > + */ > enum drm_connector_status status; > > - /* these are modes added by probing with DDC or the BIOS */ > + /** > + * @probed_modes: > + * These are modes added by probing with DDC or the BIOS, before > + * filtering is applied. Used by the probe helpers.Protected by > + * dev->mode_config.mutex. > + */ > struct list_head probed_modes; > > /** > @@ -644,6 +657,8 @@ struct drm_connector { > * flat panels in embedded systems, the driver should initialize the > * display_info.width_mm and display_info.height_mm fields with the > * physical size of the display. > + * > + * Protected by dev->mode_config.mutex. > */ > struct drm_display_info display_info; > const struct drm_connector_funcs *funcs; > -- > 2.11.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, Dec 16, 2016 at 10:03:32AM -0500, Sean Paul wrote: > On Tue, Dec 13, 2016 at 6:08 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > - Modeset state needs mode_config->connection mutex, that covers > > figuring out the encoder, and reading properties (since in the > > atomic case those need to look at connector->state). > > > > - Don't hold any locks for stuff that's invariant (i.e. possible > > connectors). > > > > - Same for connector lookup and unref, those don't need any locks. > > > > - And finally the probe stuff is only protected by mode_config->mutex. > > > > While at it updated the kerneldoc for these fields in drm_connector > > and add docs explaining what's protected by which locks. > > > > > Reviewed-by: Sean Paul <seanpaul@chromium.org> Merged up to this patch to drm-misc, thanks for the reviews to everyone. -Daniel > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > --- > > drivers/gpu/drm/drm_connector.c | 92 ++++++++++++++++++++--------------------- > > include/drm/drm_connector.h | 23 +++++++++-- > > 2 files changed, 63 insertions(+), 52 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > > index 0d4728704099..44b556d5d40c 100644 > > --- a/drivers/gpu/drm/drm_connector.c > > +++ b/drivers/gpu/drm/drm_connector.c > > @@ -1160,43 +1160,65 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, > > > > memset(&u_mode, 0, sizeof(struct drm_mode_modeinfo)); > > > > - mutex_lock(&dev->mode_config.mutex); > > - > > connector = drm_connector_lookup(dev, out_resp->connector_id); > > - if (!connector) { > > - ret = -ENOENT; > > - goto out_unlock; > > - } > > + if (!connector) > > + return -ENOENT; > > + > > + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > > + encoder = drm_connector_get_encoder(connector); > > + if (encoder) > > + out_resp->encoder_id = encoder->base.id; > > + else > > + out_resp->encoder_id = 0; > > + > > + ret = drm_mode_object_get_properties(&connector->base, file_priv->atomic, > > + (uint32_t __user *)(unsigned long)(out_resp->props_ptr), > > + (uint64_t __user *)(unsigned long)(out_resp->prop_values_ptr), > > + &out_resp->count_props); > > + drm_modeset_unlock(&dev->mode_config.connection_mutex); > > + if (ret) > > + goto out_unref; > > > > for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) > > if (connector->encoder_ids[i] != 0) > > encoders_count++; > > > > + if ((out_resp->count_encoders >= encoders_count) && encoders_count) { > > + copied = 0; > > + encoder_ptr = (uint32_t __user *)(unsigned long)(out_resp->encoders_ptr); > > + for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) { > > + if (connector->encoder_ids[i] != 0) { > > + if (put_user(connector->encoder_ids[i], > > + encoder_ptr + copied)) { > > + ret = -EFAULT; > > + goto out_unref; > > + } > > + copied++; > > + } > > + } > > + } > > + out_resp->count_encoders = encoders_count; > > + > > + out_resp->connector_id = connector->base.id; > > + out_resp->connector_type = connector->connector_type; > > + out_resp->connector_type_id = connector->connector_type_id; > > + > > + mutex_lock(&dev->mode_config.mutex); > > if (out_resp->count_modes == 0) { > > connector->funcs->fill_modes(connector, > > dev->mode_config.max_width, > > dev->mode_config.max_height); > > } > > > > - /* delayed so we get modes regardless of pre-fill_modes state */ > > - list_for_each_entry(mode, &connector->modes, head) > > - if (drm_mode_expose_to_userspace(mode, file_priv)) > > - mode_count++; > > - > > - out_resp->connector_id = connector->base.id; > > - out_resp->connector_type = connector->connector_type; > > - out_resp->connector_type_id = connector->connector_type_id; > > out_resp->mm_width = connector->display_info.width_mm; > > out_resp->mm_height = connector->display_info.height_mm; > > out_resp->subpixel = connector->display_info.subpixel_order; > > out_resp->connection = connector->status; > > > > - drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > > - encoder = drm_connector_get_encoder(connector); > > - if (encoder) > > - out_resp->encoder_id = encoder->base.id; > > - else > > - out_resp->encoder_id = 0; > > + /* delayed so we get modes regardless of pre-fill_modes state */ > > + list_for_each_entry(mode, &connector->modes, head) > > + if (drm_mode_expose_to_userspace(mode, file_priv)) > > + mode_count++; > > > > /* > > * This ioctl is called twice, once to determine how much space is > > @@ -1219,36 +1241,10 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, > > } > > } > > out_resp->count_modes = mode_count; > > - > > - ret = drm_mode_object_get_properties(&connector->base, file_priv->atomic, > > - (uint32_t __user *)(unsigned long)(out_resp->props_ptr), > > - (uint64_t __user *)(unsigned long)(out_resp->prop_values_ptr), > > - &out_resp->count_props); > > - if (ret) > > - goto out; > > - > > - if ((out_resp->count_encoders >= encoders_count) && encoders_count) { > > - copied = 0; > > - encoder_ptr = (uint32_t __user *)(unsigned long)(out_resp->encoders_ptr); > > - for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) { > > - if (connector->encoder_ids[i] != 0) { > > - if (put_user(connector->encoder_ids[i], > > - encoder_ptr + copied)) { > > - ret = -EFAULT; > > - goto out; > > - } > > - copied++; > > - } > > - } > > - } > > - out_resp->count_encoders = encoders_count; > > - > > out: > > - drm_modeset_unlock(&dev->mode_config.connection_mutex); > > - > > - drm_connector_unreference(connector); > > -out_unlock: > > mutex_unlock(&dev->mode_config.mutex); > > +out_unref: > > + drm_connector_unreference(connector); > > > > return ret; > > } > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > > index a24559ef8bb7..9af4141165d3 100644 > > --- a/include/drm/drm_connector.h > > +++ b/include/drm/drm_connector.h > > @@ -559,9 +559,6 @@ struct drm_cmdline_mode { > > * @interlace_allowed: can this connector handle interlaced modes? > > * @doublescan_allowed: can this connector handle doublescan? > > * @stereo_allowed: can this connector handle stereo modes? > > - * @modes: modes available on this connector (from fill_modes() + user) > > - * @status: one of the drm_connector_status enums (connected, not, or unknown) > > - * @probed_modes: list of modes derived directly from the display > > * @funcs: connector control functions > > * @edid_blob_ptr: DRM property containing EDID if present > > * @properties: property tracking for this connector > > @@ -631,11 +628,27 @@ struct drm_connector { > > * Protected by @mutex. > > */ > > bool registered; > > + > > + /** > > + * @modes: > > + * Modes available on this connector (from fill_modes() + user). > > + * Protected by dev->mode_config.mutex. > > + */ > > struct list_head modes; /* list of modes on this connector */ > > > > + /** > > + * @status: > > + * One of the drm_connector_status enums (connected, not, or unknown). > > + * Protected by dev->mode_config.mutex. > > + */ > > enum drm_connector_status status; > > > > - /* these are modes added by probing with DDC or the BIOS */ > > + /** > > + * @probed_modes: > > + * These are modes added by probing with DDC or the BIOS, before > > + * filtering is applied. Used by the probe helpers.Protected by > > + * dev->mode_config.mutex. > > + */ > > struct list_head probed_modes; > > > > /** > > @@ -644,6 +657,8 @@ struct drm_connector { > > * flat panels in embedded systems, the driver should initialize the > > * display_info.width_mm and display_info.height_mm fields with the > > * physical size of the display. > > + * > > + * Protected by dev->mode_config.mutex. > > */ > > struct drm_display_info display_info; > > const struct drm_connector_funcs *funcs; > > -- > > 2.11.0 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > -- > Sean Paul, Software Engineer, Google / Chromium OS
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index 0d4728704099..44b556d5d40c 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -1160,43 +1160,65 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, memset(&u_mode, 0, sizeof(struct drm_mode_modeinfo)); - mutex_lock(&dev->mode_config.mutex); - connector = drm_connector_lookup(dev, out_resp->connector_id); - if (!connector) { - ret = -ENOENT; - goto out_unlock; - } + if (!connector) + return -ENOENT; + + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); + encoder = drm_connector_get_encoder(connector); + if (encoder) + out_resp->encoder_id = encoder->base.id; + else + out_resp->encoder_id = 0; + + ret = drm_mode_object_get_properties(&connector->base, file_priv->atomic, + (uint32_t __user *)(unsigned long)(out_resp->props_ptr), + (uint64_t __user *)(unsigned long)(out_resp->prop_values_ptr), + &out_resp->count_props); + drm_modeset_unlock(&dev->mode_config.connection_mutex); + if (ret) + goto out_unref; for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) if (connector->encoder_ids[i] != 0) encoders_count++; + if ((out_resp->count_encoders >= encoders_count) && encoders_count) { + copied = 0; + encoder_ptr = (uint32_t __user *)(unsigned long)(out_resp->encoders_ptr); + for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) { + if (connector->encoder_ids[i] != 0) { + if (put_user(connector->encoder_ids[i], + encoder_ptr + copied)) { + ret = -EFAULT; + goto out_unref; + } + copied++; + } + } + } + out_resp->count_encoders = encoders_count; + + out_resp->connector_id = connector->base.id; + out_resp->connector_type = connector->connector_type; + out_resp->connector_type_id = connector->connector_type_id; + + mutex_lock(&dev->mode_config.mutex); if (out_resp->count_modes == 0) { connector->funcs->fill_modes(connector, dev->mode_config.max_width, dev->mode_config.max_height); } - /* delayed so we get modes regardless of pre-fill_modes state */ - list_for_each_entry(mode, &connector->modes, head) - if (drm_mode_expose_to_userspace(mode, file_priv)) - mode_count++; - - out_resp->connector_id = connector->base.id; - out_resp->connector_type = connector->connector_type; - out_resp->connector_type_id = connector->connector_type_id; out_resp->mm_width = connector->display_info.width_mm; out_resp->mm_height = connector->display_info.height_mm; out_resp->subpixel = connector->display_info.subpixel_order; out_resp->connection = connector->status; - drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); - encoder = drm_connector_get_encoder(connector); - if (encoder) - out_resp->encoder_id = encoder->base.id; - else - out_resp->encoder_id = 0; + /* delayed so we get modes regardless of pre-fill_modes state */ + list_for_each_entry(mode, &connector->modes, head) + if (drm_mode_expose_to_userspace(mode, file_priv)) + mode_count++; /* * This ioctl is called twice, once to determine how much space is @@ -1219,36 +1241,10 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, } } out_resp->count_modes = mode_count; - - ret = drm_mode_object_get_properties(&connector->base, file_priv->atomic, - (uint32_t __user *)(unsigned long)(out_resp->props_ptr), - (uint64_t __user *)(unsigned long)(out_resp->prop_values_ptr), - &out_resp->count_props); - if (ret) - goto out; - - if ((out_resp->count_encoders >= encoders_count) && encoders_count) { - copied = 0; - encoder_ptr = (uint32_t __user *)(unsigned long)(out_resp->encoders_ptr); - for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) { - if (connector->encoder_ids[i] != 0) { - if (put_user(connector->encoder_ids[i], - encoder_ptr + copied)) { - ret = -EFAULT; - goto out; - } - copied++; - } - } - } - out_resp->count_encoders = encoders_count; - out: - drm_modeset_unlock(&dev->mode_config.connection_mutex); - - drm_connector_unreference(connector); -out_unlock: mutex_unlock(&dev->mode_config.mutex); +out_unref: + drm_connector_unreference(connector); return ret; } diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index a24559ef8bb7..9af4141165d3 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -559,9 +559,6 @@ struct drm_cmdline_mode { * @interlace_allowed: can this connector handle interlaced modes? * @doublescan_allowed: can this connector handle doublescan? * @stereo_allowed: can this connector handle stereo modes? - * @modes: modes available on this connector (from fill_modes() + user) - * @status: one of the drm_connector_status enums (connected, not, or unknown) - * @probed_modes: list of modes derived directly from the display * @funcs: connector control functions * @edid_blob_ptr: DRM property containing EDID if present * @properties: property tracking for this connector @@ -631,11 +628,27 @@ struct drm_connector { * Protected by @mutex. */ bool registered; + + /** + * @modes: + * Modes available on this connector (from fill_modes() + user). + * Protected by dev->mode_config.mutex. + */ struct list_head modes; /* list of modes on this connector */ + /** + * @status: + * One of the drm_connector_status enums (connected, not, or unknown). + * Protected by dev->mode_config.mutex. + */ enum drm_connector_status status; - /* these are modes added by probing with DDC or the BIOS */ + /** + * @probed_modes: + * These are modes added by probing with DDC or the BIOS, before + * filtering is applied. Used by the probe helpers.Protected by + * dev->mode_config.mutex. + */ struct list_head probed_modes; /** @@ -644,6 +657,8 @@ struct drm_connector { * flat panels in embedded systems, the driver should initialize the * display_info.width_mm and display_info.height_mm fields with the * physical size of the display. + * + * Protected by dev->mode_config.mutex. */ struct drm_display_info display_info; const struct drm_connector_funcs *funcs;
- Modeset state needs mode_config->connection mutex, that covers figuring out the encoder, and reading properties (since in the atomic case those need to look at connector->state). - Don't hold any locks for stuff that's invariant (i.e. possible connectors). - Same for connector lookup and unref, those don't need any locks. - And finally the probe stuff is only protected by mode_config->mutex. While at it updated the kerneldoc for these fields in drm_connector and add docs explaining what's protected by which locks. Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/drm_connector.c | 92 ++++++++++++++++++++--------------------- include/drm/drm_connector.h | 23 +++++++++-- 2 files changed, 63 insertions(+), 52 deletions(-)