From: Sean Paul <seanpaul@chromium.org>
To: Rob Clark <robdclark@gmail.com>
Cc: dri-devel <dri-devel@lists.freedesktop.org>
Subject: Re: [RFCv4 10/14] drm: convert crtc to properties/state
Date: Tue, 4 Mar 2014 16:29:23 -0500 [thread overview]
Message-ID: <CAOw6vbJM-7eBU2ZfRpJyfAQjV_vdAXAefUM6xMToNw6TVCUMrA@mail.gmail.com> (raw)
In-Reply-To: <1385390858-4412-11-git-send-email-robdclark@gmail.com>
On Mon, Nov 25, 2013 at 9:47 AM, Rob Clark <robdclark@gmail.com> wrote:
> Break the mutable state of a crtc out into a separate structure
> and use atomic properties mechanism to set crtc attributes. This
> makes it easier to have some helpers for crtc->set_property()
> and for checking for invalid params. The idea is that individual
> drivers can wrap the state struct in their own struct which adds
> driver specific parameters, for easy build-up of state across
> multiple set_property() calls and for easy atomic commit or roll-
> back.
> ---
<snip>
> +
> +static int remove_connector(struct drm_crtc *ocrtc,
> + struct drm_crtc_state *ostate, void *state, int idx)
> +{
> + struct drm_mode_config *config = &ocrtc->dev->mode_config;
> + uint32_t *new_connector_ids;
> + int a, b;
> +
> + /* before deletion point: */
> + a = idx * sizeof(ostate->connector_ids[0]);
> +
> + /* after deletion point: */
> + b = (ostate->num_connector_ids - 1 - idx) *
> + sizeof(ostate->connector_ids[0]);
> +
> + new_connector_ids = kmalloc(a+b, GFP_KERNEL);
> + if (!new_connector_ids)
> + return -ENOMEM;
> +
> + memcpy(new_connector_ids, ostate->connector_ids, a);
> + memcpy(&new_connector_ids[idx],
> + &ostate->connector_ids[idx + 1], b);
> +
> + return drm_mode_crtc_set_obj_prop(ocrtc, state,
> + config->prop_connector_ids, a + b,
> + new_connector_ids);
> +}
> +
> +static int check_connectors(struct drm_crtc *crtc, void *state, bool fix,
> + uint32_t *connector_ids, uint32_t num_connector_ids)
> +{
> + struct drm_mode_config *config = &crtc->dev->mode_config;
> + struct drm_crtc *ocrtc; /* other connector */
> +
> + list_for_each_entry(ocrtc, &config->crtc_list, head) {
> + struct drm_crtc_state *ostate; /* other state */
> + unsigned i;
> +
> + if (ocrtc == crtc)
> + continue;
> +
> + ostate = drm_atomic_get_crtc_state(crtc, state);
Hi Rob,
This will populate state's placeholder for ocrtc, which will have the
unintended consequence of committing ocrtc's state and thus
unreferencing ocrtc's current fb in
drm_atomic_helper_commit_crtc_state.
Maybe a new transient state bit in drm_crtc_state which avoids the
commit_crtc_state call is in order?
> + if (IS_ERR(ostate))
> + return PTR_ERR(ostate);
> +
> + for (i = 0; i < num_connector_ids; i++) {
> + struct drm_connector *connector;
> + uint32_t cid = connector_ids[i];
> + int idx;
> +
> +retry:
> + idx = connector_idx(ostate, cid);
> + if (idx < 0)
> + continue;
> +
> + if (fix) {
> + int ret = remove_connector(ocrtc,
> + ostate, state, idx);
> + if (ret)
> + return ret;
> + goto retry;
> + }
> +
> + connector = drm_connector_find(crtc->dev, cid);
> + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] already in use\n",
> + connector->base.id,
> + drm_get_connector_name(connector));
> + return -EINVAL;
> + }
> + }
> +
> + return 0;
> +}
> +
> +int drm_crtc_check_state(struct drm_crtc *crtc,
> + struct drm_crtc_state *state)
> +{
> + struct drm_framebuffer *fb = state->fb;
> + int hdisplay, vdisplay;
> + struct drm_display_mode *mode = get_mode(crtc, state);
> +
> + if (IS_ERR(mode))
> + return PTR_ERR(mode);
> +
> + /* disabling the crtc is allowed: */
> + if (!(fb && state->mode_valid))
> + return 0;
> +
> + hdisplay = state->mode.hdisplay;
> + vdisplay = state->mode.vdisplay;
> +
> + if (mode && drm_mode_is_stereo(mode)) {
> + struct drm_display_mode adjusted = *mode;
> +
> + drm_mode_set_crtcinfo(&adjusted, CRTC_STEREO_DOUBLE);
> + hdisplay = adjusted.crtc_hdisplay;
> + vdisplay = adjusted.crtc_vdisplay;
> + }
> +
> + if (state->invert_dimensions)
> + swap(hdisplay, vdisplay);
> +
> + /* For some reason crtc x/y offsets are signed internally. */
> + if (state->x > INT_MAX || state->y > INT_MAX)
> + return -ERANGE;
> +
> + if (hdisplay > fb->width ||
> + vdisplay > fb->height ||
> + state->x > fb->width - hdisplay ||
> + state->y > fb->height - vdisplay) {
> + DRM_DEBUG_KMS("Invalid fb size %ux%u for CRTC viewport %ux%u+%d+%d%s.\n",
> + fb->width, fb->height, hdisplay, vdisplay,
> + state->x, state->y,
> + state->invert_dimensions ? " (inverted)" : "");
> + return -ENOSPC;
> + }
> +
> + if (crtc->enabled && !state->set_config) {
> + if (crtc->state->fb->pixel_format != fb->pixel_format) {
> + DRM_DEBUG_KMS("Page flip is not allowed to "
> + "change frame buffer format.\n");
> + return -EINVAL;
> + }
> + }
> +
> + if (state->num_connector_ids == 0) {
> + DRM_DEBUG_KMS("Count connectors is 0 but mode set\n");
> + return -EINVAL;
> + }
> +
> + if (state->connectors_change) {
> + int ret = check_connectors(crtc, state->state, false,
> + state->connector_ids, state->num_connector_ids);
> + if (ret)
> + return ret;
> + }
> +
> + if (mode)
> + drm_mode_destroy(crtc->dev, mode);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_crtc_check_state);
> +
<snip>
> /**
> * drm_mode_setcrtc - set CRTC configuration
> * @dev: drm device for the ioctl
> @@ -2345,22 +2556,15 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
> struct drm_mode_config *config = &dev->mode_config;
> struct drm_mode_crtc *crtc_req = data;
> struct drm_crtc *crtc;
> - struct drm_connector **connector_set = NULL, *connector;
> - struct drm_framebuffer *fb = NULL;
> - struct drm_display_mode *mode = NULL;
> - struct drm_mode_set set;
> - uint32_t __user *set_connectors_ptr;
> + uint32_t fb_id = -1;
> + uint32_t *connector_ids = NULL;
> + void *state = NULL;
> int ret;
> int i;
>
> if (!drm_core_check_feature(dev, DRIVER_MODESET))
> return -EINVAL;
>
> - /* For some reason crtc x/y offsets are signed internally. */
> - if (crtc_req->x > INT_MAX || crtc_req->y > INT_MAX)
> - return -ERANGE;
> -
> - drm_modeset_lock_all(dev);
> crtc = drm_crtc_find(dev, crtc_req->crtc_id);
> if (!crtc) {
> DRM_DEBUG_KMS("Unknown CRTC ID %d\n", crtc_req->crtc_id);
> @@ -2378,55 +2582,15 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
> ret = -EINVAL;
> goto out;
> }
> - fb = crtc->fb;
> - /* Make refcounting symmetric with the lookup path. */
> - drm_framebuffer_reference(fb);
> + fb_id = crtc->base.id;
s/crtc->base.id/crtc->fb->base.id/ here?
> } else {
> - fb = drm_framebuffer_lookup(dev, crtc_req->fb_id);
> - if (!fb) {
> - DRM_DEBUG_KMS("Unknown FB ID%d\n",
> - crtc_req->fb_id);
> - ret = -ENOENT;
> - goto out;
> - }
> + fb_id = crtc_req->fb_id;
> }
> -
> - mode = drm_mode_create(dev);
> - if (!mode) {
> - ret = -ENOMEM;
> - goto out;
> - }
> -
> - ret = drm_crtc_convert_umode(mode, &crtc_req->mode);
> - if (ret) {
> - DRM_DEBUG_KMS("Invalid mode\n");
> - goto out;
> - }
> -
> - drm_mode_set_crtcinfo(mode, CRTC_INTERLACE_HALVE_V);
> -
> - ret = drm_crtc_check_viewport(crtc, crtc_req->x, crtc_req->y,
> - mode, fb);
> - if (ret)
> - goto out;
> -
> - }
> -
> - if (crtc_req->count_connectors == 0 && mode) {
> - DRM_DEBUG_KMS("Count connectors is 0 but mode set\n");
> - ret = -EINVAL;
> - goto out;
> - }
> -
> - if (crtc_req->count_connectors > 0 && (!mode || !fb)) {
> - DRM_DEBUG_KMS("Count connectors is %d but no mode or fb set\n",
> - crtc_req->count_connectors);
> - ret = -EINVAL;
> - goto out;
> }
>
> if (crtc_req->count_connectors > 0) {
> - u32 out_id;
> + uint32_t __user *set_connectors_ptr =
> + (uint32_t __user *)(unsigned long)crtc_req->set_connectors_ptr;
>
> /* Avoid unbounded kernel memory allocation */
> if (crtc_req->count_connectors > config->num_connector) {
> @@ -2434,52 +2598,63 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
> goto out;
> }
>
> - connector_set = kmalloc(crtc_req->count_connectors *
> - sizeof(struct drm_connector *),
> + connector_ids = kmalloc(crtc_req->count_connectors *
> + sizeof(connector_ids[0]),
> GFP_KERNEL);
> - if (!connector_set) {
> + if (!connector_ids) {
> ret = -ENOMEM;
> goto out;
> }
>
> for (i = 0; i < crtc_req->count_connectors; i++) {
> - set_connectors_ptr = (uint32_t __user *)(unsigned long)crtc_req->set_connectors_ptr;
> + u32 out_id;
> +
> if (get_user(out_id, &set_connectors_ptr[i])) {
> ret = -EFAULT;
> goto out;
> }
> -
> - connector = drm_connector_find(dev, out_id);
> - if (!connector) {
> - DRM_DEBUG_KMS("Connector id %d unknown\n",
> - out_id);
> - ret = -ENOENT;
> - goto out;
> - }
> - DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> - connector->base.id,
> - drm_get_connector_name(connector));
> -
> - connector_set[i] = connector;
> + connector_ids[i] = out_id;
> }
> }
>
> - set.crtc = crtc;
> - set.x = crtc_req->x;
> - set.y = crtc_req->y;
> - set.mode = mode;
> - set.connectors = connector_set;
> - set.num_connectors = crtc_req->count_connectors;
> - set.fb = fb;
> - ret = drm_mode_set_config_internal(&set);
> +retry:
> + state = dev->driver->atomic_begin(dev, 0);
> + if (IS_ERR(state))
> + return PTR_ERR(state);
>
> -out:
> - if (fb)
> - drm_framebuffer_unreference(fb);
> + /* If connectors change, we need to check if we need to steal one
> + * from another CRTC.. setcrtc makes this implicit, but atomic
> + * treats it as an error so we need to handle here:
> + */
> + ret = check_connectors(crtc, state, true,
> + connector_ids, crtc_req->count_connectors);
> + if (ret)
> + goto out;
>
> - kfree(connector_set);
> - drm_mode_destroy(dev, mode);
> - drm_modeset_unlock_all(dev);
> + ret =
> + drm_mode_crtc_set_obj_prop(crtc, state,
> + config->prop_mode, sizeof(crtc_req->mode), &crtc_req->mode) ||
> + drm_mode_crtc_set_obj_prop(crtc, state,
> + config->prop_connector_ids,
> + crtc_req->count_connectors * sizeof(connector_ids[0]),
> + connector_ids) ||
> + drm_mode_crtc_set_obj_prop(crtc, state,
> + config->prop_fb_id, fb_id, NULL) ||
> + drm_mode_crtc_set_obj_prop(crtc, state,
> + config->prop_src_x, crtc_req->x, NULL) ||
> + drm_mode_crtc_set_obj_prop(crtc, state,
> + config->prop_src_y, crtc_req->y, NULL) ||
> + dev->driver->atomic_check(dev, state);
> + if (ret)
> + goto out;
> +
> + ret = dev->driver->atomic_commit(dev, state);
> +
> +out:
> + if (state)
> + dev->driver->atomic_end(dev, state);
> + if (ret == -EDEADLK)
> + goto retry;
> return ret;
> }
>
<snip>
> int drm_mode_page_flip_ioctl(struct drm_device *dev,
> void *data, struct drm_file *file_priv)
> {
> struct drm_mode_crtc_page_flip *page_flip = data;
> + struct drm_mode_config *config = &dev->mode_config;
> struct drm_crtc *crtc;
> - struct drm_framebuffer *fb = NULL, *old_fb = NULL;
> struct drm_pending_vblank_event *e = NULL;
> - unsigned long flags;
> + void *state;
> int ret = -EINVAL;
>
> if (page_flip->flags & ~DRM_MODE_PAGE_FLIP_FLAGS ||
> @@ -3946,92 +4163,41 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> if (!crtc)
> return -ENOENT;
>
> - drm_modeset_lock(&crtc->mutex, NULL);
> - if (crtc->fb == NULL) {
> - /* The framebuffer is currently unbound, presumably
> - * due to a hotplug event, that userspace has not
> - * yet discovered.
> - */
> - ret = -EBUSY;
> - goto out;
> - }
> -
> - if (crtc->funcs->page_flip == NULL)
> - goto out;
> -
> - fb = drm_framebuffer_lookup(dev, page_flip->fb_id);
> - if (!fb) {
> - ret = -ENOENT;
> - goto out;
> - }
> -
> - ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y, &crtc->mode, fb);
> - if (ret)
> - goto out;
> -
> - if (crtc->fb->pixel_format != fb->pixel_format) {
> - DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> - ret = -EINVAL;
> - goto out;
> - }
> +retry:
> + state = dev->driver->atomic_begin(dev,
> + page_flip->flags | DRM_MODE_ATOMIC_NONBLOCK);
> + if (IS_ERR(state))
> + return PTR_ERR(state);
>
> if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT) {
> - ret = -ENOMEM;
> - spin_lock_irqsave(&dev->event_lock, flags);
> - if (file_priv->event_space < sizeof e->event) {
> - spin_unlock_irqrestore(&dev->event_lock, flags);
> + e = create_vblank_event(dev, file_priv, page_flip->user_data);
> + if (!e) {
> + ret = -ENOMEM;
> goto out;
> }
> - file_priv->event_space -= sizeof e->event;
> - spin_unlock_irqrestore(&dev->event_lock, flags);
> -
> - e = kzalloc(sizeof *e, GFP_KERNEL);
> - if (e == NULL) {
> - spin_lock_irqsave(&dev->event_lock, flags);
> - file_priv->event_space += sizeof e->event;
> - spin_unlock_irqrestore(&dev->event_lock, flags);
> + ret = dev->driver->atomic_set_event(dev, state, &crtc->base, e);
> + if (ret) {
> goto out;
> }
> -
> - e->event.base.type = DRM_EVENT_FLIP_COMPLETE;
> - e->event.base.length = sizeof e->event;
> - e->event.user_data = page_flip->user_data;
> - e->base.event = &e->event.base;
> - e->base.file_priv = file_priv;
> - e->base.destroy =
> - (void (*) (struct drm_pending_event *)) kfree;
> }
>
> - old_fb = crtc->fb;
> - ret = crtc->funcs->page_flip(crtc, fb, e, page_flip->flags);
> - if (ret) {
> - if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT) {
> - spin_lock_irqsave(&dev->event_lock, flags);
> - file_priv->event_space += sizeof e->event;
> - spin_unlock_irqrestore(&dev->event_lock, flags);
> - kfree(e);
> - }
> - /* Keep the old fb, don't unref it. */
> - old_fb = NULL;
> - } else {
> - /*
> - * Warn if the driver hasn't properly updated the crtc->fb
> - * field to reflect that the new framebuffer is now used.
> - * Failing to do so will screw with the reference counting
> - * on framebuffers.
> - */
> - WARN_ON(crtc->fb != fb);
> - /* Unref only the old framebuffer. */
> - fb = NULL;
> - }
> + ret = drm_mode_crtc_set_obj_prop(crtc, state,
> + config->prop_fb_id, page_flip->fb_id, NULL);
> + if (ret)
> + goto out;
>
> -out:
> - if (fb)
> - drm_framebuffer_unreference(fb);
> - if (old_fb)
> - drm_framebuffer_unreference(old_fb);
> - drm_modeset_unlock(&crtc->mutex);
> + ret = dev->driver->atomic_check(dev, state);
> + if (ret)
> + goto out;
If atomic_check fails, I think we need to unreference page_flip->fb_id
> +
> + ret = dev->driver->atomic_commit(dev, state);
>
> +out:
> + if (ret && e)
> + destroy_vblank_event(dev, file_priv, e);
> + dev->driver->atomic_end(dev, state);
> + if (ret == -EDEADLK)
> + goto retry;
> return ret;
> }
>
<snip>
next prev parent reply other threads:[~2014-03-04 21:29 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-11-25 14:47 [RFCv4 00/14] Atomic/nuclear modeset/pageflip Rob Clark
2013-11-25 14:47 ` [RFCv4 01/14] drm: add atomic fxns Rob Clark
2013-11-25 14:47 ` [RFCv4 02/14] drm: convert crtc to ww_mutex Rob Clark
2013-11-25 14:47 ` [RFCv4 03/14] drm: add object property type Rob Clark
2013-11-25 14:47 ` [RFCv4 04/14] drm: add signed-range " Rob Clark
2013-11-25 14:47 ` [RFCv4 05/14] drm: helpers to find mode objects Rob Clark
2013-11-25 14:47 ` [RFCv4 06/14] drm: split propvals out and blob property support Rob Clark
2013-11-25 14:47 ` [RFCv4 07/14] drm: Allow drm_mode_object_find() to look up an object of any type Rob Clark
2013-11-25 14:47 ` [RFCv4 08/14] drm: Refactor object property check code Rob Clark
2013-11-25 14:47 ` [RFCv4 09/14] drm: convert plane to properties/state Rob Clark
2014-01-28 21:52 ` Sean Paul
2014-01-29 13:44 ` Rob Clark
2014-02-07 2:53 ` Sean Paul
2014-02-07 12:28 ` Rob Clark
2014-02-10 17:45 ` Sean Paul
2014-02-26 21:30 ` Sean Paul
2014-02-27 0:18 ` Rob Clark
2014-03-03 19:22 ` Sean Paul
2014-03-03 19:40 ` Rob Clark
2014-03-18 15:48 ` Sean Paul
2014-03-18 16:24 ` Rob Clark
2014-03-18 17:33 ` Sean Paul
2013-11-25 14:47 ` [RFCv4 10/14] drm: convert crtc " Rob Clark
2013-12-11 21:48 ` Matt Roper
2013-12-11 23:38 ` Rob Clark
2014-03-04 21:29 ` Sean Paul [this message]
2014-03-04 22:04 ` Rob Clark
2014-03-04 22:36 ` Sean Paul
2013-11-25 14:47 ` [RFCv4 11/14] drm: push locking down into restore_fbdev_mode Rob Clark
2013-11-25 14:47 ` [RFCv4 12/14] drm: Atomic modeset ioctl Rob Clark
2013-11-25 14:47 ` [RFCv4 13/14] drm/msm: add atomic support Rob Clark
2013-11-25 14:47 ` [RFCv4 14/14] HACK: drm: allow FB's in drm_mode_object_find Rob Clark
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAOw6vbJM-7eBU2ZfRpJyfAQjV_vdAXAefUM6xMToNw6TVCUMrA@mail.gmail.com \
--to=seanpaul@chromium.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=robdclark@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.