Re: [PATCH] drm/atomic: Allow for holes in connector state.

On Mon, Feb 15, 2016 at 02:17:01PM +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.

Fixes: 14de6c44d149 ("drm/atomic: Remove drm_atomic_connectors_for_crtc.") Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com

---

drivers/gpu/drm/drm_atomic.c | 45 +++++++++++++++++-------------------- drivers/gpu/drm/drm_atomic_helper.c | 2 +- drivers/gpu/drm/drm_crtc.c | 45 +++++++++++++------------------------ include/drm/drm_crtc.h | 8 ++++++- 4 files changed, 45 insertions(+), 55 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 8fb469c4e4b8..5d4774450540 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,28 @@ 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;
  

• u32 alloc = max(index + 1, config->num_connector);
  

Why u32?

• 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..ea00aea88de7 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -918,12 +918,18 @@ int drm_connector_init(struct drm_device *dev, connector->base.properties = &connector->properties; connector->dev = dev; connector->funcs = funcs;

Could use an empty line here.

• 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 +937,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 +965,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);
  

Should there be an ida_remove() call somewhere when unregistering or destroying the connector?

BTW looking at intel_dp_destroy_mst_connector(), shouldn't we unregister/do something before we do the modeset? What's there to prevent userspac/someone from racing with this and re-enabling the connector before we unregister it? Or maybe the connector is already defunct by this time and something else will prevent any modeset using the connector from succeeding?

out_put: if (ret) drm_mode_object_put(dev, &connector->base); @@ -1013,32 +1024,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 +5823,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 +5904,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

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel