Re: [RFCv4 10/14] drm: convert crtc to properties/state

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>