On Sun, Apr 07, 2019 at 06:52:37PM +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 modeset connector array size is hardcoded for the cloned case to avoid
> having to pass in a value from the driver. A value of 8 is chosen to err
> on the safe side. This means that the max connector argument for
> drm_fb_helper_init() and drm_fb_helper_fbdev_setup() isn't used anymore,
> a todo entry for this is added.
>
> In pan_display_atomic() restore_fbdev_mode_force() is used instead of
> restore_fbdev_mode_atomic() because that one will later become internal
> to drm_client_modeset.
>
> Locking order:
> 1. drm_fb_helper->lock
> 2. drm_master_internal_acquire
> 3. drm_client_dev->modeset_mutex
>
> v2:
> - Add modesets array to drm_client (Daniel Vetter)
> - Use a new file for the modeset code (Daniel Vetter)
> - File has to be MIT licensed (Emmanuel Vadot)
> - Add copyrights from drm_fb_helper.c
>
> Signed-off-by: Noralf Trønnes <noralf@xxxxxxxxxxx>
> ---
> Documentation/gpu/todo.rst | 7 +
> drivers/gpu/drm/Makefile | 2 +-
> drivers/gpu/drm/drm_client.c | 10 +-
> drivers/gpu/drm/drm_client_modeset.c | 102 +++++++++
> drivers/gpu/drm/drm_fb_helper.c | 301 +++++++++++----------------
> include/drm/drm_client.h | 28 +++
> include/drm/drm_fb_helper.h | 8 -
> 7 files changed, 272 insertions(+), 186 deletions(-)
> create mode 100644 drivers/gpu/drm/drm_client_modeset.c
>
> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> index 1528ad2d598b..8fa08b5feab7 100644
> --- a/Documentation/gpu/todo.rst
> +++ b/Documentation/gpu/todo.rst
> @@ -300,6 +300,13 @@ it to use drm_mode_hsync() instead.
>
> Contact: Sean Paul
>
> +drm_fb_helper cleanup tasks
> +---------------------------
> +
> +- The max connector argument for drm_fb_helper_init() and
> + drm_fb_helper_fbdev_setup() isn't used anymore and can be removed.
> +
> +
> Core refactorings
> =================
>
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 6c7e8d162b4e..08c77c10ccbb 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -19,7 +19,7 @@ drm-y := drm_auth.o drm_bufs.o drm_cache.o \
> drm_plane.o drm_color_mgmt.o drm_print.o \
> drm_dumb_buffers.o drm_mode_config.o drm_vblank.o \
> drm_syncobj.o drm_lease.o drm_writeback.o drm_client.o \
> - drm_atomic_uapi.o
> + drm_client_modeset.o drm_atomic_uapi.o
>
> drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.o
> drm-$(CONFIG_DRM_VM) += drm_vm.o
> diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
> index 9b2bd28dde0a..3ad5b57c1419 100644
> --- a/drivers/gpu/drm/drm_client.c
> +++ b/drivers/gpu/drm/drm_client.c
> @@ -91,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);
> @@ -147,6 +153,8 @@ void drm_client_release(struct drm_client_dev *client)
>
> DRM_DEV_DEBUG_KMS(dev->dev, "%s\n", client->name);
>
> + drm_client_modeset_release(client);
> + drm_client_modeset_free(client);
> drm_client_close(client);
> drm_dev_put(dev);
> if (client->funcs)
> diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
> new file mode 100644
> index 000000000000..bb34222c9db8
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_client_modeset.c
> @@ -0,0 +1,102 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright 2018 Noralf Trønnes
> + * Copyright (c) 2006-2009 Red Hat Inc.
> + * Copyright (c) 2006-2008 Intel Corporation
> + * Jesse Barnes <jesse.barnes@xxxxxxxxx>
> + * Copyright (c) 2007 Dave Airlie <airlied@xxxxxxxx>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> +
> +#include <drm/drm_client.h>
> +#include <drm/drm_crtc.h>
> +#include <drm/drm_device.h>
> +
> +/* Used by drm_client and drm_fb_helper */
> +int drm_client_modeset_create(struct drm_client_dev *client)
> +{
> + struct drm_device *dev = client->dev;
> + unsigned int num_crtc = dev->mode_config.num_crtc;
> + unsigned int max_connector_count = 1;
> + struct drm_mode_set *modeset;
> + struct drm_crtc *crtc;
> + unsigned int i = 0;
> +
> + /* Add terminating zero entry to enable index less iteration */
> + client->modesets = kcalloc(num_crtc + 1, sizeof(*client->modesets), GFP_KERNEL);
> + if (!client->modesets)
> + return -ENOMEM;
> +
> + mutex_init(&client->modeset_mutex);
> +
> + drm_for_each_crtc(crtc, dev)
> + client->modesets[i++].crtc = crtc;
> +
> + /* Cloning is only supported in the single crtc case. */
> + if (num_crtc == 1)
> + max_connector_count = DRM_CLIENT_MAX_CLONED_CONNECTORS;
> +
> + drm_client_for_each_modeset(modeset, client) {
> + modeset->connectors = kcalloc(max_connector_count,
> + sizeof(*modeset->connectors), GFP_KERNEL);
> + if (!modeset->connectors)
> + goto err_free;
> + }
> +
> + return 0;
> +
> +err_free:
> + drm_client_modeset_free(client);
> +
> + return -ENOMEM;
> +}
> +EXPORT_SYMBOL(drm_client_modeset_create);
> +
> +/* Used by drm_client and drm_fb_helper */
> +void drm_client_modeset_free(struct drm_client_dev *client)
> +{
> + struct drm_mode_set *modeset;
> +
> + mutex_destroy(&client->modeset_mutex);
> +
> + drm_client_for_each_modeset(modeset, client)
> + kfree(modeset->connectors);
> + kfree(client->modesets);
> +}
> +EXPORT_SYMBOL(drm_client_modeset_free);
> +
> +/* Used by drm_client and drm_fb_helper */
> +void drm_client_modeset_release(struct drm_client_dev *client)
> +{
> + struct drm_mode_set *modeset;
> + unsigned int i;
> +
> + drm_client_for_each_modeset(modeset, client) {
> + drm_mode_destroy(client->dev, modeset->mode);
> + modeset->mode = NULL;
> + modeset->fb = NULL;
> +
> + for (i = 0; i < modeset->num_connectors; i++) {
> + drm_connector_put(modeset->connectors[i]);
> + modeset->connectors[i] = NULL;
> + }
> + modeset->num_connectors = 0;
> + }
> +}
> +EXPORT_SYMBOL(drm_client_modeset_release);
> +
> +struct drm_mode_set *drm_client_find_modeset(struct drm_client_dev *client, struct drm_crtc *crtc)
> +{
> + struct drm_mode_set *modeset;
> +
> + drm_client_for_each_modeset(modeset, client)
> + if (modeset->crtc == crtc)
> + return modeset;
> +
> + return NULL;
> +}
> +/* TODO: Remove export when modeset code has been moved over */
> +EXPORT_SYMBOL(drm_client_find_modeset);
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 69dddc4a9231..b65edfc99feb 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -317,13 +317,11 @@ int drm_fb_helper_debug_enter(struct fb_info *info)
> {
> struct drm_fb_helper *helper = info->par;
> const struct drm_crtc_helper_funcs *funcs;
> - int i;
> + struct drm_mode_set *mode_set;
>
> list_for_each_entry(helper, &kernel_fb_helper_list, kernel_fb_list) {
> - for (i = 0; i < helper->crtc_count; i++) {
> - struct drm_mode_set *mode_set =
> - &helper->crtc_info[i].mode_set;
> -
> + mutex_lock(&helper->client.modeset_mutex);
> + drm_client_for_each_modeset(mode_set, &helper->client) {
> if (!mode_set->crtc->enabled)
> continue;
>
> @@ -340,6 +338,7 @@ int drm_fb_helper_debug_enter(struct fb_info *info)
> mode_set->y,
> ENTER_ATOMIC_MODE_SET);
> }
> + mutex_unlock(&helper->client.modeset_mutex);
> }
>
> return 0;
> @@ -353,14 +352,14 @@ EXPORT_SYMBOL(drm_fb_helper_debug_enter);
> int drm_fb_helper_debug_leave(struct fb_info *info)
> {
> struct drm_fb_helper *helper = info->par;
> + struct drm_client_dev *client = &helper->client;
> struct drm_crtc *crtc;
> const struct drm_crtc_helper_funcs *funcs;
> + struct drm_mode_set *mode_set;
> struct drm_framebuffer *fb;
> - int i;
> -
> - for (i = 0; i < helper->crtc_count; i++) {
> - struct drm_mode_set *mode_set = &helper->crtc_info[i].mode_set;
>
> + mutex_lock(&client->modeset_mutex);
> + drm_client_for_each_modeset(mode_set, client) {
> crtc = mode_set->crtc;
> if (drm_drv_uses_atomic_modeset(crtc->dev))
> continue;
> @@ -383,6 +382,7 @@ int drm_fb_helper_debug_leave(struct fb_info *info)
> funcs->mode_set_base_atomic(mode_set->crtc, fb, crtc->x,
> crtc->y, LEAVE_ATOMIC_MODE_SET);
> }
> + mutex_unlock(&client->modeset_mutex);
>
> return 0;
> }
> @@ -433,12 +433,14 @@ static bool drm_fb_helper_panel_rotation(struct drm_mode_set *modeset,
>
> static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool active)
> {
> + struct drm_client_dev *client = &fb_helper->client;
> struct drm_device *dev = fb_helper->dev;
> struct drm_plane_state *plane_state;
> struct drm_plane *plane;
> struct drm_atomic_state *state;
> - int i, ret;
> struct drm_modeset_acquire_ctx ctx;
> + struct drm_mode_set *mode_set;
> + int ret;
>
> drm_modeset_acquire_init(&ctx, 0);
>
> @@ -468,8 +470,7 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ
> goto out_state;
> }
>
> - 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_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) {
> @@ -559,11 +561,16 @@ static int restore_fbdev_mode_legacy(struct drm_fb_helper *fb_helper)
> static int restore_fbdev_mode_force(struct drm_fb_helper *fb_helper)
> {
> struct drm_device *dev = fb_helper->dev;
> + int ret;
>
> + mutex_lock(&fb_helper->client.modeset_mutex);
> if (drm_drv_uses_atomic_modeset(dev))
> - return restore_fbdev_mode_atomic(fb_helper, true);
> + ret = restore_fbdev_mode_atomic(fb_helper, true);
> else
> - return restore_fbdev_mode_legacy(fb_helper);
> + ret = restore_fbdev_mode_legacy(fb_helper);
> + mutex_unlock(&fb_helper->client.modeset_mutex);
> +
> + return ret;
> }
>
> static int restore_fbdev_mode(struct drm_fb_helper *fb_helper)
> @@ -672,15 +679,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;
>
> @@ -697,6 +703,7 @@ static void dpms_legacy(struct drm_fb_helper *fb_helper, int dpms_mode)
> static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode)
> {
> struct drm_fb_helper *fb_helper = info->par;
> + struct drm_client_dev *client = &fb_helper->client;
> struct drm_device *dev = fb_helper->dev;
>
> /*
> @@ -706,10 +713,12 @@ static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode)
> if (!drm_master_internal_acquire(dev))
> goto unlock;
>
> + mutex_lock(&client->modeset_mutex);
> if (drm_drv_uses_atomic_modeset(dev))
> restore_fbdev_mode_atomic(fb_helper, dpms_mode == DRM_MODE_DPMS_ON);
> else
> dpms_legacy(fb_helper, dpms_mode);
> + mutex_unlock(&client->modeset_mutex);
>
> drm_master_internal_release(dev);
> unlock:
> @@ -752,43 +761,6 @@ int drm_fb_helper_blank(int blank, struct fb_info *info)
> }
> EXPORT_SYMBOL(drm_fb_helper_blank);
>
> -static void drm_fb_helper_modeset_release(struct drm_fb_helper *helper,
> - struct drm_mode_set *modeset)
> -{
> - int i;
> -
> - for (i = 0; i < modeset->num_connectors; i++) {
> - drm_connector_put(modeset->connectors[i]);
> - modeset->connectors[i] = NULL;
> - }
> - modeset->num_connectors = 0;
> -
> - drm_mode_destroy(helper->dev, modeset->mode);
> - modeset->mode = NULL;
> -
> - /* FIXME should hold a ref? */
> - modeset->fb = NULL;
> -}
> -
> -static void drm_fb_helper_crtc_free(struct drm_fb_helper *helper)
> -{
> - int i;
> -
> - for (i = 0; i < helper->connector_count; i++) {
> - drm_connector_put(helper->connector_info[i]->connector);
> - kfree(helper->connector_info[i]);
> - }
> - kfree(helper->connector_info);
> -
> - for (i = 0; i < helper->crtc_count; i++) {
> - struct drm_mode_set *modeset = &helper->crtc_info[i].mode_set;
> -
> - drm_fb_helper_modeset_release(helper, modeset);
> - kfree(modeset->connectors);
> - }
> - kfree(helper->crtc_info);
> -}
> -
> static void drm_fb_helper_resume_worker(struct work_struct *work)
> {
> struct drm_fb_helper *helper = container_of(work, struct drm_fb_helper,
> @@ -867,7 +839,7 @@ EXPORT_SYMBOL(drm_fb_helper_prepare);
> * drm_fb_helper_init - initialize a &struct drm_fb_helper
> * @dev: drm device
> * @fb_helper: driver-allocated fbdev helper structure to initialize
> - * @max_conn_count: max connector count
> + * @max_conn_count: max connector count (not used)
> *
> * This allocates the structures for the fbdev helper with the given limits.
> * Note that this won't yet touch the hardware (through the driver interfaces)
> @@ -883,53 +855,34 @@ int drm_fb_helper_init(struct drm_device *dev,
> struct drm_fb_helper *fb_helper,
> int max_conn_count)
> {
> - struct drm_crtc *crtc;
> - struct drm_mode_config *config = &dev->mode_config;
> - int i;
> + int ret;
>
> if (!drm_fbdev_emulation) {
> dev->fb_helper = fb_helper;
> return 0;
> }
>
> - if (!max_conn_count)
> - return -EINVAL;
> -
> - fb_helper->crtc_info = kcalloc(config->num_crtc, sizeof(struct drm_fb_helper_crtc), GFP_KERNEL);
> - if (!fb_helper->crtc_info)
> - return -ENOMEM;
> -
> - fb_helper->crtc_count = config->num_crtc;
> - fb_helper->connector_info = kcalloc(dev->mode_config.num_connector, sizeof(struct drm_fb_helper_connector *), GFP_KERNEL);
> - if (!fb_helper->connector_info) {
> - kfree(fb_helper->crtc_info);
> - return -ENOMEM;
> + /* The generic fbdev client has already done this */
> + if (!fb_helper->client.funcs) {
> + fb_helper->client.dev = dev;
> + ret = drm_client_modeset_create(&fb_helper->client);
> + if (ret)
> + return ret;
Hm, this is a bit annoying since it's the only reason we need to
EXPORT_SYMBOL all the new functions. What I had in mind is to call
drm_client_init here, with a no-op ->unregister function because the
lifetimes of the embedded drm_client_dev in the drm_fb_helper (which
drivers usually embedded somewhere else outside of our control) differs
from a real drm_client.
> }
> +
> + fb_helper->connector_info = kcalloc(dev->mode_config.num_connector, sizeof(struct drm_fb_helper_connector *), GFP_KERNEL);
> + if (!fb_helper->connector_info)
> + goto out_free;
> +
> fb_helper->connector_info_alloc_count = dev->mode_config.num_connector;
> fb_helper->connector_count = 0;
>
> - for (i = 0; i < fb_helper->crtc_count; i++) {
> - fb_helper->crtc_info[i].mode_set.connectors =
> - kcalloc(max_conn_count,
> - sizeof(struct drm_connector *),
> - GFP_KERNEL);
> -
> - if (!fb_helper->crtc_info[i].mode_set.connectors)
> - goto out_free;
> - fb_helper->crtc_info[i].mode_set.num_connectors = 0;
> - }
> -
> - i = 0;
> - drm_for_each_crtc(crtc, dev) {
> - fb_helper->crtc_info[i].mode_set.crtc = crtc;
> - i++;
> - }
> -
> dev->fb_helper = fb_helper;
>
> return 0;
> out_free:
> - drm_fb_helper_crtc_free(fb_helper);
> + drm_client_modeset_free(&fb_helper->client);
> +
> return -ENOMEM;
> }
> EXPORT_SYMBOL(drm_fb_helper_init);
> @@ -1005,6 +958,7 @@ EXPORT_SYMBOL(drm_fb_helper_unregister_fbi);
> void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
> {
> struct fb_info *info;
> + int i;
>
> if (!fb_helper)
> return;
> @@ -1034,8 +988,17 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
> mutex_unlock(&kernel_fb_helper_lock);
>
> mutex_destroy(&fb_helper->lock);
> - drm_fb_helper_crtc_free(fb_helper);
>
> + if (!fb_helper->client.funcs) {
> + drm_client_modeset_release(&fb_helper->client);
> + drm_client_modeset_free(&fb_helper->client);
> + }
We should then also be able to call drm_client_release here if funcs
matches our fake client functions. Since we won't call drm_client_register
there's not going to be an issue with client->list I think.
> +
> + for (i = 0; i < fb_helper->connector_count; i++) {
> + drm_connector_put(fb_helper->connector_info[i]->connector);
> + kfree(fb_helper->connector_info[i]);
> + }
> + kfree(fb_helper->connector_info);
> }
> EXPORT_SYMBOL(drm_fb_helper_fini);
>
> @@ -1380,13 +1343,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;
>
> @@ -1462,10 +1426,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);
>
> @@ -1477,8 +1442,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);
> @@ -1506,8 +1471,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;
> @@ -1557,12 +1522,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:
> @@ -1586,7 +1553,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;
>
> @@ -1614,8 +1580,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
> @@ -1812,16 +1777,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,
> @@ -1832,7 +1795,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);
> if (!ret) {
> info->var.xoffset = var->xoffset;
> info->var.yoffset = var->yoffset;
> @@ -1846,14 +1809,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;
>
> @@ -1865,6 +1827,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;
> @@ -1911,10 +1874,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));
> @@ -1965,13 +1930,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;
> @@ -2011,9 +1976,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
> @@ -2021,7 +1985,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)
> @@ -2051,6 +2014,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");
> @@ -2282,7 +2246,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;
> @@ -2471,15 +2435,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;
> @@ -2492,8 +2458,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;
>
> @@ -2509,11 +2474,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++)
> @@ -2522,7 +2486,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]))
> @@ -2530,14 +2494,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));
> }
> }
>
> @@ -2545,21 +2508,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)
> @@ -2591,7 +2542,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;
> @@ -2634,7 +2585,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
> @@ -2736,10 +2687,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;
>
> @@ -2747,8 +2699,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,
> @@ -2760,6 +2711,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");
> @@ -2784,24 +2737,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);
> @@ -2810,6 +2763,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);
> @@ -2826,13 +2781,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;
>
> @@ -2844,6 +2800,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) {
> @@ -3060,8 +3017,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
> @@ -3089,16 +3045,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;
> @@ -3411,7 +3360,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;
>
> diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h
> index 8b552b1a6ce9..858c8be70870 100644
> --- a/include/drm/drm_client.h
> +++ b/include/drm/drm_client.h
> @@ -3,8 +3,11 @@
> #ifndef _DRM_CLIENT_H_
> #define _DRM_CLIENT_H_
>
> +#include <linux/mutex.h>
> #include <linux/types.h>
>
> +#include <drm/drm_crtc.h>
> +
> struct drm_client_dev;
> struct drm_device;
> struct drm_file;
> @@ -13,6 +16,8 @@ struct drm_gem_object;
> struct drm_minor;
> struct module;
>
> +#define DRM_CLIENT_MAX_CLONED_CONNECTORS 8
> +
> /**
> * struct drm_client_funcs - DRM client callbacks
> */
> @@ -85,6 +90,16 @@ struct drm_client_dev {
> * @file: DRM file
> */
> struct drm_file *file;
> +
> + /**
> + * @modeset_mutex: Protects @modesets.
> + */
> + struct mutex modeset_mutex;
Bit on the fence about doing locking automatically, but definitely makes
it easier to use drm_client_modeset functions.
> +
> + /**
> + * @modesets: CRTC configurations
> + */
> + struct drm_mode_set *modesets;
> };
>
> int drm_client_init(struct drm_device *dev, struct drm_client_dev *client,
> @@ -135,6 +150,19 @@ struct drm_client_buffer *
> drm_client_framebuffer_create(struct drm_client_dev *client, u32 width, u32 height, u32 format);
> void drm_client_framebuffer_delete(struct drm_client_buffer *buffer);
>
> +int drm_client_modeset_create(struct drm_client_dev *client);
> +void drm_client_modeset_free(struct drm_client_dev *client);
> +void drm_client_modeset_release(struct drm_client_dev *client);
> +struct drm_mode_set *drm_client_find_modeset(struct drm_client_dev *client, struct drm_crtc *crtc);
> +
> +/**
> + * drm_client_for_each_modeset() - Iterate over client modesets
> + * @modeset: &drm_mode_set loop cursor
> + * @client: DRM client
> + */
> +#define drm_client_for_each_modeset(modeset, client) \
> + for (modeset = (client)->modesets; modeset && modeset->crtc; modeset++)
Would be good to add a assert_lock_held here, i.e.
for (assert_lock_held(client->modeset_mutex), modeset = (client)->modesets;
modeset && modeset->crtc; modeset++)
Cheers, Daniel
> +
> int drm_client_debugfs_init(struct drm_minor *minor);
>
> #endif
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index 0d6d23a15b42..8d0ddea74a9e 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -46,10 +46,6 @@ struct drm_fb_offset {
> int x, y;
> };
>
> -struct drm_fb_helper_crtc {
> - struct drm_mode_set mode_set;
> -};
> -
> /**
> * struct drm_fb_helper_surface_size - describes fbdev size and scanout surface size
> * @fb_width: fbdev width
> @@ -108,8 +104,6 @@ struct drm_fb_helper_connector {
> * struct drm_fb_helper - main structure to emulate fbdev on top of KMS
> * @fb: Scanout framebuffer object
> * @dev: DRM device
> - * @crtc_count: number of possible CRTCs
> - * @crtc_info: per-CRTC helper state (mode, x/y offset, etc)
> * @connector_count: number of connected connectors
> * @connector_info_alloc_count: size of connector_info
> * @funcs: driver callbacks for fb helper
> @@ -143,8 +137,6 @@ struct drm_fb_helper {
>
> struct drm_framebuffer *fb;
> struct drm_device *dev;
> - int crtc_count;
> - struct drm_fb_helper_crtc *crtc_info;
> int connector_count;
> int connector_info_alloc_count;
> /**
> --
> 2.20.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
