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

Commit Message

Sean Paul March 4, 2014, 10:36 p.m. UTC

On Tue, Mar 4, 2014 at 5:04 PM, Rob Clark <[e-mail]> wrote:
> On Tue, Mar 4, 2014 at 4:29 PM, Sean Paul <[e-mail]> wrote:
>> On Mon, Nov 25, 2013 at 9:47 AM, Rob Clark <[e-mail]> 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:




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

Patch

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;