On Mon, Nov 25, 2013 at 9:47 AM, Rob Clark <robdclark@xxxxxxxxx> 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>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
