On Wed, Feb 17, 2016 at 08:32:05AM +0100, Maarten Lankhorst wrote:
> Because we record connector_mask using 1 << drm_connector_index now
> the connector_mask should stay the same even when other connectors
> are removed. This was not the case with MST, in that case when removing
> a connector all other connectors may change their index.
>
> This is fixed by waiting until the first get_connector_state to allocate
> connector_state, and force reallocation when state is too small.
>
> As a side effect connector arrays no longer have to be preallocated,
> and can be allocated on first use which means a less allocations in
> the page flip only path.
>
> Changes since v1:
> - Whitespace. (Ville)
> - Call ida_remove when destroying the connector. (Ville)
> - u32 alloc -> int. (Ville)
>
> Fixes: 14de6c44d149 ("drm/atomic: Remove drm_atomic_connectors_for_crtc.")
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/drm_atomic.c | 44 +++++++++++++++------------------
> drivers/gpu/drm/drm_atomic_helper.c | 2 +-
> drivers/gpu/drm/drm_crtc.c | 49 +++++++++++++++----------------------
> include/drm/drm_crtc.h | 8 +++++-
> 4 files changed, 48 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 8fb469c4e4b8..092620c6ff32 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -65,8 +65,6 @@ drm_atomic_state_init(struct drm_device *dev, struct drm_atomic_state *state)
> */
> state->allow_modeset = true;
>
> - state->num_connector = ACCESS_ONCE(dev->mode_config.num_connector);
> -
> state->crtcs = kcalloc(dev->mode_config.num_crtc,
> sizeof(*state->crtcs), GFP_KERNEL);
> if (!state->crtcs)
> @@ -83,16 +81,6 @@ drm_atomic_state_init(struct drm_device *dev, struct drm_atomic_state *state)
> sizeof(*state->plane_states), GFP_KERNEL);
> if (!state->plane_states)
> goto fail;
> - state->connectors = kcalloc(state->num_connector,
> - sizeof(*state->connectors),
> - GFP_KERNEL);
> - if (!state->connectors)
> - goto fail;
> - state->connector_states = kcalloc(state->num_connector,
> - sizeof(*state->connector_states),
> - GFP_KERNEL);
> - if (!state->connector_states)
> - goto fail;
>
> state->dev = dev;
>
> @@ -823,19 +811,27 @@ drm_atomic_get_connector_state(struct drm_atomic_state *state,
>
> index = drm_connector_index(connector);
>
> - /*
> - * Construction of atomic state updates can race with a connector
> - * hot-add which might overflow. In this case flip the table and just
> - * restart the entire ioctl - no one is fast enough to livelock a cpu
> - * with physical hotplug events anyway.
> - *
> - * Note that we only grab the indexes once we have the right lock to
> - * prevent hotplug/unplugging of connectors. So removal is no problem,
> - * at most the array is a bit too large.
> - */
> if (index >= state->num_connector) {
> - DRM_DEBUG_ATOMIC("Hot-added connector would overflow state array, restarting\n");
> - return ERR_PTR(-EAGAIN);
> + struct drm_connector **c;
> + struct drm_connector_state **cs;
> + int alloc = max(index + 1, config->num_connector);
> +
> + c = krealloc(state->connectors, alloc * sizeof(*state->connectors), GFP_KERNEL);
> + if (!c)
> + return ERR_PTR(-ENOMEM);
> +
> + state->connectors = c;
> + memset(&state->connectors[state->num_connector], 0,
> + sizeof(*state->connectors) * (alloc - state->num_connector));
> +
> + cs = krealloc(state->connector_states, alloc * sizeof(*state->connector_states), GFP_KERNEL);
> + if (!cs)
> + return ERR_PTR(-ENOMEM);
> +
> + state->connector_states = cs;
> + memset(&state->connector_states[state->num_connector], 0,
> + sizeof(*state->connector_states) * (alloc - state->num_connector));
> + state->num_connector = alloc;
> }
>
> if (state->connector_states[index])
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 2b430b05f35d..4da4f2a49078 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1535,7 +1535,7 @@ void drm_atomic_helper_swap_state(struct drm_device *dev,
> {
> int i;
>
> - for (i = 0; i < dev->mode_config.num_connector; i++) {
> + for (i = 0; i < state->num_connector; i++) {
> struct drm_connector *connector = state->connectors[i];
>
> if (!connector)
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 65258acddb90..84514001dcef 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -918,12 +918,19 @@ int drm_connector_init(struct drm_device *dev,
> connector->base.properties = &connector->properties;
> connector->dev = dev;
> connector->funcs = funcs;
> +
> + connector->connector_id = ida_simple_get(&config->connector_ida, 0, 0, GFP_KERNEL);
> + if (connector->connector_id < 0) {
> + ret = connector->connector_id;
> + goto out_put;
> + }
> +
> connector->connector_type = connector_type;
> connector->connector_type_id =
> ida_simple_get(connector_ida, 1, 0, GFP_KERNEL);
> if (connector->connector_type_id < 0) {
> ret = connector->connector_type_id;
> - goto out_put;
> + goto out_put_id;
> }
> connector->name =
> kasprintf(GFP_KERNEL, "%s-%d",
> @@ -931,7 +938,7 @@ int drm_connector_init(struct drm_device *dev,
> connector->connector_type_id);
> if (!connector->name) {
> ret = -ENOMEM;
> - goto out_put;
> + goto out_put_type_id;
> }
>
> INIT_LIST_HEAD(&connector->probed_modes);
> @@ -959,7 +966,12 @@ int drm_connector_init(struct drm_device *dev,
> }
>
> connector->debugfs_entry = NULL;
> -
> +out_put_type_id:
> + if (ret)
> + ida_remove(connector_ida, connector->connector_type_id);
> +out_put_id:
> + if (ret)
> + ida_remove(&config->connector_ida, connector->connector_id);
> out_put:
> if (ret)
> drm_mode_object_put(dev, &connector->base);
That seems to fix a pre-existing connector_type_id leak as well. Could
mention it in the commit message.
Anyways, this seems OK to me. There's some potential for confusion with
connector_id, connector_type_id and connector->base.id perhaps, but I
don't really care so
Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> @@ -996,6 +1008,9 @@ void drm_connector_cleanup(struct drm_connector *connector)
> ida_remove(&drm_connector_enum_list[connector->connector_type].ida,
> connector->connector_type_id);
>
> + ida_remove(&dev->mode_config.connector_ida,
> + connector->connector_id);
> +
> kfree(connector->display_info.bus_formats);
> drm_mode_object_put(dev, &connector->base);
> kfree(connector->name);
> @@ -1013,32 +1028,6 @@ void drm_connector_cleanup(struct drm_connector *connector)
> EXPORT_SYMBOL(drm_connector_cleanup);
>
> /**
> - * drm_connector_index - find the index of a registered connector
> - * @connector: connector to find index for
> - *
> - * Given a registered connector, return the index of that connector within a DRM
> - * device's list of connectors.
> - */
> -unsigned int drm_connector_index(struct drm_connector *connector)
> -{
> - unsigned int index = 0;
> - struct drm_connector *tmp;
> - struct drm_mode_config *config = &connector->dev->mode_config;
> -
> - WARN_ON(!drm_modeset_is_locked(&config->connection_mutex));
> -
> - drm_for_each_connector(tmp, connector->dev) {
> - if (tmp == connector)
> - return index;
> -
> - index++;
> - }
> -
> - BUG();
> -}
> -EXPORT_SYMBOL(drm_connector_index);
> -
> -/**
> * drm_connector_register - register a connector
> * @connector: the connector to register
> *
> @@ -5838,6 +5827,7 @@ void drm_mode_config_init(struct drm_device *dev)
> INIT_LIST_HEAD(&dev->mode_config.plane_list);
> idr_init(&dev->mode_config.crtc_idr);
> idr_init(&dev->mode_config.tile_idr);
> + ida_init(&dev->mode_config.connector_ida);
>
> drm_modeset_lock_all(dev);
> drm_mode_create_standard_properties(dev);
> @@ -5918,6 +5908,7 @@ void drm_mode_config_cleanup(struct drm_device *dev)
> crtc->funcs->destroy(crtc);
> }
>
> + ida_destroy(&dev->mode_config.connector_ida);
> idr_destroy(&dev->mode_config.tile_idr);
> idr_destroy(&dev->mode_config.crtc_idr);
> drm_modeset_lock_fini(&dev->mode_config.connection_mutex);
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 8c7fb3d0f9d0..7fad193dc645 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1168,6 +1168,7 @@ struct drm_connector {
> struct drm_mode_object base;
>
> char *name;
> + int connector_id;
> int connector_type;
> int connector_type_id;
> bool interlace_allowed;
> @@ -2049,6 +2050,7 @@ struct drm_mode_config {
> struct list_head fb_list;
>
> int num_connector;
> + struct ida connector_ida;
> struct list_head connector_list;
> int num_encoder;
> struct list_head encoder_list;
> @@ -2213,7 +2215,11 @@ int drm_connector_register(struct drm_connector *connector);
> void drm_connector_unregister(struct drm_connector *connector);
>
> extern void drm_connector_cleanup(struct drm_connector *connector);
> -extern unsigned int drm_connector_index(struct drm_connector *connector);
> +static inline unsigned drm_connector_index(struct drm_connector *connector)
> +{
> + return connector->connector_id;
> +}
> +
> /* helper to unplug all connectors from sysfs for device */
> extern void drm_connector_unplug_all(struct drm_device *dev);
>
> --
> 2.1.0
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
