Hi Noralf. I have read through the cahnes a copuple of times not and feel confident to add my r-b if the comments are considered. On Mon, May 06, 2019 at 08:01:33PM +0200, Noralf Trønnes wrote: > It now only contains the modeset so use that directly instead and attach > a modeset array to drm_client_dev. drm_fb_helper will use this array. > Code will later be moved to drm_client, so add code there in a new file > drm_client_modeset.c with MIT license to match drm_fb_helper.c. The first part of this commit log could use some re-pharsing. What is "It" etc. > @@ -92,14 +91,20 @@ int drm_client_init(struct drm_device *dev, struct drm_client_dev *client, > client->name = name; > client->funcs = funcs; > > - ret = drm_client_open(client); > + ret = drm_client_modeset_create(client); > if (ret) > goto err_put_module; > > + ret = drm_client_open(client); > + if (ret) > + goto err_free; > + > drm_dev_get(dev); > > return 0; > > +err_free: > + drm_client_modeset_free(client); > err_put_module: > if (funcs) > module_put(funcs->owner); > @@ -148,6 +153,7 @@ void drm_client_release(struct drm_client_dev *client) > > DRM_DEV_DEBUG_KMS(dev->dev, "%s\n", client->name); > > + drm_client_modeset_free(client); > drm_client_close(client); > drm_dev_put(dev); If the order should be the opposite of init then call drm_client_close() before drm_client_modeset_free() > if (client->funcs) > + drm_client_for_each_modeset(mode_set, client) { > struct drm_plane *primary = mode_set->crtc->primary; > unsigned int rotation; > > @@ -517,9 +518,11 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ > > static int restore_fbdev_mode_legacy(struct drm_fb_helper *fb_helper) > { > + struct drm_client_dev *client = &fb_helper->client; > struct drm_device *dev = fb_helper->dev; > + struct drm_mode_set *mode_set; > struct drm_plane *plane; > - int i, ret = 0; > + int ret = 0; > > drm_modeset_lock_all(fb_helper->dev); > drm_for_each_plane(plane, dev) { > @@ -532,8 +535,7 @@ static int restore_fbdev_mode_legacy(struct drm_fb_helper *fb_helper) > DRM_MODE_ROTATE_0); > } > > - for (i = 0; i < fb_helper->crtc_count; i++) { > - struct drm_mode_set *mode_set = &fb_helper->crtc_info[i].mode_set; > + drm_client_for_each_modeset(mode_set, client) { > struct drm_crtc *crtc = mode_set->crtc; > > if (crtc->funcs->cursor_set2) { This function requires modeset_mutex to be held. Maybe add comment? > @@ -682,15 +689,14 @@ static struct sysrq_key_op sysrq_drm_fb_helper_restore_op = { }; > > static void dpms_legacy(struct drm_fb_helper *fb_helper, int dpms_mode) > { > + struct drm_client_dev *client = &fb_helper->client; > struct drm_device *dev = fb_helper->dev; > struct drm_connector *connector; > struct drm_mode_set *modeset; > - int i, j; > + int j; > > drm_modeset_lock_all(dev); > - for (i = 0; i < fb_helper->crtc_count; i++) { > - modeset = &fb_helper->crtc_info[i].mode_set; > - > + drm_client_for_each_modeset(modeset, client) { > if (!modeset->crtc->enabled) > continue; > This function needs modeset_mutex - maybe add comment? > @@ -1390,13 +1353,14 @@ static int setcmap_pseudo_palette(struct fb_cmap *cmap, struct fb_info *info) > static int setcmap_legacy(struct fb_cmap *cmap, struct fb_info *info) > { > struct drm_fb_helper *fb_helper = info->par; > + struct drm_mode_set *modeset; > struct drm_crtc *crtc; > u16 *r, *g, *b; > - int i, ret = 0; > + int ret = 0; > > drm_modeset_lock_all(fb_helper->dev); > - for (i = 0; i < fb_helper->crtc_count; i++) { > - crtc = fb_helper->crtc_info[i].mode_set.crtc; > + drm_client_for_each_modeset(modeset, &fb_helper->client) { > + crtc = modeset->crtc; > if (!crtc->funcs->gamma_set || !crtc->gamma_size) > return -EINVAL; > This function needs modeset_mutex - maybe add comment? > @@ -1472,10 +1436,11 @@ static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info) > struct drm_modeset_acquire_ctx ctx; > struct drm_crtc_state *crtc_state; > struct drm_atomic_state *state; > + struct drm_mode_set *modeset; > struct drm_crtc *crtc; > u16 *r, *g, *b; > - int i, ret = 0; > bool replaced; > + int ret = 0; > > drm_modeset_acquire_init(&ctx, 0); > > @@ -1487,8 +1452,8 @@ static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info) > > state->acquire_ctx = &ctx; > retry: > - for (i = 0; i < fb_helper->crtc_count; i++) { > - crtc = fb_helper->crtc_info[i].mode_set.crtc; > + drm_client_for_each_modeset(modeset, &fb_helper->client) { > + crtc = modeset->crtc; > > if (!gamma_lut) > gamma_lut = setcmap_new_gamma_lut(crtc, cmap); > @@ -1516,8 +1481,8 @@ static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info) > if (ret) > goto out_state; > > - for (i = 0; i < fb_helper->crtc_count; i++) { > - crtc = fb_helper->crtc_info[i].mode_set.crtc; > + drm_client_for_each_modeset(modeset, &fb_helper->client) { > + crtc = modeset->crtc; > > r = crtc->gamma_store; > g = r + crtc->gamma_size; This function needs modeset_mutex - maybe add comment? > @@ -1567,12 +1532,14 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info) > goto unlock; > } > > + mutex_lock(&fb_helper->client.modeset_mutex); > if (info->fix.visual == FB_VISUAL_TRUECOLOR) > ret = setcmap_pseudo_palette(cmap, info); > else if (drm_drv_uses_atomic_modeset(fb_helper->dev)) > ret = setcmap_atomic(cmap, info); > else > ret = setcmap_legacy(cmap, info); > + mutex_unlock(&fb_helper->client.modeset_mutex); > > drm_master_internal_release(dev); > unlock: > @@ -1596,7 +1563,6 @@ int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd, > { > struct drm_fb_helper *fb_helper = info->par; > struct drm_device *dev = fb_helper->dev; > - struct drm_mode_set *mode_set; > struct drm_crtc *crtc; > int ret = 0; > > @@ -1624,8 +1590,7 @@ int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd, > * make. If we're not smart enough here, one should > * just consider switch the userspace to KMS. > */ > - mode_set = &fb_helper->crtc_info[0].mode_set; > - crtc = mode_set->crtc; > + crtc = fb_helper->client.modesets[0].crtc; > > /* > * Only wait for a vblank event if the CRTC is > @@ -1822,16 +1787,14 @@ EXPORT_SYMBOL(drm_fb_helper_set_par); > > static void pan_set(struct drm_fb_helper *fb_helper, int x, int y) > { > - int i; > - > - for (i = 0; i < fb_helper->crtc_count; i++) { > - struct drm_mode_set *mode_set; > - > - mode_set = &fb_helper->crtc_info[i].mode_set; > + struct drm_mode_set *mode_set; > > + mutex_lock(&fb_helper->client.modeset_mutex); > + drm_client_for_each_modeset(mode_set, &fb_helper->client) { > mode_set->x = x; > mode_set->y = y; > } > + mutex_unlock(&fb_helper->client.modeset_mutex); > } > > static int pan_display_atomic(struct fb_var_screeninfo *var, > @@ -1842,7 +1805,7 @@ static int pan_display_atomic(struct fb_var_screeninfo *var, > > pan_set(fb_helper, var->xoffset, var->yoffset); > > - ret = restore_fbdev_mode_atomic(fb_helper, true); > + ret = restore_fbdev_mode_force(fb_helper); This change looks alien compared to other changes. Does it belong to this patchset? > if (!ret) { > info->var.xoffset = var->xoffset; > info->var.yoffset = var->yoffset; > @@ -1856,14 +1819,13 @@ static int pan_display_legacy(struct fb_var_screeninfo *var, > struct fb_info *info) > { > struct drm_fb_helper *fb_helper = info->par; > + struct drm_client_dev *client = &fb_helper->client; > struct drm_mode_set *modeset; > int ret = 0; > - int i; > > drm_modeset_lock_all(fb_helper->dev); > - for (i = 0; i < fb_helper->crtc_count; i++) { > - modeset = &fb_helper->crtc_info[i].mode_set; > - > + mutex_lock(&client->modeset_mutex); > + drm_client_for_each_modeset(modeset, client) { > modeset->x = var->xoffset; > modeset->y = var->yoffset; > > @@ -1875,6 +1837,7 @@ static int pan_display_legacy(struct fb_var_screeninfo *var, > } > } > } > + mutex_unlock(&client->modeset_mutex); > drm_modeset_unlock_all(fb_helper->dev); > > return ret; > @@ -1921,10 +1884,12 @@ EXPORT_SYMBOL(drm_fb_helper_pan_display); > static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper, > int preferred_bpp) > { > + struct drm_client_dev *client = &fb_helper->client; > int ret = 0; > int crtc_count = 0; > int i; > struct drm_fb_helper_surface_size sizes; > + struct drm_mode_set *mode_set; > int best_depth = 0; > > memset(&sizes, 0, sizeof(struct drm_fb_helper_surface_size)); > @@ -1975,13 +1940,13 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper, > * supports RGBA5551 (16 bpp, depth 15) but not RGB565 (16 bpp, depth > * 16) we need to scale down the depth of the sizes we request. > */ > - for (i = 0; i < fb_helper->crtc_count; i++) { > - struct drm_mode_set *mode_set = &fb_helper->crtc_info[i].mode_set; > + mutex_lock(&client->modeset_mutex); > + drm_client_for_each_modeset(mode_set, client) { > struct drm_crtc *crtc = mode_set->crtc; > struct drm_plane *plane = crtc->primary; > int j; > > - DRM_DEBUG("test CRTC %d primary plane\n", i); > + DRM_DEBUG("test CRTC %u primary plane\n", drm_crtc_index(crtc)); > > for (j = 0; j < plane->format_count; j++) { > const struct drm_format_info *fmt; > @@ -2021,9 +1986,8 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper, > > /* first up get a count of crtcs now in use and new min/maxes width/heights */ > crtc_count = 0; > - for (i = 0; i < fb_helper->crtc_count; i++) { > + drm_client_for_each_modeset(mode_set, client) { > struct drm_display_mode *desired_mode; > - struct drm_mode_set *mode_set; > int x, y, j; > /* in case of tile group, are we the last tile vert or horiz? > * If no tile group you are always the last one both vertically > @@ -2031,7 +1995,6 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper, > */ > bool lastv = true, lasth = true; > > - mode_set = &fb_helper->crtc_info[i].mode_set; > desired_mode = mode_set->mode; > > if (!desired_mode) > @@ -2061,6 +2024,7 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper, > if (lastv) > sizes.fb_height = min_t(u32, desired_mode->vdisplay + y, sizes.fb_height); > } > + mutex_unlock(&client->modeset_mutex); > > if (crtc_count == 0 || sizes.fb_width == -1 || sizes.fb_height == -1) { > DRM_INFO("Cannot find any crtc or sizes\n"); > @@ -2292,7 +2256,7 @@ static bool drm_target_cloned(struct drm_fb_helper *fb_helper, > struct drm_display_mode *dmt_mode, *mode; > > /* only contemplate cloning in the single crtc case */ > - if (fb_helper->crtc_count > 1) > + if (fb_helper->dev->mode_config.num_crtc > 1) > return false; > > count = 0; > @@ -2481,15 +2445,17 @@ static bool connector_has_possible_crtc(struct drm_connector *connector, > } > > static int drm_pick_crtcs(struct drm_fb_helper *fb_helper, > - struct drm_fb_helper_crtc **best_crtcs, > + struct drm_crtc **best_crtcs, > struct drm_display_mode **modes, > int n, int width, int height) > { > - int c, o; > + struct drm_client_dev *client = &fb_helper->client; > struct drm_connector *connector; > int my_score, best_score, score; > - struct drm_fb_helper_crtc **crtcs, *crtc; > + struct drm_crtc **crtcs, *crtc; > + struct drm_mode_set *modeset; > struct drm_fb_helper_connector *fb_helper_conn; > + int o; > > if (n == fb_helper->connector_count) > return 0; drm_pick_crtcs() needs the modeset mutex. And due considering the complexity of the function it would be nice to have this fact documented. > @@ -2502,8 +2468,7 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper, > if (modes[n] == NULL) > return best_score; > > - crtcs = kcalloc(fb_helper->connector_count, > - sizeof(struct drm_fb_helper_crtc *), GFP_KERNEL); > + crtcs = kcalloc(fb_helper->connector_count, sizeof(*crtcs), GFP_KERNEL); > if (!crtcs) > return best_score; > > @@ -2519,11 +2484,10 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper, > * select a crtc for this connector and then attempt to configure > * remaining connectors > */ > - for (c = 0; c < fb_helper->crtc_count; c++) { > - crtc = &fb_helper->crtc_info[c]; > + drm_client_for_each_modeset(modeset, client) { > + crtc = modeset->crtc; > > - if (!connector_has_possible_crtc(connector, > - crtc->mode_set.crtc)) > + if (!connector_has_possible_crtc(connector, crtc)) > continue; > > for (o = 0; o < n; o++) > @@ -2532,7 +2496,7 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper, > > if (o < n) { > /* ignore cloning unless only a single crtc */ > - if (fb_helper->crtc_count > 1) > + if (fb_helper->dev->mode_config.num_crtc > 1) > continue; > > if (!drm_mode_equal(modes[o], modes[n])) > @@ -2540,14 +2504,13 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper, > } > > crtcs[n] = crtc; > - memcpy(crtcs, best_crtcs, n * sizeof(struct drm_fb_helper_crtc *)); > + memcpy(crtcs, best_crtcs, n * sizeof(*crtcs)); > score = my_score + drm_pick_crtcs(fb_helper, crtcs, modes, n + 1, > width, height); > if (score > best_score) { > best_score = score; > memcpy(best_crtcs, crtcs, > - fb_helper->connector_count * > - sizeof(struct drm_fb_helper_crtc *)); > + fb_helper->connector_count * sizeof(*crtcs)); > } > } > > @@ -2555,21 +2518,9 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper, > return best_score; > } > > -static struct drm_fb_helper_crtc * > -drm_fb_helper_crtc(struct drm_fb_helper *fb_helper, struct drm_crtc *crtc) > -{ > - int i; > - > - for (i = 0; i < fb_helper->crtc_count; i++) > - if (fb_helper->crtc_info[i].mode_set.crtc == crtc) > - return &fb_helper->crtc_info[i]; > - > - return NULL; > -} > - > /* Try to read the BIOS display configuration and use it for the initial config */ > static bool drm_fb_helper_firmware_config(struct drm_fb_helper *fb_helper, > - struct drm_fb_helper_crtc **crtcs, > + struct drm_crtc **crtcs, > struct drm_display_mode **modes, > struct drm_fb_offset *offsets, > bool *enabled, int width, int height) > @@ -2605,7 +2556,7 @@ static bool drm_fb_helper_firmware_config(struct drm_fb_helper *fb_helper, > struct drm_fb_helper_connector *fb_conn; > struct drm_connector *connector; > struct drm_encoder *encoder; > - struct drm_fb_helper_crtc *new_crtc; > + struct drm_crtc *new_crtc; > > fb_conn = fb_helper->connector_info[i]; > connector = fb_conn->connector; > @@ -2647,7 +2598,7 @@ static bool drm_fb_helper_firmware_config(struct drm_fb_helper *fb_helper, > > num_connectors_enabled++; > > - new_crtc = drm_fb_helper_crtc(fb_helper, connector->state->crtc); > + new_crtc = connector->state->crtc; > > /* > * Make sure we're not trying to drive multiple connectors > @@ -2747,10 +2698,11 @@ static bool drm_fb_helper_firmware_config(struct drm_fb_helper *fb_helper, > static void drm_setup_crtcs(struct drm_fb_helper *fb_helper, > u32 width, u32 height) > { > + struct drm_client_dev *client = &fb_helper->client; > struct drm_device *dev = fb_helper->dev; > - struct drm_fb_helper_crtc **crtcs; > struct drm_display_mode **modes; > struct drm_fb_offset *offsets; > + struct drm_crtc **crtcs; > bool *enabled; > int i; > > @@ -2758,8 +2710,7 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper, > /* prevent concurrent modification of connector_count by hotplug */ > lockdep_assert_held(&fb_helper->lock); > > - crtcs = kcalloc(fb_helper->connector_count, > - sizeof(struct drm_fb_helper_crtc *), GFP_KERNEL); > + crtcs = kcalloc(fb_helper->connector_count, sizeof(*crtcs), GFP_KERNEL); > modes = kcalloc(fb_helper->connector_count, > sizeof(struct drm_display_mode *), GFP_KERNEL); > offsets = kcalloc(fb_helper->connector_count, > @@ -2771,6 +2722,8 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper, > goto out; > } > > + mutex_lock(&client->modeset_mutex); > + > mutex_lock(&fb_helper->dev->mode_config.mutex); > if (drm_fb_helper_probe_connector_modes(fb_helper, width, height) == 0) > DRM_DEBUG_KMS("No connectors reported connected with modes\n"); > @@ -2795,24 +2748,24 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper, > } > mutex_unlock(&fb_helper->dev->mode_config.mutex); > > - /* need to set the modesets up here for use later */ > - /* fill out the connector<->crtc mappings into the modesets */ > - for (i = 0; i < fb_helper->crtc_count; i++) > - drm_fb_helper_modeset_release(fb_helper, > - &fb_helper->crtc_info[i].mode_set); > + drm_client_modeset_release(client); > > drm_fb_helper_for_each_connector(fb_helper, i) { > struct drm_display_mode *mode = modes[i]; > - struct drm_fb_helper_crtc *fb_crtc = crtcs[i]; > + struct drm_crtc *crtc = crtcs[i]; > struct drm_fb_offset *offset = &offsets[i]; > > - if (mode && fb_crtc) { > - struct drm_mode_set *modeset = &fb_crtc->mode_set; > + if (mode && crtc) { > + struct drm_mode_set *modeset = drm_client_find_modeset(client, crtc); > struct drm_connector *connector = > fb_helper->connector_info[i]->connector; > > DRM_DEBUG_KMS("desired mode %s set on crtc %d (%d,%d)\n", > - mode->name, fb_crtc->mode_set.crtc->base.id, offset->x, offset->y); > + mode->name, crtc->base.id, offset->x, offset->y); > + > + if (WARN_ON_ONCE(modeset->num_connectors == DRM_CLIENT_MAX_CLONED_CONNECTORS || > + (dev->mode_config.num_crtc > 1 && modeset->num_connectors == 1))) > + break; > > modeset->mode = drm_mode_duplicate(dev, mode); > drm_connector_get(connector); > @@ -2821,6 +2774,8 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper, > modeset->y = offset->y; > } > } > + > + mutex_unlock(&client->modeset_mutex); > out: > kfree(crtcs); > kfree(modes); > @@ -2837,13 +2792,14 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper, > */ > static void drm_setup_crtcs_fb(struct drm_fb_helper *fb_helper) > { > + struct drm_client_dev *client = &fb_helper->client; > struct fb_info *info = fb_helper->fbdev; > unsigned int rotation, sw_rotations = 0; > + struct drm_mode_set *modeset; > int i; > > - for (i = 0; i < fb_helper->crtc_count; i++) { > - struct drm_mode_set *modeset = &fb_helper->crtc_info[i].mode_set; > - > + mutex_lock(&client->modeset_mutex); > + drm_client_for_each_modeset(modeset, client) { > if (!modeset->num_connectors) > continue; > > @@ -2855,6 +2811,7 @@ static void drm_setup_crtcs_fb(struct drm_fb_helper *fb_helper) > else > sw_rotations |= rotation; > } > + mutex_unlock(&client->modeset_mutex); > > mutex_lock(&fb_helper->dev->mode_config.mutex); > drm_fb_helper_for_each_connector(fb_helper, i) { > @@ -3071,8 +3028,7 @@ EXPORT_SYMBOL(drm_fb_helper_hotplug_event); > * @funcs: fbdev helper functions > * @preferred_bpp: Preferred bits per pixel for the device. > * @dev->mode_config.preferred_depth is used if this is zero. > - * @max_conn_count: Maximum number of connectors. > - * @dev->mode_config.num_connector is used if this is zero. > + * @max_conn_count: Maximum number of connectors (not used) > * > * This function sets up fbdev emulation and registers fbdev for access by > * userspace. If all connectors are disconnected, setup is deferred to the next > @@ -3100,16 +3056,9 @@ int drm_fb_helper_fbdev_setup(struct drm_device *dev, > if (!preferred_bpp) > preferred_bpp = 32; > > - if (!max_conn_count) > - max_conn_count = dev->mode_config.num_connector; > - if (!max_conn_count) { > - DRM_DEV_ERROR(dev->dev, "fbdev: No connectors\n"); > - return -EINVAL; > - } > - > drm_fb_helper_prepare(dev, fb_helper, funcs); > > - ret = drm_fb_helper_init(dev, fb_helper, max_conn_count); > + ret = drm_fb_helper_init(dev, fb_helper, 0); > if (ret < 0) { > DRM_DEV_ERROR(dev->dev, "fbdev: Failed to initialize (ret=%d)\n", ret); > return ret; > @@ -3422,7 +3371,7 @@ static int drm_fbdev_client_hotplug(struct drm_client_dev *client) > > drm_fb_helper_prepare(dev, fb_helper, &drm_fb_helper_generic_funcs); > > - ret = drm_fb_helper_init(dev, fb_helper, dev->mode_config.num_connector); > + ret = drm_fb_helper_init(dev, fb_helper, 0); > if (ret) > goto err; > _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx