On Tue, Mar 4, 2014 at 5:04 PM, Rob Clark robdclark@gmail.com wrote:
On Tue, Mar 4, 2014 at 4:29 PM, Sean Paul seanpaul@chromium.org wrote:
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 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?
probably not a bad idea to avoid unnecessary commit, but need to check if that is just masking a refcnt'ing problem. Ie. userspace *should* be allowed to set properties on the crtc (for example, set color correction properties, etc) without causing an unintended finalizing of the fb..
Right, agreed.
Whenever I do a setcrtc on crtc A, I lose a reference to crtc B's current fb (and vice versa). If I short-circuit this code, we don't populate crtc B's cstate in state and the fb is not unreferenced. The alternative would be to take a reference on crtc B's fb here, but that will cause problems if crtc B is meant to be involved in the commit.
I pasted what I have locally below, everything seems well-behaved now:
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 2386ab1..9fe1276 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -485,6 +485,7 @@ void drm_atomic_helper_init_crtc_state(struct drm_crtc *crtc, /* this should never happen.. but make sure! */ WARN_ON(cstate->event); cstate->event = NULL; + cstate->commit_state = false; } EXPORT_SYMBOL(drm_atomic_helper_init_crtc_state);
@@ -631,6 +632,9 @@ drm_atomic_helper_commit_crtc_state(struct drm_crtc *crtc, struct drm_atomic_helper_state *a = cstate->state; int ret = -EINVAL;
+ if (!cstate->commit_state) + return 0; + if (cstate->set_config) return set_config(crtc, cstate);
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 4834dd7..49eda31 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -959,6 +959,7 @@ int drm_crtc_set_property(struct drm_crtc *crtc, struct drm_device *dev = crtc->dev; struct drm_mode_config *config = &dev->mode_config;
+ state->commit_state = true; drm_object_property_set_value(&crtc->base, &state->propvals, property, value, blob_data);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index b81a64c..043ca1e 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -482,6 +482,7 @@ struct drm_crtc_state { bool set_config : 1; bool new_fb : 1; bool connectors_change : 1; + bool commit_state : 1;
uint8_t num_connector_ids; uint32_t *connector_ids;
<snip>
@@ -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
I don't have the branch in front of me (although I'm back in kernel land now, so once I finish up a few other little things, I should be rebasing in next few days).. so my memory could be a bit rusty, but:
I had added cstate->new_fb (set to true whenever we take a ref to the fb), which should probably be using that to decide when to unref when cleaning up applied or unapplied state. But I don't see any other references to new_fb in this patch, so I guess I either lost or forgot something ;-)
BR, -R
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>