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..
<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
