On Tue, Dec 13, 2016 at 6:08 PM, Daniel Vetter <daniel.vetter@xxxxxxxx> 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@xxxxxxxxxxxx>
> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
