From patchwork Mon Mar 3 19:22:50 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Sean Paul X-Patchwork-Id: 3757021 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 DCFCCBF13A for ; Mon, 3 Mar 2014 19:23:24 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 82835203ED for ; Mon, 3 Mar 2014 19:23:23 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id 291B7202EB for ; Mon, 3 Mar 2014 19:23:22 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 446F0FBFF0; Mon, 3 Mar 2014 11:23:20 -0800 (PST) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mail-ig0-f174.google.com (mail-ig0-f174.google.com [209.85.213.174]) by gabe.freedesktop.org (Postfix) with ESMTP id D3CCAFBFF0 for ; Mon, 3 Mar 2014 11:23:17 -0800 (PST) Received: by mail-ig0-f174.google.com with SMTP id h18so8466775igc.1 for ; Mon, 03 Mar 2014 11:23:17 -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=IzruZmCsqZgxsLIi7uSOsYRWjw+pR5EOvTrUKFtkjyg=; b=RufWRIxiEffNSs/AV9Xfpr773rJ5/uqmA5a/o8ILBFRnOboYPxHwEbPHTmymvieKrU yt+9in+nv5bjvAs2RBHaLTREDjH5NDasVKjqR4RI/558tD0iYTRjToEw/KGvbOQFhpzh 8t2RQyQxMu5eK21B9twn8Y6MYyfE+NpHiF80f2eTQWlZT+9d7zHwpXOp1x0l1JPZFw0y FcPdJlcDTa8ULWyCB6HBtu5doZRYBRz1zvxMZGa9qxZU70QPR0jaZhrWwId4CaQWz2D4 lsmiud7R7X/Ukbj2fcU/NKoYMJgbhD6k3UOdCw/Nw3m+yk3yfvQtvka7sHGIO2kxBqom m5AA== X-Gm-Message-State: ALoCoQkCwvlPImx//Z0tsYPOBQlSwP3xFPfsR7xqD4DbUw/7Honr8czrSmzcUIt3qyazNLUvyeE9 X-Received: by 10.43.65.145 with SMTP id xm17mr28329869icb.35.1393874593179; Mon, 03 Mar 2014 11:23:13 -0800 (PST) Received: from mail-ie0-x229.google.com (mail-ie0-x229.google.com [2607:f8b0:4001:c03::229]) by mx.google.com with ESMTPSA id m6sm41973906igx.9.2014.03.03.11.23.11 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Mon, 03 Mar 2014 11:23:11 -0800 (PST) Received: by mail-ie0-f169.google.com with SMTP id to1so349500ieb.14 for ; Mon, 03 Mar 2014 11:23:10 -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=IzruZmCsqZgxsLIi7uSOsYRWjw+pR5EOvTrUKFtkjyg=; b=J8XOOXtyYcPfrhIlF6s2ZO51Wb1yRC/8J9HCZrotxtuQAOQpXgJx0bUJfCxDP5Cjlo PePszlDH2HUVv2xYEN5sJ4RfJ1jRzDsNUeYNTYMJpVJ/QxNX6H5ew/KLmTnD/+Ywqidn wwMBMipQLh/Ax1kHJFM/AUt3FPoNp1CuhbSPU= X-Received: by 10.42.62.196 with SMTP id z4mr27296385ich.49.1393874590531; Mon, 03 Mar 2014 11:23:10 -0800 (PST) MIME-Version: 1.0 Received: by 10.64.221.103 with HTTP; Mon, 3 Mar 2014 11:22:50 -0800 (PST) In-Reply-To: References: <1385390858-4412-1-git-send-email-robdclark@gmail.com> <1385390858-4412-10-git-send-email-robdclark@gmail.com> From: Sean Paul Date: Mon, 3 Mar 2014 14:22:50 -0500 Message-ID: Subject: Re: [RFCv4 09/14] drm: convert plane 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 Wed, Feb 26, 2014 at 7:18 PM, Rob Clark wrote: > On Wed, Feb 26, 2014 at 4:30 PM, Sean Paul wrote: >> On Mon, Nov 25, 2013 at 9:47 AM, Rob Clark wrote: >>> Break the mutable state of a plane out into a separate structure >>> and use atomic properties mechanism to set plane attributes. This >>> makes it easier to have some helpers for plane->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. >>> >>> The same should be done for CRTC, encoder, and connector, but this >>> patch only includes the first part (plane). >> >> >> Hi Rob, >> I've been tracking down a crash that came up on my exynos board when I >> applied this patch. I hope you can hold my hand a little bit and give >> some guidance. >> >> >> >>> +static int >>> +drm_atomic_helper_commit_plane_state(struct drm_plane *plane, >>> + struct drm_plane_state *pstate) >>> +{ >>> + struct drm_framebuffer *old_fb = NULL, *fb = NULL; >>> + int ret = 0; >>> + >>> + fb = pstate->fb; >>> + >>> + if (pstate->crtc && fb) { >>> + ret = plane->funcs->update_plane(plane, pstate->crtc, pstate->fb, >>> + pstate->crtc_x, pstate->crtc_y, pstate->crtc_w, pstate->crtc_h, >>> + pstate->src_x, pstate->src_y, pstate->src_w, pstate->src_h); >>> + if (!ret) { >>> + /* on success, update state and fb refcnting: */ >>> + /* NOTE: if we ensure no driver sets plane->state->fb = NULL >>> + * on disable, we can move this up a level and not duplicate >>> + * nearly the same thing for both update_plane and disable_plane >>> + * cases.. I leave it like this for now to be paranoid due to >>> + * the slightly different ordering in the two cases in the >>> + * original code. >>> + */ >>> + old_fb = plane->state->fb; >> >> I think this is slightly different than what we had before in >> setplane. In the previous code, we had: >> >> ret = plane->funcs->update_plane(plane, crtc, fb, >> plane_req->crtc_x, >> plane_req->crtc_y, >> plane_req->crtc_w, >> plane_req->crtc_h, >> plane_req->src_x, >> plane_req->src_y, >> plane_req->src_w, >> plane_req->src_h); >> if (!ret) { >> old_fb = plane->fb; >> fb = NULL; >> plane->crtc = crtc; >> plane->fb = fb; >> } >> >> In this case, we'd unreference old_fb which is the previous plane->fb. >> AFAICT, update_plane did not set plane->fb to fb, so it would, in >> fact, be the old plane. > > to be honest, I need to dig up that branch again and remember (and > rebase it while I'm at it).. > > I don't think the driver should be changing state->fb (ie. the state > object should in theory, once constructed, be a sort of constant > thing). So until swap_plane_state() plane->state->fb is *supposed* to > be the old fb. > Hey Rob, Sorry for the delay getting back to you. Indeed, I was confused in my last email, thanks for straightening me out. I traced through things a little more carefully and it looks like my fb is being unreferenced every time we call set_property on the plane. On exynos, we set a private zpos property via the set_property ioctl. In this case, drm_mode_set_property_ioctl does not take a reference on the plane's current fb, but we put a reference in atomic_commit (assuming it's got a valid crtc/fb). Eventually, this runs the refcount down to 0 and we end up freeing the fb early. I think the fix here is to only unreference the plane's current fb in the case where new_fb is true. ie: fb = NULL; } Seem reasonable? Sean > For crtc, as there were so many places in code accessing crtc->fb (and > it was pretty intertwined in the crtc helpers), the way I left it as: > crtc->state->fb is what is requested by userspace, and crtc->fb is > what is currently used (which is updated to the new cstate->fb in the > course of modeset). > > I don't remember if I left it the same way for plane (which isn't > referenced in nearly as many places, and which doesn't have a big > chunk of modeset helper to care about). > >> In the new code, drm_mode_setplane calls drm_mode_plane_set_obj_prop >> on fb_id, which will update plane->state->fb. Then we come in here and >> unreference it as old_fb. I _believe_ we're taking one more reference >> than we need in this case. > > well, it should be updating 'pstate', what will become the new > plane->state.. But not the current plane->state. > > BR, > -R > >> What I'm seeing in exynos is that when we setplane to an fb, we leave >> it with refcount == 1. If we disable that plane, we'll free the fb. >> Unfortunately, this leaves the fb in the fbs list and the next time we >> try to iterate that list Bad Things Happen. >> >> Does this make any sense? >> >> Sean >> >>> + swap_plane_state(plane, pstate->state); >>> + fb = NULL; >>> + } >>> + } else { >>> + old_fb = plane->state->fb; >>> + plane->funcs->disable_plane(plane); >>> + swap_plane_state(plane, pstate->state); >>> + } >>> + >>> + >>> + if (fb) >>> + drm_framebuffer_unreference(fb); >>> + if (old_fb) >>> + drm_framebuffer_unreference(old_fb); >>> + >>> + return ret; >>> +} >>> >> >> >> >>> +int drm_plane_set_property(struct drm_plane *plane, >>> + struct drm_plane_state *state, >>> + struct drm_property *property, >>> + uint64_t value, void *blob_data) >>> +{ >>> + struct drm_device *dev = plane->dev; >>> + struct drm_mode_config *config = &dev->mode_config; >>> + >>> + drm_object_property_set_value(&plane->base, >>> + &state->propvals, property, value, blob_data); >>> + >>> + if (property == config->prop_fb_id) { >>> + state->new_fb = true; >>> + state->fb = drm_framebuffer_lookup(dev, value); >>> + } else if (property == config->prop_crtc_id) { >>> + struct drm_mode_object *obj = drm_property_get_obj(property, value); >>> + struct drm_crtc *crtc = obj ? obj_to_crtc(obj) : NULL; >>> + /* take the lock of the incoming crtc as well, moving >>> + * plane between crtcs is synchronized on both incoming >>> + * and outgoing crtc. >>> + */ >>> + if (crtc) { >>> + int ret = drm_modeset_lock(&crtc->mutex, state->state); >>> + if (ret) >>> + return ret; >>> + } >>> + state->crtc = crtc; >>> + } else if (property == config->prop_crtc_x) { >>> + state->crtc_x = U642I64(value); >>> + } else if (property == config->prop_crtc_y) { >>> + state->crtc_y = U642I64(value); >>> + } else if (property == config->prop_crtc_w) { >>> + state->crtc_w = value; >>> + } else if (property == config->prop_crtc_h) { >>> + state->crtc_h = value; >>> + } else if (property == config->prop_src_x) { >>> + state->src_x = value; >>> + } else if (property == config->prop_src_y) { >>> + state->src_y = value; >>> + } else if (property == config->prop_src_w) { >>> + state->src_w = value; >>> + } else if (property == config->prop_src_h) { >>> + state->src_h = value; >>> + } else { >>> + return -EINVAL; >>> + } >>> + >>> + return 0; >>> +} >>> +EXPORT_SYMBOL(drm_plane_set_property); >>> + >> >> >> >>> @@ -1987,20 +2192,19 @@ int drm_mode_setplane(struct drm_device *dev, void *data, >>> struct drm_file *file_priv) >>> { >>> struct drm_mode_set_plane *plane_req = data; >>> + struct drm_mode_config *config = &dev->mode_config; >>> struct drm_plane *plane; >>> - struct drm_crtc *crtc; >>> - struct drm_framebuffer *fb = NULL, *old_fb = NULL; >>> + void *state; >>> int ret = 0; >>> - unsigned int fb_width, fb_height; >>> - int i; >>> >>> if (!drm_core_check_feature(dev, DRIVER_MODESET)) >>> return -EINVAL; >>> >>> - /* >>> - * First, find the plane, crtc, and fb objects. If not available, >>> - * we don't bother to call the driver. >>> - */ >>> +retry: >>> + state = dev->driver->atomic_begin(dev, 0); >>> + if (IS_ERR(state)) >>> + return PTR_ERR(state); >>> + >>> plane = drm_plane_find(dev, plane_req->plane_id); >>> if (!plane) { >>> DRM_DEBUG_KMS("Unknown plane ID %d\n", >>> @@ -2008,98 +2212,37 @@ int drm_mode_setplane(struct drm_device *dev, void *data, >>> return -ENOENT; >>> } >>> >>> - /* No fb means shut it down */ >>> - if (!plane_req->fb_id) { >>> - drm_modeset_lock_all(dev); >>> - old_fb = plane->fb; >>> - plane->funcs->disable_plane(plane); >>> - plane->crtc = NULL; >>> - plane->fb = NULL; >>> - drm_modeset_unlock_all(dev); >>> - goto out; >>> - } >>> - >>> - crtc = drm_crtc_find(dev, plane_req->crtc_id); >>> - if (!crtc) { >>> - DRM_DEBUG_KMS("Unknown crtc ID %d\n", >>> - plane_req->crtc_id); >>> - ret = -ENOENT; >>> - goto out; >>> - } >>> - >>> - fb = drm_framebuffer_lookup(dev, plane_req->fb_id); >>> - if (!fb) { >>> - DRM_DEBUG_KMS("Unknown framebuffer ID %d\n", >>> - plane_req->fb_id); >>> - ret = -ENOENT; >>> - goto out; >>> - } >>> - >>> - /* Check whether this plane supports the fb pixel format. */ >>> - for (i = 0; i < plane->format_count; i++) >>> - if (fb->pixel_format == plane->format_types[i]) >>> - break; >>> - if (i == plane->format_count) { >>> - DRM_DEBUG_KMS("Invalid pixel format %s\n", >>> - drm_get_format_name(fb->pixel_format)); >>> - ret = -EINVAL; >>> - goto out; >>> - } >>> - >>> - fb_width = fb->width << 16; >>> - fb_height = fb->height << 16; >>> - >>> - /* Make sure source coordinates are inside the fb. */ >>> - if (plane_req->src_w > fb_width || >>> - plane_req->src_x > fb_width - plane_req->src_w || >>> - plane_req->src_h > fb_height || >>> - plane_req->src_y > fb_height - plane_req->src_h) { >>> - DRM_DEBUG_KMS("Invalid source coordinates " >>> - "%u.%06ux%u.%06u+%u.%06u+%u.%06u\n", >>> - plane_req->src_w >> 16, >>> - ((plane_req->src_w & 0xffff) * 15625) >> 10, >>> - plane_req->src_h >> 16, >>> - ((plane_req->src_h & 0xffff) * 15625) >> 10, >>> - plane_req->src_x >> 16, >>> - ((plane_req->src_x & 0xffff) * 15625) >> 10, >>> - plane_req->src_y >> 16, >>> - ((plane_req->src_y & 0xffff) * 15625) >> 10); >>> - ret = -ENOSPC; >>> - goto out; >>> - } >>> - >>> - /* Give drivers some help against integer overflows */ >>> - if (plane_req->crtc_w > INT_MAX || >>> - plane_req->crtc_x > INT_MAX - (int32_t) plane_req->crtc_w || >>> - plane_req->crtc_h > INT_MAX || >>> - plane_req->crtc_y > INT_MAX - (int32_t) plane_req->crtc_h) { >>> - DRM_DEBUG_KMS("Invalid CRTC coordinates %ux%u+%d+%d\n", >>> - plane_req->crtc_w, plane_req->crtc_h, >>> - plane_req->crtc_x, plane_req->crtc_y); >>> - ret = -ERANGE; >>> + ret = >>> + drm_mode_plane_set_obj_prop(plane, state, >>> + config->prop_crtc_id, plane_req->crtc_id, NULL) || >>> + drm_mode_plane_set_obj_prop(plane, state, >>> + config->prop_fb_id, plane_req->fb_id, NULL) || >>> + drm_mode_plane_set_obj_prop(plane, state, >>> + config->prop_crtc_x, I642U64(plane_req->crtc_x), NULL) || >>> + drm_mode_plane_set_obj_prop(plane, state, >>> + config->prop_crtc_y, I642U64(plane_req->crtc_y), NULL) || >>> + drm_mode_plane_set_obj_prop(plane, state, >>> + config->prop_crtc_w, plane_req->crtc_w, NULL) || >>> + drm_mode_plane_set_obj_prop(plane, state, >>> + config->prop_crtc_h, plane_req->crtc_h, NULL) || >>> + drm_mode_plane_set_obj_prop(plane, state, >>> + config->prop_src_w, plane_req->src_w, NULL) || >>> + drm_mode_plane_set_obj_prop(plane, state, >>> + config->prop_src_h, plane_req->src_h, NULL) || >>> + drm_mode_plane_set_obj_prop(plane, state, >>> + config->prop_src_x, plane_req->src_x, NULL) || >>> + drm_mode_plane_set_obj_prop(plane, state, >>> + config->prop_src_y, plane_req->src_y, NULL) || >>> + dev->driver->atomic_check(dev, state); >>> + if (ret) >>> goto out; >>> - } >>> >>> - drm_modeset_lock_all(dev); >>> - ret = plane->funcs->update_plane(plane, crtc, fb, >>> - plane_req->crtc_x, plane_req->crtc_y, >>> - plane_req->crtc_w, plane_req->crtc_h, >>> - plane_req->src_x, plane_req->src_y, >>> - plane_req->src_w, plane_req->src_h); >>> - if (!ret) { >>> - old_fb = plane->fb; >>> - plane->crtc = crtc; >>> - plane->fb = fb; >>> - fb = NULL; >>> - } >>> - drm_modeset_unlock_all(dev); >>> + ret = dev->driver->atomic_commit(dev, state); >>> >>> out: >>> - if (fb) >>> - drm_framebuffer_unreference(fb); >>> - if (old_fb) >>> - drm_framebuffer_unreference(old_fb); >>> - >>> + 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 14e0571..ec60d4e 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -394,7 +394,8 @@ drm_atomic_helper_commit_plane_state(struct drm_plane *plane, * the slightly different ordering in the two cases in the * original code. */ - old_fb = plane->state->fb; + if (pstate->new_fb) + old_fb = plane->state->fb; swap_plane_state(plane, pstate->state);