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

Rob Clark robdclark at gmail.com
Tue Mar 4 14:04:13 PST 2014


On Tue, Mar 4, 2014 at 4:29 PM, Sean Paul <seanpaul at chromium.org> wrote:
> On Mon, Nov 25, 2013 at 9:47 AM, Rob Clark <robdclark at 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..

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


More information about the dri-devel mailing list