On Tue, Mar 4, 2014 at 5:04 PM, Rob Clark <robdclark@xxxxxxxxx> wrote:
> On Tue, Mar 4, 2014 at 4:29 PM, Sean Paul <seanpaul@xxxxxxxxxxxx> wrote:
>> 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 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>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
