From patchwork Tue Mar 4 22:36:40 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Sean Paul X-Patchwork-Id: 3769041 Return-Path: X-Original-To: patchwork-dri-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id E9028BF13A for ; Tue, 4 Mar 2014 22:37:08 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id D0F872022A for ; Tue, 4 Mar 2014 22:37:07 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id A2EF52022F for ; Tue, 4 Mar 2014 22:37:06 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 19C54FA2ED; Tue, 4 Mar 2014 14:37:05 -0800 (PST) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mail-ie0-f171.google.com (mail-ie0-f171.google.com [209.85.223.171]) by gabe.freedesktop.org (Postfix) with ESMTP id 29968FA2ED for ; Tue, 4 Mar 2014 14:37:04 -0800 (PST) Received: by mail-ie0-f171.google.com with SMTP id ar20so230216iec.30 for ; Tue, 04 Mar 2014 14:37:03 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:dkim-signature:mime-version:in-reply-to :references:from:date:message-id:subject:to:cc:content-type; bh=1F7F9fUNu6sO+IEnVcaphU3u6nZmYeVOy7sL6dhkfAY=; b=g0uFrK9RMocvW0UTa1ScDMaMt3Hcp5cr8AJ3+CqmXoNC+/4MFhLXIslRr5Ef5ilb77 2l9Po3iSRnOe7AHZYWv3ClIHwPV/Xk2v+bkUZUYREqNHSLTlK2VErRAhgYHllvaiZ+2d G9oSv58VAWWx7Vi/zQaiz22y6rgTqwSl5/eOAYH2usBe2dJpkHq9lQb20FSNL5FDy62D BR+GwP9kozzE78eekrr/KYpEacC8dx0bPpYu3qrNzGT4C8Kcf2KFXbQb9b7FnjrNBIj1 6Z/+xm6kEjQuKq98mW8aIwU4h0I/OIUfoHvC1nhy/H/4tAFJvpOcsgub+hDySDks3jb/ S8Uw== X-Gm-Message-State: ALoCoQk0r8IoLR9v8oXERImGxuE8fa8vncrLk+Qz7SYReuDY1ANGtI33zzYqjIHq/tiKCJ1jXKnZ X-Received: by 10.43.145.137 with SMTP id ju9mr1929230icc.36.1393972623323; Tue, 04 Mar 2014 14:37:03 -0800 (PST) Received: from mail-ig0-x22e.google.com (mail-ig0-x22e.google.com [2607:f8b0:4001:c05::22e]) by mx.google.com with ESMTPSA id u1sm6857523igm.8.2014.03.04.14.37.01 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Tue, 04 Mar 2014 14:37:01 -0800 (PST) Received: by mail-ig0-f174.google.com with SMTP id h18so11708410igc.1 for ; Tue, 04 Mar 2014 14:37:01 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-type; bh=1F7F9fUNu6sO+IEnVcaphU3u6nZmYeVOy7sL6dhkfAY=; b=nhsLaOBM+LDLxq79whNUkpjGXtBhJ/DNPWfUBaSBdOqxLBLh2sUygeuOF/puDj5i+V oGL0otvt0IxCgmlsTo8jy+DOLFbetvXYlLJQXXkUEQjAVu1YO1lJ1HAlFRSo4pYMjjqI Qd+ejv6ObZ3jT4B5UWX448IM0GbDTJXrWSIsY= X-Received: by 10.50.57.9 with SMTP id e9mr5957092igq.48.1393972620968; Tue, 04 Mar 2014 14:37:00 -0800 (PST) MIME-Version: 1.0 Received: by 10.64.221.103 with HTTP; Tue, 4 Mar 2014 14:36:40 -0800 (PST) In-Reply-To: References: <1385390858-4412-1-git-send-email-robdclark@gmail.com> <1385390858-4412-11-git-send-email-robdclark@gmail.com> From: Sean Paul Date: Tue, 4 Mar 2014 17:36:40 -0500 Message-ID: Subject: Re: [RFCv4 10/14] drm: convert crtc to properties/state To: Rob Clark Cc: dri-devel X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.13 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces@lists.freedesktop.org Errors-To: dri-devel-bounces@lists.freedesktop.org X-Spam-Status: No, score=-4.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_MED,RP_MATCHES_RCVD,T_DKIM_INVALID,UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Tue, Mar 4, 2014 at 5:04 PM, Rob Clark wrote: > On Tue, Mar 4, 2014 at 4:29 PM, Sean Paul wrote: >> On Mon, Nov 25, 2013 at 9:47 AM, Rob Clark 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. >>> --- > > > >>> +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: > > >>> @@ -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; >>> } >>> >> >> 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;